new tlcs900 variant: TMP94C241 by felipesanches · Pull Request #13220 · mamedev/mame

felipesanches · 2025-01-12T03:05:38Z

Used on the KN5000 driver.

felipesanches · 2025-01-12T07:06:58Z

Before/after comparison

On current git master, the screen gets almost completely black with just a glitch of half a dozen colored pixels at the top line:

With the new CPU variant implemented in this PR, we get a good-looking system message regarding control-panel MCU communications error:

felipesanches · 2025-01-12T21:49:33Z

This PR is a followup to previous work done at #12684. I believe that I have already addressed all issues pointed out by @cuavas in his previous code review. In this PR I have included only the implementation of the new CPU variant and the small snippets of code in the KN5000 driver that use features on this variant.

The rest of the code for the kn5000 driver will be submitted as separate PRs later to make it easier to review and merge smaller chunks of code.

@wilbertpol and/or @ajrhacker, as you both are listed as authors of other variants of the TLCS900 family, could you please take a look at this PR?

felipesanches · 2025-01-15T04:18:21Z

Note: The wording of the ** ERROR in CPU data transmission ** message is a bit misleading. I know it is related to the control panel MCUs due to this description in the service manual:

felipesanches · 2025-01-15T04:18:59Z

To make that work, I'll need to implement UART support in the TMP94C241 CPU device and then use it to attach the MCUs (with HLE initially). But I'd like to land this PR before attempting that, as the amount of code needing review keeps growing.

wilbertpol · 2025-01-21T19:30:08Z

@wilbertpol and/or @ajrhacker, as you both are listed as authors of other variants of the TLCS900 family, could you please take a look at this PR?

I don't need to be in the copyright-holders section of the tmp94c241.cpp/h files since it is all code made by you.

felipesanches · 2025-01-21T20:33:33Z

@wilbertpol and/or @ajrhacker, as you both are listed as authors of other variants of the TLCS900 family, could you please take a look at this PR?

I don't need to be in the copyright-holders section of the tmp94c241.cpp/h files since it is all code made by you.

OK. I've pushed a commit removing your name from the copyright holders list of those files.

and adopt code-style suggested at mamedev#13220 (comment)

by passing it directly the operation values instead of explicitly interpreting their meaning on the calling end. (Addressing "it feels like it's always used with constant parameters" comment from mamedev#13220 (comment))

and mamedev#13220 (comment)

on the methods that write chip-select registers (as suggested at: mamedev#13220 (comment))

and adopt code-style suggested at mamedev#13220 (comment)

by passing it directly the operation values instead of explicitly interpreting their meaning on the calling end. (Addressing "it feels like it's always used with constant parameters" comment from mamedev#13220 (comment))

and mamedev#13220 (comment)

on the methods that write chip-select registers (as suggested at: mamedev#13220 (comment))

(following tip from mamedev#13220 (comment) and mamedev#13220 (comment))

(following tips from mamedev#13220 (review))

felipesanches · 2025-02-10T16:40:43Z

I've also just rebased this with current master branch.

felipesanches · 2025-02-15T19:30:05Z

I've been making commits here to facilitate code-reviews. But now that I've addressed all feedback, perhaps it would be useful for me to git squash it all into a single commit.

felipesanches · 2025-02-15T22:06:24Z

I've squashed all 30 commits into a single one and rebased it against current MAME master branch. The original commits are still available at https://github.com/felipesanches/mame/tree/tlcs900_tmp94c241_before_squashing (just in case it can be useful for the code-reviews).

felipesanches · 2025-02-16T04:18:34Z

Also use templates for the two serial channels (squashed-in code corresponding to felipesanches@eeca47a)

cuavas · 2025-02-16T18:59:45Z

+	switch( flipflop )
+		{
+		case 0x1:
+			if (!BIT(m_port_function[PORT_C], 0) || BIT(m_port_control[PORT_C], 0)) return;
+			break;
+		case 0x7:
+			if (!BIT(m_port_function[PORT_C], 0) || !BIT(m_port_control[PORT_C], 0)) return;
+			break;
+		case 0x3:
+			if (!BIT(m_port_function[PORT_C], 1) || BIT(m_port_control[PORT_C], 1)) return;
+			break;
+		case 0xb:
+			if (!BIT(m_port_function[PORT_C], 1) || !BIT(m_port_control[PORT_C], 1)) return;
+			break;
+		case 0x4:
+			if (!BIT(m_port_function[PORT_D], 0)) return;
+			break;
+		case 0x6:
+			if (!BIT(m_port_function[PORT_D], 4)) return;
+			break;
+		case 0x8:
+			if (!BIT(m_port_function[PORT_E], 0)) return;
+			break;
+		case 0xA:
+			if (!BIT(m_port_function[PORT_E], 4)) return;
+			break;
+		default:
+			// invalid flip flop
+			return;
+		}


Indentation is off – there’s an extra indent on the braces.

cuavas · 2025-02-16T19:00:30Z

+	switch( flipflop )
+		{
+		case 0x1:
+		case 0x7:
+			new_port_value = m_port_latch[PORT_C] & 0xfe;
+			if (ff_state) new_port_value |= 0x01;
+			port_w<PORT_C>(new_port_value);
+			break;
+		case 0x3:
+		case 0xb:
+			new_port_value = m_port_latch[PORT_C] & 0xfd;
+			if (ff_state) new_port_value |= 0x02;
+			port_w<PORT_C>(new_port_value);
+			break;
+		case 0x4:
+			new_port_value = m_port_latch[PORT_D] & 0xfe;
+			if (ff_state) new_port_value |= 0x01;
+			port_w<PORT_D>(new_port_value);
+			break;
+		case 0x6:
+			new_port_value = m_port_latch[PORT_D] & 0xef;
+			if (ff_state) new_port_value |= 0x10;
+			port_w<PORT_D>(new_port_value);
+			break;
+		case 0x8:
+			new_port_value = m_port_latch[PORT_E] & 0xfe;
+			if (ff_state) new_port_value |= 0x01;
+			port_w<PORT_E>(new_port_value);
+			break;
+		case 0xa:
+			new_port_value = m_port_latch[PORT_E] & 0xef;
+			if (ff_state) new_port_value |= 0x10;
+			port_w<PORT_E>(new_port_value);
+			break;
+		}


Same here – braces should be at the same level as the switch keyword.

Thanks!

Is this better now? 77cb086

cuavas · 2025-02-16T19:04:19Z

+		switch( (m_t01mod >> 2) & 3 ) /* T1_INPUT_CLOCK */
+		{
+		case 0:  /* TO0TRG */
+			break;
+		case 1:  /* T1 */
+			m_timer_change[1] += ( m_timer_pre >> 3 ) - ( old_pre >> 3 );
+			break;
+		case 2:  /* T16 */
+			m_timer_change[1] += ( m_timer_pre >> 7 ) - ( old_pre >> 7 );
+			break;
+		case 3:  /* T256 */
+			m_timer_change[1] += ( m_timer_pre >> 11 ) - ( old_pre >> 11 );
+			break;
+		}
+
+		for( ; m_timer_change[1] > 0; m_timer_change[1]-- )
+		{
+			m_timer_8[1] += 1; /* UPCOUNTER_1 */
+			if ( m_timer_8[1] == m_treg_8[TREG1] )
+			{
+				m_timer_8[1] = 0;
+				m_int_reg[INTET01] |= 0x80;
+				m_check_irqs = 1;
+
+				if ( (m_tffcr & 3) == 3 ) /* "FF1 Invert Enable" && "Invert by timer 1" */
+					change_timer_flipflop( 1, FF_INVERT );
+
+				/* In 16bit timer mode also reset timer 0 */
+				if ( ((m_t01mod >> 6) & 3) == 1 ) /* TO1_OPERATING_MODE == MODE_16BIT_TIMER */
+					m_timer_8[1] = 0;
+			}
+		}


This seems to be repeated quite a bit. Can you encapsulate at least some of it with a local lambda or something?

I've refactored a portion of this code with a local lambda (the updating of timer counter values) at cd07165 and now I'm trying to do the same with the rest of the timer code.

OK. The remainder of the implementation of timers was generalized (at 9fcd79c) with a couple local lambdas: One for the 8bit timers that can also optionally operate in pairs as 16 bit timers. And the other lambda is for the 16-bit only timers.

cuavas · 2025-02-16T19:06:44Z

+		if (level != m_level[TLCS900_INT4]) {
+			m_level[TLCS900_INT4] = level;
+			if (level == ASSERT_LINE)
+				m_int_reg[INTE45] |= 0x08;
+			else
+				m_int_reg[INTE45] &= ~0x08;
+		}
+		break;


Similarly, there are eight almost identical copies of this. Can you make a local lamba that takes the index to m_level, index to m_int_reg and bit mask for m_int_reg as parameters?

Sure! Do you like it this way? 3861008

cuavas · 2025-02-16T19:07:28Z

+
+	m_nmi_state = CLEAR_LINE;
+	for( int i = 0; i < TLCS900_NUM_INPUTS; i++ )
+	{
+		m_level[i] = CLEAR_LINE;
+	}


You really want to do this in device_resolve_objects to avoid annoyances when devices push output state on start.

Moved this into device_resolve_objects at 66bed0c

Used on the KN5000 driver.

based on prescaler inputs and input clock selection

…ut state on start.

felipesanches · 2025-02-17T03:36:10Z

@cuavas, I believe that with the few last commits pushed here, I've addressed all of the things you brought up and this PR is again ready for a review.

The rest of this folder isn't great for consistency, but we may as well make a good start with this one.

felipesanches · 2025-02-17T19:08:08Z

Awesome! :-D

wilbertpol reviewed Jan 21, 2025

View reviewed changes