A DW UART driver for Sue Creek - #957
Conversation
There was a problem hiding this comment.
Should be pre-proc'ed by CONFIG_UART (feature-orineted).
There was a problem hiding this comment.
CONFIG_TRACE_UART & CONFIG_TRACE_DMA could be more feature-oriented compilation options.
There was a problem hiding this comment.
These should all be in a struct dw_uart.
There was a problem hiding this comment.
@lgirdwood speaking of which: do we need a device object to pass it around? We'd need a global struct uart *trace_uart pointer then.
There was a problem hiding this comment.
This should be a platform macro too.
There was a problem hiding this comment.
Sorry, could you elaborate a bit. Currently that macro is defined in src/platform/suecreek/include/platform/interrupt.h i.e. it is already platform-specific. Or you mean the whole thing should be
#define PLATFORM_UART_IRQ IRQ_EXT_HOST_UART(0)
or similar? Actually wondering what that "EXT_HOST" part of the name is supposed to mean, I think I'll change it too. In fact we can remove that struct from the driver and put it in a platform file, but as long as we don't have any other uses for UARTs as a log console and we don't foresee having more than one of them, we can keep this as is.
There was a problem hiding this comment.
I mean
#define PLATFORM_UART_IRQ IRQ_WHATEVER
in platform.h
There was a problem hiding this comment.
I'd expect to see a struct uart_ops here with pointer to each UART operation. That way clients would call uart_write() instead of dw_write(). A generic struct sof_uart would also need to be defined and passed into each operation.
There was a problem hiding this comment.
can we also do it like I've done for SPI / GPIO / IOMUX? Just use generic names for exported functions and rely on linking no more than just one required driver? So I'll just drop "dw_" prefixes from exported functions and types.
There was a problem hiding this comment.
no, your solution breaks when > 1 devices exists. please use existing convention.
There was a problem hiding this comment.
You mean you want to be able to handle multiple UART models / drivers simultaneously? Ok, sure, this version only works when there's a single UART, used only for logging (the driver cannot even receive data). I'll add ops then.
There was a problem hiding this comment.
This should map onto UART driver ops via generic APIs
There was a problem hiding this comment.
This would be a generic uart = uart_init(any params) and we would use a CONFIG_CAVS_UART (which selects DW_UART in Kconfig) instead of CONFIG_USE_CREEK
97c8373 to
90c2df0
Compare
|
...once again I don't understand the reason for the "build" CI test, any ideas? |
|
Ask ci team. |
|
|
We encountered problems with tests repository server. Now everything should be back on line. |
|
@lgirdwood @mmaka1 ping any more comments? good to merge? |
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Please look at ssp.c an dw-dma from driver model info and I need to see more comments describing HW programming flow.
There was a problem hiding this comment.
I mean
#define PLATFORM_UART_IRQ IRQ_WHATEVER
in platform.h
There was a problem hiding this comment.
Can you add more comments in this function for each block
There was a problem hiding this comment.
no, your solution breaks when > 1 devices exists. please use existing convention.
Using macro arguments multiple times within the macro definition body can lead to repeated expressioon evaluation. To avoid that replace MIN() and MAX() macros with safer versions. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
This patch adds a driver for the DesignWare UART IP, used on Sue Creek boards for both the boot-loader and for the SOF runtime proper. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
On Sue Creek the UART is at 0x80800, not at 0x88000. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The DW UART driver support both trace_point() and trace_event() style debugging. For trace_event() it uses an interrupt. However, on Sue Creek the UART interrupt cannot be used yet. Enable only the trace_point() part of it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
@lgirdwood something like this? |
| static inline int uart_write(struct uart *uart, | ||
| const char *data, uint32_t size) | ||
| { | ||
| return uart ? uart->ops->write(uart, data, size) : -ENODEV; |
There was a problem hiding this comment.
Can you also change this to check for write e.g.
if (uart && uart->ops->write)
uart->ops->write();
As some drivers may not implement all the ops. Ditto for others.
| @@ -0,0 +1,105 @@ | |||
| /* | |||
| * Copyright (c) 2017-2018, Intel Corporation | |||
There was a problem hiding this comment.
Please switch to SPDX identifier in this and other added files
|
@lyakh Is this still valid? |
|
@lyakh Do you plan to work on the PR ? I'd like to close obsolete PRs. |
|
@lbetlej let's close it for now |
8 participants
This adds a DW UART driver and enables it for only the trace_point() functionality for Sue Creek, until support for DW INTC is added too.