lkml.org 
[lkml]   [2021]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2] PCI: apple: Follow the PCIe specifications when resetting the port
> Date: Tue, 23 Nov 2021 08:48:50 +0000
> From: Marc Zyngier <maz@kernel.org>
>
> Luca,
>
> On Mon, 22 Nov 2021 21:32:15 +0000,
> Luca Ceresoli <luca@lucaceresoli.net> wrote:
> >
> > >> Just one comment. PERST# (PCIe Reset) is active-low signal. De-asserting
> > >> means to really set value to 1.
> > >>
> > >> But there was a discussion that de-asserting should be done by call:
> > >> gpiod_set_value(reset, 0);
> > >>
> > >> https://lore.kernel.org/linux-pci/51be082a-ff10-8a19-5648-f279aabcac51@lucaceresoli.net/
> > >>
> > >> Could we make this new pcie-apple.c driver to use gpiod_set_value(reset, 0)
> > >> for de-asserting, like in other drivers?
> >
> > I agree it should be done right from the beginning since this is a new
> > driver. Fixing it later is a painful process.
>
> No more painful than anything else. At this stage, using a positive or
> negative polarity is immaterial, as there is no core infrastructure
> making any use of this behaviour (every single driver must reinvent
> its own square wheel). If such an infrastructure existed, that'd
> indeed be a requirement. For now, this is merely a convention.
>
> > > I guess it depends whether you care about the assertion or the signal
> > > itself. I think we may have a bug in the way the GPIOs are handled at
> > > the moment, as it makes no difference whether I register the GPIO are
> > > active high or active low...
> > >
> > > I guess that will be yet another thing to debug, but in the meantime
> > > we have a reliable reset.
> >
> > Strange, in my case the "active low" pin polarity is correctly picked up
> > from device tree by the gpiolib code, thus using gpio_set_value(gpiod,
> > 1) asserts the pin as it should, resulting in an electrically low pin.
>
> As I said, this looks like a bug, probably in the M1 DT. I'll try to
> look into it when I get the time.

So the diff below is what the changes look like for U-Boot. The
U-Boot Apple PCIe driver has not been submitted upstream yet, so
making this change is no problem.



diff --git a/arch/arm/dts/t8103-j274.dts b/arch/arm/dts/t8103-j274.dts
index aef1ae29b6..3777337033 100644
--- a/arch/arm/dts/t8103-j274.dts
+++ b/arch/arm/dts/t8103-j274.dts
@@ -65,7 +65,7 @@
device_type = "pci";
reg = <0x0 0x0 0x0 0x0 0x0>;
pwren-gpios = <&smc 13 0>;
- reset-gpios = <&pinctrl_ap 152 0>;
+ reset-gpios = <&pinctrl_ap 152 GPIO_ACTIVE_LOW>;
max-link-speed = <2>;

#address-cells = <3>;
@@ -76,7 +76,7 @@
pci1: pci@1,0 {
device_type = "pci";
reg = <0x800 0x0 0x0 0x0 0x0>;
- reset-gpios = <&pinctrl_ap 153 0>;
+ reset-gpios = <&pinctrl_ap 153 GPIO_ACTIVE_LOW>;
max-link-speed = <2>;

#address-cells = <3>;
@@ -87,7 +87,7 @@
pci2: pci@2,0 {
device_type = "pci";
reg = <0x1000 0x0 0x0 0x0 0x0>;
- reset-gpios = <&pinctrl_ap 33 0>;
+ reset-gpios = <&pinctrl_ap 33 GPIO_ACTIVE_LOW>;
max-link-speed = <1>;

#address-cells = <3>;
diff --git a/drivers/pci/pcie_apple.c b/drivers/pci/pcie_apple.c
index bef6043adb..89eec70d81 100644
--- a/drivers/pci/pcie_apple.c
+++ b/drivers/pci/pcie_apple.c
@@ -210,7 +210,7 @@ static int apple_pcie_setup_port(struct apple_pcie_priv *pcie, unsigned idx)
return 0;

dm_gpio_set_dir_flags(&pcie->perstn[idx], GPIOD_IS_OUT);
- dm_gpio_set_value(&pcie->perstn[idx], 0);
+ dm_gpio_set_value(&pcie->perstn[idx], 1);

rmwl(0, PORT_APPCLK_EN, pcie->base_port[idx] + PORT_APPCLK);

@@ -221,7 +221,7 @@ static int apple_pcie_setup_port(struct apple_pcie_priv *pcie, unsigned idx)
apple_pcie_port_pwren(pcie, idx);

rmwl(0, PORT_PERST_OFF, pcie->base_port[idx] + PORT_PERST);
- dm_gpio_set_value(&pcie->perstn[idx], 1);
+ dm_gpio_set_value(&pcie->perstn[idx], 0);

res = readl_poll_timeout(pcie->base_port[idx] + PORT_STATUS, stat, (stat & PORT_STATUS_READY), 100, 250000);
if (res < 0) {
\
 
 \ /
  Last update: 2021-11-23 13:28    [W:1.805 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site