| Message ID | 1347837696-3192-42-git-send-email-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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 <marex@denx.de> > Cc: Marek Vasut <marek.vasut@gmail.com> > Cc: Tom Rini <trini@ti.com> > Cc: Xu, Hong <Hong.Xu@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
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
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
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
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 <marex@denx.de> > Cc: Marek Vasut <marek.vasut@gmail.com> > Cc: Tom Rini <trini@ti.com> > Cc: Xu, Hong <Hong.Xu@atmel.com> > --- whole series runtime tested on avr32 and at91sam9260ek, therefore Acked-by: Andreas Bießmann <andreas.devel@googlemail.com> Best regards Andreas Bießmann
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
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 <marex@denx.de> Cc: Marek Vasut <marek.vasut@gmail.com> Cc: Tom Rini <trini@ti.com> Cc: Xu, Hong <Hong.Xu@atmel.com> --- common/serial.c | 2 ++ drivers/serial/atmel_usart.c | 67 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 6 deletions(-)