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