Skip to content

multibus: fixed regression on isbc202#13594

Merged
rb6502 merged 2 commits intomamedev:masterfrom
fulivi:mds2_dev4
Jun 4, 2025
Merged

multibus: fixed regression on isbc202#13594
rb6502 merged 2 commits intomamedev:masterfrom
fulivi:mds2_dev4

Conversation

@fulivi
Copy link
Copy Markdown
Contributor

@fulivi fulivi commented Apr 15, 2025

Hi,
this commit fixes a regression on isbc202 emulation caused by cf67936, which removed ownership of multibus slots by multibus bus. By doing so it removed the propagation of clock down to cards.
iSBC202 was affected because it (probably) is the only multibus card in MAME that relies on clock to come from the bus.
Thanks.
--F.Ulivi

Copy link
Copy Markdown
Member

@cuavas cuavas left a comment

Choose a reason for hiding this comment

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

Do you think this is the best approach? Would it be better to change the slot constructor to allow a clock frequency to be supplied rather than always using zero?

Comment thread src/mame/intel/imds2.cpp Outdated

// Y1 oscillator of IPC board: 19.6608 MHz
#define IPC_XTAL_Y1 19.6608_MHz_XTAL
constexpr auto IPC_XTAL_Y1 = 19.6608_MHz_XTAL;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you turn this into a global variable, it needs to have a constant address across an invocation and hence increases static data size. If it’s only used in a single function, you can make it a local variable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aren't constexpr variables implicitly inline since C++17?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only constexpr static data members are implicitly inline. A scope constexpr variable implicitly static.

But if the address of a static inline data member leaks out of a function (e.g. by passing it to logerror or something else that implicitly passes by reference), the compiler will instantiate an out-of-line copy, and the linker needs to coalesce the out-of-line copies at link time. It doesn’t actually prevent the increase in static data size.

@fulivi
Copy link
Copy Markdown
Contributor Author

fulivi commented Apr 26, 2025

Better like this?

@fulivi
Copy link
Copy Markdown
Contributor Author

fulivi commented May 18, 2025

@cuavas May I ask you if you think this can be merged, please? Thanks

@rb6502 rb6502 merged commit b53d568 into mamedev:master Jun 4, 2025
6 checks passed
@cuavas
Copy link
Copy Markdown
Member

cuavas commented Jun 30, 2025

Well there are two issues with it now:

  • The slot clock used at construction isn’t used, because it gets the bus clock at start anyway. It’s kind of pointless passing the clock value to slots.
  • You still can’t configure slots with different clock frequencies without a separate bus for each group (I don’t know if this can ever happen in real life for multibus, but e.g. PCI systems sometimes have a mixture of 33 MHz and 66 MHz slots).
  • If the clock frequency of the bus is changed after start, it doesn’t propagate to the slots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants