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.

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

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

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

  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_CLEAR, 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 +1 -1

index 68315a8..602b260 100644
@@ -255,6 +255,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_STRAP1_CLEAR 0x7C /* Bits written 1 to are cleared from HW_STRAP1 */
  #define SCU80           0x80 /* Multi-function Pin Control #1 */
  #define SCU84           0x84 /* Multi-function Pin Control #2 */
  #define SCU88           0x88 /* Multi-function Pin Control #3 */

@@ -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 4 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.

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

Reviewed-by Andrew Jeffery <[email protected]>

Joel: So lets drop your patches to the board file and apply this. We can switch the devicetrees over to pinctrl_spi1_default later.

Hi,

Context: re. Andrew's suggestion to a previous RFC proposal[1].

To modify SPI1 mode at runtime, pinctrl subsystem will need to add exception to the read-only HWSTRAP rule. This patch does precisely that.

Sending this out as a standalone patch since it seems to me this change could also make boot time SPI1 mode configuration easier. Instead of configuring the strap bits in mach-aspeed/aspeed.c AND make sure device tree pinctrl settings match what was configured, with this change device tree entry should dictate default SPI1 mode regardless of what's set previously. E.g. following node will be able to configure spi1 in master mode.

&spi1 {
        status = "okay";

        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_spi1_default>;
};

I saw Joel sent out a patch that configures SPI1 to master for every AST2500 machine, do we not have any uses currently for other modes?

[1] https://lists.ozlabs.org/pipermail/openbmc/2016-October/005194.html

Thanks,
Kun

Changes since v1:

- Allowed modifying partial bits of SPI1 strap bitmask instead of strictly allowing only bitmask == BIT[13:12] to go through
- Rename HW_STRAP_RESET to HW_STRAP_CLEAR and removed (to be obsoleted) macro

Kun Yi (1):
pinctrl: aspeed: Enable ASPEED pinctrl driver to modify SPI1 strap
bits

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

--
2.8.0.rc3.226.g39d4020

Quoted 18 lines · Kun Yi

Hi,

Context: re. Andrew's suggestion to a previous RFC proposal[1].

To modify SPI1 mode at runtime, pinctrl subsystem will need to add exception to the read-only HWSTRAP rule. This patch does precisely that.

Sending this out as a standalone patch since it seems to me this change could also make boot time SPI1 mode configuration easier. Instead of configuring the strap bits in mach-aspeed/aspeed.c AND make sure device tree pinctrl settings match what was configured, with this change device tree entry should dictate default SPI1 mode regardless of what's set previously. E.g. following node will be able to configure spi1 in master mode.

&spi1 {
        status = "okay";

Quoted 8 lines

        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_spi1_default>;
};

I saw Joel sent out a patch that configures SPI1 to master for every AST2500 machine, do we not have any uses currently for other modes?

[1] https://lists.ozlabs.org/pipermail/openbmc/2016-October/005194.html

Not presently. All of the other systems have the BMC as the owner of the flash, and any operations go through the software on the BMC.

For instance, booting will be done by copying the relevant parts of flash into SDRAM and pointing the LPC mapping at this location. We call that 'memboot'.

I like your patches; it's much cleaner to put the desired configuration in the device tree and have the software do the right thing than to have hacks in the board file. If Andrew and yourself can agree on an implmeentaiton, I will remove the setting of the mux from the board file and go with your patch instead.

Cheers,

Joel

Quoted 22 lines

Thanks,
Kun

Changes since v1:

- Allowed modifying partial bits of SPI1 strap bitmask instead of strictly allowing only bitmask == BIT[13:12] to go through
- Rename HW_STRAP_RESET to HW_STRAP_CLEAR and removed (to be obsoleted) macro

Kun Yi (1):
  pinctrl: aspeed: Enable ASPEED pinctrl driver to modify SPI1 strap bits

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

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

--
2.8.0.rc3.226.g39d4020