[PATCH] mtd: brcmnand: set initial ECC params based on info from HW

[PATCH] mtd: brcmnand: set initial ECC params based on info from HW

[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

Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW

[Brian Norris]

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

Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW

[Florian Fainelli]

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

Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW

[Brian Norris]

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

Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW

[Florian Fainelli]

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

Re: [PATCH] mtd: brcmnand: set initial ECC params based on info from HW

[Brian Norris]

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