[PATCH 00/11] AVR patch queue for QEMU 6.0

[PATCH 00/11] AVR patch queue for QEMU 6.0

[Philippe Mathieu-Daudé]

Hi,

This series contains all the AVR patches I could find on the list.

Niteesh, I fixed minor issues. Do you mind reviewing on top?

Pull request planned for Monday if no problem arises.

Thanks,

Phil.

G S Niteesh Babu (2):
  hw/gpio/avr_gpio: Add tracing for reads and writes
  hw/avr/arduino: Add D13 LED

Heecheol Yang (1):
  hw/avr: Add limited support for avr gpio registers

Ivanov Arkasha (1):
  target/avr: Fix interrupt execution

Lichang Zhao (1):
  target/avr: Fix some comment spelling errors

Philippe Mathieu-Daudé (6):
  hw/misc/led: Add yellow LED
  hw/avr/arduino: List board schematic links
  hw/gpio/avr_gpio: Add migration VMstate
  hw/gpio/avr_gpio: Add 'id' field in AVRGPIOState
  hw/gpio/avr_gpio: Simplify avr_gpio_write_port using extract32()
  hw/avr/arduino: Replace magic number by gpio_port_index() call

 hw/avr/atmega.h            |   2 +
 include/hw/gpio/avr_gpio.h |  54 ++++++++++++
 include/hw/misc/led.h      |   1 +
 hw/avr/arduino.c           |  44 +++++++++-
 hw/avr/atmega.c            |   8 +-
 hw/gpio/avr_gpio.c         | 173 +++++++++++++++++++++++++++++++++++++
 hw/misc/led.c              |   1 +
 target/avr/helper.c        |  10 ++-
 hw/avr/Kconfig             |   2 +
 hw/gpio/Kconfig            |   3 +
 hw/gpio/meson.build        |   1 +
 hw/gpio/trace-events       |   5 ++
 12 files changed, 294 insertions(+), 10 deletions(-)
 create mode 100644 include/hw/gpio/avr_gpio.h
 create mode 100644 hw/gpio/avr_gpio.c

-- 
2.26.2

[PATCH 01/11] hw/misc/led: Add yellow LED

[Philippe Mathieu-Daudé]

Add the yellow "lime" LED.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/led.h | 1 +
 hw/misc/led.c         | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/hw/misc/led.h b/include/hw/misc/led.h
index aa359b87c20..29c08795708 100644
--- a/include/hw/misc/led.h
+++ b/include/hw/misc/led.h
@@ -27,6 +27,7 @@ typedef enum {          /* Coarse wavelength range */
     LED_COLOR_BLUE,     /* 475 nm */
     LED_COLOR_CYAN,     /* 500 nm */
     LED_COLOR_GREEN,    /* 535 nm */
+    LED_COLOR_YELLOW,   /* 567 nm */
     LED_COLOR_AMBER,    /* 590 nm */
     LED_COLOR_ORANGE,   /* 615 nm */
     LED_COLOR_RED,      /* 630 nm */
diff --git a/hw/misc/led.c b/hw/misc/led.c
index 5266d026d0b..f552b8b6483 100644
--- a/hw/misc/led.c
+++ b/hw/misc/led.c
@@ -20,6 +20,7 @@ static const char * const led_color_name[] = {
     [LED_COLOR_BLUE]    = "blue",
     [LED_COLOR_CYAN]    = "cyan",
     [LED_COLOR_GREEN]   = "green",
+    [LED_COLOR_YELLOW]  = "yellow",
     [LED_COLOR_AMBER]   = "amber",
     [LED_COLOR_ORANGE]  = "orange",
     [LED_COLOR_RED]     = "red",
-- 
2.26.2

Re: [PATCH 01/11] hw/misc/led: Add yellow LED

[Richard Henderson]

On 3/13/21 10:54 AM, Philippe Mathieu-Daudé wrote:
> Add the yellow "lime" LED.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   include/hw/misc/led.h | 1 +
>   hw/misc/led.c         | 1 +
>   2 files changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 02/11] hw/avr/arduino: List board schematic links

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/avr/arduino.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
index 3c8388490d6..3ff31492fa6 100644
--- a/hw/avr/arduino.c
+++ b/hw/avr/arduino.c
@@ -75,7 +75,10 @@ static void arduino_duemilanove_class_init(ObjectClass *oc, void *data)
     MachineClass *mc = MACHINE_CLASS(oc);
     ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
 
-    /* https://www.arduino.cc/en/Main/ArduinoBoardDuemilanove */
+    /*
+     * https://www.arduino.cc/en/Main/ArduinoBoardDuemilanove
+     * https://www.arduino.cc/en/uploads/Main/arduino-duemilanove-schematic.pdf
+     */
     mc->desc        = "Arduino Duemilanove (ATmega168)",
     mc->alias       = "2009";
     amc->mcu_type   = TYPE_ATMEGA168_MCU;
@@ -87,7 +90,10 @@ static void arduino_uno_class_init(ObjectClass *oc, void *data)
     MachineClass *mc = MACHINE_CLASS(oc);
     ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
 
-    /* https://store.arduino.cc/arduino-uno-rev3 */
+    /*
+     * https://store.arduino.cc/arduino-uno-rev3
+     * https://www.arduino.cc/en/uploads/Main/arduino-uno-schematic.pdf
+     */
     mc->desc        = "Arduino UNO (ATmega328P)";
     mc->alias       = "uno";
     amc->mcu_type   = TYPE_ATMEGA328_MCU;
@@ -99,7 +105,10 @@ static void arduino_mega_class_init(ObjectClass *oc, void *data)
     MachineClass *mc = MACHINE_CLASS(oc);
     ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
 
-    /* https://www.arduino.cc/en/Main/ArduinoBoardMega */
+    /*
+     * https://www.arduino.cc/en/Main/ArduinoBoardMega
+     * https://www.arduino.cc/en/uploads/Main/arduino-mega2560-schematic.pdf
+     */
     mc->desc        = "Arduino Mega (ATmega1280)";
     mc->alias       = "mega";
     amc->mcu_type   = TYPE_ATMEGA1280_MCU;
@@ -111,7 +120,10 @@ static void arduino_mega2560_class_init(ObjectClass *oc, void *data)
     MachineClass *mc = MACHINE_CLASS(oc);
     ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
 
-    /* https://store.arduino.cc/arduino-mega-2560-rev3 */
+    /*
+     * https://store.arduino.cc/arduino-mega-2560-rev3
+     * https://www.arduino.cc/en/uploads/Main/arduino-mega2560_R3-sch.pdf
+     */
     mc->desc        = "Arduino Mega 2560 (ATmega2560)";
     mc->alias       = "mega2560";
     amc->mcu_type   = TYPE_ATMEGA2560_MCU;
-- 
2.26.2

Re: [PATCH 02/11] hw/avr/arduino: List board schematic links

[Richard Henderson]

On 3/13/21 10:54 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/avr/arduino.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 03/11] hw/avr: Add limited support for avr gpio registers

[Philippe Mathieu-Daudé]

From: Heecheol Yang <heecheol.yang@outlook.com>

Add some of these features for AVR GPIO:

  - GPIO I/O : PORTx registers
  - Data Direction : DDRx registers
  - DDRx toggling : PINx registers

Following things are not supported yet:
  - MCUR registers

Signed-off-by: Heecheol Yang <heecheol.yang@outlook.com>
Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
Message-Id: <DM6PR16MB247368DBD3447ABECDD795D7E6090@DM6PR16MB2473.namprd16.prod.outlook.com>
[PMD: Use AVR_GPIO_COUNT]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/avr/atmega.h            |   2 +
 include/hw/gpio/avr_gpio.h |  53 ++++++++++++++
 hw/avr/atmega.c            |   7 +-
 hw/gpio/avr_gpio.c         | 138 +++++++++++++++++++++++++++++++++++++
 hw/avr/Kconfig             |   1 +
 hw/gpio/Kconfig            |   3 +
 hw/gpio/meson.build        |   1 +
 7 files changed, 203 insertions(+), 2 deletions(-)
 create mode 100644 include/hw/gpio/avr_gpio.h
 create mode 100644 hw/gpio/avr_gpio.c

diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
index a99ee15c7e1..e2289d5744e 100644
--- a/hw/avr/atmega.h
+++ b/hw/avr/atmega.h
@@ -13,6 +13,7 @@
 
 #include "hw/char/avr_usart.h"
 #include "hw/timer/avr_timer16.h"
+#include "hw/gpio/avr_gpio.h"
 #include "hw/misc/avr_power.h"
 #include "target/avr/cpu.h"
 #include "qom/object.h"
@@ -44,6 +45,7 @@ struct AtmegaMcuState {
     DeviceState *io;
     AVRMaskState pwr[POWER_MAX];
     AVRUsartState usart[USART_MAX];
+    AVRGPIOState gpio[GPIO_MAX];
     AVRTimer16State timer[TIMER_MAX];
     uint64_t xtal_freq_hz;
 };
diff --git a/include/hw/gpio/avr_gpio.h b/include/hw/gpio/avr_gpio.h
new file mode 100644
index 00000000000..498a7275f05
--- /dev/null
+++ b/include/hw/gpio/avr_gpio.h
@@ -0,0 +1,53 @@
+/*
+ * AVR processors GPIO registers definition.
+ *
+ * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef AVR_GPIO_H
+#define AVR_GPIO_H
+
+#include "hw/sysbus.h"
+#include "qom/object.h"
+
+/* Offsets of registers. */
+#define GPIO_PIN   0x00
+#define GPIO_DDR   0x01
+#define GPIO_PORT  0x02
+
+#define TYPE_AVR_GPIO "avr-gpio"
+OBJECT_DECLARE_SIMPLE_TYPE(AVRGPIOState, AVR_GPIO)
+#define AVR_GPIO_COUNT 8
+
+struct AVRGPIOState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    MemoryRegion mmio;
+
+    struct {
+        uint8_t pin;
+        uint8_t ddr;
+        uint8_t port;
+    } reg;
+
+    /* PORTx data changed IRQs */
+    qemu_irq out[8u];
+
+};
+
+#endif /* AVR_GPIO_H */
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index 44c6afebbb6..19c3122189f 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -283,8 +283,11 @@ static void atmega_realize(DeviceState *dev, Error **errp)
             continue;
         }
         devname = g_strdup_printf("atmega-gpio-%c", 'a' + (char)i);
-        create_unimplemented_device(devname,
-                                    OFFSET_DATA + mc->dev[idx].addr, 3);
+        object_initialize_child(OBJECT(dev), devname, &s->gpio[i],
+                                TYPE_AVR_GPIO);
+        sysbus_realize(SYS_BUS_DEVICE(&s->gpio[i]), &error_abort);
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio[i]), 0,
+                        OFFSET_DATA + mc->dev[idx].addr);
         g_free(devname);
     }
 
diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
new file mode 100644
index 00000000000..cdb574ef0d8
--- /dev/null
+++ b/hw/gpio/avr_gpio.c
@@ -0,0 +1,138 @@
+/*
+ * AVR processors GPIO registers emulation.
+ *
+ * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/sysbus.h"
+#include "hw/irq.h"
+#include "hw/gpio/avr_gpio.h"
+#include "hw/qdev-properties.h"
+
+static void avr_gpio_reset(DeviceState *dev)
+{
+    AVRGPIOState *gpio = AVR_GPIO(dev);
+
+    gpio->reg.pin = 0u;
+    gpio->reg.ddr = 0u;
+    gpio->reg.port = 0u;
+}
+
+static void avr_gpio_write_port(AVRGPIOState *s, uint64_t value)
+{
+    uint8_t pin;
+    uint8_t cur_port_val = s->reg.port;
+    uint8_t cur_ddr_val = s->reg.ddr;
+
+    for (pin = 0u; pin < AVR_GPIO_COUNT ; pin++) {
+        uint8_t cur_port_pin_val = cur_port_val & 0x01u;
+        uint8_t cur_ddr_pin_val = cur_ddr_val & 0x01u;
+        uint8_t new_port_pin_val = value & 0x01u;
+
+        if (cur_ddr_pin_val && (cur_port_pin_val != new_port_pin_val)) {
+            qemu_set_irq(s->out[pin], new_port_pin_val);
+        }
+        cur_port_val >>= 1u;
+        cur_ddr_val >>= 1u;
+        value >>= 1u;
+    }
+    s->reg.port = value & s->reg.ddr;
+}
+static uint64_t avr_gpio_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    AVRGPIOState *s = (AVRGPIOState *)opaque;
+    switch (offset) {
+    case GPIO_PIN:
+        return s->reg.pin;
+    case GPIO_DDR:
+        return s->reg.ddr;
+    case GPIO_PORT:
+        return s->reg.port;
+    default:
+        g_assert_not_reached();
+        break;
+    }
+    return 0;
+}
+
+static void avr_gpio_write(void *opaque, hwaddr offset, uint64_t value,
+                                unsigned int size)
+{
+    AVRGPIOState *s = (AVRGPIOState *)opaque;
+    value = value & 0xF;
+    switch (offset) {
+    case GPIO_PIN:
+        s->reg.pin = value;
+        s->reg.port ^= s->reg.pin;
+        break;
+    case GPIO_DDR:
+        s->reg.ddr = value;
+        break;
+    case GPIO_PORT:
+        avr_gpio_write_port(s, value);
+        break;
+    default:
+        g_assert_not_reached();
+        break;
+    }
+}
+
+static const MemoryRegionOps avr_gpio_ops = {
+    .read = avr_gpio_read,
+    .write = avr_gpio_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void avr_gpio_init(Object *obj)
+{
+    AVRGPIOState *s = AVR_GPIO(obj);
+
+    qdev_init_gpio_out(DEVICE(obj), s->out, ARRAY_SIZE(s->out));
+    memory_region_init_io(&s->mmio, obj, &avr_gpio_ops, s, TYPE_AVR_GPIO, 3);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
+}
+static void avr_gpio_realize(DeviceState *dev, Error **errp)
+{
+    /* Do nothing currently */
+}
+
+
+static void avr_gpio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = avr_gpio_reset;
+    dc->realize = avr_gpio_realize;
+}
+
+static const TypeInfo avr_gpio_info = {
+    .name          = TYPE_AVR_GPIO,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AVRGPIOState),
+    .instance_init = avr_gpio_init,
+    .class_init    = avr_gpio_class_init,
+};
+
+static void avr_gpio_register_types(void)
+{
+    type_register_static(&avr_gpio_info);
+}
+
+type_init(avr_gpio_register_types)
diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
index d31298c3cce..16a57ced11f 100644
--- a/hw/avr/Kconfig
+++ b/hw/avr/Kconfig
@@ -3,6 +3,7 @@ config AVR_ATMEGA_MCU
     select AVR_TIMER16
     select AVR_USART
     select AVR_POWER
+    select AVR_GPIO
 
 config ARDUINO
     select AVR_ATMEGA_MCU
diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
index f0e7405f6e6..fde7019b2ba 100644
--- a/hw/gpio/Kconfig
+++ b/hw/gpio/Kconfig
@@ -13,3 +13,6 @@ config GPIO_PWR
 
 config SIFIVE_GPIO
     bool
+
+config AVR_GPIO
+    bool
diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
index 79568f00ce3..366aca52ca2 100644
--- a/hw/gpio/meson.build
+++ b/hw/gpio/meson.build
@@ -13,3 +13,4 @@
 softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_gpio.c'))
 softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_gpio.c'))
 softmmu_ss.add(when: 'CONFIG_SIFIVE_GPIO', if_true: files('sifive_gpio.c'))
+softmmu_ss.add(when: 'CONFIG_AVR_GPIO', if_true: files('avr_gpio.c'))
-- 
2.26.2

Re: [PATCH 03/11] hw/avr: Add limited support for avr gpio registers

[Richard Henderson]

On 3/13/21 10:54 AM, Philippe Mathieu-Daudé wrote:
> +#define AVR_GPIO_COUNT 8
> +
> +struct AVRGPIOState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion mmio;
> +
> +    struct {
> +        uint8_t pin;
> +        uint8_t ddr;
> +        uint8_t port;
> +    } reg;
> +
> +    /* PORTx data changed IRQs */
> +    qemu_irq out[8u];

AVR_GPIO_COUNT?

Can we drop all the useless 'u' suffixes from all over?

> +    gpio->reg.pin = 0u;
> +    gpio->reg.ddr = 0u;
> +    gpio->reg.port = 0u;
...
> +        uint8_t cur_port_pin_val = cur_port_val & 0x01u;
> +        uint8_t cur_ddr_pin_val = cur_ddr_val & 0x01u;
> +        uint8_t new_port_pin_val = value & 0x01u;
...
> +        cur_port_val >>= 1u;
> +        cur_ddr_val >>= 1u;
> +        value >>= 1u;

etc.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 03/11] hw/avr: Add limited support for avr gpio registers

[Mark Cave-Ayland]

On 13/03/2021 16:54, Philippe Mathieu-Daudé wrote:

> From: Heecheol Yang <heecheol.yang@outlook.com>
> 
> Add some of these features for AVR GPIO:
> 
>    - GPIO I/O : PORTx registers
>    - Data Direction : DDRx registers
>    - DDRx toggling : PINx registers
> 
> Following things are not supported yet:
>    - MCUR registers
> 
> Signed-off-by: Heecheol Yang <heecheol.yang@outlook.com>
> Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
> Message-Id: <DM6PR16MB247368DBD3447ABECDD795D7E6090@DM6PR16MB2473.namprd16.prod.outlook.com>
> [PMD: Use AVR_GPIO_COUNT]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/avr/atmega.h            |   2 +
>   include/hw/gpio/avr_gpio.h |  53 ++++++++++++++
>   hw/avr/atmega.c            |   7 +-
>   hw/gpio/avr_gpio.c         | 138 +++++++++++++++++++++++++++++++++++++
>   hw/avr/Kconfig             |   1 +
>   hw/gpio/Kconfig            |   3 +
>   hw/gpio/meson.build        |   1 +
>   7 files changed, 203 insertions(+), 2 deletions(-)
>   create mode 100644 include/hw/gpio/avr_gpio.h
>   create mode 100644 hw/gpio/avr_gpio.c
> 
> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
> index a99ee15c7e1..e2289d5744e 100644
> --- a/hw/avr/atmega.h
> +++ b/hw/avr/atmega.h
> @@ -13,6 +13,7 @@
>   
>   #include "hw/char/avr_usart.h"
>   #include "hw/timer/avr_timer16.h"
> +#include "hw/gpio/avr_gpio.h"
>   #include "hw/misc/avr_power.h"
>   #include "target/avr/cpu.h"
>   #include "qom/object.h"
> @@ -44,6 +45,7 @@ struct AtmegaMcuState {
>       DeviceState *io;
>       AVRMaskState pwr[POWER_MAX];
>       AVRUsartState usart[USART_MAX];
> +    AVRGPIOState gpio[GPIO_MAX];
>       AVRTimer16State timer[TIMER_MAX];
>       uint64_t xtal_freq_hz;
>   };
> diff --git a/include/hw/gpio/avr_gpio.h b/include/hw/gpio/avr_gpio.h
> new file mode 100644
> index 00000000000..498a7275f05
> --- /dev/null
> +++ b/include/hw/gpio/avr_gpio.h
> @@ -0,0 +1,53 @@
> +/*
> + * AVR processors GPIO registers definition.
> + *
> + * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 or
> + * (at your option) version 3 of the License.

Minor nit: isn't the wording of the existing QEMU license "licensed under the terms 
of the GNU GPL, version 2 or later" rather than explicitly stating versions 2 and 3?

> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef AVR_GPIO_H
> +#define AVR_GPIO_H
> +
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
> +
> +/* Offsets of registers. */
> +#define GPIO_PIN   0x00
> +#define GPIO_DDR   0x01
> +#define GPIO_PORT  0x02
> +
> +#define TYPE_AVR_GPIO "avr-gpio"
> +OBJECT_DECLARE_SIMPLE_TYPE(AVRGPIOState, AVR_GPIO)
> +#define AVR_GPIO_COUNT 8
> +
> +struct AVRGPIOState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion mmio;
> +
> +    struct {
> +        uint8_t pin;
> +        uint8_t ddr;
> +        uint8_t port;
> +    } reg;
> +
> +    /* PORTx data changed IRQs */
> +    qemu_irq out[8u];
> +
> +};
> +
> +#endif /* AVR_GPIO_H */
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index 44c6afebbb6..19c3122189f 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -283,8 +283,11 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>               continue;
>           }
>           devname = g_strdup_printf("atmega-gpio-%c", 'a' + (char)i);
> -        create_unimplemented_device(devname,
> -                                    OFFSET_DATA + mc->dev[idx].addr, 3);
> +        object_initialize_child(OBJECT(dev), devname, &s->gpio[i],
> +                                TYPE_AVR_GPIO);
> +        sysbus_realize(SYS_BUS_DEVICE(&s->gpio[i]), &error_abort);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio[i]), 0,
> +                        OFFSET_DATA + mc->dev[idx].addr);
>           g_free(devname);
>       }
>   
> diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
> new file mode 100644
> index 00000000000..cdb574ef0d8
> --- /dev/null
> +++ b/hw/gpio/avr_gpio.c
> @@ -0,0 +1,138 @@
> +/*
> + * AVR processors GPIO registers emulation.
> + *
> + * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 or
> + * (at your option) version 3 of the License.

And here too.

> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/sysbus.h"
> +#include "hw/irq.h"
> +#include "hw/gpio/avr_gpio.h"
> +#include "hw/qdev-properties.h"
> +
> +static void avr_gpio_reset(DeviceState *dev)
> +{
> +    AVRGPIOState *gpio = AVR_GPIO(dev);
> +
> +    gpio->reg.pin = 0u;
> +    gpio->reg.ddr = 0u;
> +    gpio->reg.port = 0u;
> +}
> +
> +static void avr_gpio_write_port(AVRGPIOState *s, uint64_t value)
> +{
> +    uint8_t pin;
> +    uint8_t cur_port_val = s->reg.port;
> +    uint8_t cur_ddr_val = s->reg.ddr;
> +
> +    for (pin = 0u; pin < AVR_GPIO_COUNT ; pin++) {
> +        uint8_t cur_port_pin_val = cur_port_val & 0x01u;
> +        uint8_t cur_ddr_pin_val = cur_ddr_val & 0x01u;
> +        uint8_t new_port_pin_val = value & 0x01u;
> +
> +        if (cur_ddr_pin_val && (cur_port_pin_val != new_port_pin_val)) {
> +            qemu_set_irq(s->out[pin], new_port_pin_val);
> +        }
> +        cur_port_val >>= 1u;
> +        cur_ddr_val >>= 1u;
> +        value >>= 1u;
> +    }
> +    s->reg.port = value & s->reg.ddr;
> +}
> +static uint64_t avr_gpio_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> +    AVRGPIOState *s = (AVRGPIOState *)opaque;
> +    switch (offset) {
> +    case GPIO_PIN:
> +        return s->reg.pin;
> +    case GPIO_DDR:
> +        return s->reg.ddr;
> +    case GPIO_PORT:
> +        return s->reg.port;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +    return 0;
> +}
> +
> +static void avr_gpio_write(void *opaque, hwaddr offset, uint64_t value,
> +                                unsigned int size)
> +{
> +    AVRGPIOState *s = (AVRGPIOState *)opaque;
> +    value = value & 0xF;
> +    switch (offset) {
> +    case GPIO_PIN:
> +        s->reg.pin = value;
> +        s->reg.port ^= s->reg.pin;
> +        break;
> +    case GPIO_DDR:
> +        s->reg.ddr = value;
> +        break;
> +    case GPIO_PORT:
> +        avr_gpio_write_port(s, value);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps avr_gpio_ops = {
> +    .read = avr_gpio_read,
> +    .write = avr_gpio_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void avr_gpio_init(Object *obj)
> +{
> +    AVRGPIOState *s = AVR_GPIO(obj);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->out, ARRAY_SIZE(s->out));
> +    memory_region_init_io(&s->mmio, obj, &avr_gpio_ops, s, TYPE_AVR_GPIO, 3);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +}
> +static void avr_gpio_realize(DeviceState *dev, Error **errp)
> +{
> +    /* Do nothing currently */
> +}
> +
> +
> +static void avr_gpio_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = avr_gpio_reset;
> +    dc->realize = avr_gpio_realize;
> +}
> +
> +static const TypeInfo avr_gpio_info = {
> +    .name          = TYPE_AVR_GPIO,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AVRGPIOState),
> +    .instance_init = avr_gpio_init,
> +    .class_init    = avr_gpio_class_init,
> +};
> +
> +static void avr_gpio_register_types(void)
> +{
> +    type_register_static(&avr_gpio_info);
> +}
> +
> +type_init(avr_gpio_register_types)
> diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
> index d31298c3cce..16a57ced11f 100644
> --- a/hw/avr/Kconfig
> +++ b/hw/avr/Kconfig
> @@ -3,6 +3,7 @@ config AVR_ATMEGA_MCU
>       select AVR_TIMER16
>       select AVR_USART
>       select AVR_POWER
> +    select AVR_GPIO
>   
>   config ARDUINO
>       select AVR_ATMEGA_MCU
> diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
> index f0e7405f6e6..fde7019b2ba 100644
> --- a/hw/gpio/Kconfig
> +++ b/hw/gpio/Kconfig
> @@ -13,3 +13,6 @@ config GPIO_PWR
>   
>   config SIFIVE_GPIO
>       bool
> +
> +config AVR_GPIO
> +    bool
> diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
> index 79568f00ce3..366aca52ca2 100644
> --- a/hw/gpio/meson.build
> +++ b/hw/gpio/meson.build
> @@ -13,3 +13,4 @@
>   softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_gpio.c'))
>   softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_gpio.c'))
>   softmmu_ss.add(when: 'CONFIG_SIFIVE_GPIO', if_true: files('sifive_gpio.c'))
> +softmmu_ss.add(when: 'CONFIG_AVR_GPIO', if_true: files('avr_gpio.c'))

Not that I can see GPL 4 coming on the horizon, but given there was a large exercise 
a while back to clarify the licensing of all the files in the QEMU tree it is worth 
someone who knows more about licensing to check this.


ATB,

Mark.

Re: [PATCH 03/11] hw/avr: Add limited support for avr gpio registers

[Philippe Mathieu-Daudé]

On 3/14/21 11:26 AM, Mark Cave-Ayland wrote:
> On 13/03/2021 16:54, Philippe Mathieu-Daudé wrote:
> 
>> From: Heecheol Yang <heecheol.yang@outlook.com>
>>
>> Add some of these features for AVR GPIO:
>>
>>    - GPIO I/O : PORTx registers
>>    - Data Direction : DDRx registers
>>    - DDRx toggling : PINx registers
>>
>> Following things are not supported yet:
>>    - MCUR registers
>>
>> Signed-off-by: Heecheol Yang <heecheol.yang@outlook.com>
>> Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
>> Message-Id:
>> <DM6PR16MB247368DBD3447ABECDD795D7E6090@DM6PR16MB2473.namprd16.prod.outlook.com>
>>
>> [PMD: Use AVR_GPIO_COUNT]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/avr/atmega.h            |   2 +
>>   include/hw/gpio/avr_gpio.h |  53 ++++++++++++++
>>   hw/avr/atmega.c            |   7 +-
>>   hw/gpio/avr_gpio.c         | 138 +++++++++++++++++++++++++++++++++++++
>>   hw/avr/Kconfig             |   1 +
>>   hw/gpio/Kconfig            |   3 +
>>   hw/gpio/meson.build        |   1 +
>>   7 files changed, 203 insertions(+), 2 deletions(-)
>>   create mode 100644 include/hw/gpio/avr_gpio.h
>>   create mode 100644 hw/gpio/avr_gpio.c
>>
>> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
>> index a99ee15c7e1..e2289d5744e 100644
>> --- a/hw/avr/atmega.h
>> +++ b/hw/avr/atmega.h
>> @@ -13,6 +13,7 @@
>>     #include "hw/char/avr_usart.h"
>>   #include "hw/timer/avr_timer16.h"
>> +#include "hw/gpio/avr_gpio.h"
>>   #include "hw/misc/avr_power.h"
>>   #include "target/avr/cpu.h"
>>   #include "qom/object.h"
>> @@ -44,6 +45,7 @@ struct AtmegaMcuState {
>>       DeviceState *io;
>>       AVRMaskState pwr[POWER_MAX];
>>       AVRUsartState usart[USART_MAX];
>> +    AVRGPIOState gpio[GPIO_MAX];
>>       AVRTimer16State timer[TIMER_MAX];
>>       uint64_t xtal_freq_hz;
>>   };
>> diff --git a/include/hw/gpio/avr_gpio.h b/include/hw/gpio/avr_gpio.h
>> new file mode 100644
>> index 00000000000..498a7275f05
>> --- /dev/null
>> +++ b/include/hw/gpio/avr_gpio.h
>> @@ -0,0 +1,53 @@
>> +/*
>> + * AVR processors GPIO registers definition.
>> + *
>> + * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 or
>> + * (at your option) version 3 of the License.
> 
> Minor nit: isn't the wording of the existing QEMU license "licensed
> under the terms of the GNU GPL, version 2 or later" rather than
> explicitly stating versions 2 and 3?

Good point.

Heecheol Yang, do you mind resending your patch with the correct
license please?

> 
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef AVR_GPIO_H
>> +#define AVR_GPIO_H
>> +
>> +#include "hw/sysbus.h"
>> +#include "qom/object.h"
>> +
>> +/* Offsets of registers. */
>> +#define GPIO_PIN   0x00
>> +#define GPIO_DDR   0x01
>> +#define GPIO_PORT  0x02
>> +
>> +#define TYPE_AVR_GPIO "avr-gpio"
>> +OBJECT_DECLARE_SIMPLE_TYPE(AVRGPIOState, AVR_GPIO)
>> +#define AVR_GPIO_COUNT 8
>> +
>> +struct AVRGPIOState {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +
>> +    /*< public >*/
>> +    MemoryRegion mmio;
>> +
>> +    struct {
>> +        uint8_t pin;
>> +        uint8_t ddr;
>> +        uint8_t port;
>> +    } reg;
>> +
>> +    /* PORTx data changed IRQs */
>> +    qemu_irq out[8u];
>> +
>> +};
>> +
>> +#endif /* AVR_GPIO_H */
>> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
>> index 44c6afebbb6..19c3122189f 100644
>> --- a/hw/avr/atmega.c
>> +++ b/hw/avr/atmega.c
>> @@ -283,8 +283,11 @@ static void atmega_realize(DeviceState *dev,
>> Error **errp)
>>               continue;
>>           }
>>           devname = g_strdup_printf("atmega-gpio-%c", 'a' + (char)i);
>> -        create_unimplemented_device(devname,
>> -                                    OFFSET_DATA + mc->dev[idx].addr, 3);
>> +        object_initialize_child(OBJECT(dev), devname, &s->gpio[i],
>> +                                TYPE_AVR_GPIO);
>> +        sysbus_realize(SYS_BUS_DEVICE(&s->gpio[i]), &error_abort);
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio[i]), 0,
>> +                        OFFSET_DATA + mc->dev[idx].addr);
>>           g_free(devname);
>>       }
>>   diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
>> new file mode 100644
>> index 00000000000..cdb574ef0d8
>> --- /dev/null
>> +++ b/hw/gpio/avr_gpio.c
>> @@ -0,0 +1,138 @@
>> +/*
>> + * AVR processors GPIO registers emulation.
>> + *
>> + * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 or
>> + * (at your option) version 3 of the License.
> 
> And here too.
> 
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> along
>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/module.h"
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/irq.h"
>> +#include "hw/gpio/avr_gpio.h"
>> +#include "hw/qdev-properties.h"
>> +
>> +static void avr_gpio_reset(DeviceState *dev)
>> +{
>> +    AVRGPIOState *gpio = AVR_GPIO(dev);
>> +
>> +    gpio->reg.pin = 0u;
>> +    gpio->reg.ddr = 0u;
>> +    gpio->reg.port = 0u;
>> +}
>> +
>> +static void avr_gpio_write_port(AVRGPIOState *s, uint64_t value)
>> +{
>> +    uint8_t pin;
>> +    uint8_t cur_port_val = s->reg.port;
>> +    uint8_t cur_ddr_val = s->reg.ddr;
>> +
>> +    for (pin = 0u; pin < AVR_GPIO_COUNT ; pin++) {
>> +        uint8_t cur_port_pin_val = cur_port_val & 0x01u;
>> +        uint8_t cur_ddr_pin_val = cur_ddr_val & 0x01u;
>> +        uint8_t new_port_pin_val = value & 0x01u;
>> +
>> +        if (cur_ddr_pin_val && (cur_port_pin_val != new_port_pin_val)) {
>> +            qemu_set_irq(s->out[pin], new_port_pin_val);
>> +        }
>> +        cur_port_val >>= 1u;
>> +        cur_ddr_val >>= 1u;
>> +        value >>= 1u;
>> +    }
>> +    s->reg.port = value & s->reg.ddr;
>> +}
>> +static uint64_t avr_gpio_read(void *opaque, hwaddr offset, unsigned
>> int size)
>> +{
>> +    AVRGPIOState *s = (AVRGPIOState *)opaque;
>> +    switch (offset) {
>> +    case GPIO_PIN:
>> +        return s->reg.pin;
>> +    case GPIO_DDR:
>> +        return s->reg.ddr;
>> +    case GPIO_PORT:
>> +        return s->reg.port;
>> +    default:
>> +        g_assert_not_reached();
>> +        break;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static void avr_gpio_write(void *opaque, hwaddr offset, uint64_t value,
>> +                                unsigned int size)
>> +{
>> +    AVRGPIOState *s = (AVRGPIOState *)opaque;
>> +    value = value & 0xF;
>> +    switch (offset) {
>> +    case GPIO_PIN:
>> +        s->reg.pin = value;
>> +        s->reg.port ^= s->reg.pin;
>> +        break;
>> +    case GPIO_DDR:
>> +        s->reg.ddr = value;
>> +        break;
>> +    case GPIO_PORT:
>> +        avr_gpio_write_port(s, value);
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +        break;
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps avr_gpio_ops = {
>> +    .read = avr_gpio_read,
>> +    .write = avr_gpio_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static void avr_gpio_init(Object *obj)
>> +{
>> +    AVRGPIOState *s = AVR_GPIO(obj);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), s->out, ARRAY_SIZE(s->out));
>> +    memory_region_init_io(&s->mmio, obj, &avr_gpio_ops, s,
>> TYPE_AVR_GPIO, 3);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
>> +}
>> +static void avr_gpio_realize(DeviceState *dev, Error **errp)
>> +{
>> +    /* Do nothing currently */
>> +}
>> +
>> +
>> +static void avr_gpio_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->reset = avr_gpio_reset;
>> +    dc->realize = avr_gpio_realize;
>> +}
>> +
>> +static const TypeInfo avr_gpio_info = {
>> +    .name          = TYPE_AVR_GPIO,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(AVRGPIOState),
>> +    .instance_init = avr_gpio_init,
>> +    .class_init    = avr_gpio_class_init,
>> +};
>> +
>> +static void avr_gpio_register_types(void)
>> +{
>> +    type_register_static(&avr_gpio_info);
>> +}
>> +
>> +type_init(avr_gpio_register_types)
>> diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
>> index d31298c3cce..16a57ced11f 100644
>> --- a/hw/avr/Kconfig
>> +++ b/hw/avr/Kconfig
>> @@ -3,6 +3,7 @@ config AVR_ATMEGA_MCU
>>       select AVR_TIMER16
>>       select AVR_USART
>>       select AVR_POWER
>> +    select AVR_GPIO
>>     config ARDUINO
>>       select AVR_ATMEGA_MCU
>> diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
>> index f0e7405f6e6..fde7019b2ba 100644
>> --- a/hw/gpio/Kconfig
>> +++ b/hw/gpio/Kconfig
>> @@ -13,3 +13,6 @@ config GPIO_PWR
>>     config SIFIVE_GPIO
>>       bool
>> +
>> +config AVR_GPIO
>> +    bool
>> diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
>> index 79568f00ce3..366aca52ca2 100644
>> --- a/hw/gpio/meson.build
>> +++ b/hw/gpio/meson.build
>> @@ -13,3 +13,4 @@
>>   softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_gpio.c'))
>>   softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true:
>> files('aspeed_gpio.c'))
>>   softmmu_ss.add(when: 'CONFIG_SIFIVE_GPIO', if_true:
>> files('sifive_gpio.c'))
>> +softmmu_ss.add(when: 'CONFIG_AVR_GPIO', if_true: files('avr_gpio.c'))
> 
> Not that I can see GPL 4 coming on the horizon, but given there was a
> large exercise a while back to clarify the licensing of all the files in
> the QEMU tree it is worth someone who knows more about licensing to
> check this.
> 
> 
> ATB,
> 
> Mark.
> 

Re: [PATCH 03/11] hw/avr: Add limited support for avr gpio registers

[Sarah Harris]

On Sat, 13 Mar 2021 17:54:37 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> From: Heecheol Yang <heecheol.yang@outlook.com>
> 
> Add some of these features for AVR GPIO:
> 
>   - GPIO I/O : PORTx registers
>   - Data Direction : DDRx registers
>   - DDRx toggling : PINx registers
> 
> Following things are not supported yet:
>   - MCUR registers
> 
> Signed-off-by: Heecheol Yang <heecheol.yang@outlook.com>
> Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
> Message-Id: <DM6PR16MB247368DBD3447ABECDD795D7E6090@DM6PR16MB2473.namprd16.prod.outlook.com>
> [PMD: Use AVR_GPIO_COUNT]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/avr/atmega.h            |   2 +
>  include/hw/gpio/avr_gpio.h |  53 ++++++++++++++
>  hw/avr/atmega.c            |   7 +-
>  hw/gpio/avr_gpio.c         | 138 +++++++++++++++++++++++++++++++++++++
>  hw/avr/Kconfig             |   1 +
>  hw/gpio/Kconfig            |   3 +
>  hw/gpio/meson.build        |   1 +
>  7 files changed, 203 insertions(+), 2 deletions(-)
>  create mode 100644 include/hw/gpio/avr_gpio.h
>  create mode 100644 hw/gpio/avr_gpio.c
> 
> diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
> index a99ee15c7e1..e2289d5744e 100644
> --- a/hw/avr/atmega.h
> +++ b/hw/avr/atmega.h
> @@ -13,6 +13,7 @@
>  
>  #include "hw/char/avr_usart.h"
>  #include "hw/timer/avr_timer16.h"
> +#include "hw/gpio/avr_gpio.h"
>  #include "hw/misc/avr_power.h"
>  #include "target/avr/cpu.h"
>  #include "qom/object.h"
> @@ -44,6 +45,7 @@ struct AtmegaMcuState {
>      DeviceState *io;
>      AVRMaskState pwr[POWER_MAX];
>      AVRUsartState usart[USART_MAX];
> +    AVRGPIOState gpio[GPIO_MAX];
>      AVRTimer16State timer[TIMER_MAX];
>      uint64_t xtal_freq_hz;
>  };
> diff --git a/include/hw/gpio/avr_gpio.h b/include/hw/gpio/avr_gpio.h
> new file mode 100644
> index 00000000000..498a7275f05
> --- /dev/null
> +++ b/include/hw/gpio/avr_gpio.h
> @@ -0,0 +1,53 @@
> +/*
> + * AVR processors GPIO registers definition.
> + *
> + * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 or
> + * (at your option) version 3 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef AVR_GPIO_H
> +#define AVR_GPIO_H
> +
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
> +
> +/* Offsets of registers. */
> +#define GPIO_PIN   0x00
> +#define GPIO_DDR   0x01
> +#define GPIO_PORT  0x02
> +
> +#define TYPE_AVR_GPIO "avr-gpio"
> +OBJECT_DECLARE_SIMPLE_TYPE(AVRGPIOState, AVR_GPIO)
> +#define AVR_GPIO_COUNT 8
> +
> +struct AVRGPIOState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion mmio;
> +
> +    struct {
> +        uint8_t pin;
> +        uint8_t ddr;
> +        uint8_t port;
> +    } reg;
> +
> +    /* PORTx data changed IRQs */
> +    qemu_irq out[8u];
> +
> +};
> +
> +#endif /* AVR_GPIO_H */
> diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
> index 44c6afebbb6..19c3122189f 100644
> --- a/hw/avr/atmega.c
> +++ b/hw/avr/atmega.c
> @@ -283,8 +283,11 @@ static void atmega_realize(DeviceState *dev, Error **errp)
>              continue;
>          }
>          devname = g_strdup_printf("atmega-gpio-%c", 'a' + (char)i);
> -        create_unimplemented_device(devname,
> -                                    OFFSET_DATA + mc->dev[idx].addr, 3);
> +        object_initialize_child(OBJECT(dev), devname, &s->gpio[i],
> +                                TYPE_AVR_GPIO);
> +        sysbus_realize(SYS_BUS_DEVICE(&s->gpio[i]), &error_abort);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio[i]), 0,
> +                        OFFSET_DATA + mc->dev[idx].addr);
>          g_free(devname);
>      }
>  
> diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
> new file mode 100644
> index 00000000000..cdb574ef0d8
> --- /dev/null
> +++ b/hw/gpio/avr_gpio.c
> @@ -0,0 +1,138 @@
> +/*
> + * AVR processors GPIO registers emulation.
> + *
> + * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 or
> + * (at your option) version 3 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/sysbus.h"
> +#include "hw/irq.h"
> +#include "hw/gpio/avr_gpio.h"
> +#include "hw/qdev-properties.h"
> +
> +static void avr_gpio_reset(DeviceState *dev)
> +{
> +    AVRGPIOState *gpio = AVR_GPIO(dev);
> +
> +    gpio->reg.pin = 0u;
> +    gpio->reg.ddr = 0u;
> +    gpio->reg.port = 0u;
> +}
> +
> +static void avr_gpio_write_port(AVRGPIOState *s, uint64_t value)
> +{
> +    uint8_t pin;
> +    uint8_t cur_port_val = s->reg.port;
> +    uint8_t cur_ddr_val = s->reg.ddr;
> +
> +    for (pin = 0u; pin < AVR_GPIO_COUNT ; pin++) {
> +        uint8_t cur_port_pin_val = cur_port_val & 0x01u;
> +        uint8_t cur_ddr_pin_val = cur_ddr_val & 0x01u;
> +        uint8_t new_port_pin_val = value & 0x01u;
> +
> +        if (cur_ddr_pin_val && (cur_port_pin_val != new_port_pin_val)) {
> +            qemu_set_irq(s->out[pin], new_port_pin_val);
> +        }
> +        cur_port_val >>= 1u;
> +        cur_ddr_val >>= 1u;
> +        value >>= 1u;
> +    }
> +    s->reg.port = value & s->reg.ddr;
> +}

Unless I've misunderstood something about how this is designed to be used, I think the logic here and below is slightly wrong.

PORT should be set to the value written, regardless of the state of DDR.
From re-reading the ATmega2560 datasheet (http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-2549-8-bit-AVR-Microcontroller-ATmega640-1280-1281-2560-2561_datasheet.pdf) description of the GPIOs (section 13):
PORT is defined to be read/write in 13.1 and the diagram in 13.2 (by my understanding) indicates that reading PORT gives the state last written.
Reinforcing this, zero bits in DDR indicate a pin in input mode (13.2.3), in which case the corresponding bit in PORT controls the pull up resistor and shouldn't be discarded.

This can be fixed by: s->reg.port = value;

> +static uint64_t avr_gpio_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> +    AVRGPIOState *s = (AVRGPIOState *)opaque;
> +    switch (offset) {
> +    case GPIO_PIN:
> +        return s->reg.pin;
> +    case GPIO_DDR:
> +        return s->reg.ddr;
> +    case GPIO_PORT:
> +        return s->reg.port;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +    return 0;
> +}
> +
> +static void avr_gpio_write(void *opaque, hwaddr offset, uint64_t value,
> +                                unsigned int size)
> +{
> +    AVRGPIOState *s = (AVRGPIOState *)opaque;
> +    value = value & 0xF;
> +    switch (offset) {
> +    case GPIO_PIN:
> +        s->reg.pin = value;
> +        s->reg.port ^= s->reg.pin;
> +        break;
> +    case GPIO_DDR:
> +        s->reg.ddr = value;
> +        break;
> +    case GPIO_PORT:
> +        avr_gpio_write_port(s, value);
> +        break;
> +    default:
> +        g_assert_not_reached();
> +        break;
> +    }
> +}

Again, I think the logic here isn't quite right.
Writing ones to PIN toggles the corresponding bits in PORT (section 13.2.2), but shouldn't change the state of PIN itself because it's read only (section 13.1)
Judging by the diagram in 13.2, PIN is controlled only by the voltage on the pin itself (and synchronisation logic), confirming that it shouldn't be modified here when writes to PORT don't change it.
This code also won't generate pin change interrupts for pins toggled via PIN that would generate interrupts if toggled via PORT.

This can be fixed by:
case GPIO_PIN:
    avr_gpio_write_port(s, s->reg.port^value);
    break;

Also, note that I don't think masking the value with 0xF is right (should be 0xFF), though this is already fixed by a later patch in this series.

Given that this patch generates pin change interrupts when pins states are written, which I assume only happens in hardware due to the voltage on the pin changing, it might actually be arguable that the value read from PIN should always be equal to PORT (i.e. pins set output-high or input-pull-up read high).

> +
> +static const MemoryRegionOps avr_gpio_ops = {
> +    .read = avr_gpio_read,
> +    .write = avr_gpio_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void avr_gpio_init(Object *obj)
> +{
> +    AVRGPIOState *s = AVR_GPIO(obj);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->out, ARRAY_SIZE(s->out));
> +    memory_region_init_io(&s->mmio, obj, &avr_gpio_ops, s, TYPE_AVR_GPIO, 3);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +}
> +static void avr_gpio_realize(DeviceState *dev, Error **errp)
> +{
> +    /* Do nothing currently */
> +}
> +
> +
> +static void avr_gpio_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = avr_gpio_reset;
> +    dc->realize = avr_gpio_realize;
> +}
> +
> +static const TypeInfo avr_gpio_info = {
> +    .name          = TYPE_AVR_GPIO,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AVRGPIOState),
> +    .instance_init = avr_gpio_init,
> +    .class_init    = avr_gpio_class_init,
> +};
> +
> +static void avr_gpio_register_types(void)
> +{
> +    type_register_static(&avr_gpio_info);
> +}
> +
> +type_init(avr_gpio_register_types)
> diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
> index d31298c3cce..16a57ced11f 100644
> --- a/hw/avr/Kconfig
> +++ b/hw/avr/Kconfig
> @@ -3,6 +3,7 @@ config AVR_ATMEGA_MCU
>      select AVR_TIMER16
>      select AVR_USART
>      select AVR_POWER
> +    select AVR_GPIO
>  
>  config ARDUINO
>      select AVR_ATMEGA_MCU
> diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
> index f0e7405f6e6..fde7019b2ba 100644
> --- a/hw/gpio/Kconfig
> +++ b/hw/gpio/Kconfig
> @@ -13,3 +13,6 @@ config GPIO_PWR
>  
>  config SIFIVE_GPIO
>      bool
> +
> +config AVR_GPIO
> +    bool
> diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
> index 79568f00ce3..366aca52ca2 100644
> --- a/hw/gpio/meson.build
> +++ b/hw/gpio/meson.build
> @@ -13,3 +13,4 @@
>  softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_gpio.c'))
>  softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_gpio.c'))
>  softmmu_ss.add(when: 'CONFIG_SIFIVE_GPIO', if_true: files('sifive_gpio.c'))
> +softmmu_ss.add(when: 'CONFIG_AVR_GPIO', if_true: files('avr_gpio.c'))
> -- 
> 2.26.2

Regards,
Sarah Harris

[PATCH 04/11] hw/gpio/avr_gpio: Add migration VMstate

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/gpio/avr_gpio.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
index cdb574ef0d8..da34009daea 100644
--- a/hw/gpio/avr_gpio.c
+++ b/hw/gpio/avr_gpio.c
@@ -25,6 +25,7 @@
 #include "hw/irq.h"
 #include "hw/gpio/avr_gpio.h"
 #include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
 
 static void avr_gpio_reset(DeviceState *dev)
 {
@@ -100,6 +101,18 @@ static const MemoryRegionOps avr_gpio_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static const VMStateDescription avr_gpio_vmstate = {
+    .name = "avr-gpio",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(reg.pin, AVRGPIOState),
+        VMSTATE_UINT8(reg.ddr, AVRGPIOState),
+        VMSTATE_UINT8(reg.port, AVRGPIOState),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
 static void avr_gpio_init(Object *obj)
 {
     AVRGPIOState *s = AVR_GPIO(obj);
@@ -120,6 +133,7 @@ static void avr_gpio_class_init(ObjectClass *klass, void *data)
 
     dc->reset = avr_gpio_reset;
     dc->realize = avr_gpio_realize;
+    dc->vmsd = &avr_gpio_vmstate;
 }
 
 static const TypeInfo avr_gpio_info = {
-- 
2.26.2

Re: [PATCH 04/11] hw/gpio/avr_gpio: Add migration VMstate

[Richard Henderson]

On 3/13/21 10:54 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/gpio/avr_gpio.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 05/11] hw/gpio/avr_gpio: Add 'id' field in AVRGPIOState

[Philippe Mathieu-Daudé]

AVR MCU have various GPIO ports. Add an 'id' property to distinct
all instances.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/gpio/avr_gpio.h |  1 +
 hw/avr/atmega.c            |  1 +
 hw/gpio/avr_gpio.c         | 14 ++++++++++++--
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/hw/gpio/avr_gpio.h b/include/hw/gpio/avr_gpio.h
index 498a7275f05..e982f627ea3 100644
--- a/include/hw/gpio/avr_gpio.h
+++ b/include/hw/gpio/avr_gpio.h
@@ -48,6 +48,7 @@ struct AVRGPIOState {
     /* PORTx data changed IRQs */
     qemu_irq out[8u];
 
+    uint8_t id;
 };
 
 #endif /* AVR_GPIO_H */
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index 19c3122189f..a19e3035022 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -285,6 +285,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         devname = g_strdup_printf("atmega-gpio-%c", 'a' + (char)i);
         object_initialize_child(OBJECT(dev), devname, &s->gpio[i],
                                 TYPE_AVR_GPIO);
+        qdev_prop_set_uint8(DEVICE(&s->gpio[i]), "id", i);
         sysbus_realize(SYS_BUS_DEVICE(&s->gpio[i]), &error_abort);
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio[i]), 0,
                         OFFSET_DATA + mc->dev[idx].addr);
diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
index da34009daea..3db55bfa77f 100644
--- a/hw/gpio/avr_gpio.c
+++ b/hw/gpio/avr_gpio.c
@@ -113,6 +113,11 @@ static const VMStateDescription avr_gpio_vmstate = {
     },
 };
 
+static Property avr_gpio_properties[] = {
+    DEFINE_PROP_UINT8("id", AVRGPIOState, id, UINT8_MAX),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void avr_gpio_init(Object *obj)
 {
     AVRGPIOState *s = AVR_GPIO(obj);
@@ -123,9 +128,13 @@ static void avr_gpio_init(Object *obj)
 }
 static void avr_gpio_realize(DeviceState *dev, Error **errp)
 {
-    /* Do nothing currently */
-}
+    AVRGPIOState *s = AVR_GPIO(dev);
 
+    if (s->id == UINT8_MAX) {
+        error_setg(errp, "property 'id' not set");
+        return;
+    }
+}
 
 static void avr_gpio_class_init(ObjectClass *klass, void *data)
 {
@@ -134,6 +143,7 @@ static void avr_gpio_class_init(ObjectClass *klass, void *data)
     dc->reset = avr_gpio_reset;
     dc->realize = avr_gpio_realize;
     dc->vmsd = &avr_gpio_vmstate;
+    device_class_set_props(dc, avr_gpio_properties);
 }
 
 static const TypeInfo avr_gpio_info = {
-- 
2.26.2

Re: [PATCH 05/11] hw/gpio/avr_gpio: Add 'id' field in AVRGPIOState

[Richard Henderson]

On 3/13/21 10:54 AM, Philippe Mathieu-Daudé wrote:
> +    if (s->id == UINT8_MAX) {
> +        error_setg(errp, "property 'id' not set");
> +        return;
> +    }

This error message would be a tad confusing if one set the id to 255.  What's 
the point?


r~

[PATCH 06/11] hw/gpio/avr_gpio: Simplify avr_gpio_write_port using extract32()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/gpio/avr_gpio.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
index 3db55bfa77f..e4c7122e62c 100644
--- a/hw/gpio/avr_gpio.c
+++ b/hw/gpio/avr_gpio.c
@@ -39,20 +39,15 @@ static void avr_gpio_reset(DeviceState *dev)
 static void avr_gpio_write_port(AVRGPIOState *s, uint64_t value)
 {
     uint8_t pin;
-    uint8_t cur_port_val = s->reg.port;
-    uint8_t cur_ddr_val = s->reg.ddr;
 
     for (pin = 0u; pin < AVR_GPIO_COUNT ; pin++) {
-        uint8_t cur_port_pin_val = cur_port_val & 0x01u;
-        uint8_t cur_ddr_pin_val = cur_ddr_val & 0x01u;
-        uint8_t new_port_pin_val = value & 0x01u;
+        uint8_t cur_port_pin_val = extract32(s->reg.port, pin, 1);
+        uint8_t cur_ddr_pin_val = extract32(s->reg.ddr, pin, 1);
+        uint8_t new_port_pin_val = extract32(value, pin, 1);
 
         if (cur_ddr_pin_val && (cur_port_pin_val != new_port_pin_val)) {
             qemu_set_irq(s->out[pin], new_port_pin_val);
         }
-        cur_port_val >>= 1u;
-        cur_ddr_val >>= 1u;
-        value >>= 1u;
     }
     s->reg.port = value & s->reg.ddr;
 }
-- 
2.26.2

Re: [PATCH 06/11] hw/gpio/avr_gpio: Simplify avr_gpio_write_port using extract32()

[Richard Henderson]

On 3/13/21 10:54 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/gpio/avr_gpio.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 07/11] hw/gpio/avr_gpio: Add tracing for reads and writes

[Philippe Mathieu-Daudé]

From: G S Niteesh Babu <niteesh.gs@gmail.com>

Added tracing for gpio read, write, and update output irq.

1) trace_avr_gpio_update_ouput_irq
2) trace_avr_gpio_read
3) trace_avr_gpio_write

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
Message-Id: <20210311135539.10206-3-niteesh.gs@gmail.com>
[PMD: Added port_name(), display port name in trace events]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/gpio/avr_gpio.c   | 26 +++++++++++++++++++++-----
 hw/gpio/trace-events |  5 +++++
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
index e4c7122e62c..29252d6ccfe 100644
--- a/hw/gpio/avr_gpio.c
+++ b/hw/gpio/avr_gpio.c
@@ -2,6 +2,7 @@
  * AVR processors GPIO registers emulation.
  *
  * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
+ * Copyright (C) 2021 Niteesh Babu G S <niteesh.gs@gmail.com>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -26,6 +27,12 @@
 #include "hw/gpio/avr_gpio.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "trace.h"
+
+static char port_name(AVRGPIOState *s)
+{
+    return 'A' + s->id;
+}
 
 static void avr_gpio_reset(DeviceState *dev)
 {
@@ -47,32 +54,41 @@ static void avr_gpio_write_port(AVRGPIOState *s, uint64_t value)
 
         if (cur_ddr_pin_val && (cur_port_pin_val != new_port_pin_val)) {
             qemu_set_irq(s->out[pin], new_port_pin_val);
+            trace_avr_gpio_update_output_irq(port_name(s), pin, new_port_pin_val);
         }
     }
     s->reg.port = value & s->reg.ddr;
 }
 static uint64_t avr_gpio_read(void *opaque, hwaddr offset, unsigned int size)
 {
+    uint8_t val = 0;
     AVRGPIOState *s = (AVRGPIOState *)opaque;
     switch (offset) {
     case GPIO_PIN:
-        return s->reg.pin;
+        val = s->reg.pin;
+        break;
     case GPIO_DDR:
-        return s->reg.ddr;
+        val = s->reg.ddr;
+        break;
     case GPIO_PORT:
-        return s->reg.port;
+        val = s->reg.port;
+        break;
     default:
         g_assert_not_reached();
         break;
     }
-    return 0;
+
+    trace_avr_gpio_read(port_name(s), offset, val);
+    return val;
 }
 
 static void avr_gpio_write(void *opaque, hwaddr offset, uint64_t value,
                                 unsigned int size)
 {
     AVRGPIOState *s = (AVRGPIOState *)opaque;
-    value = value & 0xF;
+    value = value & 0xFF;
+
+    trace_avr_gpio_write(port_name(s), offset, value);
     switch (offset) {
     case GPIO_PIN:
         s->reg.pin = value;
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index 46ab9323bd0..640834597a8 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -18,3 +18,8 @@ sifive_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 " value 0x%" P
 sifive_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 " value 0x%" PRIx64
 sifive_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value %" PRIi64
 sifive_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 " value %" PRIi64
+
+# avr_gpio.c
+avr_gpio_read(unsigned id, uint64_t offset, uint64_t r) "port %c offset 0x%" PRIx64 " value 0x%" PRIx64
+avr_gpio_write(unsigned id, uint64_t offset, uint64_t value) "port %c offset 0x%" PRIx64 " value 0x%" PRIx64
+avr_gpio_update_output_irq(unsigned id, int64_t line, int64_t value) "port %c pin %" PRIi64 " value %" PRIi64
-- 
2.26.2

Re: [PATCH 07/11] hw/gpio/avr_gpio: Add tracing for reads and writes

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 4018 bytes --]

Reviewed-by: Niteesh G S <niteesh.gs@gmail.com>

On Sat, Mar 13, 2021 at 10:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> From: G S Niteesh Babu <niteesh.gs@gmail.com>
>
> Added tracing for gpio read, write, and update output irq.
>
> 1) trace_avr_gpio_update_ouput_irq
> 2) trace_avr_gpio_read
> 3) trace_avr_gpio_write
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
> Message-Id: <20210311135539.10206-3-niteesh.gs@gmail.com>
> [PMD: Added port_name(), display port name in trace events]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/gpio/avr_gpio.c   | 26 +++++++++++++++++++++-----
>  hw/gpio/trace-events |  5 +++++
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
> index e4c7122e62c..29252d6ccfe 100644
> --- a/hw/gpio/avr_gpio.c
> +++ b/hw/gpio/avr_gpio.c
> @@ -2,6 +2,7 @@
>   * AVR processors GPIO registers emulation.
>   *
>   * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
> + * Copyright (C) 2021 Niteesh Babu G S <niteesh.gs@gmail.com>
>   *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License as
> @@ -26,6 +27,12 @@
>  #include "hw/gpio/avr_gpio.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
> +#include "trace.h"
> +
> +static char port_name(AVRGPIOState *s)
> +{
> +    return 'A' + s->id;
> +}
>
>  static void avr_gpio_reset(DeviceState *dev)
>  {
> @@ -47,32 +54,41 @@ static void avr_gpio_write_port(AVRGPIOState *s,
> uint64_t value)
>
>          if (cur_ddr_pin_val && (cur_port_pin_val != new_port_pin_val)) {
>              qemu_set_irq(s->out[pin], new_port_pin_val);
> +            trace_avr_gpio_update_output_irq(port_name(s), pin,
> new_port_pin_val);
>          }
>      }
>      s->reg.port = value & s->reg.ddr;
>  }
>  static uint64_t avr_gpio_read(void *opaque, hwaddr offset, unsigned int
> size)
>  {
> +    uint8_t val = 0;
>      AVRGPIOState *s = (AVRGPIOState *)opaque;
>      switch (offset) {
>      case GPIO_PIN:
> -        return s->reg.pin;
> +        val = s->reg.pin;
> +        break;
>      case GPIO_DDR:
> -        return s->reg.ddr;
> +        val = s->reg.ddr;
> +        break;
>      case GPIO_PORT:
> -        return s->reg.port;
> +        val = s->reg.port;
> +        break;
>      default:
>          g_assert_not_reached();
>          break;
>      }
> -    return 0;
> +
> +    trace_avr_gpio_read(port_name(s), offset, val);
> +    return val;
>  }
>
>  static void avr_gpio_write(void *opaque, hwaddr offset, uint64_t value,
>                                  unsigned int size)
>  {
>      AVRGPIOState *s = (AVRGPIOState *)opaque;
> -    value = value & 0xF;
> +    value = value & 0xFF;
> +
> +    trace_avr_gpio_write(port_name(s), offset, value);
>      switch (offset) {
>      case GPIO_PIN:
>          s->reg.pin = value;
> diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
> index 46ab9323bd0..640834597a8 100644
> --- a/hw/gpio/trace-events
> +++ b/hw/gpio/trace-events
> @@ -18,3 +18,8 @@ sifive_gpio_read(uint64_t offset, uint64_t r) "offset
> 0x%" PRIx64 " value 0x%" P
>  sifive_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 "
> value 0x%" PRIx64
>  sifive_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value %"
> PRIi64
>  sifive_gpio_update_output_irq(int64_t line, int64_t value) "line %"
> PRIi64 " value %" PRIi64
> +
> +# avr_gpio.c
> +avr_gpio_read(unsigned id, uint64_t offset, uint64_t r) "port %c offset
> 0x%" PRIx64 " value 0x%" PRIx64
> +avr_gpio_write(unsigned id, uint64_t offset, uint64_t value) "port %c
> offset 0x%" PRIx64 " value 0x%" PRIx64
> +avr_gpio_update_output_irq(unsigned id, int64_t line, int64_t value)
> "port %c pin %" PRIi64 " value %" PRIi64
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 5463 bytes --]

Re: [PATCH 07/11] hw/gpio/avr_gpio: Add tracing for reads and writes

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 4301 bytes --]

Hii Phil,

A gentle reminder to push these patches.

Thanks,
Niteesh.

On Sat, Mar 13, 2021 at 10:51 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

> Reviewed-by: Niteesh G S <niteesh.gs@gmail.com>
>
> On Sat, Mar 13, 2021 at 10:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
>
>> From: G S Niteesh Babu <niteesh.gs@gmail.com>
>>
>> Added tracing for gpio read, write, and update output irq.
>>
>> 1) trace_avr_gpio_update_ouput_irq
>> 2) trace_avr_gpio_read
>> 3) trace_avr_gpio_write
>>
>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>> Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
>> Message-Id: <20210311135539.10206-3-niteesh.gs@gmail.com>
>> [PMD: Added port_name(), display port name in trace events]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/gpio/avr_gpio.c   | 26 +++++++++++++++++++++-----
>>  hw/gpio/trace-events |  5 +++++
>>  2 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/gpio/avr_gpio.c b/hw/gpio/avr_gpio.c
>> index e4c7122e62c..29252d6ccfe 100644
>> --- a/hw/gpio/avr_gpio.c
>> +++ b/hw/gpio/avr_gpio.c
>> @@ -2,6 +2,7 @@
>>   * AVR processors GPIO registers emulation.
>>   *
>>   * Copyright (C) 2020 Heecheol Yang <heecheol.yang@outlook.com>
>> + * Copyright (C) 2021 Niteesh Babu G S <niteesh.gs@gmail.com>
>>   *
>>   * This program is free software; you can redistribute it and/or
>>   * modify it under the terms of the GNU General Public License as
>> @@ -26,6 +27,12 @@
>>  #include "hw/gpio/avr_gpio.h"
>>  #include "hw/qdev-properties.h"
>>  #include "migration/vmstate.h"
>> +#include "trace.h"
>> +
>> +static char port_name(AVRGPIOState *s)
>> +{
>> +    return 'A' + s->id;
>> +}
>>
>>  static void avr_gpio_reset(DeviceState *dev)
>>  {
>> @@ -47,32 +54,41 @@ static void avr_gpio_write_port(AVRGPIOState *s,
>> uint64_t value)
>>
>>          if (cur_ddr_pin_val && (cur_port_pin_val != new_port_pin_val)) {
>>              qemu_set_irq(s->out[pin], new_port_pin_val);
>> +            trace_avr_gpio_update_output_irq(port_name(s), pin,
>> new_port_pin_val);
>>          }
>>      }
>>      s->reg.port = value & s->reg.ddr;
>>  }
>>  static uint64_t avr_gpio_read(void *opaque, hwaddr offset, unsigned int
>> size)
>>  {
>> +    uint8_t val = 0;
>>      AVRGPIOState *s = (AVRGPIOState *)opaque;
>>      switch (offset) {
>>      case GPIO_PIN:
>> -        return s->reg.pin;
>> +        val = s->reg.pin;
>> +        break;
>>      case GPIO_DDR:
>> -        return s->reg.ddr;
>> +        val = s->reg.ddr;
>> +        break;
>>      case GPIO_PORT:
>> -        return s->reg.port;
>> +        val = s->reg.port;
>> +        break;
>>      default:
>>          g_assert_not_reached();
>>          break;
>>      }
>> -    return 0;
>> +
>> +    trace_avr_gpio_read(port_name(s), offset, val);
>> +    return val;
>>  }
>>
>>  static void avr_gpio_write(void *opaque, hwaddr offset, uint64_t value,
>>                                  unsigned int size)
>>  {
>>      AVRGPIOState *s = (AVRGPIOState *)opaque;
>> -    value = value & 0xF;
>> +    value = value & 0xFF;
>> +
>> +    trace_avr_gpio_write(port_name(s), offset, value);
>>      switch (offset) {
>>      case GPIO_PIN:
>>          s->reg.pin = value;
>> diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
>> index 46ab9323bd0..640834597a8 100644
>> --- a/hw/gpio/trace-events
>> +++ b/hw/gpio/trace-events
>> @@ -18,3 +18,8 @@ sifive_gpio_read(uint64_t offset, uint64_t r) "offset
>> 0x%" PRIx64 " value 0x%" P
>>  sifive_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 "
>> value 0x%" PRIx64
>>  sifive_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value %"
>> PRIi64
>>  sifive_gpio_update_output_irq(int64_t line, int64_t value) "line %"
>> PRIi64 " value %" PRIi64
>> +
>> +# avr_gpio.c
>> +avr_gpio_read(unsigned id, uint64_t offset, uint64_t r) "port %c offset
>> 0x%" PRIx64 " value 0x%" PRIx64
>> +avr_gpio_write(unsigned id, uint64_t offset, uint64_t value) "port %c
>> offset 0x%" PRIx64 " value 0x%" PRIx64
>> +avr_gpio_update_output_irq(unsigned id, int64_t line, int64_t value)
>> "port %c pin %" PRIi64 " value %" PRIi64
>> --
>> 2.26.2
>>
>>

[-- Attachment #2: Type: text/html, Size: 6269 bytes --]

Re: [PATCH 07/11] hw/gpio/avr_gpio: Add tracing for reads and writes

[Philippe Mathieu-Daudé]

Hi Niteesh,

On 3/23/21 3:42 AM, Niteesh G. S. wrote:
> Hii Phil,
> 
> A gentle reminder to push these patches.

Unfortunately the series is blocked by a legal issue in a previous
patch:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg791443.html

We are pending an update from Heecheol Yang.

Regards,

Phil.

[PATCH 08/11] hw/avr/arduino: Add D13 LED

[Philippe Mathieu-Daudé]

From: G S Niteesh Babu <niteesh.gs@gmail.com>

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
Message-Id: <20210311135539.10206-4-niteesh.gs@gmail.com>
[PMD: Added ArduinoMachineClass::d13_led_portb_bit]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/avr/arduino.c | 18 ++++++++++++++++++
 hw/avr/Kconfig   |  1 +
 2 files changed, 19 insertions(+)

diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
index 3ff31492fa6..73563a35d0d 100644
--- a/hw/avr/arduino.c
+++ b/hw/avr/arduino.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/boards.h"
+#include "hw/misc/led.h"
 #include "atmega.h"
 #include "boot.h"
 #include "qom/object.h"
@@ -22,6 +23,8 @@ struct ArduinoMachineState {
     MachineState parent_obj;
     /*< public >*/
     AtmegaMcuState mcu;
+
+    LEDState *onboard_led;
 };
 typedef struct ArduinoMachineState ArduinoMachineState;
 
@@ -31,6 +34,7 @@ struct ArduinoMachineClass {
     /*< public >*/
     const char *mcu_type;
     uint64_t xtal_hz;
+    unsigned d13_led_portb_bit; /* PORTB GPIO for D13 yellow LED */
 };
 typedef struct ArduinoMachineClass ArduinoMachineClass;
 
@@ -49,6 +53,16 @@ static void arduino_machine_init(MachineState *machine)
                              amc->xtal_hz, &error_abort);
     sysbus_realize(SYS_BUS_DEVICE(&ams->mcu), &error_abort);
 
+    /* Onboard led connected to digital header PIN 13 */
+    ams->onboard_led = led_create_simple(OBJECT(ams),
+                                         GPIO_POLARITY_ACTIVE_HIGH,
+                                         LED_COLOR_YELLOW,
+                                         "D13 LED");
+
+    qdev_connect_gpio_out(DEVICE(&ams->mcu.gpio[1]),
+                          amc->d13_led_portb_bit,
+                          qdev_get_gpio_in(DEVICE(ams->onboard_led), 0));
+
     if (machine->firmware) {
         if (!avr_load_firmware(&ams->mcu.cpu, machine,
                                &ams->mcu.flash, machine->firmware)) {
@@ -83,6 +97,7 @@ static void arduino_duemilanove_class_init(ObjectClass *oc, void *data)
     mc->alias       = "2009";
     amc->mcu_type   = TYPE_ATMEGA168_MCU;
     amc->xtal_hz    = 16 * 1000 * 1000;
+    amc->d13_led_portb_bit = 5;
 };
 
 static void arduino_uno_class_init(ObjectClass *oc, void *data)
@@ -98,6 +113,7 @@ static void arduino_uno_class_init(ObjectClass *oc, void *data)
     mc->alias       = "uno";
     amc->mcu_type   = TYPE_ATMEGA328_MCU;
     amc->xtal_hz    = 16 * 1000 * 1000;
+    amc->d13_led_portb_bit = 5;
 };
 
 static void arduino_mega_class_init(ObjectClass *oc, void *data)
@@ -113,6 +129,7 @@ static void arduino_mega_class_init(ObjectClass *oc, void *data)
     mc->alias       = "mega";
     amc->mcu_type   = TYPE_ATMEGA1280_MCU;
     amc->xtal_hz    = 16 * 1000 * 1000;
+    amc->d13_led_portb_bit = 7;
 };
 
 static void arduino_mega2560_class_init(ObjectClass *oc, void *data)
@@ -128,6 +145,7 @@ static void arduino_mega2560_class_init(ObjectClass *oc, void *data)
     mc->alias       = "mega2560";
     amc->mcu_type   = TYPE_ATMEGA2560_MCU;
     amc->xtal_hz    = 16 * 1000 * 1000; /* CSTCE16M0V53-R0 */
+    amc->d13_led_portb_bit = 7;
 };
 
 static const TypeInfo arduino_machine_types[] = {
diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
index 16a57ced11f..e0d4fc5537a 100644
--- a/hw/avr/Kconfig
+++ b/hw/avr/Kconfig
@@ -8,3 +8,4 @@ config AVR_ATMEGA_MCU
 config ARDUINO
     select AVR_ATMEGA_MCU
     select UNIMP
+    select LED
-- 
2.26.2

Re: [PATCH 08/11] hw/avr/arduino: Add D13 LED

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 3993 bytes --]

Hii Phil,

Just a few mins earlier than me :)

Thanks,
Niteesh

On Sat, Mar 13, 2021 at 10:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> From: G S Niteesh Babu <niteesh.gs@gmail.com>
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
> Message-Id: <20210311135539.10206-4-niteesh.gs@gmail.com>
> [PMD: Added ArduinoMachineClass::d13_led_portb_bit]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/avr/arduino.c | 18 ++++++++++++++++++
>  hw/avr/Kconfig   |  1 +
>  2 files changed, 19 insertions(+)
>
> diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
> index 3ff31492fa6..73563a35d0d 100644
> --- a/hw/avr/arduino.c
> +++ b/hw/avr/arduino.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/boards.h"
> +#include "hw/misc/led.h"
>  #include "atmega.h"
>  #include "boot.h"
>  #include "qom/object.h"
> @@ -22,6 +23,8 @@ struct ArduinoMachineState {
>      MachineState parent_obj;
>      /*< public >*/
>      AtmegaMcuState mcu;
> +
> +    LEDState *onboard_led;
>  };
>  typedef struct ArduinoMachineState ArduinoMachineState;
>
> @@ -31,6 +34,7 @@ struct ArduinoMachineClass {
>      /*< public >*/
>      const char *mcu_type;
>      uint64_t xtal_hz;
> +    unsigned d13_led_portb_bit; /* PORTB GPIO for D13 yellow LED */
>  };
>  typedef struct ArduinoMachineClass ArduinoMachineClass;
>
> @@ -49,6 +53,16 @@ static void arduino_machine_init(MachineState *machine)
>                               amc->xtal_hz, &error_abort);
>      sysbus_realize(SYS_BUS_DEVICE(&ams->mcu), &error_abort);
>
> +    /* Onboard led connected to digital header PIN 13 */
> +    ams->onboard_led = led_create_simple(OBJECT(ams),
> +                                         GPIO_POLARITY_ACTIVE_HIGH,
> +                                         LED_COLOR_YELLOW,
> +                                         "D13 LED");
> +
> +    qdev_connect_gpio_out(DEVICE(&ams->mcu.gpio[1]),
> +                          amc->d13_led_portb_bit,
> +                          qdev_get_gpio_in(DEVICE(ams->onboard_led), 0));
> +
>      if (machine->firmware) {
>          if (!avr_load_firmware(&ams->mcu.cpu, machine,
>                                 &ams->mcu.flash, machine->firmware)) {
> @@ -83,6 +97,7 @@ static void arduino_duemilanove_class_init(ObjectClass
> *oc, void *data)
>      mc->alias       = "2009";
>      amc->mcu_type   = TYPE_ATMEGA168_MCU;
>      amc->xtal_hz    = 16 * 1000 * 1000;
> +    amc->d13_led_portb_bit = 5;
>  };
>
>  static void arduino_uno_class_init(ObjectClass *oc, void *data)
> @@ -98,6 +113,7 @@ static void arduino_uno_class_init(ObjectClass *oc,
> void *data)
>      mc->alias       = "uno";
>      amc->mcu_type   = TYPE_ATMEGA328_MCU;
>      amc->xtal_hz    = 16 * 1000 * 1000;
> +    amc->d13_led_portb_bit = 5;
>  };
>
>  static void arduino_mega_class_init(ObjectClass *oc, void *data)
> @@ -113,6 +129,7 @@ static void arduino_mega_class_init(ObjectClass *oc,
> void *data)
>      mc->alias       = "mega";
>      amc->mcu_type   = TYPE_ATMEGA1280_MCU;
>      amc->xtal_hz    = 16 * 1000 * 1000;
> +    amc->d13_led_portb_bit = 7;
>  };
>
>  static void arduino_mega2560_class_init(ObjectClass *oc, void *data)
> @@ -128,6 +145,7 @@ static void arduino_mega2560_class_init(ObjectClass
> *oc, void *data)
>      mc->alias       = "mega2560";
>      amc->mcu_type   = TYPE_ATMEGA2560_MCU;
>      amc->xtal_hz    = 16 * 1000 * 1000; /* CSTCE16M0V53-R0 */
> +    amc->d13_led_portb_bit = 7;
>  };
>
>  static const TypeInfo arduino_machine_types[] = {
> diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
> index 16a57ced11f..e0d4fc5537a 100644
> --- a/hw/avr/Kconfig
> +++ b/hw/avr/Kconfig
> @@ -8,3 +8,4 @@ config AVR_ATMEGA_MCU
>  config ARDUINO
>      select AVR_ATMEGA_MCU
>      select UNIMP
> +    select LED
> --
> 2.26.2
>
>

[-- Attachment #2: Type: text/html, Size: 5640 bytes --]

Re: [PATCH 08/11] hw/avr/arduino: Add D13 LED

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 4247 bytes --]

Reviewed-by: Niteesh G S <niteesh.gs@gmail.com>

On Sat, Mar 13, 2021 at 10:32 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

> Hii Phil,
>
> Just a few mins earlier than me :)
>
> Thanks,
> Niteesh
>
> On Sat, Mar 13, 2021 at 10:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
>
>> From: G S Niteesh Babu <niteesh.gs@gmail.com>
>>
>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>> Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
>> Message-Id: <20210311135539.10206-4-niteesh.gs@gmail.com>
>> [PMD: Added ArduinoMachineClass::d13_led_portb_bit]
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/avr/arduino.c | 18 ++++++++++++++++++
>>  hw/avr/Kconfig   |  1 +
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
>> index 3ff31492fa6..73563a35d0d 100644
>> --- a/hw/avr/arduino.c
>> +++ b/hw/avr/arduino.c
>> @@ -13,6 +13,7 @@
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>>  #include "hw/boards.h"
>> +#include "hw/misc/led.h"
>>  #include "atmega.h"
>>  #include "boot.h"
>>  #include "qom/object.h"
>> @@ -22,6 +23,8 @@ struct ArduinoMachineState {
>>      MachineState parent_obj;
>>      /*< public >*/
>>      AtmegaMcuState mcu;
>> +
>> +    LEDState *onboard_led;
>>  };
>>  typedef struct ArduinoMachineState ArduinoMachineState;
>>
>> @@ -31,6 +34,7 @@ struct ArduinoMachineClass {
>>      /*< public >*/
>>      const char *mcu_type;
>>      uint64_t xtal_hz;
>> +    unsigned d13_led_portb_bit; /* PORTB GPIO for D13 yellow LED */
>>  };
>>  typedef struct ArduinoMachineClass ArduinoMachineClass;
>>
>> @@ -49,6 +53,16 @@ static void arduino_machine_init(MachineState *machine)
>>                               amc->xtal_hz, &error_abort);
>>      sysbus_realize(SYS_BUS_DEVICE(&ams->mcu), &error_abort);
>>
>> +    /* Onboard led connected to digital header PIN 13 */
>> +    ams->onboard_led = led_create_simple(OBJECT(ams),
>> +                                         GPIO_POLARITY_ACTIVE_HIGH,
>> +                                         LED_COLOR_YELLOW,
>> +                                         "D13 LED");
>> +
>> +    qdev_connect_gpio_out(DEVICE(&ams->mcu.gpio[1]),
>> +                          amc->d13_led_portb_bit,
>> +                          qdev_get_gpio_in(DEVICE(ams->onboard_led), 0));
>> +
>>      if (machine->firmware) {
>>          if (!avr_load_firmware(&ams->mcu.cpu, machine,
>>                                 &ams->mcu.flash, machine->firmware)) {
>> @@ -83,6 +97,7 @@ static void arduino_duemilanove_class_init(ObjectClass
>> *oc, void *data)
>>      mc->alias       = "2009";
>>      amc->mcu_type   = TYPE_ATMEGA168_MCU;
>>      amc->xtal_hz    = 16 * 1000 * 1000;
>> +    amc->d13_led_portb_bit = 5;
>>  };
>>
>>  static void arduino_uno_class_init(ObjectClass *oc, void *data)
>> @@ -98,6 +113,7 @@ static void arduino_uno_class_init(ObjectClass *oc,
>> void *data)
>>      mc->alias       = "uno";
>>      amc->mcu_type   = TYPE_ATMEGA328_MCU;
>>      amc->xtal_hz    = 16 * 1000 * 1000;
>> +    amc->d13_led_portb_bit = 5;
>>  };
>>
>>  static void arduino_mega_class_init(ObjectClass *oc, void *data)
>> @@ -113,6 +129,7 @@ static void arduino_mega_class_init(ObjectClass *oc,
>> void *data)
>>      mc->alias       = "mega";
>>      amc->mcu_type   = TYPE_ATMEGA1280_MCU;
>>      amc->xtal_hz    = 16 * 1000 * 1000;
>> +    amc->d13_led_portb_bit = 7;
>>  };
>>
>>  static void arduino_mega2560_class_init(ObjectClass *oc, void *data)
>> @@ -128,6 +145,7 @@ static void arduino_mega2560_class_init(ObjectClass
>> *oc, void *data)
>>      mc->alias       = "mega2560";
>>      amc->mcu_type   = TYPE_ATMEGA2560_MCU;
>>      amc->xtal_hz    = 16 * 1000 * 1000; /* CSTCE16M0V53-R0 */
>> +    amc->d13_led_portb_bit = 7;
>>  };
>>
>>  static const TypeInfo arduino_machine_types[] = {
>> diff --git a/hw/avr/Kconfig b/hw/avr/Kconfig
>> index 16a57ced11f..e0d4fc5537a 100644
>> --- a/hw/avr/Kconfig
>> +++ b/hw/avr/Kconfig
>> @@ -8,3 +8,4 @@ config AVR_ATMEGA_MCU
>>  config ARDUINO
>>      select AVR_ATMEGA_MCU
>>      select UNIMP
>> +    select LED
>> --
>> 2.26.2
>>
>>

[-- Attachment #2: Type: text/html, Size: 6172 bytes --]

[PATCH 09/11] hw/avr/arduino: Replace magic number by gpio_port_index() call

[Philippe Mathieu-Daudé]

The '1' magic value means 'Port B'. Introduce and use the
gpio_port_index() helper to explicit the port name.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/avr/arduino.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
index 73563a35d0d..87124d17f18 100644
--- a/hw/avr/arduino.c
+++ b/hw/avr/arduino.c
@@ -43,6 +43,12 @@ typedef struct ArduinoMachineClass ArduinoMachineClass;
 DECLARE_OBJ_CHECKERS(ArduinoMachineState, ArduinoMachineClass,
                      ARDUINO_MACHINE, TYPE_ARDUINO_MACHINE)
 
+static unsigned gpio_port_index(char c)
+{
+    assert(c >= 'A' && c < 'A' + GPIO_MAX);
+    return c - 'A';
+}
+
 static void arduino_machine_init(MachineState *machine)
 {
     ArduinoMachineClass *amc = ARDUINO_MACHINE_GET_CLASS(machine);
@@ -59,7 +65,7 @@ static void arduino_machine_init(MachineState *machine)
                                          LED_COLOR_YELLOW,
                                          "D13 LED");
 
-    qdev_connect_gpio_out(DEVICE(&ams->mcu.gpio[1]),
+    qdev_connect_gpio_out(DEVICE(&ams->mcu.gpio[gpio_port_index('B')]),
                           amc->d13_led_portb_bit,
                           qdev_get_gpio_in(DEVICE(ams->onboard_led), 0));
 
-- 
2.26.2

Re: [PATCH 09/11] hw/avr/arduino: Replace magic number by gpio_port_index() call

[Richard Henderson]

On 3/13/21 10:54 AM, Philippe Mathieu-Daudé wrote:
> +static unsigned gpio_port_index(char c)
> +{
> +    assert(c >= 'A' && c < 'A' + GPIO_MAX);
> +    return c - 'A';
> +}

If you're not going to use this for anything else, isn't

#define PORT_B 1

enough?


r~

Re: [PATCH 09/11] hw/avr/arduino: Replace magic number by gpio_port_index() call

[Philippe Mathieu-Daudé]

On 3/13/21 9:02 PM, Richard Henderson wrote:
> On 3/13/21 10:54 AM, Philippe Mathieu-Daudé wrote:
>> +static unsigned gpio_port_index(char c)
>> +{
>> +    assert(c >= 'A' && c < 'A' + GPIO_MAX);
>> +    return c - 'A';
>> +}
> 
> If you're not going to use this for anything else, isn't
> 
> #define PORT_B 1
> 
> enough?

Surely :) I'll squash that in previous patch.

[PATCH 10/11] target/avr: Fix some comment spelling errors

[Philippe Mathieu-Daudé]

From: Lichang Zhao <zhaolichang@huawei.com>

I found that there are many spelling errors in the comments of qemu/target/avr.
I used spellcheck to check the spelling errors and found some errors in the folder.

Signed-off-by: Lichang Zhao <zhaolichang@huawei.com>
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
Reviewed-by: Philippe Mathieu-Daude<f4bug@amsat.org>
Message-Id: <20201009064449.2336-12-zhaolichang@huawei.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index 65880b9928c..b4532de2523 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -98,7 +98,7 @@ int avr_cpu_memory_rw_debug(CPUState *cs, vaddr addr, uint8_t *buf,
 
 hwaddr avr_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
 {
-    return addr; /* I assume 1:1 address correspondance */
+    return addr; /* I assume 1:1 address correspondence */
 }
 
 bool avr_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
@@ -299,7 +299,7 @@ void helper_outb(CPUAVRState *env, uint32_t port, uint32_t data)
 }
 
 /*
- *  this function implements LD instruction when there is a posibility to read
+ *  this function implements LD instruction when there is a possibility to read
  *  from a CPU register
  */
 target_ulong helper_fullrd(CPUAVRState *env, uint32_t addr)
@@ -323,7 +323,7 @@ target_ulong helper_fullrd(CPUAVRState *env, uint32_t addr)
 }
 
 /*
- *  this function implements ST instruction when there is a posibility to write
+ *  this function implements ST instruction when there is a possibility to write
  *  into a CPU register
  */
 void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr)
-- 
2.26.2

[PATCH 11/11] target/avr: Fix interrupt execution

[Philippe Mathieu-Daudé]

From: Ivanov Arkasha <ivanovrkasha@gmail.com>

Only one interrupt is in progress at the moment.
It is only necessary to set to reset interrupt_request
after all interrupts have been executed.

Signed-off-by: Ivanov Arkasha <ivanovrkasha@gmail.com>
Message-Id: <20210312164754.18437-1-arkaisp2021@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index b4532de2523..35e10195940 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -49,7 +49,9 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             cc->tcg_ops->do_interrupt(cs);
 
             env->intsrc &= env->intsrc - 1; /* clear the interrupt */
-            cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
+            if (!env->intsrc) {
+                cs->interrupt_request &= ~CPU_INTERRUPT_HARD;
+            }
 
             ret = true;
         }
-- 
2.26.2

Re: [PATCH 00/11] AVR patch queue for QEMU 6.0

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

Reviewed-by: Michael Rolnik <mrolnik@gmail.com>

On Sat, Mar 13, 2021 at 6:54 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi,
>
> This series contains all the AVR patches I could find on the list.
>
> Niteesh, I fixed minor issues. Do you mind reviewing on top?
>
> Pull request planned for Monday if no problem arises.
>
> Thanks,
>
> Phil.
>
> G S Niteesh Babu (2):
>   hw/gpio/avr_gpio: Add tracing for reads and writes
>   hw/avr/arduino: Add D13 LED
>
> Heecheol Yang (1):
>   hw/avr: Add limited support for avr gpio registers
>
> Ivanov Arkasha (1):
>   target/avr: Fix interrupt execution
>
> Lichang Zhao (1):
>   target/avr: Fix some comment spelling errors
>
> Philippe Mathieu-Daudé (6):
>   hw/misc/led: Add yellow LED
>   hw/avr/arduino: List board schematic links
>   hw/gpio/avr_gpio: Add migration VMstate
>   hw/gpio/avr_gpio: Add 'id' field in AVRGPIOState
>   hw/gpio/avr_gpio: Simplify avr_gpio_write_port using extract32()
>   hw/avr/arduino: Replace magic number by gpio_port_index() call
>
>  hw/avr/atmega.h            |   2 +
>  include/hw/gpio/avr_gpio.h |  54 ++++++++++++
>  include/hw/misc/led.h      |   1 +
>  hw/avr/arduino.c           |  44 +++++++++-
>  hw/avr/atmega.c            |   8 +-
>  hw/gpio/avr_gpio.c         | 173 +++++++++++++++++++++++++++++++++++++
>  hw/misc/led.c              |   1 +
>  target/avr/helper.c        |  10 ++-
>  hw/avr/Kconfig             |   2 +
>  hw/gpio/Kconfig            |   3 +
>  hw/gpio/meson.build        |   1 +
>  hw/gpio/trace-events       |   5 ++
>  12 files changed, 294 insertions(+), 10 deletions(-)
>  create mode 100644 include/hw/gpio/avr_gpio.h
>  create mode 100644 hw/gpio/avr_gpio.c
>
> --
> 2.26.2
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 2421 bytes --]

Re: [PATCH 00/11] AVR patch queue for QEMU 6.0

[Philippe Mathieu-Daudé]

On 3/13/21 5:54 PM, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This series contains all the AVR patches I could find on the list.
> 
> Niteesh, I fixed minor issues. Do you mind reviewing on top?
> 
> Pull request planned for Monday if no problem arises.
> 
> Thanks,
> 
> Phil.
> 
> G S Niteesh Babu (2):
>   hw/gpio/avr_gpio: Add tracing for reads and writes
>   hw/avr/arduino: Add D13 LED
> 
> Heecheol Yang (1):
>   hw/avr: Add limited support for avr gpio registers
> 
> Ivanov Arkasha (1):
>   target/avr: Fix interrupt execution
> 
> Lichang Zhao (1):
>   target/avr: Fix some comment spelling errors
> 
> Philippe Mathieu-Daudé (6):
>   hw/misc/led: Add yellow LED
>   hw/avr/arduino: List board schematic links
>   hw/gpio/avr_gpio: Add migration VMstate
>   hw/gpio/avr_gpio: Add 'id' field in AVRGPIOState
>   hw/gpio/avr_gpio: Simplify avr_gpio_write_port using extract32()
>   hw/avr/arduino: Replace magic number by gpio_port_index() call
> 
>  hw/avr/atmega.h            |   2 +
>  include/hw/gpio/avr_gpio.h |  54 ++++++++++++
>  include/hw/misc/led.h      |   1 +
>  hw/avr/arduino.c           |  44 +++++++++-
>  hw/avr/atmega.c            |   8 +-
>  hw/gpio/avr_gpio.c         | 173 +++++++++++++++++++++++++++++++++++++
>  hw/misc/led.c              |   1 +
>  target/avr/helper.c        |  10 ++-
>  hw/avr/Kconfig             |   2 +
>  hw/gpio/Kconfig            |   3 +
>  hw/gpio/meson.build        |   1 +
>  hw/gpio/trace-events       |   5 ++
>  12 files changed, 294 insertions(+), 10 deletions(-)
>  create mode 100644 include/hw/gpio/avr_gpio.h
>  create mode 100644 hw/gpio/avr_gpio.c

Patches 1, 2, 10, 11 queued to avr-next.