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

[Dan Williams <dan.j.williams@intel.com>]

Xu Yilun wrote:
> > > +/* 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?

Agree, updated.

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

I also agree with this. I had copied these from Alexey, but for the ones
that actually got used in the code I ended up using mask defines with
FIELD_PREP/GET(). I think I will just delete them for now, and we can
add them back as masks later.

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

Agree, I ended up with this definition inline in ide.c:

#define SEL_ADDR1_LOWER_MASK GENMASK(31, 20)
#define PREP_PCI_IDE_SEL_ADDR1(base, limit)                   \
        FIELD_PREP(PCI_IDE_SEL_ADDR_1_VALID, 1) |             \
        FIELD_PREP(PCI_IDE_SEL_ADDR_1_BASE_LOW_MASK,          \
                   FIELD_GET(SEL_ADDR1_LOWER_MASK, (base))) | \
        FIELD_PREP(PCI_IDE_SEL_ADDR_1_LIMIT_LOW_MASK,         \
                   FIELD_GET(SEL_ADDR1_LOWER_MASK, (limit)))