Enable pinctrl to modify SPI1 strap bits

Current pinctrl driver treats HWSTRAP1/HWSTRAP2 bits as read-only. Changing SPI1 mode requires pinctrl to modify bit 12 and 13 of HWSTRAP1, and keep other strap bits as read-only.

Note: with this change devicetree default pinctrl settings for spi1 will effectively override any previous mode set by physical strapping or aspeed.c when Aspeed smc driver is probed.

Signed-off-by Kun Yi <[email protected]>

---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 22 ++++++++++++++++++++-- drivers/pinctrl/aspeed/pinctrl-aspeed.h |  2 +-

 2 files changed, 21 insertions(+), 3 deletions(-)

drivers/pinctrl/aspeed/pinctrl-aspeed.c +0 -0

index 21ef195..c647503 100644
@@ -16,6 +16,8 @@

 const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" };

drivers/pinctrl/aspeed/pinctrl-aspeed.c +2 -0

+#define SPI1_REG_MASK   0x3000
+

 int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev) {
  struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);

drivers/pinctrl/aspeed/pinctrl-aspeed.c +1 -0

@@ -178,6 +180,7 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
   *
   * @return true if the expression is configured as requested, false otherwise
   */
+
  static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
  				bool enable, struct regmap *map)
  {

@@ -197,15 +200,30 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,

  /*
   * Strap registers are configured in hardware or by early-boot

drivers/pinctrl/aspeed/pinctrl-aspeed.c +2 -1

-		 * firmware. Treat them as read-only despite that we can write
+		 * firmware. With the exception of SPI1 interface bits, treat
+		 * them as read-only despite that we can write

   * them. This may mean that certain functions cannot be
   * deconfigured and is the reason we re-evaluate after writing
   * all descriptor bits.
   */

  continue;

  /*

drivers/pinctrl/aspeed/pinctrl-aspeed.c +13 -0

+		 * HW_STRAP1 bits can only be set to 0 by writing 1 into
+		 * bits of same offset in SCU7C. To configure different SPI1
+		 * modes, we write 1 to SCU7C[13:12] to clear SPI1 mask to make
+		 * sure later write to strap register can take effect.
+		 */
+		if (is_scu && offset == HW_STRAP1 &&
+		    desc->mask == SPI1_REG_MASK) {
+			ret = regmap_write(map, HW_STRAP1_RESET, desc->mask);
+			if (ret)
+				return false;
+		};
+
+		/*

   * Sometimes we need help from IP outside the SCU to activate a
   * mux request. Report that we need its cooperation. */

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -0

index 68315a8..46ba16d 100644
@@ -268,6 +268,7 @@
  #define HW_STRAP2       0xD0 /* Strapping */

 #define SIORD30 SIG_DESC_TO_REG(ASPEED_IP_SIO, 0x30)
+#define HW_STRAP1_RESET SIG_DESC_TO_REG(ASPEED_IP_SCU, 0x7C)

  /**
   * A signal descriptor, which describes the register, bits and the

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -1

@@ -580,5 +581,4 @@ int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
  int aspeed_pinctrl_probe(struct platform_device *pdev,
  		struct pinctrl_desc *pdesc,
  		struct aspeed_pinctrl_data *pdata);
-
  #endif /* PINCTRL_ASPEED */

Hi Kun,

Quoted 39 lines · Kun Yi

Current pinctrl driver treats HWSTRAP1/HWSTRAP2 bits as read-only. Changing SPI1 mode requires pinctrl to modify bit 12 and 13 of HWSTRAP1, and keep other strap bits as read-only.

Note: with this change devicetree default pinctrl settings for spi1 will effectively override any previous mode set by physical strapping or aspeed.c when Aspeed smc driver is probed.

Signed-off-by Kun Yi <[email protected]>

---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 22 ++++++++++++++++++++--
 drivers/pinctrl/aspeed/pinctrl-aspeed.h |  2 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

drivers/pinctrl/aspeed/pinctrl-aspeed.c +3 -0

index 21ef195..c647503 100644
@@ -16,6 +16,8 @@
 
 const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" };
 
+#define SPI1_REG_MASK   0x3000
+
 int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
 {
      struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);

@@ -178,6 +180,7 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
  *
  * @return true if the expression is configured as requested, false otherwise
  */
+
 static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
                              bool enable, struct regmap *map)
 {

@@ -197,15 +200,30 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 
              /*
               * Strap registers are configured in hardware or by early-boot

drivers/pinctrl/aspeed/pinctrl-aspeed.c static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, · line 210

Quoted 10 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.c +4 -2

-              * firmware. Treat them as read-only despite that we can write
+              * firmware. With the exception of SPI1 interface bits, treat
+              * them as read-only despite that we can write
               * them. This may mean that certain functions cannot be
               * deconfigured and is the reason we re-evaluate after writing
               * all descriptor bits.
               */
-             if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2))
+             if (is_scu && (offset == HW_STRAP2 ||
+                     offset == HW_STRAP1 && desc->mask != SPI1_REG_MASK))

Maybe the mask comparison would be safer by rewriting as:

drivers/pinctrl/aspeed/pinctrl-aspeed.c static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, · line 212

Quoted 2 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.c +2 -0

+             if (is_scu && (offset == HW_STRAP2 ||
+                     offset == HW_STRAP1 && !(desc->mask & SPI1_REG_MASK)))

That way if (for whatever reason) we change the approach used in the -g4 and -g5 implementations we still get the expected outcome.

Quoted 24 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.c +13 -0

                      continue;
 
              /*
+              * HW_STRAP1 bits can only be set to 0 by writing 1 into
+              * bits of same offset in SCU7C. To configure different SPI1
+              * modes, we write 1 to SCU7C[13:12] to clear SPI1 mask to make
+              * sure later write to strap register can take effect.
+              */
+             if (is_scu && offset == HW_STRAP1 &&
+                 desc->mask == SPI1_REG_MASK) {
+                     ret = regmap_write(map, HW_STRAP1_RESET, desc->mask);
+                     if (ret)
+                             return false;
+             };
+
+             /*
               * Sometimes we need help from IP outside the SCU to activate a
               * mux request. Report that we need its cooperation.
               */

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -0

index 68315a8..46ba16d 100644

drivers/pinctrl/aspeed/pinctrl-aspeed.h line 271

Quoted 4 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.h +1 -0

@@ -268,6 +268,7 @@
 #define HW_STRAP2       0xD0 /* Strapping */
 
 #define SIORD30              SIG_DESC_TO_REG(ASPEED_IP_SIO, 0x30)
+#define HW_STRAP1_RESET      SIG_DESC_TO_REG(ASPEED_IP_SCU, 0x7C)

Maybe HW_STRAP1_CLEAR would be ... clearer? Also in this case the SIG_DESC_TO_REG() macro is not entirely necessary as we're talking about the SCU, but being explicit is good. As a note the macro(s) have gone away in the v2 of the series I've recently sent upstream due to feedback from the maintainer:

https://www.spinics.net/lists/arm-kernel/msg540082.html

Otherwise this looks good to me. Thanks for looking at the problem!

Andrew

Quoted 9 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -1

  /**
   * A signal descriptor, which describes the register, bits and the

@@ -580,5 +581,4 @@ int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
 int aspeed_pinctrl_probe(struct platform_device *pdev,
              struct pinctrl_desc *pdesc,
              struct aspeed_pinctrl_data *pdata);
-
 #endif /* PINCTRL_ASPEED */

Thanks for reviewing, Andrew!

Quoted 58 lines

Hi Kun,

Quoted 40 lines · Kun Yi

Current pinctrl driver treats HWSTRAP1/HWSTRAP2 bits as read-only. Changing SPI1 mode requires pinctrl to modify bit 12 and 13 of HWSTRAP1, and keep other strap bits as read-only.

Note: with this change devicetree default pinctrl settings for spi1 will effectively override any previous mode set by physical strapping or aspeed.c when Aspeed smc driver is probed.

Signed-off-by Kun Yi <[email protected]>

---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 22 ++++++++++++++++++++-- drivers/pinctrl/aspeed/pinctrl-aspeed.h |  2 +-

 2 files changed, 21 insertions(+), 3 deletions(-)

drivers/pinctrl/aspeed/pinctrl-aspeed.c +3 -0

index 21ef195..c647503 100644
@@ -16,6 +16,8 @@
 
  const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" };
 
+#define SPI1_REG_MASK   0x3000
+
  int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
  {
       struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);

@@ -178,6 +180,7 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
   *
   * @return true if the expression is configured as requested, false otherwise
   */
+
  static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
                               bool enable, struct regmap *map)
  {

@@ -197,15 +200,30 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 
               /*
                * Strap registers are configured in hardware or by early-boot

drivers/pinctrl/aspeed/pinctrl-aspeed.c static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, · line 210

Quoted 10 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.c +4 -2

-              * firmware. Treat them as read-only despite that we can write
+              * firmware. With the exception of SPI1 interface bits, treat
+              * them as read-only despite that we can write
                * them. This may mean that certain functions cannot be
                * deconfigured and is the reason we re-evaluate after writing
                * all descriptor bits.
                */
-             if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2))
+             if (is_scu && (offset == HW_STRAP2 ||
+                     offset == HW_STRAP1 && desc->mask != SPI1_REG_MASK))

Maybe the mask comparison would be safer by rewriting as:

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

+             if (is_scu && (offset == HW_STRAP2 ||
+                     offset == HW_STRAP1 && !(desc->mask & SPI1_REG_MASK)))

That way if (for whatever reason) we change the approach used in the -g4 and -g5 implementations we still get the expected outcome.

Good point. I will change this and the next mask comparisons in case only one of strap bit 12/13 is used as desc->mask.

Quoted 40 lines

Quoted 26 lines

                      continue;

              /*
+              * HW_STRAP1 bits can only be set to 0 by writing 1 into +              * bits of same offset in SCU7C. To configure different SPI1 +              * modes, we write 1 to SCU7C[13:12] to clear SPI1 mask to make +              * sure later write to strap register can take effect. +              */
+             if (is_scu && offset == HW_STRAP1 &&
+                 desc->mask == SPI1_REG_MASK) {
+                     ret = regmap_write(map, HW_STRAP1_RESET, desc->mask); +                     if (ret)
+                             return false;
+             };
+
+             /*
               * Sometimes we need help from IP outside the SCU to activate a
               * mux request. Report that we need its cooperation. */

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -0

index 68315a8..46ba16d 100644

drivers/pinctrl/aspeed/pinctrl-aspeed.h line 271

Quoted 4 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.h +1 -0

@@ -268,6 +268,7 @@
  #define HW_STRAP2       0xD0 /* Strapping */
 
  #define SIORD30              SIG_DESC_TO_REG(ASPEED_IP_SIO, 0x30)
+#define HW_STRAP1_RESET      SIG_DESC_TO_REG(ASPEED_IP_SCU, 0x7C)

Maybe HW_STRAP1_CLEAR would be ... clearer? Also in this case the SIG_DESC_TO_REG() macro is not entirely necessary as we're talking about the SCU, but being explicit is good. As a note the macro(s) have gone away in the v2 of the series I've recently sent upstream due to feedback from the maintainer:

https://www.spinics.net/lists/arm-kernel/msg540082.html

Otherwise this looks good to me. Thanks for looking at the problem!

Quoted 2 lines

Andrew

HW_STRAP1_CLEAR does sound better, will rename in v2.
Since SIG_DESC_TO_REG() is removed in your v2 patch, should I directly define it as this?

#define HW_STRAP1_CLEAR      0x7C

Quoted 12 lines

Quoted 12 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -1

   /**
    * A signal descriptor, which describes the register, bits and the

@@ -580,5 +581,4 @@ int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
  int aspeed_pinctrl_probe(struct platform_device *pdev,
               struct pinctrl_desc *pdesc,
               struct aspeed_pinctrl_data *pdata);
-
  #endif /* PINCTRL_ASPEED */

Quoted 1 line · Kun Yi

Thanks for reviewing, Andrew!

No worries!

Quoted 115 lines · Andrew Jeffery

Quoted 58 lines

Hi Kun,

Quoted 40 lines · Kun Yi

Current pinctrl driver treats HWSTRAP1/HWSTRAP2 bits as read-only. Changing SPI1 mode requires pinctrl to modify bit 12 and 13 of HWSTRAP1, and keep other strap bits as read-only.

Note: with this change devicetree default pinctrl settings for spi1 will effectively override any previous mode set by physical strapping or aspeed.c when Aspeed smc driver is probed.

Signed-off-by Kun Yi <[email protected]>

---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 22 ++++++++++++++++++++--
 drivers/pinctrl/aspeed/pinctrl-aspeed.h |  2 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

drivers/pinctrl/aspeed/pinctrl-aspeed.c +3 -0

index 21ef195..c647503 100644
@@ -16,6 +16,8 @@
 
 const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" };
 
+#define SPI1_REG_MASK   0x3000
+
 int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
 {
      struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);

@@ -178,6 +180,7 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
  *
  * @return true if the expression is configured as requested, false otherwise
  */
+
 static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
                              bool enable, struct regmap *map)
 {

@@ -197,15 +200,30 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 
              /*
               * Strap registers are configured in hardware or by early-boot

drivers/pinctrl/aspeed/pinctrl-aspeed.c static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, · line 210

Quoted 10 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.c +4 -2

-              * firmware. Treat them as read-only despite that we can write
+              * firmware. With the exception of SPI1 interface bits, treat
+              * them as read-only despite that we can write
               * them. This may mean that certain functions cannot be
               * deconfigured and is the reason we re-evaluate after writing
               * all descriptor bits.
               */
-             if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2))
+             if (is_scu && (offset == HW_STRAP2 ||
+                     offset == HW_STRAP1 && desc->mask != SPI1_REG_MASK))

Maybe the mask comparison would be safer by rewriting as:

Quoted 1 line

Quoted 6 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.c

Quoted 2 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.c +2 -0

+             if (is_scu && (offset == HW_STRAP2 ||
+                     offset == HW_STRAP1 && !(desc->mask & SPI1_REG_MASK)))

That way if (for whatever reason) we change the approach used in the -g4 and -g5 implementations we still get the expected outcome.

Good point. I will change this and the next mask comparisons in case only one of strap bit 12/13 is used as desc->mask.

Quoted 36 lines

Quoted 26 lines

                      continue;

              /*

drivers/pinctrl/aspeed/pinctrl-aspeed.c +13 -0

+              * HW_STRAP1 bits can only be set to 0 by writing 1 into
+              * bits of same offset in SCU7C. To configure different SPI1
+              * modes, we write 1 to SCU7C[13:12] to clear SPI1 mask to make
+              * sure later write to strap register can take effect.
+              */
+             if (is_scu && offset == HW_STRAP1 &&
+                 desc->mask == SPI1_REG_MASK) {
+                     ret = regmap_write(map, HW_STRAP1_RESET, desc->mask);
+                     if (ret)
+                             return false;
+             };
+
+             /*
               * Sometimes we need help from IP outside the SCU to activate a
               * mux request. Report that we need its cooperation.
               */

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -0

index 68315a8..46ba16d 100644

drivers/pinctrl/aspeed/pinctrl-aspeed.h line 271

Quoted 4 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.h +1 -0

@@ -268,6 +268,7 @@
 #define HW_STRAP2       0xD0 /* Strapping */
 
 #define SIORD30              SIG_DESC_TO_REG(ASPEED_IP_SIO, 0x30)
+#define HW_STRAP1_RESET      SIG_DESC_TO_REG(ASPEED_IP_SCU, 0x7C)

Maybe HW_STRAP1_CLEAR would be ... clearer? Also in this case the SIG_DESC_TO_REG() macro is not entirely necessary as we're talking about the SCU, but being explicit is good. As a note the macro(s) have gone away in the v2 of the series I've recently sent upstream due to feedback from the maintainer:

Quoted 6 lines

https://www.spinics.net/lists/arm-kernel/msg540082.html

Otherwise this looks good to me. Thanks for looking at the problem!

Andrew

HW_STRAP1_CLEAR does sound better, will rename in v2.
Since SIG_DESC_TO_REG() is removed in your v2 patch, should I directly define it as this?

Quoted 1 line

#define HW_STRAP1_CLEAR 0x7C

Yes, that would then work in either case (with or without being applied atop v2).

Thanks,

Andrew

Quoted 14 lines

Quoted 12 lines

Quoted 12 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -1

  /**
   * A signal descriptor, which describes the register, bits and the

@@ -580,5 +581,4 @@ int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
 int aspeed_pinctrl_probe(struct platform_device *pdev,
              struct pinctrl_desc *pdesc,
              struct aspeed_pinctrl_data *pdata);
-
 #endif /* PINCTRL_ASPEED */

Quoted 127 lines · Andrew Jeffery

Quoted 2 lines · Kun Yi

Thanks for reviewing, Andrew!

No worries!

Quoted 121 lines · Andrew Jeffery

Quoted 61 lines

Hi Kun,

Quoted 41 lines · Kun Yi

Current pinctrl driver treats HWSTRAP1/HWSTRAP2 bits as read-only. Changing SPI1 mode requires pinctrl to modify bit 12 and 13 of HWSTRAP1, and keep other strap bits as read-only.

Note: with this change devicetree default pinctrl settings for spi1 will effectively override any previous mode set by physical strapping or aspeed.c when Aspeed smc driver is probed.

Signed-off-by Kun Yi <[email protected]>

---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 22 ++++++++++++++++++++-- drivers/pinctrl/aspeed/pinctrl-aspeed.h |  2 +-

 2 files changed, 21 insertions(+), 3 deletions(-)

drivers/pinctrl/aspeed/pinctrl-aspeed.c +3 -0

index 21ef195..c647503 100644
@@ -16,6 +16,8 @@
 
  const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" };
 
+#define SPI1_REG_MASK   0x3000
+
  int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
  {
       struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);

@@ -178,6 +180,7 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
   *
   * @return true if the expression is configured as requested, false otherwise
   */
+
  static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
                               bool enable, struct regmap *map)
  {

@@ -197,15 +200,30 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 
               /*
                * Strap registers are configured in hardware or by early-boot

drivers/pinctrl/aspeed/pinctrl-aspeed.c static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, · line 210

Quoted 10 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.c +4 -2

-              * firmware. Treat them as read-only despite that we can write
+              * firmware. With the exception of SPI1 interface bits, treat
+              * them as read-only despite that we can write
                * them. This may mean that certain functions cannot be
                * deconfigured and is the reason we re-evaluate after writing
                * all descriptor bits.
                */
-             if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2))
+             if (is_scu && (offset == HW_STRAP2 ||
+                     offset == HW_STRAP1 && desc->mask != SPI1_REG_MASK))

Maybe the mask comparison would be safer by rewriting as:

Quoted 2 lines

Quoted 6 lines

Quoted 2 lines

+             if (is_scu && (offset == HW_STRAP2 ||
+                     offset == HW_STRAP1 && !(desc->mask & SPI1_REG_MASK)))

That way if (for whatever reason) we change the approach used in the -g4 and -g5 implementations we still get the expected outcome.

Good point. I will change this and the next mask comparisons in case only one of strap bit 12/13 is used as desc->mask.

Quoted 38 lines

Quoted 28 lines

                      continue;

              /*
+              * HW_STRAP1 bits can only be set to 0 by writing 1 into +              * bits of same offset in SCU7C. To configure different SPI1 +              * modes, we write 1 to SCU7C[13:12] to clear SPI1 mask to make +              * sure later write to strap register can take effect. +              */
+             if (is_scu && offset == HW_STRAP1 &&
+                 desc->mask == SPI1_REG_MASK) {
+                     ret = regmap_write(map, HW_STRAP1_RESET, desc->mask); +                     if (ret)
+                             return false;
+             };
+
+             /*
               * Sometimes we need help from IP outside the SCU to activate a
               * mux request. Report that we need its cooperation. */

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -0

index 68315a8..46ba16d 100644

drivers/pinctrl/aspeed/pinctrl-aspeed.h line 271

Quoted 4 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.h +1 -0

@@ -268,6 +268,7 @@
  #define HW_STRAP2       0xD0 /* Strapping */
 
  #define SIORD30              SIG_DESC_TO_REG(ASPEED_IP_SIO, 0x30)
+#define HW_STRAP1_RESET      SIG_DESC_TO_REG(ASPEED_IP_SCU, 0x7C)

Maybe HW_STRAP1_CLEAR would be ... clearer? Also in this case the SIG_DESC_TO_REG() macro is not entirely necessary as we're talking about the SCU, but being explicit is good. As a note the macro(s) have gone away in the v2 of the series I've recently sent upstream due to feedback from the maintainer:

Quoted 6 lines

https://www.spinics.net/lists/arm-kernel/msg540082.html

Otherwise this looks good to me. Thanks for looking at the problem!

Andrew

HW_STRAP1_CLEAR does sound better, will rename in v2.
Since SIG_DESC_TO_REG() is removed in your v2 patch, should I directly define it as this?

Quoted 24 lines

Quoted 1 line

#define HW_STRAP1_CLEAR      0x7C

Yes, that would then work in either case (with or without being applied atop v2).

Thanks,

Andrew

Quoted 16 lines

Quoted 15 lines

Quoted 15 lines

drivers/pinctrl/aspeed/pinctrl-aspeed.h +0 -1

   /**
    * A signal descriptor, which describes the register, bits and the

@@ -580,5 +581,4 @@ int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
  int aspeed_pinctrl_probe(struct platform_device *pdev,
               struct pinctrl_desc *pdesc,
               struct aspeed_pinctrl_data *pdata);
-
  #endif /* PINCTRL_ASPEED */

Since the SPI1 strap bits will be mutable by the pinctrl driver, should we take the hard-coding out of the mach-aspeed AST2500 fixup?

xo