| Message ID | 20190822112242.16309-7-xiaowei.bao@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/10] PCI: designware-ep: Add multiple PFs support for DWC | expand |
On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > The layerscape platform use the doorbell way to trigger MSIX > interrupt in EP mode. > I have no problems with this patch, however... Are you able to add to this message a reason for why you are making this change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient? Thanks, Andrew Murray > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com> > --- > v2: > - No change. > > drivers/pci/controller/dwc/pci-layerscape-ep.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c > index 8461f62..7ca5fe8 100644 > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c > @@ -74,7 +74,8 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no, > case PCI_EPC_IRQ_MSI: > return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > case PCI_EPC_IRQ_MSIX: > - return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); > + return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no, > + interrupt_num); > default: > dev_err(pci->dev, "UNKNOWN IRQ type\n"); > return -EINVAL; > -- > 2.9.5 >
> -----Original Message----- > From: Andrew Murray <andrew.murray@arm.com> > Sent: 2019年8月23日 21:58 > To: Xiaowei Bao <xiaowei.bao@nxp.com> > Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the > doorbell way > > On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > > The layerscape platform use the doorbell way to trigger MSIX interrupt > > in EP mode. > > > > I have no problems with this patch, however... > > Are you able to add to this message a reason for why you are making this > change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did > it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient? The fact is that, this driver is verified in ls1046a platform of NXP before, and ls1046a don't support MSIX feature, so I set the msix_capable of pci_epc_features struct is false, but in other platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX feature in ls1088a, it is not OK, so I changed to another way. Thanks. > > Thanks, > > Andrew Murray > > > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com> > > --- > > v2: > > - No change. > > > > drivers/pci/controller/dwc/pci-layerscape-ep.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c > > b/drivers/pci/controller/dwc/pci-layerscape-ep.c > > index 8461f62..7ca5fe8 100644 > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c > > @@ -74,7 +74,8 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, > u8 func_no, > > case PCI_EPC_IRQ_MSI: > > return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > > case PCI_EPC_IRQ_MSIX: > > - return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); > > + return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no, > > + interrupt_num); > > default: > > dev_err(pci->dev, "UNKNOWN IRQ type\n"); > > return -EINVAL; > > -- > > 2.9.5 > >
On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: > > > > -----Original Message----- > > From: Andrew Murray <andrew.murray@arm.com> > > Sent: 2019年8月23日 21:58 > > To: Xiaowei Bao <xiaowei.bao@nxp.com> > > Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. > > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > > gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org > > Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the > > doorbell way > > > > On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > > > The layerscape platform use the doorbell way to trigger MSIX interrupt > > > in EP mode. > > > > > > > I have no problems with this patch, however... > > > > Are you able to add to this message a reason for why you are making this > > change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did > > it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient? > > The fact is that, this driver is verified in ls1046a platform of NXP before, and ls1046a don't > support MSIX feature, so I set the msix_capable of pci_epc_features struct is false, > but in other platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX > feature in ls1088a, it is not OK, so I changed to another way. Thanks. Right, so the existing pci-layerscape-ep.c driver never supported MSIX yet it erroneously had a switch case statement to call dw_pcie_ep_raise_msix_irq which would never get used. Now that we're adding a platform with MSIX support the existing dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are adding a different method. Given that dw_pcie_ep_raise_msix_irq is used by pcie-designware-plat.c we can assume this function at least works for it's use case. Please update the commit message - It would be helpful to suggest that dw_pcie_ep_raise_msix_irq was never called in the exisitng driver because msix_capable was always set to false. Thanks, Andrew Murray > > > > > Thanks, > > > > Andrew Murray > > > > > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com> > > > --- > > > v2: > > > - No change. > > > > > > drivers/pci/controller/dwc/pci-layerscape-ep.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c > > > b/drivers/pci/controller/dwc/pci-layerscape-ep.c > > > index 8461f62..7ca5fe8 100644 > > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c > > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c > > > @@ -74,7 +74,8 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, > > u8 func_no, > > > case PCI_EPC_IRQ_MSI: > > > return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > > > case PCI_EPC_IRQ_MSIX: > > > - return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); > > > + return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no, > > > + interrupt_num); > > > default: > > > dev_err(pci->dev, "UNKNOWN IRQ type\n"); > > > return -EINVAL; > > > -- > > > 2.9.5 > > >
> -----Original Message----- > From: Andrew Murray <andrew.murray@arm.com> > Sent: 2019年8月27日 21:25 > To: Xiaowei Bao <xiaowei.bao@nxp.com> > Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the > doorbell way > > On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: > > > > > > > -----Original Message----- > > > From: Andrew Murray <andrew.murray@arm.com> > > > Sent: 2019年8月23日 21:58 > > > To: Xiaowei Bao <xiaowei.bao@nxp.com> > > > Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > > > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > > > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; > M.h. > > > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > > > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > > > gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org > > > Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to > > > the doorbell way > > > > > > On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > > > > The layerscape platform use the doorbell way to trigger MSIX > > > > interrupt in EP mode. > > > > > > > > > > I have no problems with this patch, however... > > > > > > Are you able to add to this message a reason for why you are making > > > this change? Did dw_pcie_ep_raise_msix_irq not work when func_no != > > > 0? Or did it work yet dw_pcie_ep_raise_msix_irq_doorbell is more > efficient? > > > > The fact is that, this driver is verified in ls1046a platform of NXP > > before, and ls1046a don't support MSIX feature, so I set the > > msix_capable of pci_epc_features struct is false, but in other > > platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX > feature in ls1088a, it is not OK, so I changed to another way. Thanks. > > Right, so the existing pci-layerscape-ep.c driver never supported MSIX yet it > erroneously had a switch case statement to call dw_pcie_ep_raise_msix_irq > which would never get used. > > Now that we're adding a platform with MSIX support the existing > dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are adding > a different method. > > Given that dw_pcie_ep_raise_msix_irq is used by pcie-designware-plat.c we > can assume this function at least works for it's use case. > > Please update the commit message - It would be helpful to suggest that > dw_pcie_ep_raise_msix_irq was never called in the exisitng driver because > msix_capable was always set to false. Agree, this is much clearer, I will modify the commit message in the next version patch, thanks a lot. > > Thanks, > > Andrew Murray > > > > > > > > > Thanks, > > > > > > Andrew Murray > > > > > > > Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com> > > > > --- > > > > v2: > > > > - No change. > > > > > > > > drivers/pci/controller/dwc/pci-layerscape-ep.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c > > > > b/drivers/pci/controller/dwc/pci-layerscape-ep.c > > > > index 8461f62..7ca5fe8 100644 > > > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c > > > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c > > > > @@ -74,7 +74,8 @@ static int ls_pcie_ep_raise_irq(struct > > > > dw_pcie_ep *ep, > > > u8 func_no, > > > > case PCI_EPC_IRQ_MSI: > > > > return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); > > > > case PCI_EPC_IRQ_MSIX: > > > > - return dw_pcie_ep_raise_msix_irq(ep, func_no, > interrupt_num); > > > > + return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no, > > > > + interrupt_num); > > > > default: > > > > dev_err(pci->dev, "UNKNOWN IRQ type\n"); > > > > return -EINVAL; > > > > -- > > > > 2.9.5 > > > >
Gustavo, On 27/08/19 6:55 PM, Andrew Murray wrote: > On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: >> >> >>> -----Original Message----- >>> From: Andrew Murray <andrew.murray@arm.com> >>> Sent: 2019年8月23日 21:58 >>> To: Xiaowei Bao <xiaowei.bao@nxp.com> >>> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; >>> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; >>> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. >>> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy >>> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; >>> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; >>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >>> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org >>> Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the >>> doorbell way >>> >>> On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: >>>> The layerscape platform use the doorbell way to trigger MSIX interrupt >>>> in EP mode. >>>> >>> >>> I have no problems with this patch, however... >>> >>> Are you able to add to this message a reason for why you are making this >>> change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did >>> it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient? >> >> The fact is that, this driver is verified in ls1046a platform of NXP before, and ls1046a don't >> support MSIX feature, so I set the msix_capable of pci_epc_features struct is false, >> but in other platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX >> feature in ls1088a, it is not OK, so I changed to another way. Thanks. > > Right, so the existing pci-layerscape-ep.c driver never supported MSIX yet it > erroneously had a switch case statement to call dw_pcie_ep_raise_msix_irq which > would never get used. > > Now that we're adding a platform with MSIX support the existing > dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are adding a > different method. Gustavo, can you confirm dw_pcie_ep_raise_msix_irq() works for designware as it didn't work for both me and Xiaowei? Thanks Kishon
On Thu, Aug 29, 2019 at 10:43:18AM +0530, Kishon Vijay Abraham I wrote: > Gustavo, > > On 27/08/19 6:55 PM, Andrew Murray wrote: > > On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: > >> > >> > >>> -----Original Message----- > >>> From: Andrew Murray <andrew.murray@arm.com> > >>> Sent: 2019年8月23日 21:58 > >>> To: Xiaowei Bao <xiaowei.bao@nxp.com> > >>> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > >>> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > >>> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. > >>> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > >>> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > >>> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > >>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > >>> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org > >>> Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the > >>> doorbell way > >>> > >>> On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > >>>> The layerscape platform use the doorbell way to trigger MSIX interrupt > >>>> in EP mode. > >>>> > >>> > >>> I have no problems with this patch, however... > >>> > >>> Are you able to add to this message a reason for why you are making this > >>> change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did > >>> it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient? > >> > >> The fact is that, this driver is verified in ls1046a platform of NXP before, and ls1046a don't > >> support MSIX feature, so I set the msix_capable of pci_epc_features struct is false, > >> but in other platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX > >> feature in ls1088a, it is not OK, so I changed to another way. Thanks. > > > > Right, so the existing pci-layerscape-ep.c driver never supported MSIX yet it > > erroneously had a switch case statement to call dw_pcie_ep_raise_msix_irq which > > would never get used. > > > > Now that we're adding a platform with MSIX support the existing > > dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are adding a > > different method. > > Gustavo, can you confirm dw_pcie_ep_raise_msix_irq() works for > designware as it didn't work for both me and Xiaowei? This question needs an answer. Thanks, Lorenzo
> -----Original Message----- > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: 2019年11月5日 20:38 > To: Kishon Vijay Abraham I <kishon@ti.com> > Cc: Andrew Murray <andrew.murray@arm.com>; Xiaowei Bao > <xiaowei.bao@nxp.com>; gustavo.pimentel@synopsys.com; > bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > linux-pci@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the > doorbell way > > On Thu, Aug 29, 2019 at 10:43:18AM +0530, Kishon Vijay Abraham I wrote: > > Gustavo, > > > > On 27/08/19 6:55 PM, Andrew Murray wrote: > > > On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: > > >> > > >> > > >>> -----Original Message----- > > >>> From: Andrew Murray <andrew.murray@arm.com> > > >>> Sent: 2019年8月23日 21:58 > > >>> To: Xiaowei Bao <xiaowei.bao@nxp.com> > > >>> Cc: bhelgaas@google.com; robh+dt@kernel.org; > mark.rutland@arm.com; > > >>> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > > >>> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; > M.h. > > >>> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; > Roy > > >>> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > > >>> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > > >>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > >>> linux-arm-kernel@lists.infradead.org; > > >>> linuxppc-dev@lists.ozlabs.org > > >>> Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to > > >>> the doorbell way > > >>> > > >>> On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > > >>>> The layerscape platform use the doorbell way to trigger MSIX > > >>>> interrupt in EP mode. > > >>>> > > >>> > > >>> I have no problems with this patch, however... > > >>> > > >>> Are you able to add to this message a reason for why you are > > >>> making this change? Did dw_pcie_ep_raise_msix_irq not work when > > >>> func_no != 0? Or did it work yet dw_pcie_ep_raise_msix_irq_doorbell is > more efficient? > > >> > > >> The fact is that, this driver is verified in ls1046a platform of > > >> NXP before, and ls1046a don't support MSIX feature, so I set the > > >> msix_capable of pci_epc_features struct is false, but in other > > >> platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX > feature in ls1088a, it is not OK, so I changed to another way. Thanks. > > > > > > Right, so the existing pci-layerscape-ep.c driver never supported > > > MSIX yet it erroneously had a switch case statement to call > > > dw_pcie_ep_raise_msix_irq which would never get used. > > > > > > Now that we're adding a platform with MSIX support the existing > > > dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are > > > adding a different method. > > > > Gustavo, can you confirm dw_pcie_ep_raise_msix_irq() works for > > designware as it didn't work for both me and Xiaowei? > > This question needs an answer. This question have answered by Gustavo in "[PATCH v3 08/11] PCI: layerscape: Modify the MSIX to the doorbell mode", please refer to the comments, due to add a new patch for this sets of patch, this patch number changed from 07/10 of v2 to 08/11 of v3, sorry for causing your confuse. Thanks Xiaowei > > Thanks, > Lorenzo
On Thu, Aug 29, 2019 at 6:13:18, Kishon Vijay Abraham I <kishon@ti.com> wrote: Hi, this email slip away from my attention... > Gustavo, > > On 27/08/19 6:55 PM, Andrew Murray wrote: > > On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: > >> > >> > >>> -----Original Message----- > >>> From: Andrew Murray <andrew.murray@arm.com> > >>> Sent: 2019年8月23日 21:58 > >>> To: Xiaowei Bao <xiaowei.bao@nxp.com> > >>> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > >>> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > >>> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. > >>> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > >>> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > >>> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > >>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > >>> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org > >>> Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the > >>> doorbell way > >>> > >>> On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > >>>> The layerscape platform use the doorbell way to trigger MSIX interrupt > >>>> in EP mode. > >>>> > >>> > >>> I have no problems with this patch, however... > >>> > >>> Are you able to add to this message a reason for why you are making this > >>> change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did > >>> it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient? > >> > >> The fact is that, this driver is verified in ls1046a platform of NXP before, and ls1046a don't > >> support MSIX feature, so I set the msix_capable of pci_epc_features struct is false, > >> but in other platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX > >> feature in ls1088a, it is not OK, so I changed to another way. Thanks. > > > > Right, so the existing pci-layerscape-ep.c driver never supported MSIX yet it > > erroneously had a switch case statement to call dw_pcie_ep_raise_msix_irq which > > would never get used. > > > > Now that we're adding a platform with MSIX support the existing > > dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are adding a > > different method. > > Gustavo, can you confirm dw_pcie_ep_raise_msix_irq() works for designware as it > didn't work for both me and Xiaowei? When I implemented the dw_pcie_ep_raise_msix_irq(), the implementation was working quite fine on DesignWare solution. Otherwise, I wouldn't submit it to the kernel. From what I have seen and if I recall well, Xiaowei implementation was done having PF's configurated on his solution, which is a configuration that I don't have in my solution, I believe this could be the missing piece that differs between our 2 implementations. Since patch submission into the kernel related to msix feature on pcitest tool, I didn't touch or re-tested the msix feature by lack of time (other projects requires my full attention for now). However is on my roadmap to came back to add some other features on DesignWare eDMA driver and I can do at that time some tests to see if the dw_pcie_ep_raise_msix_irq_doorbell() is compatible or not with my solution. If so, I can do some patch to simplify and use the dw_pcie_ep_raise_msix_irq_doorbell() if it still works as expected like on dw_pcie_ep_raise_msix_irq(). Agree? Gustavo > > Thanks > Kishon
> -----Original Message----- > From: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com> > Sent: 2019年11月6日 17:40 > To: Kishon Vijay Abraham I <kishon@ti.com>; Andrew Murray > <andrew.murray@arm.com>; Xiaowei Bao <xiaowei.bao@nxp.com>; > gustavo.pimentel@synopsys.com > Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. > Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > linux-pci@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linuxppc-dev@lists.ozlabs.org > Subject: RE: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the > doorbell way > > On Thu, Aug 29, 2019 at 6:13:18, Kishon Vijay Abraham I <kishon@ti.com> > wrote: > > Hi, this email slip away from my attention... > > > Gustavo, > > > > On 27/08/19 6:55 PM, Andrew Murray wrote: > > > On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: > > >> > > >> > > >>> -----Original Message----- > > >>> From: Andrew Murray <andrew.murray@arm.com> > > >>> Sent: 2019年8月23日 21:58 > > >>> To: Xiaowei Bao <xiaowei.bao@nxp.com> > > >>> Cc: bhelgaas@google.com; robh+dt@kernel.org; > mark.rutland@arm.com; > > >>> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > > >>> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; > M.h. > > >>> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; > Roy > > >>> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > > >>> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > > >>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > >>> linux-arm-kernel@lists.infradead.org; > > >>> linuxppc-dev@lists.ozlabs.org > > >>> Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to > > >>> the doorbell way > > >>> > > >>> On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > > >>>> The layerscape platform use the doorbell way to trigger MSIX > > >>>> interrupt in EP mode. > > >>>> > > >>> > > >>> I have no problems with this patch, however... > > >>> > > >>> Are you able to add to this message a reason for why you are > > >>> making this change? Did dw_pcie_ep_raise_msix_irq not work when > > >>> func_no != 0? Or did it work yet dw_pcie_ep_raise_msix_irq_doorbell is > more efficient? > > >> > > >> The fact is that, this driver is verified in ls1046a platform of > > >> NXP before, and ls1046a don't support MSIX feature, so I set the > > >> msix_capable of pci_epc_features struct is false, but in other > > >> platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX > feature in ls1088a, it is not OK, so I changed to another way. Thanks. > > > > > > Right, so the existing pci-layerscape-ep.c driver never supported > > > MSIX yet it erroneously had a switch case statement to call > > > dw_pcie_ep_raise_msix_irq which would never get used. > > > > > > Now that we're adding a platform with MSIX support the existing > > > dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are > > > adding a different method. > > > > Gustavo, can you confirm dw_pcie_ep_raise_msix_irq() works for > > designware as it didn't work for both me and Xiaowei? > > When I implemented the dw_pcie_ep_raise_msix_irq(), the implementation > was working quite fine on DesignWare solution. Otherwise, I wouldn't submit > it to the kernel. > From what I have seen and if I recall well, Xiaowei implementation was done > having PF's configurated on his solution, which is a configuration that I don't > have in my solution, I believe this could be the missing piece that differs > between our 2 implementations. > > Since patch submission into the kernel related to msix feature on pcitest tool, I > didn't touch or re-tested the msix feature by lack of time (other projects > requires my full attention for now). However is on my roadmap to came back > to add some other features on DesignWare eDMA driver and I can do at that > time some tests to see if the > dw_pcie_ep_raise_msix_irq_doorbell() is compatible or not with my solution. > If so, I can do some patch to simplify and use the > dw_pcie_ep_raise_msix_irq_doorbell() if it still works as expected like on > dw_pcie_ep_raise_msix_irq(). Agree? > Thanks Gustavo, it looks well for me. Thanks Xiaowei > Gustavo > > > > > Thanks > > Kishon >
Gustavo, On 06/11/19 3:10 PM, Gustavo Pimentel wrote: > On Thu, Aug 29, 2019 at 6:13:18, Kishon Vijay Abraham I <kishon@ti.com> > wrote: > > Hi, this email slip away from my attention... > >> Gustavo, >> >> On 27/08/19 6:55 PM, Andrew Murray wrote: >>> On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Andrew Murray <andrew.murray@arm.com> >>>>> Sent: 2019年8月23日 21:58 >>>>> To: Xiaowei Bao <xiaowei.bao@nxp.com> >>>>> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; >>>>> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; >>>>> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. >>>>> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy >>>>> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; >>>>> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; >>>>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >>>>> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org >>>>> Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the >>>>> doorbell way >>>>> >>>>> On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: >>>>>> The layerscape platform use the doorbell way to trigger MSIX interrupt >>>>>> in EP mode. >>>>>> >>>>> >>>>> I have no problems with this patch, however... >>>>> >>>>> Are you able to add to this message a reason for why you are making this >>>>> change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did >>>>> it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient? >>>> >>>> The fact is that, this driver is verified in ls1046a platform of NXP before, and ls1046a don't >>>> support MSIX feature, so I set the msix_capable of pci_epc_features struct is false, >>>> but in other platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX >>>> feature in ls1088a, it is not OK, so I changed to another way. Thanks. >>> >>> Right, so the existing pci-layerscape-ep.c driver never supported MSIX yet it >>> erroneously had a switch case statement to call dw_pcie_ep_raise_msix_irq which >>> would never get used. >>> >>> Now that we're adding a platform with MSIX support the existing >>> dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are adding a >>> different method. >> >> Gustavo, can you confirm dw_pcie_ep_raise_msix_irq() works for designware as it >> didn't work for both me and Xiaowei? > > When I implemented the dw_pcie_ep_raise_msix_irq(), the implementation > was working quite fine on DesignWare solution. Otherwise, I wouldn't > submit it to the kernel. > From what I have seen and if I recall well, Xiaowei implementation was > done having PF's configurated on his solution, which is a configuration > that I don't have in my solution, I believe this could be the missing > piece that differs between our 2 implementations. I haven't debugged the issue yet but in my understanding the MSI-X table should be in the memory (DDR) of EP system. This table will be populated by RC while configuring MSI-X (with msg address and msg data). The EP will use the populated msg address and msg data for raising MSI-X interrupt. From the dw_pcie_ep_raise_msix_irq() (copied below), nowhere the MSI-X table is being read from the memory of EP system. I've given my comments below. int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, u16 interrupt_num) { . . reg = PCI_BASE_ADDRESS_0 + (4 * bir); bar_addr_upper = 0; bar_addr_lower = dw_pcie_readl_dbi(pci, reg); BAR register will hold the "PCI address" programmed by the host. So "bar_addr_lower" will have PCI address. reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK); if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64) bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4); tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower; The "tbl_addr" now has the PCI address programmed by the host. tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE)); tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK; msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr, PCI_MSIX_ENTRY_SIZE); "ep->phys_base" will have EPs outbound memory address and "tbl_addr" will have PCI address. So msix_tbl points to the EPs outbound memory region. if (!msix_tbl) return -EINVAL; msg_addr_lower = readl(msix_tbl + PCI_MSIX_ENTRY_LOWER_ADDR); msg_addr_upper = readl(msix_tbl + PCI_MSIX_ENTRY_UPPER_ADDR); Here an access to the EP outbound region is made (and the transaction will be based on ATU configuration). The message address should ideally be obtained from the MSI-X table present in the EP system. There need not be any access to the OB region for getting data from MSI-X table. msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower; msg_data = readl(msix_tbl + PCI_MSIX_ENTRY_DATA); vec_ctrl = readl(msix_tbl + PCI_MSIX_ENTRY_VECTOR_CTRL); All this should be obtained from the memory of EP. . . } I'm not sure how this worked for you. Thanks Kishon > > Since patch submission into the kernel related to msix feature on pcitest > tool, I didn't touch or re-tested the msix feature by lack of time (other > projects requires my full attention for now). However is on my roadmap to > came back to add some other features on DesignWare eDMA driver and I can > do at that time some tests to see if the > dw_pcie_ep_raise_msix_irq_doorbell() is compatible or not with my > solution. If so, I can do some patch to simplify and use the > dw_pcie_ep_raise_msix_irq_doorbell() if it still works as expected like > on dw_pcie_ep_raise_msix_irq(). Agree? > > Gustavo > >> >> Thanks >> Kishon > >
On Wed, Nov 6, 2019 at 13:39:5, Kishon Vijay Abraham I <kishon@ti.com> wrote: Hi Kishon, > Gustavo, > > On 06/11/19 3:10 PM, Gustavo Pimentel wrote: > > On Thu, Aug 29, 2019 at 6:13:18, Kishon Vijay Abraham I <kishon@ti.com> > > wrote: > > > > Hi, this email slip away from my attention... > > > >> Gustavo, > >> > >> On 27/08/19 6:55 PM, Andrew Murray wrote: > >>> On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote: > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: Andrew Murray <andrew.murray@arm.com> > >>>>> Sent: 2019年8月23日 21:58 > >>>>> To: Xiaowei Bao <xiaowei.bao@nxp.com> > >>>>> Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com; > >>>>> shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; kishon@ti.com; > >>>>> lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h. > >>>>> Lian <minghuan.lian@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; Roy > >>>>> Zang <roy.zang@nxp.com>; jingoohan1@gmail.com; > >>>>> gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org; > >>>>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > >>>>> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org > >>>>> Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the > >>>>> doorbell way > >>>>> > >>>>> On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote: > >>>>>> The layerscape platform use the doorbell way to trigger MSIX interrupt > >>>>>> in EP mode. > >>>>>> > >>>>> > >>>>> I have no problems with this patch, however... > >>>>> > >>>>> Are you able to add to this message a reason for why you are making this > >>>>> change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did > >>>>> it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient? > >>>> > >>>> The fact is that, this driver is verified in ls1046a platform of NXP before, and ls1046a don't > >>>> support MSIX feature, so I set the msix_capable of pci_epc_features struct is false, > >>>> but in other platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX > >>>> feature in ls1088a, it is not OK, so I changed to another way. Thanks. > >>> > >>> Right, so the existing pci-layerscape-ep.c driver never supported MSIX yet it > >>> erroneously had a switch case statement to call dw_pcie_ep_raise_msix_irq which > >>> would never get used. > >>> > >>> Now that we're adding a platform with MSIX support the existing > >>> dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are adding a > >>> different method. > >> > >> Gustavo, can you confirm dw_pcie_ep_raise_msix_irq() works for designware as it > >> didn't work for both me and Xiaowei? > > > > When I implemented the dw_pcie_ep_raise_msix_irq(), the implementation > > was working quite fine on DesignWare solution. Otherwise, I wouldn't > > submit it to the kernel. > > From what I have seen and if I recall well, Xiaowei implementation was > > done having PF's configurated on his solution, which is a configuration > > that I don't have in my solution, I believe this could be the missing > > piece that differs between our 2 implementations. > > I haven't debugged the issue yet but in my understanding the MSI-X table should > be in the memory (DDR) of EP system. This table will be populated by RC while > configuring MSI-X (with msg address and msg data). The EP will use the > populated msg address and msg data for raising MSI-X interrupt. Right. > > From the dw_pcie_ep_raise_msix_irq() (copied below), nowhere the MSI-X table is > being read from the memory of EP system. I've given my comments below. > > int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, > u16 interrupt_num) > { > . > . > reg = PCI_BASE_ADDRESS_0 + (4 * bir); > bar_addr_upper = 0; > bar_addr_lower = dw_pcie_readl_dbi(pci, reg); > > BAR register will hold the "PCI address" programmed by the host. So > "bar_addr_lower" will have PCI address. > > reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK); > if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64) > bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4); > > tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower; > > The "tbl_addr" now has the PCI address programmed by the host. > > tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE)); > tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK; > > msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr, > PCI_MSIX_ENTRY_SIZE); > > "ep->phys_base" will have EPs outbound memory address and "tbl_addr" will have > PCI address. So msix_tbl points to the EPs outbound memory region. > if (!msix_tbl) > return -EINVAL; > > msg_addr_lower = readl(msix_tbl + PCI_MSIX_ENTRY_LOWER_ADDR); > msg_addr_upper = readl(msix_tbl + PCI_MSIX_ENTRY_UPPER_ADDR); > > Here an access to the EP outbound region is made (and the transaction will be > based on ATU configuration). > The message address should ideally be obtained from the MSI-X table present in > the EP system. There need not be any access to the OB region for getting data > from MSI-X table. > > msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower; > msg_data = readl(msix_tbl + PCI_MSIX_ENTRY_DATA); > vec_ctrl = readl(msix_tbl + PCI_MSIX_ENTRY_VECTOR_CTRL); > > All this should be obtained from the memory of EP. > . > . > } > > I'm not sure how this worked for you. Hum, it was a very time ago that I implemented this. I've to debug it to give you an accurate answer. However, it very likely that my DesignWare prototype solution might have some peculiarities that could allow this type of access. At the time, nobody had the msix feature enabled that was working with pcitest EP in their SOCs that could be used to test my implementation against, at least that I know of. As soon as I get back to work on PCI, I will retest this with the other function, if it works I'll make a patch to use that function instead, I'm assuming that function is working for other SOCs, right? Gustavo > > Thanks > Kishon > > > > > Since patch submission into the kernel related to msix feature on pcitest > > tool, I didn't touch or re-tested the msix feature by lack of time (other > > projects requires my full attention for now). However is on my roadmap to > > came back to add some other features on DesignWare eDMA driver and I can > > do at that time some tests to see if the > > dw_pcie_ep_raise_msix_irq_doorbell() is compatible or not with my > > solution. If so, I can do some patch to simplify and use the > > dw_pcie_ep_raise_msix_irq_doorbell() if it still works as expected like > > on dw_pcie_ep_raise_msix_irq(). Agree? > > > > Gustavo > > > >> > >> Thanks > >> Kishon > > > >
diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c index 8461f62..7ca5fe8 100644 --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c @@ -74,7 +74,8 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no, case PCI_EPC_IRQ_MSI: return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); case PCI_EPC_IRQ_MSIX: - return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num); + return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no, + interrupt_num); default: dev_err(pci->dev, "UNKNOWN IRQ type\n"); return -EINVAL;
The layerscape platform use the doorbell way to trigger MSIX interrupt in EP mode. Signed-off-by: Xiaowei Bao <xiaowei.bao@nxp.com> --- v2: - No change. drivers/pci/controller/dwc/pci-layerscape-ep.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)