[PATCH v1 0/3] Add initialization of clock for StarFive JH7110 SoC

[PATCH v1 0/3] Add initialization of clock for StarFive JH7110 SoC

[William Qiu]

Hi,

This patchset adds initial rudimentary support for the StarFive
Quad SPI controller driver. And this driver will be used in
StarFive's VisionFive 2 board. In 6.4, the QSPI_AHB and QSPI_APB
clocks changed from the default ON state to the default OFF state,
so these clocks need to be enabled in the driver.At the same time,
dts patch is added to this series.

The patch series is based on v6.4rc3.

William Qiu (3):
  dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC
  spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI
  riscv: dts: starfive: Add QSPI controller node for StarFive JH7110 SoC

 .../bindings/spi/cdns,qspi-nor.yaml           | 15 +++++++--
 .../jh7110-starfive-visionfive-2.dtsi         | 32 +++++++++++++++++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      | 18 +++++++++++
 drivers/spi/spi-cadence-quadspi.c             | 27 ++++++++++++++++
 4 files changed, 89 insertions(+), 3 deletions(-)

--
2.34.1

[PATCH v1 1/3] dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC

[William Qiu]

The QSPI controller needs three clock items to work properly on StarFive
JH7110 SoC, so there is need to change the maxItems's value to 3. Other
platforms do not have this constraint.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../devicetree/bindings/spi/cdns,qspi-nor.yaml    | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index b310069762dd..737f1c162e01 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -26,6 +26,15 @@ allOf:
             const: starfive,jh7110-qspi
     then:
       properties:
+        clocks:
+          maxItems: 3
+
+        clock-names:
+          items:
+            - const: qspi-ref
+            - const: qspi-ahb
+            - const: qspi-apb
+
         resets:
           minItems: 2
           maxItems: 3
@@ -38,6 +47,9 @@ allOf:
 
     else:
       properties:
+        clocks:
+          maxItems: 1
+
         resets:
           maxItems: 2
 
@@ -69,9 +81,6 @@ properties:
   interrupts:
     maxItems: 1
 
-  clocks:
-    maxItems: 1
-
   cdns,fifo-depth:
     description:
       Size of the data FIFO in words.
-- 
2.34.1

[PATCH v1 2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

[William Qiu]

Add QSPI clock operation in device probe.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
---
 drivers/spi/spi-cadence-quadspi.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 6ddb2dfc0f00..c6430fb3a0a4 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1624,6 +1624,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
 static int cqspi_probe(struct platform_device *pdev)
 {
 	const struct cqspi_driver_platdata *ddata;
+	struct clk *qspi_ahb, *qspi_apb;
 	struct reset_control *rstc, *rstc_ocp, *rstc_ref;
 	struct device *dev = &pdev->dev;
 	struct spi_master *master;
@@ -1715,6 +1716,32 @@ static int cqspi_probe(struct platform_device *pdev)
 	}
 
 	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;
+		}
+
+		qspi_apb = devm_clk_get(dev, "qspi-apb");
+		if (IS_ERR(qspi_apb)) {
+			dev_err(dev, "Cannot claim QSPI_APB clock.\n");
+			ret = PTR_ERR(qspi_apb);
+			return ret;
+		}
+
+		ret = clk_prepare_enable(qspi_apb);
+		if (ret) {
+			dev_err(dev, "Cannot enable QSPI APB clock.\n");
+			goto probe_clk_failed;
+		}
+
 		rstc_ref = devm_reset_control_get_optional_exclusive(dev, "rstc_ref");
 		if (IS_ERR(rstc_ref)) {
 			ret = PTR_ERR(rstc_ref);
-- 
2.34.1

[PATCH v1 3/3] riscv: dts: starfive: Add QSPI controller node for StarFive JH7110 SoC

[William Qiu]

Add the quad spi controller node for the StarFive JH7110 SoC.

Co-developed-by: Ziv Xu <ziv.xu@starfivetech.com>
Signed-off-by: Ziv Xu <ziv.xu@starfivetech.com>
Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../jh7110-starfive-visionfive-2.dtsi         | 32 +++++++++++++++++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      | 18 +++++++++++
 2 files changed, 50 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index 2a6d81609284..22212c1150f9 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -126,6 +126,38 @@ &i2c6 {
 	status = "okay";
 };
 
+&qspi {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	nor_flash: flash@0 {
+		compatible = "jedec,spi-nor";
+		reg=<0>;
+		cdns,read-delay = <5>;
+		spi-max-frequency = <12000000>;
+		cdns,tshsl-ns = <1>;
+		cdns,tsd2d-ns = <1>;
+		cdns,tchsh-ns = <1>;
+		cdns,tslch-ns = <1>;
+
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			spl@0 {
+				reg = <0x0 0x20000>;
+			};
+			uboot@100000 {
+				reg = <0x100000 0x300000>;
+			};
+			data@f00000 {
+				reg = <0xf00000 0x100000>;
+			};
+		};
+	};
+};
+
 &sysgpio {
 	i2c0_pins: i2c0-0 {
 		i2c-pins {
diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 4c5fdb905da8..6385443d011f 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -440,6 +440,24 @@ i2c6: i2c@12060000 {
 			status = "disabled";
 		};
 
+		qspi: spi@13010000 {
+			compatible = "starfive,jh7110-qspi", "cdns,qspi-nor";
+			reg = <0x0 0x13010000 0x0 0x10000
+				0x0 0x21000000 0x0 0x400000>;
+			interrupts = <25>;
+			clocks = <&syscrg JH7110_SYSCLK_QSPI_REF>,
+				 <&syscrg JH7110_SYSCLK_QSPI_AHB>,
+				 <&syscrg JH7110_SYSCLK_QSPI_APB>;
+			clock-names = "qspi-ref", "qspi-ahb", "qspi-apb";
+			resets = <&syscrg JH7110_SYSRST_QSPI_APB>,
+				 <&syscrg JH7110_SYSRST_QSPI_AHB>,
+				 <&syscrg JH7110_SYSRST_QSPI_REF>;
+			reset-names = "qspi", "qspi-ocp", "rstc_ref";
+			cdns,fifo-depth = <256>;
+			cdns,fifo-width = <4>;
+			cdns,trigger-address = <0x0>;
+		};
+
 		syscrg: clock-controller@13020000 {
 			compatible = "starfive,jh7110-syscrg";
 			reg = <0x0 0x13020000 0x0 0x10000>;
-- 
2.34.1

Re: [PATCH v1 1/3] dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

On Fri, May 26, 2023 at 02:25:27PM +0800, William Qiu wrote:

>      then:
>        properties:
> +        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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On Fri, May 26, 2023 at 02:25:28PM +0800, William Qiu wrote:

>  	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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/3] dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC

[William Qiu]

On 2023/5/26 23:33, Mark Brown wrote:
> On Fri, May 26, 2023 at 02:25:27PM +0800, William Qiu wrote:
> 
>>      then:
>>        properties:
>> +        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.
Hi Mark,

	These three clocks are the internal clocks in the IP. The AHB
clock is the main system clock used to transfer data over the AHB bus
between an external master and the QSPI controller. The APB clock is used
to access the register map of the QSPI controller, perform controller and
device configuration.service interrupts and control certain run time modes.
The reference clock is used to serialize the data and drive the external
SPI interface.

	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?

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

Best regards,
William

Re: [PATCH v1 2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

[William Qiu]

On 2023/5/26 23:36, Mark Brown wrote:
> On Fri, May 26, 2023 at 02:25:28PM +0800, William Qiu wrote:
> 
>>  	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

Re: [PATCH v1 2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

[William Qiu]

On 2023/5/29 14:44, William Qiu wrote:
> 
> 
> On 2023/5/26 23:36, Mark Brown wrote:
>> On Fri, May 26, 2023 at 02:25:28PM +0800, William Qiu wrote:
>> 
>>>  	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

Hi Mark,

	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?

Best regards,
William

Re: [PATCH v1 1/3] dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

On Mon, May 29, 2023 at 02:44:13PM +0800, William Qiu wrote:
> On 2023/5/26 23:33, Mark Brown wrote:
> > On Fri, May 26, 2023 at 02:25:27PM +0800, William Qiu wrote:

> >> +        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?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

On Tue, May 30, 2023 at 10:05:38AM +0800, William Qiu wrote:
> On 2023/5/29 14:44, William Qiu wrote:
> > On 2023/5/26 23:36, Mark Brown wrote:

> >> 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.

> 	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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/3] dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC

[William Qiu]

On 2023/5/30 18:02, Mark Brown wrote:
> On Mon, May 29, 2023 at 02:44:13PM +0800, William Qiu wrote:
>> On 2023/5/26 23:33, Mark Brown wrote:
>> > On Fri, May 26, 2023 at 02:25:27PM +0800, William Qiu wrote:
> 
>> >> +        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?
> 
> That looks fine.  ref, ahb and apb would also be fine, it's just the
> qspi- prefix that I was querying.

That's fine. I will fix it in next version.

Best regards,
William

Re: [PATCH v1 2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

[William Qiu]

On 2023/5/30 18:33, Mark Brown wrote:
> On Tue, May 30, 2023 at 10:05:38AM +0800, William Qiu wrote:
>> On 2023/5/29 14:44, William Qiu wrote:
>> > On 2023/5/26 23:36, Mark Brown wrote:
> 
>> >> 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.
> 
>> 	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.
Hi Mark,

	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.
	My thoughts are as follows:
	Modify the following code

1658	/* Obtain QSPI clock. */
1659	cqspi->clk = devm_clk_get(dev, NULL);
1660	if (IS_ERR(cqspi->clk)) {
1661		dev_err(dev, "Cannot claim QSPI clock.\n");
1662		ret = PTR_ERR(cqspi->clk);
1663		return ret;
1664	}

	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.

Best Regards,
William

Re: [PATCH v1 2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Wed, May 31, 2023 at 02:19:16PM +0800, William Qiu wrote:
> On 2023/5/30 18:33, Mark Brown wrote:

> > 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.

> 	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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/3] spi: cadence-quadspi: Add clock configuration for StarFive JH7110 QSPI

[William Qiu]

On 2023/5/31 21:20, Mark Brown wrote:
> On Wed, May 31, 2023 at 02:19:16PM +0800, William Qiu wrote:
>> On 2023/5/30 18:33, Mark Brown wrote:
> 
>> > 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.
> 
>> 	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.
> 
> I'm not clear how enable and disable would then work?

enable use this API:
clk_bulk_prepare_enable(dev->num_clks, dev->clks);

then disable:
clk_bulk_disable_unprepare(dev->num_clks, dev->clks);

But I'll first try specify a different array of clocks depending on which
compatible the driver sees first.

Best regards,
William