| Message ID | 1444979960-24100-6-git-send-email-Minghuan.Lian@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@freescale.com> > --- > 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. > +} > + > 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 = { > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, I thank you very much to help review and help revise my patches. 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. Thanks, Minghaun > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: Thursday, October 22, 2015 5:34 AM > To: Lian Minghuan-B31939 <Minghuan.Lian@freescale.com> > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zang > Roy-R61911 <tie-fei.zang@freescale.com>; Hu Mingkai-B21284 > <Mingkai.Hu@freescale.com>; Yoder Stuart-B08248 > <stuart.yoder@freescale.com>; Li Yang-Leo-R58472 <LeoLi@freescale.com>; > Arnd Bergmann <arnd@arndb.de>; Bjorn Helgaas <bhelgaas@google.com>; > Jingoo Han <jg1.han@samsung.com>; Zhou Wang <wangzhou1@hisilicon.com> > Subject: Re: [PATCH v4 6/6] PCI: layerscape: add ls_pcie_msi_host_init > > 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@freescale.com> > > --- > > 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. > > > +} > > + > > 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 = { > > -- > > 1.9.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" > > in the body of a message to majordomo@vger.kernel.org More majordomo > > info at http://vger.kernel.org/majordomo-info.html
[+cc Thomas for MSI driver file placement question + PCI MSI driver structure] Hi Minghuan, Your reply was base64-encoded and thus rejected by the vger mailing lists. I mentioned this before (http://lkml.kernel.org/r/20151011191027.GA29221@localhost). You might want to fix your mail strategy, because it's really hard to carry on a conversation if nobody can hear your side :) But I'll include your response here again by hand. 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@freescale.com> > > > --- > > > 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. Bjorn
Hi Bjorn, I am sorry, I just change to outlook as email client instead of thunderbird. And I changed email type to plain text but forgot to check code type. Now, I have updated the options and hope it is ok. > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: Friday, October 23, 2015 12:22 AM > To: Lian Minghuan-B31939 <Minghuan.Lian@freescale.com> > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Zang > Roy-R61911 <tie-fei.zang@freescale.com>; Hu Mingkai-B21284 > <Mingkai.Hu@freescale.com>; Yoder Stuart-B08248 > <stuart.yoder@freescale.com>; Li Yang-Leo-R58472 <LeoLi@freescale.com>; > Arnd Bergmann <arnd@arndb.de>; Bjorn Helgaas <bhelgaas@google.com>; > Jingoo Han <jg1.han@samsung.com>; Zhou Wang > <wangzhou1@hisilicon.com>; Thomas Gleixner <tglx@linutronix.de> > Subject: Re: [PATCH v4 6/6] PCI: layerscape: add ls_pcie_msi_host_init > > [+cc Thomas for MSI driver file placement question + PCI MSI driver structure] > > Hi Minghuan, > > Your reply was base64-encoded and thus rejected by the vger mailing lists. > I mentioned this before > (http://lkml.kernel.org/r/20151011191027.GA29221@localhost). You might > want to fix your mail strategy, because it's really hard to carry on a > conversation if nobody can hear your side :) > > But I'll include your response here again by hand. > > 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@freescale.com> > > > > --- > > > > 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. > [Lian Minghuan-B31939] 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. Thanks, Minghuan > Bjorn
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; +} + 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 = {
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@freescale.com> --- 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(+)