[PATCH] mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling

[PATCH] mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling

[Kamal Dasu]

If mtd_oops is in progress switch to polling for nand command completion
interrupts and use PIO mode wihtout DMA so that the mtd_oops buffer can
be completely written in the assinged nand partition. This is needed in
cases where the panic does not happen on cpu0 and there is only one online
CPU and the panic is not on cpu0.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 55 ++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 482c6f0..cfbe51a 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -823,6 +823,12 @@ static inline bool has_flash_dma(struct brcmnand_controller *ctrl)
 	return ctrl->flash_dma_base;
 }
 
+static inline void disable_flash_dma_xfer(struct brcmnand_controller *ctrl)
+{
+	if (has_flash_dma(ctrl))
+		ctrl->flash_dma_base = 0;
+}
+
 static inline bool flash_dma_buf_ok(const void *buf)
 {
 	return buf && !is_vmalloc_addr(buf) &&
@@ -1237,15 +1243,58 @@ static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat,
 	/* intentionally left blank */
 }
 
+static bool is_mtd_oops_in_progress(void)
+{
+	int i = 0;
+
+#ifdef CONFIG_MTD_OOPS
+	if (oops_in_progress && smp_processor_id()) {
+		int cpu = 0;
+
+		for_each_online_cpu(cpu)
+			++i;
+	}
+#endif
+	return i == 1 ? true : false;
+}
+
+static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
+{
+	struct brcmnand_host *host = nand_get_controller_data(chip);
+	struct brcmnand_controller *ctrl = host->ctrl;
+	bool err = false;
+	int sts;
+
+	if (is_mtd_oops_in_progress()) {
+		/* Switch to interrupt polling and PIO mode */
+		disable_flash_dma_xfer(ctrl);
+		sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
+					       NAND_STATUS_READY,
+					       NAND_CTRL_RDY |
+					       NAND_STATUS_READY, 0);
+		err = (sts < 0) ? true : false;
+	} else {
+		unsigned long timeo = msecs_to_jiffies(
+						NAND_POLL_STATUS_TIMEOUT_MS);
+		/* wait for completion interrupt */
+		sts = wait_for_completion_timeout(&ctrl->done, timeo);
+		err = (sts <= 0) ? true : false;
+	}
+
+	return err;
+}
+
 static int brcmnand_waitfunc(struct nand_chip *chip)
 {
 	struct brcmnand_host *host = nand_get_controller_data(chip);
 	struct brcmnand_controller *ctrl = host->ctrl;
-	unsigned long timeo = msecs_to_jiffies(100);
+	bool err = false;
 
 	dev_dbg(ctrl->dev, "wait on native cmd %d\n", ctrl->cmd_pending);
-	if (ctrl->cmd_pending &&
-			wait_for_completion_timeout(&ctrl->done, timeo) <= 0) {
+	if (ctrl->cmd_pending)
+		err = brcmstb_nand_wait_for_completion(chip);
+
+	if (err) {
 		u32 cmd = brcmnand_read_reg(ctrl, BRCMNAND_CMD_START)
 					>> brcmnand_cmd_shift(ctrl);
 
-- 
1.9.0.138.g2de3478

Re: [PATCH] mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling

[Miquel Raynal]

Hi Kamal,

Kamal Dasu <kdasu.kdev@gmail.com> wrote on Wed,  1 May 2019 13:46:15
-0400:

> If mtd_oops is in progress switch to polling for nand command completion

s/nand/NAND/

> interrupts and use PIO mode wihtout DMA so that the mtd_oops buffer can
> be completely written in the assinged nand partition.

What about:

"If mtd_oops is in progress, switch to polling during NAND command
completion instead of relying on DMA/interrupts so that the mtd_oops
buffer can be completely written in the assigned NAND partition."

> This is needed in
> cases where the panic does not happen on cpu0 and there is only one online
> CPU and the panic is not on cpu0.

"This is needed in case the panic does not happen on CPU0 and there is
only one online CPU."

I am not sure to understand the problem or how this can happen (and
be a problem). Have you met such issue already or is this purely
speculative?

> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 55 ++++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 482c6f0..cfbe51a 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -823,6 +823,12 @@ static inline bool has_flash_dma(struct brcmnand_controller *ctrl)
>  	return ctrl->flash_dma_base;
>  }
>  
> +static inline void disable_flash_dma_xfer(struct brcmnand_controller *ctrl)
> +{
> +	if (has_flash_dma(ctrl))
> +		ctrl->flash_dma_base = 0;
> +}
> +
>  static inline bool flash_dma_buf_ok(const void *buf)
>  {
>  	return buf && !is_vmalloc_addr(buf) &&
> @@ -1237,15 +1243,58 @@ static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat,
>  	/* intentionally left blank */
>  }
>  
> +static bool is_mtd_oops_in_progress(void)
> +{
> +	int i = 0;
> +
> +#ifdef CONFIG_MTD_OOPS
> +	if (oops_in_progress && smp_processor_id()) {
> +		int cpu = 0;
> +
> +		for_each_online_cpu(cpu)
> +			++i;
> +	}
> +#endif
> +	return i == 1 ? true : false;

I suppose return (i == 1); is enough

> +}
> +
> +static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
> +{
> +	struct brcmnand_host *host = nand_get_controller_data(chip);
> +	struct brcmnand_controller *ctrl = host->ctrl;
> +	bool err = false;
> +	int sts;
> +
> +	if (is_mtd_oops_in_progress()) {
> +		/* Switch to interrupt polling and PIO mode */
> +		disable_flash_dma_xfer(ctrl);
> +		sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
> +					       NAND_STATUS_READY,
> +					       NAND_CTRL_RDY |
> +					       NAND_STATUS_READY, 0);
> +		err = (sts < 0) ? true : false;
> +	} else {
> +		unsigned long timeo = msecs_to_jiffies(
> +						NAND_POLL_STATUS_TIMEOUT_MS);
> +		/* wait for completion interrupt */
> +		sts = wait_for_completion_timeout(&ctrl->done, timeo);
> +		err = (sts <= 0) ? true : false;
> +	}
> +
> +	return err;
> +}
> +
>  static int brcmnand_waitfunc(struct nand_chip *chip)
>  {
>  	struct brcmnand_host *host = nand_get_controller_data(chip);
>  	struct brcmnand_controller *ctrl = host->ctrl;
> -	unsigned long timeo = msecs_to_jiffies(100);
> +	bool err = false;
>  
>  	dev_dbg(ctrl->dev, "wait on native cmd %d\n", ctrl->cmd_pending);
> -	if (ctrl->cmd_pending &&
> -			wait_for_completion_timeout(&ctrl->done, timeo) <= 0) {

What about the wait_for_completion_timeout() call in brcmnand_write()?

> +	if (ctrl->cmd_pending)
> +		err = brcmstb_nand_wait_for_completion(chip);
> +
> +	if (err) {
>  		u32 cmd = brcmnand_read_reg(ctrl, BRCMNAND_CMD_START)
>  					>> brcmnand_cmd_shift(ctrl);  
>  


Thanks,
Miquèl

Re: [PATCH] mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling

[Kamal Dasu]

On Thu, May 2, 2019 at 4:25 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Kamal,
>
> Kamal Dasu <kdasu.kdev@gmail.com> wrote on Wed,  1 May 2019 13:46:15
> -0400:
>
> > If mtd_oops is in progress switch to polling for nand command completion
>
> s/nand/NAND/

Will change this.

>
> > interrupts and use PIO mode wihtout DMA so that the mtd_oops buffer can
> > be completely written in the assinged nand partition.
>
> What about:
>
> "If mtd_oops is in progress, switch to polling during NAND command
> completion instead of relying on DMA/interrupts so that the mtd_oops
> buffer can be completely written in the assigned NAND partition."
>

Will make this change as well

> > This is needed in
> > cases where the panic does not happen on cpu0 and there is only one online
> > CPU and the panic is not on cpu0.
>
> "This is needed in case the panic does not happen on CPU0 and there is
> only one online CPU."
>
> I am not sure to understand the problem or how this can happen (and
> be a problem). Have you met such issue already or is this purely
> speculative?

We have seen this issue and tested it on multi core SoCs.

>
> >
> > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> > ---
> >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 55 ++++++++++++++++++++++++++++++--
> >  1 file changed, 52 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > index 482c6f0..cfbe51a 100644
> > --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> > @@ -823,6 +823,12 @@ static inline bool has_flash_dma(struct brcmnand_controller *ctrl)
> >       return ctrl->flash_dma_base;
> >  }
> >
> > +static inline void disable_flash_dma_xfer(struct brcmnand_controller *ctrl)
> > +{
> > +     if (has_flash_dma(ctrl))
> > +             ctrl->flash_dma_base = 0;
> > +}
> > +
> >  static inline bool flash_dma_buf_ok(const void *buf)
> >  {
> >       return buf && !is_vmalloc_addr(buf) &&
> > @@ -1237,15 +1243,58 @@ static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat,
> >       /* intentionally left blank */
> >  }
> >
> > +static bool is_mtd_oops_in_progress(void)
> > +{
> > +     int i = 0;
> > +
> > +#ifdef CONFIG_MTD_OOPS
> > +     if (oops_in_progress && smp_processor_id()) {
> > +             int cpu = 0;
> > +
> > +             for_each_online_cpu(cpu)
> > +                     ++i;
> > +     }
> > +#endif
> > +     return i == 1 ? true : false;
>
> I suppose return (i == 1); is enough
>

Ok will make the change.

> > +}
> > +
> > +static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
> > +{
> > +     struct brcmnand_host *host = nand_get_controller_data(chip);
> > +     struct brcmnand_controller *ctrl = host->ctrl;
> > +     bool err = false;
> > +     int sts;
> > +
> > +     if (is_mtd_oops_in_progress()) {
> > +             /* Switch to interrupt polling and PIO mode */
> > +             disable_flash_dma_xfer(ctrl);
> > +             sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
> > +                                            NAND_STATUS_READY,
> > +                                            NAND_CTRL_RDY |
> > +                                            NAND_STATUS_READY, 0);
> > +             err = (sts < 0) ? true : false;
> > +     } else {
> > +             unsigned long timeo = msecs_to_jiffies(
> > +                                             NAND_POLL_STATUS_TIMEOUT_MS);
> > +             /* wait for completion interrupt */
> > +             sts = wait_for_completion_timeout(&ctrl->done, timeo);
> > +             err = (sts <= 0) ? true : false;
> > +     }
> > +
> > +     return err;
> > +}
> > +
> >  static int brcmnand_waitfunc(struct nand_chip *chip)
> >  {
> >       struct brcmnand_host *host = nand_get_controller_data(chip);
> >       struct brcmnand_controller *ctrl = host->ctrl;
> > -     unsigned long timeo = msecs_to_jiffies(100);
> > +     bool err = false;
> >
> >       dev_dbg(ctrl->dev, "wait on native cmd %d\n", ctrl->cmd_pending);
> > -     if (ctrl->cmd_pending &&
> > -                     wait_for_completion_timeout(&ctrl->done, timeo) <= 0) {
>
> What about the wait_for_completion_timeout() call in brcmnand_write()?
>

brcmnand_write() too calls brcmnand_waitfunc(), will poll if it needs
to for completion.

> > +     if (ctrl->cmd_pending)
> > +             err = brcmstb_nand_wait_for_completion(chip);
> > +
> > +     if (err) {
> >               u32 cmd = brcmnand_read_reg(ctrl, BRCMNAND_CMD_START)
> >                                       >> brcmnand_cmd_shift(ctrl);
> >
>
>
> Thanks,
> Miquèl

Thanks
Kamal

Re: [PATCH] mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling

[Richard Weinberger]

On Wed, May 1, 2019 at 7:52 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>
> If mtd_oops is in progress switch to polling for nand command completion
> interrupts and use PIO mode wihtout DMA so that the mtd_oops buffer can
> be completely written in the assinged nand partition. This is needed in
> cases where the panic does not happen on cpu0 and there is only one online
> CPU and the panic is not on cpu0.

This optimization is highly specific to your hardware and AFAIK cannot
be applied
in general to brcmnand.

So the problem you see is that depending on the oops you can no longer use dma
or interrupts in the driver?

How about adding a new flag to panic_nand_write() which tells the nand
driver that
this is a panic write?
That way you can fall back to pio and polling mode without checking cpu numbers
and oops_in_progress.

-- 
Thanks,
//richard

Re: [PATCH] mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling

[Kamal Dasu]

On Mon, May 6, 2019 at 12:01 PM Richard Weinberger
<richard.weinberger@gmail.com> wrote:
>
> On Wed, May 1, 2019 at 7:52 PM Kamal Dasu <kdasu.kdev@gmail.com> wrote:
> >
> > If mtd_oops is in progress switch to polling for nand command completion
> > interrupts and use PIO mode wihtout DMA so that the mtd_oops buffer can
> > be completely written in the assinged nand partition. This is needed in
> > cases where the panic does not happen on cpu0 and there is only one online
> > CPU and the panic is not on cpu0.
>
> This optimization is highly specific to your hardware and AFAIK cannot
> be applied
> in general to brcmnand.
>
> So the problem you see is that depending on the oops you can no longer use dma
> or interrupts in the driver?
>
> How about adding a new flag to panic_nand_write() which tells the nand
> driver that
> this is a panic write?
> That way you can fall back to pio and polling mode without checking cpu numbers
> and oops_in_progress.
>

Thanks for your review  Richard. Will add flag to let low level
controller drivers know that that its a panic_write and make brcmnand
code more generic and simply fallback to pio and polling in such a
case. Will send a V2 patch with these recommended changes.

Thanks
Kamal

> --
> Thanks,
> //richard