Re: [RFC 13/47] mtd: nand: stm_nand_bch: provide Device Tree support

[Lee Jones <lee.jones@linaro.org>]

> >From: Lee Jones [mailto:lee.jones@linaro.org]
> >
> >Fetch platform specific data from Device Tree. Any functions which
> >are useful to other STM NAND Controllers have been separated into a
> >separate file so they can be easily referenced by them as they
> >appear.
> >
> >Signed-off-by: Lee Jones <lee.jones@linaro.org>

[...]

> >+#ifdef CONFIG_OF
> >+static void *stm_bch_dt_get_pdata(struct platform_device *pdev)
> >+{
> >+	struct device_node *np = pdev->dev.of_node;
> >+	struct stm_plat_nand_bch_data *pdata;
> >+	const char *ecc_config;
> >+	int ret;
> >+
> >+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> >+	if (!pdata)
> >+		return ERR_PTR(-ENOMEM);
> >+
> >+	of_property_read_string(np, "st,bch-ecc-mode", &ecc_config);
> 
> Can you use any of the existing generic NAND bindings like combination of
> "nand-ecc-mode" and "nand-ecc-strength" ?
> Example: nand-ecc-mode = "bch" & nand-ecc-strength=18
> Some of generic bindings are recently added
> 	commit 8dd49165ef5f46b5ad9ba296c559ccff315f9421  (currently in l2-mtd tree)
> 	mtd: nand: Add a devicetree binding for ECC strength and ECC step size
> 
> It's good to use generic bindings as much as you can, and you don't need
> any approvals from DT Maintainers too. Though you may need to add
> new values for "nand-ecc-mode" like {auto, hw-ham, hw-bch}

Sure, I can make use of the generic bindings already offered here.

> >+	if (!strcmp("noecc", ecc_config))
> >+		pdata->bch_ecc_cfg = BCH_NO_ECC;
> >+	else if (!strcmp("18bit", ecc_config))
> >+		pdata->bch_ecc_cfg = BCH_18BIT_ECC;
> >+	else if (!strcmp("30bit", ecc_config))
> >+		pdata->bch_ecc_cfg = BCH_30BIT_ECC;
> >+	else
> >+		pdata->bch_ecc_cfg = BCH_ECC_AUTO;
> >+
> >+	ret = stm_of_get_nand_banks(&pdev->dev, np, &pdata->bank);
> >+	if (ret < 0)
> >+		return ERR_PTR(ret);
> >+
> >+	pdata->flashss = of_property_read_bool(np, "st,nand-flashss");
> >+
> >+	of_property_read_u32(np, "st,bch-bitflip-threshold",
> >+			     &pdata->bch_bitflip_threshold);
> >+
> mtd->bitflip_threshold is by default set to ecc.strength (unless a driver initializes it).
> And then can be re-configured for each MTD partition separately 
> 	/sys/class/mtd/mtdX/bitflip_threshold
> 	Refer: $kernel/Documentation/ABI/testing/sysfs-class-mtd
> So, I don't think this is a HW parameter, and so should not be passed from DT.

I think the bit-flip threshold is/can be chip specific, and as we know
which chip we're likely to be booting on, we can specify this via the
hardware description.  Thus, I think it's perfectly viable for an
option to pass through DT to exist.


> >+struct device_node *stm_of_get_partitions_node(struct device_node *np,
> >+					       int bank_nr)
> >+{
> >+	struct device_node *banksnp, *banknp, *partsnp = NULL;
> >+	char name[10];
> >+
> >+	banksnp = of_parse_phandle(np, "st,nand-banks", 0);
> >+	if (!banksnp)
> >+		return NULL;
> >+
> >+	sprintf(name, "bank%d", bank_nr);
> >+	banknp = of_get_child_by_name(banksnp, name);
> >+	if (banknp)
> >+		return NULL;
> >+
> >+	partsnp = of_get_child_by_name(banknp, "partitions");
> >+	of_node_put(banknp);
> >+
> Sorry, I'm bit confused here .. I think you don't need to find children of
> Your bank node. This should already taken care in default parser
> drivers/mtd/ofpart.c : parse_ofpart_partitions()
> And all you need to pass is 'of_node' of bank (device).
> Is my understanding correct ?

We have 3 options here, you _can_ use parse_ofpart_partitions() if
your partition information conforms to its schema, but said schema
does not support banks and/or other information that we choose to
place within the bank node.  The second option is to register a
parser.  My personal view is that registering a parser is using the
framework for 'using the framework's' sake i.e. doesn't actually
achieve anything special.  We've chosen the third option, which is to
parse and extract the information ourselves - which is actually fairly
trivial, and pass the required partition data in through the
mtd_device_parse_register() call - which is where the information
would be parsed in the case of the first two options.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog