[PATCH] pinctrl: aspeed: Fix hardware strap register write logic

[PATCH] pinctrl: aspeed: Fix hardware strap register write logic

[Yong Li]

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 | 9 +++++++--
 drivers/pinctrl/aspeed/pinctrl-aspeed.h | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
index a86a4d6..4305052 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
@@ -213,8 +213,13 @@ 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);
+		if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1 &&
+			val == 0)
+			ret = regmap_update_bits(maps[desc->ip], HW_REVISION_ID,
+				desc->mask, desc->mask);
+		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] pinctrl: aspeed: Fix hardware strap register write logic

[Andrew Jeffery]

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

Hi Yong,

On Fri, 2017-08-11 at 15:27 +0800, Yong Li wrote:
> 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 | 9 +++++++--
>  drivers/pinctrl/aspeed/pinctrl-aspeed.h | 1 +
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index a86a4d6..4305052 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -213,8 +213,13 @@ 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);
> > +		if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1 &&
> > +			val == 0)
> > +			ret = regmap_update_bits(maps[desc->ip], HW_REVISION_ID,
> > +				desc->mask, desc->mask);
> > +		else
> > +			ret = regmap_update_bits(maps[desc->ip], desc->reg,
> +				desc->mask, val);

Good catch! However, this looks like it's only true on the AST2500 (and
related) SoCs. The AST2400 will clear the bits on writing 0 to
HW_STRAP1, so you will need some strategy to differentiate the two.
You're adding the HW_REVISION_ID offset, so maybe we could read that
back and write HW_REVISION_ID if the top byte is 0x04. This would save
some pain propagating the compatible through to here.

For reference, HW_REVISION_ID is structured as:

31:24: Reserved - SoC generation
	AST11xx: 0x00
	AST20xx: 0x00
	AST21xx: 0x00
	AST22xx: 0x00
	AST23xx: 0x01
	AST13xx: 0x01
	AST10xx: 0x01
	AST24xx: 0x02
	AST14xx: 0x02
	AST12xx: 0x02
	AST25xx: 0x04

23:16: Hardware revision
	A0: 0x00
	A1: 0x01
	A2: 0x03

15:8: Chip bonding option

7:0: Reserved

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] pinctrl: aspeed: Fix hardware strap register write logic

[Linus Walleij]

On Fri, Aug 11, 2017 at 9:27 AM, Yong Li <sdliyong@gmail.com> wrote:

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

I'm waiting for an updated patch addressing Andrew's comment
(unless there is one in my mailbox, already, I am swamped as
usual).

Yours,
Linus Walleij