Message ID | 20230608093652.1409485-1-vidyas@nvidia.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp162919vqr; Thu, 8 Jun 2023 02:59:14 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ59izYTyU8zD7YWKoaHgJ5JheXGuCGnQDxVtH49GUU0BXY8M9CphN/8imCZvxr1W1taidSr X-Received: by 2002:aca:2809:0:b0:398:29bb:dd4a with SMTP id 9-20020aca2809000000b0039829bbdd4amr6781757oix.54.1686218354225; Thu, 08 Jun 2023 02:59:14 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1686218354; cv=pass; d=google.com; s=arc-20160816; b=yseVm5CnPy2Nt5hNSuOxUpuXrs6j06SENSc4awbVq2Ds+j5I4J5G7uFHf738by1xN0 fhbZePa+hyruxm4QCbuIzDBHg8RqUfUSLkHC3GKmuDqfwDirwHMg+DlBSxjQLx2lJSTU QjDjppB7AGS9dZi0u0vJQNKI/7U7+w6oDKoBU+Wc+0vebYn9/j/pHNPfKQWrg8iTJXQL M00lZo6iURbcucIozQviJTOnpxabXDyDDKaxi0aRSb8Zsdrq4muQ0chYeUjhzlkBvWEH 7tG0HjCdwG6vFcoDZp0FRa9c4ZL9UXyqfQMnEXdSfwMBYMWvgUHaQA5xqX8VYJUjLRR3 DDQw== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=fl5G5HVh3aPVzpaNZoJRVqEuSii6FEWHaQozl8QYNXk=; b=VMLksiMjeGHofUDvdP/S/DRZtzlCCPuHItW1R2B9IcBGIWcmUV07WJFWTHs4or5YsM AR6bRAzLDDQag/LFOsPd/79Em8bPfHgAa+IV+fSYaih/K3tbpH/yAigG1pajViG0+tg1 +2TZGzrDkJngUIOnk81FnkVQpboMBO2GpT9rhfYkAcUzP2q599S6G5OklZ0obRGaWR5x EJyAjda4P9BHi/fq9wFxWuDznKzGO/E664csSuumh1v7KVUJYvX8qY7OLkO8k8kdZLno AxwVonPWVbj1FwTKKN4HQ/mGKqWI1hPotTmvzW10tdiLG0/M45ETOsOgm6DPfbhi1q4z l7CQ== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@Nvidia.com header.s=selector2 header.b=juRQcz1p; arc=pass (i=1 spf=pass spfdomain=nvidia.com dmarc=pass fromdomain=nvidia.com); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=nvidia.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id g14-20020a17090a4b0e00b00246bd5445d7si802106pjh.104.2023.06.08.02.58.57; Thu, 08 Jun 2023 02:59:14 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@Nvidia.com header.s=selector2 header.b=juRQcz1p; arc=pass (i=1 spf=pass spfdomain=nvidia.com dmarc=pass fromdomain=nvidia.com); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=nvidia.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235994AbjFHJhO (ORCPT <rfc822;liningstudo@gmail.com> + 99 others); Thu, 8 Jun 2023 05:37:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42476 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232262AbjFHJhM (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 8 Jun 2023 05:37:12 -0400 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (mail-bn8nam11on2040.outbound.protection.outlook.com [40.107.236.40]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E82C7E4E; Thu, 8 Jun 2023 02:37:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Z8iI16nho+7ze1AkFkWi7Jt8tjg57+CadoNnYTbefW42b5FU7vMiblo071CWlIKXqmanTow3Kkp6TAP5xkU1F6SyR0eOoqaVritA9VeiolU3UZb1tu9jqMhc3d4nUkx3scvjbxG18Iviyc4cCzq3a9PN4lJvwqZzxVW7urLOQqA17PRU6iV+2ryGAIjx0gwoLRqGm6eV4V9g3yBspvhartI0Yqn+EmhrIMOznvClhiKZzBIJbhtmCIRiSo6St4uqiz8mWTq8qcCvVxJ6k2lGOKZLYWBqo7nVrEgwuwriZPofSKz1cHcBE23tEWqQzEIYPAtxkbZEbd466TI1NnZcaw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=fl5G5HVh3aPVzpaNZoJRVqEuSii6FEWHaQozl8QYNXk=; b=mtl2vuoa6wMGhnIVQEI0m5pPoqM0kXlgoN791grtz9r7r2tB7JnTbVdnOnhvzp9mlL6f3r9mT5VRVDc0akmPSo7xlQCjI92UHxCelRHnldIuzvEHS9nIGwW7Ncp6sTgCS2RO9u3GZFdnpZSfXNW2g01GqPlzH0c2suH9mPCrhGfuNIZ6y/N/+phThg9quIJyYcKr+HVmSwg8Lj5jr142NlMoNznb4Z5+4N/puhv9Zvz2XqC2GfitraAS9uCyOtL7mm1+EWNtQVdOEiGieEWZrF7TaQAyjIDTYW6QCgwER8DxXGJQjcZCVSTh2ZlBbdXg+b9mGOJnxDr35iEGuHOYSg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 216.228.118.232) smtp.rcpttodomain=kernel.org smtp.mailfrom=nvidia.com; dmarc=pass (p=reject sp=reject pct=100) action=none header.from=nvidia.com; dkim=none (message not signed); arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=fl5G5HVh3aPVzpaNZoJRVqEuSii6FEWHaQozl8QYNXk=; b=juRQcz1p1xURX1zjn6KDahsIChs6fvUJK0rNxRcgEJdUc7Dp+VP7NtXAeHLsJ8qA5Z6iYYd5/gADrWGIcjEv/AWFwpqzLGqBE97epXpG1ClPNLjdWWhZNoO6zv3r0kPrZZVvB33VuuQhqxgmZfZKNoyn1U6eGsjEN+n9K/j8FrOX4mBGV0FgHEJh4r4mLPfybLtclOIowj0iWiMrW32gbTg3P2PVGcKbSmIgke+WzQ26tnJpkoi9Ama3N4caf4pgsovcv2B4z58soDH9wFfWFW59i95izC6Rll2i/agOewKlM9CcDUHA6l4sVwsZBxWaRZ6m1raBCkm0eQ6gk/2mgw== Received: from DS7PR05CA0077.namprd05.prod.outlook.com (2603:10b6:8:57::17) by PH0PR12MB7080.namprd12.prod.outlook.com (2603:10b6:510:21d::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6455.33; Thu, 8 Jun 2023 09:37:08 +0000 Received: from CY4PEPF0000EDD7.namprd03.prod.outlook.com (2603:10b6:8:57:cafe::92) by DS7PR05CA0077.outlook.office365.com (2603:10b6:8:57::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6500.12 via Frontend Transport; Thu, 8 Jun 2023 09:37:07 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 216.228.118.232) smtp.mailfrom=nvidia.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=nvidia.com; Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates 216.228.118.232 as permitted sender) receiver=protection.outlook.com; client-ip=216.228.118.232; helo=mail.nvidia.com; pr=C Received: from mail.nvidia.com (216.228.118.232) by CY4PEPF0000EDD7.mail.protection.outlook.com (10.167.241.211) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6477.13 via Frontend Transport; Thu, 8 Jun 2023 09:37:07 +0000 Received: from drhqmail202.nvidia.com (10.126.190.181) by mail.nvidia.com (10.127.129.5) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.5; Thu, 8 Jun 2023 02:37:00 -0700 Received: from drhqmail202.nvidia.com (10.126.190.181) by drhqmail202.nvidia.com (10.126.190.181) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.37; Thu, 8 Jun 2023 02:36:59 -0700 Received: from vidyas-desktop.nvidia.com (10.127.8.9) by mail.nvidia.com (10.126.190.181) with Microsoft SMTP Server id 15.2.986.37 via Frontend Transport; Thu, 8 Jun 2023 02:36:56 -0700 From: Vidya Sagar <vidyas@nvidia.com> To: <lpieralisi@kernel.org>, <kw@linux.com>, <robh@kernel.org>, <bhelgaas@google.com>, <thierry.reding@gmail.com>, <jonathanh@nvidia.com>, <Sergey.Semin@baikalelectronics.ru> CC: <linux-pci@vger.kernel.org>, <linux-tegra@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <kthota@nvidia.com>, <mmaddireddy@nvidia.com>, <vidyas@nvidia.com>, <sagar.tv@gmail.com> Subject: [PATCH V1] Revert "PCI: tegra194: Enable support for 256 Byte payload" Date: Thu, 8 Jun 2023 15:06:52 +0530 Message-ID: <20230608093652.1409485-1-vidyas@nvidia.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 X-NVConfidentiality: public Content-Transfer-Encoding: 8bit Content-Type: text/plain X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CY4PEPF0000EDD7:EE_|PH0PR12MB7080:EE_ X-MS-Office365-Filtering-Correlation-Id: 035b6a95-6aed-4c89-8bfb-08db6803ed57 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: B5d+qXFdztGIQ1GP/833l0mN7b/obWcZK2TSTFnYL2+d9kFXTiBcVDwdtevzm9hzVu4kH94iJ5tgWcwdrfhQQdQxIlDqBZFR5cVzPu7E4KOzoDzOYf38/2ogG9UBvPn9i79FNM0bLDPs3rXOFDbevwZGeijFEs9Wv48d/XUVHWxurihoJ7kL+j/WE97BmE+7HNIYi9chZGs8tIvRAg9WiiJlISZdPmW0L4Pkvl70u7/xNdc6TEDZ3l2FH4abeme0jnz/fpzD5tRdUKHuTzmMIUtZn3BkWQuwKbPhmC2j8OILdGMKvlzjpQpFJj/nhMY6T9+s+/iuiQcDUlsgh/oat7oRQHM867gjb3eDO3HkykuumsmuzbWD89y+vWyKTV0zGpX42Z3hnHgf73BQadLTROFyGT65zo6AGCjts5JQyWmyhssaFeC1tSsCWAxXTX7Nrp+PsU7rK/vKFdj9YEtuFkB9dcvBlGhWFsPoA9PB2WB1hZxUnyjo3dFAYwO8MhTfOCLeUqEA8F++lxtBghF5GD3dA4NhJlOHh7bVbLvuQlXlAAsnMnaXHtWGUshqv0B3O0NytKUfq01Y6CPNVkZ2C2HqN4jNX36Ohk91c3UOZuUAfTjfAsmBQ/Yq5ku0ap9yo1f8B2O01lblmyjz4vUCaJNKGFzeEIqI9YQRaPint+SSSo+ikcizgec6CWYSjADohxfvspNOBLYtXj9BEDH6eaGjYvx+gNHSUwgXNFUDyWy1h5mYdEbjgedx8Hz5w7mU X-Forefront-Antispam-Report: CIP:216.228.118.232;CTRY:US;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:mail.nvidia.com;PTR:dc7edge1.nvidia.com;CAT:NONE;SFS:(13230028)(4636009)(376002)(396003)(136003)(346002)(39860400002)(451199021)(40470700004)(46966006)(36840700001)(36756003)(5660300002)(2906002)(82310400005)(86362001)(7416002)(40480700001)(6666004)(1076003)(83380400001)(40460700003)(36860700001)(336012)(426003)(7636003)(26005)(316002)(356005)(478600001)(54906003)(7696005)(110136005)(70586007)(82740400003)(4326008)(41300700001)(2616005)(186003)(70206006)(8936002)(47076005)(8676002);DIR:OUT;SFP:1101; X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jun 2023 09:37:07.7935 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 035b6a95-6aed-4c89-8bfb-08db6803ed57 X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.118.232];Helo=[mail.nvidia.com] X-MS-Exchange-CrossTenant-AuthSource: CY4PEPF0000EDD7.namprd03.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB7080 X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768128097118912418?= X-GMAIL-MSGID: =?utf-8?q?1768128097118912418?= |
Series |
[V1] Revert "PCI: tegra194: Enable support for 256 Byte payload"
|
|
Commit Message
Vidya Sagar
June 8, 2023, 9:36 a.m. UTC
This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable
support for 256 Byte payload")
Consider a PCIe hierarchy with a PCIe switch and a device connected
downstream of the switch that has support for MPS which is the minimum
in the hierarchy, and root port programmed with an MPS in its DevCtl
register that is greater than the minimum. In this scenario, the default
bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't
configure the MPS settings in the hierarchy correctly resulting in the
device with support for minimum MPS in the hierarchy receiving the TLPs
of size more than that. Although this can be addresed by appending
"pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a
good idea to always have this commandline argument even for the basic
functionality to work.
Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256
Byte payload") avoids this requirement and ensures that the basic
functionality of the devices irrespective of the hierarchy and the MPS of
the devices in the hierarchy.
To reap the benefits of having support for higher MPS, optionally, one can
always append the kernel command line with "pci=pcie_bus_perf".
Fixes: 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte payload")
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
drivers/pci/controller/dwc/pcie-tegra194.c | 13 -------------
1 file changed, 13 deletions(-)
Comments
On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: > This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable > support for 256 Byte payload") > > Consider a PCIe hierarchy with a PCIe switch and a device connected > downstream of the switch that has support for MPS which is the minimum > in the hierarchy, and root port programmed with an MPS in its DevCtl > register that is greater than the minimum. In this scenario, the default > bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't > configure the MPS settings in the hierarchy correctly resulting in the > device with support for minimum MPS in the hierarchy receiving the TLPs > of size more than that. Although this can be addresed by appending > "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a > good idea to always have this commandline argument even for the basic > functionality to work. > Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 > Byte payload") avoids this requirement and ensures that the basic > functionality of the devices irrespective of the hierarchy and the MPS of > the devices in the hierarchy. > To reap the benefits of having support for higher MPS, optionally, one can > always append the kernel command line with "pci=pcie_bus_perf". Please add blank lines between paragraphs and wrap to fill 75 columns. Also add a period at the end of the very first sentence. s/addresed/addressed/ I guess that without 4fb8e46c1bc4, Linux configured everything with 128 byte MPS, and 4fb8e46c1bc4 was intended as an optimization to allow 256 byte MPS. If the Root Port advertises Max_Payload_Size Supported as 256 bytes in DevCap, and the PCI core doesn't configure MPS=256 when possible, I'd argue that should be fixed in the PCI core without a driver change like this. Bjorn
On 6/8/2023 10:03 PM, Bjorn Helgaas wrote: > External email: Use caution opening links or attachments > > > On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: >> This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable >> support for 256 Byte payload") >> >> Consider a PCIe hierarchy with a PCIe switch and a device connected >> downstream of the switch that has support for MPS which is the minimum >> in the hierarchy, and root port programmed with an MPS in its DevCtl >> register that is greater than the minimum. In this scenario, the default >> bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't >> configure the MPS settings in the hierarchy correctly resulting in the >> device with support for minimum MPS in the hierarchy receiving the TLPs >> of size more than that. Although this can be addresed by appending >> "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a >> good idea to always have this commandline argument even for the basic >> functionality to work. >> Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 >> Byte payload") avoids this requirement and ensures that the basic >> functionality of the devices irrespective of the hierarchy and the MPS of >> the devices in the hierarchy. >> To reap the benefits of having support for higher MPS, optionally, one can >> always append the kernel command line with "pci=pcie_bus_perf". > > Please add blank lines between paragraphs and wrap to fill 75 columns. > Also add a period at the end of the very first sentence. > > s/addresed/addressed/ > I'll address your comments in the next patch. > I guess that without 4fb8e46c1bc4, Linux configured everything with > 128 byte MPS, and 4fb8e46c1bc4 was intended as an optimization to > allow 256 byte MPS. Correct. > > If the Root Port advertises Max_Payload_Size Supported as 256 bytes in > DevCap, and the PCI core doesn't configure MPS=256 when possible, I'd > argue that should be fixed in the PCI core without a driver change > like this. Well, kernel does configure MPS=256 but only if the 'perf' configuration option is selected. 'perf' configuration option also changes the MRRS, to extract the maximum performance. I'm not sure about the reasons for not making 'perf' configuration as the default configuration though. (IIUC, this is what you are coming to, right?) The current patch which is a revert of an earlier patch is to keep things working for different PCIe hierarchies given the default configuration that kernel is using at the moment. -Vidya Sagar > > Bjorn
Hi Bjorn, On 09/06/2023 03:23, Vidya Sagar wrote: > > > On 6/8/2023 10:03 PM, Bjorn Helgaas wrote: >> External email: Use caution opening links or attachments >> >> >> On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: >>> This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable >>> support for 256 Byte payload") >>> >>> Consider a PCIe hierarchy with a PCIe switch and a device connected >>> downstream of the switch that has support for MPS which is the minimum >>> in the hierarchy, and root port programmed with an MPS in its DevCtl >>> register that is greater than the minimum. In this scenario, the default >>> bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't >>> configure the MPS settings in the hierarchy correctly resulting in the >>> device with support for minimum MPS in the hierarchy receiving the TLPs >>> of size more than that. Although this can be addresed by appending >>> "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a >>> good idea to always have this commandline argument even for the basic >>> functionality to work. >>> Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 >>> Byte payload") avoids this requirement and ensures that the basic >>> functionality of the devices irrespective of the hierarchy and the >>> MPS of >>> the devices in the hierarchy. >>> To reap the benefits of having support for higher MPS, optionally, >>> one can >>> always append the kernel command line with "pci=pcie_bus_perf". >> >> Please add blank lines between paragraphs and wrap to fill 75 columns. >> Also add a period at the end of the very first sentence. >> >> s/addresed/addressed/ >> > I'll address your comments in the next patch. > >> I guess that without 4fb8e46c1bc4, Linux configured everything with >> 128 byte MPS, and 4fb8e46c1bc4 was intended as an optimization to >> allow 256 byte MPS. > Correct. > >> >> If the Root Port advertises Max_Payload_Size Supported as 256 bytes in >> DevCap, and the PCI core doesn't configure MPS=256 when possible, I'd >> argue that should be fixed in the PCI core without a driver change >> like this. > Well, kernel does configure MPS=256 but only if the 'perf' configuration > option is selected. 'perf' configuration option also changes the MRRS, > to extract the maximum performance. I'm not sure about the reasons for > not making 'perf' configuration as the default configuration though. > (IIUC, this is what you are coming to, right?) > > The current patch which is a revert of an earlier patch is to keep > things working for different PCIe hierarchies given the default > configuration that kernel is using at the moment. Any more feedback on this? If not, would be great to queue for v6.5. Feel free to add my ... Acked-by: Jon Hunter <jonathanh@nvidia.com> Cheers Jon
On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: > This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable > support for 256 Byte payload") > > Consider a PCIe hierarchy with a PCIe switch and a device connected > downstream of the switch that has support for MPS which is the minimum > in the hierarchy, and root port programmed with an MPS in its DevCtl > register that is greater than the minimum. In this scenario, the default > bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't > configure the MPS settings in the hierarchy correctly resulting in the > device with support for minimum MPS in the hierarchy receiving the TLPs > of size more than that. Although this can be addresed by appending > "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a > good idea to always have this commandline argument even for the basic > functionality to work. > Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 > Byte payload") avoids this requirement and ensures that the basic > functionality of the devices irrespective of the hierarchy and the MPS of > the devices in the hierarchy. > To reap the benefits of having support for higher MPS, optionally, one can > always append the kernel command line with "pci=pcie_bus_perf". > > Fixes: 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte payload") > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> I know that this patch is merged. But I happen to test a similar change on Qcom platform during a patch review and found that the PCI core changes MPS to 128 when a 128byte supported device is found: [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128) [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128) This was just randomly tested on a platform whose Root port DEVCAP was 128, but it shouldn't matter. And I didn't change the default bus configuration. Wondering how you ended up facing issues with it. - Mani > --- > drivers/pci/controller/dwc/pcie-tegra194.c | 13 ------------- > 1 file changed, 13 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > index 4fdadc7b045f..877d81b13334 100644 > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct tegra_pcie_dw *pcie = to_tegra_pcie(pci); > u32 val; > - u16 val_16; > > pp->bridge->ops = &tegra_pci_ops; > > @@ -900,11 +899,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) > pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, > PCI_CAP_ID_EXP); > > - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); > - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; > - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; > - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); > - > val = dw_pcie_readl_dbi(pci, PCI_IO_BASE); > val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8); > dw_pcie_writel_dbi(pci, PCI_IO_BASE, val); > @@ -1756,7 +1750,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > struct device *dev = pcie->dev; > u32 val; > int ret; > - u16 val_16; > > if (pcie->ep_state == EP_STATE_ENABLED) > return; > @@ -1887,11 +1880,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, > PCI_CAP_ID_EXP); > > - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); > - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; > - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; > - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); > - > /* Clear Slot Clock Configuration bit if SRNS configuration */ > if (pcie->enable_srns) { > val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + > @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA, > val_16); > } > - > clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ); > > val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK); > -- > 2.25.1 >
On 7/25/2023 1:21 PM, Manivannan Sadhasivam wrote: > External email: Use caution opening links or attachments > > > On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: >> This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable >> support for 256 Byte payload") >> >> Consider a PCIe hierarchy with a PCIe switch and a device connected >> downstream of the switch that has support for MPS which is the minimum >> in the hierarchy, and root port programmed with an MPS in its DevCtl >> register that is greater than the minimum. In this scenario, the default >> bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't >> configure the MPS settings in the hierarchy correctly resulting in the >> device with support for minimum MPS in the hierarchy receiving the TLPs >> of size more than that. Although this can be addresed by appending >> "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a >> good idea to always have this commandline argument even for the basic >> functionality to work. >> Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 >> Byte payload") avoids this requirement and ensures that the basic >> functionality of the devices irrespective of the hierarchy and the MPS of >> the devices in the hierarchy. >> To reap the benefits of having support for higher MPS, optionally, one can >> always append the kernel command line with "pci=pcie_bus_perf". >> >> Fixes: 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte payload") >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > I know that this patch is merged. But I happen to test a similar change on Qcom > platform during a patch review and found that the PCI core changes MPS to 128 > when a 128byte supported device is found: > > [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128) > [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128) > > This was just randomly tested on a platform whose Root port DEVCAP was 128, but > it shouldn't matter. And I didn't change the default bus configuration. > > Wondering how you ended up facing issues with it. If the endpiont device that has support only for 128byte MPS is connected directly to the root port, then, I agree that the PCIe sub-system takes care of changing the MPS to the common lowest MPS, but if the endpoint device is connected behind a PCIe switch, then the PCIe subsystem doesn't configure the MPS properly. -Vidya Sagar > > - Mani > >> --- >> drivers/pci/controller/dwc/pcie-tegra194.c | 13 ------------- >> 1 file changed, 13 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c >> index 4fdadc7b045f..877d81b13334 100644 >> --- a/drivers/pci/controller/dwc/pcie-tegra194.c >> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c >> @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> struct tegra_pcie_dw *pcie = to_tegra_pcie(pci); >> u32 val; >> - u16 val_16; >> >> pp->bridge->ops = &tegra_pci_ops; >> >> @@ -900,11 +899,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) >> pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, >> PCI_CAP_ID_EXP); >> >> - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); >> - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; >> - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; >> - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); >> - >> val = dw_pcie_readl_dbi(pci, PCI_IO_BASE); >> val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8); >> dw_pcie_writel_dbi(pci, PCI_IO_BASE, val); >> @@ -1756,7 +1750,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) >> struct device *dev = pcie->dev; >> u32 val; >> int ret; >> - u16 val_16; >> >> if (pcie->ep_state == EP_STATE_ENABLED) >> return; >> @@ -1887,11 +1880,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) >> pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, >> PCI_CAP_ID_EXP); >> >> - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); >> - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; >> - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; >> - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); >> - >> /* Clear Slot Clock Configuration bit if SRNS configuration */ >> if (pcie->enable_srns) { >> val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + >> @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) >> dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA, >> val_16); >> } >> - >> clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ); >> >> val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK); >> -- >> 2.25.1 >> > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Jul 25, 2023 at 01:49:35PM +0530, Vidya Sagar wrote: > > > On 7/25/2023 1:21 PM, Manivannan Sadhasivam wrote: > > External email: Use caution opening links or attachments > > > > > > On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: > > > This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable > > > support for 256 Byte payload") > > > > > > Consider a PCIe hierarchy with a PCIe switch and a device connected > > > downstream of the switch that has support for MPS which is the minimum > > > in the hierarchy, and root port programmed with an MPS in its DevCtl > > > register that is greater than the minimum. In this scenario, the default > > > bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't > > > configure the MPS settings in the hierarchy correctly resulting in the > > > device with support for minimum MPS in the hierarchy receiving the TLPs > > > of size more than that. Although this can be addresed by appending > > > "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a > > > good idea to always have this commandline argument even for the basic > > > functionality to work. > > > Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 > > > Byte payload") avoids this requirement and ensures that the basic > > > functionality of the devices irrespective of the hierarchy and the MPS of > > > the devices in the hierarchy. > > > To reap the benefits of having support for higher MPS, optionally, one can > > > always append the kernel command line with "pci=pcie_bus_perf". > > > > > > Fixes: 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte payload") > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > I know that this patch is merged. But I happen to test a similar change on Qcom > > platform during a patch review and found that the PCI core changes MPS to 128 > > when a 128byte supported device is found: > > > > [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128) > > [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128) > > > > This was just randomly tested on a platform whose Root port DEVCAP was 128, but > > it shouldn't matter. And I didn't change the default bus configuration. > > > > Wondering how you ended up facing issues with it. > > If the endpiont device that has support only for 128byte MPS is connected > directly to the root port, then, I agree that the PCIe sub-system takes care > of changing the MPS to the common lowest MPS, but if the endpoint device is > connected behind a PCIe switch, then the PCIe subsystem doesn't configure > the MPS properly. > Ah, I missed the fact that your issue only happens with a PCIe switch. But shouldn't this be fixed in the PCI core instead? I mean, PCI core should use 128byte in your case for Root port unless it is not allowed in the spec (which I doubt). - Mani > -Vidya Sagar > > > > > - Mani > > > > > --- > > > drivers/pci/controller/dwc/pcie-tegra194.c | 13 ------------- > > > 1 file changed, 13 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > > > index 4fdadc7b045f..877d81b13334 100644 > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > > > @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) > > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > > struct tegra_pcie_dw *pcie = to_tegra_pcie(pci); > > > u32 val; > > > - u16 val_16; > > > > > > pp->bridge->ops = &tegra_pci_ops; > > > > > > @@ -900,11 +899,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) > > > pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, > > > PCI_CAP_ID_EXP); > > > > > > - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); > > > - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; > > > - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; > > > - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); > > > - > > > val = dw_pcie_readl_dbi(pci, PCI_IO_BASE); > > > val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8); > > > dw_pcie_writel_dbi(pci, PCI_IO_BASE, val); > > > @@ -1756,7 +1750,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > > > struct device *dev = pcie->dev; > > > u32 val; > > > int ret; > > > - u16 val_16; > > > > > > if (pcie->ep_state == EP_STATE_ENABLED) > > > return; > > > @@ -1887,11 +1880,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > > > pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, > > > PCI_CAP_ID_EXP); > > > > > > - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); > > > - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; > > > - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; > > > - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); > > > - > > > /* Clear Slot Clock Configuration bit if SRNS configuration */ > > > if (pcie->enable_srns) { > > > val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + > > > @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > > > dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA, > > > val_16); > > > } > > > - > > > clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ); > > > > > > val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK); > > > -- > > > 2.25.1 > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
On 7/25/2023 2:00 PM, Manivannan Sadhasivam wrote: > External email: Use caution opening links or attachments > > > On Tue, Jul 25, 2023 at 01:49:35PM +0530, Vidya Sagar wrote: >> >> >> On 7/25/2023 1:21 PM, Manivannan Sadhasivam wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: >>>> This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable >>>> support for 256 Byte payload") >>>> >>>> Consider a PCIe hierarchy with a PCIe switch and a device connected >>>> downstream of the switch that has support for MPS which is the minimum >>>> in the hierarchy, and root port programmed with an MPS in its DevCtl >>>> register that is greater than the minimum. In this scenario, the default >>>> bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't >>>> configure the MPS settings in the hierarchy correctly resulting in the >>>> device with support for minimum MPS in the hierarchy receiving the TLPs >>>> of size more than that. Although this can be addresed by appending >>>> "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a >>>> good idea to always have this commandline argument even for the basic >>>> functionality to work. >>>> Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 >>>> Byte payload") avoids this requirement and ensures that the basic >>>> functionality of the devices irrespective of the hierarchy and the MPS of >>>> the devices in the hierarchy. >>>> To reap the benefits of having support for higher MPS, optionally, one can >>>> always append the kernel command line with "pci=pcie_bus_perf". >>>> >>>> Fixes: 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte payload") >>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >>> >>> I know that this patch is merged. But I happen to test a similar change on Qcom >>> platform during a patch review and found that the PCI core changes MPS to 128 >>> when a 128byte supported device is found: >>> >>> [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128) >>> [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128) >>> >>> This was just randomly tested on a platform whose Root port DEVCAP was 128, but >>> it shouldn't matter. And I didn't change the default bus configuration. >>> >>> Wondering how you ended up facing issues with it. >> >> If the endpiont device that has support only for 128byte MPS is connected >> directly to the root port, then, I agree that the PCIe sub-system takes care >> of changing the MPS to the common lowest MPS, but if the endpoint device is >> connected behind a PCIe switch, then the PCIe subsystem doesn't configure >> the MPS properly. >> > > Ah, I missed the fact that your issue only happens with a PCIe switch. But > shouldn't this be fixed in the PCI core instead? > > I mean, PCI core should use 128byte in your case for Root port unless it is not > allowed in the spec (which I doubt). well, if the RP's DevCtl is set to 256MPS by default, then, the core wouldn't do it automatically unless either 'pcie_bus_safe' or 'pcie_bus_perf' is passed. > > - Mani > >> -Vidya Sagar >> >>> >>> - Mani >>> >>>> --- >>>> drivers/pci/controller/dwc/pcie-tegra194.c | 13 ------------- >>>> 1 file changed, 13 deletions(-) >>>> >>>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c >>>> index 4fdadc7b045f..877d81b13334 100644 >>>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c >>>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c >>>> @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) >>>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >>>> struct tegra_pcie_dw *pcie = to_tegra_pcie(pci); >>>> u32 val; >>>> - u16 val_16; >>>> >>>> pp->bridge->ops = &tegra_pci_ops; >>>> >>>> @@ -900,11 +899,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) >>>> pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, >>>> PCI_CAP_ID_EXP); >>>> >>>> - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); >>>> - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; >>>> - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; >>>> - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); >>>> - >>>> val = dw_pcie_readl_dbi(pci, PCI_IO_BASE); >>>> val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8); >>>> dw_pcie_writel_dbi(pci, PCI_IO_BASE, val); >>>> @@ -1756,7 +1750,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) >>>> struct device *dev = pcie->dev; >>>> u32 val; >>>> int ret; >>>> - u16 val_16; >>>> >>>> if (pcie->ep_state == EP_STATE_ENABLED) >>>> return; >>>> @@ -1887,11 +1880,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) >>>> pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, >>>> PCI_CAP_ID_EXP); >>>> >>>> - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); >>>> - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; >>>> - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; >>>> - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); >>>> - >>>> /* Clear Slot Clock Configuration bit if SRNS configuration */ >>>> if (pcie->enable_srns) { >>>> val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + >>>> @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) >>>> dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA, >>>> val_16); >>>> } >>>> - >>>> clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ); >>>> >>>> val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK); >>>> -- >>>> 2.25.1 >>>> >>> >>> -- >>> மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
On Tue, Jul 25, 2023 at 02:51:10PM +0530, Vidya Sagar wrote: > > > On 7/25/2023 2:00 PM, Manivannan Sadhasivam wrote: > > External email: Use caution opening links or attachments > > > > > > On Tue, Jul 25, 2023 at 01:49:35PM +0530, Vidya Sagar wrote: > > > > > > > > > On 7/25/2023 1:21 PM, Manivannan Sadhasivam wrote: > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > On Thu, Jun 08, 2023 at 03:06:52PM +0530, Vidya Sagar wrote: > > > > > This reverts commit 4fb8e46c1bc4 ("PCI: tegra194: Enable > > > > > support for 256 Byte payload") > > > > > > > > > > Consider a PCIe hierarchy with a PCIe switch and a device connected > > > > > downstream of the switch that has support for MPS which is the minimum > > > > > in the hierarchy, and root port programmed with an MPS in its DevCtl > > > > > register that is greater than the minimum. In this scenario, the default > > > > > bus configuration of the kernel i.e. "PCIE_BUS_DEFAULT" doesn't > > > > > configure the MPS settings in the hierarchy correctly resulting in the > > > > > device with support for minimum MPS in the hierarchy receiving the TLPs > > > > > of size more than that. Although this can be addresed by appending > > > > > "pci=pcie_bus_safe" to the kernel command line, it doesn't seem to be a > > > > > good idea to always have this commandline argument even for the basic > > > > > functionality to work. > > > > > Reverting commit 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 > > > > > Byte payload") avoids this requirement and ensures that the basic > > > > > functionality of the devices irrespective of the hierarchy and the MPS of > > > > > the devices in the hierarchy. > > > > > To reap the benefits of having support for higher MPS, optionally, one can > > > > > always append the kernel command line with "pci=pcie_bus_perf". > > > > > > > > > > Fixes: 4fb8e46c1bc4 ("PCI: tegra194: Enable support for 256 Byte payload") > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > > > > > I know that this patch is merged. But I happen to test a similar change on Qcom > > > > platform during a patch review and found that the PCI core changes MPS to 128 > > > > when a 128byte supported device is found: > > > > > > > > [ 3.174290] pci 0000:01:00.0: Upstream bridge's Max Payload Size set to 128 (was 256, max 128) > > > > [ 3.186538] pci 0000:01:00.0: Max Payload Size set to 128 (was 128, max 128) > > > > > > > > This was just randomly tested on a platform whose Root port DEVCAP was 128, but > > > > it shouldn't matter. And I didn't change the default bus configuration. > > > > > > > > Wondering how you ended up facing issues with it. > > > > > > If the endpiont device that has support only for 128byte MPS is connected > > > directly to the root port, then, I agree that the PCIe sub-system takes care > > > of changing the MPS to the common lowest MPS, but if the endpoint device is > > > connected behind a PCIe switch, then the PCIe subsystem doesn't configure > > > the MPS properly. > > > > > > > Ah, I missed the fact that your issue only happens with a PCIe switch. But > > shouldn't this be fixed in the PCI core instead? > > > > I mean, PCI core should use 128byte in your case for Root port unless it is not > > allowed in the spec (which I doubt). > well, if the RP's DevCtl is set to 256MPS by default, then, the core > wouldn't do it automatically unless either 'pcie_bus_safe' or > 'pcie_bus_perf' is passed. > That's what I'm referring to. The default configuration shouldn't cause Root port to send TLPs with unsupported payload. Moreover, this is not the case for immediate children. So definitely the PCI core should exhibit the same behavior for all downstream devices. - Mani > > > > > - Mani > > > > > -Vidya Sagar > > > > > > > > > > > - Mani > > > > > > > > > --- > > > > > drivers/pci/controller/dwc/pcie-tegra194.c | 13 ------------- > > > > > 1 file changed, 13 deletions(-) > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > > > > > index 4fdadc7b045f..877d81b13334 100644 > > > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > > > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > > > > > @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) > > > > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > > > > > struct tegra_pcie_dw *pcie = to_tegra_pcie(pci); > > > > > u32 val; > > > > > - u16 val_16; > > > > > > > > > > pp->bridge->ops = &tegra_pci_ops; > > > > > > > > > > @@ -900,11 +899,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) > > > > > pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, > > > > > PCI_CAP_ID_EXP); > > > > > > > > > > - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); > > > > > - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; > > > > > - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; > > > > > - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); > > > > > - > > > > > val = dw_pcie_readl_dbi(pci, PCI_IO_BASE); > > > > > val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8); > > > > > dw_pcie_writel_dbi(pci, PCI_IO_BASE, val); > > > > > @@ -1756,7 +1750,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > > > > > struct device *dev = pcie->dev; > > > > > u32 val; > > > > > int ret; > > > > > - u16 val_16; > > > > > > > > > > if (pcie->ep_state == EP_STATE_ENABLED) > > > > > return; > > > > > @@ -1887,11 +1880,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > > > > > pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, > > > > > PCI_CAP_ID_EXP); > > > > > > > > > > - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); > > > > > - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; > > > > > - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; > > > > > - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); > > > > > - > > > > > /* Clear Slot Clock Configuration bit if SRNS configuration */ > > > > > if (pcie->enable_srns) { > > > > > val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + > > > > > @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) > > > > > dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA, > > > > > val_16); > > > > > } > > > > > - > > > > > clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ); > > > > > > > > > > val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK); > > > > > -- > > > > > 2.25.1 > > > > > > > > > > > > > -- > > > > மணிவண்ணன் சதாசிவம் > > > > -- > > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index 4fdadc7b045f..877d81b13334 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -892,7 +892,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) struct dw_pcie *pci = to_dw_pcie_from_pp(pp); struct tegra_pcie_dw *pcie = to_tegra_pcie(pci); u32 val; - u16 val_16; pp->bridge->ops = &tegra_pci_ops; @@ -900,11 +899,6 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp) pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, PCI_CAP_ID_EXP); - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); - val = dw_pcie_readl_dbi(pci, PCI_IO_BASE); val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8); dw_pcie_writel_dbi(pci, PCI_IO_BASE, val); @@ -1756,7 +1750,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) struct device *dev = pcie->dev; u32 val; int ret; - u16 val_16; if (pcie->ep_state == EP_STATE_ENABLED) return; @@ -1887,11 +1880,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) pcie->pcie_cap_base = dw_pcie_find_capability(&pcie->pci, PCI_CAP_ID_EXP); - val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL); - val_16 &= ~PCI_EXP_DEVCTL_PAYLOAD; - val_16 |= PCI_EXP_DEVCTL_PAYLOAD_256B; - dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_DEVCTL, val_16); - /* Clear Slot Clock Configuration bit if SRNS configuration */ if (pcie->enable_srns) { val_16 = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + @@ -1900,7 +1888,6 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie) dw_pcie_writew_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA, val_16); } - clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ); val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK);