| Message ID | 946f2426bc542638240980931eae924c57f2ba27.1626855713.git.mchehab+huawei@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Hikey 970 PCIe | expand |
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@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. 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). 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. Rob
Em Fri, 23 Jul 2021 16:50:59 -0600 Rob Herring <robh@kernel.org> 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@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? > 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. 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. 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 you have a better idea, I'm all ears. > > 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. Thanks, Mauro
On Fri, Jul 23, 2021 at 6:12 PM Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Em Fri, 23 Jul 2021 16:50:59 -0600 > Rob Herring <robh@kernel.org> 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@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. > > 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
Em Mon, 26 Jul 2021 15:37:28 -0600 Rob Herring <robh@kernel.org> escreveu: > On Fri, Jul 23, 2021 at 6:12 PM Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> wrote: > > > > Em Fri, 23 Jul 2021 16:50:59 -0600 > > Rob Herring <robh@kernel.org> 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@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. > > > > 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). There are probably thousands of alternatives, but I don't see any benefit on trying to do write a very complex abstraction here. > > > 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. No idea, as the available documentation doesn't mention. As this is an old hardware, finding any extra documentation about it is not easy, and, afaikt, the developers who originally worked on it (back in 2017) were already moved to work with least two or three generations that came after this SoC. So, we need to check what the code does. Looking at the code, both the PERST# signals and the CLKREQ happen during the PHY power on sequence, and don't require any special order. They just need to be initialized altogether at the same power on step. The power on steps are: 1. turn on the power supply from the regulator to feed the bridge and other PCI devices; 2. turn on the PHY; 3. request clocks; 4. set PHY clock and enable the other clocks; 5. configure some parameters at the PHY layer; 6. send the PERST# signals. This part has actually a 21 ms delay before sending the signal and will wait for 10ms after sending PERST# to all PCI devices. The actual PERST# code is: usleep_range(21000, 23000); for (i = 0; i < phy->n_gpio_resets; i++) { ret = gpio_direction_output(phy->gpio_id_reset[i], 1); if (ret) return ret; } usleep_range(10000, 11000); In summary, all PERST# signals are sent (about) the same time, and the driver logic will wait for 10 ms. 7. Wait for the PCIe to be stable; 8. Adjust the eye parameter; 9. power off NOC. All the above happens before the PCI bus scan. > > 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. I wouldn't expect a new device using this chipset being upstreamed. As I said before, there are 3 generations after Kirin 970. > > > 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. I'll try to find something, but still I think that this is overdesign, as this is really a single event that was split on multiple GPIOs just due to some voltage/current/temperature constraints. > 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. Yeah, I understand that DT changes would be painful, but, IMHO, writing something very complex here just because the hardware design opted to use multiple GPIOs instead of a single one is overkill. > > > 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. As explained above, after sending the PERST# signal and wait for 10 ms, it tunes the PHY eye diagram and powers off NOC. > [1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps Thanks, Mauro
Em Tue, 27 Jul 2021 01:50:20 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > Em Mon, 26 Jul 2021 15:37:28 -0600 > Rob Herring <robh@kernel.org> escreveu: > > > > > > + 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. The original DT from the downstream version (found at Linaro's tree) has: pcie@f4000000 { compatible = "hisilicon,hikey970"; ... switch,reset-gpios = <&gpio7 0 0 >; eth,reset-gpios = <&gpio25 2 0 >; m_2,reset-gpios = <&gpio3 1 0 >; mini1,reset-gpios = <&gpio27 4 0 >; eth,clkreq-gpios = <&gpio20 6 0 >; m_2,clkreq-gpios = <&gpio27 3 0 >; mini1,clkreq-gpios = <&gpio17 0 0 >; }; So, if we're willing to have a single reset-gpios for the PCIe interface, in order to follow the current pci-bus.yaml schema, this would probably be: reset-gpios = <&gpio7 0 0 >; which maps to the PEX8606 PCIe bridge chip. With that, DT still need to point a per-slot clkreq and reset-gpio. One alternative would be to map it as either 3 PCI or PHY child nodes. E. g. something like this: pcie@f4000000 { compatible = "hisilicon,kirin970-pcie"; ... reset-gpios = <&gpio7 0 0 >; slot { eth { reset-gpios = <&gpio25 2 0>; clkreq-gpios = <&gpio20 6 0>; }; m2 { reset-gpios = <&gpio3 1 0>; clkreq-gpios = <&gpio27 3 0>; }; mini1 { reset-gpios = <&gpio27 4 0>; clkreq-gpios = <&gpio17 0 0>; }; }; }; Placing the child nodes ("slot"?) at the pci bus properties makes more sense to me, but placing them at the PHY node has the advantage of only affecting Kirin 970. In either case, if each child would need a different power supply, it won't be hard to add a "slot-supply" property later on. Would something like that be acceptable for you? > > > 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. > > > > [1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps I was unable to find anything useful there at the two PCI documents. This one: https://www.devicetree.org/open-firmware/bindings/pci/pci-express.txt has just one property that might be useful: physical-slot# The main one: https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf mentions a few child properties, but it doesn't show how those were supposed to be mapped, and none of the properties mentioned there specify clocks, gpios, or reset pins. Thanks, Mauro
On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Em Tue, 27 Jul 2021 01:50:20 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > > > Em Mon, 26 Jul 2021 15:37:28 -0600 > > Rob Herring <robh@kernel.org> escreveu: > > > > > > > > > + 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. > > The original DT from the downstream version (found at Linaro's tree) > has: > > pcie@f4000000 { > compatible = "hisilicon,hikey970"; > ... > switch,reset-gpios = <&gpio7 0 0 >; > eth,reset-gpios = <&gpio25 2 0 >; > m_2,reset-gpios = <&gpio3 1 0 >; > mini1,reset-gpios = <&gpio27 4 0 >; > > eth,clkreq-gpios = <&gpio20 6 0 >; > m_2,clkreq-gpios = <&gpio27 3 0 >; > mini1,clkreq-gpios = <&gpio17 0 0 >; > }; > > So, if we're willing to have a single reset-gpios for the PCIe > interface, in order to follow the current pci-bus.yaml schema, > this would probably be: > > reset-gpios = <&gpio7 0 0 >; > > which maps to the PEX8606 PCIe bridge chip. > > With that, DT still need to point a per-slot clkreq and > reset-gpio. > > One alternative would be to map it as either 3 PCI or PHY > child nodes. E. g. something like this: > > pcie@f4000000 { > compatible = "hisilicon,kirin970-pcie"; > ... > reset-gpios = <&gpio7 0 0 >; > > slot { > eth { > reset-gpios = <&gpio25 2 0>; > clkreq-gpios = <&gpio20 6 0>; > }; > m2 { > reset-gpios = <&gpio3 1 0>; > clkreq-gpios = <&gpio27 3 0>; > }; > mini1 { > reset-gpios = <&gpio27 4 0>; > clkreq-gpios = <&gpio17 0 0>; > }; > }; > }; > > > Placing the child nodes ("slot"?) at the pci bus properties makes more > sense to me, but placing them at the PHY node has the advantage of > only affecting Kirin 970. > > In either case, if each child would need a different power supply, > it won't be hard to add a "slot-supply" property later on. > > Would something like that be acceptable for you? On the right track, but there's already a definition for what child devices look like in pci2_1.pdf. I think you want something like this: pcie@f4000000 { // RP: Bus 0, Device 0 compatible = "hisilicon,kirin970-pcie"; ... reset-gpios = <&gpio7 0 0>; // PERST to switch pcie@0 { // PCIe switch: Bus 1, Device 0 reg = <0 0 0 0 0>; compatible = "pciclass,0604"; device_type = "pci"; pcie@1 { // NC (Can omit this node) reg = <0x80 0 0 0 0>; compatible = "pciclass,0604"; device_type = "pci"; }; pcie@4 { // M.2 reg = <0x200 0 0 0 0>; compatible = "pciclass,0604"; device_type = "pci"; reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot }; pcie@5 { // Mini reg = <0x280 0 0 0 0>; compatible = "pciclass,0604"; device_type = "pci"; reset-gpios = <&gpio7 2 0>; // PERST to Mini slot }; pcie@7 { // Ethernet reg = <0x380 0 0 0 0>; compatible = "pciclass,0604"; device_type = "pci"; reset-gpios = <&gpio7 3 0>; // PERST to Ethernet ethernet@0 { reg = <0 0 0 0 0>; local-mac-address = [ 00 01 02 03 04 05 06 ]; }; }; pcie@9 { // NC reg = <0x480 0 0 0 0>; compatible = "pciclass,0604"; device_type = "pci"; }; }; This is based on what you previously sent: 00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01) 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07) A few notes: I left out #size-cells, #address-cells, and ranges for brevity. I'm not completely sure I've got the bridges mapped to the right functions on Hikey970. That's my best guess looking at the schematics. You should be able to confirm which bridge is the parent bridge for ethernet at least. The compatible strings aren't strictly needed. Linux doesn't look at them. There's a pretty complete example in: arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi The simplest Linux implementation to handle the above is just walk the child nodes and get all the 'reset-gpios' properties. That's not the implementation I think we should have though. We should handle the GPIO as each bridge is probed and children scanned. Rob
Em Tue, 27 Jul 2021 16:17:43 -0600 Rob Herring <robh@kernel.org> escreveu: > On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab > <mchehab+huawei@kernel.org> wrote: > > > > Em Tue, 27 Jul 2021 01:50:20 +0200 > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > > > > > Em Mon, 26 Jul 2021 15:37:28 -0600 > > > Rob Herring <robh@kernel.org> escreveu: > > > > > > > > > > > > + 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. > > > > The original DT from the downstream version (found at Linaro's tree) > > has: > > > > pcie@f4000000 { > > compatible = "hisilicon,hikey970"; > > ... > > switch,reset-gpios = <&gpio7 0 0 >; > > eth,reset-gpios = <&gpio25 2 0 >; > > m_2,reset-gpios = <&gpio3 1 0 >; > > mini1,reset-gpios = <&gpio27 4 0 >; > > > > eth,clkreq-gpios = <&gpio20 6 0 >; > > m_2,clkreq-gpios = <&gpio27 3 0 >; > > mini1,clkreq-gpios = <&gpio17 0 0 >; > > }; > > > > So, if we're willing to have a single reset-gpios for the PCIe > > interface, in order to follow the current pci-bus.yaml schema, > > this would probably be: > > > > reset-gpios = <&gpio7 0 0 >; > > > > which maps to the PEX8606 PCIe bridge chip. > > > > With that, DT still need to point a per-slot clkreq and > > reset-gpio. > > > > One alternative would be to map it as either 3 PCI or PHY > > child nodes. E. g. something like this: > > > > pcie@f4000000 { > > compatible = "hisilicon,kirin970-pcie"; > > ... > > reset-gpios = <&gpio7 0 0 >; > > > > slot { > > eth { > > reset-gpios = <&gpio25 2 0>; > > clkreq-gpios = <&gpio20 6 0>; > > }; > > m2 { > > reset-gpios = <&gpio3 1 0>; > > clkreq-gpios = <&gpio27 3 0>; > > }; > > mini1 { > > reset-gpios = <&gpio27 4 0>; > > clkreq-gpios = <&gpio17 0 0>; > > }; > > }; > > }; > > > > > > Placing the child nodes ("slot"?) at the pci bus properties makes more > > sense to me, but placing them at the PHY node has the advantage of > > only affecting Kirin 970. > > > > In either case, if each child would need a different power supply, > > it won't be hard to add a "slot-supply" property later on. > > > > Would something like that be acceptable for you? > > On the right track, but there's already a definition for what child > devices look like in pci2_1.pdf. I think you want something like this: > > pcie@f4000000 { // RP: Bus 0, Device 0 > compatible = "hisilicon,kirin970-pcie"; > ... > reset-gpios = <&gpio7 0 0>; // PERST to switch > > pcie@0 { // PCIe switch: Bus 1, Device 0 > reg = <0 0 0 0 0>; > compatible = "pciclass,0604"; > device_type = "pci"; > > pcie@1 { // NC (Can omit this node) > reg = <0x80 0 0 0 0>; > compatible = "pciclass,0604"; > device_type = "pci"; > }; > > pcie@4 { // M.2 > reg = <0x200 0 0 0 0>; Not sure what to put at reg. I suspect that the best would be to follow the PEX port number, as, if one day someone decides to implement an I2C driver, this might be useful. > compatible = "pciclass,0604"; > device_type = "pci"; > reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot We also need the clock-req phandle for the three devices. > }; > > pcie@5 { // Mini > reg = <0x280 0 0 0 0>; > compatible = "pciclass,0604"; > device_type = "pci"; > reset-gpios = <&gpio7 2 0>; // PERST to Mini slot > }; > > pcie@7 { // Ethernet > reg = <0x380 0 0 0 0>; > compatible = "pciclass,0604"; > device_type = "pci"; > reset-gpios = <&gpio7 3 0>; // PERST to Ethernet > > ethernet@0 { > reg = <0 0 0 0 0>; > local-mac-address = [ 00 01 02 03 04 05 06 ]; > }; No need to add a mac address here. The Ethernet card has a mac already: 60:fa:9d:xx:xx:xx Which seems to be a valid one: https://hwaddress.com/?q=60%3Afa%3A9d > }; > > pcie@9 { // NC > reg = <0x480 0 0 0 0>; > compatible = "pciclass,0604"; > device_type = "pci"; > }; > }; > > This is based on what you previously sent: > 00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01) > 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > Express Gen 2 (5.0 GT/s) Switch (rev ba) > 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > Express Gen 2 (5.0 GT/s) Switch (rev ba) > 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > Express Gen 2 (5.0 GT/s) Switch (rev ba) > 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > Express Gen 2 (5.0 GT/s) Switch (rev ba) > 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > Express Gen 2 (5.0 GT/s) Switch (rev ba) > 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > Express Gen 2 (5.0 GT/s) Switch (rev ba) > 06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07) > > A few notes: > I left out #size-cells, #address-cells, and ranges for brevity. > > I'm not completely sure I've got the bridges mapped to the right > functions on Hikey970. That's my best guess looking at the schematics. > You should be able to confirm which bridge is the parent bridge for > ethernet at least. I added a NVMe to M.2 slot: $ lspci -D -P -PP 0000:00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01) 0000:00:00.0/01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 0000:00:00.0/01:00.0/02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 0000:00:00.0/01:00.0/02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 0000:00:00.0/01:00.0/02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 0000:00:00.0/01:00.0/02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 0000:00:00.0/01:00.0/02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) 0000:00:00.0/01:00.0/02:01.0/03:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd Device a809 0000:00:00.0/01:00.0/02:07.0/06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07) it sounds that all devices are behind the PEX 8606 bridge: - port 1 seems to be the M.2 slot - port 7 seems to be the Ethernet adapter I don't have any mini PCIe devices here. I'll try to get one in order to be sure about the topology. > The compatible strings aren't strictly needed. Linux doesn't look at them. If not needed, IMO the best would be to not add it, keeping it as simple as possible. > > There's a pretty complete example in: > arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi Thanks! > The simplest Linux implementation to handle the above is just walk the > child nodes and get all the 'reset-gpios' properties. That's not the > implementation I think we should have though. We should handle the > GPIO as each bridge is probed and children scanned. The power-on sequence is: 1. CLK, PHY and DWC initialization; 2. 21ms delay; 3. PERST# sent to each device; 4. 10ms delay; 5. adjust the eye diagram; 6. power off NOC. Most of the above are at the PHY driver. Now, it would be possible to map those as: phy->init() - steps 1 and 2; phy->power_on() - steps 4, 5 and 6. And change somehow the pcie-kirin driver to only call phy->power_on() after doing the bus probing sequence, but a change like that would mean that the eye diagram will only be adjusted at the end. That doesn't sound a good idea to me, as probing devices with a wrong eye diagram could cause bit errors when talking to the devices inside the PCI bus. This is likely not a problem if all devices are directly connected to the hardware, but it could be an issue if someone uses either a M.2 or a mini-PCI extensor. So, IMO, the best would be for the PHY driver to walk the child nodes and get all the 'reset-gpios' properties. With regards to the clock-req phandles, those should be enabled before the PHY clocks, in order to avoid the SError issue. It should be easy to implement this at the the PCIe driver, but, this should happen in early stages at the power-on sequence (before enabling the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to walk the child nodes and get all the 'reset-gpios' properties. For the sake of avoiding to duplicate the walk-though and parsing logic, I would do it only at the PHY driver. Thanks, Mauro
On Wed, Jul 28, 2021 at 1:38 AM Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > Em Tue, 27 Jul 2021 16:17:43 -0600 > Rob Herring <robh@kernel.org> escreveu: > > > On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab > > <mchehab+huawei@kernel.org> wrote: > > > > > > Em Tue, 27 Jul 2021 01:50:20 +0200 > > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > > > > > > > Em Mon, 26 Jul 2021 15:37:28 -0600 > > > > Rob Herring <robh@kernel.org> escreveu: > > > > > > > > > > > > > > > + 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. > > > > > > The original DT from the downstream version (found at Linaro's tree) > > > has: > > > > > > pcie@f4000000 { > > > compatible = "hisilicon,hikey970"; > > > ... > > > switch,reset-gpios = <&gpio7 0 0 >; > > > eth,reset-gpios = <&gpio25 2 0 >; > > > m_2,reset-gpios = <&gpio3 1 0 >; > > > mini1,reset-gpios = <&gpio27 4 0 >; > > > > > > eth,clkreq-gpios = <&gpio20 6 0 >; > > > m_2,clkreq-gpios = <&gpio27 3 0 >; > > > mini1,clkreq-gpios = <&gpio17 0 0 >; > > > }; > > > > > > So, if we're willing to have a single reset-gpios for the PCIe > > > interface, in order to follow the current pci-bus.yaml schema, > > > this would probably be: > > > > > > reset-gpios = <&gpio7 0 0 >; > > > > > > which maps to the PEX8606 PCIe bridge chip. > > > > > > With that, DT still need to point a per-slot clkreq and > > > reset-gpio. > > > > > > One alternative would be to map it as either 3 PCI or PHY > > > child nodes. E. g. something like this: > > > > > > pcie@f4000000 { > > > compatible = "hisilicon,kirin970-pcie"; > > > ... > > > reset-gpios = <&gpio7 0 0 >; > > > > > > slot { > > > eth { > > > reset-gpios = <&gpio25 2 0>; > > > clkreq-gpios = <&gpio20 6 0>; > > > }; > > > m2 { > > > reset-gpios = <&gpio3 1 0>; > > > clkreq-gpios = <&gpio27 3 0>; > > > }; > > > mini1 { > > > reset-gpios = <&gpio27 4 0>; > > > clkreq-gpios = <&gpio17 0 0>; > > > }; > > > }; > > > }; > > > > > > > > > Placing the child nodes ("slot"?) at the pci bus properties makes more > > > sense to me, but placing them at the PHY node has the advantage of > > > only affecting Kirin 970. > > > > > > In either case, if each child would need a different power supply, > > > it won't be hard to add a "slot-supply" property later on. > > > > > > Would something like that be acceptable for you? > > > > On the right track, but there's already a definition for what child > > devices look like in pci2_1.pdf. I think you want something like this: > > > > pcie@f4000000 { // RP: Bus 0, Device 0 > > compatible = "hisilicon,kirin970-pcie"; > > ... > > reset-gpios = <&gpio7 0 0>; // PERST to switch > > > > pcie@0 { // PCIe switch: Bus 1, Device 0 > > reg = <0 0 0 0 0>; > > compatible = "pciclass,0604"; > > device_type = "pci"; > > > > pcie@1 { // NC (Can omit this node) > > reg = <0x80 0 0 0 0>; > > compatible = "pciclass,0604"; > > device_type = "pci"; > > }; > > > > pcie@4 { // M.2 > > reg = <0x200 0 0 0 0>; > > Not sure what to put at reg. I suspect that the best would be to follow > the PEX port number, as, if one day someone decides to implement an > I2C driver, this might be useful. It's defined in the PCI bus binding. Basically, it's the BDF of the device. However, as the bus number is dynamic, I think we want to leave that as 0 for FDT. The function is optional and always 0 in this case. > > compatible = "pciclass,0604"; > > device_type = "pci"; > > reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot > > We also need the clock-req phandle for the three devices. Hopefully, you can figure out where those belong now... > > > }; > > > > pcie@5 { // Mini > > reg = <0x280 0 0 0 0>; > > compatible = "pciclass,0604"; > > device_type = "pci"; > > reset-gpios = <&gpio7 2 0>; // PERST to Mini slot > > }; > > > > pcie@7 { // Ethernet > > reg = <0x380 0 0 0 0>; > > compatible = "pciclass,0604"; > > device_type = "pci"; > > reset-gpios = <&gpio7 3 0>; // PERST to Ethernet > > > > ethernet@0 { > > reg = <0 0 0 0 0>; > > local-mac-address = [ 00 01 02 03 04 05 06 ]; > > }; > > No need to add a mac address here. The Ethernet card has a mac > already: That was just for illustration of what a device node would look like should you need extra properties for a device. If you only needed to set the MAC address, guess what, you need to create the hierarchy above. > > 60:fa:9d:xx:xx:xx > > Which seems to be a valid one: > > https://hwaddress.com/?q=60%3Afa%3A9d > > > }; > > > > pcie@9 { // NC > > reg = <0x480 0 0 0 0>; > > compatible = "pciclass,0604"; > > device_type = "pci"; > > }; > > }; > > > > This is based on what you previously sent: > > 00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01) > > 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > > Express Gen 2 (5.0 GT/s) Switch (rev ba) > > 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > > Express Gen 2 (5.0 GT/s) Switch (rev ba) > > 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > > Express Gen 2 (5.0 GT/s) Switch (rev ba) > > 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > > Express Gen 2 (5.0 GT/s) Switch (rev ba) > > 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > > Express Gen 2 (5.0 GT/s) Switch (rev ba) > > 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI > > Express Gen 2 (5.0 GT/s) Switch (rev ba) > > 06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. > > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07) > > > > A few notes: > > I left out #size-cells, #address-cells, and ranges for brevity. > > > > I'm not completely sure I've got the bridges mapped to the right > > functions on Hikey970. That's my best guess looking at the schematics. > > You should be able to confirm which bridge is the parent bridge for > > ethernet at least. > > I added a NVMe to M.2 slot: > > $ lspci -D -P -PP > 0000:00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01) > 0000:00:00.0/01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) > 0000:00:00.0/01:00.0/02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) > 0000:00:00.0/01:00.0/02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) > 0000:00:00.0/01:00.0/02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) > 0000:00:00.0/01:00.0/02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) > 0000:00:00.0/01:00.0/02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI Express Gen 2 (5.0 GT/s) Switch (rev ba) > 0000:00:00.0/01:00.0/02:01.0/03:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd Device a809 > 0000:00:00.0/01:00.0/02:07.0/06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07) > > it sounds that all devices are behind the PEX 8606 bridge: > - port 1 seems to be the M.2 slot > - port 7 seems to be the Ethernet adapter > > I don't have any mini PCIe devices here. I'll try to get one in order to be > sure about the topology. I found the mapping in table 4-1 from https://docs.broadcom.com/doc/PEX_8606-BA_Data_Book_v1.3_31Mar11.pdf So it is like this: Device 0 - lane 0 - upstream Device 1 - lane 4 - m.2 Device 5 - lane 5 - mini PCIe Device 7 - lane 6 - ethernet 'lane' is the signal numbering in the schematics. > > > The compatible strings aren't strictly needed. Linux doesn't look at them. > > If not needed, IMO the best would be to not add it, keeping it as > simple as possible. I would say that anything with extra properties should have a compatible. > > > > There's a pretty complete example in: > > arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi > > Thanks! > > > The simplest Linux implementation to handle the above is just walk the > > child nodes and get all the 'reset-gpios' properties. That's not the > > implementation I think we should have though. We should handle the > > GPIO as each bridge is probed and children scanned. > > The power-on sequence is: > > 1. CLK, PHY and DWC initialization; > 2. 21ms delay; > 3. PERST# sent to each device; > 4. 10ms delay; > 5. adjust the eye diagram; > 6. power off NOC. > > Most of the above are at the PHY driver. Now, it would be possible to > map those as: > > phy->init() - steps 1 and 2; > phy->power_on() - steps 4, 5 and 6. > > And change somehow the pcie-kirin driver to only call phy->power_on() > after doing the bus probing sequence, but a change like that would mean > that the eye diagram will only be adjusted at the end. That doesn't > sound a good idea to me, as probing devices with a wrong eye diagram > could cause bit errors when talking to the devices inside the PCI bus. > This is likely not a problem if all devices are directly connected to > the hardware, but it could be an issue if someone uses either a M.2 or > a mini-PCI extensor. The eye diagram only applies to the link between the RP and switch. There's no way that any of the downstream PERSTs matter, that's just not logical. So you only need to deassert PERST on that link. What happens if you only handle the switch PERST and CLKREQ? You should simply only discover the switch and no downstream devices. > So, IMO, the best would be for the PHY driver to walk the child nodes > and get all the 'reset-gpios' properties. > > With regards to the clock-req phandles, those should be enabled before > the PHY clocks, in order to avoid the SError issue. Huh? What exactly causes an SError. Has to be some bus access. But again, it's only going to be the one for the RP link that matters here. > It should be easy to implement this at the the PCIe driver, but, this > should happen in early stages at the power-on sequence (before enabling > the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to > walk the child nodes and get all the 'reset-gpios' properties. > > For the sake of avoiding to duplicate the walk-though and parsing > logic, I would do it only at the PHY driver. Everyone else handles this stuff in their PCIe driver. You are not special... I have a plan to make the PERST handling common across all PCIe host drivers and also make the PHY handling common across DWC drivers. Don't make that harder. Rob
Hi Rob, Em Wed, 28 Jul 2021 08:28:26 -0600 Rob Herring <robh@kernel.org> escreveu: > > > pcie@f4000000 { // RP: Bus 0, Device 0 > > > compatible = "hisilicon,kirin970-pcie"; > > > ... > > > reset-gpios = <&gpio7 0 0>; // PERST to switch > > > > > > pcie@0 { // PCIe switch: Bus 1, Device 0 > > > reg = <0 0 0 0 0>; > > > compatible = "pciclass,0604"; > > > device_type = "pci"; > > > > > > pcie@1 { // NC (Can omit this node) > > > reg = <0x80 0 0 0 0>; > > > compatible = "pciclass,0604"; > > > device_type = "pci"; > > > }; > > > > > > pcie@4 { // M.2 > > > reg = <0x200 0 0 0 0>; > > > > Not sure what to put at reg. I suspect that the best would be to follow > > the PEX port number, as, if one day someone decides to implement an > > I2C driver, this might be useful. > > It's defined in the PCI bus binding. Basically, it's the BDF of the > device. However, as the bus number is dynamic, I think we want to > leave that as 0 for FDT. The function is optional and always 0 in this > case. Ok. I'll use 0 there. > > > compatible = "pciclass,0604"; > > > device_type = "pci"; > > > reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot > > > > We also need the clock-req phandle for the three devices. > > Hopefully, you can figure out where those belong now... I added it here too. > > With regards to the clock-req phandles, those should be enabled before > > the PHY clocks, in order to avoid the SError issue. > > Huh? What exactly causes an SError. Has to be some bus access. I'm not sure, but I got lots of those when playing with drivers for this SoC. It is not easy to check the root case when such errors happen, as the backtrace is useless. > > > It should be easy to implement this at the the PCIe driver, but, this > > should happen in early stages at the power-on sequence (before enabling > > the DWC PHY clocks). So, the PCIe driver (or the PHY) will need to > > walk the child nodes and get all the 'reset-gpios' properties. > > > > For the sake of avoiding to duplicate the walk-though and parsing > > logic, I would do it only at the PHY driver. > > Everyone else handles this stuff in their PCIe driver. You are not special... > > I have a plan to make the PERST handling common across all PCIe host > drivers and also make the PHY handling common across DWC drivers. > Don't make that harder. It turns that changing this to work the way you're expecting was a lot simpler than I was expecting ;-) The enclosed patch, applied on the top of my past series, does that. I'll rebase my patch series to take this change into account. Could you please check if the DTS there is OK from your side? I'll then change the dt-schema to match such change. Thanks, Mauro [PATCH] PCI: kirin: change DT schema to use child nodes Instead of having multiple PERST# pins as part of pcie, add PCIe child slots, and place there the information related to clock request and PERST# signals. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi index cae90cd0b06a..a17562bb4dff 100644 --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi @@ -687,9 +687,6 @@ pcie_phy: pcie-phy@fc000000 { "apb_phy", "apb_sys", "aclk"; - clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >, - <&gpio17 0 0 >; - /* vboost iboost pre post main */ hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF @@ -726,8 +723,40 @@ &gic GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>, &gic GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>, <0x0 0 0 4 &gic GIC_SPI 285 IRQ_TYPE_LEVEL_HIGH>; - reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >, - <&gpio3 1 0 >, <&gpio27 4 0 >; + reset-gpios = <&gpio7 0 0 >; + + pcie@4 { // Lane 4: M.2 + reg = <0 0 0 0 0>; + compatible = "pciclass,0604"; + device_type = "pci"; + reset-gpios = <&gpio7 1 0>; + clkreq-gpios = <&gpio27 3 0 >; + #address-cells = <3>; + #size-cells = <2>; + ranges; + }; + + pcie@5 { // Lane 5: Mini PCIe + reg = <0 0 0 0 0>; + compatible = "pciclass,0604"; + device_type = "pci"; + reset-gpios = <&gpio7 2 0>; + clkreq-gpios = <&gpio17 0 0 >; + #address-cells = <3>; + #size-cells = <2>; + ranges; + }; + + pcie@7 { // Lane 7: Ethernet + reg = <0 0 0 0 0>; + compatible = "pciclass,0604"; + device_type = "pci"; + reset-gpios = <&gpio7 3 0>; + clkreq-gpios = <&gpio20 0 0 >; + #address-cells = <3>; + #size-cells = <2>; + ranges; + }; }; /* UFS */ diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c index 9dad14929538..5f515c4c6076 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,18 @@ 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; + + int num_slots; + /* Per-slot PERST# */ + int gpio_id_reset[MAX_PCI_SLOTS]; + const char *reset_names[MAX_PCI_SLOTS]; + + /* Per-slot clkreq */ + int gpio_id_clkreq[MAX_PCI_SLOTS]; + const char *clkreq_names[MAX_PCI_SLOTS]; }; /* @@ -108,7 +133,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 +195,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,16 +311,6 @@ 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; - } - disable_clks: hi3660_pcie_phy_clk_ctrl(phy, false); return ret; @@ -347,11 +351,54 @@ static const struct regmap_config pcie_kirin_regmap_conf = { .reg_stride = 4, }; +static int kirin_pcie_parse_port(struct kirin_pcie *kirin_pcie, + struct platform_device *pdev, + struct device_node *node) +{ + struct device *dev = &pdev->dev; + int i = kirin_pcie->num_slots; + char name[32]; + + if (i + 1 > MAX_PCI_SLOTS) { + dev_err(dev, "Too many PCI slots!\n"); + return -EINVAL; + } + + kirin_pcie->num_slots++; + + /* perst reset gpio */ + kirin_pcie->gpio_id_reset[i] = of_get_named_gpio(node, + "reset-gpios", 0); + + if (kirin_pcie->gpio_id_reset[i] < 0) { + dev_err(dev, "%d: Missing PERST# for %pOF\n", i, node); + return kirin_pcie->gpio_id_reset[i]; + } + + /* clkreq gpio */ + kirin_pcie->gpio_id_clkreq[i] = of_get_named_gpio(node, + "clkreq-gpios", 0); + if (kirin_pcie->gpio_id_clkreq[i] < 0) { + dev_err(dev, "%d: Missing clqreq for %pOF\n", i, node); + return kirin_pcie->gpio_id_clkreq[i]; + } + + sprintf(name, "pcie_clkreq_%d", i); + kirin_pcie->clkreq_names[i] = devm_kstrdup_const(dev, name, + GFP_KERNEL); + if (!kirin_pcie->clkreq_names[i]) + return -ENOMEM; + + return 0; +} + static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie, struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct device_node *node = dev->of_node, *child; void __iomem *apb_base; + int ret; apb_base = devm_platform_ioremap_resource_byname(pdev, "apb"); if (IS_ERR(apb_base)) @@ -362,6 +409,29 @@ 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; + } + + /* Parse OF children */ + for_each_available_child_of_node(node, child) { + ret = of_pci_get_devfn(child); + if (ret < 0) { + dev_info(dev, "failed to parse devfn: %d\n", ret); + return ret; + } + + ret = kirin_pcie_parse_port(kirin_pcie, pdev, child); + if (ret) + return ret; + } + return 0; } @@ -419,9 +489,31 @@ 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; + + /* perst assert Endpoint */ + usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX); + + /* 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) + return 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 +569,46 @@ 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->num_slots; 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; + } + + for (i = 0; i < kirin_pcie->num_slots; i++) { + ret = gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 0); + if (ret) + return ret; + } + + return ret; +} + static const struct dw_pcie_ops kirin_dw_pcie_ops = { .read_dbi = kirin_pcie_read_dbi, .write_dbi = kirin_pcie_write_dbi, @@ -499,24 +631,43 @@ static int kirin_pcie_power_on(struct platform_device *pdev, if (ret) return ret; - return hi3660_pcie_phy_power_on(kirin_pcie); + ret = kirin_pcie_gpio_request(kirin_pcie, dev); + if (ret) + return ret; + + 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 = 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")) { + 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; } diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c index 82cc5fc4eac2..ac6052a05788 100644 --- a/drivers/phy/hisilicon/phy-hi3670-pcie.c +++ b/drivers/phy/hisilicon/phy-hi3670-pcie.c @@ -87,9 +87,6 @@ #define NOC_PW_MASK 0x10000 #define NOC_PW_SET_BIT 0x1 -/* Number of GPIOs required by PHY */ -#define MAX_GPIO_RESETS 4 -#define MAX_GPIO_CLKREQ 3 #define NUM_EYEPARAM 5 /* info located in sysctrl */ @@ -108,10 +105,6 @@ #define CRGCTRL_PCIE_ASSERT_BIT 0x8c000000 /* Time for delay */ -#define REF_2_PERST_MIN 20000 -#define REF_2_PERST_MAX 25000 -#define PERST_2_ACCESS_MIN 10000 -#define PERST_2_ACCESS_MAX 12000 #define PIPE_CLK_WAIT_MIN 550 #define PIPE_CLK_WAIT_MAX 600 #define TIME_CMOS_MIN 100 @@ -131,12 +124,6 @@ struct hi3670_pcie_phy { struct clk *phy_ref_clk; struct clk *aclk; struct clk *aux_clk; - int n_gpio_resets; - int n_gpio_clkreq; - int gpio_id_reset[MAX_GPIO_RESETS]; - const char *reset_names[MAX_GPIO_RESETS]; - int gpio_id_clkreq[MAX_GPIO_CLKREQ]; - const char *clkreq_names[MAX_GPIO_CLKREQ]; u32 eye_param[NUM_EYEPARAM]; }; @@ -230,40 +217,6 @@ static void hi3670_pcie_set_eyeparam(struct hi3670_pcie_phy *phy) kirin_apb_natural_phy_writel(phy, val, LANEN_DIG_ASIC_TX_OVRD_IN_1); } -static int hi3670_pcie_gpio_request(struct hi3670_pcie_phy *phy, - struct device *dev) -{ - int ret, i; - - for (i = 0; i < phy->n_gpio_resets; i++) { - if (!gpio_is_valid(phy->gpio_id_reset[i])) { - dev_err(dev, "unable to get a valid %s gpio\n", - phy->reset_names[i]); - return -ENODEV; - } - - ret = devm_gpio_request(dev, phy->gpio_id_reset[i], - phy->reset_names[i]); - if (ret) - return ret; - } - - for (i = 0; i < phy->n_gpio_clkreq; i++) { - if (!gpio_is_valid(phy->gpio_id_clkreq[i])) { - dev_err(dev, "unable to get a valid %s gpio\n", - phy->clkreq_names[i]); - return -ENODEV; - } - - ret = devm_gpio_request(dev, phy->gpio_id_clkreq[i], - phy->clkreq_names[i]); - if (ret) - return ret; - } - - return ret; -} - static void hi3670_pcie_natural_cfg(struct hi3670_pcie_phy *phy) { u32 val; @@ -558,8 +511,6 @@ static int hi3670_pcie_get_resources_from_pcie(struct hi3670_pcie_phy *phy) struct device_node *pcie_port; struct device *dev = phy->dev; struct device *pcie_dev; - char name[32]; - int i; pcie_port = of_get_child_by_name(dev->parent->of_node, "pcie"); if (!pcie_port) { @@ -588,27 +539,6 @@ static int hi3670_pcie_get_resources_from_pcie(struct hi3670_pcie_phy *phy) return -ENODEV; } - /* perst reset gpios */ - phy->n_gpio_resets = of_gpio_named_count(pcie_dev->of_node, - "reset-gpios"); - if (phy->n_gpio_resets > MAX_GPIO_RESETS) { - dev_err(dev, "Too many GPIO resets!\n"); - return -EINVAL; - } - for (i = 0; i < phy->n_gpio_resets; i++) { - phy->gpio_id_reset[i] = of_get_named_gpio(pcie_dev->of_node, - "reset-gpios", i); - if (phy->gpio_id_reset[i] < 0) - return phy->gpio_id_reset[i]; - - sprintf(name, "pcie_perst_%d", i); - - phy->reset_names[i] = devm_kstrdup_const(dev, name, - GFP_KERNEL); - if (!phy->reset_names[i]) - return -ENOMEM; - } - return 0; } @@ -663,7 +593,6 @@ static int kirin_pcie_clk_ctrl(struct hi3670_pcie_phy *phy, bool enable) static int hi3670_pcie_phy_init(struct phy *generic_phy) { struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy); - struct device *dev = phy->dev; int ret; /* @@ -681,15 +610,6 @@ static int hi3670_pcie_phy_init(struct phy *generic_phy) if (ret) return ret; - /* phy regulator needs to be powered on before calling it */ - return hi3670_pcie_gpio_request(phy, dev); -} - -static int hi3670_pcie_phy_power_on(struct phy *generic_phy) -{ - struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy); - int val, ret, i; - /* Power supply for Host */ regmap_write(phy->sysctrl, SCTRL_PCIE_CMOS_OFFSET, SCTRL_PCIE_CMOS_BIT); @@ -697,11 +617,13 @@ static int hi3670_pcie_phy_power_on(struct phy *generic_phy) hi3670_pcie_phy_oe_enable(phy); - for (i = 0; i < phy->n_gpio_clkreq; i++) { - ret = gpio_direction_output(phy->gpio_id_clkreq[i], 0); - if (ret) - return ret; - } + return 0; +} + +static int hi3670_pcie_phy_power_on(struct phy *generic_phy) +{ + struct hi3670_pcie_phy *phy = phy_get_drvdata(generic_phy); + int val, ret; ret = kirin_pcie_clk_ctrl(phy, true); if (ret) @@ -732,15 +654,6 @@ static int hi3670_pcie_phy_power_on(struct phy *generic_phy) regmap_write(phy->apb, SOC_PCIECTRL_CTRL12_ADDR, val); udelay(10); - /* perst assert Endpoints */ - usleep_range(21000, 23000); - for (i = 0; i < phy->n_gpio_resets; i++) { - ret = gpio_direction_output(phy->gpio_id_reset[i], 1); - if (ret) - return ret; - } - usleep_range(10000, 11000); - ret = is_pipe_clk_stable(phy); if (!ret) goto disable_clks; @@ -781,9 +694,6 @@ static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *phy, struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *np = dev->of_node; - char name[32]; - int i; /* syscon */ phy->crgctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3670-crgctrl"); @@ -824,25 +734,6 @@ static int hi3670_pcie_phy_get_resources(struct hi3670_pcie_phy *phy, if (IS_ERR(phy->base)) return PTR_ERR(phy->base); - /* clock request gpios */ - phy->n_gpio_clkreq = of_gpio_named_count(np, "clkreq-gpios"); - if (phy->n_gpio_clkreq > MAX_GPIO_CLKREQ) { - dev_err(dev, "Too many GPIO clock requests!\n"); - return -EINVAL; - } - for (i = 0; i < phy->n_gpio_clkreq; i++) { - phy->gpio_id_clkreq[i] = of_get_named_gpio(dev->of_node, - "clkreq-gpios", i); - if (phy->gpio_id_clkreq[i] < 0) - return phy->gpio_id_clkreq[i]; - - sprintf(name, "pcie_clkreq_%d", i); - phy->clkreq_names[i] = devm_kstrdup_const(dev, name, - GFP_KERNEL); - if (!phy->clkreq_names[i]) - return -ENOMEM; - } - hi3670_pcie_get_eyeparam(phy); return 0;
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 + + hisilicon,eye-diagram-param: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: Eye diagram for phy. + +required: + - "#phy-cells" + - compatible + - reg + - clocks + - clock-names + - reset-gpios + - clkreq-gpios + - hisilicon,eye-diagram-param + - phy-supply + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/hi3670-clock.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + pcie_phy: pcie-phy@fc000000 { + compatible = "hisilicon,hi970-pcie-phy"; + reg = <0x0 0xfc000000 0x0 0x80000>; + #phy-cells = <0>; + phy-supply = <&ldo33>; + clocks = <&crg_ctrl HI3670_CLK_GATE_PCIEPHY_REF>, + <&crg_ctrl HI3670_CLK_GATE_PCIEAUX>, + <&crg_ctrl HI3670_PCLK_GATE_PCIE_PHY>, + <&crg_ctrl HI3670_PCLK_GATE_PCIE_SYS>, + <&crg_ctrl HI3670_ACLK_GATE_PCIE>; + clock-names = "phy_ref", "aux", + "apb_phy", "apb_sys", "aclk"; + reset-gpios = <&gpio7 0 0 >, <&gpio25 2 0 >, + <&gpio3 1 0 >, <&gpio27 4 0 >; + clkreq-gpios = <&gpio20 6 0 >, <&gpio27 3 0 >, <&gpio17 0 0 >; + hisilicon,eye-diagram-param = <0xFFFFFFFF 0xFFFFFFFF + 0xFFFFFFFF 0xFFFFFFFF 0xFFFFFFFF>; + }; + };
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