From patchwork Sat May 28 22:44:23 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jim Quinlan X-Patchwork-Id: 12864101 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 08279C433F5 for ; Sat, 28 May 2022 22:46: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:MIME-Version:List-Subscribe:List-Help: List-Post:List-Archive:List-Unsubscribe:List-Id:References:In-Reply-To: Message-Id:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=q4gR8b+JL4k8t7Tbctj7zxaCFtN64mpurEc/gdGrf9s=; b=F+L88f/mLO+M+K n7NMS9ejPHqeoJNaFHxC39lJpYiDAGa5+/NwG4+P27hkHdOhZjqKtqvTF7YSGvgNOPGdU2c/ziABR DbULFengNdJulUrFUv1/J1Qxgq5d/5ffDUKRPMtC4mYNPCalV6hsE7iS9S0OtP6uZ/D92OytPK45J lLOG51bZeMo2LO7hnGu7+26cMlflocJjsFjiYN5o/0+ehmhsscQDRgzYLxM8ygyWdFb/q2LGJUTu+ m8/MJLdNXzeToHcIhnpYSyCY2ytBWlPoDVcOjYObSasST78sAW/ap0/+WFo/yKxrhTnmXOCvBqiba 2nLdoPw4xIuwK4TgNdyg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nv5Ap-002rPi-QJ; Sat, 28 May 2022 22:44:43 +0000 Received: from mail-pf1-x435.google.com ([2607:f8b0:4864:20::435]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nv5Ae-002rMg-OM; Sat, 28 May 2022 22:44:34 +0000 Received: by mail-pf1-x435.google.com with SMTP id y1so7393193pfr.6; Sat, 28 May 2022 15:44:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=jKLY2zuvZewS/BwLmJvq6qxpOINA6m+jHazoY1CR+1E=; b=eYyugB7SlJeyfJ8Meaw2RV0Z7j7t43v2JfETGz+J9TpkrtImv7fWN8BqZ65UN2n5Zu Jm/w/rDTsmvrhSXQ5P/7kg3WeCP1JCNi4oSl/3Nf/rWoaqS45qHu1AwPQHbwwpljEjt0 hly0K3X5y0Id0Nd4VeILDg2lX4Pw+kj1BtjvH69YZfhhWdO7Z2HCKe1fZ/n9A7BgsZ9h fB2RXRIPO3hunYRrjBQQM+oKYmNatsSmqDsDEiCCo9MD3iLT7K4IJ+OJJgY6pZq64pDn q1yQyURG7ThiNQee3MJw1lKuh+7XCXqs3xdthpzO2/ZmmKUT11yZwQZDylr/axzEAQTu WFiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=jKLY2zuvZewS/BwLmJvq6qxpOINA6m+jHazoY1CR+1E=; b=Y/47ucDeYN43dkpygRnytIGCMfOoVmYmYdyf4RzZt6VT6FICfXd29DW1fXTY0MjtNh SZw5bbHQJ3UgPfSvh2s7XxCSktyTcWfmwW5RuVB+s+eQmLbZHl3LHGhBXzmbKdy64umk ie0ql1Hf3VU78pNu0UoPUgSdl3rWFgDFK6XE0suN0Z16LG2aDp//BPdAmJIbZrmNCi0d Cxjn2JVavQqmkvcNt7fL2cHuj15jI0CPy43ebpqbPhRUDuFKZ4N0xNakILq1hNs7NOrf gIBpMBxMBj1Jzt8UuwHQ9eZIgqEm88BA2k1Z71qm/caruuRLShaWs54arUCdj5A5QOmr Z6xw== X-Gm-Message-State: AOAM533yuttvWrEoZ/hvU49RknZ2qKZ81ahv9+QWx2OgGjQ0g3Hy9ieM jgXtwXCRpOwIc2MQXckdguQ= X-Google-Smtp-Source: ABdhPJyA/BKlk4Roppn6E2jXmyTWjXLBOuYK4p6OT1DEZxmNyaYvzQPTIaEJqaWY8mTvJ3QnALuFhA== X-Received: by 2002:a63:5a23:0:b0:3f2:678b:8de8 with SMTP id o35-20020a635a23000000b003f2678b8de8mr42486377pgb.226.1653777871733; Sat, 28 May 2022 15:44:31 -0700 (PDT) Received: from stbsrv-and-01.and.broadcom.net ([192.19.11.250]) by smtp.gmail.com with ESMTPSA id x22-20020a170902b41600b001635a8f9dfdsm6159514plr.26.2022.05.28.15.44.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 28 May 2022 15:44:31 -0700 (PDT) From: Jim Quinlan To: linux-pci@vger.kernel.org, Nicolas Saenz Julienne , Bjorn Helgaas , james.dutton@gmail.com, Cyril Brulebois , bcm-kernel-feedback-list@broadcom.com, jim2101024@gmail.com, james.quinlan@broadcom.com Cc: Florian Fainelli , Lorenzo Pieralisi , Rob Herring , =?utf-8?q?Krzysztof_Wilczy=C5=84ski?= , linux-rpi-kernel@lists.infradead.org (moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE), linux-arm-kernel@lists.infradead.org (moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE), linux-kernel@vger.kernel.org (open list) Subject: [PATCH v2 1/1] PCI: brcmstb: Fix regression regarding missing PCIe linkup Date: Sat, 28 May 2022 18:44:23 -0400 Message-Id: <20220528224423.7017-2-jim2101024@gmail.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20220528224423.7017-1-jim2101024@gmail.com> References: <20220528224423.7017-1-jim2101024@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220528_154432_837927_9140E3CE X-CRM114-Status: GOOD ( 24.42 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org commit 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators") introduced a regression on the PCIe RPi4 Compute Module. If the PCIe root port DT node described in [2] was missing, no linkup would be attempted, and subsequent accesses would cause a panic because this particular PCIe HW causes a CPU abort on illegal accesses (instead of returning 0xffffffff). We fix this by allowing the DT root port node to be missing, as it behaved before the original patchset messed things up. In addition, two small changes are made: 1. Having pci_subdev_regulators_remove_bus() call regulator_bulk_free() in addtion to regulator_bulk_disable(). 2. Having brcm_pcie_add_bus() return 0 if there is an error in calling pci_subdev_regulators_add_bus(). Instead, we dev_err() and turn on our refusal mode instead. It would be best if this commit were tested by someone with a Rpi CM4 platform, as that is how the regression was found. I have only emulated the problem and fix on different platform. Note that a bisection identified commit 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs") as the first failing commit. This commit is a regression, but is unrelated and was fixed by a subsequent commit in the original patchset. [1] https://bugzilla.kernel.org/show_bug.cgi?id=215925 [2] Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml Fixes: 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators") Fixes: 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs") Link: https://bugzilla.kernel.org/show_bug.cgi?id=215925 Signed-off-by: Jim Quinlan --- drivers/pci/controller/pcie-brcmstb.c | 43 +++++++++++++++++++-------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index ba5c120816b2..0839325f79ab 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -540,29 +540,42 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus) static int brcm_pcie_add_bus(struct pci_bus *bus) { - struct device *dev = &bus->dev; - struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata; + struct brcm_pcie *pcie; int ret; - if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent)) + /* + * Right now we only alloc/enable regulators and initiate pcie link + * when under the root port bus of the current domain. In the + * future we may want to alloc/enable regulators under any port + * device (e.g. a switch). + */ + if (!bus->parent || !pci_is_root_bus(bus->parent)) return 0; ret = pci_subdev_regulators_add_bus(bus); - if (ret) - return ret; + if (ret) { + dev_err(pcie->dev, "failed to alloc/enable regulators\n"); + goto err; + } - /* Grab the regulators for suspend/resume */ + /* Save the regulators for RC suspend/resume */ + pcie = (struct brcm_pcie *) bus->sysdata; pcie->sr = bus->dev.driver_data; + /* Attempt PCIe link-up */ + if (brcm_pcie_linkup(pcie) == 0) + return 0; +err: /* - * If we have failed linkup there is no point to return an error as - * currently it will cause a WARNING() from pci_alloc_child_bus(). - * We return 0 and turn on the "refusal_mode" so that any further - * accesses to the pci_dev just get 0xffffffff + * If we have failed linkup or have an error when turning on + * regulators, there is no point to return an error value to the + * caller (pci_alloc_child_bus()) as it will summarily execute a + * WARNING(). Instead, we turn on our "refusal_mode" and return 0 + * so that any further PCIe accesses succeed but do nothing (reads + * return 0xffffffff). If we do not turn on refusal mode, our + * unforgiving PCIe HW will signal a CPU abort. */ - if (brcm_pcie_linkup(pcie) != 0) - pcie->refusal_mode = true; - + pcie->refusal_mode = true; return 0; } @@ -570,13 +583,17 @@ static void pci_subdev_regulators_remove_bus(struct pci_bus *bus) { struct device *dev = &bus->dev; struct subdev_regulators *sr = dev->driver_data; + struct brcm_pcie *pcie; if (!sr || !bus->parent || !pci_is_root_bus(bus->parent)) return; if (regulator_bulk_disable(sr->num_supplies, sr->supplies)) dev_err(dev, "failed to disable regulators for downstream device\n"); + regulator_bulk_free(sr->num_supplies, sr->supplies); dev->driver_data = NULL; + pcie = (struct brcm_pcie *) bus->sysdata; + pcie->sr = NULL; } /* Limits operation to a specific generation (1, 2, or 3) */