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

On Fri, Jul 23, 2021 at 6:12 PM Mauro Carvalho Chehab
[off-list ref] wrote:

Em Fri, 23 Jul 2021 16:50:59 -0600
Rob Herring [off-list ref] escreveu:

quoted

On Wed, Jul 21, 2021 at 10:39:08AM +0200, Mauro Carvalho Chehab wrote:

quoted

Document the bindings for HiKey 970 (hi3670) PCIe PHY
interface, supported via the pcie-kirin driver.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../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@kernel.org>
+
+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.

quoted

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.

quoted

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy