Hi Geert,
Thanks for your review.
On Thursday, September 06, 2018, Geert Uytterhoeven wrote:
> > +#define CPG_FRQCR 0x00
> > +#define CPG_CKIOSEL 0xF0
> > +#define CPG_SCLKSEL 0xF4
>
> The last two are unused?
In this driver they are not. I can remove them.
> > +
> > +#define PORTL_PIDR 0xFCFFE074
>
> Unused?
Oops. That was left over from when I first was reading the port pin to
find out the mode. But then I realize I could get the info from DT.
>
> > +static u8 cpg_mode;
> > +
> > +/* Internal Clock ratio table */
> > +static const unsigned int ratio_tab[5][5] = {
> > + /* I, G, B, P1, P0 */
>
> Use a struct instead?
>
> struct {
> unsigned int i;
> unsigned int g;
> unsigned int ib
> unsigned int p1;
> unsigned int p0;
> } ratio_tab[5] = { ... }
That's a good idea. Thanks.
>
> > + { 2, 4, 8, 16, 32 }, /* FRQCR = 0x012 */
> > + { 4, 4, 8, 16, 32 }, /* FRQCR = 0x112 */
> > + { 8, 4, 8, 16, 32 }, /* FRQCR = 0x212 */
> > + { 16, 8, 16, 16, 32 }, /* FRQCR = 0x322 */
> > + { 16, 16, 32, 32, 32 }, /* FRQCR = 0x333 */
>
> The P0 divider is fixed to 32, so you can remove it from the table?
OK, I can do that. I was just doing it to be consistent.
> > + /* Internal Core Clocks */
> > + CLK_MAIN,
> > + CLK_PLL,
> > + CLK_I,
> > + CLK_G,
> > + CLK_B,
> > + CLK_P1,
> > + CLK_P1C,
> > + CLK_P0,
>
> The last six are not used and can be removed
> (the driver uses R7S9210_CLK_* instead).
OK.
> > +static const struct mssr_mod_clk r7s9210_mod_clks[] __initconst = {
> > + DEF_MOD_STB("ostm0", 36, R7S9210_CLK_P1C),
> > + DEF_MOD_STB("ostm1", 35, R7S9210_CLK_P1C),
> > + DEF_MOD_STB("ostm2", 34, R7S9210_CLK_P1C),
>
> I think the table is easier to read if you sort by MSTP number.
OK. I will switch them all around.
> > + /* Adjust the dividers based on the current FRQCR setting */
> > + if (core->id == CLK_MAIN) {
> > +
> > + /* If EXTAL is above 12MHz, then we know it is Mode 1 */
> > + if (clk_get_rate((struct clk *)parent) > 12000000)
>
> Why the cast?
I was getting compiler warning because parent was const.
const struct clk *parent;
../drivers/clk/renesas/r7s9210-cpg-mssr.c:144:20: warning: passing argument 1 of ‘clk_get_rate’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
if (clk_get_rate(parent) > 12000000)
^
In file included from ../drivers/clk/renesas/r7s9210-cpg-mssr.c:12:0:
../include/linux/clk.h:463:15: note: expected ‘struct clk *’ but argument is of type ‘const struct clk *’
unsigned long clk_get_rate(struct clk *clk);
^
make[1]: Leaving directory '/home/renesas/tools/upstream/renesas-drivers/.out_rza2m'
I can remove const, then I don't need the cast anymore.
> > + /* Module Clocks */
> > + .mod_clks = r7s9210_mod_clks,
> > + .num_mod_clks = ARRAY_SIZE(r7s9210_mod_clks),
> > + .num_hw_mod_clks = 11 * 32, /* includes STBCR0/1/2 which don't
> exist */
>
> According to the HW manual, STBCR1/2 do not exist?
The problem is that there are registers named "STBCR1" and "STBCR2", but
they are not pure MSTP registers, and they sit at a different address
location.
Would it be better if I say the MSTP registers start at STBCR3, and just
subtract "30" from the numbers in the device tree?
> > +static const u16 stbcr[] = {
> > + 0x000, 0x000, 0x014, 0x410, 0x414, 0x418, 0x41C, 0x420,
>
> stbcr[1] should be 0x010?
Technically, stbcr[0] = 0x10, stbcr[1] = 0x14,
I was just thinking that I would never be access it anyway since they
are not really MSTP registers.
>
> > + 0x424, 0x428, 0x42C, 0x430, 0x434, 0x460,
>
> The last 3 don't exist?
Opps, you're right! They are left over from when I change the table around.
Thanks.
> > + } else {
> > + idx = MOD_CLK_PACK_10(clkidx);
>
> MOD_CLK_PACK()
Good find! I would have broken existing drivers!
> > +#ifdef CONFIG_CLK_R7S9210
> > + {
> > + .compatible = "renesas,r7s9210-cpg-mssr",
> > + .data = &r7s9210_cpg_mssr_info,
> > + },
>
> Please preserve sort order.
> > extern const struct cpg_mssr_info r8a77980_cpg_mssr_info;
> > extern const struct cpg_mssr_info r8a77990_cpg_mssr_info;
> > extern const struct cpg_mssr_info r8a77995_cpg_mssr_info;
> > +extern const struct cpg_mssr_info r7s9210_cpg_mssr_info;
>
> Please preserve sort order.
OK
> > +/* R7S9210 CPG Core Clocks */
> > +#define R7S9210_CLK_PLL 0
>
> Should that be an internal clock, not referred to from DT?
> There's already the internal CLK_PLL clock.
OK, I see what you mean. I will leave it out of this header file.
> > +#define R7S9210_CLK_I 1
> > +#define R7S9210_CLK_G 2
> > +#define R7S9210_CLK_B 3
> > +#define R7S9210_CLK_P1 4
> > +#define R7S9210_CLK_P1C 5
> > +#define R7S9210_CLK_P0 6
>
> The comment in Figure 6.1 suggests there's also P0C, but that may be a
> mistake, as I can find no other references to it?
Funny, I didn't see that in there before.
Like you said, it's probably just a mistake.
I'll point that out to the device team.
> What about other clocks: BSCCLK, OCTCLK, HYPCLK, and SPICLK?
At this time, I'm considering them 'out of scope' from this driver as
they really need to be set up early in device boot (ie, u-boot).
Unless you were just thinking of adding them as #defines, but never
really using them.
Chris
