[PATCH 0/4] Loadable Module support for PCIe Cadence and J721E

Tue Mar 25 09:36:32 PDT 2025

On Wed, Mar 26, 2025 at 12:03:01AM +0800, Hans Zhang wrote:
> 
> 
> On 2025/3/25 23:26, manivannan.sadhasivam at linaro.org wrote:
> > EXTERNAL EMAIL
> > 
> > On Thu, Mar 20, 2025 at 10:14:02AM +0800, hans.zhang wrote:
> > > 
> > > 
> > > On 2025/3/19 17:55, manivannan.sadhasivam at linaro.org wrote:
> > > > EXTERNAL EMAIL
> > > > 
> > > > On Wed, Mar 19, 2025 at 05:31:01PM +0800, Peter Chen wrote:
> > > > > On 25-03-19 14:25:34, Siddharth Vadapalli wrote:
> > > > > > > > 
> > > > > > > > Hello,
> > > > > > > > 
> > > > > > > > This series enables support to build the PCIe Cadence Controller drivers
> > > > > > > > and the PCI J721E Application/Wrapper/Glue driver as Loadable Kernel
> > > > > > > > Modules. The motivation for this series is that PCIe is not a necessity
> > > > > > > > for booting the SoC, due to which it doesn't have to be a built-in
> > > > > > > > module. Additionally, the defconfig doesn't enable the PCIe Cadence
> > > > > > > > Controller drivers and the PCI J721E driver, due to which PCIe is not
> > > > > > > > supported by default. Enabling the configs as of now (i.e. without this
> > > > > > > > series) will result in built-in drivers i.e. a bloated Linux Image for
> > > > > > > > everyone who doesn't have the PCIe Controller.
> > > > > > > 
> > > > > > > If the user doesn't enable PCIe controller device through DTS/ACPI,
> > > > > > > that's doesn't matter.
> > > > > > 
> > > > > > The Linux Image for arm64 systems built using:
> > > > > > arch/arm64/configs/defconfig
> > > > > > will not have support for the Cadence PCIe Controller and the PCIe J721e
> > > > > > driver, because these configs aren't enabled.
> > > > > > 
> > > > > > > 
> > > > > > > > @@ -209,6 +209,12 @@ CONFIG_NFC=m
> > > > > > > >    CONFIG_NFC_NCI=m
> > > > > > > >    CONFIG_NFC_S3FWRN5_I2C=m
> > > > > > > >    CONFIG_PCI=y
> > > > > > > > +CONFIG_PCI_J721E=m
> > > > > > > > +CONFIG_PCI_J721E_HOST=m
> > > > > > > > +CONFIG_PCI_J721E_EP=m
> > > > > > > > +CONFIG_PCIE_CADENCE=m
> > > > > > > > +CONFIG_PCIE_CADENCE_HOST=m
> > > > > > > > +CONFIG_PCIE_CADENCE_EP=m
> > > > > > > 
> > > > > > > The common Cadence configuration will be select if the glue layer's
> > > > > > > configuration is select according to Kconfig.
> > > > > > > 
> > > > > > > Please do not set common configuration as module, some user may need
> > > > > > > it as build-in like dw's. Considering the situation, the rootfs is at
> > > > > > > NVMe.
> > > > > > 
> > > > > > The common configuration at the moment is "DISABLED" i.e. no support for
> > > > > > the Cadence Controller at all. Which "user" are you referring to? This
> > > > > > series was introduced since having the drivers built-in was pushed back at:
> > > > > 
> > > > > We are using Cadence controller, and prepare upstream radxa-o6 board
> > > > > whose rootfs is at PCIe NVMe.
> > > > > 
> > > > 
> > > > It doesn't matter. Only criteria AFAIK to build the driver as built-in in
> > > > defconfig is that it should be a depedency for console. For some time, storage
> > > > was also a dependency, but for sure PCIe is not.
> > > > 
> > > > Moreover, CONFIG_BLK_DEV_NVME is built as a module in ARM64 defconfig. So it
> > > > doesn't matter if you build PCIe controller driver as a built-in or not. You
> > > > need to load the NVMe driver somehow.
> > > > 
> > > > So please use initramfs.
> > > > 
> > > > > You could build driver as module for TI glue layer, but don't force
> > > > > other vendors using module as well, see dwc as an example please.
> > > > > 
> > > > 
> > > > DWC is a bad example here. Only reason the DWC drivers are not loadable is due
> > > > to the in-built MSI controller implementation as irqchip. People tend to build
> > > > the irqchip controllers as always built-in for some known issues. Even then some
> > > > driver developers prefer to built them as loadable module but suppress unbind to
> > > > avoid rmmoding the module.
> > > Hi Mani,
> > > 
> > > I think the MSI RTL module provided by Synopsys PCIe controller IP is not a
> > > standard operation. The reason for this MSI module is probably to be used by
> > > some cpus that do not have ITS(LPI interrupt) designed. Or RISC-V SOC, etc.
> > > MSI is defined as an MSI/MSIX interrupt that starts with a direct write
> > > memory access.
> > > 
> > 
> > Yeah, DWC MSI controller is not a great design. The older ones are even more
> > horrible (using SPI interrupts for reporting AERs etc...).
> 
> Hi Mani,
> 
> Currently Synopsys and Cadence provide SPI interrupts for reporting AERs
> etc... This IP vendor only provides an alternative approach that actually
> requires SOC design companies to design according to PCIe SPEC and conform
> to linux OS software behavior.
> 
> I have a way to workaround AER is SPI interrupt. It can also use aer.c
> drivers. However, I have been afraid to submit patch, because this is a
> problem of SOC designers themselves, which does not conform to the port
> driver of linux os (aer.c). So it will certainly not be accepted.
> 

Right. There is not clean way afaik.

> 
> > 
> > > There are also SOC vendors that do not use the built-in MSI RTL module.
> > > Instead, MSI/MSIX interrupts are transmitted directly to the GIC's ITS
> > > module via the GIC V3/V4 interface. For example, RK3588, they do not use the
> > > PCIe controller built-in MSI module. Some Qualcomm platforms also modify the
> > > PCIe controller's built-in MSI modules to connect each of them to 32 SPI
> > > interrupts to the GIC. I was under the impression that the SDM845 was
> > > designed that way. The only explanation is that SPI interrupts are faster
> > > than LPI interrupts without having to look up some tables.
> > > 
> > 
> > If ITS is available, platforms should make use of that. There is no way DWC MSI
> > is superior than ITS. We are slowly migrating the Qcom platforms to use ITS.
> > 
> 
> I agree with you.
> 
> > And btw, Qcom DWC MSI controller raise interrupts for AER/PME sent by the
> > downstream components. So enabling ITS is uncovering AER errors which were
> > already present :)
> > 
> > > So the dwc driver can also compile to ko?
> > > 
> > 
> > Only if the MSI support is made as a build time option and there is a guarantee
> > that the platform will never use it (which is difficult to do as the driver can
> > only detect it during the runtime based on devicetree).
> 
> Anyway, I would still like to request that the Cadence PCIe controller
> driver not be in module mode. Cadence also has a lot of customers, we are
> one of them, it's just that many customers don't have upstream. We are about
> to upstream.
> 
> This series was introduced since having the drivers built-in was pushed back
> at:
> https://lore.kernel.org/linux-arm-kernel/20250122145822.4ewsmkk6ztbeejzf@slashing/
> 
> Hans:
> The Cadence PCIe root port driver can not be made into module mode because
> of TI's idea. We should consider the ideas of other customers. If you have
> to make it module mode, I think all peripheral drivers should be module
> mode. Maybe I'm being direct, but that's probably the case.
> 

It is not about one company's idea to make the driver as a module. Linux kernel
community in general strongly encourages developers to build the drivers as
module unless there are exceptions (which I have already quoted). If you keep
building the drivers as built-in, it will result in a bloated kernel image. For
sure vendors would want *their* interested drivers to be built-in so that they
do not need to package the drivers in initramfs/rootfs, but that's not a
practice which is encouraged by the Linux community.

So I'm in favor of making the PCIe controllers as module if there are no
technical issues.

- Mani

-- 
மணிவண்ணன் சதாசிவம்