[PATCH 0/3] firmware: qcom: scm: Improve clock handling

[PATCH 0/3] firmware: qcom: scm: Improve clock handling

[Bjorn Andersson]

We're currently facing the issue that every new platform requires the addition
of a compatible to the DT binding as well as the driver.

The DT binding patch to allow us to use qcom,scm for all these new platforms
that doesn't require any clocks and the driver is reworked to make the qcom,scm
still pick up specified clocks in this case but won't require them.

This makes it possible to add new platforms by simply add the new compatible to
the list in the DT binding, but no changes needs to be done in the driver.
Which is what is done in patch 3.

Bjorn Andersson (3):
  dt-bindings: firmware: scm: Refactor compatibles and clocks
  firmware: qcom: scm: Refactor clock handling
  dt-bindings: firmware: scm: Add MSM8998 and SDM845

 .../devicetree/bindings/firmware/qcom,scm.txt | 33 ++++++---
 drivers/firmware/qcom_scm.c                   | 74 +++++++++++--------
 2 files changed, 63 insertions(+), 44 deletions(-)

-- 
2.18.0

[PATCH 1/3] dt-bindings: firmware: scm: Refactor compatibles and clocks

[Bjorn Andersson]

When the binding was written all "future" platforms required three
clocks, so the default compatible (qcom,scm) was defined to require
this. But as history shows all "future" platforms actually lack required
clocks. Given how the binding is written these compatibles have to be
added as an exception to the default.

Refactor the description of compatible to define that a platform
compatible should be given, followed by the fallback of qcom,scm. Also
refactor the description of the clocks in a way that this does not need
to be updated as new platform specific compatibles are added.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 .../devicetree/bindings/firmware/qcom,scm.txt | 31 ++++++++++++-------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
index fcf6979c0b6d..1c8e24530f7c 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.txt
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -7,16 +7,21 @@ assorted actions.
 
 Required properties:
 - compatible: must contain one of the following:
- * "qcom,scm-apq8064" for APQ8064 platforms
- * "qcom,scm-msm8660" for MSM8660 platforms
- * "qcom,scm-msm8690" for MSM8690 platforms
- * "qcom,scm-msm8996" for MSM8996 platforms
- * "qcom,scm-ipq4019" for IPQ4019 platforms
- * "qcom,scm" for later processors (MSM8916, APQ8084, MSM8974, etc)
-- clocks: One to three clocks may be required based on compatible.
- * No clock required for "qcom,scm-msm8996", "qcom,scm-ipq4019"
- * Only core clock required for "qcom,scm-apq8064", "qcom,scm-msm8660", and "qcom,scm-msm8960"
- * Core, iface, and bus clocks required for "qcom,scm"
+ * "qcom,scm-apq8064"
+ * "qcom,scm-apq8084"
+ * "qcom,scm-msm8660"
+ * "qcom,scm-msm8916"
+ * "qcom,scm-msm8960"
+ * "qcom,scm-msm8974"
+ * "qcom,scm-msm8996"
+ * "qcom,scm-ipq4019"
+ and:
+ * "qcom,scm"
+- clocks: Specifies clocks needed by the SCM interface, if any:
+ * core clock required for "qcom,scm-apq8064", "qcom,scm-msm8660" and
+   "qcom,scm-msm8960"
+ * core, iface and bus clocks required for "qcom,scm-apq8084",
+   "qcom,scm-msm8916" and "qcom,scm-msm8974"
 - clock-names: Must contain "core" for the core clock, "iface" for the interface
   clock and "bus" for the bus clock per the requirements of the compatible.
 - qcom,dload-mode: phandle to the TCSR hardware block and offset of the
@@ -26,8 +31,10 @@ Example for MSM8916:
 
 	firmware {
 		scm {
-			compatible = "qcom,scm";
-			clocks = <&gcc GCC_CRYPTO_CLK> , <&gcc GCC_CRYPTO_AXI_CLK>, <&gcc GCC_CRYPTO_AHB_CLK>;
+			compatible = "qcom,msm8916", "qcom,scm";
+			clocks = <&gcc GCC_CRYPTO_CLK> ,
+				 <&gcc GCC_CRYPTO_AXI_CLK>,
+				 <&gcc GCC_CRYPTO_AHB_CLK>;
 			clock-names = "core", "bus", "iface";
 		};
 	};
-- 
2.18.0

Re: [PATCH 1/3] dt-bindings: firmware: scm: Refactor compatibles and clocks

[Stephen Boyd]

Quoting Bjorn Andersson (2018-08-29 16:15:03)
> When the binding was written all "future" platforms required three
> clocks, so the default compatible (qcom,scm) was defined to require
> this. But as history shows all "future" platforms actually lack required
> clocks. Given how the binding is written these compatibles have to be
> added as an exception to the default.
> 
> Refactor the description of compatible to define that a platform
> compatible should be given, followed by the fallback of qcom,scm. Also
> refactor the description of the clocks in a way that this does not need
> to be updated as new platform specific compatibles are added.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 2/3] firmware: qcom: scm: Refactor clock handling

[Bjorn Andersson]

At one point in time all "future" platforms required three clocks, so
the binding and driver was written to treat this as the default case.
But new platforms has no clock requirements, which currently makes them
all a special case, causing the need for a patch in the binding and
driver for each new platform added.

This patch reworks the driver logic so that it will attempt to acquire
all three clocks and fail based on the given compatible. This allow us
to drop the clock requirement from "qcom,scm", in a way that will remain
backwards compatible with existing DT files.

Specific compatibles are added for apq8084, msm8916 and msm8974 to match
the updated binding and although equivalent to qcom,scm both ipq4019 and
msm8996 are kept as these have been used without fallback to qcom,scm.

The result of this patch is that new platforms, that require no clocks,
can be use the fallback compatible of "qcom,scm".

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/firmware/qcom_scm.c | 74 +++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 32 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index e778af766fae..af4eee86919d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -525,34 +525,44 @@ static int qcom_scm_probe(struct platform_device *pdev)
 		return ret;
 
 	clks = (unsigned long)of_device_get_match_data(&pdev->dev);
-	if (clks & SCM_HAS_CORE_CLK) {
-		scm->core_clk = devm_clk_get(&pdev->dev, "core");
-		if (IS_ERR(scm->core_clk)) {
-			if (PTR_ERR(scm->core_clk) != -EPROBE_DEFER)
-				dev_err(&pdev->dev,
-					"failed to acquire core clk\n");
+
+	scm->core_clk = devm_clk_get(&pdev->dev, "core");
+	if (IS_ERR(scm->core_clk)) {
+		if (PTR_ERR(scm->core_clk) == -EPROBE_DEFER)
+			return PTR_ERR(scm->core_clk);
+
+		if (clks & SCM_HAS_CORE_CLK) {
+			dev_err(&pdev->dev, "failed to acquire core clk\n");
 			return PTR_ERR(scm->core_clk);
 		}
+
+		scm->core_clk = NULL;
 	}
 
-	if (clks & SCM_HAS_IFACE_CLK) {
-		scm->iface_clk = devm_clk_get(&pdev->dev, "iface");
-		if (IS_ERR(scm->iface_clk)) {
-			if (PTR_ERR(scm->iface_clk) != -EPROBE_DEFER)
-				dev_err(&pdev->dev,
-					"failed to acquire iface clk\n");
+	scm->iface_clk = devm_clk_get(&pdev->dev, "iface");
+	if (IS_ERR(scm->iface_clk)) {
+		if (PTR_ERR(scm->iface_clk) == -EPROBE_DEFER)
+			return PTR_ERR(scm->iface_clk);
+
+		if (clks & SCM_HAS_IFACE_CLK) {
+			dev_err(&pdev->dev, "failed to acquire iface clk\n");
 			return PTR_ERR(scm->iface_clk);
 		}
+
+		scm->iface_clk = NULL;
 	}
 
-	if (clks & SCM_HAS_BUS_CLK) {
-		scm->bus_clk = devm_clk_get(&pdev->dev, "bus");
-		if (IS_ERR(scm->bus_clk)) {
-			if (PTR_ERR(scm->bus_clk) != -EPROBE_DEFER)
-				dev_err(&pdev->dev,
-					"failed to acquire bus clk\n");
+	scm->bus_clk = devm_clk_get(&pdev->dev, "bus");
+	if (IS_ERR(scm->bus_clk)) {
+		if (PTR_ERR(scm->bus_clk) == -EPROBE_DEFER)
+			return PTR_ERR(scm->bus_clk);
+
+		if (clks & SCM_HAS_BUS_CLK) {
+			dev_err(&pdev->dev, "failed to acquire bus clk\n");
 			return PTR_ERR(scm->bus_clk);
 		}
+
+		scm->bus_clk = NULL;
 	}
 
 	scm->reset.ops = &qcom_scm_pas_reset_ops;
@@ -594,23 +604,23 @@ static const struct of_device_id qcom_scm_dt_match[] = {
 	{ .compatible = "qcom,scm-apq8064",
 	  /* FIXME: This should have .data = (void *) SCM_HAS_CORE_CLK */
 	},
-	{ .compatible = "qcom,scm-msm8660",
-	  .data = (void *) SCM_HAS_CORE_CLK,
-	},
-	{ .compatible = "qcom,scm-msm8960",
-	  .data = (void *) SCM_HAS_CORE_CLK,
-	},
-	{ .compatible = "qcom,scm-msm8996",
-	  .data = NULL, /* no clocks */
+	{ .compatible = "qcom,scm-apq8084", .data = (void *)(SCM_HAS_CORE_CLK |
+							     SCM_HAS_IFACE_CLK |
+							     SCM_HAS_BUS_CLK)
 	},
-	{ .compatible = "qcom,scm-ipq4019",
-	  .data = NULL, /* no clocks */
+	{ .compatible = "qcom,scm-ipq4019" },
+	{ .compatible = "qcom,scm-msm8660", .data = (void *) SCM_HAS_CORE_CLK },
+	{ .compatible = "qcom,scm-msm8960", .data = (void *) SCM_HAS_CORE_CLK },
+	{ .compatible = "qcom,scm-msm8916", .data = (void *)(SCM_HAS_CORE_CLK |
+							     SCM_HAS_IFACE_CLK |
+							     SCM_HAS_BUS_CLK)
 	},
-	{ .compatible = "qcom,scm",
-	  .data = (void *)(SCM_HAS_CORE_CLK
-			   | SCM_HAS_IFACE_CLK
-			   | SCM_HAS_BUS_CLK),
+	{ .compatible = "qcom,scm-msm8974", .data = (void *)(SCM_HAS_CORE_CLK |
+							     SCM_HAS_IFACE_CLK |
+							     SCM_HAS_BUS_CLK)
 	},
+	{ .compatible = "qcom,scm-msm8996" },
+	{ .compatible = "qcom,scm" },
 	{}
 };
 
-- 
2.18.0

Re: [PATCH 2/3] firmware: qcom: scm: Refactor clock handling

[Stephen Boyd]

Quoting Bjorn Andersson (2018-08-29 16:15:04)
> At one point in time all "future" platforms required three clocks, so
> the binding and driver was written to treat this as the default case.
> But new platforms has no clock requirements, which currently makes them
> all a special case, causing the need for a patch in the binding and
> driver for each new platform added.
> 
> This patch reworks the driver logic so that it will attempt to acquire
> all three clocks and fail based on the given compatible. This allow us
> to drop the clock requirement from "qcom,scm", in a way that will remain
> backwards compatible with existing DT files.
> 
> Specific compatibles are added for apq8084, msm8916 and msm8974 to match
> the updated binding and although equivalent to qcom,scm both ipq4019 and
> msm8996 are kept as these have been used without fallback to qcom,scm.
> 
> The result of this patch is that new platforms, that require no clocks,
> can be use the fallback compatible of "qcom,scm".
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 3/3] dt-bindings: firmware: scm: Add MSM8998 and SDM845

[Bjorn Andersson]

Now that the compatible/clock handling is reworked add compatibles for
MSM8998 and SDM845 to the SCM binding.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 Documentation/devicetree/bindings/firmware/qcom,scm.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
index 1c8e24530f7c..41f133a4e2fa 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.txt
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -14,7 +14,9 @@ Required properties:
  * "qcom,scm-msm8960"
  * "qcom,scm-msm8974"
  * "qcom,scm-msm8996"
+ * "qcom,scm-msm8998"
  * "qcom,scm-ipq4019"
+ * "qcom,scm-sdm845"
  and:
  * "qcom,scm"
 - clocks: Specifies clocks needed by the SCM interface, if any:
-- 
2.18.0

Re: [PATCH 3/3] dt-bindings: firmware: scm: Add MSM8998 and SDM845

[Stephen Boyd]

Quoting Bjorn Andersson (2018-08-29 16:15:05)
> Now that the compatible/clock handling is reworked add compatibles for
> MSM8998 and SDM845 to the SCM binding.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

Re: [PATCH 0/3] firmware: qcom: scm: Improve clock handling

[Stephen Boyd]

Quoting Bjorn Andersson (2018-08-29 16:15:02)
> We're currently facing the issue that every new platform requires the addition
> of a compatible to the DT binding as well as the driver.
> 
> The DT binding patch to allow us to use qcom,scm for all these new platforms
> that doesn't require any clocks and the driver is reworked to make the qcom,scm
> still pick up specified clocks in this case but won't require them.
> 
> This makes it possible to add new platforms by simply add the new compatible to
> the list in the DT binding, but no changes needs to be done in the driver.
> Which is what is done in patch 3.
> 

I still think it may be even simpler to move to the "get all the clks"
API and then stop caring completely. Any reason to not do that?

Re: [PATCH 0/3] firmware: qcom: scm: Improve clock handling

[Bjorn Andersson]

On Thu 30 Aug 18:07 PDT 2018, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2018-08-29 16:15:02)
> > We're currently facing the issue that every new platform requires the addition
> > of a compatible to the DT binding as well as the driver.
> > 
> > The DT binding patch to allow us to use qcom,scm for all these new platforms
> > that doesn't require any clocks and the driver is reworked to make the qcom,scm
> > still pick up specified clocks in this case but won't require them.
> > 
> > This makes it possible to add new platforms by simply add the new compatible to
> > the list in the DT binding, but no changes needs to be done in the driver.
> > Which is what is done in patch 3.
> > 
> 
> I still think it may be even simpler to move to the "get all the clks"
> API and then stop caring completely. Any reason to not do that?
> 

I love working with drivers that tells me things like "hey you forgot to
add the 'core' clock", rather than having to spend hours bisecting which
register access it is that's causing that reboot and then try to drill
that down to a particular clock.

Apart from that, the "get-all-the-clocks" would work fine as well. Does
this API exist yet?

Regards,
Bjorn