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

[Manivannan Sadhasivam]

On 2026-09-30 17:44 +0800, Inochi Amaoto wrote:
> 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....
> 

If the line is within 80 chars, then there is no need to break. If your plugin
is the issue, then fix it please.

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

Here, regmap on MMIO is just readl+writel, so there is no way it can fail.

- Mani