* pci-exynos.c phy_init() usage @ 2022-06-24 17:35 Bjorn Helgaas 2022-06-24 18:07 ` Krzysztof Kozlowski 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2022-06-24 17:35 UTC (permalink / raw) To: Jaehoon Chung, Jingoo Han Cc: Krzysztof Kozlowski, Krzysztof Wilczyński, Alim Akhtar, linux-pci, linux-arm-kernel, linux-samsung-soc 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? Bjorn [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-exynos.c?id=v5.19-rc1#n252 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/phy-core.c?id=v5.19-rc1#n233 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pci-exynos.c phy_init() usage 2022-06-24 17:35 pci-exynos.c phy_init() usage Bjorn Helgaas @ 2022-06-24 18:07 ` Krzysztof Kozlowski 2022-06-27 10:30 ` Marek Szyprowski 0 siblings, 1 reply; 7+ messages in thread From: Krzysztof Kozlowski @ 2022-06-24 18:07 UTC (permalink / raw) To: Bjorn Helgaas, Jaehoon Chung, Jingoo Han Cc: Krzysztof Wilczyński, Alim Akhtar, linux-pci, linux-arm-kernel, linux-samsung-soc 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 > > Bjorn > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-exynos.c?id=v5.19-rc1#n252 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/phy-core.c?id=v5.19-rc1#n233 Best regards, Krzysztof ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pci-exynos.c phy_init() usage 2022-06-24 18:07 ` Krzysztof Kozlowski @ 2022-06-27 10:30 ` Marek Szyprowski 2022-06-27 10:47 ` Krzysztof Kozlowski 0 siblings, 1 reply; 7+ messages in thread From: Marek Szyprowski @ 2022-06-27 10:30 UTC (permalink / raw) To: Krzysztof Kozlowski, Bjorn Helgaas, Jaehoon Chung, Jingoo Han Cc: Krzysztof Wilczyński, Alim Akhtar, linux-pci, linux-arm-kernel, linux-samsung-soc 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? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pci-exynos.c phy_init() usage 2022-06-27 10:30 ` Marek Szyprowski @ 2022-06-27 10:47 ` Krzysztof Kozlowski 2022-06-28 8:13 ` Marek Szyprowski 0 siblings, 1 reply; 7+ messages in thread From: Krzysztof Kozlowski @ 2022-06-27 10:47 UTC (permalink / raw) To: Marek Szyprowski, Bjorn Helgaas, Jaehoon Chung, Jingoo Han Cc: Krzysztof Wilczyński, Alim Akhtar, linux-pci, linux-arm-kernel, linux-samsung-soc 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pci-exynos.c phy_init() usage 2022-06-27 10:47 ` Krzysztof Kozlowski @ 2022-06-28 8:13 ` Marek Szyprowski 2022-06-28 8:27 ` Krzysztof Kozlowski 0 siblings, 1 reply; 7+ messages in thread From: Marek Szyprowski @ 2022-06-28 8:13 UTC (permalink / raw) To: Krzysztof Kozlowski, Bjorn Helgaas, Jaehoon Chung, Jingoo Han Cc: Krzysztof Wilczyński, Alim Akhtar, linux-pci, linux-arm-kernel, linux-samsung-soc Hi Krzysztof, 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. While touching this - I would also remove the phy_reset() call in the exynos-pcie driver. It is a left over from the old, obsoleted exynos5440 pcie code, not implemented in the current phy driver, also only a few drivers use or implement it. IMHO it doesn't make sense to keep such dead code. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pci-exynos.c phy_init() usage 2022-06-28 8:13 ` Marek Szyprowski @ 2022-06-28 8:27 ` Krzysztof Kozlowski 2022-06-28 10:59 ` Bjorn Helgaas 0 siblings, 1 reply; 7+ messages in thread From: Krzysztof Kozlowski @ 2022-06-28 8:27 UTC (permalink / raw) To: Marek Szyprowski, Bjorn Helgaas, Jaehoon Chung, Jingoo Han Cc: Krzysztof Wilczyński, Alim Akhtar, linux-pci, linux-arm-kernel, linux-samsung-soc On 28/06/2022 10:13, Marek Szyprowski wrote: > Hi Krzysztof, > > 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. > > While touching this - I would also remove the phy_reset() call in the > exynos-pcie driver. It is a left over from the old, obsoleted exynos5440 > pcie code, not implemented in the current phy driver, also only a few > drivers use or implement it. IMHO it doesn't make sense to keep such > dead code. Sure, looks ok. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: pci-exynos.c phy_init() usage 2022-06-28 8:27 ` Krzysztof Kozlowski @ 2022-06-28 10:59 ` Bjorn Helgaas 0 siblings, 0 replies; 7+ messages in thread From: Bjorn Helgaas @ 2022-06-28 10:59 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Marek Szyprowski, Jaehoon Chung, Jingoo Han, Krzysztof Wilczyński, Alim Akhtar, linux-pci, linux-arm-kernel, linux-samsung-soc 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-06-28 10:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-24 17:35 pci-exynos.c phy_init() usage Bjorn Helgaas 2022-06-24 18:07 ` Krzysztof Kozlowski 2022-06-27 10:30 ` Marek Szyprowski 2022-06-27 10:47 ` Krzysztof Kozlowski 2022-06-28 8:13 ` Marek Szyprowski 2022-06-28 8:27 ` Krzysztof Kozlowski 2022-06-28 10:59 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).