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

[Tero Kristo <t-kristo@ti.com>]

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