Re: [PATCH v6] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC
On 2014-01-06 17:22, Mark Rutland wrote:
Hi,
Apologies for the late reply, I wasn't able to access my mail much over
the Christmas break.
The patch is already applied to both the next branch of Felipe Balbi's
usb/next branch and merged from there into Greg Kroah-Hartman's
usb/usb-next branch, so it might be too late for including in the
initial patch.
I'll be happy to send a patch series to improve things as indicated
inline below. There are no functional bugs running on SPARC which is the
ordinary environment for this core, so adding patches to do these
improvements should work fine.
On Mon, Dec 23, 2013 at 08:25:49PM +, Andreas Larsson wrote:
This adds an UDC driver for GRUSBDC USB Device Controller cores available in the
GRLIB VHDL IP core library. The driver only supports DMA mode.
Signed-off-by: Andreas Larsson andr...@gaisler.com
---
Differences since v1:
- Use hexprint for debug printoutes instead of homemade equivalent
- Use the dev_* printk variants over the board
- Get rid of unnecessary casts and includes
- Use USB_STATE_NOTATTACHED instead of USB_STATE_ATTACHED for clarity
- Get rid of commented out VERBOSE_DEBUG definition
- Do not devm-allocate any requests
- Get rid of unnecessary resqueduling of the workqueue handler
- Make sure that gadget setup function is called with interrupts disabled
Differences since v2:
- Fixed an error printout
- Got rid of the work queue in favor of threaded interrupts
Differences since v3:
- Disable interrupts when locking spinlocks instead of using
local_irq_save deeper within critical section
Differences since v4:
- Set quirk_ep_out_aligned_size
- Use usb_ep_set_maxpacket_limit
- Add devicetree documentation
Differences since v5:
- Declare unexpected spin_unlock and spin_lock for sparse
- Fix a bad comment
- Use ACCESS_ONCE instead of gr_read32 when checking for update of dma
descriptor
Documentation/devicetree/bindings/usb/gr-udc.txt | 28 +
drivers/usb/gadget/Kconfig |7 +
drivers/usb/gadget/Makefile |1 +
drivers/usb/gadget/gr_udc.c | 2242 ++
drivers/usb/gadget/gr_udc.h | 220 +++
5 files changed, 2498 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/gr-udc.txt
create mode 100644 drivers/usb/gadget/gr_udc.c
create mode 100644 drivers/usb/gadget/gr_udc.h
diff --git a/Documentation/devicetree/bindings/usb/gr-udc.txt
b/Documentation/devicetree/bindings/usb/gr-udc.txt
new file mode 100644
index 000..0c5118f
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/gr-udc.txt
@@ -0,0 +1,28 @@
+USB Peripheral Controller driver for Aeroflex Gaisler GRUSBDC.
+
+The GRUSBDC USB Device Controller core is available in the GRLIB VHDL
+IP core library.
+
+Note: In the ordinary environment for the core, a Leon SPARC system,
+these properties are built from information in the AMBA plugplay.
+
+Required properties:
+
+- name : Should be GAISLER_USBDC or 01_021
What's this for?
Why is this not matched using a compatible string?
What does 01_021 mean?
Regarding using name, this is standard for SPARC. The names in the
device tree originates from the PROM.
The name field is the actually the first field checked for a match in
of_match_node, followed by type then compatible. See
http://lxr.free-electrons.com/source/drivers/of/base.c?v=3.12#L723
Examples can be found among others in:
- drivers/watchdog/riowd.c
- drivers/watchdog/cpwd.c
- drivers/mtd/maps/sun_uflash.c
- drivers/input/misc/sparcspkr.c
- drivers/input/serio/i8042-sparcio.h
- drivers/hwmon/ultra45_env.c
As for the 01_021, the LEON SPARC systems uses a plugplay to identify
different IP cores in the system. When the PROM is unaware of the name
of a certain core, the name field presented from the PROM will be on
this form. This is standard handling for LEON SPARC drivers.
Examples of this can be found in:
- drivers/gpio/gpio-grgpio.c
- drivers/usb/host/uhci-grlib.c
- drivers/usb/host/ehci-grlib.c
- drivers/video/grvga.c
- drivers/net/can/grcan.c
- drivers/net/ethernet/aeroflex/greth.c
- drivers/tty/serial/apbuart.c
- drivers/gpio/gpio-grgpio.c
+
+- reg : Address and length of the register set for the device
+
+- interrupts : Interrupt numbers for this device
How many, and what do they correspond to?
I'll add text on that
+
+Optional properties:
+
+- epobufsizes : An array of buffer sizes for OUT endpoints. If the property is
+ not present, or for endpoints outside of the array, 1024 is assumed by
+ the driver.
+
+- epibufsizes : An array of buffer sizes for IN endpoints. If the property is
+ not present, or for endpoints outside of the array, 1024 is assumed by
+ the driver.
These names are rather opaque. Something like {input,output}-buf-lens
would be far clearer.
Unless Felipe wants to merge with the original patch I don't think it is
a good idea to have the property name change from
Re: [PATCH v6] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC
On Wed, Jan 08, 2014 at 01:23:04PM +, Andreas Larsson wrote: On 2014-01-06 17:22, Mark Rutland wrote: Hi, Apologies for the late reply, I wasn't able to access my mail much over the Christmas break. The patch is already applied to both the next branch of Felipe Balbi's usb/next branch and merged from there into Greg Kroah-Hartman's usb/usb-next branch, so it might be too late for including in the initial patch. I'll be happy to send a patch series to improve things as indicated inline below. There are no functional bugs running on SPARC which is the ordinary environment for this core, so adding patches to do these improvements should work fine. On Mon, Dec 23, 2013 at 08:25:49PM +, Andreas Larsson wrote: This adds an UDC driver for GRUSBDC USB Device Controller cores available in the GRLIB VHDL IP core library. The driver only supports DMA mode. Signed-off-by: Andreas Larsson andr...@gaisler.com --- Differences since v1: - Use hexprint for debug printoutes instead of homemade equivalent - Use the dev_* printk variants over the board - Get rid of unnecessary casts and includes - Use USB_STATE_NOTATTACHED instead of USB_STATE_ATTACHED for clarity - Get rid of commented out VERBOSE_DEBUG definition - Do not devm-allocate any requests - Get rid of unnecessary resqueduling of the workqueue handler - Make sure that gadget setup function is called with interrupts disabled Differences since v2: - Fixed an error printout - Got rid of the work queue in favor of threaded interrupts Differences since v3: - Disable interrupts when locking spinlocks instead of using local_irq_save deeper within critical section Differences since v4: - Set quirk_ep_out_aligned_size - Use usb_ep_set_maxpacket_limit - Add devicetree documentation Differences since v5: - Declare unexpected spin_unlock and spin_lock for sparse - Fix a bad comment - Use ACCESS_ONCE instead of gr_read32 when checking for update of dma descriptor Documentation/devicetree/bindings/usb/gr-udc.txt | 28 + drivers/usb/gadget/Kconfig |7 + drivers/usb/gadget/Makefile |1 + drivers/usb/gadget/gr_udc.c | 2242 ++ drivers/usb/gadget/gr_udc.h | 220 +++ 5 files changed, 2498 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/gr-udc.txt create mode 100644 drivers/usb/gadget/gr_udc.c create mode 100644 drivers/usb/gadget/gr_udc.h diff --git a/Documentation/devicetree/bindings/usb/gr-udc.txt b/Documentation/devicetree/bindings/usb/gr-udc.txt new file mode 100644 index 000..0c5118f --- /dev/null +++ b/Documentation/devicetree/bindings/usb/gr-udc.txt @@ -0,0 +1,28 @@ +USB Peripheral Controller driver for Aeroflex Gaisler GRUSBDC. + +The GRUSBDC USB Device Controller core is available in the GRLIB VHDL +IP core library. + +Note: In the ordinary environment for the core, a Leon SPARC system, +these properties are built from information in the AMBA plugplay. + +Required properties: + +- name : Should be GAISLER_USBDC or 01_021 What's this for? Why is this not matched using a compatible string? What does 01_021 mean? Regarding using name, this is standard for SPARC. The names in the device tree originates from the PROM. The name field is the actually the first field checked for a match in of_match_node, followed by type then compatible. See http://lxr.free-electrons.com/source/drivers/of/base.c?v=3.12#L723 Examples can be found among others in: - drivers/watchdog/riowd.c - drivers/watchdog/cpwd.c - drivers/mtd/maps/sun_uflash.c - drivers/input/misc/sparcspkr.c - drivers/input/serio/i8042-sparcio.h - drivers/hwmon/ultra45_env.c As for the 01_021, the LEON SPARC systems uses a plugplay to identify different IP cores in the system. When the PROM is unaware of the name of a certain core, the name field presented from the PROM will be on this form. This is standard handling for LEON SPARC drivers. Examples of this can be found in: - drivers/gpio/gpio-grgpio.c - drivers/usb/host/uhci-grlib.c - drivers/usb/host/ehci-grlib.c - drivers/video/grvga.c - drivers/net/can/grcan.c - drivers/net/ethernet/aeroflex/greth.c - drivers/tty/serial/apbuart.c - drivers/gpio/gpio-grgpio.c OK. Sorry for the confusion there. + +- reg : Address and length of the register set for the device + +- interrupts : Interrupt numbers for this device How many, and what do they correspond to? I'll add text on that + +Optional properties: + +- epobufsizes : An array of buffer sizes for OUT endpoints. If the property is + not present, or for endpoints outside of the array, 1024 is assumed by + the driver. + +- epibufsizes : An array of buffer sizes for IN endpoints. If the property is +
Re: [PATCH v6] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC
Hi,
Apologies for the late reply, I wasn't able to access my mail much over
the Christmas break.
On Mon, Dec 23, 2013 at 08:25:49PM +, Andreas Larsson wrote:
This adds an UDC driver for GRUSBDC USB Device Controller cores available in
the
GRLIB VHDL IP core library. The driver only supports DMA mode.
Signed-off-by: Andreas Larsson andr...@gaisler.com
---
Differences since v1:
- Use hexprint for debug printoutes instead of homemade equivalent
- Use the dev_* printk variants over the board
- Get rid of unnecessary casts and includes
- Use USB_STATE_NOTATTACHED instead of USB_STATE_ATTACHED for clarity
- Get rid of commented out VERBOSE_DEBUG definition
- Do not devm-allocate any requests
- Get rid of unnecessary resqueduling of the workqueue handler
- Make sure that gadget setup function is called with interrupts disabled
Differences since v2:
- Fixed an error printout
- Got rid of the work queue in favor of threaded interrupts
Differences since v3:
- Disable interrupts when locking spinlocks instead of using
local_irq_save deeper within critical section
Differences since v4:
- Set quirk_ep_out_aligned_size
- Use usb_ep_set_maxpacket_limit
- Add devicetree documentation
Differences since v5:
- Declare unexpected spin_unlock and spin_lock for sparse
- Fix a bad comment
- Use ACCESS_ONCE instead of gr_read32 when checking for update of dma
descriptor
Documentation/devicetree/bindings/usb/gr-udc.txt | 28 +
drivers/usb/gadget/Kconfig |7 +
drivers/usb/gadget/Makefile |1 +
drivers/usb/gadget/gr_udc.c | 2242
++
drivers/usb/gadget/gr_udc.h | 220 +++
5 files changed, 2498 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/gr-udc.txt
create mode 100644 drivers/usb/gadget/gr_udc.c
create mode 100644 drivers/usb/gadget/gr_udc.h
diff --git a/Documentation/devicetree/bindings/usb/gr-udc.txt
b/Documentation/devicetree/bindings/usb/gr-udc.txt
new file mode 100644
index 000..0c5118f
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/gr-udc.txt
@@ -0,0 +1,28 @@
+USB Peripheral Controller driver for Aeroflex Gaisler GRUSBDC.
+
+The GRUSBDC USB Device Controller core is available in the GRLIB VHDL
+IP core library.
+
+Note: In the ordinary environment for the core, a Leon SPARC system,
+these properties are built from information in the AMBA plugplay.
+
+Required properties:
+
+- name : Should be GAISLER_USBDC or 01_021
What's this for?
Why is this not matched using a compatible string?
What does 01_021 mean?
+
+- reg : Address and length of the register set for the device
+
+- interrupts : Interrupt numbers for this device
How many, and what do they correspond to?
+
+Optional properties:
+
+- epobufsizes : An array of buffer sizes for OUT endpoints. If the property
is
+ not present, or for endpoints outside of the array, 1024 is assumed by
+ the driver.
+
+- epibufsizes : An array of buffer sizes for IN endpoints. If the property is
+ not present, or for endpoints outside of the array, 1024 is assumed by
+ the driver.
These names are rather opaque. Something like {input,output}-buf-lens
would be far clearer.
How many entries are expected?
A specific driver should have no relevance to the binding. Just say if
not 1024.
[...]
+/* Must be called with dev-lock held */
+static int gr_udc_init(struct gr_udc *dev)
+{
+ struct device_node *np = dev-dev-of_node;
+ u32 epctrl_val;
+ u32 dmactrl_val;
+ int i;
+ int ret = 0;
+ u32 *bufsizes;
+ u32 bufsize;
+ int len;
+
+ gr_set_address(dev, 0);
+
+ INIT_LIST_HEAD(dev-gadget.ep_list);
+ dev-gadget.speed = USB_SPEED_UNKNOWN;
+ dev-gadget.ep0 = dev-epi[0].ep;
+
+ INIT_LIST_HEAD(dev-ep_list);
+ gr_set_ep0state(dev, GR_EP0_DISCONNECT);
+
+ bufsizes = (u32 *)of_get_property(np, epobufsizes, len);
of_get_property gives you the raw property value bye string, which is
_not_ a u32 pointer -- it's not necessarily the same endianness as the
kernel. Please use an appropriate accessor.
+ len /= sizeof(u32);
+ for (i = 0; i dev-nepo; i++) {
+ bufsize = (bufsizes i len) ? bufsizes[i] : 1024;
You can use of_property_read_u32_index within the loop for this:
if (of_property_read_u32_index(np, epobufsizes, bufsize)
bufsize = 1024;
+ ret = gr_ep_init(dev, i, 0, bufsize);
+ if (ret)
+ return ret;
+ }
+
+ bufsizes = (u32 *)of_get_property(np, epibufsizes, len);
+ len /= sizeof(u32);
+ for (i = 0; i dev-nepi; i++) {
+ bufsize = (bufsizes i len) ? bufsizes[i] : 1024;
Likewise here.
[...]
+static int gr_probe(struct platform_device *ofdev)
+{
+
Re: [PATCH v6] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC
Hi, On Mon, Dec 23, 2013 at 09:25:49PM +0100, Andreas Larsson wrote: @@ -216,6 +216,13 @@ config USB_FOTG210_UDC Say y to link the driver statically, or m to build a dynamically linked module called fotg210_udc. +config USB_GR_UDC + tristate Aeroflex Gaisler GRUSBDC USB Peripheral Controller Driver + depends on OF HAS_DMA I applied to my testing/next branch after removing OF dependency since OF provides empty stubs. I checked that it builds cleanly in x86 and ARM. -- balbi signature.asc Description: Digital signature
