Re: [Qemu-devel] [PATCH 7/9] add L2x0/PL310 cache controller device

[Peter Maydell]

On 20 December 2011 19:13, Mark Langsdorf <address@hidden> wrote:
> From: Rob Herring <address@hidden>
>
> This is just a dummy device for ARM L2 cache controllers.
>
> Signed-off-by: Rob Herring <address@hidden>
> Signed-off-by: Mark Langsdorf <address@hidden>

This is missing save/load support.

> ---
>  Makefile.target |    2 +-
>  hw/arm_l2x0.c   |  109
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 110 insertions(+), 1 deletions(-)
>  create mode 100644 hw/arm_l2x0.c
>
> diff --git a/Makefile.target b/Makefile.target
> index 3261383..e4132d6 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -335,7 +335,7 @@ endif
>  obj-arm-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o
>  obj-arm-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o 
> pl190.o
>  obj-arm-y += versatile_pci.o
> -obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
> +obj-arm-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o 
> arm_l2x0.o

This puts this line beyond 80 chars, put the new .o on a different line
(on the next one after arm_mptimer.o is fine).

>  obj-arm-y += arm_mptimer.o
>  obj-arm-y += armv7m.o armv7m_nvic.o stellaris.o pl022.o stellaris_enet.o
>  obj-arm-y += pl061.o
> diff --git a/hw/arm_l2x0.c b/hw/arm_l2x0.c
> new file mode 100644
> index 0000000..ce13311
> --- /dev/null
> +++ b/hw/arm_l2x0.c
> @@ -0,0 +1,109 @@
> +/*
> + * ARM dummy L210, L220, PL310 cache controller.
> + *
> + * Copyright (c) 2006-2007 CodeSourcery.
> + * Copyright (c) 2010-2012 Calxeda
> + * Written by Rob Herring
> + *
> + * This code is licenced under the GPL.

If this file contains code which is Copyright CodeSourcery there
should probably be a Signed-off-by: from somebody at CodeSourcery.

I think it's nicer to explicitly say which GPL versions you mean
("v2 or later" being the usual).

> + */
> +
> +#include "sysbus.h"
> +
> +typedef struct l2x0_state {
> +    SysBusDevice busdev;
> +    MemoryRegion iomem;
> +    uint32_t ctrl;
> +    uint32_t aux_ctrl;
> +    uint32_t data_ctrl;
> +    uint32_t tag_ctrl;
> +    uint32_t filter_start;
> +    uint32_t filter_end;
> +} l2x0_state;
> +
> +static uint64_t l2x0_priv_read(void *opaque, target_phys_addr_t offset,
> unsigned size)

This is an overly long line, and you've sent the email through something
that wraps long lines, which means the patch doesn't apply.

> +{
> +    l2x0_state *s = (l2x0_state *)opaque;
> +    offset &= 0xfff;
> +    if (offset == 0)

scripts/checkpatch.pl would have warned you about all the missing braces
here. switch() statements are the more usual style in qemu for
register read/write functions, incidentally.

> +        return 0x410000c6;

So this is an L2C-310 r3p1. Any particular reason for picking that?
(r3p2 is the most recent one with a manual on infocenter).

(Also it would be nice to say this rather than requiring the reader
to go digging in manuals.)

> +    else if (offset == 0x4)
> +        return 0x19080800;
> +    else if (offset == 0x100)
> +        return s->ctrl;
> +    else if (offset == 0x104)
> +        return s->aux_ctrl;
> +    else if (offset == 0x108)
> +        return s->tag_ctrl;
> +    else if (offset == 0x10C)
> +        return s->data_ctrl;
> +    else if (offset >= 0x730 && offset < 0x800)
> +        return 0; /* cache ops complete */
> +    else if (offset == 0xC00)
> +        return s->filter_start;
> +    else if (offset == 0xC04)
> +        return s->filter_end;
> +    else if (offset == 0xF40)
> +        return 0;
> +    else if (offset == 0xF60)
> +        return 0;
> +    else if (offset == 0xF80)
> +        return 0;
> +
> +    hw_error("l2x0_priv_read: Bad offset %x\n", (int)offset);

Don't hw_error() on things a guest can provoke.

> +    return 0;
> +}
> +
> +static void l2x0_priv_write(void *opaque, target_phys_addr_t offset,
> uint64_t value, unsigned size)
> +{
> +    l2x0_state *s = (l2x0_state *)opaque;
> +    offset &= 0xfff;
> +    if (offset == 0x100)
> +        s->ctrl = value & 1;
> +    else if (offset == 0x104)
> +        s->aux_ctrl = value;
> +    else if (offset == 0x108)
> +        s->tag_ctrl = value;
> +    else if (offset == 0x10C)
> +        s->data_ctrl = value;
> +    else if (offset >= 0x730 && offset < 0x800)
> +        /* ignore */
> +        return;
> +    else if (offset == 0xC00)
> +        s->filter_start = value;
> +    else if (offset == 0xC04)
> +        s->filter_end = value;
> +    else if (offset == 0xF40)
> +        return;
> +    else if (offset == 0xF60)
> +        return;
> +    else if (offset == 0xF80)
> +        return;
> +    else
> +        hw_error("l2x0_priv_write: Bad offset %x\n", (int)offset);
> +}
> +
> +static const MemoryRegionOps l2x0_mem_ops = {
> +    .read = l2x0_priv_read,
> +    .write = l2x0_priv_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> + };
> +
> +static int l2x0_priv_init(SysBusDevice *dev)
> +{
> +    l2x0_state *s = FROM_SYSBUS(l2x0_state, dev);
> +
> +    s->aux_ctrl = 0x00020000;

This initialisation should be in a reset function.

> +
> +    memory_region_init_io(&s->iomem, &l2x0_mem_ops, s, "l2x0_cc", 0x1000);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    return 0;
> +}
> +
> +static void l2x0_register_device(void)
> +{
> +    sysbus_register_dev("l2x0_cc", sizeof(l2x0_state), l2x0_priv_init);
> +}
> +
> +device_init(l2x0_register_device)
> +
> --
> 1.7.5.4

-- PMM