[U-Boot] [PATCH] i2c: mvtwsi: avoid writing to twsi_control_flags prior to relocation

[Chris Packham]

On Thu, May 12, 2016 at 9:50 PM, Stefan Roese <sr at denx.de> wrote:
> Hi Chris,
>
>
> On 12.05.2016 04:55, Chris Packham wrote:
>>
>> In a system where the initial u-boot location is genuinely NOR flash (as
>> opposed to RAM or a cache-line setup by a pre-bootloader) writes to the
>> data section are problematic. At best these writes have no effect at
>> worse they put the flash memory into a status mode which changes the
>> executable code underneath us.
>>
>> Only write to twsi_control_flags once we know we've relocated to RAM.
>>
>> Signed-off-by: Chris Packham <judge.packham at gmail.com>
>> ---
>>
>>   drivers/i2c/mvtwsi.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
>> index 221ff4f..aee28c4 100644
>> --- a/drivers/i2c/mvtwsi.c
>> +++ b/drivers/i2c/mvtwsi.c
>> @@ -28,6 +28,8 @@
>>   #error Driver mvtwsi not supported by SoC or board
>>   #endif
>>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>>   /*
>>    * TWSI register structure
>>    */
>> @@ -297,7 +299,8 @@ static void twsi_reset(struct i2c_adapter *adap)
>>   {
>>         struct mvtwsi_registers *twsi = twsi_get_base(adap);
>>         /* ensure controller will be enabled by any twsi*() function */
>> -       twsi_control_flags = MVTWSI_CONTROL_TWSIEN;
>> +       if (gd->flags & GD_FLG_RELOC)
>> +               twsi_control_flags = MVTWSI_CONTROL_TWSIEN;
>>         /* reset controller */
>>         writel(0, &twsi->soft_reset);
>>         /* wait 2 ms -- this is what the Marvell LSP does */
>
>
> I've stumbled over this global data variable also before and would
> very much like to get rid of it. Can't you move this variable into
> a (newly created) private data struct instead?

I'll take a look. The other deficiency with my solution is that
although it avoids the hang the driver still won't work because the
state that is reflected in twsi_control_flags will either cause a new
hang or not be updated.

> This is how it needs
> to be done in DM (Driver Model) later as well.
>
> BTW: Your platform does not support DM (yet), right?

Correct. I'm not against switching to it but given the fact that the
SoC support is also out of tree it may be a large change to make.

>
> Thanks,
> Stefan