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

[Bjorn Helgaas]

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@xxxxxx>
> Acked-by: Xiaowei Song <songxiaowei@xxxxxxxxxxxxx>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> ---
> 
> See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@xxxxxxxxxx/
> 
>  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@xxxxxxxx/