Re: [PATCH V1] PCI: tegra194: Add support for PCIe RC & EP in Tegra234 Platforms

[Niklas Cassel <cassel@kernel.org>]

On Tue, Feb 04, 2025 at 06:19:51PM +0100, Thierry Reding wrote:
> On Mon, Feb 03, 2025 at 10:29:32PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Jan 28, 2025 at 01:04:32PM +0100, Niklas Cassel wrote:
> > > Hello Vidya,
> > > 
> > > On Tue, Jan 28, 2025 at 10:12:44AM +0530, Vidya Sagar wrote:
> > > > Add PCIe RC & EP support for Tegra234 Platforms.
> > > 
> > > The commit log does leave quite a few questions unanswered.
> > > 
> > > Since you are just updating the Kconfig and nothing else:
> > > Does the DT binding already have support for the Tegra234 SoC?
> > > Does the driver already have support for the Tegra234 SoC?
> > > 
> > > Looking at the DT binding and driver, the answer to both questions
> > > is yes. (This should have been in the commit message IMO.)
> > > 
> > > 
> > > But that leads me to the question, since there is support for Tegra234
> > > SoC in the driver, does this means that this fixes a regression, e.g.
> > > the Kconfig ARCH_TEGRA_234_SOC was added after the driver support in
> > > this driver was added. In this case, you should have a Fixes: tag that
> > > points to the commit that added ARCH_TEGRA_234_SOC.
> > > 
> > > Or has the the driver support for Tegra234 been "dead-code" since it
> > > was originally added? (Because without this patch, no one can have
> > > tested it, at least not without COMPILE_TEST.)
> > > In this case, you should add:
> > > Fixes: a54e19073718 ("PCI: tegra194: Add Tegra234 PCIe support")
> > > 
> > 
> > TBH, I don't like muddling with Kconfig like this. Ideally, the driver should
> > just depend on ARCH_TEGRA || COMPILE_TEST and the driver should be selected by
> > the relevant defconfig.
> 
> ARCH_TEGRA is a symbol that exists both on 32-bit and 64-bit ARM. This
> driver is completely useless on 32-bit ARM and only used on a very small
> subset of 64-bit ARM devices. It doesn't make sense to be able to enable
> this if you want to build a kernel for say Tegra210.

Well, if you look in drivers/pci/controller/dwc/Kconfig
there are quite a few drivers that does:

depends on ARM64 or ARM and then a ARCH_ something.

I don't see why you can't do
depends on (ARCH_TEGRA && ARM64) || COMPILE_TEST


Kind regards,
Niklas