Re: [PATCH v4 2/2] phy: qcom: qmp-pcie: Add PHY register retention support
From: Qiang Yu
Date: Tue Feb 25 2025 - 22:18:54 EST
On 2/25/2025 7:46 PM, Dmitry Baryshkov wrote:
On Tue, Feb 25, 2025 at 06:08:03PM +0800, Qiang Yu wrote:
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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
Signed-off-by: Wenbin Yao <quic_wenbyao@xxxxxxxxxxx>
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.
Failing the training is a random error which can mean a lot, e.g. the
missing voltage rail. An explicit check is beneficial, it helps
developers (and users) to better understand the reason of the failure.
In the struct qmp_phy_cfg, tbls is not a pointer, we can't directly check
if tbls == NULL to determine if the PHY init sequence is available. Can we
add a separate patch to change the definition from
"const struct qmp_phy_cfg_tbls tbls" to
"const struct qmp_phy_cfg_tbls *tbls" like tlbs_rc and tbls_ep, even though
this will affect all platforms?