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