Big Endian/Little Endian headache in cfi_cmdset_0002.c

Big Endian/Little Endian headache in cfi_cmdset_0002.c

[mark.langsdorf]

	The write code in cfi_amdext_write_bytes_32 uses
be32_to_cpu and cpu_to_be32 to make the last four or 
less bytes of a buffer Big Endian.  However, none of 
the other write or read code in cfi_cmdset_002 is 
concerned with Endianess, so it looks like (on my 
x86 chip) that the first x bytes of a write are
little endian, and then the last 3 or 4 are reversed.
	Is there some reason for handling things this
way, or is this a bug in the code?

Mark Langsdorf
Advanced Micro Devices, Inc             Tel: 512.602.3756
5204 E. Ben White Blvd. M/S 590         Fax: 512.602.5051
Austin, TX 78741                        mark.langsdorf@amd.com


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

Re: Big Endian/Little Endian headache in cfi_cmdset_0002.c

[Alice Hennessy]

mark.langsdorf@amd.com wrote:

>         The write code in cfi_amdext_write_bytes_32 uses
> be32_to_cpu and cpu_to_be32 to make the last four or
> less bytes of a buffer Big Endian.  However, none of
> the other write or read code in cfi_cmdset_002 is
> concerned with Endianess, so it looks like (on my
> x86 chip) that the first x bytes of a write are
> little endian, and then the last 3 or 4 are reversed.
>         Is there some reason for handling things this
> way, or is this a bug in the code?
>
> Mark Langsdorf
> Advanced Micro Devices, Inc             Tel: 512.602.3756
> 5204 E. Ben White Blvd. M/S 590         Fax: 512.602.5051
> Austin, TX 78741                        mark.langsdorf@amd.com
>
> To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

I'm also having to alter cfi_cmdset_0002.c for big endianness -  I think
we should use
cfi_write and cfi_read functions that cfi_cmdset_0001.c uses  but alter
them to take care
of both be32_to_cpu and le32_to_cpu (depending on a define) to take care
of both endiannesses.
This would isolate both the bus width issue and the endianness in these
2 functions.   In other words, move the
endianness out of cfi_build_cmd and put it into cfi_read and
cfi_write.   Also,  the handling
of unaligned bytes in cfi_cmdset_0001. c is good for both
endiannesses.   We should use this
method in cfi_cmdset_0002.c

Alice



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

Re: Big Endian/Little Endian headache in cfi_cmdset_0002.c

[Alice Hennessy]

Alice Hennessy wrote:

> mark.langsdorf@amd.com wrote:
>
> >         The write code in cfi_amdext_write_bytes_32 uses
> > be32_to_cpu and cpu_to_be32 to make the last four or
> > less bytes of a buffer Big Endian.  However, none of
> > the other write or read code in cfi_cmdset_002 is
> > concerned with Endianess, so it looks like (on my
> > x86 chip) that the first x bytes of a write are
> > little endian, and then the last 3 or 4 are reversed.
> >         Is there some reason for handling things this
> > way, or is this a bug in the code?
> >
> > Mark Langsdorf
> > Advanced Micro Devices, Inc             Tel: 512.602.3756
> > 5204 E. Ben White Blvd. M/S 590         Fax: 512.602.5051
> > Austin, TX 78741                        mark.langsdorf@amd.com
> >
> > To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org
>
> I'm also having to alter cfi_cmdset_0002.c for big endianness -  I think
> we should use
> cfi_write and cfi_read functions that cfi_cmdset_0001.c uses  but alter
> them to take care
> of both be32_to_cpu and le32_to_cpu (depending on a define) to take care
> of both endiannesses.
> This would isolate both the bus width issue and the endianness in these
> 2 functions.   In other words, move the
> endianness out of cfi_build_cmd and put it into cfi_read and
> cfi_write.   Also,  the handling
> of unaligned bytes in cfi_cmdset_0001. c is good for both
> endiannesses.   We should use this
> method in cfi_cmdset_0002.c
>
> Alice
>
> To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Well,

After thinking and testing a bit more,  the endianness should only be a
concern
in the cfi_build_cmd (where it is now) or query reads (which need to be
altered slightly
when filling in the cfi_ident structure  for buswidth > 1 and big endian)
because a particular order is expected.
In data writes and reads, endianness should not be a concern except for the
unaligned bytes
which should follow the cfi_cmdset_0001.c code example instead of the
assumption of
a particular endianness that exists now in cfi_amdext_write_bytes_32.

Alice



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

RE: Big Endian/Little Endian headache in cfi_cmdset_0002.c

[mark.langsdorf]

> In data writes and reads, endianness should not be a concern 
> except for the unaligned bytes which should follow the
> cfi_cmdset_0001.c code example instead of the assumption of
> a particular endianness that exists now in cfi_amdext_write_bytes_32.

	I'm not sure what you mean - as far as I can tell,
cfi_cmdset_0001.c does deal with endianness except in 3
specific incidences, two of which cfi_cmdset_0002.c doesn't
use and the third which we don't currently implement (but
should, and probably could trivially).
	Is there something else I am missing, or can I take
all these endian references out of my code with a clear
conscience?

Mark Langsdorf
Advanced Micro Devices, Inc             Tel: 512.602.3756
5204 E. Ben White Blvd. M/S 590         Fax: 512.602.5051
Austin, TX 78741                        mark.langsdorf@amd.com
	



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

RE: Big Endian/Little Endian headache in cfi_cmdset_0002.c

[Nicolas Pitre]

On Thu, 16 Nov 2000 mark.langsdorf@amd.com wrote:

> > In data writes and reads, endianness should not be a concern
> > except for the unaligned bytes which should follow the
> > cfi_cmdset_0001.c code example instead of the assumption of
> > a particular endianness that exists now in cfi_amdext_write_bytes_32.
>
> 	I'm not sure what you mean - as far as I can tell,
> cfi_cmdset_0001.c does deal with endianness except in 3
> specific incidences, ...

Could you please point them out so I could fix them please?
AFAIK cfi_cmdset_0001.c is completely endian aware.


Nicolas




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

RE: Big Endian/Little Endian headache in cfi_cmdset_0002.c

[mark.langsdorf]

> On Thu, 16 Nov 2000 mark.langsdorf@amd.com wrote:
> 
> > > In data writes and reads, endianness should not be a concern
> > > except for the unaligned bytes which should follow the
> > > cfi_cmdset_0001.c code example instead of the assumption of
> > > a particular endianness that exists now in 
> cfi_amdext_write_bytes_32.
> >
> > 	I'm not sure what you mean - as far as I can tell,
> > cfi_cmdset_0001.c does deal with endianness except in 3
> > specific incidences, ...
> 
> Could you please point them out so I could fix them please?
> AFAIK cfi_cmdset_0001.c is completely endian aware.

	My mistake.  That should say "cfi_cmdset_001.c
doesn't deal with endianness except in 3 specific
incidences": setting the erase size (which cfi_cmdset_0002.c
does not currently do correctly right now) and setting 
the Feature Support and BlkStatusRegMask (which 
cfi_cmdset_0002.c doesn't support).
	AFAIK, cfi_cmdset_0001.c handles endianness
appropriately.  It doesn't do much with it, though, and
I wanted to be sure that we should take the same 
approach for cfi_cmdset_0002.c.  Which it looks like
we should - the correct patch is to remove all the
references to endianness.

Mark Langsdorf
Advanced Micro Devices, Inc             Tel: 512.602.3756
5204 E. Ben White Blvd. M/S 590         Fax: 512.602.5051
Austin, TX 78741                        mark.langsdorf@amd.com



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

Re: Big Endian/Little Endian headache in cfi_cmdset_0002.c

[Alice Hennessy]

mark.langsdorf@amd.com wrote:

> > In data writes and reads, endianness should not be a concern
> > except for the unaligned bytes which should follow the
> > cfi_cmdset_0001.c code example instead of the assumption of
> > a particular endianness that exists now in cfi_amdext_write_bytes_32.
>
>         I'm not sure what you mean - as far as I can tell,
> cfi_cmdset_0001.c does deal with endianness except in 3
> specific incidences, two of which cfi_cmdset_0002.c doesn't
> use and the third which we don't currently implement (but
> should, and probably could trivially).
>         Is there something else I am missing, or can I take
> all these endian references out of my code with a clear
> conscience?
>
> Mark Langsdorf
> Advanced Micro Devices, Inc             Tel: 512.602.3756
> 5204 E. Ben White Blvd. M/S 590         Fax: 512.602.5051
> Austin, TX 78741                        mark.langsdorf@amd.com
>

Sorry, I think I confused the issue with my big endian concerns.

The code that you have a problem with in cfi_amdext_write_bytes_32:

                /* if write address is aligned */
                if ((ofs & 0x03) == 0) {
                        tmp = map->read32(map, ofs);
                        tmp = be32_to_cpu(tmp);
                        memcpy((u_char *)&tmp, buf, len);
                        tmp = cpu_to_be32(tmp);
                        size = len;
                }

doesn't look like it should work for either little endian
or big endian - it just slams buf into big endian on a little endian
machine  (my big endian library
has be32_to_cpu as a nop).   This looks like a bug to me or for some
reason the coder wanted
the bytes to land in the flash in big endian order.  But this would only
be for the last bytes so it
doesn't make sense.

As far as the unaligned part of that code:

           else if ((ofs & 0x03) == 3) {
                        tmp = map->read32(map, ofs - 3);
                        tmp = (be32_to_cpu(tmp) & 0xFFFFFF00) | buf[0];
                        tmp = cpu_to_be32(tmp);
                        size = 1;
                }
                else if ((ofs & 0x03) == 2) {
                        tmp = map->read32(map, ofs - 2);
                        tmp = (be32_to_cpu(tmp) & 0xFFFF0000) | buf[1] |
(buf[0] << 8);
                        tmp = cpu_to_be32(tmp);
                        size = 2;
                }
                else if ((ofs & 0x03) == 1) {
                        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);
                        size = 3;
                }

This look ok except that unaligned data with lengths 1 or 2 would fail.
 Does this part work for you?

The code I was  referring to  in cfi_cmdset_0001.c is  for unaligned
writes in either write buffer or write words:

if (ofs & (CFIDEV_BUSWIDTH-1)) {
                unsigned long bus_ofs = ofs & ~(CFIDEV_BUSWIDTH-1);
                int gap = ofs - bus_ofs;
                int i = 0, n = 0;
                u_char tmp_buf[4];
                __u32 datum;

                while (gap--)
                        tmp_buf[i++] = 0xff;
                while (len && i < CFIDEV_BUSWIDTH)
                        tmp_buf[i++] = buf[n++], len--;
                while (i < CFIDEV_BUSWIDTH)
                        tmp_buf[i++] = 0xff;

                if (cfi_buswidth_is_2()) {
                        datum = *(__u16*)tmp_buf;
                } else if (cfi_buswidth_is_4()) {
                        datum = *(__u32*)tmp_buf;
                } else {
                        return -EINVAL;  /* should never happen, but be
safe */
                }

                ret = do_write_oneword(map, &cfi->chips[chipnum],
                                               bus_ofs, datum);
                if (ret)
                        return ret;

                ofs += n;
                buf += n;
                (*retlen) += n;

                ........

which just kepts the buf in data order (little endian) and also works for
unaligned data of lengths 1 or 2.

I altered it slightly for cfi_cmdset_0002.c  cfi_amdext_write_bytes_32()
because of the eventual check for read value matching write value:

 /* If it's not word-aligned, do the first bytes write */
                        if (ofs & 3) {
                                __u8 tmpbuf[4];
                                int i;
                                unsigned int data;

                                data = map->read32(map, ofs & 0xfffffffc);

                                for (i=0; i< (ofs & 3); i++)
                                        tmpbuf[i] = 0xff;
                                for (; i < min(4, len + (ofs & 3)) ; i++)
                                        tmpbuf[i] = buf[i - (ofs & 3)];
                                for (; i < 4; i++)
                                        tmpbuf[i] = 0xff;
                                data &= *(__u32 *)tmpbuf;
                                ret = do_write_oneword(map, chip, ofs &
0xfffffffc, (__u32 *) &data, 0);
                                if (ret)
                                        return ret;

                                buf += min(4-(ofs & 3), len);
                                ofs += min(4-(ofs & 3), len);
                                len -= min(4-(ofs & 3), len);


As far as my big endian environment,  I need to change the code slightly
in do_write_oneword()
so the data lands in flash in little endian order.  It seems the read
(memcpy_fromio)
is a byte read and therefore expects little endian order  - unless I am
missing something.

Alice



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

Big Endian issues

[Alice Hennessy]

Hello all,

I need some advice regarding my big endian arch (ppc). I am using
cfi_cmdset_0002.c  and there is
a built-in assumption that  words will be swapped when reading and writing to
a little endian device.
This assumption is present in cfi_build_cmd and in cfi_probe_new_chip.  Both
of these (in my configuration)
use map->write32 and map->read32 when dealing with commands and status
words.   This seems reasonable.

In dealing with data, however, map->write32 is used in writes but
map->copy_from is used in reads.
So, the assumption I assume is that map->copy_from will also swap bytes.
But in my environment,
map->copy_from (memcpy_fromio) doesn't swap bytes.  I don't think it needs to
if we make use of
map->copy_to (memcpy_toio)  when writing data (in amd_write_val and in the
immediate data check). In this case, copy_from and copy_to functions just
need to be consistent.

Any opinions?   Anyone else get this to work with big endian?
Is a change needed in my memcpy_fromio?

Alice



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

Re: Big Endian issues

[Nicolas Pitre]

On Tue, 28 Nov 2000, Alice Hennessy wrote:

> In dealing with data, however, map->write32 is used in writes but
> map->copy_from is used in reads.
> So, the assumption I assume is that map->copy_from will also swap bytes.
> But in my environment,
> map->copy_from (memcpy_fromio) doesn't swap bytes.  I don't think it needs to
> if we make use of
> map->copy_to (memcpy_toio)  when writing data (in amd_write_val and in the
> immediate data check). In this case, copy_from and copy_to functions just
> need to be consistent.

Neither of the write nor copy map methods is supposed to swap bytes.

Bytes might be swapped in cfi_build_cmd so you actually write commands and
compare with status in the device's endianness.  However data shouldn't be
swapped at all.


Nicolas



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