diff mbox series

[V2,3/9] PCI/TPH: Implement a command line option to disable TPH

Message ID 20240531213841.3246055-4-wei.huang2@amd.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCIe TPH and cache direct injection support | expand

Commit Message

Wei Huang May 31, 2024, 9:38 p.m. UTC
Provide a kernel option, with related helper functions, to completely
disable TPH so that no TPH headers are generated.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 .../admin-guide/kernel-parameters.txt         |  1 +
 drivers/pci/pci-driver.c                      |  7 ++++-
 drivers/pci/pci.c                             | 12 ++++++++
 drivers/pci/pcie/tph.c                        | 30 +++++++++++++++++++
 include/linux/pci-tph.h                       | 19 ++++++++++++
 include/linux/pci.h                           |  1 +
 6 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/pci-tph.h

Comments

Jonathan Cameron June 7, 2024, 4:27 p.m. UTC | #1
On Fri, 31 May 2024 16:38:35 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> Provide a kernel option, with related helper functions, to completely
> disable TPH so that no TPH headers are generated.

Why would someone use this option?

> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  1 +
>  drivers/pci/pci-driver.c                      |  7 ++++-
>  drivers/pci/pci.c                             | 12 ++++++++
>  drivers/pci/pcie/tph.c                        | 30 +++++++++++++++++++
>  include/linux/pci-tph.h                       | 19 ++++++++++++
>  include/linux/pci.h                           |  1 +
>  6 files changed, 69 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/pci-tph.h
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 500cfa776225..fedcc69e35c1 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4623,6 +4623,7 @@
>  		nomio		[S390] Do not use MIO instructions.
>  		norid		[S390] ignore the RID field and force use of
>  				one PCI domain per PCI function
> +		notph		[PCIE] Do not use PCIe TPH
>  
>  	pcie_aspm=	[PCIE] Forcibly enable or ignore PCIe Active State Power
>  			Management.
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index af2996d0d17f..9722d070c0ca 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -21,6 +21,7 @@
>  #include <linux/acpi.h>
>  #include <linux/dma-map-ops.h>
>  #include <linux/iommu.h>
> +#include <linux/pci-tph.h>
>  #include "pci.h"
>  #include "pcie/portdrv.h"
>  
> @@ -322,8 +323,12 @@ static long local_pci_probe(void *_ddi)
>  	pm_runtime_get_sync(dev);
>  	pci_dev->driver = pci_drv;
>  	rc = pci_drv->probe(pci_dev, ddi->id);
> -	if (!rc)
> +	if (!rc) {
> +		if (pci_tph_disabled())
> +			pcie_tph_disable(pci_dev);
> +
>  		return rc;
> +	}
>  	if (rc < 0) {
>  		pci_dev->driver = NULL;
>  		pm_runtime_put_sync(dev);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 59e0949fb079..31c443504ce9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -157,6 +157,9 @@ static bool pcie_ari_disabled;
>  /* If set, the PCIe ATS capability will not be used. */
>  static bool pcie_ats_disabled;
>  
> +/* If set, the PCIe TPH capability will not be used. */
> +static bool pcie_tph_disabled;
> +
>  /* If set, the PCI config space of each device is printed during boot. */
>  bool pci_early_dump;
>  
> @@ -166,6 +169,12 @@ bool pci_ats_disabled(void)
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_disabled);
>  
> +bool pci_tph_disabled(void)
> +{
> +	return pcie_tph_disabled;
> +}
> +EXPORT_SYMBOL_GPL(pci_tph_disabled);
> +
>  /* Disable bridge_d3 for all PCIe ports */
>  static bool pci_bridge_d3_disable;
>  /* Force bridge_d3 for all PCIe ports */
> @@ -6806,6 +6815,9 @@ static int __init pci_setup(char *str)
>  				pci_no_domains();
>  			} else if (!strncmp(str, "noari", 5)) {
>  				pcie_ari_disabled = true;
> +			} else if (!strcmp(str, "notph")) {
> +				pr_info("PCIe: TPH is disabled\n");
> +				pcie_tph_disabled = true;
>  			} else if (!strncmp(str, "cbiosize=", 9)) {
>  				pci_cardbus_io_size = memparse(str + 9, &str);
>  			} else if (!strncmp(str, "cbmemsize=", 10)) {
> diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
> index 5f0cc06b74bb..5dc533b89a33 100644
> --- a/drivers/pci/pcie/tph.c
> +++ b/drivers/pci/pcie/tph.c
> @@ -16,11 +16,41 @@
>  #include <linux/errno.h>
>  #include <linux/msi.h>
>  #include <linux/pci.h>
> +#include <linux/pci-tph.h>
>  #include <linux/msi.h>
>  #include <linux/pci-acpi.h>
>  
>  #include "../pci.h"
>  
> +static int tph_set_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
> +				 u8 shift, u32 field)

I'm unconvinced this helper makes sense.  Do we do similar for other
PCI capabilities?

If it does make sense to have a field update, then provide
one alongside pci_read_config_dword() etc.

> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	if (!dev->tph_cap)
> +		return -EINVAL;
> +
> +	ret = pci_read_config_dword(dev, dev->tph_cap + offset, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	reg_val &= ~mask;
> +	reg_val |= (field << shift) & mask;
> +
> +	ret = pci_write_config_dword(dev, dev->tph_cap + offset, reg_val);
> +
> +	return ret;

	return pci_write_config_dword();

> +}
> +
> +int pcie_tph_disable(struct pci_dev *dev)
> +{
> +	return  tph_set_reg_field_u32(dev, PCI_TPH_CTRL,

extra space after return

> +				      PCI_TPH_CTRL_REQ_EN_MASK,
> +				      PCI_TPH_CTRL_REQ_EN_SHIFT,
> +				      PCI_TPH_REQ_DISABLE);
> +}
> +
>  void pcie_tph_init(struct pci_dev *dev)
>  {
>  	dev->tph_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
Bjorn Helgaas June 7, 2024, 7:59 p.m. UTC | #2
On Fri, May 31, 2024 at 04:38:35PM -0500, Wei Huang wrote:
> Provide a kernel option, with related helper functions, to completely
> disable TPH so that no TPH headers are generated.

If we need this option, say what the option is, why we need it, and
include an example of how to use it in the commit log.

Include the option in the subject line, too, e.g., "Add pci=notph
option to disable TPH".  Or maybe "prevent use of TPH"?  "Disable TPH"
hints that BIOS might have enabled it, and this option would turn that
off.  I haven't looked hard enough to know exactly what you intend or
whether there's a difference.
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 500cfa776225..fedcc69e35c1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4623,6 +4623,7 @@ 
 		nomio		[S390] Do not use MIO instructions.
 		norid		[S390] ignore the RID field and force use of
 				one PCI domain per PCI function
+		notph		[PCIE] Do not use PCIe TPH
 
 	pcie_aspm=	[PCIE] Forcibly enable or ignore PCIe Active State Power
 			Management.
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index af2996d0d17f..9722d070c0ca 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -21,6 +21,7 @@ 
 #include <linux/acpi.h>
 #include <linux/dma-map-ops.h>
 #include <linux/iommu.h>
+#include <linux/pci-tph.h>
 #include "pci.h"
 #include "pcie/portdrv.h"
 
@@ -322,8 +323,12 @@  static long local_pci_probe(void *_ddi)
 	pm_runtime_get_sync(dev);
 	pci_dev->driver = pci_drv;
 	rc = pci_drv->probe(pci_dev, ddi->id);
-	if (!rc)
+	if (!rc) {
+		if (pci_tph_disabled())
+			pcie_tph_disable(pci_dev);
+
 		return rc;
+	}
 	if (rc < 0) {
 		pci_dev->driver = NULL;
 		pm_runtime_put_sync(dev);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59e0949fb079..31c443504ce9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -157,6 +157,9 @@  static bool pcie_ari_disabled;
 /* If set, the PCIe ATS capability will not be used. */
 static bool pcie_ats_disabled;
 
+/* If set, the PCIe TPH capability will not be used. */
+static bool pcie_tph_disabled;
+
 /* If set, the PCI config space of each device is printed during boot. */
 bool pci_early_dump;
 
@@ -166,6 +169,12 @@  bool pci_ats_disabled(void)
 }
 EXPORT_SYMBOL_GPL(pci_ats_disabled);
 
+bool pci_tph_disabled(void)
+{
+	return pcie_tph_disabled;
+}
+EXPORT_SYMBOL_GPL(pci_tph_disabled);
+
 /* Disable bridge_d3 for all PCIe ports */
 static bool pci_bridge_d3_disable;
 /* Force bridge_d3 for all PCIe ports */
@@ -6806,6 +6815,9 @@  static int __init pci_setup(char *str)
 				pci_no_domains();
 			} else if (!strncmp(str, "noari", 5)) {
 				pcie_ari_disabled = true;
+			} else if (!strcmp(str, "notph")) {
+				pr_info("PCIe: TPH is disabled\n");
+				pcie_tph_disabled = true;
 			} else if (!strncmp(str, "cbiosize=", 9)) {
 				pci_cardbus_io_size = memparse(str + 9, &str);
 			} else if (!strncmp(str, "cbmemsize=", 10)) {
diff --git a/drivers/pci/pcie/tph.c b/drivers/pci/pcie/tph.c
index 5f0cc06b74bb..5dc533b89a33 100644
--- a/drivers/pci/pcie/tph.c
+++ b/drivers/pci/pcie/tph.c
@@ -16,11 +16,41 @@ 
 #include <linux/errno.h>
 #include <linux/msi.h>
 #include <linux/pci.h>
+#include <linux/pci-tph.h>
 #include <linux/msi.h>
 #include <linux/pci-acpi.h>
 
 #include "../pci.h"
 
+static int tph_set_reg_field_u32(struct pci_dev *dev, u8 offset, u32 mask,
+				 u8 shift, u32 field)
+{
+	u32 reg_val;
+	int ret;
+
+	if (!dev->tph_cap)
+		return -EINVAL;
+
+	ret = pci_read_config_dword(dev, dev->tph_cap + offset, &reg_val);
+	if (ret)
+		return ret;
+
+	reg_val &= ~mask;
+	reg_val |= (field << shift) & mask;
+
+	ret = pci_write_config_dword(dev, dev->tph_cap + offset, reg_val);
+
+	return ret;
+}
+
+int pcie_tph_disable(struct pci_dev *dev)
+{
+	return  tph_set_reg_field_u32(dev, PCI_TPH_CTRL,
+				      PCI_TPH_CTRL_REQ_EN_MASK,
+				      PCI_TPH_CTRL_REQ_EN_SHIFT,
+				      PCI_TPH_REQ_DISABLE);
+}
+
 void pcie_tph_init(struct pci_dev *dev)
 {
 	dev->tph_cap = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_TPH);
diff --git a/include/linux/pci-tph.h b/include/linux/pci-tph.h
new file mode 100644
index 000000000000..e187d7e89e8c
--- /dev/null
+++ b/include/linux/pci-tph.h
@@ -0,0 +1,19 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * TPH (TLP Processing Hints)
+ *
+ * Copyright (C) 2024 Advanced Micro Devices, Inc.
+ *     Eric Van Tassell <Eric.VanTassell@amd.com>
+ *     Wei Huang <wei.huang2@amd.com>
+ */
+#ifndef LINUX_PCI_TPH_H
+#define LINUX_PCI_TPH_H
+
+#ifdef CONFIG_PCIE_TPH
+int pcie_tph_disable(struct pci_dev *dev);
+#else
+static inline int pcie_tph_disable(struct pci_dev *dev)
+{ return -EOPNOTSUPP; }
+#endif
+
+#endif /* LINUX_PCI_TPH_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d75a88ec5136..d88ebe87815a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1841,6 +1841,7 @@  static inline bool pci_aer_available(void) { return false; }
 #endif
 
 bool pci_ats_disabled(void);
+bool pci_tph_disabled(void);
 
 #ifdef CONFIG_PCIE_PTM
 int pci_enable_ptm(struct pci_dev *dev, u8 *granularity);