i2c: mxc: allow executing the code that only applies to i.MX platforms

PATCH New Gong Qianyu 2 participants 5 messages Dec 14, 2015

Subsystems (2)

Arm i.MX and Layerscape I2C

lore

Share & tools

lore

Gong Qianyu

author

2015-12-14 10:23 UTC #0 lore raw

PATCH 1/1 i2c: mxc: allow executing the code that only applies to i.MX platforms

The bus_i2c_init() is called before relocation and will assgin value to a static variable. If U-Boot is then still running in a flash device, it's theoretically not allowed to write data to flash without an erasing operation. For i.MX platforms, the U-Boot is always running in DDR.

Actually it causes asynchronous error when the ARM64 system error report is enabled and the flash write protect is set.

Signed-off-by Gong Qianyu <[email protected]>

---
 drivers/i2c/mxc_i2c.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/i2c/mxc_i2c.c +3 -0

index fa4c82f..4dddb83 100644
@@ -581,8 +581,11 @@ void bus_i2c_init(int index, int speed, int unused,
 		return;
 	}
 
+#if defined(CONFIG_MX51) || defined(CONFIG_MX53) || \
+	defined(CONFIG_MX6) || defined(CONFIG_MX7)
 	mxc_i2c_buses[index].idle_bus_fn = idle_bus_fn;
 	mxc_i2c_buses[index].idle_bus_data = idle_bus_data;
+#endif
 
 	ret = enable_i2c_clk(1, index);
 	if (ret < 0) {

Signature · 1 line

2.1.0.27.g96db324

York Sun

2015-12-14 10:37 UTC #1 lore raw

PATCH 1/1 i2c: mxc: allow executing the code that only applies to i.MX platforms

Quoted 19 lines · Gong Qianyu

The bus_i2c_init() is called before relocation and will assgin value to a static variable. If U-Boot is then still running in a flash device, it's theoretically not allowed to write data to flash without an erasing operation. For i.MX platforms, the U-Boot is always running in DDR.
Actually it causes asynchronous error when the ARM64 system error report is enabled and the flash write protect is set.
Signed-off-by Gong Qianyu <[email protected]>
---
 drivers/i2c/mxc_i2c.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/i2c/mxc_i2c.c +0 -0
index fa4c82f..4dddb83 100644

drivers/i2c/mxc_i2c.c void bus_i2c_init(int index, int speed, int unused, · line 589

Quoted 9 lines

drivers/i2c/mxc_i2c.c +3 -0

@@ -581,8 +581,11 @@ void bus_i2c_init(int index, int speed, int unused,
 		return;
 	}
 
+#if defined(CONFIG_MX51) || defined(CONFIG_MX53) || \
+	defined(CONFIG_MX6) || defined(CONFIG_MX7)
 	mxc_i2c_buses[index].idle_bus_fn = idle_bus_fn;
 	mxc_i2c_buses[index].idle_bus_data = idle_bus_data;
+#endif

I also think using variable mxc_i2c_buses is problematic. But using ifdef doesn't look like a solution. I think this variable should be put into stack, or use malloc. It works with execution-in-place in read-only space.

York

Gong Q.Y.

author

2015-12-14 11:03 UTC #2 lore raw

PATCH 1/1 i2c: mxc: allow executing the code that only applies to i.MX platforms

Quoted 45 lines

-----Original Message-----
From: Sun York-R58495
Sent: Monday, December 14, 2015 6:37 PM
To: Gong Qianyu-B52263; u-boot at lists.denx.de
Cc: Hu Mingkai-B21284; Sun York-R58495; Fan Peng-B51431
Subject: Re: [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms
Quoted 19 lines · Gong Qianyu
The bus_i2c_init() is called before relocation and will assgin value to a static variable. If U-Boot is then still running in a flash device, it's theoretically not allowed to write data to flash without an erasing operation. For i.MX platforms, the U-Boot is always running in DDR.
Actually it causes asynchronous error when the ARM64 system error report is enabled and the flash write protect is set.
Signed-off-by Gong Qianyu <[email protected]>
---
 drivers/i2c/mxc_i2c.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/i2c/mxc_i2c.c index renamed from drivers/i2c/mxc_i2c.c +0 -0
fa4c82f..4dddb83 100644
drivers/i2c/mxc_i2c.c void bus_i2c_init(int index, int speed, int unused, · line 589
Quoted 9 lines
drivers/i2c/mxc_i2c.c +3 -0
@@ -581,8 +581,11 @@ void bus_i2c_init(int index, int speed, int unused,
 		return;
 	}
 
+#if defined(CONFIG_MX51) || defined(CONFIG_MX53) || \
+	defined(CONFIG_MX6) || defined(CONFIG_MX7)
 	mxc_i2c_buses[index].idle_bus_fn = idle_bus_fn;
 	mxc_i2c_buses[index].idle_bus_data = idle_bus_data;
+#endif
 
I also think using variable mxc_i2c_buses is problematic. But using ifdef doesn't look like a solution. I think this variable should be put into stack, or use malloc. It works with execution-in-place in read-only space.

Quoted 1 line

York

But we don't know if the stack will be enough before relocation. For SD boot of LS1043A,

there is now only 4KB for it and if the spl image is bigger in the future, the stack is

smaller. So isn't leaving more stack(if possible) for necessary code better?

York Sun

2015-12-14 11:14 UTC #3 lore raw

PATCH 1/1 i2c: mxc: allow executing the code that only applies to i.MX platforms

Quoted 45 lines · On 12/14/2015 07:03 PM, Gong Qianyu-B52263 wrote:

Quoted 45 lines
-----Original Message-----
From: Sun York-R58495
Sent: Monday, December 14, 2015 6:37 PM
To: Gong Qianyu-B52263; u-boot at lists.denx.de
Cc: Hu Mingkai-B21284; Sun York-R58495; Fan Peng-B51431
Subject: Re: [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms
Quoted 19 lines · Gong Qianyu
The bus_i2c_init() is called before relocation and will assgin value to a static variable. If U-Boot is then still running in a flash device, it's theoretically not allowed to write data to flash without an erasing operation. For i.MX platforms, the U-Boot is always running in DDR.
Actually it causes asynchronous error when the ARM64 system error report is enabled and the flash write protect is set.
Signed-off-by Gong Qianyu <[email protected]>
---
 drivers/i2c/mxc_i2c.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/i2c/mxc_i2c.c index renamed from drivers/i2c/mxc_i2c.c +0 -0
fa4c82f..4dddb83 100644
drivers/i2c/mxc_i2c.c void bus_i2c_init(int index, int speed, int unused, · line 589
Quoted 9 lines
drivers/i2c/mxc_i2c.c +3 -0
@@ -581,8 +581,11 @@ void bus_i2c_init(int index, int speed, int unused,
 		return;
 	}
 
+#if defined(CONFIG_MX51) || defined(CONFIG_MX53) || \
+	defined(CONFIG_MX6) || defined(CONFIG_MX7)
 	mxc_i2c_buses[index].idle_bus_fn = idle_bus_fn;
 	mxc_i2c_buses[index].idle_bus_data = idle_bus_data;
+#endif
 
I also think using variable mxc_i2c_buses is problematic. But using ifdef doesn't look like a solution. I think this variable should be put into stack, or use malloc. It works with execution-in-place in read-only space.

Quoted 7 lines

Quoted 1 line
York
But we don't know if the stack will be enough before relocation. For SD boot of LS1043A,
there is now only 4KB for it and if the spl image is bigger in the future, the stack is
smaller. So isn't leaving more stack(if possible) for necessary code better?

This array is small. The size of stack depends on the deepest one in all functions. I don't think this driver use deep stack. I remember Linux has a tool to calculate the stack. You can try it to examine the stack depth.

York

Gong Q.Y.

author

2015-12-14 13:15 UTC #4 lore raw

PATCH 1/1 i2c: mxc: allow executing the code that only applies to i.MX platforms

Quoted 52 lines

Quoted 52 lines · On 12/14/2015 07:03 PM, Gong Qianyu-B52263 wrote:
Quoted 44 lines
-----Original Message-----
From: Sun York-R58495
Sent: Monday, December 14, 2015 6:37 PM
To: Gong Qianyu-B52263; u-boot at lists.denx.de
Cc: Hu Mingkai-B21284; Sun York-R58495; Fan Peng-B51431
Subject: Re: [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms
Quoted 20 lines · Gong Qianyu
The bus_i2c_init() is called before relocation and will assgin value to a static variable. If U-Boot is then still running in a flash device, it's theoretically not allowed to write data to flash without an erasing operation. For i.MX platforms, the U-Boot is always running in DDR.
Actually it causes asynchronous error when the ARM64 system error report is enabled and the flash write protect is set.
Signed-off-by Gong Qianyu <[email protected]>
---
drivers/i2c/mxc_i2c.c | 3 +++
1 file changed, 3 insertions(+)
drivers/i2c/mxc_i2c.c index renamed from drivers/i2c/mxc_i2c.c +0 -0
fa4c82f..4dddb83 100644
drivers/i2c/mxc_i2c.c void bus_i2c_init(int index, int speed, int unused, · line 588
Quoted 8 lines
drivers/i2c/mxc_i2c.c +3 -0
@@ -581,8 +581,11 @@ void bus_i2c_init(int index, int speed, int unused,
       return;
   }
 
+#if defined(CONFIG_MX51) || defined(CONFIG_MX53) || \
+    defined(CONFIG_MX6) || defined(CONFIG_MX7)
   mxc_i2c_buses[index].idle_bus_fn = idle_bus_fn;
   mxc_i2c_buses[index].idle_bus_data = idle_bus_data;
+#endif
I also think using variable mxc_i2c_buses is problematic. But using ifdef doesn't look like a solution. I think this variable should be put into stack, or use malloc. It works with execution-in-place in read-only space.
Quoted 1 line
York
But we don't know if the stack will be enough before relocation. For SD boot of LS1043A,
there is now only 4KB for it and if the spl image is bigger in the future, the stack is

Quoted 8 lines

Quoted 1 line
smaller. So isn't leaving more stack(if possible) for necessary code better?
This array is small. The size of stack depends on the deepest one in all functions. I don't think this driver use deep stack. I remember Linux has a tool to calculate the stack. You can try it to examine the stack depth.
York

Ok. So the array won't need to be reserved in stack. I just didn't read much of the driver code and wanted to keep the original meaning of the author. I'm also confused why it has to be static. But before that it was put in gdata.

Regards,
Qianyu