On Wed, Apr 18, 2012 at 10:07:30PM +0200, Marek Vasut wrote:
> Dear Sascha Hauer,
>
> > On Wed, Apr 18, 2012 at 07:46:32PM +0200, Marek Vasut wrote:
> > > This driver will handle i.MX233/i.MX28 and I hope soon i.MX6Q. I tried to
> > > keep this separate from the MXC EHCI to avoid further polution of the
> > > MXC EHCI, though eventually these two might be merged.
> > >
> > > NOTE: I still haven't figured out how to enable/disable the disconnection
> > > detector, it can't be enabled all the time, so I toggle PHY stuff from
> > > this driver, which I doubt is correct.
> >
> > [...]
> >
> > > +static int ehci_mxs_drv_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct imx_usb *data = pdev->dev.platform_data;
> > > + struct usb_hcd *hcd;
> > > + struct ehci_hcd *ehci;
> > > + struct usb_phy *phy;
> > > + int ret;
> > > +
> > > + dev_info(dev, "Initializing i.MX28 USB Controller\n");
> >
> > This is not (only) i.MX28
>
> Let's keep it like that until we get further testing on other platforms.
I see no reason for doing that. It does work on all i.MX for sure.
> > > + ret = -ENODEV;
> > > + goto err_phy_init;
> > > + }
> > > +#else
> > > + dev_info(dev, "USB_MXS_PHY must have CONFIG_USB_OTG_UTILS enabled\n");
> > > + goto err_phy;
> > > +#endif
> >
> > If your code does not work without CONFIG_USB_OTG_UTILS then why doesn't
> > the driver depend on this option?
> > It doesn't matter, all code inside these ifdefs should go to imx-otg.c.
>
> Again, you mean imx-usb (the driver registering ehci host) or mxs-usb-phy?
I mean otg/imx-usb.c (and I'll stick with the name imx-otg)
> Either way, why should it be moved there, why shouldn't it sit in this file? Can
> you elaborate?
It's the imx-otg driver that manages the state. It's the imx-otg driver
which knows how to configure the phy
>
> > > +
> > > + /* Set up the PORTSCx register */
> > > + ehci_writel(ehci, 0, &ehci->regs->port_status[0]);
> > > +
> >
> > Should also be in imx-otg.c
>
> This is clearly EHCI thing, why?
This is not ehci specific. This is specific to the whole USB controller.
You have to set it up for device mode aswell, grep -i for portsc in
drivers/usb/gadget.
> > > + struct usb_phy *phy = usb_get_transceiver();
> > > +
> > > + if (phy)
> > > + usb_phy_shutdown(phy);
> > > +
> > > + usb_remove_hcd(hcd);
> >
> > usb_add_hcd is in imx-otg.c so usb_remove_hcd should be aswell.
>
> Why would that be?
Let me try to rephrase my intention:
We have to deal with a 2-in-1 device here, a usb host and a usb device
controller. Both share some resources, namely:
- the io space
- the clocks
- the interrupt
- while being a seperate device they both share the phy
Everything that is shared should be handled by imx-otg.c. Whenever
imx-otg.c detects host mode it passes control to the ehci driver. When
imx-otg.c detects that the device is unplugged then it should remove the
host via usb_remove_hcd. device mode would be similar.
For now we don't implement device mode and do not automatically detect
host/device mode which makes things simpler. Still the calls have to be
in the right place.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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
