Re: [PATCH v2] USB: Add MSM USB Device Controller driver

[Pavan Kondeti <pkondeti@codeaurora.org>]

Hi Igor,

On Thu, Nov 11, 2010 at 07:40:28AM +0530, Pavan Kondeti wrote:
> Hi Igor,
> 
> On Wed, Nov 10, 2010 at 08:47:16AM +0200, Igor Grinberg wrote:
> >  On 11/10/10 04:19, Pavan Kondeti wrote:
> > > On Tue, Nov 09, 2010 at 05:36:28PM +0200, Igor Grinberg wrote:
> > >>
> > >> On 11/09/10 15:52, Matthieu CASTET wrote:
> > >>> Hi,
> > >>>
> > >>> Pavankumar Kondeti a écrit :
> > >>>
> > >>>> diff --git a/drivers/usb/gadget/msm72k_udc.c b/drivers/usb/gadget/msm72k_udc.c
> > >>>> new file mode 100644
> > >>>> index 0000000..3fa4192
> > >>>> --- /dev/null
> > >>>> +++ b/drivers/usb/gadget/msm72k_udc.c
> > >>>> +
> > >>>> +static unsigned ulpi_read(struct usb_info *ui, unsigned reg)
> > >>>> +{
> > >>>> +       unsigned timeout = 100000;
> > >>>> +
> > >>>> +       /* initiate read operation */
> > >>>> +       writel(ULPI_RUN | ULPI_READ | ULPI_ADDR(reg),
> > >>>> +              USB_ULPI_VIEWPORT);
> > >>>> +
> > >>>> +       /* wait for completion */
> > >>>> +       while ((readl(USB_ULPI_VIEWPORT) & ULPI_RUN) && (--timeout))
> > >>>> +               ;
> > >>>> +
> > >>>> +       if (timeout == 0) {
> > >>>> +               dev_err(&ui->pdev->dev, "ulpi_read: timeout %08x\n",
> > >>>> +                               readl(USB_ULPI_VIEWPORT));
> > >>>> +               return 0xffffffff;
> > >>>> +       }
> > >>>> +       return ULPI_DATA_READ(readl(USB_ULPI_VIEWPORT));
> > >>>> +}
> > >>>> +
> > >>>> +static int ulpi_write(struct usb_info *ui, unsigned val, unsigned reg)
> > >>>> +{
> > >>>> +       unsigned timeout = 10000;
> > >>>> +
> > >>>> +       /* initiate write operation */
> > >>>> +       writel(ULPI_RUN | ULPI_WRITE |
> > >>>> +              ULPI_ADDR(reg) | ULPI_DATA(val),
> > >>>> +              USB_ULPI_VIEWPORT);
> > >>>> +
> > >>>> +       /* wait for completion */
> > >>>> +       while ((readl(USB_ULPI_VIEWPORT) & ULPI_RUN) && (--timeout))
> > >>>> +               ;
> > >>>> +
> > >>>> +       if (timeout == 0) {
> > >>>> +               dev_err(&ui->pdev->dev, "ulpi_write: timeout\n");
> > >>>> +               return -1;
> > >>>> +       }
> > >>>> +
> > >>>> +       return 0;
> > >>>> +}
> > >>>> +
> > >>>> +static void ulpi_init(struct usb_info *ui)
> > >>>> +{
> > >>>> +       int *seq = ui->phy_init_seq;
> > >>>> +
> > >>>> +       if (!seq)
> > >>>> +               return;
> > >>>> +
> > >>>> +       while (seq[0] >= 0) {
> > >>>> +               dev_vdbg(&ui->pdev->dev, "ulpi: write 0x%02x to 0x%02x\n",
> > >>>> +                               seq[0], seq[1]);
> > >>>> +               ulpi_write(ui, seq[0], seq[1]);
> > >>>> +               seq += 2;
> > >>>> +       }
> > >>>> +}
> > >>>> +
> > >>>
> > >>> --- /dev/null
> > >>> +++ b/drivers/usb/otg/msm72k_otg.c
> > >>>
> > >>> +
> > >>> +#define ULPI_IO_TIMEOUT_USEC   (10 * 1000)
> > >>> +static int ulpi_read(struct otg_transceiver *otg, u32 reg)
> > >>> +{
> > >>> +       int cnt = 0;
> > >>> +
> > >>> +       /* initiate read operation */
> > >>> +       writel(ULPI_RUN | ULPI_READ | ULPI_ADDR(reg),
> > >>> +              USB_ULPI_VIEWPORT);
> > >>> +
> > >>> +       /* wait for completion */
> > >>> +       while (cnt < ULPI_IO_TIMEOUT_USEC) {
> > >>> +               if (!(readl(USB_ULPI_VIEWPORT) & ULPI_RUN))
> > >>> +                       break;
> > >>> +               udelay(1);
> > >>> +               cnt++;
> > >>> +       }
> > >>> +
> > >>> +       if (cnt >= ULPI_IO_TIMEOUT_USEC) {
> > >>> +               dev_err(otg->dev, "ulpi_read: timeout %08x\n",
> > >>> +                       readl(USB_ULPI_VIEWPORT));
> > >>> +               return -ETIMEDOUT;
> > >>> +       }
> > >>> +       return ULPI_DATA_READ(readl(USB_ULPI_VIEWPORT));
> > >>> +}
> > >>> +
> > >>> +static int ulpi_write(struct otg_transceiver *otg, u32 val, u32 reg)
> > >>> +{
> > >>> +       int cnt = 0;
> > >>> +
> > >>> +       /* initiate write operation */
> > >>> +       writel(ULPI_RUN | ULPI_WRITE |
> > >>> +              ULPI_ADDR(reg) | ULPI_DATA(val),
> > >>> +              USB_ULPI_VIEWPORT);
> > >>> +
> > >>> +       /* wait for completion */
> > >>> +       while (cnt < ULPI_IO_TIMEOUT_USEC) {
> > >>> +               if (!(readl(USB_ULPI_VIEWPORT) & ULPI_RUN))
> > >>> +                       break;
> > >>> +               udelay(1);
> > >>> +               cnt++;
> > >>> +       }
> > >>> +
> > >>> +       if (cnt >= ULPI_IO_TIMEOUT_USEC) {
> > >>> +               dev_err(otg->dev, "ulpi_write: timeout\n");
> > >>> +               return -ETIMEDOUT;
> > >>> +       }
> > >>> +       return 0;
> > >>> +}
> > >>> +
> > >>> +static struct otg_io_access_ops msm_otg_io_ops = {
> > >>> +       .read = ulpi_read,
> > >>> +       .write = ulpi_write,
> > >>> +};
> > >>> +
> > >>> +static void ulpi_init(struct msm_otg *motg)
> > >>> +{
> > >>> +       struct msm_otg_platform_data *pdata = motg->pdata;
> > >>> +       int *seq = pdata->phy_init_seq;
> > >>> +
> > >>> +       if (!seq)
> > >>> +               return;
> > >>> +
> > >>> +       while (seq[0] >= 0) {
> > >>> +               dev_vdbg(motg->otg.dev, "ulpi: write 0x%02x to 0x%02x\n",
> > >>> +                               seq[0], seq[1]);
> > >>> +               ulpi_write(&motg->otg, seq[0], seq[1]);
> > >>> +               seq += 2;
> > >>> +       }
> > >>> +}
> > >>>
> > >>>
> > >>> can't you share the ulpi fonctions from udc and otg ?
> > >> The best would be to use the usb/otg/ulpi.c driver for the ulpi access.
> > >>
> > > This driver will eventually use msm72k_otg.c transceiver.
> > 
> > Well, I mean, in addition to defining the struct otg_io_access_ops in some place,
> > where both device and host drivers can share the same ops code, you can also
> > use the ulpi generic driver as the protocol driver, instead of writing private code.
> 
> I have not looked at generic ULPI driver. I will see if we can re-use it in
> our msm72k_otg.c.
> 
The generic ULPI driver seems to be supporting initializing the PHY,
configuring the PHY in Host mode and driving VBUS.

The PHY we have in our MSM chips does not have an integrated charge pump. So we
depend on external voltage regulators driver VBUS. So can not use ulpi_set_vbus
method.

IMO, ulpi_set_host is useful for only Hosts but not OTG (i.e support both gadget
and host). We kick OTG state machine only after both gadget and host are
registered. So we use our own set_host method and can not use ulpi_set_host.

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.