[PATCH v3 0/4] Add QCOM graphics clock controller driver for SDM845

[PATCH v3 0/4] Add QCOM graphics clock controller driver for SDM845

[Amit Nischal]

Changes in v3:
* Modified the determine_rate() op to use the min/max rate range
  to round the requested rate within the set_rate range. With this,
  requested set rate will always stay within the limits.

Changes in v2:
Addressed review comments given by Stephen: https://lkml.org/lkml/2018/6/6/294
* Introduce clk_rcg2_gfx3d_determine_rate ops to return the best parent
  as 'gpucc_pll0_even' and best parent rate as twice of the requested rate
  always. This will eliminate the need of frequency table as source and
  div-2 are fixed for gfx3d_clk_src.
  Also modified the clk_rcg2_set_rate ops to configure the fixed source and
  div.
* Add support to check if requested rate falls within allowed set_rate range.
  This will not let the source gpucc_pll0 to go out of the supported range
  and also client can request the rate within the range.
* Fixed comment text in probe function and added module description for GPUCC
  driver.

The graphics clock driver depends upon the below change.
	https://lkml.org/lkml/2018/6/23/108

Changes in v1:
This patch series adds support for graphics clock controller for SDM845.
Below is the brief description for each change:

1. For some of the GDSCs, there is requirement to enable/disable the
   few clocks before turning on/off the gdsc power domain. This patch
   will add support to enable/disable the clock associated with the
   gdsc along with power domain on/off callbacks.

2. To turn on the gpu_gx_gdsc, there is a hardware requirement to
   turn on the root clock (GFX3D RCG) first which would be the turn
   on signal for the gdsc along with the SW_COLLAPSE. As per the
   current implementation of clk_rcg2_shared_ops, it clears the
   root_enable bit in the enable() clock ops. But due to the above
   said requirement for GFX3D shared RCG, root_enable bit would be
   already set by gdsc driver and rcg2_shared_ops should not clear
   the root unless the disable() is called.

   This patch add support for the same by reusing the existing
   clk_rcg2_shared_ops and deriving "clk_rcg2_gfx3d_ops" clk_ops
   for GFX3D clock to take care of the root set/clear requirement.

3. Add device tree bindings for graphics clock controller for SDM845.

4. Add graphics clock controller (GPUCC) driver for SDM845.

[v1] : https://lore.kernel.org/patchwork/project/lkml/list/?series=348697
[v2] : https://lore.kernel.org/patchwork/project/lkml/list/?series=359012

Amit Nischal (4):
  clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC
  clk: qcom: Add clk_rcg2_gfx3d_ops for SDM845
  dt-bindings: clock: Introduce QCOM Graphics clock bindings
  clk: qcom: Add graphics clock controller driver for SDM845

 .../devicetree/bindings/clock/qcom,gpucc.txt       |  18 +
 drivers/clk/qcom/Kconfig                           |   9 +
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/clk-rcg.h                         |   1 +
 drivers/clk/qcom/clk-rcg2.c                        |  86 +++-
 drivers/clk/qcom/gdsc.c                            |  44 +++
 drivers/clk/qcom/gdsc.h                            |   5 +
 drivers/clk/qcom/gpucc-sdm845.c                    | 438 +++++++++++++++++++++
 include/dt-bindings/clock/qcom,gpucc-sdm845.h      |  38 ++
 9 files changed, 638 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.txt
 create mode 100644 drivers/clk/qcom/gpucc-sdm845.c
 create mode 100644 include/dt-bindings/clock/qcom,gpucc-sdm845.h

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 1/4] clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC

[Amit Nischal]

For some of the GDSCs, there is a requirement to enable/disable the
few clocks before turning on/off the gdsc power domain. Add support
for the same by specifying a list of clk_hw pointers per gdsc and
enable/disable them along with power domain on/off callbacks.

Signed-off-by: Amit Nischal <anischal@codeaurora.org>
---
 drivers/clk/qcom/gdsc.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/gdsc.h |  5 +++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a077133..b6adca1 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -12,6 +12,8 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/jiffies.h>
@@ -208,11 +210,41 @@ static inline void gdsc_assert_reset_aon(struct gdsc *sc)
 	regmap_update_bits(sc->regmap, sc->clamp_io_ctrl,
 			   GMEM_RESET_MASK, 0);
 }
+
+static int gdsc_clk_prepare_enable(struct gdsc *sc)
+{
+	int i, ret;
+
+	for (i = 0; i < sc->clk_count; i++) {
+		ret = clk_prepare_enable(sc->clk_hws[i]->clk);
+		if (ret) {
+			for (i--; i >= 0; i--)
+				clk_disable_unprepare(sc->clk_hws[i]->clk);
+			return ret;
+		}
+	}
+	return 0;
+}
+
+static void gdsc_clk_disable_unprepare(struct gdsc *sc)
+{
+	int i;
+
+	for (i = 0; i < sc->clk_count; i++)
+		clk_disable_unprepare(sc->clk_hws[i]->clk);
+}
+
 static int gdsc_enable(struct generic_pm_domain *domain)
 {
 	struct gdsc *sc = domain_to_gdsc(domain);
 	int ret;
 
+	if (sc->clk_count) {
+		ret = gdsc_clk_prepare_enable(sc);
+		if (ret)
+			return ret;
+	}
+
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_deassert_reset(sc);
 
@@ -260,6 +292,9 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 		udelay(1);
 	}
 
+	if (sc->clk_count)
+		gdsc_clk_disable_unprepare(sc);
+
 	return 0;
 }
 
@@ -268,6 +303,12 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	struct gdsc *sc = domain_to_gdsc(domain);
 	int ret;
 
+	if (sc->clk_count) {
+		ret = gdsc_clk_prepare_enable(sc);
+		if (ret)
+			return ret;
+	}
+
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_assert_reset(sc);
 
@@ -299,6 +340,9 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	if (sc->flags & CLAMP_IO)
 		gdsc_assert_clamp_io(sc);
 
+	if (sc->clk_count)
+		gdsc_clk_disable_unprepare(sc);
+
 	return 0;
 }
 
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index bd1f2c7..59957d7 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -17,6 +17,7 @@
 #include <linux/err.h>
 #include <linux/pm_domain.h>
 
+struct clk_hw;
 struct regmap;
 struct reset_controller_dev;
 
@@ -32,6 +33,8 @@
  * @resets: ids of resets associated with this gdsc
  * @reset_count: number of @resets
  * @rcdev: reset controller
+ * @clk_count: number of associated clocks
+ * @clk_hws: clk_hw pointers for associated clocks with gdsc
  */
 struct gdsc {
 	struct generic_pm_domain	pd;
@@ -60,6 +63,8 @@ struct gdsc {
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
+	unsigned int			clk_count;
+	struct clk_hw			*clk_hws[];
 };
 
 struct gdsc_desc {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 1/4] clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC

[Stephen Boyd]

Quoting Amit Nischal (2018-08-12 23:33:04)
> For some of the GDSCs, there is a requirement to enable/disable the
> few clocks before turning on/off the gdsc power domain. Add support
> for the same by specifying a list of clk_hw pointers per gdsc and
> enable/disable them along with power domain on/off callbacks.
> 
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>

In v1 of this patch series I asked for much more information in this
commit text. Please add it here. I won't apply this patch until the
justification is put into this commit text.

> ---
>  drivers/clk/qcom/gdsc.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/qcom/gdsc.h |  5 +++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index a077133..b6adca1 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -12,6 +12,8 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

This makes me unhappy. It's almost always a problem when we see clk.h
and clk-provider.h included in the same file, so if gdsc has to call clk
APIs to operate correctly, then we should do that by having the gdsc
code get clks properly instead of directly reaching into the clk_hw
structure to get a clk pointer. This means we should have the gdsc code
ask the clk framework to convert a clk_hw pointer into a clk pointer
because of how so intimately connected the gdsc is to clks on this SoC.
But given all that, I'm still trying to understand why we need to do
this within the gdsc code.

Adding these clk calls to the gdsc seems like we're attaching at the
wrong abstraction level. Especially if the reason we do it is to make it
easier for the GPU driver to handle dependencies. I hope that's not the
case. Either way, it would make more sense to me if we made genpds for
the clks and genpds for the gdscs and then associated the clk genpds
with the gdsc genpds so that when a gdsc is enabled the clk domain that
it depends on is enabled first. Then we have a generic solution for
connecting clks to gdscs that doesn't require us to add more logic to
the gdsc driver and avoids having clk providers do clk consumer things.
Instead, it's all handled outside of this driver by specifying a domain
dependency. It may turn out that such a solution would still need a way
to make clk domains in the clk driver, and it will probably need to do
that by converting clk_hw structures into clk pointers, but it would be
good to do that anyway.

So in summary, I believe we should end up at a point where we have clk
domains and power domains (gdscs) all supported with genpds, and then we
can connect them together however they're connected by linking the
genpds to each other. Device drivers wouldn't really need to care how
they're connected, as long as those genpds are attached to their device
then the driver would be able to enable/disable them through runtime PM.
But I can see how this may be hard to do for this patch series, so in
the spirit of progress and getting things done, it would be OK with me
if the gdsc code called some new clk API to convert a clk_hw pointer
into a clk pointer and then did the same enable/disable things it's
doing in this patch. This whole patch would need to be completely
untangled and ripped out later when we have clk domains but at least we
could get something working now while we work on making clk domains and
plumbing them into genpds and runtime PM.

Re: [PATCH v3 1/4] clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC

[Taniya Das]

Hello Stephen,

On 11/5/2018 12:04 PM, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-08-12 23:33:04)
>> For some of the GDSCs, there is a requirement to enable/disable the
>> few clocks before turning on/off the gdsc power domain. Add support
>> for the same by specifying a list of clk_hw pointers per gdsc and
>> enable/disable them along with power domain on/off callbacks.
>>
>> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> 
> In v1 of this patch series I asked for much more information in this
> commit text. Please add it here. I won't apply this patch until the
> justification is put into this commit text.
> 

Would surely add more details.

>> ---
>>   drivers/clk/qcom/gdsc.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/clk/qcom/gdsc.h |  5 +++++
>>   2 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index a077133..b6adca1 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -12,6 +12,8 @@
>>    */
>>   
>>   #include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
> 
> This makes me unhappy. It's almost always a problem when we see clk.h
> and clk-provider.h included in the same file, so if gdsc has to call clk
> APIs to operate correctly, then we should do that by having the gdsc
> code get clks properly instead of directly reaching into the clk_hw
> structure to get a clk pointer. This means we should have the gdsc code
> ask the clk framework to convert a clk_hw pointer into a clk pointer
> because of how so intimately connected the gdsc is to clks on this SoC.
> But given all that, I'm still trying to understand why we need to do
> this within the gdsc code.
> 
> Adding these clk calls to the gdsc seems like we're attaching at the
> wrong abstraction level. Especially if the reason we do it is to make it
> easier for the GPU driver to handle dependencies. I hope that's not the
> case. Either way, it would make more sense to me if we made genpds for
> the clks and genpds for the gdscs and then associated the clk genpds
> with the gdsc genpds so that when a gdsc is enabled the clk domain that
> it depends on is enabled first. Then we have a generic solution for
> connecting clks to gdscs that doesn't require us to add more logic to
> the gdsc driver and avoids having clk providers do clk consumer things.
> Instead, it's all handled outside of this driver by specifying a domain
> dependency. It may turn out that such a solution would still need a way
> to make clk domains in the clk driver, and it will probably need to do
> that by converting clk_hw structures into clk pointers, but it would be
> good to do that anyway.
> 
> So in summary, I believe we should end up at a point where we have clk
> domains and power domains (gdscs) all supported with genpds, and then we
> can connect them together however they're connected by linking the
> genpds to each other. Device drivers wouldn't really need to care how
> they're connected, as long as those genpds are attached to their device
> then the driver would be able to enable/disable them through runtime PM.
> But I can see how this may be hard to do for this patch series, so in
> the spirit of progress and getting things done, it would be OK with me
> if the gdsc code called some new clk API to convert a clk_hw pointer
> into a clk pointer and then did the same enable/disable things it's
> doing in this patch. This whole patch would need to be completely
> untangled and ripped out later when we have clk domains but at least we
> could get something working now while we work on making clk domains and
> plumbing them into genpds and runtime PM.
> 

Yes, I agree with your points above, but as genpds currently cannot have 
a way to take in clock handles, this was the way we chose.

I would add a new clock API as suggested and submit the next series.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

[PATCH v3 2/4] clk: qcom: Add clk_rcg2_gfx3d_ops for SDM845

[Amit Nischal]

To turn on the gpu_gx_gdsc, there is a hardware requirement to
turn on the root clock (GFX3D RCG) first which would be the turn
on signal for the gdsc along with the SW_COLLAPSE. As per the
current implementation of clk_rcg2_shared_ops, it clears the
root_enable bit in the enable() clock op. But due to the above
said requirement for GFX3D shared RCG, root_enable bit would be
already set by gdsc driver and clk_rcg2_shared_enable()should
not clear the root unless the disable is called.

Add support for the same by reusing the existing clk_rcg2_shared_ops
and deriving "clk_rcg2_gfx3d_ops" clk_ops for GFX3D clock to take
care of the root set/clear requirement.

Signed-off-by: Amit Nischal <anischal@codeaurora.org>
---
 drivers/clk/qcom/clk-rcg.h  |  1 +
 drivers/clk/qcom/clk-rcg2.c | 86 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index dbd5a9e..081eca9 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -162,5 +162,6 @@ struct clk_rcg2 {
 extern const struct clk_ops clk_pixel_ops;
 extern const struct clk_ops clk_gfx3d_ops;
 extern const struct clk_ops clk_rcg2_shared_ops;
+extern const struct clk_ops clk_rcg2_gfx3d_ops;

 #endif
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 52208d4..a57ce00 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -866,7 +866,7 @@ static int clk_rcg2_shared_set_rate_and_parent(struct clk_hw *hw,
 	return clk_rcg2_shared_set_rate(hw, rate, parent_rate);
 }

-static int clk_rcg2_shared_enable(struct clk_hw *hw)
+static int __clk_rcg2_shared_enable(struct clk_hw *hw)
 {
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	int ret;
@@ -879,7 +879,14 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw)
 	if (ret)
 		return ret;

-	ret = update_config(rcg);
+	return update_config(rcg);
+}
+
+static int clk_rcg2_shared_enable(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = __clk_rcg2_shared_enable(hw);
 	if (ret)
 		return ret;

@@ -929,3 +936,78 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
 	.set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
 };
 EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
+
+static int clk_rcg2_gfx3d_enable(struct clk_hw *hw)
+{
+	return __clk_rcg2_shared_enable(hw);
+}
+
+static int clk_rcg2_gfx3d_determine_rate(struct clk_hw *hw,
+					struct clk_rate_request *req)
+{
+	struct clk_rate_request parent_req = { };
+	struct clk_hw *p;
+	unsigned long rate = req->rate;
+	int ret;
+
+	rate = clamp(rate, req->min_rate, req->max_rate);
+
+	/* Get fixed parent - GPU_CC_PLL0_OUT_EVEN */
+	p = clk_hw_get_parent_by_index(hw, 1);
+
+	/* Parent should always run at twice of the requested rate */
+	parent_req.rate = 2 * rate;
+
+	ret = __clk_determine_rate(req->best_parent_hw, &parent_req);
+	if (ret)
+		return ret;
+
+	req->best_parent_hw = p;
+	req->best_parent_rate = parent_req.rate;
+	req->rate = parent_req.rate / 2;
+
+	return 0;
+}
+
+static int clk_rcg2_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long parent_rate)
+{
+	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+	int ret;
+	u32 cfg;
+
+	/* Configure fixed SRC and DIV */
+	cfg = rcg->parent_map[1].cfg << CFG_SRC_SEL_SHIFT;
+	cfg |= 0x3 << CFG_SRC_DIV_SHIFT;
+
+	ret = regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
+	if (ret)
+		return ret;
+
+	/*
+	 * In case clock is disabled, update the SRC and DIV only
+	 * and return without configuration update.
+	 */
+	if (!__clk_is_enabled(hw->clk))
+		return 0;
+
+	return update_config(rcg);
+}
+
+static int clk_rcg2_gfx3d_set_rate_and_parent(struct clk_hw *hw,
+		unsigned long rate, unsigned long parent_rate, u8 index)
+{
+	return clk_rcg2_gfx3d_set_rate(hw, rate, parent_rate);
+}
+
+const struct clk_ops clk_rcg2_gfx3d_ops = {
+	.enable = clk_rcg2_gfx3d_enable,
+	.disable = clk_rcg2_shared_disable,
+	.get_parent = clk_rcg2_get_parent,
+	.set_parent = clk_rcg2_set_parent,
+	.recalc_rate = clk_rcg2_recalc_rate,
+	.determine_rate = clk_rcg2_gfx3d_determine_rate,
+	.set_rate = clk_rcg2_gfx3d_set_rate,
+	.set_rate_and_parent = clk_rcg2_gfx3d_set_rate_and_parent,
+};
+EXPORT_SYMBOL_GPL(clk_rcg2_gfx3d_ops);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 2/4] clk: qcom: Add clk_rcg2_gfx3d_ops for SDM845

[Stephen Boyd]

Quoting Amit Nischal (2018-08-12 23:33:05)
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 52208d4..a57ce00 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -866,7 +866,7 @@ static int clk_rcg2_shared_set_rate_and_parent(struct clk_hw *hw,
>         return clk_rcg2_shared_set_rate(hw, rate, parent_rate);
>  }
> 
> -static int clk_rcg2_shared_enable(struct clk_hw *hw)
> +static int __clk_rcg2_shared_enable(struct clk_hw *hw)

Name this clk_rcg2_force_enable() please. Also add a comment above like:

/* Set RCG force enable bit and flush out any configuration settings */

>  {
>         struct clk_rcg2 *rcg = to_clk_rcg2(hw);
>         int ret;
> @@ -879,7 +879,14 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw)
>         if (ret)
>                 return ret;
> 
> -       ret = update_config(rcg);
> +       return update_config(rcg);
> +}
> +
> +static int clk_rcg2_shared_enable(struct clk_hw *hw)
> +{
> +       int ret;
> +
> +       ret = __clk_rcg2_shared_enable(hw);
>         if (ret)
>                 return ret;
> 
> @@ -929,3 +936,78 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
>         .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
>  };
>  EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
> +
> +static int clk_rcg2_gfx3d_enable(struct clk_hw *hw)
> +{
> +       return __clk_rcg2_shared_enable(hw);
> +}

And then drop this wrapper and just use clk_rcg2_force_enable() in the
clk_ops structure.

> +
> +static int clk_rcg2_gfx3d_determine_rate(struct clk_hw *hw,
> +                                       struct clk_rate_request *req)
> +{
> +       struct clk_rate_request parent_req = { };
> +       struct clk_hw *p;
> +       unsigned long rate = req->rate;
> +       int ret;
> +
> +       rate = clamp(rate, req->min_rate, req->max_rate);

Doesn't the core already clamp the rate for you? Seems wasteful to do it
again.

> +
> +       /* Get fixed parent - GPU_CC_PLL0_OUT_EVEN */

I hope this doesn't change in the future! Any way to make this
detectable by storing the parent index somewhere so this op can work
with any future index selection instead of hardcoding '1' here?

> +       p = clk_hw_get_parent_by_index(hw, 1);
> +
> +       /* Parent should always run at twice of the requested rate */
> +       parent_req.rate = 2 * rate;
> +
> +       ret = __clk_determine_rate(req->best_parent_hw, &parent_req);
> +       if (ret)
> +               return ret;
> +
> +       req->best_parent_hw = p;
> +       req->best_parent_rate = parent_req.rate;
> +       req->rate = parent_req.rate / 2;
> +
> +       return 0;
> +}
> +
> +static int clk_rcg2_gfx3d_set_rate(struct clk_hw *hw, unsigned long rate,
> +                                   unsigned long parent_rate)
> +{
> +       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +       int ret;
> +       u32 cfg;
> +
> +       /* Configure fixed SRC and DIV */
> +       cfg = rcg->parent_map[1].cfg << CFG_SRC_SEL_SHIFT;

This would need to be dynamic too instead of hardcoding '1'.

> +       cfg |= 0x3 << CFG_SRC_DIV_SHIFT;
> +
> +       ret = regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, cfg);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * In case clock is disabled, update the SRC and DIV only
> +        * and return without configuration update.

Add:

 When the clk is enabled, we'll configure the rate to be what we
 requested here. This allows other users of this clk (i.e. GPU)
 to change the rate when this clk is disabled from our perspective.

> +        */
> +       if (!__clk_is_enabled(hw->clk))
> +               return 0;
> +
> +       return update_config(rcg);
> +}

[PATCH v3 3/4] dt-bindings: clock: Introduce QCOM Graphics clock bindings

[Amit Nischal]

Add device tree bindings for graphics clock controller for
Qualcomm Technology Inc's SDM845 SoCs.

Signed-off-by: Amit Nischal <anischal@codeaurora.org>
---
 .../devicetree/bindings/clock/qcom,gpucc.txt       | 18 ++++++++++
 include/dt-bindings/clock/qcom,gpucc-sdm845.h      | 38 ++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.txt
 create mode 100644 include/dt-bindings/clock/qcom,gpucc-sdm845.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.txt b/Documentation/devicetree/bindings/clock/qcom,gpucc.txt
new file mode 100644
index 0000000..93752db
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,gpucc.txt
@@ -0,0 +1,18 @@
+Qualcomm Graphics Clock & Reset Controller Binding
+--------------------------------------------------
+
+Required properties :
+- compatible : shall contain "qcom,sdm845-gpucc".
+- reg : shall contain base register location and length.
+- #clock-cells : from common clock binding, shall contain 1.
+- #reset-cells : from common reset binding, shall contain 1.
+- #power-domain-cells : from generic power domain binding, shall contain 1.
+
+Example:
+	gpucc: clock-controller@5090000 {
+		compatible = "qcom,sdm845-gpucc";
+		reg = <0x5090000 0x9000>;
+		#clock-cells = <1>;
+		#reset-cells = <1>;
+		#power-domain-cells = <1>;
+	};
diff --git a/include/dt-bindings/clock/qcom,gpucc-sdm845.h b/include/dt-bindings/clock/qcom,gpucc-sdm845.h
new file mode 100644
index 0000000..643b42a
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,gpucc-sdm845.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_SDM_GPU_CC_SDM845_H
+#define _DT_BINDINGS_CLK_SDM_GPU_CC_SDM845_H
+
+/* GPU_CC clock registers */
+#define GPU_CC_CRC_AHB_CLK			0
+#define GPU_CC_CX_APB_CLK			1
+#define GPU_CC_CX_GFX3D_CLK			2
+#define GPU_CC_CX_GFX3D_SLV_CLK			3
+#define GPU_CC_CX_GMU_CLK			4
+#define GPU_CC_CX_SNOC_DVM_CLK			5
+#define GPU_CC_CXO_CLK				6
+#define GPU_CC_GMU_CLK_SRC			7
+#define GPU_CC_GX_GMU_CLK			8
+#define GPU_CC_GX_GFX3D_CLK_SRC			9
+#define GPU_CC_GX_GFX3D_CLK			10
+#define GPU_CC_PLL0				11
+#define GPU_CC_PLL0_OUT_EVEN			12
+#define GPU_CC_PLL1				13
+
+/* GPU_CC Resets */
+#define GPUCC_GPU_CC_ACD_BCR			0
+#define GPUCC_GPU_CC_CX_BCR			1
+#define GPUCC_GPU_CC_GFX3D_AON_BCR		2
+#define GPUCC_GPU_CC_GMU_BCR			3
+#define GPUCC_GPU_CC_GX_BCR			4
+#define GPUCC_GPU_CC_SPDM_BCR			5
+#define GPUCC_GPU_CC_XO_BCR			6
+
+/* GPU_CC GDSCRs */
+#define GPU_CX_GDSC				0
+#define GPU_GX_GDSC				1
+
+#endif
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v3 4/4] clk: qcom: Add graphics clock controller driver for SDM845

[Amit Nischal]

Add support for the graphics clock controller found on SDM845
based devices. This would allow graphics drivers to probe and
control their clocks.

Signed-off-by: Amit Nischal <anischal@codeaurora.org>
---
 drivers/clk/qcom/Kconfig        |   9 +
 drivers/clk/qcom/Makefile       |   1 +
 drivers/clk/qcom/gpucc-sdm845.c | 438 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 448 insertions(+)
 create mode 100644 drivers/clk/qcom/gpucc-sdm845.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 0647686..1595464 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -244,6 +244,15 @@ config SDM_GCC_845
 	  Say Y if you want to use peripheral devices such as UART, SPI,
 	  i2C, USB, UFS, SDDC, PCIe, etc.
 
+config SDM_GPUCC_845
+	tristate "SDM845 Graphics Clock Controller"
+	depends on COMMON_CLK_QCOM
+	select SDM_GCC_845
+	help
+	  Support for the graphics clock controller on SDM845 devices.
+	  Say Y if you want to support graphics controller devices and
+	  functionality such as 3D graphics.
+
 config SDM_VIDEOCC_845
 	tristate "SDM845 Video Clock Controller"
 	depends on COMMON_CLK_QCOM
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index c4ed36f..93c1089 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -42,5 +42,6 @@ obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
 obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
 obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
 obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
+obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
 obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
 obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/gpucc-sdm845.c b/drivers/clk/qcom/gpucc-sdm845.c
new file mode 100644
index 0000000..7a11b70
--- /dev/null
+++ b/drivers/clk/qcom/gpucc-sdm845.c
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,gpucc-sdm845.h>
+
+#include "common.h"
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-pll.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "gdsc.h"
+
+#define CX_GMU_CBCR_SLEEP_MASK		0xf
+#define CX_GMU_CBCR_SLEEP_SHIFT		4
+#define CX_GMU_CBCR_WAKE_MASK		0xf
+#define CX_GMU_CBCR_WAKE_SHIFT		8
+#define CLK_DIS_WAIT_SHIFT		12
+#define CLK_DIS_WAIT_MASK		(0xf << CLK_DIS_WAIT_SHIFT)
+
+enum {
+	P_BI_TCXO,
+	P_CORE_BI_PLL_TEST_SE,
+	P_GPLL0_OUT_MAIN,
+	P_GPLL0_OUT_MAIN_DIV,
+	P_GPU_CC_PLL0_OUT_EVEN,
+	P_GPU_CC_PLL0_OUT_MAIN,
+	P_GPU_CC_PLL0_OUT_ODD,
+	P_GPU_CC_PLL1_OUT_EVEN,
+	P_GPU_CC_PLL1_OUT_MAIN,
+	P_GPU_CC_PLL1_OUT_ODD,
+};
+
+static const struct parent_map gpu_cc_parent_map_0[] = {
+	{ P_BI_TCXO, 0 },
+	{ P_GPU_CC_PLL0_OUT_MAIN, 1 },
+	{ P_GPU_CC_PLL1_OUT_MAIN, 3 },
+	{ P_GPLL0_OUT_MAIN, 5 },
+	{ P_GPLL0_OUT_MAIN_DIV, 6 },
+	{ P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gpu_cc_parent_names_0[] = {
+	"bi_tcxo",
+	"gpu_cc_pll0",
+	"gpu_cc_pll1",
+	"gcc_gpu_gpll0_clk_src",
+	"gcc_gpu_gpll0_div_clk_src",
+	"core_bi_pll_test_se",
+};
+
+static const struct parent_map gpu_cc_parent_map_1[] = {
+	{ P_BI_TCXO, 0 },
+	{ P_GPU_CC_PLL0_OUT_EVEN, 1 },
+	{ P_GPU_CC_PLL0_OUT_ODD, 2 },
+	{ P_GPU_CC_PLL1_OUT_EVEN, 3 },
+	{ P_GPU_CC_PLL1_OUT_ODD, 4 },
+	{ P_GPLL0_OUT_MAIN, 5 },
+	{ P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gpu_cc_parent_names_1[] = {
+	"bi_tcxo",
+	"gpu_cc_pll0_out_even",
+	"gpu_cc_pll0_out_odd",
+	"gpu_cc_pll1_out_even",
+	"gpu_cc_pll1_out_odd",
+	"gcc_gpu_gpll0_clk_src",
+	"core_bi_pll_test_se",
+};
+
+static const struct alpha_pll_config gpu_cc_pll0_config = {
+	.l = 0x1d,
+	.alpha = 0x2aaa,
+};
+
+static const struct alpha_pll_config gpu_cc_pll1_config = {
+	.l = 0x1a,
+	.alpha = 0xaab,
+};
+
+static struct clk_alpha_pll gpu_cc_pll0 = {
+	.offset = 0x0,
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+	.clkr = {
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_pll0",
+			.parent_names = (const char *[]){ "bi_tcxo" },
+			.num_parents = 1,
+			.ops = &clk_alpha_pll_fabia_ops,
+		},
+	},
+};
+
+static const struct clk_div_table post_div_table_fabia_even[] = {
+	{ 0x0, 1 },
+};
+
+static struct clk_alpha_pll_postdiv gpu_cc_pll0_out_even = {
+	.offset = 0x0,
+	.post_div_shift = 8,
+	.post_div_table = post_div_table_fabia_even,
+	.num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
+	.width = 4,
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "gpu_cc_pll0_out_even",
+		.parent_names = (const char *[]){ "gpu_cc_pll0" },
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+		.ops = &clk_alpha_pll_postdiv_fabia_ops,
+	},
+};
+
+static struct clk_alpha_pll gpu_cc_pll1 = {
+	.offset = 0x100,
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+	.clkr = {
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_pll1",
+			.parent_names = (const char *[]){ "bi_tcxo" },
+			.num_parents = 1,
+			.ops = &clk_alpha_pll_fabia_ops,
+		},
+	},
+};
+
+static const struct freq_tbl ftbl_gpu_cc_gmu_clk_src[] = {
+	F(19200000, P_BI_TCXO, 1, 0, 0),
+	F(200000000, P_GPLL0_OUT_MAIN_DIV, 1.5, 0, 0),
+	F(500000000, P_GPU_CC_PLL1_OUT_MAIN, 1, 0, 0),
+	{ }
+};
+
+static struct clk_rcg2 gpu_cc_gmu_clk_src = {
+	.cmd_rcgr = 0x1120,
+	.mnd_width = 0,
+	.hid_width = 5,
+	.parent_map = gpu_cc_parent_map_0,
+	.freq_tbl = ftbl_gpu_cc_gmu_clk_src,
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "gpu_cc_gmu_clk_src",
+		.parent_names = gpu_cc_parent_names_0,
+		.num_parents = 6,
+		.ops = &clk_rcg2_shared_ops,
+	},
+};
+
+static struct clk_rcg2 gpu_cc_gx_gfx3d_clk_src = {
+	.cmd_rcgr = 0x101c,
+	.mnd_width = 0,
+	.hid_width = 5,
+	.parent_map = gpu_cc_parent_map_1,
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "gpu_cc_gx_gfx3d_clk_src",
+		.parent_names = gpu_cc_parent_names_1,
+		.num_parents = 7,
+		.flags = CLK_SET_RATE_PARENT,
+		.ops = &clk_rcg2_gfx3d_ops,
+	},
+};
+
+static struct clk_branch gpu_cc_crc_ahb_clk = {
+	.halt_reg = 0x107c,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x107c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_crc_ahb_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gpu_cc_cx_apb_clk = {
+	.halt_reg = 0x1088,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x1088,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_cx_apb_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gpu_cc_cx_gfx3d_clk = {
+	.halt_reg = 0x10a4,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x10a4,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_cx_gfx3d_clk",
+			.parent_names = (const char *[]){
+				"gpu_cc_gx_gfx3d_clk_src",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gpu_cc_cx_gfx3d_slv_clk = {
+	.halt_reg = 0x10a8,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x10a8,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_cx_gfx3d_slv_clk",
+			.parent_names = (const char *[]){
+				"gpu_cc_gx_gfx3d_clk_src",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gpu_cc_cx_gmu_clk = {
+	.halt_reg = 0x1098,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x1098,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_cx_gmu_clk",
+			.parent_names = (const char *[]){
+				"gpu_cc_gmu_clk_src",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gpu_cc_cx_snoc_dvm_clk = {
+	.halt_reg = 0x108c,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x108c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_cx_snoc_dvm_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gpu_cc_cxo_clk = {
+	.halt_reg = 0x109c,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x109c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_cxo_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gpu_cc_gx_gfx3d_clk = {
+	.halt_reg = 0x1054,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x1054,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_gx_gfx3d_clk",
+			.parent_names = (const char *[]){
+				"gpu_cc_gx_gfx3d_clk_src",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gpu_cc_gx_gmu_clk = {
+	.halt_reg = 0x1064,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x1064,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_gx_gmu_clk",
+			.parent_names = (const char *[]){
+				"gpu_cc_gmu_clk_src",
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct gdsc gpu_cx_gdsc = {
+	.gdscr = 0x106c,
+	.gds_hw_ctrl = 0x1540,
+	.pd = {
+		.name = "gpu_cx_gdsc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
+};
+
+static struct gdsc gpu_gx_gdsc = {
+	.gdscr = 0x100c,
+	.clamp_io_ctrl = 0x1508,
+	.pd = {
+		.name = "gpu_gx_gdsc",
+	},
+	.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,
+};
+
+static struct clk_regmap *gpu_cc_sdm845_clocks[] = {
+	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr,
+	[GPU_CC_CX_APB_CLK] = &gpu_cc_cx_apb_clk.clkr,
+	[GPU_CC_CX_GFX3D_CLK] = &gpu_cc_cx_gfx3d_clk.clkr,
+	[GPU_CC_CX_GFX3D_SLV_CLK] = &gpu_cc_cx_gfx3d_slv_clk.clkr,
+	[GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr,
+	[GPU_CC_CX_SNOC_DVM_CLK] = &gpu_cc_cx_snoc_dvm_clk.clkr,
+	[GPU_CC_CXO_CLK] = &gpu_cc_cxo_clk.clkr,
+	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
+	[GPU_CC_GX_GMU_CLK] = &gpu_cc_gx_gmu_clk.clkr,
+	[GPU_CC_GX_GFX3D_CLK_SRC] = &gpu_cc_gx_gfx3d_clk_src.clkr,
+	[GPU_CC_GX_GFX3D_CLK] = &gpu_cc_gx_gfx3d_clk.clkr,
+	[GPU_CC_PLL0] = &gpu_cc_pll0.clkr,
+	[GPU_CC_PLL0_OUT_EVEN] = &gpu_cc_pll0_out_even.clkr,
+	[GPU_CC_PLL1] = &gpu_cc_pll1.clkr,
+};
+
+static struct gdsc *gpu_cc_sdm845_gdscs[] = {
+	[GPU_CX_GDSC] = &gpu_cx_gdsc,
+	[GPU_GX_GDSC] = &gpu_gx_gdsc,
+};
+
+static const struct regmap_config gpu_cc_sdm845_regmap_config = {
+	.reg_bits	= 32,
+	.reg_stride	= 4,
+	.val_bits	= 32,
+	.max_register	= 0x8008,
+	.fast_io	= true,
+};
+
+static const struct qcom_cc_desc gpu_cc_sdm845_desc = {
+	.config = &gpu_cc_sdm845_regmap_config,
+	.clks = gpu_cc_sdm845_clocks,
+	.num_clks = ARRAY_SIZE(gpu_cc_sdm845_clocks),
+	.gdscs = gpu_cc_sdm845_gdscs,
+	.num_gdscs = ARRAY_SIZE(gpu_cc_sdm845_gdscs),
+};
+
+static const struct of_device_id gpu_cc_sdm845_match_table[] = {
+	{ .compatible = "qcom,sdm845-gpucc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gpu_cc_sdm845_match_table);
+
+static int gpu_cc_sdm845_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+	unsigned int value, mask;
+	int ret;
+
+	regmap = qcom_cc_map(pdev, &gpu_cc_sdm845_desc);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	clk_fabia_pll_configure(&gpu_cc_pll0, regmap, &gpu_cc_pll0_config);
+	clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll1_config);
+
+	/*
+	 * Configure gpu_cc_cx_gmu_clk with recommended
+	 * wakeup/sleep settings
+	 */
+	mask = CX_GMU_CBCR_WAKE_MASK << CX_GMU_CBCR_WAKE_SHIFT;
+	mask |= CX_GMU_CBCR_SLEEP_MASK << CX_GMU_CBCR_SLEEP_SHIFT;
+	value = 0xf << CX_GMU_CBCR_WAKE_SHIFT | 0xf << CX_GMU_CBCR_SLEEP_SHIFT;
+	regmap_update_bits(regmap, 0x1098, mask, value);
+
+	ret = qcom_cc_really_probe(pdev, &gpu_cc_sdm845_desc, regmap);
+	if (ret)
+		return ret;
+
+	/* Configure clk_dis_wait for gpu_cx_gdsc */
+	regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
+						8 << CLK_DIS_WAIT_SHIFT);
+
+	/* Set supported range of frequencies for gfx3d clock */
+	clk_hw_set_rate_range(&gpu_cc_gx_gfx3d_clk_src.clkr.hw, 180000000,
+								710000000);
+
+	return 0;
+}
+
+static struct platform_driver gpu_cc_sdm845_driver = {
+	.probe = gpu_cc_sdm845_probe,
+	.driver = {
+		.name = "sdm845-gpucc",
+		.of_match_table = gpu_cc_sdm845_match_table,
+	},
+};
+
+static int __init gpu_cc_sdm845_init(void)
+{
+	return platform_driver_register(&gpu_cc_sdm845_driver);
+}
+subsys_initcall(gpu_cc_sdm845_init);
+
+static void __exit gpu_cc_sdm845_exit(void)
+{
+	platform_driver_unregister(&gpu_cc_sdm845_driver);
+}
+module_exit(gpu_cc_sdm845_exit);
+
+MODULE_DESCRIPTION("QTI GPUCC SDM845 Driver");
+MODULE_LICENSE("GPL v2");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 4/4] clk: qcom: Add graphics clock controller driver for SDM845

[Stephen Boyd]

Quoting Amit Nischal (2018-08-12 23:33:07)
> +
> +static int gpu_cc_sdm845_probe(struct platform_device *pdev)
> +{
> +       struct regmap *regmap;
> +       unsigned int value, mask;
> +       int ret;
> +
> +       regmap = qcom_cc_map(pdev, &gpu_cc_sdm845_desc);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       clk_fabia_pll_configure(&gpu_cc_pll0, regmap, &gpu_cc_pll0_config);
> +       clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll1_config);
> +
> +       /*
> +        * Configure gpu_cc_cx_gmu_clk with recommended
> +        * wakeup/sleep settings
> +        */
> +       mask = CX_GMU_CBCR_WAKE_MASK << CX_GMU_CBCR_WAKE_SHIFT;
> +       mask |= CX_GMU_CBCR_SLEEP_MASK << CX_GMU_CBCR_SLEEP_SHIFT;
> +       value = 0xf << CX_GMU_CBCR_WAKE_SHIFT | 0xf << CX_GMU_CBCR_SLEEP_SHIFT;
> +       regmap_update_bits(regmap, 0x1098, mask, value);
> +
> +       ret = qcom_cc_really_probe(pdev, &gpu_cc_sdm845_desc, regmap);
> +       if (ret)
> +               return ret;
> +
> +       /* Configure clk_dis_wait for gpu_cx_gdsc */
> +       regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
> +                                               8 << CLK_DIS_WAIT_SHIFT);

Is there a reason this is done after clks are registered? I'd think we
would want to do it before.

> +
> +       /* Set supported range of frequencies for gfx3d clock */
> +       clk_hw_set_rate_range(&gpu_cc_gx_gfx3d_clk_src.clkr.hw, 180000000,
> +                                                               710000000);
> +
> +       return 0;
> +}

Re: [PATCH v3 4/4] clk: qcom: Add graphics clock controller driver for SDM845

[Taniya Das]

Hello Stephen,

On 11/5/2018 12:07 PM, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-08-12 23:33:07)
>> +
>> +static int gpu_cc_sdm845_probe(struct platform_device *pdev)
>> +{
>> +       struct regmap *regmap;
>> +       unsigned int value, mask;
>> +       int ret;
>> +
>> +       regmap = qcom_cc_map(pdev, &gpu_cc_sdm845_desc);
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
>> +
>> +       clk_fabia_pll_configure(&gpu_cc_pll0, regmap, &gpu_cc_pll0_config);
>> +       clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll1_config);
>> +
>> +       /*
>> +        * Configure gpu_cc_cx_gmu_clk with recommended
>> +        * wakeup/sleep settings
>> +        */
>> +       mask = CX_GMU_CBCR_WAKE_MASK << CX_GMU_CBCR_WAKE_SHIFT;
>> +       mask |= CX_GMU_CBCR_SLEEP_MASK << CX_GMU_CBCR_SLEEP_SHIFT;
>> +       value = 0xf << CX_GMU_CBCR_WAKE_SHIFT | 0xf << CX_GMU_CBCR_SLEEP_SHIFT;
>> +       regmap_update_bits(regmap, 0x1098, mask, value);
>> +
>> +       ret = qcom_cc_really_probe(pdev, &gpu_cc_sdm845_desc, regmap);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Configure clk_dis_wait for gpu_cx_gdsc */
>> +       regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
>> +                                               8 << CLK_DIS_WAIT_SHIFT);
> 
> Is there a reason this is done after clks are registered? I'd think we
> would want to do it before.
> 

Yes, it could be done before, would move it.

>> +
>> +       /* Set supported range of frequencies for gfx3d clock */
>> +       clk_hw_set_rate_range(&gpu_cc_gx_gfx3d_clk_src.clkr.hw, 180000000,
>> +                                                               710000000);
>> +
>> +       return 0;
>> +}

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [PATCH v3 4/4] clk: qcom: Add graphics clock controller driver for SDM845

[Jordan Crouse]

On Mon, Aug 13, 2018 at 12:03:07PM +0530, Amit Nischal wrote:
> Add support for the graphics clock controller found on SDM845
> based devices. This would allow graphics drivers to probe and
> control their clocks.
> 
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> ---
>  drivers/clk/qcom/Kconfig        |   9 +
>  drivers/clk/qcom/Makefile       |   1 +
>  drivers/clk/qcom/gpucc-sdm845.c | 438 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 448 insertions(+)
>  create mode 100644 drivers/clk/qcom/gpucc-sdm845.c
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 0647686..1595464 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -244,6 +244,15 @@ config SDM_GCC_845
>  	  Say Y if you want to use peripheral devices such as UART, SPI,
>  	  i2C, USB, UFS, SDDC, PCIe, etc.
>  
> +config SDM_GPUCC_845
> +	tristate "SDM845 Graphics Clock Controller"
> +	depends on COMMON_CLK_QCOM
> +	select SDM_GCC_845
> +	help
> +	  Support for the graphics clock controller on SDM845 devices.
> +	  Say Y if you want to support graphics controller devices and
> +	  functionality such as 3D graphics.
> +
>  config SDM_VIDEOCC_845
>  	tristate "SDM845 Video Clock Controller"
>  	depends on COMMON_CLK_QCOM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index c4ed36f..93c1089 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -42,5 +42,6 @@ obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
>  obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
>  obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
>  obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
> +obj-$(CONFIG_SDM_GPUCC_845) += gpucc-sdm845.o
>  obj-$(CONFIG_SDM_VIDEOCC_845) += videocc-sdm845.o
>  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> diff --git a/drivers/clk/qcom/gpucc-sdm845.c b/drivers/clk/qcom/gpucc-sdm845.c
> new file mode 100644
> index 0000000..7a11b70
> --- /dev/null
> +++ b/drivers/clk/qcom/gpucc-sdm845.c
> @@ -0,0 +1,438 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,gpucc-sdm845.h>
> +
> +#include "common.h"
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-pll.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "gdsc.h"
> +
> +#define CX_GMU_CBCR_SLEEP_MASK		0xf
> +#define CX_GMU_CBCR_SLEEP_SHIFT		4
> +#define CX_GMU_CBCR_WAKE_MASK		0xf
> +#define CX_GMU_CBCR_WAKE_SHIFT		8
> +#define CLK_DIS_WAIT_SHIFT		12
> +#define CLK_DIS_WAIT_MASK		(0xf << CLK_DIS_WAIT_SHIFT)
> +
> +enum {
> +	P_BI_TCXO,
> +	P_CORE_BI_PLL_TEST_SE,
> +	P_GPLL0_OUT_MAIN,
> +	P_GPLL0_OUT_MAIN_DIV,
> +	P_GPU_CC_PLL0_OUT_EVEN,
> +	P_GPU_CC_PLL0_OUT_MAIN,
> +	P_GPU_CC_PLL0_OUT_ODD,
> +	P_GPU_CC_PLL1_OUT_EVEN,
> +	P_GPU_CC_PLL1_OUT_MAIN,
> +	P_GPU_CC_PLL1_OUT_ODD,
> +};
> +
> +static const struct parent_map gpu_cc_parent_map_0[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_GPU_CC_PLL0_OUT_MAIN, 1 },
> +	{ P_GPU_CC_PLL1_OUT_MAIN, 3 },
> +	{ P_GPLL0_OUT_MAIN, 5 },
> +	{ P_GPLL0_OUT_MAIN_DIV, 6 },
> +	{ P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const char * const gpu_cc_parent_names_0[] = {
> +	"bi_tcxo",
> +	"gpu_cc_pll0",
> +	"gpu_cc_pll1",
> +	"gcc_gpu_gpll0_clk_src",
> +	"gcc_gpu_gpll0_div_clk_src",
> +	"core_bi_pll_test_se",
> +};
> +
> +static const struct parent_map gpu_cc_parent_map_1[] = {
> +	{ P_BI_TCXO, 0 },
> +	{ P_GPU_CC_PLL0_OUT_EVEN, 1 },
> +	{ P_GPU_CC_PLL0_OUT_ODD, 2 },
> +	{ P_GPU_CC_PLL1_OUT_EVEN, 3 },
> +	{ P_GPU_CC_PLL1_OUT_ODD, 4 },
> +	{ P_GPLL0_OUT_MAIN, 5 },
> +	{ P_CORE_BI_PLL_TEST_SE, 7 },
> +};
> +
> +static const char * const gpu_cc_parent_names_1[] = {
> +	"bi_tcxo",
> +	"gpu_cc_pll0_out_even",
> +	"gpu_cc_pll0_out_odd",
> +	"gpu_cc_pll1_out_even",
> +	"gpu_cc_pll1_out_odd",
> +	"gcc_gpu_gpll0_clk_src",
> +	"core_bi_pll_test_se",
> +};
> +
> +static const struct alpha_pll_config gpu_cc_pll0_config = {
> +	.l = 0x1d,
> +	.alpha = 0x2aaa,
> +};
> +
> +static const struct alpha_pll_config gpu_cc_pll1_config = {
> +	.l = 0x1a,
> +	.alpha = 0xaab,
> +};
> +
> +static struct clk_alpha_pll gpu_cc_pll0 = {
> +	.offset = 0x0,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> +	.clkr = {
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_pll0",
> +			.parent_names = (const char *[]){ "bi_tcxo" },
> +			.num_parents = 1,
> +			.ops = &clk_alpha_pll_fabia_ops,
> +		},
> +	},
> +};
> +
> +static const struct clk_div_table post_div_table_fabia_even[] = {
> +	{ 0x0, 1 },
> +};
> +
> +static struct clk_alpha_pll_postdiv gpu_cc_pll0_out_even = {
> +	.offset = 0x0,
> +	.post_div_shift = 8,
> +	.post_div_table = post_div_table_fabia_even,
> +	.num_post_div = ARRAY_SIZE(post_div_table_fabia_even),
> +	.width = 4,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "gpu_cc_pll0_out_even",
> +		.parent_names = (const char *[]){ "gpu_cc_pll0" },
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +		.ops = &clk_alpha_pll_postdiv_fabia_ops,
> +	},
> +};
> +
> +static struct clk_alpha_pll gpu_cc_pll1 = {
> +	.offset = 0x100,
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> +	.clkr = {
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_pll1",
> +			.parent_names = (const char *[]){ "bi_tcxo" },
> +			.num_parents = 1,
> +			.ops = &clk_alpha_pll_fabia_ops,
> +		},
> +	},
> +};
> +
> +static const struct freq_tbl ftbl_gpu_cc_gmu_clk_src[] = {
> +	F(19200000, P_BI_TCXO, 1, 0, 0),
> +	F(200000000, P_GPLL0_OUT_MAIN_DIV, 1.5, 0, 0),
> +	F(500000000, P_GPU_CC_PLL1_OUT_MAIN, 1, 0, 0),
> +	{ }
> +};
> +
> +static struct clk_rcg2 gpu_cc_gmu_clk_src = {
> +	.cmd_rcgr = 0x1120,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = gpu_cc_parent_map_0,
> +	.freq_tbl = ftbl_gpu_cc_gmu_clk_src,
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "gpu_cc_gmu_clk_src",
> +		.parent_names = gpu_cc_parent_names_0,
> +		.num_parents = 6,
> +		.ops = &clk_rcg2_shared_ops,
> +	},
> +};
> +
> +static struct clk_rcg2 gpu_cc_gx_gfx3d_clk_src = {
> +	.cmd_rcgr = 0x101c,
> +	.mnd_width = 0,
> +	.hid_width = 5,
> +	.parent_map = gpu_cc_parent_map_1,
> +	.clkr.hw.init = &(struct clk_init_data){
> +		.name = "gpu_cc_gx_gfx3d_clk_src",
> +		.parent_names = gpu_cc_parent_names_1,
> +		.num_parents = 7,
> +		.flags = CLK_SET_RATE_PARENT,
> +		.ops = &clk_rcg2_gfx3d_ops,
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_crc_ahb_clk = {
> +	.halt_reg = 0x107c,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x107c,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_crc_ahb_clk",
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_cx_apb_clk = {
> +	.halt_reg = 0x1088,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x1088,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_cx_apb_clk",
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_cx_gfx3d_clk = {
> +	.halt_reg = 0x10a4,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x10a4,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_cx_gfx3d_clk",
> +			.parent_names = (const char *[]){
> +				"gpu_cc_gx_gfx3d_clk_src",
> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_cx_gfx3d_slv_clk = {
> +	.halt_reg = 0x10a8,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x10a8,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_cx_gfx3d_slv_clk",
> +			.parent_names = (const char *[]){
> +				"gpu_cc_gx_gfx3d_clk_src",
> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_cx_gmu_clk = {
> +	.halt_reg = 0x1098,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x1098,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_cx_gmu_clk",
> +			.parent_names = (const char *[]){
> +				"gpu_cc_gmu_clk_src",
> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_cx_snoc_dvm_clk = {
> +	.halt_reg = 0x108c,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x108c,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_cx_snoc_dvm_clk",
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_cxo_clk = {
> +	.halt_reg = 0x109c,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x109c,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_cxo_clk",
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_gx_gfx3d_clk = {
> +	.halt_reg = 0x1054,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x1054,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_gx_gfx3d_clk",
> +			.parent_names = (const char *[]){
> +				"gpu_cc_gx_gfx3d_clk_src",
> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct clk_branch gpu_cc_gx_gmu_clk = {
> +	.halt_reg = 0x1064,
> +	.halt_check = BRANCH_HALT,
> +	.clkr = {
> +		.enable_reg = 0x1064,
> +		.enable_mask = BIT(0),
> +		.hw.init = &(struct clk_init_data){
> +			.name = "gpu_cc_gx_gmu_clk",
> +			.parent_names = (const char *[]){
> +				"gpu_cc_gmu_clk_src",
> +			},
> +			.num_parents = 1,
> +			.flags = CLK_SET_RATE_PARENT,
> +			.ops = &clk_branch2_ops,
> +		},
> +	},
> +};
> +
> +static struct gdsc gpu_cx_gdsc = {
> +	.gdscr = 0x106c,
> +	.gds_hw_ctrl = 0x1540,
> +	.pd = {
> +		.name = "gpu_cx_gdsc",
> +	},
> +	.pwrsts = PWRSTS_OFF_ON,
> +	.flags = VOTABLE,
> +};
> +
> +static struct gdsc gpu_gx_gdsc = {
> +	.gdscr = 0x100c,
> +	.clamp_io_ctrl = 0x1508,
> +	.pd = {
> +		.name = "gpu_gx_gdsc",
> +	},
> +	.clk_hws = {
> +		&gpu_cc_gx_gfx3d_clk_src.clkr.hw,
> +	},
> +	.clk_count = 1,

So from my perspective, this isn't needed - it at least seems that if the CX
domain is still enabled that I can safely toggle this gdsc off even if these
clocks are not defined. Full disclosure, my experiment has the "enable" hacked
out so its possible that might have needed this in some past life but from what
I can tell we won't need it upstream.

> +	.pwrsts = PWRSTS_OFF_ON,
> +	.flags = CLAMP_IO | AON_RESET | POLL_CFG_GDSCR,
> +};
> +
> +static struct clk_regmap *gpu_cc_sdm845_clocks[] = {
> +	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_clk.clkr,

It does not appear that we control this clock from the CPU.

> +	[GPU_CC_CX_APB_CLK] = &gpu_cc_cx_apb_clk.clkr,

Same.

> +	[GPU_CC_CX_GFX3D_CLK] = &gpu_cc_cx_gfx3d_clk.clkr,

Same.

> +	[GPU_CC_CX_GFX3D_SLV_CLK] = &gpu_cc_cx_gfx3d_slv_clk.clkr,

Same.

> +	[GPU_CC_CX_GMU_CLK] = &gpu_cc_cx_gmu_clk.clkr,
> +	[GPU_CC_CX_SNOC_DVM_CLK] = &gpu_cc_cx_snoc_dvm_clk.clkr,

Same.

> +	[GPU_CC_CXO_CLK] = &gpu_cc_cxo_clk.clkr,
> +	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
> +	[GPU_CC_GX_GMU_CLK] = &gpu_cc_gx_gmu_clk.clkr,

Same.

> +	[GPU_CC_GX_GFX3D_CLK_SRC] = &gpu_cc_gx_gfx3d_clk_src.clkr,
> +	[GPU_CC_GX_GFX3D_CLK] = &gpu_cc_gx_gfx3d_clk.clkr,

With the discussion above, these are no longer used.

> +	[GPU_CC_PLL0] = &gpu_cc_pll0.clkr,
> +	[GPU_CC_PLL0_OUT_EVEN] = &gpu_cc_pll0_out_even.clkr,

If GX_GFX3D goes away, this can too.

> +	[GPU_CC_PLL1] = &gpu_cc_pll1.clkr,
> +};
> +
> +static struct gdsc *gpu_cc_sdm845_gdscs[] = {
> +	[GPU_CX_GDSC] = &gpu_cx_gdsc,
> +	[GPU_GX_GDSC] = &gpu_gx_gdsc,
> +};
> +
> +static const struct regmap_config gpu_cc_sdm845_regmap_config = {
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x8008,
> +	.fast_io	= true,
> +};
> +
> +static const struct qcom_cc_desc gpu_cc_sdm845_desc = {
> +	.config = &gpu_cc_sdm845_regmap_config,
> +	.clks = gpu_cc_sdm845_clocks,
> +	.num_clks = ARRAY_SIZE(gpu_cc_sdm845_clocks),
> +	.gdscs = gpu_cc_sdm845_gdscs,
> +	.num_gdscs = ARRAY_SIZE(gpu_cc_sdm845_gdscs),
> +};
> +
> +static const struct of_device_id gpu_cc_sdm845_match_table[] = {
> +	{ .compatible = "qcom,sdm845-gpucc" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, gpu_cc_sdm845_match_table);
> +
> +static int gpu_cc_sdm845_probe(struct platform_device *pdev)
> +{
> +	struct regmap *regmap;
> +	unsigned int value, mask;
> +	int ret;
> +
> +	regmap = qcom_cc_map(pdev, &gpu_cc_sdm845_desc);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	clk_fabia_pll_configure(&gpu_cc_pll0, regmap, &gpu_cc_pll0_config);
> +	clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll1_config);
> +
> +	/*
> +	 * Configure gpu_cc_cx_gmu_clk with recommended
> +	 * wakeup/sleep settings
> +	 */
> +	mask = CX_GMU_CBCR_WAKE_MASK << CX_GMU_CBCR_WAKE_SHIFT;
> +	mask |= CX_GMU_CBCR_SLEEP_MASK << CX_GMU_CBCR_SLEEP_SHIFT;
> +	value = 0xf << CX_GMU_CBCR_WAKE_SHIFT | 0xf << CX_GMU_CBCR_SLEEP_SHIFT;
> +	regmap_update_bits(regmap, 0x1098, mask, value);
> +
> +	ret = qcom_cc_really_probe(pdev, &gpu_cc_sdm845_desc, regmap);
> +	if (ret)
> +		return ret;
> +
> +	/* Configure clk_dis_wait for gpu_cx_gdsc */
> +	regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
> +						8 << CLK_DIS_WAIT_SHIFT);
> +
> +	/* Set supported range of frequencies for gfx3d clock */
> +	clk_hw_set_rate_range(&gpu_cc_gx_gfx3d_clk_src.clkr.hw, 180000000,
> +								710000000);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver gpu_cc_sdm845_driver = {
> +	.probe = gpu_cc_sdm845_probe,
> +	.driver = {
> +		.name = "sdm845-gpucc",
> +		.of_match_table = gpu_cc_sdm845_match_table,
> +	},
> +};
> +
> +static int __init gpu_cc_sdm845_init(void)
> +{
> +	return platform_driver_register(&gpu_cc_sdm845_driver);
> +}
> +subsys_initcall(gpu_cc_sdm845_init);
> +
> +static void __exit gpu_cc_sdm845_exit(void)
> +{
> +	platform_driver_unregister(&gpu_cc_sdm845_driver);
> +}
> +module_exit(gpu_cc_sdm845_exit);
> +
> +MODULE_DESCRIPTION("QTI GPUCC SDM845 Driver");
> +MODULE_LICENSE("GPL v2");

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

Re: [PATCH v3 0/4] Add QCOM graphics clock controller driver for SDM845

[Jordan Crouse]

On Mon, Aug 13, 2018 at 12:03:03PM +0530, Amit Nischal wrote:
> Changes in v3:
> * Modified the determine_rate() op to use the min/max rate range
>   to round the requested rate within the set_rate range. With this,
>   requested set rate will always stay within the limits.
> 
> Changes in v2:
> Addressed review comments given by Stephen: https://lkml.org/lkml/2018/6/6/294
> * Introduce clk_rcg2_gfx3d_determine_rate ops to return the best parent
>   as 'gpucc_pll0_even' and best parent rate as twice of the requested rate
>   always. This will eliminate the need of frequency table as source and
>   div-2 are fixed for gfx3d_clk_src.
>   Also modified the clk_rcg2_set_rate ops to configure the fixed source and
>   div.
> * Add support to check if requested rate falls within allowed set_rate range.
>   This will not let the source gpucc_pll0 to go out of the supported range
>   and also client can request the rate within the range.
> * Fixed comment text in probe function and added module description for GPUCC
>   driver.
> 
> The graphics clock driver depends upon the below change.
> 	https://lkml.org/lkml/2018/6/23/108
> 
> Changes in v1:
> This patch series adds support for graphics clock controller for SDM845.
> Below is the brief description for each change:
> 
> 1. For some of the GDSCs, there is requirement to enable/disable the
>    few clocks before turning on/off the gdsc power domain. This patch
>    will add support to enable/disable the clock associated with the
>    gdsc along with power domain on/off callbacks.
> 
> 2. To turn on the gpu_gx_gdsc, there is a hardware requirement to
>    turn on the root clock (GFX3D RCG) first which would be the turn
>    on signal for the gdsc along with the SW_COLLAPSE. As per the
>    current implementation of clk_rcg2_shared_ops, it clears the
>    root_enable bit in the enable() clock ops. But due to the above
>    said requirement for GFX3D shared RCG, root_enable bit would be
>    already set by gdsc driver and rcg2_shared_ops should not clear
>    the root unless the disable() is called.
> 
>    This patch add support for the same by reusing the existing
>    clk_rcg2_shared_ops and deriving "clk_rcg2_gfx3d_ops" clk_ops
>    for GFX3D clock to take care of the root set/clear requirement.
> 
> 3. Add device tree bindings for graphics clock controller for SDM845.
> 
> 4. Add graphics clock controller (GPUCC) driver for SDM845.
> 
> [v1] : https://lore.kernel.org/patchwork/project/lkml/list/?series=348697
> [v2] : https://lore.kernel.org/patchwork/project/lkml/list/?series=359012
> 
> Amit Nischal (4):
>   clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC
>   clk: qcom: Add clk_rcg2_gfx3d_ops for SDM845
>   dt-bindings: clock: Introduce QCOM Graphics clock bindings
>   clk: qcom: Add graphics clock controller driver for SDM845
> 
>  .../devicetree/bindings/clock/qcom,gpucc.txt       |  18 +
>  drivers/clk/qcom/Kconfig                           |   9 +
>  drivers/clk/qcom/Makefile                          |   1 +
>  drivers/clk/qcom/clk-rcg.h                         |   1 +
>  drivers/clk/qcom/clk-rcg2.c                        |  86 +++-
>  drivers/clk/qcom/gdsc.c                            |  44 +++
>  drivers/clk/qcom/gdsc.h                            |   5 +
>  drivers/clk/qcom/gpucc-sdm845.c                    | 438 +++++++++++++++++++++
>  include/dt-bindings/clock/qcom,gpucc-sdm845.h      |  38 ++
>  9 files changed, 638 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.txt
>  create mode 100644 drivers/clk/qcom/gpucc-sdm845.c
>  create mode 100644 include/dt-bindings/clock/qcom,gpucc-sdm845.h

This seems as good a place as any to kick off this discussion since it will
influence what these patches look like the next go around. Settle in because
this is going to get wordy...

The sdm845 GPU is comprised of two nested power domains: CX and GX. Put simply
from a CPU perspective the CX domain controls the GMU which is a microprocessor
co-located with the GPU. The GMU in turn controls the GX domain which powers the
GPU hardware. Under normal conditions the CPU powers on the CX domain and boots
the GMU and then the GMU turns on the GX and controls it under the assumption
that the GMU firmware can handle power collapse much faster than the CPU. The
GMU continues to control the GX domain up and down until we are idle for long
enough that we want to suspend the CX domain as well.

When we want to suspend the GMU we trigger a shutdown process on the GMU
which turns off GX and then the CPU turns off CX through the usual
pm_runtime_put() sequence. Under these ideal circumstances the CPU should have
no visibility into the GX at all - we should never control any headswitches or
clocks anywhere in that domain. Except....

There is one case where the CPU needs to touch the GX: due to power sequencing
requirements the GX needs to be turned off before the CX (and the CX needs to be
enabled before the GX during power up). In the very rare situation where the
GMU itself crashes and leaves the GX headswitch on the CPU needs to reach in and
turn off the GX before turning off the CX and rebooting the GMU. This is
problematic from a CPU perspective because there is no way to just go in and
hack on the register directly; we need some infrastructure set up and ready to
handle the odd use case.

We discussed this a while at LPC and Stephen had a good idea. We should keep the
GX gdsc but hack it so that the enable function does nothing (because we don't
want the GPU to turn on the headswitch ever) but leave the disable function as
is. Then we attach that genpd with dev_pm_attach_by_name() to the GMU device
and power enable/disable it at the appropriate times. Most of the time the
disable call will write to an already disabled headswitch but in that very
special situation it will actually turn off the headswitch and the world can be
right again.

So I did this and it works. At least it works for the regular case. It is
really difficult to cause the GMU to fault with the GX left on; I think I'm
going to need to hack on the firmware to test that path but I've verified that
the modified GDSC is toggling the power off before we suspend the CX so I think
thats a win.

I'll post patches against this patchset for an RFC but I mention it here because
there are a bunch of clocks defined in gpucc-sdm845.c that are not needed (
either we never call them because we don't touch GX clocks or they are parents
of uneeded clocks). It also seems like we don't need the clk_hws/clk_count
infrastructure in gdsc.c. From my testing I can safely toggle the gx gdsc off
from the CPU without 'gpu_cc_gx_gfx3d_clk_src'.

Obviously we'll need some more review and I need the actual clock experts to
weigh in but this is encouraging news because I think it lets us move ahead with
gpucc and also solve my little nagging hardware workaround issue to boot.

Look for patches shortly, in the mean time I'm going to comment to point out the
clocks I don't think we need any more so Taniya can make a new patchset.

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