diff mbox series

[2/9] clk: qcom: gdsc: Add configurable poll timeout

Message ID 20221116104716.2583320-3-abel.vesa@linaro.org
State New
Headers
Series clk: qcom: Add support for SM8550 |

Commit Message

Abel Vesa Nov. 16, 2022, 10:47 a.m. UTC 
  Depending on the platform, the poll timeout delay might be different,
so allow the platform specific drivers to specify their own values.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 5 ++++-
 drivers/clk/qcom/gdsc.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Konrad Dybcio Nov. 16, 2022, 11:19 a.m. UTC | #1 
On 16/11/2022 11:47, Abel Vesa wrote:
> Depending on the platform, the poll timeout delay might be different,
> so allow the platform specific drivers to specify their own values.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>   drivers/clk/qcom/gdsc.c | 5 ++++-
>   drivers/clk/qcom/gdsc.h | 1 +
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 0f21a8a767ac..3753f3ef7241 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -107,7 +107,7 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
>   	do {
>   		if (gdsc_check_status(sc, status))
>   			return 0;
> -	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> +	} while (ktime_us_delta(ktime_get(), start) < sc->poll_timeout);
What about the second usage of TIMEOUT_US (in gdsc_toggle_logic)? Is it 
fine for that to be the default value?


Konrad
>   
>   	if (gdsc_check_status(sc, status))
>   		return 0;
> @@ -454,6 +454,9 @@ static int gdsc_init(struct gdsc *sc)
>   	if (ret)
>   		goto err_disable_supply;
>   
> +	if (!sc->poll_timeout)
> +		sc->poll_timeout = 500;
> +
>   	return 0;
>   
>   err_disable_supply:
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 803512688336..9a1e1fb3d12f 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -36,6 +36,7 @@ struct gdsc {
>   	struct generic_pm_domain	*parent;
>   	struct regmap			*regmap;
>   	unsigned int			gdscr;
> +	unsigned int			poll_timeout;
>   	unsigned int			collapse_ctrl;
>   	unsigned int			collapse_mask;
>   	unsigned int			gds_hw_ctrl;
Abel Vesa Nov. 17, 2022, 8:05 a.m. UTC | #2 
On 22-11-16 12:19:09, Konrad Dybcio wrote:
> 
> 
> On 16/11/2022 11:47, Abel Vesa wrote:
> > Depending on the platform, the poll timeout delay might be different,
> > so allow the platform specific drivers to specify their own values.
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >   drivers/clk/qcom/gdsc.c | 5 ++++-
> >   drivers/clk/qcom/gdsc.h | 1 +
> >   2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 0f21a8a767ac..3753f3ef7241 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -107,7 +107,7 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
> >   	do {
> >   		if (gdsc_check_status(sc, status))
> >   			return 0;
> > -	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
> > +	} while (ktime_us_delta(ktime_get(), start) < sc->poll_timeout);
> What about the second usage of TIMEOUT_US (in gdsc_toggle_logic)? Is it fine
> for that to be the default value?

The usleep you mention is not really for polling the state.
So I think it should stay as is. Who knows, maybe in the future we will
need to have the configurable as well, but as a toggle delay rather than
a status poll timeout.

I added this configurable poll timeout just because I saw that
downstream, each driver has different values. And it kind of makes sense,
because the state machine inside the GDSC might be different between
platforms, and so, it might take different time to reach a certain on/off
state.

Thanks,
Abel

> 
> 
> Konrad
> >   	if (gdsc_check_status(sc, status))
> >   		return 0;
> > @@ -454,6 +454,9 @@ static int gdsc_init(struct gdsc *sc)
> >   	if (ret)
> >   		goto err_disable_supply;
> > +	if (!sc->poll_timeout)
> > +		sc->poll_timeout = 500;
> > +
> >   	return 0;
> >   err_disable_supply:
> > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> > index 803512688336..9a1e1fb3d12f 100644
> > --- a/drivers/clk/qcom/gdsc.h
> > +++ b/drivers/clk/qcom/gdsc.h
> > @@ -36,6 +36,7 @@ struct gdsc {
> >   	struct generic_pm_domain	*parent;
> >   	struct regmap			*regmap;
> >   	unsigned int			gdscr;
> > +	unsigned int			poll_timeout;
> >   	unsigned int			collapse_ctrl;
> >   	unsigned int			collapse_mask;
> >   	unsigned int			gds_hw_ctrl;
Konrad Dybcio Nov. 17, 2022, 9:23 a.m. UTC | #3 
On 17/11/2022 09:05, Abel Vesa wrote:
> On 22-11-16 12:19:09, Konrad Dybcio wrote:
>>
>>
>> On 16/11/2022 11:47, Abel Vesa wrote:
>>> Depending on the platform, the poll timeout delay might be different,
>>> so allow the platform specific drivers to specify their own values.
>>>
>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>> ---
>>>    drivers/clk/qcom/gdsc.c | 5 ++++-
>>>    drivers/clk/qcom/gdsc.h | 1 +
>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>> index 0f21a8a767ac..3753f3ef7241 100644
>>> --- a/drivers/clk/qcom/gdsc.c
>>> +++ b/drivers/clk/qcom/gdsc.c
>>> @@ -107,7 +107,7 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
>>>    	do {
>>>    		if (gdsc_check_status(sc, status))
>>>    			return 0;
>>> -	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
>>> +	} while (ktime_us_delta(ktime_get(), start) < sc->poll_timeout);
>> What about the second usage of TIMEOUT_US (in gdsc_toggle_logic)? Is it fine
>> for that to be the default value?
> 
> The usleep you mention is not really for polling the state.
> So I think it should stay as is. Who knows, maybe in the future we will
> need to have the configurable as well, but as a toggle delay rather than
> a status poll timeout.
> 
> I added this configurable poll timeout just because I saw that
> downstream, each driver has different values. And it kind of makes sense,
> because the state machine inside the GDSC might be different between
> platforms, and so, it might take different time to reach a certain on/off
> state.
> 
> Thanks,
> Abel
Okay, thanks for explaining

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
> 
>>
>>
>> Konrad
>>>    	if (gdsc_check_status(sc, status))
>>>    		return 0;
>>> @@ -454,6 +454,9 @@ static int gdsc_init(struct gdsc *sc)
>>>    	if (ret)
>>>    		goto err_disable_supply;
>>> +	if (!sc->poll_timeout)
>>> +		sc->poll_timeout = 500;
>>> +
>>>    	return 0;
>>>    err_disable_supply:
>>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>>> index 803512688336..9a1e1fb3d12f 100644
>>> --- a/drivers/clk/qcom/gdsc.h
>>> +++ b/drivers/clk/qcom/gdsc.h
>>> @@ -36,6 +36,7 @@ struct gdsc {
>>>    	struct generic_pm_domain	*parent;
>>>    	struct regmap			*regmap;
>>>    	unsigned int			gdscr;
>>> +	unsigned int			poll_timeout;
>>>    	unsigned int			collapse_ctrl;
>>>    	unsigned int			collapse_mask;
>>>    	unsigned int			gds_hw_ctrl;
diff mbox series

Patch

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 0f21a8a767ac..3753f3ef7241 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -107,7 +107,7 @@  static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
 	do {
 		if (gdsc_check_status(sc, status))
 			return 0;
-	} while (ktime_us_delta(ktime_get(), start) < TIMEOUT_US);
+	} while (ktime_us_delta(ktime_get(), start) < sc->poll_timeout);
 
 	if (gdsc_check_status(sc, status))
 		return 0;
@@ -454,6 +454,9 @@  static int gdsc_init(struct gdsc *sc)
 	if (ret)
 		goto err_disable_supply;
 
+	if (!sc->poll_timeout)
+		sc->poll_timeout = 500;
+
 	return 0;
 
 err_disable_supply:
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 803512688336..9a1e1fb3d12f 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -36,6 +36,7 @@  struct gdsc {
 	struct generic_pm_domain	*parent;
 	struct regmap			*regmap;
 	unsigned int			gdscr;
+	unsigned int			poll_timeout;
 	unsigned int			collapse_ctrl;
 	unsigned int			collapse_mask;
 	unsigned int			gds_hw_ctrl;