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

From: Baruch Siach <baruch@tkos.co.il>
To: Andrew Lunn <andrew@lunn.ch>
Cc: linux-pwm@vger.kernel.org,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Sven Auhagen <sven.auhagen@voleatech.de>,
	Jason Cooper <jason@lakedaemon.net>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	linux-pci@vger.kernel.org,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-gpio@vger.kernel.org,
	Thierry Reding <thierry.reding@gmail.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Gregory Clement <gregory.clement@bootlin.com>,
	linux-arm-kernel@lists.infradead.org,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Subject: Re: [PATCH 2/2] PCI: armada8k: don't toggle reset twice
Date: Sun, 13 Jan 2019 14:38:00 +0200	[thread overview]
Message-ID: <87pnt0vil3.fsf@tarshish> (raw)
In-Reply-To: <87zhs8h5ej.fsf@tarshish>

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 -