Message-ID: 
 <CO6PR18MB40980502E0D9841C2B75E612B01B2@CO6PR18MB4098.namprd18.prod.outlook.com>
Date: Mon, 29 Apr 2024 14:35:48 +0000
From: Witold Sadowski <wsadowski@...vell.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC: "broonie@...nel.org" <broonie@...nel.org>,
        "robh@...nel.org"
	<robh@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org"
	<krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org"
	<conor+dt@...nel.org>,
        "pthombar@...ence.com" <pthombar@...ence.com>
Subject: RE: [EXTERNAL] Re: [PATCH v3 2/5] spi: cadence: Add MRVL overlay
 bindings documentation for Cadence XSPI

> ----------------------------------------------------------------------
> On 18/04/2024 03:13, 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
> >
> 
> Do not attach (thread) your patchsets to some other threads (unrelated or
> older versions). This buries them deep in the mailbox and might interfere
> with applying entire sets.
Ok.
> 
> > 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@...vell.com>
> > ---
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
> 
> You already received *exactly* the same comment. Can you respond to
> feedbacks and acknowledge that you will implement them?
> 
> 
> Please provide changelog and explain what happened in between. There were
> several comments already, so did you implement them? Were they ignored?
> 
> There was no single response from you.

Sorry for that. I will try to do better from now on.

> 
> >  .../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
> > +      - description: Cadence xSPI controller with v2 Marvell overlay
> > +        items:
> > +          - const: mrvl,xspi-nor
> > +
> >
> 
> No need for two blank lines. BTW, that's just enum.
Ok, will change that.
> 
> 
> >    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
> >
> >    interrupts:
> >      maxItems: 1
> >
> > +  cdns,dll-phy-control:
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
> > +      PHY config register. Valid only for cdns,mrvl-xspi-nor
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 0x707
> > +
> > +  cdns,rfile-phy-control:
> > +    description: |
> > +      PHY config register. Valid only for cdns,mrvl-xspi-nor
> > +    $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:
> 
> Please use some easier to read logical properties, not just register
> values. Specifically, this is impossible to review whether any of these
> are actually OS policy, instead of hardware configuration.
> 
> You also miss constraining these and reg per variant (but that was
> mentioned by Conor, I think).

All of that will be removed, PHY configuration is stable around whole
SPI frequency range. Internal SoC structure must be changed to change
That values, it will be easier to track that in the driver, based on
SoC version/overlay version(if ever that will be necessary)

> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists