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.
> > + 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
