diff mbox series

PCI: apple: Configure link speeds properly

Message ID 20211122111332.72264-1-marcan@marcan.st (mailing list archive)
State Rejected
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: apple: Configure link speeds properly | expand

Commit Message

Hector Martin Nov. 22, 2021, 11:13 a.m. UTC
This sets the maximum link speed from the devicetree, and also requests
a link speed change from the controller. Without the request, the link
always comes up at Gen1 initially, and the core PCIe code complains
about a bandwidth bottleneck.

It turns out ASPM ends up retraining at a higher speed anyway even
without this code, but let's not rely on that.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/pci/controller/pcie-apple.c | 53 +++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Rob Herring (Arm) Nov. 24, 2021, 2:23 a.m. UTC | #1
On Mon, Nov 22, 2021 at 4:13 AM Hector Martin <marcan@marcan.st> wrote:
>
> This sets the maximum link speed from the devicetree, and also requests
> a link speed change from the controller. Without the request, the link
> always comes up at Gen1 initially, and the core PCIe code complains
> about a bandwidth bottleneck.
>
> It turns out ASPM ends up retraining at a higher speed anyway even
> without this code, but let's not rely on that.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/pci/controller/pcie-apple.c | 53 +++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index 03bfe977c579..073cbac49d8b 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -30,6 +30,10 @@
>  #include <linux/of_irq.h>
>  #include <linux/pci-ecam.h>
>
> +#include "../pci.h"
> +/* Apple PCIe is based on DesignWare IP and shares some registers */
> +#include "dwc/pcie-designware.h"

I'm starting to regret this not being part of the DWC driver...

> +
>  #define CORE_RC_PHYIF_CTL              0x00024
>  #define   CORE_RC_PHYIF_CTL_RUN                BIT(0)
>  #define CORE_RC_PHYIF_STAT             0x00028
> @@ -130,9 +134,13 @@
>   */
>  #define DOORBELL_ADDR          CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
>
> +/* The offset of the PCIe capabilities structure in bridge config space */
> +#define PCIE_CAP_BASE          0x70

This offset is discoverable, so don't hardcode it.

> +
>  struct apple_pcie {
>         struct mutex            lock;
>         struct device           *dev;
> +       struct pci_config_window *cfg;
>         void __iomem            *base;
>         struct irq_domain       *domain;
>         unsigned long           *bitmap;
> @@ -506,6 +514,48 @@ static u32 apple_pcie_rid2sid_write(struct apple_pcie_port *port,
>         return readl_relaxed(port->base + PORT_RID2SID(idx));
>  }
>
> +static inline void __iomem *bridge_reg(struct apple_pcie_port *port,
> +                                                 int where)
> +{
> +       struct pci_config_window *cfg = port->pcie->cfg;
> +
> +       return cfg->win + PCIE_ECAM_OFFSET(0, PCI_DEVFN(port->idx, 0), where);
> +}
> +
> +static void apple_pcie_unlock_dwc_regs(struct apple_pcie_port *port)
> +{
> +       rmw_set(PCIE_DBI_RO_WR_EN, bridge_reg(port, PCIE_MISC_CONTROL_1_OFF));
> +}
> +
> +static void apple_pcie_lock_dwc_regs(struct apple_pcie_port *port)
> +{
> +       rmw_clear(PCIE_DBI_RO_WR_EN, bridge_reg(port, PCIE_MISC_CONTROL_1_OFF));
> +}
> +
> +static int apple_pcie_link_configure_max_speed(struct apple_pcie_port *port)
> +{
> +       int max_gen;
> +       u32 ctrl2;
> +
> +       max_gen = of_pci_get_max_link_speed(port->np);
> +       if (max_gen < 0) {
> +               dev_err(port->pcie->dev, "max link speed not specified\n");

Better to fail than limp along in gen1? Though you don't check the
return value...

Usually, the DT property is there to limit the speed when there's a
board limitation.

> +               return max_gen;
> +       }
> +
> +       ctrl2 = readw_relaxed(bridge_reg(port, PCIE_CAP_BASE + PCI_EXP_LNKCTL2));
> +       ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
> +       ctrl2 |= FIELD_PREP(PCI_EXP_LNKCTL2_TLS, max_gen);
> +       writew_relaxed(ctrl2, bridge_reg(port, PCIE_CAP_BASE + PCI_EXP_LNKCTL2));
> +
> +       apple_pcie_unlock_dwc_regs(port);
> +       rmw_set(PORT_LOGIC_SPEED_CHANGE,
> +               bridge_reg(port, PCIE_LINK_WIDTH_SPEED_CONTROL));
> +       apple_pcie_lock_dwc_regs(port);
> +
> +       return 0;
> +}
> +
>  static int apple_pcie_setup_port(struct apple_pcie *pcie,
>                                  struct device_node *np)
>  {
> @@ -577,6 +627,8 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>         ret = apple_pcie_port_register_irqs(port);
>         WARN_ON(ret);
>
> +       apple_pcie_link_configure_max_speed(port);
> +
>         writel_relaxed(PORT_LTSSMCTL_START, port->base + PORT_LTSSMCTL);
>
>         if (!wait_for_completion_timeout(&pcie->event, HZ / 10))
> @@ -762,6 +814,7 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>                 return -ENOMEM;
>
>         pcie->dev = dev;
> +       pcie->cfg = cfg;
>
>         mutex_init(&pcie->lock);
>
> --
> 2.33.0
>
Hector Martin Nov. 24, 2021, 7:40 a.m. UTC | #2
On 24/11/2021 11.23, Rob Herring wrote:
>> +#include "../pci.h"
>> +/* Apple PCIe is based on DesignWare IP and shares some registers */
>> +#include "dwc/pcie-designware.h"
> 
> I'm starting to regret this not being part of the DWC driver...

Main issue is the DWC driver seems to have a pretty hard-coded 
assumption of one port per controller, plus does a bunch of stuff 
differently for the higher layers. It seems Apple used the DWC PHY/LTSSM 
bits, then rolled their own upper layer.

>> +/* The offset of the PCIe capabilities structure in bridge config space */
>> +#define PCIE_CAP_BASE          0x70
> 
> This offset is discoverable, so don't hardcode it.

Sure, it just means I have to reinvent the PCI capability lookup wheel 
again. I'd love to use the regular accessors, but the infrastructure 
isn't up to the point where we can do that yet yere. DWC also reinvents 
this wheel, but we can't reuse that code because it pokes these 
registers through a separate reg range, not config space (even though it 
seems like they should be the same thing? I'm not sure what's going on 
in the DWC devices... for the Apple controller it's just the ECAM).

>> +       max_gen = of_pci_get_max_link_speed(port->np);
>> +       if (max_gen < 0) {
>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
> 
> Better to fail than limp along in gen1? Though you don't check the
> return value...
> 
> Usually, the DT property is there to limit the speed when there's a
> board limitation.

The default *setting* is actually Gen4, but without 
PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make 
more sense to only set the LNKCTL field if max-link-speed is specified, 
and unconditionally poke that bit. That'll get us Gen4 by default (or 
even presumably Gen5 in future controllers, if everything else stays 
compatible).
Rob Herring (Arm) Nov. 29, 2021, 3:02 p.m. UTC | #3
On Wed, Nov 24, 2021 at 1:40 AM Hector Martin <marcan@marcan.st> wrote:
>
> On 24/11/2021 11.23, Rob Herring wrote:
> >> +#include "../pci.h"
> >> +/* Apple PCIe is based on DesignWare IP and shares some registers */
> >> +#include "dwc/pcie-designware.h"
> >
> > I'm starting to regret this not being part of the DWC driver...
>
> Main issue is the DWC driver seems to have a pretty hard-coded
> assumption of one port per controller, plus does a bunch of stuff
> differently for the higher layers. It seems Apple used the DWC PHY/LTSSM
> bits, then rolled their own upper layer.

Yeah, it would need some work...

> >> +/* The offset of the PCIe capabilities structure in bridge config space */
> >> +#define PCIE_CAP_BASE          0x70
> >
> > This offset is discoverable, so don't hardcode it.
>
> Sure, it just means I have to reinvent the PCI capability lookup wheel
> again. I'd love to use the regular accessors, but the infrastructure
> isn't up to the point where we can do that yet yere. DWC also reinvents
> this wheel, but we can't reuse that code because it pokes these
> registers through a separate reg range, not config space (even though it
> seems like they should be the same thing? I'm not sure what's going on
> in the DWC devices... for the Apple controller it's just the ECAM).

Since it is just ECAM, can you use the regular config space accessors?

> >> +       max_gen = of_pci_get_max_link_speed(port->np);
> >> +       if (max_gen < 0) {
> >> +               dev_err(port->pcie->dev, "max link speed not specified\n");
> >
> > Better to fail than limp along in gen1? Though you don't check the
> > return value...
> >
> > Usually, the DT property is there to limit the speed when there's a
> > board limitation.
>
> The default *setting* is actually Gen4, but without
> PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make
> more sense to only set the LNKCTL field if max-link-speed is specified,
> and unconditionally poke that bit. That'll get us Gen4 by default (or
> even presumably Gen5 in future controllers, if everything else stays
> compatible).

You already do some setup in firmware for ECAM, right? I think it
would be better if you can do any default setup there and then
max-link-speed is only an override for the kernel.

Rob
Hector Martin Dec. 2, 2021, 4:55 a.m. UTC | #4
On 30/11/2021 00.02, Rob Herring wrote:
>> Sure, it just means I have to reinvent the PCI capability lookup wheel
>> again. I'd love to use the regular accessors, but the infrastructure
>> isn't up to the point where we can do that yet yere. DWC also reinvents
>> this wheel, but we can't reuse that code because it pokes these
>> registers through a separate reg range, not config space (even though it
>> seems like they should be the same thing? I'm not sure what's going on
>> in the DWC devices... for the Apple controller it's just the ECAM).
> 
> Since it is just ECAM, can you use the regular config space accessors?

The problem is this is before the PCI objects are created, so those 
wouldn't work since they expect to be called on a pci_dev and such.

>>>> +       max_gen = of_pci_get_max_link_speed(port->np);
>>>> +       if (max_gen < 0) {
>>>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
>>>
>>> Better to fail than limp along in gen1? Though you don't check the
>>> return value...
>>>
>>> Usually, the DT property is there to limit the speed when there's a
>>> board limitation.
>>
>> The default *setting* is actually Gen4, but without
>> PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make
>> more sense to only set the LNKCTL field if max-link-speed is specified,
>> and unconditionally poke that bit. That'll get us Gen4 by default (or
>> even presumably Gen5 in future controllers, if everything else stays
>> compatible).
> 
> You already do some setup in firmware for ECAM, right? I think it
> would be better if you can do any default setup there and then
> max-link-speed is only an override for the kernel.

I thought the PCIE_LINK_WIDTH_SPEED_CONTROL thing had to be set later, 
but trying it now I realized we were missing a bit of initialization 
that was causing it not to work. Indeed it can be done there and we can 
drop it from the kernel.

We could even do the max-link-speed thing in m1n1 if we want. It has 
access to the value from the ADT directly, which to be correct we'd have 
to dynamically transplant to the DT, since there's at least one device 
that has different PCIe devices on one port depending on hardware 
variant, while sharing a devicetree. If we're okay with the kernel just 
not implementing this feature for now, we can say it's the bootloader's job.

Ultimately we ship the DTs along with m1n1, so there's an argument that 
if some day we need to override the max-link-speed for whatever reason 
over what the ADT says, well, we'd be shipping the updated DT along with 
m1n1 anyway, so we might as well make m1n1 do it... if so, it might make 
sense to drop those properties from the actual DTs we ship altogether, 
at least for now.

If we decide to make it m1n1's job entirely, we can drop this patch 
altogether, at least for now (I can't say how this will interact with 
suspend/resume and other power management, and hotplug... but we'll open 
that can of worms when we get there).
Rob Herring (Arm) Dec. 2, 2021, 2:33 p.m. UTC | #5
On Wed, Dec 1, 2021 at 10:55 PM Hector Martin <marcan@marcan.st> wrote:
>
> On 30/11/2021 00.02, Rob Herring wrote:
> >> Sure, it just means I have to reinvent the PCI capability lookup wheel
> >> again. I'd love to use the regular accessors, but the infrastructure
> >> isn't up to the point where we can do that yet yere. DWC also reinvents
> >> this wheel, but we can't reuse that code because it pokes these
> >> registers through a separate reg range, not config space (even though it
> >> seems like they should be the same thing? I'm not sure what's going on
> >> in the DWC devices... for the Apple controller it's just the ECAM).
> >
> > Since it is just ECAM, can you use the regular config space accessors?
>
> The problem is this is before the PCI objects are created, so those
> wouldn't work since they expect to be called on a pci_dev and such.

Ah, right. That's something I want to fix because lots of drivers have
2 sets of accessors for this reason.

> >>>> +       max_gen = of_pci_get_max_link_speed(port->np);
> >>>> +       if (max_gen < 0) {
> >>>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
> >>>
> >>> Better to fail than limp along in gen1? Though you don't check the
> >>> return value...
> >>>
> >>> Usually, the DT property is there to limit the speed when there's a
> >>> board limitation.
> >>
> >> The default *setting* is actually Gen4, but without
> >> PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make
> >> more sense to only set the LNKCTL field if max-link-speed is specified,
> >> and unconditionally poke that bit. That'll get us Gen4 by default (or
> >> even presumably Gen5 in future controllers, if everything else stays
> >> compatible).
> >
> > You already do some setup in firmware for ECAM, right? I think it
> > would be better if you can do any default setup there and then
> > max-link-speed is only an override for the kernel.
>
> I thought the PCIE_LINK_WIDTH_SPEED_CONTROL thing had to be set later,
> but trying it now I realized we were missing a bit of initialization
> that was causing it not to work. Indeed it can be done there and we can
> drop it from the kernel.
>
> We could even do the max-link-speed thing in m1n1 if we want. It has
> access to the value from the ADT directly, which to be correct we'd have
> to dynamically transplant to the DT, since there's at least one device
> that has different PCIe devices on one port depending on hardware
> variant, while sharing a devicetree. If we're okay with the kernel just
> not implementing this feature for now, we can say it's the bootloader's job.
>
> Ultimately we ship the DTs along with m1n1, so there's an argument that
> if some day we need to override the max-link-speed for whatever reason
> over what the ADT says, well, we'd be shipping the updated DT along with
> m1n1 anyway, so we might as well make m1n1 do it... if so, it might make
> sense to drop those properties from the actual DTs we ship altogether,
> at least for now.
>
> If we decide to make it m1n1's job entirely, we can drop this patch
> altogether, at least for now (I can't say how this will interact with
> suspend/resume and other power management, and hotplug... but we'll open
> that can of worms when we get there).

Shouldn't you be setting PCI_EXP_LNKCAP_SLS and/or PCI_EXP_LNKCAP2 if
you need to limit the max speed and then you can use that instead of
max-link-speed? If that's lost in low power modes, the driver just has
to save and restore it.

All this link handling is also something we want to make common as
much as possible. There's a lot of drivers poking these registers in
different ways and it's not clear whether they really need to be
different. So the less you do in the kernel the better.

Rob
Hector Martin Dec. 3, 2021, 1:47 p.m. UTC | #6
On 02/12/2021 23.33, Rob Herring wrote:
>>>>>> +       max_gen = of_pci_get_max_link_speed(port->np);
>>>>>> +       if (max_gen < 0) {
>>>>>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
>>>>>
>>>>> Better to fail than limp along in gen1? Though you don't check the
>>>>> return value...
>>>>>
>>>>> Usually, the DT property is there to limit the speed when there's a
>>>>> board limitation.
>>>>
>>>> The default *setting* is actually Gen4, but without
>>>> PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make
>>>> more sense to only set the LNKCTL field if max-link-speed is specified,
>>>> and unconditionally poke that bit. That'll get us Gen4 by default (or
>>>> even presumably Gen5 in future controllers, if everything else stays
>>>> compatible).
>>>
>>> You already do some setup in firmware for ECAM, right? I think it
>>> would be better if you can do any default setup there and then
>>> max-link-speed is only an override for the kernel.
>>
>> I thought the PCIE_LINK_WIDTH_SPEED_CONTROL thing had to be set later,
>> but trying it now I realized we were missing a bit of initialization
>> that was causing it not to work. Indeed it can be done there and we can
>> drop it from the kernel.
>>
>> We could even do the max-link-speed thing in m1n1 if we want. It has
>> access to the value from the ADT directly, which to be correct we'd have
>> to dynamically transplant to the DT, since there's at least one device
>> that has different PCIe devices on one port depending on hardware
>> variant, while sharing a devicetree. If we're okay with the kernel just
>> not implementing this feature for now, we can say it's the bootloader's job.
>>
>> Ultimately we ship the DTs along with m1n1, so there's an argument that
>> if some day we need to override the max-link-speed for whatever reason
>> over what the ADT says, well, we'd be shipping the updated DT along with
>> m1n1 anyway, so we might as well make m1n1 do it... if so, it might make
>> sense to drop those properties from the actual DTs we ship altogether,
>> at least for now.
>>
>> If we decide to make it m1n1's job entirely, we can drop this patch
>> altogether, at least for now (I can't say how this will interact with
>> suspend/resume and other power management, and hotplug... but we'll open
>> that can of worms when we get there).
> 
> Shouldn't you be setting PCI_EXP_LNKCAP_SLS and/or PCI_EXP_LNKCAP2 if
> you need to limit the max speed and then you can use that instead of
> max-link-speed? If that's lost in low power modes, the driver just has
> to save and restore it.

Those registers aren't writable as far as I can tell. All we can do is 
set LNKCTL2 to tell the hardware what actual max speed to use, the same 
thing this patch does.
Rob Herring (Arm) Dec. 3, 2021, 2:21 p.m. UTC | #7
On Fri, Dec 3, 2021 at 7:47 AM Hector Martin <marcan@marcan.st> wrote:
>
> On 02/12/2021 23.33, Rob Herring wrote:
> >>>>>> +       max_gen = of_pci_get_max_link_speed(port->np);
> >>>>>> +       if (max_gen < 0) {
> >>>>>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
> >>>>>
> >>>>> Better to fail than limp along in gen1? Though you don't check the
> >>>>> return value...
> >>>>>
> >>>>> Usually, the DT property is there to limit the speed when there's a
> >>>>> board limitation.
> >>>>
> >>>> The default *setting* is actually Gen4, but without
> >>>> PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make
> >>>> more sense to only set the LNKCTL field if max-link-speed is specified,
> >>>> and unconditionally poke that bit. That'll get us Gen4 by default (or
> >>>> even presumably Gen5 in future controllers, if everything else stays
> >>>> compatible).
> >>>
> >>> You already do some setup in firmware for ECAM, right? I think it
> >>> would be better if you can do any default setup there and then
> >>> max-link-speed is only an override for the kernel.
> >>
> >> I thought the PCIE_LINK_WIDTH_SPEED_CONTROL thing had to be set later,
> >> but trying it now I realized we were missing a bit of initialization
> >> that was causing it not to work. Indeed it can be done there and we can
> >> drop it from the kernel.
> >>
> >> We could even do the max-link-speed thing in m1n1 if we want. It has
> >> access to the value from the ADT directly, which to be correct we'd have
> >> to dynamically transplant to the DT, since there's at least one device
> >> that has different PCIe devices on one port depending on hardware
> >> variant, while sharing a devicetree. If we're okay with the kernel just
> >> not implementing this feature for now, we can say it's the bootloader's job.
> >>
> >> Ultimately we ship the DTs along with m1n1, so there's an argument that
> >> if some day we need to override the max-link-speed for whatever reason
> >> over what the ADT says, well, we'd be shipping the updated DT along with
> >> m1n1 anyway, so we might as well make m1n1 do it... if so, it might make
> >> sense to drop those properties from the actual DTs we ship altogether,
> >> at least for now.
> >>
> >> If we decide to make it m1n1's job entirely, we can drop this patch
> >> altogether, at least for now (I can't say how this will interact with
> >> suspend/resume and other power management, and hotplug... but we'll open
> >> that can of worms when we get there).
> >
> > Shouldn't you be setting PCI_EXP_LNKCAP_SLS and/or PCI_EXP_LNKCAP2 if
> > you need to limit the max speed and then you can use that instead of
> > max-link-speed? If that's lost in low power modes, the driver just has
> > to save and restore it.
>
> Those registers aren't writable as far as I can tell. All we can do is
> set LNKCTL2 to tell the hardware what actual max speed to use, the same
> thing this patch does.

I believe they are if you set the PCIE_DBI_RO_WR_EN bit. Multiple DWC
drivers write PCI_EXP_LNKCAP.

Rob
Hector Martin Dec. 3, 2021, 2:27 p.m. UTC | #8
On 03/12/2021 23.21, Rob Herring wrote:
> On Fri, Dec 3, 2021 at 7:47 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> On 02/12/2021 23.33, Rob Herring wrote:
>>>>>>>> +       max_gen = of_pci_get_max_link_speed(port->np);
>>>>>>>> +       if (max_gen < 0) {
>>>>>>>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
>>>>>>>
>>>>>>> Better to fail than limp along in gen1? Though you don't check the
>>>>>>> return value...
>>>>>>>
>>>>>>> Usually, the DT property is there to limit the speed when there's a
>>>>>>> board limitation.
>>>>>>
>>>>>> The default *setting* is actually Gen4, but without
>>>>>> PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make
>>>>>> more sense to only set the LNKCTL field if max-link-speed is specified,
>>>>>> and unconditionally poke that bit. That'll get us Gen4 by default (or
>>>>>> even presumably Gen5 in future controllers, if everything else stays
>>>>>> compatible).
>>>>>
>>>>> You already do some setup in firmware for ECAM, right? I think it
>>>>> would be better if you can do any default setup there and then
>>>>> max-link-speed is only an override for the kernel.
>>>>
>>>> I thought the PCIE_LINK_WIDTH_SPEED_CONTROL thing had to be set later,
>>>> but trying it now I realized we were missing a bit of initialization
>>>> that was causing it not to work. Indeed it can be done there and we can
>>>> drop it from the kernel.
>>>>
>>>> We could even do the max-link-speed thing in m1n1 if we want. It has
>>>> access to the value from the ADT directly, which to be correct we'd have
>>>> to dynamically transplant to the DT, since there's at least one device
>>>> that has different PCIe devices on one port depending on hardware
>>>> variant, while sharing a devicetree. If we're okay with the kernel just
>>>> not implementing this feature for now, we can say it's the bootloader's job.
>>>>
>>>> Ultimately we ship the DTs along with m1n1, so there's an argument that
>>>> if some day we need to override the max-link-speed for whatever reason
>>>> over what the ADT says, well, we'd be shipping the updated DT along with
>>>> m1n1 anyway, so we might as well make m1n1 do it... if so, it might make
>>>> sense to drop those properties from the actual DTs we ship altogether,
>>>> at least for now.
>>>>
>>>> If we decide to make it m1n1's job entirely, we can drop this patch
>>>> altogether, at least for now (I can't say how this will interact with
>>>> suspend/resume and other power management, and hotplug... but we'll open
>>>> that can of worms when we get there).
>>>
>>> Shouldn't you be setting PCI_EXP_LNKCAP_SLS and/or PCI_EXP_LNKCAP2 if
>>> you need to limit the max speed and then you can use that instead of
>>> max-link-speed? If that's lost in low power modes, the driver just has
>>> to save and restore it.
>>
>> Those registers aren't writable as far as I can tell. All we can do is
>> set LNKCTL2 to tell the hardware what actual max speed to use, the same
>> thing this patch does.
> 
> I believe they are if you set the PCIE_DBI_RO_WR_EN bit. Multiple DWC
> drivers write PCI_EXP_LNKCAP.

Oh, indeed, that works. I figured that DBI stuff was only for the higher 
registers, not the core PCIe ones. Sounds like a plan then! We might 
still want to set LNKCTL2 though, since there's no point in that 
specifying a higher speed than what we are now claiming the hardware 
supports.

We can drop this patch then, and probably the max-link-speed props in 
the DTs altogether. More stuff in m1n1 is good, makes it easier to keep 
older kernel support with new hardware :)
Hector Martin Dec. 7, 2021, 5:07 a.m. UTC | #9
On 03/12/2021 23.27, Hector Martin wrote:
> On 03/12/2021 23.21, Rob Herring wrote:
>> On Fri, Dec 3, 2021 at 7:47 AM Hector Martin <marcan@marcan.st> wrote:
>>>
>>> On 02/12/2021 23.33, Rob Herring wrote:
>>>>>>>>> +       max_gen = of_pci_get_max_link_speed(port->np);
>>>>>>>>> +       if (max_gen < 0) {
>>>>>>>>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
>>>>>>>>
>>>>>>>> Better to fail than limp along in gen1? Though you don't check the
>>>>>>>> return value...
>>>>>>>>
>>>>>>>> Usually, the DT property is there to limit the speed when there's a
>>>>>>>> board limitation.
>>>>>>>
>>>>>>> The default *setting* is actually Gen4, but without
>>>>>>> PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make
>>>>>>> more sense to only set the LNKCTL field if max-link-speed is specified,
>>>>>>> and unconditionally poke that bit. That'll get us Gen4 by default (or
>>>>>>> even presumably Gen5 in future controllers, if everything else stays
>>>>>>> compatible).
>>>>>>
>>>>>> You already do some setup in firmware for ECAM, right? I think it
>>>>>> would be better if you can do any default setup there and then
>>>>>> max-link-speed is only an override for the kernel.
>>>>>
>>>>> I thought the PCIE_LINK_WIDTH_SPEED_CONTROL thing had to be set later,
>>>>> but trying it now I realized we were missing a bit of initialization
>>>>> that was causing it not to work. Indeed it can be done there and we can
>>>>> drop it from the kernel.
>>>>>
>>>>> We could even do the max-link-speed thing in m1n1 if we want. It has
>>>>> access to the value from the ADT directly, which to be correct we'd have
>>>>> to dynamically transplant to the DT, since there's at least one device
>>>>> that has different PCIe devices on one port depending on hardware
>>>>> variant, while sharing a devicetree. If we're okay with the kernel just
>>>>> not implementing this feature for now, we can say it's the bootloader's job.
>>>>>
>>>>> Ultimately we ship the DTs along with m1n1, so there's an argument that
>>>>> if some day we need to override the max-link-speed for whatever reason
>>>>> over what the ADT says, well, we'd be shipping the updated DT along with
>>>>> m1n1 anyway, so we might as well make m1n1 do it... if so, it might make
>>>>> sense to drop those properties from the actual DTs we ship altogether,
>>>>> at least for now.
>>>>>
>>>>> If we decide to make it m1n1's job entirely, we can drop this patch
>>>>> altogether, at least for now (I can't say how this will interact with
>>>>> suspend/resume and other power management, and hotplug... but we'll open
>>>>> that can of worms when we get there).
>>>>
>>>> Shouldn't you be setting PCI_EXP_LNKCAP_SLS and/or PCI_EXP_LNKCAP2 if
>>>> you need to limit the max speed and then you can use that instead of
>>>> max-link-speed? If that's lost in low power modes, the driver just has
>>>> to save and restore it.
>>>
>>> Those registers aren't writable as far as I can tell. All we can do is
>>> set LNKCTL2 to tell the hardware what actual max speed to use, the same
>>> thing this patch does.
>>
>> I believe they are if you set the PCIE_DBI_RO_WR_EN bit. Multiple DWC
>> drivers write PCI_EXP_LNKCAP.
> 
> Oh, indeed, that works. I figured that DBI stuff was only for the higher
> registers, not the core PCIe ones. Sounds like a plan then! We might
> still want to set LNKCTL2 though, since there's no point in that
> specifying a higher speed than what we are now claiming the hardware
> supports.
> 
> We can drop this patch then, and probably the max-link-speed props in
> the DTs altogether. More stuff in m1n1 is good, makes it easier to keep
> older kernel support with new hardware :)

This is now done in m1n1 (abee2ec).

Interestingly, the ADT downgrades the link to the SD card reader to 
Gen1, even though both sides can do Gen2. I wonder why that is the case. 
Perhaps they didn't need the extra speed and Gen1 saves power?
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index 03bfe977c579..073cbac49d8b 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -30,6 +30,10 @@ 
 #include <linux/of_irq.h>
 #include <linux/pci-ecam.h>
 
+#include "../pci.h"
+/* Apple PCIe is based on DesignWare IP and shares some registers */
+#include "dwc/pcie-designware.h"
+
 #define CORE_RC_PHYIF_CTL		0x00024
 #define   CORE_RC_PHYIF_CTL_RUN		BIT(0)
 #define CORE_RC_PHYIF_STAT		0x00028
@@ -130,9 +134,13 @@ 
  */
 #define DOORBELL_ADDR		CONFIG_PCIE_APPLE_MSI_DOORBELL_ADDR
 
+/* The offset of the PCIe capabilities structure in bridge config space */
+#define PCIE_CAP_BASE		0x70
+
 struct apple_pcie {
 	struct mutex		lock;
 	struct device		*dev;
+	struct pci_config_window *cfg;
 	void __iomem            *base;
 	struct irq_domain	*domain;
 	unsigned long		*bitmap;
@@ -506,6 +514,48 @@  static u32 apple_pcie_rid2sid_write(struct apple_pcie_port *port,
 	return readl_relaxed(port->base + PORT_RID2SID(idx));
 }
 
+static inline void __iomem *bridge_reg(struct apple_pcie_port *port,
+						  int where)
+{
+	struct pci_config_window *cfg = port->pcie->cfg;
+
+	return cfg->win + PCIE_ECAM_OFFSET(0, PCI_DEVFN(port->idx, 0), where);
+}
+
+static void apple_pcie_unlock_dwc_regs(struct apple_pcie_port *port)
+{
+	rmw_set(PCIE_DBI_RO_WR_EN, bridge_reg(port, PCIE_MISC_CONTROL_1_OFF));
+}
+
+static void apple_pcie_lock_dwc_regs(struct apple_pcie_port *port)
+{
+	rmw_clear(PCIE_DBI_RO_WR_EN, bridge_reg(port, PCIE_MISC_CONTROL_1_OFF));
+}
+
+static int apple_pcie_link_configure_max_speed(struct apple_pcie_port *port)
+{
+	int max_gen;
+	u32 ctrl2;
+
+	max_gen = of_pci_get_max_link_speed(port->np);
+	if (max_gen < 0) {
+		dev_err(port->pcie->dev, "max link speed not specified\n");
+		return max_gen;
+	}
+
+	ctrl2 = readw_relaxed(bridge_reg(port, PCIE_CAP_BASE + PCI_EXP_LNKCTL2));
+	ctrl2 &= ~PCI_EXP_LNKCTL2_TLS;
+	ctrl2 |= FIELD_PREP(PCI_EXP_LNKCTL2_TLS, max_gen);
+	writew_relaxed(ctrl2, bridge_reg(port, PCIE_CAP_BASE + PCI_EXP_LNKCTL2));
+
+	apple_pcie_unlock_dwc_regs(port);
+	rmw_set(PORT_LOGIC_SPEED_CHANGE,
+		bridge_reg(port, PCIE_LINK_WIDTH_SPEED_CONTROL));
+	apple_pcie_lock_dwc_regs(port);
+
+	return 0;
+}
+
 static int apple_pcie_setup_port(struct apple_pcie *pcie,
 				 struct device_node *np)
 {
@@ -577,6 +627,8 @@  static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	ret = apple_pcie_port_register_irqs(port);
 	WARN_ON(ret);
 
+	apple_pcie_link_configure_max_speed(port);
+
 	writel_relaxed(PORT_LTSSMCTL_START, port->base + PORT_LTSSMCTL);
 
 	if (!wait_for_completion_timeout(&pcie->event, HZ / 10))
@@ -762,6 +814,7 @@  static int apple_pcie_init(struct pci_config_window *cfg)
 		return -ENOMEM;
 
 	pcie->dev = dev;
+	pcie->cfg = cfg;
 
 	mutex_init(&pcie->lock);