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

[Gupta, Pekon]

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 at arm.com]
> > 
> On Thu, Sep 26, 2013 at 11:54:39AM +0100, Gupta, Pekon wrote:
> >
> > > > From: Rob Herring [mailto:robherring2 at gmail.com]
> > > > > From: Pekon Gupta [mailto:pekon at 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