On 6/24/20 8:33 AM, Paolo Bonzini wrote:
> On 23/06/20 22:07, Eric Blake wrote:
>>>
>>> uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
>>> baud *= 1000;
>>> - baud /= 2 ^ 20;
>>> + baud >>= 20;
>>
>> Dividing by 1M instead of 22 seems much more logical, indeed :)
>
> Based on the spec, the "* 1000" is the clock, in other words this is a
> fixed point value relative to the clock:
>
> f_baud = NCO * f_clock / 2^20
>
> The example in the spec (https://docs.opentitan.org/hw/ip/uart/doc/) has
> f_clock = 50 MHz, while here it's only 1 kHz. And the register is only
> 16 bit, so the above would only allow a baud rate up to 62 (65535 * 1000
> / 2^20).
>
> Should the clock be a property of the device instead?
Ideally the device should use qdev_get_clock_in(),
(see docs/devel/clocks.rst):
static void ibex_uart_init(Object *obj)
{
...
s->f_clk = qdev_get_clock_in(DEVICE(obj), "f_clock");
...
Then in ibex_uart_write():
uint64_t baud = ((value & UART_CTRL_NCO) >> 16);
baud *= clock_get_hz(dev->f_clk));
baud >>= 20;
Devices not using the QDEV_CLOCK API use QDEV properties.
>
> Thanks,
>
> Paolo
>
>> It's odd that we are scaling up by 1000, down by 1024*1024, then
>>
>>> s->char_tx_time = (NANOSECONDS_PER_SECOND / baud) * 10;
>
>
