diff mbox series

[v5,2/8] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

Message ID baa7e71e13953b28a11fffdcef35195099feb7fd.1626157454.git.mchehab+huawei@kernel.org
State Superseded
Headers show
Series Add support for Hikey 970 PCIe | expand

Commit Message

Mauro Carvalho Chehab July 13, 2021, 6:28 a.m. UTC 
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        | 101 ++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml

Comments

Rob Herring (Arm) July 14, 2021, 2:26 a.m. UTC | #1 
On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:
> Document the bindings for HiKey 970 (hi3670) PCIe PHY
> interface, supported via the pcie-kirin driver.

Same comments on this one.

> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  .../phy/hisilicon,phy-hi3670-pcie.yaml        | 101 ++++++++++++++++++
>  1 file changed, 101 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..976ab6fe7b0a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
> @@ -0,0 +1,101 @@
> +# 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
> +
> +  reg-names:
> +    const: phy
> +
> +  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: pcie_phy_ref
> +      - const: pcie_aux
> +      - const: pcie_apb_phy
> +      - const: pcie_apb_sys
> +      - const: pcie_aclk
> +
> +  reset-gpios:
> +    description: PCI PERST reset GPIOs
> +    maxItems: 4

Hiding the 4 ports in the phy?

> +
> +  clkreq-gpios:
> +    description: Clock request GPIOs
> +    maxItems: 3
> +
> +  hisilicon,eye-diagram-param:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: Eye diagram for phy.

Is there a size to this array?

> +
> +required:
> +  - "#phy-cells"
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - reset-gpios
> +  - clkreq-gpios
> +  - hisilicon,eye-diagram-param
> +  - phy-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/hi3670-clock.h>
> +
> +    bus {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      pcie_phy: pcie-phy@fc000000 {
> +        compatible = "hisilicon,hi970-pcie-phy";
> +        reg = <0x0 0xfc000000 0x0 0x80000>;
> +        reg-names = "phy";
> +        #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 = "pcie_phy_ref", "pcie_aux",
> +                      "pcie_apb_phy", "pcie_apb_sys", "pcie_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>;
> +      };
> +    };
> +...
> -- 
> 2.31.1
> 
>
Mauro Carvalho Chehab July 14, 2021, 7:14 a.m. UTC | #2 
Em Tue, 13 Jul 2021 20:26:49 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:

> > +  reset-gpios:
> > +    description: PCI PERST reset GPIOs
> > +    maxItems: 4  
> 
> Hiding the 4 ports in the phy?

Rob,

I'm not trying to hide anything.

There are several differences with regards to how PERST# is handled between
HiKey 960 and HiKey 970.

From hardware perspective, you can see the schematics of both boards:

	https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf
	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf

The 960 PHY has the SoC directly connected to a PCIE M.2 slot 
(model 10130616) without any external bridge chipset. It uses a single 
GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter
(from 1.8V to 3.3V).

	$ lspci
	00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)

The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).
Besides the bridge, the hardware comes with an Ethernet PCI adapter, a
M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,
mapped to different GPIO pins, and each one using its own voltage
converter.

	$ lspci
	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)

On other words, there are 4 GPIOs mapped to different PERST# pins in
the hardware:

- GPIO 56 is connected to the PERST# pin at PEX 8606;
- GPIO 25 is connected to the PERST# pin at the M.2 slot;
- GPIO 220 is connected to the PERST# pin at the PCIe mini slot;
- GPIO 203 is connected to the PERST# pin at the Ethernet chipset.

Maybe due to different electrical requirements, the hardware design
use different GPIOs instead of feeding them altogether.

Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are
need in order for the hardware to work. and this is specific to this
particular PHY.

Now, from software perspective, the power on sequence on Hikey 960
finishes sending PERST# signal to the M.2 slot:

	static int hi3660_pcie_phy_power_on(struct phy *generic_phy)
	{
...
		/* 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:
		kirin_pcie_clk_ctrl(phy, false);
		return ret;
	}
	
The 970 PHY, however, sends PERST# signal in the middle of the power
on sequence, as, after sending reset, it needs to wait for the hardware
to stabilize, in order to setup an eye diagram at the PHY:

	static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
	{
...
		/* 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;

		hi3670_pcie_set_eyeparam(phy);

		ret = hi3670_pcie_noc_power(phy, false);
		if (ret)
			goto disable_clks;

		return 0;

	disable_clks:
		kirin_pcie_clk_ctrl(phy, false);
		return ret;
	}

IMO, it makes a lot more sense to map this on DT as part of the
PHY and not as part of the PCIe, but no matter how it is mapped,
this PHY still requires 4 GPIOs for PERST#.

Thanks,
Mauro
Rob Herring (Arm) July 14, 2021, 2:17 p.m. UTC | #3 
On Wed, Jul 14, 2021 at 1:14 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 13 Jul 2021 20:26:49 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:
>
> > > +  reset-gpios:
> > > +    description: PCI PERST reset GPIOs
> > > +    maxItems: 4
> >
> > Hiding the 4 ports in the phy?
>
> Rob,
>
> I'm not trying to hide anything.
>
> There are several differences with regards to how PERST# is handled between
> HiKey 960 and HiKey 970.
>
> From hardware perspective, you can see the schematics of both boards:
>
>         https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf
>         https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
>
> The 960 PHY has the SoC directly connected to a PCIE M.2 slot
> (model 10130616) without any external bridge chipset. It uses a single
> GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter
> (from 1.8V to 3.3V).
>
>         $ lspci
>         00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)
>
> The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).
> Besides the bridge, the hardware comes with an Ethernet PCI adapter, a
> M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,
> mapped to different GPIO pins, and each one using its own voltage
> converter.
>
>         $ lspci
>         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)
>
> On other words, there are 4 GPIOs mapped to different PERST# pins in
> the hardware:
>
> - GPIO 56 is connected to the PERST# pin at PEX 8606;
> - GPIO 25 is connected to the PERST# pin at the M.2 slot;
> - GPIO 220 is connected to the PERST# pin at the PCIe mini slot;
> - GPIO 203 is connected to the PERST# pin at the Ethernet chipset.
>
> Maybe due to different electrical requirements, the hardware design
> use different GPIOs instead of feeding them altogether.
>
> Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are
> need in order for the hardware to work. and this is specific to this
> particular PHY.

This hierarchy could be done on any board. It has nothing to do with the PHY.

> Now, from software perspective, the power on sequence on Hikey 960
> finishes sending PERST# signal to the M.2 slot:
>
>         static int hi3660_pcie_phy_power_on(struct phy *generic_phy)
>         {
> ...
>                 /* 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:
>                 kirin_pcie_clk_ctrl(phy, false);
>                 return ret;
>         }
>
> The 970 PHY, however, sends PERST# signal in the middle of the power
> on sequence, as, after sending reset, it needs to wait for the hardware
> to stabilize, in order to setup an eye diagram at the PHY:
>
>         static int hi3670_pcie_phy_power_on(struct phy *generic_phy)
>         {
> ...
>                 /* 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;
>
>                 hi3670_pcie_set_eyeparam(phy);
>
>                 ret = hi3670_pcie_noc_power(phy, false);
>                 if (ret)
>                         goto disable_clks;
>
>                 return 0;
>
>         disable_clks:
>                 kirin_pcie_clk_ctrl(phy, false);
>                 return ret;
>         }
>
> IMO, it makes a lot more sense to map this on DT as part of the
> PHY and not as part of the PCIe, but no matter how it is mapped,
> this PHY still requires 4 GPIOs for PERST#.

It does not because PERST# control is part of PCIe for every other driver.

Rob
Mauro Carvalho Chehab July 15, 2021, 6:37 a.m. UTC | #4 
Hi Mani,

Em Wed, 14 Jul 2021 23:12:25 +0530
Manivannan Sadhasivam <mani@kernel.org> escreveu:

> Hi Mauro,

> 

> On Wed, Jul 14, 2021 at 09:14:35AM +0200, Mauro Carvalho Chehab wrote:

> > Em Tue, 13 Jul 2021 20:26:49 -0600

> > Rob Herring <robh@kernel.org> escreveu:

> >   

> > > On Tue, Jul 13, 2021 at 08:28:35AM +0200, Mauro Carvalho Chehab wrote:  

> >   

> > > > +  reset-gpios:

> > > > +    description: PCI PERST reset GPIOs

> > > > +    maxItems: 4    

> > > 

> > > Hiding the 4 ports in the phy?  

> > 

> > Rob,

> > 

> > I'm not trying to hide anything.

> > 

> > There are several differences with regards to how PERST# is handled between

> > HiKey 960 and HiKey 970.

> > 

> > From hardware perspective, you can see the schematics of both boards:

> > 

> > 	https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf

> > 	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf

> > 

> > The 960 PHY has the SoC directly connected to a PCIE M.2 slot 

> > (model 10130616) without any external bridge chipset. It uses a single 

> > GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter

> > (from 1.8V to 3.3V).

> > 

> > 	$ lspci

> > 	00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)

> > 

> > The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).

> > Besides the bridge, the hardware comes with an Ethernet PCI adapter, a

> > M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,

> > mapped to different GPIO pins, and each one using its own voltage

> > converter.

> > 

> > 	$ lspci

> > 	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)

> > 

> > On other words, there are 4 GPIOs mapped to different PERST# pins in

> > the hardware:

> > 

> > - GPIO 56 is connected to the PERST# pin at PEX 8606;

> > - GPIO 25 is connected to the PERST# pin at the M.2 slot;

> > - GPIO 220 is connected to the PERST# pin at the PCIe mini slot;

> > - GPIO 203 is connected to the PERST# pin at the Ethernet chipset.

> > 

> > Maybe due to different electrical requirements, the hardware design

> > use different GPIOs instead of feeding them altogether.

> > 

> > Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are

> > need in order for the hardware to work. and this is specific to this

> > particular PHY.

> >   

> 

> I'm not sure about this. That fact that the PCIe device's PERST# signal

> wired to different GPIOs doesn't mean that those GPIOs belong to the PHY.

> Those GPIOs should be independent of the PCIe core controlled manually

> by the driver.

> 

> I think this issue is somewhat similar to the one we are dealing on the

> Qcom platforms [1] where each PCIe device uses a different GPIO and voltage

> config to operate. And those need to be active for the link training to

> succeed.

> 

> So perhaps we should aim for a common solution? The GPIO and voltage

> layout should be described in DT for each port exposed by the SoC/board.


It doesn't seem to be the same case. In the case of Hikey 970[1], it uses
one pullup for each GPIO to convert from 1V8 to the voltage needed by
each chipset. There's no regulators envolved: the output voltage is
fixed. From the diagram, it seems that PEX8606 uses 2V5 (via U2602), while 
the other 3 GPIOs (M.2, PCIe mini and Ethernet) use 3V3. Interesting
enough, the layout uses a separate gate chip for Ethernet (U3107),
while both M.2 and PCIe mini use two ports at the same gate chip (U2801).

[1] https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf

I can think on a couple of reasons why the hardware designers opted to
use 4 different GPIOs:

- the PCIe external bridge required a lower voltage;
- the current drained by each output port could be different;
- the chips used by the pullup could have some sort of protection to
  minimize the risk of damaging the hardware due to a bad contact
  and/or short circuit (for M.2 and PCIe mini connectors).

In any case, from DT perspective, the only thing that makes sense to
expose are the 4 GPIOs used by the PERST# signal, as the output voltages
and max currents are fixed.

Conceptually, IMO, such pullup logic can be considered as part of 
the physical layer, but, as Rob pointed, all PCIe devices need to
have PERST#. So, they could be just mapped as gpio-resets at the
PCIe OF node.

In summary, IMO, mapping those 4 GPIOs via DT can be done by
either:

1. Place all of them at the PCIe OF node;
2. Place all of them at the PHY OF node;
3. Place one of them at the PCIe, and the other three at PHY.

IMO, (3) is messier. So, I would go either for (1) or (2). Between them,
I don't have any preferences, as both would work.

Right now, pci-bus.yaml doesn't allow (1), as it limits reset-gpios
to a maximum of 1 item, but this is easily fixable, but it requires
a patch against https://github.com/devicetree-org/dt-schema to raise
MaxItems to at least 4.

Rob,

Among the above alternatives, what do you prefer?

Thanks,
Mauro
Mauro Carvalho Chehab July 19, 2021, 3:26 p.m. UTC | #5 
Hi Rob/Bjorn,

I rebased my repository in a way that the PHY for Kirin 960 won't
be touched. As I explained before:

	https://lore.kernel.org/lkml/20210716132208.3cd8f404@coco.lan/

the only way of doing that without modifying the PHY core and doing ugly
hacks is to not split Kirin 960 PHY from the pcie-kirin driver. 

So, the approach I took was to make the core code at the pcie-kirin driver
generic, ensuring that the internal PHY code is self-contained.

Then, I added support for using a PHY from drivers/phy. This way, the DT 
schema for Kirin 960 won't be touched. The patch series is at:

	https://github.com/mchehab/linux/commits/pcie-alternate

Now, this is the only pending issue on my TODO list:

Em Thu, 15 Jul 2021 08:37:09 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> > > > > +  reset-gpios:

> > > > > +    description: PCI PERST reset GPIOs

> > > > > +    maxItems: 4      

> > > > 

> > > > Hiding the 4 ports in the phy?    

> > > 

> > > Rob,

> > > 

> > > I'm not trying to hide anything.

> > > 

> > > There are several differences with regards to how PERST# is handled between

> > > HiKey 960 and HiKey 970.

> > > 

> > > From hardware perspective, you can see the schematics of both boards:

> > > 

> > > 	https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf

> > > 	https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf

> > > 

> > > The 960 PHY has the SoC directly connected to a PCIE M.2 slot 

> > > (model 10130616) without any external bridge chipset. It uses a single 

> > > GPIO (GPIO 089) for the PERST# signal, connected via a voltage converter

> > > (from 1.8V to 3.3V).

> > > 

> > > 	$ lspci

> > > 	00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)

> > > 

> > > The 970 PHY has an external PCI bridge chipset (PLX Technology PEX 8606).

> > > Besides the bridge, the hardware comes with an Ethernet PCI adapter, a

> > > M.2 slot and a mini-PCIe connector. Each one with its own PERST# signal,

> > > mapped to different GPIO pins, and each one using its own voltage

> > > converter.

> > > 

> > > 	$ lspci

> > > 	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)

> > > 

> > > On other words, there are 4 GPIOs mapped to different PERST# pins in

> > > the hardware:

> > > 

> > > - GPIO 56 is connected to the PERST# pin at PEX 8606;

> > > - GPIO 25 is connected to the PERST# pin at the M.2 slot;

> > > - GPIO 220 is connected to the PERST# pin at the PCIe mini slot;

> > > - GPIO 203 is connected to the PERST# pin at the Ethernet chipset.

> > > 

> > > Maybe due to different electrical requirements, the hardware design

> > > use different GPIOs instead of feeding them altogether.

> > > 

> > > Anyway, the fact is that the PHY on 970 has 4 different GPIOs that are

> > > need in order for the hardware to work. and this is specific to this

> > > particular PHY.

> > >     

> > 

> > I'm not sure about this. That fact that the PCIe device's PERST# signal

> > wired to different GPIOs doesn't mean that those GPIOs belong to the PHY.

> > Those GPIOs should be independent of the PCIe core controlled manually

> > by the driver.

> > 

> > I think this issue is somewhat similar to the one we are dealing on the

> > Qcom platforms [1] where each PCIe device uses a different GPIO and voltage

> > config to operate. And those need to be active for the link training to

> > succeed.

> > 

> > So perhaps we should aim for a common solution? The GPIO and voltage

> > layout should be described in DT for each port exposed by the SoC/board.  

> 

> It doesn't seem to be the same case. In the case of Hikey 970[1], it uses

> one pullup for each GPIO to convert from 1V8 to the voltage needed by

> each chipset. There's no regulators envolved: the output voltage is

> fixed. From the diagram, it seems that PEX8606 uses 2V5 (via U2602), while 

> the other 3 GPIOs (M.2, PCIe mini and Ethernet) use 3V3. Interesting

> enough, the layout uses a separate gate chip for Ethernet (U3107),

> while both M.2 and PCIe mini use two ports at the same gate chip (U2801).

> 

> [1] https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf

> 

> I can think on a couple of reasons why the hardware designers opted to

> use 4 different GPIOs:

> 

> - the PCIe external bridge required a lower voltage;

> - the current drained by each output port could be different;

> - the chips used by the pullup could have some sort of protection to

>   minimize the risk of damaging the hardware due to a bad contact

>   and/or short circuit (for M.2 and PCIe mini connectors).

> 

> In any case, from DT perspective, the only thing that makes sense to

> expose are the 4 GPIOs used by the PERST# signal, as the output voltages

> and max currents are fixed.

> 

> Conceptually, IMO, such pullup logic can be considered as part of 

> the physical layer, but, as Rob pointed, all PCIe devices need to

> have PERST#. So, they could be just mapped as gpio-resets at the

> PCIe OF node.

> 

> In summary, IMO, mapping those 4 GPIOs via DT can be done by

> either:

> 

> 1. Place all of them at the PCIe OF node;

> 2. Place all of them at the PHY OF node;

> 3. Place one of them at the PCIe, and the other three at PHY.

> 

> IMO, (3) is messier. So, I would go either for (1) or (2). Between them,

> I don't have any preferences, as both would work.

> 

> Right now, pci-bus.yaml doesn't allow (1), as it limits reset-gpios

> to a maximum of 1 item, but this is easily fixable, but it requires

> a patch against https://github.com/devicetree-org/dt-schema to raise

> MaxItems to at least 4.

> 

> Rob,

> 

> Among the above alternatives, what do you prefer?


At the patch series I mentioned, I'm keeping the PERST# signals as
part of the PHY layer, just because DT currently produces a warning
if maxItems > 1 for gpio-resets for PCI bus.

In the case it was decided to allow the PCI bus to have more than
one PERST# pin, I already sent a patch to DT-schema:

	https://github.com/devicetree-org/dt-schema/pull/56

But I'll need to change the patch series to consider such
approach, if this will be the case.

So, please let me know how do you prefer to proceed with that.

Thanks!
Mauro
Mauro Carvalho Chehab July 27, 2021, 8:11 a.m. UTC | #6 
Hi Mani,

Em Wed, 14 Jul 2021 23:12:25 +0530
Manivannan Sadhasivam <mani@kernel.org> escreveu:

> I'm not sure about this. That fact that the PCIe device's PERST# signal

> wired to different GPIOs doesn't mean that those GPIOs belong to the PHY.

> Those GPIOs should be independent of the PCIe core controlled manually

> by the driver.

> 

> I think this issue is somewhat similar to the one we are dealing on the

> Qcom platforms [1] where each PCIe device uses a different GPIO and voltage

> config to operate. And those need to be active for the link training to

> succeed.

> 

> So perhaps we should aim for a common solution? The GPIO and voltage

> layout should be described in DT for each port exposed by the SoC/board.

> 

> Thanks,

> Mani

> 

> [1] https://lkml.org/lkml/2021/6/21/1524


After re-visiting this issue, I'm starting to think that this should
be mapped as something similar to:

	pcie@xxxx {
...
		slot {
			slot#1 {
				// clock, power supply, reset pins, etc
			}
			slot#2 {
				// clock, power supply, reset pins, etc
			}
...
		}
	};

E. g. placing each specific PCIe device requirement inside the pcie
or phy, as it should be up to the driver to initialize each PCIe 
child-specific requirements when the hardware is ready for that.

---

A longer explanation why this should be initialized during PHY
power on sequence:

On my tests with Kirin 970, there are some steps to be done before
enabling the clocks and sending PERST# signals, plus some extra
steps to run after PERST# is sent to all devices.

While playing with PHY split, I noticed that Linux and/or the SoC
is very sensitive to an specific probing order. If such order is
not followed, an ARM SError happens and the Kernel panics
with something similar to:

  [    1.837458] SError Interrupt on CPU0, code 0xbf000002 -- SError
  [    1.837462] CPU: 0 PID: 74 Comm: kworker/0:1 Not tainted 5.8.0+ #205
  [    1.837463] Hardware name: HiKey970 (DT)
  [    1.837465] Workqueue: events deferred_probe_work_func
  [    1.837467] pstate: 20000005 (nzCv daif -PAN -UAO BTYPE=--)
  [    1.837468] pc : _raw_spin_unlock_irqrestore+0x18/0x50
  [    1.837469] lr : regmap_unlock_spinlock+0x14/0x20
...
  [    1.837507] Kernel panic - not syncing: Asynchronous SError Interrupt


One example is with regards to the clocks required for the PCIe
to work:

	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>;

If them aren't initialized at the expected order, the Kernel
hangs. The same applies to the slot-specific clocks.

So, basically, the driver needs to initialize them on this
sequence:

	1. PHY ref clock;
	2. APB sys and phy clock;
	3. aclk and aux_clk;
	<some settings at the PHY hardware>
	4. slot-specific clocks.

failing to follow a valid power-on sequence crashes the Kernel.


Thanks,
Mauro
diff mbox series

Patch

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..976ab6fe7b0a
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/hisilicon,phy-hi3670-pcie.yaml
@@ -0,0 +1,101 @@ 
+# 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
+
+  reg-names:
+    const: phy
+
+  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: pcie_phy_ref
+      - const: pcie_aux
+      - const: pcie_apb_phy
+      - const: pcie_apb_sys
+      - const: pcie_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
+  - reg-names
+  - clocks
+  - clock-names
+  - reset-gpios
+  - clkreq-gpios
+  - hisilicon,eye-diagram-param
+  - phy-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/hi3670-clock.h>
+
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+      pcie_phy: pcie-phy@fc000000 {
+        compatible = "hisilicon,hi970-pcie-phy";
+        reg = <0x0 0xfc000000 0x0 0x80000>;
+        reg-names = "phy";
+        #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 = "pcie_phy_ref", "pcie_aux",
+                      "pcie_apb_phy", "pcie_apb_sys", "pcie_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>;
+      };
+    };
+...