On Tue, 16 Jun 2009 23:11:06 +0800
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 16 Jun 2009, Alek Du wrote:
>
> > Have to send v2 version again, fixed a qh_destroy issue.
> >
> > From bd43fff4f26616f3abe58deffeafac3e05aea4c5 Mon Sep 17 00:00:00 2001
> > From: Alek Du <alek.du@xxxxxxxxx>
> > Date: Wed, 10 Jun 2009 11:53:59 +0800
> > Subject: [PATCH] EHCI: split ehci_qh structure into hw and sw parts
> >
> > The ehci_qh structure merged hw and sw together which is not good:
> > 1. If HCD has local SRAM, the sw part will consume it too, and it won't
> > bring any benefit.
> > 2. For non-x86 system, the entire ehci_qh is uncachable, actually we
> > only need the hw part to be uncacheable. Split them will give the sw
> > part cacheable feature.
>
>
> I'm sorry to put you through all this work, but the patch you wrote is
> backwards!
>
> Instead of letting ehci_qh be the hardware structure and defining a new
> ehci_qh_sw to hold the software part, you should have done it the other
> way around: Let ehci_qh be the software structure and call the hardware
> portion into struct ehci_qh_hw. And add a pointer to the ehci_qh_hw
> structure inside ehci_qh. (In fact, you might make it a const pointer
> because it's never going to change once it has been assigned.)
>
> Why? Because the way you did it loses all the advantage of using
> cacheable memory. For example, you have:
>
It is not simple just let ehci_qh contain the soft part, since the ehci_shadow requires
the hardware part to be the first part...
/*
* Entries in periodic shadow table are pointers to one of four kinds
* of data structure. That's dictated by the hardware; a type tag is
* encoded in the low bits of the hardware's periodic schedule. Use
* Q_NEXT_TYPE to get the tag.
*
* For entries in the async schedule, the type tag always says "qh".
*/
union ehci_shadow {
struct ehci_qh *qh; /* Q_TYPE_QH */
struct ehci_itd *itd; /* Q_TYPE_ITD */
struct ehci_sitd *sitd; /* Q_TYPE_SITD */
struct ehci_fstn *fstn; /* Q_TYPE_FSTN */
__hc32 *hw_next; /* (all types) */
void *ptr;
};
Unless if I split all the ehci_qh, ehci_itd, ehci_sitd, ehci_fstn part into two parts...
union ehci_shadow {
struct ehci_qh_hw *qh; /* Q_TYPE_QH */
struct ehci_itd_hw *itd; /* Q_TYPE_ITD */
struct ehci_sitd_hw *sitd; /* Q_TYPE_SITD */
struct ehci_fstn_hw *fstn; /* Q_TYPE_FSTN */
__hc32 *hw_next; /* (all types) */
void *ptr;
};
Thanks,
Alek
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
