Re: [U-Boot] [PATCH 2/3] serial: uartlite: Add support for debug console

[Michal Simek]

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.si...@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(&regs->control, 0);
>>>> +    out_be32(&regs->control, ULITE_CONTROL_RST_RX |
>>>> ULITE_CONTROL_RST_TX);
>>>> +    in_be32(&regs->control);
>>>> +}
>>>> +
>>>> +static inline void _debug_uart_putc(int ch)
>>>> +{
>>>> +    struct uartlite *regs = (struct uartlite *)CONFIG_DEBUG_UART_BASE;
>>>> +
>>>> +    while (in_be32(&regs->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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot