UART in sabrelite machine simulation doesn't work with VxWorks 7

[Bill Paul]

The imx_serial.c driver currently implements only partial support for the i.MX6 UART hardware. (I understand it's a work in progress and that's fine.) dIn particular, it does not implement support for the Transmit Complete Interrupt Enable bit in the UCR4 register. The VxWorks 7 i.MX6 serial driver depends on the behavior of this bit in actual hardware in order to send characters through the UART correctly. The result is that with the current machine model, VxWorks will boot and run in QEMU but it's unable to print any characters to the console serial port.

I have produced a small patch for the imx_serial.c module to make it nominally functional with VxWorks 7. It works well enough to allow the boot banner to appear and for the user to interact with the target shell.

I'm not submitting this as a patch to the development list as I'm not fully certain it complies with the hardware spec and doesn't break any other functionality. I would prefer if the maintainer (or someone) reviewed it for any issues/refinements first.

I'm attaching the patch to this bug report. A copy can also be obtained from:

http://people.freebsd.org/~wpaul/qemu/imx_serial.zip

This patch was generated against QEMU 2.11.0 but also works with QEMU 2.11.1.

My host environment is FreeBSD/amd64 11.1-RELEASE with QEMU 2.11.0/2.11.11 built from source.

[Peter Maydell (pmaydell)]

Hi. Thanks for this patch. I've had a quick look at it against the imx datasheet, and here are my comments:

* Firstly, we can't do anything with this patch without a Signed-off-by: line from you. In QEMU's process this is how people submitting code state that you're legally OK to contribute the code under QEMU's license and for it to go into QEMU.

 * Secondly, it would be very helpful if you could send patches as a simple patch format, rather than as a zipfile, and to the QEMU mailing list. https://wiki.qemu.org/Contribute/SubmitAPatch has our guidelines on this.

 * Simply adding a new VMSTATE_UINT32() field will break migration. It's better to put the new field into its own vmstate subsection so that this doesn't happen; see docs/devel/migration.rst. If we don't care about cross-version migration we could just bump the version_id/minimum_version_id fields.

 * If you run scripts/checkpatch.pl over your patch it should warn you about minor coding style issues. For instance we prefer all if() statements to use {}, even if there's only one line in them.

 * Your change to imx_update() does this:
+ if (s->ucr4 & UCR4_TXEN)
+ flags |= USR1_TRDY;

but that isn't what the spec says UCR4_TXEN does; it says we raise an interrupt only if UCR4_TXEN and USR2_TXDC are both high.

 * the imx_update() function is already rather confused in how it handles the 'flags' variable, and this change extends that confusion. The function is trying to treat 'flags' as a single set of interrupt flag bits, but the device doesn't actually have a single unified set of interrupt flags like that, they're spread over UTS and USR1 and USR2. The code as it is looks odd -- should USR1.TXMPTYEN == 0 really suppress USR1_TRDY interrupts ? I suspect this is a bug, and we should also clean things up to make 'flags' be a bool.

[Bill Paul (wpaul)]

As I said before:

"I'm not submitting this as a patch to the development list as I'm not fully certain it complies with the hardware spec and doesn't break any other functionality."

What I'm trying to say here is that while I may have been able to cobble together a hack to make the UART nominally compatible with VxWorks, I do not understand the hardware or QEMU well enough to really fix this the right way. Even with my hack, every once in a while when printing lots of data on the console, the output from the UART will stall unless I press a key on the serial port, and I don't know why it does that. I did try to investigate it a little but wasn't able to make much progress. (My suspicion is that it has something to do with the fact that the imx_serial module doesn't implement FIFO support, but even if that's true I don't know how to fix it.)

Even so, I figured it was still worth it to attach my changes to the bug report so that somebody who is better at this than me could use it as a starting point, and so that anybody else who might want to experiment with VxWorks using the QEMU sabrelite machine model wouldn't be totally stuck like I was.

In short, the changes I made are good enough for my own needs (the output stalls don't happen often enough to really bother me), but they're not a fully debugged fix. That's why I filed a bug report instead of just submitting a patch in the first place: I wanted somebody sexier than me to create a fully debugged fix.

[Peter Maydell (pmaydell)]

That's fine; Andrey Smirnov has taken your patch as a basis for a more cleaned-up set of changes:
http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04608.html
http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04609.html
What we would like from you is a Signed-off-by: line to say that you're happy for us to do that, please. (If you have a chance to test that it works for you that would also be great.)

[Peter Maydell (pmaydell)]

Now fixed in git master, should be in 2.12.