Re: [PATCH net-next 7/7] net: lan969x: add function for configuring RGMII port devices

[Daniel Machon <daniel.machon@microchip.com>]

Hi Andrew,

> > The lan969x switch device includes two RGMII interfaces (port 28 and 29)
> > supporting data speeds of 1 Gbps, 100 Mbps and 10 Mbps.
> >
> > Add new function: rgmii_config() to the match data ops, and use it to
> > configure RGMII port devices when doing a port config.  On Sparx5, the
> > RGMII configuration will always be skipped, as the is_port_rgmii() will
> > return false.
> >
> > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> > Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> > ---
> >  drivers/net/ethernet/microchip/lan969x/lan969x.c   | 105 +++++++++++++++++++++
> >  .../net/ethernet/microchip/sparx5/sparx5_main.h    |   2 +
> >  .../net/ethernet/microchip/sparx5/sparx5_port.c    |   3 +
> >  3 files changed, 110 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan969x/lan969x.c b/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > index cfd57eb42c04..0681913a05d4 100644
> > --- a/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > +++ b/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > @@ -9,6 +9,17 @@
> >  #define LAN969X_SDLB_GRP_CNT 5
> >  #define LAN969X_HSCH_LEAK_GRP_CNT 4
> >
> > +#define LAN969X_RGMII_TX_CLK_DISABLE 0  /* Disable TX clock generation*/
> > +#define LAN969X_RGMII_TX_CLK_125MHZ 1   /* 1000Mbps */
> > +#define LAN969X_RGMII_TX_CLK_25MHZ  2   /* 100Mbps */
> > +#define LAN969X_RGMII_TX_CLK_2M5MHZ 3   /* 10Mbps */
> > +#define LAN969X_RGMII_PORT_START_IDX 28 /* Index of the first RGMII port */
> > +#define LAN969X_RGMII_PORT_RATE 2       /* 1000Mbps  */
> > +#define LAN969X_RGMII_SHIFT_90DEG 3     /* Phase shift 90deg. (2 ns @ 125MHz) */
> > +#define LAN969X_RGMII_IFG_TX 4          /* TX Inter Frame Gap value */
> > +#define LAN969X_RGMII_IFG_RX1 5         /* RX1 Inter Frame Gap value */
> > +#define LAN969X_RGMII_IFG_RX2 1         /* RX2 Inter Frame Gap value */
> > +
> >  static const struct sparx5_main_io_resource lan969x_main_iomap[] =  {
> >       { TARGET_CPU,                   0xc0000, 0 }, /* 0xe00c0000 */
> >       { TARGET_FDMA,                  0xc0400, 0 }, /* 0xe00c0400 */
> > @@ -293,6 +304,99 @@ static irqreturn_t lan969x_ptp_irq_handler(int irq, void *args)
> >       return IRQ_HANDLED;
> >  }
> >
> > +static int lan969x_port_config_rgmii(struct sparx5 *sparx5,
> > +                                  struct sparx5_port *port,
> > +                                  struct sparx5_port_config *conf)
> > +{
> > +     int tx_clk_freq, idx = port->portno - LAN969X_RGMII_PORT_START_IDX;
> > +     enum sparx5_port_max_tags max_tags = port->max_vlan_tags;
> > +     enum sparx5_vlan_port_type vlan_type = port->vlan_type;
> > +     bool dtag, dotag, tx_delay = false, rx_delay = false;
> > +     u32 etype;
> > +
> > +     tx_clk_freq = (conf->speed == SPEED_10  ? LAN969X_RGMII_TX_CLK_2M5MHZ :
> > +                    conf->speed == SPEED_100 ? LAN969X_RGMII_TX_CLK_25MHZ :
> > +                                               LAN969X_RGMII_TX_CLK_125MHZ);
> 
> https://www.spinics.net/lists/netdev/msg1040925.html
> 
> Once it is merged, i think this does what you want.
>

Nice! Thanks for letting me know.

> > +     if (conf->phy_mode == PHY_INTERFACE_MODE_RGMII ||
> > +         conf->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
> > +             rx_delay = true;
> > +
> > +     if (conf->phy_mode == PHY_INTERFACE_MODE_RGMII ||
> > +         conf->phy_mode == PHY_INTERFACE_MODE_RGMII_RXID)
> > +             tx_delay = true;
> 
> O.K, now warning bells are ringing in this reviews head.
> 
> What i don't see is the value you pass to the PHY? You obviously need
> to mask out what the MAC is doing when talking to the PHY, otherwise
> both ends will add delays.
> 

What value should be passed to the PHY?

We (the MAC) add the delays based on the PHY modes - so does the PHY.

RGMII, we add both delays.
RGMII_ID, the PHY adds both delays.
RGMII_TXID, we add the rx delay, the PHY adds the tx delay.
RGMII_RXID, we add the tx delay, the PHY adds the rx delay.

Am I missing something here? :-)

> And in general in Linux, we have the PHY add the delays, not the
> MAC. It is somewhat arbitrary, but the vast majority of systems do
> that. The exception is systems where the PHY is too dumb/cheap to add
> the delays and so the MAC has to do it. I'm don't know of any
> Microchip PHYs which don't support RGMII delays.

Ack.

> 
>         Andrew

/Daniel