diff mbox series

[1/3] clk: ti: clkctrl: add support for polling clock status for enable only

Message ID 1565183079-27798-2-git-send-email-t-kristo@ti.com
State New
Headers show
Series clk: ti: couple of fixes towards 5.4 | expand

Commit Message

Tero Kristo Aug. 7, 2019, 1:04 p.m. UTC 
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(-)

-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Suman Anna Aug. 7, 2019, 10:43 p.m. UTC | #1 
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)

>  

>
Tero Kristo Aug. 19, 2019, 9:13 a.m. UTC | #2 
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
Suman Anna Aug. 19, 2019, 9:07 p.m. UTC | #3 
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
Tero Kristo Aug. 20, 2019, 8:17 a.m. UTC | #4 
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
diff mbox series

Patch

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)