Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Rob Herring]

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