Re: [RFC Patch net-next 07/10] net: dsa: microchip: apply rgmii tx and rx delay in phylink mac config

[<Arun.Ramadoss@microchip.com>]

On Tue, 2022-07-19 at 13:25 +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:05PM +0530, Arun Ramadoss wrote:
> > This patch apply the rgmii delay to the xmii tune adjust register
> > based
> > on the interface selected in phylink mac config. There are two
> > rgmii
> > port in LAN937x and value to be loaded in the register vary depends
> > on
> > the port selected.
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> >  drivers/net/dsa/microchip/lan937x_main.c | 61
> > ++++++++++++++++++++++++
> >  drivers/net/dsa/microchip/lan937x_reg.h  | 18 +++++++
> >  2 files changed, 79 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/microchip/lan937x_main.c
> > b/drivers/net/dsa/microchip/lan937x_main.c
> > index d86ffdf976b0..db88ea567ba6 100644
> > --- a/drivers/net/dsa/microchip/lan937x_main.c
> > +++ b/drivers/net/dsa/microchip/lan937x_main.c
> > @@ -315,6 +315,45 @@ int lan937x_change_mtu(struct ksz_device *dev,
> > int port, int new_mtu)
> >       return 0;
> >  }
> > 
> > +
> > +static void lan937x_set_rgmii_tx_delay(struct ksz_device *dev, int
> > port)
> > +{
> > +     u8 val;
> > +
> > +     /* Apply different codes based on the ports as per
> > characterization
> > +      * results
> > +      */
> 
> What characterization result are you referring to? Individual board
> designers should do their own characterization, that's why they
> provide
> a p->rgmii_tx_val in the device tree. The value provided there seems
> to
> be ignored and unconditionally replaced with 2 ns here.

This is the value we got from the post silicon validation team which
has to be programmed to dll register to get the proper delay. The value
is different for rgmii 1 and rgmii2.

> 
> > +     val = (port == LAN937X_RGMII_1_PORT) ? RGMII_1_TX_DELAY_2NS :
> > +             RGMII_2_TX_DELAY_2NS;
> > +
> > +     lan937x_set_tune_adj(dev, port, REG_PORT_XMII_CTRL_5, val);
> > +}
> > +
> > +
> >  @@ -341,6 +383,25 @@ void lan937x_phylink_mac_config(struct
> > ksz_device *dev, int port,
> >       }
> > 
> >       ksz_set_xmii(dev, port, state->interface);
> > +
> > +     /* if the delay is 0, do not enable DLL */
> > +     if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +         interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> 
> Why not all RGMII modes and only these 2? There was a discussion a
> long
> time ago that the "_*ID" values refer to delays applied by an
> attached PHY.
> Here you are refusing to apply RGMII TX delays in the "rgmii" and
> "rgmii-txid"
> modes.

I have reused the code of ksz9477 cpu config function and added the dll
configuration for lan937x family alone. And understood that if device
tree specificies as rgmii_txid then apply the egress delay, for
rgmii_rxid apply ingress delay, for rgmii_id apply both.
From your comment, I am inferring that apply the mac delay for all the
rgmii interface "rgmii*".
Can you correct me if am I wrong and bit elaborate on it.

> 
> > +             if (p->rgmii_tx_val) {
> > +                     lan937x_set_rgmii_tx_delay(dev, port);
> > +                     dev_info(dev->dev, "Applied rgmii tx delay
> > for the port %d\n",
> > +                              port);
> > +             }
> > +     }
> > +
> > +     if (interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +         interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> > +             if (p->rgmii_rx_val) {
> > +                     lan937x_set_rgmii_rx_delay(dev, port);
> > +                     dev_info(dev->dev, "Applied rgmii rx delay
> > for the port %d\n",
> > +                              port);
> > +             }
> > +     }
> >  }
> >