Skip to content

[RFC]ASoC: SOF: acpi: enable PM for ACPI platforms #1295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ranj063 wants to merge 1 commit into from
Closed

[RFC]ASoC: SOF: acpi: enable PM for ACPI platforms #1295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions sound/soc/sof/intel/byt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,26 @@ static int byt_reset(struct snd_sof_dev *sdev)
return 0;
}

static int byt_suspend(struct snd_sof_dev *sdev)
{
/* put DSP into reset, set reset vector and stall */
snd_sof_dsp_update_bits64(sdev, BYT_DSP_BAR, SHIM_CSR,
SHIM_BYT_CSR_RST | SHIM_BYT_CSR_VECTOR_SEL |
SHIM_BYT_CSR_STALL,
SHIM_BYT_CSR_RST | SHIM_BYT_CSR_VECTOR_SEL |
SHIM_BYT_CSR_STALL);

return 0;
}

static int byt_resume(struct snd_sof_dev *sdev)
{
/* enable Interrupt from both sides */
snd_sof_dsp_update_bits64(sdev, BYT_DSP_BAR, SHIM_IMRX, 0x3, 0x0);
snd_sof_dsp_update_bits64(sdev, BYT_DSP_BAR, SHIM_IMRD, 0x3, 0x0);
return 0;
}

/* Baytrail DAIs */
static struct snd_soc_dai_driver byt_dai[] = {
{
Expand Down Expand Up @@ -529,6 +549,10 @@ const struct snd_sof_dsp_ops sof_tng_ops = {
/*Firmware loading */
.load_firmware = snd_sof_load_firmware_memcpy,

/* PM */
.suspend = byt_suspend,
.resume = byt_resume,

/* DAI drivers */
.drv = byt_dai,
.num_drv = 3, /* we have only 3 SSPs on byt*/
Expand Down Expand Up @@ -690,6 +714,10 @@ const struct snd_sof_dsp_ops sof_byt_ops = {
/*Firmware loading */
.load_firmware = snd_sof_load_firmware_memcpy,

/* PM */
.suspend = byt_suspend,
.resume = byt_resume,

/* DAI drivers */
.drv = byt_dai,
.num_drv = 3, /* we have only 3 SSPs on byt*/
Expand Down Expand Up @@ -749,6 +777,10 @@ const struct snd_sof_dsp_ops sof_cht_ops = {
/*Firmware loading */
.load_firmware = snd_sof_load_firmware_memcpy,

/* PM */
.suspend = byt_suspend,
.resume = byt_resume,
Copy link
Member

@plbossart plbossart Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the same routine for basic PM and runtime PM?


/* DAI drivers */
.drv = byt_dai,
/* all 6 SSPs may be available for cherrytrail */
Expand Down
8 changes: 0 additions & 8 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,14 +732,6 @@ static int sof_pcm_probe(struct snd_soc_component *component)
return ret;
}

/*
* Some platforms in SOF, ex: BYT, may not have their platform PM
* callbacks set. Increment the usage count so as to
* prevent the device from entering runtime suspend.
*/
if (!sof_ops(sdev)->runtime_suspend || !sof_ops(sdev)->runtime_resume)
pm_runtime_get_noresume(sdev->dev);

return ret;
}

Expand Down
12 changes: 9 additions & 3 deletions sound/soc/sof/pm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,10 @@ static int sof_resume(struct device *dev, bool runtime_resume)
int ret;

/* do nothing if dsp resume callbacks are not set */
if (!sof_ops(sdev)->resume || !sof_ops(sdev)->runtime_resume)
if (runtime_resume && !sof_ops(sdev)->runtime_resume)
return 0;

if (!runtime_resume && !sof_ops(sdev)->resume)
return 0;

/*
Expand Down Expand Up @@ -337,8 +340,11 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
struct snd_sof_dev *sdev = dev_get_drvdata(dev);
int ret;

/* do nothing if dsp suspend callback is not set */
if (!sof_ops(sdev)->suspend)
/* do nothing if dsp suspend callbacks are not set */
if (!runtime_suspend && !sof_ops(sdev)->suspend)
return 0;

if (runtime_suspend && !sof_ops(sdev)->runtime_suspend)
Copy link
Member

@plbossart plbossart Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be a separate patch, no?

return 0;

/* release trace */
Expand Down
47 changes: 47 additions & 0 deletions sound/soc/sof/sof-acpi-dev.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <linux/firmware.h>
#include <linux/module.h>
#include <linux/pm_runtime.h>
#include <linux/dmi.h>
#include <sound/soc-acpi.h>
#include <sound/soc-acpi-intel-match.h>
#include <sound/sof.h>
Expand Down Expand Up @@ -117,6 +118,39 @@ static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
.arch_ops = &sof_xtensa_arch_ops
};

static int minnowboard_quirk_cb(const struct dmi_system_id *id)
{
struct device *dev = id->driver_data;

/*
* Increment the usage count so as to prevent the device
* from entering runtime suspend.
*/
pm_runtime_get_noresume(dev);
Copy link
Member

@plbossart plbossart Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw what happens if instead of taking a fake reference you just don't enable pm_runtime?

Copy link
Collaborator Author

@ranj063 ranj063 Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart this is what I want to do ideally. But the one test that fails with this change is the ipc_flood_test(). When this test runs, we call pm_runtime_get_sync() to resume the DSP if it is suspended and this throws an error is runtime_pm is not enabled.
So if I add a check to see if runtime PM is enabled before calling get_sync(), then it would solve the problem. And of course, every other place we add a pm_runtime_get_sync() would need to the same too. What do you think?


return 1;
}

static struct dmi_system_id minnowbard_quirk_table[] = {
{
/* Minnowboard Max B3 */
.callback = minnowboard_quirk_cb,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "Circuitco"),
DMI_MATCH(DMI_PRODUCT_NAME,
"Minnowboard Max B3 PLATFORM"),
},
},
{
/* Minnowboard Turbot */
.callback = minnowboard_quirk_cb,
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "ADI"),
DMI_MATCH(DMI_PRODUCT_NAME, "Minnowboard Turbot"),
},
},
};

#endif

static const struct dev_pm_ops sof_acpi_pm = {
Expand All @@ -127,13 +161,26 @@ static const struct dev_pm_ops sof_acpi_pm = {

static void sof_acpi_probe_complete(struct device *dev)
{
int i;

if (sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME)
return;

/* allow runtime_pm */
pm_runtime_set_autosuspend_delay(dev, SND_SOF_SUSPEND_DELAY_MS);
pm_runtime_use_autosuspend(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
pm_runtime_mark_last_busy(dev);

#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
/* set driver data in the quirk table */
for (i = 0; i < ARRAY_SIZE(minnowbard_quirk_table); i++)
minnowbard_quirk_table[i].driver_data = dev;
Copy link
Member

@plbossart plbossart Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit ugly, no? You could just check the quirk, which sets a variable and then you test said variable.

Copy link
Member

@plbossart plbossart Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and you can also use the sof-acpi-debug kernel parameter to override the DMI and also disable PM!


/* check for Minnowboards */
dmi_check_system(minnowbard_quirk_table);
#endif
}

static int sof_acpi_probe(struct platform_device *pdev)
Expand Down