Hi Julien,
On 26 April 2016 at 18:49, Julien Grall <[email protected]> wrote:
> Hello Wei,
>
> On 25/04/2016 10:39, Wei Chen wrote:
>>
>> This patch adds v2m extension support in GIC-v2 driver. The GICv2 driver
>> detects the MSI frames from device tree and creates corresponding device
>> tree nodes in Domain0's DTB. It also provides one hw_ops callback to map
>
>
> NIT: s/Domain0/dom0/
>
>> v2m MMIO regions to domain0 and route v2m SPIs to domain0.
>
>
> Ditto.
>
>>
>> With this GICv2m extension support, the domain0 kernel can do GICv2m
>
>
> Ditto
>
>> frame setup and initialization.
>>
>> This patch is based on the GICv2m patch of Suravee Suthikulpanit:
>> [PATCH 2/2] xen/arm: gicv2: Adding support for GICv2m in Dom0
>> http://lists.xen.org/archives/html/xen-devel/2015-04/msg02613.html
>>
>> Signed-off-by: Wei Chen <[email protected]>
>
>
> [...]
>
>> +static int gicv2_map_hwdown_extra_mappings(struct domain *d)
>> +{
>> + const struct v2m_data *v2m_data;
>> +
>> + /* For the moment, we'll assign all v2m frames to the hardware
>> domain. */
>> + list_for_each_entry( v2m_data, &gicv2m_info, entry )
>> + {
>> + int ret;
>> + u32 spi;
>> +
>> + printk("GICv2: Mapping v2m frame to d%d: addr=0x%lx size=0x%lx
>> spi_base=%u num_spis=%u\n",
>> + d->domain_id, v2m_data->addr, v2m_data->size,
>> + v2m_data->spi_start, v2m_data->nr_spis);
>> +
>> + ret = map_mmio_regions(d, paddr_to_pfn(v2m_data->addr),
>> + DIV_ROUND_UP(v2m_data->size, PAGE_SIZE),
>> + paddr_to_pfn(v2m_data->addr));
>> + if ( ret )
>> + {
>> + printk(XENLOG_ERR "GICv2: Map v2m frame to s%d failed.\n",
>
>
> s/s%d/d%d/
>
>> + d->domain_id);
>> + return ret;
>> + }
>> +
>> + /*
>> + * Map all SPIs that are allocated to MSIs for the frame to the
>> + * domain.
>> + */
>> + for ( spi = v2m_data->spi_start;
>> + spi < (v2m_data->spi_start + v2m_data->nr_spis); spi++ )
>> + {
>> + /*
>> + * MSIs are always edge-triggered. Configure the associated
>> SPIs
>> + * to be edge-rising.
>
>
> How did you find that SPIs should be configured edge-rising?
Before route_irq_to_guest, the SPI must be configured. I found Linux
v2m driver set
the SPI type to edge-rising, so I set edge-rising as v2m SPI default
type here too.
>
>
>> + */
>> + ret = irq_set_spi_type(spi, IRQ_TYPE_EDGE_RISING);
>> + if ( ret )
>> + {
>> + printk(XENLOG_ERR
>> + "GICv2: Failed to set v2m MSI SPI[%d] type.\n",
>> spi);
>> + return ret;
>> + }
>> +
>> + /* Route a SPI that is allocated to MSI to the domain. */
>> + ret = route_irq_to_guest(d, spi, spi, "v2m");
>> + if ( ret )
>> + {
>> + printk(XENLOG_ERR
>> + "GICv2: Failed to route v2m MSI SPI[%d] to
>> Dom%d.\n",
>> + spi, d->domain_id);
>> + return ret;
>> + }
>> +
>> + /* Reserve a SPI that is allocated to MSI for the domain. */
>> + if ( !vgic_reserve_virq(d, spi) )
>> + {
>> + printk(XENLOG_ERR
>> + "GICv2: Failed to reserve v2m MSI SPI[%d] for
>> Dom%d.\n",
>> + spi, d->domain_id);
>> + return -EINVAL;
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Set up gic v2m DT sub-node.
>> + * Please refer to the binding document:
>> + *
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
>> + */
>> +static int gicv2m_make_dt_node(const struct domain *d,
>> + const struct dt_device_node *gic,
>> + void *fdt)
>
>
> The indentation is still wrong here.
>
> The start of the second and third lines should be aligned to "const..." on
> the first line. I.e
>
> foo(const
> ____const
> ____void
>
> Where _ is the empty space.
>
> [...]
>
Thanks for your detailed example!
>> +static void gicv2_add_v2m_frame_to_list(paddr_t addr, paddr_t size,
>> + u32 spi_start, u32 nr_spis,
>> + const struct dt_device_node *v2m)
>> +{
>> + struct v2m_data *v2m_data;
>> +
>> + v2m_data = xzalloc_bytes(sizeof(struct v2m_data));
>> + if ( !v2m_data )
>> + panic("GICv2: Cannot allocate memory for v2m frame");
>> +
>> + /* Initialize a new entry to record new frame infomation. */
>
>
> s/infomation/information/
>
>
>> + INIT_LIST_HEAD(&v2m_data->entry);
>> + v2m_data->addr = addr;
>> + v2m_data->size = size;
>> + v2m_data->spi_start = spi_start;
>> + v2m_data->nr_spis = nr_spis;
>> + v2m_data->dt_node = v2m;
>> +
>> + printk("GICv2m extension register frame:\n"
>> + " gic_v2m_addr=%"PRIpaddr"\n"
>> + " gic_v2m_size=%"PRIpaddr"\n"
>> + " gic_v2m_spi_base=%u\n"
>> + " gic_v2m_num_spis=%u\n",
>> + v2m_data->addr, v2m_data->size,
>> + v2m_data->spi_start, v2m_data->nr_spis);
>> +
>> + list_add_tail(&v2m_data->entry, &gicv2m_info);
>> +}
>> +
>> +static void gicv2_extension_dt_init(const struct dt_device_node *node)
>> +{
>> + int res;
>> + u32 spi_start, nr_spis;
>> + paddr_t addr, size;
>> + const struct dt_device_node *v2m = NULL;
>> +
>> + /*
>> + * Check whether this GIC implements the v2m extension. If so,
>> + * add v2m register frames to gicv2m_info.
>> + */
>> + dt_for_each_child_node(node, v2m)
>> + {
>> + if ( !dt_device_is_compatible(v2m, "arm,gic-v2m-frame") )
>> + continue;
>> +
>> + /*
>> + * Check whether DT uses msi-base-spi and msi-num-spis properties
>> to
>> + * override the hardware setting.
>> + */
>> + if ( !dt_property_read_u32(v2m, "arm,msi-base-spi", &spi_start)
>> ||
>> + !dt_property_read_u32(v2m, "arm,msi-num-spis", &nr_spis) )
>> + {
>> + u32 msi_typer;
>> + void __iomem *base;
>> +
>> + /*
>> + * DT does not override the hardware setting, so we have to
>> read
>> + * base_spi and num_spis from hardware registers to reserve
>> irqs.
>> + */
>
>
> The indentation of the comments if wrong.
>
> It should be:
>
> /*
> * FOo
> * Bar
> */
>
>> + res = dt_device_get_address(v2m, 0, &addr, &size);
>> + if ( res )
>> + panic("GICv2: Cannot find a valid v2m frame address");
>> +
>> + base = ioremap_nocache(addr, size);
>> + if ( !base )
>> + panic("GICv2: Cannot remap v2m register frame");
>> +
>> + msi_typer = readl_relaxed(base + V2M_MSI_TYPER);
>> + spi_start = V2M_MSI_TYPER_BASE_SPI(msi_typer);
>> + nr_spis = V2M_MSI_TYPER_NUM_SPI(msi_typer);
>
>
> The ACPI code will likely need to read those registers. So this could go in
> the new function you have added.
>
> You could pass pointers to your function to know whether the values are
> overridden by the firmware.
>
I will refine the new function.
>> +
>> + iounmap(base);
>> + }
>> +
>> + /* Add this v2m frame information to list. */
>> + gicv2_add_v2m_frame_to_list(addr, size, spi_start, nr_spis, v2m);
>> + }
>> +}
>> +
>
>
> Regards,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
