Add Global Clock controller (GCC) driver for SC7180

Update the init data name for each of the dynamic frequency switch controlled clock associated with the RCG clock name, so that it can be generated as per the hardware plan. Thus update the macro accordingly.

Signed-off-by Taniya Das <[email protected]>

---

Return NULL in the cases where the clk_hw is not registered with the clock provider, but the clock consumer still requests for a clock id.

Signed-off-by Taniya Das <[email protected]>

---

The GCC 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 <[email protected]>

---
 .../devicetree/bindings/clock/qcom,gcc.txt    |  94 ----------
 .../devicetree/bindings/clock/qcom,gcc.yaml   | 174 ++++++++++++++++++
 2 files changed, 174 insertions(+), 94 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc.yaml

The GCC 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 <[email protected]>

---

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

Signed-off-by Taniya Das <[email protected]>

---
 .../devicetree/bindings/clock/qcom,gcc.yaml   |  14 ++
 include/dt-bindings/clock/qcom,gcc-sc7180.h   | 155 ++++++++++++++++++
 2 files changed, 169 insertions(+)
 create mode 100644 include/dt-bindings/clock/qcom,gcc-sc7180.h

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

Signed-off-by Taniya Das <[email protected]>

---

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

---
 drivers/clk/qcom/Kconfig      |    9 +
 drivers/clk/qcom/Makefile     |    1 +
 drivers/clk/qcom/gcc-sc7180.c | 2450 +++++++++++++++++++++++++++++++++
 3 files changed, 2460 insertions(+)
 create mode 100644 drivers/clk/qcom/gcc-sc7180.c

drivers/clk/qcom/Kconfig +9 -0

index 32dbb4f09492..91706d88eeeb 100644
@@ -227,6 +227,15 @@ config QCS_GCC_404
 	  Say Y if you want to use multimedia devices or peripheral
 	  devices such as UART, SPI, I2C, USB, SD/eMMC, PCIe etc.
 
+config SC_GCC_7180
+	tristate "SC7180 Global Clock Controller"
+	select QCOM_GDSC
+	depends on COMMON_CLK_QCOM
+	help
+	  Support for the global clock controller on SC7180 devices.
+	  Say Y if you want to use peripheral devices such as UART, SPI,
+	  I2C, USB, UFS, SDCC, etc.
+
 config SDM_CAMCC_845
 	tristate "SDM845 Camera Clock Controller"
 	select SDM_GCC_845

drivers/clk/qcom/Makefile +1 -0

index 4a813b4055d0..6fb356a0bbf5 100644
@@ -43,6 +43,7 @@ obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
 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_SDM_CAMCC_845) += camcc-sdm845.o
 obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
 obj-$(CONFIG_SDM_GCC_660) += gcc-sdm660.o

drivers/clk/qcom/gcc-sc7180.c new file +10 -0

new file mode 100644
index 000000000000..38424e63bcae
 
+static struct clk_hw *gcc_sc7180_hws[] = {
+	[GCC_GPLL0_MAIN_DIV_CDIV] = &gcc_pll0_main_div_cdiv.hw,
+};
+
+static struct clk_regmap *gcc_sc7180_clocks[] = {
+	[GCC_AGGRE_UFS_PHY_AXI_CLK] = &gcc_aggre_ufs_phy_axi_clk.clkr,
+	[GCC_AGGRE_USB3_PRIM_AXI_CLK] = &gcc_aggre_usb3_prim_axi_clk.clkr,
+	[GCC_BOOT_ROM_AHB_CLK] = &gcc_boot_rom_ahb_clk.clkr,
+	[GCC_CAMERA_AHB_CLK] = &gcc_camera_ahb_clk.clkr,
+	[GCC_CAMERA_HF_AXI_CLK] = &gcc_camera_hf_axi_clk.clkr,

drivers/clk/qcom/gcc-sc7180.c new file +6 -0

+static int gcc_sc7180_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+	int ret;
+
+	regmap = qcom_cc_map(pdev, &gcc_sc7180_desc);

Hi Taniya,

Quoted 5 lines · Taniya Das

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

v3 also had

(quoted diff) +1 -0

+	[GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,

Removing it makes the dpu_mdss driver unhappy:

[    2.999855] dpu_mdss_enable+0x2c/0x58->msm_dss_enable_clk: 'iface' is not available

because:

         mdss: mdss@ae00000 {
     	        ...

=> clocks = <&gcc GCC_DISP_AHB_CLK>,

                          <&gcc GCC_DISP_HF_AXI_CLK>,
                          <&dispcc DISP_CC_MDSS_MDP_CLK>;
                 clock-names = "iface", "gcc_bus", "core";
	};

More clocks were removed in v4:

(quoted diff) +0 -3

-       [GCC_CPUSS_GNOC_CLK] = &gcc_cpuss_gnoc_clk.clkr,
-       [GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
-       [GCC_VIDEO_AHB_CLK] = &gcc_video_ahb_clk.clkr,

I guess this part of "remove registering the CRITICAL clocks to clock provider and leave them always ON from the GCC probe." (change log entry), but are you sure nobody is going to reference these clocks?

Quoted 6 lines

drivers/clk/qcom/gcc-sc7180.c new file +6 -0

+static int gcc_sc7180_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+	int ret;
+
+	regmap = qcom_cc_map(pdev, &gcc_sc7180_desc);

drivers/clk/qcom/gcc-sc7180.c line 2409

Quoted 10 lines

drivers/clk/qcom/gcc-sc7180.c new file +10 -0

+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	/*
+	 * Disable the GPLL0 active input to MM blocks, NPU
+	 * and GPU via MISC registers.
+	 */
+	regmap_update_bits(regmap, 0x09ffc, 0x3, 0x3);
+	regmap_update_bits(regmap, 0x4d110, 0x3, 0x3);
+	regmap_update_bits(regmap, 0x71028, 0x3, 0x3);

In v3 this was:

	regmap_update_bits(regmap, GCC_MMSS_MISC, 0x3, 0x3);
	regmap_update_bits(regmap, GCC_NPU_MISC, 0x3, 0x3);
	regmap_update_bits(regmap, GCC_GPU_MISC, 0x3, 0x3);

IMO register names seem preferable, why switch to literal addresses instead?

drivers/clk/qcom/gcc-sc7180.c line 2419

Quoted 10 lines

drivers/clk/qcom/gcc-sc7180.c new file +10 -0

+
+	/*
+	 * Keep the clocks always-ON
+	 * GCC_CPUSS_GNOC_CLK, GCC_VIDEO_AHB_CLK, GCC_DISP_AHB_CLK
+	 * GCC_GPU_CFG_AHB_CLK
+	 */
+	regmap_update_bits(regmap, 0x48004, BIT(0), BIT(0));
+	regmap_update_bits(regmap, 0x0b004, BIT(0), BIT(0));
+	regmap_update_bits(regmap, 0x0b00c, BIT(0), BIT(0));
+	regmap_update_bits(regmap, 0x71004, BIT(0), BIT(0));

ditto, register names seem preferable.

Hi Matthias,

Thanks for your comments.

Quoted 23 lines · Matthias Kaehlcke

Hi Taniya,

Quoted 5 lines · Taniya Das

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

v3 also had

(quoted diff) +1 -0

+	[GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,

Removing it makes the dpu_mdss driver unhappy:

[    2.999855] dpu_mdss_enable+0x2c/0x58->msm_dss_enable_clk: 'iface' is not available

because:

         mdss: mdss@ae00000 {
     	        ...

Quoted 6 lines

=> clocks = <&gcc GCC_DISP_AHB_CLK>,

                          <&gcc GCC_DISP_HF_AXI_CLK>,
                          <&dispcc DISP_CC_MDSS_MDP_CLK>;
                 clock-names = "iface", "gcc_bus", "core";
	};

The basic idea as you mentioned below was to move the CRITICAL clocks to probe. The clock provider to return NULL in case the clocks are not registered.
This was discussed with Stephen on v3. Thus I submitted the below patch.
clk: qcom: common: Return NULL from clk_hw OF provider.

Yes it would throw these warnings, but no functional issue is observed from display. I have tested it on the cheza board.

I guess we could fix the DRM driver to use the "devm_clk_get_optional()" instead?

Quoted 10 lines

More clocks were removed in v4:

(quoted diff) +0 -3

-       [GCC_CPUSS_GNOC_CLK] = &gcc_cpuss_gnoc_clk.clkr,
-       [GCC_GPU_CFG_AHB_CLK] = &gcc_gpu_cfg_ahb_clk.clkr,
-       [GCC_VIDEO_AHB_CLK] = &gcc_video_ahb_clk.clkr,

I guess this part of "remove registering the CRITICAL clocks to clock provider and leave them always ON from the GCC probe." (change log entry), but are you sure nobody is going to reference these clocks?

Even if they are referenced clk provider would return NULL.

Quoted 23 lines

Quoted 6 lines

drivers/clk/qcom/gcc-sc7180.c new file +6 -0

+static int gcc_sc7180_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+	int ret;
+
+	regmap = qcom_cc_map(pdev, &gcc_sc7180_desc);

drivers/clk/qcom/gcc-sc7180.c line 2409

Quoted 10 lines

drivers/clk/qcom/gcc-sc7180.c new file +10 -0

+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	/*
+	 * Disable the GPLL0 active input to MM blocks, NPU
+	 * and GPU via MISC registers.
+	 */
+	regmap_update_bits(regmap, 0x09ffc, 0x3, 0x3);
+	regmap_update_bits(regmap, 0x4d110, 0x3, 0x3);
+	regmap_update_bits(regmap, 0x71028, 0x3, 0x3);

In v3 this was:

	regmap_update_bits(regmap, GCC_MMSS_MISC, 0x3, 0x3);
	regmap_update_bits(regmap, GCC_NPU_MISC, 0x3, 0x3);
	regmap_update_bits(regmap, GCC_GPU_MISC, 0x3, 0x3);

Quoted 3 lines

IMO register names seem preferable, why switch to literal addresses instead?

:). These cleanups where done based on the comments I had received during SDM845 review. If Stephen is fine moving them to names, I could submit them in the next patch series.

Hi Taniya,

Quoted 35 lines · Taniya Das

Hi Matthias,

Thanks for your comments.

Quoted 20 lines · Matthias Kaehlcke

Hi Taniya,

Quoted 5 lines · Taniya Das

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

v3 also had

+ [GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,

Removing it makes the dpu_mdss driver unhappy:

[    2.999855] dpu_mdss_enable+0x2c/0x58->msm_dss_enable_clk: 'iface' is not available

because:

Quoted 9 lines

         mdss: mdss@ae00000 {
                    ...

  =>             clocks = <&gcc GCC_DISP_AHB_CLK>,
                          <&gcc GCC_DISP_HF_AXI_CLK>,
                          <&dispcc DISP_CC_MDSS_MDP_CLK>;
                 clock-names = "iface", "gcc_bus", "core";
    };

Quoted 5 lines

The basic idea as you mentioned below was to move the CRITICAL clocks to probe. The clock provider to return NULL in case the clocks are not registered.
This was discussed with Stephen on v3. Thus I submitted the below patch.
clk: qcom: common: Return NULL from clk_hw OF provider.

I see. My assumption was that the entire clock hierarchy should be registered, but Stephen almost certainly knows better :)

Quoted 2 lines

Yes it would throw these warnings, but no functional issue is observed from display. I have tested it on the cheza board.

The driver considers it an error (uses DEV_ERR to log the message) and doesn't handle other clocks when one is found missing. I'm not really famililar with the dpu_mdss driver, but I imagine this can have some side effects. Added some of the authors/contributors to cc.

Quoting Matthias Kaehlcke (2019-10-31 10:41:49)

Quoted 50 lines

Hi Taniya,

Quoted 35 lines · Taniya Das

Hi Matthias,

Thanks for your comments.

Quoted 20 lines · Matthias Kaehlcke

Hi Taniya,

Quoted 5 lines · Taniya Das

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

v3 also had

+ [GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,

Removing it makes the dpu_mdss driver unhappy:

[    2.999855] dpu_mdss_enable+0x2c/0x58->msm_dss_enable_clk: 'iface' is not available

because:

Quoted 9 lines

         mdss: mdss@ae00000 {
                    ...

  =>             clocks = <&gcc GCC_DISP_AHB_CLK>,
                          <&gcc GCC_DISP_HF_AXI_CLK>,
                          <&dispcc DISP_CC_MDSS_MDP_CLK>;
                 clock-names = "iface", "gcc_bus", "core";
    };

Quoted 5 lines

The basic idea as you mentioned below was to move the CRITICAL clocks to probe. The clock provider to return NULL in case the clocks are not registered.
This was discussed with Stephen on v3. Thus I submitted the below patch.
clk: qcom: common: Return NULL from clk_hw OF provider.

I see. My assumption was that the entire clock hierarchy should be registered, but Stephen almost certainly knows better :)

Quoted 2 lines

Yes it would throw these warnings, but no functional issue is observed from display. I have tested it on the cheza board.

Quoted 4 lines

The driver considers it an error (uses DEV_ERR to log the message) and doesn't handle other clocks when one is found missing. I'm not really famililar with the dpu_mdss driver, but I imagine this can have some side effects. Added some of the authors/contributors to cc.

NULL is a valid clk pointer returned by clk_get(). What is the display driver doing that makes it consider NULL an error?

Quoted 57 lines

Quoting Matthias Kaehlcke (2019-10-31 10:41:49)

Quoted 54 lines

Hi Taniya,

Quoted 35 lines · Taniya Das

Hi Matthias,

Thanks for your comments.

Quoted 20 lines · Matthias Kaehlcke

Hi Taniya,

Quoted 5 lines · Taniya Das

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

v3 also had

+ [GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,

Removing it makes the dpu_mdss driver unhappy:

[    2.999855] dpu_mdss_enable+0x2c/0x58->msm_dss_enable_clk: 'iface' is not available

because:

Quoted 9 lines

         mdss: mdss@ae00000 {
                    ...

  =>             clocks = <&gcc GCC_DISP_AHB_CLK>,
                          <&gcc GCC_DISP_HF_AXI_CLK>,
                          <&dispcc DISP_CC_MDSS_MDP_CLK>;
                 clock-names = "iface", "gcc_bus", "core";
    };

Quoted 5 lines

The basic idea as you mentioned below was to move the CRITICAL clocks to probe. The clock provider to return NULL in case the clocks are not registered.
This was discussed with Stephen on v3. Thus I submitted the below patch.
clk: qcom: common: Return NULL from clk_hw OF provider.

I see. My assumption was that the entire clock hierarchy should be registered, but Stephen almost certainly knows better :)

Quoted 2 lines

Yes it would throw these warnings, but no functional issue is observed from display. I have tested it on the cheza board.

The driver considers it an error (uses DEV_ERR to log the message) and doesn't handle other clocks when one is found missing. I'm not really famililar with the dpu_mdss driver, but I imagine this can have some side effects. Added some of the authors/contributors to cc.

Quoted 3 lines

NULL is a valid clk pointer returned by clk_get(). What is the display driver doing that makes it consider NULL an error?

do we not have an iface clk? I think the driver assumes we should have one, rather than it being an optional thing.. we could ofc change that

Quoting Rob Clark (2019-11-07 18:06:19)

Quoted 58 lines · Stephen Boyd

Quoted 57 lines

Quoting Matthias Kaehlcke (2019-10-31 10:41:49)

Quoted 54 lines

Hi Taniya,

Quoted 35 lines · Taniya Das

Hi Matthias,

Thanks for your comments.

Quoted 20 lines · Matthias Kaehlcke

Hi Taniya,

Quoted 5 lines · Taniya Das

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

v3 also had

+ [GCC_DISP_AHB_CLK] = &gcc_disp_ahb_clk.clkr,

Removing it makes the dpu_mdss driver unhappy:

[    2.999855] dpu_mdss_enable+0x2c/0x58->msm_dss_enable_clk: 'iface' is not available

because:

Quoted 9 lines

         mdss: mdss@ae00000 {
                    ...

  =>             clocks = <&gcc GCC_DISP_AHB_CLK>,
                          <&gcc GCC_DISP_HF_AXI_CLK>,
                          <&dispcc DISP_CC_MDSS_MDP_CLK>;
                 clock-names = "iface", "gcc_bus", "core";
    };

Quoted 5 lines

The basic idea as you mentioned below was to move the CRITICAL clocks to probe. The clock provider to return NULL in case the clocks are not registered.
This was discussed with Stephen on v3. Thus I submitted the below patch.
clk: qcom: common: Return NULL from clk_hw OF provider.

I see. My assumption was that the entire clock hierarchy should be registered, but Stephen almost certainly knows better :)

Quoted 2 lines

Yes it would throw these warnings, but no functional issue is observed from display. I have tested it on the cheza board.

The driver considers it an error (uses DEV_ERR to log the message) and doesn't handle other clocks when one is found missing. I'm not really famililar with the dpu_mdss driver, but I imagine this can have some side effects. Added some of the authors/contributors to cc.

Quoted 7 lines

Quoted 3 lines

NULL is a valid clk pointer returned by clk_get(). What is the display driver doing that makes it consider NULL an error?

do we not have an iface clk? I think the driver assumes we should have one, rather than it being an optional thing.. we could ofc change that

I think some sort of AHB clk is always enabled so the plan is to just hand back NULL to the caller when they call clk_get() on it and nobody should be the wiser when calling clk APIs with a NULL iface clk. The common clk APIs typically just return 0 and move along. Of course, we'll also turn the clk on in the clk driver so that hardware can function properly, but we don't need to expose it as a clk object and all that stuff if we're literally just slamming a bit somewhere and never looking back.

But it sounds like we can't return NULL for this clk for some reason? I haven't tried to track it down yet but I think Matthias has found it causes some sort of problem in the display driver.

Quoted 22 lines

Quoting Rob Clark (2019-11-07 18:06:19)

Quoted 10 lines · Stephen Boyd

Quoted 5 lines

NULL is a valid clk pointer returned by clk_get(). What is the display driver doing that makes it consider NULL an error?

do we not have an iface clk? I think the driver assumes we should have one, rather than it being an optional thing.. we could ofc change that

I think some sort of AHB clk is always enabled so the plan is to just hand back NULL to the caller when they call clk_get() on it and nobody should be the wiser when calling clk APIs with a NULL iface clk. The common clk APIs typically just return 0 and move along. Of course, we'll also turn the clk on in the clk driver so that hardware can function properly, but we don't need to expose it as a clk object and all that stuff if we're literally just slamming a bit somewhere and never looking back.

Quoted 4 lines

But it sounds like we can't return NULL for this clk for some reason? I haven't tried to track it down yet but I think Matthias has found it causes some sort of problem in the display driver.

ok, I guess we can change the dpu code to allow NULL.. but what would the return be, for example on a different SoC where we do have an iface clk, but the clk driver isn't enabled? Would that also return NULL? I guess it would be nice to differentiate between those cases..

Quoting Rob Clark (2019-11-08 08:54:23)

Quoted 28 lines · Stephen Boyd

Quoted 22 lines

Quoting Rob Clark (2019-11-07 18:06:19)

Quoted 10 lines · Stephen Boyd

Quoted 5 lines

NULL is a valid clk pointer returned by clk_get(). What is the display driver doing that makes it consider NULL an error?

do we not have an iface clk? I think the driver assumes we should have one, rather than it being an optional thing.. we could ofc change that

I think some sort of AHB clk is always enabled so the plan is to just hand back NULL to the caller when they call clk_get() on it and nobody should be the wiser when calling clk APIs with a NULL iface clk. The common clk APIs typically just return 0 and move along. Of course, we'll also turn the clk on in the clk driver so that hardware can function properly, but we don't need to expose it as a clk object and all that stuff if we're literally just slamming a bit somewhere and never looking back.

Quoted 4 lines

But it sounds like we can't return NULL for this clk for some reason? I haven't tried to track it down yet but I think Matthias has found it causes some sort of problem in the display driver.

Quoted 5 lines

ok, I guess we can change the dpu code to allow NULL.. but what would the return be, for example on a different SoC where we do have an iface clk, but the clk driver isn't enabled? Would that also return NULL? I guess it would be nice to differentiate between those cases..

So the scenario is DT describes the clk

 dpu_node {
     clocks = <&cc AHB_CLK>;
     clock-names = "iface";

}

but the &cc node has a driver that doesn't probe?

I believe in this scenario we return -EPROBE_DEFER because we assume we should wait for the clk driver to probe and provide the iface clk. See of_clk_get_hw_from_clkspec() and how it looks through a list of clk providers and tries to match the &cc phandle to some provider.

Once the driver probes, the match will happen and we'll be able to look up the clk in the provider with __of_clk_get_hw_from_provider(). If the clk provider decides that there isn't a clk object, it will return NULL and then eventually clk_hw_create_clk() will turn the NULL return value into a NULL pointer to return from clk_get().

Quoted 15 lines

So the scenario is DT describes the clk

 dpu_node {
     clocks = <&cc AHB_CLK>;
     clock-names = "iface";

}

but the &cc node has a driver that doesn't probe?

I believe in this scenario we return -EPROBE_DEFER because we assume we should wait for the clk driver to probe and provide the iface clk. See of_clk_get_hw_from_clkspec() and how it looks through a list of clk providers and tries to match the &cc phandle to some provider.

Quoted 6 lines

Once the driver probes, the match will happen and we'll be able to look up the clk in the provider with __of_clk_get_hw_from_provider(). If the clk provider decides that there isn't a clk object, it will return NULL and then eventually clk_hw_create_clk() will turn the NULL return value into a NULL pointer to return from clk_get().

ok, that was the scenario I was worried about (since unclk'd register access tends to be insta-reboot and hard to debug).. so I think it should be ok to make dpu just ignore NULL clks.

From a quick look, I think something like the attached (untested). (Sorry, I'd just paste it inline but gmail somehow eats all the whitespace when I do that :-/)

Quoted 50 lines · Stephen Boyd

Quoting Rob Clark (2019-11-08 08:54:23)

Quoted 28 lines · Stephen Boyd

Quoted 22 lines

Quoting Rob Clark (2019-11-07 18:06:19)

Quoted 10 lines · Stephen Boyd

Quoted 5 lines

NULL is a valid clk pointer returned by clk_get(). What is the display driver doing that makes it consider NULL an error?

do we not have an iface clk? I think the driver assumes we should have one, rather than it being an optional thing.. we could ofc change that

I think some sort of AHB clk is always enabled so the plan is to just hand back NULL to the caller when they call clk_get() on it and nobody should be the wiser when calling clk APIs with a NULL iface clk. The common clk APIs typically just return 0 and move along. Of course, we'll also turn the clk on in the clk driver so that hardware can function properly, but we don't need to expose it as a clk object and all that stuff if we're literally just slamming a bit somewhere and never looking back.

Quoted 4 lines

But it sounds like we can't return NULL for this clk for some reason? I haven't tried to track it down yet but I think Matthias has found it causes some sort of problem in the display driver.

Quoted 5 lines

ok, I guess we can change the dpu code to allow NULL.. but what would the return be, for example on a different SoC where we do have an iface clk, but the clk driver isn't enabled? Would that also return NULL? I guess it would be nice to differentiate between those cases..

So the scenario is DT describes the clk

 dpu_node {
     clocks = <&cc AHB_CLK>;
     clock-names = "iface";

}

but the &cc node has a driver that doesn't probe?

I believe in this scenario we return -EPROBE_DEFER because we assume we should wait for the clk driver to probe and provide the iface clk. See of_clk_get_hw_from_clkspec() and how it looks through a list of clk providers and tries to match the &cc phandle to some provider.

Quoted 6 lines

Once the driver probes, the match will happen and we'll be able to look up the clk in the provider with __of_clk_get_hw_from_provider(). If the clk provider decides that there isn't a clk object, it will return NULL and then eventually clk_hw_create_clk() will turn the NULL return value into a NULL pointer to return from clk_get().

ok, that was the scenario I was worried about (since unclk'd register access tends to be insta-reboot and hard to debug).. so I think it should be ok to make dpu just ignore NULL clks.

From a quick look, I think something like the attached (untested).

Quoted 58 lines · Rob Clark

Quoted 56 lines · Stephen Boyd

Quoting Rob Clark (2019-11-08 08:54:23)

Quoted 28 lines · Stephen Boyd

Quoted 22 lines

Quoting Rob Clark (2019-11-07 18:06:19)

Quoted 10 lines · Stephen Boyd

Quoted 5 lines

NULL is a valid clk pointer returned by clk_get(). What is the display driver doing that makes it consider NULL an error?

do we not have an iface clk? I think the driver assumes we should have one, rather than it being an optional thing.. we could ofc change that

I think some sort of AHB clk is always enabled so the plan is to just hand back NULL to the caller when they call clk_get() on it and nobody should be the wiser when calling clk APIs with a NULL iface clk. The common clk APIs typically just return 0 and move along. Of course, we'll also turn the clk on in the clk driver so that hardware can function properly, but we don't need to expose it as a clk object and all that stuff if we're literally just slamming a bit somewhere and never looking back.

Quoted 4 lines

But it sounds like we can't return NULL for this clk for some reason? I haven't tried to track it down yet but I think Matthias has found it causes some sort of problem in the display driver.

Quoted 5 lines

ok, I guess we can change the dpu code to allow NULL.. but what would the return be, for example on a different SoC where we do have an iface clk, but the clk driver isn't enabled? Would that also return NULL? I guess it would be nice to differentiate between those cases..

So the scenario is DT describes the clk

 dpu_node {
     clocks = <&cc AHB_CLK>;
     clock-names = "iface";

}

but the &cc node has a driver that doesn't probe?

I believe in this scenario we return -EPROBE_DEFER because we assume we should wait for the clk driver to probe and provide the iface clk. See of_clk_get_hw_from_clkspec() and how it looks through a list of clk providers and tries to match the &cc phandle to some provider.

Once the driver probes, the match will happen and we'll be able to look up the clk in the provider with __of_clk_get_hw_from_provider(). If the clk provider decides that there isn't a clk object, it will return NULL and then eventually clk_hw_create_clk() will turn the NULL return value into a NULL pointer to return from clk_get().

Quoted 5 lines

ok, that was the scenario I was worried about (since unclk'd register access tends to be insta-reboot and hard to debug).. so I think it should be ok to make dpu just ignore NULL clks.

From a quick look, I think something like the attached (untested).

The driver appears to be happy with it, at least at probe() time.

Quoted 64 lines · Matthias Kaehlcke

Quoted 62 lines · Rob Clark

Quoted 50 lines · Stephen Boyd

Quoting Rob Clark (2019-11-08 08:54:23)

Quoted 28 lines · Stephen Boyd

Quoted 22 lines

Quoting Rob Clark (2019-11-07 18:06:19)

Quoted 10 lines · Stephen Boyd

Quoted 5 lines

NULL is a valid clk pointer returned by clk_get(). What is the display driver doing that makes it consider NULL an error?

do we not have an iface clk? I think the driver assumes we should have one, rather than it being an optional thing.. we could ofc change that

I think some sort of AHB clk is always enabled so the plan is to just hand back NULL to the caller when they call clk_get() on it and nobody should be the wiser when calling clk APIs with a NULL iface clk. The common clk APIs typically just return 0 and move along. Of course, we'll also turn the clk on in the clk driver so that hardware can function properly, but we don't need to expose it as a clk object and all that stuff if we're literally just slamming a bit somewhere and never looking back.

Quoted 4 lines

But it sounds like we can't return NULL for this clk for some reason? I haven't tried to track it down yet but I think Matthias has found it causes some sort of problem in the display driver.

Quoted 5 lines

ok, I guess we can change the dpu code to allow NULL.. but what would the return be, for example on a different SoC where we do have an iface clk, but the clk driver isn't enabled? Would that also return NULL? I guess it would be nice to differentiate between those cases..

So the scenario is DT describes the clk

 dpu_node {
     clocks = <&cc AHB_CLK>;
     clock-names = "iface";

}

but the &cc node has a driver that doesn't probe?

I believe in this scenario we return -EPROBE_DEFER because we assume we should wait for the clk driver to probe and provide the iface clk. See of_clk_get_hw_from_clkspec() and how it looks through a list of clk providers and tries to match the &cc phandle to some provider.

Quoted 6 lines

Once the driver probes, the match will happen and we'll be able to look up the clk in the provider with __of_clk_get_hw_from_provider(). If the clk provider decides that there isn't a clk object, it will return NULL and then eventually clk_hw_create_clk() will turn the NULL return value into a NULL pointer to return from clk_get().

ok, that was the scenario I was worried about (since unclk'd register access tends to be insta-reboot and hard to debug).. so I think it should be ok to make dpu just ignore NULL clks.

Quoted 3 lines

Quoted 1 line

From a quick look, I think something like the attached (untested).

The driver appears to be happy with it, at least at probe() time.

Ok, I suppose I should re-send the dpu patch to the appropriate lists.. does that count as a Tested-by?

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

---

Add support for the global clock controller found on SC7180 based devices. This should allow most non-multimedia device drivers to probe and control their clocks.

Signed-off-by Taniya Das <[email protected]>

---
 drivers/clk/qcom/Kconfig      |    9 +
 drivers/clk/qcom/Makefile     |    1 +
 drivers/clk/qcom/gcc-sc7180.c | 2450 +++++++++++++++++++++++++++++++++
 3 files changed, 2460 insertions(+)
 create mode 100644 drivers/clk/qcom/gcc-sc7180.c

drivers/clk/qcom/Kconfig +9 -0

index 32dbb4f09492..91706d88eeeb 100644
@@ -227,6 +227,15 @@ config QCS_GCC_404
 	  Say Y if you want to use multimedia devices or peripheral
 	  devices such as UART, SPI, I2C, USB, SD/eMMC, PCIe etc.
 
+config SC_GCC_7180
+	tristate "SC7180 Global Clock Controller"
+	select QCOM_GDSC
+	depends on COMMON_CLK_QCOM
+	help
+	  Support for the global clock controller on SC7180 devices.
+	  Say Y if you want to use peripheral devices such as UART, SPI,
+	  I2C, USB, UFS, SDCC, etc.
+
 config SDM_CAMCC_845
 	tristate "SDM845 Camera Clock Controller"
 	select SDM_GCC_845

drivers/clk/qcom/Makefile +1 -0

index 4a813b4055d0..6fb356a0bbf5 100644
@@ -43,6 +43,7 @@ obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
 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_SDM_CAMCC_845) += camcc-sdm845.o
 obj-$(CONFIG_SDM_DISPCC_845) += dispcc-sdm845.o
 obj-$(CONFIG_SDM_GCC_660) += gcc-sdm660.o

drivers/clk/qcom/gcc-sc7180.c new file +0 -0

new file mode 100644
index 000000000000..38424e63bcae