I'm playing around with a PIC based project at home (not an Intel
activity) and found I needed a usb driver to talk to the boot loader
so I can program my USB Bitwhacker with new custom firmware. The
following adds the pic18bl driver to the kernel. Its pretty simple
and is somewhat based on bits of a libusb driver that does some of
what this driver does.
What do you think?
--mgross
Singed-off-by: Mark Gross <[email protected]>
---
Index: linux-2.6.24-rc6/drivers/usb/misc/pic18bl.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.24-rc6/drivers/usb/misc/pic18bl.c 2007-12-29 11:21:14.000000000 -0800
@@ -0,0 +1,627 @@
+/*
+ * PIC18 usb boot loader driver based on the MicroChip firmware, and a user
+ * mode libusb driver that does some of the same operations only different.
+ * Based on the skeleton driver
+ *
+ * --mgross
+ *
+ * USB Skeleton driver - 2.2
+ *
+ * Copyright (C) 2001-2004 Greg Kroah-Hartman ([email protected])
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ *
+ * This driver is based on the 2.6.3 version of drivers/usb/usb-pic18bleton.c
+ * but has been rewritten to be easier to read and use.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/kref.h>
+#include <asm/uaccess.h>
+#include <linux/usb.h>
+#include <linux/mutex.h>
+
+/* Define these values to match your devices */
+#define USB_PIC18BL_VENDOR_ID 0x04d8
+#define USB_PIC18BL_PRODUCT_ID 0x000b
+
+/* table of devices that work with this driver */
+static struct usb_device_id pic18bl_table [] = {
+ { USB_DEVICE(USB_PIC18BL_VENDOR_ID, USB_PIC18BL_PRODUCT_ID) },
+ { } /* Terminating entry */
+};
+MODULE_DEVICE_TABLE(usb, pic18bl_table);
+
+
+/* Get a minor range for your devices from the usb maintainer */
+#define USB_PIC18BL_MINOR_BASE 192
+
+/* our private defines. if this grows any larger, use your own .h file */
+#define MAX_TRANSFER (PAGE_SIZE - 512)
+/* MAX_TRANSFER is chosen so that the VM is not stressed by
+ allocations > PAGE_SIZE and the number of packets in a page
+ is an integer 512 is the largest possible packet on EHCI */
+#define WRITES_IN_FLIGHT 8
+/* arbitrarily chosen */
+
+/* structs and information based on boot loader firmware */
+#define BOOT_EP_SIZE 64
+#define OVER_HEAD 5 /*Overhead: <CMD_CODE><LEN><ADDR:3>*/
+#define DATA_SIZE (BOOT_EP_SIZE - OVER_HEAD)
+
+#define READ_VERSION 0x00
+#define READ_FLASH 0x01
+#define WRITE_FLASH 0x02
+#define ERASE_FLASH 0x03
+#define READ_EEDATA 0x04
+#define WRITE_EEDATA 0x05
+#define READ_CONFIG 0x06
+#define WRITE_CONFIG 0x07
+#define UPDATE_LED 0x32
+#define RESET 0xFF
+
+struct address {
+ u8 low;
+ u8 high;
+ u8 upper;
+} __attribute__ ((packed));
+
+union boot_data_buffer {
+ u8 buffer[BOOT_EP_SIZE];
+ struct {
+ u8 cmd;
+ u8 len;
+ struct address addr;
+ u8 data[DATA_SIZE];
+ } __attribute__ ((packed)) data;
+ struct {
+ u8 cmd; /* should always == 32 */
+ u8 led_num; /* should only be 3 or 4 */
+ u8 led_status;
+ } __attribute__ ((packed)) led;
+};
+
+
+/* Structure to hold all of our device specific stuff */
+struct usb_pic18bl {
+ struct usb_device *udev;
+ struct usb_interface *interface;
+ struct semaphore limit_sem;
+ struct usb_anchor submitted;
+ unsigned char *bulk_in_buffer;
+ size_t bulk_in_size;
+ __u8 bulk_in_endpointAddr;
+ __u8 bulk_out_endpointAddr;
+ int errors;
+ int open_count;
+ spinlock_t err_lock;
+ struct kref kref;
+ struct mutex io_mutex;
+};
+#define to_pic18bl_dev(d) container_of(d, struct usb_pic18bl, kref)
+
+static struct usb_driver pic18bl_driver;
+static void pic18bl_draw_down(struct usb_pic18bl *dev);
+
+static void pic18bl_delete(struct kref *kref)
+{
+ struct usb_pic18bl *dev = to_pic18bl_dev(kref);
+
+ usb_put_dev(dev->udev);
+ kfree(dev->bulk_in_buffer);
+ kfree(dev);
+}
+
+static int pic18bl_open(struct inode *inode, struct file *file)
+{
+ struct usb_pic18bl *dev;
+ struct usb_interface *interface;
+ int subminor;
+ int retval = 0;
+
+ subminor = iminor(inode);
+
+ interface = usb_find_interface(&pic18bl_driver, subminor);
+ if (!interface) {
+ err("%s - error, can't find device for minor %d",
+ __FUNCTION__, subminor);
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ dev = usb_get_intfdata(interface);
+ if (!dev) {
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* increment our usage count for the device */
+ kref_get(&dev->kref);
+
+ /* lock the device to allow correctly handling errors
+ * in resumption */
+ mutex_lock(&dev->io_mutex);
+
+ if (!dev->open_count++) {
+ retval = usb_autopm_get_interface(interface);
+ if (retval) {
+ dev->open_count--;
+ mutex_unlock(&dev->io_mutex);
+ kref_put(&dev->kref, pic18bl_delete);
+ goto exit;
+ }
+ } else {
+ retval = -EBUSY;
+ dev->open_count--;
+ mutex_unlock(&dev->io_mutex);
+ kref_put(&dev->kref, pic18bl_delete);
+ goto exit;
+ }
+ /* prevent the device from being autosuspended */
+ /* save our object in the file's private structure */
+ file->private_data = dev;
+
+ mutex_unlock(&dev->io_mutex);
+exit:
+ return retval;
+}
+
+static int pic18bl_release(struct inode *inode, struct file *file)
+{
+ struct usb_pic18bl *dev;
+
+ dev = (struct usb_pic18bl *)file->private_data;
+ if (dev == NULL)
+ return -ENODEV;
+
+ /* allow the device to be autosuspended */
+ mutex_lock(&dev->io_mutex);
+ if (!--dev->open_count && dev->interface)
+ usb_autopm_put_interface(dev->interface);
+ mutex_unlock(&dev->io_mutex);
+
+ /* decrement the count on our device */
+ kref_put(&dev->kref, pic18bl_delete);
+ return 0;
+}
+
+static int pic18bl_flush(struct file *file, fl_owner_t id)
+{
+ struct usb_pic18bl *dev;
+ int res;
+
+ dev = (struct usb_pic18bl *)file->private_data;
+ if (dev == NULL)
+ return -ENODEV;
+
+ /* wait for io to stop */
+ mutex_lock(&dev->io_mutex);
+ pic18bl_draw_down(dev);
+
+ /* read out errors, leave subsequent opens a clean slate */
+ spin_lock_irq(&dev->err_lock);
+ res = dev->errors ? (dev->errors == -EPIPE ? -EPIPE : -EIO) : 0;
+ dev->errors = 0;
+ spin_unlock_irq(&dev->err_lock);
+
+ mutex_unlock(&dev->io_mutex);
+
+ return res;
+}
+
+static ssize_t pic18bl_read(struct file *file, char *buffer, size_t count,
+ loff_t *ppos)
+{
+ struct usb_pic18bl *dev;
+ int retval;
+ int bytes_read;
+
+ dev = (struct usb_pic18bl *)file->private_data;
+
+ mutex_lock(&dev->io_mutex);
+ if (!dev->interface) {
+ /* disconnect() was called */
+ retval = -ENODEV;
+ goto exit;
+ }
+
+ /* do a blocking bulk read to get data from the device */
+ retval = usb_bulk_msg(dev->udev,
+ usb_rcvbulkpipe(dev->udev, dev->bulk_in_endpointAddr),
+ dev->bulk_in_buffer,
+ min(dev->bulk_in_size, count),
+ &bytes_read, 10000);
+
+ /* if the read was successful, copy the data to userspace */
+ if (!retval) {
+ if (copy_to_user(buffer, dev->bulk_in_buffer, bytes_read))
+ retval = -EFAULT;
+ else
+ retval = bytes_read;
+ }
+
+exit:
+ mutex_unlock(&dev->io_mutex);
+ return retval;
+}
+
+static void pic18bl_write_bulk_callback(struct urb *urb)
+{
+ struct usb_pic18bl *dev;
+
+ dev = (struct usb_pic18bl *)urb->context;
+
+ /* sync/async unlink faults aren't errors */
+ if (urb->status) {
+ if (!(urb->status == -ENOENT ||
+ urb->status == -ECONNRESET ||
+ urb->status == -ESHUTDOWN))
+ err("%s - nonzero write bulk status received: %d",
+ __FUNCTION__, urb->status);
+
+ spin_lock(&dev->err_lock);
+ dev->errors = urb->status;
+ spin_unlock(&dev->err_lock);
+ }
+
+ /* free up our allocated buffer */
+ usb_buffer_free(urb->dev, urb->transfer_buffer_length,
+ urb->transfer_buffer, urb->transfer_dma);
+ up(&dev->limit_sem);
+}
+
+static ssize_t pic18bl_write(struct file *file, const char *user_buffer,
+ size_t count, loff_t *ppos)
+{
+ struct usb_pic18bl *dev;
+ int retval = -1;
+ struct urb *urb = NULL;
+ union boot_data_buffer *buf = NULL;
+ size_t writesize = min(count, sizeof(union boot_data_buffer));
+ int address = 0;
+
+ dev = (struct usb_pic18bl *)file->private_data;
+
+ /* verify that we actually have some data to write */
+ if (count == 0 || count > sizeof(union boot_data_buffer))
+ goto exit;
+
+ /* limit the number of URBs in flight to stop a user from using up all
+ * RAM
+ */
+ if (down_interruptible(&dev->limit_sem)) {
+ retval = -ERESTARTSYS;
+ goto exit;
+ }
+
+ spin_lock_irq(&dev->err_lock);
+ retval = dev->errors;
+ if (retval < 0) {
+ /* any error is reported once */
+ dev->errors = 0;
+ /* to preserve notifications about reset */
+ retval = (retval == -EPIPE) ? retval : -EIO;
+ }
+ spin_unlock_irq(&dev->err_lock);
+ if (retval < 0)
+ goto error;
+
+ /* create a urb, and a buffer for it, and copy the data to the urb */
+ urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!urb) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ buf = usb_buffer_alloc(dev->udev, writesize, GFP_KERNEL,
+ &urb->transfer_dma);
+ if (!buf) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ if (copy_from_user(buf, user_buffer, writesize)) {
+ retval = -EFAULT;
+ goto error;
+ }
+ /* check buffer for invalid noise */
+ address = buf->data.addr.low +
+ 256*(buf->data.addr.high + 256 * buf->data.addr.upper);
+ printk(KERN_ERR "pic18bl_write: writesize = %d cmd = %d, len = %d,\
+ add = 0x%02x%02x%02x\n",writesize, buf->data.cmd, buf->data.len,
+ buf->data.addr.upper, buf->data.addr.high, buf->data.addr.low);
+
+ switch (buf->data.cmd) {
+ /*TODO add more checks for bogus data */
+ /* TODO add code to make read / write transactions impossible to
+ * step on each other. i.e. starting a write transaction while
+ * a read is in flight would be bad.
+ */
+ case READ_VERSION:
+ case RESET:
+ if (buf->data.len + 5 != writesize) {
+ retval = -EINVAL;
+ goto error;
+ }
+ break;
+ case WRITE_FLASH:
+ case ERASE_FLASH:
+ if (address < 0x800) {
+ printk(KERN_ERR "pic18bl: attempt to step on boot "
+ "loader memory blocked by driver \n");
+ retval = -EINVAL;
+ goto error;
+ }
+ case READ_EEDATA:
+ case WRITE_EEDATA:
+ case WRITE_CONFIG:
+ case READ_FLASH:
+ case READ_CONFIG:
+ break;
+ case UPDATE_LED:
+ /* boot.c in the fw code looks like this
+ * command is somewhat of a no-op. perhaps
+ * i should remove this guy????
+ */
+ if (buf->led.led_num != 3 ||
+ buf->led.led_num != 4)
+ goto error;
+
+ default:
+ /*free buf */
+ goto error;
+ }
+
+ /* this lock makes sure we don't submit URBs to gone devices */
+ mutex_lock(&dev->io_mutex);
+ if (!dev->interface) {
+ /* disconnect() was called */
+ mutex_unlock(&dev->io_mutex);
+ retval = -ENODEV;
+ goto error;
+ }
+
+ /* initialize the urb properly */
+ usb_fill_bulk_urb(urb, dev->udev,
+ usb_sndbulkpipe(dev->udev, dev->bulk_out_endpointAddr),
+ buf, writesize, pic18bl_write_bulk_callback, dev);
+ urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+ usb_anchor_urb(urb, &dev->submitted);
+
+ /* send the data out the bulk port */
+ retval = usb_submit_urb(urb, GFP_KERNEL);
+ mutex_unlock(&dev->io_mutex);
+ if (retval) {
+ err("%s - failed submitting write urb, error %d", __FUNCTION__,
+ retval);
+ goto error_unanchor;
+ }
+
+ /* release our reference to this urb, the USB core will eventually free
+ * it entirely */
+ usb_free_urb(urb);
+
+
+ return writesize;
+
+error_unanchor:
+ usb_unanchor_urb(urb);
+error:
+ if (urb) {
+ usb_buffer_free(dev->udev, writesize, buf, urb->transfer_dma);
+ usb_free_urb(urb);
+ }
+ up(&dev->limit_sem);
+
+exit:
+ return retval;
+}
+
+static const struct file_operations pic18bl_fops = {
+ .owner = THIS_MODULE,
+ .read = pic18bl_read,
+ .write = pic18bl_write,
+ .open = pic18bl_open,
+ .release = pic18bl_release,
+ .flush = pic18bl_flush,
+};
+
+/*
+ * usb class driver info in order to get a minor number from the usb core,
+ * and to have the device registered with the driver core
+ */
+static struct usb_class_driver pic18bl_class = {
+ .name = "pic18bl%d",
+ .fops = &pic18bl_fops,
+ .minor_base = USB_PIC18BL_MINOR_BASE,
+};
+
+static int pic18bl_probe(struct usb_interface *interface,
+ const struct usb_device_id *id)
+{
+ struct usb_pic18bl *dev;
+ struct usb_host_interface *iface_desc;
+ struct usb_endpoint_descriptor *endpoint;
+ size_t buffer_size;
+ int i;
+ int retval = -ENOMEM;
+
+ /* allocate memory for our device state and initialize it */
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ err("Out of memory");
+ goto error;
+ }
+ kref_init(&dev->kref);
+ sema_init(&dev->limit_sem, WRITES_IN_FLIGHT);
+ mutex_init(&dev->io_mutex);
+ spin_lock_init(&dev->err_lock);
+ init_usb_anchor(&dev->submitted);
+
+ dev->udev = usb_get_dev(interface_to_usbdev(interface));
+ dev->interface = interface;
+
+ /* set up the endpoint information */
+ /* use only the first bulk-in and bulk-out endpoints */
+ iface_desc = interface->cur_altsetting;
+ for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
+ endpoint = &iface_desc->endpoint[i].desc;
+
+ if (!dev->bulk_in_endpointAddr &&
+ usb_endpoint_is_bulk_in(endpoint)) {
+ /* we found a bulk in endpoint */
+ buffer_size = le16_to_cpu(endpoint->wMaxPacketSize);
+ dev->bulk_in_size = buffer_size;
+ dev->bulk_in_endpointAddr = endpoint->bEndpointAddress;
+ dev->bulk_in_buffer = kmalloc(buffer_size, GFP_KERNEL);
+ if (!dev->bulk_in_buffer) {
+ err("Could not allocate bulk_in_buffer");
+ goto error;
+ }
+ }
+
+ if (!dev->bulk_out_endpointAddr &&
+ usb_endpoint_is_bulk_out(endpoint)) {
+ /* we found a bulk out endpoint */
+ dev->bulk_out_endpointAddr = endpoint->bEndpointAddress;
+ }
+ }
+ if (!(dev->bulk_in_endpointAddr && dev->bulk_out_endpointAddr)) {
+ err("Could not find both bulk-in and bulk-out endpoints");
+ goto error;
+ }
+
+ /* save our data pointer in this interface device */
+ usb_set_intfdata(interface, dev);
+
+ /* we can register the device now, as it is ready */
+ retval = usb_register_dev(interface, &pic18bl_class);
+ if (retval) {
+ /* something prevented us from registering this driver */
+ err("Not able to get a minor for this device.");
+ usb_set_intfdata(interface, NULL);
+ goto error;
+ }
+
+ /* let the user know what node this device is now attached to */
+ info("USB bit whaker device now attached to pic18bl-%d",
+ interface->minor);
+ return 0;
+
+error:
+ if (dev)
+ /* this frees allocated memory */
+ kref_put(&dev->kref, pic18bl_delete);
+ return retval;
+}
+
+static void pic18bl_disconnect(struct usb_interface *interface)
+{
+ struct usb_pic18bl *dev;
+ int minor = interface->minor;
+
+ dev = usb_get_intfdata(interface);
+ usb_set_intfdata(interface, NULL);
+
+ /* give back our minor */
+ usb_deregister_dev(interface, &pic18bl_class);
+
+ /* prevent more I/O from starting */
+ mutex_lock(&dev->io_mutex);
+ dev->interface = NULL;
+ mutex_unlock(&dev->io_mutex);
+
+ usb_kill_anchored_urbs(&dev->submitted);
+
+ /* decrement our usage count */
+ kref_put(&dev->kref, pic18bl_delete);
+
+ info("USB pic18bl #%d now disconnected", minor);
+}
+
+static void pic18bl_draw_down(struct usb_pic18bl *dev)
+{
+ int time;
+
+ time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000);
+ if (!time)
+ usb_kill_anchored_urbs(&dev->submitted);
+}
+
+static int pic18bl_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct usb_pic18bl *dev = usb_get_intfdata(intf);
+
+ if (!dev)
+ return 0;
+ pic18bl_draw_down(dev);
+ return 0;
+}
+
+static int pic18bl_resume(struct usb_interface *intf)
+{
+ return 0;
+}
+
+static int pic18bl_pre_reset(struct usb_interface *intf)
+{
+ struct usb_pic18bl *dev = usb_get_intfdata(intf);
+
+ mutex_lock(&dev->io_mutex);
+ pic18bl_draw_down(dev);
+
+ return 0;
+}
+
+static int pic18bl_post_reset(struct usb_interface *intf)
+{
+ struct usb_pic18bl *dev = usb_get_intfdata(intf);
+
+ /* we are sure no URBs are active - no locking needed */
+ dev->errors = -EPIPE;
+ mutex_unlock(&dev->io_mutex);
+
+ return 0;
+}
+
+static struct usb_driver pic18bl_driver = {
+ .name = "pic18bl",
+ .probe = pic18bl_probe,
+ .disconnect = pic18bl_disconnect,
+ .suspend = pic18bl_suspend,
+ .resume = pic18bl_resume,
+ .pre_reset = pic18bl_pre_reset,
+ .post_reset = pic18bl_post_reset,
+ .id_table = pic18bl_table,
+ .supports_autosuspend = 0,
+};
+
+static int __init usb_pic18bl_init(void)
+{
+ int result;
+
+ /* register this driver with the USB subsystem */
+ result = usb_register(&pic18bl_driver);
+ if (result)
+ err("usb_register failed. Error number %d", result);
+
+ return result;
+}
+
+static void __exit usb_pic18bl_exit(void)
+{
+ /* deregister this driver with the USB subsystem */
+ usb_deregister(&pic18bl_driver);
+}
+
+module_init(usb_pic18bl_init);
+module_exit(usb_pic18bl_exit);
+
+MODULE_LICENSE("GPL");
Index: linux-2.6.24-rc6/MAINTAINERS
===================================================================
--- linux-2.6.24-rc6.orig/MAINTAINERS 2007-12-29 11:04:05.000000000 -0800
+++ linux-2.6.24-rc6/MAINTAINERS 2007-12-29 11:14:19.000000000 -0800
@@ -3906,6 +3906,13 @@
W: http://pegasus2.sourceforge.net/
S: Maintained
+USB PIC18 BOOT LOADER DRIVER
+P: Mark Gross
+M: [email protected]
+L: [email protected]
+W: http://www.thegnar.org/bitwhacker/bitwhacker.html
+S: Maintained
+
USB PRINTER DRIVER (usblp)
P: Pete Zaitcev
M: [email protected]
Index: linux-2.6.24-rc6/drivers/usb/misc/Kconfig
===================================================================
--- linux-2.6.24-rc6.orig/drivers/usb/misc/Kconfig 2007-12-29 11:04:05.000000000 -0800
+++ linux-2.6.24-rc6/drivers/usb/misc/Kconfig 2007-12-29 11:19:54.000000000 -0800
@@ -258,6 +258,16 @@
To compile this driver as a module, choose M here: the
module will be called iowarrior.
+config USB_PIC18_BOOTLOADER
+ tristate "PIC 18 boot load driver support"
+ depends on USB
+ help
+ Say Y here if you want to support the programming PIC18 usb
+ devices that have the Microchip boot loader firmware on them.
+
+ To compile this driver as a module, choose M here: the
+ module will be called pic18bl.
+
config USB_TEST
tristate "USB testing driver (DEVELOPMENT)"
depends on USB && USB_DEVICEFS && EXPERIMENTAL
Index: linux-2.6.24-rc6/drivers/usb/misc/Makefile
===================================================================
--- linux-2.6.24-rc6.orig/drivers/usb/misc/Makefile 2007-12-29 11:04:05.000000000 -0800
+++ linux-2.6.24-rc6/drivers/usb/misc/Makefile 2007-12-29 11:18:46.000000000 -0800
@@ -22,6 +22,7 @@
obj-$(CONFIG_USB_PHIDGETKIT) += phidgetkit.o
obj-$(CONFIG_USB_PHIDGETMOTORCONTROL) += phidgetmotorcontrol.o
obj-$(CONFIG_USB_PHIDGETSERVO) += phidgetservo.o
+obj-$(CONFIG_USB_PIC18_BOOTLOADER) += pic18bl.o
obj-$(CONFIG_USB_RIO500) += rio500.o
obj-$(CONFIG_USB_TEST) += usbtest.o
obj-$(CONFIG_USB_TRANCEVIBRATOR) += trancevibrator.o
On Sat, 29 Dec 2007, mgross wrote:
> I'm playing around with a PIC based project at home (not an Intel
> activity) and found I needed a usb driver to talk to the boot loader
> so I can program my USB Bitwhacker with new custom firmware. The
> following adds the pic18bl driver to the kernel. Its pretty simple
> and is somewhat based on bits of a libusb driver that does some of
> what this driver does.
>
> What do you think?
Not to detract from your driver, but would it be possible to do the
whole thing in userspace using libusb? Maybe by extending the driver
you mentioned?
Alan Stern
On Dec 30, 2007 6:15 AM, Alan Stern <[email protected]> wrote:
> On Sat, 29 Dec 2007, mgross wrote:
>
> > I'm playing around with a PIC based project at home (not an Intel
> > activity) and found I needed a usb driver to talk to the boot loader
> > so I can program my USB Bitwhacker with new custom firmware. The
> > following adds the pic18bl driver to the kernel. Its pretty simple
> > and is somewhat based on bits of a libusb driver that does some of
> > what this driver does.
> >
> > What do you think?
>
> Not to detract from your driver, but would it be possible to do the
> whole thing in userspace using libusb? Maybe by extending the driver
> you mentioned?
>
The existing libusb based application works fine for PICDEM FS USB
or those based on it (like the Bitwhacker the OP is using).
Please do not add it to the kernel. There are libusb based application
for both the bootloader and the demo application and both are working
fine under Linux (along with Windows and I am trying to get FreeBSD
working).
Last time the demo application has been added to the ldusb and
I think it is not a good idea. But since then I've added patches to
the existing libusb application.
Relevant discussion in thread
'[PATCH 70/78] USB: add picdem device to ldusb'
http://marc.info/?t=117770076400003&r=1&w=2
So please do not do this again. It is not a problem for the libusb
based applications after the patches but it is really not necessary.
Original libusb based application for the bootloader:
http://www.internetking.org/fsusb/
Original libusb based application for the Demo which
also includes my patch for libusb-win32.
http://www.varxec.net/picdem_fs_usb/
Updated Patches to detach the kernel driver for both
the bootloader and Demo application.
http://forum.microchip.com/tm.aspx?m=106426
Xiaofan Chen
http://mcuee.blogspot.com
On Sat, Dec 29, 2007 at 05:15:30PM -0500, Alan Stern wrote:
> On Sat, 29 Dec 2007, mgross wrote:
>
> > I'm playing around with a PIC based project at home (not an Intel
> > activity) and found I needed a usb driver to talk to the boot loader
> > so I can program my USB Bitwhacker with new custom firmware. The
> > following adds the pic18bl driver to the kernel. Its pretty simple
> > and is somewhat based on bits of a libusb driver that does some of
> > what this driver does.
> >
> > What do you think?
>
> Not to detract from your driver, but would it be possible to do the
> whole thing in userspace using libusb? Maybe by extending the driver
> you mentioned?
>
Yeah, it has been done from user space using a libusb based
application. (that didn't work with a usb-hub in the loop) and had
code that was just too nasty for words, so I made a kernel driver that
looks nicer to me and enables a nice python FW loader program to work.
What is the linux-usb policies on new drivers that could be
implemented in user space? When does a kernel driver make sense over
a libusb one?
--mgross
On Sun, Dec 30, 2007 at 10:40:45AM +0800, Xiaofan Chen wrote:
> On Dec 30, 2007 6:15 AM, Alan Stern <[email protected]> wrote:
> > On Sat, 29 Dec 2007, mgross wrote:
> >
> > > I'm playing around with a PIC based project at home (not an Intel
> > > activity) and found I needed a usb driver to talk to the boot loader
> > > so I can program my USB Bitwhacker with new custom firmware. The
> > > following adds the pic18bl driver to the kernel. Its pretty simple
> > > and is somewhat based on bits of a libusb driver that does some of
> > > what this driver does.
> > >
> > > What do you think?
> >
> > Not to detract from your driver, but would it be possible to do the
> > whole thing in userspace using libusb? Maybe by extending the driver
> > you mentioned?
> >
>
> The existing libusb based application works fine for PICDEM FS USB
> or those based on it (like the Bitwhacker the OP is using).
The device ID's are different 0x000C in ldusb.c vrs 0x000b in the
driver I just posted.
Have you read my patch yet?
>
> Please do not add it to the kernel. There are libusb based application
> for both the bootloader and the demo application and both are working
> fine under Linux (along with Windows and I am trying to get FreeBSD
> working).
The libusb based FW loader http://www.internetking.org/fsusb/ program
is nasty and didn't work on one of my systems, so I refactored it into
a kernel driver and python program.
>
> Last time the demo application has been added to the ldusb and
> I think it is not a good idea. But since then I've added patches to
> the existing libusb application.
>
> Relevant discussion in thread
> '[PATCH 70/78] USB: add picdem device to ldusb'
> http://marc.info/?t=117770076400003&r=1&w=2
>
> So please do not do this again. It is not a problem for the libusb
> based applications after the patches but it is really not necessary.
Why not?
There are a lot of redundant things in the world. Linux is not
necessary if you really want to take this argument to its extreme.
>
> Original libusb based application for the bootloader:
> http://www.internetking.org/fsusb/
Yup thats the code. I found it way complex to read and felt a simple
kernel driver and simple python program much nicer to my
sensibilities.
We are getting quickly getting into a fuzzy/ opinion, area on this
thread. Is there a technical angle we can discuss? My LOC count of
the kernel driver and boot loader is smaller than the fsusb thing.
Also, with a kernel driver and a python lib, a GUI based boot loader
utility can be had with little effort.
> Original libusb based application for the Demo which
> also includes my patch for libusb-win32.
> http://www.varxec.net/picdem_fs_usb/
>
> Updated Patches to detach the kernel driver for both
> the bootloader and Demo application.
> http://forum.microchip.com/tm.aspx?m=106426
>
> Xiaofan Chen
> http://mcuee.blogspot.com
You blogging about me already?
I wont comment on that.
--mgross
On Dec 30, 2007 12:29 PM, mgross <[email protected]> wrote:
> The device ID's are different 0x000C in ldusb.c vrs 0x000b in the
> driver I just posted.
I know that. 000b is for the demo application. 000c is for the
bootloader application.
I am not a progammer myself but I think the USB communication part
of both the two libusb based application are fine. I have no comments
about the other part of fsusb coce.
http://forum.microchip.com/tm.aspx?m=106426
> > Please do not add it to the kernel. There are libusb based application
> > for both the bootloader and the demo application and both are working
> > fine under Linux (along with Windows and I am trying to get FreeBSD
> > working).
>
> The libusb based FW loader http://www.internetking.org/fsusb/ program
> is nasty and didn't work on one of my systems, so I refactored it into
> a kernel driver and python program.
>
If it does not work, read my patches and see if it will work.
If you do not like the existing fsusb application, you can rewrite
it in python with pyusb (which is based on libusb) but you do not
need a kernel driver.
pyusb: http://pyusb.berlios.de/
Hex file parsing in pyk by Mark Rages. He is using the Bitpim
libusb wrapper which IMHO is not as good as pyusb.
http://groups.google.com/group/pickit-devel/msg/35e850832256e890
Xiaofan
On Dec 30, 2007 11:53 AM, mgross <[email protected]> wrote:
> Yeah, it has been done from user space using a libusb based
> application. (that didn't work with a usb-hub in the loop) and had
> code that was just too nasty for words, so I made a kernel driver that
> looks nicer to me and enables a nice python FW loader program to work.
http://forum.microchip.com/tm.aspx?m=275422&mpage=2
Just a guess why it does not work: this might be due to the auto-suspend.
If you update your kernel, it should be ok. The firmware is also to blame.
> What is the linux-usb policies on new drivers that could be
> implemented in user space? When does a kernel driver make sense over
> a libusb one?
>
That would be interesting to know.
Xiaofan
On Dec 30, 2007 12:46 PM, Xiaofan Chen <[email protected]> wrote:
> If you do not like the existing fsusb application, you can rewrite
> it in python with pyusb (which is based on libusb) but you do not
> need a kernel driver.
>
> pyusb: http://pyusb.berlios.de/
>
> Hex file parsing in pyk by Mark Rages. He is using the Bitpim
> libusb wrapper which IMHO is not as good as pyusb.
> http://groups.google.com/group/pickit-devel/msg/35e850832256e890
The pyk code is for PICkit 2 and include the PICKit 2 bootloader.
it is an HID device so it is different from PICDEM FS-USB but you
might be able to reuse part of the codes.
By the way, there is another perl based code for the bootloader.
Just in case you are interested. Again I do not know perl and I
am not so sure if it is good enough for you.
http://www.nabble.com/HID-keyboard-code-for-gpasm-and-the-fs-picdem-board-td860144.html
http://www.nabble.com/attachment/878980/0/picdem.pl.bz2
Thinking about it, if the Linux USB maintainers agree to include your
driver, I have no objections. It should not affect the existing libusb
based applications and the users have one more choices.
Xiaofan
Xiaofan Chen wrote:
> On Dec 30, 2007 11:53 AM, mgross <[email protected]> wrote:
>> [...]
>> What is the linux-usb policies on new drivers that could be
>> implemented in user space? When does a kernel driver make sense over
>> a libusb one?
>
> That would be interesting to know.
I myself have been faced with this question before, and I think we
should try to clarify this by adding a document with some guidelines to
Documentation/usb.
So, to get the ball rolling, here are some factors that IMHO help decide
in which side to implement a driver:
- if the driver ties a hardware device to an existing in-kernel
interface (network, block, serial, bluetooth, video4linux, etc.), it
should probably be implemented in-kernel.
- on the other hand, if the driver doesn't use an existing kernel
interface and creates a new user-visible interface that is going to be
used by a single userspace application, it should probably be done in
userspace.
- if it is going to be used by several applications it could still be
implemented as a library, but it starts moving into the gray area.
- performance might be a reason to move to kernel space, but I don't
think it matters for transfer rates below 10Mbytes/sec or so.
Anyway, this is just MHO, so feel free to discuss this further. I'm
simply volunteering to sum up this thread into a patch to add a
Documentation/usb/userspace_drivers.txt (or something like that), so
that we can help future developers decide where to write their drivers.
--
Paulo Marques - http://www.grupopie.com
"Very funny Scotty. Now beam up my clothes."
On Wed, Jan 02, 2008 at 07:59:15PM +0000, Paulo Marques wrote:
> Xiaofan Chen wrote:
>> On Dec 30, 2007 11:53 AM, mgross <[email protected]> wrote:
>>> [...]
>>> What is the linux-usb policies on new drivers that could be
>>> implemented in user space? When does a kernel driver make sense over
>>> a libusb one?
>> That would be interesting to know.
>
> I myself have been faced with this question before, and I think we should
> try to clarify this by adding a document with some guidelines to
> Documentation/usb.
>
> So, to get the ball rolling, here are some factors that IMHO help decide in
> which side to implement a driver:
>
> - if the driver ties a hardware device to an existing in-kernel interface
> (network, block, serial, bluetooth, video4linux, etc.), it should probably
> be implemented in-kernel.
Agreed, I think this is clear.
>
> - on the other hand, if the driver doesn't use an existing kernel
> interface and creates a new user-visible interface that is going to be used
> by a single userspace application, it should probably be done in userspace.
>
To me this is still grey, and comes down to opinions of style. I
happen to like the way code looks when things are split up into
drivers (that know a lot about the hardware and protects it from data
that will turn it into a brick) and application code that talks to the
interface defined by the driver.
The libusb based applications I've seen tend to be quite convoluted
and do a poor job of separating the USB protocol from the application
protocol for talking to the device.
I don't think there is a clear way to define when to do a kernel
driver vrs just use a libusb thing, other than if no one does a kernel
driver for a device then users are stuck with the libusb applications.
If someone steps up and does one and is willing to support it, then to
me its like, "whatever" add the driver.
BTW I don't know if its worth to code bloat for my driver as I ponder
this issue now. I like the way non-libusb applications look more but
I guess I could create a lib of api-wrappers to the libusb interface
and use that, but I really think its less code to create a simple
kernel driver.
If it where up to me, I would say if the LOC is smaller to have a
kernel driver and application then a kernel driver is justified
otherwise its not.
--mgross
> - if it is going to be used by several applications it could still be
> implemented as a library, but it starts moving into the gray area.
>
> - performance might be a reason to move to kernel space, but I don't think
> it matters for transfer rates below 10Mbytes/sec or so.
>
> Anyway, this is just MHO, so feel free to discuss this further. I'm simply
> volunteering to sum up this thread into a patch to add a
> Documentation/usb/userspace_drivers.txt (or something like that), so that
> we can help future developers decide where to write their drivers.
>
> --
> Paulo Marques - http://www.grupopie.com
>
> "Very funny Scotty. Now beam up my clothes."
On Thu, Jan 03, 2008 at 03:08:55PM -0800, mgross wrote:
> On Wed, Jan 02, 2008 at 07:59:15PM +0000, Paulo Marques wrote:
> > Xiaofan Chen wrote:
> >> On Dec 30, 2007 11:53 AM, mgross <[email protected]> wrote:
> >>> [...]
> >>> What is the linux-usb policies on new drivers that could be
> >>> implemented in user space? When does a kernel driver make sense over
> >>> a libusb one?
Good question.
> >> That would be interesting to know.
> >
> > I myself have been faced with this question before, and I think we should
> > try to clarify this by adding a document with some guidelines to
> > Documentation/usb.
> >
> > So, to get the ball rolling, here are some factors that IMHO help decide in
> > which side to implement a driver:
> >
> > - if the driver ties a hardware device to an existing in-kernel interface
> > (network, block, serial, bluetooth, video4linux, etc.), it should probably
> > be implemented in-kernel.
>
> Agreed, I think this is clear.
Yes, this the primary decision point, everything after this depends on
lots of variables :)
> > - on the other hand, if the driver doesn't use an existing kernel
> > interface and creates a new user-visible interface that is going to be used
> > by a single userspace application, it should probably be done in userspace.
> >
>
> To me this is still grey, and comes down to opinions of style. I
> happen to like the way code looks when things are split up into
> drivers (that know a lot about the hardware and protects it from data
> that will turn it into a brick) and application code that talks to the
> interface defined by the driver.
>
> The libusb based applications I've seen tend to be quite convoluted
> and do a poor job of separating the USB protocol from the application
> protocol for talking to the device.
>
> I don't think there is a clear way to define when to do a kernel
> driver vrs just use a libusb thing, other than if no one does a kernel
> driver for a device then users are stuck with the libusb applications.
>
> If someone steps up and does one and is willing to support it, then to
> me its like, "whatever" add the driver.
Agreed. It all depends on the situation, we have kernel drivers for
devices that can be done in userspace, but not as cleanly or nicely, and
so, they stay as kernel drivers.
In the end, it comes down to individual cases, so let's handle them at
that level, it's easier that way.
thanks,
greg k-h
> > > So, to get the ball rolling, here are some factors that IMHO
> > > help decide in which side to implement a driver:
> > >
> > > - if the driver ties a hardware device to an existing
> > > in-kernel interface (network, block, serial, bluetooth,
> > > video4linux, etc.), it should probably be implemented
> > > in-kernel.
> >
> > Agreed, I think this is clear.
>
> Yes, this the primary decision point, everything after this depends on
> lots of variables :)
Including a pragmatic concern: performance requirements.
Today's usbfs-based userspace drivers don't get any zerocopy
benefits. So if you're passing around enough data that your
target environment needs a zerocopy I/O model (maybe it's got
to run on an embedded system with a small battery and not much
spare CPU power), that can argue in favor of a kernel driver.
I don't know whether the "usbfs2" work addresses that issue.
- Dave
> Agreed. It all depends on the situation, we have kernel drivers for
> devices that can be done in userspace, but not as cleanly or nicely, and
> so, they stay as kernel drivers.
>
> In the end, it comes down to individual cases, so let's handle them at
> that level, it's easier that way.