| Message ID | E1svfMA-005ZI3-Va@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: pcs: xpcs: cleanups batch 1 | expand |
Hi Russell On Tue, Oct 01, 2024 at 05:04:10PM GMT, Russell King (Oracle) wrote: > Move the PCS reset to .pcs_pre_config() rather than at creation time, > which means we call the reset function with the interface that we're > actually going to be using to talk to the downstream device. > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> # sja1105 > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Continuing the RFC discussion. As I mentioned here: https://lore.kernel.org/netdev/mykeabksgikgk6otbub2i3ksfettbozuhqy3gt5vyezmemvttg@cpjn5bcfiwei/ The PCS-reset procedure actually can be converted to being independent from the PHY-interface. Thus you won't need to move the PCS resetting to the pre_config() method, and get rid from the pointer to dw_xpcs_compat utilization each time the reset is required. So why not to merge in my patch to your series as a pre-requisite change and then this patch can be converted to just dropping the xpcs_find_compat() method call from the xpcs_init_iface() function? Alternatively the dropping can be just incorporated into my patch. -Serge(y) > --- > drivers/net/pcs/pcs-xpcs.c | 39 +++++++++++++++++++++++++++--------- > include/linux/pcs/pcs-xpcs.h | 1 + > 2 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c > index 82463f9d50c8..7c6c40ddf722 100644 > --- a/drivers/net/pcs/pcs-xpcs.c > +++ b/drivers/net/pcs/pcs-xpcs.c > @@ -659,6 +659,30 @@ int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns, int enable) > } > EXPORT_SYMBOL_GPL(xpcs_config_eee); > > +static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface) > +{ > + struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs); > + const struct dw_xpcs_compat *compat; > + int ret; > + > + if (!xpcs->need_reset) > + return; > + > + compat = xpcs_find_compat(xpcs->desc, interface); > + if (!compat) { > + dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n", > + phy_modes(interface)); > + return; > + } > + > + ret = xpcs_soft_reset(xpcs, compat); > + if (ret) > + dev_err(&xpcs->mdiodev->dev, "soft reset failed: %pe\n", > + ERR_PTR(ret)); > + > + xpcs->need_reset = false; > +} > + > static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, > unsigned int neg_mode) > { > @@ -1365,6 +1389,7 @@ static const struct dw_xpcs_desc xpcs_desc_list[] = { > > static const struct phylink_pcs_ops xpcs_phylink_ops = { > .pcs_validate = xpcs_validate, > + .pcs_pre_config = xpcs_pre_config, > .pcs_config = xpcs_config, > .pcs_get_state = xpcs_get_state, > .pcs_an_restart = xpcs_an_restart, > @@ -1460,18 +1485,12 @@ static int xpcs_init_id(struct dw_xpcs *xpcs) > > static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface) > { > - const struct dw_xpcs_compat *compat; > - > - compat = xpcs_find_compat(xpcs->desc, interface); > - if (!compat) > - return -EINVAL; > - > - if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { > + if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) > xpcs->pcs.poll = false; > - return 0; > - } > + else > + xpcs->need_reset = true; > > - return xpcs_soft_reset(xpcs, compat); > + return 0; > } > > static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, > diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h > index b4a4eb6c8866..fd75d0605bb6 100644 > --- a/include/linux/pcs/pcs-xpcs.h > +++ b/include/linux/pcs/pcs-xpcs.h > @@ -61,6 +61,7 @@ struct dw_xpcs { > struct clk_bulk_data clks[DW_XPCS_NUM_CLKS]; > struct phylink_pcs pcs; > phy_interface_t interface; > + bool need_reset; > }; > > int xpcs_get_an_mode(struct dw_xpcs *xpcs, phy_interface_t interface); > -- > 2.30.2 > >
On Tue, Oct 01, 2024 at 11:34:42PM +0300, Serge Semin wrote: > Hi Russell > > On Tue, Oct 01, 2024 at 05:04:10PM GMT, Russell King (Oracle) wrote: > > Move the PCS reset to .pcs_pre_config() rather than at creation time, > > which means we call the reset function with the interface that we're > > actually going to be using to talk to the downstream device. > > > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> # sja1105 > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Continuing the RFC discussion. As I mentioned here: > https://lore.kernel.org/netdev/mykeabksgikgk6otbub2i3ksfettbozuhqy3gt5vyezmemvttg@cpjn5bcfiwei/ > The PCS-reset procedure actually can be converted to being independent > from the PHY-interface. Thus you won't need to move the PCS resetting > to the pre_config() method, and get rid from the pointer to > dw_xpcs_compat utilization each time the reset is required. > > So why not to merge in my patch to your series as a pre-requisite > change and then this patch can be converted to just dropping the > xpcs_find_compat() method call from the xpcs_init_iface() function? > Alternatively the dropping can be just incorporated into my patch. I'm wondering why we seem to be having a communication issue here. I'm not sure which part of "keeping the functional changes to a minimum for a cleanup series" you're not understanding. This is one of the basics for kernel development... and given that you're effectively maintaining stmmac, it's something you _should_ know. So no, I'm going to outright refuse to merge your patch in to this series, because as I see it, it would be wrong to do so. This is a _cleanup_ series, not a functional change series, and what you're proposing _changes_ the _way_ reset happens in this driver beyond the minimum that is required for this cleanup. It's introducing a completely _new_ way of writing to the devices registers to do the reset that's different. The more differences there are, the more chances there are of regressions. So, again, no..
> I'm wondering why we seem to be having a communication issue here. > > I'm not sure which part of "keeping the functional changes to a > minimum for a cleanup series" you're not understanding. This is > one of the basics for kernel development... and given that you're > effectively maintaining stmmac, it's something you _should_ know. > > So no, I'm going to outright refuse to merge your patch in to this > series, because as I see it, it would be wrong to do so. This is > a _cleanup_ series, not a functional change series, and what you're > proposing _changes_ the _way_ reset happens in this driver beyond > the minimum that is required for this cleanup. It's introducing a > completely _new_ way of writing to the devices registers to do > the reset that's different. I have to agree with Russell. Cleanups should be as simple as possible, and hopefully obviously correct. They should be low risk. Lets do all the simple cleanups first. Later we can consider more invasive and risky changes. Andrew
On Wed, Oct 02, 2024 at 12:09:22AM GMT, Andrew Lunn wrote: > > I'm wondering why we seem to be having a communication issue here. No communication issue. I just didn't find the discussion over with all the aspects clarified. That's why I've got back to the topic here. > > > > I'm not sure which part of "keeping the functional changes to a > > minimum for a cleanup series" you're not understanding. This is > > one of the basics for kernel development... and given that you're > > effectively maintaining stmmac, it's something you _should_ know. > > > > So no, I'm going to outright refuse to merge your patch in to this > > series, because as I see it, it would be wrong to do so. This is > > a _cleanup_ series, not a functional change series, and what you're > > proposing _changes_ the _way_ reset happens in this driver beyond > > the minimum that is required for this cleanup. It's introducing a > > completely _new_ way of writing to the devices registers to do > > the reset that's different. > > I have to agree with Russell. Cleanups should be as simple as > possible, and hopefully obviously correct. They should be low risk. In general as a thing in itself with no better option to improve the code logic I agree, it should be kept simple. But since the cleanups normally land to net-next, and seeing the patch set still implies some level of the functional change, I don't see much problem with adding a one more change to simplify the driver logic, decrease the level of cohesions (by eliminating the PHY-interface passing to the soft-reset method) and avoid some unneeded change in this patch set. Yes, my patch adds some amount of functional change, but is that that a big problem if both this series and my patch (set) are going to land in net-next anyway, and probably with a little time-lag? Here what we'll see in the commits-tree if my patch is applied as a pre-requisite one of this series: 1.0 Serge: net: pcs: xpcs: Drop compat arg from soft-reset method - 1.1 Russell: net: pcs: xpcs: move PCS reset to .pcs_pre_config() * This patch won't be needed since the PHY-interface will be no longer used for the soft-reset to be performed. 1.2 Russell: net: pcs: xpcs: drop interface argument from internal functions - 1.3 net: pcs: xpcs: get rid of xpcs_init_iface() * This patch won't be applicable since the xpcs_init_iface() method will be still utilized for the basic dw_xpcs initializations and the controller soft-resetting. ... 1.1x Serge: my series rebased onto the Russell' patch set Here is what we'll see in the git-tree if my patch left omitted in this patch set: 2.1 Russell: net: pcs: xpcs: move PCS reset to .pcs_pre_config() 2.2 Russell: net: pcs: xpcs: drop interface argument from internal functions 2.3 Russell: net: pcs: xpcs: get rid of xpcs_init_iface() ... 2.1x Serge: net: pcs: xpcs: Drop compat arg from soft-reset method + 2.1y Serge: net: pcs: xpcs: Get back xpcs_init_iface() * Since the PHY-interface is no longer required for the XPCS soft-resetting I'll move the basic dw_xpcs initializations to the xpcs_init_iface() in order to simplify the driver logic by consolidating the initial setups at the early XPCS-setup stage. This will basically mean to revert the Russell' patches 2.1 and 2.3. 2.1z Serge: the rest of my series rebased onto the Russell' patch set > > Lets do all the simple cleanups first. Later we can consider more > invasive and risky changes. Based on all the considerations above I still think that option 1. described above looks better since it decreases the changes volume in general and decreases the number of patches (by three actually), conserves the changes linearity. But if my reasoning haven't been persuasive enough anyway, then fine by me. I'll just add a new patch (as described in 2.1y) to my series. But please be ready that it will look as a reversion of the Russell' patches 2.1 and 2.3. -Serge(y) > > Andrew
> But if my reasoning haven't been persuasive enough anyway, then fine by > me. I'll just add a new patch (as described in 2.1y) to my series. > But please be ready that it will look as a reversion of the Russell' > patches 2.1 and 2.3. Note what Russell said in patch 0/X: > First, sorry for the bland series subject - this is the first in a > number of cleanup series to the XPCS driver. I suspect you need to wait until all the series have landed before your patches can be applied on top. Andrew
On Thu, Oct 03, 2024 at 01:56:27AM +0300, Serge Semin wrote: > On Wed, Oct 02, 2024 at 12:09:22AM GMT, Andrew Lunn wrote: > > > I'm wondering why we seem to be having a communication issue here. > > No communication issue. I just didn't find the discussion over with > all the aspects clarified. That's why I've got back to the topic here. > > > > > > > I'm not sure which part of "keeping the functional changes to a > > > minimum for a cleanup series" you're not understanding. This is > > > one of the basics for kernel development... and given that you're > > > effectively maintaining stmmac, it's something you _should_ know. > > > > > > So no, I'm going to outright refuse to merge your patch in to this > > > series, because as I see it, it would be wrong to do so. This is > > > a _cleanup_ series, not a functional change series, and what you're > > > proposing _changes_ the _way_ reset happens in this driver beyond > > > the minimum that is required for this cleanup. It's introducing a > > > completely _new_ way of writing to the devices registers to do > > > the reset that's different. > > > > I have to agree with Russell. Cleanups should be as simple as > > possible, and hopefully obviously correct. They should be low risk. > > In general as a thing in itself with no better option to improve the > code logic I agree, it should be kept simple. But since the cleanups > normally land to net-next, and seeing the patch set still implies some > level of the functional change, I don't see much problem with adding a > one more change to simplify the driver logic, decrease the level > of cohesions (by eliminating the PHY-interface passing to the > soft-reset method) and avoid some unneeded change in this patch set. > Yes, my patch adds some amount of functional change, but is that that > a big problem if both this series and my patch (set) are going to land > in net-next anyway, and probably with a little time-lag? > > Here what we'll see in the commits-tree if my patch is applied as a > pre-requisite one of this series: > > 1.0 Serge: net: pcs: xpcs: Drop compat arg from soft-reset method > - 1.1 Russell: net: pcs: xpcs: move PCS reset to .pcs_pre_config() > * This patch won't be needed since the PHY-interface will be no > longer used for the soft-reset to be performed. > 1.2 Russell: net: pcs: xpcs: drop interface argument from internal functions > - 1.3 net: pcs: xpcs: get rid of xpcs_init_iface() > * This patch won't be applicable since the xpcs_init_iface() method > will be still utilized for the basic dw_xpcs initializations and the > controller soft-resetting. > ... > 1.1x Serge: my series rebased onto the Russell' patch set > > Here is what we'll see in the git-tree if my patch left omitted in > this patch set: > > 2.1 Russell: net: pcs: xpcs: move PCS reset to .pcs_pre_config() > 2.2 Russell: net: pcs: xpcs: drop interface argument from internal functions > 2.3 Russell: net: pcs: xpcs: get rid of xpcs_init_iface() > ... > 2.1x Serge: net: pcs: xpcs: Drop compat arg from soft-reset method > + 2.1y Serge: net: pcs: xpcs: Get back xpcs_init_iface() > * Since the PHY-interface is no longer required for the XPCS soft-resetting > I'll move the basic dw_xpcs initializations to the xpcs_init_iface() > in order to simplify the driver logic by consolidating the initial > setups at the early XPCS-setup stage. This will basically mean to > revert the Russell' patches 2.1 and 2.3. > 2.1z Serge: the rest of my series rebased onto the Russell' patch set > > > > > Lets do all the simple cleanups first. Later we can consider more > > invasive and risky changes. > > Based on all the considerations above I still think that option 1. > described above looks better since it decreases the changes volume > in general and decreases the number of patches (by three actually), > conserves the changes linearity. > > But if my reasoning haven't been persuasive enough anyway, then fine by > me. I'll just add a new patch (as described in 2.1y) to my series. > But please be ready that it will look as a reversion of the Russell' > patches 2.1 and 2.3. Oh, sod it. Do whatever you bloody well want. I don't care. You're constantly arguing against me, and I've had enough of this.
On Thu, Oct 03, 2024 at 01:12:58AM GMT, Andrew Lunn wrote: > > But if my reasoning haven't been persuasive enough anyway, then fine by > > me. I'll just add a new patch (as described in 2.1y) to my series. > > But please be ready that it will look as a reversion of the Russell' > > patches 2.1 and 2.3. > > Note what Russell said in patch 0/X: > > > First, sorry for the bland series subject - this is the first in a > > number of cleanup series to the XPCS driver. > > I suspect you need to wait until all the series have landed before > your patches can be applied on top. Of course I have no intention to needlessly over-complicate the review/maintenance process by submitting a new series interfering with the already sent work. That's what I mentioned on the RFC-stage of this series a few days ago: https://lore.kernel.org/netdev/mykeabksgikgk6otbub2i3ksfettbozuhqy3gt5vyezmemvttg@cpjn5bcfiwei/ But for the reason that I've already done some improvements too, why not to use some of them to simplify the Russell' and further changes if they concern the same functionality?.. That's why I originally suggested my patch as a pre-requisite change. Anyway the Russell' patch set in general looks good to me. I have no more comments other than regarding the soft-reset change I described in my previous message. -Serge(y) > > Andrew
> Anyway the Russell' patch set in general looks good to me. I have no > more comments other than regarding the soft-reset change I described in > my previous message. Sorry, i've not been keeping track. Have you sent reviewed-by: and Tested-by: for them? Andrew
On Thu, Oct 03, 2024 at 02:04:36AM +0200, Andrew Lunn wrote: > > Anyway the Russell' patch set in general looks good to me. I have no > > more comments other than regarding the soft-reset change I described in > > my previous message. > > Sorry, i've not been keeping track. Have you sent reviewed-by: and > Tested-by: for them? Of course Serge hasn't. He hasn't even said he's tested them. He's more concerned with the soft-reset change to do anything else other than whinge about that. After the previous debacle over the stmmac PCS cleanup (that I've given up with) I decided later in the series of XPCS cleanups I have to touch stmmac as little as possible because I don't want to interact with Serge anymore. This has now been reinforced further, to the extent that I'm now going to ask Serge to _remove_ all usage of phylink from stmmac for this very reason - I do not wish to interact further with Serge.
On Thu, Oct 03, 2024 at 02:04:36AM GMT, Andrew Lunn wrote: > > Anyway the Russell' patch set in general looks good to me. I have no > > more comments other than regarding the soft-reset change I described in > > my previous message. > > Sorry, i've not been keeping track. Have you sent reviewed-by: and > Tested-by: for them? I have reviewed the series twice on the RFC and v1 stages. But I haven't sent the Rb-tag for the series, just the standard phrase above. I was and still am going to test it out today, when I get to reach my hardware treasury (alas, I couldn't do that earlier this week due to my time-table). Regarding the tags, sorry for not explicitly submitting them. I was going to send them after testing the series out. Seeing the patch set has already been merged in it won't much of importance to do that now though, but I'll send at least Tb-tag anyway after the testing. -Serge(y) > > Andrew
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index 82463f9d50c8..7c6c40ddf722 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -659,6 +659,30 @@ int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns, int enable) } EXPORT_SYMBOL_GPL(xpcs_config_eee); +static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface) +{ + struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs); + const struct dw_xpcs_compat *compat; + int ret; + + if (!xpcs->need_reset) + return; + + compat = xpcs_find_compat(xpcs->desc, interface); + if (!compat) { + dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n", + phy_modes(interface)); + return; + } + + ret = xpcs_soft_reset(xpcs, compat); + if (ret) + dev_err(&xpcs->mdiodev->dev, "soft reset failed: %pe\n", + ERR_PTR(ret)); + + xpcs->need_reset = false; +} + static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int neg_mode) { @@ -1365,6 +1389,7 @@ static const struct dw_xpcs_desc xpcs_desc_list[] = { static const struct phylink_pcs_ops xpcs_phylink_ops = { .pcs_validate = xpcs_validate, + .pcs_pre_config = xpcs_pre_config, .pcs_config = xpcs_config, .pcs_get_state = xpcs_get_state, .pcs_an_restart = xpcs_an_restart, @@ -1460,18 +1485,12 @@ static int xpcs_init_id(struct dw_xpcs *xpcs) static int xpcs_init_iface(struct dw_xpcs *xpcs, phy_interface_t interface) { - const struct dw_xpcs_compat *compat; - - compat = xpcs_find_compat(xpcs->desc, interface); - if (!compat) - return -EINVAL; - - if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) { + if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) xpcs->pcs.poll = false; - return 0; - } + else + xpcs->need_reset = true; - return xpcs_soft_reset(xpcs, compat); + return 0; } static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h index b4a4eb6c8866..fd75d0605bb6 100644 --- a/include/linux/pcs/pcs-xpcs.h +++ b/include/linux/pcs/pcs-xpcs.h @@ -61,6 +61,7 @@ struct dw_xpcs { struct clk_bulk_data clks[DW_XPCS_NUM_CLKS]; struct phylink_pcs pcs; phy_interface_t interface; + bool need_reset; }; int xpcs_get_an_mode(struct dw_xpcs *xpcs, phy_interface_t interface);