* [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic @ 2017-08-15 16:21 Yong Li 2017-08-16 13:45 ` Andrew Jeffery 2017-08-22 12:52 ` Linus Walleij 0 siblings, 2 replies; 9+ messages in thread From: Yong Li @ 2017-08-15 16:21 UTC (permalink / raw) To: linus.walleij, andrew, joel, arnd, raltherr, robh, linux-gpio, linux-kernel Cc: sdliyong 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> --- 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 + 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); 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 */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic 2017-08-15 16:21 [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic Yong Li @ 2017-08-16 13:45 ` Andrew Jeffery 2017-08-16 15:05 ` Yong Li 2017-08-22 12:52 ` Linus Walleij 1 sibling, 1 reply; 9+ messages in thread From: Andrew Jeffery @ 2017-08-16 13:45 UTC (permalink / raw) To: Yong Li, linus.walleij, joel, arnd, raltherr, robh, linux-gpio, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3091 bytes --] 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 > > + 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); > > > 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic 2017-08-16 13:45 ` Andrew Jeffery @ 2017-08-16 15:05 ` Yong Li 2017-08-17 3:19 ` Andrew Jeffery 0 siblings, 1 reply; 9+ messages in thread From: Yong Li @ 2017-08-16 15:05 UTC (permalink / raw) To: Andrew Jeffery Cc: Linus Walleij, joel, arnd, Rick Altherr, robh, linux-gpio, Linux Kernel Mailing List 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 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 >> > + 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); >> >> > 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 */ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic 2017-08-16 15:05 ` Yong Li @ 2017-08-17 3:19 ` Andrew Jeffery 2017-08-17 14:54 ` Yong Li 0 siblings, 1 reply; 9+ messages in thread From: Andrew Jeffery @ 2017-08-17 3:19 UTC (permalink / raw) To: Yong Li Cc: Linus Walleij, joel, arnd, Rick Altherr, robh, linux-gpio, Linux Kernel Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic 2017-08-17 3:19 ` Andrew Jeffery @ 2017-08-17 14:54 ` Yong Li 0 siblings, 0 replies; 9+ messages in thread From: Yong Li @ 2017-08-17 14:54 UTC (permalink / raw) To: Andrew Jeffery Cc: Linus Walleij, joel, Arnd Bergmann, Rick Altherr, robh, linux-gpio, Linux Kernel Mailing List Thanks Andrew for your thoughtful input. Let me send out another version :) 2017-08-16 20:19 GMT-07:00 Andrew Jeffery <andrew@aj.id.au>: > 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 */ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic 2017-08-15 16:21 [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic Yong Li 2017-08-16 13:45 ` Andrew Jeffery @ 2017-08-22 12:52 ` Linus Walleij 2017-08-22 23:54 ` Andrew Jeffery 1 sibling, 1 reply; 9+ messages in thread From: Linus Walleij @ 2017-08-22 12:52 UTC (permalink / raw) To: Yong Li Cc: Andrew Jeffery, Joel Stanley, Arnd Bergmann, Rick Altherr, Rob Herring, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org On Tue, Aug 15, 2017 at 6:21 PM, Yong Li <sdliyong@gmail.com> 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> Patch applied with Andrew's review/test tags. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic 2017-08-22 12:52 ` Linus Walleij @ 2017-08-22 23:54 ` Andrew Jeffery 2017-08-23 8:23 ` Linus Walleij 0 siblings, 1 reply; 9+ messages in thread From: Andrew Jeffery @ 2017-08-22 23:54 UTC (permalink / raw) To: Linus Walleij, Yong Li Cc: Joel Stanley, Arnd Bergmann, Rick Altherr, Rob Herring, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 865 bytes --] Hi Linus, On Tue, 2017-08-22 at 14:52 +0200, Linus Walleij wrote: > > On Tue, Aug 15, 2017 at 6:21 PM, Yong Li <sdliyong@gmail.com> 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> > > Patch applied with Andrew's review/test tags. I realised after I sent the tags on v3 that I'd made a mistake: There's a slightly awkward to test bug in the v3 implementation. I followed up on v3 with this: https://lkml.org/lkml/2017/8/16/905 And Yong sent out a corresponding v4: https://patchwork.ozlabs.org/patch/802946/ I see you've pushed Yong's v3 in pinctrl/devel - can we revert/remove that and apply v4? Sorry for the confusion. Andrew > > Yours, > Linus Walleij [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic 2017-08-22 23:54 ` Andrew Jeffery @ 2017-08-23 8:23 ` Linus Walleij 2017-08-23 13:47 ` Andrew Jeffery 0 siblings, 1 reply; 9+ messages in thread From: Linus Walleij @ 2017-08-23 8:23 UTC (permalink / raw) To: Andrew Jeffery Cc: Yong Li, Joel Stanley, Arnd Bergmann, Rick Altherr, Rob Herring, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org On Wed, Aug 23, 2017 at 1:54 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > On Tue, 2017-08-22 at 14:52 +0200, Linus Walleij wrote: >> > On Tue, Aug 15, 2017 at 6:21 PM, Yong Li <sdliyong@gmail.com> 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> >> >> Patch applied with Andrew's review/test tags. > > I realised after I sent the tags on v3 that I'd made a mistake: There's a > slightly awkward to test bug in the v3 implementation. I followed up on v3 with > this: > > https://lkml.org/lkml/2017/8/16/905 > > And Yong sent out a corresponding v4: > > https://patchwork.ozlabs.org/patch/802946/ Ah. I got confused and picked the wrong version. > I see you've pushed Yong's v3 in pinctrl/devel - can we revert/remove that and > apply v4? No I would have to revert the patch. Can't we simply make a small fixup patch? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic 2017-08-23 8:23 ` Linus Walleij @ 2017-08-23 13:47 ` Andrew Jeffery 0 siblings, 0 replies; 9+ messages in thread From: Andrew Jeffery @ 2017-08-23 13:47 UTC (permalink / raw) To: Linus Walleij Cc: Yong Li, Joel Stanley, Arnd Bergmann, Rick Altherr, Rob Herring, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 1489 bytes --] On Wed, 2017-08-23 at 10:23 +0200, Linus Walleij wrote: > > On Wed, Aug 23, 2017 at 1:54 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > On Tue, 2017-08-22 at 14:52 +0200, Linus Walleij wrote: > > > > > > > > On Tue, Aug 15, 2017 at 6:21 PM, Yong Li <sdliyong@gmail.com> 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> > > > > > > Patch applied with Andrew's review/test tags. > > > > I realised after I sent the tags on v3 that I'd made a mistake: There's a > > slightly awkward to test bug in the v3 implementation. I followed up on v3 with > > this: > > > > https://lkml.org/lkml/2017/8/16/905 > > > > And Yong sent out a corresponding v4: > > > > https://patchwork.ozlabs.org/patch/802946/ > > Ah. I got confused and picked the wrong version. Entirely my fault. Apologies again for the confusion. I owe you a few drinks if we ever meet :) > > > I see you've pushed Yong's v3 in pinctrl/devel - can we revert/remove that and > > apply v4? > > No I would have to revert the patch. > > Can't we simply make a small fixup patch? I couldn't recall what the rebase policy was for pinctrl/devel. I have sent the fixup patch: http://patchwork.ozlabs.org/patch/804981/ Cheers, Andrew > > Yours, > Linus Walleij [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-23 13:47 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-15 16:21 [PATCH v3] pinctrl: aspeed: Fix ast2500 strap register write logic Yong Li 2017-08-16 13:45 ` Andrew Jeffery 2017-08-16 15:05 ` Yong Li 2017-08-17 3:19 ` Andrew Jeffery 2017-08-17 14:54 ` Yong Li 2017-08-22 12:52 ` Linus Walleij 2017-08-22 23:54 ` Andrew Jeffery 2017-08-23 8:23 ` Linus Walleij 2017-08-23 13:47 ` Andrew Jeffery
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).