> +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@xxxxxxxxx>
> + * Amadeusz Slawinski <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> + */
> +
> +#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 */
