On Tue, Jun 28, 2022 at 10:27:31AM +0200, Krzysztof Kozlowski wrote: > On 28/06/2022 10:13, Marek Szyprowski wrote: > > On 27.06.2022 12:47, Krzysztof Kozlowski wrote: > >> On 27/06/2022 12:30, Marek Szyprowski wrote: > >>> 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/ > > > > I've played a bit with those register writes in exynos_pcie_phy and > > frankly speaking the currenly used (power_on + init) is the only > > sequence that works properly. I'm leaning to move everything to > > phy_init/exit. I really don't see how to split it into init + power_on > > callbacks. > > I was afraid it will be like this. I imagine that certain (not > explicitly documented) init operations cannot even happen before power > on, so this would be a lot of tries. > > I am fine with it. Thanks for doing it. If nothing can be improved, a comment to this effect might make it look less like a mistake. Bjorn
