Dear Miquel,
Thank you for your reply.
On 2024/11/12 上午 02:22, Miquel Raynal wrote:
Hello,
+static int ma35_nand_attach_chip(struct nand_chip *chip)
+{
+ struct ma35_nand_info *nand = nand_get_controller_data(chip);
+ struct ma35_nand_chip *nvtnand = to_ma35_nand(chip);
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct device *dev = mtd->dev.parent;
+ u32 reg;
+
+ if (chip->options & NAND_BUSWIDTH_16) {
+ dev_err(dev, "16 bits bus width not supported");
+ return -EINVAL;
+ }
+
+ nvtnand->nchunks = mtd->writesize / chip->ecc.steps;
+ nvtnand->nchunks = (nvtnand->nchunks < 4) ? 1 : nvtnand->nchunks / 4;
This second division looks broken. Also, you probably don't want to do
that outside of the ON_HOST situation. Finally, you should probably
update chip->ecc.steps and chip->ecc.size to your final choice.
Regarding these two lines, I will remove them and instead set them in
ma35_nand_hwecc_init().
However, since the nchunks is actually used to get the ECC status
register count,
I will rename nchunks to eccstatus and perform the calculation using
ecc.steps.
+
+ reg = readl(nand->regs + MA35_NFI_REG_NANDCTL) & (~PSIZE_MASK);
+ if (mtd->writesize == 2048)
+ writel(reg | PSIZE_2K, nand->regs + MA35_NFI_REG_NANDCTL);
+ else if (mtd->writesize == 4096)
+ writel(reg | PSIZE_4K, nand->regs + MA35_NFI_REG_NANDCTL);
+ else if (mtd->writesize == 8192)
+ writel(reg | PSIZE_8K, nand->regs + MA35_NFI_REG_NANDCTL);
+
+ switch (chip->ecc.engine_type) {
+ case NAND_ECC_ENGINE_TYPE_ON_HOST:
+ chip->options |= NAND_NO_SUBPAGE_WRITE | NAND_USES_DMA;
What is the reason for refusing subpage writes? This is not something
you can do later, so unless there is a good reason, please do not set
this flag.
I will remove the NAND_NO_SUBPAGE_WRITE flag.
+ chip->ecc.write_page = ma35_nand_write_page_hwecc;
+ chip->ecc.read_page = ma35_nand_read_page_hwecc;
+ chip->ecc.read_oob = ma35_nand_read_oob_hwecc;
+ return ma35_nand_hwecc_init(chip, nand);
+ case NAND_ECC_ENGINE_TYPE_NONE:
+ case NAND_ECC_ENGINE_TYPE_SOFT:
+ case NAND_ECC_ENGINE_TYPE_ON_DIE:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
...
+static int ma35_nand_chip_init(struct device *dev, struct ma35_nand_info *nand,
+ struct device_node *np)
+{
+ struct ma35_nand_chip *nvtnand;
+ struct nand_chip *chip;
+ struct mtd_info *mtd;
+ int nsels;
+ u32 tmp;
+ int ret;
+ int i;
+
+ if (!of_get_property(np, "reg", &nsels))
Please convert to device_property_ helpers. And remove the of include
once you no longer need it.
I will use device_property_read_u32 instead.
+ return -ENODEV;
+
+ nsels /= sizeof(u32);
+ if (!nsels || nsels > MA35_MAX_NSELS) {
+ dev_err(dev, "invalid reg property size %d\n", nsels);
+ return -EINVAL;
+ }
+
+ nvtnand = devm_kzalloc(dev, struct_size(nvtnand, sels, nsels),
+ GFP_KERNEL);
+ if (!nvtnand)
+ return -ENOMEM;
+
+ nvtnand->nsels = nsels;
+ for (i = 0; i < nsels; i++) {
+ ret = of_property_read_u32_index(np, "reg", i, &tmp);
+ if (ret) {
+ dev_err(dev, "reg property failure : %d\n", ret);
+ return ret;
+ }
+
+ if (tmp >= MA35_MAX_NSELS) {
+ dev_err(dev, "invalid CS: %u\n", tmp);
+ return -EINVAL;
+ }
+
+ if (test_and_set_bit(tmp, &nand->assigned_cs)) {
+ dev_err(dev, "CS %u already assigned\n", tmp);
+ return -EINVAL;
+ }
+
+ nvtnand->sels[i] = tmp;
+ }
+
...
+
+ ret = mtd_device_register(mtd, NULL, 0);
+ if (ret) {
+ dev_err(dev, "MTD parse partition error\n");
probably useless error message?
I will remove this error message.
+ nand_cleanup(chip);
+ return ret;
+ }
+
+ list_add_tail(&nvtnand->node, &nand->chips);
+
+ return 0;
+}
I believe next iteration should be the one, I'm rather happy with the
overall look.
Hope so, thank you!
Thanks,
Miquèl
Best regards,
Hui-Ping Chen
