Hi William,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.
william.zhang@xxxxxxxxxxxx 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@xxxxxxxxxxxx>
Reviewed-by: David Regan <dregan@xxxxxxxxxxxx>
---
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.
These macros do not work here as the mask is not constant.+}
+
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.
Will update.+ 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.
+ }
+
+ 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
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature