| Message ID | 20190822190425.23001-30-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Clenaup SST initialization | expand |
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 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. >> 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-26 18:39, Pierre-Louis Bossart wrote: > > > 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/? > In essence, most of what is required is actually done even in existing /skylake init. How it is done leaves a lot to be desired, though. Initialization is cleaned up to improve code quality and make it look cohesive - removal of duplications, usage of sst-framework functions and so on. Followup segments - including but not limited to power-management - touch this stuff quite a bit. With initialization updated, each power-management patch fixes single spot rather than 3 (skl-sst, bxt-sst, cnl-sst). Should probe more readable and easy to review. That's why I described it via 'ill'. Although I do agree, could have chosen a better description.
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);
To this date Skylake SST were following ill flow of initialization by 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(-)