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

[Inochi Amaoto <inochiama@gmail.com>]

On Wed, Sep 30, 2026 at 10:54:00AM +0300, Andy Shevchenko wrote:
> On Tue, Sep 29, 2026 at 04:37:52PM +0800, Inochi Amaoto wrote:
> > The PCIe controller on Spacemit K3 is almost a standard Synopsys
> > DesignWare PCIe IP with extra link and reset control. Unlike
> > the PCIe controller on K1, this controller supports external MSI
> > interrupt controller and can use multiple PHYs at the same time.
> > 
> > Add driver to support PCIe controller on Spacemit K3 PCIe.
> 
> ...
> 
> > +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);
> 
> Would it make sense to define permutations
> 
>   PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE
> 
> for here...
> 

I think it does not make sense, they are two separate fields.
A combine does not make the thing easier.

> > +	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_init;
> > +
> > +	ret = phy_bulk_power_on(k1->phy_count, k1->phys);
> > +	if (ret)
> > +		goto failed_phy_power_on;
> > +
> > +	msleep(PCIE_T_PVPERL_MS);
> > +
> > +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
> > +			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);
> 
> ...and
> 
>   PCIE_PERSTN_OUT | PCIE_PERSTN_OE
> 
> for here and elsewhere?
> 
> > +	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
> 
> > +	val = u32_replace_bits(val, BIT(7),
> > +			       GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
> 
> It's perfectly a single line. Check your editor settings (I believe I have
> commented on a such in one of the previous rounds).
> 

Yes, but it is too hard for me as the intellisense plugin will show
the argument name so I am toot hard to keep that for a line that near
80 chars....

> > +	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
> > +
> > +	k1_pcie_set_device_id(k1);
> > +
> > +	/* Finally, as a workaround, disable ASPM L1 */
> > +	k1_pcie_disable_aspm_l1(k1);
> > +
> > +	return 0;
> > +
> > +failed_phy_power_on:
> > +	phy_bulk_exit(k1->phy_count, k1->phys);
> > +failed_phy_init:
> > +	k1_pcie_disable_resources(k1);
> > +failed_resources:
> > +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
> > +			   PCIE_PERSTN_OUT | PCIE_PERSTN_OE,
> > +			   PCIE_PERSTN_OE);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> You checked everything but regmap IO. Why? Do you except it won't ever fail?
> Perhaps to add a note about this (if not yet) to the cover letter?
> 

It should not fail. IIRC the regmap will not fail for an
exclusive usage. The regmap here is just as handle for
the common device, but the device driver only use its
exclusive area so it should be safe.

Regards,
Inochi