mbox series

[v4,00/12] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

Message ID 20200430080625.26070-1-pali@kernel.org
Headers show
Series PCI: aardvark: Fix support for Turris MOX and Compex wifi cards | expand

Message

Pali Rohár April 30, 2020, 8:06 a.m. UTC
Hello,

this is the fourth version of the patch series for Armada 3720 PCIe
controller (aardvark). It's main purpose is to fix some bugs regarding
buggy ath10k cards, but we also found out some suspicious stuff about
the driver and the SOC itself, which we try to address.

Patches are available also in my git branch pci-aardvark:
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark

Changes since v3:
- do not change return value of of_pci_get_max_link_speed() function
- mark zero 'max-link-speed' as invalid
- silently use gen3 speed when 'max-link-speed' as invalid

Changes since v2:
- move PCIe max-link-speed property to armada-37xx.dtsi
- replace custom macros by standard linux/pci_regs.h macros
- increase PERST delay to 10ms (needed for initialized Compex WLE900VX)
- disable link training before PERST (needed for Compex WLE900VX)
- change of_pci_get_max_link_speed() function to signal -ENOENT
- handle errors from of_pci_get_max_link_speed() function
- updated comments, commit titles and messages

Changes since v1:
- commit titles and messages were reviewed and some of them were rewritten
- patches 1 and 5 from v1 which touch PCIe speed configuration were
  reworked into one patch
- patch 2 from v1 was removed, it is not needed anymore
- patch 7 from v1 now touches the device tree of armada-3720-db
- a patch was added that tries to enable PCIe PHY via generic-phy API
  (if a phandle to the PHY is found in the device tree)
- a patch describing the new PCIe node DT properties was added
- a patch was added that moves the PHY phandle from board device trees
  to armada-37xx.dtsi

Marek and Pali

Marek Behún (5):
  PCI: aardvark: Improve link training
  PCI: aardvark: Add PHY support
  dt-bindings: PCI: aardvark: Describe new properties
  arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
  arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property

Pali Rohár (7):
  PCI: aardvark: Train link immediately after enabling training
  PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
    read-only register
  PCI: of: Zero max-link-speed value is invalid
  PCI: aardvark: Issue PERST via GPIO
  PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
  PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
    macros
  arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property

 .../devicetree/bindings/pci/aardvark-pci.txt  |   4 +
 .../arm64/boot/dts/marvell/armada-3720-db.dts |   3 +
 .../dts/marvell/armada-3720-espressobin.dtsi  |   2 +-
 .../dts/marvell/armada-3720-turris-mox.dts    |   6 -
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |   4 +-
 drivers/pci/controller/pci-aardvark.c         | 263 +++++++++++++++---
 drivers/pci/of.c                              |   2 +-
 7 files changed, 231 insertions(+), 53 deletions(-)

Comments

Tomasz Maciej Nowak May 8, 2020, 1:11 p.m. UTC | #1
W dniu 30.04.2020 o 10:06, Pali Rohár pisze:
> Hello,
> 
> this is the fourth version of the patch series for Armada 3720 PCIe
> controller (aardvark). It's main purpose is to fix some bugs regarding
> buggy ath10k cards, but we also found out some suspicious stuff about
> the driver and the SOC itself, which we try to address.
> 
> Patches are available also in my git branch pci-aardvark:
> https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
> 
> Changes since v3:
> - do not change return value of of_pci_get_max_link_speed() function
> - mark zero 'max-link-speed' as invalid
> - silently use gen3 speed when 'max-link-speed' as invalid
> 
> Changes since v2:
> - move PCIe max-link-speed property to armada-37xx.dtsi
> - replace custom macros by standard linux/pci_regs.h macros
> - increase PERST delay to 10ms (needed for initialized Compex WLE900VX)
> - disable link training before PERST (needed for Compex WLE900VX)
> - change of_pci_get_max_link_speed() function to signal -ENOENT
> - handle errors from of_pci_get_max_link_speed() function
> - updated comments, commit titles and messages
> 
> Changes since v1:
> - commit titles and messages were reviewed and some of them were rewritten
> - patches 1 and 5 from v1 which touch PCIe speed configuration were
>   reworked into one patch
> - patch 2 from v1 was removed, it is not needed anymore
> - patch 7 from v1 now touches the device tree of armada-3720-db
> - a patch was added that tries to enable PCIe PHY via generic-phy API
>   (if a phandle to the PHY is found in the device tree)
> - a patch describing the new PCIe node DT properties was added
> - a patch was added that moves the PHY phandle from board device trees
>   to armada-37xx.dtsi
> 
> Marek and Pali
> 
> Marek Behún (5):
>   PCI: aardvark: Improve link training
>   PCI: aardvark: Add PHY support
>   dt-bindings: PCI: aardvark: Describe new properties
>   arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
>   arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> 
> Pali Rohár (7):
>   PCI: aardvark: Train link immediately after enabling training
>   PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
>     read-only register
>   PCI: of: Zero max-link-speed value is invalid
>   PCI: aardvark: Issue PERST via GPIO
>   PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
>   PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
>     macros
>   arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property

Hi.
The PCI interface seems to work fine as in the first series, so

Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Pali Rohár May 13, 2020, 11:16 a.m. UTC | #2
On Thursday 30 April 2020 10:06:13 Pali Rohár wrote:
> Hello,
> 
> this is the fourth version of the patch series for Armada 3720 PCIe
> controller (aardvark). It's main purpose is to fix some bugs regarding
> buggy ath10k cards, but we also found out some suspicious stuff about
> the driver and the SOC itself, which we try to address.
> 
> Patches are available also in my git branch pci-aardvark:
> https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark

Hello! Thanks everybody for review and testing of this patch series.

I would like to ask, is there something needed to fix / modify in this
patch series? If everything is OK, would you Bjorn or Lorenzo take this
patch series into your tree?
Lorenzo Pieralisi May 13, 2020, 11:33 a.m. UTC | #3
On Wed, May 13, 2020 at 01:16:51PM +0200, Pali Rohár wrote:
> On Thursday 30 April 2020 10:06:13 Pali Rohár wrote:
> > Hello,
> > 
> > this is the fourth version of the patch series for Armada 3720 PCIe
> > controller (aardvark). It's main purpose is to fix some bugs regarding
> > buggy ath10k cards, but we also found out some suspicious stuff about
> > the driver and the SOC itself, which we try to address.
> > 
> > Patches are available also in my git branch pci-aardvark:
> > https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
> 
> Hello! Thanks everybody for review and testing of this patch series.
> 
> I would like to ask, is there something needed to fix / modify in this
> patch series? If everything is OK, would you Bjorn or Lorenzo take this
> patch series into your tree?

We need Thomas' ACK on the series. We don't have this HW and
we comment on the generic code, Thomas owns it and must check that
what you are changing is sound.

On patch 5 I share Rob's concerns - it does not make much sense
to have something driver specific there, need to look further.

Lorenzo
Thomas Petazzoni May 13, 2020, 11:56 a.m. UTC | #4
Hello,

On Thu, 30 Apr 2020 10:06:13 +0200
Pali Rohár <pali@kernel.org> wrote:

> Marek Behún (5):
>   PCI: aardvark: Improve link training
>   PCI: aardvark: Add PHY support
>   dt-bindings: PCI: aardvark: Describe new properties
>   arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
>   arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> 
> Pali Rohár (7):
>   PCI: aardvark: Train link immediately after enabling training
>   PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
>     read-only register
>   PCI: of: Zero max-link-speed value is invalid
>   PCI: aardvark: Issue PERST via GPIO
>   PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
>   PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
>     macros
>   arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property

Thanks a lot for this work. For a number of reasons, I'm less involved
in Marvell platform support in Linux, but I reviewed your series and
followed the discussions around it, and I'm happy to give my:

Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

for the whole series. The changes all seem sensible, and have been
tested by several folks.

Thanks!

Thomas
Pali Rohár May 13, 2020, 11:59 a.m. UTC | #5
On Wednesday 13 May 2020 12:33:14 Lorenzo Pieralisi wrote:
> On Wed, May 13, 2020 at 01:16:51PM +0200, Pali Rohár wrote:
> > On Thursday 30 April 2020 10:06:13 Pali Rohár wrote:
> > > Hello,
> > > 
> > > this is the fourth version of the patch series for Armada 3720 PCIe
> > > controller (aardvark). It's main purpose is to fix some bugs regarding
> > > buggy ath10k cards, but we also found out some suspicious stuff about
> > > the driver and the SOC itself, which we try to address.
> > > 
> > > Patches are available also in my git branch pci-aardvark:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark
> > 
> > Hello! Thanks everybody for review and testing of this patch series.
> > 
> > I would like to ask, is there something needed to fix / modify in this
> > patch series? If everything is OK, would you Bjorn or Lorenzo take this
> > patch series into your tree?
> 
> We need Thomas' ACK on the series. We don't have this HW and
> we comment on the generic code, Thomas owns it and must check that
> what you are changing is sound.

Ok, we will wait for Thomas ACK/review.

> On patch 5 I share Rob's concerns - it does not make much sense
> to have something driver specific there, need to look further.

I fully understand yours concerns. I wanted to solve it. Problem is that
I really do not know which timeout is there applicable. I read
information about PERST# more times but I was not able to clearly deduce
that minimal timeout/delay needed for this reset scenario.

So what I was able to do are just experiments. I found out what is the
minimal needed time to correctly initialize wifi cars which I used for
testing.

You can look into my previous email [1] where I wrote which timeouts are
used by which drivers. Basically every driver is using its own custom
timeout and this is something which should be fixed / improved. In my
opinion authors tested their own (wifi) cards and measured minimal
timeout needed for initializing them.

So somebody with deeper PCI knowledge should look at this PERST# problem
and try to address it.

After it happens I see there two scenarios:

1) Timeout according to specification/authority is lower than what we
currently use. In this case it would mean that we have buggy wifi cards
(and we already know that people reported issues with Compex cards) and
we would have to stay with higher timeout. Probably we can define common
macro with timeout value and use it.

2) Timeout according to specification/authority is bigger then what we
currently use. In this case there is no problem to increase it, card
would be just longer in reset state. What could be problematic for
somebody is that this increase boot / initialization time.

[1] - https://lore.kernel.org/linux-pci/20200424092546.25p3hdtkehohe3xw@pali/
Gregory CLEMENT May 17, 2020, 3:57 p.m. UTC | #6
Hello,

> Hello,
>
> On Thu, 30 Apr 2020 10:06:13 +0200
> Pali Rohár <pali@kernel.org> wrote:
>
>> Marek Behún (5):
>>   PCI: aardvark: Improve link training
>>   PCI: aardvark: Add PHY support
>>   dt-bindings: PCI: aardvark: Describe new properties
>>   arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
>>   arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
>> 
>> Pali Rohár (7):
>>   PCI: aardvark: Train link immediately after enabling training
>>   PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
>>     read-only register
>>   PCI: of: Zero max-link-speed value is invalid
>>   PCI: aardvark: Issue PERST via GPIO
>>   PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
>>   PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
>>     macros
>>   arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
>
> Thanks a lot for this work. For a number of reasons, I'm less involved
> in Marvell platform support in Linux, but I reviewed your series and
> followed the discussions around it, and I'm happy to give my:
>
> Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

With this acked-by for the series, the reviewed-by from Rob on the
binding and the tested-by, I am pretty confident so I applied the
patches 10, 11 and 12 on mvebu/dt64.

Thanks,

Gregory


>
> for the whole series. The changes all seem sensible, and have been
> tested by several folks.
>
> Thanks!
>
> Thomas
> -- 
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Pali Rohár May 18, 2020, 10:30 a.m. UTC | #7
On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:
> Hello,
> 
> > Hello,
> >
> > On Thu, 30 Apr 2020 10:06:13 +0200
> > Pali Rohár <pali@kernel.org> wrote:
> >
> >> Marek Behún (5):
> >>   PCI: aardvark: Improve link training
> >>   PCI: aardvark: Add PHY support
> >>   dt-bindings: PCI: aardvark: Describe new properties
> >>   arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> >>   arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> >> 
> >> Pali Rohár (7):
> >>   PCI: aardvark: Train link immediately after enabling training
> >>   PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> >>     read-only register
> >>   PCI: of: Zero max-link-speed value is invalid
> >>   PCI: aardvark: Issue PERST via GPIO
> >>   PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> >>   PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> >>     macros
> >>   arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
> >
> > Thanks a lot for this work. For a number of reasons, I'm less involved
> > in Marvell platform support in Linux, but I reviewed your series and
> > followed the discussions around it, and I'm happy to give my:
> >
> > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> With this acked-by for the series, the reviewed-by from Rob on the
> binding and the tested-by, I am pretty confident so I applied the
> patches 10, 11 and 12 on mvebu/dt64.
> 
> Thanks,
> 
> Gregory

Thank you!

Lorenzo, would you now take remaining patches?

> 
> >
> > for the whole series. The changes all seem sensible, and have been
> > tested by several folks.
> >
> > Thanks!
> >
> > Thomas
> > -- 
> > Thomas Petazzoni, CTO, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> 
> -- 
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com
Lorenzo Pieralisi May 18, 2020, 1:46 p.m. UTC | #8
On Mon, May 18, 2020 at 12:30:04PM +0200, Pali Rohár wrote:
> On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:
> > Hello,
> > 
> > > Hello,
> > >
> > > On Thu, 30 Apr 2020 10:06:13 +0200
> > > Pali Rohár <pali@kernel.org> wrote:
> > >
> > >> Marek Behún (5):
> > >>   PCI: aardvark: Improve link training
> > >>   PCI: aardvark: Add PHY support
> > >>   dt-bindings: PCI: aardvark: Describe new properties
> > >>   arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> > >>   arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> > >> 
> > >> Pali Rohár (7):
> > >>   PCI: aardvark: Train link immediately after enabling training
> > >>   PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> > >>     read-only register
> > >>   PCI: of: Zero max-link-speed value is invalid
> > >>   PCI: aardvark: Issue PERST via GPIO
> > >>   PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> > >>   PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> > >>     macros
> > >>   arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property
> > >
> > > Thanks a lot for this work. For a number of reasons, I'm less involved
> > > in Marvell platform support in Linux, but I reviewed your series and
> > > followed the discussions around it, and I'm happy to give my:
> > >
> > > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > 
> > With this acked-by for the series, the reviewed-by from Rob on the
> > binding and the tested-by, I am pretty confident so I applied the
> > patches 10, 11 and 12 on mvebu/dt64.
> > 
> > Thanks,
> > 
> > Gregory
> 
> Thank you!
> 
> Lorenzo, would you now take remaining patches?

Yes - even though I have reservations about patch (5) and the
problem is related to a complete lack of programming model for
these host controllers and a clear separation between what's
done in the OS vs bootloader, PERST handling in this host
bridge is *really* a mess.

I applied 1-9 to pci/aardvark.

Lorenzo
Marek Behun May 18, 2020, 1:50 p.m. UTC | #9
On Mon, 18 May 2020 14:46:14 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> On Mon, May 18, 2020 at 12:30:04PM +0200, Pali Rohár wrote:
> > On Sunday 17 May 2020 17:57:02 Gregory CLEMENT wrote:  
> > > Hello,
> > >   
> > > > Hello,
> > > >
> > > > On Thu, 30 Apr 2020 10:06:13 +0200
> > > > Pali Rohár <pali@kernel.org> wrote:
> > > >  
> > > >> Marek Behún (5):
> > > >>   PCI: aardvark: Improve link training
> > > >>   PCI: aardvark: Add PHY support
> > > >>   dt-bindings: PCI: aardvark: Describe new properties
> > > >>   arm64: dts: marvell: armada-37xx: Set pcie_reset_pin to gpio function
> > > >>   arm64: dts: marvell: armada-37xx: Move PCIe comphy handle property
> > > >> 
> > > >> Pali Rohár (7):
> > > >>   PCI: aardvark: Train link immediately after enabling training
> > > >>   PCI: aardvark: Don't blindly enable ASPM L0s and don't write to
> > > >>     read-only register
> > > >>   PCI: of: Zero max-link-speed value is invalid
> > > >>   PCI: aardvark: Issue PERST via GPIO
> > > >>   PCI: aardvark: Add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> > > >>   PCI: aardvark: Replace custom macros by standard linux/pci_regs.h
> > > >>     macros
> > > >>   arm64: dts: marvell: armada-37xx: Move PCIe max-link-speed property  
> > > >
> > > > Thanks a lot for this work. For a number of reasons, I'm less involved
> > > > in Marvell platform support in Linux, but I reviewed your series and
> > > > followed the discussions around it, and I'm happy to give my:
> > > >
> > > > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>  
> > > 
> > > With this acked-by for the series, the reviewed-by from Rob on the
> > > binding and the tested-by, I am pretty confident so I applied the
> > > patches 10, 11 and 12 on mvebu/dt64.
> > > 
> > > Thanks,
> > > 
> > > Gregory  
> > 
> > Thank you!
> > 
> > Lorenzo, would you now take remaining patches?  
> 
> Yes - even though I have reservations about patch (5) and the
> problem is related to a complete lack of programming model for
> these host controllers and a clear separation between what's
> done in the OS vs bootloader, PERST handling in this host
> bridge is *really* a mess.
> 
> I applied 1-9 to pci/aardvark.
> 
> Lorenzo

Hooray, thanks, Lorenzo (and everyone else).