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

[Witold Sadowski <wsadowski@xxxxxxxxxxx>]

> ----------------------------------------------------------------------
> 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?

It works without items, but I will try also with enums.

> 
> > +      - 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.

The intention is to add overlay on top of existing IP block to gain some
More features from it. So if there will be different SoC with same xSPI
IP, we can simply use that property, as internal SoC structure will be the same.
On the other hand, if there will be used different IP to handle SPI operations
It should use different driver. Also, I do not expect that new version of the
Overlay will be developed to handle different IP.

> 
> >    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.

Ok.

> 
> >
> >    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...

I will remove that PHY configuration block. That can be d in driver, based
on SoC version/HW overlay version.
I believe to change that values or some internal clock should be changed,
or whole internal structure, have to be changed. After few internal
discussions, I don't think only changing the SoC will be enough to change
that values.

> 
> > +    $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.

Ok.

> 
> 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
> >

Regards
Witek