Re: [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>
Cc: pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org, filip.kaczmarski@intel.com,
	harshapriya.n@intel.com, marcin.barlik@intel.com,
	zwisler@google.com, lgirdwood@gmail.com, tiwai@suse.com,
	filip.proborszcz@intel.com, broonie@kernel.org,
	amadeuszx.slawinski@linux.intel.com, michal.wasko@intel.com,
	cujomalainey@chromium.org, krzysztof.hejmowski@intel.com,
	ppapierkowski@habana.ai, vamshi.krishna.gopal@intel.com
Subject: Re: [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point
Date: Fri, 18 Sep 2020 16:55:46 +0300	[thread overview]
Message-ID: <20200918135546.GE3956970@smile.fi.intel.com> (raw)
In-Reply-To: <20200917141242.9081-1-cezary.rojewski@intel.com>

On Thu, Sep 17, 2020 at 04:12:28PM +0200, Cezary Rojewski wrote:
> Implement support for Lynxpoint and Wildcat Point AudioDSP. Catpt
> solution deprecates existing sound/soc/intel/haswell which is removed in
> the following series. This cover-letter is followed by 'Developer's deep
> dive' message schedding light on catpt's key concepts and areas
> addressed.
> 
> Due to high range of errors and desynchronization from recommendations
> set by Windows solution, re-write came as a lower-cost solution compared
> to refactoring /haswell/ with several series of patches.
> 
> Series is dependent on linux-spi change:
> spi: pxa2xx: Add SSC2 and SSPSP2 SSP registers
> https://www.spinics.net/lists/linux-spi/msg23885.html
> which has been already merged and is now part of linux-spi tree.
> 
> Bulk of series content is device driver core code - everything up to
> patch 7/14 - with fs entries and trace macros introduced right after.
> While each core patch is shaped in such a way that no unavailable
> members are ever called, until patch 10/14 is applied, no code
> compilation can occur as no Makefile is present. Once said patch is
> added, Makefile and Kconfig are implemented and driver module compiles
> as expected.
> 
> 
> Special thanks go to Marcin Barlik and Piotr Papierkowski for sharing
> their LPT/WPT AudioDSP architecture expertise as well as helping
> backtrack its historical background.
> My thanks go to Amadeusz Slawinski for reviews and improvements proposed
> on and off the internal list. Most of internal diff below is his
> contribution.
> Krzysztof Hejmowski helped me setup my own Xtensa environment and
> recompile LPT/WPT FW binary sources what sped up the development greatly.
> 
> This would not have been possible without help from these champions,
> especially considering how quickly the catpt was written: 2 weeks
> features, 3 weeks optimizations. Thank you.
> 
> Userspace-exposed members are compatible with what is exposed by
> deprecated solution as well as FW binary being re-used thus no harm is
> done. The only visible differences are: the newly added 'Loopback Mute'
> kcontrol and volume support extending to quad from stereo.
> 
> On top of fixing erros and design flows, catpt also adds module reload,
> dynamic SRAM memory allocation during PCM runtime and exposes missing
> userspace API: 'Loopback Mute' kcontrol, quad volume controls and sysfs
> fw-version entries. Event tracing is provided to easy solution
> debugging.
> 
> Following are not included in this update and are scheduled as later
> addition:
> - fw logging
> - module (library) support
> 
> Note: LPT power up/down sequences might get aligned with WPT once enough
> testing is done as capabilities are shared for both DSPs.
> Note #2: Both LPT and WPT power up/down sequences may get optimized in
> future updates as thanks to help from the Windows team, most of nuances
> behind why/what/when in regard to hw registers have been backtracked and
> reviewed again.
> 
> Link to developer's deep dive message:
> https://www.spinics.net/lists/alsa-devel/msg113563.html

Okay, I don't see any reason to resend a series right now and all my comments
that left are rather nit-picks, so

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Changes in v6:
> - reordered and reorganized code for patches 1/13 - 8/13 of v5, so each
>   patches makes use of no member or function which is unavailable to it.
>   Series size increased from 13 to 14 patches: addition of base members
>   e.g.: registers has been split from addition of device.c file which
>   describes acpi device behavior
> 
> 
> Changes in v5:
> https://www.spinics.net/lists/alsa-devel/msg115621.html
> Basically everything below is result of Andy's review. Thank you Andy
> for taking time into this detailed review
> 
> - catpt now makes use of common linux/pxa2xx_ssp.h header file, removing
>   redundant SSP register declarations in the process. As stated in the
>   opening, this is dependent upon linux-spi change:
>   spi: pxa2xx: Add SSC2 and SSPSP2 SSP registers
> 
> - updated Kconfig by removing DMADEVICES and adding COMPILE_TEST
>   as optional depends-on
> - updated all register macros definitions to be more safe against common
>   arithmetics when specifying macro's parameters
> - removed CONFIG_PM and CONFIG_PM_SLEEP usage in favor of __maybe_unused
> - all 'if (ret < 0)' converted to simple 'if (ret)' whenever possible
> - fixed erroneous check for platform_device_register_data within
>   catpt_register_board()
> - _SLAVE/_MASTER replaced with more inclusive _CONSUMER/_PROVIDER for
>   enum catpt_ssp_mode
> - catpt_acpi_probe() is now making use of high-level wrappers for
>   ioremapping and resource assignment, reducing function's code size
> - due to improved catpt_acpi_probe() behavior, catpt_acpi_remove() needs
>   not to cast dma_free_coherent() any longer
> - DMA source and destrination maxburst now of value 16, see:
> https://www.spinics.net/lists/alsa-devel/msg114394.html
> 
> - simplified catpt_dsp_update_lpclock() as list_for_each_entry() is
>   empty-safe by default
> - dropped '_SSP_' from all names of all CATPT_SSP_SSXXX_DEFAULT macros
> - catpt_updatel_pci now makes use of linux/pci.h and uapi/linux/pci.h
>   constants such as: PCI_PM_CTRL_STATE_MASK and PCI_D3hot
> 
> 
> Changes in v4:
> https://www.spinics.net/lists/alsa-devel/msg113762.html
> - fixed compilation with i386 kconfig (conflicting names)
> - streamlined naming for SHIM and PCI registers to match SSP ones
>   (SHIM_REG -> SHIM)
> - catpt_component_probe removed and kcontrols again initializzed
>   statically via snd_kcontrol_new array: this is to remove
>   kctl->id.device shenanigans
> - renamed catpt_set_ctlvol to catpt_set_dspvol - function name wasn't
>   matching its purpose
> 
> 
> Changes in v3:
> - fixed IRAM mask usage in lpt_dsp_power_up (dsp.c)
> - updated dbg message formatting in catpt_restore_fwimage as suggested
>   by Andy
> - fixed alignment for struct catpt_ssp_device_format
> - catpt_set_ctlvol now verifies all-equal scenario based on all
>   channels rather than just first two as requested by Amadeo
> - fixed SPDX for registers.h
> 
> 
> Changes in v2:
> https://www.spinics.net/lists/alsa-devel/msg113660.html
> - fixed SPDX formatting for all header files as well as pcm.c
> - fixed size provided to memcpy() in fw_build_read() as reported by Mark
> - renamed struct catpt_pdata to struct catpt_spec (cosmetic)
> - fixed erroneous path in catpt_load_block: region is properly released
> - trace.h events for updating registers have been removed and usages
>   replaced by dev_dbg (SRAMPGE/ LPCS)
> 
> - as requested by Andy, struct resource has replaced struct catpt_mbank
>   and struct catpt_mregion. This change cascaded into:
> 
>   - catpt_mbank_size and catpt_mregion_size replaced by resource_size
>   - catpt_mregion_intersects replaced by resource_overlaps
>   - all catpt_mbank_ and catpt_mregion_ handlers found in loader.c
>     (_request, _reserve, _release, _extract, _split, _join) have been
>     removed
>   - __request_region and __release_region have been enlisted in their
>     place
>   - catpt_mregion_intersecting renamed to catpt_resource_overlapping
>   - catpt_request_region helper has been provided to deal with -size
>     based requests
>       o haven't found direct replacements in resource.c/ ioport.h for
>       both functions
> 
>   - catpt_mbank_create and catpt_mbank_remove renamed to catpt_sram_init
>     and catpt_sram_free respectively
>   - catpt_sram_init now returns void instead of int and has been
>     converted to simple initialized. This change ultimately cascaded
>     into:
>       o both SRAM banks initialization being moved to catpt_dev_init
>         from catpt_acpi_probe (device.c)
>       o catpt_dev::spec is now initialized first, with catpt_dev_init
>         following it soon after
>       o catpt_acpi_probe erroneous path has been simplified as SRAM
>         banks no longer need to be freed
> 
>   - catpt_sram_free now frees all resources via child -> sibling
>     enumeration rather than region_list iteration
>   - catpt_dsp_update_srampge and catpt_dsp_set_srampge now accept new
>     argument: unsigned long mask. Caused by removal of catpt_mbank -
>     mask is taken directly from catpt_dev::spec::d/iram_mask
>   - trace.h events for catpt_mbank and catpt_mregion have been removed
> 
> 
> Diff against last drop on internal list:
> https://www.spinics.net/lists/alsa-devel/msg113549.html
> - replaced spinlock with mutex for mregion allocation and release to
>   address sleeping in atomic context warnings
> - fixed coredump fw_hash dumping
> - kcontrol values are now always stored regardless of stream of interest
>   is running or not
> - kcontrol values are now applied after stream is prepared instead of
>   ignoring what has been set by user initially
> - catpt_pdata instances have been renamed from hsw_ and bdw_ to lpt_ and
>   wpt_ respectively
> - reordered Makefile .o(s) (cosmetic)
> 
> Cezary Rojewski (14):
>   ASoC: Intel: Add catpt base members
>   ASoC: Intel: catpt: Implement IPC protocol
>   ASoC: Intel: catpt: Add IPC message handlers
>   ASoC: Intel: catpt: Define DSP operations
>   ASoC: Intel: catpt: Firmware loading and context restore
>   ASoC: Intel: catpt: PCM operations
>   ASoC: Intel: catpt: Device driver lifecycle
>   ASoC: Intel: catpt: Event tracing
>   ASoC: Intel: catpt: Simple sysfs attributes
>   ASoC: Intel: Select catpt and deprecate haswell
>   ASoC: Intel: haswell: Remove haswell-solution specific code
>   ASoC: Intel: broadwell: Remove haswell-solution specific code
>   ASoC: Intel: bdw-5650: Remove haswell-solution specific code
>   ASoC: Intel: bdw-5677: Remove haswell-solution specific code
> 
>  sound/soc/intel/Kconfig             |   24 +-
>  sound/soc/intel/Makefile            |    2 +-
>  sound/soc/intel/boards/Kconfig      |    8 +-
>  sound/soc/intel/boards/bdw-rt5650.c |   36 -
>  sound/soc/intel/boards/bdw-rt5677.c |   33 -
>  sound/soc/intel/boards/broadwell.c  |   33 -
>  sound/soc/intel/boards/haswell.c    |   28 +-
>  sound/soc/intel/catpt/Makefile      |    6 +
>  sound/soc/intel/catpt/core.h        |  189 +++++
>  sound/soc/intel/catpt/device.c      |  354 ++++++++
>  sound/soc/intel/catpt/dsp.c         |  572 +++++++++++++
>  sound/soc/intel/catpt/fs.c          |   79 ++
>  sound/soc/intel/catpt/ipc.c         |  298 +++++++
>  sound/soc/intel/catpt/loader.c      |  671 +++++++++++++++
>  sound/soc/intel/catpt/messages.c    |  313 +++++++
>  sound/soc/intel/catpt/messages.h    |  401 +++++++++
>  sound/soc/intel/catpt/pcm.c         | 1212 +++++++++++++++++++++++++++
>  sound/soc/intel/catpt/registers.h   |  178 ++++
>  sound/soc/intel/catpt/trace.h       |   83 ++
>  19 files changed, 4377 insertions(+), 143 deletions(-)
>  create mode 100644 sound/soc/intel/catpt/Makefile
>  create mode 100644 sound/soc/intel/catpt/core.h
>  create mode 100644 sound/soc/intel/catpt/device.c
>  create mode 100644 sound/soc/intel/catpt/dsp.c
>  create mode 100644 sound/soc/intel/catpt/fs.c
>  create mode 100644 sound/soc/intel/catpt/ipc.c
>  create mode 100644 sound/soc/intel/catpt/loader.c
>  create mode 100644 sound/soc/intel/catpt/messages.c
>  create mode 100644 sound/soc/intel/catpt/messages.h
>  create mode 100644 sound/soc/intel/catpt/pcm.c
>  create mode 100644 sound/soc/intel/catpt/registers.h
>  create mode 100644 sound/soc/intel/catpt/trace.h
> 
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko