diff mbox series

[2/2] PCI: armada8k: don't toggle reset twice

Message ID 94cd23a60c647020dd87a923684b59255b89f02c.1547123182.git.baruch@tkos.co.il (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series [1/2] gpio: mvebu: implement get_direction | expand

Commit Message

Baruch Siach Jan. 10, 2019, 12:26 p.m. UTC
Commit 3d71746c42 ("PCI: armada8k: Add support for gpio controlled reset
signal") added reset signal support. Reset is unconditionally asserted
and deasserted.

Unfortunately, that commit breaks boot on Macchiatobin when a Mellanox
NIC is in the PCIe slot.

It turns out that you can toggle the reset signal only once. Another
reset signal toggle makes access to PCI configuration registers stall
indefinitely. U-Boot toggles the Macchiatobin PCIe reset line already at
boot.

Detect whether the bootloader changed the reset signal state using the
get_direction operation. If direction is output don't touch the reset
signal again. Otherwise, set direction to output and keep the reset
asserted.

Reported-by: Sven Auhagen <sven.auhagen@voleatech.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---

This patch depends on the mvebu get_direction implementation in the
patch #1 of this series. Since get_direction implementation is not a
pure fix it might be considered unfit for v5.0. In that case I'm OK with
reverting 3d71746c42 in v5.0 to fix the Macchiatobin regression, and
postpone this series to v5.1.
---
 drivers/pci/controller/dwc/pcie-armada8k.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Andrew Lunn Jan. 10, 2019, 12:55 p.m. UTC | #1
On Thu, Jan 10, 2019 at 02:26:22PM +0200, Baruch Siach wrote:
> Commit 3d71746c42 ("PCI: armada8k: Add support for gpio controlled reset
> signal") added reset signal support. Reset is unconditionally asserted
> and deasserted.
> 
> Unfortunately, that commit breaks boot on Macchiatobin when a Mellanox
> NIC is in the PCIe slot.
> 
> It turns out that you can toggle the reset signal only once. Another
> reset signal toggle makes access to PCI configuration registers stall
> indefinitely. U-Boot toggles the Macchiatobin PCIe reset line already at
> boot.

Hi Baruch

Is this double reset issue limited to just Mellanox NICs, or any
device in the PCIe slot?

This sounds more like a workaround than a fix. It would be good to
investigate further and determine which end of the PCIe link has the
problem.

Thanks
	Andrew
Baruch Siach Jan. 10, 2019, 1:05 p.m. UTC | #2
Hi Andrew,

On Thu, Jan 10 2019, Andrew Lunn wrote:
> On Thu, Jan 10, 2019 at 02:26:22PM +0200, Baruch Siach wrote:
>> Commit 3d71746c42 ("PCI: armada8k: Add support for gpio controlled reset
>> signal") added reset signal support. Reset is unconditionally asserted
>> and deasserted.
>>
>> Unfortunately, that commit breaks boot on Macchiatobin when a Mellanox
>> NIC is in the PCIe slot.
>>
>> It turns out that you can toggle the reset signal only once. Another
>> reset signal toggle makes access to PCI configuration registers stall
>> indefinitely. U-Boot toggles the Macchiatobin PCIe reset line already at
>> boot.
>
> Hi Baruch
>
> Is this double reset issue limited to just Mellanox NICs, or any
> device in the PCIe slot?
>
> This sounds more like a workaround than a fix. It would be good to
> investigate further and determine which end of the PCIe link has the
> problem.

Sven Auhagen reported the same issue with Intel NIC attached to
mini-PCIe slots on a custom Armada 8K design.

How would you suggest to investigate this issue?

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Andrew Lunn Jan. 10, 2019, 1:19 p.m. UTC | #3
> Sven Auhagen reported the same issue with Intel NIC attached to
> mini-PCIe slots on a custom Armada 8K design.

O.K. so that suggests the issue is on the Armada side.
 
> How would you suggest to investigate this issue?

I presume reboot works O.K? So it is possible to toggle the reset
multiple times, but reboot must do something additional which makes it
work? Do you have sources for the bootloader?

Are there status bits in the comphy about the state of the link? Maybe
comphy needs to be kicked to reestablish the link?

       Andrew
Baruch Siach Jan. 10, 2019, 3:57 p.m. UTC | #4
Hi Andrew,

On Thu, Jan 10 2019, Andrew Lunn wrote:
>> Sven Auhagen reported the same issue with Intel NIC attached to
>> mini-PCIe slots on a custom Armada 8K design.
>
> O.K. so that suggests the issue is on the Armada side.
>  
>> How would you suggest to investigate this issue?
>
> I presume reboot works O.K? So it is possible to toggle the reset
> multiple times, but reboot must do something additional which makes it
> work? Do you have sources for the bootloader?

The bootloader is current U-Boot master as BL33 of Marvell provided ATF
version 18.12, current latest.

> Are there status bits in the comphy about the state of the link? Maybe
> comphy needs to be kicked to reestablish the link?

Maybe. The U-Boot comphy PCIe initialization routine
comphy_pcie_power_up() is long and complex.

The ATF code also carries PCIe comphy initialization with this text:

https://github.com/MarvellEmbeddedProcessors/atf-marvell/blob/atf-v1.5-armada-18.12/drivers/marvell/comphy/phy-comphy-cp110.c#L1170

        /* In Armada 8K DB boards, PCIe initialization can be executed
         * only once (PCIe reset performed during chip power on and
         * it cannot be executed via GPIO later).
         * This means that power on can be executed only once, so let's
         * mark if the caller is bootloader or Linux.
         * If bootloader -> run power on.
         * If Linux -> exit.
         *
         * TODO: In MacciatoBIN, PCIe reset is connected via GPIO,
         * so after GPIO reset is added to Linux Kernel, it can be
         * powered-on by Linux.
         */
        if (!called_from_uboot)
                return ret;

It looks like vendor code is using a similar trick.

baruch
Baruch Siach Jan. 13, 2019, 12:38 p.m. UTC | #5
Hi Andrew,

On Thu, Jan 10 2019, Baruch Siach wrote:
> On Thu, Jan 10 2019, Andrew Lunn wrote:
>>> Sven Auhagen reported the same issue with Intel NIC attached to
>>> mini-PCIe slots on a custom Armada 8K design.
>>
>> O.K. so that suggests the issue is on the Armada side.
>>
>>> How would you suggest to investigate this issue?
>>
>> I presume reboot works O.K? So it is possible to toggle the reset
>> multiple times, but reboot must do something additional which makes it
>> work? Do you have sources for the bootloader?
>
> The bootloader is current U-Boot master as BL33 of Marvell provided ATF
> version 18.12, current latest.
>
>> Are there status bits in the comphy about the state of the link? Maybe
>> comphy needs to be kicked to reestablish the link?
>
> Maybe. The U-Boot comphy PCIe initialization routine
> comphy_pcie_power_up() is long and complex.
>
> The ATF code also carries PCIe comphy initialization with this text:
>
> https://github.com/MarvellEmbeddedProcessors/atf-marvell/blob/atf-v1.5-armada-18.12/drivers/marvell/comphy/phy-comphy-cp110.c#L1170
>
>         /* In Armada 8K DB boards, PCIe initialization can be executed
>          * only once (PCIe reset performed during chip power on and
>          * it cannot be executed via GPIO later).
>          * This means that power on can be executed only once, so let's
>          * mark if the caller is bootloader or Linux.
>          * If bootloader -> run power on.
>          * If Linux -> exit.
>          *
>          * TODO: In MacciatoBIN, PCIe reset is connected via GPIO,
>          * so after GPIO reset is added to Linux Kernel, it can be
>          * powered-on by Linux.
>          */
>         if (!called_from_uboot)
>                 return ret;

Another look at this comment made me realize that we need comphy
initialization support in the kernel for PCIe reset to work correctly.

The workaround that this patch proposes will not solve the problem for
v5.0, since the GPIO get_direction patch will only appear in v5.1.

So the only viable solution for v5.0 is to revert the gpio reset signal
patch (commit 3d71746c42).

Thanks for your review.

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Andrew Lunn Jan. 13, 2019, 3:40 p.m. UTC | #6
> Another look at this comment made me realize that we need comphy
> initialization support in the kernel for PCIe reset to work correctly.
> 
> The workaround that this patch proposes will not solve the problem for
> v5.0, since the GPIO get_direction patch will only appear in v5.1.

You could ask for it to be applied to v5.0 since a real fix depends on
it.

We have a bit of a balancing act to sort out. The revert will fix
Macchiatobin. But i assume it breaks some other platforms which just
started to work. Ideally we want the best of both worlds. So maybe
getting the GPIO change into stable is the correct thing to do?

	Andrew
Baruch Siach Jan. 13, 2019, 6:42 p.m. UTC | #7
Hi Andrew,

On Sun, Jan 13 2019, Andrew Lunn wrote:
>> Another look at this comment made me realize that we need comphy
>> initialization support in the kernel for PCIe reset to work correctly.
>>
>> The workaround that this patch proposes will not solve the problem for
>> v5.0, since the GPIO get_direction patch will only appear in v5.1.
>
> You could ask for it to be applied to v5.0 since a real fix depends on
> it.
>
> We have a bit of a balancing act to sort out. The revert will fix
> Macchiatobin. But i assume it breaks some other platforms which just
> started to work. Ideally we want the best of both worlds. So maybe
> getting the GPIO change into stable is the correct thing to do?

Support for that other board (Clearfog GT-8K) has only been introduced
in v5.0. The kernel never supported PCIe on that platform. So there is
no regression for CF GT-8K like there is for the Macchiatobin.

The issue is quite easy to deal with at the U-Boot level even for v5.0.
The workaround that this patch suggests is pretty ugly, as you noted. I
think we can live with no PCIe reset support in the kernel for another
release or two.

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Linus Walleij Jan. 13, 2019, 11:35 p.m. UTC | #8
On Sun, Jan 13, 2019 at 4:40 PM Andrew Lunn <andrew@lunn.ch> wrote:

> > Another look at this comment made me realize that we need comphy
> > initialization support in the kernel for PCIe reset to work correctly.
> >
> > The workaround that this patch proposes will not solve the problem for
> > v5.0, since the GPIO get_direction patch will only appear in v5.1.
>
> You could ask for it to be applied to v5.0 since a real fix depends on
> it.

If it makes a piece of hardware work that was working before and
now is not working as a result of other kernel changes I would
definately apply it because that is a clear cut regression.

I am under the impression that this is the case?

Yours,
Linus Walleij
Baruch Siach Jan. 14, 2019, 7:19 a.m. UTC | #9
Hi Linus,

On Mon, Jan 14, 2019 at 12:35:01AM +0100, Linus Walleij wrote:
> On Sun, Jan 13, 2019 at 4:40 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > Another look at this comment made me realize that we need comphy
> > > initialization support in the kernel for PCIe reset to work correctly.
> > >
> > > The workaround that this patch proposes will not solve the problem for
> > > v5.0, since the GPIO get_direction patch will only appear in v5.1.
> >
> > You could ask for it to be applied to v5.0 since a real fix depends on
> > it.
> 
> If it makes a piece of hardware work that was working before and
> now is not working as a result of other kernel changes I would
> definately apply it because that is a clear cut regression.
> 
> I am under the impression that this is the case?

The gpio-mvebu get_direction support enables a workaround for a regression 
introduced in commit 3d71746c420c1 on the Macchiatobin platform. An 
alternative solution is to revert the offending commit. The revert will not 
break any known platform that has been previously supported in mainline 
kernel. I tend to think that revert is a better course of action.

It is up to the PCI maintainers to decide.

baruch
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
index b171b6bc15c8..132a86a1e1e7 100644
--- a/drivers/pci/controller/dwc/pcie-armada8k.c
+++ b/drivers/pci/controller/dwc/pcie-armada8k.c
@@ -257,12 +257,23 @@  static int armada8k_pcie_probe(struct platform_device *pdev)
 		goto fail_clkreg;
 	}
 
-	/* Get reset gpio signal and hold asserted (logically high) */
+	/* Get reset gpio signal, don't change its setting */
 	pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset",
-						   GPIOD_OUT_HIGH);
+						   GPIOD_ASIS);
 	if (IS_ERR(pcie->reset_gpio)) {
 		ret = PTR_ERR(pcie->reset_gpio);
 		goto fail_clkreg;
+	} else if (pcie->reset_gpio &&
+			gpiod_get_direction(pcie->reset_gpio) == 0) {
+		/* Reset signal is output. The bootloader has deasserted reset
+		 * signal already. Don't do it again.
+		 */
+		dev_info(dev, "%s: leave reset signal unchanged\n", __func__);
+		devm_gpiod_put(dev, pcie->reset_gpio);
+		pcie->reset_gpio = NULL;
+	} else if (pcie->reset_gpio) {
+		/* Assert reset */
+		gpiod_direction_output(pcie->reset_gpio, 1);
 	}
 
 	platform_set_drvdata(pdev, pcie);