[3/8] dt-bindings: spi: Add Nuvoton WPCM450 Flash Interface Unit (FIU)

  The Flash Interface Unit (FIU) is the SPI flash controller in the
Nuvoton WPCM450 BMC SoC. It supports four chip selects, and direct
(memory-mapped) access to 16 MiB per chip. Larger flash chips can be
accessed by software-defined SPI transfers.

The FIU in newer NPCM7xx SoCs is not compatible with the WPCM450 FIU.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../bindings/spi/nuvoton,wpcm450-fiu.yaml     | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml

--
2.35.1
  

On 05/11/2022 19:59, Jonathan Neuschäfer wrote:
> The Flash Interface Unit (FIU) is the SPI flash controller in the
> Nuvoton WPCM450 BMC SoC. It supports four chip selects, and direct
> (memory-mapped) access to 16 MiB per chip. Larger flash chips can be
> accessed by software-defined SPI transfers.
> 
> The FIU in newer NPCM7xx SoCs is not compatible with the WPCM450 FIU.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  .../bindings/spi/nuvoton,wpcm450-fiu.yaml     | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml b/Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml
> new file mode 100644
> index 0000000000000..dc0ea2184f8d0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/nuvoton,wpcm450-fiu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton WPCM450 Flash Interface Unit (FIU)
> +
> +maintainers:
> +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> +
> +allOf:
> +  - $ref: "/schemas/spi/spi-controller.yaml#"

Drop the quotes.

> +
> +properties:
> +  compatible:
> +    const: nuvoton,wpcm450-fiu
> +
> +  reg:
> +    items:
> +      - description: FIU registers
> +      - description: Memory-mapped flash contents
> +
> +  reg-names:
> +    items:
> +      - const: control
> +      - const: memory
> +    minItems: 1

This does not match your 'reg'. Two items are required there.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  nuvoton,shm:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: a phandle to the SHM block (see ../arm/nuvoton,shm.yaml)
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
> +    spi@c8000000 {
> +      compatible = "nuvoton,wpcm450-fiu";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      reg = <0xc8000000 0x1000>, <0xc0000000 0x4000000>;

reg is the second property.

> +      reg-names = "control", "memory";
> +      clocks = <&clk WPCM450_CLK_FIU>;
> +      nuvoton,shm = <&shm>;
> +
> +      flash@0 {
> +        compatible = "jedec,spi-nor";
> +      };
> +    };
> +
> +    shm: syscon@c8001000 {
> +      compatible = "nuvoton,wpcm450-shm", "syscon";
> +      reg = <0xc8001000 0x1000>;
> +    };
> +
> +  - |
> +    #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
> +    spi@c8000000 {
> +      compatible = "nuvoton,wpcm450-fiu";
> +      // the "memory" resource may be omitted

This is rather obvious, so what you should comment is WHY or WHEN second
resource can be omitted.

Not every instance on the hardware has it?

Just to remind - this is the description of hardware, not Linux behavior.

> +      reg = <0xc8000000 0x1000>;
> +      reg-names = "control";
> +    };
> --
> 2.35.1
> 

Best regards,
Krzysztof
  

On Sat, 05 Nov 2022 19:59:06 +0100, Jonathan Neuschäfer wrote:
> The Flash Interface Unit (FIU) is the SPI flash controller in the
> Nuvoton WPCM450 BMC SoC. It supports four chip selects, and direct
> (memory-mapped) access to 16 MiB per chip. Larger flash chips can be
> accessed by software-defined SPI transfers.
> 
> The FIU in newer NPCM7xx SoCs is not compatible with the WPCM450 FIU.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  .../bindings/spi/nuvoton,wpcm450-fiu.yaml     | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml
> 

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:
Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.example.dts:18:18: fatal error: dt-bindings/clock/nuvoton,wpcm450-clk.h: No such file or directory
   18 |         #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.
  

On Sun, Nov 06, 2022 at 10:38:45AM +0100, Krzysztof Kozlowski wrote:
> On 05/11/2022 19:59, Jonathan Neuschäfer wrote:
> > The Flash Interface Unit (FIU) is the SPI flash controller in the
> > Nuvoton WPCM450 BMC SoC. It supports four chip selects, and direct
> > (memory-mapped) access to 16 MiB per chip. Larger flash chips can be
> > accessed by software-defined SPI transfers.
> > 
> > The FIU in newer NPCM7xx SoCs is not compatible with the WPCM450 FIU.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
[...]
> > +allOf:
> > +  - $ref: "/schemas/spi/spi-controller.yaml#"
> 
> Drop the quotes.

Will do.


> > +
> > +properties:
> > +  compatible:
> > +    const: nuvoton,wpcm450-fiu
> > +
> > +  reg:
> > +    items:
> > +      - description: FIU registers
> > +      - description: Memory-mapped flash contents
> > +
> > +  reg-names:
> > +    items:
> > +      - const: control
> > +      - const: memory
> > +    minItems: 1
> 
> This does not match your 'reg'. Two items are required there.

My intention was rather to make the second reg item actually optional,
i.e. add minItems: 1 for reg as well.  (But, further discussion below.)


> > +    spi@c8000000 {
> > +      compatible = "nuvoton,wpcm450-fiu";
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      reg = <0xc8000000 0x1000>, <0xc0000000 0x4000000>;
> 
> reg is the second property.

Ok, I'll move it up.

> 
> > +      reg-names = "control", "memory";
> > +      clocks = <&clk WPCM450_CLK_FIU>;
> > +      nuvoton,shm = <&shm>;
> > +
> > +      flash@0 {
> > +        compatible = "jedec,spi-nor";
> > +      };
> > +    };
> > +
> > +    shm: syscon@c8001000 {
> > +      compatible = "nuvoton,wpcm450-shm", "syscon";
> > +      reg = <0xc8001000 0x1000>;
> > +    };
> > +
> > +  - |
> > +    #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
> > +    spi@c8000000 {
> > +      compatible = "nuvoton,wpcm450-fiu";
> > +      // the "memory" resource may be omitted
> 
> This is rather obvious, so what you should comment is WHY or WHEN second
> resource can be omitted.

Ok, I'll add more reasoning, which is basically: The "memory" mapping is
only an optimization for faster access, knowledge of it is not necessary
for full operation of the device.

> Not every instance on the hardware has it?

AFAIK every instance has it, and there's unlikely to be any variation on
this fact anymore, because newer Nuvoton SoCs replaced the FIU with a
redesigned and incompatible version.

I admit that the value of making the "memory" mapping optional is rather
theoretical, and I'm open to making this reg item mandatory to simplify
the binding.

> Just to remind - this is the description of hardware, not Linux behavior.

Indeed.


Thanks for your review,
Jonathan
  

On 06/11/2022 15:45, Jonathan Neuschäfer wrote:
> On Sun, Nov 06, 2022 at 10:38:45AM +0100, Krzysztof Kozlowski wrote:
>> On 05/11/2022 19:59, Jonathan Neuschäfer wrote:
>>> The Flash Interface Unit (FIU) is the SPI flash controller in the
>>> Nuvoton WPCM450 BMC SoC. It supports four chip selects, and direct
>>> (memory-mapped) access to 16 MiB per chip. Larger flash chips can be
>>> accessed by software-defined SPI transfers.
>>>
>>> The FIU in newer NPCM7xx SoCs is not compatible with the WPCM450 FIU.
>>>
>>> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
>>> ---
> [...]
>>> +allOf:
>>> +  - $ref: "/schemas/spi/spi-controller.yaml#"
>>
>> Drop the quotes.
> 
> Will do.
> 
> 
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: nuvoton,wpcm450-fiu
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: FIU registers
>>> +      - description: Memory-mapped flash contents
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: control
>>> +      - const: memory
>>> +    minItems: 1
>>
>> This does not match your 'reg'. Two items are required there.
> 
> My intention was rather to make the second reg item actually optional,
> i.e. add minItems: 1 for reg as well.  (But, further discussion below.)
> 
> 
>>> +    spi@c8000000 {
>>> +      compatible = "nuvoton,wpcm450-fiu";
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +      reg = <0xc8000000 0x1000>, <0xc0000000 0x4000000>;
>>
>> reg is the second property.
> 
> Ok, I'll move it up.
> 
>>
>>> +      reg-names = "control", "memory";
>>> +      clocks = <&clk WPCM450_CLK_FIU>;
>>> +      nuvoton,shm = <&shm>;
>>> +
>>> +      flash@0 {
>>> +        compatible = "jedec,spi-nor";
>>> +      };
>>> +    };
>>> +
>>> +    shm: syscon@c8001000 {
>>> +      compatible = "nuvoton,wpcm450-shm", "syscon";
>>> +      reg = <0xc8001000 0x1000>;
>>> +    };
>>> +
>>> +  - |
>>> +    #include <dt-bindings/clock/nuvoton,wpcm450-clk.h>
>>> +    spi@c8000000 {
>>> +      compatible = "nuvoton,wpcm450-fiu";
>>> +      // the "memory" resource may be omitted
>>
>> This is rather obvious, so what you should comment is WHY or WHEN second
>> resource can be omitted.
> 
> Ok, I'll add more reasoning, which is basically: The "memory" mapping is
> only an optimization for faster access, knowledge of it is not necessary
> for full operation of the device.
> 
>> Not every instance on the hardware has it?
> 
> AFAIK every instance has it, and there's unlikely to be any variation on
> this fact anymore, because newer Nuvoton SoCs replaced the FIU with a
> redesigned and incompatible version.
> 
> I admit that the value of making the "memory" mapping optional is rather
> theoretical, and I'm open to making this reg item mandatory to simplify
> the binding.

If every instance has it, then regardless whether it is actually used or
not, just require second address?

Best regards,
Krzysztof
  

On Mon, Nov 07, 2022 at 09:09:15AM +0100, Krzysztof Kozlowski wrote:
> On 06/11/2022 15:45, Jonathan Neuschäfer wrote:
> > On Sun, Nov 06, 2022 at 10:38:45AM +0100, Krzysztof Kozlowski wrote:
[...]
> >> This is rather obvious, so what you should comment is WHY or WHEN second
> >> resource can be omitted.
> > 
> > Ok, I'll add more reasoning, which is basically: The "memory" mapping is
> > only an optimization for faster access, knowledge of it is not necessary
> > for full operation of the device.
> > 
> >> Not every instance on the hardware has it?
> > 
> > AFAIK every instance has it, and there's unlikely to be any variation on
> > this fact anymore, because newer Nuvoton SoCs replaced the FIU with a
> > redesigned and incompatible version.
> > 
> > I admit that the value of making the "memory" mapping optional is rather
> > theoretical, and I'm open to making this reg item mandatory to simplify
> > the binding.
> 
> If every instance has it, then regardless whether it is actually used or
> not, just require second address?

Alright, I'll do that.

Thanks,
Jonathan