Skip to content

plat: apl: clock gating adjusted for dai, dma, and cores #452

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

Merged
lgirdwood merged 2 commits into from
Oct 8, 2018
Merged

plat: apl: clock gating adjusted for dai, dma, and cores #452

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
f42c657
tplg: result of the free function propagated
Oct 8, 2018
1ba7b5e
plat: apl: clock gating adjusted for dai, dma, and cores
Oct 2, 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
5 changes: 0 additions & 5 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,6 @@ export ARCH_INCDIR

PLATFORM_INCDIR = -I $(SRC_DIR)/platform/$(PLATFORM)/include

if BUILD_CAVS
PLATFORM_INCDIR += \
-I $(SRC_DIR)/platform/intel/include
endif

if XCC
PLATFORM_INCDIR += \
-I $(ROOT_DIR)/arch/include
Expand Down
3 changes: 0 additions & 3 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,9 +566,6 @@ AC_CONFIG_FILES([
src/platform/icelake/include/platform/Makefile
src/platform/intel/Makefile
src/platform/intel/cavs/Makefile
src/platform/intel/include/Makefile
src/platform/intel/include/platform/Makefile
src/platform/intel/include/platform/cavs/Makefile
test/Makefile
test/cmocka/Makefile
])
Expand Down
25 changes: 11 additions & 14 deletions src/drivers/intel/baytrail/ssp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,11 @@ static int hweight_32(uint32_t mask)
/* empty SSP receive FIFO */
static void ssp_empty_rx_fifo(struct dai *dai)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);
uint32_t sssr;
uint32_t entries;
uint32_t i;

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

sssr = ssp_read(dai, SSSR);

Expand All @@ -77,7 +76,7 @@ static void ssp_empty_rx_fifo(struct dai *dai)
ssp_read(dai, SSDR);
}

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);
}

/* save SSP context prior to entering D3 */
Expand Down Expand Up @@ -132,7 +131,7 @@ static inline int ssp_set_config(struct dai *dai,
bool cbs = false;
int ret = 0;

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

/* is playback/capture already running */
if (ssp->state[DAI_DIR_PLAYBACK] == COMP_STATE_ACTIVE ||
Expand Down Expand Up @@ -473,22 +472,20 @@ static inline int ssp_set_config(struct dai *dai,
ssp->state[DAI_DIR_CAPTURE] = COMP_STATE_PREPARE;

out:
spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);

return ret;
}

/* Digital Audio interface formatting */
static inline int ssp_set_loopback_mode(struct dai *dai, uint32_t lbm)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);

trace_ssp("loo");
spin_lock(&ssp->lock);
spin_lock(&dai->lock);

ssp_update_bits(dai, SSCR1, SSCR1_LBM, lbm ? SSCR1_LBM : 0);

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);

return 0;
}
Expand All @@ -498,7 +495,7 @@ static void ssp_start(struct dai *dai, int direction)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

/* enable port */
ssp_update_bits(dai, SSCR0, SSCR0_SSE, SSCR0_SSE);
Expand All @@ -512,15 +509,15 @@ static void ssp_start(struct dai *dai, int direction)
else
ssp_update_bits(dai, SSCR1, SSCR1_RSRE, SSCR1_RSRE);

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);
}

/* stop the SSP for either playback or capture */
static void ssp_stop(struct dai *dai, int direction)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

/* stop Rx if neeed */
if (direction == DAI_DIR_CAPTURE &&
Expand Down Expand Up @@ -548,7 +545,7 @@ static void ssp_stop(struct dai *dai, int direction)
trace_ssp("Ss2");
}

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);
}

static int ssp_trigger(struct dai *dai, int cmd, int direction)
Expand Down Expand Up @@ -607,7 +604,7 @@ static int ssp_probe(struct dai *dai)
sizeof(*ssp));
dai_set_drvdata(dai, ssp);

spinlock_init(&ssp->lock);
spinlock_init(&dai->lock);

ssp->state[DAI_DIR_PLAYBACK] = COMP_STATE_READY;
ssp->state[DAI_DIR_CAPTURE] = COMP_STATE_READY;
Expand Down
4 changes: 3 additions & 1 deletion src/drivers/intel/cavs/clk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ uint32_t clock_set_freq(int clock, uint32_t hz)
notify_data.old_freq = clk_pdata->clk[clock].freq;
notify_data.old_ticks_per_msec = clk_pdata->clk[clock].ticks_per_msec;

/* atomic context for chaning clocks */
/* atomic context for changing clocks */
spin_lock_irq(&clk_pdata->clk[clock].lock, flags);

switch (clock) {
Expand All @@ -157,6 +157,8 @@ uint32_t clock_set_freq(int clock, uint32_t hz)
io_reg_update_bits(SHIM_BASE + SHIM_CLKCTL,
SHIM_CLKCTL_HDCS, 0);
#endif

/* TODO: should handle all cores or pass the index as arg */
Copy link
Copy Markdown
Member

@lgirdwood lgirdwood Oct 3, 2018

Choose a reason for hiding this comment

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

We should probably pass index in and combine with clock ID, e.g. CPU_CLOCK_CORE0, etc ?

io_reg_update_bits(SHIM_BASE + SHIM_CLKCTL,
SHIM_CLKCTL_DPCS_MASK(0), cpu_freq[idx].enc);

Expand Down
21 changes: 13 additions & 8 deletions src/drivers/intel/cavs/dmic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <sof/stream.h>
#include <sof/dmic.h>
#include <sof/interrupt.h>
#include <sof/pm_runtime.h>
#include <sof/math/numbers.h>
#include <sof/audio/format.h>

Expand Down Expand Up @@ -207,7 +208,7 @@ static uint64_t dmic_work(void *data, uint64_t delay)
int i;

tracev_dmic("wrk");
spin_lock(&dmic->lock);
spin_lock(&dai->lock);

/* Increment gain with logaritmic step.
* Gain is Q2.30 and gain modifier is Q2.14.
Expand Down Expand Up @@ -257,7 +258,7 @@ static uint64_t dmic_work(void *data, uint64_t delay)
dmic_write(dai, base[i] + OUT_GAIN_RIGHT_B, val);
}
}
spin_unlock(&dmic->lock);
spin_unlock(&dai->lock);

if (gval)
return DMIC_UNMUTE_RAMP_US;
Expand Down Expand Up @@ -1236,7 +1237,7 @@ static void dmic_start(struct dai *dai)
int fir_b;

/* enable port */
spin_lock(&dmic->lock);
spin_lock(&dai->lock);
trace_dmic("sta");
dmic->state = COMP_STATE_ACTIVE;
dmic->startcount = 0;
Expand Down Expand Up @@ -1305,7 +1306,7 @@ static void dmic_start(struct dai *dai)
CIC_CONTROL_SOFT_RESET_BIT, 0);
}

spin_unlock(&dmic->lock);
spin_unlock(&dai->lock);

/* Currently there's no DMIC HW internal mutings and wait times
* applied into this start sequence. It can be implemented here if
Expand All @@ -1324,7 +1325,7 @@ static void dmic_stop(struct dai *dai)
int i;

trace_dmic("sto")
spin_lock(&dmic->lock);
spin_lock(&dai->lock);
dmic->state = COMP_STATE_PREPARE;

/* Stop FIFO packers and set FIFO initialize bits */
Expand Down Expand Up @@ -1352,7 +1353,7 @@ static void dmic_stop(struct dai *dai)
FIR_CONTROL_B_MUTE_BIT);
}

spin_unlock(&dmic->lock);
spin_unlock(&dai->lock);
}

/* save DMIC context prior to entering D3 */
Expand Down Expand Up @@ -1446,6 +1447,12 @@ static int dmic_probe(struct dai *dai)

trace_dmic("pro");

if (dai_get_drvdata(dai))
return -EEXIST; /* already created */

/* Disable dynamic clock gating for dmic before touching any reg */
pm_runtime_get_sync(DMIC_CLK, dai->index);

/* allocate private data */
dmic = rzalloc(RZONE_SYS | RZONE_FLAG_UNCACHED, SOF_MEM_CAPS_RAM,
sizeof(*dmic));
Expand All @@ -1455,8 +1462,6 @@ static int dmic_probe(struct dai *dai)
}
dai_set_drvdata(dai, dmic);

spinlock_init(&dmic->lock);

/* Set state, note there is no playback direction support */
dmic->state = COMP_STATE_READY;

Expand Down
8 changes: 6 additions & 2 deletions src/drivers/intel/cavs/hda-dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,13 +612,17 @@ static int hda_dma_probe(struct dma *dma)
int i;
struct hda_chan_data *chan;

trace_event(TRACE_CLASS_DMA, "hda-dma-probe %p id %d",
(uintptr_t)dma, dma->plat_data.id);

if (dma_get_drvdata(dma))
Copy link
Copy Markdown
Member

@lgirdwood lgirdwood Oct 3, 2018

Choose a reason for hiding this comment

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

What's the use case here ? I guess topology could create the device twice in error ?

Copy link
Copy Markdown
Author

@mmaka1 mmaka1 Oct 3, 2018

Choose a reason for hiding this comment

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

(also applies to the multiple probe question) my assumptions:

  • some devices may be created on init, some on demand, the client does not need to know whether the probe() must be called before get() or not, otherwise may try to probe the device that is already created.
  • there is no api to 'return' the device back to the pool (calling remove() implicitly inside for some of them) yet, so get-probe sequences may be called again by a destroyed and then re-created topology component.

Copy link
Copy Markdown
Author

@mmaka1 mmaka1 Oct 3, 2018

Choose a reason for hiding this comment

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

Actually the get() could check whether the device exists before calling probe() and the get_drvdata() test could stay there but result in error (probe had not been protected against double allocation before I guess).

Copy link
Copy Markdown
Member

@lgirdwood lgirdwood Oct 3, 2018

Choose a reason for hiding this comment

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

ok thanks, I see - I think the simplest approach for us would be to only probe() devices when added by topology and remove() them when topology is unloaded. This probably can be summarized with some rules (that we could also document).

  1. we dont automatically probe() any pipeline device unless its's added by topology.
  2. we only remove component device when it's unloaded by topology.
  3. component added twice would return an error.
  4. active component removal would return an error.
  5. pipeline trigger start will fail if pipeline has no valid sink and source endpoints.

The driver would manage the component load/unload and ref count components in pipelines that shared components. @ranj063 is working on driver code for this shortly (PM related)

The thing we have to watch for is non pipeline IO drivers like low level DMACs, SSP, these would need reference counted by host.c and dai.c respectively.

Copy link
Copy Markdown
Author

@mmaka1 mmaka1 Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@lgirdwood Agree. [Edit: missed last paragraph, removed re-phrased, similar conclusion].
However reference counting in host.c and dai.c might not be sufficient if you add another client and there would be a need for a more "global" reference counting. How about doing this inside specific device driver (dw-dma.c for example)? This approach would require each client (host, dai) to "put" the device, but I planned to add dai_put(), dma_put() calls anyway. Atm there is dma_get-channel_get-channel_put sequence implemented and I planned to balance them inside the _free() ops.

Copy link
Copy Markdown
Author

@mmaka1 mmaka1 Oct 5, 2018

Choose a reason for hiding this comment

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

@lgirdwood Please take a look at the latest implementation that handles ref counting (simple one, protected by early initialized locks) in the lib layer. Done for the get() side, to be completed on the put() side if this is acceptable direction.

Copy link
Copy Markdown
Member

@lgirdwood lgirdwood Oct 6, 2018

Choose a reason for hiding this comment

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

I like the locking update, it's a needed simplification, but still not sure about calling probe() twice as it diverges from the Linux model. It could also be due to a recent? change to the probe() API that

static int hda_dma_probe(struct dma *dma)

returns an int instead of returning a "struct device *". If we returned a struct dma device (which can contain a generic base structure) the we can managed devices at a global level.

return -EEXIST; /* already created */

/* allocate private data */
hda_pdata = rzalloc(RZONE_SYS | RZONE_FLAG_UNCACHED, SOF_MEM_CAPS_RAM,
sizeof(*hda_pdata));
dma_set_drvdata(dma, hda_pdata);

spinlock_init(&dma->lock);

/* init channel status */
chan = hda_pdata->chan;

Expand Down
37 changes: 19 additions & 18 deletions src/drivers/intel/cavs/ssp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include <sof/ssp.h>
#include <sof/alloc.h>
#include <sof/interrupt.h>
#include <sof/pm_runtime.h>
#include <sof/math/numbers.h>
#include <config.h>

Expand Down Expand Up @@ -65,29 +66,27 @@ static int hweight_32(uint32_t mask)
/* empty SSP transmit FIFO */
static void ssp_empty_tx_fifo(struct dai *dai)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);
uint32_t sssr;

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

sssr = ssp_read(dai, SSSR);

/* clear interrupt */
if (sssr & SSSR_TUR)
ssp_write(dai, SSSR, sssr);

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);
}

/* empty SSP receive FIFO */
static void ssp_empty_rx_fifo(struct dai *dai)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);
uint32_t sssr;
uint32_t entries;
uint32_t i;

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

sssr = ssp_read(dai, SSSR);

Expand All @@ -102,7 +101,7 @@ static void ssp_empty_rx_fifo(struct dai *dai)
ssp_read(dai, SSDR);
}

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);
}

/* save SSP context prior to entering D3 */
Expand Down Expand Up @@ -171,7 +170,7 @@ static inline int ssp_set_config(struct dai *dai,
bool start_delay = false;
int ret = 0;

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

/* is playback/capture already running */
if (ssp->state[DAI_DIR_PLAYBACK] == COMP_STATE_ACTIVE ||
Expand Down Expand Up @@ -733,22 +732,20 @@ static inline int ssp_set_config(struct dai *dai,
ssp->state[DAI_DIR_CAPTURE] = COMP_STATE_PREPARE;

out:
spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);

return ret;
}

/* Digital Audio interface formatting */
static inline int ssp_set_loopback_mode(struct dai *dai, uint32_t lbm)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);

trace_ssp("loo");
spin_lock(&ssp->lock);
spin_lock(&dai->lock);

ssp_update_bits(dai, SSCR1, SSCR1_LBM, lbm ? SSCR1_LBM : 0);

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);

return 0;
}
Expand All @@ -758,7 +755,7 @@ static void ssp_start(struct dai *dai, int direction)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

/* enable port */
ssp_update_bits(dai, SSCR0, SSCR0_SSE, SSCR0_SSE);
Expand All @@ -775,15 +772,15 @@ static void ssp_start(struct dai *dai, int direction)
ssp_update_bits(dai, SSRSA, 0x1 << 8, 0x1 << 8);
}

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);
}

/* stop the SSP for either playback or capture */
static void ssp_stop(struct dai *dai, int direction)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);

spin_lock(&ssp->lock);
spin_lock(&dai->lock);

/* wait to get valid fifo status */
wait_delay(PLATFORM_SSP_STOP_DELAY);
Expand Down Expand Up @@ -817,7 +814,7 @@ static void ssp_stop(struct dai *dai, int direction)
trace_ssp("Ss2");
}

spin_unlock(&ssp->lock);
spin_unlock(&dai->lock);
}

static int ssp_trigger(struct dai *dai, int cmd, int direction)
Expand Down Expand Up @@ -871,13 +868,17 @@ static int ssp_probe(struct dai *dai)
{
struct ssp_pdata *ssp;

if (dai_get_drvdata(dai))
return -EEXIST; /* already created */

/* Disable dynamic clock gating before touching any register */
pm_runtime_get_sync(SSP_CLK, dai->index);

/* allocate private data */
ssp = rzalloc(RZONE_SYS | RZONE_FLAG_UNCACHED, SOF_MEM_CAPS_RAM,
sizeof(*ssp));
dai_set_drvdata(dai, ssp);

spinlock_init(&ssp->lock);

ssp->state[DAI_DIR_PLAYBACK] = COMP_STATE_READY;
ssp->state[DAI_DIR_CAPTURE] = COMP_STATE_READY;

Expand Down
Loading