From: Thierry Reding <thierry.reding@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
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 13:16:12 +0100 [thread overview]
Message-ID: <t3la3jte5tia7rh5ftuv5cchrwdxe4cxa2v3g6lxgoh5u6rmcy@hzw7lbke2vac> (raw)
In-Reply-To: <20251029174654.GA1571737@bhelgaas>
[-- Attachment #1: Type: text/plain, Size: 5045 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2025-10-30 12:16 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 [this message]
2025-10-30 15:31 ` Bjorn Helgaas
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=t3la3jte5tia7rh5ftuv5cchrwdxe4cxa2v3g6lxgoh5u6rmcy@hzw7lbke2vac \
--to=thierry.reding@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=helgaas@kernel.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=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).