Hi Peter,
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 Fri, Sep 15, 2023 at 4:23 PM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Tue, 5 Sept 2023 at 21:14, Strahinja Jankovic
> <strahinjapjanko...@gmail.com> wrote:
> >
> > Add emulation for PS2-0 and PS2-1 for keyboard/mouse.
> >
> > Signed-off-by: Strahinja Jankovic <strahinja.p.janko...@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
