[PATCH v2 0/2] Add MSI support

[PATCH v2 0/2] Add MSI support

[Yao Chen]

Changes between V2 and V1
=========================
1. seperate DT binding patch.
2. fix issues according to review comments
   (1)from Bjorn Helgaas: Update the style of subject and changelog.
   (2)from Bjorn Helgaas: Add msi irq number in the message.
   (3)from Bjorn Helgaas: Delete unnecessary code 'pci->pp.root_bus_nr = -1'.
   (4)from Dmitry Shmidt: Fix typing error. Replace 'interrupts-names' of 'interrupt-names'.

Yao Chen (2):
  PCI: kirin: Add MSI support
  arm64: dts: hi3660: Add pcie msi interrupt attribute

 arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
 drivers/pci/dwc/pcie-kirin.c              | 39 +++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

-- 
1.9.1

[PATCH v2 1/2] PCI: kirin: Add MSI support

[Yao Chen]

Add support for MSI.

Signed-off-by: Yao Chen <chenyao11@huawei.com>
---
 drivers/pci/dwc/pcie-kirin.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
index d2970a0..4328efe 100644
--- a/drivers/pci/dwc/pcie-kirin.c
+++ b/drivers/pci/dwc/pcie-kirin.c
@@ -426,9 +426,28 @@ static int kirin_pcie_establish_link(struct pcie_port *pp)
 	return 0;
 }
 
+static irqreturn_t kirin_pcie_msi_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+
+	return dw_handle_msi_irq(pp);
+}
+
+static void kirin_pcie_msi_init(struct pcie_port *pp)
+{
+	dw_pcie_msi_init(pp);
+}
+
+static void kirin_pcie_enable_interrupts(struct pcie_port *pp)
+{
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		kirin_pcie_msi_init(pp);
+}
+
 static int kirin_pcie_host_init(struct pcie_port *pp)
 {
 	kirin_pcie_establish_link(pp);
+	kirin_pcie_enable_interrupts(pp);
 
 	return 0;
 }
@@ -448,6 +467,26 @@ static int kirin_pcie_host_init(struct pcie_port *pp)
 static int __init kirin_add_pcie_port(struct dw_pcie *pci,
 				      struct platform_device *pdev)
 {
+	int ret;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		pci->pp.msi_irq = platform_get_irq(pdev, 0);
+		if (!pci->pp.msi_irq) {
+			dev_err(&pdev->dev, "failed to get msi irq[%d]\n",
+				pci->pp.msi_irq);
+			return -ENODEV;
+		}
+		ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq,
+				       kirin_pcie_msi_irq_handler,
+				       IRQF_SHARED | IRQF_NO_THREAD,
+				       "kirin_pcie_msi", &pci->pp);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request msi irq[%d]\n",
+				pci->pp.msi_irq);
+			return ret;
+		}
+	}
+
 	pci->pp.ops = &kirin_pcie_host_ops;
 
 	return dw_pcie_host_init(&pci->pp);
-- 
1.9.1

[PATCH v2 2/2] arm64: dts: hi3660: Add pcie msi interrupt attribute

[Yao Chen]

Add pcie msi interrupt attribute for hi3660 SOC.

Signed-off-by: Yao Chen <chenyao11@huawei.com>
---
 arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
index ec3eb8e..2cef8f4 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
@@ -872,6 +872,8 @@
 				  0x0 0x02000000>;
 			num-lanes = <1>;
 			#interrupt-cells = <1>;
+			interrupts = <0 283 4>;
+			interrupt-names = "msi";
 			interrupt-map-mask = <0xf800 0 0 7>;
 			interrupt-map = <0x0 0 0 1
 					 &gic GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
-- 
1.9.1

Re: [PATCH v2 1/2] PCI: kirin: Add MSI support

[Bjorn Helgaas]

[+cc Tejun, Wolfram]

On Wed, May 09, 2018 at 04:20:11PM +0800, Yao Chen wrote:
> Add support for MSI.
> ...

> @@ -448,6 +467,26 @@ static int kirin_pcie_host_init(struct pcie_port *pp)
>  static int __init kirin_add_pcie_port(struct dw_pcie *pci,
>  				      struct platform_device *pdev)
>  {
> +	int ret;
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		pci->pp.msi_irq = platform_get_irq(pdev, 0);
> +		if (!pci->pp.msi_irq) {

I think this test is incorrect.  platform_get_irq() returns a negative
errno value when it fails.  Most calls test "irq < 0" to check for failure.

There's a lot of duplicated code like this, so maybe we should consider
putting that check into devm_request_irq(), similar to what
devm_ioremap_resource() does, so the driver code could look like this:

  pci->pp.msi_irq = platform_get_irq(pdev, 0);
  ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, ...);
  if (ret) {
    dev_err(&pdev->dev, "failed to request MSI IRQ\n");
    return ret;
  }

The basic devm_ioremap_resource() motivation is here: 72f8c0bfa0de ("lib:
devres: add convenience function to remap a resource") and the same
considerations seem to apply here.

But that's more than you need to do for *this* series.  So for now, I would
simply fix the test to check for "irq < 0" and update the messages as I
mention below.

> +			dev_err(&pdev->dev, "failed to get msi irq[%d]\n",
> +				pci->pp.msi_irq);
> +			return -ENODEV;
> +		}
> +		ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq,
> +				       kirin_pcie_msi_irq_handler,
> +				       IRQF_SHARED | IRQF_NO_THREAD,
> +				       "kirin_pcie_msi", &pci->pp);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to request msi irq[%d]\n",

s/msi irq/MSI IRQ/ in both dev_err() messages above.  This is because the
message is English text (not code), and the convention is that non-words
like these initialisms written in all caps.

I would style the first one as "failed to get MSI IRQ (%d)" because the %d
there is a return code, probably -ENXIO.

The second one should be "failed to request MSI IRQ %d" because here the %d
is the actual IRQ.

> +				pci->pp.msi_irq);
> +			return ret;
> +		}
> +	}
> +
>  	pci->pp.ops = &kirin_pcie_host_ops;
>  
>  	return dw_pcie_host_init(&pci->pp);
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 1/2] PCI: kirin: Add MSI support

[chenyao (F)]

Hi Bjorn,

> 
> [+cc Tejun, Wolfram]
> 
> On Wed, May 09, 2018 at 04:20:11PM +0800, Yao Chen wrote:
> > Add support for MSI.
> > ...
> 
> > @@ -448,6 +467,26 @@ static int kirin_pcie_host_init(struct pcie_port
> > *pp)  static int __init kirin_add_pcie_port(struct dw_pcie *pci,
> >  				      struct platform_device *pdev)  {
> > +	int ret;
> > +
> > +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +		pci->pp.msi_irq = platform_get_irq(pdev, 0);
> > +		if (!pci->pp.msi_irq) {
> 
> I think this test is incorrect.  platform_get_irq() returns a negative errno
> value when it fails.  Most calls test "irq < 0" to check for failure.
> 
> There's a lot of duplicated code like this, so maybe we should consider
> putting that check into devm_request_irq(), similar to what
> devm_ioremap_resource() does, so the driver code could look like this:
> 
>   pci->pp.msi_irq = platform_get_irq(pdev, 0);
>   ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq, ...);
>   if (ret) {
>     dev_err(&pdev->dev, "failed to request MSI IRQ\n");
>     return ret;
>   }
> 
> The basic devm_ioremap_resource() motivation is here: 72f8c0bfa0de ("lib:
> devres: add convenience function to remap a resource") and the same
> considerations seem to apply here.
> 
> But that's more than you need to do for *this* series.  So for now, I would
> simply fix the test to check for "irq < 0" and update the messages as I
> mention below.

Thank you for pointing out my mistake.  I'll fix it.

> 
> > +			dev_err(&pdev->dev, "failed to get msi irq[%d]\n",
> > +				pci->pp.msi_irq);
> > +			return -ENODEV;
> > +		}
> > +		ret = devm_request_irq(&pdev->dev, pci->pp.msi_irq,
> > +				       kirin_pcie_msi_irq_handler,
> > +				       IRQF_SHARED | IRQF_NO_THREAD,
> > +				       "kirin_pcie_msi", &pci->pp);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to request msi
> irq[%d]\n",
> 
> s/msi irq/MSI IRQ/ in both dev_err() messages above.  This is because the
> message is English text (not code), and the convention is that non-words like
> these initialisms written in all caps.
> 
> I would style the first one as "failed to get MSI IRQ (%d)" because the %d
> there is a return code, probably -ENXIO.
> 
> The second one should be "failed to request MSI IRQ %d" because here
> the %d is the actual IRQ.
> 

I'll fix it. Thanks again.

> > +				pci->pp.msi_irq);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	pci->pp.ops = &kirin_pcie_host_ops;
> >
> >  	return dw_pcie_host_init(&pci->pp);
> > --
> > 1.9.1
> >

Best regards,
Yao