Usage of MTD_UADDR_UNNECESSARY broken?

DISCUSSION Alexander Hoffmann 5 participants 15 messages Nov 8, 2004 – Nov 19, 2004 Memory technology devices (MTD)

Share & tools

Alexander Hoffmann

author

2004-11-08 11:54 UTC #0 lore raw

Hi everyone,

can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY . Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):

/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;

If I change MTD_UADDR_DONT_CARE to MTD_UADDR_UNNECESSARY everything works fine!

Ben Dooks

2004-11-08 12:06 UTC #1 lore raw

Quoted 8 lines · Alexander Hoffmann

Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY .
Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):

Quoted 4 lines

/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;

hmm, thought this masking had been eliminated in later copies of the mtd code?

Signature · 3 lines

Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Alexander Hoffmann

author

2004-11-08 12:55 UTC #2 lore raw

Quoted 16 lines · Ben Dooks

Quoted 12 lines · Alexander Hoffmann
Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY . Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):
/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;

Quoted 5 lines

hmm, thought this masking had been eliminated in later copies of the mtd code?

Ok, you are right. But this doesn't change the fact that

unlock_addrs[uaddr].addr1

refers to an nonexisting field in the unlock_addrs array.

Alex.

Thayne Harbaugh

2004-11-08 18:53 UTC #14 lore raw

In reply to Alexander Hoffmann

Quoted 4 lines · Alexander Hoffmann

Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY .

It's exactly what the comments state:

MTD_UADDR_DONT_CARE, /* Requires an arbitrary address */
MTD_UADDR_UNNECESSARY, /* Does not require any address */

Quoted 3 lines

Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):

Unlock addresses shouldn't be assigned if they aren't necessary - hence why there isn't an entry for them in unlock_addrs[]. There should be logic that checks this before making the assignment. Of course another way to do it would be to have an entry assigned, but not use it. I prefer, however, that things break when the logic is incorrect - which is the way it is now.

Quoted 7 lines

/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
If I change MTD_UADDR_DONT_CARE to MTD_UADDR_UNNECESSARY everything works fine!

Apparently there is a problem that it isn't exiting the function earlier for the MTD_UADDR_UNNECESSARY case.

Of course you also need to know which kind of chip you have: Is it a DONT_CARE or an UNNECESSARY?

Signature · 2 lines

Thayne Harbaugh
Linux Networx

Thayne Harbaugh

2004-11-08 20:13 UTC #3 lore raw

In reply to Alexander Hoffmann

Quoted 20 lines · Alexander Hoffmann

Quoted 18 lines · Ben Dooks
Quoted 14 lines · Alexander Hoffmann
Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY .
Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):
/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;

Quoted 2 lines

Quoted 2 lines
hmm, thought this masking had been eliminated in later copies of the mtd code?

Yes, the masking has been eliminated, but someone left the comment in (doh!).

Quoted 5 lines

Ok, you are right. But this doesn't change the fact that
unlock_addrs[uaddr].addr1
refers to an nonexisting field in the unlock_addrs array.

I don't see how the code you that you described is being reached. It looks like the start of jedec_probe_chip() checks for UNNECESSARY and returns 0 (although I would expect 1) and so cfi_jedec_setup() should never be called with UNNECESSARY (even for subsequent chips).

Can you tell me what your chip is and why you think that cfi_jedec_setup () is called?

Signature · 2 lines

Thayne Harbaugh
Linux Networx

Alexander Hoffmann

author

2004-11-12 15:15 UTC #4 lore raw

Hi Thayne,

sorry for the delayed response.

Quoted 36 lines · Thayne Harbaugh

Quoted 19 lines · Alexander Hoffmann
Quoted 15 lines · Ben Dooks
Quoted 12 lines · Alexander Hoffmann
Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY . Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):
/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
Quoted 4 lines
Quoted 4 lines
hmm, thought this masking had been eliminated in later copies of the mtd code?
Yes, the masking has been eliminated, but someone left the comment in (doh!).
Quoted 4 lines
Ok, you are right. But this doesn't change the fact that
unlock_addrs[uaddr].addr1

Quoted 9 lines

Quoted 3 lines
refers to an nonexisting field in the unlock_addrs array.
I don't see how the code you that you described is being reached. It looks like the start of jedec_probe_chip() checks for UNNECESSARY and returns 0 (although I would expect 1) and so cfi_jedec_setup() should never be called with UNNECESSARY (even for subsequent chips).

I am working with the cdb89712 development board from cirrus. This board has an "Intel 28F320B3B" chip (device_id = 0x8897). Apparently, jedec_probe() finds MTD_UADDR_0x0555_0x02AA.
for this chip, while the jedec_table[] specifies it as MTD_UADDR_UNNECESSARY.
Since the probed unlock type is overidden by the static one, the code _does_ reach jedec_setup().

What I haven't really understood is this: if the code refuses chips of type UNNECESSARY (the return code 0 from jedec_probe() is an error), then why are so many chips declared as UNECESSARY in jedec_table[]?

Erwin Authried

2004-11-12 15:53 UTC #5 lore raw

Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 16:15:

Quoted 63 lines

Hi Thayne,
sorry for the delayed response.
Quoted 38 lines · Thayne Harbaugh
Quoted 21 lines · Alexander Hoffmann
Quoted 17 lines · Ben Dooks
Quoted 14 lines · Alexander Hoffmann
Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY .
Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):
/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
Quoted 4 lines
Quoted 4 lines
hmm, thought this masking had been eliminated in later copies of the mtd code?
Yes, the masking has been eliminated, but someone left the comment in (doh!).
Quoted 4 lines
Ok, you are right. But this doesn't change the fact that
unlock_addrs[uaddr].addr1
Quoted 9 lines
Quoted 3 lines
refers to an nonexisting field in the unlock_addrs array.
I don't see how the code you that you described is being reached. It looks like the start of jedec_probe_chip() checks for UNNECESSARY and returns 0 (although I would expect 1) and so cfi_jedec_setup() should never be called with UNNECESSARY (even for subsequent chips).
I am working with the cdb89712 development board from cirrus. This board has an "Intel 28F320B3B" chip
(device_id = 0x8897). Apparently, jedec_probe() finds MTD_UADDR_0x0555_0x02AA.
for this chip, while the jedec_table[] specifies it as MTD_UADDR_UNNECESSARY.
Since the probed unlock type is overidden by the static one, the code _does_ reach
jedec_setup().

Quoted 5 lines

What I haven't really understood is this: if the code refuses chips of type UNNECESSARY
(the return code 0 from jedec_probe() is an error), then why are so many chips declared
as UNECESSARY in jedec_table[]?

MTD_UADDR_UNNECESSARY is used to specify that the chip does not need any unlock sequence at all. The "return 0" statement near the top of jedec_probe_chip is executed if all possible unlock sequences have been tried without finding a match in jedec table, thus it indicates an error. It's correct that your chip finds a match with MTD_UADDR_0x0555_0x02aa, because that's the first try and the chip doesn't care at all about the unlock sequence.

Regards,
Erwin

Alexander Hoffmann

author

2004-11-12 16:18 UTC #6 lore raw

Quoted 82 lines · Erwin Authried

Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 16:15:
Quoted 73 lines
Hi Thayne,
sorry for the delayed response.
Quoted 52 lines · Thayne Harbaugh
Quoted 25 lines · Alexander Hoffmann
Quoted 19 lines · Ben Dooks
Quoted 14 lines · Alexander Hoffmann
Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY . Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):
/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
Quoted 6 lines
Quoted 6 lines
hmm, thought this masking had been eliminated in later copies of the mtd code?
Yes, the masking has been eliminated, but someone left the comment in (doh!).
Quoted 9 lines
Ok, you are right. But this doesn't change the fact that
unlock_addrs[uaddr].addr1
refers to an nonexisting field in the unlock_addrs array.
Quoted 7 lines
I don't see how the code you that you described is being reached. It looks like the start of jedec_probe_chip() checks for UNNECESSARY and returns 0 (although I would expect 1) and so cfi_jedec_setup() should never be called with UNNECESSARY (even for subsequent chips).
I am working with the cdb89712 development board from cirrus. This board has an "Intel 28F320B3B" chip (device_id = 0x8897). Apparently, jedec_probe() finds MTD_UADDR_0x0555_0x02AA.
for this chip, while the jedec_table[] specifies it as MTD_UADDR_UNNECESSARY.
Since the probed unlock type is overidden by the static one, the code _does_ reach jedec_setup().
Quoted 5 lines
What I haven't really understood is this: if the code refuses chips of type UNNECESSARY (the return code 0 from jedec_probe() is an error), then why are so many chips declared as UNECESSARY in jedec_table[]?

Quoted 9 lines

MTD_UADDR_UNNECESSARY is used to specify that the chip does not need any unlock sequence at all. The "return 0" statement near the top of jedec_probe_chip is executed if all possible unlock sequences have been tried without finding a match in jedec table, thus it indicates an error. It's correct that your chip finds a match with MTD_UADDR_0x0555_0x02aa, because that's the first try and the chip doesn't care at all about the unlock sequence.

Does this mean, that the "return 0" statement at the beginning of jedec_probe_chip() is only reached when my chip isn't lsited in jedec_table[] ?
If it is listed , there will always be a match and therefore the functions cfi_jedec_setup() and finfo_uaddr() will be called !?

Alex.

Erwin Authried

2004-11-12 16:39 UTC #7 lore raw

Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 17:17:

Quoted 101 lines

Quoted 90 lines · Erwin Authried
Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 16:15:
Quoted 79 lines
Hi Thayne,
sorry for the delayed response.
Quoted 54 lines · Thayne Harbaugh
Quoted 27 lines · Alexander Hoffmann
Quoted 21 lines · Ben Dooks
Quoted 16 lines · Alexander Hoffmann
Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY .
Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):
/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
Quoted 6 lines
Quoted 6 lines
hmm, thought this masking had been eliminated in later copies of the mtd code?
Yes, the masking has been eliminated, but someone left the comment in (doh!).
Quoted 9 lines
Ok, you are right. But this doesn't change the fact that
unlock_addrs[uaddr].addr1
refers to an nonexisting field in the unlock_addrs array.
Quoted 7 lines
I don't see how the code you that you described is being reached. It looks like the start of jedec_probe_chip() checks for UNNECESSARY and returns 0 (although I would expect 1) and so cfi_jedec_setup() should never be called with UNNECESSARY (even for subsequent chips).
I am working with the cdb89712 development board from cirrus. This board has an "Intel 28F320B3B" chip
(device_id = 0x8897). Apparently, jedec_probe() finds MTD_UADDR_0x0555_0x02AA.
for this chip, while the jedec_table[] specifies it as MTD_UADDR_UNNECESSARY.
Since the probed unlock type is overidden by the static one, the code _does_ reach
jedec_setup().
Quoted 7 lines
What I haven't really understood is this: if the code refuses chips of type UNNECESSARY
(the return code 0 from jedec_probe() is an error), then why are so many chips declared
as UNECESSARY in jedec_table[]?
Quoted 9 lines
MTD_UADDR_UNNECESSARY is used to specify that the chip does not need any unlock sequence at all. The "return 0" statement near the top of jedec_probe_chip is executed if all possible unlock sequences have been tried without finding a match in jedec table, thus it indicates an error. It's correct that your chip finds a match with MTD_UADDR_0x0555_0x02aa, because that's the first try and the chip doesn't care at all about the unlock sequence.

Quoted 9 lines

Does this mean, that the "return 0" statement at the beginning of jedec_probe_chip() is only reached
when my chip isn't lsited in jedec_table[] ?
If it is listed , there will always be a match and therefore the functions cfi_jedec_setup() and finfo_uaddr()
will be called !?
Alex.

Yes, I think it should work that way. For flash with Intel cmdset, MTD_UADDR_UNNECESSARY is always used, because they do not use unlocking at all. In contrast, all AMD cmdset flashchips use unlocking.

Regards,
Erwin

Alexander Hoffmann

author

2004-11-18 10:50 UTC #8 lore raw

Hi,

Quoted 131 lines

Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 17:17:
Quoted 118 lines
Quoted 103 lines · Erwin Authried
Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 16:15:
Quoted 91 lines
Hi Thayne,
sorry for the delayed response.
Quoted 66 lines · Thayne Harbaugh
Quoted 31 lines · Alexander Hoffmann
Quoted 23 lines · Ben Dooks
Quoted 16 lines · Alexander Hoffmann
Hi everyone,
can anybody please explain me the exact difference between MTD_UADDR_DONT_CARE and MTD_UADDR_UNNECESSARY . Because if I use MTD_UADDR_UNNECESSARY an not existing field in the unlock_addrs array is beeing referenced (/drivers/mtd/chips/jedec_probe.c, function cfi_jedec_setup, line 1740):
/* Mask out address bits which are smaller than the device type */ mask = ~(p_cfi->device_type-1);
p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask; p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
Quoted 8 lines
Quoted 8 lines
hmm, thought this masking had been eliminated in later copies of the mtd code?
Yes, the masking has been eliminated, but someone left the comment in (doh!).
Quoted 11 lines
Ok, you are right. But this doesn't change the fact that
unlock_addrs[uaddr].addr1
refers to an nonexisting field in the unlock_addrs array.
Quoted 9 lines
I don't see how the code you that you described is being reached. It looks like the start of jedec_probe_chip() checks for UNNECESSARY and returns 0 (although I would expect 1) and so cfi_jedec_setup() should never be called with UNNECESSARY (even for subsequent chips).
I am working with the cdb89712 development board from cirrus. This board has an "Intel 28F320B3B" chip (device_id = 0x8897). Apparently, jedec_probe() finds MTD_UADDR_0x0555_0x02AA.
for this chip, while the jedec_table[] specifies it as MTD_UADDR_UNNECESSARY.
Since the probed unlock type is overidden by the static one, the code _does_ reach jedec_setup().
Quoted 7 lines
What I haven't really understood is this: if the code refuses chips of type UNNECESSARY (the return code 0 from jedec_probe() is an error), then why are so many chips declared as UNECESSARY in jedec_table[]?
Quoted 11 lines
MTD_UADDR_UNNECESSARY is used to specify that the chip does not need any unlock sequence at all. The "return 0" statement near the top of jedec_probe_chip is executed if all possible unlock sequences have been tried without finding a match in jedec table, thus it indicates an error. It's correct that your chip finds a match with MTD_UADDR_0x0555_0x02aa, because that's the first try and the chip doesn't care at all about the unlock sequence.
Quoted 9 lines
Does this mean, that the "return 0" statement at the beginning of jedec_probe_chip() is only reached when my chip isn't lsited in jedec_table[] ?
If it is listed , there will always be a match and therefore the functions cfi_jedec_setup() and finfo_uaddr() will be called !?
Alex.

Quoted 7 lines

Yes, I think it should work that way. For flash with Intel cmdset, MTD_UADDR_UNNECESSARY is always used, because they do not use unlocking at all. In contrast, all AMD cmdset flashchips use unlocking.
Regards,
Erwin

Then I guess that there is really the bug I described in my first mail. I would recommend a check for MTD_UADDR_UNNECESSARY in the cfi_jedec_setup(), before the unlock_addrs[] array is referenced:

if ( uaddr != MTD_UADDR_UNNECESSARY ) {
               p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask;
               p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
}

return 1;

Furthermore I dont understand the following code in the finfo_uaddr() function:

if (uaddr != MTD_UADDR_NOT_SUPPORTED ) {
               /* ASSERT("The unlock addresses for non-8-bit mode
                  are bollocks. We don't really need an array."); */
               uaddr = finfo->uaddr[0];
       }

In my opinion this can't work, because there are a lot of entries in the jedec_table[] where uaddr[0] is not defined ?

Best regards,
Alex.

Thayne Harbaugh

2004-11-18 15:08 UTC #9 lore raw

Quoted 10 lines · Alexander Hoffmann

Then I guess that there is really the bug I described in my first mail. I would recommend a check for MTD_UADDR_UNNECESSARY in the cfi_jedec_setup(), before
the unlock_addrs[] array is referenced:
if ( uaddr != MTD_UADDR_UNNECESSARY ) {
                p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask;
                p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
}
return 1;

Apparently the "& mask" is not done anymore - you must be using an older version of jedec_probe.c.

It looks like your suggestion might be appropriate after the window check in jedec_setup() and before the finfo_uaddr() lookup.

I'm still trying to sort the test at the top of jedec_probe_chip():

retry:

	if (!cfi->numchips) {
		uaddr_idx++;

		if (MTD_UADDR_UNNECESSARY == uaddr_idx)
			return 0;

		cfi->addr_unlock1 = unlock_addrs[uaddr_idx].addr1;
		cfi->addr_unlock2 = unlock_addrs[uaddr_idx].addr2;
	}

I'm thinking that the MTD_UADDR_UNNECESSARY test (which checks for the end-of-loop condition) should be *prior* to the uaddr_idx++. The way it is, chips that are MTD_UADDR_UNNECESSARY will *never* be tested and will *always* fail.

Quoted 8 lines

Furthermore I dont understand the following code in the finfo_uaddr() function:

if (uaddr != MTD_UADDR_NOT_SUPPORTED ) {
                /* ASSERT("The unlock addresses for non-8-bit mode
                   are bollocks. We don't really need an array."); */
                uaddr = finfo->uaddr[0];
        }

I don't necessarily understand it either. I have chip documentation that shows that the unlock addresses of a chip are different for different modes. Unfortunately, I don't have a chip to test.

Quoted 3 lines

In my opinion this can't work, because there are a lot of entries in the jedec_table[] where uaddr[0]
is not defined ?

C specifications require that structures with static initializers always have uninitialized members set to 0. That means that entries in jedec_table[] that don't have a specific uaddr[0] initializer then it is 0 - which is MTD_UADDR_UNSUPPORTED. What it buys us is that there's less typing and any width that isn't explicitly stated as supported is not supported.

Considering the above WRT the code logic, it means that if a width for a chip is not supported (MTD_UADDR_NOT_SUPPORTED) then it will return whatever the unlock address is for X8. The return value for finfo_uaddr () is used to not only determine what the unlock addresses should be, but also if a chip functions in a particular width. This could cause chips that only have an X16 and greater modes, but no X8 mode to fail in jedec_match() and jedec_setup().

Yes, it does look broken. If the ASSERT comment is *really* true (I am not convinced) then they should accomplish what they want by using the same uaddr initializer for all modes of a chip.

Signature · 2 lines

Thayne Harbaugh
Linux Networx

Erwin Authried

2004-11-18 15:27 UTC #10 lore raw

Am Don, den 18.11.2004 schrieb Thayne Harbaugh um 15:44:

Quoted 33 lines · Alexander Hoffmann

Quoted 11 lines
Then I guess that there is really the bug I described in my first mail. I would recommend a check for MTD_UADDR_UNNECESSARY in the cfi_jedec_setup(), before
the unlock_addrs[] array is referenced:
if ( uaddr != MTD_UADDR_UNNECESSARY ) {
                p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask;
                p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
}
return 1;
Apparently the "& mask" is not done anymore - you must be using an older version of jedec_probe.c.
It looks like your suggestion might be appropriate after the window check in jedec_setup() and before the finfo_uaddr() lookup.
I'm still trying to sort the test at the top of jedec_probe_chip():
retry:
	if (!cfi->numchips) {
		uaddr_idx++;

		if (MTD_UADDR_UNNECESSARY == uaddr_idx)
			return 0;

		cfi->addr_unlock1 = unlock_addrs[uaddr_idx].addr1;
		cfi->addr_unlock2 = unlock_addrs[uaddr_idx].addr2;
	}

Quoted 4 lines

I'm thinking that the MTD_UADDR_UNNECESSARY test (which checks for the end-of-loop condition) should be *prior* to the uaddr_idx++. The way it is, chips that are MTD_UADDR_UNNECESSARY will *never* be tested and will *always* fail.

I believe that your conclusion is wrong. jedec_probe doesn't know a-priori which chip is there, thus it can't omit the test. A test for a chip with MTD_UADDR_UNNECESSARY will match at the first tested unlock address, because a chip with MTD_UADDR_UNNECESSARY doesn't care about the unlock sequence at all.

Regards,
Erwin

Marius Groeger

2004-11-19 12:50 UTC #11 lore raw

Quoted 10 lines · Erwin Authried

Quoted 4 lines
I'm thinking that the MTD_UADDR_UNNECESSARY test (which checks for the end-of-loop condition) should be *prior* to the uaddr_idx++. The way it is, chips that are MTD_UADDR_UNNECESSARY will *never* be tested and will *always* fail.
I believe that your conclusion is wrong. jedec_probe doesn't know a-priori which chip is there, thus it can't omit the test. A test for a chip with MTD_UADDR_UNNECESSARY will match at the first tested unlock address, because a chip with MTD_UADDR_UNNECESSARY doesn't care about the unlock sequence at all.

Yes, this makes sense. However, I believe the real problem Alex ran into is that after the chip responded to the first address tested (as it should) and then found to be MTD_UADDR_UNNECESSARY, the uaddr index is _still_ used, subsequently. As far as I understand it, by way of the finfo_uaddr() function.

So what seems to be needed as a fix would be this: once a chip is found to be MTD_UADDR_UNNECESSARY, uaddr stuff should be done, at all, any more.

Does this make sense to you guys?

Regards,
Marius

Signature · 4 lines

Marius Groeger <[email protected]>
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.imerva.com

Marius Groeger

2004-11-19 13:13 UTC #12 lore raw

Quoted 1 line · Marius Groeger

So what seems to be needed as a fix would be this: once a chip is found to be MTD_UADDR_UNNECESSARY, uaddr stuff should be done, at all, any more.

Please change this to "should NOT be done" :-)

Cheers,
Marius

Signature · 4 lines

Marius Groeger <[email protected]>
SYSGO AG                      Embedded and Real-Time Software
Voice: +49 6136 9948 0                  FAX: +49 6136 9948 10
www.sysgo.com | www.elinos.com | www.osek.de | www.imerva.com

Thayne Harbaugh

2004-11-19 20:59 UTC #13 lore raw

In reply to Alexander Hoffmann

Quoted 10 lines · Alexander Hoffmann

Then I guess that there is really the bug I described in my first mail. I would recommend a check for MTD_UADDR_UNNECESSARY in the cfi_jedec_setup(), before
the unlock_addrs[] array is referenced:
if ( uaddr != MTD_UADDR_UNNECESSARY ) {
                p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask;
                p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
}
return 1;

Well, I committed a change, even though I originally wasn't going to do it this way:

mtd/chips/jedec_probe.c +5 -0

@@ -227,6 +227,11 @@
 	[MTD_UADDR_DONT_CARE] = {
 		.addr1 = 0x0000,      /* Doesn't matter which address */
 		.addr2 = 0x0000       /* is used - must be last entry */
+	},
+
+	[MTD_UADDR_UNNECESSARY] = {
+		.addr1 = 0x0000,
+		.addr2 = 0x0000
 	}
 };

I figure that simply adding a dummy entry will prevent having to always branch on MTD_UADDR_UNNECESSARRY when the remaining logic can be identical.

My apologies for taking so long - thanks for your patience.

BTW, I haven't seen any comments regarding this:

Quoted 36 lines · Thayne Harbaugh

Quoted 8 lines · Alexander Hoffmann
Furthermore I dont understand the following code in the finfo_uaddr() function:
if (uaddr != MTD_UADDR_NOT_SUPPORTED ) {
                /* ASSERT("The unlock addresses for non-8-bit mode
                   are bollocks. We don't really need an array."); */
                uaddr = finfo->uaddr[0];
        }
I don't necessarily understand it either. I have chip documentation that shows that the unlock addresses of a chip are different for different modes. Unfortunately, I don't have a chip to test.
Quoted 3 lines
In my opinion this can't work, because there are a lot of entries in the jedec_table[] where uaddr[0]
is not defined ?
C specifications require that structures with static initializers always have uninitialized members set to 0. That means that entries in jedec_table[] that don't have a specific uaddr[0] initializer then it is 0 - which is MTD_UADDR_UNSUPPORTED. What it buys us is that there's less typing and any width that isn't explicitly stated as supported is not supported.
Considering the above WRT the code logic, it means that if a width for a chip is not supported (MTD_UADDR_NOT_SUPPORTED) then it will return whatever the unlock address is for X8. The return value for finfo_uaddr () is used to not only determine what the unlock addresses should be, but also if a chip functions in a particular width. This could cause chips that only have an X16 and greater modes, but no X8 mode to fail in jedec_match() and jedec_setup().
Yes, it does look broken. If the ASSERT comment is *really* true (I am not convinced) then they should accomplish what they want by using the same uaddr initializer for all modes of a chip.

Signature · 2 lines

Thayne Harbaugh
Linux Networx