diff mbox series

[1/3] clk: renesas: r8a77995: Add ZA2 clock

Message ID 87im3ici1u.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series arm64: dts: renesas: r8a77995: draak: add R-Car Sound support | expand

Commit Message

Kuninori Morimoto May 17, 2021, 12:36 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Implement support for the ZA2 clock which is needed
for R-Car Sound.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/clk/renesas/r8a77995-cpg-mssr.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Geert Uytterhoeven May 17, 2021, 9:36 a.m. UTC | #1 
Hi Morimoto-san,

On Mon, May 17, 2021 at 2:36 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> Implement support for the ZA2 clock which is needed
> for R-Car Sound.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r8a77995-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a77995-cpg-mssr.c
> @@ -35,6 +35,7 @@ enum clk_ids {
>         CLK_PLL0D2,
>         CLK_PLL0D3,
>         CLK_PLL0D5,
> +       CLK_PLL0D24,
>         CLK_PLL1D2,
>         CLK_PE,
>         CLK_S0,
> @@ -62,6 +63,7 @@ static const struct cpg_core_clk r8a77995_core_clks[] __initconst = {
>         DEF_FIXED(".pll0d2",   CLK_PLL0D2,         CLK_PLL0,       2, 1),
>         DEF_FIXED(".pll0d3",   CLK_PLL0D3,         CLK_PLL0,       3, 1),
>         DEF_FIXED(".pll0d5",   CLK_PLL0D5,         CLK_PLL0,       5, 1),
> +       DEF_FIXED(".pll0d24",  CLK_PLL0D24,        CLK_PLL0,      24, 1),
>         DEF_FIXED(".pll1d2",   CLK_PLL1D2,         CLK_PLL1,       2, 1),
>         DEF_FIXED(".pe",       CLK_PE,             CLK_PLL0D3,     4, 1),
>         DEF_FIXED(".s0",       CLK_S0,             CLK_PLL1,       2, 1),
> @@ -75,6 +77,7 @@ static const struct cpg_core_clk r8a77995_core_clks[] __initconst = {
>         DEF_RATE(".oco",       CLK_OCO,            8 * 1000 * 1000),
>
>         /* Core Clock Outputs */
> +       DEF_FIXED("za2",       R8A77995_CLK_ZA2,   CLK_PLL0D24,    1, 1),

This does not match the Hardware User's Manual (Rev. 2.20):
  1. ZA2 is not a fixed clock, but can be controlled through the ZA2CKCR
     register.  Adding support for that requires writing a custom clock
     driver.
     Of course we can consider it a fixed clock initially, and make it
     configurable later, when time permits.
  2. The parent clock is either PLL0D3 or S0, with a configurable
     post-divider of 2 or 4, yielding 200, 250, 400, or 500[*] MHz.
     Using plain PLL0D24 would mean a post-divider of 8, yielding
     125 MHz, which is not documented as a supported value.

[*] Using the default would mean:

    DEF_FIXED("za2", R8A77995_CLK_ZA2, CLK_PLL0D3, 4, 1);

>         DEF_FIXED("z2",        R8A77995_CLK_Z2,    CLK_PLL0D3,     1, 1),
>         DEF_FIXED("ztr",       R8A77995_CLK_ZTR,   CLK_PLL1,       6, 1),
>         DEF_FIXED("zt",        R8A77995_CLK_ZT,    CLK_PLL1,       4, 1),

Gr{oetje,eeting}s,

                        Geert
Kuninori Morimoto May 17, 2021, 10:20 p.m. UTC | #2 
Hi Geert

Thank you for your review

> > @@ -75,6 +77,7 @@ static const struct cpg_core_clk r8a77995_core_clks[] __initconst = {
> >         DEF_RATE(".oco",       CLK_OCO,            8 * 1000 * 1000),
> >
> >         /* Core Clock Outputs */
> > +       DEF_FIXED("za2",       R8A77995_CLK_ZA2,   CLK_PLL0D24,    1, 1),
> 
> This does not match the Hardware User's Manual (Rev. 2.20):
>   1. ZA2 is not a fixed clock, but can be controlled through the ZA2CKCR
>      register.  Adding support for that requires writing a custom clock
>      driver.
>      Of course we can consider it a fixed clock initially, and make it
>      configurable later, when time permits.
>   2. The parent clock is either PLL0D3 or S0, with a configurable
>      post-divider of 2 or 4, yielding 200, 250, 400, or 500[*] MHz.
>      Using plain PLL0D24 would mean a post-divider of 8, yielding
>      125 MHz, which is not documented as a supported value.
> 
> [*] Using the default would mean:
> 
>     DEF_FIXED("za2", R8A77995_CLK_ZA2, CLK_PLL0D3, 4, 1);

Oops, I had checked E3 block.
Thank you pointing it. will fix in v2


Thank you for your help !!

Best regards
---
Kuninori Morimoto
Kuninori Morimoto May 24, 2021, 12:57 a.m. UTC | #3 
Hi Geert

> > This does not match the Hardware User's Manual (Rev. 2.20):
> >   1. ZA2 is not a fixed clock, but can be controlled through the ZA2CKCR
> >      register.  Adding support for that requires writing a custom clock
> >      driver.
> >      Of course we can consider it a fixed clock initially, and make it
> >      configurable later, when time permits.
> >   2. The parent clock is either PLL0D3 or S0, with a configurable
> >      post-divider of 2 or 4, yielding 200, 250, 400, or 500[*] MHz.
> >      Using plain PLL0D24 would mean a post-divider of 8, yielding
> >      125 MHz, which is not documented as a supported value.
> > 
> > [*] Using the default would mean:
> > 
> >     DEF_FIXED("za2", R8A77995_CLK_ZA2, CLK_PLL0D3, 4, 1);

PLL0 * 1/3 = 1GHz.
And default ZA2 on D3 is 500MHz thus it will be below
but am I misunderstanding ?

	- DEF_FIXED("za2", R8A77995_CLK_ZA2, CLK_PLL0D3, 4, 1);
	+ DEF_FIXED("za2", R8A77995_CLK_ZA2, CLK_PLL0D3, 2, 1);

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Geert Uytterhoeven May 25, 2021, 7:42 a.m. UTC | #4 
Hi Morimoto-san,

On Mon, May 24, 2021 at 2:57 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> > > This does not match the Hardware User's Manual (Rev. 2.20):
> > >   1. ZA2 is not a fixed clock, but can be controlled through the ZA2CKCR
> > >      register.  Adding support for that requires writing a custom clock
> > >      driver.
> > >      Of course we can consider it a fixed clock initially, and make it
> > >      configurable later, when time permits.
> > >   2. The parent clock is either PLL0D3 or S0, with a configurable
> > >      post-divider of 2 or 4, yielding 200, 250, 400, or 500[*] MHz.
> > >      Using plain PLL0D24 would mean a post-divider of 8, yielding
> > >      125 MHz, which is not documented as a supported value.
> > >
> > > [*] Using the default would mean:
> > >
> > >     DEF_FIXED("za2", R8A77995_CLK_ZA2, CLK_PLL0D3, 4, 1);
>
> PLL0 * 1/3 = 1GHz.
> And default ZA2 on D3 is 500MHz thus it will be below
> but am I misunderstanding ?
>
>         - DEF_FIXED("za2", R8A77995_CLK_ZA2, CLK_PLL0D3, 4, 1);
>         + DEF_FIXED("za2", R8A77995_CLK_ZA2, CLK_PLL0D3, 2, 1);

Yes, /2 instead of /4. Sorry for the confusion.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/clk/renesas/r8a77995-cpg-mssr.c b/drivers/clk/renesas/r8a77995-cpg-mssr.c
index 9cfd00cf4e69..8fb84ed6fe08 100644
--- a/drivers/clk/renesas/r8a77995-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a77995-cpg-mssr.c
@@ -35,6 +35,7 @@  enum clk_ids {
 	CLK_PLL0D2,
 	CLK_PLL0D3,
 	CLK_PLL0D5,
+	CLK_PLL0D24,
 	CLK_PLL1D2,
 	CLK_PE,
 	CLK_S0,
@@ -62,6 +63,7 @@  static const struct cpg_core_clk r8a77995_core_clks[] __initconst = {
 	DEF_FIXED(".pll0d2",   CLK_PLL0D2,         CLK_PLL0,       2, 1),
 	DEF_FIXED(".pll0d3",   CLK_PLL0D3,         CLK_PLL0,       3, 1),
 	DEF_FIXED(".pll0d5",   CLK_PLL0D5,         CLK_PLL0,       5, 1),
+	DEF_FIXED(".pll0d24",  CLK_PLL0D24,        CLK_PLL0,      24, 1),
 	DEF_FIXED(".pll1d2",   CLK_PLL1D2,         CLK_PLL1,       2, 1),
 	DEF_FIXED(".pe",       CLK_PE,             CLK_PLL0D3,     4, 1),
 	DEF_FIXED(".s0",       CLK_S0,             CLK_PLL1,       2, 1),
@@ -75,6 +77,7 @@  static const struct cpg_core_clk r8a77995_core_clks[] __initconst = {
 	DEF_RATE(".oco",       CLK_OCO,            8 * 1000 * 1000),
 
 	/* Core Clock Outputs */
+	DEF_FIXED("za2",       R8A77995_CLK_ZA2,   CLK_PLL0D24,    1, 1),
 	DEF_FIXED("z2",        R8A77995_CLK_Z2,    CLK_PLL0D3,     1, 1),
 	DEF_FIXED("ztr",       R8A77995_CLK_ZTR,   CLK_PLL1,       6, 1),
 	DEF_FIXED("zt",        R8A77995_CLK_ZT,    CLK_PLL1,       4, 1),