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

[Hans Zhang <hans.zhang@cixtech.com>]

On 2025/3/26 00:36, manivannan.sadhasivam@linaro.org wrote:
> EXTERNAL EMAIL
> 
> On Wed, Mar 26, 2025 at 12:03:01AM +0800, Hans Zhang wrote:
>>
>>
>> On 2025/3/25 23:26, manivannan.sadhasivam@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@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.
> 

Hi Mani,

Okay, you must be more thoughtful than I am.

Best regards,
Hans