From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E33B62620C8; Wed, 26 Feb 2025 23:33:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740612823; cv=none; b=ToNiWlKcWU+ClqUp9ifJZDA5ELImfj4Q0LQO2H06QqR68YEPlWrHDUoLCyaV2UaigmrKegAbvBRsxuFSN9LzFT3lhQzgoR+I07MI8yAj5lB99jAjM9jwvrMIMY064fpYsyocTF/Ke8DQFPhWQZZs5QSTLws6v5h85CrZqlUzP/k= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740612823; c=relaxed/simple; bh=JU2kjfLvtCKvCAcKZzE2OOPkpTNFwZNgTF7Ciu+cRA4=; h=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type: Content-Disposition:In-Reply-To; b=ehWFxzqKcU4ucnBUvvVcycE+IU/o1tBv58OVQ6eHXbXhFliDA3StmAd1ZXkuReM6i4Z4CFAxTOPgr5c+RU4cDdyyOvpfgzasnZfFGxzs9TXqmaic/sCoRf69m5/KT/lyXP9PKe7SoBctAOG18S6ZAqsS/7aZy2SckzARYGCjnUA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=mph1cYH0; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="mph1cYH0" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2FD53C4CED6; Wed, 26 Feb 2025 23:33:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1740612822; bh=JU2kjfLvtCKvCAcKZzE2OOPkpTNFwZNgTF7Ciu+cRA4=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=mph1cYH0/GtxEnkYNjCOOhVVx56YpsQjSOk1NStNxEAjASxrD7HdP89Eu4QiFsjQp FaY8Xks0fxxWsXbG0tGc1hVg1E8MwdMRqFTK8X7jjvATAcXjECwZqgiyPCvT3dHxUt qW1qWzKdtvNXzRAGOsnQSV66plI7HGitg8nnLwYby1irqcym0luHmdV+wN76k2JwOm iR34vJbsDmq6bIXion0gGlpstWi49EKe12WKUXlHfzk0KtCantRd19Hb9ATkeh73Qx sOq2e+gAUnuYFRHoKt4gCCqbzhMUX33KGRP89sfavZuSOlnmEtXD32v2ULnB/BGLti +yYugUjDQL0qQ== Date: Wed, 26 Feb 2025 17:33:39 -0600 From: Bjorn Helgaas To: Frank Li Cc: Rob Herring , Saravana Kannan , Jingoo Han , Manivannan Sadhasivam , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Bjorn Helgaas , Richard Zhu , Lucas Stach , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, imx@lists.linux.dev, Niklas Cassel Subject: Re: [PATCH v9 4/7] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr_fixup() callback Message-ID: <20250226233339.GA562682@bhelgaas> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250128-pci_fixup_addr-v9-4-3c4bb506f665@nxp.com> On Tue, Jan 28, 2025 at 05:07:37PM -0500, Frank Li wrote: > parent_bus_offset in resource_entry can indicate address information just > ahead of PCIe controller. Most system's bus fabric use 1:1 map between > input and output address. but some hardware like i.MX8QXP doesn't use 1:1 > map. See below diagram: > ... > @@ -448,6 +451,26 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > if (IS_ERR(pp->va_cfg0_base)) > return PTR_ERR(pp->va_cfg0_base); > > + if (pci->use_parent_dt_ranges) { > + if (pci->ops->cpu_addr_fixup) { > + dev_err(dev, "Use parent bus DT ranges, cpu_addr_fixup() must be removed\n"); > + return -EINVAL; > + } > + > + index = of_property_match_string(np, "reg-names", "config"); > + if (index < 0) > + return -EINVAL; > + > + /* > + * Retrieve the parent bus address of PCI config space. > + * If the parent bus ranges in the device tree provide > + * the correct address conversion information, set > + * 'use_parent_dt_ranges' to true, The > + * 'cpu_addr_fixup()' can be eliminated. > + */ > + of_property_read_reg(np, index, &pp->cfg0_base, NULL); > + } I think all this code dealing with the "config" resource could go in a helper function. It's kind of a lot of clutter in dw_pcie_host_init(). It would be nice to assign pp->cfg0_base once, not assign res->start to it and then possibly overwrite it later. > @@ -841,6 +841,15 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci) > pci->region_align = 1 << fls(min); > pci->region_limit = (max << 32) | (SZ_4G - 1); > > + if (pci->ops && pci->ops->cpu_addr_fixup) { > + /* > + * If the parent 'ranges' property in DT correctly describes > + * the address translation, cpu_addr_fixup() callback is not > + * needed. > + */ > + dev_warn_once(pci->dev, "cpu_addr_fixup() usage detected. Please fix DT!\n"); > + } Can you split the warnings out to a separate patch? I think we should warn once in every initialization path where .cpu_addr_fixup() could be used, i.e., dw_pcie_host_init() dw_pcie_ep_init() cdns_pcie_host_setup() cdns_pcie_ep_setup() IMO these should warn if .cpu_addr_fixup() is implemented, regardless of use_parent_dt_ranges. I'm still puzzling over some of the rest of this, so no need to post a revised series yet. Bjorn