[RFC 0/4] msm: clk: Define a special power domain for SDM845 GX

[RFC 0/4] msm: clk: Define a special power domain for SDM845 GX

[Jordan Crouse]

The GPU GX domain on SDM845 is nominally managed by the GMU microcontroller
but there are certain circumstances when the CPU needs to be sure that the
GX headswitch is off.

This RFC series adds a special modification for the GX power domain
that always returns success for domain power on and uses the default
gdsc functions for power down.

With this, we should be able to remove most of GX clocks from the gpucc
and have a relatively clean way of handling the hardware workaround.

This is based on the series from [1] with some slight changes for the
current for-next from Andy.

[1] https://patchwork.kernel.org/patch/10563887/

Jordan Crouse (4):
  drm/msm/a6xx: Remove unwanted regulator code
  clk: qcom: gdsc: Don't override existing gdsc pd functions
  clk: qcom: Add a dummy enable function for GX gdsc
  drm/msm/gpu: Attach to the GPU GX power domain

 drivers/clk/qcom/gdsc.c               |  6 ++--
 drivers/clk/qcom/gpucc-sdm845.c       | 30 ++++++++++++++++---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 43 ++++++++++++++++++++++++---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  4 +--
 4 files changed, 71 insertions(+), 12 deletions(-)

-- 
2.18.0

[PATCH 1/4] drm/msm/a6xx: Remove unwanted regulator code

[Jordan Crouse]

The GMU code currently has some misguided code to try to work around
a hardware quirk that requires the power domains on the GPU be
collapsed in a certain order. Upcoming patches will do this the
right way so get rid of the unused and unwanted regulator
code.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 4 ----
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 2 --
 2 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 546599a7ab05..51493f409358 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -646,9 +646,6 @@ int a6xx_gmu_reset(struct a6xx_gpu *a6xx_gpu)
 	gmu_poll_timeout(gmu, REG_A6XX_RSCC_TCS3_DRV0_STATUS, val,
 		(val & 1), 100, 1000);
 
-	/* Force off the GX GSDC */
-	regulator_force_disable(gmu->gx);
-
 	/* Disable the resources */
 	clk_bulk_disable_unprepare(gmu->nr_clocks, gmu->clocks);
 	pm_runtime_put_sync(gmu->dev);
@@ -1173,7 +1170,6 @@ int a6xx_gmu_probe(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 	gmu->idle_level = GMU_IDLE_STATE_ACTIVE;
 
 	pm_runtime_enable(gmu->dev);
-	gmu->gx = devm_regulator_get(gmu->dev, "vdd");
 
 	/* Get the list of clocks */
 	ret = a6xx_gmu_clocks_probe(gmu);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 35f765afae45..a871cae2fc5e 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -52,8 +52,6 @@ struct a6xx_gmu {
 	int hfi_irq;
 	int gmu_irq;
 
-	struct regulator *gx;
-
 	struct iommu_domain *domain;
 	u64 uncached_iova_base;
 
-- 
2.18.0

Re: [PATCH 1/4] drm/msm/a6xx: Remove unwanted regulator code

[Stephen Boyd]

Quoting Jordan Crouse (2018-11-19 15:47:03)
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 546599a7ab05..51493f409358 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -646,9 +646,6 @@ int a6xx_gmu_reset(struct a6xx_gpu *a6xx_gpu)
>         gmu_poll_timeout(gmu, REG_A6XX_RSCC_TCS3_DRV0_STATUS, val,
>                 (val & 1), 100, 1000);
>  
> -       /* Force off the GX GSDC */
> -       regulator_force_disable(gmu->gx);
> -

I was going to ask if the regulator header file was included, but it
isn't, so nothing to fix!

>         /* Disable the resources */
>         clk_bulk_disable_unprepare(gmu->nr_clocks, gmu->clocks);
>         pm_runtime_put_sync(gmu->dev);

[PATCH 2/4] clk: qcom: gdsc: Don't override existing gdsc pd functions

[Jordan Crouse]

In very extreme cases an individual gdsc may wish to override the
power domain enable or disable callback functions for their own
purposes. Only set the generic gdsc callback if the function pointers
are not already set.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index b6adca1f3918..7b55368b9a9c 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -394,8 +394,10 @@ static int gdsc_init(struct gdsc *sc)
 	else
 		gdsc_clear_mem_on(sc);
 
-	sc->pd.power_off = gdsc_disable;
-	sc->pd.power_on = gdsc_enable;
+	if (!sc->pd.power_off)
+		sc->pd.power_off = gdsc_disable;
+	if (!sc->pd.power_on)
+		sc->pd.power_on = gdsc_enable;
 	pm_genpd_init(&sc->pd, NULL, !on);
 
 	return 0;
-- 
2.18.0

Re: [PATCH 2/4] clk: qcom: gdsc: Don't override existing gdsc pd functions

[Rajendra Nayak]

On 11/20/2018 5:17 AM, Jordan Crouse wrote:
> In very extreme cases an individual gdsc may wish to override the
> power domain enable or disable callback functions for their own
> purposes. Only set the generic gdsc callback if the function pointers
> are not already set.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>

Looks good to me,
Acked-by: Rajendra Nayak <rnayak@codeaurora.org>

> ---
>   drivers/clk/qcom/gdsc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index b6adca1f3918..7b55368b9a9c 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -394,8 +394,10 @@ static int gdsc_init(struct gdsc *sc)
>   	else
>   		gdsc_clear_mem_on(sc);
>   
> -	sc->pd.power_off = gdsc_disable;
> -	sc->pd.power_on = gdsc_enable;
> +	if (!sc->pd.power_off)
> +		sc->pd.power_off = gdsc_disable;
> +	if (!sc->pd.power_on)
> +		sc->pd.power_on = gdsc_enable;
>   	pm_genpd_init(&sc->pd, NULL, !on);
>   
>   	return 0;
> 

[PATCH 3/4] clk: qcom: Add a dummy enable function for GX gdsc

[Jordan Crouse]

Most of the time the CPU should not be touching the GX
domain on the GPU except for a very special use case when
the CPU needs to force the GX headswitch off. Add a
dummy enable function for the GX gdsc to simulate success
so that the pm_runtime reference counting is correct.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/clk/qcom/gpucc-sdm845.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/gpucc-sdm845.c b/drivers/clk/qcom/gpucc-sdm845.c
index 7a11b70b33f4..06254329ea33 100644
--- a/drivers/clk/qcom/gpucc-sdm845.c
+++ b/drivers/clk/qcom/gpucc-sdm845.c
@@ -319,16 +319,38 @@ static struct gdsc gpu_cx_gdsc = {
 	.flags = VOTABLE,
 };
 
+/*
+ * On SDM845 the GPU GX domain is *almost* entirely controlled by the GMU
+ * running in the CX domain so the CPU doesn't need to know anything about the
+ * GX domain EXCEPT....
+ *
+ * Hardware constraints dictate that the GX be powered down before the CX. If
+ * the GMU crashes it could leave the GX on. In order to successfully bring back
+ * the device the CPU needs to disable the GX headswitch. There being no sane
+ * way to reach in and touch that register from deep inside the GPU driver we
+ * need to set up the infrastructure to be able to ensure that the GPU can
+ * ensure that the GX is off during this super special case. We do this by
+ * defining a GX gdsc with a dummy enable function and a "default" disable
+ * function.
+ *
+ * This allows us to attach with genpd_dev_pm_attach_by_name() in the GPU
+ * driver. During power up, nothing will happen from the CPU (and the GMU will
+ * power up normally but during power down this will ensure that the GX domain
+ * is *really* off - this gives us a semi standard way of doing what we need.
+ */
+static int gx_gdsc_enable(struct generic_pm_domain *domain)
+{
+	/* Do nothing but give genpd the impression that we were successful */
+	return 0;
+}
+
 static struct gdsc gpu_gx_gdsc = {
 	.gdscr = 0x100c,
 	.clamp_io_ctrl = 0x1508,
 	.pd = {
 		.name = "gpu_gx_gdsc",
+		.power_on = gx_gdsc_enable,
 	},
-	.clk_hws = {
-		&gpu_cc_gx_gfx3d_clk_src.clkr.hw,
-	},
-	.clk_count = 1,
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = CLAMP_IO | AON_RESET | POLL_CFG_GDSCR,
 };
-- 
2.18.0

[PATCH 4/4] drm/msm/gpu: Attach to the GPU GX power domain

[Jordan Crouse]

99.999% of the time during normal operation the GMU is responsible
for power and clock control on the GX domain and the CPU remains
blissfully unaware. However, there is one situation where the CPU
needs to get involved:

The power sequencing rules dictate that the GX needs to be turned
off before the CX so that the CX can be turned on before the GX
during power up. During normal operation when the CPU is taking
down the CX domain a stop command is sent to the GMU which turns
off the GX domain and then the CPU handles the CX domain.

But if the GMU happened to be unresponsive while the GX domain was
left then the CPU will need to step in and turn off the GX domain
before resetting the CX and rebooting the GMU. This unfortunately
means that the CPU needs to be marginally aware of the GX domain
even though it is expected to usually keep its hands off.

To support this we create a semi-disabled GX power domain that
does nothing to the hardware on power up but tries to shut it
down normally on power down. In this method the reference counting
is correct and we can step in with the pm_runtime_put() at the right
time during the failure path.

This patch sets up the connection to the GX power domain and does
the magic to "enable" and disable it at the right points.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 41 ++++++++++++++++++++++++++-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 ++
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 51493f409358..ca71709efc94 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -2,6 +2,7 @@
 /* Copyright (c) 2017-2018 The Linux Foundation. All rights reserved. */
 
 #include <linux/clk.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_opp.h>
 #include <soc/qcom/cmd-db.h>
 
@@ -646,6 +647,16 @@ int a6xx_gmu_reset(struct a6xx_gpu *a6xx_gpu)
 	gmu_poll_timeout(gmu, REG_A6XX_RSCC_TCS3_DRV0_STATUS, val,
 		(val & 1), 100, 1000);
 
+	/*
+	 * Depending on the state of the GMU at this point the GX domain might
+	 * have been left on. Hardware sequencing rules state that the GX has to
+	 * be turned off before the CX domain so this is that one time that
+	 * that calling pm_runtime_put_sync() is expected to do something useful
+	 * (turn off the headswitch)
+	 */
+	if (!IS_ERR(gmu->gxpd))
+		pm_runtime_put_sync(gmu->gxpd);
+
 	/* Disable the resources */
 	clk_bulk_disable_unprepare(gmu->nr_clocks, gmu->clocks);
 	pm_runtime_put_sync(gmu->dev);
@@ -707,6 +718,14 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
 	/* Set the GPU to the highest power frequency */
 	__a6xx_gmu_set_freq(gmu, gmu->nr_gpu_freqs - 1);
 
+	/*
+	 * "enable" the GX power domain which won't actually do anything but it
+	 * will make sure that the refcounting is correct in case we need to
+	 * bring down the GX after a GMU failure
+	 */
+	if (!IS_ERR(gmu->gxpd))
+		pm_runtime_get(gmu->gxpd);
+
 out:
 	/* Make sure to turn off the boot OOB request on error */
 	if (ret)
@@ -778,6 +797,14 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
 	/* Tell RPMh to power off the GPU */
 	a6xx_rpmh_stop(gmu);
 
+	/*
+	 * Mark the GPU power domain as off. During the shutdown process the GMU
+	 * should actually turn off the power so this is really just a
+	 * houskeeping step
+	 */
+	if (!IS_ERR(gmu->gxpd))
+		pm_runtime_put_sync(gmu->gxpd);
+
 	clk_bulk_disable_unprepare(gmu->nr_clocks, gmu->clocks);
 
 	pm_runtime_put_sync(gmu->dev);
@@ -1142,9 +1169,15 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
 	if (IS_ERR_OR_NULL(gmu->mmio))
 		return;
 
-	pm_runtime_disable(gmu->dev);
 	a6xx_gmu_stop(a6xx_gpu);
 
+	pm_runtime_disable(gmu->dev);
+
+	if (!IS_ERR(gmu->gxpd)) {
+		pm_runtime_disable(gmu->gxpd);
+		dev_pm_domain_detach(gmu->gxpd, false);
+	}
+
 	a6xx_gmu_irq_disable(gmu);
 	a6xx_gmu_memory_free(gmu, gmu->hfi);
 
@@ -1203,6 +1236,12 @@ int a6xx_gmu_probe(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 	if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0)
 		goto err;
 
+	/*
+	 * Get a link to the GX power domain to reset the GPU in case of GMU
+	 * crash
+	 */
+	gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
+
 	/* Get the power levels for the GMU and GPU */
 	a6xx_gmu_pwrlevels_probe(gmu);
 
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index a871cae2fc5e..dcc172d55f49 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -55,6 +55,8 @@ struct a6xx_gmu {
 	struct iommu_domain *domain;
 	u64 uncached_iova_base;
 
+	struct device *gxpd;
+
 	int idle_level;
 
 	struct a6xx_gmu_bo *hfi;
-- 
2.18.0

Re: [PATCH 4/4] drm/msm/gpu: Attach to the GPU GX power domain

[Stephen Boyd]

Quoting Jordan Crouse (2018-11-19 15:47:06)
> 99.999% of the time during normal operation the GMU is responsible
> for power and clock control on the GX domain and the CPU remains
> blissfully unaware. However, there is one situation where the CPU
> needs to get involved:
> 
> The power sequencing rules dictate that the GX needs to be turned
> off before the CX so that the CX can be turned on before the GX
> during power up. During normal operation when the CPU is taking
> down the CX domain a stop command is sent to the GMU which turns
> off the GX domain and then the CPU handles the CX domain.
> 
> But if the GMU happened to be unresponsive while the GX domain was
> left then the CPU will need to step in and turn off the GX domain

      ^ left on?

> before resetting the CX and rebooting the GMU. This unfortunately
> means that the CPU needs to be marginally aware of the GX domain
> even though it is expected to usually keep its hands off.
> 
> To support this we create a semi-disabled GX power domain that
> does nothing to the hardware on power up but tries to shut it
> down normally on power down. In this method the reference counting
> is correct and we can step in with the pm_runtime_put() at the right
> time during the failure path.
> 
> This patch sets up the connection to the GX power domain and does
> the magic to "enable" and disable it at the right points.
> 
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>

The pm_runtime gymnastics is scary! But I'm willing to go with it.

> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 41 ++++++++++++++++++++++++++-
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 ++
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 51493f409358..ca71709efc94 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -1142,9 +1169,15 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
>         if (IS_ERR_OR_NULL(gmu->mmio))
>                 return;
>  
> -       pm_runtime_disable(gmu->dev);
>         a6xx_gmu_stop(a6xx_gpu);
>  
> +       pm_runtime_disable(gmu->dev);
> +
> +       if (!IS_ERR(gmu->gxpd)) {
> +               pm_runtime_disable(gmu->gxpd);
> +               dev_pm_domain_detach(gmu->gxpd, false);
> +       }
> +
>         a6xx_gmu_irq_disable(gmu);
>         a6xx_gmu_memory_free(gmu, gmu->hfi);
>  
> @@ -1203,6 +1236,12 @@ int a6xx_gmu_probe(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
>         if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0)
>                 goto err;
>  
> +       /*
> +        * Get a link to the GX power domain to reset the GPU in case of GMU
> +        * crash
> +        */
> +       gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");

Are there a6xx devices that don't have this genpd? Just curious if we
could always assume that if it's an a6xx gpu then this must be there and
we can always call pm_runtime_get/put on it and avoid the if (!IS_ERR())
checks.

> +
>         /* Get the power levels for the GMU and GPU */
>         a6xx_gmu_pwrlevels_probe(gmu);
>  

Re: [PATCH 4/4] drm/msm/gpu: Attach to the GPU GX power domain

[Jordan Crouse]

On Tue, Nov 20, 2018 at 11:54:46PM -0800, Stephen Boyd wrote:
> Quoting Jordan Crouse (2018-11-19 15:47:06)
> > 99.999% of the time during normal operation the GMU is responsible
> > for power and clock control on the GX domain and the CPU remains
> > blissfully unaware. However, there is one situation where the CPU
> > needs to get involved:
> > 
> > The power sequencing rules dictate that the GX needs to be turned
> > off before the CX so that the CX can be turned on before the GX
> > during power up. During normal operation when the CPU is taking
> > down the CX domain a stop command is sent to the GMU which turns
> > off the GX domain and then the CPU handles the CX domain.
> > 
> > But if the GMU happened to be unresponsive while the GX domain was
> > left then the CPU will need to step in and turn off the GX domain
> 
>       ^ left on?
> 
> > before resetting the CX and rebooting the GMU. This unfortunately
> > means that the CPU needs to be marginally aware of the GX domain
> > even though it is expected to usually keep its hands off.
> > 
> > To support this we create a semi-disabled GX power domain that
> > does nothing to the hardware on power up but tries to shut it
> > down normally on power down. In this method the reference counting
> > is correct and we can step in with the pm_runtime_put() at the right
> > time during the failure path.
> > 
> > This patch sets up the connection to the GX power domain and does
> > the magic to "enable" and disable it at the right points.
> > 
> > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> 
> The pm_runtime gymnastics is scary! But I'm willing to go with it.
> 
> > ---
> >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 41 ++++++++++++++++++++++++++-
> >  drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  2 ++
> >  2 files changed, 42 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > index 51493f409358..ca71709efc94 100644
> > --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> > @@ -1142,9 +1169,15 @@ void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu)
> >         if (IS_ERR_OR_NULL(gmu->mmio))
> >                 return;
> >  
> > -       pm_runtime_disable(gmu->dev);
> >         a6xx_gmu_stop(a6xx_gpu);
> >  
> > +       pm_runtime_disable(gmu->dev);
> > +
> > +       if (!IS_ERR(gmu->gxpd)) {
> > +               pm_runtime_disable(gmu->gxpd);
> > +               dev_pm_domain_detach(gmu->gxpd, false);
> > +       }
> > +
> >         a6xx_gmu_irq_disable(gmu);
> >         a6xx_gmu_memory_free(gmu, gmu->hfi);
> >  
> > @@ -1203,6 +1236,12 @@ int a6xx_gmu_probe(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> >         if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0)
> >                 goto err;
> >  
> > +       /*
> > +        * Get a link to the GX power domain to reset the GPU in case of GMU
> > +        * crash
> > +        */
> > +       gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
> 
> Are there a6xx devices that don't have this genpd? Just curious if we
> could always assume that if it's an a6xx gpu then this must be there and
> we can always call pm_runtime_get/put on it and avoid the if (!IS_ERR())
> checks.

Mainly I was trying to be backwards compatible with the device tree. I didn't
want to derail various development efforts. I don't mind the checks (though I
wish that pm_runtime_* was error tolerant) but folks really hate them we can
replace this with an error return and force people to update their device trees.

Jordan

> +
> >         /* Get the power levels for the GMU and GPU */
> >         a6xx_gmu_pwrlevels_probe(gmu);
> >  

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/4] drm/msm/gpu: Attach to the GPU GX power domain

[Stephen Boyd]

Quoting Jordan Crouse (2018-11-21 07:00:06)
> On Tue, Nov 20, 2018 at 11:54:46PM -0800, Stephen Boyd wrote:
> > Quoting Jordan Crouse (2018-11-19 15:47:06)
> > > @@ -1203,6 +1236,12 @@ int a6xx_gmu_probe(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
> > >         if (gmu->hfi_irq < 0 || gmu->gmu_irq < 0)
> > >                 goto err;
> > >  
> > > +       /*
> > > +        * Get a link to the GX power domain to reset the GPU in case of GMU
> > > +        * crash
> > > +        */
> > > +       gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
> > 
> > Are there a6xx devices that don't have this genpd? Just curious if we
> > could always assume that if it's an a6xx gpu then this must be there and
> > we can always call pm_runtime_get/put on it and avoid the if (!IS_ERR())
> > checks.
> 
> Mainly I was trying to be backwards compatible with the device tree. I didn't
> want to derail various development efforts. I don't mind the checks (though I
> wish that pm_runtime_* was error tolerant) but folks really hate them we can
> replace this with an error return and force people to update their device trees.
> 

Ok. Well we don't have this DT node in mainline yet so either followup
after the merge cycle and remove the checks, or just take the pain now
and remove the checks and tell people using out of tree DT to update?

Re: [RFC 0/4] msm: clk: Define a special power domain for SDM845 GX

[Stephen Boyd]

Quoting Jordan Crouse (2018-11-19 15:47:02)
> The GPU GX domain on SDM845 is nominally managed by the GMU microcontroller
> but there are certain circumstances when the CPU needs to be sure that the
> GX headswitch is off.
> 
> This RFC series adds a special modification for the GX power domain
> that always returns success for domain power on and uses the default
> gdsc functions for power down.
> 
> With this, we should be able to remove most of GX clocks from the gpucc
> and have a relatively clean way of handling the hardware workaround.
> 
> This is based on the series from [1] with some slight changes for the
> current for-next from Andy.
> 
> [1] https://patchwork.kernel.org/patch/10563887/
> 
> Jordan Crouse (4):
>   drm/msm/a6xx: Remove unwanted regulator code
>   clk: qcom: gdsc: Don't override existing gdsc pd functions
>   clk: qcom: Add a dummy enable function for GX gdsc
>   drm/msm/gpu: Attach to the GPU GX power domain

The clk bits look good to me, and would simplify the gpucc patches
sitting on the list. Can we roll those into or on top of the GPU clk
driver patches and split them from the GPU driver parts? I think the DTS
will be blocked on everything coming together, so it should be OK to let
the DTS bits come in and meet up with drm bits and clk bits all from
different trees.

Re: [RFC 0/4] msm: clk: Define a special power domain for SDM845 GX

[Jordan Crouse]

On Wed, Nov 21, 2018 at 12:00:51AM -0800, Stephen Boyd wrote:
> Quoting Jordan Crouse (2018-11-19 15:47:02)
> > The GPU GX domain on SDM845 is nominally managed by the GMU microcontroller
> > but there are certain circumstances when the CPU needs to be sure that the
> > GX headswitch is off.
> > 
> > This RFC series adds a special modification for the GX power domain
> > that always returns success for domain power on and uses the default
> > gdsc functions for power down.
> > 
> > With this, we should be able to remove most of GX clocks from the gpucc
> > and have a relatively clean way of handling the hardware workaround.
> > 
> > This is based on the series from [1] with some slight changes for the
> > current for-next from Andy.
> > 
> > [1] https://patchwork.kernel.org/patch/10563887/
> > 
> > Jordan Crouse (4):
> >   drm/msm/a6xx: Remove unwanted regulator code
> >   clk: qcom: gdsc: Don't override existing gdsc pd functions
> >   clk: qcom: Add a dummy enable function for GX gdsc
> >   drm/msm/gpu: Attach to the GPU GX power domain
> 
> The clk bits look good to me, and would simplify the gpucc patches
> sitting on the list. Can we roll those into or on top of the GPU clk
> driver patches and split them from the GPU driver parts? I think the DTS
> will be blocked on everything coming together, so it should be OK to let
> the DTS bits come in and meet up with drm bits and clk bits all from
> different trees.

AFAIK Taniya is working on a new rev of the clock patches. We can either take
the two clk patches and roll/add on to that and I'll send the GPU patches
along with Rob (and possibly squash them as well).

Jordan
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project