Re: [PATCH v5 6/6] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

[Inochi Amaoto]

On Mon, Sep 07, 2026 at 09:14:23PM +0800, Troy Mitchell wrote:
> On Mon, Sep 7, 2026 at 07:26:05PM +0800, Inochi Amaoto wrote:
> > [...]
> >
> > @@ -303,6 +316,116 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)
> > [...]
> >
> > +static int k3_pcie_init(struct dw_pcie_rp *pp)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	struct k1_pcie *k1 = to_k1_pcie(pci);
> > +	u32 reset_ctrl = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
> > +	u32 val;
> > +	int ret;
> > +
> > +	regmap_clear_bits(k1->pmu, reset_ctrl, LTSSM_EN);
> > +
> > +	k1_pcie_toggle_soft_reset(k1);
> > +
> > +	/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */
> > +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
> > +			   PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE | PCIE_PERSTN_OUT,
> > +			   PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE);
> > +
> > +	ret = k1_pcie_enable_resources(k1);
> > +	if (ret)
> > +		goto failed_resources;
> > +
> > +	regmap_set_bits(k1->pmu, reset_ctrl, PCIE_AUX_PWR_DET);
> > +	regmap_clear_bits(k1->pmu, reset_ctrl, APP_HOLD_PHY_RST);
> > +
> > +	ret = phy_bulk_init(k1->phy_count, k1->phys);
> > +	if (ret)
> > +		goto failed_phy;
> > +
> > +	msleep(PCIE_T_PVPERL_MS);
> > +
> > +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
> > +			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);
> > +
> 
> Should we use pci->pe_rst when reset-gpios is provided, and keep the PMU path as
> a fallback? The SDK handles both cases. The DWC core requests that GPIO with
> GPIOD_OUT_HIGH, but this path only releases PERST# through the PMU, so an
> endpoint using the GPIO would remain in reset.
> 

I do not think this should be included in this version. I found PICO-ITX
has no reset gpio support. This means I can not test this feature.

I suggest adding this function when there is a board using this function.

> > [...]
> >
> > +	/* Finally, as a workaround, disable ASPM L1 */
> > +	k1_pcie_disable_aspm_l1(k1);
> > +
> > +	return 0;
> > +
> 
> Would we also need to configure IOMMU bypass during initialization? The SDK sets
> the PCIe A/B/C bypass bits in PMUA_PCIE_SUBSYS_MGMT when there is no usable
> iommu-map. The proposed K3 PCIe DTS has no iommu-map, and I could not find the
> corresponding bypass setup in this series.
> 
> Is bypass already guaranteed by firmware or the reset state, or should the
> driver set it here? My concern is that enumeration could succeed while endpoint
> DMA still goes through an unconfigured IOMMU.
> 

I think the firmware should mark it bypassed as the default, at least
I have notice this behavior, but I am not sure whether it is the builtin
firmware or the uboot do this trick.

> > [...]
> >
> > +static int k3_pcie_parse_port(struct k1_pcie *k1)
> > +{
> > +	u32 status0, status1, status2;
> > +
> > +	/* This register require a RAW for cleanup */
> > +	status0 = readl_relaxed(k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
> > +	status1 = readl_relaxed(k1->link + INTR_STATUS);
> > +	status2 = readl_relaxed(k1->link + K3_ADDR_INTR_STATUS1);
> > +
> > +	writel_relaxed(status0, k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
> > +	writel_relaxed(status1, k1->link + INTR_STATUS);
> > +	writel_relaxed(status2, k1->link + K3_ADDR_INTR_STATUS1);
> > +
> > +	return k1_pcie_parse_port(k1);
> > +}
> > +
> 
> Are these status registers accessible before the controller clocks are enabled
> and resets released? k3_pcie_parse_port() runs before dw_pcie_host_init(), which
> calls k3_pcie_init() to enable those resources.
> 

Yes they can. It is something interesting.

> The SDK uses the same ordering, but I am not sure whether it relies on firmware
> leaving the registers accessible. If so, would it be safer to move this clearing
> into k3_pcie_init(), after enabling the resources?
> 

In fact, I have no idea about which clock control this MMIO area, if it is dbi
clock (but I guest it is not), it is kind of weird for this clear and should
move to the init. Do you have some knowledge on this?

Regards,
Inochi