[RFC][PATCH] bcmai: introduce AI driver
Arend van Spriel
arend at broadcom.com
Wed Apr 6 13:25:33 PDT 2011
On Wed, 06 Apr 2011 20:02:20 +0200, Rafał Miłecki <zajec5 at gmail.com> wrote:
> 2011/4/6 Arend van Spriel <arend at broadcom.com>:
>> 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:
>> 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
>> 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.
>> 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
>>> + 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.
>>> + bcmai_scan_switch_core(bus, BCMAI_ADDR_BASE);
>> Is BCMAI_ADDR_BASE used in other source file in this module? Otherwise,
>> 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.
"The most merciful thing in the world, I think, is the inability of the
mind to correlate all its contents." - "The Call of Cthulhu"
More information about the devel