> Date: Tue, 23 Nov 2021 08:48:50 +0000 > From: Marc Zyngier <maz@xxxxxxxxxx> > > Luca, > > On Mon, 22 Nov 2021 21:32:15 +0000, > Luca Ceresoli <luca@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx/ > > >> > > >> 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) {
