lkml.org 
[lkml]   [2017]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic
From
Date
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 */[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2017-08-17 05:21    [W:0.052 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site