Re: [PATCH v4 2/2] phy: qcom: qmp-pcie: Add PHY register retention support

[Qiang Yu <quic_qianyu@quicinc.com>]

On 2/25/2025 4:17 PM, Manivannan Sadhasivam wrote:
> On Tue, Feb 25, 2025 at 04:06:16PM +0800, Wenbin Yao (Consultant) wrote:
>> On 2/24/2025 8:24 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Feb 24, 2025 at 12:46:44PM +0100, Konrad Dybcio wrote:
>>>> On 24.02.2025 9:46 AM, Wenbin Yao (Consultant) wrote:
>>>>> On 2/24/2025 3:33 PM, Manivannan Sadhasivam wrote:
>>>>>> On Thu, Feb 20, 2025 at 06:22:53PM +0800, Wenbin Yao wrote:
>>>>>>> From: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>>
>>>>>>> Some QCOM PCIe PHYs support no_csr reset. Unlike BCR reset which resets the
>>>>>>> whole PHY (hardware and register), no_csr reset only resets PHY hardware
>>>>>>> but retains register values, which means PHY setting can be skipped during
>>>>>>> PHY init if PCIe link is enabled in booltloader and only no_csr is toggled
>>>>>>> after that.
>>>>>>>
>>>>>>> Hence, determine whether the PHY has been enabled in bootloader by
>>>>>>> verifying QPHY_START_CTRL register. If it's programmed and no_csr reset is
>>>>>>> available, skip BCR reset and PHY register setting to establish the PCIe
>>>>>>> link with bootloader - programmed PHY settings.
>>>>>>>
>>>>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>>> Signed-off-by: Wenbin Yao <quic_wenbyao@quicinc.com>
>>>>>> Some nitpicks below.
>>>>>>
>>>>>>> ---
>>>> [...]
>>>>
>>>>>>> +     * In this way, no matter whether the PHY settings were initially
>>>>>>> +     * programmed by bootloader or PHY driver itself, we can reuse them
>>>>>> It is really possible to have bootloader not programming the init sequence for
>>>>>> no_csr reset platforms? The comment sounds like it is possible. But I heard the
>>>>>> opposite.
>>>>> PCIe3 on X1E80100 QCP is disabled by default in UEFI. We need to enable it
>>>>> manually in UEFI shell if we want.
>>>> IIUC this will not be a concern going forward, and this is a special case
>>>>
>>> I'm wondering how many *special* cases we may have to deal with going forward.
>>> Anyhow, I would propose to atleast throw an error and fail probe() if:
>>>
>>> * the platform has no_csr reset AND
>>> * bootloader has not initialized the PHY AND
>>> * there are no init sequences in the kernel
>>>
>>> - Mani
>> Hmmm, regardless of whether it's a special case, we can't assume that UEFI
>> will enable the PHY supporting no_csr reset on all platforms. It's a bit
>> risky. If we make such an assumption, we also won't need to check whether
>> the PHY is enabled by UEFI during powering on. We just need to check
>> whether no_csr reset is available.
>>
> I am not supportive of this assumption to be clear. While I am OK with relying
> on no_csr reset and bootloader programming the PHY, we should also make sure to
> catch if the PHY doesn't initialize it. Otherwise, the driver would assume that
> the PHY is working, but the users won't see any PCIe devices.
>
>> But it makes sense to check the exsitence of PHY senquence. How about
>> adding the check in qmp_pcie_init, if a PHY supports no_csr reset and isn't
>> initialized in UEFI and there is no cfg->tbls, return error and print some
>> error log so that the PCIe controller will fail to probe.
>>
> Sounds good to me.
I'm wondering is it necessary to add this check? In current PHY driver,
for PHY that doesn't suppot no_csr reset there is also no such check.
If a PHY supports no_csr reset and isn't init in UEFI and there is no
cfg->tbls, the worst issue is link training fail and PCIe controller will
also fail to probe. Adding sucj check seems not change the result.

Thanks,
Qiang
>
> - Mani
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy