[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device m
From: |
Paul Brook |
Subject: |
Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model |
Date: |
Mon, 27 Feb 2012 15:45:08 +0000 |
User-agent: |
KMail/1.13.7 (Linux/3.2.0-1-amd64; KDE/4.6.5; x86_64; ; ) |
> On Tue, Feb 21, 2012 at 11:04 PM, Paul Brook <address@hidden> wrote:
> >> > +static inline int64_t is_between(int64_t x, int64_t a, int64_t b)
> >> > +{
> >> > + if (a < b) {
> >> > + return x > a && x <= b;
> >> > + }
> >> > + return x < a && x >= b;
> >> > +}
> >>
> >> This looks slightly odd -- should the boundary condition for whether
> >> a value equal to the max/min really change depending on :whether a
> >> or b is greater?
>
> The function determines whether x is in-between a and b exclusive of
> a, inclusive of b, so it is consistent with itself in that regard.
>
> > This is a ugly hack. Instead of figuring out whether we have a count-up
> > or count-down timer the code checks for both, and have the "in_between"
> > function magically DTRT. I haven't followed the paths through in enough
> > detail to figure out whether it gets all the corner cases right.
>
> Is it really a "hack"?? For count up b will always be greater than a,
> and for count down the reverse. I suppose I could assert these
> conditions at the call site for peace of mind? The invocation from
> cadence_timer_run doesn't care whether it is count up of count down,
> it really does just only care if the match value is in-between the
> current timer value and the next timer value, which is exactly what
> this function determines.
When you explain it like this, it makes a more sense. But this isn't
immediately obvious from the code. It took me at least a couple of readings
to figure out what was going on. This is exactly the sort of thing that should
be described in comments. A function with a very generic name is used in a
way that has fairly subtle implications. There's a good chance someone[1]
will come along in a few months/years, reuse this function and "fix" the
wierdness at the same time.
Annother non-obvious detail is the way you handle overflow. Specifically you
check a range both plus and minus the wrap value before wrapping the final
count. This is certainly confusing/surprising when you first encounter it.
Very large steps result in overlapping ranges, which triggers [in this case
harmless] warning bells.
Thinking about that, I realised why I don't like the following line:
> + s->reg_value = (uint32_t)((x + interval) % interval);
This assumes x > -interval, which is not always true.
Paul
[1] "someone" includes me. After I've forgotten this obscure detail.
- [Qemu-devel] [PATCH v6 0/4] Zynq-7000 EPP platform model, Peter A. G. Crosthwaite, 2012/02/19
- [Qemu-devel] [PATCH v6 1/4] cadence_uart: initial version of device model, Peter A. G. Crosthwaite, 2012/02/19
- [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter A. G. Crosthwaite, 2012/02/19
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Maydell, 2012/02/20
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Paul Brook, 2012/02/21
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/22
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model,
Paul Brook <=
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/27
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/27
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Paul Brook, 2012/02/28
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/28
- Re: [Qemu-devel] [PATCH v6 2/4] cadence_ttc: initial version of device model, Peter Crosthwaite, 2012/02/22
[Qemu-devel] [PATCH v6 3/4] cadence_gem: initial version of device model, Peter A. G. Crosthwaite, 2012/02/19
[Qemu-devel] [PATCH v6 4/4] xilinx_zynq: machine model initial version, Peter A. G. Crosthwaite, 2012/02/19
Re: [Qemu-devel] [PATCH v6 0/4] Zynq-7000 EPP platform model, Peter Crosthwaite, 2012/02/19
[Qemu-devel] [PULL] Zynq-7000 EPP platform model, Peter Crosthwaite, 2012/02/20