From: Bjorn Helgaas <helgaas@kernel.org>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
x86@kernel.org, linux-arm-kernel@lists.infradead.org,
linux-riscv@lists.infradead.org, linux-mips@vger.kernel.org,
loongarch@lists.linux.dev, linuxppc-dev@lists.ozlabs.org,
linux-sh@vger.kernel.org, linux-pci@vger.kernel.org,
linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/7] MIPS: PCI: Use contextual data instead of global variable
Date: Thu, 30 Oct 2025 10:31:21 -0500 [thread overview]
Message-ID: <20251030153121.GA1624982@bhelgaas> (raw)
In-Reply-To: <t3la3jte5tia7rh5ftuv5cchrwdxe4cxa2v3g6lxgoh5u6rmcy@hzw7lbke2vac>
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
next prev parent reply other threads:[~2025-10-30 15:31 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-29 16:33 [PATCH v3 0/7] syscore: Pass context data to callbacks Thierry Reding
2025-10-29 16:33 ` [PATCH v3 1/7] " Thierry Reding
2025-11-03 16:18 ` Rafael J. Wysocki
2025-11-05 16:52 ` Thierry Reding
2025-11-13 18:32 ` Thierry Reding
2025-11-13 19:18 ` Rafael J. Wysocki
2025-10-29 16:33 ` [PATCH v3 2/7] MIPS: PCI: Use contextual data instead of global variable Thierry Reding
2025-10-29 17:46 ` Bjorn Helgaas
2025-10-30 12:16 ` Thierry Reding
2025-10-30 15:31 ` Bjorn Helgaas [this message]
2025-10-30 20:10 ` Maciej W. Rozycki
2025-10-29 16:33 ` [PATCH v3 3/7] bus: mvebu-mbus: " Thierry Reding
2025-10-29 16:33 ` [PATCH v3 4/7] clk: ingenic: tcu: " Thierry Reding
2025-10-29 17:56 ` Bjorn Helgaas
2025-10-30 12:28 ` Thierry Reding
2025-10-29 16:33 ` [PATCH v3 5/7] clk: mvebu: " Thierry Reding
2025-10-29 16:33 ` [PATCH v3 6/7] irqchip/irq-imx-gpcv2: " Thierry Reding
2025-10-29 16:33 ` [PATCH v3 7/7] soc/tegra: pmc: " Thierry Reding
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251030153121.GA1624982@bhelgaas \
--to=helgaas@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-sh@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=loongarch@lists.linux.dev \
--cc=rafael@kernel.org \
--cc=thierry.reding@gmail.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).