| Message ID | 1591701056-3944-3-git-send-email-sivaprak@codeaurora.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | Fix issues related to register access in IPQ NAND | expand |
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
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
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
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); }
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(-)