* 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).