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.
The relevant defconfig in this case would be the arm64 defconfig, which
isn't very authoritative.
> And this is what all other rest of the platforms are doing. Why should Nvidia be
> different? It makes me feel that this Kconfig dependency is used as a workaround
> for defconfig updates.
Well, it's certainly not used as a workaround for defconfig updates
because the change itself doesn't enable this symbol. You'd still need a
defconfig change to do that.
Also, we do this primarily because we've always done things this way on
Tegra. As I said, for a lot of drivers it doesn't make sense to include
them in a 32-bit build or 64-bit build because the hardware simply
doesn't exist. Having per-SoC generation Kconfig symbols allows this to
be modelled more accurately and if desired to build compact images.
Thierry
Attachment:
signature.asc
Description: PGP signature
