Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic
From: Yong Li
Date: Thu Aug 17 2017 - 10:55:08 EST
Thanks Andrew for your thoughtful input. Let me send out another version :)
2017-08-16 20:19 GMT-07:00 Andrew Jeffery <andrew@xxxxxxxx>:
> Hi Yong,
>
> On Wed, 2017-08-16 at 08:05 -0700, Yong Li wrote:
>> Hi Andrew,
>>
>> Thanks for your review. I checked the patch before I sent out, but the
>> tool did not report any problems. Could you help to share your
>> checking commands?
>>
>> scripts/checkpatch.pl
>> 0001-pinctrl-aspeed-Fix-ast2500-strap-register-write-logi.patch
>> total: 0 errors, 0 warnings, 38 lines checked
>
> Gah, turns out it was a problem with my mail client, which violates RFC4155 and
> writes an mbox with CRLF. Awesome.
>
> I grabbed the mbox from patchwork and it was fine. Sorry for the noise.
>
> Separately, I slept on this and think there's a remaining issue. I'll outline
> it below in context.
>
>>
>> Thanks,
>> Yong
>>
>> > 2017-08-16 6:45 GMT-07:00 Andrew Jeffery <andrew@xxxxxxxx>:
>> > Hi Yong,
>> >
>> > On Wed, 2017-08-16 at 00:21 +0800, Yong Li wrote:
>> > > On AST2500, the hardware strap register(SCU70) only accepts write â1â,
>> > > to clear it to â0â, must set bits(write â1â) to SCU7C
>> > >
>> > > Signed-off-by: Yong Li <sdliyong@xxxxxxxxx>
>> >
>> > ./scripts/checkpatch.pl complains about DOS line-endings thoughout -
>> > you should fix your editor to use Unix line endings (at least for
>> > kernel work). Maybe if Linus is feeling charitable he can fix it up for
>> > you. Regarding the meat of the change:
>> >
>> > > > Reviewed-by: Andrew Jeffery <andrew@xxxxxxxx>
>> > > > Tested-by: Andrew Jeffery <andrew@xxxxxxxx>
>> >
>> > Thanks for the patch!
>> >
>> > Andrew
>> >
>> > > ---
>> > > drivers/pinctrl/aspeed/pinctrl-aspeed.c | 19 +++++++++++++++++--
>> > > drivers/pinctrl/aspeed/pinctrl-aspeed.h | 1 +
>> > > 2 files changed, 18 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
>> > > index a86a4d6..f2d5133 100644
>> > > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
>> > > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
>> > > @@ -183,6 +183,7 @@ static int aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>> > > {
>> > > > int ret;
>> > > > int i;
>> > > > + unsigned int rev_id;
>> > > > for (i = 0; i < expr->ndescs; i++) {
>> > > > const struct aspeed_sig_desc *desc = &expr->descs[i];
>> > >
>> > > @@ -213,8 +214,22 @@ static int aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>> > > > if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP2)
>> > > > continue;
>> > > > - ret = regmap_update_bits(maps[desc->ip], desc->reg,
>> > > > - desc->mask, val);
>> > > > + /* On AST2500, Set bits in SCU7C are cleared from SCU70 */
>> > > > + if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1) {
>> > > > + ret = regmap_read(maps[ASPEED_IP_SCU],
>> > > > + HW_REVISION_ID, &rev_id);
>> > > > + if (ret < 0)
>> > > > + return ret;
>> > >
>> > > +
>> > > > + if (0x04 == ((rev_id >> 24) & 0xff))
>> > > > + ret = regmap_write(maps[desc->ip],
>> > > > + HW_REVISION_ID, (~val & desc->mask));
>> > > > + else
>
> Looking back at v2 what you had was correct for the single-bit field case (if
> 'val == 0' was false we went and wrote the set bit to HW_STRAP1), and it seems
> I didn't think through my suggestion enough to catch the problem I created.
>
> Essentially we should drop the 'else' above and always perform the
> regmap_update_bits() HW_STRAP1 because it is write-1-set. Otherwise we can
> only clear bits in the strapping register on the AST2500 and not set them.
> However, given the duplication pointed out below, I've suggested a further
> rearrangement.
>
>> > > > + ret = regmap_update_bits(maps[desc->ip],
>> > > > + desc->reg, desc->mask, val);
>> > > > + } else
>> > > > + ret = regmap_update_bits(maps[desc->ip], desc->reg,
>> > > > + desc->mask, val);
>
> I note that we're doing the same regmap_update_bits() operation twice in two
> separate branches of the code. How about this (note: it's a sketch, and is
> untested)?
>
> (1) if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1) {
> unsigned int rev_id;
>
> (2) ret = regmap_read(maps[ASPEED_IP_SCU], HW_REVISION_ID, &rev_id);
> if (ret < 0)
> return ret;
>
> (3) if (((rev_id >> 24) == 0x04) {
> (4) if (~val & desc->mask) {
> (5) ret = regmap_write(maps[desc->ip],
> HW_REVISION_ID,
> (~val & desc->mask));
> if (ret < 0)
> return ret;
> }
> }
> }
>
> (6) ret = regmap_update_bits(maps[desc->ip], desc->reg, desc->mask, val);
>
> So we have a few cases we need to deal with, AST2500 or not, writing to
> HW_STRAP1 or not, and assuming AST2500/HW_STRAP1, setting and clearing bits. So:
>
> A. AST25xx, only clearing bits in HW_STRAP1:
> The condition at (1) ensures we only perform (2) if we need to, which because
> we're modifying HW_STRAP1 state on an AST25xx we do. Thus (3) evaluates true
> as does (4) due to clearing bits, and we perform a straight regmap_write() at
> (5) due to W1C behaviour. We take a performance penalty by always writing
> HW_STRAP1 at (6) which has no effect as we're only clearing bits. HW_STRAP1 is
> modified as desired.
>
> B. AST25xx, only setting bits in HW_STRAP1:
> We pass the conditions at (1) and (3), however (4) evaluates false as we're
> only setting bits. Thus we reach (6), which modifies HW_STRAP1 as desired.
>
> C. AST25xx, both setting and clearing bits in HW_STRAP1 (e.g. multi-bit field):
> We pass the conditions at (1), (3) and (4), issue (5) to clear the bits then
> hit (6) to write the set bits. HW_STRAP1 is modified as desired.
>
> D. AST25xx, modifying non-HW_STRAP1 registers:
> The condition at (1) fails, we jump straight to (6) and update the register.
>
> E. AST24xx, clearing bits in HW_STRAP1:
> F. AST24xx, setting bits in HW_STRAP1:
> G. AST24xx, setting and clearing bits in HW_STRAP1:
> We pass the condition at (1), but the value read at (2) gives a failure at (3).
> We jump to (6) and update HW_STRAP1.
>
> H. AST24xx, modifying non-HW_STRAP1 registers:
> The condition at (1) fails, we jump straight to (6) and update the register
>
> I don't think I've missed any cases there. Can you shoot holes in it? If not I
> think that's the approach we should take, despite the redundant write for case
> A. Otherwise the code gets messy.
>
> Sorry that this has turned a small problem into such an exercise. I Hope I
> haven't extinguished your drive to get a fix integrated :/ Apologies again for
> the oversight in my comment on v2.
>
> Cheers,
>
> Andrew
>
>> > > > if (ret)
>> > > > return ret;
>> > >
>> > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
>> > > index fa125db..d4d7f03 100644
>> > > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h
>> > > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
>> > > @@ -251,6 +251,7 @@
>> > > #define SCU3C 0x3C /* System Reset Control/Status Register */
>> > > #define SCU48 0x48 /* MAC Interface Clock Delay Setting */
>> > > #define HW_STRAP1 0x70 /* AST2400 strapping is 33 bits, is split */
>> > > +#define HW_REVISION_ID 0x7C /* Silicon revision ID register */
>> > > #define SCU80 0x80 /* Multi-function Pin Control #1 */
>> > > #define SCU84 0x84 /* Multi-function Pin Control #2 */
>> > > #define SCU88 0x88 /* Multi-function Pin Control #3 */