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.
...
+static void catpt_dma_transfer_complete(void *arg) +{
- struct catpt_dev *cdev = arg;
- dev_dbg(cdev->dev, "%s\n", __func__);
Noise. Somebody should provide either trace events, of get used to function tracer / perf.
+}
...
+static bool catpt_dma_filter(struct dma_chan *chan, void *param) +{
- return chan->device->dev == (struct device *)param;
Redundant casting, and please, consider to swap left and right side (in this case easily to find these and perhaps provide a generic helper function).
+}
...
+#define CATPT_DMA_DEVID 1 /* dma engine used */
Not sure I understand what exactly this means.
...
+#define CATPT_DMA_MAXBURST 0x3
We have DMA engine definitions for that, please avoid magic numbers.
...
+#define CATPT_DMA_DSP_ADDR_MASK 0xFFF00000
GENMASK()
...
- dma_cap_set(DMA_SLAVE, mask);
How this helps with mem2mem channel?
...
- chan = dma_request_channel(mask, catpt_dma_filter, cdev->dev);
- if (!chan) {
dev_err(cdev->dev, "request channel failed\n");
dump_stack();
Huh?!
return ERR_PTR(-EPROBE_DEFER);- }
...
- 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?
- return (status == DMA_COMPLETE) ? 0 : -EPROTO;
...
+static void catpt_dsp_set_srampge(struct catpt_dev *cdev, struct resource *sram,
unsigned long mask, unsigned long new)+{
- unsigned long old;
- u32 off = sram->start;
- u32 b = __ffs(mask);
- old = catpt_readl_pci(cdev, VDRTCTL0) & mask;
- dev_dbg(cdev->dev, "SRAMPGE [0x%08lx] 0x%08lx -> 0x%08lx",
mask, old, new);
trace event / point?
- if (old == new)
return;- catpt_updatel_pci(cdev, VDRTCTL0, mask, new);
- udelay(60);
Delays should be commented.
- /*
* dummy read as the very first access after block enable* to prevent byte loss in future operations*/- for_each_clear_bit_from(b, &new, fls(mask)) {
fls_long()
u8 buf[4];/* newly enabled: new bit=0 while old bit=1 */if (test_bit(b, &old)) {dev_dbg(cdev->dev, "sanitize block %ld: off 0x%08x\n",
(b - __ffs(mask)), off);
Unneeded parentheses.
memcpy_fromio(buf, cdev->lpe_ba + off, sizeof(buf));}off += CATPT_MEMBLOCK_SIZE;- }
+}
...
- /* offset value given mask's start and invert it as ON=b0 */
- new <<= __ffs(mask);
- new = ~(new) & mask;
Unneeded parentheses. And perhaps in one line it will be better to understand:
new = ~(new << __ffs(mask)) & mask;
...
- if (waiti) {
/* wait for DSP to signal WAIT state */ret = catpt_readl_poll_shim(cdev, ISD,reg, (reg & CATPT_ISD_DCPWM),500, 10000);if (ret < 0) {
dev_warn(cdev->dev, "await WAITI timeout\n");
Shouldn't be an error level?
mutex_unlock(&cdev->clk_mutex);return ret;}- }
...
+int catpt_dsp_update_lpclock(struct catpt_dev *cdev) +{
- struct catpt_stream_runtime *stream;
- 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?
+}
...
- /* set D3 */
- catpt_updatel_pci(cdev, PMCS, CATPT_PMCS_PS, CATPT_PMCS_PS_D3HOT);
- udelay(50);
Don't we have PCI core function for this?
...
- /* set D0 */
- catpt_updatel_pci(cdev, PMCS, CATPT_PMCS_PS, 0);
- udelay(100);
Ditto.
...
- /* set D3 */
- catpt_updatel_pci(cdev, PMCS, CATPT_PMCS_PS, CATPT_PMCS_PS_D3HOT);
- udelay(50);
Ditto.
...
- /* set D0 */
- catpt_updatel_pci(cdev, PMCS, CATPT_PMCS_PS, 0);
Ditto.