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.
>
As Niklas pointed out, why can't you have (ARCH_TEGRA && ARM) in Kconfig?
> 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.
>
Well, certainly other SoC architectures are not following what Tegra does and I
don't see why Tegra should be different than others.
- Mani
--
மணிவண்ணன் சதாசிவம்
