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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> +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

> +	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.


> +/* 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.

> +	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.

> +
> +	void __iomem *adsp_ba;

I would guess 'ba' is base address? this could be added with comments or
kernel-doc

> +	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.

> +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?


> +#include <linux/module.h>
> +#include <sound/hdaudio_ext.h>
> +#include "avs.h"
> +#include "registers.h"

consider renaming as avs_registers.h?

> +
> +#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.

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.

> +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.

> +
> +	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.

> +
> +	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.

> +
> +	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?

> +	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"

> diff --git a/sound/soc/intel/avs/registers.h b/sound/soc/intel/avs/registers.h
> new file mode 100644
> index 000000000000..e0b6c8ffe633
> --- /dev/null
> +++ b/sound/soc/intel/avs/registers.h

avs_registers.h?

> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright(c) 2021 Intel Corporation. All rights reserved.
> + *
> + * Authors: Cezary Rojewski <cezary.rojewski@intel.com>
> + *          Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>
> + */
> +
> +#ifndef __SOUND_SOC_INTEL_AVS_REGS_H
> +#define __SOUND_SOC_INTEL_AVS_REGS_H
> +
> +/* Intel HD Audio General DSP Registers */
> +#define AVS_ADSP_GEN_BASE		0x0
> +#define AVS_ADSP_REG_ADSPCS		(AVS_ADSP_GEN_BASE + 0x04)
> +
> +#define AVS_ADSPCS_CRST_MASK(cm)	(cm)
> +#define AVS_ADSPCS_CSTALL_MASK(cm)	((cm) << 8)
> +#define AVS_ADSPCS_SPA_MASK(cm)		((cm) << 16)
> +#define AVS_ADSPCS_CPA_MASK(cm)		((cm) << 24)
> +#define AVS_MAIN_CORE_MASK		BIT(0)
> +
> +#endif /* __SOUND_SOC_INTEL_AVS_REGS_H */