| Message ID | 20200717092104.15428-1-zhukeqian1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] drivers: arm arch timer: Correct fault programming of CNTKCTL_EL1.EVNTI | expand |
Friendly ping. Is this an effective bugfix? On 2020/7/17 17:21, Keqian Zhu wrote: > ARM virtual counter supports event stream. It can only trigger an event > when the trigger bit of CNTVCT_EL0 changes from 0 to 1 (or from 1 to 0), > so the actual period of event stream is 2 ^ (cntkctl_evnti + 1). For > example, when the trigger bit is 0, then it triggers an event for every > two cycles. > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > --- > drivers/clocksource/arm_arch_timer.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index ecf7b7db2d05..06d99a4b1b9b 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -799,10 +799,20 @@ static void __arch_timer_setup(unsigned type, > static void arch_timer_evtstrm_enable(int divider) > { > u32 cntkctl = arch_timer_get_cntkctl(); > + int cntkctl_evnti; > + > + /* > + * Note that it can only trigger an event when the trigger bit > + * of CNTVCT_EL0 changes from 1 to 0 (or from 0 to 1), so the > + * actual period of event stream is 2 ^ (cntkctl_evnti + 1). > + */ > + cntkctl_evnti = divider - 1; > + cntkctl_evnti = min(cntkctl_evnti, 15); > + cntkctl_evnti = max(cntkctl_evnti, 0); > > cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK; > /* Set the divider and enable virtual event stream */ > - cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT) > + cntkctl |= (cntkctl_evnti << ARCH_TIMER_EVT_TRIGGER_SHIFT) > | ARCH_TIMER_VIRT_EVT_EN; > arch_timer_set_cntkctl(cntkctl); > arch_timer_set_evtstrm_feature(); > @@ -816,10 +826,11 @@ static void arch_timer_configure_evtstream(void) > /* Find the closest power of two to the divisor */ > evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ; > pos = fls(evt_stream_div); > - if (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))) > + if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))) > pos--; > + > /* enable event stream */ > - arch_timer_evtstrm_enable(min(pos, 15)); > + arch_timer_evtstrm_enable(pos); > } > > static void arch_counter_set_user_access(void) >
On 2020-07-17 10:21, Keqian Zhu wrote: > ARM virtual counter supports event stream. It can only trigger an event > when the trigger bit of CNTVCT_EL0 changes from 0 to 1 (or from 1 to > 0), > so the actual period of event stream is 2 ^ (cntkctl_evnti + 1). For > example, when the trigger bit is 0, then it triggers an event for every > two cycles. > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > --- > drivers/clocksource/arm_arch_timer.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c > b/drivers/clocksource/arm_arch_timer.c > index ecf7b7db2d05..06d99a4b1b9b 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -799,10 +799,20 @@ static void __arch_timer_setup(unsigned type, > static void arch_timer_evtstrm_enable(int divider) > { > u32 cntkctl = arch_timer_get_cntkctl(); > + int cntkctl_evnti; > + > + /* > + * Note that it can only trigger an event when the trigger bit > + * of CNTVCT_EL0 changes from 1 to 0 (or from 0 to 1), so the > + * actual period of event stream is 2 ^ (cntkctl_evnti + 1). > + */ > + cntkctl_evnti = divider - 1; > + cntkctl_evnti = min(cntkctl_evnti, 15); > + cntkctl_evnti = max(cntkctl_evnti, 0); > > cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK; > /* Set the divider and enable virtual event stream */ > - cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT) > + cntkctl |= (cntkctl_evnti << ARCH_TIMER_EVT_TRIGGER_SHIFT) > | ARCH_TIMER_VIRT_EVT_EN; > arch_timer_set_cntkctl(cntkctl); > arch_timer_set_evtstrm_feature(); > @@ -816,10 +826,11 @@ static void arch_timer_configure_evtstream(void) > /* Find the closest power of two to the divisor */ > evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ; > pos = fls(evt_stream_div); > - if (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))) > + if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))) > pos--; > + > /* enable event stream */ > - arch_timer_evtstrm_enable(min(pos, 15)); > + arch_timer_evtstrm_enable(pos); > } > > static void arch_counter_set_user_access(void) This looks like a very convoluted fix. If the problem you are trying to fix is that the event frequency is at most half of that of the counter, why isn't the patch below the most obvious fix? Thanks, M. diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 6c3e84180146..0a65414b781f 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -824,8 +824,12 @@ static void arch_timer_configure_evtstream(void) { int evt_stream_div, pos; - /* Find the closest power of two to the divisor */ - evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ; + /* + * Find the closest power of two to the divisor. As the event + * stream can at most be generated at half the frequency of the + * counter, use half the frequency when computing the divider. + */ + evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ / 2; pos = fls(evt_stream_div); if (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))) pos--;
Hi Marc, On 2020/7/28 18:16, Marc Zyngier wrote: > On 2020-07-17 10:21, Keqian Zhu wrote: >> ARM virtual counter supports event stream. It can only trigger an event >> when the trigger bit of CNTVCT_EL0 changes from 0 to 1 (or from 1 to 0), >> so the actual period of event stream is 2 ^ (cntkctl_evnti + 1). For >> example, when the trigger bit is 0, then it triggers an event for every >> two cycles. >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> drivers/clocksource/arm_arch_timer.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clocksource/arm_arch_timer.c >> b/drivers/clocksource/arm_arch_timer.c >> index ecf7b7db2d05..06d99a4b1b9b 100644 >> --- a/drivers/clocksource/arm_arch_timer.c >> +++ b/drivers/clocksource/arm_arch_timer.c >> @@ -799,10 +799,20 @@ static void __arch_timer_setup(unsigned type, >> static void arch_timer_evtstrm_enable(int divider) >> { >> u32 cntkctl = arch_timer_get_cntkctl(); >> + int cntkctl_evnti; >> + >> + /* >> + * Note that it can only trigger an event when the trigger bit >> + * of CNTVCT_EL0 changes from 1 to 0 (or from 0 to 1), so the >> + * actual period of event stream is 2 ^ (cntkctl_evnti + 1). >> + */ >> + cntkctl_evnti = divider - 1; >> + cntkctl_evnti = min(cntkctl_evnti, 15); >> + cntkctl_evnti = max(cntkctl_evnti, 0); >> >> cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK; >> /* Set the divider and enable virtual event stream */ >> - cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT) >> + cntkctl |= (cntkctl_evnti << ARCH_TIMER_EVT_TRIGGER_SHIFT) >> | ARCH_TIMER_VIRT_EVT_EN; >> arch_timer_set_cntkctl(cntkctl); >> arch_timer_set_evtstrm_feature(); >> @@ -816,10 +826,11 @@ static void arch_timer_configure_evtstream(void) >> /* Find the closest power of two to the divisor */ >> evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ; >> pos = fls(evt_stream_div); >> - if (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))) >> + if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))) >> pos--; >> + >> /* enable event stream */ >> - arch_timer_evtstrm_enable(min(pos, 15)); >> + arch_timer_evtstrm_enable(pos); >> } >> >> static void arch_counter_set_user_access(void) > > This looks like a very convoluted fix. If the problem you are > trying to fix is that the event frequency is at most half of > that of the counter, why isn't the patch below the most > obvious fix? > > Thanks, > > M. > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index 6c3e84180146..0a65414b781f 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -824,8 +824,12 @@ static void arch_timer_configure_evtstream(void) > { > int evt_stream_div, pos; > > - /* Find the closest power of two to the divisor */ > - evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ; > + /* > + * Find the closest power of two to the divisor. As the event > + * stream can at most be generated at half the frequency of the > + * counter, use half the frequency when computing the divider. > + */ > + evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ / 2; > pos = fls(evt_stream_div); > if (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))) I think here does not consider the case of pos==1 (though it will not occur...) > pos--; > It looks good to me. Thanks, Keqian
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index ecf7b7db2d05..06d99a4b1b9b 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -799,10 +799,20 @@ static void __arch_timer_setup(unsigned type, static void arch_timer_evtstrm_enable(int divider) { u32 cntkctl = arch_timer_get_cntkctl(); + int cntkctl_evnti; + + /* + * Note that it can only trigger an event when the trigger bit + * of CNTVCT_EL0 changes from 1 to 0 (or from 0 to 1), so the + * actual period of event stream is 2 ^ (cntkctl_evnti + 1). + */ + cntkctl_evnti = divider - 1; + cntkctl_evnti = min(cntkctl_evnti, 15); + cntkctl_evnti = max(cntkctl_evnti, 0); cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK; /* Set the divider and enable virtual event stream */ - cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT) + cntkctl |= (cntkctl_evnti << ARCH_TIMER_EVT_TRIGGER_SHIFT) | ARCH_TIMER_VIRT_EVT_EN; arch_timer_set_cntkctl(cntkctl); arch_timer_set_evtstrm_feature(); @@ -816,10 +826,11 @@ static void arch_timer_configure_evtstream(void) /* Find the closest power of two to the divisor */ evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ; pos = fls(evt_stream_div); - if (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))) + if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))) pos--; + /* enable event stream */ - arch_timer_evtstrm_enable(min(pos, 15)); + arch_timer_evtstrm_enable(pos); } static void arch_counter_set_user_access(void)
ARM virtual counter supports event stream. It can only trigger an event when the trigger bit of CNTVCT_EL0 changes from 0 to 1 (or from 1 to 0), so the actual period of event stream is 2 ^ (cntkctl_evnti + 1). For example, when the trigger bit is 0, then it triggers an event for every two cycles. Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> --- drivers/clocksource/arm_arch_timer.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)