On Tue, 14 Jul 2009 00:27:35 +0800
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, 13 Jul 2009, Alek Du wrote:
>
> > From 53ab3cbd332e22eee90a3f73de1bc5d865fd83d1 Mon Sep 17 00:00:00 2001
> > From: Alek Du <alek.du@xxxxxxxxx>
> > Date: Wed, 10 Jun 2009 14:38:15 +0800
> > Subject: [PATCH] EHCI: Add Intel Moorestown EHCI controller HOSTPCx extensions and support phy low power mode
> >
> > The Intel Moorestown EHCI controller supports non-standard HOSTPCx register
> > extension. This register controls the LPM behaviour and controls the behaviour
> > of each USB port.
>
> I just noticed this:
>
> > index eab8499..0d77974 100644
> > --- a/drivers/usb/host/ehci-hcd.c
> > +++ b/drivers/usb/host/ehci-hcd.c
> > @@ -249,6 +249,12 @@ static int ehci_reset (struct ehci_hcd *ehci)
> > retval = handshake (ehci, &ehci->regs->command,
> > CMD_RESET, 0, 250 * 1000);
> >
> > + if (ehci->has_hostpc) {
> > + ehci_writel(ehci, USBMODE_EX_HC | USBMODE_EX_VBPS,
> > + (u32 __iomem *)(((u8 *)ehci->regs) + USBMODE_EX));
> > + ehci_writel(ehci, TXFIFO_DEFAULT,
> > + (u32 __iomem *)(((u8 *)ehci->regs) + TXFILLTUNING));
> > + }
>
> It seems odd to cast between u8 * and u32 * this way. Wouldn't it be
> better if you could simply write:
>
> ehci_writel(ehci, USBMODE_EX_HC | USBMODE_EX_VBPS,
> ehci->regs + USBMODE_EX);
>
> All that's needed is a simple change to the definitions of the register
> offsets:
>
> > --- a/include/linux/usb/ehci_def.h
> > +++ b/include/linux/usb/ehci_def.h
> > @@ -132,6 +132,19 @@ struct ehci_regs {
> > #define USBMODE_CM_HC (3<<0) /* host controller mode */
> > #define USBMODE_CM_IDLE (0<<0) /* idle state */
> >
> > +/* Moorestown has some non-standard registers, partially due to the fact that
> > + * its EHCI controller has both TT and LPM support. HOSTPCx are extentions to
> > + * PORTSCx
> > + */
> > +#define HOSTPC0 0x84 /* HOSTPC extension */
> ...
> > +#define USBMODE_EX 0xc8 /* USB Device mode extension */
> ...
> > +#define TXFILLTUNING 0x24 /* TX FIFO Tuning register */
>
> Change these to (0x84/4), (0xc8/4), and (0x24/4) respectively.
>
> Alan Stern
>
Hi Alan,
Basically, this is not something new my patch introduced, I just follow the way that how USBMODE defined & used.
If I change the definition, I need to modify USBMODE too, otherwise the ehci_def.h seems odd. If we must change
this, it need another patch that fix all the registers that use this way.
/* put TDI/ARC silicon into EHCI mode */
static void tdi_reset (struct ehci_hcd *ehci)
{
u32 __iomem *reg_ptr;
u32 tmp;
reg_ptr = (u32 __iomem *)(((u8 __iomem *)ehci->regs) + USBMODE);
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
