lkml.org 
[lkml]   [2009]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/8] SFI: core support
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.

> 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

> >
> > 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.

>
> >
> > > +
> > > + 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.

-Andi
--
ak@linux.intel.com -- Speaking for myself only.


\
 
 \ /
  Last update: 2009-06-24 09:15    [W:0.071 / U:1.540 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site