Re: [PATCH v7 2/6] phy: qcom: qmp-pcie: Add QMP PCIe Multi-PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing AHB clock `cfg_ahb_b` (and `rchng_b`) in X8 mode clock list will likely cause bus hangs when polling `port_b` registers.
- [Low] Unused `pipe_clk_fixed` field in `struct qmp_pcie`.
--

--- Patch [2]: [PATCH v7 2/6] phy: qcom: qmp-pcie: Add QMP PCIe Multi-PHY driver ---
commit f6ac710b3219c54fc755a82474f60ba0b32281bb
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

phy: qcom: qmp-pcie: Add QMP PCIe Multi-PHY driver
    
Add a dedicated driver for QMP PCIe PHY hardware blocks that support
multiple link topologies (e.g. x8 or x4+x4). Match data carries a per-mode
cfg table; qmp_pcie_multiphy_probe() reads the current link mode and
instantiates one qmp_pcie per sub-PHY.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c
> new file mode 100644
> index 0000000000000..210c951e997d2
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c
[ ... ]
> +struct qmp_pcie {
> +	struct device *dev;
> +	const struct qmp_phy_cfg *cfg;
> +	void __iomem **base;
> +	struct clk_bulk_data *clks;
> +	struct clk_bulk_data *pipe_clks;
> +	struct reset_control_bulk_data *nocsr_resets;
> +	struct regulator_bulk_data *vregs;
> +	struct device **pd_devs;
> +	struct clk_fixed_rate pipe_clk_fixed;

[Severity: Low]
Is this pipe_clk_fixed field used anywhere?

It appears the driver dynamically allocates an array of struct clk_fixed_rate
in qmp_data->pipe_out_clks instead, so this might be an unused leftover from
the single-PHY driver.

> +};
[ ... ]
> +static const char * const glymur_pciephy_clk_l[] = {
> +	"aux", "cfg_ahb", "ref", "rchng", "phy_b_aux",
> +};

[Severity: High]
Does this clock list for X8 mode need to include cfg_ahb_b and rchng_b?

In X8 mode, glymur_qmp_gen5x8_pciephy_cfg sets num_regs = 2 and includes both
"port_a" and "port_b". When qmp_pcie_power_on() runs, it iterates over both
sub-PHYs:

	for (i = 0; i < cfg->num_regs; i++) {
		status = qmp->base[i] + offs->pcs + cfg->regs[QPHY_PCS_STATUS];
		ret = readl_poll_timeout(status, val, !(val & cfg->phy_status), 200,

If cfg_ahb_b is not enabled, reading port_b's PCS status register at
qmp->base[1] could cause an unclocked MMIO access, which typically leads to a
synchronous external abort on these platforms.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-glymur_linkmode_0724-v7-0-65c8469c6b65@oss.qualcomm.com?part=2

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