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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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.

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

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.

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


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

Best regards,
Krzysztof