From mboxrd@z Thu Jan  1 00:00:00 1970
From: micro1183 <micro1183@gmail.com>
Date: Mon, 27 Jan 2014 11:40:44 +0100
Subject: [U-Boot] [RFC PATCH 1/1] Add support for pengwyn board
In-Reply-To: <20140124141254.EEE0B3801AD@gemini.denx.de>
References: <52E2615C.1000207@gmail.com>
	<20140124141254.EEE0B3801AD@gemini.denx.de>
Message-ID: <52E637AC.7040800@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Wolfgang,

thanks for your feedback.

On 01/24/2014 03:12 PM, Wolfgang Denk wrote:
> Dear micro1183,
> 
> In message <52E2615C.1000207@gmail.com> you wrote:
>> This patch adds support for the silica pengwyn board with AM335x SoC
> 
> Your patch is line-wrapped and does not apply.  Please fix your mailer
> configuration.

I tried to fix my settings and will test it by sending a patch to myself
using git send-email.

>> +/*
>> + * board.c
>> + *
>> + * Copyright (C) 2013 Lothar Felten <lothar.felten@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
> 
> Please use a SPDX license tag instead.  And please notice that new code
> should have GPL-2.0+; GPL-2.0 is not sufficient.
> 
> Please fix globally.

ok, done. I use a single line SPDX identifier for GPL-2.0+ now.

>> +#include <common.h>
>> +#include "board.h"
> 
> Do you really need all these headers?

cleaned them up now.

>> +#define MACH_TYPE_PENGWYN		1234
> 
> MACH-ID 1234 is registered for the gene1270 bord.  Please do not
> hijack other board's IDs!  Register your own instead, please.

Where can I find a list of IDs?
I can't find much information on a gene1270 board in
arch/arm/tools/mach-types.
Is a MACH_TYPE identifier mandatory?

> 
>> +/* Always 128 KiB env size */
>> +#define CONFIG_ENV_SIZE			(128 << 10)
> 
> This makes no sense.  You will never need that much, and such a huge
> size just slows down booting etc.

Thanks. Set to 0x4000.

>> +/* NS16550 Configuration */
>> +#define CONFIG_SYS_NS16550_COM1		0x44e09000	/* Base EVM has UART0 */
>> +#define CONFIG_SYS_NS16550_COM2		0x48022000	/* UART1 */
>> +#define CONFIG_SYS_NS16550_COM3		0x48024000	/* UART2 */
>> +#define CONFIG_SYS_NS16550_COM4		0x481a6000	/* UART3 */
>> +#define CONFIG_SYS_NS16550_COM5		0x481a8000	/* UART4 */
>> +#define CONFIG_SYS_NS16550_COM6		0x481aa000	/* UART5 */
> 
> Do you really need all of these?

No, removed all but the console one.

>> +#define CONFIG_USBNET_HOST_ADDR	"de:ad:be:af:00:00"
> 
> Is this a good idea?
>> +/* USB TI's IDs */
>> +#define CONFIG_G_DNL_VENDOR_NUM 0x0403
>> +#define CONFIG_G_DNL_PRODUCT_NUM 0xBD00
>> +#define CONFIG_G_DNL_MANUFACTURER "Texas Instruments"
> 
> Is this really a TI board?

I need some HW address and USB VID:PID to get it working.
TI's code does it and so does siemens' rut.
I could drop this functionality and remove it all.

Best regards,
Lothar