[RFC PATCH 0/5] Add QPIC SPI NAND driver support

[RFC PATCH 0/5] Add QPIC SPI NAND driver support

[Md Sadre Alam]

Hi Miquel,

This series is RFC for QPIC NAND driver design for
both SPI NAND and RAW NAND.

We have already discuss this design in the below link

https://patchwork.kernel.org/project/linux-arm-msm/patch/1602307902-16761-3-git-send-email-mdalam@codeaurora.org/#25270814

Since QPIC controller support both raw and as wel as
serial nand, In these patch series I am trying to write
these driver as per above discussion.

As per this design we are having new drivrs for:

1) SPI-NAND Driver
2) RAW-NAND Driver
3) QPIC-COMMON-API Driver
4) ECC ENGINE Driver

Could you plese review these RFC patches and let me know
if i am doing as per design and my code are proper so that
i can proceed further.

I have testd SPI NAND enumeration with this new design.

Command supported currently by spi nand driver
1) RESET
2) READ ID
3) GET FEATURE
4) SET FEATURE

Currently READ_PAGE, WRITE_PAGE are dummy API. Will write
this later on after your review.

One more thisng wanted to add here Since for QPIC ECC engine
its not a separte HW IP, and only one register is there to control ECC
enable/disable. So for just for one register writing a separte driver
is fine or not?
In dt I have added like as below 

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

Is this ok ?

Thanks,
Alam.

Md Sadre Alam (5):
  mtd: nand: ecc-qcom: Add support for ECC Engine Driver
  arm64: dts: qcom: ipq9574: Add ecc engine support
  mtd: nand: qpic_common: Add support for qpic common API
  spi: qpic: Add support for qpic spi nand driver
  arm64: dts: qcom: ipq9574: Add support for SPI nand

 arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts |  56 +-
 arch/arm64/boot/dts/qcom/ipq9574.dtsi       |  33 +
 drivers/mtd/nand/Kconfig                    |   7 +
 drivers/mtd/nand/Makefile                   |   1 +
 drivers/mtd/nand/ecc-qcom.c                 | 198 +++++
 drivers/mtd/nand/qpic_common.c              | 840 ++++++++++++++++++++
 drivers/spi/Kconfig                         |   7 +
 drivers/spi/Makefile                        |   1 +
 drivers/spi/spi-qpic-snand.c                | 604 ++++++++++++++
 include/linux/mtd/nand-qpic-common.h        | 641 +++++++++++++++
 10 files changed, 2360 insertions(+), 28 deletions(-)
 create mode 100644 drivers/mtd/nand/ecc-qcom.c
 create mode 100644 drivers/mtd/nand/qpic_common.c
 create mode 100644 drivers/spi/spi-qpic-snand.c
 create mode 100644 include/linux/mtd/nand-qpic-common.h

-- 
2.34.1

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

[Md Sadre Alam]

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

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");
-- 
2.34.1

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

[Miquel Raynal]

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

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

[Md Sadre Alam]

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.

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

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

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

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

[Krzysztof Kozlowski]

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

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

[Md Sadre Alam]

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
> 

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

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

[Md Sadre Alam]

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

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

[Miquel Raynal]

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

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

[Md Sadre Alam]

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

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

[Dmitry Baryshkov]

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



-- 
With best wishes
Dmitry

[RFC PATCH 2/5] arm64: dts: qcom: ipq9574: Add ecc engine support

[Md Sadre Alam]

Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 5f83ee42a719..b44acb1fac74 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -336,6 +336,11 @@ sdhc_1: mmc@7804000 {
 			status = "disabled";
 		};
 
+		bch: qpic_ecc {
+			compatible = "qcom,ipq9574-ecc";
+			status = "ok";
+		}
+
 		blsp_dma: dma-controller@7884000 {
 			compatible = "qcom,bam-v1.7.0";
 			reg = <0x07884000 0x2b000>;
-- 
2.34.1

Re: [RFC PATCH 2/5] arm64: dts: qcom: ipq9574: Add ecc engine support

[Konrad Dybcio]

On 31.10.2023 13:03, Md Sadre Alam wrote:
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> ---
Hello,

you're missing:

- dt-bindings (make dtbs_check is unhappy)
- a commit message
- Co-developed-by for Sricharan


status should read "okay" instead, but in this case it's unnecessary
as you're defining the node and lack of the status property also means
that device is enabled

however

this ECC engine seems to be a part of the NAND controller, so it's
unlikely that the DT maintainers will agree for it to have a separate
node

Konrad
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 5f83ee42a719..b44acb1fac74 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -336,6 +336,11 @@ sdhc_1: mmc@7804000 {
>  			status = "disabled";
>  		};
>  
> +		bch: qpic_ecc {
> +			compatible = "qcom,ipq9574-ecc";
> +			status = "ok";
> +		}
> +
>  		blsp_dma: dma-controller@7884000 {
>  			compatible = "qcom,bam-v1.7.0";
>  			reg = <0x07884000 0x2b000>;

Re: [RFC PATCH 2/5] arm64: dts: qcom: ipq9574: Add ecc engine support

[Md Sadre Alam]

On 10/31/2023 8:53 PM, Konrad Dybcio wrote:
> On 31.10.2023 13:03, Md Sadre Alam wrote:
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
>> ---
> Hello,
> 
> you're missing:
> 
> - dt-bindings (make dtbs_check is unhappy)
> - a commit message
> - Co-developed-by for Sricharan

> 
> status should read "okay" instead, but in this case it's unnecessary
> as you're defining the node and lack of the status property also means
> that device is enabled
> 
> however
> 
> this ECC engine seems to be a part of the NAND controller, so it's
> unlikely that the DT maintainers will agree for it to have a separate
> node
> 


Will drop this patch as this was NAK-ed
QPIC controller has the ecc pipelined so will keep the ecc support
inlined in both raw nand and serial nand driver.

Regards
Alam.

Re: [RFC PATCH 2/5] arm64: dts: qcom: ipq9574: Add ecc engine support

[Krzysztof Kozlowski]

On 31/10/2023 13:03, Md Sadre Alam wrote:
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 5f83ee42a719..b44acb1fac74 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -336,6 +336,11 @@ sdhc_1: mmc@7804000 {
>  			status = "disabled";
>  		};
>  
> +		bch: qpic_ecc {
> +			compatible = "qcom,ipq9574-ecc";

NAK. There are so many wrong things with it... Let's start with missing
checkpatch. Then with unndeeded label. Then with not allowed underscores
in node names.
Finally:
> +			status = "ok";

Drop.



Best regards,
Krzysztof

Re: [RFC PATCH 2/5] arm64: dts: qcom: ipq9574: Add ecc engine support

[Md Sadre Alam]

On 10/31/2023 10:42 PM, Krzysztof Kozlowski wrote:
> On 31/10/2023 13:03, Md Sadre Alam wrote:
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> index 5f83ee42a719..b44acb1fac74 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -336,6 +336,11 @@ sdhc_1: mmc@7804000 {
>>   			status = "disabled";
>>   		};
>>   
>> +		bch: qpic_ecc {
>> +			compatible = "qcom,ipq9574-ecc";
> 
> NAK. There are so many wrong things with it... Let's start with missing
> checkpatch. Then with unndeeded label. Then with not allowed underscores
> in node names.
> Finally:
>> +			status = "ok";
> 
> Drop.
> 
Ok
> 
> 
> Best regards,
> Krzysztof
> 

[RFC PATCH 3/5] mtd: nand: qpic_common: Add support for qpic common API

[Md Sadre Alam]

Add qpic command API in sperate file so that it will be
use by both spi and request and raw nand request.

Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
---
 drivers/mtd/nand/qpic_common.c       | 840 +++++++++++++++++++++++++++
 include/linux/mtd/nand-qpic-common.h | 641 ++++++++++++++++++++
 2 files changed, 1481 insertions(+)
 create mode 100644 drivers/mtd/nand/qpic_common.c
 create mode 100644 include/linux/mtd/nand-qpic-common.h

diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c
new file mode 100644
index 000000000000..983768a1ea94
--- /dev/null
+++ b/drivers/mtd/nand/qpic_common.c
@@ -0,0 +1,840 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * QPIC common API file.
+ * Copyright (C) 2023  Qualcomm Inc.
+ * Authors:	Md sadre Alam		<quic_mdalam@quicinc.com>
+ *		Sricharan R		<quic_srichara@quicinc.com>
+ */
+
+#include <linux/mtd/nand-qpic-common.h>
+
+/* Frees the BAM transaction memory */
+void free_bam_transaction(struct qcom_nand_controller *nandc)
+{
+	struct bam_transaction *bam_txn = nandc->bam_txn;
+
+	devm_kfree(nandc->dev, bam_txn);
+}
+EXPORT_SYMBOL(free_bam_transaction);
+
+/* Callback for DMA descriptor completion */
+void qpic_bam_dma_done(void *data)
+{
+	struct bam_transaction *bam_txn = data;
+
+	/*
+	 * In case of data transfer with NAND, 2 callbacks will be generated.
+	 * One for command channel and another one for data channel.
+	 * If current transaction has data descriptors
+	 * (i.e. wait_second_completion is true), then set this to false
+	 * and wait for second DMA descriptor completion.
+	 */
+	if (bam_txn->wait_second_completion)
+		bam_txn->wait_second_completion = false;
+	else
+		complete(&bam_txn->txn_done);
+}
+EXPORT_SYMBOL(qpic_bam_dma_done);
+
+u32 nandc_read(struct qcom_nand_controller *nandc, int offset)
+{
+	return ioread32(nandc->base + offset);
+}
+EXPORT_SYMBOL(nandc_read);
+
+void nandc_write(struct qcom_nand_controller *nandc, int offset,
+			       u32 val)
+{
+	iowrite32(val, nandc->base + offset);
+}
+EXPORT_SYMBOL(nandc_write);
+
+void nandc_read_buffer_sync(struct qcom_nand_controller *nandc,
+					  bool is_cpu)
+{
+	if (!nandc->props->is_bam)
+		return;
+
+	if (is_cpu)
+		dma_sync_single_for_cpu(nandc->dev, nandc->reg_read_dma,
+					MAX_REG_RD *
+					sizeof(*nandc->reg_read_buf),
+					DMA_FROM_DEVICE);
+	else
+		dma_sync_single_for_device(nandc->dev, nandc->reg_read_dma,
+					   MAX_REG_RD *
+					   sizeof(*nandc->reg_read_buf),
+					   DMA_FROM_DEVICE);
+}
+EXPORT_SYMBOL(nandc_read_buffer_sync);
+
+__le32 *offset_to_nandc_reg(struct nandc_regs *regs, int offset)
+{
+	switch (offset) {
+	case NAND_FLASH_CMD:
+		return &regs->cmd;
+	case NAND_ADDR0:
+		return &regs->addr0;
+	case NAND_ADDR1:
+		return &regs->addr1;
+	case NAND_FLASH_CHIP_SELECT:
+		return &regs->chip_sel;
+	case NAND_EXEC_CMD:
+		return &regs->exec;
+	case NAND_FLASH_STATUS:
+		return &regs->clrflashstatus;
+	case NAND_DEV0_CFG0:
+		return &regs->cfg0;
+	case NAND_DEV0_CFG1:
+		return &regs->cfg1;
+	case NAND_DEV0_ECC_CFG:
+		return &regs->ecc_bch_cfg;
+	case NAND_READ_STATUS:
+		return &regs->clrreadstatus;
+	case NAND_DEV_CMD1:
+		return &regs->cmd1;
+	case NAND_DEV_CMD1_RESTORE:
+		return &regs->orig_cmd1;
+	case NAND_DEV_CMD_VLD:
+		return &regs->vld;
+	case NAND_DEV_CMD_VLD_RESTORE:
+		return &regs->orig_vld;
+	case NAND_EBI2_ECC_BUF_CFG:
+		return &regs->ecc_buf_cfg;
+	case NAND_READ_LOCATION_0:
+		return &regs->read_location0;
+	case NAND_READ_LOCATION_1:
+		return &regs->read_location1;
+	case NAND_READ_LOCATION_2:
+		return &regs->read_location2;
+	case NAND_READ_LOCATION_3:
+		return &regs->read_location3;
+	case NAND_READ_LOCATION_LAST_CW_0:
+		return &regs->read_location_last0;
+	case NAND_READ_LOCATION_LAST_CW_1:
+		return &regs->read_location_last1;
+	case NAND_READ_LOCATION_LAST_CW_2:
+		return &regs->read_location_last2;
+	case NAND_READ_LOCATION_LAST_CW_3:
+		return &regs->read_location_last3;
+	case NAND_FLASH_SPI_CFG:
+		return &regs->spi_cfg;
+	case NAND_NUM_ADDR_CYCLES:
+		return &regs->num_addr_cycle;
+	case NAND_BUSY_CHECK_WAIT_CNT:
+		return &regs->busy_wait_cnt;
+	case NAND_MSTR_CONFIG:
+		return &regs->mstr_cfg;
+	case NAND_FLASH_FEATURES:
+		return &regs->flash_feature;
+	default:
+		return NULL;
+	}
+}
+EXPORT_SYMBOL(offset_to_nandc_reg);
+
+/* reset the register read buffer for next NAND operation */
+void clear_read_regs(struct qcom_nand_controller *nandc)
+{
+	nandc->reg_read_pos = 0;
+	nandc_read_buffer_sync(nandc, false);
+}
+EXPORT_SYMBOL(clear_read_regs);
+
+int prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read,
+			     int reg_off, const void *vaddr, int size,
+			     bool flow_control)
+{
+	struct desc_info *desc;
+	struct dma_async_tx_descriptor *dma_desc;
+	struct scatterlist *sgl;
+	struct dma_slave_config slave_conf;
+	struct qcom_adm_peripheral_config periph_conf = {};
+	enum dma_transfer_direction dir_eng;
+	int ret;
+
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	sgl = &desc->adm_sgl;
+
+	sg_init_one(sgl, vaddr, size);
+
+	if (read) {
+		dir_eng = DMA_DEV_TO_MEM;
+		desc->dir = DMA_FROM_DEVICE;
+	} else {
+		dir_eng = DMA_MEM_TO_DEV;
+		desc->dir = DMA_TO_DEVICE;
+	}
+
+	ret = dma_map_sg(nandc->dev, sgl, 1, desc->dir);
+	if (ret == 0) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	memset(&slave_conf, 0x00, sizeof(slave_conf));
+
+	slave_conf.device_fc = flow_control;
+	if (read) {
+		slave_conf.src_maxburst = 16;
+		slave_conf.src_addr = nandc->base_dma + reg_off;
+		if (nandc->data_crci) {
+			periph_conf.crci = nandc->data_crci;
+			slave_conf.peripheral_config = &periph_conf;
+			slave_conf.peripheral_size = sizeof(periph_conf);
+		}
+	} else {
+		slave_conf.dst_maxburst = 16;
+		slave_conf.dst_addr = nandc->base_dma + reg_off;
+		if (nandc->cmd_crci) {
+			periph_conf.crci = nandc->cmd_crci;
+			slave_conf.peripheral_config = &periph_conf;
+			slave_conf.peripheral_size = sizeof(periph_conf);
+		}
+	}
+
+	ret = dmaengine_slave_config(nandc->chan, &slave_conf);
+	if (ret) {
+		dev_err(nandc->dev, "failed to configure dma channel\n");
+		goto err;
+	}
+
+	dma_desc = dmaengine_prep_slave_sg(nandc->chan, sgl, 1, dir_eng, 0);
+	if (!dma_desc) {
+		dev_err(nandc->dev, "failed to prepare desc\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	desc->dma_desc = dma_desc;
+
+	list_add_tail(&desc->node, &nandc->desc_list);
+
+	return 0;
+err:
+	kfree(desc);
+
+	return ret;
+}
+
+/* helpers to submit/free our list of dma descriptors */
+int submit_descs(struct qcom_nand_controller *nandc)
+{
+	struct desc_info *desc;
+	dma_cookie_t cookie = 0;
+	struct bam_transaction *bam_txn = nandc->bam_txn;
+	int r;
+
+	if (nandc->props->is_bam) {
+		if (bam_txn->rx_sgl_pos > bam_txn->rx_sgl_start) {
+			r = prepare_bam_async_desc(nandc, nandc->rx_chan, 0);
+			if (r)
+				return r;
+		}
+
+		if (bam_txn->tx_sgl_pos > bam_txn->tx_sgl_start) {
+			r = prepare_bam_async_desc(nandc, nandc->tx_chan,
+						   DMA_PREP_INTERRUPT);
+			if (r)
+				return r;
+		}
+
+		if (bam_txn->cmd_sgl_pos > bam_txn->cmd_sgl_start) {
+			r = prepare_bam_async_desc(nandc, nandc->cmd_chan,
+						   DMA_PREP_CMD);
+			if (r)
+				return r;
+		}
+	}
+
+	list_for_each_entry(desc, &nandc->desc_list, node)
+		cookie = dmaengine_submit(desc->dma_desc);
+
+	if (nandc->props->is_bam) {
+		bam_txn->last_cmd_desc->callback = qpic_bam_dma_done;
+		bam_txn->last_cmd_desc->callback_param = bam_txn;
+		if (bam_txn->last_data_desc) {
+			bam_txn->last_data_desc->callback = qpic_bam_dma_done;
+			bam_txn->last_data_desc->callback_param = bam_txn;
+			bam_txn->wait_second_completion = true;
+		}
+
+		dma_async_issue_pending(nandc->tx_chan);
+		dma_async_issue_pending(nandc->rx_chan);
+		dma_async_issue_pending(nandc->cmd_chan);
+
+		if (!wait_for_completion_timeout(&bam_txn->txn_done,
+						 QPIC_NAND_COMPLETION_TIMEOUT))
+			return -ETIMEDOUT;
+	} else {
+		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE)
+			return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(submit_descs);
+
+void free_descs(struct qcom_nand_controller *nandc)
+{
+	struct desc_info *desc, *n;
+
+	list_for_each_entry_safe(desc, n, &nandc->desc_list, node) {
+		list_del(&desc->node);
+
+		if (nandc->props->is_bam)
+			dma_unmap_sg(nandc->dev, desc->bam_sgl,
+				     desc->sgl_cnt, desc->dir);
+		else
+			dma_unmap_sg(nandc->dev, &desc->adm_sgl, 1,
+				     desc->dir);
+
+		kfree(desc);
+	}
+}
+EXPORT_SYMBOL(free_descs);
+
+/*
+ * Maps the scatter gather list for DMA transfer and forms the DMA descriptor
+ * for BAM. This descriptor will be added in the NAND DMA descriptor queue
+ * which will be submitted to DMA engine.
+ */
+int prepare_bam_async_desc(struct qcom_nand_controller *nandc,
+				  struct dma_chan *chan,
+				  unsigned long flags)
+{
+	struct desc_info *desc;
+	struct scatterlist *sgl;
+	unsigned int sgl_cnt;
+	int ret;
+	struct bam_transaction *bam_txn = nandc->bam_txn;
+	enum dma_transfer_direction dir_eng;
+	struct dma_async_tx_descriptor *dma_desc;
+
+	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	if (chan == nandc->cmd_chan) {
+		sgl = &bam_txn->cmd_sgl[bam_txn->cmd_sgl_start];
+		sgl_cnt = bam_txn->cmd_sgl_pos - bam_txn->cmd_sgl_start;
+		bam_txn->cmd_sgl_start = bam_txn->cmd_sgl_pos;
+		dir_eng = DMA_MEM_TO_DEV;
+		desc->dir = DMA_TO_DEVICE;
+	} else if (chan == nandc->tx_chan) {
+		sgl = &bam_txn->data_sgl[bam_txn->tx_sgl_start];
+		sgl_cnt = bam_txn->tx_sgl_pos - bam_txn->tx_sgl_start;
+		bam_txn->tx_sgl_start = bam_txn->tx_sgl_pos;
+		dir_eng = DMA_MEM_TO_DEV;
+		desc->dir = DMA_TO_DEVICE;
+	} else {
+		sgl = &bam_txn->data_sgl[bam_txn->rx_sgl_start];
+		sgl_cnt = bam_txn->rx_sgl_pos - bam_txn->rx_sgl_start;
+		bam_txn->rx_sgl_start = bam_txn->rx_sgl_pos;
+		dir_eng = DMA_DEV_TO_MEM;
+		desc->dir = DMA_FROM_DEVICE;
+	}
+
+	sg_mark_end(sgl + sgl_cnt - 1);
+	ret = dma_map_sg(nandc->dev, sgl, sgl_cnt, desc->dir);
+	if (ret == 0) {
+		dev_err(nandc->dev, "failure in mapping desc\n");
+		kfree(desc);
+		return -ENOMEM;
+	}
+
+	desc->sgl_cnt = sgl_cnt;
+	desc->bam_sgl = sgl;
+
+	dma_desc = dmaengine_prep_slave_sg(chan, sgl, sgl_cnt, dir_eng,
+					   flags);
+
+	if (!dma_desc) {
+		dev_err(nandc->dev, "failure in prep desc\n");
+		dma_unmap_sg(nandc->dev, sgl, sgl_cnt, desc->dir);
+		kfree(desc);
+		return -EINVAL;
+	}
+
+	desc->dma_desc = dma_desc;
+
+	/* update last data/command descriptor */
+	if (chan == nandc->cmd_chan)
+		bam_txn->last_cmd_desc = dma_desc;
+	else
+		bam_txn->last_data_desc = dma_desc;
+
+	list_add_tail(&desc->node, &nandc->desc_list);
+
+	return 0;
+}
+EXPORT_SYMBOL(prepare_bam_async_desc);
+
+/*
+ * Prepares the command descriptor for BAM DMA which will be used for NAND
+ * register reads and writes. The command descriptor requires the command
+ * to be formed in command element type so this function uses the command
+ * element from bam transaction ce array and fills the same with required
+ * data. A single SGL can contain multiple command elements so
+ * NAND_BAM_NEXT_SGL will be used for starting the separate SGL
+ * after the current command element.
+ */
+int prep_bam_dma_desc_cmd(struct qcom_nand_controller *nandc, bool read,
+				 int reg_off, const void *vaddr,
+				 int size, unsigned int flags)
+{
+	int bam_ce_size;
+	int i, ret;
+	struct bam_cmd_element *bam_ce_buffer;
+	struct bam_transaction *bam_txn = nandc->bam_txn;
+
+	bam_ce_buffer = &bam_txn->bam_ce[bam_txn->bam_ce_pos];
+
+	/* fill the command desc */
+	for (i = 0; i < size; i++) {
+		if (read)
+			bam_prep_ce(&bam_ce_buffer[i],
+				    nandc_reg_phys(nandc, reg_off + 4 * i),
+				    BAM_READ_COMMAND,
+				    reg_buf_dma_addr(nandc,
+						     (__le32 *)vaddr + i));
+		else
+			bam_prep_ce_le32(&bam_ce_buffer[i],
+					 nandc_reg_phys(nandc, reg_off + 4 * i),
+					 BAM_WRITE_COMMAND,
+					 *((__le32 *)vaddr + i));
+	}
+
+	bam_txn->bam_ce_pos += size;
+
+	/* use the separate sgl after this command */
+	if (flags & NAND_BAM_NEXT_SGL) {
+		bam_ce_buffer = &bam_txn->bam_ce[bam_txn->bam_ce_start];
+		bam_ce_size = (bam_txn->bam_ce_pos -
+				bam_txn->bam_ce_start) *
+				sizeof(struct bam_cmd_element);
+		sg_set_buf(&bam_txn->cmd_sgl[bam_txn->cmd_sgl_pos],
+			   bam_ce_buffer, bam_ce_size);
+		bam_txn->cmd_sgl_pos++;
+		bam_txn->bam_ce_start = bam_txn->bam_ce_pos;
+
+		if (flags & NAND_BAM_NWD) {
+			ret = prepare_bam_async_desc(nandc, nandc->cmd_chan,
+						     DMA_PREP_FENCE |
+						     DMA_PREP_CMD);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(prep_bam_dma_desc_cmd);
+
+/*
+ * Prepares the data descriptor for BAM DMA which will be used for NAND
+ * data reads and writes.
+ */
+int prep_bam_dma_desc_data(struct qcom_nand_controller *nandc, bool read,
+				  const void *vaddr,
+				  int size, unsigned int flags)
+{
+	int ret;
+	struct bam_transaction *bam_txn = nandc->bam_txn;
+
+	if (read) {
+		sg_set_buf(&bam_txn->data_sgl[bam_txn->rx_sgl_pos],
+			   vaddr, size);
+		bam_txn->rx_sgl_pos++;
+	} else {
+		sg_set_buf(&bam_txn->data_sgl[bam_txn->tx_sgl_pos],
+			   vaddr, size);
+		bam_txn->tx_sgl_pos++;
+
+		/*
+		 * BAM will only set EOT for DMA_PREP_INTERRUPT so if this flag
+		 * is not set, form the DMA descriptor
+		 */
+		if (!(flags & NAND_BAM_NO_EOT)) {
+			ret = prepare_bam_async_desc(nandc, nandc->tx_chan,
+						     DMA_PREP_INTERRUPT);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(prep_bam_dma_desc_data);
+
+/*
+ * read_reg_dma:	prepares a descriptor to read a given number of
+ *			contiguous registers to the reg_read_buf pointer
+ *
+ * @first:		offset of the first register in the contiguous block
+ * @num_regs:		number of registers to read
+ * @flags:		flags to control DMA descriptor preparation
+ */
+int read_reg_dma(struct qcom_nand_controller *nandc, int first,
+			int num_regs, unsigned int flags)
+{
+	bool flow_control = false;
+	void *vaddr;
+
+	vaddr = nandc->reg_read_buf + nandc->reg_read_pos;
+	nandc->reg_read_pos += num_regs;
+
+	if (first == NAND_DEV_CMD_VLD || first == NAND_DEV_CMD1)
+		first = dev_cmd_reg_addr(nandc, first);
+
+	if (nandc->props->is_bam)
+		return prep_bam_dma_desc_cmd(nandc, true, first, vaddr,
+					     num_regs, flags);
+
+	if (first == NAND_READ_ID || first == NAND_FLASH_STATUS)
+		flow_control = true;
+
+	return prep_adm_dma_desc(nandc, true, first, vaddr,
+				 num_regs * sizeof(u32), flow_control);
+}
+EXPORT_SYMBOL(read_reg_dma);
+
+/*
+ * write_reg_dma:	prepares a descriptor to write a given number of
+ *			contiguous registers
+ *
+ * @first:		offset of the first register in the contiguous block
+ * @num_regs:		number of registers to write
+ * @flags:		flags to control DMA descriptor preparation
+ */
+int write_reg_dma(struct qcom_nand_controller *nandc, int first,
+			 int num_regs, unsigned int flags)
+{
+	bool flow_control = false;
+	struct nandc_regs *regs = nandc->regs;
+	void *vaddr;
+
+	vaddr = offset_to_nandc_reg(regs, first);
+
+	if (first == NAND_ERASED_CW_DETECT_CFG) {
+		if (flags & NAND_ERASED_CW_SET)
+			vaddr = &regs->erased_cw_detect_cfg_set;
+		else
+			vaddr = &regs->erased_cw_detect_cfg_clr;
+	}
+
+	if (first == NAND_EXEC_CMD)
+		flags |= NAND_BAM_NWD;
+
+	if (first == NAND_FLASH_SPI_CFG || first == NAND_NUM_ADDR_CYCLES
+		|| first == NAND_BUSY_CHECK_WAIT_CNT
+		|| first == NAND_MSTR_CONFIG)
+		first = dev_cmd_reg_addr(nandc, first);
+
+	if (first == NAND_DEV_CMD1_RESTORE || first == NAND_DEV_CMD1)
+		first = dev_cmd_reg_addr(nandc, NAND_DEV_CMD1);
+
+	if (first == NAND_DEV_CMD_VLD_RESTORE || first == NAND_DEV_CMD_VLD)
+		first = dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD);
+
+	if (nandc->props->is_bam)
+		return prep_bam_dma_desc_cmd(nandc, false, first, vaddr,
+					     num_regs, flags);
+
+	if (first == NAND_FLASH_CMD)
+		flow_control = true;
+
+	return prep_adm_dma_desc(nandc, false, first, vaddr,
+				 num_regs * sizeof(u32), flow_control);
+}
+EXPORT_SYMBOL(write_reg_dma);
+
+/*
+ * read_data_dma:	prepares a DMA descriptor to transfer data from the
+ *			controller's internal buffer to the buffer 'vaddr'
+ *
+ * @reg_off:		offset within the controller's data buffer
+ * @vaddr:		virtual address of the buffer we want to write to
+ * @size:		DMA transaction size in bytes
+ * @flags:		flags to control DMA descriptor preparation
+ */
+int read_data_dma(struct qcom_nand_controller *nandc, int reg_off,
+			 const u8 *vaddr, int size, unsigned int flags)
+{
+	if (nandc->props->is_bam)
+		return prep_bam_dma_desc_data(nandc, true, vaddr, size, flags);
+
+	return prep_adm_dma_desc(nandc, true, reg_off, vaddr, size, false);
+}
+EXPORT_SYMBOL(read_data_dma);
+
+/*
+ * write_data_dma:	prepares a DMA descriptor to transfer data from
+ *			'vaddr' to the controller's internal buffer
+ *
+ * @reg_off:		offset within the controller's data buffer
+ * @vaddr:		virtual address of the buffer we want to read from
+ * @size:		DMA transaction size in bytes
+ * @flags:		flags to control DMA descriptor preparation
+ */
+int write_data_dma(struct qcom_nand_controller *nandc, int reg_off,
+			  const u8 *vaddr, int size, unsigned int flags)
+{
+	if (nandc->props->is_bam)
+		return prep_bam_dma_desc_data(nandc, false, vaddr, size, flags);
+
+	return prep_adm_dma_desc(nandc, false, reg_off, vaddr, size, false);
+}
+EXPORT_SYMBOL(write_data_dma);
+
+void nandc_set_reg(struct qcom_nand_controller *nandc, int offset,
+			  u32 val)
+{
+	struct nandc_regs *regs = nandc->regs;
+	__le32 *reg;
+
+	reg = offset_to_nandc_reg(regs, offset);
+	if (reg)
+		*reg = cpu_to_le32(val);
+}
+EXPORT_SYMBOL(nandc_set_reg);
+
+/* Allocates and Initializes the BAM transaction */
+struct bam_transaction *
+alloc_bam_transaction(struct qcom_nand_controller *nandc)
+{
+	struct bam_transaction *bam_txn;
+	size_t bam_txn_size;
+	unsigned int num_cw = nandc->max_cwperpage;
+	void *bam_txn_buf;
+
+	bam_txn_size =
+		sizeof(*bam_txn) + num_cw *
+		((sizeof(*bam_txn->bam_ce) * QPIC_PER_CW_CMD_ELEMENTS) +
+		(sizeof(*bam_txn->cmd_sgl) * QPIC_PER_CW_CMD_SGL) +
+		(sizeof(*bam_txn->data_sgl) * QPIC_PER_CW_DATA_SGL));
+
+	bam_txn_buf = devm_kzalloc(nandc->dev, bam_txn_size, GFP_KERNEL);
+	if (!bam_txn_buf)
+		return NULL;
+
+	bam_txn = bam_txn_buf;
+	bam_txn_buf += sizeof(*bam_txn);
+
+	bam_txn->bam_ce = bam_txn_buf;
+	bam_txn_buf +=
+		sizeof(*bam_txn->bam_ce) * QPIC_PER_CW_CMD_ELEMENTS * num_cw;
+
+	bam_txn->cmd_sgl = bam_txn_buf;
+	bam_txn_buf +=
+		sizeof(*bam_txn->cmd_sgl) * QPIC_PER_CW_CMD_SGL * num_cw;
+
+	bam_txn->data_sgl = bam_txn_buf;
+
+	init_completion(&bam_txn->txn_done);
+
+	return bam_txn;
+}
+EXPORT_SYMBOL(alloc_bam_transaction);
+
+/* Clears the BAM transaction indexes */
+void clear_bam_transaction(struct qcom_nand_controller *nandc)
+{
+	struct bam_transaction *bam_txn = nandc->bam_txn;
+
+	if (!nandc->props->is_bam)
+		return;
+
+	bam_txn->bam_ce_pos = 0;
+	bam_txn->bam_ce_start = 0;
+	bam_txn->cmd_sgl_pos = 0;
+	bam_txn->cmd_sgl_start = 0;
+	bam_txn->tx_sgl_pos = 0;
+	bam_txn->tx_sgl_start = 0;
+	bam_txn->rx_sgl_pos = 0;
+	bam_txn->rx_sgl_start = 0;
+	bam_txn->last_data_desc = NULL;
+	bam_txn->wait_second_completion = false;
+
+	sg_init_table(bam_txn->cmd_sgl, nandc->max_cwperpage *
+		      QPIC_PER_CW_CMD_SGL);
+	sg_init_table(bam_txn->data_sgl, nandc->max_cwperpage *
+		      QPIC_PER_CW_DATA_SGL);
+
+	reinit_completion(&bam_txn->txn_done);
+}
+EXPORT_SYMBOL(clear_bam_transaction);
+
+/* one time setup of a few nand controller registers */
+int qcom_nandc_setup(struct qcom_nand_controller *nandc)
+{
+	u32 nand_ctrl;
+
+	/* kill onenand */
+	if (!nandc->props->is_qpic)
+		nandc_write(nandc, SFLASHC_BURST_CFG, 0);
+
+	if (!nandc->props->qpic_v2)
+		nandc_write(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD),
+			    NAND_DEV_CMD_VLD_VAL);
+
+	/* enable ADM or BAM DMA */
+	if (nandc->props->is_bam) {
+		nand_ctrl = nandc_read(nandc, NAND_CTRL);
+
+		/*
+		 *NAND_CTRL is an operational registers, and CPU
+		 * access to operational registers are read only
+		 * in BAM mode. So update the NAND_CTRL register
+		 * only if it is not in BAM mode. In most cases BAM
+		 * mode will be enabled in bootloader
+		 */
+		if (!(nand_ctrl & BAM_MODE_EN))
+			nandc_write(nandc, NAND_CTRL, nand_ctrl | BAM_MODE_EN);
+	} else {
+		nandc_write(nandc, NAND_FLASH_CHIP_SELECT, DM_EN);
+	}
+
+	/* save the original values of these registers */
+	if (!nandc->props->qpic_v2) {
+		nandc->cmd1 = nandc_read(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD1));
+		nandc->vld = NAND_DEV_CMD_VLD_VAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(qcom_nandc_setup);
+
+void qcom_nandc_unalloc(struct qcom_nand_controller *nandc)
+{
+	if (nandc->props->is_bam) {
+		if (!dma_mapping_error(nandc->dev, nandc->reg_read_dma))
+			dma_unmap_single(nandc->dev, nandc->reg_read_dma,
+					 MAX_REG_RD *
+					 sizeof(*nandc->reg_read_buf),
+					 DMA_FROM_DEVICE);
+
+		if (nandc->tx_chan)
+			dma_release_channel(nandc->tx_chan);
+
+		if (nandc->rx_chan)
+			dma_release_channel(nandc->rx_chan);
+
+		if (nandc->cmd_chan)
+			dma_release_channel(nandc->cmd_chan);
+	} else {
+		if (nandc->chan)
+			dma_release_channel(nandc->chan);
+	}
+}
+EXPORT_SYMBOL(qcom_nandc_unalloc);
+
+int qcom_nandc_alloc(struct qcom_nand_controller *nandc)
+{
+	int ret;
+
+	ret = dma_set_coherent_mask(nandc->dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_err(nandc->dev, "failed to set DMA mask\n");
+		return ret;
+	}
+
+	/*
+	 * we use the internal buffer for reading ONFI params, reading small
+	 * data like ID and status, and preforming read-copy-write operations
+	 * when writing to a codeword partially. 532 is the maximum possible
+	 * size of a codeword for our nand controller
+	 */
+	nandc->buf_size = 532;
+
+	nandc->data_buffer = devm_kzalloc(nandc->dev, nandc->buf_size,
+					GFP_KERNEL);
+	if (!nandc->data_buffer)
+		return -ENOMEM;
+
+	nandc->regs = devm_kzalloc(nandc->dev, sizeof(*nandc->regs),
+					GFP_KERNEL);
+	if (!nandc->regs)
+		return -ENOMEM;
+
+	nandc->reg_read_buf = devm_kcalloc(nandc->dev,
+				MAX_REG_RD, sizeof(*nandc->reg_read_buf),
+				GFP_KERNEL);
+	if (!nandc->reg_read_buf)
+		return -ENOMEM;
+
+	if (nandc->props->is_bam) {
+		nandc->reg_read_dma =
+			dma_map_single(nandc->dev, nandc->reg_read_buf,
+				       MAX_REG_RD *
+				       sizeof(*nandc->reg_read_buf),
+				       DMA_FROM_DEVICE);
+		if (dma_mapping_error(nandc->dev, nandc->reg_read_dma)) {
+			dev_err(nandc->dev, "failed to DMA MAP reg buffer\n");
+			return -EIO;
+		}
+
+		nandc->tx_chan = dma_request_chan(nandc->dev, "tx");
+		if (IS_ERR(nandc->tx_chan)) {
+			ret = PTR_ERR(nandc->tx_chan);
+			nandc->tx_chan = NULL;
+			dev_err_probe(nandc->dev, ret,
+				      "tx DMA channel request failed\n");
+			goto unalloc;
+		}
+
+		nandc->rx_chan = dma_request_chan(nandc->dev, "rx");
+		if (IS_ERR(nandc->rx_chan)) {
+			ret = PTR_ERR(nandc->rx_chan);
+			nandc->rx_chan = NULL;
+			dev_err_probe(nandc->dev, ret,
+				      "rx DMA channel request failed\n");
+			goto unalloc;
+		}
+
+		nandc->cmd_chan = dma_request_chan(nandc->dev, "cmd");
+		if (IS_ERR(nandc->cmd_chan)) {
+			ret = PTR_ERR(nandc->cmd_chan);
+			nandc->cmd_chan = NULL;
+			dev_err_probe(nandc->dev, ret,
+				      "cmd DMA channel request failed\n");
+			goto unalloc;
+		}
+
+		/*
+		 * Initially allocate BAM transaction to read ONFI param page.
+		 * After detecting all the devices, this BAM transaction will
+		 * be freed and the next BAM tranasction will be allocated with
+		 * maximum codeword size
+		 */
+		nandc->max_cwperpage = 1;
+		nandc->bam_txn = alloc_bam_transaction(nandc);
+		if (!nandc->bam_txn) {
+			dev_err(nandc->dev,
+				"failed to allocate bam transaction\n");
+			ret = -ENOMEM;
+			goto unalloc;
+		}
+	} else {
+		nandc->chan = dma_request_chan(nandc->dev, "rxtx");
+		if (IS_ERR(nandc->chan)) {
+			ret = PTR_ERR(nandc->chan);
+			nandc->chan = NULL;
+			dev_err_probe(nandc->dev, ret,
+				      "rxtx DMA channel request failed\n");
+			return ret;
+		}
+	}
+
+	INIT_LIST_HEAD(&nandc->desc_list);
+	INIT_LIST_HEAD(&nandc->host_list);
+
+	return 0;
+unalloc:
+	qcom_nandc_unalloc(nandc);
+	return ret;
+}
+EXPORT_SYMBOL(qcom_nandc_alloc);
diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h
new file mode 100644
index 000000000000..c461c1781330
--- /dev/null
+++ b/include/linux/mtd/nand-qpic-common.h
@@ -0,0 +1,641 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * QCOM QPIC common APIs
+ *
+ * Copyright (c) 2023 Qualcomm Inc.
+ * Authors:	Md Sadre Alam		<quic_mdalam@quicinc.com>
+ *		Sricharan R		<quic_srichara@quicinc.com>
+ */
+
+#ifndef __DRIVERS_MTD_NAND_QCOM_ECC_H__
+#define __DRIVERS_MTD_NAND_QCOM_ECC_H__
+#include <linux/clk.h>
+#include <linux/slab.h>
+#include <linux/bitops.h>
+#include <linux/dma/qcom_adm.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/delay.h>
+#include <linux/dma/qcom_bam_dma.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/partitions.h>
+#include <linux/types.h>
+
+/* NANDc reg offsets */
+#define	NAND_FLASH_CMD			0x00
+#define	NAND_ADDR0			0x04
+#define	NAND_ADDR1			0x08
+#define	NAND_FLASH_CHIP_SELECT		0x0c
+#define	NAND_EXEC_CMD			0x10
+#define	NAND_FLASH_STATUS		0x14
+#define	NAND_BUFFER_STATUS		0x18
+#define	NAND_DEV0_CFG0			0x20
+#define	NAND_DEV0_CFG1			0x24
+#define	NAND_DEV0_ECC_CFG		0x28
+#define	NAND_AUTO_STATUS_EN		0x2c
+#define	NAND_DEV1_CFG0			0x30
+#define	NAND_DEV1_CFG1			0x34
+#define	NAND_READ_ID			0x40
+#define	NAND_READ_STATUS		0x44
+#define	NAND_DEV_CMD0			0xa0
+#define	NAND_DEV_CMD1			0xa4
+#define	NAND_DEV_CMD2			0xa8
+#define	NAND_DEV_CMD_VLD		0xac
+#define NAND_FLASH_SPI_CFG              0xc0
+#define NAND_NUM_ADDR_CYCLES        	0xc4
+#define NAND_BUSY_CHECK_WAIT_CNT    	0xc8
+#define	SFLASHC_BURST_CFG		0xe0
+#define	NAND_ERASED_CW_DETECT_CFG	0xe8
+#define	NAND_ERASED_CW_DETECT_STATUS	0xec
+#define	NAND_EBI2_ECC_BUF_CFG		0xf0
+#define	FLASH_BUF_ACC			0x100
+
+#define	NAND_CTRL			0xf00
+#define	NAND_VERSION			0xf08
+#define	NAND_READ_LOCATION_0		0xf20
+#define	NAND_READ_LOCATION_1		0xf24
+#define	NAND_READ_LOCATION_2		0xf28
+#define	NAND_READ_LOCATION_3		0xf2c
+#define	NAND_READ_LOCATION_LAST_CW_0	0xf40
+#define	NAND_READ_LOCATION_LAST_CW_1	0xf44
+#define	NAND_READ_LOCATION_LAST_CW_2	0xf48
+#define	NAND_READ_LOCATION_LAST_CW_3	0xf4c
+#define NAND_MSTR_CONFIG           	0xf60
+#define NAND_FLASH_FEATURES             0xf64
+
+/*
+ * the NAND controller performs reads/writes with ECC in 516 byte chunks.
+ * the driver calls the chunks 'step' or 'codeword' interchangeably
+ */
+#define	NANDC_STEP_SIZE			512
+
+/* dummy register offsets, used by write_reg_dma */
+#define	NAND_DEV_CMD1_RESTORE		0xdead
+#define	NAND_DEV_CMD_VLD_RESTORE	0xbeef
+
+/* NAND_FLASH_CMD bits */
+#define	PAGE_ACC			BIT(4)
+#define	LAST_PAGE			BIT(5)
+
+/* NAND_FLASH_CHIP_SELECT bits */
+#define	NAND_DEV_SEL			0
+#define	DM_EN				BIT(2)
+
+/* NAND_FLASH_STATUS bits */
+#define	FS_OP_ERR			BIT(4)
+#define	FS_READY_BSY_N			BIT(5)
+#define	FS_MPU_ERR			BIT(8)
+#define	FS_DEVICE_STS_ERR		BIT(16)
+#define	FS_DEVICE_WP			BIT(23)
+
+/* NAND_BUFFER_STATUS bits */
+#define	BS_UNCORRECTABLE_BIT		BIT(8)
+#define	BS_CORRECTABLE_ERR_MSK		0x1f
+
+/* NAND_DEVn_CFG0 bits */
+#define	DISABLE_STATUS_AFTER_WRITE	4
+#define	CW_PER_PAGE			6
+#define	UD_SIZE_BYTES			9
+#define	UD_SIZE_BYTES_MASK		GENMASK(18, 9)
+#define	ECC_PARITY_SIZE_BYTES_RS	19
+#define	SPARE_SIZE_BYTES		23
+#define	SPARE_SIZE_BYTES_MASK		GENMASK(26, 23)
+#define	NUM_ADDR_CYCLES			27
+#define	STATUS_BFR_READ			30
+#define	SET_RD_MODE_AFTER_STATUS	31
+
+/* NAND_DEVn_CFG0 bits */
+#define	DEV0_CFG1_ECC_DISABLE		0
+#define	WIDE_FLASH			1
+#define	NAND_RECOVERY_CYCLES		2
+#define	CS_ACTIVE_BSY			5
+#define	BAD_BLOCK_BYTE_NUM		6
+#define	BAD_BLOCK_IN_SPARE_AREA		16
+#define	WR_RD_BSY_GAP			17
+#define	ENABLE_BCH_ECC			27
+
+/* NAND_DEV0_ECC_CFG bits */
+#define	ECC_CFG_ECC_DISABLE		0
+#define	ECC_SW_RESET			1
+#define	ECC_MODE			4
+#define	ECC_PARITY_SIZE_BYTES_BCH	8
+#define	ECC_NUM_DATA_BYTES		16
+#define	ECC_NUM_DATA_BYTES_MASK		GENMASK(25, 16)
+#define	ECC_FORCE_CLK_OPEN		30
+
+/* NAND_DEV_CMD1 bits */
+#define	READ_ADDR			0
+
+/* NAND_DEV_CMD_VLD bits */
+#define	READ_START_VLD			BIT(0)
+#define	READ_STOP_VLD			BIT(1)
+#define	WRITE_START_VLD			BIT(2)
+#define	ERASE_START_VLD			BIT(3)
+#define	SEQ_READ_START_VLD		BIT(4)
+
+/* NAND_EBI2_ECC_BUF_CFG bits */
+#define	NUM_STEPS			0
+
+/* NAND_ERASED_CW_DETECT_CFG bits */
+#define	ERASED_CW_ECC_MASK		1
+#define	AUTO_DETECT_RES			0
+#define	MASK_ECC			(1 << ERASED_CW_ECC_MASK)
+#define	RESET_ERASED_DET		(1 << AUTO_DETECT_RES)
+#define	ACTIVE_ERASED_DET		(0 << AUTO_DETECT_RES)
+#define	CLR_ERASED_PAGE_DET		(RESET_ERASED_DET | MASK_ECC)
+#define	SET_ERASED_PAGE_DET		(ACTIVE_ERASED_DET | MASK_ECC)
+
+/* NAND_ERASED_CW_DETECT_STATUS bits */
+#define	PAGE_ALL_ERASED			BIT(7)
+#define	CODEWORD_ALL_ERASED		BIT(6)
+#define	PAGE_ERASED			BIT(5)
+#define	CODEWORD_ERASED			BIT(4)
+#define	ERASED_PAGE			(PAGE_ALL_ERASED | PAGE_ERASED)
+#define	ERASED_CW			(CODEWORD_ALL_ERASED | CODEWORD_ERASED)
+
+/* NAND_READ_LOCATION_n bits */
+#define READ_LOCATION_OFFSET		0
+#define READ_LOCATION_SIZE		16
+#define READ_LOCATION_LAST		31
+
+/* Version Mask */
+#define	NAND_VERSION_MAJOR_MASK		0xf0000000
+#define	NAND_VERSION_MAJOR_SHIFT	28
+#define	NAND_VERSION_MINOR_MASK		0x0fff0000
+#define	NAND_VERSION_MINOR_SHIFT	16
+
+/* NAND OP_CMDs */
+#define	OP_PAGE_READ			0x2
+#define	OP_PAGE_READ_WITH_ECC		0x3
+#define	OP_PAGE_READ_WITH_ECC_SPARE	0x4
+#define	OP_PAGE_READ_ONFI_READ		0x5
+#define	OP_PROGRAM_PAGE			0x6
+#define	OP_PAGE_PROGRAM_WITH_ECC	0x7
+#define	OP_PROGRAM_PAGE_SPARE		0x9
+#define	OP_BLOCK_ERASE			0xa
+#define	OP_CHECK_STATUS			0xc
+#define	OP_FETCH_ID			0xb
+#define	OP_RESET_DEVICE			0xd
+#define ACC_FEATURE			0xe
+
+/* Default Value for NAND_DEV_CMD_VLD */
+#define NAND_DEV_CMD_VLD_VAL		(READ_START_VLD | WRITE_START_VLD | \
+					 ERASE_START_VLD | SEQ_READ_START_VLD)
+/* NAND_CTRL bits */
+#define	BAM_MODE_EN			BIT(0)
+
+/*
+ * the largest page size we support is 8K, this will have 16 steps/codewords
+ * of 512 bytes each
+ */
+#define	MAX_NUM_STEPS			(SZ_8K / NANDC_STEP_SIZE)
+
+/* we read at most 3 registers per codeword scan */
+#define	MAX_REG_RD			(3 * MAX_NUM_STEPS)
+
+/* 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)
+
+#define nandc_set_read_loc_first(chip, reg, cw_offset, read_size, is_last_read_loc)	\
+nandc_set_reg(chip, reg,			\
+	      ((cw_offset) << READ_LOCATION_OFFSET) |		\
+	      ((read_size) << READ_LOCATION_SIZE) |			\
+	      ((is_last_read_loc) << READ_LOCATION_LAST))
+
+#define nandc_set_read_loc_last(chip, reg, cw_offset, read_size, is_last_read_loc)	\
+nandc_set_reg(chip, reg,			\
+	      ((cw_offset) << READ_LOCATION_OFFSET) |		\
+	      ((read_size) << READ_LOCATION_SIZE) |			\
+	      ((is_last_read_loc) << READ_LOCATION_LAST))
+
+/*
+ * Returns the actual register address for all NAND_DEV_ registers
+ * (i.e. NAND_DEV_CMD0, NAND_DEV_CMD1, NAND_DEV_CMD2 and NAND_DEV_CMD_VLD)
+ */
+#define dev_cmd_reg_addr(nandc, reg) ((nandc)->props->dev_cmd_reg_start + (reg))
+
+/* Returns the NAND register physical address */
+#define nandc_reg_phys(chip, offset) ((chip)->base_phys + (offset))
+
+/* Returns the dma address for reg read buffer */
+#define reg_buf_dma_addr(chip, vaddr) \
+	((chip)->reg_read_dma + \
+	((uint8_t *)(vaddr) - (uint8_t *)(chip)->reg_read_buf))
+
+#define QPIC_PER_CW_CMD_ELEMENTS	32
+#define QPIC_PER_CW_CMD_SGL		32
+#define QPIC_PER_CW_DATA_SGL		8
+
+#define QPIC_NAND_COMPLETION_TIMEOUT	msecs_to_jiffies(2000)
+
+/*
+ * Flags used in DMA descriptor preparation helper functions
+ * (i.e. read_reg_dma/write_reg_dma/read_data_dma/write_data_dma)
+ */
+/* Don't set the EOT in current tx BAM sgl */
+#define NAND_BAM_NO_EOT			BIT(0)
+/* Set the NWD flag in current BAM sgl */
+#define NAND_BAM_NWD			BIT(1)
+/* Finish writing in the current BAM sgl and start writing in another BAM sgl */
+#define NAND_BAM_NEXT_SGL		BIT(2)
+/*
+ * Erased codeword status is being used two times in single transfer so this
+ * flag will determine the current value of erased codeword status register
+ */
+#define NAND_ERASED_CW_SET		BIT(4)
+
+#define MAX_ADDRESS_CYCLE		5
+
+struct qpic_ecc {
+	struct device *dev;
+	const struct qpic_ecc_caps *caps;
+	struct completion done;
+	struct mutex lock;
+	u32 sectors;
+	u8 *eccdata;
+	bool use_ecc;
+	u32 ecc_modes;
+	int ecc_bytes_hw;
+	int spare_bytes;
+	int bbm_size;
+	int ecc_mode;
+	int bytes;
+	int steps;
+	int step_size;
+	int strength;
+	bool bch_enabled;
+};
+
+
+struct device_node;
+struct qpic_ecc;
+
+struct qcom_ecc_stats {
+	u32 corrected;
+	u32 bitflips;
+	u32 failed;
+};
+
+struct qcom_ecc {
+	bool use_ecc;
+	u32 ecc_modes;
+	int ecc_bytes_hw;
+	int spare_bytes;
+	int bbm_size;
+	int ecc_mode;
+	int bytes;
+	int steps;
+	bool bch_enabled;
+};
+
+/*
+ * This data type corresponds to the BAM transaction which will be used for all
+ * NAND transfers.
+ * @bam_ce - the array of BAM command elements
+ * @cmd_sgl - sgl for NAND BAM command pipe
+ * @data_sgl - sgl for NAND BAM consumer/producer pipe
+ * @last_data_desc - last DMA desc in data channel (tx/rx).
+ * @last_cmd_desc - last DMA desc in command channel.
+ * @txn_done - completion for NAND transfer.
+ * @bam_ce_pos - the index in bam_ce which is available for next sgl
+ * @bam_ce_start - the index in bam_ce which marks the start position ce
+ *		   for current sgl. It will be used for size calculation
+ *		   for current sgl
+ * @cmd_sgl_pos - current index in command sgl.
+ * @cmd_sgl_start - start index in command sgl.
+ * @tx_sgl_pos - current index in data sgl for tx.
+ * @tx_sgl_start - start index in data sgl for tx.
+ * @rx_sgl_pos - current index in data sgl for rx.
+ * @rx_sgl_start - start index in data sgl for rx.
+ * @wait_second_completion - wait for second DMA desc completion before making
+ *			     the NAND transfer completion.
+ */
+struct bam_transaction {
+	struct bam_cmd_element *bam_ce;
+	struct scatterlist *cmd_sgl;
+	struct scatterlist *data_sgl;
+	struct dma_async_tx_descriptor *last_data_desc;
+	struct dma_async_tx_descriptor *last_cmd_desc;
+	struct completion txn_done;
+	u32 bam_ce_pos;
+	u32 bam_ce_start;
+	u32 cmd_sgl_pos;
+	u32 cmd_sgl_start;
+	u32 tx_sgl_pos;
+	u32 tx_sgl_start;
+	u32 rx_sgl_pos;
+	u32 rx_sgl_start;
+	bool wait_second_completion;
+};
+
+/*
+ * This data type corresponds to the nand dma descriptor
+ * @dma_desc - low level DMA engine descriptor
+ * @list - list for desc_info
+ *
+ * @adm_sgl - sgl which will be used for single sgl dma descriptor. Only used by
+ *	      ADM
+ * @bam_sgl - sgl which will be used for dma descriptor. Only used by BAM
+ * @sgl_cnt - number of SGL in bam_sgl. Only used by BAM
+ * @dir - DMA transfer direction
+ */
+struct desc_info {
+	struct dma_async_tx_descriptor *dma_desc;
+	struct list_head node;
+
+	union {
+		struct scatterlist adm_sgl;
+		struct {
+			struct scatterlist *bam_sgl;
+			int sgl_cnt;
+		};
+	};
+	enum dma_data_direction dir;
+};
+
+/*
+ * holds the current register values that we want to write. acts as a contiguous
+ * chunk of memory which we use to write the controller registers through DMA.
+ */
+struct nandc_regs {
+	__le32 cmd;
+	__le32 addr0;
+	__le32 addr1;
+	__le32 chip_sel;
+	__le32 exec;
+
+	__le32 cfg0;
+	__le32 cfg1;
+	__le32 ecc_bch_cfg;
+
+	__le32 clrflashstatus;
+	__le32 clrreadstatus;
+
+	__le32 cmd1;
+	__le32 vld;
+
+	__le32 orig_cmd1;
+	__le32 orig_vld;
+
+	__le32 ecc_buf_cfg;
+	__le32 read_location0;
+	__le32 read_location1;
+	__le32 read_location2;
+	__le32 read_location3;
+	__le32 read_location_last0;
+	__le32 read_location_last1;
+	__le32 read_location_last2;
+	__le32 read_location_last3;
+	__le32 flash_feature;
+	__le32 spi_cfg;
+	__le32 num_addr_cycle;
+	__le32 busy_wait_cnt;
+	__le32 mstr_cfg;
+
+	__le32 erased_cw_detect_cfg_clr;
+	__le32 erased_cw_detect_cfg_set;
+};
+
+/*
+ * This data type corresponds to the NAND controller properties which varies
+ * among different NAND controllers.
+ * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
+ * @is_bam - whether NAND controller is using BAM
+ * @is_qpic - whether NAND CTRL is part of qpic IP
+ * @qpic_v2 - flag to indicate QPIC IP version 2
+ * @use_codeword_fixup - whether NAND has different layout for boot partitions
+ */
+struct qcom_nandc_props {
+	u32 dev_cmd_reg_start;
+	bool is_bam;
+	bool is_qpic;
+	bool qpic_v2;
+	bool use_codeword_fixup;
+};
+
+/*
+ * NAND controller data struct
+ *
+ * @dev:			parent device
+ *
+ * @base:			MMIO base
+ *
+ * @core_clk:			controller clock
+ * @aon_clk:			another controller clock
+ *
+ * @regs:			a contiguous chunk of memory for DMA register
+ *				writes. contains the register values to be
+ *				written to controller
+ *
+ * @props:			properties of current NAND controller,
+ *				initialized via DT match data
+ *
+ * @controller:			base controller structure
+ * @host_list:			list containing all the chips attached to the
+ *				controller
+ *
+ * @chan:			dma channel
+ * @cmd_crci:			ADM DMA CRCI for command flow control
+ * @data_crci:			ADM DMA CRCI for data flow control
+ *
+ * @desc_list:			DMA descriptor list (list of desc_infos)
+ *
+ * @data_buffer:		our local DMA buffer for page read/writes,
+ *				used when we can't use the buffer provided
+ *				by upper layers directly
+ * @reg_read_buf:		local buffer for reading back registers via DMA
+ *
+ * @base_phys:			physical base address of controller registers
+ * @base_dma:			dma base address of controller registers
+ * @reg_read_dma:		contains dma address for register read buffer
+ *
+ * @buf_size/count/start:	markers for chip->legacy.read_buf/write_buf
+ *				functions
+ * @max_cwperpage:		maximum QPIC codewords required. calculated
+ *				from all connected NAND devices pagesize
+ *
+ * @reg_read_pos:		marker for data read in reg_read_buf
+ *
+ * @cmd1/vld:			some fixed controller register values
+ *
+ * @exec_opwrite:		flag to select correct number of code word
+ *				while reading status
+ */
+struct qcom_nand_controller {
+	struct spi_controller *ctlr;
+	struct device *dev;
+
+	void __iomem *base;
+
+	struct clk *core_clk;
+	struct clk *aon_clk;
+	struct clk *iomacro_clk;
+
+	struct nandc_regs *regs;
+	struct bam_transaction *bam_txn;
+
+	const struct qcom_nandc_props *props;
+
+	struct nand_controller controller;
+	struct list_head host_list;
+
+	union {
+		/* will be used only by QPIC for BAM DMA */
+		struct {
+			struct dma_chan *tx_chan;
+			struct dma_chan *rx_chan;
+			struct dma_chan *cmd_chan;
+		};
+
+		/* will be used only by EBI2 for ADM DMA */
+		struct {
+			struct dma_chan *chan;
+			unsigned int cmd_crci;
+			unsigned int data_crci;
+		};
+	};
+
+	struct list_head desc_list;
+
+	u8		*data_buffer;
+	__le32		*reg_read_buf;
+
+	struct qpic_ecc *ecc;
+	struct qcom_ecc_stats ecc_stats;
+	struct nand_ecc_engine ecc_eng;
+	phys_addr_t base_phys;
+	dma_addr_t base_dma;
+	dma_addr_t reg_read_dma;
+
+	int		buf_size;
+	int		buf_count;
+	int		buf_start;
+	unsigned int	max_cwperpage;
+
+	int reg_read_pos;
+
+	u32 cmd1, vld;
+	bool exec_opwrite;
+};
+
+/*
+ * NAND chip structure
+ *
+ * @boot_partitions:		array of boot partitions where offset and size of the
+ *				boot partitions are stored
+ *
+ * @chip:			base NAND chip structure
+ * @node:			list node to add itself to host_list in
+ *				qcom_nand_controller
+ *
+ * @nr_boot_partitions:		count of the boot partitions where spare data is not
+ *				protected by ECC
+ *
+ * @cs:				chip select value for this chip
+ * @cw_size:			the number of bytes in a single step/codeword
+ *				of a page, consisting of all data, ecc, spare
+ *				and reserved bytes
+ * @cw_data:			the number of bytes within a codeword protected
+ *				by ECC
+ * @ecc_bytes_hw:		ECC bytes used by controller hardware for this
+ *				chip
+ *
+ * @last_command:		keeps track of last command on this chip. used
+ *				for reading correct status
+ *
+ * @cfg0, cfg1, cfg0_raw..:	NANDc register configurations needed for
+ *				ecc/non-ecc mode for the current nand flash
+ *				device
+ *
+ * @status:			value to be returned if NAND_CMD_STATUS command
+ *				is executed
+ * @codeword_fixup:		keep track of the current layout used by
+ *				the driver for read/write operation.
+ * @use_ecc:			request the controller to use ECC for the
+ *				upcoming read/write
+ * @bch_enabled:		flag to tell whether BCH ECC mode is used
+ */
+struct qcom_nand_host {
+	struct qcom_nand_boot_partition *boot_partitions;
+
+	struct nand_chip chip;
+	struct list_head node;
+
+	int nr_boot_partitions;
+
+	int cs;
+	int cw_size;
+	int cw_data;
+	int ecc_bytes_hw;
+	int spare_bytes;
+	int bbm_size;
+
+	int last_command;
+
+	u32 cfg0, cfg1;
+	u32 cfg0_raw, cfg1_raw;
+	u32 ecc_buf_cfg;
+	u32 ecc_bch_cfg;
+	u32 clrflashstatus;
+	u32 clrreadstatus;
+
+	u8 status;
+	bool codeword_fixup;
+	bool use_ecc;
+	bool bch_enabled;
+};
+
+struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip);
+
+struct qcom_nand_controller *get_qcom_nand_controller(struct nand_chip *chip);
+
+void qcom_ecc_enable(struct qcom_ecc *ecc);
+
+void qcom_ecc_disable(struct qcom_ecc *ecc);
+
+struct qcom_ecc *of_qcom_ecc_get(struct device_node *);
+
+int qcom_ecc_config(struct qpic_ecc *ecc, int ecc_strength,
+			bool wide_bus);
+
+int qcom_nandc_alloc(struct qcom_nand_controller *nandc);
+
+void qcom_nandc_unalloc(struct qcom_nand_controller *nandc);
+
+int qcom_nandc_setup(struct qcom_nand_controller *nandc);
+
+struct bam_transaction *
+alloc_bam_transaction(struct qcom_nand_controller *nandc);
+
+void clear_bam_transaction(struct qcom_nand_controller *nandc);
+int prepare_bam_async_desc(struct qcom_nand_controller *nandc,
+				  struct dma_chan *chan,
+				  unsigned long flags);
+int prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read,
+			     int reg_off, const void *vaddr, int size,
+			     bool flow_control);
+
+int submit_descs(struct qcom_nand_controller *nandc);
+
+void free_descs(struct qcom_nand_controller *nandc);
+
+void nandc_set_reg(struct qcom_nand_controller *nandc, int offset,
+			  u32 val);
+int write_reg_dma(struct qcom_nand_controller *nandc, int first,
+			 int num_regs, unsigned int flags);
+
+int read_reg_dma(struct qcom_nand_controller *nandc, int first,
+			int num_regs, unsigned int flags);
+void clear_read_regs(struct qcom_nand_controller *nandc);
+
+void nandc_read_buffer_sync(struct qcom_nand_controller *nandc,
+					  bool is_cpu);
+struct qpic_ecc *of_qpic_ecc_get(struct device_node *);
+#endif
+
-- 
2.34.1

Re: [RFC PATCH 3/5] mtd: nand: qpic_common: Add support for qpic common API

[Miquel Raynal]

Hi,

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

> Add qpic command API in sperate file so that it will be
> use by both spi and request and raw nand request.

		nand?

> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> ---
>  drivers/mtd/nand/qpic_common.c       | 840 +++++++++++++++++++++++++++
>  include/linux/mtd/nand-qpic-common.h | 641 ++++++++++++++++++++
>  2 files changed, 1481 insertions(+)
>  create mode 100644 drivers/mtd/nand/qpic_common.c
>  create mode 100644 include/linux/mtd/nand-qpic-common.h
> 
> diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c
> new file mode 100644
> index 000000000000..983768a1ea94
> --- /dev/null
> +++ b/drivers/mtd/nand/qpic_common.c
> @@ -0,0 +1,840 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * QPIC common API file.
> + * Copyright (C) 2023  Qualcomm Inc.
> + * Authors:	Md sadre Alam		<quic_mdalam@quicinc.com>
> + *		Sricharan R		<quic_srichara@quicinc.com>
> + */
> +
> +#include <linux/mtd/nand-qpic-common.h>
> +
> +/* Frees the BAM transaction memory */
> +void free_bam_transaction(struct qcom_nand_controller *nandc)

This is not a generic object, it's not meant to be shared with spi. So
if it's meant to be used in a single place, please don't share it.
Otherwise please use a more generic name.

> +{
> +	struct bam_transaction *bam_txn = nandc->bam_txn;
> +
> +	devm_kfree(nandc->dev, bam_txn);
> +}
> +EXPORT_SYMBOL(free_bam_transaction);
> +
> +/* Callback for DMA descriptor completion */
> +void qpic_bam_dma_done(void *data)
> +{
> +	struct bam_transaction *bam_txn = data;

Just provide the right pointer in the first place.

> +
> +	/*
> +	 * In case of data transfer with NAND, 2 callbacks will be generated.

What if we are not dealing with a NAND?

> +	 * One for command channel and another one for data channel.
> +	 * If current transaction has data descriptors
> +	 * (i.e. wait_second_completion is true), then set this to false

Why do you need a boolean for that if having a data descriptor is the
condition you want to check against?

> +	 * and wait for second DMA descriptor completion.
> +	 */
> +	if (bam_txn->wait_second_completion)
> +		bam_txn->wait_second_completion = false;

This looks very racy.

> +	else
> +		complete(&bam_txn->txn_done);
> +}
> +EXPORT_SYMBOL(qpic_bam_dma_done);
> +
> +u32 nandc_read(struct qcom_nand_controller *nandc, int offset)
> +{
> +	return ioread32(nandc->base + offset);

I don't see the need for that.

> +}
> +EXPORT_SYMBOL(nandc_read);
> +
> +void nandc_write(struct qcom_nand_controller *nandc, int offset,
> +			       u32 val)
> +{
> +	iowrite32(val, nandc->base + offset);

Same here.

> +}
> +EXPORT_SYMBOL(nandc_write);
> +
> +void nandc_read_buffer_sync(struct qcom_nand_controller *nandc,
> +					  bool is_cpu)
> +{
> +	if (!nandc->props->is_bam)
> +		return;

I thought BAM was a per transaction thing, and here you check it
against a controller parameter. It looks wrong.

> +
> +	if (is_cpu)

Whuut? naming, naming, naming, naming. Please.

> +		dma_sync_single_for_cpu(nandc->dev, nandc->reg_read_dma,
> +					MAX_REG_RD *

Remove this new line

> +					sizeof(*nandc->reg_read_buf),

Are you sure this sizeof() is safe?

> +					DMA_FROM_DEVICE);
> +	else
> +		dma_sync_single_for_device(nandc->dev, nandc->reg_read_dma,
> +					   MAX_REG_RD *
> +					   sizeof(*nandc->reg_read_buf),
> +					   DMA_FROM_DEVICE);
> +}

You better have a really good reason to sync in the middle of an
operation...

> +EXPORT_SYMBOL(nandc_read_buffer_sync);
> +
> +__le32 *offset_to_nandc_reg(struct nandc_regs *regs, int offset)
> +{
> +	switch (offset) {
> +	case NAND_FLASH_CMD:
> +		return &regs->cmd;
> +	case NAND_ADDR0:
> +		return &regs->addr0;
> +	case NAND_ADDR1:
> +		return &regs->addr1;
> +	case NAND_FLASH_CHIP_SELECT:
> +		return &regs->chip_sel;
> +	case NAND_EXEC_CMD:
> +		return &regs->exec;
> +	case NAND_FLASH_STATUS:
> +		return &regs->clrflashstatus;
> +	case NAND_DEV0_CFG0:
> +		return &regs->cfg0;
> +	case NAND_DEV0_CFG1:
> +		return &regs->cfg1;
> +	case NAND_DEV0_ECC_CFG:
> +		return &regs->ecc_bch_cfg;
> +	case NAND_READ_STATUS:
> +		return &regs->clrreadstatus;
> +	case NAND_DEV_CMD1:
> +		return &regs->cmd1;
> +	case NAND_DEV_CMD1_RESTORE:
> +		return &regs->orig_cmd1;
> +	case NAND_DEV_CMD_VLD:
> +		return &regs->vld;
> +	case NAND_DEV_CMD_VLD_RESTORE:
> +		return &regs->orig_vld;
> +	case NAND_EBI2_ECC_BUF_CFG:
> +		return &regs->ecc_buf_cfg;
> +	case NAND_READ_LOCATION_0:
> +		return &regs->read_location0;
> +	case NAND_READ_LOCATION_1:
> +		return &regs->read_location1;
> +	case NAND_READ_LOCATION_2:
> +		return &regs->read_location2;
> +	case NAND_READ_LOCATION_3:
> +		return &regs->read_location3;
> +	case NAND_READ_LOCATION_LAST_CW_0:
> +		return &regs->read_location_last0;
> +	case NAND_READ_LOCATION_LAST_CW_1:
> +		return &regs->read_location_last1;
> +	case NAND_READ_LOCATION_LAST_CW_2:
> +		return &regs->read_location_last2;
> +	case NAND_READ_LOCATION_LAST_CW_3:
> +		return &regs->read_location_last3;
> +	case NAND_FLASH_SPI_CFG:
> +		return &regs->spi_cfg;
> +	case NAND_NUM_ADDR_CYCLES:
> +		return &regs->num_addr_cycle;
> +	case NAND_BUSY_CHECK_WAIT_CNT:
> +		return &regs->busy_wait_cnt;
> +	case NAND_MSTR_CONFIG:
> +		return &regs->mstr_cfg;
> +	case NAND_FLASH_FEATURES:
> +		return &regs->flash_feature;

Just... no? What is the point of using an intermediate int here? just
dereference the right value?

> +	default:
> +		return NULL;
> +	}
> +}
> +EXPORT_SYMBOL(offset_to_nandc_reg);
> +
> +/* reset the register read buffer for next NAND operation */
> +void clear_read_regs(struct qcom_nand_controller *nandc)
> +{
> +	nandc->reg_read_pos = 0;
> +	nandc_read_buffer_sync(nandc, false);
> +}
> +EXPORT_SYMBOL(clear_read_regs);
> +
> +int prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read,

adm?


Please always prefix your symbols (especially when exporting them) with
a decent a easy to recognize prefix.

> +			     int reg_off, const void *vaddr, int size,
> +			     bool flow_control)
> +{
> +	struct desc_info *desc;
> +	struct dma_async_tx_descriptor *dma_desc;
> +	struct scatterlist *sgl;
> +	struct dma_slave_config slave_conf;
> +	struct qcom_adm_peripheral_config periph_conf = {};
> +	enum dma_transfer_direction dir_eng;
> +	int ret;
> +
> +	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +	if (!desc)
> +		return -ENOMEM;
> +
> +	sgl = &desc->adm_sgl;
> +
> +	sg_init_one(sgl, vaddr, size);
> +
> +	if (read) {
> +		dir_eng = DMA_DEV_TO_MEM;
> +		desc->dir = DMA_FROM_DEVICE;
> +	} else {
> +		dir_eng = DMA_MEM_TO_DEV;
> +		desc->dir = DMA_TO_DEVICE;
> +	}
> +
> +	ret = dma_map_sg(nandc->dev, sgl, 1, desc->dir);
> +	if (ret == 0) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	memset(&slave_conf, 0x00, sizeof(slave_conf));
> +
> +	slave_conf.device_fc = flow_control;
> +	if (read) {
> +		slave_conf.src_maxburst = 16;
> +		slave_conf.src_addr = nandc->base_dma + reg_off;
> +		if (nandc->data_crci) {
> +			periph_conf.crci = nandc->data_crci;
> +			slave_conf.peripheral_config = &periph_conf;
> +			slave_conf.peripheral_size = sizeof(periph_conf);
> +		}
> +	} else {
> +		slave_conf.dst_maxburst = 16;
> +		slave_conf.dst_addr = nandc->base_dma + reg_off;
> +		if (nandc->cmd_crci) {
> +			periph_conf.crci = nandc->cmd_crci;
> +			slave_conf.peripheral_config = &periph_conf;
> +			slave_conf.peripheral_size = sizeof(periph_conf);
> +		}
> +	}
> +
> +	ret = dmaengine_slave_config(nandc->chan, &slave_conf);
> +	if (ret) {
> +		dev_err(nandc->dev, "failed to configure dma channel\n");
> +		goto err;
> +	}
> +
> +	dma_desc = dmaengine_prep_slave_sg(nandc->chan, sgl, 1, dir_eng, 0);
> +	if (!dma_desc) {
> +		dev_err(nandc->dev, "failed to prepare desc\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	desc->dma_desc = dma_desc;
> +
> +	list_add_tail(&desc->node, &nandc->desc_list);
> +
> +	return 0;
> +err:

your error path is missing steps

> +	kfree(desc);
> +
> +	return ret;
> +}
> +
> +/* helpers to submit/free our list of dma descriptors */
> +int submit_descs(struct qcom_nand_controller *nandc)
> +{
> +	struct desc_info *desc;
> +	dma_cookie_t cookie = 0;
> +	struct bam_transaction *bam_txn = nandc->bam_txn;
> +	int r;
> +
> +	if (nandc->props->is_bam) {
> +		if (bam_txn->rx_sgl_pos > bam_txn->rx_sgl_start) {
> +			r = prepare_bam_async_desc(nandc, nandc->rx_chan, 0);
> +			if (r)
> +				return r;
> +		}
> +
> +		if (bam_txn->tx_sgl_pos > bam_txn->tx_sgl_start) {
> +			r = prepare_bam_async_desc(nandc, nandc->tx_chan,
> +						   DMA_PREP_INTERRUPT);
> +			if (r)
> +				return r;
> +		}
> +
> +		if (bam_txn->cmd_sgl_pos > bam_txn->cmd_sgl_start) {
> +			r = prepare_bam_async_desc(nandc, nandc->cmd_chan,
> +						   DMA_PREP_CMD);
> +			if (r)
> +				return r;
> +		}
> +	}
> +
> +	list_for_each_entry(desc, &nandc->desc_list, node)
> +		cookie = dmaengine_submit(desc->dma_desc);
> +
> +	if (nandc->props->is_bam) {
> +		bam_txn->last_cmd_desc->callback = qpic_bam_dma_done;
> +		bam_txn->last_cmd_desc->callback_param = bam_txn;
> +		if (bam_txn->last_data_desc) {
> +			bam_txn->last_data_desc->callback = qpic_bam_dma_done;
> +			bam_txn->last_data_desc->callback_param = bam_txn;
> +			bam_txn->wait_second_completion = true;
> +		}
> +
> +		dma_async_issue_pending(nandc->tx_chan);
> +		dma_async_issue_pending(nandc->rx_chan);
> +		dma_async_issue_pending(nandc->cmd_chan);
> +
> +		if (!wait_for_completion_timeout(&bam_txn->txn_done,
> +						 QPIC_NAND_COMPLETION_TIMEOUT))
> +			return -ETIMEDOUT;
> +	} else {
> +		if (dma_sync_wait(nandc->chan, cookie) != DMA_COMPLETE)
> +			return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(submit_descs);
> +
> +void free_descs(struct qcom_nand_controller *nandc)
> +{
> +	struct desc_info *desc, *n;
> +
> +	list_for_each_entry_safe(desc, n, &nandc->desc_list, node) {
> +		list_del(&desc->node);
> +
> +		if (nandc->props->is_bam)
> +			dma_unmap_sg(nandc->dev, desc->bam_sgl,
> +				     desc->sgl_cnt, desc->dir);
> +		else
> +			dma_unmap_sg(nandc->dev, &desc->adm_sgl, 1,
> +				     desc->dir);
> +
> +		kfree(desc);
> +	}
> +}
> +EXPORT_SYMBOL(free_descs);
> +
> +/*
> + * Maps the scatter gather list for DMA transfer and forms the DMA descriptor
> + * for BAM. This descriptor will be added in the NAND DMA descriptor queue
> + * which will be submitted to DMA engine.
> + */
> +int prepare_bam_async_desc(struct qcom_nand_controller *nandc,
> +				  struct dma_chan *chan,
> +				  unsigned long flags)
> +{
> +	struct desc_info *desc;
> +	struct scatterlist *sgl;
> +	unsigned int sgl_cnt;
> +	int ret;
> +	struct bam_transaction *bam_txn = nandc->bam_txn;
> +	enum dma_transfer_direction dir_eng;
> +	struct dma_async_tx_descriptor *dma_desc;
> +
> +	desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> +	if (!desc)
> +		return -ENOMEM;
> +
> +	if (chan == nandc->cmd_chan) {
> +		sgl = &bam_txn->cmd_sgl[bam_txn->cmd_sgl_start];
> +		sgl_cnt = bam_txn->cmd_sgl_pos - bam_txn->cmd_sgl_start;
> +		bam_txn->cmd_sgl_start = bam_txn->cmd_sgl_pos;
> +		dir_eng = DMA_MEM_TO_DEV;
> +		desc->dir = DMA_TO_DEVICE;
> +	} else if (chan == nandc->tx_chan) {
> +		sgl = &bam_txn->data_sgl[bam_txn->tx_sgl_start];
> +		sgl_cnt = bam_txn->tx_sgl_pos - bam_txn->tx_sgl_start;
> +		bam_txn->tx_sgl_start = bam_txn->tx_sgl_pos;
> +		dir_eng = DMA_MEM_TO_DEV;
> +		desc->dir = DMA_TO_DEVICE;
> +	} else {
> +		sgl = &bam_txn->data_sgl[bam_txn->rx_sgl_start];
> +		sgl_cnt = bam_txn->rx_sgl_pos - bam_txn->rx_sgl_start;
> +		bam_txn->rx_sgl_start = bam_txn->rx_sgl_pos;
> +		dir_eng = DMA_DEV_TO_MEM;
> +		desc->dir = DMA_FROM_DEVICE;
> +	}
> +
> +	sg_mark_end(sgl + sgl_cnt - 1);
> +	ret = dma_map_sg(nandc->dev, sgl, sgl_cnt, desc->dir);
> +	if (ret == 0) {
> +		dev_err(nandc->dev, "failure in mapping desc\n");
> +		kfree(desc);
> +		return -ENOMEM;
> +	}
> +
> +	desc->sgl_cnt = sgl_cnt;
> +	desc->bam_sgl = sgl;
> +
> +	dma_desc = dmaengine_prep_slave_sg(chan, sgl, sgl_cnt, dir_eng,
> +					   flags);
> +
> +	if (!dma_desc) {
> +		dev_err(nandc->dev, "failure in prep desc\n");
> +		dma_unmap_sg(nandc->dev, sgl, sgl_cnt, desc->dir);
> +		kfree(desc);
> +		return -EINVAL;
> +	}
> +
> +	desc->dma_desc = dma_desc;
> +
> +	/* update last data/command descriptor */
> +	if (chan == nandc->cmd_chan)
> +		bam_txn->last_cmd_desc = dma_desc;
> +	else
> +		bam_txn->last_data_desc = dma_desc;
> +
> +	list_add_tail(&desc->node, &nandc->desc_list);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(prepare_bam_async_desc);
> +
> +/*
> + * Prepares the command descriptor for BAM DMA which will be used for NAND
> + * register reads and writes. The command descriptor requires the command
> + * to be formed in command element type so this function uses the command
> + * element from bam transaction ce array and fills the same with required

ce?

> + * data. A single SGL can contain multiple command elements so
> + * NAND_BAM_NEXT_SGL will be used for starting the separate SGL
> + * after the current command element.
> + */
> +int prep_bam_dma_desc_cmd(struct qcom_nand_controller *nandc, bool read,
> +				 int reg_off, const void *vaddr,
> +				 int size, unsigned int flags)
> +{
> +	int bam_ce_size;
> +	int i, ret;
> +	struct bam_cmd_element *bam_ce_buffer;
> +	struct bam_transaction *bam_txn = nandc->bam_txn;
> +
> +	bam_ce_buffer = &bam_txn->bam_ce[bam_txn->bam_ce_pos];
> +
> +	/* fill the command desc */
> +	for (i = 0; i < size; i++) {
> +		if (read)
> +			bam_prep_ce(&bam_ce_buffer[i],
> +				    nandc_reg_phys(nandc, reg_off + 4 * i),
> +				    BAM_READ_COMMAND,
> +				    reg_buf_dma_addr(nandc,
> +						     (__le32 *)vaddr + i));
> +		else
> +			bam_prep_ce_le32(&bam_ce_buffer[i],
> +					 nandc_reg_phys(nandc, reg_off + 4 * i),
> +					 BAM_WRITE_COMMAND,
> +					 *((__le32 *)vaddr + i));

The _le handling looks really fragile.

> +	}
> +
> +	bam_txn->bam_ce_pos += size;
> +
> +	/* use the separate sgl after this command */
> +	if (flags & NAND_BAM_NEXT_SGL) {
> +		bam_ce_buffer = &bam_txn->bam_ce[bam_txn->bam_ce_start];
> +		bam_ce_size = (bam_txn->bam_ce_pos -
> +				bam_txn->bam_ce_start) *
> +				sizeof(struct bam_cmd_element);
> +		sg_set_buf(&bam_txn->cmd_sgl[bam_txn->cmd_sgl_pos],
> +			   bam_ce_buffer, bam_ce_size);
> +		bam_txn->cmd_sgl_pos++;
> +		bam_txn->bam_ce_start = bam_txn->bam_ce_pos;
> +
> +		if (flags & NAND_BAM_NWD) {
> +			ret = prepare_bam_async_desc(nandc, nandc->cmd_chan,
> +						     DMA_PREP_FENCE |
> +						     DMA_PREP_CMD);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(prep_bam_dma_desc_cmd);
> +
> +/*
> + * Prepares the data descriptor for BAM DMA which will be used for NAND
> + * data reads and writes.
> + */
> +int prep_bam_dma_desc_data(struct qcom_nand_controller *nandc, bool read,
> +				  const void *vaddr,
> +				  int size, unsigned int flags)
> +{
> +	int ret;
> +	struct bam_transaction *bam_txn = nandc->bam_txn;
> +
> +	if (read) {
> +		sg_set_buf(&bam_txn->data_sgl[bam_txn->rx_sgl_pos],
> +			   vaddr, size);
> +		bam_txn->rx_sgl_pos++;
> +	} else {
> +		sg_set_buf(&bam_txn->data_sgl[bam_txn->tx_sgl_pos],
> +			   vaddr, size);
> +		bam_txn->tx_sgl_pos++;
> +
> +		/*
> +		 * BAM will only set EOT for DMA_PREP_INTERRUPT so if this flag
> +		 * is not set, form the DMA descriptor
> +		 */
> +		if (!(flags & NAND_BAM_NO_EOT)) {
> +			ret = prepare_bam_async_desc(nandc, nandc->tx_chan,
> +						     DMA_PREP_INTERRUPT);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(prep_bam_dma_desc_data);
> +
> +/*
> + * read_reg_dma:	prepares a descriptor to read a given number of
> + *			contiguous registers to the reg_read_buf pointer
> + *
> + * @first:		offset of the first register in the contiguous block
> + * @num_regs:		number of registers to read
> + * @flags:		flags to control DMA descriptor preparation
> + */
> +int read_reg_dma(struct qcom_nand_controller *nandc, int first,
> +			int num_regs, unsigned int flags)
> +{
> +	bool flow_control = false;
> +	void *vaddr;
> +
> +	vaddr = nandc->reg_read_buf + nandc->reg_read_pos;
> +	nandc->reg_read_pos += num_regs;
> +
> +	if (first == NAND_DEV_CMD_VLD || first == NAND_DEV_CMD1)
> +		first = dev_cmd_reg_addr(nandc, first);
> +
> +	if (nandc->props->is_bam)
> +		return prep_bam_dma_desc_cmd(nandc, true, first, vaddr,
> +					     num_regs, flags);
> +
> +	if (first == NAND_READ_ID || first == NAND_FLASH_STATUS)
> +		flow_control = true;

This flow_control property really bothers me.

> +
> +	return prep_adm_dma_desc(nandc, true, first, vaddr,
> +				 num_regs * sizeof(u32), flow_control);
> +}
> +EXPORT_SYMBOL(read_reg_dma);
> +
> +/*
> + * write_reg_dma:	prepares a descriptor to write a given number of
> + *			contiguous registers
> + *
> + * @first:		offset of the first register in the contiguous block
> + * @num_regs:		number of registers to write
> + * @flags:		flags to control DMA descriptor preparation
> + */
> +int write_reg_dma(struct qcom_nand_controller *nandc, int first,
> +			 int num_regs, unsigned int flags)
> +{
> +	bool flow_control = false;
> +	struct nandc_regs *regs = nandc->regs;
> +	void *vaddr;
> +
> +	vaddr = offset_to_nandc_reg(regs, first);
> +
> +	if (first == NAND_ERASED_CW_DETECT_CFG) {
> +		if (flags & NAND_ERASED_CW_SET)
> +			vaddr = &regs->erased_cw_detect_cfg_set;
> +		else
> +			vaddr = &regs->erased_cw_detect_cfg_clr;
> +	}
> +
> +	if (first == NAND_EXEC_CMD)
> +		flags |= NAND_BAM_NWD;
> +
> +	if (first == NAND_FLASH_SPI_CFG || first == NAND_NUM_ADDR_CYCLES
> +		|| first == NAND_BUSY_CHECK_WAIT_CNT
> +		|| first == NAND_MSTR_CONFIG)
> +		first = dev_cmd_reg_addr(nandc, first);
> +
> +	if (first == NAND_DEV_CMD1_RESTORE || first == NAND_DEV_CMD1)
> +		first = dev_cmd_reg_addr(nandc, NAND_DEV_CMD1);
> +
> +	if (first == NAND_DEV_CMD_VLD_RESTORE || first == NAND_DEV_CMD_VLD)
> +		first = dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD);
> +

What about a switch case here?

> +	if (nandc->props->is_bam)
> +		return prep_bam_dma_desc_cmd(nandc, false, first, vaddr,
> +					     num_regs, flags);
> +
> +	if (first == NAND_FLASH_CMD)
> +		flow_control = true;
> +
> +	return prep_adm_dma_desc(nandc, false, first, vaddr,
> +				 num_regs * sizeof(u32), flow_control);
> +}
> +EXPORT_SYMBOL(write_reg_dma);
> +
> +/*
> + * read_data_dma:	prepares a DMA descriptor to transfer data from the
> + *			controller's internal buffer to the buffer 'vaddr'
> + *
> + * @reg_off:		offset within the controller's data buffer
> + * @vaddr:		virtual address of the buffer we want to write to
> + * @size:		DMA transaction size in bytes
> + * @flags:		flags to control DMA descriptor preparation
> + */
> +int read_data_dma(struct qcom_nand_controller *nandc, int reg_off,
> +			 const u8 *vaddr, int size, unsigned int flags)
> +{
> +	if (nandc->props->is_bam)
> +		return prep_bam_dma_desc_data(nandc, true, vaddr, size, flags);
> +
> +	return prep_adm_dma_desc(nandc, true, reg_off, vaddr, size, false);
> +}
> +EXPORT_SYMBOL(read_data_dma);
> +
> +/*
> + * write_data_dma:	prepares a DMA descriptor to transfer data from
> + *			'vaddr' to the controller's internal buffer
> + *
> + * @reg_off:		offset within the controller's data buffer
> + * @vaddr:		virtual address of the buffer we want to read from
> + * @size:		DMA transaction size in bytes
> + * @flags:		flags to control DMA descriptor preparation
> + */
> +int write_data_dma(struct qcom_nand_controller *nandc, int reg_off,
> +			  const u8 *vaddr, int size, unsigned int flags)
> +{
> +	if (nandc->props->is_bam)
> +		return prep_bam_dma_desc_data(nandc, false, vaddr, size, flags);
> +
> +	return prep_adm_dma_desc(nandc, false, reg_off, vaddr, size, false);
> +}
> +EXPORT_SYMBOL(write_data_dma);
> +
> +void nandc_set_reg(struct qcom_nand_controller *nandc, int offset,
> +			  u32 val)
> +{
> +	struct nandc_regs *regs = nandc->regs;
> +	__le32 *reg;
> +
> +	reg = offset_to_nandc_reg(regs, offset);
> +	if (reg)
> +		*reg = cpu_to_le32(val);
> +}
> +EXPORT_SYMBOL(nandc_set_reg);
> +
> +/* Allocates and Initializes the BAM transaction */
> +struct bam_transaction *
> +alloc_bam_transaction(struct qcom_nand_controller *nandc)
> +{
> +	struct bam_transaction *bam_txn;
> +	size_t bam_txn_size;
> +	unsigned int num_cw = nandc->max_cwperpage;
> +	void *bam_txn_buf;
> +
> +	bam_txn_size =
> +		sizeof(*bam_txn) + num_cw *
> +		((sizeof(*bam_txn->bam_ce) * QPIC_PER_CW_CMD_ELEMENTS) +
> +		(sizeof(*bam_txn->cmd_sgl) * QPIC_PER_CW_CMD_SGL) +
> +		(sizeof(*bam_txn->data_sgl) * QPIC_PER_CW_DATA_SGL));
> +
> +	bam_txn_buf = devm_kzalloc(nandc->dev, bam_txn_size, GFP_KERNEL);
> +	if (!bam_txn_buf)
> +		return NULL;
> +
> +	bam_txn = bam_txn_buf;
> +	bam_txn_buf += sizeof(*bam_txn);
> +
> +	bam_txn->bam_ce = bam_txn_buf;
> +	bam_txn_buf +=
> +		sizeof(*bam_txn->bam_ce) * QPIC_PER_CW_CMD_ELEMENTS * num_cw;
> +
> +	bam_txn->cmd_sgl = bam_txn_buf;
> +	bam_txn_buf +=
> +		sizeof(*bam_txn->cmd_sgl) * QPIC_PER_CW_CMD_SGL * num_cw;
> +
> +	bam_txn->data_sgl = bam_txn_buf;
> +
> +	init_completion(&bam_txn->txn_done);
> +
> +	return bam_txn;
> +}
> +EXPORT_SYMBOL(alloc_bam_transaction);
> +
> +/* Clears the BAM transaction indexes */
> +void clear_bam_transaction(struct qcom_nand_controller *nandc)
> +{
> +	struct bam_transaction *bam_txn = nandc->bam_txn;
> +
> +	if (!nandc->props->is_bam)
> +		return;
> +
> +	bam_txn->bam_ce_pos = 0;
> +	bam_txn->bam_ce_start = 0;
> +	bam_txn->cmd_sgl_pos = 0;
> +	bam_txn->cmd_sgl_start = 0;
> +	bam_txn->tx_sgl_pos = 0;
> +	bam_txn->tx_sgl_start = 0;
> +	bam_txn->rx_sgl_pos = 0;
> +	bam_txn->rx_sgl_start = 0;
> +	bam_txn->last_data_desc = NULL;
> +	bam_txn->wait_second_completion = false;
> +
> +	sg_init_table(bam_txn->cmd_sgl, nandc->max_cwperpage *
> +		      QPIC_PER_CW_CMD_SGL);
> +	sg_init_table(bam_txn->data_sgl, nandc->max_cwperpage *
> +		      QPIC_PER_CW_DATA_SGL);
> +
> +	reinit_completion(&bam_txn->txn_done);
> +}
> +EXPORT_SYMBOL(clear_bam_transaction);
> +
> +/* one time setup of a few nand controller registers */

Does this needs to be shared?

> +int qcom_nandc_setup(struct qcom_nand_controller *nandc)
> +{
> +	u32 nand_ctrl;
> +
> +	/* kill onenand */
> +	if (!nandc->props->is_qpic)
> +		nandc_write(nandc, SFLASHC_BURST_CFG, 0);
> +
> +	if (!nandc->props->qpic_v2)
> +		nandc_write(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD),
> +			    NAND_DEV_CMD_VLD_VAL);
> +
> +	/* enable ADM or BAM DMA */
> +	if (nandc->props->is_bam) {
> +		nand_ctrl = nandc_read(nandc, NAND_CTRL);
> +
> +		/*
> +		 *NAND_CTRL is an operational registers, and CPU
> +		 * access to operational registers are read only
> +		 * in BAM mode. So update the NAND_CTRL register
> +		 * only if it is not in BAM mode. In most cases BAM
> +		 * mode will be enabled in bootloader
> +		 */
> +		if (!(nand_ctrl & BAM_MODE_EN))
> +			nandc_write(nandc, NAND_CTRL, nand_ctrl | BAM_MODE_EN);
> +	} else {
> +		nandc_write(nandc, NAND_FLASH_CHIP_SELECT, DM_EN);
> +	}
> +
> +	/* save the original values of these registers */
> +	if (!nandc->props->qpic_v2) {
> +		nandc->cmd1 = nandc_read(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD1));
> +		nandc->vld = NAND_DEV_CMD_VLD_VAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(qcom_nandc_setup);
> +
> +void qcom_nandc_unalloc(struct qcom_nand_controller *nandc)
> +{
> +	if (nandc->props->is_bam) {
> +		if (!dma_mapping_error(nandc->dev, nandc->reg_read_dma))
> +			dma_unmap_single(nandc->dev, nandc->reg_read_dma,
> +					 MAX_REG_RD *
> +					 sizeof(*nandc->reg_read_buf),
> +					 DMA_FROM_DEVICE);
> +
> +		if (nandc->tx_chan)
> +			dma_release_channel(nandc->tx_chan);
> +
> +		if (nandc->rx_chan)
> +			dma_release_channel(nandc->rx_chan);
> +
> +		if (nandc->cmd_chan)
> +			dma_release_channel(nandc->cmd_chan);
> +	} else {
> +		if (nandc->chan)
> +			dma_release_channel(nandc->chan);
> +	}
> +}
> +EXPORT_SYMBOL(qcom_nandc_unalloc);
> +
> +int qcom_nandc_alloc(struct qcom_nand_controller *nandc)
> +{
> +	int ret;
> +
> +	ret = dma_set_coherent_mask(nandc->dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(nandc->dev, "failed to set DMA mask\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * we use the internal buffer for reading ONFI params, reading small
> +	 * data like ID and status, and preforming read-copy-write operations

performing

Please run checkpatch.pl, I believe the tool should catch these.

> +	 * when writing to a codeword partially. 532 is the maximum possible
> +	 * size of a codeword for our nand controller
> +	 */
> +	nandc->buf_size = 532;
> +
> +	nandc->data_buffer = devm_kzalloc(nandc->dev, nandc->buf_size,
> +					GFP_KERNEL);
> +	if (!nandc->data_buffer)
> +		return -ENOMEM;
> +
> +	nandc->regs = devm_kzalloc(nandc->dev, sizeof(*nandc->regs),
> +					GFP_KERNEL);
> +	if (!nandc->regs)
> +		return -ENOMEM;
> +
> +	nandc->reg_read_buf = devm_kcalloc(nandc->dev,
> +				MAX_REG_RD, sizeof(*nandc->reg_read_buf),
> +				GFP_KERNEL);
> +	if (!nandc->reg_read_buf)
> +		return -ENOMEM;
> +
> +	if (nandc->props->is_bam) {

It's probably the 10th time I ask, but what the heck is this is_bam
parameter showing? It looks like you're handling two different
controllers in the same driver without using different compatibles.

> +		nandc->reg_read_dma =
> +			dma_map_single(nandc->dev, nandc->reg_read_buf,
> +				       MAX_REG_RD *
> +				       sizeof(*nandc->reg_read_buf),
> +				       DMA_FROM_DEVICE);
> +		if (dma_mapping_error(nandc->dev, nandc->reg_read_dma)) {
> +			dev_err(nandc->dev, "failed to DMA MAP reg buffer\n");
> +			return -EIO;
> +		}
> +
> +		nandc->tx_chan = dma_request_chan(nandc->dev, "tx");
> +		if (IS_ERR(nandc->tx_chan)) {
> +			ret = PTR_ERR(nandc->tx_chan);
> +			nandc->tx_chan = NULL;
> +			dev_err_probe(nandc->dev, ret,
> +				      "tx DMA channel request failed\n");
> +			goto unalloc;
> +		}
> +
> +		nandc->rx_chan = dma_request_chan(nandc->dev, "rx");
> +		if (IS_ERR(nandc->rx_chan)) {
> +			ret = PTR_ERR(nandc->rx_chan);
> +			nandc->rx_chan = NULL;
> +			dev_err_probe(nandc->dev, ret,
> +				      "rx DMA channel request failed\n");
> +			goto unalloc;
> +		}
> +
> +		nandc->cmd_chan = dma_request_chan(nandc->dev, "cmd");
> +		if (IS_ERR(nandc->cmd_chan)) {
> +			ret = PTR_ERR(nandc->cmd_chan);
> +			nandc->cmd_chan = NULL;
> +			dev_err_probe(nandc->dev, ret,
> +				      "cmd DMA channel request failed\n");
> +			goto unalloc;
> +		}
> +
> +		/*
> +		 * Initially allocate BAM transaction to read ONFI param page.
> +		 * After detecting all the devices, this BAM transaction will
> +		 * be freed and the next BAM tranasction will be allocated with

Typo

> +		 * maximum codeword size
> +		 */
> +		nandc->max_cwperpage = 1;
> +		nandc->bam_txn = alloc_bam_transaction(nandc);
> +		if (!nandc->bam_txn) {
> +			dev_err(nandc->dev,
> +				"failed to allocate bam transaction\n");
> +			ret = -ENOMEM;
> +			goto unalloc;
> +		}
> +	} else {
> +		nandc->chan = dma_request_chan(nandc->dev, "rxtx");
> +		if (IS_ERR(nandc->chan)) {
> +			ret = PTR_ERR(nandc->chan);
> +			nandc->chan = NULL;
> +			dev_err_probe(nandc->dev, ret,
> +				      "rxtx DMA channel request failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	INIT_LIST_HEAD(&nandc->desc_list);
> +	INIT_LIST_HEAD(&nandc->host_list);
> +
> +	return 0;
> +unalloc:
> +	qcom_nandc_unalloc(nandc);
> +	return ret;
> +}
> +EXPORT_SYMBOL(qcom_nandc_alloc);
> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h
> new file mode 100644
> index 000000000000..c461c1781330
> --- /dev/null
> +++ b/include/linux/mtd/nand-qpic-common.h

...

> +	u32 cmd1, vld;
> +	bool exec_opwrite;
> +};
> +
> +/*
> + * NAND chip structure

qcom_nand_host is a nand chip structure?

> + *
> + * @boot_partitions:		array of boot partitions where offset and size of the
> + *				boot partitions are stored
> + *
> + * @chip:			base NAND chip structure
> + * @node:			list node to add itself to host_list in
> + *				qcom_nand_controller
> + *
> + * @nr_boot_partitions:		count of the boot partitions where spare data is not
> + *				protected by ECC
> + *
> + * @cs:				chip select value for this chip
> + * @cw_size:			the number of bytes in a single step/codeword
> + *				of a page, consisting of all data, ecc, spare
> + *				and reserved bytes
> + * @cw_data:			the number of bytes within a codeword protected
> + *				by ECC
> + * @ecc_bytes_hw:		ECC bytes used by controller hardware for this
> + *				chip
> + *
> + * @last_command:		keeps track of last command on this chip. used
> + *				for reading correct status
> + *
> + * @cfg0, cfg1, cfg0_raw..:	NANDc register configurations needed for
> + *				ecc/non-ecc mode for the current nand flash
> + *				device
> + *
> + * @status:			value to be returned if NAND_CMD_STATUS command
> + *				is executed
> + * @codeword_fixup:		keep track of the current layout used by
> + *				the driver for read/write operation.
> + * @use_ecc:			request the controller to use ECC for the
> + *				upcoming read/write
> + * @bch_enabled:		flag to tell whether BCH ECC mode is used
> + */
> +struct qcom_nand_host {
> +	struct qcom_nand_boot_partition *boot_partitions;
> +
> +	struct nand_chip chip;
> +	struct list_head node;
> +
> +	int nr_boot_partitions;
> +
> +	int cs;
> +	int cw_size;
> +	int cw_data;
> +	int ecc_bytes_hw;
> +	int spare_bytes;
> +	int bbm_size;
> +
> +	int last_command;
> +
> +	u32 cfg0, cfg1;
> +	u32 cfg0_raw, cfg1_raw;
> +	u32 ecc_buf_cfg;
> +	u32 ecc_bch_cfg;
> +	u32 clrflashstatus;
> +	u32 clrreadstatus;
> +
> +	u8 status;
> +	bool codeword_fixup;
> +	bool use_ecc;
> +	bool bch_enabled;
> +};
> +

I would like to see how useful are all these exports. I believe the
current NAND controller driver already has some of these functions
defined, could you please move the functions rather than adding them
here without removing them in the other driver?

Thanks, Miquèl

[RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Md Sadre Alam]

Add qpic spi nand driver support for qcom soc.

Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
---
 drivers/spi/Kconfig          |   7 +
 drivers/spi/Makefile         |   1 +
 drivers/spi/spi-qpic-snand.c | 604 +++++++++++++++++++++++++++++++++++
 3 files changed, 612 insertions(+)
 create mode 100644 drivers/spi/spi-qpic-snand.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 35dbfacecf1c..8dc96bda8b17 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -206,6 +206,13 @@ config SPI_BCM_QSPI
 	  based platforms. This driver works for both SPI master for SPI NOR
 	  flash device as well as MSPI device.
 
+config SPI_QPIC_SNAND
+	tristate "QPIC SNAND controller"
+	default y
+	depends on ARCH_QCOM
+	help
+	  QPIC_SNAND(Quad SPI) driver for Qualcomm QPIC_SNAND controller.
+
 config SPI_BCMBCA_HSSPI
 	tristate "Broadcom BCMBCA HS SPI controller driver"
 	depends on ARCH_BCMBCA || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 4ff8d725ba5e..1ac3bac35007 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -153,6 +153,7 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
 obj-$(CONFIG_SPI_ZYNQ_QSPI)		+= spi-zynq-qspi.o
 obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
 obj-$(CONFIG_SPI_AMD)			+= spi-amd.o
+obj-$(CONFIG_SPI_QPIC_SNAND)            += spi-qpic-snand.o
 
 # SPI slave protocol handlers
 obj-$(CONFIG_SPI_SLAVE_TIME)		+= spi-slave-time.o
diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
new file mode 100644
index 000000000000..1c7957079741
--- /dev/null
+++ b/drivers/spi/spi-qpic-snand.c
@@ -0,0 +1,604 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ *
+ * Authors:
+ * 	Md Sadre Alam <quic_mdalam@quicinc.com>
+ * 	Sricharan R <quic_srichara@quicinc.com>
+ */
+
+#include <linux/mtd/spinand.h>
+#include <linux/mtd/nand-qpic-common.h>
+
+/* QSPI NAND config reg bits */
+#define LOAD_CLK_CNTR_INIT_EN   (1 << 28)
+#define CLK_CNTR_INIT_VAL_VEC   0x924
+#define FEA_STATUS_DEV_ADDR     0xc0
+#define SPI_CFG (1 << 0)
+#define SPI_NUM_ADDR    0xDA4DB
+#define SPI_WAIT_CNT        0x10
+#define QPIC_QSPI_NUM_CS	1
+#define SPI_TRANSFER_MODE_x1	(1 << 29)
+#define SPI_TRANSFER_MODE_x4	(3 << 29)
+#define SPI_WP			(1 << 28)
+#define SPI_HOLD		(1 << 27)
+#define QPIC_SET_FEATURE	(1 << 31)
+
+
+#define SPINAND_RESET		0xff
+#define SPINAND_READID		0x9f
+#define SPINAND_GET_FEATURE	0x0f
+#define SPINAND_SET_FEATURE	0x1f
+
+struct qpic_snand_xfer {
+	union {
+		const void *snand_tx_buf;
+		void *snand_rx_buf;
+	};
+	unsigned int snand_rem_bytes;
+	unsigned int snand_buswidth;
+};
+
+struct qpic_snand_op {
+	u32 cmd_reg;
+	u32 addr1_reg;
+	u32 addr2_reg;
+};
+
+static struct qcom_nand_controller *nand_to_qcom_snand(struct nand_device *nand)
+{
+	struct nand_ecc_engine *eng = nand->ecc.engine;
+
+	return container_of(eng, struct qcom_nand_controller, ecc_eng);
+}
+
+static int qcom_snand_init(struct qcom_nand_controller *snandc)
+{
+	u32 snand_cfg_val = 0x0;
+	int ret;
+
+	snand_cfg_val |= (LOAD_CLK_CNTR_INIT_EN | (CLK_CNTR_INIT_VAL_VEC << 16)
+			| (FEA_STATUS_DEV_ADDR << 8) | SPI_CFG);
+
+	nandc_set_reg(snandc, NAND_FLASH_SPI_CFG, 0);
+	nandc_set_reg(snandc, NAND_FLASH_SPI_CFG, snand_cfg_val);
+	nandc_set_reg(snandc, NAND_NUM_ADDR_CYCLES, SPI_NUM_ADDR);
+	nandc_set_reg(snandc, NAND_BUSY_CHECK_WAIT_CNT, SPI_WAIT_CNT);
+
+	write_reg_dma(snandc, NAND_FLASH_SPI_CFG, 1, 0);
+	write_reg_dma(snandc, NAND_FLASH_SPI_CFG, 1, 0);
+
+	snand_cfg_val &= ~LOAD_CLK_CNTR_INIT_EN;
+	nandc_set_reg(snandc, NAND_FLASH_SPI_CFG, snand_cfg_val);
+
+	write_reg_dma(snandc, NAND_FLASH_SPI_CFG, 1, 0);
+
+	write_reg_dma(snandc, NAND_NUM_ADDR_CYCLES, 1, 0);
+	write_reg_dma(snandc, NAND_BUSY_CHECK_WAIT_CNT, 1, NAND_BAM_NEXT_SGL);
+
+	ret = submit_descs(snandc);
+	if (ret)
+		dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n");
+
+	free_descs(snandc);
+
+	return 0;
+}
+
+static int qcom_snand_ooblayout_ecc(struct mtd_info *mtd, int section,
+				   struct mtd_oob_region *oobregion)
+{
+	struct nand_device *nand = mtd_to_nanddev(mtd);
+	struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
+	struct qpic_ecc *qecc = snandc->ecc;
+
+	if (section > 1)
+		return -ERANGE;
+
+	if (!section) {
+		oobregion->length = (qecc->bytes * (qecc->steps - 1)) +
+			qecc->bbm_size;
+		oobregion->offset = 0;
+	} else {
+		 oobregion->length = qecc->ecc_bytes_hw + qecc->spare_bytes;
+		 oobregion->offset = mtd->oobsize - oobregion->length;
+	}
+
+	return 0;
+}
+
+static int qcom_snand_ooblayout_free(struct mtd_info *mtd, int section,
+				    struct mtd_oob_region *oobregion)
+{
+	struct nand_device *nand = mtd_to_nanddev(mtd);
+	struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
+	struct qpic_ecc *qecc = snandc->ecc;
+
+	if (section)
+		return -ERANGE;
+
+	oobregion->length = qecc->steps * 4;
+	oobregion->offset = ((qecc->steps - 1) * qecc->bytes) + qecc->bbm_size;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops qcom_snand_ooblayout = {
+	.ecc = qcom_snand_ooblayout_ecc,
+	.free = qcom_snand_ooblayout_free,
+};
+
+static int qpic_snand_ecc_init_ctx(struct nand_device *nand)
+{
+	struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
+	struct nand_ecc_props *conf = &nand->ecc.ctx.conf;
+	struct nand_ecc_props *reqs = &nand->ecc.requirements;
+	struct nand_ecc_props *user = &nand->ecc.user_conf;
+	struct mtd_info *mtd = nanddev_to_mtd(nand);
+	int step_size = 0, strength = 0, desired_correction = 0, steps;
+	bool ecc_user = false;
+	int ret;
+	struct qpic_ecc *ecc_cfg;
+
+	ecc_cfg = kzalloc(sizeof(*ecc_cfg), GFP_KERNEL);
+	if (!ecc_cfg)
+		return -ENOMEM;
+
+	nand->ecc.ctx.priv = ecc_cfg;
+
+	if (user->step_size && user->strength) {
+		step_size = user->step_size;
+		strength = user->strength;
+		ecc_user = true;
+	} else if (reqs->step_size && reqs->strength) {
+		step_size = reqs->step_size;
+		strength = reqs->strength;
+	}
+
+	if (step_size && strength) {
+		steps = mtd->writesize / step_size;
+		desired_correction = steps * strength;
+	}
+
+	qcom_ecc_config(ecc_cfg, 4, false);
+
+	ecc_cfg->bytes = ecc_cfg->ecc_bytes_hw + ecc_cfg->spare_bytes + ecc_cfg->bbm_size;
+
+	ecc_cfg->steps = 4;
+	ecc_cfg->strength = 4;
+	ecc_cfg->step_size = 512;
+
+	mtd_set_ooblayout(mtd, &qcom_snand_ooblayout);
+
+	conf->step_size = ecc_cfg->step_size;
+	conf->strength = ecc_cfg->strength;
+
+	if (ecc_cfg->strength < strength)
+		dev_warn(snandc->dev, "Unable to fulfill ECC requirements of %u bits.\n",
+			strength);
+
+	dev_info(snandc->dev, "ECC strength: %u bits per %u bytes\n",
+		ecc_cfg->strength,ecc_cfg->step_size);
+
+	return 0;
+}
+
+static void qpic_snand_ecc_cleanup_ctx(struct nand_device *nand)
+{
+	struct qcom_ecc *ecc_cfg = nand_to_ecc_ctx(nand);
+
+	kfree(ecc_cfg);
+}
+
+static int qpic_snand_ecc_prepare_io_req(struct nand_device *nand,
+					struct nand_page_io_req *req)
+{
+	struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
+	struct qpic_ecc *ecc_cfg = nand_to_ecc_ctx(nand);
+	int ret;
+
+	snandc->ecc = ecc_cfg;
+	return 0;
+}
+
+static int qpic_snand_ecc_finish_io_req(struct nand_device *nand,
+				       struct nand_page_io_req *req)
+{
+	struct qcom_nand_controller *snandc = nand_to_qcom_snand(nand);
+	struct mtd_info *mtd = nanddev_to_mtd(nand);
+
+	snandc->ecc = NULL;
+	if ((req->mode == MTD_OPS_RAW) || (req->type != NAND_PAGE_READ))
+		return 0;
+
+	if (snandc->ecc_stats.failed)
+		mtd->ecc_stats.failed += snandc->ecc_stats.failed;
+	mtd->ecc_stats.corrected += snandc->ecc_stats.corrected;
+
+	return snandc->ecc_stats.failed ? -EBADMSG : snandc->ecc_stats.bitflips;
+}
+
+static struct nand_ecc_engine_ops qcom_snand_ecc_engine_ops = {
+	.init_ctx = qpic_snand_ecc_init_ctx,
+	.cleanup_ctx = qpic_snand_ecc_cleanup_ctx,
+	.prepare_io_req = qpic_snand_ecc_prepare_io_req,
+	.finish_io_req = qpic_snand_ecc_finish_io_req,
+};
+
+static int qpic_snand_read_page(struct qcom_nand_controller *snandc,
+				const struct spi_mem_op *op)
+{
+	return 0;
+}
+
+static int qpic_snand_write_page(struct qcom_nand_controller *snandc,
+				const struct spi_mem_op *op)
+{
+	return 0;
+}
+
+static u32 qpic_snand_cmd_mapping(u32 opcode)
+{
+	u32 cmd = 0x0;
+	switch (opcode) {
+
+		case SPINAND_RESET:
+			cmd = (SPI_WP | SPI_HOLD | SPI_TRANSFER_MODE_x1 |
+				OP_RESET_DEVICE);
+			break;
+		case SPINAND_READID:
+			cmd = (SPI_WP | SPI_HOLD | SPI_TRANSFER_MODE_x1 |
+				OP_FETCH_ID);
+			break;
+		case SPINAND_GET_FEATURE:
+			cmd = (SPI_TRANSFER_MODE_x1 | SPI_WP |
+				SPI_HOLD | ACC_FEATURE);
+			break;
+		case SPINAND_SET_FEATURE:
+			cmd = (SPI_TRANSFER_MODE_x1 | SPI_WP |
+				SPI_HOLD | ACC_FEATURE |
+				QPIC_SET_FEATURE);
+			break;
+		default:
+			break;
+	}
+
+	return cmd;
+}
+
+static int qpic_snand_send_cmdaddr(const struct spi_mem_op *op,
+			struct qcom_nand_controller *snandc)
+{
+	struct qpic_snand_op s_op = {};
+	u32 cmd;
+	int ret, i;
+
+	cmd = qpic_snand_cmd_mapping(op->cmd.opcode);
+
+	s_op.cmd_reg = cmd;
+
+	for (i = 0; i < op->addr.nbytes; i++) {
+		s_op.addr1_reg = op->addr.val;
+		s_op.addr2_reg = 0;
+	}
+
+	snandc->buf_count = 0;
+	snandc->buf_start = 0;
+	clear_read_regs(snandc);
+	clear_bam_transaction(snandc);
+
+	nandc_set_reg(snandc, NAND_FLASH_CMD, s_op.cmd_reg);
+	nandc_set_reg(snandc, NAND_EXEC_CMD, 0x1);
+	nandc_set_reg(snandc, NAND_ADDR0, s_op.addr1_reg);
+	nandc_set_reg(snandc, NAND_ADDR1, s_op.addr2_reg);
+
+	write_reg_dma(snandc, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL);
+	write_reg_dma(snandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
+
+	ret = submit_descs(snandc);
+	if (ret)
+		dev_err(snandc->dev, "failure in sbumitting cmd descriptor\n");
+
+	free_descs(snandc);
+
+	return ret;
+}
+
+static int qpic_snand_io_op(struct qcom_nand_controller *snandc,
+		const struct spi_mem_op *op)
+{
+	int ret, val;
+	bool sub_desc = false;
+
+	ret = qpic_snand_send_cmdaddr(op, snandc);
+	if (ret)
+		return ret;
+
+	snandc->buf_count = 0;
+	snandc->buf_start = 0;
+	clear_read_regs(snandc);
+	clear_bam_transaction(snandc);
+
+	if (op->cmd.opcode == SPINAND_READID) {
+		snandc->buf_count = 4;
+		read_reg_dma(snandc, NAND_READ_ID, 1,
+			NAND_BAM_NEXT_SGL);
+		sub_desc = true;
+	}
+
+	if (sub_desc) {
+		ret = submit_descs(snandc);
+		if (ret)
+			dev_err(snandc->dev, "failure in submitting descriptor\n");
+
+		free_descs(snandc);
+		nandc_read_buffer_sync(snandc, true);
+		memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count);
+
+		return ret;
+	}
+
+	if (op->cmd.opcode == SPINAND_GET_FEATURE) {
+
+		snandc->buf_count = 4;
+		read_reg_dma(snandc, NAND_FLASH_FEATURES, 1,
+				NAND_BAM_NEXT_SGL);
+
+		ret = submit_descs(snandc);
+		if (ret)
+			dev_err(snandc->dev, "failure in submitting descriptor\n");
+
+		free_descs(snandc);
+		nandc_read_buffer_sync(snandc, true);
+
+		val = le32_to_cpu(*(__le32 *)snandc->reg_read_buf);
+		val >>= 8;
+		memcpy(op->data.buf.in, &val, snandc->buf_count);
+	}
+
+	if (op->cmd.opcode == SPINAND_SET_FEATURE) {
+
+		nandc_set_reg(snandc, NAND_FLASH_FEATURES, *(u32 *)op->data.buf.out);
+		write_reg_dma(snandc, NAND_FLASH_FEATURES, 1, NAND_BAM_NEXT_SGL);
+
+		ret = submit_descs(snandc);
+		if (ret)
+			dev_err(snandc->dev, "failure in submitting descriptor\n");
+
+		free_descs(snandc);
+	}
+
+	return ret;
+}
+
+static bool qpic_snand_is_page_op(const struct spi_mem_op *op)
+{
+	if (op->addr.nbytes != 3)
+		return false;
+
+	if (op->addr.buswidth != 1 && op->addr.buswidth != 2 &&
+	    op->addr.buswidth != 4)
+		return false;
+
+	if (op->data.dir == SPI_MEM_DATA_IN) {
+		/* quad io */
+		if ((op->addr.buswidth == 4 || op->addr.buswidth == 1) &&
+		     op->data.buswidth == 4)
+			return true;
+
+		/* standard spi */
+		if (op->addr.buswidth == 1 && op->data.buswidth == 1)
+			return true;
+
+	} else if (op->data.dir == SPI_MEM_DATA_OUT) {
+		/* quad io*/
+		if (op->addr.buswidth == 1 && op->data.buswidth == 4)
+			return true;
+
+		/* standard spi */
+		if (op->addr.buswidth == 1 && op->data.buswidth == 1)
+			return true;
+	}
+
+	return false;
+}
+
+static bool qpic_snand_supports_op(struct spi_mem *mem,
+				   const struct spi_mem_op *op)
+{
+	if (!spi_mem_default_supports_op(mem, op))
+		return false;
+
+	if (op->cmd.nbytes != 1 || op->cmd.buswidth != 1)
+		return false;
+
+	return ((op->addr.nbytes == 0 || op->addr.buswidth == 1) &&
+		(op->dummy.nbytes == 0 || op->dummy.buswidth == 1) &&
+		(op->data.nbytes == 0 || op->data.buswidth == 1));
+}
+
+static int qpic_snand_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+	struct qcom_nand_controller *snandc = spi_controller_get_devdata(mem->spi->master);
+	dev_dbg(snandc->dev, "OP %02x ADDR %08llX@%d:%u DATA %d:%u", op->cmd.opcode,
+		op->addr.val, op->addr.buswidth, op->addr.nbytes,
+		op->data.buswidth, op->data.nbytes);
+
+	/*
+	 * Check for page ops or normal ops
+	 */
+	if (qpic_snand_is_page_op(op)) {
+		if (op->data.dir == SPI_MEM_DATA_IN)
+			return qpic_snand_read_page(snandc, op);
+		else
+			return qpic_snand_write_page(snandc, op);
+	} else {
+		return qpic_snand_io_op(snandc, op);
+	}
+
+	return 0;
+}
+
+static const struct spi_controller_mem_ops qcom_spi_mem_ops = {
+	.supports_op = qpic_snand_supports_op,
+	.exec_op = qpic_snand_exec_op,
+};
+
+static const struct spi_controller_mem_caps qcom_snand_mem_caps = {
+	.ecc = true,
+};
+
+static int qcom_snand_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct spi_controller *ctlr;
+	struct qcom_nand_controller *snandc;
+	struct resource *res;
+	const void *dev_data;
+	int ret;
+
+	ctlr = devm_spi_alloc_master(dev, sizeof(*snandc));
+	if (!ctlr)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ctlr);
+
+	snandc = spi_controller_get_devdata(ctlr);
+
+	snandc->ctlr = ctlr;
+	snandc->dev = dev;
+
+	snandc->ecc = of_qpic_ecc_get(np);
+	if (IS_ERR(snandc->ecc))
+		return PTR_ERR(snandc->ecc);
+	else if (!snandc->ecc)
+		return -ENODEV;
+
+	dev_data = of_device_get_match_data(dev);
+	if (!dev_data) {
+		dev_err(&pdev->dev, "failed to get device data\n");
+		return -ENODEV;
+	}
+
+	snandc->props = dev_data;
+	snandc->dev = &pdev->dev;
+
+	snandc->core_clk = devm_clk_get(dev, "core");
+	if (IS_ERR(snandc->core_clk))
+		return PTR_ERR(snandc->core_clk);
+
+	snandc->core_clk = devm_clk_get(dev, "aon");
+	if (IS_ERR(snandc->core_clk))
+		return PTR_ERR(snandc->core_clk);
+
+	snandc->core_clk = devm_clk_get(dev, "io_macro");
+	if (IS_ERR(snandc->core_clk))
+		return PTR_ERR(snandc->core_clk);
+
+	snandc->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(snandc->base))
+		return PTR_ERR(snandc->base);
+
+	snandc->base_phys = res->start;
+	snandc->base_dma = dma_map_resource(dev, res->start,
+					   resource_size(res),
+					   DMA_BIDIRECTIONAL, 0);
+	if (dma_mapping_error(dev, snandc->base_dma))
+		return -ENXIO;
+
+	ret = clk_prepare_enable(snandc->core_clk);
+	if (ret)
+		goto err_core_clk;
+
+	ret = clk_prepare_enable(snandc->aon_clk);
+	if (ret)
+		goto err_aon_clk;
+
+	ret = clk_prepare_enable(snandc->iomacro_clk);
+	if (ret)
+		goto err_snandc_alloc;
+
+	ret = qcom_nandc_alloc(snandc);
+	if (ret)
+		goto err_snandc_alloc;
+
+	ret = qcom_snand_init(snandc);
+	if (ret)
+		goto err_init;
+
+	// setup ECC engine
+	snandc->ecc_eng.dev = &pdev->dev;
+	snandc->ecc_eng.integration = NAND_ECC_ENGINE_INTEGRATION_PIPELINED;
+	snandc->ecc_eng.ops = &qcom_snand_ecc_engine_ops;
+	snandc->ecc_eng.priv = snandc;
+
+	ret = nand_ecc_register_on_host_hw_engine(&snandc->ecc_eng);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register ecc engine.\n");
+		goto err_init;
+	}
+
+	ctlr->num_chipselect = QPIC_QSPI_NUM_CS;
+	ctlr->mem_ops = &qcom_spi_mem_ops;
+	ctlr->mem_caps = &qcom_snand_mem_caps;
+	ctlr->dev.of_node = pdev->dev.of_node;
+	ctlr->mode_bits = SPI_TX_DUAL | SPI_RX_DUAL |
+			    SPI_TX_QUAD | SPI_RX_QUAD;
+
+	ret = spi_register_master(ctlr);
+	if (ret) {
+		dev_err(&pdev->dev, "spi_register_controller failed.\n");
+		goto err_init;
+	}
+
+	return 0;
+err_init:
+	qcom_nandc_unalloc(snandc);
+err_snandc_alloc:
+	clk_disable_unprepare(snandc->aon_clk);
+err_aon_clk:
+	clk_disable_unprepare(snandc->core_clk);
+err_core_clk:
+	dma_unmap_resource(dev, res->start, resource_size(res),
+			   DMA_BIDIRECTIONAL, 0);
+
+	return ret;
+}
+
+static int qcom_snand_remove(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
+	spi_unregister_master(ctlr);
+
+	return 0;
+}
+
+static const struct qcom_nandc_props ipq9574_snandc_props = {
+	.dev_cmd_reg_start = 0x7000,
+	.is_bam = true,
+	.qpic_v2 = true,
+};
+
+static const struct of_device_id qcom_snandc_of_match[] = {
+	{
+		.compatible = "qcom,ipq9574-nand",
+		.data = &ipq9574_snandc_props,
+	},
+	{}
+}
+MODULE_DEVICE_TABLE(of, qcom_snandc_of_match);
+
+static struct platform_driver qcom_snand_driver = {
+	.driver = {
+		.name		= "qcom_snand",
+		.of_match_table = qcom_snandc_of_match,
+	},
+	.probe = qcom_snand_probe,
+	.remove = qcom_snand_remove,
+};
+module_platform_driver(qcom_snand_driver);
+
+MODULE_DESCRIPTION("SPI driver for QPIC QSPI cores");
+MODULE_AUTHOR("Md Sadre Alam <quic_mdalam@quicinc.com>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 5698 bytes --]

On Tue, Oct 31, 2023 at 05:33:06PM +0530, Md Sadre Alam wrote:

> +config SPI_QPIC_SNAND
> +	tristate "QPIC SNAND controller"
> +	default y
> +	depends on ARCH_QCOM
> +	help
> +	  QPIC_SNAND(Quad SPI) driver for Qualcomm QPIC_SNAND controller.
> +

I don't see any build dependencies on anything QC specific so please add
an || COMPILE_TEST here, this makes it much easier to do generic changes
without having to build some specific config.

> +++ b/drivers/spi/Makefile
> @@ -153,6 +153,7 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
>  obj-$(CONFIG_SPI_ZYNQ_QSPI)		+= spi-zynq-qspi.o
>  obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
>  obj-$(CONFIG_SPI_AMD)			+= spi-amd.o
> +obj-$(CONFIG_SPI_QPIC_SNAND)            += spi-qpic-snand.o

Please keep this alphabetically sorted (there are some mistakes there
but no need to add to them).

> + * 	Sricharan R <quic_srichara@quicinc.com>
> + */
> +
> +#include <linux/mtd/spinand.h>
> +#include <linux/mtd/nand-qpic-common.h>
> +

This should be including the SPI API, and other API headers that are
used directly like the platform and clock APIs.

> +static int qcom_snand_init(struct qcom_nand_controller *snandc)
> +{
> +	u32 snand_cfg_val = 0x0;
> +	int ret;

...

> +	ret = submit_descs(snandc);
> +	if (ret)
> +		dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n");
> +
> +	free_descs(snandc);

This seems to be doing a bit more than I would expect an init function
to, and it's very surprising to see the descriptors freed immediately
after something called a submit (which suggests that the descriptors are
still in flight).

> +static int qpic_snand_read_page(struct qcom_nand_controller *snandc,
> +				const struct spi_mem_op *op)
> +{
> +	return 0;
> +}
> +
> +static int qpic_snand_write_page(struct qcom_nand_controller *snandc,
> +				const struct spi_mem_op *op)
> +{
> +	return 0;
> +}

...

> +static int qpic_snand_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct qcom_nand_controller *snandc = spi_controller_get_devdata(mem->spi->master);
> +	dev_dbg(snandc->dev, "OP %02x ADDR %08llX@%d:%u DATA %d:%u", op->cmd.opcode,
> +		op->addr.val, op->addr.buswidth, op->addr.nbytes,
> +		op->data.buswidth, op->data.nbytes);
> +
> +	/*
> +	 * Check for page ops or normal ops
> +	 */
> +	if (qpic_snand_is_page_op(op)) {
> +		if (op->data.dir == SPI_MEM_DATA_IN)
> +			return qpic_snand_read_page(snandc, op);
> +		else
> +			return qpic_snand_write_page(snandc, op);

So does the device actually support page operations?  The above looks
like the driver will silently noop them.

> +	snandc->base_phys = res->start;
> +	snandc->base_dma = dma_map_resource(dev, res->start,
> +					   resource_size(res),
> +					   DMA_BIDIRECTIONAL, 0);
> +	if (dma_mapping_error(dev, snandc->base_dma))
> +		return -ENXIO;
> +
> +	ret = clk_prepare_enable(snandc->core_clk);
> +	if (ret)
> +		goto err_core_clk;

The DMA mapping and clock enables only get undone in error handling,
they're not undone in the normal device release path.

> +
> +	ret = clk_prepare_enable(snandc->aon_clk);
> +	if (ret)
> +		goto err_aon_clk;
> +
> +	ret = clk_prepare_enable(snandc->iomacro_clk);
> +	if (ret)
> +		goto err_snandc_alloc;
> +
> +	ret = qcom_nandc_alloc(snandc);
> +	if (ret)
> +		goto err_snandc_alloc;
> +
> +	ret = qcom_snand_init(snandc);
> +	if (ret)
> +		goto err_init;
> +
> +	// setup ECC engine
> +	snandc->ecc_eng.dev = &pdev->dev;
> +	snandc->ecc_eng.integration = NAND_ECC_ENGINE_INTEGRATION_PIPELINED;
> +	snandc->ecc_eng.ops = &qcom_snand_ecc_engine_ops;
> +	snandc->ecc_eng.priv = snandc;
> +
> +	ret = nand_ecc_register_on_host_hw_engine(&snandc->ecc_eng);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register ecc engine.\n");
> +		goto err_init;
> +	}
> +
> +	ctlr->num_chipselect = QPIC_QSPI_NUM_CS;
> +	ctlr->mem_ops = &qcom_spi_mem_ops;
> +	ctlr->mem_caps = &qcom_snand_mem_caps;
> +	ctlr->dev.of_node = pdev->dev.of_node;
> +	ctlr->mode_bits = SPI_TX_DUAL | SPI_RX_DUAL |
> +			    SPI_TX_QUAD | SPI_RX_QUAD;
> +
> +	ret = spi_register_master(ctlr);
> +	if (ret) {
> +		dev_err(&pdev->dev, "spi_register_controller failed.\n");
> +		goto err_init;
> +	}
> +
> +	return 0;
> +err_init:
> +	qcom_nandc_unalloc(snandc);
> +err_snandc_alloc:
> +	clk_disable_unprepare(snandc->aon_clk);
> +err_aon_clk:
> +	clk_disable_unprepare(snandc->core_clk);
> +err_core_clk:
> +	dma_unmap_resource(dev, res->start, resource_size(res),
> +			   DMA_BIDIRECTIONAL, 0);
> +
> +	return ret;
> +}
> +
> +static int qcom_snand_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	spi_unregister_master(ctlr);
> +
> +	return 0;
> +}
> +
> +static const struct qcom_nandc_props ipq9574_snandc_props = {
> +	.dev_cmd_reg_start = 0x7000,
> +	.is_bam = true,
> +	.qpic_v2 = true,
> +};
> +
> +static const struct of_device_id qcom_snandc_of_match[] = {
> +	{
> +		.compatible = "qcom,ipq9574-nand",
> +		.data = &ipq9574_snandc_props,
> +	},
> +	{}
> +}
> +MODULE_DEVICE_TABLE(of, qcom_snandc_of_match);
> +
> +static struct platform_driver qcom_snand_driver = {
> +	.driver = {
> +		.name		= "qcom_snand",
> +		.of_match_table = qcom_snandc_of_match,
> +	},
> +	.probe = qcom_snand_probe,
> +	.remove = qcom_snand_remove,
> +};
> +module_platform_driver(qcom_snand_driver);
> +
> +MODULE_DESCRIPTION("SPI driver for QPIC QSPI cores");
> +MODULE_AUTHOR("Md Sadre Alam <quic_mdalam@quicinc.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Md Sadre Alam]

On 10/31/2023 7:53 PM, Mark Brown wrote:
> On Tue, Oct 31, 2023 at 05:33:06PM +0530, Md Sadre Alam wrote:
> 
>> +config SPI_QPIC_SNAND
>> +	tristate "QPIC SNAND controller"
>> +	default y
>> +	depends on ARCH_QCOM
>> +	help
>> +	  QPIC_SNAND(Quad SPI) driver for Qualcomm QPIC_SNAND controller.
>> +
> 
> I don't see any build dependencies on anything QC specific so please add
> an || COMPILE_TEST here, this makes it much easier to do generic changes
> without having to build some specific config.

Ok

> 
>> +++ b/drivers/spi/Makefile
>> @@ -153,6 +153,7 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA)		+= spi-xtensa-xtfpga.o
>>   obj-$(CONFIG_SPI_ZYNQ_QSPI)		+= spi-zynq-qspi.o
>>   obj-$(CONFIG_SPI_ZYNQMP_GQSPI)		+= spi-zynqmp-gqspi.o
>>   obj-$(CONFIG_SPI_AMD)			+= spi-amd.o
>> +obj-$(CONFIG_SPI_QPIC_SNAND)            += spi-qpic-snand.o
> 
> Please keep this alphabetically sorted (there are some mistakes there
> but no need to add to them).

Ok

> 
>> + * 	Sricharan R <quic_srichara@quicinc.com>
>> + */
>> +
>> +#include <linux/mtd/spinand.h>
>> +#include <linux/mtd/nand-qpic-common.h>
>> +
> 
> This should be including the SPI API, and other API headers that are
> used directly like the platform and clock APIs.
>

Ok

>> +static int qcom_snand_init(struct qcom_nand_controller *snandc)
>> +{
>> +	u32 snand_cfg_val = 0x0;
>> +	int ret;
> 
> ...
> 
>> +	ret = submit_descs(snandc);
>> +	if (ret)
>> +		dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n");
>> +
>> +	free_descs(snandc);
> 
> This seems to be doing a bit more than I would expect an init function
> to, and it's very surprising to see the descriptors freed immediately
> after something called a submit (which suggests that the descriptors are
> still in flight).
>

Our controller supports only bam mode , that means for writing/reading even
single register we have to use bam.
submit_descs() is synchronous so I/O is complete when it returns.
Hence freeing the descriptor after it.


>> +static int qpic_snand_read_page(struct qcom_nand_controller *snandc,
>> +				const struct spi_mem_op *op)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int qpic_snand_write_page(struct qcom_nand_controller *snandc,
>> +				const struct spi_mem_op *op)
>> +{
>> +	return 0;
>> +}
> 
> ...
> 
>> +static int qpic_snand_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>> +{
>> +	struct qcom_nand_controller *snandc = spi_controller_get_devdata(mem->spi->master);
>> +	dev_dbg(snandc->dev, "OP %02x ADDR %08llX@%d:%u DATA %d:%u", op->cmd.opcode,
>> +		op->addr.val, op->addr.buswidth, op->addr.nbytes,
>> +		op->data.buswidth, op->data.nbytes);
>> +
>> +	/*
>> +	 * Check for page ops or normal ops
>> +	 */
>> +	if (qpic_snand_is_page_op(op)) {
>> +		if (op->data.dir == SPI_MEM_DATA_IN)
>> +			return qpic_snand_read_page(snandc, op);
>> +		else
>> +			return qpic_snand_write_page(snandc, op);
> 
> So does the device actually support page operations?  The above looks
> like the driver will silently noop them.

Sorry It was to do item and I missed to mention that in commit log.
Will add in V1.

> 
>> +	snandc->base_phys = res->start;
>> +	snandc->base_dma = dma_map_resource(dev, res->start,
>> +					   resource_size(res),
>> +					   DMA_BIDIRECTIONAL, 0);
>> +	if (dma_mapping_error(dev, snandc->base_dma))
>> +		return -ENXIO;
>> +
>> +	ret = clk_prepare_enable(snandc->core_clk);
>> +	if (ret)
>> +		goto err_core_clk;
> 
> The DMA mapping and clock enables only get undone in error handling,
> they're not undone in the normal device release path.

Will fix in V1

Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On Fri, Nov 03, 2023 at 04:50:54PM +0530, Md Sadre Alam wrote:
> On 10/31/2023 7:53 PM, Mark Brown wrote:
> > On Tue, Oct 31, 2023 at 05:33:06PM +0530, Md Sadre Alam wrote:

> > > +	ret = submit_descs(snandc);
> > > +	if (ret)
> > > +		dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n");
> > > +
> > > +	free_descs(snandc);

> > This seems to be doing a bit more than I would expect an init function
> > to, and it's very surprising to see the descriptors freed immediately
> > after something called a submit (which suggests that the descriptors are
> > still in flight).

> Our controller supports only bam mode , that means for writing/reading even
> single register we have to use bam.
> submit_descs() is synchronous so I/O is complete when it returns.
> Hence freeing the descriptor after it.

That seems like the BAM API is very confusing and error prone.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Md Sadre Alam]

On 11/3/2023 6:17 PM, Mark Brown wrote:
> On Fri, Nov 03, 2023 at 04:50:54PM +0530, Md Sadre Alam wrote:
>> On 10/31/2023 7:53 PM, Mark Brown wrote:
>>> On Tue, Oct 31, 2023 at 05:33:06PM +0530, Md Sadre Alam wrote:
> 
>>>> +	ret = submit_descs(snandc);
>>>> +	if (ret)
>>>> +		dev_err(snandc->dev, "failure in sbumitting spiinit descriptor\n");
>>>> +
>>>> +	free_descs(snandc);
> 
>>> This seems to be doing a bit more than I would expect an init function
>>> to, and it's very surprising to see the descriptors freed immediately
>>> after something called a submit (which suggests that the descriptors are
>>> still in flight).
> 
>> Our controller supports only bam mode , that means for writing/reading even
>> single register we have to use bam.
>> submit_descs() is synchronous so I/O is complete when it returns.
>> Hence freeing the descriptor after it.
> 
> That seems like the BAM API is very confusing and error prone.

Sorry for late reply
Yeah, By HW itself its complex due to security reason. We are trying
to write a BAM common API interface for all the driver which will use BAM.

Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Krzysztof Kozlowski]

On 31/10/2023 13:03, Md Sadre Alam wrote:
> Add qpic spi nand driver support for qcom soc.

What is "qcom soc"? Did you mean Qualcomm and SoC?

> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> ---
>  drivers/spi/Kconfig          |   7 +
>  drivers/spi/Makefile         |   1 +
>  drivers/spi/spi-qpic-snand.c | 604 +++++++++++++++++++++++++++++++++++
>  3 files changed, 612 insertions(+)
>  create mode 100644 drivers/spi/spi-qpic-snand.c
> 

...

> +
> +static int qcom_snand_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	spi_unregister_master(ctlr);
> +
> +	return 0;
> +}
> +
> +static const struct qcom_nandc_props ipq9574_snandc_props = {
> +	.dev_cmd_reg_start = 0x7000,
> +	.is_bam = true,
> +	.qpic_v2 = true,
> +};
> +
> +static const struct of_device_id qcom_snandc_of_match[] = {
> +	{
> +		.compatible = "qcom,ipq9574-nand",

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.

Best regards,
Krzysztof

Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Md Sadre Alam]

On 10/31/2023 10:43 PM, Krzysztof Kozlowski wrote:
> On 31/10/2023 13:03, Md Sadre Alam wrote:
>> Add qpic spi nand driver support for qcom soc.
> 
> What is "qcom soc"? Did you mean Qualcomm and SoC?

Yes Qualcomm SoC

> 
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
>> ---
>>   drivers/spi/Kconfig          |   7 +
>>   drivers/spi/Makefile         |   1 +
>>   drivers/spi/spi-qpic-snand.c | 604 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 612 insertions(+)
>>   create mode 100644 drivers/spi/spi-qpic-snand.c
>>
> 
> ...
> 
>> +
>> +static int qcom_snand_remove(struct platform_device *pdev)
>> +{
>> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
>> +	spi_unregister_master(ctlr);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct qcom_nandc_props ipq9574_snandc_props = {
>> +	.dev_cmd_reg_start = 0x7000,
>> +	.is_bam = true,
>> +	.qpic_v2 = true,
>> +};
>> +
>> +static const struct of_device_id qcom_snandc_of_match[] = {
>> +	{
>> +		.compatible = "qcom,ipq9574-nand",
> 
> 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.

Ok
> 
> Best regards,
> Krzysztof
> 

Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Krzysztof Kozlowski]

On 03/11/2023 13:13, Md Sadre Alam wrote:
> 
> 
> On 10/31/2023 10:43 PM, Krzysztof Kozlowski wrote:
>> On 31/10/2023 13:03, Md Sadre Alam wrote:
>>> Add qpic spi nand driver support for qcom soc.
>>
>> What is "qcom soc"? Did you mean Qualcomm and SoC?
> 
> Yes Qualcomm SoC
> 

Then please use full sentences and full names with proper capitalization
of letters for acronyms.

Best regards,
Krzysztof

Re: [RFC PATCH 4/5] spi: qpic: Add support for qpic spi nand driver

[Md Sadre Alam]

On 10/31/2023 10:43 PM, Krzysztof Kozlowski wrote:
> On 31/10/2023 13:03, Md Sadre Alam wrote:
>> Add qpic spi nand driver support for qcom soc.
> 
> What is "qcom soc"? Did you mean Qualcomm and SoC?

Yes Qualcomm SoC

> 
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
>> ---
>>   drivers/spi/Kconfig          |   7 +
>>   drivers/spi/Makefile         |   1 +
>>   drivers/spi/spi-qpic-snand.c | 604 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 612 insertions(+)
>>   create mode 100644 drivers/spi/spi-qpic-snand.c
>>
> 
> ...
> 
>> +
>> +static int qcom_snand_remove(struct platform_device *pdev)
>> +{
>> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
>> +	spi_unregister_master(ctlr);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct qcom_nandc_props ipq9574_snandc_props = {
>> +	.dev_cmd_reg_start = 0x7000,
>> +	.is_bam = true,
>> +	.qpic_v2 = true,
>> +};
>> +
>> +static const struct of_device_id qcom_snandc_of_match[] = {
>> +	{
>> +		.compatible = "qcom,ipq9574-nand",
> 
> 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.

Ok
> 
> Best regards,
> Krzysztof
> 

[RFC PATCH 5/5] arm64: dts: qcom: ipq9574: Add support for SPI nand

[Md Sadre Alam]

Add support for QPIC SPI NAND for IPQ9574

Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 56 ++++++++++-----------
 arch/arm64/boot/dts/qcom/ipq9574.dtsi       | 30 ++++++++++-
 2 files changed, 57 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
index 1bb8d96c9a82..5e4200edb873 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
+++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
@@ -15,48 +15,48 @@ / {
 	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
 };
 
-&sdhc_1 {
-	pinctrl-0 = <&sdc_default_state>;
-	pinctrl-names = "default";
-	mmc-ddr-1_8v;
-	mmc-hs200-1_8v;
-	mmc-hs400-1_8v;
-	mmc-hs400-enhanced-strobe;
-	max-frequency = <384000000>;
-	bus-width = <8>;
-	status = "okay";
-};
-
 &tlmm {
-	sdc_default_state: sdc-default-state {
-		clk-pins {
+	qspi_nand_pins: qspi_nand_pins {
+		spi_clock {
 			pins = "gpio5";
-			function = "sdc_clk";
+			function = "qspi_clk";
 			drive-strength = <8>;
 			bias-disable;
 		};
 
-		cmd-pins {
+		qspi_cs {
 			pins = "gpio4";
-			function = "sdc_cmd";
+			function = "qspi_cs";
 			drive-strength = <8>;
 			bias-pull-up;
 		};
 
-		data-pins {
-			pins = "gpio0", "gpio1", "gpio2",
-			       "gpio3", "gpio6", "gpio7",
-			       "gpio8", "gpio9";
-			function = "sdc_data";
+		qspi_data {
+			pins = "gpio0", "gpio1", "gpio2";
+			function = "qspi_data";
 			drive-strength = <8>;
 			bias-pull-up;
 		};
 
-		rclk-pins {
-			pins = "gpio10";
-			function = "sdc_rclk";
-			drive-strength = <8>;
-			bias-pull-down;
-		};
+	};
+};
+
+&qpic_bam {
+	status = "okay";
+};
+
+&qpic_nand {
+	status = "okay";
+	pinctrl-0 = <&qspi_nand_pins>;
+	pinctrl-names = "default";
+	spi_nand: spi_nand@0 {
+		compatible = "spi-nand";
+		nand-ecc-engine = <&qpic_nand>;
+		reg = <0>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		nand-ecc-strength = <4>;
+		nand-ecc-step-size = <512>;
+		spi-max-frequency = <8000000>;
 	};
 };
diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index b44acb1fac74..f9c21373f5e6 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -336,10 +336,38 @@ sdhc_1: mmc@7804000 {
 			status = "disabled";
 		};
 
+		qpic_bam: dma@7984000 {
+			compatible = "qcom,bam-v1.7.0";
+			reg = <0x7984000 0x1c000>;
+			interrupts = <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&gcc GCC_QPIC_AHB_CLK>;
+			clock-names = "bam_clk";
+			#dma-cells = <1>;
+			qcom,ee = <0>;
+			status = "disabled";
+		};
+
+		qpic_nand: spi@79b0000 {
+			compatible = "qcom,ipq9574-nand";
+			reg = <0x79b0000 0x10000>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			clocks = <&gcc GCC_QPIC_CLK>,
+			<&gcc GCC_QPIC_AHB_CLK>,
+			<&gcc GCC_QPIC_IO_MACRO_CLK>;
+			clock-names = "core", "aon", "io_macro";
+			dmas = <&qpic_bam 0>,
+				<&qpic_bam 1>,
+				<&qpic_bam 2>;
+			dma-names = "tx", "rx", "cmd";
+			nand-ecc-engine = <&bch>;
+			status = "disabled";
+		};
+
 		bch: qpic_ecc {
 			compatible = "qcom,ipq9574-ecc";
 			status = "ok";
-		}
+		};
 
 		blsp_dma: dma-controller@7884000 {
 			compatible = "qcom,bam-v1.7.0";
-- 
2.34.1

Re: [RFC PATCH 5/5] arm64: dts: qcom: ipq9574: Add support for SPI nand

[Konrad Dybcio]

On 31.10.2023 13:03, Md Sadre Alam wrote:
> Add support for QPIC SPI NAND for IPQ9574
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 56 ++++++++++-----------
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi       | 30 ++++++++++-
>  2 files changed, 57 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
> index 1bb8d96c9a82..5e4200edb873 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
> @@ -15,48 +15,48 @@ / {
>  	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
>  };
>  
> -&sdhc_1 {
> -	pinctrl-0 = <&sdc_default_state>;
> -	pinctrl-names = "default";
> -	mmc-ddr-1_8v;
> -	mmc-hs200-1_8v;
> -	mmc-hs400-1_8v;
> -	mmc-hs400-enhanced-strobe;
> -	max-frequency = <384000000>;
> -	bus-width = <8>;
> -	status = "okay";
> -};
How is removing SDHCI related to adding support for SPI NAND flash?
You must explain your changes in the commit message.

> -
>  &tlmm {
> -	sdc_default_state: sdc-default-state {
> -		clk-pins {
> +	qspi_nand_pins: qspi_nand_pins {
node names (between : and {) must not include underscores, use
hyphens instead

> +		spi_clock {
>  			pins = "gpio5";
> -			function = "sdc_clk";
> +			function = "qspi_clk";
>  			drive-strength = <8>;
>  			bias-disable;
>  		};
>  
> -		cmd-pins {
> +		qspi_cs {
>  			pins = "gpio4";
> -			function = "sdc_cmd";
> +			function = "qspi_cs";
>  			drive-strength = <8>;
>  			bias-pull-up;
>  		};
>  
> -		data-pins {
> -			pins = "gpio0", "gpio1", "gpio2",
> -			       "gpio3", "gpio6", "gpio7",
> -			       "gpio8", "gpio9";
> -			function = "sdc_data";
> +		qspi_data {
> +			pins = "gpio0", "gpio1", "gpio2";
> +			function = "qspi_data";
>  			drive-strength = <8>;
>  			bias-pull-up;
>  		};
>  
> -		rclk-pins {
> -			pins = "gpio10";
> -			function = "sdc_rclk";
> -			drive-strength = <8>;
> -			bias-pull-down;
> -		};
> +	};
> +};
> +
> +&qpic_bam {
> +	status = "okay";
> +};
> +
> +&qpic_nand {
> +	status = "okay";
status should come last
> +	pinctrl-0 = <&qspi_nand_pins>;
> +	pinctrl-names = "default";
> +	spi_nand: spi_nand@0 {
no underscores in node names
missing newline between properties and subnodes

> +		compatible = "spi-nand";
> +		nand-ecc-engine = <&qpic_nand>;
> +		reg = <0>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		nand-ecc-strength = <4>;
> +		nand-ecc-step-size = <512>;
> +		spi-max-frequency = <8000000>;
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index b44acb1fac74..f9c21373f5e6 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -336,10 +336,38 @@ sdhc_1: mmc@7804000 {
>  			status = "disabled";
>  		};
>  
> +		qpic_bam: dma@7984000 {
> +			compatible = "qcom,bam-v1.7.0";
> +			reg = <0x7984000 0x1c000>;
> +			interrupts = <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&gcc GCC_QPIC_AHB_CLK>;
> +			clock-names = "bam_clk";
> +			#dma-cells = <1>;
> +			qcom,ee = <0>;
> +			status = "disabled";
> +		};
You're modifying the SoC and board devicetrees in one go, this won't fly

> +
> +		qpic_nand: spi@79b0000 {
> +			compatible = "qcom,ipq9574-nand";
> +			reg = <0x79b0000 0x10000>;

> +			#address-cells = <1>;
> +			#size-cells = <0>;
these two properties usually go below status, at the end

> +			clocks = <&gcc GCC_QPIC_CLK>,
> +			<&gcc GCC_QPIC_AHB_CLK>,
> +			<&gcc GCC_QPIC_IO_MACRO_CLK>;
Indentation here is messy

> +			clock-names = "core", "aon", "io_macro";
one per line, please

> +			dmas = <&qpic_bam 0>,
> +				<&qpic_bam 1>,
> +				<&qpic_bam 2>;
ditto

> +			dma-names = "tx", "rx", "cmd";
ditto

> +			nand-ecc-engine = <&bch>;
> +			status = "disabled";
> +		};
> +
>  		bch: qpic_ecc {
>  			compatible = "qcom,ipq9574-ecc";
>  			status = "ok";
> -		}
> +		};
This means the previous dt patch would not compile

Konrad

Re: [RFC PATCH 5/5] arm64: dts: qcom: ipq9574: Add support for SPI nand

[Md Sadre Alam]

On 10/31/2023 8:57 PM, Konrad Dybcio wrote:
> On 31.10.2023 13:03, Md Sadre Alam wrote:
>> Add support for QPIC SPI NAND for IPQ9574
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 56 ++++++++++-----------
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi       | 30 ++++++++++-
>>   2 files changed, 57 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
>> index 1bb8d96c9a82..5e4200edb873 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
>> @@ -15,48 +15,48 @@ / {
>>   	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
>>   };
>>   
>> -&sdhc_1 {
>> -	pinctrl-0 = <&sdc_default_state>;
>> -	pinctrl-names = "default";
>> -	mmc-ddr-1_8v;
>> -	mmc-hs200-1_8v;
>> -	mmc-hs400-1_8v;
>> -	mmc-hs400-enhanced-strobe;
>> -	max-frequency = <384000000>;
>> -	bus-width = <8>;
>> -	status = "okay";
>> -};
> How is removing SDHCI related to adding support for SPI NAND flash?
> You must explain your changes in the commit message.
>


its my mistake will fix in V1

>> -
>>   &tlmm {
>> -	sdc_default_state: sdc-default-state {
>> -		clk-pins {
>> +	qspi_nand_pins: qspi_nand_pins {
> node names (between : and {) must not include underscores, use
> hyphens instead

ok

> 
>> +		spi_clock {
>>   			pins = "gpio5";
>> -			function = "sdc_clk";
>> +			function = "qspi_clk";
>>   			drive-strength = <8>;
>>   			bias-disable;
>>   		};
>>   
>> -		cmd-pins {
>> +		qspi_cs {
>>   			pins = "gpio4";
>> -			function = "sdc_cmd";
>> +			function = "qspi_cs";
>>   			drive-strength = <8>;
>>   			bias-pull-up;
>>   		};
>>   
>> -		data-pins {
>> -			pins = "gpio0", "gpio1", "gpio2",
>> -			       "gpio3", "gpio6", "gpio7",
>> -			       "gpio8", "gpio9";
>> -			function = "sdc_data";
>> +		qspi_data {
>> +			pins = "gpio0", "gpio1", "gpio2";
>> +			function = "qspi_data";
>>   			drive-strength = <8>;
>>   			bias-pull-up;
>>   		};
>>   
>> -		rclk-pins {
>> -			pins = "gpio10";
>> -			function = "sdc_rclk";
>> -			drive-strength = <8>;
>> -			bias-pull-down;
>> -		};
>> +	};
>> +};
>> +
>> +&qpic_bam {
>> +	status = "okay";
>> +};
>> +
>> +&qpic_nand {
>> +	status = "okay";
> status should come last
>> +	pinctrl-0 = <&qspi_nand_pins>;
>> +	pinctrl-names = "default";
>> +	spi_nand: spi_nand@0 {
> no underscores in node names
> missing newline between properties and subnodes

ok

> 
>> +		compatible = "spi-nand";
>> +		nand-ecc-engine = <&qpic_nand>;
>> +		reg = <0>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		nand-ecc-strength = <4>;
>> +		nand-ecc-step-size = <512>;
>> +		spi-max-frequency = <8000000>;
>>   	};
>>   };
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> index b44acb1fac74..f9c21373f5e6 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -336,10 +336,38 @@ sdhc_1: mmc@7804000 {
>>   			status = "disabled";
>>   		};
>>   
>> +		qpic_bam: dma@7984000 {
>> +			compatible = "qcom,bam-v1.7.0";
>> +			reg = <0x7984000 0x1c000>;
>> +			interrupts = <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&gcc GCC_QPIC_AHB_CLK>;
>> +			clock-names = "bam_clk";
>> +			#dma-cells = <1>;
>> +			qcom,ee = <0>;
>> +			status = "disabled";
>> +		};
> You're modifying the SoC and board devicetrees in one go, this won't fly

Will fix in V1

> 
>> +
>> +		qpic_nand: spi@79b0000 {
>> +			compatible = "qcom,ipq9574-nand";
>> +			reg = <0x79b0000 0x10000>;
> 
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
> these two properties usually go below status, at the end

Ok

> 
>> +			clocks = <&gcc GCC_QPIC_CLK>,
>> +			<&gcc GCC_QPIC_AHB_CLK>,
>> +			<&gcc GCC_QPIC_IO_MACRO_CLK>;
> Indentation here is messy

Will fix in V1

> 
>> +			clock-names = "core", "aon", "io_macro";
> one per line, please
Ok
> 
>> +			dmas = <&qpic_bam 0>,
>> +				<&qpic_bam 1>,
>> +				<&qpic_bam 2>;
> ditto
Ok
> 
>> +			dma-names = "tx", "rx", "cmd";
> ditto
Ok
> 
>> +			nand-ecc-engine = <&bch>;
>> +			status = "disabled";
>> +		};
>> +
>>   		bch: qpic_ecc {
>>   			compatible = "qcom,ipq9574-ecc";
>>   			status = "ok";
>> -		}
>> +		};
> This means the previous dt patch would not compile

Will fix in V1


Regards
Alam.

Re: [RFC PATCH 5/5] arm64: dts: qcom: ipq9574: Add support for SPI nand

[Krzysztof Kozlowski]

On 31/10/2023 13:03, Md Sadre Alam wrote:
> Add support for QPIC SPI NAND for IPQ9574
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> Signed-off-by: Sricharan R <quic_srichara@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 56 ++++++++++-----------
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi       | 30 ++++++++++-
>  2 files changed, 57 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
> index 1bb8d96c9a82..5e4200edb873 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
> +++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
> @@ -15,48 +15,48 @@ / {
>  	compatible = "qcom,ipq9574-ap-al02-c7", "qcom,ipq9574";
>  };
>  
> -&sdhc_1 {
> -	pinctrl-0 = <&sdc_default_state>;
> -	pinctrl-names = "default";
> -	mmc-ddr-1_8v;
> -	mmc-hs200-1_8v;
> -	mmc-hs400-1_8v;
> -	mmc-hs400-enhanced-strobe;
> -	max-frequency = <384000000>;
> -	bus-width = <8>;
> -	status = "okay";

Why? This is not explained in commit msg.

> -};
> -
>  &tlmm {
> -	sdc_default_state: sdc-default-state {
> -		clk-pins {
> +	qspi_nand_pins: qspi_nand_pins {
> +		spi_clock {
>  			pins = "gpio5";
> -			function = "sdc_clk";
> +			function = "qspi_clk";

Why?

>  			drive-strength = <8>;
>  			bias-disable;
>  		};
>  
> -		cmd-pins {
> +		qspi_cs {

No, come one. Code was good and you replace it to incorrect one. Please
stop bringing more issues to fix.

>  			pins = "gpio4";
> -			function = "sdc_cmd";
> +			function = "qspi_cs";
>  			drive-strength = <8>;
>  			bias-pull-up;
>  		};
>  

...

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

This is the saddest part of the entire patchset...

>  
>  		blsp_dma: dma-controller@7884000 {
>  			compatible = "qcom,bam-v1.7.0";

Best regards,
Krzysztof