[PATCH V2 0/2] Fix issues related to register access in IPQ NAND

[PATCH V2 0/2] Fix issues related to register access in IPQ NAND

[Sivaprakash Murugesan]

Patch 1: avoids register write to unavailable SFLASHC_BURST_CFG register
Patch 2: set BAM mode only if not set by bootloader

[V2]
 * As per review comments from Miquèl split the original patch into two
   addressing independent issues.  

Sivaprakash Murugesan (2):
  mtd: rawnand: qcom: remove write to unavailable register
  mtd: rawnand: qcom: set BAM mode only if not set already

 drivers/mtd/nand/raw/qcom_nandc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH V2 1/2] mtd: rawnand: qcom: remove write to unavailable register

[Sivaprakash Murugesan]

SFLASHC_BURST_CFG register is not available on all ipq nand platforms,
it is available only on ipq8064 devices and the nand controller works
without configuring these registers in this platform, so register write
to this can be removed.

Signed-off-by: Sivaprakash Murugesan <sivaprak@codeaurora.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 5b11c70..e0afa2c 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -36,7 +36,6 @@
 #define	NAND_DEV_CMD1			0xa4
 #define	NAND_DEV_CMD2			0xa8
 #define	NAND_DEV_CMD_VLD		0xac
-#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
@@ -2774,7 +2773,6 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
 	u32 nand_ctrl;
 
 	/* kill onenand */
-	nandc_write(nandc, SFLASHC_BURST_CFG, 0);
 	nandc_write(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD),
 		    NAND_DEV_CMD_VLD_VAL);
 
-- 
2.7.4

Re: [PATCH V2 1/2] mtd: rawnand: qcom: remove write to unavailable register

[Miquel Raynal]

Hi Sivaprakash,

Sivaprakash Murugesan <sivaprak@codeaurora.org> wrote on Tue,  9 Jun
2020 16:40:55 +0530:

> SFLASHC_BURST_CFG register is not available on all ipq nand platforms,
> it is available only on ipq8064 devices and the nand controller works
> without configuring these registers in this platform, so register write
> to this can be removed.

Maybe it works because the bootloader is setting the register itself.
What if you use a different bootloader, or the same bootloader without
NAND support?

I don't think this is a proper fix, you should instead have a different
compatible if the IP is not the same and depending on this compatible
do the write, or not.

> 
> Signed-off-by: Sivaprakash Murugesan <sivaprak@codeaurora.org>
> ---
>  drivers/mtd/nand/raw/qcom_nandc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 5b11c70..e0afa2c 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -36,7 +36,6 @@
>  #define	NAND_DEV_CMD1			0xa4
>  #define	NAND_DEV_CMD2			0xa8
>  #define	NAND_DEV_CMD_VLD		0xac
> -#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
> @@ -2774,7 +2773,6 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
>  	u32 nand_ctrl;
>  
>  	/* kill onenand */
> -	nandc_write(nandc, SFLASHC_BURST_CFG, 0);
>  	nandc_write(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD),
>  		    NAND_DEV_CMD_VLD_VAL);
>  

Thanks,
Miquèl

Re: [PATCH V2 1/2] mtd: rawnand: qcom: remove write to unavailable register

[Sivaprakash Murugesan]

Hi Miquel,

On 6/9/2020 7:32 PM, Miquel Raynal wrote:
> Hi Sivaprakash,
>
> Sivaprakash Murugesan <sivaprak@codeaurora.org> wrote on Tue,  9 Jun
> 2020 16:40:55 +0530:
>
>> SFLASHC_BURST_CFG register is not available on all ipq nand platforms,
>> it is available only on ipq8064 devices and the nand controller works
>> without configuring these registers in this platform, so register write
>> to this can be removed.
> Maybe it works because the bootloader is setting the register itself.
> What if you use a different bootloader, or the same bootloader without
> NAND support?
>
> I don't think this is a proper fix, you should instead have a different
> compatible if the IP is not the same and depending on this compatible
> do the write, or not.

I understand your point, will fix this in next patch.

Thanks,

Siva

> Thanks,
> Miquèl

[PATCH V2 2/2] mtd: rawnand: qcom: set BAM mode only if not set already

[Sivaprakash Murugesan]

BAM mode is set by writing BAM_MODE_EN bit on NAND_CTRL register.
NAND_CTRL is an operational register and in BAM mode operational
registers are read only.

So, before writing into NAND_CTRL register check if BAM mode is already
enabled by bootloader, and set BAM mode only if it is not set already.

Signed-off-by: Sivaprakash Murugesan <sivaprak@codeaurora.org>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index e0afa2c..7740059 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2779,7 +2779,14 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
 	/* enable ADM or BAM DMA */
 	if (nandc->props->is_bam) {
 		nand_ctrl = nandc_read(nandc, NAND_CTRL);
-		nandc_write(nandc, NAND_CTRL, nand_ctrl | BAM_MODE_EN);
+		/* 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);
 	}
-- 
2.7.4

Re: [PATCH V2 2/2] mtd: rawnand: qcom: set BAM mode only if not set already

[Miquel Raynal]

Hi Sivaprakash,

Sivaprakash Murugesan <sivaprak@codeaurora.org> wrote on Tue,  9 Jun
2020 16:40:56 +0530:

> BAM mode is set by writing BAM_MODE_EN bit on NAND_CTRL register.
> NAND_CTRL is an operational register and in BAM mode operational
> registers are read only.
> 
> So, before writing into NAND_CTRL register check if BAM mode is already
> enabled by bootloader, and set BAM mode only if it is not set already.
> 
> Signed-off-by: Sivaprakash Murugesan <sivaprak@codeaurora.org>
> ---
>  drivers/mtd/nand/raw/qcom_nandc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index e0afa2c..7740059 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2779,7 +2779,14 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
>  	/* enable ADM or BAM DMA */
>  	if (nandc->props->is_bam) {
>  		nand_ctrl = nandc_read(nandc, NAND_CTRL);
> -		nandc_write(nandc, NAND_CTRL, nand_ctrl | BAM_MODE_EN);
> +		/* 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);
>  	}

Does this currently produces an issue at runtime?

If yes, you should have a Fixes/CC: stable pair of tags.

Also, what is BAM mode? Please tell us in the commit log.

Thanks,
Miquèl

Re: [PATCH V2 2/2] mtd: rawnand: qcom: set BAM mode only if not set already

[Sivaprakash Murugesan]

Hi Miquel,

Thanks for the review.

On 6/9/2020 7:33 PM, Miquel Raynal wrote:
> Hi Sivaprakash,
>
> Sivaprakash Murugesan <sivaprak@codeaurora.org> wrote on Tue,  9 Jun
> 2020 16:40:56 +0530:
>
>> BAM mode is set by writing BAM_MODE_EN bit on NAND_CTRL register.
>> NAND_CTRL is an operational register and in BAM mode operational
>> registers are read only.
>>
>> So, before writing into NAND_CTRL register check if BAM mode is already
>> enabled by bootloader, and set BAM mode only if it is not set already.
>>
>> Signed-off-by: Sivaprakash Murugesan <sivaprak@codeaurora.org>
>> ---
>>   drivers/mtd/nand/raw/qcom_nandc.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
>> index e0afa2c..7740059 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -2779,7 +2779,14 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
>>   	/* enable ADM or BAM DMA */
>>   	if (nandc->props->is_bam) {
>>   		nand_ctrl = nandc_read(nandc, NAND_CTRL);
>> -		nandc_write(nandc, NAND_CTRL, nand_ctrl | BAM_MODE_EN);
>> +		/* 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);
>>   	}
> Does this currently produces an issue at runtime?
>
> If yes, you should have a Fixes/CC: stable pair of tags.
>
> Also, what is BAM mode? Please tell us in the commit log.

Currently this is not causing any issue on run time.

The writes to this register is silently ignored.

However, this could be an issue in future Hardware designs.

BAM is the DMA engine on QCOM IPQ platforms, sure will explain this

mode in next patchset.

>
> Thanks,
> Miquèl

Re: [PATCH V2 2/2] mtd: rawnand: qcom: set BAM mode only if not set already

[Miquel Raynal]

Hi Sivaprakash,

Sivaprakash Murugesan <sivaprak@codeaurora.org> wrote on Thu, 11 Jun
2020 09:57:59 +0530:

> Hi Miquel,
> 
> Thanks for the review.
> 
> On 6/9/2020 7:33 PM, Miquel Raynal wrote:
> > Hi Sivaprakash,
> >
> > Sivaprakash Murugesan <sivaprak@codeaurora.org> wrote on Tue,  9 Jun
> > 2020 16:40:56 +0530:
> >  
> >> BAM mode is set by writing BAM_MODE_EN bit on NAND_CTRL register.
> >> NAND_CTRL is an operational register and in BAM mode operational
> >> registers are read only.
> >>
> >> So, before writing into NAND_CTRL register check if BAM mode is already
> >> enabled by bootloader, and set BAM mode only if it is not set already.
> >>
> >> Signed-off-by: Sivaprakash Murugesan <sivaprak@codeaurora.org>
> >> ---
> >>   drivers/mtd/nand/raw/qcom_nandc.c | 9 ++++++++-
> >>   1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> >> index e0afa2c..7740059 100644
> >> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> >> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> >> @@ -2779,7 +2779,14 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
> >>   	/* enable ADM or BAM DMA */
> >>   	if (nandc->props->is_bam) {
> >>   		nand_ctrl = nandc_read(nandc, NAND_CTRL);
> >> -		nandc_write(nandc, NAND_CTRL, nand_ctrl | BAM_MODE_EN);
> >> +		/* 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);
> >>   	}  
> > Does this currently produces an issue at runtime?
> >
> > If yes, you should have a Fixes/CC: stable pair of tags.
> >
> > Also, what is BAM mode? Please tell us in the commit log.  
> 
> Currently this is not causing any issue on run time.
> 
> The writes to this register is silently ignored.
> 
> However, this could be an issue in future Hardware designs.
> 
> BAM is the DMA engine on QCOM IPQ platforms, sure will explain this
> 
> mode in next patchset.

I don't like so much the idea of DMA being enabled by the Bootloader or
not, this is something that should need to be fixed.


Thanks,
Miquèl