* [PATCH 0/3] clk: ti: couple of fixes towards 5.4
@ 2019-08-07 13:04 Tero Kristo
2019-08-07 13:04 ` [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only Tero Kristo
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Tero Kristo @ 2019-08-07 13:04 UTC (permalink / raw)
To: linux-clk, sboyd, mturquette; +Cc: linux-omap, tony, s-anna
Hi,
Here are some TI clock fixes which can be queued for 5.4. These are
needed for getting remoteproc functionality working properly, as these
depend on reset handling also and timing out with clocks is bad for
them. The timer clock alias fix is needed for the same, as remoteprocs
depend on certain HW timers for their functionality.
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only 2019-08-07 13:04 [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Tero Kristo @ 2019-08-07 13:04 ` Tero Kristo 2019-08-07 22:43 ` Suman Anna 2019-08-07 13:04 ` [PATCH 2/3] clk: ti: dra7xx: remove idlest polling from disabling ipu/dsp clocks Tero Kristo ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Tero Kristo @ 2019-08-07 13:04 UTC (permalink / raw) To: linux-clk, sboyd, mturquette; +Cc: linux-omap, tony, s-anna The activity status for certain clocks is possible to be polled only during enable phase; the disable phase depends on additional reset logic. If the disable phase is polled with these clocks, it will result in a timeout. To fix this, add logic for polling the clock activity only during enable, and add a new flag for this purpose. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/clk/ti/clkctrl.c | 5 ++++- drivers/clk/ti/clock.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c index 975995e..f5517a8 100644 --- a/drivers/clk/ti/clkctrl.c +++ b/drivers/clk/ti/clkctrl.c @@ -25,6 +25,7 @@ #include "clock.h" #define NO_IDLEST 0x1 +#define NO_IDLE_POLL 0x2 #define OMAP4_MODULEMODE_MASK 0x3 @@ -187,7 +188,7 @@ static void _omap4_clkctrl_clk_disable(struct clk_hw *hw) ti_clk_ll_ops->clk_writel(val, &clk->enable_reg); - if (clk->flags & NO_IDLEST) + if (clk->flags & (NO_IDLEST | NO_IDLE_POLL)) goto exit; /* Wait until module is disabled */ @@ -597,6 +598,8 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node) hw->enable_bit = MODULEMODE_HWCTRL; if (reg_data->flags & CLKF_NO_IDLEST) hw->flags |= NO_IDLEST; + if (reg_data->flags & CLKF_NO_IDLE_POLL) + hw->flags |= NO_IDLE_POLL; if (reg_data->clkdm_name) hw->clkdm_name = reg_data->clkdm_name; diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h index e4b8392..6410ff6 100644 --- a/drivers/clk/ti/clock.h +++ b/drivers/clk/ti/clock.h @@ -82,6 +82,7 @@ enum { #define CLKF_SW_SUP BIT(5) #define CLKF_HW_SUP BIT(6) #define CLKF_NO_IDLEST BIT(7) +#define CLKF_NO_IDLE_POLL BIT(8) #define CLKF_SOC_MASK GENMASK(11, 8) -- 1.9.1 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only 2019-08-07 13:04 ` [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only Tero Kristo @ 2019-08-07 22:43 ` Suman Anna 2019-08-19 9:13 ` Tero Kristo 0 siblings, 1 reply; 18+ messages in thread From: Suman Anna @ 2019-08-07 22:43 UTC (permalink / raw) To: Tero Kristo, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony Hi Tero, On 8/7/19 8:04 AM, Tero Kristo wrote: > The activity status for certain clocks is possible to be polled only > during enable phase; the disable phase depends on additional reset > logic. I am not sure this is an entirely accurate statement. Can you explain why this is an issue only with disable sequence and not enable sequence? Almost sounds like you are doing some asymmetric sequence w.r.t clocks and resets. On the downstream kernel, we have reused the existing NO_IDLEST flag as a quirk within both the enable and disable functions for the IPs with hardreset lines, and this patch seems to introduce a new NO_IDLE_POLL flag but only during the disable path. regards Suman If the disable phase is polled with these clocks, it will > result in a timeout. To fix this, add logic for polling the clock > activity only during enable, and add a new flag for this purpose. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/ti/clkctrl.c | 5 ++++- > drivers/clk/ti/clock.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c > index 975995e..f5517a8 100644 > --- a/drivers/clk/ti/clkctrl.c > +++ b/drivers/clk/ti/clkctrl.c > @@ -25,6 +25,7 @@ > #include "clock.h" > > #define NO_IDLEST 0x1 > +#define NO_IDLE_POLL 0x2 > > #define OMAP4_MODULEMODE_MASK 0x3 > > @@ -187,7 +188,7 @@ static void _omap4_clkctrl_clk_disable(struct clk_hw *hw) > > ti_clk_ll_ops->clk_writel(val, &clk->enable_reg); > > - if (clk->flags & NO_IDLEST) > + if (clk->flags & (NO_IDLEST | NO_IDLE_POLL)) > goto exit; > > /* Wait until module is disabled */ > @@ -597,6 +598,8 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node) > hw->enable_bit = MODULEMODE_HWCTRL; > if (reg_data->flags & CLKF_NO_IDLEST) > hw->flags |= NO_IDLEST; > + if (reg_data->flags & CLKF_NO_IDLE_POLL) > + hw->flags |= NO_IDLE_POLL; > > if (reg_data->clkdm_name) > hw->clkdm_name = reg_data->clkdm_name; > diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h > index e4b8392..6410ff6 100644 > --- a/drivers/clk/ti/clock.h > +++ b/drivers/clk/ti/clock.h > @@ -82,6 +82,7 @@ enum { > #define CLKF_SW_SUP BIT(5) > #define CLKF_HW_SUP BIT(6) > #define CLKF_NO_IDLEST BIT(7) > +#define CLKF_NO_IDLE_POLL BIT(8) > > #define CLKF_SOC_MASK GENMASK(11, 8) > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only 2019-08-07 22:43 ` Suman Anna @ 2019-08-19 9:13 ` Tero Kristo 2019-08-19 21:07 ` Suman Anna 0 siblings, 1 reply; 18+ messages in thread From: Tero Kristo @ 2019-08-19 9:13 UTC (permalink / raw) To: Suman Anna, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony On 08/08/2019 01:43, Suman Anna wrote: > Hi Tero, > > On 8/7/19 8:04 AM, Tero Kristo wrote: >> The activity status for certain clocks is possible to be polled only >> during enable phase; the disable phase depends on additional reset >> logic. > > I am not sure this is an entirely accurate statement. Can you explain > why this is an issue only with disable sequence and not enable sequence? > Almost sounds like you are doing some asymmetric sequence w.r.t clocks > and resets. This follows the recommended ordering sequence from TRM, so reset will be de-asserted before enabling clock, so we can keep the polling there. Going down, reset must be asserted post disabling clocks, which results a timeout if the idle status check is not bypassed. This is kind of not perfect and should be fixed later to somehow add a direct link between the clock and reset lines, so that we know when there is dependency between the two and can check the status of both to see if we should poll something or not. > > On the downstream kernel, we have reused the existing NO_IDLEST flag as > a quirk within both the enable and disable functions for the IPs with > hardreset lines, and this patch seems to introduce a new NO_IDLE_POLL > flag but only during the disable path. The NO_IDLEST patch is not perfect, as it introduces a timing hazard where while enabling the module one can access the IP registers before it has left idle, leading into a crash. -Tero > > regards > Suman > > If the disable phase is polled with these clocks, it will >> result in a timeout. To fix this, add logic for polling the clock >> activity only during enable, and add a new flag for this purpose. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/clk/ti/clkctrl.c | 5 ++++- >> drivers/clk/ti/clock.h | 1 + >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c >> index 975995e..f5517a8 100644 >> --- a/drivers/clk/ti/clkctrl.c >> +++ b/drivers/clk/ti/clkctrl.c >> @@ -25,6 +25,7 @@ >> #include "clock.h" >> >> #define NO_IDLEST 0x1 >> +#define NO_IDLE_POLL 0x2 >> >> #define OMAP4_MODULEMODE_MASK 0x3 >> >> @@ -187,7 +188,7 @@ static void _omap4_clkctrl_clk_disable(struct clk_hw *hw) >> >> ti_clk_ll_ops->clk_writel(val, &clk->enable_reg); >> >> - if (clk->flags & NO_IDLEST) >> + if (clk->flags & (NO_IDLEST | NO_IDLE_POLL)) >> goto exit; >> >> /* Wait until module is disabled */ >> @@ -597,6 +598,8 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node) >> hw->enable_bit = MODULEMODE_HWCTRL; >> if (reg_data->flags & CLKF_NO_IDLEST) >> hw->flags |= NO_IDLEST; >> + if (reg_data->flags & CLKF_NO_IDLE_POLL) >> + hw->flags |= NO_IDLE_POLL; >> >> if (reg_data->clkdm_name) >> hw->clkdm_name = reg_data->clkdm_name; >> diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h >> index e4b8392..6410ff6 100644 >> --- a/drivers/clk/ti/clock.h >> +++ b/drivers/clk/ti/clock.h >> @@ -82,6 +82,7 @@ enum { >> #define CLKF_SW_SUP BIT(5) >> #define CLKF_HW_SUP BIT(6) >> #define CLKF_NO_IDLEST BIT(7) >> +#define CLKF_NO_IDLE_POLL BIT(8) >> >> #define CLKF_SOC_MASK GENMASK(11, 8) >> >> > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only 2019-08-19 9:13 ` Tero Kristo @ 2019-08-19 21:07 ` Suman Anna 2019-08-20 8:17 ` Tero Kristo 0 siblings, 1 reply; 18+ messages in thread From: Suman Anna @ 2019-08-19 21:07 UTC (permalink / raw) To: Kristo, Tero, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony On 8/19/19 4:13 AM, Tero Kristo wrote: > On 08/08/2019 01:43, Suman Anna wrote: >> Hi Tero, >> >> On 8/7/19 8:04 AM, Tero Kristo wrote: >>> The activity status for certain clocks is possible to be polled only >>> during enable phase; the disable phase depends on additional reset >>> logic. >> >> I am not sure this is an entirely accurate statement. Can you explain >> why this is an issue only with disable sequence and not enable sequence? >> Almost sounds like you are doing some asymmetric sequence w.r.t clocks >> and resets. > > This follows the recommended ordering sequence from TRM, so reset will > be de-asserted before enabling clock, so we can keep the polling there. Can you please point out the section where this ordering sequence is mentioned? If anything, this is quite the opposite, and that is what the existing hwmod code also reflects. Please see the NOTE in Section 5.3.3.2 of the DRA7 TRM [1] for reference and the section 3.5.6.6 and/or 3.5.6.7. Your patch is a consequence of your on-going ti-sysc code where you have flipped the logic compared to hwmod code. In anycase, the mainline kernel has the various MMUs on OMAP3, OMAP4 and OMAP5 SoCs functional a long-time before ti-sysc and clkctrl are introduced and this was broken when clkctrl clks were introduced in 4.16 kernel. The issue can be seen rather easily with an OMAP IOMMU unit-test [2], and the error issue signatures are something like below. Below log is an example log generated when using OMAP5 DSP MMU on 5.3-rc1 + addition of test nodes from [2], and similar crashes are also seen with other MMUs. # insmod iommu_dt_test.ko count=4 [ 126.070188] iommu_dt_test: loading out-of-tree module taints kernel. [ 126.077568] omap_iommu_test iommu_test: ignoring dependency for device, assuming no driver [ 126.085963] omap_iommu_test_init: iommu_test_init entered [ 126.091495] omap_iommu_test iommu_test: Enabling IOMMU... [ 126.096997] omap_iommu_test iommu_test: dev->of_node->name: iommu_test dev_name iommu_test [ 126.107352] dsp_cm:clk:0000:0: failed to enable [ 126.111907] ------------[ cut here ]------------ [ 126.116553] WARNING: CPU: 0 PID: 1013 at drivers/clk/clk.c:924 clk_core_disable_lock+0x18/0x24 [ 126.125198] dsp_cm:clk:0000:0 already disabled [ 126.129656] Modules linked in: iommu_dt_test(O+) [ 126.134299] CPU: 0 PID: 1013 Comm: insmod Tainted: G O 5.3.0-rc1-00005-gd893572f52c6 #129 [ 126.143816] Hardware name: Generic OMAP5 (Flattened Device Tree) [ 126.149859] [<c01122d8>] (unwind_backtrace) from [<c010c8b8>] (show_stack+0x10/0x14) [ 126.157641] [<c010c8b8>] (show_stack) from [<c08cce38>] (dump_stack+0xb4/0xd4) [ 126.164900] [<c08cce38>] (dump_stack) from [<c0139d70>] (__warn.part.3+0xa8/0xd4) [ 126.172419] [<c0139d70>] (__warn.part.3) from [<c0139df8>] (warn_slowpath_fmt+0x5c/0x88) [ 126.180549] [<c0139df8>] (warn_slowpath_fmt) from [<c05733b4>] (clk_core_disable_lock+0x18/0x24) [ 126.189376] [<c05733b4>] (clk_core_disable_lock) from [<c0122d5c>] (_disable_clocks+0x18/0x98) [ 126.198027] [<c0122d5c>] (_disable_clocks) from [<c012569c>] (omap_hwmod_deassert_hardreset+0xc8/0x174) [ 126.207463] [<c012569c>] (omap_hwmod_deassert_hardreset) from [<c0126160>] (omap_device_deassert_hardreset+0x30/0x50) [ 126.218121] [<c0126160>] (omap_device_deassert_hardreset) from [<c05d5f24>] (omap_iommu_attach_dev+0x298/0x418) [ 126.228256] [<c05d5f24>] (omap_iommu_attach_dev) from [<c05d0ba8>] (__iommu_attach_device+0x44/0xdc) [ 126.237430] [<c05d0ba8>] (__iommu_attach_device) from [<c05d28e8>] (__iommu_attach_group+0x40/0x68) [ 126.246517] [<c05d28e8>] (__iommu_attach_group) from [<c05d29c8>] (iommu_attach_device+0x80/0x88) [ 126.255434] [<c05d29c8>] (iommu_attach_device) from [<bf000188>] (omap_iommu_test_probe+0x10c/0x210 [iommu_dt_test]) [ 126.266011] [<bf000188>] (omap_iommu_test_probe [iommu_dt_test]) from [<c05e0d8c>] (platform_drv_probe+0x48/0x98) [ 126.276321] [<c05e0d8c>] (platform_drv_probe) from [<c05dedd0>] (really_probe+0xec/0x2cc) [ 126.284537] [<c05dedd0>] (really_probe) from [<c05df134>] (driver_probe_device+0x5c/0x160) [ 126.292839] [<c05df134>] (driver_probe_device) from [<c05df3d8>] (device_driver_attach+0x58/0x60) [ 126.301751] [<c05df3d8>] (device_driver_attach) from [<c05df438>] (__driver_attach+0x58/0xcc) [ 126.310314] [<c05df438>] (__driver_attach) from [<c05dd264>] (bus_for_each_dev+0x70/0xb4) [ 126.318529] [<c05dd264>] (bus_for_each_dev) from [<c05de2ac>] (bus_add_driver+0x198/0x1d0) [ 126.326831] [<c05de2ac>] (bus_add_driver) from [<c05dfea0>] (driver_register+0x74/0x108) [ 126.334959] [<c05dfea0>] (driver_register) from [<c0102e80>] (do_one_initcall+0x48/0x224) [ 126.343177] [<c0102e80>] (do_one_initcall) from [<c01d6fa4>] (do_init_module+0x5c/0x234) [ 126.351307] [<c01d6fa4>] (do_init_module) from [<c01d9404>] (load_module+0x2200/0x24d0) [ 126.359347] [<c01d9404>] (load_module) from [<c01d9928>] (sys_finit_module+0xbc/0xdc) [ 126.367213] [<c01d9928>] (sys_finit_module) from [<c01011e0>] (__sys_trace_return+0x0/0x20) [ 126.375598] Exception stack(0xebc99fa8 to 0xebc99ff0) [ 126.380671] 9fa0: 00000000 00035160 00000003 00035150 00000000 00035ee8 [ 126.388885] 9fc0: 00000000 00035160 00000000 0000017b 00000007 00000003 00035150 be8e3c2c [ 126.397095] 9fe0: be8e3a80 be8e3a70 0001b42d b6e4a1b0 [ 126.402166] ---[ end trace 9ba6f4788aad890b ]--- [ 126.406896] omap-iommu 4a066000.mmu: 4a066000.mmu: version 2.0 [ 126.412765] omap_iommu_test iommu_test: Mapping da 0xa0100000, pa 0x95100000, len 0x100000 [ 126.421196] omap_iommu_test iommu_test: Mapping da 0xa0200000, pa 0x95200000, len 0x100000 [ 126.429622] omap_iommu_test iommu_test: Mapping da 0xa0300000, pa 0x95300000, len 0x100000 [ 126.437997] omap_iommu_test iommu_test: Mapping da 0xa0400000, pa 0x95400000, len 0x100000 The fix for that is actually doing this poll bailout in _enable rather than disable. I would rather see these fixed first before the ti-sysc conversions and logic are vetted. > > Going down, reset must be asserted post disabling clocks, which results > a timeout if the idle status check is not bypassed. > > This is kind of not perfect and should be fixed later to somehow add a > direct link between the clock and reset lines, so that we know when > there is dependency between the two and can check the status of both to > see if we should poll something or not. Yeah, agreed. Unfortunately, I do not there is a clean way of doing this given that typically clocks and resets are treated and managed by separate subsystems in kernel. You will always end up with a quirk flags like this in either of the drivers. > >> >> On the downstream kernel, we have reused the existing NO_IDLEST flag as >> a quirk within both the enable and disable functions for the IPs with >> hardreset lines, and this patch seems to introduce a new NO_IDLE_POLL >> flag but only during the disable path. > > The NO_IDLEST patch is not perfect, as it introduces a timing hazard > where while enabling the module one can access the IP registers before > it has left idle, leading into a crash. Both these flag macro names are misnomers IMO, the IP registers cannot be accessed without releasing the resets and clocks on the IPs with the hard-reset lines. regards Suman [1] http://www.ti.com/lit/pdf/sprui30 [2] https://github.com/sumananna/omap-test-iommu > > -Tero > >> >> regards >> Suman >> >> If the disable phase is polled with these clocks, it will >>> result in a timeout. To fix this, add logic for polling the clock >>> activity only during enable, and add a new flag for this purpose. >>> >>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>> --- >>> drivers/clk/ti/clkctrl.c | 5 ++++- >>> drivers/clk/ti/clock.h | 1 + >>> 2 files changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c >>> index 975995e..f5517a8 100644 >>> --- a/drivers/clk/ti/clkctrl.c >>> +++ b/drivers/clk/ti/clkctrl.c >>> @@ -25,6 +25,7 @@ >>> #include "clock.h" >>> #define NO_IDLEST 0x1 >>> +#define NO_IDLE_POLL 0x2 >>> #define OMAP4_MODULEMODE_MASK 0x3 >>> @@ -187,7 +188,7 @@ static void _omap4_clkctrl_clk_disable(struct >>> clk_hw *hw) >>> ti_clk_ll_ops->clk_writel(val, &clk->enable_reg); >>> - if (clk->flags & NO_IDLEST) >>> + if (clk->flags & (NO_IDLEST | NO_IDLE_POLL)) >>> goto exit; >>> /* Wait until module is disabled */ >>> @@ -597,6 +598,8 @@ static void __init _ti_omap4_clkctrl_setup(struct >>> device_node *node) >>> hw->enable_bit = MODULEMODE_HWCTRL; >>> if (reg_data->flags & CLKF_NO_IDLEST) >>> hw->flags |= NO_IDLEST; >>> + if (reg_data->flags & CLKF_NO_IDLE_POLL) >>> + hw->flags |= NO_IDLE_POLL; >>> if (reg_data->clkdm_name) >>> hw->clkdm_name = reg_data->clkdm_name; >>> diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h >>> index e4b8392..6410ff6 100644 >>> --- a/drivers/clk/ti/clock.h >>> +++ b/drivers/clk/ti/clock.h >>> @@ -82,6 +82,7 @@ enum { >>> #define CLKF_SW_SUP BIT(5) >>> #define CLKF_HW_SUP BIT(6) >>> #define CLKF_NO_IDLEST BIT(7) >>> +#define CLKF_NO_IDLE_POLL BIT(8) >>> #define CLKF_SOC_MASK GENMASK(11, 8) >>> >> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only 2019-08-19 21:07 ` Suman Anna @ 2019-08-20 8:17 ` Tero Kristo 0 siblings, 0 replies; 18+ messages in thread From: Tero Kristo @ 2019-08-20 8:17 UTC (permalink / raw) To: Suman Anna, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony On 20.8.2019 0.07, Suman Anna wrote: > On 8/19/19 4:13 AM, Tero Kristo wrote: >> On 08/08/2019 01:43, Suman Anna wrote: >>> Hi Tero, >>> >>> On 8/7/19 8:04 AM, Tero Kristo wrote: >>>> The activity status for certain clocks is possible to be polled only >>>> during enable phase; the disable phase depends on additional reset >>>> logic. >>> >>> I am not sure this is an entirely accurate statement. Can you explain >>> why this is an issue only with disable sequence and not enable sequence? >>> Almost sounds like you are doing some asymmetric sequence w.r.t clocks >>> and resets. >> >> This follows the recommended ordering sequence from TRM, so reset will >> be de-asserted before enabling clock, so we can keep the polling there. > > Can you please point out the section where this ordering sequence is > mentioned? If anything, this is quite the opposite, and that is what the > existing hwmod code also reflects. Please see the NOTE in Section > 5.3.3.2 of the DRA7 TRM [1] for reference and the section 3.5.6.6 and/or > 3.5.6.7. Ah you are right. This was actually because of the flipped logic within ti-sysc driver. I'll have a look if I can figure out a better way to handle this. -Tero > > Your patch is a consequence of your on-going ti-sysc code where you have > flipped the logic compared to hwmod code. In anycase, the mainline > kernel has the various MMUs on OMAP3, OMAP4 and OMAP5 SoCs functional a > long-time before ti-sysc and clkctrl are introduced and this was broken > when clkctrl clks were introduced in 4.16 kernel. The issue can be seen > rather easily with an OMAP IOMMU unit-test [2], and the error issue > signatures are something like below. Below log is an example log > generated when using OMAP5 DSP MMU on 5.3-rc1 + addition of test nodes > from [2], and similar crashes are also seen with other MMUs. > > # insmod iommu_dt_test.ko count=4 > [ 126.070188] iommu_dt_test: loading out-of-tree module taints kernel. > [ 126.077568] omap_iommu_test iommu_test: ignoring dependency for > device, assuming no driver > [ 126.085963] omap_iommu_test_init: iommu_test_init entered > [ 126.091495] omap_iommu_test iommu_test: Enabling IOMMU... > [ 126.096997] omap_iommu_test iommu_test: dev->of_node->name: > iommu_test dev_name iommu_test > [ 126.107352] dsp_cm:clk:0000:0: failed to enable > [ 126.111907] ------------[ cut here ]------------ > [ 126.116553] WARNING: CPU: 0 PID: 1013 at drivers/clk/clk.c:924 > clk_core_disable_lock+0x18/0x24 > [ 126.125198] dsp_cm:clk:0000:0 already disabled > [ 126.129656] Modules linked in: iommu_dt_test(O+) > [ 126.134299] CPU: 0 PID: 1013 Comm: insmod Tainted: G O > 5.3.0-rc1-00005-gd893572f52c6 #129 > [ 126.143816] Hardware name: Generic OMAP5 (Flattened Device Tree) > [ 126.149859] [<c01122d8>] (unwind_backtrace) from [<c010c8b8>] > (show_stack+0x10/0x14) > [ 126.157641] [<c010c8b8>] (show_stack) from [<c08cce38>] > (dump_stack+0xb4/0xd4) > [ 126.164900] [<c08cce38>] (dump_stack) from [<c0139d70>] > (__warn.part.3+0xa8/0xd4) > [ 126.172419] [<c0139d70>] (__warn.part.3) from [<c0139df8>] > (warn_slowpath_fmt+0x5c/0x88) > [ 126.180549] [<c0139df8>] (warn_slowpath_fmt) from [<c05733b4>] > (clk_core_disable_lock+0x18/0x24) > [ 126.189376] [<c05733b4>] (clk_core_disable_lock) from [<c0122d5c>] > (_disable_clocks+0x18/0x98) > [ 126.198027] [<c0122d5c>] (_disable_clocks) from [<c012569c>] > (omap_hwmod_deassert_hardreset+0xc8/0x174) > [ 126.207463] [<c012569c>] (omap_hwmod_deassert_hardreset) from > [<c0126160>] (omap_device_deassert_hardreset+0x30/0x50) > [ 126.218121] [<c0126160>] (omap_device_deassert_hardreset) from > [<c05d5f24>] (omap_iommu_attach_dev+0x298/0x418) > [ 126.228256] [<c05d5f24>] (omap_iommu_attach_dev) from [<c05d0ba8>] > (__iommu_attach_device+0x44/0xdc) > [ 126.237430] [<c05d0ba8>] (__iommu_attach_device) from [<c05d28e8>] > (__iommu_attach_group+0x40/0x68) > [ 126.246517] [<c05d28e8>] (__iommu_attach_group) from [<c05d29c8>] > (iommu_attach_device+0x80/0x88) > [ 126.255434] [<c05d29c8>] (iommu_attach_device) from [<bf000188>] > (omap_iommu_test_probe+0x10c/0x210 [iommu_dt_test]) > [ 126.266011] [<bf000188>] (omap_iommu_test_probe [iommu_dt_test]) from > [<c05e0d8c>] (platform_drv_probe+0x48/0x98) > [ 126.276321] [<c05e0d8c>] (platform_drv_probe) from [<c05dedd0>] > (really_probe+0xec/0x2cc) > [ 126.284537] [<c05dedd0>] (really_probe) from [<c05df134>] > (driver_probe_device+0x5c/0x160) > [ 126.292839] [<c05df134>] (driver_probe_device) from [<c05df3d8>] > (device_driver_attach+0x58/0x60) > [ 126.301751] [<c05df3d8>] (device_driver_attach) from [<c05df438>] > (__driver_attach+0x58/0xcc) > [ 126.310314] [<c05df438>] (__driver_attach) from [<c05dd264>] > (bus_for_each_dev+0x70/0xb4) > [ 126.318529] [<c05dd264>] (bus_for_each_dev) from [<c05de2ac>] > (bus_add_driver+0x198/0x1d0) > [ 126.326831] [<c05de2ac>] (bus_add_driver) from [<c05dfea0>] > (driver_register+0x74/0x108) > [ 126.334959] [<c05dfea0>] (driver_register) from [<c0102e80>] > (do_one_initcall+0x48/0x224) > [ 126.343177] [<c0102e80>] (do_one_initcall) from [<c01d6fa4>] > (do_init_module+0x5c/0x234) > [ 126.351307] [<c01d6fa4>] (do_init_module) from [<c01d9404>] > (load_module+0x2200/0x24d0) > [ 126.359347] [<c01d9404>] (load_module) from [<c01d9928>] > (sys_finit_module+0xbc/0xdc) > [ 126.367213] [<c01d9928>] (sys_finit_module) from [<c01011e0>] > (__sys_trace_return+0x0/0x20) > [ 126.375598] Exception stack(0xebc99fa8 to 0xebc99ff0) > [ 126.380671] 9fa0: 00000000 00035160 00000003 > 00035150 00000000 00035ee8 > [ 126.388885] 9fc0: 00000000 00035160 00000000 0000017b 00000007 > 00000003 00035150 be8e3c2c > [ 126.397095] 9fe0: be8e3a80 be8e3a70 0001b42d b6e4a1b0 > [ 126.402166] ---[ end trace 9ba6f4788aad890b ]--- > [ 126.406896] omap-iommu 4a066000.mmu: 4a066000.mmu: version 2.0 > [ 126.412765] omap_iommu_test iommu_test: Mapping da 0xa0100000, pa > 0x95100000, len 0x100000 > [ 126.421196] omap_iommu_test iommu_test: Mapping da 0xa0200000, pa > 0x95200000, len 0x100000 > [ 126.429622] omap_iommu_test iommu_test: Mapping da 0xa0300000, pa > 0x95300000, len 0x100000 > [ 126.437997] omap_iommu_test iommu_test: Mapping da 0xa0400000, pa > 0x95400000, len 0x100000 > > The fix for that is actually doing this poll bailout in _enable rather > than disable. I would rather see these fixed first before the ti-sysc > conversions and logic are vetted. > >> >> Going down, reset must be asserted post disabling clocks, which results >> a timeout if the idle status check is not bypassed. >> >> This is kind of not perfect and should be fixed later to somehow add a >> direct link between the clock and reset lines, so that we know when >> there is dependency between the two and can check the status of both to >> see if we should poll something or not. > > Yeah, agreed. Unfortunately, I do not there is a clean way of doing this > given that typically clocks and resets are treated and managed by > separate subsystems in kernel. You will always end up with a quirk flags > like this in either of the drivers. > >> >>> >>> On the downstream kernel, we have reused the existing NO_IDLEST flag as >>> a quirk within both the enable and disable functions for the IPs with >>> hardreset lines, and this patch seems to introduce a new NO_IDLE_POLL >>> flag but only during the disable path. >> >> The NO_IDLEST patch is not perfect, as it introduces a timing hazard >> where while enabling the module one can access the IP registers before >> it has left idle, leading into a crash. > > Both these flag macro names are misnomers IMO, the IP registers cannot > be accessed without releasing the resets and clocks on the IPs with the > hard-reset lines. > > regards > Suman > > [1] http://www.ti.com/lit/pdf/sprui30 > [2] https://github.com/sumananna/omap-test-iommu > >> >> -Tero >> >>> >>> regards >>> Suman >>> >>> If the disable phase is polled with these clocks, it will >>>> result in a timeout. To fix this, add logic for polling the clock >>>> activity only during enable, and add a new flag for this purpose. >>>> >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> drivers/clk/ti/clkctrl.c | 5 ++++- >>>> drivers/clk/ti/clock.h | 1 + >>>> 2 files changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c >>>> index 975995e..f5517a8 100644 >>>> --- a/drivers/clk/ti/clkctrl.c >>>> +++ b/drivers/clk/ti/clkctrl.c >>>> @@ -25,6 +25,7 @@ >>>> #include "clock.h" >>>> #define NO_IDLEST 0x1 >>>> +#define NO_IDLE_POLL 0x2 >>>> #define OMAP4_MODULEMODE_MASK 0x3 >>>> @@ -187,7 +188,7 @@ static void _omap4_clkctrl_clk_disable(struct >>>> clk_hw *hw) >>>> ti_clk_ll_ops->clk_writel(val, &clk->enable_reg); >>>> - if (clk->flags & NO_IDLEST) >>>> + if (clk->flags & (NO_IDLEST | NO_IDLE_POLL)) >>>> goto exit; >>>> /* Wait until module is disabled */ >>>> @@ -597,6 +598,8 @@ static void __init _ti_omap4_clkctrl_setup(struct >>>> device_node *node) >>>> hw->enable_bit = MODULEMODE_HWCTRL; >>>> if (reg_data->flags & CLKF_NO_IDLEST) >>>> hw->flags |= NO_IDLEST; >>>> + if (reg_data->flags & CLKF_NO_IDLE_POLL) >>>> + hw->flags |= NO_IDLE_POLL; >>>> if (reg_data->clkdm_name) >>>> hw->clkdm_name = reg_data->clkdm_name; >>>> diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h >>>> index e4b8392..6410ff6 100644 >>>> --- a/drivers/clk/ti/clock.h >>>> +++ b/drivers/clk/ti/clock.h >>>> @@ -82,6 +82,7 @@ enum { >>>> #define CLKF_SW_SUP BIT(5) >>>> #define CLKF_HW_SUP BIT(6) >>>> #define CLKF_NO_IDLEST BIT(7) >>>> +#define CLKF_NO_IDLE_POLL BIT(8) >>>> #define CLKF_SOC_MASK GENMASK(11, 8) >>>> >>> >> >> -- > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] clk: ti: dra7xx: remove idlest polling from disabling ipu/dsp clocks 2019-08-07 13:04 [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Tero Kristo 2019-08-07 13:04 ` [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only Tero Kristo @ 2019-08-07 13:04 ` Tero Kristo 2019-08-07 22:50 ` Suman Anna 2019-08-07 13:04 ` [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias Tero Kristo 2019-08-07 22:25 ` [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Stephen Boyd 3 siblings, 1 reply; 18+ messages in thread From: Tero Kristo @ 2019-08-07 13:04 UTC (permalink / raw) To: linux-clk, sboyd, mturquette; +Cc: linux-omap, tony, s-anna These cause some unwanted timeouts in the kernel, as they depend on reset and the execution state of the remotecore itself. These details should be handled by the driver with proper sequencing of events. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/clk/ti/clk-7xx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c index b57fe09..5208eb8 100644 --- a/drivers/clk/ti/clk-7xx.c +++ b/drivers/clk/ti/clk-7xx.c @@ -25,7 +25,7 @@ }; static const struct omap_clkctrl_reg_data dra7_dsp1_clkctrl_regs[] __initconst = { - { DRA7_DSP1_MMU0_DSP1_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_dsp_m2_ck" }, + { DRA7_DSP1_MMU0_DSP1_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_dsp_m2_ck" }, { 0 }, }; @@ -41,7 +41,7 @@ }; static const struct omap_clkctrl_reg_data dra7_ipu1_clkctrl_regs[] __initconst = { - { DRA7_IPU1_MMU_IPU1_CLKCTRL, dra7_mmu_ipu1_bit_data, CLKF_HW_SUP, "ipu1-clkctrl:0000:24" }, + { DRA7_IPU1_MMU_IPU1_CLKCTRL, dra7_mmu_ipu1_bit_data, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "ipu1-clkctrl:0000:24" }, { 0 }, }; @@ -137,7 +137,7 @@ }; static const struct omap_clkctrl_reg_data dra7_dsp2_clkctrl_regs[] __initconst = { - { DRA7_DSP2_MMU0_DSP2_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_dsp_m2_ck" }, + { DRA7_DSP2_MMU0_DSP2_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_dsp_m2_ck" }, { 0 }, }; @@ -164,7 +164,7 @@ }; static const struct omap_clkctrl_reg_data dra7_ipu2_clkctrl_regs[] __initconst = { - { DRA7_IPU2_MMU_IPU2_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_core_h22x2_ck" }, + { DRA7_IPU2_MMU_IPU2_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_core_h22x2_ck" }, { 0 }, }; -- 1.9.1 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] clk: ti: dra7xx: remove idlest polling from disabling ipu/dsp clocks 2019-08-07 13:04 ` [PATCH 2/3] clk: ti: dra7xx: remove idlest polling from disabling ipu/dsp clocks Tero Kristo @ 2019-08-07 22:50 ` Suman Anna 2019-08-19 9:19 ` Tero Kristo 0 siblings, 1 reply; 18+ messages in thread From: Suman Anna @ 2019-08-07 22:50 UTC (permalink / raw) To: Tero Kristo, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony Hi Tero, On 8/7/19 8:04 AM, Tero Kristo wrote: > These cause some unwanted timeouts in the kernel, as they depend on > reset and the execution state of the remotecore itself. These details > should be handled by the driver with proper sequencing of events. This can definitely do with some better patch description. What about the changes to the OMAP4 and OMAP5 files? Please see my equivalent downstream patches [1][2][3] for the same though they are using the CLKF_NO_IDLEST flag. regards Suman [1] OMAP4: http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=1979318da6f76809a5e6d652f814b2e80836aa21 [2] OMAP5: http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=69b31b56ceffdec3aed5b0feaa06090f8ee318b6 [3] DRA7: http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=a26129c10cda1d64bec3cd7a03b9acd447df84ea (portions of this patch content are already upstream, the delta being these additional flags). > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/ti/clk-7xx.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c > index b57fe09..5208eb8 100644 > --- a/drivers/clk/ti/clk-7xx.c > +++ b/drivers/clk/ti/clk-7xx.c > @@ -25,7 +25,7 @@ > }; > > static const struct omap_clkctrl_reg_data dra7_dsp1_clkctrl_regs[] __initconst = { > - { DRA7_DSP1_MMU0_DSP1_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_dsp_m2_ck" }, > + { DRA7_DSP1_MMU0_DSP1_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_dsp_m2_ck" }, > { 0 }, > }; > > @@ -41,7 +41,7 @@ > }; > > static const struct omap_clkctrl_reg_data dra7_ipu1_clkctrl_regs[] __initconst = { > - { DRA7_IPU1_MMU_IPU1_CLKCTRL, dra7_mmu_ipu1_bit_data, CLKF_HW_SUP, "ipu1-clkctrl:0000:24" }, > + { DRA7_IPU1_MMU_IPU1_CLKCTRL, dra7_mmu_ipu1_bit_data, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "ipu1-clkctrl:0000:24" }, > { 0 }, > }; > > @@ -137,7 +137,7 @@ > }; > > static const struct omap_clkctrl_reg_data dra7_dsp2_clkctrl_regs[] __initconst = { > - { DRA7_DSP2_MMU0_DSP2_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_dsp_m2_ck" }, > + { DRA7_DSP2_MMU0_DSP2_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_dsp_m2_ck" }, > { 0 }, > }; > > @@ -164,7 +164,7 @@ > }; > > static const struct omap_clkctrl_reg_data dra7_ipu2_clkctrl_regs[] __initconst = { > - { DRA7_IPU2_MMU_IPU2_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_core_h22x2_ck" }, > + { DRA7_IPU2_MMU_IPU2_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_core_h22x2_ck" }, > { 0 }, > }; > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] clk: ti: dra7xx: remove idlest polling from disabling ipu/dsp clocks 2019-08-07 22:50 ` Suman Anna @ 2019-08-19 9:19 ` Tero Kristo 2019-08-19 21:11 ` Suman Anna 0 siblings, 1 reply; 18+ messages in thread From: Tero Kristo @ 2019-08-19 9:19 UTC (permalink / raw) To: Suman Anna, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony On 08/08/2019 01:50, Suman Anna wrote: > Hi Tero, > > On 8/7/19 8:04 AM, Tero Kristo wrote: >> These cause some unwanted timeouts in the kernel, as they depend on >> reset and the execution state of the remotecore itself. These details >> should be handled by the driver with proper sequencing of events. > > This can definitely do with some better patch description. Can you elaborate a bit what you are missing here? > What about > the changes to the OMAP4 and OMAP5 files? Please see my equivalent > downstream patches [1][2][3] for the same though they are using the > CLKF_NO_IDLEST flag. I did not want to touch OMAP4/OMAP5 for now, they can be fixed with trivial data only patches separately once the approach has been approved on one device (DRA7.) -Tero > > regards > Suman > > [1] OMAP4: > http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=1979318da6f76809a5e6d652f814b2e80836aa21 > [2] OMAP5: > http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=69b31b56ceffdec3aed5b0feaa06090f8ee318b6 > [3] DRA7: > http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=a26129c10cda1d64bec3cd7a03b9acd447df84ea > (portions of this patch content are already upstream, the delta being > these additional flags). > >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/clk/ti/clk-7xx.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c >> index b57fe09..5208eb8 100644 >> --- a/drivers/clk/ti/clk-7xx.c >> +++ b/drivers/clk/ti/clk-7xx.c >> @@ -25,7 +25,7 @@ >> }; >> >> static const struct omap_clkctrl_reg_data dra7_dsp1_clkctrl_regs[] __initconst = { >> - { DRA7_DSP1_MMU0_DSP1_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_dsp_m2_ck" }, >> + { DRA7_DSP1_MMU0_DSP1_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_dsp_m2_ck" }, >> { 0 }, >> }; >> >> @@ -41,7 +41,7 @@ >> }; >> >> static const struct omap_clkctrl_reg_data dra7_ipu1_clkctrl_regs[] __initconst = { >> - { DRA7_IPU1_MMU_IPU1_CLKCTRL, dra7_mmu_ipu1_bit_data, CLKF_HW_SUP, "ipu1-clkctrl:0000:24" }, >> + { DRA7_IPU1_MMU_IPU1_CLKCTRL, dra7_mmu_ipu1_bit_data, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "ipu1-clkctrl:0000:24" }, >> { 0 }, >> }; >> >> @@ -137,7 +137,7 @@ >> }; >> >> static const struct omap_clkctrl_reg_data dra7_dsp2_clkctrl_regs[] __initconst = { >> - { DRA7_DSP2_MMU0_DSP2_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_dsp_m2_ck" }, >> + { DRA7_DSP2_MMU0_DSP2_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_dsp_m2_ck" }, >> { 0 }, >> }; >> >> @@ -164,7 +164,7 @@ >> }; >> >> static const struct omap_clkctrl_reg_data dra7_ipu2_clkctrl_regs[] __initconst = { >> - { DRA7_IPU2_MMU_IPU2_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_core_h22x2_ck" }, >> + { DRA7_IPU2_MMU_IPU2_CLKCTRL, NULL, CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "dpll_core_h22x2_ck" }, >> { 0 }, >> }; >> >> > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] clk: ti: dra7xx: remove idlest polling from disabling ipu/dsp clocks 2019-08-19 9:19 ` Tero Kristo @ 2019-08-19 21:11 ` Suman Anna 0 siblings, 0 replies; 18+ messages in thread From: Suman Anna @ 2019-08-19 21:11 UTC (permalink / raw) To: Tero Kristo, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony Hi Tero, On 8/19/19 4:19 AM, Tero Kristo wrote: > On 08/08/2019 01:50, Suman Anna wrote: >> Hi Tero, >> >> On 8/7/19 8:04 AM, Tero Kristo wrote: >>> These cause some unwanted timeouts in the kernel, as they depend on >>> reset and the execution state of the remotecore itself. These details >>> should be handled by the driver with proper sequencing of events. >> >> This can definitely do with some better patch description. > > Can you elaborate a bit what you are missing here? Well the above explanation is vague. Please see the descriptions from my patches referenced below. > >> What about >> the changes to the OMAP4 and OMAP5 files? Please see my equivalent >> downstream patches [1][2][3] for the same though they are using the >> CLKF_NO_IDLEST flag. > > I did not want to touch OMAP4/OMAP5 for now, they can be fixed with > trivial data only patches separately once the approach has been approved > on one device (DRA7.) We might as well as fix all of them while doing the development. I do not see any advantage in mixing them up while we go through the hwmod to ti-sysc conversion. There are multiple subsystems involved, and so you have to ensure functionality is not broken with individual series during the transition. regards Suman > > -Tero > >> >> regards >> Suman >> >> [1] OMAP4: >> http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=1979318da6f76809a5e6d652f814b2e80836aa21 >> >> [2] OMAP5: >> http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=69b31b56ceffdec3aed5b0feaa06090f8ee318b6 >> >> [3] DRA7: >> http://git.ti.com/gitweb/?p=rpmsg/iommu.git;a=commit;h=a26129c10cda1d64bec3cd7a03b9acd447df84ea >> >> (portions of this patch content are already upstream, the delta being >> these additional flags). >> >>> >>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>> --- >>> drivers/clk/ti/clk-7xx.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c >>> index b57fe09..5208eb8 100644 >>> --- a/drivers/clk/ti/clk-7xx.c >>> +++ b/drivers/clk/ti/clk-7xx.c >>> @@ -25,7 +25,7 @@ >>> }; >>> static const struct omap_clkctrl_reg_data >>> dra7_dsp1_clkctrl_regs[] __initconst = { >>> - { DRA7_DSP1_MMU0_DSP1_CLKCTRL, NULL, CLKF_HW_SUP, >>> "dpll_dsp_m2_ck" }, >>> + { DRA7_DSP1_MMU0_DSP1_CLKCTRL, NULL, CLKF_HW_SUP | >>> CLKF_NO_IDLE_POLL, "dpll_dsp_m2_ck" }, >>> { 0 }, >>> }; >>> @@ -41,7 +41,7 @@ >>> }; >>> static const struct omap_clkctrl_reg_data >>> dra7_ipu1_clkctrl_regs[] __initconst = { >>> - { DRA7_IPU1_MMU_IPU1_CLKCTRL, dra7_mmu_ipu1_bit_data, >>> CLKF_HW_SUP, "ipu1-clkctrl:0000:24" }, >>> + { DRA7_IPU1_MMU_IPU1_CLKCTRL, dra7_mmu_ipu1_bit_data, >>> CLKF_HW_SUP | CLKF_NO_IDLE_POLL, "ipu1-clkctrl:0000:24" }, >>> { 0 }, >>> }; >>> @@ -137,7 +137,7 @@ >>> }; >>> static const struct omap_clkctrl_reg_data >>> dra7_dsp2_clkctrl_regs[] __initconst = { >>> - { DRA7_DSP2_MMU0_DSP2_CLKCTRL, NULL, CLKF_HW_SUP, >>> "dpll_dsp_m2_ck" }, >>> + { DRA7_DSP2_MMU0_DSP2_CLKCTRL, NULL, CLKF_HW_SUP | >>> CLKF_NO_IDLE_POLL, "dpll_dsp_m2_ck" }, >>> { 0 }, >>> }; >>> @@ -164,7 +164,7 @@ >>> }; >>> static const struct omap_clkctrl_reg_data >>> dra7_ipu2_clkctrl_regs[] __initconst = { >>> - { DRA7_IPU2_MMU_IPU2_CLKCTRL, NULL, CLKF_HW_SUP, >>> "dpll_core_h22x2_ck" }, >>> + { DRA7_IPU2_MMU_IPU2_CLKCTRL, NULL, CLKF_HW_SUP | >>> CLKF_NO_IDLE_POLL, "dpll_core_h22x2_ck" }, >>> { 0 }, >>> }; >>> >> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias 2019-08-07 13:04 [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Tero Kristo 2019-08-07 13:04 ` [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only Tero Kristo 2019-08-07 13:04 ` [PATCH 2/3] clk: ti: dra7xx: remove idlest polling from disabling ipu/dsp clocks Tero Kristo @ 2019-08-07 13:04 ` Tero Kristo 2019-08-07 22:56 ` Suman Anna 2019-08-07 22:25 ` [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Stephen Boyd 3 siblings, 1 reply; 18+ messages in thread From: Tero Kristo @ 2019-08-07 13:04 UTC (permalink / raw) To: linux-clk, sboyd, mturquette; +Cc: linux-omap, tony, s-anna This is needed by the TI DM timer driver. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/clk/ti/clk-7xx.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c index 5208eb8..64507b8 100644 --- a/drivers/clk/ti/clk-7xx.c +++ b/drivers/clk/ti/clk-7xx.c @@ -792,6 +792,7 @@ static struct ti_dt_clk dra7xx_clks[] = { DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"), DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"), + DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"), DT_CLK(NULL, "sys_clkin", "sys_clkin1"), DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"), DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"), -- 1.9.1 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias 2019-08-07 13:04 ` [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias Tero Kristo @ 2019-08-07 22:56 ` Suman Anna 2019-08-19 21:14 ` Suman Anna 0 siblings, 1 reply; 18+ messages in thread From: Suman Anna @ 2019-08-07 22:56 UTC (permalink / raw) To: Tero Kristo, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony Hi Tero, On 8/7/19 8:04 AM, Tero Kristo wrote: > This is needed by the TI DM timer driver. Again can do with some better patch descriptions. Similar to the previous patch, missing the equivalent patches for OMAP4 and OMAP5. You can use my downstream patches for these - [1][2][3] that has all the needed Fixes by details. Only difference is that you used a single line change on DRA7, and this should suffice since all the sources are same, but OMAP4 and OMAP5 needed different ones. [1] OMAP4: http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f [2] OMAP5: http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2 [3] DRA7: http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058 Technically, this data need to be added back for all OMAP2+ SoCs which support dmtimer with any other drivers wanting to use the timers. regards Suman > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/ti/clk-7xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c > index 5208eb8..64507b8 100644 > --- a/drivers/clk/ti/clk-7xx.c > +++ b/drivers/clk/ti/clk-7xx.c > @@ -792,6 +792,7 @@ > static struct ti_dt_clk dra7xx_clks[] = { > DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"), > DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"), > + DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"), > DT_CLK(NULL, "sys_clkin", "sys_clkin1"), > DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"), > DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"), > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias 2019-08-07 22:56 ` Suman Anna @ 2019-08-19 21:14 ` Suman Anna 2019-08-23 18:16 ` Tero Kristo 0 siblings, 1 reply; 18+ messages in thread From: Suman Anna @ 2019-08-19 21:14 UTC (permalink / raw) To: Kristo, Tero, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony Hi Tero, On 8/7/19 5:56 PM, Anna, Suman wrote: > Hi Tero, > > On 8/7/19 8:04 AM, Tero Kristo wrote: >> This is needed by the TI DM timer driver. > > Again can do with some better patch descriptions. Similar to the > previous patch, missing the equivalent patches for OMAP4 and OMAP5. > You can use my downstream patches for these - [1][2][3] that has all the > needed Fixes by details. Only difference is that you used a single line > change on DRA7, and this should suffice since all the sources are same, > but OMAP4 and OMAP5 needed different ones. > > [1] OMAP4: > http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f > [2] OMAP5: > http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2 > [3] DRA7: > http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058 > > Technically, this data need to be added back for all OMAP2+ SoCs which > support dmtimer with any other drivers wanting to use the timers. So, I checked and these aliases already are defined on OMAP2, OMAP3, AM33xx, AM43xx, DM814x and DM816x SoCs. So, just include the OMAP4 and OMAP5 ones along with the DRA7x one. regards Suman > > regards > Suman > >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/clk/ti/clk-7xx.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c >> index 5208eb8..64507b8 100644 >> --- a/drivers/clk/ti/clk-7xx.c >> +++ b/drivers/clk/ti/clk-7xx.c >> @@ -792,6 +792,7 @@ >> static struct ti_dt_clk dra7xx_clks[] = { >> DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"), >> DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"), >> + DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"), >> DT_CLK(NULL, "sys_clkin", "sys_clkin1"), >> DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"), >> DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"), >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias 2019-08-19 21:14 ` Suman Anna @ 2019-08-23 18:16 ` Tero Kristo 2019-08-26 22:26 ` Suman Anna 0 siblings, 1 reply; 18+ messages in thread From: Tero Kristo @ 2019-08-23 18:16 UTC (permalink / raw) To: Suman Anna, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony On 20.8.2019 0.14, Suman Anna wrote: > Hi Tero, > > On 8/7/19 5:56 PM, Anna, Suman wrote: >> Hi Tero, >> >> On 8/7/19 8:04 AM, Tero Kristo wrote: >>> This is needed by the TI DM timer driver. >> >> Again can do with some better patch descriptions. Similar to the >> previous patch, missing the equivalent patches for OMAP4 and OMAP5. >> You can use my downstream patches for these - [1][2][3] that has all the >> needed Fixes by details. Only difference is that you used a single line >> change on DRA7, and this should suffice since all the sources are same, >> but OMAP4 and OMAP5 needed different ones. >> >> [1] OMAP4: >> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f >> [2] OMAP5: >> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2 >> [3] DRA7: >> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058 >> >> Technically, this data need to be added back for all OMAP2+ SoCs which >> support dmtimer with any other drivers wanting to use the timers. > > So, I checked and these aliases already are defined on OMAP2, OMAP3, > AM33xx, AM43xx, DM814x and DM816x SoCs. So, just include the OMAP4 and > OMAP5 ones along with the DRA7x one. Actually, all these alias definitions can be completely removed, and can be replaced with DT data. Here's sample how it can be done for dra7xx timer11: diff --git a/arch/arm/boot/dts/dra7-l4.dtsi b/arch/arm/boot/dts/dra7-l4.dtsi index bed67603c186..fafa0a131af0 100644 --- a/arch/arm/boot/dts/dra7-l4.dtsi +++ b/arch/arm/boot/dts/dra7-l4.dtsi @@ -1910,8 +1910,8 @@ timer11: timer@0 { compatible = "ti,omap5430-timer"; reg = <0x0 0x80>; - clocks = <&l4per_clkctrl DRA7_L4PER_TIMER11_CLKCTRL 24>; - clock-names = "fck"; + clocks = <&l4per_clkctrl DRA7_L4PER_TIMER11_CLKCTRL 24>, <&timer_sys_clk_div>; + clock-names = "fck", "timer_sys_ck"; interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; }; }; I will post these changes along with other DTS patches once the time is right. For now, I will just drop these aliases completely. -Tero > > regards > Suman > >> >> regards >> Suman >> >>> >>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>> --- >>> drivers/clk/ti/clk-7xx.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c >>> index 5208eb8..64507b8 100644 >>> --- a/drivers/clk/ti/clk-7xx.c >>> +++ b/drivers/clk/ti/clk-7xx.c >>> @@ -792,6 +792,7 @@ >>> static struct ti_dt_clk dra7xx_clks[] = { >>> DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"), >>> DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"), >>> + DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"), >>> DT_CLK(NULL, "sys_clkin", "sys_clkin1"), >>> DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"), >>> DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"), >>> >> > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias 2019-08-23 18:16 ` Tero Kristo @ 2019-08-26 22:26 ` Suman Anna 2019-08-27 5:57 ` Tero Kristo 0 siblings, 1 reply; 18+ messages in thread From: Suman Anna @ 2019-08-26 22:26 UTC (permalink / raw) To: Tero Kristo, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony Hi Tero, On 8/23/19 1:16 PM, Tero Kristo wrote: >>> >>> On 8/7/19 8:04 AM, Tero Kristo wrote: >>>> This is needed by the TI DM timer driver. >>> >>> Again can do with some better patch descriptions. Similar to the >>> previous patch, missing the equivalent patches for OMAP4 and OMAP5. >>> You can use my downstream patches for these - [1][2][3] that has all the >>> needed Fixes by details. Only difference is that you used a single line >>> change on DRA7, and this should suffice since all the sources are same, >>> but OMAP4 and OMAP5 needed different ones. >>> >>> [1] OMAP4: >>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f >>> >>> [2] OMAP5: >>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2 >>> >>> [3] DRA7: >>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058 >>> >>> >>> Technically, this data need to be added back for all OMAP2+ SoCs which >>> support dmtimer with any other drivers wanting to use the timers. >> >> So, I checked and these aliases already are defined on OMAP2, OMAP3, >> AM33xx, AM43xx, DM814x and DM816x SoCs. So, just include the OMAP4 and >> OMAP5 ones along with the DRA7x one. > > Actually, all these alias definitions can be completely removed, and can > be replaced with DT data. Here's sample how it can be done for dra7xx > timer11: > > diff --git a/arch/arm/boot/dts/dra7-l4.dtsi > b/arch/arm/boot/dts/dra7-l4.dtsi > index bed67603c186..fafa0a131af0 100644 > --- a/arch/arm/boot/dts/dra7-l4.dtsi > +++ b/arch/arm/boot/dts/dra7-l4.dtsi > @@ -1910,8 +1910,8 @@ > timer11: timer@0 { > compatible = "ti,omap5430-timer"; > reg = <0x0 0x80>; > - clocks = <&l4per_clkctrl > DRA7_L4PER_TIMER11_CLKCTRL 24>; > - clock-names = "fck"; > + clocks = <&l4per_clkctrl > DRA7_L4PER_TIMER11_CLKCTRL 24>, <&timer_sys_clk_div>; > + clock-names = "fck", "timer_sys_ck"; > interrupts = <GIC_SPI 42 > IRQ_TYPE_LEVEL_HIGH>; > }; > }; > > I will post these changes along with other DTS patches once the time is > right. For now, I will just drop these aliases completely. I am not sure if this is gonna buy us anything and if it is scalable. The added clock is neither a functional clock nor an optional clock of the timer device, but is just a name to use to set the clock parent. Are you going to add the aliases from clk-<soc>.h to all the device nodes? DRA7 dmtimers can actually be parented from one of 13 clocks (driver was never updated to support those). Given that the dmtimers can only be requested using phandle on DT boots, it is possible to eliminate the naming and rely on assigned-clock-parents in either the dmtimer nodes or the client nodes (provided all the clock parents are listed in dts), and eliminate this set_source logic. regards Suman > > -Tero > >> >> regards >> Suman >> >>> >>> regards >>> Suman >>> >>>> >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> drivers/clk/ti/clk-7xx.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c >>>> index 5208eb8..64507b8 100644 >>>> --- a/drivers/clk/ti/clk-7xx.c >>>> +++ b/drivers/clk/ti/clk-7xx.c >>>> @@ -792,6 +792,7 @@ >>>> static struct ti_dt_clk dra7xx_clks[] = { >>>> DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"), >>>> DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"), >>>> + DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"), >>>> DT_CLK(NULL, "sys_clkin", "sys_clkin1"), >>>> DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"), >>>> DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"), >>>> >>> >> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias 2019-08-26 22:26 ` Suman Anna @ 2019-08-27 5:57 ` Tero Kristo 0 siblings, 0 replies; 18+ messages in thread From: Tero Kristo @ 2019-08-27 5:57 UTC (permalink / raw) To: Suman Anna, linux-clk, sboyd, mturquette; +Cc: linux-omap, tony On 27.8.2019 1.26, Suman Anna wrote: > Hi Tero, > > On 8/23/19 1:16 PM, Tero Kristo wrote: >>>> >>>> On 8/7/19 8:04 AM, Tero Kristo wrote: >>>>> This is needed by the TI DM timer driver. >>>> >>>> Again can do with some better patch descriptions. Similar to the >>>> previous patch, missing the equivalent patches for OMAP4 and OMAP5. >>>> You can use my downstream patches for these - [1][2][3] that has all the >>>> needed Fixes by details. Only difference is that you used a single line >>>> change on DRA7, and this should suffice since all the sources are same, >>>> but OMAP4 and OMAP5 needed different ones. >>>> >>>> [1] OMAP4: >>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f >>>> >>>> [2] OMAP5: >>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2 >>>> >>>> [3] DRA7: >>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058 >>>> >>>> >>>> Technically, this data need to be added back for all OMAP2+ SoCs which >>>> support dmtimer with any other drivers wanting to use the timers. >>> >>> So, I checked and these aliases already are defined on OMAP2, OMAP3, >>> AM33xx, AM43xx, DM814x and DM816x SoCs. So, just include the OMAP4 and >>> OMAP5 ones along with the DRA7x one. >> >> Actually, all these alias definitions can be completely removed, and can >> be replaced with DT data. Here's sample how it can be done for dra7xx >> timer11: >> >> diff --git a/arch/arm/boot/dts/dra7-l4.dtsi >> b/arch/arm/boot/dts/dra7-l4.dtsi >> index bed67603c186..fafa0a131af0 100644 >> --- a/arch/arm/boot/dts/dra7-l4.dtsi >> +++ b/arch/arm/boot/dts/dra7-l4.dtsi >> @@ -1910,8 +1910,8 @@ >> timer11: timer@0 { >> compatible = "ti,omap5430-timer"; >> reg = <0x0 0x80>; >> - clocks = <&l4per_clkctrl >> DRA7_L4PER_TIMER11_CLKCTRL 24>; >> - clock-names = "fck"; >> + clocks = <&l4per_clkctrl >> DRA7_L4PER_TIMER11_CLKCTRL 24>, <&timer_sys_clk_div>; >> + clock-names = "fck", "timer_sys_ck"; >> interrupts = <GIC_SPI 42 >> IRQ_TYPE_LEVEL_HIGH>; >> }; >> }; >> >> I will post these changes along with other DTS patches once the time is >> right. For now, I will just drop these aliases completely. > > I am not sure if this is gonna buy us anything and if it is scalable. > The added clock is neither a functional clock nor an optional clock of > the timer device, but is just a name to use to set the clock parent. Are > you going to add the aliases from clk-<soc>.h to all the device nodes? > DRA7 dmtimers can actually be parented from one of 13 clocks (driver was > never updated to support those). No, adding all of these has no point. > > Given that the dmtimers can only be requested using phandle on DT boots, > it is possible to eliminate the naming and rely on > assigned-clock-parents in either the dmtimer nodes or the client nodes > (provided all the clock parents are listed in dts), and eliminate this > set_source logic. Either way works, however the alias mechanism provided inside the TI clock driver was meant to be temporary only when it was introduced a few years back... Any clock handles needed by drivers should be provided via DT. If you need re-parenting of things, using assigned-clocks would be ideal. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] clk: ti: couple of fixes towards 5.4 2019-08-07 13:04 [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Tero Kristo ` (2 preceding siblings ...) 2019-08-07 13:04 ` [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias Tero Kristo @ 2019-08-07 22:25 ` Stephen Boyd 2019-08-19 9:17 ` Tero Kristo 3 siblings, 1 reply; 18+ messages in thread From: Stephen Boyd @ 2019-08-07 22:25 UTC (permalink / raw) To: Tero Kristo, linux-clk, mturquette; +Cc: linux-omap, tony, s-anna Quoting Tero Kristo (2019-08-07 06:04:36) > Hi, > > Here are some TI clock fixes which can be queued for 5.4. These are > needed for getting remoteproc functionality working properly, as these > depend on reset handling also and timing out with clocks is bad for > them. The timer clock alias fix is needed for the same, as remoteprocs > depend on certain HW timers for their functionality. > Looks ok to me. Are you going to add Fixes tags to any? Should I expect a PR or you want me to pick them up directly? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] clk: ti: couple of fixes towards 5.4 2019-08-07 22:25 ` [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Stephen Boyd @ 2019-08-19 9:17 ` Tero Kristo 0 siblings, 0 replies; 18+ messages in thread From: Tero Kristo @ 2019-08-19 9:17 UTC (permalink / raw) To: Stephen Boyd, linux-clk, mturquette; +Cc: linux-omap, tony, s-anna On 08/08/2019 01:25, Stephen Boyd wrote: > Quoting Tero Kristo (2019-08-07 06:04:36) >> Hi, >> >> Here are some TI clock fixes which can be queued for 5.4. These are >> needed for getting remoteproc functionality working properly, as these >> depend on reset handling also and timing out with clocks is bad for >> them. The timer clock alias fix is needed for the same, as remoteprocs >> depend on certain HW timers for their functionality. >> > > Looks ok to me. Are you going to add Fixes tags to any? Should I expect > a PR or you want me to pick them up directly? Either way is fine, let me resolve the comments from Suman before proceeding with these. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-08-27 5:57 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-07 13:04 [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Tero Kristo 2019-08-07 13:04 ` [PATCH 1/3] clk: ti: clkctrl: add support for polling clock status for enable only Tero Kristo 2019-08-07 22:43 ` Suman Anna 2019-08-19 9:13 ` Tero Kristo 2019-08-19 21:07 ` Suman Anna 2019-08-20 8:17 ` Tero Kristo 2019-08-07 13:04 ` [PATCH 2/3] clk: ti: dra7xx: remove idlest polling from disabling ipu/dsp clocks Tero Kristo 2019-08-07 22:50 ` Suman Anna 2019-08-19 9:19 ` Tero Kristo 2019-08-19 21:11 ` Suman Anna 2019-08-07 13:04 ` [PATCH 3/3] clk: ti: dra7xx: add timer_sys_ck clock alias Tero Kristo 2019-08-07 22:56 ` Suman Anna 2019-08-19 21:14 ` Suman Anna 2019-08-23 18:16 ` Tero Kristo 2019-08-26 22:26 ` Suman Anna 2019-08-27 5:57 ` Tero Kristo 2019-08-07 22:25 ` [PATCH 0/3] clk: ti: couple of fixes towards 5.4 Stephen Boyd 2019-08-19 9:17 ` Tero Kristo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).