| Message ID | 20130404190053.GA4371@kahuna (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 05 April 2013 12:30 AM, Nishanth Menon wrote: > On 10:43-20130404, Rajendra Nayak wrote: >> On Thursday 04 April 2013 08:22 AM, Nishanth Menon wrote: >>> On 11:47-20130403, Kevin Hilman wrote: >>>> Nishanth Menon <nm@ti.com> writes: >>>> >>> [...] >>>>> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c >>>>> index afa509a..5b147ef 100644 >>>>> --- a/arch/arm/mach-omap2/board-generic.c >>>>> +++ b/arch/arm/mach-omap2/board-generic.c >>>>> @@ -49,6 +49,11 @@ static void __init omap_generic_init(void) >>>>> omap4_panda_display_init_of(); >>>>> else if (of_machine_is_compatible("ti,omap4-sdp")) >>>>> omap_4430sdp_display_init_of(); >>>>> + >>>>> + if (IS_ENABLED(CONFIG_GENERIC_CPUFREQ_CPU0)) { >>>>> + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; >>>>> + platform_device_register_full(&devinfo); >>>>> + } >>>> >>>> Rather than adding new clkdev nodes below, how about using clk add_alias >>>> here? >>> Thanks for pointing this out, I spend some time implementing such a >>> scheme and following is my opinion: >>> >>> Summary: >>> There is one major problem which forces us to introduce this "clock >>> hack" - clock nodes are not in device tree yet. Yes, clock add alias >> >> There's already a patch floating around for this.. >> https://lkml.org/lkml/2013/4/2/84 > Not really. Try on OMAP4 PandaBoard: > Based on > git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git > for_3.10/dts d114294 ARM: dts: AM33XX: Corrects typo in interrupt field in SPI node > + the patch https://patchwork.kernel.org/patch/2335671/ applied > Pandaboard 4 Log: http://pastebin.com/qsdsbv7p > The Patch does not even work, unless there is un-documented patch > dependencies! I guess Roger responded to that. > > Secondly, even if it did work, it would still continue to need yet another > hack[1] Why do you think thats a hack? Besides I don't know if you are doing it right. - I am agree with Tony in his discussion in > http://marc.info/?t=136370325600009&r=1&w=2 > > *if* we are moving clock to DT, we should move the data to DT as well - I don't think Tony said 'move all clock data into DT'. He was suggesting a combination of DT and /lib/firmware > similar to what other platforms do - highbank as far as i can quickly > see. (drivers/clk/clk-highbank.c and arch/arm/boot/dts/ecx-common.dtsi Well, you chose to look at highbank (which has 8 clock nodes in DT, as opposed to OMAP5 which has around 250 in kernel) why don't you also look at imx? drivers/clk/mxs/clk-imx28.c and Documentation/devicetree/bindings/clock/imx28-clock.txt for examples. They have around 65 nodes (still way lesser than OMAP) and see what they do. Thats exactly what Roger does in his patch. regards, Rajendra > as examples). Clk alias might have been a solution, but in this case, > clk add alias (as I indicated in this thread is not really worth the > effort for the mess of code it creates for cpu clock). > > [1] > diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi > index dd8f58f..9282b4c 100644 > --- a/arch/arm/boot/dts/omap4.dtsi > +++ b/arch/arm/boot/dts/omap4.dtsi > @@ -635,4 +635,9 @@ > ti,has-mailbox; > }; > }; > + > + dpll_mpu: scrmclks { > + compatible = "ti,omap4-clock"; > + #clock-cells = <1>; > + }; > }; > diff --git a/arch/arm/boot/dts/omap443x.dtsi b/arch/arm/boot/dts/omap443x.dtsi > index cccf39a..1587a5f 100644 > --- a/arch/arm/boot/dts/omap443x.dtsi > +++ b/arch/arm/boot/dts/omap443x.dtsi > @@ -21,6 +21,8 @@ > 800000 1313000 > 1008000 1375000 > >; > + clocks = <&dpll_mpu>; > + clock-names = "cpu"; > clock-latency = <300000>; /* From legacy driver */ > }; > }; > diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c > index afa509a..5b147ef 100644 > --- a/arch/arm/mach-omap2/board-generic.c > +++ b/arch/arm/mach-omap2/board-generic.c > @@ -49,6 +49,11 @@ static void __init omap_generic_init(void) > omap4_panda_display_init_of(); > else if (of_machine_is_compatible("ti,omap4-sdp")) > omap_4430sdp_display_init_of(); > + > + if (IS_ENABLED(CONFIG_GENERIC_CPUFREQ_CPU0)) { > + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; > + platform_device_register_full(&devinfo); > + } > } > > #ifdef CONFIG_SOC_OMAP2420 > diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c > index a93617b..ba4562a 100644 > --- a/arch/arm/mach-omap2/cclock44xx_data.c > +++ b/arch/arm/mach-omap2/cclock44xx_data.c > @@ -1675,6 +1675,7 @@ static struct clk *dt_clks[] = { > &auxclk3_ck, > &auxclk4_ck, > &auxclk5_ck, > + &dpll_mpu_ck, > }; > > static struct clk_onecell_data clock_data; > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 5, 2013 at 4:50 AM, Rajendra Nayak <rnayak@ti.com> wrote: > On Friday 05 April 2013 12:30 AM, Nishanth Menon wrote: >> On 10:43-20130404, Rajendra Nayak wrote: >>> On Thursday 04 April 2013 08:22 AM, Nishanth Menon wrote: >>>> On 11:47-20130403, Kevin Hilman wrote: >>>>> Nishanth Menon <nm@ti.com> writes: >>>>> >>>> [...] >>>>>> diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c >>>>>> index afa509a..5b147ef 100644 >>>>>> --- a/arch/arm/mach-omap2/board-generic.c >>>>>> +++ b/arch/arm/mach-omap2/board-generic.c >>>>>> @@ -49,6 +49,11 @@ static void __init omap_generic_init(void) >>>>>> omap4_panda_display_init_of(); >>>>>> else if (of_machine_is_compatible("ti,omap4-sdp")) >>>>>> omap_4430sdp_display_init_of(); >>>>>> + >>>>>> + if (IS_ENABLED(CONFIG_GENERIC_CPUFREQ_CPU0)) { >>>>>> + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; >>>>>> + platform_device_register_full(&devinfo); >>>>>> + } >>>>> >>>>> Rather than adding new clkdev nodes below, how about using clk add_alias >>>>> here? >>>> Thanks for pointing this out, I spend some time implementing such a >>>> scheme and following is my opinion: >>>> >>>> Summary: >>>> There is one major problem which forces us to introduce this "clock >>>> hack" - clock nodes are not in device tree yet. Yes, clock add alias >>> >>> There's already a patch floating around for this.. >>> https://lkml.org/lkml/2013/4/2/84 >> Not really. Try on OMAP4 PandaBoard: >> Based on >> git://git.kernel.org/pub/scm/linux/kernel/git/bcousson/linux-omap-dt.git >> for_3.10/dts d114294 ARM: dts: AM33XX: Corrects typo in interrupt field in SPI node >> + the patch https://patchwork.kernel.org/patch/2335671/ applied >> Pandaboard 4 Log: http://pastebin.com/qsdsbv7p >> The Patch does not even work, unless there is un-documented patch >> dependencies! > > I guess Roger responded to that. yep, I am beyond that now :) > >> >> Secondly, even if it did work, it would still continue to need yet another >> hack[1] > > Why do you think thats a hack? Besides I don't know if you are > doing it right. traditionally, we state the following: DT will represent actual hardware data. what we are doing is the following: DT has virtual clk nodes and kernel contains the actual data. This is counter intuitive to the original idea of DT containing hardware data - hence the notion, IMHO, of a hack. > > - I am agree with Tony in his discussion in >> http://marc.info/?t=136370325600009&r=1&w=2 >> >> *if* we are moving clock to DT, we should move the data to DT as well - > > I don't think Tony said 'move all clock data into DT'. He was suggesting > a combination of DT and /lib/firmware > >> similar to what other platforms do - highbank as far as i can quickly >> see. (drivers/clk/clk-highbank.c and arch/arm/boot/dts/ecx-common.dtsi > > Well, you chose to look at highbank (which has 8 clock nodes in DT, as > opposed to OMAP5 which has around 250 in kernel) why don't you also look at imx? > drivers/clk/mxs/clk-imx28.c and Documentation/devicetree/bindings/clock/imx28-clock.txt > for examples. > They have around 65 nodes (still way lesser than OMAP) and see what > they do. Thats exactly what Roger does in his patch. > Precisely - thanks for highlighting my view - all the more reason for us to move clock data out of kernel image :). $ c_count arch/arm/mach-omap2/cclock*_data.c|tail -1 8110 $ wc -l arch/arm/mach-omap2/cclock*_data.c|tail -1 10310 total that much code sitting in kernel makes it heavier for us(and others working on clock framework code) - we do also foresee that eventually dt data might move out to it's own repository. Further, maintaining dts vs kernel code compatibility aside, consider adding new platforms - more code containing clock data that we have to carry on in kernel - If we have ! I love the idea that Roger's patch does, as a *step #1* of transitioning clock data out-of-kernel image and enabling drivers to entitle DT based boot. if this is the only step we will ever take, then it will be a disappointing decision. whether the final data goes to /lib/firmware or arch/arm/boot/dts - even though, I prefer it in dts, I have no strong feelings about it. However, we need to be prepared (at the earliest possible time) to move that data out of kernel image as *step #2*. Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Nishanth Menon <nm@ti.com> [130405 04:31]: > On Fri, Apr 5, 2013 at 4:50 AM, Rajendra Nayak <rnayak@ti.com> wrote: > > On Friday 05 April 2013 12:30 AM, Nishanth Menon wrote: > > - I am agree with Tony in his discussion in > >> http://marc.info/?t=136370325600009&r=1&w=2 > >> > >> *if* we are moving clock to DT, we should move the data to DT as well - > > > > I don't think Tony said 'move all clock data into DT'. He was suggesting > > a combination of DT and /lib/firmware Yes that also covers the case of 100% DT clocks and 0% /lib/firmware clocks if somebody wants that. But most usable combination would be: 1. Parent clocks allocated by the driver 2. Timer, serial, MMC, Ethernet and whatever is needed for mounting root clocks defined in DT 3. The rest of the clocs loaded from /lib/firmware > >> similar to what other platforms do - highbank as far as i can quickly > >> see. (drivers/clk/clk-highbank.c and arch/arm/boot/dts/ecx-common.dtsi > > > > Well, you chose to look at highbank (which has 8 clock nodes in DT, as > > opposed to OMAP5 which has around 250 in kernel) why don't you also look at imx? > > drivers/clk/mxs/clk-imx28.c and Documentation/devicetree/bindings/clock/imx28-clock.txt > > for examples. > > They have around 65 nodes (still way lesser than OMAP) and see what > > they do. Thats exactly what Roger does in his patch. > > > > Precisely - thanks for highlighting my view - all the more reason for > us to move clock data out of kernel image :). > $ c_count arch/arm/mach-omap2/cclock*_data.c|tail -1 > 8110 > $ wc -l arch/arm/mach-omap2/cclock*_data.c|tail -1 > 10310 total > > that much code sitting in kernel makes it heavier for us(and others > working on clock framework code) - > we do also foresee that eventually dt data might move out to it's own > repository. Yes we want to get rid of the large amounts of static data in the kernel. It's just a constant source for flames, and I'm getting pretty tired of that. We already did that with pinctrl-single.c for the mux data, so it's cleary doable even with more complex data such as the clock data or hwmod data. BTW, regarding the legacy mux data, although we have not yet been able to remove the legacy mux data, we have am33xx already using it, and can drop the omap4 legacy mux data probably for v3.11. > Further, maintaining dts vs kernel code compatibility aside, consider > adding new platforms - more code containing clock data that we have to > carry on in kernel - If we have ! > > I love the idea that Roger's patch does, as a *step #1* of > transitioning clock data out-of-kernel image and enabling drivers to > entitle DT based boot. > > if this is the only step we will ever take, then it will be a > disappointing decision. whether the final data goes to /lib/firmware > or arch/arm/boot/dts - even though, I prefer it in dts, I have no > strong feelings about it. > However, we need to be prepared (at the earliest possible time) to > move that data out of kernel image as *step #2*. Yes agreed, Roger's approach is a good first step. It just needs to be a proper device driver under drivers/clock/omap. Also note that the /lib/firmware part is an additional step to the DT defined clocks, but might actually make it easier to implement various PM related bits than trying to map them as DT properties. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi index dd8f58f..9282b4c 100644 --- a/arch/arm/boot/dts/omap4.dtsi +++ b/arch/arm/boot/dts/omap4.dtsi @@ -635,4 +635,9 @@ ti,has-mailbox; }; }; + + dpll_mpu: scrmclks { + compatible = "ti,omap4-clock"; + #clock-cells = <1>; + }; }; diff --git a/arch/arm/boot/dts/omap443x.dtsi b/arch/arm/boot/dts/omap443x.dtsi index cccf39a..1587a5f 100644 --- a/arch/arm/boot/dts/omap443x.dtsi +++ b/arch/arm/boot/dts/omap443x.dtsi @@ -21,6 +21,8 @@ 800000 1313000 1008000 1375000 >; + clocks = <&dpll_mpu>; + clock-names = "cpu"; clock-latency = <300000>; /* From legacy driver */ }; }; diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c index afa509a..5b147ef 100644 --- a/arch/arm/mach-omap2/board-generic.c +++ b/arch/arm/mach-omap2/board-generic.c @@ -49,6 +49,11 @@ static void __init omap_generic_init(void) omap4_panda_display_init_of(); else if (of_machine_is_compatible("ti,omap4-sdp")) omap_4430sdp_display_init_of(); + + if (IS_ENABLED(CONFIG_GENERIC_CPUFREQ_CPU0)) { + struct platform_device_info devinfo = { .name = "cpufreq-cpu0", }; + platform_device_register_full(&devinfo); + } } #ifdef CONFIG_SOC_OMAP2420 diff --git a/arch/arm/mach-omap2/cclock44xx_data.c b/arch/arm/mach-omap2/cclock44xx_data.c index a93617b..ba4562a 100644 --- a/arch/arm/mach-omap2/cclock44xx_data.c +++ b/arch/arm/mach-omap2/cclock44xx_data.c @@ -1675,6 +1675,7 @@ static struct clk *dt_clks[] = { &auxclk3_ck, &auxclk4_ck, &auxclk5_ck, + &dpll_mpu_ck, }; static struct clk_onecell_data clock_data;