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

[Inochi Amaoto <inochiama@gmail.com>]

On Thu, Jul 09, 2026 at 10:16:28AM +0300, Andy Shevchenko wrote:
> 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() ?
> 

Yes, I think it can, this is something I have missed.

> > +	unsigned int phy_count;
> 
> Ah, you allocate much more memory than possible PHYs... Can you redesign and
> use the above annotation?
> 

IIRC use the annotation does not reduce this memory usage...


> >  	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]);
> 

Yeah, this is more clear. Thanks.

> 
> > +	}
> 
> > +	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.)
> 

I guess you think 0 is a valid number? I can not understand what you thing
Assign this to 0 if there is no phy is fine to me, which shows there is 0
vaild phy found.

> 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.
> 

I have not found any api for this. But the actual problem is, how the api
is designed. I have checked both the array bulk api for reset and clock,
it seems like it is much more than 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++)
> 

I agree with the unsigned int, but I guess this definition is not
allowed in linux.

> > +		phy_exit(k1->phy[i]);
> >  
> >  	k1_pcie_disable_resources(k1);
> >  }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Regards,
Inochi