[PATCH 0/7] PCI: Use FIELD_GET/PREP() & other reg field cleanups

[PATCH 0/7] PCI: Use FIELD_GET/PREP() & other reg field cleanups

[Ilpo Järvinen]

Hi,

Here's what I think should cover most of the remaining FIELD_GET/PREP()
conversions under drivers/pci/.

The patch from Bjorn is from
  https://lore.kernel.org/linux-pci/20231010204436.1000644-7-helgaas@kernel.org/
But has been adjusted to better blend in with the other DPC changes.
I've preserved Bjorn as the main From/SoB, and added myself before his
name instead (since I modified it but my main contribution was to
remove stuff he had made to it).

Bjorn Helgaas (1):
  PCI/DPC: Use FIELD_GET()

Ilpo Järvinen (6):
  PCI: cadence: Use FIELD_GET()
  PCI: dwc: Use FIELD_GET/PREP()
  PCI: hotplug: Use FIELD_GET/PREP()
  PCI/DPC: Use defined fields with DPC_CTL register
  PCI/DPC: Use defines with DPC reason fields
  PCI/MSI: Use FIELD_GET/PREP()

 .../pci/controller/cadence/pcie-cadence-ep.c  |  9 ++--
 .../pci/controller/dwc/pcie-designware-ep.c   |  7 ++--
 drivers/pci/controller/dwc/pcie-tegra194.c    |  5 +--
 drivers/pci/hotplug/pciehp_core.c             |  3 +-
 drivers/pci/hotplug/pciehp_hpc.c              |  5 ++-
 drivers/pci/hotplug/pnv_php.c                 |  3 +-
 drivers/pci/msi/msi.c                         | 10 +++--
 drivers/pci/pcie/dpc.c                        | 42 ++++++++++++-------
 drivers/pci/quirks.c                          |  2 +-
 include/uapi/linux/pci_regs.h                 |  9 ++++
 10 files changed, 61 insertions(+), 34 deletions(-)

-- 
2.30.2

[PATCH 1/7] PCI: cadence: Use FIELD_GET()

[Ilpo Järvinen]

Convert open-coded variants of PCI field access into FIELD_GET() to
make the code easier to understand.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/cadence/pcie-cadence-ep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index b8b655d4047e..3142feb8ac19 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -3,6 +3,7 @@
 // Cadence PCIe endpoint controller driver.
 // Author: Cyrille Pitchen <cyrille.pitchen@free-electrons.com>
 
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
@@ -262,7 +263,7 @@ static int cdns_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn)
 	 * Get the Multiple Message Enable bitfield from the Message Control
 	 * register.
 	 */
-	mme = (flags & PCI_MSI_FLAGS_QSIZE) >> 4;
+	mme = FIELD_GET(PCI_MSI_FLAGS_QSIZE, flags);
 
 	return mme;
 }
@@ -394,7 +395,7 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
 		return -EINVAL;
 
 	/* Get the number of enabled MSIs */
-	mme = (flags & PCI_MSI_FLAGS_QSIZE) >> 4;
+	mme = FIELD_GET(PCI_MSI_FLAGS_QSIZE, flags);
 	msi_count = 1 << mme;
 	if (!interrupt_num || interrupt_num > msi_count)
 		return -EINVAL;
@@ -449,7 +450,7 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
 		return -EINVAL;
 
 	/* Get the number of enabled MSIs */
-	mme = (flags & PCI_MSI_FLAGS_QSIZE) >> 4;
+	mme = FIELD_GET(PCI_MSI_FLAGS_QSIZE, flags);
 	msi_count = 1 << mme;
 	if (!interrupt_num || interrupt_num > msi_count)
 		return -EINVAL;
@@ -506,7 +507,7 @@ static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
 
 	reg = cap + PCI_MSIX_TABLE;
 	tbl_offset = cdns_pcie_ep_fn_readl(pcie, fn, reg);
-	bir = tbl_offset & PCI_MSIX_TABLE_BIR;
+	bir = FIELD_GET(PCI_MSIX_TABLE_BIR, tbl_offset);
 	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
 
 	msix_tbl = epf->epf_bar[bir]->addr + tbl_offset;
-- 
2.30.2

[PATCH 2/7] PCI: dwc: Use FIELD_GET/PREP()

[Ilpo Järvinen]

Convert open-coded variants of PCI field access into FIELD_GET/PREP()
to make the code easier to understand.

Add two missing defines into pci_regs.h. Logically, the Max No-Snoop
Latency Register is a separate word sized register in the PCIe spec,
but the pre-existing LTR defines in pci_regs.h with dword long values
seem to consider the registers together (the same goes for the only
user). Thus, follow the custom and make the new values also take both
word long LTR registers as a joint dword register.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 7 ++++---
 drivers/pci/controller/dwc/pcie-tegra194.c      | 5 ++---
 include/uapi/linux/pci_regs.h                   | 2 ++
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index f9182f8d552f..20bef1436bfb 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -6,6 +6,7 @@
  * Author: Kishon Vijay Abraham I <kishon@ti.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 
@@ -334,7 +335,7 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
 	if (!(val & PCI_MSI_FLAGS_ENABLE))
 		return -EINVAL;
 
-	val = (val & PCI_MSI_FLAGS_QSIZE) >> 4;
+	val = FIELD_GET(PCI_MSI_FLAGS_QSIZE, val);
 
 	return val;
 }
@@ -357,7 +358,7 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
 	reg = ep_func->msi_cap + func_offset + PCI_MSI_FLAGS;
 	val = dw_pcie_readw_dbi(pci, reg);
 	val &= ~PCI_MSI_FLAGS_QMASK;
-	val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
+	val |= FIELD_PREP(PCI_MSI_FLAGS_QMASK, interrupts);
 	dw_pcie_dbi_ro_wr_en(pci);
 	dw_pcie_writew_dbi(pci, reg, val);
 	dw_pcie_dbi_ro_wr_dis(pci);
@@ -584,7 +585,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
 
 	reg = ep_func->msix_cap + func_offset + PCI_MSIX_TABLE;
 	tbl_offset = dw_pcie_readl_dbi(pci, reg);
-	bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
+	bir = FIELD_GET(PCI_MSIX_TABLE_BIR, tbl_offset);
 	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
 
 	msix_tbl = ep->epf_bar[bir]->addr + tbl_offset;
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 248cd9347e8f..12d5ab2f5219 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -126,7 +126,6 @@
 
 #define APPL_LTR_MSG_1				0xC4
 #define LTR_MSG_REQ				BIT(15)
-#define LTR_MST_NO_SNOOP_SHIFT			16
 
 #define APPL_LTR_MSG_2				0xC8
 #define APPL_LTR_MSG_2_LTR_MSG_REQ_STATE	BIT(3)
@@ -496,8 +495,8 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
 		ktime_t timeout;
 
 		/* 110us for both snoop and no-snoop */
-		val = 110 | (2 << PCI_LTR_SCALE_SHIFT) | LTR_MSG_REQ;
-		val |= (val << LTR_MST_NO_SNOOP_SHIFT);
+		val = 110 | FIELD_PREP(PCI_LTR_SCALE_SHIFT, 2) | LTR_MSG_REQ;
+		val |= FIELD_PREP(PCI_LTR_NOSNOOP_VALUE, val);
 		appl_writel(pcie, val, APPL_LTR_MSG_1);
 
 		/* Send LTR upstream */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e5f558d96493..495f0ae4ecd5 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -975,6 +975,8 @@
 #define  PCI_LTR_VALUE_MASK	0x000003ff
 #define  PCI_LTR_SCALE_MASK	0x00001c00
 #define  PCI_LTR_SCALE_SHIFT	10
+#define  PCI_LTR_NOSNOOP_VALUE	0x03ff0000 /* Max No-Snoop Latency Value */
+#define  PCI_LTR_NOSNOOP_SCALE	0x1c000000 /* Scale for Max Value */
 #define PCI_EXT_CAP_LTR_SIZEOF	8
 
 /* Access Control Service */
-- 
2.30.2

[PATCH 3/7] PCI: hotplug: Use FIELD_GET/PREP()

[Ilpo Järvinen]

Instead of handcrafted shifts to handle register fields, use
FIELD_GET/FIELD_PREP().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_core.c | 3 ++-
 drivers/pci/hotplug/pciehp_hpc.c  | 5 +++--
 drivers/pci/hotplug/pnv_php.c     | 3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 4042d87d539d..ddd55ad97a58 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -20,6 +20,7 @@
 #define pr_fmt(fmt) "pciehp: " fmt
 #define dev_fmt pr_fmt
 
+#include <linux/bitfield.h>
 #include <linux/moduleparam.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -103,7 +104,7 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
 	struct pci_dev *pdev = ctrl->pcie->port;
 
 	if (status)
-		status <<= PCI_EXP_SLTCTL_ATTN_IND_SHIFT;
+		status = FIELD_PREP(PCI_EXP_SLTCTL_AIC, status);
 	else
 		status = PCI_EXP_SLTCTL_ATTN_IND_OFF;
 
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index fd713abdfb9f..b1d0a1b3917d 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -14,6 +14,7 @@
 
 #define dev_fmt(fmt) "pciehp: " fmt
 
+#include <linux/bitfield.h>
 #include <linux/dmi.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
@@ -484,7 +485,7 @@ int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot,
 	struct pci_dev *pdev = ctrl_dev(ctrl);
 
 	pci_config_pm_runtime_get(pdev);
-	pcie_write_cmd_nowait(ctrl, status << 6,
+	pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC, status),
 			      PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC);
 	pci_config_pm_runtime_put(pdev);
 	return 0;
@@ -1028,7 +1029,7 @@ struct controller *pcie_init(struct pcie_device *dev)
 		PCI_EXP_SLTSTA_DLLSC | PCI_EXP_SLTSTA_PDC);
 
 	ctrl_info(ctrl, "Slot #%d AttnBtn%c PwrCtrl%c MRL%c AttnInd%c PwrInd%c HotPlug%c Surprise%c Interlock%c NoCompl%c IbPresDis%c LLActRep%c%s\n",
-		(slot_cap & PCI_EXP_SLTCAP_PSN) >> 19,
+		FIELD_GET(PCI_EXP_SLTCAP_PSN, slot_cap),
 		FLAG(slot_cap, PCI_EXP_SLTCAP_ABP),
 		FLAG(slot_cap, PCI_EXP_SLTCAP_PCP),
 		FLAG(slot_cap, PCI_EXP_SLTCAP_MRLSP),
diff --git a/drivers/pci/hotplug/pnv_php.c b/drivers/pci/hotplug/pnv_php.c
index 881d420637bf..694349be9d0a 100644
--- a/drivers/pci/hotplug/pnv_php.c
+++ b/drivers/pci/hotplug/pnv_php.c
@@ -5,6 +5,7 @@
  * Copyright Gavin Shan, IBM Corporation 2016.
  */
 
+#include <linux/bitfield.h>
 #include <linux/libfdt.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -731,7 +732,7 @@ static int pnv_php_enable_msix(struct pnv_php_slot *php_slot)
 
 	/* Check hotplug MSIx entry is in range */
 	pcie_capability_read_word(pdev, PCI_EXP_FLAGS, &pcie_flag);
-	entry.entry = (pcie_flag & PCI_EXP_FLAGS_IRQ) >> 9;
+	entry.entry = FIELD_GET(PCI_EXP_FLAGS_IRQ, pcie_flag);
 	if (entry.entry >= nr_entries)
 		return -ERANGE;
 
-- 
2.30.2

[PATCH 4/7] PCI/DPC: Use FIELD_GET()

[Ilpo Järvinen]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependencies on the field position, i.e., the
shift value. No functional change intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/dpc.c        | 5 +++--
 drivers/pci/quirks.c          | 2 +-
 include/uapi/linux/pci_regs.h | 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..a5c259ada9ea 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -9,6 +9,7 @@
 #define dev_fmt(fmt) "DPC: " fmt
 
 #include <linux/aer.h>
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/init.h>
@@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
 
 	/* Get First Error Pointer */
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
-	first_error = (dpc_status & 0x1f00) >> 8;
+	first_error = FIELD_GET(PCI_EXP_DPC_RP_PIO_FEP, dpc_status);
 
 	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
 		if ((status & ~mask) & (1 << i))
@@ -338,7 +339,7 @@ void pci_dpc_init(struct pci_dev *pdev)
 	/* Quirks may set dpc_rp_log_size if device or firmware is buggy */
 	if (!pdev->dpc_rp_log_size) {
 		pdev->dpc_rp_log_size =
-			(cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
+				FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, cap);
 		if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
 			pci_err(pdev, "RP PIO log size %u is invalid\n",
 				pdev->dpc_rp_log_size);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index eeec1d6f9023..a9fdc2e3f110 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
 	if (!(val & PCI_EXP_DPC_CAP_RP_EXT))
 		return;
 
-	if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
+	if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
 		pci_info(dev, "Overriding RP PIO Log Size to 4\n");
 		dev->dpc_rp_log_size = 4;
 	}
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 495f0ae4ecd5..2d6df02a4b93 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1047,6 +1047,7 @@
 #define  PCI_EXP_DPC_STATUS_INTERRUPT	    0x0008 /* Interrupt Status */
 #define  PCI_EXP_DPC_RP_BUSY		    0x0010 /* Root Port Busy */
 #define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x0060 /* Trig Reason Extension */
+#define  PCI_EXP_DPC_RP_PIO_FEP		    0x1f00 /* Root Port PIO First Error Pointer */
 
 #define PCI_EXP_DPC_SOURCE_ID		 0x0A	/* DPC Source Identifier */
 
-- 
2.30.2

[PATCH 5/7] PCI/DPC: Use defined fields with DPC_CTL register

[Ilpo Järvinen]

Instead of using a literal to clear bits, add PCI_EXP_DPC_CTL_EN_MASK
and use the usual pattern to modify a bitfield.

While at it, rearrange RMW code more logically together.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/dpc.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index a5c259ada9ea..0048a11bd119 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -18,6 +18,9 @@
 #include "portdrv.h"
 #include "../pci.h"
 
+#define PCI_EXP_DPC_CTL_EN_MASK	(PCI_EXP_DPC_CTL_EN_FATAL | \
+				 PCI_EXP_DPC_CTL_EN_NONFATAL)
+
 static const char * const rp_pio_error_string[] = {
 	"Configuration Request received UR Completion",	 /* Bit Position 0  */
 	"Configuration Request received CA Completion",	 /* Bit Position 1  */
@@ -369,12 +372,13 @@ static int dpc_probe(struct pcie_device *dev)
 	}
 
 	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap);
-	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
 
-	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
+	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
+	ctl &= ~PCI_EXP_DPC_CTL_EN_MASK;
+	ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
 	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
-	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
 
+	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
 	pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n",
 		 cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT),
 		 FLAG(cap, PCI_EXP_DPC_CAP_POISONED_TLP),
-- 
2.30.2

[PATCH 6/7] PCI/DPC: Use defines with DPC reason fields

[Ilpo Järvinen]

Add new defines for DPC reason fields and use them instead of literals.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pcie/dpc.c        | 27 +++++++++++++++++----------
 include/uapi/linux/pci_regs.h |  6 ++++++
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 0048a11bd119..94111e438241 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -274,20 +274,27 @@ void dpc_process_error(struct pci_dev *pdev)
 	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
 		 status, source);
 
-	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
-	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
+	reason = status & PCI_EXP_DPC_STATUS_TRIGGER_RSN;
+	ext_reason = status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT;
 	pci_warn(pdev, "%s detected\n",
-		 (reason == 0) ? "unmasked uncorrectable error" :
-		 (reason == 1) ? "ERR_NONFATAL" :
-		 (reason == 2) ? "ERR_FATAL" :
-		 (ext_reason == 0) ? "RP PIO error" :
-		 (ext_reason == 1) ? "software trigger" :
-				     "reserved error");
+		 (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR) ?
+		 "unmasked uncorrectable error" :
+		 (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE) ?
+		 "ERR_NONFATAL" :
+		 (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE) ?
+		 "ERR_FATAL" :
+		 (ext_reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO) ?
+		 "RP PIO error" :
+		 (ext_reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_SW_TRIGGER) ?
+		 "software trigger" :
+		 "reserved error");
 
 	/* show RP PIO error detail information */
-	if (pdev->dpc_rp_extensions && reason == 3 && ext_reason == 0)
+	if (pdev->dpc_rp_extensions &&
+	    reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_IN_EXT &&
+	    ext_reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO)
 		dpc_process_rp_pio_error(pdev);
-	else if (reason == 0 &&
+	else if (reason == PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR &&
 		 dpc_get_aer_uncorrect_severity(pdev, &info) &&
 		 aer_get_device_error_info(pdev, &info)) {
 		aer_print_error(pdev, &info);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 2d6df02a4b93..c4d67ceae20d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1044,9 +1044,15 @@
 #define PCI_EXP_DPC_STATUS		0x08	/* DPC Status */
 #define  PCI_EXP_DPC_STATUS_TRIGGER	    0x0001 /* Trigger Status */
 #define  PCI_EXP_DPC_STATUS_TRIGGER_RSN	    0x0006 /* Trigger Reason */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR  0x0000 /* DPC due to unmasked uncorrectable error */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE    0x0002 /* DPC due to receiving ERR_NONFATAL */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE     0x0004 /* DPC due to receiving ERR_FATAL */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_IN_EXT 0x0006 /* Reason in Trig Reason Extension field */
 #define  PCI_EXP_DPC_STATUS_INTERRUPT	    0x0008 /* Interrupt Status */
 #define  PCI_EXP_DPC_RP_BUSY		    0x0010 /* Root Port Busy */
 #define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x0060 /* Trig Reason Extension */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO		0x0000	/* DPC due to RP PIO error */
+#define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_SW_TRIGGER	0x0020	/* DPC due to DPC SW Trigger bit */
 #define  PCI_EXP_DPC_RP_PIO_FEP		    0x1f00 /* Root Port PIO First Error Pointer */
 
 #define PCI_EXP_DPC_SOURCE_ID		 0x0A	/* DPC Source Identifier */
-- 
2.30.2

[PATCH 7/7] PCI/MSI: Use FIELD_GET/PREP()

[Ilpo Järvinen]

Instead of custom masking and shifting, use FIELD_GET/PREP() with
register fields.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/msi/msi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index ef1d8857a51b..682fa877478f 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -6,6 +6,7 @@
  * Copyright (C) Tom Long Nguyen (tom.l.nguyen@intel.com)
  * Copyright (C) 2016 Christoph Hellwig.
  */
+#include <linux/bitfield.h>
 #include <linux/err.h>
 #include <linux/export.h>
 #include <linux/irq.h>
@@ -188,7 +189,7 @@ static inline void pci_write_msg_msi(struct pci_dev *dev, struct msi_desc *desc,
 
 	pci_read_config_word(dev, pos + PCI_MSI_FLAGS, &msgctl);
 	msgctl &= ~PCI_MSI_FLAGS_QSIZE;
-	msgctl |= desc->pci.msi_attrib.multiple << 4;
+	msgctl |= FIELD_PREP(PCI_MSI_FLAGS_QSIZE, desc->pci.msi_attrib.multiple);
 	pci_write_config_word(dev, pos + PCI_MSI_FLAGS, msgctl);
 
 	pci_write_config_dword(dev, pos + PCI_MSI_ADDRESS_LO, msg->address_lo);
@@ -299,7 +300,7 @@ static int msi_setup_msi_desc(struct pci_dev *dev, int nvec,
 	desc.pci.msi_attrib.is_64	= !!(control & PCI_MSI_FLAGS_64BIT);
 	desc.pci.msi_attrib.can_mask	= !!(control & PCI_MSI_FLAGS_MASKBIT);
 	desc.pci.msi_attrib.default_irq	= dev->irq;
-	desc.pci.msi_attrib.multi_cap	= (control & PCI_MSI_FLAGS_QMASK) >> 1;
+	desc.pci.msi_attrib.multi_cap	= FIELD_GET(PCI_MSI_FLAGS_QMASK, control);
 	desc.pci.msi_attrib.multiple	= ilog2(__roundup_pow_of_two(nvec));
 	desc.affinity			= masks;
 
@@ -478,7 +479,7 @@ int pci_msi_vec_count(struct pci_dev *dev)
 		return -EINVAL;
 
 	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &msgctl);
-	ret = 1 << ((msgctl & PCI_MSI_FLAGS_QMASK) >> 1);
+	ret = 1 << FIELD_GET(PCI_MSI_FLAGS_QMASK, msgctl);
 
 	return ret;
 }
@@ -511,7 +512,8 @@ void __pci_restore_msi_state(struct pci_dev *dev)
 	pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &control);
 	pci_msi_update_mask(entry, 0, 0);
 	control &= ~PCI_MSI_FLAGS_QSIZE;
-	control |= (entry->pci.msi_attrib.multiple << 4) | PCI_MSI_FLAGS_ENABLE;
+	control |= PCI_MSI_FLAGS_ENABLE |
+		   FIELD_PREP(PCI_MSI_FLAGS_QSIZE, entry->pci.msi_attrib.multiple);
 	pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, control);
 }
 
-- 
2.30.2

Re: [PATCH 2/7] PCI: dwc: Use FIELD_GET/PREP()

[Serge Semin]

On Wed, Oct 18, 2023 at 02:32:49PM +0300, Ilpo Järvinen wrote:
> Convert open-coded variants of PCI field access into FIELD_GET/PREP()
> to make the code easier to understand.
> 
> Add two missing defines into pci_regs.h. Logically, the Max No-Snoop
> Latency Register is a separate word sized register in the PCIe spec,
> but the pre-existing LTR defines in pci_regs.h with dword long values
> seem to consider the registers together (the same goes for the only
> user). Thus, follow the custom and make the new values also take both
> word long LTR registers as a joint dword register.

Nice work. Thanks! Could you also have a look at
drivers/pci/controller/dwc/pcie-designware.c
?
It contains two open-coded patterns:
(bar << 8) - FIELD_PREP()
next_cap_ptr = (reg & 0xff00) >> 8; - FIELD_GET().
next_cap_ptr = (reg & 0x00ff); - FIELD_GET().
At least the later two statements concern the generic PCIe capability CSR.

-Serge(y)


> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-ep.c | 7 ++++---
>  drivers/pci/controller/dwc/pcie-tegra194.c      | 5 ++---
>  include/uapi/linux/pci_regs.h                   | 2 ++
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index f9182f8d552f..20bef1436bfb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -6,6 +6,7 @@
>   * Author: Kishon Vijay Abraham I <kishon@ti.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  
> @@ -334,7 +335,7 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
>  	if (!(val & PCI_MSI_FLAGS_ENABLE))
>  		return -EINVAL;
>  
> -	val = (val & PCI_MSI_FLAGS_QSIZE) >> 4;
> +	val = FIELD_GET(PCI_MSI_FLAGS_QSIZE, val);
>  
>  	return val;
>  }
> @@ -357,7 +358,7 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	reg = ep_func->msi_cap + func_offset + PCI_MSI_FLAGS;
>  	val = dw_pcie_readw_dbi(pci, reg);
>  	val &= ~PCI_MSI_FLAGS_QMASK;
> -	val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
> +	val |= FIELD_PREP(PCI_MSI_FLAGS_QMASK, interrupts);
>  	dw_pcie_dbi_ro_wr_en(pci);
>  	dw_pcie_writew_dbi(pci, reg, val);
>  	dw_pcie_dbi_ro_wr_dis(pci);
> @@ -584,7 +585,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
>  
>  	reg = ep_func->msix_cap + func_offset + PCI_MSIX_TABLE;
>  	tbl_offset = dw_pcie_readl_dbi(pci, reg);
> -	bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
> +	bir = FIELD_GET(PCI_MSIX_TABLE_BIR, tbl_offset);
>  	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
>  
>  	msix_tbl = ep->epf_bar[bir]->addr + tbl_offset;
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 248cd9347e8f..12d5ab2f5219 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -126,7 +126,6 @@
>  
>  #define APPL_LTR_MSG_1				0xC4
>  #define LTR_MSG_REQ				BIT(15)
> -#define LTR_MST_NO_SNOOP_SHIFT			16
>  
>  #define APPL_LTR_MSG_2				0xC8
>  #define APPL_LTR_MSG_2_LTR_MSG_REQ_STATE	BIT(3)
> @@ -496,8 +495,8 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
>  		ktime_t timeout;
>  
>  		/* 110us for both snoop and no-snoop */
> -		val = 110 | (2 << PCI_LTR_SCALE_SHIFT) | LTR_MSG_REQ;
> -		val |= (val << LTR_MST_NO_SNOOP_SHIFT);
> +		val = 110 | FIELD_PREP(PCI_LTR_SCALE_SHIFT, 2) | LTR_MSG_REQ;
> +		val |= FIELD_PREP(PCI_LTR_NOSNOOP_VALUE, val);
>  		appl_writel(pcie, val, APPL_LTR_MSG_1);
>  
>  		/* Send LTR upstream */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e5f558d96493..495f0ae4ecd5 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -975,6 +975,8 @@
>  #define  PCI_LTR_VALUE_MASK	0x000003ff
>  #define  PCI_LTR_SCALE_MASK	0x00001c00
>  #define  PCI_LTR_SCALE_SHIFT	10
> +#define  PCI_LTR_NOSNOOP_VALUE	0x03ff0000 /* Max No-Snoop Latency Value */
> +#define  PCI_LTR_NOSNOOP_SCALE	0x1c000000 /* Scale for Max Value */
>  #define PCI_EXT_CAP_LTR_SIZEOF	8
>  
>  /* Access Control Service */
> -- 
> 2.30.2
> 

Re: [PATCH 2/7] PCI: dwc: Use FIELD_GET/PREP()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5003 bytes --]

On Wed, 18 Oct 2023, Serge Semin wrote:

> On Wed, Oct 18, 2023 at 02:32:49PM +0300, Ilpo Järvinen wrote:
> > Convert open-coded variants of PCI field access into FIELD_GET/PREP()
> > to make the code easier to understand.
> > 
> > Add two missing defines into pci_regs.h. Logically, the Max No-Snoop
> > Latency Register is a separate word sized register in the PCIe spec,
> > but the pre-existing LTR defines in pci_regs.h with dword long values
> > seem to consider the registers together (the same goes for the only
> > user). Thus, follow the custom and make the new values also take both
> > word long LTR registers as a joint dword register.
> 
> Nice work. Thanks! Could you also have a look at
> drivers/pci/controller/dwc/pcie-designware.c
> ?
> It contains two open-coded patterns:
> (bar << 8) - FIELD_PREP()
> next_cap_ptr = (reg & 0xff00) >> 8; - FIELD_GET().
> next_cap_ptr = (reg & 0x00ff); - FIELD_GET().
> At least the later two statements concern the generic PCIe capability CSR.

The problem with cap id / next cap is that there are currently no defines 
for them AFAICT, at least not in pci_regs.h. And pci_regs.h defines those 
as different registers so if I'm to add defines from them, I don't know 
which size would be the most appropriate since that 0xff00 goes across 
that register boundary. I've not had time to study the related core code 
yet but I intend to take a look at it to see what's the best course of 
action forward.

-- 
 i.


> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-ep.c | 7 ++++---
> >  drivers/pci/controller/dwc/pcie-tegra194.c      | 5 ++---
> >  include/uapi/linux/pci_regs.h                   | 2 ++
> >  3 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index f9182f8d552f..20bef1436bfb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -6,6 +6,7 @@
> >   * Author: Kishon Vijay Abraham I <kishon@ti.com>
> >   */
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  
> > @@ -334,7 +335,7 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> >  	if (!(val & PCI_MSI_FLAGS_ENABLE))
> >  		return -EINVAL;
> >  
> > -	val = (val & PCI_MSI_FLAGS_QSIZE) >> 4;
> > +	val = FIELD_GET(PCI_MSI_FLAGS_QSIZE, val);
> >  
> >  	return val;
> >  }
> > @@ -357,7 +358,7 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> >  	reg = ep_func->msi_cap + func_offset + PCI_MSI_FLAGS;
> >  	val = dw_pcie_readw_dbi(pci, reg);
> >  	val &= ~PCI_MSI_FLAGS_QMASK;
> > -	val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
> > +	val |= FIELD_PREP(PCI_MSI_FLAGS_QMASK, interrupts);
> >  	dw_pcie_dbi_ro_wr_en(pci);
> >  	dw_pcie_writew_dbi(pci, reg, val);
> >  	dw_pcie_dbi_ro_wr_dis(pci);
> > @@ -584,7 +585,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> >  
> >  	reg = ep_func->msix_cap + func_offset + PCI_MSIX_TABLE;
> >  	tbl_offset = dw_pcie_readl_dbi(pci, reg);
> > -	bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
> > +	bir = FIELD_GET(PCI_MSIX_TABLE_BIR, tbl_offset);
> >  	tbl_offset &= PCI_MSIX_TABLE_OFFSET;
> >  
> >  	msix_tbl = ep->epf_bar[bir]->addr + tbl_offset;
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index 248cd9347e8f..12d5ab2f5219 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -126,7 +126,6 @@
> >  
> >  #define APPL_LTR_MSG_1				0xC4
> >  #define LTR_MSG_REQ				BIT(15)
> > -#define LTR_MST_NO_SNOOP_SHIFT			16
> >  
> >  #define APPL_LTR_MSG_2				0xC8
> >  #define APPL_LTR_MSG_2_LTR_MSG_REQ_STATE	BIT(3)
> > @@ -496,8 +495,8 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
> >  		ktime_t timeout;
> >  
> >  		/* 110us for both snoop and no-snoop */
> > -		val = 110 | (2 << PCI_LTR_SCALE_SHIFT) | LTR_MSG_REQ;
> > -		val |= (val << LTR_MST_NO_SNOOP_SHIFT);
> > +		val = 110 | FIELD_PREP(PCI_LTR_SCALE_SHIFT, 2) | LTR_MSG_REQ;
> > +		val |= FIELD_PREP(PCI_LTR_NOSNOOP_VALUE, val);
> >  		appl_writel(pcie, val, APPL_LTR_MSG_1);
> >  
> >  		/* Send LTR upstream */
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index e5f558d96493..495f0ae4ecd5 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -975,6 +975,8 @@
> >  #define  PCI_LTR_VALUE_MASK	0x000003ff
> >  #define  PCI_LTR_SCALE_MASK	0x00001c00
> >  #define  PCI_LTR_SCALE_SHIFT	10
> > +#define  PCI_LTR_NOSNOOP_VALUE	0x03ff0000 /* Max No-Snoop Latency Value */
> > +#define  PCI_LTR_NOSNOOP_SCALE	0x1c000000 /* Scale for Max Value */
> >  #define PCI_EXT_CAP_LTR_SIZEOF	8
> >  
> >  /* Access Control Service */
> > -- 
> > 2.30.2
> > 
> 

Re: [PATCH 0/7] PCI: Use FIELD_GET/PREP() & other reg field cleanups

[Bjorn Helgaas]

On Wed, Oct 18, 2023 at 02:32:47PM +0300, Ilpo Järvinen wrote:
> Hi,
> 
> Here's what I think should cover most of the remaining FIELD_GET/PREP()
> conversions under drivers/pci/.
> 
> The patch from Bjorn is from
>   https://lore.kernel.org/linux-pci/20231010204436.1000644-7-helgaas@kernel.org/
> But has been adjusted to better blend in with the other DPC changes.
> I've preserved Bjorn as the main From/SoB, and added myself before his
> name instead (since I modified it but my main contribution was to
> remove stuff he had made to it).
> 
> Bjorn Helgaas (1):
>   PCI/DPC: Use FIELD_GET()
> 
> Ilpo Järvinen (6):
>   PCI: cadence: Use FIELD_GET()
>   PCI: dwc: Use FIELD_GET/PREP()
>   PCI: hotplug: Use FIELD_GET/PREP()
>   PCI/DPC: Use defined fields with DPC_CTL register
>   PCI/DPC: Use defines with DPC reason fields
>   PCI/MSI: Use FIELD_GET/PREP()
> 
>  .../pci/controller/cadence/pcie-cadence-ep.c  |  9 ++--
>  .../pci/controller/dwc/pcie-designware-ep.c   |  7 ++--
>  drivers/pci/controller/dwc/pcie-tegra194.c    |  5 +--
>  drivers/pci/hotplug/pciehp_core.c             |  3 +-
>  drivers/pci/hotplug/pciehp_hpc.c              |  5 ++-
>  drivers/pci/hotplug/pnv_php.c                 |  3 +-
>  drivers/pci/msi/msi.c                         | 10 +++--
>  drivers/pci/pcie/dpc.c                        | 42 ++++++++++++-------
>  drivers/pci/quirks.c                          |  2 +-
>  include/uapi/linux/pci_regs.h                 |  9 ++++
>  10 files changed, 61 insertions(+), 34 deletions(-)

Applied to pci/field-get for v6.7, thanks, Ilpo!