Re: [RFC Patch net-next v2 12/15] net: dsa: microchip: ksz9477: separate phylink mode from switch register

Arun.Ramadoss
2022-06-15T08:50:02Z

On Tue, 2022-06-14 at 11:24 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Mon, May 30, 2022 at 04:12:54PM +0530, Arun Ramadoss wrote:
> > As per 'commit 3506b2f42dff ("net: dsa: microchip: call
> > phy_remove_link_mode during probe")' phy_remove_link_mode is added
> > in
> > the switch_register function after dsa_switch_register. In order to
> > have
> > the common switch register function, moving this phy init after
> > dsa_register_switch using the new ksz_dev_ops.dsa_init hook.
> >
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> > drivers/net/dsa/microchip/ksz9477.c | 49 ++++++++++++++------
> > ------
> > drivers/net/dsa/microchip/ksz_common.c | 5 ++-
> > drivers/net/dsa/microchip/ksz_common.h | 1 +
> > 3 files changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c
> > b/drivers/net/dsa/microchip/ksz9477.c
> > index ecce99b77ef6..c87ce0e2afd8 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -1349,6 +1349,30 @@ static void ksz9477_switch_exit(struct
> > ksz_device *dev)
> > ksz9477_reset_switch(dev);
> > }
> >
> > +static int ksz9477_dsa_init(struct ksz_device *dev)
> > +{
> > + struct phy_device *phydev;
> > + int i;
> > +
> > + for (i = 0; i < dev->phy_port_cnt; ++i) {
> > + if (!dsa_is_user_port(dev->ds, i))
> > + continue;
>
> I understand this is just code movement, but this is more efficient:
>
> struct dsa_switch *ds = dev->ds;
> struct dsa_port *dp;
>
> dsa_switch_for_each_user_port(dp, ds) {
> ...
> }

Yes. I will use the macro where ever possible.

>
> > +
> > + phydev = dsa_to_port(dev->ds, i)->slave->phydev;
> > +
> > + /* The MAC actually cannot run in 1000 half-duplex
> > mode. */
> > + phy_remove_link_mode(phydev,
> > + ETHTOOL_LINK_MODE_1000baseT_Half
> > _BIT);
> > +
> > + /* PHY does not support gigabit. */
> > + if (!(dev->features & GBIT_SUPPORT))
> > + phy_remove_link_mode(phydev,
> > + ETHTOOL_LINK_MODE_1000ba
> > seT_Full_BIT);
> > + }
>
> I wonder why the driver did not just remove these from the supported
> mask in the phylink validation procedure in the first place?
> Adding these link mode fixups to a dev_ops callback named "dsa_init"
> does not sound quite right.

So, it means if the link modes are updated correctly in the
phylink_get_caps then we don't need these link mode removal. Is my
understanding correct?

>
> > +
> > + return 0;
> > +}
> > +
> > static const struct ksz_dev_ops ksz9477_dev_ops = {
> > .setup = ksz9477_setup,
> > .get_port_addr = ksz9477_get_port_addr,
> > @@ -1377,35 +1401,14 @@ static const struct ksz_dev_ops
> > ksz9477_dev_ops = {
> > .change_mtu = ksz9477_change_mtu,
> > .max_mtu = ksz9477_max_mtu,
> > .shutdown = ksz9477_reset_switch,
> > + .dsa_init = ksz9477_dsa_init,
> > .init = ksz9477_switch_init,
> > .exit = ksz9477_switch_exit,
> > };
> >
> > diff --git a/drivers/net/dsa/microchip/ksz_common.h
> > b/drivers/net/dsa/microchip/ksz_common.h
> > index 872d378ac45c..23962f47df46 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.h
> > +++ b/drivers/net/dsa/microchip/ksz_common.h
> > @@ -213,6 +213,7 @@ struct ksz_dev_ops {
> > void (*freeze_mib)(struct ksz_device *dev, int port, bool
> > freeze);
> > void (*port_init_cnt)(struct ksz_device *dev, int port);
> > int (*shutdown)(struct ksz_device *dev);
> > + int (*dsa_init)(struct ksz_device *dev);
> > int (*init)(struct ksz_device *dev);
> > void (*exit)(struct ksz_device *dev);
> > };
> > --
> > 2.36.1
> >
>
>