[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC Patch 5/5] hw/input: Add Allwinner-A10 PS2 emulation
From: |
Peter Maydell |
Subject: |
Re: [RFC Patch 5/5] hw/input: Add Allwinner-A10 PS2 emulation |
Date: |
Fri, 15 Sep 2023 15:23:27 +0100 |
On Tue, 5 Sept 2023 at 21:14, Strahinja Jankovic
<strahinjapjankovic@gmail.com> wrote:
>
> Add emulation for PS2-0 and PS2-1 for keyboard/mouse.
>
> Signed-off-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
> +static int allwinner_a10_ps2_fctl_is_irq(AwA10PS2State *s)
> +{
> + return (s->regs[REG_INDEX(REG_FCTL)] & FIELD_REG_FCTL_TXRDY_IEN) ||
> + (s->pending &&
> + (s->regs[REG_INDEX(REG_FCTL)] & FIELD_REG_FCTL_RXRDY_IEN));
It looks a little odd that you need a separate s->pending bool here.
Sometimes hardware does odd things, but the most usual situation
is that the pending status of an interrupt is directly reflected
in a register bit somewhere, and "is the interrupt high" logic
is then just "is the pending bit set and is the enable bit set".
Often the bit positions are deliberately the same in the two
registers and then "is an interrupt set" is something like
if (s->regs[REG_INDEX(REG_FCTL)] & s->regs[REG_INDEX(REG_FSTS)] &
(TXRDY_IEN | RXRDY_IEN))
> +}
> +
> +static void allwinner_a10_ps2_update_irq(AwA10PS2State *s)
> +{
> + int level = (s->regs[REG_INDEX(REG_GCTL)] & FIELD_REG_GCTL_INT_EN) &&
> + allwinner_a10_ps2_fctl_is_irq(s);
> +
> + qemu_set_irq(s->irq, level);
> +}
> +
> +static void allwinner_a10_ps2_set_irq(void *opaque, int n, int level)
> +{
> + AwA10PS2State *s = (AwA10PS2State *)opaque;
> +
> + s->pending = level;
> + allwinner_a10_ps2_update_irq(s);
> +}
> +
> +static uint64_t allwinner_a10_ps2_read(void *opaque, hwaddr offset,
> + unsigned size)
> +{
> + AwA10PS2State *s = AW_A10_PS2(opaque);
> + const uint32_t idx = REG_INDEX(offset);
> +
> + switch (offset) {
> + case REG_FSTS:
> + {
> + uint32_t stat = FIELD_REG_FSTS_TX_RDY;
> + if (s->pending) {
> + stat |= FIELD_REG_FSTS_RX_LEVEL1 | FIELD_REG_FSTS_RX_RDY;
> + }
> + return stat;
The logic here also suggests that the code would be simpler if you
keep the TX_RDY and RX_RDY state directly in this register value,
rather than hardcoding TX_RDY to always-set and keeping RX_RDY
in a separate pending field.
> + }
> + break;
> + case REG_DATA:
> + if (s->pending) {
> + s->last = ps2_read_data(s->ps2dev);
> + }
> + return s->last;
You could keep the last returned data in s->regs[REG_IDX(REG_DATA)]
and avoid having to have an extra s->last field in the state struct.
> + case REG_GCTL:
> + {
> + if (allwinner_a10_ps2_fctl_is_irq(s)) {
> + s->regs[idx] |= FIELD_REG_GCTL_INT_FLAG;
> + } else {
> + s->regs[idx] &= FIELD_REG_GCTL_INT_FLAG;
> + }
> + }
> + break;
> + case REG_LCTL:
> + case REG_LSTS:
> + case REG_FCTL:
> + case REG_CLKDR:
> + break;
> + case 0x1C ... AW_A10_PS2_IOSIZE:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds offset 0x%04x\n",
> + __func__, (uint32_t)offset);
> + return 0;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: unimplemented read offset 0x%04x\n",
> + __func__, (uint32_t)offset);
> + return 0;
> + }
> +
> + return s->regs[idx];
> +}
> +
> +static void allwinner_a10_ps2_write(void *opaque, hwaddr offset,
> + uint64_t val, unsigned size)
> +{
> + AwA10PS2State *s = AW_A10_PS2(opaque);
> + const uint32_t idx = REG_INDEX(offset);
> +
> + s->regs[idx] = (uint32_t) val;
> +
> + switch (offset) {
> + case REG_GCTL:
> + allwinner_a10_ps2_update_irq(s);
> + s->regs[idx] &= ~FIELD_REG_GCTL_SOFT_RST;
> + break;
> + case REG_DATA:
> + /* ??? This should toggle the TX interrupt line. */
> + /* ??? This means kbd/mouse can block each other. */
I don't understand this comment. It looks like it was cut-and-pasted
from another device where it was originally written in 2005 (and
I don't understand it there either). We should either understand
what we mean here, or else not have the comment at all...
> + if (s->is_mouse) {
> + ps2_write_mouse(PS2_MOUSE_DEVICE(s->ps2dev), val);
> + } else {
> + ps2_write_keyboard(PS2_KBD_DEVICE(s->ps2dev), val);
> + }
> + break;
> + case REG_LCTL:
> + case REG_LSTS:
> + case REG_FCTL:
> + case REG_FSTS:
> + case REG_CLKDR:
> + break;
> + case 0x1C ... AW_A10_PS2_IOSIZE:
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds offset 0x%04x\n",
> + __func__, (uint32_t)offset);
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: unimplemented write offset 0x%04x\n",
> + __func__, (uint32_t)offset);
> + break;
> + }
> +}
thanks
-- PMM
- [RFC Patch 0/5] Allwinner A10 input/output peripherals, Strahinja Jankovic, 2023/09/05
- [RFC Patch 1/5] hw/display: Allwinner A10 HDMI controller emulation, Strahinja Jankovic, 2023/09/05
- [RFC Patch 2/5] hw/display: Allwinner basic MALI GPU emulation, Strahinja Jankovic, 2023/09/05
- [RFC Patch 3/5] hw/display: Allwinner A10 Display Engine Backend emulation, Strahinja Jankovic, 2023/09/05
- [RFC Patch 5/5] hw/input: Add Allwinner-A10 PS2 emulation, Strahinja Jankovic, 2023/09/05
- Re: [RFC Patch 5/5] hw/input: Add Allwinner-A10 PS2 emulation,
Peter Maydell <=
- [RFC Patch 4/5] hw/display: Allwinner A10 LCDC emulation, Strahinja Jankovic, 2023/09/05