| Message ID | 1452486886-8049-1-git-send-email-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
Hi Marek, On 01/10/2016 10:34 PM, Marek Vasut wrote: > From: Graham Moore <grmoore@opensource.altera.com> > > Add binding document for the Cadence QSPI controller. > > Signed-off-by: Graham Moore <grmoore@opensource.altera.com> > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alan Tull <atull@opensource.altera.com> > Cc: Brian Norris <computersforpeace@gmail.com> > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > Cc: Graham Moore <grmoore@opensource.altera.com> > Cc: "R, Vignesh" <vigneshr@ti.com> > Cc: Yves Vandervennet <yvanderv@opensource.altera.com> > Cc: devicetree@vger.kernel.org > --- I think you also need to CC the following people for proper review of these bindings: Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Pawel Moll <pawel.moll@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Ian Campbell <ijc+devicetree@hellion.org.uk> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Kumar Gala <galak@codeaurora.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Dinh
On Monday, January 11, 2016 at 05:06:30 PM, Dinh Nguyen wrote: > Hi Marek, Hi! > On 01/10/2016 10:34 PM, Marek Vasut wrote: > > From: Graham Moore <grmoore@opensource.altera.com> > > > > Add binding document for the Cadence QSPI controller. > > > > Signed-off-by: Graham Moore <grmoore@opensource.altera.com> > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Alan Tull <atull@opensource.altera.com> > > Cc: Brian Norris <computersforpeace@gmail.com> > > Cc: David Woodhouse <dwmw2@infradead.org> > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > > Cc: Graham Moore <grmoore@opensource.altera.com> > > Cc: "R, Vignesh" <vigneshr@ti.com> > > Cc: Yves Vandervennet <yvanderv@opensource.altera.com> > > Cc: devicetree@vger.kernel.org > > --- > > I think you also need to CC the following people for proper review of > these bindings: > > Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS) > Pawel Moll <pawel.moll@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS) > Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND > FLATTENED DEVICE TREE BINDINGS) > Ian Campbell <ijc+devicetree@hellion.org.uk> (maintainer:OPEN FIRMWARE > AND FLATTENED DEVICE TREE BINDINGS) > Kumar Gala <galak@codeaurora.org> (maintainer:OPEN FIRMWARE AND > FLATTENED DEVICE TREE BINDINGS) Isn't it enough to submit them to devicetree@vger ? Best regards, Marek Vasut
On 01/11/2016 10:32 AM, Marek Vasut wrote: > On Monday, January 11, 2016 at 05:06:30 PM, Dinh Nguyen wrote: >> Hi Marek, > > Hi! > >> On 01/10/2016 10:34 PM, Marek Vasut wrote: >>> From: Graham Moore <grmoore@opensource.altera.com> >>> >>> Add binding document for the Cadence QSPI controller. >>> >>> Signed-off-by: Graham Moore <grmoore@opensource.altera.com> >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Cc: Alan Tull <atull@opensource.altera.com> >>> Cc: Brian Norris <computersforpeace@gmail.com> >>> Cc: David Woodhouse <dwmw2@infradead.org> >>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com> >>> Cc: Graham Moore <grmoore@opensource.altera.com> >>> Cc: "R, Vignesh" <vigneshr@ti.com> >>> Cc: Yves Vandervennet <yvanderv@opensource.altera.com> >>> Cc: devicetree@vger.kernel.org >>> --- >> >> I think you also need to CC the following people for proper review of >> these bindings: >> >> Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED >> DEVICE TREE BINDINGS) >> Pawel Moll <pawel.moll@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED >> DEVICE TREE BINDINGS) >> Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND >> FLATTENED DEVICE TREE BINDINGS) >> Ian Campbell <ijc+devicetree@hellion.org.uk> (maintainer:OPEN FIRMWARE >> AND FLATTENED DEVICE TREE BINDINGS) >> Kumar Gala <galak@codeaurora.org> (maintainer:OPEN FIRMWARE AND >> FLATTENED DEVICE TREE BINDINGS) > > Isn't it enough to submit them to devicetree@vger ? > Not sure, but I always CC those names explicitly for DTS bindings since they are listed in the MAINTAINERS file. Dinh
On Monday, January 11, 2016 at 06:03:53 PM, Dinh Nguyen wrote: > On 01/11/2016 10:32 AM, Marek Vasut wrote: > > On Monday, January 11, 2016 at 05:06:30 PM, Dinh Nguyen wrote: > >> Hi Marek, > > > > Hi! > > > >> On 01/10/2016 10:34 PM, Marek Vasut wrote: > >>> From: Graham Moore <grmoore@opensource.altera.com> > >>> > >>> Add binding document for the Cadence QSPI controller. > >>> > >>> Signed-off-by: Graham Moore <grmoore@opensource.altera.com> > >>> Signed-off-by: Marek Vasut <marex@denx.de> > >>> Cc: Alan Tull <atull@opensource.altera.com> > >>> Cc: Brian Norris <computersforpeace@gmail.com> > >>> Cc: David Woodhouse <dwmw2@infradead.org> > >>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > >>> Cc: Graham Moore <grmoore@opensource.altera.com> > >>> Cc: "R, Vignesh" <vigneshr@ti.com> > >>> Cc: Yves Vandervennet <yvanderv@opensource.altera.com> > >>> Cc: devicetree@vger.kernel.org > >>> --- > >> > >> I think you also need to CC the following people for proper review of > >> these bindings: > >> > >> Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED > >> DEVICE TREE BINDINGS) > >> Pawel Moll <pawel.moll@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED > >> DEVICE TREE BINDINGS) > >> Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND > >> FLATTENED DEVICE TREE BINDINGS) > >> Ian Campbell <ijc+devicetree@hellion.org.uk> (maintainer:OPEN FIRMWARE > >> AND FLATTENED DEVICE TREE BINDINGS) > >> Kumar Gala <galak@codeaurora.org> (maintainer:OPEN FIRMWARE AND > >> FLATTENED DEVICE TREE BINDINGS) > > > > Isn't it enough to submit them to devicetree@vger ? > > Not sure, but I always CC those names explicitly for DTS bindings since > they are listed in the MAINTAINERS file. OK, will do in the next revision (if needed). Thanks! Best regards, Marek Vasut
On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: > From: Graham Moore <grmoore@opensource.altera.com> > > Add binding document for the Cadence QSPI controller. > > Signed-off-by: Graham Moore <grmoore@opensource.altera.com> > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alan Tull <atull@opensource.altera.com> > Cc: Brian Norris <computersforpeace@gmail.com> > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > Cc: Graham Moore <grmoore@opensource.altera.com> > Cc: "R, Vignesh" <vigneshr@ti.com> > Cc: Yves Vandervennet <yvanderv@opensource.altera.com> > Cc: devicetree@vger.kernel.org > --- > .../devicetree/bindings/mtd/cadence-quadspi.txt | 56 ++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt > > V2: Add cdns prefix to driver-specific bindings. > V3: Use existing property "is-decoded-cs" instead of creating a > duplicate, "ext-decoder". Timing parameters are in nanoseconds, > not master reference clocks. Remove bus-num completely. > V4: Add new properties fifo-width and trigger-address > V7: - Prefix all of the Cadence-specific properties with cdns prefix, > those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth", > "cdns,fifo-width", "cdns,trigger-address". > - Drop bogus properties which were not used and were incorrect. > V8: Align lines to 80 chars. > > diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt > new file mode 100644 > index 0000000..f248056 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt > @@ -0,0 +1,56 @@ > +* Cadence Quad SPI controller > + > +Required properties: > +- compatible : Should be "cdns,qspi-nor". Fine, but I expect to see SOCs using this block add their own compatible strings. It wouldn't surprise me that we already have some using this block. Acked-by: Rob Herring <robh@kernel.org>
On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: > On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: > > From: Graham Moore <grmoore@opensource.altera.com> > > > > Add binding document for the Cadence QSPI controller. > > > > Signed-off-by: Graham Moore <grmoore@opensource.altera.com> > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Alan Tull <atull@opensource.altera.com> > > Cc: Brian Norris <computersforpeace@gmail.com> > > Cc: David Woodhouse <dwmw2@infradead.org> > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > > Cc: Graham Moore <grmoore@opensource.altera.com> > > Cc: "R, Vignesh" <vigneshr@ti.com> > > Cc: Yves Vandervennet <yvanderv@opensource.altera.com> > > Cc: devicetree@vger.kernel.org > > --- > > > > .../devicetree/bindings/mtd/cadence-quadspi.txt | 56 > > ++++++++++++++++++++++ 1 file changed, 56 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/mtd/cadence-quadspi.txt > > > > V2: Add cdns prefix to driver-specific bindings. > > V3: Use existing property "is-decoded-cs" instead of creating a > > > > duplicate, "ext-decoder". Timing parameters are in nanoseconds, > > not master reference clocks. Remove bus-num completely. > > > > V4: Add new properties fifo-width and trigger-address > > V7: - Prefix all of the Cadence-specific properties with cdns prefix, > > > > those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth", > > "cdns,fifo-width", "cdns,trigger-address". > > > > - Drop bogus properties which were not used and were incorrect. > > > > V8: Align lines to 80 chars. > > > > diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt > > b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt new file > > mode 100644 > > index 0000000..f248056 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt > > @@ -0,0 +1,56 @@ > > +* Cadence Quad SPI controller > > + > > +Required properties: > > +- compatible : Should be "cdns,qspi-nor". > > Fine, but I expect to see SOCs using this block add their own compatible > strings. It wouldn't surprise me that we already have some using this > block. > > Acked-by: Rob Herring <robh@kernel.org> I finally got an Ack on this, I am so happy :-) As for the SoCs, there is Altera SoCFPGA Gen 5 and Gen 10 which uses this. Then there is some TI SoC, but I don't know the model. Vignesh (on CC) would. Then there is some ST SoC, but I have no idea what that's all about, sorry. All these SoCs should be capable of tweaking the block to fit their needs by just the DT properties. I believe they differ only in the FIFO depth and sometimes someone is greedy and uses 4:16 CS multiplexer, which is an external passive component, but that's all. Would we need soc-specific compatible strings if this is the case? Best regards, Marek Vasut
On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote: > On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: > > On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt > > > @@ -0,0 +1,56 @@ > > > +* Cadence Quad SPI controller > > > + > > > +Required properties: > > > +- compatible : Should be "cdns,qspi-nor". > > > > Fine, but I expect to see SOCs using this block add their own compatible > > strings. It wouldn't surprise me that we already have some using this > > block. > > > > Acked-by: Rob Herring <robh@kernel.org> > > I finally got an Ack on this, I am so happy :-) > > As for the SoCs, there is Altera SoCFPGA Gen 5 and Gen 10 which uses this. > Then there is some TI SoC, but I don't know the model. Vignesh (on CC) would. > Then there is some ST SoC, but I have no idea what that's all about, sorry. > > All these SoCs should be capable of tweaking the block to fit their needs > by just the DT properties. I believe they differ only in the FIFO depth and > sometimes someone is greedy and uses 4:16 CS multiplexer, which is an external > passive component, but that's all. > > Would we need soc-specific compatible strings if this is the case? It's nice when most things can be supported with a small set of DT properties, as you've done. But IUIC, I think it's usually good practice to define and use SoC-specific (or maybe SoC family) compatible strings in the docs and DTS files, in addition to the generic one, in case there are future quirks that need to be handled. Note that you don't actually have to use these in the driver yet, but it's good to have a definition. So you can, today, have: foo@xxxx { compatible = "ti,baz-12345", "cdns,qspi-nor"; ... }; And we have the option to pick up "ti,baz-12345" in the Linux driver *if needed.* Brian
On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote: > On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote: > > On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: > > > On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt > > > > @@ -0,0 +1,56 @@ > > > > +* Cadence Quad SPI controller > > > > + > > > > +Required properties: > > > > +- compatible : Should be "cdns,qspi-nor". > > > > > > Fine, but I expect to see SOCs using this block add their own > > > compatible strings. It wouldn't surprise me that we already have some > > > using this block. > > > > > > Acked-by: Rob Herring <robh@kernel.org> > > > > I finally got an Ack on this, I am so happy :-) > > > > As for the SoCs, there is Altera SoCFPGA Gen 5 and Gen 10 which uses > > this. Then there is some TI SoC, but I don't know the model. Vignesh (on > > CC) would. Then there is some ST SoC, but I have no idea what that's all > > about, sorry. > > > > All these SoCs should be capable of tweaking the block to fit their needs > > by just the DT properties. I believe they differ only in the FIFO depth > > and sometimes someone is greedy and uses 4:16 CS multiplexer, which is > > an external passive component, but that's all. > > > > Would we need soc-specific compatible strings if this is the case? > > It's nice when most things can be supported with a small set of DT > properties, as you've done. But IUIC, I think it's usually good practice > to define and use SoC-specific (or maybe SoC family) compatible strings > in the docs and DTS files, in addition to the generic one, in case there > are future quirks that need to be handled. Note that you don't actually > have to use these in the driver yet, but it's good to have a definition. > So you can, today, have: > > foo@xxxx { > compatible = "ti,baz-12345", "cdns,qspi-nor"; > ... > }; > > And we have the option to pick up "ti,baz-12345" in the Linux driver *if > needed.* Ah, got it, thanks! Best regards, Marek Vasut
On 02/02/2016 02:43 AM, Marek Vasut wrote: > On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote: >> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote: >>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: >>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: [...] >>> >>> All these SoCs should be capable of tweaking the block to fit their needs >>> by just the DT properties. I believe they differ only in the FIFO depth >>> and sometimes someone is greedy and uses 4:16 CS multiplexer, which is >>> an external passive component, but that's all. >>> >>> Would we need soc-specific compatible strings if this is the case? >> >> It's nice when most things can be supported with a small set of DT >> properties, as you've done. But IUIC, I think it's usually good practice >> to define and use SoC-specific (or maybe SoC family) compatible strings >> in the docs and DTS files, in addition to the generic one, in case there >> are future quirks that need to be handled. Note that you don't actually >> have to use these in the driver yet, but it's good to have a definition. >> So you can, today, have: >> >> foo@xxxx { >> compatible = "ti,baz-12345", "cdns,qspi-nor"; >> ... >> }; >> >> And we have the option to pick up "ti,baz-12345" in the Linux driver *if >> needed.* The support for TI SoC that has this IP is not in upstream yet. I will add TI-specific compatible later. It will be: foo@xxxx { compatible = "ti,k2g-qspi", "cdns,qspi-nor"; ... };
On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote: > On 02/02/2016 02:43 AM, Marek Vasut wrote: > > On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote: > >> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote: > >>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: > >>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: > [...] > > >>> All these SoCs should be capable of tweaking the block to fit their > >>> needs by just the DT properties. I believe they differ only in the > >>> FIFO depth and sometimes someone is greedy and uses 4:16 CS > >>> multiplexer, which is an external passive component, but that's all. > >>> > >>> Would we need soc-specific compatible strings if this is the case? > >> > >> It's nice when most things can be supported with a small set of DT > >> properties, as you've done. But IUIC, I think it's usually good practice > >> to define and use SoC-specific (or maybe SoC family) compatible strings > >> in the docs and DTS files, in addition to the generic one, in case there > >> are future quirks that need to be handled. Note that you don't actually > >> have to use these in the driver yet, but it's good to have a definition. > >> > >> So you can, today, have: > >> foo@xxxx { > >> > >> compatible = "ti,baz-12345", "cdns,qspi-nor"; > >> ... > >> > >> }; > >> > >> And we have the option to pick up "ti,baz-12345" in the Linux driver *if > >> needed.* > > The support for TI SoC that has this IP is not in upstream yet. I will > add TI-specific compatible later. It will be: > > foo@xxxx { > compatible = "ti,k2g-qspi", "cdns,qspi-nor"; > ... > }; Do you expect any specifics which cannot be handled by the current bindings btw? In my socfpga case, the compatible strings will be probably: "altr,socfpga-gen5-qspi" Dinh, Graham, do you agree with this or should we use something else ? Thanks! Best regards, Marek Vasut
On 02/04/2016 05:25 AM, Marek Vasut wrote: > On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote: >> On 02/02/2016 02:43 AM, Marek Vasut wrote: >>> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote: >>>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote: >>>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: >>>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: >> [...] >> >>>>> All these SoCs should be capable of tweaking the block to fit their >>>>> needs by just the DT properties. I believe they differ only in the >>>>> FIFO depth and sometimes someone is greedy and uses 4:16 CS >>>>> multiplexer, which is an external passive component, but that's all. >>>>> >>>>> Would we need soc-specific compatible strings if this is the case? >>>> >>>> It's nice when most things can be supported with a small set of DT >>>> properties, as you've done. But IUIC, I think it's usually good practice >>>> to define and use SoC-specific (or maybe SoC family) compatible strings >>>> in the docs and DTS files, in addition to the generic one, in case there >>>> are future quirks that need to be handled. Note that you don't actually >>>> have to use these in the driver yet, but it's good to have a definition. >>>> >>>> So you can, today, have: >>>> foo@xxxx { >>>> >>>> compatible = "ti,baz-12345", "cdns,qspi-nor"; >>>> ... >>>> >>>> }; >>>> >>>> And we have the option to pick up "ti,baz-12345" in the Linux driver *if >>>> needed.* >> >> The support for TI SoC that has this IP is not in upstream yet. I will >> add TI-specific compatible later. It will be: >> >> foo@xxxx { >> compatible = "ti,k2g-qspi", "cdns,qspi-nor"; >> ... >> }; > > Do you expect any specifics which cannot be handled by the current bindings btw? > In my socfpga case, the compatible strings will be probably: > > "altr,socfpga-gen5-qspi" > > Dinh, Graham, do you agree with this or should we use something else ? > I agree. Dinh
On 2/4/2016 4:55 PM, Marek Vasut wrote: > On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote: >> On 02/02/2016 02:43 AM, Marek Vasut wrote: >>> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote: >>>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote: >>>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: >>>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: >> [...] >> >>>>> All these SoCs should be capable of tweaking the block to fit their >>>>> needs by just the DT properties. I believe they differ only in the >>>>> FIFO depth and sometimes someone is greedy and uses 4:16 CS >>>>> multiplexer, which is an external passive component, but that's all. >>>>> >>>>> Would we need soc-specific compatible strings if this is the case? >>>> >>>> It's nice when most things can be supported with a small set of DT >>>> properties, as you've done. But IUIC, I think it's usually good practice >>>> to define and use SoC-specific (or maybe SoC family) compatible strings >>>> in the docs and DTS files, in addition to the generic one, in case there >>>> are future quirks that need to be handled. Note that you don't actually >>>> have to use these in the driver yet, but it's good to have a definition. >>>> >>>> So you can, today, have: >>>> foo@xxxx { >>>> >>>> compatible = "ti,baz-12345", "cdns,qspi-nor"; >>>> ... >>>> >>>> }; >>>> >>>> And we have the option to pick up "ti,baz-12345" in the Linux driver *if >>>> needed.* >> >> The support for TI SoC that has this IP is not in upstream yet. I will >> add TI-specific compatible later. It will be: >> >> foo@xxxx { >> compatible = "ti,k2g-qspi", "cdns,qspi-nor"; >> ... >> }; > > Do you expect any specifics which cannot be handled by the current bindings btw? > In my socfpga case, the compatible strings will be probably: > Yeah, there is delay(of few ns) required between writing to INDIRECTWR_START bit and actually writing data to flash(i.e writesl() call). This is specific to TI K2G SoC and needs to be tied to the new binding.
On Thursday, February 04, 2016 at 06:04:08 PM, Dinh Nguyen wrote: > On 02/04/2016 05:25 AM, Marek Vasut wrote: > > On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote: > >> On 02/02/2016 02:43 AM, Marek Vasut wrote: > >>> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote: > >>>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote: > >>>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: > >>>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: > >> [...] > >> > >>>>> All these SoCs should be capable of tweaking the block to fit their > >>>>> needs by just the DT properties. I believe they differ only in the > >>>>> FIFO depth and sometimes someone is greedy and uses 4:16 CS > >>>>> multiplexer, which is an external passive component, but that's all. > >>>>> > >>>>> Would we need soc-specific compatible strings if this is the case? > >>>> > >>>> It's nice when most things can be supported with a small set of DT > >>>> properties, as you've done. But IUIC, I think it's usually good > >>>> practice to define and use SoC-specific (or maybe SoC family) > >>>> compatible strings in the docs and DTS files, in addition to the > >>>> generic one, in case there are future quirks that need to be handled. > >>>> Note that you don't actually have to use these in the driver yet, but > >>>> it's good to have a definition. > >>>> > >>>> So you can, today, have: > >>>> foo@xxxx { > >>>> > >>>> compatible = "ti,baz-12345", "cdns,qspi-nor"; > >>>> ... > >>>> > >>>> }; > >>>> > >>>> And we have the option to pick up "ti,baz-12345" in the Linux driver > >>>> *if needed.* > >> > >> The support for TI SoC that has this IP is not in upstream yet. I will > >> > >> add TI-specific compatible later. It will be: > >> foo@xxxx { > >> > >> compatible = "ti,k2g-qspi", "cdns,qspi-nor"; > >> > >> ... > >> > >> }; > > > > Do you expect any specifics which cannot be handled by the current > > bindings btw? In my socfpga case, the compatible strings will be > > probably: > > > > "altr,socfpga-gen5-qspi" > > > > Dinh, Graham, do you agree with this or should we use something else ? > > I agree. Thanks Best regards, Marek Vasut
On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote: > On 2/4/2016 4:55 PM, Marek Vasut wrote: > > On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote: > >> On 02/02/2016 02:43 AM, Marek Vasut wrote: > >>> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote: > >>>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote: > >>>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote: > >>>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote: > >> [...] > >> > >>>>> All these SoCs should be capable of tweaking the block to fit their > >>>>> needs by just the DT properties. I believe they differ only in the > >>>>> FIFO depth and sometimes someone is greedy and uses 4:16 CS > >>>>> multiplexer, which is an external passive component, but that's all. > >>>>> > >>>>> Would we need soc-specific compatible strings if this is the case? > >>>> > >>>> It's nice when most things can be supported with a small set of DT > >>>> properties, as you've done. But IUIC, I think it's usually good > >>>> practice to define and use SoC-specific (or maybe SoC family) > >>>> compatible strings in the docs and DTS files, in addition to the > >>>> generic one, in case there are future quirks that need to be handled. > >>>> Note that you don't actually have to use these in the driver yet, but > >>>> it's good to have a definition. > >>>> > >>>> So you can, today, have: > >>>> foo@xxxx { > >>>> > >>>> compatible = "ti,baz-12345", "cdns,qspi-nor"; > >>>> ... > >>>> > >>>> }; > >>>> > >>>> And we have the option to pick up "ti,baz-12345" in the Linux driver > >>>> *if needed.* > >> > >> The support for TI SoC that has this IP is not in upstream yet. I will > >> > >> add TI-specific compatible later. It will be: > >> foo@xxxx { > >> > >> compatible = "ti,k2g-qspi", "cdns,qspi-nor"; > >> > >> ... > >> > >> }; > > > > Do you expect any specifics which cannot be handled by the current > > bindings btw? > > > In my socfpga case, the compatible strings will be probably: > Yeah, there is delay(of few ns) required between writing to > INDIRECTWR_START bit and actually writing data to flash(i.e writesl() > call). This is specific to TI K2G SoC and needs to be tied to the new > binding. Can't you somehow poll the hardware to check whether or not it's ready instead of adding some random delay ? Best regards, Marek Vasut
On 02/06/2016 01:12 PM, Marek Vasut wrote: > On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote: >> On 2/4/2016 4:55 PM, Marek Vasut wrote: [...] >> Yeah, there is delay(of few ns) required between writing to >> INDIRECTWR_START bit and actually writing data to flash(i.e writesl() >> call). This is specific to TI K2G SoC and needs to be tied to the new >> binding. > > Can't you somehow poll the hardware to check whether or not it's ready instead > of adding some random delay ? There is no dedicated register to poll as such. According to TRM: "Wait for couple of cycles of QSPI_REF_CLK(functional clock of QSPI @384MHz) until CQSPI_REG_INDIRECTWR[0] bit is internally synchronized by the QSPI module before writing to flash". So, a few ns(~6ns @384MHz) delay is needed (or accessing a QSPI module register should be sufficient as it will take more than 2 clock cycles). I believe this delay is specific to TI K2G SoC and maybe needs to be tied to the binding.
On Monday, February 08, 2016 at 12:19:25 PM, Vignesh R wrote: > On 02/06/2016 01:12 PM, Marek Vasut wrote: > > On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote: > >> On 2/4/2016 4:55 PM, Marek Vasut wrote: > [...] > > >> Yeah, there is delay(of few ns) required between writing to > >> INDIRECTWR_START bit and actually writing data to flash(i.e writesl() > >> call). This is specific to TI K2G SoC and needs to be tied to the new > >> binding. > > > > Can't you somehow poll the hardware to check whether or not it's ready > > instead of adding some random delay ? > > There is no dedicated register to poll as such. > > According to TRM: > "Wait for couple of cycles of QSPI_REF_CLK(functional clock of QSPI > @384MHz) until CQSPI_REG_INDIRECTWR[0] bit is internally synchronized by > the QSPI module before writing to flash". > > So, a few ns(~6ns @384MHz) delay is needed (or accessing a QSPI module > register should be sufficient as it will take more than 2 clock cycles). > I believe this delay is specific to TI K2G SoC and maybe needs to be > tied to the binding. OK, got it. Dinh/Graham, can you check if this might be needed on SoCFPGA too please? Best regards, Marek Vasut
On 02/08/2016 09:27 AM, Marek Vasut wrote: > On Monday, February 08, 2016 at 12:19:25 PM, Vignesh R wrote: >> On 02/06/2016 01:12 PM, Marek Vasut wrote: >>> On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote: >>>> On 2/4/2016 4:55 PM, Marek Vasut wrote: >> [...] >> >>>> Yeah, there is delay(of few ns) required between writing to >>>> INDIRECTWR_START bit and actually writing data to flash(i.e writesl() >>>> call). This is specific to TI K2G SoC and needs to be tied to the new >>>> binding. >>> >>> Can't you somehow poll the hardware to check whether or not it's ready >>> instead of adding some random delay ? >> >> There is no dedicated register to poll as such. >> >> According to TRM: >> "Wait for couple of cycles of QSPI_REF_CLK(functional clock of QSPI >> @384MHz) until CQSPI_REG_INDIRECTWR[0] bit is internally synchronized by >> the QSPI module before writing to flash". >> >> So, a few ns(~6ns @384MHz) delay is needed (or accessing a QSPI module >> register should be sufficient as it will take more than 2 clock cycles). >> I believe this delay is specific to TI K2G SoC and maybe needs to be >> tied to the binding. > > OK, got it. Dinh/Graham, can you check if this might be needed on SoCFPGA too > please? > I don't see any such requirement in the data sheet. It's working without it. So I think it's not needed on SoCFPGA -Graham
On 02/10/2016 05:10 PM, Graham Moore wrote: > On 02/08/2016 09:27 AM, Marek Vasut wrote: >> On Monday, February 08, 2016 at 12:19:25 PM, Vignesh R wrote: >>> On 02/06/2016 01:12 PM, Marek Vasut wrote: >>>> On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote: >>>>> On 2/4/2016 4:55 PM, Marek Vasut wrote: >>> [...] >>> >>>>> Yeah, there is delay(of few ns) required between writing to >>>>> INDIRECTWR_START bit and actually writing data to flash(i.e writesl() >>>>> call). This is specific to TI K2G SoC and needs to be tied to the new >>>>> binding. >>>> >>>> Can't you somehow poll the hardware to check whether or not it's ready >>>> instead of adding some random delay ? >>> >>> There is no dedicated register to poll as such. >>> >>> According to TRM: >>> "Wait for couple of cycles of QSPI_REF_CLK(functional clock of QSPI >>> @384MHz) until CQSPI_REG_INDIRECTWR[0] bit is internally synchronized by >>> the QSPI module before writing to flash". >>> >>> So, a few ns(~6ns @384MHz) delay is needed (or accessing a QSPI module >>> register should be sufficient as it will take more than 2 clock cycles). >>> I believe this delay is specific to TI K2G SoC and maybe needs to be >>> tied to the binding. >> >> OK, got it. Dinh/Graham, can you check if this might be needed on >> SoCFPGA too >> please? >> > > I don't see any such requirement in the data sheet. It's working > without it. So I think it's not needed on SoCFPGA All right, so we will just add a special property or compat string for the TI SoC. But we need to get this driver mainlined first :)
On 02/10/2016 10:17 AM, Marek Vasut wrote: [...] > All right, so we will just add a special property or compat string for > the TI SoC. But we need to get this driver mainlined first :) > Hi Marek, How's that mainlining going? You probably noticed this patch needed some refactoring for 4.4. In the course of testing, we realized this driver needs to enable its clock. Do you have a 4.4 or later version in your tree? I'd like to add the clock enabling. -Graham
On 03/10/2016 09:55 PM, Graham Moore wrote: > On 02/10/2016 10:17 AM, Marek Vasut wrote: > > [...] > >> All right, so we will just add a special property or compat string for >> the TI SoC. But we need to get this driver mainlined first :) >> > > Hi Marek, Hi Graham, > How's that mainlining going? You probably noticed this patch needed > some refactoring for 4.4. In the course of testing, we realized this > driver needs to enable its clock. > > Do you have a 4.4 or later version in your tree? I'd like to add the > clock enabling. Still waiting for the patches from Cyrille to go in, so this patch is stuck. I just checked next and some of them made it, but there are more which didn't. I have a V10 of this patch , it is in the ML already [1], so try playing around with that. I have linux 4.4 branch with all the necessary patches backported and/or applied if you'd be interested in that, but I do my development on linux-next . What sort of clock patch are you missing ? [1] https://patchwork.ozlabs.org/patch/565599/ > -Graham
On 03/10/2016 03:10 PM, Marek Vasut wrote: > On 03/10/2016 09:55 PM, Graham Moore wrote: >> On 02/10/2016 10:17 AM, Marek Vasut wrote: >> >> [...] >> >>> All right, so we will just add a special property or compat string for >>> the TI SoC. But we need to get this driver mainlined first :) >>> >> >> Hi Marek, > > Hi Graham, > >> How's that mainlining going? You probably noticed this patch needed >> some refactoring for 4.4. In the course of testing, we realized this >> driver needs to enable its clock. >> >> Do you have a 4.4 or later version in your tree? I'd like to add the >> clock enabling. > > Still waiting for the patches from Cyrille to go in, so this patch is > stuck. I just checked next and some of them made it, but there are more > which didn't. I have a V10 of this patch , it is in the ML already [1], > so try playing around with that. > > I have linux 4.4 branch with all the necessary patches backported and/or > applied if you'd be interested in that, but I do my development on > linux-next . > > What sort of clock patch are you missing ? > The clock needs to be enabled, after the devm_clk_get(). I've added a clk_prepare_enable() there, and also clk_disable_unprepare() in the remove function. That ML patch won't apply to any tree I have, 4.4, 4.5, linux-next, etc. Maybe I'll just wait until it's in linux-next. -Graham
On 03/14/2016 07:17 PM, Graham Moore wrote: > On 03/10/2016 03:10 PM, Marek Vasut wrote: >> On 03/10/2016 09:55 PM, Graham Moore wrote: >>> On 02/10/2016 10:17 AM, Marek Vasut wrote: >>> >>> [...] >>> >>>> All right, so we will just add a special property or compat string for >>>> the TI SoC. But we need to get this driver mainlined first :) >>>> >>> >>> Hi Marek, >> >> Hi Graham, >> >>> How's that mainlining going? You probably noticed this patch needed >>> some refactoring for 4.4. In the course of testing, we realized this >>> driver needs to enable its clock. >>> >>> Do you have a 4.4 or later version in your tree? I'd like to add the >>> clock enabling. >> >> Still waiting for the patches from Cyrille to go in, so this patch is >> stuck. I just checked next and some of them made it, but there are more >> which didn't. I have a V10 of this patch , it is in the ML already [1], >> so try playing around with that. >> >> I have linux 4.4 branch with all the necessary patches backported and/or >> applied if you'd be interested in that, but I do my development on >> linux-next . >> >> What sort of clock patch are you missing ? >> > > The clock needs to be enabled, after the devm_clk_get(). I've added a > clk_prepare_enable() there, and also clk_disable_unprepare() in the > remove function. Yeah, please send me a patch and I will add it into the driver, add your SoB line and repost. > That ML patch won't apply to any tree I have, 4.4, 4.5, linux-next, etc. > Maybe I'll just wait until it's in linux-next. I'll send you the backport to 4.4 off-list, so I don't bother people here. > -Graham >
diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt new file mode 100644 index 0000000..f248056 --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt @@ -0,0 +1,56 @@ +* Cadence Quad SPI controller + +Required properties: +- compatible : Should be "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. + +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. + +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>; + + flash0: n25q00@0 { + ... + cdns,read-delay = <4>; + cdns,tshsl-ns = <50>; + cdns,tsd2d-ns = <50>; + cdns,tchsh-ns = <4>; + cdns,tslch-ns = <4>; + }; + };