From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id ACF65C4332F for ; Tue, 28 Dec 2021 13:34:30 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 0688E83768; Tue, 28 Dec 2021 14:34:28 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="JpSZ8oK/"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id BDBFB83768; Tue, 28 Dec 2021 14:34:25 +0100 (CET) Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 0199D83740 for ; Tue, 28 Dec 2021 14:34:21 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=pali@kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 6DE5F61195; Tue, 28 Dec 2021 13:34:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 832EBC36AE7; Tue, 28 Dec 2021 13:34:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1640698459; bh=a8TJkKjX0HG8CQ40kv8eXCYZnO/Frk3HPlBXt8u8Vh8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=JpSZ8oK/WetNlRgqKDEpIIB8e9UcShrkpXv/bjXSZoLEjoFet9MdBzXGxgH0Tj8Jq 9x2UYClJbrKLlt+j1LwA23vy1i2rfjORjFnnOiiOKOumcf3haOfb0jNu5Qod8f9qLo QvMKWMsCZibLgsI1D1hSKKOvPMkJk+JsaWCHS7iukAeL5z91KI6zSOBtZX50t4yY8u IRspWGT8hkupU8NqrXRPcpEpBg1nYcxzcWgFD90HlvIPGSE9+1MK6qeIodjKVPU1Zw hiTZgri6WcqPeOZd8H6B3OQs8RNK8iOG1/SakyhwWN5HA4A8hdvPy7gVbX8ajBPdJd yP73NCn6h1p6w== Received: by pali.im (Postfix) id ACAA1856; Tue, 28 Dec 2021 14:34:16 +0100 (CET) Date: Tue, 28 Dec 2021 14:34:16 +0100 From: Pali =?utf-8?B?Um9ow6Fy?= To: Simon Glass Cc: Stefan Roese , Bin Meng , Daniel Schwierzeck , Heiko Schocher , Marek Vasut , Ryder Lee , Weijie Gao , Chunfeng Yun , GSS_MTK_Uboot_upstream , Huan Wang , Angelo Dureghello , U-Boot Mailing List Subject: Re: [PATCH u-boot-next 01/12] pci: Add standard PCI Config Address macros Message-ID: <20211228133416.6hktblnz7eifcnhy@pali> References: <20211126104252.5443-1-pali@kernel.org> <20211126104252.5443-2-pali@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20180716 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.38 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On Tuesday 28 December 2021 01:32:02 Simon Glass wrote: > Hi Paul, > > On Fri, 26 Nov 2021 at 03:43, Pali Rohár wrote: > > > > Lot of PCI and PCIe controllers are using standard Config Address for PCI > > Configuration Mechanism #1 or its extended version. > > > > So add PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() macros into U-Boot's > > pci.h header file which can be suitable for most PCI and PCIe controller > > drivers. Drivers do not have to invent their own macros and can use these > > new U-Boot macros. > > > > Signed-off-by: Pali Rohár > > --- > > include/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 45 insertions(+) > > > > Reviewed-by: Simon Glass > > Thoughts below > > > diff --git a/include/pci.h b/include/pci.h > > index 6c1094d72998..0ea41a7e1ba2 100644 > > --- a/include/pci.h > > +++ b/include/pci.h > > @@ -522,6 +522,51 @@ > > > > #include > > > > +/* > > + * Config Address for PCI Configuration Mechanism #1 > > + * > > + * See PCI Local Bus Specification, Revision 3.0, > > + * Section 3.2.2.3.2, Figure 3-2, p. 50. > > + */ > > + > > +#define PCI_CONF1_BUS_SHIFT 16 /* Bus number */ > > +#define PCI_CONF1_DEV_SHIFT 11 /* Device number */ > > +#define PCI_CONF1_FUNC_SHIFT 8 /* Function number */ > > + > > +#define PCI_CONF1_BUS_MASK 0xff > > +#define PCI_CONF1_DEV_MASK 0x1f > > +#define PCI_CONF1_FUNC_MASK 0x7 > > +#define PCI_CONF1_REG_MASK 0xfc /* Limit aligned offset to a maximum of 256B */ > > + > > +#define PCI_CONF1_ENABLE BIT(31) > > +#define PCI_CONF1_BUS(x) (((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT) > > +#define PCI_CONF1_DEV(x) (((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT) > > +#define PCI_CONF1_FUNC(x) (((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT) > > +#define PCI_CONF1_REG(x) ((x) & PCI_CONF1_REG_MASK) > > + > > +#define PCI_CONF1_ADDRESS(bus, dev, func, reg) \ > > Just for brevity, how about _ADDR() instead of _ADDRESS() ? I do not know... I think both are OK. > > + (PCI_CONF1_ENABLE | \ > > + PCI_CONF1_BUS(bus) | \ > > + PCI_CONF1_DEV(dev) | \ > > + PCI_CONF1_FUNC(func) | \ > > + PCI_CONF1_REG(reg)) > > + > > +/* > > + * Extension of PCI Config Address for accessing extended PCIe registers > > + * > > + * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs > > + * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address > > + * are used for specifying additional 4 high bits of PCI Express register. > > + */ > > + > > +#define PCI_CONF1_EXT_REG_SHIFT 16 > > +#define PCI_CONF1_EXT_REG_MASK 0xf00 > > How about s/EXT_REG/EXT/ ? I'm not sure... Name "EXT" does not say what it is. This macro defines access to the extended part (higher bits) of register value. That is why I called it "EXT_REG" so reader would see that it belongs to plain "REG" macro (which defines lower bits of register value). > For shifts and masks we normally like to define the mask in terms of > the shift, so: > > > +#define PCI_CONF1_EXT_REG_MASK (0xf00 << PCI_CONF1_EXT_REG_SHIFT) > > > +#define PCI_CONF1_EXT_REG(x) (((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT) Well, I wanted these PCI_CONF1_* macros to be "compatible" with PCIE_ECAM_* macros which are already in next and which I reused from Linux kernel header files. And these existing macros are using the style which I used also for PCI_CONF1_* macros. > > +#define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \ > > + (PCI_CONF1_ADDRESS(bus, dev, func, reg) | \ > > + PCI_CONF1_EXT_REG(reg)) > > + > > /* > > * Enhanced Configuration Access Mechanism (ECAM) > > * > > -- > > 2.20.1 > > > > Regards, > Simon