> > +/* Selective IDE Stream Capability Register */ > > +#define PCI_IDE_SEL_CAP 0 > > +#define PCI_IDE_SEL_CAP_ASSOC_NUM(x) ((x) & 0xf) /* Address Association Register Blocks Number */ > > +#define PCI_IDE_SEL_CAP_ASSOC_MASK 0xf PCI_IDE_SEL_CAP_ASSOC_NUM_MASK is better? > > +/* Selective IDE Stream Control Register */ > > +#define PCI_IDE_SEL_CTL 4 > > +#define PCI_IDE_SEL_CTL_EN 0x1 /* Selective IDE Stream Enable */ > > +#define PCI_IDE_SEL_CTL_TX_AGGR_NPR(x) (((x) >> 2) & 0x3) /* Tx Aggregation Mode NPR */ > > +#define PCI_IDE_SEL_CTL_TX_AGGR_PR(x) (((x) >> 4) & 0x3) /* Tx Aggregation Mode PR */ > > +#define PCI_IDE_SEL_CTL_TX_AGGR_CPL(x) (((x) >> 6) & 0x3) /* Tx Aggregation Mode CPL */ These fields are more likely to be written to the register than read out, so may need other definitions. I think generally _XXX(x) Macros are less useful than _MASK because of FIELD_PREP/GET(), so maybe by default we define _MASK Macros and on demand define _XXX(x) Macros for all registers. > > +#define PCI_IDE_SEL_CTL_PCRC_EN 0x100 /* PCRC Enable */ > > +#define PCI_IDE_SEL_CTL_CFG_EN 0x200 /* Selective IDE for Configuration Requests */ > > +#define PCI_IDE_SEL_CTL_PART_ENC(x) (((x) >> 10) & 0xf) /* Partial Header Encryption Mode */ > > +#define PCI_IDE_SEL_CTL_ALG(x) (((x) >> 14) & 0x1f) /* Selected Algorithm */ > > +#define PCI_IDE_SEL_CTL_TC(x) (((x) >> 19) & 0x7) /* Traffic Class */ > > +#define PCI_IDE_SEL_CTL_DEFAULT 0x400000 /* Default Stream */ > > +#define PCI_IDE_SEL_CTL_TEE_LIMITED (1 << 23) /* TEE-Limited Stream */ > > +#define PCI_IDE_SEL_CTL_ID_MASK 0xff000000 > > +#define PCI_IDE_SEL_CTL_ID_MAX 255 > > +/* Selective IDE Stream Status Register */ > > +#define PCI_IDE_SEL_STS 8 > > +#define PCI_IDE_SEL_STS_STATUS(x) ((x) & 0xf) /* Selective IDE Stream State */ > > +#define PCI_IDE_SEL_STS_RECVD_INTEGRITY_CHECK 0x80000000 /* Received Integrity Check Fail Msg */ > > +/* IDE RID Association Register 1 */ > > +#define PCI_IDE_SEL_RID_1 12 > > +#define PCI_IDE_SEL_RID_1_LIMIT_MASK 0xffff00 > > +/* IDE RID Association Register 2 */ > > +#define PCI_IDE_SEL_RID_2 16 > > +#define PCI_IDE_SEL_RID_2_VALID 0x1 > > +#define PCI_IDE_SEL_RID_2_BASE_MASK 0x00ffff00 > > +#define PCI_IDE_SEL_RID_2_SEG_MASK 0xff000000 > > +/* Selective IDE Address Association Register Block, up to PCI_IDE_SEL_CAP_ASSOC_NUM */ > > +#define PCI_IDE_SEL_ADDR_1(x) (20 + (x) * 12) > > +#define PCI_IDE_SEL_ADDR_1_VALID 0x1 > > +#define PCI_IDE_SEL_ADDR_1_BASE_LOW_MASK 0x000fff0 > > 0x000fff00 (missing a zero) > > > +#define PCI_IDE_SEL_ADDR_1_BASE_LOW_SHIFT 20 > > +#define PCI_IDE_SEL_ADDR_1_LIMIT_LOW_MASK 0xfff0000 > > > 0xfff00000 > > 31:20 Memory Limit Lower – Corresponds to Address bits [31:20]. Address bits > [19:0] are implicitly F_FFFFh. RW > 19:8 Memory Base Lower – Corresponds to Address bits [31:20]. Address[19:0] > bits are implicitly 0_0000h. > > > > +#define PCI_IDE_SEL_ADDR_1_LIMIT_LOW_SHIFT 20 I don't think _SHIFT MACRO is needed, also because of FIELD_PREP/GET(). > > I like mine better :) Shows in one place how addr_1 is made: > > #define PCI_IDE_SEL_ADDR_1(v, base, limit) \ > ((FIELD_GET(0xfff00000, (limit)) << 20) | \ > (FIELD_GET(0xfff00000, (base)) << 8) | \ > ((v) ? 1 : 0)) This Macro is useful for SEL_ADDR_1 but not generally useful for other registers like SEL_CTRL, which has far more fields to input. So I'd rather have only _MASK Macros here to make things simpler. This specific Macro for SEL_ADDR_1 could be put in like pci-ide.h if really needed. Thanks, Yilun > > Also, when something uses "SHIFT", I expect left shift (like, PAGE_SHIFT), > but this is right shift. > > Otherwise, looks good. Thanks, > > > +/* IDE Address Association Register 2 is "Memory Limit Upper" */ > > +/* IDE Address Association Register 3 is "Memory Base Upper" */ > > +#define PCI_IDE_SEL_ADDR_2(x) (24 + (x) * 12) > > +#define PCI_IDE_SEL_ADDR_3(x) (28 + (x) * 12) > > + > > #endif /* LINUX_PCI_REGS_H */ > > > > -- > Alexey > >
