[PATCH 0/3] Add USB HCD support for STi SoCs

[PATCH 0/3] Add USB HCD support for STi SoCs

[Peter Griffin]

This driver adds support for the USB HCD controller present in STi
SoC's from STMicroelectronics. It has been tested on the stih416-b2020
board.

The original author is no longer with the company and therefore emails
to his address bounce, thus I have not added him to CC.

Peter Griffin (3):
  usb: host: st-hcd: Add USB HCD support for STi SoCs
  usb: host: st-hcd: Add st-hcd devicetree bindings documentation.
  MAINTAINERS: Add st-hcd to ARCH/STI architecture

 Documentation/devicetree/bindings/usb/st-hcd.txt |  51 +++
 MAINTAINERS                                      |   1 +
 drivers/usb/host/Kconfig                         |  17 +
 drivers/usb/host/Makefile                        |   1 +
 drivers/usb/host/st-hcd.c                        | 471 +++++++++++++++++++++++
 5 files changed, 541 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/st-hcd.txt
 create mode 100644 drivers/usb/host/st-hcd.c

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Peter Griffin]

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
+	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");
-- 
1.9.1

Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Alan Stern]

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

Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Peter Griffin]

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.

Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Alan Stern]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Lee Jones]

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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Alan Stern]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Peter Griffin]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Lee Jones]

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");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Peter Griffin]

Hi Lee,

Thanks for reviewing. All your review comments have been fixed in v2.

regards,

Peter.

[PATCH 2/3] usb: host: st-hcd: Add st-hcd devicetree bindings documentation.

[Peter Griffin]

This patch documents the device tree documentation required for
the ST HCD controller found in STMicroelectronics SoCs.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 Documentation/devicetree/bindings/usb/st-hcd.txt | 51 ++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/st-hcd.txt

diff --git a/Documentation/devicetree/bindings/usb/st-hcd.txt b/Documentation/devicetree/bindings/usb/st-hcd.txt
new file mode 100644
index 0000000..f2500dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/st-hcd.txt
@@ -0,0 +1,51 @@
+ST HCD (Host Controller Driver) for USB 2.0 and 1.1
+
+The device node has the following properties.
+
+Required properties:
+ - compatible		: must be "st,usb-300x"
+ - reg			: physical base addresses of the controller and length of memory mapped
+			  region
+ - reg-names		: names associated to the reg defines above, should be "ehci" and "ohci"
+ - interrupts		: interrupt numbers to the cpu
+ - interrupt-names	: should be "ehci" and "ohci"
+
+ - pinctrl-names	: a pinctrl state named "default" must be defined
+-  pinctrl-0		: phandle referencing pin configuration of the USB controller
+See: Documentation/devicetree/bindings/pinctrl/pinctrl-binding.txt
+
+ - clocks		: phandle list of usb clocks.
+ - clock-names		: should be "ic" for interconnect clock and "ohci" for the 48MHz clock
+See: Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+ - resets		: phandle to the powerdown and reset controller for the USB IP
+ - reset-names		: should be "power" and "softreset".
+See: Documentation/devicetree/bindings/reset/st,sti-powerdown.txt
+See: Documentation/devicetree/bindings/reset/reset.txt
+
+Example:
+
+usb0: usb@fe100000 {
+	compatible	= "st,usb-300x";
+	reg		= <0xfe1ffc00 0x100>,
+			  <0xfe1ffe00 0x100>;
+	reg-names	= "ohci", "ehci";
+
+	interrupts	=  <GIC_SPI 148 IRQ_TYPE_NONE>,
+			   <GIC_SPI 149 IRQ_TYPE_NONE>;
+	interrupt-names	= "ehci","ohci";
+	pinctrl-names	= "default";
+	pinctrl-0	= <&pinctrl_usb0>;
+	clocks		= <&clk_s_a1_ls CLK_ICN_IF_2>,
+			  <&clockgen_b0 0>;
+	clock-names	= "ic", "ohci";
+
+	resets		= <&powerdown STIH416_USB0_POWERDOWN>,
+			  <&softreset STIH416_USB0_SOFTRESET>;
+	reset-names	= "power", "softreset";
+
+	phys = <&usb2_phy>;
+	phy-names = "usb2-phy";
+};
+
+
-- 
1.9.1

Re: [PATCH 2/3] usb: host: st-hcd: Add st-hcd devicetree bindings documentation.

[Lee Jones]

On Thu, 10 Jul 2014, Peter Griffin wrote:

> This patch documents the device tree documentation required for
> the ST HCD controller found in STMicroelectronics SoCs.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  Documentation/devicetree/bindings/usb/st-hcd.txt | 51 ++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/st-hcd.txt

Just some nits:

> +Example:
> +
> +usb0: usb@fe100000 {
> +	compatible	= "st,usb-300x";
> +	reg		= <0xfe1ffc00 0x100>,
> +			  <0xfe1ffe00 0x100>;
> +	reg-names	= "ohci", "ehci";
> +
> +	interrupts	=  <GIC_SPI 148 IRQ_TYPE_NONE>,
> +			   <GIC_SPI 149 IRQ_TYPE_NONE>;
> +	interrupt-names	= "ehci","ohci";

s/","/", "

> +	pinctrl-names	= "default";
> +	pinctrl-0	= <&pinctrl_usb0>;
> +	clocks		= <&clk_s_a1_ls CLK_ICN_IF_2>,
> +			  <&clockgen_b0 0>;
> +	clock-names	= "ic", "ohci";
> +
> +	resets		= <&powerdown STIH416_USB0_POWERDOWN>,
> +			  <&softreset STIH416_USB0_SOFTRESET>;
> +	reset-names	= "power", "softreset";
> +
> +	phys = <&usb2_phy>;
> +	phy-names = "usb2-phy";

Tabbing of the '='s not consistent.

> +};
> +
> +

Too many '\n' at EOF.

Nothing controversial here, so once this nits are rectified:

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/3] MAINTAINERS: Add st-hcd to ARCH/STI architecture

[Peter Griffin]

Signed-off-by: Peter Griffin <peter.griffin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 702ca10..359a64e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1325,6 +1325,7 @@ F:	drivers/pinctrl/pinctrl-st.c
 F:	drivers/media/rc/st_rc.c
 F:	drivers/i2c/busses/i2c-st.c
 F:	drivers/tty/serial/st-asc.c
+F:	drivers/usb/host/st-hcd.c
 
 ARM/TECHNOLOGIC SYSTEMS TS7250 MACHINE SUPPORT
 M:	Lennert Buytenhek <kernel-OLH4Qvv75CYX/NnBR394Jw@public.gmane.org>
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] MAINTAINERS: Add st-hcd to ARCH/STI architecture

[Lee Jones]

On Thu, 10 Jul 2014, Peter Griffin wrote:

> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 702ca10..359a64e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1325,6 +1325,7 @@ F:	drivers/pinctrl/pinctrl-st.c
>  F:	drivers/media/rc/st_rc.c
>  F:	drivers/i2c/busses/i2c-st.c
>  F:	drivers/tty/serial/st-asc.c
> +F:	drivers/usb/host/st-hcd.c
>  
>  ARM/TECHNOLOGIC SYSTEMS TS7250 MACHINE SUPPORT
>  M:	Lennert Buytenhek <kernel@wantstofly.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog