| Message ID | 20200618121139.1703762-4-heiko@sntech.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | add clkout support to mscc phys | expand |
On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > At least VSC8530/8531/8540/8541 contain a clock output that can emit > a predefined rate of 25, 50 or 125MHz. > > This may then feed back into the network interface as source clock. > So expose a clock-provider from the phy using the common clock framework > to allow setting the rate. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > --- > drivers/net/phy/mscc/mscc.h | 13 +++ > drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++-- > 2 files changed, 187 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h > index fbcee5fce7b2..94883dab5cc1 100644 > --- a/drivers/net/phy/mscc/mscc.h > +++ b/drivers/net/phy/mscc/mscc.h > @@ -218,6 +218,13 @@ enum rgmii_clock_delay { > #define INT_MEM_DATA_M 0x00ff > #define INT_MEM_DATA(x) (INT_MEM_DATA_M & (x)) > > +#define MSCC_CLKOUT_CNTL 13 > +#define CLKOUT_ENABLE BIT(15) > +#define CLKOUT_FREQ_MASK GENMASK(14, 13) > +#define CLKOUT_FREQ_25M (0x0 << 13) > +#define CLKOUT_FREQ_50M (0x1 << 13) > +#define CLKOUT_FREQ_125M (0x2 << 13) > + > #define MSCC_PHY_PROC_CMD 18 > #define PROC_CMD_NCOMPLETED 0x8000 > #define PROC_CMD_FAILED 0x4000 > @@ -360,6 +367,12 @@ struct vsc8531_private { > */ > unsigned int base_addr; > > +#ifdef CONFIG_COMMON_CLK > + struct clk_hw clkout_hw; > +#endif > + u32 clkout_rate; > + int clkout_enabled; > + > #if IS_ENABLED(CONFIG_MACSEC) > /* MACsec fields: > * - One SecY per device (enforced at the s/w implementation level) > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c > index 5d2777522fb4..727a9dd58403 100644 > --- a/drivers/net/phy/mscc/mscc_main.c > +++ b/drivers/net/phy/mscc/mscc_main.c > @@ -7,6 +7,7 @@ > * Copyright (c) 2016 Microsemi Corporation > */ > > +#include <linux/clk-provider.h> > #include <linux/firmware.h> > #include <linux/jiffies.h> > #include <linux/kernel.h> > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev, > > return led_mode; > } > - > #else > static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev) > { > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev) > return 0; > } > > +static int vsc8531_config_init(struct phy_device *phydev) > +{ > + struct vsc8531_private *vsc8531 = phydev->priv; > + u16 val; > + int rc; > + > + rc = vsc85xx_config_init(phydev); > + if (rc) > + return rc; > + > +#ifdef CONFIG_COMMON_CLK > + switch (vsc8531->clkout_rate) { > + case 25000000: > + val = CLKOUT_FREQ_25M; > + break; > + case 50000000: > + val = CLKOUT_FREQ_50M; > + break; > + case 125000000: > + val = CLKOUT_FREQ_125M; > + break; > + default: > + return -EINVAL; > + } > + > + if (vsc8531->clkout_enabled) > + val |= CLKOUT_ENABLE; > + > + rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO, > + MSCC_CLKOUT_CNTL, val); > + if (rc) > + return rc; > +#endif > + > + return 0; > +} > + > +static int vsc8531_clkout_prepare(struct clk_hw *hw) > +{ > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > + > + vsc8531->clkout_enabled = true; > + return 0; > +} > + > +static void vsc8531_clkout_unprepare(struct clk_hw *hw) > +{ > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > + > + vsc8531->clkout_enabled = false; > +} > + > +static const struct clk_ops vsc8531_clkout_ops = { > + .prepare = vsc8531_clkout_prepare, > + .unprepare = vsc8531_clkout_unprepare, > + .is_prepared = vsc8531_clkout_is_prepared, > + .recalc_rate = vsc8531_clkout_recalc_rate, > + .round_rate = vsc8531_clkout_round_rate, > + .set_rate = vsc8531_clkout_set_rate, I'm not sure this is the expected behaviour. The clk itself should only start ticking when the enable callback is called. But this code will enable the clock when config_init() is called. I think you should implement the enable and disable methods. Andrew
On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote: > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > > At least VSC8530/8531/8540/8541 contain a clock output that can emit > > a predefined rate of 25, 50 or 125MHz. > > > > This may then feed back into the network interface as source clock. > > So expose a clock-provider from the phy using the common clock framework > > to allow setting the rate. > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > --- > > drivers/net/phy/mscc/mscc.h | 13 +++ > > drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++-- > > 2 files changed, 187 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h > > index fbcee5fce7b2..94883dab5cc1 100644 > > --- a/drivers/net/phy/mscc/mscc.h > > +++ b/drivers/net/phy/mscc/mscc.h > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay { > > #define INT_MEM_DATA_M 0x00ff > > #define INT_MEM_DATA(x) (INT_MEM_DATA_M & (x)) > > > > +#define MSCC_CLKOUT_CNTL 13 > > +#define CLKOUT_ENABLE BIT(15) > > +#define CLKOUT_FREQ_MASK GENMASK(14, 13) > > +#define CLKOUT_FREQ_25M (0x0 << 13) > > +#define CLKOUT_FREQ_50M (0x1 << 13) > > +#define CLKOUT_FREQ_125M (0x2 << 13) > > + > > #define MSCC_PHY_PROC_CMD 18 > > #define PROC_CMD_NCOMPLETED 0x8000 > > #define PROC_CMD_FAILED 0x4000 > > @@ -360,6 +367,12 @@ struct vsc8531_private { > > */ > > unsigned int base_addr; > > > > +#ifdef CONFIG_COMMON_CLK > > + struct clk_hw clkout_hw; > > +#endif > > + u32 clkout_rate; > > + int clkout_enabled; > > + > > #if IS_ENABLED(CONFIG_MACSEC) > > /* MACsec fields: > > * - One SecY per device (enforced at the s/w implementation level) > > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c > > index 5d2777522fb4..727a9dd58403 100644 > > --- a/drivers/net/phy/mscc/mscc_main.c > > +++ b/drivers/net/phy/mscc/mscc_main.c > > @@ -7,6 +7,7 @@ > > * Copyright (c) 2016 Microsemi Corporation > > */ > > > > +#include <linux/clk-provider.h> > > #include <linux/firmware.h> > > #include <linux/jiffies.h> > > #include <linux/kernel.h> > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev, > > > > return led_mode; > > } > > - > > #else > > static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev) > > { > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev) > > return 0; > > } > > > > +static int vsc8531_config_init(struct phy_device *phydev) > > +{ > > + struct vsc8531_private *vsc8531 = phydev->priv; > > + u16 val; > > + int rc; > > + > > + rc = vsc85xx_config_init(phydev); > > + if (rc) > > + return rc; > > + > > +#ifdef CONFIG_COMMON_CLK > > + switch (vsc8531->clkout_rate) { > > + case 25000000: > > + val = CLKOUT_FREQ_25M; > > + break; > > + case 50000000: > > + val = CLKOUT_FREQ_50M; > > + break; > > + case 125000000: > > + val = CLKOUT_FREQ_125M; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + if (vsc8531->clkout_enabled) > > + val |= CLKOUT_ENABLE; > > + > > + rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO, > > + MSCC_CLKOUT_CNTL, val); > > + if (rc) > > + return rc; > > +#endif > > + > > + return 0; > > +} > > + > > > +static int vsc8531_clkout_prepare(struct clk_hw *hw) > > +{ > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > > + > > + vsc8531->clkout_enabled = true; > > + return 0; > > +} > > + > > +static void vsc8531_clkout_unprepare(struct clk_hw *hw) > > +{ > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > > + > > + vsc8531->clkout_enabled = false; > > +} > > + > > > +static const struct clk_ops vsc8531_clkout_ops = { > > + .prepare = vsc8531_clkout_prepare, > > + .unprepare = vsc8531_clkout_unprepare, > > + .is_prepared = vsc8531_clkout_is_prepared, > > + .recalc_rate = vsc8531_clkout_recalc_rate, > > + .round_rate = vsc8531_clkout_round_rate, > > + .set_rate = vsc8531_clkout_set_rate, > > I'm not sure this is the expected behaviour. The clk itself should > only start ticking when the enable callback is called. But this code > will enable the clock when config_init() is called. I think you should > implement the enable and disable methods. That is actually incorrect. The whole "prepare" vs "enable" difference is that prepare can schedule, enable isn't permitted. So, if you need to sleep to enable the clock, then enabling the clock in the prepare callback is the right thing to do. However, the above driver just sets a flag, which only gets used when the PHY's config_init method is called; that really doesn't seem to be sane - the clock is available from the point that the PHY has been probed, and it'll be expected that once the clock is published, it can be made functional.
Am Donnerstag, 18. Juni 2020, 15:41:02 CEST schrieb Russell King - ARM Linux admin: > On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote: > > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote: > > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > > > > At least VSC8530/8531/8540/8541 contain a clock output that can emit > > > a predefined rate of 25, 50 or 125MHz. > > > > > > This may then feed back into the network interface as source clock. > > > So expose a clock-provider from the phy using the common clock framework > > > to allow setting the rate. > > > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > --- > > > drivers/net/phy/mscc/mscc.h | 13 +++ > > > drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++-- > > > 2 files changed, 187 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h > > > index fbcee5fce7b2..94883dab5cc1 100644 > > > --- a/drivers/net/phy/mscc/mscc.h > > > +++ b/drivers/net/phy/mscc/mscc.h > > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay { > > > #define INT_MEM_DATA_M 0x00ff > > > #define INT_MEM_DATA(x) (INT_MEM_DATA_M & (x)) > > > > > > +#define MSCC_CLKOUT_CNTL 13 > > > +#define CLKOUT_ENABLE BIT(15) > > > +#define CLKOUT_FREQ_MASK GENMASK(14, 13) > > > +#define CLKOUT_FREQ_25M (0x0 << 13) > > > +#define CLKOUT_FREQ_50M (0x1 << 13) > > > +#define CLKOUT_FREQ_125M (0x2 << 13) > > > + > > > #define MSCC_PHY_PROC_CMD 18 > > > #define PROC_CMD_NCOMPLETED 0x8000 > > > #define PROC_CMD_FAILED 0x4000 > > > @@ -360,6 +367,12 @@ struct vsc8531_private { > > > */ > > > unsigned int base_addr; > > > > > > +#ifdef CONFIG_COMMON_CLK > > > + struct clk_hw clkout_hw; > > > +#endif > > > + u32 clkout_rate; > > > + int clkout_enabled; > > > + > > > #if IS_ENABLED(CONFIG_MACSEC) > > > /* MACsec fields: > > > * - One SecY per device (enforced at the s/w implementation level) > > > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c > > > index 5d2777522fb4..727a9dd58403 100644 > > > --- a/drivers/net/phy/mscc/mscc_main.c > > > +++ b/drivers/net/phy/mscc/mscc_main.c > > > @@ -7,6 +7,7 @@ > > > * Copyright (c) 2016 Microsemi Corporation > > > */ > > > > > > +#include <linux/clk-provider.h> > > > #include <linux/firmware.h> > > > #include <linux/jiffies.h> > > > #include <linux/kernel.h> > > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev, > > > > > > return led_mode; > > > } > > > - > > > #else > > > static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev) > > > { > > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev) > > > return 0; > > > } > > > > > > +static int vsc8531_config_init(struct phy_device *phydev) > > > +{ > > > + struct vsc8531_private *vsc8531 = phydev->priv; > > > + u16 val; > > > + int rc; > > > + > > > + rc = vsc85xx_config_init(phydev); > > > + if (rc) > > > + return rc; > > > + > > > +#ifdef CONFIG_COMMON_CLK > > > + switch (vsc8531->clkout_rate) { > > > + case 25000000: > > > + val = CLKOUT_FREQ_25M; > > > + break; > > > + case 50000000: > > > + val = CLKOUT_FREQ_50M; > > > + break; > > > + case 125000000: > > > + val = CLKOUT_FREQ_125M; > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + if (vsc8531->clkout_enabled) > > > + val |= CLKOUT_ENABLE; > > > + > > > + rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO, > > > + MSCC_CLKOUT_CNTL, val); > > > + if (rc) > > > + return rc; > > > +#endif > > > + > > > + return 0; > > > +} > > > + > > > > > +static int vsc8531_clkout_prepare(struct clk_hw *hw) > > > +{ > > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > > > + > > > + vsc8531->clkout_enabled = true; > > > + return 0; > > > +} > > > + > > > +static void vsc8531_clkout_unprepare(struct clk_hw *hw) > > > +{ > > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > > > + > > > + vsc8531->clkout_enabled = false; > > > +} > > > + > > > > > +static const struct clk_ops vsc8531_clkout_ops = { > > > + .prepare = vsc8531_clkout_prepare, > > > + .unprepare = vsc8531_clkout_unprepare, > > > + .is_prepared = vsc8531_clkout_is_prepared, > > > + .recalc_rate = vsc8531_clkout_recalc_rate, > > > + .round_rate = vsc8531_clkout_round_rate, > > > + .set_rate = vsc8531_clkout_set_rate, > > > > I'm not sure this is the expected behaviour. The clk itself should > > only start ticking when the enable callback is called. But this code > > will enable the clock when config_init() is called. I think you should > > implement the enable and disable methods. > > That is actually incorrect. The whole "prepare" vs "enable" difference > is that prepare can schedule, enable isn't permitted. So, if you need > to sleep to enable the clock, then enabling the clock in the prepare > callback is the right thing to do. > > However, the above driver just sets a flag, which only gets used when > the PHY's config_init method is called; that really doesn't seem to be > sane - the clock is available from the point that the PHY has been > probed, and it'll be expected that once the clock is published, it can > be made functional. Though I'm not sure how this fits in the whole bringup of ethernet phys. Like the phy is dependent on the underlying ethernet controller to actually turn it on. I guess we should check the phy-state and if it's not accessible, just keep the values and if it's in a suitable state do the configuration. Calling a vsc8531_config_clkout() from both the vsc8531_config_init() as well as the clk_(un-)prepare and clk_set_rate functions and being protected by a check against phy_is_started() ? Heiko
On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote: > Am Donnerstag, 18. Juni 2020, 15:41:02 CEST schrieb Russell King - ARM Linux admin: > > On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote: > > > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote: > > > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > > > > > > At least VSC8530/8531/8540/8541 contain a clock output that can emit > > > > a predefined rate of 25, 50 or 125MHz. > > > > > > > > This may then feed back into the network interface as source clock. > > > > So expose a clock-provider from the phy using the common clock framework > > > > to allow setting the rate. > > > > > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > > --- > > > > drivers/net/phy/mscc/mscc.h | 13 +++ > > > > drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++-- > > > > 2 files changed, 187 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h > > > > index fbcee5fce7b2..94883dab5cc1 100644 > > > > --- a/drivers/net/phy/mscc/mscc.h > > > > +++ b/drivers/net/phy/mscc/mscc.h > > > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay { > > > > #define INT_MEM_DATA_M 0x00ff > > > > #define INT_MEM_DATA(x) (INT_MEM_DATA_M & (x)) > > > > > > > > +#define MSCC_CLKOUT_CNTL 13 > > > > +#define CLKOUT_ENABLE BIT(15) > > > > +#define CLKOUT_FREQ_MASK GENMASK(14, 13) > > > > +#define CLKOUT_FREQ_25M (0x0 << 13) > > > > +#define CLKOUT_FREQ_50M (0x1 << 13) > > > > +#define CLKOUT_FREQ_125M (0x2 << 13) > > > > + > > > > #define MSCC_PHY_PROC_CMD 18 > > > > #define PROC_CMD_NCOMPLETED 0x8000 > > > > #define PROC_CMD_FAILED 0x4000 > > > > @@ -360,6 +367,12 @@ struct vsc8531_private { > > > > */ > > > > unsigned int base_addr; > > > > > > > > +#ifdef CONFIG_COMMON_CLK > > > > + struct clk_hw clkout_hw; > > > > +#endif > > > > + u32 clkout_rate; > > > > + int clkout_enabled; > > > > + > > > > #if IS_ENABLED(CONFIG_MACSEC) > > > > /* MACsec fields: > > > > * - One SecY per device (enforced at the s/w implementation level) > > > > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c > > > > index 5d2777522fb4..727a9dd58403 100644 > > > > --- a/drivers/net/phy/mscc/mscc_main.c > > > > +++ b/drivers/net/phy/mscc/mscc_main.c > > > > @@ -7,6 +7,7 @@ > > > > * Copyright (c) 2016 Microsemi Corporation > > > > */ > > > > > > > > +#include <linux/clk-provider.h> > > > > #include <linux/firmware.h> > > > > #include <linux/jiffies.h> > > > > #include <linux/kernel.h> > > > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev, > > > > > > > > return led_mode; > > > > } > > > > - > > > > #else > > > > static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev) > > > > { > > > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev) > > > > return 0; > > > > } > > > > > > > > +static int vsc8531_config_init(struct phy_device *phydev) > > > > +{ > > > > + struct vsc8531_private *vsc8531 = phydev->priv; > > > > + u16 val; > > > > + int rc; > > > > + > > > > + rc = vsc85xx_config_init(phydev); > > > > + if (rc) > > > > + return rc; > > > > + > > > > +#ifdef CONFIG_COMMON_CLK > > > > + switch (vsc8531->clkout_rate) { > > > > + case 25000000: > > > > + val = CLKOUT_FREQ_25M; > > > > + break; > > > > + case 50000000: > > > > + val = CLKOUT_FREQ_50M; > > > > + break; > > > > + case 125000000: > > > > + val = CLKOUT_FREQ_125M; > > > > + break; > > > > + default: > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (vsc8531->clkout_enabled) > > > > + val |= CLKOUT_ENABLE; > > > > + > > > > + rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO, > > > > + MSCC_CLKOUT_CNTL, val); > > > > + if (rc) > > > > + return rc; > > > > +#endif > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > > +static int vsc8531_clkout_prepare(struct clk_hw *hw) > > > > +{ > > > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > > > > + > > > > + vsc8531->clkout_enabled = true; > > > > + return 0; > > > > +} > > > > + > > > > +static void vsc8531_clkout_unprepare(struct clk_hw *hw) > > > > +{ > > > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > > > > + > > > > + vsc8531->clkout_enabled = false; > > > > +} > > > > + > > > > > > > +static const struct clk_ops vsc8531_clkout_ops = { > > > > + .prepare = vsc8531_clkout_prepare, > > > > + .unprepare = vsc8531_clkout_unprepare, > > > > + .is_prepared = vsc8531_clkout_is_prepared, > > > > + .recalc_rate = vsc8531_clkout_recalc_rate, > > > > + .round_rate = vsc8531_clkout_round_rate, > > > > + .set_rate = vsc8531_clkout_set_rate, > > > > > > I'm not sure this is the expected behaviour. The clk itself should > > > only start ticking when the enable callback is called. But this code > > > will enable the clock when config_init() is called. I think you should > > > implement the enable and disable methods. > > > > That is actually incorrect. The whole "prepare" vs "enable" difference > > is that prepare can schedule, enable isn't permitted. So, if you need > > to sleep to enable the clock, then enabling the clock in the prepare > > callback is the right thing to do. > > > > However, the above driver just sets a flag, which only gets used when > > the PHY's config_init method is called; that really doesn't seem to be > > sane - the clock is available from the point that the PHY has been > > probed, and it'll be expected that once the clock is published, it can > > be made functional. > > Though I'm not sure how this fits in the whole bringup of ethernet phys. > Like the phy is dependent on the underlying ethernet controller to > actually turn it on. > > I guess we should check the phy-state and if it's not accessible, just > keep the values and if it's in a suitable state do the configuration. > > Calling a vsc8531_config_clkout() from both the vsc8531_config_init() > as well as the clk_(un-)prepare and clk_set_rate functions and being > protected by a check against phy_is_started() ? It sounds like it doesn't actually fit the clk API paradym then. I see that Rob suggested it, and from the DT point of view, it makes complete sense, but then if the hardware can't actually be used in the way the clk API expects it to be used, then there's a semantic problem. What is this clock used for?
On Thu, 18 Jun 2020 14:11:39 +0200 Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > At least VSC8530/8531/8540/8541 contain a clock output that can emit > a predefined rate of 25, 50 or 125MHz. > > This may then feed back into the network interface as source clock. > So expose a clock-provider from the phy using the common clock framework > to allow setting the rate. > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> Doesn't build with allmodconfig: ../drivers/net/phy/mscc/mscc_macsec.c:391:42: warning: cast from restricted sci_t ../drivers/net/phy/mscc/mscc_macsec.c:393:42: warning: restricted sci_t degrades to integer ../drivers/net/phy/mscc/mscc_macsec.c:400:42: warning: restricted __be16 degrades to integer ../drivers/net/phy/mscc/mscc_macsec.c:606:34: warning: cast from restricted sci_t ../drivers/net/phy/mscc/mscc_macsec.c:608:34: warning: restricted sci_t degrades to integer ../drivers/net/phy/mscc/mscc_macsec.c:391:42: warning: cast from restricted sci_t ../drivers/net/phy/mscc/mscc_macsec.c:393:42: warning: restricted sci_t degrades to integer ../drivers/net/phy/mscc/mscc_macsec.c:400:42: warning: restricted __be16 degrades to integer ../drivers/net/phy/mscc/mscc_macsec.c:606:34: warning: cast from restricted sci_t ../drivers/net/phy/mscc/mscc_macsec.c:608:34: warning: restricted sci_t degrades to integer In file included from ../drivers/net/phy/mscc/mscc_macsec.c:17: ../drivers/net/phy/mscc/mscc.h:371:16: error: field ‘clkout_hw’ has incomplete type 371 | struct clk_hw clkout_hw; | ^~~~~~~~~ make[5]: *** [drivers/net/phy/mscc/mscc_macsec.o] Error 1 make[5]: *** Waiting for unfinished jobs.... make[4]: *** [drivers/net/phy/mscc] Error 2 make[3]: *** [drivers/net/phy] Error 2 make[3]: *** Waiting for unfinished jobs.... make[2]: *** [drivers/net] Error 2 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [drivers] Error 2 make: *** [__sub-make] Error 2
Am Donnerstag, 18. Juni 2020, 17:47:48 CEST schrieb Russell King - ARM Linux admin: > On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote: > > Am Donnerstag, 18. Juni 2020, 15:41:02 CEST schrieb Russell King - ARM Linux admin: > > > On Thu, Jun 18, 2020 at 03:28:22PM +0200, Andrew Lunn wrote: > > > > On Thu, Jun 18, 2020 at 02:11:39PM +0200, Heiko Stuebner wrote: > > > > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > > > > > > > > At least VSC8530/8531/8540/8541 contain a clock output that can emit > > > > > a predefined rate of 25, 50 or 125MHz. > > > > > > > > > > This may then feed back into the network interface as source clock. > > > > > So expose a clock-provider from the phy using the common clock framework > > > > > to allow setting the rate. > > > > > > > > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > > > --- > > > > > drivers/net/phy/mscc/mscc.h | 13 +++ > > > > > drivers/net/phy/mscc/mscc_main.c | 182 +++++++++++++++++++++++++++++-- > > > > > 2 files changed, 187 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h > > > > > index fbcee5fce7b2..94883dab5cc1 100644 > > > > > --- a/drivers/net/phy/mscc/mscc.h > > > > > +++ b/drivers/net/phy/mscc/mscc.h > > > > > @@ -218,6 +218,13 @@ enum rgmii_clock_delay { > > > > > #define INT_MEM_DATA_M 0x00ff > > > > > #define INT_MEM_DATA(x) (INT_MEM_DATA_M & (x)) > > > > > > > > > > +#define MSCC_CLKOUT_CNTL 13 > > > > > +#define CLKOUT_ENABLE BIT(15) > > > > > +#define CLKOUT_FREQ_MASK GENMASK(14, 13) > > > > > +#define CLKOUT_FREQ_25M (0x0 << 13) > > > > > +#define CLKOUT_FREQ_50M (0x1 << 13) > > > > > +#define CLKOUT_FREQ_125M (0x2 << 13) > > > > > + > > > > > #define MSCC_PHY_PROC_CMD 18 > > > > > #define PROC_CMD_NCOMPLETED 0x8000 > > > > > #define PROC_CMD_FAILED 0x4000 > > > > > @@ -360,6 +367,12 @@ struct vsc8531_private { > > > > > */ > > > > > unsigned int base_addr; > > > > > > > > > > +#ifdef CONFIG_COMMON_CLK > > > > > + struct clk_hw clkout_hw; > > > > > +#endif > > > > > + u32 clkout_rate; > > > > > + int clkout_enabled; > > > > > + > > > > > #if IS_ENABLED(CONFIG_MACSEC) > > > > > /* MACsec fields: > > > > > * - One SecY per device (enforced at the s/w implementation level) > > > > > diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c > > > > > index 5d2777522fb4..727a9dd58403 100644 > > > > > --- a/drivers/net/phy/mscc/mscc_main.c > > > > > +++ b/drivers/net/phy/mscc/mscc_main.c > > > > > @@ -7,6 +7,7 @@ > > > > > * Copyright (c) 2016 Microsemi Corporation > > > > > */ > > > > > > > > > > +#include <linux/clk-provider.h> > > > > > #include <linux/firmware.h> > > > > > #include <linux/jiffies.h> > > > > > #include <linux/kernel.h> > > > > > @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev, > > > > > > > > > > return led_mode; > > > > > } > > > > > - > > > > > #else > > > > > static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev) > > > > > { > > > > > @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev) > > > > > return 0; > > > > > } > > > > > > > > > > +static int vsc8531_config_init(struct phy_device *phydev) > > > > > +{ > > > > > + struct vsc8531_private *vsc8531 = phydev->priv; > > > > > + u16 val; > > > > > + int rc; > > > > > + > > > > > + rc = vsc85xx_config_init(phydev); > > > > > + if (rc) > > > > > + return rc; > > > > > + > > > > > +#ifdef CONFIG_COMMON_CLK > > > > > + switch (vsc8531->clkout_rate) { > > > > > + case 25000000: > > > > > + val = CLKOUT_FREQ_25M; > > > > > + break; > > > > > + case 50000000: > > > > > + val = CLKOUT_FREQ_50M; > > > > > + break; > > > > > + case 125000000: > > > > > + val = CLKOUT_FREQ_125M; > > > > > + break; > > > > > + default: > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + if (vsc8531->clkout_enabled) > > > > > + val |= CLKOUT_ENABLE; > > > > > + > > > > > + rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO, > > > > > + MSCC_CLKOUT_CNTL, val); > > > > > + if (rc) > > > > > + return rc; > > > > > +#endif > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > > > > > +static int vsc8531_clkout_prepare(struct clk_hw *hw) > > > > > +{ > > > > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > > > > > + > > > > > + vsc8531->clkout_enabled = true; > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static void vsc8531_clkout_unprepare(struct clk_hw *hw) > > > > > +{ > > > > > + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); > > > > > + > > > > > + vsc8531->clkout_enabled = false; > > > > > +} > > > > > + > > > > > > > > > +static const struct clk_ops vsc8531_clkout_ops = { > > > > > + .prepare = vsc8531_clkout_prepare, > > > > > + .unprepare = vsc8531_clkout_unprepare, > > > > > + .is_prepared = vsc8531_clkout_is_prepared, > > > > > + .recalc_rate = vsc8531_clkout_recalc_rate, > > > > > + .round_rate = vsc8531_clkout_round_rate, > > > > > + .set_rate = vsc8531_clkout_set_rate, > > > > > > > > I'm not sure this is the expected behaviour. The clk itself should > > > > only start ticking when the enable callback is called. But this code > > > > will enable the clock when config_init() is called. I think you should > > > > implement the enable and disable methods. > > > > > > That is actually incorrect. The whole "prepare" vs "enable" difference > > > is that prepare can schedule, enable isn't permitted. So, if you need > > > to sleep to enable the clock, then enabling the clock in the prepare > > > callback is the right thing to do. > > > > > > However, the above driver just sets a flag, which only gets used when > > > the PHY's config_init method is called; that really doesn't seem to be > > > sane - the clock is available from the point that the PHY has been > > > probed, and it'll be expected that once the clock is published, it can > > > be made functional. > > > > Though I'm not sure how this fits in the whole bringup of ethernet phys. > > Like the phy is dependent on the underlying ethernet controller to > > actually turn it on. > > > > I guess we should check the phy-state and if it's not accessible, just > > keep the values and if it's in a suitable state do the configuration. > > > > Calling a vsc8531_config_clkout() from both the vsc8531_config_init() > > as well as the clk_(un-)prepare and clk_set_rate functions and being > > protected by a check against phy_is_started() ? > > It sounds like it doesn't actually fit the clk API paradym then. I > see that Rob suggested it, and from the DT point of view, it makes > complete sense, but then if the hardware can't actually be used in > the way the clk API expects it to be used, then there's a semantic > problem. > > What is this clock used for? It provides a source for the mac-clk for the actual transfers, here to provide the 125MHz clock needed for the RGMII interface . So right now the old rk3368-lion devicetree just declares a stub fixed-clock and instructs the soc's clock controller to use it [0] . And in the cover-letter here, I show the update variant with using the clock defined here. I've added the idea from my previous mail like shown below [1]. which would take into account the phy-state. But I guess I'll wait for more input before spamming people with v6. Thanks Heiko [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3368-lion.dtsi#n150 [1] @@ -1508,6 +1508,157 @@ static int vsc85xx_config_init(struct phy_device *phydev) return 0; } +#ifdef CONFIG_COMMON_CLK +#define clkout_hw_to_vsc8531(_hw) container_of(_hw, struct vsc8531_private, clkout_hw) + +static int clkout_rates[] = { + 125000000, + 50000000, + 25000000, +}; + +static int vsc8531_config_clkout(struct phy_device *phydev) +{ + struct vsc8531_private *vsc8531 = phydev->priv; + u16 val; + + /* when called from clk functions, make sure phy is running */ + if (phy_is_started(phydev)) + return 0; + + switch (vsc8531->clkout_rate) { + case 25000000: + val = CLKOUT_FREQ_25M; + break; + case 50000000: + val = CLKOUT_FREQ_50M; + break; + case 125000000: + val = CLKOUT_FREQ_125M; + break; + default: + return -EINVAL; + } + + if (vsc8531->clkout_enabled) + val |= CLKOUT_ENABLE; + + return phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO, + MSCC_CLKOUT_CNTL, val); +} + +static unsigned long vsc8531_clkout_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + + return vsc8531->clkout_rate; +} + +static long vsc8531_clkout_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *prate) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(clkout_rates); i++) + if (clkout_rates[i] <= rate) + return clkout_rates[i]; + return 0; +} + +static int vsc8531_clkout_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + struct phy_device *phydev = vsc8531->phydev; + + vsc8531->clkout_rate = rate; + return vsc8531_config_clkout(phydev); +} + +static int vsc8531_clkout_prepare(struct clk_hw *hw) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + struct phy_device *phydev = vsc8531->phydev; + + vsc8531->clkout_enabled = true; + return vsc8531_config_clkout(phydev); +} + +static void vsc8531_clkout_unprepare(struct clk_hw *hw) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + struct phy_device *phydev = vsc8531->phydev; + + vsc8531->clkout_enabled = false; + vsc8531_config_clkout(phydev); +} + +static int vsc8531_clkout_is_prepared(struct clk_hw *hw) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + + return vsc8531->clkout_enabled; +} + +static const struct clk_ops vsc8531_clkout_ops = { + .prepare = vsc8531_clkout_prepare, + .unprepare = vsc8531_clkout_unprepare, + .is_prepared = vsc8531_clkout_is_prepared, + .recalc_rate = vsc8531_clkout_recalc_rate, + .round_rate = vsc8531_clkout_round_rate, + .set_rate = vsc8531_clkout_set_rate, +}; + +static int vsc8531_register_clkout(struct phy_device *phydev) +{ + struct vsc8531_private *vsc8531 = phydev->priv; + struct device *dev = &phydev->mdio.dev; + struct device_node *of_node = dev->of_node; + struct clk_init_data init; + int ret; + + init.name = "vsc8531-clkout"; + init.ops = &vsc8531_clkout_ops; + init.flags = 0; + init.parent_names = NULL; + init.num_parents = 0; + vsc8531->clkout_hw.init = &init; + + /* optional override of the clockname */ + of_property_read_string(of_node, "clock-output-names", &init.name); + + /* register the clock */ + ret = devm_clk_hw_register(dev, &vsc8531->clkout_hw); + if (!ret) + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, + &vsc8531->clkout_hw); + + return ret; +} +#else +static int vsc8531_register_clkout(struct phy_device *phydev) +{ + return 0; +} + +static int vsc8531_config_clkout(struct phy_device *phydev) +{ + return 0; +} +#endif + +static int vsc8531_config_init(struct phy_device *phydev) +{ + int rc; + + rc = vsc85xx_config_init(phydev); + if (rc) + return rc; + + return vsc8531_config_clkout(phydev); +} +
On Thu, Jun 18, 2020 at 06:01:29PM +0200, Heiko Stübner wrote: > Am Donnerstag, 18. Juni 2020, 17:47:48 CEST schrieb Russell King - ARM Linux admin: > > On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote: > > > Though I'm not sure how this fits in the whole bringup of ethernet phys. > > > Like the phy is dependent on the underlying ethernet controller to > > > actually turn it on. > > > > > > I guess we should check the phy-state and if it's not accessible, just > > > keep the values and if it's in a suitable state do the configuration. > > > > > > Calling a vsc8531_config_clkout() from both the vsc8531_config_init() > > > as well as the clk_(un-)prepare and clk_set_rate functions and being > > > protected by a check against phy_is_started() ? > > > > It sounds like it doesn't actually fit the clk API paradym then. I > > see that Rob suggested it, and from the DT point of view, it makes > > complete sense, but then if the hardware can't actually be used in > > the way the clk API expects it to be used, then there's a semantic > > problem. > > > > What is this clock used for? > > It provides a source for the mac-clk for the actual transfers, here to > provide the 125MHz clock needed for the RGMII interface . > > So right now the old rk3368-lion devicetree just declares a stub > fixed-clock and instructs the soc's clock controller to use it [0] . > And in the cover-letter here, I show the update variant with using > the clock defined here. > > > I've added the idea from my previous mail like shown below [1]. > which would take into account the phy-state. > > But I guess I'll wait for more input before spamming people with v6. Let's get a handle on exactly what this is. The RGMII bus has two clocks: RXC and TXC, which are clocked at one of 125MHz, 25MHz or 2.5MHz depending on the RGMII data rate. Some PHYs replace TXC with GTX clock, which always runs at 125MHz. These clocks are not what you're referring to. You are referring to another commonly provided clock between the MAC and the PHY, something which is not unique to your PHY. We seem to be heading down a path where different PHYs end up doing different things in DT for what is basically the same hardware setup, which really isn't good. :( We have at803x using: qca,clk-out-frequency qca,clk-out-strength qca,keep-pll-enabled which are used to control the CLK_25M output pin on the device, which may be used to provide a reference clock for the MAC side, selecting between 25M, 50M, 62.5M and 125MHz. This was introduced in November 2019, so not that long ago. Broadcom PHYs configure their 125MHz clock through the PHY device flags passed from the MAC at attach/connect time. There's the dp83867 and dp83869 configuration (I'm not sure I can make sense of it from reading the code) using ti,clk-output-sel - but it looks like it's the same kind of thing. Introduced February 2018 into one driver, and November 2019 in the other. It seems the Micrel PHYs produce a 125MHz clock irrespective of any configuration (maybe configured by firmware, or hardware strapping?) So it seems we have four ways of doing the same thing today, and now the suggestion is to implement a fifth different way. I think there needs to be some consolidation here, maybe choosing one approach and sticking with it. Hence, I disagree with Rob - we don't need a fifth approach, we need to choose one approach and decide that's our policy for this and apply it evenly across the board, rather than making up something different each time a new PHY comes along.
Hi Heiko,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on sparc-next/master net-next/master net/master linus/master v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Heiko-Stuebner/add-clkout-support-to-mscc-phys/20200618-201732
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/net/phy/mscc/mscc_macsec.c:17:
>> drivers/net/phy/mscc/mscc.h:371:16: error: field has incomplete type 'struct clk_hw'
struct clk_hw clkout_hw;
^
drivers/net/phy/mscc/mscc.h:371:9: note: forward declaration of 'struct clk_hw'
struct clk_hw clkout_hw;
^
1 error generated.
vim +371 drivers/net/phy/mscc/mscc.h
354
355 struct vsc8531_private {
356 int rate_magic;
357 u16 supp_led_modes;
358 u32 leds_mode[MAX_LEDS];
359 u8 nleds;
360 const struct vsc85xx_hw_stat *hw_stats;
361 u64 *stats;
362 int nstats;
363 /* PHY address within the package. */
364 u8 addr;
365 /* For multiple port PHYs; the MDIO address of the base PHY in the
366 * package.
367 */
368 unsigned int base_addr;
369
370 #ifdef CONFIG_COMMON_CLK
> 371 struct clk_hw clkout_hw;
372 #endif
373 u32 clkout_rate;
374 int clkout_enabled;
375
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Am Donnerstag, 18. Juni 2020, 18:40:15 CEST schrieb Russell King - ARM Linux admin: > On Thu, Jun 18, 2020 at 06:01:29PM +0200, Heiko Stübner wrote: > > Am Donnerstag, 18. Juni 2020, 17:47:48 CEST schrieb Russell King - ARM Linux admin: > > > On Thu, Jun 18, 2020 at 05:41:54PM +0200, Heiko Stübner wrote: > > > > Though I'm not sure how this fits in the whole bringup of ethernet phys. > > > > Like the phy is dependent on the underlying ethernet controller to > > > > actually turn it on. > > > > > > > > I guess we should check the phy-state and if it's not accessible, just > > > > keep the values and if it's in a suitable state do the configuration. > > > > > > > > Calling a vsc8531_config_clkout() from both the vsc8531_config_init() > > > > as well as the clk_(un-)prepare and clk_set_rate functions and being > > > > protected by a check against phy_is_started() ? > > > > > > It sounds like it doesn't actually fit the clk API paradym then. I > > > see that Rob suggested it, and from the DT point of view, it makes > > > complete sense, but then if the hardware can't actually be used in > > > the way the clk API expects it to be used, then there's a semantic > > > problem. > > > > > > What is this clock used for? > > > > It provides a source for the mac-clk for the actual transfers, here to > > provide the 125MHz clock needed for the RGMII interface . > > > > So right now the old rk3368-lion devicetree just declares a stub > > fixed-clock and instructs the soc's clock controller to use it [0] . > > And in the cover-letter here, I show the update variant with using > > the clock defined here. > > > > > > I've added the idea from my previous mail like shown below [1]. > > which would take into account the phy-state. > > > > But I guess I'll wait for more input before spamming people with v6. > > Let's get a handle on exactly what this is. > > The RGMII bus has two clocks: RXC and TXC, which are clocked at one > of 125MHz, 25MHz or 2.5MHz depending on the RGMII data rate. Some > PHYs replace TXC with GTX clock, which always runs at 125MHz. These > clocks are not what you're referring to. > > You are referring to another commonly provided clock between the MAC > and the PHY, something which is not unique to your PHY. > > We seem to be heading down a path where different PHYs end up doing > different things in DT for what is basically the same hardware setup, > which really isn't good. :( > > We have at803x using: > > qca,clk-out-frequency > qca,clk-out-strength > qca,keep-pll-enabled > > which are used to control the CLK_25M output pin on the device, which > may be used to provide a reference clock for the MAC side, selecting > between 25M, 50M, 62.5M and 125MHz. This was introduced in November > 2019, so not that long ago. Because it was not that old, was the reason I followed that example in my v1 :-) And Andrew then suggested to at least try to use a common property for the target frequency that other phys could migrate to. > Broadcom PHYs configure their 125MHz clock through the PHY device > flags passed from the MAC at attach/connect time. > > There's the dp83867 and dp83869 configuration (I'm not sure I can > make sense of it from reading the code) using ti,clk-output-sel - > but it looks like it's the same kind of thing. Introduced February > 2018 into one driver, and November 2019 in the other. > > It seems the Micrel PHYs produce a 125MHz clock irrespective of any > configuration (maybe configured by firmware, or hardware strapping?) > > So it seems we have four ways of doing the same thing today, and now > the suggestion is to implement a fifth different way. I think there > needs to be some consolidation here, maybe choosing one approach and > sticking with it. > > Hence, I disagree with Rob - we don't need a fifth approach, we need > to choose one approach and decide that's our policy for this and > apply it evenly across the board, rather than making up something > different each time a new PHY comes along. @Dave, @Andrew: what's you opinion here? As Russell above (and Florian in the binding patch) pointed out, integrating this into the clock-framework may prove difficult not only for consistency but also for dependency reasons. Personally I'm fine with either solution, I just want to achieve a working ethernet on my board :-D . Thanks Heiko
diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h index fbcee5fce7b2..94883dab5cc1 100644 --- a/drivers/net/phy/mscc/mscc.h +++ b/drivers/net/phy/mscc/mscc.h @@ -218,6 +218,13 @@ enum rgmii_clock_delay { #define INT_MEM_DATA_M 0x00ff #define INT_MEM_DATA(x) (INT_MEM_DATA_M & (x)) +#define MSCC_CLKOUT_CNTL 13 +#define CLKOUT_ENABLE BIT(15) +#define CLKOUT_FREQ_MASK GENMASK(14, 13) +#define CLKOUT_FREQ_25M (0x0 << 13) +#define CLKOUT_FREQ_50M (0x1 << 13) +#define CLKOUT_FREQ_125M (0x2 << 13) + #define MSCC_PHY_PROC_CMD 18 #define PROC_CMD_NCOMPLETED 0x8000 #define PROC_CMD_FAILED 0x4000 @@ -360,6 +367,12 @@ struct vsc8531_private { */ unsigned int base_addr; +#ifdef CONFIG_COMMON_CLK + struct clk_hw clkout_hw; +#endif + u32 clkout_rate; + int clkout_enabled; + #if IS_ENABLED(CONFIG_MACSEC) /* MACsec fields: * - One SecY per device (enforced at the s/w implementation level) diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c index 5d2777522fb4..727a9dd58403 100644 --- a/drivers/net/phy/mscc/mscc_main.c +++ b/drivers/net/phy/mscc/mscc_main.c @@ -7,6 +7,7 @@ * Copyright (c) 2016 Microsemi Corporation */ +#include <linux/clk-provider.h> #include <linux/firmware.h> #include <linux/jiffies.h> #include <linux/kernel.h> @@ -431,7 +432,6 @@ static int vsc85xx_dt_led_mode_get(struct phy_device *phydev, return led_mode; } - #else static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev) { @@ -1508,6 +1508,43 @@ static int vsc85xx_config_init(struct phy_device *phydev) return 0; } +static int vsc8531_config_init(struct phy_device *phydev) +{ + struct vsc8531_private *vsc8531 = phydev->priv; + u16 val; + int rc; + + rc = vsc85xx_config_init(phydev); + if (rc) + return rc; + +#ifdef CONFIG_COMMON_CLK + switch (vsc8531->clkout_rate) { + case 25000000: + val = CLKOUT_FREQ_25M; + break; + case 50000000: + val = CLKOUT_FREQ_50M; + break; + case 125000000: + val = CLKOUT_FREQ_125M; + break; + default: + return -EINVAL; + } + + if (vsc8531->clkout_enabled) + val |= CLKOUT_ENABLE; + + rc = phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO, + MSCC_CLKOUT_CNTL, val); + if (rc) + return rc; +#endif + + return 0; +} + static int vsc8584_did_interrupt(struct phy_device *phydev) { int rc = 0; @@ -1935,6 +1972,107 @@ static int vsc85xx_read_status(struct phy_device *phydev) return genphy_read_status(phydev); } +#ifdef CONFIG_COMMON_CLK +#define clkout_hw_to_vsc8531(_hw) container_of(_hw, struct vsc8531_private, clkout_hw) + +static int clkout_rates[] = { + 125000000, + 50000000, + 25000000, +}; + +static unsigned long vsc8531_clkout_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + + return vsc8531->clkout_rate; +} + +static long vsc8531_clkout_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *prate) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(clkout_rates); i++) + if (clkout_rates[i] <= rate) + return clkout_rates[i]; + return 0; +} + +static int vsc8531_clkout_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + + vsc8531->clkout_rate = rate; + return 0; +} + +static int vsc8531_clkout_prepare(struct clk_hw *hw) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + + vsc8531->clkout_enabled = true; + return 0; +} + +static void vsc8531_clkout_unprepare(struct clk_hw *hw) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + + vsc8531->clkout_enabled = false; +} + +static int vsc8531_clkout_is_prepared(struct clk_hw *hw) +{ + struct vsc8531_private *vsc8531 = clkout_hw_to_vsc8531(hw); + + return vsc8531->clkout_enabled; +} + +static const struct clk_ops vsc8531_clkout_ops = { + .prepare = vsc8531_clkout_prepare, + .unprepare = vsc8531_clkout_unprepare, + .is_prepared = vsc8531_clkout_is_prepared, + .recalc_rate = vsc8531_clkout_recalc_rate, + .round_rate = vsc8531_clkout_round_rate, + .set_rate = vsc8531_clkout_set_rate, +}; + +static int vsc8531_register_clkout(struct phy_device *phydev) +{ + struct vsc8531_private *vsc8531 = phydev->priv; + struct device *dev = &phydev->mdio.dev; + struct device_node *of_node = dev->of_node; + struct clk_init_data init; + int ret; + + init.name = "vsc8531-clkout"; + init.ops = &vsc8531_clkout_ops; + init.flags = 0; + init.parent_names = NULL; + init.num_parents = 0; + vsc8531->clkout_hw.init = &init; + + /* optional override of the clockname */ + of_property_read_string(of_node, "clock-output-names", &init.name); + + /* register the clock */ + ret = devm_clk_hw_register(dev, &vsc8531->clkout_hw); + if (!ret) + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, + &vsc8531->clkout_hw); + + return ret; +} +#else +static int vsc8531_register_clkout(struct phy_device *phydev) +{ + return 0; +} +#endif + static int vsc85xx_probe_helper(struct phy_device *phydev, u32 *leds, int num_leds, u16 led_modes, const struct vsc85xx_hw_stat *stats, int nstats) @@ -1981,6 +2119,34 @@ static int vsc8514_probe(struct phy_device *phydev) vsc8531->base_addr, 0); } +static int vsc8531_probe(struct phy_device *phydev) +{ + struct vsc8531_private *vsc8531; + int rate_magic, rc; + u32 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY, + VSC8531_LINK_100_ACTIVITY}; + + rate_magic = vsc85xx_edge_rate_magic_get(phydev); + if (rate_magic < 0) + return rate_magic; + + rc = vsc85xx_probe_helper(phydev, default_mode, + ARRAY_SIZE(default_mode), + VSC85XX_SUPP_LED_MODES, + vsc85xx_hw_stats, + ARRAY_SIZE(vsc85xx_hw_stats)); + if (rc < 0) + return rc; + + vsc8531 = phydev->priv; + vsc8531->rate_magic = rate_magic; + rc = vsc8531_register_clkout(phydev); + if (rc < 0) + return rc; + + return 0; +} + static int vsc8574_probe(struct phy_device *phydev) { struct vsc8531_private *vsc8531; @@ -2136,14 +2302,14 @@ static struct phy_driver vsc85xx_driver[] = { .phy_id_mask = 0xfffffff0, /* PHY_BASIC_FEATURES */ .soft_reset = &genphy_soft_reset, - .config_init = &vsc85xx_config_init, + .config_init = &vsc8531_config_init, .config_aneg = &vsc85xx_config_aneg, .read_status = &vsc85xx_read_status, .ack_interrupt = &vsc85xx_ack_interrupt, .config_intr = &vsc85xx_config_intr, .suspend = &genphy_suspend, .resume = &genphy_resume, - .probe = &vsc85xx_probe, + .probe = &vsc8531_probe, .set_wol = &vsc85xx_wol_set, .get_wol = &vsc85xx_wol_get, .get_tunable = &vsc85xx_get_tunable, @@ -2160,14 +2326,14 @@ static struct phy_driver vsc85xx_driver[] = { .phy_id_mask = 0xfffffff0, /* PHY_GBIT_FEATURES */ .soft_reset = &genphy_soft_reset, - .config_init = &vsc85xx_config_init, + .config_init = &vsc8531_config_init, .config_aneg = &vsc85xx_config_aneg, .read_status = &vsc85xx_read_status, .ack_interrupt = &vsc85xx_ack_interrupt, .config_intr = &vsc85xx_config_intr, .suspend = &genphy_suspend, .resume = &genphy_resume, - .probe = &vsc85xx_probe, + .probe = &vsc8531_probe, .set_wol = &vsc85xx_wol_set, .get_wol = &vsc85xx_wol_get, .get_tunable = &vsc85xx_get_tunable, @@ -2184,14 +2350,14 @@ static struct phy_driver vsc85xx_driver[] = { .phy_id_mask = 0xfffffff0, /* PHY_BASIC_FEATURES */ .soft_reset = &genphy_soft_reset, - .config_init = &vsc85xx_config_init, + .config_init = &vsc8531_config_init, .config_aneg = &vsc85xx_config_aneg, .read_status = &vsc85xx_read_status, .ack_interrupt = &vsc85xx_ack_interrupt, .config_intr = &vsc85xx_config_intr, .suspend = &genphy_suspend, .resume = &genphy_resume, - .probe = &vsc85xx_probe, + .probe = &vsc8531_probe, .set_wol = &vsc85xx_wol_set, .get_wol = &vsc85xx_wol_get, .get_tunable = &vsc85xx_get_tunable, @@ -2208,7 +2374,7 @@ static struct phy_driver vsc85xx_driver[] = { .phy_id_mask = 0xfffffff0, /* PHY_GBIT_FEATURES */ .soft_reset = &genphy_soft_reset, - .config_init = &vsc85xx_config_init, + .config_init = &vsc8531_config_init, .config_aneg = &vsc85xx_config_aneg, .read_status = &vsc85xx_read_status, .ack_interrupt = &vsc85xx_ack_interrupt,