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

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

[-- Attachment #1: Type: text/plain, Size: 4985 bytes --]

On Wed Sep 9, 2026 at 3:51 PM +08, Inochi Amaoto wrote:
> 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.
Agreed, we can defer GPIO reset support until a board needs it and
we can test it. I checked again, and the SDK only added this support
recently. It provides GPIO-controlled PERST# as an alternative to
the native PERST# control through the PMU.
>
>> > [...]
>> >
>> > +	/* 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.
I checked the register specification. The PCIe A/B/C IOMMU bypass
bits reset to 1, so bypass is the hardware reset default and does
not require firmware to enable it. That resolves my concern.

>
>> > [...]
>> >
>> > +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?
I have not confirmed which clock controls this MMIO region yet.
I have asked our clock team about its clock and reset dependencies
and whether access before resource initialization is guaranteed.
I will follow up once I have clarification.

>
> Regards,
> Inochi


-- 
Troy Mitchell


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