| Message ID | 20191030224251.21578-4-michael@walle.cc |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: at803x device tree binding | expand |
On Wed, Oct 30, 2019 at 11:42:51PM +0100, 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@walle.cc> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
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@walle.cc> > --- > 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).
Am 31. Oktober 2019 00:28:02 MEZ schrieb Florian Fainelli <f.fainelli@gmail.com>: >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@walle.cc> >> --- >> 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
Am 2019-10-31 00:59, schrieb Michael Walle: >>> + >>> + 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. Although it could be used to provide any clock on the board, I don't know if that is possible at the moment, because the PHY is configured in config_init() which is only called when someone brings the interface up, correct? Anyway, I don't know if that is worth the hassle because in almost all cases the use case is to provide a fixed clock to the MAC for an RGMII interface. I don't know if that really fits a PHY tunable, because in the worst case the link won't work at all if the SoC expects an always-on clock.
On 10/31/19 10:22 AM, Michael Walle wrote: > Am 2019-10-31 00:59, schrieb Michael Walle: >>>> + >>>> + 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. > > Although it could be used to provide any clock on the board, I don't know > if that is possible at the moment, because the PHY is configured in > config_init() which is only called when someone brings the interface up, > correct? > > Anyway, I don't know if that is worth the hassle because in almost all > cases the use case is to provide a fixed clock to the MAC for an RGMII > interface. I don't know if that really fits a PHY tunable, because in > the worst case the link won't work at all if the SoC expects an > always-on clock. Well, that was my question really, is the clock output being controlled the actual RXC that will feed back to the MAC or is this is another clock output pin (sorry if you indicated that before and I missed it)? If this is the PHY's RXC, then does the configuration (DSP, PLL, XTAL) matter at all on the generated output frequency, or is this just a choice for the board designer, and whether the PHY is configured for MII/RGMII, it outputs the appropriate clock at 25/125Mhz?
Am 2019-10-31 18:35, schrieb Florian Fainelli: > On 10/31/19 10:22 AM, Michael Walle wrote: >> Am 2019-10-31 00:59, schrieb Michael Walle: >>>>> + >>>>> + 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. >> >> Although it could be used to provide any clock on the board, I don't >> know >> if that is possible at the moment, because the PHY is configured in >> config_init() which is only called when someone brings the interface >> up, >> correct? >> >> Anyway, I don't know if that is worth the hassle because in almost all >> cases the use case is to provide a fixed clock to the MAC for an RGMII >> interface. I don't know if that really fits a PHY tunable, because in >> the worst case the link won't work at all if the SoC expects an >> always-on clock. > > Well, that was my question really, is the clock output being controlled > the actual RXC that will feed back to the MAC or is this is another > clock output pin (sorry if you indicated that before and I missed it)? No it is not the RX clock. The PHY has three clock pins, RX clock, TX clock and a general purpose CLK_25M pin. > If this is the PHY's RXC, then does the configuration (DSP, PLL, XTAL) > matter at all on the generated output frequency, or is this just a > choice for the board designer, and whether the PHY is configured for > MII/RGMII, it outputs the appropriate clock at 25/125Mhz? The RXC changes the frequency according to the speed.
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. + * + * By default the PLL is only enabled if there is a link. Otherwise + * the PHY will go into low power state and disabled the PLL. You can + * set the PLL_ON bit (see debug register 0x1f) to keep the PLL always + * enabled. + */ +#define AT803X_MMD7_CLK25M 0x8016 +#define AT803X_CLK_OUT_MASK GENMASK(4, 2) +#define AT803X_CLK_OUT_25MHZ_XTAL 0 +#define AT803X_CLK_OUT_25MHZ_DSP 1 +#define AT803X_CLK_OUT_50MHZ_PLL 2 +#define AT803X_CLK_OUT_50MHZ_DSP 3 +#define AT803X_CLK_OUT_62_5MHZ_PLL 4 +#define AT803X_CLK_OUT_62_5MHZ_DSP 5 +#define AT803X_CLK_OUT_125MHZ_PLL 6 +#define AT803X_CLK_OUT_125MHZ_DSP 7 + +#define AT803X_CLK_OUT_STRENGTH_MASK GENMASK(8, 7) +#define AT803X_CLK_OUT_STRENGTH_FULL 0 +#define AT803X_CLK_OUT_STRENGTH_HALF 1 +#define AT803X_CLK_OUT_STRENGTH_QUARTER 2 + #define ATH8030_PHY_ID 0x004dd076 #define ATH8031_PHY_ID 0x004dd074 #define ATH8035_PHY_ID 0x004dd072 @@ -73,6 +106,11 @@ MODULE_LICENSE("GPL"); struct at803x_priv { bool phy_reset:1; + int flags; +#define AT803X_KEEP_PLL_ENABLED BIT(0) /* don't turn off internal PLL */ +#define AT803X_RGMII_1V8 BIT(1) /* use 1.8V RGMII voltage */ + u16 clk_25m_reg; + u16 clk_25m_mask; }; struct at803x_context { @@ -240,6 +278,74 @@ static int at803x_resume(struct phy_device *phydev) return phy_modify(phydev, MII_BMCR, BMCR_PDOWN | BMCR_ISOLATE, 0); } +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; + + if (of_property_read_bool(node, "atheros,keep-pll-enabled")) + priv->flags |= AT803X_KEEP_PLL_ENABLED; + + 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; + } + + priv->clk_25m_reg |= FIELD_PREP(AT803X_CLK_OUT_MASK, sel); + priv->clk_25m_mask |= AT803X_CLK_OUT_MASK; + } + + ret = of_property_read_u32(node, "atheros,clk-out-strength", &strength); + if (!ret) { + priv->clk_25m_mask |= AT803X_CLK_OUT_STRENGTH_MASK; + switch (strength) { + case AT803X_STRENGTH_FULL: + priv->clk_25m_reg |= AT803X_CLK_OUT_STRENGTH_FULL; + break; + case AT803X_STRENGTH_HALF: + priv->clk_25m_reg |= AT803X_CLK_OUT_STRENGTH_HALF; + break; + case AT803X_STRENGTH_QUARTER: + priv->clk_25m_reg |= AT803X_CLK_OUT_STRENGTH_QUARTER; + break; + default: + phydev_err(phydev, + "invalid atheros,clk-out-strength\n"); + return -EINVAL; + } + } + + return 0; +} + static int at803x_probe(struct phy_device *phydev) { struct device *dev = &phydev->mdio.dev; @@ -251,11 +357,31 @@ static int at803x_probe(struct phy_device *phydev) phydev->priv = priv; - return 0; + return at803x_parse_dt(phydev); +} + +static int at803x_clk_out_config(struct phy_device *phydev) +{ + struct at803x_priv *priv = phydev->priv; + int val; + + if (!priv->clk_25m_mask) + return 0; + + val = phy_read_mmd(phydev, MDIO_MMD_AN, AT803X_MMD7_CLK25M); + if (val < 0) + return val; + + val &= ~priv->clk_25m_mask; + val |= priv->clk_25m_reg; + + return phy_write_mmd(phydev, MDIO_MMD_AN, AT803X_MMD7_CLK25M, val); } static int at803x_config_init(struct phy_device *phydev) { + struct at803x_priv *priv = phydev->priv; + u16 set = 0, clear = 0; int ret; /* The RX and TX delay default is: @@ -276,8 +402,34 @@ static int at803x_config_init(struct phy_device *phydev) ret = at803x_enable_tx_delay(phydev); else ret = at803x_disable_tx_delay(phydev); + if (ret < 0) + return ret; - return ret; + ret = at803x_clk_out_config(phydev); + if (ret < 0) + return ret; + + /* The default after hardware reset is: + * 1.5V RGMII I/O voltage + * PLL OFF (which means it is off if there is no link) + * + * After a soft reset, the values are retained. + */ + if (priv->flags & AT803X_KEEP_PLL_ENABLED) + set |= AT803X_DEBUG_PLL_ON; + else + clear |= AT803X_DEBUG_PLL_ON; + + if (priv->flags & AT803X_RGMII_1V8) + set |= AT803X_DEBUG_RGMII_1V8; + else + clear |= AT803X_DEBUG_RGMII_1V8; + + ret = at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_1F, clear, set); + if (ret < 0) + return ret; + + return 0; } static int at803x_ack_interrupt(struct phy_device *phydev)
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@walle.cc> --- drivers/net/phy/at803x.c | 156 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 154 insertions(+), 2 deletions(-)