| Message ID | 20240509010523.3152264-3-wsadowski@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Marvell HW overlay support for Cadence xSPI | expand |
Hey Witold, On Wed, May 08, 2024 at 06:05:20PM -0700, Witold Sadowski wrote: > allOf: > - $ref: spi-controller.yaml# > + - if: > + properties: > + compatible: > + contains: > + const: marvell,cn10-xspi-nor > + then: > + properties: > + reg-names: > + items: > + - const: io > + - const: sdma > + - const: aux > + - const: xferbase > + reg: > + 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 > + else: > + properties: > + reg-names: > + items: > + - const: io > + - const: sdma > + - const: aux > + reg: > + 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 The usual approach here is to define the loosest possible constraints at the top level, so unconditionally define the xfer register region, and then constrain things based on compatible. In this case, you can set minItems to 3 unconditionally and then do (in psuedocode): if: marvell then: reg: minitems: 4 else reg: maxItems: 3 Additionally, when the allOf: is more then just references to other documents, it should be moved below the required list. Thanks, Conor.
On 09/05/2024 03:05, Witold Sadowski wrote: > Add new bindings for v2 Marvell xSPI overlay: > mrvl,xspi-nor compatible string Where? Why double space? subject prefix: spi goes first 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. For bindings, the preferred subjects are explained here: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters > 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 I have troubles reading this. Is this some sort of one long sentence or a list? > > Signed-off-by: Witold Sadowski <wsadowski@marvell.com> > --- > .../devicetree/bindings/spi/cdns,xspi.yaml | 78 +++++++++++++++---- > 1 file changed, 65 insertions(+), 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > index eb0f92468185..094f8b7ffc49 100644 > --- a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > +++ b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > @@ -17,22 +17,43 @@ description: | > > allOf: > - $ref: spi-controller.yaml# > + - if: Move the allOf after required block. > + properties: > + compatible: > + contains: > + const: marvell,cn10-xspi-nor > + then: > + properties: > + reg-names: > + items: > + - const: io > + - const: sdma > + - const: aux > + - const: xferbase > + reg: > + 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 > + else: > + properties: > + reg-names: > + items: > + - const: io > + - const: sdma > + - const: aux > + reg: > + 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 > > properties: > compatible: > - const: cdns,xspi-nor > - > - reg: > - 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 Widest constraints stay here. > - > - reg-names: > - items: > - - const: io > - - const: sdma > - - const: aux Widest constraints stay here. > + enum: > + - cdns,xspi-nor > + - marvell,cn10-xspi-nor > > interrupts: > maxItems: 1 > @@ -68,6 +89,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>; > + > + spi@d0010000 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "marvell,cn10-xspi-nor"; > + reg = <0x0 0xa0010000 0x0 0x1040>, > + <0x0 0xb0000000 0x0 0x1000>, > + <0x0 0xa0020000 0x0 0x100>, > + <0x0 0xa0090000 0x0 0x100>; No need for new example for difference in one property. Best regards, Krzysztof
> On 09/05/2024 03:05, Witold Sadowski wrote: > > Add new bindings for v2 Marvell xSPI overlay: > > mrvl,xspi-nor compatible string > > Where? > > Why double space? > > subject prefix: spi goes first > > 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. For bindings, the preferred subjects are explained > here: > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__www.kernel.org_doc_html_latest_devicetree_bindings_submitting- > 2Dpatches.html-23i-2Dfor-2Dpatch- > 2Dsubmitters&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=GKgcn-g6ZX- > JmCL3S2qKgVQhvhv7hu2n8En- > dZbLTa8&m=qeqJlq7clOFz9f6HAtdYw0Qlc95bPthAbwzLip4BHM3fDtQ_rIx- > R6WPFdKtjM5T&s=d2T7Rq5gLIrXRKVDPv9VDiGgiQVD7GFAxw7lFJ1tgvg&e= > Looks like old compatible string was included in commit message. > > > 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 > > I have troubles reading this. Is this some sort of one long sentence or a > list? > > > > > Signed-off-by: Witold Sadowski <wsadowski@marvell.com> > > --- > > .../devicetree/bindings/spi/cdns,xspi.yaml | 78 +++++++++++++++---- > > 1 file changed, 65 insertions(+), 13 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > > b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > > index eb0f92468185..094f8b7ffc49 100644 > > --- a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > > +++ b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml > > @@ -17,22 +17,43 @@ description: | > > > > allOf: > > - $ref: spi-controller.yaml# > > + - if: > > Move the allOf after required block. > > > + properties: > > + compatible: > > + contains: > > + const: marvell,cn10-xspi-nor > > + then: > > + properties: > > + reg-names: > > + items: > > + - const: io > > + - const: sdma > > + - const: aux > > + - const: xferbase > > + reg: > > + 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 > > + else: > > + properties: > > + reg-names: > > + items: > > + - const: io > > + - const: sdma > > + - const: aux > > + reg: > > + 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 > > > > properties: > > compatible: > > - const: cdns,xspi-nor > > - > > - reg: > > - 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 > > Widest constraints stay here. > > > - > > - reg-names: > > - items: > > - - const: io > > - - const: sdma > > - - const: aux > > Widest constraints stay here. Yaml file will be reworked to match guidelines. > > > + enum: > > + - cdns,xspi-nor > > + - marvell,cn10-xspi-nor > > > > interrupts: > > maxItems: 1 > > @@ -68,6 +89,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>; > > + > > + spi@d0010000 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + compatible = "marvell,cn10-xspi-nor"; > > + reg = <0x0 0xa0010000 0x0 0x1040>, > > + <0x0 0xb0000000 0x0 0x1000>, > > + <0x0 0xa0020000 0x0 0x100>, > > + <0x0 0xa0090000 0x0 0x100>; > > No need for new example for difference in one property. Ok, I will keep only original one > > Best regards, > Krzysztof
> ---------------------------------------------------------------------- > Hey Witold, > > On Wed, May 08, 2024 at 06:05:20PM -0700, Witold Sadowski wrote: > > > allOf: > > - $ref: spi-controller.yaml# > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: marvell,cn10-xspi-nor > > + then: > > + properties: > > + reg-names: > > + items: > > + - const: io > > + - const: sdma > > + - const: aux > > + - const: xferbase > > + reg: > > + 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 > > + else: > > + properties: > > + reg-names: > > + items: > > + - const: io > > + - const: sdma > > + - const: aux > > + reg: > > + 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 > > The usual approach here is to define the loosest possible constraints at > the top level, so unconditionally define the xfer register region, and > then constrain things based on compatible. In this case, you can set > minItems to 3 unconditionally and then do (in psuedocode): > if: > marvell > then: > reg: > minitems: 4 > else > reg: > maxItems: 3 > > Additionally, when the allOf: is more then just references to other > documents, it should be moved below the required list. Thanks for hints. Yaml file will be reworked to match guideliness > > Thanks, > Conor. Regards Witek
diff --git a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml index eb0f92468185..094f8b7ffc49 100644 --- a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml +++ b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml @@ -17,22 +17,43 @@ description: | allOf: - $ref: spi-controller.yaml# + - if: + properties: + compatible: + contains: + const: marvell,cn10-xspi-nor + then: + properties: + reg-names: + items: + - const: io + - const: sdma + - const: aux + - const: xferbase + reg: + 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 + else: + properties: + reg-names: + items: + - const: io + - const: sdma + - const: aux + reg: + 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 properties: compatible: - const: cdns,xspi-nor - - reg: - 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 - - reg-names: - items: - - const: io - - const: sdma - - const: aux + enum: + - cdns,xspi-nor + - marvell,cn10-xspi-nor interrupts: maxItems: 1 @@ -68,6 +89,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>; + + spi@d0010000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "marvell,cn10-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>;
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 | 78 +++++++++++++++---- 1 file changed, 65 insertions(+), 13 deletions(-)