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

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

[Alexey Brodkin]

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

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

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

[Marek Vasut]

On Friday, November 13, 2015 at 07:10:42 PM, Alexey Brodkin 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>

Reviewed-by: 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

I'd like to get review from Simon too on the DM part, but from my side, it's OK.

Best regards,
Marek Vasut

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

[Simon Glass]

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.

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

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

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

[Marek Vasut]

On Saturday, November 14, 2015 at 03:05:37 AM, 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 ...

Please don't add "such as FOO", it's confusing.

> what does 'generic' mean?
> Please add a few more details.

Otherwise, I agree with the rest.

Thanks!

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

[Alexey Brodkin]

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