Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Philippe Mathieu-Daudé]

Hi Aleksandar, Richard.

On 11/28/19 10:28 AM, Aleksandar Markovic wrote:
> On Thursday, November 28, 2019, Philippe Mathieu-Daudé <address@hidden 
> <mailto:address@hidden>> wrote:
>
>     Add famous ATmega MCUs:
>
>     - middle range: ATmega168 and ATmega328
>     - high range: ATmega1280 and ATmega2560
>
>     Signed-off-by: Philippe Mathieu-Daudé <address@hidden
>     <mailto:address@hidden>>
>     ---
>
>
> Philippe, hi.
>
> Thank you for the impetus you give us all.
>
> However, is this the right direction?

I am not the maintainer who will merge the AVR port, so I'm not an 
authoritative figure. I simply wanted to help Michael and Joaquin, so 
spent 6h of my personal time here.

> Let's analyse some bits and pieces.
>
> Starting from the commit message, the word "famous" is used, but I 
> really don't see enumerated CPUs/MCUs are any special in Atmel lineup. 
> Other than we often used the doc describing them (cited several times in 
> our discussions) as our reference, but that doesn't make them "famous". 
> Ofcourse, there other docs for other Atmel CPUs/MCUs, of at lest 
> equivalent significance. For example, "tiny" ones are at least as famous 
> as "mega" ones.

As noted in the cover, this is sent as RFC and I was quite tired when 
posting so the documentation is rather scarce, but the patches were 
targeted at Michael/Joaquin as example how they could properly add AVR 
machines.

> Then, you introduce the term MCU, without proper discussion with others 
> on terminology. In parlance of QEMU, ATmega168 at al. are CPUs (we all 
> know and assume that that are some peripherals in it). I am not against 
> using the term MCU, but let's first sync up on that.
>
> The added terminology trouble is that MCUs, as you defined them, have in 
> array atmega_mcu[] a field called "cpu_type" - why is that field not 
> called "mcu_type"? *Very* confusing for any future reader. And then, 
> similar terminology conundrum continues with macro AVR_CPU_TYPE_NAME().
>
> All of the above is far less important than this question: What are we 
> achieving with proposed CPU/MCU definitions? I certainly see the value 
> of fitting the complex variety of AVR CPUs/MCUs into QEMU object model. 
> No question about that. However, is this the right moment to do it? 
> There are still some unresolved architectural problems with peripheral 
> definitions, as I noted in yhe comment to Michael's last cover letter. 
> This patch does not solve them. It just assumes everything is ready with 
> peripherals, let's build CPUs/MCUs. The machines based on proposed 
> CPUs/MCUs are not more real that machine based on Michael's "sample" 
> machine. At least Michal says "this is not a real machine". If we used 
> proposed CPUs/MCUs from this patch, the resulting machine is as "paper" 
> machine as yhe "sample" machine. We would just live in a la-la lend of 
> thinking: "wow, we model real hardware now".
>
> I would rather accept into QEMU a series admitting we are still far from 
> modelling real machine or CPU/MCU, than a series that gives an illusion 
> that we are modelling real machine or CPU/MCU.

These patches try to do the same as the 'sample' machine modeled by 
Michael, but reordered in a different way.
The only documentation was "The CPU is an approximation of an 
ATmega2560" so I assumed he could be using the Arduino MEGA2560 board 
which matches. I used the same peripherals that Michael used, simply 
presented a more QEMU-up-to-date way.

> As far as utility of this patch for Michael's series, it looks to me 
> they add more headake than help (not saying that the help is not 
> present) to Michael. He would have anotter abstraction layer to think 
> of, at the moment he desperately needs (in my opinion) to focus on 
> providing the as solid as possible, and as complete as possinle 
> foundations. This patch looks like building castles in the air. Again, I 
> am not claiming that the patch is not helpful at all.
>
> In summary, I think that this patch is premature.

If we merge the 'sample' board, the deprecation process will take at 
least 8 months, hoping no-one start to hack it.

So to save time, we can merge the architectural part (target/avr) of 
Michael work, without the hardware part (hw/avr).

Michael/Sarah can still test their 'instruction-tests' [*] suite using 
the 'none' machine.

See this example which creates a testing machine with a AVR3 core and 
1MB of RAM mapped at 0x0000:

$ avr-softmmu/qemu-system-avr -M none -cpu avr3-avr-cpu -m 1 -S -monitor 
stdio
QEMU 4.1.93 monitor - type 'help' for more information
(qemu) info mtree
address-space: cpu-memory-0
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-00000000000fffff (prio 0, ram): ram

(qemu) info qom-tree
/machine (none-machine)
  /unattached (container)
    /ram[0] (qemu:memory-region)
    /sysbus (System)
    /system[0] (qemu:memory-region)
    /io[0] (qemu:memory-region)
    /device[0] (avr3-avr-cpu)
    [...]

They won't be able to run the FreeRTOS test suite [*] until we find a 
consensus with the AVR hardware/boards.

This seems the clever outcome, so the AVR architectural part get merged 
as soon as 5.0 opens, and we have time to improve the hardware part.
And Michael won't have to repost his series until v42 :)

Richard, you seem to be the de-facto maintainer, is that correct?
If you agree with my suggestion, I can prepare a v38 based on Michael's 
last series, sanitized and without the HW part, so it you can queue it 
for avr-next.

[*] https://github.com/seharris/qemu-avr-tests