Re: [PATCH 3/8] SFI: core support

[Feng Tang <feng.tang@intel.com>]

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