[PATCH v1 0/7] Add GPU & Video Clock controller driver for SC7180

[PATCH v1 0/7] Add GPU & Video Clock controller driver for SC7180

[Taniya Das]

[v1]
 * Fabia PLLs could fail latching in the case where the PLL is not
   calibrated, so add support to calibrate in prepare clock ops.

 * Add driver support for Graphics clock controller for SC7180 and also
   update device tree bindings for the various clocks supported in the
   clock controller.

 * Add driver support for Video clock controller for SC7180 and also
   update device tree bindings for the various clocks supported in the
   clock controller.

This change depends on below GCC clock driver series
https://patchwork.kernel.org/project/linux-clk/list/?series=187089

Taniya Das (7):
  clk: qcom: clk-alpha-pll: Add support for Fabia PLL calibration
  dt-bindings: clock: Add YAML schemas for the QCOM GPUCC clock bindings
  dt-bindings: clock: Introduce QCOM Graphics clock bindings
  clk: qcom: Add graphics clock controller driver for SC7180
  dt-bindings: clock: Add YAML schemas for the QCOM VIDEOCC clock
    bindings
  dt-bindings: clock: Introduce QCOM Video clock bindings
  clk: qcom: Add video clock controller driver for SC7180

 .../devicetree/bindings/clock/qcom,gpucc.txt       |  24 --
 .../devicetree/bindings/clock/qcom,gpucc.yaml      |  70 ++++++
 .../devicetree/bindings/clock/qcom,videocc.txt     |  18 --
 .../devicetree/bindings/clock/qcom,videocc.yaml    |  62 +++++
 drivers/clk/qcom/Kconfig                           |  16 ++
 drivers/clk/qcom/Makefile                          |   2 +
 drivers/clk/qcom/clk-alpha-pll.c                   |  84 ++++++-
 drivers/clk/qcom/clk-alpha-pll.h                   |   4 +
 drivers/clk/qcom/gpucc-sc7180.c                    | 274 +++++++++++++++++++++
 drivers/clk/qcom/videocc-sc7180.c                  | 263 ++++++++++++++++++++
 include/dt-bindings/clock/qcom,gpucc-sc7180.h      |  21 ++
 include/dt-bindings/clock/qcom,videocc-sc7180.h    |  23 ++
 12 files changed, 814 insertions(+), 47 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
 delete mode 100644 Documentation/devicetree/bindings/clock/qcom,videocc.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,videocc.yaml
 create mode 100644 drivers/clk/qcom/gpucc-sc7180.c
 create mode 100644 drivers/clk/qcom/videocc-sc7180.c
 create mode 100644 include/dt-bindings/clock/qcom,gpucc-sc7180.h
 create mode 100644 include/dt-bindings/clock/qcom,videocc-sc7180.h

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

[PATCH v1 1/7] clk: qcom: clk-alpha-pll: Add support for Fabia PLL calibration

[Taniya Das]

In the cases where the PLL is not calibrated the PLL could fail to lock.
Add support for prepare ops which would take care of the same.

Fabia PLL user/test control registers might required to be configured, so
add support for configuring them.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/clk-alpha-pll.c | 84 +++++++++++++++++++++++++++++++++++++---
 drivers/clk/qcom/clk-alpha-pll.h |  4 ++
 2 files changed, 83 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 055318f..8cb77ca 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -1024,6 +1024,25 @@ void clk_fabia_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
 		regmap_write(regmap, PLL_CONFIG_CTL(pll),
 						config->config_ctl_val);

+	if (config->config_ctl_hi_val)
+		regmap_write(regmap, PLL_CONFIG_CTL_U(pll),
+						config->config_ctl_hi_val);
+
+	if (config->user_ctl_val)
+		regmap_write(regmap, PLL_USER_CTL(pll), config->user_ctl_val);
+
+	if (config->user_ctl_hi_val)
+		regmap_write(regmap, PLL_USER_CTL_U(pll),
+						config->user_ctl_hi_val);
+
+	if (config->test_ctl_val)
+		regmap_write(regmap, PLL_TEST_CTL(pll),
+						config->test_ctl_val);
+
+	if (config->test_ctl_hi_val)
+		regmap_write(regmap, PLL_TEST_CTL_U(pll),
+						config->test_ctl_hi_val);
+
 	if (config->post_div_mask) {
 		mask = config->post_div_mask;
 		val = config->post_div_val;
@@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
 						unsigned long prate)
 {
 	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
-	u32 val, l, alpha_width = pll_alpha_width(pll);
+	u32 l, alpha_width = pll_alpha_width(pll);
 	u64 a;
 	unsigned long rrate;
 	int ret = 0;

-	ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
-	if (ret)
-		return ret;
-
 	rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width);

 	/*
@@ -1167,7 +1182,66 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
 	return __clk_alpha_pll_update_latch(pll);
 }

+static int alpha_pll_fabia_prepare(struct clk_hw *hw)
+{
+	struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
+	const struct pll_vco *vco;
+	struct clk_hw *parent_hw;
+	unsigned long cal_freq, rrate;
+	u32 cal_l, regval, alpha_width = pll_alpha_width(pll);
+	u64 a;
+	int ret;
+
+	/* Check if calibration needs to be done i.e. PLL is in reset */
+	ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &regval);
+	if (ret)
+		return ret;
+
+	/* Return early if calibration is not needed. */
+	if (regval & PLL_RESET_N)
+		return 0;
+
+	vco = alpha_pll_find_vco(pll, clk_hw_get_rate(hw));
+	if (!vco) {
+		pr_err("alpha pll: not in a valid vco range\n");
+		return -EINVAL;
+	}
+
+	cal_freq = DIV_ROUND_CLOSEST((pll->vco_table[0].min_freq +
+				pll->vco_table[0].max_freq) * 54, 100);
+
+	parent_hw = clk_hw_get_parent(hw);
+	if (!parent_hw)
+		return -EINVAL;
+
+	rrate = alpha_pll_round_rate(cal_freq, clk_hw_get_rate(parent_hw),
+					&cal_l, &a, alpha_width);
+	/*
+	 * Due to a limited number of bits for fractional rate programming, the
+	 * rounded up rate could be marginally higher than the requested rate.
+	 */
+	if (rrate > (cal_freq + FABIA_PLL_RATE_MARGIN) || rrate < cal_freq) {
+		pr_err("Call set rate on the PLL with rounded rates!\n");
+		return -EINVAL;
+	}
+
+	/* Setup PLL for calibration frequency */
+	regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), cal_l);
+
+	/* Bringup the pll at calibration frequency */
+	ret = clk_alpha_pll_enable(hw);
+	if (ret) {
+		pr_err("alpha pll calibration failed\n");
+		return ret;
+	}
+
+	clk_alpha_pll_disable(hw);
+
+	return 0;
+}
+
 const struct clk_ops clk_alpha_pll_fabia_ops = {
+	.prepare = alpha_pll_fabia_prepare,
 	.enable = alpha_pll_fabia_enable,
 	.disable = alpha_pll_fabia_disable,
 	.is_enabled = clk_alpha_pll_is_enabled,
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index 15f27f4..b03eea0 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -94,6 +94,10 @@ struct alpha_pll_config {
 	u32 alpha_hi;
 	u32 config_ctl_val;
 	u32 config_ctl_hi_val;
+	u32 user_ctl_val;
+	u32 user_ctl_hi_val;
+	u32 test_ctl_val;
+	u32 test_ctl_hi_val;
 	u32 main_output_mask;
 	u32 aux_output_mask;
 	u32 aux2_output_mask;
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v1 1/7] clk: qcom: clk-alpha-pll: Add support for Fabia PLL calibration

[Stephen Boyd]

Quoting Taniya Das (2019-10-31 05:21:07)
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 055318f..8cb77ca 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>                                                 unsigned long prate)
>  {
>         struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> -       u32 val, l, alpha_width = pll_alpha_width(pll);
> +       u32 l, alpha_width = pll_alpha_width(pll);
>         u64 a;
>         unsigned long rrate;
>         int ret = 0;
> 
> -       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
> -       if (ret)
> -               return ret;
> -
>         rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width);
> 
>         /*

How is this diff related? Looks like it should be split off into another
patch to remove a useless register read.

> @@ -1167,7 +1182,66 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>         return __clk_alpha_pll_update_latch(pll);
>  }
> 
> +static int alpha_pll_fabia_prepare(struct clk_hw *hw)
> +{

Why are we doing this in prepare vs. doing it at PLL configuration time?
Does it need to be recalibrated each time the PLL is enabled?

> +       struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> +       const struct pll_vco *vco;
> +       struct clk_hw *parent_hw;
> +       unsigned long cal_freq, rrate;
> +       u32 cal_l, regval, alpha_width = pll_alpha_width(pll);
> +       u64 a;
> +       int ret;
> +
> +       /* Check if calibration needs to be done i.e. PLL is in reset */
> +       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &regval);

Please use 'val' instead of 'regval' as regval almost never appears in
this file already.

> +       if (ret)
> +               return ret;
> +
> +       /* Return early if calibration is not needed. */
> +       if (regval & PLL_RESET_N)
> +               return 0;
> +
> +       vco = alpha_pll_find_vco(pll, clk_hw_get_rate(hw));
> +       if (!vco) {
> +               pr_err("alpha pll: not in a valid vco range\n");
> +               return -EINVAL;
> +       }
> +
> +       cal_freq = DIV_ROUND_CLOSEST((pll->vco_table[0].min_freq +
> +                               pll->vco_table[0].max_freq) * 54, 100);

Do we need to cast the first argument to a u64 to avoid overflow?

> +
> +       parent_hw = clk_hw_get_parent(hw);
> +       if (!parent_hw)
> +               return -EINVAL;
> +
> +       rrate = alpha_pll_round_rate(cal_freq, clk_hw_get_rate(parent_hw),
> +                                       &cal_l, &a, alpha_width);
> +       /*
> +        * Due to a limited number of bits for fractional rate programming, the
> +        * rounded up rate could be marginally higher than the requested rate.
> +        */
> +       if (rrate > (cal_freq + FABIA_PLL_RATE_MARGIN) || rrate < cal_freq) {
> +               pr_err("Call set rate on the PLL with rounded rates!\n");

This message is weird. Drivers shouldn't need to call set rate with
rounded rates. What is going on?

> +               return -EINVAL;
> +       }
> +
> +       /* Setup PLL for calibration frequency */
> +       regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), cal_l);
> +
> +       /* Bringup the pll at calibration frequency */

capitalize PLL.

> +       ret = clk_alpha_pll_enable(hw);
> +       if (ret) {
> +               pr_err("alpha pll calibration failed\n");
> +               return ret;
> +       }
> +
> +       clk_alpha_pll_disable(hw);
> +
> +       return 0;
> +}
> +

Re: [PATCH v1 1/7] clk: qcom: clk-alpha-pll: Add support for Fabia PLL calibration

[Taniya Das]

Hi Stephen,

Thanks for your review.

On 11/6/2019 6:06 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2019-10-31 05:21:07)
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
>> index 055318f..8cb77ca 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>>                                                  unsigned long prate)
>>   {
>>          struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>> -       u32 val, l, alpha_width = pll_alpha_width(pll);
>> +       u32 l, alpha_width = pll_alpha_width(pll);
>>          u64 a;
>>          unsigned long rrate;
>>          int ret = 0;
>>
>> -       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
>> -       if (ret)
>> -               return ret;
>> -
>>          rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width);
>>
>>          /*
> 
> How is this diff related? Looks like it should be split off into another
> patch to remove a useless register read.
> 

I will split this in different patch.

>> @@ -1167,7 +1182,66 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>>          return __clk_alpha_pll_update_latch(pll);
>>   }
>>
>> +static int alpha_pll_fabia_prepare(struct clk_hw *hw)
>> +{
> 
> Why are we doing this in prepare vs. doing it at PLL configuration time?
> Does it need to be recalibrated each time the PLL is enabled?
> 

In the case if PLL looses the configuration then we would encounter PLL 
locking issues. Thus want to go ahead with prepare. In the case it is 
calibrated it would return.

>> +       struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
>> +       const struct pll_vco *vco;
>> +       struct clk_hw *parent_hw;
>> +       unsigned long cal_freq, rrate;
>> +       u32 cal_l, regval, alpha_width = pll_alpha_width(pll);
>> +       u64 a;
>> +       int ret;
>> +
>> +       /* Check if calibration needs to be done i.e. PLL is in reset */
>> +       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &regval);
> 
> Please use 'val' instead of 'regval' as regval almost never appears in
> this file already.
> 

Sure, will use 'val'.

>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Return early if calibration is not needed. */
>> +       if (regval & PLL_RESET_N)
>> +               return 0;
>> +
>> +       vco = alpha_pll_find_vco(pll, clk_hw_get_rate(hw));
>> +       if (!vco) {
>> +               pr_err("alpha pll: not in a valid vco range\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       cal_freq = DIV_ROUND_CLOSEST((pll->vco_table[0].min_freq +
>> +                               pll->vco_table[0].max_freq) * 54, 100);
> 
> Do we need to cast the first argument to a u64 to avoid overflow?
> 

No we do not need.

>> +
>> +       parent_hw = clk_hw_get_parent(hw);
>> +       if (!parent_hw)
>> +               return -EINVAL;
>> +
>> +       rrate = alpha_pll_round_rate(cal_freq, clk_hw_get_rate(parent_hw),
>> +                                       &cal_l, &a, alpha_width);
>> +       /*
>> +        * Due to a limited number of bits for fractional rate programming, the
>> +        * rounded up rate could be marginally higher than the requested rate.
>> +        */
>> +       if (rrate > (cal_freq + FABIA_PLL_RATE_MARGIN) || rrate < cal_freq) {
>> +               pr_err("Call set rate on the PLL with rounded rates!\n");
> 
> This message is weird. Drivers shouldn't need to call set rate with
> rounded rates. What is going on?
> 

:), my bad, copy paste from another function. I will remove this print.

>> +               return -EINVAL;
>> +       }
>> +
>> +       /* Setup PLL for calibration frequency */
>> +       regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), cal_l);
>> +
>> +       /* Bringup the pll at calibration frequency */
> 
> capitalize PLL.
> 

Will take care of it.

>> +       ret = clk_alpha_pll_enable(hw);
>> +       if (ret) {
>> +               pr_err("alpha pll calibration failed\n");
>> +               return ret;
>> +       }
>> +
>> +       clk_alpha_pll_disable(hw);
>> +
>> +       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 v1 1/7] clk: qcom: clk-alpha-pll: Add support for Fabia PLL calibration

[Doug Anderson]

Hi,

On Thu, Oct 31, 2019 at 5:21 AM Taniya Das <tdas@codeaurora.org> wrote:
>
> @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate,
>                                                 unsigned long prate)
>  {
>         struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> -       u32 val, l, alpha_width = pll_alpha_width(pll);
> +       u32 l, alpha_width = pll_alpha_width(pll);
>         u64 a;
>         unsigned long rrate;
>         int ret = 0;
>
> -       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
> -       if (ret)
> -               return ret;

My compiler happened to notice that with the change to this function:

drivers/clk/qcom/clk-alpha-pll.c:1166:6: error: unused variable 'ret'
[-Werror,-Wunused-variable]
        int ret = 0;

-Doug

Re: [PATCH v1 1/7] clk: qcom: clk-alpha-pll: Add support for Fabia PLL calibration

[Taniya Das]

Hi Doug,

On 11/7/2019 9:54 AM, Doug Anderson wrote:

>> -       ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val);
>> -       if (ret)
>> -               return ret;
> 
> My compiler happened to notice that with the change to this function:
> 
> drivers/clk/qcom/clk-alpha-pll.c:1166:6: error: unused variable 'ret'
> [-Werror,-Wunused-variable]
>          int ret = 0;
> 
> -Doug
> 

Thanks for the review, will remove the unused variable.

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

--

[PATCH v1 2/7] dt-bindings: clock: Add YAML schemas for the QCOM GPUCC clock bindings

[Taniya Das]

The GPUCC clock provider have a bunch of generic properties that
are needed in a device tree. Add a YAML schemas for those.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../devicetree/bindings/clock/qcom,gpucc.txt       | 24 --------
 .../devicetree/bindings/clock/qcom,gpucc.yaml      | 69 ++++++++++++++++++++++
 2 files changed, 69 insertions(+), 24 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.yaml

diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.txt b/Documentation/devicetree/bindings/clock/qcom,gpucc.txt
deleted file mode 100644
index 269afe8a..0000000
--- a/Documentation/devicetree/bindings/clock/qcom,gpucc.txt
+++ /dev/null
@@ -1,24 +0,0 @@
-Qualcomm Graphics Clock & Reset Controller Binding
---------------------------------------------------
-
-Required properties :
-- compatible : shall contain "qcom,sdm845-gpucc" or "qcom,msm8998-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
-- clocks : shall contain the XO clock
-	   shall contain the gpll0 out main clock (msm8998)
-- clock-names : shall be "xo"
-		shall be "gpll0" (msm8998)
-
-Example:
-	gpucc: clock-controller@5090000 {
-		compatible = "qcom,sdm845-gpucc";
-		reg = <0x5090000 0x9000>;
-		#clock-cells = <1>;
-		#reset-cells = <1>;
-		#power-domain-cells = <1>;
-		clocks = <&rpmhcc RPMH_CXO_CLK>;
-		clock-names = "xo";
-	};
diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
new file mode 100644
index 0000000..96aaf36
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bindings/clock/qcom,gpucc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Graphics Clock & Reset Controller Binding
+
+maintainers:
+  - Taniya Das <tdas@codeaurora.org>
+
+description: |
+  Qualcomm grpahics clock control module which supports the clocks, resets and
+  power domains.
+
+properties:
+  compatible:
+    enum:
+      - qcom,sdm845-gpucc
+      - qcom,msm8998-gpucc
+
+  clocks:
+    minItems: 1
+    maxItems: 2
+    items:
+      - description: Board XO source
+      - description: GPLL0 source from GCC
+
+  clock-names:
+    minItems: 1
+    maxItems: 2
+    items:
+      - const: xo
+      - const: gpll0
+
+  '#clock-cells':
+    const: 1
+
+  '#reset-cells':
+    const: 1
+
+  '#power-domain-cells':
+    const: 1
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+  - '#reset-cells'
+  - '#power-domain-cells'
+
+examples:
+  # Example of GPUCC with clock node properties for SDM845:
+  - |
+    clock-controller@5090000 {
+      compatible = "qcom,sdm845-gpucc";
+      reg = <0x5090000 0x9000>;
+      clocks = <&rpmhcc 0>, <&gcc 32>;
+      clock-names = "xo", "gpll0";
+      #clock-cells = <1>;
+      #reset-cells = <1>;
+      #power-domain-cells = <1>;
+     };
+...
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v1 2/7] dt-bindings: clock: Add YAML schemas for the QCOM GPUCC clock bindings

[Stephen Boyd]

Quoting Taniya Das (2019-10-31 05:21:08)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
> new file mode 100644
> index 0000000..96aaf36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Can it be GPL2 or BSD? I think Rob is asking for that sort of license on
these files.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/clock/qcom,gpucc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Graphics Clock & Reset Controller Binding
> +
> +maintainers:
> +  - Taniya Das <tdas@codeaurora.org>
> +
> +description: |
> +  Qualcomm grpahics clock control module which supports the clocks, resets and
> +  power domains.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sdm845-gpucc
> +      - qcom,msm8998-gpucc

Sort please.

Re: [PATCH v1 2/7] dt-bindings: clock: Add YAML schemas for the QCOM GPUCC clock bindings

[Rob Herring]

On Tue, Nov 5, 2019 at 6:26 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Taniya Das (2019-10-31 05:21:08)
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
> > new file mode 100644
> > index 0000000..96aaf36
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
>
> Can it be GPL2 or BSD? I think Rob is asking for that sort of license on
> these files.

I do, but only on new bindings unless we determine relicensing is
okay. Though here it doesn't look like much is copied over.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/bindings/clock/qcom,gpucc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Graphics Clock & Reset Controller Binding
> > +
> > +maintainers:
> > +  - Taniya Das <tdas@codeaurora.org>
> > +
> > +description: |
> > +  Qualcomm grpahics clock control module which supports the clocks, resets and
> > +  power domains.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - qcom,sdm845-gpucc
> > +      - qcom,msm8998-gpucc
>
> Sort please.

When you get tired of telling people to do this we can make the
tooling do it. :) Shouldn't be too hard. The majority of the work is
probably fixing the existing cases that aren't sorted.

Rob

[PATCH v1 3/7] dt-bindings: clock: Introduce QCOM Graphics clock bindings

[Taniya Das]

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

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../devicetree/bindings/clock/qcom,gpucc.yaml       |  1 +
 include/dt-bindings/clock/qcom,gpucc-sc7180.h       | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)
 create mode 100644 include/dt-bindings/clock/qcom,gpucc-sc7180.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
index 96aaf36..140df91 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
@@ -18,6 +18,7 @@ properties:
     enum:
       - qcom,sdm845-gpucc
       - qcom,msm8998-gpucc
+      - qcom,sc7180-gpucc

   clocks:
     minItems: 1
diff --git a/include/dt-bindings/clock/qcom,gpucc-sc7180.h b/include/dt-bindings/clock/qcom,gpucc-sc7180.h
new file mode 100644
index 0000000..0e4643b
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,gpucc-sc7180.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_QCOM_GPU_CC_SC7180_H
+#define _DT_BINDINGS_CLK_QCOM_GPU_CC_SC7180_H
+
+#define GPU_CC_PLL1			0
+#define GPU_CC_AHB_CLK			1
+#define GPU_CC_CRC_AHB_CLK		2
+#define GPU_CC_CX_GMU_CLK		3
+#define GPU_CC_CX_SNOC_DVM_CLK		4
+#define GPU_CC_CXO_AON_CLK		5
+#define GPU_CC_CXO_CLK			6
+#define GPU_CC_GMU_CLK_SRC		7
+
+/* CAM_CC GDSCRs */
+#define CX_GDSC				0
+
+#endif
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

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

[Rob Herring]

On Thu, 31 Oct 2019 17:51:09 +0530, Taniya Das wrote:
> Add device tree bindings for graphics clock controller for
> Qualcomm Technology Inc's SC7180 SoCs.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  .../devicetree/bindings/clock/qcom,gpucc.yaml       |  1 +
>  include/dt-bindings/clock/qcom,gpucc-sc7180.h       | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
>  create mode 100644 include/dt-bindings/clock/qcom,gpucc-sc7180.h
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v1 4/7] clk: qcom: Add graphics clock controller driver for SC7180

[Taniya Das]

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

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/Kconfig        |   8 ++
 drivers/clk/qcom/Makefile       |   1 +
 drivers/clk/qcom/gpucc-sc7180.c | 274 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 283 insertions(+)
 create mode 100644 drivers/clk/qcom/gpucc-sc7180.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 91706d8..c898e62 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -236,6 +236,14 @@ config SC_GCC_7180
 	  Say Y if you want to use peripheral devices such as UART, SPI,
 	  I2C, USB, UFS, SDCC, etc.

+config SC_GPUCC_7180
+	tristate "SC7180 Graphics Clock Controller"
+	select SC_GCC_7180
+	help
+	  Support for the graphics clock controller on SC7180 devices.
+	  Say Y if you want to support graphics controller devices and
+	  functionality such as 3D graphics.
+
 config SDM_CAMCC_845
 	tristate "SDM845 Camera Clock Controller"
 	select SDM_GCC_845
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 6fb356a..b89a292 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
 obj-$(CONFIG_QCS_GCC_404) += gcc-qcs404.o
 obj-$(CONFIG_QCS_TURING_404) += turingcc-qcs404.o
 obj-$(CONFIG_SC_GCC_7180) += gcc-sc7180.o
+obj-$(CONFIG_SC_GPUCC_7180) += gpucc-sc7180.o
 obj-$(CONFIG_SDM_CAMCC_845) += camcc-sdm845.o
 obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
 obj-$(CONFIG_SDM_GCC_660) += gcc-sdm660.o
diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c
new file mode 100644
index 0000000..0d893e6
--- /dev/null
+++ b/drivers/clk/qcom/gpucc-sc7180.c
@@ -0,0 +1,274 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,gpucc-sc7180.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "common.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_PLL1_OUT_EVEN,
+	P_GPU_CC_PLL1_OUT_MAIN,
+	P_GPU_CC_PLL1_OUT_ODD,
+};
+
+static struct pll_vco fabia_vco[] = {
+	{ 249600000, 2000000000, 0 },
+};
+
+static struct clk_alpha_pll gpu_cc_pll1 = {
+	.offset = 0x100,
+	.vco_table = fabia_vco,
+	.num_vco = ARRAY_SIZE(fabia_vco),
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+	.clkr = {
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_pll1",
+			.parent_data =  &(const struct clk_parent_data){
+				.fw_name = "bi_tcxo",
+				.name = "bi_tcxo",
+			},
+			.num_parents = 1,
+			.ops = &clk_alpha_pll_fabia_ops,
+		},
+	},
+};
+
+static const struct parent_map gpu_cc_parent_map_0[] = {
+	{ P_BI_TCXO, 0 },
+	{ 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 struct clk_parent_data gpu_cc_parent_data_0[] = {
+	{ .fw_name = "bi_tcxo", .name = "bi_tcxo" },
+	{ .hw = &gpu_cc_pll1.clkr.hw },
+	{ .fw_name = "gcc_gpu_gpll0_clk_src", .name = "gcc_gpu_gpll0_clk_src" },
+	{ .fw_name = "gcc_gpu_gpll0_div_clk_src",
+					.name = "gcc_gpu_gpll0_div_clk_src" },
+	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
+};
+
+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),
+	{ }
+};
+
+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_data = gpu_cc_parent_data_0,
+		.num_parents = 5,
+		.flags = CLK_SET_RATE_PARENT,
+		.ops = &clk_rcg2_shared_ops,
+	},
+};
+
+static struct clk_branch gpu_cc_crc_ahb_clk = {
+	.halt_reg = 0x107c,
+	.halt_check = BRANCH_HALT_DELAY,
+	.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_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_data =  &(const struct clk_parent_data){
+				.hw = &gpu_cc_gmu_clk_src.clkr.hw,
+			},
+			.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_DELAY,
+	.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_aon_clk = {
+	.halt_reg = 0x1004,
+	.halt_check = BRANCH_HALT_DELAY,
+	.clkr = {
+		.enable_reg = 0x1004,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gpu_cc_cxo_aon_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 gdsc cx_gdsc = {
+	.gdscr = 0x106c,
+	.gds_hw_ctrl = 0x1540,
+	.pd = {
+		.name = "cx_gdsc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+	.flags = VOTABLE,
+};
+
+static struct gdsc *gpu_cc_sc7180_gdscs[] = {
+	[CX_GDSC] = &cx_gdsc,
+};
+
+static struct clk_regmap *gpu_cc_sc7180_clocks[] = {
+	[GPU_CC_CXO_CLK] = &gpu_cc_cxo_clk.clkr,
+	[GPU_CC_CRC_AHB_CLK] = &gpu_cc_crc_ahb_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_AON_CLK] = &gpu_cc_cxo_aon_clk.clkr,
+	[GPU_CC_GMU_CLK_SRC] = &gpu_cc_gmu_clk_src.clkr,
+	[GPU_CC_PLL1] = &gpu_cc_pll1.clkr,
+};
+
+static const struct regmap_config gpu_cc_sc7180_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_sc7180_desc = {
+	.config = &gpu_cc_sc7180_regmap_config,
+	.clks = gpu_cc_sc7180_clocks,
+	.num_clks = ARRAY_SIZE(gpu_cc_sc7180_clocks),
+	.gdscs = gpu_cc_sc7180_gdscs,
+	.num_gdscs = ARRAY_SIZE(gpu_cc_sc7180_gdscs),
+};
+
+static const struct of_device_id gpu_cc_sc7180_match_table[] = {
+	{ .compatible = "qcom,sc7180-gpucc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gpu_cc_sc7180_match_table);
+
+static int gpu_cc_sc7180_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+	struct alpha_pll_config gpu_cc_pll_config = {};
+	unsigned int value, mask;
+
+	regmap = qcom_cc_map(pdev, &gpu_cc_sc7180_desc);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	/* 360MHz Configuration */
+	gpu_cc_pll_config.l = 0x12;
+	gpu_cc_pll_config.alpha = 0xC000;
+	gpu_cc_pll_config.config_ctl_val = 0x20485699;
+	gpu_cc_pll_config.config_ctl_hi_val = 0x00002067;
+	gpu_cc_pll_config.user_ctl_val = 0x00000001;
+	gpu_cc_pll_config.user_ctl_hi_val = 0x00004805;
+	gpu_cc_pll_config.test_ctl_hi_val = 0x40000000;
+
+	clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll_config);
+
+	/* Recommended WAKEUP/SLEEP settings for the gpu_cc_cx_gmu_clk */
+	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);
+
+	/* gpu_cc_ahb_clk */
+	regmap_update_bits(regmap, 0x1078, 0x1, 0x1);
+
+	/* Configure clk_dis_wait 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_sc7180_desc, regmap);
+}
+
+static struct platform_driver gpu_cc_sc7180_driver = {
+	.probe = gpu_cc_sc7180_probe,
+	.driver = {
+		.name = "sc7180-gpucc",
+		.of_match_table = gpu_cc_sc7180_match_table,
+	},
+};
+
+static int __init gpu_cc_sc7180_init(void)
+{
+	return platform_driver_register(&gpu_cc_sc7180_driver);
+}
+core_initcall(gpu_cc_sc7180_init);
+
+static void __exit gpu_cc_sc7180_exit(void)
+{
+	platform_driver_unregister(&gpu_cc_sc7180_driver);
+}
+module_exit(gpu_cc_sc7180_exit);
+
+MODULE_DESCRIPTION("QTI GPU_CC SC7180 Driver");
+MODULE_LICENSE("GPL v2");
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v1 4/7] clk: qcom: Add graphics clock controller driver for SC7180

[Stephen Boyd]

Quoting Taniya Das (2019-10-31 05:21:10)
> diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c
> new file mode 100644
> index 0000000..0d893e6
> --- /dev/null
> +++ b/drivers/clk/qcom/gpucc-sc7180.c
> @@ -0,0 +1,274 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>

Are these of includes used?

> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,gpucc-sc7180.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "common.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_PLL1_OUT_EVEN,
> +       P_GPU_CC_PLL1_OUT_MAIN,
> +       P_GPU_CC_PLL1_OUT_ODD,
> +};
> +
> +static struct pll_vco fabia_vco[] = {

const?

> +       { 249600000, 2000000000, 0 },
> +};
> +
> +static struct clk_alpha_pll gpu_cc_pll1 = {
> +       .offset = 0x100,
> +       .vco_table = fabia_vco,
> +       .num_vco = ARRAY_SIZE(fabia_vco),
> +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
> +       .clkr = {
> +               .hw.init = &(struct clk_init_data){
> +                       .name = "gpu_cc_pll1",
> +                       .parent_data =  &(const struct clk_parent_data){
> +                               .fw_name = "bi_tcxo",
> +                               .name = "bi_tcxo",

Do we need both? This is new so it should just work with .fw_name right?

> +                       },
> +                       .num_parents = 1,
> +                       .ops = &clk_alpha_pll_fabia_ops,
> +               },
> +       },
> +};
> +
> +static const struct parent_map gpu_cc_parent_map_0[] = {
> +       { P_BI_TCXO, 0 },
> +       { 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 struct clk_parent_data gpu_cc_parent_data_0[] = {
> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
> +       { .hw = &gpu_cc_pll1.clkr.hw },
> +       { .fw_name = "gcc_gpu_gpll0_clk_src", .name = "gcc_gpu_gpll0_clk_src" },
> +       { .fw_name = "gcc_gpu_gpll0_div_clk_src",
> +                                       .name = "gcc_gpu_gpll0_div_clk_src" },
> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +};

Same for these.

> +
> +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),
> +       { }
> +};
[..]
> +
> +
> +static int gpu_cc_sc7180_probe(struct platform_device *pdev)
> +{
> +       struct regmap *regmap;
> +       struct alpha_pll_config gpu_cc_pll_config = {};
> +       unsigned int value, mask;
> +
> +       regmap = qcom_cc_map(pdev, &gpu_cc_sc7180_desc);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       /* 360MHz Configuration */
> +       gpu_cc_pll_config.l = 0x12;
> +       gpu_cc_pll_config.alpha = 0xC000;
> +       gpu_cc_pll_config.config_ctl_val = 0x20485699;
> +       gpu_cc_pll_config.config_ctl_hi_val = 0x00002067;
> +       gpu_cc_pll_config.user_ctl_val = 0x00000001;
> +       gpu_cc_pll_config.user_ctl_hi_val = 0x00004805;
> +       gpu_cc_pll_config.test_ctl_hi_val = 0x40000000;

Is there a reason this is built on the stack? Save space or something?

> +
> +       clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll_config);
> +
> +       /* Recommended WAKEUP/SLEEP settings for the gpu_cc_cx_gmu_clk */
> +       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;

mask and value can just be big constants? I'm not sure anyone cares to
tweak this later, but I guess it's fine this way too.

> +       regmap_update_bits(regmap, 0x1098, mask, value);
> +
> +       /* gpu_cc_ahb_clk */

What are we doing to gpu_cc_ahb_clk??

> +       regmap_update_bits(regmap, 0x1078, 0x1, 0x1);
> +
> +       /* Configure clk_dis_wait 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_sc7180_desc, regmap);
> +}
> +
> +static struct platform_driver gpu_cc_sc7180_driver = {
> +       .probe = gpu_cc_sc7180_probe,
> +       .driver = {
> +               .name = "sc7180-gpucc",
> +               .of_match_table = gpu_cc_sc7180_match_table,
> +       },
> +};
> +
> +static int __init gpu_cc_sc7180_init(void)
> +{
> +       return platform_driver_register(&gpu_cc_sc7180_driver);
> +}
> +core_initcall(gpu_cc_sc7180_init);

Does it need to be core initcal? Maybe module_platform_driver() works
just as well?

> +
> +static void __exit gpu_cc_sc7180_exit(void)
> +{
> +       platform_driver_unregister(&gpu_cc_sc7180_driver);
> +}
> +module_exit(gpu_cc_sc7180_exit);
> +
> +MODULE_DESCRIPTION("QTI GPU_CC SC7180 Driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v1 4/7] clk: qcom: Add graphics clock controller driver for SC7180

[Taniya Das]

Hi Stephen,

Thanks for the review.

On 11/6/2019 6:00 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2019-10-31 05:21:10)
>> diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c
>> new file mode 100644
>> index 0000000..0d893e6
>> --- /dev/null
>> +++ b/drivers/clk/qcom/gpucc-sc7180.c
>> @@ -0,0 +1,274 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
> 
> Are these of includes used?
>

yes, would clean up these headers.


>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/qcom,gpucc-sc7180.h>
>> +
>> +#include "clk-alpha-pll.h"
>> +#include "clk-branch.h"
>> +#include "clk-rcg.h"
>> +#include "clk-regmap.h"
>> +#include "common.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_PLL1_OUT_EVEN,
>> +       P_GPU_CC_PLL1_OUT_MAIN,
>> +       P_GPU_CC_PLL1_OUT_ODD,
>> +};
>> +
>> +static struct pll_vco fabia_vco[] = {
> 
> const?
> 

Will take care.

>> +       { 249600000, 2000000000, 0 },
>> +};
>> +
>> +static struct clk_alpha_pll gpu_cc_pll1 = {
>> +       .offset = 0x100,
>> +       .vco_table = fabia_vco,
>> +       .num_vco = ARRAY_SIZE(fabia_vco),
>> +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
>> +       .clkr = {
>> +               .hw.init = &(struct clk_init_data){
>> +                       .name = "gpu_cc_pll1",
>> +                       .parent_data =  &(const struct clk_parent_data){
>> +                               .fw_name = "bi_tcxo",
>> +                               .name = "bi_tcxo",
> 
> Do we need both? This is new so it should just work with .fw_name right?
> 

yes, will clean this up.

>> +                       },
>> +                       .num_parents = 1,
>> +                       .ops = &clk_alpha_pll_fabia_ops,
>> +               },
>> +       },
>> +};
>> +
>> +static const struct parent_map gpu_cc_parent_map_0[] = {
>> +       { P_BI_TCXO, 0 },
>> +       { 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 struct clk_parent_data gpu_cc_parent_data_0[] = {
>> +       { .fw_name = "bi_tcxo", .name = "bi_tcxo" },
>> +       { .hw = &gpu_cc_pll1.clkr.hw },
>> +       { .fw_name = "gcc_gpu_gpll0_clk_src", .name = "gcc_gpu_gpll0_clk_src" },
>> +       { .fw_name = "gcc_gpu_gpll0_div_clk_src",
>> +                                       .name = "gcc_gpu_gpll0_div_clk_src" },
>> +       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
>> +};
> 
> Same for these.
> 
>> +
>> +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),
>> +       { }
>> +};
> [..]
>> +
>> +
>> +static int gpu_cc_sc7180_probe(struct platform_device *pdev)
>> +{
>> +       struct regmap *regmap;
>> +       struct alpha_pll_config gpu_cc_pll_config = {};
>> +       unsigned int value, mask;
>> +
>> +       regmap = qcom_cc_map(pdev, &gpu_cc_sc7180_desc);
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
>> +
>> +       /* 360MHz Configuration */
>> +       gpu_cc_pll_config.l = 0x12;
>> +       gpu_cc_pll_config.alpha = 0xC000;
>> +       gpu_cc_pll_config.config_ctl_val = 0x20485699;
>> +       gpu_cc_pll_config.config_ctl_hi_val = 0x00002067;
>> +       gpu_cc_pll_config.user_ctl_val = 0x00000001;
>> +       gpu_cc_pll_config.user_ctl_hi_val = 0x00004805;
>> +       gpu_cc_pll_config.test_ctl_hi_val = 0x40000000;
> 
> Is there a reason this is built on the stack? Save space or something?
> 

I have done as we had discussed during the dispcc review for SDM845
https://patchwork.kernel.org/patch/10446073/
 >>>
 >> +static const struct alpha_pll_config disp_cc_pll0_config = {
 >> +       .l = 0x2c,
 >> +       .alpha = 0xcaaa,
 >> +};
 >
 > Any reason this can't be put on the stack in the probe function?
 >
I would move it.
 >>>

In case you think I should move it outside I can do that too.

>> +
>> +       clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll_config);
>> +
>> +       /* Recommended WAKEUP/SLEEP settings for the gpu_cc_cx_gmu_clk */
>> +       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;
> 
> mask and value can just be big constants? I'm not sure anyone cares to
> tweak this later, but I guess it's fine this way too.
> 
>> +       regmap_update_bits(regmap, 0x1098, mask, value);
>> +
>> +       /* gpu_cc_ahb_clk */
> 
> What are we doing to gpu_cc_ahb_clk??
> 

I was marking this CRITICAL clock, but I checked the HW specs and it is 
always left ON from HW, so will remove it.

>> +       regmap_update_bits(regmap, 0x1078, 0x1, 0x1);
>> +
>> +       /* Configure clk_dis_wait 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_sc7180_desc, regmap);
>> +}
>> +
>> +static struct platform_driver gpu_cc_sc7180_driver = {
>> +       .probe = gpu_cc_sc7180_probe,
>> +       .driver = {
>> +               .name = "sc7180-gpucc",
>> +               .of_match_table = gpu_cc_sc7180_match_table,
>> +       },
>> +};
>> +
>> +static int __init gpu_cc_sc7180_init(void)
>> +{
>> +       return platform_driver_register(&gpu_cc_sc7180_driver);
>> +}
>> +core_initcall(gpu_cc_sc7180_init);
> 
> Does it need to be core initcal? Maybe module_platform_driver() works
> just as well?
> 

I will leave it to subsys_initcall.

>> +
>> +static void __exit gpu_cc_sc7180_exit(void)
>> +{
>> +       platform_driver_unregister(&gpu_cc_sc7180_driver);
>> +}
>> +module_exit(gpu_cc_sc7180_exit);
>> +
>> +MODULE_DESCRIPTION("QTI GPU_CC SC7180 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 v1 4/7] clk: qcom: Add graphics clock controller driver for SC7180

[Stephen Boyd]

Quoting Taniya Das (2019-11-15 00:11:28)
> Hi Stephen,
> 
> Thanks for the review.
> 
> On 11/6/2019 6:00 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2019-10-31 05:21:10)
> >> diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c
> >> new file mode 100644
> >> index 0000000..0d893e6
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/gpucc-sc7180.c
> >> @@ -0,0 +1,274 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#include <linux/clk-provider.h>
> >> +#include <linux/err.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> > 
> > Are these of includes used?
> >
> 
> yes, would clean up these headers.
> 

Maybe they're used. I'm not sure.

> >> +       regmap = qcom_cc_map(pdev, &gpu_cc_sc7180_desc);
> >> +       if (IS_ERR(regmap))
> >> +               return PTR_ERR(regmap);
> >> +
> >> +       /* 360MHz Configuration */
> >> +       gpu_cc_pll_config.l = 0x12;
> >> +       gpu_cc_pll_config.alpha = 0xC000;
> >> +       gpu_cc_pll_config.config_ctl_val = 0x20485699;
> >> +       gpu_cc_pll_config.config_ctl_hi_val = 0x00002067;
> >> +       gpu_cc_pll_config.user_ctl_val = 0x00000001;
> >> +       gpu_cc_pll_config.user_ctl_hi_val = 0x00004805;
> >> +       gpu_cc_pll_config.test_ctl_hi_val = 0x40000000;
> > 
> > Is there a reason this is built on the stack? Save space or something?
> > 
> 
> I have done as we had discussed during the dispcc review for SDM845
> https://patchwork.kernel.org/patch/10446073/
>  >>>
>  >> +static const struct alpha_pll_config disp_cc_pll0_config = {
>  >> +       .l = 0x2c,
>  >> +       .alpha = 0xcaaa,
>  >> +};
>  >
>  > Any reason this can't be put on the stack in the probe function?
>  >
> I would move it.
>  >>>
> 
> In case you think I should move it outside I can do that too.

No I was just wondering what prompted it.

[PATCH v1 5/7] dt-bindings: clock: Add YAML schemas for the QCOM VIDEOCC clock bindings

[Taniya Das]

The VIDEOCC clock provider have a bunch of generic properties that
are needed in a device tree. Add a YAML schemas for those.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../devicetree/bindings/clock/qcom,videocc.txt     | 18 -------
 .../devicetree/bindings/clock/qcom,videocc.yaml    | 61 ++++++++++++++++++++++
 2 files changed, 61 insertions(+), 18 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/qcom,videocc.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,videocc.yaml

diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.txt b/Documentation/devicetree/bindings/clock/qcom,videocc.txt
deleted file mode 100644
index 8a8622c..0000000
--- a/Documentation/devicetree/bindings/clock/qcom,videocc.txt
+++ /dev/null
@@ -1,18 +0,0 @@
-Qualcomm Video Clock & Reset Controller Binding
------------------------------------------------
-
-Required properties :
-- compatible : shall contain "qcom,sdm845-videocc"
-- reg : shall contain base register location and length
-- #clock-cells : from common clock binding, shall contain 1.
-- #power-domain-cells : from generic power domain binding, shall contain 1.
-- #reset-cells : from common reset binding, shall contain 1.
-
-Example:
-	videocc: clock-controller@ab00000 {
-		compatible = "qcom,sdm845-videocc";
-		reg = <0xab00000 0x10000>;
-		#clock-cells = <1>;
-		#power-domain-cells = <1>;
-		#reset-cells = <1>;
-	};
diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
new file mode 100644
index 0000000..fc3fcca
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bindings/clock/qcom,videocc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Video Clock & Reset Controller Binding
+
+maintainers:
+  - Taniya Das <tdas@codeaurora.org>
+
+description: |
+  Qualcomm video clock control module which supports the clocks, resets and
+  power domains.
+
+properties:
+  compatible:
+    enum:
+      - qcom,sdm845-videocc
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: xo
+
+  '#clock-cells':
+    const: 1
+
+  '#reset-cells':
+    const: 1
+
+  '#power-domain-cells':
+    const: 1
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - '#clock-cells'
+  - '#reset-cells'
+  - '#power-domain-cells'
+
+examples:
+  # Example of VIDEOCC with clock node properties for SDM845:
+  - |
+    clock-controller@ab00000 {
+      compatible = "qcom,sdm845-videocc";
+      reg = <0xab00000 0x10000>;
+      clocks = <&rpmhcc 0>;
+      clock-names = "xo";
+      #clock-cells = <1>;
+      #reset-cells = <1>;
+      #power-domain-cells = <1>;
+     };
+...
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v1 5/7] dt-bindings: clock: Add YAML schemas for the QCOM VIDEOCC clock bindings

[Rob Herring]

On Thu, 31 Oct 2019 17:51:11 +0530, Taniya Das wrote:
> The VIDEOCC clock provider have a bunch of generic properties that
> are needed in a device tree. Add a YAML schemas for those.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  .../devicetree/bindings/clock/qcom,videocc.txt     | 18 -------
>  .../devicetree/bindings/clock/qcom,videocc.yaml    | 61 ++++++++++++++++++++++
>  2 files changed, 61 insertions(+), 18 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/clock/qcom,videocc.txt
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,videocc.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v1 6/7] dt-bindings: clock: Introduce QCOM Video clock bindings

[Taniya Das]

Add device tree bindings for video clock controller for
Qualcomm Technology Inc's SC7180 SoCs.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../devicetree/bindings/clock/qcom,videocc.yaml    |  1 +
 include/dt-bindings/clock/qcom,videocc-sc7180.h    | 23 ++++++++++++++++++++++
 2 files changed, 24 insertions(+)
 create mode 100644 include/dt-bindings/clock/qcom,videocc-sc7180.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
index fc3fcca..9b8690c 100644
--- a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
@@ -17,6 +17,7 @@ properties:
   compatible:
     enum:
       - qcom,sdm845-videocc
+      - qcom,sc7180-videocc

   clocks:
     maxItems: 1
diff --git a/include/dt-bindings/clock/qcom,videocc-sc7180.h b/include/dt-bindings/clock/qcom,videocc-sc7180.h
new file mode 100644
index 0000000..7acaf13
--- /dev/null
+++ b/include/dt-bindings/clock/qcom,videocc-sc7180.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_QCOM_VIDEO_CC_SC7180_H
+#define _DT_BINDINGS_CLK_QCOM_VIDEO_CC_SC7180_H
+
+/* VIDEO_CC clocks */
+#define VIDEO_PLL0				0
+#define VIDEO_CC_VCODEC0_AXI_CLK		1
+#define VIDEO_CC_VCODEC0_CORE_CLK		2
+#define VIDEO_CC_VENUS_AHB_CLK			3
+#define VIDEO_CC_VENUS_CLK_SRC			4
+#define VIDEO_CC_VENUS_CTL_AXI_CLK		5
+#define VIDEO_CC_VENUS_CTL_CORE_CLK		6
+#define VIDEO_CC_XO_CLK				7
+
+/* VIDEO_CC GDSCRs */
+#define VENUS_GDSC				0
+#define VCODEC0_GDSC				1
+
+#endif
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v1 6/7] dt-bindings: clock: Introduce QCOM Video clock bindings

[Stephen Boyd]

Quoting Taniya Das (2019-10-31 05:21:12)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
> index fc3fcca..9b8690c 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
> @@ -17,6 +17,7 @@ properties:
>    compatible:
>      enum:
>        - qcom,sdm845-videocc
> +      - qcom,sc7180-videocc

Sort.

> 
>    clocks:
>      maxItems: 1

[PATCH v1 7/7] clk: qcom: Add video clock controller driver for SC7180

[Taniya Das]

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

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/clk/qcom/Kconfig          |   8 ++
 drivers/clk/qcom/Makefile         |   1 +
 drivers/clk/qcom/videocc-sc7180.c | 263 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 272 insertions(+)
 create mode 100644 drivers/clk/qcom/videocc-sc7180.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index c898e62..c5ad2cc 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -244,6 +244,14 @@ config SC_GPUCC_7180
 	  Say Y if you want to support graphics controller devices and
 	  functionality such as 3D graphics.

+config SC_VIDEOCC_7180
+	tristate "SC7180 Video Clock Controller"
+	select SC_GCC_7180
+	help
+	  Support for the video clock controller on SC7180 devices.
+	  Say Y if you want to support video devices and functionality such as
+	  video encode and decode.
+
 config SDM_CAMCC_845
 	tristate "SDM845 Camera Clock Controller"
 	select SDM_GCC_845
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index b89a292..0906e5d 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_QCS_GCC_404) += gcc-qcs404.o
 obj-$(CONFIG_QCS_TURING_404) += turingcc-qcs404.o
 obj-$(CONFIG_SC_GCC_7180) += gcc-sc7180.o
 obj-$(CONFIG_SC_GPUCC_7180) += gpucc-sc7180.o
+obj-$(CONFIG_SC_VIDEOCC_7180) += videocc-sc7180.o
 obj-$(CONFIG_SDM_CAMCC_845) += camcc-sdm845.o
 obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
 obj-$(CONFIG_SDM_GCC_660) += gcc-sdm660.o
diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
new file mode 100644
index 0000000..bef034b
--- /dev/null
+++ b/drivers/clk/qcom/videocc-sc7180.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/qcom,videocc-sc7180.h>
+
+#include "clk-alpha-pll.h"
+#include "clk-branch.h"
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+#include "common.h"
+#include "gdsc.h"
+
+enum {
+	P_BI_TCXO,
+	P_CHIP_SLEEP_CLK,
+	P_CORE_BI_PLL_TEST_SE,
+	P_VIDEO_PLL0_OUT_EVEN,
+	P_VIDEO_PLL0_OUT_MAIN,
+	P_VIDEO_PLL0_OUT_ODD,
+};
+
+static struct pll_vco fabia_vco[] = {
+	{ 249600000, 2000000000, 0 },
+};
+
+static struct clk_alpha_pll video_pll0 = {
+	.offset = 0x42c,
+	.vco_table = fabia_vco,
+	.num_vco = ARRAY_SIZE(fabia_vco),
+	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA],
+	.clkr = {
+		.hw.init = &(struct clk_init_data){
+			.name = "video_pll0",
+			.parent_data = &(const struct clk_parent_data){
+				.fw_name = "bi_tcxo",
+				.name = "bi_tcxo",
+			},
+			.num_parents = 1,
+			.ops = &clk_alpha_pll_fabia_ops,
+		},
+	},
+};
+
+static const struct parent_map video_cc_parent_map_1[] = {
+	{ P_BI_TCXO, 0 },
+	{ P_VIDEO_PLL0_OUT_MAIN, 1 },
+	{ P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const struct clk_parent_data video_cc_parent_data_1[] = {
+	{ .fw_name = "bi_tcxo", .name = "bi_tcxo" },
+	{ .hw = &video_pll0.clkr.hw },
+	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
+};
+
+static const struct freq_tbl ftbl_video_cc_venus_clk_src[] = {
+	F(19200000, P_BI_TCXO, 1, 0, 0),
+	F(150000000, P_VIDEO_PLL0_OUT_MAIN, 4, 0, 0),
+	F(270000000, P_VIDEO_PLL0_OUT_MAIN, 2.5, 0, 0),
+	F(340000000, P_VIDEO_PLL0_OUT_MAIN, 2, 0, 0),
+	F(434000000, P_VIDEO_PLL0_OUT_MAIN, 2, 0, 0),
+	F(500000000, P_VIDEO_PLL0_OUT_MAIN, 2, 0, 0),
+	{ }
+};
+
+static struct clk_rcg2 video_cc_venus_clk_src = {
+	.cmd_rcgr = 0x7f0,
+	.mnd_width = 0,
+	.hid_width = 5,
+	.parent_map = video_cc_parent_map_1,
+	.freq_tbl = ftbl_video_cc_venus_clk_src,
+	.clkr.hw.init = &(struct clk_init_data){
+		.name = "video_cc_venus_clk_src",
+		.parent_data = video_cc_parent_data_1,
+		.num_parents = 3,
+		.flags = CLK_SET_RATE_PARENT,
+		.ops = &clk_rcg2_shared_ops,
+	},
+};
+
+static struct clk_branch video_cc_vcodec0_axi_clk = {
+	.halt_reg = 0x9ec,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x9ec,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "video_cc_vcodec0_axi_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch video_cc_vcodec0_core_clk = {
+	.halt_reg = 0x890,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x890,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "video_cc_vcodec0_core_clk",
+			.parent_data = &(const struct clk_parent_data){
+				.hw = &video_cc_venus_clk_src.clkr.hw,
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch video_cc_venus_ahb_clk = {
+	.halt_reg = 0xa4c,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0xa4c,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "video_cc_venus_ahb_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch video_cc_venus_ctl_axi_clk = {
+	.halt_reg = 0x9cc,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x9cc,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "video_cc_venus_ctl_axi_clk",
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch video_cc_venus_ctl_core_clk = {
+	.halt_reg = 0x850,
+	.halt_check = BRANCH_HALT,
+	.clkr = {
+		.enable_reg = 0x850,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "video_cc_venus_ctl_core_clk",
+			.parent_data = &(const struct clk_parent_data){
+				.hw = &video_cc_venus_clk_src.clkr.hw,
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct gdsc venus_gdsc = {
+	.gdscr = 0x814,
+	.pd = {
+		.name = "venus_gdsc",
+	},
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct gdsc vcodec0_gdsc = {
+	.gdscr = 0x874,
+	.pd = {
+		.name = "vcodec0_gdsc",
+	},
+	.flags = HW_CTRL,
+	.pwrsts = PWRSTS_OFF_ON,
+};
+
+static struct clk_regmap *video_cc_sc7180_clocks[] = {
+	[VIDEO_CC_VCODEC0_AXI_CLK] = &video_cc_vcodec0_axi_clk.clkr,
+	[VIDEO_CC_VCODEC0_CORE_CLK] = &video_cc_vcodec0_core_clk.clkr,
+	[VIDEO_CC_VENUS_AHB_CLK] = &video_cc_venus_ahb_clk.clkr,
+	[VIDEO_CC_VENUS_CLK_SRC] = &video_cc_venus_clk_src.clkr,
+	[VIDEO_CC_VENUS_CTL_AXI_CLK] = &video_cc_venus_ctl_axi_clk.clkr,
+	[VIDEO_CC_VENUS_CTL_CORE_CLK] = &video_cc_venus_ctl_core_clk.clkr,
+	[VIDEO_PLL0] = &video_pll0.clkr,
+};
+
+static struct gdsc *video_cc_sc7180_gdscs[] = {
+	[VENUS_GDSC] = &venus_gdsc,
+	[VCODEC0_GDSC] = &vcodec0_gdsc,
+};
+
+static const struct regmap_config video_cc_sc7180_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = 0xb94,
+	.fast_io = true,
+};
+
+static const struct qcom_cc_desc video_cc_sc7180_desc = {
+	.config = &video_cc_sc7180_regmap_config,
+	.clks = video_cc_sc7180_clocks,
+	.num_clks = ARRAY_SIZE(video_cc_sc7180_clocks),
+	.gdscs = video_cc_sc7180_gdscs,
+	.num_gdscs = ARRAY_SIZE(video_cc_sc7180_gdscs),
+};
+
+static const struct of_device_id video_cc_sc7180_match_table[] = {
+	{ .compatible = "qcom,sc7180-videocc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, video_cc_sc7180_match_table);
+
+static int video_cc_sc7180_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+	struct alpha_pll_config video_pll0_config = {};
+
+	regmap = qcom_cc_map(pdev, &video_cc_sc7180_desc);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	video_pll0_config.l = 0x1F;
+	video_pll0_config.alpha = 0x4000;
+	video_pll0_config.user_ctl_val = 0x00000001;
+	video_pll0_config.user_ctl_hi_val = 0x00004805;
+
+	clk_fabia_pll_configure(&video_pll0, regmap, &video_pll0_config);
+
+	/* video_cc_xo_clk */
+	regmap_update_bits(regmap, 0x984, 0x1, 0x1);
+
+	return qcom_cc_really_probe(pdev, &video_cc_sc7180_desc, regmap);
+}
+
+static struct platform_driver video_cc_sc7180_driver = {
+	.probe = video_cc_sc7180_probe,
+	.driver = {
+		.name = "sc7180-videocc",
+		.of_match_table = video_cc_sc7180_match_table,
+	},
+};
+
+static int __init video_cc_sc7180_init(void)
+{
+	return platform_driver_register(&video_cc_sc7180_driver);
+}
+core_initcall(video_cc_sc7180_init);
+
+static void __exit video_cc_sc7180_exit(void)
+{
+	platform_driver_unregister(&video_cc_sc7180_driver);
+}
+module_exit(video_cc_sc7180_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("QTI VIDEOCC SC7180 Driver");
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v1 7/7] clk: qcom: Add video clock controller driver for SC7180

[Stephen Boyd]

Quoting Taniya Das (2019-10-31 05:21:13)
> diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
> new file mode 100644
> index 0000000..bef034b
> --- /dev/null
> +++ b/drivers/clk/qcom/videocc-sc7180.c
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>

Are these of includes used?

> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,videocc-sc7180.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "common.h"
> +#include "gdsc.h"
> +
> +enum {
> +       P_BI_TCXO,
> +       P_CHIP_SLEEP_CLK,
> +       P_CORE_BI_PLL_TEST_SE,
> +       P_VIDEO_PLL0_OUT_EVEN,
> +       P_VIDEO_PLL0_OUT_MAIN,
> +       P_VIDEO_PLL0_OUT_ODD,
> +};
> +
> +static struct pll_vco fabia_vco[] = {

const?

> +       { 249600000, 2000000000, 0 },
> +};
> +
[...]
> +
> +static int video_cc_sc7180_probe(struct platform_device *pdev)
> +{
> +       struct regmap *regmap;
> +       struct alpha_pll_config video_pll0_config = {};
> +
> +       regmap = qcom_cc_map(pdev, &video_cc_sc7180_desc);
> +       if (IS_ERR(regmap))
> +               return PTR_ERR(regmap);
> +
> +       video_pll0_config.l = 0x1F;

lowercase hex please.

> +       video_pll0_config.alpha = 0x4000;
> +       video_pll0_config.user_ctl_val = 0x00000001;
> +       video_pll0_config.user_ctl_hi_val = 0x00004805;

Same question, why on stack?

> +
> +       clk_fabia_pll_configure(&video_pll0, regmap, &video_pll0_config);
> +
> +       /* video_cc_xo_clk */

What are we doing? Enabling it?

> +       regmap_update_bits(regmap, 0x984, 0x1, 0x1);
> +
> +       return qcom_cc_really_probe(pdev, &video_cc_sc7180_desc, regmap);
> +}
> +
> +static struct platform_driver video_cc_sc7180_driver = {
> +       .probe = video_cc_sc7180_probe,
> +       .driver = {
> +               .name = "sc7180-videocc",
> +               .of_match_table = video_cc_sc7180_match_table,
> +       },
> +};
> +
> +static int __init video_cc_sc7180_init(void)
> +{
> +       return platform_driver_register(&video_cc_sc7180_driver);
> +}
> +core_initcall(video_cc_sc7180_init);
> +
> +static void __exit video_cc_sc7180_exit(void)
> +{
> +       platform_driver_unregister(&video_cc_sc7180_driver);
> +}
> +module_exit(video_cc_sc7180_exit);

Same question, module platform driver perhaps?

Re: [PATCH v1 7/7] clk: qcom: Add video clock controller driver for SC7180

[Taniya Das]

Hi Stephen,

Thanks for the review.

On 11/6/2019 6:09 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2019-10-31 05:21:13)
>> diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
>> new file mode 100644
>> index 0000000..bef034b
>> --- /dev/null
>> +++ b/drivers/clk/qcom/videocc-sc7180.c
>> @@ -0,0 +1,263 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2019, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
> 
> Are these of includes used?
> 

I will clean them up.

>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/qcom,videocc-sc7180.h>
>> +
>> +#include "clk-alpha-pll.h"
>> +#include "clk-branch.h"
>> +#include "clk-rcg.h"
>> +#include "clk-regmap.h"
>> +#include "common.h"
>> +#include "gdsc.h"
>> +
>> +enum {
>> +       P_BI_TCXO,
>> +       P_CHIP_SLEEP_CLK,
>> +       P_CORE_BI_PLL_TEST_SE,
>> +       P_VIDEO_PLL0_OUT_EVEN,
>> +       P_VIDEO_PLL0_OUT_MAIN,
>> +       P_VIDEO_PLL0_OUT_ODD,
>> +};
>> +
>> +static struct pll_vco fabia_vco[] = {
> 
> const?
> 

yes, will add it.

>> +       { 249600000, 2000000000, 0 },
>> +};
>> +
> [...]
>> +
>> +static int video_cc_sc7180_probe(struct platform_device *pdev)
>> +{
>> +       struct regmap *regmap;
>> +       struct alpha_pll_config video_pll0_config = {};
>> +
>> +       regmap = qcom_cc_map(pdev, &video_cc_sc7180_desc);
>> +       if (IS_ERR(regmap))
>> +               return PTR_ERR(regmap);
>> +
>> +       video_pll0_config.l = 0x1F;
> 
> lowercase hex please.
> 
>> +       video_pll0_config.alpha = 0x4000;
>> +       video_pll0_config.user_ctl_val = 0x00000001;
>> +       video_pll0_config.user_ctl_hi_val = 0x00004805;
> 
> Same question, why on stack?
> 

Will update the same.

>> +
>> +       clk_fabia_pll_configure(&video_pll0, regmap, &video_pll0_config);
>> +
>> +       /* video_cc_xo_clk */
> 
> What are we doing? Enabling it?
> 

yes, enabling it. Did not model and mark it CRITICAL.

>> +       regmap_update_bits(regmap, 0x984, 0x1, 0x1);
>> +
>> +       return qcom_cc_really_probe(pdev, &video_cc_sc7180_desc, regmap);
>> +}
>> +
>> +static struct platform_driver video_cc_sc7180_driver = {
>> +       .probe = video_cc_sc7180_probe,
>> +       .driver = {
>> +               .name = "sc7180-videocc",
>> +               .of_match_table = video_cc_sc7180_match_table,
>> +       },
>> +};
>> +
>> +static int __init video_cc_sc7180_init(void)
>> +{
>> +       return platform_driver_register(&video_cc_sc7180_driver);
>> +}
>> +core_initcall(video_cc_sc7180_init);
>> +
>> +static void __exit video_cc_sc7180_exit(void)
>> +{
>> +       platform_driver_unregister(&video_cc_sc7180_driver);
>> +}
>> +module_exit(video_cc_sc7180_exit);
> 
> Same question, module platform driver perhaps?
> 

I will move it to subsys_initcall().

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

--

Re: [PATCH v1 7/7] clk: qcom: Add video clock controller driver for SC7180

[Stephen Boyd]

Quoting Taniya Das (2019-11-15 00:11:31)
> Hi Stephen,
> 
> Thanks for the review.
> 
> On 11/6/2019 6:09 AM, Stephen Boyd wrote:
> > Quoting Taniya Das (2019-10-31 05:21:13)
> >> diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c
> >> new file mode 100644
> >> index 0000000..bef034b
> >> --- /dev/null
> >> +++ b/drivers/clk/qcom/videocc-sc7180.c
> >> @@ -0,0 +1,263 @@
> > 
> >> +       video_pll0_config.alpha = 0x4000;
> >> +       video_pll0_config.user_ctl_val = 0x00000001;
> >> +       video_pll0_config.user_ctl_hi_val = 0x00004805;
> > 
> > Same question, why on stack?
> > 
> 
> Will update the same.

Sounds like nothing to do.

> 
> >> +
> >> +       clk_fabia_pll_configure(&video_pll0, regmap, &video_pll0_config);
> >> +
> >> +       /* video_cc_xo_clk */
> > 
> > What are we doing? Enabling it?
> > 
> 
> yes, enabling it. Did not model and mark it CRITICAL.

Ok. Please describe that in the comment.

> >> +}
> >> +module_exit(video_cc_sc7180_exit);
> > 
> > Same question, module platform driver perhaps?
> > 
> 
> I will move it to subsys_initcall().
> 

Hmm ok.