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

[Simon Glass]

Hi,

On 13 November 2015 at 11:10, Alexey Brodkin
<Alexey.Brodkin at 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 at synopsys.com>
> Cc: Stephen Warren <swarren at nvidia.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Marek Vasut <marex at 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 at 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.

> +
>  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 at 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.

> + */
> +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);

> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id ehci_usb_ids[] = {
> +       { .compatible = "generic-ehci" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(usb_ehci) = {
> +       .name   = "ehci_generic",
> +       .id     = UCLASS_USB,
> +       .of_match = ehci_usb_ids,
> +       .probe = ehci_usb_probe,
> +       .remove = ehci_usb_remove,
> +       .ops    = &ehci_usb_ops,
> +       .priv_auto_alloc_size = sizeof(struct generic_ehci),
> +       .flags  = DM_FLAG_ALLOC_PRIV_DMA,
> +};
> +
> --
> 2.4.3
>

Regards,
Simon