usb: ehci: mx6: Add support for i.MX6SL
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL
Hi Igor,
My personal opinion is that unless you intend to run the binary on multiple IMX6 variants, there's no
need to do expensive checks in runtime, when you can do the same at compile-time. For me it's the
same as choosing puts() vs printf() - you know at compile time whether you need to print arguments
or not, so same with the USB controller base address - you know in advance that you target a specific
CPU variant and not the other ones.
To a certain extent I agree that it would be awesome to have the same code running on all IMX6
variants, but the run-time checks will increase somewhat the binary footprint and U-Boot community has
already gone to great efforts to remove unnecessary bloat. My personal assumption is that such generic
approach would be more tolerated in the Linux kernel.
Kind regards,
Nikolay
Quoted 12 lines · u-boot-request at lists.denx.de
Message: 30 Date: Mon, 16 Jun 2014 10:05:08 +0300 From: Igor Grinberg <[email protected]> Subject: Re: [U-Boot] [PATCH 1/6] usb: ehci:
mx6: Add support for i.MX6SL To: Otavio Salvador <[email protected]>, U-Boot Mailing List <[email protected]>
Cc: Marek Vasut <[email protected]>, Fabio Estevam <[email protected]> Message-ID:
<[email protected]> Content-Type: text/plain; charset=ISO-8859-1 Hmmm... Can't this be done dynamically? I mean... you can check the SoC type in runtime, right? And then substitute the correct address in a variable or so... I would really prefer such kind of things done in runtime.
-- Regards, Igor.
PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL
Quoted 1 line · Nikolay Dimitrov
Hi Igor,
Please do not top-post.
Quoted 4 lines
My personal opinion is that unless you intend to run the binary on multiple IMX6 variants, there's no
need to do expensive checks in runtime, when you can do the same at compile-time.
With the upcoming DM implementation, that's what we plan. Moreover, the checks are not expensive, you just decide the address based on the CPU model and assign it into some variable. You then access the registers via that variable + offset, as usual.
Quoted 15 lines
For me it's the
same as choosingputs()vsprintf()- you know at compile time whether you need to print arguments
or not, so same with the USB controller base address - you know in advance that you target a specific
CPU variant and not the other ones.To a certain extent I agree that it would be awesome to have the same code running on all IMX6
variants, but the run-time checks will increase somewhat the binary footprint and U-Boot community has
already gone to great efforts to remove unnecessary bloat. My personal assumption is that such generic
approach would be more tolerated in the Linux kernel.
Quoted 16 lines
Kind regards,
NikolayQuoted 12 lines · u-boot-request at lists.denx.de
Message: 30 Date: Mon, 16 Jun 2014 10:05:08 +0300 From: Igor Grinberg <[email protected]> Subject: Re: [U-Boot] [PATCH 1/6] usb: ehci:
mx6: Add support for i.MX6SL To: Otavio Salvador <[email protected]>, U-Boot Mailing List <[email protected]>
Cc: Marek Vasut <[email protected]>, Fabio Estevam <[email protected]> Message-ID:
<[email protected]> Content-Type: text/plain; charset=ISO-8859-1 Hmmm... Can't this be done dynamically? I mean... you can check the SoC type in runtime, right? And then substitute the correct address in a variable or so... I would really prefer such kind of things done in runtime.
-- Regards, Igor.
Best regards,
Marek Vasut
PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL
Hi Marek,
Quoted 1 line · Marek Vasut
Please do not top-post.
Please excuse me - I was unable to find posting guidelines for the mailing list, so just posted by habit (last time I had to post to another list, they wanted me to top-post).
Quoted 4 lines
With the upcoming DM implementation, that's what we plan. Moreover, the checks are not expensive, you just decide the address based on the CPU model and assign it into some variable. You then access the registers via that variable + offset, as usual.
I see. Would be great to see this compatibility coming.
Quoted 2 lines
Best regards,
Marek Vasut
Thanks for taking your time to reply.
Kind regards,
Nikolay
PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL
Quoted 8 lines · Nikolay Dimitrov
Hi Marek,
Quoted 1 line · Marek Vasut
Please do not top-post.
Please excuse me - I was unable to find posting guidelines for the mailing list, so just posted by habit (last time I had to post to another list, they wanted me to top-post).
Then follow RFC1855, quote:
"
- If you are sending a reply to a message or a posting be sure you summarize the original at the top of the message, or include just enough text of the original to give a context. This will make sure readers understand when they start to read your response.
"
;-)
Best regards,
Marek Vasut
PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL
In reply to Nikolay DimitrovHi Nikolay,
Quoted 3 lines · Nikolay Dimitrov
Hi Igor,
My personal opinion is that unless you intend to run the binary on multiple IMX6 variants,
That is exactly what we do already (code is on the way) and IMO what we should aim for.
Quoted 2 lines
there's no
need to do expensive checks in runtime, when you can do the same at compile-time.
Why do you think it is expensive? Any benchmarks? Also, the solution is what Marek already said, in short:
do the check once and store the result for future use...
Quoted 4 lines
For me it's the
same as choosingputs()vsprintf()- you know at compile time whether you need to print arguments or not, so same with the USB controller base address - you know in advance that you target a specific CPU variant and not the other ones.
For me it is just an artificial complication which prevents single binary for i.MX6 based boards.
Don't get me wrong, I think that in your board code you can choose which approach you want, whether it will be single or multi binary, but this is i.MX6 (and possibly future i.MX*) USB code which can be used on many i.MX6 boards.
Quoted 4 lines
To a certain extent I agree that it would be awesome to have the same code running on all IMX6 variants, but the run-time checks will increase somewhat the binary footprint and U-Boot community has already gone to great efforts to remove unnecessary bloat.
Again, what are we talking about? A couple of bytes?
Quoted 2 lines
My personal assumption is that such generic approach would be more tolerated in the Linux kernel.
For Linux kernel this is the only acceptable way now.
Signature · 2 lines
Regards, Igor.
PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL
Hi Igor,
Quoted 2 lines · Igor Grinberg
That is exactly what we do already (code is on the way) and IMO what we should aim for.
I really didn't knew this in the beginning before seeing your answers, this would be definitely easier
to support.
Quoted 6 lines
For me it is just an artificial complication which prevents single binary for i.MX6 based boards.
Don't get me wrong, I think that in your board code you can choose which approach you want, whether it will be single or multi binary, but this is i.MX6 (and possibly future i.MX*) USB code which can be used on many i.MX6 boards.
I definitely like the idea, no argument on this.
Quoted 1 line
Again, what are we talking about? A couple of bytes?
I think it will cost 22 additional bytes per check. Here's a rough example:
(Code that just assigns the address to the struct pointer -> 10 bytes)
p = (mx6_usb_t*)0x55667788;
8380: f247 7388 movw r3, #30600 ; 0x7788
8384: f2c5 5366 movt r3, #21862 ; 0x5566
8388: 60fb str r3, [r7, #12](Code that checks in runtime the CPU type -> 32 bytes)
if (is_imx6q())
8380: f7ff ffee bl 8360 <is_imx6q>
8384: 4603 mov r3, r0
8386: 2b00 cmp r3, #0
8388: d005 beq.n 8396 <main+0x26>
{
p = (mx6_usb_t*)0x11223344;
838a: f243 3344 movw r3, #13124 ; 0x3344
838e: f2c1 1322 movt r3, #4386 ; 0x1122
8392: 60fb str r3, [r7, #12]
8394: e004 b.n 83a0 <main+0x30>
}
else
{
p = (mx6_usb_t*)0x55667788;
8396: f247 7388 movw r3, #30600 ; 0x7788
839a: f2c5 5366 movt r3, #21862 ; 0x5566
839e: 60fb str r3, [r7, #12]
}If I assume that we have 20 sub-systems in U-Boot, and each needs to check the CPU type
in 10 places, this makes 4400 bytes difference, which is roughly the size of a moderately
small driver in U-Boot. I need to say that I'm in no way expert in ARM assembly, so please
feel free to point out any mistakes in my assumptions.
Please don't get me wrong - I don't want to argue at all. I was just wondering about this topic
and decided to check with you guys just to be sure.
Kind regards,
Nikolay
PATCH 1/6 usb: ehci: mx6: Add support for i.MX6SL
Hi Nikolay,
Quoted 46 lines · Nikolay Dimitrov
Hi Igor,
Quoted 1 line · Igor Grinberg
That is exactly what we do already (code is on the way) and IMO what we should aim for.
I really didn't knew this in the beginning before seeing your answers, this would be definitely easier to support.
Quoted 6 lines
For me it is just an artificial complication which prevents single binary for i.MX6 based boards.
Don't get me wrong, I think that in your board code you can choose which approach you want, whether it will be single or multi binary, but this is i.MX6 (and possibly future i.MX*) USB code which can be used on many i.MX6 boards.I definitely like the idea, no argument on this.
Quoted 1 line
Again, what are we talking about? A couple of bytes?
I think it will cost 22 additional bytes per check. Here's a rough example:
(Code that just assigns the address to the struct pointer -> 10 bytes)
p = (mx6_usb_t*)0x55667788; 8380: f247 7388 movw r3, #30600 ; 0x7788 8384: f2c5 5366 movt r3, #21862 ; 0x5566 8388: 60fb str r3, [r7, #12](Code that checks in runtime the CPU type -> 32 bytes)
if (is_imx6q()) 8380: f7ff ffee bl 8360 <is_imx6q> 8384: 4603 mov r3, r0 8386: 2b00 cmp r3, #0 8388: d005 beq.n 8396 <main+0x26> { p = (mx6_usb_t*)0x11223344; 838a: f243 3344 movw r3, #13124 ; 0x3344 838e: f2c1 1322 movt r3, #4386 ; 0x1122 8392: 60fb str r3, [r7, #12] 8394: e004 b.n 83a0 <main+0x30> } else { p = (mx6_usb_t*)0x55667788; 8396: f247 7388 movw r3, #30600 ; 0x7788 839a: f2c5 5366 movt r3, #21862 ; 0x5566 839e: 60fb str r3, [r7, #12] }
Quoted 3 lines
If I assume that we have 20 sub-systems in U-Boot, and each needs to check the CPU type in 10 places, this makes 4400 bytes difference, which is roughly the size of a moderately small driver in U-Boot.
This looks like a very rough estimation.
It also can be improved by various ways.
Anyway, I think that even 4K addition is an acceptable compromise for a single binary.
Quoted 5 lines
I need to say that I'm in no way expert in ARM assembly, so please feel free to point out any mistakes in my assumptions.
Please don't get me wrong - I don't want to argue at all. I was just wondering about this topic and decided to check with you guys just to be sure.
No problem.
Signature · 2 lines
Regards, Igor.