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.
- 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.
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. Maximum supported chip size on the other hand is a concept that likely applies sensibly apply to every type of programmer.
- 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.
And moving it into the parallel programmer struct may not have been the best idea. Sure, that gets it out of the way conveniently, but once the universal programmer registration patch is in, we will not have any dummy parallel programmer struct for such fallbacks anymore.
[drkaiser]
- max_rom_decode.parallel = 131072;
"128 * 1024" to comply with the flashrom style?
- register_par_programmer(&par_programmer_drkaiser, BUS_PARALLEL);
- return 0;
}
- /* 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" ;)
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...
/* 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.
Looks fine so far. So go ahead and commit.
Acked-By: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Regards, Michael Karcher