Same subject-prefix as commented by Andreas. Should be "target-arm".
On Wed, Jan 22, 2014 at 9:40 AM, Alistair Francis
<alistair.fran...@xilinx.com> wrote:
> This patch implements the ARM PMCCNTR register including
> the disable and reset components of the PMCR register.
>
> Signed-off-by: Alistair Francis <alistair.fran...@xilinx.com>
> ---
> This patch assumes that non-invasive debugging is not permitted
> when determining if the counter is disabled
> V3: Fixed up incorrect reset, disable and enable handling that
> was submitted in V2. The patch should now also correctly handle
> on the fly changing of the clock scaling factor.
> V2: Incorporated the comments that Peter Maydell and Peter
> Crosthwaite had. Now the implementation only requires one
> CPU state
>
> target-arm/cpu.h | 3 ++
> target-arm/helper.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 198b6b8..2fdab58 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -215,6 +215,9 @@ typedef struct CPUARMState {
> uint32_t c15_diagnostic; /* diagnostic register */
> uint32_t c15_power_diagnostic;
> uint32_t c15_power_control; /* power control */
> + /* If the counter is enabled, this stores the last time the counter
> + * was reset. Otherwise it stores the counter value */
> + uint32_t c15_ccnt;
> } cp15;
>
> /* System registers (AArch64) */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index c708f15..ad87136 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -13,6 +13,12 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t
> address,
> target_ulong *page_size);
> #endif
>
> +/* Definitions for the PMCCNTR and PMCR registers */
> +#define PMCRDP 0x20
> +#define PMCRD 0x8
> +#define PMCRC 0x4
> +#define PMCRE 0x1
> +
> static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> {
> int nregs;
> @@ -502,12 +508,44 @@ static int pmreg_read(CPUARMState *env, const
> ARMCPRegInfo *ri,
> static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t value)
> {
> + uint32_t temp_ticks, diff;
> +
> if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
> return EXCP_UDEF;
> }
> +
> + temp_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
> + get_ticks_per_sec())/1000000000) >> 8) &
> + 0xFFFFFFFF);
> +
> + /* This assumes that non-invasive debugging is not permitted */
> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
> + env->cp15.c9_pmcr & PMCRE) {
> + /* If the counter is enabled */
> + env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
Should this be frequency scalar aware? I think its calculating and
saving the current timer value but assuming the X64 prescale is
disabled. But since the intent is to save the current timer value to
c15_ccnt, is it as simple as just calling pmccntr_read() here to get
the desired to-be-saved value?
> + }
> +
> + if (value & PMCRC) {
> + /* The counter has been reset */
> + env->cp15.c15_ccnt = 0;
> + }
> +
> /* only the DP, X, D and E bits are writable */
> env->cp15.c9_pmcr &= ~0x39;
> env->cp15.c9_pmcr |= (value & 0x39);
> +
> + /* This assumes that non-invasive debugging is not permitted */
> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
> + env->cp15.c9_pmcr & PMCRE) {
> + if (env->cp15.c9_pmcr & PMCRDP) {
> + /* Increment once every 64 processor clock cycles */
> + diff = (temp_ticks/64) - env->cp15.c15_ccnt;
I think you can simplify with just
diff = (temp_ticks/64);
and ...
> + } else {
> + diff = temp_ticks - env->cp15.c15_ccnt;
> + }
> + env->cp15.c15_ccnt += diff;
... env->cp15.c15_ccnt = diff;
Although it's probably even simpler to just drop this and assign
env->cp15.c15_ccnt in the two if-else branches directly.
> + }
> +
> return 0;
> }
>
> @@ -584,6 +622,33 @@ static int vbar_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> return 0;
> }
>
> +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t *value)
> +{
> + uint32_t total_ticks;
> +
> + /* This assumes that non-invasive debugging is not permitted */
> + if (env->cp15.c9_pmcr & PMCRDP ||
> + !(env->cp15.c9_pmcr & PMCRE)) {
> + /* Counter is disabled, do not change value */
> + *value = env->cp15.c15_ccnt;
> + return 0;
> + }
> +
> + total_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
> + get_ticks_per_sec())/1000000000) >> 8) &
Theres an un-needed parenthesis of multiplication before division.
Whats the shift by 8 for?
> + 0xFFFFFFFF);
The mask-to-32bit is un-needed I think.
> +
> + if (env->cp15.c9_pmcr & PMCRDP) {
> + /* Increment once every 64 processor clock cycles */
> + *value = (total_ticks/64) - env->cp15.c15_ccnt;
You can save on a common sub-express by just
total_ticks /= 64;
Then ...
> + } else {
> + *value = total_ticks - env->cp15.c15_ccnt;
.. you can do this unconditionally.
> + }
> +
> + return 0;
> +}
> +
> static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t *value)
> {
> @@ -644,9 +709,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> */
> { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
> .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> - /* Unimplemented, RAZ/WI. XXX PMUSERENR */
> { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> + .access = PL1_RW, .readfn = pmccntr_read,
> + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
You don't implement write, so I'm not sure exactly what you would do
to implement readfn with UNIMP writefn. But this as-is will allow the
guest to overwrite the "diff" value with an absolute, which would
cause very strange behaviour.
Regards,
Peter
> + .resetvalue = 0, .type = ARM_CP_IO },
> { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2
> = 1,
> .access = PL0_RW,
> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
> --
> 1.7.1
>
>
