Re: [RFC Patch net-next 06/10] net: dsa: microchip: lan937x: add support for configuing xMII register

[<Arun.Ramadoss@microchip.com>]

Hi Vladimir,

On Tue, 2022-07-19 at 14:04 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Jul 12, 2022 at 09:33:04PM +0530, Arun Ramadoss wrote:
> > This patch add the common ksz_set_xmii function for ksz series
> > switch
> > and update the lan937x code phylink mac config. The register
> > address for
> > the ksz8795 is Port 5 Interface control 6 and for all other switch
> > is
> > xMII Control 1.
> > The bit value for selecting the interface is same for
> > KSZ8795 and KSZ9893 are same. The bit values for KSZ9477 and
> > lan973x are
> > same. So, this patch add the bit value for each switches in
> > ksz_chip_data and configure the registers based on the chip id.
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> >  drivers/net/dsa/microchip/ksz_common.c   | 57
> > ++++++++++++++++++++++++
> >  drivers/net/dsa/microchip/ksz_common.h   |  8 ++++
> >  drivers/net/dsa/microchip/lan937x_main.c | 32 +------------
> >  drivers/net/dsa/microchip/lan937x_reg.h  |  9 ----
> >  4 files changed, 66 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index 0cb711fcf046..649da4c361c1 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -284,6 +284,10 @@ static const u32 ksz8795_masks[] = {
> >  };
> > 
> > 
> >  +void ksz_set_xmii(struct ksz_device *dev, int port,
> > phy_interface_t interface)
> > +{
> > +     const u8 *bitval = dev->info->bitval;
> > +     const u16 *regs = dev->info->regs;
> > +     u8 data8;
> > +
> > +     ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8);
> > +
> > +     data8 &= ~(P_MII_SEL_M | P_RGMII_ID_IG_ENABLE |
> > +                P_RGMII_ID_EG_ENABLE);
> > +
> > +     switch (interface) {
> > +     case PHY_INTERFACE_MODE_MII:
> > +             data8 |= bitval[P_MII_SEL];
> > +             break;
> > +     case PHY_INTERFACE_MODE_RMII:
> > +             data8 |= bitval[P_RMII_SEL];
> > +             break;
> > +     case PHY_INTERFACE_MODE_GMII:
> > +             data8 |= bitval[P_GMII_SEL];
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII:
> > +     case PHY_INTERFACE_MODE_RGMII_ID:
> > +     case PHY_INTERFACE_MODE_RGMII_TXID:
> > +     case PHY_INTERFACE_MODE_RGMII_RXID:
> > +             data8 |= bitval[P_RGMII_SEL];
> > +             break;
> > +     default:
> > +             dev_err(dev->dev, "Unsupported interface '%s' for
> > port %d\n",
> > +                     phy_modes(interface), port);
> > +             return;
> > +     }
> > +
> > +     if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +         interface == PHY_INTERFACE_MODE_RGMII_RXID)
> > +             data8 |= P_RGMII_ID_IG_ENABLE;
> > +
> > +     if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +         interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +             data8 |= P_RGMII_ID_EG_ENABLE;
> 
> I'm confused to see RGMII delay handling both in ksz_set_xmii() and
> in
> lan937x_phylink_mac_config(), called immediately afterwards via
> dev->dev_ops->phylink_mac_config(). Can you explain the differences
> between P_RGMII_ID_IG_ENABLE in regs[P_XMII_CTRL_1] and
> RGMII_1_RX_DELAY_2NS
> in REG_PORT_XMII_CTRL_4?

In lan937x RGMII delays are managed by dll register which is not
supported in other ksz switches. REG_PORT_XMII_CTRL_4 and CTRL_5 are
controlled by the P_RGMII_ID_IG_ENABLE & P_RGMII_ID_EG_ENABLE bit. So I
have moved the generic portion of implementation in ksz_set_xmii and
product specific implementation through phylink_mac_config hooks.

> 
> > +
> > +     /* Write the updated value */
> > +     ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
> > +}
> > +
> >  static void ksz_phylink_mac_config(struct dsa_switch *ds, int
> > port,
> >                                  unsigned int mode,
> >                                  const struct phylink_link_state
> > *state)
> > diff --git a/drivers/net/dsa/microchip/ksz_common.h
> > b/drivers/net/dsa/microchip/ksz_common.h
> > index db836b376341..90f3ec9ddaec 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.h
> > +++ b/drivers/net/dsa/microchip/ksz_common.h
> > @@ -216,6 +216,10 @@ enum ksz_shifts {
> >  };
> > 
> > --
> > 2.36.1
> > 
> 
>