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

[Brian Norris <computersforpeace@gmail.com>]

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