next-cube: more tidy-ups and improvements

[PATCH 26/36] next-cube: don't use rtc phase value of -1

Posted by Mark Cave-Ayland 1 month ago

The rtc phase value of -1 is directly equivalent to using a phase value of 0 so
simplify the logic to use an initial rtc phase of 0.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/next-cube.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 43b2c775c0..e4d0083eb0 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -265,9 +265,6 @@ static void next_scr2_rtc_update(NeXTPC *s)
 
     if (scr2_2 & 0x1) {
         /* DPRINTF("RTC %x phase %i\n", scr2_2, rtc->phase); */
-        if (rtc->phase == -1) {
-            rtc->phase = 0;
-        }
         /* If we are in going down clock... do something */
         if (((old_scr2 & SCR2_RTCLK) != (scr2_2 & SCR2_RTCLK)) &&
                 ((scr2_2 & SCR2_RTCLK) == 0)) {
@@ -282,7 +279,7 @@ static void next_scr2_rtc_update(NeXTPC *s)
         }
     } else {
         /* else end or abort */
-        rtc->phase = -1;
+        rtc->phase = 0;
         rtc->command = 0;
         rtc->value = 0;
     }
-- 
2.39.5

Re: [PATCH 26/36] next-cube: don't use rtc phase value of -1

Posted by Thomas Huth 2 weeks ago

Am Wed, 23 Oct 2024 09:58:42 +0100
schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

> The rtc phase value of -1 is directly equivalent to using a phase value of 0 so
> simplify the logic to use an initial rtc phase of 0.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/next-cube.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 26/36] next-cube: don't use rtc phase value of -1

Posted by BALATON Zoltan 1 month ago

On Wed, 23 Oct 2024, Mark Cave-Ayland wrote:
> The rtc phase value of -1 is directly equivalent to using a phase value of 0 so
> simplify the logic to use an initial rtc phase of 0.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/m68k/next-cube.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 43b2c775c0..e4d0083eb0 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -265,9 +265,6 @@ static void next_scr2_rtc_update(NeXTPC *s)
>
>     if (scr2_2 & 0x1) {
>         /* DPRINTF("RTC %x phase %i\n", scr2_2, rtc->phase); */
> -        if (rtc->phase == -1) {
> -            rtc->phase = 0;
> -        }
>         /* If we are in going down clock... do something */
>         if (((old_scr2 & SCR2_RTCLK) != (scr2_2 & SCR2_RTCLK)) &&
>                 ((scr2_2 & SCR2_RTCLK) == 0)) {
> @@ -282,7 +279,7 @@ static void next_scr2_rtc_update(NeXTPC *s)
>         }
>     } else {
>         /* else end or abort */
> -        rtc->phase = -1;
> +        rtc->phase = 0;
>         rtc->command = 0;
>         rtc->value = 0;
>     }

Additionally, maybe it would be simpler to invert the if condition and 
move this else branch up there to the beginning so you can return early 
after this reset (the deposit at the end does nothing after the else case 
as it's just storing back the unmodified value) and then you can deindent 
the big if where most of the functionality is now.

Regards,
BALATON Zoltan

Re: [PATCH 26/36] next-cube: don't use rtc phase value of -1

Posted by Mark Cave-Ayland 1 month ago

On 23/10/2024 11:37, BALATON Zoltan wrote:

> On Wed, 23 Oct 2024, Mark Cave-Ayland wrote:
>> The rtc phase value of -1 is directly equivalent to using a phase value of 0 so
>> simplify the logic to use an initial rtc phase of 0.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/m68k/next-cube.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
>> index 43b2c775c0..e4d0083eb0 100644
>> --- a/hw/m68k/next-cube.c
>> +++ b/hw/m68k/next-cube.c
>> @@ -265,9 +265,6 @@ static void next_scr2_rtc_update(NeXTPC *s)
>>
>>     if (scr2_2 & 0x1) {
>>         /* DPRINTF("RTC %x phase %i\n", scr2_2, rtc->phase); */
>> -        if (rtc->phase == -1) {
>> -            rtc->phase = 0;
>> -        }
>>         /* If we are in going down clock... do something */
>>         if (((old_scr2 & SCR2_RTCLK) != (scr2_2 & SCR2_RTCLK)) &&
>>                 ((scr2_2 & SCR2_RTCLK) == 0)) {
>> @@ -282,7 +279,7 @@ static void next_scr2_rtc_update(NeXTPC *s)
>>         }
>>     } else {
>>         /* else end or abort */
>> -        rtc->phase = -1;
>> +        rtc->phase = 0;
>>         rtc->command = 0;
>>         rtc->value = 0;
>>     }
> 
> Additionally, maybe it would be simpler to invert the if condition and move this else 
> branch up there to the beginning so you can return early after this reset (the 
> deposit at the end does nothing after the else case as it's just storing back the 
> unmodified value) and then you can deindent the big if where most of the 
> functionality is now.

I didn't change the layout of the outer if() mainly because Bryce reversed engineered 
a lot of this from Mach/NetBSD, and wasn't 100% confident that there was any other 
logic that needed to be applied to bit 0. It's also worth pointing out that by the 
end of the series all of the functionality has been moved to gpios, so all the 
complexity within the big if() disappears anyway.


ATB,

Mark.