Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1364668 > unrolled thread
| Started by | khalasa@piap.pl (Krzysztof Hałasa) |
|---|---|
| First post | 2016-03-25 14:40 +0100 |
| Last post | 2016-03-30 18:20 +0200 |
| Articles | 20 on this page of 41 — 9 participants |
Back to article view | Back to linux.kernel
[PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity khalasa@piap.pl (Krzysztof Hałasa) - 2016-03-25 14:40 +0100
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Fabio Estevam <festevam@gmail.com> - 2016-03-27 16:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Fabio Estevam <festevam@gmail.com> - 2016-03-28 02:30 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-28 22:00 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Fabio Estevam <festevam@gmail.com> - 2016-03-28 22:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-28 22:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Fabio Estevam <festevam@gmail.com> - 2016-03-28 23:40 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-29 00:10 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Fabio Estevam <festevam@gmail.com> - 2016-03-29 00:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity khalasa@piap.pl (Krzysztof Hałasa) - 2016-03-29 07:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity khalasa@piap.pl (Krzysztof Hałasa) - 2016-03-29 07:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity khalasa@piap.pl (Krzysztof Hałasa) - 2016-03-29 07:30 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Lucas Stach <l.stach@pengutronix.de> - 2016-03-29 11:00 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity khalasa@piap.pl (Krzysztof Hałasa) - 2016-03-29 12:40 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Lucas Stach <l.stach@pengutronix.de> - 2016-03-29 13:00 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Arnd Bergmann <arnd@arndb.de> - 2016-03-29 15:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-29 15:40 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Arnd Bergmann <arnd@arndb.de> - 2016-03-29 16:00 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-29 16:30 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Arnd Bergmann <arnd@arndb.de> - 2016-03-29 17:00 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-29 17:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Arnd Bergmann <arnd@arndb.de> - 2016-03-29 17:30 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-29 19:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Arnd Bergmann <arnd@arndb.de> - 2016-03-29 21:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Marc Zyngier <marc.zyngier@arm.com> - 2016-03-29 20:00 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Roberto Fichera <kernel@tekno-soft.it> - 2016-03-29 18:30 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Roberto Fichera <kernel@tekno-soft.it> - 2016-03-29 18:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-29 19:40 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Roberto Fichera <kernel@tekno-soft.it> - 2016-03-30 10:10 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Arnd Bergmann <arnd@arndb.de> - 2016-03-30 12:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Roberto Fichera <kernel@tekno-soft.it> - 2016-03-30 15:00 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-30 15:40 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Roberto Fichera <kernel@tekno-soft.it> - 2016-03-30 17:30 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-29 18:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity khalasa@piap.pl (Krzysztof Hałasa) - 2016-03-30 10:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Tim Harvey <tharvey@gateworks.com> - 2016-03-31 18:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Fabio Estevam <festevam@gmail.com> - 2016-03-29 16:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity khalasa@piap.pl (Krzysztof Hałasa) - 2016-03-29 07:30 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Petr Štetiar <ynezz@true.cz> - 2016-03-30 14:20 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Fabio Estevam <festevam@gmail.com> - 2016-03-30 14:50 +0200
Re: [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity Marcel Ziswiler <marcel.ziswiler@toradex.com> - 2016-03-30 18:20 +0200
Page 1 of 3 [1] 2 3 Next page →
| From | khalasa@piap.pl (Krzysztof Hałasa) |
|---|---|
| Date | 2016-03-25 14:40 +0100 |
| Subject | [PATCH] i.MX6 PCIe: Fix imx6_pcie_deassert_core_reset() polarity |
| Message-ID | <rgA82-2iN-7@gated-at.bofh.it> |
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>
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;
[toc] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2016-03-27 16:50 +0200 |
| Message-ID | <rhkaR-1jy-1@gated-at.bofh.it> |
| In reply to | #1364668 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2016-03-28 02:30 +0200 |
| Message-ID | <rhtea-7Go-19@gated-at.bofh.it> |
| In reply to | #1365118 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Tim Harvey <tharvey@gateworks.com> |
|---|---|
| Date | 2016-03-28 22:00 +0200 |
| Message-ID | <rhLup-3dJ-3@gated-at.bofh.it> |
| In reply to | #1365249 |
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
[toc] | [prev] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2016-03-28 22:20 +0200 |
| Message-ID | <rhLNM-3BZ-5@gated-at.bofh.it> |
| In reply to | #1365651 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tim Harvey <tharvey@gateworks.com> |
|---|---|
| Date | 2016-03-28 22:50 +0200 |
| Message-ID | <rhMgO-3OW-3@gated-at.bofh.it> |
| In reply to | #1365656 |
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
[toc] | [prev] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2016-03-28 23:40 +0200 |
| Message-ID | <rhN3c-4uL-7@gated-at.bofh.it> |
| In reply to | #1365664 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Tim Harvey <tharvey@gateworks.com> |
|---|---|
| Date | 2016-03-29 00:10 +0200 |
| Message-ID | <rhNwe-509-11@gated-at.bofh.it> |
| In reply to | #1365689 |
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
[toc] | [prev] | [next] | [standalone]
| From | Fabio Estevam <festevam@gmail.com> |
|---|---|
| Date | 2016-03-29 00:20 +0200 |
| Message-ID | <rhNFU-53u-9@gated-at.bofh.it> |
| In reply to | #1365704 |
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.
[toc] | [prev] | [next] | [standalone]
| From | khalasa@piap.pl (Krzysztof Hałasa) |
|---|---|
| Date | 2016-03-29 07:50 +0200 |
| Message-ID | <rhUHn-1Ar-1@gated-at.bofh.it> |
| In reply to | #1365709 |
> 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. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[toc] | [prev] | [next] | [standalone]
| From | khalasa@piap.pl (Krzysztof Hałasa) |
|---|---|
| Date | 2016-03-29 07:50 +0200 |
| Message-ID | <rhUHn-1Ar-3@gated-at.bofh.it> |
| In reply to | #1365709 |
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 Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[toc] | [prev] | [next] | [standalone]
| From | khalasa@piap.pl (Krzysztof Hałasa) |
|---|---|
| Date | 2016-03-29 07:30 +0200 |
| Message-ID | <rhUo2-1tB-7@gated-at.bofh.it> |
| In reply to | #1365704 |
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 Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[toc] | [prev] | [next] | [standalone]
| From | Lucas Stach <l.stach@pengutronix.de> |
|---|---|
| Date | 2016-03-29 11:00 +0200 |
| Message-ID | <rhXFh-3uj-15@gated-at.bofh.it> |
| In reply to | #1365704 |
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
[toc] | [prev] | [next] | [standalone]
| From | khalasa@piap.pl (Krzysztof Hałasa) |
|---|---|
| Date | 2016-03-29 12:40 +0200 |
| Message-ID | <rhZe2-4MC-31@gated-at.bofh.it> |
| In reply to | #1366026 |
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
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
[toc] | [prev] | [next] | [standalone]
| From | Lucas Stach <l.stach@pengutronix.de> |
|---|---|
| Date | 2016-03-29 13:00 +0200 |
| Message-ID | <rhZxo-4TD-17@gated-at.bofh.it> |
| In reply to | #1366131 |
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 >
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-03-29 15:20 +0200 |
| Message-ID | <ri1IR-6LW-1@gated-at.bofh.it> |
| In reply to | #1366156 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tim Harvey <tharvey@gateworks.com> |
|---|---|
| Date | 2016-03-29 15:40 +0200 |
| Message-ID | <ri22g-6TC-25@gated-at.bofh.it> |
| In reply to | #1366156 |
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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-03-29 16:00 +0200 |
| Message-ID | <ri2lB-72M-31@gated-at.bofh.it> |
| In reply to | #1366293 |
On Tuesday 29 March 2016 06:32:29 Tim Harvey wrote:
> >
> > 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.
That doesn't sound like a helpful solution, multi_v7_defconfig for
instance will still be broken because it enables PCI_MSI, and so
will be all major distros.
What happens if we patch the pci-imx6 driver to not make use of
its MSI support even when that is enabled in the kernel? Does that
get both devices in your GW5xxx to work with legacy interrupts or
is one or both of them still broken?
Arnd
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index eb5a2755a164..d7607b2695c6 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -470,7 +470,7 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
imx6_pcie_establish_link(pp);
- if (IS_ENABLED(CONFIG_PCI_MSI))
+ if (0 && IS_ENABLED(CONFIG_PCI_MSI))
dw_pcie_msi_init(pp);
}
@@ -490,7 +490,7 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
{
int ret;
- if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ if (0 && IS_ENABLED(CONFIG_PCI_MSI)) {
pp->msi_irq = platform_get_irq_byname(pdev, "msi");
if (pp->msi_irq <= 0) {
dev_err(&pdev->dev, "failed to get MSI irq\n");
[toc] | [prev] | [next] | [standalone]
| From | Tim Harvey <tharvey@gateworks.com> |
|---|---|
| Date | 2016-03-29 16:30 +0200 |
| Message-ID | <ri2OC-7x2-37@gated-at.bofh.it> |
| In reply to | #1366309 |
On Tue, Mar 29, 2016 at 6:52 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 29 March 2016 06:32:29 Tim Harvey wrote:
>> >
>> > 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.
>
> That doesn't sound like a helpful solution, multi_v7_defconfig for
> instance will still be broken because it enables PCI_MSI, and so
> will be all major distros.
Arnd,
True, but keep in mind that as far as I can tell this behavior has
been around since MSI was added to the IMX6 (breakage of devices that
use legacy irq's if MSI is enabled) which was in 3.16.
>
> What happens if we patch the pci-imx6 driver to not make use of
> its MSI support even when that is enabled in the kernel? Does that
> get both devices in your GW5xxx to work with legacy interrupts or
> is one or both of them still broken?
>
> Arnd
>
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index eb5a2755a164..d7607b2695c6 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -470,7 +470,7 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
>
> imx6_pcie_establish_link(pp);
>
> - if (IS_ENABLED(CONFIG_PCI_MSI))
> + if (0 && IS_ENABLED(CONFIG_PCI_MSI))
> dw_pcie_msi_init(pp);
> }
>
> @@ -490,7 +490,7 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
> {
> int ret;
>
> - if (IS_ENABLED(CONFIG_PCI_MSI)) {
> + if (0 && IS_ENABLED(CONFIG_PCI_MSI)) {
> pp->msi_irq = platform_get_irq_byname(pdev, "msi");
> if (pp->msi_irq <= 0) {
> dev_err(&pdev->dev, "failed to get MSI irq\n");
>
That is not enough - we would also need to disable a couple more in
the designware core that imx6 uses, which is also used by several
other SoC's. We should probably get some feedback from people with
those SoC's regarding MSI breaking legacy irqs.
PCI_MSI was enabled in imx_v6_v7_defconfig in 4.5 and enabled in
multi_v7_defconfig back in 3.16. PCI MSI was first introduced for the
IMX6 host controller in 3.16 as well. I verified that the same issue
exists all the way back to 3.16.
I don't know if its worse to disable PCI MSI for IMX6/designware all
the way back to 3.16 or to disable it just back to 4.5 where imx_v6_v7
enabled it, or perhaps just figure out the actual issue and get that
backported?
Lucas, have you had any thoughts or time yet to think about why
enabling MSI breaks legacy IRQs?
Tim
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-03-29 17:00 +0200 |
| Message-ID | <ri3hF-7IL-25@gated-at.bofh.it> |
| In reply to | #1366335 |
On Tuesday 29 March 2016 07:29:34 Tim Harvey wrote:
> On Tue, Mar 29, 2016 at 6:52 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 29 March 2016 06:32:29 Tim Harvey wrote:
> >> I think 31e98e0d24cd2537a63e06e235e050a06b175df7 "ARM:
> >> imx_v6_v7_defconfig: enable PCI_MSI" should be reverted as well until
> >> we figure this out.
> >
> > That doesn't sound like a helpful solution, multi_v7_defconfig for
> > instance will still be broken because it enables PCI_MSI, and so
> > will be all major distros.
>
> Arnd,
>
> True, but keep in mind that as far as I can tell this behavior has
> been around since MSI was added to the IMX6 (breakage of devices that
> use legacy irq's if MSI is enabled) which was in 3.16.
I see. This part wasn't clear to me.
> > What happens if we patch the pci-imx6 driver to not make use of
> > its MSI support even when that is enabled in the kernel? Does that
> > get both devices in your GW5xxx to work with legacy interrupts or
> > is one or both of them still broken?
> >
> > Arnd
> >
> > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> > index eb5a2755a164..d7607b2695c6 100644
> > --- a/drivers/pci/host/pci-imx6.c
> > +++ b/drivers/pci/host/pci-imx6.c
> > @@ -470,7 +470,7 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
> >
> > imx6_pcie_establish_link(pp);
> >
> > - if (IS_ENABLED(CONFIG_PCI_MSI))
> > + if (0 && IS_ENABLED(CONFIG_PCI_MSI))
> > dw_pcie_msi_init(pp);
> > }
> >
> > @@ -490,7 +490,7 @@ static int __init imx6_add_pcie_port(struct pcie_port *pp,
> > {
> > int ret;
> >
> > - if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > + if (0 && IS_ENABLED(CONFIG_PCI_MSI)) {
> > pp->msi_irq = platform_get_irq_byname(pdev, "msi");
> > if (pp->msi_irq <= 0) {
> > dev_err(&pdev->dev, "failed to get MSI irq\n");
> >
>
> That is not enough - we would also need to disable a couple more in
> the designware core that imx6 uses, which is also used by several
> other SoC's. We should probably get some feedback from people with
> those SoC's regarding MSI breaking legacy irqs.
Good point. I really just meant this as an experiment, trying to
figure out what causes it to break. I'd be surprised if the
MSI support in the generic pcie-designware driver caused the same
problem on the other SoCs.
> PCI_MSI was enabled in imx_v6_v7_defconfig in 4.5 and enabled in
> multi_v7_defconfig back in 3.16. PCI MSI was first introduced for the
> IMX6 host controller in 3.16 as well. I verified that the same issue
> exists all the way back to 3.16.
Thanks for doing that research.
> I don't know if its worse to disable PCI MSI for IMX6/designware all
> the way back to 3.16 or to disable it just back to 4.5 where imx_v6_v7
> enabled it, or perhaps just figure out the actual issue and get that
> backported?
I'd like to see the problem understood better before we talk about
backports.
One thing I just noticed is that the same GPC interrupt line (GIC_SPI
120) is used for MSI and for IntD. Maybe there is something going on with
sharing an interrupt line between a nested irqchip and a device?
Could this be a bug in the generic IRQ handling code? Can you check
which interrupt the broken device(s) on your machine are using? Is
it always the 120 (IntD) line, or are the other three lines broken
as well? I don't actually know how to look it up, but the 'lspci -t'
output should let us reconstruct the swizzling manually.
Arnd
[toc] | [prev] | [next] | [standalone]
Page 1 of 3 [1] 2 3 Next page →
Back to top | Article view | linux.kernel
csiph-web