Re: [U-Boot] [PATCH 41/71] serial: arm: Implement CONFIG_SERIAL_MULTI into atmel serial driver

2012-09-18 Thread Andreas Bießmann 

Dear Marek Vasut,

On 17.09.2012 01:21, Marek Vasut wrote:

Implement support for CONFIG_SERIAL_MULTI into atmel serial driver.
This driver was so far only usable directly, but this patch also adds
support for the multi method. This allows using more than one serial
driver alongside the atmel driver. Also, add a weak implementation
of default_serial_console() returning this driver.

Signed-off-by: Marek Vasut ma...@denx.de
Cc: Marek Vasut marek.va...@gmail.com
Cc: Tom Rini tr...@ti.com
Cc: Xu, Hong hong...@atmel.com
---


whole series runtime tested on avr32 and at91sam9260ek, therefore

Acked-by: Andreas Bießmann andreas.de...@googlemail.com

Best regards

Andreas Bießmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 41/71] serial: arm: Implement CONFIG_SERIAL_MULTI into atmel serial driver

2012-09-17 Thread Andreas Bießmann 
Dear Marek Vasut,

I have to admit that when reading this patch I got attention of your
UDM-serial.txt for the first time. However when reading this patch some
questions come to my mind.

On 17.09.12 01:21, Marek Vasut wrote:
 Implement support for CONFIG_SERIAL_MULTI into atmel serial driver.
 This driver was so far only usable directly, but this patch also adds
 support for the multi method. This allows using more than one serial
 driver alongside the atmel driver. Also, add a weak implementation
 of default_serial_console() returning this driver.
 
 Signed-off-by: Marek Vasut ma...@denx.de
 Cc: Marek Vasut marek.va...@gmail.com
 Cc: Tom Rini tr...@ti.com
 Cc: Xu, Hong hong...@atmel.com
 ---
  common/serial.c  |2 ++
  drivers/serial/atmel_usart.c |   67 
 ++
  2 files changed, 63 insertions(+), 6 deletions(-)
 
 diff --git a/common/serial.c b/common/serial.c
 index e6566da..b880303 100644
 --- a/common/serial.c
 +++ b/common/serial.c
 @@ -71,6 +71,7 @@ serial_initfunc(sconsole_serial_initialize);
  serial_initfunc(p3mx_serial_initialize);
  serial_initfunc(altera_jtag_serial_initialize);
  serial_initfunc(altera_serial_initialize);
 +serial_initfunc(atmel_serial_initialize);
  
  void serial_register(struct serial_device *dev)
  {
 @@ -120,6 +121,7 @@ void serial_initialize(void)
   p3mx_serial_initialize();
   altera_jtag_serial_initialize();
   altera_serial_initialize();
 + atmel_serial_initialize();
  
   serial_assign(default_serial_console()-name);
  }
 diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
 index 943ef70..d49d5d4 100644
 --- a/drivers/serial/atmel_usart.c
 +++ b/drivers/serial/atmel_usart.c
 @@ -20,6 +20,8 @@
   */
  #include common.h
  #include watchdog.h
 +#include serial.h
 +#include linux/compiler.h
  
  #include asm/io.h
  #include asm/arch/clk.h
 @@ -29,7 +31,7 @@
  
  DECLARE_GLOBAL_DATA_PTR;
  
 -void serial_setbrg(void)
 +static void atmel_serial_setbrg(void)
  {
   atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;

shouldn't this USART_BASE be carried by the driver struct in some way? I
wonder how one should implement multiple interfaces later on with this
atmel_serial_xx(void) interface.

   unsigned long divisor;
 @@ -45,7 +47,7 @@ void serial_setbrg(void)
   writel(USART3_BF(CD, divisor), usart-brgr);
  }
  
 -int serial_init(void)
 +static int atmel_serial_init(void)
  {
   atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
  
 @@ -73,7 +75,7 @@ int serial_init(void)
   return 0;
  }
  
 -void serial_putc(char c)
 +static void atmel_serial_putc(char c)
  {
   atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
  
 @@ -84,13 +86,13 @@ void serial_putc(char c)
   writel(c, usart-thr);
  }
  
 -void serial_puts(const char *s)
 +static void atmel_serial_puts(const char *s)
  {
   while (*s)
   serial_putc(*s++);
  }

I have seen this one in a lot of drivers ... shouldn't we build a
generic one?

  
 -int serial_getc(void)
 +static int atmel_serial_getc(void)
  {
   atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
  
 @@ -99,8 +101,61 @@ int serial_getc(void)
   return readl(usart-rhr);
  }
  
 -int serial_tstc(void)
 +static int atmel_serial_tstc(void)
  {
   atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
   return (readl(usart-csr)  USART3_BIT(RXRDY)) != 0;
  }
 +
 +#ifdef CONFIG_SERIAL_MULTI
 +static struct serial_device atmel_serial_drv = {
 + .name   = atmel_serial,

even though here is just one instance shouldn't the name reflect the
multiplicity of this driver (e.g. 'atmel_serial0')?

 + .start  = atmel_serial_init,
 + .stop   = NULL,
 + .setbrg = atmel_serial_setbrg,
 + .putc   = atmel_serial_putc,
 + .puts   = atmel_serial_puts,
 + .getc   = atmel_serial_getc,
 + .tstc   = atmel_serial_tstc,

As I understand this struct we need a start/stop/setbgr/... for each
instance we build.
Shouldn't we carry some void* private in this struct instead (I have
none seen in '[PATCH 01/71] serial: Coding style cleanup of struct
serial_device') to be able to reuse the interface with multiple
instances of the same driver class?
I think this is my main objection to this structure. I wonder how
existing SERIAL_MULTI implementations handle the need of private driver
information bound to an instance.

Best regards

Andreas Bießmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 41/71] serial: arm: Implement CONFIG_SERIAL_MULTI into atmel serial driver

2012-09-17 Thread Marek Vasut 
Dear Andreas Bießmann,

 Dear Marek Vasut,
 
 I have to admit that when reading this patch I got attention of your
 UDM-serial.txt for the first time. However when reading this patch some
 questions come to my mind.
[...]

  -void serial_setbrg(void)
  +static void atmel_serial_setbrg(void)
  
   {
   
  atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
 
 shouldn't this USART_BASE be carried by the driver struct in some way? I
 wonder how one should implement multiple interfaces later on with this
 atmel_serial_xx(void) interface.

We can't rework the whole stdio/serial subsystem right away. All such calls 
(serial_setbrg, serial_putc etc) will be augmented by one more parameter to 
push 
such information through at runtime. This will be done in subsequent patch, 
stage 1 in only a preparation stage.

  unsigned long divisor;
  
  @@ -45,7 +47,7 @@ void serial_setbrg(void)
  
  writel(USART3_BF(CD, divisor), usart-brgr);
   
   }
  
  -int serial_init(void)
  +static int atmel_serial_init(void)
  
   {
   
  atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
  
  @@ -73,7 +75,7 @@ int serial_init(void)
  
  return 0;
   
   }
  
  -void serial_putc(char c)
  +static void atmel_serial_putc(char c)
  
   {
   
  atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
  
  @@ -84,13 +86,13 @@ void serial_putc(char c)
  
  writel(c, usart-thr);
   
   }
  
  -void serial_puts(const char *s)
  +static void atmel_serial_puts(const char *s)
  
   {
   
  while (*s)
  
  serial_putc(*s++);
   
   }
 
 I have seen this one in a lot of drivers ... shouldn't we build a
 generic one?

Indeed, but only in stage 2 or somewhere later ... I have that in mind, but the 
serial subsystem needs a bit of a patching for that.

  -int serial_getc(void)
  +static int atmel_serial_getc(void)
  
   {
   
  atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
  
  @@ -99,8 +101,61 @@ int serial_getc(void)
  
  return readl(usart-rhr);
   
   }
  
  -int serial_tstc(void)
  +static int atmel_serial_tstc(void)
  
   {
   
  atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
  return (readl(usart-csr)  USART3_BIT(RXRDY)) != 0;
   
   }
  
  +
  +#ifdef CONFIG_SERIAL_MULTI
  +static struct serial_device atmel_serial_drv = {
  +   .name   = atmel_serial,
 
 even though here is just one instance shouldn't the name reflect the
 multiplicity of this driver (e.g. 'atmel_serial0')?

This is the name of the driver, not the name of the instance of the driver. I'd 
like to add .id field later on.

  +   .start  = atmel_serial_init,
  +   .stop   = NULL,
  +   .setbrg = atmel_serial_setbrg,
  +   .putc   = atmel_serial_putc,
  +   .puts   = atmel_serial_puts,
  +   .getc   = atmel_serial_getc,
  +   .tstc   = atmel_serial_tstc,
 
 As I understand this struct we need a start/stop/setbgr/... for each
 instance we build.

No, this isn't instance. This are driver ops combined with it's name. I can not 
split it yet.

 Shouldn't we carry some void* private in this struct instead (I have
 none seen in '[PATCH 01/71] serial: Coding style cleanup of struct
 serial_device') to be able to reuse the interface with multiple
 instances of the same driver class?

Yes, but not now, not yet. I'm trying to keep this changes incremental as much 
as possible.

 I think this is my main objection to this structure. I wonder how
 existing SERIAL_MULTI implementations handle the need of private driver
 information bound to an instance.

They have multiple drivers so far and a default_serial_console call. That is 
indeed stupid, but fixing this is not part of this patchset, but a subsequent 
one. This one is only a preparation, trying not to break anything and unify the 
drivers under the serial_multi api, so the further stages can easily continue 
reworking it.

 Best regards
 
 Andreas Bießmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 41/71] serial: arm: Implement CONFIG_SERIAL_MULTI into atmel serial driver

2012-09-17 Thread Andreas Bießmann 
Dear Marek Vasut,

On 17.09.2012 12:00, Marek Vasut wrote:
 Dear Andreas Bießmann,
 
 Dear Marek Vasut,

 I have to admit that when reading this patch I got attention of your
 UDM-serial.txt for the first time. However when reading this patch some
 questions come to my mind.
 [...]
 
 -void serial_setbrg(void)
 +static void atmel_serial_setbrg(void)

  {
  
 atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;

 shouldn't this USART_BASE be carried by the driver struct in some way? I
 wonder how one should implement multiple interfaces later on with this
 atmel_serial_xx(void) interface.
 
 We can't rework the whole stdio/serial subsystem right away. All such calls 
 (serial_setbrg, serial_putc etc) will be augmented by one more parameter to 
 push 
 such information through at runtime. This will be done in subsequent patch, 
 stage 1 in only a preparation stage.

Ok.

snip

 -void serial_puts(const char *s)
 +static void atmel_serial_puts(const char *s)

  {
  
 while (*s)
 
 serial_putc(*s++);
  
  }

 I have seen this one in a lot of drivers ... shouldn't we build a
 generic one?
 
 Indeed, but only in stage 2 or somewhere later ... I have that in mind, but 
 the 
 serial subsystem needs a bit of a patching for that.

Ok.

snip

 +
 +#ifdef CONFIG_SERIAL_MULTI
 +static struct serial_device atmel_serial_drv = {
 +   .name   = atmel_serial,

 even though here is just one instance shouldn't the name reflect the
 multiplicity of this driver (e.g. 'atmel_serial0')?
 
 This is the name of the driver, not the name of the instance of the driver. 
 I'd 
 like to add .id field later on.

Ah, ok. Sounds good.

 +   .start  = atmel_serial_init,
 +   .stop   = NULL,
 +   .setbrg = atmel_serial_setbrg,
 +   .putc   = atmel_serial_putc,
 +   .puts   = atmel_serial_puts,
 +   .getc   = atmel_serial_getc,
 +   .tstc   = atmel_serial_tstc,

 As I understand this struct we need a start/stop/setbgr/... for each
 instance we build.
 
 No, this isn't instance. This are driver ops combined with it's name. I can 
 not 
 split it yet.
 
 Shouldn't we carry some void* private in this struct instead (I have
 none seen in '[PATCH 01/71] serial: Coding style cleanup of struct
 serial_device') to be able to reuse the interface with multiple
 instances of the same driver class?
 
 Yes, but not now, not yet. I'm trying to keep this changes incremental as 
 much 
 as possible.
 
 I think this is my main objection to this structure. I wonder how
 existing SERIAL_MULTI implementations handle the need of private driver
 information bound to an instance.
 
 They have multiple drivers so far and a default_serial_console call. That is 
 indeed stupid, but fixing this is not part of this patchset, but a subsequent 
 one. This one is only a preparation, trying not to break anything and unify 
 the 
 drivers under the serial_multi api, so the further stages can easily continue 
 reworking it.

Understood, I'm fine with it. I will run a test on avr32/some at91 board
these days and send my ACK then.

Best regards

Andreas Bießmann
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 41/71] serial: arm: Implement CONFIG_SERIAL_MULTI into atmel serial driver

2012-09-17 Thread Marek Vasut 
Dear Andreas Bießmann,

[...]

  They have multiple drivers so far and a default_serial_console call. That
  is indeed stupid, but fixing this is not part of this patchset, but a
  subsequent one. This one is only a preparation, trying not to break
  anything and unify the drivers under the serial_multi api, so the
  further stages can easily continue reworking it.
 
 Understood, I'm fine with it. I will run a test on avr32/some at91 board
 these days and send my ACK then.

Thank you!

Please note, I have the subsequent stages planned and some already being worked 
on, but please let's wait with them until this gets into -next.

 Best regards
 
 Andreas Bießmann

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 41/71] serial: arm: Implement CONFIG_SERIAL_MULTI into atmel serial driver

2012-09-16 Thread Marek Vasut 
Implement support for CONFIG_SERIAL_MULTI into atmel serial driver.
This driver was so far only usable directly, but this patch also adds
support for the multi method. This allows using more than one serial
driver alongside the atmel driver. Also, add a weak implementation
of default_serial_console() returning this driver.

Signed-off-by: Marek Vasut ma...@denx.de
Cc: Marek Vasut marek.va...@gmail.com
Cc: Tom Rini tr...@ti.com
Cc: Xu, Hong hong...@atmel.com
---
 common/serial.c  |2 ++
 drivers/serial/atmel_usart.c |   67 ++
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/common/serial.c b/common/serial.c
index e6566da..b880303 100644
--- a/common/serial.c
+++ b/common/serial.c
@@ -71,6 +71,7 @@ serial_initfunc(sconsole_serial_initialize);
 serial_initfunc(p3mx_serial_initialize);
 serial_initfunc(altera_jtag_serial_initialize);
 serial_initfunc(altera_serial_initialize);
+serial_initfunc(atmel_serial_initialize);
 
 void serial_register(struct serial_device *dev)
 {
@@ -120,6 +121,7 @@ void serial_initialize(void)
p3mx_serial_initialize();
altera_jtag_serial_initialize();
altera_serial_initialize();
+   atmel_serial_initialize();
 
serial_assign(default_serial_console()-name);
 }
diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
index 943ef70..d49d5d4 100644
--- a/drivers/serial/atmel_usart.c
+++ b/drivers/serial/atmel_usart.c
@@ -20,6 +20,8 @@
  */
 #include common.h
 #include watchdog.h
+#include serial.h
+#include linux/compiler.h
 
 #include asm/io.h
 #include asm/arch/clk.h
@@ -29,7 +31,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-void serial_setbrg(void)
+static void atmel_serial_setbrg(void)
 {
atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
unsigned long divisor;
@@ -45,7 +47,7 @@ void serial_setbrg(void)
writel(USART3_BF(CD, divisor), usart-brgr);
 }
 
-int serial_init(void)
+static int atmel_serial_init(void)
 {
atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
 
@@ -73,7 +75,7 @@ int serial_init(void)
return 0;
 }
 
-void serial_putc(char c)
+static void atmel_serial_putc(char c)
 {
atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
 
@@ -84,13 +86,13 @@ void serial_putc(char c)
writel(c, usart-thr);
 }
 
-void serial_puts(const char *s)
+static void atmel_serial_puts(const char *s)
 {
while (*s)
serial_putc(*s++);
 }
 
-int serial_getc(void)
+static int atmel_serial_getc(void)
 {
atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
 
@@ -99,8 +101,61 @@ int serial_getc(void)
return readl(usart-rhr);
 }
 
-int serial_tstc(void)
+static int atmel_serial_tstc(void)
 {
atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
return (readl(usart-csr)  USART3_BIT(RXRDY)) != 0;
 }
+
+#ifdef CONFIG_SERIAL_MULTI
+static struct serial_device atmel_serial_drv = {
+   .name   = atmel_serial,
+   .start  = atmel_serial_init,
+   .stop   = NULL,
+   .setbrg = atmel_serial_setbrg,
+   .putc   = atmel_serial_putc,
+   .puts   = atmel_serial_puts,
+   .getc   = atmel_serial_getc,
+   .tstc   = atmel_serial_tstc,
+};
+
+void atmel_serial_initialize(void)
+{
+   serial_register(atmel_serial_drv);
+}
+
+__weak struct serial_device *default_serial_console(void)
+{
+   return atmel_serial_drv;
+}
+#else
+int serial_init(void)
+{
+   return atmel_serial_init();
+}
+
+void serial_setbrg(void)
+{
+   atmel_serial_setbrg();
+}
+
+void serial_putc(const char c)
+{
+   atmel_serial_putc(c);
+}
+
+void serial_puts(const char *s)
+{
+   atmel_serial_puts(s);
+}
+
+int serial_getc(void)
+{
+   return atmel_serial_getc();
+}
+
+int serial_tstc(void)
+{
+   return atmel_serial_tstc();
+}
+#endif
-- 
1.7.10.4

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot