Skip to contentSkip to conversation

mtd: pxa3xx_nand: don't duplicate MTD suspend/resume

PATCH In review Brian Norris 3 participants 11 messages Oct 12, 2015 – Oct 20, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/2 mtd: pxa3xx_nand: don't duplicate MTD suspend/resume

mtd_{suspend,resume}() get called from mtdcore in a class suspend/resume callback. We don't need to call them again here. In practice, this would actually work OK, as nand_base actually handles nesting OK -- it just might print warnings.

Untested, but there are few (no?) users of PM for this driver AFAIK.

---
 drivers/mtd/nand/pxa3xx_nand.c | 17 -----------------
 1 file changed, 17 deletions(-)
drivers/mtd/nand/pxa3xx_nand.c +0 -17
index 232c7074624a..cce5a32eef60 100644
@@ -1892,32 +1892,19 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 static int pxa3xx_nand_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
-	struct pxa3xx_nand_platform_data *pdata;
-	struct mtd_info *mtd;
-	int cs;
 
-	pdata = dev_get_platdata(&pdev->dev);
 	if (info->state) {
 		dev_err(&pdev->dev, "driver busy, state = %d\n", info->state);
 		return -EAGAIN;
 	}
 
-	for (cs = 0; cs < pdata->num_cs; cs++) {
-		mtd = info->host[cs]->mtd;
-		mtd_suspend(mtd);
-	}
-
 	return 0;
 }
 
 static int pxa3xx_nand_resume(struct platform_device *pdev)
 {
 	struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
-	struct pxa3xx_nand_platform_data *pdata;
-	struct mtd_info *mtd;
-	int cs;
 
-	pdata = dev_get_platdata(&pdev->dev);
 	/* We don't want to handle interrupt without calling mtd routine */
 	disable_int(info, NDCR_INT_MASK);
 
@@ -1935,10 +1922,6 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
 	 * all status before resume
 	 */
 	nand_writel(info, NDSR, NDSR_MASK);
-	for (cs = 0; cs < pdata->num_cs; cs++) {
-		mtd = info->host[cs]->mtd;
-		mtd_resume(mtd);
-	}
 
 	return 0;
 }
Signature · 1 line
2.6.0.rc2.230.g3dd15c0
#1 lore raw

PATCH 2/2 mtd: pxa3xx_nand: switch to device PM

The old PM model is deprecated. This is equivalent.

---
 drivers/mtd/nand/pxa3xx_nand.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
drivers/mtd/nand/pxa3xx_nand.c +9 -7
index cce5a32eef60..e074a3b406f6 100644
@@ -1889,21 +1889,21 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int pxa3xx_nand_suspend(struct platform_device *pdev, pm_message_t state)
+static int pxa3xx_nand_suspend(struct device *dev)
 {
-	struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
+	struct pxa3xx_nand_info *info = dev_get_drvdata(dev);
 
 	if (info->state) {
-		dev_err(&pdev->dev, "driver busy, state = %d\n", info->state);
+		dev_err(dev, "driver busy, state = %d\n", info->state);
 		return -EAGAIN;
 	}
 
 	return 0;
 }
 
-static int pxa3xx_nand_resume(struct platform_device *pdev)
+static int pxa3xx_nand_resume(struct device *dev)
 {
-	struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
+	struct pxa3xx_nand_info *info = dev_get_drvdata(dev);
 
 	/* We don't want to handle interrupt without calling mtd routine */
 	disable_int(info, NDCR_INT_MASK);
@@ -1934,11 +1934,13 @@ static struct platform_driver pxa3xx_nand_driver = {
 	.driver = {
 		.name	= "pxa3xx-nand",
 		.of_match_table = pxa3xx_nand_dt_ids,
+		.pm = {
+			.suspend	= pxa3xx_nand_suspend,
+			.resume		= pxa3xx_nand_resume,
+		},
 	},
 	.probe		= pxa3xx_nand_probe,
 	.remove		= pxa3xx_nand_remove,
-	.suspend	= pxa3xx_nand_suspend,
-	.resume		= pxa3xx_nand_resume,
 };
 
 module_platform_driver(pxa3xx_nand_driver);
Signature · 1 line
2.6.0.rc2.230.g3dd15c0
#2 lore raw

Re: [PATCH 2/2] mtd: pxa3xx_nand: switch to device PM

Quoted 3 lines · Brian Norris

The old PM model is deprecated. This is equivalent.

Sorry, this patch was garbage. Below is the correct one:

---

---
 drivers/mtd/nand/pxa3xx_nand.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/mtd/nand/pxa3xx_nand.c +11 -7
index cce5a32eef60..51b67ae5ac1c 100644
@@ -1889,21 +1889,21 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int pxa3xx_nand_suspend(struct platform_device *pdev, pm_message_t state)
+static int pxa3xx_nand_suspend(struct device *dev)
 {
-	struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
+	struct pxa3xx_nand_info *info = dev_get_drvdata(dev);
 
 	if (info->state) {
-		dev_err(&pdev->dev, "driver busy, state = %d\n", info->state);
+		dev_err(dev, "driver busy, state = %d\n", info->state);
 		return -EAGAIN;
 	}
 
 	return 0;
 }
 
-static int pxa3xx_nand_resume(struct platform_device *pdev)
+static int pxa3xx_nand_resume(struct device *dev)
 {
-	struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
+	struct pxa3xx_nand_info *info = dev_get_drvdata(dev);
 
 	/* We don't want to handle interrupt without calling mtd routine */
 	disable_int(info, NDCR_INT_MASK);
@@ -1930,15 +1930,19 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
 #define pxa3xx_nand_resume	NULL
 #endif
 
+static const struct dev_pm_ops pxa3xx_nand_pm_ops = {
+	.suspend	= pxa3xx_nand_suspend,
+	.resume		= pxa3xx_nand_resume,
+};
+
 static struct platform_driver pxa3xx_nand_driver = {
 	.driver = {
 		.name	= "pxa3xx-nand",
 		.of_match_table = pxa3xx_nand_dt_ids,
+		.pm	= &pxa3xx_nand_pm_ops,
 	},
 	.probe		= pxa3xx_nand_probe,
 	.remove		= pxa3xx_nand_remove,
-	.suspend	= pxa3xx_nand_suspend,
-	.resume		= pxa3xx_nand_resume,
 };
 
 module_platform_driver(pxa3xx_nand_driver);
Signature · 1 line
2.6.0.rc2.230.g3dd15c0
#3 lore raw

Re: [PATCH 2/2] mtd: pxa3xx_nand: switch to device PM

+Robert

Quoted 74 lines · Brian Norris
Quoted 3 lines · Brian Norris

The old PM model is deprecated. This is equivalent.

Sorry, this patch was garbage. Below is the correct one:

---

---
 drivers/mtd/nand/pxa3xx_nand.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/mtd/nand/pxa3xx_nand.c +11 -8
index cce5a32eef60..51b67ae5ac1c 100644
@@ -1889,21 +1889,21 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int pxa3xx_nand_suspend(struct platform_device *pdev, pm_message_t state)
+static int pxa3xx_nand_suspend(struct device *dev)
 {
-       struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
+       struct pxa3xx_nand_info *info = dev_get_drvdata(dev);
 
        if (info->state) {
-               dev_err(&pdev->dev, "driver busy, state = %d\n", info->state);
+               dev_err(dev, "driver busy, state = %d\n", info->state);
                return -EAGAIN;
        }
 
        return 0;
 }
 
-static int pxa3xx_nand_resume(struct platform_device *pdev)
+static int pxa3xx_nand_resume(struct device *dev)
 {
-       struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
+       struct pxa3xx_nand_info *info = dev_get_drvdata(dev);
 
        /* We don't want to handle interrupt without calling mtd routine */
        disable_int(info, NDCR_INT_MASK);
@@ -1930,15 +1930,19 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
 #define pxa3xx_nand_resume     NULL
 #endif
 
+static const struct dev_pm_ops pxa3xx_nand_pm_ops = {
+       .suspend        = pxa3xx_nand_suspend,
+       .resume         = pxa3xx_nand_resume,
+};
+
 static struct platform_driver pxa3xx_nand_driver = {
        .driver = {
                .name   = "pxa3xx-nand",
                .of_match_table = pxa3xx_nand_dt_ids,
+               .pm     = &pxa3xx_nand_pm_ops,
        },
        .probe          = pxa3xx_nand_probe,
        .remove         = pxa3xx_nand_remove,
-       .suspend        = pxa3xx_nand_suspend,
-       .resume         = pxa3xx_nand_resume,
 };
 
 module_platform_driver(pxa3xx_nand_driver);
--

2.6.0.rc2.230.g3dd15c0

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Signature · 2 lines
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
Tested-by Acked-by
#4 lore raw

Re: [PATCH 2/2] mtd: pxa3xx_nand: switch to device PM

Quoted 10 lines · Ezequiel Garcia

+Robert

Quoted 7 lines · Brian Norris
Quoted 3 lines · Brian Norris

The old PM model is deprecated. This is equivalent.

Sorry, this patch was garbage. Below is the correct one:

Tested on Armada XP-GP using standby and checking nandtest continues to work after a resume.

Patch looks good, so:

Thanks!

Quoted 73 lines
Quoted 67 lines

---

---
 drivers/mtd/nand/pxa3xx_nand.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
drivers/mtd/nand/pxa3xx_nand.c +11 -8
index cce5a32eef60..51b67ae5ac1c 100644
@@ -1889,21 +1889,21 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int pxa3xx_nand_suspend(struct platform_device *pdev, pm_message_t state)
+static int pxa3xx_nand_suspend(struct device *dev)
 {
-       struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
+       struct pxa3xx_nand_info *info = dev_get_drvdata(dev);
 
        if (info->state) {
-               dev_err(&pdev->dev, "driver busy, state = %d\n", info->state);
+               dev_err(dev, "driver busy, state = %d\n", info->state);
                return -EAGAIN;
        }
 
        return 0;
 }
 
-static int pxa3xx_nand_resume(struct platform_device *pdev)
+static int pxa3xx_nand_resume(struct device *dev)
 {
-       struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
+       struct pxa3xx_nand_info *info = dev_get_drvdata(dev);
 
        /* We don't want to handle interrupt without calling mtd routine */
        disable_int(info, NDCR_INT_MASK);
@@ -1930,15 +1930,19 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
 #define pxa3xx_nand_resume     NULL
 #endif
 
+static const struct dev_pm_ops pxa3xx_nand_pm_ops = {
+       .suspend        = pxa3xx_nand_suspend,
+       .resume         = pxa3xx_nand_resume,
+};
+
 static struct platform_driver pxa3xx_nand_driver = {
        .driver = {
                .name   = "pxa3xx-nand",
                .of_match_table = pxa3xx_nand_dt_ids,
+               .pm     = &pxa3xx_nand_pm_ops,
        },
        .probe          = pxa3xx_nand_probe,
        .remove         = pxa3xx_nand_remove,
-       .suspend        = pxa3xx_nand_suspend,
-       .resume         = pxa3xx_nand_resume,
 };
 
 module_platform_driver(pxa3xx_nand_driver);
--

2.6.0.rc2.230.g3dd15c0

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Signature · 2 lines
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
Signature · 2 lines
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
author Applied
#5 lore raw

Re: [PATCH 2/2] mtd: pxa3xx_nand: switch to device PM

Quoted 15 lines · Ezequiel Garcia
Quoted 10 lines · Ezequiel Garcia

+Robert

Quoted 7 lines · Brian Norris
Quoted 3 lines · Brian Norris

The old PM model is deprecated. This is equivalent.

Sorry, this patch was garbage. Below is the correct one:

Tested on Armada XP-GP using standby and checking nandtest continues to work after a resume.

Quoted 7 lines

Patch looks good, so:

Thanks!

Pushed to l2-mtd.git

#6 lore raw
In reply to Brian Norris

Robert:

Can you test this on a platform with PM support, and make sure the NAND works fine after a resume?

Quoted 67 lines · Brian Norris

mtd_{suspend,resume}() get called from mtdcore in a class suspend/resume callback. We don't need to call them again here. In practice, this would actually work OK, as nand_base actually handles nesting OK -- it just might print warnings.

Untested, but there are few (no?) users of PM for this driver AFAIK.

---
 drivers/mtd/nand/pxa3xx_nand.c | 17 -----------------
 1 file changed, 17 deletions(-)
drivers/mtd/nand/pxa3xx_nand.c +0 -18
index 232c7074624a..cce5a32eef60 100644
@@ -1892,32 +1892,19 @@ static int pxa3xx_nand_probe(struct platform_device *pdev)
 static int pxa3xx_nand_suspend(struct platform_device *pdev, pm_message_t state)
 {
        struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
-       struct pxa3xx_nand_platform_data *pdata;
-       struct mtd_info *mtd;
-       int cs;
 
-       pdata = dev_get_platdata(&pdev->dev);
        if (info->state) {
                dev_err(&pdev->dev, "driver busy, state = %d\n", info->state);
                return -EAGAIN;
        }
 
-       for (cs = 0; cs < pdata->num_cs; cs++) {
-               mtd = info->host[cs]->mtd;
-               mtd_suspend(mtd);
-       }
-
        return 0;
 }
 
 static int pxa3xx_nand_resume(struct platform_device *pdev)
 {
        struct pxa3xx_nand_info *info = platform_get_drvdata(pdev);
-       struct pxa3xx_nand_platform_data *pdata;
-       struct mtd_info *mtd;
-       int cs;
 
-       pdata = dev_get_platdata(&pdev->dev);
        /* We don't want to handle interrupt without calling mtd routine */
        disable_int(info, NDCR_INT_MASK);
 
@@ -1935,10 +1922,6 @@ static int pxa3xx_nand_resume(struct platform_device *pdev)
         * all status before resume
         */
        nand_writel(info, NDSR, NDSR_MASK);
-       for (cs = 0; cs < pdata->num_cs; cs++) {
-               mtd = info->host[cs]->mtd;
-               mtd_resume(mtd);
-       }
 
        return 0;
 }
--

2.6.0.rc2.230.g3dd15c0

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Signature · 2 lines
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
Tested-by Acked-by
#7 lore raw
Quoted 13 lines · Ezequiel Garcia

Robert:

Can you test this on a platform with PM support, and make sure the NAND works fine after a resume?

Quoted 7 lines · Brian Norris

mtd_{suspend,resume}() get called from mtdcore in a class suspend/resume callback. We don't need to call them again here. In practice, this would actually work OK, as nand_base actually handles nesting OK -- it just might print warnings.

Untested, but there are few (no?) users of PM for this driver AFAIK.

Brian,

We discussed this on IRC, but I completely forgot about it :/

Without this patch nand_{suspend, resume} is called twice (which seems harmless), tested on Armada XP-GP using nandtest.

Signature · 2 lines
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar
author Applied
#8 lore raw
Quoted 12 lines · Ezequiel Garcia
Quoted 11 lines · Ezequiel Garcia

Robert:

Can you test this on a platform with PM support, and make sure the NAND works fine after a resume?

Quoted 5 lines · Brian Norris

mtd_{suspend,resume}() get called from mtdcore in a class suspend/resume callback. We don't need to call them again here. In practice, this would actually work OK, as nand_base actually handles nesting OK -- it just might print warnings.

Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

Untested, but there are few (no?) users of PM for this driver AFAIK.

Brian,

We discussed this on IRC, but I completely forgot about it :/

No problem.

Quoted 2 lines

Without this patch nand_{suspend, resume} is called twice (which seems harmless), tested on Armada XP-GP using nandtest.

Right, I think it's incidentally harmless. But you do get this warning without the patch set, right?

nand_resume called for a chip which is not in suspended state

Quoted 2 lines

Pushed to l2-mtd.git

#9 lore raw
Quoted 22 lines · Brian Norris
Quoted 12 lines · Ezequiel Garcia
Quoted 11 lines · Ezequiel Garcia

Robert:

Can you test this on a platform with PM support, and make sure the NAND works fine after a resume?

Quoted 5 lines · Brian Norris

mtd_{suspend,resume}() get called from mtdcore in a class suspend/resume callback. We don't need to call them again here. In practice, this would actually work OK, as nand_base actually handles nesting OK -- it just might print warnings.

Quoted 6 lines
Quoted 2 lines
Quoted 2 lines

Untested, but there are few (no?) users of PM for this driver AFAIK.

Brian,

We discussed this on IRC, but I completely forgot about it :/

No problem.

Quoted 8 lines
Quoted 2 lines

Without this patch nand_{suspend, resume} is called twice (which seems harmless), tested on Armada XP-GP using nandtest.

Right, I think it's incidentally harmless. But you do get this warning without the patch set, right?

nand_resume called for a chip which is not in suspended state

Yup, I get that warning.

Quoted 4 lines
Quoted 2 lines

Pushed to l2-mtd.git

Signature · 2 lines
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
#10 lore raw
In reply to Ezequiel Garcia
Quoted 4 lines · Ezequiel Garcia

Robert:

Can you test this on a platform with PM support, and make sure the NAND works fine after a resume?

Euh actually I can't. I'm only sure Suspend to RAM is working for pxa27x, I have not yet this proof for pxa3xx, and my last try wasn't very successful 3 monthes ago ...

As this regression might be within my platform's configuration (IPL setup, ...), I don't yet know where the bug is.

Cheers.

--
Robert