From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f182.google.com (mail-pf1-f182.google.com [209.85.210.182]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2C9C81C1F33 for ; Tue, 19 Nov 2024 10:11:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.182 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732011092; cv=none; b=f/BZC+dl4/LJ7gYsYoxkygNVNplDel77kkWTf2GEAX1P6j4vFXRKP94skl1E0bGBz3Y9QO5PjtrXOsK4jayd5ywO8toNfrDJpOSUSKcZv8dlpk04JdqvsqomdpP+h7DpQeNLlYrLNwea7LC4/5+xFOA96Pg98eAWG/ePz8Cx3EY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1732011092; c=relaxed/simple; bh=PlG7ajC4HIproRyok2uR0kaxQChqGXyTvCvbDgTqby8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=HwJMnun0uXfO+l+tZboVZW/5ZhkwCxuM3GUpNbCEl6e9I46kQVs2OtO2dVaT4IBEOSnuOmf897n4abSg5XmzuksaoQmv0GoFF4KR3c0cgNSiq7RsFzp/f6WF7SsfkxaLe/JJ529t8jJ4fguDTS8ALfRh0VtdDXnDu1qkaxYv5n4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=M/+mIZs4; arc=none smtp.client-ip=209.85.210.182 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="M/+mIZs4" Received: by mail-pf1-f182.google.com with SMTP id d2e1a72fcca58-72041ff06a0so2086218b3a.2 for ; Tue, 19 Nov 2024 02:11:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1732011090; x=1732615890; darn=lists.linux.dev; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=CnyTQoucjCJF6wmIwhYG5inOePLfmtO8FEJdC34/wtE=; b=M/+mIZs4bn7UBbx844EY+tfO34SNUk8m0FQ1+Q/RQHlfUzSGUAINSJ69tbZQsnGsr8 ADPObYeV6tzxLMl0aYdxG9XpQ8Wx0MoMzpywYv1plymoo6wvw3FQioGUOirNch6YGN5V NmdwmlR41yk+QDUOJQPpsW/UAwiSdZOLiXo4kzrQ0HAIMJ1/3hFWWS+d5s9ke+S+h/SJ rrg5oqynEz9LstkYXZOV/2yClJ2mFDUyrJW/zRnLUEQmrzMpaXBmUzM5jf62lWcYnZNe 8gVDNMLVrqjJ44/cFsldG62f+cPMOuf+iGWjYIXyi3HjhCct2WMzhN2GPDwbphxNhdKM jOGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732011090; x=1732615890; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=CnyTQoucjCJF6wmIwhYG5inOePLfmtO8FEJdC34/wtE=; b=Ra6x7HyaFH78WZHtsAUJa3QiOmAi7WJ4IMA7kTedO5VrMpC3Jd2HBzvPWBALRNKjGJ pJHEkyvbZ5ONZ9J7QQLrD8fnIm07wylxVgBuLFrKrRfrx0S2c+HKz6LqS4ZVndjF0cvo O0OdnKUkc8sxwRv9gvkDmbDqahwRhgHxprMnyKqinUwveTWX5s7WqiaYbuUS8vqIn0// 3qQSmAXvVzAs9sWfodAD6YTeMrAIxgQt95x9zt6Zm9Vb8guhfIPNOrY047Ucot0NL2cN GbuKczLnZ9lOBEzFbGKjnPgu2ZvUO6loZBUAb+b2F0yDO+OSxbjnXPx2aI0yv0HdqFUq JOqQ== X-Forwarded-Encrypted: i=1; AJvYcCWUIkyKwaT5IEM9sHDOQSwpYT11qNrHR5553WVrcHsgalhh9NMdtXuF6iCk1llx9FDfPr8=@lists.linux.dev X-Gm-Message-State: AOJu0Yw9w6xOxFYMGXwfr88Pv84JgkjcFPPCEVA/TMxe4dvFS6OxZN+G qmZczLHzS2VnrPpQbpgvAvlK0or1OLBnJTV0gzcErlVsKgTH2Z5EAE8O8d6CzQ== X-Google-Smtp-Source: AGHT+IESlkjEE6zH0kn+oCxIKUTmqhbcdMhskn/HWsahaCPc0PrMTVIaTfJQLiOkn57Z+JD1MOMz3Q== X-Received: by 2002:a05:6a00:9294:b0:71d:f15e:d026 with SMTP id d2e1a72fcca58-72476b7271emr20918708b3a.3.1732011090344; Tue, 19 Nov 2024 02:11:30 -0800 (PST) Received: from thinkpad ([117.213.96.14]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-72477120a13sm8051655b3a.60.2024.11.19.02.11.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 Nov 2024 02:11:29 -0800 (PST) Date: Tue, 19 Nov 2024 15:41:21 +0530 From: Manivannan Sadhasivam To: Frank Li Cc: Rob Herring , Saravana Kannan , Jingoo Han , 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 Subject: Re: [PATCH v7 2/7] PCI: dwc: Using parent_bus_addr in of_range to eliminate cpu_addr_fixup() Message-ID: <20241119101121.t4kaaeuvj37scmxm@thinkpad> References: <20241029-pci_fixup_addr-v7-0-8310dc24fb7c@nxp.com> <20241029-pci_fixup_addr-v7-2-8310dc24fb7c@nxp.com> <20241115175148.tqzqiv53mccz52tq@thinkpad> Precedence: bulk X-Mailing-List: imx@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Fri, Nov 15, 2024 at 02:18:38PM -0500, Frank Li wrote: [...] > > > + if (pci->using_dtbus_info) { > > > + 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 > > > + * 'using_dtbus_info' to true, The 'cpu_addr_fixup()' > > > + * can be eliminated. > > > + */ > > > > Nobody will switch to 'ranges' property if you mention it in comments. We > > usually add dev_warn_once() to print a warning for broken DT so that the users > > will try to fix it. You can use below diff (as a separate patch ofc): > > > > ``` > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index 6d6cbc8b5b2c..d1e5395386fe 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -844,6 +844,9 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci) > > dw_pcie_cap_is(pci, IATU_UNROLL) ? "T" : "F", > > pci->num_ob_windows, pci->num_ib_windows, > > pci->region_align / SZ_1K, (pci->region_limit + 1) / SZ_1G); > > + > > + if (pci->ops && pci->ops->cpu_addr_fixup) > > + dev_warn_once(pci->dev, "Broken \"ranges\" property detected. Please fix DT!\n"); > > How about "Detect use old method "cpu_addr_fixup()", it should correct DT's > 'ranges' and remove cpu_addr_fixup()? > Hmm, yeah makes sense: /* * 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"; But then the drivers need to be smart enough to detect the valid parent 'ranges' property and then only use the callback. Because, callback has to be present to support older DTs. > > } > > > > static u32 dw_pcie_readl_dma(struct dw_pcie *pci, u32 reg) > > ``` > > > > > + of_property_read_reg(np, index, &pp->cfg0_base, NULL); > > > > Can you explain what is going on here? > > Because dwc use reg-name 'config' to get config space, > of_property_read_reg() will get untranslate address 'parent' bus address. > <0x8ff00000 0x80000> at example address. > > cfg0_base is used to set outbound ATU. > Ok, please add a comment like this: /* Get the untranslated 'config' address */ Same for other usage of of_property_read_reg(). > > > > > + } > > > + > > > pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res); > > > if (IS_ERR(pp->va_cfg0_base)) > > > return PTR_ERR(pp->va_cfg0_base); > > > @@ -462,6 +505,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > > > pp->io_base = pci_pio_to_address(win->res->start); > > > } > > > > > > + if (dw_pcie_get_untranslate_addr(pci, pp->io_bus_addr, &pp->io_base)) > > > + return -ENODEV; > > > > Use actual return value here and below. > > > > > + > > > /* Set default bus ops */ > > > bridge->ops = &dw_pcie_ops; > > > bridge->child_ops = &dw_child_pcie_ops; > > > @@ -722,6 +768,8 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) > > > > > > i = 0; > > > resource_list_for_each_entry(entry, &pp->bridge->windows) { > > > + resource_size_t parent_bus_addr; > > > + > > > if (resource_type(entry->res) != IORESOURCE_MEM) > > > continue; > > > > > > @@ -730,9 +778,14 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp) > > > > > > atu.index = i; > > > atu.type = PCIE_ATU_TYPE_MEM; > > > - atu.cpu_addr = entry->res->start; > > > + parent_bus_addr = entry->res->start; > > > atu.pci_addr = entry->res->start - entry->offset; > > > > > > + if (dw_pcie_get_untranslate_addr(pci, entry->res->start, &parent_bus_addr)) > > > + return -EINVAL; > > > + > > > + atu.cpu_addr = parent_bus_addr; > > > + > > > /* Adjust iATU size if MSG TLP region was allocated before */ > > > if (pp->msg_res && pp->msg_res->parent == entry->res) > > > atu.size = resource_size(entry->res) - > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > > > index 347ab74ac35aa..f8067393ad35a 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.h > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h > > > @@ -463,6 +463,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. > > > > /* > > * This flag indicates that the vendor driver uses devicetree 'ranges' > > * property to allow iATU to use the Intermediate Address (IA) for > > * outbound mapping. Using this flag also avoids the usage of > > * 'cpu_addr_fixup' callback implementation in the driver. > > */ > > > > > + */ > > > + bool using_dtbus_info; > > > > 'use_dt_ranges'? > > It will be confused because pcie node alreadys use ranges, just parent bus > 's ranges is wrong. > > 'use_dtbus_ranges' ? > There is nothing called 'dtbus'. How about, "use_parent_dt_ranges"? - Mani -- மணிவண்ணன் சதாசிவம்