Re: pci-exynos.c phy_init() usage

[Marek Szyprowski <m.szyprowski@samsung.com>]

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