| Message ID | 1378986619-26765-2-git-send-email-pekon@ti.com |
|---|---|
| State | Not Applicable |
| Headers | show |
> > This patch > - updates DT binding for selection of ecc-scheme > - updates DT binding for detection of ELM h/w engine > - removes following obselete ECC schemes > OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming > ECC) > OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit > Hamming ECC scheme) > - updates DT binding documentation for mtd/gpmc-nand > > Signed-off-by: Pekon Gupta <pekon@ti.com> > --- Dear Olof and other DT Maintainers, This patch series has missed multiple merge windows, and much of the other development work on mtd/nand/omap driver is gated due to this. So, request you to please review this patch set and Ack it if all your mentioned concerns are resolved. With regards, pekon
+ akpm On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote: > > > > > This patch > > - updates DT binding for selection of ecc-scheme > > - updates DT binding for detection of ELM h/w engine > > - removes following obselete ECC schemes > > OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming > > ECC) > > OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit > > Hamming ECC scheme) > > - updates DT binding documentation for mtd/gpmc-nand > > > > Signed-off-by: Pekon Gupta <pekon@ti.com> > > --- > > Dear Olof and other DT Maintainers, > > This patch series has missed multiple merge windows, and > much of the other development work on mtd/nand/omap > driver is gated due to this. > So, request you to please review this patch set and Ack it > if all your mentioned concerns are resolved.
On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote: > + akpm > > On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote: > > > > > > > > This patch > > > - updates DT binding for selection of ecc-scheme > > > - updates DT binding for detection of ELM h/w engine > > > - removes following obselete ECC schemes > > > OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming > > > ECC) > > > OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit > > > Hamming ECC scheme) > > > - updates DT binding documentation for mtd/gpmc-nand > > > > > > Signed-off-by: Pekon Gupta <pekon@ti.com> > > > --- > > > > Dear Olof and other DT Maintainers, > > > > This patch series has missed multiple merge windows, and > > much of the other development work on mtd/nand/omap > > driver is gated due to this. > > So, request you to please review this patch set and Ack it > > if all your mentioned concerns are resolved. I've been waiting on Olof's response for this one. It seems like you addressed his comments well enough for me, although (in line with his comments) you are admittedly still breaking the DT ABI for these devices. IMO, that's OK given the low quality of the original bindings. I will make my own last pass at this series and push it to l2-mtd.git if it looks OK. Brian
Hi Pekon, On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote: > + akpm > > On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote: > > > > > > > > This patch > > > - updates DT binding for selection of ecc-scheme > > > - updates DT binding for detection of ELM h/w engine > > > - removes following obselete ECC schemes > > > OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming > > > ECC) > > > OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit > > > Hamming ECC scheme) > > > - updates DT binding documentation for mtd/gpmc-nand > > > > > > Signed-off-by: Pekon Gupta <pekon@ti.com> > > > --- > > > > Dear Olof and other DT Maintainers, > > > > This patch series has missed multiple merge windows, and > > much of the other development work on mtd/nand/omap > > driver is gated due to this. Also, to be fair here: you only started CC'ing the appropriate DT people/list around v4, after which you got a (somewhat) prompt and thorough review of the DT bindings. Then several months passed before you addressed the reviews. So the "multiple merge windows" is not entirely to be blamed on others ;) Brian
On Thu, Sep 12, 2013 at 05:20:16PM +0530, Pekon Gupta wrote: > OMAP NAND driver support multiple ECC scheme, which can used in following > different flavours, depending on in-build Hardware engines supported by SoC. > > +---------------------------------------+---------------+---------------+ > | ECC scheme |ECC calculation|Error detection| > +---------------------------------------+---------------+---------------+ > |OMAP_ECC_HAMMING_CODE_HW |H/W (GPMC) |S/W | > +---------------------------------------+---------------+---------------+ > |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |H/W (GPMC) |S/W | > |(requires CONFIG_MTD_NAND_ECC_BCH) | | | > +---------------------------------------+---------------+---------------+ > |OMAP_ECC_BCH8_CODE_HW |H/W (GPMC) |H/W (ELM) | > |(requires CONFIG_MTD_NAND_OMAP_BCH && | | | > | ti,elm-id in DT) | | | > +---------------------------------------+---------------+---------------+ > To optimize footprint of omap2-nand driver, selection of some ECC schemes > also require enabling following Kconfigs, in addition to setting appropriate > DT bindings > - Kconfig:CONFIG_MTD_NAND_ECC_BCH enables S/W based BCH ECC algorithm > - Kconfig:CONFIG_MTD_NAND_OMAP_BCH enables H/W based BCH ECC algorithm > > This patch > - updates DT binding for selection of ecc-scheme > - updates DT binding for detection of ELM h/w engine > - removes following obselete ECC schemes > OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming ECC) > OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit Hamming ECC scheme) > - updates DT binding documentation for mtd/gpmc-nand > > Signed-off-by: Pekon Gupta <pekon@ti.com> > --- > .../devicetree/bindings/mtd/gpmc-nand.txt | 14 +++---- > arch/arm/mach-omap2/board-flash.c | 2 +- > arch/arm/mach-omap2/gpmc.c | 47 +++++++++++++++------- > include/linux/platform_data/mtd-nand-omap2.h | 23 +++++++---- > 4 files changed, 56 insertions(+), 30 deletions(-) > ... > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c > index 9f4795a..6409884 100644 > --- a/arch/arm/mach-omap2/gpmc.c > +++ b/arch/arm/mach-omap2/gpmc.c ... > @@ -1378,12 +1371,36 @@ static int gpmc_probe_nand_child(struct platform_device *pdev, > gpmc_nand_data->cs = val; > gpmc_nand_data->of_node = child; > > - if (!of_property_read_string(child, "ti,nand-ecc-opt", &s)) > - for (val = 0; val < ARRAY_SIZE(nand_ecc_opts); val++) > - if (!strcasecmp(s, nand_ecc_opts[val])) { > - gpmc_nand_data->ecc_opt = val; > - break; > - } > + /* Detect availability of ELM module */ > + parp = of_get_property(child, "ti,elm-id", &lenp); > + if ((parp == NULL) && (lenp != (sizeof(void *) * 2))) { I think && should be || > + pr_warn("%s: ti,elm-id property not found\n", __func__); > + gpmc_nand_data->elm_of_node = NULL; > + } else { > + gpmc_nand_data->elm_of_node = > + of_find_node_by_phandle(be32_to_cpup(parp)); > + } > + /* select NAND ecc-scheme */ > + if (of_property_read_string(child, "ti,nand-ecc-scheme", &s)) { > + pr_err("%s: valid ti,nand-ecc-scheme not found\n", __func__); > + return -ENODEV; > + } > + if (!strcmp(s, "ham1")) > + gpmc_nand_data->ecc_opt = OMAP_ECC_HAMMING_CODE_HW; > + else if (!strcmp(s, "bch4")) > + if (gpmc_nand_data->elm_of_node) > + gpmc_nand_data->ecc_opt = OMAP_ECC_BCH4_CODE_HW; > + else > + gpmc_nand_data->ecc_opt = > + OMAP_ECC_BCH4_CODE_HW_DETECTION_SW; > + else if (!strcmp(s, "bch8")) > + if (gpmc_nand_data->elm_of_node) > + gpmc_nand_data->ecc_opt = OMAP_ECC_BCH8_CODE_HW; > + else > + gpmc_nand_data->ecc_opt = > + OMAP_ECC_BCH8_CODE_HW_DETECTION_SW; > + else > + pr_err("%s: ti,ecc-scheme: invalid property value\n", __func__); > > if (!of_property_read_string(child, "ti,nand-xfer-type", &s)) > for (val = 0; val < ARRAY_SIZE(nand_xfer_types); val++) Otherwise, I think this patch is OK. Brian
Hi Brian, > > Hi Pekon, > > On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote: > > + akpm > > > > On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote: [snip] > > > > > > Dear Olof and other DT Maintainers, > > > > > > This patch series has missed multiple merge windows, and > > > much of the other development work on mtd/nand/omap > > > driver is gated due to this. > > Also, to be fair here: you only started CC'ing the appropriate DT > people/list around v4, after which you got a (somewhat) prompt and > thorough review of the DT bindings. Then several months passed before > you addressed the reviews. So the "multiple merge windows" is not > entirely to be blamed on others ;) > > Brian Few points I would like to clarify here, *without* pointing anyone.. (1) It was Olof's comments which directed me to cc: devicetree-discuss list. So wrong list was always cc-ed till v4. Refer http://permalink.gmane.org/gmane.linux.ports.arm.kernel/249662 (2) The devicetree list has been updated in MAINTAINER file towards end of July (22/July/2013) in following commit. Whereas the patch v4 was submitted on 2/July/2013. So I wasn't aware of this new DT maillist. commit d0fb18c5c0caf2ed0eecf3d0145450ae708ed75a Commit: Grant Likely <grant.likely@linaro.org> CommitDate: 2013-07-22 (3) When a maintainer gives a NAK, I expect him to at-least give directions on what to change in the patch. There were no comments given, neither new patch reviewed by the DT maintainer even after sending multiple request directly. http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html http://lists.infradead.org/pipermail/linux-mtd/2013-July/047629.html Its only when Artem and yourself pitched in by sending a mail to new DT mail-list that the DT maintainer reviewed and provided the comments for fixes. But by that time 3.11-rc6 had already gone past, and so I knew this Series cannot go in, so I wanted to wait some more, to see if there were any more comments on this. And frankly I was too much frustrated by then. Just to speak my opinion. We all understand that maintainers are heavily loaded by tons of emails, And reviewing each patch instantly is not possible. But this load is now passing to developers like me as frustration, because most of energy is being spent in re-submitting patches again and again, without any proper direction or conclusion. And then there is no time and energy left for good work, where we can contribute to optimizing frame-works for performance or adding new features. So I see many good developers distracting away from mainline. Thus, there should be a mechanism, where such load can be distributed, and there are less emails. And developers don't have to re-submit patch multiple times, by collecting most reviews at once. This way developer's like me can spend more time in doing other constructive things. Please consider this as constructive feedback, without targeting any individual or team. with regards, pekon
On Wed, Sep 25, 2013 at 07:24:26PM +0000, Gupta, Pekon wrote: > > On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote: > > > + akpm > > > > > > On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote: > [snip] > > > > > > > > Dear Olof and other DT Maintainers, > > > > > > > > This patch series has missed multiple merge windows, and > > > > much of the other development work on mtd/nand/omap > > > > driver is gated due to this. > > > > Also, to be fair here: you only started CC'ing the appropriate DT > > people/list around v4, after which you got a (somewhat) prompt and > > thorough review of the DT bindings. Then several months passed before > > you addressed the reviews. So the "multiple merge windows" is not > > entirely to be blamed on others ;) > > > > Brian > > Few points I would like to clarify here, *without* pointing anyone.. > > (1) It was Olof's comments which directed me to cc: devicetree-discuss list. > So wrong list was always cc-ed till v4. > Refer http://permalink.gmane.org/gmane.linux.ports.arm.kernel/249662 > > (2) The devicetree list has been updated in MAINTAINER file towards > end of July (22/July/2013) in following commit. Whereas the patch v4 > was submitted on 2/July/2013. So I wasn't aware of this new DT maillist. > commit d0fb18c5c0caf2ed0eecf3d0145450ae708ed75a > Commit: Grant Likely <grant.likely@linaro.org> > CommitDate: 2013-07-22 Did you not notice the 'bounce' emails once Grant made the (IMO unwise) decision to shut down the DT mailing list instead of just redirecting? > (3) When a maintainer gives a NAK, I expect him to at-least give > directions on what to change in the patch. There were no comments > given, neither new patch reviewed by the DT maintainer even after > sending multiple request directly. > http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html > http://lists.infradead.org/pipermail/linux-mtd/2013-July/047629.html Those are links to your own emails, not to any NAKs. Any NAKs I saw listed reasons. I NAK'd based on the ABI breakage and lack of review by the DT maintainership; Arnd NAK'd based on similar reasons; Olof later (once you got the right list) gave constructive criticism on how to remove the Linux-isms and other software implementation details from the DT binding. > Its only when Artem and yourself pitched in by sending a mail to > new DT mail-list that the DT maintainer reviewed and provided the > comments for fixes. > But by that time 3.11-rc6 had already gone past, and so I knew this > Series cannot go in, so I wanted to wait some more, to see if there > were any more comments on this. And frankly I was too much > frustrated by then. > > Just to speak my opinion. > We all understand that maintainers are heavily loaded by tons of > emails, And reviewing each patch instantly is not possible. > > But this load is now passing to developers like me as frustration, > because most of energy is being spent in re-submitting patches > again and again, without any proper direction or conclusion. > And then there is no time and energy left for good work, where > we can contribute to optimizing frame-works for performance or > adding new features. > So I see many good developers distracting away from mainline. I sincerely hope that this isn't (or at least doesn't continue to be) the norm. > Thus, there should be a mechanism, where such load can be > distributed, and there are less emails. And developers don't > have to re-submit patch multiple times, by collecting most reviews > at once. This way developer's like me can spend more time in doing > other constructive things. I understand your concerns. I have been frustrated with slow responses on MTD stuff as well, which is why I'm stepping in to review and commit more. But I see that your patch hit a unique combination of events that helped slow things down even more than usual. MTD maintainership has been slowing down for a while, and in the midst of your patch series, I began stepping in to take care of some of that load. I didn't quite have a good picture of what patches were pending without comments at that point, so patch series like yours were in limbo with no one to review. Additionally, Grant stepped down from DT maintainership and correspondingly shut down and transitioned the DT mailing list. This directly affected your patch series. Lastly, it is extra difficult to deal with patch sets like this that cross subsystems and require reviews from multiple constituencies. And that is even more difficult when you aren't even emailing the correct people (for whatever reason). I believe that many of the factors that slowed down your particular series have been ameliorated. Time has allowed developers to become more aware of the change in DT maintainership and mailing list, and I can more promptly redirect developers who are still CC'ing the wrong people/lists. I am able to spend a little more time on MTD stuff, to help push development along a little faster. Olof has given good advice on your DT binding and has (slowly) been responding to other requests for DT review that make it to his list. I see that he hasn't followed up on your changes (this v6), so pinging him (as you did) is probably the correct approach. But please do recognize that the DT list is very high volume, so it's hard to get good timely responses there. Anyway, at this point I think your patch series should be nearly complete. I made a few comments on your patches, and I'd imagine you only should need one more revision (v7) before I can accept it to the l2-mtd.git tree. > Please consider this as constructive feedback, without targeting > any individual or team. Thank you for your thoughtful response. Brian
On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris <computersforpeace@gmail.com> wrote: > Olof has given good advice on your DT binding and has (slowly) been > responding to other requests for DT review that make it to his list. I > see that he hasn't followed up on your changes (this v6), so pinging him > (as you did) is probably the correct approach. But please do recognize > that the DT list is very high volume, so it's hard to get good timely > responses there. I am not a DT mainainer, but sometimes when I see a binding that appears to be wrong I speak up. In this case, the binding was one of those. That doesn't mean you should necessarily rely on me for the rest of the review, there are several dedicated maintainers right now that should be able to spread the load across them, and it is their ack you should seek on the binding, not mine. That being said: this latest version looks good to me. See how much simpler the binding got when you stopped trying to describe driver behavior and focused on describing hardware? That's the way we want it to look like. So, I have no more objections to it, and I hope you can get a quick review from a DT maintainer on the rest of the binding. -Olof
On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote: > On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > > Olof has given good advice on your DT binding and has (slowly) been > > responding to other requests for DT review that make it to his list. I > > see that he hasn't followed up on your changes (this v6), so pinging him > > (as you did) is probably the correct approach. But please do recognize > > that the DT list is very high volume, so it's hard to get good timely > > responses there. > > I am not a DT mainainer, but sometimes when I see a binding that > appears to be wrong I speak up. In this case, the binding was one of > those. Whoops, my bad. I was deceived by the responses I've seen from you on other issues (thanks, BTW). In that case, I haven't seen any response from a proper DT binding maintainer :( > So, I have no more objections to it, and I hope you can get a quick > review from a DT maintainer on the rest of the binding. At this point, I'm comfortable going ahead without their ack, since they obviously don't care/don't have the manpower to review. Brian
On Wed, Sep 25, 2013 at 2:29 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote: >> On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris >> <computersforpeace@gmail.com> wrote: >> >> > Olof has given good advice on your DT binding and has (slowly) been >> > responding to other requests for DT review that make it to his list. I >> > see that he hasn't followed up on your changes (this v6), so pinging him >> > (as you did) is probably the correct approach. But please do recognize >> > that the DT list is very high volume, so it's hard to get good timely >> > responses there. >> >> I am not a DT mainainer, but sometimes when I see a binding that >> appears to be wrong I speak up. In this case, the binding was one of >> those. > > Whoops, my bad. I was deceived by the responses I've seen from you on > other issues (thanks, BTW). In that case, I haven't seen any response > from a proper DT binding maintainer :( > >> So, I have no more objections to it, and I hope you can get a quick >> review from a DT maintainer on the rest of the binding. > > At this point, I'm comfortable going ahead without their ack, since they > obviously don't care/don't have the manpower to review. No, that is not how we handle device tree bindings. They need to be reviewed, since we are moving over to a model where they will be considered ABI and can't be changed after the fact. We have a long backlog of mostly-unreviewed old bindings that we're going to do a pass through and then lock down, but it would be good to not add to that backlog with newer bindings. In other words, there's a strong desire for actual acks on bindings from those maintainers these days. -Olof
On Wed, Sep 25, 2013 at 2:32 PM, Olof Johansson <olof@lixom.net> wrote: > On Wed, Sep 25, 2013 at 2:29 PM, Brian Norris > <computersforpeace@gmail.com> wrote: >> On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote: >>> On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris >>> <computersforpeace@gmail.com> wrote: >>> >>> > Olof has given good advice on your DT binding and has (slowly) been >>> > responding to other requests for DT review that make it to his list. I >>> > see that he hasn't followed up on your changes (this v6), so pinging him >>> > (as you did) is probably the correct approach. But please do recognize >>> > that the DT list is very high volume, so it's hard to get good timely >>> > responses there. >>> >>> I am not a DT mainainer, but sometimes when I see a binding that >>> appears to be wrong I speak up. In this case, the binding was one of >>> those. >> >> Whoops, my bad. I was deceived by the responses I've seen from you on >> other issues (thanks, BTW). In that case, I haven't seen any response >> from a proper DT binding maintainer :( >> >>> So, I have no more objections to it, and I hope you can get a quick >>> review from a DT maintainer on the rest of the binding. >> >> At this point, I'm comfortable going ahead without their ack, since they >> obviously don't care/don't have the manpower to review. > > No, that is not how we handle device tree bindings. They need to be > reviewed, since we are moving over to a model where they will be > considered ABI and can't be changed after the fact. We have a long > backlog of mostly-unreviewed old bindings that we're going to do a > pass through and then lock down, but it would be good to not add to > that backlog with newer bindings. > > In other words, there's a strong desire for actual acks on bindings > from those maintainers these days. This only works if we get a response. I'll repeat this fact: I have seen absolutely zero response from any DT maintainer regarding this binding, and they've been CC'd in some capacity since July: Old revision from July (cross-posted, including the old DT list): http://thread.gmane.org/gmane.linux.ports.arm.omap/100484/ New list: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg95238.html All official DT binding maintainers are CC'd here: you can't say you want more review of bindings, yet fail to review them across 3 versions and almost 3 months. Ball's in your court. Brian
On Thu, Sep 12, 2013 at 6:50 AM, Pekon Gupta <pekon@ti.com> wrote: > OMAP NAND driver support multiple ECC scheme, which can used in following > different flavours, depending on in-build Hardware engines supported by SoC. > > +---------------------------------------+---------------+---------------+ > | ECC scheme |ECC calculation|Error detection| > +---------------------------------------+---------------+---------------+ > |OMAP_ECC_HAMMING_CODE_HW |H/W (GPMC) |S/W | > +---------------------------------------+---------------+---------------+ > |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |H/W (GPMC) |S/W | > |(requires CONFIG_MTD_NAND_ECC_BCH) | | | > +---------------------------------------+---------------+---------------+ > |OMAP_ECC_BCH8_CODE_HW |H/W (GPMC) |H/W (ELM) | > |(requires CONFIG_MTD_NAND_OMAP_BCH && | | | > | ti,elm-id in DT) | | | > +---------------------------------------+---------------+---------------+ > To optimize footprint of omap2-nand driver, selection of some ECC schemes > also require enabling following Kconfigs, in addition to setting appropriate > DT bindings > - Kconfig:CONFIG_MTD_NAND_ECC_BCH enables S/W based BCH ECC algorithm > - Kconfig:CONFIG_MTD_NAND_OMAP_BCH enables H/W based BCH ECC algorithm > > This patch > - updates DT binding for selection of ecc-scheme > - updates DT binding for detection of ELM h/w engine > - removes following obselete ECC schemes > OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming ECC) > OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit Hamming ECC scheme) > - updates DT binding documentation for mtd/gpmc-nand > > Signed-off-by: Pekon Gupta <pekon@ti.com> > --- > .../devicetree/bindings/mtd/gpmc-nand.txt | 14 +++---- > arch/arm/mach-omap2/board-flash.c | 2 +- > arch/arm/mach-omap2/gpmc.c | 47 +++++++++++++++------- > include/linux/platform_data/mtd-nand-omap2.h | 23 +++++++---- > 4 files changed, 56 insertions(+), 30 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > index df338cb..958e5d5 100644 > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > @@ -21,11 +21,8 @@ Optional properties: > is wired that way. If not specified, a bus > width of 8 is assumed. > > - - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > - > - "sw" Software method (default) > - "hw" Hardware method > - "hw-romcode" gpmc hamming mode method & romcode layout > + - ti,nand-ecc-scheme: A string setting the ECC layout to use. One of: > + "ham1" 1-bit Hamming ecc code As has been pointed out, this breaks compatibility which should be avoided unless you know the state of use of this binding. I fail to see the advantage of using scheme over opt. You could simply add ham1 here and maintain compatibility. Instead of removing sw, hw, etc. you can simply deprecate them or decide that the kernel doesn't support those options. However, since you are willing to break compatibility, then you should the generic NAND binding and use nand-ecc-mode adding the values you need. Documentation/devicetree/bindings/mtd/nand.txt: * MTD generic binding - nand-ecc-mode : String, operation mode of the NAND ecc mode. Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first", "soft_bch". - nand-bus-width : 8 or 16 bus width if not present 8 - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false > "bch4" 4-bit BCH ecc code > "bch8" 8-bit BCH ecc code > > @@ -36,8 +33,11 @@ Optional properties: > "prefetch-dma" Prefetch enabled sDMA mode > "prefetch-irq" Prefetch enabled irq mode > > - - elm_id: Specifies elm device node. This is required to support BCH > - error correction using ELM module. > + - ti,elm-id: Specifies pHandle of the ELM devicetree node. > + ELM is an on-chip hardware engine on TI SoC which is used for > + locating ECC errors for BCHx algorithms. SoC devices which have > + ELM hardware engines should specify this device node in .dtsi > + Using ELM for ECC error correction frees some CPU cycles. While yes, this is better name and documentation, I don't know that breaking compatibility is justified. Rob
Hi, > > > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > index df338cb..958e5d5 100644 > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > @@ -21,11 +21,8 @@ Optional properties: > > is wired that way. If not specified, a bus > > width of 8 is assumed. > > > > - - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > > - > > - "sw" Software method (default) > > - "hw" Hardware method > > - "hw-romcode" gpmc hamming mode method & romcode layout > > + - ti,nand-ecc-scheme: A string setting the ECC layout to use. One of: > > + "ham1" 1-bit Hamming ecc code > > As has been pointed out, this breaks compatibility which should be > avoided unless you know the state of use of this binding. I fail to > see the advantage of using scheme over opt. You could simply add ham1 > here and maintain compatibility. Instead of removing sw, hw, etc. you > can simply deprecate them or decide that the kernel doesn't support > those options. > Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier comments from Olof. So either way is fine with me. Should I revert it back to 'ti,nand-ecc-opt' ? Also, "sw", "hw_romcode" have been deprecated, they are no longer supported in driver. So shouldn't they be removed from the documentation ? > However, since you are willing to break compatibility, then you should > the generic NAND binding and use nand-ecc-mode adding the values you > need. > > Documentation/devicetree/bindings/mtd/nand.txt: > * MTD generic binding > > - nand-ecc-mode : String, operation mode of the NAND ecc mode. > Supported values are: "none", "soft", "hw", "hw_syndrome", > "hw_oob_first", > "soft_bch". Yes I can use generic 'nand-ecc-mode', But the binding values like "soft", "hw", "soft_bch" are too generic to describe the hardware. - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16 so "soft_bch" is ambiguous. - Similarly "soft" and "hw" do not determine the algorithm used like Hamming or BCH. (a) Should I update the generic binding values (if no one else is using) ? like sw -> ham1_sw hw -> ham1_hw soft_bch-> soft_bch4, soft_bch8 OR (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ? keeping the old ones intact. And how should I handle un-supported values, (Is pr_err during kernel probe enough ?). [...] > > - - elm_id: Specifies elm device node. This is required to support BCH > > - error correction using ELM module. > > + - ti,elm-id: Specifies pHandle of the ELM devicetree node. > > + ELM is an on-chip hardware engine on TI SoC which is used for > > + locating ECC errors for BCHx algorithms. SoC devices which have > > + ELM hardware engines should specify this device node in .dtsi > > + Using ELM for ECC error correction frees some CPU cycles. > > While yes, this is better name and documentation, I don't know that > breaking compatibility is justified. > As this is TI specific binding, so manufacturer's name should have been included. But as this was missed while adding this binding, so this should be fixed now. (Olof also recommended the same). AFAIK, For TI's NAND OMAP driver, currently there are not many end-users are using these bindings from mainline DT kernel. So this patch series mainly aims to cleanup NAND driver so that migrate to latest DT based kernel from board-file one is easy. So changes should be acceptable from end-user's point, its only matter of which path to take. Request you to please help me finalize this before I resend next patch series addressing other comments from Brian. Thank You.. with regards, pekon
> > Anyway, at this point I think your patch series should be nearly > complete. I made a few comments on your patches, and I'd imagine you > only should need one more revision (v7) before I can accept it to the > l2-mtd.git tree. > Yes surely I will send next version (v7), but it might take few days. As some more feedbacks on [PATCH 1] are pending for final conclusion and this needs to be properly re-tested. And thanks much to you and Olof for the feedbacks. I agree some of Olof's feedbacks on DT bindings gave me new view to look at things, And further simplified the NAND driver code. with regards, pekon
On Wed, Sep 25, 2013 at 10:29:03PM +0100, Brian Norris wrote: > On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote: > > On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris > > <computersforpeace@gmail.com> wrote: > > > > > Olof has given good advice on your DT binding and has (slowly) been > > > responding to other requests for DT review that make it to his list. I > > > see that he hasn't followed up on your changes (this v6), so pinging him > > > (as you did) is probably the correct approach. But please do recognize > > > that the DT list is very high volume, so it's hard to get good timely > > > responses there. > > > > I am not a DT mainainer, but sometimes when I see a binding that > > appears to be wrong I speak up. In this case, the binding was one of > > those. > > Whoops, my bad. I was deceived by the responses I've seen from you on > other issues (thanks, BTW). In that case, I haven't seen any response > from a proper DT binding maintainer :( Hmm... this is the first email in this thread I've received, and I don't have older postings either. It looks like I must have cocked up subscribing to the devicetree list, but as I was CC'd directly on many patches I hadn't noticed. As far as I can see I wasn't CC'd directly on any version of this series, which would have helped to highlight the series as needing review. Apologies for that. I've attempted to correct the problem. Hopefully I've got this right and mails are not being silently dropped somewhere. Pekon, could you please re-send this version of the patches? Cheers, Mark. > > > So, I have no more objections to it, and I hope you can get a quick > > review from a DT maintainer on the rest of the binding. > > At this point, I'm comfortable going ahead without their ack, since they > obviously don't care/don't have the manpower to review. > > Brian >
Hi Mark, > > Pekon, could you please re-send this version of the patches? > As already there are feedbacks on the patches, so re-sending the Patch series might clutter someone else's mailbox. Will it be possible for you to fetch the patches from MTD archives? else I would send you the patches separately.. I'm attaching the URL from MTD archives for each patch separately, and you can follow that thread to see existing comments. [PATCH v6 0/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048613.html [PATCH v6 1/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048611.html [PATCH v6 2/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048612.html [PATCH v6 3/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048615.html [PATCH v6 3/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048614.html with regards, pekon
> > From: Rob Herring [mailto:robherring2@gmail.com] > > > From: Pekon Gupta [mailto:pekon@ti.com] > > > > > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > index df338cb..958e5d5 100644 > > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > @@ -21,11 +21,8 @@ Optional properties: > > > is wired that way. If not specified, a bus > > > width of 8 is assumed. > > > > > > - - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > > > - > > > - "sw" Software method (default) > > > - "hw" Hardware method > > > - "hw-romcode" gpmc hamming mode method & romcode > layout > > > + - ti,nand-ecc-scheme: A string setting the ECC layout to use. One of: > > > + "ham1" 1-bit Hamming ecc code > > > > As has been pointed out, this breaks compatibility which should be > > avoided unless you know the state of use of this binding. I fail to > > see the advantage of using scheme over opt. You could simply add ham1 > > here and maintain compatibility. Instead of removing sw, hw, etc. you > > can simply deprecate them or decide that the kernel doesn't support > > those options. > > > Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier > comments from Olof. So either way is fine with me. Should I revert > it back to 'ti,nand-ecc-opt' ? > > Also, "sw", "hw_romcode" have been deprecated, they are no longer > supported in driver. So shouldn't they be removed from the documentation > ? > > > However, since you are willing to break compatibility, then you should > > the generic NAND binding and use nand-ecc-mode adding the values you > > need. > > > > Documentation/devicetree/bindings/mtd/nand.txt: > > * MTD generic binding > > > > - nand-ecc-mode : String, operation mode of the NAND ecc mode. > > Supported values are: "none", "soft", "hw", "hw_syndrome", > > "hw_oob_first", > > "soft_bch". > > Yes I can use generic 'nand-ecc-mode', But the binding values like > "soft", "hw", "soft_bch" are too generic to describe the hardware. > - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16 > so "soft_bch" is ambiguous. > - Similarly "soft" and "hw" do not determine the algorithm used > like Hamming or BCH. > > (a) Should I update the generic binding values (if no one else is using) ? like > sw -> ham1_sw > hw -> ham1_hw > soft_bch-> soft_bch4, soft_bch8 > OR > (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ? > keeping the old ones intact. And how should I handle un-supported > values, (Is pr_err during kernel probe enough ?). > > [...] > > > > - - elm_id: Specifies elm device node. This is required to support BCH > > > - error correction using ELM module. > > > + - ti,elm-id: Specifies pHandle of the ELM devicetree node. > > > + ELM is an on-chip hardware engine on TI SoC which is used for > > > + locating ECC errors for BCHx algorithms. SoC devices which have > > > + ELM hardware engines should specify this device node in .dtsi > > > + Using ELM for ECC error correction frees some CPU cycles. > > > > While yes, this is better name and documentation, I don't know that > > breaking compatibility is justified. > > > As this is TI specific binding, so manufacturer's name should have been > included. But as this was missed while adding this binding, so this should > be fixed now. (Olof also recommended the same). > > AFAIK, For TI's NAND OMAP driver, currently there are not many > end-users are using these bindings from mainline DT kernel. > So this patch series mainly aims to cleanup NAND driver so that migrate > to latest DT based kernel from board-file one is easy. > So changes should be acceptable from end-user's point, its only matter > of which path to take. > Request you to please help me finalize this before I resend next patch > series addressing other comments from Brian. > + Mark Rutland <mark.rutland@arm.com> + Pawel Moll <pawel.moll@arm.com> + Ian Campbell <ijc+devicetree@hellion.org.uk> + Stephen Warren <swarren@wwwdotorg.org> CC other DT maintainers, who were missed in this branch of mail-chain. with regards, pekon
On Thu, Sep 26, 2013 at 11:54:39AM +0100, Gupta, Pekon wrote: > > > > From: Rob Herring [mailto:robherring2@gmail.com] > > > > From: Pekon Gupta [mailto:pekon@ti.com] > > > > > > > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > > index df338cb..958e5d5 100644 > > > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > > @@ -21,11 +21,8 @@ Optional properties: > > > > is wired that way. If not specified, a bus > > > > width of 8 is assumed. > > > > > > > > - - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > > > > - > > > > - "sw" Software method (default) > > > > - "hw" Hardware method > > > > - "hw-romcode" gpmc hamming mode method & romcode > > layout > > > > + - ti,nand-ecc-scheme: A string setting the ECC layout to use. One of: > > > > + "ham1" 1-bit Hamming ecc code > > > > > > As has been pointed out, this breaks compatibility which should be > > > avoided unless you know the state of use of this binding. I fail to > > > see the advantage of using scheme over opt. You could simply add ham1 > > > here and maintain compatibility. Instead of removing sw, hw, etc. you > > > can simply deprecate them or decide that the kernel doesn't support > > > those options. > > > > > Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier > > comments from Olof. So either way is fine with me. Should I revert > > it back to 'ti,nand-ecc-opt' ? I think that if the binding is already in use then we shouldn't break it, unless you're _definitely_ the only user. > > > > Also, "sw", "hw_romcode" have been deprecated, they are no longer > > supported in driver. So shouldn't they be removed from the documentation > > ? Deprecated properties should be marked as deprecated, but continue to be documented. That at least prevents the names being repurposed in an incompatible way, and explains to anyone who looks at the document that the proeprty is deprecated rather than simply undocumented. > > > > > However, since you are willing to break compatibility, then you should > > > the generic NAND binding and use nand-ecc-mode adding the values you > > > need. > > > > > > Documentation/devicetree/bindings/mtd/nand.txt: > > > * MTD generic binding > > > > > > - nand-ecc-mode : String, operation mode of the NAND ecc mode. > > > Supported values are: "none", "soft", "hw", "hw_syndrome", > > > "hw_oob_first", > > > "soft_bch". > > > > Yes I can use generic 'nand-ecc-mode', But the binding values like > > "soft", "hw", "soft_bch" are too generic to describe the hardware. > > - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16 > > so "soft_bch" is ambiguous. It does indeed seem like the generic properties are not generic enough, at least from my quick look other them. However, I am not familiar with NAND, so I may have misunderstood. > > - Similarly "soft" and "hw" do not determine the algorithm used > > like Hamming or BCH. > > > > (a) Should I update the generic binding values (if no one else is using) ? like > > sw -> ham1_sw > > hw -> ham1_hw > > soft_bch-> soft_bch4, soft_bch8 What do the current property names actually correspond to logically? If we did this and there are existing users, the old DTs need to continue functioning. > > OR > > (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ? > > keeping the old ones intact. And how should I handle un-supported > > values, (Is pr_err during kernel probe enough ?). I think something like this, but with the names from (a) would be preferrable. > > > > [...] > > > > > > - - elm_id: Specifies elm device node. This is required to support BCH > > > > - error correction using ELM module. > > > > + - ti,elm-id: Specifies pHandle of the ELM devicetree node. > > > > + ELM is an on-chip hardware engine on TI SoC which is used for > > > > + locating ECC errors for BCHx algorithms. SoC devices which have > > > > + ELM hardware engines should specify this device node in .dtsi > > > > + Using ELM for ECC error correction frees some CPU cycles. > > > > > > While yes, this is better name and documentation, I don't know that > > > breaking compatibility is justified. > > > > > As this is TI specific binding, so manufacturer's name should have been > > included. But as this was missed while adding this binding, so this should > > be fixed now. (Olof also recommended the same). We could update the binding to prefer ti,elm-id, and deprecate elm_id while maintaining support in the kernel. Thanks, Mark. > > > > AFAIK, For TI's NAND OMAP driver, currently there are not many > > end-users are using these bindings from mainline DT kernel. > > So this patch series mainly aims to cleanup NAND driver so that migrate > > to latest DT based kernel from board-file one is easy. > > So changes should be acceptable from end-user's point, its only matter > > of which path to take. > > Request you to please help me finalize this before I resend next patch > > series addressing other comments from Brian. > > > > + Mark Rutland <mark.rutland@arm.com> > + Pawel Moll <pawel.moll@arm.com> > + Ian Campbell <ijc+devicetree@hellion.org.uk> > + Stephen Warren <swarren@wwwdotorg.org> > > CC other DT maintainers, who were missed in this branch of mail-chain. > > with regards, pekon >
[I see Mark made some of the same comments while I was typing this email] On Thu, Sep 26, 2013 at 06:08:42AM +0000, Gupta, Pekon wrote: > > From: Rob Herring [mailto:robherring2@gmail.com] > > > From: Pekon Gupta [mailto:pekon@ti.com] > > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > index df338cb..958e5d5 100644 > > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > @@ -21,11 +21,8 @@ Optional properties: > > > is wired that way. If not specified, a bus > > > width of 8 is assumed. > > > > > > - - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > > > - > > > - "sw" Software method (default) > > > - "hw" Hardware method > > > - "hw-romcode" gpmc hamming mode method & romcode layout > > > + - ti,nand-ecc-scheme: A string setting the ECC layout to use. One of: > > > + "ham1" 1-bit Hamming ecc code > > > > As has been pointed out, this breaks compatibility which should be > > avoided unless you know the state of use of this binding. I fail to > > see the advantage of using scheme over opt. You could simply add ham1 > > here and maintain compatibility. Instead of removing sw, hw, etc. you > > can simply deprecate them or decide that the kernel doesn't support > > those options. > > > Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier > comments from Olof. So either way is fine with me. Should I revert > it back to 'ti,nand-ecc-opt' ? > > Also, "sw", "hw_romcode" have been deprecated, they are no longer > supported in driver. So shouldn't they be removed from the documentation ? I think leaving them in the documentation but marking them as "deprecated" makes sense. > > However, since you are willing to break compatibility, then you should > > the generic NAND binding and use nand-ecc-mode adding the values you > > need. > > > > Documentation/devicetree/bindings/mtd/nand.txt: > > * MTD generic binding > > > > - nand-ecc-mode : String, operation mode of the NAND ecc mode. > > Supported values are: "none", "soft", "hw", "hw_syndrome", > > "hw_oob_first", > > "soft_bch". > > Yes I can use generic 'nand-ecc-mode', But the binding values like > "soft", "hw", "soft_bch" are too generic to describe the hardware. > - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16 > so "soft_bch" is ambiguous. > - Similarly "soft" and "hw" do not determine the algorithm used > like Hamming or BCH. > > (a) Should I update the generic binding values (if no one else is using) ? like > sw -> ham1_sw > hw -> ham1_hw > soft_bch-> soft_bch4, soft_bch8 > OR > (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ? > keeping the old ones intact. And how should I handle un-supported > values, (Is pr_err during kernel probe enough ?). The existing nand-ecc-mode binding is a bit peculiar and hard to maintain generically for all NAND, IMO. ECC is often very specific to each driver/controller. What's to say that a given software BCH4 library (such as the one found in Linux) will match a given controller's HW BCH4 layout and encoding format? Perhaps Pekon's OMAP NAND driver can straighten things out such that HW and SW ECC are interchangeable for a given BCH mode, but we can't guarantee all drivers/controllers make this possible. So, what's the advantage of using this generic binding (nand-ecc-mode) for all NAND hardware instead of defining distinct hardware-specific bindings for different sets of hardware? The former seems like we will clutter up the nand-ecc-mode namespace such that any one set of hardware/software will only support a small subset of the potential options. And it doesn't seem to win us a lot. What's more, this single binding doesn't seem flexible enough for the hardware I deal with. I have a NAND controller whose ECC HW can be programmed to one of dozens of different ECC modes, parameterized by strength (i.e., BCH-x, where x varies) and ECC step/sector size (i.e., each ECC block covers 512B or 1024B of data). So I'm not convinced that extending this nand-ecc-mode property is correct at all. But if we do want to, perhaps we'd need to introduce additional orthogonal properties to specify strength and step size, rather than listing all combinations as separate values for nand-ecc-mode. Brian
Hi All, So, based on feedbacks from everyone, I could come to following conclusions. Please confirm, if those are acceptable ? > From: Mark Rutland [mailto:mark.rutland@arm.com] > > > On Thu, Sep 26, 2013 at 11:54:39AM +0100, Gupta, Pekon wrote: > > > > > > From: Rob Herring [mailto:robherring2@gmail.com] > > > > > From: Pekon Gupta [mailto:pekon@ti.com] > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > > b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > > > index df338cb..958e5d5 100644 > > > > > --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > > > +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt > > > > > @@ -21,11 +21,8 @@ Optional properties: > > > > > is wired that way. If not specified, a bus > > > > > width of 8 is assumed. > > > > > > > > > > - - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > > > > > - > > > > > - "sw" Software method (default) > > > > > - "hw" Hardware method > > > > > - "hw-romcode" gpmc hamming mode method & romcode > > > layout > > > > > + - ti,nand-ecc-scheme: A string setting the ECC layout to use. > One of: > > > > > + "ham1" 1-bit Hamming ecc code > > > > > > > > As has been pointed out, this breaks compatibility which should be > > > > avoided unless you know the state of use of this binding. I fail to > > > > see the advantage of using scheme over opt. You could simply add > ham1 > > > > here and maintain compatibility. Instead of removing sw, hw, etc. you > > > > can simply deprecate them or decide that the kernel doesn't support > > > > those options. > > > > > > > Renaming 'ti,nand-ecc-opt' to 'ti,nand-ecc-scheme' was as per earlier > > > comments from Olof. So either way is fine with me. Should I revert > > > it back to 'ti,nand-ecc-opt' ? > > I think that if the binding is already in use then we shouldn't break > it, unless you're _definitely_ the only user. > Agreed, would revert back to 'ti,nand-ecc-scheme' > > > > > > Also, "sw", "hw_romcode" have been deprecated, they are no longer > > > supported in driver. So shouldn't they be removed from the > documentation > > > ? > > Deprecated properties should be marked as deprecated, but continue to be > documented. That at least prevents the names being repurposed in an > incompatible way, and explains to anyone who looks at the document that > the proeprty is deprecated rather than simply undocumented. > Agreed. - keep existing values in documentation (sw, hw, hw_romcode) but mark them as deprecated. - add new values (ham1, bch4, bch8,..) > > > > > > > However, since you are willing to break compatibility, then you should > > > > the generic NAND binding and use nand-ecc-mode adding the values > you > > > > need. > > > > > > > > Documentation/devicetree/bindings/mtd/nand.txt: > > > > * MTD generic binding > > > > > > > > - nand-ecc-mode : String, operation mode of the NAND ecc mode. > > > > Supported values are: "none", "soft", "hw", "hw_syndrome", > > > > "hw_oob_first", > > > > "soft_bch". > > > > > > Yes I can use generic 'nand-ecc-mode', But the binding values like > > > "soft", "hw", "soft_bch" are too generic to describe the hardware. > > > - BCH ECC scheme can itself be of multiple types, bch4/bch8/bch16 > > > so "soft_bch" is ambiguous. > > It does indeed seem like the generic properties are not generic enough, > at least from my quick look other them. However, I am not familiar with > NAND, so I may have misunderstood. > Would not use generic 'nand-ecc-mode' property, instead continue with 'ti,nand-ecc-opt'. > > > - Similarly "soft" and "hw" do not determine the algorithm used > > > like Hamming or BCH. > > > > > > (a) Should I update the generic binding values (if no one else is using) ? > like > > > sw -> ham1_sw > > > hw -> ham1_hw > > > soft_bch-> soft_bch4, soft_bch8 > > What do the current property names actually correspond to logically? If > we did this and there are existing users, the old DTs need to continue > functioning. > > > > OR > > > (b) Should I add newer ones to it (like "ham1", "bch4", "bch8", "bch16") ? > > > keeping the old ones intact. And how should I handle un-supported > > > values, (Is pr_err during kernel probe enough ?). > > I think something like this, but with the names from (a) would be > preferrable. > As Brian pointed, implementation of ecc-scheme can be very different from vendor to vendor, and even SoC to SoC within same vendor, Thus its difficult to generalize as common DT binding for everyone. - Even if we try to do, there would be too many values, out of which only selectable would be applicable for a given SoC. - And some platforms might need extra DT information to get driver working, because h/w was designed that way. So it would be mess. So, its better not to have a generic ecc-scheme binding, instead let every vendor define it specifically. So for TI OMAP NAND driver, I'm continuing to use 'ti,nand-ecc-opt' as described above. Is this acceptable ? > > > > > > [...] > > > > > > > > - - elm_id: Specifies elm device node. This is required to support > BCH > > > > > - error correction using ELM module. > > > > > + - ti,elm-id: Specifies pHandle of the ELM devicetree node. > > > > > + ELM is an on-chip hardware engine on TI SoC which is used > for > > > > > + locating ECC errors for BCHx algorithms. SoC devices which > have > > > > > + ELM hardware engines should specify this device node in > .dtsi > > > > > + Using ELM for ECC error correction frees some CPU cycles. > > > > > > > > While yes, this is better name and documentation, I don't know that > > > > breaking compatibility is justified. > > > > > > > As this is TI specific binding, so manufacturer's name should have been > > > included. But as this was missed while adding this binding, so this should > > > be fixed now. (Olof also recommended the same). > > We could update the binding to prefer ti,elm-id, and deprecate elm_id > while maintaining support in the kernel. > Agreed. - would keep elm_id but mark it deprecated in Documentation - add new ti,elm-id > > > > > > AFAIK, For TI's NAND OMAP driver, currently there are not many > > > end-users are using these bindings from mainline DT kernel. > > > So this patch series mainly aims to cleanup NAND driver so that migrate > > > to latest DT based kernel from board-file one is easy. > > > So changes should be acceptable from end-user's point, its only matter > > > of which path to take. > > > Request you to please help me finalize this before I resend next patch > > > series addressing other comments from Brian. > > > with regards, pekon
diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt index df338cb..958e5d5 100644 --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt @@ -21,11 +21,8 @@ Optional properties: is wired that way. If not specified, a bus width of 8 is assumed. - - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: - - "sw" Software method (default) - "hw" Hardware method - "hw-romcode" gpmc hamming mode method & romcode layout + - ti,nand-ecc-scheme: A string setting the ECC layout to use. One of: + "ham1" 1-bit Hamming ecc code "bch4" 4-bit BCH ecc code "bch8" 8-bit BCH ecc code @@ -36,8 +33,11 @@ Optional properties: "prefetch-dma" Prefetch enabled sDMA mode "prefetch-irq" Prefetch enabled irq mode - - elm_id: Specifies elm device node. This is required to support BCH - error correction using ELM module. + - ti,elm-id: Specifies pHandle of the ELM devicetree node. + ELM is an on-chip hardware engine on TI SoC which is used for + locating ECC errors for BCHx algorithms. SoC devices which have + ELM hardware engines should specify this device node in .dtsi + Using ELM for ECC error correction frees some CPU cycles. For inline partiton table parsing (optional): diff --git a/arch/arm/mach-omap2/board-flash.c b/arch/arm/mach-omap2/board-flash.c index fc20a61..ac82512 100644 --- a/arch/arm/mach-omap2/board-flash.c +++ b/arch/arm/mach-omap2/board-flash.c @@ -142,7 +142,7 @@ __init board_nand_init(struct mtd_partition *nand_parts, u8 nr_parts, u8 cs, board_nand_data.nr_parts = nr_parts; board_nand_data.devsize = nand_type; - board_nand_data.ecc_opt = OMAP_ECC_HAMMING_CODE_DEFAULT; + board_nand_data.ecc_opt = OMAP_ECC_BCH8_CODE_HW; gpmc_nand_init(&board_nand_data, gpmc_t); } #endif /* CONFIG_MTD_NAND_OMAP2 || CONFIG_MTD_NAND_OMAP2_MODULE */ diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index 9f4795a..6409884 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -1340,15 +1340,6 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np, } #ifdef CONFIG_MTD_NAND - -static const char * const nand_ecc_opts[] = { - [OMAP_ECC_HAMMING_CODE_DEFAULT] = "sw", - [OMAP_ECC_HAMMING_CODE_HW] = "hw", - [OMAP_ECC_HAMMING_CODE_HW_ROMCODE] = "hw-romcode", - [OMAP_ECC_BCH4_CODE_HW] = "bch4", - [OMAP_ECC_BCH8_CODE_HW] = "bch8", -}; - static const char * const nand_xfer_types[] = { [NAND_OMAP_PREFETCH_POLLED] = "prefetch-polled", [NAND_OMAP_POLLED] = "polled", @@ -1363,6 +1354,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev, const char *s; struct gpmc_timings gpmc_t; struct omap_nand_platform_data *gpmc_nand_data; + const __be32 *parp; + int lenp; if (of_property_read_u32(child, "reg", &val) < 0) { dev_err(&pdev->dev, "%s has no 'reg' property\n", @@ -1378,12 +1371,36 @@ static int gpmc_probe_nand_child(struct platform_device *pdev, gpmc_nand_data->cs = val; gpmc_nand_data->of_node = child; - if (!of_property_read_string(child, "ti,nand-ecc-opt", &s)) - for (val = 0; val < ARRAY_SIZE(nand_ecc_opts); val++) - if (!strcasecmp(s, nand_ecc_opts[val])) { - gpmc_nand_data->ecc_opt = val; - break; - } + /* Detect availability of ELM module */ + parp = of_get_property(child, "ti,elm-id", &lenp); + if ((parp == NULL) && (lenp != (sizeof(void *) * 2))) { + pr_warn("%s: ti,elm-id property not found\n", __func__); + gpmc_nand_data->elm_of_node = NULL; + } else { + gpmc_nand_data->elm_of_node = + of_find_node_by_phandle(be32_to_cpup(parp)); + } + /* select NAND ecc-scheme */ + if (of_property_read_string(child, "ti,nand-ecc-scheme", &s)) { + pr_err("%s: valid ti,nand-ecc-scheme not found\n", __func__); + return -ENODEV; + } + if (!strcmp(s, "ham1")) + gpmc_nand_data->ecc_opt = OMAP_ECC_HAMMING_CODE_HW; + else if (!strcmp(s, "bch4")) + if (gpmc_nand_data->elm_of_node) + gpmc_nand_data->ecc_opt = OMAP_ECC_BCH4_CODE_HW; + else + gpmc_nand_data->ecc_opt = + OMAP_ECC_BCH4_CODE_HW_DETECTION_SW; + else if (!strcmp(s, "bch8")) + if (gpmc_nand_data->elm_of_node) + gpmc_nand_data->ecc_opt = OMAP_ECC_BCH8_CODE_HW; + else + gpmc_nand_data->ecc_opt = + OMAP_ECC_BCH8_CODE_HW_DETECTION_SW; + else + pr_err("%s: ti,ecc-scheme: invalid property value\n", __func__); if (!of_property_read_string(child, "ti,nand-xfer-type", &s)) for (val = 0; val < ARRAY_SIZE(nand_xfer_types); val++) diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h index 6bf9ef4..b4c2c5a 100644 --- a/include/linux/platform_data/mtd-nand-omap2.h +++ b/include/linux/platform_data/mtd-nand-omap2.h @@ -23,13 +23,21 @@ enum nand_io { }; enum omap_ecc { - /* 1-bit ecc: stored at end of spare area */ - OMAP_ECC_HAMMING_CODE_DEFAULT = 0, /* Default, s/w method */ - OMAP_ECC_HAMMING_CODE_HW, /* gpmc to detect the error */ - /* 1-bit ecc: stored at beginning of spare area as romcode */ - OMAP_ECC_HAMMING_CODE_HW_ROMCODE, /* gpmc method & romcode layout */ - OMAP_ECC_BCH4_CODE_HW, /* 4-bit BCH ecc code */ - OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */ + /* 1-bit ECC calculation by Software, Error detection by Software */ + OMAP_ECC_HAMMING_CODE_DEFAULT = 0, + /* 1-bit ECC calculation by GPMC, Error detection by Software */ + OMAP_ECC_HAMMING_CODE_HW, + /* 1-bit ECC calculation by GPMC, Error detection by Software */ + /* ECC layout compatible to legacy ROMCODE. */ + OMAP_ECC_HAMMING_CODE_HW_ROMCODE, + /* 4-bit ECC calculation by GPMC, Error detection by Software */ + OMAP_ECC_BCH4_CODE_HW_DETECTION_SW, + /* 4-bit ECC calculation by GPMC, Error detection by ELM */ + OMAP_ECC_BCH4_CODE_HW, + /* 8-bit ECC calculation by GPMC, Error detection by Software */ + OMAP_ECC_BCH8_CODE_HW_DETECTION_SW, + /* 8-bit ECC calculation by GPMC, Error detection by ELM */ + OMAP_ECC_BCH8_CODE_HW, }; struct gpmc_nand_regs { @@ -63,5 +71,6 @@ struct omap_nand_platform_data { /* for passing the partitions */ struct device_node *of_node; + struct device_node *elm_of_node; }; #endif
OMAP NAND driver support multiple ECC scheme, which can used in following different flavours, depending on in-build Hardware engines supported by SoC. +---------------------------------------+---------------+---------------+ | ECC scheme |ECC calculation|Error detection| +---------------------------------------+---------------+---------------+ |OMAP_ECC_HAMMING_CODE_HW |H/W (GPMC) |S/W | +---------------------------------------+---------------+---------------+ |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |H/W (GPMC) |S/W | |(requires CONFIG_MTD_NAND_ECC_BCH) | | | +---------------------------------------+---------------+---------------+ |OMAP_ECC_BCH8_CODE_HW |H/W (GPMC) |H/W (ELM) | |(requires CONFIG_MTD_NAND_OMAP_BCH && | | | | ti,elm-id in DT) | | | +---------------------------------------+---------------+---------------+ To optimize footprint of omap2-nand driver, selection of some ECC schemes also require enabling following Kconfigs, in addition to setting appropriate DT bindings - Kconfig:CONFIG_MTD_NAND_ECC_BCH enables S/W based BCH ECC algorithm - Kconfig:CONFIG_MTD_NAND_OMAP_BCH enables H/W based BCH ECC algorithm This patch - updates DT binding for selection of ecc-scheme - updates DT binding for detection of ELM h/w engine - removes following obselete ECC schemes OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming ECC) OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit Hamming ECC scheme) - updates DT binding documentation for mtd/gpmc-nand Signed-off-by: Pekon Gupta <pekon@ti.com> --- .../devicetree/bindings/mtd/gpmc-nand.txt | 14 +++---- arch/arm/mach-omap2/board-flash.c | 2 +- arch/arm/mach-omap2/gpmc.c | 47 +++++++++++++++------- include/linux/platform_data/mtd-nand-omap2.h | 23 +++++++---- 4 files changed, 56 insertions(+), 30 deletions(-)