Re: [PATCH v4 03/17] xen/arm: ITS: Port ITS driver to Xen

[Ian Campbell <ian.campbell@citrix.com>]

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.