On Fri, Jul 23, 2021 at 6:12 PM Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote: > > Em Fri, 23 Jul 2021 16:50:59 -0600 > Rob Herring <robh@xxxxxxxxxx> escreveu: > > > On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote: > > > Document the bindings for HiKey 970 (hi3670) PCIe PHY > > > interface, supported via the pcie-kirin driver. > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > > --- > > > .../phy/hisilicon,phy-hi3670-pcie.yaml | 95 +++++++++++++++++++ > > > 1 file changed, 95 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml > > > new file mode 100644 > > > index 000000000000..a5ea13332cac > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml > > > @@ -0,0 +1,95 @@ > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/phy/hisilicon,phy-hi3670-pcie.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: HiSilicon Kirin970 PCIe PHY > > > + > > > +maintainers: > > > + - Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > > + > > > +description: |+ > > > + Bindings for PCIe PHY on HiSilicon Kirin 970. > > > + > > > +properties: > > > + compatible: > > > + const: hisilicon,hi970-pcie-phy > > > + > > > + "#phy-cells": > > > + const: 0 > > > + > > > + reg: > > > + maxItems: 1 > > > + description: PHY Control registers > > > + > > > + phy-supply: > > > + description: The PCIe PHY power supply > > > + > > > + clocks: > > > + items: > > > + - description: PCIe PHY clock > > > + - description: PCIe AUX clock > > > + - description: PCIe APB PHY clock > > > + - description: PCIe APB SYS clock > > > + - description: PCIe ACLK clock > > > + > > > + clock-names: > > > + items: > > > + - const: phy_ref > > > + - const: aux > > > + - const: apb_phy > > > + - const: apb_sys > > > + - const: aclk > > > + > > > + reset-gpios: > > > + description: PCI PERST reset GPIOs > > > + maxItems: 4 > > > + > > > + clkreq-gpios: > > > + description: Clock request GPIOs > > > + maxItems: 3 > > > > Again, this will not work. > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios) > here, right? Both that and CLKREQ. > > It boils down to this fails to describe how the GPIOs are connected > > which is the point of GPIOs in DT. This in no way captures the hierarchy > > of devices. While you may be lucky that you can just assert or > > deassert all the lines at one time, that's not likely to work in a > > more complicated case (such as having to power up/down each device). > > There's no way to power up/down each device, as they all share the > same regulator line (LDO33). So, when this is powered on, all PCI > devices are powered at the same time. I understand that for your board, but you could easily have a power supply per device (or multiple supplies per device). > The original DT had names for each reset-gpio, but this was just > informative, as the only possible way for this hardware to work is > to send the PERST# signal via all GPIOs at the same time. What's the timing requirement here? I doubt 'at the same time' is the actual h/w requirement. My guess is it is before the PCI bus scan if you don't have any hook before each child bus is scanned. > Ok, we might overdesign the DT, in order to consider a non-existent > scenario where it would be possible to power on and reset the devices > in separate, but I can't think on a way to do that, except by maybe > creating virtual phy (or pcie) DT nodes, one for each combination of > regulator + PERST#, and have separate drivers for each one. Such kind > of scenario only makes sense when each PCIe device can be powered on > independently (which is not the case here). If someone made hikey970 with the topology you have, then someone can just as easily make a different topology and one that doesn't work with the assumptions you've made. We're only going to see more and more embedded boards with multiple PCI devices. > If you have a better idea, I'm all ears. There's already a spec for populating PCI devices in DT. It's existed for over 20 years with OpenFirmware[1]. It's not widely used on FDT systems because most cases to date are just a single device attached and they don't have extra things needing to be described in DT. There are a few, but not many examples in the tree of PCI devices with DT nodes. That's the only way to generically describe the topology you have. While I haven't seen another case exactly like yours yet, there are frequent cases of PCI devices (and other discoverable buses) that have extra resources that are not discoverable. And some of those need control before the device can be discovered. I see various work-arounds to the problem, but describing the devices in DT is the right way. It's only going to get solved if the work-arounds are rejected. I care more that the DT binding is correct and less if the kernel side is clean. The kernel implementation can evolve, the DT cannot. > > I realize the right solution is more complex, but that's the only way to > > handle this in a host bridge and board independent way. > > > > If you want the simple solution, just configure all these GPIOs in > > firmware before Linux boots. > > This won't work. The PERST# signal should be sent after initializing > the PCIe + PHY and powering up the PEX8606 PCIe bridge chipset > (via LDO33). That happens when the PCIe driver is loaded. Only because you have no hooks for handling PERST# on devices downstream of the PEX8606. Surely a sequence like this would work: deassert root PERST# (to PEX8606), scan root bus, find and init PCIe bridge, deassert PEX8606 child bus(es) PERST#, scan child bus(es), find and init child devices. I think the .add_bus() hook could work for you. IIRC, that's called before a child bus is scanned. Rob [1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps
