[v2] PCI: imx: make msi work without pcieportbus

[v2] PCI: imx: make msi work without pcieportbus

[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.

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;
 }
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2] PCI: imx: make msi work without pcieportbus

[Baruch Siach]

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

-- 
     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 -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [v2] PCI: imx: make msi work without pcieportbus

[Richard Zhu]

> -----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&amp;data=02%7C01%7Chongxing.zhu%40nxp.co
> m%7Cacb95dbd3c26442df6f608d660d2c042%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636802855648799644&amp;sdata=qjIi004UrbTYrT
> VjDL%2B1THVCbZbTfkwTu%2BeztlE6NXY%3D&amp;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&amp;data=02%7C01%7Chongxing.zhu%40nxp.com%7Cacb95dbd3
> c26442df6f608d660d2c042%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636802855648799644&amp;sdata=DQOZR2ZvwkAiWmdHsGJBT7h
> NwEdpQj6BMdG9vx90gSw%3D&amp;reserved=0 -
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2] PCI: imx: make msi work without pcieportbus

[Baruch Siach]

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 -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2] PCI: imx: make msi work without pcieportbus

[Lucas Stach]

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;
>  }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [v2] PCI: imx: make msi work without pcieportbus

[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.

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;
> >  }
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2] PCI: imx: make msi work without pcieportbus

[Lucas Stach]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [v2] PCI: imx: make msi work without pcieportbus

[Richard Zhu]

> -----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2] PCI: imx: make msi work without pcieportbus

[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 ?

Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2] PCI: imx: make msi work without pcieportbus

[Lucas Stach]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [v2] PCI: imx: make msi work without pcieportbus

[Bjorn Helgaas]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [v2] PCI: imx: make msi work without pcieportbus

[Richard Zhu]

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&amp;data=02%7
> C
> >
> 01%7Chongxing.zhu%40nxp.com%7C58a92c7a1c3e4a1fa43908d661090b4f%
> 7C686ea
> >
> 1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636803088809543819&amp;
> sdata=qy6
> > q1tawIz%2FU757D5PN5vE3OqNOf70fMHLFBVZPI9AI%3D&amp;reserved=0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel