* Richard Retanubun | 2010-11-10 15:47:49 [-0500]:
>This patch adds support for STE-ISP1763 USB host and device controller
>The host-side is based largely on isp1760-hcd and STE pehci driver
>The device-side and platform parsing code is written mostly from scratch.
>
>Co-authored-by: Florian Vögel <fvoegel@xxxxxxxxxxxx>
>---
>
>v2: Added missing include/linux/usb/isp1763.h
> thanks to David for pointing it out :)
>
checkpatch says:
total: 132 errors, 128 warnings, 5891 lines checked
please fix this
Please make all EXPORT_SYMBOL_GPL
Please try not to include asm/ headers. It is usually evil. You should
get all what you need from linux/
You copy some of my code from the isp1760. It looks that parts of it are
not altered and fit for both drivers. You think thatit would make sense
to share this code between both drivers instead duplicating code and fix
bus just in of them two?
>
>diff --git a/drivers/usb/isp1763/Kconfig b/drivers/usb/isp1763/Kconfig
>new file mode 100644
>index 0000000..ca1bec2
>--- /dev/null
>+++ b/drivers/usb/isp1763/Kconfig
>@@ -0,0 +1,59 @@
>+#
>+#
>+
>+comment "Enable Host or Gadget support to see ISP1763 options"
>+ depends on !USB && USB_GADGET=n
>+
>+menuconfig USB_ISP1763
>+ tristate "ISP 1763A highspeed dual role controller support"
>+ depends on (USB || USB_GADGET)
>+ help
>+ Support for ST Ericsson ISP1763A USB OTG controller
>+
>+
>+if USB_ISP1763
>+
>+config USB_ISP1763_UDC
>+ tristate
>+
>+config USB_ISP1763_HCD
>+ tristate
>+
>+choice
>+ prompt "ISP1763 controller drivers"
>+ depends on USB_ISP1763
>+ help
>+ Blah
Yeah, blah. Blah.
>diff --git a/drivers/usb/isp1763/isp1763_base.c b/drivers/usb/isp1763/isp1763_base.c
>new file mode 100644
>index 0000000..ac8ed71
>--- /dev/null
>+++ b/drivers/usb/isp1763/isp1763_base.c
>@@ -0,0 +1,976 @@
>+/*
>+ * 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 of the License.
>+ *
>+ * This program is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>+ * See the GNU General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with this program; if not, write to the Free Software
>+ * Foundation, Inc., 51 Franklin Street, Fifth Floor,
>+ * Boston, MA 02110-1301, USA.
>+ *
>+ * Description:
>+ *
>+ * ISP1763 BASE driver - common initialization and OTG support
>+ *
>+ * Originally being written for a simple bus powered device, this
>+ * driver does not support or implement HRP or SRP.
>+ *
>+ * (c) 2010 F.A. Voegel, Carangul.Tech
>+ * (c) 2010 I+ME ACTIA GmbH
>+ */
>+#include <linux/device.h>
>+#include <linux/module.h>
>+#include <linux/kernel.h>
>+#include <linux/ioport.h>
>+#include <linux/types.h>
>+#include <linux/errno.h>
>+#include <linux/delay.h>
>+#include <linux/slab.h>
>+#include <linux/init.h>
>+#include <linux/timer.h>
>+#include <linux/list.h>
>+#include <linux/interrupt.h>
>+#include <linux/mm.h>
>+#include <linux/dma-mapping.h>
>+#include <linux/irq.h>
>+#include <linux/clk.h>
>+#include <linux/err.h>
>+#include <linux/seq_file.h>
>+#include <linux/debugfs.h>
>+#include <linux/io.h>
>+#include <linux/workqueue.h>
>+
>+#include <linux/usb.h>
>+#include <linux/usb/ch9.h>
>+#include <linux/usb/gadget.h>
>+#include <linux/usb/otg.h>
>+#include <linux/sched.h>
>+#include <linux/kthread.h>
>+
>+#ifdef CONFIG_PPC_OF
>+#include <linux/of.h>
>+#include <linux/of_platform.h>
>+#include <asm/prom.h>
>+#endif
>+
>+#ifdef CONFIG_PCI
>+#include <linux/pci.h>
>+#endif
>+
>+#include <linux/usb/isp1763.h>
>+#include <linux/platform_device.h>
>+
>+
>+#include <asm/byteorder.h>
>+#include <asm/dma.h>
>+#include <asm/gpio.h>
>+#include <asm/system.h>
>+
>+#include "isp1763.h"
>+#include "isp1763_udc.h"
>+#include "isp1763_hcd.h"
>+
>+static struct isp1763_dev *ispdev = NULL;
why this global pointer?
>+
>+
>+#define DRIVER_NAME "isp1763"
>+static const char driver_name[] = DRIVER_NAME;
>+static const char driver_desc[] = "ISP 1763A device controller driver";
>+static struct task_struct *otgtask = NULL;
>+static char *role[] = { "HOST", "PERIPHERAL" };
>+
>+/**
>+ * otg_set_role - set/switch role between host and device
>+ * @dev: isp1763_dev structure
>+ * @role: role to switch to
>+*/
>+static void otg_set_role(struct isp1763_dev *dev, int role)
>+{
>+ unsigned long flags;
why are you disabling interrupts here? I hope this is not locking.
>+ local_irq_save(flags);
>+ if (role == ROLE_HOST) {
>+ printk(KERN_ERR "%s() switching to HOST role\n", __FUNCTION__);
>+ isp1763_writel(0xFFFF << 16, &dev->regs->otg_ctrl);
>+ isp1763_writel(MASK_OTGCTRL_VBUS_DRV |
>+ MASK_OTGCTRL_VBUS_CHRG |
>+ MASK_OTGCTRL_DM_PULLDOWN |
>+ MASK_OTGCTRL_DP_PULLDOWN,
>+ &dev->regs->otg_ctrl);
>+ }
>+
>+ else {
>+ printk(KERN_ERR "%s() switching to PERIPHERAL role\n",
>+ __FUNCTION__);
>+ isp1763_writel(0xFFFF << 16, &dev->regs->otg_ctrl);
>+ isp1763_writel(MASK_OTGCTRL_SW_SEL_HC_DC |
>+ MASK_OTGCTRL_DP_PULLUP,
>+ &dev->regs->otg_ctrl);
>+ }
>+ dev->current_role = role;
>+ local_irq_restore(flags);
>+}
>+
>+static int isp1763_init_device(struct isp1763_dev *dev);
>+
>+/**
>+* otg_role_switcher - role switch handling task
>+* @data: pointer to struct isp1763_dev
>+*/
>+int otg_role_switcher(void *data)
>+{
>+ struct isp1763_dev *dev = data;
>+ int prev_role = dev->current_role;
>+ int id_pin;
>+
>+ set_current_state(TASK_RUNNING);
>+ printk(KERN_ERR "%s starting up...\n", __FUNCTION__);
>+
>+ do {
>+
>+ if (kthread_should_stop()) {
>+ set_current_state(TASK_RUNNING);
>+ break;
>+ }
>+
>+ schedule_timeout_interruptible(1);
>+
>+ if((id_pin = get_id_pin(dev)) == dev->current_role)
+ continue;
Is this required or is it triggered by the interrupt anyway?
>+
>+ printk(KERN_ERR "%s: init role change %s => %s \n", __FUNCTION__,
I guess dev_info() /debug fits this kind of output better unless you want
to hide behind some debug switch.
>+ role[dev->current_role], role[id_pin]);
>+
>+
>+ if (dev->ctrl[dev->current_role]) {
>+ if (dev->ctrl[dev->current_role]->suspend)
>+ printk(KERN_ERR "=> SUSPEND %s\n", role[dev->current_role]);
>+ dev->ctrl[dev->current_role]->suspend(dev->ctrl
>+ [dev->current_role]);
>+ }
>+
>+ isp1763_init_device(dev);
>+ otg_set_role(dev, id_pin);
>+
>+ if (dev->ctrl[dev->current_role]) {
>+ if (dev->ctrl[dev->current_role]->resume) {
>+ printk(KERN_ERR "=> RESUME %s\n", role[dev->current_role]);
>+ dev->ctrl[dev->current_role]->resume(dev->ctrl
>+ [dev->current_role]);
>+ dev->ctrl[dev->current_role]->do_irq(dev->ctrl[dev->current_role], MASK_DCINT_VBUS);
>+ }
>+ }
>+ prev_role = dev->current_role;
>+
>+ } while (1);
>+ return 0;
>+}
>+
>+/**
>+ * otg_do_interrupt - Handle OTG interrupt
>+ * @dev: pointer to isp1763_dev struct
>+*/
>+static void otg_do_interrupt(struct isp1763_dev *dev, u32 latch)
>+{
>+
>+ if ((latch & MASK_OTGSTATUS_ID)
>+ && !static_role(dev->devflags)) {
>+ wake_up_process(otgtask);
>+ } else if (latch & MASK_OTGSTATUS_ID) {
>+ printk(KERN_EMERG "Ignoring OTG event\n");
>+ }
>+ if (latch & MASK_OTGINTEN_TMR_TIMEOUT) {
>+ if (dev->otgtimer_cb) {
>+ dev->otgtimer_cb(dev->otgtimer_data);
>+ }
>+ }
>+}
>+
>+void *mbar = NULL;
You don't want to keep global variables of this kind and I don't really see any users of this.
>+
>+
>+/**
>+ * isp1763 - main IRQ handler
>+ * @data: pointer to struct isp1763_dev
>+ */
>+static irqreturn_t isp1763_irq(int irq, void *data)
>+{
>+ struct isp1763_dev *dev = (struct isp1763_dev *) data;
>+ u32 flags_hc;
>+ u32 flags_dc;
>+ u32 flags_otg;
>+
>+ isp1763_writew(UNLOCK_CODE, &dev->regs->unlock);
>+
>+ flags_dc = isp1763_readl(&dev->regs->dc_interrupt);
>+ isp1763_writel(flags_dc, &dev->regs->dc_interrupt);
>+ flags_hc = isp1763_readw(&dev->regs->hc_interrupt);
>+ isp1763_writew(flags_hc, &dev->regs->hc_interrupt);
>+ flags_otg = isp1763_readl(&dev->regs->otg_int_latch);
>+ isp1763_writel(0xFFFF0000, &dev->regs->otg_int_latch);
>+
>+ //printk(KERN_DEBUG "%.8lx %.4x %.8lx\n", flags_dc, flags_hc, flags_otg);
>+
>+ if(flags_otg)
>+ otg_do_interrupt(dev, flags_otg);
would it easy things if you would use here threaded interrupts instead of
the task struct which you fire up here?
>+ if (dev->current_role == ROLE_HOST) {
>+ if(flags_hc && dev->ctrl[ROLE_HOST]
>+ /*&& dev->ctrl[ROLE_HOST]->do_irq*/)
>+ dev->ctrl[ROLE_HOST]->do_irq(dev->ctrl[ROLE_HOST], flags_hc);
>+ } else {
>+ if (flags_dc && dev->ctrl[ROLE_DEVICE]
>+ /*&& dev->ctrl[ROLE_DEVICE]->do_irq*/)
>+ dev->ctrl[ROLE_DEVICE]->do_irq(dev->ctrl[ROLE_DEVICE], flags_dc);
>+ }
>+
>+ return IRQ_HANDLED;
>+}
>+
>+static char *bustypes[] = {
>+ "NAND", "Generic", "NOR", "SRAM"
>+};
>+
>+/**
>+ * isp1763_init_device - reset & initialize controller hardware
>+ * @dev: pointer to struct isp1763_dev
>+*/
>+static int isp1763_init_device(struct isp1763_dev *dev)
>+{
>+ int i;
>+ int ret = 0;
>+ volatile u16 hwmode =
>+ MASK_HWMODECTRL_COMN_INT | MASK_HWMODECTRL_GLOBAL_INT_ENABLE;
>+
>+ mdelay(10);
>+ /* Dummy reads to "stabilize host controller access" */
>+ isp1763_readw(&dev->regs->chip_id);
>+ isp1763_readw(&dev->regs->chip_id);
>+ isp1763_readw(&dev->regs->chip_id);
>+ mdelay(20);
>+
>+ if (dev->devflags & ISP1763_FLAG_BUS_WIDTH_8) {
>+ hwmode |= MASK_HWMODECTRL_DATA_BUS_WIDTH;
>+ printk(KERN_NOTICE "hwmode 8bit\n");
>+ }
>+ if (dev->devflags & ISP1763_FLAG_DACK_POL_HIGH) {
>+ hwmode |= MASK_HWMODECTRL_DACK_POL;
>+ printk(KERN_NOTICE "hwmode dack polarity high\n");
>+ }
>+ if (dev->devflags & ISP1763_FLAG_DREQ_POL_HIGH) {
>+ hwmode |= MASK_HWMODECTRL_DREQ_POL;
>+ printk(KERN_NOTICE "hwmode dreq polarity hight\n");
>+ }
>+ if (dev->devflags & ISP1763_FLAG_INTR_POL_HIGH) {
>+ hwmode |= MASK_HWMODECTRL_INTR_POLARITY;
>+ printk(KERN_NOTICE "hwmode int polarity high\n");
>+ }
>+ if (dev->devflags & ISP1763_FLAG_INTR_EDGE_TRIG) {
>+ hwmode |= MASK_HWMODECTRL_INTR_EDGE;
>+ printk(KERN_NOTICE "hwmode int edge triggered\n");
>+ }
>+ if (static_role(dev->devflags)) {
>+ hwmode |= MASK_HWMODECTRL_ID_PU_DISABLE;
>+ printk(KERN_NOTICE "hwmode pullup disable\n");
>+ }
>+ // DEBUG
>+ hwmode &= ~MASK_HWMODECTRL_INTR_POLARITY;
>+ hwmode &= ~MASK_HWMODECTRL_INTR_EDGE;
>+ isp1763_writew(hwmode, &dev->regs->hwmodectrl);
>+ isp1763_writew(hwmode, &dev->regs->hwmodectrl);
>+
>+ if (isp1763_readl(&dev->regs->chip_id) != ISP_CHIP_ID) {
>+ printk(KERN_ERR
>+ "Error: ISP1763 chip ID wrong! (%.8lx, expected %.8lx)\n",
>+ (unsigned long) isp1763_readl(&dev->regs->chip_id),
why are we using casts here?
>+ ISP_CHIP_ID);
>+ ret = -EIO;
>+ goto out;
>+ }
>+
>+ printk(KERN_NOTICE "%s: %s mode, %i bit bus width\n", DRIVER_NAME,
>+ bustypes[(isp1763_readw(&dev->regs->swreset) >> 6) & 0x3],
>+ isp1763_readw(&dev->regs->hwmodectrl) &
>+ MASK_HWMODECTRL_DATA_BUS_WIDTH ? 8 : 16);
>+
>+ isp1763_writew(UNLOCK_CODE, &dev->regs->unlock);
>+
>+ printk(KERN_NOTICE "Resetting ISP1763...\n");
>+ isp1763_writew(isp1763_readw(&dev->regs->swreset) | 1 | 8,
please use define for BITS 1 and 8 so I have an impression what is
happening here
>+ &dev->regs->swreset);
>+ isp1763_writew(isp1763_readw(&dev->regs->mode) | MASK_MODE_SFRESET,
>+ &dev->regs->mode);
please try to document why you need this delay
>+ udelay(50);
>+ isp1763_writew(isp1763_readw(&dev->regs->mode) &
>+ ~MASK_MODE_SFRESET, &dev->regs->mode);
>+ udelay(1000);
this is a mdelay(1)
>+
>+ isp1763_writew(UNLOCK_CODE, &dev->regs->unlock);
>+
>+ if (isp1763_readl(&dev->regs->chip_id) != ISP_CHIP_ID) {
>+ printk(KERN_ERR
>+ "Error: ISP1763 chip ID wrong after reset! (%.8lx, expected %.8lx)\n",
>+ (unsigned long) isp1763_readl(&dev->regs->chip_id),
>+ ISP_CHIP_ID);
>+ ret = -EIO;
>+ goto out;
>+ }
>+ printk(KERN_NOTICE "Reset done\n");
>+
>+ isp1763_writew(hwmode, &dev->regs->hwmodectrl);
>+ isp1763_writew(MASK_HCINT_OTG, &dev->regs->hc_interrupt_enable);
>+
>+ /*
>+ You'd think there was one single way to configure the interrupt for
>+ this chip, but no. You have to independently configure both
>+ peripheral and host controller IRQs despite the settings obviously
>+ having to be the same...
>+ */
>+ isp1763_writew(0xFC |
>+ ((ispdev->devflags & ISP1763_FLAG_INTR_EDGE_TRIG) >> 1)
>+ | (ispdev->devflags & ISP1763_FLAG_INTR_POL_HIGH),
>+ &dev->regs->icr);
>+
>+ /* data line test */
>+ for (i = 0; i < 0x10000; i++) {
>+ u16 readback = 0;
>+ isp1763_writew(i, &dev->regs->scratch);
>+ readback = isp1763_readw(&dev->regs->scratch);
>+ if (readback != i) {
>+ printk(KERN_ERR
>+ "ERROR: Scratch register write test failed: scratch=%.4x != i=%.4x!\n",
>+ readback, i);
>+ ret = -EIO;
>+ /*
>+ * Don't break here, let us see the other errors too,
>+ * helps with diagnosis
>+ */
>+ }
>+ }
>+
>+ isp1763_writew(1, &dev->regs->int_pulse_width);
>+
>+out:
>+ return ret;
>+}
>+
>+/**
>+* isp1763_register_ctrl - register a host or device controller driver
>+* @ctrl: controller structure to register
>+* @role: role for which to register driver
>+*
>+* Register a driver for a host or peripheral controller driver. After
>+* registering, the driver's probe function will be called, and if OTG is
>+* enabled and the current role matches the driver, it's resume function
>+* will be called as well.
>+*/
>+int isp1763_register_ctrl(struct isp1763_controller *ctrl, int role)
>+{
>+ if (!ispdev)
>+ return -ENODEV;
>+
>+ if (ispdev->ctrl[role]) {
>+ printk(KERN_ERR
>+ "Trying to register driver for role %s which already has driver registered\n",
>+ role == ROLE_HOST ? "host" : "device");
>+ return -EBUSY;
>+ }
>+ ispdev->ctrl[role] = ctrl;
>+
>+ ctrl->device = ispdev->dev;
>+ ctrl->regs = ispdev->regs;
>+
>+ if (!ctrl->probe(ctrl) < 0)
this should give you a warning. The ! ensures that you get 0 or 1 and
bothe of them are not less then 0
>+ return -ENODEV;
>+
>+ /* driver for the current role? resume right now. */
>+ if (role == ispdev->current_role) {
>+ printk(KERN_ERR "Registered %s as active driver\n",
>+ role == ROLE_HOST ? "host" : "device");
>+ ctrl->resume(ctrl);
>+ } else
>+ printk(KERN_ERR "Registered %s as suspended driver\n",
>+ role == ROLE_HOST ? "host" : "device");
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL(isp1763_register_ctrl);
>+
>+
>+/*******************************************/
>+/* module init / exit / probe related code */
>+/*******************************************/
_I_ don't think that this kind of comments are useful.
>+
>+
>+/**
>+* isp1763_common_init - common module init
>+* @dev: struct device pointer passed from probe function
>+* @devflags: hardware flags as determined by calling probe function
>+* @irq: IRQ number to use
>+* @res: ISP1763 register memory resource pointer
>+* @res_len: length of resources
>+*
>+* Called by all probe functions to initialize the hardware and setup data
>+* structures.
>+*/
>+static int isp1763_common_init(struct device *dev,
>+ unsigned long devflags,
>+ int irq,
>+ struct resource *res,
>+ resource_size_t res_len)
>+{
>+ int ret = 0;
no need for =0 unless you return it. If you do, you don't see jumps to
the error label without setting ret first.
>+
>+ ispdev = kmalloc(sizeof(struct isp1763_dev), GFP_ATOMIC);
GFP_KERNEL please. There is no need for ATOMIC
>+ if (!ispdev) {
>+ ret = -ENOMEM;
>+ goto error;
>+ }
>+ memset(ispdev, 0, sizeof(struct isp1763_dev));
instead of that, use kzalloc() please
>+
>+ printk(KERN_ERR "devflags passed by probe: 0x%.8lx\n", devflags);
>+
>+ ispdev->irq = irq;
>+ ispdev->devflags = devflags;
>+ ispdev->regs = ioremap(res->start, res->end - res->start);
>+
>+ if (request_irq(ispdev->irq, isp1763_irq, IRQF_SHARED, "ISP1763 IRQ",
>+ (void *) ispdev)) {
>+ ret = -EIO;
>+ goto error_free;
>+ }
>+
>+ isp1763_init_device(ispdev);
>+ otg_setup(ispdev);
>+
>+ /* Only start role switch thread if we're doing OTG */
>+ if (!static_role(ispdev->devflags)) {
>+ otgtask = kthread_create(otg_role_switcher, ispdev,
>+ "isp1763otg");
>+ if(otgtask)
>+ wake_up_process(otgtask);
>+ }
>+
>+ ispdev->dev = dev;
>+
>+ printk(KERN_INFO "%s initialization successfully\n", DRIVER_NAME);
>+
>+ return 0;
>+ error_free:
>+ kfree(ispdev);
>+ error:
>+ return ret;
>+}
>+
>+#ifdef CONFIG_PPC_OF
>+static int __devinit isp1763_of_probe(struct of_device *dev,
>+ const struct of_device_id *match)
>+{
>+ struct device_node *np = dev->node;
>+ const u32 *prop;
>+ int ret = 0;
>+ unsigned long devflags = 0;
>+ struct resource *res;
>+ struct resource memory;
>+ int irq;
>+ resource_size_t res_len;
>+ const char *port1_role = NULL;
>+
>+ prop = of_get_property(np, "bus-width", NULL);
>+ if (prop && *prop == 8)
no. this is not u32 it is be32. So you need be32_to_cpu on this.
>+ devflags |= ISP1763_FLAG_BUS_WIDTH_8;
>+
>+ port1_role = of_get_property(np, "port1-role", NULL);
>+ if (port1_role) {
>+ printk(KERN_ERR "PORT1 OTG setting: %s\n", port1_role);
>+ if (strcmp(port1_role, "otg") == 0)
>+ devflags |= ISP1763_FLAG_PORT1_ROLE_OTG;
>+ else if (strcmp(port1_role, "host") == 0)
>+ devflags |= ISP1763_FLAG_PORT1_ROLE_HOST;
>+ else
>+ devflags |= ISP1763_FLAG_PORT1_ROLE_GADGET;
>+ } else
>+ devflags |= ISP1763_FLAG_PORT1_ROLE_GADGET;
>+
>+ if (of_get_property(np, "dack-polarity-high", NULL) != NULL)
>+ devflags |= ISP1763_FLAG_DACK_POL_HIGH;
>+
>+ if (of_get_property(np, "dreq-polarity-high", NULL) != NULL)
>+ devflags |= ISP1763_FLAG_DREQ_POL_HIGH;
>+
>+ if (of_get_property(np, "intr-polarity-high", NULL) != NULL)
>+ devflags |= ISP1763_FLAG_INTR_POL_HIGH;
>+
>+ if (of_get_property(np, "intr-edge-trig", NULL) != NULL)
>+ devflags |= ISP1763_FLAG_INTR_EDGE_TRIG;
>+
>+ irq = irq_of_parse_and_map(np, 0);
>+
>+ if (irq == NO_IRQ) {
>+ pr_warning("of_isp1763: no interrupt!\n");
>+ ret = -EIO;
>+ goto error_out;
>+ }
>+
>+ ret = of_address_to_resource(np, 0, &memory);
>+ if (ret) {
>+ pr_warning("of_isp1763: Memory resource not available\n");
>+ goto error_out;
>+ }
>+
>+ res_len = resource_size(&memory);
>+
>+ res =
>+ request_mem_region(memory.start, res_len, dev_name(&dev->dev));
>+ if (!res) {
>+ pr_warning
>+ ("of_isp1763: Cannot reserve the memory resource\n");
>+ return -EBUSY;
>+ }
>+ /* FIXME: what's the idea behind this? */
>+ res_len = memory.end - memory.start + 1;
You tell me :) It is not used in isp1763_common_init
>+
>+ return isp1763_common_init(&dev->dev, devflags, irq, res, res_len);
>+
>+ error_out:
>+ return ret;
>+}
>+
>+static int __devexit isp1763_of_remove(struct of_device *ofdev)
>+{
>+ if (ispdev->irq)
>+ free_irq(ispdev->irq, (void *) ispdev);
The void cast it not required and it should be called unconditional.
Anyway, shouldn't you detach yourself from the usb stack, free mem
region and do this kind of things?
>+ kfree(ispdev);
>+ return 0;
>+}
>+
>+
>+static struct of_device_id __devinitdata isp1763_match[] = {
>+ {
>+ .compatible = "st,usb-isp1763",
Why the usb- prefix? It is usually vendor,device
>+ },
>+ {},
>+};
>+
>+MODULE_DEVICE_TABLE(of, isp1763_match);
>+
>+static struct of_platform_driver isp1763_driver = {
>+ .name = (char *) driver_name,
cast shouldn't be required
>+ .match_table = isp1763_match,
>+ .probe = isp1763_of_probe,
>+ .remove = __devexit_p(isp1763_of_remove),
>+};
>+
>+
>+#endif
>+
>+
>+/******************************************************************/
>+/* PCI PROBE */
>+/******************************************************************/
>+
>+#ifdef CONFIG_PCI
>+static int __devinit isp1763_pci_probe(struct pci_dev *dev,
>+ const struct pci_device_id *id)
>+{
>+ u8 latency, limit;
>+ __u32 reg_data;
>+ int retry_count;
>+ unsigned int devflags = 0;
>+ int ret_status = 0;
>+
>+ resource_size_t pci_mem_phy0;
>+ resource_size_t memlength;
>+
>+ u8 __iomem *chip_addr;
>+ u8 __iomem *iobase;
>+ resource_size_t nxp_pci_io_base;
>+ resource_size_t iolength;
>+
>+ if (usb_disabled())
>+ return -ENODEV;
>+
>+ if (pci_enable_device(dev) < 0)
>+ return -ENODEV;
>+
>+ if (!dev->irq)
you should call pci_disable_device() here
>+ return -ENODEV;
>+
>+ /* Grab the PLX PCI mem maped port start address we need */
>+ nxp_pci_io_base = pci_resource_start(dev, 0);
>+ iolength = pci_resource_len(dev, 0);
>+
>+ if (!request_mem_region
>+ (nxp_pci_io_base, iolength, "ISP1763 IO MEM")) {
>+ printk(KERN_ERR "request region #1\n");
>+ return -EBUSY;
>+ }
>+
>+ iobase = ioremap_nocache(nxp_pci_io_base, iolength);
ioremap() is enough.
>+ if (!iobase) {
>+ printk(KERN_ERR "ioremap #1\n");
>+ ret_status = -ENOMEM;
>+ goto cleanup1;
>+ }
>+ /* Grab the PLX PCI shared memory of the ISP 1763 we need */
>+ pci_mem_phy0 = pci_resource_start(dev, 3);
>+ memlength = pci_resource_len(dev, 3);
>+ if (memlength < 0xffff) {
>+ printk(KERN_ERR
>+ "memory length for this resource is wrong\n");
>+ ret_status = -ENOMEM;
>+ goto cleanup2;
>+ }
>+
>+ if (!request_mem_region(pci_mem_phy0, memlength, "ISP-PCI")) {
>+ printk(KERN_ERR "host controller already in use\n");
>+ ret_status = -EBUSY;
>+ goto cleanup2;
>+ }
>+
>+ /* map available memory */
>+ chip_addr = ioremap_nocache(pci_mem_phy0, memlength);
>+ if (!chip_addr) {
>+ printk(KERN_ERR "Error ioremap failed\n");
>+ ret_status = -ENOMEM;
>+ goto cleanup3;
>+ }
>+
>+ /* bad pci latencies can contribute to overruns */
>+ pci_read_config_byte(dev, PCI_LATENCY_TIMER, &latency);
>+ if (latency) {
>+ pci_read_config_byte(dev, PCI_MAX_LAT, &limit);
>+ if (limit && limit < latency)
>+ pci_write_config_byte(dev, PCI_LATENCY_TIMER,
>+ limit);
>+ }
>+
>+ /* Try to check whether we can access Scratch Register of
>+ * Host Controller or not. The initial PCI access is retried until
>+ * local init for the PCI bridge is completed
>+ */
>+ retry_count = 20;
>+ reg_data = 0;
>+ while ((reg_data != 0xFACE) && retry_count) {
>+ /*by default host is in 16bit mode, so
>+ * io operations at this stage must be 16 bit
>+ * */
>+ writel(0xface, chip_addr + HC_SCRATCH_REG);
>+ udelay(100);
>+ reg_data = readl(chip_addr + HC_SCRATCH_REG) & 0x0000ffff;
>+ retry_count--;
>+ }
>+
>+ iounmap(chip_addr);
>+
>+ /* Host Controller presence is detected by writing to scratch register
>+ * and reading back and checking the contents are same or not
>+ */
>+ if (reg_data != 0xFACE) {
>+ dev_err(&dev->dev, "scratch register mismatch %x\n",
>+ reg_data);
>+ ret_status = -ENOMEM;
>+ goto cleanup3;
>+ }
>+
>+ pci_set_master(dev);
>+
>+ /* configure PLX PCI chip to pass interrupts */
>+#define PLX_INT_CSR_REG 0x68
>+ reg_data = readl(iobase + PLX_INT_CSR_REG);
>+ reg_data |= 0x900;
>+ writel(reg_data, iobase + PLX_INT_CSR_REG);
>+
>+ dev->dev.dma_mask = NULL;
>+ /* hcd = isp1763_register(pci_mem_phy0, memlength, dev->irq,
>+ IRQF_SHARED | IRQF_DISABLED, &dev->dev, dev_name(&dev->dev),
>+ devflags); */
>+ if (isp1763_common_init
>+ (&dev->dev, devflags, dev->irq, (void *) pci_mem_phy0,
>+ memlength) < 0)
>+ goto cleanup3;
>+
>+ /* done with PLX IO access */
>+ iounmap(iobase);
>+ release_mem_region(nxp_pci_io_base, iolength);
>+
>+ return 0;
>+
>+ cleanup3:
>+ release_mem_region(pci_mem_phy0, memlength);
>+ cleanup2:
>+ iounmap(iobase);
>+ cleanup1:
>+ release_mem_region(nxp_pci_io_base, iolength);
>+ return ret_status;
>+}
>+
>+static void isp1763_pci_remove(struct pci_dev *dev)
>+{
>+ struct usb_hcd *hcd;
>+
>+ hcd = pci_get_drvdata(dev);
>+ pci_disable_device(dev);
>+}
>+
>+static void isp1763_pci_shutdown(struct pci_dev *dev)
>+{
>+ printk(KERN_ERR "ips1763_pci_shutdown\n");
shutdown is called during kexec for instance. It should ensure that the
device remains quite and cause no trouble during reboot and on the next
init.
>+}
>+
>+static const struct pci_device_id isp1763_plx[] = {
>+ {
>+ .class = PCI_CLASS_BRIDGE_OTHER << 8,
>+ .class_mask = ~0,
>+ .vendor = PCI_VENDOR_ID_PLX,
>+ .device = 0x5406,
>+ .subvendor = PCI_VENDOR_ID_PLX,
>+ .subdevice = 0x9054,
>+ },
>+ {}
>+};
>+
>+MODULE_DEVICE_TABLE(pci, isp1763_plx);
>+
>+static int __init isp1763_init(void)
>+{
>+ int ret = 0;
>+ int any_ret = -ENODEV;
>+
>+ mbar = ioremap(0x80000000, 0x10000);
interresting.
>+
>+ ret = platform_driver_register(&isp1763_plat_driver);
>+ if (!ret)
>+ any_ret = 0;
>+
>+#ifdef CONFIG_PPC_OF
>+ ret = of_register_platform_driver(&isp1763_driver);
>+ if(!ret)
>+ any_ret = 0;
>+#endif
>+
>+#ifdef CONFIG_PCI
>+ ret = pci_register_driver(&isp1763_pci_driver);
>+ if (!ret)
>+ any_ret = 0;
>+#endif
>+
>+ return any_ret;
>+}
>+
>+static void __exit isp1763_exit(void)
>+{
>+#ifdef CONFIG_PPC_OF
>+ of_unregister_platform_driver(&isp1763_driver);
>+#endif
>+
>+#ifdef CONFIG_PCI
>+ pci_unregister_driver(&isp1763_pci_driver);
>+#endif
>+
>+ platform_driver_unregister(&isp1763_plat_driver);
>+}
>+
>+module_init(isp1763_init);
>+module_exit(isp1763_exit);
>+
>+MODULE_DESCRIPTION(DRIVER_NAME);
>+MODULE_AUTHOR("F.A. Voegel, Carangul.Tech");
>+MODULE_LICENSE("GPL");
>diff --git a/drivers/usb/isp1763/isp1763_hcd.c b/drivers/usb/isp1763/isp1763_hcd.c
>new file mode 100644
>index 0000000..e4f9384
>--- /dev/null
>+++ b/drivers/usb/isp1763/isp1763_hcd.c
>+/**
>+ * init_memory - initialize HC memory management
>+ * @priv: HC private data
>+ *
>+ * memory management of the 20kb payload on the chip from 0x1000 to 0x5fff
>+ */
>+static void init_memory(struct isp1763_hcd *priv)
oh yeah, This is pretty awesome :) At the time I wrote this I knew no
alternatives and it was sufficient plus easy / simple code. In case
you need more granular allocations or you run out of a particular slot
you could try to look at gen_pool_create() and friends in lib/genalloc.c.
>+{
>+ int i;
>+ u32 payload;
>+
>+ payload = 0x1000;
>+ for (i = 0; i < BLOCK_1_NUM; i++) {
>+ priv->memory_pool[i].start = payload;
>+ priv->memory_pool[i].size = BLOCK_1_SIZE;
>+ priv->memory_pool[i].free = 1;
>+ payload += priv->memory_pool[i].size;
>+ }
>+
>+
>+ for (i = BLOCK_1_NUM; i < BLOCK_1_NUM + BLOCK_2_NUM; i++) {
>+ priv->memory_pool[i].start = payload;
>+ priv->memory_pool[i].size = BLOCK_2_SIZE;
>+ priv->memory_pool[i].free = 1;
>+ payload += priv->memory_pool[i].size;
>+ }
>+
>+
>+ for (i = BLOCK_1_NUM + BLOCK_2_NUM; i < BLOCKS; i++) {
>+ priv->memory_pool[i].start = payload;
>+ priv->memory_pool[i].size = BLOCK_3_SIZE;
>+ priv->memory_pool[i].free = 1;
>+ payload += priv->memory_pool[i].size;
>+ }
>+
>+ BUG_ON(payload - priv->memory_pool[i - 1].size > PAYLOAD_SIZE);
>+}
>+
>+/**
>+ * isp1763_hc_setup - setup host controller
>+ * @hcd: host controller structure
>+ *
>+ * Fill registers with initial values and reset EHCI controller, enable IRQs
>+ */
>+static int isp1763_hc_setup(struct usb_hcd *hcd)
>+{
>+ struct isp1763_hcd *priv = hcd_to_priv(hcd);
>+ int result;
>+
>+ isp1763_init_regs(hcd);
>+
>+ isp1763_writew(isp1763_readw(hcd->regs + HC_RESET_REG) |
>+ SW_RESET_RESET_HC,
>+ hcd->regs + HC_RESET_REG);
>+ result = ehci_reset(priv);
>+ if (result)
>+ return result;
>+
>+ isp1763_info(priv, "bus width: %d\n",
>+ (priv->devflags & ISP1763_FLAG_BUS_WIDTH_8) ? 8 : 16);
>+
>+ isp1763_dbg(priv, "hwmode: 0x%04X\n",
>+ isp1763_readw(hcd->regs + HC_HW_MODE_CTRL));
>+
>+ isp1763_writew(INTERRUPT_ENABLE_MASK,
>+ hcd->regs + HC_INTERRUPT_REG);
>+ isp1763_writew(INTERRUPT_ENABLE_MASK,
>+ hcd->regs + HC_INTERRUPT_ENABLE);
>+
>+ /* This chip does not have HCSPARAMS register, use a hardcoded value. */
>+ priv->hcs_params = HCS_HARDCODE;
I haven't seen a write to this variable except here. So if this is really
read only you could just get rid of the variable and use the define
instead.
>+
>+ return priv_init(hcd);
>+}
>+
>+static int isp1763_run(struct usb_hcd *hcd)
>+{
>+ struct isp1763_hcd *priv = hcd_to_priv(hcd);
>+ int retval;
>+ u16 temp;
>+ u32 command;
>+ u32 chipid;
>+
>+
>+ hcd->uses_new_polling = 1;
>+ hcd->poll_rh = 0;
FYI: This tells the USB stack that the it does not need to poll the root
hub and see if there are any port changes. I did not do this for the 1760
as it did not look required: The only root hub is discovered on power
up / probe and can not be disconnected during its life time. I guess
this is also the case for their new chip.
>
>+ hcd->state = HC_STATE_RUNNING;
>+
>+ isp1763_enable_interrupts(hcd);
>+ temp = isp1763_readw(hcd->regs + HC_HW_MODE_CTRL);
>+ isp1763_writew(temp | HW_GLOBAL_INTR_EN,
>+ hcd->regs + HC_HW_MODE_CTRL);
>+
>+ command = isp1763_readl(hcd->regs + HC_USBCMD);
>+ command &= ~(CMD_LRESET | CMD_RESET);
>+ command |= CMD_RUN;
>+ isp1763_writel(command, hcd->regs + HC_USBCMD);
>+
>+ retval = handshake(priv, hcd->regs + HC_USBCMD, CMD_RUN, CMD_RUN,
>+ 250 * 1000);
>+ if (retval)
>+ return retval;
>+
>+ down_write(&ehci_cf_port_reset_rwsem);
>+ isp1763_writel(FLAG_CF, hcd->regs + HC_CONFIGFLAG);
>+
>+ retval = handshake(priv, hcd->regs + HC_CONFIGFLAG, FLAG_CF, FLAG_CF,
>+ 250 * 1000);
>+ up_write(&ehci_cf_port_reset_rwsem);
>+ if (retval)
>+ return retval;
>+
>+ chipid = isp1763_readl(hcd->regs + HC_CHIP_ID_REG);
>+ isp1763_info(priv, "USB ISP %04x HW rev. %d started\n",
>+ ((chipid & 0x00ffff00) >> 8), (chipid & 0x000000ff));
>+
>+ /* PTD Register Init Part 2, Step 28 */
>+ /* enable INTs */
>+ isp1763_init_maps(hcd);
>+
>+ return 0;
>+}
>+
>+/* NOTE:
>+ * It seems that setting up two memory banks in the isp1760's code
>+ * is merely for convenience's sake; right now trying with:
>+ *
>+ * 1. Setup up the memory register for the PTD
>+ * 2. Copy PTD from device to the driver's PTD copy
>+ * 3. Process the PTD data
>+ * (later on)
>+ * 4. Setup the memory register for the payload
>+ * 5. Copy payload from device to the driver's payload space
>+ *
>+ * There may be a gotcha here if the isp1760 is taking advantage of the ISP_BANK
>+ * ability to automatically increment the address per bank on consecutive access
>+ * to the same bank; It does not seem so because atl_regs & payload are
>+ * reassigned at every while(done_map) iteration.
The two banks are used in order to obey chip's timming requirements and
not to use a delay function for this.
>+/*
>+ * GIT COMMIT: 1b9a38bfa6e664ff02511314f5586d711c83cc91 adapted
>+ * "USB: EHCI: fix handling of unusual interrupt intervals"
ah, shouldn't this be part of 1760 as well?
>+ */
>+static struct isp1763_qh *qh_make(struct isp1763_hcd *priv,
>+ struct urb *urb, gfp_t flags)
>+{
>+ struct isp1763_qh *qh;
>+ int is_input, type;
>+
>+ qh = isp1763_qh_alloc(priv, flags);
>+ if (!qh)
>+ return qh;
>+
>+ /*
>+ * init endpoint/device data for this QH
>+ */
>+ is_input = usb_pipein(urb->pipe);
>+ type = usb_pipetype(urb->pipe);
>+
>+ if (type == PIPE_INTERRUPT) {
>+
>+ if (urb->dev->speed == USB_SPEED_HIGH) {
>+
>+ qh->period = urb->interval >> 3;
>+ if (qh->period == 0 && urb->interval != 1) {
>+ /* NOTE interval 2 or 4 uframes could work.
>+ * But interval 1 scheduling is simpler, and
>+ * includes high bandwidth.
>+ */
>+ urb->interval = 1;
>+ } else if (qh->period > priv->periodic_size) {
>+ qh->period = priv->periodic_size;
>+ urb->interval = qh->period << 3;
>+ }
>+ } else {
>+ qh->period = urb->interval;
>+ if (qh->period > priv->periodic_size) {
>+ qh->period = priv->periodic_size;
>+ urb->interval = qh->period;
>+ }
>+ }
>+ }
>+
>+ /* support for tt scheduling, and access to toggles */
>+ qh->dev = urb->dev;
>+
>+ if (!usb_pipecontrol(urb->pipe))
>+ usb_settoggle(urb->dev, usb_pipeendpoint(urb->pipe),
>+ !is_input, 1);
>+ return qh;
>+}
>+
>+/*
>+ * create a list of filled qtds for this URB; won't link into qh.
>+ */
>+static struct list_head *qh_urb_transaction(struct isp1763_hcd *priv,
>+ struct urb *urb,
>+ struct list_head *head,
>+ gfp_t flags)
>+{
....
>+ /*
>+ * buffer gets wrapped in one or more qtds;
>+ * last one may be "short" (including zero len)
>+ * and may serve as a control status ack
>+ */
>+ for (;;) {
>+ int this_qtd_len;
>+
>+ if (!buf && len) {
>+ /* XXX This looks like usb storage / SCSI bug */
>+ printk(KERN_ERR
>+ "buf is null, dma is %08lx len is %d\n",
>+ (long unsigned) urb->transfer_dma, len);
>+ WARN_ON(1);
yeah, and it got fixed :)
>+ }
...
>+ return head;
>+
>+ cleanup:
>+ qtd_list_free(priv, urb, head);
>+ return NULL;
>+}
>+
>diff --git a/drivers/usb/isp1763/isp1763_udc.c b/drivers/usb/isp1763/isp1763_udc.c
>new file mode 100644
>index 0000000..74f1272
>--- /dev/null
>+++ b/drivers/usb/isp1763/isp1763_udc.c
>+
>+/* Ladies and gentlemen, I give you... THE WHEEL */
>+
>+/**
>+* queue_add - add item to queue
>+* @head: queue head
>+* @elem: element for which to create entry
>+*/
No, not another wheel. Please use struct list_head for this.
>+static void queue_add(struct queue **head, void *elem)
Sebastian
--
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
