| Message ID | 20240223034758.13753-13-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 Thu, 22 Feb 2024 19:47:57 -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. However these two > options can not be used at the same time. > > Signed-off-by: William Zhang <william.zhang@broadcom.com> > Reviewed-by: David Regan <dregan@broadcom.com> > FYI I did not receive patches 7, 8, 9, which makes the series numbering very odd. > --- > > Changes in v6: > - Combine the ecc step size and ecc strength into one get function > - Treat it as error condition if both brcm,nand-ecc-use-strap and nand > ecc dts properties are set > - Add intermediate steps to get the sector size bitfield > > 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, 77 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > index ef7d340475be..e8ffc283b365 100644 > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > @@ -1038,6 +1038,22 @@ 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 sector_size_bit = brcmnand_sector_1k_shift(ctrl); > + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, > + BRCMNAND_CS_ACC_CONTROL); > + u32 acc_control; > + > + if (sector_size_bit < 0) > + return 0; > + > + acc_control = nand_readreg(ctrl, acc_control_offs); > + > + return (acc_control & BIT(sector_size_bit)) >> sector_size_bit; FIELD_PREP, FIELD_GET, *please*. > +} > + > static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) > { > struct brcmnand_controller *ctrl = host->ctrl; > @@ -1055,6 +1071,43 @@ 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 void brcmnand_get_ecc_settings(struct brcmnand_host *host, struct nand_chip *chip) > +{ > + 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; > + 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; ditto > + if (sector_size_1k) > + chip->ecc.strength = ecc_level * 2; > + else if (spare_area_size == 16 && ecc_level == 15) > + chip->ecc.strength = 1; /* hamming */ > + else > + chip->ecc.strength = ecc_level; > + > + if (chip->ecc.size == 0) { > + if (sector_size_1k < 0) Should be <= 0 I guess > + chip->ecc.size = 512; > + else > + chip->ecc.size = 512 << sector_size_1k; What is this? Are you expecting sector_size_1k to be 0 or 1 and thus multiply 512 by two? Please just use: chip->ecc.size = SZ_1K; > + } > +} > + > /*********************************************************************** > * CS_NAND_SELECT > ***********************************************************************/ > @@ -2625,19 +2678,37 @@ 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; > + bool use_strap = false; > + char msg[128]; > int ret; > > 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", > + /* > + * Either nand-ecc-xxx or brcm,nand-ecc-use-strap can be set. Error out > + * if both exist. > + */ Thanks for the comment but I think the error string is clear enough. > + if (chip->ecc.strength && use_strap) { > + dev_err(ctrl->dev, > + "nand ecc and strap ecc settings can't be set at the same time\n"); Can we change to "ECC strap and DT ECC configuration properties are mutually exclusive" > + return -EINVAL; > + } > + > + if (use_strap) > + brcmnand_get_ecc_settings(host, chip); > + > + 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; > } The rest of the series looks good to me. Thanks, Miquèl
Hi Miquel, On 2/23/24 01:18, Miquel Raynal wrote: > Hi William, > > william.zhang@broadcom.com wrote on Thu, 22 Feb 2024 19:47:57 -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. However these two >> options can not be used at the same time. >> >> Signed-off-by: William Zhang <william.zhang@broadcom.com> >> Reviewed-by: David Regan <dregan@broadcom.com> >> > > FYI I did not receive patches 7, 8, 9, which makes the series numbering > very odd. > I was using the get maintainer script mainly and it sends to the linux MTD list. I will add your email directly next time. >> --- >> >> Changes in v6: >> - Combine the ecc step size and ecc strength into one get function >> - Treat it as error condition if both brcm,nand-ecc-use-strap and nand >> ecc dts properties are set >> - Add intermediate steps to get the sector size bitfield >> >> 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, 77 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> index ef7d340475be..e8ffc283b365 100644 >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >> @@ -1038,6 +1038,22 @@ 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 sector_size_bit = brcmnand_sector_1k_shift(ctrl); >> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, >> + BRCMNAND_CS_ACC_CONTROL); >> + u32 acc_control; >> + >> + if (sector_size_bit < 0) >> + return 0; >> + >> + acc_control = nand_readreg(ctrl, acc_control_offs); >> + >> + return (acc_control & BIT(sector_size_bit)) >> sector_size_bit; > > FIELD_PREP, FIELD_GET, *please*. You probably missed my reply to your comments on the same patch in v5. Here is the link for the post in case it lost in your email: https://lore.kernel.org/lkml/c145b90c-e9f0-4d82-94cc-baf7bfda5954@gmail.com/T/#m1d911d2f119f3bd345c575a81b60bc2bd8c461eb The mask is not constant here and cause build errors. > >> +} >> + >> static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) >> { >> struct brcmnand_controller *ctrl = host->ctrl; >> @@ -1055,6 +1071,43 @@ 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 void brcmnand_get_ecc_settings(struct brcmnand_host *host, struct nand_chip *chip) >> +{ >> + 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; >> + 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; > > ditto > >> + if (sector_size_1k) >> + chip->ecc.strength = ecc_level * 2; >> + else if (spare_area_size == 16 && ecc_level == 15) >> + chip->ecc.strength = 1; /* hamming */ >> + else >> + chip->ecc.strength = ecc_level; >> + >> + if (chip->ecc.size == 0) { >> + if (sector_size_1k < 0) > > Should be <= 0 I guess > >> + chip->ecc.size = 512; >> + else >> + chip->ecc.size = 512 << sector_size_1k; > > What is this? Are you expecting sector_size_1k to be 0 or 1 > and thus multiply 512 by two? > Explained in the same post above. Sector_size_1k can be negative number for error condition where we default to 512 step size. Otherwise 0 for 512 and 1 for 1K which the above shift takes care of. > Please just use: > chip->ecc.size = SZ_1K; > >> + } >> +} >> + >> /*********************************************************************** >> * CS_NAND_SELECT >> ***********************************************************************/ >> @@ -2625,19 +2678,37 @@ 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; >> + bool use_strap = false; >> + char msg[128]; >> int ret; >> >> 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", >> + /* >> + * Either nand-ecc-xxx or brcm,nand-ecc-use-strap can be set. Error out >> + * if both exist. >> + */ > > Thanks for the comment but I think the error string is clear enough. > >> + if (chip->ecc.strength && use_strap) { >> + dev_err(ctrl->dev, >> + "nand ecc and strap ecc settings can't be set at the same time\n"); > > Can we change to > "ECC strap and DT ECC configuration properties are mutually exclusive" > Will do. >> + return -EINVAL; >> + } >> + >> + if (use_strap) >> + brcmnand_get_ecc_settings(host, chip); >> + >> + 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; >> } > > The rest of the series looks good to me. > > Thanks, > Miquèl
Hi William, william.zhang@broadcom.com wrote on Fri, 23 Feb 2024 09:25:09 -0800: > Hi Miquel, > > On 2/23/24 01:18, Miquel Raynal wrote: > > Hi William, > > > > william.zhang@broadcom.com wrote on Thu, 22 Feb 2024 19:47:57 -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. However these two > >> options can not be used at the same time. > >> > >> Signed-off-by: William Zhang <william.zhang@broadcom.com> > >> Reviewed-by: David Regan <dregan@broadcom.com> > >> > > > > FYI I did not receive patches 7, 8, 9, which makes the series numbering > > very odd. > > > I was using the get maintainer script mainly and it sends to the linux MTD list. I will add your email directly next time. Yes, I prefer to be in Cc of the whole series, please. > >> --- > >> > >> Changes in v6: > >> - Combine the ecc step size and ecc strength into one get function > >> - Treat it as error condition if both brcm,nand-ecc-use-strap and nand > >> ecc dts properties are set > >> - Add intermediate steps to get the sector size bitfield > >> > >> 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, 77 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > >> index ef7d340475be..e8ffc283b365 100644 > >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c > >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c > >> @@ -1038,6 +1038,22 @@ 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 sector_size_bit = brcmnand_sector_1k_shift(ctrl); > >> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, > >> + BRCMNAND_CS_ACC_CONTROL); > >> + u32 acc_control; > >> + > >> + if (sector_size_bit < 0) > >> + return 0; > >> + > >> + acc_control = nand_readreg(ctrl, acc_control_offs); > >> + > >> + return (acc_control & BIT(sector_size_bit)) >> sector_size_bit; > > > > FIELD_PREP, FIELD_GET, *please*. > You probably missed my reply to your comments on the same patch in v5. Here is the link for the post in case it lost in your email: > https://lore.kernel.org/lkml/c145b90c-e9f0-4d82-94cc-baf7bfda5954@gmail.com/T/#m1d911d2f119f3bd345c575a81b60bc2bd8c461eb I didn't miss it, but the reason does not sound legitimate to me. Please work on it, it will be so much cleaner. > The mask is not constant here and cause build errors. > > > >> +} > >> + > >> static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) > >> { > >> struct brcmnand_controller *ctrl = host->ctrl; > >> @@ -1055,6 +1071,43 @@ 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 void brcmnand_get_ecc_settings(struct brcmnand_host *host, struct nand_chip *chip) > >> +{ > >> + 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; > >> + 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; > > > > ditto > > > >> + if (sector_size_1k) > >> + chip->ecc.strength = ecc_level * 2; > >> + else if (spare_area_size == 16 && ecc_level == 15) > >> + chip->ecc.strength = 1; /* hamming */ > >> + else > >> + chip->ecc.strength = ecc_level; > >> + > >> + if (chip->ecc.size == 0) { > >> + if (sector_size_1k < 0) > > > > Should be <= 0 I guess > > > >> + chip->ecc.size = 512; > >> + else > >> + chip->ecc.size = 512 << sector_size_1k; > > > > What is this? Are you expecting sector_size_1k to be 0 or 1 > > and thus multiply 512 by two? > > > Explained in the same post above. Sector_size_1k can be negative number for error condition where we default to 512 step size. Otherwise 0 for 512 and 1 for 1K which the above shift takes care of. The logic is unclear, unnatural. Please simplify. You have the possibility to change all the driver, so please simplify and clarify the logic. > > Please just use: > > chip->ecc.size = SZ_1K; > > Thanks, Miquèl
On 2/26/24 00:36, Miquel Raynal wrote: > Hi William, > > william.zhang@broadcom.com wrote on Fri, 23 Feb 2024 09:25:09 -0800: > >> Hi Miquel, >> >> On 2/23/24 01:18, Miquel Raynal wrote: >>> Hi William, >>> >>> william.zhang@broadcom.com wrote on Thu, 22 Feb 2024 19:47:57 -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. However these two >>>> options can not be used at the same time. >>>> >>>> Signed-off-by: William Zhang <william.zhang@broadcom.com> >>>> Reviewed-by: David Regan <dregan@broadcom.com> >>>> >>> >>> FYI I did not receive patches 7, 8, 9, which makes the series numbering >>> very odd. >>> >> I was using the get maintainer script mainly and it sends to the linux MTD list. I will add your email directly next time. > > Yes, I prefer to be in Cc of the whole series, please. > Sure. And thanks for applying other patches. Do you want me to just send a new single patch for the update? >>>> --- >>>> >>>> Changes in v6: >>>> - Combine the ecc step size and ecc strength into one get function >>>> - Treat it as error condition if both brcm,nand-ecc-use-strap and nand >>>> ecc dts properties are set >>>> - Add intermediate steps to get the sector size bitfield >>>> >>>> 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, 77 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>>> index ef7d340475be..e8ffc283b365 100644 >>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>>> @@ -1038,6 +1038,22 @@ 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 sector_size_bit = brcmnand_sector_1k_shift(ctrl); >>>> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, >>>> + BRCMNAND_CS_ACC_CONTROL); >>>> + u32 acc_control; >>>> + >>>> + if (sector_size_bit < 0) >>>> + return 0; >>>> + >>>> + acc_control = nand_readreg(ctrl, acc_control_offs); >>>> + >>>> + return (acc_control & BIT(sector_size_bit)) >> sector_size_bit; >>> >>> FIELD_PREP, FIELD_GET, *please*. >> You probably missed my reply to your comments on the same patch in v5. Here is the link for the post in case it lost in your email: >> https://lore.kernel.org/lkml/c145b90c-e9f0-4d82-94cc-baf7bfda5954@gmail.com/T/#m1d911d2f119f3bd345c575a81b60bc2bd8c461eb > > I didn't miss it, but the reason does not sound legitimate to me. > Please work on it, it will be so much cleaner. > I understand FIELD_PREP/GET is the preferred way of linux accessing the register fields but it requires a constant MASK value and does not apply to our case as we have different versions of the register and have different mask. There is way to workaround it. i.e defining the multiple constants directly and using these macros with if/else based on reg version. But it is not clean and since we already have helper functions that handle and return different shift/mask value, I see this is a perfect way for our situation and can adapt to future reg version change easily and cleanly. >> The mask is not constant here and cause build errors. >>> >>>> +} >>>> + >>>> static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) >>>> { >>>> struct brcmnand_controller *ctrl = host->ctrl; >>>> @@ -1055,6 +1071,43 @@ 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 void brcmnand_get_ecc_settings(struct brcmnand_host *host, struct nand_chip *chip) >>>> +{ >>>> + 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; >>>> + 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; >>> >>> ditto >>> >>>> + if (sector_size_1k) >>>> + chip->ecc.strength = ecc_level * 2; >>>> + else if (spare_area_size == 16 && ecc_level == 15) >>>> + chip->ecc.strength = 1; /* hamming */ >>>> + else >>>> + chip->ecc.strength = ecc_level; >>>> + >>>> + if (chip->ecc.size == 0) { >>>> + if (sector_size_1k < 0) >>> >>> Should be <= 0 I guess >>> >>>> + chip->ecc.size = 512; >>>> + else >>>> + chip->ecc.size = 512 << sector_size_1k; >>> >>> What is this? Are you expecting sector_size_1k to be 0 or 1 >>> and thus multiply 512 by two? >>> >> Explained in the same post above. Sector_size_1k can be negative number for error condition where we default to 512 step size. Otherwise 0 for 512 and 1 for 1K which the above shift takes care of. > > The logic is unclear, unnatural. Please simplify. You have the > possibility to change all the driver, so please simplify and clarify > the logic. > Will update. >>> Please just use: >>> chip->ecc.size = SZ_1K; >>> > > Thanks, > Miquèl
Hi William, william.zhang@broadcom.com wrote on Mon, 26 Feb 2024 12:05:18 -0800: > On 2/26/24 00:36, Miquel Raynal wrote: > > Hi William, > > > > william.zhang@broadcom.com wrote on Fri, 23 Feb 2024 09:25:09 -0800: > > > >> Hi Miquel, > >> > >> On 2/23/24 01:18, Miquel Raynal wrote: > >>> Hi William, > >>> > >>> william.zhang@broadcom.com wrote on Thu, 22 Feb 2024 19:47:57 -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. However these two > >>>> options can not be used at the same time. > >>>> > >>>> Signed-off-by: William Zhang <william.zhang@broadcom.com> > >>>> Reviewed-by: David Regan <dregan@broadcom.com> > >>>> >>> > >>> FYI I did not receive patches 7, 8, 9, which makes the series numbering > >>> very odd. > >>> >> I was using the get maintainer script mainly and it sends to the linux MTD list. I will add your email directly next time. > > > > Yes, I prefer to be in Cc of the whole series, please. > > > Sure. And thanks for applying other patches. Do you want me to just send a new single patch for the update? Yes just the missing patch. > >>>> >> +static int brcmnand_get_sector_size_1k(struct brcmnand_host *host) > >>>> +{ > >>>> + struct brcmnand_controller *ctrl = host->ctrl; > >>>> + int sector_size_bit = brcmnand_sector_1k_shift(ctrl); > >>>> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, > >>>> + BRCMNAND_CS_ACC_CONTROL); > >>>> + u32 acc_control; > >>>> + > >>>> + if (sector_size_bit < 0) > >>>> + return 0; > >>>> + > >>>> + acc_control = nand_readreg(ctrl, acc_control_offs); > >>>> + > >>>> + return (acc_control & BIT(sector_size_bit)) >> sector_size_bit; > >>> > >>> FIELD_PREP, FIELD_GET, *please*. > >> You probably missed my reply to your comments on the same patch in v5. Here is the link for the post in case it lost in your email: > >> https://lore.kernel.org/lkml/c145b90c-e9f0-4d82-94cc-baf7bfda5954@gmail.com/T/#m1d911d2f119f3bd345c575a81b60bc2bd8c461eb > > > > I didn't miss it, but the reason does not sound legitimate to me. > > Please work on it, it will be so much cleaner. > > > I understand FIELD_PREP/GET is the preferred way of linux accessing the register fields but it requires a constant MASK value and does not apply to our case as we have different versions of the register and have different mask. There is way to workaround it. i.e defining the multiple constants directly and using these macros with if/else based on reg version. But it is not clean and since we already have helper functions that handle and return different shift/mask value, I see this is a perfect way for our situation and can adapt to future reg version change easily and cleanly. > > >> The mask is not constant here and cause build errors. Which errors? + acc_control = nand_readreg(ctrl, acc_control_offs); + return FIELD_GET(BIT(sector_size_bit), acc_control); Does not return any error here. Thanks, Miquèl
Hi Minquel, On 2/29/24 02:31, Miquel Raynal wrote: > Hi William, > > william.zhang@broadcom.com wrote on Mon, 26 Feb 2024 12:05:18 -0800: > >> On 2/26/24 00:36, Miquel Raynal wrote: >>> Hi William, >>> >>> william.zhang@broadcom.com wrote on Fri, 23 Feb 2024 09:25:09 -0800: >>> >>>> Hi Miquel, >>>> >>>> On 2/23/24 01:18, Miquel Raynal wrote: >>>>> Hi William, >>>>> >>>>> william.zhang@broadcom.com wrote on Thu, 22 Feb 2024 19:47:57 -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. However these two >>>>>> options can not be used at the same time. >>>>>> >>>>>> Signed-off-by: William Zhang <william.zhang@broadcom.com> >>>>>> Reviewed-by: David Regan <dregan@broadcom.com> >>>>>> >>> >>>>> FYI I did not receive patches 7, 8, 9, which makes the series numbering >>>>> very odd. >>>>> >> I was using the get maintainer script mainly and it sends to the linux MTD list. I will add your email directly next time. >>> >>> Yes, I prefer to be in Cc of the whole series, please. >>> >> Sure. And thanks for applying other patches. Do you want me to just send a new single patch for the update? > > Yes just the missing patch. > v7 of this patch was sent early >>>>>> >> +static int brcmnand_get_sector_size_1k(struct brcmnand_host *host) >>>>>> +{ >>>>>> + struct brcmnand_controller *ctrl = host->ctrl; >>>>>> + int sector_size_bit = brcmnand_sector_1k_shift(ctrl); >>>>>> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, >>>>>> + BRCMNAND_CS_ACC_CONTROL); >>>>>> + u32 acc_control; >>>>>> + >>>>>> + if (sector_size_bit < 0) >>>>>> + return 0; >>>>>> + >>>>>> + acc_control = nand_readreg(ctrl, acc_control_offs); >>>>>> + >>>>>> + return (acc_control & BIT(sector_size_bit)) >> sector_size_bit; >>>>> >>>>> FIELD_PREP, FIELD_GET, *please*. >>>> You probably missed my reply to your comments on the same patch in v5. Here is the link for the post in case it lost in your email: >>>> https://lore.kernel.org/lkml/c145b90c-e9f0-4d82-94cc-baf7bfda5954@gmail.com/T/#m1d911d2f119f3bd345c575a81b60bc2bd8c461eb >>> >>> I didn't miss it, but the reason does not sound legitimate to me. >>> Please work on it, it will be so much cleaner. >>> >> I understand FIELD_PREP/GET is the preferred way of linux accessing the register fields but it requires a constant MASK value and does not apply to our case as we have different versions of the register and have different mask. There is way to workaround it. i.e defining the multiple constants directly and using these macros with if/else based on reg version. But it is not clean and since we already have helper functions that handle and return different shift/mask value, I see this is a perfect way for our situation and can adapt to future reg version change easily and cleanly. >> >>>> The mask is not constant here and cause build errors. > > Which errors? > If I use the code below In function brcmnand_get_sector_size_1k, inlined from brcmnand_get_ecc_settings at drivers/mtd/nand/raw/brcmnand/brcmnand.c:1089:24, inlined from brcmnand_setup_dev at drivers/mtd/nand/raw/brcmnand/brcmnand.c:2701:3: ././include/linux/compiler_types.h:442:38: error: call to compiletime_assert_254 declared with attribute error: FIELD_GET: mask is not constant > + acc_control = nand_readreg(ctrl, acc_control_offs); > + return FIELD_GET(BIT(sector_size_bit), acc_control); > > Does not return any error here. > Right, this function does not return error. brcmnand_sector_1k_shift does. I didn't make that clear enough. This function is updated and logic that calls this function is simplified in v7 based on your feedbacks. > Thanks, > Miquèl
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c index ef7d340475be..e8ffc283b365 100644 --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c @@ -1038,6 +1038,22 @@ 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 sector_size_bit = brcmnand_sector_1k_shift(ctrl); + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs, + BRCMNAND_CS_ACC_CONTROL); + u32 acc_control; + + if (sector_size_bit < 0) + return 0; + + acc_control = nand_readreg(ctrl, acc_control_offs); + + return (acc_control & BIT(sector_size_bit)) >> sector_size_bit; +} + static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val) { struct brcmnand_controller *ctrl = host->ctrl; @@ -1055,6 +1071,43 @@ 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 void brcmnand_get_ecc_settings(struct brcmnand_host *host, struct nand_chip *chip) +{ + 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; + 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) + chip->ecc.strength = ecc_level * 2; + else if (spare_area_size == 16 && ecc_level == 15) + chip->ecc.strength = 1; /* hamming */ + else + chip->ecc.strength = ecc_level; + + if (chip->ecc.size == 0) { + if (sector_size_1k < 0) + chip->ecc.size = 512; + else + chip->ecc.size = 512 << sector_size_1k; + } +} + /*********************************************************************** * CS_NAND_SELECT ***********************************************************************/ @@ -2625,19 +2678,37 @@ 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; + bool use_strap = false; + char msg[128]; int ret; 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", + /* + * Either nand-ecc-xxx or brcm,nand-ecc-use-strap can be set. Error out + * if both exist. + */ + if (chip->ecc.strength && use_strap) { + dev_err(ctrl->dev, + "nand ecc and strap ecc settings can't be set at the same time\n"); + return -EINVAL; + } + + if (use_strap) + brcmnand_get_ecc_settings(host, chip); + + 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; }