Re: [PATCH net-next 3/3] net: phy: mscc: implement RGMII skew delay configuration

[Quentin Schulz]

Hi Antoine,

It's still me, nitpicker.

On 2020-02-27 16:28, Antoine Tenart wrote:
> This patch adds support for configuring the RGMII skews in Rx and Tx
> thanks to properties defined in the device tree.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> ---
>  drivers/net/phy/mscc.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index ecb45c43e5ed..56d6a45a90c2 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -192,6 +192,10 @@ enum macsec_bank {
>  /* Extended Page 2 Registers */
>  #define MSCC_PHY_CU_PMD_TX_CNTL		  16
>
> +#define MSCC_PHY_RGMII_SETTINGS		  18
> +#define RGMII_SKEW_RX_POS		  1
> +#define RGMII_SKEW_TX_POS		  4
> +
>  #define MSCC_PHY_RGMII_CNTL		  20
>  #define RGMII_RX_CLK_DELAY_MASK		  0x0070
>  #define RGMII_RX_CLK_DELAY_POS		  4
> @@ -2682,6 +2686,7 @@ static bool vsc8584_is_pkg_init(struct
> phy_device *phydev, bool reversed)
>
>  static int vsc8584_config_init(struct phy_device *phydev)
>  {
> +	u32 skew_rx = VSC8584_RGMII_SKEW_0_2, skew_tx = 
> VSC8584_RGMII_SKEW_0_2;
>  	struct vsc8531_private *vsc8531 = phydev->priv;
>  	u16 addr, val;
>  	int ret, i;
> @@ -2830,6 +2835,19 @@ static int vsc8584_config_init(struct phy_device 
> *phydev)
>  	if (ret)
>  		return ret;
>
> +	if (of_find_property(dev->of_node, "vsc8584,rgmii-skew-rx", NULL) ||
> +	    of_find_property(dev->of_node, "vsc8584,rgmii-skew-tx", NULL)) {
> +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-rx", 
> &skew_rx);
> +		of_property_read_u32(dev->of_node, "vsc8584,rgmii-skew-tx", 
> &skew_tx);
> +

Reading the code, I think **!**of_property_read_u32 could directly 
replace of_find_property in your condition and spare you two calls to 
that function.

Also, do we actually need to write that register only when skews are 
defined in the DT? Can't we just write to it anyway (I guess the fact 
that 0_2 skew is actually 0 in value should put me on the right path but 
I prefer to ask).

Final nitpick: I would see a check of the skew_rx/tx from DT before you 
put them in the following line, they could be drastically different from 
0-8 value set that you expect considering you're reading a u32 (pass 
them through a GENMASK at least?)

> +		phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> +				 MSCC_PHY_RGMII_SETTINGS,
> +				 (0x7 << RGMII_SKEW_RX_POS) |
> +				 (0x7 << RGMII_SKEW_TX_POS),
> +				 (skew_rx << RGMII_SKEW_RX_POS) |
> +				 (skew_tx << RGMII_SKEW_TX_POS));
> +	}
> +

Thanks,
Quentin