mbox series

[v2,00/40] PCI: dwc: Driver clean-ups

Message ID 20200821035420.380495-1-robh@kernel.org (mailing list archive)
Headers show
Series PCI: dwc: Driver clean-ups | expand

Message

Rob Herring (Arm) Aug. 21, 2020, 3:53 a.m. UTC
This is a series of clean-ups for the Designware PCI driver. The series
initially reworks the config space accessors to use the existing pci_ops
struct. Then there's removal of various private data that's also present
in the pci_host_bridge struct. There's also some duplicated common (PCI
and DWC) register defines which I converted to use the common defines.
Finally, the initialization for speed/gen, number of lanes, and N_FTS
are all moved to the common DWC code.

This is compile tested only as I don't have any DWC based h/w, so any
testing would be helpful. A branch is here[1].

Rob

[1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git pci-dw-cleanups

Rob Herring (40):
  PCI: Allow root and child buses to have different pci_ops
  PCI: dwc: Use DBI accessors instead of own config accessors
  PCI: dwc: Allow overriding bridge pci_ops
  PCI: dwc: Add a default pci_ops.map_bus for root port
  PCI: dwc: al: Use pci_ops for child config space accessors
  PCI: dwc: keystone: Use pci_ops for config space accessors
  PCI: dwc: tegra: Use pci_ops for root config space accessors
  PCI: dwc: meson: Use pci_ops for root config space accessors
  PCI: dwc: kirin: Use pci_ops for root config space accessors
  PCI: dwc: exynos: Use pci_ops for root config space accessors
  PCI: dwc: histb: Use pci_ops for root config space accessors
  PCI: dwc: Remove dwc specific config accessor ops
  PCI: dwc: Use generic config accessors
  PCI: Also call .add_bus() callback for root bus
  PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus
  PCI: dwc: Convert to use pci_host_probe()
  PCI: dwc: Remove root_bus pointer
  PCI: dwc: Remove storing of PCI resources
  PCI: dwc: Simplify config space handling
  PCI: dwc/keystone: Drop duplicated 'num-viewport'
  PCI: dwc: Check CONFIG_PCI_MSI inside dw_pcie_msi_init()
  PCI: dwc/imx6: Remove duplicate define PCIE_LINK_WIDTH_SPEED_CONTROL
  PCI: dwc: Add a 'num_lanes' field to struct dw_pcie
  PCI: dwc: Ensure FAST_LINK_MODE is cleared
  PCI: dwc/meson: Drop the duplicate number of lanes setup
  PCI: dwc/meson: Drop unnecessary RC config space initialization
  PCI: dwc/meson: Rework PCI config and DW port logic register accesses
  PCI: dwc/imx6: Use common PCI register definitions
  PCI: dwc/qcom: Use common PCI register definitions
  PCI: dwc: Remove hardcoded PCI_CAP_ID_EXP offset
  PCI: dwc/tegra: Use common Designware port logic register definitions
  PCI: dwc: Remove read_dbi2 code
  PCI: dwc: Make ATU accessors private
  PCI: dwc: Centralize link gen setting
  PCI: dwc: Set PORT_LINK_DLL_LINK_EN in common setup code
  PCI: dwc/intel-gw: Drop unnecessary checking of DT 'device_type'
    property
  PCI: dwc/intel-gw: Move getting PCI_CAP_ID_EXP offset to
    intel_pcie_link_setup()
  PCI: dwc/intel-gw: Drop unused max_width
  PCI: dwc: Move N_FTS setup to common setup
  PCI: dwc: Use DBI accessors

 drivers/pci/controller/dwc/pci-dra7xx.c       |  29 +-
 drivers/pci/controller/dwc/pci-exynos.c       |  45 +--
 drivers/pci/controller/dwc/pci-imx6.c         |  52 +--
 drivers/pci/controller/dwc/pci-keystone.c     | 126 ++-----
 drivers/pci/controller/dwc/pci-meson.c        | 156 ++-------
 drivers/pci/controller/dwc/pcie-al.c          |  70 +---
 drivers/pci/controller/dwc/pcie-artpec6.c     |  48 +--
 .../pci/controller/dwc/pcie-designware-ep.c   |  11 +-
 .../pci/controller/dwc/pcie-designware-host.c | 319 ++++++------------
 .../pci/controller/dwc/pcie-designware-plat.c |   4 +-
 drivers/pci/controller/dwc/pcie-designware.c  | 104 +++---
 drivers/pci/controller/dwc/pcie-designware.h  |  54 +--
 drivers/pci/controller/dwc/pcie-histb.c       |  45 +--
 drivers/pci/controller/dwc/pcie-intel-gw.c    |  65 +---
 drivers/pci/controller/dwc/pcie-kirin.c       |  43 +--
 drivers/pci/controller/dwc/pcie-qcom.c        |  33 +-
 drivers/pci/controller/dwc/pcie-spear13xx.c   |  39 +--
 drivers/pci/controller/dwc/pcie-tegra194.c    | 120 ++-----
 drivers/pci/controller/dwc/pcie-uniphier.c    |   3 +-
 drivers/pci/probe.c                           |  14 +-
 include/linux/pci.h                           |   1 +
 21 files changed, 443 insertions(+), 938 deletions(-)

--
2.25.1

Comments

Lorenzo Pieralisi Sept. 7, 2020, 9:35 a.m. UTC | #1
On Thu, Aug 20, 2020 at 09:53:40PM -0600, Rob Herring wrote:
> This is a series of clean-ups for the Designware PCI driver. The series
> initially reworks the config space accessors to use the existing pci_ops
> struct. Then there's removal of various private data that's also present
> in the pci_host_bridge struct. There's also some duplicated common (PCI
> and DWC) register defines which I converted to use the common defines.
> Finally, the initialization for speed/gen, number of lanes, and N_FTS
> are all moved to the common DWC code.
> 
> This is compile tested only as I don't have any DWC based h/w, so any
> testing would be helpful. A branch is here[1].

Applied the series to pci/dwc, thanks.

Lorenzo

> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git pci-dw-cleanups
> 
> Rob Herring (40):
>   PCI: Allow root and child buses to have different pci_ops
>   PCI: dwc: Use DBI accessors instead of own config accessors
>   PCI: dwc: Allow overriding bridge pci_ops
>   PCI: dwc: Add a default pci_ops.map_bus for root port
>   PCI: dwc: al: Use pci_ops for child config space accessors
>   PCI: dwc: keystone: Use pci_ops for config space accessors
>   PCI: dwc: tegra: Use pci_ops for root config space accessors
>   PCI: dwc: meson: Use pci_ops for root config space accessors
>   PCI: dwc: kirin: Use pci_ops for root config space accessors
>   PCI: dwc: exynos: Use pci_ops for root config space accessors
>   PCI: dwc: histb: Use pci_ops for root config space accessors
>   PCI: dwc: Remove dwc specific config accessor ops
>   PCI: dwc: Use generic config accessors
>   PCI: Also call .add_bus() callback for root bus
>   PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus
>   PCI: dwc: Convert to use pci_host_probe()
>   PCI: dwc: Remove root_bus pointer
>   PCI: dwc: Remove storing of PCI resources
>   PCI: dwc: Simplify config space handling
>   PCI: dwc/keystone: Drop duplicated 'num-viewport'
>   PCI: dwc: Check CONFIG_PCI_MSI inside dw_pcie_msi_init()
>   PCI: dwc/imx6: Remove duplicate define PCIE_LINK_WIDTH_SPEED_CONTROL
>   PCI: dwc: Add a 'num_lanes' field to struct dw_pcie
>   PCI: dwc: Ensure FAST_LINK_MODE is cleared
>   PCI: dwc/meson: Drop the duplicate number of lanes setup
>   PCI: dwc/meson: Drop unnecessary RC config space initialization
>   PCI: dwc/meson: Rework PCI config and DW port logic register accesses
>   PCI: dwc/imx6: Use common PCI register definitions
>   PCI: dwc/qcom: Use common PCI register definitions
>   PCI: dwc: Remove hardcoded PCI_CAP_ID_EXP offset
>   PCI: dwc/tegra: Use common Designware port logic register definitions
>   PCI: dwc: Remove read_dbi2 code
>   PCI: dwc: Make ATU accessors private
>   PCI: dwc: Centralize link gen setting
>   PCI: dwc: Set PORT_LINK_DLL_LINK_EN in common setup code
>   PCI: dwc/intel-gw: Drop unnecessary checking of DT 'device_type'
>     property
>   PCI: dwc/intel-gw: Move getting PCI_CAP_ID_EXP offset to
>     intel_pcie_link_setup()
>   PCI: dwc/intel-gw: Drop unused max_width
>   PCI: dwc: Move N_FTS setup to common setup
>   PCI: dwc: Use DBI accessors
> 
>  drivers/pci/controller/dwc/pci-dra7xx.c       |  29 +-
>  drivers/pci/controller/dwc/pci-exynos.c       |  45 +--
>  drivers/pci/controller/dwc/pci-imx6.c         |  52 +--
>  drivers/pci/controller/dwc/pci-keystone.c     | 126 ++-----
>  drivers/pci/controller/dwc/pci-meson.c        | 156 ++-------
>  drivers/pci/controller/dwc/pcie-al.c          |  70 +---
>  drivers/pci/controller/dwc/pcie-artpec6.c     |  48 +--
>  .../pci/controller/dwc/pcie-designware-ep.c   |  11 +-
>  .../pci/controller/dwc/pcie-designware-host.c | 319 ++++++------------
>  .../pci/controller/dwc/pcie-designware-plat.c |   4 +-
>  drivers/pci/controller/dwc/pcie-designware.c  | 104 +++---
>  drivers/pci/controller/dwc/pcie-designware.h  |  54 +--
>  drivers/pci/controller/dwc/pcie-histb.c       |  45 +--
>  drivers/pci/controller/dwc/pcie-intel-gw.c    |  65 +---
>  drivers/pci/controller/dwc/pcie-kirin.c       |  43 +--
>  drivers/pci/controller/dwc/pcie-qcom.c        |  33 +-
>  drivers/pci/controller/dwc/pcie-spear13xx.c   |  39 +--
>  drivers/pci/controller/dwc/pcie-tegra194.c    | 120 ++-----
>  drivers/pci/controller/dwc/pcie-uniphier.c    |   3 +-
>  drivers/pci/probe.c                           |  14 +-
>  include/linux/pci.h                           |   1 +
>  21 files changed, 443 insertions(+), 938 deletions(-)
> 
> --
> 2.25.1
Michael Walle Sept. 15, 2020, 9:12 a.m. UTC | #2
Hi Rob,

> This is a series of clean-ups for the Designware PCI driver. The series
> initially reworks the config space accessors to use the existing pci_ops
> struct. Then there's removal of various private data that's also present
> in the pci_host_bridge struct. There's also some duplicated common (PCI
> and DWC) register defines which I converted to use the common defines.
> Finally, the initialization for speed/gen, number of lanes, and N_FTS
> are all moved to the common DWC code.

> This is compile tested only as I don't have any DWC based h/w, so any
> testing would be helpful. A branch is here[1].

I've noticed that with the latest linux-next, my board doesn't boot
anymore. I've traced it back to this series. There is a similar
board in kernelci [1,2] where you can have a look at the backtrace.

I've bisected this to the following patch:
  PCI: dwc: Use generic config accessors

I'm pretty much lost here. It seems that the kernel tries to read from
an invalid/unmapped memory address.

[1] https://kernelci.org/test/plan/id/5f5f4992d1c53777a0a6092d/
[2] https://storage.kernelci.org/next/master/next-20200914/arm64/defconfig/gcc-8/lab-nxp/baseline-fsl-ls1028a-rdb.txt

-michael
Rob Herring (Arm) Sept. 15, 2020, 10:02 p.m. UTC | #3
On Tue, Sep 15, 2020 at 3:12 AM Michael Walle <michael@walle.cc> wrote:
>
> Hi Rob,
>
> > This is a series of clean-ups for the Designware PCI driver. The series
> > initially reworks the config space accessors to use the existing pci_ops
> > struct. Then there's removal of various private data that's also present
> > in the pci_host_bridge struct. There's also some duplicated common (PCI
> > and DWC) register defines which I converted to use the common defines.
> > Finally, the initialization for speed/gen, number of lanes, and N_FTS
> > are all moved to the common DWC code.
>
> > This is compile tested only as I don't have any DWC based h/w, so any
> > testing would be helpful. A branch is here[1].
>
> I've noticed that with the latest linux-next, my board doesn't boot
> anymore. I've traced it back to this series. There is a similar
> board in kernelci [1,2] where you can have a look at the backtrace.
>
> I've bisected this to the following patch:
>   PCI: dwc: Use generic config accessors

That's helpful.

> I'm pretty much lost here. It seems that the kernel tries to read from
> an invalid/unmapped memory address.
>
> [1] https://kernelci.org/test/plan/id/5f5f4992d1c53777a0a6092d/
> [2] https://storage.kernelci.org/next/master/next-20200914/arm64/defconfig/gcc-8/lab-nxp/baseline-fsl-ls1028a-rdb.txt

Thanks for the pointers. I was wondering if kernelci had any boards with DWC.

Can you try this? The link up check seemed unnecessary as it is racy.
What happens if the link goes down right after checking? That's the
only thing in the change that sticks out.

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
b/drivers/pci/controller/dwc/pcie-designware-host.c
index 317ff512f8df..afee1a0e8883 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -441,6 +441,9 @@ static void __iomem
*dw_pcie_other_conf_map_bus(struct pci_bus *bus,
        struct pcie_port *pp = bus->sysdata;
        struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

+       if (!dw_pcie_link_up(pci))
+               return NULL;
+
        busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
                 PCIE_ATU_FUNC(PCI_FUNC(devfn));
Michael Walle Sept. 16, 2020, 7:54 a.m. UTC | #4
Am 2020-09-16 00:02, schrieb Rob Herring:
> Can you try this? The link up check seemed unnecessary as it is racy.
> What happens if the link goes down right after checking? That's the
> only thing in the change that sticks out.
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 317ff512f8df..afee1a0e8883 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -441,6 +441,9 @@ static void __iomem
> *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
>         struct pcie_port *pp = bus->sysdata;
>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> 
> +       if (!dw_pcie_link_up(pci))
> +               return NULL;
> +
>         busdev = PCIE_ATU_BUS(bus->number) | 
> PCIE_ATU_DEV(PCI_SLOT(devfn)) |
>                  PCIE_ATU_FUNC(PCI_FUNC(devfn));

This will fix the issue.

-michael
Kishon Vijay Abraham I Sept. 29, 2020, 5:23 a.m. UTC | #5
Hi,

On 16/09/20 1:24 pm, Michael Walle wrote:
> Am 2020-09-16 00:02, schrieb Rob Herring:
>> Can you try this? The link up check seemed unnecessary as it is racy.
>> What happens if the link goes down right after checking? That's the
>> only thing in the change that sticks out.
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
>> b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 317ff512f8df..afee1a0e8883 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -441,6 +441,9 @@ static void __iomem
>> *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
>>         struct pcie_port *pp = bus->sysdata;
>>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>
>> +       if (!dw_pcie_link_up(pci))
>> +               return NULL;
>> +
>>         busdev = PCIE_ATU_BUS(bus->number) |
>> PCIE_ATU_DEV(PCI_SLOT(devfn)) |
>>                  PCIE_ATU_FUNC(PCI_FUNC(devfn));
> 
> This will fix the issue.

This fix is required to get DRA7 EVM booting again in linux-next.

Thanks
Kishon
Rob Herring (Arm) Sept. 29, 2020, 2:32 p.m. UTC | #6
On Tue, Sep 29, 2020 at 12:24 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi,
>
> On 16/09/20 1:24 pm, Michael Walle wrote:
> > Am 2020-09-16 00:02, schrieb Rob Herring:
> >> Can you try this? The link up check seemed unnecessary as it is racy.
> >> What happens if the link goes down right after checking? That's the
> >> only thing in the change that sticks out.
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> >> b/drivers/pci/controller/dwc/pcie-designware-host.c
> >> index 317ff512f8df..afee1a0e8883 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> >> @@ -441,6 +441,9 @@ static void __iomem
> >> *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> >>         struct pcie_port *pp = bus->sysdata;
> >>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>
> >> +       if (!dw_pcie_link_up(pci))
> >> +               return NULL;
> >> +
> >>         busdev = PCIE_ATU_BUS(bus->number) |
> >> PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> >>                  PCIE_ATU_FUNC(PCI_FUNC(devfn));
> >
> > This will fix the issue.
>
> This fix is required to get DRA7 EVM booting again in linux-next.

Did you see the discussion here[1]? Is firmware setting up the same
register in question?

Rob

[1] http://lore.kernel.org/r/HE1PR0402MB33713A623A37D08AE3253DEB84320@HE1PR0402MB3371.eurprd04.prod.outlook.com