Re: [avrdude-dev] attiny10 with ftdi and bitbang

[Darell Tan]

Hi Hannes,

Can I ask why you did not simply implement cmd_tpi, chip_erase,
program_enable and initialize functions accordingly?

When avrdude is called to read, avr_read() is called and within
avr_read() there is already code that checks for cmd_tpi and calls it.
If you implement the above-mentioned functions, you should immediately
be able to read/write using TPI. The same thing happens for
avr_write()... unless I totally misunderstood how avrdude or avrftdi
works.


> I don't like the way how, for example usbasp, does change callbacks 
> (simplified):
>
> if((AVRPART*)->flags & AVRPART_HAS_TPI)
> {
> ...
>     pgm->program_enable = usbasp_tpi_program_enable;
>     pgm->chip_erase     = usbasp_tpi_chip_erase;
>     pgm->cmd            = usbasp_tpi_cmd;
>     pgm->read_byte      = usbasp_tpi_read_byte;
>     pgm->write_byte     = usbasp_tpi_write_byte;
>     pgm->paged_write    = usbasp_tpi_paged_write;
>     pgm->paged_load     = usbasp_tpi_paged_load;
>     pgm->set_sck_period = usbasp_tpi_set_sck_period;
> ...
> } else
> ...
>

Yes I feel the same way about this piece of code. After doing a quick
search in the source code, I realised that usbasp re-implements the
TPI protocol again. I guess it could be refactored to rely on a single
protocol implementation in avr.c and just implement cmd_tpi() as well
(I haven't really looked at the code in detail).

The reason why I am emphasizing on cmd_tpi() is because when I read
the avrdude code, I believe this is the way it was designed to be.
Each of the opcodes are in the config file, which can be accessed by
the mem->op[] array, and cmd() is called with the appropriate opcode
which transmits it over the physical layer, whether bitbang or SPI or
MPSSE.

When I implemented TPI, I followed the same design, except that I
introduced cmd_tpi instead of "overloading" cmd(). Another reason is
due to the function prototype, which was fixed to cmd[4] and res[4],
which are not necessarily the case for TPI.

--
Regards,
Darell Tan


On Mon, Jul 9, 2012 at 8:35 PM, Hannes Weisbach <address@hidden> wrote:
>
> Am 09.07.2012 um 10:53 schrieb Darell Tan:
>
>> Hi Hannes,
>>
>> That's good news! Actually I am not sure if it would cause a lot of
>> duplication - it may be just a bit, which is fine.
> It's rather a lot. I essentially copied the code from usbasp and renamed 
> everything from usbasp to avrftdi. The only thing /really/ new thing are 
> read/write byte functions, which of course, are programmer-specific. This is 
> essentially what I would call the "TPI access layer" in avrdude.
>>
>> The TPI code is currently implemented in 2 main files: bitbang.c and
>> avr.c. It is probably already split into the access layer and physical
>> layer.
> Nope, 3. usbasp also implements TPI and duplicates the below-mentioned 
> initialize, erase and enable functions.
>>
>> In bitbang.c, the TPI "physical layer" is in these functions:
>> - bitbang_tpi_clk
>> - bitbang_tpi_tx
>> - bitbang_tpi_rx
>> - bitbang_cmd_tpi (exposed in pgm struct)
> Maybe exposing cmd_tpi in the PROGRAMMER struct is sufficient, and read/write 
> byte commands (as suggested by me in my previous mail) are not necessary.
>>
>> The only place where code is duplicated is in these functions:
>> - bitbang_chip_erase
>> - bitbang_program_enable
>> - bitbang_initialize
> Correct. But in the end all these functions could be replaced by a generic 
> version, which calls pgm->cmd_tpi.
> Probably paged programming/loading could also implemented with single 
> pgm->cmd_tpi callbacks (instead of using bulk transfers).
> I'm not sure how to integrate paged programming in the existing framework.
> I don't like the way how, for example usbasp, does change callbacks 
> (simplified):
>
> if((AVRPART*)->flags & AVRPART_HAS_TPI)
> {
> ...
>     pgm->program_enable = usbasp_tpi_program_enable;
>     pgm->chip_erase     = usbasp_tpi_chip_erase;
>     pgm->cmd            = usbasp_tpi_cmd;
>     pgm->read_byte      = usbasp_tpi_read_byte;
>     pgm->write_byte     = usbasp_tpi_write_byte;
>     pgm->paged_write    = usbasp_tpi_paged_write;
>     pgm->paged_load     = usbasp_tpi_paged_load;
>     pgm->set_sck_period = usbasp_tpi_set_sck_period;
> ...
> } else
> ...
>
> avrftdi doesn't have as much callbacks for TPI (yet, ... working on it ...).
>
> So, I don't like that. The next thing to do would be using the "standard" 
> page programming function and putting something like:
> ...
> ((AVRPART*)->flags & AVRPART_HAS_TPI)
>         do_tpi_page_programming(...);
> ...
> which is not elegant either. So I guess I would like a callback in the 
> programmer struct for every programming mechanism (ISP, TPI, PDI, JTAG, ...) 
> the programmer supports.
> However I have only a limited overview on avrdude and possible problems 
> associated with it.
>>
>> The cmd_tpi is the main function which should be implemented in
>> avrftdi for the physical layer. As for the chip_erase and
>> program_enable, there is no avr_xxx common function available for
>> these, so the code has to be duplicated here. The bitbang_initialize
>> also needs to be implemented to enable the TPI interface.
> That's what I want: avr.c /should/ implement chip_erase and program_enable, 
> because they are generic when relying on the PROGRAMMER->cmd_tpi callback.
> A value for guard time bits could be put in the PROGRAMMER struct, so that 
> the code in avr.c knows about what the programmer actually needs.
>>
>> All the code in avr.c is already calling cmd_tpi, so there's not much
>> problems there. There's no real paged reads/writes, since if avr_read
>> and avr_write is called with a block of contiguous data, the reads and
>> writes can be performed faster using post-increment.
> Bundling reads and writes with post-increment enabled should be way faster 
> when only one USB transfer is used. For my 4232H device (USB 2.0), 
> programming an AVR with single byte instruction (using ISP however) is not 
> much slower than paged programming. The difference in speed is much more 
> clearly using a USB 1.1 device (2232C, in my case).
>
> Regards,
> Hannes