Merge the Sonics Silicon Backplane subsystem
Subsystems (3)
PATCH 1/1 Merge the Sonics Silicon Backplane subsystem
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.
drivers/Makefile +1 -0
@@ -87,3 +87,4 @@ obj-$(CONFIG_DMA_ENGINE) += dma/ obj-$(CONFIG_HID) += hid/ obj-$(CONFIG_PPC_PS3) += ps3/ obj-$(CONFIG_OF) += of/ +obj-$(CONFIG_SSB) += ssb/
drivers/Kconfig +2 -0
@@ -58,6 +58,8 @@ source "drivers/power/Kconfig" source "drivers/hwmon/Kconfig" +source "drivers/ssb/Kconfig" + source "drivers/mfd/Kconfig" source "drivers/media/Kconfig"
drivers/ssb/Makefile new file +13 -0
@@ -0,0 +1,13 @@ +# core +ssb-y += main.o scan.o + +# host support +ssb-$(CONFIG_SSB_PCIHOST) += pci.o pcihost_wrapper.o +ssb-$(CONFIG_SSB_PCMCIAHOST) += pcmcia.o + +# built-in drivers +ssb-y += driver_chipcommon.o +ssb-$(CONFIG_SSB_DRIVER_MIPS) += driver_mipscore.o +ssb-$(CONFIG_SSB_DRIVER_PCICORE) += driver_pcicore.o + +obj-$(CONFIG_SSB) += ssb.o
drivers/ssb/Kconfig new file +92 -0
92 lines not on this page · raw message
drivers/ssb/driver_chipcommon.c new file +394 -0
394 lines not on this page · raw message
drivers/ssb/driver_mipscore.c new file +258 -0
258 lines not on this page · raw message
drivers/ssb/driver_pcicore.c new file +564 -0
564 lines not on this page · raw message
drivers/ssb/main.c new file +1104 -0
1,104 lines not on this page · raw message
drivers/ssb/pci.c new file +697 -0
563 lines not on this page · raw message
drivers/ssb/pcihost_wrapper.c new file +104 -0
104 lines not on this page · raw message
drivers/ssb/pcmcia.c new file +264 -0
264 lines not on this page · raw message
drivers/ssb/scan.c new file +413 -0
413 lines not on this page · raw message
drivers/ssb/ssb_private.h new file +121 -0
121 lines not on this page · raw message
include/linux/ssb/ssb_driver_chipcommon.h new file +385 -0
385 lines not on this page · raw message
include/linux/ssb/ssb_driver_extif.h new file +160 -0
160 lines not on this page · raw message
include/linux/ssb/ssb_driver_mips.h new file +44 -0
44 lines not on this page · raw message
include/linux/ssb/ssb_driver_pci.h new file +106 -0
106 lines not on this page · raw message
include/linux/ssb/ssb.h new file +431 -0
431 lines not on this page · raw message
include/linux/ssb/ssb_regs.h new file +292 -0
292 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Part of the diff is not shown here. View the raw message.
Quoted 6 lines · On Fri, 27 Jul 2007 18:57:20 +0200 Michael Buesch…
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.
hm, slim pickings for me here. Nice looking code.
checkpatch finds a few coding style glitches. Lots of 80-col violations which you might choose ignore (comments on #defines in headers are especially hard) but these guys:
ERROR: "foo * bar" should be "foo *bar"
#4156: FILE: drivers/ssb/ssb_private.h:119:
+extern struct ssb_bus * ssb_pci_dev_to_bus(struct pci_dev *pdev);
are worth addressing.
drivers/ssb/driver_pcicore.c line 51
Quoted 10 lines
...
drivers/ssb/driver_pcicore.c new file +2
-0@@ -0,0 +1,564 @@ +u32 pci_iobase =0x100 ; +u32 pci_membase =SSB_PCI_DMA ;
These are quite poor names for global symbols.
drivers/ssb/driver_pcicore.c line 277
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+static struct resource ssb_pcicore_mem_resource = { + .name ="SSB PCIcore external memory" , + .start =SSB_PCI_DMA , + .end = (u32 )SSB_PCI_DMA + (u32 )SSB_PCI_DMA_SZ -1 , + .flags =IORESOURCE_MEM , +};
start and end here are resource_size_t. Forcing them to u32 seems inappropriate.
Quoted 1 line
+void
ssb_pcicore_init(struct ssb_pcicore *pc)
Please check that all newly-added global symbols do indeed need global scope.
Quoted 10 lines
drivers/ssb/driver_pcicore.c new file +10
-0+static void ssb_pcie_mdio_write (struct ssb_pcicore *pc,u8 device, +u8 address,u16 data) +{ +const u16 mdio_control =0x128 ; +const u16 mdio_data =0x12C ; +u32 v; +int i; + + v =0x80 ;/* Enable Preamble Sequence */ + v |=0x2 ;/* MDIO Clock Divisor */
drivers/ssb/driver_pcicore.c line 434
Quoted 10 lines
drivers/ssb/driver_pcicore.c new file +10
-0+pcicore_write32 (pc, mdio_control, v); + + v = (1 <<30 );/* Start of Transaction */ + v |= (1 <<28 );/* Write Transaction */ + v |= (1 <<17 );/* Turnaround */ + v |= (u32 )device <<22 ; + v |= (u32 )address <<18 ; + v |= data; +pcicore_write32 (pc, mdio_data, v); +udelay (10 );
mystery udelays are usually worth a comment.
Quoted 13 lines
drivers/ssb/driver_pcicore.c new file +10
-0+for (i =0 ; i <10 ; i++) { + v =pcicore_read32 (pc, mdio_control); +if (v &0x100 /* Trans complete */ ) +break ; +msleep (1 ); + } +pcicore_write32 (pc, mdio_control,0 ); +} + +static void ssb_buses_lock (void )
drivers/ssb/driver_pcicore.c line 466
Quoted 10 lines
drivers/ssb/driver_pcicore.c new file +10
-0+{ +if (!ssb_is_early_boot) +mutex_lock (&buses_mutex); +} + +static void ssb_buses_unlock (void ) +{ +if (!ssb_is_early_boot) +mutex_unlock (&buses_mutex); +}
Some comments are neeeded here to explain why we're jumping through this hoop. It's normally OK to take a mutex early in boot, so one wonders what's happening.
Quoted 1 line
+EXPORT_SYMBOL(ssb_clockspeed);
Please check that all newly-added exports really need to be exported (I'm sure they do, but..)
Quoted 13 lines
drivers/ssb/driver_pcicore.c new file +13
-0+static int ssb_wait_bit (struct ssb_device *dev,u16 reg,u32 bitmask, +int timeout,int set) +{ +int i; +u32 val; + +for (i =0 ; i < timeout; i++) { + val =ssb_read32 (dev, reg); +if (set) { +if (val & bitmask) +return 0 ; + }else { +if (!(val & bitmask))
drivers/ssb/driver_pcicore.c
Quoted 10 lines
drivers/ssb/driver_pcicore.c new file +10
-0+return 0 ; + } +udelay (10 ); + } + printk(KERN_ERR PFX "Timeout waiting for bitmask %08X on " +"register %04X to %s.\n" , + bitmask, reg, (set ? "set" : "clear")); + +return -ETIMEDOUT ; +}
So `timeout' is in units of ten-microseconds. Unusual.
Quoted 15 lines
drivers/ssb/driver_pcicore.c new file +15
-0+void ssb_device_disable (struct ssb_device *dev,u32 core_specific_flags) +{ +u32 reject; + +if (ssb_read32 (dev,SSB_TMSLOW ) &SSB_TMSLOW_RESET ) +return ; + + reject =ssb_tmslow_reject_bitmask (dev); +ssb_write32 (dev,SSB_TMSLOW , reject |SSB_TMSLOW_CLOCK ); +ssb_wait_bit (dev,SSB_TMSLOW , reject,1000 ,1 ); +ssb_wait_bit (dev,SSB_TMSHIGH ,SSB_TMSHIGH_BUSY ,1000 ,0 ); +ssb_write32 (dev,SSB_TMSLOW , +SSB_TMSLOW_FGC |SSB_TMSLOW_CLOCK | + reject |SSB_TMSLOW_RESET | + core_specific_flags);
drivers/ssb/driver_pcicore.c
Quoted 10 lines
drivers/ssb/driver_pcicore.c new file +10
-0+/* flush */ +ssb_read32 (dev,SSB_TMSLOW ); +udelay (1 ); + +ssb_write32 (dev,SSB_TMSLOW , + reject |SSB_TMSLOW_RESET | + core_specific_flags); +/* flush */ +ssb_read32 (dev,SSB_TMSLOW ); +udelay (1 );
mystery udelay.
drivers/ssb/driver_pcicore.c
Quoted 11 lines
drivers/ssb/driver_pcicore.c new file +8
-0+} +EXPORT_SYMBOL (ssb_device_disable); +const struct ssb_bus_ops ssb_pci_ops = { + .read16 = ssb_pci_read16, + .read32 = ssb_pci_read32, + .write16 = ssb_pci_write16, + .write32 = ssb_pci_write32, +};
static?
Quoted 22 lines
drivers/ssb/driver_pcicore.c new file +22
-0+static ssize_t ssb_pci_attr_sprom_show (struct device *pcidev, +struct device_attribute *attr, +char *buf) +{ + struct pci_dev *pdev = container_of(pcidev, struct pci_dev, dev); +struct ssb_bus *bus; +u16 *sprom; +int err = -ENODEV ; +ssize_t count =0 ; + + bus =ssb_pci_dev_to_bus (pdev); +if (!bus) + goto out; + err = -ENOMEM ; + sprom =kcalloc (SSB_SPROMSIZE_WORDS ,sizeof (u16),GFP_KERNEL ); +if (!sprom) + goto out; + + err = -ERESTARTSYS ; +if (mutex_lock_interruptible (&bus->pci_sprom_mutex)) + goto out_kfree; +sprom_do_read (bus, sprom);
drivers/ssb/driver_pcicore.c
Quoted 10 lines
drivers/ssb/driver_pcicore.c new file +10
-0+mutex_unlock (&bus->pci_sprom_mutex); + + count =sprom2hex (sprom, buf,PAGE_SIZE ); + err =0 ; + +out_kfree: +kfree (sprom); +out: +return err ? err : count; +}
The mutex_lock_interruptible() looks fishy. Some commented explanation of what it's doing would be good here. It's quite unobvious to this reader. Cheesy deadlock avoidance? Hope not.
Quoted 16 lines
drivers/ssb/driver_pcicore.c new file +16
-0+ +/* These are the main device register access functions. +* do_select_core is inline to have the likely hotpath inline. +* All unlikely codepaths are out-of-line. */ +static inline int do_select_core (struct ssb_bus *bus, +struct ssb_device *dev, +u16 *offset) +{ +int err; +u8 need_seg = (*offset >=0x800 ) ?1 :0 ; + +if (unlikely (dev != bus->mapped_device)) { + err =ssb_pcmcia_switch_core (bus, dev); +if (unlikely (err)) +return err; + }
drivers/ssb/driver_pcicore.c
Quoted 10 lines
drivers/ssb/driver_pcicore.c new file +10
-0+if (unlikely (need_seg != bus->mapped_pcmcia_seg)) { + err =ssb_pcmcia_switch_segment (bus, need_seg); +if (unlikely (err)) +return err; + } +if (need_seg ==1 ) + *offset -=0x800 ; + +return 0 ; +}
Looks too large for inlining.
drivers/ssb/driver_pcicore.c
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+const struct ssb_bus_ops ssb_pcmcia_ops = { + .read16 = ssb_pcmcia_read16, + .read32 = ssb_pcmcia_read32, + .write16 = ssb_pcmcia_write16, + .write32 = ssb_pcmcia_write32, +};
static?
Quoted 10 lines · On Friday 27 July 2007 21:03:18 Andrew Morton wrote:
Quoted 6 lines · On Fri, 27 Jul 2007 18:57:20 +0200 Michael Buesch…
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.
Quoted 5 lines
hm, slim pickings for me here. Nice looking code.
checkpatch finds a few coding style glitches. Lots of 80-col violations which you might choose ignore (comments on #defines in headers are especially hard) but these guys:
I tried to fix most of the checkpatch warning, except these 80col warnings.
I don't think it's worth the work to fix the 80col stuff, yet. I develop kernel code in the real console, and I have no problems with this. Though, I use a wider console than 80col. Let's better fix the worst 80col violations over time and not now.
Quoted 5 lines
ERROR: "foo * bar" should be "foo *bar"
#4156: FILE:drivers/ssb/ssb_private.h:119:
+extern structssb_bus*ssb_pci_dev_to_bus(struct pci_dev *pdev);are worth addressing.
Well, I intentionally wrote that this way, as in my opinion it it easier to read. I only use this additional space for functions returning a pointer.
struct foo * function(int a, int b);
vs:
struct foo *function(int a, int b);
But I can change that, if that's really an issue and a style violation.
Quoted 6 lines
Quoted 6 lines
...
drivers/ssb/driver_pcicore.c new file +0
-0@@ -0,0 +1,564 @@
Quoted 6 lines
drivers/ssb/driver_pcicore.c line 51
Quoted 4 lines
...
drivers/ssb/driver_pcicore.c new file +2
-0+u32 pci_iobase =0x100 ; +u32 pci_membase =SSB_PCI_DMA ;These are quite poor names for global symbols.
Agreed. I will change this.
Quoted 9 lines
drivers/ssb/driver_pcicore.c line 277
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+static struct resource ssb_pcicore_mem_resource = { + .name ="SSB PCIcore external memory" , + .start =SSB_PCI_DMA , + .end = (u32 )SSB_PCI_DMA + (u32 )SSB_PCI_DMA_SZ -1 , + .flags =IORESOURCE_MEM , +};start and end here are
resource_size_t. Forcing them to u32 seems inappropriate.
You're right. I'll fix that.
Quoted 3 lines
Quoted 1 line
+void
ssb_pcicore_init(struct ssb_pcicore *pc)Please check that all newly-added global symbols do indeed need global scope.
ok.
Quoted 14 lines
Quoted 14 lines
drivers/ssb/driver_pcicore.c new file +14
-0+static void ssb_pcie_mdio_write (struct ssb_pcicore *pc,u8 device, +u8 address,u16 data) +{ +const u16 mdio_control =0x128 ; +const u16 mdio_data =0x12C ; +u32 v; +int i; + + v =0x80 ;/* Enable Preamble Sequence */ + v |=0x2 ;/* MDIO Clock Divisor */ +pcicore_write32 (pc, mdio_control, v); + + v = (1 <<30 );/* Start of Transaction */ + v |= (1 <<28 );/* Write Transaction */
Quoted 8 lines
drivers/ssb/driver_pcicore.c line 434
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+ v |= (1 <<17 );/* Turnaround */ + v |= (u32 )device <<22 ; + v |= (u32 )address <<18 ; + v |= data; +pcicore_write32 (pc, mdio_data, v); +udelay (10 );mystery udelays are usually worth a comment.
Sure.
Quoted 6 lines
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+static void ssb_buses_lock (void ) +{ +if (!ssb_is_early_boot) +mutex_lock (&buses_mutex); +} +
Quoted 8 lines
drivers/ssb/driver_pcicore.c
Quoted 5 lines
drivers/ssb/driver_pcicore.c new file +5
-0+static void ssb_buses_unlock (void ) +{ +if (!ssb_is_early_boot) +mutex_unlock (&buses_mutex); +}Some comments are neeeded here to explain why we're jumping through this hoop.
I added a comment at the declaration of ssb_is_early_boot, which tries to explain this in detail.
Quoted 2 lines
It's normally OK to take a mutex early in boot, so one wonders what's happening.
Hm, I tried that. There were problems. I don't remember this in detail, though. This is called _really_ early in boot.
Quoted 50 lines
Quoted 1 line
+EXPORT_SYMBOL(ssb_clockspeed);
Please check that all newly-added exports really need to be exported (I'm sure they do, but..)
Quoted 13 lines
drivers/ssb/driver_pcicore.c new file +13
-0+static int ssb_wait_bit (struct ssb_device *dev,u16 reg,u32 bitmask, +int timeout,int set) +{ +int i; +u32 val; + +for (i =0 ; i < timeout; i++) { + val =ssb_read32 (dev, reg); +if (set) { +if (val & bitmask) +return 0 ; + }else { +if (!(val & bitmask))drivers/ssb/driver_pcicore.c
Quoted 10 lines
drivers/ssb/driver_pcicore.c new file +10
-0+return 0 ; + } +udelay (10 ); + } + printk(KERN_ERR PFX "Timeout waiting for bitmask %08X on " +"register %04X to %s.\n" , + bitmask, reg, (set ? "set" : "clear")); + +return -ETIMEDOUT ; +}So `timeout' is in units of ten-microseconds. Unusual.
Quoted 19 lines
drivers/ssb/driver_pcicore.c new file +19
-0+void ssb_device_disable (struct ssb_device *dev,u32 core_specific_flags) +{ +u32 reject; + +if (ssb_read32 (dev,SSB_TMSLOW ) &SSB_TMSLOW_RESET ) +return ; + + reject =ssb_tmslow_reject_bitmask (dev); +ssb_write32 (dev,SSB_TMSLOW , reject |SSB_TMSLOW_CLOCK ); +ssb_wait_bit (dev,SSB_TMSLOW , reject,1000 ,1 ); +ssb_wait_bit (dev,SSB_TMSHIGH ,SSB_TMSHIGH_BUSY ,1000 ,0 ); +ssb_write32 (dev,SSB_TMSLOW , +SSB_TMSLOW_FGC |SSB_TMSLOW_CLOCK | + reject |SSB_TMSLOW_RESET | + core_specific_flags); +/* flush */ +ssb_read32 (dev,SSB_TMSLOW ); +udelay (1 ); +
Quoted 8 lines
drivers/ssb/driver_pcicore.c
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+ssb_write32 (dev,SSB_TMSLOW , + reject |SSB_TMSLOW_RESET | + core_specific_flags); +/* flush */ +ssb_read32 (dev,SSB_TMSLOW ); +udelay (1 );mystery udelay.
Well, /* flush */ :)
It simply waits and makes really sure the device is done. This is a really critical place and we must make 100% sure or we risk running into a machine check exception.
Quoted 5 lines
Quoted 5 lines
drivers/ssb/driver_pcicore.c new file +2
-0+} +EXPORT_SYMBOL (ssb_device_disable);
Quoted 8 lines
drivers/ssb/driver_pcicore.c
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+const struct ssb_bus_ops ssb_pci_ops = { + .read16 = ssb_pci_read16, + .read32 = ssb_pci_read32, + .write16 = ssb_pci_write16, + .write32 = ssb_pci_write32, +};static?
Yeah, think so. Good catch.
Quoted 28 lines
Quoted 28 lines
drivers/ssb/driver_pcicore.c new file +28
-0+static ssize_t ssb_pci_attr_sprom_show (struct device *pcidev, +struct device_attribute *attr, +char *buf) +{ + struct pci_dev *pdev = container_of(pcidev, struct pci_dev, dev); +struct ssb_bus *bus; +u16 *sprom; +int err = -ENODEV ; +ssize_t count =0 ; + + bus =ssb_pci_dev_to_bus (pdev); +if (!bus) + goto out; + err = -ENOMEM ; + sprom =kcalloc (SSB_SPROMSIZE_WORDS ,sizeof (u16),GFP_KERNEL ); +if (!sprom) + goto out; + + err = -ERESTARTSYS ; +if (mutex_lock_interruptible (&bus->pci_sprom_mutex)) + goto out_kfree; +sprom_do_read (bus, sprom); +mutex_unlock (&bus->pci_sprom_mutex); + + count =sprom2hex (sprom, buf,PAGE_SIZE ); + err =0 ; + +out_kfree:
Quoted 8 lines
drivers/ssb/driver_pcicore.c
Quoted 4 lines
drivers/ssb/driver_pcicore.c new file +4
-0+kfree (sprom); +out: +return err ? err : count; +}The
mutex_lock_interruptible()looks fishy. Some commented explanation of what it's doing would be good here. It's quite unobvious to this reader. Cheesy deadlock avoidance? Hope not.
No, it's simply to avoid writing the SPROM concurrently. SPROM writing is hairy and we must make sure here that we are the only one accessing the whole bus. We do that by suspending all devices and taking a lock to protect the SPROM from write concurrency.
Quoted 20 lines
Quoted 20 lines
drivers/ssb/driver_pcicore.c new file +20
-0+ +/* These are the main device register access functions. +* do_select_core is inline to have the likely hotpath inline. +* All unlikely codepaths are out-of-line. */ +static inline int do_select_core (struct ssb_bus *bus, +struct ssb_device *dev, +u16 *offset) +{ +int err; +u8 need_seg = (*offset >=0x800 ) ?1 :0 ; + +if (unlikely (dev != bus->mapped_device)) { + err =ssb_pcmcia_switch_core (bus, dev); +if (unlikely (err)) +return err; + } +if (unlikely (need_seg != bus->mapped_pcmcia_seg)) { + err =ssb_pcmcia_switch_segment (bus, need_seg); +if (unlikely (err)) +return err;
Quoted 8 lines
drivers/ssb/driver_pcicore.c
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+ } +if (need_seg ==1 ) + *offset -=0x800 ; + +return 0 ; +}Looks too large for inlining.
Well, as I already explained in a previous submission, this is the hottest path in the SSB code. It's called with every MMIO access. I intentionally did this inline to have the likely path inline and all the unlikely paths out of line. See the unlikely() branches, which are out of line.
Quoted 8 lines
drivers/ssb/driver_pcicore.c
Quoted 6 lines
drivers/ssb/driver_pcicore.c new file +6
-0+const struct ssb_bus_ops ssb_pcmcia_ops = { + .read16 = ssb_pcmcia_read16, + .read32 = ssb_pcmcia_read32, + .write16 = ssb_pcmcia_write16, + .write32 = ssb_pcmcia_write32, +};static?
Yep.
Signature · 1 line
Greetings Michael.
Quoted 10 lines · On Fri, 27 Jul 2007 21:30:48 +0200 Michael Buesch…
Quoted 5 lines
ERROR: "foo * bar" should be "foo *bar"
#4156: FILE:drivers/ssb/ssb_private.h:119:
+extern structssb_bus*ssb_pci_dev_to_bus(struct pci_dev *pdev);are worth addressing.
Well, I intentionally wrote that this way, as in my opinion it it easier to read. I only use this additional space for functions returning a pointer.
Quoted 8 lines
struct foo *
function(int a, int b);vs:
struct foo *function(int a, int b);
But I can change that, if that's really an issue and a style violation.
It's a microissue but yeah, no-space is more conventional.
Quoted 37 lines
Quoted 28 lines
Quoted 28 lines
drivers/ssb/pci.c new file +28
-0+static ssize_t ssb_pci_attr_sprom_show (struct device *pcidev, +struct device_attribute *attr, +char *buf) +{ + struct pci_dev *pdev = container_of(pcidev, struct pci_dev, dev); +struct ssb_bus *bus; +u16 *sprom; +int err = -ENODEV ; +ssize_t count =0 ; + + bus =ssb_pci_dev_to_bus (pdev); +if (!bus) + goto out; + err = -ENOMEM ; + sprom =kcalloc (SSB_SPROMSIZE_WORDS ,sizeof (u16),GFP_KERNEL ); +if (!sprom) + goto out; + + err = -ERESTARTSYS ; +if (mutex_lock_interruptible (&bus->pci_sprom_mutex)) + goto out_kfree; +sprom_do_read (bus, sprom); +mutex_unlock (&bus->pci_sprom_mutex); + + count =sprom2hex (sprom, buf,PAGE_SIZE ); + err =0 ; + +out_kfree:Quoted 8 lines
drivers/ssb/pci.c line 610
Quoted 4 lines
drivers/ssb/pci.c new file +4
-0+kfree (sprom); +out: +return err ? err : count; +}The
mutex_lock_interruptible()looks fishy. Some commented explanation of what it's doing would be good here. It's quite unobvious to this reader. Cheesy deadlock avoidance? Hope not.
Quoted 5 lines
No, it's simply to avoid writing the SPROM concurrently. SPROM writing is hairy and we must make sure here that we are the only one accessing the whole bus. We do that by suspending all devices and taking a lock to protect the SPROM from write concurrency.
Sure, but why is the locking interruptible rather than plain old mutex_lock()?
Quoted 37 lines · On Friday 27 July 2007 21:38:53 Andrew Morton wrote:
Quoted 37 lines
Quoted 36 lines
Quoted 22 lines
drivers/ssb/pci.c new file +22
-0+static ssize_t ssb_pci_attr_sprom_show (struct device *pcidev, +struct device_attribute *attr, +char *buf) +{ + struct pci_dev *pdev = container_of(pcidev, struct pci_dev, dev); +struct ssb_bus *bus; +u16 *sprom; +int err = -ENODEV ; +ssize_t count =0 ; + + bus =ssb_pci_dev_to_bus (pdev); +if (!bus) + goto out; + err = -ENOMEM ; + sprom =kcalloc (SSB_SPROMSIZE_WORDS ,sizeof (u16),GFP_KERNEL ); +if (!sprom) + goto out; + + err = -ERESTARTSYS ; +if (mutex_lock_interruptible (&bus->pci_sprom_mutex)) + goto out_kfree; +sprom_do_read (bus, sprom);drivers/ssb/pci.c line 610
Quoted 10 lines
drivers/ssb/pci.c new file +10
-0+mutex_unlock (&bus->pci_sprom_mutex); + + count =sprom2hex (sprom, buf,PAGE_SIZE ); + err =0 ; + +out_kfree: +kfree (sprom); +out: +return err ? err : count; +}The
mutex_lock_interruptible()looks fishy. Some commented explanation of what it's doing would be good here. It's quite unobvious to this reader. Cheesy deadlock avoidance? Hope not.
Quoted 8 lines
Quoted 5 lines
No, it's simply to avoid writing the SPROM concurrently. SPROM writing is hairy and we must make sure here that we are the only one accessing the whole bus. We do that by suspending all devices and taking a lock to protect the SPROM from write concurrency.
Sure, but why is the locking interruptible rather than plain old
mutex_lock()?
Hm, well. We hold this mutex for several seconds, as writing takes this long. So I simply thought it was worth allowing the waiter to interrupt here. If you say that's not an issue, I'll be happy to use mutex_lock() and reduce code complexity in this area.
Signature · 1 line
Greetings Michael.
Quoted 7 lines · On Fri, 27 Jul 2007 21:43:59 +0200 Michael Buesch…
Quoted 2 lines
Sure, but why is the locking interruptible rather than plain old
mutex_lock()?Hm, well. We hold this mutex for several seconds, as writing takes this long. So I simply thought it was worth allowing the waiter to interrupt here. If you say that's not an issue, I'll be happy to use
mutex_lock()and reduce code complexity in this area.
So.. is that what the _interruptible() is for? To allow an impatient user to ^c a read?
If so, that sounds reasonable. It's worth a comment explaining these decisions to future readers, because it is hard to work out this sort of thinking just from the bare C code.
Quoted 6 lines · On Friday 27 July 2007 22:12:49 Andrew Morton wrote:
Quoted 3 lines · On Fri, 27 Jul 2007 21:43:59 +0200 Michael Buesch…
Quoted 2 lines
Sure, but why is the locking interruptible rather than plain old
mutex_lock()?
Quoted 7 lines
Quoted 4 lines
Hm, well. We hold this mutex for several seconds, as writing takes this long. So I simply thought it was worth allowing the waiter to interrupt here. If you say that's not an issue, I'll be happy to use
mutex_lock()and reduce code complexity in this area.So.. is that what the
_interruptible()is for? To allow an impatient user to ^c a read?
Yeah, I thought so.
Quoted 3 lines
If so, that sounds reasonable. It's worth a comment explaining these decisions to future readers, because it is hard to work out this sort of thinking just from the bare C code.
Ok, no problem.
Signature · 1 line
Greetings Michael.
Quoted 11 lines · Andrew Morton
Quoted 7 lines · On Fri, 27 Jul 2007 21:43:59 +0200 Michael Buesch <[email protected]> wrote:
Quoted 2 lines
Sure, but why is the locking interruptible rather than plain old
mutex_lock()?Hm, well. We hold this mutex for several seconds, as writing takes this long. So I simply thought it was worth allowing the waiter to interrupt here. If you say that's not an issue, I'll be happy to use
mutex_lock()and reduce code complexity in this area.
Quoted 6 lines
So.. is that what the
_interruptible()is for? To allow an impatient user to ^c a read?If so, that sounds reasonable. It's worth a comment explaining these decisions to future readers, because it is hard to work out this sort of thinking just from the bare C code.
I think most of sysfs ->show() and ->store() implementations use _interruptible() variant to allow user to interrupt and return early.
Signature · 1 line
Dmitry
Quoted 7 lines · Michael Buesch
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.Signed-off-by Michael Buesch <[email protected]>
At first glance it looks like there might be some tab/space issues in some of the #define blocks toward the end of the patch, although those might be intentional.
Aside from whatever other style issues that might be identified, I'll state that this code has been carried in wireless-dev for months and thereby also spent a lot of time in -mm as well as Fedora (rawhide and F-7). The code has proven to be reasonably stable and reliable.
Signature · 2 lines
John W. Linville [email protected]
Quoted 7 lines · On Friday 27 July 2007 21:21:20 John W. Linville wrote:
Quoted 6 lines · Michael Buesch
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.
Quoted 5 lines
Quoted 1 line
Signed-off-by Michael Buesch <[email protected]>At first glance it looks like there might be some tab/space issues in some of the #define blocks toward the end of the patch, although those might be intentional.
They are intentional. It's something like this:
#define SSB_REGISTER_XX 0xF88
#define SSB_VALUE_FOR_REGISTER_XX 0x0001
#define SSB_MASK_FOR_REGISTER_XX 0xFF00
#define SSB_REGISTER_YY 0xF99
...
Quoted 4 lines
Aside from whatever other style issues that might be identified, I'll state that this code has been carried in wireless-dev for months and thereby also spent a lot of time in -mm as well as Fedora (rawhide and F-7). The code has proven to be reasonably stable and reliable.
And it's in the OpenWRT trunk since quite some time.
Signature · 1 line
Greetings Michael.
Quoted 21 lines · Michael Buesch
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.drivers/Kconfig +2
-0@@ -58,6 +58,8 @@ source "drivers/power/Kconfig" source "drivers/hwmon/Kconfig" +source "drivers/ssb/Kconfig" + source "drivers/mfd/Kconfig" source "drivers/media/Kconfig"
drivers/ssb/Kconfig line 5
Quoted 5 lines
drivers/ssb/Kconfig new file +5
-0@@ -0,0 +1,92 @@ +menu "Sonics Silicon Backplane" + +config SSB + tristate "Sonics Silicon Backplane support" + depends on EXPERIMENTAL
Hence this will show up on all platforms?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Quoted 19 lines · Geert Uytterhoeven
Quoted 17 lines · Michael Buesch
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.drivers/Kconfig +2
-0@@ -58,6 +58,8 @@ source "drivers/power/Kconfig" source "drivers/hwmon/Kconfig" +source "drivers/ssb/Kconfig" + source "drivers/mfd/Kconfig" source "drivers/media/Kconfig"
Quoted 10 lines
drivers/ssb/Kconfig line 5
Quoted 8 lines
drivers/ssb/Kconfig new file +5
-0@@ -0,0 +1,92 @@ +menu "Sonics Silicon Backplane" + +config SSB + tristate "Sonics Silicon Backplane support" + depends on EXPERIMENTALHence this will show up on all platforms?
So?
Quoted 31 lines · Michael Buesch
Quoted 19 lines · Geert Uytterhoeven
Quoted 17 lines · Michael Buesch
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.drivers/Kconfig +2
-0@@ -58,6 +58,8 @@ source "drivers/power/Kconfig" source "drivers/hwmon/Kconfig" +source "drivers/ssb/Kconfig" + source "drivers/mfd/Kconfig" source "drivers/media/Kconfig"Quoted 10 lines
drivers/ssb/Kconfig line 5
Quoted 8 lines
drivers/ssb/Kconfig new file +5
-0@@ -0,0 +1,92 @@ +menu "Sonics Silicon Backplane" + +config SSB + tristate "Sonics Silicon Backplane support" + depends on EXPERIMENTALHence this will show up on all platforms?
Quoted 1 line
So?
Shouldn't you add a dependency for platforms where it make sense to have SSB?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Quoted 32 lines · Geert Uytterhoeven
Quoted 31 lines · Michael Buesch
Quoted 29 lines · Geert Uytterhoeven
Quoted 21 lines · Michael Buesch
The Sonics Silicon Backplane is a mini-bus used on various Broadcom chips and embedded devices.
Devices using the SSB include b44, bcm43xx and various Broadcom based wireless routers.
A b44 and bcm43xx port and a SSB based OHCI driver is available.drivers/Kconfig +2
-0@@ -58,6 +58,8 @@ source "drivers/power/Kconfig" source "drivers/hwmon/Kconfig" +source "drivers/ssb/Kconfig" + source "drivers/mfd/Kconfig" source "drivers/media/Kconfig"drivers/ssb/Kconfig line 5
Quoted 5 lines
drivers/ssb/Kconfig new file +5
-0@@ -0,0 +1,92 @@ +menu "Sonics Silicon Backplane" + +config SSB + tristate "Sonics Silicon Backplane support" + depends on EXPERIMENTALHence this will show up on all platforms?
Quoted 3 lines
Quoted 1 line
So?
Shouldn't you add a dependency for platforms where it make sense to have SSB?
Well, that's everything where you can stick a PCI, PCMCIA, PC-Card or CF-Card into, plus the MIPS platform, where we have the embedded SSB. That's basically everything, no? Except these strange !HAS_IOMEM platforms, which we already take care of in a followup patch.