Re: [flashrom] [PATCH 3/3] Refine physical address mapping of flash chips.

[Stefan Tauner]

On Sun, 17 Aug 2014 01:09:00 +0200
Carl-Daniel Hailfinger  wrote:
> Am 16.08.2014 01:43 schrieb Stefan Tauner:
> > On Fri, 15 Aug 2014 01:09:44 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > > -#if CONFIG_INTERNAL == 1
> > > > if (programmer_table[programmer].map_flash_region == physmap)
> > > > snprintf(location, sizeof(location), "at physical address 0x%lx", base);
> > > > else
> > > >
> > > > -#endif
> > > > snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
> > >
> > > That info should not have disappeared, and we may soon have to extend it
> > > for funny constellations like two chips in a programmer (EC flash and
> > > chipset flash for laptops).
> >
> > If this is an important detail we want to tell the user then we need to
> > define a proper interface... there is no reason at all to know the base
> > address in the probing function and that's why I have removed that bit
> > completely, and because I don't see how this is important at all at the
> > msg_pinfo level(!).
>
> What about dual flash chip setups in the FWH/LPC case? There we have
> multiple flash chips at different addresses, e.g. a 2 MByte chip at
> 4G-4M and a 2 MByte chip at 4G-2M. Yes, such hardware exists. It's the
> reason we have the feature to optionally set FWH IDSEL on ICH for
> misconfigured boards of that type. Admittedly the LPC case is a bit less
> weird and works by strapping LPC chips to the correct address.

Well OK, so we still want to print that but not necessarily at info
level. The problem is that the cli code does not know the base address,
not even the probing code knows it (and that's a good thing!). That's a
problem for the print though. The pseudo code of the old probe (with
this (refined) patch applied looks like this:

cli_classic.c:
loop over chips, call probe_flash with incremented index
...
if a single flash chip was detected then {
    if (probe only)
    	exit()
    map_flash()
    doit()
    unmap_flash()	
}

flashrom.c:
probe_flash:
loop over chips starting at the given index {
    map_flash()
    call chip probe function()
    unmap_flash()
}

Only map_flash() knows the base address but now we want to print it in
cli_classic.c.

We have at least these options:
 - print it all the time in map_flash() but on a high verbosity level
   like dbg2 or spew (BTW on spew it is already visible due to the
   output in programmer_map_flash_region())
 - refine map_flash and add a bool print parameter
 - add a get_map_base() function that can be called in map_flash() and
   gets exported to be callable in cli_classic etc. as well.
> > I would be ok with printing the programmer (well,
> > the master would be better if that's already possible... do they
> > have names already? :) ... but the base address printing needs to die or
> > be moved elsewhere.
>
> Indeed, we should print the master name, not the programmer name. I
> thought I had a patch doing that, but digging through my archive it
> seems I can't find that patch anymore and/or it was a figment of my
> imagination.

That can be tackled later though.