Thank you for your comments.
I used the PL050 component as a starting point, but I did not clean things up well after I saw it working. I will clean it up before sending the new patch version.
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))
Yes, that makes sense. I will try to improve this.
> +}
> +
> +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.
That makes sense, I'll fix it.
> + }
> + 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.
Ok.
> + 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...
Yes, unfortunately I missed this one before sending it out...
> + 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
Thanks!
Best regards,
Strahinja