Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Bjorn Helgaas]

On Wed, May 09, 2018 at 12:51:53PM +0800, Shawn Guo wrote:
> Hi Bjorn,
> 
> On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote:
> ...
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + pci->pp.root_bus_nr = -1;
> > 
> > Setting root_bus_nr looks like an unrelated change that should be in a
> > separate patch.
> > 
> > But I'm not sure why you need to set root_bus_nr at all, since
> > dw_pcie_host_init() always sets it.
> > 
> > Some other callers of dw_pcie_host_init() do set it:
> > 
> >   exynos_add_pcie_port()
> >   imx6_add_pcie_port()
> >   armada8k_add_pcie_port()
> >   artpec6_add_pcie_port()
> >   dw_plat_add_pcie_port()
> >   histb_pcie_probe()
> >   qcom_pcie_probe()
> >   spear13xx_add_pcie_port()
> > 
> > But I don't see *why* any of these need to set it.  If they don't need to
> > set it, they shouldn't.
> 
> Mostly it's a blind copy of unnecessary code.  I tested histb driver
> by dropping the line, and did not see anything broken.  I will cook up
> a series to remove the code from all above drivers, and copy
> corresponding driver owner to comment.

Thanks, Shawn, I really appreciate that!

> > And it would be nice if histb and qcom followed the structure and naming
> > conventions of the other drivers, i.e., they should have
> > histb_add_pcie_port() and qcom_add_pcie_port().
> 
> I can create a patch for histb driver, but will leave qcom one to
> Stanimir to decide.

Sounds good, thanks again!

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Bjorn Helgaas]

On Wed, May 09, 2018 at 12:51:53PM +0800, Shawn Guo wrote:
> Hi Bjorn,
> 
> On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote:
> ...
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + pci->pp.root_bus_nr = -1;
> > 
> > Setting root_bus_nr looks like an unrelated change that should be in a
> > separate patch.
> > 
> > But I'm not sure why you need to set root_bus_nr at all, since
> > dw_pcie_host_init() always sets it.
> > 
> > Some other callers of dw_pcie_host_init() do set it:
> > 
> >   exynos_add_pcie_port()
> >   imx6_add_pcie_port()
> >   armada8k_add_pcie_port()
> >   artpec6_add_pcie_port()
> >   dw_plat_add_pcie_port()
> >   histb_pcie_probe()
> >   qcom_pcie_probe()
> >   spear13xx_add_pcie_port()
> > 
> > But I don't see *why* any of these need to set it.  If they don't need to
> > set it, they shouldn't.
> 
> Mostly it's a blind copy of unnecessary code.  I tested histb driver
> by dropping the line, and did not see anything broken.  I will cook up
> a series to remove the code from all above drivers, and copy
> corresponding driver owner to comment.

Thanks, Shawn, I really appreciate that!

> > And it would be nice if histb and qcom followed the structure and naming
> > conventions of the other drivers, i.e., they should have
> > histb_add_pcie_port() and qcom_add_pcie_port().
> 
> I can create a patch for histb driver, but will leave qcom one to
> Stanimir to decide.

Sounds good, thanks again!

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Stanimir Varbanov]

Hi,

On  9.05.2018 07:51, Shawn Guo wrote:

Hi Bjorn,

On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote:
...

+   return ret;
+   }
+   }
+
+   pci->pp.root_bus_nr = -1;


Setting root_bus_nr looks like an unrelated change that should be in a
separate patch.

But I'm not sure why you need to set root_bus_nr at all, since
dw_pcie_host_init() always sets it.

Some other callers of dw_pcie_host_init() do set it:

   exynos_add_pcie_port()
   imx6_add_pcie_port()
   armada8k_add_pcie_port()
   artpec6_add_pcie_port()
   dw_plat_add_pcie_port()
   histb_pcie_probe()
   qcom_pcie_probe()
   spear13xx_add_pcie_port()

But I don't see *why* any of these need to set it.  If they don't need to
set it, they shouldn't.


Mostly it's a blind copy of unnecessary code.  I tested histb driver
by dropping the line, and did not see anything broken.  I will cook up
a series to remove the code from all above drivers, and copy
corresponding driver owner to comment.


And it would be nice if histb and qcom followed the structure and naming
conventions of the other drivers, i.e., they should have
histb_add_pcie_port() and qcom_add_pcie_port().


I can create a patch for histb driver, but will leave qcom one to
Stanimir to decide.


Thanks, will see what can be done.

regards,
Stan

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Stanimir Varbanov]

Hi,

On  9.05.2018 07:51, Shawn Guo wrote:

Hi Bjorn,

On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote:
...

+   return ret;
+   }
+   }
+
+   pci->pp.root_bus_nr = -1;


Setting root_bus_nr looks like an unrelated change that should be in a
separate patch.

But I'm not sure why you need to set root_bus_nr at all, since
dw_pcie_host_init() always sets it.

Some other callers of dw_pcie_host_init() do set it:

   exynos_add_pcie_port()
   imx6_add_pcie_port()
   armada8k_add_pcie_port()
   artpec6_add_pcie_port()
   dw_plat_add_pcie_port()
   histb_pcie_probe()
   qcom_pcie_probe()
   spear13xx_add_pcie_port()

But I don't see *why* any of these need to set it.  If they don't need to
set it, they shouldn't.


Mostly it's a blind copy of unnecessary code.  I tested histb driver
by dropping the line, and did not see anything broken.  I will cook up
a series to remove the code from all above drivers, and copy
corresponding driver owner to comment.


And it would be nice if histb and qcom followed the structure and naming
conventions of the other drivers, i.e., they should have
histb_add_pcie_port() and qcom_add_pcie_port().


I can create a patch for histb driver, but will leave qcom one to
Stanimir to decide.


Thanks, will see what can be done.

regards,
Stan

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Yao Chen]

On 2018/5/8 20:56, Bjorn Helgaas wrote:
> [+cc Stanimir, Jianguo, Shawn for qcom, histb comments below]
> 
> On Tue, May 08, 2018 at 03:03:48PM +0800, Yao Chen wrote:
>> This patch adds kirin pcie msi feature.
> 
> Thanks for the patch!  Please update your subject and changelog like this:
> 
>   PCI: kirin: Add MSI support
> 
>   Add support for MSI.
> 
> You can figure out what style to use for the subject by running:
> 
>   git log --oneline drivers/pci/dwc/pcie-kirin.c
> 
> Then follow the existing style.
> 

Ok, I will update the style of subject and changelog.

>> Signed-off-by: Yao Chen 
>> ---
>>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
>>  drivers/pci/dwc/pcie-kirin.c  | 38 
>> +++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
>> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> index ec3eb8e..4ef684f 100644
>> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> @@ -872,6 +872,8 @@
>>0x0 0x0200>;
>>  num-lanes = <1>;
>>  #interrupt-cells = <1>;
>> +interrupts = <0 283 4>;
>> +interrupts-names = "msi";
>>  interrupt-map-mask = <0xf800 0 0 7>;
>>  interrupt-map = <0x0 0 0 1
>>GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
>> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
>> index d2970a0..2319c9c 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,25 @@ 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(>dev, "failed to get msi irq\n");
> 
> s/msi irq/MSI IRQ/
> 
>> +return -ENODEV;
>> +}
>> +ret = devm_request_irq(>dev, pci->pp.msi_irq,
>> +   kirin_pcie_msi_irq_handler,
>> +   IRQF_SHARED | IRQF_NO_THREAD,
>> +   "kirin_pcie_msi", >pp);
>> +if (ret) {
>> +dev_err(>dev, "failed to request msi irq\n");
> 
> s/msi irq/MSI IRQ/
> Also, please include the IRQ number (pci->pp.msi_irq) in the message.
> 
Ok, I will add msi_irq message.

>> +return ret;
>> +}
>> +}
>> +
>> +pci->pp.root_bus_nr = -1;
> 
> Setting root_bus_nr looks like an unrelated change that should be in a
> separate patch.
> 
> But I'm not sure why you need to set root_bus_nr at all, since
> dw_pcie_host_init() always sets it.
> 
> Some other callers of dw_pcie_host_init() do set it:
> 
>   exynos_add_pcie_port()
>   imx6_add_pcie_port()
>   armada8k_add_pcie_port()
>   artpec6_add_pcie_port()
>   dw_plat_add_pcie_port()
>   histb_pcie_probe()
>   qcom_pcie_probe()
>   spear13xx_add_pcie_port()
> 
> But I don't see *why* any of these need to set it.  If they don't need to
> set it, they shouldn't.
> 
> And it would be nice if histb and qcom followed the structure and naming
> conventions of the other drivers, i.e., they should have
> histb_add_pcie_port() and qcom_add_pcie_port().
> 

You're right, "pci->pp.root_bus_nr = -1;" is truly unnecessary. I will delete 
it.

>>  pci->pp.ops = _pcie_host_ops;
>>  
>>  return dw_pcie_host_init(>pp);
>> -- 
>> 1.9.1
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> .
> 
Thanks,
Yao

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Yao Chen]

On 2018/5/8 20:56, Bjorn Helgaas wrote:
> [+cc Stanimir, Jianguo, Shawn for qcom, histb comments below]
> 
> On Tue, May 08, 2018 at 03:03:48PM +0800, Yao Chen wrote:
>> This patch adds kirin pcie msi feature.
> 
> Thanks for the patch!  Please update your subject and changelog like this:
> 
>   PCI: kirin: Add MSI support
> 
>   Add support for MSI.
> 
> You can figure out what style to use for the subject by running:
> 
>   git log --oneline drivers/pci/dwc/pcie-kirin.c
> 
> Then follow the existing style.
> 

Ok, I will update the style of subject and changelog.

>> Signed-off-by: Yao Chen 
>> ---
>>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
>>  drivers/pci/dwc/pcie-kirin.c  | 38 
>> +++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
>> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> index ec3eb8e..4ef684f 100644
>> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> @@ -872,6 +872,8 @@
>>0x0 0x0200>;
>>  num-lanes = <1>;
>>  #interrupt-cells = <1>;
>> +interrupts = <0 283 4>;
>> +interrupts-names = "msi";
>>  interrupt-map-mask = <0xf800 0 0 7>;
>>  interrupt-map = <0x0 0 0 1
>>GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
>> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
>> index d2970a0..2319c9c 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,25 @@ 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(>dev, "failed to get msi irq\n");
> 
> s/msi irq/MSI IRQ/
> 
>> +return -ENODEV;
>> +}
>> +ret = devm_request_irq(>dev, pci->pp.msi_irq,
>> +   kirin_pcie_msi_irq_handler,
>> +   IRQF_SHARED | IRQF_NO_THREAD,
>> +   "kirin_pcie_msi", >pp);
>> +if (ret) {
>> +dev_err(>dev, "failed to request msi irq\n");
> 
> s/msi irq/MSI IRQ/
> Also, please include the IRQ number (pci->pp.msi_irq) in the message.
> 
Ok, I will add msi_irq message.

>> +return ret;
>> +}
>> +}
>> +
>> +pci->pp.root_bus_nr = -1;
> 
> Setting root_bus_nr looks like an unrelated change that should be in a
> separate patch.
> 
> But I'm not sure why you need to set root_bus_nr at all, since
> dw_pcie_host_init() always sets it.
> 
> Some other callers of dw_pcie_host_init() do set it:
> 
>   exynos_add_pcie_port()
>   imx6_add_pcie_port()
>   armada8k_add_pcie_port()
>   artpec6_add_pcie_port()
>   dw_plat_add_pcie_port()
>   histb_pcie_probe()
>   qcom_pcie_probe()
>   spear13xx_add_pcie_port()
> 
> But I don't see *why* any of these need to set it.  If they don't need to
> set it, they shouldn't.
> 
> And it would be nice if histb and qcom followed the structure and naming
> conventions of the other drivers, i.e., they should have
> histb_add_pcie_port() and qcom_add_pcie_port().
> 

You're right, "pci->pp.root_bus_nr = -1;" is truly unnecessary. I will delete 
it.

>>  pci->pp.ops = _pcie_host_ops;
>>  
>>  return dw_pcie_host_init(>pp);
>> -- 
>> 1.9.1
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> .
> 
Thanks,
Yao

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Shawn Guo]

Hi Bjorn,

On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote:
...
> > +   return ret;
> > +   }
> > +   }
> > +
> > +   pci->pp.root_bus_nr = -1;
> 
> Setting root_bus_nr looks like an unrelated change that should be in a
> separate patch.
> 
> But I'm not sure why you need to set root_bus_nr at all, since
> dw_pcie_host_init() always sets it.
> 
> Some other callers of dw_pcie_host_init() do set it:
> 
>   exynos_add_pcie_port()
>   imx6_add_pcie_port()
>   armada8k_add_pcie_port()
>   artpec6_add_pcie_port()
>   dw_plat_add_pcie_port()
>   histb_pcie_probe()
>   qcom_pcie_probe()
>   spear13xx_add_pcie_port()
> 
> But I don't see *why* any of these need to set it.  If they don't need to
> set it, they shouldn't.

Mostly it's a blind copy of unnecessary code.  I tested histb driver
by dropping the line, and did not see anything broken.  I will cook up
a series to remove the code from all above drivers, and copy
corresponding driver owner to comment.

> And it would be nice if histb and qcom followed the structure and naming
> conventions of the other drivers, i.e., they should have
> histb_add_pcie_port() and qcom_add_pcie_port().

I can create a patch for histb driver, but will leave qcom one to
Stanimir to decide.

Shawn

> 
> > pci->pp.ops = _pcie_host_ops;
> >  
> > return dw_pcie_host_init(>pp);
> > -- 
> > 1.9.1
> > 
> > 
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Shawn Guo]

Hi Bjorn,

On Tue, May 08, 2018 at 07:56:58AM -0500, Bjorn Helgaas wrote:
...
> > +   return ret;
> > +   }
> > +   }
> > +
> > +   pci->pp.root_bus_nr = -1;
> 
> Setting root_bus_nr looks like an unrelated change that should be in a
> separate patch.
> 
> But I'm not sure why you need to set root_bus_nr at all, since
> dw_pcie_host_init() always sets it.
> 
> Some other callers of dw_pcie_host_init() do set it:
> 
>   exynos_add_pcie_port()
>   imx6_add_pcie_port()
>   armada8k_add_pcie_port()
>   artpec6_add_pcie_port()
>   dw_plat_add_pcie_port()
>   histb_pcie_probe()
>   qcom_pcie_probe()
>   spear13xx_add_pcie_port()
> 
> But I don't see *why* any of these need to set it.  If they don't need to
> set it, they shouldn't.

Mostly it's a blind copy of unnecessary code.  I tested histb driver
by dropping the line, and did not see anything broken.  I will cook up
a series to remove the code from all above drivers, and copy
corresponding driver owner to comment.

> And it would be nice if histb and qcom followed the structure and naming
> conventions of the other drivers, i.e., they should have
> histb_add_pcie_port() and qcom_add_pcie_port().

I can create a patch for histb driver, but will leave qcom one to
Stanimir to decide.

Shawn

> 
> > pci->pp.ops = _pcie_host_ops;
> >  
> > return dw_pcie_host_init(>pp);
> > -- 
> > 1.9.1
> > 
> > 
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Yao Chen]

On 2018/5/9 5:31, Dmitry Shmidt wrote:
> On Tue, May 8, 2018 at 12:03 AM, Yao Chen  wrote:
>> This patch adds kirin pcie msi feature.
>>
>> Signed-off-by: Yao Chen 
>> ---
>>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
>>  drivers/pci/dwc/pcie-kirin.c  | 38 
>> +++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
>> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> index ec3eb8e..4ef684f 100644
>> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> @@ -872,6 +872,8 @@
>>   0x0 0x0200>;
>> num-lanes = <1>;
>> #interrupt-cells = <1>;
>> +   interrupts = <0 283 4>;
>> +   interrupts-names = "msi";
> 
> Possible typo? Should be interrupt-names = "msi";

Yes, you're right. It should be interrupt-names = "msi". I will fix it. Thanks.

> 
>> interrupt-map-mask = <0xf800 0 0 7>;
>> interrupt-map = <0x0 0 0 1
>>   GIC_SPI 282 
>> IRQ_TYPE_LEVEL_HIGH>,
>> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
>> index d2970a0..2319c9c 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,25 @@ 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(>dev, "failed to get msi irq\n");
>> +   return -ENODEV;
>> +   }
>> +   ret = devm_request_irq(>dev, pci->pp.msi_irq,
>> +  kirin_pcie_msi_irq_handler,
>> +  IRQF_SHARED | IRQF_NO_THREAD,
>> +  "kirin_pcie_msi", >pp);
>> +   if (ret) {
>> +   dev_err(>dev, "failed to request msi irq\n");
>> +   return ret;
>> +   }
>> +   }
>> +
>> +   pci->pp.root_bus_nr = -1;
>> pci->pp.ops = _pcie_host_ops;
>>
>> return dw_pcie_host_init(>pp);
>> --
>> 1.9.1
>>
> 
> .
> 

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Yao Chen]

On 2018/5/9 5:31, Dmitry Shmidt wrote:
> On Tue, May 8, 2018 at 12:03 AM, Yao Chen  wrote:
>> This patch adds kirin pcie msi feature.
>>
>> Signed-off-by: Yao Chen 
>> ---
>>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
>>  drivers/pci/dwc/pcie-kirin.c  | 38 
>> +++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
>> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> index ec3eb8e..4ef684f 100644
>> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>> @@ -872,6 +872,8 @@
>>   0x0 0x0200>;
>> num-lanes = <1>;
>> #interrupt-cells = <1>;
>> +   interrupts = <0 283 4>;
>> +   interrupts-names = "msi";
> 
> Possible typo? Should be interrupt-names = "msi";

Yes, you're right. It should be interrupt-names = "msi". I will fix it. Thanks.

> 
>> interrupt-map-mask = <0xf800 0 0 7>;
>> interrupt-map = <0x0 0 0 1
>>   GIC_SPI 282 
>> IRQ_TYPE_LEVEL_HIGH>,
>> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
>> index d2970a0..2319c9c 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,25 @@ 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(>dev, "failed to get msi irq\n");
>> +   return -ENODEV;
>> +   }
>> +   ret = devm_request_irq(>dev, pci->pp.msi_irq,
>> +  kirin_pcie_msi_irq_handler,
>> +  IRQF_SHARED | IRQF_NO_THREAD,
>> +  "kirin_pcie_msi", >pp);
>> +   if (ret) {
>> +   dev_err(>dev, "failed to request msi irq\n");
>> +   return ret;
>> +   }
>> +   }
>> +
>> +   pci->pp.root_bus_nr = -1;
>> pci->pp.ops = _pcie_host_ops;
>>
>> return dw_pcie_host_init(>pp);
>> --
>> 1.9.1
>>
> 
> .
> 

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Dmitry Shmidt]

On Tue, May 8, 2018 at 12:03 AM, Yao Chen  wrote:
> This patch adds kirin pcie msi feature.
>
> Signed-off-by: Yao Chen 
> ---
>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
>  drivers/pci/dwc/pcie-kirin.c  | 38 
> +++
>  2 files changed, 40 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index ec3eb8e..4ef684f 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -872,6 +872,8 @@
>   0x0 0x0200>;
> num-lanes = <1>;
> #interrupt-cells = <1>;
> +   interrupts = <0 283 4>;
> +   interrupts-names = "msi";

Possible typo? Should be interrupt-names = "msi";

> interrupt-map-mask = <0xf800 0 0 7>;
> interrupt-map = <0x0 0 0 1
>   GIC_SPI 282 
> IRQ_TYPE_LEVEL_HIGH>,
> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
> index d2970a0..2319c9c 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,25 @@ 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(>dev, "failed to get msi irq\n");
> +   return -ENODEV;
> +   }
> +   ret = devm_request_irq(>dev, pci->pp.msi_irq,
> +  kirin_pcie_msi_irq_handler,
> +  IRQF_SHARED | IRQF_NO_THREAD,
> +  "kirin_pcie_msi", >pp);
> +   if (ret) {
> +   dev_err(>dev, "failed to request msi irq\n");
> +   return ret;
> +   }
> +   }
> +
> +   pci->pp.root_bus_nr = -1;
> pci->pp.ops = _pcie_host_ops;
>
> return dw_pcie_host_init(>pp);
> --
> 1.9.1
>

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Dmitry Shmidt]

On Tue, May 8, 2018 at 12:03 AM, Yao Chen  wrote:
> This patch adds kirin pcie msi feature.
>
> Signed-off-by: Yao Chen 
> ---
>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
>  drivers/pci/dwc/pcie-kirin.c  | 38 
> +++
>  2 files changed, 40 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index ec3eb8e..4ef684f 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -872,6 +872,8 @@
>   0x0 0x0200>;
> num-lanes = <1>;
> #interrupt-cells = <1>;
> +   interrupts = <0 283 4>;
> +   interrupts-names = "msi";

Possible typo? Should be interrupt-names = "msi";

> interrupt-map-mask = <0xf800 0 0 7>;
> interrupt-map = <0x0 0 0 1
>   GIC_SPI 282 
> IRQ_TYPE_LEVEL_HIGH>,
> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
> index d2970a0..2319c9c 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,25 @@ 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(>dev, "failed to get msi irq\n");
> +   return -ENODEV;
> +   }
> +   ret = devm_request_irq(>dev, pci->pp.msi_irq,
> +  kirin_pcie_msi_irq_handler,
> +  IRQF_SHARED | IRQF_NO_THREAD,
> +  "kirin_pcie_msi", >pp);
> +   if (ret) {
> +   dev_err(>dev, "failed to request msi irq\n");
> +   return ret;
> +   }
> +   }
> +
> +   pci->pp.root_bus_nr = -1;
> pci->pp.ops = _pcie_host_ops;
>
> return dw_pcie_host_init(>pp);
> --
> 1.9.1
>

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Bjorn Helgaas]

[+cc Stanimir, Jianguo, Shawn for qcom, histb comments below]

On Tue, May 08, 2018 at 03:03:48PM +0800, Yao Chen wrote:
> This patch adds kirin pcie msi feature.

Thanks for the patch!  Please update your subject and changelog like this:

  PCI: kirin: Add MSI support

  Add support for MSI.

You can figure out what style to use for the subject by running:

  git log --oneline drivers/pci/dwc/pcie-kirin.c

Then follow the existing style.

> Signed-off-by: Yao Chen 
> ---
>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
>  drivers/pci/dwc/pcie-kirin.c  | 38 
> +++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index ec3eb8e..4ef684f 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -872,6 +872,8 @@
> 0x0 0x0200>;
>   num-lanes = <1>;
>   #interrupt-cells = <1>;
> + interrupts = <0 283 4>;
> + interrupts-names = "msi";
>   interrupt-map-mask = <0xf800 0 0 7>;
>   interrupt-map = <0x0 0 0 1
> GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
> index d2970a0..2319c9c 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,25 @@ 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(>dev, "failed to get msi irq\n");

s/msi irq/MSI IRQ/

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

s/msi irq/MSI IRQ/
Also, please include the IRQ number (pci->pp.msi_irq) in the message.

> + return ret;
> + }
> + }
> +
> + pci->pp.root_bus_nr = -1;

Setting root_bus_nr looks like an unrelated change that should be in a
separate patch.

But I'm not sure why you need to set root_bus_nr at all, since
dw_pcie_host_init() always sets it.

Some other callers of dw_pcie_host_init() do set it:

  exynos_add_pcie_port()
  imx6_add_pcie_port()
  armada8k_add_pcie_port()
  artpec6_add_pcie_port()
  dw_plat_add_pcie_port()
  histb_pcie_probe()
  qcom_pcie_probe()
  spear13xx_add_pcie_port()

But I don't see *why* any of these need to set it.  If they don't need to
set it, they shouldn't.

And it would be nice if histb and qcom followed the structure and naming
conventions of the other drivers, i.e., they should have
histb_add_pcie_port() and qcom_add_pcie_port().

>   pci->pp.ops = _pcie_host_ops;
>  
>   return dw_pcie_host_init(>pp);
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] Kirin-PCIe: Add kirin pcie msi feature.

[Bjorn Helgaas]

[+cc Stanimir, Jianguo, Shawn for qcom, histb comments below]

On Tue, May 08, 2018 at 03:03:48PM +0800, Yao Chen wrote:
> This patch adds kirin pcie msi feature.

Thanks for the patch!  Please update your subject and changelog like this:

  PCI: kirin: Add MSI support

  Add support for MSI.

You can figure out what style to use for the subject by running:

  git log --oneline drivers/pci/dwc/pcie-kirin.c

Then follow the existing style.

> Signed-off-by: Yao Chen 
> ---
>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi |  2 ++
>  drivers/pci/dwc/pcie-kirin.c  | 38 
> +++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index ec3eb8e..4ef684f 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -872,6 +872,8 @@
> 0x0 0x0200>;
>   num-lanes = <1>;
>   #interrupt-cells = <1>;
> + interrupts = <0 283 4>;
> + interrupts-names = "msi";
>   interrupt-map-mask = <0xf800 0 0 7>;
>   interrupt-map = <0x0 0 0 1
> GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
> diff --git a/drivers/pci/dwc/pcie-kirin.c b/drivers/pci/dwc/pcie-kirin.c
> index d2970a0..2319c9c 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,25 @@ 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(>dev, "failed to get msi irq\n");

s/msi irq/MSI IRQ/

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

s/msi irq/MSI IRQ/
Also, please include the IRQ number (pci->pp.msi_irq) in the message.

> + return ret;
> + }
> + }
> +
> + pci->pp.root_bus_nr = -1;

Setting root_bus_nr looks like an unrelated change that should be in a
separate patch.

But I'm not sure why you need to set root_bus_nr at all, since
dw_pcie_host_init() always sets it.

Some other callers of dw_pcie_host_init() do set it:

  exynos_add_pcie_port()
  imx6_add_pcie_port()
  armada8k_add_pcie_port()
  artpec6_add_pcie_port()
  dw_plat_add_pcie_port()
  histb_pcie_probe()
  qcom_pcie_probe()
  spear13xx_add_pcie_port()

But I don't see *why* any of these need to set it.  If they don't need to
set it, they shouldn't.

And it would be nice if histb and qcom followed the structure and naming
conventions of the other drivers, i.e., they should have
histb_add_pcie_port() and qcom_add_pcie_port().

>   pci->pp.ops = _pcie_host_ops;
>  
>   return dw_pcie_host_init(>pp);
> -- 
> 1.9.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel