On Wed, 18 Oct 2023, Serge Semin wrote: > On Wed, Oct 18, 2023 at 02:32:49PM +0300, Ilpo Järvinen wrote: > > Convert open-coded variants of PCI field access into FIELD_GET/PREP() > > to make the code easier to understand. > > > > Add two missing defines into pci_regs.h. Logically, the Max No-Snoop > > Latency Register is a separate word sized register in the PCIe spec, > > but the pre-existing LTR defines in pci_regs.h with dword long values > > seem to consider the registers together (the same goes for the only > > user). Thus, follow the custom and make the new values also take both > > word long LTR registers as a joint dword register. > > Nice work. Thanks! Could you also have a look at > drivers/pci/controller/dwc/pcie-designware.c > ? > It contains two open-coded patterns: > (bar << 8) - FIELD_PREP() > next_cap_ptr = (reg & 0xff00) >> 8; - FIELD_GET(). > next_cap_ptr = (reg & 0x00ff); - FIELD_GET(). > At least the later two statements concern the generic PCIe capability CSR. The problem with cap id / next cap is that there are currently no defines for them AFAICT, at least not in pci_regs.h. And pci_regs.h defines those as different registers so if I'm to add defines from them, I don't know which size would be the most appropriate since that 0xff00 goes across that register boundary. I've not had time to study the related core code yet but I intend to take a look at it to see what's the best course of action forward. -- i. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> > > --- > > 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@xxxxxx> > > */ > > > > +#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 > > >
