On Mon, Aug 17, 2020 at 01:12:01PM +0200, Cezary Rojewski wrote:
On 2020-08-13 8:51 PM, Andy Shevchenko wrote:
On Wed, Aug 12, 2020 at 10:57:42PM +0200, Cezary Rojewski wrote:
Implement dsp lifecycle functions such as core RESET and STALL, SRAM power control and LP clock selection. This also adds functions for handling transport over DW DMA controller.
Thanks for your input Andy!
You're welcome!
...
+#define CATPT_DMA_DEVID 1 /* dma engine used */
Not sure I understand what exactly this means.
Well, you may choose either engine 0 or 1 for loading images. Reference solution which I'm basing catpt on - Windows driver equivalent - makes use of engine 1. Goal of this implementation is to align closely to stable Windows solution wherever possible to reduce maintainance cost.
Please, give extended comment here.
...
- status = dma_wait_for_async_tx(desc);
- catpt_updatel_shim(cdev, HMDC,
CATPT_HMDC_HDDA(CATPT_DMA_DEVID, chan->chan_id), 0);Update even in erroneous case?
Yes. This is based on stable Windows solution equivalent and get's updated even in failure case to disable access to HOST memory in demand more.
I guess this deserves a comment.
- return (status == DMA_COMPLETE) ? 0 : -EPROTO;
...
- new <<= __ffs(mask);
- new = ~(new) & mask;
Unneeded parentheses. And perhaps in one line it will be better to understand:
new = ~(new << __ffs(mask)) & mask;
Was called out in the past not to combine everything in one-line like if I'm to hide something from reviewer.
No problem with combining these together in v5.
you also may consider to use u32_replace_bits() or so.
...
- bool lp;
- if (list_empty(&cdev->stream_list))
return catpt_dsp_select_lpclock(cdev, true, true);- lp = true;
- list_for_each_entry(stream, &cdev->stream_list, node) {
if (stream->prepared) {lp = false;break;}- }
- return catpt_dsp_select_lpclock(cdev, lp, true);
Seems too much duplication.
struct catpt_stream_runtime *stream;
list_for_each_entry(stream, &cdev->stream_list, node) { if (stream->prepared) return catpt_dsp_select_lpclock(cdev, false, true); }
return catpt_dsp_select_lpclock(cdev, true, true);
Better?
list_first_entry (part of list_for_each_entry) expects list to be non-empty. ->streal_list may be empty when invoking catpt_dsp_update_lpclock().
I didn't get this. Can you point out where is exactly problematic place?