| Message ID | 20190108162441.5278-1-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Bring suspend to RAM support to PCIe Aardvark driver | expand |
Hi Miquel, On mar., janv. 08 2019, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hello, > > As part of an effort to bring suspend to RAM support to Armada 3700 > SoCs (main target: ESPRESSObin), this series handles the work around > the PCIe IP. > > First, more configuration is done in the 'setup' helper as inspired > from the U-Boot driver. This is needed to entirely initialize the IP > during future resume operation (patch 1). > > Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As > current device trees do not provide the corresponding properties, not > finding one of these properties is not an error and just produces a > warning. However, if the property is present, an error during PHY > initialization will fail the probe of the driver. > > Note: To be sure the clock will be resumed before this driver, a first > series adding links between clocks and consumers has been submitted, > see [1]. Anyway, having the clock series applied first is not needed. > > Patch 5 adds suspend/resume hooks, re-using all the above. > > Finally, bindings and device trees are updated to reflect the hardware > (patch 6-12). While the clock depends on the SoC, the reset GPIO and > the PHY depends on the board so the clock is added in the > armada-37xx.dtsi file while the two other properties are added in > armada-3720-espressobin.dts. > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/623885.html > > Thanks, > Miquèl > > > Changes since v2: > ================= > * Minor patches reordering. > * Added pinctrl patches from Gregory Clement fixing the PCIe pins. His > changes implied modifications in the DT/bindings patches adding PCIe > reset pin support. Actually these patches are not in this series. You propably meant that this series is depend on these patches. If needed, for peoaple who wanted to test this series, the pinctrl changes are now in linux-next and also in pinctrl/for-next. Gregpry > * Added a new patch that enlarges the PIO timeout of the driver > (explanations in the commit log). > * With the timeout changed, removed the "experimental delay" that was > needed at resume time before accessing any register. > > Changes since v1: > ================= > * Change the capitalization in commit titles to follow the PCI > subsystem rules. > * Added Suggested-by tag to the patch adding PHY support and to the > patch adding the PHY property in the DT. > * Added Rob's Reviewed-by tags on bindings. > * I am following the discussion about calling functions that might > sleep in a NOIRQ context. As there is no real problem yet (as per my > understanding), I did not change anything on this regard. > > > Miquel Raynal (15): > PCI: aardvark: Enlarge PIO timeout > PCI: aardvark: Configure more registers in the configuration helper > PCI: aardvark: Add clock support > PCI: aardvark: Add PHY support > PCI: aardvark: Add PCIe warm reset support > PCI: aardvark: Add external reset GPIO support > PCI: aardvark: Add suspend to RAM support > dt-bindings: PCI: aardvark: Describe the clocks property > dt-bindings: PCI: aardvark: Describe the PHY property > dt-bindings: PCI: aardvark: Describe the PCIe endpoint card reset pins > dt-bindings: PCI: aardvark: Describe the reset-gpios property > ARM64: dts: marvell: armada-37xx: declare PCIe clock > ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY > ARM64: dts: marvell: armada-37xx: declare PCIe reset pin > ARM64: dts: marvell: armada-3720-espressobin: declare PCIe warm reset > pin > > .../devicetree/bindings/pci/aardvark-pci.txt | 14 ++ > .../dts/marvell/armada-3720-espressobin.dts | 3 + > arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 10 + > drivers/pci/controller/pci-aardvark.c | 217 +++++++++++++++++- > 4 files changed, 243 insertions(+), 1 deletion(-) > > -- > 2.19.1 >
Hi Gregory, Gregory CLEMENT <gregory.clement@bootlin.com> wrote on Fri, 18 Jan 2019 17:51:52 +0100: > Hi Miquel, > > On mar., janv. 08 2019, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Hello, > > > > As part of an effort to bring suspend to RAM support to Armada 3700 > > SoCs (main target: ESPRESSObin), this series handles the work around > > the PCIe IP. > > > > First, more configuration is done in the 'setup' helper as inspired > > from the U-Boot driver. This is needed to entirely initialize the IP > > during future resume operation (patch 1). > > > > Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As > > current device trees do not provide the corresponding properties, not > > finding one of these properties is not an error and just produces a > > warning. However, if the property is present, an error during PHY > > initialization will fail the probe of the driver. > > > > Note: To be sure the clock will be resumed before this driver, a first > > series adding links between clocks and consumers has been submitted, > > see [1]. Anyway, having the clock series applied first is not needed. > > > > Patch 5 adds suspend/resume hooks, re-using all the above. > > > > Finally, bindings and device trees are updated to reflect the hardware > > (patch 6-12). While the clock depends on the SoC, the reset GPIO and > > the PHY depends on the board so the clock is added in the > > armada-37xx.dtsi file while the two other properties are added in > > armada-3720-espressobin.dts. > > > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/623885.html > > > > Thanks, > > Miquèl > > > > > > Changes since v2: > > ================= > > * Minor patches reordering. > > * Added pinctrl patches from Gregory Clement fixing the PCIe pins. His > > changes implied modifications in the DT/bindings patches adding PCIe > > reset pin support. > > Actually these patches are not in this series. You propably meant that > this series is depend on these patches. > > If needed, for peoaple who wanted to test this series, the pinctrl > changes are now in linux-next and also in pinctrl/for-next. Absolutely, this is what I meant, thanks for clarifying. Thanks, Miquèl
On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal wrote: > Hello, > > As part of an effort to bring suspend to RAM support to Armada 3700 > SoCs (main target: ESPRESSObin), this series handles the work around > the PCIe IP. > > First, more configuration is done in the 'setup' helper as inspired > from the U-Boot driver. This is needed to entirely initialize the IP > during future resume operation (patch 1). > > Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As > current device trees do not provide the corresponding properties, not > finding one of these properties is not an error and just produces a > warning. However, if the property is present, an error during PHY > initialization will fail the probe of the driver. > > Note: To be sure the clock will be resumed before this driver, a first > series adding links between clocks and consumers has been submitted, > see [1]. Anyway, having the clock series applied first is not needed. I do not understand what this means, in particular in relation to the blocking clock calls in the suspend/resume NOIRQ hooks. Thanks, Lorenzo > Patch 5 adds suspend/resume hooks, re-using all the above. > > Finally, bindings and device trees are updated to reflect the hardware > (patch 6-12). While the clock depends on the SoC, the reset GPIO and > the PHY depends on the board so the clock is added in the > armada-37xx.dtsi file while the two other properties are added in > armada-3720-espressobin.dts. > > [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/623885.html > > Thanks, > Miqu??l > > > Changes since v2: > ================= > * Minor patches reordering. > * Added pinctrl patches from Gregory Clement fixing the PCIe pins. His > changes implied modifications in the DT/bindings patches adding PCIe > reset pin support. > * Added a new patch that enlarges the PIO timeout of the driver > (explanations in the commit log). > * With the timeout changed, removed the "experimental delay" that was > needed at resume time before accessing any register. > > Changes since v1: > ================= > * Change the capitalization in commit titles to follow the PCI > subsystem rules. > * Added Suggested-by tag to the patch adding PHY support and to the > patch adding the PHY property in the DT. > * Added Rob's Reviewed-by tags on bindings. > * I am following the discussion about calling functions that might > sleep in a NOIRQ context. As there is no real problem yet (as per my > understanding), I did not change anything on this regard. > > > Miquel Raynal (15): > PCI: aardvark: Enlarge PIO timeout > PCI: aardvark: Configure more registers in the configuration helper > PCI: aardvark: Add clock support > PCI: aardvark: Add PHY support > PCI: aardvark: Add PCIe warm reset support > PCI: aardvark: Add external reset GPIO support > PCI: aardvark: Add suspend to RAM support > dt-bindings: PCI: aardvark: Describe the clocks property > dt-bindings: PCI: aardvark: Describe the PHY property > dt-bindings: PCI: aardvark: Describe the PCIe endpoint card reset pins > dt-bindings: PCI: aardvark: Describe the reset-gpios property > ARM64: dts: marvell: armada-37xx: declare PCIe clock > ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY > ARM64: dts: marvell: armada-37xx: declare PCIe reset pin > ARM64: dts: marvell: armada-3720-espressobin: declare PCIe warm reset > pin > > .../devicetree/bindings/pci/aardvark-pci.txt | 14 ++ > .../dts/marvell/armada-3720-espressobin.dts | 3 + > arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 10 + > drivers/pci/controller/pci-aardvark.c | 217 +++++++++++++++++- > 4 files changed, 243 insertions(+), 1 deletion(-) > > -- > 2.19.1 >
Hi Lorenzo, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Wed, 23 Jan 2019 17:05:09 +0000: > On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal wrote: > > Hello, > > > > As part of an effort to bring suspend to RAM support to Armada 3700 > > SoCs (main target: ESPRESSObin), this series handles the work around > > the PCIe IP. > > > > First, more configuration is done in the 'setup' helper as inspired > > from the U-Boot driver. This is needed to entirely initialize the IP > > during future resume operation (patch 1). > > > > Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As > > current device trees do not provide the corresponding properties, not > > finding one of these properties is not an error and just produces a > > warning. However, if the property is present, an error during PHY > > initialization will fail the probe of the driver. > > > > Note: To be sure the clock will be resumed before this driver, a first > > series adding links between clocks and consumers has been submitted, > > see [1]. Anyway, having the clock series applied first is not needed. > > I do not understand what this means, in particular in relation > to the blocking clock calls in the suspend/resume NOIRQ hooks. I am not sure to understand your question. As there are multiple points in this sentence I will detail each of them so please comment on the one which is bothering you: * I am working in parallel on a series adding device links to the clock framework. This way when a driver consumes a clock, the clock provider driver will be resumed first. * If the clock series I am talking about is applied after this one, there is no build issue. Of course suspending the platform may not work but this is a new feature so nothing will be broken. * Device links do not enforce any priority if the suspend/resume phase between two drivers is not the same. The PCIe driver suspends in the NOIRQ phase. If we want the clock driver to suspend *after* PCIe, its suspend/resume callbacks must be promoted to the NOIRQ phase as well (and this is part of another series). As of today there is no alternative. Thanks, Miquèl
On Fri, Jan 25, 2019 at 11:05:30AM +0100, Miquel Raynal wrote: > Hi Lorenzo, > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Wed, 23 Jan 2019 > 17:05:09 +0000: > > > On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal wrote: > > > Hello, > > > > > > As part of an effort to bring suspend to RAM support to Armada 3700 > > > SoCs (main target: ESPRESSObin), this series handles the work around > > > the PCIe IP. > > > > > > First, more configuration is done in the 'setup' helper as inspired > > > from the U-Boot driver. This is needed to entirely initialize the IP > > > during future resume operation (patch 1). > > > > > > Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As > > > current device trees do not provide the corresponding properties, not > > > finding one of these properties is not an error and just produces a > > > warning. However, if the property is present, an error during PHY > > > initialization will fail the probe of the driver. > > > > > > Note: To be sure the clock will be resumed before this driver, a first > > > series adding links between clocks and consumers has been submitted, > > > see [1]. Anyway, having the clock series applied first is not needed. > > > > I do not understand what this means, in particular in relation > > to the blocking clock calls in the suspend/resume NOIRQ hooks. > > I am not sure to understand your question. > > As there are multiple points in this sentence I will detail each of > them so please comment on the one which is bothering you: > * I am working in parallel on a series adding device links to the clock > framework. This way when a driver consumes a clock, the clock > provider driver will be resumed first. > * If the clock series I am talking about is applied after this one, > there is no build issue. Of course suspending the platform may > not work but this is a new feature so nothing will be broken. Suspend to RAM will be broken if the clock is suspended and no notification will happen in the NOIRQ phase, it is a new-broken-feature. > * Device links do not enforce any priority if the suspend/resume phase > between two drivers is not the same. The PCIe driver suspends in the > NOIRQ phase. If we want the clock driver to suspend *after* PCIe, its > suspend/resume callbacks must be promoted to the NOIRQ phase as well > (and this is part of another series). As of today there is > no alternative. I will merge this series when it works, I have no evidence that it does given what you are writing above, if the series you mention are *necessary* for suspend-to-RAM to work they ought to be merged first. Thanks, Lorenzo
Hi Lorenzo, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Fri, 25 Jan 2019 12:40:11 +0000: > On Fri, Jan 25, 2019 at 11:05:30AM +0100, Miquel Raynal wrote: > > Hi Lorenzo, > > > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Wed, 23 Jan 2019 > > 17:05:09 +0000: > > > > > On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal wrote: > > > > Hello, > > > > > > > > As part of an effort to bring suspend to RAM support to Armada 3700 > > > > SoCs (main target: ESPRESSObin), this series handles the work around > > > > the PCIe IP. > > > > > > > > First, more configuration is done in the 'setup' helper as inspired > > > > from the U-Boot driver. This is needed to entirely initialize the IP > > > > during future resume operation (patch 1). > > > > > > > > Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As > > > > current device trees do not provide the corresponding properties, not > > > > finding one of these properties is not an error and just produces a > > > > warning. However, if the property is present, an error during PHY > > > > initialization will fail the probe of the driver. > > > > > > > > Note: To be sure the clock will be resumed before this driver, a first > > > > series adding links between clocks and consumers has been submitted, > > > > see [1]. Anyway, having the clock series applied first is not needed. > > > > > > I do not understand what this means, in particular in relation > > > to the blocking clock calls in the suspend/resume NOIRQ hooks. > > > > I am not sure to understand your question. > > > > As there are multiple points in this sentence I will detail each of > > them so please comment on the one which is bothering you: > > * I am working in parallel on a series adding device links to the clock > > framework. This way when a driver consumes a clock, the clock > > provider driver will be resumed first. > > * If the clock series I am talking about is applied after this one, > > there is no build issue. Of course suspending the platform may > > not work but this is a new feature so nothing will be broken. > > Suspend to RAM will be broken if the clock is suspended and no > notification will happen in the NOIRQ phase, it is a new-broken-feature. > > > > * Device links do not enforce any priority if the suspend/resume phase > > between two drivers is not the same. The PCIe driver suspends in the > > NOIRQ phase. If we want the clock driver to suspend *after* PCIe, its > > suspend/resume callbacks must be promoted to the NOIRQ phase as well > > (and this is part of another series). As of today there is > > no alternative. > > I will merge this series when it works, I have no evidence that it does > given what you are writing above, if the series you mention are > *necessary* for suspend-to-RAM to work they ought to be merged first. I am working actively to bring A3700 SoC suspend to RAM support almost from scratch. As of today I have contributed 65 patches spread in 8 series for the PHY, clk, PCIe, SATA, USB, pinctrl and net subsystems. Some of them have been merged, but the vast majority has not, yet. I mentioned this run-time dependency because it exists for people who would like to test just the PCIe IP. But S2RAM on A3700 will be a new-broken-feature until all patches are merged. While there is still one missing, the feature is broken. If everybody waits for the other patches to be merged first, it is gonna be a long process :) However, if you want to wait for the clock core series to be applied first I respect this choice and I will update you when it will be the case. Thanks, Miquèl
On Fri, Jan 25, 2019 at 01:57:57PM +0100, Miquel Raynal wrote: > Hi Lorenzo, > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Fri, 25 Jan 2019 > 12:40:11 +0000: > > > On Fri, Jan 25, 2019 at 11:05:30AM +0100, Miquel Raynal wrote: > > > Hi Lorenzo, > > > > > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote on Wed, 23 Jan 2019 > > > 17:05:09 +0000: > > > > > > > On Tue, Jan 08, 2019 at 05:24:25PM +0100, Miquel Raynal wrote: > > > > > Hello, > > > > > > > > > > As part of an effort to bring suspend to RAM support to Armada 3700 > > > > > SoCs (main target: ESPRESSObin), this series handles the work around > > > > > the PCIe IP. > > > > > > > > > > First, more configuration is done in the 'setup' helper as inspired > > > > > from the U-Boot driver. This is needed to entirely initialize the IP > > > > > during future resume operation (patch 1). > > > > > > > > > > Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As > > > > > current device trees do not provide the corresponding properties, not > > > > > finding one of these properties is not an error and just produces a > > > > > warning. However, if the property is present, an error during PHY > > > > > initialization will fail the probe of the driver. > > > > > > > > > > Note: To be sure the clock will be resumed before this driver, a first > > > > > series adding links between clocks and consumers has been submitted, > > > > > see [1]. Anyway, having the clock series applied first is not needed. > > > > > > > > I do not understand what this means, in particular in relation > > > > to the blocking clock calls in the suspend/resume NOIRQ hooks. > > > > > > I am not sure to understand your question. > > > > > > As there are multiple points in this sentence I will detail each of > > > them so please comment on the one which is bothering you: > > > * I am working in parallel on a series adding device links to the clock > > > framework. This way when a driver consumes a clock, the clock > > > provider driver will be resumed first. > > > * If the clock series I am talking about is applied after this one, > > > there is no build issue. Of course suspending the platform may > > > not work but this is a new feature so nothing will be broken. > > > > Suspend to RAM will be broken if the clock is suspended and no > > notification will happen in the NOIRQ phase, it is a new-broken-feature. > > > > > > > * Device links do not enforce any priority if the suspend/resume phase > > > between two drivers is not the same. The PCIe driver suspends in the > > > NOIRQ phase. If we want the clock driver to suspend *after* PCIe, its > > > suspend/resume callbacks must be promoted to the NOIRQ phase as well > > > (and this is part of another series). As of today there is > > > no alternative. > > > > I will merge this series when it works, I have no evidence that it does > > given what you are writing above, if the series you mention are > > *necessary* for suspend-to-RAM to work they ought to be merged first. > > I am working actively to bring A3700 SoC suspend to RAM support almost > from scratch. > > As of today I have contributed 65 patches spread in 8 series for the > PHY, clk, PCIe, SATA, USB, pinctrl and net subsystems. Some of them > have been merged, but the vast majority has not, yet. > > I mentioned this run-time dependency because it exists for people who > would like to test just the PCIe IP. But S2RAM on A3700 will be a > new-broken-feature until all patches are merged. While there is > still one missing, the feature is broken. If everybody waits for > the other patches to be merged first, it is gonna be a long process :) > > However, if you want to wait for the clock core series to be applied > first I respect this choice and I will update you when it will be the > case. I do not want to merge new code with known issues, it makes no sense. There are other drivers with a similar problem in the mainline, we ought to fix those but certainly adding more won't help in that respect. There is nothing I am saying here that goes against your work, I just want to have working code in the mainlike kernel, we will merge this series when it is ready, thanks for bearing with me. Feel free to ping me/rebase/repost when dependencies are sorted. Thanks, Lorenzo
Hello, As part of an effort to bring suspend to RAM support to Armada 3700 SoCs (main target: ESPRESSObin), this series handles the work around the PCIe IP. First, more configuration is done in the 'setup' helper as inspired from the U-Boot driver. This is needed to entirely initialize the IP during future resume operation (patch 1). Then, reset GPIO, PHY and clock support are introduced (patch 2-4). As current device trees do not provide the corresponding properties, not finding one of these properties is not an error and just produces a warning. However, if the property is present, an error during PHY initialization will fail the probe of the driver. Note: To be sure the clock will be resumed before this driver, a first series adding links between clocks and consumers has been submitted, see [1]. Anyway, having the clock series applied first is not needed. Patch 5 adds suspend/resume hooks, re-using all the above. Finally, bindings and device trees are updated to reflect the hardware (patch 6-12). While the clock depends on the SoC, the reset GPIO and the PHY depends on the board so the clock is added in the armada-37xx.dtsi file while the two other properties are added in armada-3720-espressobin.dts. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-January/623885.html Thanks, Miquèl Changes since v2: ================= * Minor patches reordering. * Added pinctrl patches from Gregory Clement fixing the PCIe pins. His changes implied modifications in the DT/bindings patches adding PCIe reset pin support. * Added a new patch that enlarges the PIO timeout of the driver (explanations in the commit log). * With the timeout changed, removed the "experimental delay" that was needed at resume time before accessing any register. Changes since v1: ================= * Change the capitalization in commit titles to follow the PCI subsystem rules. * Added Suggested-by tag to the patch adding PHY support and to the patch adding the PHY property in the DT. * Added Rob's Reviewed-by tags on bindings. * I am following the discussion about calling functions that might sleep in a NOIRQ context. As there is no real problem yet (as per my understanding), I did not change anything on this regard. Miquel Raynal (15): PCI: aardvark: Enlarge PIO timeout PCI: aardvark: Configure more registers in the configuration helper PCI: aardvark: Add clock support PCI: aardvark: Add PHY support PCI: aardvark: Add PCIe warm reset support PCI: aardvark: Add external reset GPIO support PCI: aardvark: Add suspend to RAM support dt-bindings: PCI: aardvark: Describe the clocks property dt-bindings: PCI: aardvark: Describe the PHY property dt-bindings: PCI: aardvark: Describe the PCIe endpoint card reset pins dt-bindings: PCI: aardvark: Describe the reset-gpios property ARM64: dts: marvell: armada-37xx: declare PCIe clock ARM64: dts: marvell: armada-3720-espressobin: declare PCIe PHY ARM64: dts: marvell: armada-37xx: declare PCIe reset pin ARM64: dts: marvell: armada-3720-espressobin: declare PCIe warm reset pin .../devicetree/bindings/pci/aardvark-pci.txt | 14 ++ .../dts/marvell/armada-3720-espressobin.dts | 3 + arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 10 + drivers/pci/controller/pci-aardvark.c | 217 +++++++++++++++++- 4 files changed, 243 insertions(+), 1 deletion(-)