[PATCH 0/2] Fix and enable camcc-sm8250

[PATCH 0/2] Fix and enable camcc-sm8250

[Bryan O'Donoghue]

This series contains two patches.

First up the fix, we are missing the mmcx regulator for the camcc-sm8250
block, so if you run minus X you'll find mmcx hasn't been switched on and
camcc register access resets the board.

Second is a bog standard enablement for the camcc in the sm8250 dtsi its
nearly a 1:1 mapping with the videocc node.

Bryan O'Donoghue (2):
  clk: qcom: camcc-sm8250: Fix absent mmcx regulator reference
  arm64: dts: qcom: sm8250: Add camcc DT node

 arch/arm64/boot/dts/qcom/sm8250.dtsi | 14 ++++++++++++++
 drivers/clk/qcom/camcc-sm8250.c      |  6 ++++++
 2 files changed, 20 insertions(+)

-- 
2.30.1

[PATCH 1/2] clk: qcom: camcc-sm8250: Fix absent mmcx regulator reference

[Bryan O'Donoghue]

The current implementation omits the necessary mmcx supply, which means if
you are running the code for this block and a prior clock driver, like say
the videocc hasn't run, then a reset will be generated the first time we
touch these registers.

Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/clk/qcom/camcc-sm8250.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/qcom/camcc-sm8250.c b/drivers/clk/qcom/camcc-sm8250.c
index 439eaafdcc86..c51112546bfc 100644
--- a/drivers/clk/qcom/camcc-sm8250.c
+++ b/drivers/clk/qcom/camcc-sm8250.c
@@ -2212,6 +2212,7 @@ static struct gdsc bps_gdsc = {
 	},
 	.flags = HW_CTRL | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
+	.supply = "mmcx",
 };
 
 static struct gdsc ipe_0_gdsc = {
@@ -2221,6 +2222,7 @@ static struct gdsc ipe_0_gdsc = {
 	},
 	.flags = HW_CTRL | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
+	.supply = "mmcx",
 };
 
 static struct gdsc sbi_gdsc = {
@@ -2230,6 +2232,7 @@ static struct gdsc sbi_gdsc = {
 	},
 	.flags = HW_CTRL | POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
+	.supply = "mmcx",
 };
 
 static struct gdsc ife_0_gdsc = {
@@ -2239,6 +2242,7 @@ static struct gdsc ife_0_gdsc = {
 	},
 	.flags = POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
+	.supply = "mmcx",
 };
 
 static struct gdsc ife_1_gdsc = {
@@ -2248,6 +2252,7 @@ static struct gdsc ife_1_gdsc = {
 	},
 	.flags = POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
+	.supply = "mmcx",
 };
 
 static struct gdsc titan_top_gdsc = {
@@ -2257,6 +2262,7 @@ static struct gdsc titan_top_gdsc = {
 	},
 	.flags = POLL_CFG_GDSCR,
 	.pwrsts = PWRSTS_OFF_ON,
+	.supply = "mmcx",
 };
 
 static struct clk_regmap *cam_cc_sm8250_clocks[] = {
-- 
2.30.1

[PATCH 2/2] arm64: dts: qcom: sm8250: Add camcc DT node

[Bryan O'Donoghue]

Add the camcc DT node for the Camera Clock Controller on sm8250.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 4c0de12aaba6..7ac6ae50779c 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -18,6 +18,7 @@
 #include <dt-bindings/sound/qcom,q6afe.h>
 #include <dt-bindings/thermal/thermal.h>
 #include <dt-bindings/clock/qcom,videocc-sm8250.h>
+#include <dt-bindings/clock/qcom,camcc-sm8250.h>
 
 / {
 	interrupt-parent = <&intc>;
@@ -2369,6 +2370,19 @@ videocc: clock-controller@abf0000 {
 			#power-domain-cells = <1>;
 		};
 
+		clock_camcc: clock-controller@ad00000 {
+			compatible = "qcom,sm8250-camcc";
+			reg = <0 0x0ad00000 0 0x10000>;
+			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
+				 <&rpmhcc RPMH_CXO_CLK>,
+				 <&rpmhcc RPMH_CXO_CLK_A>;
+			clock-names = "iface", "bi_tcxo", "bi_tcxo_ao";
+			mmcx-supply = <&mmcx_reg>;
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
 		mdss: mdss@ae00000 {
 			compatible = "qcom,sdm845-mdss";
 			reg = <0 0x0ae00000 0 0x1000>;
-- 
2.30.1

Re: [PATCH 1/2] clk: qcom: camcc-sm8250: Fix absent mmcx regulator reference

[Bjorn Andersson]

On Wed 07 Jul 20:08 CDT 2021, Bryan O'Donoghue wrote:

> The current implementation omits the necessary mmcx supply, which means if
> you are running the code for this block and a prior clock driver, like say
> the videocc hasn't run, then a reset will be generated the first time we
> touch these registers.
> 
> Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

camcc isn't enabled in the upstream dts yet and I expect that we're
going to conclude on defining these GDSCs as subdomains of the cc's
power-domain in time for v5.15.

I don't mind if Stephen picks this to make sure the driver is
functional, but I will hold off on the dts change.

Regards,
Bjorn

> ---
>  drivers/clk/qcom/camcc-sm8250.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/clk/qcom/camcc-sm8250.c b/drivers/clk/qcom/camcc-sm8250.c
> index 439eaafdcc86..c51112546bfc 100644
> --- a/drivers/clk/qcom/camcc-sm8250.c
> +++ b/drivers/clk/qcom/camcc-sm8250.c
> @@ -2212,6 +2212,7 @@ static struct gdsc bps_gdsc = {
>  	},
>  	.flags = HW_CTRL | POLL_CFG_GDSCR,
>  	.pwrsts = PWRSTS_OFF_ON,
> +	.supply = "mmcx",
>  };
>  
>  static struct gdsc ipe_0_gdsc = {
> @@ -2221,6 +2222,7 @@ static struct gdsc ipe_0_gdsc = {
>  	},
>  	.flags = HW_CTRL | POLL_CFG_GDSCR,
>  	.pwrsts = PWRSTS_OFF_ON,
> +	.supply = "mmcx",
>  };
>  
>  static struct gdsc sbi_gdsc = {
> @@ -2230,6 +2232,7 @@ static struct gdsc sbi_gdsc = {
>  	},
>  	.flags = HW_CTRL | POLL_CFG_GDSCR,
>  	.pwrsts = PWRSTS_OFF_ON,
> +	.supply = "mmcx",
>  };
>  
>  static struct gdsc ife_0_gdsc = {
> @@ -2239,6 +2242,7 @@ static struct gdsc ife_0_gdsc = {
>  	},
>  	.flags = POLL_CFG_GDSCR,
>  	.pwrsts = PWRSTS_OFF_ON,
> +	.supply = "mmcx",
>  };
>  
>  static struct gdsc ife_1_gdsc = {
> @@ -2248,6 +2252,7 @@ static struct gdsc ife_1_gdsc = {
>  	},
>  	.flags = POLL_CFG_GDSCR,
>  	.pwrsts = PWRSTS_OFF_ON,
> +	.supply = "mmcx",
>  };
>  
>  static struct gdsc titan_top_gdsc = {
> @@ -2257,6 +2262,7 @@ static struct gdsc titan_top_gdsc = {
>  	},
>  	.flags = POLL_CFG_GDSCR,
>  	.pwrsts = PWRSTS_OFF_ON,
> +	.supply = "mmcx",
>  };
>  
>  static struct clk_regmap *cam_cc_sm8250_clocks[] = {
> -- 
> 2.30.1
> 

Re: [PATCH 1/2] clk: qcom: camcc-sm8250: Fix absent mmcx regulator reference

[Stephen Boyd]

Quoting Bjorn Andersson (2021-07-07 21:03:06)
> On Wed 07 Jul 20:08 CDT 2021, Bryan O'Donoghue wrote:
> 
> > The current implementation omits the necessary mmcx supply, which means if
> > you are running the code for this block and a prior clock driver, like say
> > the videocc hasn't run, then a reset will be generated the first time we
> > touch these registers.
> > 
> > Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250")
> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> camcc isn't enabled in the upstream dts yet and I expect that we're
> going to conclude on defining these GDSCs as subdomains of the cc's
> power-domain in time for v5.15.
> 
> I don't mind if Stephen picks this to make sure the driver is
> functional, but I will hold off on the dts change.
> 

Seems like this is superseded by the GDSC patches that use subdomains
instead of a fake supply?

Re: [PATCH 1/2] clk: qcom: camcc-sm8250: Fix absent mmcx regulator reference

[Bryan O'Donoghue]

On 27/07/2021 19:29, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-07-07 21:03:06)
>> On Wed 07 Jul 20:08 CDT 2021, Bryan O'Donoghue wrote:
>>
>>> The current implementation omits the necessary mmcx supply, which means if
>>> you are running the code for this block and a prior clock driver, like say
>>> the videocc hasn't run, then a reset will be generated the first time we
>>> touch these registers.
>>>
>>> Fixes: 5d66ca79b58c ("clk: qcom: Add camera clock controller driver for SM8250")
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>
>> camcc isn't enabled in the upstream dts yet and I expect that we're
>> going to conclude on defining these GDSCs as subdomains of the cc's
>> power-domain in time for v5.15.
>>
>> I don't mind if Stephen picks this to make sure the driver is
>> functional, but I will hold off on the dts change.
>>
> 
> Seems like this is superseded by the GDSC patches that use subdomains
> instead of a fake supply?
> 
yep