Re: [Qemu-devel] [PATCH v2 2/4] m68k: Add NeXTcube keyboard device

[Thomas Huth]

On 29/06/2019 14.02, Philippe Mathieu-Daudé wrote:
> On 6/28/19 8:15 PM, Thomas Huth wrote:
>> It is likely still quite incomplete (e.g. mouse and interrupts are not
>> implemented yet), but it is good enough for keyboard input at the firmware
>> monitor.
[...]
>> +
>> +static uint32_t kbd_read_word(void *opaque, hwaddr addr)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "NeXT kbd read word %"HWADDR_PRIx"\n", addr);
> 
> This sounds odd... a bus working in 32-bit/8-bit but not 16...

I guess the bus is working in 16-bit, too, it's just that the firmware
(and the OS) does not access it in that mode, so neither Bryce nor I
spent time implementing this. It's an "UNIMP" log entry ... so whenever
we see it during runtime, we still can add it.

>> +    return 0;
>> +}
[...]
>> +static void kbd_write_byte(void *opaque, hwaddr addr, uint32_t val)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "NeXT kbd write byte %"HWADDR_PRIx"\n", addr);
>> +}
>> +static void kbd_write_word(void *opaque, hwaddr addr, uint32_t val)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "NeXT kbd write addr %"HWADDR_PRIx"\n", addr);
>> +}
>> +static void kbd_write_long(void *opaque, hwaddr addr, uint32_t val)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "NeXT kbd write long %"HWADDR_PRIx"\n", addr);
>> +}
>> +
>> +static uint64_t kbd_readfn(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    switch (size) {
>> +    case 1:
>> +        return kbd_read_byte(opaque, addr);
>> +    case 2:
>> +        return kbd_read_word(opaque, addr);
>> +    case 4:
>> +        return kbd_read_long(opaque, addr);
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
>> +
>> +static void kbd_writefn(void *opaque, hwaddr addr, uint64_t value,
>> +                        unsigned size)
>> +{
>> +    switch (size) {
>> +    case 1:
>> +        kbd_write_byte(opaque, addr, value);
>> +        break;
>> +    case 2:
>> +        kbd_write_word(opaque, addr, value);
>> +        break;
>> +    case 4:
>> +        kbd_write_long(opaque, addr, value);
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
> 
> Well, you can replace this by:
> 
>     qemu_log_mask(LOG_UNIMP,
>                   "NeXT kbd write size:%u 0x%"HWADDR_PRIx"\n",
>                   size, addr);
> 
> and kill the kbd_write_*() functions and the assert (never
> reached since .valid.max_access_size = 4).

Good idea, I'll do it in v3.

>> +}
>> +
>> +static const MemoryRegionOps kbd_ops = {
>> +    .read = kbd_readfn,
>> +    .write = kbd_writefn,
>> +    .valid.min_access_size = 1,
>> +    .valid.max_access_size = 4,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static void nextkbd_event(void *opaque, int ch)
>> +{
>> +    /*
>> +     * Will want to set vars for caps/num lock
>> +     * if (ch & 0x80) -> key release
>> +     * there's also e0 escaped scancodes that might need to be handled
>> +     */
>> +    queue_code(opaque, ch);
>> +}
>> +
>> +static const unsigned char next_keycodes[128] = {
>> +    0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F,
>> +    0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00,
>> +    0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06,
>> +    0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A,
>> +    0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C,
>> +    0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34,
>> +    0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00,
>> +    0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
>> +    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
>> +};
>> +
>> +static void queue_code(void *opaque, int code)
>> +{
>> +    NextKBDState *s = NEXTKBD(opaque);
>> +    KBDQueue *q = &s->queue;
>> +    int key = code & 0x7F;
> 
> Here 0x7f is ARRAY_SIZE(next_keycodes) - 1, ok.
> Isn't it the KD_KEYMASK definition from above then?

I guess so ... I'll use it in v3.

>> +    int release = code & 0x80;
>> +    static int ext;
>> +
>> +    if (code == 0xE0) {
>> +        ext = 1;
>> +    }
>> +
>> +    if (code == 0x2A || code == 0x1D || code == 0x36) {
>> +        if (code == 0x2A) {
>> +            s->shift = KD_LSHIFT;
>> +        } else if (code == 0x36) {
>> +            s->shift = KD_RSHIFT;
>> +            ext = 0;
>> +        } else if (code == 0x1D && !ext) {
>> +            s->shift = KD_LCOMM;
>> +        } else if (code == 0x1D && ext) {
>> +            ext = 0;
>> +            s->shift = KD_RCOMM;
>> +        }
>> +        return;
>> +    } else if (code == (0x2A | 0x80) || code == (0x1D | 0x80) ||
>> +               code == (0x36 | 0x80)) {
>> +        s->shift = 0;
>> +        return;
>> +    }
>> +
>> +    if (q->count >= KBD_QUEUE_SIZE) {
>> +        return;
>> +    }
>> +
>> +    q->data[q->wptr] = next_keycodes[key] | release;
>> +
>> +    if (++q->wptr == KBD_QUEUE_SIZE) {
>> +        q->wptr = 0;
>> +    }
>> +
>> +    q->count++;
>> +
>> +    /*
>> +     * might need to actually trigger the NeXT irq, but as the keyboard 
>> works
>> +     * at the moment, I'll worry about it later
>> +     */
>> +    /* s->update_irq(s->update_arg, 1); */
>> +}
>> +
>> +static void nextkbd_reset(DeviceState *dev)
>> +{
>> +    NextKBDState *nks = NEXTKBD(dev);
>> +
>> +    memset(&nks->queue, 0, sizeof(KBDQueue));
>> +    nks->shift = 0;
>> +}
>> +
>> +static void nextkbd_realize(DeviceState *dev, Error **errp)
>> +{
>> +    NextKBDState *s = NEXTKBD(dev);
>> +
>> +    memory_region_init_io(&s->mr, OBJECT(dev), &kbd_ops, s, "next.kbd", 
>> 0x1000);
>> +
>> +    qemu_add_kbd_event_handler(nextkbd_event, s);
>> +}
>> +
>> +static void nextkbd_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>> +    dc->realize = nextkbd_realize;
>> +    dc->reset = nextkbd_reset;
>> +}
>> +
>> +static const TypeInfo nextkbd_info = {
>> +    .name          = TYPE_NEXTKBD,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(NextKBDState),
>> +    .class_init    = nextkbd_class_init,
>> +};
>> +
>> +static void nextkbd_register_types(void)
>> +{
>> +    type_register_static(&nextkbd_info);
>> +}
>> +
>> +type_init(nextkbd_register_types)
>> +
>> +void nextkbd_init(void)
>> +{
>> +    DeviceState *dev;
>> +    NextKBDState *nks;
>> +
>> +    dev = qdev_create(NULL, TYPE_NEXTKBD);
>> +    qdev_init_nofail(dev);
>> +
>> +    nks = NEXTKBD(dev);
>> +    memory_region_add_subregion(get_system_memory(), 0x200e000, &nks->mr);
> 
> 0x200e000 -> 0x0200e000 :)

Ok.

>> +}
> 
> Again, nextkbd_init() is board-specific code.
> Could you move this function there?

Will do.

 Thanks,
  Thomas