Message ID | 20230324093644.464704-5-maxime.chevallier@bootlin.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp495013vqo; Fri, 24 Mar 2023 02:45:10 -0700 (PDT) X-Google-Smtp-Source: AKy350Y5ZxF7/XbOsg2LAqEaWY1XNbuJju5vdaCzHeND1eV6OY1QpHIbt8oq9RTqOii6YCbeWzQr X-Received: by 2002:a17:906:3a41:b0:931:8e8c:2db5 with SMTP id a1-20020a1709063a4100b009318e8c2db5mr2019418ejf.69.1679651109833; Fri, 24 Mar 2023 02:45:09 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679651109; cv=none; d=google.com; s=arc-20160816; b=QLf4ldpNCgTnFZ2XntCYJXTdGULpzN/zT4fZEsKhb9QXkHuOHHHuE3eNoQZeMOLsuH rPHkBqkOoadSq7HLtYecdZxllvOO2rhltM1avleIxXi+jC4o6YuaRWaGZsx3v/C/CAHZ tM/J9lr+e22VypFzw0YVkdcw3+rZGos61Cms07B64mk5pB5K3saWG9FFN1FkG6FZkX/5 R158msPGfKmaMyN7VRUqiYGW1GDzfF2ox9DY58VSYiasuO7E0LPm08ePe7PjXbQoIqxN IHcsDHm2DwZJRSxLMWy+sV1BmznO2IXDvHvr7p5uFhsvOO69N3Cp4Nj5LHFHibd4uqJS Qa5A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=PjAcdhdUxcOsTOcGYGylxKe370xV/9wJPyxiOPxbjJQ=; b=Jw5kRe2S8jLo023BeNKMtsUyT53mHqfiOctN1fcuvYMLLANMVCYtcqrpquqnn+8rNs WiHffkXsgDSGPL8WXJTM0is/VSVjuRQQs1ft2UhxhURnLpaLw9uhVWOKDn5jya+LlXAo hEeMPWyQkVOQ7TYQ6P4ZbtHx/OueSCQAtD75YWZKITc9m/MAT6kcNXo26/B9MqR4E7jN AB5KeSIVq1foG6qja+wlJRvXlVUO3V/8A4KeTmTFskKHxpjY0fL5wbEx04FWUCFCNy45 pZpxp2jioi0ECPHgaNVohar7ej7wjes/cR0scRsn+aOagi3Z+WGfMaqsIxIIk3nVFPLc X5dQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=aD7qwCNe; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id ti9-20020a170907c20900b0093d1bbf9fdcsi3456452ejc.926.2023.03.24.02.44.46; Fri, 24 Mar 2023 02:45:09 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=aD7qwCNe; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231839AbjCXJhk (ORCPT <rfc822;ezelljr.billy@gmail.com> + 99 others); Fri, 24 Mar 2023 05:37:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46630 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231824AbjCXJha (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 24 Mar 2023 05:37:30 -0400 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [IPv6:2001:4b98:dc4:8::231]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 086CD2596C; Fri, 24 Mar 2023 02:37:11 -0700 (PDT) Received: (Authenticated sender: maxime.chevallier@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id 902EF100007; Fri, 24 Mar 2023 09:37:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1679650630; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PjAcdhdUxcOsTOcGYGylxKe370xV/9wJPyxiOPxbjJQ=; b=aD7qwCNeffDbZvrw87ON/4Mt8iFPClmi2/+tKAwBZZmLe+K46FDsiAApGI5eDGo3psk9tB XojG9bMLW7hZrFLpDOdHywmG2hVDGbtcktohEHjX7mlNd0moM4T1metH7P5MYrwnBbkn7W rAOhJvnie7dXxkGDQqePWgx6LKj1X+eFOdUdQG5X9Pq+RfSkI2fGxyTKqXHoILYg440PID AC2M0aIFq5J3hbR/wdld0ZasvVgzYptp6fMZ1jKfay2GMFjlWsVUAggbnKnqoH7pi0hpXN 8jyB5xCFZXrosWTqvEgcgf0j9SEu6RT3VBooWtrIeCIYRmVGsZ6YF9ZHB+vPVw== From: Maxime Chevallier <maxime.chevallier@bootlin.com> To: Mark Brown <broonie@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, rafael@kernel.org, Colin Foster <colin.foster@in-advantage.com>, Vladimir Oltean <vladimir.oltean@nxp.com>, Lee Jones <lee@kernel.org>, davem@davemloft.net, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk> Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, thomas.petazzoni@bootlin.com Subject: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one Date: Fri, 24 Mar 2023 10:36:41 +0100 Message-Id: <20230324093644.464704-5-maxime.chevallier@bootlin.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230324093644.464704-1-maxime.chevallier@bootlin.com> References: <20230324093644.464704-1-maxime.chevallier@bootlin.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761241841833107856?= X-GMAIL-MSGID: =?utf-8?q?1761241841833107856?= |
Series |
Introduce a generic regmap-based MDIO driver
|
|
Commit Message
Maxime Chevallier
March 24, 2023, 9:36 a.m. UTC
When used over SPI, the register addresses needs to be translated,
compared to when used over MMIO. The translation consists in applying an
offset with reg_base, then downshifting the registers by 2. This
actually changes the register stride from 4 to 1.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/mfd/ocelot-spi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote: > When used over SPI, the register addresses needs to be translated, > compared to when used over MMIO. The translation consists in applying an > offset with reg_base, then downshifting the registers by 2. This > actually changes the register stride from 4 to 1. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > --- > drivers/mfd/ocelot-spi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c > index 2d1349a10ca9..107cda0544aa 100644 > --- a/drivers/mfd/ocelot-spi.c > +++ b/drivers/mfd/ocelot-spi.c > @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device *dev) > > static const struct regmap_config ocelot_spi_regmap_config = { > .reg_bits = 24, > - .reg_stride = 4, > + .reg_stride = 1, > .reg_shift = REGMAP_DOWNSHIFT(2), > .val_bits = 32, This does not look like a bisectable change? Or did it never work before? Andrew
Hello Andrew, On Fri, 24 Mar 2023 13:11:07 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote: > > When used over SPI, the register addresses needs to be translated, > > compared to when used over MMIO. The translation consists in > > applying an offset with reg_base, then downshifting the registers > > by 2. This actually changes the register stride from 4 to 1. > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > --- > > drivers/mfd/ocelot-spi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c > > index 2d1349a10ca9..107cda0544aa 100644 > > --- a/drivers/mfd/ocelot-spi.c > > +++ b/drivers/mfd/ocelot-spi.c > > @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device > > *dev) > > static const struct regmap_config ocelot_spi_regmap_config = { > > .reg_bits = 24, > > - .reg_stride = 4, > > + .reg_stride = 1, > > .reg_shift = REGMAP_DOWNSHIFT(2), > > .val_bits = 32, > > This does not look like a bisectable change? Or did it never work > before? Actually this works in all cases because of "regmap: check for alignment on translated register addresses" in this series. Before this series, I think using a stride of 1 would have worked too, as any 4-byte-aligned accesses are also 1-byte aligned. But that's also why I need review on this, my understanding is that reg_stride is used just as a check for alignment, and I couldn't test this ocelot-related patch on the real HW, so please take it with a grain of salt :( Thanks, Maxime > Andrew
Hi Maxime, On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote: > Hello Andrew, > > On Fri, 24 Mar 2023 13:11:07 +0100 > Andrew Lunn <andrew@lunn.ch> wrote: > > > On Fri, Mar 24, 2023 at 10:36:41AM +0100, Maxime Chevallier wrote: > > > When used over SPI, the register addresses needs to be translated, > > > compared to when used over MMIO. The translation consists in > > > applying an offset with reg_base, then downshifting the registers > > > by 2. This actually changes the register stride from 4 to 1. > > > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > --- > > > drivers/mfd/ocelot-spi.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c > > > index 2d1349a10ca9..107cda0544aa 100644 > > > --- a/drivers/mfd/ocelot-spi.c > > > +++ b/drivers/mfd/ocelot-spi.c > > > @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device > > > *dev) > > > static const struct regmap_config ocelot_spi_regmap_config = { > > > .reg_bits = 24, > > > - .reg_stride = 4, > > > + .reg_stride = 1, > > > .reg_shift = REGMAP_DOWNSHIFT(2), > > > .val_bits = 32, > > > > This does not look like a bisectable change? Or did it never work > > before? > > Actually this works in all cases because of "regmap: check for alignment > on translated register addresses" in this series. Before this series, > I think using a stride of 1 would have worked too, as any 4-byte-aligned > accesses are also 1-byte aligned. > > But that's also why I need review on this, my understanding is that > reg_stride is used just as a check for alignment, and I couldn't test > this ocelot-related patch on the real HW, so please take it with a > grain of salt :( You're exactly right. reg_stride wasn't used anywhere in the ocelot-spi path before this patch series. When I build against patch 3 ("regmap: allow upshifting register addresses before performing operations") ocelot-spi breaks. [ 3.207711] ocelot-soc spi0.0: error -EINVAL: Error initializing SPI bus When I build against the whole series, or even just up to patch 4 ("mfd: ocelot-spi: Change the regmap stride to reflect the real one") functionality returns. If you keep patch 4 and apply it before patch 2, everything should work. Sorry for the bug. Thanks for the fix. And I'm glad I'm not the only one taking advantage of the "reg_shift" regmap operation! I thought I'd be the only one. Let me know if you want me to take any action on this fix. Colin
On Fri, Mar 24, 2023 at 08:48:18AM -0700, Colin Foster wrote: > Hi Maxime, > > On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote: > > Hello Andrew, > > > > On Fri, 24 Mar 2023 13:11:07 +0100 > > Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > .reg_bits = 24, > > > > - .reg_stride = 4, > > > > + .reg_stride = 1, > > > > .reg_shift = REGMAP_DOWNSHIFT(2), > > > > .val_bits = 32, > > > > > > This does not look like a bisectable change? Or did it never work > > > before? > > > > Actually this works in all cases because of "regmap: check for alignment > > on translated register addresses" in this series. Before this series, > > I think using a stride of 1 would have worked too, as any 4-byte-aligned > > accesses are also 1-byte aligned. > > > > But that's also why I need review on this, my understanding is that > > reg_stride is used just as a check for alignment, and I couldn't test > > this ocelot-related patch on the real HW, so please take it with a > > grain of salt :( > > You're exactly right. reg_stride wasn't used anywhere in the > ocelot-spi path before this patch series. When I build against patch 3 > ("regmap: allow upshifting register addresses before performing > operations") ocelot-spi breaks. > > [ 3.207711] ocelot-soc spi0.0: error -EINVAL: Error initializing SPI bus > > When I build against the whole series, or even just up to patch 4 ("mfd: > ocelot-spi: Change the regmap stride to reflect the real one") > functionality returns. > > If you keep patch 4 and apply it before patch 2, everything should > work. I replied too soon, before looking more into patch 2. Some context from that patch: --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -2016,7 +2016,7 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val) { int ret; - if (!IS_ALIGNED(reg, map->reg_stride)) + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) return -EINVAL; map->lock(map->lock_arg); I don't know whether checking IS_ALIGNED before or after the shift is the right thing to do. My initial intention was to perform the shift at the last possible moment before calling into the read / write routines. That way it wouldn't interfere with any underlying regcache mechanisms (which aren't used by ocelot-spi) But to me it seems like patch 2 changes this expected behavior, so the two patches should be squashed. ... Thinking more about it ... In ocelot-spi, at the driver layer, we're accessing two registers. They'd be at address 0x71070000 and 0x71070004. The driver uses those addresses, so there's a stride of 4. I can't access 0x71070001. The fact that the translation from "address" to "bits that go out the SPI bus" shifts out the last two bits and hacks off a couple of the MSBs doesn't seem like it should affect the 'reg_stride'. So maybe patches 2 and 4 should be dropped, and your patch 6 alterra_tse_main should use a reg_stride of 1? That has a subtle benefit of not needing an additional operation or two from regmap_reg_addr(). Would that cause any issues? Hopefully there isn't something I'm missing. (Aside: I'm now curious how the compiler will optimize regmap_reg_addr()) Colin
> > > static const struct regmap_config ocelot_spi_regmap_config = { > > > .reg_bits = 24, > > > - .reg_stride = 4, > > > + .reg_stride = 1, > > > .reg_shift = REGMAP_DOWNSHIFT(2), > > > .val_bits = 32, > > > > This does not look like a bisectable change? Or did it never work > > before? > > Actually this works in all cases because of "regmap: check for alignment > on translated register addresses" in this series. Before this series, > I think using a stride of 1 would have worked too, as any 4-byte-aligned > accesses are also 1-byte aligned. This is the sort of think which is good to explain in the commit message. That is the place to answer questions reviewers are likely to ask for things which are not obvious from just the patch. Andrew
Hello Andrew, On Mon, 27 Mar 2023 02:02:55 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > > > static const struct regmap_config ocelot_spi_regmap_config = { > > > > .reg_bits = 24, > > > > - .reg_stride = 4, > > > > + .reg_stride = 1, > > > > .reg_shift = REGMAP_DOWNSHIFT(2), > > > > .val_bits = 32, > > > > > > This does not look like a bisectable change? Or did it never work > > > before? > > > > Actually this works in all cases because of "regmap: check for > > alignment on translated register addresses" in this series. Before > > this series, I think using a stride of 1 would have worked too, as > > any 4-byte-aligned accesses are also 1-byte aligned. > > This is the sort of think which is good to explain in the commit > message. That is the place to answer questions reviewers are likely to > ask for things which are not obvious from just the patch. That's right, I will include this explanation in the next iteration. Thanks for the review, Maxime > Andrew
On Fri, 24 Mar 2023 10:56:05 -0700 Colin Foster <colin.foster@in-advantage.com> wrote: > On Fri, Mar 24, 2023 at 08:48:18AM -0700, Colin Foster wrote: > > Hi Maxime, > > > > On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote: > > > Hello Andrew, > > > > > > On Fri, 24 Mar 2023 13:11:07 +0100 > > > Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > .reg_bits = 24, > > > > > - .reg_stride = 4, > > > > > + .reg_stride = 1, > > > > > .reg_shift = REGMAP_DOWNSHIFT(2), > > > > > .val_bits = 32, > > > > > > > > This does not look like a bisectable change? Or did it never > > > > work before? > > > > > > Actually this works in all cases because of "regmap: check for > > > alignment on translated register addresses" in this series. > > > Before this series, I think using a stride of 1 would have worked > > > too, as any 4-byte-aligned accesses are also 1-byte aligned. > > > > > > But that's also why I need review on this, my understanding is > > > that reg_stride is used just as a check for alignment, and I > > > couldn't test this ocelot-related patch on the real HW, so please > > > take it with a grain of salt :( > > > > You're exactly right. reg_stride wasn't used anywhere in the > > ocelot-spi path before this patch series. When I build against > > patch 3 ("regmap: allow upshifting register addresses before > > performing operations") ocelot-spi breaks. > > > > [ 3.207711] ocelot-soc spi0.0: error -EINVAL: Error initializing > > SPI bus > > > > When I build against the whole series, or even just up to patch 4 > > ("mfd: ocelot-spi: Change the regmap stride to reflect the real > > one") functionality returns. > > > > If you keep patch 4 and apply it before patch 2, everything should > > work. > > I replied too soon, before looking more into patch 2. > > Some context from that patch: > > --- a/drivers/base/regmap/regmap.c > +++ b/drivers/base/regmap/regmap.c > @@ -2016,7 +2016,7 @@ int regmap_write(struct regmap *map, unsigned > int reg, unsigned int val) { > int ret; > > - if (!IS_ALIGNED(reg, map->reg_stride)) > + if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride)) > return -EINVAL; > > map->lock(map->lock_arg); > > > I don't know whether checking IS_ALIGNED before or after the shift is > the right thing to do. My initial intention was to perform the shift > at the last possible moment before calling into the read / write > routines. That way it wouldn't interfere with any underlying regcache > mechanisms (which aren't used by ocelot-spi) > > But to me it seems like patch 2 changes this expected behavior, so the > two patches should be squashed. > > > ... Thinking more about it ... > > > In ocelot-spi, at the driver layer, we're accessing two registers. > They'd be at address 0x71070000 and 0x71070004. The driver uses those > addresses, so there's a stride of 4. I can't access 0x71070001. > > The fact that the translation from "address" to "bits that go out the > SPI bus" shifts out the last two bits and hacks off a couple of the > MSBs doesn't seem like it should affect the 'reg_stride'. > > > So maybe patches 2 and 4 should be dropped, and your patch 6 > alterra_tse_main should use a reg_stride of 1? That has a subtle > benefit of not needing an additional operation or two from > regmap_reg_addr(). > > Would that cause any issues? Hopefully there isn't something I'm > missing. Well here I guess it's also about the semantic of reg_stride. Should it represent the alignment constraints of the register address we feed as an input to a regmap_read/regmap_write operation, or the alignment constraints of the underlying bus ? This is kind of a new concern, as we are now translating register addresses. I asked myself the same question, so I'm very open for discussion, but my gut feeling is that the reg_stride is there to make sure we don't perform an access whose alignment won't work with the bus we are using, so using a stride of 1 on a memory-mapped device with 2 or 4 byte register alignment is a bit counter-intuitive. Thanks a lot for the review, suggestions and tests ! Best regards, Maxime > > (Aside: I'm now curious how the compiler will optimize > regmap_reg_addr()) > > > Colin
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c index 2d1349a10ca9..107cda0544aa 100644 --- a/drivers/mfd/ocelot-spi.c +++ b/drivers/mfd/ocelot-spi.c @@ -124,7 +124,7 @@ static int ocelot_spi_initialize(struct device *dev) static const struct regmap_config ocelot_spi_regmap_config = { .reg_bits = 24, - .reg_stride = 4, + .reg_stride = 1, .reg_shift = REGMAP_DOWNSHIFT(2), .val_bits = 32,