Linux 4.4 at91

[aystarik]

No description provided.

[aystarik]

Signed-off-by: Alexey Starikovskiy aystarik@gmail.com

[noglitch]

Hi @aystarik,
Your code is clearly neat and I do like it.
Anyway, I would to know if you modified it because the previous code produced some issue during the rounding or if you experienced some bugs with it?

[aystarik]

Hi Nicolas,
The problem with previous code was it rounded values in wrong place and
produced wrong baud rate as a result. It will work with two same
implementations but will fail if the other side has proper baud rate.
In particular without my patch I was unable to get correct rate for 460800
and 921600 bauds and connect to PC with MOXA serial card. With my patch I
can.

Regards,
Alex.

On Tue, Sep 20, 2016 at 6:22 PM, Nicolas Ferre notifications@github.com
wrote:

Hi @aystarik https://github.com/aystarik,
Your code is clearly neat and I do like it.
Anyway, I would to know if you modified it because the previous code
produced some issue during the rounding or if you experienced some bugs
with it?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIQfXg2oiW9mGMAncn7iZryeuPeGrgdcks5qr_pQgaJpZM4J5JL4
.

[ldesroches]

Hi Alexey,

After discussing a bit with other guys, I think there is something better to do. You probably had this issue because the fp overflow was not managed. The use of the rounding to compute fp can lead to set it to 8 that is not supported.
With the patch you propose, we lose a bit of accuracy at the beginning and I think the compiler can optimize this part instead of using shift and mask operators. It allows to stick to the datasheet which says CD = Selected clock / (16 * baudrate).
To come back to the baudrate accuracy, on my side, I had a better one with this lastest round_closest. So if your issue comes from the overflow error then I would prefer to manage it:
if (fp > 7) fp = 7;

What is your feeling about this solution, is it working on your side?

Regards

Ludovic

[noglitch]

@aystarik sorry to bother you but can you give us your serial IP clock speed (and on which product actually) so that we can double check the computation of the 460800 and 921600 baud rates that you gave us.

[aystarik]

Hi Nicolas,
I've raised it to 132MHz on SAMA5D35. With default DT it was 66MHz.

On Wed, Sep 21, 2016 at 11:14 AM, Nicolas Ferre notifications@github.com
wrote:

@aystarik https://github.com/aystarik sorry to bother you but can you
give us your serial IP clock speed (and on which product actually) so that
we can double check the computation of the 460800 and 921600 baud rates
that you gave us.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIQfXtI3yyZn770G9TnYYn_6nIh_Pw6bks5qsOdfgaJpZM4J5JL4
.

[aystarik]

#include <stdio.h>
/*

• Divide positive or negative dividend by positive divisor and round
• to closest integer. Result is undefined for negative divisors and
• for negative dividends if the divisor variable type is unsigned.
  */
  #define DIV_ROUND_CLOSEST(x, divisor)(
  {
  typeof(x) __x = x;
  typeof(divisor) __d = divisor;
  (((typeof(x))-1) > 0 ||
  ((typeof(divisor))-1) > 0 || (__x) > 0) ?
  (((__x) + ((__d) / 2)) / (__d)) :
  (((__x) - ((__d) / 2)) / (__d));
  }
  )

float old_calc(unsigned clk, unsigned baud) {
unsigned div = DIV_ROUND_CLOSEST(clk, baud);
unsigned cd = div / 16;
unsigned fp = DIV_ROUND_CLOSEST(div % 16, 2);
fp &= 7;
printf("old: cd = %d, fp = %d\n", cd, fp);
return clk/(16.0 * (cd + fp / 8.0));
}

float new_calc(unsigned clk, unsigned baud) {
unsigned div = DIV_ROUND_CLOSEST(clk, baud * 2);
unsigned cd = div >> 3;
unsigned fp = div & 7;
printf("new: cd = %d, fp = %d\n", cd, fp);
return clk/(16.0 * (cd + fp / 8.0));

}

int main() {
unsigned clk = 66000000;
unsigned i;
for (i = 9600; i < 1000000; i += 9600) {
float old = old_calc(clk, i);
float old_err = 100.0 * (old - i)/i;

float new = new_calc(clk, i);
float new_err = 100.0 * (new - i)/i;
printf("baud = %d, old = %f (%.2f%%), new = %f (%.2f%%)\n\n", i, old,
old_err, new, new_err);
}
return 0;
}

On Wed, Sep 21, 2016 at 11:25 AM, Alexey Starikovskiy aystarik@gmail.com
wrote:

Hi Nicolas,
I've raised it to 132MHz on SAMA5D35. With default DT it was 66MHz.

On Wed, Sep 21, 2016 at 11:14 AM, Nicolas Ferre notifications@github.com
wrote:

@aystarik https://github.com/aystarik sorry to bother you but can you
give us your serial IP clock speed (and on which product actually) so that
we can double check the computation of the 460800 and 921600 baud rates
that you gave us.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIQfXtI3yyZn770G9TnYYn_6nIh_Pw6bks5qsOdfgaJpZM4J5JL4
.

[bbrezillon]

@aystarik are you sure old_calc() is correct?

can you replace

return clk/(16.0 * (cd + fp / 8.0));

by

return clk/((16.0 * cd) + (fp * 2));

And, by the way, you did not apply the changes suggested by @ldesroches.

Can you try again with:

float old_calc(unsigned clk, unsigned baud) {
unsigned div = DIV_ROUND_CLOSEST(clk, baud);
unsigned cd = div / 16;
unsigned fp = DIV_ROUND_CLOSEST(div % 16, 2);
if (fp > 7)
fp = 7;
printf("old: cd = %d, fp = %d\n", cd, fp);
return clk/((16.0 * cd) + (fp * 2));
}

[aystarik]

Hi Ludovic,
I have the solution, which is simpler than yours and easier (for me) to
understand. I'll stick with it, unless you prove that I lost precision.
Regards,
Alex.

On Wed, Sep 21, 2016 at 9:35 AM, Ludovic Desroches <notifications@github.com

wrote:

Hi Alexey,

After discussing a bit with other guys, I think there is something better
to do. You probably had this issue because the fp overflow was not managed.
The use of the rounding to compute fp can lead to set it to 8 that is not
supported.
With the patch you propose, we lose a bit of accuracy at the beginning and
I think the compiler can optimize this part instead of using shift and mask
operators. It allows to stick to the datasheet which says CD = Selected
clock / (16 * baudrate).
To come back to the baudrate accuracy, on my side, I had a better one with
this lastest round_closest. So if your issue comes from the overflow error
then I would prefer to manage it:
if (fp == 8) {
fp = 0;
cd++;
}
What is your feeling about this solution, is it working on your side?

Regards

Ludovic

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIQfXpyeiMtsQhV3w7ZNl8PO77A4xMSyks5qsNApgaJpZM4J5JL4
.

[bbrezillon]

@aystarik you already introduce an approximation with your first DIV_ROUND_CLOSEST(), because, instead of dividing by baud, you divide by (baud * 2).

[aystarik]

Boris,
This is fractional arithmetic, we have 16.3 bits for divider and best
approach (IMHO) is to calculate 19 bits as integer and then split it into
CD and FP.
If you know any better approach, I am listening.

On Wed, Sep 21, 2016 at 11:46 AM, Boris Brezillon notifications@github.com
wrote:

@aystarik https://github.com/aystarik you already introduce an
approximation with your first DIV_ROUND_CLOSEST(), because, instead of
dividing by baud, you divide by (baud * 2).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIQfXigMg5sMETJSkSewwof41DYPneyZks5qsO79gaJpZM4J5JL4
.

[bbrezillon]

@aystarik you're right.

[ldesroches]

I have the same results if I remove the first round_closest and keep the latest. You can take the fix you prefer Nicolas :p

[noglitch]

@ldesroches, I prefer @aystarik 's patch. I sent it to mainline with some slight modifications right now.
Thanks a lot for your patience and explanations!