| Message ID | E1ssjcZ-005Nrf-QL@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: xpcs: cleanups batch 1 | expand |
On Mon, Sep 23, 2024 at 03:00:59PM +0100, 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. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> # sja1105
Hi Russell On Mon, Sep 23, 2024 at 03:00:59PM 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. 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. Please see the attached patch for details.* * I was going to submit it as a part of a one more XPCS-related series, but seeing my work interfere with yours I'll hold on with sending my patch set for until yours is merged in. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > 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; > + } Please note, it's better to preserve the xpcs_find_compat() call even if the need_reset flag is false, since it makes sure that the PHY-interface is actually supported by the PCS. > + > + 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; If you still prefer the PCS-reset being done in the pre_config() function, then what about just directly checking the PMA id in there? if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) return 0; return xpcs_soft_reset(xpcs); -Serge(y) > }; > > int xpcs_get_an_mode(struct dw_xpcs *xpcs, phy_interface_t interface); > -- > 2.30.2 > >
On Mon, Sep 30, 2024 at 01:16:57AM +0300, Serge Semin wrote: > Hi Russell > > On Mon, Sep 23, 2024 at 03:00:59PM GMT, Russell King (Oracle) wrote: > > +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; > > + } > > Please note, it's better to preserve the xpcs_find_compat() call even > if the need_reset flag is false, since it makes sure that the > PHY-interface is actually supported by the PCS. Sorry, but I strongly disagree. xpcs_validate() will already have dealt with that, so we can be sure at this point that the interface is always valid. The NULL check is really only there because it'll stop the "you've forgotten to check for NULL on this function that can return NULL" brigade endlessly submitting patches to add something there - just like xpcs_get_state() and xpcs_do_config(). > > + bool need_reset; > > If you still prefer the PCS-reset being done in the pre_config() > function, then what about just directly checking the PMA id in there? > > if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) > return 0; > > return xpcs_soft_reset(xpcs); I think you've missed what "need_reset" is doing as you seem to think it's just to make it conditional on the PMA ID. That's only part of the story. In the existing code, the reset only happens _once_ when the create happens, not every time the PCS is configured. I am preserving this behaviour, because I do _NOT_ wish to incorporate multiple functional changes into one patch - and certainly in a cleanup series keep the number of functional changes to a minimum. So, all in all, I don't see the need to change anything in my patch. Thanks for the feedback anyway.
On Mon, Sep 30, 2024 at 11:14:15AM GMT, Russell King (Oracle) wrote: > On Mon, Sep 30, 2024 at 01:16:57AM +0300, Serge Semin wrote: > > Hi Russell > > > > On Mon, Sep 23, 2024 at 03:00:59PM GMT, Russell King (Oracle) wrote: > > > +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; > > > + } > > > > Please note, it's better to preserve the xpcs_find_compat() call even > > if the need_reset flag is false, since it makes sure that the > > PHY-interface is actually supported by the PCS. > > Sorry, but I strongly disagree. xpcs_validate() will already have dealt > with that, so we can be sure at this point that the interface is always > valid. The NULL check is really only there because it'll stop the > "you've forgotten to check for NULL on this function that can return > NULL" brigade endlessly submitting patches to add something there - > just like xpcs_get_state() and xpcs_do_config(). Thanks for the detailed answer. Indeed, I missed the part that the pcs_validate() already does the interface check. > > > > + bool need_reset; > > > > If you still prefer the PCS-reset being done in the pre_config() > > function, then what about just directly checking the PMA id in there? > > > > if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) > > return 0; > > > > return xpcs_soft_reset(xpcs); > > I think you've missed what "need_reset" is doing as you seem to > think it's just to make it conditional on the PMA ID. That's only > part of the story. > > In the existing code, the reset only happens _once_ when the create > happens, not every time the PCS is configured. I am preserving this > behaviour, because I do _NOT_ wish to incorporate multiple functional > changes into one patch - and certainly in a cleanup series keep the > number of functional changes to a minimum. Ok. So the goal is to preserve the semantics. Seems reasonable. But... > > So, all in all, I don't see the need to change anything in my patch. I'll get back to this patch discussion in the v1 series since you have already submitted it. -Serge(y) > > Thanks for the feedback anyway. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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);
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. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/pcs/pcs-xpcs.c | 39 +++++++++++++++++++++++++++--------- include/linux/pcs/pcs-xpcs.h | 1 + 2 files changed, 30 insertions(+), 10 deletions(-)