Re: [Xen-devel] [RFC PATCH v3 05/18] xen/arm: ITS: Port ITS driver to xen

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien,

On Mon, Jun 22, 2015 at 10:46 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi,

>> +    cmd->mapd.cmd = GITS_CMD_MAPD;
>> +    cmd->mapd.devid = desc->its_mapd_cmd.dev->device_id;
>> +    cmd->mapd.size = size - 1;
>> +    cmd->mapd.itt = itt_addr >> 8;
>
> I think the code is more difficult to read without the helpers. You
> opencode every trick in all the builder rather than in a single place.

    ITS commands does not consider all the bits of the values esp. when
encoding addresses. So we need to shift and write it.
Helpers might be useful, but usage is hardly once or twice in the code.

>> +static void its_cpu_init_collection(void)
>> +{
>> +    struct its_node *its;
>> +    int cpu;
>> +
>> +    spin_lock(&its_lock);
>> +    cpu = smp_processor_id();
>> +
>> +    list_for_each_entry(its, &its_nodes, entry)
>> +    {
>> +        u64 target;
>> +        /*
>> +         * We now have to bind each collection to its target
>> +         * redistributor.
>> +         */
>> +        if ( readq_relaxed(its->base + GITS_TYPER) & GITS_TYPER_PTA )
>> +        {
>> +            /*
>> +             * This ITS wants the physical address of the
>> +             * redistributor.
>> +             */
>> +            target = gic_data_rdist().phys_base;
>> +        }
>> +        else
>> +        {
>> +            /*
>> +             * This ITS wants a linear CPU number.
>> +             */
>> +            target = readq_relaxed(gic_data_rdist_rd_base() + GICR_TYPER);
>> +            target = GICR_TYPER_CPU_NUMBER(target);
>
> Why did you drop the << 16?
>
> Although, as said earlier, given the usage of target_address you could
> do shift >> directly in this function rather than on multiple__ place.

Yes, we can shift at the setup. But we lose actual value of target_address.
Also we shift because ITS command skips lower 16 bits

>> +int its_init(struct rdist_prop *rdists)
>> +{
>> +    struct dt_device_node *np = NULL;
>> +
>> +    static const struct dt_device_match its_device_ids[] __initconst =
>
> If the structure is __initconst, the function should be __init too.
>
>> +    {
>> +        DT_MATCH_GIC_ITS,
>> +        { /* sentinel */ },
>> +    };
>> +
>> +    for (np = dt_find_matching_node(NULL, its_device_ids); np;
>> +             np = dt_find_matching_node(np, its_device_ids))
>> +        its_probe(np);
>> +
>> +    if ( list_empty(&its_nodes) )
>> +    {
>> +        its_warn("ITS: No ITS available, not enabling LPIs\n");
>> +        return -ENXIO;
>> +    }
>> +
>> +    gic_rdists = rdists;
>> +    its_alloc_lpi_tables();
>> +
>> +    BUILD_BUG_ON(sizeof(its_cmd_block) != 32);
>
> Why this build bug on here? Shouldn't it be part of the builder code?

 Where is builder code in xen?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel