Re: [RFC PATCH 2/2] PCI: Add Qualcomm PCIe ECAM root complex driver

[Mayank Rana <quic_mrana@quicinc.com>]

On 6/5/2024 7:39 PM, Manivannan Sadhasivam wrote:
> On Fri, May 31, 2024 at 03:47:24PM -0700, Mayank Rana wrote:
>> Hi Rob / Mani
>>
>> On 4/15/2024 4:30 PM, Mayank Rana wrote:
>>> Hi Rob
>>>
>>> Excuse me for late response on this (was OOO).
>>> On 4/10/2024 9:58 AM, Rob Herring wrote:
>>>> On Mon, Apr 08, 2024 at 11:57:58AM -0700, Mayank Rana wrote:
>>>>> Hi Mani
>>>>>
>>>>> On 4/5/2024 9:17 PM, Manivannan Sadhasivam wrote:
>>>>>> On Fri, Apr 05, 2024 at 10:41:15AM -0700, Mayank Rana wrote:
>>>>>>> Hi Mani
>>>>>>>
>>>>>>> On 4/4/2024 10:30 PM, Manivannan Sadhasivam wrote:
>>>>>>>> On Thu, Apr 04, 2024 at 12:11:24PM -0700, Mayank Rana wrote:
>>>>>>>>> On some of Qualcomm platform, firmware
>>>>>>>>> configures PCIe controller into
>>>>>>>>> ECAM mode allowing static memory allocation for
>>>>>>>>> configuration space of
>>>>>>>>> supported bus range. Firmware also takes care of
>>>>>>>>> bringing up PCIe PHY
>>>>>>>>> and performing required operation to bring PCIe
>>>>>>>>> link into D0. Firmware
>>>>>>>>> also manages system resources (e.g.
>>>>>>>>> clocks/regulators/resets/ bus voting).
>>>>>>>>> Hence add Qualcomm PCIe ECAM root complex driver
>>>>>>>>> which enumerates PCIe
>>>>>>>>> root complex and connected PCIe devices.
>>>>>>>>> Firmware won't be enumerating
>>>>>>>>> or powering up PCIe root complex until this
>>>>>>>>> driver invokes power domain
>>>>>>>>> based notification to bring PCIe link into D0/D3cold mode.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Is this an in-house PCIe IP of Qualcomm or the same
>>>>>>>> DWC IP that is used in other
>>>>>>>> SoCs?
>>>>>>>>
>>>>>>>> - Mani
>>>>>>> Driver is validated on SA8775p-ride platform using PCIe DWC IP for
>>>>>>> now.Although this driver doesn't need to know used PCIe
>>>>>>> controller and PHY
>>>>>>> IP as well programming sequence as that would be taken
>>>>>>> care by firmware.
>>>>>>>
>>>>>>
>>>>>> Ok, so it is the same IP but firmware is controlling the
>>>>>> resources now. This
>>>>>> information should be present in the commit message.
>>>>>>
>>>>>> Btw, there is an existing generic ECAM host controller driver:
>>>>>> drivers/pci/controller/pci-host-generic.c
>>>>>>
>>>>>> This driver is already being used by several vendors as
>>>>>> well. So we should try
>>>>>> to extend it for Qcom usecase also.
>>>>
>>>> I would take it a bit further and say if you need your own driver, then
>>>> just use the default QCom driver. Perhaps extend it to support ECAM.
>>>> Better yet, copy your firmware setup and always configure the QCom h/w
>>>> to use ECAM.
>>> Good suggestion. Although here we are having 2 set of requirements:
>>> 1. ECAM configuration
>>> 2. Managing PCIe controller and PHY resources and programming from
>>> firmware as well
>>> Hence it is not feasible to use default QCOM driver.
>>>> If you want to extend the generic driver, that's fine, but we don't need
>>>> a 3rd.
>>> I did consider this part before coming up with new driver. Although I
>>> felt that
>>> below mentioned functionality may not look more generic to be part of
>>> pci-host-generic.c driver.
>>>>> I did review pci-host-generic.c driver for usage. although there
>>>>> are more
>>>>> functionalityneeded for use case purpose as below:
>>>>> 1. MSI functionality
>>>>
>>>> Pretty sure the generic driver already supports that.
>>> I don't find any MSI support with pci-host-generic.c driver.
>>>>> 2. Suspend/Resume
>>>>
>>>> Others might want that to work as well.
>>> Others firmware won't have way to handle D3cold and D0 functionality
>>> handling as
>>> needed here for supporting suspend/resume as I don't find any interface
>>> for pci-host-generic.c driver to notify firmware. here we are having way
>>> to talk to firmware using GenPD based power domain usage to communicate
>>> with firmware.
>>>
>>>>> 3. Wakeup Functionality (not part of current change, but would be added
>>>>> later)
>>>>
>>>> Others might want that to work as well.
>>> possible if suspend/resume support is available or used.
>>>>> 4. Here this driver provides way to virtualized PCIe controller.
>>>>> So VMs only
>>>>> talk to a generic ECAM whereas HW is only directed accessed by
>>>>> service VM.
>>>>
>>>> That's the existing driver. If if doesn't work for a VM, fix the VM.
>>> Correct.
>>>>> 5. Adding more Auto based safety use cases related implementation
>>>>
>>>> Now that's just hand waving.
>>> Here I am trying to provide new set of changes plan to be added as part
>>> of required functionality.
>>>
>>>>> Hence keeping pci-host-generic.c as generic driver where above
>>>>> functionality
>>>>> may not be needed.
>>>>
>>>> Duplicating things to avoid touching existing drivers is not how kernel
>>>> development works.
>>> I shall try your suggestion and see how it looks in terms of code
>>> changes. Perhaps then we can have more clarity in terms of adding more
>>> functionality into generic or having separate driver.
>> I just learnt that previously dwc related PCIe ECAM driver and MSI
>> controller driver tried out as:
>>
>> https://lore.kernel.org/linux-pci/20170821192907.8695-1-ard.biesheuvel@linaro.org/
>>
>> Although there were few concerns at that time. Due to that having dwc
>> specific MSI functionality based driver was dropped, and pci-host-generic.c
>> driver is being updated using with dwc/snps specific ECAM operation.
>>
>> In current discussion, it seems that we are discussing to have identical
>> approach here.
>>
>> Atleast on Qualcomm SA8775p platform, I don't have any other way to support
>> MSI functionality i.e. extended SPI or ITS/LPI based MSI or using GICv2m
>> functionality are not supported.
>>
>> I don't see any other approach other than MSI based implementation within
>> pci-host-generic.c driver for dwc/snps based MSI controller.
>>
>> Do you have any suggestion on this ?
>>
> 
> Since this ECAM driver is going to be used in newer Qcom SoCs, why can't you use
> GICv3 for MSI handling?
Yes, that is plan further as look like we have limitation on just SA8775.
So I see two options here:
1. Update pcie-host-generic.c without MSI based functionality, and leave 
with MSI functionality differently on SA8775
2. Also possible to make pcie-host-designware.c based MSI functionality 
as separate driver, and try to use with pcie-host-generic.c driver. That 
way we would still use existing MSI related code base, and able to use 
with ECAM driver.

Do you see using above option 2 as good way to allow SNPS/DWC based MSI 
controller functionality with ECAM and Non-ECAM driver ?

Regards,
Mayank