[PATCH V2 7/7] PCI: host-generic: Add dwc PCIe controller based MSI controller usage

[Mayank Rana]

Hi Rob

On 7/16/2024 3:32 PM, Mayank Rana wrote:
> Hi Will and Rob
> 
> Thank you for your quick review comments.
> 
> On 7/16/2024 6:42 AM, Rob Herring wrote:
>> On Tue, Jul 16, 2024 at 09:58:12AM +0100, Will Deacon wrote:
>>> On Mon, Jul 15, 2024 at 11:13:35AM -0700, Mayank Rana wrote:
>>>> Add usage of Synopsys Designware PCIe controller based MSI 
>>>> controller to
>>>> support MSI functionality with ECAM compliant Synopsys Designware PCIe
>>>> controller. To use this functionality add device compatible string as
>>>> "snps,dw-pcie-ecam-msi".
>>>>
>>>> Signed-off-by: Mayank Rana <quic_mrana at quicinc.com>
>>>> ---
>>>>   drivers/pci/controller/pci-host-generic.c | 92 
>>>> ++++++++++++++++++++++++++++++-
>>>>   1 file changed, 91 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/controller/pci-host-generic.c 
>>>> b/drivers/pci/controller/pci-host-generic.c
>>>> index c2c027f..457ae44 100644
>>>> --- a/drivers/pci/controller/pci-host-generic.c
>>>> +++ b/drivers/pci/controller/pci-host-generic.c
>>>> @@ -8,13 +8,73 @@
>>>>    * Author: Will Deacon <will.deacon at arm.com>
>>>>    */
>>>> -#include <linux/kernel.h>
>>>>   #include <linux/init.h>
>>>> +#include <linux/kernel.h>
>>>>   #include <linux/module.h>
>>>> +#include <linux/of_address.h>
>>>>   #include <linux/pci-ecam.h>
>>>>   #include <linux/platform_device.h>
>>>>   #include <linux/pm_runtime.h>
>>>> +#include "dwc/pcie-designware-msi.h"
>>>> +
>>>> +struct dw_ecam_pcie {
>>>> +    void __iomem *cfg;
>>>> +    struct dw_msi *msi;
>>>> +    struct pci_host_bridge *bridge;
>>>> +};
>>>> +
>>>> +static u32 dw_ecam_pcie_readl(void *p_data, u32 reg)
>>>> +{
>>>> +    struct dw_ecam_pcie *ecam_pcie = (struct dw_ecam_pcie *)p_data;
>>>> +
>>>> +    return readl(ecam_pcie->cfg + reg);
>>>> +}
>>>> +
>>>> +static void dw_ecam_pcie_writel(void *p_data, u32 reg, u32 val)
>>>> +{
>>>> +    struct dw_ecam_pcie *ecam_pcie = (struct dw_ecam_pcie *)p_data;
>>>> +
>>>> +    writel(val, ecam_pcie->cfg + reg);
>>>> +}
>>>> +
>>>> +static struct dw_ecam_pcie *dw_pcie_ecam_msi(struct platform_device 
>>>> *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct dw_ecam_pcie *ecam_pcie;
>>>> +    struct dw_msi_ops *msi_ops;
>>>> +    u64 addr;
>>>> +
>>>> +    ecam_pcie = devm_kzalloc(dev, sizeof(*ecam_pcie), GFP_KERNEL);
>>>> +    if (!ecam_pcie)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    if (of_property_read_reg(dev->of_node, 0, &addr, NULL) < 0) {
>>
>> Using this function on MMIO addresses is wrong. It is an untranslated
>> address.
> ok. do you prefer me to use of_address_to_resource() instead here ?
> 
>>>> +        dev_err(dev, "Failed to get reg address\n");
>>>> +        return ERR_PTR(-ENODEV);
>>>> +    }
>>>> +
>>>> +    ecam_pcie->cfg = devm_ioremap(dev, addr, PAGE_SIZE);
>>>> +    if (ecam_pcie->cfg == NULL)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    msi_ops = devm_kzalloc(dev, sizeof(*msi_ops), GFP_KERNEL);
>>>> +    if (!msi_ops)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    msi_ops->readl_msi = dw_ecam_pcie_readl;
>>>> +    msi_ops->writel_msi = dw_ecam_pcie_writel;
>>>> +    msi_ops->pp = ecam_pcie;
>>>> +    ecam_pcie->msi = dw_pcie_msi_host_init(pdev, msi_ops, 0);
>>>> +    if (IS_ERR(ecam_pcie->msi)) {
>>>> +        dev_err(dev, "dw_pcie_msi_host_init() failed\n");
>>>> +        return ERR_PTR(-EINVAL);
>>>> +    }
>>>> +
>>>> +    dw_pcie_msi_init(ecam_pcie->msi);
>>>> +    return ecam_pcie;
>>>> +}
>>>
>>> Hmm. This looks like quite a lot of not-very-generic code to be adding
>>> to pci-host-generic.c. The file is now, what, 50% designware logic?
>>
>> Agreed.
>>
>> I would suggest you add ECAM support to the DW/QCom driver reusing some
>> of the common ECAM support code.
> I can try although there is very limited reusage of code with 
> pcie-qcom.c and pcie-designware-host.c except reusing MSI functionality. 
> That would make more new OPs within pcie-designware-host.c and 
> pcie-qcom.c just to perform few operation. As now MSI functionality is 
> available outside pcie core designware driver (although those changes 
> are under review), will you be ok to allow separate Qualcomm PCIe ECAM 
> driver as previously submitted RFC as 
> https://lore.kernel.org/all/d10199df-5fb3-407b-b404-a0a4d067341f@quicinc.com/T/
> 
> I can modify above ECAM driver to call into PCIe designware module based 
> MSI ops as doing here and that would allow reusing of MSI functionality 
> at same time allowing separate driver for handling firmware VM based 
> implementation.
Can you consider above request to have separate driver here ?
Please suggest on this.

>>
>> I suppose another option would be to define a node and driver which is
>> just the DW MSI controller. That might not work given the power domain
>> being added (which is not very generic either).
> yes, I did consider this approach, and haven't used this due to concern 
> as you mentioned, and also that ask for modifying devicetree usage for 
> existing user of PCIe Designware controller based MSI controller.
> 
> Regards,
> Mayank
>