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

[Rob Herring <robh@kernel.org>]

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