Hi Geert,

thanks for the review - largely uncontroversial except for the volatile...

Am 14.03.2018 um 20:49 schrieb Geert Uytterhoeven:
>> +/* Blizzard 1230 DMA interface */
>> +
>> +struct blz1230_dma_registers {
>> +       volatile unsigned char dma_addr;        /* DMA address      [0x0000] 
>> */
> 
> volatile considered harmful.

Yes, I saw that. I also saw gcc miscompile the DMA set-up (in
particular, the case where three bytes of the transfer address are
stuffed consecutively into the same DMA address register).

> If you would use proper *{read,write}*() accessors instead of direct
> assignments,
> you can drop the volatile's here.

Meaning writeb(val, reg) instead of reg = val?

#define out_8(addr,b) (void)((*(__force volatile u8 *) (addr)) = (b))

nicely hides the 'volatile' but suggests I also need to pass it a
pointer, so

writeb((addr >> 24) & 0xff, &dregs->dma_addr)

would do the same as

dregs->dma_addr  = (addr >> 24) & 0xff; ??

I'll have to compare the assembly generated by the two versions before I
dare test that, but I'll give that a try. Liberal use of wmb() did fix
the miscompile but that just looked too ugly.

>> +#define ZORRO_ESP_PIO_LOOP(operands, reg1) \
>> +       { \
>> +       asm volatile ( \
>> +            "1:     moveb " operands "\n" \
>> +            "       subqw #1,%1       \n" \
>> +            "       jbne 1b           \n" \
>> +            : "+a" (addr), "+r" (reg1) \
>> +            : "a" (fifo)); \
>> +       }
> 
> Please pass "addr" and "fifo" as macro parameters, too, so it's easier for
> the reviewer to notice they are used.

Yes, I can do that (meaning Finn would need to make the same change to
keep our versions in sync).

>> +       /* Switch to the correct the DMA routine and clock frequency. */
>> +       switch (ent->id) {
>> +       case ZORRO_PROD_PHASE5_BLIZZARD_2060:
>> +               zorro_esp_ops.send_dma_cmd = zorro_esp_send_blz2060_dma_cmd;
> 
> Please use function pointers in struct zorro_driver_data, so you don't need
> a switch() here (except for Fastlane vs. B1230II).

At that point, the Blizzard 1230 II zorro_driver_data has been replaced
by the Fastlane one so the correct function pointer would be used. I
didn't realize that also nicely solves my problem here.

Thanks!

Cheers,

        Michael

Reply via email to