| Message ID | 597caa1a-a790-3629-3186-5d2152f45e06@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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)