| Message ID | CAMuHMdXDuG5izkMCrBmTtznbFNQzXUVLKTrG9a_K=L2qbbkv5Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hello, On Wednesday 19 August 2015 09:49:28 Geert Uytterhoeven wrote: > On Tue, Aug 18, 2015 at 2:20 AM, Michael Turquette wrote: > > Quoting Geert Uytterhoeven (2015-08-04 05:34:06) > >> On Tue, Aug 4, 2015 at 2:22 PM, Laurent Pinchart wrote: > >>> On Monday 03 August 2015 01:53:23 Kuninori Morimoto wrote: > >>>> --- /dev/null > >>>> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi > >>>> @@ -0,0 +1,93 @@ > >>>> +/ { > >>>> + clocks { > >>> > >>> Let's try to make it right from the start on Gen3. The CPG node should > >>> be a direct child of the bus node mentioned above, and the MSTP clocks > >>> should be children of the CPG node. > >> > >> Agreed. > >> > >>> I'm not sure where to put the non-memory-mapped clocks though, should > >>> they be directly under the root node ? It would make sense for > >>> extal_clk, but how about the fixed-factor clocks ? Should they be > >>> children of the CPG node too ? > >> > >> I believe the current trend is to put clocks like "extal_clk" under the > >> root node. As the fixed-factor clocks are generated by the CPG module, > >> and we do have a device node for it, I'd make them children of the CPG > >> node, too. > >> > >> Any comments from the clk+dt experts? > > > > I don't know if anyone is an expert ;-) > > > > extal_clk should be under the root node. That is true for all > > board-level clocks and clock controllers. > > OK. I agree too. > > Within the SoC we want to model the clock controller as a node in DT, > > not necessarily all of the individual clocks. So you definitely need a > > "cpg" node in DT with #clock-cells > 0. > > OK. Ditto. > > Whether or not you enumerate the individual clocks in DT is up to you. I > > do not like the data-driven approach of putting the clock definition > > data into DT. It makes it awkward to do things like set a flag on a > > single clock later on. Simply using the clock controller phandle plus > > one or more offsets is preferred over a per-clock phandle. > > > > Stephen and I have been discussing what a formal clock-controller > > binding would look like, and one item we came up with is that any > > sub-nodes of the controller would not be allowed to have a #clock-cells > > property. > > Does that mean #clock-cells is inherited from the parent (cpg) node, or does > that preclude having any "fixed-factor-clock" or "renesas,cpg-div6-clock" > (both use #clock-cells = <0>), or "renesas,cpg-mstp-clocks" subnodes? We were thinking about moving the fixed factor clocks and the gate clocks as subnodes as the CPG DT node, as those clocks are provided by the CPG. This would require setting #clock-cells to 1 in the CPG node and to 0 in some of the subnodes. If that's not allowed, how should the fixed factor clocks and gate clocks be declared ? > > Also, while you're thinking about the perfect clock binding, please do > > consider dropping clock-output-names if you can. Specifying clock-names > > alongside the clocks property inside of the consumer node is a bit more > > elegant in my opinion. This is also a bit easier if you think about > > expressing your clock data with C inside of your provider driver. > > Makes sense. I don't think anything relies on the "clock-output-names" > ... currently. > > I was going to use it for identifying the GIC clock, though: > > --- a/drivers/clk/shmobile/clk-mstp.c > +++ b/drivers/clk/shmobile/clk-mstp.c > @@ -144,6 +144,9 @@ cpg_mstp_clock_register(const char *name, const char > *paren init.name = name; > init.ops = &cpg_mstp_clock_ops; > init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT; > + /* INTC-SYS is the module clock of the GIC, and must not be disabled > */ > + if (!strcmp(name, "intc-sys")) > + init.flags |= CLK_ENABLE_HAND_OFF; > init.parent_names = &parent_name; > init.num_parents = 1; > > (FTR, on R-Car Gen3 the clock is called "intc-ap", not "intc-sys", doh...). > > This does put some reliance on (undocumented) naming in DT, though, so not > having the "clock-output-names" would solve that. Using the clock name is indeed very messy, let's avoid that. > However, while "intc-sys"/"intc-ap" (if existent) is always MSTP408, MSTP408 > is used for other modules on R-Car Gen1 and most older SoCs. so it would > complicate the driver code. How about setting the flag based on information provided in DT ? > >>>> + #address-cells = <2>; > >>>> + #size-cells = <2>; > >>>> + ranges; > >>>> + > >>>> + extal_clk: extal_clk { > >>>> + compatible = "fixed-clock"; > >>>> + #clock-cells = <0>; > >>>> + clock-frequency = <0>; > >>>> + clock-output-names = "extal"; > >>>> + }; > >>>> + cpg_clocks: cpg_clocks@e6150000 { > >>>> + compatible = "renesas,r8a7795-cpg-clocks", > >>>> + "renesas,rcar-gen3-cpg-clocks"; > >>>> + reg = <0 0xe6150000 0 0x1000>; > >>>> + clocks = <&extal_clk>; > >>>> + #clock-cells = <1>; > >>>> + clock-output-names = "main", "pll0", > >>>> "pll1","pll2", > >>>> + "pll3", "pll4"; > >>>> + }; > >>>> + p_clk: p_clk { > >>>> + compatible = "fixed-factor-clock"; > >>>> + clocks = <&cpg_clocks RCAR_GEN3_CLK_PLL1>; > >>>> + #clock-cells = <0>; > >>>> + clock-div = <24>; > >>>> + clock-mult = <1>; > >>>> + clock-output-names = "p"; > >>>> + }; > >>>> + mstp3_clks: mstp3_clks@e615013c { > >>>> + compatible = "renesas,r8a7795-mstp-clocks", > >>>> + "renesas,cpg-mstp-clocks"; > >>>> + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; > >>>> + clocks = <&p_clk>; > >>>> + #clock-cells = <1>; > >>>> + renesas,clock-indices = <RCAR_GEN3_CLK_IRDA>; > >>>> + clock-output-names = "irda"; > >>>> + }; > >>>> + }; > >>>> +};
Hi Laurent, On Wed, Aug 19, 2015 at 11:29 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> > Also, while you're thinking about the perfect clock binding, please do >> > consider dropping clock-output-names if you can. Specifying clock-names >> > alongside the clocks property inside of the consumer node is a bit more >> > elegant in my opinion. This is also a bit easier if you think about >> > expressing your clock data with C inside of your provider driver. >> >> Makes sense. I don't think anything relies on the "clock-output-names" >> ... currently. >> >> I was going to use it for identifying the GIC clock, though: >> >> --- a/drivers/clk/shmobile/clk-mstp.c >> +++ b/drivers/clk/shmobile/clk-mstp.c >> @@ -144,6 +144,9 @@ cpg_mstp_clock_register(const char *name, const char >> *paren init.name = name; >> init.ops = &cpg_mstp_clock_ops; >> init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT; >> + /* INTC-SYS is the module clock of the GIC, and must not be disabled >> */ >> + if (!strcmp(name, "intc-sys")) >> + init.flags |= CLK_ENABLE_HAND_OFF; >> init.parent_names = &parent_name; >> init.num_parents = 1; >> >> (FTR, on R-Car Gen3 the clock is called "intc-ap", not "intc-sys", doh...). >> >> This does put some reliance on (undocumented) naming in DT, though, so not >> having the "clock-output-names" would solve that. > > Using the clock name is indeed very messy, let's avoid that. > >> However, while "intc-sys"/"intc-ap" (if existent) is always MSTP408, MSTP408 >> is used for other modules on R-Car Gen1 and most older SoCs. so it would >> complicate the driver code. > > How about setting the flag based on information provided in DT ? Like scanning DT for the GIC, and looking for its clock? I don't want to add an explicit flag in DT, as it's not a hardware property, but a deficiency of the current GIC driver/subsystem. 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 20 August 2015 09:43:34 Geert Uytterhoeven wrote: > On Wed, Aug 19, 2015 at 11:29 PM, Laurent Pinchart wrote: > >> > Also, while you're thinking about the perfect clock binding, please do > >> > consider dropping clock-output-names if you can. Specifying clock-names > >> > alongside the clocks property inside of the consumer node is a bit more > >> > elegant in my opinion. This is also a bit easier if you think about > >> > expressing your clock data with C inside of your provider driver. > >> > >> Makes sense. I don't think anything relies on the "clock-output-names" > >> ... currently. > >> > >> I was going to use it for identifying the GIC clock, though: > >> > >> --- a/drivers/clk/shmobile/clk-mstp.c > >> +++ b/drivers/clk/shmobile/clk-mstp.c > >> @@ -144,6 +144,9 @@ cpg_mstp_clock_register(const char *name, const char > >> *paren init.name = name; > >> > >> init.ops = &cpg_mstp_clock_ops; > >> init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT; > >> > >> + /* INTC-SYS is the module clock of the GIC, and must not be > >> disabled */ > >> + if (!strcmp(name, "intc-sys")) > >> + init.flags |= CLK_ENABLE_HAND_OFF; > >> > >> init.parent_names = &parent_name; > >> init.num_parents = 1; > >> > >> (FTR, on R-Car Gen3 the clock is called "intc-ap", not "intc-sys", > >> doh...). > >> > >> This does put some reliance on (undocumented) naming in DT, though, so > >> not > >> having the "clock-output-names" would solve that. > > > > Using the clock name is indeed very messy, let's avoid that. > > > >> However, while "intc-sys"/"intc-ap" (if existent) is always MSTP408, > >> MSTP408 is used for other modules on R-Car Gen1 and most older SoCs. so > >> it would complicate the driver code. > > > > How about setting the flag based on information provided in DT ? > > Like scanning DT for the GIC, and looking for its clock? > > I don't want to add an explicit flag in DT, as it's not a hardware property, > but a deficiency of the current GIC driver/subsystem. What are the changes we can fix the GIC driver in a reasonable time frame ?
Hi Laurent, On Thu, Aug 20, 2015 at 9:48 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thursday 20 August 2015 09:43:34 Geert Uytterhoeven wrote: >> On Wed, Aug 19, 2015 at 11:29 PM, Laurent Pinchart wrote: >> >> > Also, while you're thinking about the perfect clock binding, please do >> >> > consider dropping clock-output-names if you can. Specifying clock-names >> >> > alongside the clocks property inside of the consumer node is a bit more >> >> > elegant in my opinion. This is also a bit easier if you think about >> >> > expressing your clock data with C inside of your provider driver. >> >> >> >> Makes sense. I don't think anything relies on the "clock-output-names" >> >> ... currently. >> >> >> >> I was going to use it for identifying the GIC clock, though: >> >> >> >> --- a/drivers/clk/shmobile/clk-mstp.c >> >> +++ b/drivers/clk/shmobile/clk-mstp.c >> >> @@ -144,6 +144,9 @@ cpg_mstp_clock_register(const char *name, const char >> >> *paren init.name = name; >> >> >> >> init.ops = &cpg_mstp_clock_ops; >> >> init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT; >> >> >> >> + /* INTC-SYS is the module clock of the GIC, and must not be >> >> disabled */ >> >> + if (!strcmp(name, "intc-sys")) >> >> + init.flags |= CLK_ENABLE_HAND_OFF; >> >> >> >> init.parent_names = &parent_name; >> >> init.num_parents = 1; >> >> >> >> (FTR, on R-Car Gen3 the clock is called "intc-ap", not "intc-sys", >> >> doh...). >> >> >> >> This does put some reliance on (undocumented) naming in DT, though, so >> >> not >> >> having the "clock-output-names" would solve that. >> > >> > Using the clock name is indeed very messy, let's avoid that. >> > >> >> However, while "intc-sys"/"intc-ap" (if existent) is always MSTP408, >> >> MSTP408 is used for other modules on R-Car Gen1 and most older SoCs. so >> >> it would complicate the driver code. >> > >> > How about setting the flag based on information provided in DT ? >> >> Like scanning DT for the GIC, and looking for its clock? >> >> I don't want to add an explicit flag in DT, as it's not a hardware property, >> but a deficiency of the current GIC driver/subsystem. > > What are the changes we can fix the GIC driver in a reasonable time frame ? Very low. The GIC driver uses IRQCHIP_DECLARE(), hence it's initialized from of_irq_init() <- irqchip_init() <- init_IRQ() <- start_kernel(). The MSTP clock driver uses CLK_OF_DECLARE(), hence it's initialized from of_clk_init() <- time_init() <- start_kernel(), but 7 calls later. IRQCHIP_DECLARE() doesn't support deferred probing. 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 Torvald -- 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 Mike, On Wed, Aug 19, 2015 at 9:49 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Aug 18, 2015 at 2:20 AM, Michael Turquette > <mturquette@baylibre.com> wrote: >> Also, while you're thinking about the perfect clock binding, please do >> consider dropping clock-output-names if you can. Specifying clock-names >> alongside the clocks property inside of the consumer node is a bit more >> elegant in my opinion. This is also a bit easier if you think about >> expressing your clock data with C inside of your provider driver. > > Makes sense. I don't think anything relies on the "clock-output-names" > ... currently. BTW, was "dropping clock-output-names" a general comment for all clock drivers, or specific to the SoC's Clock Pulse Generator? For e.g. "fixed-factor-clock", the driver falls back to using the node's name if "clock-output-names" is not present. But e.g. for MSTP clocks, that can't be done, as the clocks wouldn't have names in that case (single node with multiple clocks). Unless we start fabricating them from the node name and the indices. > I was going to use it for identifying the GIC clock, though: That applies to MSTP clocks ("intc-sys"), not the main CPG block clocks. 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
--- a/drivers/clk/shmobile/clk-mstp.c +++ b/drivers/clk/shmobile/clk-mstp.c @@ -144,6 +144,9 @@ cpg_mstp_clock_register(const char *name, const char *paren init.name = name; init.ops = &cpg_mstp_clock_ops; init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT; + /* INTC-SYS is the module clock of the GIC, and must not be disabled */ + if (!strcmp(name, "intc-sys")) + init.flags |= CLK_ENABLE_HAND_OFF; init.parent_names = &parent_name; init.num_parents = 1;