Re: [PATCH v3 2/7] MIPS: PCI: Use contextual data instead of global variable

Thierry Reding <[email protected]> Thu, 30 Oct 2025 13:16:12 +0100
Newsgroups	gmane.linux.ports.sh.devel,gmane.linux.ports.arm.kernel,gmane.linux.ports.riscv,gmane.linux.ports.mips,gmane.linux.ports.ppc64.devel,gmane.linux.kernel.pci,gmane.linux.acpi.devel,gmane.linux.kernel
Message-ID	<t3la3jte5tia7rh5ftuv5cchrwdxe4cxa2v3g6lxgoh5u6rmcy@hzw7lbke2vac>
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 <[email protected]>
> > 
> > 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 <[email protected]>
    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 <[email protected]>
        Cc: [email protected]
        Patchwork: https://patchwork.linux-mips.org/patch/3007/
        Signed-off-by: Ralf Baechle <[email protected]>

So unfortunately I don't think it'll work for this driver.

Thierry
signature.asc (application/pgp-signature, 833 B)