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