Skip to content
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.

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

Resurrect SD card operation #323

Closed
wants to merge 1 commit into from

Conversation

alees24
Copy link
Contributor

@alees24 alees24 commented Nov 5, 2024

Combining CIPO lines directly without regard to Chip Selects is risky, and in particular the line appspi_d1 from the flash is tri-stated without a pullup resistor on the FPGA board.

Modifications to the top_verilator implmentation to match the top_sonata implementation more closely, and paving the way for the introduction of the microSD card DPI model (already built and operational).

Without these changes the flash works only by virtue of the fact the the Sonata FPGA board carries a pullup on microSD card CIPO line, and the SD card is unusable because the flash CIPO does not. With these changes in place, and appropriate updates to the software that drives the microSD card and LCD, both the reading of SD card filing systems and the basic video player are operational once more.

@elliotb-lowrisc FPGA build remains timing clean (v2022.2), but it would be worth giving it the once over, please.

Combining CIPO lines directly without regard to Chip Selects
is risky, and in particular the line `appspi_d1` from the
flash is tri-stated without a pullup resistor on the FPGA
board.

Modifications to the top_verilator implementation to match
the top_sonata implementation more closely, and paving the
way for the introduction of the microSD card DPI model
(already built and operational).
@alees24 alees24 force-pushed the sdcard-resurrection branch from 42645c8 to 5544a67 Compare November 5, 2024 19:14
@alees24 alees24 added the bug Something isn't working label Nov 5, 2024
Copy link
Contributor

@GregAC GregAC left a comment

Choose a reason for hiding this comment

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

Thanks for spotting this @alees24 but I think I'd prefer we use the pinmux for this.

I can see the logic in doing this shared bus arrangement but two reasons to use the pinmux

  1. We end up having to implement a custom pinmux like arrangement for the incoming data lines
  2. Anything with access to the SPI instance will be able to access both flash and SD card. If we pinmux things you can come up with an arrangement where a compartment gets temporary access to the SPI capability but not access to the pinmux itself so can ensure it only gets access to flash or the SD card but not both.

@alees24
Copy link
Contributor Author

alees24 commented Nov 5, 2024

I'm fine with CIPO selection being returned to the pinmux; it's only implemented in sonata_system presently because all of the signals to/from that SPI controller have been removed from the pinmux.

@GregAC
Copy link
Contributor

GregAC commented Nov 5, 2024

On point 2. I imagining most uses of the pinmux will see things get setup immediately after RTOS boot so for this SPI you'd mux to the SD card and leave the flash non-connected. This then gives the pleasing property that the software cannot modify the boot medium.

@elliotb-lowrisc
Copy link
Contributor

For what it's worth, the current changes don't seem to have caused a drop in overall QoR when building with Vivado 2021.1

@marnovandermaas
Copy link
Contributor

Closing this as it's been solved by #326

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants