+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 DSPcapabilities. This includes Skylake, Kabylake, Amberlake andApollolake. This option is mutually exclusive with SKYLAKEdriver.
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 */