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

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

[Yong Li]

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

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

[Andrew Jeffery]

[-- 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 --]

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

[Yong Li]

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 */

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

[Andrew Jeffery]

[-- 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 --]

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

[Yong Li]

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 */

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

[Linus Walleij]

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

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

[Andrew Jeffery]

[-- 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 --]

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

[Linus Walleij]

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

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

[Andrew Jeffery]

[-- 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 --]