linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] PCI: imx6: introduce drvdata
@ 2018-11-20 16:56 Stefan Agner
  2018-11-20 16:56 ` [PATCH v3 2/2] PCI: imx6: limit DBI register length Stefan Agner
  0 siblings, 1 reply; 15+ messages in thread
From: Stefan Agner @ 2018-11-20 16:56 UTC (permalink / raw)
  To: lorenzo.pieralisi, jingoohan1, gustavo.pimentel, l.stach, tpiepho
  Cc: bhelgaas, linux-pci, linux-kernel, Stefan Agner

Introduce driver data struct. This will simplify handling of device
specific differences.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
Changes in v2:
- Split drvdata introduction in a separate patch
- Use an array instead of individual struct imx6_pcie_drvdata declarations
Changes in v3:
- Rebase on pci/dwc

 drivers/pci/controller/dwc/pci-imx6.c | 46 ++++++++++++++++-----------
 1 file changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 69f86234f7c0..cdcf54ff30fb 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -41,6 +41,10 @@ enum imx6_pcie_variants {
 	IMX7D,
 };
 
+struct imx6_pcie_drvdata {
+	enum imx6_pcie_variants variant;
+};
+
 struct imx6_pcie {
 	struct dw_pcie		*pci;
 	int			reset_gpio;
@@ -53,7 +57,7 @@ struct imx6_pcie {
 	struct reset_control	*pciephy_reset;
 	struct reset_control	*apps_reset;
 	struct reset_control	*turnoff_reset;
-	enum imx6_pcie_variants variant;
+	const struct imx6_pcie_drvdata *drvdata;
 	u32			tx_deemph_gen1;
 	u32			tx_deemph_gen2_3p5db;
 	u32			tx_deemph_gen2_6db;
@@ -340,7 +344,7 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
 {
 	struct device *dev = imx6_pcie->pci->dev;
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX7D:
 		reset_control_assert(imx6_pcie->pciephy_reset);
 		reset_control_assert(imx6_pcie->apps_reset);
@@ -382,7 +386,7 @@ static int imx6_pcie_enable_ref_clk(struct imx6_pcie *imx6_pcie)
 	struct device *dev = pci->dev;
 	int ret = 0;
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX6SX:
 		ret = clk_prepare_enable(imx6_pcie->pcie_inbound_axi);
 		if (ret) {
@@ -485,7 +489,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 					!imx6_pcie->gpio_active_high);
 	}
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX7D:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
 		imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
@@ -523,7 +527,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 
 static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
 {
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX7D:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, 0);
@@ -645,7 +649,7 @@ static void imx6_pcie_ltssm_enable(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX6Q:
 	case IMX6SX:
 	case IMX6QP:
@@ -698,7 +702,7 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie)
 		tmp |= PORT_LOGIC_SPEED_CHANGE;
 		dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, tmp);
 
-		if (imx6_pcie->variant != IMX7D) {
+		if (imx6_pcie->drvdata->variant != IMX7D) {
 			/*
 			 * On i.MX7, DIRECT_SPEED_CHANGE behaves differently
 			 * from i.MX6 family when no link speed transition
@@ -801,7 +805,7 @@ static void imx6_pcie_ltssm_disable(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX6SX:
 	case IMX6QP:
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
@@ -836,7 +840,7 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
 	clk_disable_unprepare(imx6_pcie->pcie_phy);
 	clk_disable_unprepare(imx6_pcie->pcie_bus);
 
-	if (imx6_pcie->variant == IMX7D) {
+	if (imx6_pcie->drvdata->variant == IMX7D) {
 		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
 				   IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
@@ -847,7 +851,7 @@ static int imx6_pcie_suspend_noirq(struct device *dev)
 {
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 
-	if (imx6_pcie->variant != IMX7D)
+	if (imx6_pcie->drvdata->variant != IMX7D)
 		return 0;
 
 	imx6_pcie_pm_turnoff(imx6_pcie);
@@ -863,7 +867,7 @@ static int imx6_pcie_resume_noirq(struct device *dev)
 	struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
 	struct pcie_port *pp = &imx6_pcie->pci->pp;
 
-	if (imx6_pcie->variant != IMX7D)
+	if (imx6_pcie->drvdata->variant != IMX7D)
 		return 0;
 
 	imx6_pcie_assert_core_reset(imx6_pcie);
@@ -905,8 +909,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	pci->ops = &dw_pcie_ops;
 
 	imx6_pcie->pci = pci;
-	imx6_pcie->variant =
-		(enum imx6_pcie_variants)of_device_get_match_data(dev);
+	imx6_pcie->drvdata = of_device_get_match_data(dev);
 
 	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
@@ -950,7 +953,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(imx6_pcie->pcie);
 	}
 
-	switch (imx6_pcie->variant) {
+	switch (imx6_pcie->drvdata->variant) {
 	case IMX6SX:
 		imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
 							   "pcie_inbound_axi");
@@ -1048,11 +1051,18 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
 	imx6_pcie_assert_core_reset(imx6_pcie);
 }
 
+static const struct imx6_pcie_drvdata drvdata[] = {
+	[IMX6Q] = { .variant = IMX6Q },
+	[IMX6SX] = { .variant = IMX6SX },
+	[IMX6QP] = { .variant = IMX6QP },
+	[IMX7D] = { .variant = IMX7D },
+};
+
 static const struct of_device_id imx6_pcie_of_match[] = {
-	{ .compatible = "fsl,imx6q-pcie",  .data = (void *)IMX6Q,  },
-	{ .compatible = "fsl,imx6sx-pcie", .data = (void *)IMX6SX, },
-	{ .compatible = "fsl,imx6qp-pcie", .data = (void *)IMX6QP, },
-	{ .compatible = "fsl,imx7d-pcie",  .data = (void *)IMX7D,  },
+	{ .compatible = "fsl,imx6q-pcie",  .data = &drvdata[IMX6Q],  },
+	{ .compatible = "fsl,imx6sx-pcie", .data = &drvdata[IMX6SX], },
+	{ .compatible = "fsl,imx6qp-pcie", .data = &drvdata[IMX6QP], },
+	{ .compatible = "fsl,imx7d-pcie",  .data = &drvdata[IMX7D],  },
 	{},
 };
 
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-20 16:56 [PATCH v3 1/2] PCI: imx6: introduce drvdata Stefan Agner
@ 2018-11-20 16:56 ` Stefan Agner
  2018-11-20 18:19   ` Leonard Crestez
  0 siblings, 1 reply; 15+ messages in thread
From: Stefan Agner @ 2018-11-20 16:56 UTC (permalink / raw)
  To: lorenzo.pieralisi, jingoohan1, gustavo.pimentel, l.stach, tpiepho
  Cc: bhelgaas, linux-pci, linux-kernel, Stefan Agner

Define the length of the DBI registers. This makes sure that
the kernel does not access registers beyond that point, avoiding
the following abort on a i.MX 6Quad:
  # cat /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
  [  100.021433] Unhandled fault: imprecise external abort (0x1406) at 0xb6ea7000
  ...
  [  100.056423] PC is at dw_pcie_read+0x50/0x84
  [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
  ...

Signed-off-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---
Changes in v3:
- Rebase on pci/dwc

 drivers/pci/controller/dwc/pci-imx6.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index cdcf54ff30fb..7015bda22aef 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -43,6 +43,7 @@ enum imx6_pcie_variants {
 
 struct imx6_pcie_drvdata {
 	enum imx6_pcie_variants variant;
+	int			dbi_length;
 };
 
 struct imx6_pcie {
@@ -981,6 +982,8 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 		break;
 	}
 
+	pci->dbi_length = imx6_pcie->drvdata->dbi_length;
+
 	/* Grab turnoff reset */
 	imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
 	if (IS_ERR(imx6_pcie->turnoff_reset)) {
@@ -1052,7 +1055,7 @@ static void imx6_pcie_shutdown(struct platform_device *pdev)
 }
 
 static const struct imx6_pcie_drvdata drvdata[] = {
-	[IMX6Q] = { .variant = IMX6Q },
+	[IMX6Q] = { .variant = IMX6Q, .dbi_length = 0x15c },
 	[IMX6SX] = { .variant = IMX6SX },
 	[IMX6QP] = { .variant = IMX6QP },
 	[IMX7D] = { .variant = IMX7D },
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-20 16:56 ` [PATCH v3 2/2] PCI: imx6: limit DBI register length Stefan Agner
@ 2018-11-20 18:19   ` Leonard Crestez
  2018-11-20 19:13     ` Trent Piepho
  0 siblings, 1 reply; 15+ messages in thread
From: Leonard Crestez @ 2018-11-20 18:19 UTC (permalink / raw)
  To: stefan, l.stach, lorenzo.pieralisi
  Cc: Richard Zhu, linux-kernel, jingoohan1, gustavo.pimentel,
	andrew.smirnov, tpiepho, linux-pci, bhelgaas

On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote:
> Define the length of the DBI registers. This makes sure that
> the kernel does not access registers beyond that point, avoiding
> the following abort on a i.MX 6Quad:
>   # cat
> /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
>   [  100.021433] Unhandled fault: imprecise external abort (0x1406)
> at 0xb6ea7000
>   ...
>   [  100.056423] PC is at dw_pcie_read+0x50/0x84
>   [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48

I don't know exactly where this limitation comes from, I can indeed
reproduce a stack dump when dumping pci config from /sys/

Unfortunately this seems to block access to registers used for
functionality like interrupts. For example dw_handle_msi_irq does:

	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
			    (i * MSI_REG_CTRL_BLOCK_SIZE),
			    4, &val);

where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.

Testing on 6dl-sabreauto (dts change required) with an ath9k pcie card
with your series I sometimes get "irq 295: nobody cared" on boot. Maybe
I'm missing something?

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-20 18:19   ` Leonard Crestez
@ 2018-11-20 19:13     ` Trent Piepho
  2018-11-20 20:42       ` Stefan Agner
  0 siblings, 1 reply; 15+ messages in thread
From: Trent Piepho @ 2018-11-20 19:13 UTC (permalink / raw)
  To: stefan, l.stach, lorenzo.pieralisi, leonard.crestez
  Cc: jingoohan1, linux-kernel, andrew.smirnov, hongxing.zhu,
	gustavo.pimentel, linux-pci, bhelgaas

On Tue, 2018-11-20 at 18:19 +0000, Leonard Crestez wrote:
> On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote:
> > Define the length of the DBI registers. This makes sure that
> > the kernel does not access registers beyond that point, avoiding
> > the following abort on a i.MX 6Quad:
> >   # cat
> > /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
> >   [  100.021433] Unhandled fault: imprecise external abort (0x1406)
> > at 0xb6ea7000
> >   ...
> >   [  100.056423] PC is at dw_pcie_read+0x50/0x84
> >   [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
> 
> I don't know exactly where this limitation comes from, I can indeed
> reproduce a stack dump when dumping pci config from /sys/
> 
> Unfortunately this seems to block access to registers used for
> functionality like interrupts. For example dw_handle_msi_irq does:
> 
> 	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
> 			    (i * MSI_REG_CTRL_BLOCK_SIZE),
> 			    4, &val);
> 
> where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.
> 
> Testing on 6dl-sabreauto (dts change required) with an ath9k pcie card
> with your series I sometimes get "irq 295: nobody cared" on boot. Maybe
> I'm missing something?

On IMX7d, there are significant blocks of 00s in the config space, and
all 0xff at 0xb50 on up.

I.e., significant portions are empty, in the middle of the config
space, not just at the end.

But they can be read without problem.

Perhaps imx6q aborts on a read of an unimplemented address instead of
returning zeros like imx7d.  In that case it really needs something
more complex to prevent abort than just a length.

It also seems to me that this doesn't need to be in the internal pci
config access functions.  The driver shouldn't be reading registers
that don't exist anyway.  It's really about trying to fix sysfs access
to registers that don't exist.  So maybe it should be done there.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-20 19:13     ` Trent Piepho
@ 2018-11-20 20:42       ` Stefan Agner
  2018-11-20 21:28         ` Trent Piepho
  2018-11-26 10:16         ` Leonard Crestez
  0 siblings, 2 replies; 15+ messages in thread
From: Stefan Agner @ 2018-11-20 20:42 UTC (permalink / raw)
  To: Trent Piepho
  Cc: l.stach, lorenzo.pieralisi, leonard.crestez, jingoohan1,
	linux-kernel, andrew.smirnov, hongxing.zhu, gustavo.pimentel,
	linux-pci, bhelgaas

On 20.11.2018 20:13, Trent Piepho wrote:
> On Tue, 2018-11-20 at 18:19 +0000, Leonard Crestez wrote:
>> On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote:
>> > Define the length of the DBI registers. This makes sure that
>> > the kernel does not access registers beyond that point, avoiding
>> > the following abort on a i.MX 6Quad:
>> >   # cat
>> > /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
>> >   [  100.021433] Unhandled fault: imprecise external abort (0x1406)
>> > at 0xb6ea7000
>> >   ...
>> >   [  100.056423] PC is at dw_pcie_read+0x50/0x84
>> >   [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
>>
>> I don't know exactly where this limitation comes from, I can indeed
>> reproduce a stack dump when dumping pci config from /sys/
>>
>> Unfortunately this seems to block access to registers used for
>> functionality like interrupts. For example dw_handle_msi_irq does:
>>
>> 	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
>> 			    (i * MSI_REG_CTRL_BLOCK_SIZE),
>> 			    4, &val);
>>
>> where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.
>>
>> Testing on 6dl-sabreauto (dts change required) with an ath9k pcie card
>> with your series I sometimes get "irq 295: nobody cared" on boot. Maybe
>> I'm missing something?
> 
> On IMX7d, there are significant blocks of 00s in the config space, and
> all 0xff at 0xb50 on up.
> 
> I.e., significant portions are empty, in the middle of the config
> space, not just at the end.
> 
> But they can be read without problem.
> 
> Perhaps imx6q aborts on a read of an unimplemented address instead of
> returning zeros like imx7d.  In that case it really needs something
> more complex to prevent abort than just a length.

Yeah it seems those SoCs behave differently.

Describing a register set with holes will get complicated, I guess it
would ask for a regmap...

> 
> It also seems to me that this doesn't need to be in the internal pci
> config access functions.  The driver shouldn't be reading registers
> that don't exist anyway.  It's really about trying to fix sysfs access
> to registers that don't exist.  So maybe it should be done there.

That was my first approach, see:
https://lkml.org/lkml/2018/11/14/716

--
Stefan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-20 20:42       ` Stefan Agner
@ 2018-11-20 21:28         ` Trent Piepho
  2018-11-21 13:47           ` Leonard Crestez
  2018-11-26 10:16         ` Leonard Crestez
  1 sibling, 1 reply; 15+ messages in thread
From: Trent Piepho @ 2018-11-20 21:28 UTC (permalink / raw)
  To: stefan
  Cc: hongxing.zhu, linux-kernel, jingoohan1, lorenzo.pieralisi,
	gustavo.pimentel, andrew.smirnov, linux-pci, l.stach,
	leonard.crestez, bhelgaas

On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> On 20.11.2018 20:13, Trent Piepho wrote:
> > 
> > On IMX7d, there are significant blocks of 00s in the config space, and
> > all 0xff at 0xb50 on up.
> > 
> > I.e., significant portions are empty, in the middle of the config
> > space, not just at the end.
> > 
> > But they can be read without problem.
> > 
> > Perhaps imx6q aborts on a read of an unimplemented address instead of
> > returning zeros like imx7d.  In that case it really needs something
> > more complex to prevent abort than just a length.
> 
> Yeah it seems those SoCs behave differently.
> 
> Describing a register set with holes will get complicated, I guess it
> would ask for a regmap...
> 
> > 
> > It also seems to me that this doesn't need to be in the internal pci
> > config access functions.  The driver shouldn't be reading registers
> > that don't exist anyway.  It's really about trying to fix sysfs access
> > to registers that don't exist.  So maybe it should be done there.
> 
> That was my first approach, see:

https://lkml.org/lkml/2018/11/14/716

Yes, but that just used the pci device id which applies to every IMX
design.

It's also not totally correct, as it seems real registers after 0x200
do work on imx6, and that would prevent access to them.

Like you say, it could use a regmap.  Seems kinda overkill to me
though.

I wonder if regmap based caching of register to avoid RMW cycles would
be generally useful?  I know the enable and mask registers are/were
cached in the driver (irq_state[]).

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-20 21:28         ` Trent Piepho
@ 2018-11-21 13:47           ` Leonard Crestez
  2018-11-21 14:17             ` Lorenzo Pieralisi
  2018-11-28 12:19             ` Stefan Agner
  0 siblings, 2 replies; 15+ messages in thread
From: Leonard Crestez @ 2018-11-21 13:47 UTC (permalink / raw)
  To: stefan, lorenzo.pieralisi
  Cc: Trent Piepho, Richard Zhu, linux-kernel, jingoohan1,
	gustavo.pimentel, andrew.smirnov, linux-pci, l.stach, bhelgaas

On 11/20/2018 11:28 PM, Trent Piepho wrote:
> On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
>> On 20.11.2018 20:13, Trent Piepho wrote:

>>> It also seems to me that this doesn't need to be in the internal pci
>>> config access functions.  The driver shouldn't be reading registers
>>> that don't exist anyway.  It's really about trying to fix sysfs access
>>> to registers that don't exist.  So maybe it should be done there.
>>
>> That was my first approach, see:
> 
> Yes, but that just used the pci device id which applies to every IMX
> design.
> 
> It's also not totally correct, as it seems real registers after 0x200
> do work on imx6, and that would prevent access to them.

I see that Lorenzo already accepted the patch in pci/dwc:

https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/dwc&id=f14eaec153aaebbe940ddd21e4198cc2abc927c2

My tests show that this series breaks pci cards on 6qdl and I think it 
should be reverted until a fix is found. Are you OK with this?

Fixing might require an entirely different approach.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-21 13:47           ` Leonard Crestez
@ 2018-11-21 14:17             ` Lorenzo Pieralisi
  2018-11-28 12:19             ` Stefan Agner
  1 sibling, 0 replies; 15+ messages in thread
From: Lorenzo Pieralisi @ 2018-11-21 14:17 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: stefan, Trent Piepho, Richard Zhu, linux-kernel, jingoohan1,
	gustavo.pimentel, andrew.smirnov, linux-pci, l.stach, bhelgaas

On Wed, Nov 21, 2018 at 01:47:05PM +0000, Leonard Crestez wrote:
> On 11/20/2018 11:28 PM, Trent Piepho wrote:
> > On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> >> On 20.11.2018 20:13, Trent Piepho wrote:
> 
> >>> It also seems to me that this doesn't need to be in the internal pci
> >>> config access functions.  The driver shouldn't be reading registers
> >>> that don't exist anyway.  It's really about trying to fix sysfs access
> >>> to registers that don't exist.  So maybe it should be done there.
> >>
> >> That was my first approach, see:
> > 
> > Yes, but that just used the pci device id which applies to every IMX
> > design.
> > 
> > It's also not totally correct, as it seems real registers after 0x200
> > do work on imx6, and that would prevent access to them.
> 
> I see that Lorenzo already accepted the patch in pci/dwc:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/dwc&id=f14eaec153aaebbe940ddd21e4198cc2abc927c2
> 
> My tests show that this series breaks pci cards on 6qdl and I think it 
> should be reverted until a fix is found. Are you OK with this?

I will drop the patches from the PCI queue.

Lorenzo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-20 20:42       ` Stefan Agner
  2018-11-20 21:28         ` Trent Piepho
@ 2018-11-26 10:16         ` Leonard Crestez
  2018-11-26 16:34           ` Trent Piepho
  1 sibling, 1 reply; 15+ messages in thread
From: Leonard Crestez @ 2018-11-26 10:16 UTC (permalink / raw)
  To: stefan, l.stach, tpiepho
  Cc: Richard Zhu, linux-kernel, jingoohan1, lorenzo.pieralisi,
	gustavo.pimentel, andrew.smirnov, bhelgaas, linux-pci

On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> On 20.11.2018 20:13, Trent Piepho wrote:
> > On Tue, 2018-11-20 at 18:19 +0000, Leonard Crestez wrote:
> > > On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote:
> > > > Define the length of the DBI registers. This makes sure that
> > > > the kernel does not access registers beyond that point, avoiding
> > > > the following abort on a i.MX 6Quad:
> > > >   # cat
> > > > /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
> > > >   [  100.021433] Unhandled fault: imprecise external abort (0x1406)
> > > > at 0xb6ea7000
> > > >   ...
> > > >   [  100.056423] PC is at dw_pcie_read+0x50/0x84
> > > >   [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
> > > 
> > > I don't know exactly where this limitation comes from, I can indeed
> > > reproduce a stack dump when dumping pci config from /sys/
> > > 
> > > Unfortunately this seems to block access to registers used for
> > > functionality like interrupts. For example dw_handle_msi_irq does:
> > > 
> > > 	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
> > > 			    (i * MSI_REG_CTRL_BLOCK_SIZE),
> > > 			    4, &val);
> > > 
> > > where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.
> > 
> > On IMX7d, there are significant blocks of 00s in the config space, and
> > all 0xff at 0xb50 on up.
> > 
> > I.e., significant portions are empty, in the middle of the config
> > space, not just at the end.
> > 
> > But they can be read without problem.
> > 
> > Perhaps imx6q aborts on a read of an unimplemented address instead of
> > returning zeros like imx7d.  In that case it really needs something
> > more complex to prevent abort than just a length.
> 
> Yeah it seems those SoCs behave differently.
> 
> Describing a register set with holes will get complicated, I guess it
> would ask for a regmap...

The PortLogic area seems to be always at 0x700-0xA00, there are defines
for it. So this would only require one additional range, no regmap
stuff.

I don't know if making portlogic accessible to userspace is useful. In
theory somebody could read debug DWC registers via /sys/blah/config,
but there are better ways to read HW registers from userspace.

Browsing through the docs I can't find stuff like reads with side-
effects so I can't say that it's harmful either.

> > It also seems to me that this doesn't need to be in the internal pci
> > config access functions.  The driver shouldn't be reading registers
> > that don't exist anyway.  It's really about trying to fix sysfs access
> > to registers that don't exist.  So maybe it should be done there.
> 
> That was my first approach

Doing it on a per-soc basis is better, this seems to affect both 6q and
6qp (those are distinct!) but not others.

The pci config area from userspace is accessed through pci_ops.read =
dw_pcie_rd_conf while internal accesses go through dw_pcie_rd_own_conf.
So moving the dbi_length check up one level would work easily.

What about assigning to pci_dev->cfg_size from probe code?
Unfortunately there doesn't seem to be any straight-forward way to do
that from imx6_pcie or even dw_pcie, but maybe I'm missing something?

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-26 10:16         ` Leonard Crestez
@ 2018-11-26 16:34           ` Trent Piepho
  0 siblings, 0 replies; 15+ messages in thread
From: Trent Piepho @ 2018-11-26 16:34 UTC (permalink / raw)
  To: stefan, l.stach, leonard.crestez
  Cc: hongxing.zhu, linux-kernel, jingoohan1, lorenzo.pieralisi,
	gustavo.pimentel, andrew.smirnov, linux-pci, bhelgaas

On Mon, 2018-11-26 at 10:16 +0000, Leonard Crestez wrote:
> On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> > On 20.11.2018 20:13, Trent Piepho wrote:
> > > On Tue, 2018-11-20 at 18:19 +0000, Leonard Crestez wrote:
> > > > On Tue, 2018-11-20 at 17:56 +0100, Stefan Agner wrote:
> > > > > Define the length of the DBI registers. This makes sure that
> > > > > the kernel does not access registers beyond that point, avoiding
> > > > > the following abort on a i.MX 6Quad:
> > > > >   # cat
> > > > > /sys/devices/soc0/soc/1ffc000.pcie/pci0000\:00/0000\:00\:00.0/config
> > > > >   [  100.021433] Unhandled fault: imprecise external abort (0x1406)
> > > > > at 0xb6ea7000
> > > > >   ...
> > > > >   [  100.056423] PC is at dw_pcie_read+0x50/0x84
> > > > >   [  100.060790] LR is at dw_pcie_rd_own_conf+0x44/0x48
> > > > 
> > > > I don't know exactly where this limitation comes from, I can indeed
> > > > reproduce a stack dump when dumping pci config from /sys/
> > > > 
> > > > Unfortunately this seems to block access to registers used for
> > > > functionality like interrupts. For example dw_handle_msi_irq does:
> > > > 
> > > > 	dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS +
> > > > 			    (i * MSI_REG_CTRL_BLOCK_SIZE),
> > > > 			    4, &val);
> > > > 
> > > > where PCI_MSI_INTR0_STATUS is 0x830. There are more accesses like this.
> > > 
> > > On IMX7d, there are significant blocks of 00s in the config space, and
> > > all 0xff at 0xb50 on up.
> > > 
> > > I.e., significant portions are empty, in the middle of the config
> > > space, not just at the end.
> > > 
> > > But they can be read without problem.
> > > 
> > > Perhaps imx6q aborts on a read of an unimplemented address instead of
> > > returning zeros like imx7d.  In that case it really needs something
> > > more complex to prevent abort than just a length.
> > 
> > Yeah it seems those SoCs behave differently.
> > 
> > Describing a register set with holes will get complicated, I guess it
> > would ask for a regmap...
> 
> The PortLogic area seems to be always at 0x700-0xA00, there are defines
> for it. So this would only require one additional range, no regmap
> stuff.
> 
> I don't know if making portlogic accessible to userspace is useful. In
> theory somebody could read debug DWC registers via /sys/blah/config,
> but there are better ways to read HW registers from userspace.
> 
> Browsing through the docs I can't find stuff like reads with side-
> effects so I can't say that it's harmful either.

I doubt those register are used much from userspace, since AFAIK there
are no public docs for them.  I have dumped the config regs from
userspace in imx7d to debug a hang problem, but those registers weren't
useful to me.  Maybe they would have been, no docs...

> > > It also seems to me that this doesn't need to be in the internal pci
> > > config access functions.  The driver shouldn't be reading registers
> > > that don't exist anyway.  It's really about trying to fix sysfs access
> > > to registers that don't exist.  So maybe it should be done there.
> > 
> > That was my first approach
> 
> Doing it on a per-soc basis is better, this seems to affect both 6q and
> 6qp (those are distinct!) but not others.
> 
> The pci config area from userspace is accessed through pci_ops.read =
> dw_pcie_rd_conf while internal accesses go through dw_pcie_rd_own_conf.
> So moving the dbi_length check up one level would work easily.

What about something like:

static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
                           int size, u32 *val)
{
	struct pcie_port *pp = bus->sysdata;

	if (!dw_pcie_valid_device(pp, bus, PCI_SLOT(devfn))) {
		*val = 0xffffffff;
		return PCIBIOS_DEVICE_NOT_FOUND;
	}

	if (bus->number == pp->root_bus_nr) {
+		if (pp->ops->cfg_valid && !pp->ops_cfg_valid(pp, where, size)) {
+			*val = 0xffffffff;
+			return PCIBIOS_SOMETHING_GOES_HERE;
+		}
+
                return dw_pcie_rd_own_conf(pp, where, size, val);
+	}
	return dw_pcie_rd_other_conf(pp, bus, devfn, where, size, val);
}

imx6q and imx6qp can provide cfg_valid that checks their range.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-21 13:47           ` Leonard Crestez
  2018-11-21 14:17             ` Lorenzo Pieralisi
@ 2018-11-28 12:19             ` Stefan Agner
  2018-11-28 17:36               ` Stefan Agner
  1 sibling, 1 reply; 15+ messages in thread
From: Stefan Agner @ 2018-11-28 12:19 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: lorenzo.pieralisi, Trent Piepho, Richard Zhu, linux-kernel,
	jingoohan1, gustavo.pimentel, andrew.smirnov, linux-pci, l.stach,
	bhelgaas

On 21.11.2018 14:47, Leonard Crestez wrote:
> On 11/20/2018 11:28 PM, Trent Piepho wrote:
>> On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
>>> On 20.11.2018 20:13, Trent Piepho wrote:
> 
>>>> It also seems to me that this doesn't need to be in the internal pci
>>>> config access functions.  The driver shouldn't be reading registers
>>>> that don't exist anyway.  It's really about trying to fix sysfs access
>>>> to registers that don't exist.  So maybe it should be done there.
>>>
>>> That was my first approach, see:
>>
>> Yes, but that just used the pci device id which applies to every IMX
>> design.
>>
>> It's also not totally correct, as it seems real registers after 0x200
>> do work on imx6, and that would prevent access to them.
> 
> I see that Lorenzo already accepted the patch in pci/dwc:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/dwc&id=f14eaec153aaebbe940ddd21e4198cc2abc927c2
> 
> My tests show that this series breaks pci cards on 6qdl and I think it 
> should be reverted until a fix is found. Are you OK with this?
> 
> Fixing might require an entirely different approach.

I tried to reproduce this issue on Apalis iMX6 (i.MX 6Q) with a ath9k
PCIe WiFi card, the issue you are seeing did not happen. My lspci looks
as follows:

root@ea210c63d739:/# lspci -v
00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
[Normal decode])
        Flags: bus master, fast devsel, latency 0, IRQ 255
        Memory at 01000000 (32-bit, non-prefetchable) [size=1M]
        Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
        Memory behind bridge: 01100000-011fffff
        [virtual] Expansion ROM at 01200000 [disabled] [size=64K]
        Capabilities: [40] Power Management version 3
        Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
        Capabilities: [70] Express Root Port (Slot-), MSI 00
        Capabilities: [100] Advanced Error Reporting
        Capabilities: [140] Virtual Channel
lspci: Unable to load libkmod resources: error -12

01:00.0 Network controller: Qualcomm Atheros AR928X Wireless Network
Adapter (PCI-Express) (rev 01)
        Subsystem: Foxconn International, Inc. Device e007
        Flags: bus master, fast devsel, latency 0, IRQ 312
        Memory at 01100000 (64-bit, non-prefetchable) [size=64K]
        Capabilities: [40] Power Management version 2
        Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit-
        Capabilities: [60] Express Legacy Endpoint, MSI 00
        Capabilities: [90] MSI-X: Enable- Count=1 Masked-
        Capabilities: [100] Advanced Error Reporting
        Capabilities: [140] Virtual Channel
        Capabilities: [160] Device Serial Number 00-00-00-00-00-00-00-00
        Kernel driver in use: ath9k


I did also setup a WiFi network and transmitted some packages, but I did
not get a nobody carred message. Do you have an idea why that might be?

# cat /proc/interrupts
...
312:      10967          0          0          0       GPC 123 Level    
ath9k
...


Your conclusion in this thread seem reasonable, hence reverting the
issue does. However, I still would like to reproduce the issue so I can
make sure that future patches don't break it :-)

--
Stefan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-28 12:19             ` Stefan Agner
@ 2018-11-28 17:36               ` Stefan Agner
  2018-11-28 17:50                 ` Lucas Stach
  2018-11-28 18:01                 ` Leonard Crestez
  0 siblings, 2 replies; 15+ messages in thread
From: Stefan Agner @ 2018-11-28 17:36 UTC (permalink / raw)
  To: Leonard Crestez, l.stach
  Cc: lorenzo.pieralisi, Trent Piepho, Richard Zhu, linux-kernel,
	jingoohan1, gustavo.pimentel, andrew.smirnov, linux-pci,
	bhelgaas

On 28.11.2018 13:19, Stefan Agner wrote:
> On 21.11.2018 14:47, Leonard Crestez wrote:
>> On 11/20/2018 11:28 PM, Trent Piepho wrote:
>>> On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
>>>> On 20.11.2018 20:13, Trent Piepho wrote:
>>
>>>>> It also seems to me that this doesn't need to be in the internal pci
>>>>> config access functions.  The driver shouldn't be reading registers
>>>>> that don't exist anyway.  It's really about trying to fix sysfs access
>>>>> to registers that don't exist.  So maybe it should be done there.
>>>>
>>>> That was my first approach, see:
>>>
>>> Yes, but that just used the pci device id which applies to every IMX
>>> design.
>>>
>>> It's also not totally correct, as it seems real registers after 0x200
>>> do work on imx6, and that would prevent access to them.
>>
>> I see that Lorenzo already accepted the patch in pci/dwc:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/dwc&id=f14eaec153aaebbe940ddd21e4198cc2abc927c2
>>
>> My tests show that this series breaks pci cards on 6qdl and I think it
>> should be reverted until a fix is found. Are you OK with this?
>>
>> Fixing might require an entirely different approach.
> 
> I tried to reproduce this issue on Apalis iMX6 (i.MX 6Q) with a ath9k
> PCIe WiFi card, the issue you are seeing did not happen. My lspci looks
> as follows:
> 
> root@ea210c63d739:/# lspci -v
> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
> [Normal decode])
>         Flags: bus master, fast devsel, latency 0, IRQ 255
>         Memory at 01000000 (32-bit, non-prefetchable) [size=1M]
>         Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
>         Memory behind bridge: 01100000-011fffff
>         [virtual] Expansion ROM at 01200000 [disabled] [size=64K]
>         Capabilities: [40] Power Management version 3
>         Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
>         Capabilities: [70] Express Root Port (Slot-), MSI 00
>         Capabilities: [100] Advanced Error Reporting
>         Capabilities: [140] Virtual Channel
> lspci: Unable to load libkmod resources: error -12
> 
> 01:00.0 Network controller: Qualcomm Atheros AR928X Wireless Network
> Adapter (PCI-Express) (rev 01)
>         Subsystem: Foxconn International, Inc. Device e007
>         Flags: bus master, fast devsel, latency 0, IRQ 312
>         Memory at 01100000 (64-bit, non-prefetchable) [size=64K]
>         Capabilities: [40] Power Management version 2
>         Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit-
>         Capabilities: [60] Express Legacy Endpoint, MSI 00
>         Capabilities: [90] MSI-X: Enable- Count=1 Masked-
>         Capabilities: [100] Advanced Error Reporting
>         Capabilities: [140] Virtual Channel
>         Capabilities: [160] Device Serial Number 00-00-00-00-00-00-00-00
>         Kernel driver in use: ath9k
> 
> 
> I did also setup a WiFi network and transmitted some packages, but I did
> not get a nobody carred message. Do you have an idea why that might be?
> 
> # cat /proc/interrupts
> ...
> 312:      10967          0          0          0       GPC 123 Level    
> ath9k
> ...
> 
> 
> Your conclusion in this thread seem reasonable, hence reverting the
> issue does. However, I still would like to reproduce the issue so I can
> make sure that future patches don't break it :-)

Hm, I realized that I need to enable CONFIG_PCIEPORTBUS and set
ath9k.use_msi=1 to get MSI for that card. However, it seems that ath9k
does not behave well in that setup. It does get interrupts, and seems to
work to some degree, but I was not successful in transmitting data over
WiFi, but that might be an entirly different thing.

However, what I noticed is that when CONFIG_PCIEPORTBUS and
CONFIG_PCI_MSI is enabled, MSI works but legacy interrupt seem not to
fire anymore. That is true for ath9k as well as e1000e (using
e1000e.IntMode=0 to force legacy). Is that a known issue/limitation with
i.MX 6 PCIe?

--
Stefan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-28 17:36               ` Stefan Agner
@ 2018-11-28 17:50                 ` Lucas Stach
  2018-11-28 17:56                   ` Stefan Agner
  2018-11-28 18:01                 ` Leonard Crestez
  1 sibling, 1 reply; 15+ messages in thread
From: Lucas Stach @ 2018-11-28 17:50 UTC (permalink / raw)
  To: Stefan Agner, Leonard Crestez
  Cc: lorenzo.pieralisi, Trent Piepho, Richard Zhu, linux-kernel,
	jingoohan1, gustavo.pimentel, andrew.smirnov, linux-pci,
	bhelgaas

Am Mittwoch, den 28.11.2018, 18:36 +0100 schrieb Stefan Agner:
> On 28.11.2018 13:19, Stefan Agner wrote:
> > On 21.11.2018 14:47, Leonard Crestez wrote:
> > > On 11/20/2018 11:28 PM, Trent Piepho wrote:
> > > > On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
> > > > > On 20.11.2018 20:13, Trent Piepho wrote:
> > > > > > It also seems to me that this doesn't need to be in the internal pci
> > > > > > config access functions.  The driver shouldn't be reading registers
> > > > > > that don't exist anyway.  It's really about trying to fix sysfs access
> > > > > > to registers that don't exist.  So maybe it should be done there.
> > > > > 
> > > > > That was my first approach, see:
> > > > 
> > > > Yes, but that just used the pci device id which applies to every IMX
> > > > design.
> > > > 
> > > > It's also not totally correct, as it seems real registers after 0x200
> > > > do work on imx6, and that would prevent access to them.
> > > 
> > > I see that Lorenzo already accepted the patch in pci/dwc:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/dwc&id=f14eaec153aaebbe940ddd21e4198cc2abc927c2
> > > 
> > > My tests show that this series breaks pci cards on 6qdl and I think it
> > > should be reverted until a fix is found. Are you OK with this?
> > > 
> > > Fixing might require an entirely different approach.
> > 
> > I tried to reproduce this issue on Apalis iMX6 (i.MX 6Q) with a ath9k
> > PCIe WiFi card, the issue you are seeing did not happen. My lspci looks
> > as follows:
> > 
> > root@ea210c63d739:/# lspci -v
> > 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
> > [Normal decode])
> >         Flags: bus master, fast devsel, latency 0, IRQ 255
> >         Memory at 01000000 (32-bit, non-prefetchable) [size=1M]
> >         Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
> >         Memory behind bridge: 01100000-011fffff
> >         [virtual] Expansion ROM at 01200000 [disabled] [size=64K]
> >         Capabilities: [40] Power Management version 3
> >         Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
> >         Capabilities: [70] Express Root Port (Slot-), MSI 00
> >         Capabilities: [100] Advanced Error Reporting
> >         Capabilities: [140] Virtual Channel
> > lspci: Unable to load libkmod resources: error -12
> > 
> > 01:00.0 Network controller: Qualcomm Atheros AR928X Wireless Network
> > Adapter (PCI-Express) (rev 01)
> >         Subsystem: Foxconn International, Inc. Device e007
> >         Flags: bus master, fast devsel, latency 0, IRQ 312
> >         Memory at 01100000 (64-bit, non-prefetchable) [size=64K]
> >         Capabilities: [40] Power Management version 2
> >         Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit-
> >         Capabilities: [60] Express Legacy Endpoint, MSI 00
> >         Capabilities: [90] MSI-X: Enable- Count=1 Masked-
> >         Capabilities: [100] Advanced Error Reporting
> >         Capabilities: [140] Virtual Channel
> >         Capabilities: [160] Device Serial Number 00-00-00-00-00-00-00-00
> >         Kernel driver in use: ath9k
> > 
> > 
> > I did also setup a WiFi network and transmitted some packages, but I did
> > not get a nobody carred message. Do you have an idea why that might be?
> > 
> > # cat /proc/interrupts
> > ...
> > 312:      10967          0          0          0       GPC 123 Level    
> > ath9k
> > ...
> > 
> > 
> > Your conclusion in this thread seem reasonable, hence reverting the
> > issue does. However, I still would like to reproduce the issue so I can
> > make sure that future patches don't break it :-)
> 
> Hm, I realized that I need to enable CONFIG_PCIEPORTBUS and set
> ath9k.use_msi=1 to get MSI for that card. However, it seems that ath9k
> does not behave well in that setup. It does get interrupts, and seems to
> work to some degree, but I was not successful in transmitting data over
> WiFi, but that might be an entirly different thing.
> 
> However, what I noticed is that when CONFIG_PCIEPORTBUS and
> CONFIG_PCI_MSI is enabled, MSI works but legacy interrupt seem not to
> fire anymore. That is true for ath9k as well as e1000e (using
> e1000e.IntMode=0 to force legacy). Is that a known issue/limitation with
> i.MX 6 PCIe?

Yes, this is a known issue with the Designware PCIe core, not just on
i.MX6. As soon as any MSI interrupt is enabled, the core doesn't
forward legacy IRQs anymore.

So if any card in your system needs legacy interrupts (and ath9k is
very likely to need this, as MSI support is pretty new and
experimental), you need to boot with "nomsi" set on the kernel command
line.

Regards,
Lucas

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-28 17:50                 ` Lucas Stach
@ 2018-11-28 17:56                   ` Stefan Agner
  0 siblings, 0 replies; 15+ messages in thread
From: Stefan Agner @ 2018-11-28 17:56 UTC (permalink / raw)
  To: Lucas Stach
  Cc: Leonard Crestez, lorenzo.pieralisi, Trent Piepho, Richard Zhu,
	linux-kernel, jingoohan1, gustavo.pimentel, andrew.smirnov,
	linux-pci, bhelgaas

On 28.11.2018 18:50, Lucas Stach wrote:
> Am Mittwoch, den 28.11.2018, 18:36 +0100 schrieb Stefan Agner:
>> On 28.11.2018 13:19, Stefan Agner wrote:
>> > On 21.11.2018 14:47, Leonard Crestez wrote:
>> > > On 11/20/2018 11:28 PM, Trent Piepho wrote:
>> > > > On Tue, 2018-11-20 at 21:42 +0100, Stefan Agner wrote:
>> > > > > On 20.11.2018 20:13, Trent Piepho wrote:
>> > > > > > It also seems to me that this doesn't need to be in the internal pci
>> > > > > > config access functions.  The driver shouldn't be reading registers
>> > > > > > that don't exist anyway.  It's really about trying to fix sysfs access
>> > > > > > to registers that don't exist.  So maybe it should be done there.
>> > > > >
>> > > > > That was my first approach, see:
>> > > >
>> > > > Yes, but that just used the pci device id which applies to every IMX
>> > > > design.
>> > > >
>> > > > It's also not totally correct, as it seems real registers after 0x200
>> > > > do work on imx6, and that would prevent access to them.
>> > >
>> > > I see that Lorenzo already accepted the patch in pci/dwc:
>> > >
>> > > https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/commit/?h=pci/dwc&id=f14eaec153aaebbe940ddd21e4198cc2abc927c2
>> > >
>> > > My tests show that this series breaks pci cards on 6qdl and I think it
>> > > should be reverted until a fix is found. Are you OK with this?
>> > >
>> > > Fixing might require an entirely different approach.
>> >
>> > I tried to reproduce this issue on Apalis iMX6 (i.MX 6Q) with a ath9k
>> > PCIe WiFi card, the issue you are seeing did not happen. My lspci looks
>> > as follows:
>> >
>> > root@ea210c63d739:/# lspci -v
>> > 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
>> > [Normal decode])
>> >         Flags: bus master, fast devsel, latency 0, IRQ 255
>> >         Memory at 01000000 (32-bit, non-prefetchable) [size=1M]
>> >         Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
>> >         Memory behind bridge: 01100000-011fffff
>> >         [virtual] Expansion ROM at 01200000 [disabled] [size=64K]
>> >         Capabilities: [40] Power Management version 3
>> >         Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
>> >         Capabilities: [70] Express Root Port (Slot-), MSI 00
>> >         Capabilities: [100] Advanced Error Reporting
>> >         Capabilities: [140] Virtual Channel
>> > lspci: Unable to load libkmod resources: error -12
>> >
>> > 01:00.0 Network controller: Qualcomm Atheros AR928X Wireless Network
>> > Adapter (PCI-Express) (rev 01)
>> >         Subsystem: Foxconn International, Inc. Device e007
>> >         Flags: bus master, fast devsel, latency 0, IRQ 312
>> >         Memory at 01100000 (64-bit, non-prefetchable) [size=64K]
>> >         Capabilities: [40] Power Management version 2
>> >         Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit-
>> >         Capabilities: [60] Express Legacy Endpoint, MSI 00
>> >         Capabilities: [90] MSI-X: Enable- Count=1 Masked-
>> >         Capabilities: [100] Advanced Error Reporting
>> >         Capabilities: [140] Virtual Channel
>> >         Capabilities: [160] Device Serial Number 00-00-00-00-00-00-00-00
>> >         Kernel driver in use: ath9k
>> >
>> >
>> > I did also setup a WiFi network and transmitted some packages, but I did
>> > not get a nobody carred message. Do you have an idea why that might be?
>> >
>> > # cat /proc/interrupts
>> > ...
>> > 312:      10967          0          0          0       GPC 123 Level    
>> > ath9k
>> > ...
>> >
>> >
>> > Your conclusion in this thread seem reasonable, hence reverting the
>> > issue does. However, I still would like to reproduce the issue so I can
>> > make sure that future patches don't break it :-)
>>
>> Hm, I realized that I need to enable CONFIG_PCIEPORTBUS and set
>> ath9k.use_msi=1 to get MSI for that card. However, it seems that ath9k
>> does not behave well in that setup. It does get interrupts, and seems to
>> work to some degree, but I was not successful in transmitting data over
>> WiFi, but that might be an entirly different thing.
>>
>> However, what I noticed is that when CONFIG_PCIEPORTBUS and
>> CONFIG_PCI_MSI is enabled, MSI works but legacy interrupt seem not to
>> fire anymore. That is true for ath9k as well as e1000e (using
>> e1000e.IntMode=0 to force legacy). Is that a known issue/limitation with
>> i.MX 6 PCIe?
> 
> Yes, this is a known issue with the Designware PCIe core, not just on
> i.MX6. As soon as any MSI interrupt is enabled, the core doesn't
> forward legacy IRQs anymore.

Oh I see, unfortunate!

> 
> So if any card in your system needs legacy interrupts (and ath9k is
> very likely to need this, as MSI support is pretty new and
> experimental), you need to boot with "nomsi" set on the kernel command
> line.

Ok, thanks for clarification.

FWIW, e1000e with MSI works perfectly fine, its just ath9k when forcing
MSI using the kernel parameter which does not work really. I guess that
is the reason it is not enabled by default.

--
Stefan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length
  2018-11-28 17:36               ` Stefan Agner
  2018-11-28 17:50                 ` Lucas Stach
@ 2018-11-28 18:01                 ` Leonard Crestez
  1 sibling, 0 replies; 15+ messages in thread
From: Leonard Crestez @ 2018-11-28 18:01 UTC (permalink / raw)
  To: stefan, l.stach
  Cc: Richard Zhu, linux-kernel, jingoohan1, lorenzo.pieralisi,
	gustavo.pimentel, tpiepho, andrew.smirnov, linux-pci, bhelgaas

On Wed, 2018-11-28 at 18:36 +0100, Stefan Agner wrote:
> On 28.11.2018 13:19, Stefan Agner wrote:
> > On 21.11.2018 14:47, Leonard Crestez wrote:
> > > My tests show that this series breaks pci cards on 6qdl and I
> > > think it should be reverted until a fix is found. Are you OK with
> > > this?
> > > 
> > > Fixing might require an entirely different approach.
> > 
> > I tried to reproduce this issue on Apalis iMX6 (i.MX 6Q) with a ath9k
> > PCIe WiFi card, the issue you are seeing did not happen. My lspci looks
> > as follows:

I think in order to get "irq: nobody cared" you need to do a soft
reboot after scanning with the card on a functional kernel.

A better way to check for issues is to print when the dbi_length check
fails. I get stack traces like this:

imx6q-pcie 1ffc000.pcie: host bridge /soc/pcie@1ffc000 ranges:
imx6q-pcie 1ffc000.pcie: Parsing ranges property...
imx6q-pcie 1ffc000.pcie:    IO 0x01f80000..0x01f8ffff -> 0x00000000
imx6q-pcie 1ffc000.pcie:   MEM 0x01000000..0x01efffff -> 0x01000000
refuse rd_own_conf where=828
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.20.0-rc1-00012-g864d906b2f62 #3
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[<c01133cc>] (unwind_backtrace) from [<c010da90>] (show_stack+0x10/0x14)
[<c010da90>] (show_stack) from [<c0b30610>] (dump_stack+0x8c/0xac)
[<c0b30610>] (dump_stack) from [<c04f1ff4>] (dw_pcie_rd_own_conf+0x6c/0x78)
[<c04f1ff4>] (dw_pcie_rd_own_conf) from [<c04f2db4>] (dw_pcie_setup_rc+0x54/0x394)
[<c04f2db4>] (dw_pcie_setup_rc) from [<c04f46bc>] (imx6_pcie_host_init+0x80/0x138)
[<c04f46bc>] (imx6_pcie_host_init) from [<c04f2ad8>] (dw_pcie_host_init+0x258/0x4e0)
[<c04f2ad8>] (dw_pcie_host_init) from [<c04f4380>] (imx6_pcie_probe+0x2b8/0x574)
[<c04f4380>] (imx6_pcie_probe) from [<c05d4834>] (platform_drv_probe+0x48/0x98)
[<c05d4834>] (platform_drv_probe) from [<c05d2630>] (really_probe+0x2a8/0x40c)
[<c05d2630>] (really_probe) from [<c05d293c>] (driver_probe_device+0x6c/0x1c0)
[<c05d293c>] (driver_probe_device) from [<c05d2ba0>] (__driver_attach+0x110/0x138)
[<c05d2ba0>] (__driver_attach) from [<c05d04e4>] (bus_for_each_dev+0x70/0xb4)
[<c05d04e4>] (bus_for_each_dev) from [<c05d17f0>] (bus_add_driver+0x1a4/0x264)
[<c05d17f0>] (bus_add_driver) from [<c05d38b4>] (driver_register+0x74/0x108)
[<c05d38b4>] (driver_register) from [<c0102f8c>] (do_one_initcall+0x80/0x26c)
[<c0102f8c>] (do_one_initcall) from [<c11010cc>] (kernel_init_freeable+0x268/0x33c)
[<c11010cc>] (kernel_init_freeable) from [<c0b492e0>] (kernel_init+0x8/0x114)
[<c0b492e0>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)

If rd/wr_own_conf is incorrectly ignored then behavior is somewhat impredictible.

--
Regards,
Leonard

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-11-28 18:01 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-20 16:56 [PATCH v3 1/2] PCI: imx6: introduce drvdata Stefan Agner
2018-11-20 16:56 ` [PATCH v3 2/2] PCI: imx6: limit DBI register length Stefan Agner
2018-11-20 18:19   ` Leonard Crestez
2018-11-20 19:13     ` Trent Piepho
2018-11-20 20:42       ` Stefan Agner
2018-11-20 21:28         ` Trent Piepho
2018-11-21 13:47           ` Leonard Crestez
2018-11-21 14:17             ` Lorenzo Pieralisi
2018-11-28 12:19             ` Stefan Agner
2018-11-28 17:36               ` Stefan Agner
2018-11-28 17:50                 ` Lucas Stach
2018-11-28 17:56                   ` Stefan Agner
2018-11-28 18:01                 ` Leonard Crestez
2018-11-26 10:16         ` Leonard Crestez
2018-11-26 16:34           ` Trent Piepho

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).