On 06/06/2011 12:22 PM, RafaÅ MiÅecki wrote:
> Hauke,
>
> My idea for naming schema was to use:
> bcma_host_TYPE_*
>
> Like:
> bcma_host_pci_*
> bcma_host_sdio_*
>
> You are using:
> bcma_host_bcma_*
>
> What do you think about changing this to:
> bcma_host_embedded_*
> or just some:
> bcma_host_emb_*
> ?
>
> Does it make more sense to you? I was trying to keep names in bcma
> really clear, so every first-time-reader can see differences between
> hosts, host and driver, etc.
>
At first I named it bcma_host_bcma_ but then renamed the file name, but
forgot the function names. I will rename it all to bcma_host_soc_*
host_sco.c and so on.
> 2011/6/6 Hauke Mehrtens <hauke@xxxxxxxxxx>:
>> --- /dev/null
>> +++ b/drivers/bcma/host_embedded.c
>> @@ -0,0 +1,93 @@
>> +/*
>> + * Broadcom specific AMBA
>> + * PCI Host
>
> s/PCI/Embedded/
>
>
>> +int bcma_host_bcma_register(struct bcma_bus *bus)
>> +{
>> + u32 __iomem *mmio;
>> + /* iomap only first core. We have to read some register on this core
>> + * to get the number of cores. This is sone in bcma_scan()
>> + */
>> + mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * 1);
>> + if (!mmio)
>> + return -ENOMEM;
>> + bus->mmio = mmio;
>
> Maybe just:
> bus->mmio = ioremap(...);
> ? :)
yes makes sens.
>
>> + /* Host specific */
>> + bus->hosttype = BCMA_HOSTTYPE_EMBEDDED;
>> + bus->ops = &bcma_host_bcma_ops;
>> +
>> + /* Register */
>> + return bcma_bus_register(bus);
>> +}
>> diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
>> index 1afa107..c5bcb5f 100644
>> --- a/drivers/bcma/main.c
>> +++ b/drivers/bcma/main.c
>> @@ -119,6 +119,7 @@ static int bcma_register_cores(struct bcma_bus *bus)
>> break;
>> case BCMA_HOSTTYPE_NONE:
>> case BCMA_HOSTTYPE_SDIO:
>> + case BCMA_HOSTTYPE_EMBEDDED:
>> break;
>> }
>>
>> diff --git a/drivers/bcma/scan.c b/drivers/bcma/scan.c
>> index 70b39f7..9229615 100644
>> --- a/drivers/bcma/scan.c
>> +++ b/drivers/bcma/scan.c
>> @@ -203,7 +203,7 @@ static s32 bcma_erom_get_addr_desc(struct bcma_bus *bus, u32 **eromptr,
>> int bcma_bus_scan(struct bcma_bus *bus)
>> {
>> u32 erombase;
>> - u32 __iomem *eromptr, *eromend;
>> + u32 __iomem *eromptr, *eromend, *mmio;
>>
>> s32 cia, cib;
>> u8 ports[2], wrappers[2];
>> @@ -219,9 +219,34 @@ int bcma_bus_scan(struct bcma_bus *bus)
>> bus->chipinfo.id = (tmp & BCMA_CC_ID_ID) >> BCMA_CC_ID_ID_SHIFT;
>> bus->chipinfo.rev = (tmp & BCMA_CC_ID_REV) >> BCMA_CC_ID_REV_SHIFT;
>> bus->chipinfo.pkg = (tmp & BCMA_CC_ID_PKG) >> BCMA_CC_ID_PKG_SHIFT;
>> + bus->nr_cores = (tmp & BCMA_CC_ID_NRCORES) >> BCMA_CC_ID_NRCORES_SHIFT;
>
> I'd use different variable as Julian suggested.
yes
>
>
>> +
>> + /* If we are an embedded device we now know the number of avaliable
>> + * core and ioremap the correct space.
>> + */
>
> Typo: avaliable
my favorite typo ;-)
>
>
>> + if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
>> + iounmap(bus->mmio);
>> + mmio = ioremap(BCMA_ADDR_BASE, BCMA_CORE_SIZE * bus->nr_cores);
>> + if (!mmio)
>> + return -ENOMEM;
>> + bus->mmio = mmio;
>> +
>> + mmio = ioremap(BCMA_WRAP_BASE, BCMA_CORE_SIZE * bus->nr_cores);
>> + if (!mmio)
>> + return -ENOMEM;
>> + bus->host_embedded = mmio;
>
> Do we really need both? mmio and host_embedded? What about keeping
> mmio only and using it in calculation for read/write[8,16,32]?
These are two different memory regions, it should be possible to
calculate the other address, but I do not like that. As host_embedded is
in a union this does not waste any memory.
>
>> + }
>> + /* reset it to 0 as we use it for counting */
>> + bus->nr_cores = 0;
>>
>> erombase = bcma_scan_read32(bus, 0, BCMA_CC_EROM);
>> - eromptr = bus->mmio;
>> + if (bus->hosttype == BCMA_HOSTTYPE_EMBEDDED) {
>> + eromptr = ioremap(erombase, BCMA_CORE_SIZE);
>> + if (!eromptr)
>> + return -ENOMEM;
>> + } else
>> + eromptr = bus->mmio;
>
> I though eromptr = bus->mmio; will do the trick for embedded as well.
> I think I need some time to read about IO mapping and understand that.
>
No they are different eromptr is 0x1810e000 and bus->mmio is 0xb8000000
for example on my device. I tried using eromptr = bus->mmio; on embedded
and it did not work.
Hauke
