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

[Michael Walle]

Am 31. Oktober 2019 00:28:02 MEZ schrieb Florian Fainelli <f.fainelli@xxxxxxxxx>:
>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?

what do you mean by mode? the "clock output pad" (maybe the term is wrong) is just an additional clock output. And I've ignored syncE mode for now. I don't think there is a real use case for now. because in almost all cases the clock out is used to generate 125MHz required by an RGMII core in the SoC. 


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

The first is needed because otherwise the of_* return -ENOSYS IIRC. I guess it would make no difference here though. Although I don't know how clever the compiler is as it could optimize the whole function away if CONFIG_OF_MDIO is not enabled. 

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

To be frank. I'll first need to look into PHY tunables before answering ;) 
But keep in mind that this clock output might be used anywhere on the board. It must not have something to do with networking. The PHY has a crystal and it can generate these couple of frequencies regardless of its network operation. 

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

Could be the case, I don't know. I'm developing this on a NXP layerscape LS1028A and this SoC needs a fixed 125MHz clock for its RGMII interface (regardless if its 10/100 or 100 Mbit/s). I guess the same is true for the i.MX series. 

-michael