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

update: bump jh71xx-hal to v0.3.0 #740

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

rmsyn
Copy link
Contributor

@rmsyn rmsyn commented Apr 17, 2024

Updates the jh71xx-hal dependency to v0.3.0, and drops the direct dependency on jh71xx-pac.

Removes unused code from the starfive/visionfive2 mainboard and soc/starfive/jh7110 modules.

Reverts the changes from 646abdd. The changes make the vf2-loader step of make run stall forever (until retries are exhausted). Reverting the changes restores expected behavior.

If the changes in 646abdd are fixed in another PR, I'll rebase on those changes here.

This reverts commit 646abdd.

Signed-off-by: rmsyn <rmsynchls@gmail.com>
Signed-off-by: rmsyn <rmsynchls@gmail.com>
Removes unused DDR and PLL initialization code moved into `jh71xx-hal`.

Signed-off-by: rmsyn <rmsynchls@gmail.com>
Removes the unused `soc::starfive::jh7110::pac` module.

Signed-off-by: rmsyn <rmsynchls@gmail.com>
@orangecms
Copy link
Contributor

orangecms commented Apr 17, 2024

I can confirm that the loader step may sometimes hang.
That is, it happens with USB serial converters other than FTDI.
However, there is no clear indicator that this is related to the header rework. Looking at multiple binaries, before and after the change, they are the same.

There is likely an issue with vf2-loader / the xmodem implementation.
It is known that the xmodem implementation in the JH7110 itself is wonky.

The UART behaves funky in other ways, on that note. I tried using the same UART init in the main stage that we now have in bt0, but got no more output then.

Regarding moving out the DDR init code:
That loses some flexibility within oreboot. To work on that code, one would need to clone the HAL repo, point to it, and file PRs to both repos. I would thus suggest keeping the code here so that it can be reused with other SoCs, like the upcoming JH81xx. So far, the JH7100 had been quite similar.

@rmsyn
Copy link
Contributor Author

rmsyn commented Apr 18, 2024

That loses some flexibility within oreboot.

If you look at the jh71xx-hal code, all of the functions for DDR bring-up are still callable individually. Meaning, instead of just calling Ddr::init, one can still call all the inner functions to do the bring-up manually.

You can also still do everything manually via the re-exported jh71xx-pac:

use jh71xx_hal::pac;

// ... do manual PAC stuff

So, no, you don't need to "point" at anything.

@rmsyn
Copy link
Contributor Author

rmsyn commented Apr 18, 2024

However, there is no clear indicator that this is related to the header rework.

Except there is! I did a git-bisect on the recent commits, and wouldn't you know removing the header rework actually allows reliably flashing to the device in UART mode!

Seems fairly weird for you to claim that all of the sudden vf2-loader is buggy, when it hasn't changed. Especially when reverting the header rework you did, which changed a lot, fixes things.

Signed-off-by: rmsyn <rmsynchls@gmail.com>
@rmsyn
Copy link
Contributor Author

rmsyn commented Apr 18, 2024

That loses some flexibility within oreboot.

Here is a commit showing doing each part of Ddr::init manually: ff8f439

To work on that code, one would need to clone the HAL repo, point to it, and file PRs to both repos.

This is no different than the jh71xx-hal code that is already present in oreboot. All the equivalent DDR init functions are accessible via the public API. If there is one that is protected/private, let me know, and I can expose it as public with a "if you know what you're doing" warning in the docs.

Again, if there is still reusable code from JH7110 for JH81xx, great.

I also plan on doing a HAL for that once JH81xx hardware and docs are available.

If you don't want to have a HAL as a dependency, then please rip out all of my contributions, and this will be my last interaction with you.

I submitted this code out of appreciation for the project, and the help it provided in getting bare-metal code running on the VisionFive2. If I'm not wanted here, I have no intentions of out-staying my welcome.

@orangecms
Copy link
Contributor

orangecms commented Apr 18, 2024

However, there is no clear indicator that this is related to the header rework.

Except there is! I did a git-bisect on the recent commits, and wouldn't you know removing the header rework actually allows reliably flashing to the device in UART mode!

I would like to understand and dig into this. Here is a repo containing both implementations and comparing their results:
https://github.com/orangecms/vf2-header

Could you please provide an example binary that creates different results?

Seems fairly weird for you to claim that all of the sudden vf2-loader is buggy, when it hasn't changed. Especially when reverting the header rework you did, which changed a lot, fixes things.

I experienced that before the rework already many times, which you can also see in the live stream recordings I did last year, archived on https://youtube.com/@cyrevolt

The funkiness has been reported by multiple people, which is why e.g. Heinrich Schuchardt worked on his own (forked) tool.
Otherwise, we could just use tio or any other one implementing xmodem instead. I only got it to barely work. See also:

@orangecms
Copy link
Contributor

That loses some flexibility within oreboot.

Here is a commit showing doing each part of Ddr::init manually: ff8f439

To work on that code, one would need to clone the HAL repo, point to it, and file PRs to both repos.

This is no different than the jh71xx-hal code that is already present in oreboot. All the equivalent DDR init functions are accessible via the public API. If there is one that is protected/private, let me know, and I can expose it as public with a "if you know what you're doing" warning in the docs.

There are two things that would require more exposure:

  1. Adding support for 1GB of DRAM. U-Boot already has that now, and I hadn't had the time to add it yet.
  2. Switching to runtime configuration. The boards feature an I2C EEPROM telling the DRAM size. U-Boot uses that.

Again, if there is still reusable code from JH7110 for JH81xx, great.

I also plan on doing a HAL for that once JH81xx hardware and docs are available.

That is great! There are parts in SoCs that are reused across vendors. I am hoping that this will cause more flexibility at some point within the Rust ecosystem, so that libraries for peripheral blocks occur which are reexposed from HALs.

If you don't want to have a HAL as a dependency, then please rip out all of my contributions, and this will be my last interaction with you.

I submitted this code out of appreciation for the project, and the help it provided in getting bare-metal code running on the VisionFive2. If I'm not wanted here, I have no intentions of out-staying my welcome.

I really welcome your contributions. This is not about you or having a HAL or not, really about the specific concerns I am raising here. I'd be happy to have a discussion rather than just tossing things away. You have done great work that I know cost a lot of time and effort. I appreciate that a lot.

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.

None yet

2 participants