Add initialization of clock for StarFive JH7110 SoC

Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml allOf: · line 37

Quoted 11 lines · William Qiu

then:
properties:

Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml +9 -0

+        clocks:
+          maxItems: 3
+
+        clock-names:
+          items:
+            - const: qspi-ref
+            - const: qspi-ahb
+            - const: qspi-apb
+

Are these really the names that the clocks have in the IP? It seems weird that they'd include the IP name in there and not just be ref, ahb and apb.

Quoted 7 lines · Mark Brown

Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml allOf: · line 36

Quoted 5 lines · William Qiu

Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml +5 -0

+        clock-names:
+          items:
+            - const: qspi-ref
+            - const: qspi-ahb
+            - const: qspi-apb

I'm going to change the names of these three clocks to hclk, pclk,
and ref_clk, as defined in the data book. What do you think?

Quoted 8 lines · William Qiu

Quoted 7 lines · Mark Brown

Quoted 5 lines · William Qiu

Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml +5 -0

+        clock-names:
+          items:
+            - const: qspi-ref
+            - const: qspi-ahb
+            - const: qspi-apb

Quoted 2 lines

I'm going to change the names of these three clocks to hclk, pclk,
and ref_clk, as defined in the data book. What do you think?

That looks fine. ref, ahb and apb would also be fine, it's just the qspi- prefix that I was querying.

drivers/spi/spi-cadence-quadspi.c static int cqspi_probe(struct platform_device *pdev) · line 1730

Quoted 13 lines · William Qiu

drivers/spi/spi-cadence-quadspi.c +12 -0

 	if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) {
+		qspi_ahb = devm_clk_get(dev, "qspi-ahb");
+		if (IS_ERR(qspi_ahb)) {
+			dev_err(dev, "Cannot claim QSPI_AHB clock.\n");
+			ret = PTR_ERR(qspi_ahb);
+			return ret;
+		}
+
+		ret = clk_prepare_enable(qspi_ahb);
+		if (ret) {
+			dev_err(dev, "Cannot enable QSPI AHB clock.\n");
+			goto probe_clk_failed;
+		}

Nothing ever disables or unprepares this clock as far as I can tell? Perhaps also consider using the clk_bulk_ APIs.

Quoted 18 lines · Mark Brown

drivers/spi/spi-cadence-quadspi.c static int cqspi_probe(struct platform_device *pdev) · line 1730

Quoted 13 lines · William Qiu

drivers/spi/spi-cadence-quadspi.c +12 -0

 	if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) {
+		qspi_ahb = devm_clk_get(dev, "qspi-ahb");
+		if (IS_ERR(qspi_ahb)) {
+			dev_err(dev, "Cannot claim QSPI_AHB clock.\n");
+			ret = PTR_ERR(qspi_ahb);
+			return ret;
+		}
+
+		ret = clk_prepare_enable(qspi_ahb);
+		if (ret) {
+			dev_err(dev, "Cannot enable QSPI AHB clock.\n");
+			goto probe_clk_failed;
+		}

Nothing ever disables or unprepares this clock as far as I can tell? Perhaps also consider using the clk_bulk_ APIs.

I will add in next version.

Thanks for taking time to review this patch series and give useful suggestions.

Best regards,
William

Quoted 4 lines · William Qiu

Quoted 2 lines · Mark Brown

Nothing ever disables or unprepares this clock as far as I can tell? Perhaps also consider using the clk_bulk_ APIs.

Quoted 1 line

I will add in next version.

Now I want to replace the original devm_clk_get API in the
driver with devm_clk_bulk_get_all API, which can achieve compatibility, but it seems that it is not good for other ip with only one clock, so I want to ask about that can I replace it? Or define that inside jh7110?

Quoted 7 lines · William Qiu

Quoted 6 lines · William Qiu

Quoted 2 lines · Mark Brown

Nothing ever disables or unprepares this clock as far as I can tell? Perhaps also consider using the clk_bulk_ APIs.

I will add in next version.

Quoted 4 lines

Now I want to replace the original devm_clk_get API in the
driver with devm_clk_bulk_get_all API, which can achieve compatibility, but it seems that it is not good for other ip with only one clock, so I want to ask about that can I replace it? Or define that inside jh7110?

You could always specify a different array of clocks depending on which compatible the driver sees, just like you'd conditionally request clocks individually.

Quoted 3 lines · Mark Brown

You could always specify a different array of clocks depending on which compatible the driver sees, just like you'd conditionally request clocks individually.

If specify a different array of clocks depending on which compatible
the driver sees, since there will also be clock operations in the suspend and resume interfaces, this can make the code look complicated.

	as following:

	/* Obtain QSPI clock. */
	cqspi->num_clks = devm_clk_bulk_get_all(dev, &cqspi->clks);
	if (cqspi->num_clks < 0) {
		dev_err(dev, "Cannot claim QSPI clock: %u\n", cqspi->num_clks);
		return -EINVAL;
	}

	This way, the code will look simpler and clearer. How do you think

about it.

Quoted 9 lines · William Qiu

Quoted 3 lines · Mark Brown

You could always specify a different array of clocks depending on which compatible the driver sees, just like you'd conditionally request clocks individually.

If specify a different array of clocks depending on which compatible
the driver sees, since there will also be clock operations in the suspend and resume interfaces, this can make the code look complicated.

If you store the clock count and array in the driver data that should be fairly simple I think.

Quoted 8 lines

	as following:

	/* Obtain QSPI clock. */
	cqspi->num_clks = devm_clk_bulk_get_all(dev, &cqspi->clks);
	if (cqspi->num_clks < 0) {
		dev_err(dev, "Cannot claim QSPI clock: %u\n", cqspi->num_clks);
		return -EINVAL;
	}

Quoted 2 lines

This way, the code will look simpler and clearer. How do you think
about it.

I'm not clear how enable and disable would then work?