Re: [RFC PATCH 3/3] net: phy: at803x: add device tree binding

[Florian Fainelli]

On 10/30/19 3:42 PM, Michael Walle wrote:
> Add support for configuring the CLK_25M pin as well as the RGMII I/O
> voltage by the device tree.
> 
> Signed-off-by: Michael Walle <michael@xxxxxxxx>
> ---
>  drivers/net/phy/at803x.c | 156 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 154 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 1eb5d4fb8925..32be4c72cf4b 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -13,7 +13,9 @@
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
>  #include <linux/of_gpio.h>
> +#include <linux/bitfield.h>
>  #include <linux/gpio/consumer.h>
> +#include <dt-bindings/net/atheros-at803x.h>
>  
>  #define AT803X_SPECIFIC_STATUS			0x11
>  #define AT803X_SS_SPEED_MASK			(3 << 14)
> @@ -62,6 +64,37 @@
>  #define AT803X_DEBUG_REG_5			0x05
>  #define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
>  
> +#define AT803X_DEBUG_REG_1F			0x1F
> +#define AT803X_DEBUG_PLL_ON			BIT(2)
> +#define AT803X_DEBUG_RGMII_1V8			BIT(3)
> +
> +/* AT803x supports either the XTAL input pad, an internal PLL or the
> + * DSP as clock reference for the clock output pad. The XTAL reference
> + * is only used for 25 MHz output, all other frequencies need the PLL.
> + * The DSP as a clock reference is used in synchronous ethernet
> + * applications.

How does that tie in the mode in which the PHY is configured? In reverse
MII mode, the PHY provides the TX clock which can be either 25Mhz or
50Mhz AFAIR, in RGMII mode, the TXC provided by the MAC is internally
resynchronized and then fed back to the MAC as a 125Mhz clock.

Do you possibly need to cross check the clock output selection with the
PHY interface?

[snip]
> +static int at803x_parse_dt(struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct at803x_priv *priv = phydev->priv;
> +	u32 freq, strength;
> +	unsigned int sel;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> +		return 0;
> +
> +	if (!node)
> +		return 0;

I don't think you need either of those two things, every of_* function
would check whether the node reference is non-NULL.

> +
> +	if (of_property_read_bool(node, "atheros,keep-pll-enabled"))
> +		priv->flags |= AT803X_KEEP_PLL_ENABLED;

This should probably be a PHY tunable rather than a Device Tree property
as this delves more into the policy than the pure hardware description.

> +
> +	if (of_property_read_bool(node, "atheros,rgmii-io-1v8"))
> +		priv->flags |= AT803X_RGMII_1V8;> +
> +	ret = of_property_read_u32(node, "atheros,clk-out-frequency", &freq);
> +	if (!ret) {
> +		switch (freq) {
> +		case 25000000:
> +			sel = AT803X_CLK_OUT_25MHZ_XTAL;
> +			break;
> +		case 50000000:
> +			sel = AT803X_CLK_OUT_50MHZ_PLL;
> +			break;
> +		case 62500000:
> +			sel = AT803X_CLK_OUT_62_5MHZ_PLL;
> +			break;
> +		case 125000000:
> +			sel = AT803X_CLK_OUT_125MHZ_PLL;
> +			break;
> +		default:
> +			phydev_err(phydev,
> +				   "invalid atheros,clk-out-frequency\n");
> +			return -EINVAL;
> +		}

Maybe the PHY should be a clock provider of some sort, this might be
especially important if the PHY supplies the Ethernet MAC's RXC (which
would be the case in a RGMII configuration).
-- 
Florian