| Message ID | 20221116104716.2583320-3-abel.vesa@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: qcom: Add support for SM8550 | expand |
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;
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;
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 --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;
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(-)