Patchwork New flash chip, AT49F010

login
register
about
Submitter Andrew Morgan
Date 2011-08-31 00:52:42
Message ID <4E5D85DA.3070308@ziltro.com>
Download mbox | patch
Permalink /patch/3396/
State Accepted
Commit r1572
Headers show

Comments

Andrew Morgan - 2011-08-31 00:52:42
On 30/08/11 23:25, Stefan Tauner wrote:

> review of the AT49F010 patch follows:
>> Index: flashchips.c
>> ===================================================================
>> --- flashchips.c	(revision 1422)
>> +++ flashchips.c	(working copy)
>> @@ -2218,6 +2218,30 @@
>>
>>   	{
>>   		.vendor		= "Atmel",
>> +		.name		= "AT49F010",
> 		.name		= "AT49(H)F010",
> The H version is just a faster version (but with same VCC).
>
Are you suggesting I change the name to include (H)? If so does AT49F020 
have an H version too and should that be changed? If it wants to be 
changed what should flashchips.h have for the name?

>> +		.bustype	= BUS_PARALLEL,
>> +		.manufacture_id	= ATMEL_ID,
>> +		.model_id	= ATMEL_AT49F010,
>> +		.total_size	= 128,
>> +		.page_size	= 128,
> should probably be 256 for now (semantics are different for each chip
> driver for parallel chips afaik and it does not matter for jedec
> routines iirc. NB: page_size is in bytes, so syncing it with total_size
> does not make sense.)...
>
Ok, changed. I didn't know where to find information on what to set 
page_size, feature_bits or probe_timing to.


> this chip and also the 2 Mb and 4 Mb versions support a boot block
> protection that can be detected by software. it would be nice to add
> a .printlock function to do this and inform the user. i have seen the
> scheme before. Maybe there is already code in flashrom... but it is not
> that important.
I've looked at the patch mentioned in your later messages, and modified 
w39.c and chipdrivers.h, and added a .printlock = line to the AT49F010. 
The code compiled and ran and it told me the boot block lock was not 
active. This was just a test so I have left this out of the attached 
patch. I didn't do any tests to see if the printlock code affected 
read/write at all.

> Adding the 4 Mb version OTOH is trivial (ID 0x13) and would be
> appreciated. Nevertheless after addressing the in-line comments this
> patch is:
> Acked-by: Stefan Tauner<stefan.tauner@student.tuwien.ac.at>
>
> do you have commit rights?

I do not. Updated patch attached. I hope I got all the changes. :)

If I get a chance and can find datasheets I'll have a go at adding the 
larger AT49F chips.
Stefan Tauner - 2011-08-31 09:50:51
On Wed, 31 Aug 2011 01:52:42 +0100
Andrew Morgan <ziltro@ziltro.com> wrote:

> On 30/08/11 23:25, Stefan Tauner wrote:
> 
> > review of the AT49F010 patch follows:
> >> Index: flashchips.c
> >> ===================================================================
> >> --- flashchips.c	(revision 1422)
> >> +++ flashchips.c	(working copy)
> >> @@ -2218,6 +2218,30 @@
> >>
> >>   	{
> >>   		.vendor		= "Atmel",
> >> +		.name		= "AT49F010",
> > 		.name		= "AT49(H)F010",
> > The H version is just a faster version (but with same VCC).
> >
> Are you suggesting I change the name to include (H)?

yes. this is the way we designate chip variations in one entry.
(x) means an optional part x in the name.
x/y means alternative x or y.

look at the output of flashrom -L for some examples, a good one is
AT49F002(N) and AT49F002(N)T. The N variants have a pin not connected
and can be combined. but the T versions need their own entry due to the
erase block layout.

> If so does AT49F020 
> have an H version too and should that be changed?

i have not found one/a datasheet.... neither (for) a HF040 and HF080, so
no.

> If it wants to be 
> changed what should flashchips.h have for the name?

this is handled by the comment. we usually take the name that matches
the other names of a series. in this case without the H because the 010
is the exception.

> 
> >> +		.bustype	= BUS_PARALLEL,
> >> +		.manufacture_id	= ATMEL_ID,
> >> +		.model_id	= ATMEL_AT49F010,
> >> +		.total_size	= 128,
> >> +		.page_size	= 128,
> > should probably be 256 for now (semantics are different for each chip
> > driver for parallel chips afaik and it does not matter for jedec
> > routines iirc. NB: page_size is in bytes, so syncing it with total_size
> > does not make sense.)...
> >
> Ok, changed. I didn't know where to find information on what to set 
> page_size, feature_bits or probe_timing to.

jup that's all a bit of a mystery... in the feature bits various chip
properties are saved. these are retrieved by programmer and chip
drivers to change the behavior of generic functions in them e.g.
getaddrmask and its users.
page_size will be removed soon(tm).
probe_timing... i am not sure myself. from what i have read in the
code, some chips need some time to "startup" and be ready for more
commands after probing.
if in doubt leave them alone. the defaults are usually sane. in the
at49f case one could add FEATURE_ADDR_FULL to the feature bits, but
that's the default anyway (all 0s in the respective bits).

> > this chip and also the 2 Mb and 4 Mb versions support a boot block
> > protection that can be detected by software. it would be nice to add
> > a .printlock function to do this and inform the user. i have seen the
> > scheme before. Maybe there is already code in flashrom... but it is not
> > that important.
> I've looked at the patch mentioned in your later messages, and modified 
> w39.c and chipdrivers.h, and added a .printlock = line to the AT49F010. 
> The code compiled and ran and it told me the boot block lock was not 
> active. This was just a test so I have left this out of the attached 
> patch. I didn't do any tests to see if the printlock code affected 
> read/write at all.

of course it would be more interesting to see if it would detect if the
lock is engaged, but the nature of the lock (afaics it is permanent?
not that clear in the datasheets imho), it's not a good idea to tamper
with it :)
the printlock is just executed once when the chip is found in the
probing method hence it should have almost no effect. i think it is
safe and good to include it now. the only question is, if the location
and name of the function is right (it is not.) and how to change it.

> > Adding the 4 Mb version OTOH is trivial (ID 0x13) and would be
> > appreciated. Nevertheless after addressing the in-line comments this
> > patch is:
> > Acked-by: Stefan Tauner<stefan.tauner@student.tuwien.ac.at>
> >
> > do you have commit rights?
> 
> I do not. Updated patch attached. I hope I got all the changes. :)

apart from the (H) i explained above, i think it was only the 128 page
size and the .h comment, which are in, thanks.

> If I get a chance and can find datasheets I'll have a go at adding the 
> larger AT49F chips.
http://www.alldatasheet.com/datasheet-pdf/pdf/56185/ATMEL/AT49F080.html

if you combine the patches please move the entries like i have done in
the 040 patch, add yours in the right order and attach all
signed-off-bys involved. if you don't want to do it, just say so and i
will do it when i have time. thanks for your effort!
Stefan Tauner - 2012-08-09 20:52:09
On Wed, 31 Aug 2011 11:50:51 +0200
Stefan Tauner <stefan.tauner@student.tuwien.ac.at> wrote:

> if you don't want to do it, just say so and i
> will do it when i have time. thanks for your effort!

hi andrew!

may i presume that you wont refine the patch? :)
Stefan Tauner - 2012-08-13 23:45:03
On Thu, 9 Aug 2012 22:52:09 +0200
Stefan Tauner <stefan.tauner@student.tuwien.ac.at> wrote:

> On Wed, 31 Aug 2011 11:50:51 +0200
> Stefan Tauner <stefan.tauner@student.tuwien.ac.at> wrote:
> 
> > if you don't want to do it, just say so and i
> > will do it when i have time. thanks for your effort!
> 
> hi andrew!
> 
> may i presume that you wont refine the patch? :)
> 

i have added some stuff and committed the patch in r1572.
thanks!

Patch

Index: flashchips.c
===================================================================
--- flashchips.c	(revision 1423)
+++ flashchips.c	(working copy)
@@ -2218,6 +2218,30 @@ 
 
 	{
 		.vendor		= "Atmel",
+		.name		= "AT49F010",
+		.bustype	= BUS_PARALLEL,
+		.manufacture_id	= ATMEL_ID,
+		.model_id	= ATMEL_AT49F010,
+		.total_size	= 128,
+		.page_size	= 256,
+		.feature_bits	= FEATURE_EITHER_RESET,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_jedec,
+		.probe_timing	= TIMING_ZERO,	/* Datasheet has no timing info specified */
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {128 * 1024, 1} },
+				.block_erase = erase_chip_block_jedec,
+			}
+		},
+ 		.write		= write_jedec_1,
+		.read		= read_memmapped,
+		.voltage	= {4500, 5500},
+	},
+
+	{
+		.vendor		= "Atmel",
 		.name		= "AT49F020",
 		.bustype	= BUS_PARALLEL,
 		.manufacture_id	= ATMEL_ID,
Index: flashchips.h
===================================================================
--- flashchips.h	(revision 1423)
+++ flashchips.h	(working copy)
@@ -181,6 +181,7 @@ 
 #define ATMEL_AT45DB642		/* No ID available */
 #define ATMEL_AT45DB642D	0x2800
 #define ATMEL_AT49BV512		0x03
+#define ATMEL_AT49F010		0x17	/* Same as AT49HF010 */
 #define ATMEL_AT49F020		0x0B
 #define ATMEL_AT49F002N		0x07	/* for AT49F002(N)  */
 #define ATMEL_AT49F002NT		0x08	/* for AT49F002(N)T */