| Message ID | 1307311658-15853-4-git-send-email-hauke@hauke-m.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hauke, Minor nit: On Mon, Jun 6, 2011 at 08:07, Hauke Mehrtens <hauke@hauke-m.de> wrote: > This patch adds support for using bcma on an embedded bus. An embedded > system like the bcm4716 could register this bus and it searches for the > bcma cores then. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > 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 > @@ -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; > + > + /* If we are an embedded device we now know the number of avaliable > + * core and ioremap the correct space. > + */ > + 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; > + } > + /* reset it to 0 as we use it for counting */ > + bus->nr_cores = 0; Would it make sense to use a local variable for nr_cores, and only use it within the BCMA_HOSTTYPE_EMBEDDED if statement, rather than re-using bus->nr_cores and having to reset it? Thanks,
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. 2011/6/6 Hauke Mehrtens <hauke@hauke-m.de>: > --- /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(...); ? :) > + /* 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. > + > + /* If we are an embedded device we now know the number of avaliable > + * core and ioremap the correct space. > + */ Typo: avaliable > + 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]? > + } > + /* 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.
Hi, > 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. how about bcma_host_soc ? > > > 2011/6/6 Hauke Mehrtens <hauke@hauke-m.de>: > > --- /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(...); > ? :) > > > > + /* 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; > To avoid using wrapper struct and at the same time to save on embedded reservations you could let the bus get scanned twice on SoC - first time discovering just system devices (chipcommon and mips core) required for early setup (you will never register those to the linux device subsystem so you can have them marked as __initdata and have no ->release callback therefore), second time full scan with registering the whole bus when done. Have nice day, George -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
W dniu 6 czerwca 2011 12:32 u?ytkownik George Kashperko <george@znau.edu.ua> napisa?: > Hi, > >> 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. > > how about bcma_host_soc ? We get then inconsistency with "BCMA_HOSTTYPE_EMBEDDED". I'd like to 1) See something like bcma_host_emb... xor 2) Use bcma_host_soc_* and BCMA_HOSTTYPE_SOC
On 06/06/2011 12:51 PM, Rafa? Mi?ecki wrote: > W dniu 6 czerwca 2011 12:32 u?ytkownik George Kashperko > <george@znau.edu.ua> napisa?: >> Hi, >> >>> 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. >> how about bcma_host_soc ? > We get then inconsistency with "BCMA_HOSTTYPE_EMBEDDED". I'd like to > 1) See something like bcma_host_emb... > xor > 2) Use bcma_host_soc_* and BCMA_HOSTTYPE_SOC > I would go for option 2). It more clearly says what it is. Embedded is a broader term. As an example, a handset is an embedded device, but it may use BCMA_HOSTTYPE_SDIO. Gr. AvS
W dniu 6 czerwca 2011 12:55 u?ytkownik Arend van Spriel <arend@broadcom.com> napisa?: > On 06/06/2011 12:51 PM, Rafa? Mi?ecki wrote: >> >> W dniu 6 czerwca 2011 12:32 u?ytkownik George Kashperko >> <george@znau.edu.ua> napisa?: >>> >>> Hi, >>> >>>> 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. >>> >>> how about bcma_host_soc ? >> >> We get then inconsistency with "BCMA_HOSTTYPE_EMBEDDED". I'd like to >> 1) See something like bcma_host_emb... >> xor >> 2) Use bcma_host_soc_* and BCMA_HOSTTYPE_SOC >> > > I would go for option 2). It more clearly says what it is. Embedded is a > broader term. As an example, a handset is an embedded device, but it may use > BCMA_HOSTTYPE_SDIO. Good point, agree.
On 06/06/2011 01:22 AM, Julian Calaby wrote: > Hauke, > > Minor nit: > > On Mon, Jun 6, 2011 at 08:07, Hauke Mehrtens <hauke@hauke-m.de> wrote: >> This patch adds support for using bcma on an embedded bus. An embedded >> system like the bcm4716 could register this bus and it searches for the >> bcma cores then. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> --- >> 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 >> @@ -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; >> + >> + /* If we are an embedded device we now know the number of avaliable >> + * core and ioremap the correct space. >> + */ >> + 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; >> + } >> + /* reset it to 0 as we use it for counting */ >> + bus->nr_cores = 0; > > Would it make sense to use a local variable for nr_cores, and only use > it within the BCMA_HOSTTYPE_EMBEDDED if statement, rather than > re-using bus->nr_cores and having to reset it? Yes that looks better. Hauke -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@hauke-m.de>: >> --- /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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
W dniu 7 czerwca 2011 00:00 u?ytkownik Hauke Mehrtens <hauke@hauke-m.de> napisa?: > On 06/06/2011 12:22 PM, Rafa? Mi?ecki wrote: >>> + 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. Ah, OK, I can see what does happen here. You are using: 1) bus->mmio for first core 2) bus->host_embedded for first agent/wrapper I'm not sure if this is a correct approach. Doing "core_index * BCMA_CORE_SIZE" comes from ssb, where it was the way to calculate offset. In case of BCMA we are reading all the info from (E)EPROM, which also includes addresses of the cores. IMO you should use core->addr and core->wrap for read/write ops. I believe this is approach Broadcom decided to use for BCMA, when designing (E)EPROM. You should not need bus->host_embedded then, maybe you could even do not set bus->mmio?
On 06/07/2011 02:33 AM, Rafa? Mi?ecki wrote: > W dniu 7 czerwca 2011 00:00 u?ytkownik Hauke Mehrtens > <hauke@hauke-m.de> napisa?: >> On 06/06/2011 12:22 PM, Rafa? Mi?ecki wrote: >>>> + 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. > Ah, OK, I can see what does happen here. You are using: > 1) bus->mmio for first core > 2) bus->host_embedded for first agent/wrapper > > I'm not sure if this is a correct approach. Doing "core_index * > BCMA_CORE_SIZE" comes from ssb, where it was the way to calculate > offset. In case of BCMA we are reading all the info from (E)EPROM, > which also includes addresses of the cores. > > IMO you should use core->addr and core->wrap for read/write ops. I > believe this is approach Broadcom decided to use for BCMA, when > designing (E)EPROM. Agree. There is no guarantee for the core index to relate to the physical address. Chip designer may be systematic in this and the index*size method may work, but not by design. Gr. AvS
On 06/07/2011 12:30 PM, Arend van Spriel wrote: > On 06/07/2011 02:33 AM, Rafa? Mi?ecki wrote: >> W dniu 7 czerwca 2011 00:00 u?ytkownik Hauke Mehrtens >> <hauke@hauke-m.de> napisa?: >>> On 06/06/2011 12:22 PM, Rafa? Mi?ecki wrote: >>>>> + 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. >> Ah, OK, I can see what does happen here. You are using: >> 1) bus->mmio for first core >> 2) bus->host_embedded for first agent/wrapper >> >> I'm not sure if this is a correct approach. Doing "core_index * >> BCMA_CORE_SIZE" comes from ssb, where it was the way to calculate >> offset. In case of BCMA we are reading all the info from (E)EPROM, >> which also includes addresses of the cores. >> >> IMO you should use core->addr and core->wrap for read/write ops. I >> believe this is approach Broadcom decided to use for BCMA, when >> designing (E)EPROM. > > Agree. There is no guarantee for the core index to relate to the > physical address. Chip designer may be systematic in this and the > index*size method may work, but not by design. > > Gr. AvS > Ok I will use these addresses. Hauke -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/bcma/Kconfig b/drivers/bcma/Kconfig index 83e9adf..0390e32 100644 --- a/drivers/bcma/Kconfig +++ b/drivers/bcma/Kconfig @@ -27,6 +27,11 @@ config BCMA_HOST_PCI bool "Support for BCMA on PCI-host bus" depends on BCMA_HOST_PCI_POSSIBLE +config BCMA_HOST_EMBEDDED + bool + depends on BCMA && MIPS + default n + config BCMA_DEBUG bool "BCMA debugging" depends on BCMA diff --git a/drivers/bcma/Makefile b/drivers/bcma/Makefile index 0d56245..e509b1b 100644 --- a/drivers/bcma/Makefile +++ b/drivers/bcma/Makefile @@ -2,6 +2,7 @@ bcma-y += main.o scan.o core.o bcma-y += driver_chipcommon.o driver_chipcommon_pmu.o bcma-y += driver_pci.o bcma-$(CONFIG_BCMA_HOST_PCI) += host_pci.o +bcma-$(CONFIG_BCMA_HOST_EMBEDDED) += host_embedded.o obj-$(CONFIG_BCMA) += bcma.o ccflags-$(CONFIG_BCMA_DEBUG) := -DDEBUG diff --git a/drivers/bcma/host_embedded.c b/drivers/bcma/host_embedded.c new file mode 100644 index 0000000..6942440 --- /dev/null +++ b/drivers/bcma/host_embedded.c @@ -0,0 +1,93 @@ +/* + * Broadcom specific AMBA + * PCI Host + * + * Licensed under the GNU/GPL. See COPYING for details. + */ + +#include "bcma_private.h" +#include "scan.h" +#include <linux/bcma/bcma.h> + +static u8 bcma_host_bcma_read8(struct bcma_device *core, u16 offset) +{ + offset += core->core_index * BCMA_CORE_SIZE; + return readb(core->bus->mmio + offset); +} + +static u16 bcma_host_bcma_read16(struct bcma_device *core, u16 offset) +{ + offset += core->core_index * BCMA_CORE_SIZE; + return readw(core->bus->mmio + offset); +} + +static u32 bcma_host_bcma_read32(struct bcma_device *core, u16 offset) +{ + offset += core->core_index * BCMA_CORE_SIZE; + return readl(core->bus->mmio + offset); +} + +static void bcma_host_bcma_write8(struct bcma_device *core, u16 offset, + u8 value) +{ + offset += core->core_index * BCMA_CORE_SIZE; + writeb(value, core->bus->mmio + offset); +} + +static void bcma_host_bcma_write16(struct bcma_device *core, u16 offset, + u16 value) +{ + offset += core->core_index * BCMA_CORE_SIZE; + writew(value, core->bus->mmio + offset); +} + +static void bcma_host_bcma_write32(struct bcma_device *core, u16 offset, + u32 value) +{ + offset += core->core_index * BCMA_CORE_SIZE; + writel(value, core->bus->mmio + offset); +} + +static u32 bcma_host_bcma_aread32(struct bcma_device *core, u16 offset) +{ + offset += core->core_index * BCMA_CORE_SIZE; + return readl(core->bus->host_embedded + offset); +} + +static void bcma_host_bcma_awrite32(struct bcma_device *core, u16 offset, + u32 value) +{ + offset += core->core_index * BCMA_CORE_SIZE; + writel(value, core->bus->host_embedded + offset); +} + +const struct bcma_host_ops bcma_host_bcma_ops = { + .read8 = bcma_host_bcma_read8, + .read16 = bcma_host_bcma_read16, + .read32 = bcma_host_bcma_read32, + .write8 = bcma_host_bcma_write8, + .write16 = bcma_host_bcma_write16, + .write32 = bcma_host_bcma_write32, + .aread32 = bcma_host_bcma_aread32, + .awrite32 = bcma_host_bcma_awrite32, +}; + +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; + + /* 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; + + /* If we are an embedded device we now know the number of avaliable + * core and ioremap the correct space. + */ + 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; + } + /* 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; + eromend = eromptr + BCMA_CORE_SIZE / sizeof(u32); bcma_scan_switch_core(bus, erombase); diff --git a/include/linux/bcma/bcma.h b/include/linux/bcma/bcma.h index 8b6feca..192c4ae 100644 --- a/include/linux/bcma/bcma.h +++ b/include/linux/bcma/bcma.h @@ -16,6 +16,7 @@ enum bcma_hosttype { BCMA_HOSTTYPE_NONE, BCMA_HOSTTYPE_PCI, BCMA_HOSTTYPE_SDIO, + BCMA_HOSTTYPE_EMBEDDED, }; struct bcma_chipinfo { @@ -185,6 +186,8 @@ struct bcma_bus { struct pci_dev *host_pci; /* Pointer to the SDIO device (only for BCMA_HOSTTYPE_SDIO) */ struct sdio_func *host_sdio; + /* Pointer to the embedded iomem (only for BCMA_HOSTTYPE_EMBEDDED) */ + void __iomem *host_embedded; }; struct bcma_chipinfo chipinfo; diff --git a/include/linux/bcma/bcma_embedded.h b/include/linux/bcma/bcma_embedded.h new file mode 100644 index 0000000..0faf46d --- /dev/null +++ b/include/linux/bcma/bcma_embedded.h @@ -0,0 +1,8 @@ +#ifndef LINUX_BCMA_EMBEDDED_H_ +#define LINUX_BCMA_EMBEDDED_H_ + +#include <linux/bcma/bcma.h> + +extern int bcma_host_bcma_register(struct bcma_bus *bus); + +#endif /* LINUX_BCMA_EMBEDDED_H_ */
This patch adds support for using bcma on an embedded bus. An embedded system like the bcm4716 could register this bus and it searches for the bcma cores then. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- drivers/bcma/Kconfig | 5 ++ drivers/bcma/Makefile | 1 + drivers/bcma/host_embedded.c | 93 ++++++++++++++++++++++++++++++++++++ drivers/bcma/main.c | 1 + drivers/bcma/scan.c | 29 ++++++++++- include/linux/bcma/bcma.h | 3 + include/linux/bcma/bcma_embedded.h | 8 +++ 7 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 drivers/bcma/host_embedded.c create mode 100644 include/linux/bcma/bcma_embedded.h