Re: [PATCH 04/11] PCI/IDE: Selective Stream IDE enumeration

[Xu Yilun <yilun.xu@linux.intel.com>]

> > +/* 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
> 
>