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 425121E495; Fri, 24 Jan 2025 06:12:53 +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=1737699174; cv=none; b=EPHQyuMfkNe/u4f43IOkrLNnfhLYzTD2VGMeQTgayrpHy0czWZfODt2cCLh1X/jQ3GlWpym8UXnAf7bch2Mfo6OtPflqq9VUbXxEc5IixZFnSC8IAvZx06qkkqIYy9qdlkNqXRbtJXj8IlNPBIE4ZzTNwqpf/TPpWosWNlrj/zM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737699174; c=relaxed/simple; bh=K6SNz+C4TSRHavBgGbpfP1sGsC4ulC5iTxo42sxZJAs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=GNmyhZ1c1eWQ6Dji4py9nDZGW786ni9a2gjh7yUTy980mcZJNl1bJVuktq5/sX9lSblYVzNuEunX+hlhneSJrBOOilJQAVvbsQ3vREj7O4I/VuLydYQ+dbHajjwRU8oGiLhq0HVBw1K8ZXfUebLib5BnnkwJywzy+A0dMzxnvzw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=rz6LNp1R; 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="rz6LNp1R" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 44545C4CED2; Fri, 24 Jan 2025 06:12:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1737699173; bh=K6SNz+C4TSRHavBgGbpfP1sGsC4ulC5iTxo42sxZJAs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=rz6LNp1RuGdGS8oEuwu9Gsue3ol6b3saVs1GcL0VOZtVKdEzje6kKeOTVYn3kSgx3 ztRYJSRe1P//a/vxcutQa/pKIN1DlBUGTxndwkLSw7l2DmPlNW8350pAZB8mph8cYa +CEz88UaBPb7BT//JW71DiYaKEE/9+pWBmDRpy9nmQka3J0PN5TDtCztdQxkrgj+Fa JPCc1ckeUSNjLT+MCvusNYoPAPG/6p5GzyuuWeuy/gKd7MLu6sqT6C0DJ/5JhCp68X 0FNiaL/d4vUuHuMtBAOBouVzVnwgQdwLqOimZ6uR8n76Kej7QZq8Ak73sWar6ZHOfT AXQKnCRiZQdtw== Date: Fri, 24 Jan 2025 11:42:37 +0530 From: Manivannan Sadhasivam To: Krishna Chaitanya Chundru Cc: cros-qcom-dts-watchers@chromium.org, Bjorn Andersson , Konrad Dybcio , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Manivannan Sadhasivam , Bjorn Helgaas , Jingoo Han , linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, quic_vbadigan@quicinc.com, quic_vpernami@quicinc.com, quic_mrana@quicinc.com, mmareddy@quicinc.com, Krishna chaitanya chundru Subject: Re: [PATCH v3 2/4] PCI: dwc: Add ECAM support with iATU configuration Message-ID: <20250124061237.he34clwukqwzqx2t@thinkpad> References: <20250121-enable_ecam-v3-0-cd84d3b2a7ba@oss.qualcomm.com> <20250121-enable_ecam-v3-2-cd84d3b2a7ba@oss.qualcomm.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org 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: <20250121-enable_ecam-v3-2-cd84d3b2a7ba@oss.qualcomm.com> On Tue, Jan 21, 2025 at 02:32:20PM +0530, Krishna Chaitanya Chundru wrote: > The current implementation requires iATU for every configuration > space access which increases latency & cpu utilization. > > Designware databook 5.20a, section 3.10.10.3 says about CFG Shift Feature, > which shifts/maps the BDF (bits [31:16] of the third header DWORD, which > would be matched against the Base and Limit addresses) of the incoming > CfgRd0/CfgWr0 down to bits[27:12]of the translated address. > > Configuring iATU in config shift feature enables ECAM feature to access the > config space, which avoids iATU configuration for every config access. > > Add "ctrl2" into struct dw_pcie_ob_atu_cfg to enable config shift feature. > > As DBI comes under config space, this avoids remapping of DBI space > separately. Instead, it uses the mapped config space address returned from > ECAM initialization. Change the order of dw_pcie_get_resources() execution > to achieve this. > > Enable the ECAM feature if the config space size is equal to size required > to represent number of buses in the bus range property, add a function > which checks this. The DWC glue drivers uses this function and decide to > enable ECAM mode or not. > > Signed-off-by: Krishna Chaitanya Chundru Some minor comments inline. Overall, the patch LGTM! > --- > drivers/pci/controller/dwc/Kconfig | 1 + > drivers/pci/controller/dwc/pcie-designware-host.c | 139 +++++++++++++++++++--- > drivers/pci/controller/dwc/pcie-designware.c | 2 +- > drivers/pci/controller/dwc/pcie-designware.h | 11 ++ > 4 files changed, 133 insertions(+), 20 deletions(-) > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index b6d6778b0698..73c3aed6b60a 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -9,6 +9,7 @@ config PCIE_DW > config PCIE_DW_HOST > bool > select PCIE_DW > + select PCI_HOST_COMMON > > config PCIE_DW_EP > bool > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index d2291c3ceb8b..3888f9fe5af1 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -418,6 +418,66 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp) > } > } > > +static int dw_pcie_config_ecam_iatu(struct dw_pcie_rp *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct dw_pcie_ob_atu_cfg atu = {0}; > + resource_size_t bus_range_max; > + struct resource_entry *bus; > + int ret; > + > + bus = resource_list_first_type(&pp->bridge->windows, IORESOURCE_BUS); > + > + /* > + * Root bus under the root port doesn't require any iATU configuration You mean 'Root bus under host bridge'? > + * as DBI space will represent Root bus configuration space. > + * Immediate bus under Root Bus, needs type 0 iATU configuration and > + * remaining buses need type 1 iATU configuration. > + */ > + atu.index = 0; > + atu.type = PCIE_ATU_TYPE_CFG0; > + atu.cpu_addr = pp->cfg0_base + SZ_1M; > + atu.size = SZ_1M; > + atu.ctrl2 = PCIE_ATU_CFG_SHIFT_MODE_ENABLE; > + ret = dw_pcie_prog_outbound_atu(pci, &atu); > + if (ret) > + return ret; > + > + bus_range_max = resource_size(bus->res); > + > + if (bus_range_max < 2) > + return 0; > + > + /* Configure remaining buses in type 1 iATU configuration */ > + atu.index = 1; > + atu.type = PCIE_ATU_TYPE_CFG1; > + atu.cpu_addr = pp->cfg0_base + SZ_2M; > + atu.size = (SZ_1M * (bus_range_max - 2)); > + atu.ctrl2 = PCIE_ATU_CFG_SHIFT_MODE_ENABLE; > + > + return dw_pcie_prog_outbound_atu(pci, &atu); > +} > + > +static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *res) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct device *dev = pci->dev; > + struct resource_entry *bus; > + > + bus = resource_list_first_type(&pp->bridge->windows, IORESOURCE_BUS); > + if (!bus) > + return -ENODEV; > + > + pp->cfg = pci_ecam_create(dev, res, bus->res, &pci_generic_ecam_ops); > + if (IS_ERR(pp->cfg)) > + return PTR_ERR(pp->cfg); > + > + pci->dbi_base = pp->cfg->win; > + pci->dbi_phys_addr = res->start; > + > + return 0; > +} > + > int dw_pcie_host_init(struct dw_pcie_rp *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > @@ -431,19 +491,8 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > > raw_spin_lock_init(&pp->lock); > > - ret = dw_pcie_get_resources(pci); > - if (ret) > - return ret; > - > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); > - if (res) { > - pp->cfg0_size = resource_size(res); > - pp->cfg0_base = res->start; > - > - 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); > - } else { > + if (!res) { > dev_err(dev, "Missing *config* reg space\n"); > return -ENODEV; > } > @@ -454,6 +503,31 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > > pp->bridge = bridge; > > + pp->cfg0_size = resource_size(res); > + pp->cfg0_base = res->start; > + > + if (pp->ecam_mode) { > + ret = dw_pcie_create_ecam_window(pp, res); > + if (ret) > + return ret; Add a newline here. > + bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops; > + pp->bridge->sysdata = pp->cfg; > + pp->cfg->priv = pp; > + } else { > + 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); > + > + /* Set default bus ops */ > + bridge->ops = &dw_pcie_ops; > + bridge->child_ops = &dw_child_pcie_ops; > + bridge->sysdata = pp; > + } > + > + ret = dw_pcie_get_resources(pci); > + if (ret) > + goto err_free_ecam; > + > /* Get the I/O range from DT */ > win = resource_list_first_type(&bridge->windows, IORESOURCE_IO); > if (win) { > @@ -462,14 +536,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > pp->io_base = pci_pio_to_address(win->res->start); > } > > - /* Set default bus ops */ > - bridge->ops = &dw_pcie_ops; > - bridge->child_ops = &dw_child_pcie_ops; > - > if (pp->ops->init) { > ret = pp->ops->init(pp); > if (ret) > - return ret; > + goto err_free_ecam; > } > > if (pci_msi_enabled()) { > @@ -504,6 +574,12 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > > dw_pcie_iatu_detect(pci); > > + if (pp->ecam_mode) { > + ret = dw_pcie_config_ecam_iatu(pp); > + if (ret) Add a error print so that users will know what is going wrong. > + goto err_free_msi; > + } > + > /* > * Allocate the resource for MSG TLP before programming the iATU > * outbound window in dw_pcie_setup_rc(). Since the allocation depends > @@ -533,8 +609,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > /* Ignore errors, the link may come up later */ > dw_pcie_wait_for_link(pci); > > - bridge->sysdata = pp; > - > ret = pci_host_probe(bridge); > if (ret) > goto err_stop_link; > @@ -558,6 +632,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp) > if (pp->ops->deinit) > pp->ops->deinit(pp); > > +err_free_ecam: > + if (pp->cfg) > + pci_ecam_free(pp->cfg); > + > return ret; > } > EXPORT_SYMBOL_GPL(dw_pcie_host_init); > @@ -578,6 +656,9 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp) > > if (pp->ops->deinit) > pp->ops->deinit(pp); > + > + if (pp->cfg) > + pci_ecam_free(pp->cfg); > } > EXPORT_SYMBOL_GPL(dw_pcie_host_deinit); > > @@ -985,3 +1066,23 @@ int dw_pcie_resume_noirq(struct dw_pcie *pci) > return ret; > } > EXPORT_SYMBOL_GPL(dw_pcie_resume_noirq); > + > +bool dw_pcie_ecam_supported(struct dw_pcie_rp *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct platform_device *pdev = to_platform_device(pci->dev); > + struct resource *config_res, *bus_range; > + u64 bus_config_space_count; > + > + bus_range = resource_list_first_type(&pp->bridge->windows, IORESOURCE_BUS)->res; > + if (!bus_range) > + return false; > + > + config_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); > + if (!config_res) > + return false; > + > + bus_config_space_count = resource_size(config_res) >> PCIE_ECAM_BUS_SHIFT; > + > + return bus_config_space_count >= resource_size(bus_range); return !!(bus_config_space_count >= resource_size(bus_range)); You should export this API also to avoid someone sending a fix patch later if this gets called from a modular driver. - Mani -- மணிவண்ணன் சதாசிவம்