lkml.org 
[lkml]   [2009]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] hpet: hpet driver periodic timer setup bug fixes
    From
    Hi,

    One comment on this patch.

    On Mon, Aug 10, 2009 at 5:05 PM, Pallipadi,
    Venkatesh<venkatesh.pallipadi@intel.com> wrote:
    > From: Nils Carlson <nils.carlson@ericsson.com>
    >
    > The periodic interrupt from drivers/char/hpet.c does not work correctly,
    > both when using the periodic capability of the hardware and while
    > emulating the periodic interrupt (when hardware does not support
    > periodic mode).
    >
    > With timers capable of periodic interrupts, the comparator field is first
    > set with the period value followed by set of hidden accumulator,
    > which has the side effect of overwriting the comparator value. This
    > results in wrong periodicity for the interrupts. For,
    > periodic interrupts to work, following steps are necessary, in that order.
    > * Set config with Tn_VAL_SET_CNF bit
    > * Write to hidden accumulator, the value written is the time when the
    >  first interrupt should be generated
    > * Write compartor with period interval for subsequent interrupts
    > (http://www.intel.com/hardwaredesign/hpetspec_1.pdf )
    >
    > When emulating periodic timer with timers not capable of periodic
    > interrupt, driver is adding the period to counter value instead of
    > comparator value, which causes slow drift when using this emulation.
    >
    > Also, driver seems to add hpetp->hp_delta both while setting up
    > periodic interrupt and while emulating periodic interrupts with timers
    > not capable of doing periodic interrupts. This hp_delta will result in
    > slower than expected interrupt rate and should not be used while setting
    > the interval.
    >
    > Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
    > Signed-off-by: Nils Carlson <nils.carlson@ericsson.com>
    > ---
    >  drivers/char/hpet.c |   21 ++++++++++++---------
    >  1 files changed, 12 insertions(+), 9 deletions(-)
    >
    > diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
    > index 4a9f349..70a770a 100644
    > --- a/drivers/char/hpet.c
    > +++ b/drivers/char/hpet.c
    > @@ -166,9 +166,8 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
    >                unsigned long m, t;
    >
    >                t = devp->hd_ireqfreq;
    > -               m = read_counter(&devp->hd_hpet->hpet_mc);
    > -               write_counter(t + m + devp->hd_hpets->hp_delta,
    > -                             &devp->hd_timer->hpet_compare);
    > +               m = read_counter(&devp->hd_timer->hpet_compare);
    > +               write_counter(t + m, &devp->hd_timer->hpet_compare);
    >        }
    >
    >        if (devp->hd_flags & HPET_SHARED_IRQ)
    > @@ -504,21 +503,25 @@ static int hpet_ioctl_ieon(struct hpet_dev *devp)
    >        g = v | Tn_32MODE_CNF_MASK | Tn_INT_ENB_CNF_MASK;
    >
    >        if (devp->hd_flags & HPET_PERIODIC) {
    > -               write_counter(t, &timer->hpet_compare);
    >                g |= Tn_TYPE_CNF_MASK;
    > -               v |= Tn_TYPE_CNF_MASK;
    > -               writeq(v, &timer->hpet_config);
    > -               v |= Tn_VAL_SET_CNF_MASK;
    > +               v |= Tn_TYPE_CNF_MASK | Tn_VAL_SET_CNF_MASK;
    >                writeq(v, &timer->hpet_config);
    >                local_irq_save(flags);
    >
    > -               /* NOTE:  what we modify here is a hidden accumulator
    > +               /*
    > +                * NOTE: First we modify the hidden accumulator
    >                 * register supported by periodic-capable comparators.
    >                 * We never want to modify the (single) counter; that
    > -                * would affect all the comparators.
    > +                * would affect all the comparators. The value written
    > +                * is the counter value when the first interrupt is due.
    >                 */
    >                m = read_counter(&hpet->hpet_mc);
    >                write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
    > +               /*
    > +                * Then we modify the comparator, indicating the period
    > +                * for subsequent interrupt.
    > +                */
    > +               write_counter(t, &timer->hpet_compare);
    >        } else {
    >                local_irq_save(flags);
    >                m = read_counter(&hpet->hpet_mc);

    Shouldn't
    write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
    be deleted if we are doing
    write_counter(t, &timer->hpet_compare);
    ?

    Regards,

    --
    Goldwyn
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2009-08-27 21:29    [W:0.029 / U:59.156 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site