Message-ID: <20140108134347.GI6701@e106331-lin.cambridge.arm.com>
Date:	Wed, 8 Jan 2014 13:43:47 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Andreas Larsson <andreas@...sler.com>
Cc:	Felipe Balbi <balbi@...com>,
	Robert Baldyga <r.baldyga@...sung.com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"software@...sler.com" <software@...sler.com>
Subject: Re: [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@...sler.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists