| Message ID | 1515377863-20358-13-git-send-email-david@lechnology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) > +{ > + struct clk_onecell_data *clk_data; > + > + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); > + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); > + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); > + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); > + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); > + clk_register_clkdev(clk_data->clks[14], "arm", NULL); > + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); > + > + clk_free_onecell_data(clk_data); > + > + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); Is this con_id really needed now? Searching for "usb20_psc_clk" in your tree results in only this one hit. > + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); > + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); I guess multiple dev_id matches like these are another hurdle in moving them to davinci_psc_clk_info[] table? If its too cumbersome to keep multiple entries in the table, they can be handled as an exception at the end of processing the table? Still they are not the norm so I hope the normal case will still benefit. > + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); > + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); > + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); > + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); > + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); > + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); > + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); > + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); > + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); > + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); > + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); > + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); > + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); > + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); > + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); > + > + clk_free_onecell_data(clk_data); > +} Thanks, Sekhar
On 01/16/2018 08:00 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >> +{ >> + struct clk_onecell_data *clk_data; >> + >> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >> + >> + clk_free_onecell_data(clk_data); >> + >> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); > > Is this con_id really needed now? Searching for "usb20_psc_clk" in your > tree results in only this one hit. Yes, this is left over from previous attempts. > >> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); > > I guess multiple dev_id matches like these are another hurdle in moving > them to davinci_psc_clk_info[] table? If its too cumbersome to keep > multiple entries in the table, they can be handled as an exception at > the end of processing the table? Still they are not the norm so I hope > the normal case will still benefit. Right, as I mentioned in the reply to the previous patch, instead of assigning a con_id and dev_id to each clock, we would need to assign an array with a list of clocks. I think that would work better than trying to handle the extras as an exception since there, on average, about 5 per SoC. > >> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >> + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); >> + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); >> + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); >> + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); >> + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); >> + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); >> + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); >> + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); >> + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); >> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); >> + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); >> + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); >> + >> + clk_free_onecell_data(clk_data); >> +} > > Thanks, > Sekhar >
On Tuesday 16 January 2018 10:51 PM, David Lechner wrote: > On 01/16/2018 08:00 AM, Sekhar Nori wrote: >> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>> +{ >>> + struct clk_onecell_data *clk_data; >>> + >>> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >>> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >>> + >>> + clk_free_onecell_data(clk_data); >>> + >>> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >> >> Is this con_id really needed now? Searching for "usb20_psc_clk" in your >> tree results in only this one hit. > > Yes, this is left over from previous attempts. > >> >>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >> >> I guess multiple dev_id matches like these are another hurdle in moving >> them to davinci_psc_clk_info[] table? If its too cumbersome to keep >> multiple entries in the table, they can be handled as an exception at >> the end of processing the table? Still they are not the norm so I hope >> the normal case will still benefit. > > Right, as I mentioned in the reply to the previous patch, instead of > assigning a con_id and dev_id to each clock, we would need to assign > an array with a list of clocks. I think that would work better than > trying to handle the extras as an exception since there, on average, > about 5 per SoC. Okay, are you going to try this to see how it looks? It looks like samsung (clk-s3c2410.c) and tegra (clk-tegra20.c) use such tables (although both use separate tables mapping just the gate number to con_id/dev_id). Others like u8540_clk.c and clk-mmp2.c have multiple calls in code to clk_register_clkdev() like you have, but they keep them right after the gate clock registration which makes it easy to see the mapping. clk-imx35.c has multiple clk_register_clkdev() calls, but uses an enum for the gates so its easy to see the mapping. This approach looks fine to me as well. So looks like there is a whole gamut of ways people have approached this. But I do think we need to change the scheme you have currently since it is difficult to review and audit (believe me on this one :)) Thanks, Sekhar
On 01/17/2018 05:57 AM, Sekhar Nori wrote: > On Tuesday 16 January 2018 10:51 PM, David Lechner wrote: >> On 01/16/2018 08:00 AM, Sekhar Nori wrote: >>> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>>> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>>> +{ >>>> + struct clk_onecell_data *clk_data; >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >>>> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >>>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >>>> + >>>> + clk_free_onecell_data(clk_data); >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >>> >>> Is this con_id really needed now? Searching for "usb20_psc_clk" in your >>> tree results in only this one hit. >> >> Yes, this is left over from previous attempts. >> >>> >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>> >>> I guess multiple dev_id matches like these are another hurdle in moving >>> them to davinci_psc_clk_info[] table? If its too cumbersome to keep >>> multiple entries in the table, they can be handled as an exception at >>> the end of processing the table? Still they are not the norm so I hope >>> the normal case will still benefit. >> >> Right, as I mentioned in the reply to the previous patch, instead of >> assigning a con_id and dev_id to each clock, we would need to assign >> an array with a list of clocks. I think that would work better than >> trying to handle the extras as an exception since there, on average, >> about 5 per SoC. > > Okay, are you going to try this to see how it looks? It looks like > samsung (clk-s3c2410.c) and tegra (clk-tegra20.c) use such tables > (although both use separate tables mapping just the gate number to > con_id/dev_id). > > Others like u8540_clk.c and clk-mmp2.c have multiple calls in code to > clk_register_clkdev() like you have, but they keep them right after the > gate clock registration which makes it easy to see the mapping. > > clk-imx35.c has multiple clk_register_clkdev() calls, but uses an enum > for the gates so its easy to see the mapping. This approach looks fine > to me as well. > > So looks like there is a whole gamut of ways people have approached > this. But I do think we need to change the scheme you have currently > since it is difficult to review and audit (believe me on this one :)) > OK, I'll figure out something here.
On 01/17/2018 05:57 AM, Sekhar Nori wrote: > On Tuesday 16 January 2018 10:51 PM, David Lechner wrote: >> On 01/16/2018 08:00 AM, Sekhar Nori wrote: >>> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>>> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>>> +{ >>>> + struct clk_onecell_data *clk_data; >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >>>> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >>>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >>>> + >>>> + clk_free_onecell_data(clk_data); >>>> + >>>> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >>>> + if (!clk_data) >>>> + return; >>>> + >>>> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >>> >>> Is this con_id really needed now? Searching for "usb20_psc_clk" in your >>> tree results in only this one hit. >> >> Yes, this is left over from previous attempts. >> >>> >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>> >>> I guess multiple dev_id matches like these are another hurdle in moving >>> them to davinci_psc_clk_info[] table? If its too cumbersome to keep >>> multiple entries in the table, they can be handled as an exception at >>> the end of processing the table? Still they are not the norm so I hope >>> the normal case will still benefit. >> >> Right, as I mentioned in the reply to the previous patch, instead of >> assigning a con_id and dev_id to each clock, we would need to assign >> an array with a list of clocks. I think that would work better than >> trying to handle the extras as an exception since there, on average, >> about 5 per SoC. > > Okay, are you going to try this to see how it looks? It is looking like this: static const struct davinci_psc_clkdev_info emfia_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "ti-aemif"), LPSC_CLKDEV("aemif", "davinci-nand.0"), { } }; static const struct davinci_psc_clkdev_info spi0_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "spi_davinci.0"), { } }; static const struct davinci_psc_clkdev_info mmcsd0_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "da830-mmc.0"), { } }; static const struct davinci_psc_clkdev_info uart0_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "serial8250.0"), { } }; static const struct davinci_psc_clkdev_info arm_clkdev[] __initconst = { /* * REVISIT: cpufreq-davinci should be modified to use dev_id and drop * use of con_id. */ LPSC_CLKDEV("arm", NULL), { } }; static const struct davinci_psc_clkdev_info dsp_clkdev[] __initconst = { LPSC_CLKDEV(NULL, "davinci-rproc.0"), { } }; static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { LPSC(0, 0, tpcc0, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), LPSC(1, 0, tptc0, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), LPSC(2, 0, tptc1, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), LPSC(3, 0, emifa, async1, emfia_clkdev, 0), LPSC(4, 0, spi0, pll0_sysclk2, spi0_clkdev, 0), LPSC(5, 0, mmcsd0, pll0_sysclk2, mmcsd0_clkdev, 0), LPSC(6, 0, aintc, pll0_sysclk4, NULL, LPSC_ALWAYS_ENABLED), LPSC(7, 0, arm_rom, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), LPSC(9, 0, uart0, pll0_sysclk2, uart0_clkdev, 0), LPSC(13, 0, pruss, pll0_sysclk2, NULL, 0), LPSC(14, 0, arm, pll0_sysclk6, arm_clkdev, LPSC_ALWAYS_ENABLED | LPSC_ARM_RATE), LPSC(15, 1, dsp, pll0_sysclk1, dsp_clkdev, LPSC_FORCE | LPSC_LOCAL_RESET), { } };
On Thursday 18 January 2018 12:38 AM, David Lechner wrote: > It is looking like this: > > > static const struct davinci_psc_clkdev_info emfia_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "ti-aemif"), > LPSC_CLKDEV("aemif", "davinci-nand.0"), > { } > }; > > static const struct davinci_psc_clkdev_info spi0_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "spi_davinci.0"), > { } > }; > > static const struct davinci_psc_clkdev_info mmcsd0_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "da830-mmc.0"), > { } > }; > > static const struct davinci_psc_clkdev_info uart0_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "serial8250.0"), > { } > }; > > static const struct davinci_psc_clkdev_info arm_clkdev[] __initconst = { > /* > * REVISIT: cpufreq-davinci should be modified to use dev_id and drop > * use of con_id. > */ > LPSC_CLKDEV("arm", NULL), > { } > }; > > static const struct davinci_psc_clkdev_info dsp_clkdev[] __initconst = { > LPSC_CLKDEV(NULL, "davinci-rproc.0"), > { } > }; > > static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { > LPSC(0, 0, tpcc0, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), > LPSC(1, 0, tptc0, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), > LPSC(2, 0, tptc1, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), > LPSC(3, 0, emifa, async1, emfia_clkdev, 0), > LPSC(4, 0, spi0, pll0_sysclk2, spi0_clkdev, 0), > LPSC(5, 0, mmcsd0, pll0_sysclk2, mmcsd0_clkdev, 0), > LPSC(6, 0, aintc, pll0_sysclk4, NULL, LPSC_ALWAYS_ENABLED), > LPSC(7, 0, arm_rom, pll0_sysclk2, NULL, LPSC_ALWAYS_ENABLED), > LPSC(9, 0, uart0, pll0_sysclk2, uart0_clkdev, 0), > LPSC(13, 0, pruss, pll0_sysclk2, NULL, 0), > LPSC(14, 0, arm, pll0_sysclk6, arm_clkdev, LPSC_ALWAYS_ENABLED | > LPSC_ARM_RATE), > LPSC(15, 1, dsp, pll0_sysclk1, dsp_clkdev, LPSC_FORCE | > LPSC_LOCAL_RESET), > { } > }; This looks good to me! Regards, Sekhar
2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: > This adds platform-specific declarations for the PSC clocks on TI DA850/ > OMAP-L138/AM18XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > drivers/clk/davinci/Makefile | 1 + > drivers/clk/davinci/psc-da850.c | 117 ++++++++++++++++++++++++++++++++++++++++ > include/linux/clk/davinci.h | 1 + > 3 files changed, 119 insertions(+) > create mode 100644 drivers/clk/davinci/psc-da850.c > > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > index fb14c8c..aef0390 100644 > --- a/drivers/clk/davinci/Makefile > +++ b/drivers/clk/davinci/Makefile > @@ -11,4 +11,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o > > obj-y += psc.o > obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o > +obj-$(CONFIG_ARCH_DAVINCI_DA850) += psc-da850.o > endif > diff --git a/drivers/clk/davinci/psc-da850.c b/drivers/clk/davinci/psc-da850.c > new file mode 100644 > index 0000000..3b4583d > --- /dev/null > +++ b/drivers/clk/davinci/psc-da850.c > @@ -0,0 +1,117 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PSC clock descriptions for TI DA850/OMAP-L138/AM18XX > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > +#include <linux/init.h> > +#include <linux/of.h> > +#include <linux/types.h> > + > +#include "psc.h" > + > +static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { > + LPSC(0, 0, tpcc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(3, 0, aemif, pll0_sysclk3, 0), > + LPSC(4, 0, spi0, pll0_sysclk2, 0), > + LPSC(5, 0, mmcsd0, pll0_sysclk2, 0), > + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), > + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(9, 0, uart0, pll0_sysclk2, 0), > + LPSC(13, 0, pruss, pll0_sysclk2, 0), > + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), > + LPSC(15, 1, dsp, pll0_sysclk1, LPSC_FORCE | LPSC_LOCAL_RESET), > + { } > +}; > + > +static const struct davinci_psc_clk_info da850_psc1_info[] __initconst = { > + LPSC(0, 0, tpcc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + LPSC(1, 0, usb0, pll0_sysclk2, 0), > + LPSC(2, 0, usb1, pll0_sysclk4, 0), > + LPSC(3, 0, gpio, pll0_sysclk4, 0), > + LPSC(5, 0, emac, pll0_sysclk4, 0), > + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), > + LPSC(7, 0, mcasp0, async3, 0), > + LPSC(8, 0, sata, pll0_sysclk2, LPSC_FORCE), > + LPSC(9, 0, vpif, pll0_sysclk2, 0), > + LPSC(10, 0, spi1, async3, 0), > + LPSC(11, 0, i2c1, pll0_sysclk4, 0), > + LPSC(12, 0, uart1, async3, 0), > + LPSC(13, 0, uart2, async3, 0), > + LPSC(14, 0, mcbsp0, async3, 0), > + LPSC(15, 0, mcbsp1, async3, 0), > + LPSC(16, 0, lcdc, pll0_sysclk2, 0), > + LPSC(17, 0, ehrpwm, async3, 0), > + LPSC(18, 0, mmcsd1, pll0_sysclk2, 0), > + LPSC(20, 0, ecap, async3, 0), > + LPSC(21, 0, tptc2, pll0_sysclk2, LPSC_ALWAYS_ENABLED), > + { } > +}; > + > +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) > +{ > + struct clk_onecell_data *clk_data; > + > + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); > + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); > + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); > + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); > + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); > + clk_register_clkdev(clk_data->clks[14], "arm", NULL); > + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); > + > + clk_free_onecell_data(clk_data); > + > + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); > + if (!clk_data) > + return; > + > + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); > + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); > + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); > + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); > + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); > + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); > + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); > + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); > + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); > + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); > + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); > + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); > + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); > + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); > + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); > + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); > + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); > + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); > + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); > + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); > + > + clk_free_onecell_data(clk_data); > +} > + > +#ifdef CONFIG_OF > +static void __init of_da850_psc0_clk_init(struct device_node *node) > +{ > + of_davinci_psc_clk_init(node, da850_psc0_info, 16); > +} > +CLK_OF_DECLARE(da850_psc0_clk, "ti,da850-psc0", of_da850_psc0_clk_init); > + > +static void __init of_da850_psc1_clk_init(struct device_node *node) > +{ > + of_davinci_psc_clk_init(node, da850_psc1_info, 32); > +} > +CLK_OF_DECLARE(da850_psc1_clk, "ti,da850-psc1", of_da850_psc1_clk_init); > +#endif > diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h > index 3ec8100..3d8bdfa 100644 > --- a/include/linux/clk/davinci.h > +++ b/include/linux/clk/davinci.h > @@ -17,5 +17,6 @@ void dm644x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); > void dm646x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); > > void da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1); > +void da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1); > > #endif /* __LINUX_CLK_DAVINCI_H__ */ > -- > 2.7.4 > Hi David, I've been working on moving the genpd code from its own driver to the psc one. I couldn't get the system to boot though and problems happened very early in the boot sequence. I struggled to figure out what's happening, but eventually I noticed that psc uses CLK_OF_DECLARE() to initialize clocks. The functions registered this way are called very early in the boot sequence, way before late_initcall() in which the genpd framework is initialized. This of course makes it impossible to register genpd at the same time we register PSCs. Mike, Stephen - it would be great if you could confirm, but I believe the clock framework now only accepts clock drivers which create real platform drivers, in which case this series would need to be modified. David: FYI I quickly converted the functions in psc-da850.c to actual probe callbacks and it worked just fine on a da850-lcdk board. Best regards, Bartosz Golaszewski
Hi Bartosz, all, On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >> This adds platform-specific declarations for the PSC clocks on TI DA850/ >> OMAP-L138/AM18XX SoCs. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> drivers/clk/davinci/Makefile | 1 + >> drivers/clk/davinci/psc-da850.c | 117 ++++++++++++++++++++++++++++++++++++++++ >> include/linux/clk/davinci.h | 1 + >> 3 files changed, 119 insertions(+) >> create mode 100644 drivers/clk/davinci/psc-da850.c >> >> diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile >> index fb14c8c..aef0390 100644 >> --- a/drivers/clk/davinci/Makefile >> +++ b/drivers/clk/davinci/Makefile >> @@ -11,4 +11,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o >> >> obj-y += psc.o >> obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o >> +obj-$(CONFIG_ARCH_DAVINCI_DA850) += psc-da850.o >> endif >> diff --git a/drivers/clk/davinci/psc-da850.c b/drivers/clk/davinci/psc-da850.c >> new file mode 100644 >> index 0000000..3b4583d >> --- /dev/null >> +++ b/drivers/clk/davinci/psc-da850.c >> @@ -0,0 +1,117 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PSC clock descriptions for TI DA850/OMAP-L138/AM18XX >> + * >> + * Copyright (C) 2017 David Lechner <david@lechnology.com> >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/clkdev.h> >> +#include <linux/init.h> >> +#include <linux/of.h> >> +#include <linux/types.h> >> + >> +#include "psc.h" >> + >> +static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { >> + LPSC(0, 0, tpcc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(3, 0, aemif, pll0_sysclk3, 0), >> + LPSC(4, 0, spi0, pll0_sysclk2, 0), >> + LPSC(5, 0, mmcsd0, pll0_sysclk2, 0), >> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(9, 0, uart0, pll0_sysclk2, 0), >> + LPSC(13, 0, pruss, pll0_sysclk2, 0), >> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), >> + LPSC(15, 1, dsp, pll0_sysclk1, LPSC_FORCE | LPSC_LOCAL_RESET), >> + { } >> +}; >> + >> +static const struct davinci_psc_clk_info da850_psc1_info[] __initconst = { >> + LPSC(0, 0, tpcc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + LPSC(1, 0, usb0, pll0_sysclk2, 0), >> + LPSC(2, 0, usb1, pll0_sysclk4, 0), >> + LPSC(3, 0, gpio, pll0_sysclk4, 0), >> + LPSC(5, 0, emac, pll0_sysclk4, 0), >> + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), >> + LPSC(7, 0, mcasp0, async3, 0), >> + LPSC(8, 0, sata, pll0_sysclk2, LPSC_FORCE), >> + LPSC(9, 0, vpif, pll0_sysclk2, 0), >> + LPSC(10, 0, spi1, async3, 0), >> + LPSC(11, 0, i2c1, pll0_sysclk4, 0), >> + LPSC(12, 0, uart1, async3, 0), >> + LPSC(13, 0, uart2, async3, 0), >> + LPSC(14, 0, mcbsp0, async3, 0), >> + LPSC(15, 0, mcbsp1, async3, 0), >> + LPSC(16, 0, lcdc, pll0_sysclk2, 0), >> + LPSC(17, 0, ehrpwm, async3, 0), >> + LPSC(18, 0, mmcsd1, pll0_sysclk2, 0), >> + LPSC(20, 0, ecap, async3, 0), >> + LPSC(21, 0, tptc2, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >> + { } >> +}; >> + >> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >> +{ >> + struct clk_onecell_data *clk_data; >> + >> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >> + >> + clk_free_onecell_data(clk_data); >> + >> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >> + if (!clk_data) >> + return; >> + >> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >> + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); >> + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); >> + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); >> + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); >> + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); >> + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); >> + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); >> + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); >> + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); >> + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); >> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); >> + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); >> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); >> + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); >> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); >> + >> + clk_free_onecell_data(clk_data); >> +} >> + >> +#ifdef CONFIG_OF >> +static void __init of_da850_psc0_clk_init(struct device_node *node) >> +{ >> + of_davinci_psc_clk_init(node, da850_psc0_info, 16); >> +} >> +CLK_OF_DECLARE(da850_psc0_clk, "ti,da850-psc0", of_da850_psc0_clk_init); >> + >> +static void __init of_da850_psc1_clk_init(struct device_node *node) >> +{ >> + of_davinci_psc_clk_init(node, da850_psc1_info, 32); >> +} >> +CLK_OF_DECLARE(da850_psc1_clk, "ti,da850-psc1", of_da850_psc1_clk_init); >> +#endif >> diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h >> index 3ec8100..3d8bdfa 100644 >> --- a/include/linux/clk/davinci.h >> +++ b/include/linux/clk/davinci.h >> @@ -17,5 +17,6 @@ void dm644x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); >> void dm646x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); >> >> void da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1); >> +void da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1); >> >> #endif /* __LINUX_CLK_DAVINCI_H__ */ >> -- >> 2.7.4 >> > > Hi David, > > I've been working on moving the genpd code from its own driver to the > psc one. I couldn't get the system to boot though and problems > happened very early in the boot sequence. I struggled to figure out > what's happening, but eventually I noticed that psc uses > CLK_OF_DECLARE() to initialize clocks. The functions registered this > way are called very early in the boot sequence, way before > late_initcall() in which the genpd framework is initialized. This of > course makes it impossible to register genpd at the same time we > register PSCs. > > Mike, Stephen - it would be great if you could confirm, but I believe > the clock framework now only accepts clock drivers which create real > platform drivers, in which case this series would need to be modified. You are correct. All new uses of CLK_OF_DECLARE are met with high suspicion, and all new drivers should be platform drivers. There are cases when CLK_OF_DECLARE is necessary (sometimes temporarily while things are reworked), but it seems this is not the case for this driver based on your testing of the platform driver approach. Please use the platform driver approach instead. Thanks, Mike > > David: FYI I quickly converted the functions in psc-da850.c to actual > probe callbacks and it worked just fine on a da850-lcdk board. > > Best regards, > Bartosz Golaszewski
On 02/09/2018 10:48 AM, Michael Turquette wrote: > Hi Bartosz, all, > > On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >>> This adds platform-specific declarations for the PSC clocks on TI DA850/ >>> OMAP-L138/AM18XX SoCs. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >>> --- >>> drivers/clk/davinci/Makefile | 1 + >>> drivers/clk/davinci/psc-da850.c | 117 ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/clk/davinci.h | 1 + >>> 3 files changed, 119 insertions(+) >>> create mode 100644 drivers/clk/davinci/psc-da850.c >>> >>> diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile >>> index fb14c8c..aef0390 100644 >>> --- a/drivers/clk/davinci/Makefile >>> +++ b/drivers/clk/davinci/Makefile >>> @@ -11,4 +11,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o >>> >>> obj-y += psc.o >>> obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o >>> +obj-$(CONFIG_ARCH_DAVINCI_DA850) += psc-da850.o >>> endif >>> diff --git a/drivers/clk/davinci/psc-da850.c b/drivers/clk/davinci/psc-da850.c >>> new file mode 100644 >>> index 0000000..3b4583d >>> --- /dev/null >>> +++ b/drivers/clk/davinci/psc-da850.c >>> @@ -0,0 +1,117 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * PSC clock descriptions for TI DA850/OMAP-L138/AM18XX >>> + * >>> + * Copyright (C) 2017 David Lechner <david@lechnology.com> >>> + */ >>> + >>> +#include <linux/clk-provider.h> >>> +#include <linux/clkdev.h> >>> +#include <linux/init.h> >>> +#include <linux/of.h> >>> +#include <linux/types.h> >>> + >>> +#include "psc.h" >>> + >>> +static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { >>> + LPSC(0, 0, tpcc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(3, 0, aemif, pll0_sysclk3, 0), >>> + LPSC(4, 0, spi0, pll0_sysclk2, 0), >>> + LPSC(5, 0, mmcsd0, pll0_sysclk2, 0), >>> + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), >>> + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(9, 0, uart0, pll0_sysclk2, 0), >>> + LPSC(13, 0, pruss, pll0_sysclk2, 0), >>> + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), >>> + LPSC(15, 1, dsp, pll0_sysclk1, LPSC_FORCE | LPSC_LOCAL_RESET), >>> + { } >>> +}; >>> + >>> +static const struct davinci_psc_clk_info da850_psc1_info[] __initconst = { >>> + LPSC(0, 0, tpcc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + LPSC(1, 0, usb0, pll0_sysclk2, 0), >>> + LPSC(2, 0, usb1, pll0_sysclk4, 0), >>> + LPSC(3, 0, gpio, pll0_sysclk4, 0), >>> + LPSC(5, 0, emac, pll0_sysclk4, 0), >>> + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), >>> + LPSC(7, 0, mcasp0, async3, 0), >>> + LPSC(8, 0, sata, pll0_sysclk2, LPSC_FORCE), >>> + LPSC(9, 0, vpif, pll0_sysclk2, 0), >>> + LPSC(10, 0, spi1, async3, 0), >>> + LPSC(11, 0, i2c1, pll0_sysclk4, 0), >>> + LPSC(12, 0, uart1, async3, 0), >>> + LPSC(13, 0, uart2, async3, 0), >>> + LPSC(14, 0, mcbsp0, async3, 0), >>> + LPSC(15, 0, mcbsp1, async3, 0), >>> + LPSC(16, 0, lcdc, pll0_sysclk2, 0), >>> + LPSC(17, 0, ehrpwm, async3, 0), >>> + LPSC(18, 0, mmcsd1, pll0_sysclk2, 0), >>> + LPSC(20, 0, ecap, async3, 0), >>> + LPSC(21, 0, tptc2, pll0_sysclk2, LPSC_ALWAYS_ENABLED), >>> + { } >>> +}; >>> + >>> +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) >>> +{ >>> + struct clk_onecell_data *clk_data; >>> + >>> + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); >>> + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); >>> + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); >>> + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); >>> + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); >>> + clk_register_clkdev(clk_data->clks[14], "arm", NULL); >>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); >>> + >>> + clk_free_onecell_data(clk_data); >>> + >>> + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); >>> + if (!clk_data) >>> + return; >>> + >>> + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); >>> + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); >>> + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); >>> + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); >>> + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); >>> + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); >>> + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); >>> + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); >>> + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); >>> + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); >>> + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); >>> + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); >>> + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); >>> + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); >>> + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); >>> + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); >>> + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); >>> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); >>> + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); >>> + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); >>> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); >>> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); >>> + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); >>> + >>> + clk_free_onecell_data(clk_data); >>> +} >>> + >>> +#ifdef CONFIG_OF >>> +static void __init of_da850_psc0_clk_init(struct device_node *node) >>> +{ >>> + of_davinci_psc_clk_init(node, da850_psc0_info, 16); >>> +} >>> +CLK_OF_DECLARE(da850_psc0_clk, "ti,da850-psc0", of_da850_psc0_clk_init); >>> + >>> +static void __init of_da850_psc1_clk_init(struct device_node *node) >>> +{ >>> + of_davinci_psc_clk_init(node, da850_psc1_info, 32); >>> +} >>> +CLK_OF_DECLARE(da850_psc1_clk, "ti,da850-psc1", of_da850_psc1_clk_init); >>> +#endif >>> diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h >>> index 3ec8100..3d8bdfa 100644 >>> --- a/include/linux/clk/davinci.h >>> +++ b/include/linux/clk/davinci.h >>> @@ -17,5 +17,6 @@ void dm644x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); >>> void dm646x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); >>> >>> void da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1); >>> +void da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1); >>> >>> #endif /* __LINUX_CLK_DAVINCI_H__ */ >>> -- >>> 2.7.4 >>> >> >> Hi David, >> >> I've been working on moving the genpd code from its own driver to the >> psc one. I couldn't get the system to boot though and problems >> happened very early in the boot sequence. I struggled to figure out >> what's happening, but eventually I noticed that psc uses >> CLK_OF_DECLARE() to initialize clocks. The functions registered this >> way are called very early in the boot sequence, way before >> late_initcall() in which the genpd framework is initialized. This of >> course makes it impossible to register genpd at the same time we >> register PSCs. >> >> Mike, Stephen - it would be great if you could confirm, but I believe >> the clock framework now only accepts clock drivers which create real >> platform drivers, in which case this series would need to be modified. > > You are correct. All new uses of CLK_OF_DECLARE are met with high > suspicion, and all new drivers should be platform drivers. Oh boy. I'll try to get a v7 out this week with these changes. > > There are cases when CLK_OF_DECLARE is necessary (sometimes > temporarily while things are reworked), but it seems this is not the > case for this driver based on your testing of the platform driver > approach. Please use the platform driver approach instead. > > Thanks, > Mike > >> >> David: FYI I quickly converted the functions in psc-da850.c to actual >> probe callbacks and it worked just fine on a da850-lcdk board. >> >> Best regards, >> Bartosz Golaszewski
Hi Bartosz, On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: > On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >> Hi David, >> >> I've been working on moving the genpd code from its own driver to the >> psc one. I couldn't get the system to boot though and problems >> happened very early in the boot sequence. I struggled to figure out >> what's happening, but eventually I noticed that psc uses >> CLK_OF_DECLARE() to initialize clocks. The functions registered this >> way are called very early in the boot sequence, way before >> late_initcall() in which the genpd framework is initialized. This of late_initcall() is too late for genpd to be initialized. As you may have seen with the latest set of patches, we have problems with timer initialization. After converting to platform devices, PSC and PLL clocks get initialized post time_init(). We are working that around using fixed-clocks, which hopefully will work (I still need to test many of the affected platforms). Can you please reply with the exact issue you faced with genpd framework initialization so we do have that on record. Thanks, Sekhar >> course makes it impossible to register genpd at the same time we >> register PSCs. >> >> Mike, Stephen - it would be great if you could confirm, but I believe >> the clock framework now only accepts clock drivers which create real >> platform drivers, in which case this series would need to be modified. > > You are correct. All new uses of CLK_OF_DECLARE are met with high > suspicion, and all new drivers should be platform drivers. > > There are cases when CLK_OF_DECLARE is necessary (sometimes > temporarily while things are reworked), but it seems this is not the > case for this driver based on your testing of the platform driver > approach. Please use the platform driver approach instead. > > Thanks, > Mike > >> >> David: FYI I quickly converted the functions in psc-da850.c to actual >> probe callbacks and it worked just fine on a da850-lcdk board. >> >> Best regards, >> Bartosz Golaszewski
2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: > Hi Bartosz, > > On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: >> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: > >>> Hi David, >>> >>> I've been working on moving the genpd code from its own driver to the >>> psc one. I couldn't get the system to boot though and problems >>> happened very early in the boot sequence. I struggled to figure out >>> what's happening, but eventually I noticed that psc uses >>> CLK_OF_DECLARE() to initialize clocks. The functions registered this >>> way are called very early in the boot sequence, way before >>> late_initcall() in which the genpd framework is initialized. This of > > late_initcall() is too late for genpd to be initialized. As you may have > seen with the latest set of patches, we have problems with timer > initialization. After converting to platform devices, PSC and PLL clocks > get initialized post time_init(). We are working that around using > fixed-clocks, which hopefully will work (I still need to test many of > the affected platforms). > > Can you please reply with the exact issue you faced with genpd framework > initialization so we do have that on record. > The exact issue manifested itself in a NULL-pointer dereference panic when I tried moving the genpd code I had initially implemented as a separate platform driver to what I believe was v6 or v7 of David's series (before the psc driver became a platform driver, when it was still using CLK_OF_DECLARE()). When I had tested a simple conversion of that version to a platform_driver, genpd worked fine. I don't have the stack traces from these panics, but I recall some debugfs functions being involved and the genpd late_initcalls are related to debugfs. Looking at it now I don't see how exactly it could fail though. Best regards, Bartosz
On Thursday 05 April 2018 07:14 PM, Bartosz Golaszewski wrote: > 2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: >> Hi Bartosz, >> >> On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: >>> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >> >>>> Hi David, >>>> >>>> I've been working on moving the genpd code from its own driver to the >>>> psc one. I couldn't get the system to boot though and problems >>>> happened very early in the boot sequence. I struggled to figure out >>>> what's happening, but eventually I noticed that psc uses >>>> CLK_OF_DECLARE() to initialize clocks. The functions registered this >>>> way are called very early in the boot sequence, way before >>>> late_initcall() in which the genpd framework is initialized. This of >> >> late_initcall() is too late for genpd to be initialized. As you may have >> seen with the latest set of patches, we have problems with timer >> initialization. After converting to platform devices, PSC and PLL clocks >> get initialized post time_init(). We are working that around using >> fixed-clocks, which hopefully will work (I still need to test many of >> the affected platforms). >> >> Can you please reply with the exact issue you faced with genpd framework >> initialization so we do have that on record. >> > > The exact issue manifested itself in a NULL-pointer dereference panic > when I tried moving the genpd code I had initially implemented as a > separate platform driver to what I believe was v6 or v7 of David's > series (before the psc driver became a platform driver, when it was > still using CLK_OF_DECLARE()). When I had tested a simple conversion > of that version to a platform_driver, genpd worked fine. > > I don't have the stack traces from these panics, but I recall some > debugfs functions being involved and the genpd late_initcalls are > related to debugfs. Looking at it now I don't see how exactly it could > fail though. Do you have the code where you faced the problem stashed somewhere? I am not (yet) advocating going back to CLK_OF_DECLARE(). But there is a definite issue with timer being ready when not using CLK_OF_DECLARE(). So, I want to make sure there the reason why we are going down the platform device path is a amply clear. Thanks, Sekhar
On 04/05/2018 09:36 AM, Sekhar Nori wrote: > On Thursday 05 April 2018 07:14 PM, Bartosz Golaszewski wrote: >> 2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: >>> Hi Bartosz, >>> >>> On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: >>>> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>>>> Hi David, >>>>> >>>>> I've been working on moving the genpd code from its own driver to the >>>>> psc one. I couldn't get the system to boot though and problems >>>>> happened very early in the boot sequence. I struggled to figure out >>>>> what's happening, but eventually I noticed that psc uses >>>>> CLK_OF_DECLARE() to initialize clocks. The functions registered this >>>>> way are called very early in the boot sequence, way before >>>>> late_initcall() in which the genpd framework is initialized. This of >>> >>> late_initcall() is too late for genpd to be initialized. As you may have >>> seen with the latest set of patches, we have problems with timer >>> initialization. After converting to platform devices, PSC and PLL clocks >>> get initialized post time_init(). We are working that around using >>> fixed-clocks, which hopefully will work (I still need to test many of >>> the affected platforms). >>> >>> Can you please reply with the exact issue you faced with genpd framework >>> initialization so we do have that on record. >>> >> >> The exact issue manifested itself in a NULL-pointer dereference panic >> when I tried moving the genpd code I had initially implemented as a >> separate platform driver to what I believe was v6 or v7 of David's >> series (before the psc driver became a platform driver, when it was >> still using CLK_OF_DECLARE()). When I had tested a simple conversion >> of that version to a platform_driver, genpd worked fine. >> >> I don't have the stack traces from these panics, but I recall some >> debugfs functions being involved and the genpd late_initcalls are >> related to debugfs. Looking at it now I don't see how exactly it could >> fail though. > > Do you have the code where you faced the problem stashed somewhere? I am > not (yet) advocating going back to CLK_OF_DECLARE(). But there is a > definite issue with timer being ready when not using CLK_OF_DECLARE(). > So, I want to make sure there the reason why we are going down the > platform device path is a amply clear. > > Thanks, > Sekhar > This is the thread that led to the conversion to platform devices: https://lkml.org/lkml/2018/2/9/541
2018-04-05 16:36 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: > On Thursday 05 April 2018 07:14 PM, Bartosz Golaszewski wrote: >> 2018-04-05 15:09 GMT+02:00 Sekhar Nori <nsekhar@ti.com>: >>> Hi Bartosz, >>> >>> On Friday 09 February 2018 10:18 PM, Michael Turquette wrote: >>>> On Fri, Feb 9, 2018 at 8:22 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>>> 2018-01-08 3:17 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>>>> Hi David, >>>>> >>>>> I've been working on moving the genpd code from its own driver to the >>>>> psc one. I couldn't get the system to boot though and problems >>>>> happened very early in the boot sequence. I struggled to figure out >>>>> what's happening, but eventually I noticed that psc uses >>>>> CLK_OF_DECLARE() to initialize clocks. The functions registered this >>>>> way are called very early in the boot sequence, way before >>>>> late_initcall() in which the genpd framework is initialized. This of >>> >>> late_initcall() is too late for genpd to be initialized. As you may have >>> seen with the latest set of patches, we have problems with timer >>> initialization. After converting to platform devices, PSC and PLL clocks >>> get initialized post time_init(). We are working that around using >>> fixed-clocks, which hopefully will work (I still need to test many of >>> the affected platforms). >>> >>> Can you please reply with the exact issue you faced with genpd framework >>> initialization so we do have that on record. >>> >> >> The exact issue manifested itself in a NULL-pointer dereference panic >> when I tried moving the genpd code I had initially implemented as a >> separate platform driver to what I believe was v6 or v7 of David's >> series (before the psc driver became a platform driver, when it was >> still using CLK_OF_DECLARE()). When I had tested a simple conversion >> of that version to a platform_driver, genpd worked fine. >> >> I don't have the stack traces from these panics, but I recall some >> debugfs functions being involved and the genpd late_initcalls are >> related to debugfs. Looking at it now I don't see how exactly it could >> fail though. > > Do you have the code where you faced the problem stashed somewhere? I am > not (yet) advocating going back to CLK_OF_DECLARE(). But there is a > definite issue with timer being ready when not using CLK_OF_DECLARE(). > So, I want to make sure there the reason why we are going down the > platform device path is a amply clear. > > Thanks, > Sekhar Yes, you can still find it on my github[1]. Bart [1] github.com:brgl/linux.git topic/davinci-genpd-final-v2
diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile index fb14c8c..aef0390 100644 --- a/drivers/clk/davinci/Makefile +++ b/drivers/clk/davinci/Makefile @@ -11,4 +11,5 @@ obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o obj-y += psc.o obj-$(CONFIG_ARCH_DAVINCI_DA830) += psc-da830.o +obj-$(CONFIG_ARCH_DAVINCI_DA850) += psc-da850.o endif diff --git a/drivers/clk/davinci/psc-da850.c b/drivers/clk/davinci/psc-da850.c new file mode 100644 index 0000000..3b4583d --- /dev/null +++ b/drivers/clk/davinci/psc-da850.c @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PSC clock descriptions for TI DA850/OMAP-L138/AM18XX + * + * Copyright (C) 2017 David Lechner <david@lechnology.com> + */ + +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/init.h> +#include <linux/of.h> +#include <linux/types.h> + +#include "psc.h" + +static const struct davinci_psc_clk_info da850_psc0_info[] __initconst = { + LPSC(0, 0, tpcc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), + LPSC(1, 0, tptc0, pll0_sysclk2, LPSC_ALWAYS_ENABLED), + LPSC(2, 0, tptc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), + LPSC(3, 0, aemif, pll0_sysclk3, 0), + LPSC(4, 0, spi0, pll0_sysclk2, 0), + LPSC(5, 0, mmcsd0, pll0_sysclk2, 0), + LPSC(6, 0, aintc, pll0_sysclk4, LPSC_ALWAYS_ENABLED), + LPSC(7, 0, arm_rom, pll0_sysclk2, LPSC_ALWAYS_ENABLED), + LPSC(9, 0, uart0, pll0_sysclk2, 0), + LPSC(13, 0, pruss, pll0_sysclk2, 0), + LPSC(14, 0, arm, pll0_sysclk6, LPSC_ALWAYS_ENABLED), + LPSC(15, 1, dsp, pll0_sysclk1, LPSC_FORCE | LPSC_LOCAL_RESET), + { } +}; + +static const struct davinci_psc_clk_info da850_psc1_info[] __initconst = { + LPSC(0, 0, tpcc1, pll0_sysclk2, LPSC_ALWAYS_ENABLED), + LPSC(1, 0, usb0, pll0_sysclk2, 0), + LPSC(2, 0, usb1, pll0_sysclk4, 0), + LPSC(3, 0, gpio, pll0_sysclk4, 0), + LPSC(5, 0, emac, pll0_sysclk4, 0), + LPSC(6, 0, emif3, pll0_sysclk5, LPSC_ALWAYS_ENABLED), + LPSC(7, 0, mcasp0, async3, 0), + LPSC(8, 0, sata, pll0_sysclk2, LPSC_FORCE), + LPSC(9, 0, vpif, pll0_sysclk2, 0), + LPSC(10, 0, spi1, async3, 0), + LPSC(11, 0, i2c1, pll0_sysclk4, 0), + LPSC(12, 0, uart1, async3, 0), + LPSC(13, 0, uart2, async3, 0), + LPSC(14, 0, mcbsp0, async3, 0), + LPSC(15, 0, mcbsp1, async3, 0), + LPSC(16, 0, lcdc, pll0_sysclk2, 0), + LPSC(17, 0, ehrpwm, async3, 0), + LPSC(18, 0, mmcsd1, pll0_sysclk2, 0), + LPSC(20, 0, ecap, async3, 0), + LPSC(21, 0, tptc2, pll0_sysclk2, LPSC_ALWAYS_ENABLED), + { } +}; + +void __init da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1) +{ + struct clk_onecell_data *clk_data; + + clk_data = davinci_psc_register_clocks(psc0, da850_psc0_info, 16); + if (!clk_data) + return; + + clk_register_clkdev(clk_data->clks[3], NULL, "ti-aemif"); + clk_register_clkdev(clk_data->clks[3], "aemif", "davinci-nand.0"); + clk_register_clkdev(clk_data->clks[4], NULL, "spi_davinci.0"); + clk_register_clkdev(clk_data->clks[5], NULL, "da830-mmc.0"); + clk_register_clkdev(clk_data->clks[9], NULL, "serial8250.0"); + clk_register_clkdev(clk_data->clks[14], "arm", NULL); + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-rproc.0"); + + clk_free_onecell_data(clk_data); + + clk_data = davinci_psc_register_clocks(psc1, da850_psc1_info, 32); + if (!clk_data) + return; + + clk_register_clkdev(clk_data->clks[1], "usb20_psc_clk", NULL); + clk_register_clkdev(clk_data->clks[1], NULL, "musb-da8xx"); + clk_register_clkdev(clk_data->clks[1], NULL, "cppi41-dmaengine"); + clk_register_clkdev(clk_data->clks[2], NULL, "ohci-da8xx"); + clk_register_clkdev(clk_data->clks[3], "gpio", NULL); + clk_register_clkdev(clk_data->clks[5], NULL, "davinci_emac.1"); + clk_register_clkdev(clk_data->clks[5], "fck", "davinci_mdio.0"); + clk_register_clkdev(clk_data->clks[7], NULL, "davinci-mcasp.0"); + clk_register_clkdev(clk_data->clks[8], "fck", "ahci_da850"); + clk_register_clkdev(clk_data->clks[9], NULL, "vpif"); + clk_register_clkdev(clk_data->clks[10], NULL, "spi_davinci.1"); + clk_register_clkdev(clk_data->clks[11], NULL, "i2c_davinci.2"); + clk_register_clkdev(clk_data->clks[12], NULL, "serial8250.1"); + clk_register_clkdev(clk_data->clks[13], NULL, "serial8250.2"); + clk_register_clkdev(clk_data->clks[14], NULL, "davinci-mcbsp.0"); + clk_register_clkdev(clk_data->clks[15], NULL, "davinci-mcbsp.1"); + clk_register_clkdev(clk_data->clks[16], "fck", "da8xx_lcdc.0"); + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.0"); + clk_register_clkdev(clk_data->clks[17], "fck", "ehrpwm.1"); + clk_register_clkdev(clk_data->clks[18], NULL, "da830-mmc.1"); + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.0"); + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.1"); + clk_register_clkdev(clk_data->clks[20], "fck", "ecap.2"); + + clk_free_onecell_data(clk_data); +} + +#ifdef CONFIG_OF +static void __init of_da850_psc0_clk_init(struct device_node *node) +{ + of_davinci_psc_clk_init(node, da850_psc0_info, 16); +} +CLK_OF_DECLARE(da850_psc0_clk, "ti,da850-psc0", of_da850_psc0_clk_init); + +static void __init of_da850_psc1_clk_init(struct device_node *node) +{ + of_davinci_psc_clk_init(node, da850_psc1_info, 32); +} +CLK_OF_DECLARE(da850_psc1_clk, "ti,da850-psc1", of_da850_psc1_clk_init); +#endif diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h index 3ec8100..3d8bdfa 100644 --- a/include/linux/clk/davinci.h +++ b/include/linux/clk/davinci.h @@ -17,5 +17,6 @@ void dm644x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); void dm646x_pll_clk_init(void __iomem *pll1, void __iomem *pll2); void da830_psc_clk_init(void __iomem *psc0, void __iomem *psc1); +void da850_psc_clk_init(void __iomem *psc0, void __iomem *psc1); #endif /* __LINUX_CLK_DAVINCI_H__ */
This adds platform-specific declarations for the PSC clocks on TI DA850/ OMAP-L138/AM18XX SoCs. Signed-off-by: David Lechner <david@lechnology.com> --- drivers/clk/davinci/Makefile | 1 + drivers/clk/davinci/psc-da850.c | 117 ++++++++++++++++++++++++++++++++++++++++ include/linux/clk/davinci.h | 1 + 3 files changed, 119 insertions(+) create mode 100644 drivers/clk/davinci/psc-da850.c