All of lore.kernel.org
 help / color / mirror / Atom feed
* at91/USB: high speed USB support for at91sam9g45 series
@ 2009-06-09 11:38 Nicolas Ferre
  2009-06-09 11:38   ` Nicolas Ferre
  0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Ferre @ 2009-06-09 11:38 UTC (permalink / raw)
  To: linux-usb, avictor.za
  Cc: linux-kernel, david-b, haavard.skinnemoen, patrice.vilchez

Those 2 patches are adding the USB high speed 
support to the at91sam9g45 SOC family.

They are relying on the integration of at91sam9g45 
already posted. Here is the patch series that you 
will have to take into account before applying 
those patches.

clock.c changes:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5438/1

9g45 introduction:
http://lkml.org/lkml/2009/6/4/77
http://lkml.org/lkml/2009/6/4/76
http://lkml.org/lkml/2009/6/4/78

atmel_usba tiny modification (bias function only for 9rl):
http://lkml.org/lkml/2009/6/5/355

 arch/arm/mach-at91/at91sam9g45_devices.c |   56 +++++
 arch/arm/mach-at91/board-sam9m10g45ek.c  |    1 +
 arch/arm/mach-at91/include/mach/board.h  |    1 +
 arch/arm/tools/mach-types                |    1 +
 drivers/usb/Kconfig                      |    1 +
 drivers/usb/gadget/Kconfig               |    4 +-
 drivers/usb/host/ehci-atmel.c            |  251 ++++++++++++++++++++
 drivers/usb/host/ehci-hcd.c              |    5 +
 drivers/usb/host/ohci-at91.c             |    3 +-
 9 files changed, 320 insertions(+), 3 deletions(-)
 create mode 100644 drivers/usb/host/ehci-atmel.c



^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 1/2] at91/USB: Add USB drivers for at91sam9g45 series
@ 2009-06-09 11:38   ` Nicolas Ferre
  0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-06-09 11:38 UTC (permalink / raw)
  To: linux-usb, avictor.za
  Cc: linux-kernel, david-b, haavard.skinnemoen, patrice.vilchez,
	Nicolas Ferre

Add both host and gadget USB drivers for at91sam9g45 series. Those SOC embed
high speed USB interfaces.
The gadget driver is the already available atmel_usba_udc.
The host driver is an EHCI with its companion OHCI. EHCI is handled by the new
ehci-atmel.c whereas the OHCI is always handled by ohci-at91.c. This last
wrapper is modified to allow IRQ sharing between two controllers.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/usb/Kconfig           |    1 +
 drivers/usb/gadget/Kconfig    |    4 +-
 drivers/usb/host/ehci-atmel.c |  251 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/ehci-hcd.c   |    5 +
 drivers/usb/host/ohci-at91.c  |    3 +-
 5 files changed, 261 insertions(+), 3 deletions(-)
 create mode 100644 drivers/usb/host/ehci-atmel.c

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index c6c816b..3974c9c 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -57,6 +57,7 @@ config USB_ARCH_HAS_EHCI
 	default y if PPC_83xx
 	default y if SOC_AU1200
 	default y if ARCH_IXP4XX
+	default y if ARCH_AT91SAM9G45
 	default PCI
 
 # ARM SA1111 chips have a non-PCI based "OHCI-compatible" USB host interface.
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 080bb1e..9beea52 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -124,7 +124,7 @@ choice
 
 config USB_GADGET_AT91
 	boolean "Atmel AT91 USB Device Port"
-	depends on ARCH_AT91 && !ARCH_AT91SAM9RL && !ARCH_AT91CAP9
+	depends on ARCH_AT91 && !ARCH_AT91SAM9RL && !ARCH_AT91CAP9 && !ARCH_AT91SAM9G45
 	select USB_GADGET_SELECTED
 	help
 	   Many Atmel AT91 processors (such as the AT91RM2000) have a
@@ -143,7 +143,7 @@ config USB_AT91
 config USB_GADGET_ATMEL_USBA
 	boolean "Atmel USBA"
 	select USB_GADGET_DUALSPEED
-	depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL
+	depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
 	help
 	  USBA is the integrated high-speed USB Device controller on
 	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
new file mode 100644
index 0000000..c5f2303
--- /dev/null
+++ b/drivers/usb/host/ehci-atmel.c
@@ -0,0 +1,251 @@
+/*
+ * Driver for EHCI UHP on Atmel chips
+ *
+ *  Copyright (C) 2009 Atmel Corporation,
+ *                     Nicolas Ferre <nicolas.ferre@atmel.com>
+ *
+ *  Based on various ehci-*.c drivers
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+
+/* interface and function clocks */
+static struct clk *iclk, *fclk;
+static int clocked;
+
+/*-------------------------------------------------------------------------*/
+
+static void atmel_start_clock(void)
+{
+	clk_enable(iclk);
+	clk_enable(fclk);
+	clocked = 1;
+}
+
+static void atmel_stop_clock(void)
+{
+	clk_disable(fclk);
+	clk_disable(iclk);
+	clocked = 0;
+}
+
+static void atmel_start_ehci(struct platform_device *pdev)
+{
+
+	dev_dbg(&pdev->dev, "start\n");
+
+	/*
+	 * Start the USB clocks.
+	 */
+	atmel_start_clock();
+}
+
+static void atmel_stop_ehci(struct platform_device *pdev)
+{
+	dev_dbg(&pdev->dev, "stop\n");
+
+	/*
+	 * Stop the USB clocks.
+	 */
+	atmel_stop_clock();
+}
+
+/*-------------------------------------------------------------------------*/
+
+static int ehci_atmel_setup(struct usb_hcd *hcd)
+{
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	int retval = 0;
+
+	/*
+	 * registers start at offset 0x0
+	 */
+	ehci->caps = hcd->regs;
+	ehci->regs = hcd->regs +
+		HC_LENGTH(ehci_readl(ehci, &ehci->caps->hc_capbase));
+	dbg_hcs_params(ehci, "reset");
+	dbg_hcc_params(ehci, "reset");
+
+	/*
+	 * cache this readonly data; minimize chip reads
+	 */
+	ehci->hcs_params = ehci_readl(ehci, &ehci->caps->hcs_params);
+
+	retval = ehci_halt(ehci);
+	if (retval)
+		return retval;
+
+	/*
+	 * data structure init
+	 */
+	retval = ehci_init(hcd);
+	if (retval)
+		return retval;
+
+	ehci->sbrn = 0x20;
+
+	ehci_reset(ehci);
+	ehci_port_power(ehci, 0);
+
+	return retval;
+}
+
+static const struct hc_driver ehci_atmel_hc_driver = {
+	.description		= hcd_name,
+	.product_desc		= "Atmel EHCI UHP HS",
+	.hcd_priv_size		= sizeof(struct ehci_hcd),
+
+	/*
+	 * generic hardware linkage
+	 */
+	.irq			= ehci_irq,
+	.flags			= HCD_MEMORY | HCD_USB2,
+
+	/*
+	 * basic lifecycle operations
+	 */
+	.reset			= ehci_atmel_setup,
+	.start			= ehci_run,
+	.stop			= ehci_stop,
+	.shutdown		= ehci_shutdown,
+
+	/*
+	 * managing i/o requests and associated device resources
+	 */
+	.urb_enqueue		= ehci_urb_enqueue,
+	.urb_dequeue		= ehci_urb_dequeue,
+	.endpoint_disable	= ehci_endpoint_disable,
+
+	/*
+	 * scheduling support
+	 */
+	.get_frame_number	= ehci_get_frame,
+
+	/*
+	 * root hub support
+	 */
+	.hub_status_data	= ehci_hub_status_data,
+	.hub_control		= ehci_hub_control,
+	.bus_suspend		= ehci_bus_suspend,
+	.bus_resume		= ehci_bus_resume,
+	.relinquish_port	= ehci_relinquish_port,
+	.port_handed_over	= ehci_port_handed_over,
+};
+
+static int __init ehci_atmel_drv_probe(struct platform_device *pdev)
+{
+	struct usb_hcd *hcd;
+	const struct hc_driver *driver = &ehci_atmel_hc_driver;
+	struct resource *res;
+	int irq;
+	int retval;
+
+	if (usb_disabled())
+		return -ENODEV;
+
+	pr_debug("Initializing Atmel-SoC USB Host Controller\n");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		dev_err(&pdev->dev,
+			"Found HC with no IRQ. Check %s setup!\n",
+			dev_name(&pdev->dev));
+		retval = -ENODEV;
+		goto fail_create_hcd;
+	}
+
+	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
+	if (!hcd) {
+		retval = -ENOMEM;
+		goto fail_create_hcd;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev,
+			"Found HC with no register addr. Check %s setup!\n",
+			dev_name(&pdev->dev));
+		retval = -ENODEV;
+		goto fail_request_resource;
+	}
+	hcd->rsrc_start = res->start;
+	hcd->rsrc_len = res->end - res->start + 1;
+
+	if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
+				driver->description)) {
+		dev_dbg(&pdev->dev, "controller already in use\n");
+		retval = -EBUSY;
+		goto fail_request_resource;
+	}
+
+	hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
+	if (hcd->regs == NULL) {
+		dev_dbg(&pdev->dev, "error mapping memory\n");
+		retval = -EFAULT;
+		goto fail_ioremap;
+	}
+
+	iclk = clk_get(&pdev->dev, "ehci_clk");
+	fclk = clk_get(&pdev->dev, "uhpck");
+	if (IS_ERR(iclk) || IS_ERR(fclk)) {
+		dev_err(&pdev->dev, "Error getting clocks\n");
+		retval = -ENOENT;
+		goto fail_get_clk;
+	}
+
+	atmel_start_ehci(pdev);
+
+	retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
+	if (retval)
+		goto fail_add_hcd;
+
+	return retval;
+
+fail_add_hcd:
+	atmel_stop_ehci(pdev);
+fail_get_clk:
+	clk_put(fclk);
+	clk_put(iclk);
+	iounmap(hcd->regs);
+fail_ioremap:
+	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+fail_request_resource:
+	usb_put_hcd(hcd);
+fail_create_hcd:
+	dev_err(&pdev->dev, "init %s fail, %d\n",
+		dev_name(&pdev->dev), retval);
+
+	return retval;
+}
+
+static int __exit ehci_atmel_drv_remove(struct platform_device *pdev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+
+	ehci_shutdown(hcd);
+	usb_remove_hcd(hcd);
+	iounmap(hcd->regs);
+	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+	usb_put_hcd(hcd);
+
+	atmel_stop_ehci(pdev);
+	clk_put(fclk);
+	clk_put(iclk);
+	fclk = iclk = NULL;
+
+	return 0;
+}
+
+MODULE_ALIAS("platform:atmel-ehci");
+
+static struct platform_driver ehci_atmel_driver = {
+	.probe		= ehci_atmel_drv_probe,
+	.remove		= __exit_p(ehci_atmel_drv_remove),
+	.shutdown	= usb_hcd_platform_shutdown,
+	.driver.name	= "atmel-ehci",
+};
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index c637207..df2ddee 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1072,6 +1072,11 @@ MODULE_LICENSE ("GPL");
 #define	PLATFORM_DRIVER		ixp4xx_ehci_driver
 #endif
 
+#ifdef CONFIG_ARCH_AT91
+#include "ehci-atmel.c"
+#define	PLATFORM_DRIVER		ehci_atmel_driver
+#endif
+
 #if !defined(PCI_DRIVER) && !defined(PLATFORM_DRIVER) && \
     !defined(PS3_SYSTEM_BUS_DRIVER) && !defined(OF_PLATFORM_DRIVER)
 #error "missing bus glue for ehci-hcd"
diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index bb5e6f6..b29b0fe 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -148,7 +148,8 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
 	at91_start_hc(pdev);
 	ohci_hcd_init(hcd_to_ohci(hcd));
 
-	retval = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_DISABLED);
+	retval = usb_add_hcd(hcd, pdev->resource[1].start,
+				IRQF_DISABLED | IRQF_SHARED);
 	if (retval == 0)
 		return retval;
 
-- 
1.5.3.7


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration
@ 2009-06-09 11:38     ` Nicolas Ferre
  2009-06-19  7:43       ` David Brownell
  0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Ferre @ 2009-06-09 11:38 UTC (permalink / raw)
  To: linux-usb, avictor.za
  Cc: linux-kernel, david-b, haavard.skinnemoen, patrice.vilchez,
	Nicolas Ferre

This is the at91 specific part of USB host integration. The EHCI high speed
controller has a companion OHCI controller to manage USB full and low speed.
They are sharing the same IRQ line and vbus pin.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 arch/arm/mach-at91/at91sam9g45_devices.c |   56 ++++++++++++++++++++++++++++++
 arch/arm/mach-at91/board-sam9m10g45ek.c  |    1 +
 arch/arm/mach-at91/include/mach/board.h  |    1 +
 3 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index d746e86..c2ecbb6 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -83,6 +83,62 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data) {}
 
 
 /* --------------------------------------------------------------------
+ *  USB Host HS (EHCI)
+ *  Needs an OHCI host for low and full speed management
+ * -------------------------------------------------------------------- */
+
+#if defined(CONFIG_USB_EHCI_HCD) || defined(CONFIG_USB_EHCI_HCD_MODULE)
+static u64 ehci_dmamask = DMA_BIT_MASK(32);
+static struct at91_usbh_data usbh_ehci_data;
+
+static struct resource usbh_ehci_resources[] = {
+	[0] = {
+		.start	= AT91SAM9G45_EHCI_BASE,
+		.end	= AT91SAM9G45_EHCI_BASE + SZ_1M - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+	[1] = {
+		.start	= AT91SAM9G45_ID_UHPHS,
+		.end	= AT91SAM9G45_ID_UHPHS,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device at91_usbh_ehci_device = {
+	.name		= "atmel-ehci",
+	.id		= -1,
+	.dev		= {
+				.dma_mask		= &ehci_dmamask,
+				.coherent_dma_mask	= DMA_BIT_MASK(32),
+				.platform_data		= &usbh_ehci_data,
+	},
+	.resource	= usbh_ehci_resources,
+	.num_resources	= ARRAY_SIZE(usbh_ehci_resources),
+};
+
+void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
+{
+	int i;
+
+	if (!data)
+		return;
+
+	/* Enable VBus control for UHP ports */
+	for (i = 0; i < data->ports; i++) {
+		if (data->vbus_pin[i])
+			at91_set_gpio_output(data->vbus_pin[i], 0);
+	}
+
+	usbh_ehci_data = *data;
+	at91_clock_associate("uhphs_clk", &at91_usbh_ehci_device.dev, "ehci_clk");
+	platform_device_register(&at91_usbh_ehci_device);
+}
+#else
+void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data) {}
+#endif
+
+
+/* --------------------------------------------------------------------
  *  USB HS Device (Gadget)
  * -------------------------------------------------------------------- */
 
diff --git a/arch/arm/mach-at91/board-sam9m10g45ek.c b/arch/arm/mach-at91/board-sam9m10g45ek.c
index b8558ea..d7251b7 100644
--- a/arch/arm/mach-at91/board-sam9m10g45ek.c
+++ b/arch/arm/mach-at91/board-sam9m10g45ek.c
@@ -358,6 +358,7 @@ static void __init ek_board_init(void)
 	at91_add_device_serial();
 	/* USB HS Host */
 	at91_add_device_usbh_ohci(&ek_usbh_hs_data);
+	at91_add_device_usbh_ehci(&ek_usbh_hs_data);
 	/* USB HS Device */
 	at91_add_device_usba(&ek_usba_udc_data);
 	/* SPI */
diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
index 74801d2..13640b0 100644
--- a/arch/arm/mach-at91/include/mach/board.h
+++ b/arch/arm/mach-at91/include/mach/board.h
@@ -92,6 +92,7 @@ struct at91_usbh_data {
 };
 extern void __init at91_add_device_usbh(struct at91_usbh_data *data);
 extern void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data);
+extern void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data);
 
  /* NAND / SmartMedia */
 struct atmel_nand_data {
-- 
1.5.3.7


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration
  2009-06-09 11:38     ` Nicolas Ferre
@ 2009-06-19  7:43       ` David Brownell
  2009-06-19  7:51         ` Haavard Skinnemoen
  0 siblings, 1 reply; 20+ messages in thread
From: David Brownell @ 2009-06-19  7:43 UTC (permalink / raw)
  To: Nicolas Ferre
  Cc: linux-usb, avictor.za, linux-kernel, haavard.skinnemoen, patrice.vilchez

On Tuesday 09 June 2009, Nicolas Ferre wrote:
> This is the at91 specific part of USB host integration. The EHCI high speed
> controller has a companion OHCI controller to manage USB full and low speed.
> They are sharing the same IRQ line and vbus pin.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>

My only issue here is with the GPIO stuff; see below.


> ---
>  arch/arm/mach-at91/at91sam9g45_devices.c |   56 ++++++++++++++++++++++++++++++
>  arch/arm/mach-at91/board-sam9m10g45ek.c  |    1 +
>  arch/arm/mach-at91/include/mach/board.h  |    1 +
>  3 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> index d746e86..c2ecbb6 100644
> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> @@ -83,6 +83,62 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data) {}
>  
>  
>  /* --------------------------------------------------------------------
> + *  USB Host HS (EHCI)
> + *  Needs an OHCI host for low and full speed management
> + * -------------------------------------------------------------------- */
> +
> +#if defined(CONFIG_USB_EHCI_HCD) || defined(CONFIG_USB_EHCI_HCD_MODULE)
> +static u64 ehci_dmamask = DMA_BIT_MASK(32);
> +static struct at91_usbh_data usbh_ehci_data;
> +
> +static struct resource usbh_ehci_resources[] = {
> +	[0] = {
> +		.start	= AT91SAM9G45_EHCI_BASE,
> +		.end	= AT91SAM9G45_EHCI_BASE + SZ_1M - 1,
> +		.flags	= IORESOURCE_MEM,
> +	},
> +	[1] = {
> +		.start	= AT91SAM9G45_ID_UHPHS,
> +		.end	= AT91SAM9G45_ID_UHPHS,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static struct platform_device at91_usbh_ehci_device = {
> +	.name		= "atmel-ehci",
> +	.id		= -1,
> +	.dev		= {
> +				.dma_mask		= &ehci_dmamask,
> +				.coherent_dma_mask	= DMA_BIT_MASK(32),
> +				.platform_data		= &usbh_ehci_data,
> +	},
> +	.resource	= usbh_ehci_resources,
> +	.num_resources	= ARRAY_SIZE(usbh_ehci_resources),
> +};
> +
> +void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
> +{
> +	int i;
> +
> +	if (!data)
> +		return;
> +
> +	/* Enable VBus control for UHP ports */
> +	for (i = 0; i < data->ports; i++) {
> +		if (data->vbus_pin[i])
> +			at91_set_gpio_output(data->vbus_pin[i], 0);

This should gpio_request() and gpio_direction_output().

Don't use AT91-specific GPIO calls except for things that
the generic calls don't support ... like enabling open-drain
outputs, the deglitching support, or input pullups.


> +	}
> +
> +	usbh_ehci_data = *data;
> +	at91_clock_associate("uhphs_clk", &at91_usbh_ehci_device.dev, "ehci_clk");
> +	platform_device_register(&at91_usbh_ehci_device);
> +}
> +#else
> +void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data) {}
> +#endif
> +
> +
> +/* --------------------------------------------------------------------
>   *  USB HS Device (Gadget)
>   * -------------------------------------------------------------------- */
>  
> diff --git a/arch/arm/mach-at91/board-sam9m10g45ek.c b/arch/arm/mach-at91/board-sam9m10g45ek.c
> index b8558ea..d7251b7 100644
> --- a/arch/arm/mach-at91/board-sam9m10g45ek.c
> +++ b/arch/arm/mach-at91/board-sam9m10g45ek.c
> @@ -358,6 +358,7 @@ static void __init ek_board_init(void)
>  	at91_add_device_serial();
>  	/* USB HS Host */
>  	at91_add_device_usbh_ohci(&ek_usbh_hs_data);
> +	at91_add_device_usbh_ehci(&ek_usbh_hs_data);
>  	/* USB HS Device */
>  	at91_add_device_usba(&ek_usba_udc_data);
>  	/* SPI */
> diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
> index 74801d2..13640b0 100644
> --- a/arch/arm/mach-at91/include/mach/board.h
> +++ b/arch/arm/mach-at91/include/mach/board.h
> @@ -92,6 +92,7 @@ struct at91_usbh_data {
>  };
>  extern void __init at91_add_device_usbh(struct at91_usbh_data *data);
>  extern void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data);
> +extern void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data);
>  
>   /* NAND / SmartMedia */
>  struct atmel_nand_data {
> -- 
> 1.5.3.7
> 
> 



^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration
  2009-06-19  7:43       ` David Brownell
@ 2009-06-19  7:51         ` Haavard Skinnemoen
  2009-06-19  9:26           ` David Brownell
  0 siblings, 1 reply; 20+ messages in thread
From: Haavard Skinnemoen @ 2009-06-19  7:51 UTC (permalink / raw)
  To: David Brownell
  Cc: Nicolas Ferre, linux-usb, avictor.za, linux-kernel, patrice.vilchez

David Brownell wrote:
> > --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> > +++ b/arch/arm/mach-at91/at91sam9g45_devices.c

> > +	/* Enable VBus control for UHP ports */
> > +	for (i = 0; i < data->ports; i++) {
> > +		if (data->vbus_pin[i])
> > +			at91_set_gpio_output(data->vbus_pin[i], 0);  
> 
> This should gpio_request() and gpio_direction_output().

Hmm...I thought the driver was supposed to call gpio_request(), not the
platform code?

> Don't use AT91-specific GPIO calls except for things that
> the generic calls don't support ... like enabling open-drain
> outputs, the deglitching support, or input pullups.

This call does port configuration, which you convinced me a long time
ago was a fundamentally different thing from GPIO. If the pin really
requires one of those features, this would definitely be the place to
set it up.

Haavard

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration
  2009-06-19  7:51         ` Haavard Skinnemoen
@ 2009-06-19  9:26           ` David Brownell
  2009-09-16 16:17               ` Nicolas Ferre
  2009-09-21 20:49             ` [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration Andrew Victor
  0 siblings, 2 replies; 20+ messages in thread
From: David Brownell @ 2009-06-19  9:26 UTC (permalink / raw)
  To: Haavard Skinnemoen
  Cc: Nicolas Ferre, linux-usb, avictor.za, linux-kernel, patrice.vilchez

On Friday 19 June 2009, Haavard Skinnemoen wrote:
> David Brownell wrote:
> > > --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> > > +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> 
> > > +	/* Enable VBus control for UHP ports */
> > > +	for (i = 0; i < data->ports; i++) {
> > > +		if (data->vbus_pin[i])
> > > +			at91_set_gpio_output(data->vbus_pin[i], 0);  
> > 
> > This should gpio_request() and gpio_direction_output().
> 
> Hmm...I thought the driver was supposed to call gpio_request(), not the
> platform code?

In some cases.  This isn't a good case for that.  Especially
if it's going to call gpio_direction_output() ... which needs
gpio_request() to have been done first.


> > Don't use AT91-specific GPIO calls except for things that
> > the generic calls don't support ... like enabling open-drain
> > outputs, the deglitching support, or input pullups.
> 
> This call does port configuration, which you convinced me a long time
> ago was a fundamentally different thing from GPIO.

Yes, pin/port config is certainly part of what the platform's
code to set up devices should handle.  That can include making
sure a given pin is configured as a GPIO ... and in the normal
case where it's dedicated to that task, it simplifies the driver
to have it pre-allocated and configured for I/O/both.

I'm pulling in some discussion from a different email thread
earlier, which proposed doing the right thing and finally
getting rid of the at91-specific GPIO calls except in the few
cases they could not be avoided.

It might be that AT91 needs to add some pin config calls which
resemble what you did for AT32AP7 chips.

- Dave


> If the pin really 
> requires one of those features, this would definitely be the place to
> set it up.
> 
> Haavard
> 
> 



^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration
  2009-06-19  9:26           ` David Brownell
@ 2009-09-16 16:17               ` Nicolas Ferre
  2009-09-21 20:49             ` [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration Andrew Victor
  1 sibling, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-16 16:17 UTC (permalink / raw)
  To: David Brownell, linux-usb, avictor.za
  Cc: Haavard Skinnemoen, linux-kernel, patrice.vilchez, Nicolas FERRE,
	linux-arm-kernel

Hi,

I come back on this topic as at91sam9g45 is now in mainline. I need this
integration code to have the USB EHCI work on the evaluation kit.

David Brownell :
> On Friday 19 June 2009, Haavard Skinnemoen wrote:
>> David Brownell wrote:
>>>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> +	/* Enable VBus control for UHP ports */
>>>> +	for (i = 0; i < data->ports; i++) {
>>>> +		if (data->vbus_pin[i])
>>>> +			at91_set_gpio_output(data->vbus_pin[i], 0);  
>>> This should gpio_request() and gpio_direction_output().
>> Hmm...I thought the driver was supposed to call gpio_request(), not the
>> platform code?
> 
> In some cases.  This isn't a good case for that.  Especially
> if it's going to call gpio_direction_output() ... which needs
> gpio_request() to have been done first.

Ok, I am building a patch on top of this one that uses these calls. This
way I can change both vbus pin configuration: OCHI and EHCI.

Best regards,
-- 
Nicolas Ferre


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration
@ 2009-09-16 16:17               ` Nicolas Ferre
  0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-16 16:17 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

I come back on this topic as at91sam9g45 is now in mainline. I need this
integration code to have the USB EHCI work on the evaluation kit.

David Brownell :
> On Friday 19 June 2009, Haavard Skinnemoen wrote:
>> David Brownell wrote:
>>>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> +	/* Enable VBus control for UHP ports */
>>>> +	for (i = 0; i < data->ports; i++) {
>>>> +		if (data->vbus_pin[i])
>>>> +			at91_set_gpio_output(data->vbus_pin[i], 0);  
>>> This should gpio_request() and gpio_direction_output().
>> Hmm...I thought the driver was supposed to call gpio_request(), not the
>> platform code?
> 
> In some cases.  This isn't a good case for that.  Especially
> if it's going to call gpio_direction_output() ... which needs
> gpio_request() to have been done first.

Ok, I am building a patch on top of this one that uses these calls. This
way I can change both vbus pin configuration: OCHI and EHCI.

Best regards,
-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
  2009-09-16 16:17               ` Nicolas Ferre
@ 2009-09-16 17:29                 ` Nicolas Ferre
  -1 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-16 17:29 UTC (permalink / raw)
  To: linux-usb, avictor.za
  Cc: linux-kernel, david-b, haavard.skinnemoen, patrice.vilchez,
	linux-arm-kernel, nicolas.ferre

Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
call instead of the at91 specific one.
Use gpio_request() function with same identifier for OHCI and EHCI hosts as
they are sharing the same pin.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
This patch is on top of at91sam9g45 USB integration one:
"[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
http://lkml.org/lkml/2009/6/9/221

 arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 5be8cf2..7d939c0 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
 
 	/* Enable VBus control for UHP ports */
 	for (i = 0; i < data->ports; i++) {
-		if (data->vbus_pin[i])
-			at91_set_gpio_output(data->vbus_pin[i], 0);
+		if (data->vbus_pin[i]) {
+			gpio_request(data->vbus_pin[i], "usb host vbus");
+			gpio_direction_output(data->vbus_pin[i], 0);
+		}
 	}
 
 	usbh_ohci_data = *data;
@@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
 
 	/* Enable VBus control for UHP ports */
 	for (i = 0; i < data->ports; i++) {
-		if (data->vbus_pin[i])
-			at91_set_gpio_output(data->vbus_pin[i], 0);
+		if (data->vbus_pin[i]) {
+			gpio_request(data->vbus_pin[i], "usb host vbus");
+			gpio_direction_output(data->vbus_pin[i], 0);
+		}
 	}
 
 	usbh_ehci_data = *data;
-- 
1.5.6.5


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
@ 2009-09-16 17:29                 ` Nicolas Ferre
  0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-16 17:29 UTC (permalink / raw)
  To: linux-arm-kernel

Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
call instead of the at91 specific one.
Use gpio_request() function with same identifier for OHCI and EHCI hosts as
they are sharing the same pin.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
This patch is on top of at91sam9g45 USB integration one:
"[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
http://lkml.org/lkml/2009/6/9/221

 arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 5be8cf2..7d939c0 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
 
 	/* Enable VBus control for UHP ports */
 	for (i = 0; i < data->ports; i++) {
-		if (data->vbus_pin[i])
-			at91_set_gpio_output(data->vbus_pin[i], 0);
+		if (data->vbus_pin[i]) {
+			gpio_request(data->vbus_pin[i], "usb host vbus");
+			gpio_direction_output(data->vbus_pin[i], 0);
+		}
 	}
 
 	usbh_ohci_data = *data;
@@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
 
 	/* Enable VBus control for UHP ports */
 	for (i = 0; i < data->ports; i++) {
-		if (data->vbus_pin[i])
-			at91_set_gpio_output(data->vbus_pin[i], 0);
+		if (data->vbus_pin[i]) {
+			gpio_request(data->vbus_pin[i], "usb host vbus");
+			gpio_direction_output(data->vbus_pin[i], 0);
+		}
 	}
 
 	usbh_ehci_data = *data;
-- 
1.5.6.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
  2009-09-16 17:29                 ` Nicolas Ferre
@ 2009-09-16 21:53                   ` Jean-Christophe PLAGNIOL-VILLARD
  -1 siblings, 0 replies; 20+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2009-09-16 21:53 UTC (permalink / raw)
  To: Nicolas Ferre
  Cc: linux-usb, avictor.za, patrice.vilchez, linux-kernel, david-b,
	haavard.skinnemoen, linux-arm-kernel

On 19:29 Wed 16 Sep     , Nicolas Ferre wrote:
> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
> call instead of the at91 specific one.
> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
> they are sharing the same pin.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> This patch is on top of at91sam9g45 USB integration one:
> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
> http://lkml.org/lkml/2009/6/9/221
> 
>  arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> index 5be8cf2..7d939c0 100644
> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>  
>  	/* Enable VBus control for UHP ports */
>  	for (i = 0; i < data->ports; i++) {
> -		if (data->vbus_pin[i])
> -			at91_set_gpio_output(data->vbus_pin[i], 0);
> +		if (data->vbus_pin[i]) {
> +			gpio_request(data->vbus_pin[i], "usb host vbus");
> +			gpio_direction_output(data->vbus_pin[i], 0);
> +		}
>  	}
>  
>  	usbh_ohci_data = *data;
> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>  
>  	/* Enable VBus control for UHP ports */
>  	for (i = 0; i < data->ports; i++) {
> -		if (data->vbus_pin[i])
> -			at91_set_gpio_output(data->vbus_pin[i], 0);
> +		if (data->vbus_pin[i]) {
> +			gpio_request(data->vbus_pin[i], "usb host vbus");
> +			gpio_direction_output(data->vbus_pin[i], 0);
> +		}
>  	}
as you do the same think for ehci & ohci why not factorize it?

Best Regards,
J.

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
@ 2009-09-16 21:53                   ` Jean-Christophe PLAGNIOL-VILLARD
  0 siblings, 0 replies; 20+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2009-09-16 21:53 UTC (permalink / raw)
  To: linux-arm-kernel

On 19:29 Wed 16 Sep     , Nicolas Ferre wrote:
> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
> call instead of the at91 specific one.
> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
> they are sharing the same pin.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> This patch is on top of at91sam9g45 USB integration one:
> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
> http://lkml.org/lkml/2009/6/9/221
> 
>  arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> index 5be8cf2..7d939c0 100644
> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>  
>  	/* Enable VBus control for UHP ports */
>  	for (i = 0; i < data->ports; i++) {
> -		if (data->vbus_pin[i])
> -			at91_set_gpio_output(data->vbus_pin[i], 0);
> +		if (data->vbus_pin[i]) {
> +			gpio_request(data->vbus_pin[i], "usb host vbus");
> +			gpio_direction_output(data->vbus_pin[i], 0);
> +		}
>  	}
>  
>  	usbh_ohci_data = *data;
> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>  
>  	/* Enable VBus control for UHP ports */
>  	for (i = 0; i < data->ports; i++) {
> -		if (data->vbus_pin[i])
> -			at91_set_gpio_output(data->vbus_pin[i], 0);
> +		if (data->vbus_pin[i]) {
> +			gpio_request(data->vbus_pin[i], "usb host vbus");
> +			gpio_direction_output(data->vbus_pin[i], 0);
> +		}
>  	}
as you do the same think for ehci & ohci why not factorize it?

Best Regards,
J.

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
  2009-09-16 21:53                   ` Jean-Christophe PLAGNIOL-VILLARD
@ 2009-09-17  8:13                     ` Nicolas Ferre
  -1 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-17  8:13 UTC (permalink / raw)
  To: Jean-Christophe PLAGNIOL-VILLARD
  Cc: linux-usb, avictor.za, patrice.vilchez, linux-kernel, david-b,
	haavard.skinnemoen, linux-arm-kernel

Jean-Christophe PLAGNIOL-VILLARD :
> On 19:29 Wed 16 Sep     , Nicolas Ferre wrote:
>> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
>> call instead of the at91 specific one.
>> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
>> they are sharing the same pin.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>> This patch is on top of at91sam9g45 USB integration one:
>> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
>> http://lkml.org/lkml/2009/6/9/221
>>
>>  arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
>> index 5be8cf2..7d939c0 100644
>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>>  
>>  	/* Enable VBus control for UHP ports */
>>  	for (i = 0; i < data->ports; i++) {
>> -		if (data->vbus_pin[i])
>> -			at91_set_gpio_output(data->vbus_pin[i], 0);
>> +		if (data->vbus_pin[i]) {
>> +			gpio_request(data->vbus_pin[i], "usb host vbus");
>> +			gpio_direction_output(data->vbus_pin[i], 0);
>> +		}
>>  	}
>>  
>>  	usbh_ohci_data = *data;
>> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>>  
>>  	/* Enable VBus control for UHP ports */
>>  	for (i = 0; i < data->ports; i++) {
>> -		if (data->vbus_pin[i])
>> -			at91_set_gpio_output(data->vbus_pin[i], 0);
>> +		if (data->vbus_pin[i]) {
>> +			gpio_request(data->vbus_pin[i], "usb host vbus");
>> +			gpio_direction_output(data->vbus_pin[i], 0);
>> +		}
>>  	}
> as you do the same think for ehci & ohci why not factorize it?

Just because you may choose only one or the other controller.
For instance, if you only want ohci, you can disable ehci selection and
always have its configuration done.

Best regards,
-- 
Nicolas Ferre


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
@ 2009-09-17  8:13                     ` Nicolas Ferre
  0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-17  8:13 UTC (permalink / raw)
  To: linux-arm-kernel

Jean-Christophe PLAGNIOL-VILLARD :
> On 19:29 Wed 16 Sep     , Nicolas Ferre wrote:
>> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
>> call instead of the at91 specific one.
>> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
>> they are sharing the same pin.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>> This patch is on top of at91sam9g45 USB integration one:
>> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
>> http://lkml.org/lkml/2009/6/9/221
>>
>>  arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
>> index 5be8cf2..7d939c0 100644
>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>>  
>>  	/* Enable VBus control for UHP ports */
>>  	for (i = 0; i < data->ports; i++) {
>> -		if (data->vbus_pin[i])
>> -			at91_set_gpio_output(data->vbus_pin[i], 0);
>> +		if (data->vbus_pin[i]) {
>> +			gpio_request(data->vbus_pin[i], "usb host vbus");
>> +			gpio_direction_output(data->vbus_pin[i], 0);
>> +		}
>>  	}
>>  
>>  	usbh_ohci_data = *data;
>> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>>  
>>  	/* Enable VBus control for UHP ports */
>>  	for (i = 0; i < data->ports; i++) {
>> -		if (data->vbus_pin[i])
>> -			at91_set_gpio_output(data->vbus_pin[i], 0);
>> +		if (data->vbus_pin[i]) {
>> +			gpio_request(data->vbus_pin[i], "usb host vbus");
>> +			gpio_direction_output(data->vbus_pin[i], 0);
>> +		}
>>  	}
> as you do the same think for ehci & ohci why not factorize it?

Just because you may choose only one or the other controller.
For instance, if you only want ohci, you can disable ehci selection and
always have its configuration done.

Best regards,
-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
  2009-09-17  8:13                     ` Nicolas Ferre
@ 2009-09-17  9:00                       ` Jean-Christophe PLAGNIOL-VILLARD
  -1 siblings, 0 replies; 20+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2009-09-17  9:00 UTC (permalink / raw)
  To: Nicolas Ferre
  Cc: linux-usb, patrice.vilchez, linux-kernel, david-b, avictor.za,
	haavard.skinnemoen, linux-arm-kernel

On 10:13 Thu 17 Sep     , Nicolas Ferre wrote:
> Jean-Christophe PLAGNIOL-VILLARD :
> > On 19:29 Wed 16 Sep     , Nicolas Ferre wrote:
> >> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
> >> call instead of the at91 specific one.
> >> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
> >> they are sharing the same pin.
> >>
> >> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> >> ---
> >> This patch is on top of at91sam9g45 USB integration one:
> >> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
> >> http://lkml.org/lkml/2009/6/9/221
> >>
> >>  arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
> >>  1 files changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> >> index 5be8cf2..7d939c0 100644
> >> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> >> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> >> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
> >>  
> >>  	/* Enable VBus control for UHP ports */
> >>  	for (i = 0; i < data->ports; i++) {
> >> -		if (data->vbus_pin[i])
> >> -			at91_set_gpio_output(data->vbus_pin[i], 0);
> >> +		if (data->vbus_pin[i]) {
> >> +			gpio_request(data->vbus_pin[i], "usb host vbus");
> >> +			gpio_direction_output(data->vbus_pin[i], 0);
> >> +		}
> >>  	}
> >>  
> >>  	usbh_ohci_data = *data;
> >> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
> >>  
> >>  	/* Enable VBus control for UHP ports */
> >>  	for (i = 0; i < data->ports; i++) {
> >> -		if (data->vbus_pin[i])
> >> -			at91_set_gpio_output(data->vbus_pin[i], 0);
> >> +		if (data->vbus_pin[i]) {
> >> +			gpio_request(data->vbus_pin[i], "usb host vbus");
> >> +			gpio_direction_output(data->vbus_pin[i], 0);
> >> +		}
> >>  	}
> > as you do the same think for ehci & ohci why not factorize it?
> 
> Just because you may choose only one or the other controller.
> For instance, if you only want ohci, you can disable ehci selection and
> always have its configuration done.
I agree but both configuration function share some code which could be
factorize, is it not?

Best Regards,
J.

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
@ 2009-09-17  9:00                       ` Jean-Christophe PLAGNIOL-VILLARD
  0 siblings, 0 replies; 20+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2009-09-17  9:00 UTC (permalink / raw)
  To: linux-arm-kernel

On 10:13 Thu 17 Sep     , Nicolas Ferre wrote:
> Jean-Christophe PLAGNIOL-VILLARD :
> > On 19:29 Wed 16 Sep     , Nicolas Ferre wrote:
> >> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
> >> call instead of the at91 specific one.
> >> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
> >> they are sharing the same pin.
> >>
> >> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> >> ---
> >> This patch is on top of at91sam9g45 USB integration one:
> >> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
> >> http://lkml.org/lkml/2009/6/9/221
> >>
> >>  arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
> >>  1 files changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> >> index 5be8cf2..7d939c0 100644
> >> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> >> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> >> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
> >>  
> >>  	/* Enable VBus control for UHP ports */
> >>  	for (i = 0; i < data->ports; i++) {
> >> -		if (data->vbus_pin[i])
> >> -			at91_set_gpio_output(data->vbus_pin[i], 0);
> >> +		if (data->vbus_pin[i]) {
> >> +			gpio_request(data->vbus_pin[i], "usb host vbus");
> >> +			gpio_direction_output(data->vbus_pin[i], 0);
> >> +		}
> >>  	}
> >>  
> >>  	usbh_ohci_data = *data;
> >> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
> >>  
> >>  	/* Enable VBus control for UHP ports */
> >>  	for (i = 0; i < data->ports; i++) {
> >> -		if (data->vbus_pin[i])
> >> -			at91_set_gpio_output(data->vbus_pin[i], 0);
> >> +		if (data->vbus_pin[i]) {
> >> +			gpio_request(data->vbus_pin[i], "usb host vbus");
> >> +			gpio_direction_output(data->vbus_pin[i], 0);
> >> +		}
> >>  	}
> > as you do the same think for ehci & ohci why not factorize it?
> 
> Just because you may choose only one or the other controller.
> For instance, if you only want ohci, you can disable ehci selection and
> always have its configuration done.
I agree but both configuration function share some code which could be
factorize, is it not?

Best Regards,
J.

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
  2009-09-17  9:00                       ` Jean-Christophe PLAGNIOL-VILLARD
@ 2009-09-18  8:40                         ` Nicolas Ferre
  -1 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-18  8:40 UTC (permalink / raw)
  To: Jean-Christophe PLAGNIOL-VILLARD
  Cc: linux-usb, patrice.vilchez, linux-kernel, david-b, avictor.za,
	haavard.skinnemoen, linux-arm-kernel

Jean-Christophe PLAGNIOL-VILLARD :
> On 10:13 Thu 17 Sep     , Nicolas Ferre wrote:
>> Jean-Christophe PLAGNIOL-VILLARD :
>>> On 19:29 Wed 16 Sep     , Nicolas Ferre wrote:
>>>> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
>>>> call instead of the at91 specific one.
>>>> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
>>>> they are sharing the same pin.
>>>>
>>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>> ---
>>>> This patch is on top of at91sam9g45 USB integration one:
>>>> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
>>>> http://lkml.org/lkml/2009/6/9/221
>>>>
>>>>  arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
>>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> index 5be8cf2..7d939c0 100644
>>>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>>>>  
>>>>  	/* Enable VBus control for UHP ports */
>>>>  	for (i = 0; i < data->ports; i++) {
>>>> -		if (data->vbus_pin[i])
>>>> -			at91_set_gpio_output(data->vbus_pin[i], 0);
>>>> +		if (data->vbus_pin[i]) {
>>>> +			gpio_request(data->vbus_pin[i], "usb host vbus");
>>>> +			gpio_direction_output(data->vbus_pin[i], 0);
>>>> +		}
>>>>  	}
>>>>  
>>>>  	usbh_ohci_data = *data;
>>>> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>>>>  
>>>>  	/* Enable VBus control for UHP ports */
>>>>  	for (i = 0; i < data->ports; i++) {
>>>> -		if (data->vbus_pin[i])
>>>> -			at91_set_gpio_output(data->vbus_pin[i], 0);
>>>> +		if (data->vbus_pin[i]) {
>>>> +			gpio_request(data->vbus_pin[i], "usb host vbus");
>>>> +			gpio_direction_output(data->vbus_pin[i], 0);
>>>> +		}
>>>>  	}
>>> as you do the same think for ehci & ohci why not factorize it?
>> Just because you may choose only one or the other controller.
>> For instance, if you only want ohci, you can disable ehci selection and
>> always have its configuration done.
> I agree but both configuration function share some code which could be
> factorize, is it not?

Well it is true that it is duplication *but* we are talking about only a
very few lines with very linear configuration instructions.
I do not think it is even worth doing.

Bye,
-- 
Nicolas Ferre


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45
@ 2009-09-18  8:40                         ` Nicolas Ferre
  0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-18  8:40 UTC (permalink / raw)
  To: linux-arm-kernel

Jean-Christophe PLAGNIOL-VILLARD :
> On 10:13 Thu 17 Sep     , Nicolas Ferre wrote:
>> Jean-Christophe PLAGNIOL-VILLARD :
>>> On 19:29 Wed 16 Sep     , Nicolas Ferre wrote:
>>>> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
>>>> call instead of the at91 specific one.
>>>> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
>>>> they are sharing the same pin.
>>>>
>>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>>> ---
>>>> This patch is on top of at91sam9g45 USB integration one:
>>>> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
>>>> http://lkml.org/lkml/2009/6/9/221
>>>>
>>>>  arch/arm/mach-at91/at91sam9g45_devices.c |   12 ++++++++----
>>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> index 5be8cf2..7d939c0 100644
>>>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>>>>  
>>>>  	/* Enable VBus control for UHP ports */
>>>>  	for (i = 0; i < data->ports; i++) {
>>>> -		if (data->vbus_pin[i])
>>>> -			at91_set_gpio_output(data->vbus_pin[i], 0);
>>>> +		if (data->vbus_pin[i]) {
>>>> +			gpio_request(data->vbus_pin[i], "usb host vbus");
>>>> +			gpio_direction_output(data->vbus_pin[i], 0);
>>>> +		}
>>>>  	}
>>>>  
>>>>  	usbh_ohci_data = *data;
>>>> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>>>>  
>>>>  	/* Enable VBus control for UHP ports */
>>>>  	for (i = 0; i < data->ports; i++) {
>>>> -		if (data->vbus_pin[i])
>>>> -			at91_set_gpio_output(data->vbus_pin[i], 0);
>>>> +		if (data->vbus_pin[i]) {
>>>> +			gpio_request(data->vbus_pin[i], "usb host vbus");
>>>> +			gpio_direction_output(data->vbus_pin[i], 0);
>>>> +		}
>>>>  	}
>>> as you do the same think for ehci & ohci why not factorize it?
>> Just because you may choose only one or the other controller.
>> For instance, if you only want ohci, you can disable ehci selection and
>> always have its configuration done.
> I agree but both configuration function share some code which could be
> factorize, is it not?

Well it is true that it is duplication *but* we are talking about only a
very few lines with very linear configuration instructions.
I do not think it is even worth doing.

Bye,
-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration
  2009-06-19  9:26           ` David Brownell
  2009-09-16 16:17               ` Nicolas Ferre
@ 2009-09-21 20:49             ` Andrew Victor
  2009-09-23 15:31               ` Nicolas Ferre
  1 sibling, 1 reply; 20+ messages in thread
From: Andrew Victor @ 2009-09-21 20:49 UTC (permalink / raw)
  To: David Brownell
  Cc: Haavard Skinnemoen, Nicolas Ferre, linux-usb, linux-kernel,
	patrice.vilchez

hi David,

> On Friday 19 June 2009, Haavard Skinnemoen wrote:
>> David Brownell wrote:
>> > > --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>> > > +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>
>> > > + /* Enable VBus control for UHP ports */
>> > > + for (i = 0; i < data->ports; i++) {
>> > > +         if (data->vbus_pin[i])
>> > > +                 at91_set_gpio_output(data->vbus_pin[i], 0);
>> >
>> > This should gpio_request() and gpio_direction_output().
>>
>> Hmm...I thought the driver was supposed to call gpio_request(), not the
>> platform code?
>
> In some cases.  This isn't a good case for that.  Especially
> if it's going to call gpio_direction_output() ... which needs
> gpio_request() to have been done first.

I have to agree with Haavard on this - it's really not clear if
gpio_request() should be called in the platform-code or in the driver.

If the platform code performs a gpio_request() then surely it needs to
call a gpio_free() after configuring the pin.
Otherwise the driver's initialization code performs another
gpio_request() for that pin, but it is still "in-use" by the platform
code.

Also Documentation/gpio.txt doesn't say if a GPIO pin should even
retain it's configured state across after a gpio_free().


So for the core AT91 platform code, I'd continue to use the
AT91-specific GPIO configuration functions.
The drivers should perform the gpio_request() / gpio_free(), and it
can still call gpio_direction_output() if necessary.


Regards,
  Andrew Victor

^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration
  2009-09-21 20:49             ` [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration Andrew Victor
@ 2009-09-23 15:31               ` Nicolas Ferre
  0 siblings, 0 replies; 20+ messages in thread
From: Nicolas Ferre @ 2009-09-23 15:31 UTC (permalink / raw)
  To: Andrew Victor
  Cc: David Brownell, Haavard Skinnemoen, linux-usb, linux-kernel,
	patrice.vilchez

Andrew Victor :
> hi David,
> 
>> On Friday 19 June 2009, Haavard Skinnemoen wrote:
>>> David Brownell wrote:
>>>>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>>>>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>>> + /* Enable VBus control for UHP ports */
>>>>> + for (i = 0; i < data->ports; i++) {
>>>>> +         if (data->vbus_pin[i])
>>>>> +                 at91_set_gpio_output(data->vbus_pin[i], 0);
>>>> This should gpio_request() and gpio_direction_output().
>>> Hmm...I thought the driver was supposed to call gpio_request(), not the
>>> platform code?
>> In some cases.  This isn't a good case for that.  Especially
>> if it's going to call gpio_direction_output() ... which needs
>> gpio_request() to have been done first.
> 
> I have to agree with Haavard on this - it's really not clear if
> gpio_request() should be called in the platform-code or in the driver.
> 
> If the platform code performs a gpio_request() then surely it needs to
> call a gpio_free() after configuring the pin.
> Otherwise the driver's initialization code performs another
> gpio_request() for that pin, but it is still "in-use" by the platform
> code.
> 
> Also Documentation/gpio.txt doesn't say if a GPIO pin should even
> retain it's configured state across after a gpio_free().
> 
> 
> So for the core AT91 platform code, I'd continue to use the
> AT91-specific GPIO configuration functions.
> The drivers should perform the gpio_request() / gpio_free(), and it
> can still call gpio_direction_output() if necessary.

Fair enough. So I forget my gpiolib patch on top of the former USB
integration one.

With your Ack, I will publish it to Russell's patch tracking system...

Bye,
-- 
Nicolas Ferre


^ permalink raw reply	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2009-09-23 15:31 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-09 11:38 at91/USB: high speed USB support for at91sam9g45 series Nicolas Ferre
2009-06-09 11:38 ` [PATCH 1/2] at91/USB: Add USB drivers " Nicolas Ferre
2009-06-09 11:38   ` Nicolas Ferre
2009-06-09 11:38   ` [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration Nicolas Ferre
2009-06-09 11:38     ` Nicolas Ferre
2009-06-19  7:43       ` David Brownell
2009-06-19  7:51         ` Haavard Skinnemoen
2009-06-19  9:26           ` David Brownell
2009-09-16 16:17             ` Nicolas Ferre
2009-09-16 16:17               ` Nicolas Ferre
2009-09-16 17:29               ` [PATCH] at91: use gpiolib calls for USB vbus pin on at91sam9g45 Nicolas Ferre
2009-09-16 17:29                 ` Nicolas Ferre
2009-09-16 21:53                 ` Jean-Christophe PLAGNIOL-VILLARD
2009-09-16 21:53                   ` Jean-Christophe PLAGNIOL-VILLARD
2009-09-17  8:13                   ` Nicolas Ferre
2009-09-17  8:13                     ` Nicolas Ferre
2009-09-17  9:00                     ` Jean-Christophe PLAGNIOL-VILLARD
2009-09-17  9:00                       ` Jean-Christophe PLAGNIOL-VILLARD
2009-09-18  8:40                       ` Nicolas Ferre
2009-09-18  8:40                         ` Nicolas Ferre
2009-09-21 20:49             ` [PATCH 2/2] at91/USB: at91sam9g45 series USB host integration Andrew Victor
2009-09-23 15:31               ` Nicolas Ferre

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.