From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934796AbcATQ07 (ORCPT ); Wed, 20 Jan 2016 11:26:59 -0500 Received: from mailuogwhop.emc.com ([168.159.213.141]:37578 "EHLO mailuogwhop.emc.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934015AbcATQ0z convert rfc822-to-8bit (ORCPT ); Wed, 20 Jan 2016 11:26:55 -0500 X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd04.lss.emc.com u0KGQoqT028602 X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd04.lss.emc.com u0KGQoqT028602 From: "Allen Hubbe" To: "'Yu, Xiangliang'" , , , Cc: "'SPG_Linux_Kernel'" References: <1453205805-851-1-git-send-email-Xiangliang.Yu@amd.com> <000301d152f8$2e7a7780$8b6f6680$@emc.com> In-Reply-To: Subject: RE: [PATCH V4 1/1] NTB: Add support for AMD PCI-Express Non-Transparent Bridge Date: Wed, 20 Jan 2016 11:26:42 -0500 Message-ID: <000101d1539f$5966cd70$0c346850$@emc.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT X-Mailer: Microsoft Outlook 14.0 Thread-Index: AQHRUomJ1ZmTXHz3cUmCrQUld5tVPp8DPQkAgADTgACAAHIn8A== Content-Language: en-us X-RSA-Classifications: public X-Sentrion-Hostname: mailuogwprd04.lss.emc.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > > There is some concept of "split bar" in this function, and I want to > be sure to > > understand it correctly. > > > > BAR0 - configuration? > > BAR1 - 32bit memory window? i.e. "split" bar? > > BAR2+3 - 64bit memory window? > > BAR4+5 - 64bit memory window? > > Yes Ok. > > Note that "split" in the intel driver refers to BAR4+5, which is > normally a 64bit > > memory window, split into independent 32bit windows BAR4 and BAR5 by > > bios configuration. Calling it "split" there makes sense. Here, > calling it "split" > > is confusing, but as long as the code is correct, I think it's ok. > > AMD NTB has similar design. If by similar design, you mean that BAR2+3 or BAR4+5 can be split, those configurations are not currently supported by this driver. If needed, support for those configurations can be added later, as a patch. Ok with the current implementation. > > I see a lot of readl/writel and ioread/iowrite in the same file, even > in the > > same function as there is here. Pick one variant of the functions, > preferably > > the ioread/iowrite variant, and be consistent in its usage throughout > the file. > > Ioread/iowrite is only for 64bit read/write, I don't think it has any > confusion. It's not just for 64bit: there are ioread8,16,32. In fact, ioread64 is the one that's not provided by io.h: not all hardware can do a true 64bit read or write. We don't need a true 64bit operation, so ioread64 is provided locally, for convenience within this driver. > Actually, Intel NTB driver has same behavior. Actually, Intel NTB driver does not use readl/writel. Would you please fix this? > > It's also interesting that if it hits the last branch, ndev->peer_sta > is assigned > > zero, but the function returns zero, not LNK_STA. If the function > were > > immediately called again, it would return LNK_STA. Can you please > explain > > the logic here? > > Force to read link status register again because the opposite side maybe > still > In resuming progress, the status variable can't show the right state, > have to > Read register directly. There's no indication in this function that it interacts with the timer. With your suggestion, I'll take a closer look at the interactions between this function and the timer.