[alsa-devel] [PATCH] ASoC: wm_adsp: Add basic debugfs entries
This patch adds some debugfs nodes to get information about the currently running firmware.
Signed-off-by: Richard Fitzgerald rf@opensource.wolfsonmicro.com --- sound/soc/codecs/wm5102.c | 4 + sound/soc/codecs/wm5110.c | 9 ++- sound/soc/codecs/wm_adsp.c | 182 ++++++++++++++++++++++++++++++++++++++++++-- sound/soc/codecs/wm_adsp.h | 25 ++++++- 4 files changed, 212 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c index 11eba0e..22fc9e9 100644 --- a/sound/soc/codecs/wm5102.c +++ b/sound/soc/codecs/wm5102.c @@ -1875,6 +1875,8 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec) struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec); int ret;
+ wm_adsp_init_debugfs(&priv->core.adsp[0], codec); + ret = snd_soc_add_codec_controls(codec, wm_adsp2_fw_controls, 2); if (ret != 0) return ret; @@ -1893,6 +1895,8 @@ static int wm5102_codec_remove(struct snd_soc_codec *codec) { struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec);
+ wm_adsp_cleanup_debugfs(&priv->core.adsp[0]); + priv->core.arizona->dapm = NULL;
return 0; diff --git a/sound/soc/codecs/wm5110.c b/sound/soc/codecs/wm5110.c index d65364e..f9ca1a9 100644 --- a/sound/soc/codecs/wm5110.c +++ b/sound/soc/codecs/wm5110.c @@ -1599,7 +1599,10 @@ static struct snd_soc_dai_driver wm5110_dai[] = { static int wm5110_codec_probe(struct snd_soc_codec *codec) { struct wm5110_priv *priv = snd_soc_codec_get_drvdata(codec); - int ret; + int i, ret; + + for (i = 0; i < WM5110_NUM_ADSP; i++) + wm_adsp_init_debugfs(&priv->core.adsp[i], codec);
priv->core.arizona->dapm = &codec->dapm;
@@ -1621,6 +1624,10 @@ static int wm5110_codec_probe(struct snd_soc_codec *codec) static int wm5110_codec_remove(struct snd_soc_codec *codec) { struct wm5110_priv *priv = snd_soc_codec_get_drvdata(codec); + int i; + + for (i = 0; i < WM5110_NUM_ADSP; i++) + wm_adsp_cleanup_debugfs(&priv->core.adsp[i]);
priv->core.arizona->dapm = NULL;
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 6e358b5..c8def3ea 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -23,6 +23,7 @@ #include <linux/slab.h> #include <linux/vmalloc.h> #include <linux/workqueue.h> +#include <linux/debugfs.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -243,6 +244,26 @@ struct wm_coeff_ctl { unsigned int flags; };
+#ifdef CONFIG_DEBUG_FS +static void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, const char *s); +static void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, const char *s); +static void wm_adsp_debugfs_clear(struct wm_adsp *dsp); +#else +static inline void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, + const char *s) +{ +} + +static inline void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, + const char *s) +{ +} + +static inline void wm_adsp_debugfs_clear(struct wm_adsp *dsp) +{ +} +#endif + static int wm_adsp_fw_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { @@ -1104,6 +1125,8 @@ static int wm_adsp_load(struct wm_adsp *dsp) adsp_warn(dsp, "%s.%d: %zu bytes at end of file\n", file, regions, pos - firmware->size);
+ wm_adsp_debugfs_save_wmfwname(dsp, file); + out_fw: regmap_async_complete(regmap); wm_adsp_buf_free(&buf_list); @@ -1218,11 +1241,12 @@ static int wm_adsp1_setup_algs(struct wm_adsp *dsp)
n_algs = be32_to_cpu(adsp1_id.n_algs); dsp->fw_id = be32_to_cpu(adsp1_id.fw.id); + dsp->fw_id_version = be32_to_cpu(adsp1_id.fw.ver); adsp_info(dsp, "Firmware: %x v%d.%d.%d, %zu algorithms\n", dsp->fw_id, - (be32_to_cpu(adsp1_id.fw.ver) & 0xff0000) >> 16, - (be32_to_cpu(adsp1_id.fw.ver) & 0xff00) >> 8, - be32_to_cpu(adsp1_id.fw.ver) & 0xff, + (dsp->fw_id_version & 0xff0000) >> 16, + (dsp->fw_id_version & 0xff00) >> 8, + dsp->fw_id_version & 0xff, n_algs);
alg_region = wm_adsp_create_region(dsp, WMFW_ADSP1_ZM, @@ -1321,11 +1345,12 @@ static int wm_adsp2_setup_algs(struct wm_adsp *dsp)
n_algs = be32_to_cpu(adsp2_id.n_algs); dsp->fw_id = be32_to_cpu(adsp2_id.fw.id); + dsp->fw_id_version = be32_to_cpu(adsp2_id.fw.ver); adsp_info(dsp, "Firmware: %x v%d.%d.%d, %zu algorithms\n", dsp->fw_id, - (be32_to_cpu(adsp2_id.fw.ver) & 0xff0000) >> 16, - (be32_to_cpu(adsp2_id.fw.ver) & 0xff00) >> 8, - be32_to_cpu(adsp2_id.fw.ver) & 0xff, + (dsp->fw_id_version & 0xff0000) >> 16, + (dsp->fw_id_version & 0xff00) >> 8, + dsp->fw_id_version & 0xff, n_algs);
alg_region = wm_adsp_create_region(dsp, WMFW_ADSP2_XM, @@ -1601,6 +1626,8 @@ static int wm_adsp_load_coeff(struct wm_adsp *dsp) adsp_warn(dsp, "%s.%d: %zu bytes at end of file\n", file, blocks, pos - firmware->size);
+ wm_adsp_debugfs_save_binname(dsp, file); + out_fw: regmap_async_complete(regmap); release_firmware(firmware); @@ -1869,6 +1896,10 @@ int wm_adsp2_event(struct snd_soc_dapm_widget *w, break;
case SND_SOC_DAPM_PRE_PMD: + wm_adsp_debugfs_clear(dsp); + + dsp->fw_id = 0; + dsp->fw_id_version = 0; dsp->running = false;
regmap_update_bits(dsp->regmap, dsp->base + ADSP2_CONTROL, @@ -1929,4 +1960,143 @@ int wm_adsp2_init(struct wm_adsp *dsp) } EXPORT_SYMBOL_GPL(wm_adsp2_init);
+#ifdef CONFIG_DEBUG_FS +static void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, const char *s) +{ + kfree(dsp->wmfw_file_name); + dsp->wmfw_file_name = kstrdup(s, GFP_KERNEL); +} + +static void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, const char *s) +{ + kfree(dsp->bin_file_name); + dsp->bin_file_name = kstrdup(s, GFP_KERNEL); +} + +static void wm_adsp_debugfs_clear(struct wm_adsp *dsp) +{ + kfree(dsp->wmfw_file_name); + kfree(dsp->bin_file_name); + dsp->wmfw_file_name = NULL; + dsp->bin_file_name = NULL; +} + +static ssize_t wm_adsp_debugfs_string_read(struct wm_adsp *dsp, + char __user *user_buf, + size_t count, loff_t *ppos, + const char *string) +{ + char *temp; + int len; + ssize_t ret; + + if (!string || !dsp->running) + return 0; + + temp = kmalloc(PAGE_SIZE, GFP_KERNEL); + if (!temp) + return -ENOMEM; + + len = snprintf(temp, PAGE_SIZE, "%s\n", string); + ret = simple_read_from_buffer(user_buf, count, ppos, temp, len); + kfree(temp); + return ret; +} + +static ssize_t wm_adsp_debugfs_wmfw_read(struct file *file, + char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct wm_adsp *dsp = file->private_data; + + return wm_adsp_debugfs_string_read(dsp, user_buf, count, ppos, + dsp->wmfw_file_name); +} + +static ssize_t wm_adsp_debugfs_bin_read(struct file *file, + char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct wm_adsp *dsp = file->private_data; + + return wm_adsp_debugfs_string_read(dsp, user_buf, count, ppos, + dsp->bin_file_name); +} + +static const struct { + const char *name; + const struct file_operations fops; +} wm_adsp_debugfs_fops[] = { + { + .name = "wmfw_file", + .fops = { + .open = simple_open, + .read = wm_adsp_debugfs_wmfw_read, + }, + }, + { + .name = "bin_file", + .fops = { + .open = simple_open, + .read = wm_adsp_debugfs_bin_read, + }, + }, +}; + +void wm_adsp_init_debugfs(struct wm_adsp *dsp, struct snd_soc_codec *codec) +{ + struct dentry *root = NULL; + char *root_name; + int i; + + if (!codec->component.debugfs_root) { + adsp_err(dsp, "No codec debugfs root\n"); + goto err; + } + + root_name = kmalloc(PAGE_SIZE, GFP_KERNEL); + if (!root_name) + goto err; + + snprintf(root_name, PAGE_SIZE, "dsp%d", dsp->num); + root = debugfs_create_dir(root_name, codec->component.debugfs_root); + kfree(root_name); + + if (!root) + goto err; + + if (!debugfs_create_bool("running", S_IRUGO, root, &dsp->running)) + goto err; + + if (!debugfs_create_x32("fw_id", S_IRUGO, root, &dsp->fw_id)) + goto err; + + if (!debugfs_create_x32("fw_version", S_IRUGO, root, + &dsp->fw_id_version)) + goto err; + + for (i = 0; i < ARRAY_SIZE(wm_adsp_debugfs_fops); ++i) { + if (!debugfs_create_file(wm_adsp_debugfs_fops[i].name, + S_IRUGO, root, dsp, + &wm_adsp_debugfs_fops[i].fops)) + goto err; + } + + dsp->debugfs_root = root; + return; + +err: + debugfs_remove_recursive(root); + adsp_err(dsp, "Failed to create debugfs\n"); +} +EXPORT_SYMBOL_GPL(wm_adsp_init_debugfs); + + +void wm_adsp_cleanup_debugfs(struct wm_adsp *dsp) +{ + debugfs_remove_recursive(dsp->debugfs_root); +} +EXPORT_SYMBOL_GPL(wm_adsp_cleanup_debugfs); +#endif + MODULE_LICENSE("GPL v2"); diff --git a/sound/soc/codecs/wm_adsp.h b/sound/soc/codecs/wm_adsp.h index 0e5f07c..1d563fe 100644 --- a/sound/soc/codecs/wm_adsp.h +++ b/sound/soc/codecs/wm_adsp.h @@ -46,17 +46,25 @@ struct wm_adsp { struct list_head alg_regions;
int fw_id; + int fw_id_version;
const struct wm_adsp_region *mem; int num_mems;
int fw; int fw_ver; - bool running; + u32 running;
struct list_head ctl_list;
struct work_struct boot_work; + +#ifdef CONFIG_DEBUG_FS + struct dentry *debugfs_root; + char *wmfw_file_name; + char *bin_file_name; +#endif + };
#define WM_ADSP1(wname, num) \ @@ -79,6 +87,21 @@ extern const struct snd_kcontrol_new wm_adsp2_fw_controls[];
int wm_adsp1_init(struct wm_adsp *dsp); int wm_adsp2_init(struct wm_adsp *dsp); + +#ifdef CONFIG_DEBUG_FS +void wm_adsp_init_debugfs(struct wm_adsp *dsp, struct snd_soc_codec *codec); +void wm_adsp_cleanup_debugfs(struct wm_adsp *dsp); +#else +static inline void wm_adsp_init_debugfs(struct wm_adsp *dsp, + struct snd_soc_codec *codec) +{ +} + +void wm_adsp_cleanup_debugfs(struct wm_adsp *dsp) +{ +} +#endif + int wm_adsp1_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event); int wm_adsp2_early_event(struct snd_soc_dapm_widget *w,
On Mon, Jun 08, 2015 at 03:37:02PM +0100, Richard Fitzgerald wrote:
+++ b/sound/soc/codecs/wm5102.c @@ -1875,6 +1875,8 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec) struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec); int ret;
- wm_adsp_init_debugfs(&priv->core.adsp[0], codec);
Why are we adding this init to every individual CODEC rather than doing it when we initialize the DSP (which there are calls for already)?
+#ifdef CONFIG_DEBUG_FS +static void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, const char *s); +static void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, const char *s); +static void wm_adsp_debugfs_clear(struct wm_adsp *dsp); +#else +static inline void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp,
const char *s)+{ +}
+static inline void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp,
const char *s)+{ +}
+static inline void wm_adsp_debugfs_clear(struct wm_adsp *dsp) +{ +} +#endif
Why not just put the functions here rather than prototypes?
+static ssize_t wm_adsp_debugfs_string_read(struct wm_adsp *dsp,
char __user *user_buf,size_t count, loff_t *ppos,const char *string)+{
- char *temp;
- int len;
- ssize_t ret;
- if (!string || !dsp->running)
return 0;
Does debugfs ensure that the right thing happens and this gets treated as EOF rather than a "zero length read, please retry" (which something might decide to busy wait trying)? I'd have expected either an error or substituting in an empty/informative string here.
- temp = kmalloc(PAGE_SIZE, GFP_KERNEL);
- if (!temp)
return -ENOMEM;- len = snprintf(temp, PAGE_SIZE, "%s\n", string);
Given that we already have the string I don't understand why we're allocating the temporary buffer - if it's just the length we're looking for then strlen() should be enough?
+} wm_adsp_debugfs_fops[] = {
- {
.name = "wmfw_file",
.name = "bin_file",
Bikeshedding but _name not _file perhaps? It's not going to give you a copy of the firmware/coefficients.
On Mon, Jun 08, 2015 at 06:40:41PM +0100, Mark Brown wrote:
On Mon, Jun 08, 2015 at 03:37:02PM +0100, Richard Fitzgerald wrote:
+++ b/sound/soc/codecs/wm5102.c @@ -1875,6 +1875,8 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec) struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec); int ret;
- wm_adsp_init_debugfs(&priv->core.adsp[0], codec);
Why are we adding this init to every individual CODEC rather than doing it when we initialize the DSP (which there are calls for already)?
Because we call the existing wm_adsp_init() early in probe and at that point we haven't registered the codec yet.
+#ifdef CONFIG_DEBUG_FS +static void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, const char *s); +static void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, const char *s); +static void wm_adsp_debugfs_clear(struct wm_adsp *dsp); +#else +static inline void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp,
const char *s)+{ +}
+static inline void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp,
const char *s)+{ +}
+static inline void wm_adsp_debugfs_clear(struct wm_adsp *dsp) +{ +} +#endif
Why not just put the functions here rather than prototypes?
It was just personal preference, I like to have the important code higher up in source files and keep the clutter of debug code near the end where it's not in the way but I can turn it around.
+static ssize_t wm_adsp_debugfs_string_read(struct wm_adsp *dsp,
char __user *user_buf,size_t count, loff_t *ppos,const char *string)+{
- char *temp;
- int len;
- ssize_t ret;
- if (!string || !dsp->running)
return 0;Does debugfs ensure that the right thing happens and this gets treated as EOF rather than a "zero length read, please retry" (which something might decide to busy wait trying)? I'd have expected either an error or substituting in an empty/informative string here.
- temp = kmalloc(PAGE_SIZE, GFP_KERNEL);
- if (!temp)
return -ENOMEM;- len = snprintf(temp, PAGE_SIZE, "%s\n", string);
Given that we already have the string I don't understand why we're allocating the temporary buffer - if it's just the length we're looking for then strlen() should be enough?
+} wm_adsp_debugfs_fops[] = {
- {
.name = "wmfw_file",
.name = "bin_file",Bikeshedding but _name not _file perhaps? It's not going to give you a copy of the firmware/coefficients.
On Tue, Jun 09, 2015 at 01:06:41PM +0100, Richard Fitzgerald wrote:
On Mon, Jun 08, 2015 at 06:40:41PM +0100, Mark Brown wrote:
Why are we adding this init to every individual CODEC rather than doing it when we initialize the DSP (which there are calls for already)?
Because we call the existing wm_adsp_init() early in probe and at that point we haven't registered the codec yet.
Can you not either hang it off the device or move the registration later?
On Mon, Jun 08, 2015 at 06:40:41PM +0100, Mark Brown wrote:
On Mon, Jun 08, 2015 at 03:37:02PM +0100, Richard Fitzgerald wrote:
+++ b/sound/soc/codecs/wm5102.c @@ -1875,6 +1875,8 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec) struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec); int ret;
- wm_adsp_init_debugfs(&priv->core.adsp[0], codec);
Why are we adding this init to every individual CODEC rather than doing it when we initialize the DSP (which there are calls for already)?
+#ifdef CONFIG_DEBUG_FS +static void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp, const char *s); +static void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp, const char *s); +static void wm_adsp_debugfs_clear(struct wm_adsp *dsp); +#else +static inline void wm_adsp_debugfs_save_wmfwname(struct wm_adsp *dsp,
const char *s)+{ +}
+static inline void wm_adsp_debugfs_save_binname(struct wm_adsp *dsp,
const char *s)+{ +}
+static inline void wm_adsp_debugfs_clear(struct wm_adsp *dsp) +{ +} +#endif
Why not just put the functions here rather than prototypes?
+static ssize_t wm_adsp_debugfs_string_read(struct wm_adsp *dsp,
char __user *user_buf,size_t count, loff_t *ppos,const char *string)+{
- char *temp;
- int len;
- ssize_t ret;
- if (!string || !dsp->running)
return 0;Does debugfs ensure that the right thing happens and this gets treated as EOF rather than a "zero length read, please retry" (which something might decide to busy wait trying)? I'd have expected either an error or substituting in an empty/informative string here.
If simple_read_from_buffer() is off the end of the buffer or count is 0 it returns 0 not EOF. Also I checked procfs for cases where things aren't always valid and it returns 0. So this seems to be accepted behaviour.
- temp = kmalloc(PAGE_SIZE, GFP_KERNEL);
- if (!temp)
return -ENOMEM;- len = snprintf(temp, PAGE_SIZE, "%s\n", string);
Given that we already have the string I don't understand why we're allocating the temporary buffer - if it's just the length we're looking for then strlen() should be enough?
+} wm_adsp_debugfs_fops[] = {
- {
.name = "wmfw_file",
.name = "bin_file",Bikeshedding but _name not _file perhaps? It's not going to give you a copy of the firmware/coefficients.
participants (2)
-
Mark Brown -
Richard Fitzgerald