Re: [PATCH v6 8/9] hw/char/cadence_uart: add clock support
On 12/2/19 4:24 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde wrote:
>>
>> Switch the cadence uart to multi-phase reset and add the
>> reference clock input.
>>
>> The input clock frequency is added to the migration structure.
>>
>> The reference clock controls the baudrate generation. If it disabled,
>> any input characters and events are ignored.
>>
>> If this clock remains unconnected, the uart behaves as before
>> (it default to a 50MHz ref clock).
>>
>> Signed-off-by: Damien Hedde
>
>> static void uart_parameters_setup(CadenceUARTState *s)
>> {
>> QEMUSerialSetParams ssp;
>> -unsigned int baud_rate, packet_size;
>> +unsigned int baud_rate, packet_size, input_clk;
>> +input_clk = clock_get_frequency(s->refclk);
>>
>> -baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
>> -UART_INPUT_CLK / 8 : UART_INPUT_CLK;
>> +baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
>> +baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> +trace_cadence_uart_baudrate(baud_rate);
>> +
>> +ssp.speed = baud_rate;
>>
>> -ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> packet_size = 1;
>>
>> switch (s->r[R_MR] & UART_MR_PAR) {
>> @@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
>> }
>>
>> packet_size += ssp.data_bits + ssp.stop_bits;
>> +if (ssp.speed == 0) {
>> +/*
>> + * Avoid division-by-zero below.
>> + * TODO: find something better
>> + */
>
> Any ideas what might be better? :-)
Well maybe the comment is misplaced. Because it is probably a good thing
to round up the ssp.speed in case it becomes 0 (which is very unlikely
apart from the case where the input clock is 0/disabled).
The problem is what should we do when the clock is disabled ?
Right now we:
+ set a minimal baudrate
+ ignore input characters/events
+ still forward output characters... (I just checked)
I suppose we could at least fix the last point: we can drop any output
characters. But if this happen, there is definitely a problem somewhere
(a firmware should not try to send characters to an unclocked uart). Is
there a qemu way of reporting this kind of situation ?
It would be best to somehow tell the backend we're not handling anything
anymore. So I could put that in the comment instead.
I really don't know if/how we can do that. When I looked I did not see
any way to do the opposite of qemu_chr_fe_accept_input() which is done
to start receiving stuff.
--
Damien
Re: [PATCH v6 8/9] hw/char/cadence_uart: add clock support
On Wed, 4 Sep 2019 at 13:56, Damien Hedde wrote:
>
> Switch the cadence uart to multi-phase reset and add the
> reference clock input.
>
> The input clock frequency is added to the migration structure.
>
> The reference clock controls the baudrate generation. If it disabled,
> any input characters and events are ignored.
>
> If this clock remains unconnected, the uart behaves as before
> (it default to a 50MHz ref clock).
>
> Signed-off-by: Damien Hedde
> static void uart_parameters_setup(CadenceUARTState *s)
> {
> QEMUSerialSetParams ssp;
> -unsigned int baud_rate, packet_size;
> +unsigned int baud_rate, packet_size, input_clk;
> +input_clk = clock_get_frequency(s->refclk);
>
> -baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
> -UART_INPUT_CLK / 8 : UART_INPUT_CLK;
> +baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
> +baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
> +trace_cadence_uart_baudrate(baud_rate);
> +
> +ssp.speed = baud_rate;
>
> -ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
> packet_size = 1;
>
> switch (s->r[R_MR] & UART_MR_PAR) {
> @@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
> }
>
> packet_size += ssp.data_bits + ssp.stop_bits;
> +if (ssp.speed == 0) {
> +/*
> + * Avoid division-by-zero below.
> + * TODO: find something better
> + */
Any ideas what might be better? :-)
> +ssp.speed = 1;
> +}
> s->char_tx_time = (NANOSECONDS_PER_SECOND / ssp.speed) * packet_size;
> qemu_chr_fe_ioctl(>chr, CHR_IOCTL_SERIAL_SET_PARAMS, );
> }
thanks
-- PMM
[Qemu-devel] [PATCH v6 8/9] hw/char/cadence_uart: add clock support
Switch the cadence uart to multi-phase reset and add the
reference clock input.
The input clock frequency is added to the migration structure.
The reference clock controls the baudrate generation. If it disabled,
any input characters and events are ignored.
If this clock remains unconnected, the uart behaves as before
(it default to a 50MHz ref clock).
Signed-off-by: Damien Hedde
---
hw/char/cadence_uart.c | 85 ++
hw/char/trace-events | 3 ++
include/hw/char/cadence_uart.h | 1 +
3 files changed, 79 insertions(+), 10 deletions(-)
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 0e315b2376..bae43a9679 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -31,6 +31,8 @@
#include "qemu/module.h"
#include "hw/char/cadence_uart.h"
#include "hw/irq.h"
+#include "hw/qdev-clock.h"
+#include "trace.h"
#ifdef CADENCE_UART_ERR_DEBUG
#define DB_PRINT(...) do { \
@@ -97,7 +99,7 @@
#define LOCAL_LOOPBACK (0x2 << UART_MR_CHMODE_SH)
#define REMOTE_LOOPBACK(0x3 << UART_MR_CHMODE_SH)
-#define UART_INPUT_CLK 5000
+#define UART_DEFAULT_REF_CLK (50 * 1000 * 1000)
#define R_CR (0x00/4)
#define R_MR (0x04/4)
@@ -171,12 +173,15 @@ static void uart_send_breaks(CadenceUARTState *s)
static void uart_parameters_setup(CadenceUARTState *s)
{
QEMUSerialSetParams ssp;
-unsigned int baud_rate, packet_size;
+unsigned int baud_rate, packet_size, input_clk;
+input_clk = clock_get_frequency(s->refclk);
-baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
-UART_INPUT_CLK / 8 : UART_INPUT_CLK;
+baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
+baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
+trace_cadence_uart_baudrate(baud_rate);
+
+ssp.speed = baud_rate;
-ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
packet_size = 1;
switch (s->r[R_MR] & UART_MR_PAR) {
@@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
}
packet_size += ssp.data_bits + ssp.stop_bits;
+if (ssp.speed == 0) {
+/*
+ * Avoid division-by-zero below.
+ * TODO: find something better
+ */
+ssp.speed = 1;
+}
s->char_tx_time = (NANOSECONDS_PER_SECOND / ssp.speed) * packet_size;
qemu_chr_fe_ioctl(>chr, CHR_IOCTL_SERIAL_SET_PARAMS, );
}
@@ -340,6 +352,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 or in reset */
+if (!clock_is_enabled(s->refclk) || device_is_resetting(DEVICE(s))) {
+return;
+}
+
if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
uart_write_rx_fifo(opaque, buf, size);
}
@@ -353,6 +370,11 @@ static void uart_event(void *opaque, int event)
CadenceUARTState *s = opaque;
uint8_t buf = '\0';
+/* ignore characters when unclocked or in reset */
+if (!clock_is_enabled(s->refclk) || device_is_resetting(DEVICE(s))) {
+return;
+}
+
if (event == CHR_EVENT_BREAK) {
uart_write_rx_fifo(opaque, , 1);
}
@@ -462,9 +484,9 @@ static const MemoryRegionOps uart_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static void cadence_uart_reset(DeviceState *dev)
+static void cadence_uart_reset_init(Object *obj, ResetType type)
{
-CadenceUARTState *s = CADENCE_UART(dev);
+CadenceUARTState *s = CADENCE_UART(obj);
s->r[R_CR] = 0x0128;
s->r[R_IMR] = 0;
@@ -473,6 +495,11 @@ static void cadence_uart_reset(DeviceState *dev)
s->r[R_BRGR] = 0x028B;
s->r[R_BDIV] = 0x000F;
s->r[R_TTRIG] = 0x0020;
+}
+
+static void cadence_uart_reset_hold(Object *obj)
+{
+CadenceUARTState *s = CADENCE_UART(obj);
uart_rx_reset(s);
uart_tx_reset(s);
@@ -491,6 +518,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);
@@ -500,9 +535,23 @@ static void cadence_uart_init(Object *obj)
sysbus_init_mmio(sbd, >iomem);
sysbus_init_irq(sbd, >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->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
}
+static int cadence_uart_pre_load(void *opaque)
+{
+CadenceUARTState *s = opaque;
+
+/* the frequency will be overriden if the subsection is present */
+
[Qemu-devel] [PATCH v6 8/9] hw/char/cadence_uart: add clock support
From: Damien Hedde
Switch the cadence uart to multi-phase reset and add the
reference clock input.
The input clock frequency is added to the migration structure.
The reference clock controls the baudrate generation. If it disabled,
any input characters and events are ignored.
If this clock remains unconnected, the uart behaves as before
(it default to a 50MHz ref clock).
Signed-off-by: Damien Hedde
---
hw/char/cadence_uart.c | 85 ++
hw/char/trace-events | 3 ++
include/hw/char/cadence_uart.h | 1 +
3 files changed, 79 insertions(+), 10 deletions(-)
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 0e315b2376..bae43a9679 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -31,6 +31,8 @@
#include "qemu/module.h"
#include "hw/char/cadence_uart.h"
#include "hw/irq.h"
+#include "hw/qdev-clock.h"
+#include "trace.h"
#ifdef CADENCE_UART_ERR_DEBUG
#define DB_PRINT(...) do { \
@@ -97,7 +99,7 @@
#define LOCAL_LOOPBACK (0x2 << UART_MR_CHMODE_SH)
#define REMOTE_LOOPBACK(0x3 << UART_MR_CHMODE_SH)
-#define UART_INPUT_CLK 5000
+#define UART_DEFAULT_REF_CLK (50 * 1000 * 1000)
#define R_CR (0x00/4)
#define R_MR (0x04/4)
@@ -171,12 +173,15 @@ static void uart_send_breaks(CadenceUARTState *s)
static void uart_parameters_setup(CadenceUARTState *s)
{
QEMUSerialSetParams ssp;
-unsigned int baud_rate, packet_size;
+unsigned int baud_rate, packet_size, input_clk;
+input_clk = clock_get_frequency(s->refclk);
-baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
-UART_INPUT_CLK / 8 : UART_INPUT_CLK;
+baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
+baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
+trace_cadence_uart_baudrate(baud_rate);
+
+ssp.speed = baud_rate;
-ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
packet_size = 1;
switch (s->r[R_MR] & UART_MR_PAR) {
@@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
}
packet_size += ssp.data_bits + ssp.stop_bits;
+if (ssp.speed == 0) {
+/*
+ * Avoid division-by-zero below.
+ * TODO: find something better
+ */
+ssp.speed = 1;
+}
s->char_tx_time = (NANOSECONDS_PER_SECOND / ssp.speed) * packet_size;
qemu_chr_fe_ioctl(>chr, CHR_IOCTL_SERIAL_SET_PARAMS, );
}
@@ -340,6 +352,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 or in reset */
+if (!clock_is_enabled(s->refclk) || device_is_resetting(DEVICE(s))) {
+return;
+}
+
if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
uart_write_rx_fifo(opaque, buf, size);
}
@@ -353,6 +370,11 @@ static void uart_event(void *opaque, int event)
CadenceUARTState *s = opaque;
uint8_t buf = '\0';
+/* ignore characters when unclocked or in reset */
+if (!clock_is_enabled(s->refclk) || device_is_resetting(DEVICE(s))) {
+return;
+}
+
if (event == CHR_EVENT_BREAK) {
uart_write_rx_fifo(opaque, , 1);
}
@@ -462,9 +484,9 @@ static const MemoryRegionOps uart_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static void cadence_uart_reset(DeviceState *dev)
+static void cadence_uart_reset_init(Object *obj, ResetType type)
{
-CadenceUARTState *s = CADENCE_UART(dev);
+CadenceUARTState *s = CADENCE_UART(obj);
s->r[R_CR] = 0x0128;
s->r[R_IMR] = 0;
@@ -473,6 +495,11 @@ static void cadence_uart_reset(DeviceState *dev)
s->r[R_BRGR] = 0x028B;
s->r[R_BDIV] = 0x000F;
s->r[R_TTRIG] = 0x0020;
+}
+
+static void cadence_uart_reset_hold(Object *obj)
+{
+CadenceUARTState *s = CADENCE_UART(obj);
uart_rx_reset(s);
uart_tx_reset(s);
@@ -491,6 +518,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);
@@ -500,9 +535,23 @@ static void cadence_uart_init(Object *obj)
sysbus_init_mmio(sbd, >iomem);
sysbus_init_irq(sbd, >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->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
}
+static int cadence_uart_pre_load(void *opaque)
+{
+CadenceUARTState *s = opaque;
+
+/* the frequency will be overriden if the subsection is present
