> "Pavel Dovgaluk" <pavel.dovga...@ispras.ru> wrote:
> > This patch fixes save/restore of serial interface's state.
> > It includes changing of fcr setter function (it now does not invoke
> > an interrupt while loading vmstate), and saving/restoring all
> > fields that describe the state of serial interface (including timers).
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@gmail.com>
>
> I think we can do this with a new subsection.
>
> > ---
> > hw/serial.c | 133
> > ++++++++++++++++++++++++++++++++++++++--------------------
> > 1 files changed, 87 insertions(+), 46 deletions(-)
> >
> > diff --git a/hw/serial.c b/hw/serial.c
> > index 0ee61dd..936e048 100644
> > --- a/hw/serial.c
> > +++ b/hw/serial.c
> > @@ -362,6 +362,62 @@ static void serial_xmit(void *opaque)
> > }
> >
> >
> > +/* Setter for FCR.
> > + is_load flag means, that value is set while loading VM state
> > + and interrupt should not be invoked */
> > +static void serial_write_fcr(void *opaque, uint32_t val, int is_load)
> > +{
> > + SerialState *s = opaque;
> > +
> > + val = val & 0xFF;
> > +
> > + if (s->fcr == val)
> > + return;
>
> This looks like a test. if this is true, we don't need to restore the
> other values, so we shouldbe safe, right?
Yes, that's right.
Actually, this code is moved from serial_ioport_write into separate
function. All checks and branches were not changed, only irq invocation was
made conditional.
> > + /* Did the enable/disable flag change? If so, make sure FIFOs get
> > flushed */
> > + if ((val ^ s->fcr) & UART_FCR_FE)
> > + val |= UART_FCR_XFR | UART_FCR_RFR;
> > +
> > + /* FIFO clear */
> > +
> > + if (val & UART_FCR_RFR) {
> > + qemu_del_timer(s->fifo_timeout_timer);
> > + s->timeout_ipending=0;
> > + fifo_clear(s,RECV_FIFO);
> > + }
> > +
> > + if (val & UART_FCR_XFR) {
> > + fifo_clear(s,XMIT_FIFO);
> > + }
> > +
> > + if (val & UART_FCR_FE) {
> > + s->iir |= UART_IIR_FE;
> > + /* Set RECV_FIFO trigger Level */
> > + switch (val & 0xC0) {
> > + case UART_FCR_ITL_1:
> > + s->recv_fifo.itl = 1;
> > + break;
> > + case UART_FCR_ITL_2:
> > + s->recv_fifo.itl = 4;
> > + break;
> > + case UART_FCR_ITL_3:
> > + s->recv_fifo.itl = 8;
> > + break;
> > + case UART_FCR_ITL_4:
> > + s->recv_fifo.itl = 14;
> > + break;
> > + }
> > + } else
> > + s->iir &= ~UART_IIR_FE;
> > +
> > + /* Set fcr - or at least the bits in it that are supposed to "stick" */
> > + s->fcr = val & 0xC9;
> > + if (!is_load) {
> > + serial_update_irq(s);
> > + }
>
> we can put the serial_update_irq() at caller site. Function is only
> called twice, on place need to call serial_update_irq() and the other not.
Yes, you right. I didn't think about it.
> > +static const VMStateDescription vmstate_fifo = {
> > + .name = "serial FIFO",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .fields = (VMStateField []) {
> > + VMSTATE_BUFFER(data, SerialFIFO),
> > + VMSTATE_UINT8(count, SerialFIFO),
> > + VMSTATE_UINT8(itl, SerialFIFO),
> > + VMSTATE_UINT8(tail, SerialFIFO),
> > + VMSTATE_UINT8(head, SerialFIFO),
> > + VMSTATE_END_OF_LIST()
> > + }
> > +};
> > +
> > +
> > static const VMStateDescription vmstate_serial = {
> > .name = "serial",
> > - .version_id = 3,
> > + .version_id = 4,
> > .minimum_version_id = 2,
> > .pre_save = serial_pre_save,
> > .post_load = serial_post_load,
> > .fields = (VMStateField []) {
> > VMSTATE_UINT16_V(divider, SerialState, 2),
> > VMSTATE_UINT8(rbr, SerialState),
> > + VMSTATE_UINT8_V(thr, SerialState, 4),
> > + VMSTATE_UINT8_V(tsr, SerialState, 4),
> > VMSTATE_UINT8(ier, SerialState),
> > VMSTATE_UINT8(iir, SerialState),
> > VMSTATE_UINT8(lcr, SerialState),
> > @@ -695,6 +726,16 @@ static const VMStateDescription vmstate_serial = {
> > VMSTATE_UINT8(msr, SerialState),
> > VMSTATE_UINT8(scr, SerialState),
> > VMSTATE_UINT8_V(fcr_vmstate, SerialState, 3),
> > + VMSTATE_INT32_V(thr_ipending, SerialState, 4),
> > + VMSTATE_INT32_V(last_break_enable, SerialState, 4),
> > + VMSTATE_INT32_V(tsr_retry, SerialState, 4),
> > + VMSTATE_STRUCT(recv_fifo, SerialState, 4, vmstate_fifo,
> > SerialFIFO),
> > + VMSTATE_STRUCT(xmit_fifo, SerialState, 4, vmstate_fifo,
> > SerialFIFO),
> > + VMSTATE_TIMER_V(fifo_timeout_timer, SerialState, 4),
> > + VMSTATE_INT32_V(timeout_ipending, SerialState, 4),
> > + VMSTATE_TIMER_V(transmit_timer, SerialState, 4),
> > + VMSTATE_INT32_V(poll_msl, SerialState, 4),
> > + VMSTATE_TIMER_V(modem_status_poll, SerialState, 4),
> > VMSTATE_END_OF_LIST()
> > }
> > };
>
> Anyways, I think that it is better to split the change in two patches.
> One that refactor the common code in another function. And the other
> that adds the VMSTATE bits, I can add the subsection part if you want.
What is the purpose of subsections?
And how to create them?
Pavel Dovgaluk
