[PATCH net-next 0/2] net: dsa: microchip: Add strap configuration during reset

[PATCH net-next 0/2] net: dsa: microchip: Add strap configuration during reset

[Bastien Curutchet]

Hi all,

This small series aims to allow to configure the KSZ8463 switch at
reset. This configuration is determined by pin states while the chip is
held in reset. Normally, this kind of configuration is handled with
pull-ups/pull-downs. However, in some designs these pull-ups/pull-downs
can be missing (either intentionally to save power or simply by mistake).
In such cases, we need to manually drive the configuration pins during
reset to ensure the switch is set up correctly.

PATCH 0 adds a new property to the bindings that describes the GPIOs to
be set during reset in order to configure the switch properly. Alongside
this new property, a new 'reset' pinctrl state is introduced.

PATCH 1 implements the use of this property in the driver. I only have a
KSZ8463 to test with, so only its configuration is supported.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
Bastien Curutchet (2):
      dt-bindings: net: dsa: microchip: Add strap description
      net: dsa: microchip: configure strap pins during reset

 .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 12 ++++++
 drivers/net/dsa/microchip/ksz_common.c             | 47 ++++++++++++++++++++++
 2 files changed, 59 insertions(+)
---
base-commit: d0b93fbf220b2e7be093ac336eba3433cf3cd6f0
change-id: 20250904-ksz-strap-pins-2d1305441325

Best regards,
-- 
Bastien Curutchet <bastien.curutchet@bootlin.com>

[PATCH net-next 1/2] dt-bindings: net: dsa: microchip: Add strap description

[Bastien Curutchet]

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.

Add a strap description that can be used by the driver to drive the
strap pins during reset. It consists of a 'reset' pinmux configuration
and a set of strap GPIOs.
Since the pins used and the nature of the configuration differ from one
KSZ switch to another, GPIO names aren't used.

Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index eb4607460db7f32a4dffd416e44b61c2674f731e..f40a5e3cd0e4d39c809a1fb6697bc3bc64f35fec 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -37,6 +37,13 @@ properties:
       - microchip,ksz8567
       - microchip,lan9646
 
+  pinctrl-names:
+    items:
+      - const: default
+      - const: reset
+        description:
+          Used during reset for strap configuration.
+
   reset-gpios:
     description:
       Should be a gpio specifier for a reset line.
@@ -80,6 +87,11 @@ properties:
     enum: [2000, 4000, 8000, 12000, 16000, 20000, 24000, 28000]
     default: 8000
 
+  strap-gpios:
+    description:
+      Strap pins to drive during reset. For KSZ8463, the first GPIO drives the
+      RXDO line, the second one drives the RXD1 line.
+
   interrupts:
     maxItems: 1
 

-- 
2.51.0

[PATCH net-next 2/2] net: dsa: microchip: configure strap pins during reset

[Bastien Curutchet]

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

Re: [PATCH net-next 2/2] net: dsa: microchip: configure strap pins during reset

[Miquel Raynal]

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

Re: [PATCH net-next 0/2] net: dsa: microchip: Add strap configuration during reset

[Andrew Lunn]

On Wed, Sep 10, 2025 at 04:55:23PM +0200, Bastien Curutchet wrote:
> Hi all,
> 
> This small series aims to allow to configure the KSZ8463 switch at
> reset. This configuration is determined by pin states while the chip is
> held in reset. Normally, this kind of configuration is handled with
> pull-ups/pull-downs. However, in some designs these pull-ups/pull-downs
> can be missing (either intentionally to save power or simply by mistake).
> In such cases, we need to manually drive the configuration pins during
> reset to ensure the switch is set up correctly.
> 
> PATCH 0 adds a new property to the bindings that describes the GPIOs to
> be set during reset in order to configure the switch properly. Alongside
> this new property, a new 'reset' pinctrl state is introduced.
> 
> PATCH 1 implements the use of this property in the driver. I only have a
> KSZ8463 to test with, so only its configuration is supported.

Are you setting switch state which cannot be set via register writes?

	Andrew

Re: [PATCH net-next 2/2] net: dsa: microchip: configure strap pins during reset

[Andrew Lunn]

> 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

Re: [PATCH net-next 2/2] net: dsa: microchip: configure strap pins during reset

[Bastien Curutchet]

Hi Andrew,

On 9/10/25 6:46 PM, Andrew Lunn wrote:
>> 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.
> 

Indeed, that wasn't clear, sorry. My intention was to keep it somewhat 
'generic', since other KSZ switches also use strap configurations. I 
thought the DT property could be re-used by others to enforce different 
kinds of strap configurations.

But I agree with you, it's probably better to have something KSZ8463 
specific.

>> +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() ?
> 

Ack.

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

Ack.

>> +
>> +		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()?
> 
>

Ack.

Best regards,
-- 
Bastien Curutchet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/2] net: dsa: microchip: configure strap pins during reset

[Parthiban.Veerasooran]

Hi,

On 10/09/25 8:25 pm, Bastien Curutchet wrote:
> 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;
I don't think you need to initialize here.
> +       struct pinctrl *pinctrl;
> +       int ret;
> +

Best regards,
Parthiban V

Re: [PATCH net-next 2/2] net: dsa: microchip: configure strap pins during reset

[Bastien Curutchet]

Hi Miquèl,

On 9/10/25 5:38 PM, Miquel Raynal wrote:
> 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.
> 

True, thank you.


Best regards,
Bastien