2023-06-27 11:02:11

by Abel Vesa

Subject: [RFC PATCH 2/2] clk: qcom: gdsc: Add support for set_hwmode_dev

Implement the GDSC specific genpd set_hwmode_dev callback in order to
switch the HW control on or off. For any GDSC that supports HW control
set this callback in order to allow its consumers to control it.

Signed-off-by: Abel Vesa <[email protected]>
---
drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 5358e28122ab..9a04bf2e4379 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -314,6 +314,26 @@ static int gdsc_enable(struct generic_pm_domain *domain)
return 0;
}

+static int gdsc_set_hwmode_dev(struct generic_pm_domain *domain,
+ struct device *dev, bool enable)
+{
+ int ret = gdsc_hwctrl(domain_to_gdsc(domain), enable);
+
+ if (ret)
+ goto out;
+
+ /*
+ * Wait for the GDSC to go through a power down and
+ * up cycle. In case there is a status polling going on
+ * before the power cycle is completed it might read an
+ * wrong status value.
+ */
+ udelay(1);
+
+out:
+ return ret;
+}
+
static int gdsc_disable(struct generic_pm_domain *domain)
{
struct gdsc *sc = domain_to_gdsc(domain);
@@ -451,6 +471,8 @@ static int gdsc_init(struct gdsc *sc)
sc->pd.power_off = gdsc_disable;
if (!sc->pd.power_on)
sc->pd.power_on = gdsc_enable;
+ if (sc->flags & HW_CTRL)
+ sc->pd.set_hwmode_dev = gdsc_set_hwmode_dev;

ret = pm_genpd_init(&sc->pd, NULL, !on);
if (ret)
--
2.34.1

2023-06-27 11:10:54

by Konrad Dybcio

Subject: Re: [RFC PATCH 2/2] clk: qcom: gdsc: Add support for set_hwmode_dev

On 27.06.2023 12:40, Abel Vesa wrote:
> Implement the GDSC specific genpd set_hwmode_dev callback in order to
> switch the HW control on or off. For any GDSC that supports HW control
> set this callback in order to allow its consumers to control it.
>
> Signed-off-by: Abel Vesa <[email protected]>
> ---
Currently all GDSCs with flags & HW_CTRL enable hw ctrl mode implicilty.
I didn't get any cover letter with these patches.. are you planning on
retiring that behavior? Presumably after adding a matching pair of set_hwmode
in venus!

fwiw this patch lgtm

Konrad
> drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 5358e28122ab..9a04bf2e4379 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -314,6 +314,26 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> return 0;
> }
>
> +static int gdsc_set_hwmode_dev(struct generic_pm_domain *domain,
> + struct device *dev, bool enable)
> +{
> + int ret = gdsc_hwctrl(domain_to_gdsc(domain), enable);
> +
> + if (ret)
> + goto out;
> +
> + /*
> + * Wait for the GDSC to go through a power down and
> + * up cycle. In case there is a status polling going on
> + * before the power cycle is completed it might read an
> + * wrong status value.
> + */
> + udelay(1);
> +
> +out:
> + return ret;
> +}
> +
> static int gdsc_disable(struct generic_pm_domain *domain)
> {
> struct gdsc *sc = domain_to_gdsc(domain);
> @@ -451,6 +471,8 @@ static int gdsc_init(struct gdsc *sc)
> sc->pd.power_off = gdsc_disable;
> if (!sc->pd.power_on)
> sc->pd.power_on = gdsc_enable;
> + if (sc->flags & HW_CTRL)
> + sc->pd.set_hwmode_dev = gdsc_set_hwmode_dev;
>
> ret = pm_genpd_init(&sc->pd, NULL, !on);
> if (ret)

2023-06-27 12:12:33

by Abel Vesa

Subject: Re: [RFC PATCH 2/2] clk: qcom: gdsc: Add support for set_hwmode_dev

On 23-06-27 12:47:24, Konrad Dybcio wrote:
> On 27.06.2023 12:40, Abel Vesa wrote:
> > Implement the GDSC specific genpd set_hwmode_dev callback in order to
> > switch the HW control on or off. For any GDSC that supports HW control
> > set this callback in order to allow its consumers to control it.
> >
> > Signed-off-by: Abel Vesa <[email protected]>
> > ---
> Currently all GDSCs with flags & HW_CTRL enable hw ctrl mode implicilty.
> I didn't get any cover letter with these patches.. are you planning on
> retiring that behavior? Presumably after adding a matching pair of set_hwmode
> in venus!

I didn't think a cover letter was needed here. After a chat offline with
Taniya about this and it seems there is at least one consumer driver
that needs to switch back and forth the HW control bit. For the rest of
the consumers, the safest way is to assume that they expect their GDSC
to be in HW control mode from the moment it is enabled until it gets
disabled. One example of this is venus.

>
> fwiw this patch lgtm
>
> Konrad
> > drivers/clk/qcom/gdsc.c | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 5358e28122ab..9a04bf2e4379 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -314,6 +314,26 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> > return 0;
> > }
> >
> > +static int gdsc_set_hwmode_dev(struct generic_pm_domain *domain,
> > + struct device *dev, bool enable)
> > +{
> > + int ret = gdsc_hwctrl(domain_to_gdsc(domain), enable);
> > +
> > + if (ret)
> > + goto out;
> > +
> > + /*
> > + * Wait for the GDSC to go through a power down and
> > + * up cycle. In case there is a status polling going on
> > + * before the power cycle is completed it might read an
> > + * wrong status value.
> > + */
> > + udelay(1);
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > static int gdsc_disable(struct generic_pm_domain *domain)
> > {
> > struct gdsc *sc = domain_to_gdsc(domain);
> > @@ -451,6 +471,8 @@ static int gdsc_init(struct gdsc *sc)
> > sc->pd.power_off = gdsc_disable;
> > if (!sc->pd.power_on)
> > sc->pd.power_on = gdsc_enable;
> > + if (sc->flags & HW_CTRL)
> > + sc->pd.set_hwmode_dev = gdsc_set_hwmode_dev;
> >
> > ret = pm_genpd_init(&sc->pd, NULL, !on);
> > if (ret)