Re: [PATCH v4 3/4] PCI: tegra: Add Tegra264 support

[Thierry Reding <thierry.reding@gmail.com>]

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

On Fri, Apr 10, 2026 at 10:04:20PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Apr 07, 2026 at 11:38:28AM +0200, Thierry Reding wrote:
> > On Thu, Apr 02, 2026 at 11:02:02PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Apr 02, 2026 at 04:27:37PM +0200, Thierry Reding wrote:
[...]
> > > > +	depends on ARCH_TEGRA || COMPILE_TEST
> > > > +	depends on PCI_MSI
> > > 
> > > Why?
> > 
> > I suppose it's not necessary in the sense of it being a build
> > dependency. At runtime, however, the root complex is not useful if PCI
> > MSI is not enabled. We can drop this dependency and rely on .config to
> > have it enabled as needed.
> > 
> 
> Yes. I think the rationale is to depend on the symbols that the driver needs for
> build dependency.

Done.

[...]
> > > > +						  GPIOD_IN);
> > > > +	if (IS_ERR(pcie->wake_gpio))
> > > > +		return PTR_ERR(pcie->wake_gpio);
> > > > +
> > > > +	if (pcie->wake_gpio) {
> > > 
> > > Since you are bailing out above, you don't need this check.
> > 
> > I think we still want to have this check to handle the case of optional
> > wake GPIOs. Not all controllers may have this wired up and
> > devm_gpiod_get_optional() will return NULL (not an ERR_PTR()-encoded
> > error) if the wake-gpios property is missing.
> > 
> 
> Ok. In that case you can just bail out:
> 	if (!pcie->wake_gpio)
> 		return 0;

Done.

[...]
> > > > +	bw = width * (PCIE_SPEED2MBS_ENC(speed) / BITS_PER_BYTE);
> > > > +	value = MBps_to_icc(bw);
> > > 
> > > So this becomes, 'width * (PCIE_SPEED2MBS_ENC(speed) / 8) * 1000 / 8'. But don't
> > > you want, 'width * (PCIE_SPEED2MBS_ENC(speed)) * 1000 / 8'?
> > 
> > This is M*B*ps_to_icc(), not M*b*ps_to_icc(), so we do in fact get the
> > latter. I almost fell for this as well because I got confused by some of
> > these macros being all-caps and other times the case actually mattering.
> > 
> 
> Oops, I was misleaded too. But you can simply do:
> 	bw = Mbps_to_icc(width * PCIE_SPEED2MBS_ENC(speed));
> 
> > > > +	err = icc_set_bw(pcie->icc_path, bw, bw);
> 
> And here you were setting the MBps, not Kbps.

Done.

> > > > +	if (err < 0)
> > > > +		dev_err(pcie->dev,
> > > > +			"failed to request bandwidth (%u MBps): %pe\n",
> > > > +			bw, ERR_PTR(err));
> > > 
> > > So you don't want to error out if this fails?
> > 
> > No. This is not a fatal error and the system will continue to work,
> > albeit perhaps at suboptimal performance. Given that Ethernet and mass
> > storage are connected to these, a failure to set the bandwidth and
> > erroring out here may leave the system unusable, but continuing on would
> > let the system boot and update firmware, kernel or whatever to recover.
> > 
> > I'll add a comment explaining this.
> > 
> 
> Yeah, that'll help.

Done.

[...]
> > > s/link/controller or endpoint?
> > 
> > This controls the PERST# signal, so I guess "endpoint" would be more
> > correct.
> > 
> 
> Yes!

Done.

[...]
> > > > +	if (!pcie->link_up)
> > > > +		goto free;
> > > 
> > > goto free_ecam;
> > 
> > It's not clear to me, but are you suggesting to rename the existing
> > "free" label to "free_ecam"? I can do that.
> > 
> 
> Yeah, I was just asking for a rename.

Done.

[...]
> > > > +static int tegra264_pcie_resume_noirq(struct device *dev)
> > > > +{
> > > > +	struct tegra264_pcie *pcie = dev_get_drvdata(dev);
> > > > +	int err;
> > > > +
> > > > +	if (pcie->wake_gpio && device_may_wakeup(dev)) {
> > > > +		err = disable_irq_wake(pcie->wake_irq);
> > > > +		if (err < 0)
> > > > +			dev_err(dev, "failed to disable wake IRQ: %pe\n",
> > > > +				ERR_PTR(err));
> > > > +	}
> > > > +
> > > > +	if (pcie->link_up == false)
> > > > +		return 0;
> > > > +
> > > > +	tegra264_pcie_init(pcie);
> > > > +
> > > 
> > > Why do you need init() here without deinit() in tegra264_pcie_suspend_noirq()?
> > 
> > That's because when we come out of suspend the link may have gone down
> > again, so we need to take the endpoint out of reset to retrigger the
> > link training. I think we could possibly explicitly clear that PERST_O_N
> > bit in the PERST_CONTROL register in a new tegra264_pcie_deinit() to
> > mirror what tegra264_pcie_init() does, but it's automatically done by
> > firmware anyway, so not needed.
> > 
> 
> Hmm, so firmware asserts PERST# at the end of suspend? It is not clear to me why
> it is doing so. But for symmetry I'd like to do it in tegra264_pcie_deinit().

Done.

> Also, I'm not certain about the 'pcie->link_up' check here. If it is 'false',
> then probe() should've failed. So why do you need the check here anyway?
> 
> Maybe you should get rid of this check and return the link status from
> tegra264_pcie_init() directly?

We specifically don't want to fail the probe for this when the link is
not there because we want to tighly control the power mode when the link
is not up. We also need to keep the link alive for the case where it can
be hotplug capable.

I've added a new tegra264_pcie_deinit() function to clear that PERST_O_N
bit explicitly, but I've kept the link_up flag.

Thanks,
Thierry

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