diff mbox series

[V4,02/12] PCI: Add TPH related register definition

Message ID 20240822204120.3634-3-wei.huang2@amd.com (mailing list archive)
State New
Headers show
Series PCIe TPH and cache direct injection support | expand

Commit Message

Wei Huang Aug. 22, 2024, 8:41 p.m. UTC 
Linux has some basic, but incomplete, definition for the TPH Requester
capability registers. Also the definitions of TPH Requester control
register and TPH Completer capability, as well as the ST fields of
MSI-X entry, are missing. Add all required definitions to support TPH
without changing the existing Linux UAPI.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 include/uapi/linux/pci_regs.h | 38 +++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

Comments

Bjorn Helgaas Sept. 4, 2024, 7:52 p.m. UTC | #1 
On Thu, Aug 22, 2024 at 03:41:10PM -0500, Wei Huang wrote:
> Linux has some basic, but incomplete, definition for the TPH Requester
> capability registers. Also the definitions of TPH Requester control
> register and TPH Completer capability, as well as the ST fields of
> MSI-X entry, are missing. Add all required definitions to support TPH
> without changing the existing Linux UAPI.

>  /* TPH Requester */
>  #define PCI_TPH_CAP		4	/* capability register */
> -#define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
> -#define   PCI_TPH_LOC_NONE	0x000	/* no location */
> -#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
> -#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
> -#define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
> -#define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
> -#define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
> +#define  PCI_TPH_CAP_NO_ST	0x00000001 /* No ST Mode Supported */
> +#define  PCI_TPH_CAP_INT_VEC	0x00000002 /* Interrupt Vector Mode Supported */
> +#define  PCI_TPH_CAP_DEV_SPEC	0x00000004 /* Device Specific Mode Supported */

I think these modes should all include "ST" to clearly delineate
Steering Tags from the Processing Hints.  E.g.,

  PCI_TPH_CAP_ST_NO_ST       or maybe PCI_TPH_CAP_ST_NONE
  PCI_TPH_CAP_ST_INT_VEC
  PCI_TPH_CAP_ST_DEV_SPEC

> +#define  PCI_TPH_CAP_EXT_TPH	0x00000100 /* Ext TPH Requester Supported */
> +#define  PCI_TPH_CAP_LOC_MASK	0x00000600 /* ST Table Location */
> +#define   PCI_TPH_LOC_NONE	0x00000000 /* Not present */
> +#define   PCI_TPH_LOC_CAP	0x00000200 /* In capability */
> +#define   PCI_TPH_LOC_MSIX	0x00000400 /* In MSI-X */

These are existing symbols just being tidied, so can't really add "ST"
here unless we just add aliases.  Since they're just used internally,
not by drivers, I think they're fine as-is.

> +#define  PCI_TPH_CAP_ST_MASK	0x07FF0000 /* ST Table Size */
> +#define  PCI_TPH_CAP_ST_SHIFT	16	/* ST Table Size shift */
> +#define PCI_TPH_BASE_SIZEOF	0xc	/* Size with no ST table */
> +
> +#define PCI_TPH_CTRL		8	/* control register */
> +#define  PCI_TPH_CTRL_MODE_SEL_MASK	0x00000007 /* ST Mode Select */
> +#define   PCI_TPH_NO_ST_MODE		0x0 /* No ST Mode */
> +#define   PCI_TPH_INT_VEC_MODE		0x1 /* Interrupt Vector Mode */
> +#define   PCI_TPH_DEV_SPEC_MODE		0x2 /* Device Specific Mode */

These are also internal, but they're new and I think they should also
include "ST" to match the CAP #defines.

Even better, maybe we only add these and use them for both CAP and
CTRL since they're defined with identical values.
Wei Huang Sept. 5, 2024, 3:08 p.m. UTC | #2 
On 9/4/24 14:52, Bjorn Helgaas wrote:
>> -#define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
>> -#define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
>> -#define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
>> +#define  PCI_TPH_CAP_NO_ST	0x00000001 /* No ST Mode Supported */
>> +#define  PCI_TPH_CAP_INT_VEC	0x00000002 /* Interrupt Vector Mode Supported */
>> +#define  PCI_TPH_CAP_DEV_SPEC	0x00000004 /* Device Specific Mode Supported */
> 
> I think these modes should all include "ST" to clearly delineate
> Steering Tags from the Processing Hints.  E.g.,
> 
>   PCI_TPH_CAP_ST_NO_ST       or maybe PCI_TPH_CAP_ST_NONE

Can I keep "NO_ST" instead of switching over to "ST_NONE"? First, it
matches with PCIe spec. Secondly, IMO "ST_NONE" implies no ST support at
all.

>   PCI_TPH_CAP_ST_INT_VEC
>   PCI_TPH_CAP_ST_DEV_SPEC

Will change

> 
>> +#define  PCI_TPH_CAP_EXT_TPH	0x00000100 /* Ext TPH Requester Supported */
>> +#define  PCI_TPH_CAP_LOC_MASK	0x00000600 /* ST Table Location */
>> +#define   PCI_TPH_LOC_NONE	0x00000000 /* Not present */
>> +#define   PCI_TPH_LOC_CAP	0x00000200 /* In capability */
>> +#define   PCI_TPH_LOC_MSIX	0x00000400 /* In MSI-X */
> 
> These are existing symbols just being tidied, so can't really add "ST"
> here unless we just add aliases.  Since they're just used internally,
> not by drivers, I think they're fine as-is.
> 

Yes. In V1 review, Alex Williamson and Jonathan Cameron asked not to
change these definitions as they might be used by existing software.

>> +#define  PCI_TPH_CAP_ST_MASK	0x07FF0000 /* ST Table Size */
>> +#define  PCI_TPH_CAP_ST_SHIFT	16	/* ST Table Size shift */
>> +#define PCI_TPH_BASE_SIZEOF	0xc	/* Size with no ST table */
>> +
>> +#define PCI_TPH_CTRL		8	/* control register */
>> +#define  PCI_TPH_CTRL_MODE_SEL_MASK	0x00000007 /* ST Mode Select */
>> +#define   PCI_TPH_NO_ST_MODE		0x0 /* No ST Mode */
>> +#define   PCI_TPH_INT_VEC_MODE		0x1 /* Interrupt Vector Mode */
>> +#define   PCI_TPH_DEV_SPEC_MODE		0x2 /* Device Specific Mode */
> 
> These are also internal, but they're new and I think they should also
> include "ST" to match the CAP #defines.
> 
> Even better, maybe we only add these and use them for both CAP and
> CTRL since they're defined with identical values.

Can you elaborate here? In CTRL register, "ST Mode Select" is defined as
a 2-bit field. The possible values are 0, 1, 2. But in CAP register, the
modes are individual bit masked. So I cannot use CTRL definitions in CAP
register directly unless I do shifting.
Bjorn Helgaas Sept. 5, 2024, 4:41 p.m. UTC | #3 
On Thu, Sep 05, 2024 at 10:08:33AM -0500, Wei Huang wrote:
> On 9/4/24 14:52, Bjorn Helgaas wrote:
> >> -#define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
> >> -#define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
> >> -#define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
> >> +#define  PCI_TPH_CAP_NO_ST	0x00000001 /* No ST Mode Supported */
> >> +#define  PCI_TPH_CAP_INT_VEC	0x00000002 /* Interrupt Vector Mode Supported */
> >> +#define  PCI_TPH_CAP_DEV_SPEC	0x00000004 /* Device Specific Mode Supported */
> > 
> > I think these modes should all include "ST" to clearly delineate
> > Steering Tags from the Processing Hints.  E.g.,
> > 
> >   PCI_TPH_CAP_ST_NO_ST       or maybe PCI_TPH_CAP_ST_NONE
> 
> Can I keep "NO_ST" instead of switching over to "ST_NONE"? First, it
> matches with PCIe spec. Secondly, IMO "ST_NONE" implies no ST support at
> all.

Sure.  Does PCI_TPH_CAP_ST_NO_ST work for you?  That follows the same
PCI_TPH_CAP_ST_* pattern as below.

> >   PCI_TPH_CAP_ST_INT_VEC
> >   PCI_TPH_CAP_ST_DEV_SPEC
> 
> Will change

> >> +#define  PCI_TPH_CAP_ST_MASK	0x07FF0000 /* ST Table Size */
> >> +#define  PCI_TPH_CAP_ST_SHIFT	16	/* ST Table Size shift */
> >> +#define PCI_TPH_BASE_SIZEOF	0xc	/* Size with no ST table */
> >> +
> >> +#define PCI_TPH_CTRL		8	/* control register */
> >> +#define  PCI_TPH_CTRL_MODE_SEL_MASK	0x00000007 /* ST Mode Select */
> >> +#define   PCI_TPH_NO_ST_MODE		0x0 /* No ST Mode */
> >> +#define   PCI_TPH_INT_VEC_MODE		0x1 /* Interrupt Vector Mode */
> >> +#define   PCI_TPH_DEV_SPEC_MODE		0x2 /* Device Specific Mode */
> > 
> > These are also internal, but they're new and I think they should also
> > include "ST" to match the CAP #defines.
> > 
> > Even better, maybe we only add these and use them for both CAP and
> > CTRL since they're defined with identical values.
> 
> Can you elaborate here? In CTRL register, "ST Mode Select" is defined as
> a 2-bit field. The possible values are 0, 1, 2. But in CAP register, the
> modes are individual bit masked. So I cannot use CTRL definitions in CAP
> register directly unless I do shifting.

Oops, sorry, I thought they were the same values, but they're not, so
ignore this comment.
diff mbox series

Patch

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 94c00996e633..643236f43f4d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -340,7 +340,9 @@ 
 #define PCI_MSIX_ENTRY_UPPER_ADDR	0x4  /* Message Upper Address */
 #define PCI_MSIX_ENTRY_DATA		0x8  /* Message Data */
 #define PCI_MSIX_ENTRY_VECTOR_CTRL	0xc  /* Vector Control */
-#define  PCI_MSIX_ENTRY_CTRL_MASKBIT	0x00000001
+#define  PCI_MSIX_ENTRY_CTRL_MASKBIT	0x00000001  /* Mask Bit */
+#define  PCI_MSIX_ENTRY_CTRL_ST_LOWER	0x00ff0000  /* ST Lower */
+#define  PCI_MSIX_ENTRY_CTRL_ST_UPPER	0xff000000  /* ST Upper */
 
 /* CompactPCI Hotswap Register */
 
@@ -657,6 +659,7 @@ 
 #define  PCI_EXP_DEVCAP2_ATOMIC_COMP64	0x00000100 /* 64b AtomicOp completion */
 #define  PCI_EXP_DEVCAP2_ATOMIC_COMP128	0x00000200 /* 128b AtomicOp completion */
 #define  PCI_EXP_DEVCAP2_LTR		0x00000800 /* Latency tolerance reporting */
+#define  PCI_EXP_DEVCAP2_TPH_COMP_MASK	0x00003000 /* TPH completer support */
 #define  PCI_EXP_DEVCAP2_OBFF_MASK	0x000c0000 /* OBFF support mechanism */
 #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
 #define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
@@ -1020,15 +1023,34 @@ 
 #define  PCI_DPA_CAP_SUBSTATE_MASK	0x1F	/* # substates - 1 */
 #define PCI_DPA_BASE_SIZEOF	16	/* size with 0 substates */
 
+/* TPH Completer Support */
+#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
+#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
+#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */
+
 /* TPH Requester */
 #define PCI_TPH_CAP		4	/* capability register */
-#define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
-#define   PCI_TPH_LOC_NONE	0x000	/* no location */
-#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
-#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
-#define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
-#define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
-#define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
+#define  PCI_TPH_CAP_NO_ST	0x00000001 /* No ST Mode Supported */
+#define  PCI_TPH_CAP_INT_VEC	0x00000002 /* Interrupt Vector Mode Supported */
+#define  PCI_TPH_CAP_DEV_SPEC	0x00000004 /* Device Specific Mode Supported */
+#define  PCI_TPH_CAP_EXT_TPH	0x00000100 /* Ext TPH Requester Supported */
+#define  PCI_TPH_CAP_LOC_MASK	0x00000600 /* ST Table Location */
+#define   PCI_TPH_LOC_NONE	0x00000000 /* Not present */
+#define   PCI_TPH_LOC_CAP	0x00000200 /* In capability */
+#define   PCI_TPH_LOC_MSIX	0x00000400 /* In MSI-X */
+#define  PCI_TPH_CAP_ST_MASK	0x07FF0000 /* ST Table Size */
+#define  PCI_TPH_CAP_ST_SHIFT	16	/* ST Table Size shift */
+#define PCI_TPH_BASE_SIZEOF	0xc	/* Size with no ST table */
+
+#define PCI_TPH_CTRL		8	/* control register */
+#define  PCI_TPH_CTRL_MODE_SEL_MASK	0x00000007 /* ST Mode Select */
+#define   PCI_TPH_NO_ST_MODE		0x0 /* No ST Mode */
+#define   PCI_TPH_INT_VEC_MODE		0x1 /* Interrupt Vector Mode */
+#define   PCI_TPH_DEV_SPEC_MODE		0x2 /* Device Specific Mode */
+#define  PCI_TPH_CTRL_REQ_EN_MASK	0x00000300 /* TPH Requester Enable */
+#define   PCI_TPH_REQ_DISABLE		0x0 /* No TPH requests allowed */
+#define   PCI_TPH_REQ_TPH_ONLY		0x1 /* TPH only requests allowed */
+#define   PCI_TPH_REQ_EXT_TPH		0x3 /* Extended TPH requests allowed */
 
 /* Downstream Port Containment */
 #define PCI_EXP_DPC_CAP			0x04	/* DPC Capability */