[flashrom] [PATCH] Register Parallel/LPC/FWH programmers

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Nov 10 00:47:36 CET 2011


Am 09.11.2011 23:19 schrieb Michael Karcher:
> Am Montag, den 07.11.2011, 14:47 +0100 schrieb Carl-Daniel Hailfinger:
>>>>> - Should register_par_programmer(...) be called before or after setting
>>>>>   max_rom_decode.*?
>>>> why is that not a field in the different programmer structs (yet?)?
>>> Supply it as
>>> register_par_programmer() parameter and extend register_spi_programmer
>>> to accept that parameter as well?
>> I'm now setting max_rom_decode before calling register_par_programmer.
>> Given that such restrictions may exist for any programmer, I'll postpone
>> handling max_rom_decode to the universal programmer registration patch
>> where we get the infrastructure to deal with such programmer
>> limitations. If you have any objections, please tell me.
> As we are heading strongly towards having a dynamic list of programmers,
> and there might even be two SPI programmers with different max decode
> size, storing it with the programmer structure seems like the only sane
> option. Having the registeration function fill a field in the dynamic
> programmer structure using a parameter is one sensible choice to get
> that field populated.

Indeed, that's the plan. I'll post a followup patch for this.


>>>>> - Is there a better name for register_par_programmer?
>>>>>     
>>>> register_parallel_programmer ofc, and imho it is not too long, because
>>>> it is seldom used, but i don't care that much (due to the same reason).
>>>>   
>>> 80 column limit... I think that was one of the reasons we used a shorter
>>> name.
> I am not really convinced "parallel" is to the point. While even LPC/FWH
> is still using 4 bits in parallel, the real distinction between
> parallel/LPC/FWH and SPI is that parallel/LPC/FWH is pushing commands
> through a memory read/write interface, while SPI pushes memory
> read/writes through a command interface. (At least if you look at the
> layer of abstraction applying to flashrom)
>
> But "register_memcycle_programmer" is most likely something people are
> not going to understand, although, looking at the contents of your
> par_programmer structure, all the functions really are about memory
> cylces.

There are three types of programmers:
In-band commands (Parallel/LPC/FWH)
Out-of-band commands (SPI/I2C)
Opaque programmers (ICH HWSeq)

All in-band programmers have a common way to interface with the flash
chip. Sharing a common registration function makes sense, and I'm not
too happy with the name "par" either. Admittedly people outside the x86
world usually know only Parallel flash and SPI flash, so for them a name
referencing parallel flash is easiest to understand. OTOH, we could use
"ib" or "inband" instead of "par", but I'm not sure if that is
understood more easily.
Out-of-band programmers all share a command submission mechanism which
is used for all accesses, but the mechanism differs between SPI and I2C
(and even between classic single-IO SPI and the new dual-/quad-IO SPI).
Thus a common registration function for SPI and I2C would be difficult.
Fortunately we don't support I2C yet.


>> max_rom_decode is conceptually different from buses_supported although
>> that was not obvious when I wrote the paragraph above. It will be
>> handled in the universal programmer registration patch.
> In the end, I don't even think we need buses_supported outside of the
> classic/memcycle/parallel programmer (however you call it), because SPI
> interface chips just support spi flash chips, and for opaque programmers
> we even had to introduce a fake bus type.

In the future, probing will be registered-programmer-centric (i.e. walk
the registered-programmer list, and for each registered programmer walk
the flash chip list) and buses_supported will be a
per-registered-programmer field used to match chips with compatible
buses during probing. Probing doesn't care about the bus type except for
determining compatible chips.

> Maximum supported chip size on the other hand is a concept that likely
> applies sensibly apply to every type of programmer.

Indeed.


>>>>> - Should map_flash_region/unmap_flash_region be part of the registration?
>>>> no idea what that does exactly :P
>>> It makes the flash chip accessible. This is essentially physmap for
>>> programmers with memory mapped flash and a no-op for everything else.
> If the idea of a future flashrom (or libflashrom) is that you scan the
> system for programmers and build a table from the registrations, then I
> don't think you really want to initialize the hardware/map the memory at
> the time of registration further than needed for detection, but you need
> a hardware startup callback in the programmer.

That's the idea. map_flash_region is only called during flash chip
probing and not during programmer probing/registration.


> [drkaiser]
>> +
>> +	max_rom_decode.parallel = 131072;
> "128 * 1024" to comply with the flashrom style?

Fixed.


>> +	/* FIXME: This assumes that serprog device bustypes are always
>> +	 * identical with flashrom bustype enums and that they all fit
>> +	 * in a single byte.
>> +	 */
> I guess that is how the serprog protocol is "specified" ;)

Heh, yes.


>> Index: flashrom-register_par_programmer/it85spi.c
>> ===================================================================
>> --- flashrom-register_par_programmer/it85spi.c	(Revision 1460)
>> +++ flashrom-register_par_programmer/it85spi.c	(Arbeitskopie)
>> @@ -257,8 +257,10 @@
>>  	INDIRECT_A3(shm_io_base, (base >> 24));
>>  #endif
>>  #ifdef LPC_MEMORY
>> -	base = (chipaddr)programmer_map_flash_region("it85 communication",
>> -						     0xFFFFF000, 0x1000);
>> +	/* FIXME: We should block accessing that region for anything else.
>> +	 * Major TODO here, and it will be a lot of work.
>> +	 */
> "blocking it for anything else" just means disabling the
> "classic/parallel" interface of the internal programmer, if I understand
> it correctly, so...

Not necessarily. If there are multiple flash chips on a board, e.g. a
512 kByte SPI chip behind IT85 SPI translation and a 512 kByte FWH chip
attached to the southbridge and if the SPI chip is mapped at 4G-512k to
4G and the FWH chip is mapped in the region 4G-1M to 4G-512k, no
conflict arises as long as FWH probing does not touch the top 512 kB.
A possible solution would be replacing physmap in
programmer_map_flash_region and in IT85 with a variant exclusive_physmap
which refuses mapping if another exclusive mapping exists in the same
region.


>> +		/* FIXME: Really leave FWH enabled? We can't use this region
>> +		 * anymore since accessing it would mess up IT85 communication.
>> +		 * If we decide to disable FWH for this region, we should print
>> +		 * a debug message about it.
>> +		 */
> ... of course FWH/LPC/Parallel needs to be turned off here. I hope this
> code verifies that the IT85 SPI flash translator is really enabled,
> before concluding this.

I'll try to get Google to merge their current IT85 SPI code into
mainline flashrom and then fix any issues which still arise.
To be honest, picking LPC_IO instead of LPC_MEMORY would solve the whole
problem without introducing any conflicts, but that would also need
cooperation from the IT85 chip to disable listening to LPC memory cycles
for its command interface.


> Looks fine so far. So go ahead and commit.
>
> Acked-By: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>

Thanks a lot.

Committed in r1463.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list