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

[PATCH v3 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.

Changes v1->v2:
- Rebaed to v6.4rc6.
- Renamed the clock names.
- Changed the variable definition type.

Changes v1->v2:
- Renamed the clock names.
- Specified a different array of clocks.
- Used clk_bulk_ APIs.

The patch series is based on v6.4rc6.

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           | 20 +++++++++++-
 .../jh7110-starfive-visionfive-2.dtsi         | 32 +++++++++++++++++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      | 18 +++++++++++
 drivers/spi/spi-cadence-quadspi.c             | 20 ++++++++++++
 4 files changed, 89 insertions(+), 1 deletion(-)

--
2.34.1

[PATCH v3 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>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index b310069762dd..1b83cbb9a086 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: ref
+            - const: ahb
+            - const: apb
+
         resets:
           minItems: 2
           maxItems: 3
@@ -38,6 +47,9 @@ allOf:
 
     else:
       properties:
+        clocks:
+          maxItems: 1
+
         resets:
           maxItems: 2
 
@@ -70,7 +82,13 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: ref
+      - const: ahb
+      - const: apb
 
   cdns,fifo-depth:
     description:
-- 
2.34.1

[PATCH v3 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>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202306022017.UbwjjWRN-lkp@intel.com/
Reported-by: Julia Lawall <julia.lawall@inria.fr>
Closes: https://lore.kernel.org/r/202306040644.6ZHs55x4-lkp@intel.com/
---
 drivers/spi/spi-cadence-quadspi.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 6ddb2dfc0f00..8774f9aaff61 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -63,6 +63,8 @@ struct cqspi_st {
 	struct platform_device	*pdev;
 	struct spi_master	*master;
 	struct clk		*clk;
+	struct clk_bulk_data	*clks;
+	int			num_clks;
 	unsigned int		sclk;
 
 	void __iomem		*iobase;
@@ -1715,6 +1717,16 @@ static int cqspi_probe(struct platform_device *pdev)
 	}
 
 	if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi")) {
+		cqspi->num_clks = devm_clk_bulk_get_all(dev, &cqspi->clks);
+		if (cqspi->num_clks < 0) {
+			dev_err(dev, "Cannot claim clock: %u\n", cqspi->num_clks);
+			return -EINVAL;
+		}
+
+		ret = clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks);
+		if (ret)
+			dev_err(dev, "Cannot enable clock clks\n");
+
 		rstc_ref = devm_reset_control_get_optional_exclusive(dev, "rstc_ref");
 		if (IS_ERR(rstc_ref)) {
 			ret = PTR_ERR(rstc_ref);
@@ -1816,6 +1828,9 @@ static void cqspi_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(cqspi->clk);
 
+	if (of_device_is_compatible(pdev->dev.of_node, "starfive,jh7110-qspi"))
+		clk_bulk_disable_unprepare(cqspi->num_clks, cqspi->clks);
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 }
@@ -1831,6 +1846,9 @@ static int cqspi_suspend(struct device *dev)
 
 	clk_disable_unprepare(cqspi->clk);
 
+	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
+		clk_bulk_disable_unprepare(cqspi->num_clks, cqspi->clks);
+
 	return ret;
 }
 
@@ -1840,6 +1858,8 @@ static int cqspi_resume(struct device *dev)
 	struct spi_master *master = dev_get_drvdata(dev);
 
 	clk_prepare_enable(cqspi->clk);
+	if (of_device_is_compatible(dev->of_node, "starfive,jh7110-qspi"))
+		clk_bulk_prepare_enable(cqspi->num_clks, cqspi->clks);
 	cqspi_wait_idle(cqspi);
 	cqspi_controller_init(cqspi);
 
-- 
2.34.1

[PATCH v3 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..0b24f9e66e67 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 = "ref", "ahb", "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 v3 1/3] dt-bindings: qspi: cdns,qspi-nor: Add clocks for StarFive JH7110 SoC

[Rob Herring]

On Mon, 19 Jun 2023 16:35:15 +0800, William Qiu wrote:
> 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>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dtb: spi@ff705000: clocks: [[4294967295]] is too short
	from schema $id: http://devicetree.org/schemas/spi/cdns,qspi-nor.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230619083517.415597-2-william.qiu@starfivetech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Krzysztof Kozlowski]

On 19/06/2023 10:35, William Qiu wrote:
> 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>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> index b310069762dd..1b83cbb9a086 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: ref
> +            - const: ahb
> +            - const: apb

You are duplicating top-level property. Define the items only in one
place. If this list is applicable to everything, then in top-level property.

> +
>          resets:
>            minItems: 2
>            maxItems: 3
> @@ -38,6 +47,9 @@ allOf:
>  
>      else:
>        properties:
> +        clocks:
> +          maxItems: 1

clock-names is missing. They must be in sync with clocks. What is the
first clock?

> +
>          resets:
>            maxItems: 2
>  
> @@ -70,7 +82,13 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> +    maxItems: 3


You did not test it before sending. minItems is missing.

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


>  
>    cdns,fifo-depth:
>      description:

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 19/06/2023 10:35, William Qiu wrote:
> 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>;

Missing spaces.

> +		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..0b24f9e66e67 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>;

This should be two items so <>, <>. Not one item.

Best regards,
Krzysztof

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

[William Qiu]

On 2023/6/19 20:20, Krzysztof Kozlowski wrote:
> On 19/06/2023 10:35, William Qiu wrote:
>> 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>;
> 
> Missing spaces.
> 
Will fix.
>> +		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..0b24f9e66e67 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>;
> 
> This should be two items so <>, <>. Not one item.
> 
> Best regards,
> Krzysztof
> 
Will fix.
Thanks for taking time to review this patch series.

Best reagards
William

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

[William Qiu]

On 2023/6/19 17:16, Rob Herring wrote:
> 
> On Mon, 19 Jun 2023 16:35:15 +0800, William Qiu wrote:
>> 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>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>> 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/spi/cdns,qspi-nor.example.dtb: spi@ff705000: clocks: [[4294967295]] is too short
> 	from schema $id: http://devicetree.org/schemas/spi/cdns,qspi-nor.yaml#
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230619083517.415597-2-william.qiu@starfivetech.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 
It seems that my changes are not compatible with other dts files, I'll try to
fix it.

Thanks for reminding.

Best regards
William

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

[William Qiu]

On 2023/6/19 20:17, Krzysztof Kozlowski wrote:
> On 19/06/2023 10:35, William Qiu wrote:
>> 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>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>> index b310069762dd..1b83cbb9a086 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: ref
>> +            - const: ahb
>> +            - const: apb
> 
> You are duplicating top-level property. Define the items only in one
> place. If this list is applicable to everything, then in top-level property.
> 
Only in JH7110 SoC need there clocks, other platforms do not have this constraint.
So I need to duplicating top-level property.
>> +
>>          resets:
>>            minItems: 2
>>            maxItems: 3
>> @@ -38,6 +47,9 @@ allOf:
>>  
>>      else:
>>        properties:
>> +        clocks:
>> +          maxItems: 1
> 
> clock-names is missing. They must be in sync with clocks. What is the
> first clock?
> 
But there are no clock-names before, should I add it?
>> +
>>          resets:
>>            maxItems: 2
>>  
>> @@ -70,7 +82,13 @@ properties:
>>      maxItems: 1
>>  
>>    clocks:
>> -    maxItems: 1
>> +    maxItems: 3
> 
> 
> You did not test it before sending. minItems is missing.
> 
I will add it.
As for other platforms, should I use enum to constraint the clocks?
>> +
>> +  clock-names:
>> +    items:
>> +      - const: ref
>> +      - const: ahb
>> +      - const: apb
> 
> 
>>  
>>    cdns,fifo-depth:
>>      description:
> 
> Best regards,
> Krzysztof
> 
Thanks for taking time to review this patches series.

Best regards,
William

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

[Krzysztof Kozlowski]

On 21/06/2023 08:45, William Qiu wrote:
> 
> 
> On 2023/6/19 20:17, Krzysztof Kozlowski wrote:
>> On 19/06/2023 10:35, William Qiu wrote:
>>> 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>
>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>> ---
>>>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>>> index b310069762dd..1b83cbb9a086 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: ref
>>> +            - const: ahb
>>> +            - const: apb
>>
>> You are duplicating top-level property. Define the items only in one
>> place. If this list is applicable to everything, then in top-level property.
>>
> Only in JH7110 SoC need there clocks, other platforms do not have this constraint.
> So I need to duplicating top-level property.

You don't need, why? Why writing something twice is an answer to "JH7110
needs 3 clocks"? It's not related.

What is the clock for all other variants?

>>> +
>>>          resets:
>>>            minItems: 2
>>>            maxItems: 3
>>> @@ -38,6 +47,9 @@ allOf:
>>>  
>>>      else:
>>>        properties:
>>> +        clocks:
>>> +          maxItems: 1
>>
>> clock-names is missing. They must be in sync with clocks. What is the
>> first clock?
>>
> But there are no clock-names before, should I add it?

Then let's just disallow it. Either you define it or you not allow it.

>>> +
>>>          resets:
>>>            maxItems: 2
>>>  
>>> @@ -70,7 +82,13 @@ properties:
>>>      maxItems: 1
>>>  
>>>    clocks:
>>> -    maxItems: 1
>>> +    maxItems: 3
>>
>>
>> You did not test it before sending. minItems is missing.
>>
> I will add it.
> As for other platforms, should I use enum to constraint the clocks?

What is the clock on other platforms?

Best regards,
Krzysztof

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

[William Qiu]

On 2023/6/21 16:10, Krzysztof Kozlowski wrote:
> On 21/06/2023 08:45, William Qiu wrote:
>> 
>> 
>> On 2023/6/19 20:17, Krzysztof Kozlowski wrote:
>>> On 19/06/2023 10:35, William Qiu wrote:
>>>> 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>
>>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>>> ---
>>>>  .../bindings/spi/cdns,qspi-nor.yaml           | 20 ++++++++++++++++++-
>>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
>>>> index b310069762dd..1b83cbb9a086 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: ref
>>>> +            - const: ahb
>>>> +            - const: apb
>>>
>>> You are duplicating top-level property. Define the items only in one
>>> place. If this list is applicable to everything, then in top-level property.
>>>
>> Only in JH7110 SoC need there clocks, other platforms do not have this constraint.
>> So I need to duplicating top-level property.
> 
> You don't need, why? Why writing something twice is an answer to "JH7110
> needs 3 clocks"? It's not related.
> 
> What is the clock for all other variants?
> 
I'll try to not duplicating top-level property.
>>>> +
>>>>          resets:
>>>>            minItems: 2
>>>>            maxItems: 3
>>>> @@ -38,6 +47,9 @@ allOf:
>>>>  
>>>>      else:
>>>>        properties:
>>>> +        clocks:
>>>> +          maxItems: 1
>>>
>>> clock-names is missing. They must be in sync with clocks. What is the
>>> first clock?
>>>
>> But there are no clock-names before, should I add it?
> 
> Then let's just disallow it. Either you define it or you not allow it.
> 
Fine, I'll keep it disallow.
>>>> +
>>>>          resets:
>>>>            maxItems: 2
>>>>  
>>>> @@ -70,7 +82,13 @@ properties:
>>>>      maxItems: 1
>>>>  
>>>>    clocks:
>>>> -    maxItems: 1
>>>> +    maxItems: 3
>>>
>>>
>>> You did not test it before sending. minItems is missing.
>>>
>> I will add it.
>> As for other platforms, should I use enum to constraint the clocks?
> 
> What is the clock on other platforms?
> 
Other platforms have only one clock.
> Best regards,
> Krzysztof
> 
Thanks for taking time to review this patch series and give usefull
suggestions.

Best Regards,
William