PCI: apple: Assorted #PERST fixes

While the Apple PCIe driver works correctly when directly booted from the firmware, it fails to initialise when the kernel is booted from a bootloader using PCIe such as u-boot.

That's beacuse we're missing a proper reset of the port (we only clear the reset, but never assert it).

The PCIe spec requirements are two-fold:

- #PERST must be asserted before setting up the clocks, and stay asserted for at least 100us (Tperst-clk).

- Once #PERST is deasserted, the OS must wait for at least 100ms "from the end of a Conventional Reset" before we can start talking to the devices

Implementing this results in a booting system.

Fixes 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")

Acked-by Pali Rohár <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc Alyssa Rosenzweig <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Bjorn Helgaas <[email protected]>>

---
 drivers/pci/controller/pcie-apple.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/pci/controller/pcie-apple.c +0 -0

index 1bf4d75b61be..957960a733c4 100644

While the Apple PCIe driver works correctly when directly booted from the firmware, it fails to initialise when the kernel is booted from a bootloader using PCIe such as u-boot.

That's beacuse we're missing a proper reset of the port (we only clear the reset, but never assert it).

The PCIe spec requirements are two-fold:

- #PERST must be asserted before setting up the clocks, and stay asserted for at least 100us (Tperst-clk).

- Once #PERST is deasserted, the OS must wait for at least 100ms "from the end of a Conventional Reset" before we can start talking to the devices

Implementing this results in a booting system.

Fixes 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")

Acked-by Pali Rohár <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc Alyssa Rosenzweig <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Bjorn Helgaas <[email protected]>

---
  drivers/pci/controller/pcie-apple.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

drivers/pci/controller/pcie-apple.c +0 -0

index 1bf4d75b61be..957960a733c4 100644

drivers/pci/controller/pcie-apple.c +4 -0

+
  	ret = apple_pcie_setup_refclk(pcie, port);
  	if (ret < 0)
  		return ret;
 
+	/* The minimal Tperst-clk value is 100us (PCIe CMS r2.0, 2.6.2) */
+	usleep_range(100, 200);
+

While the Apple PCIe driver works correctly when directly booted from the firmware, it fails to initialise when the kernel is booted from a bootloader using PCIe such as u-boot.

That's beacuse we're missing a proper reset of the port (we only clear the reset, but never assert it).

The PCIe spec requirements are two-fold:

- #PERST must be asserted before setting up the clocks, and stay asserted for at least 100us (Tperst-clk).

- Once #PERST is deasserted, the OS must wait for at least 100ms "from the end of a Conventional Reset" before we can start talking to the devices

Implementing this results in a booting system.

Fixes 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")

Acked-by Pali Rohár <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Cc Alyssa Rosenzweig <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Bjorn Helgaas <[email protected]>

---
 drivers/pci/controller/pcie-apple.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/pci/controller/pcie-apple.c +0 -0

index 1bf4d75b61be..957960a733c4 100644
@@ -539,13 +539,23 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 
 	rmw_set(PORT_APPCLK_EN, port->base + PORT_APPCLK);

drivers/pci/controller/pcie-apple.c +0 -0

 	ret = readl_relaxed_poll_timeout(port->base + PORT_STATUS, stat,
 					 stat & PORT_STATUS_READY, 100, 250000);
 	if (ret < 0) {

Signature · 1 line

2.30.2

From: Marc Zyngier <[email protected]>
Date: Tue, 23 Nov 2021 18:06:35 +0000

As the name indicates, #PERST is active low. So fix the DT description to match the HW behaviour.

Fixes ff2a8d91d80c ("arm64: apple: Add PCIe node")

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/boot/dts/apple/t8103.dtsi | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

arch/arm64/boot/dts/apple/t8103.dtsi +4 -3

index fc8b2bb06ffe..e22c9433d5e0 100644
@@ -7,6 +7,7 @@
  * Copyright The Asahi Linux Contributors
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/apple-aic.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/pinctrl/apple.h>

@@ -281,7 +282,7 @@ pcie0: pcie@690000000 {
 			port00: pci@0,0 {
 				device_type = "pci";
 				reg = <0x0 0x0 0x0 0x0 0x0>;
-				reset-gpios = <&pinctrl_ap 152 0>;
+				reset-gpios = <&pinctrl_ap 152 GPIO_ACTIVE_LOW>;
 				max-link-speed = <2>;
 
 				#address-cells = <3>;

@@ -301,7 +302,7 @@ port00: pci@0,0 {
 			port01: pci@1,0 {
 				device_type = "pci";
 				reg = <0x800 0x0 0x0 0x0 0x0>;
-				reset-gpios = <&pinctrl_ap 153 0>;
+				reset-gpios = <&pinctrl_ap 153 GPIO_ACTIVE_LOW>;
 				max-link-speed = <2>;
 
 				#address-cells = <3>;

@@ -321,7 +322,7 @@ port01: pci@1,0 {
 			port02: pci@2,0 {
 				device_type = "pci";
 				reg = <0x1000 0x0 0x0 0x0 0x0>;
-				reset-gpios = <&pinctrl_ap 33 0>;
+				reset-gpios = <&pinctrl_ap 33 GPIO_ACTIVE_LOW>;
 				max-link-speed = <1>;
 
 				#address-cells = <3>;

Signature · 1 line

2.30.2

As the name indicates, #PERST is active low. So fix the DT description to match the HW behaviour.

Fixes ff2a8d91d80c ("arm64: apple: Add PCIe node")

Signed-off-by Marc Zyngier <[email protected]>

Quoted 5 lines · Marc Zyngier

As the name indicates, #PERST is active low. So fix the DT description to match the HW behaviour.

Fixes ff2a8d91d80c ("arm64: apple: Add PCIe node")

Signed-off-by Marc Zyngier <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

Now that #PERST is properly defined as active-low in the device tree, fix the driver to correctly drive the line indemendently of the implied polarity.

Fixes 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")

Suggested-by Pali Rohár <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/pci/controller/pcie-apple.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/controller/pcie-apple.c +1 -1

index 957960a733c4..03bc56f39be5 100644
@@ -540,7 +540,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	rmw_set(PORT_APPCLK_EN, port->base + PORT_APPCLK);
 
 	/* Engage #PERST before setting up the clock */
 
-	gpiod_set_value(reset, 0);
+	gpiod_set_value(reset, 1);
 
 	ret = apple_pcie_setup_refclk(pcie, port);

drivers/pci/controller/pcie-apple.c +0 -0

 	if (ret < 0)

Hi Mark,

Quoted 7 lines · Marc Zyngier

Now that #PERST is properly defined as active-low in the device tree, fix the driver to correctly drive the line indemendently of the implied polarity.

Fixes 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")

Suggested-by Pali Rohár <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Thanks for quickly addressing this!

Do we need a transition path for backward compatibility with old DTs already around? Something like this [0]. You said [1] the DT actually used is not even the one in the kernel, thus how do we guarantee DT and driver switch to the new polarity all at once?

[0] https://lkml.org/lkml/2021/6/24/1049
[1] https://lkml.org/lkml/2021/11/23/455

Quoted 20 lines

---
 drivers/pci/controller/pcie-apple.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/controller/pcie-apple.c +1 -1

index 957960a733c4..03bc56f39be5 100644
@@ -540,7 +540,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	rmw_set(PORT_APPCLK_EN, port->base + PORT_APPCLK);
 
 	/* Engage #PERST before setting up the clock */
 
-	gpiod_set_value(reset, 0);
+	gpiod_set_value(reset, 1);
 
 	ret = apple_pcie_setup_refclk(pcie, port);

drivers/pci/controller/pcie-apple.c +0 -0

 	if (ret < 0)

@@ -551,7 +551,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 

drivers/pci/controller/pcie-apple.c static int apple_pcie_setup_port(struct apple_pcie *pcie, · line 554

Quoted 4 lines

drivers/pci/controller/pcie-apple.c +1 -1

 	/* Deassert #PERST */
 	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
-	gpiod_set_value(reset, 1);
+	gpiod_set_value(reset, 0);

Minor note: if it were me I would coalesce patches 1 and 3 together, otherwise we are insisting on a wrong implementation (patch 1) to later fix it all (this patch).

Quoted 12 lines · Luca Ceresoli

Hi Mark,

Quoted 7 lines · Marc Zyngier

Now that #PERST is properly defined as active-low in the device tree, fix the driver to correctly drive the line indemendently of the implied polarity.

Fixes 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")

Suggested-by Pali Rohár <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Quoted 6 lines

Thanks for quickly addressing this!

Do we need a transition path for backward compatibility with old DTs already around? Something like this [0]. You said [1] the DT actually used is not even the one in the kernel, thus how do we guarantee DT and driver switch to the new polarity all at once?

No. As it turns out, neither u-boot nor OpenBSD (the only two other payloads that can boot on M1) are upstreamed yet. So we're still in that stage where we don't need to maintain backward compatibility. If we don't get this patches merged by the end of this cycle, we will have to revisit this though.

Apologies for the rapid fire (I tend to be much more conservative when resending series), but given that this series has a direct impact on other projects (such as u-boot), I'm trying to converge as quickly as possible.

This series aims at fixing a number of issues for the recently merged Apple PCIe driver, all revolving around the mishandling of #PERST:

- we didn't properly drive #PERST, and we didn't follow the specified timings

- the DT had the wrong polarity, which has impacts on the driver itself

Hopefully, this should address all the issues reported so far.

* From v2:
- Fixed DT
- Fixed #PERST polarity in the driver
- Collected Pali's ack on patch #1

[1] https://lore.kernel.org/r/[email protected]

Marc Zyngier (3):
PCI: apple: Follow the PCIe specifications when resetting the port
arm64: dts: apple: t8103: Fix PCIe #PERST polarity
PCI: apple: Fix #PERST polarity

 arch/arm64/boot/dts/apple/t8103.dtsi |  7 ++++---
 drivers/pci/controller/pcie-apple.c  | 12 +++++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

Quoted 28 lines · Marc Zyngier

Apologies for the rapid fire (I tend to be much more conservative when resending series), but given that this series has a direct impact on other projects (such as u-boot), I'm trying to converge as quickly as possible.

This series aims at fixing a number of issues for the recently merged Apple PCIe driver, all revolving around the mishandling of #PERST:

- we didn't properly drive #PERST, and we didn't follow the specified timings

- the DT had the wrong polarity, which has impacts on the driver itself

Hopefully, this should address all the issues reported so far.

* From v2:
- Fixed DT
- Fixed #PERST polarity in the driver
- Collected Pali's ack on patch #1

[1] https://lore.kernel.org/r/[email protected]

Marc Zyngier (3):
PCI: apple: Follow the PCIe specifications when resetting the port
arm64: dts: apple: t8103: Fix PCIe #PERST polarity
PCI: apple: Fix #PERST polarity

Quoted 3 lines

 arch/arm64/boot/dts/apple/t8103.dtsi |  7 ++++---
 drivers/pci/controller/pcie-apple.c  | 12 +++++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

Hi Bjorn,

this series is v5.16-rcX material for PCI fixes, can you pick patches 1,3 up please ?

Hi Lorenzo, Bjorn,

Quoted 33 lines · Lorenzo Pieralisi

Quoted 31 lines · Marc Zyngier

Apologies for the rapid fire (I tend to be much more conservative when resending series), but given that this series has a direct impact on other projects (such as u-boot), I'm trying to converge as quickly as possible.

This series aims at fixing a number of issues for the recently merged Apple PCIe driver, all revolving around the mishandling of #PERST:

- we didn't properly drive #PERST, and we didn't follow the specified timings

- the DT had the wrong polarity, which has impacts on the driver itself

Hopefully, this should address all the issues reported so far.

* From v2:
- Fixed DT
- Fixed #PERST polarity in the driver
- Collected Pali's ack on patch #1

[1] https://lore.kernel.org/r/[email protected]

Marc Zyngier (3):
PCI: apple: Follow the PCIe specifications when resetting the port
arm64: dts: apple: t8103: Fix PCIe #PERST polarity
PCI: apple: Fix #PERST polarity

 arch/arm64/boot/dts/apple/t8103.dtsi |  7 ++++---
 drivers/pci/controller/pcie-apple.c  | 12 +++++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

Quoted 5 lines

Hi Bjorn,

this series is v5.16-rcX material for PCI fixes, can you pick patches 1,3
up please ?

Do you mind picking patch #2 as well? Or shall I route it somewhere else?

Quoted 42 lines · Marc Zyngier

Hi Lorenzo, Bjorn,

Quoted 38 lines · Lorenzo Pieralisi

Quoted 23 lines · Marc Zyngier

Apologies for the rapid fire (I tend to be much more conservative when resending series), but given that this series has a direct impact on other projects (such as u-boot), I'm trying to converge as quickly as possible.

This series aims at fixing a number of issues for the recently merged Apple PCIe driver, all revolving around the mishandling of #PERST:

- we didn't properly drive #PERST, and we didn't follow the specified timings

- the DT had the wrong polarity, which has impacts on the driver itself

Hopefully, this should address all the issues reported so far.

* From v2:
- Fixed DT
- Fixed #PERST polarity in the driver
- Collected Pali's ack on patch #1

[1] https://lore.kernel.org/r/[email protected]

Quoted 8 lines

Marc Zyngier (3):
PCI: apple: Follow the PCIe specifications when resetting the port
arm64: dts: apple: t8103: Fix PCIe #PERST polarity
PCI: apple: Fix #PERST polarity

 arch/arm64/boot/dts/apple/t8103.dtsi |  7 ++++---
 drivers/pci/controller/pcie-apple.c  | 12 +++++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

Hi Bjorn,

this series is v5.16-rcX material for PCI fixes, can you pick patches 1,3
up please ?

Quoted 1 line

Do you mind picking patch #2 as well? Or shall I route it somewhere else?

We were told that we should not pick up dts changes, they would normally go via the ARM SOC team, not sure whether the fixes policy is different though but I suspect that's not the case.

Quoted 3 lines · Lorenzo Pieralisi

Quoted 1 line · Marc Zyngier

On 2021-11-30 11:56, Lorenzo Pieralisi wrote:

Quoted 3 lines

We were told that we should not pick up dts changes, they would normally go via the ARM SOC team, not sure whether the fixes policy is different though but I suspect that's not the case.

OK. Doesn't really help with keeping these two commit close together, but hey, if that can't be helped...

Arnd, do you mind picking up patch #2 as a 5.16 fix?

Quoted 28 lines · Marc Zyngier

Apologies for the rapid fire (I tend to be much more conservative when resending series), but given that this series has a direct impact on other projects (such as u-boot), I'm trying to converge as quickly as possible.

This series aims at fixing a number of issues for the recently merged Apple PCIe driver, all revolving around the mishandling of #PERST:

- we didn't properly drive #PERST, and we didn't follow the specified timings

- the DT had the wrong polarity, which has impacts on the driver itself

Hopefully, this should address all the issues reported so far.

* From v2:
- Fixed DT
- Fixed #PERST polarity in the driver
- Collected Pali's ack on patch #1

[1] https://lore.kernel.org/r/[email protected]

Marc Zyngier (3):
PCI: apple: Follow the PCIe specifications when resetting the port
arm64: dts: apple: t8103: Fix PCIe #PERST polarity
PCI: apple: Fix #PERST polarity

Quoted 3 lines

 arch/arm64/boot/dts/apple/t8103.dtsi |  7 ++++---
 drivers/pci/controller/pcie-apple.c  | 12 +++++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

Hi Bjorn,

this series is v5.16-rcX material for PCI fixes, can you pick patches 1,3 up please ?

Quoted 35 lines · Lorenzo Pieralisi

Quoted 23 lines · Marc Zyngier

Apologies for the rapid fire (I tend to be much more conservative when resending series), but given that this series has a direct impact on other projects (such as u-boot), I'm trying to converge as quickly as possible.

This series aims at fixing a number of issues for the recently merged Apple PCIe driver, all revolving around the mishandling of #PERST:

- we didn't properly drive #PERST, and we didn't follow the specified timings

- the DT had the wrong polarity, which has impacts on the driver itself

Hopefully, this should address all the issues reported so far.

* From v2:
- Fixed DT
- Fixed #PERST polarity in the driver
- Collected Pali's ack on patch #1

[1] https://lore.kernel.org/r/[email protected]

Quoted 8 lines

Marc Zyngier (3):
PCI: apple: Follow the PCIe specifications when resetting the port
arm64: dts: apple: t8103: Fix PCIe #PERST polarity
PCI: apple: Fix #PERST polarity

 arch/arm64/boot/dts/apple/t8103.dtsi |  7 ++++---
 drivers/pci/controller/pcie-apple.c  | 12 +++++++++++-
 2 files changed, 15 insertions(+), 4 deletions(-)

Hi Bjorn,

Quoted 2 lines

this series is v5.16-rcX material for PCI fixes, can you pick patches 1,3 up please ?

Hi Bjorn,

Arnd acked patch 2, can we send the whole series upstream for one of the upcoming -rcX please ? It is fixing code that was merged in the last merge window.