Re: [Qemu-devel] [PATCH 2/2] char: i.MX: Add support for "TX complete" interrupt

[Bill Paul]

Of all the gin joints in all the towns in all the world, Andrey Smirnov had to 
walk into mine at 12:11 on Thursday 15 March 2018 and say:

> Add support for "TX complete"/TXDC interrupt generate by real HW since
> it is needed to support guests other than Linux.
> 
> Based on the patch by Bill Paul as found here:
> https://bugs.launchpad.net/qemu/+bug/1753314
> 
> Cc: address@hidden
> Cc: address@hidden
> Cc: Bill Paul <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Signed-off-by: Andrey Smirnov <address@hidden>
> ---
> 
> Bill:
> 
> I only tested this with i.MX7/Linux guest combo and am hoping you can
> take this and previous patch and give it a try on your VxWorks setup.

I tried it and it seems to work as well as my original patch did. There is one 
thing I noticed, which is that when printing a lot of output to the console, 
sometimes the output will stall until you press a key to generate an input 
interrupt, and then it resumes. This happens with my original patch too. My 
suspicion is that this has to do with the lack of emulation of the FIFO 
feature of the i.MX6 UART, but I'm not certain. (It's still much better than 
not having it work at all, so I didn't really mind this. :) ) All I know for 
sure is that the stalls don't happen on real hardware.

FYI, I uploaded a sample VxWorks image that you can use for testing. You can 
find it here:

http://people.freebsd.org/~wpaul/qemu/sabrelite

The file vxWorks is the kernel, which is really all you need. This is an SMP 
image so you need to run QEMU with the -smp 4 option. The uVxWorks image and 
.dtb file are used on a real board with U-Boot and the bootm command. The 
qemu_imx.sh script contains the options I use for testing. I usually do:

% qemu_imx.sh vxWorks

You can download all the files at once by grabbing:

http://people.freebsd.org/~wpaul/qemu/sabrelite/vxworks_test.zip

At the -> prompt, you can type the following things (among others):

-> i -- show running task info
-> vxbIoctlShow -- show VxBus device tree
-> vxbDevShow -- show VxBus device tree in a different format
-> vxbDrvShow -- show VxBus drivers
-> vxbIntShow -- show information about interrupt bindings
-> vmContextShow -- show MMU mappings
-> memShow -- show heap usage
-> ifconfig -- show network interfaces
-> routec "show" -- show network routing table
-> netstat "-a" -- show network socket info

The image also includes network support. You can use ifconfig to set the enet0 
interface's addresss like this:

-> ifconfig "enet0 10.0.2.15 netmask 255.255.255.0 up"

You should then be able to telnet to the VxWorks image from the host machine 
by doing:

% telnet localhost 10023

To exit the telnet session, type logout:

-> logout

NOTE: this only works if you've patched the imx_fec module to fix the 
interrupt vector bug that I also reported.

If you run vxbIoctlShow, which generates a lot of output, you should be able 
to see the stall condition I'm talking about. If you don't, then maybe it has 
something to do with me running QEMU on FreeBSD.
 
> Peter:
> 
> Bill is the author of original code, so I am not sure how to handle
> Signed-off-by from him. I'd like to add it to this patch, but since
> his original submission to launchpad didn't have one it is currently
> omitted.

My original goal was to report the bug and provide as much info as I could on 
how to maybe fix it, and let somebody else make a proper fix/patch.

-Bill
 
>  include/hw/char/imx_serial.h |  3 +++
>  hw/char/imx_serial.c         | 20 +++++++++++++++++---
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/char/imx_serial.h b/include/hw/char/imx_serial.h
> index baeec3183f..5b99cee7cf 100644
> --- a/include/hw/char/imx_serial.h
> +++ b/include/hw/char/imx_serial.h
> @@ -67,6 +67,8 @@
>  #define UCR2_RXEN       (1<<1)    /* Receiver enable */
>  #define UCR2_SRST       (1<<0)    /* Reset complete */
> 
> +#define UCR4_TCEN       BIT(3)    /* TX complete interrupt enable */
> +
>  #define UTS1_TXEMPTY    (1<<6)
>  #define UTS1_RXEMPTY    (1<<5)
>  #define UTS1_TXFULL     (1<<4)
> @@ -95,6 +97,7 @@ typedef struct IMXSerialState {
>      uint32_t ubmr;
>      uint32_t ubrc;
>      uint32_t ucr3;
> +    uint32_t ucr4;
> 
>      qemu_irq irq;
>      CharBackend chr;
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index d1e8586280..1e5540472b 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -37,8 +37,8 @@
> 
>  static const VMStateDescription vmstate_imx_serial = {
>      .name = TYPE_IMX_SERIAL,
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_INT32(readbuff, IMXSerialState),
>          VMSTATE_UINT32(usr1, IMXSerialState),
> @@ -50,6 +50,7 @@ static const VMStateDescription vmstate_imx_serial = {
>          VMSTATE_UINT32(ubmr, IMXSerialState),
>          VMSTATE_UINT32(ubrc, IMXSerialState),
>          VMSTATE_UINT32(ucr3, IMXSerialState),
> +        VMSTATE_UINT32(ucr4, IMXSerialState),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> @@ -71,6 +72,11 @@ static void imx_update(IMXSerialState *s)
>       * unfortunately.
>       */
>      mask = (s->ucr1 & UCR1_TXMPTYEN) ? USR2_TXFE : 0;
> +    /*
> +     * TCEN and TXDC are both bit 3
> +     */
> +    mask |= s->ucr4 & UCR4_TCEN;
> +
>      usr2 = s->usr2 & mask;
> 
>      qemu_set_irq(s->irq, usr1 || usr2);
> @@ -163,6 +169,8 @@ static uint64_t imx_serial_read(void *opaque, hwaddr
> offset, return s->ucr3;
> 
>      case 0x23: /* UCR4 */
> +        return s->ucr4;
> +
>      case 0x29: /* BRM Incremental */
>          return 0x0; /* TODO */
> 
> @@ -191,8 +199,10 @@ static void imx_serial_write(void *opaque, hwaddr
> offset, * qemu_chr_fe_write and background I/O callbacks */
>              qemu_chr_fe_write_all(&s->chr, &ch, 1);
>              s->usr1 &= ~USR1_TRDY;
> +            s->usr2 &= ~USR2_TXDC;
>              imx_update(s);
>              s->usr1 |= USR1_TRDY;
> +            s->usr2 |= USR2_TXDC;
>              imx_update(s);
>          }
>          break;
> @@ -265,8 +275,12 @@ static void imx_serial_write(void *opaque, hwaddr
> offset, s->ucr3 = value & 0xffff;
>          break;
> 
> -    case 0x2d: /* UTS1 */
>      case 0x23: /* UCR4 */
> +        s->ucr4 = value & 0xffff;
> +        imx_update(s);
> +        break;
> +
> +    case 0x2d: /* UTS1 */
>          qemu_log_mask(LOG_UNIMP, "[%s]%s: Unimplemented reg 0x%"
>                        HWADDR_PRIx "\n", TYPE_IMX_SERIAL, __func__,
> offset); /* TODO */
-- 
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Member of Technical Staff,
                 address@hidden | Master of Unix-Fu - Wind River Systems
=============================================================================
   "I put a dollar in a change machine. Nothing changed." - George Carlin
=============================================================================