On 9/4/24 14:52, Bjorn Helgaas wrote:

quoted

-#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

quoted

+#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.

quoted

+#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.