| Message ID | 20190521130357.20803-1-miquel.raynal@bootlin.com |
|---|---|
| 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
Hello, As part of an effort to bring suspend to RAM support to the Armada 3700 SoC (main target: ESPRESSObin board), there are small things to do in the Armada 3700 peripherals clock driver: * On this SoC, the PCIe controller gets fed by a gated clock in the south bridge. This clock is missing in the current driver, patch 1 adds it. * Because of a constraint in the PCI core, the resume function of a PCIe controller driver must be run at an early stage (->suspend/resume_noirq()), before the core tries to ->read/write() in the PCIe registers to do more configuration. Hence, the PCIe clock must be resumed before. This is enforced thanks to two changes: 1/ Add device links to the clock framework. This enforce order in the PM core: the clocks are resumed before the consumers. Series has been posted, see [1]. 2/ Even with the above feature, the clock's resume() callback is called after the PCI controller's resume_noirq() callback. The only way to fix this is to change the "priority" of the clock suspend/resume callbacks. This is done in patch 2. * The bindings are updated with the PCI clock in patch 4 while patch 3 is just a typo correction in the same file. If there is anything unclear please feel free to ask. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-November/614527.html Thanks, Miquèl Changes in v2: ============== * Rebased on top of v5.2-rc1. * Added Rob's R-by tags. * No change on the "change suspend/resume time" patch as, despite my pings, I got no answer and IMHO the proposed approach is entirely valid. Miquel Raynal (4): clk: mvebu: armada-37xx-periph: add PCIe gated clock clk: mvebu: armada-37xx-periph: change suspend/resume time dt-bindings: clk: armada3700: fix typo in SoC name dt-bindings: clk: armada3700: document the PCIe clock .../devicetree/bindings/clock/armada3700-periph-clock.txt | 5 +++-- drivers/clk/mvebu/armada-37xx-periph.c | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-)