mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling
PATCH 1/1 mtd: nand: raw: brcmnand: When oops in progress use pio and interrupt polling
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.
--- drivers/mtd/nand/raw/brcmnand/brcmnand.c | 55 ++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +52 -3
@@ -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);
Signature · 1 line
1.9.0.138.g2de3478
Miquel Raynal
maintainerHi Kamal,
Kamal Dasu <[email protected]> wrote on Wed, 1 May 2019 13:46:15 -0400:
Quoted 1 line
If
mtd_oopsis in progress switch to polling for nand command completion
s/nand/NAND/
Quoted 2 lines
interrupts and use PIO mode wihtout DMA so that the
mtd_oopsbuffer 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."
Quoted 3 lines
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?
Quoted 30 lines
Signed-off-by Kamal Dasu <[email protected]>--- drivers/mtd/nand/raw/brcmnand/brcmnand.c | 55 ++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-)drivers/mtd/nand/raw/brcmnand/brcmnand.c +9
-0@@ -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 ;
drivers/mtd/nand/raw/brcmnand/brcmnand.c static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat, · line 1258
Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +10
-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
Quoted 28 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +28
-0+} + +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; +} +
drivers/mtd/nand/raw/brcmnand/brcmnand.c static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat, · line 1248
Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +1
-3static 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()?
drivers/mtd/nand/raw/brcmnand/brcmnand.c static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat, · line 1300
Quoted 7 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +4
-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);
Thanks,
Miquèl
Quoted 9 lines · Miquel Raynal
Hi Kamal,
Kamal Dasu <[email protected]> wrote on Wed, 1 May 2019 13:46:15 -0400:
Quoted 1 line
If
mtd_oopsis in progress switch to polling for nand command completions/nand/NAND/
Will change this.
Quoted 10 lines
Quoted 3 lines
interrupts and use PIO mode wihtout DMA so that the
mtd_oopsbuffer can be completely written in the assinged nand partition.What about:
"If
mtd_oopsis in progress, switch to polling during NAND command completion instead of relying on DMA/interrupts so that themtd_oopsbuffer can be completely written in the assigned NAND partition."
Will make this change as well
Quoted 10 lines
Quoted 3 lines
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.
Quoted 28 lines
Quoted 28 lines
Signed-off-by Kamal Dasu <[email protected]>--- drivers/mtd/nand/raw/brcmnand/brcmnand.c | 55 ++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-)drivers/mtd/nand/raw/brcmnand/brcmnand.c +6
-0@@ -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 */ }
Quoted 16 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat, · line 1258
Quoted 13 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +13
-0+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.
Quoted 35 lines
Quoted 35 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +29
-1+} + +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;
Quoted 6 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat, · line 1248
Quoted 3 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0
-2dev_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 inbrcmnand_write()?
brcmnand_write() too calls brcmnand_waitfunc(), will poll if it needs to for completion.
Quoted 11 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat, · line 1300
Quoted 7 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +4
-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);Thanks,
Miquèl
Thanks
Kamal
Quoted 6 lines · Kamal Dasu
If
mtd_oopsis in progress switch to polling for nand command completion interrupts and use PIO mode wihtout DMA so that themtd_oopsbuffer 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.
Signature · 2 lines
Thanks, //richard
Kamal Dasu
maintainerQuoted 13 lines · Richard Weinberger
Quoted 6 lines · Kamal Dasu
If
mtd_oopsis in progress switch to polling for nand command completion interrupts and use PIO mode wihtout DMA so that themtd_oopsbuffer 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.
Quoted 9 lines
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 andoops_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
Quoted 3 lines
--
Thanks,
//richard