Re: [RFC PATCH 07/40] soundwire: intel: fix channel number reported by hardware
On 8/2/19 6:57 AM, Vinod Koul wrote:
On 25-07-19, 18:39, Pierre-Louis Bossart wrote:
PDI2 reports an invalid count, force the correct hardware-supported
value
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 497879dd9c0d..51990b192dc0 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -401,6 +401,15 @@ intel_pdi_get_ch_cap(struct sdw_intel *sdw, unsigned int
pdi_num, bool pcm)
if (pcm) {
count = intel_readw(shim, SDW_SHIM_PCMSYCHC(link_id, pdi_num));
+
+ /*
+* TODO: pdi number 2 reports channel count as 1 even though
+* it supports 8 channel. Performing hardcoding for pdi
+* number 2.
+*/
+ if (pdi_num == 2)
+ count = 7;
Is that true for all Intel controllers or some generations. Would it not
be better to put this under some flag which is set on platform basis?
This is true of all controllers released so far.
We will change this if the hardware changes.
+
} else {
count = intel_readw(shim, SDW_SHIM_PDMSCAP(link_id));
count = ((count & SDW_SHIM_PDMSCAP_CPSS) >>
--
2.20.1
Re: [alsa-devel] [RFC PATCH 06/40] soundwire: intel: prevent possible dereference in hw_params
On 8/2/19 6:55 AM, Vinod Koul wrote:
On 25-07-19, 18:39, Pierre-Louis Bossart wrote:
This should not happen in production systems but we should test for
all callback arguments before invoking the config_stream callback.
so you are saying callback arg is mandatory, if so please document that
assumption
no, what this says is that if a config_stream is provided then it needs
to have a valid argument.
I am not sure what you mean by "document that assumption", comment in
the code (where?) or SoundWire documentation?
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 68832e613b1e..497879dd9c0d 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw,
struct snd_soc_dai *dai,
struct snd_pcm_hw_params *hw_params, int link_id)
{
- if (sdw->res->ops && sdw->res->ops->config_stream)
+ if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg)
return sdw->res->ops->config_stream(sdw->res->arg,
substream, dai, hw_params, link_id);
--
2.20.1
Re: [RFC PATCH 05/40] soundwire: intel: move interrupt enable after interrupt handler registration
On 8/2/19 6:53 AM, Vinod Koul wrote: On 25-07-19, 18:39, Pierre-Louis Bossart wrote: Not sure why the existing code would enable interrupts without the ability to deal with them. Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/intel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index aeadc341c0a3..68832e613b1e 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -981,8 +981,6 @@ static int intel_probe(struct platform_device *pdev) if (ret) goto err_init; - ret = sdw_cdns_enable_interrupt(>cdns); - /* Read the PDI config and initialize cadence PDI */ intel_pdi_init(sdw, ); ret = sdw_cdns_pdi_init(>cdns, config); @@ -1000,6 +998,8 @@ static int intel_probe(struct platform_device *pdev) goto err_init; } + ret = sdw_cdns_enable_interrupt(>cdns); we should also handle the return yes, fixed already
Re: linux-next: manual merge of the sound-asoc tree with the sound tree
On 7/31/19 9:50 PM, Stephen Rothwell wrote:
Hi all,
Today's linux-next merge of the sound-asoc tree got conflicts in:
sound/soc/intel/skylake/skl-nhlt.c
sound/soc/intel/skylake/skl.h
between commit:
1169cbf6b98e ("ASoC: Intel: Skylake: use common NHLT module")
from the sound tree and commit:
bcc2a2dc3ba8 ("ASoC: Intel: Skylake: Merge skl_sst and skl into skl_dev
struct")
from the sound-asoc tree.
I fixed it up (I think, see below (I used the sound tree version of
ound/soc/intel/skylake/skl-nhlt.c)) and can carry the fix as necessary.
This is now fixed as far as linux-next is concerned, but any non
trivial conflicts should be mentioned to your upstream maintainer when
your tree is submitted for merging. You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.
Mark, this comes my NHLT fixes merged by Takashi and available on his
topic/hda-dmic branch.
This can be fixed by merging this topic/hda-dmic into your for-next
branch, with a minor set of merge conflicts already identified by Stephen.
If you don't like merge conflicts, we can also do this with
revert-merge-reapply, I pushed a branch
https://github.com/plbossart/sound/commits/fix/nhlt-conflicts to provide
the sequence needed
Sorry about that, let me know if I can help further.
-Pierre
Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled
On 7/30/19 6:21 AM, Andy Shevchenko wrote: On Mon, Jul 29, 2019 at 05:07:39PM -0500, Pierre-Louis Bossart wrote: On 7/26/19 2:08 PM, Andy Shevchenko wrote: On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote: - if (ret < 0) + if (ret < 0 && ret != -EACCES) ...and here, the pm_runtime_put_noidle() call is missed. yes but in the example you provided, they actually do more work than just decrement the device usage counter: In their case they would like to do that. You decide what is appropriate call in your case. My point is, that reference counter in case of error handling should be returned back to its value. Agree on the reference count. I am however not clear on the next step and 'what is appropriate'. If pm_runtime_get_sync() failed, then technically the device was not resumed so marking it as last_busy+autosuspend, or using a plain vanilla put() will not result in any action. I must be missing something here.
Re: [alsa-devel] [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled
On 7/26/19 2:08 PM, Andy Shevchenko wrote:
On Fri, Jul 26, 2019 at 01:08:57PM -0500, Pierre-Louis Bossart wrote:
This thread became unreadable with interleaved top-posting, allow me restate
the options and ask PM folks what they think
On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote:
Not all platforms support runtime_pm for now, let's use runtime_pm
only when enabled.
Just a side note below...
- ret = pm_runtime_get_sync(slave->bus->dev);
- if (ret < 0)
Here...
- return ret;
+ if (pm_runtime_enabled(slave->bus->dev)) {
+ ret = pm_runtime_get_sync(slave->bus->dev);
+ if (ret < 0)
...and thus here...
+ return ret;
+ }
ret = sdw_transfer(slave->bus, );
- pm_runtime_put(slave->bus->dev);
+
+ if (pm_runtime_enabled(slave->bus->dev))
+ pm_runtime_put(slave->bus->dev);
This is option1: we explicitly test if pm_runtime is enabled before calling
_get_sync() and _put()
option2 (suggested by Jan Kotas): catch the -EACCESS error code
ret = pm_runtime_get_sync(slave->bus->dev);
- if (ret < 0)
+ if (ret < 0 && ret != -EACCES)
...and here, the pm_runtime_put_noidle() call is missed.
yes but in the example you provided, they actually do more work than
just decrement the device usage counter:
static int
radeonfb_open(struct fb_info *info, int user)
{
struct radeon_fbdev *rfbdev = info->par;
struct radeon_device *rdev = rfbdev->rdev;
int ret = pm_runtime_get_sync(rdev->ddev->dev);
if (ret < 0 && ret != -EACCES) {
pm_runtime_mark_last_busy(rdev->ddev->dev);
pm_runtime_put_autosuspend(rdev->ddev->dev);
return ret;
}
return 0;
}
unless I am missing something pm_runtime_put_noidle() and
_put_autosuspend() are not equivalent, are they?
Re: [RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled
This thread became unreadable with interleaved top-posting, allow me
restate the options and ask PM folks what they think
On 7/25/19 6:40 PM, Pierre-Louis Bossart wrote:
Not all platforms support runtime_pm for now, let's use runtime_pm
only when enabled.
Suggested-by: Srinivas Kandagatla
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/bus.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 5ad4109dc72f..0a45dc5713df 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -332,12 +332,16 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t
count, u8 *val)
if (ret < 0)
return ret;
- ret = pm_runtime_get_sync(slave->bus->dev);
- if (ret < 0)
- return ret;
+ if (pm_runtime_enabled(slave->bus->dev)) {
+ ret = pm_runtime_get_sync(slave->bus->dev);
+ if (ret < 0)
+ return ret;
+ }
ret = sdw_transfer(slave->bus, );
- pm_runtime_put(slave->bus->dev);
+
+ if (pm_runtime_enabled(slave->bus->dev))
+ pm_runtime_put(slave->bus->dev);
This is option1: we explicitly test if pm_runtime is enabled before
calling _get_sync() and _put()
option2 (suggested by Jan Kotas): catch the -EACCESS error code
ret = pm_runtime_get_sync(slave->bus->dev);
- if (ret < 0)
+ if (ret < 0 && ret != -EACCES)
return ret;
option3: ignore the return value as done in quite a few drivers
Are there any other options? I am personally surprised this is not
handled in the pm_runtime core, not sure why users need to test for this?
Thanks
Pierre
Re: [alsa-devel] [RFC PATCH 27/40] soundwire: Add Intel resource management algorithm
Thanks Guennadi for looking at this code, it's hard to review and figure
things out...
I replied to each, even trivial ones, to have a trace of all the issues.
+static void sdw_compute_slave_ports(struct sdw_master_runtime *m_rt,
+ struct sdw_transport_data *t_data)
+{
+ struct sdw_slave_runtime *s_rt = NULL;
Superfluous initialisation.
ok
+ struct sdw_port_runtime *p_rt;
+ int port_bo, sample_int;
+ unsigned int rate, bps, ch = 0;
ditto for ch
ok
+
+ port_bo = t_data->block_offset;
+
+ list_for_each_entry(s_rt, _rt->slave_rt_list, m_rt_node) {
+ rate = m_rt->stream->params.rate;
+ bps = m_rt->stream->params.bps;
+ sample_int = (m_rt->bus->params.curr_dr_freq / rate);
+
+ list_for_each_entry(p_rt, _rt->port_list, port_node) {
+ ch = sdw_ch_mask_to_ch(p_rt->ch_mask);
+
+ sdw_fill_xport_params(_rt->transport_params,
+ p_rt->num, true,
+ SDW_BLK_GRP_CNT_1,
+ sample_int, port_bo, port_bo >> 8,
+ t_data->hstart,
+ t_data->hstop,
I think the above two lines could fit in one
likely yes.
+ (SDW_BLK_GRP_CNT_1 * ch), 0x0);
superfluous parentheses
yep
+static void sdw_compute_master_ports(struct sdw_master_runtime *m_rt,
+struct sdw_group_params *params,
+int port_bo, int hstop)
+{
+ struct sdw_transport_data t_data = {0};
+ struct sdw_port_runtime *p_rt;
+ struct sdw_bus *bus = m_rt->bus;
+ int sample_int, hstart = 0;
superfluous initialisation
yes
+ unsigned int rate, bps, ch, no_ch;
+
+ rate = m_rt->stream->params.rate;
+ bps = m_rt->stream->params.bps;
+ ch = m_rt->ch_count;
+ sample_int = (bus->params.curr_dr_freq / rate);
superfluous parentheses
yes
+
+ if (rate != params->rate)
+ return;
+
+ t_data.hstop = hstop;
+ hstart = hstop - params->hwidth + 1;
+ t_data.hstart = hstart;
+
+ list_for_each_entry(p_rt, _rt->port_list, port_node) {
+ no_ch = sdw_ch_mask_to_ch(p_rt->ch_mask);
+
+ sdw_fill_xport_params(_rt->transport_params, p_rt->num,
+ true, SDW_BLK_GRP_CNT_1, sample_int,
+ port_bo, port_bo >> 8, hstart, hstop,
+ (SDW_BLK_GRP_CNT_1 * no_ch), 0x0);
superfluous parentheses
yes
+
+ sdw_fill_port_params(_rt->port_params,
+p_rt->num, bps,
+SDW_PORT_FLOW_MODE_ISOCH,
+SDW_PORT_DATA_MODE_NORMAL);
+
+ /* Check for first entry */
+ if (!(p_rt == list_first_entry(_rt->port_list,
+ struct sdw_port_runtime,
+ port_node))) {
you wanted to write "if (p_rt != ...)"
bad code indeed, thanks for spotting this. I need to double-check this
one, now I don't trust the ==
it could also be that it was meant to be a NULL check on the first entry.
+ port_bo += bps * ch;
+ continue;
+ }
+
+ t_data.hstart = hstart;
+ t_data.hstop = hstop;
You already set these two above
need to check this as well.
+ t_data.block_offset = port_bo;
+ t_data.sub_block_offset = 0;
+ port_bo += bps * ch;
+ }
+
+ sdw_compute_slave_ports(m_rt, _data);
+}
+
+static void _sdw_compute_port_params(struct sdw_bus *bus,
+struct sdw_group_params *params, int count)
+{
+ struct sdw_master_runtime *m_rt = NULL;
superfluous initialisation
yes
+ int hstop = bus->params.col - 1;
+ int block_offset, port_bo, i;
+
+ /* Run loop for all groups to compute transport parameters */
+ for (i = 0; i < count; i++) {
+ port_bo = 1;
+ block_offset = 1;
+
+ list_for_each_entry(m_rt, >m_rt_list, bus_node) {
+ sdw_compute_master_ports(m_rt, [i],
+port_bo, hstop);
+
+ block_offset += m_rt->ch_count *
+ m_rt->stream->params.bps;
+ port_bo = block_offset;
+ }
+
+ hstop = hstop - params[i].hwidth;
hstop -= ...
yes
+ }
+}
+
+static int sdw_compute_group_params(struct sdw_bus *bus,
+
Re: [alsa-devel] [RFC PATCH 38/40] soundwire: cadence_master: make clock stop exit configurable on init
-int sdw_cdns_init(struct sdw_cdns *cdns); +int sdw_cdns_init(struct sdw_cdns *cdns, bool clock_stop_exit); int sdw_cdns_pdi_init(struct sdw_cdns *cdns, struct sdw_cdns_stream_config config); int sdw_cdns_exit_reset(struct sdw_cdns *cdns); diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 1192d5775484..db7bf2912767 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1043,7 +1043,7 @@ static int intel_init(struct sdw_intel *sdw) intel_link_power_up(sdw); intel_shim_init(sdw); - return sdw_cdns_init(>cdns); + return sdw_cdns_init(>cdns, false); This is the only caller of this function so far, so, it looks like the second argument ATM is always "false." I assume you foresee other uses with "true" in the future, otherwise maybe just fix it to false in the function? Since we are at RFC level things are not set in stone, it's not fully clear if this test is required or not. I added it since Rander reported an error on one of the target platforms that I didn't see personally. We'll double-check if it's needed. And if indeed it's needed, yes it'll be set to true when we add clock stop.
Re: [alsa-devel] [RFC PATCH 37/40] soundwire: cadence_master: add hw_reset capability in debugfs
+static int cdns_hw_reset(void *data, u64 value)
+{
+ struct sdw_cdns *cdns = data;
+ int ret;
+
+ if (value != 1)
+ return 0;
+
+ dev_info(cdns->dev, "starting link hw_reset\n");
+
+ ret = sdw_cdns_exit_reset(cdns);
+
+ dev_info(cdns->dev, "link hw_reset done\n");
Both really should be dev_info()? Maybe at least one of them can be dev_dbg()?
I have to walk back on what I explained to Greg. The idea was to have a
dmesg trace when this function as called when the user plays with
debugfs, otherwise the dmesg log is difficult to interpret (devices can
go off the bus on their own). I'll keep the first one only and demote it
to dev_dbg.
Re: [alsa-devel] [RFC PATCH 36/40] soundwire: intel: disable interrupts on suspend
-int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
{
u32 mask;
- cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
- CDNS_MCP_SLAVE_INTMASK0_MASK);
- cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
- CDNS_MCP_SLAVE_INTMASK1_MASK);
+ if (state) {
+ cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
+ CDNS_MCP_SLAVE_INTMASK0_MASK);
+ cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
+ CDNS_MCP_SLAVE_INTMASK1_MASK);
- /* enable detection of slave state changes */
- mask = CDNS_MCP_INT_SLAVE_RSVD | CDNS_MCP_INT_SLAVE_ALERT |
- CDNS_MCP_INT_SLAVE_ATTACH | CDNS_MCP_INT_SLAVE_NATTACH;
+ /* enable detection of slave state changes */
+ mask = CDNS_MCP_INT_SLAVE_RSVD | CDNS_MCP_INT_SLAVE_ALERT |
+ CDNS_MCP_INT_SLAVE_ATTACH | CDNS_MCP_INT_SLAVE_NATTACH;
- /* enable detection of bus issues */
- mask |= CDNS_MCP_INT_CTRL_CLASH | CDNS_MCP_INT_DATA_CLASH |
- CDNS_MCP_INT_PARITY;
+ /* enable detection of bus issues */
+ mask |= CDNS_MCP_INT_CTRL_CLASH | CDNS_MCP_INT_DATA_CLASH |
+ CDNS_MCP_INT_PARITY;
- /* no detection of port interrupts for now */
+ /* no detection of port interrupts for now */
- /* enable detection of RX fifo level */
- mask |= CDNS_MCP_INT_RX_WL;
+ /* enable detection of RX fifo level */
+ mask |= CDNS_MCP_INT_RX_WL;
- /* now enable all of the above */
- mask |= CDNS_MCP_INT_IRQ;
+ /* now enable all of the above */
+ mask |= CDNS_MCP_INT_IRQ;
- if (interrupt_mask) /* parameter override */
- mask = interrupt_mask;
+ if (interrupt_mask) /* parameter override */
+ mask = interrupt_mask;
+ } else {
+ cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, 0);
+ cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, 0);
+ mask = 0;
+ }
Looks like this should be two functions? Especially since "state" is always a
constant
when it is called. If there is still a lot of common code below, maybe make it
a helper
function.
Yes, the code is a bit ugly. I could initialize all the masks to zero,
have the if(state) block and write the masks.
Re: [alsa-devel] [RFC PATCH 35/40] soundwire: intel: export helper to exit reset
@@ -161,6 +161,7 @@ irqreturn_t sdw_cdns_thread(int irq, void *dev_id);
int sdw_cdns_init(struct sdw_cdns *cdns);
int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
struct sdw_cdns_stream_config config);
+int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index a976480d6f36..9ebe38e4d979 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1112,6 +1112,8 @@ static int intel_probe(struct platform_device *pdev)
ret = sdw_cdns_enable_interrupt(>cdns);
+ ret = sdw_cdns_exit_reset(>cdns);
This isn't something, that this patch changes, but if the return value above is
ignored, maybe no need to assign it at all?
The return of these two functions was used with in some logs at some
point but they obviously vanished.
I'll re-check if we care about the status, could be that it never fails
Thanks!
+
/* Register DAIs */
ret = intel_register_dai(sdw);
if (ret) {
@@ -1199,6 +1201,8 @@ static int intel_resume(struct device *dev)
sdw_cdns_enable_interrupt(>cdns);
+ ret = sdw_cdns_exit_reset(>cdns);
+
return ret;
Re: [alsa-devel] [PATCH v4 1/1] ASoC: Intel: Skylake: Remove static table index when parsing topology
On 7/26/19 4:09 AM, Amadeusz Sławiński wrote:
Currently when we remove and reload driver we use previous ref_count
value to start iterating over skl->modules which leads to out of table
access. To fix this just inline the function and calculate indexes
everytime we parse UUID token.
Signed-off-by: Amadeusz Sławiński
Reviewed-by: Pierre-Louis Bossart
---
sound/soc/intel/skylake/skl-topology.c | 34 +-
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-topology.c
b/sound/soc/intel/skylake/skl-topology.c
index 118866cd5075..c1c37ce759bd 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -,25 +,6 @@ static int skl_tplg_get_int_tkn(struct device *dev,
return tkn_count;
}
-static int skl_tplg_get_manifest_uuid(struct device *dev,
- struct skl_dev *skl,
- struct snd_soc_tplg_vendor_uuid_elem *uuid_tkn)
-{
- static int ref_count;
- struct skl_module *mod;
-
- if (uuid_tkn->token == SKL_TKN_UUID) {
- mod = skl->modules[ref_count];
- guid_copy(>uuid, (guid_t *)_tkn->uuid);
- ref_count++;
- } else {
- dev_err(dev, "Not an UUID token tkn %d\n", uuid_tkn->token);
- return -EINVAL;
- }
-
- return 0;
-}
-
/*
* Fill the manifest structure by parsing the tokens based on the
* type.
@@ -3362,6 +3343,7 @@ static int skl_tplg_get_manifest_tkn(struct device *dev,
{
int tkn_count = 0, ret;
int off = 0, tuple_size = 0;
+ u8 uuid_index = 0;
struct snd_soc_tplg_vendor_array *array;
struct snd_soc_tplg_vendor_value_elem *tkn_elem;
@@ -3384,9 +3366,17 @@ static int skl_tplg_get_manifest_tkn(struct device *dev,
continue;
case SND_SOC_TPLG_TUPLE_TYPE_UUID:
- ret = skl_tplg_get_manifest_uuid(dev, skl, array->uuid);
- if (ret < 0)
- return ret;
+ if (array->uuid->token != SKL_TKN_UUID) {
+ dev_err(dev, "Not an UUID token: %d\n",
+ array->uuid->token);
+ return -EINVAL;
+ }
+ if (uuid_index >= skl->nr_modules) {
+ dev_err(dev, "Too many UUID tokens\n");
+ return -EINVAL;
+ }
+ guid_copy(>modules[uuid_index++]->uuid,
+ (guid_t *)>uuid->uuid);
tuple_size += sizeof(*array->uuid);
continue;
Re: [RFC PATCH 04/40] soundwire: intel: add debugfs register dump
+static const struct file_operations intel_reg_fops = {
+ .open = simple_open,
+ .read = intel_reg_read,
+ .llseek = default_llseek,
+};
DEFINE_SIMPLE_ATTRIBUTE()?
yes
+
+static void intel_debugfs_init(struct sdw_intel *sdw)
+{
+ struct dentry *root = sdw->cdns.bus.debugfs;
+
+ if (!root)
+ return;
+
+ sdw->fs = debugfs_create_dir("intel-sdw", root);
+ if (IS_ERR_OR_NULL(sdw->fs)) {
+ dev_err(sdw->cdns.dev, "debugfs root creation failed\n");
No, come on, don't do that. I've been sweeping the kernel tree to
remove this anti-pattern.
The debugfs core will print an error if you got something wrong, just
call the function and move on, you NEVER need to check the return value
of a debugfs call.
Yes, sorry to make your blood pressure go up... I missed this one in the
cleanups yesterday. will fix.
Re: [RFC PATCH 01/40] soundwire: add debugfs support
diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c new file mode 100644 index ..8d86e100516e --- /dev/null +++ b/drivers/soundwire/debugfs.c @@ -0,0 +1,156 @@ +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) No, for debugfs-specific code, that dual license makes no sense, right? Don't cargo-cult SPDX identifiers please. It's a miss, I used EXPORT_GPL and missed this line, will fix. +// Copyright(c) 2017-19 Intel Corporation. Spell the year out fully unless you want lawyers knocking on your door :) haha, will fix. + +#include +#include +#include +#include +#include +#include +#include "bus.h" + +#ifdef CONFIG_DEBUG_FS +struct dentry *sdw_debugfs_root; +#endif This whole file is not built if that option is not enabled, so why the #ifdef? Ah, will look into this, thanks!
Re: [RFC PATCH 02/40] soundwire: cadence_master: add debugfs register dump
+static const struct file_operations cdns_reg_fops = {
+ .open = simple_open,
+ .read = cdns_reg_read,
+ .llseek = default_llseek,
+};
DEFINE_SHOW_ATTRIBUTE()?
I remember looking at this but can't recall why I left it this way.
That was before my Summer break so will relook, thanks for suggesting this.
Re: [alsa-devel] [RFC PATCH 21/40] soundwire: export helpers to find row and column values
On 7/26/19 9:43 AM, Guennadi Liakhovetski wrote:
On Thu, Jul 25, 2019 at 06:40:13PM -0500, Pierre-Louis Bossart wrote:
Add a prefix for common tables and export 2 helpers to set the frame
shapes based on row/col values.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/bus.h| 7 +--
drivers/soundwire/stream.c | 14 --
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 06ac4adb0074..c57c9c23f6ca 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -73,8 +73,11 @@ struct sdw_msg {
#define SDW_DOUBLE_RATE_FACTOR 2
-extern int rows[SDW_FRAME_ROWS];
-extern int cols[SDW_FRAME_COLS];
+extern int sdw_rows[SDW_FRAME_ROWS];
+extern int sdw_cols[SDW_FRAME_COLS];
So these arrays actually have to be exported? In the current (5.2) sources they
seem to only be used in stream.c, maybe make them static there?
+
+int sdw_find_row_index(int row);
+int sdw_find_col_index(int col);
yes, they need to be exported, they are used by the allocation algorithm
(in Patch 27).
Others will need this for non-Intel solutions, it's really a part of the
standard definition and should be shared.
I can improve the commit message to make this explicit.
Re: [alsa-devel] [RFC PATCH 00/40] soundwire: updates for 5.4
Comments and feedback welcome! Hello Pierre, This patchset is pretty large - I'd suggest dividing next RFC into segments: debugfs, info, power-management, basic flow corrections and frame shape calculator. There was an intent to provide a logical progression... First debugfs, since I believe it was reviewed before, and I wanted folks like Greg to double-check it without burrying it too deep. Then all corrections, followed by the allocator. And last all PM stuff, split by regular suspend/resume and pm_runtime. The RFC state is precisely to gather feedback, if folks want a different order that's fine. I just wanted to be transparent and share what we have. Some commits have no messages and others lack additional info - tried to provide feedback wherever I could, though, especially for the last one, it would be vital to post additional info so in-depth feedback can be provided. The lack of commits is a miss, I went from 170 debug/integration patches to 40 yesterday and my brain was fried. Maybe nothing for calculator will come up, maybe something will. In general I remember it being an essential part of SDW and one where many bugs where found during the initial verification phase. the frame allocation is a critical piece and it does need to be hardened. However we can do so in steps. The current setups we have support 1 Slave per link and a limited amount of bandwidth. The links themselves don't operate at the max frequency. Also note that that the streams are 'statically' defined by the dailinks, and the allocation is not fully dynamic with random configurations being request. If you fail you fail fast. Nevertheless I do plan to recheck the allocator with an additional scripting tool. It'd be very good to e.g. dump the current setup in a debugfs file and show to users what is happening (or not happening). I didn't recall you worked on SoundWire and I can use your practical knowledge to make the code and tools better :-) Thanks for your contribution and have a good day! Thanks for reviewing this long series and have a nice week-end. -Pierre
Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
Unrelated to this specific patch, but I looked at _sdw_disable_stream()
and I see this there (again, maybe my version is outdated already):
struct sdw_master_runtime *m_rt = NULL;
struct sdw_bus *bus = NULL;
where both those initialisations are redundant. Moreover:
will look at this, thanks.
On Thu, Jul 25, 2019 at 06:40:15PM -0500, Pierre-Louis Bossart wrote:
When we disable the stream and then call hw_free, two bank switches
will be handled and as a result we re-enable the stream on hw_free.
Make sure the stream is disabled on both banks.
TODO: we need to completely revisit all this and make sure we have a
mirroring mechanism between current and alternate banks.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/stream.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 53f5e790fcd7..75b9ad1fb1a6 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime
*stream)
}
}
- return do_bank_switch(stream);
+ ret = do_bank_switch(stream);
+ if (ret < 0) {
+ dev_err(bus->dev, "Bank switch failed: %d\n", ret);
+ return ret;
+ }
+
+ /* make sure alternate bank (previous current) is also disabled */
+ list_for_each_entry(m_rt, >master_list, stream_node) {
+ bus = m_rt->bus;
"bus" is only used below, so at least take that line under "if (ret < 0)"
or even just do "dev_err(m_rt->bus->dev,...)" everywhere in this function
and remove the variable altogether...
will look at this, thanks Guennadi
Thanks
Guennadi
+ /* Disable port(s) */
+ ret = sdw_enable_disable_ports(m_rt, false);
+ if (ret < 0) {
+ dev_err(bus->dev, "Disable port(s) failed: %d\n", ret);
+ return ret;
Re: [alsa-devel] [RFC PATCH 37/40] soundwire: cadence_master: add hw_reset capability in debugfs
Thanks for the review Greg.
+static int cdns_hw_reset(void *data, u64 value)
+{
+ struct sdw_cdns *cdns = data;
+ int ret;
+
+ if (value != 1)
+ return 0;
+
+ dev_info(cdns->dev, "starting link hw_reset\n");
+
+ ret = sdw_cdns_exit_reset(cdns);
+
+ dev_info(cdns->dev, "link hw_reset done\n");
Do not be noisy for when things always go right. This looks like
debuggging code, please remove.
yes, missed this in the cleanup, will remove.
+DEFINE_DEBUGFS_ATTRIBUTE(cdns_hw_reset_fops, NULL, cdns_hw_reset, "%llu\n");
+
/**
* sdw_cdns_debugfs_init() - Cadence debugfs init
* @cdns: Cadence instance
@@ -339,6 +358,9 @@ static const struct file_operations cdns_reg_fops = {
void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
{
debugfs_create_file("cdns-registers", 0400, root, cdns, _reg_fops);
+
+ debugfs_create_file_unsafe("cdns-hw-reset", 0200, root, cdns,
+ _hw_reset_fops);
Why unsafe?
Dunno. I followed the documentation and my take-away, along with a
number of examples, was to use _unsafe. I really have no idea if this is
correct or not, I can remove this qualifier if that's not needed.
Re: [alsa-devel] [RFC PATCH 32/40] soundwire: intel: add helper for initialization
On 7/26/19 5:42 AM, Cezary Rojewski wrote:
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
Move code to helper for reuse in power management routines
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index c40ab443e723..215dc81cdf73 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -984,6 +984,15 @@ static struct sdw_master_ops sdw_intel_ops = {
.post_bank_switch = intel_post_bank_switch,
};
+static int intel_init(struct sdw_intel *sdw)
+{
+ /* Initialize shim and controller */
+ intel_link_power_up(sdw);
+ intel_shim_init(sdw);
+
+ return sdw_cdns_init(>cdns);
+}
Why don't we check polling status for _link_power_up? I've already met
slow starting devices in the past. If polling fails and -EAGAIN is
returned, flow of initialization should react appropriately e.g. poll
till MAX_TIMEOUT of some sort -or- collapse.
The code does what it states, it initializes the Intel and Cadence IPs.
What you are referring to is a different problem: once the bus starts,
then Slave devices will report as attached, and will be enumerated. This
will take time. The devices I tested show up immediately and within a
couple of ms the bus is operational. But MIPI allows to the sync to take
up to 4096 frames, which is 85ms with a 48kHz frame rate, so yes we do
need to look into this.
We currently do not have a notification that tells us the bus is back to
normal, that's a design flaw - see the last patch of the series where I
kicked the can down the road but adding an artificial delay.
So yes good point indeed but on the wrong patch :-)
Re: [alsa-devel] [RFC PATCH 33/40] soundwire: intel: Add basic power management support
On 7/26/19 5:50 AM, Cezary Rojewski wrote:
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
+static int intel_resume(struct device *dev)
+{
+ struct sdw_intel *sdw;
+ int ret;
+
+ sdw = dev_get_drvdata(dev);
+
+ ret = intel_init(sdw);
+ if (ret) {
+ dev_err(dev, "%s failed: %d", __func__, ret);
+ return ret;
+ }
+
+ sdw_cdns_enable_interrupt(>cdns);
+
+ return ret;
+}
+
+#endif
Suggestion: the local declaration + initialization via dev_get_drvdata()
are usually combined.
yes, will do.
Given the upstream declaration of _enable_interrupt, it does return
error code/ success. Given current flow, if function gets to
_enable_interrupt call, ret is already set to 0. Returning
sdw_cds_enable_interrupt() directly would both simplify the definition
and prevent status loss.
sounds good, will do, thanks!
Re: [alsa-devel] [RFC PATCH 31/40] soundwire: intel: move shutdown() callback and don't export symbol
On 7/26/19 5:38 AM, Cezary Rojewski wrote:
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
+void intel_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct sdw_cdns_dma_data *dma;
+
+ dma = snd_soc_dai_get_dma_data(dai, substream);
+ if (!dma)
+ return;
+
+ snd_soc_dai_set_dma_data(dai, substream, NULL);
+ kfree(dma);
+}
Correct me if I'm wrong, but do we really need to _get_dma_ here?
_set_dma_ seems bulletproof, same for kfree.
I must admit I have no idea why we have a reference to DMAs here, this
looks like an abuse to store a dai-specific context, and the initial
test looks like copy-paste to detect invalid configs, as done in other
callbacks. Vinod and Sanyog might have more history than me here.
Re: [alsa-devel] [RFC PATCH 29/40] soundwire: intel_init: add kernel module parameter to filter out links
On 7/26/19 5:30 AM, Cezary Rojewski wrote: On 2019-07-26 01:40, Pierre-Louis Bossart wrote: @@ -83,6 +87,9 @@ static struct sdw_intel_ctx caps = ioread32(res->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP); caps &= GENMASK(2, 0); + dev_dbg(>dev, "SoundWire links: BIOS count %d hardware caps %d\n", + count, caps); + /* Check HW supported vs property value and use min of two */ count = min_t(u8, caps, count); This message does not look like it belongs to current patch - no link_mask dependency whatsoever. There have been couple "informative" patches in your series, maybe schedule it with them instead (as a separate series)? You're right, this log should be in a different patch. it was added when I was debugging the DisCo properties a couple of months back and should be moved. thanks for noting this.
Re: [alsa-devel] [RFC PATCH 27/40] soundwire: Add Intel resource management algorithm
On 7/26/19 6:07 AM, Cezary Rojewski wrote: On 2019-07-26 01:40, Pierre-Louis Bossart wrote: This algorithm computes bus parameters like clock frequency, frame shape and port transport parameters based on active stream(s) running on the bus. This implementation is optimal for Intel platforms. Developers can also implement their own .compute_params() callback for specific resource management algorithm. Credits: this patch is based on an earlier internal contribution by Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. All hard-coded values were removed from the initial contribution to use BIOS information instead. FIXME: remove checkpatch report WARNING: Reusing the krealloc arg is almost always a bug + group->rates = krealloc(group->rates, Signed-off-by: Pierre-Louis Bossart Could you specify the requirements and limitations for this algorithm? Last year I written calc for Linux based on Windows (please don't burn me here) equivalent though said requirements/ limitiations might have changed and nothing is valid any longer. The frame shape typically only changes by doubling the number of columns, and the priority is given to PDM streams which use columns. It's hard to document this on a public mailing list, it'd require making references to a spec that's only available to MIPI members. I remember that some parts of specification overcomplicated the calculator and due to actual, realtime usecases it could be greatly simplified (that's why I mention that my work is probably no longer valid). However, these details would help me in reviewing your implementation and providing suggestions. To the best of my knowledge, the algorithm follows a script that is used for both Windows and Linux. The code was initially written by Vinod and team, and I am not aware of simplifications. There a simplifications that are possible, e.g. we don't support PDM for now and the notion of grouping is not needed, but we have to carefully balance 'optimization' with 'introducing bugs'. if this algorithm craps out then the entire bus operation is in the weeds. If we really want people to experiment and get a feel for the performance of the algorithm, we should really provide a UI where the stream parameters can be entered and visually check what the algorithm is doing. I have a solution that shows the bits based on the stream parameters (need to make it available e.g. in Python), if we connected it with the automatic bit allocation it'd be very useful. And yes, "Frame shape calculator" probably suits this better. Though this might be just a preference thingy : ) Resource management is indeed a bit vague. But frame shape calculator is not quite right. The algorithm will find the frame shape that is required for the current bandwidth, but will also allocate the bitSlots in that frame. In MIPI circles we talk about bitSlot allocation.
Re: [alsa-devel] [RFC PATCH 26/40] soundwire: cadence_master: fix divider setting in clock register
On 7/26/19 12:19 AM, Bard liao wrote:
On 7/26/2019 7:40 AM, Pierre-Louis Bossart wrote:
From: Rander Wang
The existing code uses an OR operation which would mix the original
divider setting with the new one, resulting in an invalid
configuration that can make codecs hang.
Add the mask definition and use cdns_updatel to update divider
Signed-off-by: Rander Wang
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 10ebcef2e84e..18c6ac026e85 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -57,6 +57,7 @@
#define CDNS_MCP_SSP_CTRL1 0x28
#define CDNS_MCP_CLK_CTRL0 0x30
#define CDNS_MCP_CLK_CTRL1 0x38
+#define CDNS_MCP_CLK_MCLKD_MASK GENMASK(7, 0)
#define CDNS_MCP_STAT 0x40
@@ -988,9 +989,11 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
/* Set clock divider */
divider = (prop->mclk_freq / prop->max_clk_freq) - 1;
val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
Do we still need to read cdns_readl(cdns, CDNS_MCP_CLK_CTRL0)
after this change?
no I don't think we do indeed.
- val |= divider;
- cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
- cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
+
+ cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
+ CDNS_MCP_CLK_MCLKD_MASK, divider);
+ cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
+ CDNS_MCP_CLK_MCLKD_MASK, divider);
pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
prop->mclk_freq,
and this log needs to go away or done in a better way, I missed this.
@@ -1064,8 +1067,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct
sdw_bus_params *params)
mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);
Same here.
yep.
- mcp_clkctrl |= divider;
- cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
+ cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK,
divider);
pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
___
Alsa-devel mailing list
alsa-de...@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Re: [alsa-devel] [RFC PATCH 24/40] soundwire: cadence_master: use BIOS defaults for frame shape
On 7/26/19 5:20 AM, Cezary Rojewski wrote:
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
+static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
+{
+ u32 val;
+ int c;
+ int r;
+
+ r = sdw_find_row_index(n_rows);
+ c = sdw_find_col_index(n_cols);
+
+ val = (r << 3) | c;
+
+ return val;
+}
+
/**
* sdw_cdns_init() - Cadence initialization
* @cdns: Cadence instance
@@ -977,7 +990,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
/* Set the default frame shape */
- cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT,
CDNS_DEFAULT_FRAME_SHAPE);
+ val = cdns_set_default_frame_shape(prop->default_row,
+ prop->default_col);
+ cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
/* Set SSP interval to default value */
cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);
Suggestion:
declare "generic" _get_frame_frame(rows, cols) instead and let it do the
bitwise operations for you. Pretty sure this won't be the only place in
code where reg value for frame_shape needs to be prepared.
Said function could be as simple as:
return (row << 3) | cols;
+inline flag
i.e. could be even a macro..
Otherwise modify _set_default_frame_shape to simply:
return (r << 3) | c
without involving additional local val variable (I don't really see the
point for any locals there though).
what this function does is take the standard-defined offsets for row and
column and stores them in a Cadence-defined register. I think we can
probably use a macro to remove the magic '3' value, but there are limits
to what we can simplify. I should probably add comments so that people
figure it out.
Re: [alsa-devel] [RFC PATCH 23/40] soundwire: stream: fix disable sequence
On 7/26/19 5:14 AM, Cezary Rojewski wrote:
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
- return do_bank_switch(stream);
+ ret = do_bank_switch(stream);
+ if (ret < 0) {
+ dev_err(bus->dev, "Bank switch failed: %d\n", ret);
+ return ret;
+ }
+
+ /* make sure alternate bank (previous current) is also disabled */
+ list_for_each_entry(m_rt, >master_list, stream_node) {
+ bus = m_rt->bus;
+ /* Disable port(s) */
+ ret = sdw_enable_disable_ports(m_rt, false);
+ if (ret < 0) {
+ dev_err(bus->dev, "Disable port(s) failed: %d\n", ret);
+ return ret;
+ }
+ }
+
+ return 0;
}
/**
While not directly connected to this commit, I see that you do:
link_for_each_entry(m_rt, >master_list, stream_node)
quite often in /stream.c code. Helpful macro would prove useful.
Yes, but the flip side is that people need to look at what the macro
does to figure it out, while everyone knows what list_for_each_entry()
means. Not sure about this one.
And on top of this we'll probably have to rework this code to have a
background copy of the current bank in the alternate bank so it'd rather
leave it simple for now.
Re: [alsa-devel] [RFC PATCH 20/40] soundwire: prototypes for suspend/resume
On 7/26/19 5:04 AM, Cezary Rojewski wrote: On 2019-07-26 01:40, Pierre-Louis Bossart wrote: Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/cadence_master.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index c0bf6ff00a44..d375bbfead18 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -165,6 +165,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns); void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root); +int sdw_cdns_suspend(struct sdw_cdns *cdns); +bool sdw_cdns_check_resume_status(struct sdw_cdns *cdns); + int sdw_cdns_get_stream(struct sdw_cdns *cdns, struct sdw_cdns_streams *stream, u32 ch, u32 dir); No commit message, guess it's SQUASHME commit and shouldn't be part of overall series. I can't recall why it's separate, will look into this. Thanks.
Re: [alsa-devel] [RFC PATCH 16/40] soundwire: cadence_master: improve startup sequence with link hw_reset
+static int do_reset(struct sdw_cdns *cdns)
+{
+ int ret;
+
+ /* program maximum length reset to be safe */
+ cdns_updatel(cdns, CDNS_MCP_CONTROL,
+CDNS_MCP_CONTROL_RST_DELAY,
+CDNS_MCP_CONTROL_RST_DELAY);
+
+ /* use hardware generated reset */
+ cdns_updatel(cdns, CDNS_MCP_CONTROL,
+CDNS_MCP_CONTROL_HW_RST,
+CDNS_MCP_CONTROL_HW_RST);
+
+ /* enable bus operations with clock and data */
+ cdns_updatel(cdns, CDNS_MCP_CONFIG,
+CDNS_MCP_CONFIG_OP,
+CDNS_MCP_CONFIG_OP_NORMAL);
+
+ /* commit changes */
+ ret = cdns_update_config(cdns);
+
+ return ret;
+ return cdns_update_config(cdns);
and remove the "ret" variable.
Yes, it's the same issue as previously reported. will fix.
Re: [alsa-devel] [RFC PATCH 15/40] soundwire: cadence_master: handle multiple status reports per Slave
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 889fa2cd49ae..25d5c7267c15 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -643,13 +643,35 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
/* first check if Slave reported multiple status */
if (set_status > 1) {
+ u32 val;
+
dev_warn_ratelimited(cdns->dev,
-"Slave reported multiple Status:
%d\n",
-mask);
- /*
-* TODO: we need to reread the status here by
-* issuing a PING cmd
-*/
+"Slave %d reported multiple Status:
%d\n",
+i, mask);
+
+ /* re-check latest status extracted from PING commands
*/
+ val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT);
+ val >>= (i * 2);
Superfluous parentheses.
Humm, I don't know my left from my right and I can't remember operator
precedence, so i'd rather make it explicit...
+
+ switch (val & 0x3) {
+ case 0:
+ status[i] = SDW_SLAVE_UNATTACHED;
+ break;
+ case 1:
+ status[i] = SDW_SLAVE_ATTACHED;
+ break;
+ case 2:
+ status[i] = SDW_SLAVE_ALERT;
+ break;
+ default:
There aren't many values left for the "default" case :-) But I'm not sure
whether
any of
+ case 3:
or
+ case 3:
+ default:
would improve readability.
Thanks
Guennadi
+ status[i] = SDW_SLAVE_RESERVED;
+ break;
Yes, those defaults are annoying. Some tools complain so I tend to leave
them.
Re: [alsa-devel] [RFC PATCH 09/40] soundwire: cadence_master: fix usage of CONFIG_UPDATE
@@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns) cdns_writel(cdns, CDNS_MCP_CONFIG, val); - return 0; + /* commit changes */ + ret = cdns_update_config(cdns); + + return ret; + return cdns_update_config(cdns); yes, will fix. thanks!
Re: [alsa-devel] [RFC PATCH 06/40] soundwire: intel: prevent possible dereference in hw_params
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 68832e613b1e..497879dd9c0d 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw,
struct snd_soc_dai *dai,
struct snd_pcm_hw_params *hw_params, int link_id)
{
- if (sdw->res->ops && sdw->res->ops->config_stream)
+ if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg)
return sdw->res->ops->config_stream(sdw->res->arg,
substream, dai, hw_params, link_id);
Hmm, declaring local for sdw->res should prove useful here after
addition of 4th sdw->res dereference.
yes, it's an eyesore. I added this to quickly fix a kernel oops while
debugging, will simplify. thanks for the note.
Re: [alsa-devel] [RFC PATCH 04/40] soundwire: intel: add debugfs register dump
On 7/26/19 4:35 AM, Cezary Rojewski wrote:
On 2019-07-26 01:39, Pierre-Louis Bossart wrote:
+static void intel_debugfs_init(struct sdw_intel *sdw)
+{
+ struct dentry *root = sdw->cdns.bus.debugfs;
+
+ if (!root)
+ return;
+
+ sdw->fs = debugfs_create_dir("intel-sdw", root);
+ if (IS_ERR_OR_NULL(sdw->fs)) {
+ dev_err(sdw->cdns.dev, "debugfs root creation failed\n");
+ sdw->fs = NULL;
+ return;
+ }
+
+ debugfs_create_file("intel-registers", 0400, sdw->fs, sdw,
+ _reg_fops);
+
+ sdw_cdns_debugfs_init(>cdns, sdw->fs);
+}
I believe there should be dummy equivalent of _init and _exit if debugfs
is not enabled (if these are defined already and I've missed it, please
ignore).
I think the direction is just to keep going if there is an error or
debufs is not enabled.
+static void intel_debugfs_exit(struct sdw_intel *sdw)
+{
+ debugfs_remove_recursive(sdw->fs);
+}
Re: [alsa-devel] [RFC PATCH 03/40] soundwire: cadence_master: align debugfs to 8 digits
On 7/26/19 4:38 AM, Cezary Rojewski wrote:
On 2019-07-26 01:39, Pierre-Louis Bossart wrote:
SQUASHME
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 91e8bacb83e3..9f611a1fff0a 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -234,7 +234,7 @@ static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
char *buf, size_t pos, unsigned int reg)
{
return scnprintf(buf + pos, RD_BUF - pos,
- "%4x\t%4x\n", reg, cdns_readl(cdns, reg));
+ "%4x\t%8x\n", reg, cdns_readl(cdns, reg));
}
static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
Should just be merged together with the introducing commit. Guess it's
posted unintentionally.
Yep, I missed this, will squash in the updates as intended.
Re: [alsa-devel] [RFC PATCH 01/40] soundwire: add debugfs support
Thanks for the feedback Cezary.
+static ssize_t sdw_slave_reg_read(struct file *file, char __user
*user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct sdw_slave *slave = file->private_data;
+ unsigned int reg;
+ char *buf;
+ ssize_t ret;
+ int i, j;
+
+ buf = kzalloc(RD_BUF, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = scnprintf(buf, RD_BUF, "Register Value\n");
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP0\n");
+
+ for (i = 0; i < 6; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
In most cases explicit reg macro is used, here it's implicit. Align with
the rest?
I don't see what you are referring to, or I need more coffee.
we use this function sdw_printf in a number of places. Or are you
referring to the magic value 6? That should indeed be a macro.
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_DP0_CHANNELEN);
+ for (i = SDW_DP0_SAMPLECTRL1; i <= SDW_DP0_LANECTRL; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ ret += sdw_sprintf(slave, buf, ret,
+ SDW_DP0_CHANNELEN + SDW_BANK1_OFFSET);
+ for (i = SDW_DP0_SAMPLECTRL1 + SDW_BANK1_OFFSET;
+ i <= SDW_DP0_LANECTRL + SDW_BANK1_OFFSET; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
I'd advice to revisit macros declarations first.
There should be SDW_DP0_SAMPLECTRL1_B(bank) declared. In general all
macros for SDW should be "bank-less" (name wise). Additionally,
SDW_BANK_OFFSET(bank) could be provided for convenience i.e.: return 0
for bank0.
Yeah, there might be some speed loss in terms of operation count but in
most cases it is negligible.
Would simplify this entire reg dump greatly.
const array on top with {0, 1} elements and replacing explicit "bank0/1"
strings with "bank%d" gets code size reduced while not losing on
readability.
This could require a lot of changes in other parts of the code, and I
don't want to do this just for debugfs. It's valid point that maybe the
code can be simplified, but the changes are an across-the-board change
to be done when we don't add new functionality. I'll keep this on the
todo list.
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nSCP\n");
+ for (i = SDW_SCP_INT1; i <= SDW_SCP_BANKDELAY; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+ for (i = SDW_SCP_DEVID_0; i <= SDW_SCP_DEVID_5; i++)
+ ret += sdw_sprintf(slave, buf, ret, i);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B0);
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B0);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_FRAMECTRL_B1);
+ ret += sdw_sprintf(slave, buf, ret, SDW_SCP_NEXTFRAME_B1);
+
+ for (i = 1; i < 14; i++) {
Explicit valid slave addresses would be preferred.
no, these are ports. we should use a macro instead of the magic 14 but
it's fine to try and read all ports. As I explained it's a good way to
figure out how many ports the Slave device supports even in the absence
of documentation. This also helps figure out if the DisCo properties
make sense.
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nDP%d\n", i);
+ reg = SDW_DPN_INT(i);
+ for (j = 0; j < 6; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank0\n");
+ reg = SDW_DPN_CHANNELEN_B0(i);
+ for (j = 0; j < 9; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "Bank1\n");
+ reg = SDW_DPN_CHANNELEN_B1(i);
+ for (j = 0; j < 9; j++)
+ ret += sdw_sprintf(slave, buf, ret, reg + j);
Some sort of MAX_CHANNELS would be nice here too.
Yes, need to use macros indeed.
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{
+ struct dentry *master;
+ struct dentry *d;
+ char name[32];
+
+ master = slave->bus->debugfs;
+
+ /* create the debugfs slave-name */
+ snprintf(name, sizeof(name), "%s", dev_name(>dev));
+ d = debugfs_create_dir(name, master);
+
+ debugfs_create_file("registers", 0400, d, slave,
_slave_reg_fops);
Pointer returned by _create_file gets completely ignored here. At least
dbg msg would be nice if it fails.
+ return d;
I understood that Greg KH doesn't want us to depend on the result of
debugfs calls, but a dev_dbg is likely ok.
Re: [alsa-devel] [RFC PATCH 01/40] soundwire: add debugfs support
On 7/25/19 5:15 PM, Guennadi Liakhovetski wrote:
Hi Pierre,
A couple of nitpicks:
Thanks for the feedback!
create mode 100644 drivers/soundwire/debugfs.c
[snip]
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 3048ca153f22..06ac4adb0074 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -18,6 +18,30 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
void sdw_extract_slave_id(struct sdw_bus *bus,
u64 addr, struct sdw_slave_id *id);
+#ifdef CONFIG_DEBUG_FS
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus);
+void sdw_bus_debugfs_exit(struct dentry *d);
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave);
+void sdw_slave_debugfs_exit(struct dentry *d);
+void sdw_debugfs_init(void);
+void sdw_debugfs_exit(void);
+#else
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{ return NULL; }
static?
+
+void sdw_bus_debugfs_exit(struct dentry *d) {}
+
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{ return NULL; }
+
+void sdw_slave_debugfs_exit(struct dentry *d) {}
+
+void sdw_debugfs_init(void) {}
+
+void sdw_debugfs_exit(void) {}
Same for all the above. You could also declare them inline, but I really hope
the compiler will be smart enough to do that itself.
yes, I'll add static inline for all this.
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{
+ struct dentry *d;
I would remove the above
+ char name[16];
+
+ if (!sdw_debugfs_root)
+ return NULL;
+
+ /* create the debugfs master-N */
+ snprintf(name, sizeof(name), "master-%d", bus->link_id);
+ d = debugfs_create_dir(name, sdw_debugfs_root);
+
+ return d;
And just do
+ return debugfs_create_dir(name, sdw_debugfs_root);
yep, will do.
+static ssize_t sdw_sprintf(struct sdw_slave *slave,
+ char *buf, size_t pos, unsigned int reg)
+{
+ int value;
+
+ value = sdw_read(slave, reg);
I personally would join the two lines above, but that's just a personal
preference.
I prefer splitting variables and code, I just can't mentally split the two.
+
+ if (value < 0)
+ return scnprintf(buf + pos, RD_BUF - pos, "%3x\tXX\n", reg);
+ else
I think it's advised to not use an else in such cases.
Thanks
Guennadi
+ return scnprintf(buf + pos, RD_BUF - pos,
+ "%3x\t%2x\n", reg, value);
+}
The intent was to provide a visual cue that the register is not
implemented, which is quite useful. Not all registers are mandatory and
not all vendors document the entire set of registers, so it's a good way
to figure things out. The value is not used for any functional purpose,
it's just a register dump for the integrator to look at. I'll add a note
to explain the idea.
Re: [alsa-devel] [RFC PATCH 09/40] soundwire: cadence_master: fix usage of CONFIG_UPDATE
@@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns
*cdns)
*/
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
{
- int ret;
-
_cdns_enable_interrupt(cdns);
- ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
- CDNS_MCP_CONFIG_UPDATE_BIT);
- if (ret < 0)
- dev_err(cdns->dev, "Config update timedout\n");
- return ret;
Should we add cdns_update_config() here?
indeed, this would be a good improvement. The code works because we
added the exit_reset() sequence which does call cdns_update_config(),
but better make this function self-contained. When we enable the
clock-stop mode we will not do this reset sequence.
[RFC PATCH 40/40] soundwire: intel: add delay on restart for enumeration
We have a conceptual issue on restart: the interaction with the slaves can start before (re) enumeration is complete. Add a delay for now but we will need to have an async notification that all devices are back on the bus. Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/intel.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 1394a2322553..e4ea430b5a8e 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -1242,6 +1242,11 @@ static int intel_resume(struct device *dev) ret = sdw_cdns_exit_reset(>cdns); + /* add delay to let Slaves re-enumerate */ + usleep_range(2, 3); + + dev_dbg(dev, "%s done\n", __func__); + return ret; } -- 2.20.1
[RFC PATCH 30/40] soundwire: cadence_master: add kernel parameter to override interrupt mask
The code has a set of defaults which may not be relevant in all cases, add kernel parameter as a helper - mostly for early board bring-up. Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/cadence_master.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 18c6ac026e85..dede55072191 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -20,6 +20,10 @@ #include "bus.h" #include "cadence_master.h" +static int interrupt_mask; +module_param_named(cnds_mcp_int_mask, interrupt_mask, int, 0444); +MODULE_PARM_DESC(cdns_mcp_int_mask, "Cadence MCP IntMask"); + #define CDNS_MCP_CONFIG0x0 #define CDNS_MCP_CONFIG_MCMD_RETRY GENMASK(27, 24) @@ -830,6 +834,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns) /* now enable all of the above */ mask |= CDNS_MCP_INT_IRQ; + if (interrupt_mask) /* parameter override */ + mask = interrupt_mask; + cdns_writel(cdns, CDNS_MCP_INTMASK, mask); return do_reset(cdns); -- 2.20.1
[RFC PATCH 33/40] soundwire: intel: Add basic power management support
Implement suspend/resume capabilities (not runtime_pm for now)
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 102 ++
1 file changed, 102 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 215dc81cdf73..1477c35f616f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -278,6 +279,35 @@ static void intel_debugfs_exit(struct sdw_intel *sdw)
/*
* shim ops
*/
+static int intel_link_power_down(struct sdw_intel *sdw)
+{
+ int link_control, spa_mask, cpa_mask, ret;
+ unsigned int link_id = sdw->instance;
+ void __iomem *shim = sdw->res->shim;
+ u16 ioctl;
+
+ /* Glue logic */
+ ioctl = intel_readw(shim, SDW_SHIM_IOCTL(link_id));
+ ioctl |= SDW_SHIM_IOCTL_BKE;
+ ioctl |= SDW_SHIM_IOCTL_COE;
+ intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+
+ ioctl &= ~(SDW_SHIM_IOCTL_MIF);
+ intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
+
+ /* Link power down sequence */
+ link_control = intel_readl(shim, SDW_SHIM_LCTL);
+ spa_mask = ~(SDW_SHIM_LCTL_SPA << link_id);
+ cpa_mask = (SDW_SHIM_LCTL_CPA << link_id);
+ link_control &= spa_mask;
+
+ ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
+ if (ret < 0)
+ return ret;
+
+ sdw->cdns.link_up = false;
+ return 0;
+}
static int intel_link_power_up(struct sdw_intel *sdw)
{
@@ -300,6 +330,29 @@ static int intel_link_power_up(struct sdw_intel *sdw)
return 0;
}
+static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
+{
+ void __iomem *shim = sdw->res->shim;
+ unsigned int link_id = sdw->instance;
+ u16 wake_en, wake_sts;
+
+ if (wake_enable) {
+ /* Enable the wakeup */
+ intel_writew(shim, SDW_SHIM_WAKEEN,
+(SDW_SHIM_WAKEEN_ENABLE << link_id));
+ } else {
+ /* Disable the wake up interrupt */
+ wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
+ wake_en &= ~(SDW_SHIM_WAKEEN_ENABLE << link_id);
+ intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
+
+ /* Clear wake status */
+ wake_sts = intel_readw(shim, SDW_SHIM_WAKESTS);
+ wake_sts |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
+ intel_writew(shim, SDW_SHIM_WAKESTS_STATUS, wake_sts);
+ }
+}
+
static int intel_shim_init(struct sdw_intel *sdw)
{
void __iomem *shim = sdw->res->shim;
@@ -1095,11 +1148,60 @@ static int intel_remove(struct platform_device *pdev)
return 0;
}
+/*
+ * PM calls
+ */
+
+#ifdef CONFIG_PM
+
+static int intel_suspend(struct device *dev)
+{
+ struct sdw_intel *sdw;
+ int ret;
+
+ sdw = dev_get_drvdata(dev);
+
+ ret = intel_link_power_down(sdw);
+ if (ret) {
+ dev_err(dev, "Link power down failed: %d", ret);
+ return ret;
+ }
+
+ intel_shim_wake(sdw, false);
+
+ return 0;
+}
+
+static int intel_resume(struct device *dev)
+{
+ struct sdw_intel *sdw;
+ int ret;
+
+ sdw = dev_get_drvdata(dev);
+
+ ret = intel_init(sdw);
+ if (ret) {
+ dev_err(dev, "%s failed: %d", __func__, ret);
+ return ret;
+ }
+
+ sdw_cdns_enable_interrupt(>cdns);
+
+ return ret;
+}
+
+#endif
+
+static const struct dev_pm_ops intel_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume)
+};
+
static struct platform_driver sdw_intel_drv = {
.probe = intel_probe,
.remove = intel_remove,
.driver = {
.name = "int-sdw",
+ .pm = _pm,
},
};
--
2.20.1
[RFC PATCH 09/40] soundwire: cadence_master: fix usage of CONFIG_UPDATE
Per the hardware documentation, all changes to MCP_CONFIG,
MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL need to be validated with a
self-clearing write to MCP_CONFIG_UPDATE.
For some reason, the existing code only does this write to
CONFIG_UPDATE when enabling interrupts. Add a helper and do the update
when the CONFIG is changed.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 29 +
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 9f611a1fff0a..eb46cf651d62 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -224,6 +224,22 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int
offset, u32 value)
return -EAGAIN;
}
+/*
+ * all changes to the MCP_CONFIG, MCP_CONTROL, MCP_CMDCTRL and MCP_PHYCTRL
+ * need to be confirmed with a write to MCP_CONFIG_UPDATE
+ */
+static int cdns_update_config(struct sdw_cdns *cdns)
+{
+ int ret;
+
+ ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
+CDNS_MCP_CONFIG_UPDATE_BIT);
+ if (ret < 0)
+ dev_err(cdns->dev, "Config update timedout\n");
+
+ return ret;
+}
+
/*
* debugfs
*/
@@ -758,15 +774,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns)
*/
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
{
- int ret;
-
_cdns_enable_interrupt(cdns);
- ret = cdns_clear_bit(cdns, CDNS_MCP_CONFIG_UPDATE,
-CDNS_MCP_CONFIG_UPDATE_BIT);
- if (ret < 0)
- dev_err(cdns->dev, "Config update timedout\n");
- return ret;
+ return 0;
}
EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
@@ -943,7 +953,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_CONFIG, val);
- return 0;
+ /* commit changes */
+ ret = cdns_update_config(cdns);
+
+ return ret;
}
EXPORT_SYMBOL(sdw_cdns_init);
--
2.20.1
[RFC PATCH 37/40] soundwire: cadence_master: add hw_reset capability in debugfs
This is to kick devices into reset and see what software does
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index fa7230b0f200..53278aa2436f 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -331,6 +331,25 @@ static const struct file_operations cdns_reg_fops = {
.llseek = default_llseek,
};
+static int cdns_hw_reset(void *data, u64 value)
+{
+ struct sdw_cdns *cdns = data;
+ int ret;
+
+ if (value != 1)
+ return 0;
+
+ dev_info(cdns->dev, "starting link hw_reset\n");
+
+ ret = sdw_cdns_exit_reset(cdns);
+
+ dev_info(cdns->dev, "link hw_reset done\n");
+
+ return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(cdns_hw_reset_fops, NULL, cdns_hw_reset, "%llu\n");
+
/**
* sdw_cdns_debugfs_init() - Cadence debugfs init
* @cdns: Cadence instance
@@ -339,6 +358,9 @@ static const struct file_operations cdns_reg_fops = {
void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
{
debugfs_create_file("cdns-registers", 0400, root, cdns, _reg_fops);
+
+ debugfs_create_file_unsafe("cdns-hw-reset", 0200, root, cdns,
+ _hw_reset_fops);
}
EXPORT_SYMBOL_GPL(sdw_cdns_debugfs_init);
--
2.20.1
[RFC PATCH 11/40] soundwire: cadence_master: simplify bus clash interrupt clear
The bus clash interrupts are generated when the status is one, and
also cleared by writing a one. It's overkill/useless to use an OR when
the bit is already set.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index e85e49340986..bdc3ed844829 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -692,7 +692,6 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
if (int_status & CDNS_MCP_INT_CTRL_CLASH) {
/* Slave is driving bit slot during control word */
dev_err_ratelimited(cdns->dev, "Bus clash for control word\n");
- int_status |= CDNS_MCP_INT_CTRL_CLASH;
}
if (int_status & CDNS_MCP_INT_DATA_CLASH) {
@@ -701,7 +700,6 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
* ownership of data bits or Slave gone bonkers
*/
dev_err_ratelimited(cdns->dev, "Bus clash for data word\n");
- int_status |= CDNS_MCP_INT_DATA_CLASH;
}
if (int_status & CDNS_MCP_INT_SLAVE_MASK) {
--
2.20.1
[RFC PATCH 16/40] soundwire: cadence_master: improve startup sequence with link hw_reset
Enable interrupts first, then engage hardware bus reset with maximum
duration to make sure the Slave(s) correctly detect the reset pattern
and to ensure electrical conflicts can be resolved.
Without these changes the initialization is randomly corrupted by bus
clashes, parity errors and Slave attachment does not generate any
interrupt, despite the status showing them being attached.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 35 +-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 25d5c7267c15..442f78c00f09 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -778,6 +778,31 @@ EXPORT_SYMBOL(sdw_cdns_thread);
* init routines
*/
+static int do_reset(struct sdw_cdns *cdns)
+{
+ int ret;
+
+ /* program maximum length reset to be safe */
+ cdns_updatel(cdns, CDNS_MCP_CONTROL,
+CDNS_MCP_CONTROL_RST_DELAY,
+CDNS_MCP_CONTROL_RST_DELAY);
+
+ /* use hardware generated reset */
+ cdns_updatel(cdns, CDNS_MCP_CONTROL,
+CDNS_MCP_CONTROL_HW_RST,
+CDNS_MCP_CONTROL_HW_RST);
+
+ /* enable bus operations with clock and data */
+ cdns_updatel(cdns, CDNS_MCP_CONFIG,
+CDNS_MCP_CONFIG_OP,
+CDNS_MCP_CONFIG_OP_NORMAL);
+
+ /* commit changes */
+ ret = cdns_update_config(cdns);
+
+ return ret;
+}
+
/**
* sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
* @cdns: Cadence instance
@@ -809,7 +834,7 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
- return 0;
+ return do_reset(cdns);
}
EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
@@ -958,6 +983,10 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);
cdns_writel(cdns, CDNS_MCP_SSP_CTRL1, CDNS_DEFAULT_SSP_INTERVAL);
+ /* flush command FIFOs */
+ cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CMD_RST,
+CDNS_MCP_CONTROL_CMD_RST);
+
/* Set cmd accept mode */
cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CMD_ACCEPT,
CDNS_MCP_CONTROL_CMD_ACCEPT);
@@ -980,10 +1009,6 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
/* Set cmd mode for Tx and Rx cmds */
val &= ~CDNS_MCP_CONFIG_CMD;
- /* Set operation to normal */
- val &= ~CDNS_MCP_CONFIG_OP;
- val |= CDNS_MCP_CONFIG_OP_NORMAL;
-
cdns_writel(cdns, CDNS_MCP_CONFIG, val);
/* commit changes */
--
2.20.1
[RFC PATCH 01/40] soundwire: add debugfs support
Add base debugfs mechanism for SoundWire bus by creating soundwire
root and master-N and slave-x hierarchy.
Also add SDW Slave SCP, DP0 and DP-N register debug file.
Registers not implemented will print as "XX"
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/Makefile| 4 +-
drivers/soundwire/bus.c | 6 ++
drivers/soundwire/bus.h | 24 ++
drivers/soundwire/bus_type.c | 3 +
drivers/soundwire/debugfs.c | 156 ++
drivers/soundwire/slave.c | 1 +
include/linux/soundwire/sdw.h | 4 +
7 files changed, 197 insertions(+), 1 deletion(-)
create mode 100644 drivers/soundwire/debugfs.c
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index fd99a831b92a..88990cac48a7 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -4,7 +4,9 @@
#
#Bus Objs
-soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o
+soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
+ debugfs.o
+
obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
#Cadence Objs
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index fe745830a261..5ad4109dc72f 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -49,6 +49,8 @@ int sdw_add_bus_master(struct sdw_bus *bus)
}
}
+ bus->debugfs = sdw_bus_debugfs_init(bus);
+
/*
* Device numbers in SoundWire are 0 through 15. Enumeration device
* number (0), Broadcast device number (15), Group numbers (12 and
@@ -109,6 +111,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
struct sdw_slave *slave = dev_to_sdw_dev(dev);
struct sdw_bus *bus = slave->bus;
+ sdw_slave_debugfs_exit(slave->debugfs);
+
mutex_lock(>bus_lock);
if (slave->dev_num) /* clear dev_num if assigned */
@@ -130,6 +134,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
void sdw_delete_bus_master(struct sdw_bus *bus)
{
device_for_each_child(bus->dev, NULL, sdw_delete_slave);
+
+ sdw_bus_debugfs_exit(bus->debugfs);
}
EXPORT_SYMBOL(sdw_delete_bus_master);
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 3048ca153f22..06ac4adb0074 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -18,6 +18,30 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
void sdw_extract_slave_id(struct sdw_bus *bus,
u64 addr, struct sdw_slave_id *id);
+#ifdef CONFIG_DEBUG_FS
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus);
+void sdw_bus_debugfs_exit(struct dentry *d);
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave);
+void sdw_slave_debugfs_exit(struct dentry *d);
+void sdw_debugfs_init(void);
+void sdw_debugfs_exit(void);
+#else
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{ return NULL; }
+
+void sdw_bus_debugfs_exit(struct dentry *d) {}
+
+struct dentry *sdw_slave_debugfs_init(struct sdw_slave *slave)
+{ return NULL; }
+
+void sdw_slave_debugfs_exit(struct dentry *d) {}
+
+void sdw_debugfs_init(void) {}
+
+void sdw_debugfs_exit(void) {}
+
+#endif
+
enum {
SDW_MSG_FLAG_READ = 0,
SDW_MSG_FLAG_WRITE,
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 2655602f0cfb..4a465f55039f 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include "bus.h"
/**
* sdw_get_device_id - find the matching SoundWire device id
@@ -177,11 +178,13 @@ EXPORT_SYMBOL_GPL(sdw_unregister_driver);
static int __init sdw_bus_init(void)
{
+ sdw_debugfs_init();
return bus_register(_bus_type);
}
static void __exit sdw_bus_exit(void)
{
+ sdw_debugfs_exit();
bus_unregister(_bus_type);
}
diff --git a/drivers/soundwire/debugfs.c b/drivers/soundwire/debugfs.c
new file mode 100644
index ..8d86e100516e
--- /dev/null
+++ b/drivers/soundwire/debugfs.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2017-19 Intel Corporation.
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include "bus.h"
+
+#ifdef CONFIG_DEBUG_FS
+struct dentry *sdw_debugfs_root;
+#endif
+
+struct dentry *sdw_bus_debugfs_init(struct sdw_bus *bus)
+{
+ struct dentry *d;
+ char name[16];
+
+ if (!sdw_debugfs_root)
+ return NULL;
+
+ /* create the debugfs master-N */
+ snprintf(name, sizeof(name), "master-%d", bus->link_id);
+ d = debugfs_create_dir(name, sdw_debugfs_root);
+
+ return d;
+}
+
+void sdw_bus_debugfs_exit(struct dentry *d)
[RFC PATCH 08/40] soundwire: intel: remove BIOS work-arounds
the values passed by all existing BIOS are fine, let's use them as is. The existing code must have been needed only on early prototypes. Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/intel.c | 11 --- 1 file changed, 11 deletions(-) diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 51990b192dc0..c718c9c67a37 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -922,17 +922,6 @@ static int intel_prop_read(struct sdw_bus *bus) /* Initialize with default handler to read all DisCo properties */ sdw_master_read_prop(bus); - /* BIOS is not giving some values correctly. So, lets override them */ - bus->prop.num_clk_freq = 1; - bus->prop.clk_freq = devm_kcalloc(bus->dev, bus->prop.num_clk_freq, - sizeof(*bus->prop.clk_freq), - GFP_KERNEL); - if (!bus->prop.clk_freq) - return -ENOMEM; - - bus->prop.clk_freq[0] = bus->prop.max_clk_freq; - bus->prop.err_threshold = 5; - return 0; } -- 2.20.1
[RFC PATCH 12/40] soundwire: cadence_master: revisit interrupt settings
Adding missing interrupt masks (parity, etc) and missing checks.
Clarify which masks are for which usage.
Signed-off-by: Bard Liao
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 25 ++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index bdc3ed844829..0f3b9c160b01 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -76,9 +76,12 @@
#define CDNS_MCP_INT_DPINT BIT(11)
#define CDNS_MCP_INT_CTRL_CLASHBIT(10)
#define CDNS_MCP_INT_DATA_CLASHBIT(9)
+#define CDNS_MCP_INT_PARITYBIT(8)
#define CDNS_MCP_INT_CMD_ERR BIT(7)
+#define CDNS_MCP_INT_RX_NE BIT(3)
#define CDNS_MCP_INT_RX_WL BIT(2)
#define CDNS_MCP_INT_TXE BIT(1)
+#define CDNS_MCP_INT_TXF BIT(0)
#define CDNS_MCP_INTSET0x4C
@@ -689,6 +692,11 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
}
}
+ if (int_status & CDNS_MCP_INT_PARITY) {
+ /* Parity error detected by Master */
+ dev_err_ratelimited(cdns->dev, "Parity error\n");
+ }
+
if (int_status & CDNS_MCP_INT_CTRL_CLASH) {
/* Slave is driving bit slot during control word */
dev_err_ratelimited(cdns->dev, "Bus clash for control word\n");
@@ -761,10 +769,21 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
CDNS_MCP_SLAVE_INTMASK1_MASK);
+ /* enable detection of slave state changes */
mask = CDNS_MCP_INT_SLAVE_RSVD | CDNS_MCP_INT_SLAVE_ALERT |
- CDNS_MCP_INT_SLAVE_ATTACH | CDNS_MCP_INT_SLAVE_NATTACH |
- CDNS_MCP_INT_CTRL_CLASH | CDNS_MCP_INT_DATA_CLASH |
- CDNS_MCP_INT_RX_WL | CDNS_MCP_INT_IRQ | CDNS_MCP_INT_DPINT;
+ CDNS_MCP_INT_SLAVE_ATTACH | CDNS_MCP_INT_SLAVE_NATTACH;
+
+ /* enable detection of bus issues */
+ mask |= CDNS_MCP_INT_CTRL_CLASH | CDNS_MCP_INT_DATA_CLASH |
+ CDNS_MCP_INT_PARITY;
+
+ /* no detection of port interrupts for now */
+
+ /* enable detection of RX fifo level */
+ mask |= CDNS_MCP_INT_RX_WL;
+
+ /* now enable all of the above */
+ mask |= CDNS_MCP_INT_IRQ;
cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
--
2.20.1
[RFC PATCH 34/40] soundwire: intel: ignore disabled links for suspend/resume
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 1477c35f616f..a976480d6f36 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1161,6 +1161,12 @@ static int intel_suspend(struct device *dev)
sdw = dev_get_drvdata(dev);
+ if (sdw->cdns.bus.prop.hw_disabled) {
+ dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
+ sdw->cdns.bus.link_id);
+ return 0;
+ }
+
ret = intel_link_power_down(sdw);
if (ret) {
dev_err(dev, "Link power down failed: %d", ret);
@@ -1179,6 +1185,12 @@ static int intel_resume(struct device *dev)
sdw = dev_get_drvdata(dev);
+ if (sdw->cdns.bus.prop.hw_disabled) {
+ dev_dbg(dev, "SoundWire master %d is disabled, ignoring\n",
+ sdw->cdns.bus.link_id);
+ return 0;
+ }
+
ret = intel_init(sdw);
if (ret) {
dev_err(dev, "%s failed: %d", __func__, ret);
--
2.20.1
[RFC PATCH 10/40] soundwire: cadence_master: remove useless wrapper
Now that we've removed the update config, there's no need for a wrapper.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index eb46cf651d62..e85e49340986 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -749,7 +749,12 @@ EXPORT_SYMBOL(sdw_cdns_thread);
/*
* init routines
*/
-static int _cdns_enable_interrupt(struct sdw_cdns *cdns)
+
+/**
+ * sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
+ * @cdns: Cadence instance
+ */
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
{
u32 mask;
@@ -767,17 +772,6 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns)
return 0;
}
-
-/**
- * sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
- * @cdns: Cadence instance
- */
-int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
-{
- _cdns_enable_interrupt(cdns);
-
- return 0;
-}
EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
static int cdns_allocate_pdi(struct sdw_cdns *cdns,
--
2.20.1
[RFC PATCH 03/40] soundwire: cadence_master: align debugfs to 8 digits
SQUASHME
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 91e8bacb83e3..9f611a1fff0a 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -234,7 +234,7 @@ static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
char *buf, size_t pos, unsigned int reg)
{
return scnprintf(buf + pos, RD_BUF - pos,
-"%4x\t%4x\n", reg, cdns_readl(cdns, reg));
+"%4x\t%8x\n", reg, cdns_readl(cdns, reg));
}
static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
--
2.20.1
[RFC PATCH 06/40] soundwire: intel: prevent possible dereference in hw_params
This should not happen in production systems but we should test for
all callback arguments before invoking the config_stream callback.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 68832e613b1e..497879dd9c0d 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -509,7 +509,7 @@ static int intel_config_stream(struct sdw_intel *sdw,
struct snd_soc_dai *dai,
struct snd_pcm_hw_params *hw_params, int link_id)
{
- if (sdw->res->ops && sdw->res->ops->config_stream)
+ if (sdw->res->ops && sdw->res->ops->config_stream && sdw->res->arg)
return sdw->res->ops->config_stream(sdw->res->arg,
substream, dai, hw_params, link_id);
--
2.20.1
[RFC PATCH 25/40] soundwire: intel: use BIOS information to set clock dividers
The BIOS provides an Intel-specific property, let's use it to avoid
hard-coded clock dividers.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 26 ++
drivers/soundwire/intel.c | 26 ++
include/linux/soundwire/sdw.h | 2 ++
3 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index d84344e29f71..10ebcef2e84e 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -173,8 +173,6 @@
#define CDNS_PDI_CONFIG_PORT GENMASK(4, 0)
/* Driver defaults */
-
-#define CDNS_DEFAULT_CLK_DIVIDER 0
#define CDNS_DEFAULT_SSP_INTERVAL 0x18
#define CDNS_TX_TIMEOUT2000
@@ -973,7 +971,10 @@ static u32 cdns_set_default_frame_shape(int n_rows, int
n_cols)
*/
int sdw_cdns_init(struct sdw_cdns *cdns)
{
+ struct sdw_bus *bus = >bus;
+ struct sdw_master_prop *prop = >prop;
u32 val;
+ int divider;
int ret;
/* Exit clock stop */
@@ -985,9 +986,17 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
}
/* Set clock divider */
+ divider = (prop->mclk_freq / prop->max_clk_freq) - 1;
val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
- val |= CDNS_DEFAULT_CLK_DIVIDER;
+ val |= divider;
cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
+ cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
+
+ pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
+ prop->mclk_freq,
+ prop->max_clk_freq,
+ divider,
+ val);
/* Set the default frame shape */
val = cdns_set_default_frame_shape(prop->default_row,
@@ -1035,6 +1044,7 @@ EXPORT_SYMBOL(sdw_cdns_init);
int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params)
{
+ struct sdw_master_prop *prop = >prop;
struct sdw_cdns *cdns = bus_to_cdns(bus);
int mcp_clkctrl_off, mcp_clkctrl;
int divider;
@@ -1044,7 +1054,9 @@ int cdns_bus_conf(struct sdw_bus *bus, struct
sdw_bus_params *params)
return -EINVAL;
}
- divider = (params->max_dr_freq / params->curr_dr_freq) - 1;
+ divider = prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR /
+ params->curr_dr_freq;
+ divider--; /* divider is 1/(N+1) */
if (params->next_bank)
mcp_clkctrl_off = CDNS_MCP_CLK_CTRL1;
@@ -1055,6 +1067,12 @@ int cdns_bus_conf(struct sdw_bus *bus, struct
sdw_bus_params *params)
mcp_clkctrl |= divider;
cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
+ pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
+ prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
+ params->curr_dr_freq,
+ divider,
+ mcp_clkctrl);
+
return 0;
}
EXPORT_SYMBOL(cdns_bus_conf);
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index c718c9c67a37..796ac2bc8cea 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -917,11 +917,37 @@ static int intel_register_dai(struct sdw_intel *sdw)
dais, num_dai);
}
+static int sdw_master_read_intel_prop(struct sdw_bus *bus)
+{
+ struct sdw_master_prop *prop = >prop;
+ struct fwnode_handle *link;
+ char name[32];
+ int nval, i;
+
+ /* Find master handle */
+ snprintf(name, sizeof(name),
+"mipi-sdw-link-%d-subproperties", bus->link_id);
+
+ link = device_get_named_child_node(bus->dev, name);
+ if (!link) {
+ dev_err(bus->dev, "Master node %s not found\n", name);
+ return -EIO;
+ }
+
+ fwnode_property_read_u32(link,
+"intel-sdw-ip-clock",
+>mclk_freq);
+ return 0;
+}
+
static int intel_prop_read(struct sdw_bus *bus)
{
/* Initialize with default handler to read all DisCo properties */
sdw_master_read_prop(bus);
+ /* read Intel-specific properties */
+ sdw_master_read_intel_prop(bus);
+
return 0;
}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 31d1e8acf25b..b6acc436ac80 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -379,6 +379,7 @@ struct sdw_slave_prop {
* @dynamic_frame: Dynamic frame shape supported
* @err_threshold: Number of times that software may retry sending a single
* command
+ * @mclk_freq: clock reference passed to SoundWire Master, in Hz.
*/
struct sdw_master_prop {
u32 revision;
@@ -393,6 +394,7 @@ struct sdw_master_prop {
u32 default_col;
bool dynamic_frame;
u32 err_threshold;
+ u32 mclk_freq;
};
int sdw_master_read_prop(struct sdw_bus *bus);
--
2.20.1
[RFC PATCH 07/40] soundwire: intel: fix channel number reported by hardware
PDI2 reports an invalid count, force the correct hardware-supported
value
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 497879dd9c0d..51990b192dc0 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -401,6 +401,15 @@ intel_pdi_get_ch_cap(struct sdw_intel *sdw, unsigned int
pdi_num, bool pcm)
if (pcm) {
count = intel_readw(shim, SDW_SHIM_PCMSYCHC(link_id, pdi_num));
+
+ /*
+* TODO: pdi number 2 reports channel count as 1 even though
+* it supports 8 channel. Performing hardcoding for pdi
+* number 2.
+*/
+ if (pdi_num == 2)
+ count = 7;
+
} else {
count = intel_readw(shim, SDW_SHIM_PDMSCAP(link_id));
count = ((count & SDW_SHIM_PDMSCAP_CPSS) >>
--
2.20.1
[RFC PATCH 29/40] soundwire: intel_init: add kernel module parameter to filter out links
The hardware and ACPI info may report the presence of links that are
not physically enabled (e.g. due to pin-muxing or hardware reworks),
which in turn can result in errors being thrown. This shouldn't be the
case for production devices but will happen a lot on development
devices - even more so when they expose a connector.
Add a module parameter to filter out such links, e.g. adding the
following config to a file in /etc/modprobe.d will select the second
and third links only.
options soundwire_intel_init sdw_link_mask=0x6
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel_init.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
index 70637a0383d2..6ae8bb13f907 100644
--- a/drivers/soundwire/intel_init.c
+++ b/drivers/soundwire/intel_init.c
@@ -22,6 +22,10 @@
#define SDW_LINK_BASE 0x3
#define SDW_LINK_SIZE 0x1
+static int link_mask;
+module_param_named(sdw_link_mask, link_mask, int, 0444);
+MODULE_PARM_DESC(sdw_link_mask, "Intel link mask (one bit per link)");
+
struct sdw_link_data {
struct sdw_intel_link_res res;
struct platform_device *pdev;
@@ -83,6 +87,9 @@ static struct sdw_intel_ctx
caps = ioread32(res->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP);
caps &= GENMASK(2, 0);
+ dev_dbg(>dev, "SoundWire links: BIOS count %d hardware caps %d\n",
+ count, caps);
+
/* Check HW supported vs property value and use min of two */
count = min_t(u8, caps, count);
@@ -111,6 +118,13 @@ static struct sdw_intel_ctx
/* Create SDW Master devices */
for (i = 0; i < count; i++) {
+ if (link_mask && !(link_mask & BIT(i))) {
+ dev_dbg(>dev,
+ "Link %d masked, will not be enabled\n", i);
+ link++;
+ continue;
+ }
+
link->res.irq = res->irq;
link->res.registers = res->mmio_base + SDW_LINK_BASE
+ (SDW_LINK_SIZE * i);
--
2.20.1
[RFC PATCH 19/40] soundwire: bus: improve dynamic debug comments for enumeration
update comments to provide better understanding of enumeration flows.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/bus.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index bca378806d00..2354675ef104 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -483,7 +483,8 @@ static int sdw_assign_device_num(struct sdw_slave *slave)
ret = sdw_write(slave, SDW_SCP_DEVNUMBER, dev_num);
if (ret < 0) {
- dev_err(>dev, "Program device_num failed: %d\n", ret);
+ dev_err(>dev, "Program device_num %d failed: %d\n",
+ dev_num, ret);
return ret;
}
@@ -540,6 +541,7 @@ static int sdw_program_device_num(struct sdw_bus *bus)
do {
ret = sdw_transfer(bus, );
if (ret == -ENODATA) { /* end of device id reads */
+ dev_dbg(bus->dev, "No more devices to enumerate\n");
ret = 0;
break;
}
@@ -982,6 +984,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
int i, ret = 0;
if (status[0] == SDW_SLAVE_ATTACHED) {
+ dev_err(bus->dev, "Slave attached, programming device
number\n");
ret = sdw_program_device_num(bus);
if (ret)
dev_err(bus->dev, "Slave attach failed: %d\n", ret);
--
2.20.1
[RFC PATCH 13/40] soundwire: cadence_master: fix register definition for SLAVE_STATE
wrong prefix and wrong macro. Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/cadence_master.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 0f3b9c160b01..d9d9e3d964dd 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -85,8 +85,8 @@ #define CDNS_MCP_INTSET0x4C -#define CDNS_SDW_SLAVE_STAT0x50 -#define CDNS_MCP_SLAVE_STAT_MASK BIT(1, 0) +#define CDNS_MCP_SLAVE_STAT0x50 +#define CDNS_MCP_SLAVE_STAT_MASK GENMASK(1, 0) #define CDNS_MCP_SLAVE_INTSTAT00x54 #define CDNS_MCP_SLAVE_INTSTAT10x58 -- 2.20.1
[RFC PATCH 36/40] soundwire: intel: disable interrupts on suspend
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 42 +-
drivers/soundwire/cadence_master.h | 2 +-
drivers/soundwire/intel.c | 6 +++--
3 files changed, 29 insertions(+), 21 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index f486fe15fb46..fa7230b0f200 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -814,33 +814,39 @@ EXPORT_SYMBOL(sdw_cdns_exit_reset);
* sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
* @cdns: Cadence instance
*/
-int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
{
u32 mask;
- cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
- CDNS_MCP_SLAVE_INTMASK0_MASK);
- cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
- CDNS_MCP_SLAVE_INTMASK1_MASK);
+ if (state) {
+ cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
+ CDNS_MCP_SLAVE_INTMASK0_MASK);
+ cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
+ CDNS_MCP_SLAVE_INTMASK1_MASK);
- /* enable detection of slave state changes */
- mask = CDNS_MCP_INT_SLAVE_RSVD | CDNS_MCP_INT_SLAVE_ALERT |
- CDNS_MCP_INT_SLAVE_ATTACH | CDNS_MCP_INT_SLAVE_NATTACH;
+ /* enable detection of slave state changes */
+ mask = CDNS_MCP_INT_SLAVE_RSVD | CDNS_MCP_INT_SLAVE_ALERT |
+ CDNS_MCP_INT_SLAVE_ATTACH | CDNS_MCP_INT_SLAVE_NATTACH;
- /* enable detection of bus issues */
- mask |= CDNS_MCP_INT_CTRL_CLASH | CDNS_MCP_INT_DATA_CLASH |
- CDNS_MCP_INT_PARITY;
+ /* enable detection of bus issues */
+ mask |= CDNS_MCP_INT_CTRL_CLASH | CDNS_MCP_INT_DATA_CLASH |
+ CDNS_MCP_INT_PARITY;
- /* no detection of port interrupts for now */
+ /* no detection of port interrupts for now */
- /* enable detection of RX fifo level */
- mask |= CDNS_MCP_INT_RX_WL;
+ /* enable detection of RX fifo level */
+ mask |= CDNS_MCP_INT_RX_WL;
- /* now enable all of the above */
- mask |= CDNS_MCP_INT_IRQ;
+ /* now enable all of the above */
+ mask |= CDNS_MCP_INT_IRQ;
- if (interrupt_mask) /* parameter override */
- mask = interrupt_mask;
+ if (interrupt_mask) /* parameter override */
+ mask = interrupt_mask;
+ } else {
+ cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, 0);
+ cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, 0);
+ mask = 0;
+ }
cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
diff --git a/drivers/soundwire/cadence_master.h
b/drivers/soundwire/cadence_master.h
index 2b551f9226f3..1a0ba36dd78f 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -162,7 +162,7 @@ int sdw_cdns_init(struct sdw_cdns *cdns);
int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
struct sdw_cdns_stream_config config);
int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
-int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 9ebe38e4d979..1192d5775484 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1110,7 +1110,7 @@ static int intel_probe(struct platform_device *pdev)
goto err_init;
}
- ret = sdw_cdns_enable_interrupt(>cdns);
+ ret = sdw_cdns_enable_interrupt(>cdns, true);
ret = sdw_cdns_exit_reset(>cdns);
@@ -1169,6 +1169,8 @@ static int intel_suspend(struct device *dev)
return 0;
}
+ sdw_cdns_enable_interrupt(>cdns, false);
+
ret = intel_link_power_down(sdw);
if (ret) {
dev_err(dev, "Link power down failed: %d", ret);
@@ -1199,7 +1201,7 @@ static int intel_resume(struct device *dev)
return ret;
}
- sdw_cdns_enable_interrupt(>cdns);
+ sdw_cdns_enable_interrupt(>cdns, true);
ret = sdw_cdns_exit_reset(>cdns);
--
2.20.1
[RFC PATCH 15/40] soundwire: cadence_master: handle multiple status reports per Slave
When a Slave reports multiple status in the sticky bits, find the
latest configuration from the mirror of the PING frame status and
update the status directly.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 34 --
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 889fa2cd49ae..25d5c7267c15 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -643,13 +643,35 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns,
/* first check if Slave reported multiple status */
if (set_status > 1) {
+ u32 val;
+
dev_warn_ratelimited(cdns->dev,
-"Slave reported multiple Status:
%d\n",
-mask);
- /*
-* TODO: we need to reread the status here by
-* issuing a PING cmd
-*/
+"Slave %d reported multiple
Status: %d\n",
+i, mask);
+
+ /* re-check latest status extracted from PING commands
*/
+ val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT);
+ val >>= (i * 2);
+
+ switch (val & 0x3) {
+ case 0:
+ status[i] = SDW_SLAVE_UNATTACHED;
+ break;
+ case 1:
+ status[i] = SDW_SLAVE_ATTACHED;
+ break;
+ case 2:
+ status[i] = SDW_SLAVE_ALERT;
+ break;
+ default:
+ status[i] = SDW_SLAVE_RESERVED;
+ break;
+ }
+
+ dev_warn_ratelimited(cdns->dev,
+"Slave %d status updated to %d\n",
+i, status[i]);
+
}
}
--
2.20.1
[RFC PATCH 35/40] soundwire: intel: export helper to exit reset
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 9 +++--
drivers/soundwire/cadence_master.h | 1 +
drivers/soundwire/intel.c | 4
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 4a189e487830..f486fe15fb46 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -780,7 +780,11 @@ EXPORT_SYMBOL(sdw_cdns_thread);
* init routines
*/
-static int do_reset(struct sdw_cdns *cdns)
+/**
+ * sdw_cdns_exit_reset() - Program reset parameters and start bus operations
+ * @cdns: Cadence instance
+ */
+int sdw_cdns_exit_reset(struct sdw_cdns *cdns)
{
int ret;
@@ -804,6 +808,7 @@ static int do_reset(struct sdw_cdns *cdns)
return ret;
}
+EXPORT_SYMBOL(sdw_cdns_exit_reset);
/**
* sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
@@ -839,7 +844,7 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
- return do_reset(cdns);
+ return 0;
}
EXPORT_SYMBOL(sdw_cdns_enable_interrupt);
diff --git a/drivers/soundwire/cadence_master.h
b/drivers/soundwire/cadence_master.h
index de97bc22acb7..2b551f9226f3 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -161,6 +161,7 @@ irqreturn_t sdw_cdns_thread(int irq, void *dev_id);
int sdw_cdns_init(struct sdw_cdns *cdns);
int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
struct sdw_cdns_stream_config config);
+int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index a976480d6f36..9ebe38e4d979 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1112,6 +1112,8 @@ static int intel_probe(struct platform_device *pdev)
ret = sdw_cdns_enable_interrupt(>cdns);
+ ret = sdw_cdns_exit_reset(>cdns);
+
/* Register DAIs */
ret = intel_register_dai(sdw);
if (ret) {
@@ -1199,6 +1201,8 @@ static int intel_resume(struct device *dev)
sdw_cdns_enable_interrupt(>cdns);
+ ret = sdw_cdns_exit_reset(>cdns);
+
return ret;
}
--
2.20.1
[RFC PATCH 02/40] soundwire: cadence_master: add debugfs register dump
Add debugfs file to dump the Cadence master registers
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 98 ++
drivers/soundwire/cadence_master.h | 2 +
2 files changed, 100 insertions(+)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index ff4badc9b3de..91e8bacb83e3 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -8,6 +8,7 @@
#include
#include
+#include
#include
#include
#include
@@ -223,6 +224,103 @@ static int cdns_clear_bit(struct sdw_cdns *cdns, int
offset, u32 value)
return -EAGAIN;
}
+/*
+ * debugfs
+ */
+
+#define RD_BUF (2 * PAGE_SIZE)
+
+static ssize_t cdns_sprintf(struct sdw_cdns *cdns,
+ char *buf, size_t pos, unsigned int reg)
+{
+ return scnprintf(buf + pos, RD_BUF - pos,
+"%4x\t%4x\n", reg, cdns_readl(cdns, reg));
+}
+
+static ssize_t cdns_reg_read(struct file *file, char __user *user_buf,
+size_t count, loff_t *ppos)
+{
+ struct sdw_cdns *cdns = file->private_data;
+ char *buf;
+ ssize_t ret;
+ int i, j;
+
+ buf = kzalloc(RD_BUF, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = scnprintf(buf, RD_BUF, "Register Value\n");
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nMCP Registers\n");
+ for (i = 0; i < 8; i++) /* 8 MCP registers */
+ ret += cdns_sprintf(cdns, buf, ret, i * 4);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+"\nStatus & Intr Registers\n");
+ for (i = 0; i < 13; i++) /* 13 Status & Intr registers */
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_STAT + i * 4);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+"\nSSP & Clk ctrl Registers\n");
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL0);
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_SSP_CTRL1);
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL0);
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_MCP_CLK_CTRL1);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+"\nDPn B0 Registers\n");
+ for (i = 0; i < 7; i++) {
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+"\nDP-%d\n", i);
+ for (j = 0; j < 6; j++)
+ ret += cdns_sprintf(cdns, buf, ret,
+ CDNS_DPN_B0_CONFIG(i) + j * 4);
+ }
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+"\nDPn B1 Registers\n");
+ for (i = 0; i < 7; i++) {
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+"\nDP-%d\n", i);
+
+ for (j = 0; j < 6; j++)
+ ret += cdns_sprintf(cdns, buf, ret,
+ CDNS_DPN_B1_CONFIG(i) + j * 4);
+ }
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+"\nDPn Control Registers\n");
+ for (i = 0; i < 7; i++)
+ ret += cdns_sprintf(cdns, buf, ret,
+ CDNS_PORTCTRL + i * CDNS_PORT_OFFSET);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret,
+"\nPDIn Config Registers\n");
+ for (i = 0; i < 7; i++)
+ ret += cdns_sprintf(cdns, buf, ret, CDNS_PDI_CONFIG(i));
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+ kfree(buf);
+
+ return ret;
+}
+
+static const struct file_operations cdns_reg_fops = {
+ .open = simple_open,
+ .read = cdns_reg_read,
+ .llseek = default_llseek,
+};
+
+/**
+ * sdw_cdns_debugfs_init() - Cadence debugfs init
+ * @cdns: Cadence instance
+ * @root: debugfs root
+ */
+void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root)
+{
+ debugfs_create_file("cdns-registers", 0400, root, cdns, _reg_fops);
+}
+EXPORT_SYMBOL_GPL(sdw_cdns_debugfs_init);
+
/*
* IO Calls
*/
diff --git a/drivers/soundwire/cadence_master.h
b/drivers/soundwire/cadence_master.h
index fe2af62958b1..c0bf6ff00a44 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -163,6 +163,8 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
struct sdw_cdns_stream_config config);
int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
+void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
+
int sdw_cdns_get_stream(struct sdw_cdns *cdns,
struct sdw_cdns_streams *stream,
u32 ch, u32 dir);
--
2.20.1
[RFC PATCH 31/40] soundwire: intel: move shutdown() callback and don't export symbol
All DAI callbacks are in intel.c except for shutdown. Move and remove
export symbol
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 14 --
drivers/soundwire/cadence_master.h | 2 --
drivers/soundwire/intel.c | 17 +++--
3 files changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index dede55072191..4a189e487830 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -1381,19 +1381,5 @@ int sdw_cdns_alloc_stream(struct sdw_cdns *cdns,
}
EXPORT_SYMBOL(sdw_cdns_alloc_stream);
-void sdw_cdns_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct sdw_cdns_dma_data *dma;
-
- dma = snd_soc_dai_get_dma_data(dai, substream);
- if (!dma)
- return;
-
- snd_soc_dai_set_dma_data(dai, substream, NULL);
- kfree(dma);
-}
-EXPORT_SYMBOL(sdw_cdns_shutdown);
-
MODULE_LICENSE("Dual BSD/GPL");
MODULE_DESCRIPTION("Cadence Soundwire Library");
diff --git a/drivers/soundwire/cadence_master.h
b/drivers/soundwire/cadence_master.h
index d375bbfead18..de97bc22acb7 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -177,8 +177,6 @@ int sdw_cdns_alloc_stream(struct sdw_cdns *cdns,
void sdw_cdns_config_stream(struct sdw_cdns *cdns, struct sdw_cdns_port *port,
u32 ch, u32 dir, struct sdw_cdns_pdi *pdi);
-void sdw_cdns_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai);
int sdw_cdns_pcm_set_stream(struct snd_soc_dai *dai,
void *stream, int direction);
int sdw_cdns_pdm_set_stream(struct snd_soc_dai *dai,
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 5947fa8e840b..c40ab443e723 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -772,6 +772,19 @@ intel_hw_free(struct snd_pcm_substream *substream, struct
snd_soc_dai *dai)
return ret;
}
+void intel_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct sdw_cdns_dma_data *dma;
+
+ dma = snd_soc_dai_get_dma_data(dai, substream);
+ if (!dma)
+ return;
+
+ snd_soc_dai_set_dma_data(dai, substream, NULL);
+ kfree(dma);
+}
+
static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai,
void *stream, int direction)
{
@@ -787,14 +800,14 @@ static int intel_pdm_set_sdw_stream(struct snd_soc_dai
*dai,
static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
.hw_params = intel_hw_params,
.hw_free = intel_hw_free,
- .shutdown = sdw_cdns_shutdown,
+ .shutdown = intel_shutdown,
.set_sdw_stream = intel_pcm_set_sdw_stream,
};
static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
.hw_params = intel_hw_params,
.hw_free = intel_hw_free,
- .shutdown = sdw_cdns_shutdown,
+ .shutdown = intel_shutdown,
.set_sdw_stream = intel_pdm_set_sdw_stream,
};
--
2.20.1
[RFC PATCH 26/40] soundwire: cadence_master: fix divider setting in clock register
From: Rander Wang
The existing code uses an OR operation which would mix the original
divider setting with the new one, resulting in an invalid
configuration that can make codecs hang.
Add the mask definition and use cdns_updatel to update divider
Signed-off-by: Rander Wang
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 10ebcef2e84e..18c6ac026e85 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -57,6 +57,7 @@
#define CDNS_MCP_SSP_CTRL1 0x28
#define CDNS_MCP_CLK_CTRL0 0x30
#define CDNS_MCP_CLK_CTRL1 0x38
+#define CDNS_MCP_CLK_MCLKD_MASKGENMASK(7, 0)
#define CDNS_MCP_STAT 0x40
@@ -988,9 +989,11 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
/* Set clock divider */
divider = (prop->mclk_freq / prop->max_clk_freq) - 1;
val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
- val |= divider;
- cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
- cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
+
+ cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
+CDNS_MCP_CLK_MCLKD_MASK, divider);
+ cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
+CDNS_MCP_CLK_MCLKD_MASK, divider);
pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
prop->mclk_freq,
@@ -1064,8 +1067,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct
sdw_bus_params *params)
mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);
- mcp_clkctrl |= divider;
- cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
+ cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK, divider);
pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
--
2.20.1
[RFC PATCH 17/40] soundwire: bus: use runtime_pm_get_sync/pm when enabled
Not all platforms support runtime_pm for now, let's use runtime_pm
only when enabled.
Suggested-by: Srinivas Kandagatla
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/bus.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 5ad4109dc72f..0a45dc5713df 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -332,12 +332,16 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t
count, u8 *val)
if (ret < 0)
return ret;
- ret = pm_runtime_get_sync(slave->bus->dev);
- if (ret < 0)
- return ret;
+ if (pm_runtime_enabled(slave->bus->dev)) {
+ ret = pm_runtime_get_sync(slave->bus->dev);
+ if (ret < 0)
+ return ret;
+ }
ret = sdw_transfer(slave->bus, );
- pm_runtime_put(slave->bus->dev);
+
+ if (pm_runtime_enabled(slave->bus->dev))
+ pm_runtime_put(slave->bus->dev);
return ret;
}
@@ -359,13 +363,16 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t
count, u8 *val)
slave->dev_num, SDW_MSG_FLAG_WRITE, val);
if (ret < 0)
return ret;
-
- ret = pm_runtime_get_sync(slave->bus->dev);
- if (ret < 0)
- return ret;
+ if (pm_runtime_enabled(slave->bus->dev)) {
+ ret = pm_runtime_get_sync(slave->bus->dev);
+ if (ret < 0)
+ return ret;
+ }
ret = sdw_transfer(slave->bus, );
- pm_runtime_put(slave->bus->dev);
+
+ if (pm_runtime_enabled(slave->bus->dev))
+ pm_runtime_put(slave->bus->dev);
return ret;
}
--
2.20.1
[RFC PATCH 27/40] soundwire: Add Intel resource management algorithm
This algorithm computes bus parameters like clock frequency, frame
shape and port transport parameters based on active stream(s) running
on the bus.
This implementation is optimal for Intel platforms. Developers can
also implement their own .compute_params() callback for specific
resource management algorithm.
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. All hard-coded
values were removed from the initial contribution to use BIOS
information instead.
FIXME: remove checkpatch report
WARNING: Reusing the krealloc arg is almost always a bug
+ group->rates = krealloc(group->rates,
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/Makefile | 2 +-
drivers/soundwire/algo_dynamic_allocation.c | 403
drivers/soundwire/bus.c | 3 +
drivers/soundwire/bus.h | 46 ++-
drivers/soundwire/stream.c | 20 +
include/linux/soundwire/sdw.h | 5 +
6 files changed, 476 insertions(+), 3 deletions(-)
create mode 100644 drivers/soundwire/algo_dynamic_allocation.c
diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile
index 88990cac48a7..f59a9d4a28fd 100644
--- a/drivers/soundwire/Makefile
+++ b/drivers/soundwire/Makefile
@@ -5,7 +5,7 @@
#Bus Objs
soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o \
- debugfs.o
+ debugfs.o algo_dynamic_allocation.o
obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o
diff --git a/drivers/soundwire/algo_dynamic_allocation.c
b/drivers/soundwire/algo_dynamic_allocation.c
new file mode 100644
index ..89edb39162b8
--- /dev/null
+++ b/drivers/soundwire/algo_dynamic_allocation.c
@@ -0,0 +1,403 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2015-18 Intel Corporation.
+
+/*
+ * Bandwidth management algorithm based on 2^n gears
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include "bus.h"
+
+#define SDW_STRM_RATE_GROUPING 1
+
+struct sdw_group_params {
+ unsigned int rate;
+ int full_bw;
+ int payload_bw;
+ int hwidth;
+};
+
+struct sdw_group {
+ unsigned int count;
+ unsigned int max_size;
+ unsigned int *rates;
+};
+
+struct sdw_transport_data {
+ int hstart;
+ int hstop;
+ int block_offset;
+ int sub_block_offset;
+};
+
+static void sdw_compute_slave_ports(struct sdw_master_runtime *m_rt,
+ struct sdw_transport_data *t_data)
+{
+ struct sdw_slave_runtime *s_rt = NULL;
+ struct sdw_port_runtime *p_rt;
+ int port_bo, sample_int;
+ unsigned int rate, bps, ch = 0;
+
+ port_bo = t_data->block_offset;
+
+ list_for_each_entry(s_rt, _rt->slave_rt_list, m_rt_node) {
+ rate = m_rt->stream->params.rate;
+ bps = m_rt->stream->params.bps;
+ sample_int = (m_rt->bus->params.curr_dr_freq / rate);
+
+ list_for_each_entry(p_rt, _rt->port_list, port_node) {
+ ch = sdw_ch_mask_to_ch(p_rt->ch_mask);
+
+ sdw_fill_xport_params(_rt->transport_params,
+ p_rt->num, true,
+ SDW_BLK_GRP_CNT_1,
+ sample_int, port_bo, port_bo >> 8,
+ t_data->hstart,
+ t_data->hstop,
+ (SDW_BLK_GRP_CNT_1 * ch), 0x0);
+
+ sdw_fill_port_params(_rt->port_params,
+p_rt->num, bps,
+SDW_PORT_FLOW_MODE_ISOCH,
+SDW_PORT_DATA_MODE_NORMAL);
+
+ port_bo += bps * ch;
+ }
+ }
+}
+
+static void sdw_compute_master_ports(struct sdw_master_runtime *m_rt,
+struct sdw_group_params *params,
+int port_bo, int hstop)
+{
+ struct sdw_transport_data t_data = {0};
+ struct sdw_port_runtime *p_rt;
+ struct sdw_bus *bus = m_rt->bus;
+ int sample_int, hstart = 0;
+ unsigned int rate, bps, ch, no_ch;
+
+ rate = m_rt->stream->params.rate;
+ bps = m_rt->stream->params.bps;
+ ch = m_rt->ch_count;
+ sample_int = (bus->params.curr_dr_freq / rate);
+
+ if (rate != params->rate)
+ return;
+
+ t_data.hstop = hstop;
+ hstart = hstop - params->hwidth + 1;
+ t_data.hstart = hstart;
+
+ list_for_each_entry(p_rt, _rt->port_list, port_node) {
+ no_ch = sdw_ch
[RFC PATCH 22/40] soundwire: include mod_devicetable.h to avoid compiling warnings
From: Bard liao Reported-by: kbuild test robot Signed-off-by: Bard liao Signed-off-by: Pierre-Louis Bossart --- include/linux/soundwire/sdw.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index a49028e9d666..31d1e8acf25b 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -4,6 +4,8 @@ #ifndef __SOUNDWIRE_H #define __SOUNDWIRE_H +#include + struct sdw_bus; struct sdw_slave; -- 2.20.1
[RFC PATCH 28/40] soundwire: intel: handle disabled links
On most hardware platforms, SoundWire interfaces are pin-muxed with
other interfaces (typically DMIC or I2S) and the status of each link
needs to be checked at boot time.
For Intel platforms, the BIOS provides a menu to enable/disable the
links separately, and the information is provided to the OS with an
Intel-specific _DSD property. The same capability will be added to
revisions of the MIPI DisCo specification.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 26 ++
include/linux/soundwire/sdw.h | 2 ++
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 796ac2bc8cea..5947fa8e840b 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -90,6 +90,8 @@
#define SDW_ALH_STRMZCFG_DMAT GENMASK(7, 0)
#define SDW_ALH_STRMZCFG_CHN GENMASK(19, 16)
+#define SDW_INTEL_QUIRK_MASK_BUS_DISABLE BIT(1)
+
enum intel_pdi_type {
INTEL_PDI_IN = 0,
INTEL_PDI_OUT = 1,
@@ -922,7 +924,7 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
struct sdw_master_prop *prop = >prop;
struct fwnode_handle *link;
char name[32];
- int nval, i;
+ u32 quirk_mask;
/* Find master handle */
snprintf(name, sizeof(name),
@@ -937,6 +939,14 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
fwnode_property_read_u32(link,
"intel-sdw-ip-clock",
>mclk_freq);
+
+ fwnode_property_read_u32(link,
+"intel-quirk-mask",
+_mask);
+
+ if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE)
+ prop->hw_disabled = true;
+
return 0;
}
@@ -997,6 +1007,12 @@ static int intel_probe(struct platform_device *pdev)
goto err_master_reg;
}
+ if (sdw->cdns.bus.prop.hw_disabled) {
+ dev_info(>dev, "SoundWire master %d is disabled,
ignoring\n",
+sdw->cdns.bus.link_id);
+ return 0;
+ }
+
/* Initialize shim and controller */
intel_link_power_up(sdw);
intel_shim_init(sdw);
@@ -1050,9 +1066,11 @@ static int intel_remove(struct platform_device *pdev)
sdw = platform_get_drvdata(pdev);
- intel_debugfs_exit(sdw);
- free_irq(sdw->res->irq, sdw);
- snd_soc_unregister_component(sdw->cdns.dev);
+ if (!sdw->cdns.bus.prop.hw_disabled) {
+ intel_debugfs_exit(sdw);
+ free_irq(sdw->res->irq, sdw);
+ snd_soc_unregister_component(sdw->cdns.dev);
+ }
sdw_delete_bus_master(>cdns.bus);
return 0;
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index c7dfc824be80..f78b076a8782 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -380,6 +380,7 @@ struct sdw_slave_prop {
* @err_threshold: Number of times that software may retry sending a single
* command
* @mclk_freq: clock reference passed to SoundWire Master, in Hz.
+ * @hw_disabled: if true, the Master is not functional, typically due to
pin-mux
*/
struct sdw_master_prop {
u32 revision;
@@ -395,6 +396,7 @@ struct sdw_master_prop {
bool dynamic_frame;
u32 err_threshold;
u32 mclk_freq;
+ bool hw_disabled;
};
int sdw_master_read_prop(struct sdw_bus *bus);
--
2.20.1
[RFC PATCH 24/40] soundwire: cadence_master: use BIOS defaults for frame shape
Remove hard-coding and use BIOS values. If they are wrong use default
48x2 frame shape.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 442f78c00f09..d84344e29f71 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -175,7 +175,6 @@
/* Driver defaults */
#define CDNS_DEFAULT_CLK_DIVIDER 0
-#define CDNS_DEFAULT_FRAME_SHAPE 0x30
#define CDNS_DEFAULT_SSP_INTERVAL 0x18
#define CDNS_TX_TIMEOUT2000
@@ -954,6 +953,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
}
EXPORT_SYMBOL(sdw_cdns_pdi_init);
+static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
+{
+ u32 val;
+ int c;
+ int r;
+
+ r = sdw_find_row_index(n_rows);
+ c = sdw_find_col_index(n_cols);
+
+ val = (r << 3) | c;
+
+ return val;
+}
+
/**
* sdw_cdns_init() - Cadence initialization
* @cdns: Cadence instance
@@ -977,7 +990,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
/* Set the default frame shape */
- cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, CDNS_DEFAULT_FRAME_SHAPE);
+ val = cdns_set_default_frame_shape(prop->default_row,
+ prop->default_col);
+ cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
/* Set SSP interval to default value */
cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);
--
2.20.1
[RFC PATCH 32/40] soundwire: intel: add helper for initialization
Move code to helper for reuse in power management routines
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index c40ab443e723..215dc81cdf73 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -984,6 +984,15 @@ static struct sdw_master_ops sdw_intel_ops = {
.post_bank_switch = intel_post_bank_switch,
};
+static int intel_init(struct sdw_intel *sdw)
+{
+ /* Initialize shim and controller */
+ intel_link_power_up(sdw);
+ intel_shim_init(sdw);
+
+ return sdw_cdns_init(>cdns);
+}
+
/*
* probe and init
*/
@@ -1026,11 +1035,8 @@ static int intel_probe(struct platform_device *pdev)
return 0;
}
- /* Initialize shim and controller */
- intel_link_power_up(sdw);
- intel_shim_init(sdw);
-
- ret = sdw_cdns_init(>cdns);
+ /* Initialize shim, controller and Cadence IP */
+ ret = intel_init(sdw);
if (ret)
goto err_init;
--
2.20.1
[RFC PATCH 18/40] soundwire: bus: split handling of Device0 events
Assigning a device number to a Slave will result in additional events when it reports its status in a PING frame. There is no point in dealing with all the other devices in a loop, this can be done when a non-device0 event happens. Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/bus.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 0a45dc5713df..bca378806d00 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -985,6 +985,11 @@ int sdw_handle_slave_status(struct sdw_bus *bus, ret = sdw_program_device_num(bus); if (ret) dev_err(bus->dev, "Slave attach failed: %d\n", ret); + /* +* programming a device number will have side effects, +* so we deal with other devices at a later time +*/ + return ret; } /* Continue to check other slave statuses */ -- 2.20.1
[RFC PATCH 20/40] soundwire: prototypes for suspend/resume
Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/cadence_master.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index c0bf6ff00a44..d375bbfead18 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -165,6 +165,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns); void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root); +int sdw_cdns_suspend(struct sdw_cdns *cdns); +bool sdw_cdns_check_resume_status(struct sdw_cdns *cdns); + int sdw_cdns_get_stream(struct sdw_cdns *cdns, struct sdw_cdns_streams *stream, u32 ch, u32 dir); -- 2.20.1
[RFC PATCH 21/40] soundwire: export helpers to find row and column values
Add a prefix for common tables and export 2 helpers to set the frame
shapes based on row/col values.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/bus.h| 7 +--
drivers/soundwire/stream.c | 14 --
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 06ac4adb0074..c57c9c23f6ca 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -73,8 +73,11 @@ struct sdw_msg {
#define SDW_DOUBLE_RATE_FACTOR 2
-extern int rows[SDW_FRAME_ROWS];
-extern int cols[SDW_FRAME_COLS];
+extern int sdw_rows[SDW_FRAME_ROWS];
+extern int sdw_cols[SDW_FRAME_COLS];
+
+int sdw_find_row_index(int row);
+int sdw_find_col_index(int col);
/**
* sdw_port_runtime: Runtime port parameters for Master or Slave
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index a0476755a459..53f5e790fcd7 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -21,37 +21,39 @@
* The rows are arranged as per the array index value programmed
* in register. The index 15 has dummy value 0 in order to fill hole.
*/
-int rows[SDW_FRAME_ROWS] = {48, 50, 60, 64, 75, 80, 125, 147,
+int sdw_rows[SDW_FRAME_ROWS] = {48, 50, 60, 64, 75, 80, 125, 147,
96, 100, 120, 128, 150, 160, 250, 0,
192, 200, 240, 256, 72, 144, 90, 180};
-int cols[SDW_FRAME_COLS] = {2, 4, 6, 8, 10, 12, 14, 16};
+int sdw_cols[SDW_FRAME_COLS] = {2, 4, 6, 8, 10, 12, 14, 16};
-static int sdw_find_col_index(int col)
+int sdw_find_col_index(int col)
{
int i;
for (i = 0; i < SDW_FRAME_COLS; i++) {
- if (cols[i] == col)
+ if (sdw_cols[i] == col)
return i;
}
pr_warn("Requested column not found, selecting lowest column no: 2\n");
return 0;
}
+EXPORT_SYMBOL(sdw_find_col_index);
-static int sdw_find_row_index(int row)
+int sdw_find_row_index(int row)
{
int i;
for (i = 0; i < SDW_FRAME_ROWS; i++) {
- if (rows[i] == row)
+ if (sdw_rows[i] == row)
return i;
}
pr_warn("Requested row not found, selecting lowest row no: 48\n");
return 0;
}
+EXPORT_SYMBOL(sdw_find_row_index);
static int _sdw_program_slave_port_params(struct sdw_bus *bus,
struct sdw_slave *slave,
--
2.20.1
[RFC PATCH 23/40] soundwire: stream: fix disable sequence
When we disable the stream and then call hw_free, two bank switches
will be handled and as a result we re-enable the stream on hw_free.
Make sure the stream is disabled on both banks.
TODO: we need to completely revisit all this and make sure we have a
mirroring mechanism between current and alternate banks.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/stream.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 53f5e790fcd7..75b9ad1fb1a6 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1637,7 +1637,24 @@ static int _sdw_disable_stream(struct sdw_stream_runtime
*stream)
}
}
- return do_bank_switch(stream);
+ ret = do_bank_switch(stream);
+ if (ret < 0) {
+ dev_err(bus->dev, "Bank switch failed: %d\n", ret);
+ return ret;
+ }
+
+ /* make sure alternate bank (previous current) is also disabled */
+ list_for_each_entry(m_rt, >master_list, stream_node) {
+ bus = m_rt->bus;
+ /* Disable port(s) */
+ ret = sdw_enable_disable_ports(m_rt, false);
+ if (ret < 0) {
+ dev_err(bus->dev, "Disable port(s) failed: %d\n", ret);
+ return ret;
+ }
+ }
+
+ return 0;
}
/**
--
2.20.1
[RFC PATCH 38/40] soundwire: cadence_master: make clock stop exit configurable on init
The use of clock stop is not a requirement, the IP can e.g. be
completely power gated and not detect any wakes while in s2idle/deep
sleep.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/cadence_master.c | 15 ---
drivers/soundwire/cadence_master.h | 2 +-
drivers/soundwire/intel.c | 2 +-
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c
b/drivers/soundwire/cadence_master.c
index 53278aa2436f..4ab6f70d7705 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -1010,7 +1010,7 @@ static u32 cdns_set_default_frame_shape(int n_rows, int
n_cols)
* sdw_cdns_init() - Cadence initialization
* @cdns: Cadence instance
*/
-int sdw_cdns_init(struct sdw_cdns *cdns)
+int sdw_cdns_init(struct sdw_cdns *cdns, bool clock_stop_exit)
{
struct sdw_bus *bus = >bus;
struct sdw_master_prop *prop = >prop;
@@ -1018,12 +1018,13 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
int divider;
int ret;
- /* Exit clock stop */
- ret = cdns_clear_bit(cdns, CDNS_MCP_CONTROL,
-CDNS_MCP_CONTROL_CLK_STOP_CLR);
- if (ret < 0) {
- dev_err(cdns->dev, "Couldn't exit from clock stop\n");
- return ret;
+ if (clock_stop_exit) {
+ ret = cdns_clear_bit(cdns, CDNS_MCP_CONTROL,
+CDNS_MCP_CONTROL_CLK_STOP_CLR);
+ if (ret < 0) {
+ dev_err(cdns->dev, "Couldn't exit from clock stop\n");
+ return ret;
+ }
}
/* Set clock divider */
diff --git a/drivers/soundwire/cadence_master.h
b/drivers/soundwire/cadence_master.h
index 1a0ba36dd78f..091b771b570d 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -158,7 +158,7 @@ extern struct sdw_master_ops sdw_cdns_master_ops;
irqreturn_t sdw_cdns_irq(int irq, void *dev_id);
irqreturn_t sdw_cdns_thread(int irq, void *dev_id);
-int sdw_cdns_init(struct sdw_cdns *cdns);
+int sdw_cdns_init(struct sdw_cdns *cdns, bool clock_stop_exit);
int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
struct sdw_cdns_stream_config config);
int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 1192d5775484..db7bf2912767 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1043,7 +1043,7 @@ static int intel_init(struct sdw_intel *sdw)
intel_link_power_up(sdw);
intel_shim_init(sdw);
- return sdw_cdns_init(>cdns);
+ return sdw_cdns_init(>cdns, false);
}
/*
--
2.20.1
[RFC PATCH 39/40] soundwire: intel: add pm_runtime support
Add basic hooks in DAI .startup and .shutdown callbacks. The SoundWire
IP should be powered between those two calls.
By default the platform_device is in SUSPENDED mode, it is required to
call pm_runtime_set_active() before _enable()
FIXME: do we need to use mark_last_busy()?
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 38 ++
1 file changed, 38 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index db7bf2912767..1394a2322553 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -707,6 +707,23 @@ static void intel_port_cleanup(struct sdw_cdns_dma_data
*dma)
}
}
+static int intel_startup(struct snd_pcm_substream *substream,
+struct snd_soc_dai *dai)
+{
+ struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
+ int ret;
+
+ ret = pm_runtime_get_sync(cdns->dev);
+ if (ret < 0) {
+ dev_err_ratelimited(cdns->dev,
+ "pm_runtime_get_sync failed in %s, ret
%d\n",
+ __func__, ret);
+ pm_runtime_put_noidle(cdns->dev);
+ }
+
+ return ret;
+}
+
static int intel_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
@@ -829,6 +846,8 @@ void intel_shutdown(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct sdw_cdns_dma_data *dma;
+ struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
+ int ret;
dma = snd_soc_dai_get_dma_data(dai, substream);
if (!dma)
@@ -836,6 +855,13 @@ void intel_shutdown(struct snd_pcm_substream *substream,
snd_soc_dai_set_dma_data(dai, substream, NULL);
kfree(dma);
+
+ pm_runtime_mark_last_busy(cdns->dev);
+ ret = pm_runtime_put_autosuspend(cdns->dev);
+ if (ret < 0)
+ dev_err_ratelimited(cdns->dev,
+ "pM_runtime_put_autosuspend failed in %s:,
ret %d\n",
+ __func__, ret);
}
static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai,
@@ -851,6 +877,7 @@ static int intel_pdm_set_sdw_stream(struct snd_soc_dai *dai,
}
static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
+ .startup = intel_startup,
.hw_params = intel_hw_params,
.hw_free = intel_hw_free,
.shutdown = intel_shutdown,
@@ -1124,6 +1151,15 @@ static int intel_probe(struct platform_device *pdev)
intel_debugfs_init(sdw);
+ /* Enable PM */
+ pm_runtime_set_autosuspend_delay(>dev, 3000);
+ pm_runtime_use_autosuspend(>dev);
+
+ pm_runtime_mark_last_busy(>dev); /* FIXME: needed? */
+
+ pm_runtime_set_active(>dev);
+ pm_runtime_enable(>dev);
+
return 0;
err_dai:
@@ -1141,6 +1177,7 @@ static int intel_remove(struct platform_device *pdev)
sdw = platform_get_drvdata(pdev);
if (!sdw->cdns.bus.prop.hw_disabled) {
+ pm_runtime_disable(>dev);
intel_debugfs_exit(sdw);
free_irq(sdw->res->irq, sdw);
snd_soc_unregister_component(sdw->cdns.dev);
@@ -1212,6 +1249,7 @@ static int intel_resume(struct device *dev)
static const struct dev_pm_ops intel_pm = {
SET_SYSTEM_SLEEP_PM_OPS(intel_suspend, intel_resume)
+ SET_RUNTIME_PM_OPS(intel_suspend, intel_resume, NULL)
};
static struct platform_driver sdw_intel_drv = {
--
2.20.1
[RFC PATCH 00/40] soundwire: updates for 5.4
The existing upstream code allows for SoundWire devices to be enumerated and managed by the bus, but streaming is not currently supported. Bard Liao, Rander Wang and I did quite a bit of integration/validation work to close this gap and we now have SoundWire streaming + basic power managemement on Intel CometLake and IceLake reference boards. These changes are still preliminary and should not be merged as is, but it's time to start reviews. While the number of patches is quite large, each of the changes is quite small. SOF driver changes will be submitted shortly as well but are still being validated. ClockStop modes and synchronized playback on multiple links are not supported for now and will likely be part of the next cycle (dependencies on codec drivers and multi-cpu DAI support). Acknowledgements: This work would not have been possible without the support of Slawomir Blauciak and Tomasz Lauda on the SOF side, currently being reviewed, see https://github.com/thesofproject/sof/pull/1638 Comments and feedback welcome! Bard liao (1): soundwire: include mod_devicetable.h to avoid compiling warnings Pierre-Louis Bossart (38): soundwire: add debugfs support soundwire: cadence_master: add debugfs register dump soundwire: cadence_master: align debugfs to 8 digits soundwire: intel: add debugfs register dump soundwire: intel: move interrupt enable after interrupt handler registration soundwire: intel: prevent possible dereference in hw_params soundwire: intel: fix channel number reported by hardware soundwire: intel: remove BIOS work-arounds soundwire: cadence_master: fix usage of CONFIG_UPDATE soundwire: cadence_master: remove useless wrapper soundwire: cadence_master: simplify bus clash interrupt clear soundwire: cadence_master: revisit interrupt settings soundwire: cadence_master: fix register definition for SLAVE_STATE soundwire: cadence_master: fix definitions for INTSTAT0/1 soundwire: cadence_master: handle multiple status reports per Slave soundwire: cadence_master: improve startup sequence with link hw_reset soundwire: bus: use runtime_pm_get_sync/pm when enabled soundwire: bus: split handling of Device0 events soundwire: bus: improve dynamic debug comments for enumeration soundwire: prototypes for suspend/resume soundwire: export helpers to find row and column values soundwire: stream: fix disable sequence soundwire: cadence_master: use BIOS defaults for frame shape soundwire: intel: use BIOS information to set clock dividers soundwire: Add Intel resource management algorithm soundwire: intel: handle disabled links soundwire: intel_init: add kernel module parameter to filter out links soundwire: cadence_master: add kernel parameter to override interrupt mask soundwire: intel: move shutdown() callback and don't export symbol soundwire: intel: add helper for initialization soundwire: intel: Add basic power management support soundwire: intel: ignore disabled links for suspend/resume soundwire: intel: export helper to exit reset soundwire: intel: disable interrupts on suspend soundwire: cadence_master: add hw_reset capability in debugfs soundwire: cadence_master: make clock stop exit configurable on init soundwire: intel: add pm_runtime support soundwire: intel: add delay on restart for enumeration Rander Wang (1): soundwire: cadence_master: fix divider setting in clock register drivers/soundwire/Makefile | 4 +- drivers/soundwire/algo_dynamic_allocation.c | 403 drivers/soundwire/bus.c | 44 ++- drivers/soundwire/bus.h | 77 +++- drivers/soundwire/bus_type.c| 3 + drivers/soundwire/cadence_master.c | 365 ++ drivers/soundwire/cadence_master.h | 12 +- drivers/soundwire/debugfs.c | 156 drivers/soundwire/intel.c | 381 +- drivers/soundwire/intel_init.c | 14 + drivers/soundwire/slave.c | 1 + drivers/soundwire/stream.c | 53 ++- include/linux/soundwire/sdw.h | 15 + 13 files changed, 1414 insertions(+), 114 deletions(-) create mode 100644 drivers/soundwire/algo_dynamic_allocation.c create mode 100644 drivers/soundwire/debugfs.c -- 2.20.1
[RFC PATCH 04/40] soundwire: intel: add debugfs register dump
Add debugfs file to dump the Intel SoundWire registers
Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah. The main change
is the use of scnprintf to avoid known issues with snprintf.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 115 ++
1 file changed, 115 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 317873bc0555..aeadc341c0a3 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -6,6 +6,7 @@
*/
#include
+#include
#include
#include
#include
@@ -16,6 +17,7 @@
#include
#include
#include "cadence_master.h"
+#include "bus.h"
#include "intel.h"
/* Intel SHIM Registers Definition */
@@ -98,6 +100,7 @@ struct sdw_intel {
struct sdw_cdns cdns;
int instance;
struct sdw_intel_link_res *res;
+ struct dentry *fs;
};
#define cdns_to_intel(_cdns) container_of(_cdns, struct sdw_intel, cdns)
@@ -161,6 +164,115 @@ static int intel_set_bit(void __iomem *base, int offset,
u32 value, u32 mask)
return -EAGAIN;
}
+/*
+ * debugfs
+ */
+
+#define RD_BUF (2 * PAGE_SIZE)
+
+static ssize_t intel_sprintf(void __iomem *mem, bool l,
+char *buf, size_t pos, unsigned int reg)
+{
+ int value;
+
+ if (l)
+ value = intel_readl(mem, reg);
+ else
+ value = intel_readw(mem, reg);
+
+ return scnprintf(buf + pos, RD_BUF - pos, "%4x\t%4x\n", reg, value);
+}
+
+static ssize_t intel_reg_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct sdw_intel *sdw = file->private_data;
+ void __iomem *s = sdw->res->shim;
+ void __iomem *a = sdw->res->alh;
+ char *buf;
+ ssize_t ret;
+ int i, j;
+ unsigned int links, reg;
+
+ buf = kzalloc(RD_BUF, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ links = intel_readl(s, SDW_SHIM_LCAP) & GENMASK(2, 0);
+
+ ret = scnprintf(buf, RD_BUF, "Register Value\n");
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nShim\n");
+
+ for (i = 0; i < 4; i++) {
+ reg = SDW_SHIM_LCAP + i * 4;
+ ret += intel_sprintf(s, true, buf, ret, reg);
+ }
+
+ for (i = 0; i < links; i++) {
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nLink%d\n", i);
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLSCAP(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS0CM(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS1CM(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS2CM(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTLS3CM(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_PCMSCAP(i));
+
+ for (j = 0; j < 8; j++) {
+ ret += intel_sprintf(s, false, buf, ret,
+ SDW_SHIM_PCMSYCHM(i, j));
+ ret += intel_sprintf(s, false, buf, ret,
+ SDW_SHIM_PCMSYCHC(i, j));
+ }
+
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_PDMSCAP(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_IOCTL(i));
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_CTMCTL(i));
+ }
+
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_WAKEEN);
+ ret += intel_sprintf(s, false, buf, ret, SDW_SHIM_WAKESTS);
+
+ ret += scnprintf(buf + ret, RD_BUF - ret, "\nALH\n");
+ for (i = 0; i < 8; i++)
+ ret += intel_sprintf(a, true, buf, ret, SDW_ALH_STRMZCFG(i));
+
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+ kfree(buf);
+
+ return ret;
+}
+
+static const struct file_operations intel_reg_fops = {
+ .open = simple_open,
+ .read = intel_reg_read,
+ .llseek = default_llseek,
+};
+
+static void intel_debugfs_init(struct sdw_intel *sdw)
+{
+ struct dentry *root = sdw->cdns.bus.debugfs;
+
+ if (!root)
+ return;
+
+ sdw->fs = debugfs_create_dir("intel-sdw", root);
+ if (IS_ERR_OR_NULL(sdw->fs)) {
+ dev_err(sdw->cdns.dev, "debugfs root creation failed\n");
+ sdw->fs = NULL;
+ return;
+ }
+
+ debugfs_create_file("intel-registers", 0400, sdw->fs, sdw,
+ _reg_fops);
+
+ sdw_cdns_debugfs_init(>cdns, sdw->fs);
+}
+
+static void intel_debugfs_exit(struct sdw_intel *sdw)
+{
+ debugfs_remove_recursive(sdw->fs);
+}
+
/*
* shim ops
*/
@@ -896,6 +
[RFC PATCH 14/40] soundwire: cadence_master: fix definitions for INTSTAT0/1
Two off-by-one errors: INTSTAT0 missed BIT(31) and INTSTAT1 is only defined on first 16 bits. Signed-off-by: Pierre-Louis Bossart --- drivers/soundwire/cadence_master.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index d9d9e3d964dd..889fa2cd49ae 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -100,8 +100,8 @@ #define CDNS_MCP_SLAVE_INTMASK00x5C #define CDNS_MCP_SLAVE_INTMASK10x60 -#define CDNS_MCP_SLAVE_INTMASK0_MASK GENMASK(30, 0) -#define CDNS_MCP_SLAVE_INTMASK1_MASK GENMASK(16, 0) +#define CDNS_MCP_SLAVE_INTMASK0_MASK GENMASK(31, 0) +#define CDNS_MCP_SLAVE_INTMASK1_MASK GENMASK(15, 0) #define CDNS_MCP_PORT_INTSTAT 0x64 #define CDNS_MCP_PDI_STAT 0x6C -- 2.20.1
[RFC PATCH 05/40] soundwire: intel: move interrupt enable after interrupt handler registration
Not sure why the existing code would enable interrupts without the
ability to deal with them.
Signed-off-by: Pierre-Louis Bossart
---
drivers/soundwire/intel.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index aeadc341c0a3..68832e613b1e 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -981,8 +981,6 @@ static int intel_probe(struct platform_device *pdev)
if (ret)
goto err_init;
- ret = sdw_cdns_enable_interrupt(>cdns);
-
/* Read the PDI config and initialize cadence PDI */
intel_pdi_init(sdw, );
ret = sdw_cdns_pdi_init(>cdns, config);
@@ -1000,6 +998,8 @@ static int intel_probe(struct platform_device *pdev)
goto err_init;
}
+ ret = sdw_cdns_enable_interrupt(>cdns);
+
/* Register DAIs */
ret = intel_register_dai(sdw);
if (ret) {
--
2.20.1
Re: [PATCH] ASoC: Intel: Fix some acpi vs apci typo in somme comments
On 7/25/19 12:35 AM, Christophe JAILLET wrote: Fix some typo to have the filaname given in a comment match the real name of the file. Some 'acpi' have erroneously been written 'apci' Signed-off-by: Christophe JAILLET yes, thanks for the corrections. Acked-by: Pierre-Louis Bossart --- sound/soc/intel/common/soc-acpi-intel-bxt-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-byt-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-cht-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-cnl-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-glk-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-hda-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-icl-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-kbl-match.c | 2 +- sound/soc/intel/common/soc-acpi-intel-skl-match.c | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/sound/soc/intel/common/soc-acpi-intel-bxt-match.c b/sound/soc/intel/common/soc-acpi-intel-bxt-match.c index 229e39586868..4a5adae1d785 100644 --- a/sound/soc/intel/common/soc-acpi-intel-bxt-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-bxt-match.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * soc-apci-intel-bxt-match.c - tables and support for BXT ACPI enumeration. + * soc-acpi-intel-bxt-match.c - tables and support for BXT ACPI enumeration. * * Copyright (c) 2018, Intel Corporation. * diff --git a/sound/soc/intel/common/soc-acpi-intel-byt-match.c b/sound/soc/intel/common/soc-acpi-intel-byt-match.c index b94b482ac34f..1cc801ba92eb 100644 --- a/sound/soc/intel/common/soc-acpi-intel-byt-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-byt-match.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * soc-apci-intel-byt-match.c - tables and support for BYT ACPI enumeration. + * soc-acpi-intel-byt-match.c - tables and support for BYT ACPI enumeration. * * Copyright (c) 2017, Intel Corporation. */ diff --git a/sound/soc/intel/common/soc-acpi-intel-cht-match.c b/sound/soc/intel/common/soc-acpi-intel-cht-match.c index b7f11f6be1cf..d0fb43c2b9f6 100644 --- a/sound/soc/intel/common/soc-acpi-intel-cht-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-cht-match.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * soc-apci-intel-cht-match.c - tables and support for CHT ACPI enumeration. + * soc-acpi-intel-cht-match.c - tables and support for CHT ACPI enumeration. * * Copyright (c) 2017, Intel Corporation. */ diff --git a/sound/soc/intel/common/soc-acpi-intel-cnl-match.c b/sound/soc/intel/common/soc-acpi-intel-cnl-match.c index c36c0aa4f683..771b0ef21051 100644 --- a/sound/soc/intel/common/soc-acpi-intel-cnl-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-cnl-match.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * soc-apci-intel-cnl-match.c - tables and support for CNL ACPI enumeration. + * soc-acpi-intel-cnl-match.c - tables and support for CNL ACPI enumeration. * * Copyright (c) 2018, Intel Corporation. * diff --git a/sound/soc/intel/common/soc-acpi-intel-glk-match.c b/sound/soc/intel/common/soc-acpi-intel-glk-match.c index 616eb09e78a0..60dea358fa04 100644 --- a/sound/soc/intel/common/soc-acpi-intel-glk-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-glk-match.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * soc-apci-intel-glk-match.c - tables and support for GLK ACPI enumeration. + * soc-acpi-intel-glk-match.c - tables and support for GLK ACPI enumeration. * * Copyright (c) 2018, Intel Corporation. * diff --git a/sound/soc/intel/common/soc-acpi-intel-hda-match.c b/sound/soc/intel/common/soc-acpi-intel-hda-match.c index 68ae43f7b4b2..cc972d2ac691 100644 --- a/sound/soc/intel/common/soc-acpi-intel-hda-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-hda-match.c @@ -2,7 +2,7 @@ // Copyright (c) 2018, Intel Corporation. /* - * soc-apci-intel-hda-match.c - tables and support for HDA+ACPI enumeration. + * soc-acpi-intel-hda-match.c - tables and support for HDA+ACPI enumeration. * */ diff --git a/sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c b/sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c index d27853e7a369..34eb0baaa951 100644 --- a/sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c +++ b/sound/soc/intel/common/soc-acpi-intel-hsw-bdw-match.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * soc-apci-intel-hsw-bdw-match.c - tables and support for ACPI enumeration. + * soc-acpi-intel-hsw-bdw-match.c - tables and support for ACPI enumeration. * * Copyright (c) 2017, Intel Corporation. */ diff --git a/sound/soc/intel/common/soc-acpi-intel-icl-match.c b/sound/soc/intel/common/soc-acpi-intel-icl-match.c index 0b430b9b3673..38977669b576 100644 --- a/sound/soc/intel/common/soc-acpi-intel-icl-match.c +++ b/sound/soc/intel/common/soc-acpi-intel
Re: [Sound-open-firmware] [PATCH v2 1/5] ASoC: SOF: imx: Add i.MX8 HW support
diff --git a/sound/soc/sof/imx/Makefile b/sound/soc/sof/imx/Makefile
new file mode 100644
index ..c69237971da5
--- /dev/null
+++ b/sound/soc/sof/imx/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+
+ccflags-y += -DDEBUG
this should be removed or in a separate patch.
+struct imx8_priv {
+ struct device *dev;
+ struct snd_sof_dev *sdev;
+ struct imx_dsp_ipc *dsp_ipc;
+ struct imx_sc_ipc *sc_ipc;
maybe a comment to explain what 'sc' stands for?
+};
+
+static void imx8_get_windows(struct snd_sof_dev *sdev)
+{
+ struct sof_ipc_window_elem *elem;
+ u32 outbox_offset = 0;
+ u32 stream_offset = 0;
+ u32 inbox_offset = 0;
+ u32 outbox_size = 0;
+ u32 stream_size = 0;
+ u32 inbox_size = 0;
+ int i;
+
+ if (!sdev->info_window) {
+ dev_err(sdev->dev, "error: have no window info\n");
+ return;
+ }
+
+ for (i = 0; i < sdev->info_window->num_windows; i++) {
+ elem = >info_window->window[i];
+
+ switch (elem->type) {
+ case SOF_IPC_REGION_UPBOX:
+ inbox_offset = elem->offset + MBOX_OFFSET;
+ inbox_size = elem->size;
+ snd_sof_debugfs_io_item(sdev,
+ sdev->bar[SOF_FW_BLK_TYPE_SRAM]
+ + inbox_offset,
+ elem->size, "inbox",
+ SOF_DEBUGFS_ACCESS_D0_ONLY);
+ break;
+ case SOF_IPC_REGION_DOWNBOX:
+ outbox_offset = elem->offset + MBOX_OFFSET;
+ outbox_size = elem->size;
+ snd_sof_debugfs_io_item(sdev,
+ sdev->bar[SOF_FW_BLK_TYPE_SRAM]
+ + outbox_offset,
+ elem->size, "outbox",
+ SOF_DEBUGFS_ACCESS_D0_ONLY);
+ break;
+ case SOF_IPC_REGION_TRACE:
+ snd_sof_debugfs_io_item(sdev,
+ sdev->bar[SOF_FW_BLK_TYPE_SRAM]
+ + elem->offset + MBOX_OFFSET,
+ elem->size, "etrace",
+ SOF_DEBUGFS_ACCESS_D0_ONLY);
+ break;
+ case SOF_IPC_REGION_DEBUG:
+ snd_sof_debugfs_io_item(sdev,
+ sdev->bar[SOF_FW_BLK_TYPE_SRAM]
+ + elem->offset + MBOX_OFFSET,
+ elem->size, "debug",
+ SOF_DEBUGFS_ACCESS_D0_ONLY);
+ break;
+ case SOF_IPC_REGION_STREAM:
+ stream_offset = elem->offset + MBOX_OFFSET;
+ stream_size = elem->size;
+ snd_sof_debugfs_io_item(sdev,
+ sdev->bar[SOF_FW_BLK_TYPE_SRAM]
+ + stream_offset,
+ elem->size, "stream",
+ SOF_DEBUGFS_ACCESS_D0_ONLY);
+ break;
+ case SOF_IPC_REGION_REGS:
+ snd_sof_debugfs_io_item(sdev,
+ sdev->bar[SOF_FW_BLK_TYPE_SRAM]
+ + elem->offset + MBOX_OFFSET,
+ elem->size, "regs",
+ SOF_DEBUGFS_ACCESS_D0_ONLY);
+ break;
+ case SOF_IPC_REGION_EXCEPTION:
+ sdev->dsp_oops_offset = elem->offset + MBOX_OFFSET;
+ snd_sof_debugfs_io_item(sdev,
+ sdev->bar[SOF_FW_BLK_TYPE_SRAM]
+ + elem->offset + MBOX_OFFSET,
+ elem->size, "exception",
+ SOF_DEBUGFS_ACCESS_D0_ONLY);
+ break;
+ default:
+ dev_err(sdev->dev, "error: get illegal window info\n");
+ return;
+ }
+ }
+
+ if (outbox_size == 0 || inbox_size == 0) {
+ dev_err(sdev->dev, "error: get illegal mailbox window\n");
+ return;
+ }
+
+ snd_sof_dsp_mailbox_init(sdev, inbox_offset, inbox_size,
+outbox_offset, outbox_size);
Re: [Sound-open-firmware] [PATCH v2 3/5] ASoC: SOF: Add DT DSP device support
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 61b97fc55bb2..2aa3a1cdf60c 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -36,6 +36,15 @@ config SND_SOC_SOF_ACPI
Say Y if you need this option
If unsure select "N".
+config SND_SOC_SOF_DT
+ tristate "SOF DT enumeration support"
+ select SND_SOC_SOF
+ select SND_SOC_SOF_OPTIONS
+ help
+ This adds support for Device Tree enumeration. This option is
+ required to enable i.MX8 devices.
+ Say Y if you need this option. If unsure select "N".
+
[snip]
diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
index fff64a9970f0..fa35994a79c4 100644
--- a/sound/soc/sof/imx/Kconfig
+++ b/sound/soc/sof/imx/Kconfig
@@ -12,6 +12,7 @@ if SND_SOC_SOF_IMX_TOPLEVEL
config SND_SOC_SOF_IMX8
tristate "SOF support for i.MX8"
+ select SND_SOC_SOF_DT
This looks upside down. You should select SOF_DT first then include the
NXP stuff.
help
This adds support for Sound Open Firmware for NXP i.MX8 platforms
Say Y if you have such a device.
diff --git a/sound/soc/sof/sof-dt-dev.c b/sound/soc/sof/sof-dt-dev.c
new file mode 100644
index ..31429bbb5c7e
--- /dev/null
+++ b/sound/soc/sof/sof-dt-dev.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+//
+// Copyright 2019 NXP
+//
+// Author: Daniel Baluta
+//
+
+#include
+#include
+#include
+#include
+
+#include "ops.h"
+
+extern struct snd_sof_dsp_ops sof_imx8_ops;
+
+static char *fw_path;
+module_param(fw_path, charp, 0444);
+MODULE_PARM_DESC(fw_path, "alternate path for SOF firmware.");
+
+static char *tplg_path;
+module_param(tplg_path, charp, 0444);
+MODULE_PARM_DESC(tplg_path, "alternate path for SOF topology.");
+
+/* platform specific devices */
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_IMX8)
+static struct sof_dev_desc sof_dt_imx8qxp_desc = {
+ .default_fw_path = "imx/sof",
+ .default_tplg_path = "imx/sof-tplg",
+ .nocodec_fw_filename = "sof-imx8.ri",
+ .nocodec_tplg_filename = "sof-imx8-nocodec.tplg",
+ .ops = _imx8_ops,
+};
+#endif
+
+static const struct dev_pm_ops sof_dt_pm = {
+ SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume)
+ SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
+ NULL)
+};
+
+static void sof_dt_probe_complete(struct device *dev)
+{
+ /* allow runtime_pm */
+ pm_runtime_set_autosuspend_delay(dev, SND_SOF_SUSPEND_DELAY_MS);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_enable(dev);
+}
+
+static int sof_dt_probe(struct platform_device *pdev)
+{
+ struct device *dev = >dev;
+ const struct sof_dev_desc *desc;
+ /*TODO: create a generic snd_soc_xxx_mach */
+ struct snd_soc_acpi_mach *mach;
I wonder if you really need to use the same structures. For Intel we get
absolutely zero info from the firmware so rely on an ACPI codec ID as a
key to find information on which firmware and topology to use, and which
machine driver to load. You could have all this information in a DT blob?
+ struct snd_sof_pdata *sof_pdata;
+ const struct snd_sof_dsp_ops *ops;
+ int ret;
+
+ dev_info(>dev, "DT DSP detected");
+
+ sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL);
+ if (!sof_pdata)
+ return -ENOMEM;
+
+ desc = device_get_match_data(dev);
+ if (!desc)
+ return -ENODEV;
+
+ /* get ops for platform */
+ ops = desc->ops;
+ if (!ops) {
+ dev_err(dev, "error: no matching DT descriptor ops\n");
+ return -ENODEV;
+ }
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_FORCE_NOCODEC_MODE)
+ /* force nocodec mode */
+ dev_warn(dev, "Force to use nocodec mode\n");
+ mach = devm_kzalloc(dev, sizeof(*mach), GFP_KERNEL);
+ if (!mach)
+ return -ENOMEM;
+ ret = sof_nocodec_setup(dev, sof_pdata, mach, desc, ops);
+ if (ret < 0)
+ return ret;
+#else
+ /* TODO: implement case where we actually have a codec */
+ return -ENODEV;
+#endif
+
+ if (mach)
+ mach->mach_params.platform = dev_name(dev);
+
+ sof_pdata->machine = mach;
+ sof_pdata->desc = desc;
+ sof_pdata->dev = >dev;
+ sof_pdata->platform = dev_name(dev);
+
+ /* alternate fw and tplg filenames */
+ if (fw_path)
+ sof_pdata->fw_filename_prefix = fw_path;
+ else
+ sof_pdata->fw_filename_prefix =
+ sof_pdata->desc->default_fw_path;
+ if (tplg_path)
+ sof_pdata->tplg_filename_prefix = tplg_path;
+ else
+ sof_pdata->tplg_filename_prefix =
+ sof_pdata->desc->default_tplg_path;
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE)
+ /* set callback to enable
Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers
On 7/22/19 8:39 AM, Arnd Bergmann wrote:
On Sun, Jul 21, 2019 at 4:25 PM Masahiro Yamada
wrote:
struct snd_sof_blk_hdr {
enum snd_sof_fw_blk_type type;
- uint32_t size; /* bytes minus this header */
- uint32_t offset;/* offset from base */
+ __u32 size; /* bytes minus this header */
+ __u32 offset; /* offset from base */
} __packed;
On a related note: Using an 'enum' in an ABI structure is not portable
across architectures. This is probably fine in a UAPI as long as user
and kernel space agree on the size of an enum, but if the same
structure is used to talk to the firmware, it won't work on architectures
that have a different size for the first field.
yes, we've removed all enums in SOF and missed this one. This should be
changed, thanks for the note.
Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers
On 7/22/19 8:34 AM, Arnd Bergmann wrote:
On Mon, Jul 22, 2019 at 3:16 PM Pierre-Louis Bossart
wrote:
On 7/22/19 7:56 AM, Takashi Iwai wrote:
On Mon, 22 Jul 2019 14:49:34 +0200,
Pierre-Louis Bossart wrote:
On 7/21/19 9:23 AM, Masahiro Yamada wrote:
When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to
make sure they can be included from user-space.
Currently, header.h and fw.h are excluded from the test coverage.
To make them join the compile-test, we need to fix the build errors
attached below.
For a case like this, we decided to use __u{8,16,32,64} variable types
in this discussion:
https://lkml.org/lkml/2019/6/5/18
these files are shared with the SOF project and used as is (with minor
formatting) for the firmware compilation. I am not sure I understand
the ask here, are you really asking SOF to use linux-specific type
definitions?
Actually this is linux-kernel UAPI header files, so yes, we should
follow the convention there as much as possible.
So far we haven't been strict about these types. But now we have a
unit test for checking it, so it's a good opportunity to address the
issues.
Maybe a bit of background. For SOF we split the includes in 4 directories
https://github.com/thesofproject/sof/tree/master/src/include
- sof: internal includes for firmware only
- ipc: definitions of the structures for information exchanged over the
IPC channel. This directory is used as is by the Linux kernel and
mirrored in include/sound/sof
- user: definitions needed for firmware tools, e.g. to generate the
image or parse the trace. this directory is not used by the Linux kernel.
- kernel: definitions for the firmware format, needed for the loader to
parse the firmware files. This is not directly used by applications
running on the target, it really defines the content passed to the
kernel with request_firmware. This directory is mirrored in the Linux
include/uapi/sound/sof directory.
Our goal is to minimize the differences and allow deltas e.g. for
license or comments. We could add a definition for __u32 when linux is
not used, I am just not sure if these two files really fall in the UAPI
category and if the checks make sense.
If you can build all the SOF user space without these headers being
installed to /usr/include/sound/sof/, you can move them from
include/uapi/sound/sof to include/sounds/sof and leave the types
unchanged.
yes we don't need those files to build userspace stuff. The idea was
that these format definitions establish a contract between userspace
(more specifically the files stored in /lib/firmware) and the kernel.
IIRC we wanted to make sure that any changes would be tracked as
breaking userspace. If the consensus is that the uapi directory is
strictly used for builds then we should indeed move those files
Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers
On 7/22/19 7:56 AM, Takashi Iwai wrote:
On Mon, 22 Jul 2019 14:49:34 +0200,
Pierre-Louis Bossart wrote:
On 7/21/19 9:23 AM, Masahiro Yamada wrote:
When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to
make sure they can be included from user-space.
Currently, header.h and fw.h are excluded from the test coverage.
To make them join the compile-test, we need to fix the build errors
attached below.
For a case like this, we decided to use __u{8,16,32,64} variable types
in this discussion:
https://lkml.org/lkml/2019/6/5/18
these files are shared with the SOF project and used as is (with minor
formatting) for the firmware compilation. I am not sure I understand
the ask here, are you really asking SOF to use linux-specific type
definitions?
Actually this is linux-kernel UAPI header files, so yes, we should
follow the convention there as much as possible.
So far we haven't been strict about these types. But now we have a
unit test for checking it, so it's a good opportunity to address the
issues.
Maybe a bit of background. For SOF we split the includes in 4 directories
https://github.com/thesofproject/sof/tree/master/src/include
- sof: internal includes for firmware only
- ipc: definitions of the structures for information exchanged over the
IPC channel. This directory is used as is by the Linux kernel and
mirrored in include/sound/sof
- user: definitions needed for firmware tools, e.g. to generate the
image or parse the trace. this directory is not used by the Linux kernel.
- kernel: definitions for the firmware format, needed for the loader to
parse the firmware files. This is not directly used by applications
running on the target, it really defines the content passed to the
kernel with request_firmware. This directory is mirrored in the Linux
include/uapi/sound/sof directory.
Our goal is to minimize the differences and allow deltas e.g. for
license or comments. We could add a definition for __u32 when linux is
not used, I am just not sure if these two files really fall in the UAPI
category and if the checks make sense.
Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers
On 7/21/19 9:23 AM, Masahiro Yamada wrote:
When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to
make sure they can be included from user-space.
Currently, header.h and fw.h are excluded from the test coverage.
To make them join the compile-test, we need to fix the build errors
attached below.
For a case like this, we decided to use __u{8,16,32,64} variable types
in this discussion:
https://lkml.org/lkml/2019/6/5/18
these files are shared with the SOF project and used as is (with minor
formatting) for the firmware compilation. I am not sure I understand the
ask here, are you really asking SOF to use linux-specific type definitions?
Build log:
CC usr/include/sound/sof/header.h.s
CC usr/include/sound/sof/fw.h.s
In file included from :32:0:
./usr/include/sound/sof/header.h:19:2: error: unknown type name ‘uint32_t’
uint32_t magic; /**< 'S', 'O', 'F', '\0' */
^~~~
./usr/include/sound/sof/header.h:20:2: error: unknown type name ‘uint32_t’
uint32_t type; /**< component specific type */
^~~~
./usr/include/sound/sof/header.h:21:2: error: unknown type name ‘uint32_t’
uint32_t size; /**< size in bytes of data excl. this struct */
^~~~
./usr/include/sound/sof/header.h:22:2: error: unknown type name ‘uint32_t’
uint32_t abi; /**< SOF ABI version */
^~~~
./usr/include/sound/sof/header.h:23:2: error: unknown type name ‘uint32_t’
uint32_t reserved[4]; /**< reserved for future use */
^~~~
./usr/include/sound/sof/header.h:24:2: error: unknown type name ‘uint32_t’
uint32_t data[0]; /**< Component data - opaque to core */
^~~~
In file included from :32:0:
./usr/include/sound/sof/fw.h:49:2: error: unknown type name ‘uint32_t’
uint32_t size; /* bytes minus this header */
^~~~
./usr/include/sound/sof/fw.h:50:2: error: unknown type name ‘uint32_t’
uint32_t offset; /* offset from base */
^~~~
./usr/include/sound/sof/fw.h:64:2: error: unknown type name ‘uint32_t’
uint32_t size; /* bytes minus this header */
^~~~
./usr/include/sound/sof/fw.h:65:2: error: unknown type name ‘uint32_t’
uint32_t num_blocks; /* number of blocks */
^~~~
./usr/include/sound/sof/fw.h:73:2: error: unknown type name ‘uint32_t’
uint32_t file_size; /* size of file minus this header */
^~~~
./usr/include/sound/sof/fw.h:74:2: error: unknown type name ‘uint32_t’
uint32_t num_modules; /* number of modules */
^~~~
./usr/include/sound/sof/fw.h:75:2: error: unknown type name ‘uint32_t’
uint32_t abi; /* version of header format */
^~~~
Signed-off-by: Masahiro Yamada
---
include/uapi/sound/sof/fw.h | 16 +---
include/uapi/sound/sof/header.h | 14 --
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/include/uapi/sound/sof/fw.h b/include/uapi/sound/sof/fw.h
index 1afca973eb09..e9f697467a86 100644
--- a/include/uapi/sound/sof/fw.h
+++ b/include/uapi/sound/sof/fw.h
@@ -13,6 +13,8 @@
#ifndef __INCLUDE_UAPI_SOF_FW_H__
#define __INCLUDE_UAPI_SOF_FW_H__
+#include
+
#define SND_SOF_FW_SIG_SIZE 4
#define SND_SOF_FW_ABI1
#define SND_SOF_FW_SIG"Reef"
@@ -46,8 +48,8 @@ enum snd_sof_fw_blk_type {
struct snd_sof_blk_hdr {
enum snd_sof_fw_blk_type type;
- uint32_t size; /* bytes minus this header */
- uint32_t offset;/* offset from base */
+ __u32 size; /* bytes minus this header */
+ __u32 offset; /* offset from base */
} __packed;
/*
@@ -61,8 +63,8 @@ enum snd_sof_fw_mod_type {
struct snd_sof_mod_hdr {
enum snd_sof_fw_mod_type type;
- uint32_t size; /* bytes minus this header */
- uint32_t num_blocks;/* number of blocks */
+ __u32 size; /* bytes minus this header */
+ __u32 num_blocks; /* number of blocks */
} __packed;
/*
@@ -70,9 +72,9 @@ struct snd_sof_mod_hdr {
*/
struct snd_sof_fw_header {
unsigned char sig[SND_SOF_FW_SIG_SIZE]; /* "Reef" */
- uint32_t file_size; /* size of file minus this header */
- uint32_t num_modules; /* number of modules */
- uint32_t abi; /* version of header format */
+ __u32 file_size;/* size of file minus this header */
+ __u32 num_modules; /* number of modules */
+ __u32 abi; /* version of header format */
} __packed;
#endif
diff --git a/include/uapi/sound/sof/header.h b/include/uapi/sound/sof/header.h
index 7868990b0d6f..5f4518e7a972 100644
--- a/include/uapi/sound/sof/header.h
+++ b/include/uapi/sound/sof/header.h
@@ -9,6 +9,8 @@
#ifndef __INCLUDE_UAPI_SOUND_SOF_USER_HEADER_H__
#define __INCLUDE_UAPI_SOUND_SOF_USER_HEADER_H__
+#include
+
/*
* Header for all non IPC ABI data.
*
@@ -16,12 +18,12 @@
* Used by any bespoke component data structures or binary blobs.
*/
struct sof_abi_hdr {
-
[PATCH] soundwire: fix regmap dependencies and align with other serial links
The existing code has a mixed select/depend usage which makes no sense. config SOUNDWIRE_BUS tristate select REGMAP_SOUNDWIRE config REGMAP_SOUNDWIRE tristate depends on SOUNDWIRE_BUS Let's remove one layer of Kconfig definitions and align with the solutions used by all other serial links. Signed-off-by: Pierre-Louis Bossart --- drivers/base/regmap/Kconfig | 2 +- drivers/soundwire/Kconfig | 7 +-- drivers/soundwire/Makefile | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig index 6ad5ef48b61e..8cd2ac650b50 100644 --- a/drivers/base/regmap/Kconfig +++ b/drivers/base/regmap/Kconfig @@ -44,7 +44,7 @@ config REGMAP_IRQ config REGMAP_SOUNDWIRE tristate - depends on SOUNDWIRE_BUS + depends on SOUNDWIRE config REGMAP_SCCB tristate diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig index 3a01cfd70fdc..f518273cfbe3 100644 --- a/drivers/soundwire/Kconfig +++ b/drivers/soundwire/Kconfig @@ -4,7 +4,7 @@ # menuconfig SOUNDWIRE - bool "SoundWire support" + tristate "SoundWire support" help SoundWire is a 2-Pin interface with data and clock line ratified by the MIPI Alliance. SoundWire is used for transporting data @@ -17,17 +17,12 @@ if SOUNDWIRE comment "SoundWire Devices" -config SOUNDWIRE_BUS - tristate - select REGMAP_SOUNDWIRE - config SOUNDWIRE_CADENCE tristate config SOUNDWIRE_INTEL tristate "Intel SoundWire Master driver" select SOUNDWIRE_CADENCE - select SOUNDWIRE_BUS depends on X86 && ACPI && SND_SOC help SoundWire Intel Master driver. diff --git a/drivers/soundwire/Makefile b/drivers/soundwire/Makefile index fd99a831b92a..45b7e5001653 100644 --- a/drivers/soundwire/Makefile +++ b/drivers/soundwire/Makefile @@ -5,7 +5,7 @@ #Bus Objs soundwire-bus-objs := bus_type.o bus.o slave.o mipi_disco.o stream.o -obj-$(CONFIG_SOUNDWIRE_BUS) += soundwire-bus.o +obj-$(CONFIG_SOUNDWIRE) += soundwire-bus.o #Cadence Objs soundwire-cadence-objs := cadence_master.o -- 2.20.1
Re: [alsa-devel] [PATCH] ASoC: Intel: Atom: read timestamp moved to period_elapsed
On 7/10/19 6:15 PM, Curtis Malainey wrote:
On Tue, Jul 9, 2019 at 4:45 AM Andy Shevchenko
wrote:
On Mon, Jul 08, 2019 at 09:01:47PM -0700, Alex Levin wrote:
sst_platform_pcm_pointer is called from both snd_pcm_period_elapsed and
from snd_pcm_ioctl. Calling read timestamp results in recalculating
pcm_delay and buffer_ptr (sst_calc_tstamp) which consumes buffers in a
faster rate than intended.
In a tested BSW system with chtrt5650, for a rate of 48000, the
measured rate was sometimes 10 times more than that.
After moving the timestamp read to period elapsed, buffer consumption is
as expected.
From code prospective it looks good. You may take mine
Reviewed-by: Andy Shevchenko
Though I'm not an expert in the area, Pierre and / or Liam should give
their blessing.
Agreed, Liam or Pierre should also give their ok since this is one of
the closed source firmware drivers.
Reviewed-by: Curtis Malainey
Humm, this first review after my Summer break isn't straightforward.
By moving the timestamp update to the period elapsed event, don't you
prevent the use of this driver in no-interrupt mode - which I understood
as the baseline for Chrome?
And I also don't get how this timestamp might lead to 10x speed issues,
this driver has been around for a number of years and that specific
error was never seen. What is different in this platform and can this be
seen e.g. on a Cyan device?
Signed-off-by: Alex Levin
---
sound/soc/intel/atom/sst-mfld-platform-pcm.c | 23 +---
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 0e8b1c5eec88..196af0b30b41 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -265,16 +265,28 @@ static void sst_period_elapsed(void *arg)
{
struct snd_pcm_substream *substream = arg;
struct sst_runtime_stream *stream;
- int status;
+ struct snd_soc_pcm_runtime *rtd;
+ int status, ret_val;
if (!substream || !substream->runtime)
return;
stream = substream->runtime->private_data;
if (!stream)
return;
+
+ rtd = substream->private_data;
+ if (!rtd)
+ return;
+
status = sst_get_stream_status(stream);
if (status != SST_PLATFORM_RUNNING)
return;
+
+ ret_val = stream->ops->stream_read_tstamp(sst->dev, >stream_info);
+ if (ret_val) {
+ dev_err(rtd->dev, "stream_read_tstamp err code = %d\n", ret_val);
+ return;
+ }
snd_pcm_period_elapsed(substream);
}
@@ -658,20 +670,15 @@ static snd_pcm_uframes_t sst_platform_pcm_pointer
(struct snd_pcm_substream *substream)
{
struct sst_runtime_stream *stream;
- int ret_val, status;
+ int status;
struct pcm_stream_info *str_info;
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
stream = substream->runtime->private_data;
status = sst_get_stream_status(stream);
if (status == SST_PLATFORM_INIT)
return 0;
+
str_info = >stream_info;
- ret_val = stream->ops->stream_read_tstamp(sst->dev, str_info);
- if (ret_val) {
- dev_err(rtd->dev, "sst: error code = %d\n", ret_val);
- return ret_val;
- }
substream->runtime->delay = str_info->pcm_delay;
return str_info->buffer_ptr;
}
--
2.22.0.410.gd8fdbe21b5-goog
--
With Best Regards,
Andy Shevchenko
___
Alsa-devel mailing list
alsa-de...@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Re: [alsa-devel] [PATCH 1/2] ASoC: soc-core: defer card registration if codec component is missing
On 6/26/19 3:36 PM, Jerome Brunet wrote:
Like cpus and platforms, defer sound card initialization if the codec
component is missing when initializing the dai_link
Signed-off-by: Jerome Brunet
---
sound/soc/soc-core.c | 8
1 file changed, 8 insertions(+)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 358f1fbf9a30..002ddbf4e5a3 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1064,12 +1064,20 @@ static int soc_init_dai_link(struct snd_soc_card *card,
link->name);
return -EINVAL;
}
+
/* Codec DAI name must be specified */
if (!codec->dai_name) {
dev_err(card->dev, "ASoC: codec_dai_name not set for
%s\n",
link->name);
return -EINVAL;
}
+
+ /*
+* Defer card registartion if codec component is not added to
registration
+* component list.
+*/
+ if (!soc_find_component(codec))
+ return -EPROBE_DEFER;
}
/*
Re: [alsa-devel] [PATCH v2 00/11] Fix driver reload issues
On 6/25/19 7:04 AM, Mark Brown wrote: On Mon, Jun 17, 2019 at 01:36:33PM +0200, Amadeusz Sławiński wrote: Hi, This series of patches introduces fixes to various issues found while trying to unload all snd* modules and then loading them again. This allows for modules to be really _modules_ and be unloaded and loaded on demand, making it easier to develop and test them without constant system reboots. Pierre? You did comment on the general concept in one of the patches but not on any of the patches directly. I did review the patches internally and the v1. For the v2 I could only do an airport lounge review and didn't see any blatant issues, so feel free to take the following tag for the series. Reviewed-by: Pierre-Louis Bossart
Re: [PATCH -next] ASoC: SOF: Intel: hda: remove duplicated include from hda.c
On 6/20/19 4:57 PM, YueHaibing wrote: Remove duplicated include. Signed-off-by: YueHaibing Acked-by: Pierre-Louis Bossart --- sound/soc/sof/intel/hda.c | 1 - 1 file changed, 1 deletion(-) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 51c1c1787de7..7f665392618f 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -19,7 +19,6 @@ #include #include -#include #include #include #include "../ops.h"
Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove
Could you please give a bit more context on what error you see when this happens? Hi, I get Oops. This is what happens with all other patches in this series and only this one reverted: root@APL:~# rmmod snd_soc_sst_bxt_rt298 root@APL:~# rmmod snd_soc_hdac_hdmi root@APL:~# rmmod snd_soc_skl Thanks, Amadeusz. I think the order in which the drivers are removed is what's causing the oops in your case. With SOF, the order we remove is 1. rmmod sof_pci_dev 2. rmmod snd_soc_sst_bxt_rt298 3. rmmod snd_soc_hdac_hdmi Well, there is nothing enforcing the order in which modules can be unloaded (and I see no reason to force it), as you can see from following excerpt, you can either start unloading from snd_soc_sst_bxt_rt298 or snd_soc_skl, and yes if you start from snd_soc_skl, there is no problem. there is a fundamental dependency that you are ignoring: the module snd_soc_sst_bxt_rt298 is a machine driver which will be probed when snd_soc_skl creates a platform_device. Sure you can remove modules in a different order, but that's a bit of an artificial/academic exercise isn't it? I am good with this patch. I just wanted to understand why we werent seeing this error with SOF. Sure, there's nothing enforcing the order in which modules are unloaded but there must be a logical order for testing purposes. Pierre, can you please comment on it. I vaguely remember discussing this with you last year. Our tests remove the modules by taking care of dependencies and it's already unveiled dozens of issues. We could add a sequence similar to Amadeusz and unbind the modules which are loaded with the creation of a platform_device (machine driver, dmic), I am just not sure how of useful this would be.
Re: [alsa-devel] [PATCH] ASoC: SOF: disallow building without CONFIG_PCI again
On 6/17/19 2:45 PM, Arnd Bergmann wrote:
Compile-testing without PCI just causes warnings:
sound/soc/sof/sof-pci-dev.c:330:13: error: 'sof_pci_remove' defined but not
used [-Werror=unused-function]
static void sof_pci_remove(struct pci_dev *pci)
^~
sound/soc/sof/sof-pci-dev.c:230:12: error: 'sof_pci_probe' defined but not used
[-Werror=unused-function]
static int sof_pci_probe(struct pci_dev *pci,
^
I tried to fix this in a way that would still allow compile
tests, but it got too ugly, so this just reverts the patch
that allowed it in the first place.
Most architectures do allow enabling PCI, so the value of the
COMPILE_TEST alternative was not very high to start with.
I think COMPILE_TEST has value in that it exposed issues in the PCI
headers, and in general COMPILE_TEST exposes code that can be
simplified/refactored. That said I don't have the time to suggest an
alternative at the moment so it's fine with me if you want to revert. If
you don't mind sharing your config it'd help me work on this when I get
a chance.
Thanks!
Fixes: e13ef82a9ab8 ("ASoC: SOF: add COMPILE_TEST for PCI options")
Signed-off-by: Arnd Bergmann
---
sound/soc/sof/Kconfig | 2 +-
sound/soc/sof/intel/hda.c | 13 ++---
sound/soc/sof/sof-pci-dev.c | 4
3 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 41f79cdcbf47..fb01f0ca6027 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -11,7 +11,7 @@ if SND_SOC_SOF_TOPLEVEL
config SND_SOC_SOF_PCI
tristate "SOF PCI enumeration support"
- depends on PCI || COMPILE_TEST
+ depends on PCI
select SND_SOC_SOF
select SND_SOC_ACPI if ACPI
select SND_SOC_SOF_OPTIONS
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 140b1424f291..51c1c1787de7 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -533,9 +533,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
* TODO: support interrupt mode selection with kernel parameter
* support msi multiple vectors
*/
-#if IS_ENABLED(CONFIG_PCI)
ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_MSI);
-#endif
if (ret < 0) {
dev_info(sdev->dev, "use legacy interrupt mode\n");
/*
@@ -547,9 +545,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
sdev->msi_enabled = 0;
} else {
dev_info(sdev->dev, "use msi interrupt mode\n");
-#if IS_ENABLED(CONFIG_PCI)
hdev->irq = pci_irq_vector(pci, 0);
-#endif
/* ipc irq number is the same of hda irq */
sdev->ipc_irq = hdev->irq;
sdev->msi_enabled = 1;
@@ -606,10 +602,8 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
free_hda_irq:
free_irq(hdev->irq, bus);
free_irq_vector:
-#if IS_ENABLED(CONFIG_PCI)
if (sdev->msi_enabled)
pci_free_irq_vectors(pci);
-#endif
free_streams:
hda_dsp_stream_free(sdev);
/* dsp_unmap: not currently used */
@@ -624,6 +618,7 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
{
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
struct hdac_bus *bus = sof_to_bus(sdev);
+ struct pci_dev *pci = to_pci_dev(sdev->dev);
const struct sof_intel_dsp_desc *chip = hda->desc;
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
@@ -652,12 +647,8 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
free_irq(sdev->ipc_irq, sdev);
free_irq(hda->irq, bus);
-#if IS_ENABLED(CONFIG_PCI)
- if (sdev->msi_enabled) {
- struct pci_dev *pci = to_pci_dev(sdev->dev);
+ if (sdev->msi_enabled)
pci_free_irq_vectors(pci);
- }
-#endif
hda_dsp_stream_free(sdev);
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c
index ab58d4f9119f..e2b19782f01a 100644
--- a/sound/soc/sof/sof-pci-dev.c
+++ b/sound/soc/sof/sof-pci-dev.c
@@ -251,11 +251,9 @@ static int sof_pci_probe(struct pci_dev *pci,
if (!sof_pdata)
return -ENOMEM;
-#if IS_ENABLED(CONFIG_PCI)
ret = pcim_enable_device(pci);
if (ret < 0)
return ret;
-#endif
ret = pci_request_regions(pci, "Audio DSP");
if (ret < 0)
@@ -388,7 +386,6 @@ static const struct pci_device_id sof_pci_ids[] = {
};
MODULE_DEVICE_TABLE(pci, sof_pci_ids);
-#if IS_ENABLED(CONFIG_PCI)
/* pci_driver definition */
static struct pci_driver snd_sof_pci_driver = {
.name = "sof-audio-pci",
@@ -400,6 +397,5 @@ static struct pci_driver snd_sof_pci_driver = {
},
};
module_pci_driver(snd_sof_pci_driver);
-#endif
MODULE_LICENSE("Dual BSD/GPL");
Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller
On 6/11/19 5:29 AM, Srinivas Kandagatla wrote:
On 10/06/2019 15:12, Pierre-Louis Bossart wrote:
+
+ if (dev_addr == SDW_BROADCAST_DEV_NUM) {
+ ctrl->fifo_status = 0;
+ ret = wait_for_completion_timeout(>sp_cmd_comp,
+ msecs_to_jiffies(TIMEOUT_MS));
This is odd. The SoundWire spec does not handle writes to a single
device or broadcast writes differently. I don't see a clear reason
why you would only timeout for a broadcast write.
There is danger of blocking here without timeout.
Right, and it's fine to add a timeout. The question is why add a
timeout *only* for a broadcast operation? It should be added for every
transaction IMO, unless you have a reason not to do so.
I did try this before, the issue is when we read/write registers from
interrupt handler, these can be deadlocked as we will be interrupt
handler waiting for another completion interrupt, which will never
happen unless we return from the first interrupt.
I don't quite get the issue. With the Intel hardware we only deal with
Master registers (some of which mirror the bus state) in the handler and
will only modify Slave registers in the thread. All changes to Slave
registers will be subject to a timeout as well as a check for no
response or NAK. Not sure what is specific about your solution that
requires a different handling of commands depending on which device
number is used. It could very well be that you've uncovered a flaw in
the bus design but I still don't see how it would be Qualcomm-specific?
Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller
+#define SWRM_COMP_HW_VERSION 0x00
Can we please use SDW_ or QCOM_SDW_ as prefix?
SWRM prefix is as per the data sheet register names, If it help am happy
to add QCOM_ prefix it.
That'd be fine. As long as there is no duplication and two
terms/prefixes used for the same thing I am happy.
+
+ val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
+ ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
+ if (ret < 0) {
+ dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
+ __func__, val, ret);
+ goto err;
+ }
+
+ if (dev_addr == SDW_BROADCAST_DEV_NUM) {
+ ctrl->fifo_status = 0;
+ ret = wait_for_completion_timeout(>sp_cmd_comp,
+ msecs_to_jiffies(TIMEOUT_MS));
This is odd. The SoundWire spec does not handle writes to a single
device or broadcast writes differently. I don't see a clear reason why
you would only timeout for a broadcast write.
There is danger of blocking here without timeout.
Right, and it's fine to add a timeout. The question is why add a timeout
*only* for a broadcast operation? It should be added for every
transaction IMO, unless you have a reason not to do so.
+
+ /* Mask soundwire interrupts */
+ ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
+ SWRM_INTERRUPT_STATUS_RMSK);
+
+ /* Configure No pings */
+ val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
If there is any sort of PREQ signaling for Slave-initiated interrupts,
disabling PINGs is likely a non-conformant implementation since the
master is required to issue a PING command within 32 frames. That's
also the only way to know if a device is attached, so additional
comments are likely required.
This is the value of Maximum number of consiecutive read/write commands
without ping command in between. I will try to collect more details and
add some comments here.
Right, so it's probably the comment that's too strict. It's not no pings
it's no pings during a sequence of up to N read/writes.
Re: [alsa-devel] [RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller
diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt new file mode 100644 index ..eb84d0f4f36f --- /dev/null +++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt @@ -0,0 +1,62 @@ +Qualcomm SoundWire Controller + +This binding describes the Qualcomm SoundWire Controller Bindings. + +Required properties: + +- compatible: Must be "qcom,soundwire-v..", + example: + "qcom,soundwire-v1.3.0" + "qcom,soundwire-v1.5.0" + "qcom,soundwire-v1.6.0" +- reg: SoundWire controller address space. +- interrupts: SoundWire controller interrupt. +- clock-names: Must contain "iface". +- clocks: Interface clocks needed for controller. +- #sound-dai-cells:Must be 1 for digital audio interfaces on the controllers. +- #address-cells: Must be 1 for SoundWire devices; +- #size-cells: Must be <0> as SoundWire addresses have no size component. +- qcom,dout-ports: Must be count of data out ports +- qcom,din-ports: Must be count of data in ports On these I think we should have specified dpn properties as specified in DisCo, but then looking at spec we do not define that for master, but bus seems to have it. Pierre do you why master does not have dpn properties in DisCo? Because there are no DP0 or DPn registers defined for Masters in the SoundWire 1.x spec. DisCo is about specifying properties for standard registers, when they are not standard vendor extensions need to come into play. +- qcom,ports-offset1: Must be frame offset1 of each data port. + Out followed by In. Used for Block size calculation. +- qcom,ports-offset2: Must be frame offset2 of each data port. + Out followed by In. Used for Block size calculation. +- qcom,ports-sinterval-low: Must be sample interval low of each data port. + Out followed by In. Used for Sample Interval calculation. These are software so do not belong here Not necessarily. They define the allocation expected on that link and I see no problem specifying those values here. It's the moral equivalent of specifying which TDM slots and the bit depth of one slot you'd use for DSP_A/B. And if you push back, then what would be your alternate proposal on where those values might be stored? This is a very specific usage of the link and it makes sense to me to have the information in firmware - exposed with properties - rather than hard-coded in a pretend bus allocation routine in the kernel. Either the allocation is fully dynamic and it's handled by the kernel or it's static and it's better to store it in firmware to deal with platform-to-platform variations.
Re: [PATCH] ASoC: Intel: sst: fix kmalloc call with wrong flags
On 6/7/19 5:19 PM, Alex Levin wrote:
When calling kmalloc with GFP_KERNEL in case CONFIG_SLOB is unset,
kmem_cache_alloc_trace is called.
In case CONFIG_TRACING is set, kmem_cache_alloc_trace will ball
slab_alloc, which will call slab_pre_alloc_hook which might_sleep_if.
The context in which it is called in this case, the
intel_sst_interrupt_mrfld, calling a sleeping kmalloc generates a BUG():
Thanks for reporting this and suggesting a fix.
I just checked and all our test configs have CONFIG_SLOB unset and
CONFIG_TRACING=y, but I don't recall having seen this. Could you share
your config so that we can see if we are missing something?
Fixes: 972b0d456e64 ("ASoC: Intel: remove GFP_ATOMIC, use GFP_KERNEL")
[ 20.250671] BUG: sleeping function called from invalid context at
mm/slab.h:422
[ 20.250683] in_atomic(): 1, irqs_disabled(): 1, pid: 1791, name:
Chrome_IOThread
[ 20.250690] CPU: 0 PID: 1791 Comm: Chrome_IOThread Tainted: GW
4.19.43 #61
[ 20.250693] Hardware name: GOOGLE Kefka, BIOS Google_Kefka.7287.337.0
03/02/2017
[ 20.250697] Call Trace:
[ 20.250704]
[ 20.250716] dump_stack+0x7e/0xc3
[ 20.250725] ___might_sleep+0x12a/0x140
[ 20.250731] kmem_cache_alloc_trace+0x53/0x1c5
[ 20.250736] ? update_cfs_rq_load_avg+0x17e/0x1aa
[ 20.250740] ? cpu_load_update+0x6c/0xc2
[ 20.250746] sst_create_ipc_msg+0x2d/0x88
[ 20.250752] intel_sst_interrupt_mrfld+0x12a/0x22c
[ 20.250758] __handle_irq_event_percpu+0x133/0x228
[ 20.250764] handle_irq_event_percpu+0x35/0x7a
[ 20.250768] handle_irq_event+0x36/0x55
[ 20.250773] handle_fasteoi_irq+0xab/0x16c
[ 20.250779] handle_irq+0xd9/0x11e
[ 20.250785] do_IRQ+0x54/0xe0
[ 20.250791] common_interrupt+0xf/0xf
[ 20.250795]
[ 20.250800] RIP: 0010:__lru_cache_add+0x4e/0xad
[ 20.250806] Code: 00 01 48 c7 c7 b8 df 01 00 65 48 03 3c 25 28 f1 00 00 48 8b 48
08 48 89 ca 48 ff ca f6 c1 01 48 0f 44 d0 f0 ff 42 34 0f b6 0f <89> ca fe c2 88
17 48 89 44 cf 08 80 fa 0f 74 0e 48 8b 08 66 85 c9
[ 20.250809] RSP: :a568810bfd98 EFLAGS: 0202 ORIG_RAX:
ffd6
[ 20.250814] RAX: d3b904eb1940 RBX: d3b904eb1940 RCX: 0004
[ 20.250817] RDX: d3b904eb1940 RSI: a10ee5c47450 RDI: a10efba1dfb8
[ 20.250821] RBP: a568810bfda8 R08: a10ef9c741c1 R09: dead0100
[ 20.250824] R10: R11: R12: a10ee8d52a40
[ 20.250827] R13: a10ee8d52000 R14: a10ee5c47450 R15: 80013ac65067
[ 20.250835] lru_cache_add_active_or_unevictable+0x4e/0xb8
[ 20.250841] handle_mm_fault+0xd98/0x10c4
[ 20.250848] __do_page_fault+0x235/0x42d
[ 20.250853] ? page_fault+0x8/0x30
[ 20.250858] do_page_fault+0x3d/0x17a
[ 20.250862] ? page_fault+0x8/0x30
[ 20.250866] page_fault+0x1e/0x30
[ 20.250872] RIP: 0033:0x7962fdea9304
[ 20.250875] Code: 0f 11 4c 17 f0 c3 48 3b 15 f1 26 31 00 0f 83 e2 00 00 00 48 39
f7 72 0f 74 12 4c 8d 0c 16 4c 39 cf 0f 82 63 01 00 00 48 89 d1 a4 c3 80 fa
08 73 12 80 fa 04 73 1e 80 fa 01 77 26 72 05 0f b6
[ 20.250879] RSP: 002b:7962f4db5468 EFLAGS: 00010206
[ 20.250883] RAX: 3c8cc9d47008 RBX: RCX: 1b48
[ 20.250886] RDX: 2b40 RSI: 3c8cc9551000 RDI: 3c8cc9d48000
[ 20.250890] RBP: 7962f4db5820 R08: R09: 3c8cc9552b48
[ 20.250893] R10: 562dd1064d30 R11: 3c8cc825b908 R12: 3c8cc966d3c0
[ 20.250896] R13: 3c8cc9e280c0 R14: R15:
Signed-off-by: Alex Levin
---
sound/soc/intel/atom/sst/sst_pvt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/atom/sst/sst_pvt.c
b/sound/soc/intel/atom/sst/sst_pvt.c
index 00a37a09dc9b..dba0ca07ebf9 100644
--- a/sound/soc/intel/atom/sst/sst_pvt.c
+++ b/sound/soc/intel/atom/sst/sst_pvt.c
@@ -166,11 +166,11 @@ int sst_create_ipc_msg(struct ipc_post **arg, bool large)
{
struct ipc_post *msg;
- msg = kzalloc(sizeof(*msg), GFP_KERNEL);
+ msg = kzalloc(sizeof(*msg), GFP_ATOMIC);
if (!msg)
return -ENOMEM;
if (large) {
- msg->mailbox_data = kzalloc(SST_MAILBOX_SIZE, GFP_KERNEL);
+ msg->mailbox_data = kzalloc(SST_MAILBOX_SIZE, GFP_ATOMIC);
if (!msg->mailbox_data) {
kfree(msg);
return -ENOMEM;
Re: [alsa-devel] next/master boot bisection: next-20190528 on sun8i-h3-libretech-all-h3-cc
On 6/7/19 2:00 PM, Mark Brown wrote:
On Fri, Jun 07, 2019 at 05:31:12PM +0100, Guillaume Tucker wrote:
On 30/05/2019 16:53, Takashi Iwai wrote:
+ mutex_lock(_mutex);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
if (component->driver->remove_order == order)
soc_remove_component(component);
}
+ mutex_unlock(_mutex);
Ranjani, which code path your patch tries to address? Maybe better to
wrap client_mutex() in the caller side like snd_soc_unbind_card()?
Is anyone looking into this issue?
It is still occurring in next-20190606, there was a bisection
today which landed on the same commit. There just hasn't been
any new bisection reports because they have been temporarily
disabled while we fix some issues on kernelci.org.
I was expecting that Ranjani or one of the other Intel people was
looking into it...
Ack. We've all been underwater this week and this wasn't addressed,
sorry about the delay. It's probably wise to revert this commit at this
point while we look for an alternate solution?
There was an initial proposal submitted on GitHub [1] (patch attached)
which implemented what Takashi suggested in his comments. This proposal
was later optimized further, it could be that the optimization was one
bridge too far.
Could you let us know if this attached patch has any negative effects on
non-Intel platforms?
Thanks!
[1]
https://github.com/thesofproject/linux/commit/9fd09dd417bc8be7a4a8bdd1621558151f8d117b
>From 9fd09dd417bc8be7a4a8bdd1621558151f8d117b Mon Sep 17 00:00:00 2001
From: Ranjani Sridharan
Date: Wed, 22 May 2019 10:52:40 -0700
Subject: [PATCH] ASoC: core: lock client_mutex while removing link components
Removing link components results in topology unloading. So,
acquire the client_mutex before removing components in
snd_soc_unbind_card(). This will prevent lockdep warning
when the dai link is removed.
Signed-off-by: Ranjani Sridharan
---
sound/soc/soc-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 2403bec2fccf3..5609398f05d80 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2839,12 +2839,14 @@ static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
snd_soc_dapm_shutdown(card);
snd_soc_flush_all_delayed_work(card);
+ mutex_lock(_mutex);
/* remove all components used by DAI links on this card */
for_each_comp_order(order) {
for_each_card_rtds(card, rtd) {
soc_remove_link_components(card, rtd, order);
}
}
+ mutex_unlock(_mutex);
soc_cleanup_card_resources(card);
if (!unregister)
Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller
+config SOUNDWIRE_QCOM
+ tristate "Qualcomm SoundWire Master driver"
+ select SOUNDWIRE_BUS
+ depends on SND_SOC
depends on SLIMBUS if you need the SlimBus link to talk to your
SoundWire Master?
Also depends on device tree since you use of_ functions?
+#define SWRM_COMP_HW_VERSION 0x00
Can we please use SDW_ or QCOM_SDW_ as prefix?
+#define SWRM_INTERRUPT_STATUS_NEW_SLAVE_AUTO_ENUM_FINISHED BIT(11)
+#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_FAILED BIT(12)
+#define SWRM_INTERRUPT_STATUS_AUTO_ENUM_TABLE_IS_FULL BIT(13)
This hints at hardware support to assign Device Numbers automagically so
will likely have impacts on the bus driver code, no?
+#define SWRM_MAX_ROW_VAL 0 /* Rows = 48 */
+#define SWRM_DEFAULT_ROWS 48
+#define SWRM_MIN_COL_VAL 0 /* Cols = 2 */
+#define SWRM_DEFAULT_COL 16
+#define SWRM_SPECIAL_CMD_ID0xF
+#define MAX_FREQ_NUM 1
+#define TIMEOUT_MS 1000
+#define QCOM_SWRM_MAX_RD_LEN 0xf
+#define DEFAULT_CLK_FREQ 960
The clocks and frame shape don't match usual expectations for PDM.
For a 9.6 MHz support, you would typically use 8 columns and 50 rows to
transport PDM with a 50x oversampling. I've never seen anyone use 48x
for PDM.
+#define SWRM_MAX_DAIS 0xF
+
+struct qcom_swrm_port_config {
+ u8 si;
+ u8 off1;
+ u8 off2;
+};
+
+struct qcom_swrm_ctrl {
+ struct sdw_bus bus;
+ struct device *dev;
+ struct regmap *regmap;
+ struct completion sp_cmd_comp;
+ struct work_struct slave_work;
+ /* read/write lock */
+ struct mutex lock;
+ /* Port alloc/free lock */
+ struct mutex port_lock;
+ struct clk *hclk;
+ int fifo_status;
+ void __iomem *base;
+ u8 wr_cmd_id;
+ u8 rd_cmd_id;
+ int irq;
+ unsigned int version;
+ int num_din_ports;
+ int num_dout_ports;
+ unsigned long dout_port_mask;
+ unsigned long din_port_mask;
+ struct qcom_swrm_port_config pconfig[SDW_MAX_PORTS];
this is not necessarily correct. the initial definitions for
SDW_MAX_PORTS was for Slave devices. There is no definitions for Masters
in the SoundWire spec, so you could use whatever constant you want for
your hardware.
+ struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
+ enum sdw_slave_status status[SDW_MAX_DEVICES];
+ u32 (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg);
+ int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
+};
+
+#define to_qcom_sdw(b) container_of(b, struct qcom_swrm_ctrl, bus)
+
+struct usecase {
+ u8 num_port;
+ u8 num_ch;
+ u32 chrate;
+};
this structure doesn't seem to be used?
+static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
+u8 dev_addr, u16 reg_addr)
+{
+ int ret = 0;
+ u8 cmd_id;
+ u32 val;
+
+ mutex_lock(>lock);
+ if (dev_addr == SDW_BROADCAST_DEV_NUM) {
+ cmd_id = SWRM_SPECIAL_CMD_ID;
+ } else {
+ if (++ctrl->wr_cmd_id == SWRM_SPECIAL_CMD_ID)
+ ctrl->wr_cmd_id = 0;
+
+ cmd_id = ctrl->wr_cmd_id;
+ }
might be worth having a helper/macro since you are doing the same thing
below.
+
+ val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
+ ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
+ if (ret < 0) {
+ dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
+ __func__, val, ret);
+ goto err;
+ }
+
+ if (dev_addr == SDW_BROADCAST_DEV_NUM) {
+ ctrl->fifo_status = 0;
+ ret = wait_for_completion_timeout(>sp_cmd_comp,
+ msecs_to_jiffies(TIMEOUT_MS));
This is odd. The SoundWire spec does not handle writes to a single
device or broadcast writes differently. I don't see a clear reason why
you would only timeout for a broadcast write.
+
+ if (!ret || ctrl->fifo_status) {
+ dev_err(ctrl->dev, "reg 0x%x write failed\n", val);
+ ret = -ENODATA;
+ goto err;
+ }
+ }
+err:
+ mutex_unlock(>lock);
+ return ret;
+}
+
+static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
+u8 dev_addr, u16 reg_addr,
+u32 len, u8 *rval)
+{
+ int i, ret = 0;
+ u8 cmd_id = 0;
+ u32 val;
+
+ mutex_lock(>lock);
+ if (dev_addr == SDW_ENUM_DEV_NUM) {
+ cmd_id = SWRM_SPECIAL_CMD_ID;
+ } else {
+ if (++ctrl->rd_cmd_id == SWRM_SPECIAL_CMD_ID)
+ ctrl->rd_cmd_id = 0;
+
+ cmd_id = ctrl->rd_cmd_id;
+ }
helper?
+
Re: [alsa-devel] [RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller
On 6/7/19 3:56 AM, Srinivas Kandagatla wrote:
This patch adds bindings for Qualcomm soundwire controller.
Qualcomm SoundWire Master controller is present in most Qualcomm SoCs
either integrated as part of WCD audio codecs via slimbus or
as part of SOC I/O.
Signed-off-by: Srinivas Kandagatla
---
.../bindings/soundwire/qcom,swr.txt | 62 +++
1 file changed, 62 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soundwire/qcom,swr.txt
you seem to use the 'swr' prefix in this patch. Most implementers use
'sdw', and that's the default also used in the MIPI DisCo spec for
properties. Can we align on the same naming conventions?
diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
new file mode 100644
index ..eb84d0f4f36f
--- /dev/null
+++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
@@ -0,0 +1,62 @@
+Qualcomm SoundWire Controller
+
+This binding describes the Qualcomm SoundWire Controller Bindings.
+
+Required properties:
+
+- compatible: Must be "qcom,soundwire-v..",
+ example:
+ "qcom,soundwire-v1.3.0"
+ "qcom,soundwire-v1.5.0"
+ "qcom,soundwire-v1.6.0"
+- reg: SoundWire controller address space.
+- interrupts: SoundWire controller interrupt.
+- clock-names: Must contain "iface".
+- clocks: Interface clocks needed for controller.
+- #sound-dai-cells:Must be 1 for digital audio interfaces on the
controllers.
+- #address-cells: Must be 1 for SoundWire devices;
+- #size-cells: Must be <0> as SoundWire addresses have no size
component.
+- qcom,dout-ports: Must be count of data out ports
+- qcom,din-ports: Must be count of data in ports
+- qcom,ports-offset1: Must be frame offset1 of each data port.
+ Out followed by In. Used for Block size calculation.
+- qcom,ports-offset2: Must be frame offset2 of each data port.
+ Out followed by In. Used for Block size calculation.
+- qcom,ports-sinterval-low: Must be sample interval low of each data port.
+ Out followed by In. Used for Sample Interval
calculation.
These definitions are valid only for specific types of ports, I believe
here it's a 'reduced' port since offset2 is not required for simpler
ports and you don't have Hstart/Hstop.
so if you state that all of these properties are required, you are
explicitly ruling out future implementations of simple ports or will
have to redefine them later.
Also the definition 'frame offset1/2' is incorrect. the offset is
defined within each Payload Transport Window - not each frame - and its
definition depends on the packing mode used, which isn't defined or
stated here.
And last it looks like you assume a fixed frame shape - likely 50 rows
by 8 columns, it might be worth adding a note on the max values for
offset1/2 implied by this frame shape.
+
+= SoundWire devices
+Each subnode of the bus represents SoundWire device attached to it.
+The properties of these nodes are defined by the individual bindings.
+
+= EXAMPLE
+The following example represents a SoundWire controller on DB845c board
+which has controller integrated inside WCD934x codec on SDM845 SoC.
+
+soundwire: soundwire@c85 {
+ compatible = "qcom,soundwire-v1.3.0";
+ reg = <0xc85 0x20>;
+ interrupts = <20 IRQ_TYPE_EDGE_RISING>;
+ clocks = <>;
+ clock-names = "iface";
+ #sound-dai-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ qcom,dout-ports = <6>;
+ qcom,din-ports = <2>;
+ qcom,ports-sinterval-low =/bits/ 8 <0x07 0x1F 0x3F 0x7 0x1F 0x3F 0x0F
0x0F>;
+ qcom,ports-offset1 = /bits/ 8 <0x01 0x02 0x0C 0x6 0x12 0x0D 0x07 0x0A >;
+ qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x1F 0x00 0x00 0x1F 0x00 0x00>;
+
+ /* Left Speaker */
+ wsa8810@1{
+
+ reg = <1>;
+ };
+
+ /* Right Speaker */
+ wsa8810@2{
+
+ reg = <2>;
+ };
+};
