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
