| Message ID | 20150831062952.24004.17072.sendpatchset@little-apple (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Magnus and Geert, Thank you for the patch. On Monday 31 August 2015 15:29:52 Magnus Damm wrote: > From: Geert Uytterhoeven <geert+renesas@glider.be> > > Add the device nodes for all R-Car H3 SCIF serial ports, incl. clocks, > clock domain, and dma properties. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Signed-off-by: Gaku Inami <gaku.inami.xw@bp.renesas.com> > Signed-off-by: Magnus Damm <damm+renesas@opensource.se> > --- > > Based on: > [PATCH 9/25] arm64: renesas: r8a7795: Add SCIF2 support > [PATCH 1/6] arm64: renesas: r8a7795 dtsi: Mark scif2 disabled > [PATCH 3/6] arm64: renesas: r8a7795 dtsi: Add all SCIF nodes > > Changes: (Magnus Damm <damm+renesas@opensource.se>) > - Folded together above SCIF2 patches > - Added SCIF2 DMA bits > - Got rid of clock-output-names > - Replaced renesas,clock-indices with clock-indices > > TODO: > - Double check if R-Car H3 SCIF2 really has DMA support or not > > arch/arm64/boot/dts/renesas/r8a7795.dtsi | 105 ++++++++++++++++++++++++++ > include/dt-bindings/clock/r8a7795-clock.h | 6 + > 2 files changed, 111 insertions(+) > > --- 0014/arch/arm64/boot/dts/renesas/r8a7795.dtsi > +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi 2015-08-29 > 18:25:06.922366518 +0900 @@ -231,6 +231,11 @@ > }; > > cpg_clocks: cpg_clocks@e6150000 { > + #address-cells = <2>; > + #size-cells = <2>; > + #clock-cells = <1>; > + ranges; > + > compatible = "renesas,r8a7795-cpg-clocks", > "renesas,rcar-gen3-cpg-clocks"; > reg = <0 0xe6150000 0 0x1000>; > @@ -241,6 +246,34 @@ > R8A7795_CLK_PLL3 R8A7795_CLK_PLL4 > > >; > > #power-domain-cells = <0>; > + > + mstp2_clks: mstp2_clks@e6150138 { > + compatible = > + "renesas,r8a7795-mstp-clocks", > + "renesas,cpg-mstp-clocks"; > + reg = <0 0xe6150138 0 4>, > + <0 0xe6150040 0 4>; > + clocks = <&s3d4_clk>, <&s3d4_clk>, > + <&s3d4_clk>, <&s3d4_clk>, > + <&s3d4_clk>; > + #clock-cells = <1>; > + clock-indices = < > + R8A7795_CLK_SCIF5 > + R8A7795_CLK_SCIF4 > + R8A7795_CLK_SCIF3 > + R8A7795_CLK_SCIF1 > + R8A7795_CLK_SCIF0 > + >; > + }; > + > + mstp3_clks: mstp3_clks@e615013c { > + compatible = "renesas,r8a7795-mstp-clocks", > + "renesas,cpg-mstp-clocks"; > + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; > + clocks = <&s3d4_clk>; > + #clock-cells = <1>; > + clock-indices = <R8A7795_CLK_SCIF2>; > + }; > }; > }; > > @@ -255,5 +288,77 @@ > dmac2: dma-controller@e7310000 { > /* Empty node for now */ > }; > + > + scif0: serial@e6e60000 { > + compatible = "renesas,scif-r8a7795", "renesas,scif"; > + reg = <0 0xe6e60000 0 64>; > + interrupts = <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp2_clks R8A7795_CLK_SCIF0>; > + clock-names = "sci_ick"; > + dmas = <&dmac1 0x51>, <&dmac1 0x50>; > + dma-names = "tx", "rx"; > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > + > + scif1: serial@e6e68000 { > + compatible = "renesas,scif-r8a7795", "renesas,scif"; > + reg = <0 0xe6e68000 0 64>; > + interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp2_clks R8A7795_CLK_SCIF1>; > + clock-names = "sci_ick"; > + dmas = <&dmac1 0x53>, <&dmac1 0x52>; > + dma-names = "tx", "rx"; > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > + > + scif2: serial@e6e88000 { > + compatible = "renesas,scif-r8a7795", "renesas,scif"; > + reg = <0 0xe6e88000 0 64>; > + interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp3_clks R8A7795_CLK_SCIF2>; > + clock-names = "sci_ick"; > + dmas = <&dmac0 0x55>, <&dmac0 0x54>; > + dma-names = "tx", "rx"; Depending at which version of the datasheet you look at SCIF2 will or will not support DMA. This needs to be clarified, in the meantime I'd drop the DMA channels. Apart from that, everything looks good to me. Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > + > + scif3: serial@e6c50000 { > + compatible = "renesas,scif-r8a7795", "renesas,scif"; > + reg = <0 0xe6c50000 0 64>; > + interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp2_clks R8A7795_CLK_SCIF3>; > + clock-names = "sci_ick"; > + dmas = <&dmac0 0x57>, <&dmac0 0x56>; > + dma-names = "tx", "rx"; > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > + > + scif4: serial@e6c40000 { > + compatible = "renesas,scif-r8a7795", "renesas,scif"; > + reg = <0 0xe6c40000 0 64>; > + interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp2_clks R8A7795_CLK_SCIF4>; > + clock-names = "sci_ick"; > + dmas = <&dmac0 0x59>, <&dmac0 0x58>; > + dma-names = "tx", "rx"; > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > + > + scif5: serial@e6f30000 { > + compatible = "renesas,scif-r8a7795", "renesas,scif"; > + reg = <0 0xe6f30000 0 64>; > + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&mstp2_clks R8A7795_CLK_SCIF5>; > + clock-names = "sci_ick"; > + dmas = <&dmac1 0x5b>, <&dmac1 0x5a>; > + dma-names = "tx", "rx"; > + power-domains = <&cpg_clocks>; > + status = "disabled"; > + }; > }; > }; > --- 0012/include/dt-bindings/clock/r8a7795-clock.h > +++ work/include/dt-bindings/clock/r8a7795-clock.h 2015-08-29 > 18:22:14.282366518 +0900 @@ -22,8 +22,14 @@ > /* MSTP1 */ > > /* MSTP2 */ > +#define R8A7795_CLK_SCIF5 2 > +#define R8A7795_CLK_SCIF4 3 > +#define R8A7795_CLK_SCIF3 4 > +#define R8A7795_CLK_SCIF1 6 > +#define R8A7795_CLK_SCIF0 7 > > /* MSTP3 */ > +#define R8A7795_CLK_SCIF2 10 > > /* MSTP5 */
Hi Magnus, On Mon, Aug 31, 2015 at 8:29 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > --- 0014/arch/arm64/boot/dts/renesas/r8a7795.dtsi > +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi 2015-08-29 18:25:06.922366518 +0900 > @@ -241,6 +246,34 @@ > R8A7795_CLK_PLL3 R8A7795_CLK_PLL4 > >; > #power-domain-cells = <0>; > + > + mstp2_clks: mstp2_clks@e6150138 { With the "clock-output-names" dropped, I think the node should be called "mstp2" (without "_clks") suffix. > + compatible = > + "renesas,r8a7795-mstp-clocks", > + "renesas,cpg-mstp-clocks"; > + reg = <0 0xe6150138 0 4>, > + <0 0xe6150040 0 4>; > + clocks = <&s3d4_clk>, <&s3d4_clk>, > + <&s3d4_clk>, <&s3d4_clk>, > + <&s3d4_clk>; > + #clock-cells = <1>; > + clock-indices = < > + R8A7795_CLK_SCIF5 > + R8A7795_CLK_SCIF4 > + R8A7795_CLK_SCIF3 > + R8A7795_CLK_SCIF1 > + R8A7795_CLK_SCIF0 > + >; > + }; > + > + mstp3_clks: mstp3_clks@e615013c { Likewise. > + compatible = "renesas,r8a7795-mstp-clocks", > + "renesas,cpg-mstp-clocks"; > + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; > + clocks = <&s3d4_clk>; > + #clock-cells = <1>; > + clock-indices = <R8A7795_CLK_SCIF2>; Sample (part of) /sys/kernel/debug/clk/clk_summary output: s3 1 1 0 0 0 s3d4 1 2 0 0 0 mstp3_clks.10 2 2 0 0 0 I think "mstp3.10" looks nicer than "mstp3_clks.10". Note that before we had "scif2". 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Mon, Aug 31, 2015 at 9:47 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Magnus, > > On Mon, Aug 31, 2015 at 8:29 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >> --- 0014/arch/arm64/boot/dts/renesas/r8a7795.dtsi >> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi 2015-08-29 18:25:06.922366518 +0900 >> @@ -241,6 +246,34 @@ >> R8A7795_CLK_PLL3 R8A7795_CLK_PLL4 >> >; >> #power-domain-cells = <0>; >> + >> + mstp2_clks: mstp2_clks@e6150138 { > > With the "clock-output-names" dropped, I think the node should be called > "mstp2" (without "_clks") suffix. Makes sense! >> + compatible = >> + "renesas,r8a7795-mstp-clocks", >> + "renesas,cpg-mstp-clocks"; >> + reg = <0 0xe6150138 0 4>, >> + <0 0xe6150040 0 4>; >> + clocks = <&s3d4_clk>, <&s3d4_clk>, >> + <&s3d4_clk>, <&s3d4_clk>, >> + <&s3d4_clk>; >> + #clock-cells = <1>; >> + clock-indices = < >> + R8A7795_CLK_SCIF5 >> + R8A7795_CLK_SCIF4 >> + R8A7795_CLK_SCIF3 >> + R8A7795_CLK_SCIF1 >> + R8A7795_CLK_SCIF0 >> + >; >> + }; >> + >> + mstp3_clks: mstp3_clks@e615013c { > > Likewise. Sure... >> + compatible = "renesas,r8a7795-mstp-clocks", >> + "renesas,cpg-mstp-clocks"; But if we're going down that route then may I ask why we have "-clocks" suffix for the MSTP/CPG compat strings? I'd rather make them shorter and more similar to the rest of the compat strings on the SoC. >> + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; >> + clocks = <&s3d4_clk>; >> + #clock-cells = <1>; >> + clock-indices = <R8A7795_CLK_SCIF2>; > > Sample (part of) /sys/kernel/debug/clk/clk_summary output: > > s3 1 1 0 > 0 0 > s3d4 1 2 0 > 0 0 > mstp3_clks.10 2 2 0 > 0 0 > > I think "mstp3.10" looks nicer than "mstp3_clks.10". > Note that before we had "scif2". It is still possible to add extended information in clock-output-names to the DTS for debugging purpose, right? Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Mon, Aug 31, 2015 at 2:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote: > On Mon, Aug 31, 2015 at 9:47 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> Hi Magnus, >> >> On Mon, Aug 31, 2015 at 8:29 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >>> --- 0014/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi 2015-08-29 18:25:06.922366518 +0900 >>> @@ -241,6 +246,34 @@ >>> R8A7795_CLK_PLL3 R8A7795_CLK_PLL4 >>> >; >>> #power-domain-cells = <0>; >>> + >>> + mstp2_clks: mstp2_clks@e6150138 { >> >> With the "clock-output-names" dropped, I think the node should be called >> "mstp2" (without "_clks") suffix. > > Makes sense! > >>> + compatible = >>> + "renesas,r8a7795-mstp-clocks", >>> + "renesas,cpg-mstp-clocks"; >>> + reg = <0 0xe6150138 0 4>, >>> + <0 0xe6150040 0 4>; >>> + clocks = <&s3d4_clk>, <&s3d4_clk>, >>> + <&s3d4_clk>, <&s3d4_clk>, >>> + <&s3d4_clk>; >>> + #clock-cells = <1>; >>> + clock-indices = < >>> + R8A7795_CLK_SCIF5 >>> + R8A7795_CLK_SCIF4 >>> + R8A7795_CLK_SCIF3 >>> + R8A7795_CLK_SCIF1 >>> + R8A7795_CLK_SCIF0 >>> + >; >>> + }; >>> + >>> + mstp3_clks: mstp3_clks@e615013c { >> >> Likewise. > > Sure... > >>> + compatible = "renesas,r8a7795-mstp-clocks", >>> + "renesas,cpg-mstp-clocks"; > > But if we're going down that route then may I ask why we have > "-clocks" suffix for the MSTP/CPG compat strings? I'd rather make them > shorter and more similar to the rest of the compat strings on the SoC. It uses plural because CPG and MSTP nodes provide more than one clock. Cfr. DIV6, which provides a single clock, and uses e.g. "renesas,r8a7791-div6-clock", "renesas,cpg-div6-clock" (singular). >>> + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; >>> + clocks = <&s3d4_clk>; >>> + #clock-cells = <1>; >>> + clock-indices = <R8A7795_CLK_SCIF2>; >> >> Sample (part of) /sys/kernel/debug/clk/clk_summary output: >> >> s3 1 1 0 >> 0 0 >> s3d4 1 2 0 >> 0 0 >> mstp3_clks.10 2 2 0 >> 0 0 >> >> I think "mstp3.10" looks nicer than "mstp3_clks.10". >> Note that before we had "scif2". > > It is still possible to add extended information in clock-output-names > to the DTS for debugging purpose, right? Sure, but we can no longer rely on its existence if it's declared optional. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Mon, Aug 31, 2015 at 10:08 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Magnus, > > On Mon, Aug 31, 2015 at 2:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote: >> On Mon, Aug 31, 2015 at 9:47 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> Hi Magnus, >>> >>> On Mon, Aug 31, 2015 at 8:29 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >>>> --- 0014/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>>> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi 2015-08-29 18:25:06.922366518 +0900 >>>> @@ -241,6 +246,34 @@ >>>> R8A7795_CLK_PLL3 R8A7795_CLK_PLL4 >>>> >; >>>> #power-domain-cells = <0>; >>>> + >>>> + mstp2_clks: mstp2_clks@e6150138 { >>> >>> With the "clock-output-names" dropped, I think the node should be called >>> "mstp2" (without "_clks") suffix. >> >> Makes sense! >> >>>> + compatible = >>>> + "renesas,r8a7795-mstp-clocks", >>>> + "renesas,cpg-mstp-clocks"; >>>> + reg = <0 0xe6150138 0 4>, >>>> + <0 0xe6150040 0 4>; >>>> + clocks = <&s3d4_clk>, <&s3d4_clk>, >>>> + <&s3d4_clk>, <&s3d4_clk>, >>>> + <&s3d4_clk>; >>>> + #clock-cells = <1>; >>>> + clock-indices = < >>>> + R8A7795_CLK_SCIF5 >>>> + R8A7795_CLK_SCIF4 >>>> + R8A7795_CLK_SCIF3 >>>> + R8A7795_CLK_SCIF1 >>>> + R8A7795_CLK_SCIF0 >>>> + >; >>>> + }; >>>> + >>>> + mstp3_clks: mstp3_clks@e615013c { >>> >>> Likewise. >> >> Sure... >> >>>> + compatible = "renesas,r8a7795-mstp-clocks", >>>> + "renesas,cpg-mstp-clocks"; >> >> But if we're going down that route then may I ask why we have >> "-clocks" suffix for the MSTP/CPG compat strings? I'd rather make them >> shorter and more similar to the rest of the compat strings on the SoC. > > It uses plural because CPG and MSTP nodes provide more than one clock. > > Cfr. DIV6, which provides a single clock, and uses e.g. > "renesas,r8a7791-div6-clock", "renesas,cpg-div6-clock" (singular). Ok, thanks but my concern was not about singular vs plural. Why do we need the "-clocks" suffix? It's a detail, but for me the shorter "renesas,r8a7795-mstp" makes more sense than "renesas,r8a7795-mstp-clocks" >>>> + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; >>>> + clocks = <&s3d4_clk>; >>>> + #clock-cells = <1>; >>>> + clock-indices = <R8A7795_CLK_SCIF2>; >>> >>> Sample (part of) /sys/kernel/debug/clk/clk_summary output: >>> >>> s3 1 1 0 >>> 0 0 >>> s3d4 1 2 0 >>> 0 0 >>> mstp3_clks.10 2 2 0 >>> 0 0 >>> >>> I think "mstp3.10" looks nicer than "mstp3_clks.10". >>> Note that before we had "scif2". >> >> It is still possible to add extended information in clock-output-names >> to the DTS for debugging purpose, right? > > Sure, but we can no longer rely on its existence if it's declared optional. Right, and we should not have to rely on it either I think! Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Thu, Sep 3, 2015 at 9:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > On Mon, Aug 31, 2015 at 10:08 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Mon, Aug 31, 2015 at 2:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote: >>> On Mon, Aug 31, 2015 at 9:47 PM, Geert Uytterhoeven >>> <geert@linux-m68k.org> wrote: >>>> On Mon, Aug 31, 2015 at 8:29 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >>>>> --- 0014/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>>>> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi 2015-08-29 18:25:06.922366518 +0900 >>>>> @@ -241,6 +246,34 @@ >>>>> R8A7795_CLK_PLL3 R8A7795_CLK_PLL4 >>>>> >; >>>>> #power-domain-cells = <0>; >>>>> + >>>>> + mstp2_clks: mstp2_clks@e6150138 { >>>> >>>> With the "clock-output-names" dropped, I think the node should be called >>>> "mstp2" (without "_clks") suffix. >>> >>> Makes sense! >>> >>>>> + compatible = >>>>> + "renesas,r8a7795-mstp-clocks", >>>>> + "renesas,cpg-mstp-clocks"; >>>>> + reg = <0 0xe6150138 0 4>, >>>>> + <0 0xe6150040 0 4>; >>>>> + clocks = <&s3d4_clk>, <&s3d4_clk>, >>>>> + <&s3d4_clk>, <&s3d4_clk>, >>>>> + <&s3d4_clk>; >>>>> + #clock-cells = <1>; >>>>> + clock-indices = < >>>>> + R8A7795_CLK_SCIF5 >>>>> + R8A7795_CLK_SCIF4 >>>>> + R8A7795_CLK_SCIF3 >>>>> + R8A7795_CLK_SCIF1 >>>>> + R8A7795_CLK_SCIF0 >>>>> + >; >>>>> + }; >>>>> + >>>>> + mstp3_clks: mstp3_clks@e615013c { >>>> >>>> Likewise. >>> >>> Sure... >>> >>>>> + compatible = "renesas,r8a7795-mstp-clocks", >>>>> + "renesas,cpg-mstp-clocks"; >>> >>> But if we're going down that route then may I ask why we have >>> "-clocks" suffix for the MSTP/CPG compat strings? I'd rather make them >>> shorter and more similar to the rest of the compat strings on the SoC. >> >> It uses plural because CPG and MSTP nodes provide more than one clock. >> >> Cfr. DIV6, which provides a single clock, and uses e.g. >> "renesas,r8a7791-div6-clock", "renesas,cpg-div6-clock" (singular). > > Ok, thanks but my concern was not about singular vs plural. > Why do we need the "-clocks" suffix? > > It's a detail, but for me the shorter "renesas,r8a7795-mstp" makes > more sense than "renesas,r8a7795-mstp-clocks" The MSTP blocks are subsets of the CPG block, and their registers are heavily entangled with other registers inside the CPG and other MSTP blocks. So currently the MSTP nodes don't represent the MSTP blocks, but their clocks only (and not e.g. reset control). I'm afraid the only sane way to express their full functionality is to have a single cpg_mstp node... >>>>> + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; >>>>> + clocks = <&s3d4_clk>; >>>>> + #clock-cells = <1>; >>>>> + clock-indices = <R8A7795_CLK_SCIF2>; >>>> >>>> Sample (part of) /sys/kernel/debug/clk/clk_summary output: >>>> >>>> s3 1 1 0 >>>> 0 0 >>>> s3d4 1 2 0 >>>> 0 0 >>>> mstp3_clks.10 2 2 0 >>>> 0 0 >>>> >>>> I think "mstp3.10" looks nicer than "mstp3_clks.10". >>>> Note that before we had "scif2". >>> >>> It is still possible to add extended information in clock-output-names >>> to the DTS for debugging purpose, right? >> >> Sure, but we can no longer rely on its existence if it's declared optional. > > Right, and we should not have to rely on it either I think! Ideally not. Practically, I need a simple way to identify the clock used by the GIC. Using the "intc-sys"/"intc-ap" name looked like a good idea. While it's always MSTP408, MSTP408 is used for other purposes on some SoCs. Let's give DT scanning a try... 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Thu, Sep 3, 2015 at 4:54 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Magnus, > > On Thu, Sep 3, 2015 at 9:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >> On Mon, Aug 31, 2015 at 10:08 PM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Mon, Aug 31, 2015 at 2:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote: >>>> On Mon, Aug 31, 2015 at 9:47 PM, Geert Uytterhoeven >>>> <geert@linux-m68k.org> wrote: >>>>> On Mon, Aug 31, 2015 at 8:29 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >>>>>> --- 0014/arch/arm64/boot/dts/renesas/r8a7795.dtsi >>>>>> +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi 2015-08-29 18:25:06.922366518 +0900 >>>>>> @@ -241,6 +246,34 @@ >>>>>> R8A7795_CLK_PLL3 R8A7795_CLK_PLL4 >>>>>> >; >>>>>> #power-domain-cells = <0>; >>>>>> + >>>>>> + mstp2_clks: mstp2_clks@e6150138 { >>>>> >>>>> With the "clock-output-names" dropped, I think the node should be called >>>>> "mstp2" (without "_clks") suffix. >>>> >>>> Makes sense! >>>> >>>>>> + compatible = >>>>>> + "renesas,r8a7795-mstp-clocks", >>>>>> + "renesas,cpg-mstp-clocks"; >>>>>> + reg = <0 0xe6150138 0 4>, >>>>>> + <0 0xe6150040 0 4>; >>>>>> + clocks = <&s3d4_clk>, <&s3d4_clk>, >>>>>> + <&s3d4_clk>, <&s3d4_clk>, >>>>>> + <&s3d4_clk>; >>>>>> + #clock-cells = <1>; >>>>>> + clock-indices = < >>>>>> + R8A7795_CLK_SCIF5 >>>>>> + R8A7795_CLK_SCIF4 >>>>>> + R8A7795_CLK_SCIF3 >>>>>> + R8A7795_CLK_SCIF1 >>>>>> + R8A7795_CLK_SCIF0 >>>>>> + >; >>>>>> + }; >>>>>> + >>>>>> + mstp3_clks: mstp3_clks@e615013c { >>>>> >>>>> Likewise. >>>> >>>> Sure... >>>> >>>>>> + compatible = "renesas,r8a7795-mstp-clocks", >>>>>> + "renesas,cpg-mstp-clocks"; >>>> >>>> But if we're going down that route then may I ask why we have >>>> "-clocks" suffix for the MSTP/CPG compat strings? I'd rather make them >>>> shorter and more similar to the rest of the compat strings on the SoC. >>> >>> It uses plural because CPG and MSTP nodes provide more than one clock. >>> >>> Cfr. DIV6, which provides a single clock, and uses e.g. >>> "renesas,r8a7791-div6-clock", "renesas,cpg-div6-clock" (singular). >> >> Ok, thanks but my concern was not about singular vs plural. >> Why do we need the "-clocks" suffix? >> >> It's a detail, but for me the shorter "renesas,r8a7795-mstp" makes >> more sense than "renesas,r8a7795-mstp-clocks" > > The MSTP blocks are subsets of the CPG block, and their registers are > heavily entangled with other registers inside the CPG and other MSTP blocks. > So currently the MSTP nodes don't represent the MSTP blocks, but > their clocks only (and not e.g. reset control). Ok, thanks for explaining. Then the current style makes sense. > I'm afraid the only sane way to express their full functionality is to have > a single cpg_mstp node... We've been talking about different ways how to rework the MSTP DT structures. So far we've had some loose ideas but no actual code has come out of it (unless I recall wrong). So based on that it must be good to follow same style as we have today. >>>>>> + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; >>>>>> + clocks = <&s3d4_clk>; >>>>>> + #clock-cells = <1>; >>>>>> + clock-indices = <R8A7795_CLK_SCIF2>; >>>>> >>>>> Sample (part of) /sys/kernel/debug/clk/clk_summary output: >>>>> >>>>> s3 1 1 0 >>>>> 0 0 >>>>> s3d4 1 2 0 >>>>> 0 0 >>>>> mstp3_clks.10 2 2 0 >>>>> 0 0 >>>>> >>>>> I think "mstp3.10" looks nicer than "mstp3_clks.10". >>>>> Note that before we had "scif2". >>>> >>>> It is still possible to add extended information in clock-output-names >>>> to the DTS for debugging purpose, right? >>> >>> Sure, but we can no longer rely on its existence if it's declared optional. >> >> Right, and we should not have to rely on it either I think! > > Ideally not. > > Practically, I need a simple way to identify the clock used by the GIC. > Using the "intc-sys"/"intc-ap" name looked like a good idea. > While it's always MSTP408, MSTP408 is used for other purposes on some SoCs. > > Let's give DT scanning a try... Uhm, sounds a bit overly complicated to me. This has probably been discussed wildly before, but please remind me: What is wrong again with being standard and adding "clocks" and "clock-names" to the GIC node? Cheers, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus, On Thu, Sep 3, 2015 at 10:06 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >> Practically, I need a simple way to identify the clock used by the GIC. >> Using the "intc-sys"/"intc-ap" name looked like a good idea. >> While it's always MSTP408, MSTP408 is used for other purposes on some SoCs. >> >> Let's give DT scanning a try... > > Uhm, sounds a bit overly complicated to me. This has probably been > discussed wildly before, but please remind me: > > What is wrong again with being standard and adding "clocks" and > "clock-names" to the GIC node? That needs to be done anyway (still have to dive into GIC architecture and come up with bindings the ARM people like). But it's not sufficient as: - The GIC driver is not PM Runtime aware. It doesn't even use a platform device, - The GIC is instantiated from IRQCHIP_DECLARE(), which is way before clocks are ready. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Thu, Sep 3, 2015 at 5:23 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Magnus, > > On Thu, Sep 3, 2015 at 10:06 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >>> Practically, I need a simple way to identify the clock used by the GIC. >>> Using the "intc-sys"/"intc-ap" name looked like a good idea. >>> While it's always MSTP408, MSTP408 is used for other purposes on some SoCs. >>> >>> Let's give DT scanning a try... >> >> Uhm, sounds a bit overly complicated to me. This has probably been >> discussed wildly before, but please remind me: >> >> What is wrong again with being standard and adding "clocks" and >> "clock-names" to the GIC node? > > That needs to be done anyway (still have to dive into GIC architecture > and come up with bindings the ARM people like). > > But it's not sufficient as: > - The GIC driver is not PM Runtime aware. It doesn't even use a platform > device, > - The GIC is instantiated from IRQCHIP_DECLARE(), which is way before > clocks are ready. Ouch! Hopefully our CPG hardware does not have interrupts hooked up to the GIC. That would make turn this mess into a full circle. =) / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Thursday 03 September 2015 09:54:17 Geert Uytterhoeven wrote: > On Thu, Sep 3, 2015 at 9:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > > On Mon, Aug 31, 2015 at 10:08 PM, Geert Uytterhoeven wrote: > >> On Mon, Aug 31, 2015 at 2:59 PM, Magnus Damm wrote: [snip] > >>> But if we're going down that route then may I ask why we have > >>> "-clocks" suffix for the MSTP/CPG compat strings? I'd rather make them > >>> shorter and more similar to the rest of the compat strings on the SoC. > >> > >> It uses plural because CPG and MSTP nodes provide more than one clock. > >> > >> Cfr. DIV6, which provides a single clock, and uses e.g. > >> "renesas,r8a7791-div6-clock", "renesas,cpg-div6-clock" (singular). > > > > Ok, thanks but my concern was not about singular vs plural. > > Why do we need the "-clocks" suffix? > > > > It's a detail, but for me the shorter "renesas,r8a7795-mstp" makes > > more sense than "renesas,r8a7795-mstp-clocks" > > The MSTP blocks are subsets of the CPG block, and their registers are > heavily entangled with other registers inside the CPG and other MSTP blocks. > So currently the MSTP nodes don't represent the MSTP blocks, but > their clocks only (and not e.g. reset control). > > I'm afraid the only sane way to express their full functionality is to have > a single cpg_mstp node... That might be a good idea. We could just use two clock cells and hide all the dirty details in C code. Anyone wants to give it a try ? :-)
Hi Laurent, On Thu, Sep 3, 2015 at 1:48 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thursday 03 September 2015 09:54:17 Geert Uytterhoeven wrote: >> On Thu, Sep 3, 2015 at 9:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >> > On Mon, Aug 31, 2015 at 10:08 PM, Geert Uytterhoeven wrote: >> >> On Mon, Aug 31, 2015 at 2:59 PM, Magnus Damm wrote: > > [snip] > >> >>> But if we're going down that route then may I ask why we have >> >>> "-clocks" suffix for the MSTP/CPG compat strings? I'd rather make them >> >>> shorter and more similar to the rest of the compat strings on the SoC. >> >> >> >> It uses plural because CPG and MSTP nodes provide more than one clock. >> >> >> >> Cfr. DIV6, which provides a single clock, and uses e.g. >> >> "renesas,r8a7791-div6-clock", "renesas,cpg-div6-clock" (singular). >> > >> > Ok, thanks but my concern was not about singular vs plural. >> > Why do we need the "-clocks" suffix? >> > >> > It's a detail, but for me the shorter "renesas,r8a7795-mstp" makes >> > more sense than "renesas,r8a7795-mstp-clocks" >> >> The MSTP blocks are subsets of the CPG block, and their registers are >> heavily entangled with other registers inside the CPG and other MSTP blocks. >> So currently the MSTP nodes don't represent the MSTP blocks, but >> their clocks only (and not e.g. reset control). >> >> I'm afraid the only sane way to express their full functionality is to have >> a single cpg_mstp node... > > That might be a good idea. We could just use two clock cells and hide all the > dirty details in C code. Anyone wants to give it a try ? :-) I think we have to keep at least the references to the parent clocks of the MSTP clocks in DT, as many of them are instantiated from DT (fixed-factor, div6, ...) instead of the renesas,<soc>-cpg-clocks C code. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Thursday 03 September 2015 21:03:10 Geert Uytterhoeven wrote: > On Thu, Sep 3, 2015 at 1:48 PM, Laurent Pinchart wrote: > > On Thursday 03 September 2015 09:54:17 Geert Uytterhoeven wrote: > >> On Thu, Sep 3, 2015 at 9:41 AM, Magnus Damm wrote: > >>> On Mon, Aug 31, 2015 at 10:08 PM, Geert Uytterhoeven wrote: > >>>> On Mon, Aug 31, 2015 at 2:59 PM, Magnus Damm wrote: > > > > [snip] > > > >>>>> But if we're going down that route then may I ask why we have > >>>>> "-clocks" suffix for the MSTP/CPG compat strings? I'd rather make > >>>>> them shorter and more similar to the rest of the compat strings on > >>>>> the SoC. > >>>> > >>>> It uses plural because CPG and MSTP nodes provide more than one clock. > >>>> > >>>> Cfr. DIV6, which provides a single clock, and uses e.g. > >>>> "renesas,r8a7791-div6-clock", "renesas,cpg-div6-clock" (singular). > >>> > >>> Ok, thanks but my concern was not about singular vs plural. > >>> Why do we need the "-clocks" suffix? > >>> > >>> It's a detail, but for me the shorter "renesas,r8a7795-mstp" makes > >>> more sense than "renesas,r8a7795-mstp-clocks" > >> > >> The MSTP blocks are subsets of the CPG block, and their registers are > >> heavily entangled with other registers inside the CPG and other MSTP > >> blocks. So currently the MSTP nodes don't represent the MSTP blocks, but > >> their clocks only (and not e.g. reset control). > >> > >> I'm afraid the only sane way to express their full functionality is to > >> have a single cpg_mstp node... > > > > That might be a good idea. We could just use two clock cells and hide all > > the dirty details in C code. Anyone wants to give it a try ? :-) > > I think we have to keep at least the references to the parent clocks of the > MSTP clocks in DT, as many of them are instantiated from DT (fixed-factor, > div6, ...) instead of the renesas,<soc>-cpg-clocks C code. We could use clock-indices for that, although it might quickly become ugly. And it would require removing the hardcoded assumption in the CCF core that the number of clock cells is 1 at most.
--- 0014/arch/arm64/boot/dts/renesas/r8a7795.dtsi +++ work/arch/arm64/boot/dts/renesas/r8a7795.dtsi 2015-08-29 18:25:06.922366518 +0900 @@ -231,6 +231,11 @@ }; cpg_clocks: cpg_clocks@e6150000 { + #address-cells = <2>; + #size-cells = <2>; + #clock-cells = <1>; + ranges; + compatible = "renesas,r8a7795-cpg-clocks", "renesas,rcar-gen3-cpg-clocks"; reg = <0 0xe6150000 0 0x1000>; @@ -241,6 +246,34 @@ R8A7795_CLK_PLL3 R8A7795_CLK_PLL4 >; #power-domain-cells = <0>; + + mstp2_clks: mstp2_clks@e6150138 { + compatible = + "renesas,r8a7795-mstp-clocks", + "renesas,cpg-mstp-clocks"; + reg = <0 0xe6150138 0 4>, + <0 0xe6150040 0 4>; + clocks = <&s3d4_clk>, <&s3d4_clk>, + <&s3d4_clk>, <&s3d4_clk>, + <&s3d4_clk>; + #clock-cells = <1>; + clock-indices = < + R8A7795_CLK_SCIF5 + R8A7795_CLK_SCIF4 + R8A7795_CLK_SCIF3 + R8A7795_CLK_SCIF1 + R8A7795_CLK_SCIF0 + >; + }; + + mstp3_clks: mstp3_clks@e615013c { + compatible = "renesas,r8a7795-mstp-clocks", + "renesas,cpg-mstp-clocks"; + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; + clocks = <&s3d4_clk>; + #clock-cells = <1>; + clock-indices = <R8A7795_CLK_SCIF2>; + }; }; }; @@ -255,5 +288,77 @@ dmac2: dma-controller@e7310000 { /* Empty node for now */ }; + + scif0: serial@e6e60000 { + compatible = "renesas,scif-r8a7795", "renesas,scif"; + reg = <0 0xe6e60000 0 64>; + interrupts = <GIC_SPI 152 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp2_clks R8A7795_CLK_SCIF0>; + clock-names = "sci_ick"; + dmas = <&dmac1 0x51>, <&dmac1 0x50>; + dma-names = "tx", "rx"; + power-domains = <&cpg_clocks>; + status = "disabled"; + }; + + scif1: serial@e6e68000 { + compatible = "renesas,scif-r8a7795", "renesas,scif"; + reg = <0 0xe6e68000 0 64>; + interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp2_clks R8A7795_CLK_SCIF1>; + clock-names = "sci_ick"; + dmas = <&dmac1 0x53>, <&dmac1 0x52>; + dma-names = "tx", "rx"; + power-domains = <&cpg_clocks>; + status = "disabled"; + }; + + scif2: serial@e6e88000 { + compatible = "renesas,scif-r8a7795", "renesas,scif"; + reg = <0 0xe6e88000 0 64>; + interrupts = <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp3_clks R8A7795_CLK_SCIF2>; + clock-names = "sci_ick"; + dmas = <&dmac0 0x55>, <&dmac0 0x54>; + dma-names = "tx", "rx"; + power-domains = <&cpg_clocks>; + status = "disabled"; + }; + + scif3: serial@e6c50000 { + compatible = "renesas,scif-r8a7795", "renesas,scif"; + reg = <0 0xe6c50000 0 64>; + interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp2_clks R8A7795_CLK_SCIF3>; + clock-names = "sci_ick"; + dmas = <&dmac0 0x57>, <&dmac0 0x56>; + dma-names = "tx", "rx"; + power-domains = <&cpg_clocks>; + status = "disabled"; + }; + + scif4: serial@e6c40000 { + compatible = "renesas,scif-r8a7795", "renesas,scif"; + reg = <0 0xe6c40000 0 64>; + interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp2_clks R8A7795_CLK_SCIF4>; + clock-names = "sci_ick"; + dmas = <&dmac0 0x59>, <&dmac0 0x58>; + dma-names = "tx", "rx"; + power-domains = <&cpg_clocks>; + status = "disabled"; + }; + + scif5: serial@e6f30000 { + compatible = "renesas,scif-r8a7795", "renesas,scif"; + reg = <0 0xe6f30000 0 64>; + interrupts = <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&mstp2_clks R8A7795_CLK_SCIF5>; + clock-names = "sci_ick"; + dmas = <&dmac1 0x5b>, <&dmac1 0x5a>; + dma-names = "tx", "rx"; + power-domains = <&cpg_clocks>; + status = "disabled"; + }; }; }; --- 0012/include/dt-bindings/clock/r8a7795-clock.h +++ work/include/dt-bindings/clock/r8a7795-clock.h 2015-08-29 18:22:14.282366518 +0900 @@ -22,8 +22,14 @@ /* MSTP1 */ /* MSTP2 */ +#define R8A7795_CLK_SCIF5 2 +#define R8A7795_CLK_SCIF4 3 +#define R8A7795_CLK_SCIF3 4 +#define R8A7795_CLK_SCIF1 6 +#define R8A7795_CLK_SCIF0 7 /* MSTP3 */ +#define R8A7795_CLK_SCIF2 10 /* MSTP5 */