* [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode @ 2015-11-16 22:05 Simon Arlott 2015-11-17 0:40 ` Brian Norris 2015-12-01 1:53 ` Brian Norris 0 siblings, 2 replies; 7+ messages in thread From: Simon Arlott @ 2015-11-16 22:05 UTC (permalink / raw) To: Brian Norris, David Woodhouse Cc: linux-mtd, Linux Kernel Mailing List, Kevin Cernekee If an error occurs in flash above 4GB in PIO mode then the EXT_ADDR registers will be set to the location of the error and never cleared. Reset them to 0 before reading. Signed-off-by: Simon Arlott <simon@fire.lp0.eu> --- drivers/mtd/nand/brcmnand/brcmnand.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c index 12c6190..2c8f67f 100644 --- a/drivers/mtd/nand/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/brcmnand/brcmnand.c @@ -1400,6 +1400,8 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, /* Clear error addresses */ brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0); brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0); + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0); + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0); brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS, (host->cs << 16) | ((addr >> 32) & 0xffff)); -- 2.1.4 -- Simon Arlott ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode 2015-11-16 22:05 [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode Simon Arlott @ 2015-11-17 0:40 ` Brian Norris 2015-11-17 7:41 ` Simon Arlott 2015-12-01 1:53 ` Brian Norris 1 sibling, 1 reply; 7+ messages in thread From: Brian Norris @ 2015-11-17 0:40 UTC (permalink / raw) To: Simon Arlott Cc: David Woodhouse, linux-mtd, Linux Kernel Mailing List, Kevin Cernekee, bcm-kernel-feedback-list + bcm-kernel-feedback-list On Mon, Nov 16, 2015 at 10:05:39PM +0000, Simon Arlott wrote: > If an error occurs in flash above 4GB in PIO mode then the EXT_ADDR > registers will be set to the location of the error and never cleared. > > Reset them to 0 before reading. > > Signed-off-by: Simon Arlott <simon@fire.lp0.eu> Patch looks OK. Did you see this problem in practice, or is this just theoretical? I thought the documentation seemed to suggest these registers were cleared together with their non-_EXT counterparts. But implementation definitely trumps documentation for HW. Brian > --- > drivers/mtd/nand/brcmnand/brcmnand.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > index 12c6190..2c8f67f 100644 > --- a/drivers/mtd/nand/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > @@ -1400,6 +1400,8 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, > /* Clear error addresses */ > brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0); > brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0); > + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0); > + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0); > > brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS, > (host->cs << 16) | ((addr >> 32) & 0xffff)); > -- > 2.1.4 > > -- > Simon Arlott ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode 2015-11-17 0:40 ` Brian Norris @ 2015-11-17 7:41 ` Simon Arlott 2015-11-17 17:55 ` Brian Norris 0 siblings, 1 reply; 7+ messages in thread From: Simon Arlott @ 2015-11-17 7:41 UTC (permalink / raw) To: Brian Norris Cc: David Woodhouse, linux-mtd, Linux Kernel Mailing List, Kevin Cernekee, bcm-kernel-feedback-list On 17/11/15 00:40, Brian Norris wrote: > + bcm-kernel-feedback-list > > On Mon, Nov 16, 2015 at 10:05:39PM +0000, Simon Arlott wrote: >> If an error occurs in flash above 4GB in PIO mode then the EXT_ADDR >> registers will be set to the location of the error and never cleared. >> >> Reset them to 0 before reading. >> >> Signed-off-by: Simon Arlott <simon@fire.lp0.eu> > > Patch looks OK. Did you see this problem in practice, or is this just > theoretical? I thought the documentation seemed to suggest these > registers were cleared together with their non-_EXT counterparts. But > implementation definitely trumps documentation for HW. It's theoretical (I don't have 4GB+ flash), but the Broadcom version of the NAND driver does this. > Brian > >> --- >> drivers/mtd/nand/brcmnand/brcmnand.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c >> index 12c6190..2c8f67f 100644 >> --- a/drivers/mtd/nand/brcmnand/brcmnand.c >> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c >> @@ -1400,6 +1400,8 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, >> /* Clear error addresses */ >> brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0); >> brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0); >> + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0); >> + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0); >> >> brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS, >> (host->cs << 16) | ((addr >> 32) & 0xffff)); >> -- >> 2.1.4 >> >> -- >> Simon Arlott > -- Simon Arlott ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode 2015-11-17 7:41 ` Simon Arlott @ 2015-11-17 17:55 ` Brian Norris 2015-11-20 18:42 ` Simon Arlott 0 siblings, 1 reply; 7+ messages in thread From: Brian Norris @ 2015-11-17 17:55 UTC (permalink / raw) To: Simon Arlott Cc: David Woodhouse, linux-mtd, Linux Kernel Mailing List, Kevin Cernekee, bcm-kernel-feedback-list On Tue, Nov 17, 2015 at 07:41:21AM +0000, Simon Arlott wrote: > On 17/11/15 00:40, Brian Norris wrote: > > + bcm-kernel-feedback-list > > > > On Mon, Nov 16, 2015 at 10:05:39PM +0000, Simon Arlott wrote: > >> If an error occurs in flash above 4GB in PIO mode then the EXT_ADDR > >> registers will be set to the location of the error and never cleared. > >> > >> Reset them to 0 before reading. > >> > >> Signed-off-by: Simon Arlott <simon@fire.lp0.eu> > > > > Patch looks OK. Did you see this problem in practice, or is this just > > theoretical? I thought the documentation seemed to suggest these > > registers were cleared together with their non-_EXT counterparts. But > > implementation definitely trumps documentation for HW. > > It's theoretical (I don't have 4GB+ flash), but the Broadcom version of > the NAND driver does this. That's a funny thing to say :) There never really was a single "Broadcom version" until we settled on upstreaming this one. It's a direct descendant of this [1], which also does not do these writes, and was tested on >4GB flash, though not extensively. Which product line did you get your driver from, then? Anyway, unless someone still at Broadcom can confirm or deny, I suppose this patch is mostly harmless and I can take it. Brian > > Brian > > > >> --- > >> drivers/mtd/nand/brcmnand/brcmnand.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > >> index 12c6190..2c8f67f 100644 > >> --- a/drivers/mtd/nand/brcmnand/brcmnand.c > >> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > >> @@ -1400,6 +1400,8 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, > >> /* Clear error addresses */ > >> brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0); > >> brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0); > >> + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0); > >> + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0); > >> > >> brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS, > >> (host->cs << 16) | ((addr >> 32) & 0xffff)); > >> -- > >> 2.1.4 > >> > >> -- > >> Simon Arlott > > > > > -- > Simon Arlott [1] https://github.com/Broadcom/stblinux-3.8/blob/master/linux/drivers/mtd/nand/brcmstb_nand.c Disclaimer: I used to work at Broadcom. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode 2015-11-17 17:55 ` Brian Norris @ 2015-11-20 18:42 ` Simon Arlott 2015-12-01 1:47 ` Brian Norris 0 siblings, 1 reply; 7+ messages in thread From: Simon Arlott @ 2015-11-20 18:42 UTC (permalink / raw) To: Brian Norris Cc: David Woodhouse, linux-mtd, Linux Kernel Mailing List, Kevin Cernekee, bcm-kernel-feedback-list On 17/11/15 17:55, Brian Norris wrote: > On Tue, Nov 17, 2015 at 07:41:21AM +0000, Simon Arlott wrote: >> On 17/11/15 00:40, Brian Norris wrote: >> > + bcm-kernel-feedback-list >> > >> > On Mon, Nov 16, 2015 at 10:05:39PM +0000, Simon Arlott wrote: >> >> If an error occurs in flash above 4GB in PIO mode then the EXT_ADDR >> >> registers will be set to the location of the error and never cleared. >> >> >> >> Reset them to 0 before reading. >> >> >> >> Signed-off-by: Simon Arlott <simon@fire.lp0.eu> >> > >> > Patch looks OK. Did you see this problem in practice, or is this just >> > theoretical? I thought the documentation seemed to suggest these >> > registers were cleared together with their non-_EXT counterparts. But >> > implementation definitely trumps documentation for HW. >> >> It's theoretical (I don't have 4GB+ flash), but the Broadcom version of >> the NAND driver does this. > > That's a funny thing to say :) There never really was a single "Broadcom > version" until we settled on upstreaming this one. It's a direct > descendant of this [1], which also does not do these writes, and was > tested on >4GB flash, though not extensively. > > Which product line did you get your driver from, then? I have a file called bcm963xx_4.12L.06B_consumer/kernel/linux/drivers/mtd/brcmnand/brcmnand_base.c: /* Clear ECC registers */ chip->ctrl_write(BCHP_NAND_ECC_CORR_ADDR, 0); chip->ctrl_write(BCHP_NAND_ECC_UNC_ADDR, 0); #if CONFIG_MTD_BRCMNAND_VERSION >= CONFIG_MTD_BRCMNAND_VERS_1_0 chip->ctrl_write(BCHP_NAND_ECC_CORR_EXT_ADDR, 0); chip->ctrl_write(BCHP_NAND_ECC_UNC_EXT_ADDR, 0); #endif There's also a workaround (brcmnand_handle_false_read_ecc_unc_errors) that I'm going to write a patch for as it affects my v4.0 device. It'd be useful to know which version of the controller fixes the issue: /* Flash chip returns errors || There is a bug in the controller, where if one reads from an erased block that has NOT been written to, || this error is raised. || (Writing to OOB area does not have any effect on this bug) || The workaround is to also look into the OOB area, to see if they are all 0xFF */ > Anyway, unless someone still at Broadcom can confirm or deny, I suppose > this patch is mostly harmless and I can take it. > > Brian > >> > Brian >> > >> >> --- >> >> drivers/mtd/nand/brcmnand/brcmnand.c | 2 ++ >> >> 1 file changed, 2 insertions(+) >> >> >> >> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c >> >> index 12c6190..2c8f67f 100644 >> >> --- a/drivers/mtd/nand/brcmnand/brcmnand.c >> >> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c >> >> @@ -1400,6 +1400,8 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, >> >> /* Clear error addresses */ >> >> brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0); >> >> brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0); >> >> + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0); >> >> + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0); >> >> >> >> brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS, >> >> (host->cs << 16) | ((addr >> 32) & 0xffff)); >> >> -- >> >> 2.1.4 >> >> >> >> -- >> >> Simon Arlott >> > >> >> >> -- >> Simon Arlott > > [1] https://github.com/Broadcom/stblinux-3.8/blob/master/linux/drivers/mtd/nand/brcmstb_nand.c > Disclaimer: I used to work at Broadcom. > -- Simon Arlott ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode 2015-11-20 18:42 ` Simon Arlott @ 2015-12-01 1:47 ` Brian Norris 0 siblings, 0 replies; 7+ messages in thread From: Brian Norris @ 2015-12-01 1:47 UTC (permalink / raw) To: Simon Arlott Cc: David Woodhouse, linux-mtd, Linux Kernel Mailing List, Kevin Cernekee, bcm-kernel-feedback-list, Kamal Dasu Hi, On Fri, Nov 20, 2015 at 06:42:08PM +0000, Simon Arlott wrote: > On 17/11/15 17:55, Brian Norris wrote: > > On Tue, Nov 17, 2015 at 07:41:21AM +0000, Simon Arlott wrote: > >> On 17/11/15 00:40, Brian Norris wrote: > >> > + bcm-kernel-feedback-list > >> > > >> > On Mon, Nov 16, 2015 at 10:05:39PM +0000, Simon Arlott wrote: > >> >> If an error occurs in flash above 4GB in PIO mode then the EXT_ADDR > >> >> registers will be set to the location of the error and never cleared. > >> >> > >> >> Reset them to 0 before reading. > >> >> > >> >> Signed-off-by: Simon Arlott <simon@fire.lp0.eu> > >> > > >> > Patch looks OK. Did you see this problem in practice, or is this just > >> > theoretical? I thought the documentation seemed to suggest these > >> > registers were cleared together with their non-_EXT counterparts. But > >> > implementation definitely trumps documentation for HW. > >> > >> It's theoretical (I don't have 4GB+ flash), but the Broadcom version of > >> the NAND driver does this. > > > > That's a funny thing to say :) There never really was a single "Broadcom > > version" until we settled on upstreaming this one. It's a direct > > descendant of this [1], which also does not do these writes, and was > > tested on >4GB flash, though not extensively. > > > > Which product line did you get your driver from, then? > > I have a file called bcm963xx_4.12L.06B_consumer/kernel/linux/drivers/mtd/brcmnand/brcmnand_base.c: > > /* Clear ECC registers */ > chip->ctrl_write(BCHP_NAND_ECC_CORR_ADDR, 0); > chip->ctrl_write(BCHP_NAND_ECC_UNC_ADDR, 0); > #if CONFIG_MTD_BRCMNAND_VERSION >= CONFIG_MTD_BRCMNAND_VERS_1_0 > chip->ctrl_write(BCHP_NAND_ECC_CORR_EXT_ADDR, 0); > chip->ctrl_write(BCHP_NAND_ECC_UNC_EXT_ADDR, 0); > #endif I'd bet it was done without ever actually testing or observing the behavior. The version 1.0 controller is extremely old and most definitely never was used with >4GB NAND. But since this should be harmless and has some small chance of fixing a bug, I'll take it anyway. > There's also a workaround (brcmnand_handle_false_read_ecc_unc_errors) > that I'm going to write a patch for as it affects my v4.0 device. It'd > be useful to know which version of the controller fixes the issue: > > /* Flash chip returns errors > || There is a bug in the controller, where if one reads from an erased block that has NOT been written to, > || this error is raised. > || (Writing to OOB area does not have any effect on this bug) > || The workaround is to also look into the OOB area, to see if they are all 0xFF > */ That one's pretty ugly... but that's a different story. Brian ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode 2015-11-16 22:05 [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode Simon Arlott 2015-11-17 0:40 ` Brian Norris @ 2015-12-01 1:53 ` Brian Norris 1 sibling, 0 replies; 7+ messages in thread From: Brian Norris @ 2015-12-01 1:53 UTC (permalink / raw) To: Simon Arlott Cc: David Woodhouse, linux-mtd, Linux Kernel Mailing List, Kevin Cernekee, bcm-kernel-feedback-list, Kamal Dasu On Mon, Nov 16, 2015 at 10:05:39PM +0000, Simon Arlott wrote: > If an error occurs in flash above 4GB in PIO mode then the EXT_ADDR > registers will be set to the location of the error and never cleared. > > Reset them to 0 before reading. > > Signed-off-by: Simon Arlott <simon@fire.lp0.eu> > --- > drivers/mtd/nand/brcmnand/brcmnand.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > index 12c6190..2c8f67f 100644 > --- a/drivers/mtd/nand/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > @@ -1400,6 +1400,8 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip, > /* Clear error addresses */ > brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_ADDR, 0); > brcmnand_write_reg(ctrl, BRCMNAND_CORR_ADDR, 0); > + brcmnand_write_reg(ctrl, BRCMNAND_UNCORR_EXT_ADDR, 0); > + brcmnand_write_reg(ctrl, BRCMNAND_CORR_EXT_ADDR, 0); > > brcmnand_write_reg(ctrl, BRCMNAND_CMD_EXT_ADDRESS, > (host->cs << 16) | ((addr >> 32) & 0xffff)); Applied to l2-mtd.git ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-01 1:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-16 22:05 [PATCH] brcmnand: Clear EXT_ADDR error registers in PIO mode Simon Arlott 2015-11-17 0:40 ` Brian Norris 2015-11-17 7:41 ` Simon Arlott 2015-11-17 17:55 ` Brian Norris 2015-11-20 18:42 ` Simon Arlott 2015-12-01 1:47 ` Brian Norris 2015-12-01 1:53 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.