On Tue, Nov 19, 2024 at 02:50:57PM +0530, Md Sadre Alam wrote:
> Currently we are configuring lower 24 bits of address in descriptor
> whereas QPIC design expects 18 bit register offset from QPIC base
You mean 'QPIC IP' here? But is it QPIC or NANDc? I guess the later.
> address to be configured in cmd descriptors. This is leading to a
> different address actually being used in HW, leading to wrong value
> read.
>
This doesn't clearly say what the actual issue is. IIUC, the issue is that the
NANDc base address is different from the QPIC base address. But the driver
doesn't take it into account and just used the QPIC base as the NANDc base. This
used to work as the NANDc IP only considers the lower 18 bits of the address
passed by the driver to derive the register offset. Since the base address of
QPIC used to contain all 0 for lower 18 bits (like 0x07980000), the driver ended
up passing the actual register offset in it and NANDc worked properly. But on
newer SoCs like SDX75, the QPIC base address doesn't contain all 0 for lower 18
bits (like 0x01C98000). So NANDc sees wrong offset as per the current logic.
> Older targets also used same configuration (lower 24 bits) like sdxpinn,
Please use actual product names and not internal names. I believe you are
referring to SDX55/SDX65 here.
> ipq etc. but issue is masked in older targets due to lower 18 bits of QPIC
> base address being zero leading to expected address generation.
>
> Sdxpinn : QPIC_QPIC | 0x01C98000 (Lower 18 bits are non zero)
> Sdxnightjar : QPIC_QPIC | 0x07980000 (Lower 18 bits are zero) Same for
> older targets.
Same here.
>
> Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>
Please add relevant Fixes tag.
> ---
> drivers/mtd/nand/raw/qcom_nandc.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index b8cff9240b28..34ee8555fb8a 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -207,7 +207,7 @@ nandc_set_reg(chip, reg, \
> #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))
> +#define nandc_reg_phys(chip, offset) ((nandc)->props->offset_from_qpic + (offset))
>
> /* Returns the dma address for reg read buffer */
> #define reg_buf_dma_addr(chip, vaddr) \
> @@ -561,6 +561,7 @@ struct qcom_nandc_props {
> bool is_qpic;
> bool qpic_v2;
> bool use_codeword_fixup;
> + u32 offset_from_qpic;
nandc_offset?
> };
>
> /* Frees the BAM transaction memory */
> @@ -3477,6 +3478,7 @@ static const struct qcom_nandc_props ipq806x_nandc_props = {
> .is_bam = false,
> .use_codeword_fixup = true,
> .dev_cmd_reg_start = 0x0,
> + .offset_from_qpic = 0x30000,
How 0x30000 is supposed to work? You said the NANDc ignores lower 18 bits, but
this has 17th and 18th bits set.
- Mani
--
மணிவண்ணன் சதாசிவம்
