spi_sdcard: add CMD18 - CMD_READ_MULTIPLE_BLOCK

[holub]

SPI_SDCARD:

• implement state changes closer to spec
• CMD18 - CMD_READ_MULTIPLE_BLOCK

[rb6502]

Thanks! At a glance this looks like a great cleanup and addition, and I'll do a more in-depth review when I have some time. Are you working on something either inside or outside of MAME that uses CMD_READ_MULTIPLE_BLOCK?

[holub]

Hi, Sure take your time. I have one concern about with this implementation. I changed idle response from 0..0 to 1..1 which per my understanding is correct. Ideally that needs to be tested in existing drivers which use SPI. I'm trying to implement tsconf zx-evolution driver in mame. READ_MULTI was essential. Later I will add WRITE as well when reach this point. Just tring to minimize pressure on reviewers. Let me know if you have any questions regarding this pr.

…

On Tue, Nov 30, 2021, 9:03 PM R. Belmont ***@***.***> wrote: Thanks! At a glance this looks like a great cleanup and addition, and I'll do a more in-depth review when I have some time. Are you working on something either inside or outside of MAME that uses CMD_READ_MULTIPLE_BLOCK? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8913 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOGK6QZRTUUY54CZ2GQ5DUOV65LANCNFSM5JDHXBHA> .

[cuavas]

Why have you added the m_cmd parameter to do_command in the first place? It’s an instance member function, so the instance data member m_cmd is accessible directly. As it’s an array, it’s implicitly passed as a pointer, so it’s not making a local copy of the command to pass to the function. It just seems like a roundabout and error-prone way of accessing a member.

[cuavas]

The “checklist review” stuff I was looking at has all been addressed. I’ll leave it to @rb6502 to review the actual logic here.

[holub]

The “checklist review” stuff I was looking at has all been addressed. I’ll leave it to @rb6502 to review the actual logic here.

Thank your rapid responses and patience!

[rb6502]

Hey @holub, was the change to the CMD8 response intentional? It should be valid for SD version 2 and later - does the Spectrum device assume version 1 and a different response? Anyway, I've merged your changes and reverted the CMD8 response, because the different response was confusing the firmware of the Apple II SD.

[holub]

Hey @holub, was the change to the CMD8 response intentional? It should be valid for SD version 2 and later - does the Spectrum device assume version 1 and a different response? Anyway, I've merged your changes and reverted the CMD8 response, because the different response was confusing the firmware of the Apple II SD.

Interesting case indeed. Ref (4.3.13 Send Interface Condition Command (CMD8)) respond with voltage info. Weird Apple ll just echos back. I'll take a look what can be missed and where.
UPDATED: It's opposite case. Looks like my fault. Sorry.

[rb6502]

You can see what the Apple II firmware's doing at line 249 here:
https://github.com/freitz85/AppleIISd/blob/master/Firmware/src/AppleIISd.s

It wants the first response byte to be 0x01, and it wants R33 to contain 0xAA for the card to pass as version 2.

[holub]

Appreciate your help. I've just checked: Evo perfectly works with voltage response. That was hardcoded for early experiments and unfortunately missed when I reviewed this large refactoring of the code.

…

On Wed, Dec 1, 2021, 9:00 PM R. Belmont ***@***.***> wrote: You can see what the Apple II firmware's doing at line 249 here: https://github.com/freitz85/AppleIISd/blob/master/Firmware/src/AppleIISd.s It wants the first response byte to be 0x01, and it wants R33 to contain 0xAA for the card to pass as version 2. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8913 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOGK56P32KSYB6OHNHAILUO3HKRANCNFSM5JDHXBHA> .