From: Thierry Reding <treding@nvidia.com>
Pass the driver-specific data via the syscore struct and use it in the
syscore ops.
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- adjust for API changes and update commit message
Changes in v2:
- remove unused global variable
arch/mips/pci/pci-alchemy.c | 24 ++++++------------------
1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/arch/mips/pci/pci-alchemy.c b/arch/mips/pci/pci-alchemy.c
index 6bfee0f71803..f73bf60bd069 100644
--- a/arch/mips/pci/pci-alchemy.c
+++ b/arch/mips/pci/pci-alchemy.c
@@ -33,6 +33,7 @@
struct alchemy_pci_context {
struct pci_controller alchemy_pci_ctrl; /* leave as first member! */
+ struct syscore syscore;
void __iomem *regs; /* ctrl base */
/* tools for wired entry for config space access */
unsigned long last_elo0;
@@ -46,12 +47,6 @@ struct alchemy_pci_context {
int (*board_pci_idsel)(unsigned int devsel, int assert);
};
-/* for syscore_ops. There's only one PCI controller on Alchemy chips, so this
- * should suffice for now.
- */
-static struct alchemy_pci_context *__alchemy_pci_ctx;
-
-
/* IO/MEM resources for PCI. Keep the memres in sync with fixup_bigphys_addr
* in arch/mips/alchemy/common/setup.c
*/
@@ -306,9 +301,7 @@ static int alchemy_pci_def_idsel(unsigned int devsel, int assert)
/* save PCI controller register contents. */
static int alchemy_pci_suspend(void *data)
{
- struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
- if (!ctx)
- return 0;
+ struct alchemy_pci_context *ctx = data;
ctx->pm[0] = __raw_readl(ctx->regs + PCI_REG_CMEM);
ctx->pm[1] = __raw_readl(ctx->regs + PCI_REG_CONFIG) & 0x0009ffff;
@@ -328,9 +321,7 @@ static int alchemy_pci_suspend(void *data)
static void alchemy_pci_resume(void *data)
{
- struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
- if (!ctx)
- return;
+ struct alchemy_pci_context *ctx = data;
__raw_writel(ctx->pm[0], ctx->regs + PCI_REG_CMEM);
__raw_writel(ctx->pm[2], ctx->regs + PCI_REG_B2BMASK_CCH);
@@ -359,10 +350,6 @@ static const struct syscore_ops alchemy_pci_syscore_ops = {
.resume = alchemy_pci_resume,
};
-static struct syscore alchemy_pci_syscore = {
- .ops = &alchemy_pci_syscore_ops,
-};
-
static int alchemy_pci_probe(struct platform_device *pdev)
{
struct alchemy_pci_platdata *pd = pdev->dev.platform_data;
@@ -480,9 +467,10 @@ static int alchemy_pci_probe(struct platform_device *pdev)
__raw_writel(val, ctx->regs + PCI_REG_CONFIG);
wmb();
- __alchemy_pci_ctx = ctx;
platform_set_drvdata(pdev, ctx);
- register_syscore(&alchemy_pci_syscore);
+ ctx->syscore.ops = &alchemy_pci_syscore_ops;
+ ctx->syscore.data = ctx;
+ register_syscore(&ctx->syscore);
register_pci_controller(&ctx->alchemy_pci_ctrl);
dev_info(&pdev->dev, "PCI controller at %ld MHz\n",
--
2.51.0On Wed, Oct 29, 2025 at 05:33:31PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Pass the driver-specific data via the syscore struct and use it in the
> syscore ops.
Would be nice to include the "instead of global variable" part here so
the commit log includes the benefit and can stand alone even without
the subject.
Awesome to get rid of another global variable! More comments below.
> +++ b/arch/mips/pci/pci-alchemy.c
> @@ -33,6 +33,7 @@
>
> struct alchemy_pci_context {
> struct pci_controller alchemy_pci_ctrl; /* leave as first member! */
> + struct syscore syscore;
> void __iomem *regs; /* ctrl base */
> /* tools for wired entry for config space access */
> unsigned long last_elo0;
> @@ -46,12 +47,6 @@ struct alchemy_pci_context {
> int (*board_pci_idsel)(unsigned int devsel, int assert);
> };
>
> -/* for syscore_ops. There's only one PCI controller on Alchemy chips, so this
> - * should suffice for now.
> - */
> -static struct alchemy_pci_context *__alchemy_pci_ctx;
> -
> -
> /* IO/MEM resources for PCI. Keep the memres in sync with fixup_bigphys_addr
> * in arch/mips/alchemy/common/setup.c
> */
> @@ -306,9 +301,7 @@ static int alchemy_pci_def_idsel(unsigned int devsel, int assert)
> /* save PCI controller register contents. */
> static int alchemy_pci_suspend(void *data)
> {
> - struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
> - if (!ctx)
> - return 0;
> + struct alchemy_pci_context *ctx = data;
>
> ctx->pm[0] = __raw_readl(ctx->regs + PCI_REG_CMEM);
> ctx->pm[1] = __raw_readl(ctx->regs + PCI_REG_CONFIG) & 0x0009ffff;
> @@ -328,9 +321,7 @@ static int alchemy_pci_suspend(void *data)
>
> static void alchemy_pci_resume(void *data)
> {
> - struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
> - if (!ctx)
> - return;
> + struct alchemy_pci_context *ctx = data;
>
> __raw_writel(ctx->pm[0], ctx->regs + PCI_REG_CMEM);
> __raw_writel(ctx->pm[2], ctx->regs + PCI_REG_B2BMASK_CCH);
> @@ -359,10 +350,6 @@ static const struct syscore_ops alchemy_pci_syscore_ops = {
> .resume = alchemy_pci_resume,
> };
>
> -static struct syscore alchemy_pci_syscore = {
> - .ops = &alchemy_pci_syscore_ops,
> -};
> -
> static int alchemy_pci_probe(struct platform_device *pdev)
> {
> struct alchemy_pci_platdata *pd = pdev->dev.platform_data;
> @@ -480,9 +467,10 @@ static int alchemy_pci_probe(struct platform_device *pdev)
> __raw_writel(val, ctx->regs + PCI_REG_CONFIG);
> wmb();
>
> - __alchemy_pci_ctx = ctx;
> platform_set_drvdata(pdev, ctx);
> - register_syscore(&alchemy_pci_syscore);
> + ctx->syscore.ops = &alchemy_pci_syscore_ops;
> + ctx->syscore.data = ctx;
> + register_syscore(&ctx->syscore);
As far as I can tell, the only use of syscore in this driver is for
suspend/resume.
This is a regular platform_device driver, so instead of syscore, I
think it should use generic power management like other PCI host
controller drivers do, something like this:
static int alchemy_pci_suspend_noirq(struct device *dev)
...
static int alchemy_pci_resume_noirq(struct device *dev)
...
static DEFINE_NOIRQ_DEV_PM_OPS(alchemy_pci_pm_ops,
alchemy_pci_suspend_noirq,
alchemy_pci_resume_noirq);
static struct platform_driver alchemy_pcictl_driver = {
.probe = alchemy_pci_probe,
.driver = {
.name = "alchemy-pci",
.pm = pm_sleep_ptr(&alchemy_pci_pm_ops),
},
};
Here's a sample in another driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/cadence/pci-j721e.c?id=v6.17#n663
> register_pci_controller(&ctx->alchemy_pci_ctrl);
>
> dev_info(&pdev->dev, "PCI controller at %ld MHz\n",
> --
> 2.51.0
>
On Wed, Oct 29, 2025 at 12:46:54PM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 29, 2025 at 05:33:31PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Pass the driver-specific data via the syscore struct and use it in the
> > syscore ops.
>
> Would be nice to include the "instead of global variable" part here so
> the commit log includes the benefit and can stand alone even without
> the subject.
Good point.
> Awesome to get rid of another global variable! More comments below.
\o/
> > +++ b/arch/mips/pci/pci-alchemy.c
> > @@ -33,6 +33,7 @@
> >
> > struct alchemy_pci_context {
> > struct pci_controller alchemy_pci_ctrl; /* leave as first member! */
> > + struct syscore syscore;
> > void __iomem *regs; /* ctrl base */
> > /* tools for wired entry for config space access */
> > unsigned long last_elo0;
> > @@ -46,12 +47,6 @@ struct alchemy_pci_context {
> > int (*board_pci_idsel)(unsigned int devsel, int assert);
> > };
> >
> > -/* for syscore_ops. There's only one PCI controller on Alchemy chips, so this
> > - * should suffice for now.
> > - */
> > -static struct alchemy_pci_context *__alchemy_pci_ctx;
> > -
> > -
> > /* IO/MEM resources for PCI. Keep the memres in sync with fixup_bigphys_addr
> > * in arch/mips/alchemy/common/setup.c
> > */
> > @@ -306,9 +301,7 @@ static int alchemy_pci_def_idsel(unsigned int devsel, int assert)
> > /* save PCI controller register contents. */
> > static int alchemy_pci_suspend(void *data)
> > {
> > - struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
> > - if (!ctx)
> > - return 0;
> > + struct alchemy_pci_context *ctx = data;
> >
> > ctx->pm[0] = __raw_readl(ctx->regs + PCI_REG_CMEM);
> > ctx->pm[1] = __raw_readl(ctx->regs + PCI_REG_CONFIG) & 0x0009ffff;
> > @@ -328,9 +321,7 @@ static int alchemy_pci_suspend(void *data)
> >
> > static void alchemy_pci_resume(void *data)
> > {
> > - struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
> > - if (!ctx)
> > - return;
> > + struct alchemy_pci_context *ctx = data;
> >
> > __raw_writel(ctx->pm[0], ctx->regs + PCI_REG_CMEM);
> > __raw_writel(ctx->pm[2], ctx->regs + PCI_REG_B2BMASK_CCH);
> > @@ -359,10 +350,6 @@ static const struct syscore_ops alchemy_pci_syscore_ops = {
> > .resume = alchemy_pci_resume,
> > };
> >
> > -static struct syscore alchemy_pci_syscore = {
> > - .ops = &alchemy_pci_syscore_ops,
> > -};
> > -
> > static int alchemy_pci_probe(struct platform_device *pdev)
> > {
> > struct alchemy_pci_platdata *pd = pdev->dev.platform_data;
> > @@ -480,9 +467,10 @@ static int alchemy_pci_probe(struct platform_device *pdev)
> > __raw_writel(val, ctx->regs + PCI_REG_CONFIG);
> > wmb();
> >
> > - __alchemy_pci_ctx = ctx;
> > platform_set_drvdata(pdev, ctx);
> > - register_syscore(&alchemy_pci_syscore);
> > + ctx->syscore.ops = &alchemy_pci_syscore_ops;
> > + ctx->syscore.data = ctx;
> > + register_syscore(&ctx->syscore);
>
> As far as I can tell, the only use of syscore in this driver is for
> suspend/resume.
>
> This is a regular platform_device driver, so instead of syscore, I
> think it should use generic power management like other PCI host
> controller drivers do, something like this:
>
> static int alchemy_pci_suspend_noirq(struct device *dev)
> ...
>
> static int alchemy_pci_resume_noirq(struct device *dev)
> ...
>
> static DEFINE_NOIRQ_DEV_PM_OPS(alchemy_pci_pm_ops,
> alchemy_pci_suspend_noirq,
> alchemy_pci_resume_noirq);
>
> static struct platform_driver alchemy_pcictl_driver = {
> .probe = alchemy_pci_probe,
> .driver = {
> .name = "alchemy-pci",
> .pm = pm_sleep_ptr(&alchemy_pci_pm_ops),
> },
> };
>
> Here's a sample in another driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/cadence/pci-j721e.c?id=v6.17#n663
I thought so too, but then I looked at the history and saw that it was
initially regular PM ops and then fixed by using syscore in this commit:
commit 864c6c22e9a5742b0f43c983b6c405d52817bacd
Author: Manuel Lauss <manuel.lauss@googlemail.com>
Date: Wed Nov 16 15:42:28 2011 +0100
MIPS: Alchemy: Fix PCI PM
Move PCI Controller PM to syscore_ops since the platform_driver PM methods
are called way too late on resume and far too early on suspend (after and
before PCI device resume/suspend).
This also allows to simplify wired entry management a bit.
Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
Cc: linux-mips@linux-mips.org
Patchwork: https://patchwork.linux-mips.org/patch/3007/
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
So unfortunately I don't think it'll work for this driver.
Thierry
On Thu, Oct 30, 2025 at 01:16:12PM +0100, Thierry Reding wrote:
> On Wed, Oct 29, 2025 at 12:46:54PM -0500, Bjorn Helgaas wrote:
> > On Wed, Oct 29, 2025 at 05:33:31PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > Pass the driver-specific data via the syscore struct and use it in the
> > > syscore ops.
> > > +++ b/arch/mips/pci/pci-alchemy.c
> > > @@ -33,6 +33,7 @@
> > >
> > > struct alchemy_pci_context {
> > > struct pci_controller alchemy_pci_ctrl; /* leave as first member! */
> > > + struct syscore syscore;
> > > void __iomem *regs; /* ctrl base */
> > > /* tools for wired entry for config space access */
> > > unsigned long last_elo0;
> > > @@ -46,12 +47,6 @@ struct alchemy_pci_context {
> > > int (*board_pci_idsel)(unsigned int devsel, int assert);
> > > };
> > >
> > > -/* for syscore_ops. There's only one PCI controller on Alchemy chips, so this
> > > - * should suffice for now.
> > > - */
> > > -static struct alchemy_pci_context *__alchemy_pci_ctx;
> > > -
> > > -
> > > /* IO/MEM resources for PCI. Keep the memres in sync with fixup_bigphys_addr
> > > * in arch/mips/alchemy/common/setup.c
> > > */
> > > @@ -306,9 +301,7 @@ static int alchemy_pci_def_idsel(unsigned int devsel, int assert)
> > > /* save PCI controller register contents. */
> > > static int alchemy_pci_suspend(void *data)
> > > {
> > > - struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
> > > - if (!ctx)
> > > - return 0;
> > > + struct alchemy_pci_context *ctx = data;
> > >
> > > ctx->pm[0] = __raw_readl(ctx->regs + PCI_REG_CMEM);
> > > ctx->pm[1] = __raw_readl(ctx->regs + PCI_REG_CONFIG) & 0x0009ffff;
> > > @@ -328,9 +321,7 @@ static int alchemy_pci_suspend(void *data)
> > >
> > > static void alchemy_pci_resume(void *data)
> > > {
> > > - struct alchemy_pci_context *ctx = __alchemy_pci_ctx;
> > > - if (!ctx)
> > > - return;
> > > + struct alchemy_pci_context *ctx = data;
> > >
> > > __raw_writel(ctx->pm[0], ctx->regs + PCI_REG_CMEM);
> > > __raw_writel(ctx->pm[2], ctx->regs + PCI_REG_B2BMASK_CCH);
> > > @@ -359,10 +350,6 @@ static const struct syscore_ops alchemy_pci_syscore_ops = {
> > > .resume = alchemy_pci_resume,
> > > };
> > >
> > > -static struct syscore alchemy_pci_syscore = {
> > > - .ops = &alchemy_pci_syscore_ops,
> > > -};
> > > -
> > > static int alchemy_pci_probe(struct platform_device *pdev)
> > > {
> > > struct alchemy_pci_platdata *pd = pdev->dev.platform_data;
> > > @@ -480,9 +467,10 @@ static int alchemy_pci_probe(struct platform_device *pdev)
> > > __raw_writel(val, ctx->regs + PCI_REG_CONFIG);
> > > wmb();
> > >
> > > - __alchemy_pci_ctx = ctx;
> > > platform_set_drvdata(pdev, ctx);
> > > - register_syscore(&alchemy_pci_syscore);
> > > + ctx->syscore.ops = &alchemy_pci_syscore_ops;
> > > + ctx->syscore.data = ctx;
> > > + register_syscore(&ctx->syscore);
> >
> > As far as I can tell, the only use of syscore in this driver is for
> > suspend/resume.
> >
> > This is a regular platform_device driver, so instead of syscore, I
> > think it should use generic power management like other PCI host
> > controller drivers do, something like this:
> >
> > static int alchemy_pci_suspend_noirq(struct device *dev)
> > ...
> >
> > static int alchemy_pci_resume_noirq(struct device *dev)
> > ...
> >
> > static DEFINE_NOIRQ_DEV_PM_OPS(alchemy_pci_pm_ops,
> > alchemy_pci_suspend_noirq,
> > alchemy_pci_resume_noirq);
> >
> > static struct platform_driver alchemy_pcictl_driver = {
> > .probe = alchemy_pci_probe,
> > .driver = {
> > .name = "alchemy-pci",
> > .pm = pm_sleep_ptr(&alchemy_pci_pm_ops),
> > },
> > };
> >
> > Here's a sample in another driver:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/cadence/pci-j721e.c?id=v6.17#n663
>
> I thought so too, but then I looked at the history and saw that it was
> initially regular PM ops and then fixed by using syscore in this commit:
>
> commit 864c6c22e9a5742b0f43c983b6c405d52817bacd
> Author: Manuel Lauss <manuel.lauss@googlemail.com>
> Date: Wed Nov 16 15:42:28 2011 +0100
>
> MIPS: Alchemy: Fix PCI PM
>
> Move PCI Controller PM to syscore_ops since the platform_driver PM methods
> are called way too late on resume and far too early on suspend (after and
> before PCI device resume/suspend).
> This also allows to simplify wired entry management a bit.
>
> Signed-off-by: Manuel Lauss <manuel.lauss@googlemail.com>
> Cc: linux-mips@linux-mips.org
> Patchwork: https://patchwork.linux-mips.org/patch/3007/
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
The alchemy PCI controller is a platform_device, and it must be
initialized before enumerating the PCI devices below it. The same
order should apply for suspend/resume (suspend PCI devices, then PCI
controller; resume PCI controller, then PCI devices).
So if this didn't work before, I think it means something is messed up
with the device hierarchy.
But I understand the difficulty of testing changes here, so syscore is
simplest from that point of view.
It does complicate maintenance though. I think all of mips ultimately
uses register_pci_controller() and pcibios_scanbus(). Neither really
contains anything mips-specific, so they duplicate a lot of the code
in pci_host_probe(). Oh well, I guess that's part of the burden of
supporting old platforms forever.
Bjorn
On Thu, 30 Oct 2025, Bjorn Helgaas wrote: > It does complicate maintenance though. I think all of mips ultimately > uses register_pci_controller() and pcibios_scanbus(). Neither really > contains anything mips-specific, so they duplicate a lot of the code > in pci_host_probe(). Oh well, I guess that's part of the burden of > supporting old platforms forever. FWIW new MIPS hardware continues being manufactured and if there is anything needed to clean up in generic MIPS/PCI platform code, then that can certainly be scheduled, subject to developers' resource availability. Individual MIPS platforms may vary of course, and with the solely legacy ones it will depend on the availability of hardware and engineers willing to maintain it. Maciej
© 2016 - 2025 Red Hat, Inc.