[U-Boot] [PATCH 0/1] i2c: mvtwsi: running from flash ROM

[U-Boot] [PATCH 0/1] i2c: mvtwsi: running from flash ROM

[Chris Packham]

We have a board using Marvell's MV78100 SoC (note despite the name this
is quite different to the armada MV782x0/MV784x0 SoCs).

The SoC support isn't upstream but since it's quite similar to the
Orion/Kirkwood we've managed to keep reasonably up to date. One
difference with our system is that it boots directly from NOR flash (as
opposed to many Kirkwood implementations which make use of a
pre-bootloader to setup RAM).

Somewhere between v2015.04 and v2015.10 we started seeing a hang at
startup. I eventually tracked this down to the write to
twsi_control_flags which is putting the NOR flash into status mode
leading to the hang when the next instruction is executed.

I'm not sure how we haven't hit this before. In v2015.04 the same code
path is executed but somehow no hang occurs. This code hasn't changed
much between v2015.04 and v2015.10 and certainly not in any way that
would change this behaviour. Presumably _something_ else has changed in
a way that is affecting this behaviour but so far I haven't been able to
narrow this down to a particular change.


Chris Packham (1):
  i2c: mvtwsi: avoid writing to twsi_control_flags prior to relocation

 drivers/i2c/mvtwsi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.8.2

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

[Chris Packham]

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@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 */
-- 
2.8.2

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

[Stefan Roese]

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@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? This is how it needs
to be done in DM (Driver Model) later as well.

BTW: Your platform does not support DM (yet), right?

Thanks,
Stefan

[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@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@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

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

[Chris Packham]

On Fri, May 13, 2016 at 10:35 AM, Chris Packham <judge.packham@gmail.com> wrote:
> On Thu, May 12, 2016 at 9:50 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Chris,
>>
>>
>> On 12.05.2016 04:55, Chris Packham wrote:
<snip>
>>>         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.
>

Actually I might need some pointers on this. Where could I keep such a
private data struct. About the only thing I can think of is either to
add to arch_global_data or pass a flags variable through the call
chain.

[U-Boot] [PATCH v2] i2c: mvtwsi: Eliminate twsi_control_flags

[Chris Packham]

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
worst they put the flash memory into a status mode which changes the
executable code underneath us.

Pass around a stack variable from the top of the twsi i2c driver to
avoid writing to global data.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
> On Thu, May 12, 2016 at 9:50 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Chris,
>>
>>
>> On 12.05.2016 04:55, Chris Packham wrote:
<snip>
>>>         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.
>

So I think this should work (I have tested it on my platform but if
someone else could give it ago on theirs that'd be helpful). By passing
the flags around I avoid the global data variable problems.

Changes in v2:
- Rather than just avoiding writing to twsi_control_flags pass a
  variable on the stack

 drivers/i2c/mvtwsi.c | 62 ++++++++++++++++++++++++----------------------------
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
index 221ff4f..bf44432 100644
--- a/drivers/i2c/mvtwsi.c
+++ b/drivers/i2c/mvtwsi.c
@@ -185,26 +185,17 @@ static int twsi_wait(struct i2c_adapter *adap, int expected_status)
 }
 
 /*
- * These flags are ORed to any write to the control register
- * They allow global setting of TWSIEN and ACK.
- * By default none are set.
- * twsi_start() sets TWSIEN (in case the controller was disabled)
- * twsi_recv() sets ACK or resets it depending on expected status.
- */
-static u8 twsi_control_flags = MVTWSI_CONTROL_TWSIEN;
-
-/*
  * Assert the START condition, either in a single I2C transaction
  * or inside back-to-back ones (repeated starts).
  */
-static int twsi_start(struct i2c_adapter *adap, int expected_status)
+static int twsi_start(struct i2c_adapter *adap, int expected_status, u8 *flags)
 {
 	struct mvtwsi_registers *twsi = twsi_get_base(adap);
 
 	/* globally set TWSIEN in case it was not */
-	twsi_control_flags |= MVTWSI_CONTROL_TWSIEN;
+	*flags |= MVTWSI_CONTROL_TWSIEN;
 	/* assert START */
-	writel(twsi_control_flags | MVTWSI_CONTROL_START |
+	writel(*flags | MVTWSI_CONTROL_START |
 				    MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control);
 	/* wait for controller to process START */
 	return twsi_wait(adap, expected_status);
@@ -213,14 +204,15 @@ static int twsi_start(struct i2c_adapter *adap, int expected_status)
 /*
  * Send a byte (i2c address or data).
  */
-static int twsi_send(struct i2c_adapter *adap, u8 byte, int expected_status)
+static int twsi_send(struct i2c_adapter *adap, u8 byte, int expected_status,
+		     u8 *flags)
 {
 	struct mvtwsi_registers *twsi = twsi_get_base(adap);
 
 	/* put byte in data register for sending */
 	writel(byte, &twsi->data);
 	/* clear any pending interrupt -- that'll cause sending */
-	writel(twsi_control_flags | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control);
+	writel(*flags | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control);
 	/* wait for controller to receive byte and check ACK */
 	return twsi_wait(adap, expected_status);
 }
@@ -229,18 +221,18 @@ static int twsi_send(struct i2c_adapter *adap, u8 byte, int expected_status)
  * Receive a byte.
  * Global mvtwsi_control_flags variable says if we should ack or nak.
  */
-static int twsi_recv(struct i2c_adapter *adap, u8 *byte)
+static int twsi_recv(struct i2c_adapter *adap, u8 *byte, u8 *flags)
 {
 	struct mvtwsi_registers *twsi = twsi_get_base(adap);
 	int expected_status, status;
 
 	/* compute expected status based on ACK bit in global control flags */
-	if (twsi_control_flags & MVTWSI_CONTROL_ACK)
+	if (*flags & MVTWSI_CONTROL_ACK)
 		expected_status = MVTWSI_STATUS_DATA_R_ACK;
 	else
 		expected_status = MVTWSI_STATUS_DATA_R_NAK;
 	/* acknowledge *previous state* and launch receive */
-	writel(twsi_control_flags | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control);
+	writel(*flags | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control);
 	/* wait for controller to receive byte and assert ACK or NAK */
 	status = twsi_wait(adap, expected_status);
 	/* if we did receive expected byte then store it */
@@ -296,8 +288,7 @@ static unsigned int twsi_calc_freq(const int n, const int m)
 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;
+
 	/* reset controller */
 	writel(0, &twsi->soft_reset);
 	/* wait 2 ms -- this is what the Marvell LSP does */
@@ -353,7 +344,7 @@ static void twsi_i2c_init(struct i2c_adapter *adap, int speed, int slaveadd)
  * Expected address status will derive from direction bit (bit 0) in addr.
  */
 static int i2c_begin(struct i2c_adapter *adap, int expected_start_status,
-		     u8 addr)
+		     u8 addr, u8 *flags)
 {
 	int status, expected_addr_status;
 
@@ -363,10 +354,11 @@ static int i2c_begin(struct i2c_adapter *adap, int expected_start_status,
 	else /* writing */
 		expected_addr_status = MVTWSI_STATUS_ADDR_W_ACK;
 	/* assert START */
-	status = twsi_start(adap, expected_start_status);
+	status = twsi_start(adap, expected_start_status, flags);
 	/* send out the address if the start went well */
 	if (status == 0)
-		status = twsi_send(adap, addr, expected_addr_status);
+		status = twsi_send(adap, addr, expected_addr_status,
+				   flags);
 	/* return ok or status of first failure to caller */
 	return status;
 }
@@ -378,13 +370,14 @@ static int i2c_begin(struct i2c_adapter *adap, int expected_start_status,
 static int twsi_i2c_probe(struct i2c_adapter *adap, uchar chip)
 {
 	u8 dummy_byte;
+	u8 flags = 0;
 	int status;
 
 	/* begin i2c read */
-	status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1) | 1);
+	status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1) | 1, &flags);
 	/* dummy read was accepted: receive byte but NAK it. */
 	if (status == 0)
-		status = twsi_recv(adap, &dummy_byte);
+		status = twsi_recv(adap, &dummy_byte, &flags);
 	/* Stop transaction */
 	twsi_stop(adap, 0);
 	/* return 0 or status of first failure */
@@ -405,27 +398,28 @@ static int twsi_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr,
 			int alen, uchar *data, int length)
 {
 	int status;
+	u8 flags = 0;
 
 	/* begin i2c write to send the address bytes */
-	status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1));
+	status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1), &flags);
 	/* send addr bytes */
 	while ((status == 0) && alen--)
 		status = twsi_send(adap, addr >> (8*alen),
-			MVTWSI_STATUS_DATA_W_ACK);
+			MVTWSI_STATUS_DATA_W_ACK, &flags);
 	/* begin i2c read to receive eeprom data bytes */
 	if (status == 0)
 		status = i2c_begin(adap, MVTWSI_STATUS_REPEATED_START,
-				   (chip << 1) | 1);
+				   (chip << 1) | 1, &flags);
 	/* prepare ACK if@least one byte must be received */
 	if (length > 0)
-		twsi_control_flags |= MVTWSI_CONTROL_ACK;
+		flags |= MVTWSI_CONTROL_ACK;
 	/* now receive actual bytes */
 	while ((status == 0) && length--) {
 		/* reset NAK if we if no more to read now */
 		if (length == 0)
-			twsi_control_flags &= ~MVTWSI_CONTROL_ACK;
+			flags &= ~MVTWSI_CONTROL_ACK;
 		/* read current byte */
-		status = twsi_recv(adap, data++);
+		status = twsi_recv(adap, data++, &flags);
 	}
 	/* Stop transaction */
 	status = twsi_stop(adap, status);
@@ -441,16 +435,18 @@ static int twsi_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr,
 			int alen, uchar *data, int length)
 {
 	int status;
+	u8 flags = 0;
 
 	/* begin i2c write to send the eeprom adress bytes then data bytes */
-	status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1));
+	status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1), &flags);
 	/* send addr bytes */
 	while ((status == 0) && alen--)
 		status = twsi_send(adap, addr >> (8*alen),
-			MVTWSI_STATUS_DATA_W_ACK);
+			MVTWSI_STATUS_DATA_W_ACK, &flags);
 	/* send data bytes */
 	while ((status == 0) && (length-- > 0))
-		status = twsi_send(adap, *(data++), MVTWSI_STATUS_DATA_W_ACK);
+		status = twsi_send(adap, *(data++), MVTWSI_STATUS_DATA_W_ACK,
+				   &flags);
 	/* Stop transaction */
 	status = twsi_stop(adap, status);
 	/* return 0 or status of first failure */
-- 
2.8.2

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

[Stefan Roese]

Hi Chris,

On 13.05.2016 03:20, Chris Packham wrote:
> On Fri, May 13, 2016 at 10:35 AM, Chris Packham <judge.packham@gmail.com> wrote:
>> On Thu, May 12, 2016 at 9:50 PM, Stefan Roese <sr@denx.de> wrote:
>>> Hi Chris,
>>>
>>>
>>> On 12.05.2016 04:55, Chris Packham wrote:
> <snip>
>>>>          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.
>>
>
> Actually I might need some pointers on this. Where could I keep such a
> private data struct. About the only thing I can think of is either to
> add to arch_global_data or pass a flags variable through the call
> chain.

I've checked and you are correct. Unfortunately we are missing a private 
pointer in the legacy "struct i2c_adapter". So perhaps
the flags variable is the way to go (for the non-DM I2C driver).

Thanks,
Stefan