diff mbox series

[2/8] clk: qcom: gdsc: Enable supply reglator in GPU GX handler

Message ID 20231220-sa8295p-gpu-v1-2-d8cdf2257f97@quicinc.com
State New
Headers
Series arm64: dts: qcom: sa8295p: Enable GPU |

Commit Message

Bjorn Andersson Dec. 21, 2023, 3:50 a.m. UTC 
  The GX GDSC is modelled to aid the GMU in powering down the GPU in the
event that the GPU crashes, so that it can be restarted again. But in
the event that the power-domain is supplied through a dedicated
regulator (in contrast to being a subdomin of another power-domain),
something needs to turn that regulator on, both to make sure things are
powered and to match the operation in gdsc_disable().

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/clk/qcom/gdsc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Stephen Boyd Dec. 21, 2023, 4:36 a.m. UTC | #1 
Quoting Bjorn Andersson (2023-12-20 19:50:36)
> The GX GDSC is modelled to aid the GMU in powering down the GPU in the
> event that the GPU crashes, so that it can be restarted again. But in
> the event that the power-domain is supplied through a dedicated
> regulator (in contrast to being a subdomin of another power-domain),
> something needs to turn that regulator on, both to make sure things are
> powered and to match the operation in gdsc_disable().
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/clk/qcom/gdsc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 5358e28122ab..d1139c895503 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc)
>   */
>  int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
>  {
> +       struct gdsc *sc = domain_to_gdsc(domain);
> +       int ret = 0;
> +
>         /* Do nothing but give genpd the impression that we were successful */

Update this comment.

> -       return 0;
> +       if (sc->rsupply)
> +               ret = regulator_enable(sc->rsupply);
> +
> +       return ret;
>  }
Konrad Dybcio Dec. 21, 2023, 12:57 p.m. UTC | #2 
On 21.12.2023 04:50, Bjorn Andersson wrote:
> The GX GDSC is modelled to aid the GMU in powering down the GPU in the
> event that the GPU crashes, so that it can be restarted again. But in
> the event that the power-domain is supplied through a dedicated
> regulator (in contrast to being a subdomin of another power-domain),
> something needs to turn that regulator on, both to make sure things are
> powered and to match the operation in gdsc_disable().
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/clk/qcom/gdsc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 5358e28122ab..d1139c895503 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc)
>   */
>  int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
I suppose the name is confusing now..

But at the same time I can't come up with anything that's less than
like 6 words..

Konrad
Dmitry Baryshkov Dec. 21, 2023, 1:04 p.m. UTC | #3 
On Thu, 21 Dec 2023 at 15:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 21.12.2023 04:50, Bjorn Andersson wrote:
> > The GX GDSC is modelled to aid the GMU in powering down the GPU in the
> > event that the GPU crashes, so that it can be restarted again. But in
> > the event that the power-domain is supplied through a dedicated
> > regulator (in contrast to being a subdomin of another power-domain),
> > something needs to turn that regulator on, both to make sure things are
> > powered and to match the operation in gdsc_disable().
> >
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  drivers/clk/qcom/gdsc.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 5358e28122ab..d1139c895503 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc)
> >   */
> >  int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
> I suppose the name is confusing now..
>
> But at the same time I can't come up with anything that's less than
> like 6 words..

gdsc_gx_enable() ;-)
Konrad Dybcio Dec. 21, 2023, 1:16 p.m. UTC | #4 
On 21.12.2023 14:04, Dmitry Baryshkov wrote:
> On Thu, 21 Dec 2023 at 15:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 21.12.2023 04:50, Bjorn Andersson wrote:
>>> The GX GDSC is modelled to aid the GMU in powering down the GPU in the
>>> event that the GPU crashes, so that it can be restarted again. But in
>>> the event that the power-domain is supplied through a dedicated
>>> regulator (in contrast to being a subdomin of another power-domain),
>>> something needs to turn that regulator on, both to make sure things are
>>> powered and to match the operation in gdsc_disable().
>>>
>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>> ---
>>>  drivers/clk/qcom/gdsc.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>> index 5358e28122ab..d1139c895503 100644
>>> --- a/drivers/clk/qcom/gdsc.c
>>> +++ b/drivers/clk/qcom/gdsc.c
>>> @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc)
>>>   */
>>>  int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
>> I suppose the name is confusing now..
>>
>> But at the same time I can't come up with anything that's less than
>> like 6 words..
> 
> gdsc_gx_enable() ;-)
except not really only gx and not really enable :(

gdsc_shared_enable would probably be closer to our current
nomenclature..

Konrad
Konrad Dybcio Dec. 21, 2023, 1:17 p.m. UTC | #5 
On 21.12.2023 14:16, Konrad Dybcio wrote:
> On 21.12.2023 14:04, Dmitry Baryshkov wrote:
>> On Thu, 21 Dec 2023 at 15:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>
>>> On 21.12.2023 04:50, Bjorn Andersson wrote:
>>>> The GX GDSC is modelled to aid the GMU in powering down the GPU in the
>>>> event that the GPU crashes, so that it can be restarted again. But in
>>>> the event that the power-domain is supplied through a dedicated
>>>> regulator (in contrast to being a subdomin of another power-domain),
>>>> something needs to turn that regulator on, both to make sure things are
>>>> powered and to match the operation in gdsc_disable().
>>>>
>>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>>> ---
>>>>  drivers/clk/qcom/gdsc.c | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>>>> index 5358e28122ab..d1139c895503 100644
>>>> --- a/drivers/clk/qcom/gdsc.c
>>>> +++ b/drivers/clk/qcom/gdsc.c
>>>> @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc)
>>>>   */
>>>>  int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
>>> I suppose the name is confusing now..
>>>
>>> But at the same time I can't come up with anything that's less than
>>> like 6 words..
>>
>> gdsc_gx_enable() ;-)
> except not really only gx and not really enable :(
> 
> gdsc_shared_enable would probably be closer to our current
> nomenclature..
but then VOTABLE also taps into the concept of "shared" :/

Konrad
Dmitry Baryshkov Dec. 21, 2023, 1:18 p.m. UTC | #6 
On Thu, 21 Dec 2023 at 15:16, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 21.12.2023 14:04, Dmitry Baryshkov wrote:
> > On Thu, 21 Dec 2023 at 15:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 21.12.2023 04:50, Bjorn Andersson wrote:
> >>> The GX GDSC is modelled to aid the GMU in powering down the GPU in the
> >>> event that the GPU crashes, so that it can be restarted again. But in
> >>> the event that the power-domain is supplied through a dedicated
> >>> regulator (in contrast to being a subdomin of another power-domain),
> >>> something needs to turn that regulator on, both to make sure things are
> >>> powered and to match the operation in gdsc_disable().
> >>>
> >>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> >>> ---
> >>>  drivers/clk/qcom/gdsc.c | 8 +++++++-
> >>>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> >>> index 5358e28122ab..d1139c895503 100644
> >>> --- a/drivers/clk/qcom/gdsc.c
> >>> +++ b/drivers/clk/qcom/gdsc.c
> >>> @@ -557,7 +557,13 @@ void gdsc_unregister(struct gdsc_desc *desc)
> >>>   */
> >>>  int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
> >> I suppose the name is confusing now..
> >>
> >> But at the same time I can't come up with anything that's less than
> >> like 6 words..
> >
> > gdsc_gx_enable() ;-)
> except not really only gx and not really enable :(
>
> gdsc_shared_enable would probably be closer to our current
> nomenclature..

gdsc_dummy_gx_enable*(

Or gdsc_dummy_gmu_gx_enable(). Still less than 6 words. I'm trying my best!
diff mbox series

Patch

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 5358e28122ab..d1139c895503 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -557,7 +557,13 @@  void gdsc_unregister(struct gdsc_desc *desc)
  */
 int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
 {
+	struct gdsc *sc = domain_to_gdsc(domain);
+	int ret = 0;
+
 	/* Do nothing but give genpd the impression that we were successful */
-	return 0;
+	if (sc->rsupply)
+		ret = regulator_enable(sc->rsupply);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);