Re: [PATCH v3 2/5] spi: cadence: Add MRVL overlay bindings documentation for Cadence XSPI

[Conor Dooley]

On Wed, Apr 17, 2024 at 06:13:49PM -0700, Witold Sadowski wrote:
> Add new bindings for v2 Marvell xSPI overlay:
> mrvl,xspi-nor  compatible string
> New compatible string to distinguish between orginal and modified xSPI
> block
> 
> PHY configuration registers
> Allow to change orginal xSPI PHY configuration values. If not set, and
> Marvell overlay is enabled, safe defaults will be written into xSPI PHY
> 
> Optional base for xfer register set
> Additional reg field to allocate xSPI Marvell overlay XFER block
> 
> Signed-off-by: Witold Sadowski <wsadowski@xxxxxxxxxxx>
> ---
>  .../devicetree/bindings/spi/cdns,xspi.yaml    | 92 ++++++++++++++++++-
>  1 file changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> index eb0f92468185..0e608245b136 100644
> --- a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> +++ b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> @@ -20,23 +20,82 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: cdns,xspi-nor
> +    oneOf:
> +      - description: Vanilla Cadence xSPI controller
> +        items:
> +          - const: cdns,xspi-nor

The "items: isn't required here is it? Can't you just have
    oneOf:
      - description: Vanilla Cadence xSPI controller
        const: cdns,xspi-nor
      - description: Cadence xSPI controller with v2 Marvell overlay
        const: mrvl,xspi-nor
if you don't want to use an enum?

> +      - description: Cadence xSPI controller with v2 Marvell overlay
> +        items:
> +          - const: mrvl,xspi-nor


"mrvl" is deprecated, please use "marvell". You're also missing a
soc-specific compatible here, I doubt there's only going to be one
device from marvell with an xspi controller ever.

>    reg:
> +    minItems: 3
>      items:
>        - description: address and length of the controller register set
>        - description: address and length of the Slave DMA data port
>        - description: address and length of the auxiliary registers
> +      - description: address and length of the xfer registers
>  
>    reg-names:
> +    minItems: 3
>      items:
>        - const: io
>        - const: sdma
>        - const: aux
> +      - const: xferbase

Please constrain the 4th reg to only the marvell device.

>  
>    interrupts:
>      maxItems: 1
>  
> +  cdns,dll-phy-control:
> +    description: |
> +      PHY config register. Valid only for cdns,mrvl-xspi-nor

Under what circumstances do you expect these things to change for a
particular SoC? If it's fixed per SoC, you could deduce it from the
compatible rather than needing properties.

None of these properties explain what they do and all appear to just
set register values directly, which is not generally something that we
permit in DT. Some explanation of how these values vary would help a
lot...

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x707
> +
> +  cdns,rfile-phy-control:
> +    description: |
> +      PHY config register. Valid only for cdns,mrvl-xspi-nor

Please enforce constraints like which compatibles something is valid for
in the binding, not in free-form text.


> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x40000
> +
> +  cdns,rfile-phy-tsel:
> +    description: |
> +      PHY config register. Valid only for cdns,mrvl-xspi-nor
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0
> +
> +  cdns,phy-dq-timing:
> +    description: |
> +      PHY config register. Valid only for cdns,mrvl-xspi-nor
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x101
> +
> +  cdns,phy-dqs-timing:
> +    description: |
> +      PHY config register. Valid only for cdns,mrvl-xspi-nor
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x700404
> +
> +  cdns,phy-gate-lpbk-ctrl:
> +    description: |
> +      PHY config register. Valid only for cdns,mrvl-xspi-nor
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x200030
> +
> +  cdns,phy-dll-master-ctrl:
> +    description: |
> +      PHY config register. Valid only for cdns,mrvl-xspi-nor
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x00800000
> +
> +  cdns,phy-dll-slave-ctrl:
> +    description: |
> +      PHY config register. Valid only for cdns,mrvl-xspi-nor
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0x0000ff01
> +
>  required:
>    - compatible
>    - reg
> @@ -68,6 +127,37 @@ examples:
>                  reg = <0>;
>              };
>  
> +            flash@1 {
> +                compatible = "jedec,spi-nor";
> +                spi-max-frequency = <75000000>;
> +                reg = <1>;
> +            };
> +        };
> +    };
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        mrvl_xspi: spi@d0010000 {

Drop the node label here, nothing ever refers to it.

Thanks,
Conor.

> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "mrvl,xspi-nor";
> +            reg = <0x0 0xa0010000 0x0 0x1040>,
> +                  <0x0 0xb0000000 0x0 0x1000>,
> +                  <0x0 0xa0020000 0x0 0x100>,
> +                  <0x0 0xa0090000 0x0 0x100>;
> +            reg-names = "io", "sdma", "aux", "xferbase";
> +            interrupts = <0 90 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-parent = <&gic>;
> +
> +            flash@0 {
> +                compatible = "jedec,spi-nor";
> +                spi-max-frequency = <75000000>;
> +                reg = <0>;
> +            };
> +
>              flash@1 {
>                  compatible = "jedec,spi-nor";
>                  spi-max-frequency = <75000000>;
> -- 
> 2.43.0
>

Attachment: signature.asc
Description: PGP signature