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).
Bring the port back to life by wiggling the #PERST pin for 100ms
(as per the spec).
Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
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 | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 1bf4d75b61be..bbea5f6e0a68 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -543,6 +543,9 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
if (ret < 0)
return ret;
+ /* Hold #PERST for 100ms as per the spec */
+ gpiod_set_value(reset, 0);
+ msleep(100);
rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
gpiod_set_value(reset, 1);
--
2.30.2
[+cc Pali]
On Wed, Nov 17, 2021 at 04:00:53PM +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).
s/beacuse/because/
> Bring the port back to life by wiggling the #PERST pin for 100ms
> (as per the spec).
I cc'd Pali because I think he's interested in consolidating or
somehow rationalizing delays like this.
If we have a specific spec reference here, I think it would help that
effort. I *think* it's PCIe r5.0, sec 6.6.1, which mentions the 100ms
along with some additional constraints, like waiting 100ms after Link
training completes for ports that support > 5.0 GT/s, whether
Readiness Notifications are used, and CRS Software Visiblity.
> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> 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 | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 1bf4d75b61be..bbea5f6e0a68 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -543,6 +543,9 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
> if (ret < 0)
> return ret;
>
> + /* Hold #PERST for 100ms as per the spec */
> + gpiod_set_value(reset, 0);
> + msleep(100);
> rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
> gpiod_set_value(reset, 1);
>
> --
> 2.30.2
>
Hello!
On Wednesday 17 November 2021 14:12:45 Bjorn Helgaas wrote:
> [+cc Pali]
>
> On Wed, Nov 17, 2021 at 04:00:53PM +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).
>
> s/beacuse/because/
>
> > Bring the port back to life by wiggling the #PERST pin for 100ms
> > (as per the spec).
>
> I cc'd Pali because I think he's interested in consolidating or
> somehow rationalizing delays like this.
>
> If we have a specific spec reference here, I think it would help that
> effort. I *think* it's PCIe r5.0, sec 6.6.1, which mentions the 100ms
> along with some additional constraints, like waiting 100ms after Link
> training completes for ports that support > 5.0 GT/s, whether
> Readiness Notifications are used, and CRS Software Visiblity.
This is not 100ms timeout "after link training completes".
Timeout in this patch is between flipping PERST# signal, so timeout
means how long needs to be endpoint card in reset state. And this
timeout cannot be controller specific. In past I have tried to find this
timeout in specifications, I was not able. Some summary is in my email:
https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
So I would like to know, why was chosen 100ms for msleep() in this
patch?
> > Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> > 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 | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> > index 1bf4d75b61be..bbea5f6e0a68 100644
> > --- a/drivers/pci/controller/pcie-apple.c
> > +++ b/drivers/pci/controller/pcie-apple.c
> > @@ -543,6 +543,9 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
> > if (ret < 0)
> > return ret;
> >
> > + /* Hold #PERST for 100ms as per the spec */
> > + gpiod_set_value(reset, 0);
> > + msleep(100);
> > rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
> > gpiod_set_value(reset, 1);
> >
> > --
> > 2.30.2
> >
On Wed, 17 Nov 2021 20:28:59 +0000,
Pali Rohár <[email protected]> wrote:
>
> Hello!
>
> On Wednesday 17 November 2021 14:12:45 Bjorn Helgaas wrote:
> > [+cc Pali]
> >
> > On Wed, Nov 17, 2021 at 04:00:53PM +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).
> >
> > s/beacuse/because/
> >
> > > Bring the port back to life by wiggling the #PERST pin for 100ms
> > > (as per the spec).
> >
> > I cc'd Pali because I think he's interested in consolidating or
> > somehow rationalizing delays like this.
> >
> > If we have a specific spec reference here, I think it would help that
> > effort. I *think* it's PCIe r5.0, sec 6.6.1, which mentions the 100ms
> > along with some additional constraints, like waiting 100ms after Link
> > training completes for ports that support > 5.0 GT/s, whether
> > Readiness Notifications are used, and CRS Software Visiblity.
>
> This is not 100ms timeout "after link training completes".
>
> Timeout in this patch is between flipping PERST# signal, so timeout
> means how long needs to be endpoint card in reset state. And this
> timeout cannot be controller specific. In past I have tried to find this
> timeout in specifications, I was not able. Some summary is in my email:
> https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
>
> So I would like to know, why was chosen 100ms for msleep() in this
> patch?
Excellent question. I went back to my notes (and the spec), and it
looks like I have mistakenly conflated *two* delays here:
- The post-#PERST delay, which is 100ms, and which is *not* what this
patch is doing while it really should be doing it. This is
documented in the base PCIe spec (in Rev 2.0, this is part of
6.6.1). The amusing part is that on this HW, it seems that only the
delay from the falling edge matters (which is why I didn't spot the
issue).
- The duration of the power-on #PERST assertion (Tpvperl), which is
also 100ms, and documented in the PCIe Card Electromechanical
Specification (Rev 1.0a, 2.2 and 2.2.1).
There is also a third delay (Tperst-clk) which represents the time
required for the clock to ramp up before releasing #PERST. No, there
is no value associated with this.
Having come to my senses, and with these constraints in mind, this is
what I currently have in my tree:
/* Engage #PERST */
gpiod_set_value(reset, 0);
ret = apple_pcie_setup_refclk(pcie, port);
if (ret < 0)
return ret;
/* Hold #PERST for 100ms as per the electromechanical spec */
msleep(100);
rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
gpiod_set_value(reset, 1);
/* Wait for 100ms after #PERST deassertion before anothing else */
msleep(100);
Yes, this is totally overkill, as I assume that each port has gone
through a complete power-off and is only slowly coming back from the
dead.
In practice, I can completely remove the initial Tpvperl delay (we
have been powered-on for a long time already, and the clock is stable
when we come back from setting it up), and cut the second one by half
without observing any ill effect (though I feel safer keeping it to
its nominal value).
If nobody screams, I'll respin something shortly.
M.
--
Without deviation from the norm, progress is not possible.
On Thursday 18 November 2021 10:01:58 Marc Zyngier wrote:
> On Wed, 17 Nov 2021 20:28:59 +0000,
> Pali Rohár <[email protected]> wrote:
> >
> > Hello!
> >
> > On Wednesday 17 November 2021 14:12:45 Bjorn Helgaas wrote:
> > > [+cc Pali]
> > >
> > > On Wed, Nov 17, 2021 at 04:00:53PM +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).
> > >
> > > s/beacuse/because/
> > >
> > > > Bring the port back to life by wiggling the #PERST pin for 100ms
> > > > (as per the spec).
> > >
> > > I cc'd Pali because I think he's interested in consolidating or
> > > somehow rationalizing delays like this.
> > >
> > > If we have a specific spec reference here, I think it would help that
> > > effort. I *think* it's PCIe r5.0, sec 6.6.1, which mentions the 100ms
> > > along with some additional constraints, like waiting 100ms after Link
> > > training completes for ports that support > 5.0 GT/s, whether
> > > Readiness Notifications are used, and CRS Software Visiblity.
> >
> > This is not 100ms timeout "after link training completes".
> >
> > Timeout in this patch is between flipping PERST# signal, so timeout
> > means how long needs to be endpoint card in reset state. And this
> > timeout cannot be controller specific. In past I have tried to find this
> > timeout in specifications, I was not able. Some summary is in my email:
> > https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> >
> > So I would like to know, why was chosen 100ms for msleep() in this
> > patch?
>
> Excellent question. I went back to my notes (and the spec), and it
> looks like I have mistakenly conflated *two* delays here:
>
> - The post-#PERST delay, which is 100ms, and which is *not* what this
> patch is doing while it really should be doing it. This is
> documented in the base PCIe spec (in Rev 2.0, this is part of
> 6.6.1). The amusing part is that on this HW, it seems that only the
> delay from the falling edge matters (which is why I didn't spot the
> issue).
>
> - The duration of the power-on #PERST assertion (Tpvperl), which is
> also 100ms, and documented in the PCIe Card Electromechanical
> Specification (Rev 1.0a, 2.2 and 2.2.1).
I think that your patch is doing also something different. It uses
PERST# signal to reset card _after_ card was fully powered on and
_maybe_ link was already established (depends on bootloader if it
initialized PCIe, etc...).
Important is that this reset is really needed for some cards (e.g. lot
of Atheros wifi cards as they can be stuck somewhere in broken state)
and I do not think it is Tpvperl delay. More controller drivers add some
delay between flipping PERST# signal. In past I wrote summary of it:
https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
> There is also a third delay (Tperst-clk) which represents the time
> required for the clock to ramp up before releasing #PERST. No, there
> is no value associated with this.
But there is minimal value for Tperst-clk which is 100us defined in PCIe
CEM spec (3.0) and also in M.2 CEM spec.
> Having come to my senses, and with these constraints in mind, this is
> what I currently have in my tree:
>
> /* Engage #PERST */
> gpiod_set_value(reset, 0);
>
> ret = apple_pcie_setup_refclk(pcie, port);
> if (ret < 0)
> return ret;
>
> /* Hold #PERST for 100ms as per the electromechanical spec */
> msleep(100);
> rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
> gpiod_set_value(reset, 1);
> /* Wait for 100ms after #PERST deassertion before anothing else */
> msleep(100);
>
> Yes, this is totally overkill, as I assume that each port has gone
> through a complete power-off and is only slowly coming back from the
> dead.
For power-on it is probably overkill, but I think that delay between
flipping PERST# should be there. IIRC Compex WLE1216 wifi card needs to
be at least 10-11ms in reset. Last year, during testing of this card I
saw that if PERST#-based reset was shorter then card was completely
undetected.
> In practice, I can completely remove the initial Tpvperl delay (we
> have been powered-on for a long time already, and the clock is stable
> when we come back from setting it up), and cut the second one by half
> without observing any ill effect (though I feel safer keeping it to
> its nominal value).
My opinion is that this patch does not power on/off card in PCIe slot.
And because card is powered-on for a long time (as you wrote), it means
that Tpvperl delay does not apply here. That is why I think that
different delay (How long should be PCIe card in Warm Reset state)
should be used _between_ flipping PERST# signal.
And of course after the releasing PERST# that 100ms post-PERST# delay is
required.
I have an idea to move PERST# handling (with all delays) from controller
drivers to pci core functions. Because basically every driver
re-implements these delays in its probe function. I wrote this idea with
some details in email. If you have a time, could you look at it? I
summarized here also details about delays (like Tpvperl, Tperstclk, ..):
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
> If nobody screams, I'll respin something shortly.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
On Thu, 18 Nov 2021 10:01:58 +0000,
Marc Zyngier <[email protected]> wrote:
>
> There is also a third delay (Tperst-clk) which represents the time
> required for the clock to ramp up before releasing #PERST. No, there
> is no value associated with this.
Actually, there is. At least the PCIe CMS r2.0 (2.6.2. AC
Specifications) does provide a table of the timings. Tperst-clk has a
minimum value of 100us.
Which means I can tighten things further.
M.
--
Without deviation from the norm, progress is not possible.
On Thu, 18 Nov 2021 10:31:56 +0000,
Pali Rohár <[email protected]> wrote:
>
> On Thursday 18 November 2021 10:01:58 Marc Zyngier wrote:
> > On Wed, 17 Nov 2021 20:28:59 +0000,
> > Pali Rohár <[email protected]> wrote:
> > >
> > > Hello!
> > >
> > > On Wednesday 17 November 2021 14:12:45 Bjorn Helgaas wrote:
> > > > [+cc Pali]
> > > >
> > > > On Wed, Nov 17, 2021 at 04:00:53PM +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).
> > > >
> > > > s/beacuse/because/
> > > >
> > > > > Bring the port back to life by wiggling the #PERST pin for 100ms
> > > > > (as per the spec).
> > > >
> > > > I cc'd Pali because I think he's interested in consolidating or
> > > > somehow rationalizing delays like this.
> > > >
> > > > If we have a specific spec reference here, I think it would help that
> > > > effort. I *think* it's PCIe r5.0, sec 6.6.1, which mentions the 100ms
> > > > along with some additional constraints, like waiting 100ms after Link
> > > > training completes for ports that support > 5.0 GT/s, whether
> > > > Readiness Notifications are used, and CRS Software Visiblity.
> > >
> > > This is not 100ms timeout "after link training completes".
> > >
> > > Timeout in this patch is between flipping PERST# signal, so timeout
> > > means how long needs to be endpoint card in reset state. And this
> > > timeout cannot be controller specific. In past I have tried to find this
> > > timeout in specifications, I was not able. Some summary is in my email:
> > > https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > >
> > > So I would like to know, why was chosen 100ms for msleep() in this
> > > patch?
> >
> > Excellent question. I went back to my notes (and the spec), and it
> > looks like I have mistakenly conflated *two* delays here:
> >
> > - The post-#PERST delay, which is 100ms, and which is *not* what this
> > patch is doing while it really should be doing it. This is
> > documented in the base PCIe spec (in Rev 2.0, this is part of
> > 6.6.1). The amusing part is that on this HW, it seems that only the
> > delay from the falling edge matters (which is why I didn't spot the
> > issue).
> >
> > - The duration of the power-on #PERST assertion (Tpvperl), which is
> > also 100ms, and documented in the PCIe Card Electromechanical
> > Specification (Rev 1.0a, 2.2 and 2.2.1).
>
> I think that your patch is doing also something different. It uses
> PERST# signal to reset card _after_ card was fully powered on and
> _maybe_ link was already established (depends on bootloader if it
> initialized PCIe, etc...).
Yes, and that's what I have said above.
> Important is that this reset is really needed for some cards (e.g. lot
> of Atheros wifi cards as they can be stuck somewhere in broken state)
> and I do not think it is Tpvperl delay. More controller drivers add some
> delay between flipping PERST# signal. In past I wrote summary of it:
> https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
I don't think any of this applies here.
> > There is also a third delay (Tperst-clk) which represents the time
> > required for the clock to ramp up before releasing #PERST. No, there
> > is no value associated with this.
>
> But there is minimal value for Tperst-clk which is 100us defined in PCIe
> CEM spec (3.0) and also in M.2 CEM spec.
Which is what I have pointed out in my followup to my original email.
>
> > Having come to my senses, and with these constraints in mind, this is
> > what I currently have in my tree:
> >
> > /* Engage #PERST */
> > gpiod_set_value(reset, 0);
> >
> > ret = apple_pcie_setup_refclk(pcie, port);
> > if (ret < 0)
> > return ret;
> >
> > /* Hold #PERST for 100ms as per the electromechanical spec */
> > msleep(100);
> > rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
> > gpiod_set_value(reset, 1);
> > /* Wait for 100ms after #PERST deassertion before anothing else */
> > msleep(100);
> >
> > Yes, this is totally overkill, as I assume that each port has gone
> > through a complete power-off and is only slowly coming back from the
> > dead.
>
> For power-on it is probably overkill, but I think that delay between
> flipping PERST# should be there. IIRC Compex WLE1216 wifi card needs to
> be at least 10-11ms in reset. Last year, during testing of this card I
> saw that if PERST#-based reset was shorter then card was completely
> undetected.
The only delay we really need is Tperst-clk. Random bugs on random
devices don't apply here, as the system is completely closed (there is
no slot to add anything). Once we have TB running one of these days,
we will see whether this still holds.
> > In practice, I can completely remove the initial Tpvperl delay (we
> > have been powered-on for a long time already, and the clock is stable
> > when we come back from setting it up), and cut the second one by half
> > without observing any ill effect (though I feel safer keeping it to
> > its nominal value).
>
> My opinion is that this patch does not power on/off card in PCIe slot.
> And because card is powered-on for a long time (as you wrote), it means
> that Tpvperl delay does not apply here. That is why I think that
> different delay (How long should be PCIe card in Warm Reset state)
> should be used _between_ flipping PERST# signal.
My reading of the spec is that the only thing we need while #PERST is
asserted is Tperst-clk. The value you keep arguing about doesn't seem
to exist as such in the spec, because it appears to be endpoint
specific.
> And of course after the releasing PERST# that 100ms post-PERST# delay is
> required.
That we agree on.
> I have an idea to move PERST# handling (with all delays) from controller
> drivers to pci core functions. Because basically every driver
> re-implements these delays in its probe function. I wrote this idea with
> some details in email. If you have a time, could you look at it? I
> summarized here also details about delays (like Tpvperl, Tperstclk, ..):
> https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
That's a laudable goal. What isn't clear to me is whether you intend
to move the whole state machine into core code, or just have a set of
helpers that the driver calls into. IMO, the former is what we really
need, while the latter only rids us of the simple stuff.
M.
--
Without deviation from the norm, progress is not possible.
On Thursday 18 November 2021 12:57:46 Marc Zyngier wrote:
> On Thu, 18 Nov 2021 10:31:56 +0000,
> Pali Rohár <[email protected]> wrote:
> > For power-on it is probably overkill, but I think that delay between
> > flipping PERST# should be there. IIRC Compex WLE1216 wifi card needs to
> > be at least 10-11ms in reset. Last year, during testing of this card I
> > saw that if PERST#-based reset was shorter then card was completely
> > undetected.
>
> The only delay we really need is Tperst-clk. Random bugs on random
> devices don't apply here, as the system is completely closed (there is
> no slot to add anything). Once we have TB running one of these days,
> we will see whether this still holds.
Ok!
> > > In practice, I can completely remove the initial Tpvperl delay (we
> > > have been powered-on for a long time already, and the clock is stable
> > > when we come back from setting it up), and cut the second one by half
> > > without observing any ill effect (though I feel safer keeping it to
> > > its nominal value).
> >
> > My opinion is that this patch does not power on/off card in PCIe slot.
> > And because card is powered-on for a long time (as you wrote), it means
> > that Tpvperl delay does not apply here. That is why I think that
> > different delay (How long should be PCIe card in Warm Reset state)
> > should be used _between_ flipping PERST# signal.
>
> My reading of the spec is that the only thing we need while #PERST is
> asserted is Tperst-clk. The value you keep arguing about doesn't seem
> to exist as such in the spec, because it appears to be endpoint
> specific.
Well, I was not able to find it in the spec too, that is why I do not
know...
> > And of course after the releasing PERST# that 100ms post-PERST# delay is
> > required.
>
> That we agree on.
>
> > I have an idea to move PERST# handling (with all delays) from controller
> > drivers to pci core functions. Because basically every driver
> > re-implements these delays in its probe function. I wrote this idea with
> > some details in email. If you have a time, could you look at it? I
> > summarized here also details about delays (like Tpvperl, Tperstclk, ..):
> > https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
>
> That's a laudable goal. What isn't clear to me is whether you intend
> to move the whole state machine into core code, or just have a set of
> helpers that the driver calls into. IMO, the former is what we really
> need, while the latter only rids us of the simple stuff.
Now I'm just collecting comments and feedbacks for this idea. I think
that state machine in core code is what we need.