lkml.org 
[lkml]   [2014]   [Jan]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v6] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC
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@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 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.


\
 
 \ /
  Last update: 2014-01-08 17:41    [W:1.358 / U:0.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site