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

[Alex Elder <elder@riscstar.com>]

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@gmail.com>
> ---
>   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.

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

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

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.

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

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

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

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

					-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[] = {