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 */Attachment:
signature.asc
Description: This is a digitally signed message part
