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
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 <[email protected]>
---
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
Hi Sivaprakash,
Sivaprakash Murugesan <[email protected]> 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 <[email protected]>
> ---
> 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
Hi Miquel,
Thanks for the review.
On 6/9/2020 7:33 PM, Miquel Raynal wrote:
> Hi Sivaprakash,
>
> Sivaprakash Murugesan <[email protected]> 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 <[email protected]>
>> ---
>> 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
Hi Sivaprakash,
Sivaprakash Murugesan <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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