diff mbox series

[v14,04/11] PCI: kirin: Add support for bridge slot DT schema

Message ID f838165c9d279cd1abbacb61fccb74e2a1fbb793.1634622716.git.mchehab+huawei@kernel.org (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series Add support for Hikey 970 PCIe | expand

Commit Message

Mauro Carvalho Chehab Oct. 19, 2021, 6:06 a.m. UTC 
On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
6 lanes. Only 4 lanes are connected:

	lane 0 - connected to Kirin 970;
	lane 4 - M.2 slot;
	lane 5 - mini PCIe slot;
	lane 6 - in-board Ethernet controller.

Each lane has its own PERST# gpio pin, and needs a clock
request.

Add support to parse a DT schema containing the above data.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
 1 file changed, 231 insertions(+), 31 deletions(-)

Comments

Bjorn Helgaas May 24, 2022, 5:19 p.m. UTC | #1 
On Tue, Oct 19, 2021 at 07:06:41AM +0100, Mauro Carvalho Chehab wrote:
> On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
> 6 lanes. Only 4 lanes are connected:
> 
> 	lane 0 - connected to Kirin 970;
> 	lane 4 - M.2 slot;
> 	lane 5 - mini PCIe slot;
> 	lane 6 - in-board Ethernet controller.
> 
> Each lane has its own PERST# gpio pin, and needs a clock
> request.
> 
> Add support to parse a DT schema containing the above data.
> 
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> 
> See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> 
>  drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
>  1 file changed, 231 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index 86c13661e02d..de375795a3b8 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -52,6 +52,19 @@
>  #define PCIE_DEBOUNCE_PARAM	0xF0F400
>  #define PCIE_OE_BYPASS		(0x3 << 28)
>  
> +/*
> + * Max number of connected PCI slots at an external PCI bridge
> + *
> + * This is used on HiKey 970, which has a PEX 8606 bridge with has
> + * 4 connected lanes (lane 0 upstream, and the other tree lanes,
> + * one connected to an in-board Ethernet adapter and the other two
> + * connected to M.2 and mini PCI slots.
> + *
> + * Each slot has a different clock source and uses a separate PERST#
> + * pin.
> ...

> +static int kirin_pcie_add_bus(struct pci_bus *bus)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
> +	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
> +	int i, ret;
> +
> +	if (!kirin_pcie->num_slots)
> +		return 0;
> +
> +	/* Send PERST# to each slot */
> +	for (i = 0; i < kirin_pcie->num_slots; i++) {
> +		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
> +		if (ret) {
> +			dev_err(pci->dev, "PERST# %s error: %d\n",
> +				kirin_pcie->reset_names[i], ret);
> +		}
> +	}
> +	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
> +
> +	return 0;
> +}
> +
> +
>  static struct pci_ops kirin_pci_ops = {
>  	.read = kirin_pcie_rd_own_conf,
>  	.write = kirin_pcie_wr_own_conf,
> +	.add_bus = kirin_pcie_add_bus,

This seems a little weird.  Can you educate me?

From [1], it looks like the topology here is:

  00:00.0 Root Port
  01:00.0 PEX 8606 Upstream Port
  02:01.0 PEX 8606 Downstream Port
  02:04.0 PEX 8606 Downstream Port
  02:05.0 PEX 8606 Downstream Port
  02:07.0 PEX 8606 Downstream Port
  02:09.0 PEX 8606 Downstream Port
  06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller 

The .add_bus() callback will be called for *every* child bus we want
to enumerate.  So if any of those PEX 8606 Downstream Ports are
connected to another switch, when we enumerate the secondary buses of
that other switch, it looks like we'll send PERST# to all the slots
again, which doesn't sound right.  Am I missing something?

Bjorn

[1] https://lore.kernel.org/all/20210129173057.30288c9d@coco.lan/
Bjorn Helgaas May 24, 2022, 6:59 p.m. UTC | #2 
[resending with updated address for Mauro]

On Tue, May 24, 2022 at 12:19:50PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 19, 2021 at 07:06:41AM +0100, Mauro Carvalho Chehab wrote:
> > On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
> > 6 lanes. Only 4 lanes are connected:
> > 
> > 	lane 0 - connected to Kirin 970;
> > 	lane 4 - M.2 slot;
> > 	lane 5 - mini PCIe slot;
> > 	lane 6 - in-board Ethernet controller.
> > 
> > Each lane has its own PERST# gpio pin, and needs a clock
> > request.
> > 
> > Add support to parse a DT schema containing the above data.
> > 
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > 
> > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > 
> >  drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
> >  1 file changed, 231 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index 86c13661e02d..de375795a3b8 100644
> > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > @@ -52,6 +52,19 @@
> >  #define PCIE_DEBOUNCE_PARAM	0xF0F400
> >  #define PCIE_OE_BYPASS		(0x3 << 28)
> >  
> > +/*
> > + * Max number of connected PCI slots at an external PCI bridge
> > + *
> > + * This is used on HiKey 970, which has a PEX 8606 bridge with has
> > + * 4 connected lanes (lane 0 upstream, and the other tree lanes,
> > + * one connected to an in-board Ethernet adapter and the other two
> > + * connected to M.2 and mini PCI slots.
> > + *
> > + * Each slot has a different clock source and uses a separate PERST#
> > + * pin.
> > ...
> 
> > +static int kirin_pcie_add_bus(struct pci_bus *bus)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
> > +	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
> > +	int i, ret;
> > +
> > +	if (!kirin_pcie->num_slots)
> > +		return 0;
> > +
> > +	/* Send PERST# to each slot */
> > +	for (i = 0; i < kirin_pcie->num_slots; i++) {
> > +		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
> > +		if (ret) {
> > +			dev_err(pci->dev, "PERST# %s error: %d\n",
> > +				kirin_pcie->reset_names[i], ret);
> > +		}
> > +	}
> > +	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
> > +
> > +	return 0;
> > +}
> > +
> > +
> >  static struct pci_ops kirin_pci_ops = {
> >  	.read = kirin_pcie_rd_own_conf,
> >  	.write = kirin_pcie_wr_own_conf,
> > +	.add_bus = kirin_pcie_add_bus,
> 
> This seems a little weird.  Can you educate me?
> 
> From [1], it looks like the topology here is:
> 
>   00:00.0 Root Port
>   01:00.0 PEX 8606 Upstream Port
>   02:01.0 PEX 8606 Downstream Port
>   02:04.0 PEX 8606 Downstream Port
>   02:05.0 PEX 8606 Downstream Port
>   02:07.0 PEX 8606 Downstream Port
>   02:09.0 PEX 8606 Downstream Port
>   06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller 
> 
> The .add_bus() callback will be called for *every* child bus we want
> to enumerate.  So if any of those PEX 8606 Downstream Ports are
> connected to another switch, when we enumerate the secondary buses of
> that other switch, it looks like we'll send PERST# to all the slots
> again, which doesn't sound right.  Am I missing something?
> 
> Bjorn
> 
> [1] https://lore.kernel.org/all/20210129173057.30288c9d@coco.lan/
Mauro Carvalho Chehab May 24, 2022, 7:55 p.m. UTC | #3 
Hi Bjorn,

Em Tue, 24 May 2022 12:19:47 -0500
Bjorn Helgaas <helgaas@kernel.org> escreveu:

> On Tue, Oct 19, 2021 at 07:06:41AM +0100, Mauro Carvalho Chehab wrote:
> > On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
> > 6 lanes. Only 4 lanes are connected:
> > 
> > 	lane 0 - connected to Kirin 970;
> > 	lane 4 - M.2 slot;
> > 	lane 5 - mini PCIe slot;
> > 	lane 6 - in-board Ethernet controller.
> > 
> > Each lane has its own PERST# gpio pin, and needs a clock
> > request.
> > 
> > Add support to parse a DT schema containing the above data.
> > 
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > 
> > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > 
> >  drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
> >  1 file changed, 231 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index 86c13661e02d..de375795a3b8 100644
> > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > @@ -52,6 +52,19 @@
> >  #define PCIE_DEBOUNCE_PARAM	0xF0F400
> >  #define PCIE_OE_BYPASS		(0x3 << 28)
> >  
> > +/*
> > + * Max number of connected PCI slots at an external PCI bridge
> > + *
> > + * This is used on HiKey 970, which has a PEX 8606 bridge with has
> > + * 4 connected lanes (lane 0 upstream, and the other tree lanes,
> > + * one connected to an in-board Ethernet adapter and the other two
> > + * connected to M.2 and mini PCI slots.
> > + *
> > + * Each slot has a different clock source and uses a separate PERST#
> > + * pin.
> > ...  
> 
> > +static int kirin_pcie_add_bus(struct pci_bus *bus)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
> > +	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
> > +	int i, ret;
> > +
> > +	if (!kirin_pcie->num_slots)
> > +		return 0;
> > +
> > +	/* Send PERST# to each slot */
> > +	for (i = 0; i < kirin_pcie->num_slots; i++) {
> > +		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
> > +		if (ret) {
> > +			dev_err(pci->dev, "PERST# %s error: %d\n",
> > +				kirin_pcie->reset_names[i], ret);
> > +		}
> > +	}
> > +	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
> > +
> > +	return 0;
> > +}
> > +
> > +
> >  static struct pci_ops kirin_pci_ops = {
> >  	.read = kirin_pcie_rd_own_conf,
> >  	.write = kirin_pcie_wr_own_conf,
> > +	.add_bus = kirin_pcie_add_bus,  
> 
> This seems a little weird.  Can you educate me?
> 
> From [1], it looks like the topology here is:
> 
>   00:00.0 Root Port
>   01:00.0 PEX 8606 Upstream Port
>   02:01.0 PEX 8606 Downstream Port
>   02:04.0 PEX 8606 Downstream Port
>   02:05.0 PEX 8606 Downstream Port
>   02:07.0 PEX 8606 Downstream Port
>   02:09.0 PEX 8606 Downstream Port
>   06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller 
> 
> The .add_bus() callback will be called for *every* child bus we want
> to enumerate.  So if any of those PEX 8606 Downstream Ports are
> connected to another switch, when we enumerate the secondary buses of
> that other switch, it looks like we'll send PERST# to all the slots
> again, which doesn't sound right.  Am I missing something?

The implementation made on Kirin 960/970 for PCI is to not have a
PERST# bus. Instead, it has one independent GPIO driving the PERST#
signal for each single individual port that is physically connected.

See the schematics at:

	- https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
	- https://github.com/96boards/documentation/blob/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf

Yet, my first proposal were to send all GPIOs altogether. See, for
instance:

	https://lore.kernel.org/all/3acf2c073e8ea67efaae91074dda0763bf7a2ab9.1626768323.git.mchehab+huawei@kernel.org/

There, the PERST# signals are initialized altogether, at the end
of hi3670_pcie_phy_power_on():

	/* perst assert Endpoints */
	usleep_range(21000, 23000);
	for (i = 0; i < phy->n_gpio_resets; i++) {
		ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
		if (ret)
			return ret;
	}
	usleep_range(10000, 11000);

	ret = is_pipe_clk_stable(phy);
	if (!ret)
		goto disable_clks;

	hi3670_pcie_set_eyeparam(phy);

	ret = hi3670_pcie_noc_power(phy, false);
	if (ret)
		goto disable_clks;

During the review process, I was requested to change it in order to do it
via .add_bus. On my tests at the boards, I didn't see the same PERST#
signal to hit more than once, and the driver is working fine. 
So, this wasn't an actual issue, as far as I can tell. 

That's what it ended getting merged upstream.

I don't mind if somewone wants to return it to use the previous approach of
sending all per-port PERST# signals at the same time, just before calling 
is_pipe_clk_stable(), like the above. However, I don't have access to the 
hardware anymore, so I can't test any patches for it.

Thanks,
Mauro
Bjorn Helgaas May 24, 2022, 9:29 p.m. UTC | #4 
[+cc Yicong, Rafael, just FYI; trying to figure out how native host
bridge drivers should do power management]

On Tue, May 24, 2022 at 09:55:41PM +0200, Mauro Carvalho Chehab wrote:
> Em Tue, 24 May 2022 12:19:47 -0500
> Bjorn Helgaas <helgaas@kernel.org> escreveu:
> > On Tue, Oct 19, 2021 at 07:06:41AM +0100, Mauro Carvalho Chehab wrote:
> > > On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
> > > 6 lanes. Only 4 lanes are connected:
> > > 
> > > 	lane 0 - connected to Kirin 970;
> > > 	lane 4 - M.2 slot;
> > > 	lane 5 - mini PCIe slot;
> > > 	lane 6 - in-board Ethernet controller.
> > > 
> > > Each lane has its own PERST# gpio pin, and needs a clock
> > > request.
> > > 
> > > Add support to parse a DT schema containing the above data.
> > > 
> > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > > 
> > > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > > 
> > >  drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
> > >  1 file changed, 231 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > > index 86c13661e02d..de375795a3b8 100644
> > > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > > @@ -52,6 +52,19 @@
> > >  #define PCIE_DEBOUNCE_PARAM	0xF0F400
> > >  #define PCIE_OE_BYPASS		(0x3 << 28)
> > >  
> > > +/*
> > > + * Max number of connected PCI slots at an external PCI bridge
> > > + *
> > > + * This is used on HiKey 970, which has a PEX 8606 bridge with has
> > > + * 4 connected lanes (lane 0 upstream, and the other tree lanes,
> > > + * one connected to an in-board Ethernet adapter and the other two
> > > + * connected to M.2 and mini PCI slots.
> > > + *
> > > + * Each slot has a different clock source and uses a separate PERST#
> > > + * pin.
> > > ...  
> > 
> > > +static int kirin_pcie_add_bus(struct pci_bus *bus)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
> > > +	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
> > > +	int i, ret;
> > > +
> > > +	if (!kirin_pcie->num_slots)
> > > +		return 0;
> > > +
> > > +	/* Send PERST# to each slot */
> > > +	for (i = 0; i < kirin_pcie->num_slots; i++) {
> > > +		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
> > > +		if (ret) {
> > > +			dev_err(pci->dev, "PERST# %s error: %d\n",
> > > +				kirin_pcie->reset_names[i], ret);
> > > +		}
> > > +	}
> > > +	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +
> > >  static struct pci_ops kirin_pci_ops = {
> > >  	.read = kirin_pcie_rd_own_conf,
> > >  	.write = kirin_pcie_wr_own_conf,
> > > +	.add_bus = kirin_pcie_add_bus,  
> > 
> > This seems a little weird.  Can you educate me?
> > 
> > From [1], it looks like the topology here is:
> > 
> >   00:00.0 Root Port
> >   01:00.0 PEX 8606 Upstream Port
> >   02:01.0 PEX 8606 Downstream Port
> >   02:04.0 PEX 8606 Downstream Port
> >   02:05.0 PEX 8606 Downstream Port
> >   02:07.0 PEX 8606 Downstream Port
> >   02:09.0 PEX 8606 Downstream Port
> >   06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller 
> > 
> > The .add_bus() callback will be called for *every* child bus we want
> > to enumerate.  So if any of those PEX 8606 Downstream Ports are
> > connected to another switch, when we enumerate the secondary buses of
> > that other switch, it looks like we'll send PERST# to all the slots
> > again, which doesn't sound right.  Am I missing something?
> 
> The implementation made on Kirin 960/970 for PCI is to not have a
> PERST# bus. Instead, it has one independent GPIO driving the PERST#
> signal for each single individual port that is physically connected.
> 
> See the schematics at:
> 
> 	- https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
> 	- https://github.com/96boards/documentation/blob/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf
> 
> Yet, my first proposal were to send all GPIOs altogether. See, for
> instance:
> 
> 	https://lore.kernel.org/all/3acf2c073e8ea67efaae91074dda0763bf7a2ab9.1626768323.git.mchehab+huawei@kernel.org/
> 
> There, the PERST# signals are initialized altogether, at the end
> of hi3670_pcie_phy_power_on():
> 
> 	/* perst assert Endpoints */
> 	usleep_range(21000, 23000);
> 	for (i = 0; i < phy->n_gpio_resets; i++) {
> 		ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
> 		if (ret)
> 			return ret;
> 	}
> 	usleep_range(10000, 11000);
> 
> 	ret = is_pipe_clk_stable(phy);
> 	if (!ret)
> 		goto disable_clks;
> 
> 	hi3670_pcie_set_eyeparam(phy);
> 
> 	ret = hi3670_pcie_noc_power(phy, false);
> 	if (ret)
> 		goto disable_clks;
> 
> During the review process, I was requested to change it in order to do it
> via .add_bus. 

I see Rob suggested .add_bus() at
https://lore.kernel.org/all/CAL_JsqLA7Z908SQKkZpyEcCvpkWsW3pa42eajpxCSkbUy4rv9g@mail.gmail.com/

This seems sort of similar to what Yicong is doing here:
https://lore.kernel.org/r/20220517124319.47125-1-yangyicong@hisilicon.com
but I don't know enough to put all the pieces together.

> On my tests at the boards, I didn't see the same PERST#
> signal to hit more than once, and the driver is working fine. 
> So, this wasn't an actual issue, as far as I can tell. 

Do you remember whether you tested with a switch below the PEX 8606?
E.g., something like this:

  00:00.0 Root Port                to [bus 01-ff]
  01:00.0 PEX 8606 Upstream Port   to [bus 02-ff]
  02:01.0 PEX 8606 Downstream Port to [bus 06-ff]
  06:00.0 Switch Upstream Port     to [bus 07-ff]
  07:01.0 Switch Downstream Port   to [bus 08]
  08:00.0 Endpoint

I think kirin_pcie_add_bus() will be called several times, probably
once for each bus (00, 01, 02, 06, 07, 08), and it looks like it will
do the same thing every time.  Do we reset the 8606 again when we're
about to scan bus 08?  Surely not, because otherwise we would have
reset the 8606 when scanning bus 06 in the original topology where the
8606 is the only switch.

I don't know enough about GPIOs to understand what happens here.  The
doc suggests that gpio_direction_output() sets the direction (output)
and the initial output value (1).  But I don't see any other reference
to gpio_id_reset[i] that looks like it deasserts PERST#.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 86c13661e02d..de375795a3b8 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -52,6 +52,19 @@ 
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
+/*
+ * Max number of connected PCI slots at an external PCI bridge
+ *
+ * This is used on HiKey 970, which has a PEX 8606 bridge with has
+ * 4 connected lanes (lane 0 upstream, and the other tree lanes,
+ * one connected to an in-board Ethernet adapter and the other two
+ * connected to M.2 and mini PCI slots.
+ *
+ * Each slot has a different clock source and uses a separate PERST#
+ * pin.
+ */
+#define MAX_PCI_SLOTS		3
+
 enum pcie_kirin_phy_type {
 	PCIE_KIRIN_INTERNAL_PHY,
 	PCIE_KIRIN_EXTERNAL_PHY
@@ -64,6 +77,19 @@  struct kirin_pcie {
 	struct regmap   *apb;
 	struct phy	*phy;
 	void		*phy_priv;	/* only for PCIE_KIRIN_INTERNAL_PHY */
+
+	/* DWC PERST# */
+	int		gpio_id_dwc_perst;
+
+	/* Per-slot PERST# */
+	int		num_slots;
+	int		gpio_id_reset[MAX_PCI_SLOTS];
+	const char	*reset_names[MAX_PCI_SLOTS];
+
+	/* Per-slot clkreq */
+	int		n_gpio_clkreq;
+	int		gpio_id_clkreq[MAX_PCI_SLOTS];
+	const char	*clkreq_names[MAX_PCI_SLOTS];
 };
 
 /*
@@ -108,7 +134,6 @@  struct hi3660_pcie_phy {
 	struct clk	*phy_ref_clk;
 	struct clk	*aclk;
 	struct clk	*aux_clk;
-	int		gpio_id_reset;
 };
 
 /* Registers in PCIePHY */
@@ -171,16 +196,6 @@  static int hi3660_pcie_phy_get_resource(struct hi3660_pcie_phy *phy)
 	if (IS_ERR(phy->sysctrl))
 		return PTR_ERR(phy->sysctrl);
 
-	/* gpios */
-	phy->gpio_id_reset = of_get_named_gpio(dev->of_node,
-					       "reset-gpios", 0);
-	if (phy->gpio_id_reset == -EPROBE_DEFER) {
-		return -EPROBE_DEFER;
-	} else if (!gpio_is_valid(phy->gpio_id_reset)) {
-		dev_err(phy->dev, "unable to get a valid gpio pin\n");
-		return -ENODEV;
-	}
-
 	return 0;
 }
 
@@ -297,15 +312,7 @@  static int hi3660_pcie_phy_power_on(struct kirin_pcie *pcie)
 	if (ret)
 		goto disable_clks;
 
-	/* perst assert Endpoint */
-	if (!gpio_request(phy->gpio_id_reset, "pcie_perst")) {
-		usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
-		ret = gpio_direction_output(phy->gpio_id_reset, 1);
-		if (ret)
-			goto disable_clks;
-		usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
-		return 0;
-	}
+	return 0;
 
 disable_clks:
 	hi3660_pcie_phy_clk_ctrl(phy, false);
@@ -347,11 +354,98 @@  static const struct regmap_config pcie_kirin_regmap_conf = {
 	.reg_stride = 4,
 };
 
+static int kirin_pcie_get_gpio_enable(struct kirin_pcie *pcie,
+				      struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	char name[32];
+	int ret, i;
+
+	/* This is an optional property */
+	ret = of_gpio_named_count(np, "hisilicon,clken-gpios");
+	if (ret < 0)
+		return 0;
+
+	if (ret > MAX_PCI_SLOTS) {
+		dev_err(dev, "Too many GPIO clock requests!\n");
+		return -EINVAL;
+	}
+
+	pcie->n_gpio_clkreq = ret;
+
+	for (i = 0; i < pcie->n_gpio_clkreq; i++) {
+		pcie->gpio_id_clkreq[i] = of_get_named_gpio(dev->of_node,
+							    "hisilicon,clken-gpios", i);
+		if (pcie->gpio_id_clkreq[i] < 0)
+			return pcie->gpio_id_clkreq[i];
+
+		sprintf(name, "pcie_clkreq_%d", i);
+		pcie->clkreq_names[i] = devm_kstrdup_const(dev, name,
+							    GFP_KERNEL);
+		if (!pcie->clkreq_names[i])
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
+				 struct platform_device *pdev,
+				 struct device_node *node)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *parent, *child;
+	int ret, slot, i;
+	char name[32];
+
+	for_each_available_child_of_node(node, parent) {
+		for_each_available_child_of_node(parent, child) {
+			i = pcie->num_slots;
+
+			pcie->gpio_id_reset[i] = of_get_named_gpio(child,
+								"reset-gpios", 0);
+			if (pcie->gpio_id_reset[i] < 0)
+				continue;
+
+			pcie->num_slots++;
+			if (pcie->num_slots > MAX_PCI_SLOTS) {
+				dev_err(dev, "Too many PCI slots!\n");
+				return -EINVAL;
+			}
+
+			ret = of_pci_get_devfn(child);
+			if (ret < 0) {
+				dev_err(dev, "failed to parse devfn: %d\n", ret);
+				goto put_node;
+			}
+
+			slot = PCI_SLOT(ret);
+
+			sprintf(name, "pcie_perst_%d", slot);
+			pcie->reset_names[i] = devm_kstrdup_const(dev, name,
+								GFP_KERNEL);
+			if (!pcie->reset_names[i]) {
+				ret = -ENOMEM;
+				goto put_node;
+			}
+		}
+	}
+
+	return 0;
+
+put_node:
+	of_node_put(child);
+	return ret;
+}
+
 static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 				    struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *child, *node = dev->of_node;
 	void __iomem *apb_base;
+	int ret;
 
 	apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
 	if (IS_ERR(apb_base))
@@ -362,7 +456,32 @@  static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 	if (IS_ERR(kirin_pcie->apb))
 		return PTR_ERR(kirin_pcie->apb);
 
+	/* pcie internal PERST# gpio */
+	kirin_pcie->gpio_id_dwc_perst = of_get_named_gpio(dev->of_node,
+							  "reset-gpios", 0);
+	if (kirin_pcie->gpio_id_dwc_perst == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (!gpio_is_valid(kirin_pcie->gpio_id_dwc_perst)) {
+		dev_err(dev, "unable to get a valid gpio pin\n");
+		return -ENODEV;
+	}
+
+	ret = kirin_pcie_get_gpio_enable(kirin_pcie, pdev);
+	if (ret)
+		return ret;
+
+	/* Parse OF children */
+	for_each_available_child_of_node(node, child) {
+		ret = kirin_pcie_parse_port(kirin_pcie, pdev, child);
+		if (ret)
+			goto put_node;
+	}
+
 	return 0;
+
+put_node:
+	of_node_put(child);
+	return ret;
 }
 
 static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
@@ -419,9 +538,33 @@  static int kirin_pcie_wr_own_conf(struct pci_bus *bus, unsigned int devfn,
 	return PCIBIOS_SUCCESSFUL;
 }
 
+static int kirin_pcie_add_bus(struct pci_bus *bus)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
+	int i, ret;
+
+	if (!kirin_pcie->num_slots)
+		return 0;
+
+	/* Send PERST# to each slot */
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
+		if (ret) {
+			dev_err(pci->dev, "PERST# %s error: %d\n",
+				kirin_pcie->reset_names[i], ret);
+		}
+	}
+	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
+
+	return 0;
+}
+
+
 static struct pci_ops kirin_pci_ops = {
 	.read = kirin_pcie_rd_own_conf,
 	.write = kirin_pcie_wr_own_conf,
+	.add_bus = kirin_pcie_add_bus,
 };
 
 static u32 kirin_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
@@ -477,6 +620,44 @@  static int kirin_pcie_host_init(struct pcie_port *pp)
 	return 0;
 }
 
+static int kirin_pcie_gpio_request(struct kirin_pcie *kirin_pcie,
+				   struct device *dev)
+{
+	int ret, i;
+
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		if (!gpio_is_valid(kirin_pcie->gpio_id_reset[i])) {
+			dev_err(dev, "unable to get a valid %s gpio\n",
+				kirin_pcie->reset_names[i]);
+			return -ENODEV;
+		}
+
+		ret = devm_gpio_request(dev, kirin_pcie->gpio_id_reset[i],
+					kirin_pcie->reset_names[i]);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) {
+		if (!gpio_is_valid(kirin_pcie->gpio_id_clkreq[i])) {
+			dev_err(dev, "unable to get a valid %s gpio\n",
+				kirin_pcie->clkreq_names[i]);
+			return -ENODEV;
+		}
+
+		ret = devm_gpio_request(dev, kirin_pcie->gpio_id_clkreq[i],
+					kirin_pcie->clkreq_names[i]);
+		if (ret)
+			return ret;
+
+		ret = gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 0);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct dw_pcie_ops kirin_dw_pcie_ops = {
 	.read_dbi = kirin_pcie_read_dbi,
 	.write_dbi = kirin_pcie_write_dbi,
@@ -499,24 +680,43 @@  static int kirin_pcie_power_on(struct platform_device *pdev,
 		if (ret)
 			return ret;
 
-		return hi3660_pcie_phy_power_on(kirin_pcie);
+		ret = hi3660_pcie_phy_power_on(kirin_pcie);
+		if (ret)
+			return ret;
+	} else {
+		kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL);
+		if (IS_ERR(kirin_pcie->phy))
+			return PTR_ERR(kirin_pcie->phy);
+
+		ret = kirin_pcie_gpio_request(kirin_pcie, dev);
+		if (ret)
+			return ret;
+
+		ret = phy_init(kirin_pcie->phy);
+		if (ret)
+			goto err;
+
+		ret = phy_power_on(kirin_pcie->phy);
+		if (ret)
+			goto err;
 	}
 
-	kirin_pcie->phy = devm_of_phy_get(dev, dev->of_node, NULL);
-	if (IS_ERR(kirin_pcie->phy))
-		return PTR_ERR(kirin_pcie->phy);
+	/* perst assert Endpoint */
+	usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
 
-	ret = phy_init(kirin_pcie->phy);
-	if (ret)
-		goto err;
+	if (!gpio_request(kirin_pcie->gpio_id_dwc_perst, "pcie_perst_bridge")) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_dwc_perst, 1);
+		if (ret)
+			goto err;
+	}
 
-	ret = phy_power_on(kirin_pcie->phy);
-	if (ret)
-		goto err;
+	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
 
 	return 0;
 err:
-	phy_exit(kirin_pcie->phy);
+	if (kirin_pcie->type != PCIE_KIRIN_INTERNAL_PHY)
+		phy_exit(kirin_pcie->phy);
+
 	return ret;
 }