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.brod...@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 <abrod...@synopsys.com> > > Cc: Stephen Warren <swar...@nvidia.com> > > Cc: Simon Glass <s...@chromium.org> > > Cc: Marek Vasut <ma...@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 <s...@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 <abrod...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot