[PATCH 1/2] gpio: mvebu: implement get_direction

[PATCH 1/2] gpio: mvebu: implement get_direction

[Baruch Siach]

struct gpio_chip documentation recommends to always implement this
callback function.

A more concrete motivation is to be able (in combination with
GPIOD_ASIS) to detect whether the bootloader has changed the state of a
GPIO signal.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 drivers/gpio/gpio-mvebu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 7d5c55494ccd..f97ed32b8beb 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -376,6 +376,16 @@ static int mvebu_gpio_direction_output(struct gpio_chip *chip, unsigned int pin,
 	return 0;
 }
 
+static int mvebu_gpio_get_direction(struct gpio_chip *chip, unsigned int pin)
+{
+	struct mvebu_gpio_chip *mvchip = gpiochip_get_data(chip);
+	u32 u;
+
+	regmap_read(mvchip->regs, GPIO_IO_CONF_OFF + mvchip->offset, &u);
+
+	return !!(u & BIT(pin));
+}
+
 static int mvebu_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
 {
 	struct mvebu_gpio_chip *mvchip = gpiochip_get_data(chip);
@@ -1130,6 +1140,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	mvchip->chip.parent = &pdev->dev;
 	mvchip->chip.request = gpiochip_generic_request;
 	mvchip->chip.free = gpiochip_generic_free;
+	mvchip->chip.get_direction = mvebu_gpio_get_direction;
 	mvchip->chip.direction_input = mvebu_gpio_direction_input;
 	mvchip->chip.get = mvebu_gpio_get;
 	mvchip->chip.direction_output = mvebu_gpio_direction_output;
-- 
2.20.1

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

[Baruch Siach]

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

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

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

[Andrew Lunn]

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

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

[Baruch Siach]

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 -

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

[Andrew Lunn]

> 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

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

[Baruch Siach]

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

-- 
     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 -

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

[Baruch Siach]

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 -

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

[Andrew Lunn]

> 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

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

[Baruch Siach]

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 -

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

[Linus Walleij]

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

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

[Baruch Siach]

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

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 1/2] gpio: mvebu: implement get_direction

[Linus Walleij]

On Thu, Jan 10, 2019 at 1:27 PM Baruch Siach <baruch@tkos.co.il> wrote:

> struct gpio_chip documentation recommends to always implement this
> callback function.
>
> A more concrete motivation is to be able (in combination with
> GPIOD_ASIS) to detect whether the bootloader has changed the state of a
> GPIO signal.
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

This is strongly recommended for all drivers to implment so patch
applied.

Yours,
Linus Walleij