[PATCH] Clean up Lemote Loongson 2E Support

[PATCH] Clean up Lemote Loongson 2E Support

[Philippe Vachon]

This patch eliminates magic numbers and accessors for memory-mapped
registers. As well, it removes some inline assembly and restructures how
the early printk code behaves.

Signed-off-by: Philippe Vachon <philippe@cowpig.ca>
---
 arch/mips/include/asm/mach-lemote/loongson2e.h |   58 +++++++++++
 arch/mips/lemote/lm2e/dbg_io.c                 |  125 ++---------------------
 arch/mips/lemote/lm2e/prom.c                   |   10 +--
 arch/mips/lemote/lm2e/reset.c                  |   18 ++--
 4 files changed, 80 insertions(+), 131 deletions(-)
 create mode 100644 arch/mips/include/asm/mach-lemote/loongson2e.h

diff --git a/arch/mips/include/asm/mach-lemote/loongson2e.h b/arch/mips/include/asm/mach-lemote/loongson2e.h
new file mode 100644
index 0000000..558eaca
--- /dev/null
+++ b/arch/mips/include/asm/mach-lemote/loongson2e.h
@@ -0,0 +1,58 @@
+/* Accessor functions for the Loongson 2E MMIO registers
+ *
+ * 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;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */
+#ifndef __ASM_MACH_LEMOTE_LOONGSON2E
+#define __ASM_MACH_LEMOTE_LOONGSON2E
+
+#include <linux/types.h>
+
+/* Loongson 2E Control Registers */
+#define LS2E_REG_BASE		0x1fe00100 /* start of config registers */
+#define LS2E_GENCFG_REG		(LS2E_REG_BASE + 0x04)
+
+#define LS2E_RESET_VECTOR	0x1fc00000 /* this should be obvious! */
+
+/* UART address (16550 -- on the Fulong) */
+#define LS2E_UART_BASE		0x1fd003f8
+
+/* Various system parameters passed from PMON */
+extern unsigned long bus_clock;
+extern unsigned long cpu_clock_freq;
+extern unsigned int memsize, highmemsize;
+
+static inline void ls2e_writeb(uint8_t value, phys_addr_t addr)
+{
+	*(volatile uint8_t *)addr = value;
+}
+
+static inline void ls2e_writew(uint16_t value, phys_addr_t addr)
+{
+	*(volatile uint16_t *)addr = value;
+}
+
+static inline void ls2e_writel(uint32_t value, phys_addr_t addr)
+{
+	*(volatile uint32_t *)addr = value;
+}
+
+static inline uint8_t ls2e_readb(phys_addr_t addr)
+{
+	return *(volatile uint8_t *)addr;
+}
+
+static inline uint16_t ls2e_readw(phys_addr_t addr)
+{
+	return *(volatile uint16_t *)addr;
+}
+
+static inline uint32_t ls2e_readl(phys_addr_t addr)
+{
+	return *(volatile uint32_t *)addr;
+}
+
+#endif /* __ASM_MACH_LEMOTE_LOONGSON2E */
diff --git a/arch/mips/lemote/lm2e/dbg_io.c b/arch/mips/lemote/lm2e/dbg_io.c
index 6c95da3..988491f 100644
--- a/arch/mips/lemote/lm2e/dbg_io.c
+++ b/arch/mips/lemote/lm2e/dbg_io.c
@@ -1,10 +1,6 @@
-/*
- * Copyright 2001 MontaVista Software Inc.
- * Author: Jun Sun, jsun@mvista.com or jsun@junsun.net
- * Copyright (C) 2000, 2001 Ralf Baechle (ralf@gnu.org)
+/*  Support for the 16550 on the Lemote Fulong.
  *
- * Copyright (C) 2007 Lemote Inc. & Insititute of Computing Technology
- * Author: Fuxin Zhang, zhangfx@lemote.com
+ *  Copyright (c) 2009 Philippe Vachon <philippe@cowpig.ca>
  *
  *  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
@@ -29,118 +25,19 @@
  */
 
 #include <linux/io.h>
-#include <linux/init.h>
 #include <linux/types.h>
+#include <linux/serial_reg.h>
 
-#include <asm/serial.h>
+#include <loongson2e.h>
 
-#define         UART16550_BAUD_2400             2400
-#define         UART16550_BAUD_4800             4800
-#define         UART16550_BAUD_9600             9600
-#define         UART16550_BAUD_19200            19200
-#define         UART16550_BAUD_38400            38400
-#define         UART16550_BAUD_57600            57600
-#define         UART16550_BAUD_115200           115200
-
-#define         UART16550_PARITY_NONE           0
-#define         UART16550_PARITY_ODD            0x08
-#define         UART16550_PARITY_EVEN           0x18
-#define         UART16550_PARITY_MARK           0x28
-#define         UART16550_PARITY_SPACE          0x38
-
-#define         UART16550_DATA_5BIT             0x0
-#define         UART16550_DATA_6BIT             0x1
-#define         UART16550_DATA_7BIT             0x2
-#define         UART16550_DATA_8BIT             0x3
-
-#define         UART16550_STOP_1BIT             0x0
-#define         UART16550_STOP_2BIT             0x4
-
-/* ----------------------------------------------------- */
-
-/* === CONFIG === */
-#ifdef CONFIG_64BIT
-#define         BASE                    (0xffffffffbfd003f8)
-#else
-#define         BASE                    (0xbfd003f8)
-#endif
-
-#define         MAX_BAUD                BASE_BAUD
-/* === END OF CONFIG === */
-
-#define         REG_OFFSET              1
-
-/* register offset */
-#define         OFS_RCV_BUFFER          0
-#define         OFS_TRANS_HOLD          0
-#define         OFS_SEND_BUFFER         0
-#define         OFS_INTR_ENABLE         (1*REG_OFFSET)
-#define         OFS_INTR_ID             (2*REG_OFFSET)
-#define         OFS_DATA_FORMAT         (3*REG_OFFSET)
-#define         OFS_LINE_CONTROL        (3*REG_OFFSET)
-#define         OFS_MODEM_CONTROL       (4*REG_OFFSET)
-#define         OFS_RS232_OUTPUT        (4*REG_OFFSET)
-#define         OFS_LINE_STATUS         (5*REG_OFFSET)
-#define         OFS_MODEM_STATUS        (6*REG_OFFSET)
-#define         OFS_RS232_INPUT         (6*REG_OFFSET)
-#define         OFS_SCRATCH_PAD         (7*REG_OFFSET)
-
-#define         OFS_DIVISOR_LSB         (0*REG_OFFSET)
-#define         OFS_DIVISOR_MSB         (1*REG_OFFSET)
-
-/* memory-mapped read/write of the port */
-#define         UART16550_READ(y)	readb((char *)BASE + (y))
-#define         UART16550_WRITE(y, z)	writeb(z, (char *)BASE + (y))
-
-void debugInit(u32 baud, u8 data, u8 parity, u8 stop)
+void prom_putchar(char c)
 {
-	u32 divisor;
-
-	/* disable interrupts */
-	UART16550_WRITE(OFS_INTR_ENABLE, 0);
+	int timeout;
+	phys_addr_t uart_base = (phys_addr_t)ioremap_nocache(LS2E_UART_BASE, 8);
+	char reg = ls2e_readb(uart_base + UART_LSR) & UART_LSR_THRE;
 
-	/* set up buad rate */
-	/* set DIAB bit */
-	UART16550_WRITE(OFS_LINE_CONTROL, 0x80);
-
-	/* set divisor */
-	divisor = MAX_BAUD / baud;
-	UART16550_WRITE(OFS_DIVISOR_LSB, divisor & 0xff);
-	UART16550_WRITE(OFS_DIVISOR_MSB, (divisor & 0xff00) >> 8);
-
-	/* clear DIAB bit */
-	UART16550_WRITE(OFS_LINE_CONTROL, 0x0);
-
-	/* set data format */
-	UART16550_WRITE(OFS_DATA_FORMAT, data | parity | stop);
-}
-
-static int remoteDebugInitialized;
-
-u8 getDebugChar(void)
-{
-	if (!remoteDebugInitialized) {
-		remoteDebugInitialized = 1;
-		debugInit(UART16550_BAUD_115200,
-			  UART16550_DATA_8BIT,
-			  UART16550_PARITY_NONE, UART16550_STOP_1BIT);
-	}
-
-	while ((UART16550_READ(OFS_LINE_STATUS) & 0x1) == 0) ;
-	return UART16550_READ(OFS_RCV_BUFFER);
-}
-
-int putDebugChar(u8 byte)
-{
-	if (!remoteDebugInitialized) {
-		remoteDebugInitialized = 1;
-		/*
-		   debugInit(UART16550_BAUD_115200,
-		   UART16550_DATA_8BIT,
-		   UART16550_PARITY_NONE, UART16550_STOP_1BIT); */
-	}
+	for (timeout = 1024; reg == 0 && timeout > 0; timeout--)
+		reg = ls2e_readb(uart_base + UART_LSR) & UART_LSR_THRE;
 
-	while ((UART16550_READ(OFS_LINE_STATUS) & 0x20) == 0) ;
-	UART16550_WRITE(OFS_SEND_BUFFER, byte);
-	return 1;
+	ls2e_writeb(c, uart_base + UART_TX);
 }
diff --git a/arch/mips/lemote/lm2e/prom.c b/arch/mips/lemote/lm2e/prom.c
index 7edc15d..83777bd 100644
--- a/arch/mips/lemote/lm2e/prom.c
+++ b/arch/mips/lemote/lm2e/prom.c
@@ -18,10 +18,7 @@
 #include <linux/bootmem.h>
 #include <asm/bootinfo.h>
 
-extern unsigned long bus_clock;
-extern unsigned long cpu_clock_freq;
-extern unsigned int memsize, highmemsize;
-extern int putDebugChar(unsigned char byte);
+#include <loongson2e.h>
 
 static int argc;
 /* pmon passes arguments in 32bit pointers */
@@ -90,8 +87,3 @@ do {									\
 void __init prom_free_prom_memory(void)
 {
 }
-
-void prom_putchar(char c)
-{
-	putDebugChar(c);
-}
diff --git a/arch/mips/lemote/lm2e/reset.c b/arch/mips/lemote/lm2e/reset.c
index 099387a..0989d28 100644
--- a/arch/mips/lemote/lm2e/reset.c
+++ b/arch/mips/lemote/lm2e/reset.c
@@ -7,20 +7,22 @@
  * Copyright (C) 2007 Lemote, Inc. & Institute of Computing Technology
  * Author: Fuxin Zhang, zhangfx@lemote.com
  */
+
 #include <linux/pm.h>
+#include <linux/io.h>
+#include <loongson2e.h>
 
 #include <asm/reboot.h>
 
 static void loongson2e_restart(char *command)
 {
-#ifdef CONFIG_32BIT
-	*(unsigned long *)0xbfe00104 &= ~(1 << 2);
-	*(unsigned long *)0xbfe00104 |= (1 << 2);
-#else
-	*(unsigned long *)0xffffffffbfe00104 &= ~(1 << 2);
-	*(unsigned long *)0xffffffffbfe00104 |= (1 << 2);
-#endif
-	__asm__ __volatile__("jr\t%0"::"r"(0xbfc00000));
+	uint32_t ctl =
+		(ls2e_readl((phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4))
+		& ~(1 << 2)) | 1 << 2;
+
+	ls2e_writel(ctl, (phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4));
+
+	((void (*)(void))ioremap_nocache(LS2E_RESET_VECTOR, 4))();
 }
 
 static void loongson2e_halt(void)
-- 
1.6.1

Re: [PATCH] Clean up Lemote Loongson 2E Support

[Ralf Baechle]

On Wed, Apr 22, 2009 at 01:58:09AM -0400, Philippe Vachon wrote:

> +static inline void ls2e_writeb(uint8_t value, phys_addr_t addr)
> +{
> +	*(volatile uint8_t *)addr = value;

So is addr a physical addres or a virtual one?  In the first case you
can't dereference it, in the latter the type is wrong.

  Ralf

Re: [PATCH] Clean up Lemote Loongson 2E Support

[Philippe Vachon]

On Wed, Apr 22, 2009 at 08:09:51AM +0200, Ralf Baechle wrote:
> > +static inline void ls2e_writeb(uint8_t value, phys_addr_t addr)
> > +{
> > +	*(volatile uint8_t *)addr = value;
> 
> So is addr a physical addres or a virtual one?  In the first case you
> can't dereference it, in the latter the type is wrong.
> 
Aha, you caught me. That should be 'unsigned long' as opposed to
phys_addr_t, as the address is a virtual address.

P.

[PATCH] Clean up Lemote Loongson 2E Support

[Philippe Vachon]

This patch eliminates magic numbers and adds accessors for memory-mapped
registers. As well, it removes some inline assembly and restructures how
the early printk code behaves.

Signed-off-by: Philippe Vachon <philippe@cowpig.ca>
---
 arch/mips/include/asm/mach-lemote/loongson2e.h |   60 +++++++++++
 arch/mips/lemote/lm2e/dbg_io.c                 |  125 ++---------------------
 arch/mips/lemote/lm2e/prom.c                   |   10 +--
 arch/mips/lemote/lm2e/reset.c                  |   18 ++--
 4 files changed, 82 insertions(+), 131 deletions(-)
 create mode 100644 arch/mips/include/asm/mach-lemote/loongson2e.h

diff --git a/arch/mips/include/asm/mach-lemote/loongson2e.h b/arch/mips/include/asm/mach-lemote/loongson2e.h
new file mode 100644
index 0000000..82c1a95
--- /dev/null
+++ b/arch/mips/include/asm/mach-lemote/loongson2e.h
@@ -0,0 +1,60 @@
+/* Accessor functions for the Loongson 2E MMIO registers
+ *
+ * Copyright (c) 2009 Philippe Vachon <philippe@cowpig.ca>
+ *
+ * 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;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ */
+#ifndef __ASM_MACH_LEMOTE_LOONGSON2E
+#define __ASM_MACH_LEMOTE_LOONGSON2E
+
+#include <linux/types.h>
+
+/* Loongson 2E Control Registers */
+#define LS2E_REG_BASE		0x1fe00100 /* start of config registers */
+#define LS2E_GENCFG_REG		(LS2E_REG_BASE + 0x04)
+
+#define LS2E_RESET_VECTOR	0x1fc00000 /* this should be obvious! */
+
+/* UART address (16550 -- on the Fulong) */
+#define LS2E_UART_BASE		0x1fd003f8
+
+/* Various system parameters passed from PMON */
+extern unsigned long bus_clock;
+extern unsigned long cpu_clock_freq;
+extern unsigned int memsize, highmemsize;
+
+static inline void ls2e_writeb(uint8_t value, unsigned long addr)
+{
+	*(volatile uint8_t *)addr = value;
+}
+
+static inline void ls2e_writew(uint16_t value, unsigned long addr)
+{
+	*(volatile uint16_t *)addr = value;
+}
+
+static inline void ls2e_writel(uint32_t value, unsigned long addr)
+{
+	*(volatile uint32_t *)addr = value;
+}
+
+static inline uint8_t ls2e_readb(unsigned long addr)
+{
+	return *(volatile uint8_t *)addr;
+}
+
+static inline uint16_t ls2e_readw(unsigned long addr)
+{
+	return *(volatile uint16_t *)addr;
+}
+
+static inline uint32_t ls2e_readl(unsigned long addr)
+{
+	return *(volatile uint32_t *)addr;
+}
+
+#endif /* __ASM_MACH_LEMOTE_LOONGSON2E */
diff --git a/arch/mips/lemote/lm2e/dbg_io.c b/arch/mips/lemote/lm2e/dbg_io.c
index 6c95da3..988491f 100644
--- a/arch/mips/lemote/lm2e/dbg_io.c
+++ b/arch/mips/lemote/lm2e/dbg_io.c
@@ -1,10 +1,6 @@
-/*
- * Copyright 2001 MontaVista Software Inc.
- * Author: Jun Sun, jsun@mvista.com or jsun@junsun.net
- * Copyright (C) 2000, 2001 Ralf Baechle (ralf@gnu.org)
+/*  Support for the 16550 on the Lemote Fulong.
  *
- * Copyright (C) 2007 Lemote Inc. & Insititute of Computing Technology
- * Author: Fuxin Zhang, zhangfx@lemote.com
+ *  Copyright (c) 2009 Philippe Vachon <philippe@cowpig.ca>
  *
  *  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
@@ -29,118 +25,19 @@
  */
 
 #include <linux/io.h>
-#include <linux/init.h>
 #include <linux/types.h>
+#include <linux/serial_reg.h>
 
-#include <asm/serial.h>
+#include <loongson2e.h>
 
-#define         UART16550_BAUD_2400             2400
-#define         UART16550_BAUD_4800             4800
-#define         UART16550_BAUD_9600             9600
-#define         UART16550_BAUD_19200            19200
-#define         UART16550_BAUD_38400            38400
-#define         UART16550_BAUD_57600            57600
-#define         UART16550_BAUD_115200           115200
-
-#define         UART16550_PARITY_NONE           0
-#define         UART16550_PARITY_ODD            0x08
-#define         UART16550_PARITY_EVEN           0x18
-#define         UART16550_PARITY_MARK           0x28
-#define         UART16550_PARITY_SPACE          0x38
-
-#define         UART16550_DATA_5BIT             0x0
-#define         UART16550_DATA_6BIT             0x1
-#define         UART16550_DATA_7BIT             0x2
-#define         UART16550_DATA_8BIT             0x3
-
-#define         UART16550_STOP_1BIT             0x0
-#define         UART16550_STOP_2BIT             0x4
-
-/* ----------------------------------------------------- */
-
-/* === CONFIG === */
-#ifdef CONFIG_64BIT
-#define         BASE                    (0xffffffffbfd003f8)
-#else
-#define         BASE                    (0xbfd003f8)
-#endif
-
-#define         MAX_BAUD                BASE_BAUD
-/* === END OF CONFIG === */
-
-#define         REG_OFFSET              1
-
-/* register offset */
-#define         OFS_RCV_BUFFER          0
-#define         OFS_TRANS_HOLD          0
-#define         OFS_SEND_BUFFER         0
-#define         OFS_INTR_ENABLE         (1*REG_OFFSET)
-#define         OFS_INTR_ID             (2*REG_OFFSET)
-#define         OFS_DATA_FORMAT         (3*REG_OFFSET)
-#define         OFS_LINE_CONTROL        (3*REG_OFFSET)
-#define         OFS_MODEM_CONTROL       (4*REG_OFFSET)
-#define         OFS_RS232_OUTPUT        (4*REG_OFFSET)
-#define         OFS_LINE_STATUS         (5*REG_OFFSET)
-#define         OFS_MODEM_STATUS        (6*REG_OFFSET)
-#define         OFS_RS232_INPUT         (6*REG_OFFSET)
-#define         OFS_SCRATCH_PAD         (7*REG_OFFSET)
-
-#define         OFS_DIVISOR_LSB         (0*REG_OFFSET)
-#define         OFS_DIVISOR_MSB         (1*REG_OFFSET)
-
-/* memory-mapped read/write of the port */
-#define         UART16550_READ(y)	readb((char *)BASE + (y))
-#define         UART16550_WRITE(y, z)	writeb(z, (char *)BASE + (y))
-
-void debugInit(u32 baud, u8 data, u8 parity, u8 stop)
+void prom_putchar(char c)
 {
-	u32 divisor;
-
-	/* disable interrupts */
-	UART16550_WRITE(OFS_INTR_ENABLE, 0);
+	int timeout;
+	phys_addr_t uart_base = (phys_addr_t)ioremap_nocache(LS2E_UART_BASE, 8);
+	char reg = ls2e_readb(uart_base + UART_LSR) & UART_LSR_THRE;
 
-	/* set up buad rate */
-	/* set DIAB bit */
-	UART16550_WRITE(OFS_LINE_CONTROL, 0x80);
-
-	/* set divisor */
-	divisor = MAX_BAUD / baud;
-	UART16550_WRITE(OFS_DIVISOR_LSB, divisor & 0xff);
-	UART16550_WRITE(OFS_DIVISOR_MSB, (divisor & 0xff00) >> 8);
-
-	/* clear DIAB bit */
-	UART16550_WRITE(OFS_LINE_CONTROL, 0x0);
-
-	/* set data format */
-	UART16550_WRITE(OFS_DATA_FORMAT, data | parity | stop);
-}
-
-static int remoteDebugInitialized;
-
-u8 getDebugChar(void)
-{
-	if (!remoteDebugInitialized) {
-		remoteDebugInitialized = 1;
-		debugInit(UART16550_BAUD_115200,
-			  UART16550_DATA_8BIT,
-			  UART16550_PARITY_NONE, UART16550_STOP_1BIT);
-	}
-
-	while ((UART16550_READ(OFS_LINE_STATUS) & 0x1) == 0) ;
-	return UART16550_READ(OFS_RCV_BUFFER);
-}
-
-int putDebugChar(u8 byte)
-{
-	if (!remoteDebugInitialized) {
-		remoteDebugInitialized = 1;
-		/*
-		   debugInit(UART16550_BAUD_115200,
-		   UART16550_DATA_8BIT,
-		   UART16550_PARITY_NONE, UART16550_STOP_1BIT); */
-	}
+	for (timeout = 1024; reg == 0 && timeout > 0; timeout--)
+		reg = ls2e_readb(uart_base + UART_LSR) & UART_LSR_THRE;
 
-	while ((UART16550_READ(OFS_LINE_STATUS) & 0x20) == 0) ;
-	UART16550_WRITE(OFS_SEND_BUFFER, byte);
-	return 1;
+	ls2e_writeb(c, uart_base + UART_TX);
 }
diff --git a/arch/mips/lemote/lm2e/prom.c b/arch/mips/lemote/lm2e/prom.c
index 7edc15d..83777bd 100644
--- a/arch/mips/lemote/lm2e/prom.c
+++ b/arch/mips/lemote/lm2e/prom.c
@@ -18,10 +18,7 @@
 #include <linux/bootmem.h>
 #include <asm/bootinfo.h>
 
-extern unsigned long bus_clock;
-extern unsigned long cpu_clock_freq;
-extern unsigned int memsize, highmemsize;
-extern int putDebugChar(unsigned char byte);
+#include <loongson2e.h>
 
 static int argc;
 /* pmon passes arguments in 32bit pointers */
@@ -90,8 +87,3 @@ do {									\
 void __init prom_free_prom_memory(void)
 {
 }
-
-void prom_putchar(char c)
-{
-	putDebugChar(c);
-}
diff --git a/arch/mips/lemote/lm2e/reset.c b/arch/mips/lemote/lm2e/reset.c
index 099387a..0989d28 100644
--- a/arch/mips/lemote/lm2e/reset.c
+++ b/arch/mips/lemote/lm2e/reset.c
@@ -7,20 +7,22 @@
  * Copyright (C) 2007 Lemote, Inc. & Institute of Computing Technology
  * Author: Fuxin Zhang, zhangfx@lemote.com
  */
+
 #include <linux/pm.h>
+#include <linux/io.h>
+#include <loongson2e.h>
 
 #include <asm/reboot.h>
 
 static void loongson2e_restart(char *command)
 {
-#ifdef CONFIG_32BIT
-	*(unsigned long *)0xbfe00104 &= ~(1 << 2);
-	*(unsigned long *)0xbfe00104 |= (1 << 2);
-#else
-	*(unsigned long *)0xffffffffbfe00104 &= ~(1 << 2);
-	*(unsigned long *)0xffffffffbfe00104 |= (1 << 2);
-#endif
-	__asm__ __volatile__("jr\t%0"::"r"(0xbfc00000));
+	uint32_t ctl =
+		(ls2e_readl((phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4))
+		& ~(1 << 2)) | 1 << 2;
+
+	ls2e_writel(ctl, (phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4));
+
+	((void (*)(void))ioremap_nocache(LS2E_RESET_VECTOR, 4))();
 }
 
 static void loongson2e_halt(void)
-- 
1.6.1

Re: [PATCH] Clean up Lemote Loongson 2E Support

[Arnaud Patard]

Philippe Vachon <philippe@cowpig.ca> writes:

Hi,

> This patch eliminates magic numbers and adds accessors for memory-mapped
> registers. As well, it removes some inline assembly and restructures how
> the early printk code behaves.
>
> Signed-off-by: Philippe Vachon <philippe@cowpig.ca>
> ---
>  arch/mips/include/asm/mach-lemote/loongson2e.h |   60 +++++++++++
>  arch/mips/lemote/lm2e/dbg_io.c                 |  125 ++---------------------
>  arch/mips/lemote/lm2e/prom.c                   |   10 +--
>  arch/mips/lemote/lm2e/reset.c                  |   18 ++--
>  4 files changed, 82 insertions(+), 131 deletions(-)
>  create mode 100644 arch/mips/include/asm/mach-lemote/loongson2e.h
>
> diff --git a/arch/mips/include/asm/mach-lemote/loongson2e.h b/arch/mips/include/asm/mach-lemote/loongson2e.h
> new file mode 100644
> index 0000000..82c1a95
> --- /dev/null
> +++ b/arch/mips/include/asm/mach-lemote/loongson2e.h
> @@ -0,0 +1,60 @@
> +/* Accessor functions for the Loongson 2E MMIO registers
> + *
> + * Copyright (c) 2009 Philippe Vachon <philippe@cowpig.ca>
> + *
> + * 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;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + */
> +#ifndef __ASM_MACH_LEMOTE_LOONGSON2E
> +#define __ASM_MACH_LEMOTE_LOONGSON2E
> +
> +#include <linux/types.h>
> +
> +/* Loongson 2E Control Registers */
> +#define LS2E_REG_BASE		0x1fe00100 /* start of config registers */
> +#define LS2E_GENCFG_REG		(LS2E_REG_BASE + 0x04)
> +
> +#define LS2E_RESET_VECTOR	0x1fc00000 /* this should be obvious! */
> +

Theses are neither Lemote nor 2E specifics. 2E and 2F controllers are
very similar (and they're similar to the bonito stuff). Why do you need
a specific header instead of using the Bonito64.h header for theses
constants ? And if you really want to create it, please rename all to
loongson and remove lemote references as it'll work on 2F and on boards
from ST.

> +/* UART address (16550 -- on the Fulong) */
> +#define LS2E_UART_BASE		0x1fd003f8

It happens to be 0x1fd003f8 but it could have been at a different
address base. For instance, on 2f, depending on the board, there's
0x1ff003f8 and 0x1fd003f8 (I think there's also some board with a
different address than theses but I'm not sure).

> +/* Various system parameters passed from PMON */
> +extern unsigned long bus_clock;
> +extern unsigned long cpu_clock_freq;
> +extern unsigned int memsize, highmemsize;
> +
> +static inline void ls2e_writeb(uint8_t value, unsigned long addr)
> +{
> +	*(volatile uint8_t *)addr = value;
> +}
> +

What about readl/writel and friends ?

[...]

> -void debugInit(u32 baud, u8 data, u8 parity, u8 stop)
> +void prom_putchar(char c)
>  {
> -	u32 divisor;
> -
> -	/* disable interrupts */
> -	UART16550_WRITE(OFS_INTR_ENABLE, 0);
> +	int timeout;
> +	phys_addr_t uart_base = (phys_addr_t)ioremap_nocache(LS2E_UART_BASE, 8);
> +	char reg = ls2e_readb(uart_base + UART_LSR) & UART_LSR_THRE;

hmm... I may be wrong on that but using ioremap looks here looks not a
good idea. This code is called by early_printk so you can end up calling
it very early in the boot process.
Also, you're calling ioremap everytime this function is called. Why
don't you do that only the first time ?

[...]

> diff --git a/arch/mips/lemote/lm2e/reset.c b/arch/mips/lemote/lm2e/reset.c
> index 099387a..0989d28 100644
> --- a/arch/mips/lemote/lm2e/reset.c
> +++ b/arch/mips/lemote/lm2e/reset.c
> @@ -7,20 +7,22 @@
>   * Copyright (C) 2007 Lemote, Inc. & Institute of Computing Technology
>   * Author: Fuxin Zhang, zhangfx@lemote.com
>   */
> +
>  #include <linux/pm.h>
> +#include <linux/io.h>
> +#include <loongson2e.h>
>  
>  #include <asm/reboot.h>
>  
>  static void loongson2e_restart(char *command)
>  {
> -#ifdef CONFIG_32BIT
> -	*(unsigned long *)0xbfe00104 &= ~(1 << 2);
> -	*(unsigned long *)0xbfe00104 |= (1 << 2);
> -#else
> -	*(unsigned long *)0xffffffffbfe00104 &= ~(1 << 2);
> -	*(unsigned long *)0xffffffffbfe00104 |= (1 << 2);
> -#endif
> -	__asm__ __volatile__("jr\t%0"::"r"(0xbfc00000));
> +	uint32_t ctl =
> +		(ls2e_readl((phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4))
> +		& ~(1 << 2)) | 1 << 2;
> +
> +	ls2e_writel(ctl, (phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4));
> +
> +	((void (*)(void))ioremap_nocache(LS2E_RESET_VECTOR, 4))();

same remark as for the serial stuff. I'm really not sure that calling ioremap
in such a place is a good idea (say, you've panic'ed and booted with
panic=3, will this still work ?).


Arnaud

Re: [PATCH] Clean up Lemote Loongson 2E Support

[Philippe Vachon]

Hi,

On Wed, Apr 22, 2009 at 11:15:19AM +0200, Arnaud Patard wrote:
> 
> Theses are neither Lemote nor 2E specifics. 2E and 2F controllers are
> very similar (and they're similar to the bonito stuff). Why do you need
> a specific header instead of using the Bonito64.h header for theses
> constants ? And if you really want to create it, please rename all to
> loongson and remove lemote references as it'll work on 2F and on boards
> from ST.
> 

What might be worthwhile eventually is to move all the loongson code into
arch/mips/loongson. I know there is some duplication of code between 2F
and 2E as it stands, but since 2F isn't upstream yet (and probably won't
be for a while), I would cross that bridge when we get there.

Given how Loongson actually differs a lot from Bonito64 beyond a few of
the control registers' addresses, perhaps it is worthwhile to start
moving away from using bonito64.h anyways; it's a bit odd that ICT chose
to base some of the SoC on the Bonito64 design, IMO, but that's a
different conversation.

> > +/* UART address (16550 -- on the Fulong) */
> > +#define LS2E_UART_BASE		0x1fd003f8
> 
> It happens to be 0x1fd003f8 but it could have been at a different
> address base. For instance, on 2f, depending on the board, there's
> 0x1ff003f8 and 0x1fd003f8 (I think there's also some board with a
> different address than theses but I'm not sure).
> 

One option I was thinking of was moving that into a device-specific
header. However, of the two Loongson 2E devices I have, both have the
UART at the same address -- are there any Loongson 2E devices that have
the UART at a different location? None that I'm aware of (and on the 2F
side, I only know of the Gdium).

...

> > +static inline void ls2e_writeb(uint8_t value, unsigned long addr)
> > +{
> > +	*(volatile uint8_t *)addr = value;
> > +}
> > +
> 
> What about readl/writel and friends ?
> 

I'm ambivalent. Since they're sub-64-bit-wide reads/writes, they're
happening in a single instruction anyways. I can adjust the patch to use
them in the name of reducing code duplication.

> hmm... I may be wrong on that but using ioremap looks here looks not a
> good idea. This code is called by early_printk so you can end up calling
> it very early in the boot process.
> Also, you're calling ioremap everytime this function is called. Why
> don't you do that only the first time ?
> 
> [...]

Quite wrong -- as the address is in kseg1, when compiled this actually
will end up having the same net effect as using the 'deprecated'
CKSEG1ADDR() macro. Have a look at the implementation of
__ioremap_mode() in arch/mips/include/asm/io.h. While it's being
'called' early in the boot process, the physical resource will be 
accessed via its kseg1 address. When I was speaking to Ralf about this, 
he had mentioned this was by design.

In looking at the disassembly of prom_putchar in particular:

; ioremap_nocache()
li      $v0, 0xFFFFFFF9 
dsll32  $v0, 8
ori     $v0, 0x1FD
dsll    $v0, 20
ori     $v0, 0x3FD

; load from the UART's LSR register:
lbu     $v1, 0($v0)
...

> same remark as for the serial stuff. I'm really not sure that calling ioremap
> in such a place is a good idea (say, you've panic'ed and booted with
> panic=3, will this still work ?).
> 

Yes. See above.

I'll probably send an updated patch shortly.

Cheers,
Phil

Re: [PATCH] Clean up Lemote Loongson 2E Support

[Arnaud Patard]

Philippe Vachon <philippe@cowpig.ca> writes:

Hi,

> Hi,
>
> On Wed, Apr 22, 2009 at 11:15:19AM +0200, Arnaud Patard wrote:
>> 
>> Theses are neither Lemote nor 2E specifics. 2E and 2F controllers are
>> very similar (and they're similar to the bonito stuff). Why do you need
>> a specific header instead of using the Bonito64.h header for theses
>> constants ? And if you really want to create it, please rename all to
>> loongson and remove lemote references as it'll work on 2F and on boards
>> from ST.
>> 
>
> What might be worthwhile eventually is to move all the loongson code into
> arch/mips/loongson. I know there is some duplication of code between 2F
> and 2E as it stands, but since 2F isn't upstream yet (and probably won't
> be for a while), I would cross that bridge when we get there.

That's not a good reason to repeat wrong naming from the
past. Moverover, Lemote is trying to get 2f support merged so I would
say that you're pessimistic.

>
> Given how Loongson actually differs a lot from Bonito64 beyond a few of
> the control registers' addresses, perhaps it is worthwhile to start
> moving away from using bonito64.h anyways; it's a bit odd that ICT chose
> to base some of the SoC on the Bonito64 design, IMO, but that's a
> different conversation.
>
>> > +/* UART address (16550 -- on the Fulong) */
>> > +#define LS2E_UART_BASE		0x1fd003f8
>> 
>> It happens to be 0x1fd003f8 but it could have been at a different
>> address base. For instance, on 2f, depending on the board, there's
>> 0x1ff003f8 and 0x1fd003f8 (I think there's also some board with a
>> different address than theses but I'm not sure).
>> 
>
> One option I was thinking of was moving that into a device-specific
> header. However, of the two Loongson 2E devices I have, both have the
> UART at the same address -- are there any Loongson 2E devices that have
> the UART at a different location? None that I'm aware of (and on the 2F
> side, I only know of the Gdium).

1 is more than 0 so, please take 2f into account. I don't see the
benefit of creating yet an other header containing only difference
something like "UART_BASE 0x1ff003f8" 

>
> ...
>
>> > +static inline void ls2e_writeb(uint8_t value, unsigned long addr)
>> > +{
>> > +	*(volatile uint8_t *)addr = value;
>> > +}
>> > +
>> 
>> What about readl/writel and friends ?
>> 
>
> I'm ambivalent. Since they're sub-64-bit-wide reads/writes, they're
> happening in a single instruction anyways. I can adjust the patch to use
> them in the name of reducing code duplication.
>
>> hmm... I may be wrong on that but using ioremap looks here looks not a
>> good idea. This code is called by early_printk so you can end up calling
>> it very early in the boot process.
>> Also, you're calling ioremap everytime this function is called. Why
>> don't you do that only the first time ?
>> 
>> [...]
>
> Quite wrong -- as the address is in kseg1, when compiled this actually
> will end up having the same net effect as using the 'deprecated'
> CKSEG1ADDR() macro. Have a look at the implementation of
> __ioremap_mode() in arch/mips/include/asm/io.h. While it's being
> 'called' early in the boot process, the physical resource will be 
> accessed via its kseg1 address. When I was speaking to Ralf about this, 
> he had mentioned this was by design.

ok, I read the code too quickly. thanks.

Please note also that my remark about calling it everytime still
stands. Reading a variable is taking less instructions than calling
ioremap_nocache.

Arnaud

Re: [PATCH] Clean up Lemote Loongson 2E Support

[Philippe Vachon]

Hi,

Please refer to this mess -- 
http://lebesgue.cowpig.ca/~philippe/gdium/0001-Clean-up-Lemote-Loongson-2-Support.patch -- 
for my subsequent commentary. This is what I would envision being moved 
upstream eventually, and how I think Loongson support would eventually best 
be structured. This adds basic support for the Emtec Gdium as well as the 
Fulong. I created a special case for the Menglan, but it doesn't do
anything particularily different from the Fulong other than change what
the machine name is in /proc/cpuinfo.

I've been working on this with the following goals in mind:
- Determine what functionality is specific to a particular platform and
  split it off elsewhere.
- Build up a directory of code common to the Loongson 2 "series" of
  CPUs.
- Attempt to move away from anything using CKSEG1ADDR(); this is far
  from complete. Ralf will gut me if I try to submit a patch using
  CKSEG1ADDR().
- Attempt to unify the Loongson 2E/F register names in their own header.
  One thing I've noticed is that where there was a Bonito register it
  would be used -- otherwise, the 'raw' number would be cast to a
  pointer and dereferenced!
- Try to build a framework on which a new Loongson 2E/2F/2G(?) machines
  could be added quickly. I actually did that with the Gdium, borrowing
  some of the existing code Mandriva and Lemote have done for it.

Of course, at a glance you'll see it's far from being ready to be
submitted upstream -- and I need to do a rebase or two to make it a
little more pallatable, given the size of the patch.

I've laid out the directories as follows:

arch/mips/
	+- loongson/
	|	+- common/ /* common code */
	|	+- gdium/    /* all the other directories are for */
	|	+- fulong-2e /* particular platforms */
	|
	...
	+- include/asm/
		+- mach-loongson/ 
			/* platform-specific headers */
			+- fulong-2e/
			+- gdium/

On Thu, Apr 23, 2009 at 10:23:47AM +0200, Arnaud Patard wrote:
> That's not a good reason to repeat wrong naming from the
> past. Moverover, Lemote is trying to get 2f support merged so I would
> say that you're pessimistic.
> 

Generally, the purpose of this patch was to clean up some ugliness I saw
when I was trying to help someone figure out why they were having
trouble with kernels they were building. I didn't want to set out to do
something too invasive, as that would just lead to headaches for
everyone.

I don't mean to come off as pessimistic, either -- I'm quite excited
that Lemote is trying to move things up-stream. I do recognize how much
work it is to move a gargantuan patch like that upstream, however, and I
didn't want to get in the way of that.

> >
> > Given how Loongson actually differs a lot from Bonito64 beyond a few of
> > the control registers' addresses, perhaps it is worthwhile to start
> > moving away from using bonito64.h anyways; it's a bit odd that ICT chose
> > to base some of the SoC on the Bonito64 design, IMO, but that's a
> > different conversation.
> >
> >> > +/* UART address (16550 -- on the Fulong) */
> >> > +#define LS2E_UART_BASE		0x1fd003f8
> >> 
> >> It happens to be 0x1fd003f8 but it could have been at a different
> >> address base. For instance, on 2f, depending on the board, there's
> >> 0x1ff003f8 and 0x1fd003f8 (I think there's also some board with a
> >> different address than theses but I'm not sure).
> >> 
> >
> > One option I was thinking of was moving that into a device-specific
> > header. However, of the two Loongson 2E devices I have, both have the
> > UART at the same address -- are there any Loongson 2E devices that have
> > the UART at a different location? None that I'm aware of (and on the 2F
> > side, I only know of the Gdium).
> 
> 1 is more than 0 so, please take 2f into account. I don't see the
> benefit of creating yet an other header containing only difference
> something like "UART_BASE 0x1ff003f8" 

See my above proposed patch. Since there are definitely cases where
there will be code that will be reused (plus-or-minus an address or
two), building a mechanism that allows easy reuse of this code makes
sense in my mind. 

They way I'd propose implementing this (and is how I implemented it in
the above patch) is a per-unique-platform header. This would contain
certain values that might change depending on the platform (i.e. the
UART base address, machine name, etc.). It'd actually be nice to be able
to detect which of these paramters should be used during boot, but as
far as I know there's no universal mechanism to do this (for example,
Cisco hardware of the same series have board registers that contain a
device identifier).

> ok, I read the code too quickly. thanks.
> 
> Please note also that my remark about calling it everytime still
> stands. Reading a variable is taking less instructions than calling
> ioremap_nocache.

I'm not entirely sure I agree with you. While there is an overhead
incurred by using ioremap() on each character for prom_putc(), that
overhead is likely amortized by the fact you're waiting for the
8250-compatible's transmit buffer to clear. Those 5-6 instructions are
nothing compared to that!

Thanks for your comments, I'll try to take some of your ideas and
integrate them back into what I've been working on.

Cheers,
Phil

Re: [PATCH] Clean up Lemote Loongson 2E Support

[Zhang Le]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

On 06:03 Thu 23 Apr     , Philippe Vachon wrote:
> I've laid out the directories as follows:
> 
> arch/mips/
> 	+- loongson/
> 	|	+- common/ /* common code */
> 	|	+- gdium/    /* all the other directories are for */
> 	|	+- fulong-2e /* particular platforms */

Hi, Philippe,

Sorry for nitpicking, but fulong actually was a typo which slipped in somehow.
The correct English name should be fuloong as shown here:
http://www.lemote.com/english/fuloong.html

Also please take a look at this:
http://www.linux-mips.org/archives/linux-mips/2009-03/msg00225.html

BTW, Thank you for hard work on this. I really hope loongson 2f's code would be
merged soon, and have all the loongson related code sorted out.

> 	|
> 	...
> 	+- include/asm/
> 		+- mach-loongson/ 
> 			/* platform-specific headers */
> 			+- fulong-2e/
> 			+- gdium/
> 

-- 
Zhang, Le
Gentoo/Loongson Developer
http://zhangle.is-a-geek.org
0260 C902 B8F8 6506 6586 2B90 BC51 C808 1E4E 2973

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Clean up Lemote Loongson 2E Support

[Wu Zhangjin]

On Wed, 2009-04-22 at 11:15 +0200, Arnaud Patard wrote:
> Philippe Vachon <philippe@cowpig.ca> writes:
> 
> Hi,
> 
> > This patch eliminates magic numbers and adds accessors for memory-mapped
> > registers. As well, it removes some inline assembly and restructures how
> > the early printk code behaves.
> >
> > Signed-off-by: Philippe Vachon <philippe@cowpig.ca>
> > ---
> >  arch/mips/include/asm/mach-lemote/loongson2e.h |   60 +++++++++++
> >  arch/mips/lemote/lm2e/dbg_io.c                 |  125 ++---------------------
> >  arch/mips/lemote/lm2e/prom.c                   |   10 +--
> >  arch/mips/lemote/lm2e/reset.c                  |   18 ++--
> >  4 files changed, 82 insertions(+), 131 deletions(-)
> >  create mode 100644 arch/mips/include/asm/mach-lemote/loongson2e.h
> >
> > diff --git a/arch/mips/include/asm/mach-lemote/loongson2e.h b/arch/mips/include/asm/mach-lemote/loongson2e.h
> > new file mode 100644
> > index 0000000..82c1a95
> > --- /dev/null
> > +++ b/arch/mips/include/asm/mach-lemote/loongson2e.h
> > @@ -0,0 +1,60 @@
> > +/* Accessor functions for the Loongson 2E MMIO registers
> > + *
> > + * Copyright (c) 2009 Philippe Vachon <philippe@cowpig.ca>
> > + *
> > + * 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;  either version 2 of the  License, or (at your
> > + * option) any later version.
> > + *
> > + */
> > +#ifndef __ASM_MACH_LEMOTE_LOONGSON2E
> > +#define __ASM_MACH_LEMOTE_LOONGSON2E
> > +
> > +#include <linux/types.h>
> > +
> > +/* Loongson 2E Control Registers */
> > +#define LS2E_REG_BASE		0x1fe00100 /* start of config registers */
> > +#define LS2E_GENCFG_REG		(LS2E_REG_BASE + 0x04)
> > +
> > +#define LS2E_RESET_VECTOR	0x1fc00000 /* this should be obvious! */
> > +
> 
> Theses are neither Lemote nor 2E specifics. 2E and 2F controllers are
> very similar (and they're similar to the bonito stuff). Why do you need
> a specific header instead of using the Bonito64.h header for theses
> constants ? And if you really want to create it, please rename all to
> loongson and remove lemote references as it'll work on 2F and on boards
> from ST.
> 

I am working on merging the source code of fuloong+yeeloong(2f based)
and 2e, currently, the source code of fuloong & yeeloong have been
merged, and now, I am trying to merge 2e & 2f, lots of duplications have
been removed with the import of some new header files, and some MACROs,
Variables have been tuned. a first release may be out to dev.lemote.com
before this weekend. 

> > +/* UART address (16550 -- on the Fulong) */
> > +#define LS2E_UART_BASE		0x1fd003f8
> 
> It happens to be 0x1fd003f8 but it could have been at a different
> address base. For instance, on 2f, depending on the board, there's
> 0x1ff003f8 and 0x1fd003f8 (I think there's also some board with a
> different address than theses but I'm not sure).
> 

in fuloong(2f) & yeeloong(2f), the serial address is 0xbfd002f8, in
fuloong(2e), the serial address is 0xbfd003f8.

> > +/* Various system parameters passed from PMON */
> > +extern unsigned long bus_clock;
> > +extern unsigned long cpu_clock_freq;
> > +extern unsigned int memsize, highmemsize;
> > +
> > +static inline void ls2e_writeb(uint8_t value, unsigned long addr)
> > +{
> > +	*(volatile uint8_t *)addr = value;
> > +}
> > +
> 
> What about readl/writel and friends ?
> [...]
> 
> > -void debugInit(u32 baud, u8 data, u8 parity, u8 stop)
> > +void prom_putchar(char c)
> >  {
> > -	u32 divisor;
> > -
> > -	/* disable interrupts */
> > -	UART16550_WRITE(OFS_INTR_ENABLE, 0);
> > +	int timeout;
> > +	phys_addr_t uart_base = (phys_addr_t)ioremap_nocache(LS2E_UART_BASE, 8);
> > +	char reg = ls2e_readb(uart_base + UART_LSR) & UART_LSR_THRE;
> 
> hmm... I may be wrong on that but using ioremap looks here looks not a
> good idea. This code is called by early_printk so you can end up calling
> it very early in the boot process.
> Also, you're calling ioremap everytime this function is called. Why
> don't you do that only the first time ?
> 
> [...]
> 
> > diff --git a/arch/mips/lemote/lm2e/reset.c b/arch/mips/lemote/lm2e/reset.c
> > index 099387a..0989d28 100644
> > --- a/arch/mips/lemote/lm2e/reset.c
> > +++ b/arch/mips/lemote/lm2e/reset.c
> > @@ -7,20 +7,22 @@
> >   * Copyright (C) 2007 Lemote, Inc. & Institute of Computing Technology
> >   * Author: Fuxin Zhang, zhangfx@lemote.com
> >   */
> > +
> >  #include <linux/pm.h>
> > +#include <linux/io.h>
> > +#include <loongson2e.h>
> >  
> >  #include <asm/reboot.h>
> >  
> >  static void loongson2e_restart(char *command)
> >  {
> > -#ifdef CONFIG_32BIT
> > -	*(unsigned long *)0xbfe00104 &= ~(1 << 2);
> > -	*(unsigned long *)0xbfe00104 |= (1 << 2);
> > -#else
> > -	*(unsigned long *)0xffffffffbfe00104 &= ~(1 << 2);
> > -	*(unsigned long *)0xffffffffbfe00104 |= (1 << 2);
> > -#endif
> > -	__asm__ __volatile__("jr\t%0"::"r"(0xbfc00000));
> > +	uint32_t ctl =
> > +		(ls2e_readl((phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4))
> > +		& ~(1 << 2)) | 1 << 2;
> > +
> > +	ls2e_writel(ctl, (phys_addr_t)ioremap_nocache(LS2E_GENCFG_REG, 4));
> > +
> > +	((void (*)(void))ioremap_nocache(LS2E_RESET_VECTOR, 4))();
> 
> same remark as for the serial stuff. I'm really not sure that calling ioremap
> in such a place is a good idea (say, you've panic'ed and booted with
> panic=3, will this still work ?).
> 
> 
> Arnaud
>