Re: [Qemu-devel] [PATCH] integrator/cp: add support for REFCNT register

[Peter Maydell]

On 5 November 2013 11:41, Jan Petrouš <address@hidden> wrote:
> Linux kernel from version 3.4 requires CM_REFCNT register for sched timer
> for Integrator/CP board (integrator_defconfig).
>
> See
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0138e/ch04s06s11.html
>
> Signed-off-by: Jan Petrous <address@hidden>

Hi; thanks for this patch. (I have to wonder why on earth
people are still messing with kernel functionality for such
an ancient devboard :-)).

> diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
> index c44b2a4..f419764 100644
> --- a/hw/arm/integratorcp.c
> +++ b/hw/arm/integratorcp.c
> @@ -83,8 +83,11 @@ static uint64_t integratorcm_read(void *opaque, hwaddr
> offset,
>      case 9: /* CM_INIT */
>          return s->cm_init;
>      case 10: /* CM_REFCT */

could you fix this comment to add the 'N' to CM_REFCNT while
you're changing this bit of code, please?

> -        /* ??? High frequency timer.  */
> -        hw_error("integratorcm_read: CM_REFCT");
> +       /* This register, CM_REFCNT, provides a 32-bit count value.
> + * The count increments at the fixed reference clock frequency of 24MHz
> + * and can be used as a real-time counter.
> + */

The indent here doesn't look right, I think you have
hardcoded tabs here rather than spaces. If you run your
patch through scripts/checkpatch.pl it should catch this
sort of mistake.

> +       return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 24, 1000);
>      case 12: /* CM_FLAGS */
>          return s->cm_flags;
>      case 14: /* CM_NVFLAGS */

The spec says that this register resets to zero, so I think
it would be good to implement that: put a uint32_t cm_refcnt_offset
in the IntegratorCMState struct, initialize it with the initial
value of muldiv64(etc) in integratorcm_init(), and then in
integratorcm_read() return (uint32_t)muldiv64(etc) - cm_refcnt_offset
for the register read.

(We don't actually implement a proper reset function in this device
at the moment, but putting in the offset field gives us a marker
for doing it right if/when we add a reset function later on.)

thanks
-- PMM