[PATCH net v1 1/1] net: dsa: microchip: make sure drive strength configuration is not lost by soft reset

[PATCH net v1 1/1] net: dsa: microchip: make sure drive strength configuration is not lost by soft reset

[Oleksij Rempel]

This driver has two separate reset sequence in different places:
- gpio/HW reset on start of ksz_switch_register()
- SW reset on start of ksz_setup()

The second one will overwrite drive strength configuration made in the
ksz_switch_register().

To fix it, move ksz_parse_drive_strength() from ksz_switch_register() to
ksz_setup().

Fixes: d67d7247f641 ("net: dsa: microchip: Add drive strength configuration")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index d58cc685478b1..83a5936506059 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2260,6 +2260,8 @@ static int ksz_pirq_setup(struct ksz_device *dev, u8 p)
 	return ksz_irq_common_setup(dev, pirq);
 }
 
+static int ksz_parse_drive_strength(struct ksz_device *dev);
+
 static int ksz_setup(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
@@ -2281,6 +2283,10 @@ static int ksz_setup(struct dsa_switch *ds)
 		return ret;
 	}
 
+	ret = ksz_parse_drive_strength(dev);
+	if (ret)
+		return ret;
+
 	/* set broadcast storm protection 10% rate */
 	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
 			   BROADCAST_STORM_RATE,
@@ -4345,10 +4351,6 @@ int ksz_switch_register(struct ksz_device *dev)
 	for (port_num = 0; port_num < dev->info->port_cnt; ++port_num)
 		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
 	if (dev->dev->of_node) {
-		ret = ksz_parse_drive_strength(dev);
-		if (ret)
-			return ret;
-
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
 			dev->compat_interface = interface;
-- 
2.39.2

Re: [PATCH net v1 1/1] net: dsa: microchip: make sure drive strength configuration is not lost by soft reset

[Arun.Ramadoss]

Hi Oleksij,

On Mon, 2024-03-04 at 14:56 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> This driver has two separate reset sequence in different places:
> - gpio/HW reset on start of ksz_switch_register()
> - SW reset on start of ksz_setup()
> 
> The second one will overwrite drive strength configuration made in
> the
> ksz_switch_register().
> 
> To fix it, move ksz_parse_drive_strength() from ksz_switch_register()
> to
> ksz_setup().
> 
> Fixes: d67d7247f641 ("net: dsa: microchip: Add drive strength
> configuration")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index d58cc685478b1..83a5936506059 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2260,6 +2260,8 @@ static int ksz_pirq_setup(struct ksz_device
> *dev, u8 p)
>         return ksz_irq_common_setup(dev, pirq);
>  }
> 
> +static int ksz_parse_drive_strength(struct ksz_device *dev);
> +

IMO: move the ksz_parse_drive_strength( ) here instead of prototype
alone. Since the function is used in only one place and it will be
logical to follow. 

>  static int ksz_setup(struct dsa_switch *ds)
>  {
>         struct ksz_device *dev = ds->priv;
> @@ -2281,6 +2283,10 @@ static int ksz_setup(struct dsa_switch *ds)
>                 return ret;
>         }
> 
> +       ret = ksz_parse_drive_strength(dev);
> +       if (ret)
> +               return ret;
> +
>         /* set broadcast storm protection 10% rate */
>         regmap_update_bits(ksz_regmap_16(dev),
> regs[S_BROADCAST_CTRL],
>                            BROADCAST_STORM_RATE,
> @@ -4345,10 +4351,6 @@ int ksz_switch_register(struct ksz_device
> *dev)
>         for (port_num = 0; port_num < dev->info->port_cnt;
> ++port_num)
>                 dev->ports[port_num].interface =
> PHY_INTERFACE_MODE_NA;
>         if (dev->dev->of_node) {
> -               ret = ksz_parse_drive_strength(dev);
> -               if (ret)
> -                       return ret;
> -
>                 ret = of_get_phy_mode(dev->dev->of_node, &interface);
>                 if (ret == 0)
>                         dev->compat_interface = interface;
> --
> 2.39.2
> 

Re: [PATCH net v1 1/1] net: dsa: microchip: make sure drive strength configuration is not lost by soft reset

[Oleksij Rempel]

Hi Arun,

On Mon, Mar 04, 2024 at 02:25:54PM +0000, Arun.Ramadoss@microchip.com wrote:
> Hi Oleksij,
> 
> On Mon, 2024-03-04 at 14:56 +0100, Oleksij Rempel wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > This driver has two separate reset sequence in different places:
> > - gpio/HW reset on start of ksz_switch_register()
> > - SW reset on start of ksz_setup()
> > 
> > The second one will overwrite drive strength configuration made in
> > the
> > ksz_switch_register().
> > 
> > To fix it, move ksz_parse_drive_strength() from ksz_switch_register()
> > to
> > ksz_setup().
> > 
> > Fixes: d67d7247f641 ("net: dsa: microchip: Add drive strength
> > configuration")
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/ksz_common.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index d58cc685478b1..83a5936506059 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -2260,6 +2260,8 @@ static int ksz_pirq_setup(struct ksz_device
> > *dev, u8 p)
> >         return ksz_irq_common_setup(dev, pirq);
> >  }
> > 
> > +static int ksz_parse_drive_strength(struct ksz_device *dev);
> > +
> 
> IMO: move the ksz_parse_drive_strength( ) here instead of prototype
> alone. Since the function is used in only one place and it will be
> logical to follow. 

I fully agree, but I fear this change would be too big for stable.

@Jakub, what is your preference?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net v1 1/1] net: dsa: microchip: make sure drive strength configuration is not lost by soft reset

[Andrew Lunn]

> I fully agree, but I fear this change would be too big for stable.

How big is the change to do it correctly?

The stable rules are all about making it obviously correct, and so low
risk. In general, a big patch is not always obviously correct. But if
all you are doing is moving code around, no actual change, and it
clearly states that, the size limit should not matter, the risk is
low. Include this information as justification in the commit message,
and it should be O.K.

	Andrew

Re: [PATCH net v1 1/1] net: dsa: microchip: make sure drive strength configuration is not lost by soft reset

[Jakub Kicinski]

On Mon,  4 Mar 2024 14:56:12 +0100 Oleksij Rempel wrote:
> This driver has two separate reset sequence in different places:
> - gpio/HW reset on start of ksz_switch_register()
> - SW reset on start of ksz_setup()
> 
> The second one will overwrite drive strength configuration made in the
> ksz_switch_register().
> 
> To fix it, move ksz_parse_drive_strength() from ksz_switch_register() to
> ksz_setup().
> 
> Fixes: d67d7247f641 ("net: dsa: microchip: Add drive strength configuration")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Applied, thanks!