[U-Boot] [PATCH V2] tegra: Compulab TrimSlice board support

[Igor Grinberg]

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 at c5004000 {
>>> +        status = "disabled";
>>> +    };
>>> +
>>> +    usb at 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...

> 
>>> +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.