Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 drivers/dai/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: Apache-2.0

add_subdirectory_ifdef(CONFIG_DAI_INTEL_SSP intel/ssp)
add_subdirectory_ifdef(CONFIG_DAI_INTEL_ALH intel/alh)
1 change: 1 addition & 0 deletions drivers/dai/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,6 @@ source "subsys/logging/Kconfig.template.log_config"
comment "Device Drivers"

source "drivers/dai/intel/ssp/Kconfig.ssp"
source "drivers/dai/intel/alh/Kconfig.alh"

endif # DAI
5 changes: 5 additions & 0 deletions drivers/dai/intel/alh/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# SPDX-License-Identifier: Apache-2.0

zephyr_library()
zephyr_library_sources(alh.c)
zephyr_library_compile_options(-std=gnu99)
24 changes: 24 additions & 0 deletions drivers/dai/intel/alh/Kconfig.alh
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# SOF ALH configuration options

# Copyright (c) 2022 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

DT_COMPAT_INTEL_ALH_DAI := intel,alh-dai

config DAI_INTEL_ALH
bool "Intel ALH driver for Dai interface"
default $(dt_compat_enabled,$(DT_COMPAT_INTEL_ALH_DAI))
select DMA
help
Copy link
Member

Choose a reason for hiding this comment

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

If you add this before config,

DT_COMPAT_INTEL_ALH_DAI := intel,alh-dai

and this inside config

default $(dt_compat_enabled,$(DT_COMPAT_INTEL_ALH_DAI))

This will allow the driver to be enabled whenever a devicetree node has the compatible value is enabled. This will allow CI to build this driver for cAVS 2.5

Copy link
Member Author

@abonislawski abonislawski May 26, 2022

Choose a reason for hiding this comment

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

Sure, we can do it later but for now @juimonen requested to not enabling this driver until sof can use it (changes in review)

Copy link
Member

Choose a reason for hiding this comment

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

No, it needs to be enabled somewhere before we can merge it. Otherwise, it will not be included in CI and we won't know if it even builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

@abonislawski @juimonen I think we can proceed with a) a test case, b) enable it for cAVS2.5 SOF build, c) add a new board file using this.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dcpleung @MaureenHelm driver enabled again as you requested, hopefully it will pass CI :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @abonislawski

Select this to enable Intel ALH driver.
The ALH is an intermediary device, which acts as a hub and provides an
abstracted support for numerous sound interfaces (e.g. SoundWire).

if DAI_INTEL_ALH

config DAI_ALH_HAS_OWNERSHIP
bool "Intel ALH driver has ownership"
help
Select this to enable programming HW ownership

endif
201 changes: 201 additions & 0 deletions drivers/dai/intel/alh/alh.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
/*
* Copyright (c) 2022 Intel Corporation.
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <errno.h>
#include <stdbool.h>
#include <stdint.h>
#include <zephyr/spinlock.h>
#include <zephyr/devicetree.h>
#include <zephyr/logging/log.h>

#define DT_DRV_COMPAT intel_alh_dai
#define LOG_DOMAIN dai_intel_alh

LOG_MODULE_REGISTER(LOG_DOMAIN);

#include "alh.h"

/* Digital Audio interface formatting */
static int dai_alh_set_config_tplg(struct dai_intel_alh *dp, const void *spec_config)
{
struct dai_intel_alh_pdata *alh = dai_get_drvdata(dp);
const struct dai_intel_ipc3_alh_params *config = spec_config;

if (config->channels && config->rate) {
alh->params.channels = config->channels;
alh->params.rate = config->rate;
LOG_INF("%s channels %d rate %d", __func__, config->channels, config->rate);
}

alh->params.stream_id = config->stream_id;

return 0;
}

static int dai_alh_set_config_blob(struct dai_intel_alh *dp, const void *spec_config)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This API is also sort of confusing. Here, the channels/rate values are just set to defaults, and the stream_id is set via an alternative struct passed as an argument in spec_config.

So basically, we have two different "set_config" methods, both taking an argument of the same name ("spec_config") and type ("void *"), except that they actually point to two entirely different structs from which set set disjoint configuration values?

Can we clean this up a bit so it's clear what's being sourced from where? Maybe you just want a set of "set_{channels,rate,stream_id}" methods, and put the glue code about "tplg" and "blobs" up in the app layer?

Copy link
Contributor

Choose a reason for hiding this comment

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

This again is mirroring the design in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/dai/intel/ssp/ssp.c . We in middle of a transition with two ways to pass the configuration details and ALH has the same problem as SSP.
We have have ongoing discussion (with @lgirdwood , @juimonen and @abonislawski ) on how much app side wrapper/glue code we need for each driver. With this approach, we don't really need any glue code, but if we have the blob-to-different API conversion on app side, we need to a SW component on app side for each Zephyr driver, and this has been something we've wanted to avoid.

struct dai_intel_alh_pdata *alh = dai_get_drvdata(dp);
const struct dai_intel_ipc4_alh_configuration_blob *blob = spec_config;
const struct ipc4_alh_multi_gtw_cfg *alh_cfg = &blob->alh_cfg;

alh->params.channels = ALH_CHANNELS_DEFAULT;
alh->params.rate = ALH_RATE_DEFAULT;
/* the LSB 8bits are for stream id */
alh->params.stream_id = alh_cfg->mapping[0].alh_id & 0xff;

return 0;
}

static int dai_alh_trigger(const struct device *dev, enum dai_dir dir,
enum dai_trigger_cmd cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Most drivers are OK with a NULL pointer in an API struct being treated as a noop. Having an empty function is kinda clumsy and wastes code space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to remove this but unfortunately dai api is not checking NULL for trigger. I think this can be added as another improvement to future dai PR

{
LOG_DBG("cmd %d", cmd);

return 0;
}

static void alh_claim_ownership(void)
{
#if CONFIG_DAI_ALH_HAS_OWNERSHIP
uint32_t ALHASCTL = DT_INST_PROP_BY_IDX(0, reg, 0);
uint32_t ALHCSCTL = DT_INST_PROP_BY_IDX(0, reg, 1);
Comment on lines +63 to +64
Copy link
Member

Choose a reason for hiding this comment

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

Why is this hard coded to instance 0? Will this driver work for multiple instances?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are the same for every instance so it doesnt matter but how to do it in more clean way?

Choose a reason for hiding this comment

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

For starters, it is recommended to comment usage of a hardcode.


sys_write32(sys_read32(ALHASCTL) | ALHASCTL_OSEL(0x3), ALHASCTL);
sys_write32(sys_read32(ALHCSCTL) | ALHASCTL_OSEL(0x3), ALHCSCTL);
#endif
}

static void alh_release_ownership(void)
{
#if CONFIG_DAI_ALH_HAS_OWNERSHIP
uint32_t ALHASCTL = DT_INST_PROP_BY_IDX(0, reg, 0);
uint32_t ALHCSCTL = DT_INST_PROP_BY_IDX(0, reg, 1);
Comment on lines +74 to +75
Copy link
Member

Choose a reason for hiding this comment

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

Same question about hard coded instance


sys_write32(sys_read32(ALHASCTL) | ALHASCTL_OSEL(0), ALHASCTL);
sys_write32(sys_read32(ALHCSCTL) | ALHASCTL_OSEL(0), ALHCSCTL);
#endif
}


static const struct dai_config *dai_alh_config_get(const struct device *dev, enum dai_dir dir)
{
struct dai_config *params = (struct dai_config *)dev->config;
struct dai_intel_alh *dp = (struct dai_intel_alh *)dev->data;
struct dai_intel_alh_pdata *alh = dai_get_drvdata(dp);

params->rate = alh->params.rate;
params->channels = alh->params.channels;
params->word_size = ALH_WORD_SIZE_DEFAULT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also confused here: this is assigning into the device-owned config struct, which is (almost always) a read-only struct containing static configuration (and on many platforms is in fact in .rodata and unwritable!). And it's then returning it to the caller, which is a racy API (by the time the caller reads it, the device may have changed the values, or someone else may have called config_get).

Almost certainly you want to pass a caller-owned struct pointer here and fill values into that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no immediate take on the pro/cons of the design here, but this PR is following the design that was agreed a couple weeks back and implemented in upstream Zephyr in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/dai/intel/ssp/ssp.c . I think it would be easier address DAI interface issues in a separate PR. Given we have existing SSP driver as the model, it seems fair for this ALH driver to follow the same approach.

Copy link
Member Author

@abonislawski abonislawski May 25, 2022

Choose a reason for hiding this comment

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

Yes, this just followed already upstreamed and reviewed approach in dai ssp so I think we shouldnt change that just for ALH and also cant (dai api needs to be changed) but I can agree @andyross proposal looks like a great improvement which we can address in separate PR to dai api and all drivers.


return params;
}

static int dai_alh_config_set(const struct device *dev, const struct dai_config *cfg,
const void *bespoke_cfg)
{
struct dai_intel_alh *dp = (struct dai_intel_alh *)dev->data;

LOG_DBG("%s", __func__);

if (cfg->type == DAI_INTEL_ALH) {
return dai_alh_set_config_tplg(dp, bespoke_cfg);
} else {
return dai_alh_set_config_blob(dp, bespoke_cfg);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yikes the two set_config variants with the different types collide right here, where you have a single function that needs to be passed a different argument type based on a type field the caller needs to set in another argument. Yeah, I think this really needs to be cleaned up. I only see three fields that need to be set, surely there's a simpler way to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I fully agree, I have discussed this with the team and we want to change dai api for config so all the tplg/blob stuff will be moved to app level with only one common config in driver

Choose a reason for hiding this comment

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

Regardless of the approach chosen, please avoid if-return else-return blocks. The 'else' is not needed at all, just return immediately.

}

static const struct dai_properties *dai_alh_get_properties(const struct device *dev,
enum dai_dir dir, int stream_id)
{
struct dai_intel_alh *dp = (struct dai_intel_alh *)dev->data;
struct dai_intel_alh_pdata *alh = dai_get_drvdata(dp);
struct dai_properties *prop = &alh->props;
uint32_t offset = dir == DAI_DIR_PLAYBACK ?
ALH_TXDA_OFFSET : ALH_RXDA_OFFSET;

prop->fifo_address = dai_base(dp) + offset + ALH_STREAM_OFFSET * stream_id;
prop->dma_hs_id = alh_handshake_map[stream_id];
prop->stream_id = alh->params.stream_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue: this is populating device-owned memory and returning it to the user without locking. Much better to have the user supply the struct to fill in.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be changed at dai api level, for now it is just using the same way as ssp driver.
For the DMA api it works as you described with user supply the struct to fill and the same we need to apply to dai


LOG_DBG("dai_index %u", dp->index);
LOG_DBG("fifo %u", prop->fifo_address);
LOG_DBG("handshake %u", prop->dma_hs_id);

return prop;
}

static int dai_alh_probe(const struct device *dev)
{
struct dai_intel_alh *dp = (struct dai_intel_alh *)dev->data;
k_spinlock_key_t key;

LOG_DBG("%s", __func__);

key = k_spin_lock(&dp->lock);

if (dp->sref == 0) {
alh_claim_ownership();
}

dp->sref++;

k_spin_unlock(&dp->lock, key);

return 0;
}

static int dai_alh_remove(const struct device *dev)
{
struct dai_intel_alh *dp = (struct dai_intel_alh *)dev->data;
k_spinlock_key_t key;

LOG_DBG("%s", __func__);

key = k_spin_lock(&dp->lock);

if (--dp->sref == 0) {
alh_release_ownership();
}

k_spin_unlock(&dp->lock, key);

return 0;
}

static int alh_init(const struct device *dev)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Converse comment from above: this doesn't do anything to device state and in fact returns with the device still uninitialized. It won't get its values reset until probe() sometime later. Seems like that's backwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, please check dai api and ssp, the init is empty and we are using probe to truly init at runtime

return 0;
}

static const struct dai_driver_api dai_intel_alh_api_funcs = {
.probe = dai_alh_probe,
.remove = dai_alh_remove,
.config_set = dai_alh_config_set,
.config_get = dai_alh_config_get,
.trigger = dai_alh_trigger,
.get_properties = dai_alh_get_properties,
};

#define DAI_INTEL_ALH_DEVICE_INIT(n) \
static struct dai_config dai_intel_alh_config_##n; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Per comment earlier: traditionally the "config" struct for a zephyr device is declared const and contains only immutable data. Dynamic state goes in the "data" struct.

Copy link
Member Author

Choose a reason for hiding this comment

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

This way it was upstream with dai ssp, I think this is another thing to change in dai refactor because for now dai_config is defined in dai and cannot be changed only for alh

static struct dai_intel_alh dai_intel_alh_data_##n = { \
.index = (n / DAI_NUM_ALH_BI_DIR_LINKS_GROUP) << 8 | \
(n % DAI_NUM_ALH_BI_DIR_LINKS_GROUP), \
.plat_data = { \
.base = DT_INST_PROP_BY_IDX(n, reg, 0), \
.fifo_depth[DAI_DIR_PLAYBACK] = ALH_GPDMA_BURST_LENGTH, \
.fifo_depth[DAI_DIR_CAPTURE] = ALH_GPDMA_BURST_LENGTH, \
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like fifo_depth is an immutable hardware feature, no? That wants to be in the config struct, not data.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same approach as in dai ssp driver with common dai config (for all dai drivers) so all the rest needs to be placed in data, just reused for alh. Not sure if we should change all of this just for alh?

}, \
}; \
\
DEVICE_DT_INST_DEFINE(n, \
alh_init, NULL, \
&dai_intel_alh_data_##n, \
&dai_intel_alh_config_##n, \
POST_KERNEL, 32, \
&dai_intel_alh_api_funcs);

DT_INST_FOREACH_STATUS_OKAY(DAI_INTEL_ALH_DEVICE_INIT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Aesthetics: I personally (i.e. lots of people on the project disagree with me!) really hate this pattern we have of trying to initialize an arbitrary number of DTS devices using a preprocessor loop that recursively expands a giant macro of declarations. In this particular case, I believe the ALH is limited to just two instances architecturally, so IMHO it would be cleaner to just define two devices here with regular C code and skip all the macro nonsense.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer this way and be consistent with the majority of the code base to be honest :)

Copy link
Member Author

@abonislawski abonislawski May 25, 2022

Choose a reason for hiding this comment

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

I hate it too, when starting with zephyr I was trying to figure out whats going on with these horrible macros :)

In this case ALH will be probably extended to 16(?) instances so I think we will stay with this approach

Copy link
Member

Choose a reason for hiding this comment

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

Please stay with this approach

110 changes: 110 additions & 0 deletions drivers/dai/intel/alh/alh.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/*
* Copyright (c) 2022 Intel Corporation.
*
* SPDX-License-Identifier: Apache-2.0
*/

#ifndef __INTEL_DAI_DRIVER_ALH_H__
#define __INTEL_DAI_DRIVER_ALH_H__
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#define __INTEL_DAI_DRIVER_ALH_H__
#define __INTEL_DAI_DRIVER_ALH_H__

not following the pattern used on Zephyr.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? In ssp we have INTEL_DAI_DRIVER_SSP_H and here INTEL_DAI_DRIVER_ALH_H

Copy link
Contributor

@GeorgeCGV GeorgeCGV May 25, 2022

Choose a reason for hiding this comment

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

The majority of the drivers follow the ZEPHYR_DRIVERS_<name>_H__ pattern. Therefore, it could be ZEPHYR_DRIVERS_INTEL_DAI_DRIVER_ALH_H__. However, not every module and driver does it.


#include <stdint.h>
#include <zephyr/drivers/dai.h>

#include "alh_map.h"

#define DAI_NUM_ALH_BI_DIR_LINKS_GROUP 4

#define ALH_STREAM_OFFSET 0x4

#define IPC4_ALH_MAX_NUMBER_OF_GTW 16
#define IPC4_ALH_DAI_INDEX_OFFSET 7

#define ALH_GPDMA_BURST_LENGTH 4

#define ALH_SET_BITS(b_hi, b_lo, x) \
(((x) & ((1ULL << ((b_hi) - (b_lo) + 1ULL)) - 1ULL)) << (b_lo))
#define ALHASCTL_OSEL(x) ALH_SET_BITS(25, 24, x)

#define dai_get_drvdata(dai) &dai->priv_data
#define dai_base(dai) dai->plat_data.base

#define DAI_DIR_PLAYBACK 0
#define DAI_DIR_CAPTURE 1

#define ALH_CHANNELS_DEFAULT 2
#define ALH_RATE_DEFAULT 48000
#define ALH_WORD_SIZE_DEFAULT 32

#if CONFIG_INTEL_ADSP_CAVS
#define ALH_TXDA_OFFSET 0x400
#define ALH_RXDA_OFFSET 0x500
#else
#define ALH_TXDA_OFFSET 0
#define ALH_RXDA_OFFSET 0x100
Comment on lines +39 to +43
Copy link
Member

Choose a reason for hiding this comment

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

These might be better as devicetree properties

cc: @laurenmurphyx64

#endif

union dai_intel_ipc4_gateway_attributes {
/**< Raw value */
uint32_t dw;

/**< Access to the fields */
struct {
/**< Gateway data requested in low power memory. */
uint32_t lp_buffer_alloc : 1;

/**< Gateway data requested in register file memory. */
uint32_t alloc_from_reg_file : 1;

/**< Reserved field */
uint32_t _rsvd : 30;
} bits; /**<< Bits */
} __packed;

/* ALH Configuration Request - SOF_IPC_DAI_ALH_CONFIG */
struct dai_intel_ipc3_alh_params {
uint32_t reserved0;
uint32_t stream_id;
uint32_t rate;
uint32_t channels;

/* reserved for future use */
uint32_t reserved[13];
} __packed;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing you have this struct (and others in this file) marked __packed as you're getting them over the wire from the SOF driver on the host? That seems fragile to me. If you really want unified config structs like that, we should arrange for some other layer to specify them. Baking them into the driver internals (this header isn't part of the Zephyr API!) seems like the wrong place.

Also, packed is a noop for a struct containing only a single field type anyway.

Also also, this struct has a size of ... 17 words? Not 16? Is there an off-by-one in the computation of that reserved[] array? Is the glitch symmetric with the SOF definition? This kind of thing is exactly why you want protocol details and driver layer separated. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I can see in SOF we have exactly the same config with 17 words :) Need to check that


struct ipc4_alh_multi_gtw_cfg {
/* Number of single channels (valid items in mapping array). */
uint32_t count;
/* Single to multi aggregation mapping item. */
struct {
/* Vindex of a single ALH channel aggregated. */
uint32_t alh_id;
/* Channel mask */
uint32_t channel_mask;
} mapping[IPC4_ALH_MAX_NUMBER_OF_GTW]; /* < Mapping items */
} __packed;

struct dai_intel_ipc4_alh_configuration_blob {
union dai_intel_ipc4_gateway_attributes gtw_attributes;
struct ipc4_alh_multi_gtw_cfg alh_cfg;
} __packed;

struct dai_intel_alh_plat_data {
uint32_t base;
uint32_t fifo_depth[2];
};

struct dai_intel_alh_pdata {
struct dai_config config;
struct dai_properties props;
struct dai_intel_ipc3_alh_params params;
};

struct dai_intel_alh {
uint32_t index; /**< index */
struct k_spinlock lock; /**< locking mechanism */
int sref; /**< simple ref counter, guarded by lock */
struct dai_intel_alh_plat_data plat_data;
struct dai_intel_alh_pdata priv_data;
};

#endif
Loading