[U-Boot] [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms

[U-Boot] [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms

[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 <Qianyu.Gong@freescale.com>
---
 drivers/i2c/mxc_i2c.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index fa4c82f..4dddb83 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -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) {
-- 
2.1.0.27.g96db324

[U-Boot] [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms

[York Sun]

On 12/14/2015 06:23 PM, Gong Qianyu wrote:
> 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 <Qianyu.Gong@freescale.com>
> ---
>  drivers/i2c/mxc_i2c.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index fa4c82f..4dddb83 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
> @@ -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

[U-Boot] [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms

[Gong Q.Y.]

> -----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
> 
> 
> 
> On 12/14/2015 06:23 PM, Gong Qianyu wrote:
> > 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 <Qianyu.Gong@freescale.com>
> > ---
> >  drivers/i2c/mxc_i2c.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index
> > fa4c82f..4dddb83 100644
> > --- a/drivers/i2c/mxc_i2c.c
> > +++ b/drivers/i2c/mxc_i2c.c
> > @@ -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

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? 

[U-Boot] [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms

[York Sun]

On 12/14/2015 07:03 PM, Gong Qianyu-B52263 wrote:
> 
>> -----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
>>
>>
>>
>> On 12/14/2015 06:23 PM, Gong Qianyu wrote:
>>> 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 <Qianyu.Gong@freescale.com>
>>> ---
>>>  drivers/i2c/mxc_i2c.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index
>>> fa4c82f..4dddb83 100644
>>> --- a/drivers/i2c/mxc_i2c.c
>>> +++ b/drivers/i2c/mxc_i2c.c
>>> @@ -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
> 
> 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

[U-Boot] [PATCH] i2c: mxc: allow executing the code that only applies to i.MX platforms

[Gong Q.Y.]

>> On 12/14/2015 07:03 PM, Gong Qianyu-B52263 wrote:
>> 
>>> -----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
>>> 
>>> 
>>> 
>>>> On 12/14/2015 06:23 PM, Gong Qianyu wrote:
>>>> 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 <Qianyu.Gong@freescale.com>
>>>> ---
>>>> drivers/i2c/mxc_i2c.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>> 
>>>> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index
>>>> fa4c82f..4dddb83 100644
>>>> --- a/drivers/i2c/mxc_i2c.c
>>>> +++ b/drivers/i2c/mxc_i2c.c
>>>> @@ -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
>> 
>> 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
> 
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