diff mbox series

[v3,net] net: phy: micrel: Dynamically control external clock of KSZ PHY

Message ID 20241206012113.437029-1-wei.fang@nxp.com (mailing list archive)
State Superseded
Headers show
Series [v3,net] net: phy: micrel: Dynamically control external clock of KSZ PHY | expand

Commit Message

Wei Fang Dec. 6, 2024, 1:21 a.m. UTC
On the i.MX6ULL-14x14-EVK board, enet1_ref and enet2_ref are used as the
clock sources for two external KSZ PHYs. However, after closing the two
FEC ports, the clk_enable_count of the enet1_ref and enet2_ref clocks is
not 0. The root cause is that since the commit 985329462723 ("net: phy:
micrel: use devm_clk_get_optional_enabled for the rmii-ref clock"), the
external clock of KSZ PHY has been enabled when the PHY driver probes,
and it can only be disabled when the PHY driver is removed. This causes
the clock to continue working when the system is suspended or the network
port is down.

To solve this problem, the clock is enabled when phy_driver::resume() is
called, and the clock is disabled when phy_driver::suspend() is called.
Since phy_driver::resume() and phy_driver::suspend() are not called in
pairs, an additional clk_enable flag is added. When phy_driver::suspend()
is called, the clock is disabled only if clk_enable is true. Conversely,
when phy_driver::resume() is called, the clock is enabled if clk_enable
is false.

Fixes: 985329462723 ("net: phy: micrel: use devm_clk_get_optional_enabled for the rmii-ref clock")
Fixes: 99ac4cbcc2a5 ("net: phy: micrel: allow usage of generic ethernet-phy clock")
Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
v1 link: https://lore.kernel.org/imx/20241125022906.2140428-1-wei.fang@nxp.com/
v2 changes: only refine the commit message.
v2 link: https://lore.kernel.org/imx/20241202084535.2520151-1-wei.fang@nxp.com/
v3 changes: disable clock after getting the clock rate in kszphy_probe()
---
 drivers/net/phy/micrel.c | 101 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 95 insertions(+), 6 deletions(-)

Comments

Jakub Kicinski Dec. 10, 2024, 2:14 a.m. UTC | #1
On Fri,  6 Dec 2024 09:21:13 +0800 Wei Fang wrote:
> On the i.MX6ULL-14x14-EVK board, enet1_ref and enet2_ref are used as the
> clock sources for two external KSZ PHYs. However, after closing the two
> FEC ports, the clk_enable_count of the enet1_ref and enet2_ref clocks is
> not 0. The root cause is that since the commit 985329462723 ("net: phy:
> micrel: use devm_clk_get_optional_enabled for the rmii-ref clock"), the
> external clock of KSZ PHY has been enabled when the PHY driver probes,
> and it can only be disabled when the PHY driver is removed. This causes
> the clock to continue working when the system is suspended or the network
> port is down.
> 
> To solve this problem, the clock is enabled when phy_driver::resume() is
> called, and the clock is disabled when phy_driver::suspend() is called.
> Since phy_driver::resume() and phy_driver::suspend() are not called in
> pairs, an additional clk_enable flag is added. When phy_driver::suspend()
> is called, the clock is disabled only if clk_enable is true. Conversely,
> when phy_driver::resume() is called, the clock is enabled if clk_enable
> is false.

Sorry that nobody replied to you but yes, I believe the simpler fix you
proposed here:
https://lore.kernel.org/all/PAXPR04MB8510D36DDA1B9E98B2FB77B488362@PAXPR04MB8510.eurprd04.prod.outlook.com/
is better for net. In net-next we can try to keep the clock enabled
and/or try to fix the imbalance in resume calls that forces you to track
manually if the clock was enabled.
Wei Fang Dec. 10, 2024, 2:45 a.m. UTC | #2
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2024年12月10日 10:15
> To: Wei Fang <wei.fang@nxp.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; edumazet@google.com; pabeni@redhat.com;
> florian.fainelli@broadcom.com; heiko.stuebner@cherry.de; fank.li@nxp.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH v3 net] net: phy: micrel: Dynamically control external
> clock of KSZ PHY
>
> On Fri,  6 Dec 2024 09:21:13 +0800 Wei Fang wrote:
> > On the i.MX6ULL-14x14-EVK board, enet1_ref and enet2_ref are used as
> > the clock sources for two external KSZ PHYs. However, after closing
> > the two FEC ports, the clk_enable_count of the enet1_ref and enet2_ref
> > clocks is not 0. The root cause is that since the commit 985329462723 ("net:
> phy:
> > micrel: use devm_clk_get_optional_enabled for the rmii-ref clock"),
> > the external clock of KSZ PHY has been enabled when the PHY driver
> > probes, and it can only be disabled when the PHY driver is removed.
> > This causes the clock to continue working when the system is suspended
> > or the network port is down.
> >
> > To solve this problem, the clock is enabled when phy_driver::resume()
> > is called, and the clock is disabled when phy_driver::suspend() is called.
> > Since phy_driver::resume() and phy_driver::suspend() are not called in
> > pairs, an additional clk_enable flag is added. When
> > phy_driver::suspend() is called, the clock is disabled only if
> > clk_enable is true. Conversely, when phy_driver::resume() is called,
> > the clock is enabled if clk_enable is false.
>
> Sorry that nobody replied to you but yes, I believe the simpler fix you proposed
> here:
> https://lore.ker/
> nel.org%2Fall%2FPAXPR04MB8510D36DDA1B9E98B2FB77B488362%40PAXPR
> 04MB8510.eurprd04.prod.outlook.com%2F&data=05%7C02%7Cwei.fang%40n
> xp.com%7Cda6de7c31d2a4f93267608dd18c06fa1%7C686ea1d3bc2b4c6fa92
> cd99c5c301635%7C0%7C0%7C638693936967952515%7CUnknown%7CTWF
> pbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4z
> MiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=YjR%2Fupk
> pye%2FmMXCH02HNEUiot1kGaD1xkeUEO9hY8P0%3D&reserved=0
> is better for net. In net-next we can try to keep the clock enabled and/or try to
> fix the imbalance in resume calls that forces you to track manually if the clock
> was enabled.

Hi Jakub,

The simple fix could only fix the commit 985329462723 ("net: phy: micrel: use
devm_clk_get_optional_enabled for the rmii-ref clock"), because as the commit
message said some clock suppliers need to be enabled so that the driver can get
the correct clock rate.

But the problem is that the simple fix cannot fix the 99ac4cbcc2a5 ("net: phy:
micrel: allow usage of generic ethernet-phy clock"). The change is as follows,
this change just enables the clock when the PHY driver probes. There are no
other operations on the clock, such as obtaining the clock rate. So you still think
a simple fix is good enough for net tree?

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index de116a0753a1..d2aa3d0695e3 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -2021,6 +2021,11 @@ static int kszphy_probe(struct phy_device *phydev)
                                   rate);
                        return -EINVAL;
                }
+       } else if (!clk) {
+               /* unnamed clock from the generic ethernet-phy binding */
+               clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, NULL);
+               if (IS_ERR(clk))
+                       return PTR_ERR(clk);
        }
Jakub Kicinski Dec. 11, 2024, 12:43 a.m. UTC | #3
On Tue, 10 Dec 2024 02:45:57 +0000 Wei Fang wrote:
> The simple fix could only fix the commit 985329462723 ("net: phy: micrel: use
> devm_clk_get_optional_enabled for the rmii-ref clock"), because as the commit
> message said some clock suppliers need to be enabled so that the driver can get
> the correct clock rate.
> 
> But the problem is that the simple fix cannot fix the 99ac4cbcc2a5 ("net: phy:
> micrel: allow usage of generic ethernet-phy clock"). The change is as follows,
> this change just enables the clock when the PHY driver probes. There are no
> other operations on the clock, such as obtaining the clock rate. So you still think
> a simple fix is good enough for net tree?

I may be missing something but if you don't need to disable the generic
clock you can put the disable into the if () block for rmii-ref ?

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 3ef508840674..8bbd2018f2a6 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -2214,6 +2214,8 @@ static int kszphy_probe(struct phy_device *phydev)
                                   rate);
                        return -EINVAL;
                }
+
+               clk_disable_unprepare(clk);
        } else if (!clk) {
                /* unnamed clock from the generic ethernet-phy binding */
                clk = devm_clk_get_optional_enabled(&phydev->mdio.dev, NULL);
Wei Fang Dec. 11, 2024, 1:49 a.m. UTC | #4
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2024年12月11日 8:43
> To: Wei Fang <wei.fang@nxp.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; edumazet@google.com; pabeni@redhat.com;
> florian.fainelli@broadcom.com; heiko.stuebner@cherry.de; fank.li@nxp.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH v3 net] net: phy: micrel: Dynamically control external clock
> of KSZ PHY
> 
> On Tue, 10 Dec 2024 02:45:57 +0000 Wei Fang wrote:
> > The simple fix could only fix the commit 985329462723 ("net: phy: micrel: use
> > devm_clk_get_optional_enabled for the rmii-ref clock"), because as the
> commit
> > message said some clock suppliers need to be enabled so that the driver can
> get
> > the correct clock rate.
> >
> > But the problem is that the simple fix cannot fix the 99ac4cbcc2a5 ("net: phy:
> > micrel: allow usage of generic ethernet-phy clock"). The change is as follows,
> > this change just enables the clock when the PHY driver probes. There are no
> > other operations on the clock, such as obtaining the clock rate. So you still
> think
> > a simple fix is good enough for net tree?
> 
> I may be missing something but if you don't need to disable the generic
> clock you can put the disable into the if () block for rmii-ref ?

For my case, it's fine to disable rmii-ref because this clock source is always
enabled in FEC driver. But the commit 99ac4cbcc2a5 ("net: phy: micrel: allow
usage of generic ethernet-phy clock") was applied a year ago, so I raised a
concern in V2 [1], if a new platform only enables rmii-ref in the PHY driver,
disabling rmii-ref after getting the clock rate will cause problem, which will
cause RMII to not work. I'm not sure if any platform actually does this, if so
the following changes will be a more serious problem.

[1] https://lore.kernel.org/all/PAXPR04MB8510D36DDA1B9E98B2FB77B488362@PAXPR04MB8510.eurprd04.prod.outlook.com/

> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 3ef508840674..8bbd2018f2a6 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -2214,6 +2214,8 @@ static int kszphy_probe(struct phy_device *phydev)
>                                    rate);
>                         return -EINVAL;
>                 }
> +
> +               clk_disable_unprepare(clk);
>         } else if (!clk) {
>                 /* unnamed clock from the generic ethernet-phy binding */
>                 clk = devm_clk_get_optional_enabled(&phydev->mdio.dev,
> NULL);
Jakub Kicinski Dec. 11, 2024, 1:59 a.m. UTC | #5
On Wed, 11 Dec 2024 01:49:50 +0000 Wei Fang wrote:
> > I may be missing something but if you don't need to disable the generic
> > clock you can put the disable into the if () block for rmii-ref ?  
> 
> For my case, it's fine to disable rmii-ref because this clock source is always
> enabled in FEC driver. But the commit 99ac4cbcc2a5 ("net: phy: micrel: allow
> usage of generic ethernet-phy clock") was applied a year ago, so I raised a
> concern in V2 [1], if a new platform only enables rmii-ref in the PHY driver,
> disabling rmii-ref after getting the clock rate will cause problem, which will
> cause RMII to not work. I'm not sure if any platform actually does this, if so
> the following changes will be a more serious problem.

Put more of this explanation into the commit message and resend.
If it convinces Andrew we can apply.
Wei Fang Dec. 11, 2024, 2:02 a.m. UTC | #6
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: 2024年12月11日 9:59
> To: Wei Fang <wei.fang@nxp.com>
> Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; edumazet@google.com; pabeni@redhat.com;
> florian.fainelli@broadcom.com; heiko.stuebner@cherry.de; fank.li@nxp.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev
> Subject: Re: [PATCH v3 net] net: phy: micrel: Dynamically control external clock
> of KSZ PHY
> 
> On Wed, 11 Dec 2024 01:49:50 +0000 Wei Fang wrote:
> > > I may be missing something but if you don't need to disable the
> > > generic clock you can put the disable into the if () block for rmii-ref ?
> >
> > For my case, it's fine to disable rmii-ref because this clock source
> > is always enabled in FEC driver. But the commit 99ac4cbcc2a5 ("net:
> > phy: micrel: allow usage of generic ethernet-phy clock") was applied a
> > year ago, so I raised a concern in V2 [1], if a new platform only
> > enables rmii-ref in the PHY driver, disabling rmii-ref after getting
> > the clock rate will cause problem, which will cause RMII to not work.
> > I'm not sure if any platform actually does this, if so the following changes will
> be a more serious problem.
> 
> Put more of this explanation into the commit message and resend.
> If it convinces Andrew we can apply.

Okay, thanks
diff mbox series

Patch

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 3ef508840674..ffc2ac39fa48 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -432,10 +432,12 @@  struct kszphy_ptp_priv {
 struct kszphy_priv {
 	struct kszphy_ptp_priv ptp_priv;
 	const struct kszphy_type *type;
+	struct clk *clk;
 	int led_mode;
 	u16 vct_ctrl1000;
 	bool rmii_ref_clk_sel;
 	bool rmii_ref_clk_sel_val;
+	bool clk_enable;
 	u64 stats[ARRAY_SIZE(kszphy_hw_stats)];
 };
 
@@ -2050,8 +2052,27 @@  static void kszphy_get_stats(struct phy_device *phydev,
 		data[i] = kszphy_get_stat(phydev, i);
 }
 
+static void kszphy_enable_clk(struct kszphy_priv *priv)
+{
+	if (!priv->clk_enable && priv->clk) {
+		clk_prepare_enable(priv->clk);
+		priv->clk_enable = true;
+	}
+}
+
+static void kszphy_disable_clk(struct kszphy_priv *priv)
+{
+	if (priv->clk_enable && priv->clk) {
+		clk_disable_unprepare(priv->clk);
+		priv->clk_enable = false;
+	}
+}
+
 static int kszphy_suspend(struct phy_device *phydev)
 {
+	struct kszphy_priv *priv = phydev->priv;
+	int ret;
+
 	/* Disable PHY Interrupts */
 	if (phy_interrupt_is_valid(phydev)) {
 		phydev->interrupts = PHY_INTERRUPT_DISABLED;
@@ -2059,7 +2080,13 @@  static int kszphy_suspend(struct phy_device *phydev)
 			phydev->drv->config_intr(phydev);
 	}
 
-	return genphy_suspend(phydev);
+	ret = genphy_suspend(phydev);
+	if (ret)
+		return ret;
+
+	kszphy_disable_clk(priv);
+
+	return 0;
 }
 
 static void kszphy_parse_led_mode(struct phy_device *phydev)
@@ -2088,8 +2115,11 @@  static void kszphy_parse_led_mode(struct phy_device *phydev)
 
 static int kszphy_resume(struct phy_device *phydev)
 {
+	struct kszphy_priv *priv = phydev->priv;
 	int ret;
 
+	kszphy_enable_clk(priv);
+
 	genphy_resume(phydev);
 
 	/* After switching from power-down to normal mode, an internal global
@@ -2112,6 +2142,24 @@  static int kszphy_resume(struct phy_device *phydev)
 	return 0;
 }
 
+static int ksz8041_resume(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+
+	kszphy_enable_clk(priv);
+
+	return 0;
+}
+
+static int ksz8041_suspend(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+
+	kszphy_disable_clk(priv);
+
+	return 0;
+}
+
 static int ksz9477_resume(struct phy_device *phydev)
 {
 	int ret;
@@ -2150,8 +2198,11 @@  static int ksz9477_resume(struct phy_device *phydev)
 
 static int ksz8061_resume(struct phy_device *phydev)
 {
+	struct kszphy_priv *priv = phydev->priv;
 	int ret;
 
+	kszphy_enable_clk(priv);
+
 	/* This function can be called twice when the Ethernet device is on. */
 	ret = phy_read(phydev, MII_BMCR);
 	if (ret < 0)
@@ -2221,6 +2272,11 @@  static int kszphy_probe(struct phy_device *phydev)
 			return PTR_ERR(clk);
 	}
 
+	if (!IS_ERR_OR_NULL(clk)) {
+		clk_disable_unprepare(clk);
+		priv->clk = clk;
+	}
+
 	if (ksz8041_fiber_mode(phydev))
 		phydev->port = PORT_FIBRE;
 
@@ -5290,15 +5346,45 @@  static int lan8841_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int lan8804_suspend(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+	int ret;
+
+	ret = genphy_suspend(phydev);
+	if (ret)
+		return ret;
+
+	kszphy_disable_clk(priv);
+
+	return 0;
+}
+
+static int lan8841_resume(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+
+	kszphy_enable_clk(priv);
+
+	return genphy_resume(phydev);
+}
+
 static int lan8841_suspend(struct phy_device *phydev)
 {
 	struct kszphy_priv *priv = phydev->priv;
 	struct kszphy_ptp_priv *ptp_priv = &priv->ptp_priv;
+	int ret;
 
 	if (ptp_priv->ptp_clock)
 		ptp_cancel_worker_sync(ptp_priv->ptp_clock);
 
-	return genphy_suspend(phydev);
+	ret = genphy_suspend(phydev);
+	if (ret)
+		return ret;
+
+	kszphy_disable_clk(priv);
+
+	return 0;
 }
 
 static struct phy_driver ksphy_driver[] = {
@@ -5358,9 +5444,12 @@  static struct phy_driver ksphy_driver[] = {
 	.get_sset_count = kszphy_get_sset_count,
 	.get_strings	= kszphy_get_strings,
 	.get_stats	= kszphy_get_stats,
-	/* No suspend/resume callbacks because of errata DS80000700A,
-	 * receiver error following software power down.
+	/* Because of errata DS80000700A, receiver error following software
+	 * power down. Suspend and resume callbacks only disable and enable
+	 * external rmii reference clock.
 	 */
+	.suspend	= ksz8041_suspend,
+	.resume		= ksz8041_resume,
 }, {
 	.phy_id		= PHY_ID_KSZ8041RNLI,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
@@ -5507,7 +5596,7 @@  static struct phy_driver ksphy_driver[] = {
 	.get_sset_count	= kszphy_get_sset_count,
 	.get_strings	= kszphy_get_strings,
 	.get_stats	= kszphy_get_stats,
-	.suspend	= genphy_suspend,
+	.suspend	= lan8804_suspend,
 	.resume		= kszphy_resume,
 	.config_intr	= lan8804_config_intr,
 	.handle_interrupt = lan8804_handle_interrupt,
@@ -5526,7 +5615,7 @@  static struct phy_driver ksphy_driver[] = {
 	.get_strings	= kszphy_get_strings,
 	.get_stats	= kszphy_get_stats,
 	.suspend	= lan8841_suspend,
-	.resume		= genphy_resume,
+	.resume		= lan8841_resume,
 	.cable_test_start	= lan8814_cable_test_start,
 	.cable_test_get_status	= ksz886x_cable_test_get_status,
 }, {