| Message ID | 20240304135612.814404-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1,1/1] net: dsa: microchip: make sure drive strength configuration is not lost by soft reset | expand |
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 >
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
> 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
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!
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;
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(-)