Re: 2.4 stuff.

[Alice Hennessy <ahennessy@mvista.com>]

Nicolas Pitre wrote:

> On Wed, 13 Dec 2000, David Woodhouse wrote:
>
> >
> > nico@cam.org said:
> > >  I think the block write discussion has settled on the current code.
> >
> > Cool. I'll get the AMD code up and running on the new toy on my desk and
> > send it off to Linus then.
>
> I just looked at the cfi_cmdset_0002.c code. This will goof on big endian as
> far as I can see.  A major cleanup, like what happened for
> cfi_cmdset_0001.c, would be welcome there as well.
>
> First, those functions could be cleaned out:
>
> amd_send_cmd_at()
> -----------------
> - referenced only once;
> - it applies cpu_to_le() on values which have already been correctly
>   converted in cfi_build_cmd()... oops!;
> - it should use cfi_write() instead of the current
>   switch(map->buswidth) {...};
> - probably diserve to be killed anyway.
>
> amd_write_val()
> ---------------
> - cfi_write() already does the same.
>
> amd_read_val()
> --------------
> - cfi-read() already does the same.
>
> Next, cfi_amdstd_write_byte_16() and cfi_amdstd_write_bytes_32() could
> greatly benefit from being generalized i.e. merged together and even folded
> into cfi_amdstd_write().  The code in cfi-cmdset-001.c should be a great
> example for that.  It also removes all the #ifdef noise while still
> preserving the ability for the compiler to optimize away the unused code
> when a specific bus geometry is selected. In addition, such code as
>
>         tmp = map->read32(map, ofs);
>         tmp = be32_to_cpu(tmp);
>         memcpy((u_char *)&tmp, buf, len);
>         tmp = cpu_to_be32(tmp);
>
> or ...
>
>         tmp = map->read32(map, ofs - 1);
>         tmp = (be32_to_cpu(tmp) & 0xFF000000) | buf[2] | (buf[1]  << 8) | buf[0] << 16;
>         tmp = cpu_to_be32(tmp);
>
> looks quite awful and suspicious to me.
>
> Next, all calls to udelay() should be combined with a test on
> current->need_resched and call schedule() instead of udelay() when true.
> Otherwise kiss goodbye to your system's low latency when writes occur.
>
> The last observation is the difference between cmdset-0001 and cmdset-002
> for the state in which the task is put while waiting for the chip.  For
> cmdset-001 the task is set to TASK_UNINTERRUPTIBLE for some reasons which
> are surely valuable for cmdset-002 as well.
>
> So enough criticism for now.  I would have been happy to clean this up
> myself, however I don't have any toys with AMD flash in it so couldn't test
> the changes at all which lower my motivation to do such work...
>
> Nicolas
>
> To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

I've made some  minor local changes so that both cfi_cmdset_0002.c and
cfi_probe.c work for my big endian, 4 byte AMD flash board.   I can carry it further
by making it more like cfi_cmdset_0001.c  if you need a volunteer and don't need
it by tomorrow  :-)

Alice



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