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

[Konrad Dybcio]

On 7/24/26 12:01 PM, Qiang Yu wrote:
> Some QMP PCIe PHY hardware blocks support multiple link topologies (e.g.
> x8 or x4+x4) selected via a TCSR register. The existing single-instance
> QMP PCIe PHY driver has no way to model this: it assumes a single cfg per
> DT node and instantiates exactly one PHY.
> 
> Add a dedicated driver for this class of PHY. Match data carries a
> per-mode cfg table; qmp_pcie_multiphy_probe() reads the current link
> mode from the TCSR register pointed to by "qcom,link-mode", looks up the
> corresponding cfg array, and instantiates one qmp_pcie per sub-PHY
> required by that link mode, registering the clock and #phy-cells = <1> phy
> providers so consumers can address individual sub-PHYs by index.
> 
> The driver inherits the phy setting and link-mode programmed by firmware,
> so only the no_csr reset is used and no phy setting tables are provided.
> 
> Add the first match data and compatible, qcom,glymur-qmp-gen5x8-pcie-phy,
> for the Glymur Gen5 PCIe PHY that can bifurcate into two x4 links or
> operate as a single x8 link.
> 
> Reviewed-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Signed-off-by: Qiang Yu <qiang.yu@xxxxxxxxxxxxxxxx>
> ---

This looks really good, couple of follow-up improvement suggestions

[...]

> +static const char * const glymur_pciephy_a_reg_l[] = {
> +	"port_a",
> +};
> +
> +static const char * const glymur_pciephy_b_reg_l[] = {
> +	"port_b",
> +};
> +
> +static const char * const glymur_pciephy_reg_l[] = {
> +	"port_a", "port_b",
> +};

I think we could construct these names at the call sites, with
essentially something like

for (i = 0; i < qmp->num_ports; i++) {
	sprintf(buf, "port_%c\n", 'a' + i);
}

(is that too cheesy?)

[...]

> +err_power_off:
> +	while (--i >= 0)
> +		pm_runtime_put(qmp->pd_devs[i]);

This probably can stay non-sync

> +
> +	return ret;
> +}
> +
> +static void qmp_pcie_pd_power_off(struct qmp_pcie *qmp)
> +{
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	int i;
> +
> +	for (i = cfg->num_pds - 1; i >= 0; i--)
> +		pm_runtime_put(qmp->pd_devs[i]);

This should probably be _sync() as phy init/exit may be called in
quick succession

[...]

> +static int __phy_pipe_clk_register(struct device *dev, struct device_node *np,
> +				   int idx, struct clk_fixed_rate *fixed)
> +{
> +	struct clk_init_data init = { };
> +	int ret;
> +
> +	ret = of_property_read_string_index(np, "clock-output-names", idx,
> +					    &init.name);
> +	if (ret) {
> +		dev_err(dev, "%pOFn: No clock-output-names\n", np);
> +		return ret;
> +	}
> +
> +	init.ops = &clk_fixed_rate_ops;
> +
> +	if (!fixed->fixed_rate)
> +		fixed->fixed_rate = 125000000;

I think this always happens to be true in this driver

[...]

> +	num_pipe_outputs = of_property_count_strings(np, "clock-output-names");
> +	if (num_pipe_outputs < 0)
> +		num_pipe_outputs = 1;

This can't happen (clock-output-names is required via bindings)

[...]

> +static int qmp_pcie_read_link_mode(struct device *dev, unsigned int *link_mode)
> +{
> +	struct regmap *map;
> +	unsigned int args[1];
> +	int ret;
> +
> +	map = syscon_regmap_lookup_by_phandle_args(dev->of_node, "qcom,link-mode",
> +						   ARRAY_SIZE(args), args);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	ret = regmap_read(map, args[0], link_mode);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

ret = xxx()
if (ret)
	return ret;

return 0;

can be collapsed to simply return regmap_read()

with or without all that,

Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>

Konrad