HP98x6: added HP98628 & HP98629 expansion cards #12984
Conversation
|
Amazing timing! In the very moment I opened this PR, @happppp added function |
src/devices/bus/hp_dio/hp98628_9.cpp
Outdated
| virtual void device_start() override; | ||
| virtual void device_reset() override; |
There was a problem hiding this comment.
Can you add ATTR_COLD to the cold lifecycle functions? @holub did this for most of the existing devices.
src/devices/bus/hp_dio/hp98628_9.cpp
Outdated
| void base_config(machine_config &config); | ||
| virtual void cpu_program_mem_map(address_map &map); | ||
| void cpu_io_mem_map(address_map &map); | ||
| virtual void install_68k_map(offs_t base_addr); |
There was a problem hiding this comment.
These look like cold functions as well.
src/devices/bus/hp_dio/hp98628_9.cpp
Outdated
| virtual void tx_out(int state) {}; | ||
| virtual void tt_out(int state) {}; | ||
| virtual void rts_out(int state) {} | ||
| virtual void dtr_out(int state) {}; | ||
| virtual void ocd1_out(int state) {}; |
src/devices/bus/hp_dio/hp98628_9.cpp
Outdated
| case 2: | ||
| // Semaphore | ||
| // Read & clear | ||
| if (!m_semaphore) { | ||
| BIT_SET(res, 7); | ||
| } | ||
| m_semaphore = false; | ||
| break; |
There was a problem hiding this comment.
You need to suppress side effects of reads for debugger accesses.
src/devices/bus/hp_dio/hp98628_9.cpp
Outdated
| virtual void device_add_mconfig(machine_config &config) override; | ||
| virtual ioport_constructor device_input_ports() const override; | ||
| virtual const tiny_rom_entry *device_rom_region() const override; |
src/devices/machine/z80sio.cpp
Outdated
| if (!(data & WR1_WRDY_ENABLE)) | ||
| set_ready(false); | ||
| else if (data & WR1_WRDY_ON_RX_TX) | ||
| set_ready(bool(m_rr0 & RR0_RX_CHAR_AVAILABLE)); | ||
| else | ||
| set_ready(m_rr0 & RR0_TX_BUFFER_EMPTY); | ||
| update_wait_ready(); |
src/devices/machine/z80sio.cpp
Outdated
| return ((m_wr4 & WR4_STOP_BITS_MASK) == WR4_STOP_BITS_SYNC) ? | ||
| (RR1_RX_OVERRUN_ERROR | RR1_END_OF_FRAME) : | ||
| (RR1_RX_OVERRUN_ERROR | RR1_CRC_FRAMING_ERROR); | ||
| if ((m_wr1 & WR1_RX_INT_MODE_MASK) == WR1_RX_INT_DISABLE) | ||
| return 0; | ||
| else | ||
| { | ||
| uint8_t mask = ((m_wr4 & WR4_STOP_BITS_MASK) == WR4_STOP_BITS_SYNC) ? | ||
| (RR1_RX_OVERRUN_ERROR | RR1_END_OF_FRAME) : | ||
| (RR1_RX_OVERRUN_ERROR | RR1_CRC_FRAMING_ERROR); | ||
| if ((m_wr1 & WR1_RX_INT_MODE_MASK) == WR1_RX_INT_ALL_PARITY) | ||
| mask |= RR1_PARITY_ERROR; | ||
| return mask; | ||
| } |
There was a problem hiding this comment.
If either branch of an if/else requires braces, please use them for both branches.
| void z80sio_channel::update_rx_int() | ||
| { | ||
| bool state = false; | ||
|
|
||
| auto rx_int_mode = m_wr1 & WR1_RX_INT_MODE_MASK; | ||
| if (rx_int_mode != WR1_RX_INT_DISABLE) | ||
| { | ||
| if (m_rr1 & get_special_rx_mask()) | ||
| state = true; | ||
| else if (m_rx_fifo_depth) | ||
| { | ||
| // FIFO not empty | ||
| if (rx_int_mode != WR1_RX_INT_FIRST) | ||
| state = true; | ||
| else if (m_rx_error_fifo & RR1_HIDDEN_1ST_MARKER) | ||
| state = true; | ||
| } | ||
| } | ||
| LOGINT("rx %d wr1 %02x rr1 %02x fd %u ref %06x\n", state, m_wr1, m_rr1, m_rx_fifo_depth, m_rx_error_fifo); | ||
| if (state) | ||
| m_uart->trigger_interrupt(m_index, INT_RECEIVE); | ||
| else | ||
| m_uart->clear_interrupt(m_index, INT_RECEIVE); | ||
| } |
There was a problem hiding this comment.
Switching to using this function rather than only checking what’s actually changed is going to mean there will be a lot of unnecessary interrupt output callbacks, which will potentially trigger scheduler synchronisation because trigger_interrupt and clear_interrupt call check_interrupts which doesn’t check whether the interrupt status has changed before invoking the callback. I’d consider that a regression.
There was a problem hiding this comment.
Ok, I added checks in trigger_interrupt and clear_interrupt that cause check_interrupt to be called only in case the interrupt request changes state.
src/devices/machine/z80sio.cpp
Outdated
| if ((m_wr4 & (WR4_SYNC_MODE_MASK | WR4_STOP_BITS_MASK)) == (WR4_SYNC_MODE_SDLC | WR4_STOP_BITS_SYNC) && | ||
| !(m_tx_flags & TX_FLAG_FRAMING) && (m_tx_hist & 0x1f) == 0x1f) | ||
| // SDLC, not sending framing & 5 ones in a row: do zero insertion | ||
| new_bit = false; | ||
| if ((m_wr4 & (WR4_SYNC_MODE_MASK | WR4_STOP_BITS_MASK)) == (WR4_SYNC_MODE_SDLC | WR4_STOP_BITS_SYNC) && | ||
| (m_tx_flags & (TX_FLAG_DATA_TX | TX_FLAG_CRC_TX)) && (m_tx_hist & 0x1f) == 0x1f) | ||
| // SDLC, sending data/CRC & 5 ones in a row: do zero insertion | ||
| new_bit = false; |
There was a problem hiding this comment.
What have you done to the indentation here?
src/devices/machine/z80sio.cpp
Outdated
| // copyright-holders:Curt Coder, Joakim Larsson Edstrom | ||
| // copyright-holders:Curt Coder, Joakim Larsson Edstrom, F.Ulivi |
There was a problem hiding this comment.
I’m not sure this warrants claiming copyright, considering bigger reworkings (e.g. 8c9602f) haven’t resulted in claiming copyright.
|
BTW, after reading the docs I think |
Hi,
this PR adds the support for HP98628 & HP98629 expansion cards on HP98x6 systems.
A few notes:
to test them, though. That's the reason why for the time being I'm keeping the two
cards I developed on a separate set of optional cards (the one in dio16_hp98x6_cards
function). Once the other 16-bit cards are tested on hp98x6, the two lists will
be merged.
function of two optional ROMs I put in the ROM software list. I don't know what should
be the preferred way to add optional ROMs, either as a plug-in card or as a ROM in
a sw list.
Management). I implemented a file server in Python for it. I will release it once this
PR is accepted.
Thanks.
--F.Ulivi