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
