Messages in this thread | | | Date | Tue, 17 Jan 2023 20:42:03 +0000 | From | Conor Dooley <> | Subject | Re: [PATCH v2 1/9] RISC-V: Add AIA related CSR defines |
| |
Hey Anup,
I thought I had already replied here but clearly not, sorry!
On Mon, Jan 09, 2023 at 10:39:08AM +0530, Anup Patel wrote: > On Thu, Jan 5, 2023 at 4:37 AM Conor Dooley <conor@kernel.org> wrote: > > On Tue, Jan 03, 2023 at 07:44:01PM +0530, Anup Patel wrote:
> > > +/* AIA CSR bits */ > > > +#define TOPI_IID_SHIFT 16 > > > +#define TOPI_IID_MASK 0xfff
While I think of it, it'd be worth noting that these are generic across all of topi, mtopi etc. Initially I thought that this mask was wrong as the topi section says: bits 25:16 Interrupt identity (source number) bits 7:0 Interrupt priority
> > > +#define TOPI_IPRIO_MASK 0xff > > > +#define TOPI_IPRIO_BITS 8 > > > + > > > +#define TOPEI_ID_SHIFT 16 > > > +#define TOPEI_ID_MASK 0x7ff > > > +#define TOPEI_PRIO_MASK 0x7ff > > > + > > > +#define ISELECT_IPRIO0 0x30 > > > +#define ISELECT_IPRIO15 0x3f > > > +#define ISELECT_MASK 0x1ff > > > + > > > +#define HVICTL_VTI 0x40000000 > > > +#define HVICTL_IID 0x0fff0000 > > > +#define HVICTL_IID_SHIFT 16 > > > +#define HVICTL_IPRIOM 0x00000100 > > > +#define HVICTL_IPRIO 0x000000ff > > > > Why not name these as masks, like you did for the other masks? > > Also, the mask/shift defines appear inconsistent. TOPI_IID_MASK is > > intended to be used post-shift AFAICT, but HVICTL_IID_SHIFT is intended > > to be used *pre*-shift. > > Some consistency in naming and function would be great. > > The following convention is being followed in asm/csr.h for defining > MASK of any XYZ field in ABC CSR: > 1. ABC_XYZ : This name is used for MASK which is intended > to be used before SHIFT > 2. ABC_XYZ_MASK: This name is used for MASK which is > intended to be used after SHIFT
Which makes sense in theory.
> The existing defines for [M|S]STATUS, HSTATUS, SATP, and xENVCFG > follows the above convention. The only outlier is HGATPx_VMID_MASK > define which I will fix in my next KVM RISC-V series.
Yup, it is liable to end up like that.
> I don't see how any of the AIA CSR defines are violating the above > convention.
What I was advocating for was picking one style and sticking to it. These copy-paste from docs things are tedious and error prone to review, and I don't think having multiple styles is helpful.
Tedious as it was, I did check all the numbers though, so in that respect: Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Thanks, Conor.
[unhandled content-type:application/pgp-signature] | |