diff mbox series

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

Message ID 20240304135612.814404-1-o.rempel@pengutronix.de
State New
Headers
Series [net,v1,1/1] net: dsa: microchip: make sure drive strength configuration is not lost by soft reset |

Commit Message

Oleksij Rempel March 4, 2024, 1:56 p.m. UTC 
  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(-)

Comments

Arun Ramadoss March 4, 2024, 2:25 p.m. UTC | #1 
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
>
Oleksij Rempel March 4, 2024, 4:07 p.m. UTC | #2 
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
Andrew Lunn March 4, 2024, 4:12 p.m. UTC | #3 
> 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
diff mbox series

Patch

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;