| Message ID | 20200520123612.11797-1-vadivel.muruganx.ramuthevar@linux.intel.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/1] dt-bindings: spi: Add schema for Cadence QSPI Controller driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 3 warnings, 133 lines checked |
| robh/dt-meta-schema | success |
On Wed, May 20, 2020 at 08:36:12PM +0800, Ramuthevar,Vadivel MuruganX wrote: > From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> > > Add dt-bindings documentation for Cadence-QSPI controller to support > spi based flash memories. > > Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> > --- > .../devicetree/bindings/mtd/cadence-quadspi.txt | 67 ----------- > .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 133 +++++++++++++++++++++ The changelog says this is adding a new binding but the actual change is mostly a conversion to YAML. Please split the additions out into a separate change, ideally doing that before the conversion since there is a backlog on review of YAML conversions.
Hi Mark, Thank you for the review comments... On 20/5/2020 8:43 pm, Mark Brown wrote: > On Wed, May 20, 2020 at 08:36:12PM +0800, Ramuthevar,Vadivel MuruganX wrote: >> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> >> >> Add dt-bindings documentation for Cadence-QSPI controller to support >> spi based flash memories. >> >> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> >> --- >> .../devicetree/bindings/mtd/cadence-quadspi.txt | 67 ----------- >> .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 133 +++++++++++++++++++++ > > The changelog says this is adding a new binding but the actual change is > mostly a conversion to YAML. Please split the additions out into a > separate change, ideally doing that before the conversion since there is > a backlog on review of YAML conversions. Initially was sending the only YAML file alone, then reviewers suggest to me do this way so I did, next by split the patches like below... 1. remove the cadence-quadspi.txt (patch1) 2. convert txt to YAML (patch2) Regards Vadivel >
On Thu, May 21, 2020 at 10:18:26AM +0800, Ramuthevar, Vadivel MuruganX wrote: > On 20/5/2020 8:43 pm, Mark Brown wrote: > > On Wed, May 20, 2020 at 08:36:12PM +0800, Ramuthevar,Vadivel MuruganX wrote: > > > .../devicetree/bindings/mtd/cadence-quadspi.txt | 67 ----------- > > > .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 133 +++++++++++++++++++++ > > The changelog says this is adding a new binding but the actual change is > > mostly a conversion to YAML. Please split the additions out into a > > separate change, ideally doing that before the conversion since there is > > a backlog on review of YAML conversions. > Initially was sending the only YAML file alone, then reviewers suggest to me > do this way so I did, next by split the patches like below... > 1. remove the cadence-quadspi.txt (patch1) > 2. convert txt to YAML (patch2) That doesn't address either of the issues. The removal of the old bindings and addition of the YAML ones needs to be in a single patch doing that conversion. What I'm suggesting should be done separately is whatever changes to the semantics of the bindings you are (according to your changelog) doing.
Hi Mark, On 21/5/2020 6:56 pm, Mark Brown wrote: > On Thu, May 21, 2020 at 10:18:26AM +0800, Ramuthevar, Vadivel MuruganX wrote: >> On 20/5/2020 8:43 pm, Mark Brown wrote: >>> On Wed, May 20, 2020 at 08:36:12PM +0800, Ramuthevar,Vadivel MuruganX wrote: > >>>> .../devicetree/bindings/mtd/cadence-quadspi.txt | 67 ----------- >>>> .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 133 +++++++++++++++++++++ > >>> The changelog says this is adding a new binding but the actual change is >>> mostly a conversion to YAML. Please split the additions out into a >>> separate change, ideally doing that before the conversion since there is >>> a backlog on review of YAML conversions. > >> Initially was sending the only YAML file alone, then reviewers suggest to me >> do this way so I did, next by split the patches like below... > >> 1. remove the cadence-quadspi.txt (patch1) >> 2. convert txt to YAML (patch2) > > That doesn't address either of the issues. The removal of the old > bindings and addition of the YAML ones needs to be in a single patch > doing that conversion. What I'm suggesting should be done separately is > whatever changes to the semantics of the bindings you are (according to > your changelog) doing. You mean semantics of the binding as a single patch you are suggesting me, right? , Thanks! Regards Vadivel >
On Thu, May 21, 2020 at 08:14:04PM +0800, Ramuthevar, Vadivel MuruganX wrote: > On 21/5/2020 6:56 pm, Mark Brown wrote: > > That doesn't address either of the issues. The removal of the old > > bindings and addition of the YAML ones needs to be in a single patch > > doing that conversion. What I'm suggesting should be done separately is > > whatever changes to the semantics of the bindings you are (according to > > your changelog) doing. > You mean semantics of the binding as a single patch you are suggesting me, > right? , Thanks! I mean that any changes to the bindings ought to be split out into separate patches, if there's multiple changes it may make sense for there to be multiple patches.
Hi Mark, On 21/5/2020 8:20 pm, Mark Brown wrote: > On Thu, May 21, 2020 at 08:14:04PM +0800, Ramuthevar, Vadivel MuruganX wrote: >> On 21/5/2020 6:56 pm, Mark Brown wrote: > >>> That doesn't address either of the issues. The removal of the old >>> bindings and addition of the YAML ones needs to be in a single patch >>> doing that conversion. What I'm suggesting should be done separately is >>> whatever changes to the semantics of the bindings you are (according to >>> your changelog) doing. > >> You mean semantics of the binding as a single patch you are suggesting me, >> right? , Thanks! > > I mean that any changes to the bindings ought to be split out into > separate patches, if there's multiple changes it may make sense for > there to be multiple patches. Got it, we do not have multiple changes since it is new YAML file. in case if we feel anything to be added , we add as separate patches. Thanks! Regards Vadivel >
On Thu, May 21, 2020 at 08:34:43PM +0800, Ramuthevar, Vadivel MuruganX wrote: > On 21/5/2020 8:20 pm, Mark Brown wrote: > > I mean that any changes to the bindings ought to be split out into > > separate patches, if there's multiple changes it may make sense for > > there to be multiple patches. > Got it, we do not have multiple changes since it is new YAML file. > in case if we feel anything to be added , we add as separate patches. If this is just a conversion to YAML then your changelog is wildly inaccurate and needs to be improved, your changelog says you are adding new things.
Hi Mark, On 21/5/2020 8:37 pm, Mark Brown wrote: > On Thu, May 21, 2020 at 08:34:43PM +0800, Ramuthevar, Vadivel MuruganX wrote: >> On 21/5/2020 8:20 pm, Mark Brown wrote: > >>> I mean that any changes to the bindings ought to be split out into >>> separate patches, if there's multiple changes it may make sense for >>> there to be multiple patches. > >> Got it, we do not have multiple changes since it is new YAML file. >> in case if we feel anything to be added , we add as separate patches. > > If this is just a conversion to YAML then your changelog is wildly > inaccurate and needs to be improved, your changelog says you are adding > new things. Yes , You are right, just conversion only, over that adding/modifying few of the properties like compatible and node name in the example..etc Sure I will make a multiple patches , Thanks! Regards Vadivel >
diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt deleted file mode 100644 index 945be7d5b236..000000000000 --- a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt +++ /dev/null @@ -1,67 +0,0 @@ -* Cadence Quad SPI controller - -Required properties: -- compatible : should be one of the following: - Generic default - "cdns,qspi-nor". - For TI 66AK2G SoC - "ti,k2g-qspi", "cdns,qspi-nor". - For TI AM654 SoC - "ti,am654-ospi", "cdns,qspi-nor". -- reg : Contains two entries, each of which is a tuple consisting of a - physical address and length. The first entry is the address and - length of the controller register set. The second entry is the - address and length of the QSPI Controller data area. -- interrupts : Unit interrupt specifier for the controller interrupt. -- clocks : phandle to the Quad SPI clock. -- cdns,fifo-depth : Size of the data FIFO in words. -- cdns,fifo-width : Bus width of the data FIFO in bytes. -- cdns,trigger-address : 32-bit indirect AHB trigger address. - -Optional properties: -- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not. -- cdns,rclk-en : Flag to indicate that QSPI return clock is used to latch - the read data rather than the QSPI clock. Make sure that QSPI return - clock is populated on the board before using this property. - -Optional subnodes: -Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional -custom properties: -- cdns,read-delay : Delay for read capture logic, in clock cycles -- cdns,tshsl-ns : Delay in nanoseconds for the length that the master - mode chip select outputs are de-asserted between - transactions. -- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being - de-activated and the activation of another. -- cdns,tchsh-ns : Delay in nanoseconds between last bit of current - transaction and deasserting the device chip select - (qspi_n_ss_out). -- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low - and first bit transfer. -- resets : Must contain an entry for each entry in reset-names. - See ../reset/reset.txt for details. -- reset-names : Must include either "qspi" and/or "qspi-ocp". - -Example: - - qspi: spi@ff705000 { - compatible = "cdns,qspi-nor"; - #address-cells = <1>; - #size-cells = <0>; - reg = <0xff705000 0x1000>, - <0xffa00000 0x1000>; - interrupts = <0 151 4>; - clocks = <&qspi_clk>; - cdns,is-decoded-cs; - cdns,fifo-depth = <128>; - cdns,fifo-width = <4>; - cdns,trigger-address = <0x00000000>; - resets = <&rst QSPI_RESET>, <&rst QSPI_OCP_RESET>; - reset-names = "qspi", "qspi-ocp"; - - flash0: n25q00@0 { - ... - cdns,read-delay = <4>; - cdns,tshsl-ns = <50>; - cdns,tsd2d-ns = <50>; - cdns,tchsh-ns = <4>; - cdns,tslch-ns = <4>; - }; - }; diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml new file mode 100644 index 000000000000..1c15acc184b3 --- /dev/null +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml @@ -0,0 +1,133 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/spi/cdns,qspi-nor.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Cadence QSPI Flash Controller support + +maintainers: + - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> + +allOf: + - $ref: "spi-controller.yaml#" + +description: | + Binding Documentation for Cadence QSPI controller,This controller is + present in the Intel LGM, Altera SoCFPGA and TI SoCs and this driver + has been tested On Intel's LGM SoC. + +properties: + compatible: + enum: + - cdns,qspi-nor + - ti,k2g-qspi + - ti,am654-ospi + - intel,lgm-qspi + + reg: + items: + - description: + The first entry is the address and length of + the controller register set. + - description: + The second entry is the address and length of + the QSPI Controller data area. + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + cdns,fifo-depth: + description: + Depth of hardware FIFO in words. + allOf: + - $ref: "/schemas/types.yaml#/definitions/uint32" + - enum: [ 128, 256 ] + - default: 128 + + cdns,fifo-width: + $ref: /schemas/types.yaml#/definitions/uint32 + multipleOf: 4 + description: + 4 byte bus width of the data FIFO in bytes. + + cdns,trigger-address: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + 32-bit indirect AHB trigger address. + + cdns,rclk-en: + type: boolean + description: | + Flag to indicate that QSPI return clock is used to latch the read data + rather than the QSPI clock. Make sure that QSPI return clock is populated + on the board before using this property. + +# subnode's properties +patternProperties: + "@[0-9a-f]+$": + type: object + description: + flash device uses the subnodes below defined properties. + + cdns,read-delay: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Delay for read capture logic, in clock cycles. + + cdns,tshsl-ns: + description: | + Delay in nanoseconds for the length that the master mode chip select + outputs are de-asserted between transactions. + + cdns,tsd2d-ns: + description: | + Delay in nanoseconds between one chip select being de-activated + and the activation of another. + + cdns,tchsh-ns: + description: | + Delay in nanoseconds between last bit of current transaction and + deasserting the device chip select (qspi_n_ss_out). + + cdns,tslch-ns: + description: | + Delay in nanoseconds between setting qspi_n_ss_out low and + first bit transfer. + +required: + - compatible + - reg + - interrupts + - clocks + - cdns,fifo-depth + - cdns,fifo-width + - cdns,trigger-address + +examples: + - | + spi@ff705000 { + compatible = "cdns,qspi-nor"; + #address-cells = <1>; + #size-cells = <0>; + reg = <0xff705000 0x1000>, + <0xffa00000 0x1000>; + interrupts = <0 151 4>; + clocks = <&qspi_clk>; + cdns,fifo-depth = <128>; + cdns,fifo-width = <4>; + cdns,trigger-address = <0x00000000>; + + flash@0 { + compatible = "jedec,spi-nor"; + reg = <0x0>; + cdns,read-delay = <4>; + cdns,tshsl-ns = <50>; + cdns,tsd2d-ns = <50>; + cdns,tchsh-ns = <4>; + cdns,tslch-ns = <4>; + }; + }; +