Re: [RFC][PATCH] bcmai: introduce AI driver

[Arend van Spriel]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/