| Message ID | 20211022183709.1199701-14-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL Region Creation / HDM decoder programming | expand |
On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > Get a better naming scheme in place for upcoming additions. To solidify > the schema, add all the DVSEC identifiers to start with. The title and this changelog don't give anything of substance to review the patch. This also looks like a rename and addition of more definitions. The rename has one rationale, the additional definitions have a different one, so split those into 2 patches, or fold the additions into the patch that uses them. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > See: > https://lore.kernel.org/linux-pci/20210913190131.xiiszmno46qie7v5@intel.com/ Perhaps summarize this above, it's not clear what's relevant from that thread to this patch. > --- > drivers/cxl/core/regs.c | 14 ++++++++------ > drivers/cxl/pci.h | 38 ++++++++++++++++++++++++++++++-------- > 2 files changed, 38 insertions(+), 14 deletions(-) > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index c8ab8880b81b..b837196fbf39 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -253,9 +253,11 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi, > struct cxl_register_map *map) > { > map->block_offset = > - ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); > - map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); > - map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); > + ((u64)reg_hi << 32) | > + (reg_lo & DVSEC_REGISTER_LOCATOR_BLOCK_OFFSET_LOW_MASK); > + map->barno = FIELD_GET(DVSEC_REGISTER_LOCATOR_BIR_MASK, reg_lo); > + map->reg_type = > + FIELD_GET(DVSEC_REGISTER_LOCATOR_BLOCK_IDENTIFIER_MASK, reg_lo); > } > > /** > @@ -276,15 +278,15 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, > int regloc, i; > > regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, > - PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID); > + CXL_DVSEC_REGISTER_LOCATOR); > if (!regloc) > return -ENXIO; > > pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); > regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); > > - regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET; > - regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8; > + regloc += DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET; > + regblocks = (regloc_size - DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET) / 8; > > for (i = 0; i < regblocks; i++, regloc += 8) { > u32 reg_lo, reg_hi; > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > index 12fdcb1b14e5..fe2898b17736 100644 > --- a/drivers/cxl/pci.h > +++ b/drivers/cxl/pci.h > @@ -7,17 +7,36 @@ > > /* > * See section 8.1 Configuration Space Registers in the CXL 2.0 > - * Specification > + * Specification. Names are taken straight from the specification with "CXL" and > + * "DVSEC" redundancies removed. > */ > #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) > #define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 > -#define PCI_DVSEC_ID_CXL 0x0 > > -#define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID 0x8 > -#define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET 0xC > +/* 8.1.3: PCIe DVSEC for CXL Device */ > +#define CXL_DVSEC_PCIE_DEVICE 0 > > -/* BAR Indicator Register (BIR) */ > -#define CXL_REGLOC_BIR_MASK GENMASK(2, 0) > +/* 8.1.4: Non-CXL Function Map DVSEC */ > +#define CXL_DVSEC_FUNCTION_MAP 2 > + > +/* 8.1.5: CXL 2.0 Extensions DVSEC for Ports */ > +#define CXL_DVSEC_PORT_EXTENSIONS 3 > + > +/* 8.1.6: GPF DVSEC for CXL Port */ > +#define CXL_DVSEC_PORT_GPF 4 > + > +/* 8.1.7: GPF DVSEC for CXL Device */ > +#define CXL_DVSEC_DEVICE_GPF 5 > + > +/* 8.1.8: PCIe DVSEC for Flex Bus Port */ > +#define CXL_DVSEC_PCIE_FLEXBUS_PORT 7 > + > +/* 8.1.9: Register Locator DVSEC */ > +#define CXL_DVSEC_REGISTER_LOCATOR 8 > +#define DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET 0xC > +#define DVSEC_REGISTER_LOCATOR_BIR_MASK GENMASK(2, 0) > +#define DVSEC_REGISTER_LOCATOR_BLOCK_IDENTIFIER_MASK GENMASK(15, 8) > +#define DVSEC_REGISTER_LOCATOR_BLOCK_OFFSET_LOW_MASK GENMASK(31, 16) > > /* Register Block Identifier (RBI) */ > enum cxl_regloc_type { > @@ -28,8 +47,11 @@ enum cxl_regloc_type { > CXL_REGLOC_RBI_TYPES > }; > > -#define CXL_REGLOC_RBI_MASK GENMASK(15, 8) > -#define CXL_REGLOC_ADDR_MASK GENMASK(31, 16) > +/* 8.1.10: MLD DVSEC */ > +#define CXL_DVSEC_MLD 9 > + > +/* 14.16.1 CXL Device Test Capability Advertisement */ > +#define CXL_DVSEC_PCIE_TEST_CAPABILITY 10 > > #define cxl_reg_block(pdev, map) \ > ((resource_size_t)(pci_resource_start(pdev, (map)->barno) + \ > -- > 2.33.1 >
On 21-10-31 13:18:36, Dan Williams wrote: > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > Get a better naming scheme in place for upcoming additions. To solidify > > the schema, add all the DVSEC identifiers to start with. > > The title and this changelog don't give anything of substance to > review the patch. > > This also looks like a rename and addition of more definitions. The > rename has one rationale, the additional definitions have a different > one, so split those into 2 patches, or fold the additions into the > patch that uses them. > I added more than necessary as a means to codify the naming scheme [as stated]. Many of them are not used. I can split this patch into two, though I personally don't find it offensive to do it as one. Before I do that, I'd like to know though if you're going to reject the patch if I'm not actually using all of the defines later on. > > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > > > --- > > See: > > https://lore.kernel.org/linux-pci/20210913190131.xiiszmno46qie7v5@intel.com/ > > Perhaps summarize this above, it's not clear what's relevant from that > thread to this patch. > > > --- > > drivers/cxl/core/regs.c | 14 ++++++++------ > > drivers/cxl/pci.h | 38 ++++++++++++++++++++++++++++++-------- > > 2 files changed, 38 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > > index c8ab8880b81b..b837196fbf39 100644 > > --- a/drivers/cxl/core/regs.c > > +++ b/drivers/cxl/core/regs.c > > @@ -253,9 +253,11 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi, > > struct cxl_register_map *map) > > { > > map->block_offset = > > - ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); > > - map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); > > - map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); > > + ((u64)reg_hi << 32) | > > + (reg_lo & DVSEC_REGISTER_LOCATOR_BLOCK_OFFSET_LOW_MASK); > > + map->barno = FIELD_GET(DVSEC_REGISTER_LOCATOR_BIR_MASK, reg_lo); > > + map->reg_type = > > + FIELD_GET(DVSEC_REGISTER_LOCATOR_BLOCK_IDENTIFIER_MASK, reg_lo); > > } > > > > /** > > @@ -276,15 +278,15 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, > > int regloc, i; > > > > regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, > > - PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID); > > + CXL_DVSEC_REGISTER_LOCATOR); > > if (!regloc) > > return -ENXIO; > > > > pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); > > regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); > > > > - regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET; > > - regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8; > > + regloc += DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET; > > + regblocks = (regloc_size - DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET) / 8; > > > > for (i = 0; i < regblocks; i++, regloc += 8) { > > u32 reg_lo, reg_hi; > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > index 12fdcb1b14e5..fe2898b17736 100644 > > --- a/drivers/cxl/pci.h > > +++ b/drivers/cxl/pci.h > > @@ -7,17 +7,36 @@ > > > > /* > > * See section 8.1 Configuration Space Registers in the CXL 2.0 > > - * Specification > > + * Specification. Names are taken straight from the specification with "CXL" and > > + * "DVSEC" redundancies removed. > > */ > > #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) > > #define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 > > -#define PCI_DVSEC_ID_CXL 0x0 > > > > -#define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID 0x8 > > -#define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET 0xC > > +/* 8.1.3: PCIe DVSEC for CXL Device */ > > +#define CXL_DVSEC_PCIE_DEVICE 0 > > > > -/* BAR Indicator Register (BIR) */ > > -#define CXL_REGLOC_BIR_MASK GENMASK(2, 0) > > +/* 8.1.4: Non-CXL Function Map DVSEC */ > > +#define CXL_DVSEC_FUNCTION_MAP 2 > > + > > +/* 8.1.5: CXL 2.0 Extensions DVSEC for Ports */ > > +#define CXL_DVSEC_PORT_EXTENSIONS 3 > > + > > +/* 8.1.6: GPF DVSEC for CXL Port */ > > +#define CXL_DVSEC_PORT_GPF 4 > > + > > +/* 8.1.7: GPF DVSEC for CXL Device */ > > +#define CXL_DVSEC_DEVICE_GPF 5 > > + > > +/* 8.1.8: PCIe DVSEC for Flex Bus Port */ > > +#define CXL_DVSEC_PCIE_FLEXBUS_PORT 7 > > + > > +/* 8.1.9: Register Locator DVSEC */ > > +#define CXL_DVSEC_REGISTER_LOCATOR 8 > > +#define DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET 0xC > > +#define DVSEC_REGISTER_LOCATOR_BIR_MASK GENMASK(2, 0) > > +#define DVSEC_REGISTER_LOCATOR_BLOCK_IDENTIFIER_MASK GENMASK(15, 8) > > +#define DVSEC_REGISTER_LOCATOR_BLOCK_OFFSET_LOW_MASK GENMASK(31, 16) > > > > /* Register Block Identifier (RBI) */ > > enum cxl_regloc_type { > > @@ -28,8 +47,11 @@ enum cxl_regloc_type { > > CXL_REGLOC_RBI_TYPES > > }; > > > > -#define CXL_REGLOC_RBI_MASK GENMASK(15, 8) > > -#define CXL_REGLOC_ADDR_MASK GENMASK(31, 16) > > +/* 8.1.10: MLD DVSEC */ > > +#define CXL_DVSEC_MLD 9 > > + > > +/* 14.16.1 CXL Device Test Capability Advertisement */ > > +#define CXL_DVSEC_PCIE_TEST_CAPABILITY 10 > > > > #define cxl_reg_block(pdev, map) \ > > ((resource_size_t)(pci_resource_start(pdev, (map)->barno) + \ > > -- > > 2.33.1 > >
On Mon, Nov 1, 2021 at 3:00 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > On 21-10-31 13:18:36, Dan Williams wrote: > > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > Get a better naming scheme in place for upcoming additions. To solidify > > > the schema, add all the DVSEC identifiers to start with. > > > > The title and this changelog don't give anything of substance to > > review the patch. > > > > This also looks like a rename and addition of more definitions. The > > rename has one rationale, the additional definitions have a different > > one, so split those into 2 patches, or fold the additions into the > > patch that uses them. > > > > I added more than necessary as a means to codify the naming scheme [as stated]. > Many of them are not used. I can split this patch into two, though I personally > don't find it offensive to do it as one. Before I do that, I'd like to know > though if you're going to reject the patch if I'm not actually using all of the > defines later on. > They don't all need to be used, but a rationale for adding them is the request.
On Fri, 22 Oct 2021 11:36:54 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > Get a better naming scheme in place for upcoming additions. To solidify > the schema, add all the DVSEC identifiers to start with. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > > --- > See: > https://lore.kernel.org/linux-pci/20210913190131.xiiszmno46qie7v5@intel.com/ > --- > drivers/cxl/core/regs.c | 14 ++++++++------ > drivers/cxl/pci.h | 38 ++++++++++++++++++++++++++++++-------- > 2 files changed, 38 insertions(+), 14 deletions(-) > > diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c > index c8ab8880b81b..b837196fbf39 100644 > --- a/drivers/cxl/core/regs.c > +++ b/drivers/cxl/core/regs.c > @@ -253,9 +253,11 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi, > struct cxl_register_map *map) > { > map->block_offset = > - ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); > - map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); > - map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); > + ((u64)reg_hi << 32) | > + (reg_lo & DVSEC_REGISTER_LOCATOR_BLOCK_OFFSET_LOW_MASK); > + map->barno = FIELD_GET(DVSEC_REGISTER_LOCATOR_BIR_MASK, reg_lo); > + map->reg_type = > + FIELD_GET(DVSEC_REGISTER_LOCATOR_BLOCK_IDENTIFIER_MASK, reg_lo); > } > > /** > @@ -276,15 +278,15 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, > int regloc, i; > > regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, > - PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID); > + CXL_DVSEC_REGISTER_LOCATOR); > if (!regloc) > return -ENXIO; > > pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); > regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); > > - regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET; > - regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8; > + regloc += DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET; > + regblocks = (regloc_size - DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET) / 8; > > for (i = 0; i < regblocks; i++, regloc += 8) { > u32 reg_lo, reg_hi; > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > index 12fdcb1b14e5..fe2898b17736 100644 > --- a/drivers/cxl/pci.h > +++ b/drivers/cxl/pci.h > @@ -7,17 +7,36 @@ > > /* > * See section 8.1 Configuration Space Registers in the CXL 2.0 > - * Specification > + * Specification. Names are taken straight from the specification with "CXL" and > + * "DVSEC" redundancies removed. > */ > #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) > #define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 > -#define PCI_DVSEC_ID_CXL 0x0 > > -#define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID 0x8 > -#define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET 0xC > +/* 8.1.3: PCIe DVSEC for CXL Device */ > +#define CXL_DVSEC_PCIE_DEVICE 0 > > -/* BAR Indicator Register (BIR) */ > -#define CXL_REGLOC_BIR_MASK GENMASK(2, 0) > +/* 8.1.4: Non-CXL Function Map DVSEC */ > +#define CXL_DVSEC_FUNCTION_MAP 2 > + > +/* 8.1.5: CXL 2.0 Extensions DVSEC for Ports */ > +#define CXL_DVSEC_PORT_EXTENSIONS 3 > + > +/* 8.1.6: GPF DVSEC for CXL Port */ > +#define CXL_DVSEC_PORT_GPF 4 > + > +/* 8.1.7: GPF DVSEC for CXL Device */ > +#define CXL_DVSEC_DEVICE_GPF 5 > + > +/* 8.1.8: PCIe DVSEC for Flex Bus Port */ > +#define CXL_DVSEC_PCIE_FLEXBUS_PORT 7 > + > +/* 8.1.9: Register Locator DVSEC */ > +#define CXL_DVSEC_REGISTER_LOCATOR 8 > +#define DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET 0xC Hmm. I'm not particularly keen on not having CXL in the naming for the offset and fields. They get used in places where, to the casual reader, they may look like generic PCI_DVSEC_ defintiions.... the do get very long though, but I'm not sure that's the right place to abbreviate. Agreed with your comment in the thread that it's really hard to get a good consistent model for what are near enough free form names in a spec (or seem like it when read long after they were defined). > +#define DVSEC_REGISTER_LOCATOR_BIR_MASK GENMASK(2, 0) > +#define DVSEC_REGISTER_LOCATOR_BLOCK_IDENTIFIER_MASK GENMASK(15, 8) > +#define DVSEC_REGISTER_LOCATOR_BLOCK_OFFSET_LOW_MASK GENMASK(31, 16) > > /* Register Block Identifier (RBI) */ > enum cxl_regloc_type { > @@ -28,8 +47,11 @@ enum cxl_regloc_type { > CXL_REGLOC_RBI_TYPES > }; > > -#define CXL_REGLOC_RBI_MASK GENMASK(15, 8) > -#define CXL_REGLOC_ADDR_MASK GENMASK(31, 16) > +/* 8.1.10: MLD DVSEC */ > +#define CXL_DVSEC_MLD 9 > + > +/* 14.16.1 CXL Device Test Capability Advertisement */ > +#define CXL_DVSEC_PCIE_TEST_CAPABILITY 10 > > #define cxl_reg_block(pdev, map) \ > ((resource_size_t)(pci_resource_start(pdev, (map)->barno) + \
On 21-11-03 15:53:59, Jonathan Cameron wrote: > On Fri, 22 Oct 2021 11:36:54 -0700 > Ben Widawsky <ben.widawsky@intel.com> wrote: > [snip] > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > index 12fdcb1b14e5..fe2898b17736 100644 > > --- a/drivers/cxl/pci.h > > +++ b/drivers/cxl/pci.h > > @@ -7,17 +7,36 @@ > > > > /* > > * See section 8.1 Configuration Space Registers in the CXL 2.0 > > - * Specification > > + * Specification. Names are taken straight from the specification with "CXL" and > > + * "DVSEC" redundancies removed. > > */ > > #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) > > #define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 > > -#define PCI_DVSEC_ID_CXL 0x0 > > > > -#define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID 0x8 > > -#define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET 0xC > > +/* 8.1.3: PCIe DVSEC for CXL Device */ > > +#define CXL_DVSEC_PCIE_DEVICE 0 > > > > -/* BAR Indicator Register (BIR) */ > > -#define CXL_REGLOC_BIR_MASK GENMASK(2, 0) > > +/* 8.1.4: Non-CXL Function Map DVSEC */ > > +#define CXL_DVSEC_FUNCTION_MAP 2 > > + > > +/* 8.1.5: CXL 2.0 Extensions DVSEC for Ports */ > > +#define CXL_DVSEC_PORT_EXTENSIONS 3 > > + > > +/* 8.1.6: GPF DVSEC for CXL Port */ > > +#define CXL_DVSEC_PORT_GPF 4 > > + > > +/* 8.1.7: GPF DVSEC for CXL Device */ > > +#define CXL_DVSEC_DEVICE_GPF 5 > > + > > +/* 8.1.8: PCIe DVSEC for Flex Bus Port */ > > +#define CXL_DVSEC_PCIE_FLEXBUS_PORT 7 > > + > > +/* 8.1.9: Register Locator DVSEC */ > > +#define CXL_DVSEC_REGISTER_LOCATOR 8 > > +#define DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET 0xC > > Hmm. I'm not particularly keen on not having CXL in the naming for the offset > and fields. They get used in places where, to the casual reader, they may look > like generic PCI_DVSEC_ defintiions.... the do get very long though, but I'm not > sure that's the right place to abbreviate. > > Agreed with your comment in the thread that it's really hard to > get a good consistent model for what are near enough free form names > in a spec (or seem like it when read long after they were defined). > Hi Jonathan, thanks for looking. Well for these in particular, I'd expect the actual register/DVSEC offset to have been read a few lines above, so I'm not too worried about these ones specifically. However, it's a good point as this grows it may become problematic. Do you have a suggestion? My ideal would be for the CXL consortium to publish header files for these things and then we can just grumble about how silly they are and move along, but I'm not sure that will happen. Anyway... I really don't care what we choose as long as it's consistent. What color would you like? [snip]
On Wed, 3 Nov 2021 09:03:36 -0700 Ben Widawsky <ben.widawsky@intel.com> wrote: > On 21-11-03 15:53:59, Jonathan Cameron wrote: > > On Fri, 22 Oct 2021 11:36:54 -0700 > > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > [snip] > > > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > > index 12fdcb1b14e5..fe2898b17736 100644 > > > --- a/drivers/cxl/pci.h > > > +++ b/drivers/cxl/pci.h > > > @@ -7,17 +7,36 @@ > > > > > > /* > > > * See section 8.1 Configuration Space Registers in the CXL 2.0 > > > - * Specification > > > + * Specification. Names are taken straight from the specification with "CXL" and > > > + * "DVSEC" redundancies removed. > > > */ > > > #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) > > > #define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 > > > -#define PCI_DVSEC_ID_CXL 0x0 > > > > > > -#define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID 0x8 > > > -#define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET 0xC > > > +/* 8.1.3: PCIe DVSEC for CXL Device */ > > > +#define CXL_DVSEC_PCIE_DEVICE 0 > > > > > > -/* BAR Indicator Register (BIR) */ > > > -#define CXL_REGLOC_BIR_MASK GENMASK(2, 0) > > > +/* 8.1.4: Non-CXL Function Map DVSEC */ > > > +#define CXL_DVSEC_FUNCTION_MAP 2 > > > + > > > +/* 8.1.5: CXL 2.0 Extensions DVSEC for Ports */ > > > +#define CXL_DVSEC_PORT_EXTENSIONS 3 > > > + > > > +/* 8.1.6: GPF DVSEC for CXL Port */ > > > +#define CXL_DVSEC_PORT_GPF 4 > > > + > > > +/* 8.1.7: GPF DVSEC for CXL Device */ > > > +#define CXL_DVSEC_DEVICE_GPF 5 > > > + > > > +/* 8.1.8: PCIe DVSEC for Flex Bus Port */ > > > +#define CXL_DVSEC_PCIE_FLEXBUS_PORT 7 > > > + > > > +/* 8.1.9: Register Locator DVSEC */ > > > +#define CXL_DVSEC_REGISTER_LOCATOR 8 > > > +#define DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET 0xC > > > > Hmm. I'm not particularly keen on not having CXL in the naming for the offset > > and fields. They get used in places where, to the casual reader, they may look > > like generic PCI_DVSEC_ defintiions.... the do get very long though, but I'm not > > sure that's the right place to abbreviate. > > > > Agreed with your comment in the thread that it's really hard to > > get a good consistent model for what are near enough free form names > > in a spec (or seem like it when read long after they were defined). > > > > Hi Jonathan, thanks for looking. > > Well for these in particular, I'd expect the actual register/DVSEC offset to > have been read a few lines above, so I'm not too worried about these ones > specifically. However, it's a good point as this grows it may become > problematic. Do you have a suggestion? My ideal would be for the CXL consortium > to publish header files for these things and then we can just grumble about how > silly they are and move along, but I'm not sure that will happen. :) You can propose it... - Can you imagine how long those calls would go on for?????? > > Anyway... I really don't care what we choose as long as it's consistent. What > color would you like? Blue, no yellow.... Ahhhhhhh! I think a bit of flexibility on abbreviations is needed when they get insanely long... Meh, we can always rename them all later when we decide the choice was wrong. > > [snip] >
On 21-11-03 16:42:32, Jonathan Cameron wrote: > On Wed, 3 Nov 2021 09:03:36 -0700 > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > On 21-11-03 15:53:59, Jonathan Cameron wrote: > > > On Fri, 22 Oct 2021 11:36:54 -0700 > > > Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > [snip] > > > > > > diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h > > > > index 12fdcb1b14e5..fe2898b17736 100644 > > > > --- a/drivers/cxl/pci.h > > > > +++ b/drivers/cxl/pci.h > > > > @@ -7,17 +7,36 @@ > > > > > > > > /* > > > > * See section 8.1 Configuration Space Registers in the CXL 2.0 > > > > - * Specification > > > > + * Specification. Names are taken straight from the specification with "CXL" and > > > > + * "DVSEC" redundancies removed. > > > > */ > > > > #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) > > > > #define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 > > > > -#define PCI_DVSEC_ID_CXL 0x0 > > > > > > > > -#define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID 0x8 > > > > -#define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET 0xC > > > > +/* 8.1.3: PCIe DVSEC for CXL Device */ > > > > +#define CXL_DVSEC_PCIE_DEVICE 0 > > > > > > > > -/* BAR Indicator Register (BIR) */ > > > > -#define CXL_REGLOC_BIR_MASK GENMASK(2, 0) > > > > +/* 8.1.4: Non-CXL Function Map DVSEC */ > > > > +#define CXL_DVSEC_FUNCTION_MAP 2 > > > > + > > > > +/* 8.1.5: CXL 2.0 Extensions DVSEC for Ports */ > > > > +#define CXL_DVSEC_PORT_EXTENSIONS 3 > > > > + > > > > +/* 8.1.6: GPF DVSEC for CXL Port */ > > > > +#define CXL_DVSEC_PORT_GPF 4 > > > > + > > > > +/* 8.1.7: GPF DVSEC for CXL Device */ > > > > +#define CXL_DVSEC_DEVICE_GPF 5 > > > > + > > > > +/* 8.1.8: PCIe DVSEC for Flex Bus Port */ > > > > +#define CXL_DVSEC_PCIE_FLEXBUS_PORT 7 > > > > + > > > > +/* 8.1.9: Register Locator DVSEC */ > > > > +#define CXL_DVSEC_REGISTER_LOCATOR 8 > > > > +#define DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET 0xC > > > > > > Hmm. I'm not particularly keen on not having CXL in the naming for the offset > > > and fields. They get used in places where, to the casual reader, they may look > > > like generic PCI_DVSEC_ defintiions.... the do get very long though, but I'm not > > > sure that's the right place to abbreviate. > > > > > > Agreed with your comment in the thread that it's really hard to > > > get a good consistent model for what are near enough free form names > > > in a spec (or seem like it when read long after they were defined). > > > > > > > Hi Jonathan, thanks for looking. > > > > Well for these in particular, I'd expect the actual register/DVSEC offset to > > have been read a few lines above, so I'm not too worried about these ones > > specifically. However, it's a good point as this grows it may become > > problematic. Do you have a suggestion? My ideal would be for the CXL consortium > > to publish header files for these things and then we can just grumble about how > > silly they are and move along, but I'm not sure that will happen. > > :) You can propose it... - Can you imagine how long those calls would go on > for?????? > > > > > Anyway... I really don't care what we choose as long as it's consistent. What > > color would you like? > > Blue, no yellow.... Ahhhhhhh! > > I think a bit of flexibility on abbreviations is needed when they get insanely > long... Meh, we can always rename them all later when we decide the choice was > wrong. Two very good choices. Ack. I'll respin. Let me spell out though in case it wasn't clear, my goal was to make something grepable, which abbreviations take away from. This is because as the 3.0 spec goes through I'm noticing our chapter references are no longer 100% accurate (annoying!). > > > > > [snip] > > >
diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c index c8ab8880b81b..b837196fbf39 100644 --- a/drivers/cxl/core/regs.c +++ b/drivers/cxl/core/regs.c @@ -253,9 +253,11 @@ static void cxl_decode_regblock(u32 reg_lo, u32 reg_hi, struct cxl_register_map *map) { map->block_offset = - ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK); - map->barno = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo); - map->reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo); + ((u64)reg_hi << 32) | + (reg_lo & DVSEC_REGISTER_LOCATOR_BLOCK_OFFSET_LOW_MASK); + map->barno = FIELD_GET(DVSEC_REGISTER_LOCATOR_BIR_MASK, reg_lo); + map->reg_type = + FIELD_GET(DVSEC_REGISTER_LOCATOR_BLOCK_IDENTIFIER_MASK, reg_lo); } /** @@ -276,15 +278,15 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type, int regloc, i; regloc = pci_find_dvsec_capability(pdev, PCI_DVSEC_VENDOR_ID_CXL, - PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID); + CXL_DVSEC_REGISTER_LOCATOR); if (!regloc) return -ENXIO; pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, ®loc_size); regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size); - regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET; - regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8; + regloc += DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET; + regblocks = (regloc_size - DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET) / 8; for (i = 0; i < regblocks; i++, regloc += 8) { u32 reg_lo, reg_hi; diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h index 12fdcb1b14e5..fe2898b17736 100644 --- a/drivers/cxl/pci.h +++ b/drivers/cxl/pci.h @@ -7,17 +7,36 @@ /* * See section 8.1 Configuration Space Registers in the CXL 2.0 - * Specification + * Specification. Names are taken straight from the specification with "CXL" and + * "DVSEC" redundancies removed. */ #define PCI_DVSEC_HEADER1_LENGTH_MASK GENMASK(31, 20) #define PCI_DVSEC_VENDOR_ID_CXL 0x1E98 -#define PCI_DVSEC_ID_CXL 0x0 -#define PCI_DVSEC_ID_CXL_REGLOC_DVSEC_ID 0x8 -#define PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET 0xC +/* 8.1.3: PCIe DVSEC for CXL Device */ +#define CXL_DVSEC_PCIE_DEVICE 0 -/* BAR Indicator Register (BIR) */ -#define CXL_REGLOC_BIR_MASK GENMASK(2, 0) +/* 8.1.4: Non-CXL Function Map DVSEC */ +#define CXL_DVSEC_FUNCTION_MAP 2 + +/* 8.1.5: CXL 2.0 Extensions DVSEC for Ports */ +#define CXL_DVSEC_PORT_EXTENSIONS 3 + +/* 8.1.6: GPF DVSEC for CXL Port */ +#define CXL_DVSEC_PORT_GPF 4 + +/* 8.1.7: GPF DVSEC for CXL Device */ +#define CXL_DVSEC_DEVICE_GPF 5 + +/* 8.1.8: PCIe DVSEC for Flex Bus Port */ +#define CXL_DVSEC_PCIE_FLEXBUS_PORT 7 + +/* 8.1.9: Register Locator DVSEC */ +#define CXL_DVSEC_REGISTER_LOCATOR 8 +#define DVSEC_REGISTER_LOCATOR_BLOCK1_OFFSET 0xC +#define DVSEC_REGISTER_LOCATOR_BIR_MASK GENMASK(2, 0) +#define DVSEC_REGISTER_LOCATOR_BLOCK_IDENTIFIER_MASK GENMASK(15, 8) +#define DVSEC_REGISTER_LOCATOR_BLOCK_OFFSET_LOW_MASK GENMASK(31, 16) /* Register Block Identifier (RBI) */ enum cxl_regloc_type { @@ -28,8 +47,11 @@ enum cxl_regloc_type { CXL_REGLOC_RBI_TYPES }; -#define CXL_REGLOC_RBI_MASK GENMASK(15, 8) -#define CXL_REGLOC_ADDR_MASK GENMASK(31, 16) +/* 8.1.10: MLD DVSEC */ +#define CXL_DVSEC_MLD 9 + +/* 14.16.1 CXL Device Test Capability Advertisement */ +#define CXL_DVSEC_PCIE_TEST_CAPABILITY 10 #define cxl_reg_block(pdev, map) \ ((resource_size_t)(pci_resource_start(pdev, (map)->barno) + \
Get a better naming scheme in place for upcoming additions. To solidify the schema, add all the DVSEC identifiers to start with. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- See: https://lore.kernel.org/linux-pci/20210913190131.xiiszmno46qie7v5@intel.com/ --- drivers/cxl/core/regs.c | 14 ++++++++------ drivers/cxl/pci.h | 38 ++++++++++++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 14 deletions(-)