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/[email protected]/
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
Convert open-coded variants of PCI field access into FIELD_GET() to
make the code easier to understand.
Signed-off-by: Ilpo Järvinen <[email protected]>
---
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 <[email protected]>
+#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
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 <[email protected]>
---
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 <[email protected]>
*/
+#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
From: Bjorn Helgaas <[email protected]>
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 <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
---
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
Instead of handcrafted shifts to handle register fields, use
FIELD_GET/FIELD_PREP().
Signed-off-by: Ilpo Järvinen <[email protected]>
---
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
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 <[email protected]>
---
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
Instead of custom masking and shifting, use FIELD_GET/PREP() with
register fields.
Signed-off-by: Ilpo Järvinen <[email protected]>
---
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 ([email protected])
* 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
Add new defines for DPC reason fields and use them instead of literals.
Signed-off-by: Ilpo Järvinen <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
> */
>
> +#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
>
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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > */
> >
> > +#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
> >
>
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/[email protected]/
> 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!