Your reply was *still* base64-encoded and thus rejected by the vger
mailing lists.
Minghuan wrote:
> On Thu, Oct 22, 2015 at 11:21:30AM -0500, Bjorn Helgaas wrote:
> > Minghuan wrote:
> > > On Wed, Oct 21, 2015 at 04:34:16PM -0500, Bjorn Helgaas wrote:
> > > > Hi Minghuan,
> > > >
> > > > On Fri, Oct 16, 2015 at 03:19:20PM +0800, Minghuan Lian wrote:
> > > > > Layerscape PCIe has its own MSI implementation. The patch registers
> > > > > ls_pcie_msi_host_init() to avoid using Designware's MSI.
> > > > >
> > > > > Signed-off-by: Minghuan Lian <Minghuan.Lian@xxxxxxxxxxxxx>
> > > > > ---
> > > > > Change log
> > > > > v4: split from [PATCH v3] PCI: layerscape: Add PCIe support for LS1043a and LS2080a
> > > > >
> > > > > drivers/pci/host/pci-layerscape.c | 17 +++++++++++++++++
> > > > > 1 file changed, 17 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
> > > > > index c53692a..8fac6c8 100644
> > > > > --- a/drivers/pci/host/pci-layerscape.c
> > > > > +++ b/drivers/pci/host/pci-layerscape.c
> > > > > @@ -150,14 +150,31 @@ static void ls_pcie_host_init(struct pcie_port *pp)
> > > > > iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
> > > > > }
> > > > >
> > > > > +static int ls_pcie_msi_host_init(struct pcie_port *pp,
> > > > > + struct msi_controller *chip)
> > > > > +{
> > > > > + struct device_node *msi_node;
> > > > > + struct device_node *np = pp->dev->of_node;
> > > > > +
> > > > > + msi_node = of_parse_phandle(np, "msi-parent", 0);
> > > > > + if (!msi_node) {
> > > > > + dev_err(pp->dev, "failed to find msi-parent\n");
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > >
> > > > I don't see how this can be right. I think it's OK if you want to enforce
> > > > the presence of "msi-parent", but the other implementations of
> > > > .msi_host_init (ks_dw_pcie_msi_host_init() and the default implementation
> > > > in dw_pcie_host_init()) both set pp->irq_domain and call
> > > > irq_create_mapping().
> > > >
> > > > You don't do either of those, so I don't see how MSIs can work, because I
> > > > assume the generic DesignWare code will depend on pp->irq_domain. If
> > > > you're planning to add more Layerscape-specific MSI support later, I think
> > > > you should wait and include this patch with that work.
> >
> > > Regarding MSI, both LS1021a and LS1043a use SCFG to implement it. I have submitted patch:
> > > https://patchwork.kernel.org/patch/7411131/
> > > https://patchwork.kernel.org/patch/7411141/
> >
> > > While ls2085a use ITS for it, we just re-use the ITS driver.
> >
> > > I notice some platform MSI driver files were placed in pci/host
> > > folder not irqchip. If it is ok, I would like to change driver
> > > folder and re-submitted the MSI patch.
> >
> > I suppose you're referring to drivers/pci/host/pci-xgene-msi.c.
> > That file doesn't really have much PCI stuff in it. It does call
> > pci_msi_create_irq_domain(), but that's really the only PCI interface or
> > data structure it uses. So I don't know if drivers/pci/host or
> > drivers/irqchip is the best place for it and for your
> > irq-ls-scfg-msi.c.
> >
> > The connection between pci-xgene-msi.c and pci-xgene.c is not very
> > clear to me, and that's sort of what I'm complaining about here.
> > You're overriding a default MSI initialization method. Usually that
> > means you do the same thing as the default method, but in a different
> > way. You aren't doing the same thing at all, which makes the code
> > hard to review.
> >
> > Maybe a comment about how the MSI controller gets connected to devices
> > below this host bridge would be enough.
> 1. We need to define callback function msi_host_init() to avoid the
> running desginware MSI related code Because our PCIe controller do not
> enable this feature.
>
> 2. we do not need to do anything such as bus->msi= ls_scfg_msi Because
> with the latest code, when PCIe controller device is created
> of_msi_configure() will be called and MSI domain pointed by
> 'msi-parent' will be bound to the device. pci_msi_get_domain(struct
> pci_dev *dev) -> dev_get_msi_domain(&dev->dev) will return this MSI
> domain.
>
> 3. I will add a comment in the next version.
Don't bother, I added a comment and applied the patch like this:
commit 90cbdf8412d206f65ece3dcc53230e673da569c6
Author: Minghuan Lian <Minghuan.Lian@xxxxxxxxxxxxx>
Date: Fri Oct 16 15:19:20 2015 +0800
PCI: layerscape: Add ls_pcie_msi_host_init()
Layerscape PCIe has its own MSI implementation.
Register ls_pcie_msi_host_init() to avoid using DesignWare's MSI.
[bhelgaas: add comment]
Signed-off-by: Minghuan Lian <Minghuan.Lian@xxxxxxxxxxxxx>
Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
index 6a9fa8a..1677890 100644
--- a/drivers/pci/host/pci-layerscape.c
+++ b/drivers/pci/host/pci-layerscape.c
@@ -150,14 +150,37 @@ static void ls_pcie_host_init(struct pcie_port *pp)
iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
}
+static int ls_pcie_msi_host_init(struct pcie_port *pp,
+ struct msi_controller *chip)
+{
+ struct device_node *msi_node;
+ struct device_node *np = pp->dev->of_node;
+
+ /*
+ * The MSI domain is set by the generic of_msi_configure(). This
+ * .msi_host_init() function keeps us from doing the default MSI
+ * domain setup in dw_pcie_host_init() and also enforces the
+ * requirement that "msi-parent" exists.
+ */
+ msi_node = of_parse_phandle(np, "msi-parent", 0);
+ if (!msi_node) {
+ dev_err(pp->dev, "failed to find msi-parent\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static struct pcie_host_ops ls1021_pcie_host_ops = {
.link_up = ls1021_pcie_link_up,
.host_init = ls1021_pcie_host_init,
+ .msi_host_init = ls_pcie_msi_host_init,
};
static struct pcie_host_ops ls_pcie_host_ops = {
.link_up = ls_pcie_link_up,
.host_init = ls_pcie_host_init,
+ .msi_host_init = ls_pcie_msi_host_init,
};
static struct ls_pcie_drvdata ls1021_drvdata = {
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
