Hi Tom, On 05/31/12 19:17, Tom Warren wrote: > Igor, > >> -----Original Message----- >> From: Igor Grinberg [mailto:grinb...@compulab.co.il] >> Sent: Thursday, May 31, 2012 1:45 AM >> To: Stephen Warren >> Cc: Konstantin Sinyuk; Tom Warren; u-boot@lists.denx.de >> Subject: Re: [U-Boot] [PATCH V2] tegra: Compulab TrimSlice board support >> >> On 05/30/12 19:22, Stephen Warren wrote: >>> On 05/30/2012 09:35 AM, Konstantin Sinyuk wrote: >>>> Hi Stephen, >>>> >>>> Thanks for doing this! >>>> We highly appreciate your help. >>>> We (Igor and me) have several comments below... >>>> >>>> >>>> On 05/17/2012 07:03 PM, Stephen Warren wrote: >>> >>>>> diff --git a/board/compulab/dts/tegra2-trimslice.dts >>> >>>>> + usb@c5004000 { >>>>> + status = "disabled"; >>>>> + }; >>>>> + >>>>> + usb@c5004000 { >>>>> + status = "disabled"; >>>>> + }; >>>> >>>> This looks like a typo? >>> >>> Yes indeed. I'll send a patch to fix that up. >>> >>>>> diff --git a/board/compulab/trimslice/Makefile >>> >>>>> +# You should have received a copy of the GNU General Public >>>>> +License # along with this program; if not, write to the Free >>>>> +Software # Foundation, Inc., 59 Temple Place, Suite 330, Boston, # >>>>> +MA 02111-1307 USA >>>> >>>> Can we, please not have the postal address here and all other files? >>> >>> Hmm. Well, all the existing code I'm basing these patches on has that >>> license header. I don't feel strongly enough to change it now this >>> patch is checked in. Feel free to submit a patch to clean this up, but >>> if you do, I'd prefer if you made everything Tegra-related consistent. >> >> Ok, I see, the reason I don't like the postal addresses was explained by >> Russell King some time ago (I can't find it right now) and was about the >> postal address is subject to change without prior notice (and it did several >> times before) and that will leave majority of files with the wrong >> address... I consider it a bad practice. >> But I don't really insist on this. >> >> Now you say, that the patch has already been checked in... >> Tom, can we have a short notice on this, may be an email (e.g. Applied, >> thanks!) to the list, so we know which patches went in and which are still >> pending? >> I know we can look at your repository, but still... > > You'll see a pull request to Albert Aribaud when I've collected enough > patches to go into u-boot-tegra/master, > and if your patch (or a patch you care about) has been checked in, you'll see > it there.
Yes, but it will also be useful to know when you apply the patch to your tree, but please see below... > I can't keep track of every contributor nor every Tegra board and remember to > email them individually > when code that affects them has been checked in. And that is not what I was asking for... I'm not asking if you can CC everybody you think is interested in the patch... No! I'm asking if you can do a "Reply to all" to the patch (or a patch set) saying that the patch has been applied? Many other maintainers do this (not only in U-Boot) and it is really convenient to have that short email. > I can try to remember to add you to the CC when I send the pull request, but > no guarantees. ;) Thanks. > > Also, if you are monitoring the mailing list, you can search for TrimSlice > commits/discussions, etc. in each digest, on PatchWork, or gmane/pipermail, > etc. Well, monitoring mailing list will not help, if you don't "reply to all" (or at least reply to the list). I you had replied to the list, I would have no questions... ;-) > > Tom >> >>> >>>>> +include $(TOPDIR)/config.mk >>>>> + >>>>> +ifneq ($(OBJTREE),$(SRCTREE)) >>>>> +$(shell mkdir -p $(obj)../../nvidia/common) endif >>>>> + >>>>> +LIB = $(obj)lib$(BOARD).o >>>>> + >>>>> +COBJS := $(BOARD).o >>>>> +COBJS += ../../nvidia/common/board.o >>>>> + >>>> >>>> I feel that the common board.c file should be in the common SoC >>>> location, like arch/arm/cpu/armv7/tegra2/ ? >>>> In fact there is already a board.c file there, so how about we move >>>> the common stuff there? >>> >>> Yes, the Tegra board support is evolving and may not be optimally >>> structured right now. The logic above is what's needed given the >>> current state. Perhaps this will move to a better location in the >>> future. I don't plan on making that change right now though; I don't >>> want to conflict with other changes I know people are making such as >>> moving a bunch of code around to support a separate SPL. >> >> Ok. >> >>> >>>>> diff --git a/include/configs/trimslice.h >>>>> b/include/configs/trimslice.h >>> >>>>> +/* High-level configuration options */ >>>>> +#define V_PROMPT "Tegra2 (TrimSlice) # " >>>> >>>> Can this, please be "TrimSlice #" >>> >>> That wouldn't be consistent with any of the other Tegra boards though. >>> Is there a specific need for the prompt to be different? >> >> I see and probably you are right... >> On the second thought, it will not be consistent with other CompuLab boards >> (which are not Tegra based), so this whole consistency thing... >> I don't know... Let it be... After all, you have volunteered to maintain it >> :-) >> >>> >>>>> +#define CONFIG_TEGRA2_BOARD_STRING "NVIDIA Trimslice" >>>> >>>> Can this, please be "CompuLab TrimSlice" >>> >>> Sure, that's just something I forgot to update when copying from >>> another board's config file. >> >> Thanks >> >>> >>>>> +/* Board-specific serial config */ >>>>> +#define CONFIG_SERIAL_MULTI >>>>> +#define CONFIG_TEGRA2_ENABLE_UARTA >>>>> +#define CONFIG_TEGRA2_UARTA_GPU >>>>> +#define CONFIG_SYS_NS16550_COM1 NV_PA_APB_UARTA_BASE >>>>> + >>>>> +#define CONFIG_MACH_TYPE MACH_TYPE_TRIMSLICE >>>>> +#define CONFIG_SYS_BOARD_ODMDATA 0x300c0011 /* lp?, 1GB, UARTA */ >>>> >>>> In our downstream U-Boot, ODMDATA is 0x300d8011 /* lp1, 1GB, UARTA*/ >>>> Are you certain that your value is the correct one? >>> >>> Yes, 300d8011 is wrong, and you're just getting lucky that it's >>> working for you since nothing actually uses the ODMDATA fields that are >> wrong. >>> >>> The "d8" in the ODMDATA means to use UART D as the debug UART. >>> However, TrimSlice uses UART A, and hence needs "c0" here. >>> >>> Without this change, the mainline kernel config option >>> CONFIG_TEGRA_DEBUG_UART_AUTO_ODMDATA (which affects where >>> DEBUG_LL/earlyprintk output is routed) will not work correctly. >> >> Ok, I see, thanks for sharing this information. >> >>> >>>>> +/* USB networking support */ >>>>> +#define CONFIG_USB_HOST_ETHER >>>>> +#define CONFIG_USB_ETHER_SMSC95XX >>>>> +#define CONFIG_USB_ETHER_ASIX >>>> >>>> TrimSlice does not have any on-board Ethernet over USB devices. >>>> Do you connect them through the USB port? Or is it just a copy/paste >>>> from another board? >>> >>> A later change has removed the SMSC95XX support since as you say, that >>> chip is not present on the board. However, I actively use an ASIX USB >>> Ethernet dongle with TrimSlice, so do need that config option enabled. >>> Perhaps it can be removed if/when Tegra PCIe support is added to >>> mainline U-Boot, so the built-in Ethernet adapter can be used instead. >> >> Fare enough. >> Thanks for the patches, information and the work you've done! >> >> >> -- >> Regards, >> Igor. -- Regards, Igor. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
