| Message ID | 1379335841-13391-2-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 16 September 2013 13:50, <alex.bennee@linaro.org> wrote: > From: Alex Bennée <alex@bennee.com> > > Commit 9b8c69243 broke the ability to boot the kernel as the value > returned by unassigned_mem_read returned non-zero and left the kernel > looping forever waiting for it to change (see integrator_led_set in > the kernel code). This generally looks OK, but I have a few nits. > Relying on a varying implementation detail is incorrect anyway so this > introduces a memory region to emulate the debug/led region on the > integrator board. It is currently a basic stub as I have no idea what the > behaviour of this region should be so for now it simply returns 0's as > the old unassigned_mem_read did. It looks like you need to update the commit message -- you looked at the board docs, so we do know the correct behaviour. > +++ b/hw/misc/arm_intdbg.c > @@ -0,0 +1,90 @@ > +/* > + * LED, Switch and Debug control registers for ARM Integrator Boards > + * > + * This currently is a stub for this functionality written with > + * reference to what the Linux kernel looks at. Previously we relied > + * on the behaviour of unassigned_mem_read() in the core. This sounds like the code was written based on the kernel, not on the board docs, which is wrong, so I think it could use rephrasing. There's no need to mention previous behaviour either IMHO -- people who care can look at the commit history. > + * > + * The real h/w is described at: > + * http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html > + * > + * Written by Alex Bennée Assuming you wrote this with your Linaro hat on, the comment should have a * Copyright (c) 2013 Linaro Limited in it above your 'Written by' line. > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > +static void dbg_control_write(void *opaque, hwaddr offset, > + uint64_t value, unsigned size) > +{ > + switch (offset >> 2) { > + case 1: /* ALPHA */ > + case 2: /* LEDS */ > + case 3: /* SWITCHES */ > + /* Nothing interesting implemented yet. */ > + qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size); > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset); This looks like an overlength line : checkpatch.pl should warn if so. (I think there are others too.) thanks -- PMM
peter.maydell@linaro.org writes: > On 16 September 2013 13:50, <alex.bennee@linaro.org> wrote: >> From: Alex Bennée <alex@bennee.com> >> >> Commit 9b8c69243 broke the ability to boot the kernel as the value >> returned by unassigned_mem_read returned non-zero and left the kernel >> looping forever waiting for it to change (see integrator_led_set in >> the kernel code). > > This generally looks OK, but I have a few nits. OK >> Relying on a varying implementation detail is incorrect anyway so this >> introduces a memory region to emulate the debug/led region on the >> integrator board. It is currently a basic stub as I have no idea what the >> behaviour of this region should be so for now it simply returns 0's as >> the old unassigned_mem_read did. > > It looks like you need to update the commit message -- you > looked at the board docs, so we do know the correct behaviour. True, I shall update. >> +++ b/hw/misc/arm_intdbg.c >> @@ -0,0 +1,90 @@ >> +/* >> + * LED, Switch and Debug control registers for ARM Integrator Boards >> + * >> + * This currently is a stub for this functionality written with >> + * reference to what the Linux kernel looks at. Previously we relied >> + * on the behaviour of unassigned_mem_read() in the core. > > This sounds like the code was written based on the kernel, not > on the board docs, which is wrong, so I think it could use > rephrasing. There's no need to mention previous behaviour either > IMHO -- people who care can look at the commit history. Sure. I've got a bunch of other review comments to address so I can clean this up. Unfortunately I've been so busy with other stuff I haven't had a chance to go through them yet. As it happens it looks like the integrator image is now running again on the current HEAD which has made this a little less urgent. However it does raise an interesting question as to why! >> + * >> + * The real h/w is described at: >> + * http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html >> + * >> + * Written by Alex Bennée > > Assuming you wrote this with your Linaro hat on, the comment > should have a > * Copyright (c) 2013 Linaro Limited > > in it above your 'Written by' line. Yeah, I was holding off that until I was officially on the clock although as noticed I have been taking advantage of the email facilities as a handy place to have the mailing list go through. <snip> >> + /* Nothing interesting implemented yet. */ >> + qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size); >> + break; >> + default: >> + qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset); > > This looks like an overlength line : checkpatch.pl should > warn if so. (I think there are others too.) OK thanks.
On 15 October 2013 15:25, Alex Bennée <alex.bennee@linaro.org> wrote: > peter.maydell@linaro.org writes: >> This sounds like the code was written based on the kernel, not >> on the board docs, which is wrong, so I think it could use >> rephrasing. There's no need to mention previous behaviour either >> IMHO -- people who care can look at the commit history. > > Sure. I've got a bunch of other review comments to address so I can > clean this up. Unfortunately I've been so busy with other stuff I > haven't had a chance to go through them yet. > > As it happens it looks like the integrator image is now running again on > the current HEAD which has made this a little less urgent. However it > does raise an interesting question as to why! Commits 3bb28b72 and 68a7439a1 reverted the changes to the behaviour of reads from unassigned regions (since they were causing widespread brokenness). >>> + * Written by Alex Bennée >> >> Assuming you wrote this with your Linaro hat on, the comment >> should have a >> * Copyright (c) 2013 Linaro Limited >> >> in it above your 'Written by' line. > > Yeah, I was holding off that until I was officially on the clock > although as noticed I have been taking advantage of the email facilities > as a handy place to have the mailing list go through. Well, it needs *some* copyright line, at least, for whichever entity has the rights to the code and ability to (re)license it... thanks -- PMM
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak index ac0815d..a5718d1 100644 --- a/default-configs/arm-softmmu.mak +++ b/default-configs/arm-softmmu.mak @@ -80,3 +80,4 @@ CONFIG_VERSATILE_PCI=y CONFIG_VERSATILE_I2C=y CONFIG_SDHCI=y +CONFIG_INTEGRATOR_DBG=y diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c index 2ef93ed..46dc615 100644 --- a/hw/arm/integratorcp.c +++ b/hw/arm/integratorcp.c @@ -508,6 +508,7 @@ static void integratorcp_init(QEMUMachineInitArgs *args) icp_control_init(0xcb000000); sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]); sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]); + sysbus_create_simple("integrator_dbg", 0x1a000000, 0); sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL); if (nd_table[0].used) smc91c111_init(&nd_table[0], 0xc8000000, pic[27]); diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs index 2578e29..be284f3 100644 --- a/hw/misc/Makefile.objs +++ b/hw/misc/Makefile.objs @@ -10,6 +10,7 @@ obj-$(CONFIG_VMPORT) += vmport.o # ARM devices common-obj-$(CONFIG_PL310) += arm_l2x0.o +common-obj-$(CONFIG_INTEGRATOR_DBG) += arm_intdbg.o # PKUnity SoC devices common-obj-$(CONFIG_PUV3) += puv3_pm.o diff --git a/hw/misc/arm_intdbg.c b/hw/misc/arm_intdbg.c new file mode 100644 index 0000000..b505d09 --- /dev/null +++ b/hw/misc/arm_intdbg.c @@ -0,0 +1,90 @@ +/* + * LED, Switch and Debug control registers for ARM Integrator Boards + * + * This currently is a stub for this functionality written with + * reference to what the Linux kernel looks at. Previously we relied + * on the behaviour of unassigned_mem_read() in the core. + * + * The real h/w is described at: + * http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html + * + * Written by Alex Bennée + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "hw/hw.h" +#include "hw/sysbus.h" +#include "exec/address-spaces.h" + +#define TYPE_ARM_INTDBG "integrator_dbg" +#define ARM_INTDBG(obj) \ + OBJECT_CHECK(ARMIntDbgState, (obj), TYPE_ARM_INTDBG) + +typedef struct { + SysBusDevice parent_obj; + MemoryRegion iomem; + + uint32_t alpha; + uint32_t leds; + uint32_t switches; +} ARMIntDbgState; + +static uint64_t dbg_control_read(void *opaque, hwaddr offset, + unsigned size) +{ + switch (offset >> 2) { + case 0: /* ALPHA */ + case 1: /* LEDS */ + case 2: /* SWITCHES */ + qemu_log_mask(LOG_UNIMP, "dbg_control_read: returning zero from %x:%d\n", (int)offset, size); + return 0; + default: + qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_read: Bad offset %x\n", (int)offset); + return 0; + } +} + +static void dbg_control_write(void *opaque, hwaddr offset, + uint64_t value, unsigned size) +{ + switch (offset >> 2) { + case 1: /* ALPHA */ + case 2: /* LEDS */ + case 3: /* SWITCHES */ + /* Nothing interesting implemented yet. */ + qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx to %x:%d\n", value, (int)offset, size); + break; + default: + qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to bad offset %x\n", value, (int)offset); + } +} + +static const MemoryRegionOps dbg_control_ops = { + .read = dbg_control_read, + .write = dbg_control_write, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static void dbg_control_init(Object *obj) +{ + SysBusDevice *sd = SYS_BUS_DEVICE(obj); + ARMIntDbgState *s = ARM_INTDBG(obj); + memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "dbgleds", 0x1000000); + sysbus_init_mmio(sd, &s->iomem); +} + +static const TypeInfo arm_intdbg_info = { + .name = TYPE_ARM_INTDBG, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(ARMIntDbgState), + .instance_init = dbg_control_init, +}; + +static void arm_intdbg_register_types(void) +{ + type_register_static(&arm_intdbg_info); +} + +type_init(arm_intdbg_register_types)