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

["Troy Mitchell" <troy.mitchell@linux.spacemit.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3202 bytes --]

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.

> [...]
>
> +	/* 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.

> [...]
>
> +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.

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?

                                            - Troy

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv