Message ID | 20240210-topic-8280_pcie-v2-2-1cef4b606883@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-60488-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:50ea:b0:106:860b:bbdd with SMTP id r10csp1567429dyd; Sat, 10 Feb 2024 09:11:12 -0800 (PST) X-Google-Smtp-Source: AGHT+IEk6NOGPOoZ67sG8F9swzVVCN1DBws3eqbcnZKv4qR5u1Fj4HXo46cBqlvnq3wjnelgTO8C X-Received: by 2002:ac8:5716:0:b0:42c:68c8:fbb5 with SMTP id 22-20020ac85716000000b0042c68c8fbb5mr2185216qtw.40.1707585071882; Sat, 10 Feb 2024 09:11:11 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1707585071; cv=pass; d=google.com; s=arc-20160816; b=amvfvBXznwpAIWXg3QhhsPt4n3VmoWRfYU6NsJhnHrnfvGVd5f3eZ/YAX7QJtPSaip 4+reUC8fsqkTvfiRhyi4N9g7eyZ3/CCt2lU3XLvxt3j3F8LE13Q2JtxVdZs0kCELRk0L 1trf2X/vrm601+2ziwmGZPHhyc1ikwiz86YqSpJrNciAFOUlgGWZ/xe2+VzIyV/SMwPD AYeNlzTki/l/T3Cn4C39iwPJ9QwI7rIi5uZjsvgxgcw8g4EGP4Vto5Mrk7DZuwBrb1Mj S1H0Y9Kr0JTOOksxT3KqBmhot2VwJ4bQqVoyY7vsi3X+pR0iU83IubwxSgOdvHpDBt6I q35g== 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=e5eEDZ2SBOr178JyoKUFrLLup1vdc34CTGoZZtEa27Q=; fh=6CPB2h9Z29psT4mW0aN/qLOGQ6PlHfmN6dhyKYU9PNU=; b=bR2KzFCvx4M23p8V7W6O0oRF+5QjHSBX+2oHtYKhACA9NiUUvCjWLxeMnEi2cWvXK9 JjApXyXPR4OAd9fvl4w1PlMWQ+ub8RhkaeDFmnouqi1QGf1aMSbPWeEzRJRYNX0WYzE8 gzq8XQAI8sXF4/flabEdE3LFh1sG2J6KkxkBjd6I0XVN6fvQaWcxldm0bUQ91Agus0RF ph4406E5onupVPr6Anj5DqOt8WxobVzcuv+UnUTr14b1Xn+UQKElNH2llJqqWTdmOb6a DqmKKixdSfARGfwBE4W1WVr3wQCGi/eRfPAPb6kMWcU8lCef19ufvCEmUlblj6VsuaCp yiNQ==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b="HK+XV/IL"; 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-60488-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-60488-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org X-Forwarded-Encrypted: i=2; AJvYcCUfczgfU6z2+5aTfWwiTsDEEzGVbK+DI5PRu9bDOuongkqcYsbuUbVyu2/jgFxVLZb4N5K/eiOIEUqmDa1SJMOMDwMQXA== Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id u31-20020a05622a199f00b0042c61cb0653si2935648qtc.194.2024.02.10.09.11.11 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 10 Feb 2024 09:11:11 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-60488-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b="HK+XV/IL"; 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-60488-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-60488-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 ny.mirrors.kernel.org (Postfix) with ESMTPS id AA37B1C224C0 for <ouuuleilei@gmail.com>; Sat, 10 Feb 2024 17:11:11 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 6610A5D8E4; Sat, 10 Feb 2024 17:10:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="HK+XV/IL" Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) (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 E101B5CDFA for <linux-kernel@vger.kernel.org>; Sat, 10 Feb 2024 17:10:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.48 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1707585018; cv=none; b=m5Si7Ml9Zc9dP74V0HVnEnRM6/dBQ60Cwm2GtPyvtymPj++/vtsLSWZtrojp/gjQt84we8MibiEUb8An+KA3OUkhxCQWwHcP5Ac1OHmM8wzNfEWumqOh5oXiDfEg+GeuGtHtjwnm50gik+dGxIEAfQuzRSHz3gQa/brQ/7DpW/Y= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1707585018; c=relaxed/simple; bh=4Or5VI5ZnZ1ID78niRW0J52lNzSsB1zjEV1fVdr25BM=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=WJA75qN0ivwwq8wk3Am6S7YXZRA22cEFnLxX8oDRoQRm0IO9CNfQc//cyIWQI1wS1WJwfDDvB4djvj2kRVoR5PtPIwvdzhEcihzuLodnY89bPbviU2Xtg3bwFDbFjEL5Xi54KqE1tuT9pYkNoqnmro5eLyO/AyBqKezGgyeR+SE= 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=HK+XV/IL; arc=none smtp.client-ip=209.85.218.48 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-ej1-f48.google.com with SMTP id a640c23a62f3a-a26f73732c5so260069566b.3 for <linux-kernel@vger.kernel.org>; Sat, 10 Feb 2024 09:10:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1707585015; x=1708189815; 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=e5eEDZ2SBOr178JyoKUFrLLup1vdc34CTGoZZtEa27Q=; b=HK+XV/IL4rUBseoi2+eMZaQJEAO2rroKPxgKoCqhtIF10BVggwLPT6iTcYvfyYW6K5 Jy0gTjmJHhaLfEcC1GgLaD/ni4iNlGlBpHZ9wmTI2dGq3E4rA0WqXypo+AkQ30UXRegR auJ8T7U/BrO+SYEXOFUEXeGGbEX/RgZd3tvbAz21gkx9hiNR7g7jbPSQIf/hRdeZZl8a G1tj8qqPF4oa0DzS5NgJLj3yxDEL2F/+8odft+5FezoKGMyzzFSl4peRJfbG5cWt/rmQ +qXTcEtDOQ2Gl3swek04sfYe47e4O7VHkIC3kn/8KmcxHU48JauDMb2CluoYfHUZjpgZ Hzmw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707585015; x=1708189815; 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=e5eEDZ2SBOr178JyoKUFrLLup1vdc34CTGoZZtEa27Q=; b=LfVMsMEwjmQbgU6IOS6PMNUblgT7Kvt6Kw8pAXSH14JzPn2tMp4aPdi5uPEwg688Ih kV73VMJ2jZcSmvVBcwjoibjLgthGXbh+uijvhRtO+1XPrZkvQs6Q/vL7E7oxfJHwVbHc s4mhOYGdPOnpCHJbpvc12vruUxISXAc4CIfSHWSVXhMlZwdC7oTsPrl8fYxbKvfc4wua 7pgcq5NKzQC1cL87snQ8fYFl6zJnm9iBA8C2iwvYzjLmEZBVQY/HbJnymz/KpFaQyXzT TW0qtcWN4okIvClvfAh9DrRBaJVsY9/sgwiKEmW5smStDh7LQ2edd51xZFdJDhc4T7Ku bRqg== X-Gm-Message-State: AOJu0YxLyjDeXR+riByWrCmHNYlPkgYAKh1ORgLNp9Cp6C58LGfio42m XnU8WVdePD+1Ce2/5X97WDX8VR83ldjrBfjT2IwPKp+6Jb3Vh/Y79cJWwBNgiDc= X-Received: by 2002:a17:906:af42:b0:a3b:d9bd:be7e with SMTP id ly2-20020a170906af4200b00a3bd9bdbe7emr1514696ejb.76.1707585015036; Sat, 10 Feb 2024 09:10:15 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCVnMvj5sl6aewT1D7//JM+3l1Jkpvwv0WkYZKzK5xSsmO3amS7nMfzjqGRrebRoT7aiUuPuijxYQre3qmY1JhLOWU1O9Gp6H9YMkVoczEMZUZnG+8MBVU+FTiQUtK3t8djI0vb6FKPEeXkplXXRikKAZFutdREmxGrBjqjIscBkzaFoWs9f0MqyCHaNyI1zYXDn41JzQqW0i9V3M3nZaMTgdkVVKDwZl4gsZJulSM87Sc/cXrjFrG1SuhXuWmfw4pOE5Um0ZO+1Q9W8RTZmdenBxDP2oGX1YAggDVDtVtZKC0cwpK2Sxw2YTBhoOA5iEnC1MNy/bHLfpFdUkuk9qirJ7GxUm9VzGb1lzWALQYN5na6TUw6N+kuuiOd3TVyeCA== Received: from [127.0.1.1] (abyl12.neoplus.adsl.tpnet.pl. [83.9.31.12]) by smtp.gmail.com with ESMTPSA id lg25-20020a170907181900b00a3c1e1ca800sm973242ejc.11.2024.02.10.09.10.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 10 Feb 2024 09:10:14 -0800 (PST) From: Konrad Dybcio <konrad.dybcio@linaro.org> Date: Sat, 10 Feb 2024 18:10:06 +0100 Subject: [PATCH v2 2/3] PCI: qcom: Read back PARF_LTSSM register 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: <20240210-topic-8280_pcie-v2-2-1cef4b606883@linaro.org> References: <20240210-topic-8280_pcie-v2-0-1cef4b606883@linaro.org> In-Reply-To: <20240210-topic-8280_pcie-v2-0-1cef4b606883@linaro.org> To: Bjorn Andersson <andersson@kernel.org>, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>, 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>, Philipp Zabel <p.zabel@pengutronix.de> Cc: linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org> X-Mailer: b4 0.13-dev-0438c X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1790532724574339778 X-GMAIL-MSGID: 1790532724574339778 |
Series |
Qualcomm PCIe RC shutdown & reinit
|
|
Commit Message
Konrad Dybcio
Feb. 10, 2024, 5:10 p.m. UTC
To ensure write completion, read the PARF_LTSSM register after setting the LTSSM enable bit before polling for "link up". Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 1 + 1 file changed, 1 insertion(+)
Comments
Maybe include the reason in the subject? "Read back" is literally what the diff says. On Sat, Feb 10, 2024 at 06:10:06PM +0100, Konrad Dybcio wrote: > To ensure write completion, read the PARF_LTSSM register after setting > the LTSSM enable bit before polling for "link up". The write will obviously complete *some* time; I assume the point is that it's important for it to complete before some other event, and it would be nice to know why that's important. > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c > index cbde9effa352..6a469ed213ce 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom.c > +++ b/drivers/pci/controller/dwc/pcie-qcom.c > @@ -539,6 +539,7 @@ static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie) > val = readl(pcie->parf + PARF_LTSSM); > val |= LTSSM_EN; > writel(val, pcie->parf + PARF_LTSSM); > + readl(pcie->parf + PARF_LTSSM); > } > > static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie) > > -- > 2.40.1 >
On 12.02.2024 22:17, Bjorn Helgaas wrote: > Maybe include the reason in the subject? "Read back" is literally > what the diff says. > > On Sat, Feb 10, 2024 at 06:10:06PM +0100, Konrad Dybcio wrote: >> To ensure write completion, read the PARF_LTSSM register after setting >> the LTSSM enable bit before polling for "link up". > > The write will obviously complete *some* time; I assume the point is > that it's important for it to complete before some other event, and it > would be nice to know why that's important. Right, that's very much meaningful on non-total-store-ordering architectures, like arm64, where the CPU receives a store instruction, but that does not necessarily impact the memory/MMIO state immediately. Konrad
On Wed, Feb 14, 2024 at 10:35:16PM +0100, Konrad Dybcio wrote: > On 12.02.2024 22:17, Bjorn Helgaas wrote: > > Maybe include the reason in the subject? "Read back" is literally > > what the diff says. > > > > On Sat, Feb 10, 2024 at 06:10:06PM +0100, Konrad Dybcio wrote: > >> To ensure write completion, read the PARF_LTSSM register after setting > >> the LTSSM enable bit before polling for "link up". > > > > The write will obviously complete *some* time; I assume the point is > > that it's important for it to complete before some other event, and it > > would be nice to know why that's important. > > Right, that's very much meaningful on non-total-store-ordering > architectures, like arm64, where the CPU receives a store instruction, > but that does not necessarily impact the memory/MMIO state immediately. I was hinting that maybe we could say what the other event is, or what problem this solves? E.g., maybe it's as simple as "there's no point in polling for link up until after the PARF_LTSSM store completes." But while the read of PARF_LTSSM might reduce the number of "is the link up" polls, it probably wouldn't speed anything up otherwise, so I suspect there's an actual functional reason for this patch, and that's what I'm getting at. Bjorn
On 14.02.2024 23:28, Bjorn Helgaas wrote: > On Wed, Feb 14, 2024 at 10:35:16PM +0100, Konrad Dybcio wrote: >> On 12.02.2024 22:17, Bjorn Helgaas wrote: >>> Maybe include the reason in the subject? "Read back" is literally >>> what the diff says. >>> >>> On Sat, Feb 10, 2024 at 06:10:06PM +0100, Konrad Dybcio wrote: >>>> To ensure write completion, read the PARF_LTSSM register after setting >>>> the LTSSM enable bit before polling for "link up". >>> >>> The write will obviously complete *some* time; I assume the point is >>> that it's important for it to complete before some other event, and it >>> would be nice to know why that's important. >> >> Right, that's very much meaningful on non-total-store-ordering >> architectures, like arm64, where the CPU receives a store instruction, >> but that does not necessarily impact the memory/MMIO state immediately. > > I was hinting that maybe we could say what the other event is, or what > problem this solves? E.g., maybe it's as simple as "there's no point > in polling for link up until after the PARF_LTSSM store completes." > > But while the read of PARF_LTSSM might reduce the number of "is the > link up" polls, it probably wouldn't speed anything up otherwise, so I > suspect there's an actual functional reason for this patch, and that's > what I'm getting at. So, the register containing the "enable switch" (PARF_LTSSM) can (due to the armv8 memory model) be "written" but not "change the value of memory/mmio from the perspective of other (non-CPU) memory-readers (such as the MMIO-mapped PCI controller itself)". In that case, the CPU will happily continue calling qcom_pcie_link_up() in a loop, waiting for the PCIe controller to bring the link up, however the PCIE controller may have never received the PARF_LTSSM "enable link" write by the time we decide to time out on checking the link status. It may also never happen for you, but that's exactly like a classic race condition, where it may simply not manifest due to the code around the problematic lines hiding it. It may also only manifest on certain CPU cores that try to be smarter than you and keep reordering/delaying instructions if they don't seem immediately necessary. Konrad
On Thu, Feb 15, 2024 at 11:21:45AM +0100, Konrad Dybcio wrote: > On 14.02.2024 23:28, Bjorn Helgaas wrote: > > On Wed, Feb 14, 2024 at 10:35:16PM +0100, Konrad Dybcio wrote: > >> On 12.02.2024 22:17, Bjorn Helgaas wrote: > >>> Maybe include the reason in the subject? "Read back" is literally > >>> what the diff says. > >>> > >>> On Sat, Feb 10, 2024 at 06:10:06PM +0100, Konrad Dybcio wrote: > >>>> To ensure write completion, read the PARF_LTSSM register after setting > >>>> the LTSSM enable bit before polling for "link up". > >>> > >>> The write will obviously complete *some* time; I assume the point is > >>> that it's important for it to complete before some other event, and it > >>> would be nice to know why that's important. > >> > >> Right, that's very much meaningful on non-total-store-ordering > >> architectures, like arm64, where the CPU receives a store instruction, > >> but that does not necessarily impact the memory/MMIO state immediately. > > > > I was hinting that maybe we could say what the other event is, or what > > problem this solves? E.g., maybe it's as simple as "there's no point > > in polling for link up until after the PARF_LTSSM store completes." > > > > But while the read of PARF_LTSSM might reduce the number of "is the > > link up" polls, it probably wouldn't speed anything up otherwise, so I > > suspect there's an actual functional reason for this patch, and that's > > what I'm getting at. > > So, the register containing the "enable switch" (PARF_LTSSM) can (due > to the armv8 memory model) be "written" but not "change the value of > memory/mmio from the perspective of other (non-CPU) memory-readers > (such as the MMIO-mapped PCI controller itself)". > > In that case, the CPU will happily continue calling qcom_pcie_link_up() > in a loop, waiting for the PCIe controller to bring the link up, however > the PCIE controller may have never received the PARF_LTSSM "enable link" > write by the time we decide to time out on checking the link status. > > It may also never happen for you, but that's exactly like a classic race > condition, where it may simply not manifest due to the code around the > problematic lines hiding it. It may also only manifest on certain CPU > cores that try to be smarter than you and keep reordering/delaying > instructions if they don't seem immediately necessary. Does this mean the register is mapped incorrectly, e.g., I see arm64 has many different kinds of mappings for cacheability, write-buffering, etc? Or, if it is already mapped correctly, are we confident that none of the *other* register writes need similar treatment? Is there a rule we can apply to know when the read-after-write is needed? Bjorn
On 15.02.2024 17:11, Bjorn Helgaas wrote: > On Thu, Feb 15, 2024 at 11:21:45AM +0100, Konrad Dybcio wrote: >> On 14.02.2024 23:28, Bjorn Helgaas wrote: >>> On Wed, Feb 14, 2024 at 10:35:16PM +0100, Konrad Dybcio wrote: >>>> On 12.02.2024 22:17, Bjorn Helgaas wrote: >>>>> Maybe include the reason in the subject? "Read back" is literally >>>>> what the diff says. >>>>> >>>>> On Sat, Feb 10, 2024 at 06:10:06PM +0100, Konrad Dybcio wrote: >>>>>> To ensure write completion, read the PARF_LTSSM register after setting >>>>>> the LTSSM enable bit before polling for "link up". >>>>> >>>>> The write will obviously complete *some* time; I assume the point is >>>>> that it's important for it to complete before some other event, and it >>>>> would be nice to know why that's important. >>>> >>>> Right, that's very much meaningful on non-total-store-ordering >>>> architectures, like arm64, where the CPU receives a store instruction, >>>> but that does not necessarily impact the memory/MMIO state immediately. >>> >>> I was hinting that maybe we could say what the other event is, or what >>> problem this solves? E.g., maybe it's as simple as "there's no point >>> in polling for link up until after the PARF_LTSSM store completes." >>> >>> But while the read of PARF_LTSSM might reduce the number of "is the >>> link up" polls, it probably wouldn't speed anything up otherwise, so I >>> suspect there's an actual functional reason for this patch, and that's >>> what I'm getting at. >> >> So, the register containing the "enable switch" (PARF_LTSSM) can (due >> to the armv8 memory model) be "written" but not "change the value of >> memory/mmio from the perspective of other (non-CPU) memory-readers >> (such as the MMIO-mapped PCI controller itself)". >> >> In that case, the CPU will happily continue calling qcom_pcie_link_up() >> in a loop, waiting for the PCIe controller to bring the link up, however >> the PCIE controller may have never received the PARF_LTSSM "enable link" >> write by the time we decide to time out on checking the link status. >> >> It may also never happen for you, but that's exactly like a classic race >> condition, where it may simply not manifest due to the code around the >> problematic lines hiding it. It may also only manifest on certain CPU >> cores that try to be smarter than you and keep reordering/delaying >> instructions if they don't seem immediately necessary. > > Does this mean the register is mapped incorrectly, e.g., I see arm64 > has many different kinds of mappings for cacheability, > write-buffering, etc? No, it's correctly mapped as "device" memory, but even that gives no guarantees about when the writes are "flushed" out of the CPU/cluster > > Or, if it is already mapped correctly, are we confident that none of > the *other* register writes need similar treatment? Is there a rule > we can apply to know when the read-after-write is needed? Generally, it's a good idea to add such readbacks after all timing- critical writes, especially when they concern asserting reset, enabling/disabling power, etc., to make sure we're not assuming the hardware state of a peripheral has changed before we ask it to do so. Konrad
On Thu, Feb 15, 2024 at 07:44:27PM +0100, Konrad Dybcio wrote: > On 15.02.2024 17:11, Bjorn Helgaas wrote: > > On Thu, Feb 15, 2024 at 11:21:45AM +0100, Konrad Dybcio wrote: > >> On 14.02.2024 23:28, Bjorn Helgaas wrote: > >>> On Wed, Feb 14, 2024 at 10:35:16PM +0100, Konrad Dybcio wrote: > >>>> On 12.02.2024 22:17, Bjorn Helgaas wrote: > >>>>> Maybe include the reason in the subject? "Read back" is literally > >>>>> what the diff says. > >>>>> > >>>>> On Sat, Feb 10, 2024 at 06:10:06PM +0100, Konrad Dybcio wrote: > >>>>>> To ensure write completion, read the PARF_LTSSM register after setting > >>>>>> the LTSSM enable bit before polling for "link up". > >>>>> > >>>>> The write will obviously complete *some* time; I assume the point is > >>>>> that it's important for it to complete before some other event, and it > >>>>> would be nice to know why that's important. > >>>> > >>>> Right, that's very much meaningful on non-total-store-ordering > >>>> architectures, like arm64, where the CPU receives a store instruction, > >>>> but that does not necessarily impact the memory/MMIO state immediately. > >>> > >>> I was hinting that maybe we could say what the other event is, or what > >>> problem this solves? E.g., maybe it's as simple as "there's no point > >>> in polling for link up until after the PARF_LTSSM store completes." > >>> > >>> But while the read of PARF_LTSSM might reduce the number of "is the > >>> link up" polls, it probably wouldn't speed anything up otherwise, so I > >>> suspect there's an actual functional reason for this patch, and that's > >>> what I'm getting at. > >> > >> So, the register containing the "enable switch" (PARF_LTSSM) can (due > >> to the armv8 memory model) be "written" but not "change the value of > >> memory/mmio from the perspective of other (non-CPU) memory-readers > >> (such as the MMIO-mapped PCI controller itself)". > >> > >> In that case, the CPU will happily continue calling qcom_pcie_link_up() > >> in a loop, waiting for the PCIe controller to bring the link up, however > >> the PCIE controller may have never received the PARF_LTSSM "enable link" > >> write by the time we decide to time out on checking the link status. This makes no sense. As Bjorn already said, you're just polling for the link to come up (for a second). And unless you have something else that depends on the write to have reached the device, there is no need to read it back. It's not going to be cached indefinitely if that's what you fear. > Generally, it's a good idea to add such readbacks after all timing- > critical writes, especially when they concern asserting reset, > enabling/disabling power, etc., to make sure we're not assuming the > hardware state of a peripheral has changed before we ask it to do so. Again no, there is no general need to do that. It all depends on what the code does and how the device works. Johan
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index cbde9effa352..6a469ed213ce 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -539,6 +539,7 @@ static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie) val = readl(pcie->parf + PARF_LTSSM); val |= LTSSM_EN; writel(val, pcie->parf + PARF_LTSSM); + readl(pcie->parf + PARF_LTSSM); } static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)