| Message ID | 20190521130357.20803-3-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Prepare Armada 3700 PCIe suspend to RAM support | expand |
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. s/register/registers/ ? > Device link support in the clock framework enforce that the clock > driver's resume() callback will be called before the PCIe > driver's. But, any resume_noirq() callback will be called before all > the registered resume() callbacks. > > The solution to support PCIe resume operation is to change the > "priority" of this clock driver PM callbacks to "_noirq()". > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/clk/mvebu/armada-37xx-periph.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c > index 1e18c5a875bd..bee45e43a85f 100644 > --- a/drivers/clk/mvebu/armada-37xx-periph.c > +++ b/drivers/clk/mvebu/armada-37xx-periph.c > @@ -715,8 +715,8 @@ static int __maybe_unused armada_3700_periph_clock_resume(struct device *dev) > } > > static const struct dev_pm_ops armada_3700_periph_clock_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(armada_3700_periph_clock_suspend, > - armada_3700_periph_clock_resume) > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(armada_3700_periph_clock_suspend, > + armada_3700_periph_clock_resume) > }; > > static int armada_3700_periph_clock_probe(struct platform_device *pdev) > -- > 2.19.1 >
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. Thanks, Miquèl
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. Bjorn
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
On Mon, Jun 17, 2019 at 7:50 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > 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? You're only touching drivers/clk, so you don't need my ack, and I don't really feel qualified to add a reviewed-by. Power management and suspend/resume is still a mystery to me :) Bjorn
diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c index 1e18c5a875bd..bee45e43a85f 100644 --- a/drivers/clk/mvebu/armada-37xx-periph.c +++ b/drivers/clk/mvebu/armada-37xx-periph.c @@ -715,8 +715,8 @@ static int __maybe_unused armada_3700_periph_clock_resume(struct device *dev) } static const struct dev_pm_ops armada_3700_periph_clock_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(armada_3700_periph_clock_suspend, - armada_3700_periph_clock_resume) + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(armada_3700_periph_clock_suspend, + armada_3700_periph_clock_resume) }; static int armada_3700_periph_clock_probe(struct platform_device *pdev)
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(). Device link support in the clock framework enforce that the clock driver's resume() callback will be called before the PCIe driver's. But, any resume_noirq() callback will be called before all the registered resume() callbacks. The solution to support PCIe resume operation is to change the "priority" of this clock driver PM callbacks to "_noirq()". Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/clk/mvebu/armada-37xx-periph.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)