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

[Pali Rohár]

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@xxxxxxxxxx> 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?