Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
Wolfgang, On Mon, Jan 17, 2011 at 4:27 PM, Wolfgang Denk w...@denx.de wrote: Dear Tom Warren, In message AANLkTi=f-14jnam_dy5c1sgtt0nmqm-pnok6gdg4d...@mail.gmail.com you wrote: 1) IO access functions - I pre-reviewed my patch series with Wolfgang (to hopefully catch any blatant errors and smooth the process) and he indicated that C structs and I/O accessor funcs or macros were preferred to my base+offset original code. I don't think I said preferred. I apologize if I really should have been unclear. Fact is, the use of I/O accessors is mandatory for any new code going in. Sorry, I misunderstood what really constituted I/O accessors. Fixed in V3 (coming soon). Since the ARM is 32-bit, and all of our registers are I/O mapped, it made sense just to cast the necessary HW mem-mapped regs as volatile structs and access the members directly. Works well, is easy to read understand, etc. Let me know (with examples, if possible) how I can make it better. It does NOT make sense. Please read the linux/Documentation/volatile-considered-harmful.txt document, and see recent dicussion in the ARM: Avoid compiler optimization for usages of readb, writeb and friends. thread about what happens when you use just volatile pointers. 2) Compiling out support for UARTA or UARTD - didn't seem necessary - size isn't an issue at this point with Tegra2 U-Boot, Maybe you don't care, but we do. Please see http://www.denx.de/wiki/U-Boot/DesignPrinciples, item 5 Done in V3 patch. I was going to respond to your review w/a direct, inline reply, but I thought it better to get the V2 patch out there before the weekend (we're off for MLK, as well). I'm under some pressure to get a baseline Tegra2 patchset in before the merge window closes. I'll be sure to respond to each issue directly on the list in the future, though. Please also make sure to maintain a changelog of in each of your patches (see second bullet in http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions). OK. I added a V2/V3 changelog to each patch. Hope I got it right. New patchset incoming. Best regards, Wolfgang Denk Thanks for your patience, Tom -- 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: w...@denx.de IMPORTANT NOTICE TO PURCHASERS: The Entire Physical Universe, Inclu- ding This Product, May One Day Collapse Back into an Infinitesimally Small Space. Should Another Universe Subsequently Re-emerge, the Existence of This Product in That Universe Cannot Be Guaranteed. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
Dear Tom Warren, In message AANLkTi=f-14jnam_dy5c1sgtt0nmqm-pnok6gdg4d...@mail.gmail.com you wrote: 1) IO access functions - I pre-reviewed my patch series with Wolfgang (to hopefully catch any blatant errors and smooth the process) and he indicated that C structs and I/O accessor funcs or macros were preferred to my base+offset original code. I don't think I said preferred. I apologize if I really should have been unclear. Fact is, the use of I/O accessors is mandatory for any new code going in. Since the ARM is 32-bit, and all of our registers are I/O mapped, it made sense just to cast the necessary HW mem-mapped regs as volatile structs and access the members directly. Works well, is easy to read understand, etc. Let me know (with examples, if possible) how I can make it better. It does NOT make sense. Please read the linux/Documentation/volatile-considered-harmful.txt document, and see recent dicussion in the ARM: Avoid compiler optimization for usages of readb, writeb and friends. thread about what happens when you use just volatile pointers. 2) Compiling out support for UARTA or UARTD - didn't seem necessary - size isn't an issue at this point with Tegra2 U-Boot, Maybe you don't care, but we do. Please see http://www.denx.de/wiki/U-Boot/DesignPrinciples, item 5 I was going to respond to your review w/a direct, inline reply, but I thought it better to get the V2 patch out there before the weekend (we're off for MLK, as well). I'm under some pressure to get a baseline Tegra2 patchset in before the merge window closes. I'll be sure to respond to each issue directly on the list in the future, though. Please also make sure to maintain a changelog of in each of your patches (see second bullet in http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions). 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: w...@denx.de IMPORTANT NOTICE TO PURCHASERS: The Entire Physical Universe, Inclu- ding This Product, May One Day Collapse Back into an Infinitesimally Small Space. Should Another Universe Subsequently Re-emerge, the Existence of This Product in That Universe Cannot Be Guaranteed. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
Dear Tom Warren, In message aanlktikcq_0cnfrj_5s2zshcj-2qyvi24yeh0dcqe...@mail.gmail.com you wrote: So instead of, say uart-lcr = 0, you'd prefer writel(0, uart-lcr), where writel = __arch_putl(v, a) = (*(volatile unsigned int *)(a) = (v))? Please see the thread ARM: Avoid compiler optimization for usages of readb, writeb and friends. Is that different enough from 'uart-lcr = 0' to warrant the change? Yes, it is - in recent (i. e. fixed) code. Does it add some HW barriers or forced read-before-write that the 'volatile' struct doesn't? Yes, it does. See above. 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: w...@denx.de Conquest is easy. Control is not. -- Kirk, Mirror, Mirror, stardate unknown ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
Hi Tom, On Fri, 2011-01-14 at 10:11 -0700, Tom Warren wrote: This series of patches adds preliminary/baseline support for NVIDIA's Tegra2 SoC. Basic CPU (AVP), RAM and UART init are covered so that the system (Harmony or Seaboard) can boot to the U-Boot serial cmd prompt. Further support (for Cortex-A9 CPU(s), USB, SD/MMC, etc.) to follow. V2: Make changes based on feedback from Peter Tyser and Sandeep Paulraj. If you didn't use all the feedback to the original patches, you should state explicitly what you changed here, or respond to the original comment email as to why they weren't made. For example, I see you didn't make the suggested change to use IO access functions, or allow compiling out of support for UARTA and UARTD. That should be made clear somewhere (and the logic of why the changes weren't made) so that those reviewing the patches know what changed between v1 and v2 and why. As is its unclear why the v1 comments weren't implemented, so they also apply to this series. Best, Peter ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
On Fri, Jan 14, 2011 at 12:59 PM, Peter Tyser pty...@xes-inc.com wrote: Hi Tom, On Fri, 2011-01-14 at 10:11 -0700, Tom Warren wrote: This series of patches adds preliminary/baseline support for NVIDIA's Tegra2 SoC. Basic CPU (AVP), RAM and UART init are covered so that the system (Harmony or Seaboard) can boot to the U-Boot serial cmd prompt. Further support (for Cortex-A9 CPU(s), USB, SD/MMC, etc.) to follow. V2: Make changes based on feedback from Peter Tyser and Sandeep Paulraj. If you didn't use all the feedback to the original patches, you should state explicitly what you changed here, or respond to the original comment email as to why they weren't made. For example, I see you didn't make the suggested change to use IO access functions, or allow compiling out of support for UARTA and UARTD. That should be made clear somewhere (and the logic of why the changes weren't made) so that those reviewing the patches know what changed between v1 and v2 and why. As is its unclear why the v1 comments weren't implemented, so they also apply to this series. Peter, Sorry, since this is my first patch series to U-Boot upstream, I'm still learning the proper etiquette. Let me respond here rather than in the patch comments since there are only 2 unchanged areas WRT comments. 1) IO access functions - I pre-reviewed my patch series with Wolfgang (to hopefully catch any blatant errors and smooth the process) and he indicated that C structs and I/O accessor funcs or macros were preferred to my base+offset original code. Since the ARM is 32-bit, and all of our registers are I/O mapped, it made sense just to cast the necessary HW mem-mapped regs as volatile structs and access the members directly. Works well, is easy to read understand, etc. Let me know (with examples, if possible) how I can make it better. 2) Compiling out support for UARTA or UARTD - didn't seem necessary - size isn't an issue at this point with Tegra2 U-Boot, and some boards (Harmony, for example) are populated w/hardware for both UARTA and UARTD, and can have both on at U-Boot runtime (perhaps for debug out to UARTA whilst normal console I/O goes to UARTD), so I chose to leave the init code for both intact. Plus I've never liked code with too many unnecessary ifdef's - makes it less readable, IMO. I was going to respond to your review w/a direct, inline reply, but I thought it better to get the V2 patch out there before the weekend (we're off for MLK, as well). I'm under some pressure to get a baseline Tegra2 patchset in before the merge window closes. I'll be sure to respond to each issue directly on the list in the future, though. Thanks for the thorough review, Tom Best, Peter ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
On Fri, 2011-01-14 at 13:41 -0700, Tom Warren wrote: On Fri, Jan 14, 2011 at 12:59 PM, Peter Tyser pty...@xes-inc.com wrote: Hi Tom, On Fri, 2011-01-14 at 10:11 -0700, Tom Warren wrote: This series of patches adds preliminary/baseline support for NVIDIA's Tegra2 SoC. Basic CPU (AVP), RAM and UART init are covered so that the system (Harmony or Seaboard) can boot to the U-Boot serial cmd prompt. Further support (for Cortex-A9 CPU(s), USB, SD/MMC, etc.) to follow. V2: Make changes based on feedback from Peter Tyser and Sandeep Paulraj. If you didn't use all the feedback to the original patches, you should state explicitly what you changed here, or respond to the original comment email as to why they weren't made. For example, I see you didn't make the suggested change to use IO access functions, or allow compiling out of support for UARTA and UARTD. That should be made clear somewhere (and the logic of why the changes weren't made) so that those reviewing the patches know what changed between v1 and v2 and why. As is its unclear why the v1 comments weren't implemented, so they also apply to this series. Peter, Sorry, since this is my first patch series to U-Boot upstream, I'm still learning the proper etiquette. Let me respond here rather than in the patch comments since there are only 2 unchanged areas WRT comments. No worries. I'm glad to see you're pushing your changes upstream. 1) IO access functions - I pre-reviewed my patch series with Wolfgang (to hopefully catch any blatant errors and smooth the process) and he indicated that C structs and I/O accessor funcs or macros were preferred to my base+offset original code. Since the ARM is 32-bit, and all of our registers are I/O mapped, it made sense just to cast the necessary HW mem-mapped regs as volatile structs and access the members directly. Works well, is easy to read understand, etc. Let me know (with examples, if possible) how I can make it better. Both Linux and U-Boot recommend using IO access functions instead pointer accesses, at least in PPC-land, and even for memory mapped registers. I'm not too familiar with ARM, but assume they have the same recommendation. If pointers are used, some CPUs may optimize the access order, thus causing problems. Using IO accessors also ensures that the code is portable. Even if the Tegra doesn't re-order accesses, a driver you write for use in a Tegra could be used on other CPUs that do. eg: http://www.mail-archive.com/u-boot@lists.denx.de/msg18435.html http://lists.denx.de/pipermail/u-boot/2007-December/027595.html It looks like other ARM processors use IO accessors too, eg the recent armada CPU: arch/arm/cpu/arm926ejs/armada100/* A grep of writel in arch/arm shows a number of references. Similarly in the Linux code in arch/arm. ARM maintainers, feel free to chime in if you have comments. 2) Compiling out support for UARTA or UARTD - didn't seem necessary - size isn't an issue at this point with Tegra2 U-Boot, and some boards (Harmony, for example) are populated w/hardware for both UARTA and UARTD, and can have both on at U-Boot runtime (perhaps for debug out to UARTA whilst normal console I/O goes to UARTD), so I chose to leave the init code for both intact. Plus I've never liked code with too many unnecessary ifdef's - makes it less readable, IMO. Yeah, this is grey - I see both sides of the coin. U-Boot generally strives to be as small as possible. You may use a large flash on your eval boards and not care about space, but other system designers may place as small of flash as possible to save cost. It looked like UART_A-UART_E were also going to be supported at some time, which is quite a chunk of code. You also already are defining which serial ports are available at compile time via CONFIG_TEGRA2_ENABLE_UARTA and CONFIG_TEGRA2_ENABLE_UARTD. Anyway, I don't really care much which way you go, just wanted to make sure you were making a conscious decision to not add the ifdefs. I was going to respond to your review w/a direct, inline reply, but I thought it better to get the V2 patch out there before the weekend (we're off for MLK, as well). I'm under some pressure to get a baseline Tegra2 patchset in before the merge window closes. I'll be sure to respond to each issue directly on the list in the future, though. As long as you initially submit your patches during the merge window they will generally be merged, even if there are multiple revisions and discussion that lasts past the end of official merge window. So this stuff should get in. Best, Peter ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
On Fri, Jan 14, 2011 at 2:36 PM, Peter Tyser pty...@xes-inc.com wrote: On Fri, 2011-01-14 at 13:41 -0700, Tom Warren wrote: On Fri, Jan 14, 2011 at 12:59 PM, Peter Tyser pty...@xes-inc.com wrote: Hi Tom, On Fri, 2011-01-14 at 10:11 -0700, Tom Warren wrote: This series of patches adds preliminary/baseline support for NVIDIA's Tegra2 SoC. Basic CPU (AVP), RAM and UART init are covered so that the system (Harmony or Seaboard) can boot to the U-Boot serial cmd prompt. Further support (for Cortex-A9 CPU(s), USB, SD/MMC, etc.) to follow. V2: Make changes based on feedback from Peter Tyser and Sandeep Paulraj. If you didn't use all the feedback to the original patches, you should state explicitly what you changed here, or respond to the original comment email as to why they weren't made. For example, I see you didn't make the suggested change to use IO access functions, or allow compiling out of support for UARTA and UARTD. That should be made clear somewhere (and the logic of why the changes weren't made) so that those reviewing the patches know what changed between v1 and v2 and why. As is its unclear why the v1 comments weren't implemented, so they also apply to this series. Peter, Sorry, since this is my first patch series to U-Boot upstream, I'm still learning the proper etiquette. Let me respond here rather than in the patch comments since there are only 2 unchanged areas WRT comments. No worries. I'm glad to see you're pushing your changes upstream. 1) IO access functions - I pre-reviewed my patch series with Wolfgang (to hopefully catch any blatant errors and smooth the process) and he indicated that C structs and I/O accessor funcs or macros were preferred to my base+offset original code. Since the ARM is 32-bit, and all of our registers are I/O mapped, it made sense just to cast the necessary HW mem-mapped regs as volatile structs and access the members directly. Works well, is easy to read understand, etc. Let me know (with examples, if possible) how I can make it better. Both Linux and U-Boot recommend using IO access functions instead pointer accesses, at least in PPC-land, and even for memory mapped registers. I'm not too familiar with ARM, but assume they have the same recommendation. If pointers are used, some CPUs may optimize the access order, thus causing problems. Using IO accessors also ensures that the code is portable. Even if the Tegra doesn't re-order accesses, a driver you write for use in a Tegra could be used on other CPUs that do. eg: http://www.mail-archive.com/u-boot@lists.denx.de/msg18435.html http://lists.denx.de/pipermail/u-boot/2007-December/027595.html It looks like other ARM processors use IO accessors too, eg the recent armada CPU: arch/arm/cpu/arm926ejs/armada100/* A grep of writel in arch/arm shows a number of references. Similarly in the Linux code in arch/arm. So instead of, say uart-lcr = 0, you'd prefer writel(0, uart-lcr), where writel = __arch_putl(v, a) = (*(volatile unsigned int *)(a) = (v))? Is that different enough from 'uart-lcr = 0' to warrant the change? Does it add some HW barriers or forced read-before-write that the 'volatile' struct doesn't? I've done a ton of embedded work, but all in x86 asm (and C) on PCs, so pardon my ignorance questions. ARM maintainers, feel free to chime in if you have comments. 2) Compiling out support for UARTA or UARTD - didn't seem necessary - size isn't an issue at this point with Tegra2 U-Boot, and some boards (Harmony, for example) are populated w/hardware for both UARTA and UARTD, and can have both on at U-Boot runtime (perhaps for debug out to UARTA whilst normal console I/O goes to UARTD), so I chose to leave the init code for both intact. Plus I've never liked code with too many unnecessary ifdef's - makes it less readable, IMO. Yeah, this is grey - I see both sides of the coin. U-Boot generally strives to be as small as possible. You may use a large flash on your eval boards and not care about space, but other system designers may place as small of flash as possible to save cost. It looked like UART_A-UART_E were also going to be supported at some time, which is quite a chunk of code. You also already are defining which serial ports are available at compile time via CONFIG_TEGRA2_ENABLE_UARTA and CONFIG_TEGRA2_ENABLE_UARTD. Anyway, I don't really care much which way you go, just wanted to make sure you were making a conscious decision to not add the ifdefs. As conscious I get before noon. ;) I was going to respond to your review w/a direct, inline reply, but I thought it better to get the V2 patch out there before the weekend (we're off for MLK, as well). I'm under some pressure to get a baseline Tegra2 patchset in before the merge window closes. I'll be sure to respond to each issue directly on the list in the future, though. As long as you initially submit your patches during the merge
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
Hello, Le 14/01/2011 23:39, Tom Warren a écrit : So instead of, say uart-lcr = 0, you'd prefer writel(0, uart-lcr), where writel = __arch_putl(v, a) = (*(volatile unsigned int *)(a) = (v))? Is that different enough from 'uart-lcr = 0' to warrant the change? Does it add some HW barriers or forced read-before-write that the 'volatile' struct doesn't? writel() and readl() do not introduce read-before-write, that is, they do not perform any more than what their names imply, but yes they do introduce barriers, or more precisely, they force the compiler to do so. Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
On Sat, 2011-01-15 at 00:00 +0100, Albert ARIBAUD wrote: Hello, Le 14/01/2011 23:39, Tom Warren a écrit : So instead of, say uart-lcr = 0, you'd prefer writel(0, uart-lcr), where writel = __arch_putl(v, a) = (*(volatile unsigned int *)(a) = (v))? Is that different enough from 'uart-lcr = 0' to warrant the change? Does it add some HW barriers or forced read-before-write that the 'volatile' struct doesn't? writel() and readl() do not introduce read-before-write, that is, they do not perform any more than what their names imply, but yes they do introduce barriers, or more precisely, they force the compiler to do so. Agreed, I should have dug deeper. On PPC we use out_be32() or similar to access memory mapped registers, which does have an explicit barrier. I'm not familiar with ARM so don't know what the proper access functions are, but it looks like the defacto standard writel()/readl(). I'd personally use writel()/readl() in this patch. Functionally it may be the same as your volatile accesses, but its the generally recommended practice. It looks like most of the Tegra support in the Linux kernel also uses writel()/readl() for what its worth. Ultimately its up to the ARM U-Boot maintainer to decide if they require the access functions or not - I'm just giving my opinions from PPC experience. Regards, Peter ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 0/4 V2] Add basic NVIDIA Tegra2 SoC support
Dear Peter Tyser, In message 1295047406.29642.6729.camel@petert you wrote: Agreed, I should have dug deeper. On PPC we use out_be32() or similar to access memory mapped registers, which does have an explicit barrier. I'm not familiar with ARM so don't know what the proper access functions are, but it looks like the defacto standard writel()/readl(). I have a patch waiting o be applied that adds proper memory barriers for ARM, too. They are necessary. I'd personally use writel()/readl() in this patch. Functionally it may be the same as your volatile accesses, but its the generally recommended practice. It looks like most of the Tegra support in the Linux kernel also uses writel()/readl() for what its worth. Ultimately its up to the ARM U-Boot maintainer to decide if they require the access functions or not - I'm just giving my opinions from PPC experience. It's not only recommended, it is mandatory. I/O accesors _MUST_ be used. 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: w...@denx.de The most difficult thing in the world is to know how to do a thing and to watch someone else doing it wrong, without commenting. -- T.H. White ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
