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@xxxxxxxxxxx>
> > > > > ---
> > > > > 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@xxxxxxx>
> > > > > */
> > > > > -#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@xxxxxxxxxxx/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").
