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

[Vladimir Oltean <olteanv@gmail.com>]

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_tune_adj(struct ksz_device *dev, int port,
> +				 u16 reg, u8 val)
> +{
> +	u16 data16;
> +
> +	ksz_pread16(dev, port, reg, &data16);
> +
> +	/* Update tune Adjust */
> +	data16 |= FIELD_PREP(PORT_TUNE_ADJ, val);
> +	ksz_pwrite16(dev, port, reg, data16);
> +
> +	/* write DLL reset to take effect */
> +	data16 |= PORT_DLL_RESET;
> +	ksz_pwrite16(dev, port, reg, data16);
> +}
> +
> +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.

> +	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);
> +}
> +
> +static void lan937x_set_rgmii_rx_delay(struct ksz_device *dev, int port)
> +{
> +	u8 val;
> +
> +	val = (port == LAN937X_RGMII_1_PORT) ? RGMII_1_RX_DELAY_2NS :
> +		RGMII_2_RX_DELAY_2NS;
> +
> +	lan937x_set_tune_adj(dev, port, REG_PORT_XMII_CTRL_4, val);
> +}
> +
>  void lan937x_phylink_get_caps(struct ksz_device *dev, int port,
>  			      struct phylink_config *config)
>  {
> @@ -331,6 +370,9 @@ void lan937x_phylink_mac_config(struct ksz_device *dev, int port,
>  				unsigned int mode,
>  				const struct phylink_link_state *state)
>  {
> +	phy_interface_t interface = state->interface;
> +	struct ksz_port *p = &dev->ports[port];
> +
>  	/* Internal PHYs */
>  	if (dev->info->internal_phy[port])
>  		return;
> @@ -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.

> +		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);
> +		}
> +	}
>  }
>