[v2,07/10] PCI: layerscape: Modify the MSIX to the doorbell way
diff mbox series

Message ID 20190822112242.16309-7-xiaowei.bao@nxp.com
State Not Applicable
Headers show
Series
  • [v2,01/10] PCI: designware-ep: Add multiple PFs support for DWC
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
snowpatch_ozlabs/apply_patch success Successfully applied on branch next (0e4523c0b4f64eaf7abe59e143e6bdf8f972acff)

Commit Message

Xiaowei Bao Aug. 22, 2019, 11:22 a.m. UTC
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(-)

Comments

Andrew Murray Aug. 23, 2019, 1:58 p.m. UTC | #1
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
>
Xiaowei Bao Aug. 24, 2019, 12:08 a.m. UTC | #2
> -----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
> >
Andrew Murray Aug. 27, 2019, 1:25 p.m. UTC | #3
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
> > >
Xiaowei Bao Aug. 28, 2019, 2:49 a.m. UTC | #4
> -----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
> > > >
Kishon Vijay Abraham I Aug. 29, 2019, 5:13 a.m. UTC | #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

Patch
diff mbox series

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;