Re: [PATCH v2 4/5] PCI: tegra: Add Tegra264 support

[Thierry Reding <thierry.reding@kernel.org>]

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

On Wed, Mar 25, 2026 at 12:48:50PM +0900, Mikko Perttunen wrote:
> On Fri, 20 Mar 2026 23:54:36 +0100, Thierry Reding <thierry.reding@kernel.org> wrote:
> > diff --git a/drivers/pci/controller/pcie-tegra264.c b/drivers/pci/controller/pcie-tegra264.c
> > new file mode 100644
> > index 000000000000..3ce1ad971bdb
> > --- /dev/null
> > +++ b/drivers/pci/controller/pcie-tegra264.c
> > @@ -0,0 +1,527 @@
> > [ ... skip 123 lines ... ]
> > +	err = tegra_bpmp_transfer(pcie->bpmp, &msg);
> > +	if (err)
> > +		dev_info(pcie->dev, "failed to turn off PCIe #%u: %pe\n",
> > +			 pcie->ctl_id, ERR_PTR(err));
> > +
> > +	if (msg.rx.ret)
> > +		dev_info(pcie->dev, "failed to turn off PCIe #%u: %d\n",
> > +			 pcie->ctl_id, msg.rx.ret);
> > +}
> 
> Ideally we would distinguish by message in these two cases. I suppose
> the %pe vs. %d does that, but it's not quite obvious.

Agreed. I think something like:

	"BPMP transfer failed for PCIe #%u: %pe"

would work better for the first message. That's roughly in line with
other error messages for BPMP message transfer errors (as opposed to the
errors returned from BPMP as a result of a successful transfer).

> 
> > +
> > +static void tegra264_pcie_icc_set(struct tegra264_pcie *pcie)
> > +{
> > +	u32 value, speed, width, bw;
> > +	int err;
> > +
> > +	value = readw(pcie->ecam + XTL_RC_PCIE_CFG_LINK_STATUS);
> > +	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, value);
> > +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, value);
> > +
> > +	bw = width * (PCIE_SPEED2MBS_ENC(speed) / BITS_PER_BYTE);
> 
> PCIE_SPEED2MBS_ENC takes enum pci_bus_speed values rather than
> LNKSTA_CLS values. Perhaps it should be converted to a static inline
> function to catch such issues.

Nice catch. speed should be indexing pcie_link_speed to get the right
enum value. I've fixed that up.

> 
> > +	value = MBps_to_icc(bw);
> 
> The result of this is unused.

Ugh... this is supposed to be the value that's passed to icc_set_bw()
but I forgot to plumb that through. Fixed now.

> 
> > [ ... skip 271 lines ... ]
> > +	pcie->ecam = pcie->cfg->win;
> > +
> > +	tegra264_pcie_init(pcie);
> > +
> > +	if (!pcie->link_up)
> > +		goto free;
> 
> This path causes things to be freed, but the driver still probes. The
> remove callback will then try to remove things again.

I think the intent of the original code was to allow the driver to
continue to probe, so that the root complex would show up, but with no
devices enumerated. I don't know if we really need that, since we also
have no hotplug support there's never going to be any devices added to
the bus if the initial link up fails, as far as I can tell. I'll turn
this into a failure for now, we can revisit this if we ever need hotplug
support.

Thanks,
Thierry

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