[alsa-devel] [PATCH 00/17] soundwire: fixes for 5.4
This series provides an update on the initial RFC. Debugfs and Intel updates will be provided in follow-up patches. The order of patches was changed since the RFC so detailed change logs are provided below.
Changes since RFC:
patch1: feedback from Vinod and Cezary use local variable to reduce number of de-references document that first argument is mandatory
patch2: feedback from Vinod document that the work-around is required for all existing controllers
patch 3: feedback from Vinod clarify comment that MCP_INT_IRQ ungates all other settings. use MCP_INT_SLAVE_MASK instead of all individual settings ORed.
Patch 4: feedback from Vinod: demote dynamic debug log to dev_dbg (was dev_err)
patch5: clarify commit message that the helpers will be used in the Cadence parts as well.
patch 6: feedback from Vinod s/BIOS/firmware remove magic numbers, introduce macros
patch 7: add kbuild warning message in commit message
patch 8: no change
patch 9: feedback from Guennadi remove unnecessary initializations
patch 10/11/12: feedback from Vinod split initial patch in 3 (prototype, Intel, Cadence) add explanations on what mclk_freq is in commit messages remove pr_err logs missed in RFC
patch 13: feedback from Bard remove unecessary reads before update
patch 14: no change
patch 15: update commit message
patch 16: update commit message remove unnecessary dynamic debug log
patch 17: no change
Bard liao (1): soundwire: include mod_devicetable.h to avoid compiling warnings
Pierre-Louis Bossart (15): soundwire: intel: prevent possible dereference in hw_params soundwire: intel: fix channel number reported by hardware soundwire: cadence_master: revisit interrupt settings soundwire: bus: improve dynamic debug comments for enumeration soundwire: export helpers to find row and column values soundwire: cadence_master: use firmware defaults for frame shape soundwire: stream: fix disable sequence soundwire: stream: remove unnecessary variable initializations soundwire: add new mclk_freq field for properties soundwire: intel: read mclk_freq property from firmware soundwire: cadence_master: make use of mclk_freq property 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
Rander Wang (1): soundwire: cadence_master: fix divider setting in clock register
drivers/soundwire/bus.c | 5 +- drivers/soundwire/bus.h | 7 +- drivers/soundwire/cadence_master.c | 102 +++++++++++++++++++--------- drivers/soundwire/cadence_master.h | 2 - drivers/soundwire/intel.c | 83 ++++++++++++++++++++-- drivers/soundwire/intel_init.c | 11 +++ drivers/soundwire/stream.c | 93 ++++++++++++++++--------- include/linux/soundwire/sdw.h | 6 ++ include/linux/soundwire/sdw_intel.h | 1 + 9 files changed, 236 insertions(+), 74 deletions(-)
This should not happen in production systems but we should test for all callback arguments before invoking the config_stream callback.
Update the prototype to clarify that the first argument is mandatory.
Also use local variable instead of multiple dereferences to improve readability.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel.c | 6 ++++-- include/linux/soundwire/sdw_intel.h | 1 + 2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 3d9ca6479e94..a906789c7f78 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -498,8 +498,10 @@ 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) - return sdw->res->ops->config_stream(sdw->res->arg, + struct sdw_intel_link_res *res = sdw->res; + + if (res->ops && res->ops->config_stream && res->arg) + return res->ops->config_stream(res->arg, substream, dai, hw_params, link_id);
return -EIO; diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 4d70da45363d..c9427cb6020b 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -8,6 +8,7 @@ * struct sdw_intel_ops: Intel audio driver callback ops * * @config_stream: configure the stream with the hw_params + * the first argument containing the context is mandatory */ struct sdw_intel_ops { int (*config_stream)(void *arg, void *substream,
On all released Intel controllers (CNL/CML/ICL), PDI2 reports an invalid count, force the correct hardware-supported value
This may have to be revisited with platform-specific values if the hardware changes, but for now this is good enough.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index a906789c7f78..fe3266bc1d12 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -390,6 +390,16 @@ 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)); + + /* + * WORKAROUND: on all existing Intel controllers, pdi + * number 2 reports channel count as 1 even though it + * supports 8 channels. 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) >>
Adding missing interrupt masks (parity, etc) and missing checks. Clarify which masks are for which usage.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a1c5f05ef27b..5d9729b4d634 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_CLASH BIT(10) #define CDNS_MCP_INT_DATA_CLASH BIT(9) +#define CDNS_MCP_INT_PARITY BIT(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_INTSET 0x4C
@@ -669,6 +672,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"); @@ -736,10 +744,23 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns) cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, CDNS_MCP_SLAVE_INTMASK1_MASK);
- 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; + /* enable detection of all slave state changes */ + mask = CDNS_MCP_INT_SLAVE_MASK; + + /* 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; + + /* + * CDNS_MCP_INT_IRQ needs to be set otherwise all previous + * settings are irrelevant + */ + mask |= CDNS_MCP_INT_IRQ;
cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
update comments to provide better understanding of enumeration flows.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- 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 89d5f1537d9b..40f7d0dc59a6 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -476,7 +476,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(&slave->dev, "Program device_num failed: %d\n", ret); + dev_err(&slave->dev, "Program device_num %d failed: %d\n", + dev_num, ret); return ret; }
@@ -533,6 +534,7 @@ static int sdw_program_device_num(struct sdw_bus *bus) do { ret = sdw_transfer(bus, &msg); if (ret == -ENODATA) { /* end of device id reads */ + dev_dbg(bus->dev, "No more devices to enumerate\n"); ret = 0; break; } @@ -975,6 +977,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus, int i, ret = 0;
if (status[0] == SDW_SLAVE_ATTACHED) { + dev_dbg(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);
Add a prefix for common tables and export 2 helpers to set the frame shapes based on row/col values.
These changes simplify bandwidth allocation algorithms as well as the Cadence parts which all need to convert from frame shape to indices used by the standard. These helpers are used in the following patch.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- 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 648436a995a3..89e74adf96fa 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,
Remove hard-coding and use firmware (BIOS/DT) values. If they are wrong use default 48x2 frame shape.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 5d9729b4d634..89c55e4bb72c 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -48,6 +48,8 @@ #define CDNS_MCP_SSPSTAT 0xC #define CDNS_MCP_FRAME_SHAPE 0x10 #define CDNS_MCP_FRAME_SHAPE_INIT 0x14 +#define CDNS_MCP_FRAME_SHAPE_COL_MASK GENMASK(2, 0) +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET 3
#define CDNS_MCP_CONFIG_UPDATE 0x18 #define CDNS_MCP_CONFIG_UPDATE_BIT BIT(0) @@ -175,7 +177,6 @@ /* Driver defaults */
#define CDNS_DEFAULT_CLK_DIVIDER 0 -#define CDNS_DEFAULT_FRAME_SHAPE 0x30 #define CDNS_DEFAULT_SSP_INTERVAL 0x18 #define CDNS_TX_TIMEOUT 2000
@@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns, } EXPORT_SYMBOL(sdw_cdns_pdi_init);
+static u32 cdns_set_initial_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) & CDNS_MCP_FRAME_SHAPE_COL_MASK; + + val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c; + + return val; +} + /** * sdw_cdns_init() - Cadence initialization * @cdns: Cadence instance @@ -923,8 +938,13 @@ int sdw_cdns_init(struct sdw_cdns *cdns) val |= CDNS_DEFAULT_CLK_DIVIDER; 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); + /* + * Frame shape changes after initialization have to be done + * with the bank switch mechanism + */ + val = cdns_set_initial_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);
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 5d9729b4d634..89c55e4bb72c 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -48,6 +48,8 @@ #define CDNS_MCP_SSPSTAT 0xC #define CDNS_MCP_FRAME_SHAPE 0x10 #define CDNS_MCP_FRAME_SHAPE_INIT 0x14 +#define CDNS_MCP_FRAME_SHAPE_COL_MASK GENMASK(2, 0) +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET 3
#define CDNS_MCP_CONFIG_UPDATE 0x18 #define CDNS_MCP_CONFIG_UPDATE_BIT BIT(0) @@ -175,7 +177,6 @@ /* Driver defaults */
#define CDNS_DEFAULT_CLK_DIVIDER 0 -#define CDNS_DEFAULT_FRAME_SHAPE 0x30 #define CDNS_DEFAULT_SSP_INTERVAL 0x18 #define CDNS_TX_TIMEOUT 2000
@@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns, } EXPORT_SYMBOL(sdw_cdns_pdi_init);
+static u32 cdns_set_initial_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) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
- val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
- return val;
+}
Guess this have been said already, but this function could be simplified - unless you really want to keep explicit variable declaration. Both "c" and "r" declarations could be merged into single line while "val" is not needed at all.
One more thing - is AND bitwise op really needed for cols explicitly? We know all col values upfront - these are static and declared in global table nearby. Static declaration takes care of "initial range-check". Is another one necessary?
Moreover, this is a _get_ and certainly not a _set_ type of function. I'd even consider renaming it to: "cdns_get_frame_shape" as this is neither a _set_ nor an explicit initial frame shape setter.
It might be even helpful to split two usages:
#define sdw_frame_shape(col_idx, row_idx) \ ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
u32 cdns_get_frame_shape(u16 rows, u16 cols) { u16 c, r;
r = sdw_find_row_index(rows); c = sdw_find_col_index(cols);
return sdw_frame_shape(c, r); }
The above may even be simplified into one-liner.
On 8/6/19 10:27 AM, Cezary Rojewski wrote:
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 5d9729b4d634..89c55e4bb72c 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -48,6 +48,8 @@ #define CDNS_MCP_SSPSTAT 0xC #define CDNS_MCP_FRAME_SHAPE 0x10 #define CDNS_MCP_FRAME_SHAPE_INIT 0x14 +#define CDNS_MCP_FRAME_SHAPE_COL_MASK GENMASK(2, 0) +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET 3 #define CDNS_MCP_CONFIG_UPDATE 0x18 #define CDNS_MCP_CONFIG_UPDATE_BIT BIT(0) @@ -175,7 +177,6 @@ /* Driver defaults */ #define CDNS_DEFAULT_CLK_DIVIDER 0 -#define CDNS_DEFAULT_FRAME_SHAPE 0x30 #define CDNS_DEFAULT_SSP_INTERVAL 0x18 #define CDNS_TX_TIMEOUT 2000 @@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns, } EXPORT_SYMBOL(sdw_cdns_pdi_init); +static u32 cdns_set_initial_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) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
+ val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
+ return val; +}
Guess this have been said already, but this function could be simplified
- unless you really want to keep explicit variable declaration. Both "c"
and "r" declarations could be merged into single line while "val" is not needed at all.
One more thing - is AND bitwise op really needed for cols explicitly? We know all col values upfront - these are static and declared in global table nearby. Static declaration takes care of "initial range-check". Is another one necessary?
Moreover, this is a _get_ and certainly not a _set_ type of function. I'd even consider renaming it to: "cdns_get_frame_shape" as this is neither a _set_ nor an explicit initial frame shape setter.
It might be even helpful to split two usages:
#define sdw_frame_shape(col_idx, row_idx) \ ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
u32 cdns_get_frame_shape(u16 rows, u16 cols) { u16 c, r;
r = sdw_find_row_index(rows); c = sdw_find_col_index(cols);
return sdw_frame_shape(c, r); }
The above may even be simplified into one-liner.
This is a function used once on startup, there is no real need to simplify further. The separate variables help add debug traces as needed and keep the code readable while showing how the values are encoded into a register.
On 2019-08-06 17:36, Pierre-Louis Bossart wrote:
On 8/6/19 10:27 AM, Cezary Rojewski wrote:
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 5d9729b4d634..89c55e4bb72c 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -48,6 +48,8 @@ #define CDNS_MCP_SSPSTAT 0xC #define CDNS_MCP_FRAME_SHAPE 0x10 #define CDNS_MCP_FRAME_SHAPE_INIT 0x14 +#define CDNS_MCP_FRAME_SHAPE_COL_MASK GENMASK(2, 0) +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET 3 #define CDNS_MCP_CONFIG_UPDATE 0x18 #define CDNS_MCP_CONFIG_UPDATE_BIT BIT(0) @@ -175,7 +177,6 @@ /* Driver defaults */ #define CDNS_DEFAULT_CLK_DIVIDER 0 -#define CDNS_DEFAULT_FRAME_SHAPE 0x30 #define CDNS_DEFAULT_SSP_INTERVAL 0x18 #define CDNS_TX_TIMEOUT 2000 @@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns, } EXPORT_SYMBOL(sdw_cdns_pdi_init); +static u32 cdns_set_initial_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) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
+ val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
+ return val; +}
Guess this have been said already, but this function could be simplified - unless you really want to keep explicit variable declaration. Both "c" and "r" declarations could be merged into single line while "val" is not needed at all.
One more thing - is AND bitwise op really needed for cols explicitly? We know all col values upfront - these are static and declared in global table nearby. Static declaration takes care of "initial range-check". Is another one necessary?
Moreover, this is a _get_ and certainly not a _set_ type of function. I'd even consider renaming it to: "cdns_get_frame_shape" as this is neither a _set_ nor an explicit initial frame shape setter.
It might be even helpful to split two usages:
#define sdw_frame_shape(col_idx, row_idx) \ ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
u32 cdns_get_frame_shape(u16 rows, u16 cols) { u16 c, r;
r = sdw_find_row_index(rows); c = sdw_find_col_index(cols);
return sdw_frame_shape(c, r); }
The above may even be simplified into one-liner.
This is a function used once on startup, there is no real need to simplify further. The separate variables help add debug traces as needed and keep the code readable while showing how the values are encoded into a register.
Eh, I've thought it's gonna be exposed to userspace (via uapi) so it can be fetched by tests or tools.
In such case - if there is a single usage only - guess function is fine as is.
On 06-08-19, 18:06, Cezary Rojewski wrote:
On 2019-08-06 17:36, Pierre-Louis Bossart wrote:
On 8/6/19 10:27 AM, Cezary Rojewski wrote:
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 5d9729b4d634..89c55e4bb72c 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -48,6 +48,8 @@ #define CDNS_MCP_SSPSTAT 0xC #define CDNS_MCP_FRAME_SHAPE 0x10 #define CDNS_MCP_FRAME_SHAPE_INIT 0x14 +#define CDNS_MCP_FRAME_SHAPE_COL_MASK GENMASK(2, 0) +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET 3 #define CDNS_MCP_CONFIG_UPDATE 0x18 #define CDNS_MCP_CONFIG_UPDATE_BIT BIT(0) @@ -175,7 +177,6 @@ /* Driver defaults */ #define CDNS_DEFAULT_CLK_DIVIDER 0 -#define CDNS_DEFAULT_FRAME_SHAPE 0x30 #define CDNS_DEFAULT_SSP_INTERVAL 0x18 #define CDNS_TX_TIMEOUT 2000 @@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns, } EXPORT_SYMBOL(sdw_cdns_pdi_init); +static u32 cdns_set_initial_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) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
+ val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
+ return val; +}
Guess this have been said already, but this function could be simplified - unless you really want to keep explicit variable declaration. Both "c" and "r" declarations could be merged into single line while "val" is not needed at all.
One more thing - is AND bitwise op really needed for cols explicitly? We know all col values upfront - these are static and declared in global table nearby. Static declaration takes care of "initial range-check". Is another one necessary?
Moreover, this is a _get_ and certainly not a _set_ type of function. I'd even consider renaming it to: "cdns_get_frame_shape" as this is neither a _set_ nor an explicit initial frame shape setter.
It might be even helpful to split two usages:
#define sdw_frame_shape(col_idx, row_idx) \ ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
u32 cdns_get_frame_shape(u16 rows, u16 cols) { u16 c, r;
r = sdw_find_row_index(rows); c = sdw_find_col_index(cols);
return sdw_frame_shape(c, r); }
The above may even be simplified into one-liner.
This is a function used once on startup, there is no real need to simplify further. The separate variables help add debug traces as needed and keep the code readable while showing how the values are encoded into a register.
Eh, I've thought it's gonna be exposed to userspace (via uapi) so it can be fetched by tests or tools.
Uapi? I dont see anything in this or other series posted, did I miss something? Also I am not sure I like the idea of exposing these to userland!
In such case - if there is a single usage only - guess function is fine as is.
+static u32 cdns_set_initial_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) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
+ val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
+ return val; +}
Guess this have been said already, but this function could be simplified - unless you really want to keep explicit variable declaration. Both "c" and "r" declarations could be merged into single line while "val" is not needed at all.
One more thing - is AND bitwise op really needed for cols explicitly? We know all col values upfront - these are static and declared in global table nearby. Static declaration takes care of "initial range-check". Is another one necessary?
Moreover, this is a _get_ and certainly not a _set_ type of function. I'd even consider renaming it to: "cdns_get_frame_shape" as this is neither a _set_ nor an explicit initial frame shape setter.
It might be even helpful to split two usages:
#define sdw_frame_shape(col_idx, row_idx) \ ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
u32 cdns_get_frame_shape(u16 rows, u16 cols) { u16 c, r;
r = sdw_find_row_index(rows); c = sdw_find_col_index(cols);
return sdw_frame_shape(c, r); }
The above may even be simplified into one-liner.
This is a function used once on startup, there is no real need to simplify further. The separate variables help add debug traces as needed and keep the code readable while showing how the values are encoded into a register.
Eh, I've thought it's gonna be exposed to userspace (via uapi) so it can be fetched by tests or tools.
Uapi? I dont see anything in this or other series posted, did I miss something? Also I am not sure I like the idea of exposing these to userland!
Vinod, that was never the intent, and Cezary agreed, see following line
In such case - if there is a single usage only - guess function is fine as is.
From: Bard liao yung-chuan.liao@linux.intel.com
When integrating SoundWire, kbuild throws this warning with randconfig:
include/linux/soundwire/sdw.h:571:17: warning: 'struct
sdw_device_id' declared inside parameter list will not be visible outside of this definition or declaration
const struct sdw_device_id *id); ^~~~~~~~~~~~~
Fix by adding the relevant include
Reported-by: kbuild test robot lkp@intel.com Signed-off-by: Bard liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- 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 <linux/mod_devicetable.h> + struct sdw_bus; struct sdw_slave;
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 pierre-louis.bossart@linux.intel.com --- 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, &stream->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; }
/**
A number of variables don't need to be initialized.
In a couple of cases where we loop on a list of runtimes, the code handling of the 'bus' variable leads to warnings that it may not be initialized. Add a specific error case to trap such cases.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/stream.c | 64 ++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 26 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 75b9ad1fb1a6..8d6c13528b68 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -369,7 +369,7 @@ static int sdw_enable_disable_master_ports(struct sdw_master_runtime *m_rt, static int sdw_enable_disable_ports(struct sdw_master_runtime *m_rt, bool en) { struct sdw_port_runtime *s_port, *m_port; - struct sdw_slave_runtime *s_rt = NULL; + struct sdw_slave_runtime *s_rt; int ret = 0;
/* Enable/Disable Slave port(s) */ @@ -417,7 +417,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus, struct sdw_port_runtime *p_rt, bool prep) { - struct completion *port_ready = NULL; + struct completion *port_ready; struct sdw_dpn_prop *dpn_prop; struct sdw_prepare_ch prep_ch; unsigned int time_left; @@ -537,7 +537,7 @@ static int sdw_prep_deprep_master_ports(struct sdw_master_runtime *m_rt, */ static int sdw_prep_deprep_ports(struct sdw_master_runtime *m_rt, bool prep) { - struct sdw_slave_runtime *s_rt = NULL; + struct sdw_slave_runtime *s_rt; struct sdw_port_runtime *p_rt; int ret = 0;
@@ -605,7 +605,7 @@ static int sdw_notify_config(struct sdw_master_runtime *m_rt) */ static int sdw_program_params(struct sdw_bus *bus) { - struct sdw_master_runtime *m_rt = NULL; + struct sdw_master_runtime *m_rt; int ret = 0;
list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) { @@ -642,8 +642,8 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) int col_index, row_index; bool multi_link; struct sdw_msg *wr_msg; - u8 *wbuf = NULL; - int ret = 0; + u8 *wbuf; + int ret; u16 addr;
wr_msg = kzalloc(sizeof(*wr_msg), GFP_KERNEL); @@ -741,9 +741,9 @@ static int sdw_ml_sync_bank_switch(struct sdw_bus *bus)
static int do_bank_switch(struct sdw_stream_runtime *stream) { - struct sdw_master_runtime *m_rt = NULL; + struct sdw_master_runtime *m_rt; const struct sdw_master_ops *ops; - struct sdw_bus *bus = NULL; + struct sdw_bus *bus; bool multi_link = false; int ret = 0;
@@ -886,7 +886,7 @@ static struct sdw_master_runtime *sdw_find_master_rt(struct sdw_bus *bus, struct sdw_stream_runtime *stream) { - struct sdw_master_runtime *m_rt = NULL; + struct sdw_master_runtime *m_rt;
/* Retrieve Bus handle if already available */ list_for_each_entry(m_rt, &stream->master_list, stream_node) { @@ -955,7 +955,7 @@ static struct sdw_slave_runtime struct sdw_stream_config *stream_config, struct sdw_stream_runtime *stream) { - struct sdw_slave_runtime *s_rt = NULL; + struct sdw_slave_runtime *s_rt;
s_rt = kzalloc(sizeof(*s_rt), GFP_KERNEL); if (!s_rt) @@ -1261,7 +1261,7 @@ int sdw_stream_add_master(struct sdw_bus *bus, unsigned int num_ports, struct sdw_stream_runtime *stream) { - struct sdw_master_runtime *m_rt = NULL; + struct sdw_master_runtime *m_rt; int ret;
mutex_lock(&bus->bus_lock); @@ -1428,7 +1428,7 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave, */ static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream) { - struct sdw_master_runtime *m_rt = NULL; + struct sdw_master_runtime *m_rt; struct sdw_bus *bus = NULL;
/* Iterate for all Master(s) in Master list */ @@ -1462,9 +1462,9 @@ static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) { - struct sdw_master_runtime *m_rt = NULL; + struct sdw_master_runtime *m_rt; struct sdw_bus *bus = NULL; - struct sdw_master_prop *prop = NULL; + struct sdw_master_prop *prop; struct sdw_bus_params params; int ret;
@@ -1493,6 +1493,11 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) } }
+ if (!bus) { + pr_err("Configuration error in %s\n", __func__); + return -EINVAL; + } + ret = do_bank_switch(stream); if (ret < 0) { dev_err(bus->dev, "Bank switch failed: %d\n", ret); @@ -1549,7 +1554,7 @@ EXPORT_SYMBOL(sdw_prepare_stream);
static int _sdw_enable_stream(struct sdw_stream_runtime *stream) { - struct sdw_master_runtime *m_rt = NULL; + struct sdw_master_runtime *m_rt; struct sdw_bus *bus = NULL; int ret;
@@ -1573,6 +1578,11 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream) } }
+ if (!bus) { + pr_err("Configuration error in %s\n", __func__); + return -EINVAL; + } + ret = do_bank_switch(stream); if (ret < 0) { dev_err(bus->dev, "Bank switch failed: %d\n", ret); @@ -1592,7 +1602,7 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream) */ int sdw_enable_stream(struct sdw_stream_runtime *stream) { - int ret = 0; + int ret;
if (!stream) { pr_err("SoundWire: Handle not found for stream\n"); @@ -1612,12 +1622,12 @@ EXPORT_SYMBOL(sdw_enable_stream);
static int _sdw_disable_stream(struct sdw_stream_runtime *stream) { - struct sdw_master_runtime *m_rt = NULL; - struct sdw_bus *bus = NULL; + struct sdw_master_runtime *m_rt; int ret;
list_for_each_entry(m_rt, &stream->master_list, stream_node) { - bus = m_rt->bus; + struct sdw_bus *bus = m_rt->bus; + /* Disable port(s) */ ret = sdw_enable_disable_ports(m_rt, false); if (ret < 0) { @@ -1628,7 +1638,8 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) stream->state = SDW_STREAM_DISABLED;
list_for_each_entry(m_rt, &stream->master_list, stream_node) { - bus = m_rt->bus; + struct sdw_bus *bus = m_rt->bus; + /* Program params */ ret = sdw_program_params(bus); if (ret < 0) { @@ -1639,13 +1650,14 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
ret = do_bank_switch(stream); if (ret < 0) { - dev_err(bus->dev, "Bank switch failed: %d\n", ret); + pr_err("Bank switch failed: %d\n", ret); return ret; }
/* make sure alternate bank (previous current) is also disabled */ list_for_each_entry(m_rt, &stream->master_list, stream_node) { - bus = m_rt->bus; + struct sdw_bus *bus = m_rt->bus; + /* Disable port(s) */ ret = sdw_enable_disable_ports(m_rt, false); if (ret < 0) { @@ -1666,7 +1678,7 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) */ int sdw_disable_stream(struct sdw_stream_runtime *stream) { - int ret = 0; + int ret;
if (!stream) { pr_err("SoundWire: Handle not found for stream\n"); @@ -1686,8 +1698,8 @@ EXPORT_SYMBOL(sdw_disable_stream);
static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream) { - struct sdw_master_runtime *m_rt = NULL; - struct sdw_bus *bus = NULL; + struct sdw_master_runtime *m_rt; + struct sdw_bus *bus; int ret = 0;
list_for_each_entry(m_rt, &stream->master_list, stream_node) { @@ -1725,7 +1737,7 @@ static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream) */ int sdw_deprepare_stream(struct sdw_stream_runtime *stream) { - int ret = 0; + int ret;
if (!stream) { pr_err("SoundWire: Handle not found for stream\n");
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
@@ -1493,6 +1493,11 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) } }
- if (!bus) {
pr_err("Configuration error in %s\n", __func__);return -EINVAL;- }
This should probably be located in separate path - not at all an initialization removal.
@@ -1573,6 +1578,11 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream) } }
- if (!bus) {
pr_err("Configuration error in %s\n", __func__);return -EINVAL;- }
Same here.
@@ -1639,13 +1650,14 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
ret = do_bank_switch(stream); if (ret < 0) {
dev_err(bus->dev, "Bank switch failed: %d\n", ret);
pr_err("Bank switch failed: %d\n", ret);
Here too.
I might have missed something though I bet you got my point.
On 8/6/19 10:31 AM, Cezary Rojewski wrote:
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
@@ -1493,6 +1493,11 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) } } + if (!bus) { + pr_err("Configuration error in %s\n", __func__); + return -EINVAL; + }
This should probably be located in separate path - not at all an initialization removal.
It's a consequence of the initialization removal: because we are removing the default init, there is a risk that the loop just before do not set it, so it's required to trap the case where the variable in not initialized.
@@ -1573,6 +1578,11 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream) } } + if (!bus) { + pr_err("Configuration error in %s\n", __func__); + return -EINVAL; + }
Same here.
same reply
@@ -1639,13 +1650,14 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream) ret = do_bank_switch(stream); if (ret < 0) { - dev_err(bus->dev, "Bank switch failed: %d\n", ret); + pr_err("Bank switch failed: %d\n", ret); return ret; }
Here too.
no, same thing, the bus variable is initialized in loops so tools will report a possible path where bus->dev is an invalid dereference.
I might have missed something though I bet you got my point.
To help pass platform-specific values, add a new field that can either be set by the Master driver or read from firmware (BIOS/DT).
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- 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 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);
The BIOS provides an Intel-specific property, let's use it to avoid hard-coded clock dividers.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index fe3266bc1d12..3f876642cb7f 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -909,11 +909,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 = &bus->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", + &prop->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; }
Now that the prototype and Intel implementation are enabled, use this property to avoid hard-coded values.
For example for ICL the mclk_freq value is 38.4 MHz while on CNL/CML it's 24 MHz. The mclk_freq should not be confused with the max_clk_freq, which si the maximum bus clock. The mclk_freq is typically tied to the oscillator frequency and does not change between platforms. The max_clk_freq value is linked to the maximum bandwidth needed and topology/trace length.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 89c55e4bb72c..049ecfad2c00 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -175,8 +175,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_TIMEOUT 2000
@@ -922,7 +920,10 @@ static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols) */ int sdw_cdns_init(struct sdw_cdns *cdns) { + struct sdw_bus *bus = &cdns->bus; + struct sdw_master_prop *prop = &bus->prop; u32 val; + int divider; int ret;
/* Exit clock stop */ @@ -934,9 +935,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 |= CDNS_DEFAULT_CLK_DIVIDER; + val |= divider; cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val); + cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
/* * Frame shape changes after initialization have to be done @@ -984,6 +987,7 @@ EXPORT_SYMBOL(sdw_cdns_init);
int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params) { + struct sdw_master_prop *prop = &bus->prop; struct sdw_cdns *cdns = bus_to_cdns(bus); int mcp_clkctrl_off, mcp_clkctrl; int divider; @@ -993,7 +997,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;
From: Rander Wang rander.wang@linux.intel.com
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 rander.wang@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/cadence_master.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 049ecfad2c00..fb198a806efd 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -59,6 +59,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
@@ -936,10 +937,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);
/* * Frame shape changes after initialization have to be done @@ -989,7 +991,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params) { struct sdw_master_prop *prop = &bus->prop; struct sdw_cdns *cdns = bus_to_cdns(bus); - int mcp_clkctrl_off, mcp_clkctrl; + int mcp_clkctrl_off; int divider;
if (!params->curr_dr_freq) { @@ -1006,9 +1008,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params) else 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);
return 0; }
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 pierre-louis.bossart@linux.intel.com --- 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 3f876642cb7f..e702187c0609 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, @@ -914,7 +916,7 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus) struct sdw_master_prop *prop = &bus->prop; struct fwnode_handle *link; char name[32]; - int nval, i; + u32 quirk_mask;
/* Find master handle */ snprintf(name, sizeof(name), @@ -929,6 +931,14 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus) fwnode_property_read_u32(link, "intel-sdw-ip-clock", &prop->mclk_freq); + + fwnode_property_read_u32(link, + "intel-quirk-mask", + &quirk_mask); + + if (quirk_mask & SDW_INTEL_QUIRK_MASK_BUS_DISABLE) + prop->hw_disabled = true; + return 0; }
@@ -989,6 +999,12 @@ static int intel_probe(struct platform_device *pdev) goto err_master_reg; }
+ if (sdw->cdns.bus.prop.hw_disabled) { + dev_info(&pdev->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); @@ -1042,9 +1058,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(&sdw->cdns.bus);
return 0; diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index b6acc436ac80..a3c7448d5817 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);
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.
Even when the ACPI information is correct, it's useful to be able to only enable the links that need attention - mostly to filter out dynamic debug messages.
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 pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel_init.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 70637a0383d2..b74c2f144962 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -22,6 +22,10 @@ #define SDW_LINK_BASE 0x30000 #define SDW_LINK_SIZE 0x10000
+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; @@ -111,6 +115,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(&adev->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);
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 pierre-louis.bossart@linux.intel.com --- 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 fb198a806efd..4ab3174cbb04 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_CONFIG 0x0
#define CDNS_MCP_CONFIG_MCMD_RETRY GENMASK(27, 24) @@ -762,6 +766,9 @@ static int _cdns_enable_interrupt(struct sdw_cdns *cdns) */ mask |= CDNS_MCP_INT_IRQ;
+ if (interrupt_mask) /* parameter override */ + mask = interrupt_mask; + cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
return 0;
All DAI callbacks are in intel.c except for shutdown. Move and remove export symbol
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- 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 4ab3174cbb04..c5891c8a824e 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -1316,19 +1316,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 c0bf6ff00a44..d1022125f182 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -174,8 +174,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 e702187c0609..fe5f21edccfc 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -764,6 +764,19 @@ intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) return ret; }
+static 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) { @@ -779,14 +792,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, };
On 05-08-19, 19:55, Pierre-Louis Bossart wrote:
This series provides an update on the initial RFC. Debugfs and Intel updates will be provided in follow-up patches. The order of patches was changed since the RFC so detailed change logs are provided below.
Applied all except 14, which didnt apply, thanks
On 8/21/19 4:07 AM, Vinod Koul wrote:
On 05-08-19, 19:55, Pierre-Louis Bossart wrote:
This series provides an update on the initial RFC. Debugfs and Intel updates will be provided in follow-up patches. The order of patches was changed since the RFC so detailed change logs are provided below.
Applied all except 14, which didnt apply, thanks
yes, that's a miss, there was a debugfs line that created a dependency with the other patches that were not applied because of the lack of alignment between soundwire/fixes and soundwire/next. Will resubmit.
participants (3)
-
Cezary Rojewski -
Pierre-Louis Bossart -
Vinod Koul