diff mbox series

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

Message ID 20231031120307.1600689-2-quic_mdalam@quicinc.com
State New
Headers
Series Add QPIC SPI NAND driver support |

Commit Message

Md Sadre Alam Oct. 31, 2023, 12:03 p.m. UTC 
  Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
---
 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

Comments

Miquel Raynal Oct. 31, 2023, 3:28 p.m. UTC | #1 
Hi,

quic_mdalam@quicinc.com wrote on Tue, 31 Oct 2023 17:33:03 +0530:

Commit log is missing.

> 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.

> ---
>  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

> +	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

> +	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.

> +		/* 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.

> +
> +		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.

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.

> +			ecc->bbm_size = 2;
> +		} else {
> +			ecc->ecc_bytes_hw = 13;
> +			ecc->spare_bytes = 2;
> +			ecc->bbm_size = 1;
> +		}
> +	} else {
> +		/*
> +		 * if the controller supports BCH for 4 bit ECC, the controller
> +		 * uses lesser bytes for ECC. If RS is used, the ECC bytes is
> +		 * always 10 bytes
> +		 */
> +		if (ecc->ecc_modes & ECC_BCH_4BIT) {
> +			/* BCH */
> +			ecc->bch_enabled = true;
> +			ecc->ecc_mode = 0;
> +			if (wide_bus) {
> +				ecc->ecc_bytes_hw = 8;
> +				ecc->spare_bytes = 2;
> +				ecc->bbm_size = 2;
> +			} else {
> +				ecc->ecc_bytes_hw = 7;
> +				ecc->spare_bytes = 4;
> +				ecc->bbm_size = 1;
> +			}
> +		} else {
> +			/* RS */
> +			ecc->ecc_bytes_hw = 10;
> +			if (wide_bus) {
> +				ecc->spare_bytes = 0;
> +				ecc->bbm_size = 2;
> +			} else {
> +				ecc->spare_bytes = 1;
> +				ecc->bbm_size = 1;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(qcom_ecc_config);

Thanks,
Miquèl
Krzysztof Kozlowski Oct. 31, 2023, 5:11 p.m. UTC | #2 
On 31/10/2023 13:03, Md Sadre Alam wrote:

Eh? Empty?

> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> ---
>  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
> 

...

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(qcom_ecc_config);
> +
> +void qcom_ecc_enable(struct qcom_ecc *ecc)
> +{
> +	ecc->use_ecc = true;
> +}
> +EXPORT_SYMBOL(qcom_ecc_enable);

Drop this and all other exports. Nothing here explains the need for them.

> +
> +void qcom_ecc_disable(struct qcom_ecc *ecc)
> +{
> +	ecc->use_ecc = false;
> +}
> +EXPORT_SYMBOL(qcom_ecc_disable);
> +
> +static const struct of_device_id qpic_ecc_dt_match[] = {
> +	{
> +		.compatible = "qcom,ipq9574-ecc",

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Checkpatch is preerquisite. Don't send patches which have obvious issues
pointed out by checkpatch. It's a waste of reviewers time.

> +	},
> +	{},
> +};
> +
> +static int qpic_ecc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qpic_ecc *ecc;
> +	u32 max_eccdata_size;
> +
> +	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> +	if (!ecc)
> +		return -ENOMEM;
> +
> +	ecc->caps = of_device_get_match_data(dev);
> +
> +	ecc->dev = dev;
> +	platform_set_drvdata(pdev, ecc);
> +	dev_info(dev, "probed\n");

No, no such messages.



Best regards,
Krzysztof
Md Sadre Alam Nov. 3, 2023, 12:06 p.m. UTC | #3 
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.
Krzysztof Kozlowski Nov. 3, 2023, 12:08 p.m. UTC | #4 
On 03/11/2023 13:06, Md Sadre Alam wrote:
> 
> 
> 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 was NAKed because it was not ready for submission. Please perform
internal review, which will fix such trivial issues, before posting on
mailing lists.

Since you did not post any bindings, we could not have even discussion
whether this is acceptable or not...

Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 3, 2023, 12:11 p.m. UTC | #5 
On 03/11/2023 13:08, Krzysztof Kozlowski wrote:
> On 03/11/2023 13:06, Md Sadre Alam wrote:
>>
>>
>> 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 was NAKed because it was not ready for submission. Please perform
> internal review, which will fix such trivial issues, before posting on
> mailing lists.

To clarify: trivial issues including compile-like testing the code, not
even on the hardware, but with automated, open-source tools like
checkpatch, dtc and dtbs_check. It's like sending driver code which has
obvious warnings.

Best regards,
Krzysztof
Md Sadre Alam Nov. 3, 2023, 12:24 p.m. UTC | #6 
On 10/31/2023 10:41 PM, Krzysztof Kozlowski wrote:
> On 31/10/2023 13:03, Md Sadre Alam wrote:
> 
> Eh? Empty?

QPIC controller has the ecc pipelined so will keep the ecc support
inlined in both raw nand and serial nand driver.

Droping this driver since device node was NAK-ed
https://www.spinics.net/lists/linux-arm-msm/msg177596.html
> 
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
>> ---
>>   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
>>
> 
> ...
> 
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(qcom_ecc_config);
>> +
>> +void qcom_ecc_enable(struct qcom_ecc *ecc)
>> +{
>> +	ecc->use_ecc = true;
>> +}
>> +EXPORT_SYMBOL(qcom_ecc_enable);
> 
> Drop this and all other exports. Nothing here explains the need for them.
> 
>> +
>> +void qcom_ecc_disable(struct qcom_ecc *ecc)
>> +{
>> +	ecc->use_ecc = false;
>> +}
>> +EXPORT_SYMBOL(qcom_ecc_disable);
>> +
>> +static const struct of_device_id qpic_ecc_dt_match[] = {
>> +	{
>> +		.compatible = "qcom,ipq9574-ecc",
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
> 
> Checkpatch is preerquisite. Don't send patches which have obvious issues
> pointed out by checkpatch. It's a waste of reviewers time.
> 
>> +	},
>> +	{},
>> +};
>> +
>> +static int qpic_ecc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct qpic_ecc *ecc;
>> +	u32 max_eccdata_size;
>> +
>> +	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
>> +	if (!ecc)
>> +		return -ENOMEM;
>> +
>> +	ecc->caps = of_device_get_match_data(dev);
>> +
>> +	ecc->dev = dev;
>> +	platform_set_drvdata(pdev, ecc);
>> +	dev_info(dev, "probed\n");
> 
> No, no such messages.
> 
> 
> 
> Best regards,
> Krzysztof
>
Dmitry Baryshkov Nov. 3, 2023, 12:33 p.m. UTC | #7 
On Fri, 3 Nov 2023 at 14:25, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:
>
>
>
> On 10/31/2023 10:41 PM, Krzysztof Kozlowski wrote:
> > On 31/10/2023 13:03, Md Sadre Alam wrote:
> >
> > Eh? Empty?
>
> QPIC controller has the ecc pipelined so will keep the ecc support
> inlined in both raw nand and serial nand driver.
>
> Droping this driver since device node was NAK-ed
> https://www.spinics.net/lists/linux-arm-msm/msg177596.html

It seems, we have to repeat the same thing again and again:

It was not the device node that was NAKed. It was the patch that was
NAKed. Please read the emails carefully.

And next time please perform dtbs_check, dt_binding_check and
checkpatch before sending the patch.

It is perfectly fine to ask questions 'like we are getting we are
getting this and that issues with the bindings, please advise'. It is
not fine to skip that step completely.

> >
> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> >> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> >> ---
> >>   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
> >>
> >
> > ...
> >
> >> +
> >> +    return 0;
> >> +}
> >> +EXPORT_SYMBOL(qcom_ecc_config);
> >> +
> >> +void qcom_ecc_enable(struct qcom_ecc *ecc)
> >> +{
> >> +    ecc->use_ecc = true;
> >> +}
> >> +EXPORT_SYMBOL(qcom_ecc_enable);
> >
> > Drop this and all other exports. Nothing here explains the need for them.
> >
> >> +
> >> +void qcom_ecc_disable(struct qcom_ecc *ecc)
> >> +{
> >> +    ecc->use_ecc = false;
> >> +}
> >> +EXPORT_SYMBOL(qcom_ecc_disable);
> >> +
> >> +static const struct of_device_id qpic_ecc_dt_match[] = {
> >> +    {
> >> +            .compatible = "qcom,ipq9574-ecc",
> >
> > Please run scripts/checkpatch.pl and fix reported warnings. Some
> > warnings can be ignored, but the code here looks like it needs a fix.
> > Feel free to get in touch if the warning is not clear.
> >
> > Checkpatch is preerquisite. Don't send patches which have obvious issues
> > pointed out by checkpatch. It's a waste of reviewers time.
> >
> >> +    },
> >> +    {},
> >> +};
> >> +
> >> +static int qpic_ecc_probe(struct platform_device *pdev)
> >> +{
> >> +    struct device *dev = &pdev->dev;
> >> +    struct qpic_ecc *ecc;
> >> +    u32 max_eccdata_size;
> >> +
> >> +    ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> >> +    if (!ecc)
> >> +            return -ENOMEM;
> >> +
> >> +    ecc->caps = of_device_get_match_data(dev);
> >> +
> >> +    ecc->dev = dev;
> >> +    platform_set_drvdata(pdev, ecc);
> >> +    dev_info(dev, "probed\n");
> >
> > No, no such messages.
> >
> >
> >
> > Best regards,
> > Krzysztof
> >
Md Sadre Alam Nov. 3, 2023, 1:23 p.m. UTC | #8 
On 11/3/2023 6:03 PM, Dmitry Baryshkov wrote:
> On Fri, 3 Nov 2023 at 14:25, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:
>>
>>
>>
>> On 10/31/2023 10:41 PM, Krzysztof Kozlowski wrote:
>>> On 31/10/2023 13:03, Md Sadre Alam wrote:
>>>
>>> Eh? Empty?
>>
>> QPIC controller has the ecc pipelined so will keep the ecc support
>> inlined in both raw nand and serial nand driver.
>>
>> Droping this driver since device node was NAK-ed
>> https://www.spinics.net/lists/linux-arm-msm/msg177596.html
> 
> It seems, we have to repeat the same thing again and again:
> 
> It was not the device node that was NAKed. It was the patch that was
> NAKed. Please read the emails carefully.
> 
> And next time please perform dtbs_check, dt_binding_check and
> checkpatch before sending the patch.
> 
> It is perfectly fine to ask questions 'like we are getting we are
> getting this and that issues with the bindings, please advise'. It is
> not fine to skip that step completely.

Sorry in V1 will run all basic checks.

Based on below feedback [1] and NAK on the device node patch
got idea of having separate device node for ECC is not acceptable.
Could you please help to clarify that.

Since ECC block is inlined with QPIC controller so is the below
device node acceptable ?

    bch: qpic_ecc {
                           compatible = "qcom,ipq9574-ecc";
                           status = "ok";
                   };

  [1] https://www.spinics.net/lists/linux-arm-msm/msg177525.html

> 
>>>
>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>>>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
>>>> ---
>>>>    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
>>>>
>>>
>>> ...
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(qcom_ecc_config);
>>>> +
>>>> +void qcom_ecc_enable(struct qcom_ecc *ecc)
>>>> +{
>>>> +    ecc->use_ecc = true;
>>>> +}
>>>> +EXPORT_SYMBOL(qcom_ecc_enable);
>>>
>>> Drop this and all other exports. Nothing here explains the need for them.
>>>
>>>> +
>>>> +void qcom_ecc_disable(struct qcom_ecc *ecc)
>>>> +{
>>>> +    ecc->use_ecc = false;
>>>> +}
>>>> +EXPORT_SYMBOL(qcom_ecc_disable);
>>>> +
>>>> +static const struct of_device_id qpic_ecc_dt_match[] = {
>>>> +    {
>>>> +            .compatible = "qcom,ipq9574-ecc",
>>>
>>> Please run scripts/checkpatch.pl and fix reported warnings. Some
>>> warnings can be ignored, but the code here looks like it needs a fix.
>>> Feel free to get in touch if the warning is not clear.
>>>
>>> Checkpatch is preerquisite. Don't send patches which have obvious issues
>>> pointed out by checkpatch. It's a waste of reviewers time.
>>>
>>>> +    },
>>>> +    {},
>>>> +};
>>>> +
>>>> +static int qpic_ecc_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct qpic_ecc *ecc;
>>>> +    u32 max_eccdata_size;
>>>> +
>>>> +    ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
>>>> +    if (!ecc)
>>>> +            return -ENOMEM;
>>>> +
>>>> +    ecc->caps = of_device_get_match_data(dev);
>>>> +
>>>> +    ecc->dev = dev;
>>>> +    platform_set_drvdata(pdev, ecc);
>>>> +    dev_info(dev, "probed\n");
>>>
>>> No, no such messages.
>>>
>>>
>>>
>>> Best regards,
>>> Krzysztof
>>>
> 
> 
>
Miquel Raynal Nov. 3, 2023, 1:46 p.m. UTC | #9 
Hello,

> Based on below feedback [1] and NAK on the device node patch
> got idea of having separate device node for ECC is not acceptable.
> Could you please help to clarify that.

If I may try to compare with the Macronix situation, the ECC engine
was an independent hardware block, with its own mapping and its own
registers, so it was described as an independent node in the DT. The
type of ECC controller (pipelined or external) is described by the
nand-ecc-engine property which either points at the parent node
(pipelined) or an external node (external). The SPI host would itself
point at the external ECC engine node with its own nand-ecc-engine
property (see mtd/mxicy,nand-ecc-engine.yaml in the bindings).

> Since ECC block is inlined with QPIC controller so is the below
> device node acceptable ?
> 
>     bch: qpic_ecc {
>                            compatible = "qcom,ipq9574-ecc";
>                            status = "ok";
>                    };

If it does not has its own mapping and if you access the ECC engine
through the host registers then the controller should be part of the
host node, but I am not sure it really needs to be described
explicitly, most of them are not for historical reasons. Conceptually
there is a problem with subnodes of each of these controllers having
a signification already: SPI devices or NAND chips.

Thanks,
Miquèl
Dmitry Baryshkov Nov. 3, 2023, 1:54 p.m. UTC | #10 
On Fri, 3 Nov 2023 at 15:24, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:
>
>
>
> On 11/3/2023 6:03 PM, Dmitry Baryshkov wrote:
> > On Fri, 3 Nov 2023 at 14:25, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 10/31/2023 10:41 PM, Krzysztof Kozlowski wrote:
> >>> On 31/10/2023 13:03, Md Sadre Alam wrote:
> >>>
> >>> Eh? Empty?
> >>
> >> QPIC controller has the ecc pipelined so will keep the ecc support
> >> inlined in both raw nand and serial nand driver.
> >>
> >> Droping this driver since device node was NAK-ed
> >> https://www.spinics.net/lists/linux-arm-msm/msg177596.html
> >
> > It seems, we have to repeat the same thing again and again:
> >
> > It was not the device node that was NAKed. It was the patch that was
> > NAKed. Please read the emails carefully.
> >
> > And next time please perform dtbs_check, dt_binding_check and
> > checkpatch before sending the patch.
> >
> > It is perfectly fine to ask questions 'like we are getting we are
> > getting this and that issues with the bindings, please advise'. It is
> > not fine to skip that step completely.
>
> Sorry in V1 will run all basic checks.
>
> Based on below feedback [1] and NAK on the device node patch
> got idea of having separate device node for ECC is not acceptable.
> Could you please help to clarify that.
>
> Since ECC block is inlined with QPIC controller so is the below
> device node acceptable ?

No, the node below is not acceptable. And you have already got two
reasons for that. Let me repeat them for you:

- it is "okay" not "ok"
- no underscores in node names.

If you want to have a more meaningful discussion, please provide full
ECC + NAND + SPI DT bindings, only then we can discuss them.

>     bch: qpic_ecc {
>                            compatible = "qcom,ipq9574-ecc";
>                            status = "ok";
>                    };
>
>   [1] https://www.spinics.net/lists/linux-arm-msm/msg177525.html
>
> >
> >>>
> >>>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> >>>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> >>>> ---
> >>>>    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
> >>>>
> >>>
> >>> ...
> >>>
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> +EXPORT_SYMBOL(qcom_ecc_config);
> >>>> +
> >>>> +void qcom_ecc_enable(struct qcom_ecc *ecc)
> >>>> +{
> >>>> +    ecc->use_ecc = true;
> >>>> +}
> >>>> +EXPORT_SYMBOL(qcom_ecc_enable);
> >>>
> >>> Drop this and all other exports. Nothing here explains the need for them.
> >>>
> >>>> +
> >>>> +void qcom_ecc_disable(struct qcom_ecc *ecc)
> >>>> +{
> >>>> +    ecc->use_ecc = false;
> >>>> +}
> >>>> +EXPORT_SYMBOL(qcom_ecc_disable);
> >>>> +
> >>>> +static const struct of_device_id qpic_ecc_dt_match[] = {
> >>>> +    {
> >>>> +            .compatible = "qcom,ipq9574-ecc",
> >>>
> >>> Please run scripts/checkpatch.pl and fix reported warnings. Some
> >>> warnings can be ignored, but the code here looks like it needs a fix.
> >>> Feel free to get in touch if the warning is not clear.
> >>>
> >>> Checkpatch is preerquisite. Don't send patches which have obvious issues
> >>> pointed out by checkpatch. It's a waste of reviewers time.
> >>>
> >>>> +    },
> >>>> +    {},
> >>>> +};
> >>>> +
> >>>> +static int qpic_ecc_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +    struct device *dev = &pdev->dev;
> >>>> +    struct qpic_ecc *ecc;
> >>>> +    u32 max_eccdata_size;
> >>>> +
> >>>> +    ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
> >>>> +    if (!ecc)
> >>>> +            return -ENOMEM;
> >>>> +
> >>>> +    ecc->caps = of_device_get_match_data(dev);
> >>>> +
> >>>> +    ecc->dev = dev;
> >>>> +    platform_set_drvdata(pdev, ecc);
> >>>> +    dev_info(dev, "probed\n");
> >>>
> >>> No, no such messages.
> >>>
> >>>
> >>>
> >>> Best regards,
> >>> Krzysztof
> >>>
> >
> >
> >
Md Sadre Alam Nov. 20, 2023, 6:30 a.m. UTC | #11 
On 11/3/2023 7:16 PM, Miquel Raynal wrote:
> Hello,
> 
>> Based on below feedback [1] and NAK on the device node patch
>> got idea of having separate device node for ECC is not acceptable.
>> Could you please help to clarify that.
> 
> If I may try to compare with the Macronix situation, the ECC engine
> was an independent hardware block, with its own mapping and its own
> registers, so it was described as an independent node in the DT. The
> type of ECC controller (pipelined or external) is described by the
> nand-ecc-engine property which either points at the parent node
> (pipelined) or an external node (external). The SPI host would itself
> point at the external ECC engine node with its own nand-ecc-engine
> property (see mtd/mxicy,nand-ecc-engine.yaml in the bindings).

Sorry for late reply.

Since QPIC controller ECC engine is not a separate HW block. To control ECC
functionality there is only one register 4-bytes long.As you suggested above,
ECC controller described by the property nand-ecc-engine.I have checked
mtd/mxicy,nand-ecc-engine.yaml file and got to know I can use like
nand-ecc-engine = <&qpic_nand>; in dts.Now additional ECC node not needed
in DTS. Will clean up everything in next patch.

> 
>> Since ECC block is inlined with QPIC controller so is the below
>> device node acceptable ?
>>
>>      bch: qpic_ecc {
>>                             compatible = "qcom,ipq9574-ecc";
>>                             status = "ok";
>>                     };
> 
> If it does not has its own mapping and if you access the ECC engine
> through the host registers then the controller should be part of the
> host node, but I am not sure it really needs to be described
> explicitly, most of them are not for historical reasons. Conceptually
> there is a problem with subnodes of each of these controllers having
> a signification already: SPI devices or NAND chips.

New device node for spi nand looks like as below.

&qpic_nand {
          status = "okay";
          pinctrl-0 = <&qspi_nand_pins>;
          pinctrl-names = "default";
          spi_nand: spi_nand@0 {
                  compatible = "spi-nand";
                  reg = <0>;
                  #address-cells = <1>;
                  #size-cells = <1>;
                  nand-ecc-engine = <&qpic_nand>;
                  nand-ecc-strength = <4>;
                  nand-ecc-step-size = <512>;
                  spi-max-frequency = <8000000>;
          };
};

With the above device node I have tested the spi nand device enumeration
its working fine. Will cleanup everything and post in next patch.


> 
> Thanks,
> Miquèl
diff mbox series

Patch

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
+	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 */
+	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) {
+		/* 8 bit ECC defaults to BCH ECC on all platforms */
+		ecc->bch_enabled = true;
+		ecc->ecc_mode = 1;
+
+		if (wide_bus) {
+			ecc->ecc_bytes_hw = 14;
+			ecc->spare_bytes = 0;
+			ecc->bbm_size = 2;
+		} else {
+			ecc->ecc_bytes_hw = 13;
+			ecc->spare_bytes = 2;
+			ecc->bbm_size = 1;
+		}
+	} else {
+		/*
+		 * if the controller supports BCH for 4 bit ECC, the controller
+		 * uses lesser bytes for ECC. If RS is used, the ECC bytes is
+		 * always 10 bytes
+		 */
+		if (ecc->ecc_modes & ECC_BCH_4BIT) {
+			/* BCH */
+			ecc->bch_enabled = true;
+			ecc->ecc_mode = 0;
+			if (wide_bus) {
+				ecc->ecc_bytes_hw = 8;
+				ecc->spare_bytes = 2;
+				ecc->bbm_size = 2;
+			} else {
+				ecc->ecc_bytes_hw = 7;
+				ecc->spare_bytes = 4;
+				ecc->bbm_size = 1;
+			}
+		} else {
+			/* RS */
+			ecc->ecc_bytes_hw = 10;
+			if (wide_bus) {
+				ecc->spare_bytes = 0;
+				ecc->bbm_size = 2;
+			} else {
+				ecc->spare_bytes = 1;
+				ecc->bbm_size = 1;
+			}
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(qcom_ecc_config);
+
+void qcom_ecc_enable(struct qcom_ecc *ecc)
+{
+	ecc->use_ecc = true;
+}
+EXPORT_SYMBOL(qcom_ecc_enable);
+
+void qcom_ecc_disable(struct qcom_ecc *ecc)
+{
+	ecc->use_ecc = false;
+}
+EXPORT_SYMBOL(qcom_ecc_disable);
+
+static const struct of_device_id qpic_ecc_dt_match[] = {
+	{
+		.compatible = "qcom,ipq9574-ecc",
+	},
+	{},
+};
+
+static int qpic_ecc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct qpic_ecc *ecc;
+	u32 max_eccdata_size;
+
+	ecc = devm_kzalloc(dev, sizeof(*ecc), GFP_KERNEL);
+	if (!ecc)
+		return -ENOMEM;
+
+	ecc->caps = of_device_get_match_data(dev);
+
+	ecc->dev = dev;
+	platform_set_drvdata(pdev, ecc);
+	dev_info(dev, "probed\n");
+
+	return 0;
+}
+
+
+MODULE_DEVICE_TABLE(of, qpic_ecc_dt_match);
+
+static struct platform_driver qpic_ecc_driver = {
+	.probe  = qpic_ecc_probe,
+	.driver = {
+		.name  = "qpic-ecc",
+		.of_match_table = qpic_ecc_dt_match,
+	},
+};
+
+module_platform_driver(qpic_ecc_driver);
+
+MODULE_AUTHOR("Md Sadre Alam <quic_mdalam@quicinc.com>");
+MODULE_DESCRIPTION("QPIC Nand ECC Driver");
+MODULE_LICENSE("Dual MIT/GPL");