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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 28 Sep 2023 09:06:37 +0000
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>, Andrew Lunn <andrew@...n.ch>
CC: "s.shtylyov@....ru" <s.shtylyov@....ru>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>, Tam
 Nguyen <tam.nguyen.xa@...esas.com>, Kuninori Morimoto
	<kuninori.morimoto.gx@...esas.com>
Subject: RE: [PATCH net v3] rswitch: Fix PHY station management clock setting

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@...esas.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@...ux-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