On Wed, Jan 08, 2014 at 01:23:04PM +0000, 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 +0000, 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 <andreas@xxxxxxxxxxx> > >> --- > >> > >> 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 0000000..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 plug&play. > >> + > >> +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 plug&play 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 > >> + 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 one version of the > driver to another - especially if the change does not makes it into > 3.14. There are no dts files for SPARC. Given this is already out there, let's leave it as-is. > > > > > > How many entries are expected? > > I'll add text on that. > > > A specific driver should have no relevance to the binding. Just say "if > > not 1024". > > Sure! > > > [...] > > > >> +/* 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. > > Sure, it makes for better looking code in general as well. > > > > >> + 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) > >> +{ > >> + struct gr_udc *dev; > >> + struct resource *res; > >> + struct gr_regs __iomem *regs; > >> + int retval; > >> + u32 status; > >> + > >> + dev = devm_kzalloc(&ofdev->dev, sizeof(*dev), GFP_KERNEL); > >> + if (!dev) > >> + return -ENOMEM; > >> + dev->dev = &ofdev->dev; > >> + > >> + res = platform_get_resource(ofdev, IORESOURCE_MEM, 0); > >> + regs = devm_ioremap_resource(dev->dev, res); > >> + if (IS_ERR(regs)) > >> + return PTR_ERR(regs); > >> + > >> + dev->irq = irq_of_parse_and_map(dev->dev->of_node, 0); > >> + if (!dev->irq) { > >> + dev_err(dev->dev, "No irq found\n"); > >> + return -ENODEV; > >> + } > > > > The platform_device probing code will have parsed and mapped the irq > > already. You can use platform_get_irq(ofdev->dev, 0) here. > > Sure > > > Also, naming the platform_device ofdev is confusing. It has nothing to > > do with OF, and the more common pdev name would be far clearer. > > Sure > > >> + > >> + /* Some core configurations has separate irqs for IN and OUT events */ > >> + dev->irqi = irq_of_parse_and_map(dev->dev->of_node, 1); > >> + if (dev->irqi) { > >> + dev->irqo = irq_of_parse_and_map(dev->dev->of_node, 2); > >> + if (!dev->irqo) { > >> + dev_err(dev->dev, "Found irqi but not irqo\n"); > >> + return -ENODEV; > >> + } > >> + } > > > > Likewise here you can use platform_get_irq. > > > > [...] > > > >> +static struct of_device_id gr_match[] = { > >> + {.name = "GAISLER_USBDC"}, > >> + {.name = "01_021"}, > >> + {}, > > > > This seems extremely odd to me. I think a compatible string would be far > > preferable. > > As explained above it is the standard method of matching for SPARC. Ok. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
