2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> 2011/4/6 Rafał Miłecki <>:
> >> > If we want to have two drivers working on two (different) cores
> >> > simultaneously, we will have to add trivial mutex to group core
> >> > switching with core operation (read/write).
> >>
> >> With a little of work we could avoid switching and mutexes on no-host
> >> boards. MMIO is not limited to one core at once in such a case.
> >
> > I don't think that this is a problem at all.
> > All that magic does happen inside of the bus I/O handlers.
> > Just like SSB does it.
> > From a driver point of view, the I/O functions just need to
> > be atomic.
> >
> > For SSB it's not always 100% atomic, but we're always safe
> > due to some assumptions being made. But this is an SSB implementation
> > detail that is different from AXI. So don't look too closely
> > at the SSB implementation of the I/O functions. You certainly want
> > to implement them slightly differently in AXI. SSB currently doesn't
> > make use of the additional sliding windows, because they are not
> > available in the majority of SSB devices.
> >
> > The AXI bus subsystem will manage the sliding windows and the driver
> > doesn't know about the details.
>
> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>
> In BCMAI:
> bcmai_read() {
> mutex_get();
> switch_core();
> ioread();
> mutex_release();
> }

Yeah that basically is the idea. But it's a little bit harder than that.
The problem is that the mutex cannot be taken in interrupt context.
A spinlock probably is a bit hairy, too, depending on how heavy
a core switch is on AXI.

On SSB we workaround this with some (dirty but working) assumptions.

On AXI you probably can do lockless I/O, if you use the two windows
(how many windows are there?) in a clever way to avoid core switching
completely after the system was initialized.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> 2011/4/6 Rafał Miłecki <>:
> >> > If we want to have two drivers working on two (different) cores
> >> > simultaneously, we will have to add trivial mutex to group core
> >> > switching with core operation (read/write).
> >>
> >> With a little of work we could avoid switching and mutexes on no-host
> >> boards. MMIO is not limited to one core at once in such a case.
> >
> > I don't think that this is a problem at all.
> > All that magic does happen inside of the bus I/O handlers.
> > Just like SSB does it.
> > From a driver point of view, the I/O functions just need to
> > be atomic.
> >
> > For SSB it's not always 100% atomic, but we're always safe
> > due to some assumptions being made. But this is an SSB implementation
> > detail that is different from AXI. So don't look too closely
> > at the SSB implementation of the I/O functions. You certainly want
> > to implement them slightly differently in AXI. SSB currently doesn't
> > make use of the additional sliding windows, because they are not
> > available in the majority of SSB devices.
> >
> > The AXI bus subsystem will manage the sliding windows and the driver
> > doesn't know about the details.
>
> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>
> In BCMAI:
> bcmai_read() {
> mutex_get();
> switch_core();
> ioread();
> mutex_release();
> }

Yeah that basically is the idea. But it's a little bit harder than that.
The problem is that the mutex cannot be taken in interrupt context.
A spinlock probably is a bit hairy, too, depending on how heavy
a core switch is on AXI.

On SSB we workaround this with some (dirty but working) assumptions.

On AXI you probably can do lockless I/O, if you use the two windows
(how many windows are there?) in a clever way to avoid core switching
completely after the system was initialized.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
>> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
>> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> >> 2011/4/6 Rafał Miłecki <>:
>> >> > If we want to have two drivers working on two (different) cores
>> >> > simultaneously, we will have to add trivial mutex to group core
>> >> > switching with core operation (read/write).
>> >>
>> >> With a little of work we could avoid switching and mutexes on no-host
>> >> boards. MMIO is not limited to one core at once in such a case.
>> >
>> > I don't think that this is a problem at all.
>> > All that magic does happen inside of the bus I/O handlers.
>> > Just like SSB does it.
>> > From a driver point of view, the I/O functions just need to
>> > be atomic.
>> >
>> > For SSB it's not always 100% atomic, but we're always safe
>> > due to some assumptions being made. But this is an SSB implementation
>> > detail that is different from AXI. So don't look too closely
>> > at the SSB implementation of the I/O functions. You certainly want
>> > to implement them slightly differently in AXI. SSB currently doesn't
>> > make use of the additional sliding windows, because they are not
>> > available in the majority of SSB devices.
>> >
>> > The AXI bus subsystem will manage the sliding windows and the driver
>> > doesn't know about the details.
>>
>> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>>
>> In BCMAI:
>> bcmai_read() {
>> mutex_get();
>> switch_core();
>> ioread();
>> mutex_release();
>> }
>
> Yeah that basically is the idea. But it's a little bit harder than that.
> The problem is that the mutex cannot be taken in interrupt context.
> A spinlock probably is a bit hairy, too, depending on how heavy
> a core switch is on AXI.
>
> On SSB we workaround this with some (dirty but working) assumptions.
>
> On AXI you probably can do lockless I/O, if you use the two windows
> (how many windows are there?) in a clever way to avoid core switching
> completely after the system was initialized.

We have 2 windows. I didn't try this, but let's assume they have no
limitations. We can use first window for one driver only, second
driver for second driver only. That gives us 2 drivers simultaneously
working drivers. No driver need to reset core really often (and not
inside interrupt context) so we will switch driver's window to agent
(from core) only at init/reset.

The question is what amount of driver we will need to support at the same time.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> 2011/4/6 Rafał Miłecki <>:
> >> > If we want to have two drivers working on two (different) cores
> >> > simultaneously, we will have to add trivial mutex to group core
> >> > switching with core operation (read/write).
> >>
> >> With a little of work we could avoid switching and mutexes on no-host
> >> boards. MMIO is not limited to one core at once in such a case.
> >
> > I don't think that this is a problem at all.
> > All that magic does happen inside of the bus I/O handlers.
> > Just like SSB does it.
> > From a driver point of view, the I/O functions just need to
> > be atomic.
> >
> > For SSB it's not always 100% atomic, but we're always safe
> > due to some assumptions being made. But this is an SSB implementation
> > detail that is different from AXI. So don't look too closely
> > at the SSB implementation of the I/O functions. You certainly want
> > to implement them slightly differently in AXI. SSB currently doesn't
> > make use of the additional sliding windows, because they are not
> > available in the majority of SSB devices.
> >
> > The AXI bus subsystem will manage the sliding windows and the driver
> > doesn't know about the details.
>
> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>
> In BCMAI:
> bcmai_read() {
> mutex_get();
> switch_core();
> ioread();
> mutex_release();
> }

Yeah that basically is the idea. But it's a little bit harder than that.
The problem is that the mutex cannot be taken in interrupt context.
A spinlock probably is a bit hairy, too, depending on how heavy
a core switch is on AXI.

On SSB we workaround this with some (dirty but working) assumptions.

On AXI you probably can do lockless I/O, if you use the two windows
(how many windows are there?) in a clever way to avoid core switching
completely after the system was initialized.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
>> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
>> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> >> 2011/4/6 Rafał Miłecki <>:
>> >> > If we want to have two drivers working on two (different) cores
>> >> > simultaneously, we will have to add trivial mutex to group core
>> >> > switching with core operation (read/write).
>> >>
>> >> With a little of work we could avoid switching and mutexes on no-host
>> >> boards. MMIO is not limited to one core at once in such a case.
>> >
>> > I don't think that this is a problem at all.
>> > All that magic does happen inside of the bus I/O handlers.
>> > Just like SSB does it.
>> > From a driver point of view, the I/O functions just need to
>> > be atomic.
>> >
>> > For SSB it's not always 100% atomic, but we're always safe
>> > due to some assumptions being made. But this is an SSB implementation
>> > detail that is different from AXI. So don't look too closely
>> > at the SSB implementation of the I/O functions. You certainly want
>> > to implement them slightly differently in AXI. SSB currently doesn't
>> > make use of the additional sliding windows, because they are not
>> > available in the majority of SSB devices.
>> >
>> > The AXI bus subsystem will manage the sliding windows and the driver
>> > doesn't know about the details.
>>
>> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>>
>> In BCMAI:
>> bcmai_read() {
>> mutex_get();
>> switch_core();
>> ioread();
>> mutex_release();
>> }
>
> Yeah that basically is the idea. But it's a little bit harder than that.
> The problem is that the mutex cannot be taken in interrupt context.
> A spinlock probably is a bit hairy, too, depending on how heavy
> a core switch is on AXI.
>
> On SSB we workaround this with some (dirty but working) assumptions.
>
> On AXI you probably can do lockless I/O, if you use the two windows
> (how many windows are there?) in a clever way to avoid core switching
> completely after the system was initialized.

We have 2 windows. I didn't try this, but let's assume they have no
limitations. We can use first window for one driver only, second
driver for second driver only. That gives us 2 drivers simultaneously
working drivers. No driver need to reset core really often (and not
inside interrupt context) so we will switch driver's window to agent
(from core) only at init/reset.

The question is what amount of driver we will need to support at the same time.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.
>
> The question is what amount of driver we will need to support at the same time.
>

I guess (correct me please, Broadcom guys if I'm wrong) there are two
functions two-head w11 pci host and therefore 4 sliding windows, 2 per
each function.

You really was in need for core switching for PCI SSB hosts, but seem
all that stuff for PCI switching in current bcm80211/utils code is
rudimentary stuff left from PCI times when you was required to use
sliding window for chipcommon and pci bridge core access.

Have nice day,
George



_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> 2011/4/6 Rafał Miłecki <>:
> >> > If we want to have two drivers working on two (different) cores
> >> > simultaneously, we will have to add trivial mutex to group core
> >> > switching with core operation (read/write).
> >>
> >> With a little of work we could avoid switching and mutexes on no-host
> >> boards. MMIO is not limited to one core at once in such a case.
> >
> > I don't think that this is a problem at all.
> > All that magic does happen inside of the bus I/O handlers.
> > Just like SSB does it.
> > From a driver point of view, the I/O functions just need to
> > be atomic.
> >
> > For SSB it's not always 100% atomic, but we're always safe
> > due to some assumptions being made. But this is an SSB implementation
> > detail that is different from AXI. So don't look too closely
> > at the SSB implementation of the I/O functions. You certainly want
> > to implement them slightly differently in AXI. SSB currently doesn't
> > make use of the additional sliding windows, because they are not
> > available in the majority of SSB devices.
> >
> > The AXI bus subsystem will manage the sliding windows and the driver
> > doesn't know about the details.
>
> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>
> In BCMAI:
> bcmai_read() {
> mutex_get();
> switch_core();
> ioread();
> mutex_release();
> }

Yeah that basically is the idea. But it's a little bit harder than that.
The problem is that the mutex cannot be taken in interrupt context.
A spinlock probably is a bit hairy, too, depending on how heavy
a core switch is on AXI.

On SSB we workaround this with some (dirty but working) assumptions.

On AXI you probably can do lockless I/O, if you use the two windows
(how many windows are there?) in a clever way to avoid core switching
completely after the system was initialized.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
>> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
>> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> >> 2011/4/6 Rafał Miłecki <>:
>> >> > If we want to have two drivers working on two (different) cores
>> >> > simultaneously, we will have to add trivial mutex to group core
>> >> > switching with core operation (read/write).
>> >>
>> >> With a little of work we could avoid switching and mutexes on no-host
>> >> boards. MMIO is not limited to one core at once in such a case.
>> >
>> > I don't think that this is a problem at all.
>> > All that magic does happen inside of the bus I/O handlers.
>> > Just like SSB does it.
>> > From a driver point of view, the I/O functions just need to
>> > be atomic.
>> >
>> > For SSB it's not always 100% atomic, but we're always safe
>> > due to some assumptions being made. But this is an SSB implementation
>> > detail that is different from AXI. So don't look too closely
>> > at the SSB implementation of the I/O functions. You certainly want
>> > to implement them slightly differently in AXI. SSB currently doesn't
>> > make use of the additional sliding windows, because they are not
>> > available in the majority of SSB devices.
>> >
>> > The AXI bus subsystem will manage the sliding windows and the driver
>> > doesn't know about the details.
>>
>> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>>
>> In BCMAI:
>> bcmai_read() {
>> mutex_get();
>> switch_core();
>> ioread();
>> mutex_release();
>> }
>
> Yeah that basically is the idea. But it's a little bit harder than that.
> The problem is that the mutex cannot be taken in interrupt context.
> A spinlock probably is a bit hairy, too, depending on how heavy
> a core switch is on AXI.
>
> On SSB we workaround this with some (dirty but working) assumptions.
>
> On AXI you probably can do lockless I/O, if you use the two windows
> (how many windows are there?) in a clever way to avoid core switching
> completely after the system was initialized.

We have 2 windows. I didn't try this, but let's assume they have no
limitations. We can use first window for one driver only, second
driver for second driver only. That gives us 2 drivers simultaneously
working drivers. No driver need to reset core really often (and not
inside interrupt context) so we will switch driver's window to agent
(from core) only at init/reset.

The question is what amount of driver we will need to support at the same time.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.
>
> The question is what amount of driver we will need to support at the same time.
>

I guess (correct me please, Broadcom guys if I'm wrong) there are two
functions two-head w11 pci host and therefore 4 sliding windows, 2 per
each function.

You really was in need for core switching for PCI SSB hosts, but seem
all that stuff for PCI switching in current bcm80211/utils code is
rudimentary stuff left from PCI times when you was required to use
sliding window for chipcommon and pci bridge core access.

Have nice day,
George



_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:12 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.

On SSB one major assumption is that on non-embedded no coreswitch will
happen after init. (On embedded, the I/O accesses are safe anyway,
because all cores are always mapped).

> The question is what amount of driver we will need to support at the same time.

I guess (guess!) that the answer will be similar to SSB:
On non-embedded after init only one core is accessed: The wireless core.
And on embedded cores are accessed in random order. But it's safe
because the MMIO is always mapped.
Does this also apply to AXI?
Even if we would need to access two cores after init (wireless and
chipcommon, perhaps), we would use the two windows to survive without
an actual core switch while running.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> 2011/4/6 Rafał Miłecki <>:
> >> > If we want to have two drivers working on two (different) cores
> >> > simultaneously, we will have to add trivial mutex to group core
> >> > switching with core operation (read/write).
> >>
> >> With a little of work we could avoid switching and mutexes on no-host
> >> boards. MMIO is not limited to one core at once in such a case.
> >
> > I don't think that this is a problem at all.
> > All that magic does happen inside of the bus I/O handlers.
> > Just like SSB does it.
> > From a driver point of view, the I/O functions just need to
> > be atomic.
> >
> > For SSB it's not always 100% atomic, but we're always safe
> > due to some assumptions being made. But this is an SSB implementation
> > detail that is different from AXI. So don't look too closely
> > at the SSB implementation of the I/O functions. You certainly want
> > to implement them slightly differently in AXI. SSB currently doesn't
> > make use of the additional sliding windows, because they are not
> > available in the majority of SSB devices.
> >
> > The AXI bus subsystem will manage the sliding windows and the driver
> > doesn't know about the details.
>
> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>
> In BCMAI:
> bcmai_read() {
> mutex_get();
> switch_core();
> ioread();
> mutex_release();
> }

Yeah that basically is the idea. But it's a little bit harder than that.
The problem is that the mutex cannot be taken in interrupt context.
A spinlock probably is a bit hairy, too, depending on how heavy
a core switch is on AXI.

On SSB we workaround this with some (dirty but working) assumptions.

On AXI you probably can do lockless I/O, if you use the two windows
(how many windows are there?) in a clever way to avoid core switching
completely after the system was initialized.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
>> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
>> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> >> 2011/4/6 Rafał Miłecki <>:
>> >> > If we want to have two drivers working on two (different) cores
>> >> > simultaneously, we will have to add trivial mutex to group core
>> >> > switching with core operation (read/write).
>> >>
>> >> With a little of work we could avoid switching and mutexes on no-host
>> >> boards. MMIO is not limited to one core at once in such a case.
>> >
>> > I don't think that this is a problem at all.
>> > All that magic does happen inside of the bus I/O handlers.
>> > Just like SSB does it.
>> > From a driver point of view, the I/O functions just need to
>> > be atomic.
>> >
>> > For SSB it's not always 100% atomic, but we're always safe
>> > due to some assumptions being made. But this is an SSB implementation
>> > detail that is different from AXI. So don't look too closely
>> > at the SSB implementation of the I/O functions. You certainly want
>> > to implement them slightly differently in AXI. SSB currently doesn't
>> > make use of the additional sliding windows, because they are not
>> > available in the majority of SSB devices.
>> >
>> > The AXI bus subsystem will manage the sliding windows and the driver
>> > doesn't know about the details.
>>
>> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>>
>> In BCMAI:
>> bcmai_read() {
>> mutex_get();
>> switch_core();
>> ioread();
>> mutex_release();
>> }
>
> Yeah that basically is the idea. But it's a little bit harder than that.
> The problem is that the mutex cannot be taken in interrupt context.
> A spinlock probably is a bit hairy, too, depending on how heavy
> a core switch is on AXI.
>
> On SSB we workaround this with some (dirty but working) assumptions.
>
> On AXI you probably can do lockless I/O, if you use the two windows
> (how many windows are there?) in a clever way to avoid core switching
> completely after the system was initialized.

We have 2 windows. I didn't try this, but let's assume they have no
limitations. We can use first window for one driver only, second
driver for second driver only. That gives us 2 drivers simultaneously
working drivers. No driver need to reset core really often (and not
inside interrupt context) so we will switch driver's window to agent
(from core) only at init/reset.

The question is what amount of driver we will need to support at the same time.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.
>
> The question is what amount of driver we will need to support at the same time.
>

I guess (correct me please, Broadcom guys if I'm wrong) there are two
functions two-head w11 pci host and therefore 4 sliding windows, 2 per
each function.

You really was in need for core switching for PCI SSB hosts, but seem
all that stuff for PCI switching in current bcm80211/utils code is
rudimentary stuff left from PCI times when you was required to use
sliding window for chipcommon and pci bridge core access.

Have nice day,
George



_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:12 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.

On SSB one major assumption is that on non-embedded no coreswitch will
happen after init. (On embedded, the I/O accesses are safe anyway,
because all cores are always mapped).

> The question is what amount of driver we will need to support at the same time.

I guess (guess!) that the answer will be similar to SSB:
On non-embedded after init only one core is accessed: The wireless core.
And on embedded cores are accessed in random order. But it's safe
because the MMIO is always mapped.
Does this also apply to AXI?
Even if we would need to access two cores after init (wireless and
chipcommon, perhaps), we would use the two windows to survive without
an actual core switch while running.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:18 użytkownik George Kashperko
<> napisał:
>> We have 2 windows. I didn't try this, but let's assume they have no
>> limitations. We can use first window for one driver only, second
>> driver for second driver only. That gives us 2 drivers simultaneously
>> working drivers. No driver need to reset core really often (and not
>> inside interrupt context) so we will switch driver's window to agent
>> (from core) only at init/reset.
>>
>> The question is what amount of driver we will need to support at the same time.
>>
>
> I guess (correct me please, Broadcom guys if I'm wrong) there are two
> functions two-head w11 pci host and therefore 4 sliding windows, 2 per
> each function.

I don't understand you. Can you use more friendly language? functions?
2head? w11?

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> 2011/4/6 Rafał Miłecki <>:
> >> > If we want to have two drivers working on two (different) cores
> >> > simultaneously, we will have to add trivial mutex to group core
> >> > switching with core operation (read/write).
> >>
> >> With a little of work we could avoid switching and mutexes on no-host
> >> boards. MMIO is not limited to one core at once in such a case.
> >
> > I don't think that this is a problem at all.
> > All that magic does happen inside of the bus I/O handlers.
> > Just like SSB does it.
> > From a driver point of view, the I/O functions just need to
> > be atomic.
> >
> > For SSB it's not always 100% atomic, but we're always safe
> > due to some assumptions being made. But this is an SSB implementation
> > detail that is different from AXI. So don't look too closely
> > at the SSB implementation of the I/O functions. You certainly want
> > to implement them slightly differently in AXI. SSB currently doesn't
> > make use of the additional sliding windows, because they are not
> > available in the majority of SSB devices.
> >
> > The AXI bus subsystem will manage the sliding windows and the driver
> > doesn't know about the details.
>
> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>
> In BCMAI:
> bcmai_read() {
> mutex_get();
> switch_core();
> ioread();
> mutex_release();
> }

Yeah that basically is the idea. But it's a little bit harder than that.
The problem is that the mutex cannot be taken in interrupt context.
A spinlock probably is a bit hairy, too, depending on how heavy
a core switch is on AXI.

On SSB we workaround this with some (dirty but working) assumptions.

On AXI you probably can do lockless I/O, if you use the two windows
(how many windows are there?) in a clever way to avoid core switching
completely after the system was initialized.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
>> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
>> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> >> 2011/4/6 Rafał Miłecki <>:
>> >> > If we want to have two drivers working on two (different) cores
>> >> > simultaneously, we will have to add trivial mutex to group core
>> >> > switching with core operation (read/write).
>> >>
>> >> With a little of work we could avoid switching and mutexes on no-host
>> >> boards. MMIO is not limited to one core at once in such a case.
>> >
>> > I don't think that this is a problem at all.
>> > All that magic does happen inside of the bus I/O handlers.
>> > Just like SSB does it.
>> > From a driver point of view, the I/O functions just need to
>> > be atomic.
>> >
>> > For SSB it's not always 100% atomic, but we're always safe
>> > due to some assumptions being made. But this is an SSB implementation
>> > detail that is different from AXI. So don't look too closely
>> > at the SSB implementation of the I/O functions. You certainly want
>> > to implement them slightly differently in AXI. SSB currently doesn't
>> > make use of the additional sliding windows, because they are not
>> > available in the majority of SSB devices.
>> >
>> > The AXI bus subsystem will manage the sliding windows and the driver
>> > doesn't know about the details.
>>
>> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>>
>> In BCMAI:
>> bcmai_read() {
>> mutex_get();
>> switch_core();
>> ioread();
>> mutex_release();
>> }
>
> Yeah that basically is the idea. But it's a little bit harder than that.
> The problem is that the mutex cannot be taken in interrupt context.
> A spinlock probably is a bit hairy, too, depending on how heavy
> a core switch is on AXI.
>
> On SSB we workaround this with some (dirty but working) assumptions.
>
> On AXI you probably can do lockless I/O, if you use the two windows
> (how many windows are there?) in a clever way to avoid core switching
> completely after the system was initialized.

We have 2 windows. I didn't try this, but let's assume they have no
limitations. We can use first window for one driver only, second
driver for second driver only. That gives us 2 drivers simultaneously
working drivers. No driver need to reset core really often (and not
inside interrupt context) so we will switch driver's window to agent
(from core) only at init/reset.

The question is what amount of driver we will need to support at the same time.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.
>
> The question is what amount of driver we will need to support at the same time.
>

I guess (correct me please, Broadcom guys if I'm wrong) there are two
functions two-head w11 pci host and therefore 4 sliding windows, 2 per
each function.

You really was in need for core switching for PCI SSB hosts, but seem
all that stuff for PCI switching in current bcm80211/utils code is
rudimentary stuff left from PCI times when you was required to use
sliding window for chipcommon and pci bridge core access.

Have nice day,
George



_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:12 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.

On SSB one major assumption is that on non-embedded no coreswitch will
happen after init. (On embedded, the I/O accesses are safe anyway,
because all cores are always mapped).

> The question is what amount of driver we will need to support at the same time.

I guess (guess!) that the answer will be similar to SSB:
On non-embedded after init only one core is accessed: The wireless core.
And on embedded cores are accessed in random order. But it's safe
because the MMIO is always mapped.
Does this also apply to AXI?
Even if we would need to access two cores after init (wireless and
chipcommon, perhaps), we would use the two windows to survive without
an actual core switch while running.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:18 użytkownik George Kashperko
<> napisał:
>> We have 2 windows. I didn't try this, but let's assume they have no
>> limitations. We can use first window for one driver only, second
>> driver for second driver only. That gives us 2 drivers simultaneously
>> working drivers. No driver need to reset core really often (and not
>> inside interrupt context) so we will switch driver's window to agent
>> (from core) only at init/reset.
>>
>> The question is what amount of driver we will need to support at the same time.
>>
>
> I guess (correct me please, Broadcom guys if I'm wrong) there are two
> functions two-head w11 pci host and therefore 4 sliding windows, 2 per
> each function.

I don't understand you. Can you use more friendly language? functions?
2head? w11?

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:18 użytkownik George Kashperko
> <> napisał:
> >> We have 2 windows. I didn't try this, but let's assume they have no
> >> limitations. We can use first window for one driver only, second
> >> driver for second driver only. That gives us 2 drivers simultaneously
> >> working drivers. No driver need to reset core really often (and not
> >> inside interrupt context) so we will switch driver's window to agent
> >> (from core) only at init/reset.
> >>
> >> The question is what amount of driver we will need to support at the same time.
> >>
> >
> > I guess (correct me please, Broadcom guys if I'm wrong) there are two
> > functions two-head w11 pci host and therefore 4 sliding windows, 2 per
> > each function.
>
> I don't understand you. Can you use more friendly language? functions?
> 2head? w11?
>

For PCI function description take a look at PCI specs or PCI
configuration space description (e. g.
http://en.wikipedia.org/wiki/PCI_configuration_space)

Sorry for missleading short-ups, w11 - bcm80211 core, under two-head I
mean ssb/axi with two functional cores on same interconnect (like w11
+w11, not a lot of these exists I guess). Also there were some b43+b44
on single PCI ssb host and those where implemented as ssb interconnect
on multifunctional PCI host therefore providing separate access windows
for each function.

Might I mussunderstood something (its late night here at my place) when
you where talking about using coreswitching involved for two drivers
therefore I remembered about those functions. Seems now you were talking
about chipcommon+b43 access sharing same window.

As for core switching requirments for earlier SSB interconnects on PCI
hosts where there were no direct chipcommon access, that one can be
accomplished without spin_lock/mutex for b43 or b44 cores with proper
bus design.

AXI doesn't need spinlocks/mutexes as both chipcommon and pci bridge are
available directly and b43 will be the only one requiring window access.

Have nice day,
George


_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> 2011/4/6 Rafał Miłecki <>:
> >> > If we want to have two drivers working on two (different) cores
> >> > simultaneously, we will have to add trivial mutex to group core
> >> > switching with core operation (read/write).
> >>
> >> With a little of work we could avoid switching and mutexes on no-host
> >> boards. MMIO is not limited to one core at once in such a case.
> >
> > I don't think that this is a problem at all.
> > All that magic does happen inside of the bus I/O handlers.
> > Just like SSB does it.
> > From a driver point of view, the I/O functions just need to
> > be atomic.
> >
> > For SSB it's not always 100% atomic, but we're always safe
> > due to some assumptions being made. But this is an SSB implementation
> > detail that is different from AXI. So don't look too closely
> > at the SSB implementation of the I/O functions. You certainly want
> > to implement them slightly differently in AXI. SSB currently doesn't
> > make use of the additional sliding windows, because they are not
> > available in the majority of SSB devices.
> >
> > The AXI bus subsystem will manage the sliding windows and the driver
> > doesn't know about the details.
>
> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>
> In BCMAI:
> bcmai_read() {
> mutex_get();
> switch_core();
> ioread();
> mutex_release();
> }

Yeah that basically is the idea. But it's a little bit harder than that.
The problem is that the mutex cannot be taken in interrupt context.
A spinlock probably is a bit hairy, too, depending on how heavy
a core switch is on AXI.

On SSB we workaround this with some (dirty but working) assumptions.

On AXI you probably can do lockless I/O, if you use the two windows
(how many windows are there?) in a clever way to avoid core switching
completely after the system was initialized.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
>> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
>> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> >> 2011/4/6 Rafał Miłecki <>:
>> >> > If we want to have two drivers working on two (different) cores
>> >> > simultaneously, we will have to add trivial mutex to group core
>> >> > switching with core operation (read/write).
>> >>
>> >> With a little of work we could avoid switching and mutexes on no-host
>> >> boards. MMIO is not limited to one core at once in such a case.
>> >
>> > I don't think that this is a problem at all.
>> > All that magic does happen inside of the bus I/O handlers.
>> > Just like SSB does it.
>> > From a driver point of view, the I/O functions just need to
>> > be atomic.
>> >
>> > For SSB it's not always 100% atomic, but we're always safe
>> > due to some assumptions being made. But this is an SSB implementation
>> > detail that is different from AXI. So don't look too closely
>> > at the SSB implementation of the I/O functions. You certainly want
>> > to implement them slightly differently in AXI. SSB currently doesn't
>> > make use of the additional sliding windows, because they are not
>> > available in the majority of SSB devices.
>> >
>> > The AXI bus subsystem will manage the sliding windows and the driver
>> > doesn't know about the details.
>>
>> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>>
>> In BCMAI:
>> bcmai_read() {
>> mutex_get();
>> switch_core();
>> ioread();
>> mutex_release();
>> }
>
> Yeah that basically is the idea. But it's a little bit harder than that.
> The problem is that the mutex cannot be taken in interrupt context.
> A spinlock probably is a bit hairy, too, depending on how heavy
> a core switch is on AXI.
>
> On SSB we workaround this with some (dirty but working) assumptions.
>
> On AXI you probably can do lockless I/O, if you use the two windows
> (how many windows are there?) in a clever way to avoid core switching
> completely after the system was initialized.

We have 2 windows. I didn't try this, but let's assume they have no
limitations. We can use first window for one driver only, second
driver for second driver only. That gives us 2 drivers simultaneously
working drivers. No driver need to reset core really often (and not
inside interrupt context) so we will switch driver's window to agent
(from core) only at init/reset.

The question is what amount of driver we will need to support at the same time.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.
>
> The question is what amount of driver we will need to support at the same time.
>

I guess (correct me please, Broadcom guys if I'm wrong) there are two
functions two-head w11 pci host and therefore 4 sliding windows, 2 per
each function.

You really was in need for core switching for PCI SSB hosts, but seem
all that stuff for PCI switching in current bcm80211/utils code is
rudimentary stuff left from PCI times when you was required to use
sliding window for chipcommon and pci bridge core access.

Have nice day,
George



_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:12 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.

On SSB one major assumption is that on non-embedded no coreswitch will
happen after init. (On embedded, the I/O accesses are safe anyway,
because all cores are always mapped).

> The question is what amount of driver we will need to support at the same time.

I guess (guess!) that the answer will be similar to SSB:
On non-embedded after init only one core is accessed: The wireless core.
And on embedded cores are accessed in random order. But it's safe
because the MMIO is always mapped.
Does this also apply to AXI?
Even if we would need to access two cores after init (wireless and
chipcommon, perhaps), we would use the two windows to survive without
an actual core switch while running.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:18 użytkownik George Kashperko
<> napisał:
>> We have 2 windows. I didn't try this, but let's assume they have no
>> limitations. We can use first window for one driver only, second
>> driver for second driver only. That gives us 2 drivers simultaneously
>> working drivers. No driver need to reset core really often (and not
>> inside interrupt context) so we will switch driver's window to agent
>> (from core) only at init/reset.
>>
>> The question is what amount of driver we will need to support at the same time.
>>
>
> I guess (correct me please, Broadcom guys if I'm wrong) there are two
> functions two-head w11 pci host and therefore 4 sliding windows, 2 per
> each function.

I don't understand you. Can you use more friendly language? functions?
2head? w11?

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:18 użytkownik George Kashperko
> <> napisał:
> >> We have 2 windows. I didn't try this, but let's assume they have no
> >> limitations. We can use first window for one driver only, second
> >> driver for second driver only. That gives us 2 drivers simultaneously
> >> working drivers. No driver need to reset core really often (and not
> >> inside interrupt context) so we will switch driver's window to agent
> >> (from core) only at init/reset.
> >>
> >> The question is what amount of driver we will need to support at the same time.
> >>
> >
> > I guess (correct me please, Broadcom guys if I'm wrong) there are two
> > functions two-head w11 pci host and therefore 4 sliding windows, 2 per
> > each function.
>
> I don't understand you. Can you use more friendly language? functions?
> 2head? w11?
>

For PCI function description take a look at PCI specs or PCI
configuration space description (e. g.
http://en.wikipedia.org/wiki/PCI_configuration_space)

Sorry for missleading short-ups, w11 - bcm80211 core, under two-head I
mean ssb/axi with two functional cores on same interconnect (like w11
+w11, not a lot of these exists I guess). Also there were some b43+b44
on single PCI ssb host and those where implemented as ssb interconnect
on multifunctional PCI host therefore providing separate access windows
for each function.

Might I mussunderstood something (its late night here at my place) when
you where talking about using coreswitching involved for two drivers
therefore I remembered about those functions. Seems now you were talking
about chipcommon+b43 access sharing same window.

As for core switching requirments for earlier SSB interconnects on PCI
hosts where there were no direct chipcommon access, that one can be
accomplished without spin_lock/mutex for b43 or b44 cores with proper
bus design.

AXI doesn't need spinlocks/mutexes as both chipcommon and pci bridge are
available directly and b43 will be the only one requiring window access.

Have nice day,
George


_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 7 kwietnia 2011 02:00 użytkownik George Kashperko
<> napisał:
> For PCI function description take a look at PCI specs or PCI
> configuration space description (e. g.
> http://en.wikipedia.org/wiki/PCI_configuration_space)
>
> Sorry for missleading short-ups, w11 - bcm80211 core, under two-head I
> mean ssb/axi with two functional cores on same interconnect (like w11
> +w11, not a lot of these exists I guess). Also there were some b43+b44
> on single PCI ssb host and those where implemented as ssb interconnect
> on multifunctional PCI host therefore providing separate access windows
> for each function.
>
> Might I mussunderstood something (its late night here at my place) when
> you where talking about using coreswitching involved for two drivers
> therefore I remembered about those functions. Seems now you were talking
> about chipcommon+b43 access sharing same window.
>
> As for core switching requirments for earlier SSB interconnects on PCI
> hosts where there were no direct chipcommon access, that one can be
> accomplished without spin_lock/mutex for b43 or b44 cores with proper
> bus design.
>
> AXI doesn't need spinlocks/mutexes as both chipcommon and pci bridge are
> available directly and b43 will be the only one requiring window access.

Ahh, so while talking about 4 windows, I guess you counted fixes
windows as well. That would be right, matching my knowledge.

When asking question about amount of cores we may want to use
simultaneously I didn't think about ChipCommon or PCIe. The real
problem would be to support for example two 802.11 cores and one
ethernet core at the same time. That gives us 3 cores while we have
only 2 sliding windows. But I would not care for that too much for
now.

For the rest (PCI function) I have to sleep before reading specs ;)

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> 1. Term Broadcom AI
>
> You are referring to this as Broadcom AI. However, there is nothing Broadcom
> specific about this. Basically, this functionality (core enumeration and
> generic core functions) is provided by ARM with the AMBA AXI and named
> Device Management Plugin (abbrev. DMP).

I'm still little confused with that, let me read old mails, google a
little, etc. I though AMBA AXI is AI on ARM host, give me some time
for this.


> 2. Bus registration
>
> The bus registration mandates the use of chipcommon driver if a chipcommon
> core is detected (same for pcie). In brcm80211 driver we access the d11core
> and chipcommon. Is this still possible or should we remove all register
> access to chipcommon from our driver if we were to use bcmai. Also as each
> core is registered as a linux device does that imply a one-on-one relation?
> So one core equals one driver?

You should drop initialization (to do not perform it twice), but
ChipCommon ops are still allowed. See: bcmai_cc_read32,
bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.

You can simply call:
bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);

There is nothing stopping you from registering one driver for few
cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
this is not possible to use 2 drivers for 1 core at the same time.


> 3. Device identification
>
> The cores are identified by manufacturer, core id and revision in your
> patch. I would not use the revision because 4 out of 5 times a revision
> change does indicate a hardware change but no change in programming
> interface. The enumeration data does contain a more selective field
> indicating the core class (4 bits following the core identifier). I suggest
> to replace the revision field by this class field.

Well, maybe we don't use core revision all the time, but we do.
1) In the past, we decided if core is supported by b43 or b43legacy
exactly by using core revision
2) For N PHY on SSB we check core revision in b43_nphy_classifier
3) In main.c we check for id.revision a few times
You can say it's all SSB, OK, so:
4) On AI ChipCommon has capabilities_ext on rev >= 35 only
5) PMU init requires "Fix for 4329b0 bad LPOM state", 4329b0 check is
core rev based

So there are few places where we need core revision. If you want to
register driver for every core, you can always use BCMAI_ANY_REV.
Michael mentioned recently that mod_devicetable.h is ABI, so I really
prefer to have one more u8 than end with workarounds.

Please tell me sth more about "core class (4 bits following the core
identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
0x000F0000 which is revision? I guess you meant 0x00F00000 which is
package? Thank you for pointing this, it may be very important. For
the same reasons I want to have revision, I do not want to miss
something else that is important. I think we should add package as one
another core attribute.


> 4. PCI Host interface
>
> When building the PCI host interface variant of the bcmai it compiles in the
> source file b43_pci_ai_bridge.c. This name suggests that this module can
> only be used with b43 driver. Is that a correct observation? I see this as a
> major restriction to using the new module or could another driver (like
> ours) use the bcmai_host_pci_register() function.

No, it is not for b43 only. You are right of course, I'll rename this.
It's pretty simple (dumb) driver which is just here just to auto-load
bcmai module for given PCI IDs. You could just register driver for
802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
correct name for this anyway.


> Now for the code comments, see inline remarks below.

> Probably a different name would be better. bcm suggests this is broadcom
> specific, but it is hardware functionality provided by ARM. Suggestions:
> axi, axidmp,amba_axi.

Let me read more abouth this.


>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16 offset)
>> +{
>> +       if (unlikely(core->bus->mapped_core != core))
>> +               bcmai_host_pci_switch_core(core);
>> +       return ioread32(core->bus->mmio + 0x1000 + offset);
>> +}
>
> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
> correct define).

I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
more self-explaining for new developers.


>> +       if (!pci_is_pcie(dev))
>> +               bcmai_err("PCI card detected, report problems.\n");
>
> Not that I know off with Broadcom chips. Can ask around here, but I don't
> know about other manufacturers. Not even sure if there are others using the
> AMBA AXI DMP functionality.

The purpose was to track existence of PCI cards to implement XTAL
manual powering up in case of problems. Maybe we could drop this but
on the other hand it's just a one small single check once-executed.
And it seems Broadcom didn't abound PCI cards long, long time ago as I
have mini PCI 14e4:4329 with BCM4321.


>> +               core->dev.release = bcmai_release_core_dev;
>> +               core->dev.bus = &bcmai_bus_type;
>> +               dev_set_name(&core->dev, "ssbX:%d", /*bus->busnumber,*/
>> dev_id);
>
> The device name should probably start with something other than 'ssb' ;-)

Whoops! ;)


>> +static u32 bcmai_scan_read32(struct bcmai_bus *bus, u8 current_coreidx,
>> +                      u16 offset)
>> +{
>> +       return readl(bus->mmio + offset);
>> +}
>
> I have seen both ioread32() and readl() in this patch. Are these doing the
> same thing?

This readl is used while scanning, when we don't use host native
functions as they are core-based. Not sure if we can use ioread32 for
embedded systems without host device.


>> +       bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>
> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise, it
> could be defined in this source file only instead of in a header file.

I though we prefer to keep defines in .h in Linux, let me check (Google) for it.


>> +               core.id.id = (cia & SCAN_CIA_ID) >> SCAN_CIA_ID_SHIFT;
>> +               core.id.manuf = (cia & SCAN_CIA_MANUF) >>
>> SCAN_CIA_MANUF_SHIFT;
>
> You can also get the component class here. Next 4 bits in cia.

Ahh, now I see your explanation here, thanks, I'll check this! Ignore
me previous question about class for now.


>> +#define bcmai_info(fmt, args...)       printk(KERN_INFO "bcmai: " fmt,
>> ##args)
>> +#ifdef CONFIG_BCMAI_DEBUG
>> +#define bcmai_dbg(fmt, args...)                printk(KERN_DEBUG "bcmai
>> debug: " fmt, ##args)
>> +#else
>> +#define bcmai_dbg(fmt, args...)                do { } while (0)
>> +#endif
>> +#define bcmai_err(fmt, args...)                printk(KERN_ERR "bcmai
>> error: " fmt, ##args)
>> +
>
> Would go for the pr_... functions. I used that in brcmaxi.

I got comments to that already, will fix.


>> +#define BCMAI_CORE_(...)
>> +#define BCMAI_CORE_SHIM                        0x837   /* SHIM component
>> in ubus/6362 */
>> +#define BCMAI_CORE_DEFAULT             0xFFF
>
> Probably this list includes some cores that were in old chips, but will
> never show up in bcmai chips.

Can you point which cores we should keep/drop?


>> +#define   SSB_PLLTYPE_5                        0x00018000      /* 25Mhz,
>> 4 dividers */
>> +#define   SSB_PLLTYPE_6                        0x00028000      /* 100/200
>> or 120/240 only */
>> +#define   SSB_PLLTYPE_7                        0x00038000      /* 25Mhz,
>> 4 dividers */
>
> Here and below there are quite some definitions still starting with SSB.

Right, I still have some defines to clean/drop in cc pmu.h

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:

> 2011/4/6 Arend van Spriel <>:
>> 1. Term Broadcom AI
>>
>
> I'm still little confused with that, let me read old mails, google a
> little, etc. I though AMBA AXI is AI on ARM host, give me some time
> for this.

It is the interconnect or backplane which the cores in the chip are hooked
up to. See the ARM website for some more info:
http://www.arm.com/products/system-ip/interconnect/axi/index.php

>
>> 2. Bus registration
>>
>
> You should drop initialization (to do not perform it twice), but
> ChipCommon ops are still allowed. See: bcmai_cc_read32,
> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>
> You can simply call:
> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>
> There is nothing stopping you from registering one driver for few
> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
> this is not possible to use 2 drivers for 1 core at the same time.

So in theory 2 drivers for 2 separate cores can both call
bcmai_cc_read32(). 2 drivers for 1 core indeed seems a 'little awkward' ;-)

>
>> 3. Device identification
>>
>> The cores are identified by manufacturer, core id and revision in your
>> patch. I would not use the revision because 4 out of 5 times a revision
>> change does indicate a hardware change but no change in programming
>> interface. The enumeration data does contain a more selective field
>> indicating the core class (4 bits following the core identifier). I
>> suggest
>> to replace the revision field by this class field.
>
> Please tell me sth more about "core class (4 bits following the core
> identifier)". BCMAI_CC_ID_ID is 0x0000FFFF, did you really mean
> 0x000F0000 which is revision? I guess you meant 0x00F00000 which is
> package? Thank you for pointing this, it may be very important. For
> the same reasons I want to have revision, I do not want to miss
> something else that is important. I think we should add package as one
> another core attribute.
>

You found my comment in the code on this. So given your argument above
that this is an ABI set in stone I propose to add the component class
instead of having it replace the revision.

>
>> 4. PCI Host interface
>>
>
> No, it is not for b43 only. You are right of course, I'll rename this.
> It's pretty simple (dumb) driver which is just here just to auto-load
> bcmai module for given PCI IDs. You could just register driver for
> 802.11 core and work fine with b43_pci_ai_bridge aside, but it is not
> correct name for this anyway.
>

ack.

>
>> Now for the code comments, see inline remarks below.
>
>> Probably a different name would be better. bcm suggests this is broadcom
>> specific, but it is hardware functionality provided by ARM. Suggestions:
>> axi, axidmp,amba_axi.
>
> Let me read more abouth this.
>
>
>>> +static u32 bcmai_host_pci_aread32(struct bcmai_device *core, u16
>>> offset)
>>> +{
>>> + if (unlikely(core->bus->mapped_core != core))
>>> + bcmai_host_pci_switch_core(core);
>>> + return ioread32(core->bus->mmio + 0x1000 + offset);
>>> +}
>>
>> Maybe you can replace the 0x1000 with BCMAI_CORE_SIZE (if that was the
>> correct define).
>
> I agree. Probably something like (1 * BCMAI_CORE_SIZE) would be even
> more self-explaining for new developers.
>

great.

>
>
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>>
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> it
>> could be defined in this source file only instead of in a header file.
>
> I though we prefer to keep defines in .h in Linux, let me check (Google)
> for it.
>
>>
>> Probably this list includes some cores that were in old chips, but will
>> never show up in bcmai chips.
>
> Can you point which cores we should keep/drop?

I have that question pending over here.

Gr. AvS
--
"The most merciful thing in the world, I think, is the inability of the
human
mind to correlate all its contents." - "The Call of Cthulhu"

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Arend van Spriel <>:
> On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <> wrote:
>
>> 2011/4/6 Arend van Spriel <>:
>>>
>>> 1. Term Broadcom AI
>>>
>>
>> I'm still little confused with that, let me read old mails, google a
>> little, etc. I though AMBA AXI is AI on ARM host, give me some time
>> for this.
>
> It is the interconnect or backplane which the cores in the chip are hooked
> up to. See the ARM website for some more info:
> http://www.arm.com/products/system-ip/interconnect/axi/index.php
>
>>
>>> 2. Bus registration
>>>
>>
>> You should drop initialization (to do not perform it twice), but
>> ChipCommon ops are still allowed. See: bcmai_cc_read32,
>> bcmai_cc_write32, bcmai_cc_mask32, bcmai_cc_set32, bcmai_cc_maskset32.
>>
>> You can simply call:
>> bcmai_cc_read32(mydev->bus.drv_cc, CC_REGISTER);
>>
>> There is nothing stopping you from registering one driver for few
>> cores. We do this in b43 for old SSBs with 2 wireless cores. Of course
>> this is not possible to use 2 drivers for 1 core at the same time.
>
> So in theory 2 drivers for 2 separate cores can both call bcmai_cc_read32().
> 2 drivers for 1 core indeed seems a 'little awkward' ;-)

If we want to have two drivers working on two (different) cores
simultaneously, we will have to add trivial mutex to group core
switching with core operation (read/write).

We could also use the fact that [base + (2 * BCMAI_CORE_SIZE)] always
points to PCIe and [base + (3 * BCMAI_CORE_SIZE)] always points to
ChipCommon. For that cores we do not need core switching and so we do
not need mutexes. (Don't take my "always" too seriously, just wanted
to explain it easier, this can be not 100% always).

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

2011/4/6 Rafał Miłecki <>:
> If we want to have two drivers working on two (different) cores
> simultaneously, we will have to add trivial mutex to group core
> switching with core operation (read/write).

With a little of work we could avoid switching and mutexes on no-host
boards. MMIO is not limited to one core at once in such a case.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> 2011/4/6 Rafał Miłecki <>:
> > If we want to have two drivers working on two (different) cores
> > simultaneously, we will have to add trivial mutex to group core
> > switching with core operation (read/write).
>
> With a little of work we could avoid switching and mutexes on no-host
> boards. MMIO is not limited to one core at once in such a case.

I don't think that this is a problem at all.
All that magic does happen inside of the bus I/O handlers.
Just like SSB does it.
From a driver point of view, the I/O functions just need to
be atomic.

For SSB it's not always 100% atomic, but we're always safe
due to some assumptions being made. But this is an SSB implementation
detail that is different from AXI. So don't look too closely
at the SSB implementation of the I/O functions. You certainly want
to implement them slightly differently in AXI. SSB currently doesn't
make use of the additional sliding windows, because they are not
available in the majority of SSB devices.

The AXI bus subsystem will manage the sliding windows and the driver
doesn't know about the details.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> 2011/4/6 Rafał Miłecki <>:
>> > If we want to have two drivers working on two (different) cores
>> > simultaneously, we will have to add trivial mutex to group core
>> > switching with core operation (read/write).
>>
>> With a little of work we could avoid switching and mutexes on no-host
>> boards. MMIO is not limited to one core at once in such a case.
>
> I don't think that this is a problem at all.
> All that magic does happen inside of the bus I/O handlers.
> Just like SSB does it.
> From a driver point of view, the I/O functions just need to
> be atomic.
>
> For SSB it's not always 100% atomic, but we're always safe
> due to some assumptions being made. But this is an SSB implementation
> detail that is different from AXI. So don't look too closely
> at the SSB implementation of the I/O functions. You certainly want
> to implement them slightly differently in AXI. SSB currently doesn't
> make use of the additional sliding windows, because they are not
> available in the majority of SSB devices.
>
> The AXI bus subsystem will manage the sliding windows and the driver
> doesn't know about the details.

Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!

In BCMAI:
bcmai_read() {
mutex_get();
switch_core();
ioread();
mutex_release();
}

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> 2011/4/6 Rafał Miłecki <>:
> >> > If we want to have two drivers working on two (different) cores
> >> > simultaneously, we will have to add trivial mutex to group core
> >> > switching with core operation (read/write).
> >>
> >> With a little of work we could avoid switching and mutexes on no-host
> >> boards. MMIO is not limited to one core at once in such a case.
> >
> > I don't think that this is a problem at all.
> > All that magic does happen inside of the bus I/O handlers.
> > Just like SSB does it.
> > From a driver point of view, the I/O functions just need to
> > be atomic.
> >
> > For SSB it's not always 100% atomic, but we're always safe
> > due to some assumptions being made. But this is an SSB implementation
> > detail that is different from AXI. So don't look too closely
> > at the SSB implementation of the I/O functions. You certainly want
> > to implement them slightly differently in AXI. SSB currently doesn't
> > make use of the additional sliding windows, because they are not
> > available in the majority of SSB devices.
> >
> > The AXI bus subsystem will manage the sliding windows and the driver
> > doesn't know about the details.
>
> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>
> In BCMAI:
> bcmai_read() {
> mutex_get();
> switch_core();
> ioread();
> mutex_release();
> }

Yeah that basically is the idea. But it's a little bit harder than that.
The problem is that the mutex cannot be taken in interrupt context.
A spinlock probably is a bit hairy, too, depending on how heavy
a core switch is on AXI.

On SSB we workaround this with some (dirty but working) assumptions.

On AXI you probably can do lockless I/O, if you use the two windows
(how many windows are there?) in a clever way to avoid core switching
completely after the system was initialized.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
>> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
>> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
>> >> 2011/4/6 Rafał Miłecki <>:
>> >> > If we want to have two drivers working on two (different) cores
>> >> > simultaneously, we will have to add trivial mutex to group core
>> >> > switching with core operation (read/write).
>> >>
>> >> With a little of work we could avoid switching and mutexes on no-host
>> >> boards. MMIO is not limited to one core at once in such a case.
>> >
>> > I don't think that this is a problem at all.
>> > All that magic does happen inside of the bus I/O handlers.
>> > Just like SSB does it.
>> > From a driver point of view, the I/O functions just need to
>> > be atomic.
>> >
>> > For SSB it's not always 100% atomic, but we're always safe
>> > due to some assumptions being made. But this is an SSB implementation
>> > detail that is different from AXI. So don't look too closely
>> > at the SSB implementation of the I/O functions. You certainly want
>> > to implement them slightly differently in AXI. SSB currently doesn't
>> > make use of the additional sliding windows, because they are not
>> > available in the majority of SSB devices.
>> >
>> > The AXI bus subsystem will manage the sliding windows and the driver
>> > doesn't know about the details.
>>
>> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
>>
>> In BCMAI:
>> bcmai_read() {
>> mutex_get();
>> switch_core();
>> ioread();
>> mutex_release();
>> }
>
> Yeah that basically is the idea. But it's a little bit harder than that.
> The problem is that the mutex cannot be taken in interrupt context.
> A spinlock probably is a bit hairy, too, depending on how heavy
> a core switch is on AXI.
>
> On SSB we workaround this with some (dirty but working) assumptions.
>
> On AXI you probably can do lockless I/O, if you use the two windows
> (how many windows are there?) in a clever way to avoid core switching
> completely after the system was initialized.

We have 2 windows. I didn't try this, but let's assume they have no
limitations. We can use first window for one driver only, second
driver for second driver only. That gives us 2 drivers simultaneously
working drivers. No driver need to reset core really often (and not
inside interrupt context) so we will switch driver's window to agent
(from core) only at init/reset.

The question is what amount of driver we will need to support at the same time.

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.
>
> The question is what amount of driver we will need to support at the same time.
>

I guess (correct me please, Broadcom guys if I'm wrong) there are two
functions two-head w11 pci host and therefore 4 sliding windows, 2 per
each function.

You really was in need for core switching for PCI SSB hosts, but seem
all that stuff for PCI switching in current bcm80211/utils code is
rudimentary stuff left from PCI times when you was required to use
sliding window for chipcommon and pci bridge core access.

Have nice day,
George



_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

On Wed, 2011-04-06 at 23:12 +0200, Rafał Miłecki wrote:
> W dniu 6 kwietnia 2011 23:08 użytkownik Michael Büsch <> napisał:
> > On Wed, 2011-04-06 at 23:01 +0200, Rafał Miłecki wrote:
> >> W dniu 6 kwietnia 2011 22:57 użytkownik Michael Büsch <> napisał:
> >> > On Wed, 2011-04-06 at 22:42 +0200, Rafał Miłecki wrote:
> >> >> 2011/4/6 Rafał Miłecki <>:
> >> >> > If we want to have two drivers working on two (different) cores
> >> >> > simultaneously, we will have to add trivial mutex to group core
> >> >> > switching with core operation (read/write).
> >> >>
> >> >> With a little of work we could avoid switching and mutexes on no-host
> >> >> boards. MMIO is not limited to one core at once in such a case.
> >> >
> >> > I don't think that this is a problem at all.
> >> > All that magic does happen inside of the bus I/O handlers.
> >> > Just like SSB does it.
> >> > From a driver point of view, the I/O functions just need to
> >> > be atomic.
> >> >
> >> > For SSB it's not always 100% atomic, but we're always safe
> >> > due to some assumptions being made. But this is an SSB implementation
> >> > detail that is different from AXI. So don't look too closely
> >> > at the SSB implementation of the I/O functions. You certainly want
> >> > to implement them slightly differently in AXI. SSB currently doesn't
> >> > make use of the additional sliding windows, because they are not
> >> > available in the majority of SSB devices.
> >> >
> >> > The AXI bus subsystem will manage the sliding windows and the driver
> >> > doesn't know about the details.
> >>
> >> Sure, I've meant mutex inside bcmai (or whatever name), not on the driver side!
> >>
> >> In BCMAI:
> >> bcmai_read() {
> >> mutex_get();
> >> switch_core();
> >> ioread();
> >> mutex_release();
> >> }
> >
> > Yeah that basically is the idea. But it's a little bit harder than that.
> > The problem is that the mutex cannot be taken in interrupt context.
> > A spinlock probably is a bit hairy, too, depending on how heavy
> > a core switch is on AXI.
> >
> > On SSB we workaround this with some (dirty but working) assumptions.
> >
> > On AXI you probably can do lockless I/O, if you use the two windows
> > (how many windows are there?) in a clever way to avoid core switching
> > completely after the system was initialized.
>
> We have 2 windows. I didn't try this, but let's assume they have no
> limitations. We can use first window for one driver only, second
> driver for second driver only. That gives us 2 drivers simultaneously
> working drivers. No driver need to reset core really often (and not
> inside interrupt context) so we will switch driver's window to agent
> (from core) only at init/reset.

On SSB one major assumption is that on non-embedded no coreswitch will
happen after init. (On embedded, the I/O accesses are safe anyway,
because all cores are always mapped).

> The question is what amount of driver we will need to support at the same time.

I guess (guess!) that the answer will be similar to SSB:
On non-embedded after init only one core is accessed: The wireless core.
And on embedded cores are accessed in random order. But it's safe
because the MMIO is always mapped.
Does this also apply to AXI?
Even if we would need to access two cores after init (wireless and
chipcommon, perhaps), we would use the two windows to survive without
an actual core switch while running.

--
Greetings Michael.

_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 6 kwietnia 2011 23:18 użytkownik George Kashperko
<> napisał:
>> We have 2 windows. I didn't try this, but let's assume they have no
>> limitations. We can use first window for one driver only, second
>> driver for second driver only. That gives us 2 drivers simultaneously
>> working drivers. No driver need to reset core really often (and not
>> inside interrupt context) so we will switch driver's window to agent
>> (from core) only at init/reset.
>>
>> The question is what amount of driver we will need to support at the same time.
>>
>
> I guess (correct me please, Broadcom guys if I'm wrong) there are two
> functions two-head w11 pci host and therefore 4 sliding windows, 2 per
> each function.

I don't understand you. Can you use more friendly language? functions?
2head? w11?

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 6 kwietnia 2011 23:18 użytkownik George Kashperko
> <> napisał:
> >> We have 2 windows. I didn't try this, but let's assume they have no
> >> limitations. We can use first window for one driver only, second
> >> driver for second driver only. That gives us 2 drivers simultaneously
> >> working drivers. No driver need to reset core really often (and not
> >> inside interrupt context) so we will switch driver's window to agent
> >> (from core) only at init/reset.
> >>
> >> The question is what amount of driver we will need to support at the same time.
> >>
> >
> > I guess (correct me please, Broadcom guys if I'm wrong) there are two
> > functions two-head w11 pci host and therefore 4 sliding windows, 2 per
> > each function.
>
> I don't understand you. Can you use more friendly language? functions?
> 2head? w11?
>

For PCI function description take a look at PCI specs or PCI
configuration space description (e. g.
http://en.wikipedia.org/wiki/PCI_configuration_space)

Sorry for missleading short-ups, w11 - bcm80211 core, under two-head I
mean ssb/axi with two functional cores on same interconnect (like w11
+w11, not a lot of these exists I guess). Also there were some b43+b44
on single PCI ssb host and those where implemented as ssb interconnect
on multifunctional PCI host therefore providing separate access windows
for each function.

Might I mussunderstood something (its late night here at my place) when
you where talking about using coreswitching involved for two drivers
therefore I remembered about those functions. Seems now you were talking
about chipcommon+b43 access sharing same window.

As for core switching requirments for earlier SSB interconnects on PCI
hosts where there were no direct chipcommon access, that one can be
accomplished without spin_lock/mutex for b43 or b44 cores with proper
bus design.

AXI doesn't need spinlocks/mutexes as both chipcommon and pci bridge are
available directly and b43 will be the only one requiring window access.

Have nice day,
George


_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

W dniu 7 kwietnia 2011 02:00 użytkownik George Kashperko
<> napisał:
> For PCI function description take a look at PCI specs or PCI
> configuration space description (e. g.
> http://en.wikipedia.org/wiki/PCI_configuration_space)
>
> Sorry for missleading short-ups, w11 - bcm80211 core, under two-head I
> mean ssb/axi with two functional cores on same interconnect (like w11
> +w11, not a lot of these exists I guess). Also there were some b43+b44
> on single PCI ssb host and those where implemented as ssb interconnect
> on multifunctional PCI host therefore providing separate access windows
> for each function.
>
> Might I mussunderstood something (its late night here at my place) when
> you where talking about using coreswitching involved for two drivers
> therefore I remembered about those functions. Seems now you were talking
> about chipcommon+b43 access sharing same window.
>
> As for core switching requirments for earlier SSB interconnects on PCI
> hosts where there were no direct chipcommon access, that one can be
> accomplished without spin_lock/mutex for b43 or b44 cores with proper
> bus design.
>
> AXI doesn't need spinlocks/mutexes as both chipcommon and pci bridge are
> available directly and b43 will be the only one requiring window access.

Ahh, so while talking about 4 windows, I guess you counted fixes
windows as well. That would be right, matching my knowledge.

When asking question about amount of cores we may want to use
simultaneously I didn't think about ChipCommon or PCIe. The real
problem would be to support for example two 802.11 cores and one
ethernet core at the same time. That gives us 3 cores while we have
only 2 sliding windows. But I would not care for that too much for
now.

For the rest (PCI function) I have to sleep before reading specs ;)

--
Rafał
_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


> W dniu 7 kwietnia 2011 02:00 użytkownik George Kashperko
> <> napisał:
> > For PCI function description take a look at PCI specs or PCI
> > configuration space description (e. g.
> > http://en.wikipedia.org/wiki/PCI_configuration_space)
> >
> > Sorry for missleading short-ups, w11 - bcm80211 core, under two-head I
> > mean ssb/axi with two functional cores on same interconnect (like w11
> > +w11, not a lot of these exists I guess). Also there were some b43+b44
> > on single PCI ssb host and those where implemented as ssb interconnect
> > on multifunctional PCI host therefore providing separate access windows
> > for each function.
> >
> > Might I mussunderstood something (its late night here at my place) when
> > you where talking about using coreswitching involved for two drivers
> > therefore I remembered about those functions. Seems now you were talking
> > about chipcommon+b43 access sharing same window.
> >
> > As for core switching requirments for earlier SSB interconnects on PCI
> > hosts where there were no direct chipcommon access, that one can be
> > accomplished without spin_lock/mutex for b43 or b44 cores with proper
> > bus design.
> >
> > AXI doesn't need spinlocks/mutexes as both chipcommon and pci bridge are
> > available directly and b43 will be the only one requiring window access.
>
> Ahh, so while talking about 4 windows, I guess you counted fixes
> windows as well. That would be right, matching my knowledge.
Nop, I mean sliding windows only.

>
> When asking question about amount of cores we may want to use
> simultaneously I didn't think about ChipCommon or PCIe. The real
> problem would be to support for example two 802.11 cores and one
> ethernet core at the same time. That gives us 3 cores while we have
> only 2 sliding windows. But I would not care for that too much for
> now.
That will give us 3 PCI functions, each with own sliding windows.

>
> For the rest (PCI function) I have to sleep before reading specs ;)
>

Have nice day,
George


_______________________________________________
devel mailing list

http://driverdev.linuxdriverproject.org/mailman/listinfo/devel