| Message ID | 1558117914-35807-2-git-send-email-kdasu.kdev@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | [1/2] dt-bindings: mtd: brcmnand: Make nand-ecc-strength andnand-ecc-step-size optional | expand |
Hi Kamal, Kamal Dasu <kdasu.kdev@gmail.com> wrote on Fri, 17 May 2019 14:29:55 -0400: > This change supports nand-ecc-step-size and nand-ecc-strenght fields in strength > brcmnand dt node to be optional. DT ^ extra space > see: Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt > > If both nand-ecc-strength and nand-ecc-step-size are not specified in > device tree node for NAND, nand_base driver does detect onfi ext ecc s/nand_base driver/the raw NAND layer/ s/onfi/ONFI/ s/ecc/ECC/ What is "ext"? Please use plain English here. > info from ONFI extended parameter page for parts using ONFI >= 2.1. In s/info/information/ > case of non-onfi NAND there could be a nand_id table entry with the ecc s/ecc/ECC/ > info. If there is a valid device tree entry for nand-ecc-strength and > nand-ecc-step-size fields it still shall override the detected values. > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > --- > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > index ce0b8ff..e967b30 100644 > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > @@ -2144,6 +2144,16 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) > return -EINVAL; > } > > + if (!(chip->ecc.size > 0 && chip->ecc.strength > 0) && Is the case where only size OR strength is valid handled? > + (chip->base.eccreq.strength > 0 && > + chip->base.eccreq.step_size > 0)) { > + /* use detected ecc parameters */ Use ECC > + chip->ecc.size = chip->base.eccreq.step_size; > + chip->ecc.strength = chip->base.eccreq.strength; > + pr_info("Using detected nand-ecc-step-size %d, nand-ecc-strength %d\n", > + chip->ecc.size, chip->ecc.strength); > + } > + > switch (chip->ecc.size) { > case 512: > if (chip->ecc.algo == NAND_ECC_HAMMING) Thanks, Miquèl
Will make the changes and send a V2 patch. On Mon, May 20, 2019 at 8:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Kamal, > > Kamal Dasu <kdasu.kdev@gmail.com> wrote on Fri, 17 May 2019 14:29:55 > -0400: > > > This change supports nand-ecc-step-size and nand-ecc-strenght fields in > > strength > > > brcmnand dt node to be optional. > > DT ^ extra space > > > see: Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt > > > > If both nand-ecc-strength and nand-ecc-step-size are not specified in > > device tree node for NAND, nand_base driver does detect onfi ext ecc > > s/nand_base driver/the raw NAND layer/ > s/onfi/ONFI/ > s/ecc/ECC/ > > What is "ext"? Please use plain English here. > > > info from ONFI extended parameter page for parts using ONFI >= 2.1. In > > s/info/information/ > > > case of non-onfi NAND there could be a nand_id table entry with the ecc > > s/ecc/ECC/ > > > info. If there is a valid device tree entry for nand-ecc-strength and > > nand-ecc-step-size fields it still shall override the detected values. > > > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > > --- > > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > index ce0b8ff..e967b30 100644 > > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > @@ -2144,6 +2144,16 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) > > return -EINVAL; > > } > > > > + if (!(chip->ecc.size > 0 && chip->ecc.strength > 0) && > > Is the case where only size OR strength is valid handled? Both strength and need to be valid, else the driver will behave like before and will fail the probe. > > > + (chip->base.eccreq.strength > 0 && > > + chip->base.eccreq.step_size > 0)) { > > + /* use detected ecc parameters */ > > Use ECC > > > + chip->ecc.size = chip->base.eccreq.step_size; > > + chip->ecc.strength = chip->base.eccreq.strength; > > + pr_info("Using detected nand-ecc-step-size %d, nand-ecc-strength %d\n", > > + chip->ecc.size, chip->ecc.strength); > > + } > > + > > switch (chip->ecc.size) { > > case 512: > > if (chip->ecc.algo == NAND_ECC_HAMMING) > > > Thanks, > Miquèl Kamal
Hi Kamal, Kamal Dasu <kdasu.kdev@gmail.com> wrote on Mon, 20 May 2019 13:31:52 -0400: > Will make the changes and send a V2 patch. > > On Mon, May 20, 2019 at 8:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Kamal, > > > > Kamal Dasu <kdasu.kdev@gmail.com> wrote on Fri, 17 May 2019 14:29:55 > > -0400: > > > > > This change supports nand-ecc-step-size and nand-ecc-strenght fields in > > > > strength > > > > > brcmnand dt node to be optional. > > > > DT ^ extra space > > > > > see: Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt > > > > > > If both nand-ecc-strength and nand-ecc-step-size are not specified in > > > device tree node for NAND, nand_base driver does detect onfi ext ecc > > > > s/nand_base driver/the raw NAND layer/ > > s/onfi/ONFI/ > > s/ecc/ECC/ > > > > What is "ext"? Please use plain English here. > > > > > info from ONFI extended parameter page for parts using ONFI >= 2.1. In > > > > s/info/information/ > > > > > case of non-onfi NAND there could be a nand_id table entry with the ecc > > > > s/ecc/ECC/ > > > > > info. If there is a valid device tree entry for nand-ecc-strength and > > > nand-ecc-step-size fields it still shall override the detected values. > > > > > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > > > --- > > > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > > index ce0b8ff..e967b30 100644 > > > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > > @@ -2144,6 +2144,16 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) > > > return -EINVAL; > > > } > > > > > > + if (!(chip->ecc.size > 0 && chip->ecc.strength > 0) && > > > > Is the case where only size OR strength is valid handled? > > Both strength and need to be valid, else the driver will behave like > before and will fail the probe. Yes, but you do not handle the case when either strength OR size is not valid but the other one is. Is it one purpose? > > > > > > + (chip->base.eccreq.strength > 0 && > > > + chip->base.eccreq.step_size > 0)) { > > > + /* use detected ecc parameters */ > > > > Use ECC > > > > > + chip->ecc.size = chip->base.eccreq.step_size; > > > + chip->ecc.strength = chip->base.eccreq.strength; > > > + pr_info("Using detected nand-ecc-step-size %d, nand-ecc-strength %d\n", > > > + chip->ecc.size, chip->ecc.strength); > > > + } > > > + > > > switch (chip->ecc.size) { > > > case 512: > > > if (chip->ecc.algo == NAND_ECC_HAMMING) > > > > > > Thanks, > > Miquèl > > Kamal Thanks, Miquèl
On Mon, May 20, 2019 at 1:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Kamal, > > Kamal Dasu <kdasu.kdev@gmail.com> wrote on Mon, 20 May 2019 13:31:52 > -0400: > > > Will make the changes and send a V2 patch. > > > > On Mon, May 20, 2019 at 8:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Kamal, > > > > > > Kamal Dasu <kdasu.kdev@gmail.com> wrote on Fri, 17 May 2019 14:29:55 > > > -0400: > > > > > > > This change supports nand-ecc-step-size and nand-ecc-strenght fields in > > > > > > strength > > > > > > > brcmnand dt node to be optional. > > > > > > DT ^ extra space > > > > > > > see: Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt > > > > > > > > If both nand-ecc-strength and nand-ecc-step-size are not specified in > > > > device tree node for NAND, nand_base driver does detect onfi ext ecc > > > > > > s/nand_base driver/the raw NAND layer/ > > > s/onfi/ONFI/ > > > s/ecc/ECC/ > > > > > > What is "ext"? Please use plain English here. > > > > > > > info from ONFI extended parameter page for parts using ONFI >= 2.1. In > > > > > > s/info/information/ > > > > > > > case of non-onfi NAND there could be a nand_id table entry with the ecc > > > > > > s/ecc/ECC/ > > > > > > > info. If there is a valid device tree entry for nand-ecc-strength and > > > > nand-ecc-step-size fields it still shall override the detected values. > > > > > > > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > > > > --- > > > > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 10 ++++++++++ > > > > 1 file changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > > > index ce0b8ff..e967b30 100644 > > > > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > > > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > > > > @@ -2144,6 +2144,16 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) > > > > return -EINVAL; > > > > } > > > > > > > > + if (!(chip->ecc.size > 0 && chip->ecc.strength > 0) && > > > > > > Is the case where only size OR strength is valid handled? > > > > Both strength and need to be valid, else the driver will behave like > > before and will fail the probe. > > Yes, but you do not handle the case when either strength OR size is not > valid but the other one is. Is it one purpose? > If I understand you want me to use the following check: if (ecc->mode != NAND_ECC_NONE && (!ecc->size || !ecc->strength)) { if (chip->base.eccreq.step_size && chip->base.eccreq.strength) { /* use the base values */ } > > > > > > > > > + (chip->base.eccreq.strength > 0 && > > > > + chip->base.eccreq.step_size > 0)) { > > > > + /* use detected ecc parameters */ > > > > > > Use ECC > > > > > > > + chip->ecc.size = chip->base.eccreq.step_size; > > > > + chip->ecc.strength = chip->base.eccreq.strength; > > > > + pr_info("Using detected nand-ecc-step-size %d, nand-ecc-strength %d\n", > > > > + chip->ecc.size, chip->ecc.strength); > > > > + } > > > > + > > > > switch (chip->ecc.size) { > > > > case 512: > > > > if (chip->ecc.algo == NAND_ECC_HAMMING) > > > > > > > > > Thanks, > > > Miquèl > > > > Kamal > > > > > Thanks, > Miquèl Kamal
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c index ce0b8ff..e967b30 100644 --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c @@ -2144,6 +2144,16 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) return -EINVAL; } + if (!(chip->ecc.size > 0 && chip->ecc.strength > 0) && + (chip->base.eccreq.strength > 0 && + chip->base.eccreq.step_size > 0)) { + /* use detected ecc parameters */ + chip->ecc.size = chip->base.eccreq.step_size; + chip->ecc.strength = chip->base.eccreq.strength; + pr_info("Using detected nand-ecc-step-size %d, nand-ecc-strength %d\n", + chip->ecc.size, chip->ecc.strength); + } + switch (chip->ecc.size) { case 512: if (chip->ecc.algo == NAND_ECC_HAMMING)
This change supports nand-ecc-step-size and nand-ecc-strenght fields in brcmnand dt node to be optional. see: Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt If both nand-ecc-strength and nand-ecc-step-size are not specified in device tree node for NAND, nand_base driver does detect onfi ext ecc info from ONFI extended parameter page for parts using ONFI >= 2.1. In case of non-onfi NAND there could be a nand_id table entry with the ecc info. If there is a valid device tree entry for nand-ecc-strength and nand-ecc-step-size fields it still shall override the detected values. Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> --- drivers/mtd/nand/raw/brcmnand/brcmnand.c | 10 ++++++++++ 1 file changed, 10 insertions(+)