From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [RFC PATCH v3 05/18] xen/arm: ITS: Port ITS driver to xen Date: Mon, 29 Jun 2015 17:49:32 +0100 Message-ID: <1435596572.32500.399.camel@citrix.com> References: <1434974517-12136-1-git-send-email-vijay.kilari@gmail.com> <1434974517-12136-6-git-send-email-vijay.kilari@gmail.com> <1435577949.32500.276.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Vijay Kilari Cc: Stefano Stabellini , Prasun Kapoor , Vijaya Kumar K , Tim Deegan , "xen-devel@lists.xen.org" , Julien Grall , Stefano Stabellini , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org On Mon, 2015-06-29 at 21:17 +0530, Vijay Kilari wrote: > On Mon, Jun 29, 2015 at 9:13 PM, Vijay Kilari wrote: > > On Mon, Jun 29, 2015 at 5:09 PM, Ian Campbell wrote: > >> On Mon, 2015-06-22 at 17:31 +0530, vijay.kilari@gmail.com wrote: > >> [...] > >>> +/* > >>> + * ITS command descriptors - parameters to be encoded in a command > >>> + * block. > >>> + */ > >>> +struct its_cmd_desc { > >>> + union { > >>> + struct { > >>> + struct its_collection *col; > >>> + u32 event_id; > >>> + u32 dev_id; > >>> + } its_inv_cmd; > >> [...] > >>> +static struct its_collection *its_build_inv_cmd(its_cmd_block *cmd, > >>> + struct its_cmd_desc *desc) > >>> +{ > >>> + memset(cmd, 0x0, sizeof(its_cmd_block)); > >>> + cmd->inv.cmd = GITS_CMD_INV; > >>> + cmd->inv.devid = desc->its_inv_cmd.dev_id; > >>> + cmd->inv.event = desc->its_inv_cmd.event_id; > >>> + > >>> +#ifdef DEBUG_GIC_ITS > >>> + dump_cmd(cmd); > >>> +#endif > >>> + > >>> + return desc->its_inv_cmd.col; > >>> +} > >> [...] > >>> +void its_send_inv(struct its_device *dev, struct its_collection *col, > >>> + u32 event_id) > >>> +{ > >>> + struct its_cmd_desc desc; > >>> + > >>> + desc.its_inv_cmd.dev_id = dev->device_id; > >>> + desc.its_inv_cmd.event_id = event_id; > >>> + desc.its_inv_cmd.col = col; > >>> + > >>> + its_send_single_command(dev->its, its_build_inv_cmd, &desc); > >>> +} > >> [...] > >>> +typedef struct __packed { > >>> + u64 cmd:8; > >>> + u64 res1:24; > >>> + u64 devid:32; > >>> + u64 event:32; > >>> + u64 res2:32; > >>> + u64 res3:64; > >>> + u64 res4:64; > >>> +}inv_cmd_t; > >> > >> (I've trimmed this to just the INV command, but it's the same for all of > >> them) > >> > >> I suppose this is a mix of the way the Linux code was structured and my > >> request to use a struct/union to encode these things, but I'm afraid > >> this is not how I intended to suggest things be done. > >> > >> What I expected was something analogous to the hsr or lpae_t types, e.g. > >> a single: > >> union its_cmd { > >> uint64_t bits[N]; > >> > >> struct { > >> uint8_t cmd; > >> uint8_t pad[...]; > >> } hdr; > >> > >> struct { > >> uint8_t cmd; > >> uint8_t res1[3]; > >> uint32_t devid; > >> uint32_t event; > >> uint64_t res2[2]; > >> } inv; > >> }; > > > > Commands like MAPD has 40 bit ITT, 5 bit Size and 1 bit valid bit. So we have > to still manage with bit fields to manage with ease. So we have to use > mix of bit fields > and normal types (uint8_t & uint32_t etc.,) That's fine. Just use normal types for fields are sized that way and bitfields for the rest. e.g. for mapd: struct { uint8_t cmd; uint8_t res1[3]; uint32_t devid; uint64_t size:5; uint64_t res2:59; ... }; Looking at ITT_addr you might consider including the res0 bits in bits 0..7 of that double word in the itt_addr, allowing you to avoid a shift on use of that field (this assumes that the reason for those res0 bits is the alignment constraints on itt_addr). Ian.