| Message ID | 1405005486-26355-2-git-send-email-peter.griffin@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 10 Jul 2014, Peter Griffin wrote: > This driver adds support for the USB HCD present in STi > SoC's from STMicroelectronics. It has been tested on the > stih416-b2020 board. This driver file, along with the Kconfig changes, belongs in the arch/platform-specific source directory. Not in drivers/usb/host/. It is, after all, a platform driver that registers two platform devices. > +++ b/drivers/usb/host/Kconfig > @@ -753,6 +753,23 @@ config USB_HCD_SSB > > If unsure, say N. > > +config USB_HCD_ST > + tristate "STMicroelectronics STi family HCD support" Why does this need to be tristate? Why not always build it into the kernel? Or at least make it boolean rather than tristate. > + depends on ARCH_STI > + select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD > + select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD > + select MFD_SYSCON > + select GENERIC_PHY > + help > + Enable support for the EHCI and OCHI host controller on ST s/OCHI/OHCI/ > + consumer electronics SoCs. > + > + It converts the ST driver into two platform device drivers It converts the ST driver? That doesn't sound right at all. You could eliminate this paragraph completely and nobody would miss it. > + for EHCI and OHCI and provides bus configuration, clock and power > + management for the combined hardware block. Alan Stern
Hi Alan, Thanks for reviewing. On Thu, 10 Jul 2014, Alan Stern wrote: > On Thu, 10 Jul 2014, Peter Griffin wrote: > > > This driver adds support for the USB HCD present in STi > > SoC's from STMicroelectronics. It has been tested on the > > stih416-b2020 board. > > This driver file, along with the Kconfig changes, belongs in the > arch/platform-specific source directory. Not in drivers/usb/host/. > It is, after all, a platform driver that registers two platform > devices. Why do think that? AFAIK certainly for ARM we are trying NOT to add code under the arch/platform directorys and get the code into the relevant subsystems. In order to prove the above if you look in drivers/usb/host/ there are many other similar platform drivers for other SoC families: - bcma-hcd.c ehci-atmel.c ssb-hcd.c ehci-exynos.c ehci-fsl.c ehci-mxc.c ehci-omap.c ehci-orion.c ohci-nxp.c and more, but you get the idea. In short I believe this to be the correct directory :-) > > > +++ b/drivers/usb/host/Kconfig > > @@ -753,6 +753,23 @@ config USB_HCD_SSB > > > > If unsure, say N. > > > > +config USB_HCD_ST > > + tristate "STMicroelectronics STi family HCD support" > > Why does this need to be tristate? Why not always build it into the > kernel? Or at least make it boolean rather than tristate. Building as a module is useful on these embedded SoCs usually as a mechanism for speeding up boot time. Anything which isn't critical to getting the core functionality of the device going (in this case decoding TV and outputting on HDMI), can be deffered to a later point. Things like USB drivers, can then be loaded in after that point (deffered module loading), to give the appearence of 'instant on' (which in consumer electronics is what everyone wishes to achieve). Going back to my examples above, most of these platforms are also defined as tristate. > > > + depends on ARCH_STI > > + select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD > > + select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD > > + select MFD_SYSCON > > + select GENERIC_PHY > > + help > > + Enable support for the EHCI and OCHI host controller on ST > > s/OCHI/OHCI/ Good spot, will fix in V2 > > > + consumer electronics SoCs. > > + > > + It converts the ST driver into two platform device drivers > > It converts the ST driver? That doesn't sound right at all. You could > eliminate this paragraph completely and nobody would miss it. I agree the wording is a little off there, I can remove or re-phrase it. What it is trying to express is that it is slightly different to some other platform drivers, in that it creates two platform drivers one for the ehci controller and the other for the OHCI controller. From looking at other drivers in this directory this driver is quite similar to USB_HCD_BCMA, which also deemed it noteworthy to mention in the help, although phrased somewhat more succinctly. regards, Peter.
On Thu, 10 Jul 2014, Peter Griffin wrote: > This driver adds support for the USB HCD present in STi > SoC's from STMicroelectronics. It has been tested on the > stih416-b2020 board. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > drivers/usb/host/Kconfig | 17 ++ > drivers/usb/host/Makefile | 1 + > drivers/usb/host/st-hcd.c | 471 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 489 insertions(+) > create mode 100644 drivers/usb/host/st-hcd.c > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index 61b7817..dc0358e 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -753,6 +753,23 @@ config USB_HCD_SSB > > If unsure, say N. > > +config USB_HCD_ST > + tristate "STMicroelectronics STi family HCD support" > + depends on ARCH_STI > + select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD > + select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD > + select MFD_SYSCON Are you still using Syscon? If not, this and the header file can be removed. > + select GENERIC_PHY > + help > + Enable support for the EHCI and OCHI host controller on ST > + consumer electronics SoCs. > + > + It converts the ST driver into two platform device drivers > + for EHCI and OHCI and provides bus configuration, clock and power > + management for the combined hardware block. > + > + If unsure, say N. > + > config USB_HCD_TEST_MODE > bool "HCD test mode support" > ---help--- > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile > index af89a90..af0b81d 100644 > --- a/drivers/usb/host/Makefile > +++ b/drivers/usb/host/Makefile > @@ -74,3 +74,4 @@ obj-$(CONFIG_USB_HCD_SSB) += ssb-hcd.o > obj-$(CONFIG_USB_FUSBH200_HCD) += fusbh200-hcd.o > obj-$(CONFIG_USB_FOTG210_HCD) += fotg210-hcd.o > obj-$(CONFIG_USB_MAX3421_HCD) += max3421-hcd.o > +obj-$(CONFIG_USB_HCD_ST) += st-hcd.o > diff --git a/drivers/usb/host/st-hcd.c b/drivers/usb/host/st-hcd.c > new file mode 100644 > index 0000000..8a9636c > --- /dev/null > +++ b/drivers/usb/host/st-hcd.c > @@ -0,0 +1,471 @@ > +/* > + * STMicroelectronics HCD (Host Controller Driver) for USB 2.0 and 1.1. > + * > + * Copyright (c) 2013 STMicroelectronics (R&D) Ltd. > + * Authors: Stephen Gallimore <stephen.gallimore@st.com> > + * Peter Griffin <peter.griffin@linaro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2, as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/dma-mapping.h> > +#include <linux/of.h> > +#include <linux/pm_runtime.h> > +#include <linux/pm_clock.h> > +#include <linux/delay.h> > +#include <linux/usb.h> > +#include <linux/usb/hcd.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/reset.h> > +#include <linux/regmap.h> Is this used? > +#include <linux/phy/phy.h> > +#include <linux/mfd/syscon.h> Is this used? > +#include <linux/usb/ohci_pdriver.h> > +#include <linux/usb/ehci_pdriver.h> > + > +#include "ohci.h" > + > +#define EHCI_CAPS_SIZE 0x10 > +#define AHB2STBUS_INSREG01 (EHCI_CAPS_SIZE + 0x84) > + > +struct st_hcd_dev { > + int port_nr; > + struct platform_device *ehci_device; > + struct platform_device *ohci_device; > + struct clk *ic_clk; /* Interconnect clock to the controller block */ > + struct clk *ohci_clk; /* 48MHz clock for OHCI */ > + struct reset_control *pwr; > + struct reset_control *rst; > + struct phy *phy; > +}; As there are comments, consider using kerneldoc instead. > +static inline void st_ehci_configure_bus(void __iomem *regs) > +{ > + /* Set EHCI packet buffer IN/OUT threshold to 128 bytes */ > + u32 threshold = 128 | (128 << 16); > + > + writel(threshold, regs + AHB2STBUS_INSREG01); > +} > + > +static int st_hcd_enable_clocks(struct device *dev, > + struct st_hcd_dev *hcd_dev) > +{ > + int err; Separate code (and comments) from declaration. > + /* > + * The interconnect input clock have either fixed s/have either/has either a/ > + * rate or the rate is defined on boot, so we are only concerned about > + * enabling any gates for this clock. > + */ > + err = clk_prepare_enable(hcd_dev->ic_clk); > + if (err) { > + dev_err(dev, "can't enable ic clock\n"); > + return err; > + } Nit: '\n' > + /* > + * The 48MHz OHCI clock is usually provided by a programmable > + * frequency synthesizer, which is often not programmed on boot/chip > + * reset, so we set its rate here to ensure it is correct. > + * > + * However not all SoC's have a dedicated ohci clock so it isn't fatal s/ohci/OHCI > + * for this not to exist. --^ > + */ > + if (!IS_ERR(hcd_dev->ohci_clk)) { This is ugly. If it's not found NULL it, then check for: if (hcd_dev->ohci_clk) { Or, even better, do: if (hcd_dev->ohci_clk) return 0; Then de-indent this: > + err = clk_set_rate(hcd_dev->ohci_clk, 48000000); > + if (err) { > + dev_err(dev, "can't set ohci_clk rate\n"); > + goto error; > + } > + > + err = clk_prepare_enable(hcd_dev->ohci_clk); > + if (err) { > + dev_err(dev, "can't enable ohci_clk\n"); > + goto error; > + } > + } > + > + return 0; > +error: > + clk_disable_unprepare(hcd_dev->ic_clk); > + return err; > +} > + > + Remove the superfluous '\n'. > +static void st_hcd_disable_clocks(struct st_hcd_dev *hcd_dev) > +{ > + /* not all SoCs provide a dedicated ohci_clk */ Really small nit: All but two of the comments in this file start with uppercase. > + if (!IS_ERR(hcd_dev->ohci_clk)) You don't need to worry about this here. The clk framework already does this check for you, so you can omit it. > + clk_disable_unprepare(hcd_dev->ohci_clk); > + > + clk_disable_unprepare(hcd_dev->ic_clk); > +} > + > +static void st_hcd_assert_resets(struct device *dev, > + struct st_hcd_dev *hcd_dev) > +{ > + int err; > + > + err = reset_control_assert(hcd_dev->pwr); > + if (err) > + dev_err(dev, "unable to put into powerdown\n"); > + > + err = reset_control_assert(hcd_dev->rst); > + if (err) > + dev_err(dev, "unable to put into softreset\n"); > +} > + > +static int st_hcd_deassert_resets(struct device *dev, > + struct st_hcd_dev *hcd_dev) > +{ > + int err; > + > + err = reset_control_deassert(hcd_dev->pwr); > + if (err) { > + dev_err(dev, "unable to bring out of powerdown\n"); > + return err; > + } > + > + err = reset_control_deassert(hcd_dev->rst); > + if (err) { > + dev_err(dev, "unable to bring out of softreset\n"); > + goto err_assert_power; > + } > + > + return 0; > + > +err_assert_power: > + reset_control_assert(hcd_dev->pwr); I would put this in reset_control_deassert()'s error path and rid the goto. > + return err; > +} > + > +static const struct usb_ehci_pdata ehci_pdata = { > +}; > + > +static const struct usb_ohci_pdata ohci_pdata = { > +}; > + > +static int st_hcd_remove(struct platform_device *pdev) .remove() usually goes below (or at least near) .probe(). > +{ > + struct st_hcd_dev *hcd_dev = platform_get_drvdata(pdev); > + > + platform_device_unregister(hcd_dev->ehci_device); > + platform_device_unregister(hcd_dev->ohci_device); > + > + phy_power_off(hcd_dev->phy); > + > + phy_exit(hcd_dev->phy); > + > + st_hcd_assert_resets(&pdev->dev, hcd_dev); > + > + st_hcd_disable_clocks(hcd_dev); > + > + return 0; > +} > + > +static struct platform_device *st_hcd_device_create(const char *name, int id, > + struct platform_device *parent) > +{ > + struct platform_device *pdev; > + const char *platform_name; > + struct resource *res; > + struct resource hcd_res[2]; > + int ret; > + > + res = platform_get_resource_byname(parent, IORESOURCE_MEM, name); > + if (!res) > + return ERR_PTR(-ENODEV); > + > + hcd_res[0] = *res; > + > + res = platform_get_resource_byname(parent, IORESOURCE_IRQ, name); > + if (!res) > + return ERR_PTR(-ENODEV); > + > + hcd_res[1] = *res; > + > + platform_name = kasprintf(GFP_KERNEL, "%s-platform", name); > + if (!platform_name) > + return ERR_PTR(-ENOMEM); > + > + pdev = platform_device_alloc(platform_name, id); > + > + kfree(platform_name); > + > + if (!pdev) > + return ERR_PTR(-ENOMEM); > + > + pdev->dev.parent = &parent->dev; > + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; > + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > + > + ret = platform_device_add_resources(pdev, hcd_res, ARRAY_SIZE(hcd_res)); > + if (ret) > + goto error; > + > + if (!strcmp(name, "ohci")) > + ret = platform_device_add_data(pdev, &ohci_pdata, > + sizeof(ohci_pdata)); > + else > + ret = platform_device_add_data(pdev, &ehci_pdata, > + sizeof(ehci_pdata)); > + > + if (ret) > + goto error; > + > + ret = platform_device_add(pdev); > + if (ret) > + goto error; > + > + return pdev; > + > +error: > + platform_device_put(pdev); > + return ERR_PTR(ret); > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int st_hcd_resume(struct device *dev) > +{ > + struct st_hcd_dev *hcd_dev = dev_get_drvdata(dev); > + struct usb_hcd *ehci_hcd = platform_get_drvdata(hcd_dev->ehci_device); > + int err; > + > + pinctrl_pm_select_default_state(dev); > + > + err = st_hcd_enable_clocks(dev, hcd_dev); > + if (err) > + return err; > + > + err = phy_init(hcd_dev->phy); > + if (err) { > + dev_err(dev, "phy initialization failed\n"); > + goto err_disable_clocks; > + } > + > + err = phy_power_on(hcd_dev->phy); > + if (err && (err != -ENOTSUPP)) { > + dev_err(dev, "phy power on failed\n"); > + goto err_phy_exit; > + } > + > + err = st_hcd_deassert_resets(dev, hcd_dev); > + if (err) > + goto err_phy_off; > + > + st_ehci_configure_bus(ehci_hcd->regs); > + > + return 0; > + > +err_phy_off: > + phy_power_off(hcd_dev->phy); > +err_phy_exit: > + phy_exit(hcd_dev->phy); > +err_disable_clocks: > + st_hcd_disable_clocks(hcd_dev); > + > + return err; > +} > + > +static int st_hcd_suspend(struct device *dev) > +{ > + struct st_hcd_dev *hcd_dev = dev_get_drvdata(dev); > + int err; > + > + err = reset_control_assert(hcd_dev->pwr); > + if (err) { > + dev_err(dev, "unable to put into powerdown\n"); > + return err; > + } > + > + err = reset_control_assert(hcd_dev->rst); > + if (err) { > + dev_err(dev, "unable to put into softreset\n"); > + return err; > + } > + > + err = phy_power_off(hcd_dev->phy); > + if (err && (err != -ENOTSUPP)) { > + dev_err(dev, "phy power off failed\n"); > + return err; > + } > + > + phy_exit(hcd_dev->phy); > + > + st_hcd_disable_clocks(hcd_dev); > + > + pinctrl_pm_select_sleep_state(dev); > + > + return 0; > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(st_hcd_pm, st_hcd_suspend, st_hcd_resume); > + > +static struct of_device_id st_hcd_match[] = { const? > + { .compatible = "st,usb-300x" }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, st_hcd_match); Why is this all the way up here? As you're not using of_match_device() you can place this down by the platform driver struct. > +static int st_hcd_probe_clocks(struct device *dev, > + struct st_hcd_dev *hcd_dev) > +{ > + hcd_dev->ic_clk = devm_clk_get(dev, "ic"); > + if (IS_ERR(hcd_dev->ic_clk)) { > + dev_err(dev, "ic clk not found\n"); > + return PTR_ERR(hcd_dev->ic_clk); > + } > + > + /* some SoCs don't have a dedicated ohci clk */ Really small nit: All but two of the comments in this file start with uppercase. > + hcd_dev->ohci_clk = devm_clk_get(dev, "ohci"); > + if (IS_ERR(hcd_dev->ohci_clk)) > + dev_info(dev, "48MHz ohci clk not found\n"); s/ohci/OHCI Also just be aware that some maintainers like s/clk/clock in error messages. > + return st_hcd_enable_clocks(dev, hcd_dev); > +} > + > + --^ > +static int st_hcd_probe_resets(struct device *dev, > + struct st_hcd_dev *hcd_dev) > +{ > + hcd_dev->pwr = devm_reset_control_get(dev, "power"); > + if (IS_ERR(hcd_dev->pwr)) { > + dev_err(dev, "power reset control not found\n"); > + return PTR_ERR(hcd_dev->pwr); > + } > + > + hcd_dev->rst = devm_reset_control_get(dev, "softreset"); > + if (IS_ERR(hcd_dev->rst)) { > + dev_err(dev, "soft reset control not found\n"); > + return PTR_ERR(hcd_dev->rst); > + } > + > + return st_hcd_deassert_resets(dev, hcd_dev); > +} > + > +static int st_hcd_probe_ehci_setup(struct platform_device *pdev) > +{ > + struct resource *res; > + void __iomem *ehci_regs; > + > + /* > + * We need to do some integration specific setup in the EHCI > + * controller, which the EHCI platform driver does not provide any > + * hooks to allow us to do during it's initialisation. Nit: "Its" can not be expressed as possessive. > + */ > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ehci"); > + if (!res) > + return -ENODEV; > + > + ehci_regs = devm_ioremap(&pdev->dev, res->start, resource_size(res)); Use devm_ioremap_resource() > + if (!ehci_regs) > + return -ENOMEM; > + > + st_ehci_configure_bus(ehci_regs); > + devm_iounmap(&pdev->dev, ehci_regs); > + > + return 0; > +} > + > +static int st_hcd_probe(struct platform_device *pdev) > +{ > + struct st_hcd_dev *hcd_dev; > + int id; > + int err; > + > + if (!pdev->dev.of_node) > + return -ENODEV; No need for this if you depend on OF. > + id = of_alias_get_id(pdev->dev.of_node, "usb"); > + Remove the '\n' between get and check. > + if (id < 0) { > + dev_err(&pdev->dev, "No ID specified via DT alias\n"); > + return -ENODEV; > + } > + > + hcd_dev = devm_kzalloc(&pdev->dev, sizeof(*hcd_dev), GFP_KERNEL); > + if (!hcd_dev) > + return -ENOMEM; > + > + hcd_dev->port_nr = id; > + > + err = st_hcd_probe_clocks(&pdev->dev, hcd_dev); > + if (err) > + return err; > + > + err = st_hcd_probe_resets(&pdev->dev, hcd_dev); > + if (err) > + goto err_disable_clocks; > + > + err = st_hcd_probe_ehci_setup(pdev); > + if (err) > + goto err_assert_resets; > + > + hcd_dev->phy = devm_phy_get(&pdev->dev, "usb2-phy"); > + if (IS_ERR(hcd_dev->phy)) { > + dev_err(&pdev->dev, "no PHY configured\n"); > + err = PTR_ERR(hcd_dev->phy); > + goto err_assert_resets; > + } > + > + err = phy_init(hcd_dev->phy); > + if (err) { > + dev_err(&pdev->dev, "phy initialization failed\n"); > + goto err_assert_resets; > + } > + > + err = phy_power_on(hcd_dev->phy); > + if (err && (err != -ENOTSUPP)) { > + dev_err(&pdev->dev, "phy power on failed\n"); > + goto err_phy_exit; > + } > + > + hcd_dev->ehci_device = st_hcd_device_create("ehci", id, pdev); > + if (IS_ERR(hcd_dev->ehci_device)) { > + err = PTR_ERR(hcd_dev->ehci_device); > + goto err_phy_off; > + } > + > + hcd_dev->ohci_device = st_hcd_device_create("ohci", id, pdev); > + if (IS_ERR(hcd_dev->ohci_device)) { > + platform_device_del(hcd_dev->ehci_device); Why do you break convention here? Place this down in amongst the gotos. > + err = PTR_ERR(hcd_dev->ohci_device); > + goto err_phy_off; > + } > + > + platform_set_drvdata(pdev, hcd_dev); > + > + return 0; > + > +err_phy_off: > + phy_power_off(hcd_dev->phy); > +err_phy_exit: > + phy_exit(hcd_dev->phy); > +err_assert_resets: > + st_hcd_assert_resets(&pdev->dev, hcd_dev); > +err_disable_clocks: > + st_hcd_disable_clocks(hcd_dev); > + > + return err; > +} > + > +static struct platform_driver st_hcd_driver = { > + .probe = st_hcd_probe, > + .remove = st_hcd_remove, > + .driver = { > + .name = "st-hcd", > + .pm = &st_hcd_pm, > + .of_match_table = st_hcd_match, > + }, > +}; > + > +module_platform_driver(st_hcd_driver); > + > +MODULE_DESCRIPTION("STMicroelectronics On-Chip USB Host Controller"); > +MODULE_AUTHOR("Stephen Gallimore <stephen.gallimore@st.com>"); > +MODULE_LICENSE("GPL v2");
On Fri, 11 Jul 2014, Peter Griffin wrote: > Hi Alan, > > Thanks for reviewing. > > On Thu, 10 Jul 2014, Alan Stern wrote: > > > On Thu, 10 Jul 2014, Peter Griffin wrote: > > > > > This driver adds support for the USB HCD present in STi > > > SoC's from STMicroelectronics. It has been tested on the > > > stih416-b2020 board. > > > > This driver file, along with the Kconfig changes, belongs in the > > arch/platform-specific source directory. Not in drivers/usb/host/. > > It is, after all, a platform driver that registers two platform > > devices. > > Why do think that? Because it is true. One can easily see that st-hcd.c is a platform driver: It contains a module_platform_driver() line and a struct platform_driver definition near the end. And it registers a platform device by calling platform_device_add() in st_hcd_device_create(), which is called twice by st_hcd_probe(). > AFAIK certainly for ARM we are trying NOT to add code > under the arch/platform directorys and get the code into the relevant subsystems. This does not agree with my experiences. > In order to prove the above if you look in drivers/usb/host/ there are many other > similar platform drivers for other SoC families: - > bcma-hcd.c > ehci-atmel.c > ssb-hcd.c > ehci-exynos.c > ehci-fsl.c > ehci-mxc.c > ehci-omap.c > ehci-orion.c > ohci-nxp.c > and more, but you get the idea. In short I believe this to be the > correct directory :-) No. bcma-hcd.c and ssb-hcd.c are similar to your st-hcd.c, but the others aren't. Take ehci-atmel.c as a typical example. It does not define any ehci_pdata structure and does not call platform_device_add(); instead it calls usb_add_hcd(). The others work the same way. I would suggest that bcma-hcd.c and ssb-hcd.c don't belong in drivers/usb/host either. For examples of drivers that _do_ resemble st-hcd.c, look in: arch/arm/mach-cns3xxx/cns3420vb.c arch/arm/mach-cns3xxx/core.c arch/arm/mach-shmobile/setup-r8a7778.c arch/arm/mach-shmobile/setup-r8a7779.c arch/arm/mach-omap2/board-cm-t3517.c arch/mips/netlogic/xlr/platform.c arch/mips/ath79/dev-usb.c arch/mips/loongson1/common/platform.c arch/mips/alchemy/common/platform.c These files all define ehci_pdata structures and register platform devices. And they obviously are all located in arch/platform-specific directories. > > > +++ b/drivers/usb/host/Kconfig > > > @@ -753,6 +753,23 @@ config USB_HCD_SSB > > > > > > If unsure, say N. > > > > > > +config USB_HCD_ST > > > + tristate "STMicroelectronics STi family HCD support" > > > > Why does this need to be tristate? Why not always build it into the > > kernel? Or at least make it boolean rather than tristate. > > Building as a module is useful on these embedded SoCs usually as a mechanism for speeding up boot time. > Anything which isn't critical to getting the core functionality of the device going (in this case decoding > TV and outputting on HDMI), can be deffered to a later point. Things like USB drivers, can then be loaded in > after that point (deffered module loading), to give the appearence of 'instant on' (which in consumer electronics > is what everyone wishes to achieve). But st-hcd.c takes very little time to run. ehci-platform will cause a delay, so it makes sense for ehci-platform to be a module. But there's no reason for st-hcd to be a module. > Going back to my examples above, most of these platforms are also defined as tristate. Probably for historical reasons. They don't need to be tristate now. > > > + consumer electronics SoCs. > > > + > > > + It converts the ST driver into two platform device drivers > > > > It converts the ST driver? That doesn't sound right at all. You could > > eliminate this paragraph completely and nobody would miss it. > > I agree the wording is a little off there, I can remove or re-phrase it. What it is trying to express > is that it is slightly different to some other platform drivers, in that it creates two platform drivers one It creates two platform _devices_, not two platform _drivers_. > for the ehci controller and the other for the OHCI controller. In that respect it is very similar to the drivers I listed above. > From looking at other drivers in this directory > this driver is quite similar to USB_HCD_BCMA, which also deemed it noteworthy to mention in the help, although > phrased somewhat more succinctly. People reading the Kconfig help text don't care what other drivers are similar to yours. All they want to know is whether or not they should enable your driver. Alan Stern
On Fri, 11 Jul 2014, Alan Stern wrote: > On Fri, 11 Jul 2014, Peter Griffin wrote: > > On Thu, 10 Jul 2014, Alan Stern wrote: > > > > > On Thu, 10 Jul 2014, Peter Griffin wrote: > > > > > > > This driver adds support for the USB HCD present in STi > > > > SoC's from STMicroelectronics. It has been tested on the > > > > stih416-b2020 board. > > > > > > This driver file, along with the Kconfig changes, belongs in the > > > arch/platform-specific source directory. Not in drivers/usb/host/. > > > It is, after all, a platform driver that registers two platform > > > devices. > > > > Why do think that? > > Because it is true. One can easily see that st-hcd.c is a platform > driver: It contains a module_platform_driver() line and a struct > platform_driver definition near the end. And it registers a platform > device by calling platform_device_add() in st_hcd_device_create(), > which is called twice by st_hcd_probe(). You are correct that this is indeed a platform driver and in the 'old days' these would have been stuffed into the ARM sub-architecture directories. However, these became far too bloated and created too much churn which angered Linus. A great deal has changed since his ARM rant [1]. All drivers (platform or otherwise) are now to live in 'drivers', which makes a great deal of sense really, doesn't it? Did you grep kernel wide for module_platform_driver()? $ git grep module_platform_driver -- arch/ | wc -l 12 $ git grep module_platform_driver -- drivers/ | wc -l 1138 Most platform drivers have already been moved. > > AFAIK certainly for ARM we are trying NOT to add code > > under the arch/platform directorys and get the code into the relevant subsystems. > > This does not agree with my experiences. Then you haven't been following what's been going on for the past 3 years or so. The majority of platform drivers have been moved out to drivers. Hence the creation of some fantastic new subsystems, such as clk and pinctrl, to name but two. These (and others) were realised as part of a push to consolidate and remove all driver code out of the arch directories. > > In order to prove the above if you look in drivers/usb/host/ there are many other > > similar platform drivers for other SoC families: - > > bcma-hcd.c > > ehci-atmel.c > > ssb-hcd.c > > ehci-exynos.c > > ehci-fsl.c > > ehci-mxc.c > > ehci-omap.c > > ehci-orion.c > > ohci-nxp.c > > and more, but you get the idea. In short I believe this to be the > > correct directory :-) > > No. bcma-hcd.c and ssb-hcd.c are similar to your st-hcd.c, but the > others aren't. > > Take ehci-atmel.c as a typical example. It does not define any > ehci_pdata structure and does not call platform_device_add(); instead > it calls usb_add_hcd(). The others work the same way. I would suggest > that bcma-hcd.c and ssb-hcd.c don't belong in drivers/usb/host either. > > For examples of drivers that _do_ resemble st-hcd.c, look in: > > arch/arm/mach-cns3xxx/cns3420vb.c > arch/arm/mach-cns3xxx/core.c > arch/arm/mach-shmobile/setup-r8a7778.c > arch/arm/mach-shmobile/setup-r8a7779.c > arch/arm/mach-omap2/board-cm-t3517.c > arch/mips/netlogic/xlr/platform.c > arch/mips/ath79/dev-usb.c > arch/mips/loongson1/common/platform.c > arch/mips/alchemy/common/platform.c > > These files all define ehci_pdata structures and register platform > devices. And they obviously are all located in arch/platform-specific > directories. Right, these are all 'old' platforms. There is no way these drivers would be accepted into the architecture directories now days. Drivers live in 'drivers'. New platforms also have to have Device Tree support, so even platform data (which I agree should live in arch/arm) doesn't live in arch/arm/{mach,plat}-* anymore. For many of the more modern architectures the aforementioned directories are bare. > > > > +++ b/drivers/usb/host/Kconfig > > > > @@ -753,6 +753,23 @@ config USB_HCD_SSB > > > > > > > > If unsure, say N. > > > > > > > > +config USB_HCD_ST > > > > + tristate "STMicroelectronics STi family HCD support" > > > > > > Why does this need to be tristate? Why not always build it into the > > > kernel? Or at least make it boolean rather than tristate. > > > > Building as a module is useful on these embedded SoCs usually as a mechanism for speeding up boot time. > > Anything which isn't critical to getting the core functionality of the device going (in this case decoding > > TV and outputting on HDMI), can be deffered to a later point. Things like USB drivers, can then be loaded in > > after that point (deffered module loading), to give the appearence of 'instant on' (which in consumer electronics > > is what everyone wishes to achieve). > > But st-hcd.c takes very little time to run. ehci-platform will cause a > delay, so it makes sense for ehci-platform to be a module. But there's > no reason for st-hcd to be a module. > > > Going back to my examples above, most of these platforms are also defined as tristate. > > Probably for historical reasons. They don't need to be tristate now. I'm not going to get into this too much, but it is _my oppinion_ that if a driver can be a module, the option for it to be a module should exist. [...] [1] https://lkml.org/lkml/2011/3/17/492
On Mon, 14 Jul 2014, Lee Jones wrote: > On Fri, 11 Jul 2014, Alan Stern wrote: > > On Fri, 11 Jul 2014, Peter Griffin wrote: > > > On Thu, 10 Jul 2014, Alan Stern wrote: > > > > > > > On Thu, 10 Jul 2014, Peter Griffin wrote: > > > > > > > > > This driver adds support for the USB HCD present in STi > > > > > SoC's from STMicroelectronics. It has been tested on the > > > > > stih416-b2020 board. > > > > > > > > This driver file, along with the Kconfig changes, belongs in the > > > > arch/platform-specific source directory. Not in drivers/usb/host/. > > > > It is, after all, a platform driver that registers two platform > > > > devices. > > > > > > Why do think that? > > > > Because it is true. One can easily see that st-hcd.c is a platform > > driver: It contains a module_platform_driver() line and a struct > > platform_driver definition near the end. And it registers a platform > > device by calling platform_device_add() in st_hcd_device_create(), > > which is called twice by st_hcd_probe(). > > You are correct that this is indeed a platform driver and in the 'old > days' these would have been stuffed into the ARM sub-architecture > directories. However, these became far too bloated and created too > much churn which angered Linus. A great deal has changed since his > ARM rant [1]. All drivers (platform or otherwise) are now to live in > 'drivers', which makes a great deal of sense really, doesn't it? > > Did you grep kernel wide for module_platform_driver()? > > $ git grep module_platform_driver -- arch/ | wc -l > 12 > > $ git grep module_platform_driver -- drivers/ | wc -l > 1138 > > Most platform drivers have already been moved. Okay, I grant the point. Objections withdrawn. Fix up the spelling error and the other stuff in the Kconfig help text, and this will be acceptable. Alan Stern
Hi Alan, > > > > Most platform drivers have already been moved. > > Okay, I grant the point. Objections withdrawn. Fix up the spelling > error and the other stuff in the Kconfig help text, and this will be > acceptable. Thanks, I intend to send a new version shortly. regards, Peter.
Hi Lee, Thanks for reviewing. All your review comments have been fixed in v2. regards, Peter.
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 61b7817..dc0358e 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -753,6 +753,23 @@ config USB_HCD_SSB If unsure, say N. +config USB_HCD_ST + tristate "STMicroelectronics STi family HCD support" + depends on ARCH_STI + select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD + select USB_EHCI_HCD_PLATFORM if USB_EHCI_HCD + select MFD_SYSCON + select GENERIC_PHY + help + Enable support for the EHCI and OCHI host controller on ST + consumer electronics SoCs. + + It converts the ST driver into two platform device drivers + for EHCI and OHCI and provides bus configuration, clock and power + management for the combined hardware block. + + If unsure, say N. + config USB_HCD_TEST_MODE bool "HCD test mode support" ---help--- diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index af89a90..af0b81d 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -74,3 +74,4 @@ obj-$(CONFIG_USB_HCD_SSB) += ssb-hcd.o obj-$(CONFIG_USB_FUSBH200_HCD) += fusbh200-hcd.o obj-$(CONFIG_USB_FOTG210_HCD) += fotg210-hcd.o obj-$(CONFIG_USB_MAX3421_HCD) += max3421-hcd.o +obj-$(CONFIG_USB_HCD_ST) += st-hcd.o diff --git a/drivers/usb/host/st-hcd.c b/drivers/usb/host/st-hcd.c new file mode 100644 index 0000000..8a9636c --- /dev/null +++ b/drivers/usb/host/st-hcd.c @@ -0,0 +1,471 @@ +/* + * STMicroelectronics HCD (Host Controller Driver) for USB 2.0 and 1.1. + * + * Copyright (c) 2013 STMicroelectronics (R&D) Ltd. + * Authors: Stephen Gallimore <stephen.gallimore@st.com> + * Peter Griffin <peter.griffin@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2, as + * published by the Free Software Foundation. + */ + +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/dma-mapping.h> +#include <linux/of.h> +#include <linux/pm_runtime.h> +#include <linux/pm_clock.h> +#include <linux/delay.h> +#include <linux/usb.h> +#include <linux/usb/hcd.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/reset.h> +#include <linux/regmap.h> +#include <linux/phy/phy.h> +#include <linux/mfd/syscon.h> +#include <linux/usb/ohci_pdriver.h> +#include <linux/usb/ehci_pdriver.h> + +#include "ohci.h" + +#define EHCI_CAPS_SIZE 0x10 +#define AHB2STBUS_INSREG01 (EHCI_CAPS_SIZE + 0x84) + +struct st_hcd_dev { + int port_nr; + struct platform_device *ehci_device; + struct platform_device *ohci_device; + struct clk *ic_clk; /* Interconnect clock to the controller block */ + struct clk *ohci_clk; /* 48MHz clock for OHCI */ + struct reset_control *pwr; + struct reset_control *rst; + struct phy *phy; +}; + +static inline void st_ehci_configure_bus(void __iomem *regs) +{ + /* Set EHCI packet buffer IN/OUT threshold to 128 bytes */ + u32 threshold = 128 | (128 << 16); + + writel(threshold, regs + AHB2STBUS_INSREG01); +} + +static int st_hcd_enable_clocks(struct device *dev, + struct st_hcd_dev *hcd_dev) +{ + int err; + /* + * The interconnect input clock have either fixed + * rate or the rate is defined on boot, so we are only concerned about + * enabling any gates for this clock. + */ + err = clk_prepare_enable(hcd_dev->ic_clk); + if (err) { + dev_err(dev, "can't enable ic clock\n"); + return err; + } + /* + * The 48MHz OHCI clock is usually provided by a programmable + * frequency synthesizer, which is often not programmed on boot/chip + * reset, so we set its rate here to ensure it is correct. + * + * However not all SoC's have a dedicated ohci clock so it isn't fatal + * for this not to exist. + */ + if (!IS_ERR(hcd_dev->ohci_clk)) { + err = clk_set_rate(hcd_dev->ohci_clk, 48000000); + if (err) { + dev_err(dev, "can't set ohci_clk rate\n"); + goto error; + } + + err = clk_prepare_enable(hcd_dev->ohci_clk); + if (err) { + dev_err(dev, "can't enable ohci_clk\n"); + goto error; + } + } + + return 0; +error: + clk_disable_unprepare(hcd_dev->ic_clk); + return err; +} + + +static void st_hcd_disable_clocks(struct st_hcd_dev *hcd_dev) +{ + /* not all SoCs provide a dedicated ohci_clk */ + if (!IS_ERR(hcd_dev->ohci_clk)) + clk_disable_unprepare(hcd_dev->ohci_clk); + + clk_disable_unprepare(hcd_dev->ic_clk); +} + +static void st_hcd_assert_resets(struct device *dev, + struct st_hcd_dev *hcd_dev) +{ + int err; + + err = reset_control_assert(hcd_dev->pwr); + if (err) + dev_err(dev, "unable to put into powerdown\n"); + + err = reset_control_assert(hcd_dev->rst); + if (err) + dev_err(dev, "unable to put into softreset\n"); +} + +static int st_hcd_deassert_resets(struct device *dev, + struct st_hcd_dev *hcd_dev) +{ + int err; + + err = reset_control_deassert(hcd_dev->pwr); + if (err) { + dev_err(dev, "unable to bring out of powerdown\n"); + return err; + } + + err = reset_control_deassert(hcd_dev->rst); + if (err) { + dev_err(dev, "unable to bring out of softreset\n"); + goto err_assert_power; + } + + return 0; + +err_assert_power: + reset_control_assert(hcd_dev->pwr); + return err; +} + +static const struct usb_ehci_pdata ehci_pdata = { +}; + +static const struct usb_ohci_pdata ohci_pdata = { +}; + +static int st_hcd_remove(struct platform_device *pdev) +{ + struct st_hcd_dev *hcd_dev = platform_get_drvdata(pdev); + + platform_device_unregister(hcd_dev->ehci_device); + platform_device_unregister(hcd_dev->ohci_device); + + phy_power_off(hcd_dev->phy); + + phy_exit(hcd_dev->phy); + + st_hcd_assert_resets(&pdev->dev, hcd_dev); + + st_hcd_disable_clocks(hcd_dev); + + return 0; +} + +static struct platform_device *st_hcd_device_create(const char *name, int id, + struct platform_device *parent) +{ + struct platform_device *pdev; + const char *platform_name; + struct resource *res; + struct resource hcd_res[2]; + int ret; + + res = platform_get_resource_byname(parent, IORESOURCE_MEM, name); + if (!res) + return ERR_PTR(-ENODEV); + + hcd_res[0] = *res; + + res = platform_get_resource_byname(parent, IORESOURCE_IRQ, name); + if (!res) + return ERR_PTR(-ENODEV); + + hcd_res[1] = *res; + + platform_name = kasprintf(GFP_KERNEL, "%s-platform", name); + if (!platform_name) + return ERR_PTR(-ENOMEM); + + pdev = platform_device_alloc(platform_name, id); + + kfree(platform_name); + + if (!pdev) + return ERR_PTR(-ENOMEM); + + pdev->dev.parent = &parent->dev; + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask; + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); + + ret = platform_device_add_resources(pdev, hcd_res, ARRAY_SIZE(hcd_res)); + if (ret) + goto error; + + if (!strcmp(name, "ohci")) + ret = platform_device_add_data(pdev, &ohci_pdata, + sizeof(ohci_pdata)); + else + ret = platform_device_add_data(pdev, &ehci_pdata, + sizeof(ehci_pdata)); + + if (ret) + goto error; + + ret = platform_device_add(pdev); + if (ret) + goto error; + + return pdev; + +error: + platform_device_put(pdev); + return ERR_PTR(ret); +} + +#ifdef CONFIG_PM_SLEEP +static int st_hcd_resume(struct device *dev) +{ + struct st_hcd_dev *hcd_dev = dev_get_drvdata(dev); + struct usb_hcd *ehci_hcd = platform_get_drvdata(hcd_dev->ehci_device); + int err; + + pinctrl_pm_select_default_state(dev); + + err = st_hcd_enable_clocks(dev, hcd_dev); + if (err) + return err; + + err = phy_init(hcd_dev->phy); + if (err) { + dev_err(dev, "phy initialization failed\n"); + goto err_disable_clocks; + } + + err = phy_power_on(hcd_dev->phy); + if (err && (err != -ENOTSUPP)) { + dev_err(dev, "phy power on failed\n"); + goto err_phy_exit; + } + + err = st_hcd_deassert_resets(dev, hcd_dev); + if (err) + goto err_phy_off; + + st_ehci_configure_bus(ehci_hcd->regs); + + return 0; + +err_phy_off: + phy_power_off(hcd_dev->phy); +err_phy_exit: + phy_exit(hcd_dev->phy); +err_disable_clocks: + st_hcd_disable_clocks(hcd_dev); + + return err; +} + +static int st_hcd_suspend(struct device *dev) +{ + struct st_hcd_dev *hcd_dev = dev_get_drvdata(dev); + int err; + + err = reset_control_assert(hcd_dev->pwr); + if (err) { + dev_err(dev, "unable to put into powerdown\n"); + return err; + } + + err = reset_control_assert(hcd_dev->rst); + if (err) { + dev_err(dev, "unable to put into softreset\n"); + return err; + } + + err = phy_power_off(hcd_dev->phy); + if (err && (err != -ENOTSUPP)) { + dev_err(dev, "phy power off failed\n"); + return err; + } + + phy_exit(hcd_dev->phy); + + st_hcd_disable_clocks(hcd_dev); + + pinctrl_pm_select_sleep_state(dev); + + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(st_hcd_pm, st_hcd_suspend, st_hcd_resume); + +static struct of_device_id st_hcd_match[] = { + { .compatible = "st,usb-300x" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, st_hcd_match); + +static int st_hcd_probe_clocks(struct device *dev, + struct st_hcd_dev *hcd_dev) +{ + hcd_dev->ic_clk = devm_clk_get(dev, "ic"); + if (IS_ERR(hcd_dev->ic_clk)) { + dev_err(dev, "ic clk not found\n"); + return PTR_ERR(hcd_dev->ic_clk); + } + + /* some SoCs don't have a dedicated ohci clk */ + hcd_dev->ohci_clk = devm_clk_get(dev, "ohci"); + if (IS_ERR(hcd_dev->ohci_clk)) + dev_info(dev, "48MHz ohci clk not found\n"); + + return st_hcd_enable_clocks(dev, hcd_dev); +} + + +static int st_hcd_probe_resets(struct device *dev, + struct st_hcd_dev *hcd_dev) +{ + hcd_dev->pwr = devm_reset_control_get(dev, "power"); + if (IS_ERR(hcd_dev->pwr)) { + dev_err(dev, "power reset control not found\n"); + return PTR_ERR(hcd_dev->pwr); + } + + hcd_dev->rst = devm_reset_control_get(dev, "softreset"); + if (IS_ERR(hcd_dev->rst)) { + dev_err(dev, "soft reset control not found\n"); + return PTR_ERR(hcd_dev->rst); + } + + return st_hcd_deassert_resets(dev, hcd_dev); +} + +static int st_hcd_probe_ehci_setup(struct platform_device *pdev) +{ + struct resource *res; + void __iomem *ehci_regs; + + /* + * We need to do some integration specific setup in the EHCI + * controller, which the EHCI platform driver does not provide any + * hooks to allow us to do during it's initialisation. + */ + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ehci"); + if (!res) + return -ENODEV; + + ehci_regs = devm_ioremap(&pdev->dev, res->start, resource_size(res)); + if (!ehci_regs) + return -ENOMEM; + + st_ehci_configure_bus(ehci_regs); + devm_iounmap(&pdev->dev, ehci_regs); + + return 0; +} + +static int st_hcd_probe(struct platform_device *pdev) +{ + struct st_hcd_dev *hcd_dev; + int id; + int err; + + if (!pdev->dev.of_node) + return -ENODEV; + + id = of_alias_get_id(pdev->dev.of_node, "usb"); + + if (id < 0) { + dev_err(&pdev->dev, "No ID specified via DT alias\n"); + return -ENODEV; + } + + hcd_dev = devm_kzalloc(&pdev->dev, sizeof(*hcd_dev), GFP_KERNEL); + if (!hcd_dev) + return -ENOMEM; + + hcd_dev->port_nr = id; + + err = st_hcd_probe_clocks(&pdev->dev, hcd_dev); + if (err) + return err; + + err = st_hcd_probe_resets(&pdev->dev, hcd_dev); + if (err) + goto err_disable_clocks; + + err = st_hcd_probe_ehci_setup(pdev); + if (err) + goto err_assert_resets; + + hcd_dev->phy = devm_phy_get(&pdev->dev, "usb2-phy"); + if (IS_ERR(hcd_dev->phy)) { + dev_err(&pdev->dev, "no PHY configured\n"); + err = PTR_ERR(hcd_dev->phy); + goto err_assert_resets; + } + + err = phy_init(hcd_dev->phy); + if (err) { + dev_err(&pdev->dev, "phy initialization failed\n"); + goto err_assert_resets; + } + + err = phy_power_on(hcd_dev->phy); + if (err && (err != -ENOTSUPP)) { + dev_err(&pdev->dev, "phy power on failed\n"); + goto err_phy_exit; + } + + hcd_dev->ehci_device = st_hcd_device_create("ehci", id, pdev); + if (IS_ERR(hcd_dev->ehci_device)) { + err = PTR_ERR(hcd_dev->ehci_device); + goto err_phy_off; + } + + hcd_dev->ohci_device = st_hcd_device_create("ohci", id, pdev); + if (IS_ERR(hcd_dev->ohci_device)) { + platform_device_del(hcd_dev->ehci_device); + err = PTR_ERR(hcd_dev->ohci_device); + goto err_phy_off; + } + + platform_set_drvdata(pdev, hcd_dev); + + return 0; + +err_phy_off: + phy_power_off(hcd_dev->phy); +err_phy_exit: + phy_exit(hcd_dev->phy); +err_assert_resets: + st_hcd_assert_resets(&pdev->dev, hcd_dev); +err_disable_clocks: + st_hcd_disable_clocks(hcd_dev); + + return err; +} + +static struct platform_driver st_hcd_driver = { + .probe = st_hcd_probe, + .remove = st_hcd_remove, + .driver = { + .name = "st-hcd", + .pm = &st_hcd_pm, + .of_match_table = st_hcd_match, + }, +}; + +module_platform_driver(st_hcd_driver); + +MODULE_DESCRIPTION("STMicroelectronics On-Chip USB Host Controller"); +MODULE_AUTHOR("Stephen Gallimore <stephen.gallimore@st.com>"); +MODULE_LICENSE("GPL v2");
This driver adds support for the USB HCD present in STi SoC's from STMicroelectronics. It has been tested on the stih416-b2020 board. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- drivers/usb/host/Kconfig | 17 ++ drivers/usb/host/Makefile | 1 + drivers/usb/host/st-hcd.c | 471 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 drivers/usb/host/st-hcd.c