Re: [PATCH 06/11] PCI: rcar-gen4: Add Root Port reset support

[Koichiro Den]

On Tue, Sep 22, 2026 at 11:22:01PM +0200, Marek Vasut wrote:
> On 9/18/26 5:20 AM, Koichiro Den wrote:
> 
> [...]
> 
> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> 
> [...]
> 
> > @@ -90,12 +91,22 @@ struct rcar_gen4_pcie_drvdata {
> >   	enum dw_pcie_device_mode mode;
> >   };
> > +enum rcar_gen4_pcie_state {
> > +	/* The controller is being reset and reinitialized */
> > +	RCAR_PCIE_RESETTING,
> > +};
> > +
> >   struct rcar_gen4_pcie {
> >   	struct dw_pcie dw;
> >   	void __iomem *base;
> >   	void __iomem *phy_base;
> >   	struct platform_device *pdev;
> >   	const struct rcar_gen4_pcie_drvdata *drvdata;
> > +	unsigned long state;
> 
> Can we simply use boolean flags here in struct rcar_gen4_pcie, instead of
> the enum rcar_gen4_pcie_state ?

Sure! let me rework this for v2.

> 
> > +	/* Protects APP interrupt enable registers and their software state. */
> > +	raw_spinlock_t app_lock;
> > +	/* Serializes Root Port hardware reinitialization. */
> > +	struct mutex reset_lock;
> >   };
> >   #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> > @@ -344,6 +355,33 @@ static int rcar_gen4_pcie_host_msi_addr(struct dw_pcie_rp *pp, u32 *msi_addr)
> >   	return 0;
> >   }
> > +/* Whether the APP interrupt sources may currently be enabled. */
> > +static bool rcar_gen4_pcie_irqs_blocked(struct rcar_gen4_pcie *rcar)
> > +{
> > +	return !!rcar->state;
> > +}
> > +
> > +static void rcar_gen4_pcie_app_irq_sync_locked(struct rcar_gen4_pcie *rcar)
> > +{
> > +	bool armed = !rcar_gen4_pcie_irqs_blocked(rcar);
> > +	u32 val;
> > +
> > +	lockdep_assert_held(&rcar->app_lock);
> > +
> > +	val = readl(rcar->base + PCIEINTSTS0EN);
> > +	val &= ~MSI_CTRL_INT;
> > +	if (armed && pci_msi_enabled())
> > +		val |= MSI_CTRL_INT;
> 
> Should this function cache the state of pci_msi_enabled() in struct
> rcar_gen4_pcie , so that in case rcar_gen4_pcie_irqs_blocked() reports MSIs
> as blocked at this point ...
> 
> > +	writel(val, rcar->base + PCIEINTSTS0EN);
> > +}
> > +
> > +static void rcar_gen4_pcie_app_irq_sync(struct rcar_gen4_pcie *rcar)
> > +{
> > +	guard(raw_spinlock_irqsave)(&rcar->app_lock);
> > +
> > +	rcar_gen4_pcie_app_irq_sync_locked(rcar);
> 
> ... this function can enable MSIs once it is called and MSIs are no longer
> blocked ?

Even leaving aside rcar_gen4_pcie_resume_irqs(), which this patch introduces,
rcar_gen4_pcie_app_irq_sync() re-evaluates the blocked state and
pci_msi_enabled() on each call, so I would expect it to enable MSIs once
unblocked, provided pci_msi_enabled() is true.

Am I missing a case where a cached value would be needed?

For example, even if there were a race with quirk_disable_all_msi(), I'm not
sure caching the value would help.

Best regards,
Koichiro

> 
> > +}
> > +
> >   static int rcar_gen4_pcie_host_msi_init(struct dw_pcie_rp *pp)
> >   {
> >   	struct dw_pcie *dw = to_dw_pcie_from_pp(pp);
> 
> [...]
> 
> -- 
> Best regards,
> Marek Vasut