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