Re: [U-Boot] [PATCH v3] usb: add support for generic EHCI devices
Hi, On 1 December 2015 at 12:07, Marek Vasut wrote: > On Tuesday, December 01, 2015 at 07:54:19 PM, Alexey Brodkin wrote: >> Hi Marek, >> >> On Mon, 2015-11-30 at 19:21 +0100, Marek Vasut wrote: >> > On Monday, November 30, 2015 at 07:13:30 PM, Alexey Brodkin wrote: >> > > Hi Marek, >> > > >> > > Please check drivers/usb/host/Kconfig. >> > >> > The order there seems correct. But how is it possible that your driver >> > triggered the build error on the ph1_sld8,ph1_sld3,ph1_ld4 boards ? I >> > suspect because it was enabled by default, but didn't "select" the >> > EHCI_HCD ? >> >> Nope EHCI_HCD gets selected, but... ehci-generic uses CONFIG_DM_USB :) >> And ehci_degister()/ehci_deregister() are only defined in EHCI_HCD >> is CONFIG_DM_USB used. >> >> So I need to add dependency on CONFIG_DM_USB for ehci_generic. >> >> Looking for other examples of dependencies on CONFIG_DM_USB I was >> surprised to see ehci-exynos, ehci-marvell, ehci-pci, ehci-sunxi and >> ehci-tegra all are not yet added in Kconfig. >> >> I.e. all those drivers are selected in headers (in include/configs/xxx.h). >> >> > > > [...] >> > > > >> > > > > +static const struct udevice_id ehci_usb_ids[] = { >> > > > > + { .compatible = "generic-ehci" }, >> > > > > + { } >> > > > > +}; >> > > > > + >> > > > > +U_BOOT_DRIVER(usb_ehci) = { >> > > > >> > > > The driver name should be ehci_generic, not usb_ehci, otherwise this >> > > > will collide with other drivers who do the same mistake. >> > > >> > > Ok but then some other drivers should be fixed as well, right? >> > >> > Yes. >> >> Ok, I'll fix it for ehci generic as well. >> >> And v4 is about to float on mailing list :) > > I see, thanks! > > I'd like to hear Simon's opinion on this. Yes, good to fix this. When CONFIG_DM_USB goes away, we can drop them all. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: add support for generic EHCI devices
On Tuesday, December 01, 2015 at 07:54:19 PM, Alexey Brodkin wrote: > Hi Marek, > > On Mon, 2015-11-30 at 19:21 +0100, Marek Vasut wrote: > > On Monday, November 30, 2015 at 07:13:30 PM, Alexey Brodkin wrote: > > > Hi Marek, > > > > > > Please check drivers/usb/host/Kconfig. > > > > The order there seems correct. But how is it possible that your driver > > triggered the build error on the ph1_sld8,ph1_sld3,ph1_ld4 boards ? I > > suspect because it was enabled by default, but didn't "select" the > > EHCI_HCD ? > > Nope EHCI_HCD gets selected, but... ehci-generic uses CONFIG_DM_USB :) > And ehci_degister()/ehci_deregister() are only defined in EHCI_HCD > is CONFIG_DM_USB used. > > So I need to add dependency on CONFIG_DM_USB for ehci_generic. > > Looking for other examples of dependencies on CONFIG_DM_USB I was > surprised to see ehci-exynos, ehci-marvell, ehci-pci, ehci-sunxi and > ehci-tegra all are not yet added in Kconfig. > > I.e. all those drivers are selected in headers (in include/configs/xxx.h). > > > > > [...] > > > > > > > > > +static const struct udevice_id ehci_usb_ids[] = { > > > > > + { .compatible = "generic-ehci" }, > > > > > + { } > > > > > +}; > > > > > + > > > > > +U_BOOT_DRIVER(usb_ehci) = { > > > > > > > > The driver name should be ehci_generic, not usb_ehci, otherwise this > > > > will collide with other drivers who do the same mistake. > > > > > > Ok but then some other drivers should be fixed as well, right? > > > > Yes. > > Ok, I'll fix it for ehci generic as well. > > And v4 is about to float on mailing list :) I see, thanks! I'd like to hear Simon's opinion on this. Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: add support for generic EHCI devices
Hi Marek, On Mon, 2015-11-30 at 19:21 +0100, Marek Vasut wrote: > On Monday, November 30, 2015 at 07:13:30 PM, Alexey Brodkin wrote: > > Hi Marek, > > Please check drivers/usb/host/Kconfig. > > The order there seems correct. But how is it possible that your driver > triggered the build error on the ph1_sld8,ph1_sld3,ph1_ld4 boards ? I > suspect because it was enabled by default, but didn't "select" the > EHCI_HCD ? Nope EHCI_HCD gets selected, but... ehci-generic uses CONFIG_DM_USB :) And ehci_degister()/ehci_deregister() are only defined in EHCI_HCD is CONFIG_DM_USB used. So I need to add dependency on CONFIG_DM_USB for ehci_generic. Looking for other examples of dependencies on CONFIG_DM_USB I was surprised to see ehci-exynos, ehci-marvell, ehci-pci, ehci-sunxi and ehci-tegra all are not yet added in Kconfig. I.e. all those drivers are selected in headers (in include/configs/xxx.h). > > > [...] > > > > > > > +static const struct udevice_id ehci_usb_ids[] = { > > > > + { .compatible = "generic-ehci" }, > > > > + { } > > > > +}; > > > > + > > > > +U_BOOT_DRIVER(usb_ehci) = { > > > > > > The driver name should be ehci_generic, not usb_ehci, otherwise this will > > > collide with other drivers who do the same mistake. > > > > Ok but then some other drivers should be fixed as well, right? > > Yes. Ok, I'll fix it for ehci generic as well. And v4 is about to float on mailing list :) -Alexey ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: add support for generic EHCI devices
On Monday, November 30, 2015 at 07:13:30 PM, Alexey Brodkin wrote: > Hi Marek, Hi! > On Mon, 2015-11-30 at 19:05 +0100, Marek Vasut wrote: > > On Monday, November 30, 2015 at 06:47:45 PM, Alexey Brodkin wrote: > > > From: Alexey Brodkin > > > > > > +config USB_EHCI_GENERIC > > > + bool "Support for generic EHCI USB controller" > > > + depends on OF_CONTROL > > > + default n > > > + ---help--- > > > + Enables support for generic EHCI controller. > > > > This should depend on EHCI_HCD somehow, no (since it's using > > ehci_deregister and friends) ? > > This symbol is in "if USB_EHCI_HCD" so if USB_EHCI_HCD is not enabled > EHCI_GENERIC won't be visible and hence USB_EHCI_HCD in defconfig. > > Otherwise we'll need to add USB_EHCI_HCD dependency for other EHCI drivers > such as USB_EHCI_MARVELL, USB_EHCI_MX6 and USB_EHCI_UNIPHIER. > Do we want to do it? :) > > Please check drivers/usb/host/Kconfig. The order there seems correct. But how is it possible that your driver triggered the build error on the ph1_sld8,ph1_sld3,ph1_ld4 boards ? I suspect because it was enabled by default, but didn't "select" the EHCI_HCD ? > > [...] > > > > > +static const struct udevice_id ehci_usb_ids[] = { > > > + { .compatible = "generic-ehci" }, > > > + { } > > > +}; > > > + > > > +U_BOOT_DRIVER(usb_ehci) = { > > > > The driver name should be ehci_generic, not usb_ehci, otherwise this will > > collide with other drivers who do the same mistake. > > Ok but then some other drivers should be fixed as well, right? Yes. > See: > --->8 > git grep U_BOOT_DRIVER drivers/usb/host/ > drivers/usb/host/dwc2.c:U_BOOT_DRIVER(usb_dwc2) = { > drivers/usb/host/ehci-exynos.c:U_BOOT_DRIVER(usb_ehci) = { CCing Lukasz > drivers/usb/host/ehci-generic.c:U_BOOT_DRIVER(usb_ehci) = { > drivers/usb/host/ehci-marvell.c:U_BOOT_DRIVER(ehci_mvebu) = { > drivers/usb/host/ehci-pci.c:U_BOOT_DRIVER(ehci_pci) = { > drivers/usb/host/ehci-sunxi.c:U_BOOT_DRIVER(usb_ehci) = { This was fixed by a patch I posted just a while ago. > drivers/usb/host/ehci-tegra.c:U_BOOT_DRIVER(usb_ehci) = { > drivers/usb/host/ohci-sunxi.c:U_BOOT_DRIVER(usb_ohci) = { CCing Hans. > drivers/usb/host/usb-sandbox.c:U_BOOT_DRIVER(usb_sandbox) = { > drivers/usb/host/usb-uclass.c:U_BOOT_DRIVER(usb_dev_generic_drv) = { > drivers/usb/host/xhci-exynos5.c:U_BOOT_DRIVER(usb_xhci) = { > --->8 > > I believe it all works because we don't enable 2 drivers at a time > [usually] :) Correct. I trapped this on sunxi just today. Propagation of this error must be stopped. > And in that light I don't see a point in having different names here. > Or you think there's a chance to have more than one USB controller enabled > simultaneously [and if it is possible at all with current implementation]? I can have ehci-pci and ehci-somethingelse enabled, so yes, the possibility is here and since Tom triggered it on sunxi, it already happens. Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: add support for generic EHCI devices
Hi Marek, On Mon, 2015-11-30 at 19:05 +0100, Marek Vasut wrote: > On Monday, November 30, 2015 at 06:47:45 PM, Alexey Brodkin wrote: > > From: Alexey Brodkin > > +config USB_EHCI_GENERIC > > + bool "Support for generic EHCI USB controller" > > + depends on OF_CONTROL > > + default n > > + ---help--- > > + Enables support for generic EHCI controller. > > This should depend on EHCI_HCD somehow, no (since it's using ehci_deregister > and > friends) ? This symbol is in "if USB_EHCI_HCD" so if USB_EHCI_HCD is not enabled EHCI_GENERIC won't be visible and hence USB_EHCI_HCD in defconfig. Otherwise we'll need to add USB_EHCI_HCD dependency for other EHCI drivers such as USB_EHCI_MARVELL, USB_EHCI_MX6 and USB_EHCI_UNIPHIER. Do we want to do it? :) Please check drivers/usb/host/Kconfig. > [...] > > > +static const struct udevice_id ehci_usb_ids[] = { > > + { .compatible = "generic-ehci" }, > > + { } > > +}; > > + > > +U_BOOT_DRIVER(usb_ehci) = { > > The driver name should be ehci_generic, not usb_ehci, otherwise this will > collide with other drivers who do the same mistake. Ok but then some other drivers should be fixed as well, right? See: --->8 git grep U_BOOT_DRIVER drivers/usb/host/ drivers/usb/host/dwc2.c:U_BOOT_DRIVER(usb_dwc2) = { drivers/usb/host/ehci-exynos.c:U_BOOT_DRIVER(usb_ehci) = { drivers/usb/host/ehci-generic.c:U_BOOT_DRIVER(usb_ehci) = { drivers/usb/host/ehci-marvell.c:U_BOOT_DRIVER(ehci_mvebu) = { drivers/usb/host/ehci-pci.c:U_BOOT_DRIVER(ehci_pci) = { drivers/usb/host/ehci-sunxi.c:U_BOOT_DRIVER(usb_ehci) = { drivers/usb/host/ehci-tegra.c:U_BOOT_DRIVER(usb_ehci) = { drivers/usb/host/ohci-sunxi.c:U_BOOT_DRIVER(usb_ohci) = { drivers/usb/host/usb-sandbox.c:U_BOOT_DRIVER(usb_sandbox) = { drivers/usb/host/usb-uclass.c:U_BOOT_DRIVER(usb_dev_generic_drv) = { drivers/usb/host/xhci-exynos5.c:U_BOOT_DRIVER(usb_xhci) = { --->8 I believe it all works because we don't enable 2 drivers at a time [usually] :) And in that light I don't see a point in having different names here. Or you think there's a chance to have more than one USB controller enabled simultaneously [and if it is possible at all with current implementation]? -Alexey ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: add support for generic EHCI devices
On Monday, November 30, 2015 at 06:47:45 PM, Alexey Brodkin wrote: > From: Alexey Brodkin > > This driver is meant to be used with any EHCI-compatible host > controller in case if there's no need for platform-specific > glue such as setup of controller or PHY's power mode via > GPIOs etc. > > Signed-off-by: Alexey Brodkin > Reviewed-by: Simon Glass > Reviewed-by: Marek Vasut > Cc: Stephen Warren > --- > > Changes compared to v2: > * Driver is disabled by default now > * Use uintptr_t instead of uint32_t for "struct ehci_hcor" >address calculation > > Changes compared to v1: > * Updated commit message with removal of Synopsys board mention > * Cleaned-up ehci_usb_remove() > > drivers/usb/host/Kconfig| 7 ++ > drivers/usb/host/Makefile | 1 + > drivers/usb/host/ehci-generic.c | 51 > + 3 files changed, 59 > insertions(+) > create mode 100644 drivers/usb/host/ehci-generic.c > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index 2a2bffe..6bb9caa 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 n > + ---help--- > + Enables support for generic EHCI controller. This should depend on EHCI_HCD somehow, no (since it's using ehci_deregister and friends) ? [...] > +static const struct udevice_id ehci_usb_ids[] = { > + { .compatible = "generic-ehci" }, > + { } > +}; > + > +U_BOOT_DRIVER(usb_ehci) = { The driver name should be ehci_generic, not usb_ehci, otherwise this will collide with other drivers who do the same mistake. > + .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, > +}; > + Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3] usb: add support for generic EHCI devices
Hi Marek, On Mon, 2015-11-30 at 20:47 +0300, Alexey Brodkin wrote: > From: Alexey Brodkin > > This driver is meant to be used with any EHCI-compatible host > controller in case if there's no need for platform-specific > glue such as setup of controller or PHY's power mode via > GPIOs etc. > > Signed-off-by: Alexey Brodkin > Reviewed-by: Simon Glass > Reviewed-by: Marek Vasut > Cc: Stephen Warren > --- > > Changes compared to v2: > * Driver is disabled by default now > * Use uintptr_t instead of uint32_t for "struct ehci_hcor" >address calculation > > Changes compared to v1: > * Updated commit message with removal of Synopsys board mention > * Cleaned-up ehci_usb_remove() git doesn't Cc people from "Reviewed-by" tags, so adding you and Simon here. Sorry for that noise. -Alexey ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3] usb: add support for generic EHCI devices
From: Alexey Brodkin This driver is meant to be used with any EHCI-compatible host controller in case if there's no need for platform-specific glue such as setup of controller or PHY's power mode via GPIOs etc. Signed-off-by: Alexey Brodkin Reviewed-by: Simon Glass Reviewed-by: Marek Vasut Cc: Stephen Warren --- Changes compared to v2: * Driver is disabled by default now * Use uintptr_t instead of uint32_t for "struct ehci_hcor" address calculation Changes compared to v1: * Updated commit message with removal of Synopsys board mention * Cleaned-up ehci_usb_remove() drivers/usb/host/Kconfig| 7 ++ drivers/usb/host/Makefile | 1 + drivers/usb/host/ehci-generic.c | 51 + 3 files changed, 59 insertions(+) create mode 100644 drivers/usb/host/ehci-generic.c diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 2a2bffe..6bb9caa 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 n + ---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 000..22e1ad0 --- /dev/null +++ b/drivers/usb/host/ehci-generic.c @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2015 Alexey Brodkin + * + * SPDX-License-Identifier:GPL-2.0+ + */ + +#include +#include +#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's 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 *)((uintptr_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) +{ + return ehci_deregister(dev); +} + +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.5.0 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot