[PATCH 00/16] ASoC: SOF: Intel: HDaudio cleanups
This is the part1 of my HDaudio cleanups, before the addition of to-be-announced HDaudio extensions.
The patchset includes more consistent use of read/write/update helpers, removal of useless waits, structure members and programming sequences, removal of confusing sharing of private_data between FE and BE.
Additional patches are coming to split the controller, codec and multi-link management functionality in well-identified files.
Pierre-Louis Bossart (16): ASoC: SOF: ops: fallback to mmio in helpers ASoC: SOF: Intel: use mmio fallback for all platforms ASoC: SOF: ops: add readb/writeb helpers ASoC: SOF: ops: add snd_sof_dsp_updateb() helper ASoC: SOF: Intel: hda-dsp: use SOF helpers for consistency ASoC: SOF: Intel: hda-dai: start removing the use of runtime->private_data in BE ASoC: SOF: Intel: hda-dai: use component_get_drvdata to find hdac_bus ASoC: SOF: Intel: hda-dai: remove useless members in hda_pipe_params ASoC: SOF: Intel: hda-ctrl: remove useless sleep ASoC: SOF: Intel: hda: always do a full reset ASoC: SOF: Intel: hda: remove useless check on GCTL ASoC: SOF: Intel: hda-stream: use SOF helpers for consistency ASoC: SOF: Intel: hda-stream: rename CL_SD_CTL registers as SD_CTL ASoC: SOF: Intel: hda: use SOF helper for consistency ASoC: SOF: Intel: hda-stream: use snd_sof_dsp_updateb() helper ASoC: SOF: Intel: hda-stream: use readb/writeb for stream registers
sound/soc/sof/intel/bdw.c | 6 +--- sound/soc/sof/intel/byt.c | 12 ++----- sound/soc/sof/intel/hda-common-ops.c | 6 +--- sound/soc/sof/intel/hda-ctrl.c | 41 ++++++++-------------- sound/soc/sof/intel/hda-dai.c | 31 +++++++---------- sound/soc/sof/intel/hda-dsp.c | 12 ++++--- sound/soc/sof/intel/hda-loader-skl.c | 30 ++++++++-------- sound/soc/sof/intel/hda-loader.c | 4 +-- sound/soc/sof/intel/hda-stream.c | 47 ++++++++++++------------- sound/soc/sof/intel/hda.c | 5 ++- sound/soc/sof/intel/hda.h | 25 +++++++------- sound/soc/sof/intel/pci-tng.c | 6 +--- sound/soc/sof/ops.h | 51 ++++++++++++++++++++-------- sound/soc/sof/sof-priv.h | 4 +++ 14 files changed, 136 insertions(+), 144 deletions(-)
Returning an error when a read/write is not implemented makes no sense, especially on read where no return value makes sense.
Change the logic to directly fallback to mmio. If a platform truly wants other read/writes that are not plain vanilla mmio, it needs to implement its own routines.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/ops.h | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h index 55d43adb6a295..a72f8043be64d 100644 --- a/sound/soc/sof/ops.h +++ b/sound/soc/sof/ops.h @@ -305,23 +305,19 @@ static inline int snd_sof_debugfs_add_region_item(struct snd_sof_dev *sdev, static inline void snd_sof_dsp_write(struct snd_sof_dev *sdev, u32 bar, u32 offset, u32 value) { - if (sof_ops(sdev)->write) { + if (sof_ops(sdev)->write) sof_ops(sdev)->write(sdev, sdev->bar[bar] + offset, value); - return; - } - - dev_err_ratelimited(sdev->dev, "error: %s not defined\n", __func__); + else + writel(value, sdev->bar[bar] + offset); }
static inline void snd_sof_dsp_write64(struct snd_sof_dev *sdev, u32 bar, u32 offset, u64 value) { - if (sof_ops(sdev)->write64) { + if (sof_ops(sdev)->write64) sof_ops(sdev)->write64(sdev, sdev->bar[bar] + offset, value); - return; - } - - dev_err_ratelimited(sdev->dev, "error: %s not defined\n", __func__); + else + writeq(value, sdev->bar[bar] + offset); }
static inline u32 snd_sof_dsp_read(struct snd_sof_dev *sdev, u32 bar, @@ -329,9 +325,8 @@ static inline u32 snd_sof_dsp_read(struct snd_sof_dev *sdev, u32 bar, { if (sof_ops(sdev)->read) return sof_ops(sdev)->read(sdev, sdev->bar[bar] + offset); - - dev_err(sdev->dev, "error: %s not defined\n", __func__); - return -ENOTSUPP; + else + return readl(sdev->bar[bar] + offset); }
static inline u64 snd_sof_dsp_read64(struct snd_sof_dev *sdev, u32 bar, @@ -339,9 +334,8 @@ static inline u64 snd_sof_dsp_read64(struct snd_sof_dev *sdev, u32 bar, { if (sof_ops(sdev)->read64) return sof_ops(sdev)->read64(sdev, sdev->bar[bar] + offset); - - dev_err(sdev->dev, "error: %s not defined\n", __func__); - return -ENOTSUPP; + else + return readq(sdev->bar[bar] + offset); }
/* block IO */
No need to expose an indirection when we can use the fallback.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/intel/bdw.c | 6 +----- sound/soc/sof/intel/byt.c | 12 ++---------- sound/soc/sof/intel/hda-common-ops.c | 6 +----- sound/soc/sof/intel/pci-tng.c | 6 +----- 4 files changed, 5 insertions(+), 25 deletions(-)
diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c index a446154f28032..812a49b1d3f49 100644 --- a/sound/soc/sof/intel/bdw.c +++ b/sound/soc/sof/intel/bdw.c @@ -575,11 +575,7 @@ static struct snd_sof_dsp_ops sof_bdw_ops = { .run = bdw_run, .reset = bdw_reset,
- /* Register IO */ - .write = sof_io_write, - .read = sof_io_read, - .write64 = sof_io_write64, - .read64 = sof_io_read64, + /* Register IO uses direct mmio */
/* Block IO */ .block_read = sof_block_read, diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c index e6dc4ff531c34..faf223b383605 100644 --- a/sound/soc/sof/intel/byt.c +++ b/sound/soc/sof/intel/byt.c @@ -225,11 +225,7 @@ static struct snd_sof_dsp_ops sof_byt_ops = { .run = atom_run, .reset = atom_reset,
- /* Register IO */ - .write = sof_io_write, - .read = sof_io_read, - .write64 = sof_io_write64, - .read64 = sof_io_read64, + /* Register IO uses direct mmio */
/* Block IO */ .block_read = sof_block_read, @@ -304,11 +300,7 @@ static struct snd_sof_dsp_ops sof_cht_ops = { .run = atom_run, .reset = atom_reset,
- /* Register IO */ - .write = sof_io_write, - .read = sof_io_read, - .write64 = sof_io_write64, - .read64 = sof_io_read64, + /* Register IO uses direct mmio */
/* Block IO */ .block_read = sof_block_read, diff --git a/sound/soc/sof/intel/hda-common-ops.c b/sound/soc/sof/intel/hda-common-ops.c index b2326396c870e..397303b3ac9d5 100644 --- a/sound/soc/sof/intel/hda-common-ops.c +++ b/sound/soc/sof/intel/hda-common-ops.c @@ -19,11 +19,7 @@ struct snd_sof_dsp_ops sof_hda_common_ops = { .probe = hda_dsp_probe, .remove = hda_dsp_remove,
- /* Register IO */ - .write = sof_io_write, - .read = sof_io_read, - .write64 = sof_io_write64, - .read64 = sof_io_read64, + /* Register IO uses direct mmio */
/* Block IO */ .block_read = sof_block_read, diff --git a/sound/soc/sof/intel/pci-tng.c b/sound/soc/sof/intel/pci-tng.c index f0f6d9ba88037..5b2b409752c58 100644 --- a/sound/soc/sof/intel/pci-tng.c +++ b/sound/soc/sof/intel/pci-tng.c @@ -144,11 +144,7 @@ struct snd_sof_dsp_ops sof_tng_ops = { .run = atom_run, .reset = atom_reset,
- /* Register IO */ - .write = sof_io_write, - .read = sof_io_read, - .write64 = sof_io_write64, - .read64 = sof_io_read64, + /* Register IO uses direct mmio */
/* Block IO */ .block_read = sof_block_read,
These will be used to add more consistency in the SOF core and drivers.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/ops.h | 18 ++++++++++++++++++ sound/soc/sof/sof-priv.h | 4 ++++ 2 files changed, 22 insertions(+)
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h index a72f8043be64d..511c798eb1ebb 100644 --- a/sound/soc/sof/ops.h +++ b/sound/soc/sof/ops.h @@ -302,6 +302,15 @@ static inline int snd_sof_debugfs_add_region_item(struct snd_sof_dev *sdev, }
/* register IO */ +static inline void snd_sof_dsp_writeb(struct snd_sof_dev *sdev, u32 bar, + u32 offset, u8 value) +{ + if (sof_ops(sdev)->writeb) + sof_ops(sdev)->writeb(sdev, sdev->bar[bar] + offset, value); + else + writeb(value, sdev->bar[bar] + offset); +} + static inline void snd_sof_dsp_write(struct snd_sof_dev *sdev, u32 bar, u32 offset, u32 value) { @@ -320,6 +329,15 @@ static inline void snd_sof_dsp_write64(struct snd_sof_dev *sdev, u32 bar, writeq(value, sdev->bar[bar] + offset); }
+static inline u8 snd_sof_dsp_readb(struct snd_sof_dev *sdev, u32 bar, + u32 offset) +{ + if (sof_ops(sdev)->readb) + return sof_ops(sdev)->readb(sdev, sdev->bar[bar] + offset); + else + return readb(sdev->bar[bar] + offset); +} + static inline u32 snd_sof_dsp_read(struct snd_sof_dev *sdev, u32 bar, u32 offset) { diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index 403e81220244a..d3ede97b67590 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -171,6 +171,10 @@ struct snd_sof_dsp_ops { * TODO: consider removing these operations and calling respective * implementations directly */ + void (*writeb)(struct snd_sof_dev *sof_dev, void __iomem *addr, + u8 value); /* optional */ + u8 (*readb)(struct snd_sof_dev *sof_dev, + void __iomem *addr); /* optional */ void (*write)(struct snd_sof_dev *sof_dev, void __iomem *addr, u32 value); /* optional */ u32 (*read)(struct snd_sof_dev *sof_dev,
Hi Pierre-Louis,
On Mon, Oct 24, 2022 at 11:52:57AM -0500, Pierre-Louis Bossart wrote:
These will be used to add more consistency in the SOF core and drivers.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com
This change is now in -next as commit 74fe0c4dcb41 ("ASoC: SOF: ops: add readb/writeb helpers"), where it breaks the build badly on at least ARCH=arm:
$ kmake ARCH=arm CROSS_COMPILE=arm-linux-gnu- allmodconfig sound/soc/sof/ ... In file included from sound/soc/sof/sof-client.c:16: sound/soc/sof/ops.h: In function ‘snd_sof_dsp_writeb’: sound/soc/sof/ops.h:309:75: error: macro "writeb" passed 3 arguments, but takes just 2 309 | sof_ops(sdev)->writeb(sdev, sdev->bar[bar] + offset, value); | ^ In file included from ./include/linux/io.h:13, from ./include/linux/irq.h:20, from ./include/asm-generic/hardirq.h:17, from ./arch/arm/include/asm/hardirq.h:10, from ./include/linux/hardirq.h:11, from ./include/linux/interrupt.h:11, from sound/soc/sof/ops.h:15: ./arch/arm/include/asm/io.h:288: note: macro "writeb" defined here 288 | #define writeb(v,c) ({ __iowmb(); writeb_relaxed(v,c); }) | sound/soc/sof/ops.h:309:30: error: statement with no effect [-Werror=unused-value] 309 | sof_ops(sdev)->writeb(sdev, sdev->bar[bar] + offset, value); | ^ sound/soc/sof/ops.h: In function ‘snd_sof_dsp_readb’: sound/soc/sof/ops.h:336:74: error: macro "readb" passed 2 arguments, but takes just 1 336 | return sof_ops(sdev)->readb(sdev, sdev->bar[bar] + offset); | ^ ./arch/arm/include/asm/io.h:284: note: macro "readb" defined here 284 | #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; }) | sound/soc/sof/ops.h:336:37: error: returning ‘u8 (*)(struct snd_sof_dev *, void *)’ {aka ‘unsigned char (*)(struct snd_sof_dev *, void *)’} from a function with return type ‘u8’ {aka ‘unsigned char’} makes integer from pointer without a cast [-Werror=int-conversion] 336 | return sof_ops(sdev)->readb(sdev, sdev->bar[bar] + offset); | ^ cc1: all warnings being treated as errors ...
I guess the preprocessor gets to these calls first and everything explodes from there. Perhaps these should be namespaced somehow?
Cheers, Nathan
On 10/27/22 14:51, Nathan Chancellor wrote:
Hi Pierre-Louis,
On Mon, Oct 24, 2022 at 11:52:57AM -0500, Pierre-Louis Bossart wrote:
These will be used to add more consistency in the SOF core and drivers.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com
This change is now in -next as commit 74fe0c4dcb41 ("ASoC: SOF: ops: add readb/writeb helpers"), where it breaks the build badly on at least ARCH=arm:
$ kmake ARCH=arm CROSS_COMPILE=arm-linux-gnu- allmodconfig sound/soc/sof/ ... In file included from sound/soc/sof/sof-client.c:16: sound/soc/sof/ops.h: In function ‘snd_sof_dsp_writeb’: sound/soc/sof/ops.h:309:75: error: macro "writeb" passed 3 arguments, but takes just 2 309 | sof_ops(sdev)->writeb(sdev, sdev->bar[bar] + offset, value); | ^ In file included from ./include/linux/io.h:13, from ./include/linux/irq.h:20, from ./include/asm-generic/hardirq.h:17, from ./arch/arm/include/asm/hardirq.h:10, from ./include/linux/hardirq.h:11, from ./include/linux/interrupt.h:11, from sound/soc/sof/ops.h:15: ./arch/arm/include/asm/io.h:288: note: macro "writeb" defined here 288 | #define writeb(v,c) ({ __iowmb(); writeb_relaxed(v,c); }) | sound/soc/sof/ops.h:309:30: error: statement with no effect [-Werror=unused-value] 309 | sof_ops(sdev)->writeb(sdev, sdev->bar[bar] + offset, value); | ^ sound/soc/sof/ops.h: In function ‘snd_sof_dsp_readb’: sound/soc/sof/ops.h:336:74: error: macro "readb" passed 2 arguments, but takes just 1 336 | return sof_ops(sdev)->readb(sdev, sdev->bar[bar] + offset); | ^ ./arch/arm/include/asm/io.h:284: note: macro "readb" defined here 284 | #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; }) | sound/soc/sof/ops.h:336:37: error: returning ‘u8 (*)(struct snd_sof_dev *, void *)’ {aka ‘unsigned char (*)(struct snd_sof_dev *, void *)’} from a function with return type ‘u8’ {aka ‘unsigned char’} makes integer from pointer without a cast [-Werror=int-conversion] 336 | return sof_ops(sdev)->readb(sdev, sdev->bar[bar] + offset); | ^ cc1: all warnings being treated as errors ...
I guess the preprocessor gets to these calls first and everything explodes from there. Perhaps these should be namespaced somehow?
Thanks for the report. We've had these patches for a while and always compile for ARM, and didn't see any issues raised. Meh.
I don't have a strong appetite for changes in common parts, maybe it's just simpler to use read8/write8 as callback names to avoid the conflict, something like the patch attached (compile-tested only). In hindsight it'd also be more consistent with the use of read64/write64 that was added earlier in SOF helpers.
On Thu, Oct 27, 2022 at 03:58:08PM -0400, Pierre-Louis Bossart wrote:
On 10/27/22 14:51, Nathan Chancellor wrote:
Hi Pierre-Louis,
On Mon, Oct 24, 2022 at 11:52:57AM -0500, Pierre-Louis Bossart wrote:
These will be used to add more consistency in the SOF core and drivers.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com
This change is now in -next as commit 74fe0c4dcb41 ("ASoC: SOF: ops: add readb/writeb helpers"), where it breaks the build badly on at least ARCH=arm:
$ kmake ARCH=arm CROSS_COMPILE=arm-linux-gnu- allmodconfig sound/soc/sof/ ... In file included from sound/soc/sof/sof-client.c:16: sound/soc/sof/ops.h: In function ‘snd_sof_dsp_writeb’: sound/soc/sof/ops.h:309:75: error: macro "writeb" passed 3 arguments, but takes just 2 309 | sof_ops(sdev)->writeb(sdev, sdev->bar[bar] + offset, value); | ^ In file included from ./include/linux/io.h:13, from ./include/linux/irq.h:20, from ./include/asm-generic/hardirq.h:17, from ./arch/arm/include/asm/hardirq.h:10, from ./include/linux/hardirq.h:11, from ./include/linux/interrupt.h:11, from sound/soc/sof/ops.h:15: ./arch/arm/include/asm/io.h:288: note: macro "writeb" defined here 288 | #define writeb(v,c) ({ __iowmb(); writeb_relaxed(v,c); }) | sound/soc/sof/ops.h:309:30: error: statement with no effect [-Werror=unused-value] 309 | sof_ops(sdev)->writeb(sdev, sdev->bar[bar] + offset, value); | ^ sound/soc/sof/ops.h: In function ‘snd_sof_dsp_readb’: sound/soc/sof/ops.h:336:74: error: macro "readb" passed 2 arguments, but takes just 1 336 | return sof_ops(sdev)->readb(sdev, sdev->bar[bar] + offset); | ^ ./arch/arm/include/asm/io.h:284: note: macro "readb" defined here 284 | #define readb(c) ({ u8 __v = readb_relaxed(c); __iormb(); __v; }) | sound/soc/sof/ops.h:336:37: error: returning ‘u8 (*)(struct snd_sof_dev *, void *)’ {aka ‘unsigned char (*)(struct snd_sof_dev *, void *)’} from a function with return type ‘u8’ {aka ‘unsigned char’} makes integer from pointer without a cast [-Werror=int-conversion] 336 | return sof_ops(sdev)->readb(sdev, sdev->bar[bar] + offset); | ^ cc1: all warnings being treated as errors ...
I guess the preprocessor gets to these calls first and everything explodes from there. Perhaps these should be namespaced somehow?
Thanks for the report. We've had these patches for a while and always compile for ARM, and didn't see any issues raised. Meh.
I don't have a strong appetite for changes in common parts, maybe it's just simpler to use read8/write8 as callback names to avoid the conflict, something like the patch attached (compile-tested only). In hindsight it'd also be more consistent with the use of read64/write64 that was added earlier in SOF helpers.
That diff resolves the build failure for me as well. I will put it into my test matrix and make sure that it does not cause any other build issues.
Cheers, Nathan
Add missing helper in SOF toolbox.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/ops.h | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h index 511c798eb1ebb..8cb93e7c0c670 100644 --- a/sound/soc/sof/ops.h +++ b/sound/soc/sof/ops.h @@ -356,6 +356,17 @@ static inline u64 snd_sof_dsp_read64(struct snd_sof_dev *sdev, u32 bar, return readq(sdev->bar[bar] + offset); }
+static inline void snd_sof_dsp_updateb(struct snd_sof_dev *sdev, u32 bar, + u32 offset, u8 value, u8 mask) +{ + u8 reg; + + reg = snd_sof_dsp_readb(sdev, bar, offset); + reg &= ~mask; + reg |= value; + snd_sof_dsp_writeb(sdev, bar, offset, reg); +} + /* block IO */ static inline int snd_sof_dsp_block_read(struct snd_sof_dev *sdev, enum snd_sof_fw_blk_type blk_type,
No functionality change.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/intel/hda-dsp.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 799c50fe24dab..74067df96fdc4 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -347,10 +347,9 @@ void hda_dsp_ipc_int_disable(struct snd_sof_dev *sdev)
static int hda_dsp_wait_d0i3c_done(struct snd_sof_dev *sdev) { - struct hdac_bus *bus = sof_to_bus(sdev); int retry = HDA_DSP_REG_POLL_RETRY_COUNT;
- while (snd_hdac_chip_readb(bus, VS_D0I3C) & SOF_HDA_VS_D0I3C_CIP) { + while (snd_sof_dsp_readb(sdev, HDA_DSP_HDA_BAR, SOF_HDA_VS_D0I3C) & SOF_HDA_VS_D0I3C_CIP) { if (!retry--) return -ETIMEDOUT; usleep_range(10, 15); @@ -380,6 +379,7 @@ static int hda_dsp_update_d0i3c_register(struct snd_sof_dev *sdev, u8 value) { struct hdac_bus *bus = sof_to_bus(sdev); int ret; + u8 reg;
/* Write to D0I3C after Command-In-Progress bit is cleared */ ret = hda_dsp_wait_d0i3c_done(sdev); @@ -389,7 +389,8 @@ static int hda_dsp_update_d0i3c_register(struct snd_sof_dev *sdev, u8 value) }
/* Update D0I3C register */ - snd_hdac_chip_updateb(bus, VS_D0I3C, SOF_HDA_VS_D0I3C_I3, value); + snd_sof_dsp_updateb(sdev, HDA_DSP_HDA_BAR, + SOF_HDA_VS_D0I3C, SOF_HDA_VS_D0I3C_I3, value);
/* Wait for cmd in progress to be cleared before exiting the function */ ret = hda_dsp_wait_d0i3c_done(sdev); @@ -398,7 +399,8 @@ static int hda_dsp_update_d0i3c_register(struct snd_sof_dev *sdev, u8 value) return ret; }
- trace_sof_intel_D0I3C_updated(sdev, snd_hdac_chip_readb(bus, VS_D0I3C)); + reg = snd_sof_dsp_readb(sdev, HDA_DSP_HDA_BAR, SOF_HDA_VS_D0I3C); + trace_sof_intel_D0I3C_updated(sdev, reg);
return 0; }
The SOF HDAudio code stores the Host DMA hdac_stream structure in the FE substream->runtime->private_data. The BE dailink also uses the substream->runtime->private_data to allocate the link DMA stream tag.
This really works by accident: the DPCM core copies the FE runtime information in the BE, which has the side-effect of sharing the FE-specific private_data with the BE.
To avoid more uses of the private_data with potential issues such as accessing stale information or use-after-free cases, this patch removes most of the usages of this private_data at the BE level. We can directly use the existing dma_data to access the relevant information.
However the hw_params still uses the information, mainly to go back to the 'bus' structure required for the link dma stream tag allocation. This is safe in that the 'bus' is not stream or PCM specific.
The next patch will completely remove this last use of private_data by using the component_drvdata - which is how SOF passes a global context around.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 64e8ca016b216..c7b07c2693653 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -138,12 +138,12 @@ hda_link_stream_assign(struct hdac_bus *bus, }
static int hda_link_dma_cleanup(struct snd_pcm_substream *substream, - struct hdac_stream *hstream, + struct hdac_ext_stream *hext_stream, struct snd_soc_dai *cpu_dai, struct snd_soc_dai *codec_dai, bool trigger_suspend_stop) { - struct hdac_ext_stream *hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); + struct hdac_stream *hstream = &hext_stream->hstream; struct hdac_bus *bus = hstream->bus; struct sof_intel_hda_stream *hda_stream; struct hdac_ext_link *hlink; @@ -257,7 +257,6 @@ static int hda_link_dma_prepare(struct snd_pcm_substream *substream)
static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd) { - struct hdac_stream *hstream = substream->runtime->private_data; struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); @@ -274,7 +273,7 @@ static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd) break; case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_STOP: - ret = hda_link_dma_cleanup(substream, hstream, cpu_dai, codec_dai, true); + ret = hda_link_dma_cleanup(substream, hext_stream, cpu_dai, codec_dai, true); if (ret < 0) return ret;
@@ -291,7 +290,6 @@ static int hda_link_dma_trigger(struct snd_pcm_substream *substream, int cmd)
static int hda_link_dma_hw_free(struct snd_pcm_substream *substream) { - struct hdac_stream *hstream = substream->runtime->private_data; struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); @@ -301,7 +299,7 @@ static int hda_link_dma_hw_free(struct snd_pcm_substream *substream) if (!hext_stream) return 0;
- return hda_link_dma_cleanup(substream, hstream, cpu_dai, codec_dai, false); + return hda_link_dma_cleanup(substream, hext_stream, cpu_dai, codec_dai, false); }
static int hda_dai_widget_update(struct snd_soc_dapm_widget *w, @@ -458,14 +456,12 @@ static int ipc4_hda_dai_trigger(struct snd_pcm_substream *substream, struct snd_sof_widget *swidget; struct snd_soc_dapm_widget *w; struct snd_soc_dai *codec_dai; - struct hdac_stream *hstream; struct snd_soc_dai *cpu_dai; int ret;
dev_dbg(dai->dev, "cmd=%d dai %s direction %d\n", cmd, dai->name, substream->stream);
- hstream = substream->runtime->private_data; rtd = asoc_substream_to_rtd(substream); cpu_dai = asoc_rtd_to_cpu(rtd, 0); codec_dai = asoc_rtd_to_codec(rtd, 0); @@ -500,7 +496,7 @@ static int ipc4_hda_dai_trigger(struct snd_pcm_substream *substream,
pipeline->state = SOF_IPC4_PIPE_RESET;
- ret = hda_link_dma_cleanup(substream, hstream, cpu_dai, codec_dai, false); + ret = hda_link_dma_cleanup(substream, hext_stream, cpu_dai, codec_dai, false); if (ret < 0) { dev_err(sdev->dev, "%s: failed to clean up link DMA\n", __func__); return ret; @@ -575,7 +571,8 @@ static int hda_dai_suspend(struct hdac_bus *bus) cpu_dai = asoc_rtd_to_cpu(rtd, 0); codec_dai = asoc_rtd_to_codec(rtd, 0);
- ret = hda_link_dma_cleanup(hext_stream->link_substream, s, + ret = hda_link_dma_cleanup(hext_stream->link_substream, + hext_stream, cpu_dai, codec_dai, false); if (ret < 0) return ret;
Remove the last usage of substream->runtime->private_data in the HDAudio BE hw_params.
The SOF core saves the 'sdev' global context as component drvdata, and we already save the bus information in sdev.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index c7b07c2693653..2bce8dff4627a 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -207,14 +207,17 @@ static int hda_link_dma_params(struct hdac_ext_stream *hext_stream, static int hda_link_dma_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { - struct hdac_stream *hstream = substream->runtime->private_data; - struct hdac_ext_stream *hext_stream; struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); struct hda_pipe_params p_params = {0}; - struct hdac_bus *bus = hstream->bus; + struct hdac_ext_stream *hext_stream; struct hdac_ext_link *hlink; + struct snd_sof_dev *sdev; + struct hdac_bus *bus; + + sdev = snd_soc_component_get_drvdata(cpu_dai->component); + bus = sof_to_bus(sdev);
hext_stream = snd_soc_dai_get_dma_data(cpu_dai, substream); if (!hext_stream) {
Some settings were never or are no longer used, remove useless definitions and assignments.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/intel/hda-dai.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c index 2bce8dff4627a..6e368974abd16 100644 --- a/sound/soc/sof/intel/hda-dai.c +++ b/sound/soc/sof/intel/hda-dai.c @@ -32,11 +32,8 @@ MODULE_PARM_DESC(sof_use_tplg_nhlt, "SOF topology nhlt override"); struct hda_pipe_params { u32 ch; u32 s_freq; - u32 s_fmt; - u8 linktype; snd_pcm_format_t format; int link_index; - int stream; unsigned int link_bps; };
@@ -235,10 +232,8 @@ static int hda_link_dma_hw_params(struct snd_pcm_substream *substream, /* set the hdac_stream in the codec dai */ snd_soc_dai_set_stream(codec_dai, hdac_stream(hext_stream), substream->stream);
- p_params.s_fmt = snd_pcm_format_width(params_format(params)); p_params.ch = params_channels(params); p_params.s_freq = params_rate(params); - p_params.stream = substream->stream; p_params.link_index = hlink->index; p_params.format = params_format(params);
The hda_dsp_ctrl_link_reset() already performs a usleep and a check that GCTL has been modified, there's no point in waiting more.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-ctrl.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/soc/sof/intel/hda-ctrl.c b/sound/soc/sof/intel/hda-ctrl.c index 0c29bb196e593..7f387ad5ef6f4 100644 --- a/sound/soc/sof/intel/hda-ctrl.c +++ b/sound/soc/sof/intel/hda-ctrl.c @@ -207,16 +207,12 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool full_reset) goto err; }
- usleep_range(500, 1000); - /* exit HDA controller reset */ ret = hda_dsp_ctrl_link_reset(sdev, false); if (ret < 0) { dev_err(sdev->dev, "error: failed to exit HDA controller reset\n"); goto err; } - - usleep_range(1000, 1200); }
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
There's no point in checking for a full-reset condition that is always-true in the callers.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-ctrl.c | 26 ++++++++++++-------------- sound/soc/sof/intel/hda-dsp.c | 2 +- sound/soc/sof/intel/hda.c | 2 +- sound/soc/sof/intel/hda.h | 2 +- 4 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/sound/soc/sof/intel/hda-ctrl.c b/sound/soc/sof/intel/hda-ctrl.c index 7f387ad5ef6f4..e3334693c2926 100644 --- a/sound/soc/sof/intel/hda-ctrl.c +++ b/sound/soc/sof/intel/hda-ctrl.c @@ -182,7 +182,7 @@ int hda_dsp_ctrl_clock_power_gating(struct snd_sof_dev *sdev, bool enable) return 0; }
-int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool full_reset) +int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev) { struct hdac_bus *bus = sof_to_bus(sdev); #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) @@ -199,20 +199,18 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool full_reset) #endif hda_dsp_ctrl_misc_clock_gating(sdev, false);
- if (full_reset) { - /* reset HDA controller */ - ret = hda_dsp_ctrl_link_reset(sdev, true); - if (ret < 0) { - dev_err(sdev->dev, "error: failed to reset HDA controller\n"); - goto err; - } + /* reset HDA controller */ + ret = hda_dsp_ctrl_link_reset(sdev, true); + if (ret < 0) { + dev_err(sdev->dev, "error: failed to reset HDA controller\n"); + goto err; + }
- /* exit HDA controller reset */ - ret = hda_dsp_ctrl_link_reset(sdev, false); - if (ret < 0) { - dev_err(sdev->dev, "error: failed to exit HDA controller reset\n"); - goto err; - } + /* exit HDA controller reset */ + ret = hda_dsp_ctrl_link_reset(sdev, false); + if (ret < 0) { + dev_err(sdev->dev, "error: failed to exit HDA controller reset\n"); + goto err; }
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 74067df96fdc4..6d896ea316806 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -691,7 +691,7 @@ static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume) snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0);
/* reset and start hda controller */ - ret = hda_dsp_ctrl_init_chip(sdev, true); + ret = hda_dsp_ctrl_init_chip(sdev); if (ret < 0) { dev_err(sdev->dev, "error: failed to start controller after resume\n"); diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index d63f843dc7aab..2ee414600f779 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -890,7 +890,7 @@ static int hda_init_caps(struct snd_sof_dev *sdev) dev_dbg(sdev->dev, "PP capability, will probe DSP later.\n");
/* Init HDA controller after i915 init */ - ret = hda_dsp_ctrl_init_chip(sdev, true); + ret = hda_dsp_ctrl_init_chip(sdev); if (ret < 0) { dev_err(bus->dev, "error: init chip failed with ret: %d\n", ret); diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index c91fc36378235..6b1474d78ebb3 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -702,7 +702,7 @@ void hda_dsp_ctrl_ppcap_int_enable(struct snd_sof_dev *sdev, bool enable); int hda_dsp_ctrl_link_reset(struct snd_sof_dev *sdev, bool reset); void hda_dsp_ctrl_misc_clock_gating(struct snd_sof_dev *sdev, bool enable); int hda_dsp_ctrl_clock_power_gating(struct snd_sof_dev *sdev, bool enable); -int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool full_reset); +int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev); void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev); /* * HDA bus operations.
Now that we always do a full reset, there's no point in checking if the controller is always out-of-reset. This is always true by construction.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-ctrl.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/sound/soc/sof/intel/hda-ctrl.c b/sound/soc/sof/intel/hda-ctrl.c index e3334693c2926..8a8b5f93db254 100644 --- a/sound/soc/sof/intel/hda-ctrl.c +++ b/sound/soc/sof/intel/hda-ctrl.c @@ -214,13 +214,6 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev) }
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) - /* check to see if controller is ready */ - if (!snd_hdac_chip_readb(bus, GCTL)) { - dev_dbg(bus->dev, "controller not ready!\n"); - ret = -EBUSY; - goto err; - } - /* Accept unsolicited responses */ snd_hdac_chip_updatel(bus, GCTL, AZX_GCTL_UNSOL, AZX_GCTL_UNSOL);
Not sure why we mixed sof and hdac helpers, this makes the code way less readable.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-stream.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c index 8cb91788912cb..7ac703515be4a 100644 --- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -697,7 +697,8 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev) /* The function can be called at irq thread, so use spin_lock_irq */ spin_lock_irq(&bus->reg_lock);
- status = snd_hdac_chip_readl(bus, INTSTS); + status = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTSTS); + trace_sof_intel_hda_dsp_check_stream_irq(sdev, status);
/* if Register inaccessible, ignore it.*/ @@ -778,7 +779,7 @@ irqreturn_t hda_dsp_stream_threaded_handler(int irq, void *context) for (i = 0, active = true; i < 10 && active; i++) { spin_lock_irq(&bus->reg_lock);
- status = snd_hdac_chip_readl(bus, INTSTS); + status = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTSTS);
/* check streams */ active = hda_dsp_stream_check(bus, status);
The use of the CL prefix is misleading. HDaudio streams are used for code loading since ApolloLake, but they are also used for regular audio transfers.
No functionality change, pure rename.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-ctrl.c | 6 ++--- sound/soc/sof/intel/hda-loader-skl.c | 30 ++++++++++++------------ sound/soc/sof/intel/hda-loader.c | 4 ++-- sound/soc/sof/intel/hda-stream.c | 34 ++++++++++++++-------------- sound/soc/sof/intel/hda.h | 22 +++++++++--------- 5 files changed, 48 insertions(+), 48 deletions(-)
diff --git a/sound/soc/sof/intel/hda-ctrl.c b/sound/soc/sof/intel/hda-ctrl.c index 8a8b5f93db254..12900965ca5ff 100644 --- a/sound/soc/sof/intel/hda-ctrl.c +++ b/sound/soc/sof/intel/hda-ctrl.c @@ -234,7 +234,7 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev) list_for_each_entry(stream, &bus->stream_list, list) { sd_offset = SOF_STREAM_SD_OFFSET(stream); snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_STS, + sd_offset + SOF_HDA_ADSP_REG_SD_STS, SOF_HDA_CL_DMA_SD_INT_MASK); }
@@ -300,7 +300,7 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev) sd_offset = SOF_STREAM_SD_OFFSET(stream); snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, sd_offset + - SOF_HDA_ADSP_REG_CL_SD_CTL, + SOF_HDA_ADSP_REG_SD_CTL, SOF_HDA_CL_DMA_SD_INT_MASK, 0); } @@ -318,7 +318,7 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev) list_for_each_entry(stream, &bus->stream_list, list) { sd_offset = SOF_STREAM_SD_OFFSET(stream); snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_STS, + sd_offset + SOF_HDA_ADSP_REG_SD_STS, SOF_HDA_CL_DMA_SD_INT_MASK); }
diff --git a/sound/soc/sof/intel/hda-loader-skl.c b/sound/soc/sof/intel/hda-loader-skl.c index 3211f561db29c..69fdef8f89ae2 100644 --- a/sound/soc/sof/intel/hda-loader-skl.c +++ b/sound/soc/sof/intel/hda-loader-skl.c @@ -141,7 +141,7 @@ static void cl_skl_cldma_stream_run(struct snd_sof_dev *sdev, bool enable) u32 run = enable ? 0x1 : 0;
snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CTL, + sd_offset + SOF_HDA_ADSP_REG_SD_CTL, HDA_CL_SD_CTL_RUN(1), HDA_CL_SD_CTL_RUN(run));
retries = 300; @@ -150,7 +150,7 @@ static void cl_skl_cldma_stream_run(struct snd_sof_dev *sdev, bool enable)
/* waiting for hardware to report the stream Run bit set */ val = snd_sof_dsp_read(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CTL); + sd_offset + SOF_HDA_ADSP_REG_SD_CTL); val &= HDA_CL_SD_CTL_RUN(1); if (enable && val) break; @@ -174,23 +174,23 @@ static void cl_skl_cldma_stream_clear(struct snd_sof_dev *sdev) * Descriptor Error Interrupt and set the cldma stream number to 0. */ snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CTL, + sd_offset + SOF_HDA_ADSP_REG_SD_CTL, HDA_CL_SD_CTL_INT_MASK, HDA_CL_SD_CTL_INT(0)); snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CTL, + sd_offset + SOF_HDA_ADSP_REG_SD_CTL, HDA_CL_SD_CTL_STRM(0xf), HDA_CL_SD_CTL_STRM(0));
snd_sof_dsp_write(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPL, HDA_CL_SD_BDLPLBA(0)); + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL, HDA_CL_SD_BDLPLBA(0)); snd_sof_dsp_write(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPU, 0); + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPU, 0);
/* Set the Cyclic Buffer Length to 0. */ snd_sof_dsp_write(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CBL, 0); + sd_offset + SOF_HDA_ADSP_REG_SD_CBL, 0); /* Set the Last Valid Index. */ snd_sof_dsp_write(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_LVI, 0); + sd_offset + SOF_HDA_ADSP_REG_SD_LVI, 0); }
static void cl_skl_cldma_setup_spb(struct snd_sof_dev *sdev, @@ -240,27 +240,27 @@ static void cl_skl_cldma_setup_controller(struct snd_sof_dev *sdev,
/* setting the stream register */ snd_sof_dsp_write(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPL, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL, HDA_CL_SD_BDLPLBA(dmab_bdl->addr)); snd_sof_dsp_write(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPU, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPU, HDA_CL_SD_BDLPUBA(dmab_bdl->addr));
/* Set the Cyclic Buffer Length. */ snd_sof_dsp_write(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CBL, max_size); + sd_offset + SOF_HDA_ADSP_REG_SD_CBL, max_size); /* Set the Last Valid Index. */ snd_sof_dsp_write(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_LVI, count - 1); + sd_offset + SOF_HDA_ADSP_REG_SD_LVI, count - 1);
/* Set the Interrupt On Completion, FIFO Error Interrupt, * Descriptor Error Interrupt and the cldma stream number. */ snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CTL, + sd_offset + SOF_HDA_ADSP_REG_SD_CTL, HDA_CL_SD_CTL_INT_MASK, HDA_CL_SD_CTL_INT(1)); snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CTL, + sd_offset + SOF_HDA_ADSP_REG_SD_CTL, HDA_CL_SD_CTL_STRM(0xf), HDA_CL_SD_CTL_STRM(1)); } @@ -439,7 +439,7 @@ static int cl_skl_cldma_wait_interruptible(struct snd_sof_dev *sdev,
/* now check DMA interrupt status */ cl_dma_intr_status = snd_sof_dsp_read(sdev, HDA_DSP_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_STS); + sd_offset + SOF_HDA_ADSP_REG_SD_STS);
if (!(cl_dma_intr_status & HDA_CL_DMA_SD_INT_COMPLETE)) { dev_err(sdev->dev, "cldma copy failed\n"); diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c index 38204541fc5d9..3a4b0b6e2c5c2 100644 --- a/sound/soc/sof/intel/hda-loader.c +++ b/sound/soc/sof/intel/hda-loader.c @@ -265,9 +265,9 @@ int hda_cl_cleanup(struct snd_sof_dev *sdev, struct snd_dma_buffer *dmab,
/* reset BDL address */ snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPL, 0); + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL, 0); snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPU, 0); + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPU, 0);
snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, sd_offset, 0); snd_dma_free_pages(dmab); diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c index 7ac703515be4a..6d130b8028b1c 100644 --- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -367,7 +367,7 @@ int hda_dsp_stream_trigger(struct snd_sof_dev *sdev,
if (ret >= 0) { snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_STS, + sd_offset + SOF_HDA_ADSP_REG_SD_STS, SOF_HDA_CL_DMA_SD_INT_MASK);
hstream->running = false; @@ -419,10 +419,10 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
/* reset BDL address */ snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPL, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL, 0x0); snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPU, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPU, 0x0);
hstream->frags = 0; @@ -435,20 +435,20 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st
/* program BDL address */ snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPL, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL, (u32)hstream->bdl.addr); snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPU, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPU, upper_32_bits(hstream->bdl.addr));
/* program cyclic buffer length */ snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CBL, + sd_offset + SOF_HDA_ADSP_REG_SD_CBL, hstream->bufsize);
/* program last valid index */ snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_LVI, + sd_offset + SOF_HDA_ADSP_REG_SD_LVI, 0xffff, (hstream->frags - 1));
/* decouple host and link DMA, enable DSP features */ @@ -520,7 +520,7 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev, }
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_STS, + sd_offset + SOF_HDA_ADSP_REG_SD_STS, SOF_HDA_CL_DMA_SD_INT_MASK, SOF_HDA_CL_DMA_SD_INT_MASK);
@@ -534,10 +534,10 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
/* reset BDL address */ snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPL, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL, 0x0); snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPU, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPU, 0x0);
/* clear stream status */ @@ -562,7 +562,7 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev, }
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_STS, + sd_offset + SOF_HDA_ADSP_REG_SD_STS, SOF_HDA_CL_DMA_SD_INT_MASK, SOF_HDA_CL_DMA_SD_INT_MASK);
@@ -582,7 +582,7 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
/* program cyclic buffer length */ snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_CBL, + sd_offset + SOF_HDA_ADSP_REG_SD_CBL, hstream->bufsize);
/* @@ -606,7 +606,7 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev, /* program stream format */ snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, sd_offset + - SOF_HDA_ADSP_REG_CL_SD_FORMAT, + SOF_HDA_ADSP_REG_SD_FORMAT, 0xffff, hstream->format_val);
if (chip->quirks & SOF_INTEL_PROCEN_FMT_QUIRK) { @@ -617,15 +617,15 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
/* program last valid index */ snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_LVI, + sd_offset + SOF_HDA_ADSP_REG_SD_LVI, 0xffff, (hstream->frags - 1));
/* program BDL address */ snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPL, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPL, (u32)hstream->bdl.addr); snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, - sd_offset + SOF_HDA_ADSP_REG_CL_SD_BDLPU, + sd_offset + SOF_HDA_ADSP_REG_SD_BDLPU, upper_32_bits(hstream->bdl.addr));
/* enable position buffer, if needed */ @@ -649,7 +649,7 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev, hstream->fifo_size = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, sd_offset + - SOF_HDA_ADSP_REG_CL_SD_FIFOSIZE); + SOF_HDA_ADSP_REG_SD_FIFOSIZE); hstream->fifo_size &= 0xffff; hstream->fifo_size += 1; } else { diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 6b1474d78ebb3..b2a785d0f0e94 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -122,17 +122,17 @@ #define SOF_HDA_ADSP_DPLBASE_ENABLE 0x01
/* Stream Registers */ -#define SOF_HDA_ADSP_REG_CL_SD_CTL 0x00 -#define SOF_HDA_ADSP_REG_CL_SD_STS 0x03 -#define SOF_HDA_ADSP_REG_CL_SD_LPIB 0x04 -#define SOF_HDA_ADSP_REG_CL_SD_CBL 0x08 -#define SOF_HDA_ADSP_REG_CL_SD_LVI 0x0C -#define SOF_HDA_ADSP_REG_CL_SD_FIFOW 0x0E -#define SOF_HDA_ADSP_REG_CL_SD_FIFOSIZE 0x10 -#define SOF_HDA_ADSP_REG_CL_SD_FORMAT 0x12 -#define SOF_HDA_ADSP_REG_CL_SD_FIFOL 0x14 -#define SOF_HDA_ADSP_REG_CL_SD_BDLPL 0x18 -#define SOF_HDA_ADSP_REG_CL_SD_BDLPU 0x1C +#define SOF_HDA_ADSP_REG_SD_CTL 0x00 +#define SOF_HDA_ADSP_REG_SD_STS 0x03 +#define SOF_HDA_ADSP_REG_SD_LPIB 0x04 +#define SOF_HDA_ADSP_REG_SD_CBL 0x08 +#define SOF_HDA_ADSP_REG_SD_LVI 0x0C +#define SOF_HDA_ADSP_REG_SD_FIFOW 0x0E +#define SOF_HDA_ADSP_REG_SD_FIFOSIZE 0x10 +#define SOF_HDA_ADSP_REG_SD_FORMAT 0x12 +#define SOF_HDA_ADSP_REG_SD_FIFOL 0x14 +#define SOF_HDA_ADSP_REG_SD_BDLPL 0x18 +#define SOF_HDA_ADSP_REG_SD_BDLPU 0x1C #define SOF_HDA_ADSP_SD_ENTRY_SIZE 0x20
/* CL: Software Position Based FIFO Capability Registers */
No functionality change, just more consistency in the code.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 2ee414600f779..79c32d948b2de 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -625,7 +625,6 @@ static bool hda_check_ipc_irq(struct snd_sof_dev *sdev)
void hda_ipc_irq_dump(struct snd_sof_dev *sdev) { - struct hdac_bus *bus = sof_to_bus(sdev); u32 adspis; u32 intsts; u32 intctl; @@ -637,7 +636,7 @@ void hda_ipc_irq_dump(struct snd_sof_dev *sdev) intsts = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTSTS); intctl = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTCTL); ppsts = snd_sof_dsp_read(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPSTS); - rirbsts = snd_hdac_chip_readb(bus, RIRBSTS); + rirbsts = snd_sof_dsp_readb(sdev, HDA_DSP_HDA_BAR, AZX_REG_RIRBSTS);
dev_err(sdev->dev, "hda irq intsts 0x%8.8x intlctl 0x%8.8x rirb %2.2x\n", intsts, intctl, rirbsts);
No functionality change, only code consistency.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-stream.c | 4 ++-- sound/soc/sof/intel/hda.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c index 6d130b8028b1c..1a39178cbf20f 100644 --- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -398,7 +398,6 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st struct snd_dma_buffer *dmab, struct snd_pcm_hw_params *params) { - struct hdac_bus *bus = sof_to_bus(sdev); struct hdac_stream *hstream = &hext_stream->hstream; int sd_offset = SOF_STREAM_SD_OFFSET(hstream); int ret; @@ -456,7 +455,8 @@ int hda_dsp_iccmax_stream_hw_params(struct snd_sof_dev *sdev, struct hdac_ext_st mask, mask);
/* Follow HW recommendation to set the guardband value to 95us during FW boot */ - snd_hdac_chip_updateb(bus, VS_LTRP, HDA_VS_INTEL_LTRP_GB_MASK, HDA_LTRP_GB_VALUE_US); + snd_sof_dsp_updateb(sdev, HDA_DSP_HDA_BAR, HDA_VS_INTEL_LTRP, + HDA_VS_INTEL_LTRP_GB_MASK, HDA_LTRP_GB_VALUE_US);
/* start DMA */ snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, sd_offset, diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index b2a785d0f0e94..17ed7e60cae81 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -307,6 +307,7 @@ /* Intel Vendor Specific Registers */ #define HDA_VS_INTEL_EM2 0x1030 #define HDA_VS_INTEL_EM2_L1SEN BIT(13) +#define HDA_VS_INTEL_LTRP 0x1048 #define HDA_VS_INTEL_LTRP_GB_MASK 0x3F
/* HIPCI */
readb/writeb are used directly without any wrappers or references to the BAR, as usually done for other registers.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Bard Liao yung-chuan.liao@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-stream.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c index 1a39178cbf20f..c858f30c08f9b 100644 --- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -736,11 +736,11 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)
list_for_each_entry(s, &bus->stream_list, list) { if (status & BIT(s->index) && s->opened) { - sd_status = snd_hdac_stream_readb(s, SD_STS); + sd_status = readb(s->sd_addr + SOF_HDA_ADSP_REG_SD_STS);
trace_sof_intel_hda_dsp_stream_status(bus->dev, s, sd_status);
- snd_hdac_stream_writeb(s, SD_STS, sd_status); + writeb(sd_status, s->sd_addr + SOF_HDA_ADSP_REG_SD_STS);
active = true; if ((!s->substream && !s->cstream) ||
On Mon, 24 Oct 2022 11:52:54 -0500, Pierre-Louis Bossart wrote:
This is the part1 of my HDaudio cleanups, before the addition of to-be-announced HDaudio extensions.
The patchset includes more consistent use of read/write/update helpers, removal of useless waits, structure members and programming sequences, removal of confusing sharing of private_data between FE and BE.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[01/16] ASoC: SOF: ops: fallback to mmio in helpers commit: 01278cb6fa82083000a9e0b56c8b004caf5b6a73 [02/16] ASoC: SOF: Intel: use mmio fallback for all platforms commit: 42b00e9da59f2220bb2a052b72ff1463c8c4ca2c [03/16] ASoC: SOF: ops: add readb/writeb helpers commit: 74fe0c4dcb41678543915cb97928c366ac1aaceb [04/16] ASoC: SOF: ops: add snd_sof_dsp_updateb() helper commit: c28a36b012f1fed177e787d242c592017d284538 [05/16] ASoC: SOF: Intel: hda-dsp: use SOF helpers for consistency commit: 33ac4ca758b80421640cb5edb08b531f5be3da20 [06/16] ASoC: SOF: Intel: hda-dai: start removing the use of runtime->private_data in BE commit: 0351a9b8f8447935e67b98829c1ad287de426f7e [07/16] ASoC: SOF: Intel: hda-dai: use component_get_drvdata to find hdac_bus commit: 4842f79f8fdd9a5aae3d5db98ab3e3a36a387cfd [08/16] ASoC: SOF: Intel: hda-dai: remove useless members in hda_pipe_params commit: 8d44a4fceeb073ee325b6ad91f7a617b9290f8ce [09/16] ASoC: SOF: Intel: hda-ctrl: remove useless sleep commit: a09d82ce0a86772a6bbfe118414708957ed1a5b1 [10/16] ASoC: SOF: Intel: hda: always do a full reset commit: b48b77d836cac43a5bce4f4a1f5e9f8f6e9b1da4 [11/16] ASoC: SOF: Intel: hda: remove useless check on GCTL commit: be4156a25dfa34c0cb2ab9e02b8a085ff986e9ec [12/16] ASoC: SOF: Intel: hda-stream: use SOF helpers for consistency commit: d66149dc0fc2b72f5f2d5050d529f5a7f212700d [13/16] ASoC: SOF: Intel: hda-stream: rename CL_SD_CTL registers as SD_CTL commit: 38bf07805955bc16ba436c9d822df43e6b4a8fa6 [14/16] ASoC: SOF: Intel: hda: use SOF helper for consistency commit: e1e71c60eed6cb057d8ded6bb543f48c32b1e029 [15/16] ASoC: SOF: Intel: hda-stream: use snd_sof_dsp_updateb() helper commit: 847fd278610dda8568e1633b80abd56e08de5690 [16/16] ASoC: SOF: Intel: hda-stream: use readb/writeb for stream registers commit: 3d824ceb8a9cd3d9947767d2ae0231f483a5bf8d
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Mark Brown -
Nathan Chancellor -
Pierre-Louis Bossart