Messages in this thread |  | | Date | Wed, 24 Jun 2009 11:34:40 +0800 | From | Feng Tang <> | Subject | Re: [PATCH 3/8] SFI: core support |
| |
On Tue, 23 Jun 2009 20:32:56 +0800 Andi Kleen <andi@firstfloor.org> wrote:
> > +static void __iomem *sfi_map_memory(u32 phys, u32 size) > > +{ > > + if (!phys || !size) > > + return NULL; > > + > > + if (sfi_tbl_permanant_mapped) > > > > > + return ioremap((unsigned long)phys, size); > > + else > > + return arch_early_ioremap((unsigned long)phys, > > size); +} > > imho it would be cleaner if the callers just called these functions > directly. Are the !phys !size checks really needed?
Andi,
Thanks for many good comments, will address them.
For this question, this sfi_map_memory() may get called before and after the ioremap() is ready, so we add a permanent flag to judge the environment and chose the right API automatically. e.g. after system is booted, cpu freq driver will implicitly call this API to get freq info > > > > + > > +void sfi_tb_install_table(u64 addr, u32 flags) > > +{ > > + struct sfi_table_header *table; > > + u32 length; > > + > > + /* only map table header before knowing actual length */ > > + table = sfi_map_memory(addr, sizeof(struct > > sfi_table_header)); > > + if (!table) > > + return; > > + > > + length = table->length; > > + sfi_unmap_memory(table, sizeof(struct sfi_table_header)); > > + > > + table = sfi_map_memory(addr, length); > > + if (!table) > > + return; > > > Since the mappings are always 4K you would only need to remap > if the size is > PAGE_SIZE
yes, some of the table may be in one page, but some may not start at page boundary and cross pages, we do it this way as this map/unmap/remap/unmap routine only happen few times in boot phase.
> > > + > > + if (sfi_tb_verify_checksum(table, length)) > > + goto unmap_and_exit; > > + > > + /* Initialize sfi_tblist entry */ > > + sfi_tblist.tables[sfi_tblist.count].flags = flags; > > + sfi_tblist.tables[sfi_tblist.count].address = addr; > > + sfi_tblist.tables[sfi_tblist.count].pointer = NULL; > > + memcpy(&sfi_tblist.tables[sfi_tblist.count].header, > > + table, sizeof(struct sfi_table_header)); > > To be honest I'm not sure why this list exists at all. > Is it that difficult to just rewalk the firmware supplied > table as needed?
Currently, there are about 10 SFI tables (more are expected), and most of them will be parsed in driver initialization phase, like timer/cpu idle/ cpu frequency/rtc/system wake driver. Using a global list may save some system overhead
Thanks, Feng > > > -andi
|  |