[PATCH v3 03/17] spi: dt-bindings: cdns,qspi-nor: Add examples for testing the specific cases

Miquel Raynal (Schneider Electric) posted 17 patches 2 weeks, 5 days ago
There is a newer version of this series
[PATCH v3 03/17] spi: dt-bindings: cdns,qspi-nor: Add examples for testing the specific cases
Posted by Miquel Raynal (Schneider Electric) 2 weeks, 5 days ago
It is very painful to modify this file because the core IP described is
so common, it has been implemented in many SoCs from different
architectures. Both `dtbs_check` and `dt_binding_check` are rather long
commands, even when restricted to a single schema files, and letting
this file evolve without risking to break other DTSs is painful, because
there are arm, arm64 and riscv platforms impacted and no way to check
all of them at the same time.

Instead, we can identify the few specific cases which may need extra
testing, and fill the examples section to cover them all.

Add examples to cover the Starfive (resets) and Pensando (fifo-depth)
cases.

Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
---
 .../devicetree/bindings/spi/cdns,qspi-nor.yaml     | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index 123caef8f61e..62b97ab607f3 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -189,3 +189,38 @@ examples:
             cdns,tslch-ns = <60>;
         };
     };
+
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/reset/starfive,jh7110-crg.h>
+    #include <dt-bindings/clock/starfive,jh7110-crg.h>
+    spi@13010000 {
+        compatible = "starfive,jh7110-qspi", "cdns,qspi-nor";
+        reg = <0x13010000 0x10000>, <0x21000000 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";
+        #address-cells = <1>;
+        #size-cells = <0>;
+        cdns,fifo-depth = <256>;
+        cdns,fifo-width = <4>;
+        cdns,trigger-address = <0x0>;
+    };
+
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    spi@2400 {
+        compatible = "amd,pensando-elba-qspi", "cdns,qspi-nor";
+        reg = <0x2400 0x400>, <0x7fff0000 0x1000>;
+        interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&flash_clk>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        cdns,fifo-depth = <1024>;
+        cdns,fifo-width = <4>;
+        cdns,trigger-address = <0x7fff0000>;
+    };

-- 
2.51.1
Re: [PATCH v3 03/17] spi: dt-bindings: cdns,qspi-nor: Add examples for testing the specific cases
Posted by Rob Herring 2 weeks, 5 days ago
On Wed, Jan 21, 2026 at 06:04:59PM +0100, Miquel Raynal (Schneider Electric) wrote:
> It is very painful to modify this file because the core IP described is
> so common, it has been implemented in many SoCs from different
> architectures. Both `dtbs_check` and `dt_binding_check` are rather long
> commands, even when restricted to a single schema files, and letting
> this file evolve without risking to break other DTSs is painful, because
> there are arm, arm64 and riscv platforms impacted and no way to check
> all of them at the same time.

OTOH, examples aren't meant to be exhaustive test cases of all 
possibilities. If it was me, I'd actually just get rid of all the 
examples. They are generally just a copy from some .dts we already have.

> Instead, we can identify the few specific cases which may need extra
> testing, and fill the examples section to cover them all.
> 
> Add examples to cover the Starfive (resets) and Pensando (fifo-depth)
> cases.
> 
> Signed-off-by: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>
> ---
>  .../devicetree/bindings/spi/cdns,qspi-nor.yaml     | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> index 123caef8f61e..62b97ab607f3 100644
> --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> @@ -189,3 +189,38 @@ examples:
>              cdns,tslch-ns = <60>;
>          };
>      };
> +
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/reset/starfive,jh7110-crg.h>
> +    #include <dt-bindings/clock/starfive,jh7110-crg.h>
> +    spi@13010000 {
> +        compatible = "starfive,jh7110-qspi", "cdns,qspi-nor";
> +        reg = <0x13010000 0x10000>, <0x21000000 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";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        cdns,fifo-depth = <256>;
> +        cdns,fifo-width = <4>;
> +        cdns,trigger-address = <0x0>;
> +    };
> +
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    spi@2400 {
> +        compatible = "amd,pensando-elba-qspi", "cdns,qspi-nor";
> +        reg = <0x2400 0x400>, <0x7fff0000 0x1000>;
> +        interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&flash_clk>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        cdns,fifo-depth = <1024>;
> +        cdns,fifo-width = <4>;
> +        cdns,trigger-address = <0x7fff0000>;

This one really just looks like a subset of the others.

Rob
Re: [PATCH v3 03/17] spi: dt-bindings: cdns,qspi-nor: Add examples for testing the specific cases
Posted by Miquel Raynal 2 weeks, 4 days ago
Hi Rob,

On 21/01/2026 at 17:07:59 -06, Rob Herring <robh@kernel.org> wrote:

> On Wed, Jan 21, 2026 at 06:04:59PM +0100, Miquel Raynal (Schneider Electric) wrote:
>> It is very painful to modify this file because the core IP described is
>> so common, it has been implemented in many SoCs from different
>> architectures. Both `dtbs_check` and `dt_binding_check` are rather long
>> commands, even when restricted to a single schema files, and letting
>> this file evolve without risking to break other DTSs is painful, because
>> there are arm, arm64 and riscv platforms impacted and no way to check
>> all of them at the same time.
>
> OTOH, examples aren't meant to be exhaustive test cases of all 
> possibilities. If it was me, I'd actually just get rid of all the 
> examples. They are generally just a copy from some .dts we already
> have.

I will align with this idea the day `make dtbs_check` (or something
similarly simple) is exhaustive and cross platform :-)

Maybe cdns,qspi-nor is an exception, but it impacts different
architectures, which means the output of `make dtbs_check` is
meaningless because it only covers a subset of the possible cases. Hence
my attempt to gather all specific cases in the bindings, so I could run
all the meaningful checks I wanted more easily.

I think this patch has its usefulness, but I don't mind dropping it.

>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/reset/starfive,jh7110-crg.h>
>> +    #include <dt-bindings/clock/starfive,jh7110-crg.h>
>> +    spi@13010000 {
>> +        compatible = "starfive,jh7110-qspi", "cdns,qspi-nor";
>> +        reg = <0x13010000 0x10000>, <0x21000000 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";
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        cdns,fifo-depth = <256>;
>> +        cdns,fifo-width = <4>;
>> +        cdns,trigger-address = <0x0>;
>> +    };
>> +
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    spi@2400 {
>> +        compatible = "amd,pensando-elba-qspi", "cdns,qspi-nor";
>> +        reg = <0x2400 0x400>, <0x7fff0000 0x1000>;
>> +        interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
>> +        clocks = <&flash_clk>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        cdns,fifo-depth = <1024>;
>> +        cdns,fifo-width = <4>;
>> +        cdns,trigger-address = <0x7fff0000>;
>
> This one really just looks like a subset of the others.

The fifo-depth possibilities are extended just for this
compatible. Basically I captured in the examples every specific case
covered with an 'if' schema.

Miquèl
Re: [PATCH v3 03/17] spi: dt-bindings: cdns,qspi-nor: Add examples for testing the specific cases
Posted by Rob Herring 2 weeks, 4 days ago
On Thu, Jan 22, 2026 at 8:35 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Rob,
>
> On 21/01/2026 at 17:07:59 -06, Rob Herring <robh@kernel.org> wrote:
>
> > On Wed, Jan 21, 2026 at 06:04:59PM +0100, Miquel Raynal (Schneider Electric) wrote:
> >> It is very painful to modify this file because the core IP described is
> >> so common, it has been implemented in many SoCs from different
> >> architectures. Both `dtbs_check` and `dt_binding_check` are rather long
> >> commands, even when restricted to a single schema files, and letting
> >> this file evolve without risking to break other DTSs is painful, because
> >> there are arm, arm64 and riscv platforms impacted and no way to check
> >> all of them at the same time.
> >
> > OTOH, examples aren't meant to be exhaustive test cases of all
> > possibilities. If it was me, I'd actually just get rid of all the
> > examples. They are generally just a copy from some .dts we already
> > have.
>
> I will align with this idea the day `make dtbs_check` (or something
> similarly simple) is exhaustive and cross platform :-)

Note that you should be able to build DTBs without $ARCH
cross-compiler. So it's less trouble than building kernels for each
arch.

> Maybe cdns,qspi-nor is an exception, but it impacts different
> architectures, which means the output of `make dtbs_check` is
> meaningless because it only covers a subset of the possible cases. Hence
> my attempt to gather all specific cases in the bindings, so I could run
> all the meaningful checks I wanted more easily.

I used to run this on all patches, but with existing warnings it was
too noisy. Once we get arm64 warning free, I can look at doing that
again. We're at about 300 unique warnings left and over half of those
are 3 platform families.

> I think this patch has its usefulness, but I don't mind dropping it.

I'm okay with adding the first example.

> >> +  - |
> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +    #include <dt-bindings/reset/starfive,jh7110-crg.h>
> >> +    #include <dt-bindings/clock/starfive,jh7110-crg.h>
> >> +    spi@13010000 {
> >> +        compatible = "starfive,jh7110-qspi", "cdns,qspi-nor";
> >> +        reg = <0x13010000 0x10000>, <0x21000000 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";
> >> +        #address-cells = <1>;
> >> +        #size-cells = <0>;
> >> +        cdns,fifo-depth = <256>;
> >> +        cdns,fifo-width = <4>;
> >> +        cdns,trigger-address = <0x0>;
> >> +    };
> >> +
> >> +  - |
> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +    spi@2400 {
> >> +        compatible = "amd,pensando-elba-qspi", "cdns,qspi-nor";
> >> +        reg = <0x2400 0x400>, <0x7fff0000 0x1000>;
> >> +        interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> >> +        clocks = <&flash_clk>;
> >> +        #address-cells = <1>;
> >> +        #size-cells = <0>;
> >> +        cdns,fifo-depth = <1024>;
> >> +        cdns,fifo-width = <4>;
> >> +        cdns,trigger-address = <0x7fff0000>;
> >
> > This one really just looks like a subset of the others.
>
> The fifo-depth possibilities are extended just for this
> compatible. Basically I captured in the examples every specific case
> covered with an 'if' schema.

I get that, but with that argument we should have an example for every
if schema.

Rob