Re: [PATCH 03/17] ASoC: Intel: Introduce AVS driver

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-02-25 12:55 AM, Pierre-Louis Bossart wrote:
>> +config SND_SOC_INTEL_AVS
>> +	tristate "Intel AVS driver"
>> +	depends on PCI && ACPI
>> +	depends on COMMON_CLK
>> +	depends on SND_SOC_INTEL_SKYLAKE_FAMILY=n
>> +	default n
> 
> default is already n


Ack.

>> +	select SND_SOC_ACPI
>> +	select SND_HDA_EXT_CORE
>> +	help
>> +	  Enable support for Intel(R) cAVS 1.5 platforms with DSP
>> +	  capabilities. This includes Skylake, Kabylake, Amberlake and
>> +	  Apollolake. This option is mutually exclusive with SKYLAKE
>> +	  driver.
> 
> The feedback from the RFC was that this is not desirable if you want
> anyone to use this driver. The suggested solution was to use the
> intel_dspcfg layer with e.g. dsp_driver=4 for avs. That would allow
> distributions to build this solution for early adopters.


Indeed, this description needs an update!

>> +/* Platform specific descriptor */
>> +struct avs_spec {
>> +	const char *name;
>> +
>> +	const struct avs_dsp_ops *const dops;
> 
> dsp_ops would be clearer. 'd' could refer to just about anything.


Ack.

>> +	const u32 core_init_mask;	/* used during DSP boot */
>> +	const u64 attributes;		/* bitmask of AVS_PLATATTR_* */
>> +};
>> +
>> +struct avs_dev {
>> +	struct hda_bus base;
>> +	struct device *dev;
> 
> question: could you directly embed a struct device instead of a pointer,
> that would simplify the conversion through dev_get_drvdata below.
> 
> Unless this *dev is related to the PCI device, in which case you could
> add a comment.


Pointer 'dev' translates to PCI device. No problem adding a comment.

>> +
>> +	void __iomem *adsp_ba;
> 
> I would guess 'ba' is base address? this could be added with comments or
> kernel-doc


Ack.

>> +	const struct avs_spec *spec;
>> +};
>> +
>> +/* from hda_bus to avs_dev */
>> +#define hda_to_avs(hda) container_of(hda, struct avs_dev, base)
>> +/* from hdac_bus to avs_dev */
>> +#define hdac_to_avs(hdac) hda_to_avs(to_hda_bus(hdac))
>> +/* from device to avs_dev */
>> +#define to_avs_dev(dev) \
>> +({ \
>> +	struct hdac_bus *__bus = dev_get_drvdata(dev); \
>> +	hdac_to_avs(__bus); \
>> +})
>> +
>> +int avs_dsp_core_power(struct avs_dev *adev, u32 core_mask, bool active);
> 
> does this mean 'active' affects all bits in the core_mask? that doesn't
> seem very intuitive.


Can reword to 'power' to match its siblings.

>> +int avs_dsp_core_reset(struct avs_dev *adev, u32 core_mask, bool reset);
>> +int avs_dsp_core_stall(struct avs_dev *adev, u32 core_mask, bool stall);
>> +int avs_dsp_core_enable(struct avs_dev *adev, u32 core_mask);
>> +int avs_dsp_core_disable(struct avs_dev *adev, u32 core_mask);
> 
> it's a bit inconsistent to have enable/disable but a boolean for other
> functions?


While there may be a grain of inconsistency, the order of operations 
does not change for power/reset/stall regardless if the operation is 
"positive" or "negative". In _enable() and _disable() case, the order 
does matter with _disable() being the reverse of its counterpart. As 
functions calling either of these already know which one to use, we can 
avoid an if-statement by providing two separate handlers.

>> +#include <linux/module.h>
>> +#include <sound/hdaudio_ext.h>
>> +#include "avs.h"
>> +#include "registers.h"
> 
> consider renaming as avs_registers.h?


This header is for internal use only and is found within directory 
already named 'avs'. "avs.h" header covers a wider range of types and 
that's why its name is generic. All others are specific and thus are not 
prefixed with "avs_".

>> +
>> +#define AVS_ADSPCS_INTERVAL_US		500
>> +#define AVS_ADSPCS_TIMEOUT_US		10000
> 
> these values don't match with anything that was previously used for
> Intel platforms, where the values could be different depending on
> generations.


Im most cases I'm relying on closed-source equivalents.

> bxt-sst.c:#define BXT_BASEFW_TIMEOUT    3000
> bxt-sst.c:#define BXT_ROM_INIT_TIMEOUT  70
> cnl-sst.c:#define CNL_INIT_TIMEOUT      300
> cnl-sst.c:#define CNL_BASEFW_TIMEOUT    3000
> skl-sst-cldma.h:#define SKL_WAIT_TIMEOUT                500     /* 500
> msec */
> skl-sst-dsp.h:#define BXT_INIT_TIMEOUT          300
> skl-sst-ipc.c:#define IPC_TIMEOUT_MSECS         3000
> skl-sst.c:#define SKL_BASEFW_TIMEOUT    300
> skl-sst.c:#define SKL_INIT_TIMEOUT      1000
> 
> please add a comment on how they were determined or align on hardware
> recommendations.


I'm unsure wheather the above is actually correct : )

e.g.: IPC timeout in skylake-drvier are inflated due to incorrect IPC 
protocol handling. Reply sent by firmware may be delayed due to 
unsoliciated notifications and the aforementioned driver did not take 
that into account extending IPC timeout to avoid the problems.

>> +int avs_dsp_core_power(struct avs_dev *adev, u32 core_mask, bool active)
>> +{
>> +	u32 value, mask, reg;
>> +	int ret;
>> +
>> +	mask = AVS_ADSPCS_SPA_MASK(core_mask);
>> +	value = active ? mask : 0;
>> +
>> +	snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPCS, mask, value);
>> +
>> +	mask = AVS_ADSPCS_CPA_MASK(core_mask);
>> +	value = active ? mask : 0;
>> +
>> +	ret = snd_hdac_adsp_readl_poll(adev, AVS_ADSP_REG_ADSPCS,
>> +				       reg, (reg & mask) == value,
>> +				       AVS_ADSPCS_INTERVAL_US,
>> +				       AVS_ADSPCS_TIMEOUT_US);
>> +	if (ret)
>> +		dev_err(adev->dev, "core_mask %d %spower failed: %d\n",
>> +			core_mask, active ? "" : "un", ret);
> 
> unpower is an odd wording.


Ack.

>> +
>> +	return ret;
>> +}
>> +
>> +int avs_dsp_core_reset(struct avs_dev *adev, u32 core_mask, bool reset)
>> +{
>> +	u32 value, mask, reg;
>> +	int ret;
>> +
>> +	mask = AVS_ADSPCS_CRST_MASK(core_mask);
>> +	value = reset ? mask : 0;
>> +
>> +	snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPCS, mask, value);
>> +
>> +	ret = snd_hdac_adsp_readl_poll(adev, AVS_ADSP_REG_ADSPCS,
>> +				       reg, (reg & mask) == value,
>> +				       AVS_ADSPCS_INTERVAL_US,
>> +				       AVS_ADSPCS_TIMEOUT_US);
>> +	if (ret)
>> +		dev_err(adev->dev, "core_mask %d %sreset failed: %d\n",
>> +			core_mask, reset ? "" : "un", ret);
> 
> unreset is even more odd. enter reset or exit reset.


Ack.

>> +
>> +	return ret;
>> +}
>> +
>> +int avs_dsp_core_stall(struct avs_dev *adev, u32 core_mask, bool stall)
>> +{
>> +	u32 value, mask, reg;
>> +	int ret;
>> +
>> +	mask = AVS_ADSPCS_CSTALL_MASK(core_mask);
>> +	value = stall ? mask : 0;
>> +
>> +	snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPCS, mask, value);
>> +
>> +	ret = snd_hdac_adsp_readl_poll(adev, AVS_ADSP_REG_ADSPCS,
>> +				       reg, (reg & mask) == value,
>> +				       AVS_ADSPCS_INTERVAL_US,
>> +				       AVS_ADSPCS_TIMEOUT_US);
>> +	if (ret)
>> +		dev_err(adev->dev, "core_mask %d %sstall failed: %d\n",
>> +			core_mask, stall ? "" : "un", ret);
> 
> that was probably a copy/paste of stall/unstall in the two cases
> above...this one works, the two above not so much.


Yeah, that's a result of copy/paste. Agree, the wording can be improved.

>> +
>> +	return ret;
>> +}
>> +
>> +int avs_dsp_core_enable(struct avs_dev *adev, u32 core_mask)
>> +{
>> +	int ret;
>> +
>> +	ret = avs_dsp_op(adev, power, core_mask, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = avs_dsp_op(adev, reset, core_mask, false);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return avs_dsp_op(adev, stall, core_mask, false);
>> +}
>> +
>> +int avs_dsp_core_disable(struct avs_dev *adev, u32 core_mask)
>> +{
>> +	/* Be permissive to allow for full DSP shutdown in disable path. */
> 
> that comment isn't very clear, what is permissive here?


There is no error checking below.

>> +	avs_dsp_op(adev, stall, core_mask, true);
>> +	avs_dsp_op(adev, reset, core_mask, true);
>> +
>> +	return avs_dsp_op(adev, power, core_mask, false);
>> +}
>> +
>> +MODULE_LICENSE("GPL v2");
> 
> "GPL"


Ack.