[v3,2/5] PCI: dwc: Skip finding eDMA channels count if glue drivers have passed them
Message ID | 20240226-dw-hdma-v3-2-cfcb8171fc24@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-81289-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:a81b:b0:108:e6aa:91d0 with SMTP id bq27csp2015216dyb; Mon, 26 Feb 2024 03:47:49 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCXrtXtOG+NS8OLishUGQmKVQm100UVDgU3NU+ySz5HCkyZ3VOHbRmRRJXqfQHBg9hqyiVtZwQDEEysSxpLHTuXQihxNYg== X-Google-Smtp-Source: AGHT+IH2zqn6xBSKc0cIV9yKg6DKLN44hP33qFeC9KN5LqKfYTVT3vM1WBgD5tvZfnT66KowOpq/ X-Received: by 2002:a17:907:bb88:b0:a43:87a3:3fe8 with SMTP id xo8-20020a170907bb8800b00a4387a33fe8mr269299ejc.72.1708948069613; Mon, 26 Feb 2024 03:47:49 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708948069; cv=pass; d=google.com; s=arc-20160816; b=wBkWzKCQ5eNjOfWIkh7fDipfKqf5cAbQTap61+iXCck75eyIfaEXmoAYX/BxSj7mCh tLPZBshHvkJxCqiZlQbhD4/UcxosluAZxxBIIClO1mXz2w0CtzFOzw6HGiMQxd9tVWPs wURtj9k7JI7qsosTnRmMkeuaoTBEC8RWMLijMua+ZT5r40KDx6P/JK4IG2x/7pItlGHz eALv64emnWMQmE3u6plp+jNo5AaG7HI4P39HBMuagnC3nG2/CTR4UtLuJKK3LwUN1IBo 669zu5SwWh1IA2+T/jGOYkZj5ZvfNK6wawQf3z6AQSGMZ76GtpdeQFGrd7vwcMlHfEgR nppA== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:list-unsubscribe:list-subscribe:list-id:precedence :subject:date:from:dkim-signature; bh=MkHgfdlubDIZc6YiSfjHe4pC8mX8qCcfR0RdSpyweMY=; fh=7S2INQxKjC7DMzKpLuWlOs87+kkrdeQW2DPiCn2r8t8=; b=aTS8kDI1rwUVrp0zJ4etDQGwMVuztzI3VfSrONH99W3mmNHX3wPr0ovhUHZxobh+b+ b/F7SRi+mZRztYxOPq0/CL2h8ZqE169zrYayIjFaOIJxgrRviHooFzLZQBiuWF4Fq1K0 d88HDCRV1wKwgr8eYFK9aWON1fBoRT4SoCgs2w9baYP/IwSYVu3vFUlemtZzrjJC+lEQ gUe1dSHk49UvIWfqgwzRmFCARk2m7DnwUQAZXQb3KJsEY+jbLOCI6EsKK38BSquDd+UK p6qj0PCpnAMEO9SSb6zOflIZLajWGbrAoXUAOmFGDxtrAxzqRjbQ0u5u+uKlpGfzoddC 33JQ==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b="e9x6N/HH"; arc=pass (i=1 spf=pass spfdomain=linaro.org dkim=pass dkdomain=linaro.org dmarc=pass fromdomain=linaro.org); spf=pass (google.com: domain of linux-kernel+bounces-81289-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-81289-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [2604:1380:4601:e00::3]) by mx.google.com with ESMTPS id lo25-20020a170906fa1900b00a4338791320si1063945ejb.764.2024.02.26.03.47.49 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Feb 2024 03:47:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-81289-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b="e9x6N/HH"; arc=pass (i=1 spf=pass spfdomain=linaro.org dkim=pass dkdomain=linaro.org dmarc=pass fromdomain=linaro.org); spf=pass (google.com: domain of linux-kernel+bounces-81289-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) smtp.mailfrom="linux-kernel+bounces-81289-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 31CAA1F2B163 for <ouuuleilei@gmail.com>; Mon, 26 Feb 2024 11:39:53 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 471C94D5A0; Mon, 26 Feb 2024 11:38:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="e9x6N/HH" Received: from mail-qk1-f174.google.com (mail-qk1-f174.google.com [209.85.222.174]) (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 EAB714CDE0 for <linux-kernel@vger.kernel.org>; Mon, 26 Feb 2024 11:38:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.222.174 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708947483; cv=none; b=GtsBis4J1Qb/3M26+YaOC3cdMsWlEqZ7EFZEvZndz8R83vqqy/HX5Hs3tQm+IQYx/hLjasTPHW9v7Oyk/Wq/3o8xHwZb1j1gyI6nW2LvJtNrWgryM7aWoeljAi3nVgxwT1SbwKPsWI/vN+tq9mG2uWunWFaSwAu8RqG4pF0NP8c= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708947483; c=relaxed/simple; bh=nHkU+s/hofqacaD6DKpKZMYQycR1YXGCRfONZEYjupQ=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=NyEMV/1n85rBFiCPgU6ZfL+lPmErfGku28gJky2BsjcjEHyBcnbAg5FEER9wH+wdOy5FQ/jykQ63yN8TbFISX28cy7XTG1uYQsJlpdYlVEswHFWxia8Bgd4kyGwBrL3PjQcfMqbIfbqJUkqSPBflXjyK3ocPX2MaIxr0QIoE9q4= 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=e9x6N/HH; arc=none smtp.client-ip=209.85.222.174 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 Received: by mail-qk1-f174.google.com with SMTP id af79cd13be357-787c900af02so110414485a.1 for <linux-kernel@vger.kernel.org>; Mon, 26 Feb 2024 03:38:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1708947480; x=1709552280; 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=MkHgfdlubDIZc6YiSfjHe4pC8mX8qCcfR0RdSpyweMY=; b=e9x6N/HHyDD5xyrpxDu4iUy9+q/O1p16MCtq8/zd2tu4MyanxwBuojWO2/7NogTpdH QuGZuVhhCMn4GstDH31mW70Ort3c0doBWVS0fTbnExhUX176YHI5h2XZ21R1lJLnUA0T UsV/ZqtyZ40PIXgdFUCcigRdPsYSt/6/KWjRTeiMxXwdBIBJQqKuLFdB+lJdQUp58Qkt ynAxbjm0J0O3e2Px3Uy70cnyRnIP+nqWqlwLGm/w6Ayfmi62Xqu8Wibtysj1tp11GTq7 8WMRUsVI6kLhE8dp7BQt+VF3cPmwslhNHy5vZ6S8GzdZWAf6PQyysUQnzST8TVVXi70/ 33MA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708947480; x=1709552280; 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=MkHgfdlubDIZc6YiSfjHe4pC8mX8qCcfR0RdSpyweMY=; b=bBzGmnHiF0V1uIVTC7BpRvImHCB1/AmOKCUo61BUh8Y2xrU5tHPo9Ua8UAuAhMMR2A jYegPhyL1u/LUvC8aXA36nWCIM2LM52TVPiFeDaRZQPW5WWwOacMsnlZmCwZDiI2j8LI hoLu7RX/hCUJhDMXw5iuHT7GmOOFHkJbVhByn7rGTBMgKPRaN/bQoRGGp3DVfvmgymSY s95hls86fhxtovb5SpI3JlSMbZCX7v3w+69nglVXri7c/vuKWgkW2xcupK88LBt9hRz8 aaCZBJzcw2rzg1J1harKjnFwe66/VjwXwc4/N/c5c94i53NMZ3bFEmsM+OcBmBTb3KMP 1spw== X-Forwarded-Encrypted: i=1; AJvYcCW/eHLpVA6F8X7Xh3o7ZEmSULlyVn9ORRS4SykREqrlt6OnCNDHm7i1kYGeBpGy0hfCyHDaKyuvMqaNgK2/xsfJXCouF7KtJiTOlcgB X-Gm-Message-State: AOJu0YzR57ijfGkxGbO+d32UOF3nLq7crMrQQmNv46y9mRPgPaGG2gUc 94VhxhzQFQZGkkNioL509tkOU04mqMiPNF7G5hNghrbQ/sK5WKuoybEL6qcF7LLHlv+vKQ6CTH8 = X-Received: by 2002:a05:620a:a09:b0:787:bd2f:e30d with SMTP id i9-20020a05620a0a0900b00787bd2fe30dmr12995662qka.19.1708947479856; Mon, 26 Feb 2024 03:37:59 -0800 (PST) Received: from [127.0.1.1] ([117.202.184.81]) by smtp.gmail.com with ESMTPSA id q15-20020a05620a0c8f00b007878babb96asm2341842qki.94.2024.02.26.03.37.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 Feb 2024 03:37:59 -0800 (PST) From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Date: Mon, 26 Feb 2024 17:07:27 +0530 Subject: [PATCH v3 2/5] PCI: dwc: Skip finding eDMA channels count if glue drivers have passed them Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20240226-dw-hdma-v3-2-cfcb8171fc24@linaro.org> References: <20240226-dw-hdma-v3-0-cfcb8171fc24@linaro.org> In-Reply-To: <20240226-dw-hdma-v3-0-cfcb8171fc24@linaro.org> To: Jingoo Han <jingoohan1@gmail.com>, Gustavo Pimentel <gustavo.pimentel@synopsys.com>, 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>, Marek Vasut <marek.vasut+renesas@gmail.com>, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>, Kishon Vijay Abraham I <kishon@kernel.org> Cc: Serge Semin <fancer.lancer@gmail.com>, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-arm-msm@vger.kernel.org, mhi@lists.linux.dev, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>, Siddharth Vadapalli <s-vadapalli@ti.com> X-Mailer: b4 0.12.4 X-Developer-Signature: v=1; a=openpgp-sha256; l=1982; i=manivannan.sadhasivam@linaro.org; h=from:subject:message-id; bh=nHkU+s/hofqacaD6DKpKZMYQycR1YXGCRfONZEYjupQ=; b=owEBbQGS/pANAwAKAVWfEeb+kc71AcsmYgBl3HgF/rASIKBtYR+ctXJQGfZnVs/PLtISbbelP 4j25M3FMvCJATMEAAEKAB0WIQRnpUMqgUjL2KRYJ5dVnxHm/pHO9QUCZdx4BQAKCRBVnxHm/pHO 9RAEB/0f+JgPn81KF1QoksP35fTFnUOaahH5T1Ggq54hdmm83UwdirBwQRPYJoo83ttnBr0g+tG mBgVdFOBCPSBv1ccH+TL3DJy1AEXKyLh3cZ8Kdaf79ZoEGpjrjCDQvj2qcRfD5yEeXjMgvjREpJ nmB3ENkWrQsx71qOeeXlq+zoAswDcGaM27iZr01yxP2xOKJyudftYZoINsGwVzqcAqf/WZ7RUtd /57hoXRwB8kDATQEWBFlAMlchTcTixFAgyeUw8iFxaJoRzziT/eyIFaLDEoHPU1cz7hlnx3bD1K P4UN5LN+i9ZoS+zGOWUkRM6SnJR1Ttljmp7hBiKlzj4fJogF X-Developer-Key: i=manivannan.sadhasivam@linaro.org; a=openpgp; fpr=C668AEC3C3188E4C611465E7488550E901166008 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791961930927289261 X-GMAIL-MSGID: 1791961930927289261 |
Series |
PCI: dwc: Add support for integrating HDMA with DWC EP driver
|
|
Commit Message
Manivannan Sadhasivam
Feb. 26, 2024, 11:37 a.m. UTC
In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way the drivers can auto detect the number of read/write channels as like its predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA have to pass the channels count during probe. To accommodate that, let's skip finding the channels if the channels count were already passed by glue drivers. If the channels count passed were wrong in any form, then the existing sanity check will catch it. Suggested-by: Serge Semin <fancer.lancer@gmail.com> Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
Comments
On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote: > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way > the drivers can auto detect the number of read/write channels as like its > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA > have to pass the channels count during probe. > > To accommodate that, let's skip finding the channels if the channels count > were already passed by glue drivers. If the channels count passed were > wrong in any form, then the existing sanity check will catch it. > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 193fcd86cf93..ce273c3c5421 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci) > { > u32 val; > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > - else > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > - > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { Are you sure that the partly initialized case should be considered as a request for the auto-detection? IMO &&-ing here and letting the sanity check to fail further would be more correct since thus the developer would know about improper initialized data. -Serge(y) > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > + else > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + } > > /* Sanity check the channels count if the mapping was incorrect */ > if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH || > > -- > 2.25.1 >
On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote: > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote: > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way > > the drivers can auto detect the number of read/write channels as like its > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA > > have to pass the channels count during probe. > > > > To accommodate that, let's skip finding the channels if the channels count > > were already passed by glue drivers. If the channels count passed were > > wrong in any form, then the existing sanity check will catch it. > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index 193fcd86cf93..ce273c3c5421 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci) > > { > > u32 val; > > > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > - else > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > - > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { > > Are you sure that the partly initialized case should be considered as > a request for the auto-detection? IMO &&-ing here and letting the > sanity check to fail further would be more correct since thus the > developer would know about improper initialized data. > We already have the check below. So the partly initialized case will fail anyway. - Mani > -Serge(y) > > > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > + else > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > + > > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > + } > > > > /* Sanity check the channels count if the mapping was incorrect */ > > if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH || > > > > -- > > 2.25.1 > >
On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote: > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way > the drivers can auto detect the number of read/write channels as like its > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA > have to pass the channels count during probe. > > To accommodate that, let's skip finding the channels if the channels count > were already passed by glue drivers. If the channels count passed were > wrong in any form, then the existing sanity check will catch it. > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> Reviewed-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 193fcd86cf93..ce273c3c5421 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci) > { > u32 val; > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > - else > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > - > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > + else > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + } > > /* Sanity check the channels count if the mapping was incorrect */ > if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH || > > -- > 2.25.1 >
On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote: > On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote: > > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote: > > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way > > > the drivers can auto detect the number of read/write channels as like its > > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA > > > have to pass the channels count during probe. > > > > > > To accommodate that, let's skip finding the channels if the channels count > > > were already passed by glue drivers. If the channels count passed were > > > wrong in any form, then the existing sanity check will catch it. > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++------- > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > index 193fcd86cf93..ce273c3c5421 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci) > > > { > > > u32 val; > > > > > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > - else > > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > - > > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > > > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { > > > > Are you sure that the partly initialized case should be considered as > > a request for the auto-detection? IMO &&-ing here and letting the > > sanity check to fail further would be more correct since thus the > > developer would know about improper initialized data. > > > > We already have the check below. So the partly initialized case will fail > anyway. Not really. If the partly initialized case activates the auto-detection procedure it will override both non-initialized and _initialized_ number of channels with the values retrieved from the hardware, which the glue driver has been willing not to use. This prone to undefined behavior depending on the reasons of skipping the auto-detection procedure. For instance, assume the DMA_CTRL register reports an invalid number of read channels. A glue driver by mistake or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't solve the problem since the auto-detection will be proceeded due to the pci->edma.ll_wr_cnt field being left uninitialized. So to speak it would be better to implement a strictly determined case for activating the auto-detection procedure: both number of channels aren't initialized; if only one field is initialized then report an error. Alternatively we can have the auto-detection executed on the per-channel basis: + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + + if (!pci->edma.ll_wr_cnt) + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + + if (!pci->edma.ll_rd_cnt) + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + } -Serge(y) > > - Mani > > > -Serge(y) > > > > > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > + else > > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > + > > > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > > + } > > > > > > /* Sanity check the channels count if the mapping was incorrect */ > > > if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH || > > > > > > -- > > > 2.25.1 > > > > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 12:32:44AM +0300, Serge Semin wrote: > On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote: > > On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote: > > > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote: > > > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way > > > > the drivers can auto detect the number of read/write channels as like its > > > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA > > > > have to pass the channels count during probe. > > > > > > > > To accommodate that, let's skip finding the channels if the channels count > > > > were already passed by glue drivers. If the channels count passed were > > > > wrong in any form, then the existing sanity check will catch it. > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- > > > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++------- > > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > index 193fcd86cf93..ce273c3c5421 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci) > > > > { > > > > u32 val; > > > > > > > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > > - else > > > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > > - > > > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > > > > > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { > > > > > > Are you sure that the partly initialized case should be considered as > > > a request for the auto-detection? IMO &&-ing here and letting the > > > sanity check to fail further would be more correct since thus the > > > developer would know about improper initialized data. > > > > > > > We already have the check below. So the partly initialized case will fail > > anyway. > > Not really. If the partly initialized case activates the > auto-detection procedure it will override both non-initialized and > _initialized_ number of channels with the values retrieved from the > hardware, which the glue driver has been willing not to use. This > prone to undefined behavior depending on the reasons of skipping the > auto-detection procedure. For instance, assume the DMA_CTRL register > reports an invalid number of read channels. A glue driver by mistake > or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't > solve the problem since the auto-detection will be proceeded due to > the pci->edma.ll_wr_cnt field being left uninitialized. > > So to speak it would be better to implement a strictly determined case > for activating the auto-detection procedure: both number of channels > aren't initialized; if only one field is initialized then report an > error. > > Alternatively we can have the auto-detection executed on the > per-channel basis: > > + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + if (!pci->edma.ll_wr_cnt) > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + > + if (!pci->edma.ll_rd_cnt) > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + } > Hmm, in this case there is no need to check for uninitialized channels count: /* * Autodetect the read/write channels count only for non-HDMA platforms. * HDMA platforms doesn't support autodetect, so the glue drivers should've * passed the valid count already. If not, the below sanity check will * catch it. */ if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); } /* Sanity check */ - Mani
On Tue, Feb 27, 2024 at 02:12:04PM +0530, Manivannan Sadhasivam wrote: > On Tue, Feb 27, 2024 at 12:32:44AM +0300, Serge Semin wrote: > > On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote: > > > On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote: > > > > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote: > > > > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way > > > > > the drivers can auto detect the number of read/write channels as like its > > > > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA > > > > > have to pass the channels count during probe. > > > > > > > > > > To accommodate that, let's skip finding the channels if the channels count > > > > > were already passed by glue drivers. If the channels count passed were > > > > > wrong in any form, then the existing sanity check will catch it. > > > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > --- > > > > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++------- > > > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > > index 193fcd86cf93..ce273c3c5421 100644 > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci) > > > > > { > > > > > u32 val; > > > > > > > > > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > > > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > > > - else > > > > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > > > - > > > > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > > > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > > > > > > > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { > > > > > > > > Are you sure that the partly initialized case should be considered as > > > > a request for the auto-detection? IMO &&-ing here and letting the > > > > sanity check to fail further would be more correct since thus the > > > > developer would know about improper initialized data. > > > > > > > > > > We already have the check below. So the partly initialized case will fail > > > anyway. > > > > Not really. If the partly initialized case activates the > > auto-detection procedure it will override both non-initialized and > > _initialized_ number of channels with the values retrieved from the > > hardware, which the glue driver has been willing not to use. This > > prone to undefined behavior depending on the reasons of skipping the > > auto-detection procedure. For instance, assume the DMA_CTRL register > > reports an invalid number of read channels. A glue driver by mistake > > or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't > > solve the problem since the auto-detection will be proceeded due to > > the pci->edma.ll_wr_cnt field being left uninitialized. > > > > So to speak it would be better to implement a strictly determined case > > for activating the auto-detection procedure: both number of channels > > aren't initialized; if only one field is initialized then report an > > error. > > > > Alternatively we can have the auto-detection executed on the > > per-channel basis: > > > > + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > + > > + if (!pci->edma.ll_wr_cnt) > > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > + > > + if (!pci->edma.ll_rd_cnt) > > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > + } > > > > Hmm, in this case there is no need to check for uninitialized channels count: > > /* > * Autodetect the read/write channels count only for non-HDMA platforms. > * HDMA platforms doesn't support autodetect, so the glue drivers should've > * passed the valid count already. If not, the below sanity check will > * catch it. > */ > if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > } > > /* Sanity check */ That is another possible implementation. Let's sum all of them up: 1. Channel fields-base conditional statement: + if (!pci->edma.ll_wr_cnt && !pci->edma.ll_rd_cnt) { + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + } pros: NoF channels override support for all IP-cores; simple. cons: incompatible with HDMA, but can be taken by mistake/bug; no partial NoF channels pre-initialization. 2. Channel fields-base conditional statement with logical OR operator #1: + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + } pros: NoF channels override support for all IP-cores; simple. cons: incompatible with HDMA, but can be taken by mistake/bug; no partial NoF channels pre-initialization; silently overrides the partial NoF channels case. 3. Channel fields-base conditional statement with logical OR operator #2: + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + + if (!pci->edma.ll_wr_cnt) + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + + if (!pci->edma.ll_rd_cnt) + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + } pros: NoF channels override support for all IP-cores; partial NoF channels pre-initialization support. cons: incompatible with HDMA, but can be taken by mistake/bug; more complex (and actually looking a bit clumsy due to two conditional statements over the same fields). 4. Unconditional auto-detection: + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + + if (!pci->edma.ll_wr_cnt) + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + + if (!pci->edma.ll_rd_cnt) + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); pros: NoF channels override support for all IP-cores; partial NoF channels pre-initialization support; simple. cons: incompatible with HDMA, but will be executed for it anyway so the NoF channels fields will be overridden with the Channel#0.prefetch CSR data if haven't been pre-initialized; 5. Mapping format-based conditional statement: + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + } pros: free of being executed for HDMA IP-core, simple cons: no NoF channels override support for non-HDMA IP-cores. 6. Mapping format-based conditional statement with partial NoF channels override: + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + + if (!pci->edma.ll_wr_cnt) + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + + if (!pci->edma.ll_rd_cnt) + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + } pros: free of being executed for HDMA IP-core; NoF channels override support for all IP-cores. cons: more complex. Looking at all of that I'd say that options 5 and 6 seems better to me now since they prohibit the auto-detection for HDMA IP-cores which have the Channel#0.prefetch CSR at the 0x8 offset. I don't have strong opinion which of those two to choose. If you think simplicity is preferable, then option 2 will be enough. If you wish to have the NoF channels override supported for all IP-cores, then option 3 will work for it. -Serge(y) > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 03:21:00PM +0300, Serge Semin wrote: > On Tue, Feb 27, 2024 at 02:12:04PM +0530, Manivannan Sadhasivam wrote: > > On Tue, Feb 27, 2024 at 12:32:44AM +0300, Serge Semin wrote: > > > On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote: > > > > On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote: > > > > > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote: > > > > > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way > > > > > > the drivers can auto detect the number of read/write channels as like its > > > > > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA > > > > > > have to pass the channels count during probe. > > > > > > > > > > > > To accommodate that, let's skip finding the channels if the channels count > > > > > > were already passed by glue drivers. If the channels count passed were > > > > > > wrong in any form, then the existing sanity check will catch it. > > > > > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > --- > > > > > > drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++------- > > > > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > index 193fcd86cf93..ce273c3c5421 100644 > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci) > > > > > > { > > > > > > u32 val; > > > > > > > > > > > > - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) > > > > > > - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); > > > > > > - else > > > > > > - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > > > > - > > > > > > - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > > > > > - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > > > > > > > > > > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { > > > > > > > > > > Are you sure that the partly initialized case should be considered as > > > > > a request for the auto-detection? IMO &&-ing here and letting the > > > > > sanity check to fail further would be more correct since thus the > > > > > developer would know about improper initialized data. > > > > > > > > > > > > > We already have the check below. So the partly initialized case will fail > > > > anyway. > > > > > > Not really. If the partly initialized case activates the > > > auto-detection procedure it will override both non-initialized and > > > _initialized_ number of channels with the values retrieved from the > > > hardware, which the glue driver has been willing not to use. This > > > prone to undefined behavior depending on the reasons of skipping the > > > auto-detection procedure. For instance, assume the DMA_CTRL register > > > reports an invalid number of read channels. A glue driver by mistake > > > or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't > > > solve the problem since the auto-detection will be proceeded due to > > > the pci->edma.ll_wr_cnt field being left uninitialized. > > > > > > So to speak it would be better to implement a strictly determined case > > > for activating the auto-detection procedure: both number of channels > > > aren't initialized; if only one field is initialized then report an > > > error. > > > > > > Alternatively we can have the auto-detection executed on the > > > per-channel basis: > > > > > > + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { > > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > + > > > + if (!pci->edma.ll_wr_cnt) > > > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > > + > > > + if (!pci->edma.ll_rd_cnt) > > > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > > + } > > > > > > > Hmm, in this case there is no need to check for uninitialized channels count: > > > > /* > > * Autodetect the read/write channels count only for non-HDMA platforms. > > * HDMA platforms doesn't support autodetect, so the glue drivers should've > > * passed the valid count already. If not, the below sanity check will > > * catch it. > > */ > > if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { > > val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > > > > pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > > pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > > } > > > > /* Sanity check */ > > That is another possible implementation. Let's sum all of them up: > > 1. Channel fields-base conditional statement: > + if (!pci->edma.ll_wr_cnt && !pci->edma.ll_rd_cnt) { > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + } > pros: NoF channels override support for all IP-cores; simple. > cons: incompatible with HDMA, but can be taken by mistake/bug; no > partial NoF channels pre-initialization. > > 2. Channel fields-base conditional statement with logical OR operator #1: > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + } > pros: NoF channels override support for all IP-cores; simple. > cons: incompatible with HDMA, but can be taken by mistake/bug; no > partial NoF channels pre-initialization; silently overrides the > partial NoF channels case. > > 3. Channel fields-base conditional statement with logical OR operator #2: > + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + if (!pci->edma.ll_wr_cnt) > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + > + if (!pci->edma.ll_rd_cnt) > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + } > pros: NoF channels override support for all IP-cores; partial NoF > channels pre-initialization support. > cons: incompatible with HDMA, but can be taken by mistake/bug; more > complex (and actually looking a bit clumsy due to two conditional > statements over the same fields). > > 4. Unconditional auto-detection: > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + if (!pci->edma.ll_wr_cnt) > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + > + if (!pci->edma.ll_rd_cnt) > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > pros: NoF channels override support for all IP-cores; partial NoF > channels pre-initialization support; simple. > cons: incompatible with HDMA, but will be executed for it anyway so > the NoF channels fields will be overridden with the Channel#0.prefetch > CSR data if haven't been pre-initialized; > > 5. Mapping format-based conditional statement: > + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + } > pros: free of being executed for HDMA IP-core, simple > cons: no NoF channels override support for non-HDMA IP-cores. > Is it possible for the non-HDMA IPs to override the channels count? Atleast any such IPs supported in mainline now? If not, then I'd like to go with this approach. Because, this makes it explicit that override is only supported for HDMA IPs and also simplifies the logic. > 6. Mapping format-based conditional statement with partial NoF channels override: > + if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) { > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); > + > + if (!pci->edma.ll_wr_cnt) > + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); > + > + if (!pci->edma.ll_rd_cnt) > + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); > + } > pros: free of being executed for HDMA IP-core; NoF channels override > support for all IP-cores. > cons: more complex. > > > Looking at all of that I'd say that options 5 and 6 seems better to me > now since they prohibit the auto-detection for HDMA IP-cores which > have the Channel#0.prefetch CSR at the 0x8 offset. I don't have strong > opinion which of those two to choose. If you think simplicity is > preferable, then option 2 will be enough. If you wish to have the NoF > channels override supported for all IP-cores, then option 3 will work > for it. > Thanks for such elaborative comparision :) - Mani
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 193fcd86cf93..ce273c3c5421 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci) { u32 val; - if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) - val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); - else - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); - - pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); - pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) { + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY) + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); + else + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + } /* Sanity check the channels count if the mapping was incorrect */ if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||