Re: [PATCH 2/4] PCI: qcom-ep: Move PHY init and drive PHY reset to align with HPG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pci: qcom-ep: missing PARF_DEVICE_TYPE relocation
--

commit 5b56bf58f9d475d704bc41d4161818703cac582d
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

PCI: qcom-ep: Move PHY init and drive PHY reset to align with HPG

This patch rearranges the PHY initialization and reset sequences to
align with the hardware programming guide (HPG). It moves phy_init()
before the controller clock enable and folds phy_reset() into the
core reset assert/deassert sequence.

> PCI: qcom-ep: Move PHY init and drive PHY reset to align with HPG
> 
> HPG requires the GDSC to be powered up and all PHY/controller clocks
> enabled before the controller's core reset and the PHY's reset are toggled
> together as one atomic group, with PARF_DEVICE_TYPE set right after that
> reset group, same as the RC-mode change.
> 
> Move phy_init() ahead of the controller's own clock enable, and fold
> phy_reset() into the core reset assert/deassert. phy_power_on() keeps
> running afterwards. Also move the PARF_DEVICE_TYPE write out of
> qcom_pcie_perst_deassert() to right after the reset group, in
> qcom_pcie_enable_resources().

[Severity: Medium]
The commit message explicitly states the PARF_DEVICE_TYPE write is being
moved out of qcom_pcie_perst_deassert() to right after the reset group in
qcom_pcie_enable_resources(). However, the diff does not seem to include this
change.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 8364696a1b980..cd7725d277c5d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c

[ ... ]

> @@ -332,25 +338,25 @@ static int qcom_pcie_enable_resources(struct qcom_pcie_ep *pcie_ep)
>  	struct dw_pcie *pci = &pcie_ep->pci;
>  	int ret;
>  
> -	ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
> +	ret = phy_init(pcie_ep->phy);
>  	if (ret)
>  		return ret;
>  
> -	ret = qcom_pcie_ep_core_reset(pcie_ep);
> +	ret = clk_bulk_prepare_enable(pcie_ep->num_clks, pcie_ep->clks);
>  	if (ret)
> -		goto err_disable_clk;
> +		goto err_phy_exit;
>  
> -	ret = phy_init(pcie_ep->phy);
> +	ret = qcom_pcie_ep_phy_core_reset(pcie_ep);
>  	if (ret)
>  		goto err_disable_clk;
>  
>  	ret = phy_set_mode_ext(pcie_ep->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_EP);

[Severity: Medium]
Was the write to PARF_DEVICE_TYPE accidentally omitted from this patch?

Based on the commit message and HPG requirements, it looks like a write to
PARF_DEVICE_TYPE was intended to be inserted right here in
qcom_pcie_enable_resources().

Looking at the code in qcom_pcie_perst_deassert(), the write remains
untouched:

drivers/pci/controller/dwc/pcie-qcom-ep.c:qcom_pcie_perst_deassert() {
	...
	/* Configure PCIe to endpoint mode */
	writel_relaxed(PARF_DEVICE_TYPE_EP, pcie_ep->parf + PARF_DEVICE_TYPE);
	...
}

Could this mismatch between the HPG sequence and the actual implemented code
cause endpoint initialization failures when the hardware requires the device
type to be set immediately after reset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-align_pcie_init_sequence_0916-v1-0-0b2195836f30@oss.qualcomm.com?part=2