[PATCH net v3] rswitch: Fix PHY station management clock setting

[PATCH net v3] rswitch: Fix PHY station management clock setting

[Yoshihiro Shimoda]

Fix the MPIC.PSMCS value following the programming example in the
section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
S4 Hardware User Manual Rev.1.00.

The value is calculated by
    MPIC.PSMCS = clk[MHz] / (MDC frequency[MHz] * 2) - 1
with the input clock frequency from clk_get_rate() and MDC frequency
of 2.5MHz. Otherwise, this driver cannot communicate PHYs on the R-Car
S4 Starter Kit board.

Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"")
Reported-by: Tam Nguyen <tam.nguyen.xa@renesas.com>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
Changes from v2:
https://lore.kernel.org/all/20230926081156.3930074-1-yoshihiro.shimoda.uh@renesas.com/
 - Change subject.

Changes from v1:
https://lore.kernel.org/all/20230925003416.3863560-1-yoshihiro.shimoda.uh@renesas.com/
 - Revise the formula on the commit description.
 - Calculate the PSMCS value by using clk_get_raate().
 -- So, change author and Add Reported-by.

 drivers/net/ethernet/renesas/rswitch.c | 13 ++++++++++++-
 drivers/net/ethernet/renesas/rswitch.h |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index ea9186178091..fc01ad3f340d 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -4,6 +4,7 @@
  * Copyright (C) 2022 Renesas Electronics Corporation
  */
 
+#include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/etherdevice.h>
@@ -1049,7 +1050,7 @@ static void rswitch_rmac_setting(struct rswitch_etha *etha, const u8 *mac)
 static void rswitch_etha_enable_mii(struct rswitch_etha *etha)
 {
 	rswitch_modify(etha->addr, MPIC, MPIC_PSMCS_MASK | MPIC_PSMHT_MASK,
-		       MPIC_PSMCS(0x05) | MPIC_PSMHT(0x06));
+		       MPIC_PSMCS(etha->psmcs) | MPIC_PSMHT(0x06));
 	rswitch_modify(etha->addr, MPSM, 0, MPSM_MFF_C45);
 }
 
@@ -1693,6 +1694,12 @@ static void rswitch_etha_init(struct rswitch_private *priv, int index)
 	etha->index = index;
 	etha->addr = priv->addr + RSWITCH_ETHA_OFFSET + index * RSWITCH_ETHA_SIZE;
 	etha->coma_addr = priv->addr;
+
+	/* MPIC.PSMCS = (clk [MHz] / (MDC frequency [MHz] * 2) - 1.
+	 * Calculating PSMCS value as MDC frequency = 2.5MHz. So, multiply
+	 * both the numerator and the denominator by 10.
+	 */
+	etha->psmcs = clk_get_rate(priv->clk) / 100000 / (25 * 2) - 1;
 }
 
 static int rswitch_device_alloc(struct rswitch_private *priv, int index)
@@ -1900,6 +1907,10 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	spin_lock_init(&priv->lock);
 
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
 	attr = soc_device_match(rswitch_soc_no_speed_change);
 	if (attr)
 		priv->etha_no_runtime_change = true;
diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h
index f0c16a37ea55..04f49a7a5843 100644
--- a/drivers/net/ethernet/renesas/rswitch.h
+++ b/drivers/net/ethernet/renesas/rswitch.h
@@ -915,6 +915,7 @@ struct rswitch_etha {
 	bool external_phy;
 	struct mii_bus *mii;
 	phy_interface_t phy_interface;
+	u32 psmcs;
 	u8 mac_addr[MAX_ADDR_LEN];
 	int link;
 	int speed;
@@ -1012,6 +1013,7 @@ struct rswitch_private {
 	struct rswitch_mfwd mfwd;
 
 	spinlock_t lock;	/* lock interrupt registers' control */
+	struct clk *clk;
 
 	bool etha_no_runtime_change;
 	bool gwca_halt;
-- 
2.25.1

Re: [PATCH net v3] rswitch: Fix PHY station management clock setting

[Andrew Lunn]

> +
> +	/* MPIC.PSMCS = (clk [MHz] / (MDC frequency [MHz] * 2) - 1.
> +	 * Calculating PSMCS value as MDC frequency = 2.5MHz. So, multiply
> +	 * both the numerator and the denominator by 10.
> +	 */
> +	etha->psmcs = clk_get_rate(priv->clk) / 100000 / (25 * 2) - 1;
>  }
>  
>  static int rswitch_device_alloc(struct rswitch_private *priv, int index)
> @@ -1900,6 +1907,10 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	spin_lock_init(&priv->lock);
>  
> +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);
> +

/**
 * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
 *		  This is only valid once the clock source has been enabled.
 * @clk: clock source
 */
unsigned long clk_get_rate(struct clk *clk);

I don't see the clock being enabled anywhere.

Also, is the clock documented in the device tree binding?

      Andrew

RE: [PATCH net v3] rswitch: Fix PHY station management clock setting

[Yoshihiro Shimoda]

Hello Andrew,

> From: Andrew Lunn, Sent: Wednesday, September 27, 2023 9:44 PM
> 
> > +
> > +	/* MPIC.PSMCS = (clk [MHz] / (MDC frequency [MHz] * 2) - 1.
> > +	 * Calculating PSMCS value as MDC frequency = 2.5MHz. So, multiply
> > +	 * both the numerator and the denominator by 10.
> > +	 */
> > +	etha->psmcs = clk_get_rate(priv->clk) / 100000 / (25 * 2) - 1;
> >  }
> >
> >  static int rswitch_device_alloc(struct rswitch_private *priv, int index)
> > @@ -1900,6 +1907,10 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	spin_lock_init(&priv->lock);
> >
> > +	priv->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->clk))
> > +		return PTR_ERR(priv->clk);
> > +
> 
> /**
>  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>  *		  This is only valid once the clock source has been enabled.
>  * @clk: clock source
>  */
> unsigned long clk_get_rate(struct clk *clk);
> 
> I don't see the clock being enabled anywhere.

Since GENPD_FLAG_PM_CLK is set in the drivers/pmdomain/renesas/rcar-gen4-sysc.c,
pm_runtime_get_sync() will enable the clock. That's why this code works correctly
without clk_enable() calling.

> Also, is the clock documented in the device tree binding?

Yes, but this is a "clocks" property only though. In the dt-bindings doc:
---
examples:
...
        clocks = <&cpg CPG_MOD 1505>;
---

And, in the drivers/clk/renesas/r8a779f0-cpg-mssr.c:
---
        DEF_FIXED("rsw2",       R8A779F0_CLK_RSW2,      CLK_PLL5_DIV2,  5, 1),
...
        DEF_MOD("rswitch2",     1505,   R8A779F0_CLK_RSW2),
---
So, the device will get the paranet clock " R8A779F0_CLK_RSW2".
And according to the clk_summary in the debugfs:
---
# grep rsw /sys/kernel/debug/clk/clk_summary
             rsw2                     1        1        0   320000000          0     0  50000         Y
                rswitch2              1        1        0   320000000          0     0  50000         Y
---

I found that i2c-rcar.c and pwm-rcar.c are also the same implementation
which call clk_get_rate() without clk_enable(). But, perhaps, we should enable
the clock by clk API?

To Geert-san, do you have any opinion?

Best regards,
Yoshihiro Shimoda

>       Andrew

Re: [PATCH net v3] rswitch: Fix PHY station management clock setting

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, Sep 28, 2023 at 4:13 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Andrew Lunn, Sent: Wednesday, September 27, 2023 9:44 PM
> > > +   /* MPIC.PSMCS = (clk [MHz] / (MDC frequency [MHz] * 2) - 1.
> > > +    * Calculating PSMCS value as MDC frequency = 2.5MHz. So, multiply
> > > +    * both the numerator and the denominator by 10.
> > > +    */
> > > +   etha->psmcs = clk_get_rate(priv->clk) / 100000 / (25 * 2) - 1;
> > >  }
> > >
> > >  static int rswitch_device_alloc(struct rswitch_private *priv, int index)
> > > @@ -1900,6 +1907,10 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
> > >             return -ENOMEM;
> > >     spin_lock_init(&priv->lock);
> > >
> > > +   priv->clk = devm_clk_get(&pdev->dev, NULL);
> > > +   if (IS_ERR(priv->clk))
> > > +           return PTR_ERR(priv->clk);
> > > +
> >
> > /**
> >  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
> >  *              This is only valid once the clock source has been enabled.

Whether clk_get_rate() works when the clock is still disabled actually
depends on the clock driver implementation/hardware.  It's not
guaranteed, so generic code cannot make that assumption.
It should work fine on all Renesas on-SoC clock generators.

> >  * @clk: clock source
> >  */
> > unsigned long clk_get_rate(struct clk *clk);
> >
> > I don't see the clock being enabled anywhere.
>
> Since GENPD_FLAG_PM_CLK is set in the drivers/pmdomain/renesas/rcar-gen4-sysc.c,
> pm_runtime_get_sync() will enable the clock. That's why this code works correctly
> without clk_enable() calling.
>
> > Also, is the clock documented in the device tree binding?
>
> Yes, but this is a "clocks" property only though. In the dt-bindings doc:
> ---
> examples:
> ...
>         clocks = <&cpg CPG_MOD 1505>;
> ---
>
> And, in the drivers/clk/renesas/r8a779f0-cpg-mssr.c:
> ---
>         DEF_FIXED("rsw2",       R8A779F0_CLK_RSW2,      CLK_PLL5_DIV2,  5, 1),
> ...
>         DEF_MOD("rswitch2",     1505,   R8A779F0_CLK_RSW2),
> ---
> So, the device will get the paranet clock " R8A779F0_CLK_RSW2".
> And according to the clk_summary in the debugfs:
> ---
> # grep rsw /sys/kernel/debug/clk/clk_summary
>              rsw2                     1        1        0   320000000          0     0  50000         Y
>                 rswitch2              1        1        0   320000000          0     0  50000         Y
> ---
>
> I found that i2c-rcar.c and pwm-rcar.c are also the same implementation
> which call clk_get_rate() without clk_enable(). But, perhaps, we should enable
> the clock by clk API?
>
> To Geert-san, do you have any opinion?

As the device is part of a clock domain, the clock is managed through
Runtime PM, and there is no need to enable or disable manually.
Just make sure to call pm_runtime_resume_and_get() before accessing
any register of the device.

Calling clk_get_rate() at any time is fine.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH net v3] rswitch: Fix PHY station management clock setting

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Thursday, September 28, 2023 4:33 PM
> 
> Hi Shimoda-san,
> 
> On Thu, Sep 28, 2023 at 4:13 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Andrew Lunn, Sent: Wednesday, September 27, 2023 9:44 PM
> > > > +   /* MPIC.PSMCS = (clk [MHz] / (MDC frequency [MHz] * 2) - 1.
> > > > +    * Calculating PSMCS value as MDC frequency = 2.5MHz. So, multiply
> > > > +    * both the numerator and the denominator by 10.
> > > > +    */
> > > > +   etha->psmcs = clk_get_rate(priv->clk) / 100000 / (25 * 2) - 1;
> > > >  }
> > > >
> > > >  static int rswitch_device_alloc(struct rswitch_private *priv, int index)
> > > > @@ -1900,6 +1907,10 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
> > > >             return -ENOMEM;
> > > >     spin_lock_init(&priv->lock);
> > > >
> > > > +   priv->clk = devm_clk_get(&pdev->dev, NULL);
> > > > +   if (IS_ERR(priv->clk))
> > > > +           return PTR_ERR(priv->clk);
> > > > +
> > >
> > > /**
> > >  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
> > >  *              This is only valid once the clock source has been enabled.
> 
> Whether clk_get_rate() works when the clock is still disabled actually
> depends on the clock driver implementation/hardware.  It's not
> guaranteed, so generic code cannot make that assumption.
> It should work fine on all Renesas on-SoC clock generators.

Thank you for the information!

> > >  * @clk: clock source
> > >  */
> > > unsigned long clk_get_rate(struct clk *clk);
> > >
> > > I don't see the clock being enabled anywhere.
> >
> > Since GENPD_FLAG_PM_CLK is set in the drivers/pmdomain/renesas/rcar-gen4-sysc.c,
> > pm_runtime_get_sync() will enable the clock. That's why this code works correctly
> > without clk_enable() calling.
> >
> > > Also, is the clock documented in the device tree binding?
> >
> > Yes, but this is a "clocks" property only though. In the dt-bindings doc:
> > ---
> > examples:
> > ...
> >         clocks = <&cpg CPG_MOD 1505>;
> > ---
> >
> > And, in the drivers/clk/renesas/r8a779f0-cpg-mssr.c:
> > ---
> >         DEF_FIXED("rsw2",       R8A779F0_CLK_RSW2,      CLK_PLL5_DIV2,  5, 1),
> > ...
> >         DEF_MOD("rswitch2",     1505,   R8A779F0_CLK_RSW2),
> > ---
> > So, the device will get the paranet clock " R8A779F0_CLK_RSW2".
> > And according to the clk_summary in the debugfs:
> > ---
> > # grep rsw /sys/kernel/debug/clk/clk_summary
> >              rsw2                     1        1        0   320000000          0     0  50000         Y
> >                 rswitch2              1        1        0   320000000          0     0  50000         Y
> > ---
> >
> > I found that i2c-rcar.c and pwm-rcar.c are also the same implementation
> > which call clk_get_rate() without clk_enable(). But, perhaps, we should enable
> > the clock by clk API?
> >
> > To Geert-san, do you have any opinion?
> 
> As the device is part of a clock domain, the clock is managed through
> Runtime PM, and there is no need to enable or disable manually.
> Just make sure to call pm_runtime_resume_and_get() before accessing
> any register of the device.
> 
> Calling clk_get_rate() at any time is fine.

I got it. Thank you for your reply!

To Andrew, I believe this v3 patch can be acceptable for upstream.
But, what do you think? Should I add some description somewhere about the clock of
Renesas SoC?

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH net v3] rswitch: Fix PHY station management clock setting

[Andrew Lunn]

On Tue, Sep 26, 2023 at 09:30:54PM +0900, Yoshihiro Shimoda wrote:
> Fix the MPIC.PSMCS value following the programming example in the
> section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
> S4 Hardware User Manual Rev.1.00.
> 
> The value is calculated by
>     MPIC.PSMCS = clk[MHz] / (MDC frequency[MHz] * 2) - 1
> with the input clock frequency from clk_get_rate() and MDC frequency
> of 2.5MHz. Otherwise, this driver cannot communicate PHYs on the R-Car
> S4 Starter Kit board.
> 
> Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"")
> Reported-by: Tam Nguyen <tam.nguyen.xa@renesas.com>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v3] rswitch: Fix PHY station management clock setting

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 26 Sep 2023 21:30:54 +0900 you wrote:
> Fix the MPIC.PSMCS value following the programming example in the
> section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
> S4 Hardware User Manual Rev.1.00.
> 
> The value is calculated by
>     MPIC.PSMCS = clk[MHz] / (MDC frequency[MHz] * 2) - 1
> with the input clock frequency from clk_get_rate() and MDC frequency
> of 2.5MHz. Otherwise, this driver cannot communicate PHYs on the R-Car
> S4 Starter Kit board.
> 
> [...]

Here is the summary with links:
  - [net,v3] rswitch: Fix PHY station management clock setting
    https://git.kernel.org/netdev/net/c/a0c55bba0d0d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html