i2c memory devices (i2cmem.cpp) wrongly ACK all device IDs

[Paul-Arnold]

Hi, I've just added an I2C eeprom to a board driver and discovered that i2cmem.cpp wrongly ACKs every device id and not just its own :( It appears wrong as follows - On 8th rising clock from start it latches the r/w bit On next falling clock it sets the data output to 0 to ACK the rx but it hasn't yet validated the device ID 9th rising clock it validates the device id but i2c master has read the ACK.

This causes other issues because if it's a read command that's issued to a device id supported elsewhere the data output remains set to 0 so when the outputs from both devices are combined the result is data value of 0 :(

Only just found this so not sure what the fix is yet. Maybe it should validate the id and process any rx'd byte when the clock goes low so it can then either ACK or NAK ? Or, as the memory read function sets/clears data out on rising clock (see below) we just do the ACK at the current point the device id is checked ?

In addition to this, when the device is being read the new data bit isn't made available until the clock goes high which isn't strictly correct (it should be valid a few ns before clock high), although this probably doesn't cause any problems.

There's also a bit in STATE_DEVSEL which sets data out to 0 if it's a software reset but I think data is already 0 at that point from the ACK so I don't really understand this part.

Anyone else come across this or do all other board drivers only have one i2c slave device or am I wrong ?

Thanks Paul

[smf-]

Where can this behavior be tested?

Is the code in MAME already?

On 15/04/2022 10:36, Paul-Arnold wrote:

Hi, I've just added an I2C eeprom to a board driver and discovered that i2cmem.cpp wrongly ACKs every device id and not just its own :( It appears wrong as follows - On 8th rising clock from start it latches the r/w bit On next falling clock it sets the data output to 0 to ACK the rx but it hasn't yet validated the device ID 9th rising clock it validates the device id.

This causes other issues because if it's a read command that's issued to a device id supported elsewhere the data output remains set to 0 so when the outputs from both devices are combined the result is data value of 0 :(

Only just found this so not sure what the fix is yet. Maybe it should validate the id and process any rx'd byte when the clock goes low so it can then either ACK or NAK ?

In addition to this, when the device is being read the new data bit isn't made available until the clock goes high which isn't strictly correct (it should be valid a few ns before clock high), although this probably doesn't cause any problems.

There's also a bit in STATE_DEVSEL which sets data out to 0 if it's a software reset but I think data is already 0 at that point from the ACK so I don't really understand this part.

Anyone else come across this or do all other board drivers only have one i2c slave device or am I wrong ?

Thanks Paul

— Reply to this email directly, view it on GitHub https://github.com/mamedev/mame/issues/9572, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGVYYXKX2GDM742F6SWGUDVFE2BTANCNFSM5TQBL3OQ. You are receiving this because you are subscribed to this thread.Message ID: @.***>

[Paul-Arnold]

I can't find anything already in MAME to prove this. It needs a driver which has at least two I2C devices, at least one of them which uses i2cmem. I only found it because the driver I'm looking at has two i2c devices and even though I'd not implemented the 2nd device I was still receiving an ACK when I addressed it. I traced the incorrect ACK to i2cmem.cpp

[smf-]

What is the driver you are looking at?

On 19/04/2022 16:25, Paul-Arnold wrote:

I can't find anything already in MAME to prove this. It needs a driver which has at least two I2C devices, at least one of them which uses i2cmem. I only found it because the driver I'm looking at has two i2c devices and even though I'd not implemented the 2nd device I was still receiving an ACK when I addressed it. I traced the incorrect ACK to i2cmem.cpp

— Reply to this email directly, view it on GitHub https://github.com/mamedev/mame/issues/9572#issuecomment-1102789424, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGVYYRL5S3K2AYEXCLAK6DVF3GANANCNFSM5TQBL3OQ. You are receiving this because you commented.Message ID: @.***>

[Paul-Arnold]

I'm looking at magicard.cpp but that wasn't my original interest. I was writing emulation for the 66470 graphics controller for the bfm_swp.cpp driver. I saw magicard.cpp also needs that chip and thought it'd be a good test to prove my implementation worked in both drivers.

What's in magicard at the moment is really just a skeleton. There is a lot missing from there, there's cpu peripherals which aren't currently supported but it looked like it might be simple to get working. Seems I'm wrong :(

What I've got at the moment is nowhere near being in a state where I could submit anything :(

I really just asked the question about I2CMEM as I was surprised nobody had seen the problem before. Having had another good search through MAME I can't find any drivers which have more than one i2c device so that probably explains why it's gone un-noticed.

[Pernod70]

In aa310.cpp there are two drivers that use two i2c devices , aa500 and aa4. In aa4 the i2c devices are pcf8583_device (pcf8583.cpp) and acorn_bmu_device (acorn_bmu.cpp), both i2c implementations are derived from i2cmem.cpp and am not aware of any issues with them both being on the same i2c bus.

[Paul-Arnold]

Thanks Pernod70, must admit I'd concentrated on i2cmem devices.

Looking at the code for those devices it does look as if there should be a problem. They do seem to ACK the device address even when they're not selected. I also don't see how they release the SDA line which is what's causing me a problem. Unfortunately I can't run either of those, AA4 requires riscos311_apps.chd and AA500 wants a500_riscos206_x.ic2x which I can't find on a site I can get to from here.

I notice in aa310.cpp the two devices are combined as follows:

m_ioc->gpio_r<0>().set("i2cmem", FUNC(pcf8583_device::sda_r));
m_ioc->gpio_r<0>().append("bmu", FUNC(acorn_bmu_device::sda_r));

In the above, when the port is read what actually happens ? Obviously both device sda_r functions are called but they are then combined in some way I need to search how that's done - still finding my round mame src atm.

edit: now have riscos311_apps.chd and there does appear to be conflict. A snippet of debug for 8583 and bmu device... After the 8th rising clock the next falling edge gives 8583 clk 0 8583 ack (sda set 0 for next read) bmu clk 0 bmu ack (sda set 0 for next read)

So both devices are setting sda output to 0 to ACK...although it's wrong it's not really a problem unless you're checking to see which devices are fitted.

The next rising edge is where the device id is checked...and the ack/nak read 8583 clk 1 8583 not selected - idle bmu clk 1 bmu selected cpu read 8583 sdar 0 cpu read acorn bmu sdar 0

From here the only device which should set SDA to 0 is the bmu...but... next falling clock nothing should change 8583 clk 0 bmu clk 0 next rising edge we read data bit 8583 clk 1 bmu clk 1 cpu read 8583 sdar 0 cpu read acorn bmu sdar 1

At this point the 8583 which is supposed to be idle has return 0 and the bmu return 1. The two combined will give 0. The above continues with the 8583 sda output remaining low until we hit a stop bit when sda is set to 1 for both devices.

[cuavas]

In the above, when the port is read what actually happens ? Obviously both device sda_r functions are called but they are then combined in some way I need to search how that's done - still finding my round mame src atm.

It ORs them together for historical reasons: https://github.com/mamedev/mame/blob/mame0242/src/emu/devcb.h#L905

This is obviously not what happens in real life with multiple I2C devices – the SDA and SCL lines have passive pull-ups and the devices only have active low drivers to pull the lines down. If that code “works”, it isn’t working for the right reasons.

[Paul-Arnold]

The code works because the i2c slave devices are wrong and the inactive device sets sda low instead of high which means combining them with OR works. I need to fix i2cmem.cpp for my driver so I'll look at that - probably validate the device id on the falling edge of clock 8 ? Apart from the i2c slaves mentioned above which are based on i2cmem I guess there are other i2c devices too. What happens about those ? Who finds and fixes those ? Me ? And once they're fixed there's the risk that things get broken such as the aa310 driver so those would need finding and then fixing and testing too. There may be other cases which get broken too.

How does fixing things like this usually work ? Is it down to the whoever finds a problem and fixes it to find all other cases and uses and make sure they're not broken due to the fix ?

[angelosa]

I'm looking at magicard.cpp but that wasn't my original interest. I was writing emulation for the 66470 graphics controller for the bfm_swp.cpp driver. I saw magicard.cpp also needs that chip and thought it'd be a good test to prove my implementation worked in both drivers.

What's in magicard at the moment is really just a skeleton. There is a lot missing from there, there's cpu peripherals which aren't currently supported but it looked like it might be simple to get working. Seems I'm wrong :(

Magic Card is a Philips CD-i derivative. Most games on that driver doesn't do anything because they lacks key features from SCC68070 and of course the video drawing is just a stub cfr. https://github.com/mamedev/mame/blob/110ecc9ec5dafe870fb8f05cf86d83f582b37b9b/src/mame/drivers/magicard.cpp#L11

I'm also a bit lost, that HW has one i2c at best?

[Paul-Arnold]

Hi Angelosa, The hardware has an I2C serial eeprom and the PIC16C54/F84 is also a bit-bashed I2C device. In addition to those the software also access another i2c address but I've not managed to workout what device that is. I've added 68070 i2c support as best I can based on the very poor 68070 document I've got - I've been unable to find any application notes for that device :( Magicarde now boots and runs the intro sequences. Puzzleme is playable but not 100% right yet. I've exchanged a few emails with Roberto Fresca to try and get more info on the board or at least photos of it. I realise you also worked on the existing driver. Seems info on this board/machine is scarce.

[smf-]

Pretty much, otherwise at best you break things that happened to work before and at worse you may introduce new bugs.

Unless people offer to help of course.

On 20/04/2022 22:46, Paul-Arnold wrote:

How does fixing things like this usually work ? Is it down to the whoever finds a problem and fixes it to find all other cases and uses and make sure they're not broken due to the fix ?Message ID: @.***>

[angelosa]

Ok so start off with a PR targeting 66470 only. I doubt it will hurt and I have no hard feeling about the existing code.

As for the rest, not having the concrete implementation at hand I'm just speculating that you're sharing the i2c bus between the internal 68070 (which I don't trust either) and the external pic, which if correct may need good sync to work right. You may try with a config.set_perfect_quantum(m_maincpu); declared in machine_config fn where the pic gets installed to quickly rule that out, otherwise handshaking is wrong and need the code at hand to further elaborate.

[Paul-Arnold]

The 68070 is the master i2c always, the pic and eeprom slaves. Comms between the 68070 and pic looks good (after I fixed i2cmem.cpp), what is sent by the cpu is received by the pic and vice versa, at least for the bytes I've analysed. I can do a PR for 66470, will need to do a lot of tidying first but not sure what it achieves atm.

The 68070 i2c is my implementation so I'm not claiming it's 100% correct but it certainly seems to work and reads/writes eeprom and the pic OK. 68070 datasheet is appalling when it comes to I2C, doesn't even tell you how to generate start, stop and re-start bits :(

As i2cmem.cpp and derived devices are wrong it's probably more important to fix those first ? Or do you not think i2cmem.cpp is wrong ?

[cuavas]

Probably a good idea to bite the bullet and start with i2cmem.cpp. Systems can be tested for regressions and fixed.

Most of the I2C code in MAME is abysmal. There are even some I2C masters that just pretend everything happens instantaneously so they have no chance of working with software slaves.

[Paul-Arnold]

I was planning on doing i2cmem and the devices used by aa310. Will fix aa310 too as we know for sure that gets broken. Will sort sometime over next few days.

[Paul-Arnold]

I will check all files using read_sda and sda_r and fix any which are broken. Will do a PR today or tomorrow.

[Paul-Arnold]

First the good, The only driver I've found which is broken by the recent fix is accomm which is flagged as MACHINE_NOT_WORKING. I can correct the I2C part and do a PR for that.

Device SAA5240 based on I2CMEM has the same ACK problem but easy to fix. All drivers using this are flagged as MACHINE_NOT_WORKING

Now the bad There are several I2C slaves which appear to have the same problem of always ACKing although none of them are used in drivers which have multiple slaves. It doesn't look like these are based on i2cmem.

PCF8593 looks completely broken. After startup it will ACK everything it's sent. If a read is performed the sda line is left in the state of the last bit read so it's pot luck whether or not following accesses get ACK'd or NAK'd. Although this is only a rtc it uses file.read/file/write to support a nv file - why ?

MAS3507d used in ksys573. A complicated device. It does attempt to ACK/NAK various things, doesn't appear to NAK invalid device id. Will need studying further to be certain any changes don't break its operation.

x76f100 device id is replaced by a command/device-id byte so effectively has multiple device id's. No validation on the commands although there is a 'to-do' comment in there. Again, would need further studying to be sure of any changes.

ZS01 used in system 573. Apparently high level emulation/simulation of a PIC. No device-id validation instead reads several bytes into a buffer and validates using crc. Only ever check bit 0 of device-id for r/w. I've no idea whether the device-id can change or not - maybe that's why only bit 0 is checked for r/w and rest ignore. Would need further investigation.

In addition to the problem of ACKing all device-id's almost all of the above appear to change SDA while SCL is high :(

@cuavas , what would you like done on the above ? accomm.cpp and saa5240.cpp are simple enough to do the rest will take longer.

[cuavas]

@Paul-Arnold the problems predate your involvement, so I’m not to concerned whether it’s you who ultimately fixes them or someone else. I’d be happy if you just opened a PR with FIXME comments to the source for the issues you’ve identified, and updated this GitHub issue to track them. If you want to fix some of the easier ones yourself, that’s great, but don’t feel under pressure to do it.

[MooglyGuy]

@Paul-Arnold, how are you searching for documentation on the 68070? Searching for "Philips SCC68070", minus the quotes, on Google turns up multiple valid results on the first page. There's a 74-page manual from "Philips Components" dated November 1990 and a 102-page manual from "Philips Semiconductors" dated April 1993. There are some inaccuracies and inconsistencies between the two, but it should be sufficient to get the on-board I2C up and running.

@angelosa @Paul-Arnold On CD-i consoles, there were usually two 66470 VSC chips running in master/slave mode, with one managing one video plane and another managing a second video plane. Typically, the two were paired as the frontend to a single Matsushita MN66460B "VSR", or a single Motorola GSC38TG307 "VSD" backend, which performed CLUT/DYUV/RGB555 decoding, region/transparency processing, and combined the incoming plane (or planes) plus a cursor into an output pixel stream that could drive a video DAC.

What replaces the VSR or VSD in Magicard is anyone's guess, and if the boards really did only have one 66470 VSC as stated by the PCB layouts, the hardware might only have a passing resemblance to CD-i functionality under the hood. However, the MCD212 device (which is in src/mame/video/mcd212) was the combined "VDSC", which rolled up the functionality of two VSCs plus the backend functionality in one chip. In theory, using MCD212 plus existing 66470 documentation as a reference point, it should be possible to tease out what the backend chip actually does on the magicard.cpp games.

[Paul-Arnold]

@MooglyGuy , I implemented how I thought the I2C worked but I wasn’t 100% sure even though it seemed to work. I’ve now received a far better doc from CDIFan so need to do a minor tweak when I get chance.

As for the 66470, I’ve implemented enough of that to keep the magicard driver happy including ica/dca processing and some of the pixac stuff.

Still more to do. Took me a while to realise there’s a bit-bashed 1-wire interface to a ds2401 serial number chip. Also some other type of bit-bashed interface I’ve not figured. Not seen either of these mentioned before. At least one of the games uses an ELO touch screen too.

most of the games use a PIC for protection but we only have dumps for a small number of them.

I will tidy it all up and do a PR when I get chance.

We’ve side tracked this thread a bit so will sort this before worrying about magcard

[Paul-Arnold]

I've created PR #9648 for all I2C slave devices which either wrongly ACK all device-id's and not just their own, or change the state of the sda line while the clock is high or leave sda low while in the idle state.

I will fix device saa5240 and driver accomm later which is why they're not in this PR.

[smf-]

These aren't i2c devices.

On 28/04/2022 15:51, Paul-Arnold wrote:

x76f100 device id is replaced by a command/device-id byte so effectively has multiple device id's. No validation on the commands although there is a 'to-do' comment in there. Again, would need further studying to be sure of any changes.

ZS01 used in system 573. Apparently high level emulation/simulation of a PIC. No device-id validation instead reads several bytes into a buffer and validates using crc. Only ever check bit 0 of device-id for r/w. I've no idea whether the device-id can change or not - maybe that's why only bit 0 is checked for r/w and rest ignore. Would need further investigation.

Message ID: @.***>

[smf-]

It's more worth testing to make sure the i2cmem change has not introduced any regressions.

On 28/04/2022 17:14, Vas Crabb wrote:

@Paul-Arnold https://github.com/Paul-Arnold the problems predate your involvement, so I’m not to concerned whether it’s you who ultimately fixes them or someone else. I’d be happy if you just opened a PR with FIXME comments to the source for the issues you’ve identified, and updated this GitHub issue to track them. If you want to fix some of the easier ones yourself, that’s great, but don’t feel under pressure to do it.

— Reply to this email directly, view it on GitHub https://github.com/mamedev/mame/issues/9572#issuecomment-1112398575, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGVYYVHLOQFAZJE2J5OPN3VHK2NHANCNFSM5TQBL3OQ. You are receiving this because you commented.Message ID: @.***>

[smf-]

You can set the control register to various modes, some of which leave registers free for use as storage.

It doesn't implement the modes properly at all, but saving an nvram file is the correct behavior.

FWIW It's missing event counter mode entirely and if the clock was stopped in the nv file at startup then it shouldn't load the current date/time.

On 28/04/2022 15:51, Paul-Arnold wrote:

PCF8593 looks completely broken. After startup it will ACK everything it's sent. If a read is performed the sda line is left in the state of the last bit read so it's pot luck whether or not following accesses get ACK'd or NAK'd. Although this is only a rtc it uses file.read/file/write to support a nv file - why ?Message ID: @.***>

[Paul-Arnold]

@smf- thanks for your comments. I'd missed the fact those two devices are not I2C, got fooled on the xf device as the sda/scl use conditioning appeared the same :( Agree about regression. My ROMS directory had only approx 60 files in it so having to download romsets before testing so not a quick job.

[smf-]

There may be issues in those devices, they just aren't i2c. There might be i2c devices that are not using sda/scl names too.

Regression testing is indeed often the hardest part of any change.

On 29/04/2022 23:23, Paul-Arnold wrote:

@smf- https://github.com/smf- thanks for your comments. I'd missed the fact those two devices are I2C, got fooled on the xf device as the sda/scl use conditioning appeared the same :( Agree about regression. My ROMS directory had only approx 60 files in it so having to download romsets before testing so not a quick job.

— Reply to this email directly, view it on GitHub https://github.com/mamedev/mame/issues/9572#issuecomment-1113819085, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGVYYQSZDDLQPUJ5TXOKTLVHROPVANCNFSM5TQBL3OQ. You are receiving this because you were mentioned.Message ID: @.***>

[ghost]

yeah the fishing controller isn't emulated, in this case there's a (dumped) mcu on the cart and one on the controller to communicate with it.

a lot of these plug and play games had weird and wonderful peripherals that we're doing to have to simulate somehow, most aren't hooked up.

[cuavas]

@Paul-Arnold is this adequately addressed now?