[U-Boot] [PATCH] usb: add support for generic EHCI devices

[Alexey Brodkin <Alexey.Brodkin@synopsys.com>]

Hi Simon,

On Fri, 2015-11-13 at 19:05 -0700, Simon Glass wrote:
> Hi,
> 
> On 13 November 2015 at 11:10, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
> > Similarly to Linux kernel it's nice to have generic driver for
> > EHCI-compatible host controllers.
> > 
> > This implementation is very minimalistic and doesn't have any
> > platform-specific glue code nor phy-related operations.
> > 
> > For example this allows usage of USB-storage devices with
> > Synopsys DesignWare AXS10x boards.
> > 
> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > Cc: Stephen Warren <swarren@nvidia.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Marek Vasut <marex@denx.de>
> > ---
> >  drivers/usb/host/Kconfig        |  7 +++++
> >  drivers/usb/host/Makefile       |  1 +
> >  drivers/usb/host/ehci-generic.c | 57 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 65 insertions(+)
> >  create mode 100644 drivers/usb/host/ehci-generic.c
> > 
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Please see nits below.
> 
> > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > index 2a2bffe..a500578 100644
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -73,4 +73,11 @@ config USB_EHCI_UNIPHIER
> >         ---help---
> >           Enables support for the on-chip EHCI controller on UniPhier SoCs.
> > 
> > +config USB_EHCI_GENERIC
> > +       bool "Support for generic EHCI USB controller"
> > +       depends on OF_CONTROL
> > +       default y
> > +       ---help---
> > +         Enables support for generic EHCI controller.
> 
> such as Synopsys ...
> what does 'generic' mean?
> Please add a few more details.

Well "generic" here really means generic.
I.e. every EHCI-compatible controller that requires
no platform glue like enabling/disabling power/phy via
GPIOs etc will work perfectly fine with this driver.

So I'm not really sure what I may put here in description
that makes more sense.

Any suggestions?

> > +
> >  endif
> > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> > index f70f38c..b9b4471 100644
> > --- a/drivers/usb/host/Makefile
> > +++ b/drivers/usb/host/Makefile
> > @@ -32,6 +32,7 @@ else
> >  obj-$(CONFIG_USB_EHCI_FSL) += ehci-fsl.o
> >  endif
> >  obj-$(CONFIG_USB_EHCI_FARADAY) += ehci-faraday.o
> > +obj-$(CONFIG_USB_EHCI_GENERIC) += ehci-generic.o
> >  obj-$(CONFIG_USB_EHCI_EXYNOS) += ehci-exynos.o
> >  obj-$(CONFIG_USB_EHCI_MXC) += ehci-mxc.o
> >  obj-$(CONFIG_USB_EHCI_MXS) += ehci-mxs.o
> > diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
> > new file mode 100644
> > index 0000000..c57ddef
> > --- /dev/null
> > +++ b/drivers/usb/host/ehci-generic.c
> > @@ -0,0 +1,57 @@
> > +/*
> > + * Copyright (C) 2015 Alexey Brodkin <abrodkin@synopsys.com>
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include "ehci.h"
> > +
> > +/*
> > + * Even though here we don't explicitly use "struct ehci_ctrl"
> > + * ehci_register() expects it to be the first thing that resides in
> > + * device private data.
> 
> Yes it probably makes sense to have your own structure here rather
> than just using struct ehci_ctrl.

Not clear what do you mean.
See http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/usb/host/ehci-hcd.c#l1636
---------------->8----------------
 int ehci_register(struct udevice *dev, struct ehci_hccr *hccr,
                   struct ehci_hcor *hcor, const struct ehci_ops *ops,
                   uint tweaks, enum usb_init_type init)
 {
...
         struct ehci_ctrl *ctrl = dev_get_priv(dev);
---------------->8----------------

And dev_get_priv(), see http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/core/device.c#l378
---------------->8----------------
 void *dev_get_priv(struct udevice *dev)
 {
         if (!dev) {
                 dm_warn("%s: null device\n", __func__);
                 return NULL;
         }
 
         return dev->priv;
 }
---------------->8----------------

So "struct ehci_ctrl" must exist and be the first member of device's
private structure.

> > + */
> > +struct generic_ehci {
> > +       struct ehci_ctrl ctrl;
> > +};
> > +
> > +static int ehci_usb_probe(struct udevice *dev)
> > +{
> > +       struct ehci_hccr *hccr = (struct ehci_hccr *)dev_get_addr(dev);
> > +       struct ehci_hcor *hcor;
> > +
> > +       hcor = (struct ehci_hcor *)((uint32_t)hccr +
> > +                                   HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
> > +
> > +       return ehci_register(dev, hccr, hcor, NULL, 0, USB_INIT_HOST);
> > +}
> > +
> > +static int ehci_usb_remove(struct udevice *dev)
> > +{
> > +       int ret;
> > +
> > +       ret = ehci_deregister(dev);
> 
> Up to you, but you could use:
> 
> return ehci_deregister(dev);

True, this is a reminder of debugging stuff.
Will rework in v2.

-Alexey