At reset, some KSZ switches use strap based configuration. If the
required pull-ups/pull-downs are missing (by mistake or by design to
save power) the pins may float and the configuration can go wrong.

Introduce a configure_strap() function, called during the device reset.
It relies on the 'strap-gpios' OF property and the 'reset' pinmux
configuration to drive the configuration pins to the proper state.
Support the KSZ8463's strap configuration that enforces SPI as
communication bus, since it is the only bus supported by the driver.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 47 ++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7292bfe2f7cac3a0d88bb51339cc287f56ca1d1f..0ab201a3c336b99ba92d87c003ba48f7f82a098a 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -23,6 +23,7 @@
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/micrel_phy.h>
+#include <linux/pinctrl/consumer.h>
 #include <net/dsa.h>
 #include <net/ieee8021q.h>
 #include <net/pkt_cls.h>
@@ -5338,6 +5339,44 @@ static int ksz_parse_drive_strength(struct ksz_device *dev)
 	return 0;
 }
 
+static int ksz_configure_strap(struct ksz_device *dev)
+{
+	struct pinctrl_state *state = NULL;
+	struct pinctrl *pinctrl;
+	int ret;
+
+	if (of_device_is_compatible(dev->dev->of_node, "microchip,ksz8463")) {
+		struct gpio_desc *rxd0;
+		struct gpio_desc *rxd1;
+
+		rxd0 = devm_gpiod_get_index_optional(dev->dev, "strap", 0, GPIOD_OUT_LOW);
+		if (IS_ERR(rxd0))
+			return PTR_ERR(rxd0);
+
+		rxd1 = devm_gpiod_get_index_optional(dev->dev, "strap", 1, GPIOD_OUT_HIGH);
+		if (IS_ERR(rxd1))
+			return PTR_ERR(rxd1);
+
+		/* If at least one strap definition is missing we don't do anything */
+		if (!rxd0 || !rxd1)
+			return 0;
+
+		pinctrl = devm_pinctrl_get(dev->dev);
+		if (IS_ERR(pinctrl))
+			return PTR_ERR(pinctrl);
+
+		state = pinctrl_lookup_state(pinctrl, "reset");
+		if (IS_ERR(state))
+			return PTR_ERR(state);
+
+		ret = pinctrl_select_state(pinctrl, state);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int ksz_switch_register(struct ksz_device *dev)
 {
 	const struct ksz_chip_data *info;
@@ -5353,10 +5392,18 @@ int ksz_switch_register(struct ksz_device *dev)
 		return PTR_ERR(dev->reset_gpio);
 
 	if (dev->reset_gpio) {
+		ret = ksz_configure_strap(dev);
+		if (ret)
+			return ret;
+
 		gpiod_set_value_cansleep(dev->reset_gpio, 1);
 		usleep_range(10000, 12000);
 		gpiod_set_value_cansleep(dev->reset_gpio, 0);
 		msleep(100);
+
+		ret = pinctrl_select_default_state(dev->dev);
+		if (ret)
+			return ret;
 	}
 
 	mutex_init(&dev->dev_mutex);

-- 
2.51.0

> Support the KSZ8463's strap configuration that enforces SPI as
> communication bus, since it is the only bus supported by the driver.

So this is the key sentence for this patchset, which should of been in
patch 0/X. You have a chicken/egg problem. You cannot talk to the
switch to put it into SPI mode because you cannot talk to the switch
using SPI.

The current patch descriptions, and the patches themselves don't make
this clear. They just vaguely mention configuration via strapping.

> +static int ksz_configure_strap(struct ksz_device *dev)

Please make it clear this function straps the switch for SPI. If
somebody does add support for I2C, they need to understand that...

> +{
> +	struct pinctrl_state *state = NULL;
> +	struct pinctrl *pinctrl;
> +	int ret;
> +
> +	if (of_device_is_compatible(dev->dev->of_node, "microchip,ksz8463")) {

I would not hide this here. Please move this if into
ksz_switch_register(). I also think this function should have the
ksz8463 prefix, since how you strap other devices might differ. So
ksz8463_configure_straps_spi() ?

> +		struct gpio_desc *rxd0;
> +		struct gpio_desc *rxd1;
> +
> +		rxd0 = devm_gpiod_get_index_optional(dev->dev, "strap", 0, GPIOD_OUT_LOW);
> +		if (IS_ERR(rxd0))
> +			return PTR_ERR(rxd0);
> +
> +		rxd1 = devm_gpiod_get_index_optional(dev->dev, "strap", 1, GPIOD_OUT_HIGH);
> +		if (IS_ERR(rxd1))
> +			return PTR_ERR(rxd1);
> +
> +		/* If at least one strap definition is missing we don't do anything */
> +		if (!rxd0 || !rxd1)
> +			return 0;

I would say, if you have one, not two, the DT blob is broken, and you
should return -EINVAL.

> +
> +		pinctrl = devm_pinctrl_get(dev->dev);
> +		if (IS_ERR(pinctrl))
> +			return PTR_ERR(pinctrl);
> +
> +		state = pinctrl_lookup_state(pinctrl, "reset");
> +		if (IS_ERR(state))
> +			return PTR_ERR(state);
> +
> +		ret = pinctrl_select_state(pinctrl, state);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int ksz_switch_register(struct ksz_device *dev)
>  {
>  	const struct ksz_chip_data *info;
> @@ -5353,10 +5392,18 @@ int ksz_switch_register(struct ksz_device *dev)
>  		return PTR_ERR(dev->reset_gpio);
>  
>  	if (dev->reset_gpio) {
> +		ret = ksz_configure_strap(dev);
> +		if (ret)
> +			return ret;
> +
>  		gpiod_set_value_cansleep(dev->reset_gpio, 1);
>  		usleep_range(10000, 12000);
>  		gpiod_set_value_cansleep(dev->reset_gpio, 0);
>  		msleep(100);
> +
> +		ret = pinctrl_select_default_state(dev->dev);
> +		if (ret)
> +			return ret;

This does not look symmetrical. Maybe put the
pinctrl_select_default_state() inside a function called
ksz8463_release_straps_spi()?

	Andrew

Hello bastien,

> +static int ksz_configure_strap(struct ksz_device *dev)
> +{
> +	struct pinctrl_state *state = NULL;
> +	struct pinctrl *pinctrl;
> +	int ret;
> +
> +	if (of_device_is_compatible(dev->dev->of_node, "microchip,ksz8463")) {
> +		struct gpio_desc *rxd0;
> +		struct gpio_desc *rxd1;
> +
> +		rxd0 = devm_gpiod_get_index_optional(dev->dev, "strap", 0, GPIOD_OUT_LOW);
> +		if (IS_ERR(rxd0))
> +			return PTR_ERR(rxd0);
> +
> +		rxd1 = devm_gpiod_get_index_optional(dev->dev, "strap", 1, GPIOD_OUT_HIGH);
> +		if (IS_ERR(rxd1))
> +			return PTR_ERR(rxd1);
> +
> +		/* If at least one strap definition is missing we don't do anything */
> +		if (!rxd0 || !rxd1)
> +			return 0;
> +
> +		pinctrl = devm_pinctrl_get(dev->dev);
> +		if (IS_ERR(pinctrl))
> +			return PTR_ERR(pinctrl);
> +
> +		state = pinctrl_lookup_state(pinctrl, "reset");
> +		if (IS_ERR(state))
> +			return PTR_ERR(state);
> +
> +		ret = pinctrl_select_state(pinctrl, state);
> +		if (ret)
> +			return ret;

In order to simplify the pinctrl handling I would propose to replace
these three function calls by:

      devm_pinctrl_get_select(dev->dev, "reset")

I do not think in this case we actually require the internal
devm_pinctrl_put() calls from the above helper, but they probably do not
hurt either.

Thanks,
Miquèl