[PATCH repost] ARM: shmobile: r8a7794: Correct SDHI base address, clock labels and output-names

[Simon Horman]

On Tue, Jan 27, 2015 at 09:48:09AM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Tue, Jan 27, 2015 at 1:59 AM, Simon Horman <horms at verge.net.au> wrote:
> > On Thu, Jan 22, 2015 at 11:25:59AM +0900, Simon Horman wrote:
> >> * Correct base address of SD3 div6 clk.
> >> * Update div6 labels and MSTP output names
> >>   There appears to have been some inconsistency and confusion here as on
> >>   the r8a7790 these clocks are referred to as SD(HI)1 and SD(HI)2 while on
> >>   the r8a7791 and r8a7794 they are referred to as SD(HI)2 and SD(HI)3.
> >>
> >> This has no run-time affect as the clock nodes are not currently used.
> >>
> >> Fixes: 8e181633e6ca96049 ("ARM: shmobile: r8a7794: Add SDHI clocks to device tree")
> >> Reported-by: Sergei Shtylyov <sergei.shtylyov at cogentembedded.com>
> >> Reported-by: Geert Uytterhoeven <geert at linux-m68k.org>
> >> Signed-off-by: Simon Horman <horms+renesas at verge.net.au>
> >
> > Geert,
> >
> > could you find a moment to look over this.
> > Its a follow-up to an earlier patch which you gave me some
> > feedback on.
> 
> Sorry, it fell through the cracks.

No problem, its a low priority item.

Thanks for pointing out all the problems with my patch.
I've attempted to address them in "ARM: shmobile: r8a7794: Correct SDHI
clock base address, labels and output-names".

> 
> >> ---
> >> * A similar change to update the div6 clock node names and labels,
> >>   and the MSTP clock output-names is required for the r8a7791.
> >>   The base addresses appear to be correct there.
> >> ---
> >>  arch/arm/boot/dts/r8a7794.dtsi | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi
> >> index 8f78da5..b98534e 100644
> >> --- a/arch/arm/boot/dts/r8a7794.dtsi
> >> +++ b/arch/arm/boot/dts/r8a7794.dtsi
> >> @@ -294,16 +294,16 @@
> >>                                            "lb", "qspi", "sdh", "sd0", "z";
> >>               };
> >>               /* Variable factor clocks */
> >> -             sd1_clk: sd2_clk at e6150078 {
> >> +             sd2_clk: sd2_clk at e6150078 {
> >>                       compatible = "renesas,r8a7794-div6-clock", "renesas,cpg-div6-clock";
> >>                       reg = <0 0xe6150078 0 4>;
> >>                       clocks = <&pll1_div2_clk>;
> >>                       #clock-cells = <0>;
> >>                       clock-output-names = "sd1";
> 
> I think these should be "sd2"...
> 
> >>               };
> >> -             sd2_clk: sd3_clk at e615007c {
> >> +             sd3_clk: sd3_clk at e615026c {
> >>                       compatible = "renesas,r8a7794-div6-clock", "renesas,cpg-div6-clock";
> >> -                     reg = <0 0xe615007c 0 4>;
> >> +                     reg = <0 0xe615026c 0 4>;
> >>                       clocks = <&pll1_div2_clk>;
> >>                       #clock-cells = <0>;
> >>                       clock-output-names = "sd2";
> 
> ... and "sd3", as they refer to the clock signals from the clock generator,
> and thus should match the register and label names.
> 
> (Note: The (old) R-Car E2 v0.1 datasheet does call them SD1 and SD2 :-(
> 
> >> @@ -518,7 +518,7 @@
> >>               mstp3_clks: mstp3_clks at e615013c {
> >>                       compatible = "renesas,r8a7794-mstp-clocks", "renesas,cpg-mstp-clocks";
> >>                       reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>;
> >> -                     clocks = <&sd2_clk>, <&sd1_clk>, <&cpg_clocks R8A7794_CLK_SD0>,
> >> +                     clocks = <&sd3_clk>, <&sd2_clk>, <&cpg_clocks R8A7794_CLK_SD0>,
> >>                                <&mmc0_clk>, <&rclk_clk>, <&hp_clk>, <&hp_clk>;
> >>                       #clock-cells = <1>;
> >>                       clock-indices = <
> >> @@ -527,7 +527,7 @@
> >>                               R8A7794_CLK_USBDMAC0 R8A7794_CLK_USBDMAC1
> >>                       >;
> >>                       clock-output-names =
> >> -                             "sdhi2", "sdhi1", "sdhi0",
> >> +                             "sdhi3", "sdhi2", "sdhi0",
> 
> These are disputable: the MSTP docs indeed call them SDHI3, SDHI2, and
> SDHI0 (in the latest R-Car Gen2 family datasheet revision).
> But for all(?) other MSTP clocks they match the actual module instances
> that are driven by the clocks.
> So I think these should stay "sdhi2", "sdhi1", and "sdhi0", as they refer to
> the SDHI instances, not the parent clocks.
> 
> At least we should be consistent with the R8A7794_CLK_SDHIx indices,
> which are 2, 1, and 0, not 3, 2, 0.
> 
> >>                               "mmcif0", "cmt1", "usbdmac0", "usbdmac1";
> >>               };
> >>               mstp7_clks: mstp7_clks at e615014c {
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>