Re: [PATCH v1] net: phy: micrel: add phy-mode support for the KSZ9031 PHY

[Philippe Schenker]

On Fri, 2020-04-03 at 10:18 +0200, Oleksij Rempel wrote:
> Add support for following phy-modes: rgmii, rgmii-id, rgmii-txid,
> rgmii-rxid.
> 
> This PHY has an internal RX delay of 1.2ns and no delay for TX.
> 
> The pad skew registers allow to set the total TX delay to max 1.38ns
> and
> the total RX delay to max of 2.58ns (configurable 1.38ns + build in
> 1.2ns) and a minimal delay of 0ns.

This skew delay registers of the KSZ9031 are not meant for this delay.
But I agree that it could make sense to implement phy-modes too for this
PHY. I even thought myself about implementing it.
But I guess it is not a
good thing to be able to set the same registers in a chip in two
different places in a DT. How is this solved generally in linux?

Another
reasoning is that this will *only* work, if the PCB traces are length-
matched. This leads me to the conclusion that throwing an error so the
PHY doesn't work if someone added e.g. 'rgmii-id' and skew registers is
a good thing.
But with that we would maybe break some boards... At least I would throw
a warning in ksz9031_of_load_skew_values.

> According to the RGMII v2 specification the delay provided by PCB
> traces

As I understood, RGMII v1.3 demands delay by PCB traces (that is for
embedded mostly not possible). Whereas RGMII v2.0 demands de MAC to add
the delay for TXC and the PHY for RXC.

I know its nitpicky but still can be confusing for someone trying to
understand that. Could you adjust that here?

> should be between 1.5ns and 2.0ns. As this PHY can provide max delay
> of
> only 1.38ns on the TX line, in RGMII-ID mode a symmetric delay of
> 1.38ns
> for both the RX and TX lines is chosen, even if the RX line could be
> configured with the 1.5ns according to the standard.

Why do you decided for a symmetric delay? I guess the hardware level
doesn't care if the input-stages of two different silicons don't care if
the delay is symmetrical. I suggest to use a delay for RXC to get the
RXC clock edge in the middle of the data lines.

Best Regards,
Philippe
> 
> The phy-modes can still be fine tuned/overwritten by *-skew-ps
> device tree properties described in:
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
> 
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> ---
>  drivers/net/phy/micrel.c | 109
> +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 2ec19e5540bff..4fe5a814f586d 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -19,6 +19,7 @@
>   *			 ksz9477
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/phy.h>
> @@ -489,9 +490,50 @@ static int ksz9021_config_init(struct phy_device
> *phydev)
>  
>  /* MMD Address 0x2 */
>  #define MII_KSZ9031RN_CONTROL_PAD_SKEW	4
> +#define MII_KSZ9031RN_RX_CTL_M		GENMASK(7, 4)
> +#define MII_KSZ9031RN_TX_CTL_M		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_RX_DATA_PAD_SKEW	5
> +#define MII_KSZ9031RN_RXD3		GENMASK(15, 12)
> +#define MII_KSZ9031RN_RXD2		GENMASK(11, 8)
> +#define MII_KSZ9031RN_RXD1		GENMASK(7, 4)
> +#define MII_KSZ9031RN_RXD0		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_TX_DATA_PAD_SKEW	6
> +#define MII_KSZ9031RN_TXD3		GENMASK(15, 12)
> +#define MII_KSZ9031RN_TXD2		GENMASK(11, 8)
> +#define MII_KSZ9031RN_TXD1		GENMASK(7, 4)
> +#define MII_KSZ9031RN_TXD0		GENMASK(3, 0)
> +
>  #define MII_KSZ9031RN_CLK_PAD_SKEW	8
> +#define MII_KSZ9031RN_GTX_CLK		GENMASK(9, 5)
> +#define MII_KSZ9031RN_RX_CLK		GENMASK(4, 0)
> +
> +/* KSZ9031 has internal RGMII_IDRX = 1.2ns and RGMII_IDTX = 0ns. To
> + * provide different RGMII options we need to configure delay offset
> + * for each pad relative to build in delay.
> + */
> +/* set rx to +0.18ns and rx_clk to "No delay adjustment" value to get
> delays of
> + * 1.38ns
> + */
> +#define RX_ID				0x1a

0.18ns would be 0xa, why do you put 0x1a a 5-bit value into a 4-bit
register?

Then there's that thing with plus/minus. You shift now data-lines with
0.18ns. Is that now adding or subtracting to the 1.2ns? I would say as
RXC is shifted by 1.2ns and you add a positive delay of 0.18ns you will
end up with 1.02ns delay in total.

> +#define RX_CLK_ID			0xf
> +
> +/* set rx to +0.30ns and rx_clk to -0.90ns to compensate the
> + * internal 1.2ns delay.
> + */
> +#define RX_ND				0xc
> +#define RX_CLK_ND			0x0
> +
> +/* set tx to -0.42ns and tx_clk to +0.96ns to get 1.38ns delay */
> +#define TX_ID				0x0
> +#define TX_CLK_ID			0x1f
> +
> +/* set tx and tx_clk to "No delay adjustment" to keep 0ns
> + * dealy
> + */
> +#define TX_ND				0x7
> +#define TX_CLK_ND			0xf
>  
>  /* MMD Address 0x1C */
>  #define MII_KSZ9031RN_EDPD		0x23
> @@ -564,6 +606,67 @@ static int ksz9031_enable_edpd(struct phy_device
> *phydev)
>  			     reg | MII_KSZ9031RN_EDPD_ENABLE);
>  }
>  
> +static int ksz9031_config_rgmii_delay(struct phy_device *phydev)
> +{
> +	u16 rx, tx, rx_clk, tx_clk;
> +	int ret;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		tx = TX_ND;
> +		tx_clk = TX_CLK_ND;
> +		rx = RX_ND;
> +		rx_clk = RX_CLK_ND;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		tx = TX_ID;
> +		tx_clk = TX_CLK_ID;
> +		rx = RX_ID;
> +		rx_clk = RX_CLK_ID;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		tx = TX_ND;
> +		tx_clk = TX_CLK_ND;
> +		rx = RX_ID;
> +		rx_clk = RX_CLK_ID;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		tx = TX_ID;
> +		tx_clk = TX_CLK_ID;
> +		rx = RX_ND;
> +		rx_clk = RX_CLK_ND;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_CONTROL_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_RX_CTL_M, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TX_CTL_M, tx));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_RX_DATA_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_RXD3, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD2, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD1, rx) |
> +			    FIELD_PREP(MII_KSZ9031RN_RXD0, rx));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, 2, MII_KSZ9031RN_TX_DATA_PAD_SKEW,
> +			    FIELD_PREP(MII_KSZ9031RN_TXD3, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD2, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD1, tx) |
> +			    FIELD_PREP(MII_KSZ9031RN_TXD0, tx));
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_write_mmd(phydev, 2, MII_KSZ9031RN_CLK_PAD_SKEW,
> +			     FIELD_PREP(MII_KSZ9031RN_GTX_CLK, tx_clk) |
> +			     FIELD_PREP(MII_KSZ9031RN_RX_CLK, rx_clk));
> +}
> +
>  static int ksz9031_config_init(struct phy_device *phydev)
>  {
>  	const struct device *dev = &phydev->mdio.dev;
> @@ -596,6 +699,12 @@ static int ksz9031_config_init(struct phy_device
> *phydev)
>  	} while (!of_node && dev_walker);
>  
>  	if (of_node) {
> +		if (phy_interface_is_rgmii(phydev)) {
> +			result = ksz9031_config_rgmii_delay(phydev);
> +			if (result < 0)
> +				return result;
> +		}
> +
>  		ksz9031_of_load_skew_values(phydev, of_node,
>  		ksz9031_of_load_skew_values		MII_KSZ9031RN_CLK_PAD_SKEW, 5,
>  				clk_skews, 2);