[1/2] PCI: dwc: Add new accessors to enable/disable DBI CS2 while setting the BAR size
Message ID | 20231017-pcie-qcom-bar-v1-1-3e26de07bec0@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2908:b0:403:3b70:6f57 with SMTP id ib8csp3930604vqb; Mon, 16 Oct 2023 23:18:47 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE6bZbhoUjdaigClSx3HxNhr/E11Z3lL/tQb3z/2O1ZYaN6qFjsscAtfEy6VUvJvb32z2JR X-Received: by 2002:a05:6358:9f9e:b0:166:d9b7:ed8d with SMTP id fy30-20020a0563589f9e00b00166d9b7ed8dmr1177286rwb.2.1697523526653; Mon, 16 Oct 2023 23:18:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697523526; cv=none; d=google.com; s=arc-20160816; b=pa+9fubrfYmPGb9GHcG6wmEIWwCaBzR7F4TqW9H/d2hYygmwowv4sOtRJLVE7B1t0l qSiUuRXu8YZ+fnW1obD64vAm7kTsUN/Bs7dwe2zRftU+BJA8vAcneATZhkyDl9WG81+w bS7QB9KqIQ3ofUMRDBFh2qcLg1R0AJ4NYMtsre9ZF5BRyNC/PeaYiG2bao+9Rg8nVR34 QcjYy4kAVb1FrNPb18hr1ZBSTNKRUDHB5OikCThAKb28qV2JMqn1n4KH3tPo74TCKXZJ mn2475aUDzBiabBVXm4PdL1AnhFaf6zvOFsZB/CCKXM2VGX8CNiKgSJVPrhhBCwDPPM+ uXiw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:in-reply-to:references:message-id :content-transfer-encoding:mime-version:subject:date:from :dkim-signature; bh=2lhtaXzzZVdki7ialq2pIqDq/s9Xa0PvlTZMBvawHvU=; fh=AL7uOW7XpDa++9W8UyVEmNJlkLbwtkYB45DjIzYa5dA=; b=IBujVdTYz3AlOCFSrVeb51SKV96ZQIQsn9HLdzMrxKYbWL8M9dL0eGFTX614uR1TiI 5jKcSL5U1JxjJpXlK1KG9lk8URy4hnYOfyASZPO33OlJWfyfmv71NPprawH8p5zs1dwd jlvVuTiyPb6a74237yzLvp3qeyQVDLtjc+unWbcLHdv9e+skM5zoILa58U9mh8Qr5OB3 XV6MNPHF6Jt5FciOnpVvVg9uAQrSPgqQo2lKEVVZzOw+eIzpLcRYujfkbK6UkOp8HTwv Dr25lJq4KsS31QiyFERZz3tR0C740E5j9XTerglpQZCwp34CQ7hniju3euDCxBdIOqBX fRbA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=K4764VyM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from pete.vger.email (pete.vger.email. [23.128.96.36]) by mx.google.com with ESMTPS id y185-20020a638ac2000000b005acfc455384si1096743pgd.339.2023.10.16.23.18.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Oct 2023 23:18:46 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=K4764VyM; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 836A6802FA72; Mon, 16 Oct 2023 23:18:44 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234541AbjJQGS1 (ORCPT <rfc822;hjfbswb@gmail.com> + 19 others); Tue, 17 Oct 2023 02:18:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36136 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234391AbjJQGSZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 17 Oct 2023 02:18:25 -0400 Received: from mail-qk1-x735.google.com (mail-qk1-x735.google.com [IPv6:2607:f8b0:4864:20::735]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1DB6DB0 for <linux-kernel@vger.kernel.org>; Mon, 16 Oct 2023 23:18:24 -0700 (PDT) Received: by mail-qk1-x735.google.com with SMTP id af79cd13be357-7742be66bd3so382180885a.3 for <linux-kernel@vger.kernel.org>; Mon, 16 Oct 2023 23:18:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1697523503; x=1698128303; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=2lhtaXzzZVdki7ialq2pIqDq/s9Xa0PvlTZMBvawHvU=; b=K4764VyMiI+1yKiv0/tGgAuUPYkwm9vC6P0v/GvfqF2b+sgzkGszp7YPfzXVlThges CLzLyf6Kx0kGspEXzjltJmnpj8+LF62tnQFHw19amHzQBp7JgTZt9hagR5FwUY7PmzII ODDqb7WsLTan1tl4CzJmH4yGZkeZajC7vKopbN/pa2yRBcp4VzqkyVPVEDvp2vN2wc9n OsK1wcj3za3mIeMkI2CB/imkittcSj9esQiPlQIOoWVmaRTGLZwfbCWdKJblVaF2GEPA NzlJ7V9RO8BQ8NBVr20r9JcgbhAKrng1xxLozFhPLZnMe5BE1EnjVxaDHQxzdHNtSUhL 8g1g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697523503; x=1698128303; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=2lhtaXzzZVdki7ialq2pIqDq/s9Xa0PvlTZMBvawHvU=; b=UOoD3eJ97Olsqk4iOEdSSL1D3QXSxoHAdV9SeEGwh0XyYIkd/ioYFgv9L5N4FkaIH9 w4VSptztMW9NIeK3RSGfKHLVuvYoX88IUni36WlhL286zsIzU2XbN6Z1UiSvAvcIeXAd l/kW8X6Z5BILxILgZeFMetmP0bu9uvOD9s8n5mvVk34UH54JNYxtBVG0KEcqVqF7/DVH SQ9sVBASIZMKUxKUBAJLf0WjmjquokFW1CahXounh+yoUdsqoTF6tWH84MuglY83zTvP Kg9rbZ5hQLoDqIn8d8kSup4PNX6NFzLPul9H75fN2vXzOyaTW/WwvTBj585oD/1WLzFS fAnw== X-Gm-Message-State: AOJu0YypL2yKz++0zbpcAQbLV/92a+tFsiGkUXVP5Pc20NClkWf0mgCD RldNIeHIGVbgAMVEAwxe7OMs X-Received: by 2002:a05:620a:4050:b0:775:9036:60f3 with SMTP id i16-20020a05620a405000b00775903660f3mr1621244qko.16.1697523503221; Mon, 16 Oct 2023 23:18:23 -0700 (PDT) Received: from [127.0.1.1] ([117.207.31.199]) by smtp.gmail.com with ESMTPSA id f22-20020a05620a12f600b00765aa3ffa07sm390304qkl.98.2023.10.16.23.18.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Oct 2023 23:18:22 -0700 (PDT) From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Date: Tue, 17 Oct 2023 11:47:54 +0530 Subject: [PATCH 1/2] PCI: dwc: Add new accessors to enable/disable DBI CS2 while setting the BAR size MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20231017-pcie-qcom-bar-v1-1-3e26de07bec0@linaro.org> References: <20231017-pcie-qcom-bar-v1-0-3e26de07bec0@linaro.org> In-Reply-To: <20231017-pcie-qcom-bar-v1-0-3e26de07bec0@linaro.org> To: Jingoo Han <jingoohan1@gmail.com>, Gustavo Pimentel <gustavo.pimentel@synopsys.com>, Manivannan Sadhasivam <mani@kernel.org>, Lorenzo Pieralisi <lpieralisi@kernel.org>, =?utf-8?q?Krzysztof_Wilczy=C5=84?= =?utf-8?q?ski?= <kw@linux.com>, Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, Manivannan Sadhasivam <mani@kernel.org> X-Mailer: b4 0.12.3 X-Developer-Signature: v=1; a=openpgp-sha256; l=2745; i=manivannan.sadhasivam@linaro.org; h=from:subject:message-id; bh=OP4W5AEhgUSDaHDy2B4OZk3Grr6XP1yl9oNf0R8rWUQ=; b=owEBbQGS/pANAwAKAVWfEeb+kc71AcsmYgBlLicmILNyHAyc5XZnCRhkGM2ygXAYYTDPEUJfF otuvUxk3HuJATMEAAEKAB0WIQRnpUMqgUjL2KRYJ5dVnxHm/pHO9QUCZS4nJgAKCRBVnxHm/pHO 9VDZCACPrTqVHF2758C661bi9dz+Xl3g3KKSa/tO6cUWWRo87iggp2VrgX4Ph9hdg7fGjHoY5O/ 7g0tAocoLXawOXjvADKmqAljh0tuTaQO9ks9Ers8xrSfa26mmgWxojG1c9jPHS0ac3uaG+YJdm+ /Bd0UVcVlrp60NpTBwDklAbuSU/KeAXPwYElDVe0JwwQ8tcYy6/3fgE7c0mbcSugF6V1fDIZ6Ei eJQRYvXUdY4czYybme0NxNol/uz5ZxMZf63BQ7cbv9i2PK5UqrRCI7k+cxMriTDLDP5zRP7fA8z WH/0kI7KuwagZa8T+f5beHAqmtoE8kWClmpJnrZwX6DdJ9NW X-Developer-Key: i=manivannan.sadhasivam@linaro.org; a=openpgp; fpr=C668AEC3C3188E4C611465E7488550E901166008 X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Mon, 16 Oct 2023 23:18:44 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779982429324109983 X-GMAIL-MSGID: 1779982429324109983 |
Series |
PCI: dwc: Fix the BAR size programming
|
|
Commit Message
Manivannan Sadhasivam
Oct. 17, 2023, 6:17 a.m. UTC
From: Manivannan Sadhasivam <mani@kernel.org> As per the DWC databook v4.21a, section M.4.1, in order to write some read only and shadow registers through application DBI, the device driver should assert DBI Chip Select 2 (CS2) in addition to DBI Chip Select (CS). This is a requirement at least on the Qcom platforms while programming the BAR size, as the BAR mask registers are marked RO. So let's add two new accessors dw_pcie_dbi_cs2_{en/dis} to enable/disable CS2 access in a vendor specific way while programming the BAR size. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/pci/controller/dwc/pcie-designware-ep.c | 6 ++++++ drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++++++ 2 files changed, 19 insertions(+)
Comments
On Tue, Oct 17, 2023 at 11:47:54AM +0530, Manivannan Sadhasivam wrote: > From: Manivannan Sadhasivam <mani@kernel.org> > > As per the DWC databook v4.21a, section M.4.1, in order to write some read > only and shadow registers through application DBI, the device driver should > assert DBI Chip Select 2 (CS2) in addition to DBI Chip Select (CS). > > This is a requirement at least on the Qcom platforms while programming the > BAR size, as the BAR mask registers are marked RO. So let's add two new > accessors dw_pcie_dbi_cs2_{en/dis} to enable/disable CS2 access in a vendor > specific way while programming the BAR size. Emm, it's a known thing for all IP-core versions: dbi_cs2 must be asserted to access the shadow DW PCIe CSRs space for both RC and EP including the BARs mask and their enabling/disabling flag (there are many more shadow CSRs available on DW PCIe EPs, and a few in DW PCIe RCs). That's why the dw_pcie_ops->writel_dbi2 pointer has been defined in the first place (dbi2 suffix means dbi_cs2). You should use it to create the platform-specific dbi_cs2 write accessors like it's done in pci-keystone.c and pcie-bt1.c. For instance like this: static void qcom_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val) { struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci); int ret; writel(1, pcie_ep->elbi + ELBI_CS2_ENABLE); ret = dw_pcie_write(pci->dbi_base2 + reg, size, val); if (ret) dev_err(pci->dev, "write DBI address failed\n"); writel(0, pcie_ep->elbi + ELBI_CS2_ENABLE); } /* Common DWC controller ops */ static const struct dw_pcie_ops pci_ops = { .link_up = qcom_pcie_dw_link_up, .start_link = qcom_pcie_dw_start_link, .stop_link = qcom_pcie_dw_stop_link, .write_dbi2 = qcom_pcie_write_dbi2, }; For that reason there is absolutely no need in adding the new callbacks. -Serge(y) > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-designware-ep.c | 6 ++++++ > drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index d34a5e87ad18..1874fb3d8df4 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -269,11 +269,17 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > > dw_pcie_dbi_ro_wr_en(pci); > > + dw_pcie_dbi_cs2_en(pci); > dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1)); > + dw_pcie_dbi_cs2_dis(pci); > + > dw_pcie_writel_dbi(pci, reg, flags); > > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) { > + dw_pcie_dbi_cs2_en(pci); > dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, upper_32_bits(size - 1)); > + dw_pcie_dbi_cs2_dis(pci); > + > dw_pcie_writel_dbi(pci, reg + 4, 0); > } > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 55ff76e3d384..3cba27b5bbe5 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -379,6 +379,7 @@ struct dw_pcie_ops { > size_t size, u32 val); > void (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg, > size_t size, u32 val); > + void (*dbi_cs2_access)(struct dw_pcie *pcie, bool enable); > int (*link_up)(struct dw_pcie *pcie); > enum dw_pcie_ltssm (*get_ltssm)(struct dw_pcie *pcie); > int (*start_link)(struct dw_pcie *pcie); > @@ -508,6 +509,18 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) > dw_pcie_writel_dbi(pci, reg, val); > } > > +static inline void dw_pcie_dbi_cs2_en(struct dw_pcie *pci) > +{ > + if (pci->ops && pci->ops->dbi_cs2_access) > + pci->ops->dbi_cs2_access(pci, true); > +} > + > +static inline void dw_pcie_dbi_cs2_dis(struct dw_pcie *pci) > +{ > + if (pci->ops && pci->ops->dbi_cs2_access) > + pci->ops->dbi_cs2_access(pci, false); > +} > + > static inline int dw_pcie_start_link(struct dw_pcie *pci) > { > if (pci->ops && pci->ops->start_link) > > -- > 2.25.1 >
On Wed, Oct 18, 2023 at 05:13:41PM +0300, Serge Semin wrote: > On Tue, Oct 17, 2023 at 11:47:54AM +0530, Manivannan Sadhasivam wrote: > > From: Manivannan Sadhasivam <mani@kernel.org> > > > > As per the DWC databook v4.21a, section M.4.1, in order to write some read > > only and shadow registers through application DBI, the device driver should > > assert DBI Chip Select 2 (CS2) in addition to DBI Chip Select (CS). > > > > This is a requirement at least on the Qcom platforms while programming the > > BAR size, as the BAR mask registers are marked RO. So let's add two new > > accessors dw_pcie_dbi_cs2_{en/dis} to enable/disable CS2 access in a vendor > > specific way while programming the BAR size. > > Emm, it's a known thing for all IP-core versions: dbi_cs2 must be > asserted to access the shadow DW PCIe CSRs space for both RC and > EP including the BARs mask and their enabling/disabling flag (there > are many more shadow CSRs available on DW PCIe EPs, and a few in DW > PCIe RCs). That's why the dw_pcie_ops->writel_dbi2 pointer has been > defined in the first place (dbi2 suffix means dbi_cs2). You should use > it to create the platform-specific dbi_cs2 write accessors like it's > done in pci-keystone.c and pcie-bt1.c. For instance like this: > > static void qcom_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val) > { > struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci); > int ret; > > writel(1, pcie_ep->elbi + ELBI_CS2_ENABLE); > > ret = dw_pcie_write(pci->dbi_base2 + reg, size, val); > if (ret) > dev_err(pci->dev, "write DBI address failed\n"); > > writel(0, pcie_ep->elbi + ELBI_CS2_ENABLE); > } > Hmm, I was not aware that this write_dbi2() callback is supposed to enable the CS2 access internally. But this approach doesn't look good to me. We already have accessors for enabling write access to DBI RO registers: dw_pcie_dbi_ro_wr_en() dw_pcie_dbi_ro_wr_dis() And IMO DBI_CS2 access should also be done this way instead of hiding it inside the register write callback. - Mani > /* Common DWC controller ops */ > static const struct dw_pcie_ops pci_ops = { > .link_up = qcom_pcie_dw_link_up, > .start_link = qcom_pcie_dw_start_link, > .stop_link = qcom_pcie_dw_stop_link, > .write_dbi2 = qcom_pcie_write_dbi2, > }; > > For that reason there is absolutely no need in adding the new > callbacks. > > -Serge(y) > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-designware-ep.c | 6 ++++++ > > drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > index d34a5e87ad18..1874fb3d8df4 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > @@ -269,11 +269,17 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > > > > dw_pcie_dbi_ro_wr_en(pci); > > > > + dw_pcie_dbi_cs2_en(pci); > > dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1)); > > + dw_pcie_dbi_cs2_dis(pci); > > + > > dw_pcie_writel_dbi(pci, reg, flags); > > > > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) { > > + dw_pcie_dbi_cs2_en(pci); > > dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, upper_32_bits(size - 1)); > > + dw_pcie_dbi_cs2_dis(pci); > > + > > dw_pcie_writel_dbi(pci, reg + 4, 0); > > } > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > index 55ff76e3d384..3cba27b5bbe5 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > @@ -379,6 +379,7 @@ struct dw_pcie_ops { > > size_t size, u32 val); > > void (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg, > > size_t size, u32 val); > > + void (*dbi_cs2_access)(struct dw_pcie *pcie, bool enable); > > int (*link_up)(struct dw_pcie *pcie); > > enum dw_pcie_ltssm (*get_ltssm)(struct dw_pcie *pcie); > > int (*start_link)(struct dw_pcie *pcie); > > @@ -508,6 +509,18 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) > > dw_pcie_writel_dbi(pci, reg, val); > > } > > > > +static inline void dw_pcie_dbi_cs2_en(struct dw_pcie *pci) > > +{ > > + if (pci->ops && pci->ops->dbi_cs2_access) > > + pci->ops->dbi_cs2_access(pci, true); > > +} > > + > > +static inline void dw_pcie_dbi_cs2_dis(struct dw_pcie *pci) > > +{ > > + if (pci->ops && pci->ops->dbi_cs2_access) > > + pci->ops->dbi_cs2_access(pci, false); > > +} > > + > > static inline int dw_pcie_start_link(struct dw_pcie *pci) > > { > > if (pci->ops && pci->ops->start_link) > > > > -- > > 2.25.1 > >
On Thu, Oct 19, 2023 at 10:58:35AM +0530, Manivannan Sadhasivam wrote: > On Wed, Oct 18, 2023 at 05:13:41PM +0300, Serge Semin wrote: > > On Tue, Oct 17, 2023 at 11:47:54AM +0530, Manivannan Sadhasivam wrote: > > > From: Manivannan Sadhasivam <mani@kernel.org> > > > > > > As per the DWC databook v4.21a, section M.4.1, in order to write some read > > > only and shadow registers through application DBI, the device driver should > > > assert DBI Chip Select 2 (CS2) in addition to DBI Chip Select (CS). > > > > > > This is a requirement at least on the Qcom platforms while programming the > > > BAR size, as the BAR mask registers are marked RO. So let's add two new > > > accessors dw_pcie_dbi_cs2_{en/dis} to enable/disable CS2 access in a vendor > > > specific way while programming the BAR size. > > > > Emm, it's a known thing for all IP-core versions: dbi_cs2 must be > > asserted to access the shadow DW PCIe CSRs space for both RC and > > EP including the BARs mask and their enabling/disabling flag (there > > are many more shadow CSRs available on DW PCIe EPs, and a few in DW > > PCIe RCs). That's why the dw_pcie_ops->writel_dbi2 pointer has been > > defined in the first place (dbi2 suffix means dbi_cs2). You should use > > it to create the platform-specific dbi_cs2 write accessors like it's > > done in pci-keystone.c and pcie-bt1.c. For instance like this: > > > > static void qcom_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val) > > { > > struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci); > > int ret; > > > > writel(1, pcie_ep->elbi + ELBI_CS2_ENABLE); > > > > ret = dw_pcie_write(pci->dbi_base2 + reg, size, val); > > if (ret) > > dev_err(pci->dev, "write DBI address failed\n"); > > > > writel(0, pcie_ep->elbi + ELBI_CS2_ENABLE); > > } > > > > Hmm, I was not aware that this write_dbi2() callback is supposed to enable the > CS2 access internally. But this approach doesn't look good to me. > > We already have accessors for enabling write access to DBI RO registers: > > dw_pcie_dbi_ro_wr_en() > dw_pcie_dbi_ro_wr_dis() > > And IMO DBI_CS2 access should also be done this way instead of hiding it inside > the register write callback. No, for many-many reasons. First of all, DBI RO/RW switch and DBI/DBI2 are absolutely different things. Former one switches the CSRs access attributes in both DBI and DBI2 CSR spaces. The later one are two CSR spaces, which access to is platform-specific. They can't and shouldn't be compared. DBI2 space is a shadow DBI CSRs space, which registers aren't just the RW versions of the DBI space, but its CSRs normally have different semantics with respect to the normal DBI CSRs available on the same offsets. I.e. BAR0 contains MEM/IO, TYPE, PREF flags and base address, meanwhile DBI2-BAR0 - BAR enable/disable flag, BAR mask. From that perspective having the dw_pcie_ops.write_dbi2 callback utilized for the DBI2 space access would provide an interface looking similar to the just DBI space - dw_pcie_ops.{write_dbi,read_dbi}. Thus the unified access interface would provide much more readable code, where based on the method name you'll be able to immediately infer the space being accessed to. Secondly, DBI RO/RW switch is a straight-forward CSR flag clearing/setting DBI_RO_WR_EN. This mechanism is available on all DW PCIe IP-cores and works in the _same_ way on all of them: just the MISC_CONTROL_1_OFF.DBI_RO_WR_EN flag switching. It switches RO/RW access attributes on both DBI_CS and DBI_CS2. So it's a cross-platform thing independent from the vendor-specific IP-core settings. That's why having generic functions for the RO/RW switch was the best choice: it's cross-platform so no need in the platform-specific callbacks, it works for both DBI and DBI2 so instead of implementing two additional RW-accessors you can call the RW en/dis method around the DBI and DBI2 accessors whenever you need to switch the access attributes. On the contrary DBI_CS2 is the DW PCIe IP-core input signal which activation is platform-specific. Some platforms have it switchable via a system-controller, some - via an additional elbi CSRs space, some - provide an additional CSR space mapping DBI2 with no need in the direct DBI_CS2 flag toggle, some may have an intermix of these setups or may need some additional manipulation to access the DBI2 space. So your case of having the DBI_CS2 flag available via the elbi space and having the DBI/DBI2 space mapped within the 4K offset with respect to DBI is just a single possible option. Finally, it's all about simplicity, maintainability and KIS principle. Your approach would imply having additional platform-specific callbacks, meanwhile there is already available DBI2 space accessor which is more than enough to implement what you need. Even if you drop the later one (and convert all the already available LLDDs to supporting what you want), having two callbacks instead of one will still make things harder to read, because the kernel hacker would need to keep in mind the DBI/DBI2 access context. Additionally calling _three_ methods instead of a _single_ one will look much more complex. Moreover having on/off antagonists prune to errors since a developer may forget to disable the DBI2 flag, which on some platforms will change the DBI CSRs semantics. Such error will be just impossible to meet should the current interface is preserved unless the platform-specific DBI2 accessor is incorrectly implemented, which would be still specific to the particular platform, but not for the entire DW PCIe driver. The last but not least, as I already mentioned dw_pcie_ops.write_dbi2 and respective wrappers look as much like the normal DBI accessors dw_pcie_ops.{write_dbi,read_dbi} which greatly improves the code readability. So no, I failed to find any firm justification for the approach you suggest. -Serge(y) > > - Mani > > > /* Common DWC controller ops */ > > static const struct dw_pcie_ops pci_ops = { > > .link_up = qcom_pcie_dw_link_up, > > .start_link = qcom_pcie_dw_start_link, > > .stop_link = qcom_pcie_dw_stop_link, > > .write_dbi2 = qcom_pcie_write_dbi2, > > }; > > > > For that reason there is absolutely no need in adding the new > > callbacks. > > > > -Serge(y) > > > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/pci/controller/dwc/pcie-designware-ep.c | 6 ++++++ > > > drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++++++ > > > 2 files changed, 19 insertions(+) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > index d34a5e87ad18..1874fb3d8df4 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > @@ -269,11 +269,17 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > > > > > > dw_pcie_dbi_ro_wr_en(pci); > > > > > > + dw_pcie_dbi_cs2_en(pci); > > > dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1)); > > > + dw_pcie_dbi_cs2_dis(pci); > > > + > > > dw_pcie_writel_dbi(pci, reg, flags); > > > > > > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) { > > > + dw_pcie_dbi_cs2_en(pci); > > > dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, upper_32_bits(size - 1)); > > > + dw_pcie_dbi_cs2_dis(pci); > > > + > > > dw_pcie_writel_dbi(pci, reg + 4, 0); > > > } > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > > index 55ff76e3d384..3cba27b5bbe5 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > @@ -379,6 +379,7 @@ struct dw_pcie_ops { > > > size_t size, u32 val); > > > void (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg, > > > size_t size, u32 val); > > > + void (*dbi_cs2_access)(struct dw_pcie *pcie, bool enable); > > > int (*link_up)(struct dw_pcie *pcie); > > > enum dw_pcie_ltssm (*get_ltssm)(struct dw_pcie *pcie); > > > int (*start_link)(struct dw_pcie *pcie); > > > @@ -508,6 +509,18 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) > > > dw_pcie_writel_dbi(pci, reg, val); > > > } > > > > > > +static inline void dw_pcie_dbi_cs2_en(struct dw_pcie *pci) > > > +{ > > > + if (pci->ops && pci->ops->dbi_cs2_access) > > > + pci->ops->dbi_cs2_access(pci, true); > > > +} > > > + > > > +static inline void dw_pcie_dbi_cs2_dis(struct dw_pcie *pci) > > > +{ > > > + if (pci->ops && pci->ops->dbi_cs2_access) > > > + pci->ops->dbi_cs2_access(pci, false); > > > +} > > > + > > > static inline int dw_pcie_start_link(struct dw_pcie *pci) > > > { > > > if (pci->ops && pci->ops->start_link) > > > > > > -- > > > 2.25.1 > > > > > -- > மணிவண்ணன் சதாசிவம்
On Thu, Oct 19, 2023 at 05:37:39PM +0300, Serge Semin wrote: > On Thu, Oct 19, 2023 at 10:58:35AM +0530, Manivannan Sadhasivam wrote: > > On Wed, Oct 18, 2023 at 05:13:41PM +0300, Serge Semin wrote: > > > On Tue, Oct 17, 2023 at 11:47:54AM +0530, Manivannan Sadhasivam wrote: > > > > From: Manivannan Sadhasivam <mani@kernel.org> > > > > > > > > As per the DWC databook v4.21a, section M.4.1, in order to write some read > > > > only and shadow registers through application DBI, the device driver should > > > > assert DBI Chip Select 2 (CS2) in addition to DBI Chip Select (CS). > > > > > > > > This is a requirement at least on the Qcom platforms while programming the > > > > BAR size, as the BAR mask registers are marked RO. So let's add two new > > > > accessors dw_pcie_dbi_cs2_{en/dis} to enable/disable CS2 access in a vendor > > > > specific way while programming the BAR size. > > > > > > Emm, it's a known thing for all IP-core versions: dbi_cs2 must be > > > asserted to access the shadow DW PCIe CSRs space for both RC and > > > EP including the BARs mask and their enabling/disabling flag (there > > > are many more shadow CSRs available on DW PCIe EPs, and a few in DW > > > PCIe RCs). That's why the dw_pcie_ops->writel_dbi2 pointer has been > > > defined in the first place (dbi2 suffix means dbi_cs2). You should use > > > it to create the platform-specific dbi_cs2 write accessors like it's > > > done in pci-keystone.c and pcie-bt1.c. For instance like this: > > > > > > static void qcom_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val) > > > { > > > struct qcom_pcie_ep *pcie_ep = to_pcie_ep(pci); > > > int ret; > > > > > > writel(1, pcie_ep->elbi + ELBI_CS2_ENABLE); > > > > > > ret = dw_pcie_write(pci->dbi_base2 + reg, size, val); > > > if (ret) > > > dev_err(pci->dev, "write DBI address failed\n"); > > > > > > writel(0, pcie_ep->elbi + ELBI_CS2_ENABLE); > > > } > > > > > > > > Hmm, I was not aware that this write_dbi2() callback is supposed to enable the > > CS2 access internally. But this approach doesn't look good to me. > > > > We already have accessors for enabling write access to DBI RO registers: > > > > dw_pcie_dbi_ro_wr_en() > > dw_pcie_dbi_ro_wr_dis() > > > > And IMO DBI_CS2 access should also be done this way instead of hiding it inside > > the register write callback. > > No, for many-many reasons. > > First of all, DBI RO/RW switch and DBI/DBI2 are absolutely different > things. Former one switches the CSRs access attributes in both DBI and > DBI2 CSR spaces. The later one are two CSR spaces, which access to is > platform-specific. They can't and shouldn't be compared. DBI2 space > is a shadow DBI CSRs space, which registers aren't just the RW > versions of the DBI space, but its CSRs normally have different > semantics with respect to the normal DBI CSRs available on the same > offsets. I.e. BAR0 contains MEM/IO, TYPE, PREF flags and base address, > meanwhile DBI2-BAR0 - BAR enable/disable flag, BAR mask. From that > perspective having the dw_pcie_ops.write_dbi2 callback utilized for > the DBI2 space access would provide an interface looking similar to > the just DBI space - dw_pcie_ops.{write_dbi,read_dbi}. Thus the > unified access interface would provide much more readable code, where > based on the method name you'll be able to immediately infer the space > being accessed to. > > Secondly, DBI RO/RW switch is a straight-forward CSR flag > clearing/setting DBI_RO_WR_EN. This mechanism is available on all DW > PCIe IP-cores and works in the _same_ way on all of them: just the > MISC_CONTROL_1_OFF.DBI_RO_WR_EN flag switching. It switches RO/RW > access attributes on both DBI_CS and DBI_CS2. So it's a cross-platform > thing independent from the vendor-specific IP-core settings. That's > why having generic functions for the RO/RW switch was the best choice: > it's cross-platform so no need in the platform-specific callbacks, it > works for both DBI and DBI2 so instead of implementing two additional > RW-accessors you can call the RW en/dis method around the DBI and DBI2 > accessors whenever you need to switch the access attributes. > > On the contrary DBI_CS2 is the DW PCIe IP-core input signal which > activation is platform-specific. Some platforms have it switchable via > a system-controller, some - via an additional elbi CSRs space, some - > provide an additional CSR space mapping DBI2 with no need in the > direct DBI_CS2 flag toggle, some may have an intermix of these > setups or may need some additional manipulation to access the DBI2 > space. So your case of having the DBI_CS2 flag available via the elbi > space and having the DBI/DBI2 space mapped within the 4K offset with > respect to DBI is just a single possible option. > > Finally, it's all about simplicity, maintainability and KIS principle. > Your approach would imply having additional platform-specific > callbacks, meanwhile there is already available DBI2 space accessor > which is more than enough to implement what you need. Even if you > drop the later one (and convert all the already available LLDDs to > supporting what you want), having two callbacks instead of one will > still make things harder to read, because the kernel hacker would need > to keep in mind the DBI/DBI2 access context. Additionally calling > _three_ methods instead of a _single_ one will look much more complex. > Moreover having on/off antagonists prune to errors since a developer > may forget to disable the DBI2 flag, which on some platforms will > change the DBI CSRs semantics. Such error will be just impossible to > meet should the current interface is preserved unless the > platform-specific DBI2 accessor is incorrectly implemented, which > would be still specific to the particular platform, but not for the > entire DW PCIe driver. The last but not least, as I already mentioned > dw_pcie_ops.write_dbi2 and respective wrappers look as much like the > normal DBI accessors dw_pcie_ops.{write_dbi,read_dbi} which greatly > improves the code readability. > Hmm, thanks for the detailed clarification, really appreciated. My understanding about the DBI2 space was not clear and your reply clarified that. I will implement the write_dbi2() callback as suggested. - Mani > So no, I failed to find any firm justification for the approach you > suggest. > > -Serge(y) > > > > > - Mani > > > > > /* Common DWC controller ops */ > > > static const struct dw_pcie_ops pci_ops = { > > > .link_up = qcom_pcie_dw_link_up, > > > .start_link = qcom_pcie_dw_start_link, > > > .stop_link = qcom_pcie_dw_stop_link, > > > .write_dbi2 = qcom_pcie_write_dbi2, > > > }; > > > > > > For that reason there is absolutely no need in adding the new > > > callbacks. > > > > > > -Serge(y) > > > > > > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > drivers/pci/controller/dwc/pcie-designware-ep.c | 6 ++++++ > > > > drivers/pci/controller/dwc/pcie-designware.h | 13 +++++++++++++ > > > > 2 files changed, 19 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > index d34a5e87ad18..1874fb3d8df4 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > > > > @@ -269,11 +269,17 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > > > > > > > > dw_pcie_dbi_ro_wr_en(pci); > > > > > > > > + dw_pcie_dbi_cs2_en(pci); > > > > dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1)); > > > > + dw_pcie_dbi_cs2_dis(pci); > > > > + > > > > dw_pcie_writel_dbi(pci, reg, flags); > > > > > > > > if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) { > > > > + dw_pcie_dbi_cs2_en(pci); > > > > dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, upper_32_bits(size - 1)); > > > > + dw_pcie_dbi_cs2_dis(pci); > > > > + > > > > dw_pcie_writel_dbi(pci, reg + 4, 0); > > > > } > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > > > index 55ff76e3d384..3cba27b5bbe5 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > > @@ -379,6 +379,7 @@ struct dw_pcie_ops { > > > > size_t size, u32 val); > > > > void (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg, > > > > size_t size, u32 val); > > > > + void (*dbi_cs2_access)(struct dw_pcie *pcie, bool enable); > > > > int (*link_up)(struct dw_pcie *pcie); > > > > enum dw_pcie_ltssm (*get_ltssm)(struct dw_pcie *pcie); > > > > int (*start_link)(struct dw_pcie *pcie); > > > > @@ -508,6 +509,18 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) > > > > dw_pcie_writel_dbi(pci, reg, val); > > > > } > > > > > > > > +static inline void dw_pcie_dbi_cs2_en(struct dw_pcie *pci) > > > > +{ > > > > + if (pci->ops && pci->ops->dbi_cs2_access) > > > > + pci->ops->dbi_cs2_access(pci, true); > > > > +} > > > > + > > > > +static inline void dw_pcie_dbi_cs2_dis(struct dw_pcie *pci) > > > > +{ > > > > + if (pci->ops && pci->ops->dbi_cs2_access) > > > > + pci->ops->dbi_cs2_access(pci, false); > > > > +} > > > > + > > > > static inline int dw_pcie_start_link(struct dw_pcie *pci) > > > > { > > > > if (pci->ops && pci->ops->start_link) > > > > > > > > -- > > > > 2.25.1 > > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c index d34a5e87ad18..1874fb3d8df4 100644 --- a/drivers/pci/controller/dwc/pcie-designware-ep.c +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c @@ -269,11 +269,17 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no, dw_pcie_dbi_ro_wr_en(pci); + dw_pcie_dbi_cs2_en(pci); dw_pcie_writel_dbi2(pci, reg_dbi2, lower_32_bits(size - 1)); + dw_pcie_dbi_cs2_dis(pci); + dw_pcie_writel_dbi(pci, reg, flags); if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64) { + dw_pcie_dbi_cs2_en(pci); dw_pcie_writel_dbi2(pci, reg_dbi2 + 4, upper_32_bits(size - 1)); + dw_pcie_dbi_cs2_dis(pci); + dw_pcie_writel_dbi(pci, reg + 4, 0); } diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 55ff76e3d384..3cba27b5bbe5 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -379,6 +379,7 @@ struct dw_pcie_ops { size_t size, u32 val); void (*write_dbi2)(struct dw_pcie *pcie, void __iomem *base, u32 reg, size_t size, u32 val); + void (*dbi_cs2_access)(struct dw_pcie *pcie, bool enable); int (*link_up)(struct dw_pcie *pcie); enum dw_pcie_ltssm (*get_ltssm)(struct dw_pcie *pcie); int (*start_link)(struct dw_pcie *pcie); @@ -508,6 +509,18 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci) dw_pcie_writel_dbi(pci, reg, val); } +static inline void dw_pcie_dbi_cs2_en(struct dw_pcie *pci) +{ + if (pci->ops && pci->ops->dbi_cs2_access) + pci->ops->dbi_cs2_access(pci, true); +} + +static inline void dw_pcie_dbi_cs2_dis(struct dw_pcie *pci) +{ + if (pci->ops && pci->ops->dbi_cs2_access) + pci->ops->dbi_cs2_access(pci, false); +} + static inline int dw_pcie_start_link(struct dw_pcie *pci) { if (pci->ops && pci->ops->start_link)