add support for the TI VLYNQ bus
PATCH 2/9 add support for the TI VLYNQ bus
This patch adds support for the TI VLYNQ high-speed, serial and packetized bus. This bus allows external devices to be connected to the System-on-Chip and appear in the main system memory just like any memory mapped peripheral. It is widely used in TI's networking and mutlimedia SoC, including the AR7 SoC.
---
MAINTAINERS +8 -0
@@ -6130,6 +6130,14 @@ F: drivers/net/macvlan.c F: include/linux/if_*vlan.h F: net/8021q/ +VLYNQ BUS +P: Florian Fainelli +M: [email protected] +L: [email protected] +S: Maintained +F: drivers/vlynq/vlynq.c +F: include/linux/vlynq.h + VOLTAGE AND CURRENT REGULATOR FRAMEWORK P: Liam Girdwood M: [email protected]
drivers/vlynq/Kconfig new file +14 -0
@@ -0,0 +1,14 @@ +menu "TI VLYNQ" + +config VLYNQ + bool "TI VLYNQ bus support" + depends on AR7 && EXPERIMENTAL + help + Support for Texas Instruments(R) VLYNQ bus. + The VLYNQ bus is a high-speed, serial and packetized + data bus which allows external peripherals of a SoC + to appear into the system's main memory. + + If unsure, say N + +endmenu
drivers/vlynq/Makefile new file +5 -0
@@ -0,0 +1,5 @@ +# +# Makefile for kernel vlynq drivers +# + +obj-$(CONFIG_VLYNQ) += vlynq.o
drivers/vlynq/vlynq.c new file +792 -0
792 lines not on this page · raw message
include/linux/vlynq.h new file +161 -0
161 lines not on this page · raw message
Quoted 18 lines · On Mon, 1 Jun 2009 13:58:27 +0200 Florian Fainelli <[email protected]> wrote:
This patch adds support for the TI VLYNQ high-speed, serial and packetized bus. This bus allows external devices to be connected to the System-on-Chip and appear in the main system memory just like any memory mapped peripheral. It is widely used in TI's networking and mutlimedia SoC, including the AR7 SoC.
...
drivers/vlynq/vlynq.c new file +8
-0+struct vlynq_regs { +u32 revision; +u32 control; +u32 status; +u32 int_prio; +u32 int_status; +u32 int_pending; +u32 int_ptr;
drivers/vlynq/vlynq.c line 77
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+u32 tx_offset; +struct vlynq_mapping rx_mapping[4 ]; +u32 chip; +u32 autonego; +u32 unused[6 ]; +u32 int_device[8 ]; +}; + +#define vlynq_reg_read (reg) readl(&(reg)) +#define vlynq_reg_write (reg, val) writel(val, &(reg))
grumble. These just make the code harder to follow. it'd be better to open-code readl() and writel() at the callsites.
Quoted 18 lines
drivers/vlynq/vlynq.c new file +18
-0+static int __vlynq_enable_device (struct vlynq_device *dev); + +#ifdef VLYNQ_DEBUG +static void vlynq_dump_regs (struct vlynq_device *dev) +{ +int i; + +printk (KERN_DEBUG "VLYNQ local=%p remote=%p\n" , + dev->local, dev->remote); +for (i =0 ; i <32 ; i++) { +printk (KERN_DEBUG "VLYNQ: local %d: %08x\n" , + i +1 , ((u32 *)dev->local)[i]); +printk (KERN_DEBUG "VLYNQ: remote %d: %08x\n" , + i +1 , ((u32 *)dev->remote)[i]); + } +} + +static void vlynq_dump_mem (u32 *base,int count)
drivers/vlynq/vlynq.c line 106
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+{ +int i; + +for (i =0 ; i < (count +3 ) /4 ; i++) { +if (i %4 ==0 ) +printk (KERN_DEBUG "\nMEM[0x%04x]:" , i *4 ); +printk (KERN_DEBUG " 0x%08x" , *(base + i)); + } +printk (KERN_DEBUG "\n" ); +}
lib/hexdump.c?
drivers/vlynq/vlynq.c line 109
Quoted 3 lines
drivers/vlynq/vlynq.c new file +3
-0+#endif + +int vlynq_linked (struct vlynq_device *dev)
afacit this didn't need to be a kernel-wide symbol?
Please review the patchset for any unnecessarily-global symbols.
Quoted 16 lines
drivers/vlynq/vlynq.c new file +13
-0+{ +int i; + +for (i =0 ; i <100 ; i++) +if (vlynq_reg_read (dev->local->status) &VLYNQ_STATUS_LINK ) +return 1 ; +else +cpu_relax (); + +return 0 ; +} + +static void vlynq_remote_ack (unsigned int irq)
drivers/vlynq/vlynq.c line 219
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+{ +struct vlynq_device *dev =get_irq_chip_data (irq); + +u32 status =vlynq_reg_read (dev->remote->status); + +if (printk_ratelimit ()) +printk (KERN_DEBUG "%s: remote status: 0x%08x\n" , +dev_name (&dev->dev), status); +vlynq_reg_write (dev->remote->status, status); +}
This code seems to do rather a lot of printks for production code?
Quoted 10 lines
...
drivers/vlynq/vlynq.c new file +7
-0+static int vlynq_device_match (struct device *dev, +struct device_driver *drv) +{ +struct vlynq_device *vdev =to_vlynq_device (dev); +struct vlynq_driver *vdrv =to_vlynq_driver (drv); +struct vlynq_device_id *ids = vdrv->id_table; +
drivers/vlynq/vlynq.c line 344
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+while (ids->id) { +if (ids->id == vdev->dev_id) { + vdev->divisor = ids->divisor; +vlynq_set_drvdata (vdev, ids); +printk (KERN_INFO "Driver found for VLYNQ " \ +"device: %08x\n" , vdev->dev_id); +return 1 ; + } +printk (KERN_DEBUG "Not using the %08x VLYNQ device's driver" \ +" for VLYNQ device: %08x\n" , ids->id, vdev->dev_id);
The backslashes here are unneeded and unconventional.
Quoted 5 lines
drivers/vlynq/vlynq.c new file +5
-0+ ids++; + } +return 0 ; +} +
drivers/vlynq/vlynq.c line 359
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+static int vlynq_device_probe (struct device *dev) +{ +struct vlynq_device *vdev =to_vlynq_device (dev); +struct vlynq_driver *drv =to_vlynq_driver (dev->driver); +struct vlynq_device_id *id =vlynq_get_drvdata (vdev); +int result = -ENODEV ; + +get_device (dev); +if (drv && drv->probe) + result = drv->probe (vdev, id);
Can `drv' really be NULL here?
drivers/vlynq/vlynq.c line 369
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+if (result) +put_device (dev); +return result; +} + +static int vlynq_device_remove (struct device *dev) +{ +struct vlynq_driver *drv =to_vlynq_driver (dev->driver); +if (drv && drv->remove) + drv->remove (to_vlynq_device (dev));
ditto.
Quoted 16 lines
drivers/vlynq/vlynq.c new file +16
-0+put_device (dev); +return 0 ; +} + +int __vlynq_register_driver (struct vlynq_driver *driver,struct module *owner) +{ + driver->driver.name = driver->name; + driver->driver.bus = &vlynq_bus_type; +return driver_register (&driver->driver); +} +EXPORT_SYMBOL (__vlynq_register_driver); + +void vlynq_unregister_driver (struct vlynq_driver *driver) +{ +driver_unregister (&driver->driver); +}
drivers/vlynq/vlynq.c line 395
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+EXPORT_SYMBOL (vlynq_unregister_driver); + +static int __vlynq_try_remote (struct vlynq_device *dev) +{ +int i; + +vlynq_reset (dev); +for (i = dev->dev_id ? vlynq_rdiv2 : vlynq_rdiv8; dev->dev_id ? + i <= vlynq_rdiv8 : i >= vlynq_rdiv2; + dev->dev_id ? i++ : i--) {
Wow.
Quoted 17 lines
drivers/vlynq/vlynq.c new file +17
-0+if (!vlynq_linked (dev)) +break ; + +vlynq_reg_write (dev->remote->control, + (vlynq_reg_read (dev->remote->control) & + ~VLYNQ_CTRL_CLOCK_MASK ) | +VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_DIV (i - vlynq_rdiv1)); +vlynq_reg_write (dev->local->control, + ((vlynq_reg_read (dev->local->control) + & ~(VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_MASK )) | +VLYNQ_CTRL_CLOCK_DIV (i - vlynq_rdiv1))); + +if (vlynq_linked (dev)) { + printk(KERN_DEBUG + "%s: using remote clock divisor %d\n",
drivers/vlynq/vlynq.c line 423
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+ dev_name(&dev->dev), i - vlynq_rdiv1 + 1); + dev->divisor = i; +return 0 ; + }else { +vlynq_reset (dev); + } + } + +return -ENODEV ; +}
This code could do with a few comments.
See, this reader of your code doesn't know what vlynq_linked() does, so I don't have a hope of working out whey this function returns -ENODEV if it couldn't find any "linked" devices. What does this _mean_ in terms of the underlying hardware and its configuration?
Dunno. But it would be nice to be able to work that out from reading the code, no?
Quoted 43 lines
drivers/vlynq/vlynq.c new file +43
-0+static int __vlynq_try_local (struct vlynq_device *dev) +{ +int i; + +vlynq_reset (dev); + +for (i = dev->dev_id ? vlynq_ldiv2 : vlynq_ldiv8; dev->dev_id ? + i <= vlynq_ldiv8 : i >= vlynq_ldiv2; + dev->dev_id ? i++ : i--) { + +vlynq_reg_write (dev->local->control, + (vlynq_reg_read (dev->local->control) & + ~VLYNQ_CTRL_CLOCK_MASK ) | +VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_DIV (i - vlynq_ldiv1)); + +if (vlynq_linked (dev)) { +printk (KERN_DEBUG +"%s: using local clock divisor %d\n" , +dev_name (&dev->dev), i - vlynq_ldiv1 +1 ); + dev->divisor = i; +return 0 ; + }else { +vlynq_reset (dev); + } + } + +return -ENODEV ; +} + +static int __vlynq_try_external (struct vlynq_device *dev) +{ +vlynq_reset (dev); +if (!vlynq_linked (dev)) +return -ENODEV ; + +vlynq_reg_write (dev->remote->control, + (vlynq_reg_read (dev->remote->control) & + ~VLYNQ_CTRL_CLOCK_INT )); + +vlynq_reg_write (dev->local->control, + (vlynq_reg_read (dev->local->control) & + ~VLYNQ_CTRL_CLOCK_INT ));
drivers/vlynq/vlynq.c line 477
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+ +if (vlynq_linked (dev)) { +printk (KERN_DEBUG "%s: using external clock\n" , +dev_name (&dev->dev)); + dev->divisor = vlynq_div_external; +return 0 ; + } + +return -ENODEV ; +}
"remote", "local", "external". What do these terms mean? Perhaps there's a TI datasheet somewhere?
Quoted 6 lines
drivers/vlynq/vlynq.c new file +6
-0+static int __vlynq_enable_device (struct vlynq_device *dev) +{ +int result; +struct plat_vlynq_ops *ops = dev->dev.platform_data; + + result = ops->on (dev);
drivers/vlynq/vlynq.c line 494
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+if (result) +return result; + +switch (dev->divisor) { +case vlynq_div_external: +case vlynq_div_auto: +/* When the device is brought from reset it should have clock +generation negotiated by hardware. +Check which device is generating clocks and perform setup +accordingly */
Preferred comment layout format is
/*
* When the device is brought from reset it should have clock
* generation negotiated by hardware. Check which device is
* generating clocks and perform setup accordingly */
Quoted 5 lines
drivers/vlynq/vlynq.c new file +5
-0+if (vlynq_linked (dev) &&vlynq_reg_read (dev->remote->control) & +VLYNQ_CTRL_CLOCK_INT ) { +if (!__vlynq_try_remote (dev) || + !__vlynq_try_local (dev) || + !__vlynq_try_external (dev))
drivers/vlynq/vlynq.c line 509
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+return 0 ; + }else { +if (!__vlynq_try_external (dev) || + !__vlynq_try_local (dev) || + !__vlynq_try_remote (dev)) +return 0 ; + } +break ; +case vlynq_ldiv1:case vlynq_ldiv2:case vlynq_ldiv3:case vlynq_ldiv4: +case vlynq_ldiv5:case vlynq_ldiv6:case vlynq_ldiv7:case vlynq_ldiv8:
One `case' per line, please.
Quoted 36 lines
drivers/vlynq/vlynq.c new file +36
-0+vlynq_reg_write (dev->local->control, +VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_DIV (dev->divisor - + vlynq_ldiv1)); +vlynq_reg_write (dev->remote->control,0 ); +if (vlynq_linked (dev)) { +printk (KERN_DEBUG +"%s: using local clock divisor %d\n" , +dev_name (&dev->dev), + dev->divisor - vlynq_ldiv1 +1 ); +return 0 ; + } +break ; +case vlynq_rdiv1:case vlynq_rdiv2:case vlynq_rdiv3:case vlynq_rdiv4: +case vlynq_rdiv5:case vlynq_rdiv6:case vlynq_rdiv7:case vlynq_rdiv8: +vlynq_reg_write (dev->local->control,0 ); +vlynq_reg_write (dev->remote->control, +VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_DIV (dev->divisor - + vlynq_rdiv1)); +if (vlynq_linked (dev)) { +printk (KERN_DEBUG +"%s: using remote clock divisor %d\n" , +dev_name (&dev->dev), + dev->divisor - vlynq_rdiv1 +1 ); +return 0 ; + } +break ; + } + + ops->off (dev); +return -ENODEV ; +} + +int vlynq_enable_device (struct vlynq_device *dev) +{
drivers/vlynq/vlynq.c line 555
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+struct plat_vlynq_ops *ops = dev->dev.platform_data; +int result = -ENODEV ; + + result =__vlynq_enable_device (dev); +if (result) +return result; + + result =vlynq_setup_irq (dev); +if (result) + ops->off (dev);
It's strange that this function directly calls __vlynq_enable_device(), and undoes that via ops->off() if vlynq_setup_irq() failed.
I'd have expected to see a call to ops->off() used as a cancallation for ops->on()?
Quoted 6 lines
include/linux/vlynq.h new file +6
-0+ dev->enabled = !result; +return result; +} +EXPORT_SYMBOL (vlynq_enable_device); + +
include/linux/vlynq.h line 67
Quoted 10 lines
include/linux/vlynq.h new file +7
-0+struct vlynq_device { +u32 id, dev_id; +int local_irq; +int remote_irq; +enum vlynq_divisor divisor; +u32 regs_start, regs_end; +u32 mem_start, mem_end;
This doesn't look 64-bit-bus friendly.
Quoted 10 lines
include/linux/vlynq.h new file +7
-0+u32 irq_start, irq_end; +int irq; +int enabled; +struct vlynq_regs *local; +struct vlynq_regs *remote; +struct device dev; +};
Hi Andrew,
Le Tuesday 02 June 2009 07:08:54 Andrew Morton, vous avez écrit :
Quoted 1 line
On Mon, 1 Jun 2009 13:58:27 +0200 Florian Fainelli <[email protected]>
Quoted 23 lines · wrote:
Quoted 23 lines
This patch adds support for the TI VLYNQ high-speed, serial and packetized bus. This bus allows external devices to be connected to the System-on-Chip and appear in the main system memory just like any memory mapped peripheral. It is widely used in TI's networking and mutlimedia SoC, including the AR7 SoC.
...
drivers/vlynq/vlynq.c new file +13
-0+struct vlynq_regs { +u32 revision; +u32 control; +u32 status; +u32 int_prio; +u32 int_status; +u32 int_pending; +u32 int_ptr; +u32 tx_offset; +struct vlynq_mapping rx_mapping[4 ]; +u32 chip; +u32 autonego; +u32 unused[6 ];
Quoted 8 lines
drivers/vlynq/vlynq.c line 77
Quoted 5 lines
drivers/vlynq/vlynq.c new file +5
-0+u32 int_device[8 ]; +}; + +#define vlynq_reg_read (reg) readl(&(reg)) +#define vlynq_reg_write (reg, val) writel(val, &(reg))grumble. These just make the code harder to follow. it'd be better to open-code
readl()andwritel()at the callsites.
Ok
Quoted 23 lines
Quoted 23 lines
drivers/vlynq/vlynq.c new file +22
-0+static int __vlynq_enable_device (struct vlynq_device *dev); + +#ifdef VLYNQ_DEBUG +static void vlynq_dump_regs (struct vlynq_device *dev) +{ +int i; + +printk (KERN_DEBUG "VLYNQ local=%p remote=%p\n" , + dev->local, dev->remote); +for (i =0 ; i <32 ; i++) { +printk (KERN_DEBUG "VLYNQ: local %d: %08x\n" , + i +1 , ((u32 *)dev->local)[i]); +printk (KERN_DEBUG "VLYNQ: remote %d: %08x\n" , + i +1 , ((u32 *)dev->remote)[i]); + } +} + +static void vlynq_dump_mem (u32 *base,int count) +{ +int i; + +for (i =0 ; i < (count +3 ) /4 ; i++) {
Quoted 8 lines
drivers/vlynq/vlynq.c line 106
Quoted 6 lines
drivers/vlynq/vlynq.c new file +6
-0+if (i %4 ==0 ) +printk (KERN_DEBUG "\nMEM[0x%04x]:" , i *4 ); +printk (KERN_DEBUG " 0x%08x" , *(base + i)); + } +printk (KERN_DEBUG "\n" ); +}
lib/hexdump.c?
Ok will use it.
Quoted 24 lines
drivers/vlynq/vlynq.c line 109
Quoted 4 lines
drivers/vlynq/vlynq.c new file +3
-0+#endif + +int vlynq_linked (struct vlynq_device *dev)afacit this didn't need to be a kernel-wide symbol?
Please review the patchset for any unnecessarily-global symbols.
Quoted 15 lines
drivers/vlynq/vlynq.c new file +12
-0+{ +int i; + +for (i =0 ; i <100 ; i++) +if (vlynq_reg_read (dev->local->status) &VLYNQ_STATUS_LINK ) +return 1 ; +else +cpu_relax (); + +return 0 ; +} +
Quoted 13 lines
drivers/vlynq/vlynq.c line 219
Quoted 11 lines
drivers/vlynq/vlynq.c new file +11
-0+static void vlynq_remote_ack (unsigned int irq) +{ +struct vlynq_device *dev =get_irq_chip_data (irq); + +u32 status =vlynq_reg_read (dev->remote->status); + +if (printk_ratelimit ()) +printk (KERN_DEBUG "%s: remote status: 0x%08x\n" , +dev_name (&dev->dev), status); +vlynq_reg_write (dev->remote->status, status); +}This code seems to do rather a lot of printks for production code?
I will use pr_debug instead.
Quoted 113 lines
Quoted 10 lines
...
drivers/vlynq/vlynq.c new file +7
-0+static int vlynq_device_match (struct device *dev, +struct device_driver *drv) +{ +struct vlynq_device *vdev =to_vlynq_device (dev); +struct vlynq_driver *vdrv =to_vlynq_driver (drv); +struct vlynq_device_id *ids = vdrv->id_table; +drivers/vlynq/vlynq.c line 344
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+while (ids->id) { +if (ids->id == vdev->dev_id) { + vdev->divisor = ids->divisor; +vlynq_set_drvdata (vdev, ids); +printk (KERN_INFO "Driver found for VLYNQ " \ +"device: %08x\n" , vdev->dev_id); +return 1 ; + } +printk (KERN_DEBUG "Not using the %08x VLYNQ device's driver" \ +" for VLYNQ device: %08x\n" , ids->id, vdev->dev_id);The backslashes here are unneeded and unconventional.
Quoted 5 lines
drivers/vlynq/vlynq.c new file +5
-0+ ids++; + } +return 0 ; +} +drivers/vlynq/vlynq.c line 359
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+static int vlynq_device_probe (struct device *dev) +{ +struct vlynq_device *vdev =to_vlynq_device (dev); +struct vlynq_driver *drv =to_vlynq_driver (dev->driver); +struct vlynq_device_id *id =vlynq_get_drvdata (vdev); +int result = -ENODEV ; + +get_device (dev); +if (drv && drv->probe) + result = drv->probe (vdev, id);Can `drv' really be NULL here?
drivers/vlynq/vlynq.c line 369
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+if (result) +put_device (dev); +return result; +} + +static int vlynq_device_remove (struct device *dev) +{ +struct vlynq_driver *drv =to_vlynq_driver (dev->driver); +if (drv && drv->remove) + drv->remove (to_vlynq_device (dev));ditto.
Quoted 16 lines
drivers/vlynq/vlynq.c new file +5
-0+put_device (dev); +return 0 ; +} + +int __vlynq_register_driver(struct vlynq_driver *driver, struct module*owner) +{
drivers/vlynq/vlynq.c new file +10
-0+ driver->driver.name = driver->name; + driver->driver.bus = &vlynq_bus_type; +return driver_register (&driver->driver); +} +EXPORT_SYMBOL (__vlynq_register_driver); + +void vlynq_unregister_driver (struct vlynq_driver *driver) +{ +driver_unregister (&driver->driver); +}drivers/vlynq/vlynq.c line 395
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+EXPORT_SYMBOL (vlynq_unregister_driver); + +static int __vlynq_try_remote (struct vlynq_device *dev) +{ +int i; + +vlynq_reset (dev); +for (i = dev->dev_id ? vlynq_rdiv2 : vlynq_rdiv8; dev->dev_id ? + i <= vlynq_rdiv8 : i >= vlynq_rdiv2; + dev->dev_id ? i++ : i--) {Wow.
Quoted 17 lines
drivers/vlynq/vlynq.c new file +17
-0+if (!vlynq_linked (dev)) +break ; + +vlynq_reg_write (dev->remote->control, + (vlynq_reg_read (dev->remote->control) & + ~VLYNQ_CTRL_CLOCK_MASK ) | +VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_DIV (i - vlynq_rdiv1)); +vlynq_reg_write (dev->local->control, + ((vlynq_reg_read (dev->local->control) + & ~(VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_MASK )) | +VLYNQ_CTRL_CLOCK_DIV (i - vlynq_rdiv1))); + +if (vlynq_linked (dev)) { + printk(KERN_DEBUG + "%s: using remote clock divisor %d\n",drivers/vlynq/vlynq.c line 423
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+ dev_name(&dev->dev), i - vlynq_rdiv1 + 1); + dev->divisor = i; +return 0 ; + }else { +vlynq_reset (dev); + } + } + +return -ENODEV ; +}This code could do with a few comments.
Quoted 4 lines
See, this reader of your code doesn't know what
vlynq_linked()does, so I don't have a hope of working out whey this function returns-ENODEVif it couldn't find any "linked" devices. What does this _mean_ in terms of the underlying hardware and its configuration?
VLYNQ devices can be daisy-chained that's why we want to know whether they are linked or not and return ENODEV in case there is no device.
Quoted 52 lines
Dunno. But it would be nice to be able to work that out from reading the code, no?
Quoted 48 lines
drivers/vlynq/vlynq.c new file +48
-0+static int __vlynq_try_local (struct vlynq_device *dev) +{ +int i; + +vlynq_reset (dev); + +for (i = dev->dev_id ? vlynq_ldiv2 : vlynq_ldiv8; dev->dev_id ? + i <= vlynq_ldiv8 : i >= vlynq_ldiv2; + dev->dev_id ? i++ : i--) { + +vlynq_reg_write (dev->local->control, + (vlynq_reg_read (dev->local->control) & + ~VLYNQ_CTRL_CLOCK_MASK ) | +VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_DIV (i - vlynq_ldiv1)); + +if (vlynq_linked (dev)) { +printk (KERN_DEBUG +"%s: using local clock divisor %d\n" , +dev_name (&dev->dev), i - vlynq_ldiv1 +1 ); + dev->divisor = i; +return 0 ; + }else { +vlynq_reset (dev); + } + } + +return -ENODEV ; +} + +static int __vlynq_try_external (struct vlynq_device *dev) +{ +vlynq_reset (dev); +if (!vlynq_linked (dev)) +return -ENODEV ; + +vlynq_reg_write (dev->remote->control, + (vlynq_reg_read (dev->remote->control) & + ~VLYNQ_CTRL_CLOCK_INT )); + +vlynq_reg_write (dev->local->control, + (vlynq_reg_read (dev->local->control) & + ~VLYNQ_CTRL_CLOCK_INT )); + +if (vlynq_linked (dev)) { +printk (KERN_DEBUG "%s: using external clock\n" , +dev_name (&dev->dev)); + dev->divisor = vlynq_div_external;
Quoted 8 lines
drivers/vlynq/vlynq.c line 423
Quoted 5 lines
drivers/vlynq/vlynq.c new file +5
-0+return 0 ; + } + +return -ENODEV ; +}"remote", "local", "external". What do these terms mean? Perhaps there's a TI datasheet somewhere?
There is an implementation datasheet here :
http://www.ti.com/litv/pdf/sprue36a which is pretty detailed. "remote" refers to the device that is connected to the vlynq bus master and is a separate chip from the SoC.
"external" and "local" refer to the clockig methods that can be used to clock out remote VLYNQ devices. "external" clocking source is an oscillator shared by both the bus master and the remote device, while "local" uses a dedicated clocking line between the bus master and the remote device.
Quoted 94 lines
Quoted 7 lines
drivers/vlynq/vlynq.c new file +6
-0+static int __vlynq_enable_device (struct vlynq_device *dev) +{ +int result; +struct plat_vlynq_ops *ops = dev->dev.platform_data; + + result = ops->on (dev);drivers/vlynq/vlynq.c line 494
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+if (result) +return result; + +switch (dev->divisor) { +case vlynq_div_external: +case vlynq_div_auto: +/* When the device is brought from reset it should have clock +generation negotiated by hardware. +Check which device is generating clocks and perform setup +accordingly */ Preferred comment layout format is
/*
* When the device is brought from reset it should have clock
* generation negotiated by hardware. Check which device is
* generating clocks and perform setup accordingly */Quoted 5 lines
drivers/vlynq/vlynq.c new file +5
-0+if (vlynq_linked (dev) &&vlynq_reg_read (dev->remote->control) & +VLYNQ_CTRL_CLOCK_INT ) { +if (!__vlynq_try_remote (dev) || + !__vlynq_try_local (dev) || + !__vlynq_try_external (dev))drivers/vlynq/vlynq.c line 509
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+return 0 ; + }else { +if (!__vlynq_try_external (dev) || + !__vlynq_try_local (dev) || + !__vlynq_try_remote (dev)) +return 0 ; + } +break ; +case vlynq_ldiv1:case vlynq_ldiv2:case vlynq_ldiv3:case vlynq_ldiv4: +case vlynq_ldiv5:case vlynq_ldiv6:case vlynq_ldiv7:case vlynq_ldiv8:One `case' per line, please.
Quoted 36 lines
drivers/vlynq/vlynq.c new file +36
-0+vlynq_reg_write (dev->local->control, +VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_DIV (dev->divisor - + vlynq_ldiv1)); +vlynq_reg_write (dev->remote->control,0 ); +if (vlynq_linked (dev)) { +printk (KERN_DEBUG +"%s: using local clock divisor %d\n" , +dev_name (&dev->dev), + dev->divisor - vlynq_ldiv1 +1 ); +return 0 ; + } +break ; +case vlynq_rdiv1:case vlynq_rdiv2:case vlynq_rdiv3:case vlynq_rdiv4: +case vlynq_rdiv5:case vlynq_rdiv6:case vlynq_rdiv7:case vlynq_rdiv8: +vlynq_reg_write (dev->local->control,0 ); +vlynq_reg_write (dev->remote->control, +VLYNQ_CTRL_CLOCK_INT | +VLYNQ_CTRL_CLOCK_DIV (dev->divisor - + vlynq_rdiv1)); +if (vlynq_linked (dev)) { +printk (KERN_DEBUG +"%s: using remote clock divisor %d\n" , +dev_name (&dev->dev), + dev->divisor - vlynq_rdiv1 +1 ); +return 0 ; + } +break ; + } + + ops->off (dev); +return -ENODEV ; +} + +int vlynq_enable_device (struct vlynq_device *dev) +{drivers/vlynq/vlynq.c line 555
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+struct plat_vlynq_ops *ops = dev->dev.platform_data; +int result = -ENODEV ; + + result =__vlynq_enable_device (dev); +if (result) +return result; + + result =vlynq_setup_irq (dev); +if (result) + ops->off (dev);It's strange that this function directly calls
__vlynq_enable_device(), and undoes that viaops->off()ifvlynq_setup_irq()failed.
Quoted 2 lines
I'd have expected to see a call to
ops->off()used as a cancallation forops->on()?
It would be more consistent, you are right.
Quoted 10 lines
Quoted 10 lines
drivers/vlynq/vlynq.c new file +6
-0+ dev->enabled = !result; +return result; +} +EXPORT_SYMBOL (vlynq_enable_device); + +
Quoted 9 lines
include/linux/vlynq.h line 67
Quoted 7 lines
include/linux/vlynq.h new file +7
-0+struct vlynq_device { +u32 id, dev_id; +int local_irq; +int remote_irq; +enum vlynq_divisor divisor; +u32 regs_start, regs_end; +u32 mem_start, mem_end;This doesn't look 64-bit-bus friendly.
The bus is not intended to be available on 64-bits architecture, at least I did not see any using VLYNQ and since existing users of it are MIPS32 or ARM9-based System-on-Chip, TI would have to redesign/extend VLYNQ to make it work on 64-bits architecture.
Thanks for your comments, will respin with them addressed.
Signature · 4 lines
Best regards, Florian Fainelli Email : [email protected] http://openwrt.org -------------------------------
Le Tuesday 02 June 2009 07:08:54 Andrew Morton, vous avez écrit :
Quoted 1 line
On Mon, 1 Jun 2009 13:58:27 +0200 Florian Fainelli <[email protected]>
Quoted 23 lines · wrote:
Quoted 23 lines
This patch adds support for the TI VLYNQ high-speed, serial and packetized bus. This bus allows external devices to be connected to the System-on-Chip and appear in the main system memory just like any memory mapped peripheral. It is widely used in TI's networking and mutlimedia SoC, including the AR7 SoC.
...
drivers/vlynq/vlynq.c new file +13
-0+struct vlynq_regs { +u32 revision; +u32 control; +u32 status; +u32 int_prio; +u32 int_status; +u32 int_pending; +u32 int_ptr; +u32 tx_offset; +struct vlynq_mapping rx_mapping[4 ]; +u32 chip; +u32 autonego; +u32 unused[6 ];
Quoted 8 lines
drivers/vlynq/vlynq.c line 77
Quoted 5 lines
drivers/vlynq/vlynq.c new file +5
-0+u32 int_device[8 ]; +}; + +#define vlynq_reg_read (reg) readl(&(reg)) +#define vlynq_reg_write (reg, val) writel(val, &(reg))grumble. These just make the code harder to follow. it'd be better to open-code
readl()andwritel()at the callsites.
I do not understand how to fix this. Would an inlined accessors be a better solution for you?
Thanks.
Signature · 4 lines
Best regards, Florian Fainelli Email : [email protected] http://openwrt.org -------------------------------
Quoted 35 lines · On Thu, 4 Jun 2009 12:52:18 +0200 Florian Fainelli <[email protected]> wrote:
Le Tuesday 02 June 2009 07:08:54 Andrew Morton, vous avez
__crit__:Quoted 1 line
On Mon, 1 Jun 2009 13:58:27 +0200 Florian Fainelli <[email protected]>
Quoted 23 lines · wrote:
Quoted 23 lines
This patch adds support for the TI VLYNQ high-speed, serial and packetized bus. This bus allows external devices to be connected to the System-on-Chip and appear in the main system memory just like any memory mapped peripheral. It is widely used in TI's networking and mutlimedia SoC, including the AR7 SoC.
...
drivers/vlynq/vlynq.c new file +13
-0+struct vlynq_regs { +u32 revision; +u32 control; +u32 status; +u32 int_prio; +u32 int_status; +u32 int_pending; +u32 int_ptr; +u32 tx_offset; +struct vlynq_mapping rx_mapping[4 ]; +u32 chip; +u32 autonego; +u32 unused[6 ];Quoted 8 lines
drivers/vlynq/vlynq.c line 77
Quoted 5 lines
drivers/vlynq/vlynq.c new file +5
-0+u32 int_device[8 ]; +}; + +#define vlynq_reg_read (reg) readl(&(reg)) +#define vlynq_reg_write (reg, val) writel(val, &(reg))grumble. These just make the code harder to follow. it'd be better to open-code
readl()andwritel()at the callsites.
Quoted 2 lines
I do not understand how to fix this. Would an inlined accessors be a better solution for you?
Just remove the accessors altogether. Each place where there is a call to vlynq_reg_read(), replace that with a call to readl().
Unless there's a reason not to do this. For example, some hardware might require a udelay(1) before each writel(), or some platforms might want to use outl()/inl(). In cases like these, sure, standalone functions are needed to handle them.
But if vlynq_reg_read() and vlynq_reg_write() will never do anything apart from a bare readl()/writel() then let's just remove them altogether, as they add nothing.
Quoted 4 lines · Florian Fainelli
drivers/vlynq/vlynq.c new file +4
-0+static int vlynq_device_probe (struct device *dev) +{ +struct vlynq_device *vdev =to_vlynq_device (dev); +struct vlynq_driver *drv =to_vlynq_driver (dev->driver);
drivers/vlynq/vlynq.c line 363
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+struct vlynq_device_id *id =vlynq_get_drvdata (vdev); +int result = -ENODEV ; + +get_device (dev); +if (drv && drv->probe) + result = drv->probe (vdev, id); +if (result) +put_device (dev); +return result; +}
I don't think you need to increment the device reference counter here. The caller has already done that.
drivers/vlynq/vlynq.c line 372
Quoted 9 lines
drivers/vlynq/vlynq.c new file +9
-0+ +static int vlynq_device_remove (struct device *dev) +{ +struct vlynq_driver *drv =to_vlynq_driver (dev->driver); +if (drv && drv->remove) + drv->remove (to_vlynq_device (dev)); +put_device (dev); +return 0 ; +}
And this put_device I think is a bug - the caller should decrement the reference.
Ralf
Quoted 7 lines · Florian Fainelli <florian <at> openwrt.org> writes:
This patch adds support for the TI VLYNQ high-speed, serial and packetized bus. This bus allows external devices to be connected to the System-on-Chip and appear in the main system memory just like any memory mapped peripheral. It is widely used in TI's networking and mutlimedia SoC, including the AR7 SoC.
In addition, the VLYNQ connectivity can also enable usage of one or more simultaneous functionitilies (such as WLAN, Ethernet or other peripherals) of o one SoC on another. In this sense, a single VLYNQ bus interface supports multifunctional devices or multiple peripherals and not just one peripheral or device.
Quoted 33 lines
...
...drivers/vlynq/vlynq.c new file +29
-0+ +int vlynq_set_local_mapping (struct vlynq_device *dev,u32 tx_offset, +struct vlynq_mapping *mapping) +{ +int i; + +if (!dev->enabled) +return -ENXIO ; + +vlynq_reg_write (dev->local->tx_offset, tx_offset); +for (i =0 ; i <4 ; i++) { +vlynq_reg_write (dev->local->rx_mapping[i].offset, + mapping[i].offset); +vlynq_reg_write (dev->local->rx_mapping[i].size, + mapping[i].size); + } +return 0 ; +} +EXPORT_SYMBOL (vlynq_set_local_mapping); + +int vlynq_set_remote_mapping (struct vlynq_device *dev,u32 tx_offset, +struct vlynq_mapping *mapping) +{ +int i; + +if (!dev->enabled) +return -ENXIO ; + +vlynq_reg_write (dev->remote->tx_offset, tx_offset);
drivers/vlynq/vlynq.c line 610
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+for (i =0 ; i <4 ; i++) { +vlynq_reg_write (dev->remote->rx_mapping[i].offset, + mapping[i].offset); +vlynq_reg_write (dev->remote->rx_mapping[i].size, + mapping[i].size); + } +return 0 ; +} +EXPORT_SYMBOL (vlynq_set_remote_mapping); +
It seems that the current VLYNQ patch assumes that there is just one funcitonality or functional device (vlynq_device) that is attached with the VLYNQ. It will help to separate out the configuration of VLYNQ (through vlynq_set_local_mapping / vlynq_set_remote_mapping) from the functionalities (i.e. vlynq_device) that can be supported over VLYNQ. Once, the functional devices / drivers are isolated from the SoC specific VLYNQ configurations, the drivers can be independently re-used across multiple VLYNQ based SoC(s).
Some of the other potential benefits, down the line, arising out of the stated isolation would be the discovery & auto-configuration of VLYNQ SoC(s), construction of daisy-chain and derive enumerated values (such as base addresses and interrupt numbers) required by the peripheral or functional drivers.
Specifically, the VLYNQ (SoC) configuration is platform specific and functional drivers (vlynq_device & vlynq_driver) should not be attached to it.
Thanks,
Suraj
Hi Suraj,
Le Monday 08 June 2009 13:28:28 Suraj Iyer, vous avez écrit :
Quoted 57 lines
Quoted 6 lines · Florian Fainelli <florian <at> openwrt.org> writes:
This patch adds support for the TI VLYNQ high-speed, serial and packetized bus. This bus allows external devices to be connected to the System-on-Chip and appear in the main system memory just like any memory mapped peripheral. It is widely used in TI's networking and mutlimedia SoC, including the AR7 SoC.
In addition, the VLYNQ connectivity can also enable usage of one or more simultaneous functionitilies (such as WLAN, Ethernet or other peripherals) of o one SoC on another. In this sense, a single VLYNQ bus interface supports multifunctional devices or multiple peripherals and not just one peripheral or device.
Quoted 32 lines
...
...drivers/vlynq/vlynq.c new file +29
-0+ +int vlynq_set_local_mapping (struct vlynq_device *dev,u32 tx_offset, +struct vlynq_mapping *mapping) +{ +int i; + +if (!dev->enabled) +return -ENXIO ; + +vlynq_reg_write (dev->local->tx_offset, tx_offset); +for (i =0 ; i <4 ; i++) { +vlynq_reg_write (dev->local->rx_mapping[i].offset, + mapping[i].offset); +vlynq_reg_write (dev->local->rx_mapping[i].size, + mapping[i].size); + } +return 0 ; +} +EXPORT_SYMBOL (vlynq_set_local_mapping); + +int vlynq_set_remote_mapping (struct vlynq_device *dev,u32 tx_offset, +struct vlynq_mapping *mapping) +{ +int i; + +if (!dev->enabled) +return -ENXIO ; + +vlynq_reg_write (dev->remote->tx_offset, tx_offset);drivers/vlynq/vlynq.c line 610
Quoted 10 lines
drivers/vlynq/vlynq.c new file +10
-0+for (i =0 ; i <4 ; i++) { +vlynq_reg_write (dev->remote->rx_mapping[i].offset, + mapping[i].offset); +vlynq_reg_write (dev->remote->rx_mapping[i].size, + mapping[i].size); + } +return 0 ; +} +EXPORT_SYMBOL (vlynq_set_remote_mapping); +
Quoted 8 lines
It seems that the current VLYNQ patch assumes that there is just one funcitonality or functional device (
vlynq_device) that is attached with the VLYNQ. It will help to separate out the configuration of VLYNQ (throughvlynq_set_local_mapping/vlynq_set_remote_mapping) from the functionalities (i.e.vlynq_device) that can be supported over VLYNQ. Once, the functional devices / drivers are isolated from the SoC specific VLYNQ configurations, the drivers can be independently re-used across multiple VLYNQ based SoC(s).
The device that allowed writing this VLYNQ implementation is a TI AR7 SoC, to which only a WLAN device is attached afaik, therefore we did not handled all the VLYNQ topology cases. If you want to submit an incremental patch that enhances the current implementation I will be glad to test and review it.
Quoted 19 lines
Some of the other potential benefits, down the line, arising out of the stated isolation would be the discovery & auto-configuration of VLYNQ SoC(s), construction of daisy-chain and derive enumerated values (such as base addresses and interrupt numbers) required by the peripheral or functional drivers.
Specifically, the VLYNQ (SoC) configuration is platform specific and functional drivers (
vlynq_device&vlynq_driver) should not be attached to it.Thanks,
Suraj--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 4 lines
Best regards, Florian Fainelli Email : [email protected] http://openwrt.org -------------------------------