| Message ID | 20200831134836.20189-5-m.felsch@pengutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | SMSC: Cleanups and clock setup | expand |
> + priv->refclk = devm_clk_get_optional(dev, NULL); > + if (IS_ERR(priv->refclk)) { > + if (PTR_ERR(priv->refclk) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + /* Clocks are optional all errors should be ignored here */ > + return 0; Since you are calling devm_clk_get_optional() isn't an error a real error, not that the clock is missing? It probably should be returned as an error code. Andrew
On 8/31/2020 6:48 AM, Marco Felsch wrote: > Add support to specify the clock provider for the phy refclk and don't > rely on 'magic' host clock setup. [1] tried to address this by > introducing a flag and fixing the corresponding host. But this commit > breaks the IRQ support since the irq setup during .config_intr() is > thrown away because the reset comes from the side without respecting the > current phy-state within the phy-state-machine. Furthermore the commit > fixed the problem only for FEC based hosts other hosts acting like the > FEC are not covered. > > This commit goes the other way around to address the bug fixed by [1]. > Instead of resetting the device from the side every time the refclk gets > (re-)enabled it requests and enables the clock till the device gets > removed. The phy is still rest but now within the phylib and with > respect to the phy-state-machine. > > [1] commit 7f64e5b18ebb ("net: phy: smsc: LAN8710/20: add > PHY_RST_AFTER_CLK_EN flag") > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/net/phy/smsc.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c > index 79574fcbd880..b98a7845681f 100644 > --- a/drivers/net/phy/smsc.c > +++ b/drivers/net/phy/smsc.c > @@ -12,6 +12,7 @@ > * > */ > > +#include <linux/clk.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/mii.h> > @@ -33,6 +34,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = { > > struct smsc_phy_priv { > bool energy_enable; > + struct clk *refclk; > }; > > static int smsc_phy_config_intr(struct phy_device *phydev) > @@ -194,11 +196,19 @@ static void smsc_get_stats(struct phy_device *phydev, > data[i] = smsc_get_stat(phydev, i); > } > > +static void smsc_clk_disable_action(void *data) > +{ > + struct smsc_phy_priv *priv = data; > + > + clk_disable_unprepare(priv->refclk); > +} > + > static int smsc_phy_probe(struct phy_device *phydev) > { > struct device *dev = &phydev->mdio.dev; > struct device_node *of_node = dev->of_node; > struct smsc_phy_priv *priv; > + int ret; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > @@ -211,6 +221,26 @@ static int smsc_phy_probe(struct phy_device *phydev) > > phydev->priv = priv; > > + priv->refclk = devm_clk_get_optional(dev, NULL); > + if (IS_ERR(priv->refclk)) { > + if (PTR_ERR(priv->refclk) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + /* Clocks are optional all errors should be ignored here */ > + return 0; > + } > + > + /* Starting from here errors should not be ignored anymore */ > + ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000); > + if (ret) > + return ret; The clock should be enabled first before attempting a rate change, and this also causes a more fundamental question: what is the sate of the clock when the PHY driver is probed, and is the reference clock feeding into the MDIO logic of the PHY. By that I mean that if the reference clock was disabled, would the PHY still respond to MDIO reads such that you would be able to probe and identify it? If not, your demv_clk_get_optional() is either too late, or assuming a prior state, or you are working around this in Device Tree by using a compatible string with the form "^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$" in which case, this is a making assumptions about how the OF MDIO layer works which is not ideal. I am preparing some patches that aim at enabling a given MDIO device's clock prior to probing it and should be able to post them by today. > + > + ret = clk_prepare_enable(priv->refclk); > + if (ret) > + return ret; > + > + devm_add_action_or_reset(dev, smsc_clk_disable_action, priv); > + > return 0; > } > >
On 20-08-31 16:08, Andrew Lunn wrote: > > + priv->refclk = devm_clk_get_optional(dev, NULL); > > + if (IS_ERR(priv->refclk)) { > > + if (PTR_ERR(priv->refclk) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > + /* Clocks are optional all errors should be ignored here */ > > + return 0; > > Since you are calling devm_clk_get_optional() isn't an error a real > error, not that the clock is missing? It probably should be returned > as an error code. Yes you're right. Actually I can't remember why went this way... I will change this to dev_err_probe() and this gets a oneliner. Regards, Marco > > Andrew >
On 20-08-31 09:32, Florian Fainelli wrote: > > > On 8/31/2020 6:48 AM, Marco Felsch wrote: > > Add support to specify the clock provider for the phy refclk and don't > > rely on 'magic' host clock setup. [1] tried to address this by > > introducing a flag and fixing the corresponding host. But this commit > > breaks the IRQ support since the irq setup during .config_intr() is > > thrown away because the reset comes from the side without respecting the > > current phy-state within the phy-state-machine. Furthermore the commit > > fixed the problem only for FEC based hosts other hosts acting like the > > FEC are not covered. > > > > This commit goes the other way around to address the bug fixed by [1]. > > Instead of resetting the device from the side every time the refclk gets > > (re-)enabled it requests and enables the clock till the device gets > > removed. The phy is still rest but now within the phylib and with > > respect to the phy-state-machine. > > > > [1] commit 7f64e5b18ebb ("net: phy: smsc: LAN8710/20: add > > PHY_RST_AFTER_CLK_EN flag") > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/net/phy/smsc.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c > > index 79574fcbd880..b98a7845681f 100644 > > --- a/drivers/net/phy/smsc.c > > +++ b/drivers/net/phy/smsc.c > > @@ -12,6 +12,7 @@ > > * > > */ > > +#include <linux/clk.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/mii.h> > > @@ -33,6 +34,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = { > > struct smsc_phy_priv { > > bool energy_enable; > > + struct clk *refclk; > > }; > > static int smsc_phy_config_intr(struct phy_device *phydev) > > @@ -194,11 +196,19 @@ static void smsc_get_stats(struct phy_device *phydev, > > data[i] = smsc_get_stat(phydev, i); > > } > > +static void smsc_clk_disable_action(void *data) > > +{ > > + struct smsc_phy_priv *priv = data; > > + > > + clk_disable_unprepare(priv->refclk); > > +} > > + > > static int smsc_phy_probe(struct phy_device *phydev) > > { > > struct device *dev = &phydev->mdio.dev; > > struct device_node *of_node = dev->of_node; > > struct smsc_phy_priv *priv; > > + int ret; > > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > if (!priv) > > @@ -211,6 +221,26 @@ static int smsc_phy_probe(struct phy_device *phydev) > > phydev->priv = priv; > > + priv->refclk = devm_clk_get_optional(dev, NULL); > > + if (IS_ERR(priv->refclk)) { > > + if (PTR_ERR(priv->refclk) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > + /* Clocks are optional all errors should be ignored here */ > > + return 0; > > + } > > + > > + /* Starting from here errors should not be ignored anymore */ > > + ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000); > > + if (ret) > > + return ret; > > The clock should be enabled first before attempting a rate change Is this the way to use the API? My understanding was to set the correct clk value before we enable the clk value can be out-of-range for the phy. But you have a point, we should: ret = clk_prepare(priv->refclk); if (ret) return ret; ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000); if (ret) return ret; ret = clk_enable(priv->refclk); if (ret) return ret; to avoide the usage of unprepared clocks. >, and this > also causes a more fundamental question: what is the sate of the clock when > the PHY driver is probed, and is the reference clock feeding into the MDIO > logic of the PHY. Currently this state is defined by the bootloader if the clk is provided by the host. > By that I mean that if the reference clock was disabled, would the PHY still > respond to MDIO reads such that you would be able to probe and identify it? Pls correct me if I'm wrong but currently all phy drivers relying on the settings made by the bootloader/firmware. > If not, your demv_clk_get_optional() is either too late Yes, I got this. > , or assuming a prior state, This is the our case. Isn't it the purpose of the bootloader to setup the HW? > or you are working around this in Device Tree by using a compatible > string with the form "^ethernet-phy-id[a-f0-9]{4}\\.[a-f0-9]{4}$" in which > case, this is a making assumptions about how the OF MDIO layer works which > is not ideal. Nope, I'm using "ethernet-phy-ieee802.3-c22". > I am preparing some patches that aim at enabling a given MDIO device's clock > prior to probing it and should be able to post them by today. Create :) Can you provide me a link? What I can say for now is: This solution was used by the micrel driver too and it seems to work. I wanted to keep the change smaller/more local because the current upstream state is: SMSC-Phy <-> FEC-Host ==> IRQ broken. If your patch fixes this too in a more general matter I'm fine with it and we can drop this patch but we should fix this as soon as possible. Regards, Marco > > + > > + ret = clk_prepare_enable(priv->refclk); > > + if (ret) > > + return ret; > > + > > + devm_add_action_or_reset(dev, smsc_clk_disable_action, priv); > > + > > return 0; > > } > > > > -- > Florian >
> Yes, I got this. > > > , or assuming a prior state, > > This is the our case. Isn't it the purpose of the bootloader to setup > the HW? This is a bit of a philosophical discussion. For PCs developers would definitely agree, the firmware should be setting up most of the hardware. And the firmware is involved in driving the hardware, via ACPI. That works because you mostly cannot replaces the firmware. In the ARM world we tend to take the opposite view. The bootloader does the minimum to get the OS running, and the OS then setups up everything. Often there are a choice of bootloaders, you have no idea if the vendor bootload has been replaced by a mainline one with extra features, etc. And we have no idea what the bootloader is actually doing, so we try to assume nothing. Andrew
On 9/1/2020 1:24 AM, Marco Felsch wrote: > Create :) Can you provide me a link? What I can say for now is: This > solution was used by the micrel driver too and it seems to work. I > wanted to keep the change smaller/more local because the current > upstream state is: SMSC-Phy <-> FEC-Host ==> IRQ broken. If your patch > fixes this too in a more general matter I'm fine with it and we can drop > this patch but we should fix this as soon as possible. Still working on it, I found a platform where there were some timing problems and am looking into it. If your SMSC changes are so important and critical, get them applied to the "net" tree, your patch posting makes no mention of which netdev tree you are targeting.
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c index 79574fcbd880..b98a7845681f 100644 --- a/drivers/net/phy/smsc.c +++ b/drivers/net/phy/smsc.c @@ -12,6 +12,7 @@ * */ +#include <linux/clk.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/mii.h> @@ -33,6 +34,7 @@ static struct smsc_hw_stat smsc_hw_stats[] = { struct smsc_phy_priv { bool energy_enable; + struct clk *refclk; }; static int smsc_phy_config_intr(struct phy_device *phydev) @@ -194,11 +196,19 @@ static void smsc_get_stats(struct phy_device *phydev, data[i] = smsc_get_stat(phydev, i); } +static void smsc_clk_disable_action(void *data) +{ + struct smsc_phy_priv *priv = data; + + clk_disable_unprepare(priv->refclk); +} + static int smsc_phy_probe(struct phy_device *phydev) { struct device *dev = &phydev->mdio.dev; struct device_node *of_node = dev->of_node; struct smsc_phy_priv *priv; + int ret; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) @@ -211,6 +221,26 @@ static int smsc_phy_probe(struct phy_device *phydev) phydev->priv = priv; + priv->refclk = devm_clk_get_optional(dev, NULL); + if (IS_ERR(priv->refclk)) { + if (PTR_ERR(priv->refclk) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* Clocks are optional all errors should be ignored here */ + return 0; + } + + /* Starting from here errors should not be ignored anymore */ + ret = clk_set_rate(priv->refclk, 50 * 1000 * 1000); + if (ret) + return ret; + + ret = clk_prepare_enable(priv->refclk); + if (ret) + return ret; + + devm_add_action_or_reset(dev, smsc_clk_disable_action, priv); + return 0; }
Add support to specify the clock provider for the phy refclk and don't rely on 'magic' host clock setup. [1] tried to address this by introducing a flag and fixing the corresponding host. But this commit breaks the IRQ support since the irq setup during .config_intr() is thrown away because the reset comes from the side without respecting the current phy-state within the phy-state-machine. Furthermore the commit fixed the problem only for FEC based hosts other hosts acting like the FEC are not covered. This commit goes the other way around to address the bug fixed by [1]. Instead of resetting the device from the side every time the refclk gets (re-)enabled it requests and enables the clock till the device gets removed. The phy is still rest but now within the phylib and with respect to the phy-state-machine. [1] commit 7f64e5b18ebb ("net: phy: smsc: LAN8710/20: add PHY_RST_AFTER_CLK_EN flag") Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/net/phy/smsc.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)