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

From: Andi Kleen
Date: Wed Jun 24 2009 - 03:12:30 EST


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@xxxxxxxxxxxxxxx -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/