Hi,
I tried to modify fusbh200 hcd driver following ehci-platform.c.
However, the register definition of fusbh200 is partially incompatible
to ehci. For fusbh200, only the elements between "command" and
"async_next" in struct ehci_regs are consistent with ehci which means
it would cause copious modification and duplication of ehci hcd
driver. For example, there is no "configured_flag" register in
fusbh200 controller, yet, ehci hcd driver accesses "configured_flag"
in function ehci_run which would cause compile errors. Therefore,
maybe my first patch which refers to oxu210hp-hcd is a better
solution?
Thanks for your time.
Yuan-Hsin
On Thu, Feb 7, 2013 at 11:03 AM, Yuan-Hsin Chen <[email protected]> wrote:
> On Thu, Feb 7, 2013 at 12:38 AM, Felipe Balbi <[email protected]> wrote:
>> On Wed, Feb 06, 2013 at 07:24:01PM +0800, Yuan-Hsin Chen wrote:
>>> From: Yuan-Hsin Chen <[email protected]>
>>>
>>> USB2.0 HCD driver for Faraday FUSBH200
>>>
>>> Signed-off-by: Yuan-Hsin Chen <[email protected]>
>>
>> just use ehci-platform.c and avoid the duplication
>>
>> --
>> balbi
>
> Thanks for your advice. I will modify and re-submit it later.
>
> Yuan-Hsin
Hi,
(don't top-post)
On Mon, Mar 18, 2013 at 06:06:18PM +0800, Yuan-Hsin Chen wrote:
> Hi,
>
> I tried to modify fusbh200 hcd driver following ehci-platform.c.
> However, the register definition of fusbh200 is partially incompatible
> to ehci. For fusbh200, only the elements between "command" and
> "async_next" in struct ehci_regs are consistent with ehci which means
> it would cause copious modification and duplication of ehci hcd
> driver. For example, there is no "configured_flag" register in
> fusbh200 controller, yet, ehci hcd driver accesses "configured_flag"
> in function ehci_run which would cause compile errors. Therefore,
> maybe my first patch which refers to oxu210hp-hcd is a better
> solution?
why don't you just add a quirk flag to ehci struct so that it knows it
shouldn't access CONFIGFLAG register when that's non-existent ?
There are only 5 uses of configured_flag in ehci-hcd.c, so it should be
easy to wrap that around a flag check.
Alan, would you have a better idea ? Looks like this is a non-standard
EHCI implementation.
--
balbi
On Mon, 18 Mar 2013, Felipe Balbi wrote:
> On Mon, Mar 18, 2013 at 06:06:18PM +0800, Yuan-Hsin Chen wrote:
> > Hi,
> >
> > I tried to modify fusbh200 hcd driver following ehci-platform.c.
> > However, the register definition of fusbh200 is partially incompatible
> > to ehci. For fusbh200, only the elements between "command" and
> > "async_next" in struct ehci_regs are consistent with ehci which means
What about the port_status registers? They're not between command and
async_next. If they aren't consistent with EHCI, it makes things a lot
more complicated.
> > it would cause copious modification and duplication of ehci hcd
> > driver. For example, there is no "configured_flag" register in
> > fusbh200 controller, yet, ehci hcd driver accesses "configured_flag"
> > in function ehci_run which would cause compile errors. Therefore,
> > maybe my first patch which refers to oxu210hp-hcd is a better
> > solution?
>
> why don't you just add a quirk flag to ehci struct so that it knows it
> shouldn't access CONFIGFLAG register when that's non-existent ?
>
> There are only 5 uses of configured_flag in ehci-hcd.c, so it should be
> easy to wrap that around a flag check.
Two of those uses turn configured_flag on and two of them turn it off.
However, one of the uses tests its value (the first one in
ehci_resume). How would that be handled if configured_flag doesn't
exist?
> Alan, would you have a better idea ? Looks like this is a non-standard
> EHCI implementation.
Yes, it does. If all we need is to protect four or five accesses with
a quirk flag, that's okay with me.
Alan Stern
Hi,
On Mon, Mar 18, 2013 at 10:16:56AM -0400, Alan Stern wrote:
> > > it would cause copious modification and duplication of ehci hcd
> > > driver. For example, there is no "configured_flag" register in
> > > fusbh200 controller, yet, ehci hcd driver accesses "configured_flag"
> > > in function ehci_run which would cause compile errors. Therefore,
> > > maybe my first patch which refers to oxu210hp-hcd is a better
> > > solution?
> >
> > why don't you just add a quirk flag to ehci struct so that it knows it
> > shouldn't access CONFIGFLAG register when that's non-existent ?
> >
> > There are only 5 uses of configured_flag in ehci-hcd.c, so it should be
> > easy to wrap that around a flag check.
>
> Two of those uses turn configured_flag on and two of them turn it off.
> However, one of the uses tests its value (the first one in
> ehci_resume). How would that be handled if configured_flag doesn't
> exist?
perhaps they don't use it because they have a root-hub TT ? In that case
you assume EHCI *always* owns the port. Would that work ?
--
balbi
On Mon, 18 Mar 2013, Felipe Balbi wrote:
> Hi,
>
> On Mon, Mar 18, 2013 at 10:16:56AM -0400, Alan Stern wrote:
> > > > it would cause copious modification and duplication of ehci hcd
> > > > driver. For example, there is no "configured_flag" register in
> > > > fusbh200 controller, yet, ehci hcd driver accesses "configured_flag"
> > > > in function ehci_run which would cause compile errors. Therefore,
> > > > maybe my first patch which refers to oxu210hp-hcd is a better
> > > > solution?
> > >
> > > why don't you just add a quirk flag to ehci struct so that it knows it
> > > shouldn't access CONFIGFLAG register when that's non-existent ?
> > >
> > > There are only 5 uses of configured_flag in ehci-hcd.c, so it should be
> > > easy to wrap that around a flag check.
> >
> > Two of those uses turn configured_flag on and two of them turn it off.
> > However, one of the uses tests its value (the first one in
> > ehci_resume). How would that be handled if configured_flag doesn't
> > exist?
>
> perhaps they don't use it because they have a root-hub TT ? In that case
> you assume EHCI *always* owns the port. Would that work ?
The usage in ehci_resume is meant for checking whether power was lost
while the controller was suspended. This probably isn't an issue
during runtime suspend; it's likely to come up only during system
suspend. If Yuan-Hsin's platform doesn't support system suspend then
we don't have to worry about it.
Alan Stern
Hi,
On Mon, Mar 18, 2013 at 10:16 PM, Alan Stern <[email protected]> wrote:
> On Mon, 18 Mar 2013, Felipe Balbi wrote:
>
>> On Mon, Mar 18, 2013 at 06:06:18PM +0800, Yuan-Hsin Chen wrote:
>> > Hi,
>> >
>> > I tried to modify fusbh200 hcd driver following ehci-platform.c.
>> > However, the register definition of fusbh200 is partially incompatible
>> > to ehci. For fusbh200, only the elements between "command" and
>> > "async_next" in struct ehci_regs are consistent with ehci which means
>
> What about the port_status registers? They're not between command and
> async_next. If they aren't consistent with EHCI, it makes things a lot
> more complicated.
fusbh200 has only one port_status register with different offset,
0x30, and the position of some bits are different from EHCI.
>
>> > it would cause copious modification and duplication of ehci hcd
>> > driver. For example, there is no "configured_flag" register in
>> > fusbh200 controller, yet, ehci hcd driver accesses "configured_flag"
>> > in function ehci_run which would cause compile errors. Therefore,
>> > maybe my first patch which refers to oxu210hp-hcd is a better
>> > solution?
>>
>> why don't you just add a quirk flag to ehci struct so that it knows it
>> shouldn't access CONFIGFLAG register when that's non-existent ?
Also, usbmode_ex, hostpc, and txfill_tuning other than configured_flag
are non-existent in fusbh200. They are used in both ehci-hcd.c and
ehci-hub.c for several times.
>>
>> There are only 5 uses of configured_flag in ehci-hcd.c, so it should be
>> easy to wrap that around a flag check.
>
> Two of those uses turn configured_flag on and two of them turn it off.
> However, one of the uses tests its value (the first one in
> ehci_resume). How would that be handled if configured_flag doesn't
> exist?
>
>> Alan, would you have a better idea ? Looks like this is a non-standard
>> EHCI implementation.
>
> Yes, it does. If all we need is to protect four or five accesses with
> a quirk flag, that's okay with me.
Thanks for your time.
>
> Alan Stern
>
On Tue, 19 Mar 2013, Yuan-Hsin Chen wrote:
> > What about the port_status registers? They're not between command and
> > async_next. If they aren't consistent with EHCI, it makes things a lot
> > more complicated.
>
> fusbh200 has only one port_status register with different offset,
> 0x30, and the position of some bits are different from EHCI.
That's pretty nasty. Integrating that with the standard EHCI driver
would be considerably more difficult.
Why was the FUSBH200 designed in this strange way? Why doesn't it use
the standard EHCI register layout? Were the engineers at Faraday
deliberately trying to make life harder for driver writers?
> Also, usbmode_ex, hostpc, and txfill_tuning other than configured_flag
> are non-existent in fusbh200. They are used in both ehci-hcd.c and
> ehci-hub.c for several times.
They are used only if the hardware supports them, that is, only if the
ehci->has_hostpc flag is set.
Alan Stern
On Tue, Mar 19, 2013 at 11:48 PM, Alan Stern <[email protected]> wrote:
> On Tue, 19 Mar 2013, Yuan-Hsin Chen wrote:
>
>> > What about the port_status registers? They're not between command and
>> > async_next. If they aren't consistent with EHCI, it makes things a lot
>> > more complicated.
>>
>> fusbh200 has only one port_status register with different offset,
>> 0x30, and the position of some bits are different from EHCI.
>
> That's pretty nasty. Integrating that with the standard EHCI driver
> would be considerably more difficult.
>
> Why was the FUSBH200 designed in this strange way? Why doesn't it use
> the standard EHCI register layout? Were the engineers at Faraday
> deliberately trying to make life harder for driver writers?
Since FUSBH200 was designed more than eight years ago, the engineers
responsible for FUSBH200 are no longer working for Faraday. I have the
same question as you, but no one here could answer it. The strange
register layout and other implementation incompatible with EHCI are
the reasons why the FUSBH200 driver was not tried to submit to
mainline in the past.
>
>> Also, usbmode_ex, hostpc, and txfill_tuning other than configured_flag
>> are non-existent in fusbh200. They are used in both ehci-hcd.c and
>> ehci-hub.c for several times.
>
> They are used only if the hardware supports them, that is, only if the
> ehci->has_hostpc flag is set.
>
> Alan Stern
>
Hi,
On Tue, Mar 19, 2013 at 11:48 PM, Alan Stern <[email protected]> wrote:
> On Tue, 19 Mar 2013, Yuan-Hsin Chen wrote:
>
>> > What about the port_status registers? They're not between command and
>> > async_next. If they aren't consistent with EHCI, it makes things a lot
>> > more complicated.
>>
>> fusbh200 has only one port_status register with different offset,
>> 0x30, and the position of some bits are different from EHCI.
How about adding kernel configuration to adjust offset for FUSBH200 in
ehci_def.h? So port_status would be in offset 0x20 from ehci_regs.
For example,
/* ASYNCLISTADDR: offset 0x18 */
u32 async_next; /* address of next async queue head */
#ifndef CONFIG_USB_EHCI_HCD_FUSBH200
u32 reserved1[2];
/* TXFILLTUNING: offset 0x24 */
u32 txfill_tuning; /* TX FIFO Tuning register */
#define TXFIFO_DEFAULT (8<<16) /* FIFO burst threshold 8 */
u32 reserved2[6];
/* CONFIGFLAG: offset 0x40 */
u32 configured_flag;
#define FLAG_CF (1<<0) /* true: we'll support "high speed" */
#else
u32 reserved1;
#endif
/* PORTSC: offset 0x44 */
u32 port_status[0]; /* up to N_PORTS */
Furthermore, there are PORT_POWER, PORT_OWNER, PORT_LED_XXX,
PORT_TEST, PORT_WKCONN_E, PORT_WKDISC_E, PORT_WKOC_E absent in
port_status of FUSBH200. Also PORT_OC and PORT_OCC are in another
register. Is it ok to use quirk flag also?
>
> That's pretty nasty. Integrating that with the standard EHCI driver
> would be considerably more difficult.
>
> Why was the FUSBH200 designed in this strange way? Why doesn't it use
> the standard EHCI register layout? Were the engineers at Faraday
> deliberately trying to make life harder for driver writers?
>
>> Also, usbmode_ex, hostpc, and txfill_tuning other than configured_flag
>> are non-existent in fusbh200. They are used in both ehci-hcd.c and
>> ehci-hub.c for several times.
>
> They are used only if the hardware supports them, that is, only if the
> ehci->has_hostpc flag is set.
>
> Alan Stern
>
Thank you for your help.
Yuan-Hsin
On Wed, 20 Mar 2013, Yuan-Hsin Chen wrote:
> Hi,
>
> On Tue, Mar 19, 2013 at 11:48 PM, Alan Stern <[email protected]> wrote:
> > On Tue, 19 Mar 2013, Yuan-Hsin Chen wrote:
> >
> >> > What about the port_status registers? They're not between command and
> >> > async_next. If they aren't consistent with EHCI, it makes things a lot
> >> > more complicated.
> >>
> >> fusbh200 has only one port_status register with different offset,
> >> 0x30, and the position of some bits are different from EHCI.
>
> How about adding kernel configuration to adjust offset for FUSBH200 in
> ehci_def.h? So port_status would be in offset 0x20 from ehci_regs.
>
> For example,
>
> /* ASYNCLISTADDR: offset 0x18 */
> u32 async_next; /* address of next async queue head */
>
> #ifndef CONFIG_USB_EHCI_HCD_FUSBH200
> u32 reserved1[2];
>
> /* TXFILLTUNING: offset 0x24 */
> u32 txfill_tuning; /* TX FIFO Tuning register */
> #define TXFIFO_DEFAULT (8<<16) /* FIFO burst threshold 8 */
>
> u32 reserved2[6];
>
> /* CONFIGFLAG: offset 0x40 */
> u32 configured_flag;
> #define FLAG_CF (1<<0) /* true: we'll support "high speed" */
>
> #else
> u32 reserved1;
> #endif
> /* PORTSC: offset 0x44 */
> u32 port_status[0]; /* up to N_PORTS */
This is acceptable _only_ if it is not possible to use an FUSBH200
controller in the same computer as a normal EHCI controller.
> Furthermore, there are PORT_POWER, PORT_OWNER, PORT_LED_XXX,
> PORT_TEST, PORT_WKCONN_E, PORT_WKDISC_E, PORT_WKOC_E absent in
> port_status of FUSBH200. Also PORT_OC and PORT_OCC are in another
> register. Is it ok to use quirk flag also?
Yes, those can be handled by a quirk flag. Does the FUSBH200 have a
built-in Transaction Translator?
Alan Stern
Hi,
On Wed, Mar 20, 2013 at 10:26 PM, Alan Stern <[email protected]> wrote:
> On Wed, 20 Mar 2013, Yuan-Hsin Chen wrote:
>
>> Hi,
>>
>> On Tue, Mar 19, 2013 at 11:48 PM, Alan Stern <[email protected]> wrote:
>> > On Tue, 19 Mar 2013, Yuan-Hsin Chen wrote:
>> >
>> >> > What about the port_status registers? They're not between command and
>> >> > async_next. If they aren't consistent with EHCI, it makes things a lot
>> >> > more complicated.
>> >>
>> >> fusbh200 has only one port_status register with different offset,
>> >> 0x30, and the position of some bits are different from EHCI.
>>
>> How about adding kernel configuration to adjust offset for FUSBH200 in
>> ehci_def.h? So port_status would be in offset 0x20 from ehci_regs.
>>
>> For example,
>>
>> /* ASYNCLISTADDR: offset 0x18 */
>> u32 async_next; /* address of next async queue head */
>>
>> #ifndef CONFIG_USB_EHCI_HCD_FUSBH200
>> u32 reserved1[2];
>>
>> /* TXFILLTUNING: offset 0x24 */
>> u32 txfill_tuning; /* TX FIFO Tuning register */
>> #define TXFIFO_DEFAULT (8<<16) /* FIFO burst threshold 8 */
>>
>> u32 reserved2[6];
>>
>> /* CONFIGFLAG: offset 0x40 */
>> u32 configured_flag;
>> #define FLAG_CF (1<<0) /* true: we'll support "high speed" */
>>
>> #else
>> u32 reserved1;
>> #endif
>> /* PORTSC: offset 0x44 */
>> u32 port_status[0]; /* up to N_PORTS */
>
> This is acceptable _only_ if it is not possible to use an FUSBH200
> controller in the same computer as a normal EHCI controller.
>
>> Furthermore, there are PORT_POWER, PORT_OWNER, PORT_LED_XXX,
>> PORT_TEST, PORT_WKCONN_E, PORT_WKDISC_E, PORT_WKOC_E absent in
>> port_status of FUSBH200. Also PORT_OC and PORT_OCC are in another
>> register. Is it ok to use quirk flag also?
>
> Yes, those can be handled by a quirk flag. Does the FUSBH200 have a
> built-in Transaction Translator?
Yes, the FUSBH200 has built-in Transaction Translator.
I will modify the driver based on what we discussed before and re-submit it.
Thank you for your time.
>
> Alan Stern
>