Usage of MTD_UADDR_UNNECESSARY broken?

Usage of MTD_UADDR_UNNECESSARY broken?

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

If I change MTD_UADDR_DONT_CARE to MTD_UADDR_UNNECESSARY everything 
works fine!

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Ben Dooks]

On Mon, Nov 08, 2004 at 12:54:16PM +0100, Alexander Hoffmann wrote:
> 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;

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

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Alexander Hoffmann]

Ben Dooks wrote:

>On Mon, Nov 08, 2004 at 12:54:16PM +0100, Alexander Hoffmann wrote:
>  
>
>>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;
>>    
>>
>
>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.

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

On Mon, 2004-11-08 at 13:55 +0100, Alexander Hoffmann wrote:
> Ben Dooks wrote:
> 
> >On Mon, Nov 08, 2004 at 12:54:16PM +0100, Alexander Hoffmann wrote:
> >  
> >
> >>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;
> >>    
> >>
> >
> >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!).

> 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?

-- 
Thayne Harbaugh
Linux Networx

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Alexander Hoffmann]

Hi Thayne,

sorry for the delayed response.

Thayne Harbaugh wrote:

>On Mon, 2004-11-08 at 13:55 +0100, Alexander Hoffmann wrote:
>  
>
>>Ben Dooks wrote:
>>
>>    
>>
>>>On Mon, Nov 08, 2004 at 12:54:16PM +0100, Alexander Hoffmann wrote:
>>>      
>>>
>>>>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;
>>>>        
>>>>
>>>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!).
>
>  
>
>>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).
>
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[]?

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Erwin Authried]

Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 16:15:
> Hi Thayne,
> 
> sorry for the delayed response.
> 
> Thayne Harbaugh wrote:
> 
> >On Mon, 2004-11-08 at 13:55 +0100, Alexander Hoffmann wrote:
> >  
> >
> >>Ben Dooks wrote:
> >>
> >>    
> >>
> >>>On Mon, Nov 08, 2004 at 12:54:16PM +0100, Alexander Hoffmann wrote:
> >>>      
> >>>
> >>>>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;
> >>>>        
> >>>>
> >>>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!).
> >
> >  
> >
> >>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).
> >
> 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[]?

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 

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Alexander Hoffmann]

Erwin Authried wrote:

>Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 16:15:
>  
>
>>Hi Thayne,
>>
>>sorry for the delayed response.
>>
>>Thayne Harbaugh wrote:
>>
>>    
>>
>>>On Mon, 2004-11-08 at 13:55 +0100, Alexander Hoffmann wrote:
>>> 
>>>
>>>      
>>>
>>>>Ben Dooks wrote:
>>>>
>>>>   
>>>>
>>>>        
>>>>
>>>>>On Mon, Nov 08, 2004 at 12:54:16PM +0100, Alexander Hoffmann wrote:
>>>>>     
>>>>>
>>>>>          
>>>>>
>>>>>>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;
>>>>>>       
>>>>>>
>>>>>>            
>>>>>>
>>>>>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!).
>>>
>>> 
>>>
>>>      
>>>
>>>>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).
>>>
>>>      
>>>
>>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[]?
>>    
>>
>
>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.

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Erwin Authried]

Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 17:17:
> Erwin Authried wrote:
> 
> >Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 16:15:
> >  
> >
> >>Hi Thayne,
> >>
> >>sorry for the delayed response.
> >>
> >>Thayne Harbaugh wrote:
> >>
> >>    
> >>
> >>>On Mon, 2004-11-08 at 13:55 +0100, Alexander Hoffmann wrote:
> >>> 
> >>>
> >>>      
> >>>
> >>>>Ben Dooks wrote:
> >>>>
> >>>>   
> >>>>
> >>>>        
> >>>>
> >>>>>On Mon, Nov 08, 2004 at 12:54:16PM +0100, Alexander Hoffmann wrote:
> >>>>>     
> >>>>>
> >>>>>          
> >>>>>
> >>>>>>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;
> >>>>>>       
> >>>>>>
> >>>>>>            
> >>>>>>
> >>>>>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!).
> >>>
> >>> 
> >>>
> >>>      
> >>>
> >>>>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).
> >>>
> >>>      
> >>>
> >>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[]?
> >>    
> >>
> >
> >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.
> 

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

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Alexander Hoffmann]

Hi,

>Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 17:17:
>  
>
>>Erwin Authried wrote:
>>
>>    
>>
>>>Am Fre, den 12.11.2004 schrieb Alexander Hoffmann um 16:15:
>>> 
>>>
>>>      
>>>
>>>>Hi Thayne,
>>>>
>>>>sorry for the delayed response.
>>>>
>>>>Thayne Harbaugh wrote:
>>>>
>>>>   
>>>>
>>>>        
>>>>
>>>>>On Mon, 2004-11-08 at 13:55 +0100, Alexander Hoffmann wrote:
>>>>>
>>>>>
>>>>>     
>>>>>
>>>>>          
>>>>>
>>>>>>Ben Dooks wrote:
>>>>>>
>>>>>>  
>>>>>>
>>>>>>       
>>>>>>
>>>>>>            
>>>>>>
>>>>>>>On Mon, Nov 08, 2004 at 12:54:16PM +0100, Alexander Hoffmann wrote:
>>>>>>>    
>>>>>>>
>>>>>>>         
>>>>>>>
>>>>>>>              
>>>>>>>
>>>>>>>>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;
>>>>>>>>      
>>>>>>>>
>>>>>>>>           
>>>>>>>>
>>>>>>>>                
>>>>>>>>
>>>>>>>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!).
>>>>>
>>>>>
>>>>>
>>>>>     
>>>>>
>>>>>          
>>>>>
>>>>>>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).
>>>>>
>>>>>     
>>>>>
>>>>>          
>>>>>
>>>>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[]?
>>>>   
>>>>
>>>>        
>>>>
>>>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.
>>
>>    
>>
>
>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.

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]

On Thu, 2004-11-18 at 11:50 +0100, Alexander Hoffmann wrote:

> 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.

> 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.

> 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.

-- 
Thayne Harbaugh
Linux Networx

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Erwin Authried]

Am Don, den 18.11.2004 schrieb Thayne Harbaugh um 15:44:
> On Thu, 2004-11-18 at 11:50 +0100, Alexander Hoffmann wrote:
> 
> > 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.

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  

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Marius Groeger]

On Thu, 18 Nov 2004, Erwin Authried wrote:

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

-- 
Marius Groeger <mgroeger@sysgo.com>
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

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Marius Groeger]

On Fri, 19 Nov 2004, Marius Groeger wrote:

> 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

-- 
Marius Groeger <mgroeger@sysgo.com>
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

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 3190 bytes --]

On Thu, 2004-11-18 at 11:50 +0100, Alexander Hoffmann wrote:

> 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:

--- drivers/mtd/chips/jedec_probe.c	18 Nov 2004 14:13:09 -0000	1.60
+++ drivers/mtd/chips/jedec_probe.c	19 Nov 2004 20:51:07 -0000
@@ -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:

On Thu, 2004-11-18 at 07:44 -0700, Thayne Harbaugh wrote: 
> On Thu, 2004-11-18 at 11:50 +0100, Alexander Hoffmann wrote:
> > 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.
> 
> > 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.


-- 
Thayne Harbaugh
Linux Networx

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Usage of MTD_UADDR_UNNECESSARY broken?

[Thayne Harbaugh]

[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]

On Mon, 2004-11-08 at 12:54 +0100, Alexander Hoffmann wrote:
> 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 */


> 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.

> /* 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?

-- 
Thayne Harbaugh
Linux Networx

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]