[PATCHv2,1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM

The CG3 framebuffer is a simple 8-bit framebuffer for use with operating
systems such as early Solaris that do not have drivers for TCX.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: Blue Swirl <blauwirbel@gmail.com>
CC: Anthony Liguori <aliguori@amazon.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Bob Breuer <breuerr@mc.net>
CC: Artyom Tarasenko <atar4qemu@gmail.com>
---
 Makefile                          |    2 +-
 default-configs/sparc-softmmu.mak |    1 +
 hw/display/Makefile.objs          |    1 +
 hw/display/cg3.c                  |  358 +++++++++++++++++++++++++++++++++++++
 pc-bios/QEMU,cgthree.bin          |  Bin 0 -> 850 bytes
 pc-bios/README                    |    4 +-
 6 files changed, 363 insertions(+), 3 deletions(-)
 create mode 100644 hw/display/cg3.c
 create mode 100644 pc-bios/QEMU,cgthree.bin

On Sun, Feb 9, 2014 at 2:38 AM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> The CG3 framebuffer is a simple 8-bit framebuffer for use with operating
> systems such as early Solaris that do not have drivers for TCX.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> CC: Blue Swirl <blauwirbel@gmail.com>
> CC: Anthony Liguori <aliguori@amazon.com>
> CC: Peter Maydell <peter.maydell@linaro.org>
> CC: Bob Breuer <breuerr@mc.net>
> CC: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  Makefile                          |    2 +-
>  default-configs/sparc-softmmu.mak |    1 +
>  hw/display/Makefile.objs          |    1 +
>  hw/display/cg3.c                  |  358 +++++++++++++++++++++++++++++++++++++
>  pc-bios/QEMU,cgthree.bin          |  Bin 0 -> 850 bytes
>  pc-bios/README                    |    4 +-
>  6 files changed, 363 insertions(+), 3 deletions(-)
>  create mode 100644 hw/display/cg3.c
>  create mode 100644 pc-bios/QEMU,cgthree.bin
>
> diff --git a/Makefile b/Makefile
> index 807054b..c3c7ccc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -293,7 +293,7 @@ ifdef INSTALL_BLOBS
>  BLOBS=bios.bin bios-256k.bin sgabios.bin vgabios.bin vgabios-cirrus.bin \
>  vgabios-stdvga.bin vgabios-vmware.bin vgabios-qxl.bin \
>  acpi-dsdt.aml q35-acpi-dsdt.aml \
> -ppc_rom.bin openbios-sparc32 openbios-sparc64 openbios-ppc QEMU,tcx.bin \
> +ppc_rom.bin openbios-sparc32 openbios-sparc64 openbios-ppc QEMU,tcx.bin QEMU,cgthree.bin \
>  pxe-e1000.rom pxe-eepro100.rom pxe-ne2k_pci.rom \
>  pxe-pcnet.rom pxe-rtl8139.rom pxe-virtio.rom \
>  efi-e1000.rom efi-eepro100.rom efi-ne2k_pci.rom \
> diff --git a/default-configs/sparc-softmmu.mak b/default-configs/sparc-softmmu.mak
> index 8fc93dd..ab796b3 100644
> --- a/default-configs/sparc-softmmu.mak
> +++ b/default-configs/sparc-softmmu.mak
> @@ -10,6 +10,7 @@ CONFIG_EMPTY_SLOT=y
>  CONFIG_PCNET_COMMON=y
>  CONFIG_LANCE=y
>  CONFIG_TCX=y
> +CONFIG_CG3=y
>  CONFIG_SLAVIO=y
>  CONFIG_CS4231=y
>  CONFIG_GRLIB=y
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index 540df82..7ed76a9 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -28,6 +28,7 @@ obj-$(CONFIG_OMAP) += omap_lcdc.o
>  obj-$(CONFIG_PXA2XX) += pxa2xx_lcd.o
>  obj-$(CONFIG_SM501) += sm501.o
>  obj-$(CONFIG_TCX) += tcx.o
> +obj-$(CONFIG_CG3) += cg3.o
>
>  obj-$(CONFIG_VGA) += vga.o
>
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> new file mode 100644
> index 0000000..3e96356
> --- /dev/null
> +++ b/hw/display/cg3.c
> @@ -0,0 +1,358 @@
> +/*
> + * QEMU CG3 Frame buffer
> + *
> + * Copyright (c) 2012 Bob Breuer
> + * Copyright (c) 2013 Mark Cave-Ayland
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu-common.h"
> +#include "ui/console.h"
> +#include "hw/sysbus.h"
> +#include "hw/loader.h"
> +
> +/* #define DEBUG_CG3 */
> +
> +#define CG3_ROM_FILE  "QEMU,cgthree.bin"
> +#define FCODE_MAX_ROM_SIZE 0x10000
> +
> +#define CG3_REG_SIZE  0x20
> +#define CG3_VRAM_SIZE 0x100000
> +#define CG3_VRAM_OFFSET 0x800000
> +
> +#ifdef DEBUG_CG3
> +#define DPRINTF(fmt, ...)                                       \
> +    printf("CG3: " fmt , ## __VA_ARGS__)
> +#else
> +#define DPRINTF(fmt, ...)
> +#endif
> +

With debug macros its better to use a regular if. Something like:

#ifndef DEBUG_CG3
#define DEBUG_CG3 0
#endif

#define DPRINTF(...) do { \
  if (DEBUG_CG3) { \
    printf(...) \
  } \
} while (0);

The reason being your debug code will always be compile checked
allowing contributors to apply type changes without breaking your
debug code accidently.

> +#define TYPE_CG3 "SUNW,cgthree"
> +#define CG3(obj) OBJECT_CHECK(CG3State, (obj), TYPE_CG3)
> +
> +typedef struct CG3State {
> +    SysBusDevice parent_obj;
> +
> +    QemuConsole *con;
> +    qemu_irq irq;
> +    hwaddr prom_addr;
> +    MemoryRegion vram_mem;
> +    MemoryRegion rom;
> +    MemoryRegion reg;
> +    uint32_t vram_size;
> +    int full_update;
> +    uint8_t regs[16];
> +    uint8_t r[256], g[256], b[256];
> +    uint16_t width, height, depth;
> +    uint8_t dac_index, dac_state;
> +} CG3State;
> +
> +static void cg3_update_display(void *opaque)
> +{
> +    CG3State *s = opaque;
> +    DisplaySurface *surface = qemu_console_surface(s->con);
> +    const uint8_t *pix;
> +    uint32_t *data;
> +    uint32_t dval;
> +    int x, y, y_start;
> +    unsigned int width, height;
> +    ram_addr_t page, page_min, page_max;
> +
> +    if (surface_bits_per_pixel(surface) != 32) {
> +        return;
> +    }
> +    width = s->width;
> +    height = s->height;
> +
> +    y_start = -1;
> +    page_min = -1;
> +    page_max = 0;
> +    page = 0;
> +    pix = memory_region_get_ram_ptr(&s->vram_mem);
> +    data = (uint32_t *)surface_data(surface);
> +
> +    for (y = 0; y < height; y++) {
> +        int update = s->full_update;
> +
> +        page = (y * width) & TARGET_PAGE_MASK;
> +        update |= memory_region_get_dirty(&s->vram_mem, page, page + width,
> +                                          DIRTY_MEMORY_VGA);
> +        if (update) {
> +            if (y_start < 0) {
> +                y_start = y;
> +            }
> +            if (page < page_min) {
> +                page_min = page;
> +            }
> +            if (page > page_max) {
> +                page_max = page;
> +            }
> +
> +            for (x = 0; x < width; x++) {
> +                dval = *pix++;
> +                dval = (s->r[dval] << 16) | (s->g[dval] << 8) | s->b[dval];
> +                *data++ = dval;
> +            }
> +        } else {
> +            if (y_start >= 0) {
> +                dpy_gfx_update(s->con, 0, y_start, s->width, y - y_start);
> +                y_start = -1;
> +            }
> +            pix += width;
> +            data += width;
> +        }
> +    }
> +    s->full_update = 0;
> +    if (y_start >= 0) {
> +        dpy_gfx_update(s->con, 0, y_start, s->width, y - y_start);
> +    }
> +    if (page_max >= page_min) {
> +        memory_region_reset_dirty(&s->vram_mem,
> +                              page_min, page_max - page_min + TARGET_PAGE_SIZE,
> +                              DIRTY_MEMORY_VGA);
> +    }
> +    /* vsync interrupt? */
> +    if (s->regs[0] & 0x80) {
> +        s->regs[1] |= 0x80;
> +        qemu_irq_raise(s->irq);
> +    }
> +}
> +
> +static void cg3_invalidate_display(void *opaque)
> +{
> +    CG3State *s = opaque;
> +
> +    memory_region_set_dirty(&s->vram_mem, 0, CG3_VRAM_SIZE);
> +}
> +
> +static uint64_t cg3_reg_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    CG3State *s = opaque;
> +    int val;
> +
> +    switch (addr) {
> +    case 0x10:

What are these magic numbers? You should define macros matching the
names in your register specs.

> +        val = s->regs[0];

Same for these hardcoded indicies into regs[].

> +        break;
> +    case 0x11:
> +        val = s->regs[1] | 0x71; /* monitor ID 7, board type = 1 (color) */
> +        break;
> +    case 0x12 ... 0x1f:
> +        val = s->regs[addr - 0x10];
> +        break;
> +    default:
> +        val = 0;
> +        break;

Is this guest error or unimplemented behaviour? You should
qemu_log_mask( accordingly.

> +    }
> +    DPRINTF("read %02x from reg %x\n", val, (int)addr);
> +    return val;
> +}
> +
> +static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
> +                          unsigned size)
> +{
> +    CG3State *s = opaque;
> +    uint8_t regval;
> +    int i;
> +
> +    DPRINTF("write %02lx to reg %x size %d\n", val, (int)addr, size);
> +
> +    switch (addr) {
> +    case 0:
> +        s->dac_index = val;
> +        s->dac_state = 0;
> +        break;
> +    case 4:
> +        /* This register can be written to as either a long word or a byte.
> +         * According to the SBus specification, byte transfers are placed
> +         * in bits 31-28 */

Very strange. Is it not just a case of generic big-endian accessible
rather than just such a very specific exception here?

> +        if (size == 1) {
> +            val <<= 24;
> +        }
> +
> +        for (i = 0; i < size; i++) {
> +            regval = val >> 24;
> +
> +            switch (s->dac_state) {
> +            case 0:
> +                s->r[s->dac_index] = regval;
> +                s->dac_state++;
> +                break;
> +            case 1:
> +                s->g[s->dac_index] = regval;
> +                s->dac_state++;
> +                break;
> +            case 2:
> +                s->b[s->dac_index] = regval;
> +                /* Index autoincrement */
> +                s->dac_index = (s->dac_index + 1) & 255;
> +            default:
> +                s->dac_state = 0;
> +                break;
> +            }
> +            val <<= 8;
> +        }
> +        s->full_update = 1;
> +        break;
> +    case 0x10:
> +        s->regs[0] = val;
> +        break;
> +    case 0x11:
> +        if (s->regs[1] & 0x80) {

Define macros for register field bits.

> +            /* clear interrupt */
> +            s->regs[1] &= ~0x80;
> +            qemu_irq_lower(s->irq);
> +        }
> +        break;
> +    case 0x12 ... 0x1f:
> +        s->regs[addr - 0x10] = val;
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps cg3_reg_ops = {
> +    .read = cg3_reg_read,
> +    .write = cg3_reg_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,

Your hander switch statements stride in 4, are you only doing this for
your one exception case of that one-byte big-endian access I commented
earlier.

> +    },
> +};
> +
> +static const GraphicHwOps cg3_ops = {
> +    .invalidate = cg3_invalidate_display,
> +    .gfx_update = cg3_update_display,
> +};
> +
> +static int cg3_init1(SysBusDevice *dev)

Use of SysBusDevice::init functions is depracated. Please use object
init fns or Device::Realize functions instead.

Regards,
Peter

> +{
> +    CG3State *s = CG3(dev);
> +    int ret;
> +    char *fcode_filename;
> +
> +    /* FCode ROM */
> +    memory_region_init_ram(&s->rom, NULL, "cg3.prom", FCODE_MAX_ROM_SIZE);
> +    vmstate_register_ram_global(&s->rom);
> +    memory_region_set_readonly(&s->rom, true);
> +    sysbus_init_mmio(dev, &s->rom);
> +
> +    fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
> +    if (fcode_filename) {
> +        ret = load_image_targphys(fcode_filename, s->prom_addr,
> +                                  FCODE_MAX_ROM_SIZE);
> +        if (ret < 0 || ret > FCODE_MAX_ROM_SIZE) {
> +            fprintf(stderr, "cg3: could not load prom '%s'\n", CG3_ROM_FILE);
> +            return -1;
> +        }
> +    }
> +
> +    memory_region_init_io(&s->reg, NULL, &cg3_reg_ops, s, "cg3.reg",
> +                          CG3_REG_SIZE);
> +    sysbus_init_mmio(dev, &s->reg);
> +
> +    memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size);
> +    vmstate_register_ram_global(&s->vram_mem);
> +    sysbus_init_mmio(dev, &s->vram_mem);
> +
> +    sysbus_init_irq(dev, &s->irq);
> +
> +    s->con = graphic_console_init(DEVICE(dev), &cg3_ops, s);
> +    qemu_console_resize(s->con, s->width, s->height);
> +
> +    return 0;
> +}
> +
> +static int vmstate_cg3_post_load(void *opaque, int version_id)
> +{
> +    CG3State *s = opaque;
> +
> +    cg3_invalidate_display(s);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_cg3 = {
> +    .name = "cg3",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = vmstate_cg3_post_load,
> +    .fields    = (VMStateField[]) {
> +        VMSTATE_UINT16(height, CG3State),
> +        VMSTATE_UINT16(width, CG3State),
> +        VMSTATE_UINT16(depth, CG3State),
> +        VMSTATE_BUFFER(r, CG3State),
> +        VMSTATE_BUFFER(g, CG3State),
> +        VMSTATE_BUFFER(b, CG3State),
> +        VMSTATE_UINT8(dac_index, CG3State),
> +        VMSTATE_UINT8(dac_state, CG3State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void cg3_reset(DeviceState *d)
> +{
> +    CG3State *s = CG3(d);
> +
> +    /* Initialize palette */
> +    memset(s->r, 0, 256);
> +    memset(s->g, 0, 256);
> +    memset(s->b, 0, 256);
> +
> +    s->dac_state = 0;
> +    s->full_update = 1;
> +    qemu_irq_lower(s->irq);
> +}
> +
> +static Property cg3_properties[] = {
> +    DEFINE_PROP_HEX32("vram_size",     CG3State, vram_size, -1),
> +    DEFINE_PROP_UINT16("width",        CG3State, width,     -1),
> +    DEFINE_PROP_UINT16("height",       CG3State, height,    -1),
> +    DEFINE_PROP_UINT16("depth",        CG3State, depth,     -1),
> +    DEFINE_PROP_HEX64("prom_addr",     CG3State, prom_addr, -1),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void cg3_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = cg3_init1;
> +    dc->reset = cg3_reset;
> +    dc->vmsd = &vmstate_cg3;
> +    dc->props = cg3_properties;
> +}
> +
> +static const TypeInfo cg3_info = {
> +    .name          = TYPE_CG3,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(CG3State),
> +    .class_init    = cg3_class_init,
> +};
> +
> +static void cg3_register_types(void)
> +{
> +    type_register_static(&cg3_info);
> +}
> +
> +type_init(cg3_register_types)
> diff --git a/pc-bios/QEMU,cgthree.bin b/pc-bios/QEMU,cgthree.bin
> new file mode 100644
> index 0000000000000000000000000000000000000000..6fec9462070bf9a0f88024c7bf66f444d1d11d61
> GIT binary patch
> literal 850
> zcmZ{i&2HL25XX10!NXT-76XB#Rk_y^s6|SoR%&la53Q7_>KX6`yTvxLO)GA>_ch=J
> zP_0Ed5~+vw1^OC&f^?RT1C@IDd*?SZ|5@Af2Y<wjX;&#S`O9L)^D4_Nujb2jiXgca
> zPDC*9!r1=eIU=;bdQRdZo4=yUd6i|Ci{<)%MIyz_ir4;eaD_K=6J(UtR=nVdN#fcA
> zFNrruCp7i~VGm}B*rM#FYA_wy$!sE!rI=f#Xh;N$<uT(|S$=6UrZaVA++l5xwGGbi
> zu)fC(Rdr#9vwOTXDN4*eUYqPSqhX~xGG|XyEYsmuks~^k)Zx)xil*!=AoCkEsCJ<O
> zoLnnXbubf1(BxVqMqm=>vIAO|=luS}_JT~FP*rk6h2b=zc%GuQBB{~))g@X_rt6=%
> zVK@$>Ha6q}>z8kpvySz{2N@kpEMXb>Jz5ksB_3_=s6dTCOX4v$*W4J65;qbe1Ke=D
> zcrxzKpvBAAAKra@*6Vcb?u%{@nkk;p^!ZDR`PfpU9u9?JLjiUu4_oRe`bNojC4ddA
> z-NOJr!DrG6H~Nkfi8uxm4a5uZ+ZQly!#DLmP9;_lsVKMI5>-P{cC&R96e!56_1J6&
> zm}<Z|R2J&PbKMJ)NOcg^Z|U+yl{R+kL90s5Wj_qOB#i7>1hD{<>+03P;w8TyOmF(b
> yWEu%F;rYw!_h)ClbGu8)^3d%^loP5i*^VudTX8sz;xLL`?}lgvPvCTor|d5SYsmWm
>
> literal 0
> HcmV?d00001
>
> diff --git a/pc-bios/README b/pc-bios/README
> index f190068..af6250a 100644
> --- a/pc-bios/README
> +++ b/pc-bios/README
> @@ -11,8 +11,8 @@
>    firmware implementation. The goal is to implement a 100% IEEE
>    1275-1994 (referred to as Open Firmware) compliant firmware.
>    The included images for PowerPC (for 32 and 64 bit PPC CPUs),
> -  Sparc32 (including QEMU,tcx.bin) and Sparc64 are built from OpenBIOS SVN
> -  revision 1246.
> +  Sparc32 (including QEMU,tcx.bin and QEMU,cgthree.bin) and Sparc64 are built
> +  from OpenBIOS SVN revision 1246.
>
>  - SLOF (Slimline Open Firmware) is a free IEEE 1275 Open Firmware
>    implementation for certain IBM POWER hardware.  The sources are at
> --
> 1.7.10.4
>
>

On 09/02/14 04:14, Peter Crosthwaite wrote:

Hi Peter,

Thanks for the review!

(cut)

>> +/* #define DEBUG_CG3 */
>> +
>> +#define CG3_ROM_FILE  "QEMU,cgthree.bin"
>> +#define FCODE_MAX_ROM_SIZE 0x10000
>> +
>> +#define CG3_REG_SIZE  0x20
>> +#define CG3_VRAM_SIZE 0x100000
>> +#define CG3_VRAM_OFFSET 0x800000
>> +
>> +#ifdef DEBUG_CG3
>> +#define DPRINTF(fmt, ...)                                       \
>> +    printf("CG3: " fmt , ## __VA_ARGS__)
>> +#else
>> +#define DPRINTF(fmt, ...)
>> +#endif
>> +
>
> With debug macros its better to use a regular if. Something like:
>
> #ifndef DEBUG_CG3
> #define DEBUG_CG3 0
> #endif
>
> #define DPRINTF(...) do { \
>    if (DEBUG_CG3) { \
>      printf(...) \
>    } \
> } while (0);
>
> The reason being your debug code will always be compile checked
> allowing contributors to apply type changes without breaking your
> debug code accidently.

Yes, I can see how this would be a good idea and will change it for the 
next version. The reason it is done like this is because where possible 
I've tried to copy the style of the existing TCX driver so that someone 
familiar with one driver can easily work on the other.

(cut)

>> +static uint64_t cg3_reg_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    CG3State *s = opaque;
>> +    int val;
>> +
>> +    switch (addr) {
>> +    case 0x10:
>
> What are these magic numbers? You should define macros matching the
> names in your register specs.
>
>> +        val = s->regs[0];
>
> Same for these hardcoded indicies into regs[].

The short answer is "we don't know" because we don't have any 
documentation. If you compare with the TCX driver, you'll see that it 
uses numbered constants in exactly the same way, for exactly the same 
reason. Few developers are willing to enter into an NDA to get the 
documentation, even Sun doesn't have all of it, and since Oracle took 
over they have removed the few bits that they could find.

So the TCX and the cg3 drivers are generally realised by looking at 
source code from the various OSs and then coming up with something that 
works.

>> +        break;
>> +    case 0x11:
>> +        val = s->regs[1] | 0x71; /* monitor ID 7, board type = 1 (color) */
>> +        break;
>> +    case 0x12 ... 0x1f:
>> +        val = s->regs[addr - 0x10];
>> +        break;
>> +    default:
>> +        val = 0;
>> +        break;
>
> Is this guest error or unimplemented behaviour? You should
> qemu_log_mask( accordingly.

Again "we don't know", but it seems to work.

>> +    }
>> +    DPRINTF("read %02x from reg %x\n", val, (int)addr);
>> +    return val;
>> +}
>> +
>> +static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
>> +                          unsigned size)
>> +{
>> +    CG3State *s = opaque;
>> +    uint8_t regval;
>> +    int i;
>> +
>> +    DPRINTF("write %02lx to reg %x size %d\n", val, (int)addr, size);
>> +
>> +    switch (addr) {
>> +    case 0:
>> +        s->dac_index = val;
>> +        s->dac_state = 0;
>> +        break;
>> +    case 4:
>> +        /* This register can be written to as either a long word or a byte.
>> +         * According to the SBus specification, byte transfers are placed
>> +         * in bits 31-28 */
>
> Very strange. Is it not just a case of generic big-endian accessible
> rather than just such a very specific exception here?

This is an interesting area. Some of the sun4m peripherals are not 
connected directly to the CPU MBus but to an external SBus accessible 
over the processor physical address lines.

Generally all SBus access is done using 4-byte accesses for efficiency, 
however probably due to the age of this driver, Solaris insists on using 
single byte transfers for the cg3 DAC registers which get converted by 
the SBus to 4-byte access but with the byte in the MSB.

So far this is the only driver we've found that tries to access the bus 
in this way, and it would be a large project to switch sun4m from sysbus 
over to something else. Hence I've added and documented the workaround 
in this fashion.

>> +        if (size == 1) {
>> +            val<<= 24;
>> +        }
>> +
>> +        for (i = 0; i<  size; i++) {
>> +            regval = val>>  24;
>> +
>> +            switch (s->dac_state) {
>> +            case 0:
>> +                s->r[s->dac_index] = regval;
>> +                s->dac_state++;
>> +                break;
>> +            case 1:
>> +                s->g[s->dac_index] = regval;
>> +                s->dac_state++;
>> +                break;
>> +            case 2:
>> +                s->b[s->dac_index] = regval;
>> +                /* Index autoincrement */
>> +                s->dac_index = (s->dac_index + 1)&  255;
>> +            default:
>> +                s->dac_state = 0;
>> +                break;
>> +            }
>> +            val<<= 8;
>> +        }
>> +        s->full_update = 1;
>> +        break;
>> +    case 0x10:
>> +        s->regs[0] = val;
>> +        break;
>> +    case 0x11:
>> +        if (s->regs[1]&  0x80) {
>
> Define macros for register field bits.

While we don't know the official name for this, I could invent one for 
this particular case if required?

>> +            /* clear interrupt */
>> +            s->regs[1]&= ~0x80;
>> +            qemu_irq_lower(s->irq);
>> +        }
>> +        break;
>> +    case 0x12 ... 0x1f:
>> +        s->regs[addr - 0x10] = val;
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps cg3_reg_ops = {
>> +    .read = cg3_reg_read,
>> +    .write = cg3_reg_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 4,
>
> Your hander switch statements stride in 4, are you only doing this for
> your one exception case of that one-byte big-endian access I commented
> earlier.

Yes, that is correct.

>> +    },
>> +};
>> +
>> +static const GraphicHwOps cg3_ops = {
>> +    .invalidate = cg3_invalidate_display,
>> +    .gfx_update = cg3_update_display,
>> +};
>> +
>> +static int cg3_init1(SysBusDevice *dev)
>
> Use of SysBusDevice::init functions is depracated. Please use object
> init fns or Device::Realize functions instead.

Right. As I mentioned earlier in the email, I tried to base the CG3 
driver on the existing TCX driver to keep things similar. Does it make 
sense to switch this patch over to use object init functions while TCX 
doesn't?

Also can the object init fns (I guess this is QOM?) still make use of 
sysbus?


Many thanks,

Mark.

On 8 February 2014 16:38, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> The CG3 framebuffer is a simple 8-bit framebuffer for use with operating
> systems such as early Solaris that do not have drivers for TCX.
>
> +static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
> +                          unsigned size)
> +{
> +    CG3State *s = opaque;
> +    uint8_t regval;
> +    int i;
> +
> +    DPRINTF("write %02lx to reg %x size %d\n", val, (int)addr, size);
> +
> +    switch (addr) {
> +    case 0:
> +        s->dac_index = val;
> +        s->dac_state = 0;
> +        break;
> +    case 4:
> +        /* This register can be written to as either a long word or a byte.
> +         * According to the SBus specification, byte transfers are placed
> +         * in bits 31-28 */
> +        if (size == 1) {
> +            val <<= 24;
> +        }

I'm a little reluctant to start talking about endianness again :-)
but that "if (size == 1)" comment looks a little odd to me. The SBus
spec says that SBus is a big-endian bus (which probably means
that the .endianness in the memops struct should be
DEVICE_BIG_ENDIAN though for SPARC targets it won't make
any actual difference)". So while it's true that for SBus you get
byte transfers in data lines 31..28 (and also possibly on some of
the other data lines if the address is not 4-aligned or if the master
just feels like sending the byte on all four lanes at once), I don't
think that's why you're doing this shift. The shift is presumably
because the behaviour of this specific device is "I treat a byte
write like a write to the most significant byte of this register",
not because of the specifics of how SBus defines a byte transfer
to occur.

thanks
-- PMM

Hi,

Am 08.02.2014 17:38, schrieb Mark Cave-Ayland:
> +static Property cg3_properties[] = {
> +    DEFINE_PROP_HEX32("vram_size",     CG3State, vram_size, -1),

Paolo is about to drop hex32 ...

> +    DEFINE_PROP_UINT16("width",        CG3State, width,     -1),
> +    DEFINE_PROP_UINT16("height",       CG3State, height,    -1),
> +    DEFINE_PROP_UINT16("depth",        CG3State, depth,     -1),
> +    DEFINE_PROP_HEX64("prom_addr",     CG3State, prom_addr, -1),

...and hex64 types. Please use UINT32 and UINT64 instead.

> +    DEFINE_PROP_END_OF_LIST(),
> +};

And as far as QOM and style is concerned, please don't use TCX as excuse
to do things the old way. If you feel you need consistency, feel free to
clean up TCX, but we only require it for new patches. Otherwise no
progress would be possible within the code base.

Regards,
Andreas

On 09/02/14 14:41, Peter Maydell wrote:

> On 8 February 2014 16:38, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk>  wrote:
>> The CG3 framebuffer is a simple 8-bit framebuffer for use with operating
>> systems such as early Solaris that do not have drivers for TCX.
>>
>> +static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
>> +                          unsigned size)
>> +{
>> +    CG3State *s = opaque;
>> +    uint8_t regval;
>> +    int i;
>> +
>> +    DPRINTF("write %02lx to reg %x size %d\n", val, (int)addr, size);
>> +
>> +    switch (addr) {
>> +    case 0:
>> +        s->dac_index = val;
>> +        s->dac_state = 0;
>> +        break;
>> +    case 4:
>> +        /* This register can be written to as either a long word or a byte.
>> +         * According to the SBus specification, byte transfers are placed
>> +         * in bits 31-28 */
>> +        if (size == 1) {
>> +            val<<= 24;
>> +        }
>
> I'm a little reluctant to start talking about endianness again :-)
> but that "if (size == 1)" comment looks a little odd to me. The SBus
> spec says that SBus is a big-endian bus (which probably means
> that the .endianness in the memops struct should be
> DEVICE_BIG_ENDIAN though for SPARC targets it won't make
> any actual difference)". So while it's true that for SBus you get
> byte transfers in data lines 31..28 (and also possibly on some of
> the other data lines if the address is not 4-aligned or if the master
> just feels like sending the byte on all four lanes at once), I don't
> think that's why you're doing this shift. The shift is presumably
> because the behaviour of this specific device is "I treat a byte
> write like a write to the most significant byte of this register",
> not because of the specifics of how SBus defines a byte transfer
> to occur.

It's been a little while since I looked, however this was my 
interpretation of the table 3-12 on p.66 of the SBus specification. 
While that particular table refers to the acknowledgment cycle from the 
slave back to the master, it seems to work here in the same way when 
transferring between the master and the slave.

One of things we do know about that card is that the DAC is a BT408, 
where the 256 * 3 shift register for the RGB values is seemingly 
controller by register 4. And I'm fairly sure that I've seen a mixture 
of byte and 4-byte accesses to the card coming from the Solaris driver 
which is why it's necessary to support both accesses in this way :/

There is also a related comment on page 80 explaining how bus sizing can 
be used to allow 8-bit framebuffer to be driven in exactly the same way 
as a 32-bit framebuffer.

FWIW I copied the .endianness part over from the TCX driver, so if you 
feel for correctness that .endianness needs to be DEVICE_BIG_ENDIAN for 
TCX too then I'm happy to submit a patch for that too.


ATB,

Mark.

On 09/02/14 15:10, Andreas Färber wrote:

Hi Andreas,

> Hi,
>
> Am 08.02.2014 17:38, schrieb Mark Cave-Ayland:
>> +static Property cg3_properties[] = {
>> +    DEFINE_PROP_HEX32("vram_size",     CG3State, vram_size, -1),
>
> Paolo is about to drop hex32 ...

Hmmmm okay...

>> +    DEFINE_PROP_UINT16("width",        CG3State, width,     -1),
>> +    DEFINE_PROP_UINT16("height",       CG3State, height,    -1),
>> +    DEFINE_PROP_UINT16("depth",        CG3State, depth,     -1),
>> +    DEFINE_PROP_HEX64("prom_addr",     CG3State, prom_addr, -1),
>
> ...and hex64 types. Please use UINT32 and UINT64 instead.

Alright I can change those for the next version of the patch. Does that 
mean the use of hex output is now a display option rather than a 
separate property type?

>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>
> And as far as QOM and style is concerned, please don't use TCX as excuse
> to do things the old way. If you feel you need consistency, feel free to
> clean up TCX, but we only require it for new patches. Otherwise no
> progress would be possible within the code base.

Okay, I understand this. I'm highly tempted to submit a clean-up patch 
for TCX too when this is complete just to keep both drivers in a similar 
state.

One thing I'm not sure about is how the QOM stuff interacts with sysbus 
- can you quickly point me towards an existing device that does this so 
I can understand how this works? These patches have been around for 
months and I really want to get them in for QEMU 2.0 if freeze is coming 
up soon.


Many thanks,

Mark.

On 9 February 2014 15:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 09/02/14 14:41, Peter Maydell wrote:
>
>> On 8 February 2014 16:38, Mark Cave-Ayland
>> <mark.cave-ayland@ilande.co.uk>  wrote:
>>> +    case 4:
>>> +        /* This register can be written to as either a long word or a
>>> byte.
>>> +         * According to the SBus specification, byte transfers are
>>> placed
>>> +         * in bits 31-28 */
>>> +        if (size == 1) {
>>> +            val<<= 24;
>>> +        }
>>
>>
>> I'm a little reluctant to start talking about endianness again :-)
>> but that "if (size == 1)" comment looks a little odd to me. The SBus
>> spec says that SBus is a big-endian bus (which probably means
>> that the .endianness in the memops struct should be
>> DEVICE_BIG_ENDIAN though for SPARC targets it won't make
>> any actual difference)". So while it's true that for SBus you get
>> byte transfers in data lines 31..28 (and also possibly on some of
>> the other data lines if the address is not 4-aligned or if the master
>> just feels like sending the byte on all four lanes at once), I don't
>> think that's why you're doing this shift. The shift is presumably
>> because the behaviour of this specific device is "I treat a byte
>> write like a write to the most significant byte of this register",
>> not because of the specifics of how SBus defines a byte transfer
>> to occur.
>
>
> It's been a little while since I looked, however this was my interpretation
> of the table 3-12 on p.66 of the SBus specification. While that particular
> table refers to the acknowledgment cycle from the slave back to the master,
> it seems to work here in the same way when transferring between the master
> and the slave.

It's not that I think your comment is wrong about how SBus does
byte transfers, or that the code is wrong, I just don't think that the
reason for the code is the SBus behaviour; it's the device behaviour.
QEMU abstracts a bus's mechanisms for transferring data into the
MemoryRegion ops, which means that for a byte access you'll
always get called with the byte value in the low 8 bits of the input
argument. This would be true whether SBus passed byte values in
lines 31..28, 7..0 or by sending them one bit at a time on D13. The
relation between a byte write and a word write here is a property
of the device, not the bus.

(You'll notice that 3-12 allows the slave to send the byte data
in various different lanes depending on which size acknowlegement
it chooses to send. The documentation of what the master does
is on page 53 and actually says you have to send it in two lanes
for a non-aligned address, and certainly QEMU won't do anything
corresponding to that. In any case I don't think it's relevant here.)

thanks
-- PMM

Am 09.02.2014 16:24, schrieb Mark Cave-Ayland:
> One thing I'm not sure about is how the QOM stuff interacts with sysbus
> - can you quickly point me towards an existing device that does this so
> I can understand how this works?

The ARM MPCore devices come to mind. But really all you've been asked to
do in that aspect is replace dc->init with dc->realize and adapt the
signature to make it compile. :) A bonus would be to separate between
.instance_init and dc->realize, with MemoryRegion initialization and
anything that does not rely on properties going into the initfn, and
everything relying on properties being specified or affecting global
state (such as RAM registration, VMState registration, ...) going into
the realizefn.

> These patches have been around for
> months and I really want to get them in for QEMU 2.0 if freeze is coming
> up soon.

Apart from Blue's absence I see no major blocker to that. :)

I initially wondered if -vga wasn't considered a legacy option, but I
figured we can't create SysBus/SBus devices through -device, so no
alternative. SBus QOM base class refactorings as discussed for an
earlier device can be applied on top and are not strictly needed for 2.0
unless you have more such devices coming up.

Cheers,
Andreas

Il 09/02/2014 16:24, Mark Cave-Ayland ha scritto:
>
> Alright I can change those for the next version of the patch. Does that
> mean the use of hex output is now a display option rather than a
> separate property type?

"info qtree" will always print both decimal and hex.

Paolo

On Sun, Feb 9, 2014 at 11:35 PM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 09/02/14 04:14, Peter Crosthwaite wrote:
>
> Hi Peter,
>
> Thanks for the review!
>
> (cut)
>
>
>>> +/* #define DEBUG_CG3 */
>>> +
>>> +#define CG3_ROM_FILE  "QEMU,cgthree.bin"
>>> +#define FCODE_MAX_ROM_SIZE 0x10000
>>> +
>>> +#define CG3_REG_SIZE  0x20
>>> +#define CG3_VRAM_SIZE 0x100000
>>> +#define CG3_VRAM_OFFSET 0x800000
>>> +
>>> +#ifdef DEBUG_CG3
>>> +#define DPRINTF(fmt, ...)                                       \
>>> +    printf("CG3: " fmt , ## __VA_ARGS__)
>>> +#else
>>> +#define DPRINTF(fmt, ...)
>>> +#endif
>>> +
>>
>>
>> With debug macros its better to use a regular if. Something like:
>>
>> #ifndef DEBUG_CG3
>> #define DEBUG_CG3 0
>> #endif
>>
>> #define DPRINTF(...) do { \
>>    if (DEBUG_CG3) { \
>>      printf(...) \
>>    } \
>> } while (0);
>>
>> The reason being your debug code will always be compile checked
>> allowing contributors to apply type changes without breaking your
>> debug code accidently.
>
>
> Yes, I can see how this would be a good idea and will change it for the next
> version. The reason it is done like this is because where possible I've
> tried to copy the style of the existing TCX driver so that someone familiar
> with one driver can easily work on the other.
>
> (cut)
>
>
>>> +static uint64_t cg3_reg_read(void *opaque, hwaddr addr, unsigned size)
>>> +{
>>> +    CG3State *s = opaque;
>>> +    int val;
>>> +
>>> +    switch (addr) {
>>> +    case 0x10:
>>
>>
>> What are these magic numbers? You should define macros matching the
>> names in your register specs.
>>
>>> +        val = s->regs[0];
>>
>>
>> Same for these hardcoded indicies into regs[].
>
>
> The short answer is "we don't know" because we don't have any documentation.

Sigh.... This has happened quite a lot lately.

If the kernel driver has macros, re-use them as much as possible. If
you have a vague idea on whats, what, a few well invented names would
help the device self-documentation.

> If you compare with the TCX driver, you'll see that it uses numbered
> constants in exactly the same way, for exactly the same reason. Few
> developers are willing to enter into an NDA to get the documentation, even
> Sun doesn't have all of it, and since Oracle took over they have removed the
> few bits that they could find.
>
> So the TCX and the cg3 drivers are generally realised by looking at source
> code from the various OSs and then coming up with something that works.
>
>
>>> +        break;
>>> +    case 0x11:
>>> +        val = s->regs[1] | 0x71; /* monitor ID 7, board type = 1 (color)
>>> */
>>> +        break;
>>> +    case 0x12 ... 0x1f:
>>> +        val = s->regs[addr - 0x10];
>>> +        break;
>>> +    default:
>>> +        val = 0;
>>> +        break;
>>
>>
>> Is this guest error or unimplemented behaviour? You should
>> qemu_log_mask( accordingly.
>
>
> Again "we don't know", but it seems to work.
>

With no-spec devices, we have been encoraging use of qemu_log_mask(LOG_UNIMP

>
>>> +    }
>>> +    DPRINTF("read %02x from reg %x\n", val, (int)addr);
>>> +    return val;
>>> +}
>>> +
>>> +static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
>>> +                          unsigned size)
>>> +{
>>> +    CG3State *s = opaque;
>>> +    uint8_t regval;
>>> +    int i;
>>> +
>>> +    DPRINTF("write %02lx to reg %x size %d\n", val, (int)addr, size);
>>> +
>>> +    switch (addr) {
>>> +    case 0:
>>> +        s->dac_index = val;
>>> +        s->dac_state = 0;
>>> +        break;
>>> +    case 4:
>>> +        /* This register can be written to as either a long word or a
>>> byte.
>>> +         * According to the SBus specification, byte transfers are
>>> placed
>>> +         * in bits 31-28 */
>>
>>
>> Very strange. Is it not just a case of generic big-endian accessible
>> rather than just such a very specific exception here?
>
>
> This is an interesting area. Some of the sun4m peripherals are not connected
> directly to the CPU MBus but to an external SBus accessible over the
> processor physical address lines.
>
> Generally all SBus access is done using 4-byte accesses for efficiency,
> however probably due to the age of this driver, Solaris insists on using
> single byte transfers for the cg3 DAC registers which get converted by the
> SBus to 4-byte access but with the byte in the MSB.
>
> So far this is the only driver we've found that tries to access the bus in
> this way, and it would be a large project to switch sun4m from sysbus over
> to something else. Hence I've added and documented the workaround in this
> fashion.
>
>>> +        if (size == 1) {
>>> +            val<<= 24;
>>> +        }
>>> +
>>> +        for (i = 0; i<  size; i++) {
>>> +            regval = val>>  24;
>>> +
>>> +            switch (s->dac_state) {
>>> +            case 0:
>>> +                s->r[s->dac_index] = regval;
>>> +                s->dac_state++;
>>> +                break;
>>> +            case 1:
>>> +                s->g[s->dac_index] = regval;
>>> +                s->dac_state++;
>>> +                break;
>>> +            case 2:
>>> +                s->b[s->dac_index] = regval;
>>> +                /* Index autoincrement */
>>> +                s->dac_index = (s->dac_index + 1)&  255;
>>>
>>> +            default:
>>> +                s->dac_state = 0;
>>> +                break;
>>> +            }
>>> +            val<<= 8;
>>> +        }
>>> +        s->full_update = 1;
>>> +        break;
>>> +    case 0x10:
>>> +        s->regs[0] = val;
>>> +        break;
>>> +    case 0x11:
>>> +        if (s->regs[1]&  0x80) {
>>
>>
>> Define macros for register field bits.
>
>
> While we don't know the official name for this, I could invent one for this
> particular case if required?
>
>
>>> +            /* clear interrupt */
>>> +            s->regs[1]&= ~0x80;
>>> +            qemu_irq_lower(s->irq);
>>> +        }
>>> +        break;
>>> +    case 0x12 ... 0x1f:
>>> +        s->regs[addr - 0x10] = val;
>>> +        break;
>>> +    default:
>>> +        break;
>>> +    }
>>> +}
>>> +
>>> +static const MemoryRegionOps cg3_reg_ops = {
>>> +    .read = cg3_reg_read,
>>> +    .write = cg3_reg_write,
>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>> +    .valid = {
>>> +        .min_access_size = 1,
>>> +        .max_access_size = 4,
>>
>>
>> Your hander switch statements stride in 4, are you only doing this for
>> your one exception case of that one-byte big-endian access I commented
>> earlier.
>
>
> Yes, that is correct.
>

Should you trap misaligned accesses then?

Regards,
Peter

>
>>> +    },
>>> +};
>>> +
>>> +static const GraphicHwOps cg3_ops = {
>>> +    .invalidate = cg3_invalidate_display,
>>> +    .gfx_update = cg3_update_display,
>>> +};
>>> +
>>> +static int cg3_init1(SysBusDevice *dev)
>>
>>
>> Use of SysBusDevice::init functions is depracated. Please use object
>> init fns or Device::Realize functions instead.
>
>
> Right. As I mentioned earlier in the email, I tried to base the CG3 driver
> on the existing TCX driver to keep things similar. Does it make sense to
> switch this patch over to use object init functions while TCX doesn't?
>
> Also can the object init fns (I guess this is QOM?) still make use of
> sysbus?
>
>
> Many thanks,
>
> Mark.
>

On 09/02/14 15:33, Peter Maydell wrote:

>> It's been a little while since I looked, however this was my interpretation
>> of the table 3-12 on p.66 of the SBus specification. While that particular
>> table refers to the acknowledgment cycle from the slave back to the master,
>> it seems to work here in the same way when transferring between the master
>> and the slave.
>
> It's not that I think your comment is wrong about how SBus does
> byte transfers, or that the code is wrong, I just don't think that the
> reason for the code is the SBus behaviour; it's the device behaviour.
> QEMU abstracts a bus's mechanisms for transferring data into the
> MemoryRegion ops, which means that for a byte access you'll
> always get called with the byte value in the low 8 bits of the input
> argument. This would be true whether SBus passed byte values in
> lines 31..28, 7..0 or by sending them one bit at a time on D13. The
> relation between a byte write and a word write here is a property
> of the device, not the bus.
>
> (You'll notice that 3-12 allows the slave to send the byte data
> in various different lanes depending on which size acknowlegement
> it chooses to send. The documentation of what the master does
> is on page 53 and actually says you have to send it in two lanes
> for a non-aligned address, and certainly QEMU won't do anything
> corresponding to that. In any case I don't think it's relevant here.)

(goes and thinks about this for a bit)

Okay I can agree with that it's more an artifact of the implementation, 
rather than dependent upon the behaviour of the bus. I'll alter the 
comment for the next revision which I hope to post to the list over the 
next day or so.


ATB,

Mark.

On 10/02/14 08:20, Paolo Bonzini wrote:

> Il 09/02/2014 16:24, Mark Cave-Ayland ha scritto:
>>
>> Alright I can change those for the next version of the patch. Does that
>> mean the use of hex output is now a display option rather than a
>> separate property type?
>
> "info qtree" will always print both decimal and hex.

If I try that here then with the CG3 patch applied the prom_addr 
property is always just displayed in decimal...? Or have I missed 
something obvious here?

(You can see the same in vanilla QEMU by changing the same property in 
hw/display/tcx.c from DEFINE_PROP_HEX64 to DEFINE_PROP_UINT64 and then 
running "info qtree" from qemu-system-sparc)


ATB,

Mark.

On 14/02/14 14:54, Peter Crosthwaite wrote:

>> The short answer is "we don't know" because we don't have any documentation.
>
> Sigh.... This has happened quite a lot lately.
>
> If the kernel driver has macros, re-use them as much as possible. If
> you have a vague idea on whats, what, a few well invented names would
> help the device self-documentation.

Okay. I now have a revised version which borrows macro names from the 
Linux and BSD drivers which I think should be more readable. I'll post 
the revised version to the list shortly.

>>> Your hander switch statements stride in 4, are you only doing this for
>>> your one exception case of that one-byte big-endian access I commented
>>> earlier.
>>
>>
>> Yes, that is correct.
>>
>
> Should you trap misaligned accesses then?

Over the weekend I found out that the non-BT458 accesses (addr >= 0x10) 
are done as byte accesses and so byte accesses do need to be allowed to 
these registers. My interpretation of reading the SBus documentation is 
that on real hardware the bus converts accesses for you, and so I don't 
think a trap would be suitable here. Also I've not found an image (yet) 
that attempts bad accesses in this way across my OpenBIOS ISO test suite...


ATB,

Mark.

> > "info qtree" will always print both decimal and hex.
> 
> If I try that here then with the CG3 patch applied the prom_addr
> property is always just displayed in decimal...? Or have I missed
> something obvious here?

Grammar sucks. :)

It *will* always print both decimal and hex once HEX64 is gone.  It
doesn't yet; right now, the exact printed format depends on UINT64 vs.
HEX64 as you found out.  But it's still easier for you to use UINT64
and avoid future conflicts.

Paolo

On 2/17/2014 6:50 AM, Mark Cave-Ayland wrote:
> On 14/02/14 14:54, Peter Crosthwaite wrote:
> 
>>> The short answer is "we don't know" because we don't have any
>>> documentation.
>>
>> Sigh.... This has happened quite a lot lately.
>>
>> If the kernel driver has macros, re-use them as much as possible. If
>> you have a vague idea on whats, what, a few well invented names would
>> help the device self-documentation.
> 
> Okay. I now have a revised version which borrows macro names from the
> Linux and BSD drivers which I think should be more readable. I'll post
> the revised version to the list shortly.
> 
>>>> Your hander switch statements stride in 4, are you only doing this for
>>>> your one exception case of that one-byte big-endian access I commented
>>>> earlier.
>>>
>>>
>>> Yes, that is correct.
>>>
>>
>> Should you trap misaligned accesses then?
> 
> Over the weekend I found out that the non-BT458 accesses (addr >= 0x10)
> are done as byte accesses and so byte accesses do need to be allowed to
> these registers. My interpretation of reading the SBus documentation is
> that on real hardware the bus converts accesses for you, and so I don't
> think a trap would be suitable here. Also I've not found an image (yet)
> that attempts bad accesses in this way across my OpenBIOS ISO test suite...


When you create the memory region for the registers, try this:

    .endianness = DEVICE_BIG_ENDIAN,
    .impl = {
        .min_access_size = 1,
        .max_access_size = 1,
    },

and ignore the bottom 2 address bits for the DAC accesses.

QEMU should then break any 32-bit accesses down to the correct 8-bit
accesses.

Bob