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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Feb 26, 2025 at 07:19:11AM +0200, Dmitry Baryshkov wrote:
> On Wed, Feb 26, 2025 at 11:12:18AM +0800, Qiang Yu wrote:
> > 
> > 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@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.
> > > 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?
> 
> Of course no. There is no need to introduce extra indirection. Checking
> for qmp_phy_cfg.tbls.serdes_num should be more than enough. No matter
> what, the PHY with a proper configuration tables will have non-empty
> SERDES table.
> 

+1. The check needs to be present in this series itself and it makes absolute
sense to have it. Otherwise, it will become a hard to debug issue.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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