Hi Philippe,
On 10/3/18 1:26 AM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
>
> On 10/2/18 4:24 PM, Damien Hedde wrote:
>> Add bus interface and uart reference clock inputs.
>>
>> Note: it is hard to find out from the doc what is the behavior when only one
>> of the clock is disabled.
>>
>> The implemented behaviour is that register access needs both clock being
>> active.
>>
>> The bus interface control the mmios visibility
>
> This sentence sounds odd.
Indeed, and also outdated.
I removed the part switching on/off the mmio visibility according to the
bus clock : accesses were silently ignored in that case, which would
probably have made some user mad at some point.
I replaced it by a check on every access and a LOG_GUEST_ERROR in case
the access is dropped.
>
>>
>> The reference clock controls the baudrate generation. If it disabled,
>> any input characters and events are ignored. Also register accesses are
>> conditioned to the clock being enabled (but is it really the case in
>> reality ?) and a guest error is triggerred if that is not the case.
>>
>> If theses clocks remains unconnected, the uart behaves as before
>> (default to 50MHz ref clock).
>>
>> Signed-off-by: Damien Hedde <damien.he...@greensocs.com>
>> ---
>> include/hw/char/cadence_uart.h | 3 ++
>> hw/char/cadence_uart.c | 92 ++++++++++++++++++++++++++++++++--
>> hw/char/trace-events | 3 ++
>> 3 files changed, 93 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/char/cadence_uart.h b/include/hw/char/cadence_uart.h
>> index 118e3f10de..fd1d4725f4 100644
>> --- a/include/hw/char/cadence_uart.h
>> +++ b/include/hw/char/cadence_uart.h
>> @@ -21,6 +21,7 @@
>> #include "hw/sysbus.h"
>> #include "chardev/char-fe.h"
>> #include "qemu/timer.h"
>> +#include "hw/clock-port.h"
>>
>> #define CADENCE_UART_RX_FIFO_SIZE 16
>> #define CADENCE_UART_TX_FIFO_SIZE 16
>> @@ -47,6 +48,8 @@ typedef struct {
>> CharBackend chr;
>> qemu_irq irq;
>> QEMUTimer *fifo_trigger_handle;
>> + ClockIn *refclk;
>> + ClockIn *busclk;
>> } CadenceUARTState;
>>
>> static inline DeviceState *cadence_uart_create(hwaddr addr,
>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>> index fbdbd463bb..feb5cee4d7 100644
>> --- a/hw/char/cadence_uart.c
>> +++ b/hw/char/cadence_uart.c
>> @@ -28,6 +28,7 @@
>> #include "qemu/timer.h"
>> #include "qemu/log.h"
>> #include "hw/char/cadence_uart.h"
>> +#include "trace.h"
>>
>> #ifdef CADENCE_UART_ERR_DEBUG
>> #define DB_PRINT(...) do { \
>> @@ -94,7 +95,7 @@
>> #define LOCAL_LOOPBACK (0x2 << UART_MR_CHMODE_SH)
>> #define REMOTE_LOOPBACK (0x3 << UART_MR_CHMODE_SH)
>>
>> -#define UART_INPUT_CLK 50000000
>> +#define UART_DEFAULT_REF_CLK (50 * 1000 * 1000)
>>
>> #define R_CR (0x00/4)
>> #define R_MR (0x04/4)
>> @@ -165,15 +166,30 @@ static void uart_send_breaks(CadenceUARTState *s)
>> &break_enabled);
>> }
>>
>> +static unsigned int uart_input_clk(CadenceUARTState *s)
>> +{
>> + return clock_get_frequency(s->refclk);
>> +}
>> +
>> static void uart_parameters_setup(CadenceUARTState *s)
>> {
>> QEMUSerialSetParams ssp;
>> unsigned int baud_rate, packet_size;
>>
>> baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
>> - UART_INPUT_CLK / 8 : UART_INPUT_CLK;
>> + uart_input_clk(s) / 8 : uart_input_clk(s);
>> + baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>
> Safe because s->r[R_BRGR] >= 1, OK.
>
>> + trace_cadence_uart_baudrate(baud_rate);
>> +
>> + ssp.speed = baud_rate;
>> + if (ssp.speed == 0) {
>> + /*
>> + * Avoid division-by-zero below.
>> + * TODO: find something better
>> + */
>> + ssp.speed = 1;
>> + }
>>
>> - ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> packet_size = 1;
>>
>> switch (s->r[R_MR] & UART_MR_PAR) {
>> @@ -337,6 +353,11 @@ static void uart_receive(void *opaque, const uint8_t
>> *buf, int size)
>> CadenceUARTState *s = opaque;
>> uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
>>
>> + /* ignore characters when unclocked */
>> + if (!clock_is_enabled(s->refclk)) {
>> + return;
>> + }
>> +
>> if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
>> uart_write_rx_fifo(opaque, buf, size);
>> }
>> @@ -350,6 +371,11 @@ static void uart_event(void *opaque, int event)
>> CadenceUARTState *s = opaque;
>> uint8_t buf = '\0';
>>
>> + /* ignore events when unclocked */
>> + if (!clock_is_enabled(s->refclk)) {
>> + return;
>> + }
>> +
>> if (event == CHR_EVENT_BREAK) {
>> uart_write_rx_fifo(opaque, &buf, 1);
>> }
>> @@ -382,6 +408,14 @@ static void uart_write(void *opaque, hwaddr offset,
>> {
>> CadenceUARTState *s = opaque;
>>
>> + /* ignore accesses when bus or ref clock is disabled */
>> + if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "cadence_uart: Trying to write register 0x%x"
>> + " while clock is disabled\n", (unsigned) offset);
>> + return;
>> + }
>> +
>> DB_PRINT(" offset:%x data:%08x\n", (unsigned)offset, (unsigned)value);
>> offset >>= 2;
>> if (offset >= CADENCE_UART_R_MAX) {
>> @@ -440,6 +474,14 @@ static uint64_t uart_read(void *opaque, hwaddr offset,
>> CadenceUARTState *s = opaque;
>> uint32_t c = 0;
>>
>> + /* ignore accesses when bus or ref clock is disabled */
>> + if (!(clock_is_enabled(s->busclk) && clock_is_enabled(s->refclk))) {
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "cadence_uart: Trying to read register 0x%x"
>> + " while clock is disabled\n", (unsigned) offset);
>> + return 0;
>> + }
>> +
>> offset >>= 2;
>> if (offset >= CADENCE_UART_R_MAX) {
>> c = 0;
>> @@ -488,6 +530,14 @@ static void cadence_uart_realize(DeviceState *dev,
>> Error **errp)
>> uart_event, NULL, s, NULL, true);
>> }
>>
>> +static void cadence_uart_refclk_update(void *opaque)
>> +{
>> + CadenceUARTState *s = opaque;
>> +
>> + /* recompute uart's speed on clock change */
>> + uart_parameters_setup(s);
>> +}
>> +
>> static void cadence_uart_init(Object *obj)
>> {
>> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> @@ -497,9 +547,26 @@ static void cadence_uart_init(Object *obj)
>> sysbus_init_mmio(sbd, &s->iomem);
>> sysbus_init_irq(sbd, &s->irq);
>>
>> + s->refclk = qdev_init_clock_in(DEVICE(obj), "refclk",
>> + cadence_uart_refclk_update, s);
>> + /* initialize the frequency in case the clock remains unconnected */
>> + clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
>> + s->busclk = qdev_init_clock_in(DEVICE(obj), "busclk", NULL, NULL);
>> + /* initialize the frequency to non-zero in case it remains unconnected
>> */
>> + clock_init_frequency(s->busclk, 100 * 1000 * 1000);
>
> #define INPUT_BUS_CLK_FREQUENCY (100 * 1000 * 1000)
>
>> +
>> s->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
>> }
>>
>> +static int cadence_uart_pre_load(void *opaque)
>> +{
>> + CadenceUARTState *s = opaque;
>> +
>> + clock_init_frequency(s->refclk, UART_DEFAULT_REF_CLK);
>> + clock_init_frequency(s->busclk, 100 * 1000 * 1000);
>
> INPUT_BUS_CLK_FREQUENCY
>
>> + return 0;
>> +}
>> +
>> static int cadence_uart_post_load(void *opaque, int version_id)
>> {
>> CadenceUARTState *s = opaque;
>> @@ -516,10 +583,22 @@ static int cadence_uart_post_load(void *opaque, int
>> version_id)
>> return 0;
>> }
>>
>> +static const VMStateDescription vmstate_cadence_uart_clocks = {
>> + .name = "cadence_uart_clocks",
>> + .version_id = 0,
>> + .minimum_version_id = 0,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_CLOCKIN(refclk, CadenceUARTState),
>> + VMSTATE_CLOCKIN(busclk, CadenceUARTState),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> static const VMStateDescription vmstate_cadence_uart = {
>> .name = "cadence_uart",
>> .version_id = 2,
>> .minimum_version_id = 2,
>> + .pre_load = cadence_uart_pre_load,
>> .post_load = cadence_uart_post_load,
>> .fields = (VMStateField[]) {
>> VMSTATE_UINT32_ARRAY(r, CadenceUARTState, CADENCE_UART_R_MAX),
>> @@ -532,7 +611,10 @@ static const VMStateDescription vmstate_cadence_uart = {
>> VMSTATE_UINT32(rx_wpos, CadenceUARTState),
>> VMSTATE_TIMER_PTR(fifo_trigger_handle, CadenceUARTState),
>> VMSTATE_END_OF_LIST()
>> - }
>> + },
>> + .subsections = (const VMStateDescription * []) {
>> + &vmstate_cadence_uart_clocks,
>> + },
>> };
>>
>> static Property cadence_uart_properties[] = {
>> @@ -548,7 +630,7 @@ static void cadence_uart_class_init(ObjectClass *klass,
>> void *data)
>> dc->vmsd = &vmstate_cadence_uart;
>> dc->reset = cadence_uart_reset;
>> dc->props = cadence_uart_properties;
>> - }
>> +}
>>
>> static const TypeInfo cadence_uart_info = {
>> .name = TYPE_CADENCE_UART,
>> diff --git a/hw/char/trace-events b/hw/char/trace-events
>> index b64213d4dd..2ea25d1ea1 100644
>> --- a/hw/char/trace-events
>> +++ b/hw/char/trace-events
>> @@ -73,3 +73,6 @@ cmsdk_apb_uart_receive(uint8_t c) "CMSDK APB UART: got
>> character 0x%x from backe
>> cmsdk_apb_uart_tx_pending(void) "CMSDK APB UART: character send to backend
>> pending"
>> cmsdk_apb_uart_tx(uint8_t c) "CMSDK APB UART: character 0x%x sent to
>> backend"
>> cmsdk_apb_uart_set_params(int speed) "CMSDK APB UART: params set to %d 8N1"
>> +
>> +# hw/char/cadence_uart.c
>> +cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
>>
>
> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>
> Except migration:
> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>
