mbox series

[0/8] Nuvoton WPCM450 FIU SPI flash controller

Message ID 20221105185911.1547847-1-j.neuschaefer@gmx.net
Headers show
Series Nuvoton WPCM450 FIU SPI flash controller | expand

Message

Jonathan Neuschäfer Nov. 5, 2022, 6:59 p.m. UTC
This patchset adds DT bindings and a driver for the Flash Interface Unit
(FIU), 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 existing NPCM7xx FIU driver is sufficitently incompatible with the
WPCM450 FIU that I decided to write a new driver.

This patchset depends on the WPCM450 clock support patchset,

	[PATCH v5 0/6] Nuvoton WPCM450 clock and reset driver


Jonathan


Jonathan Neuschäfer (8):
  pinctrl: nuvoton: wpcm450: Refactor MFSEL setting code
  pinctrl: nuvoton: wpcm450: Fix handling of inverted MFSEL bits
  dt-bindings: spi: Add Nuvoton WPCM450 Flash Interface Unit (FIU)
  dt-bindings: mfd: syscon: Add nuvoton,wpcm450-shm
  ARM: dts: wpcm450: Add FIU SPI controller node
  ARM: dts: wpcm450-supermicro-x9sci-ln4f: Add SPI flash
  spi: wpcm-fiu: Add driver for Nuvoton WPCM450 Flash Interface Unit
    (FIU)
  spi: wpcm-fiu: Add direct map support

 .../devicetree/bindings/mfd/syscon.yaml       |   1 +
 .../bindings/spi/nuvoton,wpcm450-fiu.yaml     |  76 +++
 .../nuvoton-wpcm450-supermicro-x9sci-ln4f.dts |   9 +
 arch/arm/boot/dts/nuvoton-wpcm450.dtsi        |  16 +
 drivers/pinctrl/nuvoton/pinctrl-wpcm450.c     |  32 +-
 drivers/spi/Kconfig                           |  11 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-wpcm-fiu.c                    | 507 ++++++++++++++++++
 8 files changed, 644 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/nuvoton,wpcm450-fiu.yaml
 create mode 100644 drivers/spi/spi-wpcm-fiu.c

--
2.35.1

Comments

Krzysztof Kozlowski Nov. 6, 2022, 9:38 a.m. UTC | #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
Rob Herring Nov. 6, 2022, 1:27 p.m. UTC | #2
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.
Jonathan Neuschäfer Nov. 6, 2022, 2:45 p.m. UTC | #3
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
Krzysztof Kozlowski Nov. 7, 2022, 8:09 a.m. UTC | #4
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
Jonathan Neuschäfer Nov. 7, 2022, 12:12 p.m. UTC | #5
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
Linus Walleij Nov. 9, 2022, 8:41 a.m. UTC | #6
On Sat, Nov 5, 2022 at 7:59 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:

> Jonathan Neuschäfer (8):
>   pinctrl: nuvoton: wpcm450: Refactor MFSEL setting code
>   pinctrl: nuvoton: wpcm450: Fix handling of inverted MFSEL bits

I just applied these two patches to the pinctrl tree, it looks like they
can be applied independently of the others so I just did.

So no need to resend these or include me on subsequent patch
series.

Yours,
Linus Walleij