Patchwork [v6,1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

login
register
mail settings
Submitter pekon gupta
Date Sept. 12, 2013, 11:50 a.m.
Message ID <1378986619-26765-2-git-send-email-pekon@ti.com>
Download mbox | patch
Permalink /patch/274533/
State New
Headers show

Comments

pekon gupta - Sept. 12, 2013, 11:50 a.m.
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(-)
pekon gupta - Sept. 24, 2013, 6:04 p.m.
> 
> 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
Felipe Balbi - Sept. 25, 2013, 1:46 p.m.
+ 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.
Brian Norris - Sept. 25, 2013, 5:46 p.m.
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
Brian Norris - Sept. 25, 2013, 5:55 p.m.
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
Brian Norris - Sept. 25, 2013, 6:29 p.m.
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
pekon gupta - Sept. 25, 2013, 7:24 p.m.
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
Brian Norris - Sept. 25, 2013, 8:05 p.m.
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
Olof Johansson - Sept. 25, 2013, 8:33 p.m.
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
Brian Norris - Sept. 25, 2013, 9:29 p.m.
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
Olof Johansson - Sept. 25, 2013, 9:32 p.m.
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
Brian Norris - Sept. 25, 2013, 10:22 p.m.
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
Rob Herring - Sept. 25, 2013, 10:23 p.m.
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
pekon gupta - Sept. 26, 2013, 6:08 a.m.
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
pekon gupta - Sept. 26, 2013, 6:15 a.m.
> 
> 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
Mark Rutland - Sept. 26, 2013, 9:18 a.m.
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
>
pekon gupta - Sept. 26, 2013, 9:32 a.m.
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
pekon gupta - Sept. 26, 2013, 10:54 a.m.
> > 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
Mark Rutland - Sept. 26, 2013, 5:09 p.m.
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
>
Brian Norris - Sept. 26, 2013, 5:27 p.m.
[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
pekon gupta - Sept. 27, 2013, 6:17 a.m.
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

Patch

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