Flash chip locking

Flash chip locking

[David Woodhouse]

OK. After some deliberation, this is the arrangement I'm about to code up 
for preventing concurrent access to flash chips. If you don't like it, 
feel free to present a better alternative.

Each chip is protected by a spinlock, which prevents it from being accessed 
concurrently by different CPUs. Lovely, simple, and even free on UP 
machines. 

Unfortunately, we have to consider the fact that this lock will also be 
obtained from a Bottom Half context, when the "has the erase finished yet?" 
timer runs. Therefore, all occurrences of spin_lock in the main code have 
to be spin_lock_bh()¹ instead of the nurmal (and almost free) spin_lock().

This means that bottom halves are disabled for the entire time that we're 
talking to the flash chip. Not good if we hold them for a long time, like 
for example the 128µs that we expect a typical write cycle to take. 

So we try to keep the latency down to a minimum. Rather than the naïve 
inner loop which looked like this:

foreach(word to write) {
	spin_lock_bh();
	writeb(WRITE_COMMAND);
	writeb(datum);
	while (!done)
		;
	spin_unlock_bh();
}

... we do something more like

foreach(word to write) {

retry:
	spin_lock_bh();
	if (!ready) {
		spin_unlock()
		udelay(a little while);
		goto retry;
	}
	writeb(WRITE_COMMAND);
	writeb(datum);
	spin_unlock_bh();
	udelay(expected time for the write we just started);
	spin_lock_bh();
	check final status, loop or whatever
	spin_unlock_bh();
}

We'll need to keep a 'state' variable in the per-chip data structure, so 
that if anything else grabs the lock while we're waiting for an operation 
to complete, it knows that there's an operation in progress and that it 
should either suspend it and do its thing before resuming the operation, 
or just go away and wait for the operation to finish.

We may add a wait queue to handle the latter case - so the write call can 
wake_up the waiting processes when it's completely finished. This will be
come more clear as I code it up.


¹ spin_lock_bh() doesn't exist in 2.2 so it's added to compatmac.h as:
#define spin_lock_bh(lock) do {start_bh_atomic();spin_lock(lock);}while(0);


--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[Kira Brown]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 694 bytes --]



On Wed, 28 Jun 2000, Brendan Simon wrote:

> You are implying that 128us is a large amount of time to wait.  Maybe 
> with todays processors it is, I don't really know if it is or isn't for 
> the average processor speed.

On a 1GHz processor, sometimes we can do more than one instruction every
1nsec. 128usecs is an eternity.  (128usecs was long enough to do a
complete DRAM refresh on a 1MHz 6502...)

> Does the udelay() imply that the scheduler can switch to another process ?

Specifically it's there so that interrupts can happen so that bottom ends
can run;  scheduling isn't an issue.

kira.



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[David Woodhouse]

brendan.simon@ctam.com.au said:
> Or to avoid ugly "goto" statements.
>   spin_lock_bh();
>   while (!ready)
>   {
>       spin_unlock()
>       udelay(a_little_while);
>       spin_lock_bh();
>   }

Personally, I prefer the goto version. I think it's clearer. We're 
releasing the lock and going back to exactly the state we were in at the 
'retry:' label. It doesn't really matter though. 



> You are implying that 128us is a large amount of time to wait.  Maybe
> with todays processors it is, I don't really know if it is or isn't
> for  the average processor speed. 

It's a long time to disable interrupts or bottom halves. If we didn't have 
to disable bottom halves, I wouldn't worry about it.


> Does the udelay() imply that the scheduler  can switch to another process? 

No. We'd use schedule_timeout() to allow the scheduler to switch.

> If so, I would have thought that the  scheduling process would take a lot
> longer that 128us, but I could be  wrong !!! 

I agree - that's why I used udelay() which is a busy-wait rather than 
scheduling. 

> If no scheduling is performed  then then there would be no difference to
> the naive "foreach" loop that you mention.

The difference is that in the latter version we are allowing interrupts
while we're doing the delay, while in the former they're disabled. There 
are in fact _two_ major differences between the two - the presence of the 
delay, and the place at which we actually wait for the chip to be ready.

The delay is just an optimisation - there's no a lot of point in beating on 
the chip's READY line until there's at least a chance that it'll be done, 
whether we're busy-waiting with IRQs disabled or not.

The important bit is that we let the interrupts run while we're waiting for 
the chip.




--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[Richard Gooch]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

Brendan Simon writes:
> You are implying that 128us is a large amount of time to wait.  Maybe 
> with todays processors it is, I don't really know if it is or isn't for 
> the average processor speed.  Does the udelay() imply that the scheduler 
> can switch to another process ?  If so, I would have thought that the 
> scheduling process would take a lot longer that 128us, but I could be 
> wrong !!!

Even on a P5, scheduling processes is < 10 us. The problem is getting
control back before they complete their timeslice. If we had an
interrupt after that 128 us it would be easy.

				Regards,

					Richard....
Permanent: rgooch@atnf.csiro.au
Current:   rgooch@ras.ucalgary.ca


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[Philipp Rumpf]

On Wed, Jun 28, 2000 at 11:36:03AM +0100, David Woodhouse wrote:
> OK. After some deliberation, this is the arrangement I'm about to code up 
> for preventing concurrent access to flash chips. If you don't like it, 
> feel free to present a better alternative.
> 
> Each chip is protected by a spinlock, which prevents it from being accessed 
> concurrently by different CPUs. Lovely, simple, and even free on UP 
> machines. 

on UP machines with recent compilers, at least.

> This means that bottom halves are disabled for the entire time that we're 
> talking to the flash chip. Not good if we hold them for a long time, like 
> for example the 128µs that we expect a typical write cycle to take. 

Not good, but shouldn't be fatal either.

> So we try to keep the latency down to a minimum. Rather than the naïve 
> inner loop which looked like this:
> 
> foreach(word to write) {
> 	spin_lock_bh();
> 	writeb(WRITE_COMMAND);
> 	writeb(datum);
> 	while (!done)
> 		;
> 	spin_unlock_bh();
> }
> 
> ... we do something more like
> 
> foreach(word to write) {
> 
> retry:
> 	spin_lock_bh();
> 	if (!ready) {

Just to be sure, ready is really a complicated writeb/readb sequence ?

> 		spin_unlock()
> 		udelay(a little while);

udelay(1); should be sufficient here.

> 		goto retry;
> 	}
> 	writeb(WRITE_COMMAND);
> 	writeb(datum);
> 	spin_unlock_bh();
> 	udelay(expected time for the write we just started);
> 	spin_lock_bh();
> 	check final status, loop or whatever
> 	spin_unlock_bh();
> }
> 
> We'll need to keep a 'state' variable in the per-chip data structure, so 
> that if anything else grabs the lock while we're waiting for an operation 
> to complete, it knows that there's an operation in progress and that it 
> should either suspend it and do its thing before resuming the operation, 
> or just go away and wait for the operation to finish.

Sounds like yucky hardware to me .. otoh, a state variable shouldn't be a
problem.

> We may add a wait queue to handle the latter case - so the write call can 
> wake_up the waiting processes when it's completely finished. This will be
> come more clear as I code it up.

That'd benefit SMP only, wouldn't it ?

	Philipp Rumpf


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[Brendan Simon]

David Woodhouse wrote:
> 
> So we try to keep the latency down to a minimum. Rather than the naïve 
> inner loop which looked like this:
> 
> foreach(word to write) {
> 	spin_lock_bh();
> 	writeb(WRITE_COMMAND);
> 	writeb(datum);
> 	while (!done)
> 		;
> 	spin_unlock_bh();
> }
> 
> .... we do something more like
> 
> foreach(word to write) {
> 
> retry:
> 	spin_lock_bh();
> 	if (!ready) {
> 		spin_unlock()
> 		udelay(a little while);
> 		goto retry;
> 	}
> 	writeb(WRITE_COMMAND);
> 	writeb(datum);
> 	spin_unlock_bh();
> 	udelay(expected time for the write we just started);
> 	spin_lock_bh();
> 	check final status, loop or whatever
> 	spin_unlock_bh();
Or to avoid ugly "goto" statements.
   spin_lock_bh();
   while (!ready)
   {
       spin_unlock()
       udelay(a_little_while);
       spin_lock_bh();
   }

You are implying that 128us is a large amount of time to wait.  Maybe 
with todays processors it is, I don't really know if it is or isn't for 
the average processor speed.  Does the udelay() imply that the scheduler 
can switch to another process ?  If so, I would have thought that the 
scheduling process would take a lot longer that 128us, but I could be 
wrong !!!
If no scheduling is performed then then there would be no difference to 
the naive "foreach" loop that you mention.

Are my assumptions reasonable ?

Brendan Simon.




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[Brendan Simon]

David Woodhouse wrote:
> 
> brendan.simon@ctam.com.au said:
> 
> 
> > Or to avoid ugly "goto" statements.
> >   spin_lock_bh();
> >   while (!ready)
> >   {
> >       spin_unlock()
> >       udelay(a_little_while);
> >       spin_lock_bh();
> >   }

Maybe this is a better so that the spin_lock_bh() call is not repeated.
   while(1)
   {
       spin_lock_bh();
       if (ready)
       {
           break;
       }
       spin_unlock();
       udelay(some_period);
   }

> Personally, I prefer the goto version. I think it's clearer. We're 
> releasing the lock and going back to exactly the state we were in at the 
> 'retry:' label. It doesn't really matter though. 
You are right.  At the end of the day I guess it doesn't really matter.
I just think that using labels and gotos is not a scalable technique, so 
I try to avoid them.

Thanks for the explanations below.  It is much clearer now.  I am still 
green when it comes to linux internals.  Hopefully that will change in 
the next 6 months when I have to write drivers and apps for a powerpc 
embedded product with DOC and some custom hardware.

Thanks,
Brendan Simon.

> > You are implying that 128us is a large amount of time to wait.  Maybe
> > with todays processors it is, I don't really know if it is or isn't
> > for  the average processor speed. 
> 
> It's a long time to disable interrupts or bottom halves. If we didn't have 
> to disable bottom halves, I wouldn't worry about it.
> 
> 
> > Does the udelay() imply that the scheduler  can switch to another process? 
> 
> No. We'd use schedule_timeout() to allow the scheduler to switch.
> 
> > If so, I would have thought that the  scheduling process would take a lot
> > longer that 128us, but I could be  wrong !!! 
> 
> I agree - that's why I used udelay() which is a busy-wait rather than 
> scheduling. 
> 
> > If no scheduling is performed  then then there would be no difference to
> > the naive "foreach" loop that you mention.
> 
> The difference is that in the latter version we are allowing interrupts
> while we're doing the delay, while in the former they're disabled. There 
> are in fact _two_ major differences between the two - the presence of the 
> delay, and the place at which we actually wait for the chip to be ready.
> 
> The delay is just an optimisation - there's no a lot of point in beating on 
> the chip's READY line until there's at least a chance that it'll be done, 
> whether we're busy-waiting with IRQs disabled or not.
> 
> The important bit is that we let the interrupts run while we're waiting for 
> the chip.




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[David Woodhouse]

On Thu, 12 Oct 2000, Alice Hennessy wrote:

> Was the  "has the erase finished yet?" timer the only reason for using
> spin_lock_bh instead of spin_lock?   Since the timer isn't implemented yet,
> spin_lock can be used at the moment, correct?   I'm experimenting with the
> driver in an environment that can  only tolerate disabling the bottom half for
> very brief periods.

Yes, correct.

-- 
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[Alice Hennessy]

David Woodhouse wrote:

> Unfortunately, we have to consider the fact that this lock will also be
> obtained from a Bottom Half context, when the "has the erase finished yet?"
> timer runs. Therefore, all occurrences of spin_lock in the main code have
> to be spin_lock_bh()¹ instead of the nurmal (and almost free) spin_lock().
>
>

Was the  "has the erase finished yet?" timer the only reason for using
spin_lock_bh instead of spin_lock?   Since the timer isn't implemented yet,
spin_lock can be used at the moment, correct?   I'm experimenting with the
driver in an environment that can  only tolerate disabling the bottom half for
very brief periods.

Alice



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Flash chip locking

[David Woodhouse]

On Thu, 12 Oct 2000, Alice Hennessy wrote:

> Was the  "has the erase finished yet?" timer the only reason for using
> spin_lock_bh instead of spin_lock?   Since the timer isn't implemented yet,
> spin_lock can be used at the moment, correct?   I'm experimenting with the
> driver in an environment that can  only tolerate disabling the bottom half for
> very brief periods.

Actually, even in the case where the timer _may_ be used, it's safe to
check whether a timer is _actually_ pending and only disable bottom halves
if it is. Because you hold the spinlock you know no other routine is going
to set a timer until you've finished.

-- 
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org