On Wed, 06 Apr 2011 20:02:20 +0200, RafaÅ MiÅecki <zajec5@xxxxxxxxx> wrote:
2011/4/6 Arend van Spriel <arend@xxxxxxxxxxxx>:
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"
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
