Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode
On Fri, Oct 06, 2023 at 02:36:52AM +, Damien Zammit wrote: > >From: Michael Tokarev > >26.02.2023 04:58, Damien Zammit wrote: > >> Currently, the one-shot (mode 1) PIT expires far too quickly, > >> due to the output being set under the wrong logic. > >> This change fixes the one-shot PIT mode to behave similarly to mode 0. > >> > >> TESTED: using the one-shot PIT mode to calibrate a local apic timer. > > > >Has this been forgotten, or is it not needed anymore? > > This is still required but nobody uses the > PIT one-shot mode (probably because it *is* currently broken). > > Can it be merged? > > Thanks, > Damien OK, I tagged it. thanks!
Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode
>From: Michael Tokarev >26.02.2023 04:58, Damien Zammit wrote: >> Currently, the one-shot (mode 1) PIT expires far too quickly, >> due to the output being set under the wrong logic. >> This change fixes the one-shot PIT mode to behave similarly to mode 0. >> >> TESTED: using the one-shot PIT mode to calibrate a local apic timer. > >Has this been forgotten, or is it not needed anymore? This is still required but nobody uses the PIT one-shot mode (probably because it *is* currently broken). Can it be merged? Thanks, Damien
Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode
26.02.2023 04:58, Damien Zammit wrote:
Currently, the one-shot (mode 1) PIT expires far too quickly,
due to the output being set under the wrong logic.
This change fixes the one-shot PIT mode to behave similarly to mode 0.
TESTED: using the one-shot PIT mode to calibrate a local apic timer.
Has this been forgotten, or is it not needed anymore?
Thanks,
/mjt
hw/timer/i8254_common.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index 050875b497..9164576ca9 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -52,10 +52,8 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
switch (s->mode) {
default:
case 0:
-out = (d >= s->count);
-break;
case 1:
-out = (d < s->count);
+out = (d >= s->count);
break;
case 2:
if ((d % s->count) == 0 && d != 0) {
--
2.39.0
Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode
On Sun, Feb 26, 2023 at 01:11:19PM +0100, BALATON Zoltan wrote: > On Sun, 26 Feb 2023, Max Filippov wrote: > > On Sun, Feb 26, 2023 at 1:18 AM Damien Zammit wrote: > > > > > > Hi Michael, > > > > > > Thanks for reviewing this on a weekend! > > > > > > On 26/2/23 19:51, Michael S. Tsirkin wrote: > > > > On Sun, Feb 26, 2023 at 01:58:10AM +, Damien Zammit wrote: > > > > > case 0: > > > > > -out = (d >= s->count); > > > > > -break; > > > > > > > > > > > > I think you need something like > > > > /* FALLTHRU */ > > > > here otherwise some gcc versions will warn. > > > > > > > > > case 1: > > > > > -out = (d < s->count); > > > > > +out = (d >= s->count); > > > > > > It seems that there are quite a number of these consecutive fallthrough > > > cases > > > without /* FALLTHRU */ in i8254_common.c > > > > > > Can these be fixed in a separate patch? > > > > I believe that the comment is only needed when there's code > > between the labels and is not needed between the labels that > > follow each other. > > I think so too, I have some of these consecutive case labels in my code and > never had a problem with that. Only when you have a statement between labels > without break is when a comment is needed. > > Regards, > BALATON Zoltan I just tried and it looks like you are right. Pls ignore sorry about the noise. -- MST
Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode
On Sun, 26 Feb 2023, Max Filippov wrote: On Sun, Feb 26, 2023 at 1:18 AM Damien Zammit wrote: Hi Michael, Thanks for reviewing this on a weekend! On 26/2/23 19:51, Michael S. Tsirkin wrote: On Sun, Feb 26, 2023 at 01:58:10AM +, Damien Zammit wrote: case 0: -out = (d >= s->count); -break; I think you need something like /* FALLTHRU */ here otherwise some gcc versions will warn. case 1: -out = (d < s->count); +out = (d >= s->count); It seems that there are quite a number of these consecutive fallthrough cases without /* FALLTHRU */ in i8254_common.c Can these be fixed in a separate patch? I believe that the comment is only needed when there's code between the labels and is not needed between the labels that follow each other. I think so too, I have some of these consecutive case labels in my code and never had a problem with that. Only when you have a statement between labels without break is when a comment is needed. Regards, BALATON Zoltan
Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode
On Sun, Feb 26, 2023 at 1:18 AM Damien Zammit wrote: > > Hi Michael, > > Thanks for reviewing this on a weekend! > > On 26/2/23 19:51, Michael S. Tsirkin wrote: > > On Sun, Feb 26, 2023 at 01:58:10AM +, Damien Zammit wrote: > >> case 0: > >> -out = (d >= s->count); > >> -break; > > > > > > I think you need something like > > /* FALLTHRU */ > > here otherwise some gcc versions will warn. > > > >> case 1: > >> -out = (d < s->count); > >> +out = (d >= s->count); > > It seems that there are quite a number of these consecutive fallthrough cases > without /* FALLTHRU */ in i8254_common.c > > Can these be fixed in a separate patch? I believe that the comment is only needed when there's code between the labels and is not needed between the labels that follow each other. -- Thanks. -- Max
Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode
Hi Michael, Thanks for reviewing this on a weekend! On 26/2/23 19:51, Michael S. Tsirkin wrote: > On Sun, Feb 26, 2023 at 01:58:10AM +, Damien Zammit wrote: >> case 0: >> -out = (d >= s->count); >> -break; > > > I think you need something like > /* FALLTHRU */ > here otherwise some gcc versions will warn. > >> case 1: >> -out = (d < s->count); >> +out = (d >= s->count); It seems that there are quite a number of these consecutive fallthrough cases without /* FALLTHRU */ in i8254_common.c Can these be fixed in a separate patch? Damien
Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode
On Sun, Feb 26, 2023 at 01:58:10AM +, Damien Zammit wrote:
> Currently, the one-shot (mode 1) PIT expires far too quickly,
> due to the output being set under the wrong logic.
> This change fixes the one-shot PIT mode to behave similarly to mode 0.
>
> TESTED: using the one-shot PIT mode to calibrate a local apic timer.
>
> Signed-off-by: Damien Zammit
>
> ---
> hw/timer/i8254_common.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
> index 050875b497..9164576ca9 100644
> --- a/hw/timer/i8254_common.c
> +++ b/hw/timer/i8254_common.c
> @@ -52,10 +52,8 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
> switch (s->mode) {
> default:
> case 0:
> -out = (d >= s->count);
> -break;
I think you need something like
/* FALLTHRU */
here otherwise some gcc versions will warn.
> case 1:
> -out = (d < s->count);
> +out = (d >= s->count);
> break;
> case 2:
> if ((d % s->count) == 0 && d != 0) {
> --
> 2.39.0
>
[PATCH qemu] timer/i8254: Fix one shot PIT mode
Currently, the one-shot (mode 1) PIT expires far too quickly,
due to the output being set under the wrong logic.
This change fixes the one-shot PIT mode to behave similarly to mode 0.
TESTED: using the one-shot PIT mode to calibrate a local apic timer.
Signed-off-by: Damien Zammit
---
hw/timer/i8254_common.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index 050875b497..9164576ca9 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -52,10 +52,8 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
switch (s->mode) {
default:
case 0:
-out = (d >= s->count);
-break;
case 1:
-out = (d < s->count);
+out = (d >= s->count);
break;
case 2:
if ((d % s->count) == 0 && d != 0) {
--
2.39.0
