Re: [PATCH v4 6/6] PCI: dwc: rcar-gen4: Add support for R-Car X5H PCIe4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect PHY power-on sequencing and unchecked return value in rcar_gen5_pcie_ltssm_control().
- [High] The driver fails to configure the dw-edma core for HDMA on Gen5, breaking the commit's claim of HDMA support.
- [Medium] Missing assertion of DW_PCIE_PWR_RST on the error path of rcar_gen5_pcie_init() leaks hardware reset state.
--

commit 33f1bd3dbda597464a41cce7e19a3d761ff8b7c8
Author: Marek Vasut <marek.vasut+renesas@mailbox.org>

PCI: dwc: rcar-gen4: Add support for R-Car X5H PCIe4

This patch adds support for the R8A78000 (R-Car X5H) PCIe4 controller,
differentiating it with the "renesas,rcar-gen5-pcie4" compatible string.
It implements Gen5-specific initialization, deinitialization, link speed,
and LTSSM control callbacks, and handles a separate PHY driver instance.

> The R-Car X5H PCIe4 controller embeds HDMA instead of EDMA embedded
> in the R-Car Gen4 PCIe controller, "dw-edma" driver supports both
> DMA variants.

[Severity: High]
Does the driver actually configure the dw-edma core for HDMA on Gen5 hardware?

Looking at rcar_gen4_pcie_alloc() elsewhere in the same file, the DMA mode
appears to be unconditionally hardcoded:

rcar_gen4_pcie_alloc() {
    ...
    rcar->dw.edma.mf = EDMA_MF_EDMA_UNROLL;
    ...
}

Since this mapping format is never updated for Gen5 controllers (e.g., to
EDMA_MF_HDMA_NATIVE or EDMA_MF_HDMA_COMPAT), will the dw-edma driver attempt
to program EDMA registers and descriptors instead of HDMA, causing DMA
operations to fail on Gen5?

> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
[ ... ]
> @@ -285,6 +322,49 @@ static int rcar_gen4_v4h_v4m_pcie_init(struct rcar_gen4_pcie *rcar)
[ ... ]
> +static int rcar_gen5_pcie_init(struct rcar_gen4_pcie *rcar)
> +{
[ ... ]
> +	/* R-Car Gen4 and Gen5 common initialization. */
> +	ret = rcar_gen4_pcie_common_init(rcar);
> +	if (ret)
> +		return ret;
> +
> +	/* R-Car Gen5 specific additional initialization. */
> +	ret = phy_init(rcar->phy);
> +	if (ret)
> +		goto err_unprepare;
[ ... ]
> +err_unprepare:
> +	clk_bulk_disable_unprepare(DW_PCIE_NUM_CORE_CLKS, dw->core_clks);
> +
> +	return ret;
> +}

[Severity: Medium]
Does this error path leak the hardware reset state?

rcar_gen4_pcie_common_init() successfully deasserts the DW_PCIE_PWR_RST
reset control before returning. If phy_init() subsequently fails, the
code jumps to err_unprepare, which disables the core clocks but leaves
the reset deasserted.

Should this error path also include a call to reset_control_assert() to
balance the state?

[ ... ]
> @@ -807,6 +897,28 @@ static int r8a779f0_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
[ ... ]
> +static int rcar_gen5_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable)
> +{
> +	u32 val;
> +
> +	val = readl(rcar->base + PCIERSTCTRL1);
> +	if (enable) {
> +		val |= APP_LTSSM_ENABLE;
> +		val &= ~APP_HOLD_PHY_RST;
> +	} else {
> +		val &= ~APP_LTSSM_ENABLE;
> +		val |= APP_HOLD_PHY_RST;
> +	}
> +	writel(val, rcar->base + PCIERSTCTRL1);
> +
> +	if (enable)
> +		phy_power_on(rcar->phy);
> +	else
> +		phy_power_off(rcar->phy);
> +
> +	return 0;
> +}

[Severity: High]
Does this sequence start link training before the PHY is properly powered
on and stabilized?

By writing APP_LTSSM_ENABLE to PCIERSTCTRL1 before calling phy_power_on(),
the controller might attempt training with an unpowered PHY.

Also, is it safe to ignore the return value of phy_power_on() here? If the
PHY fails to power on, the function still returns 0, leaving the controller
in a broken state with an unpowered PHY.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905212649.360498-1-marek.vasut+renesas@mailbox.org?part=6