| Message ID | 1721067215-5832-8-git-send-email-quic_mrana@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add power domain and MSI functionality with PCIe host generic ECAM driver | expand |
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@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@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) { > + 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? Will
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@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@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. > > + 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 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). Rob
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@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@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. > > 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
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@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@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 >
On Tue, Jul 23, 2024 at 03:56:35PM -0700, Mayank Rana wrote: > 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@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@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. > Generic ECAM driver is already supporting some DWC based ECAM implementations like the ones added in commit 58fb207fb100 ("PCI: generic: Add support for Synopsys DesignWare RC in ECAM mode"). From that perspective, I think it makes sense to add Qcom ECAM driver as a part of this. But at the same time, you can also add the ECAM mode to the existing DWC Qcom driver and reuse existing codes like MSI. Considering the amount of Qcom specific features you are going to add (like safety interrupts etc...), it won't look like a generic ECAM driver anyway. So I guess there is no *ideal* location for this driver. IMO as long as you avoid code duplication, I'm fine with whatever location. From a quick look, I think it you go with Rob's suggestion, you can reuse existing MSI and future RASDES functionalities, isn't it? - Mani
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@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) { + 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; +} + static const struct pci_ecam_ops gen_pci_cfg_cam_bus_ops = { .bus_shift = 16, .pci_ops = { @@ -73,6 +133,9 @@ static const struct of_device_id gen_pci_of_match[] = { { .compatible = "snps,dw-pcie-ecam", .data = &pci_dw_ecam_bus_ops }, + { .compatible = "snps,dw-pcie-ecam-msi", + .data = &pci_generic_ecam_ops }, + { }, }; MODULE_DEVICE_TABLE(of, gen_pci_of_match); @@ -80,6 +143,7 @@ MODULE_DEVICE_TABLE(of, gen_pci_of_match); static int gen_pcie_ecam_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct dw_ecam_pcie *ecam_pcie = NULL; int ret = 0; if (!IS_ERR_OR_NULL(dev->pm_domain)) { @@ -94,14 +158,30 @@ static int gen_pcie_ecam_probe(struct platform_device *pdev) } } + if (of_device_is_compatible(dev->of_node, "snps,dw-pcie-ecam-msi")) { + ecam_pcie = dw_pcie_ecam_msi(pdev); + if (IS_ERR(ecam_pcie)) { + ret = -ENODEV; + goto err; + } + } + ret = pci_host_common_probe(pdev); if (ret) { dev_err(dev, "pci_host_common_probe() failed:%d\n", ret); goto err; } + if (ecam_pcie) { + ecam_pcie->bridge = platform_get_drvdata(pdev); + platform_set_drvdata(pdev, ecam_pcie); + } + return ret; err: + if (!IS_ERR_OR_NULL(ecam_pcie)) + dw_pcie_free_msi(ecam_pcie->msi); + if (!IS_ERR_OR_NULL(dev->pm_domain)) pm_runtime_put_sync(dev); return ret; @@ -109,7 +189,17 @@ static int gen_pcie_ecam_probe(struct platform_device *pdev) static void gen_pcie_ecam_remove(struct platform_device *pdev) { + struct dw_ecam_pcie *ecam_pcie = NULL; + + if (of_device_is_compatible(pdev->dev.of_node, "snps,dw-pcie-ecam-msi")) { + ecam_pcie = platform_get_drvdata(pdev); + platform_set_drvdata(pdev, ecam_pcie->bridge); + } + pci_host_common_remove(pdev); + if (ecam_pcie) + dw_pcie_free_msi(ecam_pcie->msi); + if (pdev->dev.pm_domain) pm_runtime_put_sync(&pdev->dev); }
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@quicinc.com> --- drivers/pci/controller/pci-host-generic.c | 92 ++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 1 deletion(-)