Re: [PATCH v4 2/6] PCI: spacemit-k1: Add multiple PHY handles support
From: Alex Elder
Date: Fri Jul 10 2026 - 12:02:01 EST
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.
};
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[] = {