On 30.06.2013, at 08:42, Andreas Färber wrote:
> Am 30.06.2013 03:26, schrieb Alexander Graf:
>> The macio code is basically undebuggable as it stands today, with no
>> debug prints anywhere whatsoever. DBDMA was better, but I needed a
>> few more to create reasonable logs that tell me where breakage is.
>>
>> Add a DPRINTF macro in the macio source file and add a bunch of debug
>> prints that are all disabled by default of course.
>>
>> Signed-off-by: Alexander Graf <ag...@suse.de>
>> ---
>> hw/ide/macio.c | 39 ++++++++++++++++++++++++++++++++++++++-
>> hw/misc/macio/mac_dbdma.c | 12 ++++++++++--
>> 2 files changed, 48 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index 82409dc..5cbc923 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -30,6 +30,17 @@
>>
>> #include <hw/ide/internal.h>
>>
>> +/* debug MACIO */
>> +// #define DEBUG_MACIO
>> +
>> +#ifdef DEBUG_MACIO
>> +#define MACIO_DPRINTF(fmt, ...) \
>> + do { printf("MACIO: %s: " fmt , __func__, ## __VA_ARGS__); } while (0)
>> +#else
>> +#define MACIO_DPRINTF(fmt, ...)
>> +#endif
>
> Please use the pattern you suggested yourself of having an if
> (DEBUG_MACIO_ENABLED) {...} inside the macro rather than a second
> MACIO_DPRINTF(), so that the newly added debug output doesn'T bitrot.
Very good point. Thanks a lot!
>
>> +
>> +
>> /***********************************************************/
>> /* MacIO based PowerPC IDE */
>>
>> @@ -48,6 +59,8 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int
>> ret)
>> goto done;
>> }
>>
>> + MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
>> +
>> if (s->io_buffer_size > 0) {
>> m->aiocb = NULL;
>> qemu_sglist_destroy(&s->sg);
>> @@ -59,15 +72,22 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int
>> ret)
>> s->io_buffer_index &= 0x7ff;
>> }
>>
>> - if (s->packet_transfer_size <= 0)
>> + /* end of transfer ? */
>> + if (s->packet_transfer_size <= 0) {
>> + MACIO_DPRINTF("end of transfer\n");
>> ide_atapi_cmd_ok(s);
>> + }
>>
>> + /* end of DMA ? */
>> if (io->len == 0) {
>> + MACIO_DPRINTF("end of DMA\n");
>> goto done;
>> }
>
> Both comments duplicate your debug output module question mark. :)
Yeah, I've just synced the comments between ATAPI and ATA here. But you're
right - I should just remove them altogether.
>
>>
>> /* launch next transfer */
>>
>> + MACIO_DPRINTF("io->len = %#x\n", io->len);
>> +
>> s->io_buffer_size = io->len;
>>
>> qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
>> @@ -76,12 +96,17 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int
>> ret)
>> io->addr += io->len;
>> io->len = 0;
>>
>> + MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n",
>> + (s->lba << 2) + (s->io_buffer_index >> 9),
>> + s->packet_transfer_size, s->dma_cmd);
>> +
>> m->aiocb = dma_bdrv_read(s->bs, &s->sg,
>> (int64_t)(s->lba << 2) + (s->io_buffer_index >>
>> 9),
>> pmac_ide_atapi_transfer_cb, io);
>> return;
>>
>> done:
>> + MACIO_DPRINTF("done DMA\n");
>> bdrv_acct_done(s->bs, &s->acct);
>> io->dma_end(opaque);
>> }
>> @@ -95,6 +120,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>> int64_t sector_num;
>>
>> if (ret < 0) {
>> + MACIO_DPRINTF("DMA error\n");
>> m->aiocb = NULL;
>> qemu_sglist_destroy(&s->sg);
>> ide_dma_error(s);
>> @@ -102,6 +128,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>> }
>>
>> sector_num = ide_get_sector(s);
>> + MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
>> if (s->io_buffer_size > 0) {
>> m->aiocb = NULL;
>> qemu_sglist_destroy(&s->sg);
>> @@ -113,12 +140,14 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>>
>> /* end of transfer ? */
>> if (s->nsector == 0) {
>> + MACIO_DPRINTF("end of transfer\n");
>> s->status = READY_STAT | SEEK_STAT;
>> ide_set_irq(s->bus);
>> }
>>
>> /* end of DMA ? */
>> if (io->len == 0) {
>> + MACIO_DPRINTF("end of DMA\n");
>> goto done;
>> }
>>
>> @@ -127,12 +156,18 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>> s->io_buffer_index = 0;
>> s->io_buffer_size = io->len;
>>
>> +
>
> Intentionally two white lines?
Nope, I'm sure that gets resolved somewhere later in the patch series. But I'll
fix it up here too ;).
>
>> + MACIO_DPRINTF("io->len = %#x\n", io->len);
>> +
>> qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
>> &address_space_memory);
>> qemu_sglist_add(&s->sg, io->addr, io->len);
>> io->addr += io->len;
>> io->len = 0;
>>
>> + MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n",
>> + sector_num, n, s->nsector, s->dma_cmd);
>> +
>> switch (s->dma_cmd) {
>> case IDE_DMA_READ:
>> m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
>> @@ -162,6 +197,8 @@ static void pmac_ide_transfer(DBDMA_io *io)
>> MACIOIDEState *m = io->opaque;
>> IDEState *s = idebus_active_if(&m->bus);
>>
>> + MACIO_DPRINTF("\n", __LINE__);
>
> The argument is unused.
which proves the point that DPRINTF code shouldn't be left to bitrot ;).
>
>> +
>> s->io_buffer_size = 0;
>> if (s->drive_kind == IDE_CD) {
>> bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>> index ab174f5..903604d 100644
>> --- a/hw/misc/macio/mac_dbdma.c
>> +++ b/hw/misc/macio/mac_dbdma.c
>> @@ -233,6 +233,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
>> return;
>> case INTR_ALWAYS: /* always interrupt */
>> qemu_irq_raise(ch->irq);
>> + DBDMA_DPRINTF("conditional_interrupt: raise\n");
>
> Use %s and __func__ in case we ever rename it? More instances below. For
> dbdma_end() you did.
Ah, out of scope of this hunk is another debug print that already hardcodes the
name. But I'll just change to __func__ ;).
>
>> return;
>> }
>>
>> @@ -245,12 +246,16 @@ static void conditional_interrupt(DBDMA_channel *ch)
>>
>> switch(intr) {
>> case INTR_IFSET: /* intr if condition bit is 1 */
>> - if (cond)
>> + if (cond) {
>> qemu_irq_raise(ch->irq);
>> + DBDMA_DPRINTF("conditional_interrupt: raise\n");
>> + }
>> return;
>> case INTR_IFCLR: /* intr if condition bit is 0 */
>> - if (!cond)
>> + if (!cond) {
>> qemu_irq_raise(ch->irq);
>> + DBDMA_DPRINTF("conditional_interrupt: raise\n");
>> + }
>> return;
>> }
>> }
>> @@ -368,6 +373,8 @@ static void dbdma_end(DBDMA_io *io)
>> DBDMA_channel *ch = io->channel;
>> dbdma_cmd *current = &ch->current;
>>
>> + DBDMA_DPRINTF("%s\n", __func__, __LINE__);
>
> Unused argument.
>
>> +
>> if (conditional_wait(ch))
>> goto wait;
>>
>> @@ -422,6 +429,7 @@ static void start_input(DBDMA_channel *ch, int key,
>> uint32_t addr,
>> * are not implemented in the mac-io chip
>> */
>>
>> + DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key);
>
> PRIx32 for addr
Oh? Is there any system out there which does not work with %x and uint32_t?
Alex
