From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() and FIELD_PREP() to avoid the need for shifting.
These apply on top of the PCI patches from Ilpo's series:
https://lore.kernel.org/r/[email protected]
Bjorn Helgaas (10):
PCI: Use FIELD_GET()
PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk
PCI/ASPM: Use FIELD_GET()
PCI/ATS: Show PASID Capability register width in bitmasks
PCI/ATS: Use FIELD_GET()
PCI/DPC: Use FIELD_GET()
PCI/PME: Use FIELD_GET()
PCI/PTM: Use FIELD_GET()
PCI/VC: Use FIELD_GET()
PCI/portdrv: Use FIELD_GET()
drivers/pci/ats.c | 7 ++---
drivers/pci/pci.c | 53 +++++++++++++++++------------------
drivers/pci/pcie/aspm.c | 31 +++++++++++---------
drivers/pci/pcie/dpc.c | 9 +++---
drivers/pci/pcie/pme.c | 4 ++-
drivers/pci/pcie/portdrv.c | 7 +++--
drivers/pci/pcie/ptm.c | 5 ++--
drivers/pci/probe.c | 8 +++---
drivers/pci/quirks.c | 2 +-
drivers/pci/vc.c | 9 +++---
include/uapi/linux/pci_regs.h | 15 ++++++----
11 files changed, 81 insertions(+), 69 deletions(-)
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/pci.c | 45 ++++++++++++++++++++++-----------------------
drivers/pci/probe.c | 8 ++++----
2 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a8adc34dc86f..848c9ee65d7f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1776,8 +1776,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
return;
pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
- nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
- PCI_REBAR_CTRL_NBAR_SHIFT;
+ nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
for (i = 0; i < nbars; i++, pos += 8) {
struct resource *res;
@@ -1788,7 +1787,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
res = pdev->resource + bar_idx;
size = pci_rebar_bytes_to_size(resource_size(res));
ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
- ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
+ ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
}
}
@@ -3229,7 +3228,7 @@ void pci_pm_init(struct pci_dev *dev)
(pmc & PCI_PM_CAP_PME_D2) ? " D2" : "",
(pmc & PCI_PM_CAP_PME_D3hot) ? " D3hot" : "",
(pmc & PCI_PM_CAP_PME_D3cold) ? " D3cold" : "");
- dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;
+ dev->pme_support = FIELD_GET(PCI_PM_CAP_PME_MASK, pmc);
dev->pme_poll = true;
/*
* Make device's PM flags reflect the wake-up capability, but
@@ -3300,20 +3299,20 @@ static int pci_ea_read(struct pci_dev *dev, int offset)
ent_offset += 4;
/* Entry size field indicates DWORDs after 1st */
- ent_size = ((dw0 & PCI_EA_ES) + 1) << 2;
+ ent_size = (FIELD_GET(PCI_EA_ES, dw0) + 1) << 2;
if (!(dw0 & PCI_EA_ENABLE)) /* Entry not enabled */
goto out;
- bei = (dw0 & PCI_EA_BEI) >> 4;
- prop = (dw0 & PCI_EA_PP) >> 8;
+ bei = FIELD_GET(PCI_EA_BEI, dw0);
+ prop = FIELD_GET(PCI_EA_PP, dw0);
/*
* If the Property is in the reserved range, try the Secondary
* Property instead.
*/
if (prop > PCI_EA_P_BRIDGE_IO && prop < PCI_EA_P_MEM_RESERVED)
- prop = (dw0 & PCI_EA_SP) >> 16;
+ prop = FIELD_GET(PCI_EA_SP, dw0);
if (prop > PCI_EA_P_BRIDGE_IO)
goto out;
@@ -3720,14 +3719,13 @@ static int pci_rebar_find_pos(struct pci_dev *pdev, int bar)
return -ENOTSUPP;
pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
- nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
- PCI_REBAR_CTRL_NBAR_SHIFT;
+ nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
for (i = 0; i < nbars; i++, pos += 8) {
int bar_idx;
pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
- bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
+ bar_idx = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, ctrl);
if (bar_idx == bar)
return pos;
}
@@ -3782,7 +3780,7 @@ int pci_rebar_get_current_size(struct pci_dev *pdev, int bar)
return pos;
pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
- return (ctrl & PCI_REBAR_CTRL_BAR_SIZE) >> PCI_REBAR_CTRL_BAR_SHIFT;
+ return FIELD_GET(PCI_REBAR_CTRL_BAR_SIZE, ctrl);
}
/**
@@ -3805,7 +3803,7 @@ int pci_rebar_set_size(struct pci_dev *pdev, int bar, int size)
pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
- ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
+ ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
return 0;
}
@@ -6043,7 +6041,7 @@ int pcix_get_max_mmrbc(struct pci_dev *dev)
if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
return -EINVAL;
- return 512 << ((stat & PCI_X_STATUS_MAX_READ) >> 21);
+ return 512 << FIELD_GET(PCI_X_STATUS_MAX_READ, stat);
}
EXPORT_SYMBOL(pcix_get_max_mmrbc);
@@ -6066,7 +6064,7 @@ int pcix_get_mmrbc(struct pci_dev *dev)
if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
return -EINVAL;
- return 512 << ((cmd & PCI_X_CMD_MAX_READ) >> 2);
+ return 512 << FIELD_GET(PCI_X_CMD_MAX_READ, cmd);
}
EXPORT_SYMBOL(pcix_get_mmrbc);
@@ -6097,19 +6095,19 @@ int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
return -EINVAL;
- if (v > (stat & PCI_X_STATUS_MAX_READ) >> 21)
+ if (v > FIELD_GET(PCI_X_STATUS_MAX_READ, stat))
return -E2BIG;
if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
return -EINVAL;
- o = (cmd & PCI_X_CMD_MAX_READ) >> 2;
+ o = FIELD_GET(PCI_X_CMD_MAX_READ, cmd);
if (o != v) {
if (v > o && (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MMRBC))
return -EIO;
cmd &= ~PCI_X_CMD_MAX_READ;
- cmd |= v << 2;
+ cmd |= FIELD_PREP(PCI_X_CMD_MAX_READ, v);
if (pci_write_config_word(dev, cap + PCI_X_CMD, cmd))
return -EIO;
}
@@ -6129,7 +6127,7 @@ int pcie_get_readrq(struct pci_dev *dev)
pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
- return 128 << ((ctl & PCI_EXP_DEVCTL_READRQ) >> 12);
+ return 128 << FIELD_GET(PCI_EXP_DEVCTL_READRQ, ctl);
}
EXPORT_SYMBOL(pcie_get_readrq);
@@ -6162,7 +6160,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
rq = mps;
}
- v = (ffs(rq) - 8) << 12;
+ v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8);
if (bridge->no_inc_mrrs) {
int max_mrrs = pcie_get_readrq(dev);
@@ -6192,7 +6190,7 @@ int pcie_get_mps(struct pci_dev *dev)
pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
- return 128 << ((ctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5);
+ return 128 << FIELD_GET(PCI_EXP_DEVCTL_PAYLOAD, ctl);
}
EXPORT_SYMBOL(pcie_get_mps);
@@ -6215,7 +6213,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
v = ffs(mps) - 8;
if (v > dev->pcie_mpss)
return -EINVAL;
- v <<= 5;
+ v = FIELD_PREP(PCI_EXP_DEVCTL_PAYLOAD, v);
ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
PCI_EXP_DEVCTL_PAYLOAD, v);
@@ -6257,7 +6255,8 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
while (dev) {
pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
- next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
+ next_speed = pcie_link_speed[FIELD_GET(PCI_EXP_LNKSTA_CLS,
+ lnksta)];
next_width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 795534589b98..2036c3a120ee 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -807,8 +807,8 @@ static void pci_set_bus_speed(struct pci_bus *bus)
}
bus->max_bus_speed = max;
- bus->cur_bus_speed = pcix_bus_speed[
- (status & PCI_X_SSTATUS_FREQ) >> 6];
+ bus->cur_bus_speed =
+ pcix_bus_speed[FIELD_GET(PCI_X_SSTATUS_FREQ, status)];
return;
}
@@ -1217,8 +1217,8 @@ static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub)
offset = ea + PCI_EA_FIRST_ENT;
pci_read_config_dword(dev, offset, &dw);
- ea_sec = dw & PCI_EA_SEC_BUS_MASK;
- ea_sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
+ ea_sec = FIELD_GET(PCI_EA_SEC_BUS_MASK, dw);
+ ea_sub = FIELD_GET(PCI_EA_SUB_BUS_MASK, dw);
if (ea_sec == 0 || ea_sub < ea_sec)
return false;
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Separate because this isn't as trivial as the other FIELD_GET() changes.
See 907830b0fc9e ("PCI: Add a REBAR size quirk for Sapphire RX 5600 XT
Pulse")
Signed-off-by: Bjorn Helgaas <[email protected]>
Cc: Nirmoy Das <[email protected]>
---
drivers/pci/pci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 848c9ee65d7f..5dc6e7cdfb71 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3751,14 +3751,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar)
return 0;
pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
- cap &= PCI_REBAR_CAP_SIZES;
+ cap = FIELD_GET(PCI_REBAR_CAP_SIZES, cap);
/* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */
if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f &&
- bar == 0 && cap == 0x7000)
- cap = 0x3f000;
+ bar == 0 && cap == 0x700)
+ return 0x3f00;
- return cap >> 4;
+ return cap;
}
EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/pcie/pme.c | 4 +++-
include/uapi/linux/pci_regs.h | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index ef8ce436ead9..a2daebd9806c 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -9,6 +9,7 @@
#define dev_fmt(fmt) "PME: " fmt
+#include <linux/bitfield.h>
#include <linux/pci.h>
#include <linux/kernel.h>
#include <linux/errno.h>
@@ -235,7 +236,8 @@ static void pcie_pme_work_fn(struct work_struct *work)
pcie_clear_root_pme_status(port);
spin_unlock_irq(&data->lock);
- pcie_pme_handle_request(port, rtsta & 0xffff);
+ pcie_pme_handle_request(port,
+ FIELD_GET(PCI_EXP_RTSTA_PME_RQ_ID, rtsta));
spin_lock_irq(&data->lock);
continue;
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e97a06b50f95..9fb8a69241f4 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -637,6 +637,7 @@
#define PCI_EXP_RTCAP 0x1e /* Root Capabilities */
#define PCI_EXP_RTCAP_CRSVIS 0x0001 /* CRS Software Visibility capability */
#define PCI_EXP_RTSTA 0x20 /* Root Status */
+#define PCI_EXP_RTSTA_PME_RQ_ID 0x0000ffff /* PME Requester ID */
#define PCI_EXP_RTSTA_PME 0x00010000 /* PME status */
#define PCI_EXP_RTSTA_PENDING 0x00020000 /* PME pending */
/*
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Add #defines for T_POWER_ON in the L1 PM Substates Capability and use
FIELD_PREP() and FIELD_GET() when possible. These remove the need for
explicit shifts. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/pcie/aspm.c | 31 ++++++++++++++++++-------------
include/uapi/linux/pci_regs.h | 2 ++
2 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 1bf630059264..06f175d8dee5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -7,6 +7,7 @@
* Copyright (C) Shaohua Li ([email protected])
*/
+#include <linux/bitfield.h>
#include <linux/kernel.h>
#include <linux/math.h>
#include <linux/module.h>
@@ -267,7 +268,7 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
/* Convert L0s latency encoding to ns */
static u32 calc_l0s_latency(u32 lnkcap)
{
- u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L0SEL) >> 12;
+ u32 encoding = FIELD_GET(PCI_EXP_LNKCAP_L0SEL, lnkcap);
if (encoding == 0x7)
return (5 * 1000); /* > 4us */
@@ -285,7 +286,7 @@ static u32 calc_l0s_acceptable(u32 encoding)
/* Convert L1 latency encoding to ns */
static u32 calc_l1_latency(u32 lnkcap)
{
- u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L1EL) >> 15;
+ u32 encoding = FIELD_GET(PCI_EXP_LNKCAP_L1EL, lnkcap);
if (encoding == 0x7)
return (65 * 1000); /* > 64us */
@@ -371,11 +372,11 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
link = endpoint->bus->self->link_state;
/* Calculate endpoint L0s acceptable latency */
- encoding = (endpoint->devcap & PCI_EXP_DEVCAP_L0S) >> 6;
+ encoding = FIELD_GET(PCI_EXP_DEVCAP_L0S, endpoint->devcap);
acceptable_l0s = calc_l0s_acceptable(encoding);
/* Calculate endpoint L1 acceptable latency */
- encoding = (endpoint->devcap & PCI_EXP_DEVCAP_L1) >> 9;
+ encoding = FIELD_GET(PCI_EXP_DEVCAP_L1, endpoint->devcap);
acceptable_l1 = calc_l1_acceptable(encoding);
while (link) {
@@ -446,22 +447,24 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
u32 pl1_2_enables, cl1_2_enables;
/* Choose the greater of the two Port Common_Mode_Restore_Times */
- val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
- val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
+ val1 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, parent_l1ss_cap);
+ val2 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, child_l1ss_cap);
t_common_mode = max(val1, val2);
/* Choose the greater of the two Port T_POWER_ON times */
- val1 = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
- scale1 = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
- val2 = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
- scale2 = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
+ val1 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, parent_l1ss_cap);
+ scale1 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, parent_l1ss_cap);
+ val2 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, child_l1ss_cap);
+ scale2 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, child_l1ss_cap);
if (calc_l12_pwron(parent, scale1, val1) >
calc_l12_pwron(child, scale2, val2)) {
- ctl2 |= scale1 | (val1 << 3);
+ ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale1) |
+ FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_VALUE, val1);
t_power_on = calc_l12_pwron(parent, scale1, val1);
} else {
- ctl2 |= scale2 | (val2 << 3);
+ ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale2) |
+ FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_VALUE, val2);
t_power_on = calc_l12_pwron(child, scale2, val2);
}
@@ -477,7 +480,9 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
*/
l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
encode_l12_threshold(l1_2_threshold, &scale, &value);
- ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
+ ctl1 |= FIELD_PREP(PCI_L1SS_CTL1_CM_RESTORE_TIME, t_common_mode) |
+ FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_VALUE, value) |
+ FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_SCALE, scale);
/* Some broken devices only support dword access to L1 SS */
pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e5f558d96493..34bf037993f3 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1088,6 +1088,8 @@
#define PCI_L1SS_CTL1_LTR_L12_TH_VALUE 0x03ff0000 /* LTR_L1.2_THRESHOLD_Value */
#define PCI_L1SS_CTL1_LTR_L12_TH_SCALE 0xe0000000 /* LTR_L1.2_THRESHOLD_Scale */
#define PCI_L1SS_CTL2 0x0c /* Control 2 Register */
+#define PCI_L1SS_CTL2_T_PWR_ON_SCALE 0x00000003 /* T_POWER_ON Scale */
+#define PCI_L1SS_CTL2_T_PWR_ON_VALUE 0x000000f8 /* T_POWER_ON Value */
/* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
#define PCI_DVSEC_HEADER1 0x4 /* Designated Vendor-Specific Header1 */
--
2.34.1
From: Bjorn Helgaas <[email protected]>
The PASID Capability and Control registers are both 16 bits wide. Use
16-bit wide constants in field names to match the register width. No
functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
include/uapi/linux/pci_regs.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 34bf037993f3..6af1f8d53e97 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -930,12 +930,12 @@
/* Process Address Space ID */
#define PCI_PASID_CAP 0x04 /* PASID feature register */
-#define PCI_PASID_CAP_EXEC 0x02 /* Exec permissions Supported */
-#define PCI_PASID_CAP_PRIV 0x04 /* Privilege Mode Supported */
+#define PCI_PASID_CAP_EXEC 0x0002 /* Exec permissions Supported */
+#define PCI_PASID_CAP_PRIV 0x0004 /* Privilege Mode Supported */
#define PCI_PASID_CTRL 0x06 /* PASID control register */
-#define PCI_PASID_CTRL_ENABLE 0x01 /* Enable bit */
-#define PCI_PASID_CTRL_EXEC 0x02 /* Exec permissions Enable */
-#define PCI_PASID_CTRL_PRIV 0x04 /* Privilege Mode Enable */
+#define PCI_PASID_CTRL_ENABLE 0x0001 /* Enable bit */
+#define PCI_PASID_CTRL_EXEC 0x0002 /* Exec permissions Enable */
+#define PCI_PASID_CTRL_PRIV 0x0004 /* Privilege Mode Enable */
#define PCI_EXT_CAP_PASID_SIZEOF 8
/* Single Root I/O Virtualization */
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/pcie/dpc.c | 9 +++++----
drivers/pci/quirks.c | 2 +-
include/uapi/linux/pci_regs.h | 1 +
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..6e551f34ec63 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -8,6 +8,7 @@
#define dev_fmt(fmt) "DPC: " fmt
+#include <linux/bitfield.h>
#include <linux/aer.h>
#include <linux/delay.h>
#include <linux/interrupt.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_STATUS_FIRST_ERR, dpc_status);
for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
if ((status & ~mask) & (1 << i))
@@ -270,8 +271,8 @@ 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 = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
+ ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
pci_warn(pdev, "%s detected\n",
(reason == 0) ? "unmasked uncorrectable error" :
(reason == 1) ? "ERR_NONFATAL" :
@@ -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 833e5fb40ea5..e97a06b50f95 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1046,6 +1046,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_STATUS_FIRST_ERR 0x1f00 /* RP PIO First Error Ptr */
#define PCI_EXP_DPC_SOURCE_ID 0x0A /* DPC Source Identifier */
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/ats.c | 7 ++-----
include/uapi/linux/pci_regs.h | 1 +
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index f9cc2e10b676..c570892b2090 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -9,6 +9,7 @@
* Copyright (C) 2011 Advanced Micro Devices,
*/
+#include <linux/bitfield.h>
#include <linux/export.h>
#include <linux/pci-ats.h>
#include <linux/pci.h>
@@ -480,8 +481,6 @@ int pci_pasid_features(struct pci_dev *pdev)
}
EXPORT_SYMBOL_GPL(pci_pasid_features);
-#define PASID_NUMBER_SHIFT 8
-#define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT)
/**
* pci_max_pasids - Get maximum number of PASIDs supported by device
* @pdev: PCI device structure
@@ -503,9 +502,7 @@ int pci_max_pasids(struct pci_dev *pdev)
pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
- supported = (supported & PASID_NUMBER_MASK) >> PASID_NUMBER_SHIFT;
-
- return (1 << supported);
+ return (1 << FIELD_GET(PCI_PASID_CAP_WIDTH, supported));
}
EXPORT_SYMBOL_GPL(pci_max_pasids);
#endif /* CONFIG_PCI_PASID */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 6af1f8d53e97..833e5fb40ea5 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -932,6 +932,7 @@
#define PCI_PASID_CAP 0x04 /* PASID feature register */
#define PCI_PASID_CAP_EXEC 0x0002 /* Exec permissions Supported */
#define PCI_PASID_CAP_PRIV 0x0004 /* Privilege Mode Supported */
+#define PCI_PASID_CAP_WIDTH 0x1f00
#define PCI_PASID_CTRL 0x06 /* PASID control register */
#define PCI_PASID_CTRL_ENABLE 0x0001 /* Enable bit */
#define PCI_PASID_CTRL_EXEC 0x0002 /* Exec permissions Enable */
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/vc.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c
index 5fc59ac31145..a4ff7f5f66dd 100644
--- a/drivers/pci/vc.c
+++ b/drivers/pci/vc.c
@@ -6,6 +6,7 @@
* Author: Alex Williamson <[email protected]>
*/
+#include <linux/bitfield.h>
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -201,9 +202,9 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
/* Extended VC Count (not counting VC0) */
evcc = cap1 & PCI_VC_CAP1_EVCC;
/* Low Priority Extended VC Count (not counting VC0) */
- lpevcc = (cap1 & PCI_VC_CAP1_LPEVCC) >> 4;
+ lpevcc = FIELD_GET(PCI_VC_CAP1_LPEVCC, cap1);
/* Port Arbitration Table Entry Size (bits) */
- parb_size = 1 << ((cap1 & PCI_VC_CAP1_ARB_SIZE) >> 10);
+ parb_size = 1 << FIELD_GET(PCI_VC_CAP1_ARB_SIZE, cap1);
/*
* Port VC Control Register contains VC Arbitration Select, which
@@ -231,7 +232,7 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
int vcarb_offset;
pci_read_config_dword(dev, pos + PCI_VC_PORT_CAP2, &cap2);
- vcarb_offset = ((cap2 & PCI_VC_CAP2_ARB_OFF) >> 24) * 16;
+ vcarb_offset = FIELD_GET(PCI_VC_CAP2_ARB_OFF, cap2) * 16;
if (vcarb_offset) {
int size, vcarb_phases = 0;
@@ -277,7 +278,7 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
pci_read_config_dword(dev, pos + PCI_VC_RES_CAP +
(i * PCI_CAP_VC_PER_VC_SIZEOF), &cap);
- parb_offset = ((cap & PCI_VC_RES_CAP_ARB_OFF) >> 24) * 16;
+ parb_offset = FIELD_GET(PCI_VC_RES_CAP_ARB_OFF, cap) * 16;
if (parb_offset) {
int size, parb_phases = 0;
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/pcie/portdrv.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 46fad0d813b2..14a4b89a3b83 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -6,6 +6,7 @@
* Copyright (C) Tom Long Nguyen ([email protected])
*/
+#include <linux/bitfield.h>
#include <linux/dmi.h>
#include <linux/init.h>
#include <linux/module.h>
@@ -69,7 +70,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP |
PCIE_PORT_SERVICE_BWNOTIF)) {
pcie_capability_read_word(dev, PCI_EXP_FLAGS, ®16);
- *pme = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
+ *pme = FIELD_GET(PCI_EXP_FLAGS_IRQ, reg16);
nvec = *pme + 1;
}
@@ -81,7 +82,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
if (pos) {
pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS,
®32);
- *aer = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;
+ *aer = FIELD_GET(PCI_ERR_ROOT_AER_IRQ, reg32);
nvec = max(nvec, *aer + 1);
}
}
@@ -92,7 +93,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
if (pos) {
pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP,
®16);
- *dpc = reg16 & PCI_EXP_DPC_IRQ;
+ *dpc = FIELD_GET(PCI_EXP_DPC_IRQ, reg16);
nvec = max(nvec, *dpc + 1);
}
}
--
2.34.1
From: Bjorn Helgaas <[email protected]>
Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/pci/pcie/ptm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index b4e5f553467c..7cfb6c0d5dcb 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -4,6 +4,7 @@
* Copyright (c) 2016, Intel Corporation.
*/
+#include <linux/bitfield.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/pci.h>
@@ -53,7 +54,7 @@ void pci_ptm_init(struct pci_dev *dev)
pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u32));
pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
- dev->ptm_granularity = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
+ dev->ptm_granularity = FIELD_GET(PCI_PTM_GRANULARITY_MASK, cap);
/*
* Per the spec recommendation (PCIe r6.0, sec 7.9.15.3), select the
@@ -146,7 +147,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
ctrl |= PCI_PTM_CTRL_ENABLE;
ctrl &= ~PCI_PTM_GRANULARITY_MASK;
- ctrl |= dev->ptm_granularity << 8;
+ ctrl |= FIELD_PREP(PCI_PTM_GRANULARITY_MASK, dev->ptm_granularity);
if (dev->ptm_root)
ctrl |= PCI_PTM_CTRL_ROOT;
--
2.34.1
On Tue, Oct 10, 2023 at 03:44:29PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Add #defines for T_POWER_ON in the L1 PM Substates Capability and use
> FIELD_PREP() and FIELD_GET() when possible. These remove the need for
> explicit shifts. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
I see this is identical to your patch at
https://lore.kernel.org/r/[email protected],
Ilpo, so I'll drop this one.
> ---
> drivers/pci/pcie/aspm.c | 31 ++++++++++++++++++-------------
> include/uapi/linux/pci_regs.h | 2 ++
> 2 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 1bf630059264..06f175d8dee5 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -7,6 +7,7 @@
> * Copyright (C) Shaohua Li ([email protected])
> */
>
> +#include <linux/bitfield.h>
> #include <linux/kernel.h>
> #include <linux/math.h>
> #include <linux/module.h>
> @@ -267,7 +268,7 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
> /* Convert L0s latency encoding to ns */
> static u32 calc_l0s_latency(u32 lnkcap)
> {
> - u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L0SEL) >> 12;
> + u32 encoding = FIELD_GET(PCI_EXP_LNKCAP_L0SEL, lnkcap);
>
> if (encoding == 0x7)
> return (5 * 1000); /* > 4us */
> @@ -285,7 +286,7 @@ static u32 calc_l0s_acceptable(u32 encoding)
> /* Convert L1 latency encoding to ns */
> static u32 calc_l1_latency(u32 lnkcap)
> {
> - u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L1EL) >> 15;
> + u32 encoding = FIELD_GET(PCI_EXP_LNKCAP_L1EL, lnkcap);
>
> if (encoding == 0x7)
> return (65 * 1000); /* > 64us */
> @@ -371,11 +372,11 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
> link = endpoint->bus->self->link_state;
>
> /* Calculate endpoint L0s acceptable latency */
> - encoding = (endpoint->devcap & PCI_EXP_DEVCAP_L0S) >> 6;
> + encoding = FIELD_GET(PCI_EXP_DEVCAP_L0S, endpoint->devcap);
> acceptable_l0s = calc_l0s_acceptable(encoding);
>
> /* Calculate endpoint L1 acceptable latency */
> - encoding = (endpoint->devcap & PCI_EXP_DEVCAP_L1) >> 9;
> + encoding = FIELD_GET(PCI_EXP_DEVCAP_L1, endpoint->devcap);
> acceptable_l1 = calc_l1_acceptable(encoding);
>
> while (link) {
> @@ -446,22 +447,24 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
> u32 pl1_2_enables, cl1_2_enables;
>
> /* Choose the greater of the two Port Common_Mode_Restore_Times */
> - val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> - val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> + val1 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, parent_l1ss_cap);
> + val2 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, child_l1ss_cap);
> t_common_mode = max(val1, val2);
>
> /* Choose the greater of the two Port T_POWER_ON times */
> - val1 = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
> - scale1 = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
> - val2 = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
> - scale2 = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
> + val1 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, parent_l1ss_cap);
> + scale1 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, parent_l1ss_cap);
> + val2 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, child_l1ss_cap);
> + scale2 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, child_l1ss_cap);
>
> if (calc_l12_pwron(parent, scale1, val1) >
> calc_l12_pwron(child, scale2, val2)) {
> - ctl2 |= scale1 | (val1 << 3);
> + ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale1) |
> + FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_VALUE, val1);
> t_power_on = calc_l12_pwron(parent, scale1, val1);
> } else {
> - ctl2 |= scale2 | (val2 << 3);
> + ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale2) |
> + FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_VALUE, val2);
> t_power_on = calc_l12_pwron(child, scale2, val2);
> }
>
> @@ -477,7 +480,9 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
> */
> l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
> encode_l12_threshold(l1_2_threshold, &scale, &value);
> - ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
> + ctl1 |= FIELD_PREP(PCI_L1SS_CTL1_CM_RESTORE_TIME, t_common_mode) |
> + FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_VALUE, value) |
> + FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_SCALE, scale);
>
> /* Some broken devices only support dword access to L1 SS */
> pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e5f558d96493..34bf037993f3 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1088,6 +1088,8 @@
> #define PCI_L1SS_CTL1_LTR_L12_TH_VALUE 0x03ff0000 /* LTR_L1.2_THRESHOLD_Value */
> #define PCI_L1SS_CTL1_LTR_L12_TH_SCALE 0xe0000000 /* LTR_L1.2_THRESHOLD_Scale */
> #define PCI_L1SS_CTL2 0x0c /* Control 2 Register */
> +#define PCI_L1SS_CTL2_T_PWR_ON_SCALE 0x00000003 /* T_POWER_ON Scale */
> +#define PCI_L1SS_CTL2_T_PWR_ON_VALUE 0x000000f8 /* T_POWER_ON Value */
>
> /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
> #define PCI_DVSEC_HEADER1 0x4 /* Designated Vendor-Specific Header1 */
> --
> 2.34.1
>
On Tue, 10 Oct 2023 15:44:27 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
It's a little unfortunate that some of the masks are called *_MASK and
others are not (e.g. PCI_RBAR_CTRL_BAR_SIZE which is a mask).
but given they are in include/uapi not sure we can tidy that up unless
we add more defines that stick to consistent naming...
Otherwise very nice.
Reviewed-by: Jonathan Cameron <[email protected]>
On Tue, 10 Oct 2023 15:44:28 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Separate because this isn't as trivial as the other FIELD_GET() changes.
>
> See 907830b0fc9e ("PCI: Add a REBAR size quirk for Sapphire RX 5600 XT
> Pulse")
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> Cc: Nirmoy Das <[email protected]>
Change would be a tiny bit more obvious without the early return, but I can see
why you think that is an improvement over relying on the return just below.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/pci/pci.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 848c9ee65d7f..5dc6e7cdfb71 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3751,14 +3751,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar)
> return 0;
>
> pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> - cap &= PCI_REBAR_CAP_SIZES;
> + cap = FIELD_GET(PCI_REBAR_CAP_SIZES, cap);
>
> /* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */
> if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f &&
> - bar == 0 && cap == 0x7000)
> - cap = 0x3f000;
> + bar == 0 && cap == 0x700)
> + return 0x3f00;
>
> - return cap >> 4;
> + return cap;
> }
> EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
>
On Tue, 10 Oct 2023 15:44:30 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> The PASID Capability and Control registers are both 16 bits wide. Use
> 16-bit wide constants in field names to match the register width. No
> functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> include/uapi/linux/pci_regs.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 34bf037993f3..6af1f8d53e97 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -930,12 +930,12 @@
>
> /* Process Address Space ID */
> #define PCI_PASID_CAP 0x04 /* PASID feature register */
> -#define PCI_PASID_CAP_EXEC 0x02 /* Exec permissions Supported */
> -#define PCI_PASID_CAP_PRIV 0x04 /* Privilege Mode Supported */
> +#define PCI_PASID_CAP_EXEC 0x0002 /* Exec permissions Supported */
> +#define PCI_PASID_CAP_PRIV 0x0004 /* Privilege Mode Supported */
> #define PCI_PASID_CTRL 0x06 /* PASID control register */
> -#define PCI_PASID_CTRL_ENABLE 0x01 /* Enable bit */
> -#define PCI_PASID_CTRL_EXEC 0x02 /* Exec permissions Enable */
> -#define PCI_PASID_CTRL_PRIV 0x04 /* Privilege Mode Enable */
> +#define PCI_PASID_CTRL_ENABLE 0x0001 /* Enable bit */
> +#define PCI_PASID_CTRL_EXEC 0x0002 /* Exec permissions Enable */
> +#define PCI_PASID_CTRL_PRIV 0x0004 /* Privilege Mode Enable */
> #define PCI_EXT_CAP_PASID_SIZEOF 8
>
> /* Single Root I/O Virtualization */
On Tue, 10 Oct 2023 15:44:31 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
One trivial comment inline. Either way.
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/pci/ats.c | 7 ++-----
> include/uapi/linux/pci_regs.h | 1 +
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index f9cc2e10b676..c570892b2090 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -9,6 +9,7 @@
> * Copyright (C) 2011 Advanced Micro Devices,
> */
>
> +#include <linux/bitfield.h>
> #include <linux/export.h>
> #include <linux/pci-ats.h>
> #include <linux/pci.h>
> @@ -480,8 +481,6 @@ int pci_pasid_features(struct pci_dev *pdev)
> }
> EXPORT_SYMBOL_GPL(pci_pasid_features);
>
> -#define PASID_NUMBER_SHIFT 8
> -#define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT)
> /**
> * pci_max_pasids - Get maximum number of PASIDs supported by device
> * @pdev: PCI device structure
> @@ -503,9 +502,7 @@ int pci_max_pasids(struct pci_dev *pdev)
>
> pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
>
> - supported = (supported & PASID_NUMBER_MASK) >> PASID_NUMBER_SHIFT;
> -
> - return (1 << supported);
> + return (1 << FIELD_GET(PCI_PASID_CAP_WIDTH, supported));
Could drop the bonus set of brackets..
> }
> EXPORT_SYMBOL_GPL(pci_max_pasids);
> #endif /* CONFIG_PCI_PASID */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 6af1f8d53e97..833e5fb40ea5 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -932,6 +932,7 @@
> #define PCI_PASID_CAP 0x04 /* PASID feature register */
> #define PCI_PASID_CAP_EXEC 0x0002 /* Exec permissions Supported */
> #define PCI_PASID_CAP_PRIV 0x0004 /* Privilege Mode Supported */
> +#define PCI_PASID_CAP_WIDTH 0x1f00
> #define PCI_PASID_CTRL 0x06 /* PASID control register */
> #define PCI_PASID_CTRL_ENABLE 0x0001 /* Enable bit */
> #define PCI_PASID_CTRL_EXEC 0x0002 /* Exec permissions Enable */
On Tue, 10 Oct 2023 15:44:34 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
>
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/pcie/dpc.c | 9 +++++----
> drivers/pci/quirks.c | 2 +-
> include/uapi/linux/pci_regs.h | 1 +
> 3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 3ceed8e3de41..6e551f34ec63 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -8,6 +8,7 @@
>
> #define dev_fmt(fmt) "DPC: " fmt
>
> +#include <linux/bitfield.h>
> #include <linux/aer.h>
> #include <linux/delay.h>
> #include <linux/interrupt.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_STATUS_FIRST_ERR, dpc_status);
>
> for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> if ((status & ~mask) & (1 << i))
> @@ -270,8 +271,8 @@ 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 = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> pci_warn(pdev, "%s detected\n",
> (reason == 0) ? "unmasked uncorrectable error" :
> (reason == 1) ? "ERR_NONFATAL" :
BTW, it seems we're doing overlapping work here with many of these
patches. It takes some time to think these through one by one, I don't
just autorun through them with coccinelle so I've not posted my changes
yet.
I went to a different direction here and named all the reasons too with
defines and used & to get the reason in order to be able to compare with
the named reasons.
You also missed convering one 0xfff4 to use define (although I suspect it
never was your goal to go beyond FIELD_GET() here).
> @@ -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 833e5fb40ea5..e97a06b50f95 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1046,6 +1046,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_STATUS_FIRST_ERR 0x1f00 /* RP PIO First Error Ptr */
If you prefer consistency, there already FEP used for "First
Error Pointer" used in another define.
> #define PCI_EXP_DPC_SOURCE_ID 0x0A /* DPC Source Identifier */
--
i.
On Tue, 10 Oct 2023 15:44:33 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/pci/pcie/pme.c | 4 +++-
> include/uapi/linux/pci_regs.h | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index ef8ce436ead9..a2daebd9806c 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -9,6 +9,7 @@
>
> #define dev_fmt(fmt) "PME: " fmt
>
> +#include <linux/bitfield.h>
> #include <linux/pci.h>
> #include <linux/kernel.h>
> #include <linux/errno.h>
> @@ -235,7 +236,8 @@ static void pcie_pme_work_fn(struct work_struct *work)
> pcie_clear_root_pme_status(port);
>
> spin_unlock_irq(&data->lock);
> - pcie_pme_handle_request(port, rtsta & 0xffff);
> + pcie_pme_handle_request(port,
> + FIELD_GET(PCI_EXP_RTSTA_PME_RQ_ID, rtsta));
> spin_lock_irq(&data->lock);
>
> continue;
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e97a06b50f95..9fb8a69241f4 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -637,6 +637,7 @@
> #define PCI_EXP_RTCAP 0x1e /* Root Capabilities */
> #define PCI_EXP_RTCAP_CRSVIS 0x0001 /* CRS Software Visibility capability */
> #define PCI_EXP_RTSTA 0x20 /* Root Status */
> +#define PCI_EXP_RTSTA_PME_RQ_ID 0x0000ffff /* PME Requester ID */
> #define PCI_EXP_RTSTA_PME 0x00010000 /* PME status */
> #define PCI_EXP_RTSTA_PENDING 0x00020000 /* PME pending */
> /*
On Tue, 10 Oct 2023 15:44:35 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
On Wed, 11 Oct 2023, Jonathan Cameron wrote:
> On Tue, 10 Oct 2023 15:44:32 -0500
> Bjorn Helgaas <[email protected]> wrote:
>
> > From: Bjorn Helgaas <[email protected]>
> >
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value. No functional change intended.
> >
> > Signed-off-by: Bjorn Helgaas <[email protected]>
> A question about what 'rules' you are applying for using these macros
> vs choosing not not do so. Personally I prefer using them even for
> flag fields mostly because it makes the code more consistent and
> the compiler should remove any unnecessary shifts that result.
>
> > ---
>
> > 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))
>
> This is what I'm commenting on below.
>
> > return;
> >
> > - if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
> > + if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
>
> Why do this one and not the one just above?
> In both cases extracting a field then comparing it to 0, I'm not sure
> it makes sense to care if that field is 1 bit or multiple bit.
I cannot speak for Bjorn but at least I've left flag checks untouched
(but when pulling the flag into a variable, I've made it with FIELD_GET()).
In anycase, that seems minor issue though compared with defined values of
the field being incompatible with the FIELD_GET()ed value (when the shift
is non-zero). I wish there would be good solution to that but so far I've
not come up anything that would be short and simple enough.
--
i.
On Tue, 10 Oct 2023 15:44:36 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
Reviewed-by: Jonathan Cameron <[email protected]>
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
You seem to be using FIELD_PREP() too below.
> ---
> drivers/pci/pcie/ptm.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index b4e5f553467c..7cfb6c0d5dcb 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -4,6 +4,7 @@
> * Copyright (c) 2016, Intel Corporation.
> */
>
> +#include <linux/bitfield.h>
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/pci.h>
> @@ -53,7 +54,7 @@ void pci_ptm_init(struct pci_dev *dev)
> pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u32));
>
> pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
> - dev->ptm_granularity = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
> + dev->ptm_granularity = FIELD_GET(PCI_PTM_GRANULARITY_MASK, cap);
>
> /*
> * Per the spec recommendation (PCIe r6.0, sec 7.9.15.3), select the
> @@ -146,7 +147,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
>
> ctrl |= PCI_PTM_CTRL_ENABLE;
> ctrl &= ~PCI_PTM_GRANULARITY_MASK;
> - ctrl |= dev->ptm_granularity << 8;
> + ctrl |= FIELD_PREP(PCI_PTM_GRANULARITY_MASK, dev->ptm_granularity);
> if (dev->ptm_root)
> ctrl |= PCI_PTM_CTRL_ROOT;
>
>
--
i.
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
FIELD_PREP() is also used below.
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/pci.c | 45 ++++++++++++++++++++++-----------------------
> drivers/pci/probe.c | 8 ++++----
> 2 files changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a8adc34dc86f..848c9ee65d7f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1776,8 +1776,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> return;
>
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> - nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
> - PCI_REBAR_CTRL_NBAR_SHIFT;
> + nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
>
> for (i = 0; i < nbars; i++, pos += 8) {
> struct resource *res;
> @@ -1788,7 +1787,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
> res = pdev->resource + bar_idx;
> size = pci_rebar_bytes_to_size(resource_size(res));
> ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> - ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> + ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
> pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> }
> }
> @@ -3229,7 +3228,7 @@ void pci_pm_init(struct pci_dev *dev)
> (pmc & PCI_PM_CAP_PME_D2) ? " D2" : "",
> (pmc & PCI_PM_CAP_PME_D3hot) ? " D3hot" : "",
> (pmc & PCI_PM_CAP_PME_D3cold) ? " D3cold" : "");
> - dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;
> + dev->pme_support = FIELD_GET(PCI_PM_CAP_PME_MASK, pmc);
> dev->pme_poll = true;
> /*
> * Make device's PM flags reflect the wake-up capability, but
> @@ -3300,20 +3299,20 @@ static int pci_ea_read(struct pci_dev *dev, int offset)
> ent_offset += 4;
>
> /* Entry size field indicates DWORDs after 1st */
> - ent_size = ((dw0 & PCI_EA_ES) + 1) << 2;
> + ent_size = (FIELD_GET(PCI_EA_ES, dw0) + 1) << 2;
>
> if (!(dw0 & PCI_EA_ENABLE)) /* Entry not enabled */
> goto out;
>
> - bei = (dw0 & PCI_EA_BEI) >> 4;
> - prop = (dw0 & PCI_EA_PP) >> 8;
> + bei = FIELD_GET(PCI_EA_BEI, dw0);
> + prop = FIELD_GET(PCI_EA_PP, dw0);
>
> /*
> * If the Property is in the reserved range, try the Secondary
> * Property instead.
> */
> if (prop > PCI_EA_P_BRIDGE_IO && prop < PCI_EA_P_MEM_RESERVED)
> - prop = (dw0 & PCI_EA_SP) >> 16;
> + prop = FIELD_GET(PCI_EA_SP, dw0);
> if (prop > PCI_EA_P_BRIDGE_IO)
> goto out;
>
> @@ -3720,14 +3719,13 @@ static int pci_rebar_find_pos(struct pci_dev *pdev, int bar)
> return -ENOTSUPP;
>
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> - nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
> - PCI_REBAR_CTRL_NBAR_SHIFT;
> + nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
>
> for (i = 0; i < nbars; i++, pos += 8) {
> int bar_idx;
>
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> - bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> + bar_idx = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, ctrl);
> if (bar_idx == bar)
> return pos;
> }
> @@ -3782,7 +3780,7 @@ int pci_rebar_get_current_size(struct pci_dev *pdev, int bar)
> return pos;
>
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> - return (ctrl & PCI_REBAR_CTRL_BAR_SIZE) >> PCI_REBAR_CTRL_BAR_SHIFT;
> + return FIELD_GET(PCI_REBAR_CTRL_BAR_SIZE, ctrl);
> }
>
> /**
> @@ -3805,7 +3803,7 @@ int pci_rebar_set_size(struct pci_dev *pdev, int bar, int size)
>
> pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> - ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> + ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
> pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> return 0;
> }
> @@ -6043,7 +6041,7 @@ int pcix_get_max_mmrbc(struct pci_dev *dev)
> if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
> return -EINVAL;
>
> - return 512 << ((stat & PCI_X_STATUS_MAX_READ) >> 21);
> + return 512 << FIELD_GET(PCI_X_STATUS_MAX_READ, stat);
> }
> EXPORT_SYMBOL(pcix_get_max_mmrbc);
>
> @@ -6066,7 +6064,7 @@ int pcix_get_mmrbc(struct pci_dev *dev)
> if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
> return -EINVAL;
>
> - return 512 << ((cmd & PCI_X_CMD_MAX_READ) >> 2);
> + return 512 << FIELD_GET(PCI_X_CMD_MAX_READ, cmd);
> }
> EXPORT_SYMBOL(pcix_get_mmrbc);
>
> @@ -6097,19 +6095,19 @@ int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
> if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
> return -EINVAL;
>
> - if (v > (stat & PCI_X_STATUS_MAX_READ) >> 21)
> + if (v > FIELD_GET(PCI_X_STATUS_MAX_READ, stat))
> return -E2BIG;
>
> if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
> return -EINVAL;
>
> - o = (cmd & PCI_X_CMD_MAX_READ) >> 2;
> + o = FIELD_GET(PCI_X_CMD_MAX_READ, cmd);
> if (o != v) {
> if (v > o && (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MMRBC))
> return -EIO;
>
> cmd &= ~PCI_X_CMD_MAX_READ;
> - cmd |= v << 2;
> + cmd |= FIELD_PREP(PCI_X_CMD_MAX_READ, v);
> if (pci_write_config_word(dev, cap + PCI_X_CMD, cmd))
> return -EIO;
> }
> @@ -6129,7 +6127,7 @@ int pcie_get_readrq(struct pci_dev *dev)
>
> pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
>
> - return 128 << ((ctl & PCI_EXP_DEVCTL_READRQ) >> 12);
> + return 128 << FIELD_GET(PCI_EXP_DEVCTL_READRQ, ctl);
> }
> EXPORT_SYMBOL(pcie_get_readrq);
>
> @@ -6162,7 +6160,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> rq = mps;
> }
>
> - v = (ffs(rq) - 8) << 12;
> + v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8);
>
> if (bridge->no_inc_mrrs) {
> int max_mrrs = pcie_get_readrq(dev);
> @@ -6192,7 +6190,7 @@ int pcie_get_mps(struct pci_dev *dev)
>
> pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
>
> - return 128 << ((ctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5);
> + return 128 << FIELD_GET(PCI_EXP_DEVCTL_PAYLOAD, ctl);
> }
> EXPORT_SYMBOL(pcie_get_mps);
>
> @@ -6215,7 +6213,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
> v = ffs(mps) - 8;
> if (v > dev->pcie_mpss)
> return -EINVAL;
> - v <<= 5;
> + v = FIELD_PREP(PCI_EXP_DEVCTL_PAYLOAD, v);
--
i.
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/ats.c | 7 ++-----
> include/uapi/linux/pci_regs.h | 1 +
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index f9cc2e10b676..c570892b2090 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -9,6 +9,7 @@
> * Copyright (C) 2011 Advanced Micro Devices,
> */
>
> +#include <linux/bitfield.h>
> #include <linux/export.h>
> #include <linux/pci-ats.h>
> #include <linux/pci.h>
> @@ -480,8 +481,6 @@ int pci_pasid_features(struct pci_dev *pdev)
> }
> EXPORT_SYMBOL_GPL(pci_pasid_features);
>
> -#define PASID_NUMBER_SHIFT 8
> -#define PASID_NUMBER_MASK (0x1f << PASID_NUMBER_SHIFT)
> /**
> * pci_max_pasids - Get maximum number of PASIDs supported by device
> * @pdev: PCI device structure
> @@ -503,9 +502,7 @@ int pci_max_pasids(struct pci_dev *pdev)
>
> pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
>
> - supported = (supported & PASID_NUMBER_MASK) >> PASID_NUMBER_SHIFT;
> -
> - return (1 << supported);
> + return (1 << FIELD_GET(PCI_PASID_CAP_WIDTH, supported));
> }
> EXPORT_SYMBOL_GPL(pci_max_pasids);
> #endif /* CONFIG_PCI_PASID */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 6af1f8d53e97..833e5fb40ea5 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -932,6 +932,7 @@
> #define PCI_PASID_CAP 0x04 /* PASID feature register */
> #define PCI_PASID_CAP_EXEC 0x0002 /* Exec permissions Supported */
> #define PCI_PASID_CAP_PRIV 0x0004 /* Privilege Mode Supported */
> +#define PCI_PASID_CAP_WIDTH 0x1f00
> #define PCI_PASID_CTRL 0x06 /* PASID control register */
> #define PCI_PASID_CTRL_ENABLE 0x0001 /* Enable bit */
> #define PCI_PASID_CTRL_EXEC 0x0002 /* Exec permissions Enable */
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Separate because this isn't as trivial as the other FIELD_GET() changes.
>
> See 907830b0fc9e ("PCI: Add a REBAR size quirk for Sapphire RX 5600 XT
> Pulse")
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> Cc: Nirmoy Das <[email protected]>
> ---
> drivers/pci/pci.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 848c9ee65d7f..5dc6e7cdfb71 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3751,14 +3751,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar)
> return 0;
>
> pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> - cap &= PCI_REBAR_CAP_SIZES;
> + cap = FIELD_GET(PCI_REBAR_CAP_SIZES, cap);
>
> /* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */
> if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f &&
> - bar == 0 && cap == 0x7000)
> - cap = 0x3f000;
> + bar == 0 && cap == 0x700)
> + return 0x3f00;
>
> - return cap >> 4;
> + return cap;
> }
> EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
>
>
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> The PASID Capability and Control registers are both 16 bits wide. Use
> 16-bit wide constants in field names to match the register width. No
> functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> include/uapi/linux/pci_regs.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 34bf037993f3..6af1f8d53e97 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -930,12 +930,12 @@
>
> /* Process Address Space ID */
> #define PCI_PASID_CAP 0x04 /* PASID feature register */
> -#define PCI_PASID_CAP_EXEC 0x02 /* Exec permissions Supported */
> -#define PCI_PASID_CAP_PRIV 0x04 /* Privilege Mode Supported */
> +#define PCI_PASID_CAP_EXEC 0x0002 /* Exec permissions Supported */
> +#define PCI_PASID_CAP_PRIV 0x0004 /* Privilege Mode Supported */
> #define PCI_PASID_CTRL 0x06 /* PASID control register */
> -#define PCI_PASID_CTRL_ENABLE 0x01 /* Enable bit */
> -#define PCI_PASID_CTRL_EXEC 0x02 /* Exec permissions Enable */
> -#define PCI_PASID_CTRL_PRIV 0x04 /* Privilege Mode Enable */
> +#define PCI_PASID_CTRL_ENABLE 0x0001 /* Enable bit */
> +#define PCI_PASID_CTRL_EXEC 0x0002 /* Exec permissions Enable */
> +#define PCI_PASID_CTRL_PRIV 0x0004 /* Privilege Mode Enable */
> #define PCI_EXT_CAP_PASID_SIZEOF 8
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Tue, 10 Oct 2023 15:44:32 -0500
Bjorn Helgaas <[email protected]> wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
A question about what 'rules' you are applying for using these macros
vs choosing not not do so. Personally I prefer using them even for
flag fields mostly because it makes the code more consistent and
the compiler should remove any unnecessary shifts that result.
> ---
> 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))
This is what I'm commenting on below.
> return;
>
> - if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
> + if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
Why do this one and not the one just above?
In both cases extracting a field then comparing it to 0, I'm not sure
it makes sense to care if that field is 1 bit or multiple bit.
> pci_info(dev, "Overriding RP PIO Log Size to 4\n");
> dev->dpc_rp_log_size = 4;
> }
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/pcie/pme.c | 4 +++-
> include/uapi/linux/pci_regs.h | 1 +
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index ef8ce436ead9..a2daebd9806c 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -9,6 +9,7 @@
>
> #define dev_fmt(fmt) "PME: " fmt
>
> +#include <linux/bitfield.h>
> #include <linux/pci.h>
> #include <linux/kernel.h>
> #include <linux/errno.h>
> @@ -235,7 +236,8 @@ static void pcie_pme_work_fn(struct work_struct *work)
> pcie_clear_root_pme_status(port);
>
> spin_unlock_irq(&data->lock);
> - pcie_pme_handle_request(port, rtsta & 0xffff);
> + pcie_pme_handle_request(port,
> + FIELD_GET(PCI_EXP_RTSTA_PME_RQ_ID, rtsta));
> spin_lock_irq(&data->lock);
>
> continue;
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e97a06b50f95..9fb8a69241f4 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -637,6 +637,7 @@
> #define PCI_EXP_RTCAP 0x1e /* Root Capabilities */
> #define PCI_EXP_RTCAP_CRSVIS 0x0001 /* CRS Software Visibility capability */
> #define PCI_EXP_RTSTA 0x20 /* Root Status */
> +#define PCI_EXP_RTSTA_PME_RQ_ID 0x0000ffff /* PME Requester ID */
> #define PCI_EXP_RTSTA_PME 0x00010000 /* PME status */
> #define PCI_EXP_RTSTA_PENDING 0x00020000 /* PME pending */
Reviewed-by: Ilpo J?rvinen <[email protected]>
(I'd have used REQ myself but it seems both forms are commonly used by
the existing defines.)
--
i.
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/vc.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c
> index 5fc59ac31145..a4ff7f5f66dd 100644
> --- a/drivers/pci/vc.c
> +++ b/drivers/pci/vc.c
> @@ -6,6 +6,7 @@
> * Author: Alex Williamson <[email protected]>
> */
>
> +#include <linux/bitfield.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -201,9 +202,9 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
> /* Extended VC Count (not counting VC0) */
> evcc = cap1 & PCI_VC_CAP1_EVCC;
> /* Low Priority Extended VC Count (not counting VC0) */
> - lpevcc = (cap1 & PCI_VC_CAP1_LPEVCC) >> 4;
> + lpevcc = FIELD_GET(PCI_VC_CAP1_LPEVCC, cap1);
> /* Port Arbitration Table Entry Size (bits) */
> - parb_size = 1 << ((cap1 & PCI_VC_CAP1_ARB_SIZE) >> 10);
> + parb_size = 1 << FIELD_GET(PCI_VC_CAP1_ARB_SIZE, cap1);
>
> /*
> * Port VC Control Register contains VC Arbitration Select, which
> @@ -231,7 +232,7 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
> int vcarb_offset;
>
> pci_read_config_dword(dev, pos + PCI_VC_PORT_CAP2, &cap2);
> - vcarb_offset = ((cap2 & PCI_VC_CAP2_ARB_OFF) >> 24) * 16;
> + vcarb_offset = FIELD_GET(PCI_VC_CAP2_ARB_OFF, cap2) * 16;
>
> if (vcarb_offset) {
> int size, vcarb_phases = 0;
> @@ -277,7 +278,7 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
>
> pci_read_config_dword(dev, pos + PCI_VC_RES_CAP +
> (i * PCI_CAP_VC_PER_VC_SIZEOF), &cap);
> - parb_offset = ((cap & PCI_VC_RES_CAP_ARB_OFF) >> 24) * 16;
> + parb_offset = FIELD_GET(PCI_VC_RES_CAP_ARB_OFF, cap) * 16;
> if (parb_offset) {
> int size, parb_phases = 0;
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/pci/pcie/portdrv.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 46fad0d813b2..14a4b89a3b83 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -6,6 +6,7 @@
> * Copyright (C) Tom Long Nguyen ([email protected])
> */
>
> +#include <linux/bitfield.h>
> #include <linux/dmi.h>
> #include <linux/init.h>
> #include <linux/module.h>
> @@ -69,7 +70,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
> if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP |
> PCIE_PORT_SERVICE_BWNOTIF)) {
> pcie_capability_read_word(dev, PCI_EXP_FLAGS, ®16);
> - *pme = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
> + *pme = FIELD_GET(PCI_EXP_FLAGS_IRQ, reg16);
> nvec = *pme + 1;
> }
>
> @@ -81,7 +82,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
> if (pos) {
> pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS,
> ®32);
> - *aer = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;
> + *aer = FIELD_GET(PCI_ERR_ROOT_AER_IRQ, reg32);
> nvec = max(nvec, *aer + 1);
> }
> }
> @@ -92,7 +93,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
> if (pos) {
> pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP,
> ®16);
> - *dpc = reg16 & PCI_EXP_DPC_IRQ;
> + *dpc = FIELD_GET(PCI_EXP_DPC_IRQ, reg16);
> nvec = max(nvec, *dpc + 1);
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
On 10/10/2023 1:44 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() and FIELD_PREP() to avoid the need for shifting.
>
> These apply on top of the PCI patches from Ilpo's series:
> https://lore.kernel.org/r/[email protected]
>
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
> Bjorn Helgaas (10):
> PCI: Use FIELD_GET()
> PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk
> PCI/ASPM: Use FIELD_GET()
> PCI/ATS: Show PASID Capability register width in bitmasks
> PCI/ATS: Use FIELD_GET()
> PCI/DPC: Use FIELD_GET()
> PCI/PME: Use FIELD_GET()
> PCI/PTM: Use FIELD_GET()
> PCI/VC: Use FIELD_GET()
> PCI/portdrv: Use FIELD_GET()
>
> drivers/pci/ats.c | 7 ++---
> drivers/pci/pci.c | 53 +++++++++++++++++------------------
> drivers/pci/pcie/aspm.c | 31 +++++++++++---------
> drivers/pci/pcie/dpc.c | 9 +++---
> drivers/pci/pcie/pme.c | 4 ++-
> drivers/pci/pcie/portdrv.c | 7 +++--
> drivers/pci/pcie/ptm.c | 5 ++--
> drivers/pci/probe.c | 8 +++---
> drivers/pci/quirks.c | 2 +-
> drivers/pci/vc.c | 9 +++---
> include/uapi/linux/pci_regs.h | 15 ++++++----
> 11 files changed, 81 insertions(+), 69 deletions(-)
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Wed, 11 Oct 2023, Ilpo J?rvinen wrote:
> On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
>
> > From: Bjorn Helgaas <[email protected]>
> >
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value. No functional change intended.
> >
> > Signed-off-by: Bjorn Helgaas <[email protected]>
> > ---
> > drivers/pci/pcie/dpc.c | 9 +++++----
> > drivers/pci/quirks.c | 2 +-
> > include/uapi/linux/pci_regs.h | 1 +
> > 3 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 3ceed8e3de41..6e551f34ec63 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -8,6 +8,7 @@
> >
> > #define dev_fmt(fmt) "DPC: " fmt
> >
> > +#include <linux/bitfield.h>
> > #include <linux/aer.h>
> > #include <linux/delay.h>
> > #include <linux/interrupt.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_STATUS_FIRST_ERR, dpc_status);
> >
> > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > if ((status & ~mask) & (1 << i))
> > @@ -270,8 +271,8 @@ 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 = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > pci_warn(pdev, "%s detected\n",
> > (reason == 0) ? "unmasked uncorrectable error" :
> > (reason == 1) ? "ERR_NONFATAL" :
>
> BTW, it seems we're doing overlapping work here with many of these
> patches. It takes some time to think these through one by one, I don't
> just autorun through them with coccinelle so I've not posted my changes
> yet.
>
> I went to a different direction here and named all the reasons too with
> defines and used & to get the reason in order to be able to compare with
> the named reasons.
>
> You also missed convering one 0xfff4 to use define (although I suspect it
> never was your goal to go beyond FIELD_GET() here).
I posted my approach onto the list so you can see what it looks like:
https://lore.kernel.org/linux-pci/[email protected]/T/#t
(It will obviously conflict with this patch so both cannot be applied as
is).
--
i.
On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <[email protected]>
> >
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value. No functional change intended.
> >
> > Signed-off-by: Bjorn Helgaas <[email protected]>
> > ---
> > drivers/pci/pcie/dpc.c | 9 +++++----
> > drivers/pci/quirks.c | 2 +-
> > include/uapi/linux/pci_regs.h | 1 +
> > 3 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 3ceed8e3de41..6e551f34ec63 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -8,6 +8,7 @@
> >
> > #define dev_fmt(fmt) "DPC: " fmt
> >
> > +#include <linux/bitfield.h>
> > #include <linux/aer.h>
> > #include <linux/delay.h>
> > #include <linux/interrupt.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_STATUS_FIRST_ERR, dpc_status);
> >
> > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > if ((status & ~mask) & (1 << i))
> > @@ -270,8 +271,8 @@ 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 = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > pci_warn(pdev, "%s detected\n",
> > (reason == 0) ? "unmasked uncorrectable error" :
> > (reason == 1) ? "ERR_NONFATAL" :
>
> BTW, it seems we're doing overlapping work here with many of these
> patches. It takes some time to think these through one by one, I don't
> just autorun through them with coccinelle so I've not posted my changes
> yet.
>
> I went to a different direction here and named all the reasons too with
> defines and used & to get the reason in order to be able to compare with
> the named reasons.
>
> You also missed convering one 0xfff4 to use define (although I suspect it
> never was your goal to go beyond FIELD_GET() here).
Pure FIELD_GET() and FIELD_PREP() was my goal.
If you have patches you prefer, I'll drop mine. I did these about a
year ago and it seemed like the time to do something with them since
you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
work. Since we've started, I'd like to get as much of it done this
cycle as possible.
Bjorn
On Fri, 13 Oct 2023, Bjorn Helgaas wrote:
> On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > From: Bjorn Helgaas <[email protected]>
> > >
> > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > shift value. No functional change intended.
> > >
> > > Signed-off-by: Bjorn Helgaas <[email protected]>
> > > ---
> > > drivers/pci/pcie/dpc.c | 9 +++++----
> > > drivers/pci/quirks.c | 2 +-
> > > include/uapi/linux/pci_regs.h | 1 +
> > > 3 files changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > index 3ceed8e3de41..6e551f34ec63 100644
> > > --- a/drivers/pci/pcie/dpc.c
> > > +++ b/drivers/pci/pcie/dpc.c
> > > @@ -8,6 +8,7 @@
> > >
> > > #define dev_fmt(fmt) "DPC: " fmt
> > >
> > > +#include <linux/bitfield.h>
> > > #include <linux/aer.h>
> > > #include <linux/delay.h>
> > > #include <linux/interrupt.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_STATUS_FIRST_ERR, dpc_status);
> > >
> > > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > > if ((status & ~mask) & (1 << i))
> > > @@ -270,8 +271,8 @@ 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 = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > > pci_warn(pdev, "%s detected\n",
> > > (reason == 0) ? "unmasked uncorrectable error" :
> > > (reason == 1) ? "ERR_NONFATAL" :
> >
> > BTW, it seems we're doing overlapping work here with many of these
> > patches. It takes some time to think these through one by one, I don't
> > just autorun through them with coccinelle so I've not posted my changes
> > yet.
> >
> > I went to a different direction here and named all the reasons too with
> > defines and used & to get the reason in order to be able to compare with
> > the named reasons.
> >
> > You also missed convering one 0xfff4 to use define (although I suspect it
> > never was your goal to go beyond FIELD_GET() here).
>
> Pure FIELD_GET() and FIELD_PREP() was my goal.
>
> If you have patches you prefer, I'll drop mine. I did these about a
> year ago and it seemed like the time to do something with them since
> you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> work. Since we've started, I'd like to get as much of it done this
> cycle as possible.
Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting
more and more complicated. I can build a nice set of small changes about
what remains to do in DPC on top of your patch.
--
i.
On Mon, 16 Oct 2023, Ilpo Järvinen wrote:
> On Fri, 13 Oct 2023, Bjorn Helgaas wrote:
>
> > On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > > From: Bjorn Helgaas <[email protected]>
> > > >
> > > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > > shift value. No functional change intended.
> > > >
> > > > Signed-off-by: Bjorn Helgaas <[email protected]>
> > > > ---
> > > > drivers/pci/pcie/dpc.c | 9 +++++----
> > > > drivers/pci/quirks.c | 2 +-
> > > > include/uapi/linux/pci_regs.h | 1 +
> > > > 3 files changed, 7 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > index 3ceed8e3de41..6e551f34ec63 100644
> > > > --- a/drivers/pci/pcie/dpc.c
> > > > +++ b/drivers/pci/pcie/dpc.c
> > > > @@ -8,6 +8,7 @@
> > > >
> > > > #define dev_fmt(fmt) "DPC: " fmt
> > > >
> > > > +#include <linux/bitfield.h>
> > > > #include <linux/aer.h>
> > > > #include <linux/delay.h>
> > > > #include <linux/interrupt.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_STATUS_FIRST_ERR, dpc_status);
> > > >
> > > > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > > > if ((status & ~mask) & (1 << i))
> > > > @@ -270,8 +271,8 @@ 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 = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > > > pci_warn(pdev, "%s detected\n",
> > > > (reason == 0) ? "unmasked uncorrectable error" :
> > > > (reason == 1) ? "ERR_NONFATAL" :
> > >
> > > BTW, it seems we're doing overlapping work here with many of these
> > > patches. It takes some time to think these through one by one, I don't
> > > just autorun through them with coccinelle so I've not posted my changes
> > > yet.
> > >
> > > I went to a different direction here and named all the reasons too with
> > > defines and used & to get the reason in order to be able to compare with
> > > the named reasons.
> > >
> > > You also missed convering one 0xfff4 to use define (although I suspect it
> > > never was your goal to go beyond FIELD_GET() here).
> >
> > Pure FIELD_GET() and FIELD_PREP() was my goal.
> >
> > If you have patches you prefer, I'll drop mine. I did these about a
> > year ago and it seemed like the time to do something with them since
> > you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> > work. Since we've started, I'd like to get as much of it done this
> > cycle as possible.
>
> Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting
> more and more complicated. I can build a nice set of small changes about
> what remains to do in DPC on top of your patch.
Err, actually, there's still the naming of the define, should _FEP be used
for First Error Pointer for consistency? You should make that small change
into your patch if you think _FEP is better because of consistency.
--
i.
On Mon, 16 Oct 2023, Ilpo Järvinen wrote:
> On Mon, 16 Oct 2023, Ilpo Järvinen wrote:
>
> > On Fri, 13 Oct 2023, Bjorn Helgaas wrote:
> >
> > > On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > > > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > > > From: Bjorn Helgaas <[email protected]>
> > > > >
> > > > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > > > shift value. No functional change intended.
> > > > >
> > > > > Signed-off-by: Bjorn Helgaas <[email protected]>
> > > > > ---
> > > > > drivers/pci/pcie/dpc.c | 9 +++++----
> > > > > drivers/pci/quirks.c | 2 +-
> > > > > include/uapi/linux/pci_regs.h | 1 +
> > > > > 3 files changed, 7 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > > index 3ceed8e3de41..6e551f34ec63 100644
> > > > > --- a/drivers/pci/pcie/dpc.c
> > > > > +++ b/drivers/pci/pcie/dpc.c
> > > > > @@ -8,6 +8,7 @@
> > > > >
> > > > > #define dev_fmt(fmt) "DPC: " fmt
> > > > >
> > > > > +#include <linux/bitfield.h>
> > > > > #include <linux/aer.h>
> > > > > #include <linux/delay.h>
> > > > > #include <linux/interrupt.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_STATUS_FIRST_ERR, dpc_status);
> > > > >
> > > > > for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > > > > if ((status & ~mask) & (1 << i))
> > > > > @@ -270,8 +271,8 @@ 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 = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > > > + ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > > > > pci_warn(pdev, "%s detected\n",
> > > > > (reason == 0) ? "unmasked uncorrectable error" :
> > > > > (reason == 1) ? "ERR_NONFATAL" :
> > > >
> > > > BTW, it seems we're doing overlapping work here with many of these
> > > > patches. It takes some time to think these through one by one, I don't
> > > > just autorun through them with coccinelle so I've not posted my changes
> > > > yet.
> > > >
> > > > I went to a different direction here and named all the reasons too with
> > > > defines and used & to get the reason in order to be able to compare with
> > > > the named reasons.
> > > >
> > > > You also missed convering one 0xfff4 to use define (although I suspect it
> > > > never was your goal to go beyond FIELD_GET() here).
> > >
> > > Pure FIELD_GET() and FIELD_PREP() was my goal.
> > >
> > > If you have patches you prefer, I'll drop mine. I did these about a
> > > year ago and it seemed like the time to do something with them since
> > > you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> > > work. Since we've started, I'd like to get as much of it done this
> > > cycle as possible.
> >
> > Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting
> > more and more complicated. I can build a nice set of small changes about
> > what remains to do in DPC on top of your patch.
>
> Err, actually, there's still the naming of the define, should _FEP be used
> for First Error Pointer for consistency? You should make that small change
> into your patch if you think _FEP is better because of consistency.
There's also #include order so it seems you should just drop the patch, I
can handle this along my series.
--
i.
On Tue, Oct 10, 2023 at 03:44:26PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> Use FIELD_GET() and FIELD_PREP() to avoid the need for shifting.
>
> These apply on top of the PCI patches from Ilpo's series:
> https://lore.kernel.org/r/[email protected]
>
> Bjorn Helgaas (10):
> PCI: Use FIELD_GET()
> PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk
> PCI/ASPM: Use FIELD_GET()
> PCI/ATS: Show PASID Capability register width in bitmasks
> PCI/ATS: Use FIELD_GET()
> PCI/DPC: Use FIELD_GET()
> PCI/PME: Use FIELD_GET()
> PCI/PTM: Use FIELD_GET()
> PCI/VC: Use FIELD_GET()
> PCI/portdrv: Use FIELD_GET()
>
> drivers/pci/ats.c | 7 ++---
> drivers/pci/pci.c | 53 +++++++++++++++++------------------
> drivers/pci/pcie/aspm.c | 31 +++++++++++---------
> drivers/pci/pcie/dpc.c | 9 +++---
> drivers/pci/pcie/pme.c | 4 ++-
> drivers/pci/pcie/portdrv.c | 7 +++--
> drivers/pci/pcie/ptm.c | 5 ++--
> drivers/pci/probe.c | 8 +++---
> drivers/pci/quirks.c | 2 +-
> drivers/pci/vc.c | 9 +++---
> include/uapi/linux/pci_regs.h | 15 ++++++----
> 11 files changed, 81 insertions(+), 69 deletions(-)
I applied these to pci/field-get for v6.7, thanks for all the reviews!