Re: [PATCH 1/3] dt-bindings: spi: Add VIA/WonderMedia serial flash controller

[Alexey Charkov <alchark@gmail.com>]

On Wed, May 14, 2025 at 11:42 PM Rob Herring <robh@kernel.org> wrote:
>
> On Sat, May 10, 2025 at 11:42:21PM +0400, Alexey Charkov wrote:
> > Add a binding for the serial flash controller found on VIA/WonderMedia
> > SoCs, which provides semi-transparent access to SPI NOR chips by
> > mapping their contents to the physical CPU address space.
> >
> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > ---
> >  .../devicetree/bindings/spi/via,vt8500-sflash.yaml | 122 +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  2 files changed, 123 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/via,vt8500-sflash.yaml b/Documentation/devicetree/bindings/spi/via,vt8500-sflash.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..d2ea0dacdd56118c0cb5a1cb510ceb7591e1e5ca
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/via,vt8500-sflash.yaml
> > @@ -0,0 +1,122 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/spi/via,vt8500-sflash.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: VIA/WonderMedia serial flash controller
> > +
> > +description:
> > +  This controller is used on VIA/WonderMedia SoCs such as VIA VT8500,
> > +  WonderMedia WM8850 and similar. It provides a semi-transparent interface
> > +  for reading and writing SPI NOR chip contents via a physical memory map,
> > +  abstracting away all SPI communication, while also providing a direct
> > +  mechanism for issuing "programmable commands" to the underlying SPI chip
> > +
> > +maintainers:
> > +  - Alexey Charkov <alchark@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - via,vt8500-sflash
> > +      - wm,wm8505-sflash
> > +      - wm,wm8650-sflash
> > +      - wm,wm8750-sflash
> > +      - wm,wm8850-sflash
> > +
> > +  reg:
> > +    items:
> > +      - description: MMIO registers region of the controller
> > +      - description:
> > +          Physical memory region within which the controller will map the
> > +          flash contents of chip 0 for reading and writing. If the flash
> > +          size is smaller than this region, it will be mapped at its end.
> > +          Note that if this chip is used as the boot device (as is most
> > +          often the case), the boot ROM maps it at the very end of the
> > +          CPU address space (i.e. ending at 0xffffffff)
>
> Period needed on the end.

Noted, thank you! Will adjust in the next version.

> > +      - description:
> > +          Physical memory region within which the controller will map the
> > +          flash contents of chip 1 for reading and writing. If the flash
> > +          size is smaller than this region, it will be mapped at its end
>
> Period needed on the end.

Ditto.

> > +
> > +  reg-names:
> > +    items:
> > +      - const: io
> > +      - const: chip0-mmap
> > +      - const: chip1-mmap
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
>
> This follows the SPI binding, right? Drop these 2 and add a $ref to
> spi-controller.yaml.

Need some advice here. While this controller speaks SPI protocol to
its connected flash chips, it's a special-purpose thing that doesn't
expose much SPI functionality to the outside world, nor can it drive
any SPI devices other than SPI NOR flash. Does that still qualify as
an SPI controller as far as the bindings are concerned?

Happy to reference the spi-controller.yaml binding if so.

> > +patternProperties:
> > +  "^flash@[0-1]$":
> > +    type: object
> > +    additionalProperties: true
> > +
> > +    properties:
> > +      reg:
> > +        minimum: 0
> > +        maximum: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - clocks
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    sflash: spi-nor-controller@d8002000 {
>
> spi@...

Related to the question above... Happy to call it spi@ if appropriate
in this case.

> > +        compatible = "wm,wm8850-sflash";
> > +        reg = <0xd8002000 0x400>,
> > +              <0xff800000 0x800000>,
> > +              <0xef800000 0x800000>;
> > +        reg-names = "io", "chip0-mmap", "chip1-mmap";
> > +        clocks = <&clksf>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        flash@0 {
> > +            compatible = "jedec,spi-nor";
> > +            reg = <0>;
> > +
> > +            partitions {
> > +                compatible = "fixed-partitions";
> > +                #address-cells = <1>;
> > +                #size-cells = <1>;
> > +
> > +                partition@0 {
> > +                    label = "U-boot";
>
> The somewhat standard value here is 'u-boot'.

Noted, thanks - will adjust in the next version.

> > +                    reg = <0 0x50000>;
> > +                    read-only;
> > +                };
> > +
> > +                partition@1 {
> > +                    label = "U-boot environment 1";
>
> u-boot-env

Ditto.

> > +                    reg = <0x50000 0x10000>;
> > +                };
> > +
> > +                partition@2 {
> > +                    label = "U-boot environment 2";
>
> alt-u-boot-env or u-boot-env-backup?

Ditto.

Thanks for your review and comments Rob!

Best regards,
Alexey