Re: [Qemu-devel] [PATCH v2 4/6] hw/timer: Add Epson RX8900 RTC support

["Alastair D'Silva" <alastair@au1.ibm.com>]

On Fri, 2016-12-02 at 15:07 +1100, Alexey Kardashevskiy wrote:
> On 02/12/16 14:30, Alastair D'Silva wrote:
> > On Fri, 2016-12-02 at 13:48 +1100, Alexey Kardashevskiy wrote:
> > > On 02/12/16 11:19, Alastair D'Silva wrote:
> > > > On Thu, 2016-12-01 at 16:53 +1100, Alexey Kardashevskiy wrote:
> > > > 
> > > > > On 30/11/16 16:36, Alastair D'Silva wrote:
> > > > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > > > > 
> > > > > > This patch adds support for the Epson RX8900 I2C RTC.
> > > > > > 
> > > > > > The following chip features are implemented:
> > > > > >  - RTC (wallclock based, ptimer 10x oversampling to pick up
> > > > > > 	wallclock transitions)
> > > > > >  - Time update interrupt (per second/minute, wallclock
> > > > > > based)
> > > > > >  - Alarms (wallclock based)
> > > > > >  - Temperature (set via a property)
> > > > > >  - Countdown timer (emulated clock via ptimer)
> > > > > >  - FOUT via GPIO (emulated clock via ptimer)
> > > > > > 
> > > > > > The following chip features are unimplemented:
> > > > > >  - Low voltage detection
> > > > > >  - i2c timeout
> > > > > > 
> > > > > > The implementation exports the following named GPIOs:
> > > > > > rx8900-interrupt-out
> > > > > > rx8900-fout-enable
> > > > > > rx8900-fout
> > > > > > 
> > > > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > > > > > Signed-off-by: Chris Smart <chris@distroguy.com>
> > > > > > ---
> > > > > >  default-configs/arm-softmmu.mak |   1 +
> > > > > >  hw/timer/Makefile.objs          |   2 +
> > > > > >  hw/timer/rx8900.c               | 890
> > > > > > ++++++++++++++++++++++++++++++++++++++++
> > > > > >  hw/timer/rx8900_regs.h          | 139 +++++++
> > > > > >  hw/timer/trace-events           |  31 ++
> > > > > >  5 files changed, 1063 insertions(+)
> > > > > >  create mode 100644 hw/timer/rx8900.c
> > > > > >  create mode 100644 hw/timer/rx8900_regs.h
> > > > > > 
> > > > > > 
<snip>
> > > > > > +#define TYPE_RX8900 "rx8900"
> > > > > > +#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj),
> > > > > > TYPE_RX8900)
> > > > > > +
> > > > > > +typedef struct RX8900State {
> > > > > > +    I2CSlave parent_obj;
> > > > > > +
> > > > > > +    ptimer_state *sec_timer; /* triggered once per second
> > > > > > */
> > > > > > +    ptimer_state *fout_timer;
> > > > > > +    ptimer_state *countdown_timer;
> > > > > > +    bool fout;
> > > > > 
> > > > > Is this "FOE" on the chip?
> > > > > 
> > > > 
> > > > No, it tracks the state of the fout waveform. I'll rename it to
> > > > fout_state.
> > > 
> > > Since it is bool, fout_enabled makes more sense.
> > > 
> > 
> > No it does't, it's not an enabled control, but the phase of the
> > waveform.
> 
> I do not mind that "enabled" may be bad name but you are not helping
> :)
> 
> A "phase" can be false or true? What does "true" phase of a waveform
> mean?
> 
> I cannot find neither "phase" nor "waveform" words in the spec.
> 
FOUT outputs a 1, 1024 or 32768Hz square wave. We need to track the
current state of the wave so we know what the next state should be.

<snip>
> > > > > > +
> > > > > > +static void capture_current_time(RX8900State *s)
> > > > > > +{
> > > > > > +    /* Capture the current time into the secondary
> > > > > > registers
> > > > > > +     * which will be actually read by the data transfer
> > > > > > operation.
> > > > > > +     */
> > > > > > +    struct tm now;
> > > > > > +    qemu_get_timedate(&now, s->offset);
> > > > > > +    s->nvram[SECONDS] = to_bcd(now.tm_sec);
> > > > > > +    s->nvram[MINUTES] = to_bcd(now.tm_min);
> > > > > > +    s->nvram[HOURS] = to_bcd(now.tm_hour);
> > > > > > +
> > > > > > +    s->nvram[WEEKDAY] = 0x01 << ((now.tm_wday + s-
> > > > > > > wday_offset) %
> > > > > > 
> > > > > > 7);
> > > > > 
> > > > > s/0x01/1/ ?
> > > > 
> > > > Weekday is a walking bit, I think hex notation is clearer.
> > > 
> > > 
> > > "1" is a pure one. 0x01 suggests a bit mask so it may be possible
> > > to
> > > shift
> > > some other value. I cannot think how 0x01 is clearer, I asked
> > > Paul,
> > > he
> > > cannot either ;)
> > > 
> > 
> > From the datasheet:
> > The day data values are counted as follows: Day 01h, Day 02h, Day
> > 04h,
> > Day 08h, Day 10h, Day 20h, Day 40h, Day 01h, Day 02h, etc.   
> 
> So it is not a mask. "1" it is.
> 

I'm still not seeing your point, we are operating on raw binary data,
not a numeric. It is clearly defined as binary data in the datasheet,
and I consider hex/oct/bin notation to be the most appropriate for it
(even the datasheet authors use it). Yes, I understand that masks are
one type of binary information that is suitably represented by it, my
argument is that the use case is wider than you are asserting.

<snip>
> > > > > 
> > > > > The RX8900 spec does not use word "offset" at all so I cannot
> > > > > make
> > > > > sense
> > > > > from the paragraph above - why exactly do you need 2 offsets
> > > > > ("offset" and
> > > > > "wday_offset") and why weekday cannot be calculated when it
> > > > > is
> > > > > needed
> > > > > from
> > > > > the current time + "offset"?
> > > > > 
> > > > 
> > > > The offsets refer to the difference between the host clock &
> > > > the
> > > > emulated clock. The weekday cannot be calculated as the RX8900
> > > > does
> > > > not
> > > > validate the weekday - the user can set the weekday to
> > > > anything.
> > > 
> > > 
> > > Ufff. Now I am totally confused. You say "the weekday cannot be
> > > calculated"
> > > but the comment says you defer its calculation. The comment needs
> > > an
> > > update.
> > 
> > Sorry, my bad, the weekday cannot be calculated without the weekday
> > offset, as the weekday is not guaranteed to be correct for that
> > date.
> 
> 
> Short description of the whole weekday business in a comment would
> help.
> 
Ok. 


> > > > > > +        break;
> > > > > > +
> > > > > > +    case WEEKDAY:
> > > > > > +    case EXT_WEEKDAY: {
> > > > > > +        int user_wday = ctz32(data);
> > > > > > +        /* The day field is supposed to contain a value in
> > > > > > +         * the range 0-6. Otherwise behavior is undefined.
> > > > > > +         */
> > > > > > +        switch (data) {
> > > > > > +        case 0x01:
> > > > > > +        case 0x02:
> > > > > > +        case 0x04:
> > > > > > +        case 0x08:
> > > > > > +        case 0x10:
> > > > > > +        case 0x20:
> > > > > > +        case 0x40:
> > > > > > +            break;
> > > > > > +        default:
> > > > > 
> > > > > Instead of the switch:
> > > > > 
> > > > > if (data & 0x80) {
> > > > 
> > > > Weekday is a walking bit, the proposed check allows invalid
> > > > values.
> > > 
> > > 
> > > Ah, right, should have been if(data==0x80).
> > > 
> > > Actually you want to test that
> > > ((1<<ctz32(data)) == data) && (user_wday < 7)
> > 
> > It's short and clever, but the problem with clever code is it
> > requires
> > clever people to understand it. I had to stop and think about what
> > that
> > was doing.
> 
> 
> The only "clever" bit about it that it uses ctz() (which is not very
> common) but the code uses it anyway.
> 
> You comment says "a value in the range 0-6" but the code does not
> actually
> compare with 0 and 6, and values up to 0x40 (way beyond 6) are
> allowed - I
> stopped to think why is that.

I've updated the comment to clarify the behaviour.

<snip>
> > > > > > +    snprintf(name, sizeof(name), "rx8900-interrupt-out");
> > > > > > +    qdev_init_gpio_out_named(&i2c->qdev, &s-
> > > > > > >interrupt_pin,
> > > > > > name,
> > > > > > 1);
> > > > > > +    trace_rx8900_pin_name("Interrupt", name);
> > > > > 
> > > > > I would just pass a string to qdev_init_gpio_out_named() and
> > > > > ditch
> > > > > @name
> > > > > from tracepoints as they use unique strings anyway. And I'd
> > > > > also
> > > > 
> > > > The same trace is reused
> > > 
> > > The first parameter of reused trace is unique anyway.
> > > 
> > 
> > Yes, but the name value-adds.
> 
> Nope, not at all. See the comment below.
> 
> 
> > 
> > > 
> > > > 
> > > > > ditch
> > > > > trace_rx8900_pin_name tracepoints as they do not seem very
> > > > > useful
> > > > > -
> > > > > they do
> > > > > not report an error or a success.
> > > > 
> > > > It makes it easier when debugging to validate that your idea of
> > > > the
> > > > named interrupt matches the implementation.
> > > 
> > > I am missing the point here. If the trace printed a string from
> > > i2c-
> > > > qdev,
> > > 
> > > then yes, the trace could be used to tell if the actual name
> > > matches
> > > what
> > > you passed to qdev_init_gpio_in_named() but in this code the
> > > trace
> > > will
> > > always print the string you snprintf() 2 lines above. Quite
> > > useless.
> > > 
> > 
> > It was useful to me in development, it may be useful in the future.
> 
> I seriously doubt it but it is not me whose "reviewed-by" you need
> anyway
> and Cedric gave you one, this should do it :)
> 
> 
> > Think of it as an announcement of "this is where you can find me".
> 
> 
> The "rx8900_pin_name" tracepoint name is pretty precise in this case
> already - only one function uses it and enabling it will give you 3
> lines
> of traces while one would do exact same job - tells you that
> rx8900_realize() is called. Again, it prints static strings only, and
> the
> strings are define right there.
> 

I don't think we are going to reach alignment here, but it's a pretty
minor point.

-- 
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819