lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CABjd4Yz3w75PtkRk_edzD5yf6b2xPuf20gopbm8ygddgCBfpkw@mail.gmail.com>
Date: Thu, 15 May 2025 22:50:03 +0300
From: Alexey Charkov <alchark@...il.com>
To: Rob Herring <robh@...nel.org>
Cc: Mark Brown <broonie@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Tudor Ambarus <tudor.ambarus@...aro.org>, 
	Pratyush Yadav <pratyush@...nel.org>, Michael Walle <mwalle@...nel.org>, 
	Miquel Raynal <miquel.raynal@...tlin.com>, Richard Weinberger <richard@....at>, 
	Vignesh Raghavendra <vigneshr@...com>, Krzysztof Kozlowski <krzk@...nel.org>, linux-spi@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-mtd@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/3] dt-bindings: spi: Add VIA/WonderMedia serial flash controller

On Wed, May 14, 2025 at 11:42 PM Rob Herring <robh@...nel.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@...il.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@...il.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@...02000 {
>
> 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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ