| Message ID | 20200903043947.3272453-4-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: Support enabling clocks prior to bus probe | expand |
Hi Florian, On 20-09-02 21:39, Florian Fainelli wrote: > The internal Gigabit PHY on Broadcom STB chips has a digital clock which > drives its MDIO interface among other things, the driver now requests > and manage that clock during .probe() and .remove() accordingly. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c > index 692048d86ab1..f0ffcdcaef03 100644 > --- a/drivers/net/phy/bcm7xxx.c > +++ b/drivers/net/phy/bcm7xxx.c > @@ -11,6 +11,7 @@ > #include "bcm-phy-lib.h" > #include <linux/bitops.h> > #include <linux/brcmphy.h> > +#include <linux/clk.h> > #include <linux/mdio.h> > > /* Broadcom BCM7xxx internal PHY registers */ > @@ -39,6 +40,7 @@ > > struct bcm7xxx_phy_priv { > u64 *stats; > + struct clk *clk; > }; > > static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev) > @@ -534,7 +536,19 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) > if (!priv->stats) > return -ENOMEM; > > - return 0; > + priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL); Since the clock is binded to the mdio-dev here.. > + if (IS_ERR(priv->clk)) > + return PTR_ERR(priv->clk); > + > + return clk_prepare_enable(priv->clk); clould we use devm_add_action_or_reset() here so we don't have to register the .remove() hook? > +} > + > +static void bcm7xxx_28nm_remove(struct phy_device *phydev) > +{ > + struct bcm7xxx_phy_priv *priv = phydev->priv; > + > + clk_disable_unprepare(priv->clk); > + devm_clk_put(&phydev->mdio.dev, priv->clk); Is this really necessary? The devm_clk_get_optional() function already registers the devm_clk_release() hook. Regards, Marco > } > > #define BCM7XXX_28NM_GPHY(_oui, _name) \ > @@ -552,6 +566,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) > .get_strings = bcm_phy_get_strings, \ > .get_stats = bcm7xxx_28nm_get_phy_stats, \ > .probe = bcm7xxx_28nm_probe, \ > + .remove = bcm7xxx_28nm_remove, \ > } > > #define BCM7XXX_28NM_EPHY(_oui, _name) \ > @@ -567,6 +582,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) > .get_strings = bcm_phy_get_strings, \ > .get_stats = bcm7xxx_28nm_get_phy_stats, \ > .probe = bcm7xxx_28nm_probe, \ > + .remove = bcm7xxx_28nm_remove, \ > } > > #define BCM7XXX_40NM_EPHY(_oui, _name) \ > -- > 2.25.1 > >
On 20-09-02 21:39, Florian Fainelli wrote: > The internal Gigabit PHY on Broadcom STB chips has a digital clock which > drives its MDIO interface among other things, the driver now requests > and manage that clock during .probe() and .remove() accordingly. Hi Florian, Seems like you added the same support here like I did for the smsc driver. So should I go with my proposed patch which can be adapted later after you guys figured out who to enable the required resources? Regards, Marco
On 9/3/2020 11:15 PM, Marco Felsch wrote: > Hi Florian, > > On 20-09-02 21:39, Florian Fainelli wrote: >> The internal Gigabit PHY on Broadcom STB chips has a digital clock which >> drives its MDIO interface among other things, the driver now requests >> and manage that clock during .probe() and .remove() accordingly. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c >> index 692048d86ab1..f0ffcdcaef03 100644 >> --- a/drivers/net/phy/bcm7xxx.c >> +++ b/drivers/net/phy/bcm7xxx.c >> @@ -11,6 +11,7 @@ >> #include "bcm-phy-lib.h" >> #include <linux/bitops.h> >> #include <linux/brcmphy.h> >> +#include <linux/clk.h> >> #include <linux/mdio.h> >> >> /* Broadcom BCM7xxx internal PHY registers */ >> @@ -39,6 +40,7 @@ >> >> struct bcm7xxx_phy_priv { >> u64 *stats; >> + struct clk *clk; >> }; >> >> static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev) >> @@ -534,7 +536,19 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) >> if (!priv->stats) >> return -ENOMEM; >> >> - return 0; >> + priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL); > > Since the clock is binded to the mdio-dev here.. > >> + if (IS_ERR(priv->clk)) >> + return PTR_ERR(priv->clk); >> + >> + return clk_prepare_enable(priv->clk); > > clould we use devm_add_action_or_reset() here so we don't have to > register the .remove() hook? Maybe, more on that below. > >> +} >> + >> +static void bcm7xxx_28nm_remove(struct phy_device *phydev) >> +{ >> + struct bcm7xxx_phy_priv *priv = phydev->priv; >> + >> + clk_disable_unprepare(priv->clk); >> + devm_clk_put(&phydev->mdio.dev, priv->clk); > > Is this really necessary? The devm_clk_get_optional() function already > registers the devm_clk_release() hook. Yes, because you can unbind the PHY driver from sysfs, and if you want to bind that driver again, which will call .probe() again, you must undo strictly everything that .probe() did. The embedded mdio_device does not go away, so there will be no automatic freeing of resources. Using devm_* may be confusing, so using just the plain clk_get() and clk_put() may be clearer here.
On 9/3/2020 11:18 PM, Marco Felsch wrote: > On 20-09-02 21:39, Florian Fainelli wrote: >> The internal Gigabit PHY on Broadcom STB chips has a digital clock which >> drives its MDIO interface among other things, the driver now requests >> and manage that clock during .probe() and .remove() accordingly. > > Hi Florian, > > Seems like you added the same support here like I did for the smsc > driver. So should I go with my proposed patch which can be adapted later > after you guys figured out who to enable the required resources? That seems fine to me, on your platform there appears to be an assumption that we will be able to probe the SMSC PHY because everything we need is already enabled, right? If so, this patch series does not change that state.
On 20-09-04 08:37, Florian Fainelli wrote: > > > On 9/3/2020 11:15 PM, Marco Felsch wrote: > > Hi Florian, > > > > On 20-09-02 21:39, Florian Fainelli wrote: > > > The internal Gigabit PHY on Broadcom STB chips has a digital clock which > > > drives its MDIO interface among other things, the driver now requests > > > and manage that clock during .probe() and .remove() accordingly. > > > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > --- > > > drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++- > > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c > > > index 692048d86ab1..f0ffcdcaef03 100644 > > > --- a/drivers/net/phy/bcm7xxx.c > > > +++ b/drivers/net/phy/bcm7xxx.c > > > @@ -11,6 +11,7 @@ > > > #include "bcm-phy-lib.h" > > > #include <linux/bitops.h> > > > #include <linux/brcmphy.h> > > > +#include <linux/clk.h> > > > #include <linux/mdio.h> > > > /* Broadcom BCM7xxx internal PHY registers */ > > > @@ -39,6 +40,7 @@ > > > struct bcm7xxx_phy_priv { > > > u64 *stats; > > > + struct clk *clk; > > > }; > > > static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev) > > > @@ -534,7 +536,19 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) > > > if (!priv->stats) > > > return -ENOMEM; > > > - return 0; > > > + priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL); > > > > Since the clock is binded to the mdio-dev here.. > > > > > + if (IS_ERR(priv->clk)) > > > + return PTR_ERR(priv->clk); > > > + > > > + return clk_prepare_enable(priv->clk); > > > > clould we use devm_add_action_or_reset() here so we don't have to > > register the .remove() hook? > > Maybe, more on that below. > > > > > > +} > > > + > > > +static void bcm7xxx_28nm_remove(struct phy_device *phydev) > > > +{ > > > + struct bcm7xxx_phy_priv *priv = phydev->priv; > > > + > > > + clk_disable_unprepare(priv->clk); > > > + devm_clk_put(&phydev->mdio.dev, priv->clk); > > > > Is this really necessary? The devm_clk_get_optional() function already > > registers the devm_clk_release() hook. > > Yes, because you can unbind the PHY driver from sysfs, and if you want to > bind that driver again, which will call .probe() again, you must undo > strictly everything that .probe() did. The embedded mdio_device does not go > away, so there will be no automatic freeing of resources. Okay I got this. Sry. I'm not that deep into the net-stack and the device live time. Thanks for the clarification. > Using devm_* may > be confusing, so using just the plain clk_get() and clk_put() may be clearer > here. That would be better for others including me because I detected a failure on my patchset. Regards, Marco
On 20-09-04 08:38, Florian Fainelli wrote: > > > On 9/3/2020 11:18 PM, Marco Felsch wrote: > > On 20-09-02 21:39, Florian Fainelli wrote: > > > The internal Gigabit PHY on Broadcom STB chips has a digital clock which > > > drives its MDIO interface among other things, the driver now requests > > > and manage that clock during .probe() and .remove() accordingly. > > > > Hi Florian, > > > > Seems like you added the same support here like I did for the smsc > > driver. So should I go with my proposed patch which can be adapted later > > after you guys figured out who to enable the required resources? > > That seems fine to me, on your platform there appears to be an assumption > that we will be able to probe the SMSC PHY because everything we need is > already enabled, right? If so, this patch series does not change that state. Unfortunately yes.. The imx-fec driver enables all DT-specified clock and then the mdio-bus probe is initiated. The good point is that my patchset do not require a clock-id so the generic way can replace my work later. Regards, Marco
On 20-09-04 08:37, Florian Fainelli wrote: ... > > Is this really necessary? The devm_clk_get_optional() function already > > registers the devm_clk_release() hook. > > Yes, because you can unbind the PHY driver from sysfs, and if you want to > bind that driver again, which will call .probe() again, you must undo > strictly everything that .probe() did. The embedded mdio_device does not go > away, so there will be no automatic freeing of resources. Using devm_* may > be confusing, so using just the plain clk_get() and clk_put() may be clearer > here. Hi Florian, sorry for asking again... I'm getting a bit confused during applying your comments to my smsc-phy patchset. A few drivers are using the devm_kzalloc() (including your bcm7xxx.c and my smsc.c). Does this mean that those drivers have a memory leak since the mdio_device does not disappear and so the memory allocated during probe() isn't freed? Regards, Marco
diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c index 692048d86ab1..f0ffcdcaef03 100644 --- a/drivers/net/phy/bcm7xxx.c +++ b/drivers/net/phy/bcm7xxx.c @@ -11,6 +11,7 @@ #include "bcm-phy-lib.h" #include <linux/bitops.h> #include <linux/brcmphy.h> +#include <linux/clk.h> #include <linux/mdio.h> /* Broadcom BCM7xxx internal PHY registers */ @@ -39,6 +40,7 @@ struct bcm7xxx_phy_priv { u64 *stats; + struct clk *clk; }; static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev) @@ -534,7 +536,19 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) if (!priv->stats) return -ENOMEM; - return 0; + priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL); + if (IS_ERR(priv->clk)) + return PTR_ERR(priv->clk); + + return clk_prepare_enable(priv->clk); +} + +static void bcm7xxx_28nm_remove(struct phy_device *phydev) +{ + struct bcm7xxx_phy_priv *priv = phydev->priv; + + clk_disable_unprepare(priv->clk); + devm_clk_put(&phydev->mdio.dev, priv->clk); } #define BCM7XXX_28NM_GPHY(_oui, _name) \ @@ -552,6 +566,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) .get_strings = bcm_phy_get_strings, \ .get_stats = bcm7xxx_28nm_get_phy_stats, \ .probe = bcm7xxx_28nm_probe, \ + .remove = bcm7xxx_28nm_remove, \ } #define BCM7XXX_28NM_EPHY(_oui, _name) \ @@ -567,6 +582,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev) .get_strings = bcm_phy_get_strings, \ .get_stats = bcm7xxx_28nm_get_phy_stats, \ .probe = bcm7xxx_28nm_probe, \ + .remove = bcm7xxx_28nm_remove, \ } #define BCM7XXX_40NM_EPHY(_oui, _name) \
The internal Gigabit PHY on Broadcom STB chips has a digital clock which drives its MDIO interface among other things, the driver now requests and manage that clock during .probe() and .remove() accordingly. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)