[Sound-open-firmware] [PATCH 1/5] dai: add lbm status for dai ssp
From: Pan Xiuli xiuli.pan@linux.intel.com
Add a status to track dai ssp lbm.
Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com --- Work with patch set: SOF-Kernel(6): ASoC: SOF: Add debug_mode flag in sof dev ASoC: SOF: debug: add debugmode debugfs for sof_dev debug_mode ASoC: SOF: uapi: topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode ASoC: SOF: add headers for lbm control callback functions ASoC: SOF: add lbm kcontrol callback functions ASoC: SOF: topology: Add topology handler for dai ssp loopback mode
SOF(5): dai: add lbm status for dai ssp dai: add get_loopback_mode function DMIC: add empty get_loopback_mode function SSP: support for get/set_loopback_mode functions dai: add dai_cmd support for loopback mode switch
SOF-Tools(3): topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode topology: m4: Add DAI_OUT_SSP_LOOPBACK marco for SSP LOOPBACK dai comp topology: test: Add loopback topology
test & santity test with: Mininow max rt5651 and UP2 Hifiberry PRO and CNL nocodec SOF master: 48d2a1c551d7b3c8f76d44f3c04dd59a37ff6a8f SOF-Tool master: bd7dc88231f31d385340310cef467f211a739eeb https://github.com/plbossart/sound/tree/topic/sof-v4.14: 0d51a5ed28c5e97f09b59c4cafaddfb9d3b24b77 --- src/include/sof/ssp.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/include/sof/ssp.h b/src/include/sof/ssp.h index fdcfcac..92db71f 100644 --- a/src/include/sof/ssp.h +++ b/src/include/sof/ssp.h @@ -231,6 +231,7 @@ struct ssp_pdata { uint32_t psp; spinlock_t lock; uint32_t state[2]; /* SSP_STATE_ for each direction */ + uint32_t lbm; completion_t drain_complete; struct sof_ipc_dai_config config; struct sof_ipc_dai_ssp_params params;
From: Pan Xiuli xiuli.pan@linux.intel.com
get loopback mode status for dai.
Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com --- Work with patch set: SOF-Kernel(6): ASoC: SOF: Add debug_mode flag in sof dev ASoC: SOF: debug: add debugmode debugfs for sof_dev debug_mode ASoC: SOF: uapi: topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode ASoC: SOF: add headers for lbm control callback functions ASoC: SOF: add lbm kcontrol callback functions ASoC: SOF: topology: Add topology handler for dai ssp loopback mode
SOF(5): dai: add lbm status for dai ssp dai: add get_loopback_mode function DMIC: add empty get_loopback_mode function SSP: support for get/set_loopback_mode functions dai: add dai_cmd support for loopback mode switch
SOF-Tools(3): topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode topology: m4: Add DAI_OUT_SSP_LOOPBACK marco for SSP LOOPBACK dai comp topology: test: Add loopback topology
test & santity test with: Mininow max rt5651 and UP2 Hifiberry PRO and CNL nocodec SOF master: 48d2a1c551d7b3c8f76d44f3c04dd59a37ff6a8f SOF-Tool master: bd7dc88231f31d385340310cef467f211a739eeb https://github.com/plbossart/sound/tree/topic/sof-v4.14: 0d51a5ed28c5e97f09b59c4cafaddfb9d3b24b77 --- src/include/sof/dai.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/include/sof/dai.h b/src/include/sof/dai.h index e3bce1d..8cc579c 100644 --- a/src/include/sof/dai.h +++ b/src/include/sof/dai.h @@ -64,6 +64,7 @@ struct dai_ops { int (*pm_context_store)(struct dai *dai); int (*probe)(struct dai *dai); int (*set_loopback_mode)(struct dai *dai, uint32_t lbm); + int (*get_loopback_mode)(struct dai *dai); };
/* DAI slot map to audio channel */ @@ -129,6 +130,11 @@ static inline int dai_set_loopback_mode(struct dai *dai, uint32_t lbm) return dai->ops->set_loopback_mode(dai, lbm); }
+static inline int dai_get_loopback_mode(struct dai *dai) +{ + return dai->ops->get_loopback_mode(dai); +} + /* Digital Audio interface trigger */ static inline int dai_trigger(struct dai *dai, int cmd, int direction) {
From: Pan Xiuli xiuli.pan@linux.intel.com
Return error value if it is called.
Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com --- Work with patch set: SOF-Kernel(6): ASoC: SOF: Add debug_mode flag in sof dev ASoC: SOF: debug: add debugmode debugfs for sof_dev debug_mode ASoC: SOF: uapi: topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode ASoC: SOF: add headers for lbm control callback functions ASoC: SOF: add lbm kcontrol callback functions ASoC: SOF: topology: Add topology handler for dai ssp loopback mode
SOF(5): dai: add lbm status for dai ssp dai: add get_loopback_mode function DMIC: add empty get_loopback_mode function SSP: support for get/set_loopback_mode functions dai: add dai_cmd support for loopback mode switch
SOF-Tools(3): topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode topology: m4: Add DAI_OUT_SSP_LOOPBACK marco for SSP LOOPBACK dai comp topology: test: Add loopback topology
test & santity test with: Mininow max rt5651 and UP2 Hifiberry PRO and CNL nocodec SOF master: 48d2a1c551d7b3c8f76d44f3c04dd59a37ff6a8f SOF-Tool master: bd7dc88231f31d385340310cef467f211a739eeb https://github.com/plbossart/sound/tree/topic/sof-v4.14: 0d51a5ed28c5e97f09b59c4cafaddfb9d3b24b77 --- src/drivers/dmic.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/drivers/dmic.c b/src/drivers/dmic.c index 97ca8b2..14dc0f5 100644 --- a/src/drivers/dmic.c +++ b/src/drivers/dmic.c @@ -1353,6 +1353,11 @@ static inline int dmic_set_loopback_mode(struct dai *dai, uint32_t lbm) return -EINVAL; }
+static inline int dmic_get_loopback_mode(struct dai *dai) +{ + return -EINVAL; +} + const struct dai_ops dmic_ops = { .trigger = dmic_trigger, .set_config = dmic_set_config, @@ -1360,6 +1365,7 @@ const struct dai_ops dmic_ops = { .pm_context_restore = dmic_context_restore, .probe = dmic_probe, .set_loopback_mode = dmic_set_loopback_mode, + .get_loopback_mode = dmic_get_loopback_mode, };
#endif
On Tue, 2018-06-19 at 17:22 +0800, Xiuli Pan wrote:
+static inline int dmic_get_loopback_mode(struct dai *dai) +{
return -EINVAL;+}
const struct dai_ops dmic_ops = { .trigger = dmic_trigger, .set_config = dmic_set_config, @@ -1360,6 +1365,7 @@ const struct dai_ops dmic_ops = { .pm_context_restore = dmic_context_restore, .probe = dmic_probe, .set_loopback_mode = dmic_set_loopback_mode,
.get_loopback_mode = dmic_get_loopback_mode,
Lest get rid of set/get loopback mode and use the standard component cmd APIs, I'm sure I've said this in an earlier version of this patch.
Liam
};
On Tue, 2018-06-19 at 12:03 +0100, Liam Girdwood wrote:
On Tue, 2018-06-19 at 17:22 +0800, Xiuli Pan wrote:
+static inline int dmic_get_loopback_mode(struct dai *dai) +{
return -EINVAL;+}
const struct dai_ops dmic_ops = { .trigger = dmic_trigger, .set_config = dmic_set_config, @@ -1360,6 +1365,7 @@ const struct dai_ops dmic_ops = { .pm_context_restore = dmic_context_restore, .probe = dmic_probe, .set_loopback_mode = dmic_set_loopback_mode,
.get_loopback_mode = dmic_get_loopback_mode,Lest get rid of set/get loopback mode and use the standard component cmd APIs, I'm sure I've said this in an earlier version of this patch.
Ok, I've seen the 4/5 now and you are only using this API internally, but can we make it more generic (like ioctl()) where we can use it for other DIA types and options.
Liam
On Tue, 2018-06-19 at 12:03 +0100, Liam Girdwood wrote:
On Tue, 2018-06-19 at 17:22 +0800, Xiuli Pan wrote:
+static inline int dmic_get_loopback_mode(struct dai *dai) +{
return -EINVAL;+}
const struct dai_ops dmic_ops = { .trigger = dmic_trigger, .set_config = dmic_set_config, @@ -1360,6 +1365,7 @@ const struct dai_ops dmic_ops = { .pm_context_restore = dmic_context_restore, .probe = dmic_probe, .set_loopback_mode = dmic_set_loopback_mode,
.get_loopback_mode = dmic_get_loopback_mode,
Do we even need to define the get/set for loopback mode for DMIC if it is not supported?
Lest get rid of set/get loopback mode and use the standard component cmd APIs, I'm sure I've said this in an earlier version of this patch.
Liam
};
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 6/19/2018 19:03, Liam Girdwood wrote:
On Tue, 2018-06-19 at 17:22 +0800, Xiuli Pan wrote:
+static inline int dmic_get_loopback_mode(struct dai *dai) +{
return -EINVAL;+}
- const struct dai_ops dmic_ops = { .trigger = dmic_trigger, .set_config = dmic_set_config,
@@ -1360,6 +1365,7 @@ const struct dai_ops dmic_ops = { .pm_context_restore = dmic_context_restore, .probe = dmic_probe, .set_loopback_mode = dmic_set_loopback_mode,
.get_loopback_mode = dmic_get_loopback_mode,Lest get rid of set/get loopback mode and use the standard component cmd APIs, I'm sure I've said this in an earlier version of this patch.
This is just helper function for COMP cmd APIs We need to handle the DAI. So this wrapper is used for COMP access DAI. The comp CMD APIs is in the 5/5 patch.
Or you suggest we just directly access dai data and change register without a wrapper?
Thanks Xiuli
Liam
};
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
From: Pan Xiuli xiuli.pan@linux.intel.com
Add ssp_get_loopback_mode and refine ssp_set_loopback_mode function to support SSP loopback mode.
Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com --- Work with patch set: SOF-Kernel(6): ASoC: SOF: Add debug_mode flag in sof dev ASoC: SOF: debug: add debugmode debugfs for sof_dev debug_mode ASoC: SOF: uapi: topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode ASoC: SOF: add headers for lbm control callback functions ASoC: SOF: add lbm kcontrol callback functions ASoC: SOF: topology: Add topology handler for dai ssp loopback mode
SOF(5): dai: add lbm status for dai ssp dai: add get_loopback_mode function DMIC: add empty get_loopback_mode function SSP: support for get/set_loopback_mode functions dai: add dai_cmd support for loopback mode switch
SOF-Tools(3): topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode topology: m4: Add DAI_OUT_SSP_LOOPBACK marco for SSP LOOPBACK dai comp topology: test: Add loopback topology
test & santity test with: Mininow max rt5651 and UP2 Hifiberry PRO and CNL nocodec SOF master: 48d2a1c551d7b3c8f76d44f3c04dd59a37ff6a8f SOF-Tool master: bd7dc88231f31d385340310cef467f211a739eeb https://github.com/plbossart/sound/tree/topic/sof-v4.14: 0d51a5ed28c5e97f09b59c4cafaddfb9d3b24b77 --- src/drivers/apl-ssp.c | 22 +++++++++++++++++++--- src/drivers/byt-ssp.c | 22 +++++++++++++++++++--- src/drivers/hsw-ssp.c | 22 +++++++++++++++++++--- 3 files changed, 57 insertions(+), 9 deletions(-)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index 6571b90..e94d676 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -564,16 +564,31 @@ static inline int ssp_set_loopback_mode(struct dai *dai, uint32_t lbm) { struct ssp_pdata *ssp = dai_get_drvdata(dai);
- trace_ssp("loo"); - spin_lock(&ssp->lock); + trace_ssp("los");
- ssp_update_bits(dai, SSCR1, SSCR1_LBM, lbm ? SSCR1_LBM : 0); + if (ssp->lbm == lbm) + return 0;
+ spin_lock(&ssp->lock); + ssp->lbm = lbm; + ssp_update_bits(dai, SSCR1, SSCR1_LBM, lbm ? SSCR1_LBM : 0); spin_unlock(&ssp->lock);
return 0; }
+static inline int ssp_get_loopback_mode(struct dai *dai) +{ + struct ssp_pdata *ssp = dai_get_drvdata(dai); + int ret; + + trace_ssp("log"); + spin_lock(&ssp->lock); + ret = ssp->lbm; + spin_unlock(&ssp->lock); + return ret; +} + /* start the SSP for either playback or capture */ static void ssp_start(struct dai *dai, int direction) { @@ -711,4 +726,5 @@ const struct dai_ops ssp_ops = { .pm_context_restore = ssp_context_restore, .probe = ssp_probe, .set_loopback_mode = ssp_set_loopback_mode, + .get_loopback_mode = ssp_get_loopback_mode, }; diff --git a/src/drivers/byt-ssp.c b/src/drivers/byt-ssp.c index d058c5f..d55dc8b 100644 --- a/src/drivers/byt-ssp.c +++ b/src/drivers/byt-ssp.c @@ -472,16 +472,31 @@ static inline int ssp_set_loopback_mode(struct dai *dai, uint32_t lbm) { struct ssp_pdata *ssp = dai_get_drvdata(dai);
- trace_ssp("loo"); - spin_lock(&ssp->lock); + trace_ssp("los");
- ssp_update_bits(dai, SSCR1, SSCR1_LBM, lbm ? SSCR1_LBM : 0); + if (ssp->lbm == lbm) + return 0;
+ spin_lock(&ssp->lock); + ssp->lbm = lbm; + ssp_update_bits(dai, SSCR1, SSCR1_LBM, lbm ? SSCR1_LBM : 0); spin_unlock(&ssp->lock);
return 0; }
+static inline int ssp_get_loopback_mode(struct dai *dai) +{ + struct ssp_pdata *ssp = dai_get_drvdata(dai); + int ret; + + trace_ssp("log"); + spin_lock(&ssp->lock); + ret = ssp->lbm; + spin_unlock(&ssp->lock); + return ret; +} + /* start the SSP for either playback or capture */ static void ssp_start(struct dai *dai, int direction) { @@ -623,4 +638,5 @@ const struct dai_ops ssp_ops = { .pm_context_restore = ssp_context_restore, .probe = ssp_probe, .set_loopback_mode = ssp_set_loopback_mode, + .get_loopback_mode = ssp_get_loopback_mode, }; diff --git a/src/drivers/hsw-ssp.c b/src/drivers/hsw-ssp.c index 0d74e7d..91af784 100644 --- a/src/drivers/hsw-ssp.c +++ b/src/drivers/hsw-ssp.c @@ -395,16 +395,31 @@ static inline int ssp_set_loopback_mode(struct dai *dai, uint32_t lbm) { struct ssp_pdata *ssp = dai_get_drvdata(dai);
- trace_ssp("loo"); - spin_lock(&ssp->lock); + trace_ssp("los");
- ssp_update_bits(dai, SSCR1, SSCR1_LBM, lbm ? SSCR1_LBM : 0); + if (ssp->lbm == lbm) + return 0;
+ spin_lock(&ssp->lock); + ssp->lbm = lbm; + ssp_update_bits(dai, SSCR1, SSCR1_LBM, lbm ? SSCR1_LBM : 0); spin_unlock(&ssp->lock);
return 0; }
+static inline int ssp_get_loopback_mode(struct dai *dai) +{ + struct ssp_pdata *ssp = dai_get_drvdata(dai); + int ret; + + trace_ssp("log"); + spin_lock(&ssp->lock); + ret = ssp->lbm; + spin_unlock(&ssp->lock); + return ret; +} + /* start the SSP for either playback or capture */ static void ssp_start(struct dai *dai, int direction) { @@ -550,4 +565,5 @@ const struct dai_ops ssp_ops = { .pm_context_restore = ssp_context_restore, .probe = ssp_probe, .set_loopback_mode = ssp_set_loopback_mode, + .get_loopback_mode = ssp_get_loopback_mode, };
From: Pan Xiuli xiuli.pan@linux.intel.com
use dai_cmd to handle ssp dai loopback mode switch support
Signed-off-by: Pan Xiuli xiuli.pan@linux.intel.com --- Work with patch set: SOF-Kernel(6): ASoC: SOF: Add debug_mode flag in sof dev ASoC: SOF: debug: add debugmode debugfs for sof_dev debug_mode ASoC: SOF: uapi: topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode ASoC: SOF: add headers for lbm control callback functions ASoC: SOF: add lbm kcontrol callback functions ASoC: SOF: topology: Add topology handler for dai ssp loopback mode
SOF(5): dai: add lbm status for dai ssp dai: add get_loopback_mode function DMIC: add empty get_loopback_mode function SSP: support for get/set_loopback_mode functions dai: add dai_cmd support for loopback mode switch
SOF-Tools(3): topology: Add SOF_TKN_DAI_SSP_LBM for ssp loopback mode topology: m4: Add DAI_OUT_SSP_LOOPBACK marco for SSP LOOPBACK dai comp topology: test: Add loopback topology
test & santity test with: Mininow max rt5651 and UP2 Hifiberry PRO and CNL nocodec SOF master: 48d2a1c551d7b3c8f76d44f3c04dd59a37ff6a8f SOF-Tool master: bd7dc88231f31d385340310cef467f211a739eeb https://github.com/plbossart/sound/tree/topic/sof-v4.14: 0d51a5ed28c5e97f09b59c4cafaddfb9d3b24b77 --- src/audio/dai.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/src/audio/dai.c b/src/audio/dai.c index 21a3b80..53acbd8 100644 --- a/src/audio/dai.c +++ b/src/audio/dai.c @@ -663,6 +663,81 @@ static int dai_config(struct comp_dev *dev, struct sof_ipc_dai_config *config) return 0; }
+static int dai_ctrl_set_cmd(struct comp_dev *dev, + struct sof_ipc_ctrl_data *cdata) +{ + struct dai_data *dd = comp_get_drvdata(dev); + int ret = 0; + int val; + + /* validate */ + if (cdata->num_elems != 1) { + trace_dai_error("xs0"); + return -EINVAL; + } + + switch (cdata->cmd) { + case SOF_CTRL_CMD_SWITCH: + val = cdata->compv[0].uvalue; + trace_dai("dcs"); + trace_value(cdata->comp_id); + trace_value(val); + dai_set_loopback_mode(dd->dai, val); + break; + + default: + trace_dai_error("gs1"); + return -EINVAL; + } + + return ret; +} + +static int dai_ctrl_get_cmd(struct comp_dev *dev, + struct sof_ipc_ctrl_data *cdata) +{ + struct dai_data *dd = comp_get_drvdata(dev); + int val; + + /* validate */ + if (cdata->num_elems != 1) { + trace_dai_error("xg0"); + return -EINVAL; + } + switch (cdata->cmd) { + case SOF_CTRL_CMD_SWITCH: + val = dai_get_loopback_mode(dd->dai); + trace_dai("dcg"); + trace_value(cdata->comp_id); + trace_value(val); + cdata->compv[0].uvalue = val; + break; + + default: + trace_dai_error("xcc"); + return -EINVAL; + } + + return 0; +} + +/* used to pass standard and bespoke commands (with data) to component */ +static int dai_cmd(struct comp_dev *dev, int cmd, void *data) +{ + struct sof_ipc_ctrl_data *cdata = data; + + trace_dai("cmd"); + + switch (cmd) { + case COMP_CMD_SET_VALUE: + return dai_ctrl_set_cmd(dev, cdata); + case COMP_CMD_GET_VALUE: + return dai_ctrl_get_cmd(dev, cdata); + default: + return -EINVAL; + } +} + static struct comp_driver comp_dai = { .type = SOF_COMP_DAI, .ops = { @@ -675,6 +750,7 @@ static struct comp_driver comp_dai = { .reset = dai_reset, .dai_config = dai_config, .position = dai_position, + .cmd = dai_cmd, }, };
On Tue, 2018-06-19 at 17:22 +0800, Xiuli Pan wrote:
diff --git a/src/include/sof/ssp.h b/src/include/sof/ssp.h index fdcfcac..92db71f 100644 --- a/src/include/sof/ssp.h +++ b/src/include/sof/ssp.h @@ -231,6 +231,7 @@ struct ssp_pdata { uint32_t psp; spinlock_t lock; uint32_t state[2]; /* SSP_STATE_ for each direction */
uint32_t lbm;
This is probably better as a bitfield flag and probably made generic in sof_ipc_dai_config. That way we can reuse this whole infrastructure for other non SSP DAIs.
I would also probably rename to loopback_control;
Liam
completion_t drain_complete; struct sof_ipc_dai_config config; struct sof_ipc_dai_ssp_params params;
On 6/19/2018 19:01, Liam Girdwood wrote:
On Tue, 2018-06-19 at 17:22 +0800, Xiuli Pan wrote:
diff --git a/src/include/sof/ssp.h b/src/include/sof/ssp.h index fdcfcac..92db71f 100644 --- a/src/include/sof/ssp.h +++ b/src/include/sof/ssp.h @@ -231,6 +231,7 @@ struct ssp_pdata { uint32_t psp; spinlock_t lock; uint32_t state[2]; /* SSP_STATE_ for each direction */
uint32_t lbm;This is probably better as a bitfield flag and probably made generic in sof_ipc_dai_config. That way we can reuse this whole infrastructure for other
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
Thanks Xiuli
non SSP DAIs.
I would also probably rename to loopback_control;
Liam
completion_t drain_complete; struct sof_ipc_dai_config config; struct sof_ipc_dai_ssp_params params;
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
but loopback is not just for SSP, other non Intel DAIs also support it.
Liam
On 6/20/18 5:43 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
but loopback is not just for SSP, other non Intel DAIs also support it.
Humm, not sure I agree here with the logic.
Some non-Intel DAIs support LBM. One Intel DAI out of 4 - the SSP - supports LBM
-> so what is the reason for making this field generic? It creates a case where LBM would be configurable for Intel DAIs who don't support it...
On Wed, 2018-06-20 at 11:40 -0500, Pierre-Louis Bossart wrote:
On 6/20/18 5:43 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
but loopback is not just for SSP, other non Intel DAIs also support it.
Humm, not sure I agree here with the logic.
Some non-Intel DAIs support LBM. One Intel DAI out of 4 - the SSP - supports LBM
McASP, McBSP have loopback and probably plenty of others too.
-> so what is the reason for making this field generic? It creates a case where LBM would be configurable for Intel DAIs who don't support it...
Not really, if the LBM flag was set for HDA in topology then it would be rejected by the HDA DAI driver and passed as an error back up the stack.
Liam
On 06/20/2018 02:03 PM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 11:40 -0500, Pierre-Louis Bossart wrote:
On 6/20/18 5:43 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
but loopback is not just for SSP, other non Intel DAIs also support it.
Humm, not sure I agree here with the logic.
Some non-Intel DAIs support LBM. One Intel DAI out of 4 - the SSP - supports LBM
McASP, McBSP have loopback and probably plenty of others too.
ipc_dai_config is mostly a union today:
/* general purpose DAI configuration */ struct sof_ipc_dai_config { struct sof_ipc_hdr hdr; enum sof_ipc_dai_type type; uint32_t id; /* physical number if more than 1 of this type */
/* physical protocol and clocking */ uint16_t format; /* SOF_DAI_FMT_ */ uint16_t reserved; /* alignment */
/* HW specific data */ union { struct sof_ipc_dai_ssp_params ssp; struct sof_ipc_dai_hda_params hda; struct sof_ipc_dai_dmic_params dmic; }; };
we discussed earlier that the format was SSP specific, and if there are additional interfaces they will have to be dealt with an extension of a union. So now I am surprised to see you add a new generic field which isn't really applicable to all DAI types.
-> so what is the reason for making this field generic? It creates a case where LBM would be configurable for Intel DAIs who don't support it...
Not really, if the LBM flag was set for HDA in topology then it would be rejected by the HDA DAI driver and passed as an error back up the stack.
So we need an error check in the driver which parses the topology AND in the firmware?
I am not going to lay on the tracks but I don't see where this is going. I find it simpler to just add this field when it's supported. Not to mention that the addition of such generic parameters is very unlikely moving forward if we have to deal with backwards compatibility.
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 6/21/2018 03:58, Pierre-Louis Bossart wrote:
On 06/20/2018 02:03 PM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 11:40 -0500, Pierre-Louis Bossart wrote:
On 6/20/18 5:43 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
but loopback is not just for SSP, other non Intel DAIs also support it.
Humm, not sure I agree here with the logic.
Some non-Intel DAIs support LBM. One Intel DAI out of 4 - the SSP - supports LBM
McASP, McBSP have loopback and probably plenty of others too.
ipc_dai_config is mostly a union today:
/* general purpose DAI configuration */ struct sof_ipc_dai_config { struct sof_ipc_hdr hdr; enum sof_ipc_dai_type type; uint32_t id; /* physical number if more than 1 of this type */
/* physical protocol and clocking */ uint16_t format; /* SOF_DAI_FMT_ */ uint16_t reserved; /* alignment */
/* HW specific data */ union { struct sof_ipc_dai_ssp_params ssp; struct sof_ipc_dai_hda_params hda; struct sof_ipc_dai_dmic_params dmic; }; };
we discussed earlier that the format was SSP specific, and if there are additional interfaces they will have to be dealt with an extension of a union. So now I am surprised to see you add a new generic field which isn't really applicable to all DAI types.
-> so what is the reason for making this field generic? It creates a case where LBM would be configurable for Intel DAIs who don't support it...
Not really, if the LBM flag was set for HDA in topology then it would be rejected by the HDA DAI driver and passed as an error back up the stack.
So we need an error check in the driver which parses the topology AND in the firmware?
I am not going to lay on the tracks but I don't see where this is going. I find it simpler to just add this field when it's supported. Not to mention that the addition of such generic parameters is very unlikely moving forward if we have to deal with backwards compatibility.
Hi Liam/Pierre
I am quite confused now, let me show the current status and the planning: Current:
SOF: 1.COMP DAI CMD will handle the kcontrol IO callback IPC message 2.COMP DAI CMD will call the DAI SSP ops function to set the register. 3.SSP status is stored in DAI SSP private date
SOFT: 1. Add token for dai_in widget to create kcontrol bind with COMP DAI. 2. Create loopback specific topology file for loopback usage.
KERNEL: 1. Add debugmode debugfs 2. Parse token form 3. Add kcontrol if there is config 4. only send IPC if we are in debugmode for put IO callback handler.
From the discuss above the question now is focus on the topology tokens and the status store: 1. the token should be some more generic flag 2. The M4 macro should have some parameter for loopback topology 3. The SOF status should align with flag
So here are some questions: 1. What is the final exception for this feature? What other features are like this loopback mode? 2. What tplg files should we have? Should we enable the kcontrol as default if the platform support, or have some separate tplg file for loopback usage. 3. Where should the SSP Loopback mode kcontrol status be stored? In private data or in the sof_ipc_dai_config? The sof_ipc_dai_config is some structure used for DAI config and I did not think it should store some firmware only status.
Thanks Xiuli
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Thu, 2018-06-21 at 15:06 +0800, Pan, Xiuli wrote:
On 6/21/2018 03:58, Pierre-Louis Bossart wrote:
On 06/20/2018 02:03 PM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 11:40 -0500, Pierre-Louis Bossart wrote:
On 6/20/18 5:43 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
but loopback is not just for SSP, other non Intel DAIs also support it.
Humm, not sure I agree here with the logic.
Some non-Intel DAIs support LBM. One Intel DAI out of 4 - the SSP - supports LBM
McASP, McBSP have loopback and probably plenty of others too.
ipc_dai_config is mostly a union today:
/* general purpose DAI configuration */ struct sof_ipc_dai_config { struct sof_ipc_hdr hdr; enum sof_ipc_dai_type type; uint32_t id; /* physical number if more than 1 of this type */
/* physical protocol and clocking */ uint16_t format; /* SOF_DAI_FMT_ */ uint16_t reserved; /* alignment */ /* HW specific data */ union { struct sof_ipc_dai_ssp_params ssp; struct sof_ipc_dai_hda_params hda; struct sof_ipc_dai_dmic_params dmic; };};
we discussed earlier that the format was SSP specific, and if there are additional interfaces they will have to be dealt with an extension of a union. So now I am surprised to see you add a new generic field which isn't really applicable to all DAI types.
-> so what is the reason for making this field generic? It creates a case where LBM would be configurable for Intel DAIs who don't support it...
Not really, if the LBM flag was set for HDA in topology then it would be rejected by the HDA DAI driver and passed as an error back up the stack.
So we need an error check in the driver which parses the topology AND in the firmware?
I am not going to lay on the tracks but I don't see where this is going. I find it simpler to just add this field when it's supported. Not to mention that the addition of such generic parameters is very unlikely moving forward if we have to deal with backwards compatibility.
Hi Liam/Pierre
I am quite confused now, let me show the current status and the planning: Current:
SOF: 1.COMP DAI CMD will handle the kcontrol IO callback IPC message 2.COMP DAI CMD will call the DAI SSP ops function to set the register. 3.SSP status is stored in DAI SSP private date
SOFT:
- Add token for dai_in widget to create kcontrol bind with COMP DAI.
- Create loopback specific topology file for loopback usage.
KERNEL:
- Add debugmode debugfs
- Parse token form
- Add kcontrol if there is config
- only send IPC if we are in debugmode for put IO callback handler.
From the discuss above the question now is focus on the topology tokens and the status store:
- the token should be some more generic flag
- The M4 macro should have some parameter for loopback topology
- The SOF status should align with flag
So here are some questions:
- What is the final exception for this feature? What other features are
like this loopback mode?
tristate is similar, SSP supports it.
- What tplg files should we have? Should we enable the kcontrol as
default if the platform support, or have some separate tplg file for loopback usage.
default.
1) apl.m4 can include ssp.m4.
2) ssp.m4 will define a macro to build the kcontrol for LBM.
3) This macro can then be invoked by dai.m4, if it's undefined it will not create a kcontrol.
4) This means all Intel platforms that have SSP will include ssp.m4 and all SSP topology users will get this kcontrol for each SSP port.
- Where should the SSP Loopback mode kcontrol status be stored? In
private data or in the sof_ipc_dai_config?
status is private data, topology data should not b used for runtime status.
The sof_ipc_dai_config is some structure used for DAI config and I did not think it should store some firmware only status.
Liam
Thanks Xiuli
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Wed, 2018-06-20 at 14:58 -0500, Pierre-Louis Bossart wrote:
On 06/20/2018 02:03 PM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 11:40 -0500, Pierre-Louis Bossart wrote:
On 6/20/18 5:43 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
but loopback is not just for SSP, other non Intel DAIs also support it.
Humm, not sure I agree here with the logic.
Some non-Intel DAIs support LBM. One Intel DAI out of 4 - the SSP - supports LBM
McASP, McBSP have loopback and probably plenty of others too.
ipc_dai_config is mostly a union today:
/* general purpose DAI configuration */ struct sof_ipc_dai_config { struct sof_ipc_hdr hdr; enum sof_ipc_dai_type type; uint32_t id; /* physical number if more than 1 of this type */
/* physical protocol and clocking */ uint16_t format; /* SOF_DAI_FMT_ */ uint16_t reserved; /* alignment */ /* HW specific data */ union { struct sof_ipc_dai_ssp_params ssp; struct sof_ipc_dai_hda_params hda; struct sof_ipc_dai_dmic_params dmic; };};
we discussed earlier that the format was SSP specific, and if there are additional interfaces they will have to be dealt with an extension of a union. So now I am surprised to see you add a new generic field which isn't really applicable to all DAI types.
It would be a generic kcontrol flags field that will contain a flag for loopback, tristate and anything else that's non specific. I know they are not available on all, but this does save code duplication within the FW and driver since we dont have to duplicate logic around reading flags in different structures.
-> so what is the reason for making this field generic? It creates a case where LBM would be configurable for Intel DAIs who don't support it...
Not really, if the LBM flag was set for HDA in topology then it would be rejected by the HDA DAI driver and passed as an error back up the stack.
So we need an error check in the driver which parses the topology AND in the firmware?
No, the intention is that flags are propagated down the stack to the FW DAI driver which can then reject any incorrect request.
I am not going to lay on the tracks but I don't see where this is going. I find it simpler to just add this field when it's supported. Not to mention that the addition of such generic parameters is very unlikely moving forward if we have to deal with backwards compatibility.
I wont lie on the tracks either :) Please add the flags to SSP structure and if possible using some generic flag types/values would help is reusing this code for other DAIs.
Liam
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 6/21/18 5:49 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 14:58 -0500, Pierre-Louis Bossart wrote:
On 06/20/2018 02:03 PM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 11:40 -0500, Pierre-Louis Bossart wrote:
On 6/20/18 5:43 AM, Liam Girdwood wrote:
On Wed, 2018-06-20 at 13:55 +0800, Pan, Xiuli wrote:
ipc_dai_config is a shared structure from host to DSP, it should not be used to save some SSP only stauts and it may also not be generic. This change is made for this reason as your last two comments about adding flag in
struct sof_ipc_dai_config and struct dai with this SSP only status.
but loopback is not just for SSP, other non Intel DAIs also support it.
Humm, not sure I agree here with the logic.
Some non-Intel DAIs support LBM. One Intel DAI out of 4 - the SSP - supports LBM
McASP, McBSP have loopback and probably plenty of others too.
ipc_dai_config is mostly a union today:
/* general purpose DAI configuration */ struct sof_ipc_dai_config { struct sof_ipc_hdr hdr; enum sof_ipc_dai_type type; uint32_t id; /* physical number if more than 1 of this type */
/* physical protocol and clocking */ uint16_t format; /* SOF_DAI_FMT_ */ uint16_t reserved; /* alignment */ /* HW specific data */ union { struct sof_ipc_dai_ssp_params ssp; struct sof_ipc_dai_hda_params hda; struct sof_ipc_dai_dmic_params dmic; };};
we discussed earlier that the format was SSP specific, and if there are additional interfaces they will have to be dealt with an extension of a union. So now I am surprised to see you add a new generic field which isn't really applicable to all DAI types.
It would be a generic kcontrol flags field that will contain a flag for loopback, tristate and anything else that's non specific. I know they are not available on all, but this does save code duplication within the FW and driver since we dont have to duplicate logic around reading flags in different structures.
Part of the problem is that we bundle together DAIs that are very different in nature (SSP, DMIC, HDA, SoundWire), but indeed non-intel interfaces will be similar to SSP in terms of configuration. Similar yet different, some interfaces support different clocks for RX and TX, multiple lanes... Good luck with generic parameters...
-> so what is the reason for making this field generic? It creates a case where LBM would be configurable for Intel DAIs who don't support it...
Not really, if the LBM flag was set for HDA in topology then it would be rejected by the HDA DAI driver and passed as an error back up the stack.
So we need an error check in the driver which parses the topology AND in the firmware?
No, the intention is that flags are propagated down the stack to the FW DAI driver which can then reject any incorrect request.
I am not going to lay on the tracks but I don't see where this is going. I find it simpler to just add this field when it's supported. Not to mention that the addition of such generic parameters is very unlikely moving forward if we have to deal with backwards compatibility.
I wont lie on the tracks either :) Please add the flags to SSP structure and if possible using some generic flag types/values would help is reusing this code for other DAIs.
You lost me here. if LBM is a generic flag it should be added in the generic part of the ipc_config, not the SSP structure.
I don't have a good feel for other generic flags, tristate maybe. Any other suggestions?
Liam
Liam
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (5)
-
Liam Girdwood -
Pan, Xiuli -
Pierre-Louis Bossart -
Ranjani Sridharan -
Xiuli Pan