| Message ID | 1519071723-31790-1-git-send-email-david@lechnology.com |
|---|---|
| Headers | show |
| Series | ARM: davinci: convert to common clock framework | expand |
2018-02-19 21:21 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: davinci: pass clock as parameter to > davinci_timer_init()", 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. > > --- > > The change to make all of the clocks platform devices in v7 was a pretty > major change, so unfortunately I had to drop quite a few Reviewed-bys and > this series will need more close review and testing again. > > v7 changes (also see individual patches for details): > - Rebased on linux-davinci/master (v4.16-rc) > - Convert clock drivers to platform devices > - New patch "ARM: davinci: pass clock as parameter to davinci_timer_init()" > - Fix issues with lcdk and aemif clock lookups and power domains > - Fixed other minor issues brought up in v6 review > > 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: > > Most of the dependencies have landed in mainline already in v4.16-rc1 or > are in linux-davinci/master. > > There are no build dependencies any more, but you will need to pickup a > couple of patches to get CPU frequency scaling working because of a divider > clock bug. > > - https://patchwork.kernel.org/patch/10218941/ > - https://patchwork.kernel.org/patch/10218927/ > > You can find a working branch with everything included in the "common-clk-v7" > branch of https://github.com/dlech/ev3dev-kernel.git. > > > 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 (42): > 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: cfgchip: Add TI DA8XX USB PHY clocks > ARM: davinci: pass clock as parameter to davinci_timer_init() > ARM: davinci: da830: add new clock init using common clock framework > ARM: davinci: da850: add new clock init using common clock framework > ARM: davinci: dm355: add new clock init using common clock framework > ARM: davinci: dm365: add new clock init using common clock framework > ARM: davinci: dm644x: add new clock init using common clock framework > ARM: davinci: dm646x: add new clock init using common clock framework > ARM: davinci: da8xx: add new USB PHY clock init using common clock > framework > ARM: davinci: 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: davinci: da830: Remove legacy clock init > ARM: davinci: da850: Remove legacy clock init > ARM: davinci: dm355: Remove legacy clock init > ARM: davinci: dm365: Remove legacy clock init > ARM: davinci: dm644x: Remove legacy clock init > ARM: davinci: dm646x: Remove legacy clock init > ARM: davinci: da8xx: Remove legacy USB and SATA clock init > ARM: davinci: remove legacy clocks > ARM: davinci: add device tree support to timer > ARM: davinci: 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 | 71 ++ > MAINTAINERS | 7 + > arch/arm/Kconfig | 5 +- > arch/arm/boot/dts/da850-enbw-cmc.dts | 4 + > arch/arm/boot/dts/da850-evm.dts | 4 + > arch/arm/boot/dts/da850-lcdk.dts | 9 + > arch/arm/boot/dts/da850-lego-ev3.dts | 4 + > arch/arm/boot/dts/da850.dtsi | 159 ++++ > arch/arm/configs/davinci_all_defconfig | 1 - > arch/arm/mach-davinci/Kconfig | 13 +- > arch/arm/mach-davinci/Makefile | 4 +- > arch/arm/mach-davinci/board-da830-evm.c | 12 +- > arch/arm/mach-davinci/board-da850-evm.c | 2 + > arch/arm/mach-davinci/board-dm355-evm.c | 2 + > arch/arm/mach-davinci/board-dm355-leopard.c | 2 + > arch/arm/mach-davinci/board-dm365-evm.c | 2 + > arch/arm/mach-davinci/board-dm644x-evm.c | 2 + > arch/arm/mach-davinci/board-dm646x-evm.c | 2 + > arch/arm/mach-davinci/board-mityomapl138.c | 2 + > arch/arm/mach-davinci/board-neuros-osd2.c | 2 + > arch/arm/mach-davinci/board-omapl138-hawk.c | 11 +- > arch/arm/mach-davinci/board-sffsdr.c | 2 + > 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 | 469 ++--------- > arch/arm/mach-davinci/da850.c | 766 +++--------------- > arch/arm/mach-davinci/da8xx-dt.c | 60 -- > arch/arm/mach-davinci/davinci.h | 8 + > arch/arm/mach-davinci/devices-da8xx.c | 43 +- > arch/arm/mach-davinci/devices.c | 1 - > arch/arm/mach-davinci/dm355.c | 425 ++-------- > arch/arm/mach-davinci/dm365.c | 517 ++---------- > arch/arm/mach-davinci/dm644x.c | 363 ++------- > arch/arm/mach-davinci/dm646x.c | 391 ++------- > arch/arm/mach-davinci/include/mach/clock.h | 3 - > arch/arm/mach-davinci/include/mach/common.h | 11 +- > arch/arm/mach-davinci/include/mach/da8xx.h | 6 +- > arch/arm/mach-davinci/pm_domain.c | 5 + > arch/arm/mach-davinci/psc.c | 137 ---- > arch/arm/mach-davinci/psc.h | 12 - > arch/arm/mach-davinci/time.c | 46 +- > arch/arm/mach-davinci/usb-da8xx.c | 250 +----- > drivers/clk/Makefile | 1 + > drivers/clk/davinci/Makefile | 21 + > drivers/clk/davinci/da8xx-cfgchip.c | 786 ++++++++++++++++++ > drivers/clk/davinci/pll-da830.c | 58 ++ > drivers/clk/davinci/pll-da850.c | 178 +++++ > drivers/clk/davinci/pll-dm355.c | 75 ++ > drivers/clk/davinci/pll-dm365.c | 119 +++ > drivers/clk/davinci/pll-dm644x.c | 76 ++ > drivers/clk/davinci/pll-dm646x.c | 78 ++ > drivers/clk/davinci/pll.c | 877 +++++++++++++++++++++ > drivers/clk/davinci/pll.h | 141 ++++ > drivers/clk/davinci/psc-da830.c | 89 +++ > drivers/clk/davinci/psc-da850.c | 109 +++ > drivers/clk/davinci/psc-dm355.c | 72 ++ > drivers/clk/davinci/psc-dm365.c | 77 ++ > drivers/clk/davinci/psc-dm644x.c | 67 ++ > drivers/clk/davinci/psc-dm646x.c | 62 ++ > drivers/clk/davinci/psc.c | 505 ++++++++++++ > drivers/clk/davinci/psc.h | 101 +++ > include/linux/platform_data/clk-da8xx-cfgchip.h | 21 + > 65 files changed, 4515 insertions(+), 3846 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/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/platform_data/clk-da8xx-cfgchip.h > > -- > 2.7.4 > Hi David, just some quick results from today's playing with v7. I started out with da850-lcdk with my standard rootfs over NFS. I was not able to boot to console so far. The first problem is that mdio fails to probe: libphy: Fixed MDIO Bus: probed davinci_mdio 1e24000.mdio: davinci mdio revision 1.5, bus freq 2200000 davinci_mdio 1e24000.mdio: no live phy, scanning all davinci_mdio: probe of 1e24000.mdio failed with error -5 After some digging I noticed that the supplied clock rate differs between mainline (114000000Hz) vs common-clock-v7 (18000000). Since we're not setting the rate in mdio, using LPSC_SET_RATE_PARENT would not help like with lcdc. After that, the boot just hangs without ever getting to emac's probe. Once I set the emac clock to always enabled (a workaround that was necessary with v6, but could be dropped with my first genpd-in-a-separate-driver attempt), I'm getting a rather strange NULL pointer dereference: Backtrace: [<c049d464>] (strlen) from [<c01f32a8>] (start_creating+0x58/0xc0) [<c01f3250>] (start_creating) from [<c01f34c8>] (debugfs_create_dir+0x14/0xd8) r7:c04dadbc r6:c0567480 r5:c0656274 r4:c68a9300 [<c01f34b4>] (debugfs_create_dir) from [<c0296838>] (clk_debug_create_one+0x28/0x1d0) r5:c0656274 r4:c68a9300 [<c0296810>] (clk_debug_create_one) from [<c05cdf84>] (clk_debug_init+0x100/0x15c) r5:c0656274 r4:c68a9300 [<c05cde84>] (clk_debug_init) from [<c000a54c>] (do_one_initcall+0x50/0x19c) r7:c05e3834 r6:ffffe000 r5:c05f7008 r4:c05cde84 [<c000a4fc>] (do_one_initcall) from [<c05b6eb4>] (kernel_init_freeable+0x110/0x1cc) r9:c05b4584 r8:c05b6614 r7:c05e3834 r6:c063cc80 r5:00000073 r4:c05f2354 [<c05b6da4>] (kernel_init_freeable) from [<c04a2ce4>] (kernel_init+0x10/0xfc) r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04a2cd4 r4:00000000 [<c04a2cd4>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34) It turns out that the emac clock is correctly registered in __davinci_psc_register_clocks() and a corresponding clk_core structure is added to the list and core->name is correctly assigned, but then later when clk_debug_init() is called, the name in emac clk_core is NULL. This is the direct reason for the panic. Interestingly other members of clk_core seem to be fine. I'll continue on debugging it. Let me know if you have any ideas. Best regards, Bartosz Golaszewski -- 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/20/2018 07:33 AM, Bartosz Golaszewski wrote: > 2018-02-19 21:21 GMT+01:00 David Lechner <david@lechnology.com>: >> This series converts mach-davinci to use the common clock framework. >> >> > > Hi David, > > just some quick results from today's playing with v7. > > I started out with da850-lcdk with my standard rootfs over NFS. I was > not able to boot to console so far. The first problem is that mdio > fails to probe: > > libphy: Fixed MDIO Bus: probed > davinci_mdio 1e24000.mdio: davinci mdio revision 1.5, bus freq 2200000 > davinci_mdio 1e24000.mdio: no live phy, scanning all > davinci_mdio: probe of 1e24000.mdio failed with error -5> > After some digging I noticed that the supplied clock rate differs > between mainline (114000000Hz) vs common-clock-v7 (18000000). Since > we're not setting the rate in mdio, using LPSC_SET_RATE_PARENT would > not help like with lcdc. Can you post the output of this command so that I can see how your clocks are setup: cat /sys/kernel/debug/clk/clk_summary > > After that, the boot just hangs without ever getting to emac's probe. Using your workaround, can you run: cat /sys/kernel/debug/pm_genpd/pm_genpd_summary If you see: 1e27000.clock-controller: emac off-0 then genpd is not working like it is supposed to. You should see something like this for device that are working: 1e27000.clock-controller: uart2 on /devices/platform/soc@1c00000/1d0d000.serial active > > Once I set the emac clock to always enabled (a workaround that was > necessary with v6, but could be dropped with my first > genpd-in-a-separate-driver attempt), I'm getting a rather strange NULL > pointer dereference: I noticed this too when adding the power-domains property to some device tree nodes. This is part of the reason why I didn't add it everywhere. I wasn't able to figure out the cause of this yet. As a work around though, please try removing the power-domains property from the emac and mdio nodes and use your previous workaround of having an always enabled clock. > > Backtrace: > [<c049d464>] (strlen) from [<c01f32a8>] (start_creating+0x58/0xc0) > [<c01f3250>] (start_creating) from [<c01f34c8>] (debugfs_create_dir+0x14/0xd8) > r7:c04dadbc r6:c0567480 r5:c0656274 r4:c68a9300 > [<c01f34b4>] (debugfs_create_dir) from [<c0296838>] > (clk_debug_create_one+0x28/0x1d0) > r5:c0656274 r4:c68a9300 > [<c0296810>] (clk_debug_create_one) from [<c05cdf84>] > (clk_debug_init+0x100/0x15c) > r5:c0656274 r4:c68a9300 > [<c05cde84>] (clk_debug_init) from [<c000a54c>] (do_one_initcall+0x50/0x19c) > r7:c05e3834 r6:ffffe000 r5:c05f7008 r4:c05cde84 > [<c000a4fc>] (do_one_initcall) from [<c05b6eb4>] > (kernel_init_freeable+0x110/0x1cc) > r9:c05b4584 r8:c05b6614 r7:c05e3834 r6:c063cc80 r5:00000073 r4:c05f2354 > [<c05b6da4>] (kernel_init_freeable) from [<c04a2ce4>] (kernel_init+0x10/0xfc) > r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04a2cd4 > r4:00000000 > [<c04a2cd4>] (kernel_init) from [<c00090e0>] (ret_from_fork+0x14/0x34) > > It turns out that the emac clock is correctly registered in > __davinci_psc_register_clocks() and a corresponding clk_core structure > is added to the list and core->name is correctly assigned, but then > later when clk_debug_init() is called, the name in emac clk_core is > NULL. This is the direct reason for the panic. Interestingly other > members of clk_core seem to be fine. > > I'll continue on debugging it. Let me know if you have any ideas. > > Best regards, > Bartosz Golaszewski > -- 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-20 19:39 GMT+01:00 David Lechner <david@lechnology.com>: > On 02/20/2018 07:33 AM, Bartosz Golaszewski wrote: >> >> 2018-02-19 21:21 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>> This series converts mach-davinci to use the common clock framework. >>> >>> >> >> Hi David, >> >> just some quick results from today's playing with v7. >> >> I started out with da850-lcdk with my standard rootfs over NFS. I was >> not able to boot to console so far. The first problem is that mdio >> fails to probe: >> >> libphy: Fixed MDIO Bus: probed >> davinci_mdio 1e24000.mdio: davinci mdio revision 1.5, bus freq 2200000 >> davinci_mdio 1e24000.mdio: no live phy, scanning all >> davinci_mdio: probe of 1e24000.mdio failed with error -5> After some >> digging I noticed that the supplied clock rate differs >> between mainline (114000000Hz) vs common-clock-v7 (18000000). Since >> we're not setting the rate in mdio, using LPSC_SET_RATE_PARENT would >> not help like with lcdc. > > > Can you post the output of this command so that I can see how your > clocks are setup: > > cat /sys/kernel/debug/clk/clk_summary > > >> >> After that, the boot just hangs without ever getting to emac's probe. > > > Using your workaround, can you run: > > cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > > If you see: > 1e27000.clock-controller: emac off-0 > > then genpd is not working like it is supposed to. You should see something > like this for device that are working: > 1e27000.clock-controller: uart2 on > /devices/platform/soc@1c00000/1d0d000.serial active > > >> >> Once I set the emac clock to always enabled (a workaround that was >> necessary with v6, but could be dropped with my first >> genpd-in-a-separate-driver attempt), I'm getting a rather strange NULL >> pointer dereference: > > > I noticed this too when adding the power-domains property to some device > tree nodes. This is part of the reason why I didn't add it everywhere. > I wasn't able to figure out the cause of this yet. As a work around > though, please try removing the power-domains property from the emac > and mdio nodes and use your previous workaround of having an always > enabled clock. > > When I use any of the workarounds I just keep getting more problems (e.g. [1] from blk and pinctrl). I only had a couple hours today to play with it but it seems to me we have some memory corruption somewhere. I'll check the initialization order of all the frameworks involved tomorrow. Best regards, Bartosz [1] https://pastebin.com/75mkkuJL -- 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/21/2018 06:01 AM, Bartosz Golaszewski wrote: > 2018-02-20 19:39 GMT+01:00 David Lechner <david@lechnology.com>: >> On 02/20/2018 07:33 AM, Bartosz Golaszewski wrote: >>> >>> 2018-02-19 21:21 GMT+01:00 David Lechner <david@lechnology.com>: >>>> >>>> This series converts mach-davinci to use the common clock framework. >>>> >>>> >>> >>> Hi David, >>> >>> just some quick results from today's playing with v7. >>> >>> I started out with da850-lcdk with my standard rootfs over NFS. I was >>> not able to boot to console so far. The first problem is that mdio >>> fails to probe: >>> >>> libphy: Fixed MDIO Bus: probed >>> davinci_mdio 1e24000.mdio: davinci mdio revision 1.5, bus freq 2200000 >>> davinci_mdio 1e24000.mdio: no live phy, scanning all >>> davinci_mdio: probe of 1e24000.mdio failed with error -5> After some >>> digging I noticed that the supplied clock rate differs >>> between mainline (114000000Hz) vs common-clock-v7 (18000000). Since >>> we're not setting the rate in mdio, using LPSC_SET_RATE_PARENT would >>> not help like with lcdc. >> >> >> Can you post the output of this command so that I can see how your >> clocks are setup: >> >> cat /sys/kernel/debug/clk/clk_summary >> >> >>> >>> After that, the boot just hangs without ever getting to emac's probe. >> >> >> Using your workaround, can you run: >> >> cat /sys/kernel/debug/pm_genpd/pm_genpd_summary >> >> If you see: >> 1e27000.clock-controller: emac off-0 >> >> then genpd is not working like it is supposed to. You should see something >> like this for device that are working: >> 1e27000.clock-controller: uart2 on >> /devices/platform/soc@1c00000/1d0d000.serial active >> >> >>> >>> Once I set the emac clock to always enabled (a workaround that was >>> necessary with v6, but could be dropped with my first >>> genpd-in-a-separate-driver attempt), I'm getting a rather strange NULL >>> pointer dereference: >> >> >> I noticed this too when adding the power-domains property to some device >> tree nodes. This is part of the reason why I didn't add it everywhere. >> I wasn't able to figure out the cause of this yet. As a work around >> though, please try removing the power-domains property from the emac >> and mdio nodes and use your previous workaround of having an always >> enabled clock. >> >> > > When I use any of the workarounds I just keep getting more problems > (e.g. [1] from blk and pinctrl). I only had a couple hours today to > play with it but it seems to me we have some memory corruption > somewhere. I'll check the initialization order of all the frameworks > involved tomorrow. > > Best regards, > Bartosz > > [1] https://pastebin.com/75mkkuJL > I wonder if we need to delete all of the __init and __initconst attributes now that this has been converted to platform devices. -- 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-21 18:05 GMT+01:00 David Lechner <david@lechnology.com>: > On 02/21/2018 06:01 AM, Bartosz Golaszewski wrote: >> >> 2018-02-20 19:39 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>> On 02/20/2018 07:33 AM, Bartosz Golaszewski wrote: >>>> >>>> >>>> 2018-02-19 21:21 GMT+01:00 David Lechner <david@lechnology.com>: >>>>> >>>>> >>>>> This series converts mach-davinci to use the common clock framework. >>>>> >>>>> >>>> >>>> Hi David, >>>> >>>> just some quick results from today's playing with v7. >>>> >>>> I started out with da850-lcdk with my standard rootfs over NFS. I was >>>> not able to boot to console so far. The first problem is that mdio >>>> fails to probe: >>>> >>>> libphy: Fixed MDIO Bus: probed >>>> davinci_mdio 1e24000.mdio: davinci mdio revision 1.5, bus freq 2200000 >>>> davinci_mdio 1e24000.mdio: no live phy, scanning all >>>> davinci_mdio: probe of 1e24000.mdio failed with error -5> After some >>>> digging I noticed that the supplied clock rate differs >>>> between mainline (114000000Hz) vs common-clock-v7 (18000000). Since >>>> we're not setting the rate in mdio, using LPSC_SET_RATE_PARENT would >>>> not help like with lcdc. >>> >>> >>> >>> Can you post the output of this command so that I can see how your >>> clocks are setup: >>> >>> cat /sys/kernel/debug/clk/clk_summary >>> >>> >>>> >>>> After that, the boot just hangs without ever getting to emac's probe. >>> >>> >>> >>> Using your workaround, can you run: >>> >>> cat /sys/kernel/debug/pm_genpd/pm_genpd_summary >>> >>> If you see: >>> 1e27000.clock-controller: emac off-0 >>> >>> then genpd is not working like it is supposed to. You should see >>> something >>> like this for device that are working: >>> 1e27000.clock-controller: uart2 on >>> /devices/platform/soc@1c00000/1d0d000.serial active >>> >>> >>>> >>>> Once I set the emac clock to always enabled (a workaround that was >>>> necessary with v6, but could be dropped with my first >>>> genpd-in-a-separate-driver attempt), I'm getting a rather strange NULL >>>> pointer dereference: >>> >>> >>> >>> I noticed this too when adding the power-domains property to some device >>> tree nodes. This is part of the reason why I didn't add it everywhere. >>> I wasn't able to figure out the cause of this yet. As a work around >>> though, please try removing the power-domains property from the emac >>> and mdio nodes and use your previous workaround of having an always >>> enabled clock. >>> >>> >> >> When I use any of the workarounds I just keep getting more problems >> (e.g. [1] from blk and pinctrl). I only had a couple hours today to >> play with it but it seems to me we have some memory corruption >> somewhere. I'll check the initialization order of all the frameworks >> involved tomorrow. >> >> Best regards, >> Bartosz >> >> [1] https://pastebin.com/75mkkuJL >> > > I wonder if we need to delete all of the __init and __initconst attributes > now that this has been converted to platform devices. > Duh... Of course we do. IIRC everything in the init section gets removed after late_initcall. I'll test that tomorrow. Bart -- 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-21 18:46 GMT+01:00 Bartosz Golaszewski <brgl@bgdev.pl>: > 2018-02-21 18:05 GMT+01:00 David Lechner <david@lechnology.com>: >> On 02/21/2018 06:01 AM, Bartosz Golaszewski wrote: >>> >>> 2018-02-20 19:39 GMT+01:00 David Lechner <david@lechnology.com>: >>>> >>>> On 02/20/2018 07:33 AM, Bartosz Golaszewski wrote: >>>>> >>>>> >>>>> 2018-02-19 21:21 GMT+01:00 David Lechner <david@lechnology.com>: >>>>>> >>>>>> >>>>>> This series converts mach-davinci to use the common clock framework. >>>>>> >>>>>> >>>>> >>>>> Hi David, >>>>> >>>>> just some quick results from today's playing with v7. >>>>> >>>>> I started out with da850-lcdk with my standard rootfs over NFS. I was >>>>> not able to boot to console so far. The first problem is that mdio >>>>> fails to probe: >>>>> >>>>> libphy: Fixed MDIO Bus: probed >>>>> davinci_mdio 1e24000.mdio: davinci mdio revision 1.5, bus freq 2200000 >>>>> davinci_mdio 1e24000.mdio: no live phy, scanning all >>>>> davinci_mdio: probe of 1e24000.mdio failed with error -5> After some >>>>> digging I noticed that the supplied clock rate differs >>>>> between mainline (114000000Hz) vs common-clock-v7 (18000000). Since >>>>> we're not setting the rate in mdio, using LPSC_SET_RATE_PARENT would >>>>> not help like with lcdc. >>>> >>>> >>>> >>>> Can you post the output of this command so that I can see how your >>>> clocks are setup: >>>> >>>> cat /sys/kernel/debug/clk/clk_summary >>>> >>>> >>>>> >>>>> After that, the boot just hangs without ever getting to emac's probe. >>>> >>>> >>>> >>>> Using your workaround, can you run: >>>> >>>> cat /sys/kernel/debug/pm_genpd/pm_genpd_summary >>>> >>>> If you see: >>>> 1e27000.clock-controller: emac off-0 >>>> >>>> then genpd is not working like it is supposed to. You should see >>>> something >>>> like this for device that are working: >>>> 1e27000.clock-controller: uart2 on >>>> /devices/platform/soc@1c00000/1d0d000.serial active >>>> >>>> >>>>> >>>>> Once I set the emac clock to always enabled (a workaround that was >>>>> necessary with v6, but could be dropped with my first >>>>> genpd-in-a-separate-driver attempt), I'm getting a rather strange NULL >>>>> pointer dereference: >>>> >>>> >>>> >>>> I noticed this too when adding the power-domains property to some device >>>> tree nodes. This is part of the reason why I didn't add it everywhere. >>>> I wasn't able to figure out the cause of this yet. As a work around >>>> though, please try removing the power-domains property from the emac >>>> and mdio nodes and use your previous workaround of having an always >>>> enabled clock. >>>> >>>> >>> >>> When I use any of the workarounds I just keep getting more problems >>> (e.g. [1] from blk and pinctrl). I only had a couple hours today to >>> play with it but it seems to me we have some memory corruption >>> somewhere. I'll check the initialization order of all the frameworks >>> involved tomorrow. >>> >>> Best regards, >>> Bartosz >>> >>> [1] https://pastebin.com/75mkkuJL >>> >> >> I wonder if we need to delete all of the __init and __initconst attributes >> now that this has been converted to platform devices. >> > > Duh... Of course we do. IIRC everything in the init section gets > removed after late_initcall. I'll test that tomorrow. > > Bart It's not that. Not only anyway as it doesn't fix anything, I'm getting the exact same panics. I tried to play with various settings, like initializing the platform driver later in the boot sequence etc. but haven't yet figured out what's going on. Bart -- 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-19 21:21 GMT+01:00 David Lechner <david@lechnology.com>: > 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> > --- > > v7 changes: > - convert to platform device > - rename lpsc field to md > - rename PSC to LPSC where appropriate > - rename LPSC_ARM_RATE to LPSC_SET_RATE_PARENT > - add genpd provider > - add reset provider > > v6 changes: > - use GENMASK > - add quirk flag for FORCE bit > - add quirk flag for propagating set_rate > - fix writing to PDSTAT instead of PDCTL > - remove unused doc comment parameter > - change davinci_psc_register_clocks() to handle registering clkdev entries > > > drivers/clk/davinci/Makefile | 2 + > drivers/clk/davinci/psc.c | 495 +++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/davinci/psc.h | 89 ++++++++ > 3 files changed, 586 insertions(+) > create mode 100644 drivers/clk/davinci/psc.c > create mode 100644 drivers/clk/davinci/psc.h > > diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile > index d471386..cd1bf2c 100644 > --- a/drivers/clk/davinci/Makefile > +++ b/drivers/clk/davinci/Makefile > @@ -8,4 +8,6 @@ obj-$(CONFIG_ARCH_DAVINCI_DM355) += pll-dm355.o > obj-$(CONFIG_ARCH_DAVINCI_DM365) += pll-dm365.o > obj-$(CONFIG_ARCH_DAVINCI_DM644x) += pll-dm644x.o > obj-$(CONFIG_ARCH_DAVINCI_DM646x) += pll-dm646x.o > + > +obj-y += psc.o > endif > diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c > new file mode 100644 > index 0000000..8e08ac8 > --- /dev/null > +++ b/drivers/clk/davinci/psc.c > @@ -0,0 +1,495 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Clock driver for TI Davinci PSC controllers > + * > + * Copyright (C) 2017 David Lechner <david@lechnology.com> > + * > + * Based on: drivers/clk/keystone/gate.c > + * Copyright (C) 2013 Texas Instruments. > + * Murali Karicheri <m-karicheri2@ti.com> > + * Santosh Shilimkar <santosh.shilimkar@ti.com> > + * > + * And: arch/arm/mach-davinci/psc.c > + * Copyright (C) 2006 Texas Instruments. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/clk.h> > +#include <linux/clkdev.h> > +#include <linux/err.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_clock.h> > +#include <linux/pm_domain.h> > +#include <linux/regmap.h> > +#include <linux/reset-controller.h> > +#include <linux/slab.h> > +#include <linux/types.h> > + > +#include "psc.h" > + > +/* PSC register offsets */ > +#define EPCPR 0x070 > +#define PTCMD 0x120 > +#define PTSTAT 0x128 > +#define PDSTAT(n) (0x200 + 4 * (n)) > +#define PDCTL(n) (0x300 + 4 * (n)) > +#define MDSTAT(n) (0x800 + 4 * (n)) > +#define MDCTL(n) (0xa00 + 4 * (n)) > + > +/* PSC module states */ > +enum davinci_lpsc_state { > + LPSC_STATE_SWRSTDISABLE = 0, > + LPSC_STATE_SYNCRST = 1, > + LPSC_STATE_DISABLE = 2, > + LPSC_STATE_ENABLE = 3, > +}; > + > +#define MDSTAT_STATE_MASK GENMASK(5, 0) > +#define MDSTAT_MCKOUT BIT(12) > +#define PDSTAT_STATE_MASK GENMASK(4, 0) > +#define MDCTL_FORCE BIT(31) > +#define MDCTL_LRESET BIT(8) > +#define PDCTL_EPCGOOD BIT(8) > +#define PDCTL_NEXT BIT(0) > + > +struct davinci_psc_data { > + struct clk_onecell_data clk_data; > + struct genpd_onecell_data pm_data; > + struct reset_controller_dev rcdev; > +}; > + > +/** > + * struct davinci_lpsc_clk - LPSC clock structure > + * @hw: clk_hw for the LPSC > + * @pm_domain: power domain for the LPSC > + * @regmap: PSC MMIO region > + * @md: Module domain (LPSC module id) > + * @pd: Power domain > + * @flags: LPSC_* quirk flags > + */ > +struct davinci_lpsc_clk { > + struct clk_hw hw; > + struct generic_pm_domain pm_domain; > + struct regmap *regmap; > + u32 md; > + u32 pd; > + u32 flags; > +}; > + > +#define to_davinci_psc_data(x) container_of(x, struct davinci_psc_data, x) > +#define to_davinci_lpsc_clk(x) container_of(x, struct davinci_lpsc_clk, x) > + > +static void davinci_lpsc_config(struct davinci_lpsc_clk *lpsc, > + enum davinci_lpsc_state next_state) > +{ > + u32 epcpr, pdstat, mdstat, ptstat; > + > + regmap_write_bits(lpsc->regmap, MDCTL(lpsc->md), MDSTAT_STATE_MASK, > + next_state); > + > + if (lpsc->flags & LPSC_FORCE) > + regmap_write_bits(lpsc->regmap, MDCTL(lpsc->md), MDCTL_FORCE, > + MDCTL_FORCE); > + > + regmap_read(lpsc->regmap, PDSTAT(lpsc->pd), &pdstat); > + if ((pdstat & PDSTAT_STATE_MASK) == 0) { > + regmap_write_bits(lpsc->regmap, PDCTL(lpsc->pd), PDCTL_NEXT, > + PDCTL_NEXT); > + > + regmap_write(lpsc->regmap, PTCMD, BIT(lpsc->pd)); > + > + regmap_read_poll_timeout(lpsc->regmap, EPCPR, epcpr, > + epcpr & BIT(lpsc->pd), 0, 0); > + > + regmap_write_bits(lpsc->regmap, PDCTL(lpsc->pd), PDCTL_EPCGOOD, > + PDCTL_EPCGOOD); > + } else { > + regmap_write(lpsc->regmap, PTCMD, BIT(lpsc->pd)); > + } > + > + regmap_read_poll_timeout(lpsc->regmap, PTSTAT, ptstat, > + !(ptstat & BIT(lpsc->pd)), 0, 0); > + > + regmap_read_poll_timeout(lpsc->regmap, MDSTAT(lpsc->md), mdstat, > + (mdstat & MDSTAT_STATE_MASK) == next_state, > + 0, 0); > +} > + > +static int davinci_lpsc_clk_enable(struct clk_hw *hw) > +{ > + struct davinci_lpsc_clk *lpsc = to_davinci_lpsc_clk(hw); > + > + davinci_lpsc_config(lpsc, LPSC_STATE_ENABLE); > + > + return 0; > +} > + > +static void davinci_lpsc_clk_disable(struct clk_hw *hw) > +{ > + struct davinci_lpsc_clk *lpsc = to_davinci_lpsc_clk(hw); > + > + davinci_lpsc_config(lpsc, LPSC_STATE_DISABLE); > +} > + > +static int davinci_lpsc_clk_is_enabled(struct clk_hw *hw) > +{ > + struct davinci_lpsc_clk *lpsc = to_davinci_lpsc_clk(hw); > + u32 mdstat; > + > + regmap_read(lpsc->regmap, MDSTAT(lpsc->md), &mdstat); > + > + return (mdstat & MDSTAT_MCKOUT) ? 1 : 0; > +} > + > +static const struct clk_ops davinci_lpsc_clk_ops = { > + .enable = davinci_lpsc_clk_enable, > + .disable = davinci_lpsc_clk_disable, > + .is_enabled = davinci_lpsc_clk_is_enabled, > +}; > + > +static int davinci_psc_genpd_attach_dev(struct generic_pm_domain *pm_domain, > + struct device *dev) > +{ > + struct davinci_lpsc_clk *lpsc = to_davinci_lpsc_clk(pm_domain); > + struct clk *clk = lpsc->hw.clk; > + int ret; > + > + ret = pm_clk_create(dev); > + if (ret < 0) > + return ret; > + Hi David, I think I found the reason for the strange crashes we were experiencing (emac core->name being NULL) thanks to Sekhar who pointed me in the right direction. The mdio driver fails to probe with v7 due to the supplied clock rate being wrong. Before failing we register the emac clock with pm_clk_add_clk(). When clock_ops puts the clock, it decreases the reference count of the clock, but we never actually increased it in the first place in the line above. The core clock code then destroys the associated clk_core structure. When the next user comes around (in our case the clk debug functions) the system crashes. I believe there to be two issues: one is with v7 - we need to increase the clock reference count in davinci_psc_genpd_attach_dev(). Second is the error path in the clock framework - we should remove the destroyed clk_core from the debug list, which is not being done now. Why we even need to track the refcount of clk_core is a mistery for me though. Stephen, Mike? Best regards, Bartosz Golaszewski > + ret = pm_clk_add_clk(dev, clk); > + if (ret < 0) { > + pm_clk_destroy(dev); > + return ret; > + } > + > + return 0; > +} > + > +static void davinci_psc_genpd_detach_dev(struct generic_pm_domain *pm_domain, > + struct device *dev) > +{ > + struct davinci_lpsc_clk *lpsc = to_davinci_lpsc_clk(pm_domain); > + struct clk *clk = lpsc->hw.clk; > + > + pm_clk_remove_clk(dev, clk); > + pm_clk_destroy(dev); > +} > + > +/** > + * davinci_lpsc_clk_register - register LPSC clock > + * @name: name of this clock > + * @parent_name: name of clock's parent > + * @regmap: PSC MMIO region > + * @md: local PSC number > + * @pd: power domain > + * @flags: LPSC_* flags > + */ > +static struct davinci_lpsc_clk * > +davinci_lpsc_clk_register(struct device *dev, const char *name, > + const char *parent_name, struct regmap *regmap, > + u32 md, u32 pd, u32 flags) > +{ > + struct clk_init_data init; > + struct davinci_lpsc_clk *lpsc; > + int ret; > + bool is_on; > + > + lpsc = devm_kzalloc(dev, sizeof(*lpsc), GFP_KERNEL); > + if (!lpsc) > + return ERR_PTR(-ENOMEM); > + > + init.name = name; > + init.ops = &davinci_lpsc_clk_ops; > + init.parent_names = (parent_name ? &parent_name : NULL); > + init.num_parents = (parent_name ? 1 : 0); > + init.flags = 0; > + > + if (flags & LPSC_ALWAYS_ENABLED) > + init.flags |= CLK_IS_CRITICAL; > + > + if (flags & LPSC_SET_RATE_PARENT) > + init.flags |= CLK_SET_RATE_PARENT; > + > + lpsc->regmap = regmap; > + lpsc->hw.init = &init; > + lpsc->md = md; > + lpsc->pd = pd; > + lpsc->flags = flags; > + > + ret = devm_clk_hw_register(dev, &lpsc->hw); > + if (ret < 0) > + return ERR_PTR(ret); > + > + lpsc->pm_domain.name = devm_kasprintf(dev, GFP_KERNEL, "%s: %s", > + dev_name(dev), name); > + lpsc->pm_domain.attach_dev = davinci_psc_genpd_attach_dev; > + lpsc->pm_domain.detach_dev = davinci_psc_genpd_detach_dev; > + lpsc->pm_domain.flags = GENPD_FLAG_PM_CLK; > + > + is_on = davinci_lpsc_clk_is_enabled(&lpsc->hw); > + pm_genpd_init(&lpsc->pm_domain, NULL, is_on); > + > + return lpsc; > +} > + > +static int davinci_lpsc_clk_reset(struct clk *clk, bool reset) > +{ > + struct clk_hw *hw = __clk_get_hw(clk); > + struct davinci_lpsc_clk *lpsc = to_davinci_lpsc_clk(hw); > + u32 mdctl; > + > + if (IS_ERR_OR_NULL(lpsc)) > + return -EINVAL; > + > + mdctl = reset ? 0 : MDCTL_LRESET; > + regmap_write_bits(lpsc->regmap, MDCTL(lpsc->md), MDCTL_LRESET, mdctl); > + > + return 0; > +} > + > +/* > + * REVISIT: These exported functions can be removed after a non-DT lookup is > + * added to the reset controller framework and the davinci-rproc driver is > + * updated to use the generic reset controller framework. > + */ > + > +int davinci_clk_reset_assert(struct clk *clk) > +{ > + return davinci_lpsc_clk_reset(clk, true); > +} > +EXPORT_SYMBOL(davinci_clk_reset_assert); > + > +int davinci_clk_reset_deassert(struct clk *clk) > +{ > + return davinci_lpsc_clk_reset(clk, false); > +} > +EXPORT_SYMBOL(davinci_clk_reset_deassert); > + > +static int davinci_psc_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct davinci_psc_data *psc = to_davinci_psc_data(rcdev); > + struct clk *clk = psc->clk_data.clks[id]; > + > + return davinci_lpsc_clk_reset(clk, true); > +} > + > +static int davinci_psc_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct davinci_psc_data *psc = to_davinci_psc_data(rcdev); > + struct clk *clk = psc->clk_data.clks[id]; > + > + return davinci_lpsc_clk_reset(clk, false); > +} > + > +static const struct reset_control_ops davinci_psc_reset_ops = { > + .assert = davinci_psc_reset_assert, > + .deassert = davinci_psc_reset_deassert, > +}; > + > +static int davinci_psc_reset_of_xlate(struct reset_controller_dev *rcdev, > + const struct of_phandle_args *reset_spec) > +{ > + struct of_phandle_args clkspec = *reset_spec; /* discard const qualifier */ > + struct clk *clk; > + struct clk_hw *hw; > + struct davinci_lpsc_clk *lpsc; > + > + /* the clock node is the same as the reset node */ > + clk = of_clk_get_from_provider(&clkspec); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + hw = __clk_get_hw(clk); > + lpsc = to_davinci_lpsc_clk(hw); > + clk_put(clk); > + > + /* not all modules support local reset */ > + if (!(lpsc->flags & LPSC_LOCAL_RESET)) > + return -EINVAL; > + > + return lpsc->md; > +} > + > +static const struct regmap_config davinci_psc_regmap_config = { > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > +}; > + > +static struct davinci_psc_data * > +__davinci_psc_register_clocks(struct device *dev, > + const struct davinci_lpsc_clk_info *info, > + int num_clks, > + void __iomem *base) > +{ > + struct davinci_psc_data *psc; > + struct clk **clks; > + struct generic_pm_domain **pm_domains; > + struct regmap *regmap; > + int i, ret; > + > + psc = devm_kzalloc(dev, sizeof(*psc), GFP_KERNEL); > + if (!psc) > + return ERR_PTR(-ENOMEM); > + > + clks = devm_kmalloc_array(dev, num_clks, sizeof(*clks), GFP_KERNEL); > + if (!clks) > + return ERR_PTR(-ENOMEM); > + > + psc->clk_data.clks = clks; > + psc->clk_data.clk_num = num_clks; > + > + /* > + * init array with error so that of_clk_src_onecell_get() doesn't > + * return NULL for gaps in the sparse array > + */ > + for (i = 0; i < num_clks; i++) > + clks[i] = ERR_PTR(-ENOENT); > + > + pm_domains = devm_kcalloc(dev, num_clks, sizeof(*pm_domains), GFP_KERNEL); > + if (!pm_domains) > + return ERR_PTR(-ENOMEM); > + > + psc->pm_data.domains = pm_domains; > + psc->pm_data.num_domains = num_clks; > + > + regmap = devm_regmap_init_mmio(dev, base, &davinci_psc_regmap_config); > + if (IS_ERR(regmap)) > + return ERR_CAST(regmap); > + > + for (; info->name; info++) { > + struct davinci_lpsc_clk *lpsc; > + > + lpsc = davinci_lpsc_clk_register(dev, info->name, info->parent, > + regmap, info->md, info->pd, > + info->flags); > + if (IS_ERR(lpsc)) { > + dev_warn(dev, "Failed to register %s (%ld)\n", > + info->name, PTR_ERR(lpsc)); > + continue; > + } > + > + clks[info->md] = lpsc->hw.clk; > + pm_domains[info->md] = &lpsc->pm_domain; > + } > + > + psc->rcdev.ops = &davinci_psc_reset_ops; > + psc->rcdev.owner = THIS_MODULE; > + psc->rcdev.of_node = dev->of_node; > + psc->rcdev.of_reset_n_cells = 1; > + psc->rcdev.of_xlate = davinci_psc_reset_of_xlate; > + psc->rcdev.nr_resets = num_clks; > + > + ret = devm_reset_controller_register(dev, &psc->rcdev); > + if (ret < 0) > + dev_warn(dev, "Failed to register reset controller (%d)\n", ret); > + > + return psc; > +} > + > +int davinci_psc_register_clocks(struct device *dev, > + const struct davinci_lpsc_clk_info *info, > + u8 num_clks, > + void __iomem *base) > +{ > + struct davinci_psc_data *psc; > + > + psc = __davinci_psc_register_clocks(dev, info, num_clks, base); > + if (IS_ERR(psc)) > + return PTR_ERR(psc); > + > + for (; info->name; info++) { > + const struct davinci_lpsc_clkdev_info *cdevs = info->cdevs; > + struct clk *clk = psc->clk_data.clks[info->md]; > + > + if (!cdevs || IS_ERR_OR_NULL(clk)) > + continue; > + > + for (; cdevs->con_id || cdevs->dev_id; cdevs++) > + clk_register_clkdev(clk, cdevs->con_id, cdevs->dev_id); > + } > + > + return 0; > +} > + > +int of_davinci_psc_clk_init(struct device *dev, > + const struct davinci_lpsc_clk_info *info, > + u8 num_clks, > + void __iomem *base) > +{ > + struct device_node *node = dev->of_node; > + struct davinci_psc_data *psc; > + > + psc = __davinci_psc_register_clocks(dev, info, num_clks, base); > + if (IS_ERR(psc)) > + return PTR_ERR(psc); > + > + of_genpd_add_provider_onecell(node, &psc->pm_data); > + > + of_clk_add_provider(node, of_clk_src_onecell_get, &psc->clk_data); > + > + return 0; > +} > + > +static const struct of_device_id davinci_psc_of_match[] = { > + { } > +}; > + > +static const struct platform_device_id davinci_psc_id_table[] = { > + { } > +}; > + > +typedef int (*davinci_psc_init)(struct device *dev, void __iomem *base); > + > +static int davinci_psc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + const struct of_device_id *of_id; > + davinci_psc_init psc_init = NULL; > + struct resource *res; > + void __iomem *base; > + > + of_id = of_match_device(davinci_psc_of_match, dev); > + if (of_id) > + psc_init = of_id->data; > + else if (pdev->id_entry) > + psc_init = (void *)pdev->id_entry->driver_data; > + > + if (!psc_init) { > + dev_err(dev, "unable to find driver data\n"); > + return -EINVAL; > + } > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) { > + dev_err(dev, "ioremap failed\n"); > + return PTR_ERR(base); > + } > + > + return psc_init(dev, base); > +} > + > +static struct platform_driver davinci_psc_driver = { > + .probe = davinci_psc_probe, > + .driver = { > + .name = "davinci-psc-clk", > + .of_match_table = davinci_psc_of_match, > + }, > + .id_table = davinci_psc_id_table, > +}; > + > +static int __init davinci_psc_driver_init(void) > +{ > + return platform_driver_register(&davinci_psc_driver); > +} > + > +/* has to be postcore_initcall because davinci_gpio depend on PSC clocks */ > +postcore_initcall(davinci_psc_driver_init); > diff --git a/drivers/clk/davinci/psc.h b/drivers/clk/davinci/psc.h > new file mode 100644 > index 0000000..c342d54 > --- /dev/null > +++ b/drivers/clk/davinci/psc.h > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Clock driver for TI Davinci PSC controllers > + * > + * Copyright (C) 2018 David Lechner <david@lechnology.com> > + */ > + > +#ifndef __CLK_DAVINCI_PSC_H__ > +#define __CLK_DAVINCI_PSC_H__ > + > +#include <linux/clk-provider.h> > +#include <linux/types.h> > + > +/* PSC quirk flags */ > +#define LPSC_ALWAYS_ENABLED BIT(0) /* never disable this clock */ > +#define LPSC_SET_RATE_PARENT BIT(1) /* propagate set_rate to parent clock */ > +#define LPSC_FORCE BIT(2) /* requires MDCTL FORCE bit */ > +#define LPSC_LOCAL_RESET BIT(3) /* acts as reset provider */ > + > +struct davinci_lpsc_clkdev_info { > + const char *con_id; > + const char *dev_id; > +}; > + > +#define LPSC_CLKDEV(c, d) { \ > + .con_id = (c), \ > + .dev_id = (d) \ > +} > + > +#define LPSC_CLKDEV1(n, c, d) \ > +static const struct davinci_lpsc_clkdev_info n[] __initconst = { \ > + LPSC_CLKDEV((c), (d)), \ > + { } \ > +} > + > +#define LPSC_CLKDEV2(n, c1, d1, c2, d2) \ > +static const struct davinci_lpsc_clkdev_info n[] __initconst = { \ > + LPSC_CLKDEV((c1), (d1)), \ > + LPSC_CLKDEV((c2), (d2)), \ > + { } \ > +} > + > +#define LPSC_CLKDEV3(n, c1, d1, c2, d2, c3, d3) \ > +static const struct davinci_lpsc_clkdev_info n[] __initconst = { \ > + LPSC_CLKDEV((c1), (d1)), \ > + LPSC_CLKDEV((c2), (d2)), \ > + LPSC_CLKDEV((c3), (d3)), \ > + { } \ > +} > + > +/** > + * davinci_lpsc_clk_info - LPSC module-specific clock information > + * @name: the clock name > + * @parent: the parent clock name > + * @cdevs: optional array of clkdev lookup table info > + * @md: the local module domain (LPSC id) > + * @pd: the power domain id > + * @flags: bitmask of LPSC_* flags > + */ > +struct davinci_lpsc_clk_info { > + const char *name; > + const char *parent; > + const struct davinci_lpsc_clkdev_info *cdevs; > + u32 md; > + u32 pd; > + unsigned long flags; > +}; > + > +#define LPSC(m, d, n, p, c, f) \ > +{ \ > + .name = #n, \ > + .parent = #p, \ > + .cdevs = (c), \ > + .md = (m), \ > + .pd = (d), \ > + .flags = (f), \ > +} > + > +int davinci_psc_register_clocks(struct device *dev, > + const struct davinci_lpsc_clk_info *info, > + u8 num_clks, > + void __iomem *base); > + > +int of_davinci_psc_clk_init(struct device *dev, > + const struct davinci_lpsc_clk_info *info, > + u8 num_clks, > + void __iomem *base); > + > +#endif /* __CLK_DAVINCI_PSC_H__ */ > -- > 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
On 02/28/2018 06:38 AM, Bartosz Golaszewski wrote: > > I think I found the reason for the strange crashes we were > experiencing (emac core->name being NULL) thanks to Sekhar who pointed > me in the right direction. > > The mdio driver fails to probe with v7 due to the supplied clock rate > being wrong. Before failing we register the emac clock with > pm_clk_add_clk(). When clock_ops puts the clock, it decreases the > reference count of the clock, but we never actually increased it in > the first place in the line above. The core clock code then destroys > the associated clk_core structure. When the next user comes around (in > our case the clk debug functions) the system crashes. > > I believe there to be two issues: one is with v7 - we need to increase > the clock reference count in davinci_psc_genpd_attach_dev(). > > Second is the error path in the clock framework - we should remove the > destroyed clk_core from the debug list, which is not being done now. > > Why we even need to track the refcount of clk_core is a mistery for me > though. Stephen, Mike? > > Best regards, > Bartosz Golaszewski Great find. I figured it had to be something like this, but I wasn't able to reproduce the problem yet. I suppose it is time to spin up a v8 with some fixes. -- 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-28 22:40 GMT+01:00 David Lechner <david@lechnology.com>: > On 02/28/2018 06:38 AM, Bartosz Golaszewski wrote: >> >> >> I think I found the reason for the strange crashes we were >> experiencing (emac core->name being NULL) thanks to Sekhar who pointed >> me in the right direction. >> >> The mdio driver fails to probe with v7 due to the supplied clock rate >> being wrong. Before failing we register the emac clock with >> pm_clk_add_clk(). When clock_ops puts the clock, it decreases the >> reference count of the clock, but we never actually increased it in >> the first place in the line above. The core clock code then destroys >> the associated clk_core structure. When the next user comes around (in >> our case the clk debug functions) the system crashes. >> >> I believe there to be two issues: one is with v7 - we need to increase >> the clock reference count in davinci_psc_genpd_attach_dev(). >> >> Second is the error path in the clock framework - we should remove the >> destroyed clk_core from the debug list, which is not being done now. >> >> Why we even need to track the refcount of clk_core is a mistery for me >> though. Stephen, Mike? >> >> Best regards, >> Bartosz Golaszewski > > > Great find. I figured it had to be something like this, but I wasn't > able to reproduce the problem yet. > > I suppose it is time to spin up a v8 with some fixes. I still don't know why the mdio clock rate is much lower than in mainline though. Any ideas? Thanks, Bart -- 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 03/01/2018 02:36 AM, Bartosz Golaszewski wrote: > 2018-02-28 22:40 GMT+01:00 David Lechner <david@lechnology.com>: >> On 02/28/2018 06:38 AM, Bartosz Golaszewski wrote: >>> >>> >>> I think I found the reason for the strange crashes we were >>> experiencing (emac core->name being NULL) thanks to Sekhar who pointed >>> me in the right direction. >>> >>> The mdio driver fails to probe with v7 due to the supplied clock rate >>> being wrong. Before failing we register the emac clock with >>> pm_clk_add_clk(). When clock_ops puts the clock, it decreases the >>> reference count of the clock, but we never actually increased it in >>> the first place in the line above. The core clock code then destroys >>> the associated clk_core structure. When the next user comes around (in >>> our case the clk debug functions) the system crashes. >>> >>> I believe there to be two issues: one is with v7 - we need to increase >>> the clock reference count in davinci_psc_genpd_attach_dev(). >>> >>> Second is the error path in the clock framework - we should remove the >>> destroyed clk_core from the debug list, which is not being done now. >>> >>> Why we even need to track the refcount of clk_core is a mistery for me >>> though. Stephen, Mike? >>> >>> Best regards, >>> Bartosz Golaszewski >> >> >> Great find. I figured it had to be something like this, but I wasn't >> able to reproduce the problem yet. >> >> I suppose it is time to spin up a v8 with some fixes. > > I still don't know why the mdio clock rate is much lower than in > mainline though. Any ideas? > > Thanks, > Bart > Now that you have fixed the crash, can you answer the questions I have asked earlier? > Can you post the output of this command so that I can see how your clocks are setup: cat /sys/kernel/debug/clk/clk_summary > Using your workaround, can you run: cat /sys/kernel/debug/pm_genpd/pm_genpd_summary If you see: 1e27000.clock-controller: emac off-0 then genpd is not working like it is supposed to. You should see something like this for device that are working: 1e27000.clock-controller: uart2 on /devices/platform/soc@1c00000/1d0d000.serial active -- 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-03-01 17:44 GMT+01:00 David Lechner <david@lechnology.com>: > On 03/01/2018 02:36 AM, Bartosz Golaszewski wrote: >> >> 2018-02-28 22:40 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>> On 02/28/2018 06:38 AM, Bartosz Golaszewski wrote: >>>> >>>> >>>> >>>> I think I found the reason for the strange crashes we were >>>> experiencing (emac core->name being NULL) thanks to Sekhar who pointed >>>> me in the right direction. >>>> >>>> The mdio driver fails to probe with v7 due to the supplied clock rate >>>> being wrong. Before failing we register the emac clock with >>>> pm_clk_add_clk(). When clock_ops puts the clock, it decreases the >>>> reference count of the clock, but we never actually increased it in >>>> the first place in the line above. The core clock code then destroys >>>> the associated clk_core structure. When the next user comes around (in >>>> our case the clk debug functions) the system crashes. >>>> >>>> I believe there to be two issues: one is with v7 - we need to increase >>>> the clock reference count in davinci_psc_genpd_attach_dev(). >>>> >>>> Second is the error path in the clock framework - we should remove the >>>> destroyed clk_core from the debug list, which is not being done now. >>>> >>>> Why we even need to track the refcount of clk_core is a mistery for me >>>> though. Stephen, Mike? >>>> >>>> Best regards, >>>> Bartosz Golaszewski >>> >>> >>> >>> Great find. I figured it had to be something like this, but I wasn't >>> able to reproduce the problem yet. >>> >>> I suppose it is time to spin up a v8 with some fixes. >> >> >> I still don't know why the mdio clock rate is much lower than in >> mainline though. Any ideas? >> >> Thanks, >> Bart >> > > Now that you have fixed the crash, can you answer the questions I have > asked earlier? > >> Can you post the output of this command so that I can see how your > > clocks are setup: > > cat /sys/kernel/debug/clk/clk_summary > >> Using your workaround, can you run: > > > cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > > If you see: > 1e27000.clock-controller: emac off-0 > > then genpd is not working like it is supposed to. You should see something > like this for device that are working: > 1e27000.clock-controller: uart2 on > /devices/platform/soc@1c00000/1d0d000.serial active I used of_clk_get() in the genpd attach callback so the crash no longer happens, but I still can't boot it over NFS due to mdio failing. Do you have any idea why the clock rate differs between v7 and mainline? >From the logs I can see that genpd domains are correctly registered, and the provider is added (you should probably skip setting up the domains in legacy mode though), the pm clocks are enabled (after being disabled by mdio after its failed probe()) but the boot process gets stuck after the kernel gets an IP address over DHCP (which is strange because apparently it had some kind of network connection). On Monday I'll prepare a small ramfs and boot over tftp only and see from there. Best regards, 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-03-01 17:44 GMT+01:00 David Lechner <david@lechnology.com>: > On 03/01/2018 02:36 AM, Bartosz Golaszewski wrote: >> >> 2018-02-28 22:40 GMT+01:00 David Lechner <david@lechnology.com>: >>> >>> On 02/28/2018 06:38 AM, Bartosz Golaszewski wrote: >>>> >>>> >>>> >>>> I think I found the reason for the strange crashes we were >>>> experiencing (emac core->name being NULL) thanks to Sekhar who pointed >>>> me in the right direction. >>>> >>>> The mdio driver fails to probe with v7 due to the supplied clock rate >>>> being wrong. Before failing we register the emac clock with >>>> pm_clk_add_clk(). When clock_ops puts the clock, it decreases the >>>> reference count of the clock, but we never actually increased it in >>>> the first place in the line above. The core clock code then destroys >>>> the associated clk_core structure. When the next user comes around (in >>>> our case the clk debug functions) the system crashes. >>>> >>>> I believe there to be two issues: one is with v7 - we need to increase >>>> the clock reference count in davinci_psc_genpd_attach_dev(). >>>> >>>> Second is the error path in the clock framework - we should remove the >>>> destroyed clk_core from the debug list, which is not being done now. >>>> >>>> Why we even need to track the refcount of clk_core is a mistery for me >>>> though. Stephen, Mike? >>>> >>>> Best regards, >>>> Bartosz Golaszewski >>> >>> >>> >>> Great find. I figured it had to be something like this, but I wasn't >>> able to reproduce the problem yet. >>> >>> I suppose it is time to spin up a v8 with some fixes. >> >> >> I still don't know why the mdio clock rate is much lower than in >> mainline though. Any ideas? >> >> Thanks, >> Bart >> > > Now that you have fixed the crash, can you answer the questions I have > asked earlier? > >> Can you post the output of this command so that I can see how your > > clocks are setup: > > cat /sys/kernel/debug/clk/clk_summary > >> Using your workaround, can you run: > > > cat /sys/kernel/debug/pm_genpd/pm_genpd_summary > > If you see: > 1e27000.clock-controller: emac off-0 > > then genpd is not working like it is supposed to. You should see something > like this for device that are working: > 1e27000.clock-controller: uart2 on > /devices/platform/soc@1c00000/1d0d000.serial active Hi David, Sekhar, I tried booting the board today over tftp but didn't succeed. I then switched to a normal boot from SD card and the boot process froze at the same moment (right after the DHCP config, or after rtc config if I disabled DHCP in bootargs). I then realized that the emac clock can't be the culprit. After some digging I found out that the late_initcall to clk_disable_unused() disables sysclk6 - the parent of the arm clock, which of course freezes the device. If I remove the call to clk_disable_unused(), I can boot just fine. The following other clocks are disabled before pll0_sysclk6: pll1_sysclk3 pll0_obsclk pll0_sysclk7 davinci_lpsc_clk_enable() is never called for these clocks - in fact it's not called for any parent that's not explicitly defined in psc-da850.c - I believe this may be one of the reasons. I will get back to debugging it tomorrow. Best regards, Bartosz Golaszewski -- 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 03/05/2018 07:02 AM, Bartosz Golaszewski wrote: > 2018-03-01 17:44 GMT+01:00 David Lechner <david@lechnology.com>: >> On 03/01/2018 02:36 AM, Bartosz Golaszewski wrote: >>> >>> 2018-02-28 22:40 GMT+01:00 David Lechner <david@lechnology.com>: >>>> >>>> On 02/28/2018 06:38 AM, Bartosz Golaszewski wrote: >>>>> >>>>> >>>>> >>>>> I think I found the reason for the strange crashes we were >>>>> experiencing (emac core->name being NULL) thanks to Sekhar who pointed >>>>> me in the right direction. >>>>> >>>>> The mdio driver fails to probe with v7 due to the supplied clock rate >>>>> being wrong. Before failing we register the emac clock with >>>>> pm_clk_add_clk(). When clock_ops puts the clock, it decreases the >>>>> reference count of the clock, but we never actually increased it in >>>>> the first place in the line above. The core clock code then destroys >>>>> the associated clk_core structure. When the next user comes around (in >>>>> our case the clk debug functions) the system crashes. >>>>> >>>>> I believe there to be two issues: one is with v7 - we need to increase >>>>> the clock reference count in davinci_psc_genpd_attach_dev(). >>>>> >>>>> Second is the error path in the clock framework - we should remove the >>>>> destroyed clk_core from the debug list, which is not being done now. >>>>> >>>>> Why we even need to track the refcount of clk_core is a mistery for me >>>>> though. Stephen, Mike? >>>>> >>>>> Best regards, >>>>> Bartosz Golaszewski >>>> >>>> >>>> >>>> Great find. I figured it had to be something like this, but I wasn't >>>> able to reproduce the problem yet. >>>> >>>> I suppose it is time to spin up a v8 with some fixes. >>> >>> >>> I still don't know why the mdio clock rate is much lower than in >>> mainline though. Any ideas? >>> >>> Thanks, >>> Bart >>> >> >> Now that you have fixed the crash, can you answer the questions I have >> asked earlier? >> >>> Can you post the output of this command so that I can see how your >> >> clocks are setup: >> >> cat /sys/kernel/debug/clk/clk_summary >> >>> Using your workaround, can you run: >> >> >> cat /sys/kernel/debug/pm_genpd/pm_genpd_summary >> >> If you see: >> 1e27000.clock-controller: emac off-0 >> >> then genpd is not working like it is supposed to. You should see something >> like this for device that are working: >> 1e27000.clock-controller: uart2 on >> /devices/platform/soc@1c00000/1d0d000.serial active > > Hi David, Sekhar, > > I tried booting the board today over tftp but didn't succeed. I then > switched to a normal boot from SD card and the boot process froze at > the same moment (right after the DHCP config, or after rtc config if I > disabled DHCP in bootargs). I then realized that the emac clock can't > be the culprit. After some digging I found out that the late_initcall > to clk_disable_unused() disables sysclk6 - the parent of the arm > clock, which of course freezes the device. > > If I remove the call to clk_disable_unused(), I can boot just fine. > > The following other clocks are disabled before pll0_sysclk6: > pll1_sysclk3 > pll0_obsclk > pll0_sysclk7 > > davinci_lpsc_clk_enable() is never called for these clocks - in fact > it's not called for any parent that's not explicitly defined in > psc-da850.c - I believe this may be one of the reasons. I will get > back to debugging it tomorrow. > > Best regards, > Bartosz Golaszewski > Thanks for continuing to dig into this. I think I know what needs to be done now. I think I don't have the dependencies quite right where the PSC clocks are being registered before the PLL clocks, in which case they aren't getting the correct parent clock. -- 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 03/05/2018 10:23 AM, David Lechner wrote: > On 03/05/2018 07:02 AM, Bartosz Golaszewski wrote: >> 2018-03-01 17:44 GMT+01:00 David Lechner <david@lechnology.com>: >>> On 03/01/2018 02:36 AM, Bartosz Golaszewski wrote: >>>> >>>> 2018-02-28 22:40 GMT+01:00 David Lechner <david@lechnology.com>: >>>>> >>>>> On 02/28/2018 06:38 AM, Bartosz Golaszewski wrote: >>>>>> >>>>>> >>>>>> >>>>>> I think I found the reason for the strange crashes we were >>>>>> experiencing (emac core->name being NULL) thanks to Sekhar who pointed >>>>>> me in the right direction. >>>>>> >>>>>> The mdio driver fails to probe with v7 due to the supplied clock rate >>>>>> being wrong. Before failing we register the emac clock with >>>>>> pm_clk_add_clk(). When clock_ops puts the clock, it decreases the >>>>>> reference count of the clock, but we never actually increased it in >>>>>> the first place in the line above. The core clock code then destroys >>>>>> the associated clk_core structure. When the next user comes around (in >>>>>> our case the clk debug functions) the system crashes. >>>>>> >>>>>> I believe there to be two issues: one is with v7 - we need to increase >>>>>> the clock reference count in davinci_psc_genpd_attach_dev(). >>>>>> >>>>>> Second is the error path in the clock framework - we should remove the >>>>>> destroyed clk_core from the debug list, which is not being done now. >>>>>> >>>>>> Why we even need to track the refcount of clk_core is a mistery for me >>>>>> though. Stephen, Mike? >>>>>> >>>>>> Best regards, >>>>>> Bartosz Golaszewski >>>>> >>>>> >>>>> >>>>> Great find. I figured it had to be something like this, but I wasn't >>>>> able to reproduce the problem yet. >>>>> >>>>> I suppose it is time to spin up a v8 with some fixes. >>>> >>>> >>>> I still don't know why the mdio clock rate is much lower than in >>>> mainline though. Any ideas? >>>> >>>> Thanks, >>>> Bart >>>> >>> >>> Now that you have fixed the crash, can you answer the questions I have >>> asked earlier? >>> >>>> Can you post the output of this command so that I can see how your >>> >>> clocks are setup: >>> >>> cat /sys/kernel/debug/clk/clk_summary >>> >>>> Using your workaround, can you run: >>> >>> >>> cat /sys/kernel/debug/pm_genpd/pm_genpd_summary >>> >>> If you see: >>> 1e27000.clock-controller: emac off-0 >>> >>> then genpd is not working like it is supposed to. You should see something >>> like this for device that are working: >>> 1e27000.clock-controller: uart2 on >>> /devices/platform/soc@1c00000/1d0d000.serial active >> >> Hi David, Sekhar, >> >> I tried booting the board today over tftp but didn't succeed. I then >> switched to a normal boot from SD card and the boot process froze at >> the same moment (right after the DHCP config, or after rtc config if I >> disabled DHCP in bootargs). I then realized that the emac clock can't >> be the culprit. After some digging I found out that the late_initcall >> to clk_disable_unused() disables sysclk6 - the parent of the arm >> clock, which of course freezes the device. >> >> If I remove the call to clk_disable_unused(), I can boot just fine. >> >> The following other clocks are disabled before pll0_sysclk6: >> pll1_sysclk3 >> pll0_obsclk >> pll0_sysclk7 >> >> davinci_lpsc_clk_enable() is never called for these clocks - in fact >> it's not called for any parent that's not explicitly defined in >> psc-da850.c - I believe this may be one of the reasons. I will get >> back to debugging it tomorrow. >> >> Best regards, >> Bartosz Golaszewski >> > > Thanks for continuing to dig into this. I think I know what needs to > be done now. I think I don't have the dependencies quite right where > the PSC clocks are being registered before the PLL clocks, in which > case they aren't getting the correct parent clock. > Bartosz, One more thing to check: I think I had some typos in da850.dtsi where I wrote clock_names instead of clock-names. Please make sure this is fixed in your working branch. -- 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
Quoting Bartosz Golaszewski (2018-02-28 04:38:38) > 2018-02-19 21:21 GMT+01:00 David Lechner <david@lechnology.com>: > > I believe there to be two issues: one is with v7 - we need to increase > the clock reference count in davinci_psc_genpd_attach_dev(). > > Second is the error path in the clock framework - we should remove the > destroyed clk_core from the debug list, which is not being done now. > > Why we even need to track the refcount of clk_core is a mistery for me > though. Stephen, Mike? > Which part of the code are we talking about? I see that __clk_core_init() calls clk_debug_register() when ret == 0 and that looks fine. I do wonder why clk_debug_register() even returns a value though because we ignore it. -- 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-03-16 18:55 GMT+01:00 Stephen Boyd <sboyd@kernel.org>: > Quoting Bartosz Golaszewski (2018-02-28 04:38:38) >> 2018-02-19 21:21 GMT+01:00 David Lechner <david@lechnology.com>: >> >> I believe there to be two issues: one is with v7 - we need to increase >> the clock reference count in davinci_psc_genpd_attach_dev(). >> >> Second is the error path in the clock framework - we should remove the >> destroyed clk_core from the debug list, which is not being done now. >> >> Why we even need to track the refcount of clk_core is a mistery for me >> though. Stephen, Mike? >> > > Which part of the code are we talking about? I see that > __clk_core_init() calls clk_debug_register() when ret == 0 and that > looks fine. I do wonder why clk_debug_register() even returns a value > though because we ignore it. Hi Stephen, invalid usage of clock pm ops was the culprit here. I just had not understood why we count references in the CCF - now I get it so nevermind this mail. Thanks, Bart -- 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
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: davinci: pass clock as parameter to davinci_timer_init()", 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. --- The change to make all of the clocks platform devices in v7 was a pretty major change, so unfortunately I had to drop quite a few Reviewed-bys and this series will need more close review and testing again. v7 changes (also see individual patches for details): - Rebased on linux-davinci/master (v4.16-rc) - Convert clock drivers to platform devices - New patch "ARM: davinci: pass clock as parameter to davinci_timer_init()" - Fix issues with lcdk and aemif clock lookups and power domains - Fixed other minor issues brought up in v6 review 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: Most of the dependencies have landed in mainline already in v4.16-rc1 or are in linux-davinci/master. There are no build dependencies any more, but you will need to pickup a couple of patches to get CPU frequency scaling working because of a divider clock bug. - https://patchwork.kernel.org/patch/10218941/ - https://patchwork.kernel.org/patch/10218927/ You can find a working branch with everything included in the "common-clk-v7" branch of https://github.com/dlech/ev3dev-kernel.git. 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 (42): 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: cfgchip: Add TI DA8XX USB PHY clocks ARM: davinci: pass clock as parameter to davinci_timer_init() ARM: davinci: da830: add new clock init using common clock framework ARM: davinci: da850: add new clock init using common clock framework ARM: davinci: dm355: add new clock init using common clock framework ARM: davinci: dm365: add new clock init using common clock framework ARM: davinci: dm644x: add new clock init using common clock framework ARM: davinci: dm646x: add new clock init using common clock framework ARM: davinci: da8xx: add new USB PHY clock init using common clock framework ARM: davinci: 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: davinci: da830: Remove legacy clock init ARM: davinci: da850: Remove legacy clock init ARM: davinci: dm355: Remove legacy clock init ARM: davinci: dm365: Remove legacy clock init ARM: davinci: dm644x: Remove legacy clock init ARM: davinci: dm646x: Remove legacy clock init ARM: davinci: da8xx: Remove legacy USB and SATA clock init ARM: davinci: remove legacy clocks ARM: davinci: add device tree support to timer ARM: davinci: 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 | 71 ++ MAINTAINERS | 7 + arch/arm/Kconfig | 5 +- arch/arm/boot/dts/da850-enbw-cmc.dts | 4 + arch/arm/boot/dts/da850-evm.dts | 4 + arch/arm/boot/dts/da850-lcdk.dts | 9 + arch/arm/boot/dts/da850-lego-ev3.dts | 4 + arch/arm/boot/dts/da850.dtsi | 159 ++++ arch/arm/configs/davinci_all_defconfig | 1 - arch/arm/mach-davinci/Kconfig | 13 +- arch/arm/mach-davinci/Makefile | 4 +- arch/arm/mach-davinci/board-da830-evm.c | 12 +- arch/arm/mach-davinci/board-da850-evm.c | 2 + arch/arm/mach-davinci/board-dm355-evm.c | 2 + arch/arm/mach-davinci/board-dm355-leopard.c | 2 + arch/arm/mach-davinci/board-dm365-evm.c | 2 + arch/arm/mach-davinci/board-dm644x-evm.c | 2 + arch/arm/mach-davinci/board-dm646x-evm.c | 2 + arch/arm/mach-davinci/board-mityomapl138.c | 2 + arch/arm/mach-davinci/board-neuros-osd2.c | 2 + arch/arm/mach-davinci/board-omapl138-hawk.c | 11 +- arch/arm/mach-davinci/board-sffsdr.c | 2 + 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 | 469 ++--------- arch/arm/mach-davinci/da850.c | 766 +++--------------- arch/arm/mach-davinci/da8xx-dt.c | 60 -- arch/arm/mach-davinci/davinci.h | 8 + arch/arm/mach-davinci/devices-da8xx.c | 43 +- arch/arm/mach-davinci/devices.c | 1 - arch/arm/mach-davinci/dm355.c | 425 ++-------- arch/arm/mach-davinci/dm365.c | 517 ++---------- arch/arm/mach-davinci/dm644x.c | 363 ++------- arch/arm/mach-davinci/dm646x.c | 391 ++------- arch/arm/mach-davinci/include/mach/clock.h | 3 - arch/arm/mach-davinci/include/mach/common.h | 11 +- arch/arm/mach-davinci/include/mach/da8xx.h | 6 +- arch/arm/mach-davinci/pm_domain.c | 5 + arch/arm/mach-davinci/psc.c | 137 ---- arch/arm/mach-davinci/psc.h | 12 - arch/arm/mach-davinci/time.c | 46 +- arch/arm/mach-davinci/usb-da8xx.c | 250 +----- drivers/clk/Makefile | 1 + drivers/clk/davinci/Makefile | 21 + drivers/clk/davinci/da8xx-cfgchip.c | 786 ++++++++++++++++++ drivers/clk/davinci/pll-da830.c | 58 ++ drivers/clk/davinci/pll-da850.c | 178 +++++ drivers/clk/davinci/pll-dm355.c | 75 ++ drivers/clk/davinci/pll-dm365.c | 119 +++ drivers/clk/davinci/pll-dm644x.c | 76 ++ drivers/clk/davinci/pll-dm646x.c | 78 ++ drivers/clk/davinci/pll.c | 877 +++++++++++++++++++++ drivers/clk/davinci/pll.h | 141 ++++ drivers/clk/davinci/psc-da830.c | 89 +++ drivers/clk/davinci/psc-da850.c | 109 +++ drivers/clk/davinci/psc-dm355.c | 72 ++ drivers/clk/davinci/psc-dm365.c | 77 ++ drivers/clk/davinci/psc-dm644x.c | 67 ++ drivers/clk/davinci/psc-dm646x.c | 62 ++ drivers/clk/davinci/psc.c | 505 ++++++++++++ drivers/clk/davinci/psc.h | 101 +++ include/linux/platform_data/clk-da8xx-cfgchip.h | 21 + 65 files changed, 4515 insertions(+), 3846 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/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/platform_data/clk-da8xx-cfgchip.h