Re: [PATCH v4 2/6] PCI: spacemit-k1: Add multiple PHY handles support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Jul 09, 2026 at 12:00:22PM +0800, Inochi Amaoto wrote:
> The PCIe controller on Spacemit K3 may use multiple PHYs at the
> same time. The feature is not support by the current driver.
> So extend the PHY definition to support multiple PHY handles.

...

>  struct k1_pcie {
>  	struct dw_pcie pci;
>  	const struct k1_pcie_device_data *data;
> -	struct phy *phy;
> +	struct phy **phy;

Should it be annotated by __counted_by_ptr() ?

> +	unsigned int phy_count;

Ah, you allocate much more memory than possible PHYs... Can you redesign and
use the above annotation?

>  	void __iomem *link;
>  	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
>  	u32 pmu_off;

>  }

...

> +static int k1_pcie_get_phy_handle(struct k1_pcie *k1, struct device_node *node)
> +{
> +	const struct k1_pcie_device_data *data = k1->data;
> +	struct device *dev = k1->pci.dev;
> +	unsigned int i;
> +
> +	k1->phy = devm_kmalloc_array(dev, data->max_phy_count,
> +				     sizeof(*k1->phy), GFP_KERNEL);
> +	if (!k1->phy)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < data->max_phy_count; i++) {
> +		k1->phy[i] = devm_of_phy_get_by_index(dev, node, i);

> +		if (IS_ERR(k1->phy[i])) {
> +			if (PTR_ERR(k1->phy[i]) == -ENODEV)
> +				break;
> +
> +			return PTR_ERR(k1->phy[i]);
> +		}

		if (PTR_ERR(k1->phy[i]) == -ENODEV)
			break;
		if (IS_ERR(k1->phy[i]))
			return PTR_ERR(k1->phy[i]);


> +	}

> +	k1->phy_count = i;
> +	if (k1->phy_count == 0)
> +		return -EINVAL;
> +
> +	return 0;

This doesn't seem correct to me, I would expect phy_count to be assigned only
when it's valid. (Yes, perhaps 0 is the same as it was, but semantically it's
different 0 in this case.)

See also above. Do we have some PHY API that just counts provided PHYs?
If not, that what you should probably add first, before this patch.

> +}
> +
> +static int k1_pcie_enable_phy(struct k1_pcie *k1)
> +{
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < k1->phy_count; i++) {
> +		ret = phy_init(k1->phy[i]);
> +		if (ret)
> +			goto err_phy;
> +	}
> +
> +	return 0;
> +
> +err_phy:
> +	while (i--)
> +		phy_exit(k1->phy[i]);
> +
> +	return ret;
> +}

...

> static void k1_pcie_deinit(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct k1_pcie *k1 = to_k1_pcie(pci);

> +	int i;
>  
>  	/* Assert fundamental reset (drive PERST# low) */
>  	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>  			PCIE_RC_PERST);
>  
> -	phy_exit(k1->phy);

> +	for (i = 0; i < k1->phy_count; i++)

	for (unsigned int i = 0; i < k1->phy_count; i++)

> +		phy_exit(k1->phy[i]);
>  
>  	k1_pcie_disable_resources(k1);
>  }

-- 
With Best Regards,
Andy Shevchenko