Messages in this thread | | | From | Måns Rullgård <> | Subject | Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller | Date | Tue, 10 Nov 2015 23:07:24 +0000 |
| |
Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> On Wed, Nov 11, 2015 at 12:34 AM, Måns Rullgård <mans@mansr.com> wrote: >> Andy Shevchenko <andy.shevchenko@gmail.com> writes: >> >>>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg, >>>> + u32 mask, u32 val) >>>> +{ >>>> + u32 old = nb8800_readb(priv, reg); >>>> + u32 new = (old & ~mask) | val; >>> >>> Shoudn't be "… | (val & mask);" ? >> >> No, it's meant to replace the bits in mask with the corresponding bits >> from val. > > But you unconditionally use entire val value which might bring bits > outside of mask.
Very well, I'll apply the mask to both then.
>>>> + nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc); >>>> + wmb(); /* ensure desc addr is written before starting DMA */ >>> >>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ? >> >> Possibly. > > Standalone wmb() doesn't make sense.
It does if you need to enforce ordering between normal and I/O memory. In fact, since the descriptor is filled in using normal memory accesses, my understanding is that mmiowb() would be insufficient here. The comment could be improved, however.
-- Måns Rullgård mans@mansr.com
| |