| Message ID | 94a336e70a9b7682e914d917ebedc29a4447fa3e.1449834851.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
Hi Michal, On 2015年12月11日 19:54, Michal Simek wrote: > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > drivers/serial/Kconfig | 7 +++++++ > drivers/serial/serial_xuartlite.c | 23 +++++++++++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig > index 1fc287ee98ec..f1e221799b81 100644 > --- a/drivers/serial/Kconfig > +++ b/drivers/serial/Kconfig > @@ -92,6 +92,13 @@ config DEBUG_UART_S5P > will need to provide parameters to make this work. The driver will > be available until the real driver-model serial is running. > > +config DEBUG_UART_UARTLITE > + bool "Xilinx Uartlite" > + help > + Select this to enable a debug UART using the serial_uartlite driver. > + You will need to provide parameters to make this work. The driver will > + be available until the real driver-model serial is running. > + > config DEBUG_UART_ZYNQ > bool "Xilinx Zynq" > help > diff --git a/drivers/serial/serial_xuartlite.c b/drivers/serial/serial_xuartlite.c > index 10089f5a34b5..fe87b515d902 100644 > --- a/drivers/serial/serial_xuartlite.c > +++ b/drivers/serial/serial_xuartlite.c > @@ -120,3 +120,26 @@ U_BOOT_DRIVER(serial_uartlite) = { > .ops = &uartlite_serial_ops, > .flags = DM_FLAG_PRE_RELOC, > }; > + > +#ifdef CONFIG_DEBUG_UART_UARTLITE Better move the "#include <debug_uart.h>" here. > +void _debug_uart_init(void) Please add "static inline" to void _debug_uart_init(void). > +{ > + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; > + > + out_be32(®s->control, 0); > + out_be32(®s->control, ULITE_CONTROL_RST_RX | ULITE_CONTROL_RST_TX); > + in_be32(®s->control); > +} > + > +static inline void _debug_uart_putc(int ch) > +{ > + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; > + > + while (in_be32(®s->status) & SR_TX_FIFO_FULL) > + WATCHDOG_RESET(); WATCHDOG_RESET() is not really needed for debug serial output. > + > + out_be32(®s->tx_fifo, ch & 0xff); > +} > + > +DEBUG_UART_FUNCS > +#endif > Best regards, Thomas
Hi, On 14.12.2015 14:14, Thomas Chou wrote: > Hi Michal, > > On 2015年12月11日 19:54, Michal Simek wrote: >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> drivers/serial/Kconfig | 7 +++++++ >> drivers/serial/serial_xuartlite.c | 23 +++++++++++++++++++++++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig >> index 1fc287ee98ec..f1e221799b81 100644 >> --- a/drivers/serial/Kconfig >> +++ b/drivers/serial/Kconfig >> @@ -92,6 +92,13 @@ config DEBUG_UART_S5P >> will need to provide parameters to make this work. The driver >> will >> be available until the real driver-model serial is running. >> >> +config DEBUG_UART_UARTLITE >> + bool "Xilinx Uartlite" >> + help >> + Select this to enable a debug UART using the serial_uartlite >> driver. >> + You will need to provide parameters to make this work. The >> driver will >> + be available until the real driver-model serial is running. >> + >> config DEBUG_UART_ZYNQ >> bool "Xilinx Zynq" >> help >> diff --git a/drivers/serial/serial_xuartlite.c >> b/drivers/serial/serial_xuartlite.c >> index 10089f5a34b5..fe87b515d902 100644 >> --- a/drivers/serial/serial_xuartlite.c >> +++ b/drivers/serial/serial_xuartlite.c >> @@ -120,3 +120,26 @@ U_BOOT_DRIVER(serial_uartlite) = { >> .ops = &uartlite_serial_ops, >> .flags = DM_FLAG_PRE_RELOC, >> }; >> + >> +#ifdef CONFIG_DEBUG_UART_UARTLITE > > Better move the "#include <debug_uart.h>" here. This is the patch I sent some days ago about removing it from this location and it was Reviewed twice. http://lists.denx.de/pipermail/u-boot/2015-December/236341.html > >> +void _debug_uart_init(void) > > Please add "static inline" to void _debug_uart_init(void). Ok. Will fix this for uartlite and zynq uart in follow up patch. >> +{ >> + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; >> + >> + out_be32(®s->control, 0); >> + out_be32(®s->control, ULITE_CONTROL_RST_RX | >> ULITE_CONTROL_RST_TX); >> + in_be32(®s->control); >> +} >> + >> +static inline void _debug_uart_putc(int ch) >> +{ >> + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; >> + >> + while (in_be32(®s->status) & SR_TX_FIFO_FULL) >> + WATCHDOG_RESET(); > > WATCHDOG_RESET() is not really needed for debug serial output. TBH I don't think so. There could be watchdog running from early bootloader and needs to be service even for debugging purpose. Thanks, Michal
Hi Michal, On 2015年12月14日 23:50, Michal Simek wrote: > Hi, > > On 14.12.2015 14:14, Thomas Chou wrote: >> Hi Michal, >> >> On 2015年12月11日 19:54, Michal Simek wrote: >>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>> --- >>> >>> drivers/serial/Kconfig | 7 +++++++ >>> drivers/serial/serial_xuartlite.c | 23 +++++++++++++++++++++++ >>> 2 files changed, 30 insertions(+) >>> >>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig >>> index 1fc287ee98ec..f1e221799b81 100644 >>> --- a/drivers/serial/Kconfig >>> +++ b/drivers/serial/Kconfig >>> @@ -92,6 +92,13 @@ config DEBUG_UART_S5P >>> will need to provide parameters to make this work. The driver >>> will >>> be available until the real driver-model serial is running. >>> >>> +config DEBUG_UART_UARTLITE >>> + bool "Xilinx Uartlite" >>> + help >>> + Select this to enable a debug UART using the serial_uartlite >>> driver. >>> + You will need to provide parameters to make this work. The >>> driver will >>> + be available until the real driver-model serial is running. >>> + >>> config DEBUG_UART_ZYNQ >>> bool "Xilinx Zynq" >>> help >>> diff --git a/drivers/serial/serial_xuartlite.c >>> b/drivers/serial/serial_xuartlite.c >>> index 10089f5a34b5..fe87b515d902 100644 >>> --- a/drivers/serial/serial_xuartlite.c >>> +++ b/drivers/serial/serial_xuartlite.c >>> @@ -120,3 +120,26 @@ U_BOOT_DRIVER(serial_uartlite) = { >>> .ops = &uartlite_serial_ops, >>> .flags = DM_FLAG_PRE_RELOC, >>> }; >>> + >>> +#ifdef CONFIG_DEBUG_UART_UARTLITE >> >> Better move the "#include <debug_uart.h>" here. > > This is the patch I sent some days ago about removing it from this > location and it was Reviewed twice. > http://lists.denx.de/pipermail/u-boot/2015-December/236341.html > >> This is because commit 42800ffa7997 ("arm: zynq: Move serial driver to driver model") added the extra #include <debug_uart.h>. It is this one should be removed. The original one in commit c54c0a4c1c74 ("arm: zynq: Support the debug UART") is good. The wrong one was removed. >>> +void _debug_uart_init(void) >> >> Please add "static inline" to void _debug_uart_init(void). > > Ok. Will fix this for uartlite and zynq uart in follow up patch. > > >>> +{ >>> + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; >>> + >>> + out_be32(®s->control, 0); >>> + out_be32(®s->control, ULITE_CONTROL_RST_RX | >>> ULITE_CONTROL_RST_TX); >>> + in_be32(®s->control); >>> +} >>> + >>> +static inline void _debug_uart_putc(int ch) >>> +{ >>> + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; >>> + >>> + while (in_be32(®s->status) & SR_TX_FIFO_FULL) >>> + WATCHDOG_RESET(); >> >> WATCHDOG_RESET() is not really needed for debug serial output. > > TBH I don't think so. There could be watchdog running from early > bootloader and needs to be service even for debugging purpose. > Unless the serial input clock is dead, the serial shift out in UART won't take so long to trigger watchdog. Best regards, Thomas
Hi, On 15.12.2015 05:01, Thomas Chou wrote: > Hi Michal, > > On 2015年12月14日 23:50, Michal Simek wrote: >> Hi, >> >> On 14.12.2015 14:14, Thomas Chou wrote: >>> Hi Michal, >>> >>> On 2015年12月11日 19:54, Michal Simek wrote: >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>>> --- >>>> >>>> drivers/serial/Kconfig | 7 +++++++ >>>> drivers/serial/serial_xuartlite.c | 23 +++++++++++++++++++++++ >>>> 2 files changed, 30 insertions(+) >>>> >>>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig >>>> index 1fc287ee98ec..f1e221799b81 100644 >>>> --- a/drivers/serial/Kconfig >>>> +++ b/drivers/serial/Kconfig >>>> @@ -92,6 +92,13 @@ config DEBUG_UART_S5P >>>> will need to provide parameters to make this work. The driver >>>> will >>>> be available until the real driver-model serial is running. >>>> >>>> +config DEBUG_UART_UARTLITE >>>> + bool "Xilinx Uartlite" >>>> + help >>>> + Select this to enable a debug UART using the serial_uartlite >>>> driver. >>>> + You will need to provide parameters to make this work. The >>>> driver will >>>> + be available until the real driver-model serial is running. >>>> + >>>> config DEBUG_UART_ZYNQ >>>> bool "Xilinx Zynq" >>>> help >>>> diff --git a/drivers/serial/serial_xuartlite.c >>>> b/drivers/serial/serial_xuartlite.c >>>> index 10089f5a34b5..fe87b515d902 100644 >>>> --- a/drivers/serial/serial_xuartlite.c >>>> +++ b/drivers/serial/serial_xuartlite.c >>>> @@ -120,3 +120,26 @@ U_BOOT_DRIVER(serial_uartlite) = { >>>> .ops = &uartlite_serial_ops, >>>> .flags = DM_FLAG_PRE_RELOC, >>>> }; >>>> + >>>> +#ifdef CONFIG_DEBUG_UART_UARTLITE >>> >>> Better move the "#include <debug_uart.h>" here. >> >> This is the patch I sent some days ago about removing it from this >> location and it was Reviewed twice. >> http://lists.denx.de/pipermail/u-boot/2015-December/236341.html >> >>> > > This is because commit 42800ffa7997 ("arm: zynq: Move serial driver to > driver model") added the extra #include <debug_uart.h>. It is this one > should be removed. The original one in commit c54c0a4c1c74 ("arm: zynq: > Support the debug UART") is good. The wrong one was removed. I tend to have headers in the same location for the whole file It will be easier to include this file when CONFIG_DEBUG_UART is enabled. But no problem to move it. It is not causing any difference for me. >>>> +void _debug_uart_init(void) >>> >>> Please add "static inline" to void _debug_uart_init(void). >> >> Ok. Will fix this for uartlite and zynq uart in follow up patch. >> >> >>>> +{ >>>> + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; >>>> + >>>> + out_be32(®s->control, 0); >>>> + out_be32(®s->control, ULITE_CONTROL_RST_RX | >>>> ULITE_CONTROL_RST_TX); >>>> + in_be32(®s->control); >>>> +} >>>> + >>>> +static inline void _debug_uart_putc(int ch) >>>> +{ >>>> + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; >>>> + >>>> + while (in_be32(®s->status) & SR_TX_FIFO_FULL) >>>> + WATCHDOG_RESET(); >>> >>> WATCHDOG_RESET() is not really needed for debug serial output. >> >> TBH I don't think so. There could be watchdog running from early >> bootloader and needs to be service even for debugging purpose. >> > > Unless the serial input clock is dead, the serial shift out in UART > won't take so long to trigger watchdog. Uartlite driver is also the part of MDM which is console over jtag which can take some time. But TBH if you like, I will remove it in v2. Thanks, Michal
Hi Michal, On 2015年12月15日 23:38, Michal Simek wrote: > Hi, > > On 15.12.2015 05:01, Thomas Chou wrote: >> Hi Michal, >> >> On 2015年12月14日 23:50, Michal Simek wrote: >>> Hi, >>> >>> On 14.12.2015 14:14, Thomas Chou wrote: >>>> Hi Michal, >>>> >>>> On 2015年12月11日 19:54, Michal Simek wrote: >>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>>>> --- >>>>> >>>>> drivers/serial/Kconfig | 7 +++++++ >>>>> drivers/serial/serial_xuartlite.c | 23 +++++++++++++++++++++++ >>>>> 2 files changed, 30 insertions(+) >>>>> >>>>> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig >>>>> index 1fc287ee98ec..f1e221799b81 100644 >>>>> --- a/drivers/serial/Kconfig >>>>> +++ b/drivers/serial/Kconfig >>>>> @@ -92,6 +92,13 @@ config DEBUG_UART_S5P >>>>> will need to provide parameters to make this work. The driver >>>>> will >>>>> be available until the real driver-model serial is running. >>>>> >>>>> +config DEBUG_UART_UARTLITE >>>>> + bool "Xilinx Uartlite" >>>>> + help >>>>> + Select this to enable a debug UART using the serial_uartlite >>>>> driver. >>>>> + You will need to provide parameters to make this work. The >>>>> driver will >>>>> + be available until the real driver-model serial is running. >>>>> + >>>>> config DEBUG_UART_ZYNQ >>>>> bool "Xilinx Zynq" >>>>> help >>>>> diff --git a/drivers/serial/serial_xuartlite.c >>>>> b/drivers/serial/serial_xuartlite.c >>>>> index 10089f5a34b5..fe87b515d902 100644 >>>>> --- a/drivers/serial/serial_xuartlite.c >>>>> +++ b/drivers/serial/serial_xuartlite.c >>>>> @@ -120,3 +120,26 @@ U_BOOT_DRIVER(serial_uartlite) = { >>>>> .ops = &uartlite_serial_ops, >>>>> .flags = DM_FLAG_PRE_RELOC, >>>>> }; >>>>> + >>>>> +#ifdef CONFIG_DEBUG_UART_UARTLITE >>>> >>>> Better move the "#include <debug_uart.h>" here. >>> >>> This is the patch I sent some days ago about removing it from this >>> location and it was Reviewed twice. >>> http://lists.denx.de/pipermail/u-boot/2015-December/236341.html >>> >>>> >> >> This is because commit 42800ffa7997 ("arm: zynq: Move serial driver to >> driver model") added the extra #include <debug_uart.h>. It is this one >> should be removed. The original one in commit c54c0a4c1c74 ("arm: zynq: >> Support the debug UART") is good. The wrong one was removed. > > I tend to have headers in the same location for the whole file > It will be easier to include this file when CONFIG_DEBUG_UART is enabled. > But no problem to move it. It is not causing any difference for me. > > >>>>> +void _debug_uart_init(void) >>>> >>>> Please add "static inline" to void _debug_uart_init(void). >>> >>> Ok. Will fix this for uartlite and zynq uart in follow up patch. >>> >>> >>>>> +{ >>>>> + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; >>>>> + >>>>> + out_be32(®s->control, 0); >>>>> + out_be32(®s->control, ULITE_CONTROL_RST_RX | >>>>> ULITE_CONTROL_RST_TX); >>>>> + in_be32(®s->control); >>>>> +} >>>>> + >>>>> +static inline void _debug_uart_putc(int ch) >>>>> +{ >>>>> + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; >>>>> + >>>>> + while (in_be32(®s->status) & SR_TX_FIFO_FULL) >>>>> + WATCHDOG_RESET(); >>>> >>>> WATCHDOG_RESET() is not really needed for debug serial output. >>> >>> TBH I don't think so. There could be watchdog running from early >>> bootloader and needs to be service even for debugging purpose. >>> >> >> Unless the serial input clock is dead, the serial shift out in UART >> won't take so long to trigger watchdog. > > Uartlite driver is also the part of MDM which is console over jtag which > can take some time. > But TBH if you like, I will remove it in v2. > Now I understand. Either is fine. Thanks for the explanation. Best regards, Thomas
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index 1fc287ee98ec..f1e221799b81 100644 --- a/drivers/serial/Kconfig +++ b/drivers/serial/Kconfig @@ -92,6 +92,13 @@ config DEBUG_UART_S5P will need to provide parameters to make this work. The driver will be available until the real driver-model serial is running. +config DEBUG_UART_UARTLITE + bool "Xilinx Uartlite" + help + Select this to enable a debug UART using the serial_uartlite driver. + You will need to provide parameters to make this work. The driver will + be available until the real driver-model serial is running. + config DEBUG_UART_ZYNQ bool "Xilinx Zynq" help diff --git a/drivers/serial/serial_xuartlite.c b/drivers/serial/serial_xuartlite.c index 10089f5a34b5..fe87b515d902 100644 --- a/drivers/serial/serial_xuartlite.c +++ b/drivers/serial/serial_xuartlite.c @@ -120,3 +120,26 @@ U_BOOT_DRIVER(serial_uartlite) = { .ops = &uartlite_serial_ops, .flags = DM_FLAG_PRE_RELOC, }; + +#ifdef CONFIG_DEBUG_UART_UARTLITE +void _debug_uart_init(void) +{ + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; + + out_be32(®s->control, 0); + out_be32(®s->control, ULITE_CONTROL_RST_RX | ULITE_CONTROL_RST_TX); + in_be32(®s->control); +} + +static inline void _debug_uart_putc(int ch) +{ + struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE; + + while (in_be32(®s->status) & SR_TX_FIFO_FULL) + WATCHDOG_RESET(); + + out_be32(®s->tx_fifo, ch & 0xff); +} + +DEBUG_UART_FUNCS +#endif
Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- drivers/serial/Kconfig | 7 +++++++ drivers/serial/serial_xuartlite.c | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+)