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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 550ABC83022 for ; Mon, 30 Nov 2020 15:44:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E7FC821973 for ; Mon, 30 Nov 2020 15:44:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728220AbgK3Po1 (ORCPT ); Mon, 30 Nov 2020 10:44:27 -0500 Received: from mail-wm1-f68.google.com ([209.85.128.68]:52147 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727720AbgK3PoZ (ORCPT ); Mon, 30 Nov 2020 10:44:25 -0500 Received: by mail-wm1-f68.google.com with SMTP id v14so13295379wml.1 for ; Mon, 30 Nov 2020 07:44:09 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=wrXxUQ92cZjqbtsEEoadFbidu9p7SlbrPzlM2q7qXbQ=; b=WdPcKIFpz5NMmHybkz/SUPatAqDXnz3j2OizoXxqFAjQFhNAuQgKk1aJec+44v0P3k J+n+IAJ3zeSx4pD259mxRjWwyL2iteuEleNotqHgcOV75xskcYxDYhgNmZBN/kC6iC3n oF5jRbtVkmjF9vfzitZI8mLc3Ur9Xj5GsorkCcRSwd41Ec/TuxEloUVaP6XWmc/fUqS+ B7A0Bekw/U3mKXV/ewMWizC0r8EHQchI5Tl98t0jeMkfkJZTqdT5vyc9TiT8mWTocvBw Tk1e/wafeQfYOmZ1pVKJnrBb9KEWOk78IDHjl/pw86X4Rj5f5rsM2I3Npb8gLBUG1rdF 8ElA== X-Gm-Message-State: AOAM532XU3DiHQYUksNkAcedMbYMDl/TVmOID6LR8UWvWiHEL7p2KdWI DI031qnu4258HSvWKROIqAk= X-Google-Smtp-Source: ABdhPJwrrDJzMhj73fjDkT0ADgqZ22ISWQCdbgUESE4glj3y9mWsdL0QCouWmsg3hBrKTtQdmgQ/Kw== X-Received: by 2002:a1c:1d85:: with SMTP id d127mr7724937wmd.39.1606751023484; Mon, 30 Nov 2020 07:43:43 -0800 (PST) Received: from rocinante ([95.155.85.46]) by smtp.gmail.com with ESMTPSA id x5sm29038317wrm.96.2020.11.30.07.43.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Nov 2020 07:43:42 -0800 (PST) Date: Mon, 30 Nov 2020 16:43:41 +0100 From: Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= To: Bjorn Helgaas Cc: Bjorn Helgaas , Rob Herring , Jonathan Cameron , Jonathan Chocron , Shawn Lin , Heiko Stuebner , Zhou Wang , Lorenzo Pieralisi , Will Deacon , Robert Richter , Michal Simek , Toan Le , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Thomas Petazzoni , Nicolas Saenz Julienne , Florian Fainelli , Ray Jui , Scott Branden , Jonathan Derrick , David Laight , linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, linux-rockchip@lists.infradead.org, linux-rpi-kernel@lists.infradead.org, bcm-kernel-feedback-list@broadcom.com Subject: Re: [PATCH v5] PCI: Unify ECAM constants in native PCI Express drivers Message-ID: References: <20201127104626.3979165-1-kw@linux.com> <20201128183516.GA897329@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201128183516.GA897329@bjorn-Precision-5520> Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org [+CC David for visibility] Hi Bjorn, Thank you for the review! On 20-11-28 12:35:16, Bjorn Helgaas wrote: [...] > It's ironic that we don't use PCIE_ECAM_OFFSET in drivers/pci/ecam.c. > We could do something like this, which would also let us drop > .bus_shift completely in all the conforming implementations. It also > closes the hole that we didn't limit "where" to 4K for > pci_ecam_map_bus() users. > > if (per_bus_mapping) { > base = cfg->winp[busn]; > busn = 0; > } else { > base = cfg->win; > } > > if (cfg->ops->bus_shift) { > u32 bus_offset = (busn & 0xff) << cfg->ops->bus_shift; > u32 devfn_offset = (devfn & 0xff) << (cfg->ops->bus_shift - 8); > > where &= 0xfff; > > return base + (bus_offset | devfn_offset | where); > } > > return base + PCIE_ECAM_OFFSET(busn, devfn, where); [...] Thank you for suggesting this! I sent v6 recently that includes this. > > static void __iomem *ppc4xx_pciex_get_config_base(struct ppc4xx_pciex_port *port, > > struct pci_bus *bus, > > - unsigned int devfn) > > + unsigned int devfn, > > + int offset) > > The interface change (to add "offset") could be a preparatory patch by > itself. > > But I'm actually not sure it's worth even touching this file. This is > the only place outside drivers/pci that includes linux/pci-ecam.h. I > think I might rather put PCIE_ECAM_OFFSET() and related things in > drivers/pci/pci.h and keep it all inside drivers/pci. Makes sense to drop it. We can always introduce chances on PPC 4xx platform in the future if we ever want it to leverage all the new macros and constants. These changes are not included in v6. > > static const struct pci_ecam_ops pci_thunder_pem_ops = { > > - .bus_shift = 24, > > + .bus_shift = THUNDER_PCIE_ECAM_BUS_SHIFT, > > .init = thunder_pem_platform_init, > > .pci_ops = { > > .map_bus = pci_ecam_map_bus, > > This could be split to its own patch, no big deal either way. Done. v6 is now a series that includes this as a separate patch. > > const struct pci_ecam_ops xgene_v2_pcie_ecam_ops = { > > - .bus_shift = 16, > > .init = xgene_v2_pcie_ecam_init, > > .pci_ops = { > > .map_bus = xgene_pcie_map_bus, > > Thanks for mentioning this change in the cover letter. It could also > be split off to a preparatory patch, since it's not related to > PCIE_ECAM_OFFSET(), which is the main point of this patch. Done. > > static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie, > > unsigned int busno, > > - unsigned int slot, > > - unsigned int fn, > > + unsigned int devfn, > > This interface change *could* be a separate preparatory patch, too, > but I'm starting to feel even more OCD than usual :) Done. It's a separate patch in v6, although I kept it together with the change to introduce the PCIE_ECAM_OFFSET() macro since I was retiring the use of PCI_SLOT() and PCI_FUNC() macros. > > @@ -94,7 +95,7 @@ struct vmd_dev { > > struct pci_dev *dev; > > > > spinlock_t cfg_lock; > > - char __iomem *cfgbar; > > + void __iomem *cfgbar; > > This type change might be worth pushing to a separate patch since the > casting issues are not completely trivial. Done. The patch included in the series as part of v6 already got a review from David Laight (thank you!) who suggests that this might not be a good idea to do, and keeping existing type would be better. Krzysztof 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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C557AC63777 for ; Mon, 30 Nov 2020 15:46:31 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CF79420706 for ; Mon, 30 Nov 2020 15:46:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CF79420706 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 4Cl8jJ4HvFzDqTN for ; Tue, 1 Dec 2020 02:46:28 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gmail.com (client-ip=209.85.128.67; helo=mail-wm1-f67.google.com; envelope-from=kswilczynski@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=linux.com Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4Cl8fC1PNzzDqty for ; Tue, 1 Dec 2020 02:43:46 +1100 (AEDT) Received: by mail-wm1-f67.google.com with SMTP id c198so18886970wmd.0 for ; Mon, 30 Nov 2020 07:43:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=wrXxUQ92cZjqbtsEEoadFbidu9p7SlbrPzlM2q7qXbQ=; b=L3UIAn3knrTby5OeNbOWeQa3xtOfz1A7iw+VcNR2z6hkrFo//2Mezthnmg104fxeB4 UdB3ui4BQEmmEBKmetsJqd9CJC9CucpsrFvenXgLqqDk34h/6UmVNiAZJvLvLiBju5kH GR/fCqY3TAp94qFAWVf8aT6mSQ1rziM2LX5jgrlYMh9pGHgLvssT7adWIqwye+2cmNU6 p0AmDsFiKQIAWxK3gzgfMbJClDGHjpKlyLMQ+Gq7osoT2HmtSwRYweLwyRglFeBas3sa +mjkLE37bkvkj8amOBZMIzGeuo1yPKXzRwsGEdNfzeUQUjLNSnc9+dhQEkHGF6Kb49Iq vs+Q== X-Gm-Message-State: AOAM530Mht/vKI2XcIM/MD0FniQ2/PVxuZ5+DTU4Y1Ko5sV0XXlEbSo+ CO/Ui8snKERYRCm3O8iybt8= X-Google-Smtp-Source: ABdhPJwrrDJzMhj73fjDkT0ADgqZ22ISWQCdbgUESE4glj3y9mWsdL0QCouWmsg3hBrKTtQdmgQ/Kw== X-Received: by 2002:a1c:1d85:: with SMTP id d127mr7724937wmd.39.1606751023484; Mon, 30 Nov 2020 07:43:43 -0800 (PST) Received: from rocinante ([95.155.85.46]) by smtp.gmail.com with ESMTPSA id x5sm29038317wrm.96.2020.11.30.07.43.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Nov 2020 07:43:42 -0800 (PST) Date: Mon, 30 Nov 2020 16:43:41 +0100 From: Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= To: Bjorn Helgaas Subject: Re: [PATCH v5] PCI: Unify ECAM constants in native PCI Express drivers Message-ID: References: <20201127104626.3979165-1-kw@linux.com> <20201128183516.GA897329@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201128183516.GA897329@bjorn-Precision-5520> X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Heiko Stuebner , Shawn Lin , Paul Mackerras , Thomas Petazzoni , Jonathan Chocron , Toan Le , Will Deacon , Rob Herring , Lorenzo Pieralisi , Michal Simek , linux-rockchip@lists.infradead.org, David Laight , bcm-kernel-feedback-list@broadcom.com, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, Ray Jui , Florian Fainelli , linux-rpi-kernel@lists.infradead.org, Jonathan Cameron , Bjorn Helgaas , Jonathan Derrick , Scott Branden , Zhou Wang , Robert Richter , linuxppc-dev@lists.ozlabs.org, Nicolas Saenz Julienne Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" [+CC David for visibility] Hi Bjorn, Thank you for the review! On 20-11-28 12:35:16, Bjorn Helgaas wrote: [...] > It's ironic that we don't use PCIE_ECAM_OFFSET in drivers/pci/ecam.c. > We could do something like this, which would also let us drop > .bus_shift completely in all the conforming implementations. It also > closes the hole that we didn't limit "where" to 4K for > pci_ecam_map_bus() users. > > if (per_bus_mapping) { > base = cfg->winp[busn]; > busn = 0; > } else { > base = cfg->win; > } > > if (cfg->ops->bus_shift) { > u32 bus_offset = (busn & 0xff) << cfg->ops->bus_shift; > u32 devfn_offset = (devfn & 0xff) << (cfg->ops->bus_shift - 8); > > where &= 0xfff; > > return base + (bus_offset | devfn_offset | where); > } > > return base + PCIE_ECAM_OFFSET(busn, devfn, where); [...] Thank you for suggesting this! I sent v6 recently that includes this. > > static void __iomem *ppc4xx_pciex_get_config_base(struct ppc4xx_pciex_port *port, > > struct pci_bus *bus, > > - unsigned int devfn) > > + unsigned int devfn, > > + int offset) > > The interface change (to add "offset") could be a preparatory patch by > itself. > > But I'm actually not sure it's worth even touching this file. This is > the only place outside drivers/pci that includes linux/pci-ecam.h. I > think I might rather put PCIE_ECAM_OFFSET() and related things in > drivers/pci/pci.h and keep it all inside drivers/pci. Makes sense to drop it. We can always introduce chances on PPC 4xx platform in the future if we ever want it to leverage all the new macros and constants. These changes are not included in v6. > > static const struct pci_ecam_ops pci_thunder_pem_ops = { > > - .bus_shift = 24, > > + .bus_shift = THUNDER_PCIE_ECAM_BUS_SHIFT, > > .init = thunder_pem_platform_init, > > .pci_ops = { > > .map_bus = pci_ecam_map_bus, > > This could be split to its own patch, no big deal either way. Done. v6 is now a series that includes this as a separate patch. > > const struct pci_ecam_ops xgene_v2_pcie_ecam_ops = { > > - .bus_shift = 16, > > .init = xgene_v2_pcie_ecam_init, > > .pci_ops = { > > .map_bus = xgene_pcie_map_bus, > > Thanks for mentioning this change in the cover letter. It could also > be split off to a preparatory patch, since it's not related to > PCIE_ECAM_OFFSET(), which is the main point of this patch. Done. > > static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie, > > unsigned int busno, > > - unsigned int slot, > > - unsigned int fn, > > + unsigned int devfn, > > This interface change *could* be a separate preparatory patch, too, > but I'm starting to feel even more OCD than usual :) Done. It's a separate patch in v6, although I kept it together with the change to introduce the PCIE_ECAM_OFFSET() macro since I was retiring the use of PCI_SLOT() and PCI_FUNC() macros. > > @@ -94,7 +95,7 @@ struct vmd_dev { > > struct pci_dev *dev; > > > > spinlock_t cfg_lock; > > - char __iomem *cfgbar; > > + void __iomem *cfgbar; > > This type change might be worth pushing to a separate patch since the > casting issues are not completely trivial. Done. The patch included in the series as part of v6 already got a review from David Laight (thank you!) who suggests that this might not be a good idea to do, and keeping existing type would be better. Krzysztof 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 X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C57B1C63777 for ; Mon, 30 Nov 2020 15:43:54 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3C5FE20706 for ; Mon, 30 Nov 2020 15:43:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="FgBK4RAl" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3C5FE20706 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=aNUXpnuamJtfMWEg98Koz+pFFogQGmoel+BVTn1lA6g=; b=FgBK4RAl4MBE5V+FWjsYvQHHy c5w5bbN6Yh13nDSxneju0WAJSF8AhushSAGs+wkIVu1Z/QgPi5g5xvKeqRuxQ8RW4pZRHk2rJR5H7 adab/K5WDAR3IUE2UikvkaccZinZvQ0jPaNpI9xSLag0TQtDv14f7e5BKWZa6AW5xILWJiU9byQNc 4UAQpzR93F2RmulzE6PvmG1sDuq6BcxH59ohXKI0ADHeFD+dou06oyYyA49urH0ujupudvEft/a+L iBsBl7VBhXO7nAW+Sp/lL9XmNGZKNxzk1N2UPYBgI+7pzzKW6p+0rOKo5leDeYDosxbAXE0SVGwY5 fhx6Xr5Zg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kjlLC-0002hA-RB; Mon, 30 Nov 2020 15:43:50 +0000 Received: from mail-wm1-f66.google.com ([209.85.128.66]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kjlL6-0002f0-RI; Mon, 30 Nov 2020 15:43:45 +0000 Received: by mail-wm1-f66.google.com with SMTP id a6so5383093wmc.2; Mon, 30 Nov 2020 07:43:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=wrXxUQ92cZjqbtsEEoadFbidu9p7SlbrPzlM2q7qXbQ=; b=MdEn12v4i8qHAUBIaIXk97YVxzfHB5Bn2oup2eeSqJE++9irODB+Soujtw6IpX5Piu DQoiD6TKEpej9/LOBFSHYwdMoQTstOl2Zt8tljUWCuSvOEoX0bYCmUBRlGyp6l92HH6J B5MgSxWUbX2xEogXhRe7qEo6TjGukpxJSdgH9t/rt7OoAv8xUkCJv097kNjGsoFLxPhr aKgo+Ft0GIvLRocrCZSMRDNLaR42BRpw0O+SXU2Xj2CX56XqdoXV5axrCMcub8uf5x8A zHxEe3WF/GG7cKJ8TXsBAvwaBr5B4PxzokcXO155gSVT6BIA0MxeZGiOi1TX4PZmhqDa OtSw== X-Gm-Message-State: AOAM532TgedRchDa1h4OpMkW/36VTX65ATeOJLcBf/DcxApfbxGml3nt OCw1Rs3Inoxr+VIXE882/8U= X-Google-Smtp-Source: ABdhPJwrrDJzMhj73fjDkT0ADgqZ22ISWQCdbgUESE4glj3y9mWsdL0QCouWmsg3hBrKTtQdmgQ/Kw== X-Received: by 2002:a1c:1d85:: with SMTP id d127mr7724937wmd.39.1606751023484; Mon, 30 Nov 2020 07:43:43 -0800 (PST) Received: from rocinante ([95.155.85.46]) by smtp.gmail.com with ESMTPSA id x5sm29038317wrm.96.2020.11.30.07.43.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Nov 2020 07:43:42 -0800 (PST) Date: Mon, 30 Nov 2020 16:43:41 +0100 From: Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= To: Bjorn Helgaas Subject: Re: [PATCH v5] PCI: Unify ECAM constants in native PCI Express drivers Message-ID: References: <20201127104626.3979165-1-kw@linux.com> <20201128183516.GA897329@bjorn-Precision-5520> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201128183516.GA897329@bjorn-Precision-5520> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201130_104344_924206_5E12F94A X-CRM114-Status: GOOD ( 29.09 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Heiko Stuebner , Benjamin Herrenschmidt , Shawn Lin , Paul Mackerras , Thomas Petazzoni , Jonathan Chocron , Toan Le , Will Deacon , Rob Herring , Lorenzo Pieralisi , Michael Ellerman , Michal Simek , linux-rockchip@lists.infradead.org, David Laight , bcm-kernel-feedback-list@broadcom.com, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, Ray Jui , Florian Fainelli , linux-rpi-kernel@lists.infradead.org, Jonathan Cameron , Bjorn Helgaas , Jonathan Derrick , Scott Branden , Zhou Wang , Robert Richter , linuxppc-dev@lists.ozlabs.org, Nicolas Saenz Julienne Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org [+CC David for visibility] Hi Bjorn, Thank you for the review! On 20-11-28 12:35:16, Bjorn Helgaas wrote: [...] > It's ironic that we don't use PCIE_ECAM_OFFSET in drivers/pci/ecam.c. > We could do something like this, which would also let us drop > .bus_shift completely in all the conforming implementations. It also > closes the hole that we didn't limit "where" to 4K for > pci_ecam_map_bus() users. > > if (per_bus_mapping) { > base = cfg->winp[busn]; > busn = 0; > } else { > base = cfg->win; > } > > if (cfg->ops->bus_shift) { > u32 bus_offset = (busn & 0xff) << cfg->ops->bus_shift; > u32 devfn_offset = (devfn & 0xff) << (cfg->ops->bus_shift - 8); > > where &= 0xfff; > > return base + (bus_offset | devfn_offset | where); > } > > return base + PCIE_ECAM_OFFSET(busn, devfn, where); [...] Thank you for suggesting this! I sent v6 recently that includes this. > > static void __iomem *ppc4xx_pciex_get_config_base(struct ppc4xx_pciex_port *port, > > struct pci_bus *bus, > > - unsigned int devfn) > > + unsigned int devfn, > > + int offset) > > The interface change (to add "offset") could be a preparatory patch by > itself. > > But I'm actually not sure it's worth even touching this file. This is > the only place outside drivers/pci that includes linux/pci-ecam.h. I > think I might rather put PCIE_ECAM_OFFSET() and related things in > drivers/pci/pci.h and keep it all inside drivers/pci. Makes sense to drop it. We can always introduce chances on PPC 4xx platform in the future if we ever want it to leverage all the new macros and constants. These changes are not included in v6. > > static const struct pci_ecam_ops pci_thunder_pem_ops = { > > - .bus_shift = 24, > > + .bus_shift = THUNDER_PCIE_ECAM_BUS_SHIFT, > > .init = thunder_pem_platform_init, > > .pci_ops = { > > .map_bus = pci_ecam_map_bus, > > This could be split to its own patch, no big deal either way. Done. v6 is now a series that includes this as a separate patch. > > const struct pci_ecam_ops xgene_v2_pcie_ecam_ops = { > > - .bus_shift = 16, > > .init = xgene_v2_pcie_ecam_init, > > .pci_ops = { > > .map_bus = xgene_pcie_map_bus, > > Thanks for mentioning this change in the cover letter. It could also > be split off to a preparatory patch, since it's not related to > PCIE_ECAM_OFFSET(), which is the main point of this patch. Done. > > static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie, > > unsigned int busno, > > - unsigned int slot, > > - unsigned int fn, > > + unsigned int devfn, > > This interface change *could* be a separate preparatory patch, too, > but I'm starting to feel even more OCD than usual :) Done. It's a separate patch in v6, although I kept it together with the change to introduce the PCIE_ECAM_OFFSET() macro since I was retiring the use of PCI_SLOT() and PCI_FUNC() macros. > > @@ -94,7 +95,7 @@ struct vmd_dev { > > struct pci_dev *dev; > > > > spinlock_t cfg_lock; > > - char __iomem *cfgbar; > > + void __iomem *cfgbar; > > This type change might be worth pushing to a separate patch since the > casting issues are not completely trivial. Done. The patch included in the series as part of v6 already got a review from David Laight (thank you!) who suggests that this might not be a good idea to do, and keeping existing type would be better. Krzysztof _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip 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 X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5CCEEC63777 for ; Mon, 30 Nov 2020 15:44:59 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EA53E20725 for ; Mon, 30 Nov 2020 15:44:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="PCF5od4Z" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EA53E20725 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=vKHY3Wp9XJpr/u3e35yIHbHrBWYKb6y1omOA0VOXhOQ=; b=PCF5od4ZAZaXUDOafNGyjE/QE oeJHR4In6bnp6IOdfkcKV62wYqUvejEjEQbbxXcxxxchlIRtUQBZm8tZ9JJCtjYJ3+hsyX5Df+erX pYmq7SNn3m8qSxwO+nXIn2zlkCAndW78K4jU0sOURkD+XLM1UEtjBwZdvjuhGDVBk8BHpN9eoWsxw A+e2HeuW8kwo3KoR8BXdBF6X6wpUrWrsnu0EloTrg4w4gD3c0AuXkdS51aJuUG4h9GpX5fE3d5pDJ Bs7TmFsQSPvpr3OM+r6BD/iM2/ft1u/+LmUQb1UmbA4UIgtvxsJHz9HFeCjSetcN4BNk4PO6VKEDs /zX2S777g==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kjlLA-0002gS-5t; Mon, 30 Nov 2020 15:43:48 +0000 Received: from mail-wm1-f66.google.com ([209.85.128.66]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kjlL6-0002f0-RI; Mon, 30 Nov 2020 15:43:45 +0000 Received: by mail-wm1-f66.google.com with SMTP id a6so5383093wmc.2; Mon, 30 Nov 2020 07:43:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=wrXxUQ92cZjqbtsEEoadFbidu9p7SlbrPzlM2q7qXbQ=; b=MdEn12v4i8qHAUBIaIXk97YVxzfHB5Bn2oup2eeSqJE++9irODB+Soujtw6IpX5Piu DQoiD6TKEpej9/LOBFSHYwdMoQTstOl2Zt8tljUWCuSvOEoX0bYCmUBRlGyp6l92HH6J B5MgSxWUbX2xEogXhRe7qEo6TjGukpxJSdgH9t/rt7OoAv8xUkCJv097kNjGsoFLxPhr aKgo+Ft0GIvLRocrCZSMRDNLaR42BRpw0O+SXU2Xj2CX56XqdoXV5axrCMcub8uf5x8A zHxEe3WF/GG7cKJ8TXsBAvwaBr5B4PxzokcXO155gSVT6BIA0MxeZGiOi1TX4PZmhqDa OtSw== X-Gm-Message-State: AOAM532TgedRchDa1h4OpMkW/36VTX65ATeOJLcBf/DcxApfbxGml3nt OCw1Rs3Inoxr+VIXE882/8U= X-Google-Smtp-Source: ABdhPJwrrDJzMhj73fjDkT0ADgqZ22ISWQCdbgUESE4glj3y9mWsdL0QCouWmsg3hBrKTtQdmgQ/Kw== X-Received: by 2002:a1c:1d85:: with SMTP id d127mr7724937wmd.39.1606751023484; Mon, 30 Nov 2020 07:43:43 -0800 (PST) Received: from rocinante ([95.155.85.46]) by smtp.gmail.com with ESMTPSA id x5sm29038317wrm.96.2020.11.30.07.43.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Nov 2020 07:43:42 -0800 (PST) Date: Mon, 30 Nov 2020 16:43:41 +0100 From: Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= To: Bjorn Helgaas Subject: Re: [PATCH v5] PCI: Unify ECAM constants in native PCI Express drivers Message-ID: References: <20201127104626.3979165-1-kw@linux.com> <20201128183516.GA897329@bjorn-Precision-5520> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201128183516.GA897329@bjorn-Precision-5520> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201130_104344_924206_5E12F94A X-CRM114-Status: GOOD ( 29.09 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Heiko Stuebner , Benjamin Herrenschmidt , Shawn Lin , Paul Mackerras , Thomas Petazzoni , Jonathan Chocron , Toan Le , Will Deacon , Rob Herring , Lorenzo Pieralisi , Michael Ellerman , Michal Simek , linux-rockchip@lists.infradead.org, David Laight , bcm-kernel-feedback-list@broadcom.com, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, Ray Jui , Florian Fainelli , linux-rpi-kernel@lists.infradead.org, Jonathan Cameron , Bjorn Helgaas , Jonathan Derrick , Scott Branden , Zhou Wang , Robert Richter , linuxppc-dev@lists.ozlabs.org, Nicolas Saenz Julienne Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org [+CC David for visibility] Hi Bjorn, Thank you for the review! On 20-11-28 12:35:16, Bjorn Helgaas wrote: [...] > It's ironic that we don't use PCIE_ECAM_OFFSET in drivers/pci/ecam.c. > We could do something like this, which would also let us drop > .bus_shift completely in all the conforming implementations. It also > closes the hole that we didn't limit "where" to 4K for > pci_ecam_map_bus() users. > > if (per_bus_mapping) { > base = cfg->winp[busn]; > busn = 0; > } else { > base = cfg->win; > } > > if (cfg->ops->bus_shift) { > u32 bus_offset = (busn & 0xff) << cfg->ops->bus_shift; > u32 devfn_offset = (devfn & 0xff) << (cfg->ops->bus_shift - 8); > > where &= 0xfff; > > return base + (bus_offset | devfn_offset | where); > } > > return base + PCIE_ECAM_OFFSET(busn, devfn, where); [...] Thank you for suggesting this! I sent v6 recently that includes this. > > static void __iomem *ppc4xx_pciex_get_config_base(struct ppc4xx_pciex_port *port, > > struct pci_bus *bus, > > - unsigned int devfn) > > + unsigned int devfn, > > + int offset) > > The interface change (to add "offset") could be a preparatory patch by > itself. > > But I'm actually not sure it's worth even touching this file. This is > the only place outside drivers/pci that includes linux/pci-ecam.h. I > think I might rather put PCIE_ECAM_OFFSET() and related things in > drivers/pci/pci.h and keep it all inside drivers/pci. Makes sense to drop it. We can always introduce chances on PPC 4xx platform in the future if we ever want it to leverage all the new macros and constants. These changes are not included in v6. > > static const struct pci_ecam_ops pci_thunder_pem_ops = { > > - .bus_shift = 24, > > + .bus_shift = THUNDER_PCIE_ECAM_BUS_SHIFT, > > .init = thunder_pem_platform_init, > > .pci_ops = { > > .map_bus = pci_ecam_map_bus, > > This could be split to its own patch, no big deal either way. Done. v6 is now a series that includes this as a separate patch. > > const struct pci_ecam_ops xgene_v2_pcie_ecam_ops = { > > - .bus_shift = 16, > > .init = xgene_v2_pcie_ecam_init, > > .pci_ops = { > > .map_bus = xgene_pcie_map_bus, > > Thanks for mentioning this change in the cover letter. It could also > be split off to a preparatory patch, since it's not related to > PCIE_ECAM_OFFSET(), which is the main point of this patch. Done. > > static void __iomem *iproc_pcie_map_ep_cfg_reg(struct iproc_pcie *pcie, > > unsigned int busno, > > - unsigned int slot, > > - unsigned int fn, > > + unsigned int devfn, > > This interface change *could* be a separate preparatory patch, too, > but I'm starting to feel even more OCD than usual :) Done. It's a separate patch in v6, although I kept it together with the change to introduce the PCIE_ECAM_OFFSET() macro since I was retiring the use of PCI_SLOT() and PCI_FUNC() macros. > > @@ -94,7 +95,7 @@ struct vmd_dev { > > struct pci_dev *dev; > > > > spinlock_t cfg_lock; > > - char __iomem *cfgbar; > > + void __iomem *cfgbar; > > This type change might be worth pushing to a separate patch since the > casting issues are not completely trivial. Done. The patch included in the series as part of v6 already got a review from David Laight (thank you!) who suggests that this might not be a good idea to do, and keeping existing type would be better. Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel