| Message ID | 20240329194849.25554-1-wsadowski@marvell.com |
|---|---|
| Headers | show |
| Series | Support for Cadence xSPI Marvell modifications | expand |
On 29/03/2024 20:48, Witold Sadowski wrote: > Add new bindings: > - mrvl,xspi-nor compatible string > Compatible string to enable Marvell XSPI modification > - Multiple PHY configuration registers > - base for xfer register set > > Signed-off-by: Witold Sadowski <wsadowski@marvell.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. > --- > .../devicetree/bindings/spi/cdns,xspi.yaml | 84 ++++++++++++++++++- > 1 file changed, 83 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > index eb0f92468185..d1fde8d4e9b8 100644 > --- a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > +++ b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > @@ -20,23 +20,74 @@ allOf: > > properties: > compatible: > - const: cdns,xspi-nor > + - const: cdns,xspi-nor > + - const: mrvl,xspi-nor It does not look like you tested the bindings, at least after quick look. Please run `make dt_binding_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Maybe you need to update your dtschema and yamllint. There is a lot of things happening here, but I won't perform review if the code was never tested. Sorry, please test before sending. Best regards, Krzysztof
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@marvell.com> > --- > .../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 >
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@marvell.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. > .../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
On 30/03/2024 12:32, Krzysztof Kozlowski wrote: ... >> >> properties: >> compatible: >> - const: cdns,xspi-nor >> + - const: cdns,xspi-nor >> + - const: mrvl,xspi-nor > > It does not look like you tested the bindings, at least after quick > look. Please run `make dt_binding_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > Maybe you need to update your dtschema and yamllint. > > There is a lot of things happening here, but I won't perform review if > the code was never tested. Sorry, please test before sending. Hi, You did not address any of the comments and build issues neither here nor in previous mailings. Starting from 2022, all previous comments look ignored and then after some time you send another v1 expecting us to do review of the same code with the same issues. I gave it 1 month for some sort of response from you. None came. Therefore let's be clear: I will automatically NAK any further version you send, based on assumption you ignored entire existing feedback. Before sending any new patches for this XSPI, please first respond to numerous emails you already got (from the last two years). That's way we will know that reviewer's feedback is being addressed. Thank you for understanding. Best regards, Krzysztof
> ---------------------------------------------------------------------- > 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@marvell.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
> ---------------------------------------------------------------------- > 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@marvell.com> > > --- > > .../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
On Mon, Apr 29, 2024 at 02:47:23PM +0000, Witold Sadowski wrote: > > ---------------------------------------------------------------------- > > 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@marvell.com> > > > --- > > > .../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. I'm struggling to understand what you mean here by "overlay". Ordinarily I'd expect someone to meant a dt-overlay, but you're talking about IP blocks, so this sounds like hardware modifications. I am also a bit confused by the claim that the "internal SoC structure will be the same". Usually different SoCs have different internal structures, even when they re-use IP cores. If they have the same internal structure then they're not really different SoCs, just different packages! I think what you're saying here is that you intend using the "mrvl,xspi-nor" compatible for multiple SoCs that all contain the same modified versions of the Cadence IP, not different packages for the same SoC? Confusing wording aside, using the same generic compatible for different SoCs is what I trying to avoid. I don't mind there being a fallback compatible that's generic, but I want to see specific compatibles here for the individual SoCs. If you did actually mean that only the packaging is different between the devices, then I don't think you need specific compatibles for each different package, but you should have one for the SoC itself IMO. Cheers, Conor.
> On Mon, Apr 29, 2024 at 02:47:23PM +0000, Witold Sadowski wrote: > > > -------------------------------------------------------------------- > > > -- 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@marvell.com> > > > > --- > > > > .../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. > > I'm struggling to understand what you mean here by "overlay". Ordinarily > I'd expect someone to meant a dt-overlay, but you're talking about IP > blocks, so this sounds like hardware modifications. > I am also a bit confused by the claim that the "internal SoC structure > will be the same". Usually different SoCs have different internal > structures, even when they re-use IP cores. If they have the same internal > structure then they're not really different SoCs, just different packages! > I think what you're saying here is that you intend using the "mrvl,xspi- > nor" compatible for multiple SoCs that all contain the same modified > versions of the Cadence IP, not different packages for the same SoC? Yes, by HW overlay I meant actual HW modification. I called it overlay, as it is not modifying xSPI block itself, but it is rather build around it. As I don't have better word for it now, I will continue to use it. With that approach we can still have full functionality of xSPI block (like memory transfers), and additional features(like SPI full-duplex operations). Regarding "internal SoC structure" - I meant physical parameters of silicon, Signal propagation times inside block etc. That won't be changed if we have Two different SoC, bud made in same technology. > > Confusing wording aside, using the same generic compatible for different > SoCs is what I trying to avoid. I don't mind there being a fallback > compatible that's generic, but I want to see specific compatibles here for > the individual SoCs. > > If you did actually mean that only the packaging is different between the > devices, then I don't think you need specific compatibles for each > different package, but you should have one for the SoC itself IMO. We can have SoC A, B with common xSPI block, and both of them can share Same dtb node with compatible property "marvell,cn10k,xspi-nor" for example. I don't think it will be beneficial to have different compatible name for each different SoC, for example "marvell,t98,xspi-nor", if all other parts will be the same. Or am I not correct? > > Cheers, > Conor. Regards Witek
On 30/04/2024 00:59, Witold Sadowski wrote: > >> >> Confusing wording aside, using the same generic compatible for different >> SoCs is what I trying to avoid. I don't mind there being a fallback >> compatible that's generic, but I want to see specific compatibles here for >> the individual SoCs. >> >> If you did actually mean that only the packaging is different between the >> devices, then I don't think you need specific compatibles for each >> different package, but you should have one for the SoC itself IMO. > > We can have SoC A, B with common xSPI block, and both of them can share > Same dtb node with compatible property "marvell,cn10k,xspi-nor" for > example. I don't think it will be beneficial to have different compatible > name for each different SoC, for example "marvell,t98,xspi-nor", if all > other parts will be the same. Or am I not correct? Please see writing bindings (or any presentation for DTS and bindings): you are expected to have SoC specific compatibles for every block in the SoC. There are many examples in recent bindings, so take a look there. Best regards, Krzysztof