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

[Inochi Amaoto]

On Fri, Jul 10, 2026 at 11:01:23AM -0500, Alex Elder wrote:
> On 7/8/26 11:00 PM, Inochi Amaoto wrote:
> > The PCIe controller on Spacemit K3 may use multiple PHYs at the
> 
> s/use/uses/
> 
> > same time. The feature is not support by the current driver.
> 
> s/support/supported/
> 
> > So extend the PHY definition to support multiple PHY handles.
> > 
> > Signed-off-by: Inochi Amaoto <inochiama@xxxxxxxxx>
> > ---
> >   drivers/pci/controller/dwc/pcie-spacemit-k1.c | 70 ++++++++++++++++---
> >   1 file changed, 59 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> > index f6ae8ff3589a..e22ecbd09579 100644
> > --- a/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> > +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> > @@ -55,12 +55,14 @@ struct k1_pcie_device_data {
> >   	const struct dw_pcie_host_ops *host_ops;
> >   	const struct dw_pcie_ops *ops;
> >   	int (*parse_port)(struct k1_pcie *k1);
> > +	unsigned int max_phy_count;
> 
> Is the name "max_phy_count" meant to suggest that there
> could be fewer "actual" PHYs than the number provided in
> this field?  If not--if it is simply "the number of PHYs
> this platform uses"--then just call this phy_count.
> 

Yes.

> >   };
> >   struct k1_pcie {
> >   	struct dw_pcie pci;
> >   	const struct k1_pcie_device_data *data;
> > -	struct phy *phy;
> > +	struct phy **phy;
> > +	unsigned int phy_count;
> 
> If this is always the same as what's in data->max_phy_count,
> you don't need to replicate the value here (since you're
> also keeping the data pointer in this structure).  (But it
> looks like it might be less than max_phy_count.)
> 
> I believe I suggested making this structure use a flexible
> array member for the PHYs.  If that's possible, it should
> go at the end of the structure, and the way you allocate
> it needs to change.
> 
> >   	void __iomem *link;
> >   	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
> >   	u32 pmu_off;
> > @@ -119,6 +121,54 @@ static void k1_pcie_disable_resources(struct k1_pcie *k1)
> >   	clk_bulk_disable_unprepare(ARRAY_SIZE(pci->app_clks), pci->app_clks);
> >   }
> > +static int k1_pcie_get_phy_handle(struct k1_pcie *k1, struct device_node *node)
> 
> I would call this k1_pci_get_phy_handles() (or perhaps
> just k1_pci_get_phys()).  Or even k1_pci_phy_get_all().
> The name you have seems like you're just getting one handle.
> 

OK, it is good for me.

> > +{
> > +	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);
> 
> Use kzalloc not kmalloc.  Even if you're initializing all fields
> now, a future change might not (and in that case having it zeroed
> is safest).
> 

Thanks.

> Also, if you find fewer than max_phy_count PHYs, I think it
> would be better to only allocate as many needed.  If you
> used a flexible array size, you would need to count the
> number of entries before allocating it.  It would require
> changing the structure a bit--providing a single function
> that would allocate the k1_pcie structure after doing that,
> and most likely initializing the phy array within the same
> function.
> 

Currently, it does not know the phy number before parsing,
the only thing we can is to parse this twice. I think it is
kind of unnecessary.

> > +	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 this returns -ENODEV, you are done getting PHYs.  So
> max_phy_count could be more than the "actual" number.
> 
> Is that reasonable?  You indicate that at least one PHY
> must be found below, but is it OK for a platform to
> define fewer than some expected number of PHYs?
> 
> (Maybe it is.)
> 

Yes

> > +			if (PTR_ERR(k1->phy[i]) == -ENODEV)
> > +				break;
> > +
> > +			return PTR_ERR(k1->phy[i]);
> > +		}
> > +	}
> > +
> > +	k1->phy_count = i;
> > +	if (k1->phy_count == 0)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int k1_pcie_enable_phy(struct k1_pcie *k1)
> 
> I would call this k1_pcie_enable_phys().  But actually,
> because what you're calling within this is phy_init(),
> I'd probably call it k1_pcie_init_phys(), or possibly
> k1_pcie_phy_init_all().
> 

I will take care of this in the next version.

> > +{
> > +	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;
> > +}
> > +
> >   /* FIXME: Disable ASPM L1 to avoid errors reported on some NVMe drives */
> >   static void k1_pcie_disable_aspm_l1(struct k1_pcie *k1)
> >   {
> > @@ -174,7 +224,7 @@ static int k1_pcie_init(struct dw_pcie_rp *pp)
> >   	 */
> >   	regmap_set_bits(k1->pmu, reset_ctrl, DEVICE_TYPE_RC | PCIE_AUX_PWR_DET);
> > -	ret = phy_init(k1->phy);
> > +	ret = k1_pcie_enable_phy(k1);
> >   	if (ret) {
> >   		k1_pcie_disable_resources(k1);
> > @@ -194,12 +244,14 @@ 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++)
> > +		phy_exit(k1->phy[i]);
> 
> Please create an inverse of k1_pcie_enable_phy(), like
> k1_pcie_disable_phy(), to encapsulate this code.
> 

Sound reasonable.

> >   	k1_pcie_disable_resources(k1);
> >   }
> > @@ -266,23 +318,18 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)
> >   {
> >   	struct device *dev = k1->pci.dev;
> >   	struct device_node *root_port;
> > -	struct phy *phy;
> > +	int ret;
> >   	/* We assume only one root port */
> 
> Maybe you could get and put the root_port within
> k1_pcie_get_phy_handle(), since that's the only
> place it's needed.
> 

I think it is better to not do that, it is better to keep the function
do the thing as it claims.


> 					-Alex
> 
> >   	root_port = of_get_next_available_child(dev_of_node(dev), NULL);
> >   	if (!root_port)
> >   		return -EINVAL;
> > -	phy = devm_of_phy_get(dev, root_port, NULL);
> > +	ret = k1_pcie_get_phy_handle(k1, root_port);
> >   	of_node_put(root_port);
> > -	if (IS_ERR(phy))
> > -		return PTR_ERR(phy);
> > -
> > -	k1->phy = phy;
> > -
> > -	return 0;
> > +	return ret;
> >   }
> >   static int k1_pcie_probe(struct platform_device *pdev)
> > @@ -358,6 +405,7 @@ static const struct k1_pcie_device_data k1_pcie_device_data = {
> >   	.host_ops	= &k1_pcie_host_ops,
> >   	.ops		= &k1_pcie_ops,
> >   	.parse_port	= k1_pcie_parse_port,
> > +	.max_phy_count	= 1,
> >   };
> >   static const struct of_device_id k1_pcie_of_match_table[] = {
> 

Regards,
Inochi