[patch 0/2] Merge the SSB subsystem

[patch 0/2] Merge the SSB subsystem

[mb]

This patch series merges the SSB subsystem and the SSB based USB OHCI HCD driver.
This is the first step to finally get bcm43xx-mac80211 merged, which is also
based on the SSB subsystem.

There's also a b44 port available, but it isn't ACKed by the b44 maintainers, yet.

--
Greetings Michael.

[patch 2/2] ssb: Add a driver for the Broadcom OHCI core

[mb]

This adds a driver for the Broadcom USB OHCI HCD device.
These devices are found in various embedded Broadcom-47xx machines.

Signed-off-by: Michael Buesch <mb@bu3sch.de>
Index: linux-2.6/drivers/usb/host/ohci-ssb.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/drivers/usb/host/ohci-ssb.c	2007-07-12 10:52:45.000000000 +0200
@@ -0,0 +1,254 @@
+/*
+ * Sonics Silicon Backplane
+ * Broadcom USB-core OHCI driver
+ *
+ * Copyright 2007 Michael Buesch <mb@bu3sch.de>
+ *
+ * Derived from the OHCI-PCI driver
+ * Copyright 1999 Roman Weissgaerber
+ * Copyright 2000-2002 David Brownell
+ * Copyright 1999 Linus Torvalds
+ * Copyright 1999 Gregory P. Smith
+ *
+ * Derived from the USBcore related parts of Broadcom-SB
+ * Copyright 2005 Broadcom Corporation
+ *
+ * Licensed under the GNU/GPL. See COPYING for details.
+ */
+
+#include <linux/ssb/ssb.h>
+
+
+#define SSB_OHCI_TMSLOW_HOSTMODE	(1 << 29)
+
+struct ssb_ohci_device {
+	struct ohci_hcd ohci; /* _must_ be at the beginning. */
+
+	u32 enable_flags;
+};
+
+
+static inline
+struct ssb_ohci_device * hcd_to_ssb_ohci(struct usb_hcd *hcd)
+{
+	return (struct ssb_ohci_device *)(hcd->hcd_priv);
+}
+
+
+static const struct ssb_device_id ssb_ohci_table[] = {
+	SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_USB11_HOSTDEV, SSB_ANY_REV),
+	SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_USB11_HOST, SSB_ANY_REV),
+	SSB_DEVTABLE_END
+};
+MODULE_DEVICE_TABLE(ssb, ssb_ohci_table);
+
+
+static int ssb_ohci_reset(struct usb_hcd *hcd)
+{
+	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
+	struct ohci_hcd *ohci = &ohcidev->ohci;
+	int err;
+
+	ohci_hcd_init(ohci);
+	err = ohci_init(ohci);
+
+	return err;
+}
+
+static int ssb_ohci_start(struct usb_hcd *hcd)
+{
+	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
+	struct ohci_hcd *ohci = &ohcidev->ohci;
+	int err;
+
+	err = ohci_run(ohci);
+	if (err < 0) {
+		ohci_err(ohci, "can't start\n");
+		ohci_stop(hcd);
+	}
+
+	return err;
+}
+
+#ifdef CONFIG_PM
+static int ssb_ohci_hcd_suspend(struct usb_hcd *hcd, pm_message_t message)
+{
+	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
+	struct ohci_hcd *ohci = &ohcidev->ohci;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ohci->lock, flags);
+
+	ohci_writel(ohci, OHCI_INTR_MIE, &ohci->regs->intrdisable);
+	ohci_readl(ohci, &ohci->regs->intrdisable); /* commit write */
+
+	/* make sure snapshot being resumed re-enumerates everything */
+	if (message.event == PM_EVENT_PRETHAW)
+		ohci_usb_reset(ohci);
+
+	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+
+	spin_unlock_irqrestore(&ohci->lock, flags);
+
+	return 0;
+}
+
+static int ssb_ohci_hcd_resume(struct usb_hcd *hcd)
+{
+	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+	usb_hcd_resume_root_hub(hcd);
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct hc_driver ssb_ohci_hc_driver = {
+	.description		= "ssb-usb-ohci",
+	.product_desc		= "SSB OHCI Controller",
+	.hcd_priv_size		= sizeof(struct ssb_ohci_device),
+
+	.irq			= ohci_irq,
+	.flags			= HCD_MEMORY | HCD_USB11,
+
+	.reset			= ssb_ohci_reset,
+	.start			= ssb_ohci_start,
+	.stop			= ohci_stop,
+	.shutdown		= ohci_shutdown,
+
+#ifdef CONFIG_PM
+	.suspend		= ssb_ohci_hcd_suspend,
+	.resume			= ssb_ohci_hcd_resume,
+#endif
+
+	.urb_enqueue		= ohci_urb_enqueue,
+	.urb_dequeue		= ohci_urb_dequeue,
+	.endpoint_disable	= ohci_endpoint_disable,
+
+	.get_frame_number	= ohci_get_frame,
+
+	.hub_status_data	= ohci_hub_status_data,
+	.hub_control		= ohci_hub_control,
+	.hub_irq_enable		= ohci_rhsc_enable,
+
+#ifdef CONFIG_PM
+	.bus_suspend		= ohci_bus_suspend,
+	.bus_resume		= ohci_bus_resume,
+#endif
+
+	.start_port_reset	= ohci_start_port_reset,
+};
+
+
+static void ssb_ohci_detach(struct ssb_device *dev)
+{
+	struct usb_hcd *hcd = ssb_get_drvdata(dev);
+
+	usb_remove_hcd(hcd);
+	iounmap(hcd->regs);
+	usb_put_hcd(hcd);
+	ssb_device_disable(dev, 0);
+}
+
+static int ssb_ohci_attach(struct ssb_device *dev)
+{
+	struct ssb_ohci_device *ohcidev;
+	struct usb_hcd *hcd;
+	int err = -ENOMEM;
+	u32 tmp, flags = 0;
+
+	if (dev->id.coreid == SSB_DEV_USB11_HOSTDEV)
+		flags |= SSB_OHCI_TMSLOW_HOSTMODE;
+
+	ssb_device_enable(dev, flags);
+
+	hcd = usb_create_hcd(&ssb_ohci_hc_driver, dev->dev,
+			     dev->dev->bus_id);
+	if (!hcd)
+		goto err_dev_disable;
+	ohcidev = hcd_to_ssb_ohci(hcd);
+	ohcidev->enable_flags = flags;
+
+	tmp = ssb_read32(dev, SSB_ADMATCH0);
+	hcd->rsrc_start = ssb_admatch_base(tmp);
+	hcd->rsrc_len = ssb_admatch_size(tmp);
+	hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
+	if (!hcd->regs)
+		goto err_put_hcd;
+	err = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
+	if (err)
+		goto err_iounmap;
+
+	ssb_set_drvdata(dev, hcd);
+
+	return err;
+
+err_iounmap:
+	iounmap(hcd->regs);
+err_put_hcd:
+	usb_put_hcd(hcd);
+err_dev_disable:
+	ssb_device_disable(dev, flags);
+	return err;
+}
+
+static int ssb_ohci_probe(struct ssb_device *dev,
+			  const struct ssb_device_id *id)
+{
+	int err;
+	u16 chipid_top;
+
+	chipid_top = (dev->bus->chip_id & 0xFF00);
+	if (chipid_top != 0x4700 &&
+	    chipid_top != 0x5300) {
+		/* USBcores are only connected on embedded devices. */
+		return -ENODEV;
+	}
+	/* TODO: Probably need more checks here whether the core is connected. */
+
+	if (usb_disabled())
+		return -ENODEV;
+
+	/* We currently always attach SSB_DEV_USB11_HOSTDEV
+	 * as HOST OHCI. If we want to attach it as Client device,
+	 * we must branch here and call into the (yet to
+	 * be written) Client mode driver. Same for remove(). */
+
+	err = ssb_ohci_attach(dev);
+
+	return err;
+}
+
+static void ssb_ohci_remove(struct ssb_device *dev)
+{
+	ssb_ohci_detach(dev);
+}
+
+#ifdef CONFIG_PM
+static int ssb_ohci_suspend(struct ssb_device *dev, pm_message_t state)
+{
+	ssb_device_disable(dev, 0);
+
+	return 0;
+}
+
+static int ssb_ohci_resume(struct ssb_device *dev)
+{
+	struct usb_hcd *hcd = ssb_get_drvdata(dev);
+	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
+
+	ssb_device_enable(dev, ohcidev->enable_flags);
+
+	return 0;
+}
+#else /* CONFIG_PM */
+# define ssb_ohci_suspend	NULL
+# define ssb_ohci_resume	NULL
+#endif /* CONFIG_PM */
+
+static struct ssb_driver ssb_ohci_driver = {
+	.name		= KBUILD_MODNAME,
+	.id_table	= ssb_ohci_table,
+	.probe		= ssb_ohci_probe,
+	.remove		= ssb_ohci_remove,
+	.suspend	= ssb_ohci_suspend,
+	.resume		= ssb_ohci_resume,
+};
Index: linux-2.6/drivers/usb/host/ohci-hcd.c
===================================================================
--- linux-2.6.orig/drivers/usb/host/ohci-hcd.c	2007-07-12 10:51:46.000000000 +0200
+++ linux-2.6/drivers/usb/host/ohci-hcd.c	2007-07-12 10:52:45.000000000 +0200
@@ -920,11 +920,17 @@ MODULE_LICENSE ("GPL");
 #define PS3_SYSTEM_BUS_DRIVER	ps3_ohci_sb_driver
 #endif
 
+#ifdef CONFIG_USB_OHCI_HCD_SSB
+#include "ohci-ssb.c"
+#define SSB_OHCI_DRIVER		ssb_ohci_driver
+#endif
+
 #if	!defined(PCI_DRIVER) &&		\
 	!defined(PLATFORM_DRIVER) &&	\
 	!defined(OF_PLATFORM_DRIVER) &&	\
 	!defined(SA1111_DRIVER) &&	\
-	!defined(PS3_SYSTEM_BUS_DRIVER)
+	!defined(PS3_SYSTEM_BUS_DRIVER) && \
+	!defined(SSB_OHCI_DRIVER)
 #error "missing bus glue for ohci-hcd"
 #endif
 
@@ -972,10 +978,20 @@ static int __init ohci_hcd_mod_init(void
 		goto error_pci;
 #endif
 
+#ifdef SSB_OHCI_DRIVER
+	retval = ssb_driver_register(&SSB_OHCI_DRIVER);
+	if (retval)
+		goto error_ssb;
+#endif
+
 	return retval;
 
 	/* Error path */
+#ifdef SSB_OHCI_DRIVER
+ error_ssb:
+#endif
 #ifdef PCI_DRIVER
+	pci_unregister_driver(&PCI_DRIVER);
  error_pci:
 #endif
 #ifdef SA1111_DRIVER
@@ -1001,6 +1017,9 @@ module_init(ohci_hcd_mod_init);
 
 static void __exit ohci_hcd_mod_exit(void)
 {
+#ifdef SSB_OHCI_DRIVER
+	ssb_driver_unregister(&SSB_OHCI_DRIVER);
+#endif
 #ifdef PCI_DRIVER
 	pci_unregister_driver(&PCI_DRIVER);
 #endif
Index: linux-2.6/drivers/usb/host/Kconfig
===================================================================
--- linux-2.6.orig/drivers/usb/host/Kconfig	2007-07-12 10:51:46.000000000 +0200
+++ linux-2.6/drivers/usb/host/Kconfig	2007-07-12 10:52:45.000000000 +0200
@@ -142,6 +142,19 @@ config USB_OHCI_HCD_PCI
 	  Enables support for PCI-bus plug-in USB controller cards.
 	  If unsure, say Y.
 
+config USB_OHCI_HCD_SSB
+	bool "OHCI support for the Broadcom SSB OHCI core (embedded systems only)"
+	depends on USB_OHCI_HCD && ((USB_OHCI_HCD=m && SSB) || (USB_OHCI_HCD=y && SSB=y)) && EXPERIMENTAL
+	default n
+	---help---
+	  Support for the Sonics Silicon Backplane (SSB) attached
+	  Broadcom USB OHCI core.
+
+	  This device is only present in some embedded devices with
+	  Broadcom based SSB bus.
+
+	  If unsure, say N.
+
 config USB_OHCI_BIG_ENDIAN_DESC
 	bool
 	depends on USB_OHCI_HCD

--
Greetings Michael.

Re: [patch 2/2] ssb: Add a driver for the Broadcom OHCI core

[Randy Dunlap]

On Thu, 12 Jul 2007 10:54:34 +0200 mb@bu3sch.de wrote:

> This adds a driver for the Broadcom USB OHCI HCD device.
> These devices are found in various embedded Broadcom-47xx machines.

I would expect USB host drivers to be submitted to the
linux-usb-devel@lists.sourceforge.net mailing list...


> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> Index: linux-2.6/drivers/usb/host/ohci-ssb.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/usb/host/ohci-ssb.c	2007-07-12 10:52:45.000000000 +0200
> @@ -0,0 +1,254 @@
> +/*
> + * Sonics Silicon Backplane
> + * Broadcom USB-core OHCI driver
> + *
> + * Copyright 2007 Michael Buesch <mb@bu3sch.de>
> + *
> + * Derived from the OHCI-PCI driver
> + * Copyright 1999 Roman Weissgaerber
> + * Copyright 2000-2002 David Brownell
> + * Copyright 1999 Linus Torvalds
> + * Copyright 1999 Gregory P. Smith
> + *
> + * Derived from the USBcore related parts of Broadcom-SB
> + * Copyright 2005 Broadcom Corporation
> + *
> + * Licensed under the GNU/GPL. See COPYING for details.
> + */
> +
> +#include <linux/ssb/ssb.h>
> +
> +
> +#define SSB_OHCI_TMSLOW_HOSTMODE	(1 << 29)
> +
> +struct ssb_ohci_device {
> +	struct ohci_hcd ohci; /* _must_ be at the beginning. */
> +
> +	u32 enable_flags;
> +};
> +
> +
> +static inline
> +struct ssb_ohci_device * hcd_to_ssb_ohci(struct usb_hcd *hcd)
> +{
> +	return (struct ssb_ohci_device *)(hcd->hcd_priv);
> +}
> +
> +
> +static const struct ssb_device_id ssb_ohci_table[] = {
> +	SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_USB11_HOSTDEV, SSB_ANY_REV),
> +	SSB_DEVICE(SSB_VENDOR_BROADCOM, SSB_DEV_USB11_HOST, SSB_ANY_REV),
> +	SSB_DEVTABLE_END
> +};
> +MODULE_DEVICE_TABLE(ssb, ssb_ohci_table);
> +
> +
> +static int ssb_ohci_reset(struct usb_hcd *hcd)
> +{
> +	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
> +	struct ohci_hcd *ohci = &ohcidev->ohci;
> +	int err;
> +
> +	ohci_hcd_init(ohci);
> +	err = ohci_init(ohci);
> +
> +	return err;
> +}
> +
> +static int ssb_ohci_start(struct usb_hcd *hcd)
> +{
> +	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
> +	struct ohci_hcd *ohci = &ohcidev->ohci;
> +	int err;
> +
> +	err = ohci_run(ohci);
> +	if (err < 0) {
> +		ohci_err(ohci, "can't start\n");
> +		ohci_stop(hcd);
> +	}
> +
> +	return err;
> +}
> +
> +#ifdef CONFIG_PM
> +static int ssb_ohci_hcd_suspend(struct usb_hcd *hcd, pm_message_t message)
> +{
> +	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
> +	struct ohci_hcd *ohci = &ohcidev->ohci;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ohci->lock, flags);
> +
> +	ohci_writel(ohci, OHCI_INTR_MIE, &ohci->regs->intrdisable);
> +	ohci_readl(ohci, &ohci->regs->intrdisable); /* commit write */
> +
> +	/* make sure snapshot being resumed re-enumerates everything */
> +	if (message.event == PM_EVENT_PRETHAW)
> +		ohci_usb_reset(ohci);
> +
> +	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +
> +	spin_unlock_irqrestore(&ohci->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int ssb_ohci_hcd_resume(struct usb_hcd *hcd)
> +{
> +	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +	usb_hcd_resume_root_hub(hcd);
> +	return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct hc_driver ssb_ohci_hc_driver = {
> +	.description		= "ssb-usb-ohci",
> +	.product_desc		= "SSB OHCI Controller",
> +	.hcd_priv_size		= sizeof(struct ssb_ohci_device),
> +
> +	.irq			= ohci_irq,
> +	.flags			= HCD_MEMORY | HCD_USB11,
> +
> +	.reset			= ssb_ohci_reset,
> +	.start			= ssb_ohci_start,
> +	.stop			= ohci_stop,
> +	.shutdown		= ohci_shutdown,
> +
> +#ifdef CONFIG_PM
> +	.suspend		= ssb_ohci_hcd_suspend,
> +	.resume			= ssb_ohci_hcd_resume,
> +#endif
> +
> +	.urb_enqueue		= ohci_urb_enqueue,
> +	.urb_dequeue		= ohci_urb_dequeue,
> +	.endpoint_disable	= ohci_endpoint_disable,
> +
> +	.get_frame_number	= ohci_get_frame,
> +
> +	.hub_status_data	= ohci_hub_status_data,
> +	.hub_control		= ohci_hub_control,
> +	.hub_irq_enable		= ohci_rhsc_enable,
> +
> +#ifdef CONFIG_PM
> +	.bus_suspend		= ohci_bus_suspend,
> +	.bus_resume		= ohci_bus_resume,
> +#endif
> +
> +	.start_port_reset	= ohci_start_port_reset,
> +};
> +
> +
> +static void ssb_ohci_detach(struct ssb_device *dev)
> +{
> +	struct usb_hcd *hcd = ssb_get_drvdata(dev);
> +
> +	usb_remove_hcd(hcd);
> +	iounmap(hcd->regs);
> +	usb_put_hcd(hcd);
> +	ssb_device_disable(dev, 0);
> +}
> +
> +static int ssb_ohci_attach(struct ssb_device *dev)
> +{
> +	struct ssb_ohci_device *ohcidev;
> +	struct usb_hcd *hcd;
> +	int err = -ENOMEM;
> +	u32 tmp, flags = 0;
> +
> +	if (dev->id.coreid == SSB_DEV_USB11_HOSTDEV)
> +		flags |= SSB_OHCI_TMSLOW_HOSTMODE;
> +
> +	ssb_device_enable(dev, flags);
> +
> +	hcd = usb_create_hcd(&ssb_ohci_hc_driver, dev->dev,
> +			     dev->dev->bus_id);
> +	if (!hcd)
> +		goto err_dev_disable;
> +	ohcidev = hcd_to_ssb_ohci(hcd);
> +	ohcidev->enable_flags = flags;
> +
> +	tmp = ssb_read32(dev, SSB_ADMATCH0);
> +	hcd->rsrc_start = ssb_admatch_base(tmp);
> +	hcd->rsrc_len = ssb_admatch_size(tmp);
> +	hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
> +	if (!hcd->regs)
> +		goto err_put_hcd;
> +	err = usb_add_hcd(hcd, dev->irq, IRQF_SHARED);
> +	if (err)
> +		goto err_iounmap;
> +
> +	ssb_set_drvdata(dev, hcd);
> +
> +	return err;
> +
> +err_iounmap:
> +	iounmap(hcd->regs);
> +err_put_hcd:
> +	usb_put_hcd(hcd);
> +err_dev_disable:
> +	ssb_device_disable(dev, flags);
> +	return err;
> +}
> +
> +static int ssb_ohci_probe(struct ssb_device *dev,
> +			  const struct ssb_device_id *id)
> +{
> +	int err;
> +	u16 chipid_top;
> +
> +	chipid_top = (dev->bus->chip_id & 0xFF00);
> +	if (chipid_top != 0x4700 &&
> +	    chipid_top != 0x5300) {
> +		/* USBcores are only connected on embedded devices. */
> +		return -ENODEV;
> +	}
> +	/* TODO: Probably need more checks here whether the core is connected. */
> +
> +	if (usb_disabled())
> +		return -ENODEV;
> +
> +	/* We currently always attach SSB_DEV_USB11_HOSTDEV
> +	 * as HOST OHCI. If we want to attach it as Client device,
> +	 * we must branch here and call into the (yet to
> +	 * be written) Client mode driver. Same for remove(). */
> +
> +	err = ssb_ohci_attach(dev);
> +
> +	return err;
> +}
> +
> +static void ssb_ohci_remove(struct ssb_device *dev)
> +{
> +	ssb_ohci_detach(dev);
> +}
> +
> +#ifdef CONFIG_PM
> +static int ssb_ohci_suspend(struct ssb_device *dev, pm_message_t state)
> +{
> +	ssb_device_disable(dev, 0);
> +
> +	return 0;
> +}
> +
> +static int ssb_ohci_resume(struct ssb_device *dev)
> +{
> +	struct usb_hcd *hcd = ssb_get_drvdata(dev);
> +	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
> +
> +	ssb_device_enable(dev, ohcidev->enable_flags);
> +
> +	return 0;
> +}
> +#else /* CONFIG_PM */
> +# define ssb_ohci_suspend	NULL
> +# define ssb_ohci_resume	NULL
> +#endif /* CONFIG_PM */
> +
> +static struct ssb_driver ssb_ohci_driver = {
> +	.name		= KBUILD_MODNAME,
> +	.id_table	= ssb_ohci_table,
> +	.probe		= ssb_ohci_probe,
> +	.remove		= ssb_ohci_remove,
> +	.suspend	= ssb_ohci_suspend,
> +	.resume		= ssb_ohci_resume,
> +};
> Index: linux-2.6/drivers/usb/host/ohci-hcd.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/host/ohci-hcd.c	2007-07-12 10:51:46.000000000 +0200
> +++ linux-2.6/drivers/usb/host/ohci-hcd.c	2007-07-12 10:52:45.000000000 +0200
> @@ -920,11 +920,17 @@ MODULE_LICENSE ("GPL");
>  #define PS3_SYSTEM_BUS_DRIVER	ps3_ohci_sb_driver
>  #endif
>  
> +#ifdef CONFIG_USB_OHCI_HCD_SSB
> +#include "ohci-ssb.c"
> +#define SSB_OHCI_DRIVER		ssb_ohci_driver
> +#endif
> +
>  #if	!defined(PCI_DRIVER) &&		\
>  	!defined(PLATFORM_DRIVER) &&	\
>  	!defined(OF_PLATFORM_DRIVER) &&	\
>  	!defined(SA1111_DRIVER) &&	\
> -	!defined(PS3_SYSTEM_BUS_DRIVER)
> +	!defined(PS3_SYSTEM_BUS_DRIVER) && \
> +	!defined(SSB_OHCI_DRIVER)
>  #error "missing bus glue for ohci-hcd"
>  #endif
>  
> @@ -972,10 +978,20 @@ static int __init ohci_hcd_mod_init(void
>  		goto error_pci;
>  #endif
>  
> +#ifdef SSB_OHCI_DRIVER
> +	retval = ssb_driver_register(&SSB_OHCI_DRIVER);
> +	if (retval)
> +		goto error_ssb;
> +#endif
> +
>  	return retval;
>  
>  	/* Error path */
> +#ifdef SSB_OHCI_DRIVER
> + error_ssb:
> +#endif
>  #ifdef PCI_DRIVER
> +	pci_unregister_driver(&PCI_DRIVER);
>   error_pci:
>  #endif
>  #ifdef SA1111_DRIVER
> @@ -1001,6 +1017,9 @@ module_init(ohci_hcd_mod_init);
>  
>  static void __exit ohci_hcd_mod_exit(void)
>  {
> +#ifdef SSB_OHCI_DRIVER
> +	ssb_driver_unregister(&SSB_OHCI_DRIVER);
> +#endif
>  #ifdef PCI_DRIVER
>  	pci_unregister_driver(&PCI_DRIVER);
>  #endif
> Index: linux-2.6/drivers/usb/host/Kconfig
> ===================================================================
> --- linux-2.6.orig/drivers/usb/host/Kconfig	2007-07-12 10:51:46.000000000 +0200
> +++ linux-2.6/drivers/usb/host/Kconfig	2007-07-12 10:52:45.000000000 +0200
> @@ -142,6 +142,19 @@ config USB_OHCI_HCD_PCI
>  	  Enables support for PCI-bus plug-in USB controller cards.
>  	  If unsure, say Y.
>  
> +config USB_OHCI_HCD_SSB
> +	bool "OHCI support for the Broadcom SSB OHCI core (embedded systems only)"
> +	depends on USB_OHCI_HCD && ((USB_OHCI_HCD=m && SSB) || (USB_OHCI_HCD=y && SSB=y)) && EXPERIMENTAL
> +	default n
> +	---help---
> +	  Support for the Sonics Silicon Backplane (SSB) attached
> +	  Broadcom USB OHCI core.
> +
> +	  This device is only present in some embedded devices with
> +	  Broadcom based SSB bus.
> +
> +	  If unsure, say N.
> +
>  config USB_OHCI_BIG_ENDIAN_DESC
>  	bool
>  	depends on USB_OHCI_HCD
> 
> --
> Greetings Michael.
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch 2/2] ssb: Add a driver for the Broadcom OHCI core

[Greg KH]

On Thu, Jul 12, 2007 at 09:04:59AM -0700, Randy Dunlap wrote:
> On Thu, 12 Jul 2007 10:54:34 +0200 mb@bu3sch.de wrote:
> 
> > This adds a driver for the Broadcom USB OHCI HCD device.
> > These devices are found in various embedded Broadcom-47xx machines.
> 
> I would expect USB host drivers to be submitted to the
> linux-usb-devel@lists.sourceforge.net mailing list...

Yes, Michael, please submit this patch there for discussion.

thanks,

greg k-h

Re: [patch 2/2] ssb: Add a driver for the Broadcom OHCI core

[Andrew Morton]

On Thu, 12 Jul 2007 10:54:34 +0200
mb@bu3sch.de wrote:

> This adds a driver for the Broadcom USB OHCI HCD device.
> These devices are found in various embedded Broadcom-47xx machines.
> 

The patch generates 2 checkpatch warnings.

> +#ifdef CONFIG_PM
> +static int ssb_ohci_hcd_suspend(struct usb_hcd *hcd, pm_message_t message)
> +{
> +	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
> +	struct ohci_hcd *ohci = &ohcidev->ohci;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ohci->lock, flags);
> +
> +	ohci_writel(ohci, OHCI_INTR_MIE, &ohci->regs->intrdisable);
> +	ohci_readl(ohci, &ohci->regs->intrdisable); /* commit write */
> +
> +	/* make sure snapshot being resumed re-enumerates everything */
> +	if (message.event == PM_EVENT_PRETHAW)
> +		ohci_usb_reset(ohci);
> +
> +	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +
> +	spin_unlock_irqrestore(&ohci->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int ssb_ohci_hcd_resume(struct usb_hcd *hcd)
> +{
> +	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +	usb_hcd_resume_root_hub(hcd);
> +	return 0;
> +}

Here, we'd normally do

#else
#define ssb_ohci_hcd_suspend NULL
#define ssb_ohci_hcd_resume NULL

> +#endif /* CONFIG_PM */
> +
> +static const struct hc_driver ssb_ohci_hc_driver = {
> +	.description		= "ssb-usb-ohci",
> +	.product_desc		= "SSB OHCI Controller",
> +	.hcd_priv_size		= sizeof(struct ssb_ohci_device),
> +
> +	.irq			= ohci_irq,
> +	.flags			= HCD_MEMORY | HCD_USB11,
> +
> +	.reset			= ssb_ohci_reset,
> +	.start			= ssb_ohci_start,
> +	.stop			= ohci_stop,
> +	.shutdown		= ohci_shutdown,
> +
> +#ifdef CONFIG_PM
> +	.suspend		= ssb_ohci_hcd_suspend,
> +	.resume			= ssb_ohci_hcd_resume,
> +#endif

and then remove these ifdefs.

> +	.urb_enqueue		= ohci_urb_enqueue,
> +	.urb_dequeue		= ohci_urb_dequeue,
> +	.endpoint_disable	= ohci_endpoint_disable,
> +
> +	.get_frame_number	= ohci_get_frame,
> +
> +	.hub_status_data	= ohci_hub_status_data,
> +	.hub_control		= ohci_hub_control,
> +	.hub_irq_enable		= ohci_rhsc_enable,
> +
> +#ifdef CONFIG_PM
> +	.bus_suspend		= ohci_bus_suspend,
> +	.bus_resume		= ohci_bus_resume,
> +#endif

Perhaps the same could be done here.

> +	.start_port_reset	= ohci_start_port_reset,
> +};
> +
>
> ...
>
> +
> +#ifdef CONFIG_PM
> +static int ssb_ohci_suspend(struct ssb_device *dev, pm_message_t state)
> +{
> +	ssb_device_disable(dev, 0);
> +
> +	return 0;
> +}
> +
> +static int ssb_ohci_resume(struct ssb_device *dev)
> +{
> +	struct usb_hcd *hcd = ssb_get_drvdata(dev);
> +	struct ssb_ohci_device *ohcidev = hcd_to_ssb_ohci(hcd);
> +
> +	ssb_device_enable(dev, ohcidev->enable_flags);
> +
> +	return 0;
> +}
> +#else /* CONFIG_PM */
> +# define ssb_ohci_suspend	NULL
> +# define ssb_ohci_resume	NULL
> +#endif /* CONFIG_PM */
> +
> +static struct ssb_driver ssb_ohci_driver = {
> +	.name		= KBUILD_MODNAME,
> +	.id_table	= ssb_ohci_table,
> +	.probe		= ssb_ohci_probe,
> +	.remove		= ssb_ohci_remove,
> +	.suspend	= ssb_ohci_suspend,
> +	.resume		= ssb_ohci_resume,
> +};

Like this.

> Index: linux-2.6/drivers/usb/host/ohci-hcd.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/host/ohci-hcd.c	2007-07-12 10:51:46.000000000 +0200
> +++ linux-2.6/drivers/usb/host/ohci-hcd.c	2007-07-12 10:52:45.000000000 +0200
> @@ -920,11 +920,17 @@ MODULE_LICENSE ("GPL");
>  #define PS3_SYSTEM_BUS_DRIVER	ps3_ohci_sb_driver
>  #endif
>  
> +#ifdef CONFIG_USB_OHCI_HCD_SSB
> +#include "ohci-ssb.c"
> +#define SSB_OHCI_DRIVER		ssb_ohci_driver
> +#endif

argh.  Why did USB do this?  Sigh.

>  #if	!defined(PCI_DRIVER) &&		\
>  	!defined(PLATFORM_DRIVER) &&	\
>  	!defined(OF_PLATFORM_DRIVER) &&	\
>  	!defined(SA1111_DRIVER) &&	\
> -	!defined(PS3_SYSTEM_BUS_DRIVER)
> +	!defined(PS3_SYSTEM_BUS_DRIVER) && \
> +	!defined(SSB_OHCI_DRIVER)
>  #error "missing bus glue for ohci-hcd"
>  #endif
>  
> @@ -972,10 +978,20 @@ static int __init ohci_hcd_mod_init(void
>  		goto error_pci;
>  #endif
>  
> +#ifdef SSB_OHCI_DRIVER
> +	retval = ssb_driver_register(&SSB_OHCI_DRIVER);
> +	if (retval)
> +		goto error_ssb;
> +#endif

Why do we use SSB_OHCI_DRIVER here rather than ssb_ohci_driver?   Seems odd.

> +
>  	return retval;
>  
>  	/* Error path */
> +#ifdef SSB_OHCI_DRIVER
> + error_ssb:
> +#endif
>  #ifdef PCI_DRIVER
> +	pci_unregister_driver(&PCI_DRIVER);

hm, are you sure about this?  I don't see anywhere in here where PCI_DRIVER
got newly registered, but we are newly unregistering it?

Re: [patch 2/2] ssb: Add a driver for the Broadcom OHCI core

[Michael Buesch]

On Thursday 12 July 2007 19:49, Andrew Morton wrote:
> Here, we'd normally do
> 
> #else
> #define ssb_ohci_hcd_suspend NULL
> #define ssb_ohci_hcd_resume NULL

Ah, well. I don't care either way, so if that is the preferred way to deal
with it, I'll change :)

> > Index: linux-2.6/drivers/usb/host/ohci-hcd.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/usb/host/ohci-hcd.c	2007-07-12 10:51:46.000000000 +0200
> > +++ linux-2.6/drivers/usb/host/ohci-hcd.c	2007-07-12 10:52:45.000000000 +0200
> > @@ -920,11 +920,17 @@ MODULE_LICENSE ("GPL");
> >  #define PS3_SYSTEM_BUS_DRIVER	ps3_ohci_sb_driver
> >  #endif
> >  
> > +#ifdef CONFIG_USB_OHCI_HCD_SSB
> > +#include "ohci-ssb.c"
> > +#define SSB_OHCI_DRIVER		ssb_ohci_driver
> > +#endif
> 
> argh.  Why did USB do this?  Sigh.
> 
> >  #if	!defined(PCI_DRIVER) &&		\
> >  	!defined(PLATFORM_DRIVER) &&	\
> >  	!defined(OF_PLATFORM_DRIVER) &&	\
> >  	!defined(SA1111_DRIVER) &&	\
> > -	!defined(PS3_SYSTEM_BUS_DRIVER)
> > +	!defined(PS3_SYSTEM_BUS_DRIVER) && \
> > +	!defined(SSB_OHCI_DRIVER)
> >  #error "missing bus glue for ohci-hcd"
> >  #endif
> >  
> > @@ -972,10 +978,20 @@ static int __init ohci_hcd_mod_init(void
> >  		goto error_pci;
> >  #endif
> >  
> > +#ifdef SSB_OHCI_DRIVER
> > +	retval = ssb_driver_register(&SSB_OHCI_DRIVER);
> > +	if (retval)
> > +		goto error_ssb;
> > +#endif
> 
> Why do we use SSB_OHCI_DRIVER here rather than ssb_ohci_driver?   Seems odd.

Yes, that ohci code is horrible. But all other ohci drivers do it this way, too,
so I did it, too.

> > +
> >  	return retval;
> >  
> >  	/* Error path */
> > +#ifdef SSB_OHCI_DRIVER
> > + error_ssb:
> > +#endif
> >  #ifdef PCI_DRIVER
> > +	pci_unregister_driver(&PCI_DRIVER);
> 
> hm, are you sure about this?  I don't see anywhere in here where PCI_DRIVER
> got newly registered, but we are newly unregistering it?

Whoops, good catch. That's a merge error.

Re: [patch 1/2] Merge the Sonics Silicon Backplane subsystem

[Andrew Morton]

On Thu, 12 Jul 2007 10:54:33 +0200
mb@bu3sch.de wrote:

> This merges the SSB subsystem. SSB is a backplane bus subsystem needed
> for various Broadcom devices.

The patch generates 216 checkpatch warnings

> +	depends on EXPERIMENTAL
> +	help
> +	  Support for the Sonics Silicon Backplane bus
> +
> +	  The module will be called ssb
> +
> +	  If unsure, say M
> +
> +config SSB_PCIHOST
> +	bool "Support for SSB on PCI-bus host"
> +	depends on SSB && PCI
> +	default y
> +	help
> +	  Support for a Sonics Silicon Backplane on top
> +	  of a PCI device.
> +
> +	  If unsure, say Y
> +
> +config SSB_PCMCIAHOST
> +	bool "Support for SSB on PCMCIA-bus host"
> +	depends on SSB && PCMCIA
> +	help
> +	  Support for a Sonics Silicon Backplane on top
> +	  of a PCMCIA device.
> +
> +	  If unsure, say N
> +
> +config SSB_SILENT
> +	bool "No SSB kernel messages"
> +	depends on SSB
> +	help
> +	  This option turns off all Sonics Silicon Backplane printks.
> +	  Note that you won't be able to identify problems, once
> +	  messages are turned off.
> +	  This might only be desired for production kernels on
> +	  embedded devices to reduce the kernel size.

The fact that this is called "silent" rather than noisy makes me suspect
that you'd prefer that this option usually be enabled.  If so, perhaps a
`default y' is needed?

> +	  Say N
> +
> +config SSB_DEBUG
> +	bool "SSB debugging"
> +	depends on SSB && !SSB_SILENT
> +	help
> +	  This turns on additional runtime checks and debugging
> +	  messages. Turn this on for SSB troubleshooting.
> +
> +	  If unsure, say N

'default n', perhaps.

> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/ssb/driver_chipcommon.c	2007-07-12 10:51:49.000000000 +0200
>
> ..
>
> +void ssb_chipco_set_clockmode(struct ssb_chipcommon *cc,
> +			      enum ssb_clkmode mode)
> +{
> +	struct ssb_device *ccdev = cc->dev;
> +	struct ssb_bus *bus;
> +	u32 tmp;

eek, a tmp.

> +	if (!ccdev)
> +		return;
> +	bus = ccdev->bus;
> +	/* chipcommon cores prior to rev6 don't support dynamic clock control */
> +	if (ccdev->id.revision < 6)
> +		return;
> +	/* chipcommon cores rev10 are a whole new ball game */
> +	if (ccdev->id.revision >= 10)
> +		return;
> +	if (!(cc->capabilities & SSB_CHIPCO_CAP_PCTL))
> +		return;
> +
> +	switch (mode) {
> +	case SSB_CLKMODE_SLOW:
> +		tmp = chipco_read32(cc, SSB_CHIPCO_SLOWCLKCTL);
> +		tmp |= SSB_CHIPCO_SLOWCLKCTL_FSLOW;
> +		chipco_write32(cc, SSB_CHIPCO_SLOWCLKCTL, tmp);

hm, ok, maybe the much-derided `tmp' is applicable here.

> +		break;
> +	case SSB_CLKMODE_FAST:
> +		ssb_pci_xtal(bus, SSB_GPIO_XTAL, 1); /* Force crystal on */
> +		tmp = chipco_read32(cc, SSB_CHIPCO_SLOWCLKCTL);
> +		tmp &= ~SSB_CHIPCO_SLOWCLKCTL_FSLOW;
> +		tmp |= SSB_CHIPCO_SLOWCLKCTL_IPLL;
> +		chipco_write32(cc, SSB_CHIPCO_SLOWCLKCTL, tmp);
> +		break;
> +	case SSB_CLKMODE_DYNAMIC:
> +		tmp = chipco_read32(cc, SSB_CHIPCO_SLOWCLKCTL);
> +		tmp &= ~SSB_CHIPCO_SLOWCLKCTL_FSLOW;
> +		tmp &= ~SSB_CHIPCO_SLOWCLKCTL_IPLL;
> +		tmp &= ~SSB_CHIPCO_SLOWCLKCTL_ENXTAL;
> +		if ((tmp & SSB_CHIPCO_SLOWCLKCTL_SRC) != SSB_CHIPCO_SLOWCLKCTL_SRC_XTAL)
> +			tmp |= SSB_CHIPCO_SLOWCLKCTL_ENXTAL;
> +		chipco_write32(cc, SSB_CHIPCO_SLOWCLKCTL, tmp);
> +
> +		/* for dynamic control, we have to release our xtal_pu "force on" */
> +		if (tmp & SSB_CHIPCO_SLOWCLKCTL_ENXTAL)
> +			ssb_pci_xtal(bus, SSB_GPIO_XTAL, 0);
> +		break;
> +	default:
> +		assert(0);

whaaa?  Kernel code would use BUG(), please.

> +	}
> +}
> +
> +/* Get the Slow Clock Source */
> +static int chipco_pctl_get_slowclksrc(struct ssb_chipcommon *cc)
> +{
> +	struct ssb_bus *bus = cc->dev->bus;
> +	u32 tmp = 0;

This initialisation is here only to kill a bogus gcc warning.  It should at
least have been commented to indicate this sad fact.  Better would be to
use uninitialized_var() which has the seme effect and is self-documenting,
may be centrally disabled or altered, may be grepped for, etc.


> +	if (cc->dev->id.revision < 6) {
> +		if (bus->bustype == SSB_BUSTYPE_SSB ||
> +		    bus->bustype == SSB_BUSTYPE_PCMCIA)
> +			return SSB_CHIPCO_CLKSRC_XTALOS;
> +		if (bus->bustype == SSB_BUSTYPE_PCI) {
> +			pci_read_config_dword(bus->host_pci, SSB_GPIO_OUT, &tmp);
> +			if (tmp & 0x10)
> +				return SSB_CHIPCO_CLKSRC_PCI;
> +			return SSB_CHIPCO_CLKSRC_XTALOS;
> +		}
> +	}
> +	if (cc->dev->id.revision < 10) {
> +		tmp = chipco_read32(cc, SSB_CHIPCO_SLOWCLKCTL);
> +		tmp &= 0x7;
> +		if (tmp == 0)
> +			return SSB_CHIPCO_CLKSRC_LOPWROS;
> +		if (tmp == 1)
> +			return SSB_CHIPCO_CLKSRC_XTALOS;
> +		if (tmp == 2)
> +			return SSB_CHIPCO_CLKSRC_PCI;
> +	}
> +
> +	return SSB_CHIPCO_CLKSRC_XTALOS;
> +}
>
> ...
>
> +		assert(0);

etc.

> +		limit = 0;
> +	}
> +	limit /= divisor;
> +
> +	return limit;
> +}
>
> ...
>
> +static void calc_fast_powerup_delay(struct ssb_chipcommon *cc)
> +{
> +	struct ssb_bus *bus = cc->dev->bus;
> +	int minfreq;
> +	unsigned int tmp;
> +	u32 pll_on_delay;
> +
> +	if (bus->bustype != SSB_BUSTYPE_PCI)
> +		return;
> +	if (!(cc->capabilities & SSB_CHIPCO_CAP_PCTL))
> +		return;
> +
> +	minfreq = chipco_pctl_clockfreqlimit(cc, 0);
> +	pll_on_delay = chipco_read32(cc, SSB_CHIPCO_PLLONDELAY);
> +	tmp = (((pll_on_delay + 2) * 1000000) + (minfreq - 1)) / minfreq;
> +	assert((tmp & ~0xFFFF) == 0);
> +
> +	cc->fast_pwrup_delay = tmp;
> +}
> +
> +void ssb_chipcommon_init(struct ssb_chipcommon *cc)
> +{
> +	if (!cc->dev)
> +		return; /* We don't have a ChipCommon */
> +	chipco_powercontrol_init(cc);
> +	ssb_chipco_set_clockmode(cc, SSB_CLKMODE_FAST);
> +	calc_fast_powerup_delay(cc);
> +}
> +
> +void ssb_chipco_suspend(struct ssb_chipcommon *cc, pm_message_t state)
> +{
> +	if (!cc->dev)
> +		return;
> +	ssb_chipco_set_clockmode(cc, SSB_CLKMODE_SLOW);
> +}

Should this stuff be inside #ifdef CONFIG_PM?

> +void ssb_chipco_resume(struct ssb_chipcommon *cc)
> +{
> +	if (!cc->dev)
> +		return;
> +	chipco_powercontrol_init(cc);
> +	ssb_chipco_set_clockmode(cc, SSB_CLKMODE_FAST);
> +}
> +
> 
> ...
>
> +#endif /* CONFIG_SSB_SERIAL */
> Index: linux-2.6/drivers/ssb/driver_mipscore.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/ssb/driver_mipscore.c	2007-07-12 10:51:49.000000000 +0200
> @@ -0,0 +1,258 @@
> +/*
> + * Sonics Silicon Backplane
> + * Broadcom MIPS core driver
> + *
> + * Copyright 2005, Broadcom Corporation
> + * Copyright 2006, 2007, Michael Buesch <mb@bu3sch.de>
> + *
> + * Licensed under the GNU/GPL. See COPYING for details.
> + */
> +
> +#include <linux/ssb/ssb.h>
> +
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial_reg.h>
> +#include <asm/time.h>
> +
> +#include "ssb_private.h"
> +
> +
> +static inline u32 mips_read32(struct ssb_mipscore *mcore,
> +			      u16 offset)
> +{
> +	return ssb_read32(mcore->dev, offset);
> +}

hm, this doesn't seem like a well-chosen name for a driver-private function.

I guess it's OK given that it's in a .c file.

> ...
>
> +static void ssb_pcicore_init_hostmode(struct ssb_pcicore *pc)
> +{
> +	u32 val;
> +
> +	if (extpci_core) {
> +		WARN_ON(1);
> +		return;
> +	}

fyi, one could do

	if (WARN_ON(extpci_core))
		return;

here.

> +	extpci_core = pc;
> +
> +	ssb_dprintk(KERN_INFO PFX "PCIcore in host mode found\n");
> +	/* Reset devices on the external PCI bus */
> +	val = SSB_PCICORE_CTL_RST_OE;
> +	val |= SSB_PCICORE_CTL_CLK_OE;
> +	pcicore_write32(pc, SSB_PCICORE_CTL, val);
> +	val |= SSB_PCICORE_CTL_CLK; /* Clock on */
> +	pcicore_write32(pc, SSB_PCICORE_CTL, val);
> +	udelay(150);
> +	val |= SSB_PCICORE_CTL_RST; /* Deassert RST# */
> +	pcicore_write32(pc, SSB_PCICORE_CTL, val);
> +	udelay(1);
> +
> +	//TODO cardbus mode
> +
> +	/* 64MB I/O window */
> +	pcicore_write32(pc, SSB_PCICORE_SBTOPCI0,
> +			SSB_PCICORE_SBTOPCI_IO);
> +	/* 64MB config space */
> +	pcicore_write32(pc, SSB_PCICORE_SBTOPCI1,
> +			SSB_PCICORE_SBTOPCI_CFG0);
> +	/* 1GB memory window */
> +	pcicore_write32(pc, SSB_PCICORE_SBTOPCI2,
> +			SSB_PCICORE_SBTOPCI_MEM | SSB_PCI_DMA);
> +
> +	/* Enable PCI bridge BAR0 prefetch and burst */
> +	val = PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY;
> +	ssb_extpci_write_config(pc, 0, 0, 0, PCI_COMMAND, &val, 2);
> +	/* Clear error conditions */
> +	val = 0;
> +	ssb_extpci_write_config(pc, 0, 0, 0, PCI_STATUS, &val, 2);
> +
> +	/* Enable PCI interrupts */
> +	pcicore_write32(pc, SSB_PCICORE_IMASK,
> +			SSB_PCICORE_IMASK_INTA);
> +
> +	/* Ok, ready to run, register it to the system.
> +	 * The following needs change, if we want to port hostmode
> +	 * to non-MIPS platform. */
> +	set_io_port_base((unsigned long)ioremap_nocache(SSB_PCI_MEM, 0x04000000));
> +	register_pci_controller(&ssb_pcicore_controller);
> +}
> +
> +static int pcicore_is_in_hostmode(struct ssb_pcicore *pc)
> +{
> +	struct ssb_bus *bus = pc->dev->bus;
> +	u16 chipid_top;
> +	u32 tmp;
> +
> +	chipid_top = (bus->chip_id & 0xFF00);
> +	if (chipid_top != 0x4700 &&
> +	    chipid_top != 0x5300)
> +		return 0;
> +
> +	if (bus->sprom.r1.boardflags_lo & SSB_PCICORE_BFL_NOPCI)
> +		return 0;
> +
> +	/* The 200-pin BCM4712 package does not bond out PCI. Even when
> +	 * PCI is bonded out, some boards may leave the pins floating. */
> +	if (bus->chip_id == 0x4712) {
> +		if (bus->chip_package == SSB_CHIPPACK_BCM4712S)
> +			return 0;
> +		if (bus->chip_package == SSB_CHIPPACK_BCM4712M)
> +			return 0;
> +	}
> +	if (bus->chip_id == 0x5350)
> +		return 0;
> +
> +	return !mips_busprobe(tmp, (u32 *) (bus->mmio + (pc->dev->core_index * SSB_CORE_SIZE)));
> +}

What's the situation with 64-bit stuff here?

> +	assert(dev);

BUG_ON, please.

> +int ssb_pcicore_dev_irqvecs_enable(struct ssb_pcicore *pc,
> +				   struct ssb_device *dev)
> +{
> +	struct ssb_device *pdev = pc->dev;
> +	struct ssb_bus *bus;
> +	int err = 0;
> +	u32 tmp;
> +
> +	might_sleep();

If you say so.  It doesn't _look_ like it sleeps?

> +	if (!pdev)
> +		goto out;
> +	bus = pdev->bus;
> +
> +	/* Enable interrupts for this device. */
> +	if (bus->host_pci &&
> +	    ((pdev->id.revision >= 6) || (pdev->id.coreid == SSB_DEV_PCIE))) {
> +		u32 coremask;
> +
> +		/* Calculate the "coremask" for the device. */
> +		coremask = (1 << dev->core_index);
> +
> +		err = pci_read_config_dword(bus->host_pci, SSB_PCI_IRQMASK, &tmp);
> +		if (err)
> +			goto out;
> +		tmp |= coremask << 8;
> +		err = pci_write_config_dword(bus->host_pci, SSB_PCI_IRQMASK, tmp);
> +		if (err)
> +			goto out;
> +	} else {
> +		u32 intvec;
> +
> +		intvec = ssb_read32(pdev, SSB_INTVEC);
> +		tmp = ssb_read32(dev, SSB_TPSFLAG);
> +		tmp &= SSB_TPSFLAG_BPFLAG;
> +		intvec |= tmp;
> +		ssb_write32(pdev, SSB_INTVEC, intvec);
> +	}
> +
> +	/* Setup PCIcore operation. */
> +	if (pc->setup_done)
> +		goto out;
> +	if (pdev->id.coreid == SSB_DEV_PCI) {
> +		tmp = pcicore_read32(pc, SSB_PCICORE_SBTOPCI2);
> +		tmp |= SSB_PCICORE_SBTOPCI_PREF;
> +		tmp |= SSB_PCICORE_SBTOPCI_BURST;
> +		pcicore_write32(pc, SSB_PCICORE_SBTOPCI2, tmp);
> +
> +		if (pdev->id.revision < 5) {
> +			tmp = ssb_read32(pdev, SSB_IMCFGLO);
> +			tmp &= ~SSB_IMCFGLO_SERTO;
> +			tmp |= 2;
> +			tmp &= ~SSB_IMCFGLO_REQTO;
> +			tmp |= 3 << SSB_IMCFGLO_REQTO_SHIFT;
> +			ssb_write32(pdev, SSB_IMCFGLO, tmp);
> +			ssb_commit_settings(bus);
> +		} else if (pdev->id.revision >= 11) {
> +			tmp = pcicore_read32(pc, SSB_PCICORE_SBTOPCI2);
> +			tmp |= SSB_PCICORE_SBTOPCI_MRM;
> +			pcicore_write32(pc, SSB_PCICORE_SBTOPCI2, tmp);
> +		}
> +	} else {
> +		assert(pdev->id.coreid == SSB_DEV_PCIE);
> +		//TODO: Better make defines for all these magic PCIE values.
> +		if ((pdev->id.revision == 0) || (pdev->id.revision == 1)) {
> +			/* TLP Workaround register. */
> +			tmp = ssb_pcie_read(pc, 0x4);
> +			tmp |= 0x8;
> +			ssb_pcie_write(pc, 0x4, tmp);
> +		}
> +		if (pdev->id.revision == 0) {
> +			const u8 serdes_rx_device = 0x1F;
> +
> +			ssb_pcie_mdio_write(pc, serdes_rx_device,
> +					    2 /* Timer */, 0x8128);
> +			ssb_pcie_mdio_write(pc, serdes_rx_device,
> +					    6 /* CDR */, 0x0100);
> +			ssb_pcie_mdio_write(pc, serdes_rx_device,
> +					    7 /* CDR BW */, 0x1466);
> +		} else if (pdev->id.revision == 1) {
> +			/* DLLP Link Control register. */
> +			tmp = ssb_pcie_read(pc, 0x100);
> +			tmp |= 0x40;
> +			ssb_pcie_write(pc, 0x100, tmp);
> +		}
> +	}
> +	pc->setup_done = 1;
> +out:
> +	return err;
> +}
>
> ...
>
> +
> +#include "ssb_private.h"
> +
> +#include <linux/delay.h>
> +#include <linux/ssb/ssb.h>
> +#include <linux/ssb/ssb_regs.h>
> +
> +#ifdef CONFIG_SSB_PCIHOST
> +# include <linux/pci.h>
> +#endif

This ifdef guard is probably a bad idea.  pci.h should be OK with or
without CONFIG_SSB_PCIHOST and making the inclusion conditional serves to
add more combinations which can cause compile failures.

> +#ifdef CONFIG_SSB_PCMCIAHOST
> +# include <pcmcia/cs_types.h>
> +# include <pcmcia/cs.h>
> +# include <pcmcia/cistpl.h>
> +# include <pcmcia/ds.h>
> +#endif

Maybe here too.

> +static int ssb_device_resume(struct device *dev)
> +{
> +	struct ssb_device *ssb_dev = dev_to_ssb_dev(dev);
> +	struct ssb_driver *ssb_drv;
> +	struct ssb_bus *bus;
> +	int err = 0;
> +
> +	bus = ssb_dev->bus;
> +	if (bus->suspend_cnt == bus->nr_devices) {
> +		err = ssb_bus_resume(bus);
> +		if (err)
> +			return err;
> +	}
> +	bus->suspend_cnt--;
> +	if (dev->driver) {
> +		ssb_drv = drv_to_ssb_drv(dev->driver);
> +		if (ssb_drv && ssb_drv->resume)
> +			err = ssb_drv->resume(ssb_dev);
> +		if (err)
> +			goto out;
> +	}
> +out:
> +	return err;
> +}
> +
> +static void ssb_bus_suspend(struct ssb_bus *bus, pm_message_t state)
> +{
> +	ssb_chipco_suspend(&bus->chipco, state);
> +	ssb_pci_xtal(bus, SSB_GPIO_XTAL | SSB_GPIO_PLL, 0);
> +
> +	/* Reset HW state information in memory, so that HW is
> +	 * completely reinitialized on resume. */
> +	bus->mapped_device = NULL;
> +#ifdef CONFIG_SSB_DRIVER_PCICORE
> +	bus->pcicore.setup_done = 0;
> +#endif
> +}

No #ifdef CONFIG_PMs here?

>
> ...
>
> +#define is_early_boot()		(ssb_bustype.name == NULL)
> +
> +static void ssb_buses_lock(void)
> +{
> +	if (!is_early_boot())
> +		mutex_lock(&buses_mutex);
> +}
> +
> +static void ssb_buses_unlock(void)
> +{
> +	if (!is_early_boot())
> +		mutex_unlock(&buses_mutex);
> +}

The is_early_boot() stuff looks hacky.  It surely needs a comment to
explain to the reader what is going on.  Because it is *very* hard to work
out what this is here for otherwise.

> +static int ssb_fetch_invariants(struct ssb_bus *bus,
> +				int (*get_invariants)(struct ssb_bus *bus,
> +						      struct ssb_init_invariants *iv))

fyi, this declaration-of-a-pointer-to-a-function thing is the one place
where kernel code _will_ define a typedef merely to save typing.

> +{
> +	struct ssb_init_invariants iv;
> +	int err;
> +
> +	memset(&iv, 0, sizeof(iv));
> +	err = get_invariants(bus, &iv);
> +	if (err)
> +		goto out;
> +	memcpy(&bus->boardinfo, &iv.boardinfo, sizeof(iv.boardinfo));
> +	memcpy(&bus->sprom, &iv.sprom, sizeof(iv.sprom));
> +out:
> +	return err;
> +}
> +
>
> ...
>
> +
> +static inline u8 ssb_crc8(u8 crc, u8 data)
> +{
> +	/* Polynomial:   x^8 + x^7 + x^6 + x^4 + x^2 + 1   */
> +	static const u8 t[] = {
> +		0x00, 0xF7, 0xB9, 0x4E, 0x25, 0xD2, 0x9C, 0x6B,
> +		0x4A, 0xBD, 0xF3, 0x04, 0x6F, 0x98, 0xD6, 0x21,
> +		0x94, 0x63, 0x2D, 0xDA, 0xB1, 0x46, 0x08, 0xFF,
> +		0xDE, 0x29, 0x67, 0x90, 0xFB, 0x0C, 0x42, 0xB5,
> +		0x7F, 0x88, 0xC6, 0x31, 0x5A, 0xAD, 0xE3, 0x14,
> +		0x35, 0xC2, 0x8C, 0x7B, 0x10, 0xE7, 0xA9, 0x5E,
> +		0xEB, 0x1C, 0x52, 0xA5, 0xCE, 0x39, 0x77, 0x80,
> +		0xA1, 0x56, 0x18, 0xEF, 0x84, 0x73, 0x3D, 0xCA,
> +		0xFE, 0x09, 0x47, 0xB0, 0xDB, 0x2C, 0x62, 0x95,
> +		0xB4, 0x43, 0x0D, 0xFA, 0x91, 0x66, 0x28, 0xDF,
> +		0x6A, 0x9D, 0xD3, 0x24, 0x4F, 0xB8, 0xF6, 0x01,
> +		0x20, 0xD7, 0x99, 0x6E, 0x05, 0xF2, 0xBC, 0x4B,
> +		0x81, 0x76, 0x38, 0xCF, 0xA4, 0x53, 0x1D, 0xEA,
> +		0xCB, 0x3C, 0x72, 0x85, 0xEE, 0x19, 0x57, 0xA0,
> +		0x15, 0xE2, 0xAC, 0x5B, 0x30, 0xC7, 0x89, 0x7E,
> +		0x5F, 0xA8, 0xE6, 0x11, 0x7A, 0x8D, 0xC3, 0x34,
> +		0xAB, 0x5C, 0x12, 0xE5, 0x8E, 0x79, 0x37, 0xC0,
> +		0xE1, 0x16, 0x58, 0xAF, 0xC4, 0x33, 0x7D, 0x8A,
> +		0x3F, 0xC8, 0x86, 0x71, 0x1A, 0xED, 0xA3, 0x54,
> +		0x75, 0x82, 0xCC, 0x3B, 0x50, 0xA7, 0xE9, 0x1E,
> +		0xD4, 0x23, 0x6D, 0x9A, 0xF1, 0x06, 0x48, 0xBF,
> +		0x9E, 0x69, 0x27, 0xD0, 0xBB, 0x4C, 0x02, 0xF5,
> +		0x40, 0xB7, 0xF9, 0x0E, 0x65, 0x92, 0xDC, 0x2B,
> +		0x0A, 0xFD, 0xB3, 0x44, 0x2F, 0xD8, 0x96, 0x61,
> +		0x55, 0xA2, 0xEC, 0x1B, 0x70, 0x87, 0xC9, 0x3E,
> +		0x1F, 0xE8, 0xA6, 0x51, 0x3A, 0xCD, 0x83, 0x74,
> +		0xC1, 0x36, 0x78, 0x8F, 0xE4, 0x13, 0x5D, 0xAA,
> +		0x8B, 0x7C, 0x32, 0xC5, 0xAE, 0x59, 0x17, 0xE0,
> +		0x2A, 0xDD, 0x93, 0x64, 0x0F, 0xF8, 0xB6, 0x41,
> +		0x60, 0x97, 0xD9, 0x2E, 0x45, 0xB2, 0xFC, 0x0B,
> +		0xBE, 0x49, 0x07, 0xF0, 0x9B, 0x6C, 0x22, 0xD5,
> +		0xF4, 0x03, 0x4D, 0xBA, 0xD1, 0x26, 0x68, 0x9F,
> +	};
> +	return t[crc ^ data];
> +}

hm, the static-table-in-an-inline is a bit funny-looking.  One assumes that
gcc has the brains to not generate two copies of the table.

Is this crc function so specific to ssb that a move to lib/crc<something>.c
is inappropriate?

> +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;
> +	}
> +	if (need_seg == 1)
> +		*offset -= 0x800;
> +
> +	return 0;
> +}

This should be uninlined.

> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/drivers/ssb/ssb_private.h	2007-07-12 10:51:49.000000000 +0200
> @@ -0,0 +1,137 @@
> +#ifndef LINUX_SSB_PRIVATE_H_
> +#define LINUX_SSB_PRIVATE_H_
> +
> +#include <linux/ssb/ssb.h>
> +#include <linux/types.h>
> +#include <asm/io.h>
> +
> +
> +#define PFX	"ssb: "
> +
> +#ifdef CONFIG_SSB_SILENT
> +# define ssb_printk(fmt, x...)	do { /* nothing */ } while (0)
> +#else
> +# define ssb_printk		printk
> +#endif /* CONFIG_SSB_SILENT */
> +
> +/* dprintk: Debugging printk; vanishes for non-debug compilation */
> +#ifdef CONFIG_SSB_DEBUG
> +# define ssb_dprintk(fmt, x...)	ssb_printk(fmt ,##x)
> +#else
> +# define ssb_dprintk(fmt, x...)	do { /* nothing */ } while (0)
> +#endif
> +
> +/* printkl: Rate limited printk */
> +#define ssb_printkl(fmt, x...)	do {		\
> +	if (printk_ratelimit())			\
> +		ssb_printk(fmt ,##x);		\
> +				} while (0)
> +
> +/* dprintkl: Rate limited debugging printk */
> +#ifdef CONFIG_SSB_DEBUG
> +# define ssb_dprintkl			ssb_printkl
> +#else
> +# define ssb_dprintkl(fmt, x...)	do { /* nothing */ } while (0)
> +#endif

It continues to drive me wild how every driver has to go and implement its
own debug infrastructure.

> +#define assert(cond)	do {						\
> +	if (unlikely(!(cond))) {					\
> +		ssb_dprintk(KERN_ERR PFX "BUG: Assertion failed (%s) "	\
> +			    "at: %s:%d:%s()\n",				\
> +			    #cond, __FILE__, __LINE__, __func__);	\
> +	}								\
> +		       } while (0)

Odd.  One would normally expect an assert() to terminate execution in some
fashion if it fails.  In-kernel that means BUG.  But this assert() just
whines and continues.

So this guy should be renamed.  ssb_check_true() or someething like that.

> +#ifdef CONFIG_SSB_PCIHOST
> +extern int ssb_devices_freeze(struct ssb_bus *bus);
> +extern int ssb_devices_thaw(struct ssb_bus *bus);
> +extern struct ssb_bus * ssb_pci_dev_to_bus(struct pci_dev *pdev);
> +#endif /* CONFIG_SSB_PCIHOST */

Often we don't bother putting the ifdef guards around declarations like
this.  If you remove them and later make a mistake, you'll find out at
link-time rather than compile-time, which is a loss.  But the gain is
nicer-looking code, and code gets read 10000 times more often than it gets
written.

> +
> +#endif /* LINUX_SSB_PRIVATE_H_ */
> Index: linux-2.6/include/linux/ssb/ssb.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/include/linux/ssb/ssb.h	2007-07-12 10:51:49.000000000 +0200
> @@ -0,0 +1,422 @@
> +#ifndef LINUX_SSB_H_
> +#define LINUX_SSB_H_
> +#ifdef __KERNEL__

This header file is exported to userspace?  But I see no update to
include/linux/ssb/Kconfig.  Does it all pass `make headers_check'?  Does
`make headers_install' work correctly?

> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +#ifdef CONFIG_SSB_PCIHOST
> +# include <linux/pci.h>
> +#endif

might-be-unneeded ifdefs

> +	/* Lock for core and segment switching. */
> +	spinlock_t bar_lock;
> +
> +	/* The bus this backplane is running on. */
> +	enum ssb_bustype bustype;
> +	/* Pointer to the PCI bus (only valid if bustype == SSB_BUSTYPE_PCI). */
> +	struct pci_dev *host_pci;
> +	/* Pointer to the PCMCIA device (only if bustype == SSB_BUSTYPE_PCMCIA). */
> +	struct pcmcia_device *host_pcmcia;
> +
> +#ifdef CONFIG_SSB_PCIHOST
> +	struct mutex pci_sprom_mutex;
> +#endif
> +
> +	/* ID information about the Chip. */
> +	u16 chip_id;
> +	u16 chip_rev;
> +	u8 chip_package;
> +
> +	/* List of devices (cores) on the backplane. */
> +	struct ssb_device devices[SSB_MAX_NR_CORES];
> +	u8 nr_devices;
> +
> +	/* Reference count. Number of suspended devices. */
> +	u8 suspend_cnt;
> +
> +	/* Software ID number for this bus. */
> +	unsigned int busnumber;
> +
> +	/* The ChipCommon device (if available). */
> +	struct ssb_chipcommon chipco;
> +	/* The PCI-core device (if available). */
> +	struct ssb_pcicore pcicore;
> +	/* The MIPS-core device (if available). */
> +	struct ssb_mipscore mipscore;
> +	/* The EXTif-core device (if available). */
> +	struct ssb_extif extif;
> +
> +	/* The following structure elements are not available in early
> +	 * SSB initialization. Though, they are available for regular
> +	 * registered drivers at any stage. So be careful when
> +	 * using them in the ssb core code. */
> +
> +	/* ID information about the PCB. */
> +	struct ssb_boardinfo boardinfo;
> +	/* Contents of the SPROM. */
> +	struct ssb_sprom sprom;
> +
> +	/* Internal. */
> +	struct list_head list;
> +};

Those are useful comments.

> +#endif /* __KERNEL__ */
> +#endif /* LINUX_SSB_H_ */

OK, it wasn't exported to userspace.  Maybe the #ifdef __KERNEL__ wasn't
needed at all?



Overall: nice-looking and (modulo the above comments) quite idiomatic
kernel code.  But the low level of commenting would make it pretty tough
for anyone else to do significant maintenance work against it, I expect. 
This is also a kernel idiom :(

Re: [patch 1/2] Merge the Sonics Silicon Backplane subsystem

[Michael Buesch]

On Thursday 12 July 2007 20:27, Andrew Morton wrote:
> > +config SSB_SILENT
> > +	bool "No SSB kernel messages"
> > +	depends on SSB
> > +	help
> > +	  This option turns off all Sonics Silicon Backplane printks.
> > +	  Note that you won't be able to identify problems, once
> > +	  messages are turned off.
> > +	  This might only be desired for production kernels on
> > +	  embedded devices to reduce the kernel size.
> 
> The fact that this is called "silent" rather than noisy makes me suspect
> that you'd prefer that this option usually be enabled.  If so, perhaps a
> `default y' is needed?

SILENT removes _all_ messages, including informational and error messages.
It's for embedded systems, where we don't need those messages (as hardware
doesn't change it's damn unlikely to generate errors at SSB stage), but
where we don't want to disable printk all over the kernel.

> > +	  Say N
> > +
> > +config SSB_DEBUG
> > +	bool "SSB debugging"
> > +	depends on SSB && !SSB_SILENT
> > +	help
> > +	  This turns on additional runtime checks and debugging
> > +	  messages. Turn this on for SSB troubleshooting.
> > +
> > +	  If unsure, say N
> 
> 'default n', perhaps.

I was told that 'default n' is bad, as 'default n' is default :)

> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/drivers/ssb/driver_chipcommon.c	2007-07-12 10:51:49.000000000 +0200
> >
> > ..
> >
> > +void ssb_chipco_set_clockmode(struct ssb_chipcommon *cc,
> > +			      enum ssb_clkmode mode)
> > +{
> > +	struct ssb_device *ccdev = cc->dev;
> > +	struct ssb_bus *bus;
> > +	u32 tmp;
> 
> eek, a tmp.
>
> > +	if (!ccdev)
> > +		return;
> > +	bus = ccdev->bus;
> > +	/* chipcommon cores prior to rev6 don't support dynamic clock control */
> > +	if (ccdev->id.revision < 6)
> > +		return;
> > +	/* chipcommon cores rev10 are a whole new ball game */
> > +	if (ccdev->id.revision >= 10)
> > +		return;
> > +	if (!(cc->capabilities & SSB_CHIPCO_CAP_PCTL))
> > +		return;
> > +
> > +	switch (mode) {
> > +	case SSB_CLKMODE_SLOW:
> > +		tmp = chipco_read32(cc, SSB_CHIPCO_SLOWCLKCTL);
> > +		tmp |= SSB_CHIPCO_SLOWCLKCTL_FSLOW;
> > +		chipco_write32(cc, SSB_CHIPCO_SLOWCLKCTL, tmp);
> 
> hm, ok, maybe the much-derided `tmp' is applicable here.

Yes, using named variables for all these reduces readability
in my opinion. For "read-reg, modify value, write-reg" I always
use a "tmp" variable, as I really really really think it's best.
I don't use "tmp" for something that goes over more than
three or four lines, though.

> > +		break;
> > +	case SSB_CLKMODE_FAST:
> > +		ssb_pci_xtal(bus, SSB_GPIO_XTAL, 1); /* Force crystal on */
> > +		tmp = chipco_read32(cc, SSB_CHIPCO_SLOWCLKCTL);
> > +		tmp &= ~SSB_CHIPCO_SLOWCLKCTL_FSLOW;
> > +		tmp |= SSB_CHIPCO_SLOWCLKCTL_IPLL;
> > +		chipco_write32(cc, SSB_CHIPCO_SLOWCLKCTL, tmp);
> > +		break;
> > +	case SSB_CLKMODE_DYNAMIC:
> > +		tmp = chipco_read32(cc, SSB_CHIPCO_SLOWCLKCTL);
> > +		tmp &= ~SSB_CHIPCO_SLOWCLKCTL_FSLOW;
> > +		tmp &= ~SSB_CHIPCO_SLOWCLKCTL_IPLL;
> > +		tmp &= ~SSB_CHIPCO_SLOWCLKCTL_ENXTAL;
> > +		if ((tmp & SSB_CHIPCO_SLOWCLKCTL_SRC) != SSB_CHIPCO_SLOWCLKCTL_SRC_XTAL)
> > +			tmp |= SSB_CHIPCO_SLOWCLKCTL_ENXTAL;
> > +		chipco_write32(cc, SSB_CHIPCO_SLOWCLKCTL, tmp);
> > +
> > +		/* for dynamic control, we have to release our xtal_pu "force on" */
> > +		if (tmp & SSB_CHIPCO_SLOWCLKCTL_ENXTAL)
> > +			ssb_pci_xtal(bus, SSB_GPIO_XTAL, 0);
> > +		break;
> > +	default:
> > +		assert(0);
> 
> whaaa?  Kernel code would use BUG(), please.

Ah, well.
We _could_ use a WARN_ON here, but I really think that it's not
desired to bloat the kernel here.
And I think a WARN_ON, that doesn't vanish for nondebug builds,
does bloat the kernel here.

> > +	}
> > +}
> > +
> > +/* Get the Slow Clock Source */
> > +static int chipco_pctl_get_slowclksrc(struct ssb_chipcommon *cc)
> > +{
> > +	struct ssb_bus *bus = cc->dev->bus;
> > +	u32 tmp = 0;
> 
> This initialisation is here only to kill a bogus gcc warning.  It should at
> least have been commented to indicate this sad fact.  Better would be to
> use uninitialized_var() which has the seme effect and is self-documenting,
> may be centrally disabled or altered, may be grepped for, etc.

Ok, if that's desired.

> > +static void calc_fast_powerup_delay(struct ssb_chipcommon *cc)
> > +{
> > +	struct ssb_bus *bus = cc->dev->bus;
> > +	int minfreq;
> > +	unsigned int tmp;
> > +	u32 pll_on_delay;
> > +
> > +	if (bus->bustype != SSB_BUSTYPE_PCI)
> > +		return;
> > +	if (!(cc->capabilities & SSB_CHIPCO_CAP_PCTL))
> > +		return;
> > +
> > +	minfreq = chipco_pctl_clockfreqlimit(cc, 0);
> > +	pll_on_delay = chipco_read32(cc, SSB_CHIPCO_PLLONDELAY);
> > +	tmp = (((pll_on_delay + 2) * 1000000) + (minfreq - 1)) / minfreq;
> > +	assert((tmp & ~0xFFFF) == 0);
> > +
> > +	cc->fast_pwrup_delay = tmp;
> > +}
> > +
> > +void ssb_chipcommon_init(struct ssb_chipcommon *cc)
> > +{
> > +	if (!cc->dev)
> > +		return; /* We don't have a ChipCommon */
> > +	chipco_powercontrol_init(cc);
> > +	ssb_chipco_set_clockmode(cc, SSB_CLKMODE_FAST);
> > +	calc_fast_powerup_delay(cc);
> > +}
> > +
> > +void ssb_chipco_suspend(struct ssb_chipcommon *cc, pm_message_t state)
> > +{
> > +	if (!cc->dev)
> > +		return;
> > +	ssb_chipco_set_clockmode(cc, SSB_CLKMODE_SLOW);
> > +}
> 
> Should this stuff be inside #ifdef CONFIG_PM?

Probably. I'll take a closer look at it.

> > +static inline u32 mips_read32(struct ssb_mipscore *mcore,
> > +			      u16 offset)
> > +{
> > +	return ssb_read32(mcore->dev, offset);
> > +}
> 
> hm, this doesn't seem like a well-chosen name for a driver-private function.

Why not? This is the driver for the MIPS-CPU control unit of the system.
This func won't be used outside of this C file. If, sometime in the future,
the mipscore driver increases in size and I split it into several files
(unlikely, but hey..), I will rename this to ssb_mips_read32(). As long
as this is a static func, I don't see the need for a namespace prefix.

> I guess it's OK given that it's in a .c file.
> 
> > ...
> >
> > +static void ssb_pcicore_init_hostmode(struct ssb_pcicore *pc)
> > +{
> > +	u32 val;
> > +
> > +	if (extpci_core) {
> > +		WARN_ON(1);
> > +		return;
> > +	}
> 
> fyi, one could do
> 
> 	if (WARN_ON(extpci_core))
> 		return;
> 
> here.

Ok, I didn't know that.

> > +static int pcicore_is_in_hostmode(struct ssb_pcicore *pc)
> > +{
> > +	struct ssb_bus *bus = pc->dev->bus;
> > +	u16 chipid_top;
> > +	u32 tmp;
> > +
> > +	chipid_top = (bus->chip_id & 0xFF00);
> > +	if (chipid_top != 0x4700 &&
> > +	    chipid_top != 0x5300)
> > +		return 0;
> > +
> > +	if (bus->sprom.r1.boardflags_lo & SSB_PCICORE_BFL_NOPCI)
> > +		return 0;
> > +
> > +	/* The 200-pin BCM4712 package does not bond out PCI. Even when
> > +	 * PCI is bonded out, some boards may leave the pins floating. */
> > +	if (bus->chip_id == 0x4712) {
> > +		if (bus->chip_package == SSB_CHIPPACK_BCM4712S)
> > +			return 0;
> > +		if (bus->chip_package == SSB_CHIPPACK_BCM4712M)
> > +			return 0;
> > +	}
> > +	if (bus->chip_id == 0x5350)
> > +		return 0;
> > +
> > +	return !mips_busprobe(tmp, (u32 *) (bus->mmio + (pc->dev->core_index * SSB_CORE_SIZE)));
> > +}
> 
> What's the situation with 64-bit stuff here?

Such hardware doesn't exist.
Though, it might be OK to use no cast at all here.
Not sure why we cast here. I didn't write this part.

> > +	assert(dev);
> 
> BUG_ON, please.
> 
> > +int ssb_pcicore_dev_irqvecs_enable(struct ssb_pcicore *pc,
> > +				   struct ssb_device *dev)
> > +{
> > +	struct ssb_device *pdev = pc->dev;
> > +	struct ssb_bus *bus;
> > +	int err = 0;
> > +	u32 tmp;
> > +
> > +	might_sleep();
> 
> If you say so.  It doesn't _look_ like it sleeps?

It does only sleep on special hardware that I don't have.
So if I do a change and call this from atomic, I won't notice
the bug. That's why we have might_sleep here.

> > +#include "ssb_private.h"
> > +
> > +#include <linux/delay.h>
> > +#include <linux/ssb/ssb.h>
> > +#include <linux/ssb/ssb_regs.h>
> > +
> > +#ifdef CONFIG_SSB_PCIHOST
> > +# include <linux/pci.h>
> > +#endif
> 
> This ifdef guard is probably a bad idea.  pci.h should be OK with or
> without CONFIG_SSB_PCIHOST and making the inclusion conditional serves to
> add more combinations which can cause compile failures.

Ok.

> > +
> > +static void ssb_bus_suspend(struct ssb_bus *bus, pm_message_t state)
> > +{
> > +	ssb_chipco_suspend(&bus->chipco, state);
> > +	ssb_pci_xtal(bus, SSB_GPIO_XTAL | SSB_GPIO_PLL, 0);
> > +
> > +	/* Reset HW state information in memory, so that HW is
> > +	 * completely reinitialized on resume. */
> > +	bus->mapped_device = NULL;
> > +#ifdef CONFIG_SSB_DRIVER_PCICORE
> > +	bus->pcicore.setup_done = 0;
> > +#endif
> > +}
> 
> No #ifdef CONFIG_PMs here?

I'll check this.

> >
> > ...
> >
> > +#define is_early_boot()		(ssb_bustype.name == NULL)
> > +
> > +static void ssb_buses_lock(void)
> > +{
> > +	if (!is_early_boot())
> > +		mutex_lock(&buses_mutex);
> > +}
> > +
> > +static void ssb_buses_unlock(void)
> > +{
> > +	if (!is_early_boot())
> > +		mutex_unlock(&buses_mutex);
> > +}
> 
> The is_early_boot() stuff looks hacky.  It surely needs a comment to
> explain to the reader what is going on.  Because it is *very* hard to work
> out what this is here for otherwise.

Well, the is_early_boot hack is needed, as the SSB subsystem is kind of
special. There are situations where we call it from _very_ early boot
and there are situations where we call it from userspace.
So on early boot we don't have mutex support, yet (no scheduler).
And locking is not needed early, anyway (No SMP. No concurrency possible).
I will add a comment explaining this.

> > +static int ssb_fetch_invariants(struct ssb_bus *bus,
> > +				int (*get_invariants)(struct ssb_bus *bus,
> > +						      struct ssb_init_invariants *iv))
> 
> fyi, this declaration-of-a-pointer-to-a-function thing is the one place
> where kernel code _will_ define a typedef merely to save typing.

Ok.

> > +{
> > +	struct ssb_init_invariants iv;
> > +	int err;
> > +
> > +	memset(&iv, 0, sizeof(iv));
> > +	err = get_invariants(bus, &iv);
> > +	if (err)
> > +		goto out;
> > +	memcpy(&bus->boardinfo, &iv.boardinfo, sizeof(iv.boardinfo));
> > +	memcpy(&bus->sprom, &iv.sprom, sizeof(iv.sprom));
> > +out:
> > +	return err;
> > +}
> > +
> >
> > ...
> >
> > +
> > +static inline u8 ssb_crc8(u8 crc, u8 data)
> > +{
> > +	/* Polynomial:   x^8 + x^7 + x^6 + x^4 + x^2 + 1   */
> > +	static const u8 t[] = {
> > +		0x00, 0xF7, 0xB9, 0x4E, 0x25, 0xD2, 0x9C, 0x6B,
> > +		0x4A, 0xBD, 0xF3, 0x04, 0x6F, 0x98, 0xD6, 0x21,
> > +		0x94, 0x63, 0x2D, 0xDA, 0xB1, 0x46, 0x08, 0xFF,
> > +		0xDE, 0x29, 0x67, 0x90, 0xFB, 0x0C, 0x42, 0xB5,
> > +		0x7F, 0x88, 0xC6, 0x31, 0x5A, 0xAD, 0xE3, 0x14,
> > +		0x35, 0xC2, 0x8C, 0x7B, 0x10, 0xE7, 0xA9, 0x5E,
> > +		0xEB, 0x1C, 0x52, 0xA5, 0xCE, 0x39, 0x77, 0x80,
> > +		0xA1, 0x56, 0x18, 0xEF, 0x84, 0x73, 0x3D, 0xCA,
> > +		0xFE, 0x09, 0x47, 0xB0, 0xDB, 0x2C, 0x62, 0x95,
> > +		0xB4, 0x43, 0x0D, 0xFA, 0x91, 0x66, 0x28, 0xDF,
> > +		0x6A, 0x9D, 0xD3, 0x24, 0x4F, 0xB8, 0xF6, 0x01,
> > +		0x20, 0xD7, 0x99, 0x6E, 0x05, 0xF2, 0xBC, 0x4B,
> > +		0x81, 0x76, 0x38, 0xCF, 0xA4, 0x53, 0x1D, 0xEA,
> > +		0xCB, 0x3C, 0x72, 0x85, 0xEE, 0x19, 0x57, 0xA0,
> > +		0x15, 0xE2, 0xAC, 0x5B, 0x30, 0xC7, 0x89, 0x7E,
> > +		0x5F, 0xA8, 0xE6, 0x11, 0x7A, 0x8D, 0xC3, 0x34,
> > +		0xAB, 0x5C, 0x12, 0xE5, 0x8E, 0x79, 0x37, 0xC0,
> > +		0xE1, 0x16, 0x58, 0xAF, 0xC4, 0x33, 0x7D, 0x8A,
> > +		0x3F, 0xC8, 0x86, 0x71, 0x1A, 0xED, 0xA3, 0x54,
> > +		0x75, 0x82, 0xCC, 0x3B, 0x50, 0xA7, 0xE9, 0x1E,
> > +		0xD4, 0x23, 0x6D, 0x9A, 0xF1, 0x06, 0x48, 0xBF,
> > +		0x9E, 0x69, 0x27, 0xD0, 0xBB, 0x4C, 0x02, 0xF5,
> > +		0x40, 0xB7, 0xF9, 0x0E, 0x65, 0x92, 0xDC, 0x2B,
> > +		0x0A, 0xFD, 0xB3, 0x44, 0x2F, 0xD8, 0x96, 0x61,
> > +		0x55, 0xA2, 0xEC, 0x1B, 0x70, 0x87, 0xC9, 0x3E,
> > +		0x1F, 0xE8, 0xA6, 0x51, 0x3A, 0xCD, 0x83, 0x74,
> > +		0xC1, 0x36, 0x78, 0x8F, 0xE4, 0x13, 0x5D, 0xAA,
> > +		0x8B, 0x7C, 0x32, 0xC5, 0xAE, 0x59, 0x17, 0xE0,
> > +		0x2A, 0xDD, 0x93, 0x64, 0x0F, 0xF8, 0xB6, 0x41,
> > +		0x60, 0x97, 0xD9, 0x2E, 0x45, 0xB2, 0xFC, 0x0B,
> > +		0xBE, 0x49, 0x07, 0xF0, 0x9B, 0x6C, 0x22, 0xD5,
> > +		0xF4, 0x03, 0x4D, 0xBA, 0xD1, 0x26, 0x68, 0x9F,
> > +	};
> > +	return t[crc ^ data];
> > +}
> 
> hm, the static-table-in-an-inline is a bit funny-looking.  One assumes that
> gcc has the brains to not generate two copies of the table.

I'm sure it is.

> Is this crc function so specific to ssb that a move to lib/crc<something>.c
> is inappropriate?

This is a special polynomial, that only broadcom uses, for whatever reason.

> > +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;
> > +	}
> > +	if (need_seg == 1)
> > +		*offset -= 0x800;
> > +
> > +	return 0;
> > +}
> 
> This should be uninlined.

I'm not too sure about this. This codepath is the hottest codepath
in the whole SSB subsystem. It's called on _every_ register access.
I intentionally did it this way to have the likely path inline
and the unlikely path out of line (see the unlikely if-statements).

> > +/* dprintkl: Rate limited debugging printk */
> > +#ifdef CONFIG_SSB_DEBUG
> > +# define ssb_dprintkl			ssb_printkl
> > +#else
> > +# define ssb_dprintkl(fmt, x...)	do { /* nothing */ } while (0)
> > +#endif
> 
> It continues to drive me wild how every driver has to go and implement its
> own debug infrastructure.
> 
> > +#define assert(cond)	do {						\
> > +	if (unlikely(!(cond))) {					\
> > +		ssb_dprintk(KERN_ERR PFX "BUG: Assertion failed (%s) "	\
> > +			    "at: %s:%d:%s()\n",				\
> > +			    #cond, __FILE__, __LINE__, __func__);	\
> > +	}								\
> > +		       } while (0)
> 
> Odd.  One would normally expect an assert() to terminate execution in some
> fashion if it fails.  In-kernel that means BUG.  But this assert() just
> whines and continues.

An assertion failure is not a fatal bug, so we continue execution.
We do the same in bcm43xx and I really think we mustn't BUG on an
assertion failure, as people would already have killed me.
Additionally to that, I insert really lots of assert()s into the code.
I don't want all these to be WARN_ONs or BUGs, as it would bloat the
kernel a lot. In the places where I want runtime checks in nondebug
builds, I already use WARN_ON.

> > +#ifdef CONFIG_SSB_PCIHOST
> > +extern int ssb_devices_freeze(struct ssb_bus *bus);
> > +extern int ssb_devices_thaw(struct ssb_bus *bus);
> > +extern struct ssb_bus * ssb_pci_dev_to_bus(struct pci_dev *pdev);
> > +#endif /* CONFIG_SSB_PCIHOST */
> 
> Often we don't bother putting the ifdef guards around declarations like
> this.  If you remove them and later make a mistake, you'll find out at
> link-time rather than compile-time, which is a loss.  But the gain is
> nicer-looking code, and code gets read 10000 times more often than it gets
> written.

OK.

> > +
> > +#endif /* LINUX_SSB_PRIVATE_H_ */
> > Index: linux-2.6/include/linux/ssb/ssb.h
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/include/linux/ssb/ssb.h	2007-07-12 10:51:49.000000000 +0200
> > @@ -0,0 +1,422 @@
> > +#ifndef LINUX_SSB_H_
> > +#define LINUX_SSB_H_
> > +#ifdef __KERNEL__
> 
> This header file is exported to userspace?  But I see no update to
> include/linux/ssb/Kconfig.  Does it all pass `make headers_check'?  Does
> `make headers_install' work correctly?

No. I just wanted to make sure to humans that it is _not_ exported.

> > +#include <linux/device.h>
> > +#include <linux/list.h>
> > +#include <linux/types.h>
> > +#include <linux/spinlock.h>
> > +#ifdef CONFIG_SSB_PCIHOST
> > +# include <linux/pci.h>
> > +#endif
> 
> might-be-unneeded ifdefs

Yep.

> > +	/* ID information about the PCB. */
> > +	struct ssb_boardinfo boardinfo;
> > +	/* Contents of the SPROM. */
> > +	struct ssb_sprom sprom;
> > +
> > +	/* Internal. */
> > +	struct list_head list;
> > +};
> 
> Those are useful comments.

I'll try to add more of these kind of comments to the rest of the code. ;)

> Overall: nice-looking and (modulo the above comments) quite idiomatic
> kernel code.  But the low level of commenting would make it pretty tough
> for anyone else to do significant maintenance work against it, I expect. 
> This is also a kernel idiom :(
> 
> 

Re: [patch 1/2] Merge the Sonics Silicon Backplane subsystem

[Andrew Morton]

On Thu, 12 Jul 2007 23:42:26 +0200
Michael Buesch <mb@bu3sch.de> wrote:

> > > +#define assert(cond)	do {						\
> > > +	if (unlikely(!(cond))) {					\
> > > +		ssb_dprintk(KERN_ERR PFX "BUG: Assertion failed (%s) "	\
> > > +			    "at: %s:%d:%s()\n",				\
> > > +			    #cond, __FILE__, __LINE__, __func__);	\
> > > +	}								\
> > > +		       } while (0)
> > 
> > Odd.  One would normally expect an assert() to terminate execution in some
> > fashion if it fails.  In-kernel that means BUG.  But this assert() just
> > whines and continues.
> 
> An assertion failure is not a fatal bug, so we continue execution.
> We do the same in bcm43xx and I really think we mustn't BUG on an
> assertion failure, as people would already have killed me.
> Additionally to that, I insert really lots of assert()s into the code.
> I don't want all these to be WARN_ONs or BUGs, as it would bloat the
> kernel a lot. In the places where I want runtime checks in nondebug
> builds, I already use WARN_ON.

Do `man 3 assert'.  The reader of your code will expect that an assert()
will "terminate the program" (or the kernel equivalent) if the assertion
fails.

So the principle of least surprise tells us "this shouldn't be called
assert()".

Re: [patch 1/2] Merge the Sonics Silicon Backplane subsystem

[Michael Buesch]

On Thursday 12 July 2007 23:53, Andrew Morton wrote:
> On Thu, 12 Jul 2007 23:42:26 +0200
> Michael Buesch <mb@bu3sch.de> wrote:
> 
> > > > +#define assert(cond)	do {						\
> > > > +	if (unlikely(!(cond))) {					\
> > > > +		ssb_dprintk(KERN_ERR PFX "BUG: Assertion failed (%s) "	\
> > > > +			    "at: %s:%d:%s()\n",				\
> > > > +			    #cond, __FILE__, __LINE__, __func__);	\
> > > > +	}								\
> > > > +		       } while (0)
> > > 
> > > Odd.  One would normally expect an assert() to terminate execution in some
> > > fashion if it fails.  In-kernel that means BUG.  But this assert() just
> > > whines and continues.
> > 
> > An assertion failure is not a fatal bug, so we continue execution.
> > We do the same in bcm43xx and I really think we mustn't BUG on an
> > assertion failure, as people would already have killed me.
> > Additionally to that, I insert really lots of assert()s into the code.
> > I don't want all these to be WARN_ONs or BUGs, as it would bloat the
> > kernel a lot. In the places where I want runtime checks in nondebug
> > builds, I already use WARN_ON.
> 
> Do `man 3 assert'.  The reader of your code will expect that an assert()
> will "terminate the program" (or the kernel equivalent) if the assertion
> fails.
> 
> So the principle of least surprise tells us "this shouldn't be called
> assert()".

Well, I do know that userspace assert() terminates the program.
But, in the kernel we use BUG() for this.
So let's better rename BUG() to assert() ;)
No just kidding.
IMO the word "assert" is short and to the point what this code
is actually doing. It asserts that a condition is true and
complains otherwise.

Let's make a deal, Andrew.
As I almost always do assert(0), I will remove the assert() macro
and introduce a macro SSB_CAN_NOT_REACH() or something like that
to mark codepaths that can not be reached.
I'll replace the rest of the assert()s that check an actual condition
with WARN_ON.
OK?

Re: [patch 1/2] Merge the Sonics Silicon Backplane subsystem

[Andrew Morton]

On Fri, 13 Jul 2007 00:10:58 +0200
Michael Buesch <mb@bu3sch.de> wrote:

> > So the principle of least surprise tells us "this shouldn't be called
> > assert()".
> 
> Well, I do know that userspace assert() terminates the program.
> But, in the kernel we use BUG() for this.
> So let's better rename BUG() to assert() ;)
> No just kidding.
> IMO the word "assert" is short and to the point what this code
> is actually doing. It asserts that a condition is true and
> complains otherwise.
> 
> Let's make a deal, Andrew.
> As I almost always do assert(0), I will remove the assert() macro
> and introduce a macro SSB_CAN_NOT_REACH() or something like that
> to mark codepaths that can not be reached.
> I'll replace the rest of the assert()s that check an actual condition
> with WARN_ON.
> OK?

I don't understand all the fuss.

When I looked at that code I assumed that your assert() implementation
would be a wrapper of some from around a BUG().  But it wasn't.

A suitable way of preventing others from being similarly surprised would be
to call it something other than assert().  That's all.

Re: [patch 1/2] Merge the Sonics Silicon Backplane subsystem

[Aurelien Jarno]

On Thu, Jul 12, 2007 at 10:54:33AM +0200, mb@bu3sch.de wrote:
> This merges the SSB subsystem. SSB is a backplane bus subsystem needed
> for various Broadcom devices.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>

[...]

> Index: linux-2.6/include/linux/ssb/ssb_regs.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6/include/linux/ssb/ssb_regs.h	2007-07-12 10:51:49.000000000 +0200
> @@ -0,0 +1,294 @@

[...]

> +#define	SSB_EXTIF_BASE		0x1f000000	/* External Interface region base address */
> +#define	SSB_FLASH1		0x1fc00000	/* Flash Region 1 */
> +#define	SSB_FLASH1_SZ		0x00400000	/* Size of Flash Region 1 */
> +
> +#define SSB_PCI_DMA		0x40000000	/* Client Mode sb2pcitranslation2 (1 GB) */
> +#define SSB_PCI_DMA_SZ		0x40000000	/* Client Mode sb2pcitranslation2 size in bytes */
> +#define SSB_PCIE_DMA_L32		0x00000000	/* PCIE Client Mode sb2pcitranslation2 (2 ZettaBytes), low 32 bits */
> +#define SSB_PCIE_DMA_H32		0x80000000	/* PCIE Client Mode sb2pcitranslation2 (2 ZettaBytes), high 32 bits */
> +#define	SSB_EUART		(SB_EXTIF_BASE + 0x00800000)
> +#define	SSB_LED			(SB_EXTIF_BASE + 0x00900000)

I have just noticed a typo here. That should be SSB_EXTIF_BASE instead
of SB_EXTIF_BASE.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [patch 1/2] Merge the Sonics Silicon Backplane subsystem

[Holger Schurig]

> > +#define	SSB_EXTIF_BASE		0x1f000000	/* External Interface
> > +#define	SSB_EUART		(SB_EXTIF_BASE + 0x00800000)
> > +#define	SSB_LED			(SB_EXTIF_BASE + 0x00900000)
>
> I have just noticed a typo here. That should be SSB_EXTIF_BASE
> instead of SB_EXTIF_BASE.

Or remove those two defines completely, obviously they are unused.
Otherwise Michael would have get an compiler error.

Re: [patch 1/2] Merge the Sonics Silicon Backplane subsystem

[Michael Buesch]

On Friday 13 July 2007 08:11:00 Aurelien Jarno wrote:
> On Thu, Jul 12, 2007 at 10:54:33AM +0200, mb@bu3sch.de wrote:
> > This merges the SSB subsystem. SSB is a backplane bus subsystem needed
> > for various Broadcom devices.
> > 
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> 
> [...]
> 
> > Index: linux-2.6/include/linux/ssb/ssb_regs.h
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6/include/linux/ssb/ssb_regs.h	2007-07-12 10:51:49.000000000 +0200
> > @@ -0,0 +1,294 @@
> 
> [...]
> 
> > +#define	SSB_EXTIF_BASE		0x1f000000	/* External Interface region base address */
> > +#define	SSB_FLASH1		0x1fc00000	/* Flash Region 1 */
> > +#define	SSB_FLASH1_SZ		0x00400000	/* Size of Flash Region 1 */
> > +
> > +#define SSB_PCI_DMA		0x40000000	/* Client Mode sb2pcitranslation2 (1 GB) */
> > +#define SSB_PCI_DMA_SZ		0x40000000	/* Client Mode sb2pcitranslation2 size in bytes */
> > +#define SSB_PCIE_DMA_L32		0x00000000	/* PCIE Client Mode sb2pcitranslation2 (2 ZettaBytes), low 32 bits */
> > +#define SSB_PCIE_DMA_H32		0x80000000	/* PCIE Client Mode sb2pcitranslation2 (2 ZettaBytes), high 32 bits */
> > +#define	SSB_EUART		(SB_EXTIF_BASE + 0x00800000)
> > +#define	SSB_LED			(SB_EXTIF_BASE + 0x00900000)
> 
> I have just noticed a typo here. That should be SSB_EXTIF_BASE instead
> of SB_EXTIF_BASE.
> 

Thanks, I fixed this.

-- 
Greetings Michael.