bcma: add support for embedded devices like bcm4716

When using bcma on a embedded device it is initialized very early at boot. We have to do so as the cpu and interrupt management and all other devices are attached to this bus and it has to be initialized so early. In that stage we can not allocate memory or sleep, just use the memory on the stack and in the text segment as the kernel is not initialized far enough. This patch removed the kzallocs from the scan code. Some earlier version of the bcma implementation and the normal ssb implementation are doing it like this.
The __bcma_dev_wrapper struct is used as the container for the device struct as bcma_device will be too big if it includes struct device.

When using bcma on a embedded device it is initialized very early at boot. We have to do so as the cpu and interrupt management and all other devices are attached to this bus and it has to be initialized so early. In that stage we can not allocate memory or sleep, just use the memory on the stack and in the text segment as the kernel is not initialized far enough. This patch removed the kzallocs from the scan code. Some earlier version of the bcma implementation and the normal ssb implementation are doing it like this.
The __bcma_dev_wrapper struct is used as the container for the device struct as bcma_device will be too big if it includes struct device.

Signed-off-by Hauke Mehrtens <[email protected]>

---

?drivers/bcma/main.c ? ? ? | ? 86 ++++++++++++++++++++++++++++----------------
?drivers/bcma/scan.c ? ? ? | ? 58 +++++++++++-------------------
?include/linux/bcma/bcma.h | ? 16 ++++++--

?3 files changed, 89 insertions(+), 71 deletions(-)

drivers/bcma/main.c +24 -17

index a2f6b18..b0e7f5e 100644
@@ -17,23 +17,27 @@ static int bcma_device_remove(struct device *dev);
 
 ?static ssize_t manuf_show(struct device *dev, struct device_attribute *attr, char *buf)
 ?{
- ? ? ? struct bcma_device *core = container_of(dev, struct bcma_device, dev);
- ? ? ? return sprintf(buf, "0x%03X\n", core->id.manuf);
+ ? ? ? struct __bcma_dev_wrapper *wrapper = container_of(dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct __bcma_dev_wrapper, dev);
+ ? ? ? return sprintf(buf, "0x%03X\n", wrapper->core->id.manuf);
 ?}
 ?static ssize_t id_show(struct device *dev, struct device_attribute *attr, char *buf)
 ?{
- ? ? ? struct bcma_device *core = container_of(dev, struct bcma_device, dev);
- ? ? ? return sprintf(buf, "0x%03X\n", core->id.id);
+ ? ? ? struct __bcma_dev_wrapper *wrapper = container_of(dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct __bcma_dev_wrapper, dev);
+ ? ? ? return sprintf(buf, "0x%03X\n", wrapper->core->id.id);
 ?}
 ?static ssize_t rev_show(struct device *dev, struct device_attribute *attr, char *buf)
 ?{
- ? ? ? struct bcma_device *core = container_of(dev, struct bcma_device, dev);
- ? ? ? return sprintf(buf, "0x%02X\n", core->id.rev);
+ ? ? ? struct __bcma_dev_wrapper *wrapper = container_of(dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct __bcma_dev_wrapper, dev);
+ ? ? ? return sprintf(buf, "0x%02X\n", wrapper->core->id.rev);
 ?}
 ?static ssize_t class_show(struct device *dev, struct device_attribute *attr, char *buf)
 ?{
- ? ? ? struct bcma_device *core = container_of(dev, struct bcma_device, dev);
- ? ? ? return sprintf(buf, "0x%X\n", core->id.class);
+ ? ? ? struct __bcma_dev_wrapper *wrapper = container_of(dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct __bcma_dev_wrapper, dev);
+ ? ? ? return sprintf(buf, "0x%X\n", wrapper->core->id.class);
 ?}
 ?static struct device_attribute bcma_device_attrs[] = {
 ? ? ? ?__ATTR_RO(manuf),

@@ -53,27 +57,30 @@ static struct bus_type bcma_bus_type = {
 
 ?static struct bcma_device *bcma_find_core(struct bcma_bus *bus, u16 coreid)
 ?{
- ? ? ? struct bcma_device *core;
-
- ? ? ? list_for_each_entry(core, &bus->cores, list) {
- ? ? ? ? ? ? ? if (core->id.id == coreid)
- ? ? ? ? ? ? ? ? ? ? ? return core;
+ ? ? ? u8 i;
+ ? ? ? for (i = 0; i < bus->nr_cores; i++) {
+ ? ? ? ? ? ? ? if (bus->cores[i].id.id == coreid)
+ ? ? ? ? ? ? ? ? ? ? ? return &bus->cores[i];
 ? ? ? ?}
 ? ? ? ?return NULL;
 ?}
 
 ?static void bcma_release_core_dev(struct device *dev)
 ?{
- ? ? ? struct bcma_device *core = container_of(dev, struct bcma_device, dev);
- ? ? ? kfree(core);
+ ? ? ? struct __bcma_dev_wrapper *wrapper = container_of(dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct __bcma_dev_wrapper, dev);
+ ? ? ? kfree(wrapper);
 ?}
 
 ?static int bcma_register_cores(struct bcma_bus *bus)
 ?{
 ? ? ? ?struct bcma_device *core;
- ? ? ? int err, dev_id = 0;
+ ? ? ? struct __bcma_dev_wrapper *wrapper;
+ ? ? ? int i, err, dev_id = 0;
+
+ ? ? ? for (i = 0; i < bus->nr_cores; i++) {
+ ? ? ? ? ? ? ? core = &(bus->cores[i]);
 
- ? ? ? list_for_each_entry(core, &bus->cores, list) {

? ? ? ? ? ? ? ?/* We support that cores ourself */ ? ? ? ? ? ? ? ?switch (core->id.id) {
? ? ? ? ? ? ? ?case BCMA_CORE_CHIPCOMMON:

drivers/bcma/main.c +11 -3

@@ -82,28 +89,37 @@ static int bcma_register_cores(struct bcma_bus *bus)
 ? ? ? ? ? ? ? ? ? ? ? ?continue;
 ? ? ? ? ? ? ? ?}
 
- ? ? ? ? ? ? ? core->dev.release = bcma_release_core_dev;
- ? ? ? ? ? ? ? core->dev.bus = &bcma_bus_type;
- ? ? ? ? ? ? ? dev_set_name(&core->dev, "bcma%d:%d", 0/*bus->num*/, dev_id);
+ ? ? ? ? ? ? ? wrapper = kzalloc(sizeof(*wrapper), GFP_KERNEL);
+ ? ? ? ? ? ? ? if (!wrapper) {
+ ? ? ? ? ? ? ? ? ? ? ? pr_err("Could not allocate wrapper for core 0x%03X\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?core->id.id);
+ ? ? ? ? ? ? ? ? ? ? ? continue;
+ ? ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? ? wrapper->core = core;
+ ? ? ? ? ? ? ? wrapper->dev.release = bcma_release_core_dev;
+ ? ? ? ? ? ? ? wrapper->dev.bus = &bcma_bus_type;
+ ? ? ? ? ? ? ? dev_set_name(&wrapper->dev, "bcma%d:%d", 0/*bus->num*/, dev_id);
 
 ? ? ? ? ? ? ? ?switch (bus->hosttype) {

? ? ? ? ? ? ? ?case BCMA_HOSTTYPE_PCI:
- ? ? ? ? ? ? ? ? ? ? ? core->dev.parent = &bus->host_pci->dev;
- ? ? ? ? ? ? ? ? ? ? ? core->dma_dev = &bus->host_pci->dev;
- ? ? ? ? ? ? ? ? ? ? ? core->irq = bus->host_pci->irq;
+ ? ? ? ? ? ? ? ? ? ? ? wrapper->dev.parent = &bus->host_pci->dev;
+ ? ? ? ? ? ? ? ? ? ? ? wrapper->core->dma_dev = &bus->host_pci->dev;
+ ? ? ? ? ? ? ? ? ? ? ? wrapper->core->irq = bus->host_pci->irq; ? ? ? ? ? ? ? ? ? ? ? ?break;
? ? ? ? ? ? ? ?case BCMA_HOSTTYPE_NONE:
? ? ? ? ? ? ? ?case BCMA_HOSTTYPE_SDIO:
? ? ? ? ? ? ? ? ? ? ? ?break;
? ? ? ? ? ? ? ?}

- ? ? ? ? ? ? ? err = device_register(&core->dev);
+ ? ? ? ? ? ? ? err = device_register(&wrapper->dev);
? ? ? ? ? ? ? ?if (err) {
? ? ? ? ? ? ? ? ? ? ? ?pr_err("Could not register dev for core 0x%03X\n", ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? core->id.id);
+ ? ? ? ? ? ? ? ? ? ? ? kfree(wrapper);
? ? ? ? ? ? ? ? ? ? ? ?continue;
? ? ? ? ? ? ? ?}
- ? ? ? ? ? ? ? core->dev_registered = true;
+ ? ? ? ? ? ? ? core->dev = &wrapper->dev;
? ? ? ? ? ? ? ?dev_id++;
? ? ? ?}

drivers/bcma/main.c +14 -6

@@ -113,10 +129,12 @@ static int bcma_register_cores(struct bcma_bus *bus)
 ?static void bcma_unregister_cores(struct bcma_bus *bus)
 ?{
 ? ? ? ?struct bcma_device *core;
+ ? ? ? int i;
 
- ? ? ? list_for_each_entry(core, &bus->cores, list) {
- ? ? ? ? ? ? ? if (core->dev_registered)
- ? ? ? ? ? ? ? ? ? ? ? device_unregister(&core->dev);
+ ? ? ? for (i = 0; i < bus->nr_cores; i++) {
+ ? ? ? ? ? ? ? core = &(bus->cores[i]);
+ ? ? ? ? ? ? ? if (core->dev)
+ ? ? ? ? ? ? ? ? ? ? ? device_unregister(core->dev);
 ? ? ? ?}
 ?}
 

@@ -179,7 +197,9 @@ EXPORT_SYMBOL_GPL(bcma_driver_unregister);
 
 ?static int bcma_bus_match(struct device *dev, struct device_driver *drv)
 ?{
- ? ? ? struct bcma_device *core = container_of(dev, struct bcma_device, dev);
+ ? ? ? struct __bcma_dev_wrapper *wrapper = container_of(dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct __bcma_dev_wrapper, dev);
+ ? ? ? struct bcma_device *core = wrapper->core;
 ? ? ? ?struct bcma_driver *adrv = container_of(drv, struct bcma_driver, drv);
 ? ? ? ?const struct bcma_device_id *cid = &core->id;
 ? ? ? ?const struct bcma_device_id *did;

@@ -196,7 +216,9 @@ static int bcma_bus_match(struct device *dev, struct device_driver *drv)
 
 ?static int bcma_device_probe(struct device *dev)
 ?{
- ? ? ? struct bcma_device *core = container_of(dev, struct bcma_device, dev);
+ ? ? ? struct __bcma_dev_wrapper *wrapper = container_of(dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct __bcma_dev_wrapper, dev);
+ ? ? ? struct bcma_device *core = wrapper->core;
 ? ? ? ?struct bcma_driver *adrv = container_of(dev->driver, struct bcma_driver,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? drv);
 ? ? ? ?int err = 0;

@@ -209,7 +231,9 @@ static int bcma_device_probe(struct device *dev)
 
 ?static int bcma_device_remove(struct device *dev)
 ?{
- ? ? ? struct bcma_device *core = container_of(dev, struct bcma_device, dev);
+ ? ? ? struct __bcma_dev_wrapper *wrapper = container_of(dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct __bcma_dev_wrapper, dev);
+ ? ? ? struct bcma_device *core = wrapper->core;
 ? ? ? ?struct bcma_driver *adrv = container_of(dev->driver, struct bcma_driver,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? drv);
 

drivers/bcma/scan.c +2 -8

index 40d7dcc..70b39f7 100644
@@ -211,9 +211,6 @@ int bcma_bus_scan(struct bcma_bus *bus)
 ? ? ? ?s32 tmp;
 ? ? ? ?u8 i, j;
 
- ? ? ? int err;
-
- ? ? ? INIT_LIST_HEAD(&bus->cores);
 ? ? ? ?bus->nr_cores = 0;
 
 ? ? ? ?bcma_scan_switch_core(bus, BCMA_ADDR_BASE);

@@ -230,11 +227,8 @@ int bcma_bus_scan(struct bcma_bus *bus)
 ? ? ? ?bcma_scan_switch_core(bus, erombase);
 
 ? ? ? ?while (eromptr < eromend) {
- ? ? ? ? ? ? ? struct bcma_device *core = kzalloc(sizeof(*core), GFP_KERNEL);
- ? ? ? ? ? ? ? if (!core)
- ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
- ? ? ? ? ? ? ? INIT_LIST_HEAD(&core->list);
- ? ? ? ? ? ? ? core->bus = bus;
+ ? ? ? ? ? ? ? struct bcma_device core = { };
+ ? ? ? ? ? ? ? core.bus = bus;
 

? ? ? ? ? ? ? ?/* get CIs */
? ? ? ? ? ? ? ?cia = bcma_erom_get_ci(bus, &eromptr);

drivers/bcma/scan.c +3 -6

@@ -242,27 +236,24 @@ int bcma_bus_scan(struct bcma_bus *bus)
 ? ? ? ? ? ? ? ? ? ? ? ?bcma_erom_push_ent(&eromptr);
 ? ? ? ? ? ? ? ? ? ? ? ?if (bcma_erom_is_end(bus, &eromptr))
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
- ? ? ? ? ? ? ? ? ? ? ? err= -EILSEQ;
- ? ? ? ? ? ? ? ? ? ? ? goto out;
+ ? ? ? ? ? ? ? ? ? ? ? return -EILSEQ;
 ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ?cib = bcma_erom_get_ci(bus, &eromptr);
- ? ? ? ? ? ? ? if (cib < 0) {
- ? ? ? ? ? ? ? ? ? ? ? err= -EILSEQ;
- ? ? ? ? ? ? ? ? ? ? ? goto out;
- ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? if (cib < 0)
+ ? ? ? ? ? ? ? ? ? ? ? return -EILSEQ;
 

? ? ? ? ? ? ? ?/* parse CIs */
- ? ? ? ? ? ? ? core->id.class = (cia & SCAN_CIA_CLASS) >> SCAN_CIA_CLASS_SHIFT;
- ? ? ? ? ? ? ? core->id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
- ? ? ? ? ? ? ? core->id.manuf = (cia & SCAN_CIA_MANUF) >> SCAN_CIA_MANUF_SHIFT;
+ ? ? ? ? ? ? ? core.id.class = (cia & SCAN_CIA_CLASS) >> SCAN_CIA_CLASS_SHIFT;
+ ? ? ? ? ? ? ? core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
+ ? ? ? ? ? ? ? core.id.manuf = (cia & SCAN_CIA_MANUF) >> SCAN_CIA_MANUF_SHIFT; ? ? ? ? ? ? ? ?ports[0] = (cib & SCAN_CIB_NMP) >> SCAN_CIB_NMP_SHIFT; ? ? ? ? ? ? ? ?ports[1] = (cib & SCAN_CIB_NSP) >> SCAN_CIB_NSP_SHIFT; ? ? ? ? ? ? ? ?wrappers[0] = (cib & SCAN_CIB_NMW) >> SCAN_CIB_NMW_SHIFT; ? ? ? ? ? ? ? ?wrappers[1] = (cib & SCAN_CIB_NSW) >> SCAN_CIB_NSW_SHIFT;
- ? ? ? ? ? ? ? core->id.rev = (cib & SCAN_CIB_REV) >> SCAN_CIB_REV_SHIFT;
+ ? ? ? ? ? ? ? core.id.rev = (cib & SCAN_CIB_REV) >> SCAN_CIB_REV_SHIFT;

- ? ? ? ? ? ? ? if (((core->id.manuf == BCMA_MANUF_ARM) &&
- ? ? ? ? ? ? ? ? ? ?(core->id.id == 0xFFF)) ||
+ ? ? ? ? ? ? ? if (((core.id.manuf == BCMA_MANUF_ARM) &&
+ ? ? ? ? ? ? ? ? ? ?(core.id.id == 0xFFF)) ||
? ? ? ? ? ? ? ? ? ?(ports[1] == 0)) {
? ? ? ? ? ? ? ? ? ? ? ?bcma_erom_skip_component(bus, &eromptr); ? ? ? ? ? ? ? ? ? ? ? ?continue;

drivers/bcma/scan.c +0 -0

@@ -285,10 +276,8 @@ int bcma_bus_scan(struct bcma_bus *bus)

? ? ? ? ? ? ? ?/* get & parse master ports */
? ? ? ? ? ? ? ?for (i = 0; i < ports[0]; i++) {
? ? ? ? ? ? ? ? ? ? ? ?u32 mst_port_d = bcma_erom_get_mst_port(bus, &eromptr);
- ? ? ? ? ? ? ? ? ? ? ? if (mst_port_d < 0) {
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? err= -EILSEQ;
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto out;
- ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? ? if (mst_port_d < 0)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return -EILSEQ;
? ? ? ? ? ? ? ?}

? ? ? ? ? ? ? ?/* get & parse slave ports */

drivers/bcma/scan.c +3 -3

@@ -303,7 +292,7 @@ int bcma_bus_scan(struct bcma_bus *bus)
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?} else {
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (i == 0 && j == 0)
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? core->addr = tmp;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? core.addr = tmp;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ?}

@@ -320,7 +309,7 @@ int bcma_bus_scan(struct bcma_bus *bus)
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?} else {
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (i == 0 && j == 0)
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? core->wrap = tmp;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? core.wrap = tmp;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ?}

@@ -338,22 +327,19 @@ int bcma_bus_scan(struct bcma_bus *bus)
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?break;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?} else {
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?if (wrappers[0] == 0 && !i && !j)
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? core->wrap = tmp;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? core.wrap = tmp;
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ? ? ? ? ?}
 ? ? ? ? ? ? ? ?}
 

? ? ? ? ? ? ? ?pr_info("Core %d found: %s "
? ? ? ? ? ? ? ? ? ? ? ?"(manuf 0x%03X, id 0x%03X, rev 0x%02X, class 0x%X)\n",
- ? ? ? ? ? ? ? ? ? ? ? bus->nr_cores, bcma_device_name(&core->id),
- ? ? ? ? ? ? ? ? ? ? ? core->id.manuf, core->id.id, core->id.rev,
- ? ? ? ? ? ? ? ? ? ? ? core->id.class);

drivers/bcma/scan.c +6 -6

-
- ? ? ? ? ? ? ? core->core_index = bus->nr_cores++;
- ? ? ? ? ? ? ? list_add(&core->list, &bus->cores);
- ? ? ? ? ? ? ? continue;
-out:
- ? ? ? ? ? ? ? return err;
+ ? ? ? ? ? ? ? ? ? ? ? bus->nr_cores, bcma_device_name(&core.id),
+ ? ? ? ? ? ? ? ? ? ? ? core.id.manuf, core.id.id, core.id.rev,
+ ? ? ? ? ? ? ? ? ? ? ? core.id.class);
+
+ ? ? ? ? ? ? ? core.core_index = bus->nr_cores;
+ ? ? ? ? ? ? ? bus->cores[bus->nr_cores++] = core;

? ? ? ?}

? ? ? ?return 0;

include/linux/bcma/bcma.h +0 -0

index 27a27a7..3dc5302 100644
@@ -118,14 +118,23 @@ struct bcma_host_ops {
 

?#define BCMA_MAX_NR_CORES ? ? ? ? ? ? ?16

include/linux/bcma/bcma.h +10 -0

+/* 1) It is not allowed to put struct device statically in bcma_device
+ * 2) We can not just use pointer to struct device because we use container_of
+ * 3) We do not have pointer to struct bcma_device in struct device
+ * Solution: use such a dummy wrapper
+ */
+struct __bcma_dev_wrapper {
+ ? ? ? struct device dev;
+ ? ? ? struct bcma_device *core;
+};
+

?struct bcma_device {
? ? ? ?struct bcma_bus *bus;
? ? ? ?struct bcma_device_id id;

- ? ? ? struct device dev;
+ ? ? ? struct device *dev;
? ? ? ?struct device *dma_dev;
? ? ? ?unsigned int irq;
- ? ? ? bool dev_registered;

? ? ? ?u8 core_index;

include/linux/bcma/bcma.h +1 -3

@@ -133,7 +142,6 @@ struct bcma_device {
 ? ? ? ?u32 wrap;
 
 ? ? ? ?void *drvdata;
- ? ? ? struct list_head list;
 ?};
 
 ?static inline void *bcma_get_drvdata(struct bcma_device *core)

@@ -182,7 +190,7 @@ struct bcma_bus {
 ? ? ? ?struct bcma_chipinfo chipinfo;
 
 ? ? ? ?struct bcma_device *mapped_core;
- ? ? ? struct list_head cores;
+ ? ? ? struct bcma_device cores[BCMA_MAX_NR_CORES];
 ? ? ? ?u8 nr_cores;
 
 ? ? ? ?struct bcma_drv_cc drv_cc;
--

1.7.4.1

Greg, Arnd: could you take a look at this patch, please?

With proposed patch we are going back to this ugly array and wrappers hacks.

I was really happy with our final solution, but it seems it's not doable for embedded systems...? Is there something better we can do about this?

Greg, Arnd: could you take a look at this patch, please?

With proposed patch we are going back to this ugly array and wrappers hacks.

I was really happy with our final solution, but it seems it's not doable for embedded systems...? Is there something better we can do about this?

2011/6/6 Hauke Mehrtens <[email protected]>:

Quoted 12 lines

When using bcma on a embedded device it is initialized very early at boot. We have to do so as the cpu and interrupt management and all other devices are attached to this bus and it has to be initialized so early. In that stage we can not allocate memory or sleep, just use the memory on the stack and in the text segment as the kernel is not initialized far enough. This patch removed the kzallocs from the scan code. Some earlier version of the bcma implementation and the normal ssb implementation are doing it like this.
The __bcma_dev_wrapper struct is used as the container for the device struct as bcma_device will be too big if it includes struct device.

Signed-off-by Hauke Mehrtens <[email protected]>

Quoted 21 lines

Greg, Arnd: could you take a look at this patch, please?

With proposed patch we are going back to this ugly array and wrappers hacks.

I was really happy with our final solution, but it seems it's not doable for embedded systems...? Is there something better we can do about this?

2011/6/6 Hauke Mehrtens <[email protected]>:

Quoted 12 lines

When using bcma on a embedded device it is initialized very early at boot. We have to do so as the cpu and interrupt management and all other devices are attached to this bus and it has to be initialized so early. In that stage we can not allocate memory or sleep, just use the memory on the stack and in the text segment as the kernel is not initialized far enough. This patch removed the kzallocs from the scan code. Some earlier version of the bcma implementation and the normal ssb implementation are doing it like this.
The __bcma_dev_wrapper struct is used as the container for the device struct as bcma_device will be too big if it includes struct device.

Signed-off-by Hauke Mehrtens <[email protected]>

If you rely on device scan to find your CPUs and interrupt controllers, you are screwed already, this won't work.

In that case, it's better to have a few "early" drivers, as few as possible, that don't go through the bus scan at all but have their own ways of bootstrapping themselves. I don't know what you mean by "CPU management", but I can only assume that it's not doing that much, and you can just put the register values into the device tree.

For an interrupt controller, it should be ok to have it initialized late, as long as it's only responsible for the devices on the same bus and not for instance for IPI interrupts. Just make sure that you do the bus scan and the initialization of the IRQ driver before you initialize any drivers that rely in on the interrupts to be working.

Quoted 5 lines

For an interrupt controller, it should be ok to have it initialized late, as long as it's only responsible for the devices on the same bus and not for instance for IPI interrupts. Just make sure that you do the bus scan and the initialization of the IRQ driver before you initialize any drivers that rely in on the interrupts to be working.

Without proper timer init (which requires both the chipcommon and mips cores knowledge) kernel will get hung somewhere inside calibrate_delay. It could get addressed if get bus scan called in arch_init_irq or plat_time_init - both are executed before calibrate_delay and with slab available.

Quoted 11 lines · George Kashperko

Quoted 5 lines

For an interrupt controller, it should be ok to have it initialized late, as long as it's only responsible for the devices on the same bus and not for instance for IPI interrupts. Just make sure that you do the bus scan and the initialization of the IRQ driver before you initialize any drivers that rely in on the interrupts to be working.

Without proper timer init (which requires both the chipcommon and mips cores knowledge) kernel will get hung somewhere inside calibrate_delay. It could get addressed if get bus scan called in arch_init_irq or plat_time_init - both are executed before calibrate_delay and with slab available.

Ok, so you need the interrupt controller to be working for the timer tick, right? I think another option (if that's not what you mean already) would be to have a simpler way to find a device on the bus that can be called before doing a full scan.

Early drivers would then have to know what is there and call a function like "bcma_find_device(BCMA_DEV_ID_IRQ)", while drivers that are not required to be up just register a regular device driver with a probe function that gets called after the bus scan creates device structures.

Arnd

Accessing chip common should be possible without scanning the hole bus as it is at the first position and initializing most things just needs chip common. For initializing the interrupts scanning is needed as we do not know where the mips core is located.

As we can not use kalloc on early boot we could use a function which uses kalloc under normal conditions and when on early boot the architecture code which starts the bcma code should also provide a function which returns a pointer to some memory in its text segment to use. We need space for 16 cores in the architecture code.

In addition bcma_bus_register(struct bcma_bus *bus) has to be divided into two parts. The first part will scan the bus and initialize chip common and mips core. The second part will initialize pci core and register the devices in the system. When using this under normal conditions they will be called directly after each other.

Quoted 11 lines · On Monday 06 June 2011 23:38:50 Hauke Mehrtens wrote:

Accessing chip common should be possible without scanning the hole bus as it is at the first position and initializing most things just needs chip common. For initializing the interrupts scanning is needed as we do not know where the mips core is located.

As we can not use kalloc on early boot we could use a function which uses kalloc under normal conditions and when on early boot the architecture code which starts the bcma code should also provide a function which returns a pointer to some memory in its text segment to use. We need space for 16 cores in the architecture code.

Quoted 5 lines

In addition bcma_bus_register(struct bcma_bus *bus) has to be divided into two parts. The first part will scan the bus and initialize chip common and mips core. The second part will initialize pci core and register the devices in the system. When using this under normal conditions they will be called directly after each other.

Just split out the minimal low-level function from the bcma_bus_scan then, to locate a single device based on some identifier. The bcma_bus_scan() function can then repeatedly allocate one device and pass it to the low-level function when doing the proper scan, while the arch code calls the low-level function directly with static data.

Quoted 17 lines · Arnd Bergmann

Quoted 11 lines · On Monday 06 June 2011 23:38:50 Hauke Mehrtens wrote:

Accessing chip common should be possible without scanning the hole bus as it is at the first position and initializing most things just needs chip common. For initializing the interrupts scanning is needed as we do not know where the mips core is located.

As we can not use kalloc on early boot we could use a function which uses kalloc under normal conditions and when on early boot the architecture code which starts the bcma code should also provide a function which returns a pointer to some memory in its text segment to use. We need space for 16 cores in the architecture code.

Quoted 5 lines

In addition bcma_bus_register(struct bcma_bus *bus) has to be divided into two parts. The first part will scan the bus and initialize chip common and mips core. The second part will initialize pci core and register the devices in the system. When using this under normal conditions they will be called directly after each other.

Quoted 6 lines

Just split out the minimal low-level function from the bcma_bus_scan then, to locate a single device based on some identifier. The bcma_bus_scan() function can then repeatedly allocate one device and pass it to the low-level function when doing the proper scan, while the arch code calls the low-level function directly with static data.

If going for this we should pass struct bcma_device_id as match parameter as that identifies the core appropriately although you probably only want to match manufacturer and core identifiers.

Gr. AvS

Quoted 25 lines · Arnd Bergmann

Quoted 23 lines · On Monday 06 June 2011 23:38:50 Hauke Mehrtens wrote:

Quoted 11 lines

Accessing chip common should be possible without scanning the hole bus as it is at the first position and initializing most things just needs chip common. For initializing the interrupts scanning is needed as we do not know where the mips core is located.

As we can not use kalloc on early boot we could use a function which uses kalloc under normal conditions and when on early boot the architecture code which starts the bcma code should also provide a function which returns a pointer to some memory in its text segment to use. We need space for 16 cores in the architecture code.

Quoted 5 lines

In addition bcma_bus_register(struct bcma_bus *bus) has to be divided into two parts. The first part will scan the bus and initialize chip common and mips core. The second part will initialize pci core and register the devices in the system. When using this under normal conditions they will be called directly after each other.

Just split out the minimal low-level function from the bcma_bus_scan then, to locate a single device based on some identifier. The bcma_bus_scan() function can then repeatedly allocate one device and pass it to the low-level function when doing the proper scan, while the arch code calls the low-level function directly with static data.

Quoted 6 lines

If going for this we should pass struct bcma_device_id as match parameter as that identifies the core appropriately although you probably only want to match manufacturer and core identifiers.

Gr. AvS

What is the problem with scanning the full bus?

A special scan function would just skip the wrong cores so I do not see any advantage in that.

We could build a scan function which searches for one core and uses a struct bcma_core stored on the stack and returns the struct bcma_core if it found the wanted one.

Then we could search for chipcommon and mips
and store then in arch code in arch/mips/bcm47xx and use them.

When boot
is ready and we are searching the complete bus there is probably something differences in the init process from normal init as we already initialized chipcommon sometime earlier.

I Would prefer to scan the bus
completely and initialize chipcommon and mips in early boot.

Because full scanning needs one of the following:
1) Working alloc - not possible for SoCs

Quoted 2 lines · On Wed, 8 Jun 2011 02:06:11 +0200 Rafa? Mi?ecki <[email protected]> wrote:

Because full scanning needs one of the following:
1) Working alloc - not possible for SoCs

Isn't there a bootmem allocator available on MIPS?

This patch adds support for using bcma on an embedded bus. An embedded system like the bcm4716 could register this bus and it searches for the bcma cores then.

Signed-off-by Hauke Mehrtens <[email protected]>

---

drivers/bcma/scan.c +9 -0

index 70b39f7..9229615 100644
@@ -219,9 +219,34 @@ int bcma_bus_scan(struct bcma_bus *bus)
        bus->chipinfo.id = (tmp & BCMA_CC_ID_ID) >> BCMA_CC_ID_ID_SHIFT;
        bus->chipinfo.rev = (tmp & BCMA_CC_ID_REV) >> BCMA_CC_ID_REV_SHIFT;
        bus->chipinfo.pkg = (tmp & BCMA_CC_ID_PKG) >> BCMA_CC_ID_PKG_SHIFT;
+       bus->nr_cores = (tmp & BCMA_CC_ID_NRCORES) >> BCMA_CC_ID_NRCORES_SHIFT;
+
+       /* If we are an embedded device we now know the number of avaliable
+        * core and ioremap the correct space.
+        */
+       if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
+               iounmap(bus->mmio);
+               mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+               if (!mmio)

Hauke,

Minor nit:

Quoted 29 lines · Hauke Mehrtens

This patch adds support for using bcma on an embedded bus. An embedded system like the bcm4716 could register this bus and it searches for the bcma cores then.

Signed-off-by Hauke Mehrtens <[email protected]>

---

drivers/bcma/scan.c +15 -0

index 70b39f7..9229615 100644
@@ -219,9 +219,34 @@ int bcma_bus_scan(struct bcma_bus *bus)
       bus->chipinfo.id = (tmp & BCMA_CC_ID_ID) >> BCMA_CC_ID_ID_SHIFT;
       bus->chipinfo.rev = (tmp & BCMA_CC_ID_REV) >> BCMA_CC_ID_REV_SHIFT;
       bus->chipinfo.pkg = (tmp & BCMA_CC_ID_PKG) >> BCMA_CC_ID_PKG_SHIFT;
+       bus->nr_cores = (tmp & BCMA_CC_ID_NRCORES) >> BCMA_CC_ID_NRCORES_SHIFT;
+
+       /* If we are an embedded device we now know the number of avaliable
+        * core and ioremap the correct space.
+        */
+       if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
+               iounmap(bus->mmio);
+               mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+               if (!mmio)
+                       return -ENOMEM;
+               bus->mmio = mmio;
+
+               mmio = ioremap(BCMA_WRAP_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+               if (!mmio)
+                       return -ENOMEM;

drivers/bcma/scan.c int bcma_bus_scan(struct bcma_bus *bus) · line 240

Quoted 4 lines

drivers/bcma/scan.c +4 -0

+               bus->host_embedded = mmio;
+       }
+       /* reset it to 0 as we use it for counting */
+       bus->nr_cores = 0;

Would it make sense to use a local variable for nr_cores, and only use it within the BCMA_HOSTTYPE_EMBEDDED if statement, rather than re-using bus->nr_cores and having to reset it?

drivers/bcma/host_embedded.c new file +7 -0

+ ? ? ? /* Host specific */
+ ? ? ? bus->hosttype = BCMA_HOSTTYPE_EMBEDDED;
+ ? ? ? bus->ops = &bcma_host_bcma_ops;
+
+ ? ? ? /* Register */
+ ? ? ? return bcma_bus_register(bus);
+}

drivers/bcma/main.c +0 -0

index 1afa107..c5bcb5f 100644
@@ -119,6 +119,7 @@ static int bcma_register_cores(struct bcma_bus *bus)
 ? ? ? ? ? ? ? ? ? ? ? ?break;

? ? ? ? ? ? ? ?case BCMA_HOSTTYPE_NONE:
? ? ? ? ? ? ? ?case BCMA_HOSTTYPE_SDIO:
+ ? ? ? ? ? ? ? case BCMA_HOSTTYPE_EMBEDDED:
? ? ? ? ? ? ? ? ? ? ? ?break;
? ? ? ? ? ? ? ?}

drivers/bcma/scan.c +1 -1

index 70b39f7..9229615 100644
@@ -203,7 +203,7 @@ static s32 bcma_erom_get_addr_desc(struct bcma_bus *bus, u32 **eromptr,
 ?int bcma_bus_scan(struct bcma_bus *bus)
 ?{
 ? ? ? ?u32 erombase;
- ? ? ? u32 __iomem *eromptr, *eromend;
+ ? ? ? u32 __iomem *eromptr, *eromend, *mmio;
 
 ? ? ? ?s32 cia, cib;
 ? ? ? ?u8 ports[2], wrappers[2];

Hauke,

My idea for naming schema was to use:
bcma_host_TYPE_*

Like:
bcma_host_pci_*
bcma_host_sdio_*

You are using:
bcma_host_bcma_*

What do you think about changing this to:
bcma_host_embedded_*
or just some:
bcma_host_emb_*
?

Does it make more sense to you? I was trying to keep names in bcma really clear, so every first-time-reader can see differences between hosts, host and driver, etc.

2011/6/6 Hauke Mehrtens <[email protected]>:

drivers/bcma/host_embedded.c line 3

Quoted 6 lines

drivers/bcma/host_embedded.c new file +3 -0

@@ -0,0 +1,93 @@
+/*
+ * Broadcom specific AMBA
+ * PCI Host

s/PCI/Embedded/

drivers/bcma/host_embedded.c line 85

Quoted 10 lines

drivers/bcma/host_embedded.c new file +10 -0

+int bcma_host_bcma_register(struct bcma_bus *bus)
+{
+       u32 __iomem *mmio;
+       /* iomap only first core. We have to read some register on this core
+        * to get the number of cores. This is sone in bcma_scan()
+        */
+       mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * 1);
+       if (!mmio)
+               return -ENOMEM;
+       bus->mmio = mmio;

Maybe just:
bus->mmio = ioremap(...);
? :)

Quoted 30 lines

drivers/bcma/host_embedded.c new file +7 -0

+       /* Host specific */
+       bus->hosttype = BCMA_HOSTTYPE_EMBEDDED;
+       bus->ops = &bcma_host_bcma_ops;
+
+       /* Register */
+       return bcma_bus_register(bus);
+}

drivers/bcma/main.c +1 -0

index 1afa107..c5bcb5f 100644
@@ -119,6 +119,7 @@ static int bcma_register_cores(struct bcma_bus *bus)
                       break;
               case BCMA_HOSTTYPE_NONE:
               case BCMA_HOSTTYPE_SDIO:
+               case BCMA_HOSTTYPE_EMBEDDED:
                       break;
               }
 

drivers/bcma/scan.c +1 -1

index 70b39f7..9229615 100644
@@ -203,7 +203,7 @@ static s32 bcma_erom_get_addr_desc(struct bcma_bus *bus, u32 **eromptr,
 int bcma_bus_scan(struct bcma_bus *bus)
 {
       u32 erombase;
-       u32 __iomem *eromptr, *eromend;
+       u32 __iomem *eromptr, *eromend, *mmio;
 

drivers/bcma/scan.c int bcma_bus_scan(struct bcma_bus *bus) · line 222

Quoted 7 lines

drivers/bcma/scan.c +1 -0

       s32 cia, cib;
       u8 ports[2], wrappers[2];

@@ -219,9 +219,34 @@ int bcma_bus_scan(struct bcma_bus *bus)
       bus->chipinfo.id = (tmp & BCMA_CC_ID_ID) >> BCMA_CC_ID_ID_SHIFT;
       bus->chipinfo.rev = (tmp & BCMA_CC_ID_REV) >> BCMA_CC_ID_REV_SHIFT;
       bus->chipinfo.pkg = (tmp & BCMA_CC_ID_PKG) >> BCMA_CC_ID_PKG_SHIFT;
+       bus->nr_cores = (tmp & BCMA_CC_ID_NRCORES) >> BCMA_CC_ID_NRCORES_SHIFT;

Hi,

Quoted 18 lines

Hauke,

My idea for naming schema was to use:
bcma_host_TYPE_*

Like:
bcma_host_pci_*
bcma_host_sdio_*

You are using:
bcma_host_bcma_*

What do you think about changing this to:
bcma_host_embedded_*
or just some:
bcma_host_emb_*
?

Quoted 3 lines

Does it make more sense to you? I was trying to keep names in bcma really clear, so every first-time-reader can see differences between hosts, host and driver, etc.

how about bcma_host_soc ?

W dniu 6 czerwca 2011 12:32 użytkownik George Kashperko

Quoted 20 lines · <[email protected]> napisał:

Hi,

Quoted 18 lines

Hauke,

My idea for naming schema was to use:
bcma_host_TYPE_*

Like:
bcma_host_pci_*
bcma_host_sdio_*

You are using:
bcma_host_bcma_*

What do you think about changing this to:
bcma_host_embedded_*
or just some:
bcma_host_emb_*
?

Quoted 4 lines

Quoted 3 lines

Does it make more sense to you? I was trying to keep names in bcma really clear, so every first-time-reader can see differences between hosts, host and driver, etc.

how about bcma_host_soc ?

We get then inconsistency with "BCMA_HOSTTYPE_EMBEDDED". I'd like to
1) See something like bcma_host_emb...
xor
2) Use bcma_host_soc_* and BCMA_HOSTTYPE_SOC

Quoted 28 lines

W dniu 6 czerwca 2011 12:32 u?ytkownik George Kashperko <[email protected]> ?napisa?:

Quoted 25 lines

Hi,

Quoted 21 lines

Hauke,

My idea for naming schema was to use:
bcma_host_TYPE_*

Like:
bcma_host_pci_*
bcma_host_sdio_*

You are using:
bcma_host_bcma_*

What do you think about changing this to:
bcma_host_embedded_*
or just some:
bcma_host_emb_*
?

Does it make more sense to you? I was trying to keep names in bcma really clear, so every first-time-reader can see differences between hosts, host and driver, etc.

Quoted 7 lines

Quoted 1 line

how about bcma_host_soc ?

We get then inconsistency with "BCMA_HOSTTYPE_EMBEDDED". I'd like to
1) See something like bcma_host_emb...
xor
2) Use bcma_host_soc_* and BCMA_HOSTTYPE_SOC

I would go for option 2). It more clearly says what it is. Embedded is a broader term. As an example, a handset is an embedded device, but it may use BCMA_HOSTTYPE_SDIO.

Hauke,

My idea for naming schema was to use:
bcma_host_TYPE_*

Like:
bcma_host_pci_*
bcma_host_sdio_*

You are using:
bcma_host_bcma_*

What do you think about changing this to:
bcma_host_embedded_*
or just some:
bcma_host_emb_*
?

Does it make more sense to you? I was trying to keep names in bcma really clear, so every first-time-reader can see differences between hosts, host and driver, etc.

2011/6/6 Hauke Mehrtens <[email protected]>:

drivers/bcma/host_embedded.c line 3

Quoted 6 lines

drivers/bcma/host_embedded.c new file +3 -0

@@ -0,0 +1,93 @@
+/*
+ * Broadcom specific AMBA
+ * PCI Host

s/PCI/Embedded/

drivers/bcma/host_embedded.c line 85

Quoted 10 lines

drivers/bcma/host_embedded.c new file +10 -0

+int bcma_host_bcma_register(struct bcma_bus *bus)
+{
+       u32 __iomem *mmio;
+       /* iomap only first core. We have to read some register on this core
+        * to get the number of cores. This is sone in bcma_scan()
+        */
+       mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * 1);
+       if (!mmio)
+               return -ENOMEM;
+       bus->mmio = mmio;

Maybe just:
bus->mmio = ioremap(...);
? :)

Quoted 31 lines

drivers/bcma/host_embedded.c new file +7 -0

+       /* Host specific */
+       bus->hosttype = BCMA_HOSTTYPE_EMBEDDED;
+       bus->ops = &bcma_host_bcma_ops;
+
+       /* Register */
+       return bcma_bus_register(bus);
+}

drivers/bcma/main.c +1 -0

index 1afa107..c5bcb5f 100644
@@ -119,6 +119,7 @@ static int bcma_register_cores(struct bcma_bus *bus)
                       break;
               case BCMA_HOSTTYPE_NONE:
               case BCMA_HOSTTYPE_SDIO:
+               case BCMA_HOSTTYPE_EMBEDDED:
                       break;
               }
 

drivers/bcma/scan.c +1 -1

index 70b39f7..9229615 100644
@@ -203,7 +203,7 @@ static s32 bcma_erom_get_addr_desc(struct bcma_bus *bus, u32 **eromptr,
 int bcma_bus_scan(struct bcma_bus *bus)
 {
       u32 erombase;
-       u32 __iomem *eromptr, *eromend;
+       u32 __iomem *eromptr, *eromend, *mmio;
 

drivers/bcma/scan.c int bcma_bus_scan(struct bcma_bus *bus) · line 222

Quoted 7 lines

drivers/bcma/scan.c +1 -0

       s32 cia, cib;
       u8 ports[2], wrappers[2];

@@ -219,9 +219,34 @@ int bcma_bus_scan(struct bcma_bus *bus)
       bus->chipinfo.id = (tmp & BCMA_CC_ID_ID) >> BCMA_CC_ID_ID_SHIFT;
       bus->chipinfo.rev = (tmp & BCMA_CC_ID_REV) >> BCMA_CC_ID_REV_SHIFT;
       bus->chipinfo.pkg = (tmp & BCMA_CC_ID_PKG) >> BCMA_CC_ID_PKG_SHIFT;
+       bus->nr_cores = (tmp & BCMA_CC_ID_NRCORES) >> BCMA_CC_ID_NRCORES_SHIFT;

I'd use different variable as Julian suggested.

drivers/bcma/scan.c int bcma_bus_scan(struct bcma_bus *bus) · line 226

Quoted 6 lines

drivers/bcma/scan.c +4 -0

+
+       /* If we are an embedded device we now know the number of avaliable
+        * core and ioremap the correct space.
+        */

Typo: avaliable

drivers/bcma/scan.c int bcma_bus_scan(struct bcma_bus *bus) · line 237

Quoted 13 lines

drivers/bcma/scan.c +11 -0

+       if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
+               iounmap(bus->mmio);
+               mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+               if (!mmio)
+                       return -ENOMEM;
+               bus->mmio = mmio;
+
+               mmio = ioremap(BCMA_WRAP_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+               if (!mmio)
+                       return -ENOMEM;
+               bus->host_embedded = mmio;

Do we really need both? mmio and host_embedded? What about keeping mmio only and using it in calculation for read/write[8,16,32]?

drivers/bcma/scan.c int bcma_bus_scan(struct bcma_bus *bus) · line 248

Quoted 13 lines

drivers/bcma/scan.c +9 -1

+       }
+       /* reset it to 0 as we use it for counting */
+       bus->nr_cores = 0;
 
       erombase = bcma_scan_read32(bus, 0, BCMA_CC_EROM);
-       eromptr = bus->mmio;
+       if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
+               eromptr = ioremap(erombase, BCMA_CORE_SIZE);
+               if (!eromptr)
+                       return -ENOMEM;
+       } else
+               eromptr = bus->mmio;

I though eromptr = bus->mmio; will do the trick for embedded as well. I think I need some time to read about IO mapping and understand that.

Quoted 12 lines

Quoted 11 lines

+ ? ? ? if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
+ ? ? ? ? ? ? ? iounmap(bus->mmio);
+ ? ? ? ? ? ? ? mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+ ? ? ? ? ? ? ? if (!mmio)
+ ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
+ ? ? ? ? ? ? ? bus->mmio = mmio;
+
+ ? ? ? ? ? ? ? mmio = ioremap(BCMA_WRAP_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+ ? ? ? ? ? ? ? if (!mmio)
+ ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
+ ? ? ? ? ? ? ? bus->host_embedded = mmio;

Quoted 2 lines

Do we really need both? mmio and host_embedded? What about keeping mmio only and using it in calculation for read/write[8,16,32]?

These are two different memory regions, it should be possible to calculate the other address, but I do not like that. As host_embedded is in a union this does not waste any memory.

W dniu 7 czerwca 2011 00:00 użytkownik Hauke Mehrtens

Quoted 12 lines · <[email protected]> napisał:

Quoted 11 lines · Rafał Miłecki

Quoted 11 lines

drivers/bcma/scan.c +11 -0

+       if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
+               iounmap(bus->mmio);
+               mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+               if (!mmio)
+                       return -ENOMEM;
+               bus->mmio = mmio;
+
+               mmio = ioremap(BCMA_WRAP_BASE, BCMA_CORE_SIZE * bus->nr_cores);
+               if (!mmio)
+                       return -ENOMEM;
+               bus->host_embedded = mmio;

Quoted 5 lines

Quoted 2 lines

Do we really need both? mmio and host_embedded? What about keeping mmio only and using it in calculation for read/write[8,16,32]?

These are two different memory regions, it should be possible to calculate the other address, but I do not like that. As host_embedded is in a union this does not waste any memory.

Ah, OK, I can see what does happen here. You are using:
1) bus->mmio for first core
2) bus->host_embedded for first agent/wrapper

I'm not sure if this is a correct approach. Doing "core_index * BCMA_CORE_SIZE" comes from ssb, where it was the way to calculate offset. In case of BCMA we are reading all the info from (E)EPROM, which also includes addresses of the cores.

IMO you should use core->addr and core->wrap for read/write ops. I believe this is approach Broadcom decided to use for BCMA, when designing (E)EPROM.

Quoted 25 lines · Rafa? Mi?ecki

W dniu 7 czerwca 2011 00:00 u?ytkownik Hauke Mehrtens <[email protected]> napisa?:

Quoted 14 lines · Rafa? Mi?ecki

Quoted 13 lines

Quoted 13 lines

drivers/bcma/scan.c +3 -0

+       if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
+               iounmap(bus->mmio);
+               mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE *

bus->nr_cores);

drivers/bcma/host_embedded.c new file +5 -0

+               if (!mmio)
+                       return -ENOMEM;
+               bus->mmio = mmio;
+
+               mmio = ioremap(BCMA_WRAP_BASE, BCMA_CORE_SIZE *

bus->nr_cores);

drivers/bcma/scan.c +3 -0

+               if (!mmio)
+                       return -ENOMEM;
+               bus->host_embedded = mmio;

Quoted 5 lines

Quoted 2 lines

Do we really need both? mmio and host_embedded? What about keeping mmio only and using it in calculation for read/write[8,16,32]?

These are two different memory regions, it should be possible to calculate the other address, but I do not like that. As host_embedded is in a union this does not waste any memory.

Ah, OK, I can see what does happen here. You are using:
1) bus->mmio for first core
2) bus->host_embedded for first agent/wrapper

Quoted 8 lines

I'm not sure if this is a correct approach. Doing "core_index * BCMA_CORE_SIZE" comes from ssb, where it was the way to calculate offset. In case of BCMA we are reading all the info from (E)EPROM, which also includes addresses of the cores.

IMO you should use core->addr and core->wrap for read/write ops. I believe this is approach Broadcom decided to use for BCMA, when designing (E)EPROM.

Agree. There is no guarantee for the core index to relate to the physical address. Chip designer may be systematic in this and the index*size method may work, but not by design.

Gr. AvS

+/* Get the MIPS IRQ assignment for a specified device.
+ * If unassigned, 0 is returned.
+ * If disabled, 5 is returned.
+ * If not supported, 6 is returned.
+ */

+ ? ? ? ? ? ? ? ? ? ? ? for (i = 0; i < bus->nr_cores; i++)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if ((1 << bcma_core_mips_irqflag(&bus->cores[i])) == oldirqflag) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bcma_core_mips_set_irq(&bus->cores[i], 0);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }

+ ? ? ? pr_info("after irq reconfiguration\n");

2011/6/6 Hauke Mehrtens <[email protected]>:

drivers/bcma/bcma_private.h extern void bcma_bus_unregister(struct bcma_bus *bus); · line 23

Quoted 2 lines

drivers/bcma/bcma_private.h +2 -0

+/* driver_mips.c */
+extern unsigned int bcma_core_mips_irq(struct bcma_device *dev);

Does it compile without CONFIG_BCMA_DRIVER_MIPS?

Quoted 7 lines

+/* Get the MIPS IRQ assignment for a specified device.
+ * If unassigned, 0 is returned.
+ * If disabled, 5 is returned.
+ * If not supported, 6 is returned.
+ */

Does it ever return 6?

drivers/bcma/driver_mips.c line 84

Quoted 12 lines

drivers/bcma/driver_mips.c new file +11 -0

+unsigned int bcma_core_mips_irq(struct bcma_device *dev)
+{
+       struct bcma_device *mdev = dev->bus->drv_mips.core;
+       u32 irqflag;
+       unsigned int irq;
+
+       irqflag = bcma_core_mips_irqflag(dev);
+
+       for (irq = 1; irq <= 4; irq++)
+               if (bcma_read32(mdev, BCMA_MIPS_MIPS74K_INTMASK(irq)) & (1 << irqflag))
+                       break;

Use scripts/checkpatch*. Braces around "for" and split line to match 80 chars width.

Why don't you just use "return irq;" instead of break?

drivers/bcma/driver_mips.c line 89

Quoted 7 lines

drivers/bcma/driver_mips.c new file +5 -0

+
+       if (irq == 5)
+               irq = 0;
+
+       return irq;

You can just make it "return 0;" after changing break to return.

drivers/bcma/driver_mips.c line 129

Quoted 7 lines

drivers/bcma/driver_mips.c new file +5 -0

+                       for (i = 0; i < bus->nr_cores; i++)
+                               if ((1 << bcma_core_mips_irqflag(&bus->cores[i])) == oldirqflag) {
+                                       bcma_core_mips_set_irq(&bus->cores[i], 0);
+                                       break;
+                               }

Braces for "for".

Quoted 2 lines

+ pr_info("after irq reconfiguration\n");

Make first letter uppercase. I'm not English expert, but doesn't something like "IRQ reconfiguration done" sound better?

Quoted 7 lines

Quoted 5 lines

+ ? ? ? ? ? ? ? ? ? ? ? for (i = 0; i < bus->nr_cores; i++)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if ((1 << bcma_core_mips_irqflag(&bus->cores[i])) == oldirqflag) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bcma_core_mips_set_irq(&bus->cores[i], 0);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }

Braces for "for".

Is this needed after the coding guildlines? Shouldn't they be removed if they are not needed?

+ ? ? ? if ((ccrev >= 11) && (ccrev != 15) && (ccrev != 20)) {
+ ? ? ? ....
+ ? ? ? } else
+ ? ? ? ? ? ? ? pr_err("serial not supported on this device ccrev: 0x%x\n",
+ ? ? ? ? ? ? ? ? ? ? ?ccrev);

2011/6/6 Hauke Mehrtens <[email protected]>:

drivers/bcma/driver_mips.c static void bcma_core_mips_dump_irq(struct bcma_bus *bus) · line 168

Quoted 11 lines

drivers/bcma/driver_mips.c +11 -0

+u32 bcma_cpu_clock(struct bcma_drv_mips *mcore)
+{
+ ? ? ? struct bcma_bus *bus = mcore->core->bus;
+
+ ? ? ? if (bus->drv_cc.capabilities & BCMA_CC_CAP_PMU)
+ ? ? ? ? ? ? ? return bcma_pmu_get_clockcpu(&bus->drv_cc);
+
+ ? ? ? pr_err("No PMU available, need this to get the cpu clock\n");
+ ? ? ? return 0;
+}
+EXPORT_SYMBOL(bcma_cpu_clock);

Are you really going to use this in some separated driver? If you're, I heard that exporting symbol should go in pair with patch enabling usage of such a symbol.

+config BCMA_PCICORE_HOSTMODE
+ ? ? ? bool "Hostmode support for BCMA PCI core"
+ ? ? ? depends on BCMA_DRIVER_MIPS
+ ? ? ? help
+ ? ? ? ? PCIcore hostmode operation (external PCI bus).

2011/6/6 Hauke Mehrtens <[email protected]>:

drivers/bcma/Kconfig config BCMA_HOST_PCI · line 34

Quoted 5 lines

drivers/bcma/Kconfig +5 -0

+config BCMA_PCICORE_HOSTMODE
+       bool "Hostmode support for BCMA PCI core"
+       depends on BCMA_DRIVER_MIPS
+       help
+         PCIcore hostmode operation (external PCI bus).

I think you started to use BCMA_DRIVER_corename. Could you stick to it (one schema), please? Maybe just
BCMA_DRIVER_PCI_HOSTMODE
?

drivers/bcma/driver_pci.c static void bcma_pcicore_serdes_workaround(struct bcma_drv_pci *pc) · line 172

Quoted 8 lines

drivers/bcma/driver_pci.c +7 -0

+#ifdef CONFIG_BCMA_PCICORE_HOSTMODE
+       pc->hostmode = bcma_pcicore_is_in_hostmode(pc);
+       if (pc->hostmode)
+               bcma_pcicore_init_hostmode(pc);
+#endif /* CONFIG_BCMA_PCICORE_HOSTMODE */
+       if (!pc->hostmode)
+               bcma_pcicore_serdes_workaround(pc);

Does it make sense to init hostmode PCI like clientmode if we just disable CONFIG_BCMA_PCICORE_HOSTMODE?

I think we should always check if core is host or client mode and use correct initialization only. We should not init it as clientmode just because we do not have driver for host mode.

drivers/bcma/driver_pci_host.c line 3

Quoted 11 lines

drivers/bcma/driver_pci_host.c new file +3 -0

new file mode 100644
index 0000000..b52c6c9
@@ -0,0 +1,44 @@
+/*
+ * Broadcom specific AMBA
+ * PCI Core

Please rename "PCI Core", add something about hostmode.

This patch add support for the bcma bus. Broadcom uses only Mips 74K CPUs on the new SoC and on the old ons using ssb bus there are no Mips 74K CPUs.

Signed-off-by Hauke Mehrtens <[email protected]>

---

?arch/mips/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?4 +++
?arch/mips/bcm47xx/gpio.c ? ? ? ? ? ? ? ? ? ? | ? ?9 ++++++++
?arch/mips/bcm47xx/nvram.c ? ? ? ? ? ? ? ? ? ?| ? ?6 +++++
?arch/mips/bcm47xx/serial.c ? ? ? ? ? ? ? ? ? | ? 24 +++++++++++++++++++++++
?arch/mips/bcm47xx/setup.c ? ? ? ? ? ? ? ? ? ?| ? 27 ++++++++++++++++++++++++-
?arch/mips/bcm47xx/time.c ? ? ? ? ? ? ? ? ? ? | ? ?3 ++
?arch/mips/include/asm/mach-bcm47xx/bcm47xx.h | ? ?3 ++
?arch/mips/include/asm/mach-bcm47xx/gpio.h ? ?| ? 18 +++++++++++++++++
?drivers/watchdog/bcm47xx_wdt.c ? ? ? ? ? ? ? | ? ?6 +++++

?9 files changed, 98 insertions(+), 2 deletions(-)

arch/mips/Kconfig +0 -0

index 653da62..bdb0341 100644
@@ -100,6 +100,10 @@ config BCM47XX

? ? ? ?select SSB_EMBEDDED
? ? ? ?select SSB_B43_PCI_BRIDGE if PCI
? ? ? ?select SSB_PCICORE_HOSTMODE if PCI
+ ? ? ? select BCMA
+ ? ? ? select BCMA_HOST_EMBEDDED
+ ? ? ? select BCMA_DRIVER_MIPS
+ ? ? ? select BCMA_PCICORE_HOSTMODE

2011/6/6 Hauke Mehrtens <[email protected]>:

Quoted 22 lines

This patch add support for the bcma bus. Broadcom uses only Mips 74K CPUs on the new SoC and on the old ons using ssb bus there are no Mips 74K CPUs.

Signed-off-by Hauke Mehrtens <[email protected]>

---
 arch/mips/Kconfig                            |    4 +++
 arch/mips/bcm47xx/gpio.c                     |    9 ++++++++
 arch/mips/bcm47xx/nvram.c                    |    6 +++++
 arch/mips/bcm47xx/serial.c                   |   24 +++++++++++++++++++++++
 arch/mips/bcm47xx/setup.c                    |   27 ++++++++++++++++++++++++-
 arch/mips/bcm47xx/time.c                     |    3 ++
 arch/mips/include/asm/mach-bcm47xx/bcm47xx.h |    3 ++
 arch/mips/include/asm/mach-bcm47xx/gpio.h    |   18 +++++++++++++++++
 drivers/watchdog/bcm47xx_wdt.c               |    6 +++++
 9 files changed, 98 insertions(+), 2 deletions(-)

arch/mips/Kconfig +0 -0

index 653da62..bdb0341 100644

arch/mips/Kconfig config BCM47XX · line 106

Quoted 7 lines

arch/mips/Kconfig +4 -0

@@ -100,6 +100,10 @@ config BCM47XX
       select SSB_EMBEDDED
       select SSB_B43_PCI_BRIDGE if PCI
       select SSB_PCICORE_HOSTMODE if PCI
+       select BCMA
+       select BCMA_HOST_EMBEDDED
+       select BCMA_DRIVER_MIPS
+       select BCMA_PCICORE_HOSTMODE

I'm not involved in development for embedded devices but I believe that space is quite important for them.

You force compiling both: ssb and bcma for every device using bcm47xx. I think ppl may want to compile only one bus driver.

Signed-off-by Hauke Mehrtens<[email protected]>

---
  arch/mips/bcm47xx/irq.c |    8 ++++++++
  1 files changed, 8 insertions(+), 0 deletions(-)

arch/mips/bcm47xx/irq.c +0 -0

index 325757a..3642cee 100644