[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[timur at freescale.com]

From: Timur Tabi <timur@freescale.com>

This patch adds support for the reversed geometry data in some AMD flash
chips.

I'm not proud of this patch, so I'm posting it for review only.  I know it
works on the my board that was the problem, but I have no idea if it will
break any other board.  I'm sure this code could be improved a lot.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 drivers/cfi_flash.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c
index fd0a186..8cc8d60 100644
--- a/drivers/cfi_flash.c
+++ b/drivers/cfi_flash.c
@@ -1120,6 +1120,7 @@ ulong flash_get_size (ulong base, int ba
 	uchar num_erase_regions;
 	int erase_region_size;
 	int erase_region_count;
+	int geometry_reversed = 0;
 #ifdef CFG_FLASH_PROTECTION
 	int ext_addr;
 	info->legacy_unlock = 0;
@@ -1148,6 +1149,8 @@ #endif
 		case CFI_CMDSET_AMD_STANDARD:
 		case CFI_CMDSET_AMD_EXTENDED:
 			info->cmd_reset = AMD_CMD_RESET;
+			if (flash_read_uchar(info, FLASH_OFFSET_CFI_RESP + (0x7E / info->portwidth)) == 3)
+				geometry_reversed = 1;
 			break;
 		}

@@ -1171,8 +1174,13 @@ #endif
 					num_erase_regions, NUM_ERASE_REGIONS);
 				break;
 			}
-			tmp = flash_read_long (info, 0,
+			if (geometry_reversed)
+				tmp = flash_read_long (info, 0,
 					       FLASH_OFFSET_ERASE_REGIONS +
+					       (num_erase_regions - 1 - i) * 4);
+			else
+				tmp = flash_read_long (info, 0,
+				       FLASH_OFFSET_ERASE_REGIONS +
 					       i * 4);
 			erase_region_size =
 				(tmp & 0xffff) ? ((tmp & 0xffff) * 256) : 128;
--
1.4.2.1

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Tolunay Orkun]

timur at freescale.com wrote:
> From: Timur Tabi <timur@freescale.com>
> 
> This patch adds support for the reversed geometry data in some AMD flash
> chips.
> 
> I'm not proud of this patch, so I'm posting it for review only.  I know it
> works on the my board that was the problem, but I have no idea if it will
> break any other board.  I'm sure this code could be improved a lot.

NAK. It will break any bottom boot AMD flash. I have reviewed Linux MTD 
driver and the coder had nice things to say about AMD regarding this 
matter (!).

Anyway, I will get proper patch to you to try tonight or tomorrow. I do 
not have a board with AMD flash myself.

Tolunay

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check fortop/bottom boot?

[Spence Nick-rxtd10]

> -----Original Message-----
> From: u-boot-users-bounces at lists.sourceforge.net 
> [mailto:u-boot-users-bounces at lists.sourceforge.net] On Behalf 
> Of timur at freescale.com
> Sent: Monday, November 06, 2006 4:47 PM
> To: u-boot-users at lists.sourceforge.net
> Subject: [U-Boot-Users] [PATCH] Where does U-Boot's CFI 
> driver check fortop/bottom boot?
> 
> From: Timur Tabi <timur@freescale.com>
> 
> This patch adds support for the reversed geometry data in 
> some AMD flash chips.
>
> <snip> 
>  		case CFI_CMDSET_AMD_STANDARD:
>  		case CFI_CMDSET_AMD_EXTENDED:
>  			info->cmd_reset = AMD_CMD_RESET;
> +			if (flash_read_uchar(info, 
> FLASH_OFFSET_CFI_RESP + (0x7E / info->portwidth)) == 3)

The code also needs to check:
1) that there is an extended query table
2) The query table is version 1.1 or later

Then (and only then) is it safe to use this byte from the query table.

Nick

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Tolunay Orkun]

Here is the patch (attached) that handles top boot geometry reversal case on 
AMD flash.

This version is only for testing and commenting purposes. I've basically 
ported the way Linux MTD driver handles this issue (so I think). If it seems 
to work properly, I will prepare an official patch for submission. I 
appreciate if you can test on your AMD flash command set parts using CFI driver.

The patch is against the top of the tree.

Tolunay



-------------- next part --------------
A non-text attachment was scrubbed...
Name: cfi_geo_reversed.patch
Type: text/x-patch
Size: 3246 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20061107/d0fa3eb8/attachment.bin 

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Timur Tabi]

Tolunay Orkun wrote:
> Here is the patch (attached) that handles top boot geometry reversal 
> case on AMD flash.

This patch works for me, so I approve it.  I tested it on the board that has 
this problem, as well as an 8360-based board that has only same-sized sectors.

Just one nit:

Adds trailing whitespace.
.dotest/patch:35:               man_id = flash_read_uchar (info,
Adds trailing whitespace.
.dotest/patch:37:               dev_id = flash_read_uchar(info,
warning: 2 lines add trailing whitespaces.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Tolunay Orkun]

Timur Tabi wrote:
> Tolunay Orkun wrote:
>> Here is the patch (attached) that handles top boot geometry reversal 
>> case on AMD flash.
> 
> This patch works for me, so I approve it.  I tested it on the board that has 
> this problem, as well as an 8360-based board that has only same-sized sectors.

Glad to hear. Hopefully I will get more positive feedback.

> Just one nit:
> 
> Adds trailing whitespace.
> .dotest/patch:35:               man_id = flash_read_uchar (info,
> Adds trailing whitespace.
> .dotest/patch:37:               dev_id = flash_read_uchar(info,
> warning: 2 lines add trailing whitespaces.

I will fix that.

Questions for the maintainers:

1) I have some new variables for man_id, dev_id, cfi_version introduced by 
this patch (actually man_id is currently not used so could be removed). I 
can kept these local to the function for now or I can add them to the info 
structure along with ext_addr (to easily locate the Vendor Extended Query 
Structure in flash). What do you think?

In the minimum displaying man_id, dev_id, cfi_version when executing 
"flinfo" command could be useful for diagnostic purposes. We might need to 
use them to tweak the behavior of some other functionality in the future as 
well.

2) There are some outdated comments at the beginning of the cfi_flash.c. The 
modification history is not up to date and todo list stuff is mostly 
addressed over time. Since the actual history is already is the git, I 
propose to get rid of modification history and todo list from the comments.

Best regards,
Tolunay

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Stefan Roese]

Hi Tolunay,

On Tuesday 07 November 2006 19:01, Tolunay Orkun wrote:
> Questions for the maintainers:
>
> 1) I have some new variables for man_id, dev_id, cfi_version introduced by
> this patch (actually man_id is currently not used so could be removed). I
> can kept these local to the function for now or I can add them to the info
> structure along with ext_addr (to easily locate the Vendor Extended Query
> Structure in flash). What do you think?
>
> In the minimum displaying man_id, dev_id, cfi_version when executing
> "flinfo" command could be useful for diagnostic purposes. We might need to
> use them to tweak the behavior of some other functionality in the future as
> well.

Yes, I definitely think it's useful to display this ID information (or even 
better a real manufacturer/device string) in the "flinfo" command. So my vote 
is to extend the struct.

> 2) There are some outdated comments at the beginning of the cfi_flash.c.
> The modification history is not up to date and todo list stuff is mostly
> addressed over time. Since the actual history is already is the git, I
> propose to get rid of modification history and todo list from the comments.

ACK.

Thanks for helping out here.

Best regards,
Stefan

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Tolunay Orkun]

Stefan Roese wrote:
> Hi Tolunay,
> 
> On Tuesday 07 November 2006 19:01, Tolunay Orkun wrote:
>> Questions for the maintainers:
>>
>> 1) I have some new variables for man_id, dev_id, cfi_version introduced by
>> this patch (actually man_id is currently not used so could be removed). I
>> can kept these local to the function for now or I can add them to the info
>> structure along with ext_addr (to easily locate the Vendor Extended Query
>> Structure in flash). What do you think?
>>
>> In the minimum displaying man_id, dev_id, cfi_version when executing
>> "flinfo" command could be useful for diagnostic purposes. We might need to
>> use them to tweak the behavior of some other functionality in the future as
>> well.
> 
> Yes, I definitely think it's useful to display this ID information (or even 
> better a real manufacturer/device string) in the "flinfo" command. So my vote 
> is to extend the struct.

I just noticed the info structure has a flash_id field which the comment 
says it is for combined manufacturer id (ms 16-bit) and device id (ls 16-bit).

However, for CFI driver the value stored there is FLASH_MAN_CFI. I can get 
rid of that and store the actual flash id but I will have to get rid of the 
checks in the driver that verifies the field to be FLASH_MAN_CFI.

Best regards,
Tolunay

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Tolunay Orkun]

Stefan Roese wrote:
> Hi Tolunay,
> 
> On Tuesday 07 November 2006 19:01, Tolunay Orkun wrote:
>> Questions for the maintainers:
>>
>> 1) I have some new variables for man_id, dev_id, cfi_version introduced by
>> this patch (actually man_id is currently not used so could be removed). I
>> can kept these local to the function for now or I can add them to the info
>> structure along with ext_addr (to easily locate the Vendor Extended Query
>> Structure in flash). What do you think?
>>
>> In the minimum displaying man_id, dev_id, cfi_version when executing
>> "flinfo" command could be useful for diagnostic purposes. We might need to
>> use them to tweak the behavior of some other functionality in the future as
>> well.
> 
> Yes, I definitely think it's useful to display this ID information (or even 
> better a real manufacturer/device string) in the "flinfo" command. So my vote 
> is to extend the struct.

I just noticed the info structure has a flash_id field which the comment 
says it is for combined manufacturer id (ms 16-bit) and device id (ls 16-bit).

However, for CFI driver the value stored there is FLASH_MAN_CFI. I can get 
rid of that and store the actual flash id but I will have to get rid of the 
checks in the driver that verifies the field to be FLASH_MAN_CFI.

Best regards,
Tolunay

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Spence Nick-rxtd10]

 

Tolunay Orkun wrote:
> Subject: Re: [U-Boot-Users] [PATCH] Where does U-Boot's CFI 
> driver check for top/bottom boot?
> 
> Here is the patch (attached) that handles top boot geometry 
> reversal case on AMD flash.
> 

You probably need to add a check that ext_addr is non-zero. If the
extended query table is not present then you can't read the version
number (otherwise you read the reserved section at 0h which is
undefined)

Nick

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Tolunay Orkun]

Spence Nick-rxtd10 wrote:
>  
> 
> Tolunay Orkun wrote:
>> Subject: Re: [U-Boot-Users] [PATCH] Where does U-Boot's CFI 
>> driver check for top/bottom boot?
>>
>> Here is the patch (attached) that handles top boot geometry 
>> reversal case on AMD flash.
>>
> 
> You probably need to add a check that ext_addr is non-zero. If the
> extended query table is not present then you can't read the version
> number (otherwise you read the reserved section at 0h which is
> undefined)

I thought about it and it is simple to do. That line that sets the ext_addr 
was already present for Intel case (to check legacy lock feature from 
extended query table) so I just relocated out of the case statement.

Still, I do not know of any actual CFI compliant flash that lacks an 
extended query table. Perhaps, it might have been relevant to old non-CFI 
JEDEC flash which we do not handle with this driver.

I do not want to introduce code that will not be applicable.  What do you 
think? I guess I can put the check for robustness just in case there happens 
to be an odd one in the market.

Tolunay

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Timur Tabi]

Tolunay Orkun wrote:

> I do not want to introduce code that will not be applicable.  What do you 
> think? I guess I can put the check for robustness just in case there happens 
> to be an odd one in the market.

My vote is to add the check that Nick suggests, simply to limit the 
possibility of false positives.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] Where does U-Boot's CFI driver check for top/bottom boot?

[Stefan Roese]

On Tuesday 07 November 2006 19:38, Timur Tabi wrote:
> > I do not want to introduce code that will not be applicable.  What do you
> > think? I guess I can put the check for robustness just in case there
> > happens to be an odd one in the market.
>
> My vote is to add the check that Nick suggests, simply to limit the
> possibility of false positives.

ACK.

Best regards,
Stefan