| Message ID | 1516468460-4908-1-git-send-email-david@lechnology.com |
|---|---|
| Headers | show |
| Series | ARM: davinci: convert to common clock framework | expand |
On Sat, Jan 20, 2018 at 11:13 AM, David Lechner <david@lechnology.com> wrote: > This series converts mach-davinci to use the common clock framework. > > The series works like this, the first 19 patches create new clock drivers > using the common clock framework. There are basically 3 groups of clocks - > PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each have > unique init data, which is the reason for so many patches. > > Then, starting with "ARM: da830: add new clock init using common clock", > we get the mach code ready for the switch by adding the code needed for > the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the > legacy clocks so that we can switch easily between the old and the new. > > "ARM: davinci: switch to common clock framework" actually flips the switch > to start using the new clock drivers. Then the next 8 patches remove all > of the old clock code. > > The final three patches add device tree clock support to the one SoC that > supports it. > > v6 changes (also see individual patches for details): > - All of the device tree bindings are changed > - All of the clock drivers are changed significantly > - Fixed issues brought up during review of v5 > - "ARM: davinci: move davinci_clk_init() to init_time" is removed from this > series and submitted separately > > v5 changes: > - Basically, this is an entirely new series > - Patches are broken up into bite-sized pieces > - Converted PSC clock driver to use regmap > - Restored "force" flag for certain DA850 clocks > - Added device tree bindings > - Moved more of the clock init to drivers/clk > - Fixed frequency scaling (maybe*) > > * I have frequency scaling using cpufreq-dt, so I know the clocks are doing > what they need to do to make this work, but I haven't figured out how to > test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will be > sent separately after this series has landed.) > > Dependencies: > > This series applies on top of linux-davinci/master plus the following patches: > - [1] "clk: fix reentrancy of clk_enable() on UP systems" (in clk-next) > - [2] "clk: add helper functions for managing clk_onecell_data" > - [3] "clk: divider: read-only divider can propagate rate change" > - [4],[5],[6],[7],[8],[9] series "ARM: davinci: common clock prep work" > > You can find a working branch with everything included in the "common-clk-v6" > branch of https://github.com/dlech/ev3dev-kernel.git. > > [1]: https://patchwork.kernel.org/patch/10145933/ > [2]: https://patchwork.kernel.org/patch/10145873/ > [3]: https://patchwork.kernel.org/patch/10146829/ > [4]: https://patchwork.kernel.org/patch/10176241/ > [5]: https://patchwork.kernel.org/patch/10176249/ > [6]: https://patchwork.kernel.org/patch/10176245/ > [7]: https://patchwork.kernel.org/patch/10176251/ > [8]: https://patchwork.kernel.org/patch/10176243/ > [9]: https://patchwork.kernel.org/patch/10176247/ > > > Testing/debugging for the uninitiated: > > I only have one device to test with, which is based on da850, so I will > have to rely on others to do some testing here. Since we are dealing with > clocks, if something isn't working, you most likely won't see output on > the serial port. To figure out what is going on, you need to enable... > > CONFIG_DEBUG_LL=y > CONFIG_EARLY_PRINTK=y > > and add "earlyprintk clk_ignore_unused" to the kernel command line options. > You may need to select a different UART for this depending on your board. I > think UART1 is the default in the kernel configuration. > > On da850 devices comment out the lines: > > else > clk_set_parent(clk, parent->clk); > > in da850.c or, if using device tree, comment out the lines: > > assigned-clocks = <&async3_clk>; > assigned-clock-parents = <&pll1_sysclk 2>; > > in da850.dtsi when doing earlyprintk, otherwise the UART1 and UART2 clock > source will change during boot and cause garbled output after a point. > > > David Lechner (41): > dt-bindings: clock: Add new bindings for TI Davinci PLL clocks > clk: davinci: New driver for davinci PLL clocks > clk: davinci: Add platform information for TI DA830 PLL > clk: davinci: Add platform information for TI DA850 PLL > clk: davinci: Add platform information for TI DM355 PLL > clk: davinci: Add platform information for TI DM365 PLL > clk: davinci: Add platform information for TI DM644x PLL > clk: davinci: Add platform information for TI DM646x PLL > dt-bindings: clock: New bindings for TI Davinci PSC > clk: davinci: New driver for davinci PSC clocks > clk: davinci: Add platform information for TI DA830 PSC > clk: davinci: Add platform information for TI DA850 PSC > clk: davinci: Add platform information for TI DM355 PSC > clk: davinci: Add platform information for TI DM365 PSC > clk: davinci: Add platform information for TI DM644x PSC > clk: davinci: Add platform information for TI DM646x PSC > dt-bindings: clock: Add bindings for DA8XX CFGCHIP clocks > clk: davinci: New driver for TI DA8XX CFGCHIP clocks > clk: davinci: New driver for TI DA8XX USB PHY clocks > ARM: da830: add new clock init using common clock framework > ARM: da850: add new clock init using common clock framework > ARM: dm355: add new clock init using common clock framework > ARM: dm365: add new clock init using common clock framework > ARM: dm644x: add new clock init using common clock framework > ARM: dm646x: add new clock init using common clock framework > ARM: da8xx: add new USB PHY clock init using common clock framework > ARM: da8xx: add new sata_refclk init using common clock framework > ARM: davinci: remove CONFIG_DAVINCI_RESET_CLOCKS > ARM: davinci_all_defconfig: remove CONFIG_DAVINCI_RESET_CLOCKS > ARM: davinci: switch to common clock framework > ARM: da830: Remove legacy clock init > ARM: da850: Remove legacy clock init > ARM: dm355: Remove legacy clock init > ARM: dm365: Remove legacy clock init > ARM: dm644x: Remove legacy clock init > ARM: dm646x: Remove legacy clock init > ARM: da8xx: Remove legacy clock init > ARM: davinci: remove legacy clocks > ARM: davinci: add device tree support to timer > ARM: da8xx-dt: switch to device tree clocks > ARM: dts: da850: Add clocks > > .../bindings/clock/ti/davinci/da8xx-cfgchip.txt | 93 +++ > .../devicetree/bindings/clock/ti/davinci/pll.txt | 96 +++ > .../devicetree/bindings/clock/ti/davinci/psc.txt | 66 ++ > MAINTAINERS | 7 + > arch/arm/Kconfig | 2 +- > arch/arm/boot/dts/da850.dtsi | 162 ++++ > arch/arm/configs/davinci_all_defconfig | 1 - > arch/arm/mach-davinci/Kconfig | 13 +- > arch/arm/mach-davinci/Makefile | 4 +- > arch/arm/mach-davinci/clock.c | 745 ------------------- > arch/arm/mach-davinci/clock.h | 76 -- > arch/arm/mach-davinci/common.c | 3 - > arch/arm/mach-davinci/da830.c | 440 +---------- > arch/arm/mach-davinci/da850.c | 685 ++--------------- > arch/arm/mach-davinci/da8xx-dt.c | 61 +- > arch/arm/mach-davinci/davinci.h | 4 + > arch/arm/mach-davinci/devices-da8xx.c | 43 +- > arch/arm/mach-davinci/devices.c | 1 - > arch/arm/mach-davinci/dm355.c | 386 +--------- > arch/arm/mach-davinci/dm365.c | 472 +----------- > arch/arm/mach-davinci/dm644x.c | 318 +------- > arch/arm/mach-davinci/dm646x.c | 353 +-------- > arch/arm/mach-davinci/include/mach/clock.h | 3 - > arch/arm/mach-davinci/include/mach/common.h | 8 - > arch/arm/mach-davinci/psc.c | 137 ---- > arch/arm/mach-davinci/psc.h | 12 - > arch/arm/mach-davinci/time.c | 19 +- > arch/arm/mach-davinci/usb-da8xx.c | 256 ++----- > drivers/clk/Makefile | 1 + > drivers/clk/davinci/Makefile | 22 + > drivers/clk/davinci/da8xx-cfgchip.c | 305 ++++++++ > drivers/clk/davinci/da8xx-usb-phy-clk.c | 312 ++++++++ > drivers/clk/davinci/pll-da830.c | 51 ++ > drivers/clk/davinci/pll-da850.c | 163 +++++ > drivers/clk/davinci/pll-dm355.c | 66 ++ > drivers/clk/davinci/pll-dm365.c | 110 +++ > drivers/clk/davinci/pll-dm644x.c | 67 ++ > drivers/clk/davinci/pll-dm646x.c | 63 ++ > drivers/clk/davinci/pll.c | 813 +++++++++++++++++++++ > drivers/clk/davinci/pll.h | 118 +++ > drivers/clk/davinci/psc-da830.c | 85 +++ > drivers/clk/davinci/psc-da850.c | 109 +++ > drivers/clk/davinci/psc-dm355.c | 74 ++ > drivers/clk/davinci/psc-dm365.c | 79 ++ > drivers/clk/davinci/psc-dm644x.c | 68 ++ > drivers/clk/davinci/psc-dm646x.c | 62 ++ > drivers/clk/davinci/psc.c | 298 ++++++++ > drivers/clk/davinci/psc.h | 88 +++ > include/linux/clk/davinci.h | 37 + > 49 files changed, 3683 insertions(+), 3774 deletions(-) > create mode 100644 Documentation/devicetree/bindings/clock/ti/davinci/da8xx-cfgchip.txt > create mode 100644 Documentation/devicetree/bindings/clock/ti/davinci/pll.txt > create mode 100644 Documentation/devicetree/bindings/clock/ti/davinci/psc.txt > delete mode 100644 arch/arm/mach-davinci/clock.c > delete mode 100644 arch/arm/mach-davinci/psc.c > create mode 100644 drivers/clk/davinci/Makefile > create mode 100644 drivers/clk/davinci/da8xx-cfgchip.c > create mode 100644 drivers/clk/davinci/da8xx-usb-phy-clk.c > create mode 100644 drivers/clk/davinci/pll-da830.c > create mode 100644 drivers/clk/davinci/pll-da850.c > create mode 100644 drivers/clk/davinci/pll-dm355.c > create mode 100644 drivers/clk/davinci/pll-dm365.c > create mode 100644 drivers/clk/davinci/pll-dm644x.c > create mode 100644 drivers/clk/davinci/pll-dm646x.c > create mode 100644 drivers/clk/davinci/pll.c > create mode 100644 drivers/clk/davinci/pll.h > create mode 100644 drivers/clk/davinci/psc-da830.c > create mode 100644 drivers/clk/davinci/psc-da850.c > create mode 100644 drivers/clk/davinci/psc-dm355.c > create mode 100644 drivers/clk/davinci/psc-dm365.c > create mode 100644 drivers/clk/davinci/psc-dm644x.c > create mode 100644 drivers/clk/davinci/psc-dm646x.c > create mode 100644 drivers/clk/davinci/psc.c > create mode 100644 drivers/clk/davinci/psc.h > create mode 100644 include/linux/clk/davinci.h > I tested this tested on DA850-evm in both Device Tree mode and using the board file. The reboot is broken without the watchdog module, but the watchdog patch is in [PATCH] ARM: davinci_all_defconfig: set CONFIG_DAVINCI_WATCHDOG=y Go ahead and mark me down as tested if you want. Tested-by: Adam Ford <aford173@gmail.com> > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-20 18:13 GMT+01:00 David Lechner <david@lechnology.com>: > This series converts mach-davinci to use the common clock framework. > > The series works like this, the first 19 patches create new clock drivers > using the common clock framework. There are basically 3 groups of clocks - > PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each have > unique init data, which is the reason for so many patches. > > Then, starting with "ARM: da830: add new clock init using common clock", > we get the mach code ready for the switch by adding the code needed for > the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the > legacy clocks so that we can switch easily between the old and the new. > > "ARM: davinci: switch to common clock framework" actually flips the switch > to start using the new clock drivers. Then the next 8 patches remove all > of the old clock code. > > The final three patches add device tree clock support to the one SoC that > supports it. > > v6 changes (also see individual patches for details): > - All of the device tree bindings are changed > - All of the clock drivers are changed significantly > - Fixed issues brought up during review of v5 > - "ARM: davinci: move davinci_clk_init() to init_time" is removed from this > series and submitted separately > > v5 changes: > - Basically, this is an entirely new series > - Patches are broken up into bite-sized pieces > - Converted PSC clock driver to use regmap > - Restored "force" flag for certain DA850 clocks > - Added device tree bindings > - Moved more of the clock init to drivers/clk > - Fixed frequency scaling (maybe*) > > * I have frequency scaling using cpufreq-dt, so I know the clocks are doing > what they need to do to make this work, but I haven't figured out how to > test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will be > sent separately after this series has landed.) > > Dependencies: > > This series applies on top of linux-davinci/master plus the following patches: > - [1] "clk: fix reentrancy of clk_enable() on UP systems" (in clk-next) > - [2] "clk: add helper functions for managing clk_onecell_data" > - [3] "clk: divider: read-only divider can propagate rate change" > - [4],[5],[6],[7],[8],[9] series "ARM: davinci: common clock prep work" > Hi David, I'm getting a splat[1] when trying to mount the rootfs over nfs on a da850-lcdk. I'll try to figure out what's happening. Best regards, Bartosz Golaszewski [1] https://pastebin.com/D94z8SAe -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-20 18:13 GMT+01:00 David Lechner <david@lechnology.com>: > This series converts mach-davinci to use the common clock framework. > > The series works like this, the first 19 patches create new clock drivers > using the common clock framework. There are basically 3 groups of clocks - > PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each have > unique init data, which is the reason for so many patches. > > Then, starting with "ARM: da830: add new clock init using common clock", > we get the mach code ready for the switch by adding the code needed for > the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the > legacy clocks so that we can switch easily between the old and the new. > > "ARM: davinci: switch to common clock framework" actually flips the switch > to start using the new clock drivers. Then the next 8 patches remove all > of the old clock code. > > The final three patches add device tree clock support to the one SoC that > supports it. > > v6 changes (also see individual patches for details): > - All of the device tree bindings are changed > - All of the clock drivers are changed significantly > - Fixed issues brought up during review of v5 > - "ARM: davinci: move davinci_clk_init() to init_time" is removed from this > series and submitted separately > > v5 changes: > - Basically, this is an entirely new series > - Patches are broken up into bite-sized pieces > - Converted PSC clock driver to use regmap > - Restored "force" flag for certain DA850 clocks > - Added device tree bindings > - Moved more of the clock init to drivers/clk > - Fixed frequency scaling (maybe*) > > * I have frequency scaling using cpufreq-dt, so I know the clocks are doing > what they need to do to make this work, but I haven't figured out how to > test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will be > sent separately after this series has landed.) > This driver doesn't have DT support - I suppose it would be useful to add it and also add the corresponding DT node to da850.dtsi. If you're ok with it, I can start working on it. I also have a patch for the i2c cpufreq issue and it would be nice to test it with it. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/22/2018 07:29 AM, Bartosz Golaszewski wrote: > 2018-01-20 18:13 GMT+01:00 David Lechner <david@lechnology.com>: >> This series converts mach-davinci to use the common clock framework. >> >> The series works like this, the first 19 patches create new clock drivers >> using the common clock framework. There are basically 3 groups of clocks - >> PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each have >> unique init data, which is the reason for so many patches. >> >> Then, starting with "ARM: da830: add new clock init using common clock", >> we get the mach code ready for the switch by adding the code needed for >> the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the >> legacy clocks so that we can switch easily between the old and the new. >> >> "ARM: davinci: switch to common clock framework" actually flips the switch >> to start using the new clock drivers. Then the next 8 patches remove all >> of the old clock code. >> >> The final three patches add device tree clock support to the one SoC that >> supports it. >> >> v6 changes (also see individual patches for details): >> - All of the device tree bindings are changed >> - All of the clock drivers are changed significantly >> - Fixed issues brought up during review of v5 >> - "ARM: davinci: move davinci_clk_init() to init_time" is removed from this >> series and submitted separately >> >> v5 changes: >> - Basically, this is an entirely new series >> - Patches are broken up into bite-sized pieces >> - Converted PSC clock driver to use regmap >> - Restored "force" flag for certain DA850 clocks >> - Added device tree bindings >> - Moved more of the clock init to drivers/clk >> - Fixed frequency scaling (maybe*) >> >> * I have frequency scaling using cpufreq-dt, so I know the clocks are doing >> what they need to do to make this work, but I haven't figured out how to >> test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will be >> sent separately after this series has landed.) >> > > This driver doesn't have DT support - I suppose it would be useful to > add it and also add the corresponding DT node to da850.dtsi. If you're > ok with it, I can start working on it. I also have a patch for the i2c > cpufreq issue and it would be nice to test it with it. > Why not just use cpufreq-dt for the DT case? It seems to be working. I have preliminary patches on GitHub [1][2]. [1]: https://github.com/dlech/ev3dev-kernel/commit/e38897148a949b736ab135983887f9c81375f108 [2]: https://github.com/dlech/ev3dev-kernel/commit/bab85f9e45450a36a7ea6f4407f89a75edb63761 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/20/2018 11:14 AM, David Lechner wrote: > This adds clock provider nodes for da850 and wires them up to all of the > devices. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > > v6 changes: > - updated for device tree bindings changes earlier in this series > - use single async2 clock instead of duplicate fixed factor clocks > - add clock-names property to mdio node > > arch/arm/boot/dts/da850.dtsi | 162 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 162 insertions(+) > > diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi ... > pmx_core: pinmux@14120 { > compatible = "pinctrl-single"; > reg = <0x14120 0x50>; > @@ -264,8 +325,43 @@ > usb_phy: usb-phy { > compatible = "ti,da830-usb-phy"; > #phy-cells = <1>; > + clocks = <&usb_phy_clk 0>, <&usb_phy_clk 1>; > + clock-names = "usb20_phy", "usb11_phy"; This should be: clock-names = "usb0_clk48", "usb1_clk48"; > status = "disabled"; > }; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/20/2018 11:13 AM, David Lechner wrote: > This adds the new board-specific clock init in mach-davinci/da830.c > using the new common clock framework drivers. > > The #ifdefs are needed to prevent compile errors until the entire > ARCH_DAVINCI is converted. > > Also clean up the #includes since we are adding some here. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > > v6 changes: > - add blank lines between function calls > - include da8xx_register_cfgchip() > > arch/arm/mach-davinci/da830.c | 49 +++++++++++++++++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c ... > + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1); Should be "pll0_auxclk" instead of "pll0_aux_clk" here and 2 more below. > + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); > + > + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, "timer0", NULL); > + > + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, NULL, "davinci-wdt"); > + > + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); > + clk_register_clkdev(clk, "rmii", NULL); > +#else > da8xx_register_cfgchip(); > davinci_clk_init(da830_clks); > +#endif > davinci_timer_init(); > } > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/20/2018 11:14 AM, David Lechner wrote: > This adds the new USB PHY clock init in mach-davinci/usb-da8xx.c using > the new common clock framework drivers. > > The #ifdefs are needed to prevent compile errors until the entire > ARCH_DAVINCI is converted. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > > v6 changes: > - rename stuff to match changes in "clk: davinci: New driver for TI DA8XX USB > PHY clocks" > - take advantage of syscon lookup changes in "mfd: syscon: Add syscon_register() > function" > > arch/arm/mach-davinci/usb-da8xx.c | 78 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 77 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c ... > +/** > + * da8xx_register_usb20_phy_clk - register USB0PHYCLKMUX clock > + * > + * @use_usb_refclkin: Selects the parent clock - either "usb_refclkin" if true > + * or "pll0_aux_clk" if false. > + */ Should say "pll0_auxclk" instead of "pll0_aux_clk". -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/22/2018 05:14 AM, Bartosz Golaszewski wrote: > 2018-01-20 18:13 GMT+01:00 David Lechner <david@lechnology.com>: >> This series converts mach-davinci to use the common clock framework. >> >> The series works like this, the first 19 patches create new clock drivers >> using the common clock framework. There are basically 3 groups of clocks - >> PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each have >> unique init data, which is the reason for so many patches. >> >> Then, starting with "ARM: da830: add new clock init using common clock", >> we get the mach code ready for the switch by adding the code needed for >> the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the >> legacy clocks so that we can switch easily between the old and the new. >> >> "ARM: davinci: switch to common clock framework" actually flips the switch >> to start using the new clock drivers. Then the next 8 patches remove all >> of the old clock code. >> >> The final three patches add device tree clock support to the one SoC that >> supports it. >> >> v6 changes (also see individual patches for details): >> - All of the device tree bindings are changed >> - All of the clock drivers are changed significantly >> - Fixed issues brought up during review of v5 >> - "ARM: davinci: move davinci_clk_init() to init_time" is removed from this >> series and submitted separately >> >> v5 changes: >> - Basically, this is an entirely new series >> - Patches are broken up into bite-sized pieces >> - Converted PSC clock driver to use regmap >> - Restored "force" flag for certain DA850 clocks >> - Added device tree bindings >> - Moved more of the clock init to drivers/clk >> - Fixed frequency scaling (maybe*) >> >> * I have frequency scaling using cpufreq-dt, so I know the clocks are doing >> what they need to do to make this work, but I haven't figured out how to >> test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will be >> sent separately after this series has landed.) >> >> Dependencies: >> >> This series applies on top of linux-davinci/master plus the following patches: >> - [1] "clk: fix reentrancy of clk_enable() on UP systems" (in clk-next) >> - [2] "clk: add helper functions for managing clk_onecell_data" >> - [3] "clk: divider: read-only divider can propagate rate change" >> - [4],[5],[6],[7],[8],[9] series "ARM: davinci: common clock prep work" >> > > Hi David, > > I'm getting a splat[1] when trying to mount the rootfs over nfs on a da850-lcdk. > > I'll try to figure out what's happening. > > Best regards, > Bartosz Golaszewski > > [1] https://pastebin.com/D94z8SAe > Could it have something to do with the "rmii" clock? I don't think I added that in the device tree. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-22 18:30 GMT+01:00 David Lechner <david@lechnology.com>: > On 01/22/2018 05:14 AM, Bartosz Golaszewski wrote: >> >> 2018-01-20 18:13 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>> This series converts mach-davinci to use the common clock framework. >>> >>> The series works like this, the first 19 patches create new clock drivers >>> using the common clock framework. There are basically 3 groups of clocks >>> - >>> PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each >>> have >>> unique init data, which is the reason for so many patches. >>> >>> Then, starting with "ARM: da830: add new clock init using common clock", >>> we get the mach code ready for the switch by adding the code needed for >>> the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the >>> legacy clocks so that we can switch easily between the old and the new. >>> >>> "ARM: davinci: switch to common clock framework" actually flips the >>> switch >>> to start using the new clock drivers. Then the next 8 patches remove all >>> of the old clock code. >>> >>> The final three patches add device tree clock support to the one SoC that >>> supports it. >>> >>> v6 changes (also see individual patches for details): >>> - All of the device tree bindings are changed >>> - All of the clock drivers are changed significantly >>> - Fixed issues brought up during review of v5 >>> - "ARM: davinci: move davinci_clk_init() to init_time" is removed from >>> this >>> series and submitted separately >>> >>> v5 changes: >>> - Basically, this is an entirely new series >>> - Patches are broken up into bite-sized pieces >>> - Converted PSC clock driver to use regmap >>> - Restored "force" flag for certain DA850 clocks >>> - Added device tree bindings >>> - Moved more of the clock init to drivers/clk >>> - Fixed frequency scaling (maybe*) >>> >>> * I have frequency scaling using cpufreq-dt, so I know the clocks are >>> doing >>> what they need to do to make this work, but I haven't figured out how >>> to >>> test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will >>> be >>> sent separately after this series has landed.) >>> >>> Dependencies: >>> >>> This series applies on top of linux-davinci/master plus the following >>> patches: >>> - [1] "clk: fix reentrancy of clk_enable() on UP systems" (in clk-next) >>> - [2] "clk: add helper functions for managing clk_onecell_data" >>> - [3] "clk: divider: read-only divider can propagate rate change" >>> - [4],[5],[6],[7],[8],[9] series "ARM: davinci: common clock prep work" >>> >> >> Hi David, >> >> I'm getting a splat[1] when trying to mount the rootfs over nfs on a >> da850-lcdk. >> >> I'll try to figure out what's happening. >> >> Best regards, >> Bartosz Golaszewski >> >> [1] https://pastebin.com/D94z8SAe >> > > Could it have something to do with the "rmii" clock? I don't think I added > that > in the device tree. > I'm looking at it now. There are problems with emac & mdio both in legacy and in DT mode. The davinci_mdio driver seems to not get an actual functional clock (phy is reported as down). I'm seeing that in old legacy code, mdio_clk has emac_clk as parent, while after your changes they share pll0_sysclk4 as parent, although I'm not sure if that has anything to do with it. I'll see about the rmii clock too. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-22 18:11 GMT+01:00 David Lechner <david@lechnology.com>: > On 01/22/2018 07:29 AM, Bartosz Golaszewski wrote: >> >> 2018-01-20 18:13 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>> This series converts mach-davinci to use the common clock framework. >>> >>> The series works like this, the first 19 patches create new clock drivers >>> using the common clock framework. There are basically 3 groups of clocks >>> - >>> PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each >>> have >>> unique init data, which is the reason for so many patches. >>> >>> Then, starting with "ARM: da830: add new clock init using common clock", >>> we get the mach code ready for the switch by adding the code needed for >>> the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the >>> legacy clocks so that we can switch easily between the old and the new. >>> >>> "ARM: davinci: switch to common clock framework" actually flips the >>> switch >>> to start using the new clock drivers. Then the next 8 patches remove all >>> of the old clock code. >>> >>> The final three patches add device tree clock support to the one SoC that >>> supports it. >>> >>> v6 changes (also see individual patches for details): >>> - All of the device tree bindings are changed >>> - All of the clock drivers are changed significantly >>> - Fixed issues brought up during review of v5 >>> - "ARM: davinci: move davinci_clk_init() to init_time" is removed from >>> this >>> series and submitted separately >>> >>> v5 changes: >>> - Basically, this is an entirely new series >>> - Patches are broken up into bite-sized pieces >>> - Converted PSC clock driver to use regmap >>> - Restored "force" flag for certain DA850 clocks >>> - Added device tree bindings >>> - Moved more of the clock init to drivers/clk >>> - Fixed frequency scaling (maybe*) >>> >>> * I have frequency scaling using cpufreq-dt, so I know the clocks are >>> doing >>> what they need to do to make this work, but I haven't figured out how >>> to >>> test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will >>> be >>> sent separately after this series has landed.) >>> >> >> This driver doesn't have DT support - I suppose it would be useful to >> add it and also add the corresponding DT node to da850.dtsi. If you're >> ok with it, I can start working on it. I also have a patch for the i2c >> cpufreq issue and it would be nice to test it with it. >> > > Why not just use cpufreq-dt for the DT case? It seems to be working. > > I have preliminary patches on GitHub [1][2]. > > [1]: > https://github.com/dlech/ev3dev-kernel/commit/e38897148a949b736ab135983887f9c81375f108 > [2]: > https://github.com/dlech/ev3dev-kernel/commit/bab85f9e45450a36a7ea6f4407f89a75edb63761 > No problem with that - I'll test them as soon as I'll be able to boot my lcdk. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/23/2018 08:54 AM, Bartosz Golaszewski wrote: > 2018-01-22 18:30 GMT+01:00 David Lechner <david@lechnology.com>: >> On 01/22/2018 05:14 AM, Bartosz Golaszewski wrote: >>> >>> 2018-01-20 18:13 GMT+01:00 David Lechner <david@lechnology.com>: >>>> >>>> This series converts mach-davinci to use the common clock framework. >>>> >>>> The series works like this, the first 19 patches create new clock drivers >>>> using the common clock framework. There are basically 3 groups of clocks >>>> - >>>> PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each >>>> have >>>> unique init data, which is the reason for so many patches. >>>> >>>> Then, starting with "ARM: da830: add new clock init using common clock", >>>> we get the mach code ready for the switch by adding the code needed for >>>> the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the >>>> legacy clocks so that we can switch easily between the old and the new. >>>> >>>> "ARM: davinci: switch to common clock framework" actually flips the >>>> switch >>>> to start using the new clock drivers. Then the next 8 patches remove all >>>> of the old clock code. >>>> >>>> The final three patches add device tree clock support to the one SoC that >>>> supports it. >>>> >>>> v6 changes (also see individual patches for details): >>>> - All of the device tree bindings are changed >>>> - All of the clock drivers are changed significantly >>>> - Fixed issues brought up during review of v5 >>>> - "ARM: davinci: move davinci_clk_init() to init_time" is removed from >>>> this >>>> series and submitted separately >>>> >>>> v5 changes: >>>> - Basically, this is an entirely new series >>>> - Patches are broken up into bite-sized pieces >>>> - Converted PSC clock driver to use regmap >>>> - Restored "force" flag for certain DA850 clocks >>>> - Added device tree bindings >>>> - Moved more of the clock init to drivers/clk >>>> - Fixed frequency scaling (maybe*) >>>> >>>> * I have frequency scaling using cpufreq-dt, so I know the clocks are >>>> doing >>>> what they need to do to make this work, but I haven't figured out how >>>> to >>>> test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work will >>>> be >>>> sent separately after this series has landed.) >>>> >>>> Dependencies: >>>> >>>> This series applies on top of linux-davinci/master plus the following >>>> patches: >>>> - [1] "clk: fix reentrancy of clk_enable() on UP systems" (in clk-next) >>>> - [2] "clk: add helper functions for managing clk_onecell_data" >>>> - [3] "clk: divider: read-only divider can propagate rate change" >>>> - [4],[5],[6],[7],[8],[9] series "ARM: davinci: common clock prep work" >>>> >>> >>> Hi David, >>> >>> I'm getting a splat[1] when trying to mount the rootfs over nfs on a >>> da850-lcdk. >>> >>> I'll try to figure out what's happening. >>> >>> Best regards, >>> Bartosz Golaszewski >>> >>> [1] https://pastebin.com/D94z8SAe >>> >> >> Could it have something to do with the "rmii" clock? I don't think I added >> that >> in the device tree. >> > > I'm looking at it now. There are problems with emac & mdio both in > legacy and in DT mode. Adam, is networking working for you on the DA850 EVM? > > The davinci_mdio driver seems to not get an actual functional clock > (phy is reported as down). I'm seeing that in old legacy code, > mdio_clk has emac_clk as parent, while after your changes they share > pll0_sysclk4 as parent, although I'm not sure if that has anything to > do with it. The two clocks prior to these changes was a hack to work around a limitation of the old lock code (because of the way it used a linked list each clock could only be used once) so, yes, that should not have anything to do with it. I see that there is no clk_prepare_enable() in the mdio driver or the emac driver. I suppose it expects pm_runtime to take care of this maybe? You can see if the clock is enabled by running: cat /sys/kernel/debug/clk/clk_summary > > I'll see about the rmii clock too. FWIW, the rmii clock is in the clock init for non-DT da850. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/23/2018 10:03 AM, David Lechner wrote: > You can see if the clock is enabled by running: > > cat /sys/kernel/debug/clk/clk_summary > I just realized if you can't boot, you can't do this. :-/ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 23, 2018 at 10:06 AM, David Lechner <david@lechnology.com> wrote: > On 01/23/2018 10:03 AM, David Lechner wrote: >> >> You can see if the clock is enabled by running: >> >> cat /sys/kernel/debug/clk/clk_summary >> > > I just realized if you can't boot, you can't do this. :-/ I can boot with the latest set in your git repo, but the Ethernet doesn't apparently fully operate. I don't get errors, but I cannot get a dhcp address. I'll try to do a more exhaustive test later today to get an idea of what works and what doesn't. When I ran my basic tests, I just did a feel-good boot test (and reboot test) adam -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-23 17:03 GMT+01:00 David Lechner <david@lechnology.com>: > On 01/23/2018 08:54 AM, Bartosz Golaszewski wrote: >> >> 2018-01-22 18:30 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>> On 01/22/2018 05:14 AM, Bartosz Golaszewski wrote: >>>> >>>> >>>> 2018-01-20 18:13 GMT+01:00 David Lechner <david@lechnology.com>: >>>>> >>>>> >>>>> This series converts mach-davinci to use the common clock framework. >>>>> >>>>> The series works like this, the first 19 patches create new clock >>>>> drivers >>>>> using the common clock framework. There are basically 3 groups of >>>>> clocks >>>>> - >>>>> PLL, PSC and CFGCHIP (syscon). There are six different SoCs that each >>>>> have >>>>> unique init data, which is the reason for so many patches. >>>>> >>>>> Then, starting with "ARM: da830: add new clock init using common >>>>> clock", >>>>> we get the mach code ready for the switch by adding the code needed for >>>>> the new clock drivers and adding #ifndef CONFIG_COMMON_CLK around the >>>>> legacy clocks so that we can switch easily between the old and the new. >>>>> >>>>> "ARM: davinci: switch to common clock framework" actually flips the >>>>> switch >>>>> to start using the new clock drivers. Then the next 8 patches remove >>>>> all >>>>> of the old clock code. >>>>> >>>>> The final three patches add device tree clock support to the one SoC >>>>> that >>>>> supports it. >>>>> >>>>> v6 changes (also see individual patches for details): >>>>> - All of the device tree bindings are changed >>>>> - All of the clock drivers are changed significantly >>>>> - Fixed issues brought up during review of v5 >>>>> - "ARM: davinci: move davinci_clk_init() to init_time" is removed from >>>>> this >>>>> series and submitted separately >>>>> >>>>> v5 changes: >>>>> - Basically, this is an entirely new series >>>>> - Patches are broken up into bite-sized pieces >>>>> - Converted PSC clock driver to use regmap >>>>> - Restored "force" flag for certain DA850 clocks >>>>> - Added device tree bindings >>>>> - Moved more of the clock init to drivers/clk >>>>> - Fixed frequency scaling (maybe*) >>>>> >>>>> * I have frequency scaling using cpufreq-dt, so I know the clocks are >>>>> doing >>>>> what they need to do to make this work, but I haven't figured out >>>>> how >>>>> to >>>>> test davinci-cpufreq driver yet. (Patches to make cpufreq-dt work >>>>> will >>>>> be >>>>> sent separately after this series has landed.) >>>>> >>>>> Dependencies: >>>>> >>>>> This series applies on top of linux-davinci/master plus the following >>>>> patches: >>>>> - [1] "clk: fix reentrancy of clk_enable() on UP systems" (in clk-next) >>>>> - [2] "clk: add helper functions for managing clk_onecell_data" >>>>> - [3] "clk: divider: read-only divider can propagate rate change" >>>>> - [4],[5],[6],[7],[8],[9] series "ARM: davinci: common clock prep work" >>>>> >>>> >>>> Hi David, >>>> >>>> I'm getting a splat[1] when trying to mount the rootfs over nfs on a >>>> da850-lcdk. >>>> >>>> I'll try to figure out what's happening. >>>> >>>> Best regards, >>>> Bartosz Golaszewski >>>> >>>> [1] https://pastebin.com/D94z8SAe >>>> >>> >>> Could it have something to do with the "rmii" clock? I don't think I >>> added >>> that >>> in the device tree. >>> >> >> I'm looking at it now. There are problems with emac & mdio both in >> legacy and in DT mode. > > > Adam, is networking working for you on the DA850 EVM? > >> >> The davinci_mdio driver seems to not get an actual functional clock >> (phy is reported as down). I'm seeing that in old legacy code, >> mdio_clk has emac_clk as parent, while after your changes they share >> pll0_sysclk4 as parent, although I'm not sure if that has anything to >> do with it. > > > The two clocks prior to these changes was a hack to work around a > limitation of the old lock code (because of the way it used a linked list > each clock could only be used once) so, yes, that should not have anything > to do with it. > And it was I who did the change in commit ef37427ac567 ("ARM: davinci: da850: don't add emac clock to lookup table twice") :) Already forgot that. > I see that there is no clk_prepare_enable() in the mdio driver or the emac > driver. I suppose it expects pm_runtime to take care of this maybe? > > You can see if the clock is enabled by running: > > cat /sys/kernel/debug/clk/clk_summary > >> >> I'll see about the rmii clock too. > > FWIW, the rmii clock is in the clock init for non-DT da850. > 2018-01-23 17:06 GMT+01:00 David Lechner <david@lechnology.com>: > On 01/23/2018 10:03 AM, David Lechner wrote: >> >> You can see if the clock is enabled by running: >> >> cat /sys/kernel/debug/clk/clk_summary >> > > I just realized if you can't boot, you can't do this. :-/ I can always boot from SD or just printk it. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-23 18:03 GMT+01:00 Adam Ford <aford173@gmail.com>: > On Tue, Jan 23, 2018 at 10:06 AM, David Lechner <david@lechnology.com> wrote: >> On 01/23/2018 10:03 AM, David Lechner wrote: >>> >>> You can see if the clock is enabled by running: >>> >>> cat /sys/kernel/debug/clk/clk_summary >>> >> >> I just realized if you can't boot, you can't do this. :-/ > > I can boot with the latest set in your git repo, but the Ethernet > doesn't apparently fully operate. I don't get errors, but I cannot > get a dhcp address. > > I'll try to do a more exhaustive test later today to get an idea of > what works and what doesn't. When I ran my basic tests, I just did a > feel-good boot test (and reboot test) > > adam FYI: manually calling clk_prepare_enable() in the davinci_mdio driver seems to at least fix the ethernet. In master branch it's done by pm_runtime_get_sync() (in davinci_mdio_reset()). However I'm still getting several oopses and WARNs so there's some more work to do. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/23/2018 12:10 PM, Bartosz Golaszewski wrote: > 2018-01-23 18:03 GMT+01:00 Adam Ford <aford173@gmail.com>: >> On Tue, Jan 23, 2018 at 10:06 AM, David Lechner <david@lechnology.com> wrote: >>> On 01/23/2018 10:03 AM, David Lechner wrote: >>>> >>>> You can see if the clock is enabled by running: >>>> >>>> cat /sys/kernel/debug/clk/clk_summary >>>> >>> >>> I just realized if you can't boot, you can't do this. :-/ >> >> I can boot with the latest set in your git repo, but the Ethernet >> doesn't apparently fully operate. I don't get errors, but I cannot >> get a dhcp address. >> >> I'll try to do a more exhaustive test later today to get an idea of >> what works and what doesn't. When I ran my basic tests, I just did a >> feel-good boot test (and reboot test) >> >> adam > > FYI: manually calling clk_prepare_enable() in the davinci_mdio driver > seems to at least fix the ethernet. In master branch it's done by > pm_runtime_get_sync() (in davinci_mdio_reset()). However I'm still > getting several oopses and WARNs so there's some more work to do. > Hmm... I'm wondering if we need to also add #power-domain-cells to the PSC clocks and power-domains properties to the consumers. For this specific case though, it seems strange to me that the drivers to clk_get() and clk_get_rate() but never enable the clocks. Also, are the oopses and WARNs the same as before? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-23 19:26 GMT+01:00 David Lechner <david@lechnology.com>: > On 01/23/2018 12:10 PM, Bartosz Golaszewski wrote: >> >> 2018-01-23 18:03 GMT+01:00 Adam Ford <aford173@gmail.com>: >>> >>> On Tue, Jan 23, 2018 at 10:06 AM, David Lechner <david@lechnology.com> >>> wrote: >>>> >>>> On 01/23/2018 10:03 AM, David Lechner wrote: >>>>> >>>>> >>>>> You can see if the clock is enabled by running: >>>>> >>>>> cat /sys/kernel/debug/clk/clk_summary >>>>> >>>> >>>> I just realized if you can't boot, you can't do this. :-/ >>> >>> >>> I can boot with the latest set in your git repo, but the Ethernet >>> doesn't apparently fully operate. I don't get errors, but I cannot >>> get a dhcp address. >>> >>> I'll try to do a more exhaustive test later today to get an idea of >>> what works and what doesn't. When I ran my basic tests, I just did a >>> feel-good boot test (and reboot test) >>> >>> adam >> >> >> FYI: manually calling clk_prepare_enable() in the davinci_mdio driver >> seems to at least fix the ethernet. In master branch it's done by >> pm_runtime_get_sync() (in davinci_mdio_reset()). However I'm still >> getting several oopses and WARNs so there's some more work to do. >> > > Hmm... I'm wondering if we need to also add #power-domain-cells to the > PSC clocks and power-domains properties to the consumers. > > For this specific case though, it seems strange to me that the drivers > to clk_get() and clk_get_rate() but never enable the clocks. > > > Also, are the oopses and WARNs the same as before? No, the ones before were all related to the ethernet failing, now I get several stack traces from drm. Posted them on pastebin[1]. Thanks, Bartosz [1] https://pastebin.com/H3yZ5xHv -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/23/2018 12:34 PM, Bartosz Golaszewski wrote: > 2018-01-23 19:26 GMT+01:00 David Lechner <david@lechnology.com>: >> On 01/23/2018 12:10 PM, Bartosz Golaszewski wrote: >>> >>> 2018-01-23 18:03 GMT+01:00 Adam Ford <aford173@gmail.com>: >>>> >>>> On Tue, Jan 23, 2018 at 10:06 AM, David Lechner <david@lechnology.com> >>>> wrote: >>>>> >>>>> On 01/23/2018 10:03 AM, David Lechner wrote: >>>>>> >>>>>> >>>>>> You can see if the clock is enabled by running: >>>>>> >>>>>> cat /sys/kernel/debug/clk/clk_summary >>>>>> >>>>> >>>>> I just realized if you can't boot, you can't do this. :-/ >>>> >>>> >>>> I can boot with the latest set in your git repo, but the Ethernet >>>> doesn't apparently fully operate. I don't get errors, but I cannot >>>> get a dhcp address. >>>> >>>> I'll try to do a more exhaustive test later today to get an idea of >>>> what works and what doesn't. When I ran my basic tests, I just did a >>>> feel-good boot test (and reboot test) >>>> >>>> adam >>> >>> >>> FYI: manually calling clk_prepare_enable() in the davinci_mdio driver >>> seems to at least fix the ethernet. In master branch it's done by >>> pm_runtime_get_sync() (in davinci_mdio_reset()). However I'm still >>> getting several oopses and WARNs so there's some more work to do. >>> >> >> Hmm... I'm wondering if we need to also add #power-domain-cells to the >> PSC clocks and power-domains properties to the consumers. >> >> For this specific case though, it seems strange to me that the drivers >> to clk_get() and clk_get_rate() but never enable the clocks. >> >> >> Also, are the oopses and WARNs the same as before? > > No, the ones before were all related to the ethernet failing, now I > get several stack traces from drm. Posted them on pastebin[1]. > It looks like the LCDC driver is the same way. It does clk_get() but not clk_prepare_enable(). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-23 20:24 GMT+01:00 David Lechner <david@lechnology.com>: > On 01/23/2018 12:34 PM, Bartosz Golaszewski wrote: >> >> 2018-01-23 19:26 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>> On 01/23/2018 12:10 PM, Bartosz Golaszewski wrote: >>>> >>>> >>>> 2018-01-23 18:03 GMT+01:00 Adam Ford <aford173@gmail.com>: >>>>> >>>>> >>>>> On Tue, Jan 23, 2018 at 10:06 AM, David Lechner <david@lechnology.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> On 01/23/2018 10:03 AM, David Lechner wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> You can see if the clock is enabled by running: >>>>>>> >>>>>>> cat /sys/kernel/debug/clk/clk_summary >>>>>>> >>>>>> >>>>>> I just realized if you can't boot, you can't do this. :-/ >>>>> >>>>> >>>>> >>>>> I can boot with the latest set in your git repo, but the Ethernet >>>>> doesn't apparently fully operate. I don't get errors, but I cannot >>>>> get a dhcp address. >>>>> >>>>> I'll try to do a more exhaustive test later today to get an idea of >>>>> what works and what doesn't. When I ran my basic tests, I just did a >>>>> feel-good boot test (and reboot test) >>>>> >>>>> adam >>>> >>>> >>>> >>>> FYI: manually calling clk_prepare_enable() in the davinci_mdio driver >>>> seems to at least fix the ethernet. In master branch it's done by >>>> pm_runtime_get_sync() (in davinci_mdio_reset()). However I'm still >>>> getting several oopses and WARNs so there's some more work to do. >>>> >>> >>> Hmm... I'm wondering if we need to also add #power-domain-cells to the >>> PSC clocks and power-domains properties to the consumers. >>> >>> For this specific case though, it seems strange to me that the drivers >>> to clk_get() and clk_get_rate() but never enable the clocks. >>> >>> >>> Also, are the oopses and WARNs the same as before? >> >> >> No, the ones before were all related to the ethernet failing, now I >> get several stack traces from drm. Posted them on pastebin[1]. >> > > It looks like the LCDC driver is the same way. It does clk_get() but > not clk_prepare_enable(). > > In the mdio case - the problem is that devm_clk_get() doesn't fail, but somehow the clock doesn't end up in the list of the device's clocks - which is why it's not enabled by pm_runtime_get_sync(). Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/23/2018 01:53 PM, Bartosz Golaszewski wrote: > 2018-01-23 20:24 GMT+01:00 David Lechner <david@lechnology.com>: >> On 01/23/2018 12:34 PM, Bartosz Golaszewski wrote: >>> >>> 2018-01-23 19:26 GMT+01:00 David Lechner <david@lechnology.com>: >>>> >>>> On 01/23/2018 12:10 PM, Bartosz Golaszewski wrote: >>>>> >>>>> >>>>> 2018-01-23 18:03 GMT+01:00 Adam Ford <aford173@gmail.com>: >>>>>> >>>>>> >>>>>> On Tue, Jan 23, 2018 at 10:06 AM, David Lechner <david@lechnology.com> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> On 01/23/2018 10:03 AM, David Lechner wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> You can see if the clock is enabled by running: >>>>>>>> >>>>>>>> cat /sys/kernel/debug/clk/clk_summary >>>>>>>> >>>>>>> >>>>>>> I just realized if you can't boot, you can't do this. :-/ >>>>>> >>>>>> >>>>>> >>>>>> I can boot with the latest set in your git repo, but the Ethernet >>>>>> doesn't apparently fully operate. I don't get errors, but I cannot >>>>>> get a dhcp address. >>>>>> >>>>>> I'll try to do a more exhaustive test later today to get an idea of >>>>>> what works and what doesn't. When I ran my basic tests, I just did a >>>>>> feel-good boot test (and reboot test) >>>>>> >>>>>> adam >>>>> >>>>> >>>>> >>>>> FYI: manually calling clk_prepare_enable() in the davinci_mdio driver >>>>> seems to at least fix the ethernet. In master branch it's done by >>>>> pm_runtime_get_sync() (in davinci_mdio_reset()). However I'm still >>>>> getting several oopses and WARNs so there's some more work to do. >>>>> >>>> >>>> Hmm... I'm wondering if we need to also add #power-domain-cells to the >>>> PSC clocks and power-domains properties to the consumers. >>>> >>>> For this specific case though, it seems strange to me that the drivers >>>> to clk_get() and clk_get_rate() but never enable the clocks. >>>> >>>> >>>> Also, are the oopses and WARNs the same as before? >>> >>> >>> No, the ones before were all related to the ethernet failing, now I >>> get several stack traces from drm. Posted them on pastebin[1]. >>> >> >> It looks like the LCDC driver is the same way. It does clk_get() but >> not clk_prepare_enable(). >> >> > > In the mdio case - the problem is that devm_clk_get() doesn't fail, > but somehow the clock doesn't end up in the list of the device's > clocks - which is why it's not enabled by pm_runtime_get_sync(). > Right. This is because devm_clk_get() now finds the clock via device tree instead of a clkdev lookup entry. However, I think that the PM notifier registered in arch/arm/mach-davinci/pm_domain.c only uses the clkdev lookup to match the con_id and does not use device tree. The same thing is happing in mdio, emac and lcdc. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/23/2018 02:01 PM, David Lechner wrote: > On 01/23/2018 01:53 PM, Bartosz Golaszewski wrote: >> >> In the mdio case - the problem is that devm_clk_get() doesn't fail, >> but somehow the clock doesn't end up in the list of the device's >> clocks - which is why it's not enabled by pm_runtime_get_sync(). >> > > > Right. This is because devm_clk_get() now finds the clock via device > tree instead of a clkdev lookup entry. However, I think that the PM > notifier registered in arch/arm/mach-davinci/pm_domain.c only uses > the clkdev lookup to match the con_id and does not use device tree. > The same thing is happing in mdio, emac and lcdc. > Minor correction: It looks like emac doesn't do this because it doesn't have a con_id of "fck". But, the same clock is shared by emac and mdio, so since mdio enables the clock, emac doesn't notice or care that it did not enable the clock itself. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/23/2018 02:05 PM, David Lechner wrote: > On 01/23/2018 02:01 PM, David Lechner wrote: >> On 01/23/2018 01:53 PM, Bartosz Golaszewski wrote: >>> >>> In the mdio case - the problem is that devm_clk_get() doesn't fail, >>> but somehow the clock doesn't end up in the list of the device's >>> clocks - which is why it's not enabled by pm_runtime_get_sync(). >>> >> >> >> Right. This is because devm_clk_get() now finds the clock via device >> tree instead of a clkdev lookup entry. However, I think that the PM >> notifier registered in arch/arm/mach-davinci/pm_domain.c only uses >> the clkdev lookup to match the con_id and does not use device tree. >> The same thing is happing in mdio, emac and lcdc. >> > > Minor correction: It looks like emac doesn't do this because it doesn't > have a con_id of "fck". But, the same clock is shared by emac and mdio, so > since mdio enables the clock, emac doesn't notice or care that it did > not enable the clock itself. How about using pm_clk_add_clk() in these drivers to explicitly use the clocks for power management instead of relying on pm_clk_add_notifier() to do this implicitly? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/20/2018 11:14 AM, David Lechner wrote: > This removes all of the clock init code from da8xx-dt.c. This includes > all of the OF_DEV_AUXDATA that was just used for looking up clocks. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > > v6 changes: > - removed misleading statement from commit message > > arch/arm/mach-davinci/da8xx-dt.c | 61 +--------------------------------------- > 1 file changed, 1 insertion(+), 60 deletions(-) > > diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c > static void __init da850_init_machine(void) > { > - /* All existing boards use 100MHz SATA refclkpn */ > - static const unsigned long sata_refclkpn = 100 * 1000 * 1000; > - > - int ret; > - > - ret = da8xx_register_usb20_phy_clk(false); > - if (ret) > - pr_warn("%s: registering USB 2.0 PHY clock failed: %d", > - __func__, ret); > - ret = da8xx_register_usb11_phy_clk(false); > - if (ret) > - pr_warn("%s: registering USB 1.1 PHY clock failed: %d", > - __func__, ret); > - > - ret = da850_register_sata_refclk(sata_refclkpn); > - if (ret) > - pr_warn("%s: registering SATA REFCLK failed: %d", > - __func__, ret); > - > - of_platform_default_populate(NULL, da850_auxdata_lookup, NULL); > + of_platform_default_populate(NULL, NULL, NULL); of_platform_default_populate() can actually be removed completely. of_platform_default_populate_init() is called implicitly during arch_initcall_sync > davinci_pm_init(); > pdata_quirks_init(); > } -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/20/2018 11:14 AM, David Lechner wrote: > This adds clock provider nodes for da850 and wires them up to all of the > devices. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > > v6 changes: > - updated for device tree bindings changes earlier in this series > - use single async2 clock instead of duplicate fixed factor clocks > - add clock-names property to mdio node > > arch/arm/boot/dts/da850.dtsi | 162 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 162 insertions(+) > > diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi ... > + psc1: clock-controller@227000 { > + compatible = "ti,da850-psc1"; > + reg = <0x227000 0x1000>; > + #clock-cells = <1>; > + clocks = <&pll0_sysclk 2>, <&pll0_sysclk 4>, > + <&async3_clk>; > + clock_names = "pll0_sysclk2", "pll0_sysclk4", "async3"; Should be clock-names, not clock_names > }; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-23 21:23 GMT+01:00 David Lechner <david@lechnology.com>: > On 01/23/2018 02:05 PM, David Lechner wrote: >> >> On 01/23/2018 02:01 PM, David Lechner wrote: >>> >>> On 01/23/2018 01:53 PM, Bartosz Golaszewski wrote: >>>> >>>> >>>> In the mdio case - the problem is that devm_clk_get() doesn't fail, >>>> but somehow the clock doesn't end up in the list of the device's >>>> clocks - which is why it's not enabled by pm_runtime_get_sync(). >>>> >>> >>> >>> Right. This is because devm_clk_get() now finds the clock via device >>> tree instead of a clkdev lookup entry. However, I think that the PM >>> notifier registered in arch/arm/mach-davinci/pm_domain.c only uses >>> the clkdev lookup to match the con_id and does not use device tree. >>> The same thing is happing in mdio, emac and lcdc. >>> >> >> Minor correction: It looks like emac doesn't do this because it doesn't >> have a con_id of "fck". But, the same clock is shared by emac and mdio, so >> since mdio enables the clock, emac doesn't notice or care that it did >> not enable the clock itself. > > > How about using pm_clk_add_clk() in these drivers to explicitly use the > clocks for power management instead of relying on pm_clk_add_notifier() > to do this implicitly? Yes, this sounds good. Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-25 13:53 GMT+01:00 Sekhar Nori <nsekhar@ti.com>: > On Wednesday 24 January 2018 01:33 PM, Bartosz Golaszewski wrote: >> 2018-01-23 21:23 GMT+01:00 David Lechner <david@lechnology.com>: >>> On 01/23/2018 02:05 PM, David Lechner wrote: >>>> >>>> On 01/23/2018 02:01 PM, David Lechner wrote: >>>>> >>>>> On 01/23/2018 01:53 PM, Bartosz Golaszewski wrote: >>>>>> >>>>>> >>>>>> In the mdio case - the problem is that devm_clk_get() doesn't fail, >>>>>> but somehow the clock doesn't end up in the list of the device's >>>>>> clocks - which is why it's not enabled by pm_runtime_get_sync(). >>>>>> >>>>> >>>>> >>>>> Right. This is because devm_clk_get() now finds the clock via device >>>>> tree instead of a clkdev lookup entry. However, I think that the PM >>>>> notifier registered in arch/arm/mach-davinci/pm_domain.c only uses >>>>> the clkdev lookup to match the con_id and does not use device tree. >>>>> The same thing is happing in mdio, emac and lcdc. >>>>> >>>> >>>> Minor correction: It looks like emac doesn't do this because it doesn't >>>> have a con_id of "fck". But, the same clock is shared by emac and mdio, so >>>> since mdio enables the clock, emac doesn't notice or care that it did >>>> not enable the clock itself. >>> >>> >>> How about using pm_clk_add_clk() in these drivers to explicitly use the >>> clocks for power management instead of relying on pm_clk_add_notifier() >>> to do this implicitly? >> >> Yes, this sounds good. > > Looking at how pm_clk_notify() in clock_ops.c uses con_id[] list, right > now pm_runtime() will work only for clocks which have con_id (from the > list above) mentioned in DT. Since clk_find() mandates con_id match when > its available, NULL con_id does not match. > > For simple devices like DaVinci which uses just one clock for power > management per device (multiple devices might share a clock, but not > other way around as far as I recall, anyway I will double check this > assertion), the attached patch should make EMAC work. > Tested on da850-lcdk - patch does indeex fix emac on da850-lcdk, but tilcdc still complains. > That still leaves why lcdc does not work. One difference is it uses > PSC1. Are there other devices in PSC1 which work (just to rule out any > thing wrong with PSC1 handling). > Emac uses PSC1 too. GPIO and I2C1 work fine, but OHCI rarely lives through any cpufreq transition. I'm still trying to figure out why it's dying. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/25/2018 06:53 AM, Sekhar Nori wrote: > On Wednesday 24 January 2018 01:33 PM, Bartosz Golaszewski wrote: >> 2018-01-23 21:23 GMT+01:00 David Lechner <david@lechnology.com>: >>> On 01/23/2018 02:05 PM, David Lechner wrote: >>>> >>>> On 01/23/2018 02:01 PM, David Lechner wrote: >>>>> >>>>> On 01/23/2018 01:53 PM, Bartosz Golaszewski wrote: >>>>>> >>>>>> >>>>>> In the mdio case - the problem is that devm_clk_get() doesn't fail, >>>>>> but somehow the clock doesn't end up in the list of the device's >>>>>> clocks - which is why it's not enabled by pm_runtime_get_sync(). >>>>>> >>>>> >>>>> >>>>> Right. This is because devm_clk_get() now finds the clock via device >>>>> tree instead of a clkdev lookup entry. However, I think that the PM >>>>> notifier registered in arch/arm/mach-davinci/pm_domain.c only uses >>>>> the clkdev lookup to match the con_id and does not use device tree. >>>>> The same thing is happing in mdio, emac and lcdc. >>>>> >>>> >>>> Minor correction: It looks like emac doesn't do this because it doesn't >>>> have a con_id of "fck". But, the same clock is shared by emac and mdio, so >>>> since mdio enables the clock, emac doesn't notice or care that it did >>>> not enable the clock itself. >>> >>> >>> How about using pm_clk_add_clk() in these drivers to explicitly use the >>> clocks for power management instead of relying on pm_clk_add_notifier() >>> to do this implicitly? >> >> Yes, this sounds good. > > Looking at how pm_clk_notify() in clock_ops.c uses con_id[] list, right > now pm_runtime() will work only for clocks which have con_id (from the > list above) mentioned in DT. Since clk_find() mandates con_id match when > its available, NULL con_id does not match. > > For simple devices like DaVinci which uses just one clock for power > management per device (multiple devices might share a clock, but not > other way around as far as I recall, anyway I will double check this > assertion), the attached patch should make EMAC work. > > That still leaves why lcdc does not work. One difference is it uses > PSC1. Are there other devices in PSC1 which work (just to rule out any > thing wrong with PSC1 handling). > > Thanks, > Sekhar > > ---8<--- > diff --git a/arch/arm/mach-davinci/pm_domain.c b/arch/arm/mach-davinci/pm_domain.c > index 78eac2c0c146..0dce7397856d 100644 > --- a/arch/arm/mach-davinci/pm_domain.c > +++ b/arch/arm/mach-davinci/pm_domain.c > @@ -23,7 +23,6 @@ static struct dev_pm_domain davinci_pm_domain = { > > static struct pm_clk_notifier_block platform_bus_notifier = { > .pm_domain = &davinci_pm_domain, > - .con_ids = { "fck", "master", "slave", NULL }, > }; > > static int __init davinci_pm_runtime_init(void) > > Won't this cause *all* clocks, not just PSC clocks, to be used for power management? Some examples of devices with more than just the PSC clock are SATA and the USB PHY. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 25 January 2018 09:48 PM, David Lechner wrote: > On 01/25/2018 06:53 AM, Sekhar Nori wrote: >> On Wednesday 24 January 2018 01:33 PM, Bartosz Golaszewski wrote: >>> 2018-01-23 21:23 GMT+01:00 David Lechner <david@lechnology.com>: >>>> On 01/23/2018 02:05 PM, David Lechner wrote: >>>>> >>>>> On 01/23/2018 02:01 PM, David Lechner wrote: >>>>>> >>>>>> On 01/23/2018 01:53 PM, Bartosz Golaszewski wrote: >>>>>>> >>>>>>> >>>>>>> In the mdio case - the problem is that devm_clk_get() doesn't fail, >>>>>>> but somehow the clock doesn't end up in the list of the device's >>>>>>> clocks - which is why it's not enabled by pm_runtime_get_sync(). >>>>>>> >>>>>> >>>>>> >>>>>> Right. This is because devm_clk_get() now finds the clock via device >>>>>> tree instead of a clkdev lookup entry. However, I think that the PM >>>>>> notifier registered in arch/arm/mach-davinci/pm_domain.c only uses >>>>>> the clkdev lookup to match the con_id and does not use device tree. >>>>>> The same thing is happing in mdio, emac and lcdc. >>>>>> >>>>> >>>>> Minor correction: It looks like emac doesn't do this because it >>>>> doesn't >>>>> have a con_id of "fck". But, the same clock is shared by emac and >>>>> mdio, so >>>>> since mdio enables the clock, emac doesn't notice or care that it did >>>>> not enable the clock itself. >>>> >>>> >>>> How about using pm_clk_add_clk() in these drivers to explicitly use the >>>> clocks for power management instead of relying on pm_clk_add_notifier() >>>> to do this implicitly? >>> >>> Yes, this sounds good. >> >> Looking at how pm_clk_notify() in clock_ops.c uses con_id[] list, right >> now pm_runtime() will work only for clocks which have con_id (from the >> list above) mentioned in DT. Since clk_find() mandates con_id match when >> its available, NULL con_id does not match. >> >> For simple devices like DaVinci which uses just one clock for power >> management per device (multiple devices might share a clock, but not >> other way around as far as I recall, anyway I will double check this >> assertion), the attached patch should make EMAC work. >> >> That still leaves why lcdc does not work. One difference is it uses >> PSC1. Are there other devices in PSC1 which work (just to rule out any >> thing wrong with PSC1 handling). >> >> Thanks, >> Sekhar >> >> ---8<--- >> diff --git a/arch/arm/mach-davinci/pm_domain.c >> b/arch/arm/mach-davinci/pm_domain.c >> index 78eac2c0c146..0dce7397856d 100644 >> --- a/arch/arm/mach-davinci/pm_domain.c >> +++ b/arch/arm/mach-davinci/pm_domain.c >> @@ -23,7 +23,6 @@ static struct dev_pm_domain davinci_pm_domain = { >> static struct pm_clk_notifier_block platform_bus_notifier = { >> .pm_domain = &davinci_pm_domain, >> - .con_ids = { "fck", "master", "slave", NULL }, >> }; >> static int __init davinci_pm_runtime_init(void) >> >> > > Won't this cause *all* clocks, not just PSC clocks, to be used for power > management? In case of DT, the first clock mentioned in 'clocks' property will get picked up since it will match with wildcard con_id. This unfortunately depends on how the clocks list is prepared so quite implementation dependent. > > Some examples of devices with more than just the PSC clock are SATA and > the USB PHY. Its not a problem to have more than one clock passed in device node as long as only one clock (the first clock) needs to participate in power management. This may work for the moment, but longer term I do think genpd implementation for DaVinci will be needed. Or may be there is breakage even today that I am not able to think on top of my head. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds a new driver for mach-davinci PLL clocks. This is porting the > code from arch/arm/mach-davinci/clock.c to the common clock framework. > Additionally, it adds device tree support for these clocks. > > The ifeq ($(CONFIG_COMMON_CLK), y) in the Makefile is needed to prevent > compile errors until the clock code in arch/arm/mach-davinci is removed. > > Note: although there are similar clocks for TI Keystone we are not able > to share the code for a few reasons. The keystone clocks are device tree > only and use legacy one-node-per-clock bindings. Also the register > layouts are a bit different, which would add even more if/else mess > to the keystone clocks. And the keystone PLL driver doesn't support > setting clock rates. > > Signed-off-by: David Lechner <david@lechnology.com> Looks nice and clean to me. There is still some feedback though. One thing missing is DIV4.5 clock. It will be nice to add that too, mostly just because it will make the binding complete. > +void of_davinci_pll_init(struct device_node *node, > + const struct davinci_pll_clk_info *info, > + const struct davinci_pll_obsclk_info *obsclk_info, > + const struct davinci_pll_sysclk_info *div_info, > + u8 max_sysclk_id) > +{ > + struct device_node *child; > + const char *parent_name; > + void __iomem *base; > + struct clk *clk; > + > + base = of_iomap(node, 0); > + if (!base) { > + pr_err("ioremap failed"); > + return; > + } > + > + if (info->flags & PLL_HAS_OSCIN) > + parent_name = of_clk_get_parent_name(node, 0); > + else > + parent_name = OSCIN_CLK_NAME; I don't think the reference clock input handling is fully correct/flexible. There are two ways to provide input clock (ref_clk) to PLL. Either use the internal oscillator with a crystal connected between OSCIN and OSCOUT (CLKMODE = 0) or a clean clock input (CLKMODE = 1) connected to OSCIN (OSCOUT disconnected). This is a board specific decision. On the LogicPD EVM, the former option is used, on the LCDK board, the later. So, I think what we need is a DT property like "ti,davinci-use-internal-osc" for the PLL. Boards can set or ignore it and you can switch CLKMODE on or off based on that. Setting CLKMODE = 1 will switch off the internal oscillator (and presumably save power), but it does not act as a mux. This explains why not worrying about setting this correctly in current mainline still works. I am also not sure if we really need PLL_HAS_OSCIN since all DaVinci SoCs set it anyway. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds platform-specific declarations for the PLL clocks on TI DA830/ > OMAP-L137/AM17XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds platform-specific declarations for the PLL clocks on TI DA850/ > OMAP-L138/AM18XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> > +static const struct davinci_pll_clk_info da850_pll1_info __initconst = { > + .name = "pll1", > + .unlock_reg = CFGCHIP(3), > + .unlock_mask = CFGCHIP3_PLL1_MASTER_LOCK, I guess this will change with the cfgchip handling discussion last week. > + .pllm_mask = GENMASK(4, 0), > + .pllm_min = 4, > + .pllm_max = 32, > + .pllout_min_rate = 300000000, > + .pllout_max_rate = 600000000, > + .flags = PLL_HAS_POSTDIV, > +}; > + [...] > +void __init da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1) > +{ > + const struct davinci_pll_sysclk_info *info; > + > + davinci_pll_clk_register(&da850_pll0_info, "ref_clk", pll0); > + > + davinci_pll_auxclk_register("pll0_auxclk", pll0); > + > + for (info = da850_pll0_sysclk_info; info->name; info++) > + davinci_pll_sysclk_register(info, pll0); > + > + davinci_pll_obsclk_register(&da850_pll0_obsclk_info, pll0); > + > + davinci_pll_clk_register(&da850_pll1_info, "oscin", pll1); Both PLL0 and PLL1 use the same reference clock. So this should be "ref_clk". I dont think we ever need to register a clock called oscin along with "ref_clk". There is only one reference clock. It can either be obtained using internal oscillator or external oscillator. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > +void __init dm355_pll_clk_init(void __iomem *pll1, void __iomem *pll2) > +{ > + const struct davinci_pll_sysclk_info *info; > + > + davinci_pll_clk_register(&dm355_pll1_info, "ref_clk", pll1); > + > + for (info = dm355_pll1_sysclk_info; info->name; info++) > + davinci_pll_sysclk_register(info, pll1); > + > + davinci_pll_auxclk_register("pll1_auxclk", pll1); > + > + davinci_pll_sysclkbp_clk_register("pll1_sysclkbp", pll1); > + > + davinci_pll_clk_register(&dm355_pll2_info, "oscin", pll2); Even on DM355, both PLLs accept reference clock from the same input. So this can be "ref_clk" as well. Rest of it looks good to me (and thanks for looking up the data manual for accurate PLLM max and min settings). Regards, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > + > +void __init dm365_pll_clk_init(void __iomem *pll1, void __iomem *pll2) > +{ > + const struct davinci_pll_sysclk_info *info; > + > + davinci_pll_clk_register(&dm365_pll1_info, "ref_clk", pll1); > + > + davinci_pll_auxclk_register("pll1_auxclk", pll1); > + > + davinci_pll_sysclkbp_clk_register("pll1_sysclkbp", pll1); > + > + davinci_pll_obsclk_register(&dm365_pll1_obsclk_info, pll1); > + > + for (info = dm365_pll1_sysclk_info; info->name; info++) > + davinci_pll_sysclk_register(info, pll1); > + > + davinci_pll_clk_register(&dm365_pll2_info, "oscin", pll2); As pointed out in other patches, I think we can remove "oscin" here and also get rid of clk_register_fixed_factor() for oscin in davinci_pll_clk_register(). Rest of the patch looks good to me. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds a new driver for mach-davinci PSC clocks. This is porting the > code from arch/arm/mach-davinci/psc.c to the common clock framework and > is converting it to use regmap to simplify the code. Additionally, it > adds device tree support for these clocks. > > Note: although there are similar clocks for TI Keystone we are not able > to share the code for a few reasons. The keystone clocks are device tree > only and use legacy one-node-per-clock bindings. Also the keystone > driver makes the assumption that there is only one PSC per SoC and uses > global variables, but here we have two controllers per SoC. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI DA830/ > OMAP-L137/AM17XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI DA850/ > OMAP-L138/AM18XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > + LPSC(26, 0, gpio, pll1_sysclk2, gpio_clkdev, 0), > + LPSC(27, 0, timer0, pll1_auxclk, timer0_clkdev, 0), > + LPSC(28, 0, timer1, pll1_auxclk, NULL, 0), > + /* REVISIT: why can't this be disabled? */ > + LPSC(29, 0, timer2, pll1_auxclk, timer2_clkdev, > + LPSC_ALWAYS_ENABLED), > + LPSC(31, 0, arm, pll1_sysclk1, NULL, > + LPSC_ALWAYS_ENABLED), IMO, in this case the line break makes it more difficult to read so that checkpatch warning should be ignored. Thanks Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI > DM365 based systems. > > Signed-off-by: David Lechner <david@lechnology.com> Apart from comment given on 13/41 that applies here: Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI > DM644x based systems. > > Signed-off-by: David Lechner <david@lechnology.com> Apart from a comment given on similar file: Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > This adds platform-specific declarations for the PSC clocks on TI > DM646x based systems. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 01 February 2018 01:31 PM, Sekhar Nori wrote: > One thing missing is DIV4.5 clock. It will be nice to add that too, > mostly just because it will make the binding complete. Ah, ignore this comment please. I noticed that its part of cfgchip clocks (which makes sense). Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/01/2018 02:01 AM, Sekhar Nori wrote: > On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >> This adds a new driver for mach-davinci PLL clocks. This is porting the >> code from arch/arm/mach-davinci/clock.c to the common clock framework. >> Additionally, it adds device tree support for these clocks. >> >> The ifeq ($(CONFIG_COMMON_CLK), y) in the Makefile is needed to prevent >> compile errors until the clock code in arch/arm/mach-davinci is removed. >> >> Note: although there are similar clocks for TI Keystone we are not able >> to share the code for a few reasons. The keystone clocks are device tree >> only and use legacy one-node-per-clock bindings. Also the register >> layouts are a bit different, which would add even more if/else mess >> to the keystone clocks. And the keystone PLL driver doesn't support >> setting clock rates. >> >> Signed-off-by: David Lechner <david@lechnology.com> > > Looks nice and clean to me. There is still some feedback though. > > One thing missing is DIV4.5 clock. It will be nice to add that too, > mostly just because it will make the binding complete. > >> +void of_davinci_pll_init(struct device_node *node, >> + const struct davinci_pll_clk_info *info, >> + const struct davinci_pll_obsclk_info *obsclk_info, >> + const struct davinci_pll_sysclk_info *div_info, >> + u8 max_sysclk_id) >> +{ >> + struct device_node *child; >> + const char *parent_name; >> + void __iomem *base; >> + struct clk *clk; >> + >> + base = of_iomap(node, 0); >> + if (!base) { >> + pr_err("ioremap failed"); >> + return; >> + } >> + >> + if (info->flags & PLL_HAS_OSCIN) >> + parent_name = of_clk_get_parent_name(node, 0); >> + else >> + parent_name = OSCIN_CLK_NAME; > > I don't think the reference clock input handling is fully correct/flexible. > > There are two ways to provide input clock (ref_clk) to PLL. Either use > the internal oscillator with a crystal connected between OSCIN and > OSCOUT (CLKMODE = 0) or a clean clock input (CLKMODE = 1) connected to > OSCIN (OSCOUT disconnected). > > This is a board specific decision. On the LogicPD EVM, the former option > is used, on the LCDK board, the later. > > So, I think what we need is a DT property like > "ti,davinci-use-internal-osc" for the PLL. Boards can set or ignore it > and you can switch CLKMODE on or off based on that. > > Setting CLKMODE = 1 will switch off the internal oscillator (and > presumably save power), but it does not act as a mux. This explains why > not worrying about setting this correctly in current mainline still works. > > I am also not sure if we really need PLL_HAS_OSCIN since all DaVinci > SoCs set it anyway. I realize this is a bit confusing. I think that part of this comes from the fact that OSCIN is not used consistently in the TRMs. It is used as the name of the actual pin on the SoC for connecting an external clock signal or crystal. It is also used as an input to CLKMODE where it means the output of the internal oscillator rather than the external pin (some SoCs show CLKMODE as a mux with OSCIN and CLKIN, but I agree that it is not really a mux since OSCIN and CLKIN are the same external pin on the SoC - then other SoCs show OSCIN meaning only the external pin here). Furthermore, OSCIN is listed as one of the inputs of EXTCLKSRC also as one of the inputs of OBSCLK on da850-type SoCs. So, the option I went with here is that "ref_clk" is the external clock connected to the OSCIN pin and that the "oscin" clock is the clock domain _after_ CLKMODE. This matches the use of OSCIN in the TRMs where "OSCIN" is used as an input for EXTCLKSRC and OBSCLK. Also the fact that the external reference clock is sometimes called CLKSRC instead of OSCIN influenced the decision go with "oscin" being the internal (to the PLL) clock domain. I think what I should have done, though, is named PLL_HAS_OSCIN as PLL_HAS_CLKMODE instead. I think what you are missing here is that PLL_HAS_OSCIN (or PLL_HAS_CLKMODE) only means that the PLL _has_ PLLCTL[CLKMODE]. It does _not_ mean that we set (or clear) PLLCTL[CLKMODE]. On SoCs with two PLLs, only one of them has the PLLCTL[CLKMODE] bit (and therefore the PLL_HAS_OSCIN flag) and the output of this is shared by both PLLs, e.g. Figure 7-1. PLLC Structure in the AM1808 TRM (spruh82c). So, the clock tree in Linux ends up looking like this: ref_clk - the external clock - aka CLKSRC +-oscin - internal clock domain after CLKMODE - shared by both PLLs +-pll0_extclksrc +-pll0_prediv +-pll0_auxclk +-pll0_obsclk - via the OSCSRC mux +-pll1_pllen +-pll1_pllout +-pll1_obsclk - via the OSCSRC mux Clocks beyond two levels deep are omitted for brevity. It should be easy to see the correlation here Figure 7-1 with the exception that in Figure 7-1 "OSCIN" has the meaning of "the physical pin on the chip" (which Linux calls "ref_clk") and there is no clear label in Figure 7-1 of what the clock domain after PLLCTL[CLKMODE] is called (which Linux calls "oscin"). Sure, it would work just fine if we left "oscin" out of the picture, but it simplifies the driver implementation by having a known "oscin" clock that is fully controlled by the clock driver code instead of having to pass the user-supplied "ref_clk" around a bunch of places. And, we could try to call it something other than "oscin" to try to avoid confusion, but then you will just cause confusion in other places (which could be less total confusion, so I am open to change here). --- Switching topics to CLKMODE and DT... There is a "ti,clkmode-square-wave" property in the proposed DT bindings that does exactly what you have suggested, except the logic is reversed. When omitted, it assumes the use of a crystal oscillator. I believe this is the most common configuration, which is why I made it the default instead of the other way around. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/01/2018 02:58 AM, Sekhar Nori wrote: > On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >> This adds platform-specific declarations for the PLL clocks on TI DA850/ >> OMAP-L138/AM18XX SoCs. >> >> Signed-off-by: David Lechner <david@lechnology.com> > >> +static const struct davinci_pll_clk_info da850_pll1_info __initconst = { >> + .name = "pll1", >> + .unlock_reg = CFGCHIP(3), >> + .unlock_mask = CFGCHIP3_PLL1_MASTER_LOCK, > > I guess this will change with the cfgchip handling discussion last week. Actually no, there really weren't any changes to the clock drivers because of this change. Only a small change in mach-davinci. > >> + .pllm_mask = GENMASK(4, 0), >> + .pllm_min = 4, >> + .pllm_max = 32, >> + .pllout_min_rate = 300000000, >> + .pllout_max_rate = 600000000, >> + .flags = PLL_HAS_POSTDIV, >> +}; >> + > > [...] > >> +void __init da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1) >> +{ >> + const struct davinci_pll_sysclk_info *info; >> + >> + davinci_pll_clk_register(&da850_pll0_info, "ref_clk", pll0); >> + >> + davinci_pll_auxclk_register("pll0_auxclk", pll0); >> + >> + for (info = da850_pll0_sysclk_info; info->name; info++) >> + davinci_pll_sysclk_register(info, pll0); >> + >> + davinci_pll_obsclk_register(&da850_pll0_obsclk_info, pll0); >> + >> + davinci_pll_clk_register(&da850_pll1_info, "oscin", pll1); > > Both PLL0 and PLL1 use the same reference clock. So this should be > "ref_clk". I dont think we ever need to register a clock called oscin > along with "ref_clk". There is only one reference clock. It can either > be obtained using internal oscillator or external oscillator. > As per my response to the previous path, this depends on which both which SoC and which diagram in the TRM for that SoC you are looking at. It works either way. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/20/2018 11:13 AM, David Lechner wrote: > This adds platform-specific declarations for the PLL clocks on TI DA850/ > OMAP-L138/AM18XX SoCs. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > > v6 changes: > - Added da850_pll{0,1}_info with controller-specific information > - Added OBSCLK data > - Add empty lines between function calls > > drivers/clk/davinci/Makefile | 1 + > drivers/clk/davinci/pll-da850.c | 163 ++++++++++++++++++++++++++++++++++++++++ > include/linux/clk/davinci.h | 1 + > 3 files changed, 165 insertions(+) > create mode 100644 drivers/clk/davinci/pll-da850.c > > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > index 9061e19..13049d4 100644 > --- a/drivers/clk/davinci/Makefile > +++ b/drivers/clk/davinci/Makefile > @@ -3,4 +3,5 @@ > ifeq ($(CONFIG_COMMON_CLK), y) > obj-y += pll.o > obj-$(CONFIG_ARCH_DAVINCI_DA830) += pll-da830.o > +obj-$(CONFIG_ARCH_DAVINCI_DA850) += pll-da850.o > endif > diff --git a/drivers/clk/davinci/pll-da850.c b/drivers/clk/davinci/pll-da850.c > new file mode 100644 > index 0000000..a94e1a6 > --- /dev/null > +++ b/drivers/clk/davinci/pll-da850.c > @@ -0,0 +1,163 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PLL clock descriptions for TI DA850/OMAP-L138/AM18XX > + * > + * Copyright (C) 2018 David Lechner <david@lechnology.com> > + */ > + > +#include <linux/bitops.h> > +#include <linux/clk-provider.h> > +#include <linux/clkdev.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/mfd/da8xx-cfgchip.h> > +#include <linux/of.h> > +#include <linux/types.h> > + > +#include "pll.h" > + > +#define OCSEL_OCSRC_OSCIN 0x14 > +#define OCSEL_OCSRC_PLL0_SYSCLK(n) (0x16 + (n)) > +#define OCSEL_OCSRC_PLL1_OBSCLK 0x1e > +#define OCSEL_OCSRC_PLL1_SYSCLK(n) (0x16 + (n)) > + > +static const struct davinci_pll_clk_info da850_pll0_info __initconst = { > + .name = "pll0", > + .unlock_reg = CFGCHIP(0), > + .unlock_mask = CFGCHIP0_PLL_MASTER_LOCK, > + .pllm_mask = GENMASK(4, 0), > + .pllm_min = 4, > + .pllm_max = 32, > + .pllout_min_rate = 300000000, > + .pllout_max_rate = 600000000, > + .flags = PLL_HAS_OSCIN | PLL_HAS_PREDIV | PLL_HAS_POSTDIV | > + PLL_HAS_EXTCLKSRC, > +}; > + > +/* > + * NB: Technically, the clocks flagged as SYSCLK_FIXED_DIV are "fixed ratio", > + * meaning that we could change the divider as long as we keep the correct > + * ratio between all of the clocks, but we don't support that because there is > + * currently not a need for it. > + */ > + > +static const struct davinci_pll_sysclk_info da850_pll0_sysclk_info[] __initconst = { > + SYSCLK(1, pll0_sysclk1, pll0_pllen, 5, SYSCLK_FIXED_DIV), > + SYSCLK(2, pll0_sysclk2, pll0_pllen, 5, SYSCLK_FIXED_DIV), > + SYSCLK(3, pll0_sysclk3, pll0_pllen, 5, 0), > + SYSCLK(4, pll0_sysclk4, pll0_pllen, 5, SYSCLK_FIXED_DIV), > + SYSCLK(5, pll0_sysclk5, pll0_pllen, 5, 0), > + SYSCLK(6, pll0_sysclk6, pll0_pllen, 5, SYSCLK_ARM_RATE | SYSCLK_FIXED_DIV), > + SYSCLK(7, pll0_sysclk7, pll0_pllen, 5, 0), > + { } > +}; > + > +static const char * const da850_pll0_obsclk_parent_names[] __initconst = { > + "oscin", > + "pll0_sysclk1", > + "pll0_sysclk2", > + "pll0_sysclk3", > + "pll0_sysclk4", > + "pll0_sysclk5", > + "pll0_sysclk6", > + "pll0_sysclk7", > + "pll1_obsclk", > +}; > + > +static u32 da850_pll0_obsclk_table[] = { > + OCSEL_OCSRC_OSCIN, > + OCSEL_OCSRC_PLL0_SYSCLK(1), > + OCSEL_OCSRC_PLL0_SYSCLK(2), > + OCSEL_OCSRC_PLL0_SYSCLK(3), > + OCSEL_OCSRC_PLL0_SYSCLK(4), > + OCSEL_OCSRC_PLL0_SYSCLK(5), > + OCSEL_OCSRC_PLL0_SYSCLK(6), > + OCSEL_OCSRC_PLL0_SYSCLK(7), > + OCSEL_OCSRC_PLL1_OBSCLK, > +}; > + > +static const struct davinci_pll_obsclk_info da850_pll0_obsclk_info __initconst = { > + .name = "pll0_obsclk", > + .parent_names = da850_pll0_obsclk_parent_names, > + .num_parents = ARRAY_SIZE(da850_pll0_obsclk_parent_names), > + .table = da850_pll0_obsclk_table, > + .ocsrc_mask = GENMASK(4, 0), > +}; > + > +static const struct davinci_pll_clk_info da850_pll1_info __initconst = { > + .name = "pll1", > + .unlock_reg = CFGCHIP(3), > + .unlock_mask = CFGCHIP3_PLL1_MASTER_LOCK, > + .pllm_mask = GENMASK(4, 0), > + .pllm_min = 4, > + .pllm_max = 32, > + .pllout_min_rate = 300000000, > + .pllout_max_rate = 600000000, > + .flags = PLL_HAS_POSTDIV, > +}; > + > +static const struct davinci_pll_sysclk_info da850_pll1_sysclk_info[] __initconst = { > + SYSCLK(1, pll1_sysclk1, pll1_pllen, 5, SYSCLK_ALWAYS_ENABLED), > + SYSCLK(2, pll1_sysclk2, pll1_pllen, 5, 0), > + SYSCLK(3, pll1_sysclk3, pll1_pllen, 5, 0), > + { } > +}; > + > +static const char * const da850_pll1_obsclk_parent_names[] __initconst = { > + "oscin", Re: the issue of "ref_clk" vs. "oscin"... This is one of the places where having the otherwise unnecessary "oscin" clock really helps out. The PLL driver doesn't control "ref_clk" - it comes from somewhere else. And in the case of DT, it may not even be named "ref_clk", so we really don't want to hard-code the name "ref_clk" here. If we have to allow a variable name here, it just makes more work in the driver shuffling names around. And the name "oscin" totally makes sense here because the TRM lists this input to the mux as "OSCIN". > + "pll1_sysclk1", > + "pll1_sysclk2", > + "pll1_sysclk3", > +}; > + > +static u32 da850_pll1_obsclk_table[] = { > + OCSEL_OCSRC_OSCIN, > + OCSEL_OCSRC_PLL1_SYSCLK(1), > + OCSEL_OCSRC_PLL1_SYSCLK(2), > + OCSEL_OCSRC_PLL1_SYSCLK(3), > +}; > + > +static const struct davinci_pll_obsclk_info da850_pll1_obsclk_info __initconst = { > + .name = "pll1_obsclk", > + .parent_names = da850_pll1_obsclk_parent_names, > + .num_parents = ARRAY_SIZE(da850_pll1_obsclk_parent_names), > + .table = da850_pll1_obsclk_table, > + .ocsrc_mask = GENMASK(4, 0), > +}; > + > +void __init da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1) > +{ > + const struct davinci_pll_sysclk_info *info; > + > + davinci_pll_clk_register(&da850_pll0_info, "ref_clk", pll0); And really, we probably shouldn't be hard-coding "ref_clk" here either. Basically, we are making the assumption that the board file has registered a clock named "ref_clk". It would probably be better to pass the name as a parameter. > + > + davinci_pll_auxclk_register("pll0_auxclk", pll0); > + > + for (info = da850_pll0_sysclk_info; info->name; info++) > + davinci_pll_sysclk_register(info, pll0); > + > + davinci_pll_obsclk_register(&da850_pll0_obsclk_info, pll0); > + > + davinci_pll_clk_register(&da850_pll1_info, "oscin", pll1); > + > + for (info = da850_pll1_sysclk_info; info->name; info++) > + davinci_pll_sysclk_register(info, pll1); > + > + davinci_pll_obsclk_register(&da850_pll1_obsclk_info, pll1); > +} > + > +#ifdef CONFIG_OF > +static void __init of_da850_pll0_auxclk_init(struct device_node *node) > +{ > + of_davinci_pll_init(node, &da850_pll0_info, &da850_pll0_obsclk_info, > + da850_pll0_sysclk_info, 7); > +} > +CLK_OF_DECLARE(da850_pll0_auxclk, "ti,da850-pll0", of_da850_pll0_auxclk_init); > + > +static void __init of_da850_pll1_auxclk_init(struct device_node *node) > +{ > + of_davinci_pll_init(node, &da850_pll1_info, &da850_pll1_obsclk_info, > + da850_pll1_sysclk_info, 3); > +} > +CLK_OF_DECLARE(da850_pll1_auxclk, "ti,da850-pll1", of_da850_pll1_auxclk_init); > +#endif > diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h > index 4f4d60d..7b08fe0 100644 > --- a/include/linux/clk/davinci.h > +++ b/include/linux/clk/davinci.h > @@ -10,5 +10,6 @@ > #include <linux/types.h> > > void da830_pll_clk_init(void __iomem *pll); > +void da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1); > > #endif /* __LINUX_CLK_DAVINCI_H__ */ > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 02 February 2018 12:27 AM, David Lechner wrote: > On 02/01/2018 02:01 AM, Sekhar Nori wrote: >> On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >>> This adds a new driver for mach-davinci PLL clocks. This is porting the >>> code from arch/arm/mach-davinci/clock.c to the common clock framework. >>> Additionally, it adds device tree support for these clocks. >>> >>> The ifeq ($(CONFIG_COMMON_CLK), y) in the Makefile is needed to prevent >>> compile errors until the clock code in arch/arm/mach-davinci is removed. >>> >>> Note: although there are similar clocks for TI Keystone we are not able >>> to share the code for a few reasons. The keystone clocks are device tree >>> only and use legacy one-node-per-clock bindings. Also the register >>> layouts are a bit different, which would add even more if/else mess >>> to the keystone clocks. And the keystone PLL driver doesn't support >>> setting clock rates. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >> >> Looks nice and clean to me. There is still some feedback though. >> >> One thing missing is DIV4.5 clock. It will be nice to add that too, >> mostly just because it will make the binding complete. >> >>> +void of_davinci_pll_init(struct device_node *node, >>> + const struct davinci_pll_clk_info *info, >>> + const struct davinci_pll_obsclk_info *obsclk_info, >>> + const struct davinci_pll_sysclk_info *div_info, >>> + u8 max_sysclk_id) >>> +{ >>> + struct device_node *child; >>> + const char *parent_name; >>> + void __iomem *base; >>> + struct clk *clk; >>> + >>> + base = of_iomap(node, 0); >>> + if (!base) { >>> + pr_err("ioremap failed"); >>> + return; >>> + } >>> + >>> + if (info->flags & PLL_HAS_OSCIN) >>> + parent_name = of_clk_get_parent_name(node, 0); >>> + else >>> + parent_name = OSCIN_CLK_NAME; >> >> I don't think the reference clock input handling is fully >> correct/flexible. >> >> There are two ways to provide input clock (ref_clk) to PLL. Either use >> the internal oscillator with a crystal connected between OSCIN and >> OSCOUT (CLKMODE = 0) or a clean clock input (CLKMODE = 1) connected to >> OSCIN (OSCOUT disconnected). >> >> This is a board specific decision. On the LogicPD EVM, the former option >> is used, on the LCDK board, the later. >> >> So, I think what we need is a DT property like >> "ti,davinci-use-internal-osc" for the PLL. Boards can set or ignore it >> and you can switch CLKMODE on or off based on that. >> >> Setting CLKMODE = 1 will switch off the internal oscillator (and >> presumably save power), but it does not act as a mux. This explains why >> not worrying about setting this correctly in current mainline still >> works. >> >> I am also not sure if we really need PLL_HAS_OSCIN since all DaVinci >> SoCs set it anyway. > > > I realize this is a bit confusing. I think that part of this comes from > the fact that OSCIN is not used consistently in the TRMs. It is used as Thats right, I noticed that too. But all SoC datasheets I looked at supported both internal oscillator and external clock input. Also, no SoC had different reference clocks for its PLLs (DM355 has two crystal inputs, but one of them goes only to the video peripheral). So I still think there is benefit in standardizing on a single name in kernel/DT (I was hoping it can be "ref_clk"). > the name of the actual pin on the SoC for connecting an external clock > signal or crystal. It is also used as an input to CLKMODE where it means > the output of the internal oscillator rather than the external pin (some > SoCs show CLKMODE as a mux with OSCIN and CLKIN, but I agree that it is > not really a mux since OSCIN and CLKIN are the same external pin on the > SoC - then other SoCs show OSCIN meaning only the external pin here). > Furthermore, OSCIN is listed as one of the inputs of EXTCLKSRC also as > one of the inputs of OBSCLK on da850-type SoCs. Right. > So, the option I went with here is that "ref_clk" is the external clock > connected to the OSCIN pin and that the "oscin" clock is the clock domain > _after_ CLKMODE. This matches the use of OSCIN in the TRMs where "OSCIN" > is used as an input for EXTCLKSRC and OBSCLK. Also the fact that the > external reference clock is sometimes called CLKSRC instead of OSCIN > influenced the decision go with "oscin" being the internal (to the PLL) > clock domain. Okay, I think we need some comments in code to make this distinction clear. I do not yet understand why we need to differentiate between the external-to-chip clock domain from internal "after CLKMODE" domain. OTOH, I don't see a big harm in doing it either (as long as the distinction is clear). > > I think what I should have done, though, is named PLL_HAS_OSCIN as > PLL_HAS_CLKMODE instead. I think what you are missing here is that > PLL_HAS_OSCIN (or PLL_HAS_CLKMODE) only means that the PLL _has_ > PLLCTL[CLKMODE]. It does _not_ mean that we set (or clear) PLLCTL[CLKMODE]. > On SoCs with two PLLs, only one of them has the PLLCTL[CLKMODE] bit (and > therefore the PLL_HAS_OSCIN flag) and the output of this is shared by both > PLLs, e.g. Figure 7-1. PLLC Structure in the AM1808 TRM (spruh82c). So, > the clock tree in Linux ends up looking like this: I agree with renaming PLL_HAS_OSCIN with PLL_HAS_CLKMODE will be much clearer. > > ref_clk - the external clock - aka CLKSRC > +-oscin - internal clock domain after CLKMODE - shared by both PLLs > +-pll0_extclksrc > +-pll0_prediv > +-pll0_auxclk > +-pll0_obsclk - via the OSCSRC mux > +-pll1_pllen > +-pll1_pllout > +-pll1_obsclk - via the OSCSRC mux > > Clocks beyond two levels deep are omitted for brevity. > > It should be easy to see the correlation here Figure 7-1 with the > exception that in Figure 7-1 "OSCIN" has the meaning of "the physical > pin on the chip" (which Linux calls "ref_clk") and there is no clear > label in Figure 7-1 of what the clock domain after PLLCTL[CLKMODE] is > called (which Linux calls "oscin"). Yeah, thats why I was not sure why need to make the distinction between these domains. > Sure, it would work just fine if we left "oscin" out of the picture, > but it simplifies the driver implementation by having a known "oscin" > clock that is fully controlled by the clock driver code instead of > having to pass the user-supplied "ref_clk" around a bunch of places. Okay. > And, we could try to call it something other than "oscin" to try > to avoid confusion, but then you will just cause confusion in other > places (which could be less total confusion, so I am open to change > here). Calling external clock as "ref_clk" and internal clock domain as "oscin" is fine. Its the best choice of names given the terminology inconsistency between TRMs of different devices. > --- > > Switching topics to CLKMODE and DT... > > There is a "ti,clkmode-square-wave" property in the proposed DT bindings > that does exactly what you have suggested, except the logic is > reversed. When omitted, it assumes the use of a crystal oscillator. > I believe this is the most common configuration, which is why I made > it the default instead of the other way around. This is fine and looks like I missed its existence while making my suggestion. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 02 February 2018 12:34 AM, David Lechner wrote: > On 02/01/2018 02:58 AM, Sekhar Nori wrote: >> On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >>> This adds platform-specific declarations for the PLL clocks on TI DA850/ >>> OMAP-L138/AM18XX SoCs. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >> >>> +static const struct davinci_pll_clk_info da850_pll1_info __initconst >>> = { >>> + .name = "pll1", >>> + .unlock_reg = CFGCHIP(3), >>> + .unlock_mask = CFGCHIP3_PLL1_MASTER_LOCK, >> >> I guess this will change with the cfgchip handling discussion last week. > > Actually no, there really weren't any changes to the clock drivers because > of this change. Only a small change in mach-davinci. > >> >>> + .pllm_mask = GENMASK(4, 0), >>> + .pllm_min = 4, >>> + .pllm_max = 32, >>> + .pllout_min_rate = 300000000, >>> + .pllout_max_rate = 600000000, >>> + .flags = PLL_HAS_POSTDIV, >>> +}; >>> + >> >> [...] >> >>> +void __init da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1) >>> +{ >>> + const struct davinci_pll_sysclk_info *info; >>> + >>> + davinci_pll_clk_register(&da850_pll0_info, "ref_clk", pll0); >>> + >>> + davinci_pll_auxclk_register("pll0_auxclk", pll0); >>> + >>> + for (info = da850_pll0_sysclk_info; info->name; info++) >>> + davinci_pll_sysclk_register(info, pll0); >>> + >>> + davinci_pll_obsclk_register(&da850_pll0_obsclk_info, pll0); >>> + >>> + davinci_pll_clk_register(&da850_pll1_info, "oscin", pll1); >> >> Both PLL0 and PLL1 use the same reference clock. So this should be >> "ref_clk". I dont think we ever need to register a clock called oscin >> along with "ref_clk". There is only one reference clock. It can either >> be obtained using internal oscillator or external oscillator. >> > > As per my response to the previous path, this depends on which both > which SoC and which diagram in the TRM for that SoC you are looking at. > It works either way. I see the distinction you are making between clock inputs to the two PLLs now. A comment somewhere (probably in pll.c) should do it. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 02 February 2018 12:52 AM, David Lechner wrote: >> +static const char * const da850_pll1_obsclk_parent_names[] >> __initconst = { >> + "oscin", > > Re: the issue of "ref_clk" vs. "oscin"... > > This is one of the places where having the otherwise unnecessary "oscin" > clock > really helps out. The PLL driver doesn't control "ref_clk" - it comes > from somewhere > else. And in the case of DT, it may not even be named "ref_clk", so we > really > don't want to hard-code the name "ref_clk" here. TBH, I don't really see what is wrong with mandating the name "ref_clk" as the reference clock name to be provided. And for all board-files and DTs to supply the same name. > If we have to allow a variable name here, it just makes more work in the > driver > shuffling names around. > > And the name "oscin" totally makes sense here because the TRM lists this > input to the > mux as "OSCIN". Fine with me if you feel it simplifies implementation for you (and also because of the distinction you want to make between the external "before CLKMODE" clock and internal "after CLKMODE" clock). What I do care about though is: a) In the DT case, ability for different boards to provide different ref_clk frequencies. We never really had this in the legacy board file way (except some rudimentary support on DM6467T). And its fine to continue with status quo for board files. b) In the DT case, ability for board to specify whether it uses the on-chip oscillator or has an external clean clock provider. >> +void __init da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1) >> +{ >> + const struct davinci_pll_sysclk_info *info; >> + >> + davinci_pll_clk_register(&da850_pll0_info, "ref_clk", pll0); > > And really, we probably shouldn't be hard-coding "ref_clk" here either. > Basically, we are making the assumption that the board file has registered > a clock named "ref_clk". It would probably be better to pass the name > as a parameter. As I noted before, I am not sure if this level of naming flexibility is needed. Every board needs to have one anyway. They might as well call it by the same name. That said, I wont oppose it either if you decide to have that flexibility. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > +static const struct clk_ops da8xx_cfgchip_div4p5_clk_ops = { > + .enable = da8xx_cfgchip_gate_clk_enable, > + .disable = da8xx_cfgchip_gate_clk_disable, > + .is_enabled = da8xx_cfgchip_gate_clk_is_enabled, I assume the reason for not using clk-gate.c is lack of regmap support there? > + .recalc_rate = da8xx_cfgchip_div4p5_recalc_rate, > +}; > + > +static struct clk * __init > +da8xx_cfgchip_gate_clk_register(const struct da8xx_cfgchip_gate_clk_info *info, > + const char *parent_name, > + struct regmap *regmap) > +{ > + struct da8xx_cfgchip_gate_clk *gate; > + struct clk_init_data init; > + > + gate = kzalloc(sizeof(*gate), GFP_KERNEL); > + if (!gate) > + return ERR_PTR(-ENOMEM); > + > + init.name = info->name; > + if (info->flags & DA8XX_GATE_CLOCK_IS_DIV4P5) > + init.ops = &da8xx_cfgchip_div4p5_clk_ops; > + else > + init.ops = &da8xx_cfgchip_gate_clk_ops; This will be easier to read using ternary operator, I think. But you will probably have line breaks. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 02 February 2018 06:49 PM, Sekhar Nori wrote: > On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >> +static const struct clk_ops da8xx_cfgchip_div4p5_clk_ops = { >> + .enable = da8xx_cfgchip_gate_clk_enable, >> + .disable = da8xx_cfgchip_gate_clk_disable, >> + .is_enabled = da8xx_cfgchip_gate_clk_is_enabled, > > I assume the reason for not using clk-gate.c is lack of regmap support > there? This is rather question for da8xx_cfgchip_gate_clk_ops. Sorry about the bad context. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > diff --git a/include/linux/clk/davinci.h b/include/linux/clk/davinci.h > index 54ea3ff..04b48b3 100644 > --- a/include/linux/clk/davinci.h > +++ b/include/linux/clk/davinci.h > @@ -9,6 +9,9 @@ > > #include <linux/types.h> > > +struct clk; > +struct regmap; Its probably better to include clk-provider.h and regmap.h? Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > void __init da830_init_time(void) > { > +#ifdef CONFIG_COMMON_CLK > + void __iomem *pll0, *psc0, *psc1; > + struct clk *clk; > + > + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); > + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); > + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); > + > + da8xx_register_cfgchip(); > + > + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ); > + > + da830_pll_clk_init(pll0); > + > + da830_psc_clk_init(psc0, psc1); > + > + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); > + > + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, "timer0", NULL); > + > + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, NULL, "davinci-wdt"); Isn't this better done in da830_pll_clk_init() ? I think we can get rid of the dummy fixed factor clock too and directly use the pll0_auxclk. That reminds me, is "pll0_aux_clk" above correct, or should it be "pll0_auxclk" like in da830_pll_clk_init()? > + > + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); > + clk_register_clkdev(clk, "rmii", NULL); I don't see any driver looking for this clock using con_id "rmii". I know this came from existing code. But its most likely a vestige and can be dropped. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > void __init da850_init_time(void) > { > +#ifdef CONFIG_COMMON_CLK > + void __iomem *pll0, *pll1, *psc0, *psc1; > + struct regmap *cfgchip; > + struct clk *clk; > + struct clk_hw *parent; > + > + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); > + pll1 = ioremap(DA850_PLL1_BASE, SZ_4K); > + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); > + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); > + > + cfgchip = da8xx_register_cfgchip(); > + if (WARN(IS_ERR(cfgchip), "failed to register CFGCHIP syscon")) > + return; > + > + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA850_REF_FREQ); > + > + da850_pll_clk_init(pll0, pll1); > + > + da8xx_cfgchip_register_div4p5(cfgchip); > + > + da8xx_cfgchip_register_async1(cfgchip); > + > + clk = clk_register_fixed_factor(NULL, "async2", "pll0_auxclk", 0, 1, 1); > + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); > + clk_register_clkdev(clk, "timer0", NULL); > + clk_register_clkdev(clk, NULL, "davinci-wdt"); I think its better to get these clkdevs registered in da850_pll_clk_init() itself. > + > + clk = da8xx_cfgchip_register_async3(cfgchip); > + > + /* pll1_sysclk2 is not affected by CPU scaling, so use it for async3 */ > + parent = clk_hw_get_parent_by_index(__clk_get_hw(clk), 1); > + if (parent) > + clk_set_parent(clk, parent->clk); > + else > + pr_warn("%s: Failed to find async3 parent clock\n", __func__); > + > + da850_psc_clk_init(psc0, psc1); > + > + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); > + clk_register_clkdev(clk, "rmii", NULL); Like in da830, can drop this rmii clock, I think. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This adds the new board-specific clock init in mach-davinci/dm355.c > using the new common clock framework drivers. > > The #ifdefs are needed to prevent compile errors until the entire > ARCH_DAVINCI is converted. > > Also clean up the #includes since we are adding some here. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This adds the new board-specific clock init in mach-davinci/dm365.c > using the new common clock framework drivers. > > The #ifdefs are needed to prevent compile errors until the entire > ARCH_DAVINCI is converted. > > Also clean up the #includes since we are adding some here. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This adds the new board-specific clock init in mach-davinci/dm644x.c > using the new common clock framework drivers. > > The #ifdefs are needed to prevent compile errors until the entire > ARCH_DAVINCI is converted. > > Also clean up the #includes since we are adding some here. > > Signed-off-by: David Lechner <david@lechnology.com> I did not notice this before, but the subject line for this and other patches needs to be "ARM: davinci: " Apart from that: Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > void __init dm646x_init_time(unsigned long ref_clk_rate, > unsigned long aux_clkin_rate) > { > +#ifdef CONFIG_COMMON_CLK > + void __iomem *pll1, *pll2, *psc; > + struct clk *clk; > + > + pll1 = ioremap(DAVINCI_PLL1_BASE, SZ_4K); > + pll2 = ioremap(DAVINCI_PLL2_BASE, SZ_4K); > + psc = ioremap(DAVINCI_PWR_SLEEP_CNTRL_BASE, SZ_4K); > + > + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, ref_clk_rate); > + clk_register_fixed_rate(NULL, "aux_clkin", NULL, 0, aux_clkin_rate); > + > + dm646x_pll_clk_init(pll1, pll2); > + > + dm646x_psc_clk_init(psc); > + /* no LPSC, always enabled; c.f. spruep9a */ > + clk = clk_register_fixed_factor(NULL, "timer2", "pll1_sysclk3", 0, 1, 1); > + clk_register_clkdev(clk, NULL, "davinci-wdt"); Lets move this to dm646x_pll_clk_init() and directly register to clkdev to pll1_sysclk3? Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This adds the new SATA REFCLK clock init in mach-davinci/devices-da8xx.c > using the new common clock framework drivers. > > The #ifdefs are needed to prevent compile errors until the entire > ARCH_DAVINCI is converted. > > Also, the #includes are sorted since we are adding some here. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > The common clock framework will take care of disabling unused clocks when > we switch from the legacy davinci clocks and having this enabled will > cause compile errors after we switch, so remove it now. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes CONFIG_DAVINCI_RESET_CLOCKS. The option has been removed from > the kernel. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/02/2018 02:37 AM, Sekhar Nori wrote: > On Friday 02 February 2018 12:52 AM, David Lechner wrote: > >>> +static const char * const da850_pll1_obsclk_parent_names[] >>> __initconst = { >>> + "oscin", >> >> Re: the issue of "ref_clk" vs. "oscin"... >> >> This is one of the places where having the otherwise unnecessary "oscin" >> clock >> really helps out. The PLL driver doesn't control "ref_clk" - it comes >> from somewhere >> else. And in the case of DT, it may not even be named "ref_clk", so we >> really >> don't want to hard-code the name "ref_clk" here. > > TBH, I don't really see what is wrong with mandating the name "ref_clk" > as the reference clock name to be provided. And for all board-files and > DTs to supply the same name. > >> If we have to allow a variable name here, it just makes more work in the >> driver >> shuffling names around. >> >> And the name "oscin" totally makes sense here because the TRM lists this >> input to the >> mux as "OSCIN". > > Fine with me if you feel it simplifies implementation for you (and also > because of the distinction you want to make between the external "before > CLKMODE" clock and internal "after CLKMODE" clock). What I do care about > though is: > > a) In the DT case, ability for different boards to provide different > ref_clk frequencies. We never really had this in the legacy board > file way (except some rudimentary support on DM6467T). And its fine > to continue with status quo for board files. You can do this now. You would just add something like this to the board's .dts file: & ref_clk { clock-frequency = <30000000>; /* 30 MHz */ }; > > b) In the DT case, ability for board to specify whether it uses the > on-chip oscillator or has an external clean clock provider. Boards with an oscillator don't need to do anything since that is the default. Boards with clock will need to do this: &pll0 { ti,clkmode-square-wave; }; > >>> +void __init da850_pll_clk_init(void __iomem *pll0, void __iomem *pll1) >>> +{ >>> + const struct davinci_pll_sysclk_info *info; >>> + >>> + davinci_pll_clk_register(&da850_pll0_info, "ref_clk", pll0); >> >> And really, we probably shouldn't be hard-coding "ref_clk" here either. >> Basically, we are making the assumption that the board file has registered >> a clock named "ref_clk". It would probably be better to pass the name >> as a parameter. > > As I noted before, I am not sure if this level of naming flexibility is > needed. Every board needs to have one anyway. They might as well call it > by the same name. > > That said, I wont oppose it either if you decide to have that flexibility. > I'll sleep on it. Like you, I could go either way. I'm temped to just leave it as-is though (with some added comments for clarification, of course). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/02/2018 07:19 AM, Sekhar Nori wrote: > On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >> +static const struct clk_ops da8xx_cfgchip_div4p5_clk_ops = { >> + .enable = da8xx_cfgchip_gate_clk_enable, >> + .disable = da8xx_cfgchip_gate_clk_disable, >> + .is_enabled = da8xx_cfgchip_gate_clk_is_enabled, > > I assume the reason for not using clk-gate.c is lack of regmap support > there? Correct. I couldn't find a way to get a lock from the regmap that could be passed to clk_register_gate() to prevent non-clock drivers from trying to use the regmap at the same the the clocks are. > >> + .recalc_rate = da8xx_cfgchip_div4p5_recalc_rate, >> +}; >> + >> +static struct clk * __init >> +da8xx_cfgchip_gate_clk_register(const struct da8xx_cfgchip_gate_clk_info *info, >> + const char *parent_name, >> + struct regmap *regmap) >> +{ >> + struct da8xx_cfgchip_gate_clk *gate; >> + struct clk_init_data init; >> + >> + gate = kzalloc(sizeof(*gate), GFP_KERNEL); >> + if (!gate) >> + return ERR_PTR(-ENOMEM); >> + >> + init.name = info->name; >> + if (info->flags & DA8XX_GATE_CLOCK_IS_DIV4P5) >> + init.ops = &da8xx_cfgchip_div4p5_clk_ops; >> + else >> + init.ops = &da8xx_cfgchip_gate_clk_ops; > > This will be easier to read using ternary operator, I think. But you > will probably have line breaks. The names are so long that the ternary operator doesn't make it better IMHO. init.ops = (info->flags & DA8XX_GATE_CLOCK_IS_DIV4P5) ? &da8xx_cfgchip_div4p5_clk_ops : &da8xx_cfgchip_gate_clk_ops; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/02/2018 08:12 AM, Sekhar Nori wrote: > On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >> void __init da830_init_time(void) >> { >> +#ifdef CONFIG_COMMON_CLK >> + void __iomem *pll0, *psc0, *psc1; >> + struct clk *clk; >> + >> + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); >> + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); >> + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); >> + >> + da8xx_register_cfgchip(); >> + >> + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ); >> + >> + da830_pll_clk_init(pll0); >> + >> + da830_psc_clk_init(psc0, psc1); >> + > >> + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1); >> + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); >> + >> + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 1); >> + clk_register_clkdev(clk, "timer0", NULL); >> + >> + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 1); >> + clk_register_clkdev(clk, NULL, "davinci-wdt"); > > Isn't this better done in da830_pll_clk_init() ? I think we can get rid > of the dummy fixed factor clock too and directly use the pll0_auxclk. I considered it, but I kind of like keeping the fixed factor clocks for debugging purposes. If you just have "pll0_auxclk" the enable count is not helpful because you don't know which driver did the enabling. On the other hand, once things are working, you don't really need to do any debugging. > That reminds me, is "pll0_aux_clk" above correct, or should it be > "pll0_auxclk" like in da830_pll_clk_init()? Yes, it should be "pll0_auxclk". > >> + >> + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); >> + clk_register_clkdev(clk, "rmii", NULL); > > I don't see any driver looking for this clock using con_id "rmii". I > know this came from existing code. But its most likely a vestige and can > be dropped. > > Thanks, > Sekhar > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/02/2018 08:20 AM, Sekhar Nori wrote: > On Saturday 20 January 2018 10:44 PM, David Lechner wrote: >> void __init da850_init_time(void) >> { >> +#ifdef CONFIG_COMMON_CLK >> + void __iomem *pll0, *pll1, *psc0, *psc1; >> + struct regmap *cfgchip; >> + struct clk *clk; >> + struct clk_hw *parent; >> + >> + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); >> + pll1 = ioremap(DA850_PLL1_BASE, SZ_4K); >> + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); >> + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); >> + >> + cfgchip = da8xx_register_cfgchip(); >> + if (WARN(IS_ERR(cfgchip), "failed to register CFGCHIP syscon")) >> + return; >> + >> + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA850_REF_FREQ); >> + >> + da850_pll_clk_init(pll0, pll1); >> + >> + da8xx_cfgchip_register_div4p5(cfgchip); >> + >> + da8xx_cfgchip_register_async1(cfgchip); >> + >> + clk = clk_register_fixed_factor(NULL, "async2", "pll0_auxclk", 0, 1, 1); >> + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); >> + clk_register_clkdev(clk, "timer0", NULL); >> + clk_register_clkdev(clk, NULL, "davinci-wdt"); > > I think its better to get these clkdevs registered in > da850_pll_clk_init() itself. Sounds good to me. > >> + >> + clk = da8xx_cfgchip_register_async3(cfgchip); >> + >> + /* pll1_sysclk2 is not affected by CPU scaling, so use it for async3 */ >> + parent = clk_hw_get_parent_by_index(__clk_get_hw(clk), 1); >> + if (parent) >> + clk_set_parent(clk, parent->clk); >> + else >> + pr_warn("%s: Failed to find async3 parent clock\n", __func__); >> + >> + da850_psc_clk_init(psc0, psc1); >> + >> + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); >> + clk_register_clkdev(clk, "rmii", NULL); > > Like in da830, can drop this rmii clock, I think. > > Thanks, > Sekhar > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/02/2018 08:39 AM, Sekhar Nori wrote: > On Saturday 20 January 2018 10:44 PM, David Lechner wrote: >> This adds the new board-specific clock init in mach-davinci/dm644x.c >> using the new common clock framework drivers. >> >> The #ifdefs are needed to prevent compile errors until the entire >> ARCH_DAVINCI is converted. >> >> Also clean up the #includes since we are adding some here. >> >> Signed-off-by: David Lechner <david@lechnology.com> > > I did not notice this before, but the subject line for this and other > patches needs to be "ARM: davinci: " so "davinci" == "dm644x" and not any other SoC? > > Apart from that: > > Reviewed-by: Sekhar Nori <nsekhar@ti.com> > > Thanks, > Sekhar > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 02 February 2018 11:36 PM, David Lechner wrote: > On 02/02/2018 08:39 AM, Sekhar Nori wrote: >> On Saturday 20 January 2018 10:44 PM, David Lechner wrote: >>> This adds the new board-specific clock init in mach-davinci/dm644x.c >>> using the new common clock framework drivers. >>> >>> The #ifdefs are needed to prevent compile errors until the entire >>> ARCH_DAVINCI is converted. >>> >>> Also clean up the #includes since we are adding some here. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >> >> I did not notice this before, but the subject line for this and other >> patches needs to be "ARM: davinci: " > > so "davinci" == "dm644x" and not any other SoC? No, I meant "ARM: davinci: dm644x: " instead of just "ARM: dm644x". Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 02 February 2018 11:33 PM, David Lechner wrote: > On 02/02/2018 08:12 AM, Sekhar Nori wrote: >> On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >>> void __init da830_init_time(void) >>> { >>> +#ifdef CONFIG_COMMON_CLK >>> + void __iomem *pll0, *psc0, *psc1; >>> + struct clk *clk; >>> + >>> + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); >>> + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); >>> + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); >>> + >>> + da8xx_register_cfgchip(); >>> + >>> + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ); >>> + >>> + da830_pll_clk_init(pll0); >>> + >>> + da830_psc_clk_init(psc0, psc1); >>> + >> >>> + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, >>> 1, 1); >>> + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); >>> + >>> + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", >>> 0, 1, 1); >>> + clk_register_clkdev(clk, "timer0", NULL); >>> + >>> + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", >>> 0, 1, 1); >>> + clk_register_clkdev(clk, NULL, "davinci-wdt"); >> >> Isn't this better done in da830_pll_clk_init() ? I think we can get rid >> of the dummy fixed factor clock too and directly use the pll0_auxclk. > > > I considered it, but I kind of like keeping the fixed factor clocks for > debugging purposes. If you just have "pll0_auxclk" the enable count is > not helpful because you don't know which driver did the enabling. I think it is better to more or less reflect the hardware here. We would not be doing this in the DT case, for example. I see your point on debugging. Such code can perhaps be temporarily introduced if really debugging such an issue. This will be the case with any shared clock. Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-01-24 4:26 GMT+01:00 David Lechner <david@lechnology.com>: > On 01/20/2018 11:14 AM, David Lechner wrote: >> >> This removes all of the clock init code from da8xx-dt.c. This includes >> all of the OF_DEV_AUXDATA that was just used for looking up clocks. >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> >> v6 changes: >> - removed misleading statement from commit message >> >> arch/arm/mach-davinci/da8xx-dt.c | 61 >> +--------------------------------------- >> 1 file changed, 1 insertion(+), 60 deletions(-) >> >> diff --git a/arch/arm/mach-davinci/da8xx-dt.c >> b/arch/arm/mach-davinci/da8xx-dt.c > > >> static void __init da850_init_machine(void) >> { >> - /* All existing boards use 100MHz SATA refclkpn */ >> - static const unsigned long sata_refclkpn = 100 * 1000 * 1000; >> - >> - int ret; >> - >> - ret = da8xx_register_usb20_phy_clk(false); >> - if (ret) >> - pr_warn("%s: registering USB 2.0 PHY clock failed: %d", >> - __func__, ret); >> - ret = da8xx_register_usb11_phy_clk(false); >> - if (ret) >> - pr_warn("%s: registering USB 1.1 PHY clock failed: %d", >> - __func__, ret); >> - >> - ret = da850_register_sata_refclk(sata_refclkpn); >> - if (ret) >> - pr_warn("%s: registering SATA REFCLK failed: %d", >> - __func__, ret); >> - >> - of_platform_default_populate(NULL, da850_auxdata_lookup, NULL); >> + of_platform_default_populate(NULL, NULL, NULL); > > > of_platform_default_populate() can actually be removed completely. > of_platform_default_populate_init() is called implicitly during > arch_initcall_sync > >> davinci_pm_init(); >> pdata_quirks_init(); >> } Hi David, this patch breaks nand on da850-lcdk (and probably on da850-evm) since the nand driver can no longer clk_get() the nand clock. We would need to keep the dev_auxdata at least for aemif. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-02-05 15:04 GMT+01:00 Bartosz Golaszewski <brgl@bgdev.pl>: > 2018-01-24 4:26 GMT+01:00 David Lechner <david@lechnology.com>: >> On 01/20/2018 11:14 AM, David Lechner wrote: >>> >>> This removes all of the clock init code from da8xx-dt.c. This includes >>> all of the OF_DEV_AUXDATA that was just used for looking up clocks. >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >>> --- >>> >>> v6 changes: >>> - removed misleading statement from commit message >>> >>> arch/arm/mach-davinci/da8xx-dt.c | 61 >>> +--------------------------------------- >>> 1 file changed, 1 insertion(+), 60 deletions(-) >>> >>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c >>> b/arch/arm/mach-davinci/da8xx-dt.c >> >> >>> static void __init da850_init_machine(void) >>> { >>> - /* All existing boards use 100MHz SATA refclkpn */ >>> - static const unsigned long sata_refclkpn = 100 * 1000 * 1000; >>> - >>> - int ret; >>> - >>> - ret = da8xx_register_usb20_phy_clk(false); >>> - if (ret) >>> - pr_warn("%s: registering USB 2.0 PHY clock failed: %d", >>> - __func__, ret); >>> - ret = da8xx_register_usb11_phy_clk(false); >>> - if (ret) >>> - pr_warn("%s: registering USB 1.1 PHY clock failed: %d", >>> - __func__, ret); >>> - >>> - ret = da850_register_sata_refclk(sata_refclkpn); >>> - if (ret) >>> - pr_warn("%s: registering SATA REFCLK failed: %d", >>> - __func__, ret); >>> - >>> - of_platform_default_populate(NULL, da850_auxdata_lookup, NULL); >>> + of_platform_default_populate(NULL, NULL, NULL); >> >> >> of_platform_default_populate() can actually be removed completely. >> of_platform_default_populate_init() is called implicitly during >> arch_initcall_sync >> >>> davinci_pm_init(); >>> pdata_quirks_init(); >>> } > > Hi David, > > this patch breaks nand on da850-lcdk (and probably on da850-evm) since > the nand driver can no longer clk_get() the nand clock. We would need > to keep the dev_auxdata at least for aemif. > > Thanks, > Bartosz Actually scratch that last e-mail, I missed the fact that the nand node is only there in the board dts files. I've just sent you a patch that enables nand on da850-lcdk. Please include it in your series. I'll do the same for evm shortly. Thanks, Bartosz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This switches ARCH_DAVINCI to use the common clock framework. The legacy > clock code in arch/arm/mach-davinci/ is no longer used. New drivers in > drivers/clk/davinci/ are used instead. > > A few macros had to be moved to prevent compile errors. s/compile/compilation > Signed-off-by: David Lechner <david@lechnology.com> > --- a/arch/arm/mach-davinci/davinci.h > +++ b/arch/arm/mach-davinci/davinci.h > @@ -35,6 +35,10 @@ > #include <media/davinci/vpbe.h> > #include <media/davinci/vpbe_osd.h> > > +#define DAVINCI_PLL1_BASE 0x01c40800 > +#define DAVINCI_PLL2_BASE 0x01c40c00 > +#define DAVINCI_PWR_SLEEP_CNTRL_BASE 0x01c41000 Please drop the tab after #define With that. Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes the unused legacy clock init code from > arch/arm/mach-davinci/da830.c. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes the unused legacy clock init code from > arch/arm/mach-davinci/da850.c. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes the unused legacy clock init code from > arch/arm/mach-davinci/dm355.c. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes the unused legacy clock init code from > arch/arm/mach-davinci/dm365.c. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes the unused legacy clock init code from > arch/arm/mach-davinci/dm644x.c. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes the unused legacy clock init code from > arch/arm/mach-davinci/dm646x.c. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes the unused legacy clock init code from > arch/arm/mach-davinci/{devices,usb}-da8xx}.c. > > Signed-off-by: David Lechner <david@lechnology.com> Can you please mention SATA and USB in subject line and in patch description? Apart from that: Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > This removes the unused legacy clock code from arch/arm/mach-davinci/. > > Signed-off-by: David Lechner <david@lechnology.com> Reviewed-by: Sekhar Nori <nsekhar@ti.com> Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 January 2018 10:44 PM, David Lechner wrote: > diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi > index c66cf78..7f4acd7 100644 > --- a/arch/arm/boot/dts/da850.dtsi > +++ b/arch/arm/boot/dts/da850.dtsi > @@ -21,6 +21,36 @@ > #interrupt-cells = <1>; > ti,intc-size = <101>; > reg = <0xfffee000 0x2000>; > + clocks = <&psc0 6>; > + }; > + }; > + clocks: clocks { > + ref_clk: ref_clk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <24000000>; > + clock-output-names = "ref_clk"; > + }; The ref_clk clock-frequency should rather be added in each board file since its not fixed by the SoC. In general we dont want any SoC defined properties to be overwritten by board files (except the status = property). Thanks, Sekhar -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html