Messages in this thread | | | Date | Thu, 22 Feb 2024 19:35:12 -0800 | Subject | Re: [PATCH v5 09/12] mtd: rawnand: brcmnand: Add support for getting ecc setting from strap | From | William Zhang <> |
| |
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 [unhandled content-type:application/pkcs7-signature] | |