Re: [RFC PATCH 1/5] mtd: nand: ecc-qcom: Add support for ECC Engine Driver

[Md Sadre Alam <quic_mdalam@quicinc.com>]

On 10/31/2023 8:58 PM, Miquel Raynal wrote:
> Hi,
> 
> quic_mdalam@quicinc.com wrote on Tue, 31 Oct 2023 17:33:03 +0530:
> 
> Commit log is missing.

Having a separate device node for ECC was NAK-ed
https://www.spinics.net/lists/linux-arm-msm/msg177596.html

It is fine to drop this patch ? keep ECC support inlined in both
raw nand and Serial nand driver.


> 
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> 
> If Sricharan is a co developer you need to use the right tags. Please
> have a look at the documentation. Using the two SoB here does not mean
> anything
Ok will fix

> 
>> ---
>>   drivers/mtd/nand/Kconfig    |   7 ++
>>   drivers/mtd/nand/Makefile   |   1 +
>>   drivers/mtd/nand/ecc-qcom.c | 198 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 206 insertions(+)
>>   create mode 100644 drivers/mtd/nand/ecc-qcom.c
>>
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index 5b0c2c95f10c..333cec8187c8 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -61,6 +61,13 @@ config MTD_NAND_ECC_MEDIATEK
>>   	help
>>   	  This enables support for the hardware ECC engine from Mediatek.
>>   
>> +config MTD_NAND_ECC_QCOM
>> +	tristate "Qualcomm hardware ECC engine"
>> +	depends on ARCH_QCOM
> 
> Same comment as Mark regarding COMPILE_TEST
Ok
> 
>> +	select MTD_NAND_ECC
>> +	help
>> +	  This enables support for the hardware ECC engine from Qualcomm.
>> +
>>   endmenu
>>   
>>   endmenu
>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> index 19e1291ac4d5..c73b8a3456ec 100644
>> --- a/drivers/mtd/nand/Makefile
>> +++ b/drivers/mtd/nand/Makefile
>> @@ -3,6 +3,7 @@
>>   nandcore-objs := core.o bbt.o
>>   obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
>>   obj-$(CONFIG_MTD_NAND_ECC_MEDIATEK) += ecc-mtk.o
>> +obj-$(CONFIG_MTD_NAND_ECC_QCOM) += ecc-qcom.o qpic_common.o
>>   
>>   obj-y	+= onenand/
>>   obj-y	+= raw/
>> diff --git a/drivers/mtd/nand/ecc-qcom.c b/drivers/mtd/nand/ecc-qcom.c
>> new file mode 100644
>> index 000000000000..a85423ed368a
>> --- /dev/null
>> +++ b/drivers/mtd/nand/ecc-qcom.c
>> @@ -0,0 +1,198 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * QCOM ECC Engine  Driver.
>> + * Copyright (C) 2023  Qualcomm Inc.
>> + * Authors:	Md sadre Alam		<quic_mdalam@quicinc.com>
>> + *		Sricharan R		<quic_srichara@quicinc.com>
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/clk.h>
>> +#include <linux/module.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/mutex.h>
>> +#include <linux/mtd/nand-qpic-common.h>
>> +
>> +
>> +
>> +/* ECC modes supported by the controller */
>> +#define ECC_NONE        BIT(0)
>> +#define ECC_RS_4BIT     BIT(1)
>> +#define ECC_BCH_4BIT    BIT(2)
>> +#define ECC_BCH_8BIT    BIT(3)
>> +
>> +struct qpic_ecc_caps {
>> +	u32 err_mask;
>> +	u32 err_shift;
>> +	const u8 *ecc_strength;
>> +	const u32 *ecc_regs;
>> +	u8 num_ecc_strength;
>> +	u8 ecc_mode_shift;
>> +	u32 parity_bits;
>> +	int pg_irq_sel;
>> +};
>> +
>> +
>> +struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip)
>> +{
>> +	return container_of(chip, struct qcom_nand_host, chip);
>> +}
>> +EXPORT_SYMBOL(to_qcom_nand_host);
>> +
>> +struct qcom_nand_controller *
>> +get_qcom_nand_controller(struct nand_chip *chip)
>> +{
>> +	return container_of(chip->controller, struct qcom_nand_controller,
>> +			    controller);
>> +}
>> +EXPORT_SYMBOL(get_qcom_nand_controller);
>> +
>> +static struct qpic_ecc *qpic_ecc_get(struct device_node *np)
>> +{
>> +	struct platform_device *pdev;
>> +	struct qpic_ecc *ecc;
>> +
>> +	pdev = of_find_device_by_node(np);
>> +	if (!pdev)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	ecc = platform_get_drvdata(pdev);
>> +	if (!ecc) {
>> +		put_device(&pdev->dev);
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +	}
>> +
>> +	return ecc;
>> +}
>> +
>> +struct qpic_ecc *of_qpic_ecc_get(struct device_node *of_node)
>> +{
>> +	struct qpic_ecc *ecc = NULL;
>> +	struct device_node *np;
>> +
>> +	np = of_parse_phandle(of_node, "nand-ecc-engine", 0);
>> +	/* for backward compatibility */
> 
> There is no backward compatibility to handle upstream

Ok will fix in V1

> 
>> +	if (!np)
>> +		np = of_parse_phandle(of_node, "ecc-engine", 0);
>> +	if (np) {
>> +		ecc = qpic_ecc_get(np);
>> +		of_node_put(np);
>> +	}
>> +
>> +	return ecc;
>> +}
>> +EXPORT_SYMBOL(of_qpic_ecc_get);
>> +
>> +int qcom_ecc_config(struct qpic_ecc  *ecc, int ecc_strength,
>> +			bool wide_bus)
>> +{
>> +	ecc->ecc_modes = (ECC_RS_4BIT | ECC_BCH_8BIT);
>> +
>> +	if (ecc_strength >= 8) {
> 
> If your engine does not support more than an 8-bit strength this
> condition seems a bit strange.

Max ECC supported 8-bit only, forcing it to 8-bit.

> 
>> +		/* 8 bit ECC defaults to BCH ECC on all platforms */
>> +		ecc->bch_enabled = true;
>> +		ecc->ecc_mode = 1;
> 
> ecc_modes above, ecc_mode here, not very clear what this is.
> Please give meaningful names to your variables, not just the bit name
> that this is capturing because here it's unclear what this is.

ok will fix in V1

> 
>> +
>> +		if (wide_bus) {
>> +			ecc->ecc_bytes_hw = 14;
>> +			ecc->spare_bytes = 0;
> 
> Spare bytes depend on the flash, you can't use constant values like
> that.
Ok will fix in V1
> 
> I also don't understand what wide_bus is and why it has an impact of
> only 1 on the number of ECC bytes. Please make all this more explicit.

wide_bus 1 means 16-bit wide and wide_bus 0 means 8-bit wide.
there different configuration for ecc config for 16-bit wide bus
and 8-bit wide bus. This is recommended configuration by IP team,
Will reconfirm this with IP folks and come back.


Regards,
Alam.