RE: [PATCH 04/47] mtd: nand: adding ST's BCH NAND Controller driver

From: Gupta, Pekon
Date: Thu May 22 2014 - 14:05:45 EST


>From: Lee Jones
>
>First introduction of the driver. Includes the basic device struct
>(some functionality isn't utilised as of yet) and supplies some of the
>important resources required for basic running of the Controller.
>
>Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
>---
>--- /dev/null
>+++ b/drivers/mtd/nand/stm_nand_bch.c
[...]
>+
>+ uint32_t page_shift; /* Some working variables */
>+ uint32_t block_shift;

I think you don't really need these variables for 2 reasons:
(1) Wherever you are using these variables, you can derive
struct *nand_chip and instead directly use
chip->page_shift;
chip->phys_erase_shift;

(2) Difference chip-selects might be connected to different
NAND devices having different page-size and erase-size.
I'm not sure how that is handled in current generic NAND
framework (nand_base.c). But having different types of
NAND devices connected to different chip-selects is possible.


>+ uint32_t blocks_per_device;
>+ uint32_t sectors_per_page;
>+
>+ uint8_t *buf; /* Some buffers to use */
>+ uint8_t *page_buf;
>+ uint8_t *oob_buf;
>+ uint32_t *buf_list;
>+
>+ int cached_page; /* page number of page in */
>+ /* 'page_buf' */
>+};
>+
>+static int remap_named_resource(struct platform_device *pdev,
>+ char *name,
>+ void __iomem **io_ptr)
>+{
>+ struct resource *res, *mem;
>+ resource_size_t size;
>+ void __iomem *p;
>+
>+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>+ if (!res)
>+ return -ENXIO;
>+
>+ size = resource_size(res);
>+
>+ mem = devm_request_mem_region(&pdev->dev, res->start, size, name);
>+ if (!mem)
>+ return -EBUSY;
>+
>+ p = devm_ioremap_nocache(&pdev->dev, res->start, size);
>+ if (!p)
>+ return -ENOMEM;
>+
>+ *io_ptr = p;
>+
>+ return 0;
>+}
>+
>+static struct nandi_controller *
>+nandi_init_resources(struct platform_device *pdev)
>+{
>+ struct nandi_controller *nandi;
>+ int err;
>+
>+ nandi = devm_kzalloc(&pdev->dev, sizeof(*nandi), GFP_KERNEL);
>+ if (!nandi) {
>+ dev_err(&pdev->dev,
>+ "failed to allocate NANDi controller data\n");

Jingoo Han <jg1.han@xxxxxxxxxxx> has been cleaning these driver
specific OOM messages. So please drop dev_err(..) here. Refer
[Patch] mtd: xxxx: Remove unnecessary OOM messages
The site-specific OOM messages are unnecessary, because they
duplicate the MM subsystem generic OOM message.

>+ return ERR_PTR(-ENOMEM);
>+ }
>+
>+ nandi->dev = &pdev->dev;
>+
>+ err = remap_named_resource(pdev, "nand_mem", &nandi->base);
>+ if (err)
>+ return ERR_PTR(err);
>+
>+ err = remap_named_resource(pdev, "nand_dma", &nandi->dma);
>+ if (err)
>+ return ERR_PTR(err);
>+
>+ platform_set_drvdata(pdev, nandi);
>+
>+ return nandi;
>+}

[...]

>+
>+struct stm_plat_nand_bch_data {
>+ struct stm_nand_bank_data *bank;
>+ enum stm_nand_bch_ecc_config bch_ecc_cfg;
>+
>+ /* The threshold at which the number of corrected bit-flips per sector
>+ * is deemed to have reached an excessive level (triggers '-EUCLEAN' to
>+ * be returned to the caller). The value should be in the range 1 to
>+ * <ecc-strength> where <ecc-strength> is 18 or 30, depending on the BCH
>+ * ECC mode in operation. A value of 0 is interpreted by the driver as
>+ * <ecc-strength>.
>+ */
>+ unsigned int bch_bitflip_threshold;

As discussed in other thread, this is incorrect interpretation for
bitflip_threshold. As per MTD sub-system bitflips_threshold == 0
means ECC correction is not implemented.
@@drivers/mtd/mtdcore.c: mtd_read()
return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;

>+ bool flashss;

I could not find the use of this member I current series.
In your earlier version of patch this was used for DT binding "st,nand-flashss"
Am I missing something ?


with regards, pekon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/