linux-kernel - Re: [PATCH 1/2] mtd: rawnand: qcom: Pass 18 bit offset from QPIC base address to BAM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d3c0414c-6bd7-6b46-93e2-216dcaa4d39a@quicinc.com>
Date: Thu, 21 Nov 2024 14:31:43 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
CC: <miquel.raynal@...tlin.com>, <richard@....at>, <vigneshr@...com>,
        <linux-mtd@...ts.infradead.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_srichara@...cinc.com>,
        <quic_nainmeht@...cinc.com>, <quic_laksd@...cinc.com>,
        <quic_varada@...cinc.com>
Subject: Re: [PATCH 1/2] mtd: rawnand: qcom: Pass 18 bit offset from QPIC base
 address to BAM



On 11/21/2024 12:29 PM, Manivannan Sadhasivam wrote:
> On Thu, Nov 21, 2024 at 11:33:13AM +0530, Md Sadre Alam wrote:
>>
>>
>> On 11/20/2024 12:31 PM, Manivannan Sadhasivam wrote:
>>> 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.
>> It's QPIC IP only.
> 
> Hmm, so what is the difference between QPIC and NANDc?
QPIC is wrapper which integrates NANDc. So only QPIC (Qualcomm Parallel
Interface Controller) will be exposed for interface.
> 
>>>
>>>> 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.
>> Yes correct. If QPIC address = 0x07980000 and QPIC_EBI2NAND address = 0x079b0000
>> the the diff is 0x30000, this is the actual offset expected by QPIC RTL code.
>> and RTL needs only 18-bit offset.
> 
> Okay. So the driver used to pass 0x30000 + offset in older targets and on newer
> ones starting from SDX75, 0x30000 is not passed correctly due to the changed
> QPIC base address.
Yes, correct. In SDX75 the first 18-bits of QPIC base address are non-zero.
> 
> Please mention it clearly in description.
Ok
> 
> - Mani
> 
>>>
>>>> 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.
>> Ok , will change in next revision.
>>>
>>>> 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.
>> Ok
>>>
>>>>
>>>> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
>>>
>>> Please add relevant Fixes tag.
>> Ok
>>>
>>>> ---
>>>>    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?
>> Ok
>>>
>>>>    };
>>>>    /* 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.
>> Not this address 0x30000, this the diff b/w QPIC base and EBI2NAND base. The 18-bits we have see
>> on this address 0x07980000 and this address 0x01C98000.
>>>
>>> - Mani
>>>
>