[PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[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/20211122104156.518063-1-maz@kernel.org

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(-)

-- 
2.30.2

[PATCH v3 1/3] PCI: apple: Follow the PCIe specifications when resetting the port

[Marc Zyngier]

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 <pali@kernel.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/controller/pcie-apple.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 1bf4d75b61be..957960a733c4 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -539,13 +539,23 @@ 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);
+
 	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);
+
+	/* Deassert #PERST */
 	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
 	gpiod_set_value(reset, 1);
 
+	/* Wait for 100ms after #PERST deassertion (PCIe r2.0, 6.6.1) */
+	msleep(100);
+
 	ret = readl_relaxed_poll_timeout(port->base + PORT_STATUS, stat,
 					 stat & PORT_STATUS_READY, 100, 250000);
 	if (ret < 0) {
-- 
2.30.2

[PATCH v3 2/3] arm64: dts: apple: t8103: Fix PCIe #PERST polarity

[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 <maz@kernel.org>
---
 arch/arm64/boot/dts/apple/t8103.dtsi | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index fc8b2bb06ffe..e22c9433d5e0 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -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>;
-- 
2.30.2

[PATCH v3 3/3] PCI: apple: Fix #PERST polarity

[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 <pali@kernel.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/pci/controller/pcie-apple.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 957960a733c4..03bc56f39be5 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -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);
 	if (ret < 0)
@@ -551,7 +551,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 
 	/* Deassert #PERST */
 	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
-	gpiod_set_value(reset, 1);
+	gpiod_set_value(reset, 0);
 
 	/* Wait for 100ms after #PERST deassertion (PCIe r2.0, 6.6.1) */
 	msleep(100);
-- 
2.30.2

Re: [PATCH v3 2/3] arm64: dts: apple: t8103: Fix PCIe #PERST polarity

[Mark Kettenis]

> From: Marc Zyngier <maz@kernel.org>
> 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 <maz@kernel.org>

Reviewed-by: Mark Kettenis <kettenis@openbsd.org>

> ---
>  arch/arm64/boot/dts/apple/t8103.dtsi | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
> index fc8b2bb06ffe..e22c9433d5e0 100644
> --- a/arch/arm64/boot/dts/apple/t8103.dtsi
> +++ b/arch/arm64/boot/dts/apple/t8103.dtsi
> @@ -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>;
> -- 
> 2.30.2
> 
> 

Re: [PATCH v3 3/3] PCI: apple: Fix #PERST polarity

[Luca Ceresoli]

Hi Mark,

On 23/11/21 19:06, Marc Zyngier wrote:
> 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 <pali@kernel.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

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

> ---
>  drivers/pci/controller/pcie-apple.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 957960a733c4..03bc56f39be5 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -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);
>  	if (ret < 0)
> @@ -551,7 +551,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  
>  	/* 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).

-- 
Luca

Re: [PATCH v3 3/3] PCI: apple: Fix #PERST polarity

[Marc Zyngier]

On Tue, 23 Nov 2021 21:36:11 +0000,
Luca Ceresoli <luca@lucaceresoli.net> wrote:
> 
> Hi Mark,
> 
> On 23/11/21 19:06, Marc Zyngier wrote:
> > 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 <pali@kernel.org>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> 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.

> 
> [0] https://lkml.org/lkml/2021/6/24/1049
> [1] https://lkml.org/lkml/2021/11/23/455
> 
> > ---
> >  drivers/pci/controller/pcie-apple.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index 957960a733c4..03bc56f39be5 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -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);
> >  	if (ret < 0)
> > @@ -551,7 +551,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
> >  
> >  	/* 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).

The first patch is a clear bug fix that has a direct HW impact. The
second patch is only sugar coating with zero material impact
(absolutely nothing changes in the way the HW is driven). Squashing
these two patches would be absolutely the wrong thing to do.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 3/3] PCI: apple: Fix #PERST polarity

[Luca Ceresoli]

Hi,

On 24/11/21 10:02, Marc Zyngier wrote:
> On Tue, 23 Nov 2021 21:36:11 +0000,
> Luca Ceresoli <luca@lucaceresoli.net> wrote:
>>
>> Hi Mark,
>>
>> On 23/11/21 19:06, Marc Zyngier wrote:
>>> 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 <pali@kernel.org>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>
>> 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.

Good news!

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

Re: [PATCH v3 2/3] arm64: dts: apple: t8103: Fix PCIe #PERST polarity

[Luca Ceresoli]

Hi,

On 23/11/21 19:06, Marc Zyngier wrote:
> 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 <maz@kernel.org>

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

Re: [PATCH v3 1/3] PCI: apple: Follow the PCIe specifications when resetting the port

[Luca Ceresoli]

Hi,

On 23/11/21 19:06, Marc Zyngier wrote:
> 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 <pali@kernel.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>>
> ---
>  drivers/pci/controller/pcie-apple.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 1bf4d75b61be..957960a733c4 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -539,13 +539,23 @@ 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 */

"Assert" is the verb typically used instead of "Engage".

With this fixed, or even without as this is a pretty urgent fix:

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

Re: [PATCH v3 1/3] PCI: apple: Follow the PCIe specifications when resetting the port

[Robin Murphy]

On 2021-11-23 18:06, Marc Zyngier wrote:
> 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 <pali@kernel.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/controller/pcie-apple.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 1bf4d75b61be..957960a733c4 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -539,13 +539,23 @@ 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);

FWIW, given that getting the GPIO with GPIOD_OUT_LOW should have had 
this effect in the first place, if this isn't a no-op at this point then 
it would hint at something being more significantly wrong down at the 
GPIO/pinctrl end :/

Once you fix the polarity in the later patch, though, adding the 
explicit reset assertion here does seem a far nicer option than fiddling 
the flags to preserve the implicit assertion earlier.

Cheers,
Robin.

> +
>   	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);
> +
> +	/* Deassert #PERST */
>   	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
>   	gpiod_set_value(reset, 1);
>   
> +	/* Wait for 100ms after #PERST deassertion (PCIe r2.0, 6.6.1) */
> +	msleep(100);
> +
>   	ret = readl_relaxed_poll_timeout(port->base + PORT_STATUS, stat,
>   					 stat & PORT_STATUS_READY, 100, 250000);
>   	if (ret < 0) {
> 

Re: [PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[Lorenzo Pieralisi]

On Tue, Nov 23, 2021 at 06:06:33PM +0000, Marc Zyngier wrote:
> 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/20211122104156.518063-1-maz@kernel.org
> 
> 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 ?

Thank you very much.

Lorenzo

Re: [PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[Marc Zyngier]

Hi Lorenzo, Bjorn,

On 2021-11-30 11:56, Lorenzo Pieralisi wrote:
> On Tue, Nov 23, 2021 at 06:06:33PM +0000, Marc Zyngier wrote:
>> 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/20211122104156.518063-1-maz@kernel.org
>> 
>> 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 ?

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

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[Lorenzo Pieralisi]

On Tue, Nov 30, 2021 at 11:59:32AM +0000, Marc Zyngier wrote:
> Hi Lorenzo, Bjorn,
> 
> On 2021-11-30 11:56, Lorenzo Pieralisi wrote:
> > On Tue, Nov 23, 2021 at 06:06:33PM +0000, Marc Zyngier wrote:
> > > 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/20211122104156.518063-1-maz@kernel.org
> > > 
> > > 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 ?
> 
> 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.

Thanks,
Lorenzo

Re: [PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[Marc Zyngier]

+ Arnd,

On Tue, 30 Nov 2021 12:12:37 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> On Tue, Nov 30, 2021 at 11:59:32AM +0000, Marc Zyngier wrote:
> > Hi Lorenzo, Bjorn,
> > 
> > On 2021-11-30 11:56, Lorenzo Pieralisi wrote:
> > > On Tue, Nov 23, 2021 at 06:06:33PM +0000, Marc Zyngier wrote:
> > > > 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/20211122104156.518063-1-maz@kernel.org
> > > > 
> > > > 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 ?
> > 
> > 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.

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?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 2/3] arm64: dts: apple: t8103: Fix PCIe #PERST polarity

[Arnd Bergmann]

On Wed, Nov 24, 2021 at 10:17 AM Luca Ceresoli <luca@lucaceresoli.net> wrote:
> On 23/11/21 19:06, Marc Zyngier wrote:
> > 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 <maz@kernel.org>
>
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

Acked-by: Arnd Bergmann <arnd@arndb.de>

If the driver changes are not yet forwarded to Linus, feel free to add
this one as well. Otherwise please send it to Hector Martin so he can
add it to the other Apple DT fixes for 5.16.

      Arnd

Re: [PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[Arnd Bergmann]

On Tue, Nov 30, 2021 at 1:45 PM Marc Zyngier <maz@kernel.org> wrote:
> On Tue, 30 Nov 2021 12:12:37 +0000, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Tue, Nov 30, 2021 at 11:59:32AM +0000, Marc Zyngier wrote:
> > > On 2021-11-30 11:56, Lorenzo Pieralisi wrote:
> >
> > 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?

I try not to bypass the platform maintainers, I'd prefer if this came
my way through
the asahi tree (just replied to the patch as well). In this case it
sounds like there
is a good reason to have it go in along with the driver change, so that's fine
as well, and I provided an Ack for that.

       Arnd

Re: [PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[Lorenzo Pieralisi]

On Tue, Nov 30, 2021 at 11:56:32AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Nov 23, 2021 at 06:06:33PM +0000, Marc Zyngier wrote:
> > 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/20211122104156.518063-1-maz@kernel.org
> > 
> > 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 ?

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.

Thanks,
Lorenzo

Re: [PATCH v3 1/3] PCI: apple: Follow the PCIe specifications when resetting the port

[Bjorn Helgaas]

On Tue, Nov 23, 2021 at 06:06:34PM +0000, Marc Zyngier wrote:
> 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

Unless somebody objects, I'll s/#PERST/PERST#/ to match the spec
usage, both here and in the comments below.

I also notice gpiod_get_from_of_node(..., "#PERST") earlier in
apple_pcie_setup_port().  If it wouldn't break anything, I'd like to
change that, too.

> Implementing this results in a booting system.
> 
> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> Acked-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/controller/pcie-apple.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 1bf4d75b61be..957960a733c4 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -539,13 +539,23 @@ 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);
> +
>  	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);

Spec says "min 100us from REFCLK stable before PERST# inactive".  So I
guess when apple_pcie_setup_refclk() returns, we know REFCLK is
already stable?

> +	/* Deassert #PERST */
>  	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
>  	gpiod_set_value(reset, 1);
>  
> +	/* Wait for 100ms after #PERST deassertion (PCIe r2.0, 6.6.1) */
> +	msleep(100);

Does this port support speeds greater than 5 GT/s?  If so, 6.6.1 says
we need "100ms after Link training completes," not just after
deasserting PERST#.

I'll update this citation to "PCIe r5.0, 6.6.1" to reference the
current spec.

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

Re: [PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[Bjorn Helgaas]

On Tue, Dec 07, 2021 at 10:16:32AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Nov 30, 2021 at 11:56:32AM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Nov 23, 2021 at 06:06:33PM +0000, Marc Zyngier wrote:
> > > 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/20211122104156.518063-1-maz@kernel.org
> > > 
> > > 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 ?
> 
> 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.

I put all three of these on for-linus and will ask Linus to pull them
before -rc5.

I do have open questions about the PERST# timing, but we can update
this if needed.

[PATCH v3 0/3] PCI: apple: Assorted #PERST fixes

[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/20211122104156.518063-1-maz@kernel.org

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(-)

-- 
2.30.2