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

[Peter Crosthwaite]

On Sun, Feb 9, 2014 at 11:35 PM, Mark Cave-Ayland
<address@hidden> 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.
>