[PATCH v2 0/4] mtd: rawnand: qcom: Add MDM9607

[PATCH v2 0/4] mtd: rawnand: qcom: Add MDM9607

[Stephan Gerhold]

MDM9607 has QPIC v1.5 that supports the OP_PAGE_READ_ONFI_READ command, but
is missing the rest of the hardware changes in QPIC v2. There is also only
a single clock that can be controlled using the RPM firmware. Document and
add the new qcom,mdm9607-nand compatible for this setup.

Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
Changes in v2:
- Pick up review tags (Mani, Krzysztof)
- Clarify commit message/comments in PATCH 1 and 2 based on discussion.
  The OS can only control one clock on these platforms.
  (Krzysztof, Miquel, Konrad, Mani)
- Link to v1: https://patch.msgid.link/20260608-qcom-nandc-mdm9607-v1-0-4639a0492274@linaro.org

---
Stephan Gerhold (4):
      dt-bindings: mtd: qcom,nandc: Add MDM9607 QPIC NAND controller
      mtd: rawnand: qcom: Make "aon" clock optional
      mtd: rawnand: qcom: Make has_onfi_read_op separate from qpic_version2
      mtd: rawnand: qcom: Add MDM9607 compatible

 .../devicetree/bindings/mtd/qcom,nandc.yaml        | 25 ++++++++++++++++++
 drivers/mtd/nand/raw/qcom_nandc.c                  | 30 ++++++++++++++++------
 include/linux/mtd/nand-qpic-common.h               |  2 ++
 3 files changed, 49 insertions(+), 8 deletions(-)
---
base-commit: adfc275b317c02cd043b0cf28b8cfb7459b041f0
change-id: 20260608-qcom-nandc-mdm9607-f2196eeb6bc0

Best regards,
--  
Stephan Gerhold <stephan.gerhold@linaro.org>

[PATCH v2 1/4] dt-bindings: mtd: qcom,nandc: Add MDM9607 QPIC NAND controller

[Stephan Gerhold]

Add the qcom,mdm9607-nand compatible for the QPIC NAND controller used
inside the MDM9607 SoC.

On MDM9607 and other recent SoCs, the QPIC hardware requires 3 clocks
(core, aon, ahb). However, access to these clocks is restricted to the RPM
firmware that controls the shared power resources for the whole SoC. The
clocks cannot be controlled separately, for the OS view of the hardware
there is only a single RPM_SMD_QPIC_CLK clock that implicitly enables all
of the 3 clocks. The only exception to this are some IPQ* SoC that are not
using RPM, there the clocks are directly controlled by the kernel via the
clock controller (GCC). Require only one clock in the dt-bindings for
MDM9607 to avoid having to define dummy clock entries.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 .../devicetree/bindings/mtd/qcom,nandc.yaml        | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
index 5511389960f0..a916cac53af6 100644
--- a/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
+++ b/Documentation/devicetree/bindings/mtd/qcom,nandc.yaml
@@ -22,17 +22,20 @@ properties:
               - qcom,ipq4019-nand
               - qcom,ipq6018-nand
               - qcom,ipq8074-nand
+              - qcom,mdm9607-nand
               - qcom,sdx55-nand
 
   reg:
     maxItems: 1
 
   clocks:
+    minItems: 1
     items:
       - description: Core Clock
       - description: Always ON Clock
 
   clock-names:
+    minItems: 1
     items:
       - const: core
       - const: aon
@@ -101,6 +104,27 @@ allOf:
           items:
             - const: rxtx
 
+  # On MDM9607, the OS can only control a single clock.
+  # The 3 hardware clocks (core, aon, ahb) are invisible to the OS.
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,mdm9607-nand
+    then:
+      properties:
+        clocks:
+          maxItems: 1
+        clock-names:
+          maxItems: 1
+    else:
+      properties:
+        clocks:
+          minItems: 2
+        clock-names:
+          minItems: 2
+
   - if:
       properties:
         compatible:
@@ -121,6 +145,7 @@ allOf:
               - qcom,ipq4019-nand
               - qcom,ipq6018-nand
               - qcom,ipq8074-nand
+              - qcom,mdm9607-nand
               - qcom,sdx55-nand
 
     then:

-- 
2.54.0

[PATCH v2 2/4] mtd: rawnand: qcom: Make "aon" clock optional

[Stephan Gerhold]

Some SoCs (e.g. MDM9607, SDX55) have only a single separately controllable
clock for the NAND controller. The actual clocks in the hardware are
managed by the firmware and turned on all together when needed. In this
case, there is no separate "aon" clock that can be described in the device
tree from the OS point of view.

Make the second "aon" clock optional to avoid an error when it is missing.
For platforms that really need it, the dt-bindings are responsible for
validating that.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 4b80ce084d9a..0251dd591d40 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2280,7 +2280,7 @@ static int qcom_nandc_probe(struct platform_device *pdev)
 	if (IS_ERR(nandc->core_clk))
 		return PTR_ERR(nandc->core_clk);
 
-	nandc->aon_clk = devm_clk_get(dev, "aon");
+	nandc->aon_clk = devm_clk_get_optional(dev, "aon");
 	if (IS_ERR(nandc->aon_clk))
 		return PTR_ERR(nandc->aon_clk);
 

-- 
2.54.0

Re: [PATCH v2 2/4] mtd: rawnand: qcom: Make "aon" clock optional

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free when boot partition parsing fails
- [High] Unbalanced teardown leads to Use-After-Free when a subsequent child node fails probe
--

--- Patch [2]: [PATCH v2 2/4] mtd: rawnand: qcom: Make "aon" clock optional ---
commit 7ddf091d1f9f9e091fb62bfa7346955975fe9533
Author: Stephan Gerhold <stephan.gerhold@linaro.org>

mtd: rawnand: qcom: Make "aon" clock optional
    
This commit makes the second "aon" clock optional to avoid an error when
it is missing on certain SoCs like MDM9607 and SDX55.

> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 4b80ce084d9ac..0251dd591d40a 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2280,7 +2280,7 @@ static int qcom_nandc_probe(struct platform_device *pdev)
>  	if (IS_ERR(nandc->core_clk))
>  		return PTR_ERR(nandc->core_clk);
>  
> -	nandc->aon_clk = devm_clk_get(dev, "aon");
> +	nandc->aon_clk = devm_clk_get_optional(dev, "aon");
>  	if (IS_ERR(nandc->aon_clk))
>  		return PTR_ERR(nandc->aon_clk);
>  

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the probe
path I noticed a potential use-after-free when boot partition parsing fails.

In qcom_nand_host_init_and_register(), if mtd_device_parse_register()
succeeds but the subsequent qcom_nand_host_parse_boot_partitions() fails:

qcom_nand_host_init_and_register()
   mtd_device_parse_register()
   qcom_nand_host_parse_boot_partitions()
   goto err;

err:
   nand_cleanup(chip);
   return ret;

Since mtd_device_unregister() is not called in the error path, and the caller
qcom_probe_nand_devices() frees the host memory via devm_kfree(), does this
leave stale pointers in the MTD subsystem?

[Severity: High]
This is also a pre-existing issue, but there appears to be an unbalanced
teardown leading to a use-after-free if a subsequent child node fails
during probe.

In qcom_probe_nand_devices(), the driver iterates through child nodes:

qcom_probe_nand_devices() {
    for_each_available_child_of_node(...) {
        ...
        ret = qcom_nand_host_init_and_register();
        if (ret)
            continue;
    }
    return ret;
}

If an earlier child is successfully initialized and registered, but a later
child fails, the function eventually returns a non-zero error code. This
causes the main qcom_nandc_probe() to fail, and devres automatically frees
the memory for all children.

Since there is no error handling path to unregister previously successful
children, could this result in the MTD subsystem retaining pointers to the
implicitly freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-qcom-nandc-mdm9607-v2-0-d906f7e8b814@linaro.org?part=2

[PATCH v2 3/4] mtd: rawnand: qcom: Make has_onfi_read_op separate from qpic_version2

[Stephan Gerhold]

QPIC v1.5 requires using the OP_PAGE_READ_ONFI_READ command, but is missing
the rest of the hardware changes that are currently covered by the QPIC v2
(qpic_version2) check in the driver. Split that into an extra
has_onfi_read_op feature flag so it can be separately enabled.

No functional change.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c    | 15 ++++++++-------
 include/linux/mtd/nand-qpic-common.h |  2 ++
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 0251dd591d40..9217e8de5512 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1564,7 +1564,7 @@ static int qcom_op_cmd_mapping(struct nand_chip *chip, u8 opcode,
 		cmd = OP_FETCH_ID;
 		break;
 	case NAND_CMD_PARAM:
-		if (nandc->props->qpic_version2)
+		if (nandc->props->has_onfi_read_op)
 			cmd = OP_PAGE_READ_ONFI_READ;
 		else
 			cmd = OP_PAGE_READ;
@@ -1903,7 +1903,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 		nandc->regs->ecc_buf_cfg = cpu_to_le32(ECC_CFG_ECC_DISABLE);
 
 	/* configure CMD1 and VLD for ONFI param probing in QPIC v1 */
-	if (!nandc->props->qpic_version2) {
+	if (!nandc->props->has_onfi_read_op) {
 		nandc->regs->vld = cpu_to_le32((nandc->vld & ~READ_START_VLD));
 		nandc->regs->cmd1 = cpu_to_le32((nandc->cmd1 & ~READ_ADDR_MASK) |
 						FIELD_PREP(READ_ADDR_MASK, NAND_CMD_PARAM));
@@ -1911,7 +1911,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 
 	nandc->regs->exec = cpu_to_le32(1);
 
-	if (!nandc->props->qpic_version2) {
+	if (!nandc->props->has_onfi_read_op) {
 		nandc->regs->orig_cmd1 = cpu_to_le32(nandc->cmd1);
 		nandc->regs->orig_vld = cpu_to_le32(nandc->vld);
 	}
@@ -1925,7 +1925,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 	else
 		nandc_set_read_loc_first(chip, reg_base, 0, len, 1);
 
-	if (!nandc->props->qpic_version2) {
+	if (!nandc->props->has_onfi_read_op) {
 		qcom_write_reg_dma(nandc, &nandc->regs->vld, NAND_DEV_CMD_VLD, 1, 0);
 		qcom_write_reg_dma(nandc, &nandc->regs->cmd1, NAND_DEV_CMD1, 1, NAND_BAM_NEXT_SGL);
 	}
@@ -1939,7 +1939,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 			   nandc->buf_count, 0);
 
 	/* restore CMD1 and VLD regs */
-	if (!nandc->props->qpic_version2) {
+	if (!nandc->props->has_onfi_read_op) {
 		qcom_write_reg_dma(nandc, &nandc->regs->orig_cmd1, NAND_DEV_CMD1_RESTORE, 1, 0);
 		qcom_write_reg_dma(nandc, &nandc->regs->orig_vld, NAND_DEV_CMD_VLD_RESTORE, 1,
 				   NAND_BAM_NEXT_SGL);
@@ -2041,7 +2041,7 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
 	if (!nandc->props->nandc_part_of_qpic)
 		nandc_write(nandc, SFLASHC_BURST_CFG, 0);
 
-	if (!nandc->props->qpic_version2)
+	if (!nandc->props->has_onfi_read_op)
 		nandc_write(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD),
 			    NAND_DEV_CMD_VLD_VAL);
 
@@ -2063,7 +2063,7 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
 	}
 
 	/* save the original values of these registers */
-	if (!nandc->props->qpic_version2) {
+	if (!nandc->props->has_onfi_read_op) {
 		nandc->cmd1 = nandc_read(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD1));
 		nandc->vld = NAND_DEV_CMD_VLD_VAL;
 	}
@@ -2385,6 +2385,7 @@ static const struct qcom_nandc_props sdx55_nandc_props = {
 	.ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT),
 	.supports_bam = true,
 	.nandc_part_of_qpic = true,
+	.has_onfi_read_op = true,
 	.qpic_version2 = true,
 	.dev_cmd_reg_start = 0x7000,
 	.bam_offset = 0x30000,
diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h
index 006ca8c978a9..437448995187 100644
--- a/include/linux/mtd/nand-qpic-common.h
+++ b/include/linux/mtd/nand-qpic-common.h
@@ -443,6 +443,7 @@ struct qcom_nand_controller {
  * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
  * @supports_bam - whether NAND controller is using BAM
  * @nandc_part_of_qpic - whether NAND controller is part of qpic IP
+ * @has_onfi_read_op - whether ONFI param page read command is supported
  * @qpic_version2 - flag to indicate QPIC IP version 2
  * @use_codeword_fixup - whether NAND has different layout for boot partitions
  */
@@ -452,6 +453,7 @@ struct qcom_nandc_props {
 	u32 bam_offset;
 	bool supports_bam;
 	bool nandc_part_of_qpic;
+	bool has_onfi_read_op;
 	bool qpic_version2;
 	bool use_codeword_fixup;
 };

-- 
2.54.0

Re: [PATCH v2 3/4] mtd: rawnand: qcom: Make has_onfi_read_op separate from qpic_version2

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Incorrect condition for NAND_DEV_CMD_VLD register initialization causes it to be skipped for QPIC v1.5 controllers.

Pre-existing issues:
- [High] Uninitialized read location used during ONFI parameter probing on QPIC v2 controllers.
- [High] BAM DMA writes the last codeword read location to the wrong physical register address on QPIC v2.
--

--- Patch [3]: [PATCH v2 3/4] mtd: rawnand: qcom: Make has_onfi_read_op separate from qpic_version2 ---
commit 2349ad4df61c77511228122eab5bf45ff502d556
Author: Stephan Gerhold <stephan.gerhold@linaro.org>

mtd: rawnand: qcom: Make has_onfi_read_op separate from qpic_version2
    
QPIC v1.5 requires using the OP_PAGE_READ_ONFI_READ command, but is missing
the rest of the hardware changes that are currently covered by the QPIC v2
(qpic_version2) check in the driver. Split that into an extra
has_onfi_read_op feature flag so it can be separately enabled.
    
No functional change.

> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 0251dd591d40a..9217e8de55129 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c

[ ... ]

> @@ -1925,7 +1925,7 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
>  	else
>  		nandc_set_read_loc_first(chip, reg_base, 0, len, 1);
>  
> -	if (!nandc->props->qpic_version2) {
> +	if (!nandc->props->has_onfi_read_op) {

[Severity: High]
This is a pre-existing issue, but does the read location setup work correctly
during ONFI parameter probing on QPIC v2 controllers?

In qcom_param_page_type_exec(), nandc_set_read_loc_last() correctly
populates read_location_last0 when qpic_version2 is true. However, it later
calls config_nand_single_cw_page_read() which delegates to
config_nand_cw_read().

During ONFI probing, ecc->steps is 0. In config_nand_cw_read(), the
condition to select read_location_last0 evaluates to false because
qcom_nandc_is_last_cw(ecc, cw) compares 0 == (0 - 1):

drivers/mtd/nand/raw/qcom_nandc.c:config_nand_cw_read() {
    ...
	__le32 *reg = &nandc->regs->read_location0;

	if (nandc->props->qpic_version2 && qcom_nandc_is_last_cw(ecc, cw))
		reg = &nandc->regs->read_location_last0;
    ...
}

As a result, it falls back to read_location0, which is uninitialized and
results in a 0-sized read location being written to the hardware.

Additionally, even if read_location_last0 were correctly selected,
config_nand_cw_read() hardcodes the physical destination address to
NAND_READ_LOCATION_0:

drivers/mtd/nand/raw/qcom_nandc.c:config_nand_cw_read() {
    ...
	if (nandc->props->supports_bam)
		qcom_write_reg_dma(nandc, reg, NAND_READ_LOCATION_0, 4,
                                   NAND_BAM_NEXT_SGL);
    ...
}

Does this incorrectly write the last codeword configuration to the standard
NAND_READ_LOCATION_0 (0xf20) instead of the required
NAND_READ_LOCATION_LAST_CW_0 (0xf40) on QPIC v2 controllers?

>  		qcom_write_reg_dma(nandc, &nandc->regs->vld, NAND_DEV_CMD_VLD, 1, 0);
>  		qcom_write_reg_dma(nandc, &nandc->regs->cmd1, NAND_DEV_CMD1, 1, NAND_BAM_NEXT_SGL);
>  	}

[ ... ]

> @@ -2041,7 +2041,7 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
>  	if (!nandc->props->nandc_part_of_qpic)
>  		nandc_write(nandc, SFLASHC_BURST_CFG, 0);
>  
> -	if (!nandc->props->qpic_version2)
> +	if (!nandc->props->has_onfi_read_op)
>  		nandc_write(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD),
>  			    NAND_DEV_CMD_VLD_VAL);
>  

[Severity: High]
Does this change cause the VLD register initialization to be incorrectly
skipped for QPIC v1.5 controllers?

During qcom_nandc_setup(), if we have a controller like MDM9607 where
has_onfi_read_op is true but qpic_version2 is false, the new
!nandc->props->has_onfi_read_op condition evaluates to false.

This skips writing NAND_DEV_CMD_VLD_VAL. Will this cause standard read,
write, and erase commands to fail validation on QPIC v1.5 because
NAND_DEV_CMD_VLD is never properly initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-qcom-nandc-mdm9607-v2-0-d906f7e8b814@linaro.org?part=3

[PATCH v2 4/4] mtd: rawnand: qcom: Add MDM9607 compatible

[Stephan Gerhold]

MDM9607 has QPIC v1.5 that supports the OP_PAGE_READ_ONFI_READ command, but
is missing the rest of the hardware changes in QPIC v2. Add the new
qcom,mdm9607-nand compatible and set it to use has_onfi_read_op without
also setting qpic_version2.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 9217e8de5512..d7642db2e2df 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2381,6 +2381,15 @@ static const struct qcom_nandc_props ipq8074_nandc_props = {
 	.bam_offset = 0x30000,
 };
 
+static const struct qcom_nandc_props mdm9607_nandc_props = {
+	.ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT),
+	.supports_bam = true,
+	.nandc_part_of_qpic = true,
+	.has_onfi_read_op = true,
+	.dev_cmd_reg_start = 0x7000,
+	.bam_offset = 0x30000,
+};
+
 static const struct qcom_nandc_props sdx55_nandc_props = {
 	.ecc_modes = (ECC_BCH_4BIT | ECC_BCH_8BIT),
 	.supports_bam = true,
@@ -2412,6 +2421,10 @@ static const struct of_device_id qcom_nandc_of_match[] = {
 		.compatible = "qcom,ipq8074-nand",
 		.data = &ipq8074_nandc_props,
 	},
+	{
+		.compatible = "qcom,mdm9607-nand",
+		.data = &mdm9607_nandc_props,
+	},
 	{
 		.compatible = "qcom,sdx55-nand",
 		.data = &sdx55_nandc_props,

-- 
2.54.0