Messages in this thread | | | Date | Wed, 24 Jun 2009 15:40:52 +0800 | From | Feng Tang <> | Subject | Re: [PATCH 3/8] SFI: core support |
| |
On Wed, 24 Jun 2009 15:12:20 +0800 Andi Kleen <andi@firstfloor.org> wrote:
> On Wed, Jun 24, 2009 at 11:34:40AM +0800, Feng Tang 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 > > Yes, but the callers should know this and they can call the right > function. I suspect only very few callers will need the early > variant. > There is one sfi_table_parse() API, which is a SFI core function, it is exported out and used in both boot phase (parsing cpu/ioapic) and later driver phase (parsing idle/freq ...), when it get called, it doesn't know in which phase it get called, and need such a flag to judge.
> > 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 > > cpufreq driver shouldn't be initialized before ioremap
Right, it called the sfi_table_parse() after ioremap is ready.
> > > > > > > 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. > > The TLB flushes tend to be a few thousand cycles at least. > > It's not much, but with all the recent focus on faster boot times it's > still better to not write unnecessarily inefficient initialization > code.
good point, will take care of it
> > > > > > > > > > + > > > > + 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 > > Walking the tables as they are laid out in memory should be quite > equivalent to walking a list, shouldn't it? > > It would be only a relatively small simplification agreed, but if > you're claiming to do a "Simple Firmware Interface" imho you should > try to make it as simple possible, and that includes not setting up > redundant data structures.
understand your concern, but to walk a list we still need have some global parameter like SYST address, and do the map/unmap and checksum work.
another reason for the global sfi_table_desc[] is, we only do some time ioremap for each table and save the mapped address for future use. this idea is borrowed from ACPI table handling.
Thanks, Feng
> > -Andi
| |