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

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

[Amit Nischal]

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() and set_rate() 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.

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                        |  78 +++-
 drivers/clk/qcom/gdsc.c                            |  44 ++
 drivers/clk/qcom/gdsc.h                            |   5 +
 drivers/clk/qcom/gpucc-sdm845.c                    | 441 +++++++++++++++++++++
 include/dt-bindings/clock/qcom,gpucc-sdm845.h      |  38 ++
 9 files changed, 614 insertions(+), 21 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 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 1/4] clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC

[Stephen Boyd]

Quoting Amit Nischal (2018-06-06 04:41:45)
> 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

Why is there a requirement? Do the clks need to be in hw control mode or
they can't be turned off when the GDSC is off? It's hard for me to
understand with these vague statements.

> 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>

Ugh.

> +#include <linux/clk-provider.h>

Both, really?

>  #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;
> +}
> +

Looks an awful lot like bulk_enable clk API.

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

[Amit Nischal]

Hi Stephen,

Thanks for the review comments.

Regards,
Amit

On 2018-07-09 11:04, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-06-06 04:41:45)
>> 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
> 
> Why is there a requirement? Do the clks need to be in hw control mode 
> or
> they can't be turned off when the GDSC is off? It's hard for me to
> understand with these vague statements.
> 

This requirement is primarily to turn on the GPU GX GDSC and these 
clocks
do not need to be in HW control mode and clock disable is not related
with the GDSC.

To turn on the GX GDSC, root clock (GFX3D RCG) needs to be enabled first
before writing to SW_COLLAPSE bit of the GDSC. The CLK_ON signal from 
RCG
would power up the GPU GX GDSC.


>> 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>
> 
> Ugh.
> 
>> +#include <linux/clk-provider.h>
> 
> Both, really?
> 

Above includes are required else we get a compilation error as below:
error: dereferencing pointer to incomplete type
    ret = clk_prepare_enable(sc->clk_hws[i]->clk);
                                           ^

>>  #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;
>> +}
>> +
> 
> Looks an awful lot like bulk_enable clk API.

As mentioned above, ROOT_EN bit of GFX3D RCG needs to be enabled first 
to
turn on the GDSC. We want this enable to happen only through clock 
framework
API in order to avoid stability issues.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Boyd]

Quoting Amit Nischal (2018-07-12 05:23:48)
> On 2018-07-09 11:04, Stephen Boyd wrote:
> > Quoting Amit Nischal (2018-06-06 04:41:45)
> >> 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
> > 
> > Why is there a requirement? Do the clks need to be in hw control mode 
> > or
> > they can't be turned off when the GDSC is off? It's hard for me to
> > understand with these vague statements.
> > 
> 
> This requirement is primarily to turn on the GPU GX GDSC and these 
> clocks
> do not need to be in HW control mode and clock disable is not related
> with the GDSC.

Ok that's good to know.

> 
> To turn on the GX GDSC, root clock (GFX3D RCG) needs to be enabled first
> before writing to SW_COLLAPSE bit of the GDSC. The CLK_ON signal from 
> RCG
> would power up the GPU GX GDSC.

Can you please put this specific information in the commit text instead
of making a vague statement about GDSC hardware configurations?

Does anything go wrong if the GDSC is enabled from genpd but doesn't
actually turn on until the GFX3D RCG root bit is enabled or disabled by
the clk enable call? I suppose we won't know if the GDSC is enabled or
not until the clk is enabled? Maybe we should make the clk enable of the
RCG for GPU go check the GDSC status bit as well to make sure it's
toggling on or off?

Also, does the RCG turn on when the GX GDSC is off? I think we may be
able to rely on the GPU driver to "do the right thing" and enable the
GPU CX GDSC first, then the RCG and branch for the GFX3D clk, and then
the GPU GX GDSC for the core GPU logic. Then we don't need to do
anything special in the GDSC code for this.

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

[Amit Nischal]

On 2018-07-25 12:22, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-07-12 05:23:48)
>> On 2018-07-09 11:04, Stephen Boyd wrote:
>> > Quoting Amit Nischal (2018-06-06 04:41:45)
>> >> 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
>> >
>> > Why is there a requirement? Do the clks need to be in hw control mode
>> > or
>> > they can't be turned off when the GDSC is off? It's hard for me to
>> > understand with these vague statements.
>> >
>> 
>> This requirement is primarily to turn on the GPU GX GDSC and these
>> clocks
>> do not need to be in HW control mode and clock disable is not related
>> with the GDSC.
> 
> Ok that's good to know.
> 
>> 
>> To turn on the GX GDSC, root clock (GFX3D RCG) needs to be enabled 
>> first
>> before writing to SW_COLLAPSE bit of the GDSC. The CLK_ON signal from
>> RCG
>> would power up the GPU GX GDSC.
> 
> Can you please put this specific information in the commit text instead
> of making a vague statement about GDSC hardware configurations?
> 
> Does anything go wrong if the GDSC is enabled from genpd but doesn't
> actually turn on until the GFX3D RCG root bit is enabled or disabled by
> the clk enable call? I suppose we won't know if the GDSC is enabled or
> not until the clk is enabled? Maybe we should make the clk enable of 
> the
> RCG for GPU go check the GDSC status bit as well to make sure it's
> toggling on or off?

As per the current design, before turning on the GDSC(writing to the 
GDSCR
register) we are setting the ROOT_EN bit of RCG and GDSC's status will 
be
still off without clk_enable call to RCG even though we enable the GDSC.
clk_enable for RCG is CLK_ON signal for the GPU_GX_GDSC.

> 
> Also, does the RCG turn on when the GX GDSC is off? I think we may be
> able to rely on the GPU driver to "do the right thing" and enable the
> GPU CX GDSC first, then the RCG and branch for the GFX3D clk, and then
> the GPU GX GDSC for the core GPU logic. Then we don't need to do
> anything special in the GDSC code for this.

Its a GPU_GX_GDSC requirement to enable the RCG first and then GX_GDSC.
We want all of this sequence to be done by the GDSC driver so that 
client
only call for clk_apis for the clock branch. For clients, It will be
extra overhead to follow the below sequence.
GPU_CX_GDSC enable -> Enable RCG -> Enable GPU_GX_GDSC -> Enable Branch.

[PATCH 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() and set_rate() 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.

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 | 78 +++++++++++++++++++++++++++++++++------------
 2 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index b209a2f..c8c9558 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -160,5 +160,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..491e710 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -823,28 +823,12 @@ static int clk_rcg2_clear_force_enable(struct clk_hw *hw)
 					CMD_ROOT_EN, 0);
 }

-static int
-clk_rcg2_shared_force_enable_clear(struct clk_hw *hw, const struct freq_tbl *f)
-{
-	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
-	int ret;
-
-	ret = clk_rcg2_set_force_enable(hw);
-	if (ret)
-		return ret;
-
-	ret = clk_rcg2_configure(rcg, f);
-	if (ret)
-		return ret;
-
-	return clk_rcg2_clear_force_enable(hw);
-}
-
-static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate,
+static int __clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate,
 				    unsigned long parent_rate)
 {
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	const struct freq_tbl *f;
+	int ret;

 	f = qcom_find_freq(rcg->freq_tbl, rate);
 	if (!f)
@@ -857,7 +841,23 @@ static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (!__clk_is_enabled(hw->clk))
 		return __clk_rcg2_configure(rcg, f);

-	return clk_rcg2_shared_force_enable_clear(hw, f);
+	ret = clk_rcg2_set_force_enable(hw);
+	if (ret)
+		return ret;
+
+	return clk_rcg2_configure(rcg, f);
+}
+
+static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long parent_rate)
+{
+	int ret;
+
+	ret = __clk_rcg2_shared_set_rate(hw, rate, parent_rate);
+	if (ret)
+		return ret;
+
+	return clk_rcg2_clear_force_enable(hw);
 }

 static int clk_rcg2_shared_set_rate_and_parent(struct clk_hw *hw,
@@ -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,32 @@ 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_set_rate(struct clk_hw *hw, unsigned long rate,
+				    unsigned long parent_rate)
+{
+	return __clk_rcg2_shared_set_rate(hw, rate, parent_rate);
+}
+
+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_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 2/4] clk: qcom: Add clk_rcg2_gfx3d_ops for SDM845

[Stephen Boyd]

Quoting Amit Nischal (2018-06-06 04:41:46)
> 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() and set_rate() 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.
> 

It sounds like the GDSC enable is ANDed with the RCG's root enable
bit? Does the RCG need to be clocking for the GDSC to actually turn on?
Or is it purely that the enable bit is logically combined that way so
that if the RCG is parented to a PLL that's off the GDSC will still turn
on?

> 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.

Anyway, this patch will probably significantly change given that the RCG
is a glorified div-2 that muxes between a safe CXO speed and a
configurable PLL frequency. A lot of the logic can probably just be
hardcoded then.

> 
> Signed-off-by: Amit Nischal <anischal@codeaurora.org>

Patch looks sane.

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

[Amit Nischal]

On 2018-07-09 11:45, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-06-06 04:41:46)
>> 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() and set_rate() 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.
>> 
> 
> It sounds like the GDSC enable is ANDed with the RCG's root enable
> bit?

Here, the root clock (GFX3D RCG) needs to be enabled first before
writing to SW_COLLAPSE bit of the GDSC. RCG's CLK_ON signal would
power up the GDSC.

> Does the RCG need to be clocking for the GDSC to actually turn on?
> Or is it purely that the enable bit is logically combined that way so
> that if the RCG is parented to a PLL that's off the GDSC will still 
> turn
> on?
> 

Yes, the RCG needs to be clocking for the GPU_GX GDSC to actually turn 
on.

>> 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.
> 
> Anyway, this patch will probably significantly change given that the 
> RCG
> is a glorified div-2 that muxes between a safe CXO speed and a
> configurable PLL frequency. A lot of the logic can probably just be
> hardcoded then.
> 

Thanks for the suggestion.
We are planning to introduce the "clk_rcg2_gfx3d_determine_rate" op 
which will
always return the best parent as ‘GPU_CC_PLL0_OUT_EVEN’ and best_parent
rate equal to twice of the requested rate. With this approach frequency 
table
for rcg2 structure would not be required as all the supported 
frequencies would
be derived from the GPU_CC_PLL0_OUT_EVEN src with a divider of 2.

Also, we will add support to check the requested rate if falls within 
allowed
set_rate range. This will make sure that the source PLL would not go out 
of
the supported range. set_rate_range would be set by the GPUCC driver 
with min/max
value by calling below API.

clk_hw_set_rate_range(&gpu_cc_gx_gfx3d_clk_src.clkr.hw, 180000000, 
710000000)

>> 
>> Signed-off-by: Amit Nischal <anischal@codeaurora.org>
> 
> Patch looks sane.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Boyd]

Quoting Amit Nischal (2018-07-12 05:30:21)
> On 2018-07-09 11:45, Stephen Boyd wrote:
> > Quoting Amit Nischal (2018-06-06 04:41:46)
> >> 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() and set_rate() 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.
> >> 
> > 
> > It sounds like the GDSC enable is ANDed with the RCG's root enable
> > bit?
> 
> Here, the root clock (GFX3D RCG) needs to be enabled first before
> writing to SW_COLLAPSE bit of the GDSC. RCG's CLK_ON signal would
> power up the GDSC.
> 
> > Does the RCG need to be clocking for the GDSC to actually turn on?
> > Or is it purely that the enable bit is logically combined that way so
> > that if the RCG is parented to a PLL that's off the GDSC will still 
> > turn
> > on?
> > 
> 
> Yes, the RCG needs to be clocking for the GPU_GX GDSC to actually turn 
> on.

Cool, please add these details to the commit text.

> 
> >> 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.
> > 
> > Anyway, this patch will probably significantly change given that the 
> > RCG
> > is a glorified div-2 that muxes between a safe CXO speed and a
> > configurable PLL frequency. A lot of the logic can probably just be
> > hardcoded then.
> > 
> 
> Thanks for the suggestion.
> We are planning to introduce the "clk_rcg2_gfx3d_determine_rate" op 
> which will
> always return the best parent as ‘GPU_CC_PLL0_OUT_EVEN’ and best_parent
> rate equal to twice of the requested rate. With this approach frequency 
> table
> for rcg2 structure would not be required as all the supported 
> frequencies would
> be derived from the GPU_CC_PLL0_OUT_EVEN src with a divider of 2.
> 
> Also, we will add support to check the requested rate if falls within 
> allowed
> set_rate range. This will make sure that the source PLL would not go out 
> of
> the supported range. set_rate_range would be set by the GPUCC driver 
> with min/max
> value by calling below API.
> 
> clk_hw_set_rate_range(&gpu_cc_gx_gfx3d_clk_src.clkr.hw, 180000000, 
> 710000000)

Ok. Sounds good! Is the rate range call really needed? It can't be
determined in the PLL code with some table or avoided by making sure GPU
uses OPP table with only approved frequencies?

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

[Amit Nischal]

On 2018-07-25 12:28, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-07-12 05:30:21)
>> On 2018-07-09 11:45, Stephen Boyd wrote:
>> > Quoting Amit Nischal (2018-06-06 04:41:46)
>> >> 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() and set_rate() 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.
>> >>
>> >
>> > It sounds like the GDSC enable is ANDed with the RCG's root enable
>> > bit?
>> 
>> Here, the root clock (GFX3D RCG) needs to be enabled first before
>> writing to SW_COLLAPSE bit of the GDSC. RCG's CLK_ON signal would
>> power up the GDSC.
>> 
>> > Does the RCG need to be clocking for the GDSC to actually turn on?
>> > Or is it purely that the enable bit is logically combined that way so
>> > that if the RCG is parented to a PLL that's off the GDSC will still
>> > turn
>> > on?
>> >
>> 
>> Yes, the RCG needs to be clocking for the GPU_GX GDSC to actually turn
>> on.
> 
> Cool, please add these details to the commit text.

Thanks. I will add these details in the commit text in the next patch 
series.

> 
>> 
>> >> 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.
>> >
>> > Anyway, this patch will probably significantly change given that the
>> > RCG
>> > is a glorified div-2 that muxes between a safe CXO speed and a
>> > configurable PLL frequency. A lot of the logic can probably just be
>> > hardcoded then.
>> >
>> 
>> Thanks for the suggestion.
>> We are planning to introduce the "clk_rcg2_gfx3d_determine_rate" op
>> which will
>> always return the best parent as ‘GPU_CC_PLL0_OUT_EVEN’ and 
>> best_parent
>> rate equal to twice of the requested rate. With this approach 
>> frequency
>> table
>> for rcg2 structure would not be required as all the supported
>> frequencies would
>> be derived from the GPU_CC_PLL0_OUT_EVEN src with a divider of 2.
>> 
>> Also, we will add support to check the requested rate if falls within
>> allowed
>> set_rate range. This will make sure that the source PLL would not go 
>> out
>> of
>> the supported range. set_rate_range would be set by the GPUCC driver
>> with min/max
>> value by calling below API.
>> 
>> clk_hw_set_rate_range(&gpu_cc_gx_gfx3d_clk_src.clkr.hw, 180000000,
>> 710000000)
> 
> Ok. Sounds good! Is the rate range call really needed? It can't be
> determined in the PLL code with some table or avoided by making sure 
> GPU
> uses OPP table with only approved frequencies?
> 

Currently fabia PLL code does not have any table to check this and 
intention
was to avoid relying on the client to call set_rate with only approved
frequencies so we have added the set_rate_range() call in the GPUCC 
driver
in order to set the rate range.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Boyd]

Quoting Amit Nischal (2018-07-30 04:28:56)
> On 2018-07-25 12:28, Stephen Boyd wrote:
> > 
> > Ok. Sounds good! Is the rate range call really needed? It can't be
> > determined in the PLL code with some table or avoided by making sure 
> > GPU
> > uses OPP table with only approved frequencies?
> > 
> 
> Currently fabia PLL code does not have any table to check this and 
> intention
> was to avoid relying on the client to call set_rate with only approved
> frequencies so we have added the set_rate_range() call in the GPUCC 
> driver
> in order to set the rate range.
> 

But GPU will use OPP so it doesn't seem like it really buys us anything
here. And it really doesn't matter when the clk driver implementation
doesn't use the min/max to clamp the values of the round_rate() call. Is
that being done here? I need to double check. I would be more convinced
if the implementation was looking at min/max to constrain the rate
requested.

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

[Amit Nischal]

On 2018-08-03 04:14, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-07-30 04:28:56)
>> On 2018-07-25 12:28, Stephen Boyd wrote:
>> >
>> > Ok. Sounds good! Is the rate range call really needed? It can't be
>> > determined in the PLL code with some table or avoided by making sure
>> > GPU
>> > uses OPP table with only approved frequencies?
>> >
>> 
>> Currently fabia PLL code does not have any table to check this and
>> intention
>> was to avoid relying on the client to call set_rate with only approved
>> frequencies so we have added the set_rate_range() call in the GPUCC
>> driver
>> in order to set the rate range.
>> 
> 
> But GPU will use OPP so it doesn't seem like it really buys us anything
> here. And it really doesn't matter when the clk driver implementation
> doesn't use the min/max to clamp the values of the round_rate() call. 
> Is
> that being done here? I need to double check. I would be more convinced
> if the implementation was looking at min/max to constrain the rate
> requested.
> 

So our understanding is that GPU(client) driver will always call the
set_rate with approved frequencies only and we can completely rely on 
the
client. Is our understanding is correct?

> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jordan Crouse]

On Mon, Aug 06, 2018 at 02:37:18PM +0530, Amit Nischal wrote:
> On 2018-08-03 04:14, Stephen Boyd wrote:
> >Quoting Amit Nischal (2018-07-30 04:28:56)
> >>On 2018-07-25 12:28, Stephen Boyd wrote:
> >>>
> >>> Ok. Sounds good! Is the rate range call really needed? It can't be
> >>> determined in the PLL code with some table or avoided by making sure
> >>> GPU
> >>> uses OPP table with only approved frequencies?
> >>>
> >>
> >>Currently fabia PLL code does not have any table to check this and
> >>intention
> >>was to avoid relying on the client to call set_rate with only approved
> >>frequencies so we have added the set_rate_range() call in the GPUCC
> >>driver
> >>in order to set the rate range.
> >>
> >
> >But GPU will use OPP so it doesn't seem like it really buys us anything
> >here. And it really doesn't matter when the clk driver implementation
> >doesn't use the min/max to clamp the values of the round_rate()
> >call. Is
> >that being done here? I need to double check. I would be more convinced
> >if the implementation was looking at min/max to constrain the rate
> >requested.
> >
> 
> So our understanding is that GPU(client) driver will always call the
> set_rate with approved frequencies only and we can completely rely
> on the
> client. Is our understanding is correct?


First: on sdm845 the software doesn't set the GPU clocks - we rely on the GMU
firmware to do that on our behalf but for the GPU at least this is an academic
exercise.

But that said: traditionally we've expected that the clock driver correctly
clamp the requested rate to the correct values. In the past we have taken
advantage of this and we may in the future. I don't think it is reasonable
to require the leaf driver to only pass "approved" frequencies especially
since we depend on our own OPP table that may or may not be similar to the
one used by the clock driver.

Jordan

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

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

[Stephen Boyd]

Quoting Jordan Crouse (2018-08-06 08:04:37)
> On Mon, Aug 06, 2018 at 02:37:18PM +0530, Amit Nischal wrote:
> > On 2018-08-03 04:14, Stephen Boyd wrote:
> > >Quoting Amit Nischal (2018-07-30 04:28:56)
> > >>On 2018-07-25 12:28, Stephen Boyd wrote:
> > >>>
> > >>> Ok. Sounds good! Is the rate range call really needed? It can't be
> > >>> determined in the PLL code with some table or avoided by making sure
> > >>> GPU
> > >>> uses OPP table with only approved frequencies?
> > >>>
> > >>
> > >>Currently fabia PLL code does not have any table to check this and
> > >>intention
> > >>was to avoid relying on the client to call set_rate with only approved
> > >>frequencies so we have added the set_rate_range() call in the GPUCC
> > >>driver
> > >>in order to set the rate range.
> > >>
> > >
> > >But GPU will use OPP so it doesn't seem like it really buys us anything
> > >here. And it really doesn't matter when the clk driver implementation
> > >doesn't use the min/max to clamp the values of the round_rate()
> > >call. Is
> > >that being done here? I need to double check. I would be more convinced
> > >if the implementation was looking at min/max to constrain the rate
> > >requested.
> > >
> > 
> > So our understanding is that GPU(client) driver will always call the
> > set_rate with approved frequencies only and we can completely rely
> > on the
> > client. Is our understanding is correct?
> 
> 
> First: on sdm845 the software doesn't set the GPU clocks - we rely on the GMU
> firmware to do that on our behalf but for the GPU at least this is an academic
> exercise.

So what is this GPU clk driver for then?

> 
> But that said: traditionally we've expected that the clock driver correctly
> clamp the requested rate to the correct values. In the past we have taken
> advantage of this and we may in the future. I don't think it is reasonable
> to require the leaf driver to only pass "approved" frequencies especially
> since we depend on our own OPP table that may or may not be similar to the
> one used by the clock driver.
> 

Ok. Sounds like things can't be kept in sync between the clk driver and
the OPP tables. Why is that hard to do?

Either way, I'd be fine if the code actually used the frequency limits
to round the rate to something within range, but I don't recall seeing
that being done here. So if the min/max limits stay, the clk driver
should round to within that range.

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

[Jordan Crouse]

On Tue, Aug 07, 2018 at 10:58:19PM -0700, Stephen Boyd wrote:
> Quoting Jordan Crouse (2018-08-06 08:04:37)
> > On Mon, Aug 06, 2018 at 02:37:18PM +0530, Amit Nischal wrote:
> > > On 2018-08-03 04:14, Stephen Boyd wrote:
> > > >Quoting Amit Nischal (2018-07-30 04:28:56)
> > > >>On 2018-07-25 12:28, Stephen Boyd wrote:
> > > >>>
> > > >>> Ok. Sounds good! Is the rate range call really needed? It can't be
> > > >>> determined in the PLL code with some table or avoided by making sure
> > > >>> GPU
> > > >>> uses OPP table with only approved frequencies?
> > > >>>
> > > >>
> > > >>Currently fabia PLL code does not have any table to check this and
> > > >>intention
> > > >>was to avoid relying on the client to call set_rate with only approved
> > > >>frequencies so we have added the set_rate_range() call in the GPUCC
> > > >>driver
> > > >>in order to set the rate range.
> > > >>
> > > >
> > > >But GPU will use OPP so it doesn't seem like it really buys us anything
> > > >here. And it really doesn't matter when the clk driver implementation
> > > >doesn't use the min/max to clamp the values of the round_rate()
> > > >call. Is
> > > >that being done here? I need to double check. I would be more convinced
> > > >if the implementation was looking at min/max to constrain the rate
> > > >requested.
> > > >
> > > 
> > > So our understanding is that GPU(client) driver will always call the
> > > set_rate with approved frequencies only and we can completely rely
> > > on the
> > > client. Is our understanding is correct?
> > 
> > 
> > First: on sdm845 the software doesn't set the GPU clocks - we rely on the GMU
> > firmware to do that on our behalf but for the GPU at least this is an academic
> > exercise.
> 
> So what is this GPU clk driver for then?

That is a good question. There is a hodgepodge of clocks for the GMU, GPU and
SMMU and I'm not sure which ones are provided by the various clk drivers. This
isn't my area of expertise.

The GMU uses:

GPU_CC_CX_GMU_CLK
GPU_CC_CXO_CLK
GCC_DDRSS_GPU_AXI_CLK
GCC_GPU_MEMNOC_GFX_CLK

These are controlled by the GMU device.

The SMMU uses:

GCC_GPU_CFG_AHB_CLK
GCC_DDRSS_GPU_AXI_CLK
GCC_GPU_MEMNOC_GFX_CLK

These should be controlled by the SMMU device.

Downstream defines these drivers for the GPU but we don't get/prepare/use
them for the GPU device - I think they are there in case the GMU isn't
working or is disabled for some other reason.

GPU_CC_GX_GFX3D_CLK
GPU_CC_CXO_CLK
GCC_DDRSS_GPU_AXI_CLK
GCC_GPU_MEMNOC_GFX_CLK
GPU_CC_CX_GMU_CLK

> > 
> > But that said: traditionally we've expected that the clock driver correctly
> > clamp the requested rate to the correct values. In the past we have taken
> > advantage of this and we may in the future. I don't think it is reasonable
> > to require the leaf driver to only pass "approved" frequencies especially
> > since we depend on our own OPP table that may or may not be similar to the
> > one used by the clock driver.
> > 
> 
> Ok. Sounds like things can't be kept in sync between the clk driver and
> the OPP tables. Why is that hard to do?

Again, not my area of expertise. Traditionally the leaf driver is responsible
for setting its own OPP table. I'm not sure if the clock driver can or should
be in the role of switching up the table. We've always assumed that the clk
driver will sanity check whatever we ask it for.

Jordan

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

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

[Amit Nischal]

On 2018-08-08 11:28, Stephen Boyd wrote:
> Quoting Jordan Crouse (2018-08-06 08:04:37)
>> On Mon, Aug 06, 2018 at 02:37:18PM +0530, Amit Nischal wrote:
>> > On 2018-08-03 04:14, Stephen Boyd wrote:
>> > >Quoting Amit Nischal (2018-07-30 04:28:56)
>> > >>On 2018-07-25 12:28, Stephen Boyd wrote:
>> > >>>
>> > >>> Ok. Sounds good! Is the rate range call really needed? It can't be
>> > >>> determined in the PLL code with some table or avoided by making sure
>> > >>> GPU
>> > >>> uses OPP table with only approved frequencies?
>> > >>>
>> > >>
>> > >>Currently fabia PLL code does not have any table to check this and
>> > >>intention
>> > >>was to avoid relying on the client to call set_rate with only approved
>> > >>frequencies so we have added the set_rate_range() call in the GPUCC
>> > >>driver
>> > >>in order to set the rate range.
>> > >>
>> > >
>> > >But GPU will use OPP so it doesn't seem like it really buys us anything
>> > >here. And it really doesn't matter when the clk driver implementation
>> > >doesn't use the min/max to clamp the values of the round_rate()
>> > >call. Is
>> > >that being done here? I need to double check. I would be more convinced
>> > >if the implementation was looking at min/max to constrain the rate
>> > >requested.
>> > >
>> >
>> > So our understanding is that GPU(client) driver will always call the
>> > set_rate with approved frequencies only and we can completely rely
>> > on the
>> > client. Is our understanding is correct?
>> 
>> 
>> First: on sdm845 the software doesn't set the GPU clocks - we rely on 
>> the GMU
>> firmware to do that on our behalf but for the GPU at least this is an 
>> academic
>> exercise.
> 
> So what is this GPU clk driver for then?
> 
>> 
>> But that said: traditionally we've expected that the clock driver 
>> correctly
>> clamp the requested rate to the correct values. In the past we have 
>> taken
>> advantage of this and we may in the future. I don't think it is 
>> reasonable
>> to require the leaf driver to only pass "approved" frequencies 
>> especially
>> since we depend on our own OPP table that may or may not be similar to 
>> the
>> one used by the clock driver.
>> 
> 
> Ok. Sounds like things can't be kept in sync between the clk driver and
> the OPP tables. Why is that hard to do?
> 
> Either way, I'd be fine if the code actually used the frequency limits
> to round the rate to something within range, but I don't recall seeing
> that being done here. So if the min/max limits stay, the clk driver
> should round to within that range.
> 

Thanks Stephen for your suggestion. I have modified the existing
determine_rate() op to use the min/max limits and round the requested
rate so that it stays withing the set_rate range. I will submit the
same in the next patch series.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 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 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..e311219
--- /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..b9cbce5
--- /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				12
+
+/* 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

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

[Stephen Boyd]

Quoting Amit Nischal (2018-06-06 04:41:47)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.txt b/Documentation/devicetree/bindings/clock/qcom,gpucc.txt
> new file mode 100644
> index 0000000..e311219
> --- /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 {

Drop the double '@'

> +               compatible = "qcom,sdm845-gpucc";
> +               reg = <0x5090000 0x9000>;
> +               #clock-cells = <1>;
> +               #reset-cells = <1>;
> +               #power-domain-cells = <1>;
> +       };

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

[Amit Nischal]

On 2018-07-09 11:08, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-06-06 04:41:47)
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.txt 
>> b/Documentation/devicetree/bindings/clock/qcom,gpucc.txt
>> new file mode 100644
>> index 0000000..e311219
>> --- /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 {
> 
> Drop the double '@'

Oh. I will fix this in next patch series.

> 
>> +               compatible = "qcom,sdm845-gpucc";
>> +               reg = <0x5090000 0x9000>;
>> +               #clock-cells = <1>;
>> +               #reset-cells = <1>;
>> +               #power-domain-cells = <1>;
>> +       };
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 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 | 441 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 451 insertions(+)
 create mode 100644 drivers/clk/qcom/gpucc-sdm845.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 9c3480d..193c2f5 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -235,6 +235,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 32d17c2..8aa2bc9 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -40,5 +40,6 @@ obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
 obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.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..81f8926
--- /dev/null
+++ b/drivers/clk/qcom/gpucc-sdm845.c
@@ -0,0 +1,441 @@
+// 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)
+
+#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
+
+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 const struct freq_tbl ftbl_gpu_cc_gx_gfx3d_clk_src[] = {
+	F(180000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
+	F(257000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
+	F(342000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
+	F(414000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
+	F(520000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
+	F(596000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
+	F(675000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
+	F(710000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
+	{ }
+};
+
+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,
+	.freq_tbl = ftbl_gpu_cc_gx_gfx3d_clk_src,
+	.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;
+
+	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);
+
+	/* Configure clk_dis_wait for for gpu_cx_gdsc */
+	regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
+						(8 << CLK_DIS_WAIT_SHIFT));
+
+	return qcom_cc_really_probe(pdev, &gpu_cc_sdm845_desc, regmap);
+}
+
+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_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 4/4] clk: qcom: Add graphics clock controller driver for SDM845

[Stephen Boyd]

Quoting Amit Nischal (2018-06-06 04:41:48)
> diff --git a/drivers/clk/qcom/gpucc-sdm845.c b/drivers/clk/qcom/gpucc-sdm845.c
> new file mode 100644
> index 0000000..81f8926
> --- /dev/null
> +++ b/drivers/clk/qcom/gpucc-sdm845.c
> @@ -0,0 +1,441 @@
> +// 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)
> +
> +#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }

This should be removed and series can depend on Taniya's patch.

> +
> +enum {
> +       P_BI_TCXO,
> +       P_CORE_BI_PLL_TEST_SE,
> +       P_GPLL0_OUT_MAIN,
> +       P_GPLL0_OUT_MAIN_DIV,
[...]
> +       .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 },
> +       {},

Drop the trailing comma, it's a sentinel presumably.

> +};
> +
> +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 const struct freq_tbl ftbl_gpu_cc_gx_gfx3d_clk_src[] = {
> +       F(180000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(257000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(342000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(414000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(520000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(596000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(675000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
> +       F(710000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),

Do we really need to encode anything here in this table? Why can't we
have clk_ops that hardcode this clk to be a div-2 that passes the
frequency up to the parent source? Then this frequency table doesn't
need to be here at all, and can live in DT as an OPP table used by the
GPU driver.

> +       { }
> +};
> +
> +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,
> +       .freq_tbl = ftbl_gpu_cc_gx_gfx3d_clk_src,
> +       .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_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 = {

This isn't always on and marked critical?

> +       .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 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,

Hmm alright. So basically the core GPU power domain needs the wrapper
domain (CX) to be powered on and clocking for GX to work? Let's talk
about it in the other patch so I can understand more.

> +       },
> +       .clk_count = 1,
> +       .pwrsts = PWRSTS_OFF_ON,
> +       .flags = CLAMP_IO | AON_RESET | POLL_CFG_GDSCR,
> +};
> +
[...]
> +
> +static int gpu_cc_sdm845_probe(struct platform_device *pdev)
> +{
> +       struct regmap *regmap;
> +       unsigned int value, mask;
> +
> +       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);
> +
> +       /* Configure clk_dis_wait for for gpu_cx_gdsc */

s/for //

> +       regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
> +                                               (8 << CLK_DIS_WAIT_SHIFT));

Remove extra parenthesis.

> +
> +       return qcom_cc_really_probe(pdev, &gpu_cc_sdm845_desc, regmap);
> +}
> +
[...]
> +
> +MODULE_LICENSE("GPL v2");

MODULE_DESCRIPTION?

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

[Amit Nischal]

On 2018-07-09 11:37, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-06-06 04:41:48)
>> diff --git a/drivers/clk/qcom/gpucc-sdm845.c 
>> b/drivers/clk/qcom/gpucc-sdm845.c
>> new file mode 100644
>> index 0000000..81f8926
>> --- /dev/null
>> +++ b/drivers/clk/qcom/gpucc-sdm845.c
>> @@ -0,0 +1,441 @@
>> +// 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)
>> +
>> +#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
> 
> This should be removed and series can depend on Taniya's patch.
> 

Yes sure. I will do the required change and will submit the next
patch series.

>> +
>> +enum {
>> +       P_BI_TCXO,
>> +       P_CORE_BI_PLL_TEST_SE,
>> +       P_GPLL0_OUT_MAIN,
>> +       P_GPLL0_OUT_MAIN_DIV,
> [...]
>> +       .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 },
>> +       {},
> 
> Drop the trailing comma, it's a sentinel presumably.
> 

Will fix this in the next patch series.

>> +};
>> +
>> +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 const struct freq_tbl ftbl_gpu_cc_gx_gfx3d_clk_src[] = {
>> +       F(180000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
>> +       F(257000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
>> +       F(342000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
>> +       F(414000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
>> +       F(520000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
>> +       F(596000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
>> +       F(675000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
>> +       F(710000000, P_GPU_CC_PLL0_OUT_EVEN, 2, 0, 0),
> 
> Do we really need to encode anything here in this table? Why can't we
> have clk_ops that hardcode this clk to be a div-2 that passes the
> frequency up to the parent source? Then this frequency table doesn't
> need to be here at all, and can live in DT as an OPP table used by the
> GPU driver.
> 

Thanks for the suggestion.
I will address the same in the next patch series where
"clk_rcg2_gfx3d_determine_rate" op will always return the best parent as
‘GPU_CC_PLL0_OUT_EVEN’ and best_parent rate equal to twice of the 
requested rate.
This will eliminate the need of frequency table.

>> +       { }
>> +};
>> +
>> +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,
>> +       .freq_tbl = ftbl_gpu_cc_gx_gfx3d_clk_src,
>> +       .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_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 = {
> 
> This isn't always on and marked critical?

No, this clock is not marked critical as GCC driver doesn't
control this clock.

> 
>> +       .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 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,
> 
> Hmm alright. So basically the core GPU power domain needs the wrapper
> domain (CX) to be powered on and clocking for GX to work? Let's talk
> about it in the other patch so I can understand more.
> 

Yes, to turn on the GPU GX GDSC it requires below to be in ON state 
first.
1. GPU_CX_GDSC
2. ROOT clock GFX3D clock source.

>> +       },
>> +       .clk_count = 1,
>> +       .pwrsts = PWRSTS_OFF_ON,
>> +       .flags = CLAMP_IO | AON_RESET | POLL_CFG_GDSCR,
>> +};
>> +
> [...]
>> +
>> +static int gpu_cc_sdm845_probe(struct platform_device *pdev)
>> +{
>> +       struct regmap *regmap;
>> +       unsigned int value, mask;
>> +
>> +       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);
>> +
>> +       /* Configure clk_dis_wait for for gpu_cx_gdsc */
> 
> s/for //
> 

I will address this in the next patch series.

>> +       regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK,
>> +                                               (8 << 
>> CLK_DIS_WAIT_SHIFT));
> 
> Remove extra parenthesis.

Yes sure, I will fix this in the next patch series.
> 
>> +
>> +       return qcom_cc_really_probe(pdev, &gpu_cc_sdm845_desc, 
>> regmap);
>> +}
>> +
> [...]
>> +
>> +MODULE_LICENSE("GPL v2");
> 
> MODULE_DESCRIPTION?

I will add the module description in the next patch series.