Re: [PATCH net-next 1/2] net: pcs: xpcs: enable skip xPCS soft reset

From: Wong Vee Khee
Date: Tue Aug 10 2021 - 19:50:59 EST

Next message: Phillip Potter: "[PATCH 0/8] staging: r8188eu: cleanup several warnings"
Previous message: Randy Dunlap: "[PATCH] kernel/irq: fix kernel-doc warnings in pm.c, msi.c, and ipi.c"
In reply to: Vladimir Oltean: "Re: [PATCH net-next 1/2] net: pcs: xpcs: enable skip xPCS soft reset"
Next in thread: Andrew Lunn: "Re: [PATCH net-next 1/2] net: pcs: xpcs: enable skip xPCS soft reset"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Aug 09, 2021 at 02:06:26PM +0300, Vladimir Oltean wrote:
> Hi VK,
>
> On Mon, Aug 09, 2021 at 06:22:28PM +0800, Wong Vee Khee wrote:
> > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > index 63fda3fc40aa..c7a3aa862079 100644
> > --- a/drivers/net/pcs/pcs-xpcs.c
> > +++ b/drivers/net/pcs/pcs-xpcs.c
> > @@ -1081,7 +1081,8 @@ static const struct phylink_pcs_ops xpcs_phylink_ops = {
> > };
> >
> > struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> > - phy_interface_t interface)
> > + phy_interface_t interface,
> > + bool skip_reset)
> > {
> > struct dw_xpcs *xpcs;
> > u32 xpcs_id;
> > @@ -1113,9 +1114,16 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> > xpcs->pcs.ops = &xpcs_phylink_ops;
> > xpcs->pcs.poll = true;
> >
> > - ret = xpcs_soft_reset(xpcs, compat);
> > - if (ret)
> > - goto out;
> > + if (!skip_reset) {
> > + dev_info(&xpcs->mdiodev->dev, "%s: xPCS soft reset\n",
> > + __func__);
> > + ret = xpcs_soft_reset(xpcs, compat);
> > + if (ret)
> > + goto out;
> > + } else {
> > + dev_info(&xpcs->mdiodev->dev,
> > + "%s: skip xpcs soft reset\n", __func__);
> > + }
>
> I don't feel like the prints are really necessary.

Sounds good to me. I'll remove these.

>
> >
> > return xpcs;
> > }
> > diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
> > index add077a81b21..0c05a63f3446 100644
> > --- a/include/linux/pcs/pcs-xpcs.h
> > +++ b/include/linux/pcs/pcs-xpcs.h
> > @@ -36,7 +36,8 @@ void xpcs_validate(struct dw_xpcs *xpcs, unsigned long *supported,
> > int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
> > int enable);
> > struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> > - phy_interface_t interface);
> > + phy_interface_t interface,
> > + bool xpcs_reset);
> > void xpcs_destroy(struct dw_xpcs *xpcs);
> >
>
> How about exporting the reset functionality as a separate function, and
> the Intel Alder Lake stmmac shim just won't call it? Like this:
>
> -----------------------------[ cut here ]-----------------------------
> diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
> index 19aea8fb76f6..5acf6742da4d 100644
> --- a/drivers/net/dsa/sja1105/sja1105_mdio.c
> +++ b/drivers/net/dsa/sja1105/sja1105_mdio.c
> @@ -437,13 +437,17 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
> goto out_pcs_free;
> }
>
> - xpcs = xpcs_create(mdiodev, priv->phy_mode[port]);
> + xpcs = xpcs_create(mdiodev);
> if (IS_ERR(xpcs)) {
> rc = PTR_ERR(xpcs);
> goto out_pcs_free;
> }
>
> priv->xpcs[port] = xpcs;
> +
> + rc = xpcs_reset(xpcs, priv->phy_mode[port]);
> + if (rc)
> + goto out_pcs_free;
> }
>
> priv->mdio_pcs = bus;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index a5d150c5f3d8..81a145009488 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -401,12 +401,15 @@ int stmmac_xpcs_setup(struct mii_bus *bus)
> {
> struct net_device *ndev = bus->priv;
> struct mdio_device *mdiodev;
> + bool skip_xpcs_soft_reset;
> struct stmmac_priv *priv;
> struct dw_xpcs *xpcs;
> int mode, addr;
> + int err;
>
> priv = netdev_priv(ndev);
> mode = priv->plat->phy_interface;
> + skip_xpcs_soft_reset = priv->plat->skip_xpcs_soft_reset;
>
> /* Try to probe the XPCS by scanning all addresses. */
> for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
> @@ -414,12 +417,21 @@ int stmmac_xpcs_setup(struct mii_bus *bus)
> if (IS_ERR(mdiodev))
> continue;
>
> - xpcs = xpcs_create(mdiodev, mode);
> + xpcs = xpcs_create(mdiodev);
> if (IS_ERR_OR_NULL(xpcs)) {
> mdio_device_free(mdiodev);
> continue;
> }
>
> + if (!skip_xpcs_soft_reset) {
> + err = xpcs_reset(xpcs, mode);
> + if (err) {
> + xpcs_destroy(xpcs);
> + mdio_device_free(mdiodev);
> + continue;
> + }
> + }
> +
> priv->hw->xpcs = xpcs;
> break;
> }
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 63fda3fc40aa..2e721e57bee4 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -248,6 +248,18 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs,
> return xpcs_poll_reset(xpcs, dev);
> }
>
> +int xpcs_reset(struct dw_xpcs *xpcs, phy_interface_t interface)
> +{
> + const struct xpcs_compat *compat;
> +
> + compat = xpcs_find_compat(xpcs->id, interface);
> + if (!compat)
> + return -ENODEV;
> +
> + return xpcs_soft_reset(xpcs, compat);
> +}
> +EXPORT_SYMBOL_GPL(xpcs_reset);
> +
> #define xpcs_warn(__xpcs, __state, __args...) \
> ({ \
> if ((__state)->link) \
> @@ -1080,12 +1092,11 @@ static const struct phylink_pcs_ops xpcs_phylink_ops = {
> .pcs_link_up = xpcs_link_up,
> };
>
> -struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> - phy_interface_t interface)
> +struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
> {
> struct dw_xpcs *xpcs;
> u32 xpcs_id;
> - int i, ret;
> + int i;
>
> xpcs = kzalloc(sizeof(*xpcs), GFP_KERNEL);
> if (!xpcs)
> @@ -1097,35 +1108,18 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
>
> for (i = 0; i < ARRAY_SIZE(xpcs_id_list); i++) {
> const struct xpcs_id *entry = &xpcs_id_list[i];
> - const struct xpcs_compat *compat;
>
> if ((xpcs_id & entry->mask) != entry->id)
> continue;
>
> xpcs->id = entry;
> -
> - compat = xpcs_find_compat(entry, interface);
> - if (!compat) {
> - ret = -ENODEV;
> - goto out;
> - }
> -
> xpcs->pcs.ops = &xpcs_phylink_ops;
> xpcs->pcs.poll = true;
>
> - ret = xpcs_soft_reset(xpcs, compat);
> - if (ret)
> - goto out;
> -
> return xpcs;
> }
>
> - ret = -ENODEV;
> -
> -out:
> - kfree(xpcs);
> -
> - return ERR_PTR(ret);
> + return ERR_PTR(-ENODEV);
> }
> EXPORT_SYMBOL_GPL(xpcs_create);
>
> diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
> index add077a81b21..d841f55f12cc 100644
> --- a/include/linux/pcs/pcs-xpcs.h
> +++ b/include/linux/pcs/pcs-xpcs.h
> @@ -35,8 +35,8 @@ void xpcs_validate(struct dw_xpcs *xpcs, unsigned long *supported,
> struct phylink_link_state *state);
> int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
> int enable);
> -struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> - phy_interface_t interface);
> +int xpcs_reset(struct dw_xpcs *xpcs, phy_interface_t interface);
> +struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev);
> void xpcs_destroy(struct dw_xpcs *xpcs);
>
> #endif /* __LINUX_PCS_XPCS_H */
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index a6f03b36fc4f..0f901773c5e4 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -268,5 +268,6 @@ struct plat_stmmacenet_data {
> int msi_rx_base_vec;
> int msi_tx_base_vec;
> bool use_phy_wol;
> + bool skip_xpcs_soft_reset;
> };
> #endif
> -----------------------------[ cut here ]-----------------------------
>
> I also gave this patch a run on sja1105 and it still works.

Thanks for the suggestion. I tested it out on stmmac and it works.
I will use this as it looks neater. :)

Regards,
VK

Next message: Phillip Potter: "[PATCH 0/8] staging: r8188eu: cleanup several warnings"
Previous message: Randy Dunlap: "[PATCH] kernel/irq: fix kernel-doc warnings in pm.c, msi.c, and ipi.c"
In reply to: Vladimir Oltean: "Re: [PATCH net-next 1/2] net: pcs: xpcs: enable skip xPCS soft reset"
Next in thread: Andrew Lunn: "Re: [PATCH net-next 1/2] net: pcs: xpcs: enable skip xPCS soft reset"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]