On 2019-08-23 20:24, Pierre-Louis Bossart wrote:

On 8/22/19 2:03 PM, Cezary Rojewski wrote:

...

Implement interface for retrieving firmware configuration. Skylake driver will use this data instead of hardcoded values in updates to come.

Most params are currently unused. In time driver dependency on fw config will increase, and with it, more parsing will be unveiled.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/skl-sst-ipc.c | 122 ++++++++++++++++++++++++++   sound/soc/intel/skylake/skl-sst-ipc.h |  72 +++++++++++++++   sound/soc/intel/skylake/skl.h         |   1 +   3 files changed, 195 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index 667cdddc289f..e9e11ec4c97b 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -11,6 +11,7 @@   #include "skl.h"   #include "skl-sst-dsp.h"   #include "skl-sst-ipc.h" +#include "skl-topology.h"   #include "sound/hdaudio_ext.h" @@ -1067,3 +1068,124 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc *ipc, struct skl_ipc_d0ix_msg *msg)       return ret;   }   EXPORT_SYMBOL_GPL(skl_ipc_set_d0ix);

+int skl_ipc_fw_cfg_get(struct sst_generic_ipc *ipc, struct skl_fw_cfg *cfg) +{ +    struct skl_ipc_large_config_msg msg = {0}; +    struct skl_tlv *tlv; +    size_t bytes = 0, offset = 0; +    u8 *payload = NULL; +    int ret;

+    msg.module_id = 0; +    msg.instance_id = 0; +    msg.large_param_id = SKL_BASEFW_FIRMWARE_CONFIG;

+    ret = skl_ipc_get_large_config(ipc, &msg, (u32 **)&payload, &bytes); +    if (ret) +        goto exit;

+    while (offset < bytes) { +        tlv = (struct skl_tlv *)(payload + offset);

+        switch (tlv->type) { +        case SKL_FW_CFG_FW_VERSION: +            memcpy(&cfg->fw_version, tlv->value, +                sizeof(cfg->fw_version)); +            break;

+        case SKL_FW_CFG_MEMORY_RECLAIMED: +            cfg->memory_reclaimed = *tlv->value; +            break;

+        case SKL_FW_CFG_SLOW_CLOCK_FREQ_HZ: +            cfg->slow_clock_freq_hz = *tlv->value; +            break;

+        case SKL_FW_CFG_FAST_CLOCK_FREQ_HZ: +            cfg->fast_clock_freq_hz = *tlv->value; +            break;

+        case SKL_FW_CFG_ALH_SUPPORT_LEVEL: +            cfg->alh_support = *tlv->value; +            break;

+        case SKL_FW_CFG_IPC_DL_MAILBOX_BYTES: +            cfg->ipc_dl_mailbox_bytes = *tlv->value; +            break;

+        case SKL_FW_CFG_IPC_UL_MAILBOX_BYTES: +            cfg->ipc_ul_mailbox_bytes = *tlv->value; +            break;

+        case SKL_FW_CFG_TRACE_LOG_BYTES: +            cfg->trace_log_bytes = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_PPL_COUNT: +            cfg->max_ppl_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_ASTATE_COUNT: +            cfg->max_astate_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_MODULE_PIN_COUNT: +            cfg->max_module_pin_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MODULES_COUNT: +            cfg->modules_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_MOD_INST_COUNT: +            cfg->max_mod_inst_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT: +            cfg->max_ll_tasks_per_pri_count = *tlv->value; +            break;

+        case SKL_FW_CFG_LL_PRI_COUNT: +            cfg->ll_pri_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_DP_TASKS_COUNT: +            cfg->max_dp_tasks_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_LIBS_COUNT: +            cfg->max_libs_count = *tlv->value; +            break;

+        case SKL_FW_CFG_XTAL_FREQ_HZ: +            cfg->xtal_freq_hz = *tlv->value; +            break;

+        case SKL_FW_CFG_UAOL_SUPPORT: +            cfg->uaol_support = *tlv->value; +            break;

+        case SKL_FW_CFG_POWER_GATING_POLICY: +            cfg->power_gating_policy = *tlv->value; +            break;

+        case SKL_FW_CFG_DMA_BUFFER_CONFIG: +        case SKL_FW_CFG_SCHEDULER_CONFIG: +        case SKL_FW_CFG_CLOCKS_CONFIG: +            break;

+        default: +            dev_info(ipc->dev, "Unrecognized fw param: %d\n", +                tlv->type); +            break;

Isn't this an error? If there are other possible values, why not list them and skip them, as done above?

Pretty sure I cannot share names for all capabilities as these are EMBARGOed. Moreover, both FW_CFG and HW_CFG and constantly being updated and thus new constants are appended. I find it best to "break" when encountering known and un-EMBARGOED constant and simply dump info value if the opposite is true.

New capabilities are always tied to newer platforms. If there will be a need for adding them, these won't be even present in SKL/KBL and such FWs. At the same time if given binary drop contains something new but not required to be handled by host in generic case, dumping error is counter-intuitive.

...

+        }

+        offset += sizeof(*tlv) + tlv->length; +    }

+exit: +    kfree(payload); +    return ret; +} +EXPORT_SYMBOL_GPL(skl_ipc_fw_cfg_get);

...

+enum skl_alh_support_level { +    ALH_NO_SUPPORT = 0x00000, +    ALH_CAVS_1_8_CNL = 0x10000, +};

Support for ALH hasn't changed even past 1.8, and references to CNL are probably not needed.

These are FW types and are here to be left untouched. Ensures parsed values on host side match FW side.

On 2019-08-26 18:27, Pierre-Louis Bossart wrote:

On 8/24/19 4:17 AM, Cezary Rojewski wrote:

...

On 2019-08-23 20:24, Pierre-Louis Bossart wrote:

...

On 8/22/19 2:03 PM, Cezary Rojewski wrote:

...

Implement interface for retrieving firmware configuration. Skylake driver will use this data instead of hardcoded values in updates to come.

Most params are currently unused. In time driver dependency on fw config will increase, and with it, more parsing will be unveiled.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/skl-sst-ipc.c | 122 ++++++++++++++++++++++++++   sound/soc/intel/skylake/skl-sst-ipc.h |  72 +++++++++++++++   sound/soc/intel/skylake/skl.h         |   1 +   3 files changed, 195 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index 667cdddc289f..e9e11ec4c97b 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -11,6 +11,7 @@   #include "skl.h"   #include "skl-sst-dsp.h"   #include "skl-sst-ipc.h" +#include "skl-topology.h"   #include "sound/hdaudio_ext.h" @@ -1067,3 +1068,124 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc *ipc, struct skl_ipc_d0ix_msg *msg)       return ret;   }   EXPORT_SYMBOL_GPL(skl_ipc_set_d0ix);

+int skl_ipc_fw_cfg_get(struct sst_generic_ipc *ipc, struct skl_fw_cfg *cfg) +{ +    struct skl_ipc_large_config_msg msg = {0}; +    struct skl_tlv *tlv; +    size_t bytes = 0, offset = 0; +    u8 *payload = NULL; +    int ret;

+    msg.module_id = 0; +    msg.instance_id = 0; +    msg.large_param_id = SKL_BASEFW_FIRMWARE_CONFIG;

+    ret = skl_ipc_get_large_config(ipc, &msg, (u32 **)&payload, &bytes); +    if (ret) +        goto exit;

+    while (offset < bytes) { +        tlv = (struct skl_tlv *)(payload + offset);

+        switch (tlv->type) { +        case SKL_FW_CFG_FW_VERSION: +            memcpy(&cfg->fw_version, tlv->value, +                sizeof(cfg->fw_version)); +            break;

+        case SKL_FW_CFG_MEMORY_RECLAIMED: +            cfg->memory_reclaimed = *tlv->value; +            break;

+        case SKL_FW_CFG_SLOW_CLOCK_FREQ_HZ: +            cfg->slow_clock_freq_hz = *tlv->value; +            break;

+        case SKL_FW_CFG_FAST_CLOCK_FREQ_HZ: +            cfg->fast_clock_freq_hz = *tlv->value; +            break;

+        case SKL_FW_CFG_ALH_SUPPORT_LEVEL: +            cfg->alh_support = *tlv->value; +            break;

+        case SKL_FW_CFG_IPC_DL_MAILBOX_BYTES: +            cfg->ipc_dl_mailbox_bytes = *tlv->value; +            break;

+        case SKL_FW_CFG_IPC_UL_MAILBOX_BYTES: +            cfg->ipc_ul_mailbox_bytes = *tlv->value; +            break;

+        case SKL_FW_CFG_TRACE_LOG_BYTES: +            cfg->trace_log_bytes = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_PPL_COUNT: +            cfg->max_ppl_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_ASTATE_COUNT: +            cfg->max_astate_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_MODULE_PIN_COUNT: +            cfg->max_module_pin_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MODULES_COUNT: +            cfg->modules_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_MOD_INST_COUNT: +            cfg->max_mod_inst_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT: +            cfg->max_ll_tasks_per_pri_count = *tlv->value; +            break;

+        case SKL_FW_CFG_LL_PRI_COUNT: +            cfg->ll_pri_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_DP_TASKS_COUNT: +            cfg->max_dp_tasks_count = *tlv->value; +            break;

+        case SKL_FW_CFG_MAX_LIBS_COUNT: +            cfg->max_libs_count = *tlv->value; +            break;

+        case SKL_FW_CFG_XTAL_FREQ_HZ: +            cfg->xtal_freq_hz = *tlv->value; +            break;

+        case SKL_FW_CFG_UAOL_SUPPORT: +            cfg->uaol_support = *tlv->value; +            break;

+        case SKL_FW_CFG_POWER_GATING_POLICY: +            cfg->power_gating_policy = *tlv->value; +            break;

+        case SKL_FW_CFG_DMA_BUFFER_CONFIG: +        case SKL_FW_CFG_SCHEDULER_CONFIG: +        case SKL_FW_CFG_CLOCKS_CONFIG: +            break;

+        default: +            dev_info(ipc->dev, "Unrecognized fw param: %d\n", +                tlv->type); +            break;

Isn't this an error? If there are other possible values, why not list them and skip them, as done above?

Pretty sure I cannot share names for all capabilities as these are EMBARGOed. Moreover, both FW_CFG and HW_CFG and constantly being updated and thus new constants are appended. I find it best to "break" when encountering known and un-EMBARGOED constant and simply dump info value if the opposite is true.

New capabilities are always tied to newer platforms. If there will be a need for adding them, these won't be even present in SKL/KBL and such FWs. At the same time if given binary drop contains something new but not required to be handled by host in generic case, dumping error is counter-intuitive.

It goes back to the compatibility issue, if you have a new firmware reporting a capability that your driver cannot handle then it's an error. if you have a new firmware that reports a new capability that can be ignored then it's fine to just skip.

I am just concerned that you have no checks at all.

Still wondering why should host side be concerned about capability they are not going to use at all.

Let's assume FW binaries are being updated regularly. Doubtful developer will remember to append new switch-case every time new FW-cap pops up - requires checking internal FW headers.

In the long run, I do believe the "desynchronization" may happen. I'll recheck with our FW guys what they think about us collapsing at this point (cfg parsing). In general, cAVS specification is very permissive for "outbound" and restrictive against "inbound" data. Parsing goes into "outbound" basket.

...

...

...

+        }

+        offset += sizeof(*tlv) + tlv->length; +    }

+exit: +    kfree(payload); +    return ret; +} +EXPORT_SYMBOL_GPL(skl_ipc_fw_cfg_get);

...

+enum skl_alh_support_level { +    ALH_NO_SUPPORT = 0x00000, +    ALH_CAVS_1_8_CNL = 0x10000, +};

Support for ALH hasn't changed even past 1.8, and references to CNL are probably not needed.

These are FW types and are here to be left untouched. Ensures parsed values on host side match FW side.

Adding a comment would help then.

Agreed, thanks.

• while (offset < bytes) {

• tlv = (struct skl_tlv *)(payload + offset);
  

• switch (tlv->type) {
  

• case SKL_HW_CFG_CAVS_VER:
  

• 	cfg->cavs_version = *tlv->value;
  

• 	break;
  

• case SKL_HW_CFG_DSP_CORES:
  

• 	cfg->dsp_cores = *tlv->value;
  

• 	break;
  

• case SKL_HW_CFG_MEM_PAGE_BYTES:
  

• 	cfg->mem_page_bytes = *tlv->value;
  

• 	break;
  

• case SKL_HW_CFG_TOTAL_PHYS_MEM_PAGES:
  

• 	cfg->total_phys_mem_pages = *tlv->value;
  

• 	break;
  

• case SKL_HW_CFG_I2S_CAPS:
  

• 	cfg->i2s_caps.version = tlv->value[0];
  

• 	size = tlv->value[1];
  

• 	cfg->i2s_caps.ctrl_count = size;
  

• 	if (!size)
  

• 		break;
  

• 	size *= sizeof(*cfg->i2s_caps.ctrl_base_addr);
  

• 	cfg->i2s_caps.ctrl_base_addr =
  

• 		kmemdup(&tlv->value[2], size, GFP_KERNEL);
  

shouldn't the size be that of the source buffer instead of the destination?

• 	if (!cfg->i2s_caps.ctrl_base_addr) {
  

• 		ret = -ENOMEM;
  

• 		goto exit;
  

• 	}
  

• 	break;
  

• case SKL_HW_CFG_GATEWAY_COUNT:
  

• 	cfg->gateway_count = *tlv->value;
  

• 	break;
  

• case SKL_HW_CFG_HP_EBB_COUNT:
  

• 	cfg->hp_ebb_count = *tlv->value;
  

• 	break;
  

• case SKL_HW_CFG_LP_EBB_COUNT:
  

• 	cfg->lp_ebb_count = *tlv->value;
  

• 	break;
  

• case SKL_HW_CFG_EBB_SIZE_BYTES:
  

• 	cfg->ebb_size_bytes = *tlv->value;
  

• 	break;
  

• case SKL_HW_CFG_GPDMA_CAPS:
  

• case SKL_HW_CFG_UAOL_CAPS:
  

• 	break;
  

• default:
  

• 	dev_info(ipc->dev, "Unrecognized hw param: %d\n",
  

• 		tlv->type);
  

• 	break;
  

same feedback, it's usually better to list all values and skip them, and fail big if you see something unexpected.

• }
  

• offset += sizeof(*tlv) + tlv->length;
  

• }

+exit:

• kfree(payload);
• return ret;

+} +EXPORT_SYMBOL_GPL(skl_ipc_hw_cfg_get); diff --git a/sound/soc/intel/skylake/skl-sst-ipc.h b/sound/soc/intel/skylake/skl-sst-ipc.h index ebc5852e15d0..eefa52f7f97a 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.h +++ b/sound/soc/intel/skylake/skl-sst-ipc.h @@ -77,6 +77,7 @@ enum skl_basefw_runtime_param { SKL_BASEFW_ASTATE_TABLE = 4, SKL_BASEFW_DMA_CONTROL = 5, SKL_BASEFW_FIRMWARE_CONFIG = 7,

• SKL_BASEFW_HARDWARE_CONFIG = 8, };

  enum skl_fw_cfg_params {

@@ -141,6 +142,50 @@ struct skl_fw_cfg { u32 power_gating_policy; };

+enum skl_hw_cfg_params {

• SKL_HW_CFG_CAVS_VER,
• SKL_HW_CFG_DSP_CORES,
• SKL_HW_CFG_MEM_PAGE_BYTES,
• SKL_HW_CFG_TOTAL_PHYS_MEM_PAGES,
• SKL_HW_CFG_I2S_CAPS,
• SKL_HW_CFG_GPDMA_CAPS,
• SKL_HW_CFG_GATEWAY_COUNT,
• SKL_HW_CFG_HP_EBB_COUNT,
• SKL_HW_CFG_LP_EBB_COUNT,
• SKL_HW_CFG_EBB_SIZE_BYTES,
• SKL_HW_CFG_UAOL_CAPS

+};

+enum skl_cavs_version {

• SKL_CAVS_VER_1_5 = 0x10005,
• SKL_CAVS_VER_1_8 = 0x10008,

+};

+enum skl_i2s_version {

• SKL_I2S_VER_15_SKYLAKE = 0x00000,
• SKL_I2S_VER_15_BROXTON = 0x10000,
• SKL_I2S_VER_15_BROXTON_P = 0x20000,
• SKL_I2S_VER_18_KBL_CNL = 0x30000,

+};

The encoding is odd. Do these values mean anything (e.g. tied to firmware definitions?)

@@ -203,13 +192,15 @@ struct sst_pdata {

/* DSP */ u32 id;

• const char *fw_name;
• struct sst_ops *ops;
• struct snd_soc_acpi_mach *boards; void *dsp; };

You already have the fw_name inside of struct snd_soc_acpi_mach:

/* Descriptor for SST ASoC machine driver */ struct snd_soc_acpi_mach { const u8 id[ACPI_ID_LEN]; const char *drv_name; const char *fw_filename; const char *board; struct snd_soc_acpi_mach * (*machine_quirk)(void *arg); const void *quirk_data; void *pdata; struct snd_soc_acpi_mach_params mach_params; const char *sof_fw_filename; const char *sof_tplg_filename; };

On 2019-08-23 21:07, Pierre-Louis Bossart wrote:

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

...

Skylake is sst_dsp descendant. Rather than bypassing framework's flow, embrace it. sst_dsp_free invokes sst specific handler internally so nothing is missed.

for clarity you should also add the free_irq is also handled internally in sst_dsp_free(), otherwise this looks like a mistake to the reviewer that's not even half-way through the series...

Noted, thanks. Indeed this is done to reuse sst_dsp_free while preventing double free_irq from occurring.

...

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/cnl-sst-dsp.c | 1 -   sound/soc/intel/skylake/skl-sst-dsp.c | 1 -   sound/soc/intel/skylake/skl-sst.c     | 2 +-   3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/intel/skylake/cnl-sst-dsp.c b/sound/soc/intel/skylake/cnl-sst-dsp.c index 189c1c7086e3..48b465939ef8 100644 --- a/sound/soc/intel/skylake/cnl-sst-dsp.c +++ b/sound/soc/intel/skylake/cnl-sst-dsp.c @@ -215,7 +215,6 @@ void cnl_dsp_free(struct sst_dsp *dsp)       sst_ipc_fini(&skl->ipc);       cnl_ipc_int_disable(dsp); -    free_irq(dsp->irq, dsp);       cnl_dsp_disable_core(dsp, SKL_DSP_CORE0_MASK);   }   EXPORT_SYMBOL_GPL(cnl_dsp_free); diff --git a/sound/soc/intel/skylake/skl-sst-dsp.c b/sound/soc/intel/skylake/skl-sst-dsp.c index 348e69226e46..1c4ecbcd7db7 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.c +++ b/sound/soc/intel/skylake/skl-sst-dsp.c @@ -468,7 +468,6 @@ void skl_dsp_free(struct sst_dsp *dsp)       sst_ipc_fini(&skl->ipc);       skl_ipc_int_disable(dsp); -    free_irq(dsp->irq, dsp);       skl_dsp_disable_core(dsp, SKL_DSP_CORE0_MASK);   }   EXPORT_SYMBOL_GPL(skl_dsp_free); diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 72ee3d8a1d7d..e55523826346 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -622,7 +622,7 @@ void skl_sst_dsp_cleanup(struct skl_dev *skl)       skl_clear_module_table(dsp);       skl_freeup_uuid_list(skl); -    dsp->ops->free(dsp); +    sst_dsp_free(dsp);       if (skl->boot_complete && dsp->cl_dev.bufsize) {           dsp->cl_dev.ops.cl_cleanup_controller(dsp);

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

skl_dsp_ctx_init is dumplication of sst_dsp_new and usage of such

typo: duplication.

bypasses natural DSP framework's flow. Remove it and reuse sst_dsp_new constructor which invokes sst specific init internally so nothing is missed.

Skylake does not even define any sst_ops::init so portion of existing skl_dsp_ctx_init can be regarded as DEADCODE.

this is also hard to review, you have lines like

• return skl_dsp_acquire_irq(sst);

• return 0;

that seem like a different change than what is described here.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c | 2 +- sound/soc/intel/skylake/cnl-sst.c | 2 +- sound/soc/intel/skylake/skl-sst-dsp.c | 28 ------------------------- sound/soc/intel/skylake/skl-sst-dsp.h | 2 -- sound/soc/intel/skylake/skl-sst-utils.c | 6 +++++- sound/soc/intel/skylake/skl-sst.c | 2 +- 6 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index e3614acff34d..a8a2783f9b37 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -588,7 +588,7 @@ int bxt_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, INIT_DELAYED_WORK(&skl->d0i3.work, bxt_set_dsp_D0i3); skl->d0i3.state = SKL_DSP_D0I3_NONE;

• return skl_dsp_acquire_irq(sst);

• return 0; } EXPORT_SYMBOL_GPL(bxt_sst_dsp_init);

diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 84dc6b82831d..0b0337f6ebff 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -460,7 +460,7 @@ int cnl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, cnl->boot_complete = false; init_waitqueue_head(&cnl->boot_wait);

• return skl_dsp_acquire_irq(sst);

• return 0; } EXPORT_SYMBOL_GPL(cnl_sst_dsp_init);

diff --git a/sound/soc/intel/skylake/skl-sst-dsp.c b/sound/soc/intel/skylake/skl-sst-dsp.c index 1c4ecbcd7db7..9d8eb1af4798 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.c +++ b/sound/soc/intel/skylake/skl-sst-dsp.c @@ -418,34 +418,6 @@ int skl_dsp_sleep(struct sst_dsp *ctx) } EXPORT_SYMBOL_GPL(skl_dsp_sleep);

-struct sst_dsp *skl_dsp_ctx_init(struct device *dev,

• struct sst_pdata *pdata, int irq)
  

-{

• int ret;
• struct sst_dsp *sst;
• sst = devm_kzalloc(dev, sizeof(*sst), GFP_KERNEL);
• if (sst == NULL)

• return NULL;
  

• spin_lock_init(&sst->spinlock);
• mutex_init(&sst->mutex);
• sst->dev = dev;
• sst->pdata = pdata;
• sst->irq = irq;
• sst->ops = pdata->ops;
• sst->thread_context = pdata->dsp;
• /* Initialise SST Audio DSP */
• if (sst->ops->init) {

• ret = sst->ops->init(sst, NULL);
  

• if (ret < 0)
  

• 	return NULL;
  

• }
• return sst;

-}

• int skl_dsp_acquire_irq(struct sst_dsp *sst) { int ret;

diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index ba37433e4efa..1d579d59de60 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -196,8 +196,6 @@ int skl_cldma_prepare(struct sst_dsp *ctx); int skl_cldma_wait_interruptible(struct sst_dsp *ctx);

void skl_dsp_set_state_locked(struct sst_dsp *ctx, int state); -struct sst_dsp *skl_dsp_ctx_init(struct device *dev,

• struct sst_pdata *pdata, int irq);
  

  int skl_dsp_acquire_irq(struct sst_dsp *sst); bool is_skl_dsp_running(struct sst_dsp *ctx);

diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c index 9061a9b17ea0..8e03a10855c4 100644 --- a/sound/soc/intel/skylake/skl-sst-utils.c +++ b/sound/soc/intel/skylake/skl-sst-utils.c @@ -6,6 +6,7 @@ */

#include <linux/device.h> +#include <linux/pci.h> #include <linux/slab.h> #include <linux/uuid.h> #include "../common/sst-dsp.h" @@ -360,10 +361,13 @@ int skl_sst_ctx_init(struct device *dev, int irq, const char *fw_name, struct skl_dev *skl = *dsp; struct sst_dsp *sst;

• pdata->id = skl->pci->device;
• pdata->irq = irq;
• pdata->resindex_dma_base = -1; skl->dev = dev; pdata->dsp = skl; INIT_LIST_HEAD(&skl->uuid_list);

• skl->dsp = skl_dsp_ctx_init(dev, pdata, irq);

• skl->dsp = sst_dsp_new(dev, pdata); if (!skl->dsp) { dev_err(skl->dev, "%s: no device\n", __func__); return -ENODEV;

diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index e55523826346..6bb003add9e2 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -550,7 +550,7 @@ int skl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,

sst->fw_ops = skl_fw_ops;

• return skl_dsp_acquire_irq(sst);

• return 0; } EXPORT_SYMBOL_GPL(skl_sst_dsp_init);

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

None of skl_dsp_loader_ops are actually extended as any parameter that could be "extended" is already part of given function's parameter list. Rather than obfustace non-derived calls with ops and dereferences, make

A typo on obfuscate could be intentional?

use of said operation directly. Takes part in remal of

removal?

skl_dsp_loader_ops structure.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c | 18 +++++++++--------- sound/soc/intel/skylake/cnl-sst.c | 10 +++++----- sound/soc/intel/skylake/skl-messages.c | 10 +++++----- sound/soc/intel/skylake/skl-sst-cldma.c | 10 +++++----- sound/soc/intel/skylake/skl-sst-dsp.h | 9 +++++++++ 5 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index a8a2783f9b37..1ca4fba0f35f 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -60,7 +60,7 @@ bxt_load_library(struct sst_dsp *ctx, struct skl_lib_info *linfo, int lib_count) if (ret < 0) goto load_library_failed;

• stream_tag = ctx->dsp_ops.prepare(ctx->dev, 0x40,
  

• stream_tag = skl_dsp_prepare(ctx->dev, 0x40,
  		stripped_fw.size, &dmab);
  

fits on one line now?

if (stream_tag <= 0) {
	dev_err(ctx->dev, "Lib prepare DMA err: %x\n",

@@ -72,14 +72,14 @@ bxt_load_library(struct sst_dsp *ctx, struct skl_lib_info *linfo, int lib_count) dma_id = stream_tag - 1; memcpy(dmab.area, stripped_fw.data, stripped_fw.size);

• ctx->dsp_ops.trigger(ctx->dev, true, stream_tag);
  

• skl_dsp_trigger(ctx->dev, true, stream_tag);
  

  ret = skl_sst_ipc_load_library(&skl->ipc, dma_id, i, true); if (ret < 0) dev_err(ctx->dev, "IPC Load Lib for %s fail: %d\n", linfo[i].name, ret);

indent?

• ctx->dsp_ops.trigger(ctx->dev, false, stream_tag);
  

• ctx->dsp_ops.cleanup(ctx->dev, &dmab, stream_tag);
  

• skl_dsp_trigger(ctx->dev, false, stream_tag);
  

• skl_dsp_cleanup(ctx->dev, &dmab, stream_tag);
  

  }

  return ret;

@@ -100,7 +100,7 @@ static int sst_bxt_prepare_fw(struct sst_dsp *ctx, { int stream_tag, ret;

• stream_tag = ctx->dsp_ops.prepare(ctx->dev, 0x40, fwsize, &ctx->dmab);

• stream_tag = skl_dsp_prepare(ctx->dev, 0x40, fwsize, &ctx->dmab); if (stream_tag <= 0) { dev_err(ctx->dev, "Failed to prepare DMA FW loading err: %x\n", stream_tag);

@@ -162,7 +162,7 @@ static int sst_bxt_prepare_fw(struct sst_dsp *ctx, return ret;

base_fw_load_failed:

• ctx->dsp_ops.cleanup(ctx->dev, &ctx->dmab, stream_tag);

• skl_dsp_cleanup(ctx->dev, &ctx->dmab, stream_tag); skl_dsp_core_power_down(ctx, SKL_DSP_CORE_MASK(1)); skl_dsp_disable_core(ctx, SKL_DSP_CORE0_MASK);

those macros look confusing. COREx_MASK or CORE_MASK(x), choose one.

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

With all fields deprecated, yet another struct can be removed. Let's do so and don't forget about any functions or fields tied to its existence.

SKL/KBL and all others have different firmware loading mechanisms, so now how is the difference handled?

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/common/sst-dsp-priv.h | 1 - sound/soc/intel/skylake/bxt-sst.c | 5 ++-- sound/soc/intel/skylake/cnl-sst-dsp.h | 3 +- sound/soc/intel/skylake/cnl-sst.c | 5 ++-- sound/soc/intel/skylake/skl-messages.c | 40 +------------------------ sound/soc/intel/skylake/skl-sst-dsp.h | 24 ++------------- sound/soc/intel/skylake/skl-sst-utils.c | 3 +- sound/soc/intel/skylake/skl-sst.c | 5 ++-- sound/soc/intel/skylake/skl.h | 2 -- 9 files changed, 12 insertions(+), 76 deletions(-)

diff --git a/sound/soc/intel/common/sst-dsp-priv.h b/sound/soc/intel/common/sst-dsp-priv.h index 8a7009937d59..0fe9bebcfb38 100644 --- a/sound/soc/intel/common/sst-dsp-priv.h +++ b/sound/soc/intel/common/sst-dsp-priv.h @@ -306,7 +306,6 @@ struct sst_dsp { const char *fw_name;

/* To allocate CL dma buffers */

• struct skl_dsp_loader_ops dsp_ops; struct skl_dsp_fw_ops fw_ops; int sst_state; struct skl_cl_dev cl_dev;

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 45da02f74f4d..c7961050c2ee 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -546,14 +546,13 @@ static struct sst_pdata skl_dev = { };

int bxt_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,

• 	const char *fw_name, struct skl_dsp_loader_ops dsp_ops,
  

• 	struct skl_dev **dsp)
  

• 	const char *fw_name, struct skl_dev **dsp)
  

  { struct skl_dev *skl; struct sst_dsp *sst; int ret;

• ret = skl_sst_ctx_init(dev, irq, fw_name, dsp_ops, dsp, &skl_dev);

• ret = skl_sst_ctx_init(dev, irq, fw_name, dsp, &skl_dev); if (ret < 0) { dev_err(dev, "%s: no device\n", __func__); return ret;

diff --git a/sound/soc/intel/skylake/cnl-sst-dsp.h b/sound/soc/intel/skylake/cnl-sst-dsp.h index 30b586acc858..70da4f312f53 100644 --- a/sound/soc/intel/skylake/cnl-sst-dsp.h +++ b/sound/soc/intel/skylake/cnl-sst-dsp.h @@ -94,7 +94,6 @@ void cnl_ipc_op_int_disable(struct sst_dsp *ctx); bool cnl_ipc_int_status(struct sst_dsp *ctx);

int cnl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,

•      const char *fw_name, struct skl_dsp_loader_ops dsp_ops,
  

•      struct skl_dev **dsp);
  

•      const char *fw_name, struct skl_dev **dsp);
  
  

  #endif /*__CNL_SST_DSP_H__*/

diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 4d9a4d5886f4..2f10076cc906 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -423,14 +423,13 @@ static struct sst_pdata cnl_dev = { };

int cnl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,

•      const char *fw_name, struct skl_dsp_loader_ops dsp_ops,
  

•      struct skl_dev **dsp)
  

•      const char *fw_name, struct skl_dev **dsp)
  

  { struct skl_dev *cnl; struct sst_dsp *sst; int ret;

• ret = skl_sst_ctx_init(dev, irq, fw_name, dsp_ops, dsp, &cnl_dev);

• ret = skl_sst_ctx_init(dev, irq, fw_name, dsp, &cnl_dev); if (ret < 0) { dev_err(dev, "%s: no device\n", __func__); return ret;

diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index 20ab980fe8a1..6fbceec67b53 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -140,72 +140,37 @@ int skl_dsp_cleanup(struct device *dev, return 0; }

-static struct skl_dsp_loader_ops skl_get_loader_ops(void) -{

• struct skl_dsp_loader_ops loader_ops;
• memset(&loader_ops, 0, sizeof(struct skl_dsp_loader_ops));
• loader_ops.alloc_dma_buf = skl_alloc_dma_buf;
• loader_ops.free_dma_buf = skl_free_dma_buf;
• return loader_ops;

-};

-static struct skl_dsp_loader_ops bxt_get_loader_ops(void) -{

• struct skl_dsp_loader_ops loader_ops;
• memset(&loader_ops, 0, sizeof(loader_ops));
• loader_ops.alloc_dma_buf = skl_alloc_dma_buf;
• loader_ops.free_dma_buf = skl_free_dma_buf;
• loader_ops.prepare = skl_dsp_prepare;
• loader_ops.trigger = skl_dsp_trigger;
• loader_ops.cleanup = skl_dsp_cleanup;
• return loader_ops;

-};

• static const struct skl_dsp_ops dsp_ops[] = { { .id = 0x9d70,

• .loader_ops = skl_get_loader_ops,
  

  .init = skl_sst_dsp_init, }, { .id = 0x9d71,

• .loader_ops = skl_get_loader_ops,
  

  .init = skl_sst_dsp_init, }, { .id = 0x5a98,

• .loader_ops = bxt_get_loader_ops,
  

  .init = bxt_sst_dsp_init, }, { .id = 0x3198,

• .loader_ops = bxt_get_loader_ops,
  

  .init = bxt_sst_dsp_init, }, { .id = 0x9dc8,

• .loader_ops = bxt_get_loader_ops,
  

  .init = cnl_sst_dsp_init, }, { .id = 0xa348,

• .loader_ops = bxt_get_loader_ops,
  

  .init = cnl_sst_dsp_init, }, { .id = 0x02c8,

• .loader_ops = bxt_get_loader_ops,
  

  .init = cnl_sst_dsp_init, }, { .id = 0x06c8,

• .loader_ops = bxt_get_loader_ops,
  

  .init = cnl_sst_dsp_init, }, };

@@ -226,7 +191,6 @@ int skl_init_dsp(struct skl_dev *skl) { void __iomem *mmio_base; struct hdac_bus *bus = skl_to_bus(skl);

• struct skl_dsp_loader_ops loader_ops; int irq = bus->irq; const struct skl_dsp_ops *ops; int ret;

@@ -248,10 +212,8 @@ int skl_init_dsp(struct skl_dev *skl) goto unmap_mmio; }

• loader_ops = ops->loader_ops(); ret = ops->init(bus->dev, mmio_base, irq,

• 		skl->fw_name, loader_ops,
  

• 		&skl);
  

• 		skl->fw_name, &skl);
  
  

  if (ret < 0) goto unmap_mmio;

diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index 97e16a602331..9f2dae92c1c9 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -150,22 +150,6 @@ struct skl_dsp_fw_ops {

};

-struct skl_dsp_loader_ops {

• int stream_tag;
• int (*alloc_dma_buf)(struct device *dev,

• struct snd_dma_buffer *dmab, size_t size);
  

• int (*free_dma_buf)(struct device *dev,

• struct snd_dma_buffer *dmab);
  

• int (*prepare)(struct device *dev, unsigned int format,

• 		unsigned int byte_size,
  

• 		struct snd_dma_buffer *bufp);
  

• int (*trigger)(struct device *dev, bool start, int stream_tag);
• int (*cleanup)(struct device *dev, struct snd_dma_buffer *dmab,

• 		 int stream_tag);
  

-};

• #define MAX_INSTANCE_BUFF 2

  struct uuid_module {

@@ -227,11 +211,9 @@ int skl_dsp_put_core(struct sst_dsp *ctx, unsigned int core_id);

int skl_dsp_boot(struct sst_dsp *ctx); int skl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,

• const char *fw_name, struct skl_dsp_loader_ops dsp_ops,
  

• struct skl_dev **dsp);
  

• const char *fw_name, struct skl_dev **dsp);
  

  int bxt_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,

• const char *fw_name, struct skl_dsp_loader_ops dsp_ops,
  

• struct skl_dev **dsp);
  

• const char *fw_name, struct skl_dev **dsp);
  

  int bxt_load_library(struct sst_dsp *ctx, struct skl_lib_info *linfo, int lib_count);

@@ -248,7 +230,7 @@ int skl_dsp_strip_extended_manifest(struct firmware *fw); void skl_dsp_set_astate_cfg(struct skl_dev *skl, u32 cnt, void *data);

int skl_sst_ctx_init(struct device *dev, int irq, const char *fw_name,

• struct skl_dsp_loader_ops dsp_ops, struct skl_dev **dsp,
  

• struct skl_dev **dsp,
  

  struct sst_pdata *pdata); int skl_prepare_lib_load(struct skl_dev *skl, struct skl_lib_info *linfo, struct firmware *stripped_fw,

diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c index 8e03a10855c4..bbe67e298efe 100644 --- a/sound/soc/intel/skylake/skl-sst-utils.c +++ b/sound/soc/intel/skylake/skl-sst-utils.c @@ -355,7 +355,7 @@ int skl_dsp_strip_extended_manifest(struct firmware *fw) }

int skl_sst_ctx_init(struct device *dev, int irq, const char *fw_name,

• struct skl_dsp_loader_ops dsp_ops, struct skl_dev **dsp,

• struct skl_dev **dsp, struct sst_pdata *pdata) { struct skl_dev *skl = *dsp;

@@ -375,7 +375,6 @@ int skl_sst_ctx_init(struct device *dev, int irq, const char *fw_name,

sst = skl->dsp; sst->fw_name = fw_name;

• sst->dsp_ops = dsp_ops; init_waitqueue_head(&skl->mod_load_wait); INIT_LIST_HEAD(&sst->module_list);

diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 6bb003add9e2..e9a7b2509b35 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -517,14 +517,13 @@ static struct sst_pdata skl_dev = { };

int skl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,

• const char *fw_name, struct skl_dsp_loader_ops dsp_ops,
  

• struct skl_dev **dsp)
  

• const char *fw_name, struct skl_dev **dsp)
  

  { struct skl_dev *skl; struct sst_dsp *sst; int ret;

• ret = skl_sst_ctx_init(dev, irq, fw_name, dsp_ops, dsp, &skl_dev);

• ret = skl_sst_ctx_init(dev, irq, fw_name, dsp, &skl_dev); if (ret < 0) { dev_err(dev, "%s: no device\n", __func__); return ret;

diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 62e2f2d450e4..beeef67cb55f 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -155,10 +155,8 @@ struct skl_machine_pdata {

struct skl_dsp_ops { int id;

• struct skl_dsp_loader_ops (*loader_ops)(void); int (*init)(struct device *dev, void __iomem *mmio_base, int irq, const char *fw_name,

• 	struct skl_dsp_loader_ops loader_ops,
  struct skl_dev **skl_sst);
  

  };

On 2019-08-23 21:26, Pierre-Louis Bossart wrote:

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

...

w0_stat_sz and w0_up_sz are Skylake-specific fields and should not be part of common sst framework. The latter is also completely unused. Remove both while declaring global FW register-area size, shared for all SKL+ platforms.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/common/sst-dsp-priv.h | 2 --   sound/soc/intel/skylake/bxt-sst.c     | 2 --   sound/soc/intel/skylake/cnl-sst.c     | 2 --   sound/soc/intel/skylake/skl-debug.c   | 2 +-   sound/soc/intel/skylake/skl-sst-dsp.h | 1 +   sound/soc/intel/skylake/skl-sst.c     | 2 --   6 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/sound/soc/intel/common/sst-dsp-priv.h b/sound/soc/intel/common/sst-dsp-priv.h index 0fe9bebcfb38..53dcd87bab44 100644 --- a/sound/soc/intel/common/sst-dsp-priv.h +++ b/sound/soc/intel/common/sst-dsp-priv.h @@ -71,8 +71,6 @@ struct sst_addr {       u32 dsp_dram_offset;       u32 sram0_base;       u32 sram1_base; -    u32 w0_stat_sz; -    u32 w0_up_sz;       void __iomem *lpe;       void __iomem *shim;       void __iomem *pci_cfg; diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index c7961050c2ee..641d5cf4aeb1 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -565,8 +565,6 @@ int bxt_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,       sst->addr.shim = mmio_base;       sst->addr.sram0_base = BXT_ADSP_SRAM0_BASE;       sst->addr.sram1_base = BXT_ADSP_SRAM1_BASE; -    sst->addr.w0_stat_sz = SKL_ADSP_W0_STAT_SZ; -    sst->addr.w0_up_sz = SKL_ADSP_W0_UP_SZ;       sst_dsp_mailbox_init(sst, (BXT_ADSP_SRAM0_BASE + SKL_ADSP_W0_STAT_SZ),               SKL_ADSP_W0_UP_SZ, BXT_ADSP_SRAM1_BASE, SKL_ADSP_W1_SZ);

You are still using a SKL specific macro here...

The following patch takes case of that. This one has only one goal: removal of two redundant fields from sst_dsp::addr (struct sst_addr).

...

diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 2f10076cc906..64971966af38 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -442,8 +442,6 @@ int cnl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,       sst->addr.shim = mmio_base;       sst->addr.sram0_base = CNL_ADSP_SRAM0_BASE;       sst->addr.sram1_base = CNL_ADSP_SRAM1_BASE; -    sst->addr.w0_stat_sz = CNL_ADSP_W0_STAT_SZ; -    sst->addr.w0_up_sz = CNL_ADSP_W0_UP_SZ;       sst_dsp_mailbox_init(sst, (CNL_ADSP_SRAM0_BASE + CNL_ADSP_W0_STAT_SZ),                    CNL_ADSP_W0_UP_SZ, CNL_ADSP_SRAM1_BASE,

...and here...

...

diff --git a/sound/soc/intel/skylake/skl-debug.c b/sound/soc/intel/skylake/skl-debug.c index 212370bf704c..6781eac13232 100644 --- a/sound/soc/intel/skylake/skl-debug.c +++ b/sound/soc/intel/skylake/skl-debug.c @@ -173,7 +173,7 @@ static ssize_t fw_softreg_read(struct file *file, char __user *user_buf,   {       struct skl_debug *d = file->private_data;       struct sst_dsp *sst = d->skl->dsp; -    size_t w0_stat_sz = sst->addr.w0_stat_sz; +    size_t w0_stat_sz = SKL_FW_REGS_SIZE;

but here it's a different macro?

The following is true: [FW_REGS][INBOX] | [OUTBOX] | [TRACES] | [PERF_COUNTERS] - SRAM0 - - SRAM1 - - SRAM2 - - SRAM3 -

INBOX_SIZE == OUTBOX_SIZE == PAGE_SIZE FW_REGS_SIZE == PAGE_SIZE

w0_stat == FW_REGS w0_up == INBOX

So we come up with very simple and readable macros - two of them: - FW_REGS_SIZE - ADSP_MAILBOX_SIZE

and these will suffice.

...

void __iomem *in_base = sst->mailbox.in_base;       void __iomem *fw_reg_addr;       unsigned int offset; diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index 9f2dae92c1c9..5a0cb7f3d57e 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -63,6 +63,7 @@ struct skl_dev;   #define SKL_ADSP_W1_SZ            0x1000 +#define SKL_FW_REGS_SIZE        PAGE_SIZE   #define SKL_FW_STS_MASK            0xf   #define SKL_FW_INIT            0x1 diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index e9a7b2509b35..7e63c91cea54 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -535,8 +535,6 @@ int skl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,       sst->addr.shim = mmio_base;       sst->addr.sram0_base = SKL_ADSP_SRAM0_BASE;       sst->addr.sram1_base = SKL_ADSP_SRAM1_BASE; -    sst->addr.w0_stat_sz = SKL_ADSP_W0_STAT_SZ; -    sst->addr.w0_up_sz = SKL_ADSP_W0_UP_SZ;       sst_dsp_mailbox_init(sst, (SKL_ADSP_SRAM0_BASE + SKL_ADSP_W0_STAT_SZ),               SKL_ADSP_W0_UP_SZ, SKL_ADSP_SRAM1_BASE, SKL_ADSP_W1_SZ);

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

The existing upling, downling and FW register size macros are

uplink and downlink?

duplicates. Remove these and replace by global mailbox size macro - FW register-area size is represented by SKL_FW_REGS_SIZE added in precedding change.

preceding

use checkpatch.pl --codespell to check for typos.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c | 5 +++-- sound/soc/intel/skylake/cnl-sst-dsp.h | 6 ------ sound/soc/intel/skylake/cnl-sst.c | 10 +++++----- sound/soc/intel/skylake/skl-sst-dsp.h | 7 +------ sound/soc/intel/skylake/skl-sst-ipc.c | 12 ++++++------ sound/soc/intel/skylake/skl-sst.c | 5 +++-- 6 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 641d5cf4aeb1..fdd94711c3b4 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -566,8 +566,9 @@ int bxt_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, sst->addr.sram0_base = BXT_ADSP_SRAM0_BASE; sst->addr.sram1_base = BXT_ADSP_SRAM1_BASE;

• sst_dsp_mailbox_init(sst, (BXT_ADSP_SRAM0_BASE + SKL_ADSP_W0_STAT_SZ),

• 	SKL_ADSP_W0_UP_SZ, BXT_ADSP_SRAM1_BASE, SKL_ADSP_W1_SZ);
  

• sst_dsp_mailbox_init(sst,

• (BXT_ADSP_SRAM0_BASE + SKL_FW_REGS_SIZE), SKL_MAILBOX_SIZE,
  

• BXT_ADSP_SRAM1_BASE, SKL_MAILBOX_SIZE);
  
  

  ret = skl_ipc_init(dev, skl); if (ret) {

diff --git a/sound/soc/intel/skylake/cnl-sst-dsp.h b/sound/soc/intel/skylake/cnl-sst-dsp.h index 70da4f312f53..a465cc42b7e8 100644 --- a/sound/soc/intel/skylake/cnl-sst-dsp.h +++ b/sound/soc/intel/skylake/cnl-sst-dsp.h @@ -52,12 +52,6 @@ struct sst_generic_ipc;

#define CNL_ADSP_MMIO_LEN 0x10000

-#define CNL_ADSP_W0_STAT_SZ 0x1000

-#define CNL_ADSP_W0_UP_SZ 0x1000

-#define CNL_ADSP_W1_SZ 0x1000

• #define CNL_FW_STS_MASK 0xf

  #define CNL_ADSPIC_IPC 0x1

diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 64971966af38..d3da23e0ef05 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -390,8 +390,8 @@ static int cnl_ipc_init(struct device *dev, struct skl_dev *cnl) ipc->dsp = cnl->dsp; ipc->dev = dev;

• ipc->tx_data_max_size = CNL_ADSP_W1_SZ;
• ipc->rx_data_max_size = CNL_ADSP_W0_UP_SZ;

• ipc->tx_data_max_size = SKL_MAILBOX_SIZE;

• ipc->rx_data_max_size = SKL_MAILBOX_SIZE;

  err = sst_ipc_init(ipc); if (err)

@@ -443,9 +443,9 @@ int cnl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, sst->addr.sram0_base = CNL_ADSP_SRAM0_BASE; sst->addr.sram1_base = CNL_ADSP_SRAM1_BASE;

• sst_dsp_mailbox_init(sst, (CNL_ADSP_SRAM0_BASE + CNL_ADSP_W0_STAT_SZ),

• 	     CNL_ADSP_W0_UP_SZ, CNL_ADSP_SRAM1_BASE,
  

• 	     CNL_ADSP_W1_SZ);
  

• sst_dsp_mailbox_init(sst,

• (CNL_ADSP_SRAM0_BASE + SKL_FW_REGS_SIZE), SKL_MAILBOX_SIZE,
  

• CNL_ADSP_SRAM1_BASE, SKL_MAILBOX_SIZE);
  
  

  ret = cnl_ipc_init(dev, cnl); if (ret) {

diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index 5a0cb7f3d57e..3c92b1d849e4 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -57,12 +57,7 @@ struct skl_dev;

#define SKL_ADSP_MMIO_LEN 0x10000

-#define SKL_ADSP_W0_STAT_SZ 0x1000

-#define SKL_ADSP_W0_UP_SZ 0x1000

-#define SKL_ADSP_W1_SZ 0x1000

+#define SKL_MAILBOX_SIZE PAGE_SIZE #define SKL_FW_REGS_SIZE PAGE_SIZE #define SKL_FW_STS_MASK 0xf

diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index 2700f882103d..72d7284d2fff 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -606,8 +606,8 @@ int skl_ipc_init(struct device *dev, struct skl_dev *skl) ipc->dsp = skl->dsp; ipc->dev = dev;

• ipc->tx_data_max_size = SKL_ADSP_W1_SZ;
• ipc->rx_data_max_size = SKL_ADSP_W0_UP_SZ;

• ipc->tx_data_max_size = SKL_MAILBOX_SIZE;

• ipc->rx_data_max_size = SKL_MAILBOX_SIZE;

  err = sst_ipc_init(ipc); if (err)

@@ -922,8 +922,8 @@ int skl_ipc_set_large_config(struct sst_generic_ipc *ipc, sz_remaining = msg->param_data_size; data_offset = 0; while (sz_remaining != 0) {

• tx_size = sz_remaining > SKL_ADSP_W1_SZ
  

• 		? SKL_ADSP_W1_SZ : sz_remaining;
  

• tx_size = sz_remaining > SKL_MAILBOX_SIZE
  

• 		? SKL_MAILBOX_SIZE : sz_remaining;
  

  if (tx_size == sz_remaining) header.extension |= IPC_FINAL_BLOCK(1);

@@ -965,7 +965,7 @@ int skl_ipc_get_large_config(struct sst_generic_ipc *ipc, unsigned int *buf; int ret;

• reply.data = kzalloc(SKL_ADSP_W1_SZ, GFP_KERNEL);

• reply.data = kzalloc(SKL_MAILBOX_SIZE, GFP_KERNEL); if (!reply.data) return -ENOMEM;

@@ -983,7 +983,7 @@ int skl_ipc_get_large_config(struct sst_generic_ipc *ipc, request.header = *(u64 *)&header; request.data = *payload; request.size = *bytes;

• reply.size = SKL_ADSP_W1_SZ;

• reply.size = SKL_MAILBOX_SIZE;

  ret = sst_ipc_tx_message_wait(ipc, request, &reply); if (ret < 0)

diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 7e63c91cea54..191931578fd2 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -536,8 +536,9 @@ int skl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, sst->addr.sram0_base = SKL_ADSP_SRAM0_BASE; sst->addr.sram1_base = SKL_ADSP_SRAM1_BASE;

• sst_dsp_mailbox_init(sst, (SKL_ADSP_SRAM0_BASE + SKL_ADSP_W0_STAT_SZ),

• 	SKL_ADSP_W0_UP_SZ, SKL_ADSP_SRAM1_BASE, SKL_ADSP_W1_SZ);
  

• sst_dsp_mailbox_init(sst,

• (SKL_ADSP_SRAM0_BASE + SKL_FW_REGS_SIZE), SKL_MAILBOX_SIZE,
  

• SKL_ADSP_SRAM1_BASE, SKL_MAILBOX_SIZE);
  
  

  ret = skl_ipc_init(dev, skl); if (ret) {

On 2019-08-24 13:52, Cezary Rojewski wrote:

On 2019-08-23 21:28, Pierre-Louis Bossart wrote:

...

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

...

The existing upling, downling and FW register size macros are

uplink and downlink?

...

duplicates. Remove these and replace by global mailbox size macro - FW register-area size is represented by SKL_FW_REGS_SIZE added in precedding change.

preceding

use checkpatch.pl --codespell to check for typos.

Ack on both. Sorry for the seer amount of typos in commit messages in this series. Thanks for catching them up, Pierre.

s/seer/sheer This is escalating quickly..

...

...

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c     |  5 +++--   sound/soc/intel/skylake/cnl-sst-dsp.h |  6 ------   sound/soc/intel/skylake/cnl-sst.c     | 10 +++++-----   sound/soc/intel/skylake/skl-sst-dsp.h |  7 +------   sound/soc/intel/skylake/skl-sst-ipc.c | 12 ++++++------   sound/soc/intel/skylake/skl-sst.c     |  5 +++--   6 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 641d5cf4aeb1..fdd94711c3b4 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -566,8 +566,9 @@ int bxt_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,       sst->addr.sram0_base = BXT_ADSP_SRAM0_BASE;       sst->addr.sram1_base = BXT_ADSP_SRAM1_BASE; -    sst_dsp_mailbox_init(sst, (BXT_ADSP_SRAM0_BASE + SKL_ADSP_W0_STAT_SZ), -            SKL_ADSP_W0_UP_SZ, BXT_ADSP_SRAM1_BASE, SKL_ADSP_W1_SZ); +    sst_dsp_mailbox_init(sst, +        (BXT_ADSP_SRAM0_BASE + SKL_FW_REGS_SIZE), SKL_MAILBOX_SIZE, +        BXT_ADSP_SRAM1_BASE, SKL_MAILBOX_SIZE);       ret = skl_ipc_init(dev, skl);       if (ret) { diff --git a/sound/soc/intel/skylake/cnl-sst-dsp.h b/sound/soc/intel/skylake/cnl-sst-dsp.h index 70da4f312f53..a465cc42b7e8 100644 --- a/sound/soc/intel/skylake/cnl-sst-dsp.h +++ b/sound/soc/intel/skylake/cnl-sst-dsp.h @@ -52,12 +52,6 @@ struct sst_generic_ipc;   #define CNL_ADSP_MMIO_LEN        0x10000 -#define CNL_ADSP_W0_STAT_SZ        0x1000

-#define CNL_ADSP_W0_UP_SZ        0x1000

-#define CNL_ADSP_W1_SZ            0x1000

#define CNL_FW_STS_MASK            0xf   #define CNL_ADSPIC_IPC            0x1 diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 64971966af38..d3da23e0ef05 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -390,8 +390,8 @@ static int cnl_ipc_init(struct device *dev, struct skl_dev *cnl)       ipc->dsp = cnl->dsp;       ipc->dev = dev; -    ipc->tx_data_max_size = CNL_ADSP_W1_SZ; -    ipc->rx_data_max_size = CNL_ADSP_W0_UP_SZ; +    ipc->tx_data_max_size = SKL_MAILBOX_SIZE; +    ipc->rx_data_max_size = SKL_MAILBOX_SIZE;       err = sst_ipc_init(ipc);       if (err) @@ -443,9 +443,9 @@ int cnl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,       sst->addr.sram0_base = CNL_ADSP_SRAM0_BASE;       sst->addr.sram1_base = CNL_ADSP_SRAM1_BASE; -    sst_dsp_mailbox_init(sst, (CNL_ADSP_SRAM0_BASE + CNL_ADSP_W0_STAT_SZ), -                 CNL_ADSP_W0_UP_SZ, CNL_ADSP_SRAM1_BASE, -                 CNL_ADSP_W1_SZ); +    sst_dsp_mailbox_init(sst, +        (CNL_ADSP_SRAM0_BASE + SKL_FW_REGS_SIZE), SKL_MAILBOX_SIZE, +        CNL_ADSP_SRAM1_BASE, SKL_MAILBOX_SIZE);       ret = cnl_ipc_init(dev, cnl);       if (ret) { diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index 5a0cb7f3d57e..3c92b1d849e4 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -57,12 +57,7 @@ struct skl_dev;   #define SKL_ADSP_MMIO_LEN        0x10000 -#define SKL_ADSP_W0_STAT_SZ        0x1000

-#define SKL_ADSP_W0_UP_SZ        0x1000

-#define SKL_ADSP_W1_SZ            0x1000

+#define SKL_MAILBOX_SIZE        PAGE_SIZE   #define SKL_FW_REGS_SIZE        PAGE_SIZE   #define SKL_FW_STS_MASK            0xf diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index 2700f882103d..72d7284d2fff 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -606,8 +606,8 @@ int skl_ipc_init(struct device *dev, struct skl_dev *skl)       ipc->dsp = skl->dsp;       ipc->dev = dev; -    ipc->tx_data_max_size = SKL_ADSP_W1_SZ; -    ipc->rx_data_max_size = SKL_ADSP_W0_UP_SZ; +    ipc->tx_data_max_size = SKL_MAILBOX_SIZE; +    ipc->rx_data_max_size = SKL_MAILBOX_SIZE;       err = sst_ipc_init(ipc);       if (err) @@ -922,8 +922,8 @@ int skl_ipc_set_large_config(struct sst_generic_ipc *ipc,       sz_remaining = msg->param_data_size;       data_offset = 0;       while (sz_remaining != 0) { -        tx_size = sz_remaining > SKL_ADSP_W1_SZ -                ? SKL_ADSP_W1_SZ : sz_remaining; +        tx_size = sz_remaining > SKL_MAILBOX_SIZE +                ? SKL_MAILBOX_SIZE : sz_remaining;           if (tx_size == sz_remaining)               header.extension |= IPC_FINAL_BLOCK(1); @@ -965,7 +965,7 @@ int skl_ipc_get_large_config(struct sst_generic_ipc *ipc,       unsigned int *buf;       int ret; -    reply.data = kzalloc(SKL_ADSP_W1_SZ, GFP_KERNEL); +    reply.data = kzalloc(SKL_MAILBOX_SIZE, GFP_KERNEL);       if (!reply.data)           return -ENOMEM; @@ -983,7 +983,7 @@ int skl_ipc_get_large_config(struct sst_generic_ipc *ipc,       request.header = *(u64 *)&header;       request.data = *payload;       request.size = *bytes; -    reply.size = SKL_ADSP_W1_SZ; +    reply.size = SKL_MAILBOX_SIZE;       ret = sst_ipc_tx_message_wait(ipc, request, &reply);       if (ret < 0) diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 7e63c91cea54..191931578fd2 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -536,8 +536,9 @@ int skl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq,       sst->addr.sram0_base = SKL_ADSP_SRAM0_BASE;       sst->addr.sram1_base = SKL_ADSP_SRAM1_BASE; -    sst_dsp_mailbox_init(sst, (SKL_ADSP_SRAM0_BASE + SKL_ADSP_W0_STAT_SZ), -            SKL_ADSP_W0_UP_SZ, SKL_ADSP_SRAM1_BASE, SKL_ADSP_W1_SZ); +    sst_dsp_mailbox_init(sst, +        (SKL_ADSP_SRAM0_BASE + SKL_FW_REGS_SIZE), SKL_MAILBOX_SIZE, +        SKL_ADSP_SRAM1_BASE, SKL_MAILBOX_SIZE);       ret = skl_ipc_init(dev, skl);       if (ret) {

---

Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

On 2019-08-23 21:32, Pierre-Louis Bossart wrote:

...

diff --git a/sound/soc/intel/common/sst-dsp.h b/sound/soc/intel/common/sst-dsp.h index 63c29bb45cf1..a2ac7998fbdb 100644 --- a/sound/soc/intel/common/sst-dsp.h +++ b/sound/soc/intel/common/sst-dsp.h @@ -166,6 +166,27 @@   #define SST_PMCS_PS_MASK    0x3   struct sst_dsp; +struct platform_device;

that should be handled with an include, not redefining a type here.

Ack.

...

+/* Descriptor for setting up SST platform data */ +struct sst_acpi_desc { +    const char *drv_name; +    struct snd_soc_acpi_mach *machines; +    /* Platform resource indexes. Must set to -1 if not used */ +    int resindex_lpe_base; +    int resindex_pcicfg_base; +    int resindex_fw_base; +    int irqindex_host_ipc; +    int resindex_dma_base; +    /* Unique number identifying the SST core on platform */ +    int sst_id; +    /* DMA only valid when resindex_dma_base != -1*/ +    int dma_engine; +    int dma_size; +};

+int sst_acpi_probe(struct platform_device *pdev); +int sst_acpi_remove(struct platform_device *pdev);   /*    * SST Platform Data.

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

With common acpi code exposed, separate Haswell specific code from other legacy platforms. To reduce diff delta, it's still loaded with separate module called: hsw-acpi. This may be address later, once heavy hitters are gone.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/Kconfig | 6 ++- sound/soc/intel/common/sst-acpi.c | 32 ---------------- sound/soc/intel/haswell/Makefile | 2 + sound/soc/intel/haswell/acpi.c | 63 +++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 33 deletions(-) create mode 100644 sound/soc/intel/haswell/acpi.c

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index a3ec17fd63cd..9191773d203c 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -31,6 +31,10 @@ config SND_SST_IPC_ACPI # This option controls the ACPI-based IPC for HiFi2 platforms # (Baytrail, Cherrytrail)

+config SND_SOC_INTEL_HASWELL_ACPI

• tristate
• # This option controls the ACPI-based on HSW/BDW platforms

missing noun. ACPI-based probe?

• config SND_SOC_INTEL_SST_ACPI tristate # This option controls ACPI-based probing on

@@ -52,7 +56,7 @@ config SND_SOC_INTEL_HASWELL depends on SND_DMA_SGBUF depends on DMADEVICES && ACPI select SND_SOC_INTEL_SST

• select SND_SOC_INTEL_SST_ACPI

• select SND_SOC_INTEL_HASWELL_ACPI select SND_SOC_INTEL_SST_FIRMWARE select SND_SOC_ACPI_INTEL_MATCH help

diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c index 42f9b02f2ea3..7d2f04ff0479 100644 --- a/sound/soc/intel/common/sst-acpi.c +++ b/sound/soc/intel/common/sst-acpi.c @@ -15,10 +15,6 @@ #include <sound/soc-acpi.h> #include <sound/soc-acpi-intel-match.h>

-#define SST_LPT_DSP_DMA_ADDR_OFFSET 0x0F0000 -#define SST_WPT_DSP_DMA_ADDR_OFFSET 0x0FE000 -#define SST_LPT_DSP_DMA_SIZE (1024 - 1)

• struct sst_acpi_priv { struct platform_device *pdev_mach; struct platform_device *pdev_pcm;

@@ -156,32 +152,6 @@ int sst_acpi_remove(struct platform_device *pdev) } EXPORT_SYMBOL_GPL(sst_acpi_remove);

-static struct sst_acpi_desc sst_acpi_haswell_desc = {

• .drv_name = "haswell-pcm-audio",
• .machines = snd_soc_acpi_intel_haswell_machines,
• .resindex_lpe_base = 0,
• .resindex_pcicfg_base = 1,
• .resindex_fw_base = -1,
• .irqindex_host_ipc = 0,
• .sst_id = SST_DEV_ID_LYNX_POINT,
• .dma_engine = SST_DMA_TYPE_DW,
• .resindex_dma_base = SST_LPT_DSP_DMA_ADDR_OFFSET,
• .dma_size = SST_LPT_DSP_DMA_SIZE,

-};

-static struct sst_acpi_desc sst_acpi_broadwell_desc = {

• .drv_name = "haswell-pcm-audio",
• .machines = snd_soc_acpi_intel_broadwell_machines,
• .resindex_lpe_base = 0,
• .resindex_pcicfg_base = 1,
• .resindex_fw_base = -1,
• .irqindex_host_ipc = 0,
• .sst_id = SST_DEV_ID_WILDCAT_POINT,
• .dma_engine = SST_DMA_TYPE_DW,
• .resindex_dma_base = SST_WPT_DSP_DMA_ADDR_OFFSET,
• .dma_size = SST_LPT_DSP_DMA_SIZE,

-};

• #if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) static struct sst_acpi_desc sst_acpi_baytrail_desc = { .drv_name = "baytrail-pcm-audio",

@@ -196,8 +166,6 @@ static struct sst_acpi_desc sst_acpi_baytrail_desc = { #endif

static const struct acpi_device_id sst_acpi_match[] = {

• { "INT33C8", (unsigned long)&sst_acpi_haswell_desc },
• { "INT3438", (unsigned long)&sst_acpi_broadwell_desc }, #if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) { "80860F28", (unsigned long)&sst_acpi_baytrail_desc }, #endif

diff --git a/sound/soc/intel/haswell/Makefile b/sound/soc/intel/haswell/Makefile index ad2341aea8ae..0af852d0577b 100644 --- a/sound/soc/intel/haswell/Makefile +++ b/sound/soc/intel/haswell/Makefile @@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only snd-soc-sst-haswell-pcm-objs := \ sst-haswell-ipc.o sst-haswell-pcm.o sst-haswell-dsp.o +snd-soc-hsw-acpi-objs := acpi.o

obj-$(CONFIG_SND_SOC_INTEL_HASWELL) += snd-soc-sst-haswell-pcm.o +obj-$(CONFIG_SND_SOC_INTEL_HASWELL_ACPI) += snd-soc-hsw-acpi.o diff --git a/sound/soc/intel/haswell/acpi.c b/sound/soc/intel/haswell/acpi.c new file mode 100644 index 000000000000..7bd8b03851c4 --- /dev/null +++ b/sound/soc/intel/haswell/acpi.c @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Intel HSW loader on ACPI systems
• • Copyright (C) 2019, Intel Corporation. All rights reserved.
• */

+#include <linux/acpi.h> +#include <linux/platform_device.h> +#include <sound/soc-acpi.h> +#include <sound/soc-acpi-intel-match.h> +#include "../common/sst-dsp.h"

+#define SST_LPT_DSP_DMA_ADDR_OFFSET 0x0F0000 +#define SST_WPT_DSP_DMA_ADDR_OFFSET 0x0FE000 +#define SST_LPT_DSP_DMA_SIZE (1024 - 1)

+static struct sst_acpi_desc hsw_acpi_desc = {

• .drv_name = "haswell-pcm-audio",
• .machines = snd_soc_acpi_intel_haswell_machines,
• .resindex_lpe_base = 0,
• .resindex_pcicfg_base = 1,
• .resindex_fw_base = -1,
• .irqindex_host_ipc = 0,
• .sst_id = SST_DEV_ID_LYNX_POINT,
• .dma_engine = SST_DMA_TYPE_DW,
• .resindex_dma_base = SST_LPT_DSP_DMA_ADDR_OFFSET,
• .dma_size = SST_LPT_DSP_DMA_SIZE,

+};

+static struct sst_acpi_desc bdw_acpi_desc = {

• .drv_name = "haswell-pcm-audio",
• .machines = snd_soc_acpi_intel_broadwell_machines,
• .resindex_lpe_base = 0,
• .resindex_pcicfg_base = 1,
• .resindex_fw_base = -1,
• .irqindex_host_ipc = 0,
• .sst_id = SST_DEV_ID_WILDCAT_POINT,
• .dma_engine = SST_DMA_TYPE_DW,
• .resindex_dma_base = SST_WPT_DSP_DMA_ADDR_OFFSET,
• .dma_size = SST_LPT_DSP_DMA_SIZE,

+};

+static const struct acpi_device_id hsw_acpi_ids[] = {

• { "INT33C8", (unsigned long)&hsw_acpi_desc },
• { "INT3438", (unsigned long)&bdw_acpi_desc },
• { }

+}; +MODULE_DEVICE_TABLE(acpi, hsw_acpi_ids);

+static struct platform_driver hsw_acpi_driver = {

• .probe = sst_acpi_probe,
• .remove = sst_acpi_remove,
• .driver = {

• .name = "hsw-acpi",
  

• .acpi_match_table = ACPI_PTR(hsw_acpi_ids),
  

• },

+}; +module_platform_driver(hsw_acpi_driver);

+MODULE_AUTHOR("Cezary Rojewski cezary.rojewski@intel.com"); +MODULE_DESCRIPTION("Intel HSW loader on ACPI systems"); +MODULE_LICENSE("GPL v2");

On 2019-08-23 21:43, Pierre-Louis Bossart wrote:

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

...

Baytrail and Haswell ACPI loading is now separated and no longer clutters common code. Let's improve the loading procedure and remove some superfluous members.

This change removes sst_pdata::resindex_dma_base as it is a duplication of dma_base. dma_base field has had it's type changed to allow for -1 (not used) value.

ACPI descriptor: sst_acpi_desc loses machines field and sst_id - now accessed via sst_pdata::boards and sst_pdata::id respectively. Cleanup consists mainly of legacy platform-specific probe routines being provided for each descendant. Prevents code duplications, especially for HSW/ BDW case while not losing any readability.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/baytrail/acpi.c         | 43 +++++++++++-----   sound/soc/intel/common/sst-acpi.c       | 21 +++-----   sound/soc/intel/common/sst-dsp.h        |  8 +--   sound/soc/intel/common/sst-firmware.c   |  2 +-   sound/soc/intel/haswell/acpi.c          | 65 +++++++++++++++----------   sound/soc/intel/skylake/skl-sst-utils.c |  2 +-   6 files changed, 82 insertions(+), 59 deletions(-)

diff --git a/sound/soc/intel/baytrail/acpi.c b/sound/soc/intel/baytrail/acpi.c index 57d10a6e3be2..bf2560a8f3e2 100644 --- a/sound/soc/intel/baytrail/acpi.c +++ b/sound/soc/intel/baytrail/acpi.c @@ -11,25 +11,46 @@   #include <sound/soc-acpi-intel-match.h>   #include "../common/sst-dsp.h" -static struct sst_acpi_desc byt_acpi_desc = { -    .drv_name = "baytrail-pcm-audio", -    .machines = snd_soc_acpi_intel_baytrail_legacy_machines, -    .resindex_lpe_base = 0, -    .resindex_pcicfg_base = 1, -    .resindex_fw_base = 2, -    .irqindex_host_ipc = 5, -    .sst_id = SST_DEV_ID_BYT, -    .resindex_dma_base = -1, +static struct sst_pdata byt_desc = { +    .id = SST_DEV_ID_BYT, +    .fw_name = "intel/fw_sst_0f28.bin-48kHz_i2s_master", +    .boards = snd_soc_acpi_intel_baytrail_legacy_machines,

So instead of simplifying you are duplicating the fw_name here.

struct snd_soc_acpi_mach snd_soc_acpi_intel_baytrail_legacy_machines[] = {     {         .id = "10EC5640",         .drv_name = "byt-rt5640",         .fw_filename = "intel/fw_sst_0f28.bin-48kHz_i2s_master",     },     {         .id = "193C9890",         .drv_name = "byt-max98090",         .fw_filename = "intel/fw_sst_0f28.bin-48kHz_i2s_master",     },     {} };

What is the value of all this?

As basefw file name is never tied to specific board, snd_soc_acpi_mach::fw_filename existence ain't a good thing. It's misleading and can catch inexperienced developer off guard.

...

+    .dma_base = -1,   };   static const struct acpi_device_id byt_acpi_ids[] = { -    { "80860F28", (unsigned long)&byt_acpi_desc }, +    { "80860F28", (unsigned long)&byt_desc },       { }   };   MODULE_DEVICE_TABLE(acpi, byt_acpi_ids); +static int byt_acpi_probe(struct platform_device *pdev) +{ +    struct device *dev = &pdev->dev; +    struct sst_acpi_desc *acpi_desc; +    const struct acpi_device_id *id;

+    id = acpi_match_device(dev->driver->acpi_match_table, dev); +    if (!id) +        return -ENODEV;

+    acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); +    if (!acpi_desc) +        return -ENOMEM;

+    acpi_desc->drv_name = "baytrail-pcm-audio"; +    acpi_desc->pdata = (struct sst_pdata *)id->driver_data; +    acpi_desc->resindex_lpe_base = 0; +    acpi_desc->resindex_pcicfg_base = 1; +    acpi_desc->resindex_fw_base = 2; +    acpi_desc->irqindex_host_ipc = 5;

so we had a nice table and now it's inlined in the code. Yuk.

I really don't see the point of these 'simplifications'

And what do they have to do with Skylake anyways, it's PCI enumerated so why should we change this?

Goal is to elevate single struct - here sst_pdata - to be the unified platform descriptor. sst_acpi_desc "extends" sst_pdata. Baytrail _probe method mirrors Haswell to make both look cohesive. Some code duplication removal is also present.

Value? Since when we abandoned the quality principle? Some fields in these structs are redundant and have been removed. Haswell/ Baytrail -specific code has been separated from sst framework to make it unaware of its descendants data. I call framework which knows its children secrets upfront a bad design.

Code does not even change much, it's simply cleaned up.

...

+    platform_set_drvdata(pdev, acpi_desc);

+    return sst_acpi_probe(pdev); +}

static struct platform_driver byt_acpi_driver = { -    .probe = sst_acpi_probe, +    .probe = byt_acpi_probe,       .remove = sst_acpi_remove,       .driver = {           .name = "byt-acpi", diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c index 8e75126106ea..53ac23f05966 100644 --- a/sound/soc/intel/common/sst-acpi.c +++ b/sound/soc/intel/common/sst-acpi.c @@ -17,7 +17,6 @@   struct sst_acpi_priv {       struct platform_device *pdev_mach;       struct platform_device *pdev_pcm; -    struct sst_pdata sst_pdata;       struct sst_acpi_desc *desc;       struct snd_soc_acpi_mach *mach;   }; @@ -27,8 +26,8 @@ static void sst_acpi_fw_cb(const struct firmware *fw, void *context)       struct platform_device *pdev = context;       struct device *dev = &pdev->dev;       struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev); -    struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata;       struct sst_acpi_desc *desc = sst_acpi->desc; +    struct sst_pdata *sst_pdata = desc->pdata;       struct snd_soc_acpi_mach *mach = sst_acpi->mach;       sst_pdata->fw = fw; @@ -51,7 +50,6 @@ static void sst_acpi_fw_cb(const struct firmware *fw, void *context)   int sst_acpi_probe(struct platform_device *pdev)   { -    const struct acpi_device_id *id;       struct device *dev = &pdev->dev;       struct sst_acpi_priv *sst_acpi;       struct sst_pdata *sst_pdata; @@ -64,27 +62,20 @@ int sst_acpi_probe(struct platform_device *pdev)       if (sst_acpi == NULL)           return -ENOMEM; -    id = acpi_match_device(dev->driver->acpi_match_table, dev); -    if (!id) -        return -ENODEV;

-    desc = (struct sst_acpi_desc *)id->driver_data; -    mach = snd_soc_acpi_find_machine(desc->machines); +    desc = platform_get_drvdata(pdev); +    sst_pdata = desc->pdata; +    mach = snd_soc_acpi_find_machine(sst_pdata->boards);       if (mach == NULL) {           dev_err(dev, "No matching ASoC machine driver found\n");           return -ENODEV;       } -    sst_pdata = &sst_acpi->sst_pdata; -    sst_pdata->id = desc->sst_id;       sst_pdata->dma_dev = dev;       sst_acpi->desc = desc;       sst_acpi->mach = mach; -    sst_pdata->resindex_dma_base = desc->resindex_dma_base; -    if (desc->resindex_dma_base >= 0) { +    if (sst_pdata->dma_base >= 0) {           sst_pdata->dma_engine = desc->dma_engine; -        sst_pdata->dma_base = desc->resindex_dma_base;           sst_pdata->dma_size = desc->dma_size;       } @@ -140,7 +131,7 @@ EXPORT_SYMBOL_GPL(sst_acpi_probe);   int sst_acpi_remove(struct platform_device *pdev)   {       struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev); -    struct sst_pdata *sst_pdata = &sst_acpi->sst_pdata; +    struct sst_pdata *sst_pdata = sst_acpi->desc->pdata;       platform_device_unregister(sst_acpi->pdev_mach);       if (!IS_ERR_OR_NULL(sst_acpi->pdev_pcm)) diff --git a/sound/soc/intel/common/sst-dsp.h b/sound/soc/intel/common/sst-dsp.h index a2ac7998fbdb..87d39b0e79c0 100644 --- a/sound/soc/intel/common/sst-dsp.h +++ b/sound/soc/intel/common/sst-dsp.h @@ -171,16 +171,13 @@ struct platform_device;   /* Descriptor for setting up SST platform data */   struct sst_acpi_desc {       const char *drv_name; -    struct snd_soc_acpi_mach *machines; +    struct sst_pdata *pdata;       /* Platform resource indexes. Must set to -1 if not used */       int resindex_lpe_base;       int resindex_pcicfg_base;       int resindex_fw_base;       int irqindex_host_ipc;       int resindex_dma_base; -    /* Unique number identifying the SST core on platform */ -    int sst_id; -    /* DMA only valid when resindex_dma_base != -1*/       int dma_engine;       int dma_size;   }; @@ -205,8 +202,7 @@ struct sst_pdata {       const struct firmware *fw;       /* DMA */ -    int resindex_dma_base; /* other fields invalid if equals to -1 */ -    u32 dma_base; +    int dma_base; /* other fields invalid if equals to -1 */       u32 dma_size;       int dma_engine;       struct device *dma_dev; diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c index 6b6af11c32c3..61d3e6e46b98 100644 --- a/sound/soc/intel/common/sst-firmware.c +++ b/sound/soc/intel/common/sst-firmware.c @@ -268,7 +268,7 @@ static int sst_dma_new(struct sst_dsp *sst)       struct resource mem;       int ret = 0; -    if (sst->pdata->resindex_dma_base == -1) +    if (sst->pdata->dma_base == -1)           /* DMA is not used, return and squelsh error messages */           return 0; diff --git a/sound/soc/intel/haswell/acpi.c b/sound/soc/intel/haswell/acpi.c index 7bd8b03851c4..3c49ec257e56 100644 --- a/sound/soc/intel/haswell/acpi.c +++ b/sound/soc/intel/haswell/acpi.c @@ -15,41 +15,56 @@   #define SST_WPT_DSP_DMA_ADDR_OFFSET    0x0FE000   #define SST_LPT_DSP_DMA_SIZE        (1024 - 1) -static struct sst_acpi_desc hsw_acpi_desc = { -    .drv_name = "haswell-pcm-audio", -    .machines = snd_soc_acpi_intel_haswell_machines, -    .resindex_lpe_base = 0, -    .resindex_pcicfg_base = 1, -    .resindex_fw_base = -1, -    .irqindex_host_ipc = 0, -    .sst_id = SST_DEV_ID_LYNX_POINT, -    .dma_engine = SST_DMA_TYPE_DW, -    .resindex_dma_base = SST_LPT_DSP_DMA_ADDR_OFFSET, -    .dma_size = SST_LPT_DSP_DMA_SIZE, +static struct sst_pdata hsw_desc = { +    .id = SST_DEV_ID_LYNX_POINT, +    .fw_name = "intel/IntcSST1.bin", +    .boards = snd_soc_acpi_intel_haswell_machines, +    .dma_base = SST_LPT_DSP_DMA_ADDR_OFFSET,   }; -static struct sst_acpi_desc bdw_acpi_desc = { -    .drv_name = "haswell-pcm-audio", -    .machines = snd_soc_acpi_intel_broadwell_machines, -    .resindex_lpe_base = 0, -    .resindex_pcicfg_base = 1, -    .resindex_fw_base = -1, -    .irqindex_host_ipc = 0, -    .sst_id = SST_DEV_ID_WILDCAT_POINT, -    .dma_engine = SST_DMA_TYPE_DW, -    .resindex_dma_base = SST_WPT_DSP_DMA_ADDR_OFFSET, -    .dma_size = SST_LPT_DSP_DMA_SIZE, +static struct sst_pdata bdw_desc = { +    .id = SST_DEV_ID_WILDCAT_POINT, +    .fw_name = "intel/IntcSST2.bin", +    .boards = snd_soc_acpi_intel_broadwell_machines, +    .dma_base = SST_WPT_DSP_DMA_ADDR_OFFSET,   };   static const struct acpi_device_id hsw_acpi_ids[] = { -    { "INT33C8", (unsigned long)&hsw_acpi_desc }, -    { "INT3438", (unsigned long)&bdw_acpi_desc }, +    { "INT33C8", (unsigned long)&hsw_desc }, +    { "INT3438", (unsigned long)&bdw_desc },       { }   };   MODULE_DEVICE_TABLE(acpi, hsw_acpi_ids); +static int hsw_acpi_probe(struct platform_device *pdev) +{ +    struct device *dev = &pdev->dev; +    struct sst_acpi_desc *acpi_desc; +    const struct acpi_device_id *id;

+    id = acpi_match_device(dev->driver->acpi_match_table, dev); +    if (!id) +        return -ENODEV;

+    acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); +    if (!acpi_desc) +        return -ENOMEM;

+    acpi_desc->drv_name = "haswell-pcm-audio"; +    acpi_desc->pdata = (struct sst_pdata *)id->driver_data; +    acpi_desc->resindex_lpe_base = 0; +    acpi_desc->resindex_pcicfg_base = 1; +    acpi_desc->resindex_fw_base = -1; +    acpi_desc->irqindex_host_ipc = 0; +    acpi_desc->dma_engine = SST_DMA_TYPE_DW; +    acpi_desc->dma_size = SST_LPT_DSP_DMA_SIZE; +    platform_set_drvdata(pdev, acpi_desc);

+    return sst_acpi_probe(pdev); +}

static struct platform_driver hsw_acpi_driver = { -    .probe = sst_acpi_probe, +    .probe = hsw_acpi_probe,       .remove = sst_acpi_remove,       .driver = {           .name = "hsw-acpi", diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c index bbe67e298efe..ac0a0e4c2d68 100644 --- a/sound/soc/intel/skylake/skl-sst-utils.c +++ b/sound/soc/intel/skylake/skl-sst-utils.c @@ -363,7 +363,7 @@ int skl_sst_ctx_init(struct device *dev, int irq, const char *fw_name,       pdata->id = skl->pci->device;       pdata->irq = irq; -    pdata->resindex_dma_base = -1; +    pdata->dma_base = -1;       skl->dev = dev;       pdata->dsp = skl;       INIT_LIST_HEAD(&skl->uuid_list);

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

Make use of sst_pdata and declare platform descriptors for all existing cAVS platforms. Each carries information about base_fw filename, platform specific operations and boards supported.

if you use a constant base_fw name that cannot be made board-specific for specific usages, you will restrict the ability to deal with quirks and custom cases.

real-life example: not so long ago there were two SST firmwares for 'regular' solutions and ultra-low-latency ones, so by having a single name for all APL-based platforms you will generate issues that don't exist today, or you will force users to patch something in the core.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c | 4 ++-- sound/soc/intel/skylake/cnl-sst.c | 4 ++-- sound/soc/intel/skylake/skl-sst.c | 4 ++-- sound/soc/intel/skylake/skl.c | 38 ++++++++++++++++++++++++++++++- sound/soc/intel/skylake/skl.h | 3 +++ 5 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index a547fb84eee9..06da822790a5 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -531,7 +531,7 @@ static const struct skl_dsp_fw_ops bxt_fw_ops = { .load_library = bxt_load_library, };

-static struct sst_ops skl_ops = { +struct sst_ops apl_sst_ops = { .irq_handler = skl_dsp_sst_interrupt, .thread_fn = skl_dsp_irq_thread_handler, .write = sst_shim32_write, @@ -542,7 +542,7 @@ static struct sst_ops skl_ops = { };

static struct sst_pdata skl_dev = {

• .ops = &skl_ops,

• .ops = &apl_sst_ops, };

  int bxt_sst_dsp_init(struct skl_dev *skl, const char *fw_name)

diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 5be0a8eb154d..c4dbf6655097 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -408,7 +408,7 @@ static int cnl_ipc_init(struct device *dev, struct skl_dev *cnl) return 0; }

-static struct sst_ops cnl_ops = { +struct sst_ops cnl_sst_ops = { .irq_handler = cnl_dsp_sst_interrupt, .thread_fn = cnl_dsp_irq_thread_handler, .write = sst_shim32_write, @@ -419,7 +419,7 @@ static struct sst_ops cnl_ops = { };

static struct sst_pdata cnl_dev = {

• .ops = &cnl_ops,

• .ops = &cnl_sst_ops, };

  int cnl_sst_dsp_init(struct skl_dev *cnl, const char *fw_name)

diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 8ae7fe73534e..122c07290440 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -503,7 +503,7 @@ static const struct skl_dsp_fw_ops skl_fw_ops = { .unload_mod = skl_unload_module, };

-static struct sst_ops skl_ops = { +struct sst_ops skl_sst_ops = { .irq_handler = skl_dsp_sst_interrupt, .write = sst_shim32_write, .read = sst_shim32_read, @@ -513,7 +513,7 @@ static struct sst_ops skl_ops = { };

static struct sst_pdata skl_dev = {

• .ops = &skl_ops,

• .ops = &skl_sst_ops, };

  int skl_sst_dsp_init(struct skl_dev *skl, const char *fw_name)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 54e1f957121d..d6d099aba834 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -27,6 +27,7 @@ #include <sound/hda_i915.h> #include <sound/hda_codec.h> #include <sound/intel-nhlt.h> +#include "../common/sst-dsp.h" #include "skl.h" #include "skl-sst-dsp.h" #include "skl-sst-ipc.h" @@ -1063,7 +1064,6 @@ static int skl_probe(struct pci_dev *pci,

pci_set_drvdata(skl->pci, bus);

• err = skl_find_machine(skl, (void *)pci_id->driver_data); if (err < 0) { dev_err(bus->dev, "skl_find_machine failed with err: %d\n", err);

@@ -1153,6 +1153,42 @@ static void skl_remove(struct pci_dev *pci) dev_set_drvdata(&pci->dev, NULL); }

+static struct sst_pdata skl_desc = {

• .fw_name = "intel/dsp_fw_release.bin",
• .ops = &skl_sst_ops,
• .boards = snd_soc_acpi_intel_skl_machines,

+};

+static struct sst_pdata kbl_desc = {

• .fw_name = "intel/dsp_fw_kbl.bin",
• .ops = &skl_sst_ops,
• .boards = snd_soc_acpi_intel_kbl_machines,

+};

+static struct sst_pdata apl_desc = {

• .fw_name = "intel/dsp_fw_bxtn.bin",
• .ops = &apl_sst_ops,
• .boards = snd_soc_acpi_intel_bxt_machines,

+};

+static struct sst_pdata glk_desc = {

• .fw_name = "intel/dsp_fw_glk.bin",
• .ops = &apl_sst_ops,
• .boards = snd_soc_acpi_intel_glk_machines,

+};

+static struct sst_pdata cnl_desc = {

• .fw_name = "intel/dsp_fw_cnl.bin",
• .ops = &cnl_sst_ops,
• .boards = snd_soc_acpi_intel_cnl_machines,

+};

+static struct sst_pdata icl_desc = {

• .fw_name = "intel/dsp_fw_icl.bin",
• .ops = &cnl_sst_ops,
• .boards = snd_soc_acpi_intel_icl_machines,

+};

• /* PCI IDs */ static const struct pci_device_id skl_ids[] = { #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKL)

diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 9f5aa53df9f8..2f2b5a141abf 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -42,6 +42,9 @@ #define AZX_REG_VS_EM2_L1SEN BIT(13)

struct skl_debug; +extern struct sst_ops skl_sst_ops; +extern struct sst_ops apl_sst_ops; +extern struct sst_ops cnl_sst_ops;

struct skl_astate_param { u32 kcps;

On 8/24/19 5:51 AM, Cezary Rojewski wrote:

On 2019-08-23 21:50, Pierre-Louis Bossart wrote:

...

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

...

Make use of sst_pdata and declare platform descriptors for all existing cAVS platforms. Each carries information about base_fw filename, platform specific operations and boards supported.

if you use a constant base_fw name that cannot be made board-specific for specific usages, you will restrict the ability to deal with quirks and custom cases.

real-life example: not so long ago there were two SST firmwares for 'regular' solutions and ultra-low-latency ones, so by having a single name for all APL-based platforms you will generate issues that don't exist today, or you will force users to patch something in the core.

I did not bring up ULL case for a reason. Guess Pandora box is to be opened.. so be it.

ULL stands for Ultra Low Latency and it can be described by the following:

• exists only for APL based platforms (more like single platform/ model

though)

• in consequence, binary isn't present on any other FW branch and any

other platform apart from APL

• its existence is tied to hardware.. eh.. let's call it a "limitation"
• number of actual vendors is too Ultra Low..
• has limited functionality and validation
• is not the recommended FW for end users in any case
• binary is not going to be upstreamed
• reference board is not going to be upstreamed
• generic (so called main FW) and ULL share the board ACPI ID and thus

require kernel .config to be modified -or- blacklist.conf with be updated

• shares topology filename with generic (main) FW so user still has to

modify his /lib/firmware. Topology names are currently NHLT-based, built from NHLT header data and platform id which are BIOS/ ABL and platform specific respectively (...)

I would describe your answer as 'whatabout-ism'. Yes there are plenty of ways to screw-up, none of them is a justification for assuming that a single filename will work for everyone.

There are also plenty of good reasons to use a different fw and topology file name. Taking this capability away essentially corners users into non-upstreamed custom versions.

TLDR: There is total of 0 people sitting in front of their monitors who are consciously going to make use of ULL firmware. Any user that is going to, will have to play with their kconfig, blacklist and replace existing topology file.

that's where you are making too many assumptions, if quirks and dynamic detection capabilities are provided then it's possible to have a single kernel build that will deal with multiple configurations.

This is normally done by titanic build-bot which, among billion other things, ensures /lib/firmware looks like it should given the configuration.

So, one could have provided a nice choice-box within menuconfig to ensure only one board can be chosen. When one does it, one realizes both generic and ULL firmwares are not actually tied to any specific board and with more boards (usecases) and more kconfigs code gets bloated.

Moving further, guarding apl_desc with #if-else depending on some global generic-vs-ULL configuration which would adjust said descriptors with proper FW filename actually seems like a better solution..

..and then kBOOM comes in and actual design pattern! Board should have been stated tplg_filename, not the fw_filename. Said topology file contains manifest which tells host what libraries to load. And thus, we clear the mist and see that one single field (which is currently missing in snd_soc_acpi_mach) and some clever topology manifest make it all happen: platform-board conflicts cease to exist.

I am not going to argue further. I've spent a lot of time making sure the same kernel build can be used across multiple platforms, if you want to stick to static custom configurations I am not interested in debating. I just hope your team has enough support folks to deal with all these configurations.

Czarek

...

...

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c |  4 ++--   sound/soc/intel/skylake/cnl-sst.c |  4 ++--   sound/soc/intel/skylake/skl-sst.c |  4 ++--   sound/soc/intel/skylake/skl.c     | 38 ++++++++++++++++++++++++++++++-   sound/soc/intel/skylake/skl.h     |  3 +++   5 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index a547fb84eee9..06da822790a5 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -531,7 +531,7 @@ static const struct skl_dsp_fw_ops bxt_fw_ops = {       .load_library = bxt_load_library,   }; -static struct sst_ops skl_ops = { +struct sst_ops apl_sst_ops = {       .irq_handler = skl_dsp_sst_interrupt,       .thread_fn = skl_dsp_irq_thread_handler,       .write = sst_shim32_write, @@ -542,7 +542,7 @@ static struct sst_ops skl_ops = {   };   static struct sst_pdata skl_dev = { -    .ops = &skl_ops, +    .ops = &apl_sst_ops,   };   int bxt_sst_dsp_init(struct skl_dev *skl, const char *fw_name) diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 5be0a8eb154d..c4dbf6655097 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -408,7 +408,7 @@ static int cnl_ipc_init(struct device *dev, struct skl_dev *cnl)       return 0;   } -static struct sst_ops cnl_ops = { +struct sst_ops cnl_sst_ops = {       .irq_handler = cnl_dsp_sst_interrupt,       .thread_fn = cnl_dsp_irq_thread_handler,       .write = sst_shim32_write, @@ -419,7 +419,7 @@ static struct sst_ops cnl_ops = {   };   static struct sst_pdata cnl_dev = { -    .ops = &cnl_ops, +    .ops = &cnl_sst_ops,   };   int cnl_sst_dsp_init(struct skl_dev *cnl, const char *fw_name) diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 8ae7fe73534e..122c07290440 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -503,7 +503,7 @@ static const struct skl_dsp_fw_ops skl_fw_ops = {       .unload_mod = skl_unload_module,   }; -static struct sst_ops skl_ops = { +struct sst_ops skl_sst_ops = {       .irq_handler = skl_dsp_sst_interrupt,       .write = sst_shim32_write,       .read = sst_shim32_read, @@ -513,7 +513,7 @@ static struct sst_ops skl_ops = {   };   static struct sst_pdata skl_dev = { -    .ops = &skl_ops, +    .ops = &skl_sst_ops,   };   int skl_sst_dsp_init(struct skl_dev *skl, const char *fw_name) diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 54e1f957121d..d6d099aba834 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -27,6 +27,7 @@   #include <sound/hda_i915.h>   #include <sound/hda_codec.h>   #include <sound/intel-nhlt.h> +#include "../common/sst-dsp.h"   #include "skl.h"   #include "skl-sst-dsp.h"   #include "skl-sst-ipc.h" @@ -1063,7 +1064,6 @@ static int skl_probe(struct pci_dev *pci,       pci_set_drvdata(skl->pci, bus);

err = skl_find_machine(skl, (void *)pci_id->driver_data);       if (err < 0) {           dev_err(bus->dev, "skl_find_machine failed with err: %d\n", err); @@ -1153,6 +1153,42 @@ static void skl_remove(struct pci_dev *pci)       dev_set_drvdata(&pci->dev, NULL);   } +static struct sst_pdata skl_desc = { +    .fw_name = "intel/dsp_fw_release.bin", +    .ops = &skl_sst_ops, +    .boards = snd_soc_acpi_intel_skl_machines, +};

+static struct sst_pdata kbl_desc = { +    .fw_name = "intel/dsp_fw_kbl.bin", +    .ops = &skl_sst_ops, +    .boards = snd_soc_acpi_intel_kbl_machines, +};

+static struct sst_pdata apl_desc = { +    .fw_name = "intel/dsp_fw_bxtn.bin", +    .ops = &apl_sst_ops, +    .boards = snd_soc_acpi_intel_bxt_machines, +};

+static struct sst_pdata glk_desc = { +    .fw_name = "intel/dsp_fw_glk.bin", +    .ops = &apl_sst_ops, +    .boards = snd_soc_acpi_intel_glk_machines, +};

+static struct sst_pdata cnl_desc = { +    .fw_name = "intel/dsp_fw_cnl.bin", +    .ops = &cnl_sst_ops, +    .boards = snd_soc_acpi_intel_cnl_machines, +};

+static struct sst_pdata icl_desc = { +    .fw_name = "intel/dsp_fw_icl.bin", +    .ops = &cnl_sst_ops, +    .boards = snd_soc_acpi_intel_icl_machines, +};

/* PCI IDs */   static const struct pci_device_id skl_ids[] = {   #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKL) diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 9f5aa53df9f8..2f2b5a141abf 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -42,6 +42,9 @@   #define AZX_REG_VS_EM2_L1SEN        BIT(13)   struct skl_debug; +extern struct sst_ops skl_sst_ops; +extern struct sst_ops apl_sst_ops; +extern struct sst_ops cnl_sst_ops;   struct skl_astate_param {       u32 kcps;

---

Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

With platform descriptors defined, update PCI ID table together with skl_probe to retrieve supported boards from saved sst_pdata objects instead.

I don't get what you are trying to do here. the only 'benefit' seems to duplicate the firmware name, which as I mentioned earlier isn't such a good idea.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/skl.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index d6d099aba834..53a6befd5d68 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -985,6 +985,7 @@ static int skl_probe(struct pci_dev *pci, { struct skl_dev *skl; struct hdac_bus *bus = NULL;

• struct sst_pdata *desc; int err;

  switch (skl_pci_binding) {

@@ -1064,7 +1065,8 @@ static int skl_probe(struct pci_dev *pci,

pci_set_drvdata(skl->pci, bus);

• err = skl_find_machine(skl, (void *)pci_id->driver_data);

• desc = (struct sst_pdata *)pci_id->driver_data;
• err = skl_find_machine(skl, desc->boards); if (err < 0) { dev_err(bus->dev, "skl_find_machine failed with err: %d\n", err); goto out_nhlt_free;

@@ -1194,42 +1196,42 @@ static const struct pci_device_id skl_ids[] = { #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKL) /* Sunrise Point-LP */ { PCI_DEVICE(0x8086, 0x9d70),

• .driver_data = (unsigned long)&snd_soc_acpi_intel_skl_machines},
  

• .driver_data = (unsigned long)&skl_desc },
  

  #endif #if IS_ENABLED(CONFIG_SND_SOC_INTEL_APL) /* BXT-P */ { PCI_DEVICE(0x8086, 0x5a98),

• .driver_data = (unsigned long)&snd_soc_acpi_intel_bxt_machines},
  

• .driver_data = (unsigned long)&apl_desc },
  

  #endif #if IS_ENABLED(CONFIG_SND_SOC_INTEL_KBL) /* KBL */ { PCI_DEVICE(0x8086, 0x9D71),

• .driver_data = (unsigned long)&snd_soc_acpi_intel_kbl_machines},
  

• .driver_data = (unsigned long)&kbl_desc },
  

  #endif #if IS_ENABLED(CONFIG_SND_SOC_INTEL_GLK) /* GLK */ { PCI_DEVICE(0x8086, 0x3198),

• .driver_data = (unsigned long)&snd_soc_acpi_intel_glk_machines},
  

• .driver_data = (unsigned long)&glk_desc },
  

  #endif #if IS_ENABLED(CONFIG_SND_SOC_INTEL_CNL) /* CNL */ { PCI_DEVICE(0x8086, 0x9dc8),

• .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
  

• .driver_data = (unsigned long)&cnl_desc },
  

  #endif #if IS_ENABLED(CONFIG_SND_SOC_INTEL_CFL) /* CFL */ { PCI_DEVICE(0x8086, 0xa348),

• .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
  

• .driver_data = (unsigned long)&cnl_desc },
  

  #endif #if IS_ENABLED(CONFIG_SND_SOC_INTEL_CML_LP) /* CML-LP */ { PCI_DEVICE(0x8086, 0x02c8),

• .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
  

• .driver_data = (unsigned long)&cnl_desc },
  

  #endif #if IS_ENABLED(CONFIG_SND_SOC_INTEL_CML_H) /* CML-H */ { PCI_DEVICE(0x8086, 0x06c8),

• .driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
  

• .driver_data = (unsigned long)&cnl_desc },
  

  #endif { 0, } };

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

To this date Skylake SST were following ill flow of initialization by

'ill' as in 'sick'? that's probably a bit strong and judgmental? or is this a typo?

bypassing sst_dsp_new -> sst_ops::init order. Fix that by flipping invocation order of handlers engaged in Skylake initialization.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c | 15 ++++----------- sound/soc/intel/skylake/cnl-sst-dsp.h | 2 +- sound/soc/intel/skylake/cnl-sst.c | 15 ++++----------- sound/soc/intel/skylake/skl-messages.c | 4 ++-- sound/soc/intel/skylake/skl-sst-dsp.h | 4 ++-- sound/soc/intel/skylake/skl-sst.c | 15 ++++----------- sound/soc/intel/skylake/skl.c | 2 +- sound/soc/intel/skylake/skl.h | 4 ++-- 8 files changed, 20 insertions(+), 41 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 06da822790a5..286da9fbc4de 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -538,24 +538,17 @@ struct sst_ops apl_sst_ops = { .read = sst_shim32_read, .ram_read = sst_memcpy_fromio_32, .ram_write = sst_memcpy_toio_32,

• .init = bxt_sst_dsp_init, .free = skl_dsp_free, };

-static struct sst_pdata skl_dev = {

• .ops = &apl_sst_ops,

-};

-int bxt_sst_dsp_init(struct skl_dev *skl, const char *fw_name) +int bxt_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata) {

• struct sst_dsp *sst;

• struct skl_dev *skl = sst->thread_context; void __iomem *mmio; int ret;

• ret = skl_sst_ctx_init(skl, fw_name, &skl_dev);
• if (ret)

• return ret;
  

• sst = skl->dsp;

• skl->dsp = sst; sst->fw_ops = bxt_fw_ops; mmio = pci_ioremap_bar(skl->pci, 4); if (!mmio)

diff --git a/sound/soc/intel/skylake/cnl-sst-dsp.h b/sound/soc/intel/skylake/cnl-sst-dsp.h index 02e070fae2ce..7810ae11954a 100644 --- a/sound/soc/intel/skylake/cnl-sst-dsp.h +++ b/sound/soc/intel/skylake/cnl-sst-dsp.h @@ -87,6 +87,6 @@ void cnl_ipc_op_int_enable(struct sst_dsp *ctx); void cnl_ipc_op_int_disable(struct sst_dsp *ctx); bool cnl_ipc_int_status(struct sst_dsp *ctx);

-int cnl_sst_dsp_init(struct skl_dev *skl, const char *fw_name); +int cnl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata);

#endif /*__CNL_SST_DSP_H__*/ diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index c4dbf6655097..a6113d8afcbb 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -415,24 +415,17 @@ struct sst_ops cnl_sst_ops = { .read = sst_shim32_read, .ram_read = sst_memcpy_fromio_32, .ram_write = sst_memcpy_toio_32,

• .init = cnl_sst_dsp_init, .free = cnl_dsp_free, };

-static struct sst_pdata cnl_dev = {

• .ops = &cnl_sst_ops,

-};

-int cnl_sst_dsp_init(struct skl_dev *cnl, const char *fw_name) +int cnl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata) {

• struct sst_dsp *sst;

• struct skl_dev *cnl = sst->thread_context; void __iomem *mmio; int ret;

• ret = skl_sst_ctx_init(cnl, fw_name, &cnl_dev);
• if (ret < 0)

• return ret;
  

• sst = cnl->dsp;

• cnl->dsp = sst; sst->fw_ops = cnl_fw_ops; mmio = pci_ioremap_bar(cnl->pci, 4); if (!mmio)

diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index 8c352255ff45..372c5fb83ddb 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -187,7 +187,7 @@ const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id) return NULL; }

-int skl_init_dsp(struct skl_dev *skl) +int skl_init_dsp(struct skl_dev *skl, struct sst_pdata *pdata) { struct hdac_bus *bus = skl_to_bus(skl); const struct skl_dsp_ops *ops; @@ -201,7 +201,7 @@ int skl_init_dsp(struct skl_dev *skl) if (!ops) return -EIO;

• ret = ops->init(skl, skl->fw_name);

• ret = skl_sst_ctx_init(skl, skl->fw_name, pdata); if (ret < 0) return ret;

diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index b647e60d7a6d..8483c60f29ba 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -206,8 +206,8 @@ int skl_dsp_get_core(struct sst_dsp *ctx, unsigned int core_id); int skl_dsp_put_core(struct sst_dsp *ctx, unsigned int core_id);

int skl_dsp_boot(struct sst_dsp *ctx); -int skl_sst_dsp_init(struct skl_dev *skl, const char *fw_name); -int bxt_sst_dsp_init(struct skl_dev *skl, const char *fw_name); +int skl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata); +int bxt_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata); int bxt_load_library(struct sst_dsp *ctx, struct skl_lib_info *linfo, int lib_count);

diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 122c07290440..e0f2bf828541 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -509,24 +509,17 @@ struct sst_ops skl_sst_ops = { .read = sst_shim32_read, .ram_read = sst_memcpy_fromio_32, .ram_write = sst_memcpy_toio_32,

• .init = skl_sst_dsp_init, .free = skl_dsp_free, };

-static struct sst_pdata skl_dev = {

• .ops = &skl_sst_ops,

-};

-int skl_sst_dsp_init(struct skl_dev *skl, const char *fw_name) +int skl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata) {

• struct sst_dsp *sst;

• struct skl_dev *skl = sst->thread_context; void __iomem *mmio; int ret;

• ret = skl_sst_ctx_init(skl, fw_name, &skl_dev);
• if (ret < 0)

• return ret;
  

• sst = skl->dsp;

• skl->dsp = sst; sst->fw_ops = skl_fw_ops; mmio = pci_ioremap_bar(skl->pci, 4); if (!mmio)

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 53a6befd5d68..39442c80a179 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -1072,7 +1072,7 @@ static int skl_probe(struct pci_dev *pci, goto out_nhlt_free; }

• err = skl_init_dsp(skl);

• err = skl_init_dsp(skl, desc); if (err < 0) { dev_dbg(bus->dev, "error failed to register dsp\n"); goto out_nhlt_free;

diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 2f2b5a141abf..f4cd5ccc1ff9 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -159,7 +159,7 @@ struct skl_machine_pdata {

struct skl_dsp_ops { int id;

• int (*init)(struct skl_dev *skl, const char *fw_name);

• int (*init)(struct sst_dsp *dsp, struct sst_pdata *pdata); };

  int skl_platform_unregister(struct device *dev);

@@ -170,7 +170,7 @@ struct nhlt_specific_cfg *skl_get_ep_blob(struct skl_dev *skl, u32 instance, u32 s_rate, u8 dirn, u8 dev_type);

int skl_nhlt_update_topology_bin(struct skl_dev *skl); -int skl_init_dsp(struct skl_dev *skl); +int skl_init_dsp(struct skl_dev *skl, struct sst_pdata *pdata); int skl_free_dsp(struct skl_dev *skl); int skl_sst_init_fw(struct skl_dev *skl); void skl_sst_dsp_cleanup(struct skl_dev *skl);

On 8/24/19 5:54 AM, Cezary Rojewski wrote:

On 2019-08-23 22:18, Pierre-Louis Bossart wrote:

...

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

...

To this date Skylake SST were following ill flow of initialization by

'ill' as in 'sick'? that's probably a bit strong and judgmental? or is this a typo?

ill seems like a perfect opposite of healthy, ain't it? Because certainly, the initialization pattern observed in /skylake is everything but healthy.

I don't know what 'healthy' means either in this context.

s/ill/incorrect/?

...

...

bypassing sst_dsp_new -> sst_ops::init order. Fix that by flipping invocation order of handlers engaged in Skylake initialization.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c      | 15 ++++-----------   sound/soc/intel/skylake/cnl-sst-dsp.h  |  2 +-   sound/soc/intel/skylake/cnl-sst.c      | 15 ++++-----------   sound/soc/intel/skylake/skl-messages.c |  4 ++--   sound/soc/intel/skylake/skl-sst-dsp.h  |  4 ++--   sound/soc/intel/skylake/skl-sst.c      | 15 ++++-----------   sound/soc/intel/skylake/skl.c          |  2 +-   sound/soc/intel/skylake/skl.h          |  4 ++--   8 files changed, 20 insertions(+), 41 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 06da822790a5..286da9fbc4de 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -538,24 +538,17 @@ struct sst_ops apl_sst_ops = {       .read = sst_shim32_read,       .ram_read = sst_memcpy_fromio_32,       .ram_write = sst_memcpy_toio_32, +    .init = bxt_sst_dsp_init,       .free = skl_dsp_free,   }; -static struct sst_pdata skl_dev = { -    .ops = &apl_sst_ops, -};

-int bxt_sst_dsp_init(struct skl_dev *skl, const char *fw_name) +int bxt_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata)   { -    struct sst_dsp *sst; +    struct skl_dev *skl = sst->thread_context;       void __iomem *mmio;       int ret; -    ret = skl_sst_ctx_init(skl, fw_name, &skl_dev); -    if (ret) -        return ret;

-    sst = skl->dsp; +    skl->dsp = sst;       sst->fw_ops = bxt_fw_ops;       mmio = pci_ioremap_bar(skl->pci, 4);       if (!mmio) diff --git a/sound/soc/intel/skylake/cnl-sst-dsp.h b/sound/soc/intel/skylake/cnl-sst-dsp.h index 02e070fae2ce..7810ae11954a 100644 --- a/sound/soc/intel/skylake/cnl-sst-dsp.h +++ b/sound/soc/intel/skylake/cnl-sst-dsp.h @@ -87,6 +87,6 @@ void cnl_ipc_op_int_enable(struct sst_dsp *ctx);   void cnl_ipc_op_int_disable(struct sst_dsp *ctx);   bool cnl_ipc_int_status(struct sst_dsp *ctx); -int cnl_sst_dsp_init(struct skl_dev *skl, const char *fw_name); +int cnl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata);   #endif /*__CNL_SST_DSP_H__*/ diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index c4dbf6655097..a6113d8afcbb 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -415,24 +415,17 @@ struct sst_ops cnl_sst_ops = {       .read = sst_shim32_read,       .ram_read = sst_memcpy_fromio_32,       .ram_write = sst_memcpy_toio_32, +    .init = cnl_sst_dsp_init,       .free = cnl_dsp_free,   }; -static struct sst_pdata cnl_dev = { -    .ops = &cnl_sst_ops, -};

-int cnl_sst_dsp_init(struct skl_dev *cnl, const char *fw_name) +int cnl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata)   { -    struct sst_dsp *sst; +    struct skl_dev *cnl = sst->thread_context;       void __iomem *mmio;       int ret; -    ret = skl_sst_ctx_init(cnl, fw_name, &cnl_dev); -    if (ret < 0) -        return ret;

-    sst = cnl->dsp; +    cnl->dsp = sst;       sst->fw_ops = cnl_fw_ops;       mmio = pci_ioremap_bar(cnl->pci, 4);       if (!mmio) diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index 8c352255ff45..372c5fb83ddb 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -187,7 +187,7 @@ const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id)       return NULL;   } -int skl_init_dsp(struct skl_dev *skl) +int skl_init_dsp(struct skl_dev *skl, struct sst_pdata *pdata)   {       struct hdac_bus *bus = skl_to_bus(skl);       const struct skl_dsp_ops *ops; @@ -201,7 +201,7 @@ int skl_init_dsp(struct skl_dev *skl)       if (!ops)           return -EIO; -    ret = ops->init(skl, skl->fw_name); +    ret = skl_sst_ctx_init(skl, skl->fw_name, pdata);       if (ret < 0)           return ret; diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index b647e60d7a6d..8483c60f29ba 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -206,8 +206,8 @@ int skl_dsp_get_core(struct sst_dsp *ctx, unsigned int core_id);   int skl_dsp_put_core(struct sst_dsp *ctx, unsigned int core_id);   int skl_dsp_boot(struct sst_dsp *ctx); -int skl_sst_dsp_init(struct skl_dev *skl, const char *fw_name); -int bxt_sst_dsp_init(struct skl_dev *skl, const char *fw_name); +int skl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata); +int bxt_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata);   int bxt_load_library(struct sst_dsp *ctx, struct skl_lib_info *linfo,           int lib_count); diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 122c07290440..e0f2bf828541 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -509,24 +509,17 @@ struct sst_ops skl_sst_ops = {       .read = sst_shim32_read,       .ram_read = sst_memcpy_fromio_32,       .ram_write = sst_memcpy_toio_32, +    .init = skl_sst_dsp_init,       .free = skl_dsp_free,   }; -static struct sst_pdata skl_dev = { -    .ops = &skl_sst_ops, -};

-int skl_sst_dsp_init(struct skl_dev *skl, const char *fw_name) +int skl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata)   { -    struct sst_dsp *sst; +    struct skl_dev *skl = sst->thread_context;       void __iomem *mmio;       int ret; -    ret = skl_sst_ctx_init(skl, fw_name, &skl_dev); -    if (ret < 0) -        return ret;

-    sst = skl->dsp; +    skl->dsp = sst;       sst->fw_ops = skl_fw_ops;       mmio = pci_ioremap_bar(skl->pci, 4);       if (!mmio) diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 53a6befd5d68..39442c80a179 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -1072,7 +1072,7 @@ static int skl_probe(struct pci_dev *pci,           goto out_nhlt_free;       } -    err = skl_init_dsp(skl); +    err = skl_init_dsp(skl, desc);       if (err < 0) {           dev_dbg(bus->dev, "error failed to register dsp\n");           goto out_nhlt_free; diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 2f2b5a141abf..f4cd5ccc1ff9 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -159,7 +159,7 @@ struct skl_machine_pdata {   struct skl_dsp_ops {       int id; -    int (*init)(struct skl_dev *skl, const char *fw_name); +    int (*init)(struct sst_dsp *dsp, struct sst_pdata *pdata);   };   int skl_platform_unregister(struct device *dev); @@ -170,7 +170,7 @@ struct nhlt_specific_cfg *skl_get_ep_blob(struct skl_dev *skl, u32 instance,                       u32 s_rate, u8 dirn, u8 dev_type);   int skl_nhlt_update_topology_bin(struct skl_dev *skl); -int skl_init_dsp(struct skl_dev *skl); +int skl_init_dsp(struct skl_dev *skl, struct sst_pdata *pdata);   int skl_free_dsp(struct skl_dev *skl);   int skl_sst_init_fw(struct skl_dev *skl);   void skl_sst_dsp_cleanup(struct skl_dev *skl);

On 2019-08-23 22:20, Pierre-Louis Bossart wrote:

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

...

struct sst_pdata is equipped with fw_name field - a platform specific filename for basefw module. Usage of such allows for suther simplification of declaration of handlers directly involved with Skylake initialization procedure.

This change invalidates mach::fw_filename field and skl::fw_name.

Again bad move. While in theory it's true that a single firmware is all you need, you do want to keep the ability to quirk firmware names for specific cases. We've been there before, don't remove this capability please.

Explained this on PATCH 27/35. This is the task for topology manifest. Please note basefw binary alone does not cut it. There are external libraries and vendor cases too. In actual real-life specific example, you need to replace everything, e.g.: basefw, ARSC, WoV and so on. Don't see how fw_filename can represent several files, really.

...

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/common/sst-acpi.c       | 5 ++---   sound/soc/intel/common/sst-firmware.c   | 1 +   sound/soc/intel/skylake/skl-messages.c  | 2 +-   sound/soc/intel/skylake/skl-sst-dsp.h   | 3 +--   sound/soc/intel/skylake/skl-sst-utils.c | 4 +---   sound/soc/intel/skylake/skl.c           | 4 ----   6 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c index 53ac23f05966..15f2b27e643f 100644 --- a/sound/soc/intel/common/sst-acpi.c +++ b/sound/soc/intel/common/sst-acpi.c @@ -28,11 +28,10 @@ static void sst_acpi_fw_cb(const struct firmware *fw, void *context)       struct sst_acpi_priv *sst_acpi = platform_get_drvdata(pdev);       struct sst_acpi_desc *desc = sst_acpi->desc;       struct sst_pdata *sst_pdata = desc->pdata; -    struct snd_soc_acpi_mach *mach = sst_acpi->mach;       sst_pdata->fw = fw;       if (!fw) { -        dev_err(dev, "Cannot load firmware %s\n", mach->fw_filename); +        dev_err(dev, "Cannot load firmware %s\n", sst_pdata->fw_name);           return;       } @@ -119,7 +118,7 @@ int sst_acpi_probe(struct platform_device *pdev)           return PTR_ERR(sst_acpi->pdev_mach);       /* continue SST probing after firmware is loaded */ -    ret = request_firmware_nowait(THIS_MODULE, true, mach->fw_filename, +    ret = request_firmware_nowait(THIS_MODULE, true, sst_pdata->fw_name,                         dev, GFP_KERNEL, pdev, sst_acpi_fw_cb);       if (ret)           platform_device_unregister(sst_acpi->pdev_mach); diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c index 61d3e6e46b98..cc88849eb10f 100644 --- a/sound/soc/intel/common/sst-firmware.c +++ b/sound/soc/intel/common/sst-firmware.c @@ -1218,6 +1218,7 @@ struct sst_dsp *sst_dsp_new(struct device *dev, struct sst_pdata *pdata)       sst->thread_context = pdata->dsp;       sst->id = pdata->id;       sst->irq = pdata->irq; +    sst->fw_name = pdata->fw_name;       sst->ops = pdata->ops;       sst->pdata = pdata;       INIT_LIST_HEAD(&sst->used_block_list); diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index 372c5fb83ddb..e401edd8d44b 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -201,7 +201,7 @@ int skl_init_dsp(struct skl_dev *skl, struct sst_pdata *pdata)       if (!ops)           return -EIO; -    ret = skl_sst_ctx_init(skl, skl->fw_name, pdata); +    ret = skl_sst_ctx_init(skl, pdata);       if (ret < 0)           return ret; diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index 8483c60f29ba..a3714b706b8e 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -223,8 +223,7 @@ int skl_dsp_strip_extended_manifest(struct firmware *fw);   void skl_dsp_set_astate_cfg(struct skl_dev *skl, u32 cnt, void *data); -int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, -        struct sst_pdata *pdata); +int skl_sst_ctx_init(struct skl_dev *skl, struct sst_pdata *pdata);   int skl_prepare_lib_load(struct skl_dev *skl, struct skl_lib_info *linfo,               struct firmware *stripped_fw,               unsigned int hdr_offset, int index); diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c index a4ad213d34e0..ea5419012312 100644 --- a/sound/soc/intel/skylake/skl-sst-utils.c +++ b/sound/soc/intel/skylake/skl-sst-utils.c @@ -354,8 +354,7 @@ int skl_dsp_strip_extended_manifest(struct firmware *fw)       return 0;   } -int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name, -    struct sst_pdata *pdata) +int skl_sst_ctx_init(struct skl_dev *skl, struct sst_pdata *pdata)   {       struct sst_dsp *sst;       struct device *dev = skl->dev; @@ -372,7 +371,6 @@ int skl_sst_ctx_init(struct skl_dev *skl, const char *fw_name,       }       skl->dsp = sst; -    sst->fw_name = fw_name;       init_waitqueue_head(&skl->mod_load_wait);       skl->is_first_boot = true; diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 39442c80a179..3225f4f8793e 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -491,9 +491,6 @@ static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl,       /* point to common table */       mach = snd_soc_acpi_intel_hda_machines; -    /* all entries in the machine table use the same firmware */ -    mach->fw_filename = machines->fw_filename;

return mach;   } @@ -514,7 +511,6 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data)       }       skl->mach = mach; -    skl->fw_name = mach->fw_filename;       pdata = mach->pdata;       if (pdata) {

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

With initialization order repaired, sst_ops::init overloads can be privatized for Skylake platofmrs. Let's do so.

platforms

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/skylake/bxt-sst.c | 25 ++++++++++++------------- sound/soc/intel/skylake/cnl-sst-dsp.h | 2 -- sound/soc/intel/skylake/cnl-sst.c | 25 ++++++++++++------------- sound/soc/intel/skylake/skl-sst-dsp.h | 2 -- sound/soc/intel/skylake/skl-sst.c | 23 +++++++++++------------ 5 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 79728e418176..1edc38069615 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -533,18 +533,7 @@ static const struct skl_dsp_fw_ops bxt_fw_ops = { .load_library = bxt_load_library, };

-struct sst_ops apl_sst_ops = {

• .irq_handler = skl_dsp_sst_interrupt,
• .thread_fn = skl_dsp_irq_thread_handler,
• .write = sst_shim32_write,
• .read = sst_shim32_read,
• .ram_read = sst_memcpy_fromio_32,
• .ram_write = sst_memcpy_toio_32,
• .init = bxt_sst_dsp_init,
• .free = skl_dsp_free,

-};

-int bxt_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata) +static int bxt_sst_init(struct sst_dsp *sst, struct sst_pdata *pdata) { struct skl_dev *skl = sst->thread_context; void __iomem *mmio; @@ -578,7 +567,17 @@ int bxt_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata)

return 0; } -EXPORT_SYMBOL_GPL(bxt_sst_dsp_init);

+struct sst_ops apl_sst_ops = {

• .irq_handler = skl_dsp_sst_interrupt,
• .thread_fn = skl_dsp_irq_thread_handler,
• .write = sst_shim32_write,
• .read = sst_shim32_read,
• .ram_read = sst_memcpy_fromio_32,
• .ram_write = sst_memcpy_toio_32,
• .init = bxt_sst_init,
• .free = skl_dsp_free,

+};

should these structures be static if they are not exported? sparse should tell you that: make -C 2

MODULE_LICENSE("GPL v2"); MODULE_DESCRIPTION("Intel Broxton IPC driver"); diff --git a/sound/soc/intel/skylake/cnl-sst-dsp.h b/sound/soc/intel/skylake/cnl-sst-dsp.h index 7810ae11954a..a1ea242e9539 100644 --- a/sound/soc/intel/skylake/cnl-sst-dsp.h +++ b/sound/soc/intel/skylake/cnl-sst-dsp.h @@ -87,6 +87,4 @@ void cnl_ipc_op_int_enable(struct sst_dsp *ctx); void cnl_ipc_op_int_disable(struct sst_dsp *ctx); bool cnl_ipc_int_status(struct sst_dsp *ctx);

-int cnl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata);

• #endif /*__CNL_SST_DSP_H__*/

diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 58efeb6c8a8a..360e54d3c587 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -409,18 +409,7 @@ static int cnl_ipc_init(struct device *dev, struct skl_dev *cnl) return 0; }

-struct sst_ops cnl_sst_ops = {

• .irq_handler = cnl_dsp_sst_interrupt,
• .thread_fn = cnl_dsp_irq_thread_handler,
• .write = sst_shim32_write,
• .read = sst_shim32_read,
• .ram_read = sst_memcpy_fromio_32,
• .ram_write = sst_memcpy_toio_32,
• .init = cnl_sst_dsp_init,
• .free = cnl_dsp_free,

-};

-int cnl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata) +static int cnl_sst_init(struct sst_dsp *sst, struct sst_pdata *pdata) { struct skl_dev *cnl = sst->thread_context; void __iomem *mmio; @@ -449,7 +438,17 @@ int cnl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata)

return 0; } -EXPORT_SYMBOL_GPL(cnl_sst_dsp_init);

+struct sst_ops cnl_sst_ops = {

• .irq_handler = cnl_dsp_sst_interrupt,
• .thread_fn = cnl_dsp_irq_thread_handler,
• .write = sst_shim32_write,
• .read = sst_shim32_read,
• .ram_read = sst_memcpy_fromio_32,
• .ram_write = sst_memcpy_toio_32,
• .init = cnl_sst_init,
• .free = cnl_dsp_free,

+};

MODULE_LICENSE("GPL v2"); MODULE_DESCRIPTION("Intel Cannonlake IPC driver"); diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index a3714b706b8e..3294826d5cf7 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -206,8 +206,6 @@ int skl_dsp_get_core(struct sst_dsp *ctx, unsigned int core_id); int skl_dsp_put_core(struct sst_dsp *ctx, unsigned int core_id);

int skl_dsp_boot(struct sst_dsp *ctx); -int skl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata); -int bxt_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata); int bxt_load_library(struct sst_dsp *ctx, struct skl_lib_info *linfo, int lib_count);

diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 8e1d02e29a32..209f45e3d5d0 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -505,17 +505,7 @@ static const struct skl_dsp_fw_ops skl_fw_ops = { .unload_mod = skl_unload_module, };

-struct sst_ops skl_sst_ops = {

• .irq_handler = skl_dsp_sst_interrupt,
• .write = sst_shim32_write,
• .read = sst_shim32_read,
• .ram_read = sst_memcpy_fromio_32,
• .ram_write = sst_memcpy_toio_32,
• .init = skl_sst_dsp_init,
• .free = skl_dsp_free,

-};

-int skl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata) +static int skl_sst_init(struct sst_dsp *sst, struct sst_pdata *pdata) { struct skl_dev *skl = sst->thread_context; void __iomem *mmio; @@ -541,7 +531,16 @@ int skl_sst_dsp_init(struct sst_dsp *sst, struct sst_pdata *pdata)

return 0; } -EXPORT_SYMBOL_GPL(skl_sst_dsp_init);

+struct sst_ops skl_sst_ops = {

• .irq_handler = skl_dsp_sst_interrupt,
• .write = sst_shim32_write,
• .read = sst_shim32_read,
• .ram_read = sst_memcpy_fromio_32,
• .ram_write = sst_memcpy_toio_32,
• .init = skl_sst_init,
• .free = skl_dsp_free,

+};

int skl_sst_init_fw(struct skl_dev *skl) {

On 8/22/19 2:04 PM, Cezary Rojewski wrote:

FW filename fields are now deprecated in favour of ones coming from platform descriptors. This aligns with paradigm of FW being platform specific, not board specific.

I don't agree here. The paradigm does not survive reality where we have different versions of firmware for the same platform. This is not the product of my imagination, the ULL story proves my point.

NAK.

Any remaining deprecated survivors of the precedding tidal wave are removed here too.

Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com

sound/soc/intel/common/soc-acpi-intel-bxt-match.c | 2 -- sound/soc/intel/common/soc-acpi-intel-byt-match.c | 2 -- sound/soc/intel/common/soc-acpi-intel-cnl-match.c | 1 - sound/soc/intel/common/soc-acpi-intel-glk-match.c | 3 --- sound/soc/intel/common/soc-acpi-intel-hda-match.c | 2 -- .../soc/intel/common/soc-acpi-intel-hsw-bdw-match.c | 4 ---- sound/soc/intel/common/soc-acpi-intel-icl-match.c | 1 - sound/soc/intel/common/soc-acpi-intel-kbl-match.c | 12 ------------ sound/soc/intel/common/soc-acpi-intel-skl-match.c | 3 --- sound/soc/intel/common/sst-dsp-priv.h | 1 - sound/soc/intel/skylake/skl.h | 1 - 11 files changed, 32 deletions(-)

diff --git a/sound/soc/intel/common/soc-acpi-intel-bxt-match.c b/sound/soc/intel/common/soc-acpi-intel-bxt-match.c index 4a5adae1d785..eda799e49113 100644 --- a/sound/soc/intel/common/soc-acpi-intel-bxt-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-bxt-match.c @@ -50,14 +50,12 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_bxt_machines[] = { { .id = "INT343A", .drv_name = "bxt_alc298s_i2s",

• .fw_filename = "intel/dsp_fw_bxtn.bin",
  

  .sof_fw_filename = "sof-apl.ri", .sof_tplg_filename = "sof-apl-rt298.tplg", }, { .id = "DLGS7219", .drv_name = "bxt_da7219_max98357a",

• .fw_filename = "intel/dsp_fw_bxtn.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &bxt_codecs, .sof_fw_filename = "sof-apl.ri",

diff --git a/sound/soc/intel/common/soc-acpi-intel-byt-match.c b/sound/soc/intel/common/soc-acpi-intel-byt-match.c index 1cc801ba92eb..4d2ba663ee33 100644 --- a/sound/soc/intel/common/soc-acpi-intel-byt-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-byt-match.c @@ -124,12 +124,10 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_baytrail_legacy_machines[] = { { .id = "10EC5640", .drv_name = "byt-rt5640",

• .fw_filename = "intel/fw_sst_0f28.bin-48kHz_i2s_master",
  

  }, { .id = "193C9890", .drv_name = "byt-max98090",

• .fw_filename = "intel/fw_sst_0f28.bin-48kHz_i2s_master",
  

  }, {} };

diff --git a/sound/soc/intel/common/soc-acpi-intel-cnl-match.c b/sound/soc/intel/common/soc-acpi-intel-cnl-match.c index 771b0ef21051..edc44a0dc96f 100644 --- a/sound/soc/intel/common/soc-acpi-intel-cnl-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-cnl-match.c @@ -23,7 +23,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cnl_machines[] = { { .id = "INT34C2", .drv_name = "cnl_rt274",

• .fw_filename = "intel/dsp_fw_cnl.bin",
  

  .pdata = &cnl_pdata, .sof_fw_filename = "sof-cnl.ri", .sof_tplg_filename = "sof-cnl-rt274.tplg",

diff --git a/sound/soc/intel/common/soc-acpi-intel-glk-match.c b/sound/soc/intel/common/soc-acpi-intel-glk-match.c index 60dea358fa04..370487d13c85 100644 --- a/sound/soc/intel/common/soc-acpi-intel-glk-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-glk-match.c @@ -18,14 +18,12 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_glk_machines[] = { { .id = "INT343A", .drv_name = "glk_alc298s_i2s",

• .fw_filename = "intel/dsp_fw_glk.bin",
  

  .sof_fw_filename = "sof-glk.ri", .sof_tplg_filename = "sof-glk-alc298.tplg", }, { .id = "DLGS7219", .drv_name = "glk_da7219_max98357a",

• .fw_filename = "intel/dsp_fw_glk.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &glk_codecs, .sof_fw_filename = "sof-glk.ri",

@@ -34,7 +32,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_glk_machines[] = { { .id = "10EC5682", .drv_name = "glk_rt5682_max98357a",

• .fw_filename = "intel/dsp_fw_glk.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &glk_codecs, .sof_fw_filename = "sof-glk.ri",

diff --git a/sound/soc/intel/common/soc-acpi-intel-hda-match.c b/sound/soc/intel/common/soc-acpi-intel-hda-match.c index cc972d2ac691..39827d2e8634 100644 --- a/sound/soc/intel/common/soc-acpi-intel-hda-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-hda-match.c @@ -19,8 +19,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_hda_machines[] = { /* .id is not used in this file */ .drv_name = "skl_hda_dsp_generic",

• /* .fw_filename is dynamically set in skylake driver */
  

• /* .sof_fw_filename is dynamically set in sof/intel driver */

  .sof_tplg_filename = "sof-hda-generic.tplg",

diff --git a/sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c b/sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c index 34eb0baaa951..6b113c32aff0 100644 --- a/sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c @@ -13,7 +13,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_haswell_machines[] = { { .id = "INT33CA", .drv_name = "haswell-audio",

• .fw_filename = "intel/IntcSST1.bin",
  

  .sof_fw_filename = "sof-hsw.ri", .sof_tplg_filename = "sof-hsw.tplg", },

@@ -25,21 +24,18 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_broadwell_machines[] = { { .id = "INT343A", .drv_name = "broadwell-audio",

• .fw_filename =  "intel/IntcSST2.bin",
  

  .sof_fw_filename = "sof-bdw.ri", .sof_tplg_filename = "sof-bdw-rt286.tplg", }, { .id = "RT5677CE", .drv_name = "bdw-rt5677",

• .fw_filename =  "intel/IntcSST2.bin",
  

  .sof_fw_filename = "sof-bdw.ri", .sof_tplg_filename = "sof-bdw-rt5677.tplg", }, { .id = "INT33CA", .drv_name = "haswell-audio",

• .fw_filename = "intel/IntcSST2.bin",
  

  .sof_fw_filename = "sof-bdw.ri", .sof_tplg_filename = "sof-bdw-rt5640.tplg", },

diff --git a/sound/soc/intel/common/soc-acpi-intel-icl-match.c b/sound/soc/intel/common/soc-acpi-intel-icl-match.c index 38977669b576..04a17da96f6a 100644 --- a/sound/soc/intel/common/soc-acpi-intel-icl-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-icl-match.c @@ -18,7 +18,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = { { .id = "INT34C2", .drv_name = "icl_rt274",

• .fw_filename = "intel/dsp_fw_icl.bin",
  

  .pdata = &icl_pdata, .sof_fw_filename = "sof-icl.ri", .sof_tplg_filename = "sof-icl-rt274.tplg",

diff --git a/sound/soc/intel/common/soc-acpi-intel-kbl-match.c b/sound/soc/intel/common/soc-acpi-intel-kbl-match.c index e200baa11011..fe82d5472aa3 100644 --- a/sound/soc/intel/common/soc-acpi-intel-kbl-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-kbl-match.c @@ -46,12 +46,10 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = { { .id = "INT343A", .drv_name = "kbl_alc286s_i2s",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  }, { .id = "INT343B", .drv_name = "kbl_n88l25_s4567",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &kbl_codecs, .pdata = &skl_dmic_data,

@@ -59,7 +57,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = { { .id = "MX98357A", .drv_name = "kbl_n88l25_m98357a",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &kbl_codecs, .pdata = &skl_dmic_data,

@@ -67,7 +64,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = { { .id = "MX98927", .drv_name = "kbl_r5514_5663_max",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &kbl_5663_5514_codecs, .pdata = &skl_dmic_data,

@@ -75,7 +71,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = { { .id = "MX98927", .drv_name = "kbl_rt5663_m98927",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &kbl_poppy_codecs, .pdata = &skl_dmic_data,

@@ -83,12 +78,10 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = { { .id = "10EC5663", .drv_name = "kbl_rt5663",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  }, { .id = "DLGS7219", .drv_name = "kbl_da7219_max98357a",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &kbl_7219_98357_codecs, .pdata = &skl_dmic_data,

@@ -96,7 +89,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = { { .id = "DLGS7219", .drv_name = "kbl_da7219_max98927",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &kbl_7219_98927_codecs, .pdata = &skl_dmic_data

@@ -104,17 +96,14 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = { { .id = "10EC5660", .drv_name = "kbl_rt5660",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  }, { .id = "10EC3277", .drv_name = "kbl_rt5660",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  }, { .id = "DLGS7219", .drv_name = "kbl_da7219_max98373",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &kbl_7219_98373_codecs, .pdata = &skl_dmic_data

@@ -122,7 +111,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = { { .id = "MX98373", .drv_name = "kbl_max98373",

• .fw_filename = "intel/dsp_fw_kbl.bin",
  

  .pdata = &skl_dmic_data }, {},

diff --git a/sound/soc/intel/common/soc-acpi-intel-skl-match.c b/sound/soc/intel/common/soc-acpi-intel-skl-match.c index 42fa40a8d932..ec969044706c 100644 --- a/sound/soc/intel/common/soc-acpi-intel-skl-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-skl-match.c @@ -21,12 +21,10 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_skl_machines[] = { { .id = "INT343A", .drv_name = "skl_alc286s_i2s",

• .fw_filename = "intel/dsp_fw_release.bin",
  

  }, { .id = "INT343B", .drv_name = "skl_n88l25_s4567",

• .fw_filename = "intel/dsp_fw_release.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &skl_codecs, .pdata = &skl_dmic_data,

@@ -34,7 +32,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_skl_machines[] = { { .id = "MX98357A", .drv_name = "skl_n88l25_m98357a",

• .fw_filename = "intel/dsp_fw_release.bin",
  

  .machine_quirk = snd_soc_acpi_codec_list, .quirk_data = &skl_codecs, .pdata = &skl_dmic_data,

diff --git a/sound/soc/intel/common/sst-dsp-priv.h b/sound/soc/intel/common/sst-dsp-priv.h index a4628a89d47d..1a208ef65fa8 100644 --- a/sound/soc/intel/common/sst-dsp-priv.h +++ b/sound/soc/intel/common/sst-dsp-priv.h @@ -306,7 +306,6 @@ struct sst_dsp { int sst_state; struct skl_cl_dev cl_dev; u32 intr_status;

• const struct firmware *fw; struct snd_dma_buffer dmab; };

diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 218e8bda6cae..7b283c2bb7b5 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -76,7 +76,6 @@ struct skl_dev { struct list_head ppl_list; struct list_head bind_list;

• const char *fw_name; char tplg_name[64]; unsigned short pci_id; const struct firmware *tplg;