mtd: rawnand: marvell: remove bogus comment in marvell_nfc_select_chip()
PATCH 1/3 mtd: rawnand: marvell: remove bogus comment in marvell_nfc_select_chip()
The comment in marvell_nfc_select_chip() about ndtr0 and ndtr1 didn't reflect what the driver was doing.
The values of NDTR0 and NDTR1 are read from the registers at probe time and a copy is retained in 'struct marvell_nand_chip'. If keep-config is set in the DT properties, there are no other writers of these timing variables so they can safely be used when the chip is selected.
As suggested by Miquel Raynal, simply remove the comment.
--- drivers/mtd/nand/raw/marvell_nand.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/mtd/nand/raw/marvell_nand.c +0 -5
@@ -650,11 +650,6 @@ static void marvell_nfc_select_chip(struct mtd_info *mtd, int die_nr)return ; } -/* -* Do not change the timing registers when using the DT property -* marvell,nand-keep-config; in that case ->ndtr0 and ->ndtr1 from the -* marvell_nand structure are supposedly empty. -*/ writel_relaxed (marvell_nand->ndtr0, nfc->regs +NDTR0 );writel_relaxed (marvell_nand->ndtr1, nfc->regs +NDTR1 );
Signature · 1 line
2.17.1
PATCH 2/3 mtd: rawnand: marvell: set reg_clk to NULL if it can't be obtained
Don't keep an error-pointer around in the private struct. If this optional clock can't be obtained, simply set the pointer to NULL instead so we can use clk_* functions on it later. These functions can cope with NULL, but not with error-pointers.
--- drivers/mtd/nand/raw/marvell_nand.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/mtd/nand/raw/marvell_nand.c +10 -6
@@ -2752,15 +2752,19 @@ static int marvell_nfc_probe(struct platform_device *pdev)return ret; nfc->reg_clk =devm_clk_get (&pdev->dev,"reg" ); -if (PTR_ERR (nfc->reg_clk) != -ENOENT ) { -if (!IS_ERR (nfc->reg_clk)) { - ret =clk_prepare_enable (nfc->reg_clk); -if (ret) - goto unprepare_core_clk; - }else { +if (IS_ERR (nfc->reg_clk)) { +if (PTR_ERR (nfc->reg_clk) != -ENOENT ) { ret =PTR_ERR (nfc->reg_clk); goto unprepare_core_clk; } + + nfc->reg_clk =NULL ; + } + +if (nfc->reg_clk) { + ret =clk_prepare_enable (nfc->reg_clk); +if (ret) + goto unprepare_core_clk; }marvell_nfc_disable_int (nfc,NDCR_ALL_INT );
Signature · 1 line
2.17.1
PATCH 3/3 mtd: rawnand: marvell: add suspend and resume hooks
In reply to Daniel MackThis patch restores the suspend and resume hooks that the old driver used to have. Apart from stopping and starting the clocks, the resume callback also nullifies the selected_chip pointer, so the next command that is issued will re-select the chip and thereby restore the timing registers.
Without this patch, a PXA3xx based system would cough up an error similar to the one below after resume.
[ 44.660162] marvell-nfc 43100000.nand-controller: Timeout waiting for RB signal [ 44.671492] ubi0 error: ubi_io_write: error -110 while writing 2048 bytes to PEB 102:38912, written 0 bytes [ 44.682887] CPU: 0 PID: 1417 Comm: remote-control Not tainted 4.18.0-rc2+ #344 [ 44.691197] Hardware name: Marvell PXA3xx (Device Tree Support) [ 44.697111] Backtrace: [ 44.699593] [<c0106458>] (dump_backtrace) from [<c0106718>] (show_stack+0x18/0x1c) [ 44.708931] r7:00000800 r6:00009800 r5:00000066 r4:c6139000 [ 44.715833] [<c0106700>] (show_stack) from [<c0678a60>] (dump_stack+0x20/0x28) [ 44.724206] [<c0678a40>] (dump_stack) from [<c0456cbc>] (ubi_io_write+0x3d4/0x630) [ 44.732925] [<c04568e8>] (ubi_io_write) from [<c0454428>] (ubi_eba_write_leb+0x690/0x6fc)
...
--- drivers/mtd/nand/raw/marvell_nand.c | 47 +++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
drivers/mtd/nand/raw/marvell_nand.c +47 -0
@@ -2824,6 +2824,52 @@ static int marvell_nfc_remove(struct platform_device *pdev)return 0 ; } +static int __maybe_unused marvell_nfc_suspend (struct device *dev) +{ +struct marvell_nfc *nfc =dev_get_drvdata (dev); +struct marvell_nand_chip *chip; + +list_for_each_entry (chip, &nfc->chips, node) +marvell_nfc_wait_ndrun (&chip->chip); + +clk_disable_unprepare (nfc->reg_clk); +clk_disable_unprepare (nfc->core_clk); + +return 0 ; +} + +static int __maybe_unused marvell_nfc_resume (struct device *dev) +{ +struct marvell_nfc *nfc =dev_get_drvdata (dev); +int ret; + + ret =clk_prepare_enable (nfc->core_clk); +if (ret <0 ) +return ret; + + ret =clk_prepare_enable (nfc->reg_clk); +if (ret <0 ) +return ret; + +/* +* Reset nfc->selected_chip so the next command will cause the timing +* registers to be restored in marvell_nfc_select_chip(). +*/ + nfc->selected_chip =NULL ; + +/* Reset registers that have lots its contents */ +writel_relaxed (NDCR_ALL_INT |NDCR_ND_ARB_EN |NDCR_SPARE_EN | +NDCR_RD_ID_CNT (NFCV1_READID_LEN ), nfc->regs +NDCR ); +writel_relaxed (0xFFFFFFFF , nfc->regs +NDSR ); +writel_relaxed (0 , nfc->regs +NDECCCTRL ); + +return 0 ; +} + +static const struct dev_pm_ops marvell_nfc_pm_ops = { +SET_SYSTEM_SLEEP_PM_OPS (marvell_nfc_suspend, marvell_nfc_resume) +}; +static const struct marvell_nfc_caps marvell_armada_8k_nfc_caps = { .max_cs_nb =4 , .max_rb_nb =2 ,
@@ -2908,6 +2954,7 @@ static struct platform_driver marvell_nfc_driver = { .driver = { .name = "marvell-nfc", .of_match_table = marvell_nfc_of_ids, + .pm = &marvell_nfc_pm_ops, }, .id_table = marvell_nfc_platform_ids, .probe = marvell_nfc_probe,
Signature · 1 line
2.17.1
Re: [PATCH 3/3] mtd: rawnand: marvell: add suspend and resume hooks
Quoted 18 lines · On Fri, 6 Jul 2018 22:14:15 +0200 Daniel Mack <[email protected]> wrote:
This patch restores the suspend and resume hooks that the old driver used to have. Apart from stopping and starting the clocks, the resume callback also nullifies the
selected_chippointer, so the next command that is issued will re-select the chip and thereby restore the timing registers.Without this patch, a PXA3xx based system would cough up an error similar to the one below after resume.
[ 44.660162] marvell-nfc 43100000.nand-controller: Timeout waiting for RB signal [ 44.671492] ubi0 error: ubi_io_write: error -110 while writing 2048 bytes to PEB 102:38912, written 0 bytes [ 44.682887] CPU: 0 PID: 1417 Comm: remote-control Not tainted 4.18.0-rc2+ #344 [ 44.691197] Hardware name: Marvell PXA3xx (Device Tree Support) [ 44.697111] Backtrace: [ 44.699593] [<c0106458>] (dump_backtrace) from [<c0106718>] (show_stack+0x18/0x1c) [ 44.708931] r7:00000800 r6:00009800 r5:00000066 r4:c6139000 [ 44.715833] [<c0106700>] (show_stack) from [<c0678a60>] (dump_stack+0x20/0x28) [ 44.724206] [<c0678a40>] (dump_stack) from [<c0456cbc>] (ubi_io_write+0x3d4/0x630) [ 44.732925] [<c04568e8>] (ubi_io_write) from [<c0454428>] (ubi_eba_write_leb+0x690/0x6fc)
Quoted 3 lines
...
Signed-off-by Daniel Mack <[email protected]>
You probably want patch 2 and 3 backported to stable. Would be better if we don't need patch 2 though, since this is not exactly a fix. Maybe you can re-order things and add IS_ERR() tests in the suspend/resume funcs, which you'll then remove but this time, the cleanup patch won't be flagged "for stable".
Quoted 69 lines
--- drivers/mtd/nand/raw/marvell_nand.c | 47 +++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)drivers/mtd/nand/raw/marvell_nand.c +47
-0@@ -2824,6 +2824,52 @@ static int marvell_nfc_remove(struct platform_device *pdev)return 0 ; } +static int __maybe_unused marvell_nfc_suspend (struct device *dev) +{ +struct marvell_nfc *nfc =dev_get_drvdata (dev); +struct marvell_nand_chip *chip; + +list_for_each_entry (chip, &nfc->chips, node) +marvell_nfc_wait_ndrun (&chip->chip); + +clk_disable_unprepare (nfc->reg_clk); +clk_disable_unprepare (nfc->core_clk); + +return 0 ; +} + +static int __maybe_unused marvell_nfc_resume (struct device *dev) +{ +struct marvell_nfc *nfc =dev_get_drvdata (dev); +int ret; + + ret =clk_prepare_enable (nfc->core_clk); +if (ret <0 ) +return ret; + + ret =clk_prepare_enable (nfc->reg_clk); +if (ret <0 ) +return ret; + +/* +* Reset nfc->selected_chip so the next command will cause the timing +* registers to be restored in marvell_nfc_select_chip(). +*/ + nfc->selected_chip =NULL ; + +/* Reset registers that have lots its contents */ +writel_relaxed (NDCR_ALL_INT |NDCR_ND_ARB_EN |NDCR_SPARE_EN | +NDCR_RD_ID_CNT (NFCV1_READID_LEN ), nfc->regs +NDCR ); +writel_relaxed (0xFFFFFFFF , nfc->regs +NDSR ); +writel_relaxed (0 , nfc->regs +NDECCCTRL ); + +return 0 ; +} + +static const struct dev_pm_ops marvell_nfc_pm_ops = { +SET_SYSTEM_SLEEP_PM_OPS (marvell_nfc_suspend, marvell_nfc_resume) +}; +static const struct marvell_nfc_caps marvell_armada_8k_nfc_caps = { .max_cs_nb =4 , .max_rb_nb =2 ,@@ -2908,6 +2954,7 @@ static struct platform_driver marvell_nfc_driver = { .driver = { .name = "marvell-nfc", .of_match_table = marvell_nfc_of_ids, + .pm = &marvell_nfc_pm_ops, }, .id_table = marvell_nfc_platform_ids, .probe = marvell_nfc_probe,
Re: [PATCH 3/3] mtd: rawnand: marvell: add suspend and resume hooks
Hi,
Quoted 23 lines · Boris Brezillon
Quoted 20 lines · On Fri, 6 Jul 2018 22:14:15 +0200 Daniel Mack <[email protected]> wrote:
This patch restores the suspend and resume hooks that the old driver used to have. Apart from stopping and starting the clocks, the resume callback also nullifies the
selected_chippointer, so the next command that is issued will re-select the chip and thereby restore the timing registers.Without this patch, a PXA3xx based system would cough up an error similar to the one below after resume.
[ 44.660162] marvell-nfc 43100000.nand-controller: Timeout waiting for RB signal [ 44.671492] ubi0 error: ubi_io_write: error -110 while writing 2048 bytes to PEB 102:38912, written 0 bytes [ 44.682887] CPU: 0 PID: 1417 Comm: remote-control Not tainted 4.18.0-rc2+ #344 [ 44.691197] Hardware name: Marvell PXA3xx (Device Tree Support) [ 44.697111] Backtrace: [ 44.699593] [<c0106458>] (dump_backtrace) from [<c0106718>] (show_stack+0x18/0x1c) [ 44.708931] r7:00000800 r6:00009800 r5:00000066 r4:c6139000 [ 44.715833] [<c0106700>] (show_stack) from [<c0678a60>] (dump_stack+0x20/0x28) [ 44.724206] [<c0678a40>] (dump_stack) from [<c0456cbc>] (ubi_io_write+0x3d4/0x630) [ 44.732925] [<c04568e8>] (ubi_io_write) from [<c0454428>] (ubi_eba_write_leb+0x690/0x6fc)...
Quoted 3 lines
Quoted 1 line
Signed-off-by Daniel Mack <[email protected]>You probably want patch 2 and 3 backported to stable.
Given that nobody has cared so far and the only board that depends on proper PM that seems to be using this driver has bitrot quite badly in the past and is undergoing a major rewrite currently, I'm not sure whether it's worth it really.
I can do it if you insist though.
Thanks,
Daniel
Re: [PATCH 3/3] mtd: rawnand: marvell: add suspend and resume hooks
Quoted 26 lines · Daniel Mack
Hi,
Quoted 23 lines · Boris Brezillon
Quoted 20 lines · On Fri, 6 Jul 2018 22:14:15 +0200 Daniel Mack <[email protected]> wrote:
This patch restores the suspend and resume hooks that the old driver used to have. Apart from stopping and starting the clocks, the resume callback also nullifies the
selected_chippointer, so the next command that is issued will re-select the chip and thereby restore the timing registers.Without this patch, a PXA3xx based system would cough up an error similar to the one below after resume.
[ 44.660162] marvell-nfc 43100000.nand-controller: Timeout waiting for RB signal [ 44.671492] ubi0 error: ubi_io_write: error -110 while writing 2048 bytes to PEB 102:38912, written 0 bytes [ 44.682887] CPU: 0 PID: 1417 Comm: remote-control Not tainted 4.18.0-rc2+ #344 [ 44.691197] Hardware name: Marvell PXA3xx (Device Tree Support) [ 44.697111] Backtrace: [ 44.699593] [<c0106458>] (dump_backtrace) from [<c0106718>] (show_stack+0x18/0x1c) [ 44.708931] r7:00000800 r6:00009800 r5:00000066 r4:c6139000 [ 44.715833] [<c0106700>] (show_stack) from [<c0678a60>] (dump_stack+0x20/0x28) [ 44.724206] [<c0678a40>] (dump_stack) from [<c0456cbc>] (ubi_io_write+0x3d4/0x630) [ 44.732925] [<c04568e8>] (ubi_io_write) from [<c0454428>] (ubi_eba_write_leb+0x690/0x6fc)...
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Daniel Mack <[email protected]>You probably want patch 2 and 3 backported to stable.
Given that nobody has cared so far and the only board that depends on proper PM that seems to be using this driver has bitrot quite badly in the past and is undergoing a major rewrite currently, I'm not sure whether it's worth it really.
Ah, I only see this now, but patch 2 also fixes a problem with the .remove() callback of this driver which also blindly grabs ->reg_clk without further checks.
Hence the entire series actually qualifies for stable@ I figure?
Thanks,
Daniel
Re: [PATCH 3/3] mtd: rawnand: marvell: add suspend and resume hooks
Hi Daniel,
Quoted 31 lines · On Sat, 7 Jul 2018 00:26:22 +0200 Daniel Mack <[email protected]> wrote:
Quoted 30 lines · Daniel Mack
Hi,
Quoted 26 lines · Boris Brezillon
Quoted 18 lines · On Fri, 6 Jul 2018 22:14:15 +0200 Daniel Mack <[email protected]> wrote:
This patch restores the suspend and resume hooks that the old driver used to have. Apart from stopping and starting the clocks, the resume callback also nullifies the
selected_chippointer, so the next command that is issued will re-select the chip and thereby restore the timing registers.Without this patch, a PXA3xx based system would cough up an error similar to the one below after resume.
[ 44.660162] marvell-nfc 43100000.nand-controller: Timeout waiting for RB signal [ 44.671492] ubi0 error: ubi_io_write: error -110 while writing 2048 bytes to PEB 102:38912, written 0 bytes [ 44.682887] CPU: 0 PID: 1417 Comm: remote-control Not tainted 4.18.0-rc2+ #344 [ 44.691197] Hardware name: Marvell PXA3xx (Device Tree Support) [ 44.697111] Backtrace: [ 44.699593] [<c0106458>] (dump_backtrace) from [<c0106718>] (show_stack+0x18/0x1c) [ 44.708931] r7:00000800 r6:00009800 r5:00000066 r4:c6139000 [ 44.715833] [<c0106700>] (show_stack) from [<c0678a60>] (dump_stack+0x20/0x28) [ 44.724206] [<c0678a40>] (dump_stack) from [<c0456cbc>] (ubi_io_write+0x3d4/0x630) [ 44.732925] [<c04568e8>] (ubi_io_write) from [<c0454428>] (ubi_eba_write_leb+0x690/0x6fc)Quoted 3 lines
...
Signed-off-by Daniel Mack <[email protected]>You probably want patch 2 and 3 backported to stable.
Quoted 8 lines
Quoted 4 lines
Given that nobody has cared so far and the only board that depends on proper PM that seems to be using this driver has bitrot quite badly in the past and is undergoing a major rewrite currently, I'm not sure whether it's worth it really.
Ah, I only see this now, but patch 2 also fixes a problem with the .remove() callback of this driver which also blindly grabs ->reg_clk without further checks.
Nope, because the clk framework checks for both ERR and NULL (see [1]). I'm definitely not arguing that patch 2 is not needed (actually I pushed for this solution when Greg initially added these new clks [2]), just that it should not be flagged as stable.
Quoted 2 lines
Hence the entire series actually qualifies for stable@ I figure?
I'd really prefer to have a single patch go into stable. Patch 1 is clearly not a bug fix, and patch 2 is just a dependency of patch 3, so let's remove this dependency by either squashing both patches into a single one or by reordering the changes.
Regards,
Boris
[1]https://elixir.bootlin.com/linux/v4.18-rc3/source/drivers/clk/clk.c#L858 [2]https://www.spinics.net/lists/arm-kernel/msg639312.html
Re: [PATCH 3/3] mtd: rawnand: marvell: add suspend and resume hooks
Quoted 51 lines · On Sat, 7 Jul 2018 08:07:13 +0200 Boris Brezillon…
Hi Daniel,
Quoted 31 lines · On Sat, 7 Jul 2018 00:26:22 +0200 Daniel Mack <[email protected]> wrote:
Quoted 30 lines · Daniel Mack
Hi,
Quoted 26 lines · Boris Brezillon
Quoted 18 lines · On Fri, 6 Jul 2018 22:14:15 +0200 Daniel Mack <[email protected]> wrote:
This patch restores the suspend and resume hooks that the old driver used to have. Apart from stopping and starting the clocks, the resume callback also nullifies the
selected_chippointer, so the next command that is issued will re-select the chip and thereby restore the timing registers.Without this patch, a PXA3xx based system would cough up an error similar to the one below after resume.
[ 44.660162] marvell-nfc 43100000.nand-controller: Timeout waiting for RB signal [ 44.671492] ubi0 error: ubi_io_write: error -110 while writing 2048 bytes to PEB 102:38912, written 0 bytes [ 44.682887] CPU: 0 PID: 1417 Comm: remote-control Not tainted 4.18.0-rc2+ #344 [ 44.691197] Hardware name: Marvell PXA3xx (Device Tree Support) [ 44.697111] Backtrace: [ 44.699593] [<c0106458>] (dump_backtrace) from [<c0106718>] (show_stack+0x18/0x1c) [ 44.708931] r7:00000800 r6:00009800 r5:00000066 r4:c6139000 [ 44.715833] [<c0106700>] (show_stack) from [<c0678a60>] (dump_stack+0x20/0x28) [ 44.724206] [<c0678a40>] (dump_stack) from [<c0456cbc>] (ubi_io_write+0x3d4/0x630) [ 44.732925] [<c04568e8>] (ubi_io_write) from [<c0454428>] (ubi_eba_write_leb+0x690/0x6fc)Quoted 3 lines
...
Signed-off-by Daniel Mack <[email protected]>You probably want patch 2 and 3 backported to stable.
Quoted 8 lines
Quoted 4 lines
Given that nobody has cared so far and the only board that depends on proper PM that seems to be using this driver has bitrot quite badly in the past and is undergoing a major rewrite currently, I'm not sure whether it's worth it really.
Ah, I only see this now, but patch 2 also fixes a problem with the .remove() callback of this driver which also blindly grabs ->reg_clk without further checks.
Nope, because the clk framework checks for both ERR and NULL (see [1]). I'm definitely not arguing that patch 2 is not needed (actually I pushed for this solution when Greg initially added these new clks [2]), just that it should not be flagged as stable.
Quoted 1 line
Quoted 6 lines
Quoted 1 line
Hence the entire series actually qualifies for stable@ I figure?
I'd really prefer to have a single patch go into stable. Patch 1 is clearly not a bug fix, and patch 2 is just a dependency of patch 3, so let's remove this dependency by either squashing both patches into a single one or by reordering the changes.
Forgot to mention that you should also add
Re: [PATCH 3/3] mtd: rawnand: marvell: add suspend and resume hooks
In reply to Boris BrezillonHi Boris,
Quoted 11 lines · Boris Brezillon
Quoted 3 lines · On Sat, 7 Jul 2018 00:26:22 +0200 Daniel Mack <[email protected]> wrote:
Ah, I only see this now, but patch 2 also fixes a problem with the .remove() callback of this driver which also blindly grabs ->reg_clk without further checks.
Nope, because the clk framework checks for both ERR and NULL (see [1]). I'm definitely not arguing that patch 2 is not needed (actually I pushed for this solution when Greg initially added these new clks [2]), just that it should not be flagged as stable.
Ah, I missed that! Sorry.
Quoted 6 lines
Quoted 1 line
Hence the entire series actually qualifies for stable@ I figure?
I'd really prefer to have a single patch go into stable. Patch 1 is clearly not a bug fix, and patch 2 is just a dependency of patch 3, so let's remove this dependency by either squashing both patches into a single one or by reordering the changes.
Okay then. Hang on, I'll send a v2.
Thanks,
Daniel