Skip to content

ns32081.cpp: fix for SFSR and LDSR opcodes that dont compute anything #13626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+7 −6
Closed

ns32081.cpp: fix for SFSR and LDSR opcodes that dont compute anything #13626

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/devices/machine/ns32081.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ void ns32081_device_base::state_add(device_state_interface &parent, int &index)

template <typename T> T ns32081_device_base::read()
{
if (m_state == RESULT && m_op[2].issued < m_op[2].expected)
LOG("read: state(%d) issue(%d) expected(%d)\n", m_state, m_op[2].issued, m_op[2].expected);
if ((m_state == STATUS || m_state == RESULT) && m_op[2].issued < m_op[2].expected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not correct. Per section 3.9.1 "Slave Processor Protocol", the status must be read before attempting to read any results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is NO result to read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the status is mandatory, results or not.

{
T const data = m_op[2].value >> (m_op[2].issued * 8);

Expand All @@ -130,7 +131,7 @@ template <typename T> T ns32081_device_base::read()
return data;
}

logerror("read protocol error (%s)\n", machine().describe_context());
logerror("read protocol error state(%d) issue(%d) expected(%d) (%s)\n", m_state, m_op[2].issued, m_op[2].expected, machine().describe_context());
return 0;
}

Expand All @@ -149,7 +150,7 @@ template <typename T> void ns32081_device_base::write(T data)
}
else
{
LOG("write idbyte 0x%04x (%s)\n", data, machine().describe_context());
LOG("write idbyte 0x%02x (%s)\n", data, machine().describe_context());
if ((data == FORMAT_9) || (data == FORMAT_11) || (type() == NS32381 && data == FORMAT_12))
{
// record idbyte
Expand Down Expand Up @@ -633,9 +634,9 @@ void ns32081_device_base::execute()
}

if (m_status & SLAVE_Q)
LOG("execute %s 0x%x,0x%x exception\n", operation, m_op[0].value, m_op[1].value);
LOG("execute %s 0x%8.8x,0x%8.8x exception\n", operation, m_op[0].value, m_op[1].value);
else
LOG("execute %s 0x%x,0x%x result 0x%x\n", operation, m_op[0].value, m_op[1].value, m_op[2].value);
LOG("execute %s 0x%8.8x,0x%8.8x result 0x%x\n", operation, m_op[0].value, m_op[1].value, m_op[2].value);
}

// write-back floating point register results
Expand All @@ -656,7 +657,7 @@ void ns32081_device_base::execute()
if (!m_out_spc.isunset())
m_complete->adjust(attotime::from_ticks(m_tcy, clock()));

m_state = STATUS;
m_state = m_op[2].expected ? STATUS : IDLE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is also not correct. Reading the status is mandatory in the protocol, regardless of whether there are results or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats not what Tektronix 4404 does. Try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted and now not getting the "read protocol error" I wasn't imagining it - but I have seen tek4404 FPU get into a weird state that only a reboot can fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, helps if you turn on logging...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Tektronix documentation describes in some detail the circuit responsible for ensuring status is only read when the FPU signals completion, and there's no indication there that reading the status is optional.

The National Semiconductor documentation is also pretty clear about how the protocol works, and the only exception (where status is not read) is explicitly documented for the LMR/LCR instructions, which are a different type/format. In addition, AN-383 explicitly states (in relation to step 6 of the protocol): "This transfer is mandatory, regardless of whether the information presented by the FPU is intended to be used.".

If the Tektronix code is truly failing to read the status under some circumstances, I think the explanation and proper fix relate to the currently unemulated "abort" behavior. The datasheet states that writing a new slave ID byte (i.e., starting a new instruction) will cancel any protocol in progress. To emulate this properly will require separating the ID byte write from the existing write() function, so that the proper input status can be identified without relying on the state machine. While emulating this will likely fix the Tektronix driver, it does raise a question about how that system guards against unintentionally aborting an instruction that was in progress (again, mentioned in the application note).

}

u16 ns32081_device_base::status(int *icount)
Expand Down
Loading