| Message ID | 20240207202257.271784-10-william.zhang@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: rawnand: brcmnand: driver and doc updates | expand |
Hi William, william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:54 -0800: > BCMBCA broadband SoC based board design does not specify ecc setting in > dts but rather use the SoC NAND strap info to obtain the ecc strength > and spare area size setting. Add brcm,nand-ecc-use-strap dts propety for > this purpose and update driver to support this option. > > The generic nand ecc settings still take precedence over this flag. For > example, if nand-ecc-strength is set in the dts, the driver ignores the > strap setting and falls back to original behavior. This makes sure that > the existing BCMBCA board dts still works the old way even the strap > flag is set in the BCMBCA chip dtsi. > > Signed-off-by: William Zhang <william.zhang@broadcom.com> > Reviewed-by: David Regan <dregan@broadcom.com> > > --- > > Changes in v5: None > Changes in v4: > - Update the comments for ecc setting selection > > Changes in v3: None > Changes in v2: > - Minor cosmetic fixes > > drivers/mtd/nand/raw/brcmnand/brcmnand.c | 83 ++++++++++++++++++++++-- > 1 file changed, 76 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > index 73fdf7ce21aa..efeee9e80213 100644 > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > @@ -1038,6 +1038,19 @@ static inline int brcmnand_sector_1k_shift(struct brcmnand_controller *ctrl) > return -1; > } > > +static int brcmnand_get_sector_size_1k(struct brcmnand_host *host) > +{ > + struct brcmnand_controller *ctrl = host->ctrl; > + int shift = brcmnand_sector_1k_shift(ctrl); > + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, > + BRCMNAND_CS_ACC_CONTROL); > + > + if (shift < 0) > + return 0; > + > + return (nand_readreg(ctrl, acc_control_offs) >> shift) & 0x1; What is this & 0x1 ? If you return a yes/no value, please make this function return a bool. Also, please use intermediate steps to clarify what you do. sector_1k_bit = ...; acc = nand_readreg(); return acc & BIT(sector_1k_bit); Or something like that. > +} > + > static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) > { > struct brcmnand_controller *ctrl = host->ctrl; > @@ -1055,6 +1068,38 @@ static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) > nand_writereg(ctrl, acc_control_offs, tmp); > } > > +static int brcmnand_get_spare_size(struct brcmnand_host *host) > +{ > + struct brcmnand_controller *ctrl = host->ctrl; > + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, > + BRCMNAND_CS_ACC_CONTROL); > + u32 acc = nand_readreg(ctrl, acc_control_offs); > + > + return (acc & brcmnand_spare_area_mask(ctrl)); > +} > + > +static int brcmnand_get_ecc_strength(struct brcmnand_host *host) _from_strap > +{ > + struct brcmnand_controller *ctrl = host->ctrl; > + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, > + BRCMNAND_CS_ACC_CONTROL); > + int sector_size_1k = brcmnand_get_sector_size_1k(host); > + int spare_area_size, ecc_level, ecc_strength; > + u32 acc; > + > + spare_area_size = brcmnand_get_spare_size(host); > + acc = nand_readreg(ctrl, acc_control_offs); > + ecc_level = (acc & brcmnand_ecc_level_mask(ctrl)) >> ctrl->ecc_level_shift; Please use FIELD_PREP/FIELD_GET. > + if (sector_size_1k) > + ecc_strength = ecc_level * 2; > + else if (spare_area_size == 16 && ecc_level == 15) > + ecc_strength = 1; /* hamming */ > + else > + ecc_strength = ecc_level; > + > + return ecc_strength; > +} > + > /*********************************************************************** > * CS_NAND_SELECT > ***********************************************************************/ > @@ -2622,19 +2667,43 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) > nanddev_get_memorg(&chip->base); > struct brcmnand_controller *ctrl = host->ctrl; > struct brcmnand_cfg *cfg = &host->hwcfg; > - char msg[128]; > + struct device_node *np = nand_get_flash_node(chip); > u32 offs, tmp, oob_sector; > - int ret; > + int ret, sector_size_1k = 0; > + bool use_strap = false; > + char msg[128]; > > memset(cfg, 0, sizeof(*cfg)); > + use_strap = of_property_read_bool(np, "brcm,nand-ecc-use-strap"); > > - ret = of_property_read_u32(nand_get_flash_node(chip), > - "brcm,nand-oob-sector-size", > + /* > + * Set ECC size and strength based on hw configuration from strap > + * if brcm,nand-ecc-use-strap is set. However if nand-ecc-strength > + * is set, its value will be used and ignore the strap setting. Please error out in this case. It's one or the other, not both. > + */ > + if (chip->ecc.strength) > + use_strap = 0; > + > + if (use_strap) { > + chip->ecc.strength = brcmnand_get_ecc_strength(host); > + sector_size_1k = brcmnand_get_sector_size_1k(host); > + if (chip->ecc.size == 0) { > + if (sector_size_1k < 0) > + chip->ecc.size = 512; > + else > + chip->ecc.size = 512 << sector_size_1k; > + } I'd instead make a function named brcmnand_get_ecc_settings() with the chip->ecc parameter, so you can directly fill the entries without getting another time the sector_size_1k thing. Strength and step size are tightly linked, it does make sense to derive them both at the same time. > + } > + > + ret = of_property_read_u32(np, "brcm,nand-oob-sector-size", > &oob_sector); > if (ret) { > - /* Use detected size */ > - cfg->spare_area_size = mtd->oobsize / > - (mtd->writesize >> FC_SHIFT); > + if (use_strap) > + cfg->spare_area_size = brcmnand_get_spare_size(host); > + else > + /* Use detected size */ > + cfg->spare_area_size = mtd->oobsize / > + (mtd->writesize >> FC_SHIFT); > } else { > cfg->spare_area_size = oob_sector; > } Thanks, Miquèl
Hi William, william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:54 -0800: > BCMBCA broadband SoC based board design does not specify ecc setting in > dts but rather use the SoC NAND strap info to obtain the ecc strength > and spare area size setting. Add brcm,nand-ecc-use-strap dts propety for > this purpose and update driver to support this option. > > The generic nand ecc settings still take precedence over this flag. For And this sentence can also be dropped. Please mention they cannot be used at the same time. > example, if nand-ecc-strength is set in the dts, the driver ignores the > strap setting and falls back to original behavior. This makes sure that > the existing BCMBCA board dts still works the old way even the strap > flag is set in the BCMBCA chip dtsi. I think I already said this was a bad idea IMHO :-) Thanks, Miquèl
Hi Miquel, On 2/20/24 01:53, Miquel Raynal wrote: > Hi William, > > william.zhang@broadcom.com wrote on Wed, 7 Feb 2024 12:22:54 -0800: > >> BCMBCA broadband SoC based board design does not specify ecc setting in >> dts but rather use the SoC NAND strap info to obtain the ecc strength >> and spare area size setting. Add brcm,nand-ecc-use-strap dts propety for >> this purpose and update driver to support this option. >> >> The generic nand ecc settings still take precedence over this flag. For >> example, if nand-ecc-strength is set in the dts, the driver ignores the >> strap setting and falls back to original behavior. This makes sure that >> the existing BCMBCA board dts still works the old way even the strap >> flag is set in the BCMBCA chip dtsi. >> >> Signed-off-by: William Zhang <william.zhang@broadcom.com> >> Reviewed-by: David Regan <dregan@broadcom.com> >> >> --- >> >> Changes in v5: None >> Changes in v4: >> - Update the comments for ecc setting selection >> >> Changes in v3: None >> Changes in v2: >> - Minor cosmetic fixes >> >> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 83 ++++++++++++++++++++++-- >> 1 file changed, 76 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> index 73fdf7ce21aa..efeee9e80213 100644 >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> @@ -1038,6 +1038,19 @@ static inline int brcmnand_sector_1k_shift(struct brcmnand_controller *ctrl) >> return -1; >> } >> >> +static int brcmnand_get_sector_size_1k(struct brcmnand_host *host) >> +{ >> + struct brcmnand_controller *ctrl = host->ctrl; >> + int shift = brcmnand_sector_1k_shift(ctrl); >> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, >> + BRCMNAND_CS_ACC_CONTROL); >> + >> + if (shift < 0) >> + return 0; >> + >> + return (nand_readreg(ctrl, acc_control_offs) >> shift) & 0x1; > > What is this & 0x1 ? If you return a yes/no value, please make this > function return a bool. Also, please use intermediate steps to clarify > what you do. > > sector_1k_bit = ...; > acc = nand_readreg(); > return acc & BIT(sector_1k_bit); > > Or something like that. > This sector 1k bit is just single bit field. But we do want to return negative for error condition and 0 for 512 sector and 1 for 1K sector size. Will add intermediate steps. >> +} >> + >> static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) >> { >> struct brcmnand_controller *ctrl = host->ctrl; >> @@ -1055,6 +1068,38 @@ static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) >> nand_writereg(ctrl, acc_control_offs, tmp); >> } >> >> +static int brcmnand_get_spare_size(struct brcmnand_host *host) >> +{ >> + struct brcmnand_controller *ctrl = host->ctrl; >> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, >> + BRCMNAND_CS_ACC_CONTROL); >> + u32 acc = nand_readreg(ctrl, acc_control_offs); >> + >> + return (acc & brcmnand_spare_area_mask(ctrl)); >> +} >> + >> +static int brcmnand_get_ecc_strength(struct brcmnand_host *host) > > _from_strap > >> +{ >> + struct brcmnand_controller *ctrl = host->ctrl; >> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, >> + BRCMNAND_CS_ACC_CONTROL); >> + int sector_size_1k = brcmnand_get_sector_size_1k(host); >> + int spare_area_size, ecc_level, ecc_strength; >> + u32 acc; >> + >> + spare_area_size = brcmnand_get_spare_size(host); >> + acc = nand_readreg(ctrl, acc_control_offs); >> + ecc_level = (acc & brcmnand_ecc_level_mask(ctrl)) >> ctrl->ecc_level_shift; > > Please use FIELD_PREP/FIELD_GET. > These macros do not work here as the mask is not constant. >> + if (sector_size_1k) >> + ecc_strength = ecc_level * 2; >> + else if (spare_area_size == 16 && ecc_level == 15) >> + ecc_strength = 1; /* hamming */ >> + else >> + ecc_strength = ecc_level; >> + >> + return ecc_strength; >> +} >> + >> /*********************************************************************** >> * CS_NAND_SELECT >> ***********************************************************************/ >> @@ -2622,19 +2667,43 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) >> nanddev_get_memorg(&chip->base); >> struct brcmnand_controller *ctrl = host->ctrl; >> struct brcmnand_cfg *cfg = &host->hwcfg; >> - char msg[128]; >> + struct device_node *np = nand_get_flash_node(chip); >> u32 offs, tmp, oob_sector; >> - int ret; >> + int ret, sector_size_1k = 0; >> + bool use_strap = false; >> + char msg[128]; >> >> memset(cfg, 0, sizeof(*cfg)); >> + use_strap = of_property_read_bool(np, "brcm,nand-ecc-use-strap"); >> >> - ret = of_property_read_u32(nand_get_flash_node(chip), >> - "brcm,nand-oob-sector-size", >> + /* >> + * Set ECC size and strength based on hw configuration from strap >> + * if brcm,nand-ecc-use-strap is set. However if nand-ecc-strength >> + * is set, its value will be used and ignore the strap setting. > > Please error out in this case. It's one or the other, not both. > Will update. >> + */ >> + if (chip->ecc.strength) >> + use_strap = 0; >> + >> + if (use_strap) { >> + chip->ecc.strength = brcmnand_get_ecc_strength(host); >> + sector_size_1k = brcmnand_get_sector_size_1k(host); >> + if (chip->ecc.size == 0) { >> + if (sector_size_1k < 0) >> + chip->ecc.size = 512; >> + else >> + chip->ecc.size = 512 << sector_size_1k; >> + } > > I'd instead make a function named brcmnand_get_ecc_settings() with the > chip->ecc parameter, so you can directly fill the entries without > getting another time the sector_size_1k thing. > > Strength and step size are tightly linked, it does make sense to derive > them both at the same time. > Will update. >> + } >> + >> + ret = of_property_read_u32(np, "brcm,nand-oob-sector-size", >> &oob_sector); >> if (ret) { >> - /* Use detected size */ >> - cfg->spare_area_size = mtd->oobsize / >> - (mtd->writesize >> FC_SHIFT); >> + if (use_strap) >> + cfg->spare_area_size = brcmnand_get_spare_size(host); >> + else >> + /* Use detected size */ >> + cfg->spare_area_size = mtd->oobsize / >> + (mtd->writesize >> FC_SHIFT); >> } else { >> cfg->spare_area_size = oob_sector; >> } > > > Thanks, > Miquèl
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c index 73fdf7ce21aa..efeee9e80213 100644 --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c @@ -1038,6 +1038,19 @@ static inline int brcmnand_sector_1k_shift(struct brcmnand_controller *ctrl) return -1; } +static int brcmnand_get_sector_size_1k(struct brcmnand_host *host) +{ + struct brcmnand_controller *ctrl = host->ctrl; + int shift = brcmnand_sector_1k_shift(ctrl); + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, + BRCMNAND_CS_ACC_CONTROL); + + if (shift < 0) + return 0; + + return (nand_readreg(ctrl, acc_control_offs) >> shift) & 0x1; +} + static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) { struct brcmnand_controller *ctrl = host->ctrl; @@ -1055,6 +1068,38 @@ static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) nand_writereg(ctrl, acc_control_offs, tmp); } +static int brcmnand_get_spare_size(struct brcmnand_host *host) +{ + struct brcmnand_controller *ctrl = host->ctrl; + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, + BRCMNAND_CS_ACC_CONTROL); + u32 acc = nand_readreg(ctrl, acc_control_offs); + + return (acc & brcmnand_spare_area_mask(ctrl)); +} + +static int brcmnand_get_ecc_strength(struct brcmnand_host *host) +{ + struct brcmnand_controller *ctrl = host->ctrl; + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, + BRCMNAND_CS_ACC_CONTROL); + int sector_size_1k = brcmnand_get_sector_size_1k(host); + int spare_area_size, ecc_level, ecc_strength; + u32 acc; + + spare_area_size = brcmnand_get_spare_size(host); + acc = nand_readreg(ctrl, acc_control_offs); + ecc_level = (acc & brcmnand_ecc_level_mask(ctrl)) >> ctrl->ecc_level_shift; + if (sector_size_1k) + ecc_strength = ecc_level * 2; + else if (spare_area_size == 16 && ecc_level == 15) + ecc_strength = 1; /* hamming */ + else + ecc_strength = ecc_level; + + return ecc_strength; +} + /*********************************************************************** * CS_NAND_SELECT ***********************************************************************/ @@ -2622,19 +2667,43 @@ static int brcmnand_setup_dev(struct brcmnand_host *host) nanddev_get_memorg(&chip->base); struct brcmnand_controller *ctrl = host->ctrl; struct brcmnand_cfg *cfg = &host->hwcfg; - char msg[128]; + struct device_node *np = nand_get_flash_node(chip); u32 offs, tmp, oob_sector; - int ret; + int ret, sector_size_1k = 0; + bool use_strap = false; + char msg[128]; memset(cfg, 0, sizeof(*cfg)); + use_strap = of_property_read_bool(np, "brcm,nand-ecc-use-strap"); - ret = of_property_read_u32(nand_get_flash_node(chip), - "brcm,nand-oob-sector-size", + /* + * Set ECC size and strength based on hw configuration from strap + * if brcm,nand-ecc-use-strap is set. However if nand-ecc-strength + * is set, its value will be used and ignore the strap setting. + */ + if (chip->ecc.strength) + use_strap = 0; + + if (use_strap) { + chip->ecc.strength = brcmnand_get_ecc_strength(host); + sector_size_1k = brcmnand_get_sector_size_1k(host); + if (chip->ecc.size == 0) { + if (sector_size_1k < 0) + chip->ecc.size = 512; + else + chip->ecc.size = 512 << sector_size_1k; + } + } + + ret = of_property_read_u32(np, "brcm,nand-oob-sector-size", &oob_sector); if (ret) { - /* Use detected size */ - cfg->spare_area_size = mtd->oobsize / - (mtd->writesize >> FC_SHIFT); + if (use_strap) + cfg->spare_area_size = brcmnand_get_spare_size(host); + else + /* Use detected size */ + cfg->spare_area_size = mtd->oobsize / + (mtd->writesize >> FC_SHIFT); } else { cfg->spare_area_size = oob_sector; }