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

[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.

> >>> - 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 at mkarcher.dialup.fu-berlin.de>

Regards,
  Michael Karcher
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20111109/793e0723/attachment.asc>