Re: pci-exynos.c phy_init() usage

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Marek Szyprowski <m.szyprowski@samsung.com>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Jaehoon Chung <jh80.chung@samsung.com>,
	Jingoo Han <jingoohan1@gmail.com>
Cc: "Krzysztof Wilczyński" <kw@linux.com>,
	"Alim Akhtar" <alim.akhtar@samsung.com>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org
Subject: Re: pci-exynos.c phy_init() usage
Date: Mon, 27 Jun 2022 12:47:17 +0200	[thread overview]
Message-ID: <ae8cee69-2ca5-0b27-f6d5-0f9f74871fd8@linaro.org> (raw)
In-Reply-To: <591f696f-b55c-d267-7fcb-74f7fd4a6900@samsung.com>

On 27/06/2022 12:30, Marek Szyprowski wrote:
> Hi,
> 
> On 24.06.2022 20:07, Krzysztof Kozlowski wrote:
>> On 24/06/2022 19:35, Bjorn Helgaas wrote:
>>> In exynos_pcie_host_init() [1], we call:
>>>
>>>    phy_reset(ep->phy);
>>>    phy_power_on(ep->phy);
>>>    phy_init(ep->phy);
>>>
>>> The phy_init() function comment [2] says it must be called before
>>> phy_power_on().  Is exynos doing this backwards?
>> Looks like. I don't have Exynos hardware with a PCI, so cannot
>> test/fix/verify.
>>
>> Luckily for Exynos ;-) it's not alone in this pattern:
>> drivers/net/ethernet/marvell/sky2.c
>> drivers/usb/dwc2/platform.c
> 
> I've checked that on the real hardware. Swapping the order of 
> phy_power_on and phy_init breaks driver operation.
> 
> However pci-exynos is the only driver that uses the phy-exynos-pcie, so 
> we can simply swap the content of the init and power_on in the phy 
> driver to adjust the code to the right order. power_on/init and 
> exit/power_off are also called one after the other in pci-exynos, 
> without any activity between them, so we can also simply move all 
> operation to one pair of the callback, like power_on/off.
> 
> Krzysztof, which solution would you prefer?

I think the real problem is that the Exynos PCIe phy init
(exynos5433_pcie_phy_init) performs parts of power on procedure, so the
code is mixed. Probably also the phy init could not happen earlier due
to gated clocks (ungated in exynos5433_pcie_phy_power_on).

I would prefer to clean it up while ordering init+power_on, so figure
out more or less correct procedure.

You can also look at Artpec-8 PHY - it seems using correct order
(init+reset):
https://lore.kernel.org/all/20220614011616epcms2p7dcaa67c53b7df5802dd7a697e2d472d7@epcms2p7/
Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel