Skip to content

Audio DSP power gating and clock gating enable #247

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
wants to merge 2 commits into from
Closed

Audio DSP power gating and clock gating enable #247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc636d3
ASoC: SOF: enable clock and power gating after fw boot
ranj063 Nov 6, 2018
8088a46
ASOC: SOF: set clock and power gating ops for SKL+ platforms
ranj063 Nov 6, 2018
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
1 change: 1 addition & 0 deletions sound/soc/sof/intel/apl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,6 @@ struct snd_sof_dsp_ops sof_apl_ops = {
.resume = hda_dsp_resume,
.runtime_suspend = hda_dsp_runtime_suspend,
.runtime_resume = hda_dsp_runtime_resume,
.clock_power_gating = hda_dsp_ctrl_clock_power_gating,
};
EXPORT_SYMBOL(sof_apl_ops);
19 changes: 19 additions & 0 deletions sound/soc/sof/intel/hda-ctrl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <sound/sof.h>
#include <sound/pcm_params.h>
#include <linux/pm_runtime.h>
#include <sound/hda_register.h>

#include "../sof-priv.h"
#include "../ops.h"
Expand Down Expand Up @@ -148,6 +149,24 @@ void hda_dsp_ctrl_misc_clock_gating(struct snd_sof_dev *sdev, bool enable)
snd_sof_pci_update_bits(sdev, PCI_CGCTL, PCI_CGCTL_MISCBDCGE_MASK, val);
}

void hda_dsp_ctrl_clock_power_gating(struct snd_sof_dev *sdev, bool enable)
{
struct hdac_bus *bus = sof_to_bus(sdev);
u32 val;

/* Update PDCGE bit of CGCTL register */
val = enable ? PCI_CGCTL_ADSPDCGE : 0;
Copy link

@keyonjie keyonjie Nov 6, 2018

Choose a reason for hiding this comment

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

I think this is opposite for PCI_TCSEL_ADSPPGD?

Be careful that some bits are gating disable(e.g. ADSPPGD), some others are gating enable(e.g. PCI_CGCTL_ADSPDCGE), we need to handle them carefully.

Copy link
Collaborator Author

@ranj063 ranj063 Nov 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@keyonjie yes, but the code does the right thing here. When we want to enable clock and power gating we write 1 to PCI_CGCTL_ADSPDCGE in CGCTL and write 0 to PCI_PGCTL BIT(2).

snd_sof_pci_update_bits(sdev, PCI_CGCTL, PCI_CGCTL_ADSPDCGE, val);

/* Update L1SEN bit of EM2 register */
val = enable ? SOF_HDA_VS_EM2_L1SEN : 0;
snd_hdac_chip_updatel(bus, VS_EM2, SOF_HDA_VS_EM2_L1SEN, val);

/* Update ADSPPGD bit of PGCTL register */
val = enable ? 0 : PCI_TCSEL_ADSPPGD;
snd_sof_pci_update_bits(sdev, PCI_TCSEL, PCI_TCSEL_ADSPPGD, val);
}

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/*
* While performing reset, controller may not come back properly and causing
Expand Down
6 changes: 6 additions & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@
#define PCI_TCSEL 0x44
#define PCI_CGCTL 0x48

Copy link

@keyonjie keyonjie Nov 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

better to correct naming of these 2 registers:

#define PCI_PGCTL			0x44
#define PCI_CGCTL			0x48

/* PCI_TVSEL bits */
#define PCI_TCSEL_ADSPPGD BIT(2)

Copy link

@keyonjie keyonjie Nov 6, 2018

Choose a reason for hiding this comment

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

rename this to PCI_PGCTL_ADSPPGD

/* PCI_CGCTL bits */
#define PCI_CGCTL_MISCBDCGE_MASK BIT(6)
#define PCI_CGCTL_LSRMD_MASK BIT(4)
Copy link

@keyonjie keyonjie Nov 6, 2018

Choose a reason for hiding this comment

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

this bit should be PGCTL one, rename and move it to group /* PCI_PGCTL bits */

Copy link
Collaborator Author

@ranj063 ranj063 Nov 6, 2018

Choose a reason for hiding this comment

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

sure, that will avoid confusion!

#define PCI_CGCTL_ADSPDCGE BIT(1)

/* Legacy HDA registers and bits used - widths are variable */
#define SOF_HDA_GCAP 0x0
Expand All @@ -32,6 +36,7 @@
#define SOF_HDA_WAKESTS 0x0E
#define SOF_HDA_WAKESTS_INT_MASK ((1 << 8) - 1)
#define SOF_HDA_RIRBSTS 0x5d
#define SOF_HDA_VS_EM2_L1SEN BIT(13)

/* SOF_HDA_GCTL register bist */
#define SOF_HDA_GCTL_RESET BIT(0)
Expand Down Expand Up @@ -490,6 +495,7 @@ int hda_dsp_cl_boot_firmware_skl(struct snd_sof_dev *sdev);
int hda_dsp_ctrl_get_caps(struct snd_sof_dev *sdev);
int hda_dsp_ctrl_link_reset(struct snd_sof_dev *sdev);
void hda_dsp_ctrl_misc_clock_gating(struct snd_sof_dev *sdev, bool enable);
void hda_dsp_ctrl_clock_power_gating(struct snd_sof_dev *sdev, bool enable);
int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool full_reset);

/*
Expand Down
6 changes: 6 additions & 0 deletions sound/soc/sof/loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@ int snd_sof_run_firmware(struct snd_sof_dev *sdev)

dev_dbg(sdev->dev, "booting DSP firmware\n");

/* disable clock power gating */
snd_sof_dsp_clock_power_gating(sdev, false);

/* boot the firmware on the DSP */
ret = snd_sof_dsp_run(sdev);
if (ret < 0) {
Expand All @@ -305,6 +308,9 @@ int snd_sof_run_firmware(struct snd_sof_dev *sdev)

dev_info(sdev->dev, "firmware boot complete\n");

/* enable clock power gating */
snd_sof_dsp_clock_power_gating(sdev, true);

return 0;
}
EXPORT_SYMBOL(snd_sof_run_firmware);
Expand Down
7 changes: 7 additions & 0 deletions sound/soc/sof/ops.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,13 @@ static inline int snd_sof_dsp_runtime_suspend(struct snd_sof_dev *sdev,
return 0;
}

static inline void snd_sof_dsp_clock_power_gating(struct snd_sof_dev *sdev,
bool enable)
{
if (sdev->ops->clock_power_gating)
sdev->ops->clock_power_gating(sdev, enable);
}

static inline int snd_sof_dsp_set_clk(struct snd_sof_dev *sdev, u32 freq)
{
if (sdev->ops->set_clk)
Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ struct snd_sof_dsp_ops {
int (*resume)(struct snd_sof_dev *sof_dev);
int (*runtime_suspend)(struct snd_sof_dev *sof_dev, int state);
int (*runtime_resume)(struct snd_sof_dev *sof_dev);
void (*clock_power_gating)(struct snd_sof_dev *sof_dev, bool enable);
Copy link
Member

@lgirdwood lgirdwood Nov 6, 2018

Choose a reason for hiding this comment

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

We should name this a little more generic so we can handle more than just clocks. It does look like here we are entering an idle state ? If so snd_sof_dsp_idle(sdev, bool idle) . Please also don't name the variable enable here, best to use the verb name.

Copy link
Collaborator Author

@ranj063 ranj063 Nov 6, 2018

Choose a reason for hiding this comment

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

@lgirdwood we are enabling clock gating and power for the audio DSP here i.e. the HW can power gate when it is idle. So calling it idle would be misleading as typically in the context of power management, idle would indicate we are decrementing the ref count.

Copy link
Member

@lgirdwood lgirdwood Nov 6, 2018

Choose a reason for hiding this comment

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

@ranj063 is the DSP HW in D3 state here when we gate the clocks ?If so, shouldn't this be part of suspend/resume calls at the HW abstraction level.

Copy link
Member

@plbossart plbossart Nov 6, 2018

Choose a reason for hiding this comment

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

@ranj063 I am also not clear on the DSP state. If you do power gating why do you need clock gating? Isn't it more when you are in D0 (or D0ix) that you allow the clocks to be gated to allow parts of the hardware to save power?

Copy link
Collaborator Author

@ranj063 ranj063 Nov 6, 2018

Choose a reason for hiding this comment

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

@ranj063 is the DSP HW in D3 state here when we gate the clocks ?If so, shouldn't this be part of suspend/resume calls at the HW abstraction level.

@lgirdwood the HW is not in D3 here. And we not performing power gating or clock gating here either. We are just enabling them meaning that we are indicating to the HW that if the audio dsp is idle, it should be clocked gated and power gated. When the audio device is idle and in D3, these bits will enable the HW to power gate/clock gate.

Before we boot the firmware, we disable power gating during dsp probe. So if we dont re-enable it, I think we will never be able to power gate the audio dsp.

Copy link
Collaborator Author

@ranj063 ranj063 Nov 6, 2018

Choose a reason for hiding this comment

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

@ranj063 I am also not clear on the DSP state. If you do power gating why do you need clock gating? Isn't it more when you are in D0 (or D0ix) that you allow the clocks to be gated to allow parts of the hardware to save power?

@plbossart we are just enabling the HW to be able to do clock gating and power gating here. The dsp is not in D3 after these bits are set. But when an opportunity arises the HW will do the needful if these bits are set.

Copy link
Collaborator Author

@ranj063 ranj063 Nov 6, 2018

Choose a reason for hiding this comment

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

@lgirdwood @plbossart btw i didnt invent anything new here. I just copied it from the skylake driver. We seem to have missed this sequence earlier.

Copy link
Member

@lgirdwood lgirdwood Nov 6, 2018

Choose a reason for hiding this comment

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

@ranj063 ok, so setting these bits allows the HW to opportunistically gate clocks/PM resources when the FW is idle after boot is complete ? If so, should probably be part of the existing HW abstraction ops for fw ready and boot ?

Copy link
Collaborator Author

@ranj063 ranj063 Nov 6, 2018

Choose a reason for hiding this comment

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

@lgirdwood thats correct. I think it makes sense to make it part of the "run" ops. Let me make the change.


/* DSP clocking */
int (*set_clk)(struct snd_sof_dev *sof_dev, u32 freq);
Expand Down