From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v4 03/17] xen/arm: ITS: Port ITS driver to
	Xen
Date: Fri, 10 Jul 2015 14:01:58 +0100
Message-ID: <1436533318.10074.10.camel@citrix.com>
References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com>
	<1436514172-3263-4-git-send-email-vijay.kilari@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1436514172-3263-4-git-send-email-vijay.kilari@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@gmail.com
Cc: stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, tim@xen.org, xen-devel@lists.xen.org, julien.grall@citrix.com, stefano.stabellini@citrix.com, manish.jaggi@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org

On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:

> +/*
> + * The ITS structure - contains most of the infrastructure, with the
> + * msi_controller, the command queue, the collections, and the list of
> + * devices writing to it.
> + */
> +struct its_node {
> +    spinlock_t              lock;
> +    struct list_head        entry;
> +    void __iomem            *base;
> +    unsigned long           phys_base;
> +    unsigned long           phys_size;

These two could be paddr_t I think.

> +#ifdef DEBUG_GIC_ITS
> +void dump_cmd(its_cmd_block *cmd)
> +{
> +    printk("ITS: Phys_cmd CMD[0] = 0x%lx CMD[1] = 0x%lx CMD[2] = 0x%lx CMD[3] = 0x%lx\n",
> +           cmd->bits[0], cmd->bits[1], cmd->bits[2], cmd->bits[3]);
> +}

Please can you include a #else with a dummy version of this function and
therefore avoid the #ifdef's at the callsite.

I think this also wants a XENLOG_DEBUG prefix, or better to use
its_debug.

> +
> +static void its_send_single_command(struct its_node *its,
> +                                    its_cmd_block *src,
> +                                    struct its_collection *sync_col)
> +{
> +    its_cmd_block *cmd, *sync_cmd, *next_cmd;
> +    unsigned long flags;
> +
> +    BUILD_BUG_ON(sizeof(its_cmd_block) != 32);
> +
> +    spin_lock_irqsave(&its->lock, flags);
> +
> +    cmd = its_allocate_entry(its);
> +    if ( !cmd )
> +    {
> +        its_err("ITS can't allocate, dropping command\n");
> +        spin_unlock_irqrestore(&its->lock, flags);
> +        return;
> +    }
> +
> +    memcpy(cmd, src, sizeof(its_cmd_block));
> +#ifdef DEBUG_GIC_ITS
> +    dump_cmd(cmd);
> +#endif
> +    its_flush_cmd(its, cmd);

You could probably put the dump in its_flush_cmd, which might be a bit
less prone to forgetting it sometimes.

> +/* ITS command structure */
> +typedef union {
> +    u64 bits[4];
> +    struct __packed {
> +        uint8_t cmd;
> +        uint8_t pad[7];
> +    } 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 */

Is this intended as a TODO?

> [...]
> +} its_cmd_block;

Much preferred, thanks.

If you do at least the #else case for dump_cmd then I would ack this
patch.

Ian.