From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751922AbbKJXH3 (ORCPT ); Tue, 10 Nov 2015 18:07:29 -0500 Received: from unicorn.mansr.com ([81.2.72.234]:43345 "EHLO unicorn.mansr.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751123AbbKJXH2 (ORCPT ); Tue, 10 Nov 2015 18:07:28 -0500 From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= To: Andy Shevchenko Cc: "linux-kernel\@vger.kernel.org" , netdev , slash.tmp@free.fr Subject: Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller References: <1447172063-27234-1-git-send-email-mans@mansr.com> Date: Tue, 10 Nov 2015 23:07:24 +0000 In-Reply-To: (Andy Shevchenko's message of "Wed, 11 Nov 2015 00:40:53 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Andy Shevchenko writes: > On Wed, Nov 11, 2015 at 12:34 AM, Måns Rullgård wrote: >> Andy Shevchenko 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