Re: [PATCH v2 2/4] clk: mvebu: armada-37xx-periph: change suspend/resume time

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Bjorn,

Bjorn Helgaas <bhelgaas@google.com> wrote on Tue, 4 Jun 2019 15:52:31
-0500:

> On Mon, May 27, 2019 at 8:46 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Bjorn,
> >
> > Thanks for the feedback.
> >
> > Bjorn Helgaas <bhelgaas@google.com> wrote on Tue, 21 May 2019 17:43:05
> > -0500:
> >  
> > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Date: Tue, May 21, 2019 at 8:04 AM
> > > To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
> > > Cc: <linux-clk@vger.kernel.org>, <devicetree@vger.kernel.org>, Thomas
> > > Petazzoni, Antoine Tenart, Gregory Clement, Maxime Chevallier, Nadav
> > > Haklai, Bjorn Helgaas, Rafael J . Wysocki, <linux-pm@vger.kernel.org>,
> > > Miquel Raynal
> > >  
> > > > Armada 3700 PCIe IP relies on the PCIe clock managed by this
> > > > driver. For reasons related to the PCI core's organization when
> > > > suspending/resuming, PCI host controller drivers must reconfigure
> > > > their register at suspend_noirq()/resume_noirq() which happens after
> > > > suspend()/suspend_late() and before resume_early()/resume().  
> > >
> > > "For reasons related to the PCI core's organization" manages to
> > > suggest that this change wouldn't be needed if only the PCI core did
> > > something differently, without actually being specific about what it
> > > would need to do differently.
> > >
> > > Is there something the PCI core could do better to make this easier?
> > > Or is it just something like "the PCI core needs to access registers
> > > after suspend_late()"?  You mention the host controller, but of course
> > > that's not itself a PCI device, so the PCI core doesn't have much to
> > > do with it directly.  
> >
> > Actually, if I understand correctly the below commit [1] and the core
> > [2] & [3], PCI device fixups can happen at any time, including at the
> > _noirq phase where, obviously, the PCI controller must be already
> > setup.
> >
> > I don't think changing this behavior is a viable solution and I would
> > not see it as a "PCI core could do better" alternative.
> >
> > ---8<---
> >
> > [1]
> > commit ab14d45ea58eae67c739e4ba01871cae7b6c4586
> > Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Date:   Tue Mar 17 15:55:45 2015 +0100
> >
> >     PCI: mvebu: Add suspend/resume support
> >
> >     Add suspend/resume support for the mvebu PCIe host driver.  Without
> >     this commit, the system will panic at resume time when PCIe devices
> >     are connected.
> >
> >     Note that we have to use the ->suspend_noirq() and ->resume_noirq()
> >     hooks, because at resume time, the PCI fixups are done at  
> >     ->resume_noirq() time, so the PCIe controller has to be ready at  
> >     this point.
> >
> >     Signed-off-by: Thomas Petazzoni
> >     <thomas.petazzoni@free-electrons.com> Signed-off-by: Bjorn Helgaas
> >     <bhelgaas@google.com> Acked-by: Jason Cooper <jason@lakedaemon.net>
> >
> > [2] https://elixir.bootlin.com/linux/v5.2-rc1/source/drivers/pci/pci-driver.c#L1181
> > [3] https://elixir.bootlin.com/linux/v5.2-rc1/source/drivers/pci/pci-driver.c#L522
> >  
> > --->8---  
> >  
> > >
> > > s/register/registers/ ?  
> >
> > Indeed. I would like to sort out the above technical point before
> > sending a v3 with this typo corrected.  
> 
> I don't have anything more to contribute here; just wanted to make
> sure this wasn't working around a fixable problem in PCI.

Great! Would you mind adding a A-b/R-b tag then?


Thanks,
Miquèl