diff mbox

i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity

Message ID m3h9fuk7ik.fsf@t19.piap.pl (mailing list archive)
State New, archived
Headers show

Commit Message

Krzysztof Hałasa March 25, 2016, 1:32 p.m. UTC
A recent commit 5c5fb40de8f14391a1238db05cef88754faf9229 stated:
Follows: linus/v4.4-rc2
Precedes: linus/v4.5-rc1

    PCI: imx6: Add support for active-low reset GPIO

    We previously used of_get_named_gpio(), which ignores the OF flags cell, so
    the reset GPIO defaulted to "active high." This doesn't work on the Toradex
    Apalis SoM with Ixora base board, which has an active-low reset GPIO.

    Use devm_gpiod_get_optional() instead so we pay attention to the active
    high/low flag.  This also adds support for GPIOs described via ACPI.

The (now replaced) code doesn't support the above:
@@ -287,10 +287,10 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
     usleep_range(200, 500);
 
     /* Some boards don't have PCIe reset GPIO. */
-    if (gpio_is_valid(imx6_pcie->reset_gpio)) {
-        gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
+    if (imx6_pcie->reset_gpio) {
+        gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
         msleep(100);
-        gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+        gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
     }
     return 0;

If the reset_gpio setup code had ignored the flags (haven't checked
that), then clearly the resets were active-low (most reset lines are,
because they can be then driven with open-drain/collector output).
The gpiod_set_value*(0) activates reset, gpiod_set_value(1) -
deactivates.

Now we're told the setup code is now level-aware, but the above sequence
thus _deactivates_ reset for 100 ms, then _activates_ it again. It has
no chance to work, unless a board has a broken DTS file. A quick grep
shows that about half the IMX6 boards specify an active-low PCIe reset,
4 ask for active-high, and another 4 don't bother.


I wonder if all boards (except maybe that Toradex set) use an active-low
PCIe reset and are now broken. Perhaps Toradex uses active-high and thus
works.

I'm not fixing individual DTS files because I don't really know, though
perhaps we should change them all to "active-low", since it would work
the same as before the 5c5fb40de8f14391a1238db05cef88754faf9229 change.

Confirmed to fix Gateworks Laguna GW54xx.
Without the patch, the following happens (as expected):

PCI host bridge /soc/pcie@0x01000000 ranges:
  No bus range found for /soc/pcie@0x01000000, using [bus 00-ff]
   IO 0x01f80000..0x01f8ffff -> 0x00000000
  MEM 0x01000000..0x01efffff -> 0x01000000
imx6q-pcie 1ffc000.pcie: phy link never came up

Signed-off-by: Krzysztof Ha?asa <khalasa@piap.pl>

Comments

Fabio Estevam March 27, 2016, 2:44 p.m. UTC | #1
On Fri, Mar 25, 2016 at 10:32 AM, Krzysztof Ha?asa <khalasa@piap.pl> wrote:
> A recent commit 5c5fb40de8f14391a1238db05cef88754faf9229 stated:
> Follows: linus/v4.4-rc2
> Precedes: linus/v4.5-rc1
>
>     PCI: imx6: Add support for active-low reset GPIO
>
>     We previously used of_get_named_gpio(), which ignores the OF flags cell, so
>     the reset GPIO defaulted to "active high." This doesn't work on the Toradex
>     Apalis SoM with Ixora base board, which has an active-low reset GPIO.
>
>     Use devm_gpiod_get_optional() instead so we pay attention to the active
>     high/low flag.  This also adds support for GPIOs described via ACPI.
>
> The (now replaced) code doesn't support the above:
> @@ -287,10 +287,10 @@ static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
>      usleep_range(200, 500);
>
>      /* Some boards don't have PCIe reset GPIO. */
> -    if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> -        gpio_set_value_cansleep(imx6_pcie->reset_gpio, 0);
> +    if (imx6_pcie->reset_gpio) {
> +        gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
>          msleep(100);
> -        gpio_set_value_cansleep(imx6_pcie->reset_gpio, 1);
> +        gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
>      }
>      return 0;
>
> If the reset_gpio setup code had ignored the flags (haven't checked
> that), then clearly the resets were active-low (most reset lines are,
> because they can be then driven with open-drain/collector output).
> The gpiod_set_value*(0) activates reset, gpiod_set_value(1) -
> deactivates.
>
> Now we're told the setup code is now level-aware, but the above sequence
> thus _deactivates_ reset for 100 ms, then _activates_ it again. It has
> no chance to work, unless a board has a broken DTS file. A quick grep
> shows that about half the IMX6 boards specify an active-low PCIe reset,
> 4 ask for active-high, and another 4 don't bother.
>
>
> I wonder if all boards (except maybe that Toradex set) use an active-low
> PCIe reset and are now broken. Perhaps Toradex uses active-high and thus
> works.
>
> I'm not fixing individual DTS files because I don't really know, though
> perhaps we should change them all to "active-low", since it would work
> the same as before the 5c5fb40de8f14391a1238db05cef88754faf9229 change.
>
> Confirmed to fix Gateworks Laguna GW54xx.
> Without the patch, the following happens (as expected):
>
> PCI host bridge /soc/pcie@0x01000000 ranges:
>   No bus range found for /soc/pcie@0x01000000, using [bus 00-ff]
>    IO 0x01f80000..0x01f8ffff -> 0x00000000
>   MEM 0x01000000..0x01efffff -> 0x01000000
> imx6q-pcie 1ffc000.pcie: phy link never came up
>
> Signed-off-by: Krzysztof Ha?asa <khalasa@piap.pl>

Good catch!

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

I will fix imx6q-sabresd.dtsi when this patch gets applied.
Fabio Estevam March 28, 2016, 12:26 a.m. UTC | #2
On Sun, Mar 27, 2016 at 11:44 AM, Fabio Estevam <festevam@gmail.com> wrote:

> Good catch!
>
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
>
> I will fix imx6q-sabresd.dtsi when this patch gets applied.

After thinking more about it, I think the correct fix is to revert
5c5fb40de8f143 ("PCI: imx6: Add support for active-low reset GPIO") so
that we do not break old dtb's.

Then a new dt property can be added if someone needs gpio active high PCI reset.
Tim Harvey March 28, 2016, 7:59 p.m. UTC | #3
On Sun, Mar 27, 2016 at 5:26 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sun, Mar 27, 2016 at 11:44 AM, Fabio Estevam <festevam@gmail.com> wrote:
>
>> Good catch!
>>
>> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
>>
>> I will fix imx6q-sabresd.dtsi when this patch gets applied.
>
> After thinking more about it, I think the correct fix is to revert
> 5c5fb40de8f143 ("PCI: imx6: Add support for active-low reset GPIO") so
> that we do not break old dtb's.
>
> Then a new dt property can be added if someone needs gpio active high PCI reset.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fabio,

I would agree with you on this - 5c5fb40de8f143 ("PCI: imx6: Add
support for active-low reset GPIO")  should be reverted.

I just finished bisecting an issue to this specific patch only to find
out Krzysztof found it a few days ago ;) Thanks Krzysztof.

Tim
Fabio Estevam March 28, 2016, 8:13 p.m. UTC | #4
Hi Tim,

On Mon, Mar 28, 2016 at 4:59 PM, Tim Harvey <tharvey@gateworks.com> wrote:

> Fabio,
>
> I would agree with you on this - 5c5fb40de8f143 ("PCI: imx6: Add
> support for active-low reset GPIO")  should be reverted.
>
> I just finished bisecting an issue to this specific patch only to find
> out Krzysztof found it a few days ago ;) Thanks Krzysztof.

I sent the revert patch yesterday:
http://marc.info/?l=linux-pci&m=145912622805757&w=2
Tim Harvey March 28, 2016, 8:42 p.m. UTC | #5
On Mon, Mar 28, 2016 at 1:13 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Tim,
>
> On Mon, Mar 28, 2016 at 4:59 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>
>> Fabio,
>>
>> I would agree with you on this - 5c5fb40de8f143 ("PCI: imx6: Add
>> support for active-low reset GPIO")  should be reverted.
>>
>> I just finished bisecting an issue to this specific patch only to find
>> out Krzysztof found it a few days ago ;) Thanks Krzysztof.
>
> I sent the revert patch yesterday:
> http://marc.info/?l=linux-pci&m=145912622805757&w=2

Fabio,

ok - I'll respond there as I agree with the patch but not the wording
of the commit (It's Gateworks 'Ventana' using IMX6 not Laguna and we
do define the polarity properly as active-low in Ventana dt's). It is
the fact that the gpio polarity has the wrong logic level that breaks
Ventana.

However, there seems to be another regression in 4.5 that's keeping
PCI working for me and I'm still bisecting that (using 802.11n access
points to test). Can you confirm that PCI works in v4.5 on IMX6 boards
with only 5c5fb40de8f14391a1238db05cef88754faf9229 reverted?

Tim
Fabio Estevam March 28, 2016, 9:30 p.m. UTC | #6
On Mon, Mar 28, 2016 at 5:42 PM, Tim Harvey <tharvey@gateworks.com> wrote:

> Fabio,
>
> ok - I'll respond there as I agree with the patch but not the wording
> of the commit (It's Gateworks 'Ventana' using IMX6 not Laguna and we
> do define the polarity properly as active-low in Ventana dt's). It is
> the fact that the gpio polarity has the wrong logic level that breaks
> Ventana.

Ok, I will change the wording in v2.

>
> However, there seems to be another regression in 4.5 that's keeping
> PCI working for me and I'm still bisecting that (using 802.11n access
> points to test). Can you confirm that PCI works in v4.5 on IMX6 boards
> with only 5c5fb40de8f14391a1238db05cef88754faf9229 reverted?

On imx6qdl-sabresd.dtsi the PCI reset gpio polarity is set to high,
which is not correct, so the Wifi card could be detected even with
5c5fb40de8f. So two errors in sequence and PCI still works on this
board :-)

I don't have access to the board at the moment, but the only test I
did was to see that the Wifi card got detected. I haven't really tried
to communicate via Wifi.
Tim Harvey March 28, 2016, 10:06 p.m. UTC | #7
On Mon, Mar 28, 2016 at 2:30 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Mon, Mar 28, 2016 at 5:42 PM, Tim Harvey <tharvey@gateworks.com> wrote:
>
>> Fabio,
>>
>> ok - I'll respond there as I agree with the patch but not the wording
>> of the commit (It's Gateworks 'Ventana' using IMX6 not Laguna and we
>> do define the polarity properly as active-low in Ventana dt's). It is
>> the fact that the gpio polarity has the wrong logic level that breaks
>> Ventana.
>
> Ok, I will change the wording in v2.
>
>>
>> However, there seems to be another regression in 4.5 that's keeping
>> PCI working for me and I'm still bisecting that (using 802.11n access
>> points to test). Can you confirm that PCI works in v4.5 on IMX6 boards
>> with only 5c5fb40de8f14391a1238db05cef88754faf9229 reverted?
>
> On imx6qdl-sabresd.dtsi the PCI reset gpio polarity is set to high,
> which is not correct, so the Wifi card could be detected even with
> 5c5fb40de8f. So two errors in sequence and PCI still works on this
> board :-)

ouch - two wrongs did make a right!

It's not too easy to tell how many IMX6 boards incorrectly specify
their reset-gpio polarity. I don't know what the best way to determine
what boards use the IMX6 pcie host controller. Is there a dtc usage
that will display the compiled dtb's then we grep out 'compatible =
"fsl,imx6q-pcie"' to at least get the list of boards to inspect? I'm
curious if its just one or two boards that incorrectly specify the
polarity of their PCI reset.

>
> I don't have access to the board at the moment, but the only test I
> did was to see that the Wifi card got detected. I haven't really tried
> to communicate via Wifi.

I figured out it was the change to enable CONFIG_PCI_MSI in v4.5 that
is causing interrupts to fail for me.

Lucas, the case that is failing for me is when I have 4 miniPCI radios
behind a PCIe->PCI bridge. In this case the radios get legacy
INTA/B/C/D mapped to them correctly from what I can tell (GIC
123/122/121/120 swizzled appropriately), but those interrupts never
fire. I don't think this case is necessarily a regression, I'm not
clear that it has ever worked. In fact I can't seem to come up with a
scenario where the MSI irq is firing either: IMX6->ath9k radio (no
bridge) with MSI doesn't fire the PCI-MSI interrupt or the GPC 123
interrupt that gets mapped to it via the driver. Any ideas what can be
going on here?

Regards,

Tim
Fabio Estevam March 28, 2016, 10:13 p.m. UTC | #8
On Mon, Mar 28, 2016 at 7:06 PM, Tim Harvey <tharvey@gateworks.com> wrote:

>> On imx6qdl-sabresd.dtsi the PCI reset gpio polarity is set to high,
>> which is not correct, so the Wifi card could be detected even with
>> 5c5fb40de8f. So two errors in sequence and PCI still works on this
>> board :-)
>
> ouch - two wrongs did make a right!
>
> It's not too easy to tell how many IMX6 boards incorrectly specify
> their reset-gpio polarity. I don't know what the best way to determine
> what boards use the IMX6 pcie host controller. Is there a dtc usage
> that will display the compiled dtb's then we grep out 'compatible =
> "fsl,imx6q-pcie"' to at least get the list of boards to inspect? I'm
> curious if its just one or two boards that incorrectly specify the
> polarity of their PCI reset.

In order to keep old dtb's working we should simply ignore the GPIO
flags passed in the 'reset-gpio' property.

That's why we need a revert. Just sent a v2, BTW.
Krzysztof Hałasa March 29, 2016, 5:21 a.m. UTC | #9
Tim Harvey <tharvey@gateworks.com> writes:

> ok - I'll respond there as I agree with the patch but not the wording
> of the commit (It's Gateworks 'Ventana' using IMX6 not Laguna and we
> do define the polarity properly as active-low in Ventana dt's).

Right, it's Ventana of course (I had been working with Laguna boards
recently).

> However, there seems to be another regression in 4.5 that's keeping
> PCI working for me and I'm still bisecting that (using 802.11n access
> points to test). Can you confirm that PCI works in v4.5 on IMX6 boards
> with only 5c5fb40de8f14391a1238db05cef88754faf9229 reverted?

I will check with my frame buffer and wifi cards.
Krzysztof Hałasa March 29, 2016, 5:29 a.m. UTC | #10
Tim Harvey <tharvey@gateworks.com> writes:

> It's not too easy to tell how many IMX6 boards incorrectly specify
> their reset-gpio polarity. I don't know what the best way to determine
> what boards use the IMX6 pcie host controller. Is there a dtc usage
> that will display the compiled dtb's then we grep out 'compatible =
> "fsl,imx6q-pcie"' to at least get the list of boards to inspect? I'm
> curious if its just one or two boards that incorrectly specify the
> polarity of their PCI reset.

I guess, maybe 8 of them. Not counting those with out-of-tree DTS/DTB
files.

Something like:
$ grep reset-gpio arch/arm/boot/dts/imx6* | grep -v phy-reset

> I figured out it was the change to enable CONFIG_PCI_MSI in v4.5 that
> is causing interrupts to fail for me.

Right, a long standing issue. MSI never worked for me on i.MX6.
Krzysztof Hałasa March 29, 2016, 5:40 a.m. UTC | #11
Fabio Estevam <festevam@gmail.com> writes:

> In order to keep old dtb's working we should simply ignore the GPIO
> flags passed in the 'reset-gpio' property.
>
> That's why we need a revert. Just sent a v2, BTW.

OTOH, we should fix it some day, making sure the DTS files are fixed
first:

imx6qdl-apf6dev.dtsi:       reset-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>;
imx6qdl-aristainetos2.dtsi: reset-gpio = <&gpio2 16 GPIO_ACTIVE_HIGH>;
imx6qdl-hummingboard.dtsi:  reset-gpio = <&gpio3 4 0>; (I think RMK already handles this)
imx6qdl-phytec-pfla02.dtsi: reset-gpio = <&gpio4 17 0>;
imx6qdl-sabresd.dtsi:       reset-gpio = <&gpio7 12 0>;
imx6q-dmo-edmqmx6.dts:      reset-gpio = <&gpio4 8 0>;
imx6q-novena.dts:           reset-gpio = <&gpio3 29 GPIO_ACTIVE_HIGH>;
imx6q-tbs2910.dts:          reset-gpio = <&gpio7 12 GPIO_ACTIVE_HIGH>;

The original patch was a bad implementation of a good idea.
Krzysztof Hałasa March 29, 2016, 5:43 a.m. UTC | #12
> OTOH, we should fix it some day, making sure the DTS files are fixed
> first:
>
> imx6qdl-apf6dev.dtsi:       reset-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>;
> imx6qdl-aristainetos2.dtsi: reset-gpio = <&gpio2 16 GPIO_ACTIVE_HIGH>;
> imx6qdl-hummingboard.dtsi:  reset-gpio = <&gpio3 4 0>; (I think RMK already handles this)
> imx6qdl-phytec-pfla02.dtsi: reset-gpio = <&gpio4 17 0>;
> imx6qdl-sabresd.dtsi:       reset-gpio = <&gpio7 12 0>;
> imx6q-dmo-edmqmx6.dts:      reset-gpio = <&gpio4 8 0>;
> imx6q-novena.dts:           reset-gpio = <&gpio3 29 GPIO_ACTIVE_HIGH>;
> imx6q-tbs2910.dts:          reset-gpio = <&gpio7 12 GPIO_ACTIVE_HIGH>;

Or maybe we should simply change these to *_LOW, add that short patch
from me, and forget about it.
Lucas Stach March 29, 2016, 8:55 a.m. UTC | #13
Am Montag, den 28.03.2016, 15:06 -0700 schrieb Tim Harvey:
> On Mon, Mar 28, 2016 at 2:30 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > On Mon, Mar 28, 2016 at 5:42 PM, Tim Harvey <tharvey@gateworks.com> wrote:
> >
> >> Fabio,
> >>
> >> ok - I'll respond there as I agree with the patch but not the wording
> >> of the commit (It's Gateworks 'Ventana' using IMX6 not Laguna and we
> >> do define the polarity properly as active-low in Ventana dt's). It is
> >> the fact that the gpio polarity has the wrong logic level that breaks
> >> Ventana.
> >
> > Ok, I will change the wording in v2.
> >
> >>
> >> However, there seems to be another regression in 4.5 that's keeping
> >> PCI working for me and I'm still bisecting that (using 802.11n access
> >> points to test). Can you confirm that PCI works in v4.5 on IMX6 boards
> >> with only 5c5fb40de8f14391a1238db05cef88754faf9229 reverted?
> >
> > On imx6qdl-sabresd.dtsi the PCI reset gpio polarity is set to high,
> > which is not correct, so the Wifi card could be detected even with
> > 5c5fb40de8f. So two errors in sequence and PCI still works on this
> > board :-)
> 
> ouch - two wrongs did make a right!
> 
> It's not too easy to tell how many IMX6 boards incorrectly specify
> their reset-gpio polarity. I don't know what the best way to determine
> what boards use the IMX6 pcie host controller. Is there a dtc usage
> that will display the compiled dtb's then we grep out 'compatible =
> "fsl,imx6q-pcie"' to at least get the list of boards to inspect? I'm
> curious if its just one or two boards that incorrectly specify the
> polarity of their PCI reset.
> 
I would suspect that most boards specify the reset polarity the wrong
way around. Fixing this without breaking DT stability is hard. OTOH we
could just argue that the system description in DT is wrong and needs to
be fixed.

> >
> > I don't have access to the board at the moment, but the only test I
> > did was to see that the Wifi card got detected. I haven't really tried
> > to communicate via Wifi.
> 
> I figured out it was the change to enable CONFIG_PCI_MSI in v4.5 that
> is causing interrupts to fail for me.
> 
Is this working with v4.4 and PCI_MSI enabled? I'm sure I've tested MSI
IRQs before enabling them in the defconfig and they have been working
for me for a long time before that. Tested with i210 on Gateworks
Ventana.

> Lucas, the case that is failing for me is when I have 4 miniPCI radios
> behind a PCIe->PCI bridge. In this case the radios get legacy
> INTA/B/C/D mapped to them correctly from what I can tell (GIC
> 123/122/121/120 swizzled appropriately), but those interrupts never
> fire. I don't think this case is necessarily a regression, I'm not
> clear that it has ever worked. In fact I can't seem to come up with a
> scenario where the MSI irq is firing either: IMX6->ath9k radio (no
> bridge) with MSI doesn't fire the PCI-MSI interrupt or the GPC 123
> interrupt that gets mapped to it via the driver. Any ideas what can be
> going on here?
> 
Please test if MSI IRQs work with v4.4. I'll take a look at this later
today.

Regards,
Lucas
Krzysztof Hałasa March 29, 2016, 10:39 a.m. UTC | #14
Lucas Stach <l.stach@pengutronix.de> writes:

> Is this working with v4.4 and PCI_MSI enabled? I'm sure I've tested MSI
> IRQs before enabling them in the defconfig and they have been working
> for me for a long time before that. Tested with i210 on Gateworks
> Ventana.

MSI never worked for me on Ventana. I have been using 4.2 extensively,
and now I'm switching to 4.5 (which doesn't work either).

Could it be a DTS (bridge) problem(?)

On 4.5, trying to use it with TW6869 frame buffer and GW5410:

TW6869: PCI 0000:04:00.0, IRQ 336, MMIO 0x1100000
TW686x 0000:04:00.0: enabling device (0140 -> 0142)

           CPU0       CPU1       CPU2       CPU3
 16:       1165       1032       1271       1591     GIC-0  29 Edge      twd
 17:        879        387       1404        606       GPC  55 Level     i.MX Timer Tick
 18:       6434          0          0          0       GPC  13 Level     mxs-dma
 19:          0          0          0          0       GPC  15 Level     bch
 21:          0          0          0          0       GPC   9 Level     130000.gpu
 22:          0          0          0          0       GPC  10 Level     134000.gpu
 24:          0          0          0          0       GPC 120 Level     mx6-pcie-msi
 26:          0          0          0          0       GPC  26 Level     2020000.serial
 30:          0          0          0          0       GPC  12 Level     2040000.vpu
240:          0          0          0          0  gpio-mxc   0 Edge      2198000.usdhc cd
280:          0          0          0          0       GPC  19 Level     rtc alarm
286:          0          0          0          0       GPC   2 Level     sdma
287:          0          0          0          0       GPC  43 Level     2184000.usb
288:         32          0          0          0       GPC  40 Level     2184200.usb
289:       2294          0          0          0     GIC-0 150 Level     2188000.ethernet
290:          0          0          0          0     GIC-0 151 Level     2188000.ethernet
291:          0          0          0          0       GPC  24 Level     mmc0
292:          0          0          0          0       GPC  36 Level     21a0000.i2c
293:          0          0          0          0       GPC  37 Level     21a4000.i2c
294:          0          0          0          0       GPC  38 Level     21a8000.i2c
296:       1422          0          0          0       GPC  27 Level     21e8000.serial
297:          0          0          0          0       GPC  30 Level     21f4000.serial
300:          0          0          0          0       GPC  39 Level     ahci-imx[2200000.sata]
301:          0          0          0          0       GPC  11 Level     2204000.gpu
304:          0          0          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
336:          0          0          0          0       GPC 123 Level     TW6869
339:          0          0          0          0       IPU 457 Edge      (null)
340:          0          0          0          0       IPU 451 Edge      (null)
341:          0          0          0          0       IPU 457 Edge      (null)
342:          0          0          0          0       IPU 451 Edge      (null)
IPI0:          0          0          0          0  CPU wakeup interrupts
IPI1:        183        111         90         57  Timer broadcast interrupts
IPI2:        453       2091       6539       2088  Rescheduling interrupts
IPI3:         37         32         29         23  Function call interrupts
IPI4:          0          0          0          0  CPU stop interrupts
IPI5:          0          0          0          1  IRQ work interrupts
IPI6:          0          0          0          0  completion interrupts
Err:          0

00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01)
01:00.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
01:00.1 System peripheral: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
02:01.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
02:04.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
02:05.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
02:06.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
02:07.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
02:08.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
02:09.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
04:00.0 Multimedia controller: Techwell Inc. Device 6869 (rev 01)
08:00.0 Ethernet controller: Marvell Technology Group Ltd. 88E8057 PCI-E Gigabit Ethernet Controller

-[0000:00]---00.0-[01]--+-00.0-[02-09]--+-01.0-[03]--
                        |               +-04.0-[04]----00.0
                        |               +-05.0-[05]--
                        |               +-06.0-[06]--
                        |               +-07.0-[07]--
                        |               +-08.0-[08]----00.0
                        |               \-09.0-[09]--
                        \-00.1
Lucas Stach March 29, 2016, 10:55 a.m. UTC | #15
Am Dienstag, den 29.03.2016, 12:39 +0200 schrieb Krzysztof Ha?asa:
> Lucas Stach <l.stach@pengutronix.de> writes:
> 
> > Is this working with v4.4 and PCI_MSI enabled? I'm sure I've tested MSI
> > IRQs before enabling them in the defconfig and they have been working
> > for me for a long time before that. Tested with i210 on Gateworks
> > Ventana.
> 
> MSI never worked for me on Ventana. I have been using 4.2 extensively,
> and now I'm switching to 4.5 (which doesn't work either).
> 
> Could it be a DTS (bridge) problem(?)
> 
> On 4.5, trying to use it with TW6869 frame buffer and GW5410:
> 
> TW6869: PCI 0000:04:00.0, IRQ 336, MMIO 0x1100000
> TW686x 0000:04:00.0: enabling device (0140 -> 0142)
> 
I don't see whee the device even tries to use MSI IRQs. Even if the
infrastructure is enabled it opts to use legacy INTA.

There is no upstream driver for this chip, so I don't know where to look
to find out if the driver tries to enable MSI.

Is what you are saying that if you enable MSI support in the kernel, it
breaks legacy IRQs?

Regards,
Lucas

>            CPU0       CPU1       CPU2       CPU3
>  16:       1165       1032       1271       1591     GIC-0  29 Edge      twd
>  17:        879        387       1404        606       GPC  55 Level     i.MX Timer Tick
>  18:       6434          0          0          0       GPC  13 Level     mxs-dma
>  19:          0          0          0          0       GPC  15 Level     bch
>  21:          0          0          0          0       GPC   9 Level     130000.gpu
>  22:          0          0          0          0       GPC  10 Level     134000.gpu
>  24:          0          0          0          0       GPC 120 Level     mx6-pcie-msi
>  26:          0          0          0          0       GPC  26 Level     2020000.serial
>  30:          0          0          0          0       GPC  12 Level     2040000.vpu
> 240:          0          0          0          0  gpio-mxc   0 Edge      2198000.usdhc cd
> 280:          0          0          0          0       GPC  19 Level     rtc alarm
> 286:          0          0          0          0       GPC   2 Level     sdma
> 287:          0          0          0          0       GPC  43 Level     2184000.usb
> 288:         32          0          0          0       GPC  40 Level     2184200.usb
> 289:       2294          0          0          0     GIC-0 150 Level     2188000.ethernet
> 290:          0          0          0          0     GIC-0 151 Level     2188000.ethernet
> 291:          0          0          0          0       GPC  24 Level     mmc0
> 292:          0          0          0          0       GPC  36 Level     21a0000.i2c
> 293:          0          0          0          0       GPC  37 Level     21a4000.i2c
> 294:          0          0          0          0       GPC  38 Level     21a8000.i2c
> 296:       1422          0          0          0       GPC  27 Level     21e8000.serial
> 297:          0          0          0          0       GPC  30 Level     21f4000.serial
> 300:          0          0          0          0       GPC  39 Level     ahci-imx[2200000.sata]
> 301:          0          0          0          0       GPC  11 Level     2204000.gpu
> 304:          0          0          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
> 336:          0          0          0          0       GPC 123 Level     TW6869
> 339:          0          0          0          0       IPU 457 Edge      (null)
> 340:          0          0          0          0       IPU 451 Edge      (null)
> 341:          0          0          0          0       IPU 457 Edge      (null)
> 342:          0          0          0          0       IPU 451 Edge      (null)
> IPI0:          0          0          0          0  CPU wakeup interrupts
> IPI1:        183        111         90         57  Timer broadcast interrupts
> IPI2:        453       2091       6539       2088  Rescheduling interrupts
> IPI3:         37         32         29         23  Function call interrupts
> IPI4:          0          0          0          0  CPU stop interrupts
> IPI5:          0          0          0          1  IRQ work interrupts
> IPI6:          0          0          0          0  completion interrupts
> Err:          0
> 
> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01)
> 01:00.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 01:00.1 System peripheral: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 02:01.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 02:04.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 02:05.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 02:06.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 02:07.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 02:08.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 02:09.0 PCI bridge: PLX Technology, Inc. PEX 8609 8-lane, 8-Port PCI Express Gen 2 (5.0 GT/s) Switch with DMA (rev ba)
> 04:00.0 Multimedia controller: Techwell Inc. Device 6869 (rev 01)
> 08:00.0 Ethernet controller: Marvell Technology Group Ltd. 88E8057 PCI-E Gigabit Ethernet Controller
> 
> -[0000:00]---00.0-[01]--+-00.0-[02-09]--+-01.0-[03]--
>                         |               +-04.0-[04]----00.0
>                         |               +-05.0-[05]--
>                         |               +-06.0-[06]--
>                         |               +-07.0-[07]--
>                         |               +-08.0-[08]----00.0
>                         |               \-09.0-[09]--
>                         \-00.1
>
Arnd Bergmann March 29, 2016, 1:12 p.m. UTC | #16
On Tuesday 29 March 2016 12:55:21 Lucas Stach wrote:
> Am Dienstag, den 29.03.2016, 12:39 +0200 schrieb Krzysztof Ha?asa:
> > Lucas Stach <l.stach@pengutronix.de> writes:
> > 
> > > Is this working with v4.4 and PCI_MSI enabled? I'm sure I've tested MSI
> > > IRQs before enabling them in the defconfig and they have been working
> > > for me for a long time before that. Tested with i210 on Gateworks
> > > Ventana.
> > 
> > MSI never worked for me on Ventana. I have been using 4.2 extensively,
> > and now I'm switching to 4.5 (which doesn't work either).
> > 
> > Could it be a DTS (bridge) problem(?)
> > 
> > On 4.5, trying to use it with TW6869 frame buffer and GW5410:
> > 
> > TW6869: PCI 0000:04:00.0, IRQ 336, MMIO 0x1100000
> > TW686x 0000:04:00.0: enabling device (0140 -> 0142)
> > 
> I don't see whee the device even tries to use MSI IRQs. Even if the
> infrastructure is enabled it opts to use legacy INTA.

It just never calls pci_enable_msi(), right? MSI is purely opt-in
for the driver, but it should just work if the device supports it
and you add that call.

	Arnd
Tim Harvey March 29, 2016, 1:32 p.m. UTC | #17
On Tue, Mar 29, 2016 at 3:55 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Dienstag, den 29.03.2016, 12:39 +0200 schrieb Krzysztof Ha?asa:
>> Lucas Stach <l.stach@pengutronix.de> writes:
>>
>> > Is this working with v4.4 and PCI_MSI enabled? I'm sure I've tested MSI
>> > IRQs before enabling them in the defconfig and they have been working
>> > for me for a long time before that. Tested with i210 on Gateworks
>> > Ventana.
>>
>> MSI never worked for me on Ventana. I have been using 4.2 extensively,
>> and now I'm switching to 4.5 (which doesn't work either).
>>
>> Could it be a DTS (bridge) problem(?)

Lucas,

It's not the bridge - its the fact that not all drivers support MSI interrupts.

One of the most common uses of PCI on Ventana is 802.11n radios and of
them the ath9k driver is very commonly used and does not request an
MSI interrupt (drivers/net/wireless/ath/ath9k/pci.c)

>>
>> On 4.5, trying to use it with TW6869 frame buffer and GW5410:
>>
>> TW6869: PCI 0000:04:00.0, IRQ 336, MMIO 0x1100000
>> TW686x 0000:04:00.0: enabling device (0140 -> 0142)
>>
> I don't see whee the device even tries to use MSI IRQs. Even if the
> infrastructure is enabled it opts to use legacy INTA.

yes, that's what many drivers do.

>
> There is no upstream driver for this chip, so I don't know where to look
> to find out if the driver tries to enable MSI.
>
> Is what you are saying that if you enable MSI support in the kernel, it
> breaks legacy IRQs?

Yes - any driver that does not support MSI will use legacy IRQ's and
they never fire.

The Ventana GW53xx and GW54xx boards have a Marvell PCIe GigE
supported by the sky2 driver as eth1 which does support MSI and I
verified it gets an MSI interrupt and does work (along ath9k devices
with legacy interrupts that fail to work).

root@ventana:~# cat /proc/interrupts | grep MSI
299:          0          0   PCI-MSI   0 Edge      PCIe PME, aerdrv
308:       8726          0   PCI-MSI   9 Edge      eth1

So it appears that MSI works for those drivers that use it, but does
somehow cause legacy IRQ's to break.

I sent you a GW5400 dev kit over a while back to use for through
bridge testing on IMX6 that you should be able to repeat this with
assuming you have a PCIe card with a driver that doesn't support MSI
(or that you can tweak its driver to not support MSI).

I think 31e98e0d24cd2537a63e06e235e050a06b175df7 "ARM:
imx_v6_v7_defconfig: enable PCI_MSI" should be reverted as well until
we figure this out.

Tim
Fabio Estevam March 29, 2016, 2:14 p.m. UTC | #18
On Tue, Mar 29, 2016 at 5:55 AM, Lucas Stach <l.stach@pengutronix.de> wrote:

> I would suspect that most boards specify the reset polarity the wrong
> way around. Fixing this without breaking DT stability is hard. OTOH we

It is not hard if we just revert the buggy commit.

> could just argue that the system description in DT is wrong and needs to
> be fixed.

Sure, this would be a nice thing to do as well.
Krzysztof Hałasa March 30, 2016, 8:10 a.m. UTC | #19
Lucas Stach <l.stach@pengutronix.de> writes:

>> TW6869: PCI 0000:04:00.0, IRQ 336, MMIO 0x1100000
>> TW686x 0000:04:00.0: enabling device (0140 -> 0142)
>>
> I don't see whee the device even tries to use MSI IRQs. Even if the
> infrastructure is enabled it opts to use legacy INTA.

It only tries to use normal IRQ.

> There is no upstream driver for this chip, so I don't know where to look
> to find out if the driver tries to enable MSI.

It's been posted on linux-media list... I added pci_enable_msi() to this
driver and it didn't help.

> Is what you are saying that if you enable MSI support in the kernel, it
> breaks legacy IRQs?

Precisely.

However, MSI doesn't seem to work either. Could be a problem specific to
this TW6869 card.

Ventana GW5410 has 5 mPCIe slots, and (with MSI enabled in the system)
those IRQs (non-MSI) don't work in any slot:

304: 0 0 0 0  PCI-MSI   0 Edge   PCIe PME, aerdrv
336: 0 0 0 0      GPC 123 Level  TW8689 in J7 slot
337: 0 0 0 0      GPC 122 Level  TW8689 in J8, J10, J11
338: 0 0 0 0      GPC 121 Level  TW8689 in J6)

If I enable MSI on this card (adding pci_enable_msi()):
313: 0 0 0 0  PCI-MSI   9 Edge   TW6869 in J7 slot

The only way I can get it to work is by disabling MSI (system wide).
Petr Štetiar March 30, 2016, 12:06 p.m. UTC | #20
Krzysztof Ha?asa <khalasa@piap.pl> [2016-03-25 14:32:35]:

Cze??,

> I wonder if all boards (except maybe that Toradex set) use an active-low
> PCIe reset and are now broken. Perhaps Toradex uses active-high and thus
> works.

I'm really puzzled by this :-) With your patch applied I get following on
Toradex Apalis modules:

 DTS:         reset-gpio = <&gpio1 28 GPIO_ACTIVE_LOW>;
 dmesg:       imx6q-pcie 1ffc000.pcie: phy link never came up
 gpio:        gpio-28  (                    |reset               ) out hi
 pin voltage: 0V

 DTS:         reset-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
 dmesg:       ath9k 0000:01:00.0: enabling device (0140 -> 0142)
 gpio:        gpio-28  (                    |reset               ) out lo
 pin voltage: 3V3

So Toradex Apalis is actually active-high? Thanks.

-- ynezz
Fabio Estevam March 30, 2016, 12:45 p.m. UTC | #21
On Wed, Mar 30, 2016 at 9:06 AM, Petr Štetiar <ynezz@true.cz> wrote:

> I'm really puzzled by this :-) With your patch applied I get following on
> Toradex Apalis modules:
>
>  DTS:         reset-gpio = <&gpio1 28 GPIO_ACTIVE_LOW>;
>  dmesg:       imx6q-pcie 1ffc000.pcie: phy link never came up
>  gpio:        gpio-28  (                    |reset               ) out hi
>  pin voltage: 0V
>
>  DTS:         reset-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>  dmesg:       ath9k 0000:01:00.0: enabling device (0140 -> 0142)
>  gpio:        gpio-28  (                    |reset               ) out lo
>  pin voltage: 3V3
>
> So Toradex Apalis is actually active-high? Thanks.

Yes, exactly. That's why you need to introduce a new property to
handle the active-high case, so that old dtb's are not broken.
Marcel Ziswiler March 30, 2016, 2:38 p.m. UTC | #22
Hi Petr

On Wed, 2016-03-30 at 14:06 +0200, Petr Štetiar wrote:
> Krzysztof Ha?asa <khalasa@piap.pl> [2016-03-25 14:32:35]:

> 

> Cze??,

> 

> > 

> > I wonder if all boards (except maybe that Toradex set) use an

> > active-low

> > PCIe reset and are now broken. Perhaps Toradex uses active-high and

> > thus

> > works.

> I'm really puzzled by this :-) With your patch applied I get

> following on

> Toradex Apalis modules:

> 

>  DTS:         reset-gpio = <&gpio1 28 GPIO_ACTIVE_LOW>;

>  dmesg:       imx6q-pcie 1ffc000.pcie: phy link never came up

>  gpio:        gpio-28  (                    |reset               )

> out hi

>  pin voltage: 0V

> 

>  DTS:         reset-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;

>  dmesg:       ath9k 0000:01:00.0: enabling device (0140 -> 0142)

>  gpio:        gpio-28  (                    |reset               )

> out lo

>  pin voltage: 3V3

> 

> So Toradex Apalis is actually active-high?


Yes, I actually explained this in detail in my cover letter:

http://article.gmane.org/gmane.linux.drivers.devicetree/154829

>  Thanks.

> 

> -- ynezz



Cheers

Marcel
Tim Harvey March 31, 2016, 4:19 p.m. UTC | #23
On Wed, Mar 30, 2016 at 1:10 AM, Krzysztof Ha?asa <khalasa@piap.pl> wrote:
> Lucas Stach <l.stach@pengutronix.de> writes:
>
>>> TW6869: PCI 0000:04:00.0, IRQ 336, MMIO 0x1100000
>>> TW686x 0000:04:00.0: enabling device (0140 -> 0142)
>>>
>> I don't see whee the device even tries to use MSI IRQs. Even if the
>> infrastructure is enabled it opts to use legacy INTA.
>
> It only tries to use normal IRQ.
>
>> There is no upstream driver for this chip, so I don't know where to look
>> to find out if the driver tries to enable MSI.
>
> It's been posted on linux-media list... I added pci_enable_msi() to this
> driver and it didn't help.
>
>> Is what you are saying that if you enable MSI support in the kernel, it
>> breaks legacy IRQs?
>
> Precisely.
>
> However, MSI doesn't seem to work either. Could be a problem specific to
> this TW6869 card.
>
> Ventana GW5410 has 5 mPCIe slots, and (with MSI enabled in the system)
> those IRQs (non-MSI) don't work in any slot:
>
> 304: 0 0 0 0  PCI-MSI   0 Edge   PCIe PME, aerdrv
> 336: 0 0 0 0      GPC 123 Level  TW8689 in J7 slot
> 337: 0 0 0 0      GPC 122 Level  TW8689 in J8, J10, J11
> 338: 0 0 0 0      GPC 121 Level  TW8689 in J6)
>
> If I enable MSI on this card (adding pci_enable_msi()):
> 313: 0 0 0 0  PCI-MSI   9 Edge   TW6869 in J7 slot
>
> The only way I can get it to work is by disabling MSI (system wide).

Krzysztof,

I have found that MSI does work on IMX6 through a bridge (as on the
GW5410) and not, for devices/drivers that support MSI, but it does
break devices/drivers that use legacy irqs as we've discussed.

The MSI capable devices/drivers I've been able to show working with
MSI enabled are:
 - Marvell sky2 GigE (present on GW53xx and GW54xx both through a PEX switch)
 - Ath10k 802.11n radio miniPCIe socket (tested on GW51xx with no
switch and GW53xx with switch).

So perhaps there is something else going on with the tw8689
device/driver that is causing it to not work with MSI. We have an
avc8000 miniPCIe with tw8689 here and can test if you send me your
patches that enable tw8689 with msi.

Regardless of MSI working in our tests we still disable it because of
it breaking legacy irqs and for performance reasons.

Regards,

Tim
Krzysztof Hałasa April 4, 2016, 10:37 a.m. UTC | #24
Tim,

> So perhaps there is something else going on with the tw8689
> device/driver that is causing it to not work with MSI. We have an
> avc8000 miniPCIe with tw8689 here and can test if you send me your
> patches that enable tw8689 with msi.

I think you can use this:
http://git.linuxtv.org/hverkuil/media_tree.git/log/?h=tw686x

> Regardless of MSI working in our tests we still disable it because of
> it breaking legacy irqs and for performance reasons.

So do I. However I'm also using Fedora 23 and this means I have to
recompile the kernel, since they build it with MSI enabled.
I think we should fix it, either by disabling in run time, or making it
work.

Performance-wise, I found it "surprising" that one can't have multiple
MSIs with separate hw vector for each.
diff mbox

Patch

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index fe60096..f17fb02 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -288,9 +288,9 @@  static int imx6_pcie_deassert_core_reset(struct pcie_port *pp)
 
 	/* Some boards don't have PCIe reset GPIO. */
 	if (imx6_pcie->reset_gpio) {
-		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
-		msleep(100);
 		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 1);
+		msleep(100);
+		gpiod_set_value_cansleep(imx6_pcie->reset_gpio, 0);
 	}
 	return 0;