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@mkarcher.dialup.fu-berlin.de
Thanks a lot.
Committed in r1463.
Regards, Carl-Daniel