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 <chenya...@huawei.com>
>> ---
>> 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 0x02000000>;
>> 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 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(&pdev->dev, "failed to get msi irq\n");
>
> s/msi irq/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\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 = &kirin_pcie_host_ops;
>>
>> return dw_pcie_host_init(&pci->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
