diff mbox series

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

Message ID 20231018113254.17616-3-ilpo.jarvinen@linux.intel.com (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Use FIELD_GET/PREP() & other reg field cleanups | expand

Commit Message

Ilpo Järvinen Oct. 18, 2023, 11:32 a.m. UTC 
Convert open-coded variants of PCI field access into FIELD_GET/PREP()
to make the code easier to understand.

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

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

Comments

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

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

-Serge(y)


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

Patch

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