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

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

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

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

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