Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8225 bytes --]

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@aj.id.au>:
> > 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@gmail.com>
> > 
> > ./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@aj.id.au>
> > > > Tested-by: Andrew Jeffery <andrew@aj.id.au>
> > 
> > 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 */

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]