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

[FIX] Fix bcm pl011 UART #168

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

FelixSchladt
Copy link

I tried using the existing driver for data transmissions (with a hardware rng) and had the issue that without enabled FIFO, no interrupts are received.

As the note states, enabling the FIFO seems to cause some other trouble with the SerialServer.
I think this should be discussed, what makes the most sense and maybe a solution which fits all can be found.

@axel-h
Copy link
Member

axel-h commented Sep 4, 2023

Just to clarify, it seem to me that when you enable the receive timeout interrupt and the FIFO, then the serial server should work again. You get an interrupt either when the FIFO has a certain level or - due to the receive timeout - when there is data in the FIFO, but no additional data is received for "a 32-bit period". So we should be able to merge this without breaking thins. I wonder if should still make the interrupt behavior configurable via the flags field of ps_chardevice_t, so we can keep the old behavior by default and enable the new behavior only if explicitly selected.

Copy link
Member

@axel-h axel-h left a comment

Choose a reason for hiding this comment

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

Basically I am fine with this, and I think this is the change what we discussed back then. I can't test this in detail at the moment - anybody?

Please rebase.

@axel-h axel-h requested a review from Indanz February 23, 2024 21:25
Copy link
Contributor

@Indanz Indanz left a comment

Choose a reason for hiding this comment

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

All look like reasonable fixes.

As the old code is known not to work and this probably does, I think we should just merge this.

@Indanz Indanz added the hw-test sel4test hardware builds + runs label Feb 24, 2024
@Indanz
Copy link
Contributor

Indanz commented Feb 24, 2024

The Gitlint and License Checks are stuck, but I can't re-trigger them. I also don't have an option to bring the branch up-to-date, so I guess a manual merge needs to be done.

@Indanz Indanz added hw-build all sel4test hardware builds and removed hw-test sel4test hardware builds + runs labels Feb 24, 2024
@FelixSchladt
Copy link
Author

I will try to rebase it on monday

Using the PL011 as uart for data did not work:
-Enabling FIFO as no irqs are raised otherwise
-Added required interrupts
-Fixing incorrect irq disable
Signed-off-by: Felix Schladt <[email protected]>
Signed-off-by: Felix Schladt <[email protected]>
@Indanz
Copy link
Contributor

Indanz commented Feb 26, 2024

So I looked at the Broadcom UART documentation, but it doesn't explain what the receive time-out does. Does it trigger all the time or does it only trigger after receiving a character and nothing else arrives?

@FelixSchladt
Copy link
Author

So I looked at the Broadcom UART documentation, but it doesn't explain what the receive time-out does. Does it trigger all the time or does it only trigger after receiving a character and nothing else arrives?

As far as I understand, the receive timeout only triggers after a certain period of no incoming data

@Indanz
Copy link
Contributor

Indanz commented Feb 26, 2024

As far as I understand, the receive timeout only triggers after a certain period of no incoming data

Does that mean you get an interrupt all the time? Because that would make it "work", but not in the right way.

@@ -227,14 +230,14 @@ static int pl011_uart_configure(ps_chardevice_t *dev)
*
*/
// Enable FIFO
//pl011_uart_enable_fifo(dev);
pl011_uart_enable_fifo(dev);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced that the problem mentioned in the comment is properly addressed. I am afraid that enabling FIFOs is just a work-around for an unfixed bug, probably caused by the original's code assumptions of the current state of the UART or initialisation.

Have you tried fully configuring the UART and setting all registers, and clearing any pending IRQs and errors at init?

Copy link
Author

Choose a reason for hiding this comment

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

I tested the current state again today and it is working fine.

Sending single bytes is not an issue.
For testing I did setup a simple uart echo server which is interrupt triggered.

2024-03-15_13-43

Copy link
Contributor

Choose a reason for hiding this comment

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

Please print something out at every receive interrupt and check that you don't get any when no input is given.

Or keep a counter and print out the counter value too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hw-build all sel4test hardware builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants