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

[Qiang Yu]

On Mon, Jul 27, 2026 at 03:56:53PM +0200, Konrad Dybcio wrote:
> 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

I'd prefer to keep the async put here. The generic PHY core does the same
in phy_power_off() (phy_pm_runtime_put() -> pm_runtime_put()), and the
async behaviour is actually what helps the quick init/exit case: the
idle/suspend request queued by pm_runtime_put() gets cancelled by the
following pm_runtime_resume_and_get in rpm_resume() before the worker
runs, so the power domain doesn't actually toggle. With _sync() the exit
would drop the domain immediately and a closely following init would have
to power it back up again.

- Qiang Yu
> 
> [...]
> 
> > +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
>