Fix few omap gpmc regressions when booted with device tree

PATCH New Tony Lindgren 2 participants 8 messages Apr 22, 2014 – Apr 23, 2014

Subsystems (6)

Device tree bindings Arm (32-bit) Arm sub-architectures OMAP2+ support +2 more subsystems

lore

Share & tools

lore

Tony Lindgren

author maintainer

2014-04-22 00:54 UTC #0 lore raw

cover Fix few omap gpmc regressions when booted with device tree

Hi all,

Here are two fixes to GPMC issues I've seen. It seems that we have few more issues left to solve:

1. The remap of a device with gpmc_cs_remap seems to fail for a device if it's address specified in the .dts file is different from the address used by the bootloader

2. There seems to be some timing issues with smc911x where rsync of larger files and apt-get dist-upgrade can produce strange errors. This seems to work reliably when booted in legacy mode.

3. The DT mappings of GPMC devices are wrong for most devices where the ranges property should contain the GPMC partition size (16, 32, 128 or 256 MB), and the reg property for the device should only contain the device IO range. So only NOR should use large values for ranges and IO range, the rest should use the minimum 16 MB range for GPMC partition, and 0x4 - 0x20000 for the IO range. And while patching these it makes sense to also unify the GPMC ranges being used for devices.

Pekon, can you take a look at 1 and 2 above?

Then for 3 above, it seems that we cannot safely change ranges before 1 and 2 are working reliably. Or else we have patch things twice, once to fix the range sizes, then to unify the mappings for the range address...

Regards,

Tony

Tony Lindgren (2):
ARM: OMAP2+: Fix oops for GPMC free
ARM: OMAP2+: Fix GPMC remap for devices using an offset

 arch/arm/mach-omap2/gpmc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Signature · 1 line

1.8.1.1

Tony Lindgren

author maintainer

2014-04-22 00:54 UTC #1 lore raw

PATCH 1/2 ARM: OMAP2+: Fix oops for GPMC free

If gpmc_cs_remap() fails we will get an error because we are calling release_resource() on an uninitialized resource. Let's fix that by checking the resource flags. And while at it, let's also make gpmc_cs_delete_mem() use the res pointer that we already have to avoid confusion.

Without this patch we can get the following error:

omap-gpmc 6e000000.gpmc: cannot remap GPMC CS 1 to 0x01000300

Unable to handle kernel NULL pointer dereference at virtual address 00000018

...
(gpmc_cs_free+0x94/0xc8)
(gpmc_probe_generic_child+0x178/0x1ec)
(gpmc_probe_dt+0x1bc/0x2cc)
(gpmc_probe+0x250/0x44c)
(platform_drv_probe+0x3c/0x6c)
(really_probe+0x74/0x208)
(driver_probe_device+0x34/0x50)
(bus_for_each_drv+0x60/0x8c)
(device_attach+0x80/0xa4)
(bus_probe_device+0x88/0xb0)
(device_add+0x320/0x450)
(of_platform_device_create_pdata+0x80/0x9c)
(of_platform_bus_create+0xd0/0x170)
(of_platform_bus_create+0x12c/0x170)
(of_platform_populate+0x60/0x98)
(pdata_quirks_init+0x30/0x48)
(customize_machine+0x20/0x48)
(do_one_initcall+0x2c/0x14c)
(do_basic_setup+0x98/0xd8)
(kernel_init_freeable+0x12c/0x1e0)
(kernel_init+0x8/0xf0)
(ret_from_fork+0x14/0x2c)

Code: e1a04000 e59f0070 eb195136 e5942010 (e5923018)

Cc Pekon Gupta <[email protected]>

Signed-off-by tony Lindgren <[email protected]>

---
 arch/arm/mach-omap2/gpmc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/gpmc.c +5 -2

index ab43755..84e57e6 100644
@@ -501,7 +501,7 @@ static int gpmc_cs_delete_mem(int cs)
 	int r;
 
 	spin_lock(&gpmc_mem_lock);
-	r = release_resource(&gpmc_cs_mem[cs]);
+	r = release_resource(res);
 	res->start = 0;
 	res->end = 0;
 	spin_unlock(&gpmc_mem_lock);

@@ -586,6 +586,8 @@ EXPORT_SYMBOL(gpmc_cs_request);
 
 void gpmc_cs_free(int cs)
 {
+	struct resource	*res = &gpmc_cs_mem[cs];
+
 	spin_lock(&gpmc_mem_lock);
 	if (cs >= gpmc_cs_num || cs < 0 || !gpmc_cs_reserved(cs)) {
 		printk(KERN_ERR "Trying to free non-reserved GPMC CS%d\n", cs);

@@ -594,7 +596,8 @@ void gpmc_cs_free(int cs)
 		return;
 	}
 	gpmc_cs_disable_mem(cs);
-	release_resource(&gpmc_cs_mem[cs]);
+	if (res->flags)
+		release_resource(res);
 	gpmc_cs_set_reserved(cs, 0);
 	spin_unlock(&gpmc_mem_lock);
 }

Signature · 1 line

1.8.1.1

Tony Lindgren

author maintainer

2014-04-22 00:54 UTC #2 lore raw

PATCH 2/2 ARM: OMAP2+: Fix GPMC remap for devices using an offset

In reply to Tony Lindgren

At least the smc91x driver expects the device to be at 0x300 offset from bus base address. This does not work currently for GPMC when booted in device tree mode as it attempts to remap the the allocated GPMC partition to the address configured by the device tree plus the device offset.

Note that this works just fine when booted with legacy mode.

Let's fix the issue by just ignoring any device specific offset while remapping. And let's make sure the remap address confirms to the GPMC 16MB minimum granularity as listed in the TRM for GPMC_CONFIG7 BASEADDRESS bits.

Otherwise we can get something like this:

omap-gpmc 6e000000.gpmc: cannot remap GPMC CS 1 to 0x01000300

Cc Pekon Gupta <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 arch/arm/mach-omap2/gpmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

arch/arm/mach-omap2/gpmc.c +8 -0

index 84e57e6..9fe8c94 100644
@@ -527,6 +527,14 @@ static int gpmc_cs_remap(int cs, u32 base)
 		pr_err("%s: requested chip-select is disabled\n", __func__);
 		return -ENODEV;
 	}
+
+	/*
+	 * Make sure we ignore any device offsets from the GPMC partition
+	 * allocated for the chip select and that the new base confirms
+	 * to the GPMC 16MB minimum granularity.
+	 */ 
+	base &= ~(SZ_16M - 1);
+
 	gpmc_cs_get_memconf(cs, &old_base, &size);
 	if (base == old_base)
 		return 0;

Signature · 1 line

1.8.1.1

Javier Martinez Canillas

2014-04-22 06:55 UTC #3 lore raw

cover Fix few omap gpmc regressions when booted with device tree

In reply to Tony Lindgren

Hi Tony,

Quoted 5 lines · Tony Lindgren

Hi all,
Here are two fixes to GPMC issues I've seen. It seems that we have few more issues left to solve:

Quoted 9 lines

1. The remap of a device with gpmc_cs_remap seems to fail for a device if it's address specified in the .dts file is different from the address used by the bootloader
2. There seems to be some timing issues with smc911x where rsync of larger files and apt-get dist-upgrade can produce strange errors. This seems to work reliably when booted in legacy mode.

In what board are you having this issue? The smsc911x driver supports both SMSC's LAN911x and LAN921x families and I see that we have two .dtsi files with different timings (arm/boot/dts/omap-gpmc-smsc{911x,9221}.dtsi).

This is only a wild guess, but maybe your board has a smsc LAN921x chip but is including omap-gpmc-smsc911x.dtsi on its DTS?

Quoted 17 lines

3. The DT mappings of GPMC devices are wrong for most devices where the ranges property should contain the GPMC partition size (16, 32, 128 or 256 MB), and the reg property for the device should only contain the device IO range. So only NOR should use large values for ranges and IO range, the rest should use the minimum 16 MB range for GPMC partition, and 0x4 - 0x20000 for the IO range. And while patching these it makes sense to also unify the GPMC ranges being used for devices.
Pekon, can you take a look at 1 and 2 above?
Then for 3 above, it seems that we cannot safely change ranges before 1 and 2 are working reliably. Or else we have patch things twice, once to fix the range sizes, then to unify the mappings for the range address...

Quoted 8 lines

Regards,
Tony
Tony Lindgren (2):
ARM: OMAP2+: Fix oops for GPMC free
ARM: OMAP2+: Fix GPMC remap for devices using an offset

These fixes look good to me.

Reviewed-by Javier Martinez Canillas <[email protected]>

Quoted 6 lines

 arch/arm/mach-omap2/gpmc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

--
1.8.1.1

Tony Lindgren

author maintainer

2014-04-22 15:23 UTC #4 lore raw

cover Fix few omap gpmc regressions when booted with device tree

Quoted 8 lines · Javier Martinez Canillas

Quoted 6 lines · Tony Lindgren
2. There seems to be some timing issues with smc911x where rsync of larger files and apt-get dist-upgrade can produce strange errors. This seems to work reliably when booted in legacy mode.

Quoted 7 lines

In what board are you having this issue? The smsc911x driver supports both SMSC's LAN911x and LAN921x families and I see that we have two .dtsi files with different timings (arm/boot/dts/omap-gpmc-smsc{911x,9221}.dtsi).
This is only a wild guess, but maybe your board has a smsc LAN921x chip but is including omap-gpmc-smsc911x.dtsi on its DTS?

Yes it seems to have two LAN9220s, so this could be the reason. I don't think we had the omap-gpmc-smsc9221.dtsi when I added the timings initially.

This is on a sbc-t3730 that I'm using as a gateway that was behaving reliably before I upgraded it to DT based booting. It's currently at v3.13-rc3 something, but I don't think we've much GPMC changes since then.

I'll try upgrading the kernel today and running some tests with rsync. Looks like we can also remove quite a bit of duplicate timing data by using omap-gpmc-smsc9221.dtsi, I'll try something like the patch below.

In any case, I suggest others run some tests on their GPMC Ethernet too.

Regards,

Tony

8< ----------------------

arch/arm/boot/dts/omap3-cm-t3x30.dtsi +3 ~~-39~~

@@ -10,18 +10,6 @@
 			cpu0-supply = <&vcc>;
 		};
 	};
-
-	vddvario: regulator-vddvario {
-		compatible = "regulator-fixed";
-		regulator-name = "vddvario";
-		regulator-always-on;
-	};
-
-	vdd33a: regulator-vdd33a {
-		compatible = "regulator-fixed";
-		regulator-name = "vdd33a";
-		regulator-always-on;
-	};
 };
 
 &omap3_pmx_core {

@@ -51,42 +39,18 @@
 	};
 };
 
+#include "omap-gpmc-smsc9221.dtsi"
+
 &gpmc {
 	ranges = <5 0 0x2c000000 0x01000000>;
 
-	smsc1: ethernet at 5,0 {
+	smsc1: ethernet at gpmc {
 		compatible = "smsc,lan9221", "smsc,lan9115";
 		pinctrl-names = "default";
 		pinctrl-0 = <&smsc1_pins>;
 		interrupt-parent = <&gpio6>;
 		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
 		reg = <5 0 0xff>;
-		bank-width = <2>;
-		gpmc,mux-add-data;
-		gpmc,cs-on-ns = <0>;
-		gpmc,cs-rd-off-ns = <186>;
-		gpmc,cs-wr-off-ns = <186>;
-		gpmc,adv-on-ns = <12>;
-		gpmc,adv-rd-off-ns = <48>;
-		gpmc,adv-wr-off-ns = <48>;
-		gpmc,oe-on-ns = <54>;
-		gpmc,oe-off-ns = <168>;
-		gpmc,we-on-ns = <54>;
-		gpmc,we-off-ns = <168>;
-		gpmc,rd-cycle-ns = <186>;
-		gpmc,wr-cycle-ns = <186>;
-		gpmc,access-ns = <114>;
-		gpmc,page-burst-access-ns = <6>;
-		gpmc,bus-turnaround-ns = <12>;
-		gpmc,cycle2cycle-delay-ns = <18>;
-		gpmc,wr-data-mux-bus-ns = <90>;
-		gpmc,wr-access-ns = <186>;
-		gpmc,cycle2cycle-samecsen;
-		gpmc,cycle2cycle-diffcsen;
-		vddvario-supply = <&vddvario>;
-		vdd33a-supply = <&vdd33a>;
-		reg-io-width = <4>;
-		smsc,save-mac-address;
 	};
 };

arch/arm/boot/dts/omap3-sb-t35.dtsi +20 ~~-33~~

67 lines not on this page · raw message

arch/arm/boot/dts/omap3-sbc-t3517.dts +13 -0

@@ -8,6 +8,19 @@
 / {
 	model = "CompuLab SBC-T3517 with CM-T3517";
 	compatible = "compulab,omap3-sbc-t3517", "compulab,omap3-cm-t3517", "ti,am3517", "ti,omap3";
+
+	/* Only one GPMC smsc9220 on SBC-T3517, CM-T3517 uses am35x Ethernet */
+	vddvario: regulator-vddvario-sb-t35 {
+		compatible = "regulator-fixed";
+		regulator-name = "vddvario";
+		regulator-always-on;
+	};
+
+	vdd33a: regulator-vdd33a-sb-t35 {
+		compatible = "regulator-fixed";
+		regulator-name = "vdd33a";
+		regulator-always-on;
+	};
 };
 
 &omap3_pmx_core {

Tony Lindgren

author maintainer

2014-04-23 00:00 UTC #5 lore raw

cover Fix few omap gpmc regressions when booted with device tree

Quoted 34 lines · Tony Lindgren

Quoted 8 lines · Javier Martinez Canillas
Quoted 6 lines · Tony Lindgren
2. There seems to be some timing issues with smc911x where rsync of larger files and apt-get dist-upgrade can produce strange errors. This seems to work reliably when booted in legacy mode.
Quoted 7 lines
In what board are you having this issue? The smsc911x driver supports both SMSC's LAN911x and LAN921x families and I see that we have two .dtsi files with different timings (arm/boot/dts/omap-gpmc-smsc{911x,9221}.dtsi).
This is only a wild guess, but maybe your board has a smsc LAN921x chip but is including omap-gpmc-smsc911x.dtsi on its DTS?
Yes it seems to have two LAN9220s, so this could be the reason. I don't think we had the omap-gpmc-smsc9221.dtsi when I added the timings initially.
This is on a sbc-t3730 that I'm using as a gateway that was behaving reliably before I upgraded it to DT based booting. It's currently at v3.13-rc3 something, but I don't think we've much GPMC changes since then.
I'll try upgrading the kernel today and running some tests with rsync. Looks like we can also remove quite a bit of duplicate timing data by using omap-gpmc-smsc9221.dtsi, I'll try something like the patch below.
In any case, I suggest others run some tests on their GPMC Ethernet too.

arch/arm/boot/dts/omap3-cm-t3x30.dtsi line 43

Quoted 2 lines

arch/arm/boot/dts/omap3-cm-t3x30.dtsi +2 -0
+#include "omap-gpmc-smsc9221.dtsi"
+

The 9221 timings won't work at all on 9220, it requires a 9221. I'll post a better clean-up patch to use the 911x timings.

Upgraded the kernel and the occasional corruption is still there. I guess I need to test also the same kernel in legacy mode to try to narrow it down.

Regards,

Tony

Tony Lindgren

author maintainer

2014-04-23 18:08 UTC #6 lore raw

cover Fix few omap gpmc regressions when booted with device tree

Quoted 41 lines · Tony Lindgren

Quoted 34 lines · Tony Lindgren
Quoted 8 lines · Javier Martinez Canillas
Quoted 6 lines · Tony Lindgren
2. There seems to be some timing issues with smc911x where rsync of larger files and apt-get dist-upgrade can produce strange errors. This seems to work reliably when booted in legacy mode.
Quoted 7 lines
In what board are you having this issue? The smsc911x driver supports both SMSC's LAN911x and LAN921x families and I see that we have two .dtsi files with different timings (arm/boot/dts/omap-gpmc-smsc{911x,9221}.dtsi).
This is only a wild guess, but maybe your board has a smsc LAN921x chip but is including omap-gpmc-smsc911x.dtsi on its DTS?
Yes it seems to have two LAN9220s, so this could be the reason. I don't think we had the omap-gpmc-smsc9221.dtsi when I added the timings initially.
This is on a sbc-t3730 that I'm using as a gateway that was behaving reliably before I upgraded it to DT based booting. It's currently at v3.13-rc3 something, but I don't think we've much GPMC changes since then.
I'll try upgrading the kernel today and running some tests with rsync. Looks like we can also remove quite a bit of duplicate timing data by using omap-gpmc-smsc9221.dtsi, I'll try something like the patch below.
In any case, I suggest others run some tests on their GPMC Ethernet too.
arch/arm/boot/dts/omap3-cm-t3x30.dtsi line 43
Quoted 2 lines
arch/arm/boot/dts/omap3-cm-t3x30.dtsi +2 -0
+#include "omap-gpmc-smsc9221.dtsi"
+
The 9221 timings won't work at all on 9220, it requires a 9221. I'll post a better clean-up patch to use the 911x timings.

Quoted 3 lines

Upgraded the kernel and the occasional corruption is still there. I guess I need to test also the same kernel in legacy mode to try to narrow it down.

OK hopefully got it figured out now. For legacy booting we were always using just the bootloader timings. With device tree, we started using the timings with your commit d72b4415 (ARM: dts:
omap3-igep0020: Add SMSC911x LAN chip support) that was probably actually tested on a LAN9221 instead of LAN9220?

In any case, I've patched omap-gpmc-smsc911x.dtsi so the values match the u-boot values, and so far that seems to be working just fine. Will post few patches shortly.

Regards,

Tony

Javier Martinez Canillas

2014-04-23 18:42 UTC #7 lore raw

cover Fix few omap gpmc regressions when booted with device tree

Hello Tony,

Quoted 46 lines · Tony Lindgren

Quoted 41 lines · Tony Lindgren
Quoted 34 lines · Tony Lindgren
Quoted 8 lines · Javier Martinez Canillas
Quoted 6 lines · Tony Lindgren
2. There seems to be some timing issues with smc911x where rsync of larger files and apt-get dist-upgrade can produce strange errors. This seems to work reliably when booted in legacy mode.
Quoted 7 lines
In what board are you having this issue? The smsc911x driver supports both SMSC's LAN911x and LAN921x families and I see that we have two .dtsi files with different timings (arm/boot/dts/omap-gpmc-smsc{911x,9221}.dtsi).
This is only a wild guess, but maybe your board has a smsc LAN921x chip but is including omap-gpmc-smsc911x.dtsi on its DTS?
Yes it seems to have two LAN9220s, so this could be the reason. I don't think we had the omap-gpmc-smsc9221.dtsi when I added the timings initially.
This is on a sbc-t3730 that I'm using as a gateway that was behaving reliably before I upgraded it to DT based booting. It's currently at v3.13-rc3 something, but I don't think we've much GPMC changes since then.
I'll try upgrading the kernel today and running some tests with rsync. Looks like we can also remove quite a bit of duplicate timing data by using omap-gpmc-smsc9221.dtsi, I'll try something like the patch below.
In any case, I suggest others run some tests on their GPMC Ethernet too.
arch/arm/boot/dts/omap3-cm-t3x30.dtsi line 43
Quoted 2 lines
arch/arm/boot/dts/omap3-cm-t3x30.dtsi +2 -0
+#include "omap-gpmc-smsc9221.dtsi"
+
The 9221 timings won't work at all on 9220, it requires a 9221. I'll post a better clean-up patch to use the 911x timings.
Quoted 3 lines
Upgraded the kernel and the occasional corruption is still there. I guess I need to test also the same kernel in legacy mode to try to narrow it down.

Quoted 6 lines

OK hopefully got it figured out now. For legacy booting we were always using just the bootloader timings. With device tree, we started using the timings with your commit d72b4415 (ARM: dts:
omap3-igep0020: Add SMSC911x LAN chip support) that was probably actually tested on a LAN9221 instead of LAN9220?

Right, since the same driver (drivers/net/ethernet/smsc/smsc911x.c) is used for both SMSC families I assumed that the same timings could be used by both chips.

So I took the timings from IGEP board support in u-boot but then you did the refactoring and later Florian added another .dtsi for SMSC 9221 in commit aac9aa3 ("ARM: dts: omap: Add common file for SMSC9221").

I didn't notice about this new .dtsi file until you reported your issue and the IGEPv2 does indeed have a SMSC LAN9221i ethernet chip so it is wrongly including omap-gpmc-smsc911x.dtsi and should include omap-gpmc-smsc9221.dtsi instead.

Quoted 4 lines

In any case, I've patched omap-gpmc-smsc911x.dtsi so the values match the u-boot values, and so far that seems to be working just fine. Will post few patches shortly.

Great, I'll patch the IGEPv2 DTS file too to use omap-gpmc-smsc9221.dtsi, do some testing and post a patch.

Quoted 4 lines

Regards,
Tony

Thanks a lot and best regards,
Javier