From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C0644C27C53 for ; Fri, 7 Jun 2024 11:01:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=njG1uAlFb442v3G59f9KHB0vaF0QMHXcb+lRlt59M90=; b=Y+vC5qRnqqv7ln 8Fwke2Jd8AOMSNNryWA210N6BuyUtwZVG1DwhqZCrMcKtTseVbDhY0IfIy4dbxyG2psgMa8DOQtOv Ie1PolMFI6L+Av43o6vq7zfCqyVgjTyDmfen5zvpoBl9zycM6/b046xdd9OSSs9MaB0Ns54OX15pv bJgLR4dekyChBsnWmzZ06Q93e5RshXRXIgBk4zcKsyMo92rB61qv9rzQTN41aIzcWwUxlRL/+hKCz 2XV7OWI2MIt0LOq+yp5Tx97b6WjhjYeISpGYJrertFqtTi0g95s3lfsWRlmwafjnTPbCM8Q9SBsc0 bxRIOh8FN32F8aUcbVtA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sFXLU-0000000DdDK-1tUm; Fri, 07 Jun 2024 11:01:20 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sFXLR-0000000DdCB-0s4y for linux-rockchip@lists.infradead.org; Fri, 07 Jun 2024 11:01:18 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 85212CE1D2A; Fri, 7 Jun 2024 11:01:13 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 28329C32781; Fri, 7 Jun 2024 11:01:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1717758073; bh=mtUu/Do/GLFHawm+/M8nMC7SPdHfBpSjez+ss4kMKsU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=RWX2WR2HyLq3+UnPfPsnprPOtIaGzcPc1bsnV3duitv/Bf6mCReB5QJQvbQadEPEc iKETSHuQKnY+eo2jADupxpJMgxzbTWoXNji08JTXAIy9E7r8Is/yuqxpYZPJjxy3z3 Rbk0zriDNPXJudyRc7WhjrjSUanOnDvVfn8GFQeA5LxnGgkcCKpGy59iE2rM9PO7lS yJrN5hb4wgV3KPzMkKCkWsD/nyx2WxfsgGGTX8AEDqp2kfZw722eLixAzwxwEOh3QF f33GQedooaDeqwMRceIHZLlxhbwXDo5r1wYWn9l9DBKVR+Jyv+BTgKyi7Dv1Qh9f3P nYOzC0lEjv4GQ== Date: Fri, 7 Jun 2024 13:01:06 +0200 From: Niklas Cassel To: Manivannan Sadhasivam Cc: Jingoo Han , Manivannan Sadhasivam , Bjorn Helgaas , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Heiko Stuebner , Kishon Vijay Abraham I , Arnd Bergmann , Damien Le Moal , Jon Lin , Shawn Lin , Simon Xue , linux-pci@vger.kernel.org, devicetree@vger.kernel.org, linux-rockchip@lists.infradead.org Subject: Re: [PATCH v4 10/13] PCI: dw-rockchip: Add endpoint mode support Message-ID: References: <20240529-rockchip-pcie-ep-v1-v4-0-3dc00fe21a78@kernel.org> <20240529-rockchip-pcie-ep-v1-v4-10-3dc00fe21a78@kernel.org> <20240605081753.GK5085@thinkpad> <20240606063128.GC4441@thinkpad> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240606063128.GC4441@thinkpad> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240607_040117_627629_4ECF916C X-CRM114-Status: GOOD ( 41.10 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+linux-rockchip=archiver.kernel.org@lists.infradead.org On Thu, Jun 06, 2024 at 12:01:28PM +0530, Manivannan Sadhasivam wrote: > On Wed, Jun 05, 2024 at 08:58:06PM +0200, Niklas Cassel wrote: > > On Wed, Jun 05, 2024 at 01:47:53PM +0530, Manivannan Sadhasivam wrote: > > > On Wed, May 29, 2024 at 10:29:04AM +0200, Niklas Cassel wrote: > > > > The PCIe controller in rk3568 and rk3588 can operate in endpoint mode. > > > > This endpoint mode support heavily leverages the existing code in > > > > pcie-designware-ep.c. > > > > > > > > Add support for endpoint mode to the existing pcie-dw-rockchip glue > > > > driver. > > > > > > > > Signed-off-by: Niklas Cassel > > > > > > Couple of comments below. With those addressed, > > > > > > Reviewed-by: Manivannan Sadhasivam > > > > > > > --- > > > > drivers/pci/controller/dwc/Kconfig | 17 ++- > > > > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++ > > > > 2 files changed, 224 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > > > > index 8afacc90c63b..9fae0d977271 100644 > > > > --- a/drivers/pci/controller/dwc/Kconfig > > > > +++ b/drivers/pci/controller/dwc/Kconfig > > > > @@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP > > > > SoCs. To compile this driver as a module, choose M here: the module > > > > will be called pcie-rcar-gen4.ko. This uses the DesignWare core. > > > > > > > > +config PCIE_ROCKCHIP_DW > > > > + bool > > > > > > Where is this symbol used? > > > > It is supposed to be used by > > drivers/pci/controller/dwc/Makefile > > > > such that the driver is compiled if either _EP or _HOST is selected, just > > like how it is done for other drivers that support both in the same driver. > > Looks like I missed to update Makefile... > > Good catch, thank you! > > > > > > > > +static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg) > > > > +{ > > > > + struct rockchip_pcie *rockchip = arg; > > > > + struct dw_pcie *pci = &rockchip->pci; > > > > + struct device *dev = pci->dev; > > > > + u32 reg, val; > > > > + > > > > + reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC); > > > > + > > > > + dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg); > > > > + dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip)); > > > > + > > > > + if (reg & PCIE_LINK_REQ_RST_NOT_INT) { > > > > + dev_dbg(dev, "hot reset or link-down reset\n"); > > > > + dw_pcie_ep_linkdown(&pci->ep); > > > > + } > > > > + > > > > + if (reg & PCIE_RDLH_LINK_UP_CHGED) { > > > > + val = rockchip_pcie_get_ltssm(rockchip); > > > > + if ((val & PCIE_LINKUP) == PCIE_LINKUP) { > > > > + dev_dbg(dev, "link up\n"); > > > > + dw_pcie_ep_linkup(&pci->ep); > > > > + } > > > > + } > > > > + > > > > + rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC); > > > > > > It is recommended to clear the IRQs at the start of the handler (after status > > > read). > > > > Can you quote a reference in the databook to back this recommendation? > > > > It is just a general recommendation. > > > Otherwise I would lean towards keeping it like it is, since this is how > > it looks in the downstream driver (that *should* be well proven), and it > > also matches how it's done in dra7xx. > > > > (And since you ack only the events you read, you can not accidentally > > clear another type of event.) > > > > I haven't read the TRM, but if the IRQ line is level triggered, then if you do > not clear the IRQs immediately, you will miss some events. So I always suggest > to clear the IRQs at the start of the handler for all the platforms. They are level triggered. In this specific case, what could happen is that we fail to trigger an IRQ if we get two succeeding hot/link-down resets, or two succeding link ups. In neither case is this a serious case (compared to e.g. a host driver missing a MSI irq), but I have incorporated your suggestion for v5, thank you! Kind regards, Niklas _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip