From 9c294739cf5bdb6c613f23dff099e44ac9d722a4 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 14:01:37 +0800 Subject: [PATCH 01/12] soundwire/ASoC: add leading zeroes in peripheral device name We recently added leading zeroes in dev_dbg() messages but forgot to do the same for the peripheral device name. Adding leading zeroes makes it easier to read manufacturer ID and part ID, e.g.: sdw:0:025d:0700:00 sdw:0:025d:0711:00 sdw:1:025d:0700:00 sdw:1:025d:1308:00 sdw:2:025d:0700:00 sdw:2:025d:0701:00 sdw:3:025d:0700:00 sdw:3:025d:0715:00 The use of '01x' for link_id and unique_id is intentional to show the value range in the code, it's understood it does not actually change the format. To avoid problems with git bisect, the same change needs to be applied to the Intel SoundWire machine driver, otherwise the components can't be found and the card registration fails. Tested-by: Srinivas Kandagatla Signed-off-by: Pierre-Louis Bossart Reviewed-by: Rander Wang Signed-off-by: Bard Liao Acked-by: Mark Brown Link: https://lore.kernel.org/r/20210511060137.29856-1-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- drivers/soundwire/slave.c | 4 ++-- sound/soc/intel/boards/sof_sdw.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index 0eed38a79c6d..669d7573320b 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -39,12 +39,12 @@ int sdw_slave_add(struct sdw_bus *bus, if (id->unique_id == SDW_IGNORED_UNIQUE_ID) { /* name shall be sdw:link:mfg:part:class */ - dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x", + dev_set_name(&slave->dev, "sdw:%01x:%04x:%04x:%02x", bus->link_id, id->mfg_id, id->part_id, id->class_id); } else { /* name shall be sdw:link:mfg:part:class:unique */ - dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x", + dev_set_name(&slave->dev, "sdw:%01x:%04x:%04x:%02x:%01x", bus->link_id, id->mfg_id, id->part_id, id->class_id, id->unique_id); } diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c index ecd3f90f4bbe..8df1e69235cc 100644 --- a/sound/soc/intel/boards/sof_sdw.c +++ b/sound/soc/intel/boards/sof_sdw.c @@ -581,13 +581,13 @@ static int create_codec_dai_name(struct device *dev, comp_index = i + offset; if (is_unique_device(link, sdw_version, mfg_id, part_id, class_id, i)) { - codec_str = "sdw:%x:%x:%x:%x"; + codec_str = "sdw:%01x:%04x:%04x:%02x"; codec[comp_index].name = devm_kasprintf(dev, GFP_KERNEL, codec_str, link_id, mfg_id, part_id, class_id); } else { - codec_str = "sdw:%x:%x:%x:%x:%x"; + codec_str = "sdw:%01x:%04x:%04x:%02x:%01x"; codec[comp_index].name = devm_kasprintf(dev, GFP_KERNEL, codec_str, link_id, mfg_id, part_id, From 0531e6b60569c71a34a9c5eb9bfbb1559b661cd8 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 13:49:45 +0800 Subject: [PATCH 02/12] soundwire: bandwidth allocation: improve error messages In rare corner cases, we see an error with the log: [ 838.297840] soundwire sdw-master-1: Compute bus params failed: -22 That's not very useful, there can be two different error conditions with the same -EINVAL code provided to the caller. Let's add better dev_err() messages to figure out what went wrong. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Guennadi Liakhovetski Signed-off-by: Bard Liao Link: https://lore.kernel.org/r/20210511054945.29558-1-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- drivers/soundwire/generic_bandwidth_allocation.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c index 84d129587084..f7c66083a4dd 100644 --- a/drivers/soundwire/generic_bandwidth_allocation.c +++ b/drivers/soundwire/generic_bandwidth_allocation.c @@ -382,12 +382,18 @@ static int sdw_compute_bus_params(struct sdw_bus *bus) */ } - if (i == clk_values) + if (i == clk_values) { + dev_err(bus->dev, "%s: could not find clock value for bandwidth %d\n", + __func__, bus->params.bandwidth); return -EINVAL; + } ret = sdw_select_row_col(bus, curr_dr_freq); - if (ret < 0) + if (ret < 0) { + dev_err(bus->dev, "%s: could not find frame configuration for bus dr_freq %d\n", + __func__, curr_dr_freq); return -EINVAL; + } bus->params.curr_dr_freq = curr_dr_freq; return 0; @@ -404,10 +410,8 @@ int sdw_compute_params(struct sdw_bus *bus) /* Computes clock frequency, frame shape and frame frequency */ ret = sdw_compute_bus_params(bus); - if (ret < 0) { - dev_err(bus->dev, "Compute bus params failed: %d\n", ret); + if (ret < 0) return ret; - } /* Compute transport and port params */ ret = sdw_compute_port_params(bus); From 345e9f5ca798600e44c0843646621f2804eb99f4 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 11:00:45 +0800 Subject: [PATCH 03/12] soundwire: bus: only use CLOCK_STOP_MODE0 and fix confusions Existing devices and implementations only support the required CLOCK_STOP_MODE0. All the code related to CLOCK_STOP_MODE1 has not been tested and is highly questionable, with a clear confusion between CLOCK_STOP_MODE1 and the simple clock stop state machine. This patch removes all usages of CLOCK_STOP_MODE1 - which has no impact on any solution - and fixes the use of the simple clock stop state machine. The resulting code should be a lot more symmetrical and easier to maintain. Note that CLOCK_STOP_MODE1 is not supported in the SoundWire Device Class specification so it's rather unlikely that we need to re-add this mode later. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Guennadi Liakhovetski Reviewed-by: Rander Wang Signed-off-by: Bard Liao Link: https://lore.kernel.org/r/20210511030048.25622-2-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- drivers/soundwire/bus.c | 100 ++++++++++++++-------------------- include/linux/soundwire/sdw.h | 2 - 2 files changed, 40 insertions(+), 62 deletions(-) diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index a9e0aa72654d..dc4033b6f2e9 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -821,26 +821,6 @@ static void sdw_modify_slave_status(struct sdw_slave *slave, mutex_unlock(&bus->bus_lock); } -static enum sdw_clk_stop_mode sdw_get_clk_stop_mode(struct sdw_slave *slave) -{ - enum sdw_clk_stop_mode mode; - - /* - * Query for clock stop mode if Slave implements - * ops->get_clk_stop_mode, else read from property. - */ - if (slave->ops && slave->ops->get_clk_stop_mode) { - mode = slave->ops->get_clk_stop_mode(slave); - } else { - if (slave->prop.clk_stop_mode1) - mode = SDW_CLK_STOP_MODE1; - else - mode = SDW_CLK_STOP_MODE0; - } - - return mode; -} - static int sdw_slave_clk_stop_callback(struct sdw_slave *slave, enum sdw_clk_stop_mode mode, enum sdw_clk_stop_type type) @@ -933,7 +913,6 @@ static int sdw_bus_wait_for_clk_prep_deprep(struct sdw_bus *bus, u16 dev_num) */ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) { - enum sdw_clk_stop_mode slave_mode; bool simple_clk_stop = true; struct sdw_slave *slave; bool is_slave = false; @@ -955,10 +934,8 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) /* Identify if Slave(s) are available on Bus */ is_slave = true; - slave_mode = sdw_get_clk_stop_mode(slave); - slave->curr_clk_stop_mode = slave_mode; - - ret = sdw_slave_clk_stop_callback(slave, slave_mode, + ret = sdw_slave_clk_stop_callback(slave, + SDW_CLK_STOP_MODE0, SDW_CLK_PRE_PREPARE); if (ret < 0) { dev_err(&slave->dev, @@ -966,22 +943,29 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) return ret; } - ret = sdw_slave_clk_stop_prepare(slave, - slave_mode, true); - if (ret < 0) { - dev_err(&slave->dev, - "pre-prepare failed:%d", ret); - return ret; - } - - if (slave_mode == SDW_CLK_STOP_MODE1) + /* Only prepare a Slave device if needed */ + if (!slave->prop.simple_clk_stop_capable) { simple_clk_stop = false; + + ret = sdw_slave_clk_stop_prepare(slave, + SDW_CLK_STOP_MODE0, + true); + if (ret < 0) { + dev_err(&slave->dev, + "pre-prepare failed:%d", ret); + return ret; + } + } } /* Skip remaining clock stop preparation if no Slave is attached */ if (!is_slave) return ret; + /* + * Don't wait for all Slaves to be ready if they follow the simple + * state machine + */ if (!simple_clk_stop) { ret = sdw_bus_wait_for_clk_prep_deprep(bus, SDW_BROADCAST_DEV_NUM); @@ -998,17 +982,13 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) slave->status != SDW_SLAVE_ALERT) continue; - slave_mode = slave->curr_clk_stop_mode; + ret = sdw_slave_clk_stop_callback(slave, + SDW_CLK_STOP_MODE0, + SDW_CLK_POST_PREPARE); - if (slave_mode == SDW_CLK_STOP_MODE1) { - ret = sdw_slave_clk_stop_callback(slave, - slave_mode, - SDW_CLK_POST_PREPARE); - - if (ret < 0) { - dev_err(&slave->dev, - "post-prepare failed:%d", ret); - } + if (ret < 0) { + dev_err(&slave->dev, + "post-prepare failed:%d", ret); } } @@ -1059,7 +1039,6 @@ EXPORT_SYMBOL(sdw_bus_clk_stop); */ int sdw_bus_exit_clk_stop(struct sdw_bus *bus) { - enum sdw_clk_stop_mode mode; bool simple_clk_stop = true; struct sdw_slave *slave; bool is_slave = false; @@ -1081,31 +1060,33 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus) /* Identify if Slave(s) are available on Bus */ is_slave = true; - mode = slave->curr_clk_stop_mode; - - if (mode == SDW_CLK_STOP_MODE1) { - simple_clk_stop = false; - continue; - } - - ret = sdw_slave_clk_stop_callback(slave, mode, + ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0, SDW_CLK_PRE_DEPREPARE); if (ret < 0) dev_warn(&slave->dev, "clk stop deprep failed:%d", ret); - ret = sdw_slave_clk_stop_prepare(slave, mode, - false); + /* Only de-prepare a Slave device if needed */ + if (!slave->prop.simple_clk_stop_capable) { + simple_clk_stop = false; - if (ret < 0) - dev_warn(&slave->dev, - "clk stop deprep failed:%d", ret); + ret = sdw_slave_clk_stop_prepare(slave, SDW_CLK_STOP_MODE0, + false); + + if (ret < 0) + dev_warn(&slave->dev, + "clk stop deprep failed:%d", ret); + } } /* Skip remaining clock stop de-preparation if no Slave is attached */ if (!is_slave) return 0; + /* + * Don't wait for all Slaves to be ready if they follow the simple + * state machine + */ if (!simple_clk_stop) sdw_bus_wait_for_clk_prep_deprep(bus, SDW_BROADCAST_DEV_NUM); @@ -1117,8 +1098,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus) slave->status != SDW_SLAVE_ALERT) continue; - mode = slave->curr_clk_stop_mode; - sdw_slave_clk_stop_callback(slave, mode, + sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0, SDW_CLK_POST_DEPREPARE); } diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index ced07f8fde87..5d93d9949653 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -624,7 +624,6 @@ struct sdw_slave_ops { int (*port_prep)(struct sdw_slave *slave, struct sdw_prepare_ch *prepare_ch, enum sdw_port_prep_ops pre_ops); - int (*get_clk_stop_mode)(struct sdw_slave *slave); int (*clk_stop)(struct sdw_slave *slave, enum sdw_clk_stop_mode mode, enum sdw_clk_stop_type type); @@ -675,7 +674,6 @@ struct sdw_slave { struct list_head node; struct completion port_ready[SDW_MAX_PORTS]; unsigned int m_port_map[SDW_MAX_PORTS]; - enum sdw_clk_stop_mode curr_clk_stop_mode; u16 dev_num; u16 dev_num_sticky; bool probed; From 448df2d8fcab6cc50e0de4679ce3afe2ece282f2 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 11:00:46 +0800 Subject: [PATCH 04/12] soundwire: add missing kernel-doc description For some reason we never added a description for the clk_stop callback. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Guennadi Liakhovetski Reviewed-by: Rander Wang Signed-off-by: Bard Liao Link: https://lore.kernel.org/r/20210511030048.25622-3-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- include/linux/soundwire/sdw.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 5d93d9949653..8ca736e92d5a 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -612,6 +612,7 @@ struct sdw_bus_params { * @update_status: Update Slave status * @bus_config: Update the bus config for Slave * @port_prep: Prepare the port with parameters + * @clk_stop: handle imp-def sequences before and after prepare and de-prepare */ struct sdw_slave_ops { int (*read_prop)(struct sdw_slave *sdw); From b50bb8ba369cdc8814e82558117711c12ef3af3d Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 11:00:47 +0800 Subject: [PATCH 05/12] soundwire: bus: handle -ENODATA errors in clock stop/start sequences If a device lost sync and can no longer ACK a command, it may not be able to enter a lower-power state but it will still be able to resync when the clock restarts. In those cases, we want to continue with the clock stop sequence. This patch modifies the behavior during clock stop sequences to only log errors unrelated to -ENODATA/Command_Ignored. The flow is also modified so that loops continue to prepare/deprepare other devices even when one seems to have lost sync. When resuming the clocks, all issues are logged with a dev_warn(), previously only some of them were checked. This is the only part that now differs between the clock stop entry and clock stop exit sequences: while we don't want to stop the suspend flow, we do want information on potential issues while resuming, as they may have ripple effects. For consistency the log messages are also modified to be unique and self-explanatory. Errors in sdw_slave_clk_stop_callback() were removed, they are now handled in the caller. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Guennadi Liakhovetski Reviewed-by: Rander Wang Signed-off-by: Bard Liao Link: https://lore.kernel.org/r/20210511030048.25622-4-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- drivers/soundwire/bus.c | 69 +++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index dc4033b6f2e9..100d904bf700 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -829,11 +829,8 @@ static int sdw_slave_clk_stop_callback(struct sdw_slave *slave, if (slave->ops && slave->ops->clk_stop) { ret = slave->ops->clk_stop(slave, mode, type); - if (ret < 0) { - dev_err(&slave->dev, - "Clk Stop type =%d failed: %d\n", type, ret); + if (ret < 0) return ret; - } } return 0; @@ -860,7 +857,8 @@ static int sdw_slave_clk_stop_prepare(struct sdw_slave *slave, } else { ret = sdw_read_no_pm(slave, SDW_SCP_SYSTEMCTRL); if (ret < 0) { - dev_err(&slave->dev, "SDW_SCP_SYSTEMCTRL read failed:%d\n", ret); + if (ret != -ENODATA) + dev_err(&slave->dev, "SDW_SCP_SYSTEMCTRL read failed:%d\n", ret); return ret; } val = ret; @@ -869,9 +867,8 @@ static int sdw_slave_clk_stop_prepare(struct sdw_slave *slave, ret = sdw_write_no_pm(slave, SDW_SCP_SYSTEMCTRL, val); - if (ret < 0) - dev_err(&slave->dev, - "Clock Stop prepare failed for slave: %d", ret); + if (ret < 0 && ret != -ENODATA) + dev_err(&slave->dev, "SDW_SCP_SYSTEMCTRL write failed:%d\n", ret); return ret; } @@ -922,6 +919,9 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) * In order to save on transition time, prepare * each Slave and then wait for all Slave(s) to be * prepared for clock stop. + * If one of the Slave devices has lost sync and + * replies with Command Ignored/-ENODATA, we continue + * the loop */ list_for_each_entry(slave, &bus->slaves, node) { if (!slave->dev_num) @@ -937,9 +937,8 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0, SDW_CLK_PRE_PREPARE); - if (ret < 0) { - dev_err(&slave->dev, - "pre-prepare failed:%d", ret); + if (ret < 0 && ret != -ENODATA) { + dev_err(&slave->dev, "clock stop pre-prepare cb failed:%d\n", ret); return ret; } @@ -950,9 +949,8 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) ret = sdw_slave_clk_stop_prepare(slave, SDW_CLK_STOP_MODE0, true); - if (ret < 0) { - dev_err(&slave->dev, - "pre-prepare failed:%d", ret); + if (ret < 0 && ret != -ENODATA) { + dev_err(&slave->dev, "clock stop prepare failed:%d\n", ret); return ret; } } @@ -960,7 +958,7 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) /* Skip remaining clock stop preparation if no Slave is attached */ if (!is_slave) - return ret; + return 0; /* * Don't wait for all Slaves to be ready if they follow the simple @@ -969,6 +967,12 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) if (!simple_clk_stop) { ret = sdw_bus_wait_for_clk_prep_deprep(bus, SDW_BROADCAST_DEV_NUM); + /* + * if there are no Slave devices present and the reply is + * Command_Ignored/-ENODATA, we don't need to continue with the + * flow and can just return here. The error code is not modified + * and its handling left as an exercise for the caller. + */ if (ret < 0) return ret; } @@ -986,13 +990,13 @@ int sdw_bus_prep_clk_stop(struct sdw_bus *bus) SDW_CLK_STOP_MODE0, SDW_CLK_POST_PREPARE); - if (ret < 0) { - dev_err(&slave->dev, - "post-prepare failed:%d", ret); + if (ret < 0 && ret != -ENODATA) { + dev_err(&slave->dev, "clock stop post-prepare cb failed:%d\n", ret); + return ret; } } - return ret; + return 0; } EXPORT_SYMBOL(sdw_bus_prep_clk_stop); @@ -1015,12 +1019,8 @@ int sdw_bus_clk_stop(struct sdw_bus *bus) ret = sdw_bwrite_no_pm(bus, SDW_BROADCAST_DEV_NUM, SDW_SCP_CTRL, SDW_SCP_CTRL_CLK_STP_NOW); if (ret < 0) { - if (ret == -ENODATA) - dev_dbg(bus->dev, - "ClockStopNow Broadcast msg ignored %d", ret); - else - dev_err(bus->dev, - "ClockStopNow Broadcast msg failed %d", ret); + if (ret != -ENODATA) + dev_err(bus->dev, "ClockStopNow Broadcast msg failed %d\n", ret); return ret; } @@ -1063,8 +1063,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus) ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0, SDW_CLK_PRE_DEPREPARE); if (ret < 0) - dev_warn(&slave->dev, - "clk stop deprep failed:%d", ret); + dev_warn(&slave->dev, "clock stop pre-deprepare cb failed:%d\n", ret); /* Only de-prepare a Slave device if needed */ if (!slave->prop.simple_clk_stop_capable) { @@ -1074,8 +1073,7 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus) false); if (ret < 0) - dev_warn(&slave->dev, - "clk stop deprep failed:%d", ret); + dev_warn(&slave->dev, "clock stop deprepare failed:%d\n", ret); } } @@ -1087,8 +1085,11 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus) * Don't wait for all Slaves to be ready if they follow the simple * state machine */ - if (!simple_clk_stop) - sdw_bus_wait_for_clk_prep_deprep(bus, SDW_BROADCAST_DEV_NUM); + if (!simple_clk_stop) { + ret = sdw_bus_wait_for_clk_prep_deprep(bus, SDW_BROADCAST_DEV_NUM); + if (ret < 0) + dev_warn(&slave->dev, "clock stop deprepare wait failed:%d\n", ret); + } list_for_each_entry(slave, &bus->slaves, node) { if (!slave->dev_num) @@ -1098,8 +1099,10 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus) slave->status != SDW_SLAVE_ALERT) continue; - sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0, - SDW_CLK_POST_DEPREPARE); + ret = sdw_slave_clk_stop_callback(slave, SDW_CLK_STOP_MODE0, + SDW_CLK_POST_DEPREPARE); + if (ret < 0) + dev_warn(&slave->dev, "clock stop post-deprepare cb failed:%d\n", ret); } return 0; From 54a6ca4fa8a316b2ae26fb9ad646bfb1b0b75410 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 11:00:48 +0800 Subject: [PATCH 06/12] soundwire: bus: add missing \n in dynamic debug They were missed in previous contributions. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Guennadi Liakhovetski Reviewed-by: Rander Wang Signed-off-by: Bard Liao Link: https://lore.kernel.org/r/20210511030048.25622-5-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- drivers/soundwire/bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 100d904bf700..85bcf60f9697 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -886,7 +886,7 @@ static int sdw_bus_wait_for_clk_prep_deprep(struct sdw_bus *bus, u16 dev_num) } val &= SDW_SCP_STAT_CLK_STP_NF; if (!val) { - dev_dbg(bus->dev, "clock stop prep/de-prep done slave:%d", + dev_dbg(bus->dev, "clock stop prep/de-prep done slave:%d\n", dev_num); return 0; } @@ -895,7 +895,7 @@ static int sdw_bus_wait_for_clk_prep_deprep(struct sdw_bus *bus, u16 dev_num) retry--; } while (retry); - dev_err(bus->dev, "clock stop prep/de-prep failed slave:%d", + dev_err(bus->dev, "clock stop prep/de-prep failed slave:%d\n", dev_num); return -ETIMEDOUT; From 36eee232df7b41dce5a26e4da2d083f6a6b0f476 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 10:52:47 +0800 Subject: [PATCH 07/12] soundwire: cadence_master: always set CMD_ACCEPT The Cadence IP can be configured in two different ways to deal with CMD_IGNORED replies to broadcast commands. The CMD_ACCEPT bitfield controls whether the command is discarded or if the IP proceeds with the change (typically a bank switch or clock stop command). The existing code seems to be inconsistent: a) For some historical reason, we set this CMD_ACCEPT bitfield during the initialization, but we don't during a resume from a clock-stoppped state. b) In addition, the loop used in the clock-stop sequence is quite racy, it's possible that a device has lost sync but it's still tagged as ATTACHED. c) If somehow a Device loses sync and is unable to ack a broadcast command, we do not have an error handling mechanism anyways. The IP should go ahead and let the Device regain sync at a later time. Make sure the CMD_ACCEPT bit is always set. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Guennadi Liakhovetski Reviewed-by: Rander Wang Signed-off-by: Bard Liao Link: https://lore.kernel.org/r/20210511025247.25339-1-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- drivers/soundwire/cadence_master.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 192dac10f0c2..25950422b085 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1428,20 +1428,6 @@ int sdw_cdns_clock_stop(struct sdw_cdns *cdns, bool block_wake) } } - /* - * This CMD_ACCEPT should be used when there are no devices - * attached on the link when entering clock stop mode. If this is - * not set and there is a broadcast write then the command ignored - * will be treated as a failure - */ - if (!slave_present) - cdns_updatel(cdns, CDNS_MCP_CONTROL, - CDNS_MCP_CONTROL_CMD_ACCEPT, - CDNS_MCP_CONTROL_CMD_ACCEPT); - else - cdns_updatel(cdns, CDNS_MCP_CONTROL, - CDNS_MCP_CONTROL_CMD_ACCEPT, 0); - /* commit changes */ ret = cdns_config_update(cdns); if (ret < 0) { @@ -1508,11 +1494,8 @@ int sdw_cdns_clock_restart(struct sdw_cdns *cdns, bool bus_reset) cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_BLOCK_WAKEUP, 0); - /* - * clear CMD_ACCEPT so that the command ignored - * will be treated as a failure during a broadcast write - */ - cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CMD_ACCEPT, 0); + cdns_updatel(cdns, CDNS_MCP_CONTROL, CDNS_MCP_CONTROL_CMD_ACCEPT, + CDNS_MCP_CONTROL_CMD_ACCEPT); if (!bus_reset) { From 037219925e7a802c1d038af18cec3d0a8c88a368 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 10:50:35 +0800 Subject: [PATCH 08/12] soundwire: dmi-quirks: remove duplicate initialization cppcheck warning: drivers/soundwire/dmi-quirks.c:85:12: style: Redundant initialization for 'map'. The initialized value is overwritten before it is read. [redundantInitialization] for (map = dmi_id->driver_data; map->adr; map++) { ^ drivers/soundwire/dmi-quirks.c:83:25: note: map is initialized struct adr_remap *map = dmi_id->driver_data; ^ drivers/soundwire/dmi-quirks.c:85:12: note: map is overwritten for (map = dmi_id->driver_data; map->adr; map++) { ^ Signed-off-by: Pierre-Louis Bossart Reviewed-by: Daniel Baluta Reviewed-by: Guennadi Liakhovetski Signed-off-by: Bard Liao Link: https://lore.kernel.org/r/20210511025035.25233-1-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- drivers/soundwire/dmi-quirks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soundwire/dmi-quirks.c b/drivers/soundwire/dmi-quirks.c index 82061c1d9835..5db0a2443a1d 100644 --- a/drivers/soundwire/dmi-quirks.c +++ b/drivers/soundwire/dmi-quirks.c @@ -80,7 +80,7 @@ u64 sdw_dmi_override_adr(struct sdw_bus *bus, u64 addr) /* check if any address remap quirk applies */ dmi_id = dmi_first_match(adr_remap_quirk_table); if (dmi_id) { - struct adr_remap *map = dmi_id->driver_data; + struct adr_remap *map; for (map = dmi_id->driver_data; map->adr; map++) { if (map->adr == addr) { From 1ec9d2e7936c677a177916dab4b2bf26afbe2bed Mon Sep 17 00:00:00 2001 From: Shaokun Zhang Date: Thu, 27 May 2021 19:24:58 +0800 Subject: [PATCH 09/12] soundwire: cadence: remove the repeated declaration Function 'cdns_reset_page_addr' is declared twice, so remove the repeated declaration. Cc: Vinod Koul Cc: Bard Liao Cc: Pierre-Louis Bossart Cc: Sanyog Kale Signed-off-by: Shaokun Zhang Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/1622114698-7943-1-git-send-email-zhangshaokun@hisilicon.com Signed-off-by: Vinod Koul --- drivers/soundwire/cadence_master.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 4d1aab5b5ec2..0e7f8b35bb21 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -180,9 +180,6 @@ enum sdw_command_response cdns_xfer_msg_defer(struct sdw_bus *bus, struct sdw_msg *msg, struct sdw_defer *defer); -enum sdw_command_response -cdns_reset_page_addr(struct sdw_bus *bus, unsigned int dev_num); - int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params); int cdns_set_sdw_stream(struct snd_soc_dai *dai, From 29a269c6f54825c643a5c35762a2829ba5be67f6 Mon Sep 17 00:00:00 2001 From: Pierre-Louis Bossart Date: Tue, 11 May 2021 13:21:32 +0800 Subject: [PATCH 10/12] soundwire: intel: move to auxiliary bus Now that the auxiliary_bus exists, there's no reason to use platform devices as children of a PCI device any longer. This patch refactors the code by extending a basic auxiliary device with Intel link-specific structures that need to be passed between controller and link levels. This refactoring is much cleaner with no need for cross-pointers between device and link structures. Note that the auxiliary bus API has separate init and add steps, which requires more attention in the error unwinding paths. The main loop needs to deal with kfree() and auxiliary_device_uninit() for the current iteration before jumping to the common label which releases everything allocated in prior iterations. Signed-off-by: Pierre-Louis Bossart Reviewed-by: Guennadi Liakhovetski Reviewed-by: Ranjani Sridharan Signed-off-by: Bard Liao Link: https://lore.kernel.org/r/20210511052132.28150-1-yung-chuan.liao@linux.intel.com Signed-off-by: Vinod Koul --- drivers/soundwire/Kconfig | 1 + drivers/soundwire/intel.c | 58 +++---- drivers/soundwire/intel.h | 14 +- drivers/soundwire/intel_init.c | 234 +++++++++++++++++++--------- include/linux/soundwire/sdw_intel.h | 6 +- 5 files changed, 204 insertions(+), 109 deletions(-) diff --git a/drivers/soundwire/Kconfig b/drivers/soundwire/Kconfig index 016e74230bb7..2b7795233282 100644 --- a/drivers/soundwire/Kconfig +++ b/drivers/soundwire/Kconfig @@ -25,6 +25,7 @@ config SOUNDWIRE_INTEL tristate "Intel SoundWire Master driver" select SOUNDWIRE_CADENCE select SOUNDWIRE_GENERIC_ALLOCATION + select AUXILIARY_BUS depends on ACPI && SND_SOC help SoundWire Intel Master driver. diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index fd95f94630b1..c11e3d8cd308 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -11,7 +11,7 @@ #include #include #include -#include +#include #include #include #include @@ -1327,11 +1327,14 @@ static int intel_init(struct sdw_intel *sdw) } /* - * probe and init + * probe and init (aux_dev_id argument is required by function prototype but not used) */ -static int intel_master_probe(struct platform_device *pdev) +static int intel_link_probe(struct auxiliary_device *auxdev, + const struct auxiliary_device_id *aux_dev_id) + { - struct device *dev = &pdev->dev; + struct device *dev = &auxdev->dev; + struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev); struct sdw_intel *sdw; struct sdw_cdns *cdns; struct sdw_bus *bus; @@ -1344,14 +1347,14 @@ static int intel_master_probe(struct platform_device *pdev) cdns = &sdw->cdns; bus = &cdns->bus; - sdw->instance = pdev->id; - sdw->link_res = dev_get_platdata(dev); + sdw->instance = auxdev->id; + sdw->link_res = &ldev->link_res; cdns->dev = dev; cdns->registers = sdw->link_res->registers; cdns->instance = sdw->instance; cdns->msg_count = 0; - bus->link_id = pdev->id; + bus->link_id = auxdev->id; sdw_cdns_probe(cdns); @@ -1384,10 +1387,10 @@ static int intel_master_probe(struct platform_device *pdev) return 0; } -int intel_master_startup(struct platform_device *pdev) +int intel_link_startup(struct auxiliary_device *auxdev) { struct sdw_cdns_stream_config config; - struct device *dev = &pdev->dev; + struct device *dev = &auxdev->dev; struct sdw_cdns *cdns = dev_get_drvdata(dev); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus; @@ -1524,9 +1527,9 @@ err_init: return ret; } -static int intel_master_remove(struct platform_device *pdev) +static void intel_link_remove(struct auxiliary_device *auxdev) { - struct device *dev = &pdev->dev; + struct device *dev = &auxdev->dev; struct sdw_cdns *cdns = dev_get_drvdata(dev); struct sdw_intel *sdw = cdns_to_intel(cdns); struct sdw_bus *bus = &cdns->bus; @@ -1542,19 +1545,17 @@ static int intel_master_remove(struct platform_device *pdev) snd_soc_unregister_component(dev); } sdw_bus_master_delete(bus); - - return 0; } -int intel_master_process_wakeen_event(struct platform_device *pdev) +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev) { - struct device *dev = &pdev->dev; + struct device *dev = &auxdev->dev; struct sdw_intel *sdw; struct sdw_bus *bus; void __iomem *shim; u16 wake_sts; - sdw = platform_get_drvdata(pdev); + sdw = dev_get_drvdata(dev); bus = &sdw->cdns.bus; if (bus->prop.hw_disabled) { @@ -1976,17 +1977,22 @@ static const struct dev_pm_ops intel_pm = { SET_RUNTIME_PM_OPS(intel_suspend_runtime, intel_resume_runtime, NULL) }; -static struct platform_driver sdw_intel_drv = { - .probe = intel_master_probe, - .remove = intel_master_remove, - .driver = { - .name = "intel-sdw", - .pm = &intel_pm, - } +static const struct auxiliary_device_id intel_link_id_table[] = { + { .name = "soundwire_intel.link" }, + {}, }; +MODULE_DEVICE_TABLE(auxiliary, intel_link_id_table); -module_platform_driver(sdw_intel_drv); +static struct auxiliary_driver sdw_intel_drv = { + .probe = intel_link_probe, + .remove = intel_link_remove, + .driver = { + /* auxiliary_driver_register() sets .name to be the modname */ + .pm = &intel_pm, + }, + .id_table = intel_link_id_table +}; +module_auxiliary_driver(sdw_intel_drv); MODULE_LICENSE("Dual BSD/GPL"); -MODULE_ALIAS("platform:intel-sdw"); -MODULE_DESCRIPTION("Intel Soundwire Master Driver"); +MODULE_DESCRIPTION("Intel Soundwire Link Driver"); diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index 06bac8ba14e9..0b47b148da3f 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -7,7 +7,6 @@ /** * struct sdw_intel_link_res - Soundwire Intel link resource structure, * typically populated by the controller driver. - * @pdev: platform_device * @mmio_base: mmio base of SoundWire registers * @registers: Link IO registers base * @shim: Audio shim pointer @@ -23,7 +22,6 @@ * @list: used to walk-through all masters exposed by the same controller */ struct sdw_intel_link_res { - struct platform_device *pdev; void __iomem *mmio_base; /* not strictly needed, useful for debug */ void __iomem *registers; void __iomem *shim; @@ -48,7 +46,15 @@ struct sdw_intel { #endif }; -int intel_master_startup(struct platform_device *pdev); -int intel_master_process_wakeen_event(struct platform_device *pdev); +int intel_link_startup(struct auxiliary_device *auxdev); +int intel_link_process_wakeen_event(struct auxiliary_device *auxdev); + +struct sdw_intel_link_dev { + struct auxiliary_device auxdev; + struct sdw_intel_link_res link_res; +}; + +#define auxiliary_dev_to_sdw_intel_link_dev(auxiliary_dev) \ + container_of(auxiliary_dev, struct sdw_intel_link_dev, auxdev) #endif /* __SDW_INTEL_LOCAL_H */ diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 30ce95ec2d70..9e283bef53d2 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -12,7 +12,7 @@ #include #include #include -#include +#include #include #include #include "cadence_master.h" @@ -24,28 +24,108 @@ #define SDW_LINK_BASE 0x30000 #define SDW_LINK_SIZE 0x10000 +static void intel_link_dev_release(struct device *dev) +{ + struct auxiliary_device *auxdev = to_auxiliary_dev(dev); + struct sdw_intel_link_dev *ldev = auxiliary_dev_to_sdw_intel_link_dev(auxdev); + + kfree(ldev); +} + +/* alloc, init and add link devices */ +static struct sdw_intel_link_dev *intel_link_dev_register(struct sdw_intel_res *res, + struct sdw_intel_ctx *ctx, + struct fwnode_handle *fwnode, + const char *name, + int link_id) +{ + struct sdw_intel_link_dev *ldev; + struct sdw_intel_link_res *link; + struct auxiliary_device *auxdev; + int ret; + + ldev = kzalloc(sizeof(*ldev), GFP_KERNEL); + if (!ldev) + return ERR_PTR(-ENOMEM); + + auxdev = &ldev->auxdev; + auxdev->name = name; + auxdev->dev.parent = res->parent; + auxdev->dev.fwnode = fwnode; + auxdev->dev.release = intel_link_dev_release; + + /* we don't use an IDA since we already have a link ID */ + auxdev->id = link_id; + + /* + * keep a handle on the allocated memory, to be used in all other functions. + * Since the same pattern is used to skip links that are not enabled, there is + * no need to check if ctx->ldev[i] is NULL later on. + */ + ctx->ldev[link_id] = ldev; + + /* Add link information used in the driver probe */ + link = &ldev->link_res; + link->mmio_base = res->mmio_base; + link->registers = res->mmio_base + SDW_LINK_BASE + + (SDW_LINK_SIZE * link_id); + link->shim = res->mmio_base + SDW_SHIM_BASE; + link->alh = res->mmio_base + SDW_ALH_BASE; + + link->ops = res->ops; + link->dev = res->dev; + + link->clock_stop_quirks = res->clock_stop_quirks; + link->shim_lock = &ctx->shim_lock; + link->shim_mask = &ctx->shim_mask; + link->link_mask = ctx->link_mask; + + /* now follow the two-step init/add sequence */ + ret = auxiliary_device_init(auxdev); + if (ret < 0) { + dev_err(res->parent, "failed to initialize link dev %s link_id %d\n", + name, link_id); + kfree(ldev); + return ERR_PTR(ret); + } + + ret = auxiliary_device_add(&ldev->auxdev); + if (ret < 0) { + dev_err(res->parent, "failed to add link dev %s link_id %d\n", + ldev->auxdev.name, link_id); + /* ldev will be freed with the put_device() and .release sequence */ + auxiliary_device_uninit(&ldev->auxdev); + return ERR_PTR(ret); + } + + return ldev; +} + +static void intel_link_dev_unregister(struct sdw_intel_link_dev *ldev) +{ + auxiliary_device_delete(&ldev->auxdev); + auxiliary_device_uninit(&ldev->auxdev); +} + static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx) { - struct sdw_intel_link_res *link = ctx->links; + struct sdw_intel_link_dev *ldev; u32 link_mask; int i; - if (!link) - return 0; - link_mask = ctx->link_mask; - for (i = 0; i < ctx->count; i++, link++) { + for (i = 0; i < ctx->count; i++) { if (!(link_mask & BIT(i))) continue; - if (link->pdev) { - pm_runtime_disable(&link->pdev->dev); - platform_device_unregister(link->pdev); - } + ldev = ctx->ldev[i]; - if (!link->clock_stop_quirks) - pm_runtime_put_noidle(link->dev); + pm_runtime_disable(&ldev->auxdev.dev); + if (!ldev->link_res.clock_stop_quirks) + pm_runtime_put_noidle(ldev->link_res.dev); + + intel_link_dev_unregister(ldev); } return 0; @@ -91,9 +171,8 @@ EXPORT_SYMBOL_NS(sdw_intel_thread, SOUNDWIRE_INTEL_INIT); static struct sdw_intel_ctx *sdw_intel_probe_controller(struct sdw_intel_res *res) { - struct platform_device_info pdevinfo; - struct platform_device *pdev; struct sdw_intel_link_res *link; + struct sdw_intel_link_dev *ldev; struct sdw_intel_ctx *ctx; struct acpi_device *adev; struct sdw_slave *slave; @@ -116,67 +195,60 @@ static struct sdw_intel_ctx count = res->count; dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count); - ctx = devm_kzalloc(&adev->dev, sizeof(*ctx), GFP_KERNEL); + /* + * we need to alloc/free memory manually and can't use devm: + * this routine may be called from a workqueue, and not from + * the parent .probe. + * If devm_ was used, the memory might never be freed on errors. + */ + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); if (!ctx) return NULL; ctx->count = count; - ctx->links = devm_kcalloc(&adev->dev, ctx->count, - sizeof(*ctx->links), GFP_KERNEL); - if (!ctx->links) - return NULL; - ctx->count = count; + /* + * allocate the array of pointers. The link-specific data is allocated + * as part of the first loop below and released with the auxiliary_device_uninit(). + * If some links are disabled, the link pointer will remain NULL. Given that the + * number of links is small, this is simpler than using a list to keep track of links. + */ + ctx->ldev = kcalloc(ctx->count, sizeof(*ctx->ldev), GFP_KERNEL); + if (!ctx->ldev) { + kfree(ctx); + return NULL; + } + ctx->mmio_base = res->mmio_base; ctx->link_mask = res->link_mask; ctx->handle = res->handle; mutex_init(&ctx->shim_lock); - link = ctx->links; link_mask = ctx->link_mask; INIT_LIST_HEAD(&ctx->link_list); - /* Create SDW Master devices */ - for (i = 0; i < count; i++, link++) { - if (!(link_mask & BIT(i))) { - dev_dbg(&adev->dev, - "Link %d masked, will not be enabled\n", i); + for (i = 0; i < count; i++) { + if (!(link_mask & BIT(i))) continue; - } - link->mmio_base = res->mmio_base; - link->registers = res->mmio_base + SDW_LINK_BASE - + (SDW_LINK_SIZE * i); - link->shim = res->mmio_base + SDW_SHIM_BASE; - link->alh = res->mmio_base + SDW_ALH_BASE; - - link->ops = res->ops; - link->dev = res->dev; - - link->clock_stop_quirks = res->clock_stop_quirks; - link->shim_lock = &ctx->shim_lock; - link->shim_mask = &ctx->shim_mask; - link->link_mask = link_mask; - - memset(&pdevinfo, 0, sizeof(pdevinfo)); - - pdevinfo.parent = res->parent; - pdevinfo.name = "intel-sdw"; - pdevinfo.id = i; - pdevinfo.fwnode = acpi_fwnode_handle(adev); - pdevinfo.data = link; - pdevinfo.size_data = sizeof(*link); - - pdev = platform_device_register_full(&pdevinfo); - if (IS_ERR(pdev)) { - dev_err(&adev->dev, - "platform device creation failed: %ld\n", - PTR_ERR(pdev)); + /* + * init and add a device for each link + * + * The name of the device will be soundwire_intel.link.[i], + * with the "soundwire_intel" module prefix automatically added + * by the auxiliary bus core. + */ + ldev = intel_link_dev_register(res, + ctx, + acpi_fwnode_handle(adev), + "link", + i); + if (IS_ERR(ldev)) goto err; - } - link->pdev = pdev; - link->cdns = platform_get_drvdata(pdev); + + link = &ldev->link_res; + link->cdns = dev_get_drvdata(&ldev->auxdev.dev); if (!link->cdns) { dev_err(&adev->dev, "failed to get link->cdns\n"); @@ -194,8 +266,7 @@ static struct sdw_intel_ctx num_slaves++; } - ctx->ids = devm_kcalloc(&adev->dev, num_slaves, - sizeof(*ctx->ids), GFP_KERNEL); + ctx->ids = kcalloc(num_slaves, sizeof(*ctx->ids), GFP_KERNEL); if (!ctx->ids) goto err; @@ -213,8 +284,14 @@ static struct sdw_intel_ctx return ctx; err: - ctx->count = i; - sdw_intel_cleanup(ctx); + while (i--) { + if (!(link_mask & BIT(i))) + continue; + ldev = ctx->ldev[i]; + intel_link_dev_unregister(ldev); + } + kfree(ctx->ldev); + kfree(ctx); return NULL; } @@ -222,7 +299,7 @@ static int sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) { struct acpi_device *adev; - struct sdw_intel_link_res *link; + struct sdw_intel_link_dev *ldev; u32 caps; u32 link_mask; int i; @@ -241,27 +318,28 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) return -EINVAL; } - if (!ctx->links) + if (!ctx->ldev) return -EINVAL; - link = ctx->links; link_mask = ctx->link_mask; /* Startup SDW Master devices */ - for (i = 0; i < ctx->count; i++, link++) { + for (i = 0; i < ctx->count; i++) { if (!(link_mask & BIT(i))) continue; - intel_master_startup(link->pdev); + ldev = ctx->ldev[i]; - if (!link->clock_stop_quirks) { + intel_link_startup(&ldev->auxdev); + + if (!ldev->link_res.clock_stop_quirks) { /* * we need to prevent the parent PCI device * from entering pm_runtime suspend, so that * power rails to the SoundWire IP are not * turned off. */ - pm_runtime_get_noresume(link->dev); + pm_runtime_get_noresume(ldev->link_res.dev); } } @@ -272,8 +350,8 @@ sdw_intel_startup_controller(struct sdw_intel_ctx *ctx) * sdw_intel_probe() - SoundWire Intel probe routine * @res: resource data * - * This registers a platform device for each Master handled by the controller, - * and SoundWire Master and Slave devices will be created by the platform + * This registers an auxiliary device for each Master handled by the controller, + * and SoundWire Master and Slave devices will be created by the auxiliary * device probe. All the information necessary is stored in the context, and * the res argument pointer can be freed after this step. * This function will be called after sdw_intel_acpi_scan() by SOF probe. @@ -306,27 +384,31 @@ EXPORT_SYMBOL_NS(sdw_intel_startup, SOUNDWIRE_INTEL_INIT); void sdw_intel_exit(struct sdw_intel_ctx *ctx) { sdw_intel_cleanup(ctx); + kfree(ctx->ids); + kfree(ctx->ldev); + kfree(ctx); } EXPORT_SYMBOL_NS(sdw_intel_exit, SOUNDWIRE_INTEL_INIT); void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx) { - struct sdw_intel_link_res *link; + struct sdw_intel_link_dev *ldev; u32 link_mask; int i; - if (!ctx->links) + if (!ctx->ldev) return; - link = ctx->links; link_mask = ctx->link_mask; /* Startup SDW Master devices */ - for (i = 0; i < ctx->count; i++, link++) { + for (i = 0; i < ctx->count; i++) { if (!(link_mask & BIT(i))) continue; - intel_master_process_wakeen_event(link->pdev); + ldev = ctx->ldev[i]; + + intel_link_process_wakeen_event(&ldev->auxdev); } } EXPORT_SYMBOL_NS(sdw_intel_process_wakeen_event, SOUNDWIRE_INTEL_INIT); diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 3a5446ac014a..1ebea7764011 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -58,7 +58,7 @@ struct sdw_intel_acpi_info { u32 link_mask; }; -struct sdw_intel_link_res; +struct sdw_intel_link_dev; /* Intel clock-stop/pm_runtime quirk definitions */ @@ -109,7 +109,7 @@ struct sdw_intel_slave_id { * Controller * @num_slaves: total number of devices exposed across all enabled links * @handle: ACPI parent handle - * @links: information for each link (controller-specific and kept + * @ldev: information for each link (controller-specific and kept * opaque here) * @ids: array of slave_id, representing Slaves exposed across all enabled * links @@ -123,7 +123,7 @@ struct sdw_intel_ctx { u32 link_mask; int num_slaves; acpi_handle handle; - struct sdw_intel_link_res *links; + struct sdw_intel_link_dev **ldev; struct sdw_intel_slave_id *ids; struct list_head link_list; struct mutex shim_lock; /* lock for access to shared SHIM registers */ From 031e668bc1ad7ccdbfb2b67b838bb6b7cc44ecf3 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Wed, 16 Jun 2021 15:59:01 +0100 Subject: [PATCH 11/12] soundwire: bus: Make sdw_nwrite() data pointer argument const Idiomatically, write functions should take const pointers to the data buffer, as they don't change the data. They are also likely to be called from functions that receive a const data pointer. Internally the pointer is passed to function/structs shared with the read functions, requiring a cast, but this is an implementation detail that should be hidden by the public API. Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/20210616145901.29402-1-rf@opensource.cirrus.com Signed-off-by: Vinod Koul --- drivers/soundwire/bus.c | 8 ++++---- include/linux/soundwire/sdw.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 85bcf60f9697..adcbf3969110 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -394,13 +394,13 @@ sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) } static int -sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) +sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val) { struct sdw_msg msg; int ret; ret = sdw_fill_msg(&msg, slave, addr, count, - slave->dev_num, SDW_MSG_FLAG_WRITE, val); + slave->dev_num, SDW_MSG_FLAG_WRITE, (u8 *)val); if (ret < 0) return ret; @@ -535,9 +535,9 @@ EXPORT_SYMBOL(sdw_nread); * @slave: SDW Slave * @addr: Register address * @count: length - * @val: Buffer for values to be read + * @val: Buffer for values to be written */ -int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) +int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val) { int ret; diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 8ca736e92d5a..ddbeb00799e4 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -1039,7 +1039,7 @@ int sdw_write(struct sdw_slave *slave, u32 addr, u8 value); int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value); int sdw_read_no_pm(struct sdw_slave *slave, u32 addr); int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val); -int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val); +int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, const u8 *val); int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id); void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id); From 3d3e88e336338834086278236d42039f3cde50e1 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Fri, 18 Jun 2021 15:47:45 +0100 Subject: [PATCH 12/12] soundwire: stream: Fix test for DP prepare complete In sdw_prep_deprep_slave_ports(), after the wait_for_completion() the DP prepare status register is read. If this indicates that the port is now prepared, the code should continue with the port setup. It is irrelevant whether the wait_for_completion() timed out if the port is now ready. The previous implementation would always fail if the wait_for_completion() timed out, even if the port was reporting successful prepare. This patch also fixes a minor bug where the return from sdw_read() was not checked for error - any error code with LSBits clear could be misinterpreted as a successful port prepare. Fixes: 79df15b7d37c ("soundwire: Add helpers for ports operations") Signed-off-by: Richard Fitzgerald Reviewed-by: Pierre-Louis Bossart Link: https://lore.kernel.org/r/20210618144745.30629-1-rf@opensource.cirrus.com Signed-off-by: Vinod Koul --- drivers/soundwire/stream.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 1eaedaaba094..1a18308f4ef4 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -422,7 +422,6 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus, struct completion *port_ready; struct sdw_dpn_prop *dpn_prop; struct sdw_prepare_ch prep_ch; - unsigned int time_left; bool intr = false; int ret = 0, val; u32 addr; @@ -479,15 +478,15 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus, /* Wait for completion on port ready */ port_ready = &s_rt->slave->port_ready[prep_ch.num]; - time_left = wait_for_completion_timeout(port_ready, - msecs_to_jiffies(dpn_prop->ch_prep_timeout)); + wait_for_completion_timeout(port_ready, + msecs_to_jiffies(dpn_prop->ch_prep_timeout)); val = sdw_read(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num)); - val &= p_rt->ch_mask; - if (!time_left || val) { + if ((val < 0) || (val & p_rt->ch_mask)) { + ret = (val < 0) ? val : -ETIMEDOUT; dev_err(&s_rt->slave->dev, - "Chn prep failed for port:%d\n", prep_ch.num); - return -ETIMEDOUT; + "Chn prep failed for port %d: %d\n", prep_ch.num, ret); + return ret; } }