On Sun, Jan 16, 2011 at 12:45:53PM +0100, thus spake Christian Lamparter:
> Hello,
>
> please CC your mails to linux-wireless mailing-list in the future.
Okay, sorry about that.
> On Sunday 16 January 2011 04:16:00 Ignacy Gawedzki wrote:
> > Since I recently noticed that you added the support for the ticks_per_msec
> > field in fw, I thought it would be better to use that to convert durations
> > from ticks to milliseconds, instead of the fixed 44 literal. =)
>
> Is this a regression on my part? I don't see any references to __udivsi3_i4i
> during build.
Absolutely not. I'm talking about my (experimental) modifications to the
firmware code to measure TX frame service time and report that in msec in the
tx status.
> Also, you can almost always get around it by converting "a / b = c" to
> "a = b * c".
I doubt so, since what I need to do is roughly
msec_to_report = ticks_measured / ticks_per_msec;
my input being of course ticks_measured. :/
> > Unfortunately, it seems that dividing by a non-static value is not supported
> > natively on that platform and I get undefined reference to __udivsi3_i4i. Do
> > you, by any chance, know how to fix that? Apparently there are some support
> > libs in the toolchain's gcc with that symbol, but I just got lost in the CMake
> > file hierarchy while looking for a place to start hacking that in.
>
> No need for a hack, just get a udivsi3_i4i.S (a good place is the linux kernel
> library sources in arch/sh/lib, you could also import one from the original
> ar9170fw.git) and place it into carlfw/src. Next, you need to modify the
> carlfw/CMakeLists.txt: add src/udivsi3_i4i.S to carl9170_lib_src and put
> "set_source_files_properties(src/udivsi3_i4i.S PROPERTIES LANGUAGE C)"
> a few lines further down.
>
> compile...
>
> [Note: you probably have to change the name of the __udivsi3_i4i symbol
> to ___udivsi3_i4i too]
Great, thanks for the hint, I'll try that as soon as I can.
Regards,
Ignacy
--
To err is human, to moo bovine.
On Friday 18 March 2011 20:16:15 Ignacy Gawedzki wrote:
> On Sun, Jan 16, 2011 at 01:48:33PM +0100, thus spake Ignacy Gawedzki:
> > On Sun, Jan 16, 2011 at 12:45:53PM +0100, thus spake Christian Lamparter:
> > > No need for a hack, just get a udivsi3_i4i.S (a good place is the linux kernel
> > > library sources in arch/sh/lib, you could also import one from the original
> > > ar9170fw.git) and place it into carlfw/src. Next, you need to modify the
> > > carlfw/CMakeLists.txt: add src/udivsi3_i4i.S to carl9170_lib_src and put
> > > "set_source_files_properties(src/udivsi3_i4i.S PROPERTIES LANGUAGE C)"
> > > a few lines further down.
> > >
> > > compile...
> > >
> > > [Note: you probably have to change the name of the __udivsi3_i4i symbol
> > > to ___udivsi3_i4i too]
> >
> > Great, thanks for the hint, I'll try that as soon as I can.
>
> So I finally got the time to test that solution. Compilation worked like a
> charm indeed, thanks. All seemed fine until I ran that code. Indeed the
> result of the division is completely off. :(
>
> For instance, if I have A = 17719, B = 44, then A / B gives C = 12886, instead
> of the expected 402. It is as though the result was multiplied by 32.
I gave it a try and It worked?!
[54670.551750] ieee80211 phy14: FW: a:0x4537 b:0x2c a/b:0x192 (0x192 = 402).
(It was nothing else than just define "a" and "b" (as global variables) and
add the INFO("a:0x%x b:0x%x a/b:0x%x", a, b, a/b);)
Regards,
Chr
On Sat, Mar 19, 2011 at 03:03:06AM +0100, thus spake Christian Lamparter:
> On Saturday 19 March 2011 02:22:44 Ignacy Gawedzki wrote:
> > I suppose in your case the values are known at compile time and the compiler
> > does the calculation statically.
> sometimes its just down to "luck" ;)
>
> > If you define the global variables as volatile, then the optimization is
> > forbidden and you should get the bogus results.
>
> ???
> No, "global" is enough. As long as you don't select "const".
> (In fact there's more of a story behind "const volatile"
> than you might think, or have you ever wondered why carl9170
> still ships with gcc 4.4 instead of 4.5? Anyway that's OT)
As a rule of thumb, I remembered that "volatile" tells the compiler "you're
not the only one to manage that variable, don't make any assumptions about its
current value", as could be the case if the variable is allocated in some
shared memory segment/register/whatever and might change "on its own".
Anyway, in the case at hand, it seems to make a difference.
> > BTW, I used udivsi3_i4i.S from the Linux kernel, modified it as you indicated.
> > Please find my version attached.
> You are right, I can reproduce your problem with the attached udiv.
> The crux is seems to be "hidden" inside the kernel's Makefile:
>
> >udivsi3-y := udivsi3_i4i-Os.o
> >
> >ifneq ($(CONFIG_CC_OPTIMIZE_FOR_SIZE),y)
> >udivsi3-$(CONFIG_CPU_SH3) := udivsi3_i4i.o
> >udivsi3-$(CONFIG_CPU_SH4) := udivsi3_i4i.o
> >endif
>
> It looks like the version you are using is "made" for SH3+,
> however the AR9170 has a SH2. (Jup, I can't believe it either :-D)
Ahaaa. Thanks for your help. It works much better now.
BTW, it seems you have some detailed technical documentation about the AR9170
at your disposal. I was wondering whether this is something available
publicly or not. Could be an interesting read at times. =)
Regards,
Ignacy
--
Everything is more fun naked except cooking with grease.
On Sat, Mar 19, 2011 at 04:43:46PM +0100, thus spake Christian Lamparter:
> On Saturday 19 March 2011 10:22:37 Ignacy Gawedzki wrote:
> > As a rule of thumb, I remembered that "volatile" tells the compiler "you're
> > not the only one to manage that variable, don't make any assumptions about its
> > current value", as could be the case if the variable is allocated in some
> > shared memory segment/register/whatever and might change "on its own".
> > Anyway, in the case at hand, it seems to make a difference.
>
> why not give it a try ;). define a and b as global variables, initialize them
> and try if a/b needs __udiv or not. you'll be surprised that no c-compiler
> (unless a buggy one) will even think about this sort of "constant"
> optimization.
I almost forgot to reply to this one. Of course you were right and optimizing
globals in this way doesn't make sense anyway. After noticing the unexpected
behavior, I disassembled the object file and saw that the result of the
division was used as an immediate value. Eventually, after this round of
email exchange, I realized that it was all due to my globals being declared as
"static", just for the sake of uniformity with other globals already there. =)
When globals' scope is limited to the compilation unit, the compiler may
optimize things out.
> OT: the use of volatile is ill-reputed and many people have complained
> about it throughout the history.
> http://lwn.net/Articles/233482/ [which ended up in:]
> http://kernel.org/doc/Documentation/volatile-considered-harmful.txt
>
> the only reason why we get away with it is because most of
> it is locked up behind accessor functions in include/io.h & dma.h
> and it should remain that way.
>
> > BTW, it seems you have some detailed technical documentation about the AR9170
> > at your disposal. I was wondering whether this is something available
> > publicly or not. Could be an interesting read at times. =)
> There are a lot of easy accessible docs about the CPU on the net. In fact you
> can even get Verilog/VHDL code of various SH2-clones without any problem.
> But that's about it, for information about the MAC/USB I have to rely on
> Chen.
I see.
Thanks again.
Ignacy
--
NO CARRIER
On Saturday 19 March 2011 10:22:37 Ignacy Gawedzki wrote:
> On Sat, Mar 19, 2011 at 03:03:06AM +0100, thus spake Christian Lamparter:
> > On Saturday 19 March 2011 02:22:44 Ignacy Gawedzki wrote:
> > > If you define the global variables as volatile, then the optimization is
> > > forbidden and you should get the bogus results.
> >
> > ???
> > No, "global" is enough. As long as you don't select "const".
> > (In fact there's more of a story behind "const volatile"
> > than you might think, or have you ever wondered why carl9170
> > still ships with gcc 4.4 instead of 4.5? Anyway that's OT)
>
> As a rule of thumb, I remembered that "volatile" tells the compiler "you're
> not the only one to manage that variable, don't make any assumptions about its
> current value", as could be the case if the variable is allocated in some
> shared memory segment/register/whatever and might change "on its own".
> Anyway, in the case at hand, it seems to make a difference.
why not give it a try ;). define a and b as global variables, initialize them
and try if a/b needs __udiv or not. you'll be surprised that no c-compiler
(unless a buggy one) will even think about this sort of "constant"
optimization.
OT: the use of volatile is ill-reputed and many people have complained
about it throughout the history.
http://lwn.net/Articles/233482/ [which ended up in:]
http://kernel.org/doc/Documentation/volatile-considered-harmful.txt
the only reason why we get away with it is because most of
it is locked up behind accessor functions in include/io.h & dma.h
and it should remain that way.
> BTW, it seems you have some detailed technical documentation about the AR9170
> at your disposal. I was wondering whether this is something available
> publicly or not. Could be an interesting read at times. =)
There are a lot of easy accessible docs about the CPU on the net. In fact you
can even get Verilog/VHDL code of various SH2-clones without any problem.
But that's about it, for information about the MAC/USB I have to rely on
Chen.
Best regards,
Christian
On Sat, Mar 19, 2011 at 01:43:26AM +0100, thus spake Christian Lamparter:
> I gave it a try and It worked?!
>
> [54670.551750] ieee80211 phy14: FW: a:0x4537 b:0x2c a/b:0x192 (0x192 = 402).
>
> (It was nothing else than just define "a" and "b" (as global variables) and
> add the INFO("a:0x%x b:0x%x a/b:0x%x", a, b, a/b);)
I suppose in your case the values are known at compile time and the compiler
does the calculation statically. If you define the global variables as
volatile, then the optimization is forbidden and you should get the bogus
results.
BTW, I used udivsi3_i4i.S from the Linux kernel, modified it as you indicated.
Please find my version attached.
Ignacy
--
To err is human, to purr feline.
On Saturday 19 March 2011 02:22:44 Ignacy Gawedzki wrote:
> On Sat, Mar 19, 2011 at 01:43:26AM +0100, thus spake Christian Lamparter:
> > I gave it a try and It worked?!
> >
> > [54670.551750] ieee80211 phy14: FW: a:0x4537 b:0x2c a/b:0x192 (0x192 = 402).
> >
> > (It was nothing else than just define "a" and "b" (as global variables) and
> > add the INFO("a:0x%x b:0x%x a/b:0x%x", a, b, a/b);)
>
> I suppose in your case the values are known at compile time and the compiler
> does the calculation statically.
sometimes its just down to "luck" ;)
> If you define the global variables as volatile, then the optimization is
> forbidden and you should get the bogus results.
???
No, "global" is enough. As long as you don't select "const".
(In fact there's more of a story behind "const volatile"
than you might think, or have you ever wondered why carl9170
still ships with gcc 4.4 instead of 4.5? Anyway that's OT)
> BTW, I used udivsi3_i4i.S from the Linux kernel, modified it as you indicated.
> Please find my version attached.
You are right, I can reproduce your problem with the attached udiv.
The crux is seems to be "hidden" inside the kernel's Makefile:
>udivsi3-y := udivsi3_i4i-Os.o
>
>ifneq ($(CONFIG_CC_OPTIMIZE_FOR_SIZE),y)
>udivsi3-$(CONFIG_CPU_SH3) := udivsi3_i4i.o
>udivsi3-$(CONFIG_CPU_SH4) := udivsi3_i4i.o
>endif
It looks like the version you are using is "made" for SH3+,
however the AR9170 has a SH2. (Jup, I can't believe it either :-D)
Best Regards,
Chr
(didn't really bother with the "signed" div.)
Hi,
On Sun, Jan 16, 2011 at 01:48:33PM +0100, thus spake Ignacy Gawedzki:
> On Sun, Jan 16, 2011 at 12:45:53PM +0100, thus spake Christian Lamparter:
> > No need for a hack, just get a udivsi3_i4i.S (a good place is the linux kernel
> > library sources in arch/sh/lib, you could also import one from the original
> > ar9170fw.git) and place it into carlfw/src. Next, you need to modify the
> > carlfw/CMakeLists.txt: add src/udivsi3_i4i.S to carl9170_lib_src and put
> > "set_source_files_properties(src/udivsi3_i4i.S PROPERTIES LANGUAGE C)"
> > a few lines further down.
> >
> > compile...
> >
> > [Note: you probably have to change the name of the __udivsi3_i4i symbol
> > to ___udivsi3_i4i too]
>
> Great, thanks for the hint, I'll try that as soon as I can.
So I finally got the time to test that solution. Compilation worked like a
charm indeed, thanks. All seemed fine until I ran that code. Indeed the
result of the division is completely off. :(
For instance, if I have A = 17719, B = 44, then A / B gives C = 12886, instead
of the expected 402. It is as though the result was multiplied by 32.
At this point, it is much simpler for me to simply return both A and B, and to
perform the division on the host's CPU instead of in the FW. Unless I commit
some gross error and there is a more elegant solution around, I'll stick with
that one, since I don't feel like debugging udivsi3_i4i. :/
Ignacy
--
Information wants to be beer, or something like that.