* [PATCH] mtd: brcmnand: set initial ECC params based on info from HW @ 2016-01-27 7:58 Rafał Miłecki 2016-02-01 17:53 ` Brian Norris 0 siblings, 1 reply; 6+ messages in thread From: Rafał Miłecki @ 2016-01-27 7:58 UTC (permalink / raw) To: Brian Norris, Kamal Dasu, linux-mtd Cc: bcm-kernel-feedback-list, Hauke Mehrtens, Rafał Miłecki So far we were depending on nand_dt_init getting ECC info from DT and setting it in ECC struct. It is possible to simply read this info from hardware registers which makes adding support for new hardware way easier (no more guessing & trying all combinations). Please note it still gives a precedence to DT which was overwrite whatever was initially set by the driver. Signed-off-by: Rafał Miłecki <zajec5@gmail.com> --- It took me hours to figure out how to setup NAND on my D-Link DIR-885L. This should be very helpful for ppl adding new devices support. --- drivers/mtd/nand/brcmnand/brcmnand.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c index 844fc07..f358cda 100644 --- a/drivers/mtd/nand/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/brcmnand/brcmnand.c @@ -615,6 +615,17 @@ static inline u32 brcmnand_ecc_level_mask(struct brcmnand_controller *ctrl) return mask << NAND_ACC_CONTROL_ECC_SHIFT; } +static int brcmnand_get_ecc_strength(struct brcmnand_host *host) +{ + struct brcmnand_controller *ctrl = host->ctrl; + u32 mask = brcmnand_ecc_level_mask(ctrl); + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, + BRCMNAND_CS_ACC_CONTROL); + + return (nand_readreg(ctrl, acc_control_offs) & mask) >> + NAND_ACC_CONTROL_ECC_SHIFT; +} + static void brcmnand_set_ecc_enabled(struct brcmnand_host *host, int en) { struct brcmnand_controller *ctrl = host->ctrl; @@ -1960,6 +1971,9 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn) nand_writereg(ctrl, cfg_offs, nand_readreg(ctrl, cfg_offs) & ~CFG_BUS_WIDTH); + chip->ecc.strength = brcmnand_get_ecc_strength(host); + chip->ecc.size = brcmnand_get_sector_size_1k(host) ? 1024 : 512; + if (nand_scan_ident(mtd, 1, NULL)) return -ENXIO; -- 1.8.4.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW 2016-01-27 7:58 [PATCH] mtd: brcmnand: set initial ECC params based on info from HW Rafał Miłecki @ 2016-02-01 17:53 ` Brian Norris 2016-02-01 18:09 ` Florian Fainelli 0 siblings, 1 reply; 6+ messages in thread From: Brian Norris @ 2016-02-01 17:53 UTC (permalink / raw) To: Rafał Miłecki Cc: Kamal Dasu, linux-mtd, bcm-kernel-feedback-list, Hauke Mehrtens On Wed, Jan 27, 2016 at 08:58:20AM +0100, Rafał Miłecki wrote: > So far we were depending on nand_dt_init getting ECC info from DT and > setting it in ECC struct. It is possible to simply read this info from > hardware registers which makes adding support for new hardware way > easier (no more guessing & trying all combinations). > Please note it still gives a precedence to DT which was overwrite > whatever was initially set by the driver. Precedence of DT is extremely important, but even with that, I'm not sure it's a great idea to support this by default. It's very error prone to rely on HW defaults at boot time; they might be left at their reset values. Anyway, if we are really making the DT properties optional, we'd need to modify Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt to reflect this. > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- > It took me hours to figure out how to setup NAND on my D-Link DIR-885L. > This should be very helpful for ppl adding new devices support. What if we just print out the hardware defaults when we bail out due to missing ECC DT properties? Then developers can choose to set up their DT with these properties, if those are actually proven correct. Would that save you the hours of setup you mention? Another option: maybe a commandline boot flag? > --- > drivers/mtd/nand/brcmnand/brcmnand.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c > index 844fc07..f358cda 100644 > --- a/drivers/mtd/nand/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/brcmnand/brcmnand.c > @@ -615,6 +615,17 @@ static inline u32 brcmnand_ecc_level_mask(struct brcmnand_controller *ctrl) > return mask << NAND_ACC_CONTROL_ECC_SHIFT; > } > > +static int brcmnand_get_ecc_strength(struct brcmnand_host *host) > +{ > + struct brcmnand_controller *ctrl = host->ctrl; > + u32 mask = brcmnand_ecc_level_mask(ctrl); > + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, > + BRCMNAND_CS_ACC_CONTROL); > + > + return (nand_readreg(ctrl, acc_control_offs) & mask) >> > + NAND_ACC_CONTROL_ECC_SHIFT; This is wrong. You ignore Hamming ECC and 1K sector sizes. With Hamming ECC, you need to translate a value of '15' to 'strength = 1'. And with 1K sector sizes, you need to translate a value of 'N' to 'strength = N * 2'. > +} > + > static void brcmnand_set_ecc_enabled(struct brcmnand_host *host, int en) > { > struct brcmnand_controller *ctrl = host->ctrl; > @@ -1960,6 +1971,9 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn) > nand_writereg(ctrl, cfg_offs, > nand_readreg(ctrl, cfg_offs) & ~CFG_BUS_WIDTH); > > + chip->ecc.strength = brcmnand_get_ecc_strength(host); > + chip->ecc.size = brcmnand_get_sector_size_1k(host) ? 1024 : 512; > + > if (nand_scan_ident(mtd, 1, NULL)) > return -ENXIO; > Brian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW 2016-02-01 17:53 ` Brian Norris @ 2016-02-01 18:09 ` Florian Fainelli 2016-02-01 19:02 ` Brian Norris 0 siblings, 1 reply; 6+ messages in thread From: Florian Fainelli @ 2016-02-01 18:09 UTC (permalink / raw) To: Brian Norris, Rafał Miłecki Cc: Kamal Dasu, linux-mtd, bcm-kernel-feedback-list, Hauke Mehrtens On 01/02/16 09:53, Brian Norris wrote: > On Wed, Jan 27, 2016 at 08:58:20AM +0100, Rafał Miłecki wrote: >> So far we were depending on nand_dt_init getting ECC info from DT and >> setting it in ECC struct. It is possible to simply read this info from >> hardware registers which makes adding support for new hardware way >> easier (no more guessing & trying all combinations). >> Please note it still gives a precedence to DT which was overwrite >> whatever was initially set by the driver. > > Precedence of DT is extremely important, but even with that, I'm not > sure it's a great idea to support this by default. It's very error prone > to rely on HW defaults at boot time; they might be left at their reset > values. I second that, that are platforms which are pretty much guaranteed to be broken. > > Anyway, if we are really making the DT properties optional, we'd need to > modify Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt to > reflect this. The Marvell PXA3XX NAND controller has a "marvell,nand-keep-config" which seems to be in par with what Rafal is after here, maybe it would be worth standardizing/mimicing that kind of property for the brcmnand driver? > >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> >> --- >> It took me hours to figure out how to setup NAND on my D-Link DIR-885L. >> This should be very helpful for ppl adding new devices support. > > What if we just print out the hardware defaults when we bail out due to > missing ECC DT properties? Then developers can choose to set up their DT > with these properties, if those are actually proven correct. Would that > save you the hours of setup you mention? > > Another option: maybe a commandline boot flag? One could argue that the bootloader is patf of the platform, and so, if the bootloader is known to provide good defaults, then a DT property could be appropriate. The commandline boot flag just sounds a little impractical, in case you have more than one NAND controller, but how common is that? > >> --- >> drivers/mtd/nand/brcmnand/brcmnand.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/mtd/nand/brcmnand/brcmnand.c b/drivers/mtd/nand/brcmnand/brcmnand.c >> index 844fc07..f358cda 100644 >> --- a/drivers/mtd/nand/brcmnand/brcmnand.c >> +++ b/drivers/mtd/nand/brcmnand/brcmnand.c >> @@ -615,6 +615,17 @@ static inline u32 brcmnand_ecc_level_mask(struct brcmnand_controller *ctrl) >> return mask << NAND_ACC_CONTROL_ECC_SHIFT; >> } >> >> +static int brcmnand_get_ecc_strength(struct brcmnand_host *host) >> +{ >> + struct brcmnand_controller *ctrl = host->ctrl; >> + u32 mask = brcmnand_ecc_level_mask(ctrl); >> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, >> + BRCMNAND_CS_ACC_CONTROL); >> + >> + return (nand_readreg(ctrl, acc_control_offs) & mask) >> >> + NAND_ACC_CONTROL_ECC_SHIFT; > > This is wrong. You ignore Hamming ECC and 1K sector sizes. With Hamming > ECC, you need to translate a value of '15' to 'strength = 1'. And with > 1K sector sizes, you need to translate a value of 'N' to 'strength = N * > 2'. > >> +} >> + >> static void brcmnand_set_ecc_enabled(struct brcmnand_host *host, int en) >> { >> struct brcmnand_controller *ctrl = host->ctrl; >> @@ -1960,6 +1971,9 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn) >> nand_writereg(ctrl, cfg_offs, >> nand_readreg(ctrl, cfg_offs) & ~CFG_BUS_WIDTH); >> >> + chip->ecc.strength = brcmnand_get_ecc_strength(host); >> + chip->ecc.size = brcmnand_get_sector_size_1k(host) ? 1024 : 512; >> + >> if (nand_scan_ident(mtd, 1, NULL)) >> return -ENXIO; >> > > Brian > -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW 2016-02-01 18:09 ` Florian Fainelli @ 2016-02-01 19:02 ` Brian Norris 2016-02-01 20:33 ` Florian Fainelli 0 siblings, 1 reply; 6+ messages in thread From: Brian Norris @ 2016-02-01 19:02 UTC (permalink / raw) To: Florian Fainelli Cc: Rafał Miłecki, Kamal Dasu, linux-mtd, bcm-kernel-feedback-list, Hauke Mehrtens, Boris Brezillon On Mon, Feb 01, 2016 at 10:09:28AM -0800, Florian Fainelli wrote: > On 01/02/16 09:53, Brian Norris wrote: > > Anyway, if we are really making the DT properties optional, we'd need to > > modify Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt to > > reflect this. > > The Marvell PXA3XX NAND controller has a "marvell,nand-keep-config" > which seems to be in par with what Rafal is after here, maybe it would > be worth standardizing/mimicing that kind of property for the brcmnand > driver? Maybe. Honestly, it's not that clear to me what the pxa3xx-nand binding means. But if we make that clear, that could be a possibility. (Side note: this isn't too far from what we did the Broadcom STB BSP previously, except that we wouldn't use device tree. We attempted a heuristic to detect whether the bootloader had initialized the flash controller properly... I didn't want to maintain that crazy logic upstream though. A DT flag to enable that behavior might be OK.) > >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > >> --- > >> It took me hours to figure out how to setup NAND on my D-Link DIR-885L. > >> This should be very helpful for ppl adding new devices support. > > > > What if we just print out the hardware defaults when we bail out due to > > missing ECC DT properties? Then developers can choose to set up their DT > > with these properties, if those are actually proven correct. Would that > > save you the hours of setup you mention? > > > > Another option: maybe a commandline boot flag? > > One could argue that the bootloader is patf of the platform, and so, if > the bootloader is known to provide good defaults, then a DT property > could be appropriate. The commandline boot flag just sounds a little > impractical, in case you have more than one NAND controller, but how > common is that? Not sure if you mean multiple *controller* or multiple *flash*. The former is much less common than the latter, but neither is very common AFAIK. Brian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW 2016-02-01 19:02 ` Brian Norris @ 2016-02-01 20:33 ` Florian Fainelli 2016-02-01 20:49 ` Brian Norris 0 siblings, 1 reply; 6+ messages in thread From: Florian Fainelli @ 2016-02-01 20:33 UTC (permalink / raw) To: Brian Norris, Florian Fainelli Cc: Rafał Miłecki, Kamal Dasu, linux-mtd, bcm-kernel-feedback-list, Hauke Mehrtens, Boris Brezillon On 01/02/16 11:02, Brian Norris wrote: > On Mon, Feb 01, 2016 at 10:09:28AM -0800, Florian Fainelli wrote: >> On 01/02/16 09:53, Brian Norris wrote: >>> Anyway, if we are really making the DT properties optional, we'd need to >>> modify Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt to >>> reflect this. >> >> The Marvell PXA3XX NAND controller has a "marvell,nand-keep-config" >> which seems to be in par with what Rafal is after here, maybe it would >> be worth standardizing/mimicing that kind of property for the brcmnand >> driver? > > Maybe. Honestly, it's not that clear to me what the pxa3xx-nand binding > means. But if we make that clear, that could be a possibility. I would argue that if you were down the road of looking at the bootloader defaults to figure out why things did not work, it's not that much of a stretch to put the correct information in Device Tree, leading to predictable results, so ... > > (Side note: this isn't too far from what we did the Broadcom STB BSP > previously, except that we wouldn't use device tree. We attempted a > heuristic to detect whether the bootloader had initialized the flash > controller properly... I didn't want to maintain that crazy logic > upstream though. A DT flag to enable that behavior might be OK.) Yes, I remember that part of the code, very much happy not to see it in any upstream tree TBH. > >>>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> >>>> --- >>>> It took me hours to figure out how to setup NAND on my D-Link DIR-885L. >>>> This should be very helpful for ppl adding new devices support. >>> >>> What if we just print out the hardware defaults when we bail out due to >>> missing ECC DT properties? Then developers can choose to set up their DT >>> with these properties, if those are actually proven correct. Would that >>> save you the hours of setup you mention? >>> >>> Another option: maybe a commandline boot flag? >> >> One could argue that the bootloader is patf of the platform, and so, if >> the bootloader is known to provide good defaults, then a DT property >> could be appropriate. The commandline boot flag just sounds a little >> impractical, in case you have more than one NAND controller, but how >> common is that? > > Not sure if you mean multiple *controller* or multiple *flash*. The > former is much less common than the latter, but neither is very common > AFAIK. Multiple controllers, having multiple chips on a single controller is indeed not that uncommon. Thanks -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW 2016-02-01 20:33 ` Florian Fainelli @ 2016-02-01 20:49 ` Brian Norris 0 siblings, 0 replies; 6+ messages in thread From: Brian Norris @ 2016-02-01 20:49 UTC (permalink / raw) To: Florian Fainelli Cc: Rafał Miłecki, Kamal Dasu, linux-mtd, bcm-kernel-feedback-list, Hauke Mehrtens, Boris Brezillon On Mon, Feb 01, 2016 at 12:33:06PM -0800, Florian Fainelli wrote: > On 01/02/16 11:02, Brian Norris wrote: > > On Mon, Feb 01, 2016 at 10:09:28AM -0800, Florian Fainelli wrote: > >> The Marvell PXA3XX NAND controller has a "marvell,nand-keep-config" > >> which seems to be in par with what Rafal is after here, maybe it would > >> be worth standardizing/mimicing that kind of property for the brcmnand > >> driver? > > > > Maybe. Honestly, it's not that clear to me what the pxa3xx-nand binding > > means. But if we make that clear, that could be a possibility. > > I would argue that if you were down the road of looking at the > bootloader defaults to figure out why things did not work, it's not that > much of a stretch to put the correct information in Device Tree, leading > to predictable results, so ... I wasn't suggesting trying to reintroduce any complex bootloader-matching logic. I think either we should stick with: (a) all info (ECC step size + strength) goes in DT (as-is) or (b) we provide a DT option to say "keep the ECC settings configured by the bootloader" If we go with (a), I'd still propose my original suggestion, if that helps Rafal's use case: > >>> What if we just print out the hardware defaults when we bail out due to > >>> missing ECC DT properties? Then developers can choose to set up their DT > >>> with these properties, if those are actually proven correct. Would that > >>> save you the hours of setup you mention? Regards, Brian ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-01 20:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-27 7:58 [PATCH] mtd: brcmnand: set initial ECC params based on info from HW Rafał Miłecki 2016-02-01 17:53 ` Brian Norris 2016-02-01 18:09 ` Florian Fainelli 2016-02-01 19:02 ` Brian Norris 2016-02-01 20:33 ` Florian Fainelli 2016-02-01 20:49 ` Brian Norris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).