All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation
@ 2017-09-13  9:39 Lukasz Majewski
  2017-09-13 16:02 ` Fabio Estevam
  2017-09-25 10:09 ` Jagan Teki
  0 siblings, 2 replies; 6+ messages in thread
From: Lukasz Majewski @ 2017-09-13  9:39 UTC (permalink / raw)
  To: u-boot

The content of Bank Address Register (BAR) is volatile. It is cleared
after power cycle or reset command (RESET F0h).

Some memories (like e.g. s25fl256s) use it to access memory larger than
0x1000000 (16 MiB).

The problem shows up when one:

1. Reads/writes/erases memory > 16 MiB
2. Calls "reset" u-boot command (which is not causing BAR to be cleared)

In the above scenario, the SoC ROM sends 0x000000 address to read SPL.
Unfortunately, the BA24 bit is still set and hence it receives content
from 0x1000000 memory address.
As a result the SoC aborts and we hang. Only power cycle can take the
SoC out of this state.

How to reproduce/test:

sf probe; sf erase 0x1200000 0x800000; reset
sf probe; sf erase 0x1200000 0x800000; sf write 0x11000000 0x1200000 0x800000; reset
sf probe; sf read 0x11000000 0x1200000 0x800000; reset

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/mtd/spi/spi_flash.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 34f6888..d19d64a 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -113,6 +113,27 @@ static int write_cr(struct spi_flash *flash, u8 wc)
 #endif
 
 #ifdef CONFIG_SPI_FLASH_BAR
+/*
+ * This "cleanup" is necessary in a situation when one was accessing
+ * spi flash memory > 16 MiB by using Bank Address Register's BA24 bit.
+ *
+ * After it the BA24 bit shall be cleared to allow access to correct
+ * memory region after SW reset (by calling "reset" command).
+ *
+ * Otherwise, the BA24 bit may be left set and then after reset, the
+ * ROM would seek for SPL from 0x1000000, not 0x0.
+ */
+static int cleanup_bar(struct spi_flash *flash)
+{
+	u8 cmd, bank_sel = 0;
+
+	if (flash->bank_curr == 0)
+		return 0;
+	cmd = flash->bank_write_cmd;
+
+	return spi_flash_write_common(flash, &cmd, 1, &bank_sel, 1);
+}
+
 static int write_bar(struct spi_flash *flash, u32 offset)
 {
 	u8 cmd, bank_sel;
@@ -339,6 +360,10 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
 		len -= erase_size;
 	}
 
+#ifdef CONFIG_SPI_FLASH_BAR
+	ret = cleanup_bar(flash);
+#endif
+
 	return ret;
 }
 
@@ -397,6 +422,10 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash, u32 offset,
 		offset += chunk_len;
 	}
 
+#ifdef CONFIG_SPI_FLASH_BAR
+	ret = cleanup_bar(flash);
+#endif
+
 	return ret;
 }
 
@@ -500,6 +529,10 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash, u32 offset,
 		data += read_len;
 	}
 
+#ifdef CONFIG_SPI_FLASH_BAR
+	ret = cleanup_bar(flash);
+#endif
+
 	free(cmd);
 	return ret;
 }
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation
  2017-09-13  9:39 [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation Lukasz Majewski
@ 2017-09-13 16:02 ` Fabio Estevam
  2017-09-13 16:06   ` Łukasz Majewski
  2017-09-23 22:40   ` Łukasz Majewski
  2017-09-25 10:09 ` Jagan Teki
  1 sibling, 2 replies; 6+ messages in thread
From: Fabio Estevam @ 2017-09-13 16:02 UTC (permalink / raw)
  To: u-boot

Hi Lukasz,

On Wed, Sep 13, 2017 at 6:39 AM, Lukasz Majewski <lukma@denx.de> wrote:

>  #ifdef CONFIG_SPI_FLASH_BAR
> +/*
> + * This "cleanup" is necessary in a situation when one was accessing
> + * spi flash memory > 16 MiB by using Bank Address Register's BA24 bit.
> + *
> + * After it the BA24 bit shall be cleared to allow access to correct
> + * memory region after SW reset (by calling "reset" command).
> + *
> + * Otherwise, the BA24 bit may be left set and then after reset, the
> + * ROM would seek for SPL from 0x1000000, not 0x0.
> + */
> +static int cleanup_bar(struct spi_flash *flash)
> +{
> +       u8 cmd, bank_sel = 0;
> +
> +       if (flash->bank_curr == 0)
> +               return 0;
> +       cmd = flash->bank_write_cmd;
> +
> +       return spi_flash_write_common(flash, &cmd, 1, &bank_sel, 1);
> +}
> +

What about defining an empty stub for this function when
CONFIG_SPI_FLASH_BAR is not defined?

>  static int write_bar(struct spi_flash *flash, u32 offset)
>  {
>         u8 cmd, bank_sel;
> @@ -339,6 +360,10 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
>                 len -= erase_size;
>         }
>
> +#ifdef CONFIG_SPI_FLASH_BAR
> +       ret = cleanup_bar(flash);
> +#endif

Then you don't need to add the ifdefs when calling cleanup_bar().

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation
  2017-09-13 16:02 ` Fabio Estevam
@ 2017-09-13 16:06   ` Łukasz Majewski
  2017-09-23 22:40   ` Łukasz Majewski
  1 sibling, 0 replies; 6+ messages in thread
From: Łukasz Majewski @ 2017-09-13 16:06 UTC (permalink / raw)
  To: u-boot

Hi Fabio,

> Hi Lukasz,
> 
> On Wed, Sep 13, 2017 at 6:39 AM, Lukasz Majewski <lukma@denx.de> wrote:
> 
>>   #ifdef CONFIG_SPI_FLASH_BAR
>> +/*
>> + * This "cleanup" is necessary in a situation when one was accessing
>> + * spi flash memory > 16 MiB by using Bank Address Register's BA24 bit.
>> + *
>> + * After it the BA24 bit shall be cleared to allow access to correct
>> + * memory region after SW reset (by calling "reset" command).
>> + *
>> + * Otherwise, the BA24 bit may be left set and then after reset, the
>> + * ROM would seek for SPL from 0x1000000, not 0x0.
>> + */
>> +static int cleanup_bar(struct spi_flash *flash)
>> +{
>> +       u8 cmd, bank_sel = 0;
>> +
>> +       if (flash->bank_curr == 0)
>> +               return 0;
>> +       cmd = flash->bank_write_cmd;
>> +
>> +       return spi_flash_write_common(flash, &cmd, 1, &bank_sel, 1);
>> +}
>> +
> 
> What about defining an empty stub for this function when
> CONFIG_SPI_FLASH_BAR is not defined?
> 
>>   static int write_bar(struct spi_flash *flash, u32 offset)
>>   {
>>          u8 cmd, bank_sel;
>> @@ -339,6 +360,10 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
>>                  len -= erase_size;
>>          }
>>
>> +#ifdef CONFIG_SPI_FLASH_BAR
>> +       ret = cleanup_bar(flash);
>> +#endif
> 
> Then you don't need to add the ifdefs when calling cleanup_bar().

I took the approach already used in this file, so I would prefer to 
leave it is in this patch (to be in sync with the rest).

> 


-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation
  2017-09-13 16:02 ` Fabio Estevam
  2017-09-13 16:06   ` Łukasz Majewski
@ 2017-09-23 22:40   ` Łukasz Majewski
  1 sibling, 0 replies; 6+ messages in thread
From: Łukasz Majewski @ 2017-09-23 22:40 UTC (permalink / raw)
  To: u-boot

Hi Fabio, Jagan,

> Hi Lukasz,
> 
> On Wed, Sep 13, 2017 at 6:39 AM, Lukasz Majewski <lukma@denx.de> wrote:
> 
>>   #ifdef CONFIG_SPI_FLASH_BAR
>> +/*
>> + * This "cleanup" is necessary in a situation when one was accessing
>> + * spi flash memory > 16 MiB by using Bank Address Register's BA24 bit.
>> + *
>> + * After it the BA24 bit shall be cleared to allow access to correct
>> + * memory region after SW reset (by calling "reset" command).
>> + *
>> + * Otherwise, the BA24 bit may be left set and then after reset, the
>> + * ROM would seek for SPL from 0x1000000, not 0x0.
>> + */
>> +static int cleanup_bar(struct spi_flash *flash)
>> +{
>> +       u8 cmd, bank_sel = 0;
>> +
>> +       if (flash->bank_curr == 0)
>> +               return 0;
>> +       cmd = flash->bank_write_cmd;
>> +
>> +       return spi_flash_write_common(flash, &cmd, 1, &bank_sel, 1);
>> +}
>> +
> 
> What about defining an empty stub for this function when
> CONFIG_SPI_FLASH_BAR is not defined?
> 
>>   static int write_bar(struct spi_flash *flash, u32 offset)
>>   {
>>          u8 cmd, bank_sel;
>> @@ -339,6 +360,10 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
>>                  len -= erase_size;
>>          }
>>
>> +#ifdef CONFIG_SPI_FLASH_BAR
>> +       ret = cleanup_bar(flash);
>> +#endif
> 
> Then you don't need to add the ifdefs when calling cleanup_bar().
> 

Jagan, could you look into this patch?

I would prefer to keep the coding style similar to the one already 
present in this file.


-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation
  2017-09-13  9:39 [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation Lukasz Majewski
  2017-09-13 16:02 ` Fabio Estevam
@ 2017-09-25 10:09 ` Jagan Teki
  2017-09-25 10:23   ` Łukasz Majewski
  1 sibling, 1 reply; 6+ messages in thread
From: Jagan Teki @ 2017-09-25 10:09 UTC (permalink / raw)
  To: u-boot

On Wed, Sep 13, 2017 at 3:09 PM, Lukasz Majewski <lukma@denx.de> wrote:
> The content of Bank Address Register (BAR) is volatile. It is cleared
> after power cycle or reset command (RESET F0h).
>
> Some memories (like e.g. s25fl256s) use it to access memory larger than
> 0x1000000 (16 MiB).
>
> The problem shows up when one:
>
> 1. Reads/writes/erases memory > 16 MiB
> 2. Calls "reset" u-boot command (which is not causing BAR to be cleared)
>
> In the above scenario, the SoC ROM sends 0x000000 address to read SPL.
> Unfortunately, the BA24 bit is still set and hence it receives content
> from 0x1000000 memory address.
> As a result the SoC aborts and we hang. Only power cycle can take the
> SoC out of this state.
>
> How to reproduce/test:
>
> sf probe; sf erase 0x1200000 0x800000; reset
> sf probe; sf erase 0x1200000 0x800000; sf write 0x11000000 0x1200000 0x800000; reset
> sf probe; sf read 0x11000000 0x1200000 0x800000; reset
>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  drivers/mtd/spi/spi_flash.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index 34f6888..d19d64a 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -113,6 +113,27 @@ static int write_cr(struct spi_flash *flash, u8 wc)
>  #endif
>
>  #ifdef CONFIG_SPI_FLASH_BAR
> +/*
> + * This "cleanup" is necessary in a situation when one was accessing
> + * spi flash memory > 16 MiB by using Bank Address Register's BA24 bit.
> + *
> + * After it the BA24 bit shall be cleared to allow access to correct
> + * memory region after SW reset (by calling "reset" command).
> + *
> + * Otherwise, the BA24 bit may be left set and then after reset, the
> + * ROM would seek for SPL from 0x1000000, not 0x0.

This need to change, SPL will look 16 MiB * bank_sel

> + */
> +static int cleanup_bar(struct spi_flash *flash)

what about clear_bar?

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation
  2017-09-25 10:09 ` Jagan Teki
@ 2017-09-25 10:23   ` Łukasz Majewski
  0 siblings, 0 replies; 6+ messages in thread
From: Łukasz Majewski @ 2017-09-25 10:23 UTC (permalink / raw)
  To: u-boot

Hi Jagan,

> On Wed, Sep 13, 2017 at 3:09 PM, Lukasz Majewski <lukma@denx.de> wrote:
>> The content of Bank Address Register (BAR) is volatile. It is cleared
>> after power cycle or reset command (RESET F0h).
>>
>> Some memories (like e.g. s25fl256s) use it to access memory larger than
>> 0x1000000 (16 MiB).
>>
>> The problem shows up when one:
>>
>> 1. Reads/writes/erases memory > 16 MiB
>> 2. Calls "reset" u-boot command (which is not causing BAR to be cleared)
>>
>> In the above scenario, the SoC ROM sends 0x000000 address to read SPL.
>> Unfortunately, the BA24 bit is still set and hence it receives content
>> from 0x1000000 memory address.
>> As a result the SoC aborts and we hang. Only power cycle can take the
>> SoC out of this state.
>>
>> How to reproduce/test:
>>
>> sf probe; sf erase 0x1200000 0x800000; reset
>> sf probe; sf erase 0x1200000 0x800000; sf write 0x11000000 0x1200000 0x800000; reset
>> sf probe; sf read 0x11000000 0x1200000 0x800000; reset
>>
>> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>> ---
>>   drivers/mtd/spi/spi_flash.c | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>> index 34f6888..d19d64a 100644
>> --- a/drivers/mtd/spi/spi_flash.c
>> +++ b/drivers/mtd/spi/spi_flash.c
>> @@ -113,6 +113,27 @@ static int write_cr(struct spi_flash *flash, u8 wc)
>>   #endif
>>
>>   #ifdef CONFIG_SPI_FLASH_BAR
>> +/*
>> + * This "cleanup" is necessary in a situation when one was accessing
>> + * spi flash memory > 16 MiB by using Bank Address Register's BA24 bit.
>> + *
>> + * After it the BA24 bit shall be cleared to allow access to correct
>> + * memory region after SW reset (by calling "reset" command).
>> + *
>> + * Otherwise, the BA24 bit may be left set and then after reset, the
>> + * ROM would seek for SPL from 0x1000000, not 0x0.
> 
> This need to change, SPL will look 16 MiB * bank_sel

Ok. I will update the description.

> 
>> + */
>> +static int cleanup_bar(struct spi_flash *flash)
> 
> what about clear_bar?

Ok. I will change the name.

> 
> thanks!
> 


-- 
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-09-25 10:23 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-13  9:39 [U-Boot] [PATCH] sf: bar: Clean BA24 Bank Address Register bit after read/write/erase operation Lukasz Majewski
2017-09-13 16:02 ` Fabio Estevam
2017-09-13 16:06   ` Łukasz Majewski
2017-09-23 22:40   ` Łukasz Majewski
2017-09-25 10:09 ` Jagan Teki
2017-09-25 10:23   ` Łukasz Majewski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.