Re: [PATCH] ibex_uart: fix XOR-as-pow

[Philippe Mathieu-Daudé]

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; 
> 
>