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.
--
With Best Regards,
Andy Shevchenko
