Hi Jean-Christophe PLAGNIOL-VILLARD,
sorry to answer late. I've been on the CELF Conference last week and being busy this week delays so much.
Am Thu, 6 Nov 2008 21:53:29 +0100 schrieb Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com:
On 12:48 Wed 05 Nov , Juergen Schoew wrote:
Hi U-Boot mailling list,
First some general comments
As you anwser it's a NXP board so please create a vendor directory
OK, I use DSPG.
please use tab instead of space for code alignement and indentation please a blank line when you write block of code and comment to make it more readable please and whitespace before and after "<<" & co
OK, I try to find every position.
in the code their is a lot of hard code value please use Macro or specify why you can not do it
Will try to find suitable places.
please only one blank line consecutive
OK.
diff --git a/board/firetux/Makefile b/board/firetux/Makefile new file mode 100644 index 0000000..bf1afec --- /dev/null +++ b/board/firetux/Makefile +include $(TOPDIR)/config.mk
+LIB = $(obj)lib$(BOARD).a
+COBJS-y += firetux.o ethernet.o
as ask by Ben and I move this to drivers/net
OK, I read it more as "you may" and not "you have to". Sorry for the misinterpretation. I try to find some spare time to do this jobs. Hopefully this can work on the next weekend.
diff --git a/board/firetux/ethernet.c b/board/firetux/ethernet.c new file mode 100644 index 0000000..a4c1bc2 --- /dev/null +++ b/board/firetux/ethernet.c
+extern unsigned int boardrevision;
+int firetux_miiphy_initialize(bd_t *bis); +int mii_discover_phy(void); +int mii_negotiate_phy(void);
+#define ALIGN8 static __attribute__ ((aligned(8))) +#define ALIGN4 static __attribute__ ((aligned(4)))
IMHO this make more sense #define align8 __attribute__ ((aligned(8))
OK, changed.
- }
- if (act) {
etn_rxdescriptor = etn2_rxdescriptor;etn_rxstatus = etn2_rxstatus;^^^^^please use tab instead of space on all
Yes.
- /* get mode from environment */
- mode = getenv("phymode");
- if (mode != NULL) {
please use if (mode)
if (0 == strcmp(mode, "auto")) {please invert the condition an so on if (strcmp(mode, "auto")) == 0) {
No problem.
+void firetux_gpio_init_A(void) +{
- /* select TXD2 for GPIOa11 and select RXD2 for GPIOa1 */
- writel(((readl((void *)(PNX8181_SCON_BASE +
PNX8181_SYSMUX0))
& 0xff3ffff3) |0x00400004),
(void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX0));
- /* select ETN for GPIOc0-16 */
- writel(readl((void *)(PNX8181_SCON_BASE + PNX8181_SYSMUX5))
&0xfffffffc,
(void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX5));
- writel(readl((void *)(PNX8181_SCON_BASE + PNX8181_SYSMUX4))
&0x30000000,
(void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX4)); +
- /* use clk26m and fract-n for UART2 */
- writew(((1 << 7) | (1 << 1)),
(void *)(PNX8181_UART2_BASE +PNX8181_UART_FDIV_CTRL));
- writew(0x5F37, (void *)(PNX8181_UART2_BASE +
PNX8181_UART_FDIV_M));
- writew(0x32C8, (void *)(PNX8181_UART2_BASE +
PNX8181_UART_FDIV_N)); +}
IMHO please use deferent function for each hardware revision it will allow to make it more readable and to reduce the u-boot when compiling it for specific revision
The different revisions are very similair. I can split the routines into different functions, but I doubt that it make sense to split these into different files. The overhead is IMHO too much.
+void firetux_gpio_init_B(void) +{
- if (boardrevision < 2) /* EZ_MCP has no
leds or keys */
return;- /* set GPIOA10 and GPIOA2 for key input */
- writel(readl((void *)(PNX8181_SCON_BASE + PNX8181_SYSMUX0))
&0xffcfffcf,
(void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX0));
- writel(readl((void *)PNX8181_GPIOA_PAD_LOW) & 0xffcfffcf,
(void*)PNX8181_GPIOA_PAD_LOW);
- writel(readl((void *)PNX8181_GPIOA_DIRECTION) & ~((1 << 2)
| (1 << 10)),
(void*)PNX8181_GPIOA_DIRECTION); +
- /* set GPIOA9 and GPIOA0 for LED output */
- if (boardrevision > 2) {
writel(readl((void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX0))
&0xfff3fffc,
(void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX0));
writel(readl((void *)PNX8181_GPIOA_DIRECTION)| ((1 <<
- | (1 << 0)),
(void *)PNX8181_GPIOA_DIRECTION);writel(readl((void *)PNX8181_GPIOA_OUTPUT)| ((1 <<
- | (1 << 0)),
(void *)PNX8181_GPIOA_OUTPUT);- } else { /* Baseboard IIIa */
writel(readl((void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX0))
&0xfff3ff3f,
(void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX0));
writel(readl((void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX1))
&0x3ffffffc,
(void *)(PNX8181_SCON_BASE +PNX8181_SYSMUX1));
writel(readl((void *)PNX8181_GPIOA_DIRECTION)| ((1 << 9) | (1 << 31) | (1 <<
- | (1 << 3)),
(void *)PNX8181_GPIOA_DIRECTION);writel(readl((void *)PNX8181_GPIOA_OUTPUT)| ((1 << 9) | (1 << 31) | (1 <<
- | (1 << 3)),
(void *)PNX8181_GPIOA_OUTPUT);- }
+}
if possible split it in multiple file to avoid ifdef in the code at maximun note we will move in the futur to Kconfig to manage it and it's the same for the asm
This is normally one function, which was broken into two parts, because I had to check the board revision in between. But for the move of the network driver I have to split this part out of the network code and can do the hardware check in a extra function and merge this one.
+void firetux_irq_disable(void) +{
- int i;
- writel(31, (void *)PNX8181_INTC_PRIOMASK_IRQ);
- writel(31, (void *)PNX8181_INTC_PRIOMASK_FIQ);
- for (i = 1; i < 67; i++)
PNX8181_DISABLEIRQ(i);- writel(readl((void *)PNX8181_CGU_GATESC) & 0xfdd7becd,
(void*)PNX8181_CGU_GATESC); +}
- }
- return ret;
+}
please create a led.c
OK.
+/*
- timer without interrupts
- */
+/*
- U-Boot expects a 32 bit timer, running at CONFIG_SYS_HZ
- Keep total timer count to avoid losing decrements < div_timer
- */
is your timer is board specific or soc specific??
You are right. This is more SOC specific. I'll move it.
diff --git a/drivers/i2c/pnx8181_i2c.c b/drivers/i2c/pnx8181_i2c.c new file mode 100644 index 0000000..9979ebe --- /dev/null +++ b/drivers/i2c/pnx8181_i2c.c
Oops, I missed to rework this code last time.
+#if defined(CONFIG_CMD_I2C) && defined(CONFIG_FIRETUX)
please move to Makefile
OK, done.
+void i2c_init(int speed, int slaveadd) +{
- unsigned long timeout;
- SCON_SYSMUX1_REG &= SCON_GPIOA27_MUX_FIELD &
SCON_GPIOA28_MUX_FIELD;
- SCON_SYSMUX1_REG |= SCON_GPIOA27_SCL | SCON_GPIOA28_SDA;
- CGU_GATESC_REG |= CGU_I2CEN;
- if (speed == 400000) {
?????
Only tested so far with 100KHz, need to calculate the needed divisors.
- } else {
speed = 100000;/* here the registers are set in order to have a100KHz clk
* for a pclk1 of 23Mhz */I2C_CLKHI_REG = I2C_CLKDIV_FIELD | 0x50;I2C_CLKLO_REG = I2C_CLKDIV_FIELD | 0x96;I2C_HOLDDAT_REG = 0x10;- }
- I2C_ADR_REG = slaveadd & ~I2C_SADDR_FIELD;
- SOFT_I2C_RESET;
- timeout = 0;
- while ((I2C_ACTIVE & I2C_STS_REG) == I2C_ACTIVE) {
if (timeout > I2C_TIMEOUT)break;timeout++;udelay(I2C_DELAY);- }
- DPRINT("%s: speed: %d\n", __func__, speed);
please use debug()
Done.
diff --git a/include/configs/firetux.h b/include/configs/firetux.h new file mode 100644 index 0000000..efa27af --- /dev/null +++ b/include/configs/firetux.h +/* we can have nand _or_ nor flash */ +/* #define CONFIG_NANDFLASH 1 */
+/* #define CONFIG_SHOW_BOOT_PROGRESS 1 */
please no dead code
Sorry, I use this code for testing the release and trying to find problematic areas. This way I only need to change the config and do not need to write the code for every release cycle. So I prefer to let it there.