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

systembus read access.. #880

Closed
InspireSemi opened this issue Jul 8, 2023 · 24 comments
Closed

systembus read access.. #880

InspireSemi opened this issue Jul 8, 2023 · 24 comments

Comments

@InspireSemi
Copy link

Why is it assumed that on a write of the address for systembus read it automagically happens?

From read_memory_bus_v1()

	if (info->bus_master_read_delay) {
		jtag_add_runtest(info->bus_master_read_delay, TAP_IDLE);
		if (jtag_execute_queue() != ERROR_OK) {
			LOG_ERROR("Failed to scan idle sequence");
			return ERROR_FAIL;
		}
	}

	/* First value has been read, and is waiting for us to issue a DMI read
	 * to get it. */

Would not a better solution be to poll on sbusy and tiemout if it does not go low?
There is even a api for this read_sbcs_nonbusy()

@timsifive
Copy link
Collaborator

Polling is slow, and usually unnecessary. Instead, the code attempts to learn how many run-test/idle cycles need to be inserted to avoid ever encountering a busy state. In doing so it might end up encountering busy for the first several attempts.

@InspireSemi
Copy link
Author

polling guarantees that you are actually reading the data when you should and not when you should not...

@JanMatCodasip
Copy link
Collaborator

JanMatCodasip commented Jul 11, 2023

As @timsifive pointed out, the current code in read_memory_bus_v1() is very optimistic: It tries to read the whole requested block, and only at the very end it detects if there's been "system bus busy" situation encountered. If so, it tries to re-read the unread part of the block (starting from the point when the "busy" popped up) with higher bus_master_read_delay. So the function should behave correctly - at least based on my visual check of the current code.

Still, I think there is some room for improvement in that function:

  • Currently the bus_master_read_delay is applied only once -- after the first read of the block. But not after the subsequent reads within that block.
  • The read could be split to smaller blocks so that the "system bus busy" state is detected sooner - especially important when reading large memory blocks.
  • The code of the function could be re-structured (and perhaps split) for better readability.

@InspireSemi Please let us know if you think we missed something, and there is something to be fixed. Thanks.

@InspireSemi
Copy link
Author

item 1 above is a good example of what I am talking about. You cannot guarantee that from read to read over a multi read transaction the delay will be the same. You can guarantee that if you poll you will get the data correctly at the correct time.

@timsifive
Copy link
Collaborator

In practice, this is much faster than always polling.

Do you have any problems with OpenOCD where this is a problem?

@InspireSemi
Copy link
Author

InspireSemi commented Jul 12, 2023 via email

@timsifive
Copy link
Collaborator

What issues? Can you share an openocd -d log?

@InspireSemi
Copy link
Author

There are a lot of issues when in Sim due to sim time being not the same as PC time.

@TommyMurphyTM1234
Copy link
Collaborator

There are a lot of issues when in Sim due to sim time being not the same as PC time.

As @timsifive suggested above, whenever there are issues with OpenOCD debugging, a good first step is to collect a verbose log using openocd -d3.

@MarcKarasek
Copy link

MarcKarasek commented Oct 24, 2023

I have found 1 issue with this read before sbbusy goes away. According to the spec if a read is in progress and another read is done then sbbusyerror will be set. By doign the read before checking that sbbusy is cleared you are triggering another read ( again according to the spec) this causes sbbusyerror to be set.

Set when the debugger attempts to read data
while a read is in progress, or when the debug-
ger initiates a new access while one is already in
progress (while sbbusy is set). It remains set until
it's explicitly cleared by the debugger.
While this eld is set, no more system bus accesses
can be initiated by the Debug Module.

On a read I do not see this getting cleared (per above) at the end of the read.

I and InspireSemi are the same person BTW

@TommyMurphyTM1234
Copy link
Collaborator

I have found 1 issue with this read before sbbusy goes away. According to the spec if a read is in progress and another read is done then sbbusyerror will be set. By doign the read before checking that sbbusy is cleared you are triggering another read ( again according to the spec) this causes sbbusyerror to be set.

Set when the debugger attempts to read data while a read is in progress, or when the debug- ger initiates a new access while one is already in progress (while sbbusy is set). It remains set until it's explicitly cleared by the debugger. While this eld is set, no more system bus accesses can be initiated by the Debug Module.

On a read I do not see this getting cleared (per above) at the end of the read.

Do you have an openocd -d3 log for this scenario?

@InspireSemi
Copy link
Author

InspireSemi commented Oct 24, 2023 via email

@InspireSemi
Copy link
Author

InspireSemi commented Oct 24, 2023 via email

@timsifive
Copy link
Collaborator

The highlighted sections are where it is writing first to 0x3a, 0x39 which will trigger a read as sbreadonaddr is set. This is followed by a read from 0x3C(data) prior to any polling/reading of 0x3C(sbcs). This triggers a second read while the first one is in progress. Which in turn causes the sbbusyerror in the sbcs to be set.

The key is what OpenOCD does next. It's OK to encounter sbbusyerror like you described. In that case the OpenOCD code should increase the delay (run-test/idle cycles) it puts between scans and try the same memory operation again. Your log is cut off when sbbusyerror is first encountered. (I think you're probably in read_sbcs_nonbusy(), where we wait for riscv_command_timeout_sec waiting for sbbusy to go low. Presumably you put that timeout quite high. It might be a bug that we do this loop, in the sense that it wastes time but will eventually do the correct thing.) Does OpenOCD not retry correctly? Can you let it run a little further and share that log?

Based on the timestamps in your log, your target is outrageously slow, which matches with it being a simulation. It will take a lot of time for OpenOCD to discover the correct timing. Instead of going through this learning process every time (assuming it works) you could change the OpenOCD source (riscv-013.c, init_target()) to give bus_master_read_delay (and presumably bus_master_write_delay) higher default values.

@InspireSemi
Copy link
Author

InspireSemi commented Oct 24, 2023 via email

@timsifive
Copy link
Collaborator

There's definitely some bug(s) when encountering sbbusyerror. I'm hacking spike to replicate the problem, and will then look into exactly what OpenOCD is/should be doing.

@InspireSemi
Copy link
Author

Did you find anything on this?

@timsifive
Copy link
Collaborator

Yes. AFAICT handling sbbusyerror worked poorly if at all. I'm making progress, but don't have something that handles all the cases yet

@InspireSemi
Copy link
Author

InspireSemi commented Nov 6, 2023 via email

@InspireSemi
Copy link
Author

Will this be sent to the other openocd?

@timsifive
Copy link
Collaborator

Yes, eventually, but don't hold your breath. We have years of backlog and virtually no time is spent working on it.

@en-sc
Copy link
Collaborator

en-sc commented Jan 22, 2024

@InspireSemi, AFAIU the issue is resolved. I would like to close it.

@InspireSemi
Copy link
Author

InspireSemi commented Jan 22, 2024 via email

@InspireSemi
Copy link
Author

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

No branches or pull requests

6 participants