Re: [U-Boot] [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
On 6/16/2014 1:00 PM, u-boot-request@lists.denx.de wrote:
Message: 30 Date: Mon, 16 Jun 2014 10:05:08 +0300 From: Igor Grinberg grinberg@compulab.co.il Subject: Re: [U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL To: Otavio Salvador otavio@ossystems.com.br, U-Boot Mailing List u-boot@lists.denx.de Cc: Marek Vasut marex@denx.de, Fabio Estevam fabio.estevam@freescale.com Message-ID: 539E9724.4020809@compulab.co.il 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.
On Monday, June 16, 2014 at 04:33:09 PM, Nikolay Dimitrov wrote:
Hi Igor,
Please do not top-post.
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.
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
On 6/16/2014 1:00 PM, u-boot-request@lists.denx.de wrote:
Message: 30 Date: Mon, 16 Jun 2014 10:05:08 +0300 From: Igor Grinberg grinberg@compulab.co.il Subject: Re: [U-Boot] [PATCH 1/6] usb: ehci: mx6: Add support for i.MX6SL To: Otavio Salvador otavio@ossystems.com.br, U-Boot Mailing List u-boot@lists.denx.de Cc: Marek Vasut marex@denx.de, Fabio Estevam fabio.estevam@freescale.com Message-ID: 539E9724.4020809@compulab.co.il 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
Hi Marek,
On 6/16/2014 5:53 PM, Marek Vasut wrote:
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).
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.
Best regards, Marek Vasut
Thanks for taking your time to reply.
Kind regards, Nikolay
On Tuesday, June 17, 2014 at 06:21:05 PM, Nikolay Dimitrov wrote:
Hi Marek,
On 6/16/2014 5:53 PM, Marek Vasut wrote:
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
Hi Nikolay,
On 06/16/14 17:33, Nikolay Dimitrov wrote:
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.
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...
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.
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.
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?
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.
Hi Igor,
On 6/17/2014 9:26 AM, Igor Grinberg wrote:
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.
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.
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
Hi Nikolay,
On 06/17/14 20:49, Nikolay Dimitrov wrote:
Hi Igor,
On 6/17/2014 9:26 AM, Igor Grinberg wrote:
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.
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.
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.
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.
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.
participants (3)
-
Igor Grinberg -
Marek Vasut -
Nikolay Dimitrov