diff mbox series

[v2,10/12] ARM64: dts: marvell: armada-3720-espressobin: declare PCIe reset GPIO

Message ID 20181212102142.16053-11-miquel.raynal@bootlin.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series Bring suspend to RAM support to PCIe Aardvark driver | expand

Commit Message

Miquel Raynal Dec. 12, 2018, 10:21 a.m. UTC
Add a reset-gpios property to the PCIe node.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 3 +++
 1 file changed, 3 insertions(+)

Comments

Miquel Raynal Dec. 13, 2018, 2:33 p.m. UTC | #1
Hello,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Wed, 12 Dec 2018
11:21:40 +0100:

> Add a reset-gpios property to the PCIe node.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> index 094994a9c68e..76a508da80b9 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
> @@ -46,6 +46,9 @@
>  /* J9 */
>  &pcie0 {
>  	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie_pins>;
> +	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
>  };
>  
>  /* J6 */

While this change may be fine for platforms based on Armada 3700 SoC,
it is not for the EspressoBin that has no reset GPIO for PCIe and
instead uses this pin to control the Ethenet switch.

I will re-send a series without this patch. I think it does not hurt to
keep the previous patch adding the pinmux setting in the
Armada-37xx.dtsi file even without using it, so I will drop only this
patch.


Thanks,
Miquèl
Thomas Petazzoni Dec. 13, 2018, 2:36 p.m. UTC | #2
Hello,

On Thu, 13 Dec 2018 15:33:06 +0100, Miquel Raynal wrote:

> I will re-send a series without this patch. I think it does not hurt to
> keep the previous patch adding the pinmux setting in the
> Armada-37xx.dtsi file even without using it, so I will drop only this
> patch.

I tend to disagree here (but perhaps you'll have other arguments to
convince me otherwise): the GPIO used for PCIe reset is a completely
board-specific thing. You can chose whatever GPIO you want, and each
board can be different. Therefore, there is no reason to have such a
pinmux configuration at the SoC level (.dtsi), it should be within the
particular board that uses that pinmux configuration.

This is a rule that we have applied to mvebu platforms in general, and
which I believe is fairly common in many DTs.

Best regards,

Thomas
Miquel Raynal Dec. 17, 2018, 2:31 p.m. UTC | #3
Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Thu, 13 Dec
2018 15:36:19 +0100:

> Hello,
> 
> On Thu, 13 Dec 2018 15:33:06 +0100, Miquel Raynal wrote:
> 
> > I will re-send a series without this patch. I think it does not hurt to
> > keep the previous patch adding the pinmux setting in the
> > Armada-37xx.dtsi file even without using it, so I will drop only this
> > patch.  
> 
> I tend to disagree here (but perhaps you'll have other arguments to
> convince me otherwise): the GPIO used for PCIe reset is a completely
> board-specific thing. You can chose whatever GPIO you want, and each
> board can be different. Therefore, there is no reason to have such a
> pinmux configuration at the SoC level (.dtsi), it should be within the
> particular board that uses that pinmux configuration.
> 
> This is a rule that we have applied to mvebu platforms in general, and
> which I believe is fairly common in many DTs.

Actually this is a pin that may be driven directly by the PCI IP and is
not board-specific (note that the patch is wrong as the functions
should be "pcie" instead of "gpio"). What is board specific is if this
pin is actually wired to the endpoint PCIe card or not.

Anyway, as seen by Gregory, the pinctrl driver must be fixed as when
selecting the "pcie1" group, the driver was poking another area making
the EspressoBin switch unstable. With a quick fix on my side I realized
the reset was not behaving at all as expected. As it is not actually
needed for suspend/resume operation (at least on my setup) I will drop
the 'reset pin' related patches in the next iteration of the series.


Thanks,
Miquèl
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
index 094994a9c68e..76a508da80b9 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dts
@@ -46,6 +46,9 @@ 
 /* J9 */
 &pcie0 {
 	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_pins>;
+	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_HIGH>;
 };
 
 /* J6 */