[U-Boot] Illegal use of FP ops in clock_ti814x.c

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Wolfgang Denk]

Dear Matt,

I hope you are the right person to address this to - if not, please
help to redirect to the current responsible developer.

Function pll_sigma_delta_val() in arch/arm/cpu/armv7/am33xx/clock_ti814x.c
incorrectly uses "float" data, which results in FP operations which
are not permitted in U-Boot.

The actual computation appears simple enough so a rewrite of the code
without using any floating point operations should be fairly easy, but
I don't understand the actual logic of this code, so I'd rather leave
this to someone who does.

Could you please help and clean up these three lines of code?

Thanks in advance.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Here is an Appalachian version of management's answer  to  those  who
are  concerned  with  the fate of the project: "Don't worry about the
mule. Just load the wagon."         - Mike Dennison's hillbilly uncle

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/27/2013 05:11 PM, Wolfgang Denk wrote:
> Dear Matt,
> 
> I hope you are the right person to address this to - if not,
> please help to redirect to the current responsible developer.
> 
> Function pll_sigma_delta_val() in
> arch/arm/cpu/armv7/am33xx/clock_ti814x.c incorrectly uses "float"
> data, which results in FP operations which are not permitted in
> U-Boot.
> 
> The actual computation appears simple enough so a rewrite of the
> code without using any floating point operations should be fairly
> easy, but I don't understand the actual logic of this code, so I'd
> rather leave this to someone who does.
> 
> Could you please help and clean up these three lines of code?

Matt's moved on to Linaro now (and this is a more public poke to
update the maintainers entry you owe me).  Care to look at this?

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSbt1nAAoJENk4IS6UOR1Wdy8P/RT18/UG5TBQugaSdqAkscS1
oMRz5M35qZvUuTjRj7JwRMXs/WGhVLO4NP5zDM0eYdo6pD/Z4ZnCPXUpRa46w4X/
xvusu9m+fpKsy/+gEOeOw4+JQ1OnMCLuP0OxpJfvKYwXEOjji5mGxAhxpCTogBFX
6l6vAYiEcYYG2S58AuGFYL9l0y0x7kZNnvfAzM5xJgxNOhxmGefGABuILXWRs5mt
rHQBrPlofc+E2n73BxSAxk35cqy1Fm6CP35ETuKb3NMonoYbtXebh3ADyjxaNdf6
IZHpTserTNLaSGUSq2QVrF9zOnQKj/R4fuTbV7biUI3lF5JqVLO+jsx+E5XwTJpD
bkQlD+vbmSnzK5HebUG6qEFHWxChjPJ0URSvlb0WBrlQJX9TkK4Xetq9ou6FaSpR
xnZ/X6zDuBiuyWdmw01D0en8WSTMHCnYHQ7yAZjL3tuvjoJ6p4xCMFe2FZJE4UUn
p1pT+h3zMipwjDrZ57NureCY7mWdCsRBgIR18MTmjF7XYzo7+4F+yi2aaGifGmPu
zo1zp+1ijjQCVylw2OXGp09ej6azJcVoaRVS7FBEtd5u6MaSBhgfXTvR1bSB5mqw
PbxlnXgfumv+VK3S036FtFXuT8lCx6kWlSLOa9HZSHrwjO0eQv+IeuoVigUnzfq/
oMHEAwDoaJbMjPDgFE2O
=c8xm
-----END PGP SIGNATURE-----

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Måns Rullgård]

Wolfgang Denk <wd@denx.de> writes:

> Dear Matt,
>
> I hope you are the right person to address this to - if not, please
> help to redirect to the current responsible developer.
>
> Function pll_sigma_delta_val() in arch/arm/cpu/armv7/am33xx/clock_ti814x.c
> incorrectly uses "float" data, which results in FP operations which
> are not permitted in U-Boot.
>
> The actual computation appears simple enough so a rewrite of the code
> without using any floating point operations should be fairly easy, but
> I don't understand the actual logic of this code, so I'd rather leave
> this to someone who does.
>
> Could you please help and clean up these three lines of code?

Something like this should be equivalent.  That said, it looks
suspiciously like it's meant to simply do a division and round up.  If
that is the case, +225 should be +249.  It probably makes no difference
for the values actually encountered.

diff --git a/arch/arm/cpu/armv7/am33xx/clock_ti814x.c b/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
index ef14f47..9b5a47b 100644
--- a/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
+++ b/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
@@ -211,11 +211,8 @@ static u32 pll_dco_freq_sel(u32 clkout_dco)
 static u32 pll_sigma_delta_val(u32 clkout_dco)
 {
        u32 sig_val = 0;
-       float frac_div;
 
-       frac_div = (float) clkout_dco / 250;
-       frac_div = frac_div + 0.90;
-       sig_val = (int)frac_div;
+       sig_val = (clkout_dco + 225) / 250;
        sig_val = sig_val << 24;
 
        return sig_val;

-- 
M?ns Rullg?rd
mans@mansr.com

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Wolfgang Denk]

Dear M?ns Rullg?rd,

In message <yw1xd2mp0yqu.fsf@unicorn.mansr.com> you wrote:
> 
> Something like this should be equivalent.  That said, it looks
> suspiciously like it's meant to simply do a division and round up.  If
> that is the case, +225 should be +249.  It probably makes no difference
> for the values actually encountered.

Umm... this is the part which I do not understand.

The original code adds 90%; you add 90%, too.  However, to round up,
one usually adds only 50% ?

> diff --git a/arch/arm/cpu/armv7/am33xx/clock_ti814x.c b/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
> index ef14f47..9b5a47b 100644
> --- a/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
> +++ b/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
> @@ -211,11 +211,8 @@ static u32 pll_dco_freq_sel(u32 clkout_dco)
>  static u32 pll_sigma_delta_val(u32 clkout_dco)
>  {
>         u32 sig_val = 0;
> -       float frac_div;
>  
> -       frac_div = (float) clkout_dco / 250;
> -       frac_div = frac_div + 0.90;
> -       sig_val = (int)frac_div;
> +       sig_val = (clkout_dco + 225) / 250;
>         sig_val = sig_val << 24;

Where are these 90% coming from? Are they in any way meaningful, or
even critical?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
I still miss my ex-wife, but my aim is getting better.

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Måns Rullgård]

Wolfgang Denk <wd@denx.de> writes:

> Dear M?ns Rullg?rd,
>
> In message <yw1xd2mp0yqu.fsf@unicorn.mansr.com> you wrote:
>> 
>> Something like this should be equivalent.  That said, it looks
>> suspiciously like it's meant to simply do a division and round up.  If
>> that is the case, +225 should be +249.  It probably makes no difference
>> for the values actually encountered.
>
> Umm... this is the part which I do not understand.
>
> The original code adds 90%; you add 90%, too.  However, to round up,
> one usually adds only 50% ?

Adding 50% would round to nearest.  For integer division to round up,
you must add one less than the divisor.

>> diff --git a/arch/arm/cpu/armv7/am33xx/clock_ti814x.c b/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
>> index ef14f47..9b5a47b 100644
>> --- a/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
>> +++ b/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
>> @@ -211,11 +211,8 @@ static u32 pll_dco_freq_sel(u32 clkout_dco)
>>  static u32 pll_sigma_delta_val(u32 clkout_dco)
>>  {
>>         u32 sig_val = 0;
>> -       float frac_div;
>>  
>> -       frac_div = (float) clkout_dco / 250;
>> -       frac_div = frac_div + 0.90;
>> -       sig_val = (int)frac_div;
>> +       sig_val = (clkout_dco + 225) / 250;
>>         sig_val = sig_val << 24;
>
> Where are these 90% coming from? Are they in any way meaningful, or
> even critical?

My guess is that it was someone's approximation of 249 / 250.  I don't
know the hardware, so it's conceivable that it really should be this
way, although it seems unlikely.

-- 
M?ns Rullg?rd
mans at mansr.com

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Wolfgang Denk]

Dear M?ns Rullg?rd,

In message <yw1x8uxc28y9.fsf@unicorn.mansr.com> you wrote:
>
> >> Something like this should be equivalent.  That said, it looks
> >> suspiciously like it's meant to simply do a division and round up.  If
> >> that is the case, +225 should be +249.  It probably makes no difference
> >> for the values actually encountered.
> >
> > Umm... this is the part which I do not understand.
> >
> > The original code adds 90%; you add 90%, too.  However, to round up,
> > one usually adds only 50% ?
> 
> Adding 50% would round to nearest.  For integer division to round up,
> you must add one less than the divisor.

Agreed.  But do we want to round up?  The original code used +90%,
which is something else, too...

> > Where are these 90% coming from? Are they in any way meaningful, or
> > even critical?
> 
> My guess is that it was someone's approximation of 249 / 250.  I don't
> know the hardware, so it's conceivable that it really should be this
> way, although it seems unlikely.

Are you able to test such a modificationon actual hardware?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The X11 source code style is ATROCIOUS and should not be used  as  a
model."                                                   - Doug Gwyn

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/29/2013 06:48 AM, Wolfgang Denk wrote:
> Dear M?ns Rullg?rd,
> 
> In message <yw1x8uxc28y9.fsf@unicorn.mansr.com> you wrote:
>> 
>>>> Something like this should be equivalent.  That said, it
>>>> looks suspiciously like it's meant to simply do a division
>>>> and round up.  If that is the case, +225 should be +249.  It
>>>> probably makes no difference for the values actually
>>>> encountered.
>>> 
>>> Umm... this is the part which I do not understand.
>>> 
>>> The original code adds 90%; you add 90%, too.  However, to
>>> round up, one usually adds only 50% ?
>> 
>> Adding 50% would round to nearest.  For integer division to round
>> up, you must add one less than the divisor.
> 
> Agreed.  But do we want to round up?  The original code used +90%, 
> which is something else, too...

And I imagine it's unlikely the original author of the code is around
anymore, or recalls exactly why.  I'm pretty sure Matt just lifted the
code from the vendor tree and since it wasn't throwing warnings didn't
notice the floating point part.

>>> Where are these 90% coming from? Are they in any way
>>> meaningful, or even critical?
>> 
>> My guess is that it was someone's approximation of 249 / 250.  I
>> don't know the hardware, so it's conceivable that it really
>> should be this way, although it seems unlikely.
> 
> Are you able to test such a modificationon actual hardware?

I suspect Matt can, after Linaro Connect.  I don't have one of these
platforms handy but I think he still does.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJSb6iqAAoJENk4IS6UOR1W+JkP/j7MJ2s9RxNCpRTi+nvXgSL1
A9SWL0rAbgl6ggiYU1tIE1NUxrNmlaLJxzP/VKciJ7wPeHkmiZN4gsL5JmsbIj9y
eWh1eqyXPI8Tv69x0v8JsDBC8hHHBgnWqe8afsLN0ZspU77GGgFAx0Dq7lfaaGZt
GopTHbBEf9pePbE8X3y11kxtjyF5SJfdfvPsEFbNeuR5fkfqcanvNPi6uhYDnwgz
UwW0htFyAO3vmZHv7Yhh5XhF+sIgHzEsYzZBZ0KVf+pDpyMd2Ni8RG1RTt66UHqE
kuPLIU9EwZYBUTh0DGtx/QFYOHCAUTpAYV5OhLL2ycIpzF3CRj/cBdnhiDk7LPPz
5G5heIJmRxyTkvjdm4Lh552NQBo6oWVJP+NFIejYG3aD632lH+cp1lDxjPzk3LGQ
qVwyE+rRiY96VW6qMeBzFxBJ3J4k73zxdyY4eM9Ka2uzWqM3rykZ1EESaDTrDTQ9
DnI9E1QVuXQLjaTZ0RjKVw8XA/yPlBZ/LVIQDpEUr0WV9+Qgjs94B6H1flkygcqW
DLyg+PpB5czNNtMdAbj55wzXNmAX39MIKHkR0OWm5HEGg26u/zjlYF1kk1YeuQNT
mgWhNohtRARkJMfUXmSu5M9e2S3elNcv61GnOzRajgTzO8XjkXO4LYjyfarIzGdK
FitrvVe8n00RGK3u6M9j
=hWQX
-----END PGP SIGNATURE-----

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Matt Porter]

On Tue, Oct 29, 2013 at 08:23:07AM -0400, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 10/29/2013 06:48 AM, Wolfgang Denk wrote:
> > Dear M?ns Rullg?rd,
> > 
> > In message <yw1x8uxc28y9.fsf@unicorn.mansr.com> you wrote:
> >> 
> >>>> Something like this should be equivalent.  That said, it
> >>>> looks suspiciously like it's meant to simply do a division
> >>>> and round up.  If that is the case, +225 should be +249.  It
> >>>> probably makes no difference for the values actually
> >>>> encountered.
> >>> 
> >>> Umm... this is the part which I do not understand.
> >>> 
> >>> The original code adds 90%; you add 90%, too.  However, to
> >>> round up, one usually adds only 50% ?
> >> 
> >> Adding 50% would round to nearest.  For integer division to round
> >> up, you must add one less than the divisor.
> > 
> > Agreed.  But do we want to round up?  The original code used +90%, 
> > which is something else, too...
> 
> And I imagine it's unlikely the original author of the code is around
> anymore, or recalls exactly why.  I'm pretty sure Matt just lifted the
> code from the vendor tree and since it wasn't throwing warnings didn't
> notice the floating point part.
> 
> >>> Where are these 90% coming from? Are they in any way
> >>> meaningful, or even critical?
> >> 
> >> My guess is that it was someone's approximation of 249 / 250.  I
> >> don't know the hardware, so it's conceivable that it really
> >> should be this way, although it seems unlikely.
> > 
> > Are you able to test such a modificationon actual hardware?
> 
> I suspect Matt can, after Linaro Connect.  I don't have one of these
> platforms handy but I think he still does.

Thankfully Tom reminded me of this because I lost some of the list
traffic due to some local mail issues.

Although not explicitly mentioned in the TRM or any application
notes I can find, the 90% appears to come from jitter compensation for
the delta sigma fractional divider. I see some comments that imply this
in various old vendor kernel tree clock implementations where they are
rounding various pll constants. I can't be 100% sure without some
insight from TI folks. Given that it's working for our known users, I'd
like to preserve that until we get somebody that can shed some light on
that.

As Tom guessed, I low-level cherry-picked a lot of pieces from long-lost
authors in this area. I obviously missed this floating point math in the
cleanup.

I tested this patch on my TI8148 EVM and it works as expected.

Acked-by: Matt Porter <mporter@linaro.org>

-Matt

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Måns Rullgård]

Wolfgang Denk <wd@denx.de> writes:

> Dear M?ns Rullg?rd,
>
> In message <yw1x8uxc28y9.fsf@unicorn.mansr.com> you wrote:
>>
>> >> Something like this should be equivalent.  That said, it looks
>> >> suspiciously like it's meant to simply do a division and round up.  If
>> >> that is the case, +225 should be +249.  It probably makes no difference
>> >> for the values actually encountered.
>> >
>> > Umm... this is the part which I do not understand.
>> >
>> > The original code adds 90%; you add 90%, too.  However, to round up,
>> > one usually adds only 50% ?
>> 
>> Adding 50% would round to nearest.  For integer division to round up,
>> you must add one less than the divisor.
>
> Agreed.  But do we want to round up?  The original code used +90%,
> which is something else, too...

That rounds fractions >= 0.1 up while < 0.1 is rounded down.  It's an
unusual thing to do, which is why I suspect it's not quite correct in
the first place.

>> > Where are these 90% coming from? Are they in any way meaningful, or
>> > even critical?
>> 
>> My guess is that it was someone's approximation of 249 / 250.  I don't
>> know the hardware, so it's conceivable that it really should be this
>> way, although it seems unlikely.
>
> Are you able to test such a modificationon actual hardware?

No.

-- 
M?ns Rullg?rd
mans at mansr.com

[U-Boot] Illegal use of FP ops in clock_ti814x.c

[Tom Rini]

On Mon, Oct 28, 2013 at 11:19:53PM +0000, M?ns Rullg?rd wrote:

> Wolfgang Denk <wd@denx.de> writes:
> 
> > Dear Matt,
> >
> > I hope you are the right person to address this to - if not, please
> > help to redirect to the current responsible developer.
> >
> > Function pll_sigma_delta_val() in arch/arm/cpu/armv7/am33xx/clock_ti814x.c
> > incorrectly uses "float" data, which results in FP operations which
> > are not permitted in U-Boot.
> >
> > The actual computation appears simple enough so a rewrite of the code
> > without using any floating point operations should be fairly easy, but
> > I don't understand the actual logic of this code, so I'd rather leave
> > this to someone who does.
> >
> > Could you please help and clean up these three lines of code?
> 
> Something like this should be equivalent.  That said, it looks
> suspiciously like it's meant to simply do a division and round up.  If
> that is the case, +225 should be +249.  It probably makes no difference
> for the values actually encountered.
> Acked-by: Matt Porter <mporter@linaro.org>
> 
> 
> diff --git a/arch/arm/cpu/armv7/am33xx/clock_ti814x.c b/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
> index ef14f47..9b5a47b 100644
> --- a/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
> +++ b/arch/arm/cpu/armv7/am33xx/clock_ti814x.c
> @@ -211,11 +211,8 @@ static u32 pll_dco_freq_sel(u32 clkout_dco)
>  static u32 pll_sigma_delta_val(u32 clkout_dco)
>  {
>         u32 sig_val = 0;
> -       float frac_div;
>  
> -       frac_div = (float) clkout_dco / 250;
> -       frac_div = frac_div + 0.90;
> -       sig_val = (int)frac_div;
> +       sig_val = (clkout_dco + 225) / 250;
>         sig_val = sig_val << 24;
>  
>         return sig_val;

With a massively re-worded commit message, applied to u-boot-ti/master,
thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140221/6faf9ab1/attachment.pgp>