| Message ID | 1387184330-14448-7-git-send-email-b32955@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday, December 16, 2013 at 09:58:49 AM, Huang Shijie wrote: > This patch adds the binding file for Freescale Quadspi driver. > > Signed-off-by: Huang Shijie <b32955@freescale.com> I might be blind and/or tired, but I don't see devicetree-discuss in the CC. Next time please CC this ML ( devicetree-discuss AT lists.ozlabs.org ) so the bindings are reviewed by experts . Best regards, Marek Vasut
Hello Marek, Marek Vasut wrote on 2013-12-17: > On Monday, December 16, 2013 at 09:58:49 AM, Huang Shijie wrote: >> This patch adds the binding file for Freescale Quadspi driver. >> >> Signed-off-by: Huang Shijie <b32955@freescale.com> > > I might be blind and/or tired, but I don't see devicetree-discuss in the CC. > Next time please CC this ML ( devicetree-discuss AT lists.ozlabs.org ) so the > bindings are reviewed by experts . I am sure you talking about the new list at devicetree@vger.kernel.org? > > Best regards, > Marek Vasut Best Regards, Thomas --- There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors. ---
On Tuesday, December 17, 2013 at 02:36:33 PM, thomas.langer@lantiq.com wrote: > Hello Marek, > > Marek Vasut wrote on 2013-12-17: > > On Monday, December 16, 2013 at 09:58:49 AM, Huang Shijie wrote: > >> This patch adds the binding file for Freescale Quadspi driver. > >> > >> Signed-off-by: Huang Shijie <b32955@freescale.com> > > > > I might be blind and/or tired, but I don't see devicetree-discuss in the > > CC. Next time please CC this ML ( devicetree-discuss AT lists.ozlabs.org > > ) so the bindings are reviewed by experts . > > I am sure you talking about the new list at devicetree@vger.kernel.org? Yes, thanks for correcting it. Best regards, Marek Vasut
[ adding Cc: to devicetree; make sure to keep 'binding' in the subject line upon re-submission, such that reviewers can tell whether you introduce a new binding, or just use an existing binding and implement it in a dts/dtsi file ] On Mon, Dec 16, 2013 at 16:58 +0800, Huang Shijie wrote: > > This patch adds the binding file for Freescale Quadspi driver. > > Signed-off-by: Huang Shijie <b32955@freescale.com> > --- > .../devicetree/bindings/mtd/fsl-quadspi.txt | 31 ++++++++++++++++++++ > 1 files changed, 31 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mtd/fsl-quadspi.txt > > diff --git a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt > new file mode 100644 > index 0000000..3475cfa > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt > @@ -0,0 +1,31 @@ > +* Freescale Quad Serial Peripheral Interface(QuadSPI) > + > +Required properties: > +- compatible : Should be "fsl,vf610-qspi" > +- reg : Offset and length of the register set for the device > +- interrupts : Should contain the interrupt for the device > +- clocks : The clocks needed by the QuadSPI controller > +- clock-names : the name of the clocks > +- fsl,nor-num : Contains the number of SPI NOR chips connected to > + the controller. > +- fsl,nor-size : the size of each SPI NOR. > +- fsl,max-frequency : the frequency at which the SPI NOR works. Those "fsl,*" properties somehow feel strange. I comment on details below the example since this should even better show why I feel so. > + > +Example: > + > +qspi0: quadspi@40044000 { > + compatible = "fsl,vf610-qspi"; > + reg = <0x40044000 0x1000>; > + interrupts = <0 24 0x04>; > + clocks = <&clks VF610_CLK_QSPI0_EN>, > + <&clks VF610_CLK_QSPI0>; > + clock-names = "qspi_en", "qspi"; > + fsl,nor-num = <1>; > + fsl,nor-size = <0x1000000>; > + fsl,max-frequency = <66000000>; > + status = "okay"; > + > + flash0: s25fl128s@0 { > + .... > + }; > +}; The number of chips connected to the controller should reflect in the child nodes of the SPI NOR controller, and need not get specified in redundant ways and with potential for errors when they can get determined at runtime. The capacity of the flash chip as well as the maximum frequency which the flash chip operates at should be features of the flash chip (in combination with the board), i.e. of the child node and not the controller. SPI slaves already have a documented property for the purpose of limiting transfer rates when they are lower than the controller's i.e. busses capabilities. Can't tell from the top of my head whether there is a property for the maximum frequency which a controller should use across the whole bus. In any case, either the property needs to get moved, or the description should get updated to say "the max frequency at which the controller will send data" or something. The capacity of the flash chip should be a consequence of either having gathered CFI information (if available) or having identified the chip by its JEDEC ID and looked up its features in an internal database. Users should not need to specify the capacity of the flash chip in the device tree. If the 'fsl,nor-size' property remains (which I doubt at the moment), you cannot describe "the size of each" chip in one single-cell spec. So the documentation should get extended to reflect multi-chip setups. But I'd rather assume that the property is not needed at all. You can omit the 'status = "okay"' line since that is the default already in the absence of the keyword. This property is most useful to declare yet not enable by default components in .dtsi files and to do enable them in individual board files if applicable. This aspect need not be shown in the binding example of a QSPI controller. I'd suggest to use symbolic names for the flags in the last interrupt specifier cell, as you do for clock items. And while I'm in nitpick mode :) let me say that I dislike the blanking around the colons in the properties discussion, but I'm as well aware that these are "inspired by" the early OF/DT documents. There may be as many expectations about formatting of a document as there are readers/consumers. virtually yours Gerhard Sittig
On Wed, Dec 18, 2013 at 04:30:29PM +0100, Gerhard Sittig wrote: > > +++ b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt > > @@ -0,0 +1,31 @@ > > +* Freescale Quad Serial Peripheral Interface(QuadSPI) > > + > > +Required properties: > > +- compatible : Should be "fsl,vf610-qspi" > > +- reg : Offset and length of the register set for the device > > +- interrupts : Should contain the interrupt for the device > > +- clocks : The clocks needed by the QuadSPI controller > > +- clock-names : the name of the clocks > > +- fsl,nor-num : Contains the number of SPI NOR chips connected to > > + the controller. > > +- fsl,nor-size : the size of each SPI NOR. > > +- fsl,max-frequency : the frequency at which the SPI NOR works. > > Those "fsl,*" properties somehow feel strange. I comment on > details below the example since this should even better show why > I feel so. > > > + > > +Example: > > + > > +qspi0: quadspi@40044000 { > > + compatible = "fsl,vf610-qspi"; > > + reg = <0x40044000 0x1000>; > > + interrupts = <0 24 0x04>; > > + clocks = <&clks VF610_CLK_QSPI0_EN>, > > + <&clks VF610_CLK_QSPI0>; > > + clock-names = "qspi_en", "qspi"; > > + fsl,nor-num = <1>; > > + fsl,nor-size = <0x1000000>; > > + fsl,max-frequency = <66000000>; > > + status = "okay"; > > + > > + flash0: s25fl128s@0 { > > + .... > > + }; > > +}; > > > The number of chips connected to the controller should reflect in > the child nodes of the SPI NOR controller, and need not get > specified in redundant ways and with potential for errors when > they can get determined at runtime. thanks. I have already removed this property in the next version. > > The capacity of the flash chip as well as the maximum frequency > which the flash chip operates at should be features of the flash > chip (in combination with the board), i.e. of the child node and agree. thanks. > not the controller. SPI slaves already have a documented > property for the purpose of limiting transfer rates when they are > lower than the controller's i.e. busses capabilities. Can't tell > from the top of my head whether there is a property for the > maximum frequency which a controller should use across the whole > bus. In any case, either the property needs to get moved, or the > description should get updated to say "the max frequency at which > the controller will send data" or something. > > The capacity of the flash chip should be a consequence of either > having gathered CFI information (if available) or having > identified the chip by its JEDEC ID and looked up its features in > an internal database. Users should not need to specify the > capacity of the flash chip in the device tree. I will remove it in the next version. > > > If the 'fsl,nor-size' property remains (which I doubt at the > moment), you cannot describe "the size of each" chip in one > single-cell spec. So the documentation should get extended to > reflect multi-chip setups. But I'd rather assume that the > property is not needed at all. > > You can omit the 'status = "okay"' line since that is the default > already in the absence of the keyword. This property is most > useful to declare yet not enable by default components in .dtsi > files and to do enable them in individual board files if > applicable. This aspect need not be shown in the binding example > of a QSPI controller. > > I'd suggest to use symbolic names for the flags in the last > interrupt specifier cell, as you do for clock items. I will use the symbolic name if it has. But if it not have a symbolic name, i have to keep it as it is now. Thanks Huang Shijie
On Thu, Dec 19, 2013 at 00:03 +0800, Huang Shijie wrote: > > On Wed, Dec 18, 2013 at 04:30:29PM +0100, Gerhard Sittig wrote: > > > +++ b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt > > > [ ... ] > > > +Example: > > > + > > > +qspi0: quadspi@40044000 { > > > + compatible = "fsl,vf610-qspi"; > > > + reg = <0x40044000 0x1000>; > > > + interrupts = <0 24 0x04>; > > > [ ... ] > > > +}; > > > > [ ... ] > > I'd suggest to use symbolic names for the flags in the last > > interrupt specifier cell, as you do for clock items. > I will use the symbolic name if it has. > But if it not have a symbolic name, i have to keep it as it is now. To avoid potential confusion: I meant symbolic names for the _flags_ of the interrupt specifier (i.e. trigger type and polarity). The <dt-bindings/interrupt-controller/irq.h> file has IRQ_TYPE_LEVEL_HIGH with value 4 for you. virtually yours Gerhard Sittig
diff --git a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt new file mode 100644 index 0000000..3475cfa --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt @@ -0,0 +1,31 @@ +* Freescale Quad Serial Peripheral Interface(QuadSPI) + +Required properties: +- compatible : Should be "fsl,vf610-qspi" +- reg : Offset and length of the register set for the device +- interrupts : Should contain the interrupt for the device +- clocks : The clocks needed by the QuadSPI controller +- clock-names : the name of the clocks +- fsl,nor-num : Contains the number of SPI NOR chips connected to + the controller. +- fsl,nor-size : the size of each SPI NOR. +- fsl,max-frequency : the frequency at which the SPI NOR works. + +Example: + +qspi0: quadspi@40044000 { + compatible = "fsl,vf610-qspi"; + reg = <0x40044000 0x1000>; + interrupts = <0 24 0x04>; + clocks = <&clks VF610_CLK_QSPI0_EN>, + <&clks VF610_CLK_QSPI0>; + clock-names = "qspi_en", "qspi"; + fsl,nor-num = <1>; + fsl,nor-size = <0x1000000>; + fsl,max-frequency = <66000000>; + status = "okay"; + + flash0: s25fl128s@0 { + .... + }; +};
This patch adds the binding file for Freescale Quadspi driver. Signed-off-by: Huang Shijie <b32955@freescale.com> --- .../devicetree/bindings/mtd/fsl-quadspi.txt | 31 ++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/mtd/fsl-quadspi.txt