On 6 August 2018 at 11:01, Steffen Görtz <cont...@steffen-goertz.de> wrote:
> This adds a model of the nRF51 GPIO peripheral.
>
> Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
>
> The nRF51 series microcontrollers support up to 32 GPIO pins in various
> configurations.
> The pins can be used as input pins with pull-ups or pull-down.
> Furthermore, three different output driver modes per level are
> available (disconnected, standard, high-current).
>
> The GPIO-Peripheral has a mechanism for detecting level changes which is
> not featured in this model.
>
> Signed-off-by: Steffen Görtz <cont...@steffen-goertz.de>
I have a couple of style nits below but otherwise this looks good.
> +#define NRF51_GPIO_SIZE 0x1000
> +
> +#define NRF51_GPIO_REG_OUT 0x504
> +#define NRF51_GPIO_REG_OUTSET 0x508
> +#define NRF51_GPIO_REG_OUTCLR 0x50C
> +#define NRF51_GPIO_REG_IN 0x510
> +#define NRF51_GPIO_REG_DIR 0x514
> +#define NRF51_GPIO_REG_DIRSET 0x518
> +#define NRF51_GPIO_REG_DIRCLR 0x51C
> +#define NRF51_GPIO_REG_CNF_START 0x700
> +#define NRF51_GPIO_REG_CNF_END 0x77F
> +
> +#define GPIO_PULLDOWN 1
> +#define GPIO_PULLUP 3
> +
> +/**
"/**" is for doc-comment format comments. Generally we only
need those for functions that are global, not file-local ones.
If you want to do a doc-comment, then you want to do it in the
right syntax, with description of input arguments and result
(eg see include/qemu/bitops.h for some examples). If not, just
start with "/*".
> + * Check if the output driver is connected to the direction switch
> + * given the current configuration and logic level.
> + * It is not differentiated between standard and "high"(-power) drive modes.
> + */
> +static bool is_connected(uint32_t config, uint32_t level)
> +{
> + bool state;
> + uint32_t drive_config = extract32(config, 8, 3);
> +
> + switch (drive_config) {
> + case 0 ... 3:
> + state = true;
> + break;
> + case 4 ... 5:
> + state = level != 0;
> + break;
> + case 6 ... 7:
> + state = level == 0;
> + break;
> + }
> +
> + return state;
> +}
> +
> +static void update_output_irq(Nrf51GPIOState *s, size_t i,
> + bool connected, bool level)
> +{
> + int64_t irq_level = connected ? level : -1;
> + bool old_connected = extract32(s->old_out_connected, i, 1);
> + bool old_level = extract32(s->old_out, i, 1);
> +
> + if ((old_connected != connected) || (old_level != level)) {
> + qemu_set_irq(s->output[i], irq_level);
> + trace_nrf51_gpio_update_output_irq(i, irq_level);
> + }
> +
> + s->old_out = deposit32(s->old_out, i, 1, level);
> + s->old_out_connected = deposit32(s->old_out_connected, i, 1, connected);
> +}
> +
> +static void update_state(Nrf51GPIOState *s)
> +{
> + uint32_t pull;
> + bool connected_out, dir, connected_in, out, input;
> +
> + for (size_t i = 0; i < NRF51_GPIO_PINS; i++) {
> + pull = extract32(s->cnf[i], 2, 2);
> + dir = extract32(s->cnf[i], 0, 1);
> + connected_in = extract32(s->in_mask, i, 1);
> + out = extract32(s->out, i, 1);
> + input = !extract32(s->cnf[i], 1, 1);
> + connected_out = is_connected(s->cnf[i], out) && dir;
> +
> + update_output_irq(s, i, connected_out, out);
> +
> + /** Pin both driven externally and internally */
Inline comments like these should definitely not have the doc-comment
starting indicator.
> + if (connected_out && connected_in) {
> + qemu_log_mask(LOG_GUEST_ERROR, "GPIO pin %zu short circuited\n",
> i);
> + }
> diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
> new file mode 100644
> index 0000000000..11486b09b9
> --- /dev/null
> +++ b/hw/gpio/trace-events
> @@ -0,0 +1,7 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +
> +# hw/gpio/nrf51_gpio.c
> +nrf51_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 " value
> 0x%" PRIx64
> +nrf51_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 "
> value 0x%" PRIx64
> +nrf51_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value 0x%"
> PRIi64
> +nrf51_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 "
> value 0x%" PRIi64
> \ No newline at end of file
You want to make sure there's a newline at the end of the file...
thanks
-- PMM
