Message ID | 1390255810-19486-6-git-send-email-m-karicheri2@ti.com |
---|---|
State | Changes Requested |
Delegated to: | Scott Wood |
Headers | show |
Dear Murali Karicheri, In message <1390255810-19486-6-git-send-email-m-karicheri2@ti.com> you wrote: > This patch introduces a configurable mechanism to disable subpage writes > in the DaVinci NAND driver. > > Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > --- > drivers/mtd/nand/davinci_nand.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c > index 5b17d7b..75b03a7 100644 > --- a/drivers/mtd/nand/davinci_nand.c > +++ b/drivers/mtd/nand/davinci_nand.c > @@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand) > #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT > nand->bbt_options |= NAND_BBT_USE_FLASH; > #endif > +#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE > + nand->options |= NAND_NO_SUBPAGE_WRITE; > +#endif > #ifdef CONFIG_SYS_NAND_HW_ECC > nand->ecc.mode = NAND_ECC_HW; > nand->ecc.size = 512; New CONFIG_ options MUSt be documented in the README. Also, you are just adding dead code. There are no users for this option. Best regards, Wolfgang Denk
Dear Murali, In message <3E54258959B69E4282D79E01AB1F32B70466CE81@DFLE11.ent.ti.com> you wrote: > > >New CONFIG_ options MUSt be documented in the README. > > Will do. > > >Also, you are just adding dead code. There are no users for this option. > > This is a preparatory patch for keystone u-boot port and will bundle with > it if it make more sense. But this looks harmless since this option allow > users to disable subpage write. So I want to merge this right away after > adding the option name to README. Any issues? The adding of the documentation to the README and of the code should be in a single patch, not one after the other. And the patch should be art of a series that actually uses this feature. It has no logical place in the current patch series. Best regards, Wolfgang Denk
On Mon, 2014-01-20 at 17:10 -0500, Murali Karicheri wrote: > This patch introduces a configurable mechanism to disable subpage writes > in the DaVinci NAND driver. > > Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > --- > drivers/mtd/nand/davinci_nand.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c > index 5b17d7b..75b03a7 100644 > --- a/drivers/mtd/nand/davinci_nand.c > +++ b/drivers/mtd/nand/davinci_nand.c > @@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand) > #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT > nand->bbt_options |= NAND_BBT_USE_FLASH; > #endif > +#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE > + nand->options |= NAND_NO_SUBPAGE_WRITE; > +#endif > #ifdef CONFIG_SYS_NAND_HW_ECC > nand->ecc.mode = NAND_ECC_HW; > nand->ecc.size = 512; Why does it need to be configurable? -Scott
On Tue, 2014-01-28 at 23:19 +0000, Karicheri, Muralidharan wrote: > >-----Original Message----- > >From: Scott Wood [mailto:scottwood@freescale.com] > >Sent: Wednesday, January 22, 2014 3:48 PM > >To: Karicheri, Muralidharan > >Cc: u-boot@lists.denx.de; Rini, Tom; Andrianov, Vitaly > >Subject: Re: [U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes > > > >On Mon, 2014-01-20 at 17:10 -0500, Murali Karicheri wrote: > >> This patch introduces a configurable mechanism to disable subpage > >> writes in the DaVinci NAND driver. > >> > >> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > >> --- > >> drivers/mtd/nand/davinci_nand.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/mtd/nand/davinci_nand.c > >> b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644 > >> --- a/drivers/mtd/nand/davinci_nand.c > >> +++ b/drivers/mtd/nand/davinci_nand.c > >> @@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand) > >> #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT > >> nand->bbt_options |= NAND_BBT_USE_FLASH; > >> #endif > >> +#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE > >> + nand->options |= NAND_NO_SUBPAGE_WRITE; > >> +#endif > >> #ifdef CONFIG_SYS_NAND_HW_ECC > >> nand->ecc.mode = NAND_ECC_HW; > >> nand->ecc.size = 512; > > > >Why does it need to be configurable? > > > > Same driver used in multiple platforms, and on some platforms like the one > I work with doesn't support subpage writes and has to be disabled. The option needs to be documented, and it should be either named in a davinci-specific manner, or the documentation should clearly list which drivers pay attention to it. It seems odd though, that the ability to support subpage writes is an independent thing rather than a consequence of some other variable such as controller version. -Scott
On Wed, Feb 12, 2014 at 06:00:22PM -0600, Scott Wood wrote: > > On Tue, 2014-01-28 at 23:19 +0000, Karicheri, Muralidharan wrote: > > >-----Original Message----- > > >From: Scott Wood [mailto:scottwood@freescale.com] > > >Sent: Wednesday, January 22, 2014 3:48 PM > > >To: Karicheri, Muralidharan > > >Cc: u-boot@lists.denx.de; Rini, Tom; Andrianov, Vitaly > > >Subject: Re: [U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes > > > > > >On Mon, 2014-01-20 at 17:10 -0500, Murali Karicheri wrote: > > >> This patch introduces a configurable mechanism to disable subpage > > >> writes in the DaVinci NAND driver. > > >> > > >> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> > > >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > > >> --- > > >> drivers/mtd/nand/davinci_nand.c | 3 +++ > > >> 1 file changed, 3 insertions(+) > > >> > > >> diff --git a/drivers/mtd/nand/davinci_nand.c > > >> b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644 > > >> --- a/drivers/mtd/nand/davinci_nand.c > > >> +++ b/drivers/mtd/nand/davinci_nand.c > > >> @@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand) > > >> #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT > > >> nand->bbt_options |= NAND_BBT_USE_FLASH; > > >> #endif > > >> +#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE > > >> + nand->options |= NAND_NO_SUBPAGE_WRITE; > > >> +#endif > > >> #ifdef CONFIG_SYS_NAND_HW_ECC > > >> nand->ecc.mode = NAND_ECC_HW; > > >> nand->ecc.size = 512; > > > > > >Why does it need to be configurable? > > > > > > > Same driver used in multiple platforms, and on some platforms like the one > > I work with doesn't support subpage writes and has to be disabled. > > The option needs to be documented, and it should be either named in a > davinci-specific manner, or the documentation should clearly list which > drivers pay attention to it. Agreed. > It seems odd though, that the ability to support subpage writes is an > independent thing rather than a consequence of some other variable such > as controller version. I suspect this is similar to the OMAP driver where we could support subpages but it gets tricky coordinating with the HW ECC engine for BCH8 and higher. Perhaps there are davinci targets with smaller ECC levels where it works out OK but keystone is doing BCH8 or 16 and it's not worth the savings.
On 2/18/2014 9:57 AM, Rini, Tom wrote: > On Wed, Feb 12, 2014 at 06:00:22PM -0600, Scott Wood wrote: >> On Tue, 2014-01-28 at 23:19 +0000, Karicheri, Muralidharan wrote: >>>> -----Original Message----- >>>> From: Scott Wood [mailto:scottwood@freescale.com] >>>> Sent: Wednesday, January 22, 2014 3:48 PM >>>> To: Karicheri, Muralidharan >>>> Cc: u-boot@lists.denx.de; Rini, Tom; Andrianov, Vitaly >>>> Subject: Re: [U-Boot] [PATCH 5/5] NAND: DaVinci: allow forced disable of subpage writes >>>> >>>> On Mon, 2014-01-20 at 17:10 -0500, Murali Karicheri wrote: >>>>> This patch introduces a configurable mechanism to disable subpage >>>>> writes in the DaVinci NAND driver. >>>>> >>>>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com> >>>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>>>> --- >>>>> drivers/mtd/nand/davinci_nand.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/drivers/mtd/nand/davinci_nand.c >>>>> b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644 >>>>> --- a/drivers/mtd/nand/davinci_nand.c >>>>> +++ b/drivers/mtd/nand/davinci_nand.c >>>>> @@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand) >>>>> #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT >>>>> nand->bbt_options |= NAND_BBT_USE_FLASH; >>>>> #endif >>>>> +#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE >>>>> + nand->options |= NAND_NO_SUBPAGE_WRITE; >>>>> +#endif >>>>> #ifdef CONFIG_SYS_NAND_HW_ECC >>>>> nand->ecc.mode = NAND_ECC_HW; >>>>> nand->ecc.size = 512; >>>> Why does it need to be configurable? >>>> >>> Same driver used in multiple platforms, and on some platforms like the one >>> I work with doesn't support subpage writes and has to be disabled. >> The option needs to be documented, and it should be either named in a >> davinci-specific manner, or the documentation should clearly list which >> drivers pay attention to it. > Agreed. > >> It seems odd though, that the ability to support subpage writes is an >> independent thing rather than a consequence of some other variable such >> as controller version. > I suspect this is similar to the OMAP driver where we could support > subpages but it gets tricky coordinating with the HW ECC engine for BCH8 > and higher. Perhaps there are davinci targets with smaller ECC levels > where it works out OK but keystone is doing BCH8 or 16 and it's not > worth the savings. Tom/Scott, My next version of the series will have readme updated with this option (no change in name as this can be used by other drivers as well) and mention that davinci_nand driver is the driver that currently uses this option. Murali
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c index 5b17d7b..75b03a7 100644 --- a/drivers/mtd/nand/davinci_nand.c +++ b/drivers/mtd/nand/davinci_nand.c @@ -609,6 +609,9 @@ void davinci_nand_init(struct nand_chip *nand) #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT nand->bbt_options |= NAND_BBT_USE_FLASH; #endif +#ifdef CONFIG_SYS_NAND_NO_SUBPAGE_WRITE + nand->options |= NAND_NO_SUBPAGE_WRITE; +#endif #ifdef CONFIG_SYS_NAND_HW_ECC nand->ecc.mode = NAND_ECC_HW; nand->ecc.size = 512;