[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb-devel
Subject:    RE: [linux-usb-devel] RE: yet more on ohci-isp1362
From:       "Philipp Schmid" <schmid () amirix ! com>
Date:       2004-11-30 20:10:31
Message-ID: HCEMJJCIOBCJDAKIKENLMEHNCDAA.schmid () amirix ! com
[Download RAW message or body]

[mailto:linux-usb-devel-admin@lists.sourceforge.net]On Behalf Of Olav
Kongas
Sent: Tuesday, November 30, 2004 4:47 AM

Hi Philipp,

You have done an impressive work on documenting all this.
Thanks a lot. I'm sure this will simplify the work of
others.


> (4) As Expected: define platform specific HCHWCFG_INIT, HCDMACFG_INIT,
> CHIP_TYPE, ... macros (HCCHIPID_MAGIC needs to be defined specific to
1160
> or 1362 - inlcuded in Olav's patch)

Perhaps it would be better to remove the xxx_INIT macros
from the driver and rather expand the struct ohci_chip_info
to import these values together with other platform-specific
stuff.


That might be a good idea.


> Applying Big Endian Changes
> ---------------------------
>
> (3) The following 10-bit PTD access macros need to be changed, as it
indexes
> 16-bit fields inside the PTD structure.
>
> - PTD_GET_COUNT
> - PTD_GET_MPS
> - PTD_GET_LEN
>
> Note: only the extracts seem to need the byte swapping, as they do the
funky
> pointer conversion.

Just a question without too deep thinking - should the
macros in bitfield.h rather be changed instead of the
PTD_GET_xxx macros above? Are the bitfield.h macros
currently BE-compatible?

Overall it's hard for this driver to come up with a consistent access
method.  Normally the endian issue only becomes apparent in low level access
macros/functions, but in our case we are sandwich'ed between LE layers.
Sometimes you can do a LE/LE operation and be fine other times you are not.

bitfield.h does not do BE/LE conversion one way or another, especially since
it specifically only exists in the sa1100 and pxa trees.  But as long as you
use all bit operations in CPU type (i.e. logical values), you should be
fine, as it seems to use logical operators to create bit masks.  The file
seems simple enough (114 lines) that we could encorporate it in the driver,
hence, making the driver more portable.

The problem here is that the PTD stucture is defined in bytes

struct ptd {
	u8 count;
	u8 cc_toggle;
	u8 mps;
	u8 ep_spd;
	u8 len;
	u8 dir;
	u8 func_addr;
	u8 pr_sf;
} __attribute__((packed));

But these macros access these fields as 16-bit values
(*(u16*)(&(p)->count)).  This assumes a certain byte ordering in the ptd
structure.  Everything else seems to only access the PTD byte-by-byte so we
are fine there.  For writing, there are PTD_COUNT_H macros, but only a
single macro to read.  For consistency, we may want to do the same for
reading, preventing this particular problem altogether.  But I am fine by
simply using le16_to_cpu.

Olav



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://productguide.itmanagersjournal.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
[prev in list] [next in list] [prev in thread] [next in thread]