Hi Geert,
Thanks for the review.
> Hi Biju,
>
> On Tue, Mar 27, 2018 at 4:37 PM, Biju Das <biju.das@xxxxxxxxxxxxxx> wrote:
> > Add RZ/G1C (R8A77470) Clock Pulse Generator / Module Standby and
> > Software Reset support.
> >
> > Signed-off-by: Biju Das <biju.das@xxxxxxxxxxxxxx>
> > Reviewed-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/r8a7747x-cpg-mssr.c
>
> For consistency, I'd call this r8a77470-cpg-mssr.c.
>
> > @@ -0,0 +1,229 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * r8a7747 Clock Pulse Generator / Module Standby and Software Reset
>
> r8a77470
Will do
> > +static const struct cpg_core_clk r8a77470_core_clks[] __initconst = {
> > + /* External Clock Inputs */
> > + DEF_INPUT("extal", CLK_EXTAL),
> > + DEF_INPUT("usb_extal", CLK_USB_EXTAL),
> > +
> > + /* Internal Core Clocks */
> > + DEF_BASE(".main", CLK_MAIN, CLK_TYPE_GEN2_MAIN, CLK_EXTAL),
> > + DEF_BASE(".pll0", CLK_PLL0, CLK_TYPE_GEN2_PLL0, CLK_MAIN),
> > + DEF_BASE(".pll1", CLK_PLL1, CLK_TYPE_GEN2_PLL1, CLK_MAIN),
> > + DEF_BASE(".pll3", CLK_PLL3, CLK_TYPE_GEN2_PLL3, CLK_MAIN),
> > +
> > + DEF_FIXED(".pll1_div2", CLK_PLL1_DIV2, CLK_PLL1, 2, 1),
> > +
> > + /* Core Clock Outputs */
> > + DEF_BASE("lb", R8A77470_CLK_LB, CLK_TYPE_GEN2_LB, CLK_PLL1),
>
> DEF_FIXED("lb", R8A77470_CLK_LB, CLK_PLL1, 24, 1) and move it down
> between "b" and "p".
Will do.
> Note: apparently the dependency on MD18 is true for R-Car H2 only, so I will fix
> this in the other clock drivers.
>
> > +/*
> > + * CPG Clock Data
> > + */
> > +
> > +/*
> > + * MD EXTAL PLL0 PLL1 PLL3
> > + * 14 13 (MHz) *1 *2
> > + *---------------------------------------------------
> > + * 0 0 20 x80/2 x78 x50
> > + * 0 1 26 x60/2 x60 x56
> > + * 1 0 Prohibitted setting
> > + * 1 1 30 x52/2 x52 x50
>
> It looks like all PLL0/PLL1/PLL3 values are already the predivided values, unlike
> in other clock drivers? ...
Yes, you are correct. will correct this table.
> > + *
> > + * *1 : Table 7.4 indicates VCO output (PLL0 = VCO/2)
> > + * *2 : Table 7.4 indicates VCO output (PLL1 = VCO)
> > + */
> > +#define CPG_PLL_CONFIG_INDEX(md) ((((md) & BIT(14)) >> 13) | \
> > + (((md) & BIT(13)) >> 13))
> > +
> > +static const struct rcar_gen2_cpg_pll_config cpg_pll_configs[8]
> > +__initconst = {
>
> cpg_pll_configs[4]
>
> > + /* EXTAL div PLL1 mult PLL3 mult */
> > + { 1, 78, 50, },
> > + { 1, 60, 56, },
> > + { /* Invalid*/ },
> > + { 1, 52, 50, },
>
> ... so I think these should be multiplied by the predivider, which will allow to
> drop the corresponding test for RZ/G1C in rcar_gen2_cpg_clk_register.
Will correct this.
> > +};
> > +
> > +static int __init r8a7747x_cpg_mssr_init(struct device *dev)
>
> r8a77470_cpg_mssr_init
Will correct this
> > --- a/drivers/clk/renesas/rcar-gen2-cpg.c
> > +++ b/drivers/clk/renesas/rcar-gen2-cpg.c
> > @@ -16,6 +16,7 @@
> > #include <linux/init.h>
> > #include <linux/io.h>
> > #include <linux/slab.h>
> > +#include <linux/sys_soc.h>
> >
> > #include "renesas-cpg-mssr.h"
> > #include "rcar-gen2-cpg.h"
> > @@ -257,10 +258,21 @@ static const struct clk_div_table
> cpg_sd01_div_table[] = {
> > { 0, 0 },
> > };
> >
> > +static const struct clk_div_table rz_g1c_cpg_sd01_div_table[] = {
> > + { 5, 12 }, { 6, 16 }, { 7, 18 }, { 8, 24 },
> > + { 10, 36 }, { 11, 48 }, { 12, 10 }, { 0, 0 }, };
>
> This table is identical to cpg_sd01_div_table[], except for the missing first entry.
> So perhaps you could just use &cpg_sd01_div_table[1] instead?
Will take out this table.
> > +
> > +
> > static const struct rcar_gen2_cpg_pll_config *cpg_pll_config
> > __initdata; static unsigned int cpg_pll0_div __initdata; static u32
> > cpg_mode __initdata;
> >
> > +static const struct soc_device_attribute r8a7747xes[] = {
> > + { .soc_id = "r8a77470", .revision = "ES2.*" },
>
> So this does not apply to ES3.0 and later?
> What about ES1.*?
Will take out the revision field. Currently we got only ES2.0 version,
As per my knowledge, there is no ES1.0 version.It is basically derived from R-Car E2X.
> > + { /* sentinel */ }
> > +};
>
> > @@ -303,7 +315,10 @@ struct clk * __init
> rcar_gen2_cpg_clk_register(struct device *dev,
> > break;
> >
> > case CLK_TYPE_GEN2_PLL1:
> > - mult = cpg_pll_config->pll1_mult / 2;
> > + if (soc_device_match(r8a7747xes))
> > + mult = cpg_pll_config->pll1_mult;
> > + else
> > + mult = cpg_pll_config->pll1_mult / 2;
>
> If think this can be dropped if the values in cpg_pll_configs[] are multiplied by 2.
Will do
> > @@ -314,7 +329,10 @@ struct clk * __init
> rcar_gen2_cpg_clk_register(struct device *dev,
> > return cpg_z_clk_register(core->name, parent_name,
> > base);
> >
> > case CLK_TYPE_GEN2_LB:
> > - div = cpg_mode & BIT(18) ? 36 : 24;
> > + if (soc_device_match(r8a7747xes))
> > + div = 24;
> > + else
> > + div = cpg_mode & BIT(18) ? 36 : 24;
>
> Can be dropped if LB is modeled as a fixed clock instead.
Will do.
> > break;
> >
> > case CLK_TYPE_GEN2_ADSP:
> > @@ -326,12 +344,20 @@ struct clk * __init
> rcar_gen2_cpg_clk_register(struct device *dev,
> > break;
> >
> > case CLK_TYPE_GEN2_SD0:
> > - table = cpg_sd01_div_table;
> > + if (soc_device_match(r8a7747xes))
> > + table = rz_g1c_cpg_sd01_div_table;
> > + else
> > + table = cpg_sd01_div_table;
>
> table = cpg_sd01_div_table;
> if (soc_device_match(r8a7747xes))
> table++;
Will do
> > +
> > shift = 4;
> > break;
> >
> > case CLK_TYPE_GEN2_SD1:
> > - table = cpg_sd01_div_table;
> > + if (soc_device_match(r8a7747xes))
> > + table = rz_g1c_cpg_sd01_div_table;
> > + else
> > + table = cpg_sd01_div_table;
>
> Likewise.
>
> > +
> > shift = 0;
> > break;
>
> 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
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
