Re: [Qemu-devel] [PATCHv2 1/2] sun4m: Add Sun CG3 framebuffer and corresponding OpenBIOS FCode ROM

[Mark Cave-Ayland]

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.