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