[PATCH v3 1/2] PCI: imx6: introduce drvdata

[PATCH v3 1/2] PCI: imx6: introduce drvdata

[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

[PATCH v3 2/2] PCI: imx6: limit DBI register length

[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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Leonard Crestez]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Trent Piepho]

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.

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Stefan Agner]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Trent Piepho]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Leonard Crestez]

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.

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Lorenzo Pieralisi]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Stefan Agner]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

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

--
Stefan

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Lucas Stach]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Stefan Agner]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Leonard Crestez]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Leonard Crestez]

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

Re: [PATCH v3 2/2] PCI: imx6: limit DBI register length

[Trent Piepho]

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.