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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 55700D149F7 for ; Fri, 25 Oct 2024 22:33:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References:List-Owner; bh=+DP4AWLHlkrnxhJ5ab9uhOjA+6iQVhOlJJSSLg9sr1M=; b=DPnWj9s9U9t+9B8VUQEyAeuL5O EPxb7LK1l5py3tCvvFCQgrYC0uGIKbjcE+wwQ7dJHOxBLYAq50s8knncDJKD96uU4iwa4sbvYf55G +BBwdV7lDRUmgPrMeg66CpBF5bMbsEhAx8Gw6XQOyau7W9+P7ruaFlkUBUTwnLW1rdHfgclHrbXTN WA8wOwg61n+ta99mGQfWwvJFid0OjVyB7TNPZnU9Stl9cF8wPo5RZaaXNsbYnbtBsT/VbJ7tOAUi2 jMj/S7a/XD8lLJ1ZWovB6qm1ffs5uRD3VCJ0ZB3V+njRVGL/QEQ9K1lCPK2vqMq4PmwWObhFwUVst VpDwCpQg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t4SrR-00000005RmQ-3ywz; Fri, 25 Oct 2024 22:32:49 +0000 Received: from nyc.source.kernel.org ([147.75.193.91]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1t4Spm-00000005RPo-0xs0 for linux-arm-kernel@lists.infradead.org; Fri, 25 Oct 2024 22:31:10 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 6B7DAA44BE3; Fri, 25 Oct 2024 22:29:08 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DD3ABC4CEC3; Fri, 25 Oct 2024 22:31:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1729895464; bh=e9UZsK6yoNGuTeM8ImClDT58R9Ad0kplRUUu9ysuAJ4=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=qNEiENyRow1zkUKlGDesaiB2BCDUXVV6QJFBiRhrK7QwB0dqBCWzpQL/G16crJrxq tkqmEXINJDcVW98hnnk+gLaDxetCPb+SQ4ZJ7t+M9TCrQ1pvW+RjLNubTFBZiKFHOL fKQNw25C8RCDbkbrZHdtoEIvm2k+/+TQHc/ycYL02WXA0Q95ue85LBZMUAOXG6U4z6 1Y1hes/gexQjwToENV6Z+FrqU7R0uT/SibpnwW+hs+ZrScxYYchmZxsOcIi4N73+yX L/EV16/tyiKqhBkQCgCbLH8s9E6CgwB8nUTl22CYjCb0EvGC6OVUT7pP3Y7QBM0jPG vOe8MRT+27rtg== Date: Fri, 25 Oct 2024 17:31:02 -0500 From: Bjorn Helgaas To: Frank Li Cc: Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Manivannan Sadhasivam , Rob Herring , Bjorn Helgaas , Krzysztof Kozlowski , Conor Dooley , Abraham I , Saravana Kannan , Jingoo Han , Gustavo Pimentel , Jesper Nilsson , Richard Zhu , Lucas Stach , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , linux-pci@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@axis.com, linux-arm-kernel@lists.infradead.org, imx@lists.linux.dev, Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= Subject: Re: [PATCH v4 1/4] PCI: dwc: ep: Add bus_addr_base for outbound window Message-ID: <20241025223102.GA1029107@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20241024-pcie_ep_range-v4-1-08f8dcd4e481@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241025_153106_433344_12063CFC X-CRM114-Status: GOOD ( 39.49 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Oct 24, 2024 at 04:41:43PM -0400, Frank Li wrote: > Endpoint Root complex > ┌───────┐ ┌─────────┐ > ┌─────┐ │ EP │ │ │ ┌─────┐ > │ │ │ Ctrl │ │ │ │ CPU │ > │ DDR │ │ │ │ ┌────┐ │ └──┬──┘ > │ │◄──────┼─ATU ◄─┼────────┼─┤BarN│◄─┼─────────┘ > │ │ │ │ │ └────┘ │ Outbound Transfer > └─────┘ │ │ │ │ > │ │ │ │ > │ │ │ │ > │ │ │ │ Inbound Transfer > │ │ │ │ ┌──▼──┐ > ┌───────┐ │ │ │ ┌───────┼─────►│DDR │ > │ │ outbound Transfer* │ │ │ └─────┘ > ┌─────┐ │ Bus ┼─────►│ ATU ─┬────────┼─┘ │ > │ │ │ Fabric│Bus │ │ PCI Addr │ > │ CPU ├───►│ │Addr │ │ 0xA000_0000 │ > │ │CPU │ │0x8000_0000 │ │ │ > └─────┘Addr└───────┘ │ │ │ │ > 0x7000_0000 └───────┘ └─────────┘ > > Add `bus_addr_base` to configure the outbound window address for CPU write. > The bus fabric generally passes the same address to the PCIe EP controller, > but some bus fabrics convert the address before sending it to the PCIe EP > controller. > > Above diagram, CPU write data to outbound windows address 0x7000_0000, > Bus fabric convert it to 0x8000_0000. ATU should use bus address > 0x8000_0000 as input address and convert to PCI address 0xA000_0000. Thanks for the diagram and description. I don't think the top half is relevant to *this* patch, is it? I think this patch is only concerned with the address translations between the CPU in the endpoint and the PCI bus address. In this case it happens in two steps: the bus fabric applies one offset, and the ATU applies a second offset. Unless the top half is relevant, I would omit it and simply use something like this: Endpoint ┌───────────────────────────────────────────────┐ │ pcie-ep@5f010000 │ │ ┌────────────────┐│ │ │ Endpoint ││ │ │ PCIe ││ │ │ Controller ││ │ bus@5f000000 │ ││ │ ┌──────────┐ │ ││ │ │ │ Outbound Transfer ││ │┌─────┐ │ Bus ┼─────►│ ATU ──────────┬┬─────► ││ │ │ Fabric │Bus │ ││PCI Addr ││ CPU ├───►│ │Addr │ ││0xA000_0000 ││ │CPU │ │0x8000_0000 ││ │└─────┘Addr└──────────┘ │ ││ │ 0x7000_0000 └────────────────┘│ └───────────────────────────────────────────────┘ If you don't want a big "Endpoint" box including the CPU and bus fabric, that's OK with me, too. I added it because everything on the PCI side only sees TLPs that contain PCI bus addresses, and can't tell anything about the internal implementation of the Endpoint. > Previously, `cpu_addr_fixup()` was used to handle address conversion. Now, > the device tree provides this information, preferring a common method. > > bus@5f000000 { > compatible = "simple-bus"; > ranges = <0x80000000 0x0 0x70000000 0x10000000>; > > pcie-ep@5f010000 { > reg = <0x5f010000 0x00010000>, > <0x80000000 0x10000000>; > reg-names = "dbi", "addr_space"; > ... > }; > ... > }; I guess bus@5f000000 includes a "ranges" property because that translation from 0x7000_0000 -> 0x8000_0000 is fixed or at least not touched by Linux? And the pcie-ep@5f010000 address translation from 0x8000_0000 to 0xA000_0000 *is* programmed by Linux and therefore can't be described by a DT? But I guess Linux only programs the *PCI* side, and the parent side (0x8000_0000) is fixed? AFAICT, the "reg = <0x5f010000 0x00010000>" part is not relevant here. I guess this implementation assumes there's only a single aperture through the Bus Fabric, right? And also a single ATU aperture through the endpoint PCIe controller? And also that there's only one layer of Bus Fabric address translation? The fact that only a few DWC controllers have this translation suggests that this part of the picture might be external to the DWC IP and there could be more variation. But I guess there's no point in adding code for topologies that don't exist; we can deal with that if the need ever arises. > 'ranges' in bus@5f000000 descript how address convert from CPU address > to bus address. > > Use `of_property_read_reg()` to obtain the bus address and set it to the > ATU correctly, eliminating the need for vendor-specific cpu_addr_fixup(). > > Add 'using_dtbus_info' to indicate device tree reflect correctly bus > address translation in case break compatibility. > > Signed-off-by: Frank Li > --- > Change from v3 to v4 > - change bus_addr_base to u64 to fix 32bit build error > | Reported-by: kernel test robot > | Closes: https://lore.kernel.org/oe-kbuild-all/202410230328.BTHareG1-lkp@intel.com/ > > Change from v2 to v3 > - Add using_dtbus_info to control if use device tree bus ranges > information. > --- > drivers/pci/controller/dwc/pcie-designware-ep.c | 14 +++++++++++++- > drivers/pci/controller/dwc/pcie-designware.h | 9 +++++++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c > index 43ba5c6738df1..81b4057befa62 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c > @@ -9,6 +9,7 @@ > #include > #include > #include > +#include > #include > > #include "pcie-designware.h" > @@ -294,7 +295,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no, > > atu.func_no = func_no; > atu.type = PCIE_ATU_TYPE_MEM; > - atu.cpu_addr = addr; > + atu.cpu_addr = addr - ep->phys_base + ep->bus_addr_base; Tangent: Maybe dw_pcie_ob_atu_cfg.cpu_addr isn't exactly the right name, since it now contains an address that is not a CPU physical address. Not a question for *this* patch though. > atu.pci_addr = pci_addr; > atu.size = size; > ret = dw_pcie_ep_outbound_atu(ep, &atu); > @@ -861,6 +862,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > struct device *dev = pci->dev; > struct platform_device *pdev = to_platform_device(dev); > struct device_node *np = dev->of_node; > + int index; > > INIT_LIST_HEAD(&ep->func_list); > > @@ -873,6 +875,16 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) > return -EINVAL; > > ep->phys_base = res->start; > + ep->bus_addr_base = ep->phys_base; > + > + if (pci->using_dtbus_info) { > + index = of_property_match_string(np, "reg-names", "addr_space"); > + if (index < 0) > + return -EINVAL; > + > + of_property_read_reg(np, index, &ep->bus_addr_base, NULL); > + } If this translation were fixed, I suppose we'd extract something from a "ranges" property that contains (child-bus-address, parent-bus-address) information. So I suppose "addr_space" contains a fixed parent-bus-address, and is setting the child (PCI) bus address, right? If so, I might add a comment here for other readers who come this way. (And me, because I won't remember the next time I read it :) > ep->addr_size = resource_size(res); > > if (ep->ops->pre_init) > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 347ab74ac35aa..f10b533b04f77 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -410,6 +410,7 @@ struct dw_pcie_ep { > struct list_head func_list; > const struct dw_pcie_ep_ops *ops; > phys_addr_t phys_base; > + u64 bus_addr_base; > size_t addr_size; > size_t page_size; > u8 bar_to_atu[PCI_STD_NUM_BARS]; > @@ -463,6 +464,14 @@ struct dw_pcie { > struct reset_control_bulk_data core_rsts[DW_PCIE_NUM_CORE_RSTS]; > struct gpio_desc *pe_rst; > bool suspended; > + /* > + * Use device tree 'ranges' property of bus node instead using > + * cpu_addr_fixup(). Some old platform dts 'ranges' in bus node may not > + * reflect real hardware's behavior. In case break these platform back > + * compatibility, add below flags. Set it true if dts already correct > + * indicate bus fabric address convert. > + */ > + bool using_dtbus_info; > }; > > #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp) > > -- > 2.34.1 >