| Message ID | 1544687042-16595-1-git-send-email-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI: imx: make msi work without pcieportbus | expand |
Hi Richard, One more comment that occurred to me only now. Richard Zhu writes: > MSI_EN of iMX PCIe RC would be asserted when > PCIEPORTBUS driver is selected. > Thus, the MSI works fine on iMX PCIe before. > Assert it unconditionally when MSI is supported. > Otherwise, the MSI wouldn't be triggered although > the EP is present and the MSIs are assigned. > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 26087b3..d3e4296 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -74,6 +74,7 @@ struct imx6_pcie { > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200 > > /* PCIe Root Complex registers (memory-mapped) */ > +#define PCI_MSI_CAP 0x50 > #define PCIE_RC_LCR 0x7c > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1 > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2 0x2 > @@ -926,6 +927,7 @@ static int imx6_pcie_probe(struct platform_device *pdev) > struct resource *dbi_base; > struct device_node *node = dev->of_node; > int ret; > + u16 val; > > imx6_pcie = devm_kzalloc(dev, sizeof(*imx6_pcie), GFP_KERNEL); > if (!imx6_pcie) > @@ -1070,6 +1072,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) > ret = imx6_add_pcie_port(imx6_pcie, pdev); > if (ret < 0) > return ret; > + if (IS_ENABLED(CONFIG_PCI_MSI)) { CONFIG_PCI_IMX6 depends on CONFIG_PCI_MSI_IRQ_DOMAIN that in turn depends on CONFIG_PCI_MSI. So this condition should always be true. If so, you can drop this one as well. > + val = dw_pcie_readw_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS); > + val |= PCI_MSI_FLAGS_ENABLE; > + dw_pcie_writew_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS, val); > + } > > return 0; > } baruch
> -----Original Message----- > From: Baruch Siach [mailto:baruch@tkos.co.il] > Sent: 2018年12月13日 16:13 > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; > l.stach@pengutronix.de; andrew.smirnov@gmail.com; > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-arm-kernel@lists.infradead.org > Subject: Re: [v2] PCI: imx: make msi work without pcieportbus > > Hi Richard, > > One more comment that occurred to me only now. > > Richard Zhu writes: > > MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is > > selected. > > Thus, the MSI works fine on iMX PCIe before. > > Assert it unconditionally when MSI is supported. > > Otherwise, the MSI wouldn't be triggered although the EP is present > > and the MSIs are assigned. > > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index 26087b3..d3e4296 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -74,6 +74,7 @@ struct imx6_pcie { > > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200 > > > > /* PCIe Root Complex registers (memory-mapped) */ > > +#define PCI_MSI_CAP 0x50 > > #define PCIE_RC_LCR 0x7c > > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1 > > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2 0x2 > > @@ -926,6 +927,7 @@ static int imx6_pcie_probe(struct platform_device > *pdev) > > struct resource *dbi_base; > > struct device_node *node = dev->of_node; > > int ret; > > + u16 val; > > > > imx6_pcie = devm_kzalloc(dev, sizeof(*imx6_pcie), GFP_KERNEL); > > if (!imx6_pcie) > > @@ -1070,6 +1072,11 @@ static int imx6_pcie_probe(struct > platform_device *pdev) > > ret = imx6_add_pcie_port(imx6_pcie, pdev); > > if (ret < 0) > > return ret; > > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > > CONFIG_PCI_IMX6 depends on CONFIG_PCI_MSI_IRQ_DOMAIN that in turn > depends on CONFIG_PCI_MSI. So this condition should always be true. If so, > you can drop this one as well. [Richard Zhu] Thanks. Can we have the double check here? Thus, it's aligned to the MSI enable codes in the other place of the driver. For example: ... if (IS_ENABLED(CONFIG_PCI_MSI)) dw_pcie_msi_init(pp); ... Secondly, the driver has a better portability with the double check here. How do you think about it? > > > + val = dw_pcie_readw_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS); > > + val |= PCI_MSI_FLAGS_ENABLE; > > + dw_pcie_writew_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS, val); > > + } > > > > return 0; > > } > > baruch > > -- > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fbaru > ch.siach.name%2Fblog%2F&data=02%7C01%7Chongxing.zhu%40nxp.co > m%7Cacb95dbd3c26442df6f608d660d2c042%7C686ea1d3bc2b4c6fa92cd99 > c5c301635%7C0%7C0%7C636802855648799644&sdata=qjIi004UrbTYrT > VjDL%2B1THVCbZbTfkwTu%2BeztlE6NXY%3D&reserved=0 > ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch@tkos.co.il - tel: +972.52.368.4656, > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww > .tkos.co.il&data=02%7C01%7Chongxing.zhu%40nxp.com%7Cacb95dbd3 > c26442df6f608d660d2c042%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > %7C0%7C636802855648799644&sdata=DQOZR2ZvwkAiWmdHsGJBT7h > NwEdpQj6BMdG9vx90gSw%3D&reserved=0 -
Hi Richard, Richard Zhu writes: >> -----Original Message----- >> From: Baruch Siach [mailto:baruch@tkos.co.il] >> Sent: 2018年12月13日 16:13 >> To: Richard Zhu <hongxing.zhu@nxp.com> >> Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; >> l.stach@pengutronix.de; andrew.smirnov@gmail.com; >> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; >> linux-arm-kernel@lists.infradead.org >> Subject: Re: [v2] PCI: imx: make msi work without pcieportbus >> >> Richard Zhu writes: >> > MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is >> > selected. >> > Thus, the MSI works fine on iMX PCIe before. >> > Assert it unconditionally when MSI is supported. >> > Otherwise, the MSI wouldn't be triggered although the EP is present >> > and the MSIs are assigned. >> > >> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> >> > --- >> > drivers/pci/controller/dwc/pci-imx6.c | 7 +++++++ >> > 1 file changed, 7 insertions(+) >> > >> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c >> > b/drivers/pci/controller/dwc/pci-imx6.c >> > index 26087b3..d3e4296 100644 >> > --- a/drivers/pci/controller/dwc/pci-imx6.c >> > +++ b/drivers/pci/controller/dwc/pci-imx6.c >> > @@ -74,6 +74,7 @@ struct imx6_pcie { >> > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200 >> > >> > /* PCIe Root Complex registers (memory-mapped) */ >> > +#define PCI_MSI_CAP 0x50 >> > #define PCIE_RC_LCR 0x7c >> > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1 >> > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2 0x2 >> > @@ -926,6 +927,7 @@ static int imx6_pcie_probe(struct platform_device >> *pdev) >> > struct resource *dbi_base; >> > struct device_node *node = dev->of_node; >> > int ret; >> > + u16 val; >> > >> > imx6_pcie = devm_kzalloc(dev, sizeof(*imx6_pcie), GFP_KERNEL); >> > if (!imx6_pcie) >> > @@ -1070,6 +1072,11 @@ static int imx6_pcie_probe(struct >> platform_device *pdev) >> > ret = imx6_add_pcie_port(imx6_pcie, pdev); >> > if (ret < 0) >> > return ret; >> > + if (IS_ENABLED(CONFIG_PCI_MSI)) { >> >> CONFIG_PCI_IMX6 depends on CONFIG_PCI_MSI_IRQ_DOMAIN that in turn >> depends on CONFIG_PCI_MSI. So this condition should always be true. If so, >> you can drop this one as well. > [Richard Zhu] Thanks. > Can we have the double check here? > Thus, it's aligned to the MSI enable codes in the other place of the driver. > For example: > ... > if (IS_ENABLED(CONFIG_PCI_MSI)) > dw_pcie_msi_init(pp); > ... > Secondly, the driver has a better portability with the double check here. > How do you think about it? Kernel driver are not meant to be "portable" as far as I understand. But I'll let the PCI maintainers comment on that. For reference, mainline kernel makes CONFIG_PCI_MSI a hard dependency of pci-imx6 since commit 3ee803641e76 ("PCI/MSI: irqchip: Fix PCI_MSI dependencies") in v4.8. In my opinion, if the driver actually compiles and is usable without CONFIG_PCI_MSI then Kconfig dependencies should reflect that. >> > + val = dw_pcie_readw_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS); >> > + val |= PCI_MSI_FLAGS_ENABLE; >> > + dw_pcie_writew_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS, val); >> > + } >> > >> > return 0; >> > } baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Hi Richard, Am Donnerstag, den 13.12.2018, 08:02 +0000 schrieb Richard Zhu: > MSI_EN of iMX PCIe RC would be asserted when > PCIEPORTBUS driver is selected. > Thus, the MSI works fine on iMX PCIe before. > Assert it unconditionally when MSI is supported. > Otherwise, the MSI wouldn't be triggered although > the EP is present and the MSIs are assigned. Thanks for digging into this issue. This seems like the right way forward. However, did you test this with devices using legacy IRQs? I.e. booting with "nomsi" on the kernel command line to see if legacy IRQs still work if this bit is set, or if we need to avoid setting this when the user explicitly requests to disable MSIs? Regards, Lucas > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 26087b3..d3e4296 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -74,6 +74,7 @@ struct imx6_pcie { > > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200 > > /* PCIe Root Complex registers (memory-mapped) */ > > +#define PCI_MSI_CAP 0x50 > > #define PCIE_RC_LCR 0x7c > > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1 > > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2 0x2 > @@ -926,6 +927,7 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > struct resource *dbi_base; > > struct device_node *node = dev->of_node; > > int ret; > > + u16 val; > > > imx6_pcie = devm_kzalloc(dev, sizeof(*imx6_pcie), GFP_KERNEL); > > if (!imx6_pcie) > @@ -1070,6 +1072,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > ret = imx6_add_pcie_port(imx6_pcie, pdev); > > if (ret < 0) > > return ret; > > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > > + val = dw_pcie_readw_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS); > > + val |= PCI_MSI_FLAGS_ENABLE; > > + dw_pcie_writew_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS, val); > > + } > > > return 0; > }
Hi Lucas: > -----Original Message----- > From: Lucas Stach [mailto:l.stach@pengutronix.de] > Sent: 2018年12月13日 17:19 > To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > lorenzo.pieralisi@arm.com; andrew.smirnov@gmail.com > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org > Subject: Re: [v2] PCI: imx: make msi work without pcieportbus > > Hi Richard, > > Am Donnerstag, den 13.12.2018, 08:02 +0000 schrieb Richard Zhu: > > MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is > > selected. > > Thus, the MSI works fine on iMX PCIe before. > > Assert it unconditionally when MSI is supported. > > Otherwise, the MSI wouldn't be triggered although the EP is present > > and the MSIs are assigned. > > Thanks for digging into this issue. This seems like the right way forward. > However, did you test this with devices using legacy IRQs? > I.e. booting with "nomsi" on the kernel command line to see if legacy IRQs still > work if this bit is set, or if we need to avoid setting this when the user > explicitly requests to disable MSIs? > > Regards, > Lucas > [Richard Zhu] Thanks for your review. The Legacy INTx is broken. The MSI_EN bit shouldn't be asserted when the user explicitly requests to disable MSIs. BTW, regarding to Baruch's comments, it seems that all the (IS_ENABLED(CONFIG_PCI_MSI) check in the dwc host drivers are not required anymore, since the depends on PCI_MSI_IRQ_DOMAIN, right? > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index 26087b3..d3e4296 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -74,6 +74,7 @@ struct imx6_pcie { > > > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200 > > > > /* PCIe Root Complex registers (memory-mapped) */ > > > +#define PCI_MSI_CAP 0x50 > > > #define PCIE_RC_LCR 0x7c > > > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1 > > > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2 0x2 > > @@ -926,6 +927,7 @@ static int imx6_pcie_probe(struct platform_device > > *pdev) > > > struct resource *dbi_base; > > > struct device_node *node = dev->of_node; > > > int ret; > > > + u16 val; > > > > > imx6_pcie = devm_kzalloc(dev, sizeof(*imx6_pcie), GFP_KERNEL); > > > if (!imx6_pcie) > > @@ -1070,6 +1072,11 @@ static int imx6_pcie_probe(struct > > platform_device *pdev) > > > ret = imx6_add_pcie_port(imx6_pcie, pdev); > > > if (ret < 0) > > > return ret; > > > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > > > + val = dw_pcie_readw_dbi(pci, PCI_MSI_CAP + > PCI_MSI_FLAGS); > > > + val |= PCI_MSI_FLAGS_ENABLE; > > > + dw_pcie_writew_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS, val); > > > + } > > > > > return 0; > > }
Am Donnerstag, den 13.12.2018, 09:57 +0000 schrieb Richard Zhu: > Hi Lucas: > > > -----Original Message----- > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de] > > Sent: 2018年12月13日 17:19 > > > > > > To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > > lorenzo.pieralisi@arm.com; andrew.smirnov@gmail.com > > > > > > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > linux-kernel@vger.kernel.org > > Subject: Re: [v2] PCI: imx: make msi work without pcieportbus > > > > Hi Richard, > > > > Am Donnerstag, den 13.12.2018, 08:02 +0000 schrieb Richard Zhu: > > > MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is > > > selected. > > > Thus, the MSI works fine on iMX PCIe before. > > > Assert it unconditionally when MSI is supported. > > > Otherwise, the MSI wouldn't be triggered although the EP is present > > > and the MSIs are assigned. > > > > Thanks for digging into this issue. This seems like the right way forward. > > However, did you test this with devices using legacy IRQs? > > I.e. booting with "nomsi" on the kernel command line to see if legacy IRQs still > > work if this bit is set, or if we need to avoid setting this when the user > > explicitly requests to disable MSIs? > > > > Regards, > > Lucas > > > > [Richard Zhu] Thanks for your review. > The Legacy INTx is broken. > The MSI_EN bit shouldn't be asserted when the user explicitly requests to disable MSIs. Okay, so this patch should be extended with a check for pci_msi_enabled() to see if the user explicitly want legacy IRQs. > BTW, regarding to Baruch's comments, it seems that all the (IS_ENABLED(CONFIG_PCI_MSI) check in > the dwc host drivers are not required anymore, since the depends on PCI_MSI_IRQ_DOMAIN, right? That's correct. This is mostly a historical artifact from the time when we were able to build without MSI support. Those checks could be cleaned up now that we depend on the MSI options. Regards, Lucas
> -----Original Message----- > From: Lucas Stach [mailto:l.stach@pengutronix.de] > Sent: 2018年12月13日 18:07 > To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > lorenzo.pieralisi@arm.com; andrew.smirnov@gmail.com > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org > Subject: Re: [v2] PCI: imx: make msi work without pcieportbus > > Am Donnerstag, den 13.12.2018, 09:57 +0000 schrieb Richard Zhu: > > Hi Lucas: > > > > > -----Original Message----- > > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de] > > > Sent: 2018年12月13日 17:19 > > > > > > > To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > > > lorenzo.pieralisi@arm.com; andrew.smirnov@gmail.com > > > > > > > Cc: linux-pci@vger.kernel.org; > > > > > > > linux-arm-kernel@lists.infradead.org; > > > linux-kernel@vger.kernel.org > > > Subject: Re: [v2] PCI: imx: make msi work without pcieportbus > > > > > > Hi Richard, > > > > > > Am Donnerstag, den 13.12.2018, 08:02 +0000 schrieb Richard Zhu: > > > > MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is > > > > selected. > > > > Thus, the MSI works fine on iMX PCIe before. > > > > Assert it unconditionally when MSI is supported. > > > > Otherwise, the MSI wouldn't be triggered although the EP is > > > > present and the MSIs are assigned. > > > > > > Thanks for digging into this issue. This seems like the right way forward. > > > However, did you test this with devices using legacy IRQs? > > > I.e. booting with "nomsi" on the kernel command line to see if > > > legacy IRQs still work if this bit is set, or if we need to avoid > > > setting this when the user explicitly requests to disable MSIs? > > > > > > Regards, > > > Lucas > > > > > > > [Richard Zhu] Thanks for your review. > > The Legacy INTx is broken. > > The MSI_EN bit shouldn't be asserted when the user explicitly requests to > disable MSIs. > > Okay, so this patch should be extended with a check for > pci_msi_enabled() to see if the user explicitly want legacy IRQs. > > > BTW, regarding to Baruch's comments, it seems that all the > > (IS_ENABLED(CONFIG_PCI_MSI) check in > > the dwc host drivers are not required anymore, since the depends on > PCI_MSI_IRQ_DOMAIN, right? > > That's correct. This is mostly a historical artifact from the time when we > were able to build without MSI support. Those checks could be cleaned up > now that we depend on the MSI options. > [Richard Zhu] Great. Thanks Baruch and Lucas. I would send the v3 patch a moment later with the pci_msi_enabled() check when assert the MSI_EN of RC. Best Regards Richard Zhu > Regards, > Lucas
On Thu, Dec 13, 2018 at 11:07:16AM +0100, Lucas Stach wrote: > Am Donnerstag, den 13.12.2018, 09:57 +0000 schrieb Richard Zhu: > > Hi Lucas: > > > > > -----Original Message----- > > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de] > > > Sent: 2018年12月13日 17:19 > > > > > > > To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > > > lorenzo.pieralisi@arm.com; andrew.smirnov@gmail.com > > > > > > > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > linux-kernel@vger.kernel.org > > > Subject: Re: [v2] PCI: imx: make msi work without pcieportbus > > > > > > Hi Richard, > > > > > > Am Donnerstag, den 13.12.2018, 08:02 +0000 schrieb Richard Zhu: > > > > MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is > > > > selected. > > > > Thus, the MSI works fine on iMX PCIe before. > > > > Assert it unconditionally when MSI is supported. > > > > Otherwise, the MSI wouldn't be triggered although the EP is present > > > > and the MSIs are assigned. > > > > > > Thanks for digging into this issue. This seems like the right way forward. > > > However, did you test this with devices using legacy IRQs? > > > I.e. booting with "nomsi" on the kernel command line to see if legacy IRQs still > > > work if this bit is set, or if we need to avoid setting this when the user > > > explicitly requests to disable MSIs? > > > > > > Regards, > > > Lucas > > > > > > > [Richard Zhu] Thanks for your review. > > The Legacy INTx is broken. > > The MSI_EN bit shouldn't be asserted when the user explicitly requests to disable MSIs. > > Okay, so this patch should be extended with a check for > pci_msi_enabled() to see if the user explicitly want legacy IRQs. > > > BTW, regarding to Baruch's comments, it seems that all the (IS_ENABLED(CONFIG_PCI_MSI) check in > > the dwc host drivers are not required anymore, since the depends on PCI_MSI_IRQ_DOMAIN, right? > > That's correct. This is mostly a historical artifact from the time when > we were able to build without MSI support. Those checks could be > cleaned up now that we depend on the MSI options. The question is whether we should really depend on PCI_MSI_IRQ_DOMAIN option, should we ? Lorenzo
Am Donnerstag, den 13.12.2018, 12:34 +0000 schrieb Lorenzo Pieralisi: > On Thu, Dec 13, 2018 at 11:07:16AM +0100, Lucas Stach wrote: > > Am Donnerstag, den 13.12.2018, 09:57 +0000 schrieb Richard Zhu: > > > Hi Lucas: > > > > > > > -----Original Message----- > > > > > > From: Lucas Stach [mailto:l.stach@pengutronix.de] > > > > > > > > Sent: 2018年12月13日 17:19 > > > > > > > > To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com; > > > > > > > > lorenzo.pieralisi@arm.com; andrew.smirnov@gmail.com > > > > > > > > Cc: linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > > > > > > linux-kernel@vger.kernel.org > > > > Subject: Re: [v2] PCI: imx: make msi work without pcieportbus > > > > > > > > Hi Richard, > > > > > > > > Am Donnerstag, den 13.12.2018, 08:02 +0000 schrieb Richard Zhu: > > > > > MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is > > > > > selected. > > > > > Thus, the MSI works fine on iMX PCIe before. > > > > > Assert it unconditionally when MSI is supported. > > > > > Otherwise, the MSI wouldn't be triggered although the EP is present > > > > > and the MSIs are assigned. > > > > > > > > Thanks for digging into this issue. This seems like the right way forward. > > > > However, did you test this with devices using legacy IRQs? > > > > I.e. booting with "nomsi" on the kernel command line to see if legacy IRQs still > > > > work if this bit is set, or if we need to avoid setting this when the user > > > > explicitly requests to disable MSIs? > > > > > > > > Regards, > > > > Lucas > > > > > > > > > > [Richard Zhu] Thanks for your review. > > > The Legacy INTx is broken. > > > The MSI_EN bit shouldn't be asserted when the user explicitly requests to disable MSIs. > > > > Okay, so this patch should be extended with a check for > > pci_msi_enabled() to see if the user explicitly want legacy IRQs. > > > > > BTW, regarding to Baruch's comments, it seems that all the (IS_ENABLED(CONFIG_PCI_MSI) check in > > > the dwc host drivers are not required anymore, since the depends on PCI_MSI_IRQ_DOMAIN, right? > > > > That's correct. This is mostly a historical artifact from the time when > > we were able to build without MSI support. Those checks could be > > cleaned up now that we depend on the MSI options. > > The question is whether we should really depend on PCI_MSI_IRQ_DOMAIN > option, should we ? It certainly cuts down on the combinatorial space when testing those changes. IMHO this is a good thing, so as long as there is no compelling reason for making this dependency configurable I'm totally fine with keeping it the way it is now. Regards, Lucas
On Thu, Dec 13, 2018 at 08:02:11AM +0000, Richard Zhu wrote: > MSI_EN of iMX PCIe RC would be asserted when > PCIEPORTBUS driver is selected. > Thus, the MSI works fine on iMX PCIe before. > Assert it unconditionally when MSI is supported. > Otherwise, the MSI wouldn't be triggered although > the EP is present and the MSIs are assigned. This subject line and changelog need some rework. I can't understand what's going on at all. Lorenzo or I can help craft something, but I don't understand enough to propose anything yet. "MSI_EN" doesn't appear in the driver; I assume it's some device-specific signal. "iMX" does not look like the typical spelling. You could use "imx6" to refer to the driver, but in this case you're talking about the hardware itself, not the driver. So you should use "i.MX6" or whatever the appropriate brand is. If "PCIEPORTBUS driver is selected" means "CONFIG_PCIEPORTBUS=y", say that. The connection of portdrv (which is generic PCIe support) to MSI_EN (which is apparently device-specific) is unclear. It would be helpful if you could connect those dots a little more. "MSI works fine on iMX PCIe before." Before what? Is this a regression, where MSI worked before some commit and this patch fixes it? Please rewrap the changelog so it uses the entire 80-column width. Wrap to 75 so it still fits when "git log" adds the 4 char indent. > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 26087b3..d3e4296 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -74,6 +74,7 @@ struct imx6_pcie { > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200 > > /* PCIe Root Complex registers (memory-mapped) */ > +#define PCI_MSI_CAP 0x50 I wish this didn't look quite so much like a PCI core name, because this is really an i.MX-specific offset. It looks like the PCIE_RC_* names are all similar i.MX-specific things. Shouldn't this match those? > #define PCIE_RC_LCR 0x7c > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1 > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2 0x2 > @@ -926,6 +927,7 @@ static int imx6_pcie_probe(struct platform_device *pdev) > struct resource *dbi_base; > struct device_node *node = dev->of_node; > int ret; > + u16 val; > > imx6_pcie = devm_kzalloc(dev, sizeof(*imx6_pcie), GFP_KERNEL); > if (!imx6_pcie) > @@ -1070,6 +1072,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) > ret = imx6_add_pcie_port(imx6_pcie, pdev); > if (ret < 0) > return ret; > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + val = dw_pcie_readw_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS); > + val |= PCI_MSI_FLAGS_ENABLE; > + dw_pcie_writew_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS, val); > + } > > return 0; > } > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Bjorn: Thanks for your kindly review. > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: 2018年12月13日 22:41 > To: Richard Zhu <hongxing.zhu@nxp.com> > Cc: lorenzo.pieralisi@arm.com; l.stach@pengutronix.de; > andrew.smirnov@gmail.com; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [v2] PCI: imx: make msi work without pcieportbus > > On Thu, Dec 13, 2018 at 08:02:11AM +0000, Richard Zhu wrote: > > MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is > > selected. > > Thus, the MSI works fine on iMX PCIe before. > > Assert it unconditionally when MSI is supported. > > Otherwise, the MSI wouldn't be triggered although the EP is present > > and the MSIs are assigned. > > This subject line and changelog need some rework. I can't understand > what's going on at all. Lorenzo or I can help craft something, but I don't > understand enough to propose anything yet. > > "MSI_EN" doesn't appear in the driver; I assume it's some device-specific > signal. [Richard Zhu] MSI_EN is the MSI Enable bit(BIT(0)) of the Message Control Register for MSI. I would change it to MSI Enable bit later. > > "iMX" does not look like the typical spelling. You could use "imx6" > to refer to the driver, but in this case you're talking about the hardware itself, > not the driver. So you should use "i.MX6" or whatever the appropriate brand > is. [Richard Zhu] Okay, i.MX6 would be used. > > If "PCIEPORTBUS driver is selected" means "CONFIG_PCIEPORTBUS=y", say > that. The connection of portdrv (which is generic PCIe support) to MSI_EN > (which is apparently device-specific) is unclear. It would be helpful if you > could connect those dots a little more. [Richard Zhu] That's right, thanks. > > "MSI works fine on iMX PCIe before." Before what? Is this a regression, > where MSI worked before some commit and this patch fixes it? [Richard Zhu] MSI works fine on iMX6 PCIe before commit "f3fdfc4ac3". It's a limitation of the iMX6 PCIe, otherwise a regression. MSI should not have dependency on the selection of the PCIEPORTBUS > > Please rewrap the changelog so it uses the entire 80-column width. > Wrap to 75 so it still fits when "git log" adds the 4 char indent. > [Richard Zhu] Okay. > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c > > b/drivers/pci/controller/dwc/pci-imx6.c > > index 26087b3..d3e4296 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -74,6 +74,7 @@ struct imx6_pcie { > > #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200 > > > > /* PCIe Root Complex registers (memory-mapped) */ > > +#define PCI_MSI_CAP 0x50 > > I wish this didn't look quite so much like a PCI core name, because this is > really an i.MX-specific offset. It looks like the PCIE_RC_* names are all > similar i.MX-specific things. Shouldn't this match those? > [Richard Zhu] How about to replace it by PCIE_RC_IMX6_MSI_CAP? Best Regards Richard Zhu > > #define PCIE_RC_LCR 0x7c > > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1 > > #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2 0x2 > > @@ -926,6 +927,7 @@ static int imx6_pcie_probe(struct platform_device > *pdev) > > struct resource *dbi_base; > > struct device_node *node = dev->of_node; > > int ret; > > + u16 val; > > > > imx6_pcie = devm_kzalloc(dev, sizeof(*imx6_pcie), GFP_KERNEL); > > if (!imx6_pcie) > > @@ -1070,6 +1072,11 @@ static int imx6_pcie_probe(struct > platform_device *pdev) > > ret = imx6_add_pcie_port(imx6_pcie, pdev); > > if (ret < 0) > > return ret; > > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > > + val = dw_pcie_readw_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS); > > + val |= PCI_MSI_FLAGS_ENABLE; > > + dw_pcie_writew_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS, val); > > + } > > > > return 0; > > } > > -- > > 2.7.4 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flist > > > s.infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=02%7 > C > > > 01%7Chongxing.zhu%40nxp.com%7C58a92c7a1c3e4a1fa43908d661090b4f% > 7C686ea > > > 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636803088809543819& > sdata=qy6 > > q1tawIz%2FU757D5PN5vE3OqNOf70fMHLFBVZPI9AI%3D&reserved=0
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 26087b3..d3e4296 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -74,6 +74,7 @@ struct imx6_pcie { #define PHY_PLL_LOCK_WAIT_USLEEP_MAX 200 /* PCIe Root Complex registers (memory-mapped) */ +#define PCI_MSI_CAP 0x50 #define PCIE_RC_LCR 0x7c #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1 0x1 #define PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2 0x2 @@ -926,6 +927,7 @@ static int imx6_pcie_probe(struct platform_device *pdev) struct resource *dbi_base; struct device_node *node = dev->of_node; int ret; + u16 val; imx6_pcie = devm_kzalloc(dev, sizeof(*imx6_pcie), GFP_KERNEL); if (!imx6_pcie) @@ -1070,6 +1072,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) ret = imx6_add_pcie_port(imx6_pcie, pdev); if (ret < 0) return ret; + if (IS_ENABLED(CONFIG_PCI_MSI)) { + val = dw_pcie_readw_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS); + val |= PCI_MSI_FLAGS_ENABLE; + dw_pcie_writew_dbi(pci, PCI_MSI_CAP + PCI_MSI_FLAGS, val); + } return 0; }
MSI_EN of iMX PCIe RC would be asserted when PCIEPORTBUS driver is selected. Thus, the MSI works fine on iMX PCIe before. Assert it unconditionally when MSI is supported. Otherwise, the MSI wouldn't be triggered although the EP is present and the MSIs are assigned. Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> --- drivers/pci/controller/dwc/pci-imx6.c | 7 +++++++ 1 file changed, 7 insertions(+)