From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: "Aleksandar Markovic" <aleksandar.m.mail@gmail.com>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Joaquin de Andres <me@xcancerberox.com.ar>,
Michael Rolnik <mrolnik@gmail.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
Sarah Harris <S.E.Harris@kent.ac.uk>,
Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH] !fixup "target/avr: Add defintions of AVR core types"
Date: Sat, 8 Feb 2020 15:06:25 +0100 [thread overview]
Message-ID: <556ea4ff-7af6-5a84-b793-f31b8d58d202@amsat.org> (raw)
In-Reply-To: <CAL1e-=jqCgakxTn1zQaNu7WqLjEien_Nd+nxQRe7NdmXKiivfw@mail.gmail.com>
Hi Aleksandar,
On 2/8/20 8:10 AM, Aleksandar Markovic wrote:
> On Friday, February 7, 2020, Philippe Mathieu-Daudé <philmd@redhat.com
> <mailto:philmd@redhat.com>> wrote:
>
> These cores have unresolved review comment:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg674105.html
> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg674105.html>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg674259.html
> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg674259.html>
> and:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg676046.html
> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg676046.html>
>
> As we don't want a bad start with this architecture, remove them.
>
>
> I agree with underlying motivation of your fixup.
>
> Still, the division of AVR cores into avr1, avr2, ... , xmega7 is here
> to stay. The reason is that because such coding is a part of ELF header,
> and this means they will stay forever (as they are approved by some kind
> of ELF comitee, and are meant not to be ever changed in future).
I am not removing anything ELF related. We don't have any machine using
CPU avrtiny/avr1/avr2/avr25 so AFAIK I'm simply removing unreviewed dead
code.
> Rather than deleting definitions and references of core types we know we
> don't support (or, at least, don't fully support), let's think of some
> less intrusive way - for example, checking core type of executable given
> via CLI, and refusing to emulate, if we know we still have some issues
> with the core type in question.
>
> For example, "avrtiny" type is missing handling the fact that it has 16
> register instead of 32 registers, like otger AVR core types. But, other
> AVRFeatures of "avrtiny" are correctly identified, and it would be a
> shame to delete them now and force someone in future to reinvent the
> wheel. Just refusing to emulate "avrtiny" seems a better approach to me.
It is hard to follow you, this port is contributed by hobbyist and you
appeared lately in the review process and asked to raised the quality to
a professional level. Now professionals are taking extra care to review
little details as the CPU features implemented by each core, and you are
saying we shouldn't delete incorrect code?
I am not saying we will never accept these cores, I'm proposing to start
with reviewed cores, so we don't delay the merge if this port. There are
already a lot of cores to use:
DEFINE_AVR_CPU_TYPE("avr3", avr_avr3_initfn),
DEFINE_AVR_CPU_TYPE("avr31", avr_avr31_initfn),
DEFINE_AVR_CPU_TYPE("avr35", avr_avr35_initfn),
DEFINE_AVR_CPU_TYPE("avr4", avr_avr4_initfn),
DEFINE_AVR_CPU_TYPE("avr5", avr_avr5_initfn),
DEFINE_AVR_CPU_TYPE("avr51", avr_avr51_initfn),
DEFINE_AVR_CPU_TYPE("avr6", avr_avr6_initfn),
DEFINE_AVR_CPU_TYPE("xmega2", avr_xmega2_initfn),
DEFINE_AVR_CPU_TYPE("xmega3", avr_xmega3_initfn),
DEFINE_AVR_CPU_TYPE("xmega4", avr_xmega4_initfn),
DEFINE_AVR_CPU_TYPE("xmega5", avr_xmega5_initfn),
DEFINE_AVR_CPU_TYPE("xmega6", avr_xmega6_initfn),
DEFINE_AVR_CPU_TYPE("xmega7", avr_xmega7_initfn),
Then it will be less stressful to correct/review/add the avr1/2/tiny
cores. This seems simpler than keep respining 30 patches during the next
6 months. I don't want the community to get tired and loose interest in
this port, we already spent a fair amount of time to get it merged.
Regards,
Phil.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
> <mailto:philmd@redhat.com>>
> ---
> Based-on:
> <1581040680-308-1-git-send-email-aleksandar.markovic@rt-rk.com
> <mailto:1581040680-308-1-git-send-email-aleksandar.markovic@rt-rk.com>>
>
> Side note: typo in the subject "definitions"
> ---
> target/avr/cpu.c | 96 ------------------------------------------------
> 1 file changed, 96 deletions(-)
>
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 8a084c750f..b3d661142d 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -216,77 +216,6 @@ static void avr_cpu_class_init(ObjectClass *oc,
> void *data)
> cc->gdb_core_xml_file = "avr-cpu.xml";
> }
>
> -/*
> - * Setting features of AVR core type avr1
> - * --------------------------------------
> - *
> - * This type of AVR core is present in the following AVR MCUs:
> - *
> - * at90s1200, attiny11, attiny12, attiny15, attiny28
> - */
> -static void avr_avr1_initfn(Object *obj)
> -{
> - AVRCPU *cpu = AVR_CPU(obj);
> - CPUAVRState *env = &cpu->env;
> -
> - set_avr_feature(env, AVR_FEATURE_LPM);
> - set_avr_feature(env, AVR_FEATURE_2_BYTE_SP);
> - set_avr_feature(env, AVR_FEATURE_2_BYTE_PC);
> -}
> -
> -/*
> - * Setting features of AVR core type avr2
> - * --------------------------------------
> - *
> - * This type of AVR core is present in the following AVR MCUs:
> - *
> - * at90s2313, at90s2323, at90s2333, at90s2343, attiny22, attiny26,
> at90s4414,
> - * at90s4433, at90s4434, at90s8515, at90c8534, at90s8535
> - */
> -static void avr_avr2_initfn(Object *obj)
> -{
> - AVRCPU *cpu = AVR_CPU(obj);
> - CPUAVRState *env = &cpu->env;
> -
> - set_avr_feature(env, AVR_FEATURE_LPM);
> - set_avr_feature(env, AVR_FEATURE_IJMP_ICALL);
> - set_avr_feature(env, AVR_FEATURE_ADIW_SBIW);
> - set_avr_feature(env, AVR_FEATURE_SRAM);
> - set_avr_feature(env, AVR_FEATURE_BREAK);
> -
> - set_avr_feature(env, AVR_FEATURE_2_BYTE_PC);
> - set_avr_feature(env, AVR_FEATURE_2_BYTE_SP);
> -}
> -
> -/*
> - * Setting features of AVR core type avr25
> - * --------------------------------------
> - *
> - * This type of AVR core is present in the following AVR MCUs:
> - *
> - * ata5272, ata6616c, attiny13, attiny13a, attiny2313, attiny2313a,
> attiny24,
> - * attiny24a, attiny4313, attiny44, attiny44a, attiny441, attiny84,
> attiny84a,
> - * attiny25, attiny45, attiny85, attiny261, attiny261a, attiny461,
> attiny461a,
> - * attiny861, attiny861a, attiny43u, attiny87, attiny48, attiny88,
> attiny828,
> - * attiny841, at86rf401
> - */
> -static void avr_avr25_initfn(Object *obj)
> -{
> - AVRCPU *cpu = AVR_CPU(obj);
> - CPUAVRState *env = &cpu->env;
> -
> - set_avr_feature(env, AVR_FEATURE_LPM);
> - set_avr_feature(env, AVR_FEATURE_IJMP_ICALL);
> - set_avr_feature(env, AVR_FEATURE_ADIW_SBIW);
> - set_avr_feature(env, AVR_FEATURE_SRAM);
> - set_avr_feature(env, AVR_FEATURE_BREAK);
> -
> - set_avr_feature(env, AVR_FEATURE_2_BYTE_PC);
> - set_avr_feature(env, AVR_FEATURE_2_BYTE_SP);
> - set_avr_feature(env, AVR_FEATURE_LPMX);
> - set_avr_feature(env, AVR_FEATURE_MOVW);
> -}
> -
> /*
> * Setting features of AVR core type avr3
> * --------------------------------------
> @@ -499,27 +428,6 @@ static void avr_avr6_initfn(Object *obj)
> set_avr_feature(env, AVR_FEATURE_MUL);
> }
>
> -/*
> - * Setting features of AVR core type avrtiny
> - * --------------------------------------
> - *
> - * This type of AVR core is present in the following AVR MCUs:
> - *
> - * attiny4, attiny5, attiny9, attiny10, attiny20, attiny40
> - */
> -static void avr_avrtiny_initfn(Object *obj)
> -{
> - AVRCPU *cpu = AVR_CPU(obj);
> - CPUAVRState *env = &cpu->env;
> -
> - set_avr_feature(env, AVR_FEATURE_LPM);
> - set_avr_feature(env, AVR_FEATURE_IJMP_ICALL);
> - set_avr_feature(env, AVR_FEATURE_BREAK);
> -
> - set_avr_feature(env, AVR_FEATURE_2_BYTE_PC);
> - set_avr_feature(env, AVR_FEATURE_1_BYTE_SP);
> -}
> -
> /*
> * Setting features of AVR core type xmega2
> * --------------------------------------
> @@ -754,10 +662,6 @@ static const TypeInfo avr_cpu_type_info[] = {
> .class_init = avr_cpu_class_init,
> .abstract = true,
> },
> - DEFINE_AVR_CPU_TYPE("avrtiny", avr_avrtiny_initfn),
> - DEFINE_AVR_CPU_TYPE("avr1", avr_avr1_initfn),
> - DEFINE_AVR_CPU_TYPE("avr2", avr_avr2_initfn),
> - DEFINE_AVR_CPU_TYPE("avr25", avr_avr25_initfn),
> DEFINE_AVR_CPU_TYPE("avr3", avr_avr3_initfn),
> DEFINE_AVR_CPU_TYPE("avr31", avr_avr31_initfn),
> DEFINE_AVR_CPU_TYPE("avr35", avr_avr35_initfn),
> --
> 2.21.1
>
next prev parent reply other threads:[~2020-02-08 14:07 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-07 1:57 [PATCH rc5 00/32] target/avr merger Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 01/32] target/avr: Add basic parameters of the new platform Aleksandar Markovic
2020-02-07 15:00 ` Michael Rolnik
2020-02-07 1:57 ` [PATCH rc5 02/32] target/avr: Introduce basic CPU class object Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 03/32] target/avr: CPU class: Add interrupt handling support Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 04/32] target/avr: CPU class: Add memory menagement support Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 05/32] target/avr: CPU class: Add migration support Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 06/32] target/avr: CPU class: Add GDB support Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 07/32] target/avr: Introduce enumeration AVRFeature Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 08/32] target/avr: Add defintions of AVR core types Aleksandar Markovic
2020-02-07 11:23 ` [PATCH] !fixup "target/avr: Add defintions of AVR core types" Philippe Mathieu-Daudé
2020-02-08 7:10 ` Aleksandar Markovic
2020-02-08 14:06 ` Philippe Mathieu-Daudé [this message]
2020-02-08 16:01 ` Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 09/32] target/avr: Add instruction helpers Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 10/32] target/avr: Add instruction translation - Register definitions Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 11/32] target/avr: Add instruction translation - Arithmetic and Logic Instructions Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 12/32] target/avr: Add instruction translation - Branch Instructions Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 13/32] target/avr: Add instruction translation - Data Transfer Instructions Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 14/32] target/avr: Add instruction translation - Bit and Bit-test Instructions Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 15/32] target/avr: Add instruction translation - MCU Control Instructions Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 16/32] target/avr: Add instruction translation - CPU main translation function Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 17/32] target/avr: Initialize TCG register variables Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 18/32] target/avr: Add support for disassembling via option '-d in_asm' Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 19/32] hw/char: avr: Add limited support for USART peripheral Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 20/32] hw/timer: avr: Add limited support for 16-bit timer peripheral Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 21/32] hw/misc: avr: Add limited support for power reduction device Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 22/32] target/avr: Register AVR support with the rest of QEMU Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 23/32] hw/avr: Add support for loading ELF/raw binaries Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 24/32] hw/avr: Add some ATmega microcontrollers Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 25/32] hw/avr: Add limited support for some Arduino boards Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 26/32] target/avr: Update build system Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 27/32] tests/machine-none: Add AVR support Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 28/32] tests/boot-serial: Test some Arduino boards (AVR based) Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 29/32] tests/acceptance: Test the Arduino MEGA2560 board Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 30/32] .travis.yml: Run the AVR acceptance tests Aleksandar Markovic
2020-02-07 1:57 ` [PATCH rc5 31/32] target/avr: Simplify sections in MAINTAINERS file Aleksandar Markovic
2020-02-07 1:58 ` [PATCH rc5 32/32] target/avr: Add section into QEMU documentation Aleksandar Markovic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=556ea4ff-7af6-5a84-b793-f31b8d58d202@amsat.org \
--to=f4bug@amsat.org \
--cc=S.E.Harris@kent.ac.uk \
--cc=aleksandar.m.mail@gmail.com \
--cc=me@xcancerberox.com.ar \
--cc=mrolnik@gmail.com \
--cc=philmd@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.