[v15,04/13] PCI: kirin: Add support for bridge slot DT schema

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>
---

To mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH v15 00/13] at: https://lore.kernel.org/all/cover.1634812676.git.mchehab+huawei@kernel.org/

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

On Thu, Oct 21, 2021 at 11:45:11AM +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>
> ---
> 
> To mailbombing on a large number of people, only mailing lists were C/C on the cover.
> See [PATCH v15 00/13] at: https://lore.kernel.org/all/cover.1634812676.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.
> + */
> +#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];

I think there's been previous discussion about this, but I didn't
follow it, so I'm just double-checking that this is what we want here.

IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an
external PEX 8606 bridge, which seems a little strange to be
hard-coded into the kirin driver this way.

I see that "hisilicon,clken-gpios" is optional, but what if some
platform connects all 6 lanes?  What if there's a different bridge
altogether?

I'll assume this is actually the way we want thing unless I hear
otherwise.

>  };
>  
>  /*
> @@ -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;
>  }
>  
> -- 
> 2.31.1
>

Hi Bjorn,

Em Tue, 2 Nov 2021 11:06:12 -0500
Bjorn Helgaas <helgaas@kernel.org> escreveu:

> > +
> > +	/* Per-slot clkreq */
> > +	int		n_gpio_clkreq;
> > +	int		gpio_id_clkreq[MAX_PCI_SLOTS];
> > +	const char	*clkreq_names[MAX_PCI_SLOTS];  
> 
> I think there's been previous discussion about this, but I didn't
> follow it, so I'm just double-checking that this is what we want here.
> 
> IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an
> external PEX 8606 bridge, which seems a little strange to be
> hard-coded into the kirin driver this way.
> 
> I see that "hisilicon,clken-gpios" is optional, but what if some
> platform connects all 6 lanes?  What if there's a different bridge
> altogether?
> 
> I'll assume this is actually the way we want thing unless I hear
> otherwise.

Yes, there was past discussions about that with Rob, with regards
to how the DT would represent it, which got reflected at the code.
At the end, it was decided to just add a single property inside PCIe:


		pcie@f4000000 {
                        compatible = "hisilicon,kirin970-pcie";
...
                        hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>,
                                                <&gpio20 6 0>;

I don't think this is a problem, as, if some day another bridge would
need a larger number of slots, it is just a matter of changing the
number at the MAX_PCI_SLOTS, as this controls only the size of the array
(and the check for array overflow when parsing the properties).

Regards,
Mauro

On Tuesday 02 November 2021 17:44:15 Mauro Carvalho Chehab wrote:
> Hi Bjorn,
> 
> Em Tue, 2 Nov 2021 11:06:12 -0500
> Bjorn Helgaas <helgaas@kernel.org> escreveu:
> 
> > > +
> > > +	/* Per-slot clkreq */
> > > +	int		n_gpio_clkreq;
> > > +	int		gpio_id_clkreq[MAX_PCI_SLOTS];
> > > +	const char	*clkreq_names[MAX_PCI_SLOTS];  
> >
> > I think there's been previous discussion about this, but I didn't
> > follow it, so I'm just double-checking that this is what we want here.
> > 
> > IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an
> > external PEX 8606 bridge, which seems a little strange to be
> > hard-coded into the kirin driver this way.
> > 
> > I see that "hisilicon,clken-gpios" is optional, but what if some
> > platform connects all 6 lanes?  What if there's a different bridge
> > altogether?
> > 
> > I'll assume this is actually the way we want thing unless I hear
> > otherwise.

I proposed alternative approach how to define it:
https://lore.kernel.org/linux-pci/20211023144252.z7ou2l2tvm6cvtf7@pali/

Reason for a my new proposal is that currently there is lot of
duplicated code in every native pcie controller driver and every driver
is solving same issues by ad-doc code which is not related to host
bridge / controller itself. Like configuration of devices (e.g. PCIe
switch) to the host bridge itself. That is why I send also another RFC:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

I would be happy if we can discuss on them for future drivers. At least
if my proposals make sense or completely not.

> Yes, there was past discussions about that with Rob, with regards
> to how the DT would represent it, which got reflected at the code.
> At the end, it was decided to just add a single property inside PCIe:
> 
> 
> 		pcie@f4000000 {
>                         compatible = "hisilicon,kirin970-pcie";
> ...
>                         hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>,
>                                                 <&gpio20 6 0>;
> 
> I don't think this is a problem, as, if some day another bridge would
> need a larger number of slots, it is just a matter of changing the
> number at the MAX_PCI_SLOTS, as this controls only the size of the array
> (and the check for array overflow when parsing the properties).

It is not a problem for this particular pcie controller. And really
MAX macro can be increased in this driver if there is need for a larger
number of slots. It should work fine.


But if there are going to be added more boards with different hw
topology or even with different pcie controllers then there would be new
issues. E.g. how to figure out which gpio belongs to which slot? Or even
hot-plugging support? Port belongs to either Root Port device or to
Downstream device, which does not have to be at root level. It creates
tree topology and therefore it is not possible to represent GPIOs in
simple list, like it is for kirin DTS. Generally you cannot say to which
slot belongs second GPIO defined in reset-gpios list.

That is why I'm saying it needs some better structure and prepare pci
core code for it. So native pci controller drivers do not have to invent
ad-hoc solutions for specific board setups.

I really think that information about PCIe switch topology should be in
DTS and it should work independently of PCIe controller driver, without
need for board-specific or PCIe-switch-specific ad-hoc hooks in host
bridge drivers, like it is currently.

Bjorn, what do you think?

On Tue, Nov 2, 2021 at 12:44 PM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Hi Bjorn,
>
> Em Tue, 2 Nov 2021 11:06:12 -0500
> Bjorn Helgaas <helgaas@kernel.org> escreveu:
>
> > > +
> > > +   /* Per-slot clkreq */
> > > +   int             n_gpio_clkreq;
> > > +   int             gpio_id_clkreq[MAX_PCI_SLOTS];
> > > +   const char      *clkreq_names[MAX_PCI_SLOTS];
> >
> > I think there's been previous discussion about this, but I didn't
> > follow it, so I'm just double-checking that this is what we want here.
> >
> > IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an
> > external PEX 8606 bridge, which seems a little strange to be
> > hard-coded into the kirin driver this way.
> >
> > I see that "hisilicon,clken-gpios" is optional, but what if some
> > platform connects all 6 lanes?  What if there's a different bridge
> > altogether?

Keep in mind this is HiKey. There's never been a 2nd board upstream
for the SoCs, the boards have a short lifespan in terms of both
support and availability. And yet Hikey manages to do multiple
complicated things on the board design. I have a hard time caring...

> >
> > I'll assume this is actually the way we want thing unless I hear
> > otherwise.
>
> Yes, there was past discussions about that with Rob, with regards
> to how the DT would represent it, which got reflected at the code.

At first I thought these were CLKREQ connections which absolutely
should be per port/bridge like PERST, but they are not. They are
simply enables for the refclk's and pretty specific to HiKey.

> At the end, it was decided to just add a single property inside PCIe:
>
>
>                 pcie@f4000000 {
>                         compatible = "hisilicon,kirin970-pcie";
> ...
>                         hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>,
>                                                 <&gpio20 6 0>;
>
> I don't think this is a problem, as, if some day another bridge would
> need a larger number of slots, it is just a matter of changing the
> number at the MAX_PCI_SLOTS, as this controls only the size of the array
> (and the check for array overflow when parsing the properties).

It is a problem that your host bridge driver has hardcoded board
specifics. That's not the first time you've heard that from me. But
given the board-to-soc ratio of Hikey is 1:1, I don't care that much.

Rob

Em Thu, 4 Nov 2021 10:36:52 -0500
Rob Herring <robh@kernel.org> escreveu:

> On Tue, Nov 2, 2021 at 12:44 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Hi Bjorn,
> >
> > Em Tue, 2 Nov 2021 11:06:12 -0500
> > Bjorn Helgaas <helgaas@kernel.org> escreveu:
> >  
> > > > +
> > > > +   /* Per-slot clkreq */
> > > > +   int             n_gpio_clkreq;
> > > > +   int             gpio_id_clkreq[MAX_PCI_SLOTS];
> > > > +   const char      *clkreq_names[MAX_PCI_SLOTS];  
> > >
> > > I think there's been previous discussion about this, but I didn't
> > > follow it, so I'm just double-checking that this is what we want here.
> > >
> > > IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an
> > > external PEX 8606 bridge, which seems a little strange to be
> > > hard-coded into the kirin driver this way.
> > >
> > > I see that "hisilicon,clken-gpios" is optional, but what if some
> > > platform connects all 6 lanes?  What if there's a different bridge
> > > altogether?  
> 
> Keep in mind this is HiKey. There's never been a 2nd board upstream
> for the SoCs, the boards have a short lifespan in terms of both
> support and availability. And yet Hikey manages to do multiple
> complicated things on the board design. I have a hard time caring...
> 
> > >
> > > I'll assume this is actually the way we want thing unless I hear
> > > otherwise.  
> >
> > Yes, there was past discussions about that with Rob, with regards
> > to how the DT would represent it, which got reflected at the code.  
> 
> At first I thought these were CLKREQ connections which absolutely
> should be per port/bridge like PERST, but they are not. They are
> simply enables for the refclk's and pretty specific to HiKey.
> 
> > At the end, it was decided to just add a single property inside PCIe:
> >
> >
> >                 pcie@f4000000 {
> >                         compatible = "hisilicon,kirin970-pcie";
> > ...
> >                         hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>,
> >                                                 <&gpio20 6 0>;
> >
> > I don't think this is a problem, as, if some day another bridge would
> > need a larger number of slots, it is just a matter of changing the
> > number at the MAX_PCI_SLOTS, as this controls only the size of the array
> > (and the check for array overflow when parsing the properties).  
> 
> It is a problem that your host bridge driver has hardcoded board
> specifics. That's not the first time you've heard that from me. But
> given the board-to-soc ratio of Hikey is 1:1, I don't care that much.

Ok, understood. Yeah, it sounds unlikely that we would get more boards
for Kirin 960/970 with PCI support, as this SoC is used for mobile phones,
where neither USB nor PCI bridges are needed. 

So, AFAIKT, the only the only publicly-available boards that will be
using this driver are HiKey 960 and HiKey 970.

Regards,
Mauro