Hi Michael,
On Wed, Mar 14, 2018 at 9:23 AM, Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
> thanks for the review - largely uncontroversial except for the volatile...
The presence of volatile in drivers is always considered controversial ;-)
> 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)
Yes, you have to pass it an (__iomem) pointer.
> would do the same as
>
> dregs->dma_addr = (addr >> 24) & 0xff; ??
Right.
> 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.
Using the macros should have the same effect due to the embedded volatile.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
