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

[sashiko-bot@kernel.org]

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