Re: [Qemu-devel] [PATCH v5 1/2] pl330: Initial version

[Igor Mitsyanko <i.mitsyanko@samsung.com>]

Good day, Peter)

On 10/29/2012 10:35 AM, Peter Crosthwaite wrote:
> Device model for Primecell PL330 dma controller.
>
> +
> +static Property pl330_properties[] = {
> +    /* CR0 */
> +    DEFINE_PROP_UINT8("num_chnls", PL330, num_chnls, 8),
> +    DEFINE_PROP_UINT8("num_periph_req", PL330, num_periph_req, 8),
> +    DEFINE_PROP_UINT8("num_events", PL330, num_events, 8),
> +    DEFINE_PROP_UINT8("mgr_ns_at_rst", PL330, mgr_ns_at_rst, 0),
> +    /* CR1 */
> +    DEFINE_PROP_UINT8("i-cache_len", PL330, i_cache_len, 4),
> +    DEFINE_PROP_UINT8("num_i-cache_lines", PL330, num_i_cache_lines, 8),
> +
> +    DEFINE_PROP_UINT8("mgr_ns_at_rst", PL330, mgr_ns_at_rst, 0),

That's a duplicate, you had the same property three rows before.

> +
> +    /* CR2-4 */
> +    DEFINE_PROP_UINT32("boot_addr", PL330, cfg[CFG_BOOT_ADDR], 0),
> +    DEFINE_PROP_UINT32("INS", PL330, cfg[CFG_INS], 0),
> +    DEFINE_PROP_UINT32("PNS", PL330, cfg[CFG_PNS], 0),
> +    /* CRD */
> +    DEFINE_PROP_UINT8("data_width", PL330, data_width, 0),

You do not decode this value in pl330_init() like you do for, for 
example, i_cache_len property. I think default value
here should be 32, which corresponds to 0b010 in LSB of CRD register. 
And you should also check for reserved
values of this property and, perhaps, hw_error() on them.

> +    DEFINE_PROP_UINT8("wr_cap", PL330, wr_cap, 0),
> +    DEFINE_PROP_UINT8("wr_q_dep", PL330, wr_q_dep, 0),
> +    DEFINE_PROP_UINT8("rd_cap", PL330, rd_cap, 0),
> +    DEFINE_PROP_UINT8("rd_q_dep", PL330, rd_q_dep, 0),
> +    DEFINE_PROP_UINT16("data_buffer_dep", PL330, data_buffer_dep, 0),
> +
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pl330_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = pl330_init;
> +    dc->reset = pl330_reset;
> +    dc->props = pl330_properties;
> +    dc->vmsd = &vmstate_pl330;
> +}
> +
> +static const TypeInfo pl330_type_info = {
> +    .name           = "pl330",
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(PL330),
> +    .class_init      = pl330_class_init,
> +};
> +
> +static void pl330_register_types(void)
> +{
> +    type_register_static(&pl330_type_info);
> +}
> +
> +type_init(pl330_register_types)

-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com