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

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Nov 06, 2024 at 08:16:45PM +0100, Daniel Machon wrote:
> 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.

> +	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.

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.

	Andrew