Re: clk: clk-mstp has never worked for RZ/A1

Re: clk: clk-mstp has never worked for RZ/A1

[Geert Uytterhoeven]

Hi Chris,

CC devicetree

On Wed, Dec 14, 2016 at 3:14 AM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> Today I realized that the clock control using clk-mstp has never actually worked for RZ/A1 (r7s72100).

Oops... Where did your (offlist) bug report for rspi needing a delay after
clock enable come from?

> The reason is that the MSTP registers are 8-bit instead of the normal 32-bit, and if you do a 32-bit write to them, nothing happens.
> If you look in drivers/clk/renesas/clk-mstp.c, you'll see that clk_readl and clk_writel are used.

I was aware the registers are documented to be 8-bit wide, but I always
assumed 32-bit accesses do work.

> The reason why I have gotten this far is because u-boot has been enabling everything early in boot, so all the clocks were already on.
> Of course if I disable everything again before I booting the kernel....nothing works.

Oops, I only have MSTP early disable debug code for boards I do have
physical access to (cfr. my topic/renesas-debug branch)...

> If I change the accesses in clk-mspt.c to 8-bit, clocks get enabled/disabled as they should.
> #define clk_readl readb
> #define clk_writel writeb
>
>
> So, any suggestions on the best way to fix this???
> Ideally I would like something that can easily be integrated back into 4.9.x
>
>
>
> Maybe add a new DT property named "reg_width":
>
>         mstp3_clks: mstp3_clks@fcfe0420 {
>                 #clock-cells = <1>;
>                 compatible = "renesas,r7s72100-mstp-clocks", "renesas,cpg-mstp-clocks";
>                 reg = <0xfcfe0420 4>;
>                 reg_width = 8;
>                 clocks = <&p0_clk>;
>                 clock-indices = <R7S72100_CLK_MTU2>;
>                 clock-output-names = "mtu2";
>         };
>
> and then in cpg_mstp_clocks_init(), do:
>
>         if (of_find_property(np, "reg_width", &i))
>                 group->reg_width = i;
>         else
>                 group->reg_width = 32;  /* default */
>
>
> In cpg_mstp_clock_endisable(), do:
>
>         if (group->reg_width == 8)
>                 writeb(value, group->smstpcr);
>         else
>                 clk_writel(value, group->smstpcr);

That's one option. Note that the standard name for such property seems to be
"reg-io-width".

Another option is to let the driver bind against
"renesas,r7s72100-mstp-clocks", and switch to 8-bit access mode.

CLK_OF_DECLARE(cpg_mstp_clks, "renesas,r7s72100-mstp-clocks",
cpg_mstp_clocks_init8);

cpg_mstp_clocks_init8() can set a gloal flag, and call cpg_mstp_clocks_init().

The latter means no DT update is needed, and thus preserves compatibility
with old DTBs.

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: clk: clk-mstp has never worked for RZ/A1

[Chris Brandt]

Hi Geert,

On 12/14/206, Geert Uytterhoeven wrote:
> Oops... Where did your (offlist) bug report for rspi needing a delay after
> clock enable come from?

Because the RSPI driver in our current BSP is almost exactly the same so I was making the assumption that it would be a problem (but I have not tested it yet).


> I was aware the registers are documented to be 8-bit wide, but I always
> assumed 32-bit accesses do work.

At first, I did too.


> Another option is to let the driver bind against "renesas,r7s72100-mstp-
> clocks", and switch to 8-bit access mode.
> 
> CLK_OF_DECLARE(cpg_mstp_clks, "renesas,r7s72100-mstp-clocks",
> cpg_mstp_clocks_init8);
> 
> cpg_mstp_clocks_init8() can set a gloal flag, and call
> cpg_mstp_clocks_init().
> 
> The latter means no DT update is needed, and thus preserves compatibility
> with old DTBs.

I like this idea. I'll code it up and give it a shot.

Thank you.

Chris

RE: clk: clk-mstp has never worked for RZ/A1

[Chris Brandt]

Hi Geert,

On Dec 14, 2016, Geert Uytterhoeven wrote:
> Another option is to let the driver bind against "renesas,r7s72100-mstp-
> clocks", and switch to 8-bit access mode.
> 
> CLK_OF_DECLARE(cpg_mstp_clks, "renesas,r7s72100-mstp-clocks",
> cpg_mstp_clocks_init8);
> 
> cpg_mstp_clocks_init8() can set a gloal flag, and call
> cpg_mstp_clocks_init().
> 
> The latter means no DT update is needed, and thus preserves compatibility
> with old DTBs.

I just coded that and it works good. Thank you.


Now, one more question before I submit a patch for review:

I would like to add a "Fixes:" to the commit log so it can be considered for 4.9-stable (in reality it could go all the way back to when r7s72100 support was added)

But, the current file path didn't exist until after commit b3a33077c0dd ("clk: renesas: move drivers to renesas directory") on 2016-03-02.

So should I use that commit for my 'Fixes'?


Thanks,
Chris

Re: clk: clk-mstp has never worked for RZ/A1

[Geert Uytterhoeven]

Hi Chris,

On Wed, Dec 14, 2016 at 5:22 PM, Chris Brandt <Chris.Brandt-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> On Dec 14, 2016, Geert Uytterhoeven wrote:
>> Another option is to let the driver bind against "renesas,r7s72100-mstp-
>> clocks", and switch to 8-bit access mode.
>>
>> CLK_OF_DECLARE(cpg_mstp_clks, "renesas,r7s72100-mstp-clocks",
>> cpg_mstp_clocks_init8);
>>
>> cpg_mstp_clocks_init8() can set a gloal flag, and call
>> cpg_mstp_clocks_init().
>>
>> The latter means no DT update is needed, and thus preserves compatibility
>> with old DTBs.
>
> I just coded that and it works good. Thank you.

I'm glad to hear that!

> Now, one more question before I submit a patch for review:
>
> I would like to add a "Fixes:" to the commit log so it can be considered for 4.9-stable (in reality it could go all the way back to when r7s72100 support was added)
>
> But, the current file path didn't exist until after commit b3a33077c0dd ("clk: renesas: move drivers to renesas directory") on 2016-03-02.
>
> So should I use that commit for my 'Fixes'?

Please use the original commit, as that's where the bug was introduced.
Git is quiet good in tracking renames, so cherry-picking it to v4.5 and older
kernels may just work ;-)

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html