From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@citrix.com>
Subject: Re: [PATCH v7 05/28] xen/arm: ITS: Port ITS driver to
	Xen
Date: Tue, 22 Sep 2015 11:01:10 +0100
Message-ID: <560126E6.3060402@citrix.com>
References: <1442581755-2668-1-git-send-email-vijay.kilari@gmail.com>
	<1442581755-2668-6-git-send-email-vijay.kilari@gmail.com>
	<55FFE8AD.9060208@citrix.com>
	<CALicx6uQ52qRn5TP-zMztaX58EiuRjvzhme+sVSA9ahHq=eRqA@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CALicx6uQ52qRn5TP-zMztaX58EiuRjvzhme+sVSA9ahHq=eRqA@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Vijay Kilari <vijay.kilari@gmail.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Prasun Kapoor <Prasun.Kapoor@caviumnetworks.com>, Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>, Tim Deegan <tim@xen.org>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Stefano Stabellini <stefano.stabellini@citrix.com>, manish.jaggi@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org


Hi Vijay,

On 22/09/2015 10:55, Vijay Kilari wrote:
> On Mon, Sep 21, 2015 at 4:53 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> Hi Vijay,
>>
>> The only things I haven't check on this patch was the ITS command structure.
> [...]
>> Why a padding of only 56 bits? Shouldn't it be 248 bits (i.e 31 * 8
>> bits) to fit all the command?
>>
>>> +    } hdr;
>>> +    struct __packed {
>>> +        u8 cmd;
>>> +        u8 res1[3];
>>> +        u32 devid;
>>> +        u64 size:5;
>>> +        u64 res2:59;
>>> +        /* XXX: Though itt is 40 bit. Keep it 48 to avoid shift */
>>> +        u64 res3:8;
>>
>> It's very confusing for the reviewer to see a mix of usage (u8 res1[n],
>> u64 res3:8) within the same structure.
>>
>> The later (i.e u64 field:n) is the best to use because we can match
>> quickly the size with the spec.
>>
>
> Do you mean to convert all field type as 64 as below?

Yes.

>
>      struct __packed {
>          u64 cmd:8;
>          u64 res:24;
>          u64 devid:32;
>          u64 size:5;
>          u64 res2:59;
>          /* XXX: Though itt is 40 bit. Keep it 48 to avoid shift */

BTW, this comment is not clear. Please explain in the comment why you 
want to use 48 bit. I.e

"XXX: The ITT holds the upper bits of the address [47-8]. Include res3 
to avoid shifting?"

>          u64 res3:8;
>          u64 itt:40;
>          u64 res4:15;
>          u64 valid:1;
>          u64 res5;
>      } mapd_cmd;
>

Regards,

-- 
Julien Grall