[PATCH 0/9] soundwire: use FIELD_{GET|PREP} in subsystem
Use the FIELD_{GET|PREP} in soundwire subsytem and remove the local SDW_REG_SHIFT(). This makes code IMO look much neater
Tested this on db845c board
Bard, can you please verify this on intel boards.
Vinod Koul (9): soundwire: define and use addr bit masks soundwire: bus: use FIELD_GET() soundwire: slave: use SDW_DISCO_LINK_ID() soundwire: stream: use FIELD_{GET|PREP} soundwire: qcom : use FIELD_{GET|PREP} soundwire: cadence: use FIELD_{GET|PREP} soundwire: intel: use FIELD_{GET|PREP} soundwire: intel_init: use FIELD_{GET|PREP} soundwire: remove SDW_REG_SHIFT()
drivers/soundwire/bus.c | 6 +-- drivers/soundwire/cadence_master.c | 53 ++++++++++--------------- drivers/soundwire/intel.c | 40 +++++++------------ drivers/soundwire/intel_init.c | 2 +- drivers/soundwire/qcom.c | 22 ++++------ drivers/soundwire/slave.c | 2 +- drivers/soundwire/stream.c | 13 +++--- include/linux/soundwire/sdw.h | 21 ++++++---- include/linux/soundwire/sdw_registers.h | 7 ---- 9 files changed, 67 insertions(+), 99 deletions(-)
Soundwire addr is a 52bit value encoding link, version, unique id, mfg id, part id and class id. Define bit masks for these and use FIELD_GET() to extract these fields.
Signed-off-by: Vinod Koul vkoul@kernel.org --- include/linux/soundwire/sdw.h | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 76052f12c9f7..c7462c333480 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -5,6 +5,7 @@ #define __SOUNDWIRE_H
#include <linux/mod_devicetable.h> +#include <linux/bitfield.h>
struct sdw_bus; struct sdw_slave; @@ -455,13 +456,19 @@ struct sdw_slave_id { * * The MIPI DisCo for SoundWire defines in addition the link_id as bits 51:48 */ - -#define SDW_DISCO_LINK_ID(adr) (((adr) >> 48) & GENMASK(3, 0)) -#define SDW_VERSION(adr) (((adr) >> 44) & GENMASK(3, 0)) -#define SDW_UNIQUE_ID(adr) (((adr) >> 40) & GENMASK(3, 0)) -#define SDW_MFG_ID(adr) (((adr) >> 24) & GENMASK(15, 0)) -#define SDW_PART_ID(adr) (((adr) >> 8) & GENMASK(15, 0)) -#define SDW_CLASS_ID(adr) ((adr) & GENMASK(7, 0)) +#define SDW_DISCO_LINK_ID_MASK GENMASK(51, 48) +#define SDW_VERSION_MASK GENMASK(47, 44) +#define SDW_UNIQUE_ID_MASK GENMASK(43, 40) +#define SDW_MFG_ID_MASK GENMASK(39, 24) +#define SDW_PART_ID_MASK GENMASK(23, 8) +#define SDW_CLASS_ID_MASK GENMASK(7, 0) + +#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, addr) +#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, addr) +#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, addr) +#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, addr) +#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, addr) +#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, addr)
/** * struct sdw_slave_intr_status - Slave interrupt status
Hi Vinod, This change to use FIELD_PREP/GET looks good, the code is indeed a lot clearer, but ...
+#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, addr) +#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, addr) +#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, addr) +#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, addr) +#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, addr) +#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, addr)
...our CI stopped on a compilation error with these macros. You will need the patch1 attached.
Patch 9 also introduces conflicts with the multi-link code (fix in patch2), so would you mind if we go first with the multi-link code, or defer patch9 for now?
Our validation for CML w/ RT700 is at: https://github.com/thesofproject/linux/pull/2404
We will also test on machines that are not in the CI farm and provide feedback.
Thanks -Pierre
On 28-08-20, 11:03, Pierre-Louis Bossart wrote:
Hi Vinod, This change to use FIELD_PREP/GET looks good, the code is indeed a lot clearer, but ...
+#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, addr) +#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, addr) +#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, addr) +#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, addr) +#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, addr) +#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, addr)
...our CI stopped on a compilation error with these macros. You will need the patch1 attached.
Aha, that looks wrong indeed, somehow my test & build for aarch64 and build for x86 didnt flag this, neither this kbuild-bot!
Have fixed it up now
Patch 9 also introduces conflicts with the multi-link code (fix in patch2), so would you mind if we go first with the multi-link code, or defer patch9 for now?
We can fix the series required while applying..
Our validation for CML w/ RT700 is at: https://github.com/thesofproject/linux/pull/2404
We will also test on machines that are not in the CI farm and provide feedback.
Thanks -Pierre
From 3aba5a7229c904664dacf1843f2e925585d4bd3e Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Date: Fri, 28 Aug 2020 10:45:22 -0500 Subject: [PATCH 1/2] fixup! soundwire: define and use addr bit masks
s/addr/adr
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
include/linux/soundwire/sdw.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 892bf4718bc3..ebfabab63ec9 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -472,12 +472,12 @@ struct sdw_slave_id { #define SDW_PART_ID_MASK GENMASK(23, 8) #define SDW_CLASS_ID_MASK GENMASK(7, 0)
-#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, addr) -#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, addr) -#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, addr) -#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, addr) -#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, addr) -#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, addr) +#define SDW_DISCO_LINK_ID(adr) FIELD_GET(SDW_DISCO_LINK_ID_MASK, adr) +#define SDW_VERSION(adr) FIELD_GET(SDW_VERSION_MASK, adr) +#define SDW_UNIQUE_ID(adr) FIELD_GET(SDW_UNIQUE_ID_MASK, adr) +#define SDW_MFG_ID(adr) FIELD_GET(SDW_MFG_ID_MASK, adr) +#define SDW_PART_ID(adr) FIELD_GET(SDW_PART_ID_MASK, adr) +#define SDW_CLASS_ID(adr) FIELD_GET(SDW_CLASS_ID_MASK, adr)
/**
- struct sdw_slave_intr_status - Slave interrupt status
-- 2.25.1
From f0280ed5dbe284df628e58c5afa1e61452cd5cb8 Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Date: Fri, 28 Aug 2020 10:51:52 -0500 Subject: [PATCH 2/2] soundwire: intel: use FIELD_PREP macro
Follow upstream changes.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/intel.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 566c7a99a5c1..20f111ce8a7a 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -381,10 +381,11 @@ static int intel_link_power_up(struct sdw_intel *sdw) link_control = intel_readl(shim, SDW_SHIM_LCTL);
/* only power-up enabled links */
spa_mask = sdw->link_res->link_mask <<SDW_REG_SHIFT(SDW_SHIM_LCTL_SPA_MASK);cpa_mask = sdw->link_res->link_mask <<SDW_REG_SHIFT(SDW_SHIM_LCTL_CPA_MASK);
spa_mask = FIELD_PREP(SDW_SHIM_LCTL_SPA_MASK,sdw->link_res->link_mask);cpa_mask = FIELD_PREP(SDW_SHIM_LCTL_CPA_MASK,sdw->link_res->link_mask);link_control |= spa_mask;
@@ -555,10 +556,11 @@ static int intel_link_power_down(struct sdw_intel *sdw) link_control = intel_readl(shim, SDW_SHIM_LCTL);
/* only power-down enabled links */
spa_mask = (~sdw->link_res->link_mask) <<SDW_REG_SHIFT(SDW_SHIM_LCTL_SPA_MASK);cpa_mask = sdw->link_res->link_mask <<SDW_REG_SHIFT(SDW_SHIM_LCTL_CPA_MASK);
spa_mask = FIELD_PREP(SDW_SHIM_LCTL_SPA_MASK,~sdw->link_res->link_mask);cpa_mask = FIELD_PREP(SDW_SHIM_LCTL_CPA_MASK,sdw->link_res->link_mask);link_control &= spa_mask;
-- 2.25.1
Patch 9 also introduces conflicts with the multi-link code (fix in patch2), so would you mind if we go first with the multi-link code, or defer patch9 for now?
We can fix the series required while applying..
It's not an issue with git, it's rather that this series is quite invasive and if you add it first it becomes quite hard to bisect/debug. In terms of risk management, it's safer to isolate contributions by groups and not mix them.
use FIELD_GET() in bus code to extract field values instead of open coding masks and shift operations.
Signed-off-by: Vinod Koul vkoul@kernel.org --- drivers/soundwire/bus.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index e6e0fb9a81b4..d808f0256ba0 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -347,8 +347,8 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave, return -EINVAL; }
- msg->addr_page1 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE1_MASK)); - msg->addr_page2 = (addr >> SDW_REG_SHIFT(SDW_SCP_ADDRPAGE2_MASK)); + msg->addr_page1 = FIELD_GET(SDW_SCP_ADDRPAGE1_MASK, addr); + msg->addr_page2 = FIELD_GET(SDW_SCP_ADDRPAGE2_MASK, addr); msg->addr |= BIT(15); msg->page = true;
@@ -1420,7 +1420,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) port = buf & SDW_SCP_INT1_PORT0_3;
/* To get port number corresponding to bits, shift it */ - port = port >> SDW_REG_SHIFT(SDW_SCP_INT1_PORT0_3); + port = FIELD_GET(SDW_SCP_INT1_PORT0_3, port); for_each_set_bit(bit, &port, 8) { sdw_handle_port_interrupt(slave, bit, &port_status[bit]);
use SDW_DISCO_LINK_ID() in slave code to extract field values instead of open coding masks and shift operations to extract link_id
Signed-off-by: Vinod Koul vkoul@kernel.org --- drivers/soundwire/slave.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index 0839445ee07b..72bf15322526 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -102,7 +102,7 @@ static bool find_slave(struct sdw_bus *bus, }
/* Extract link id from ADR, Bit 51 to 48 (included) */ - link_id = (addr >> 48) & GENMASK(3, 0); + link_id = SDW_DISCO_LINK_ID(addr);
/* Check for link_id match */ if (link_id != bus->link_id)
use FIELD_{GET|PREP} in stream code to get/set field values instead of open coding masks and shift operations.
Signed-off-by: Vinod Koul vkoul@kernel.org --- drivers/soundwire/stream.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 37290a799023..91fd300739ee 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -100,9 +100,7 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus, return ret;
/* Program DPN_SampleCtrl2 register */ - wbuf = (t_params->sample_interval - 1); - wbuf &= SDW_DPN_SAMPLECTRL_HIGH; - wbuf >>= SDW_REG_SHIFT(SDW_DPN_SAMPLECTRL_HIGH); + wbuf = FIELD_GET(SDW_DPN_SAMPLECTRL_HIGH, t_params->sample_interval - 1);
ret = sdw_write(slave, addr3, wbuf); if (ret < 0) { @@ -111,9 +109,8 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus, }
/* Program DPN_HCtrl register */ - wbuf = t_params->hstart; - wbuf <<= SDW_REG_SHIFT(SDW_DPN_HCTRL_HSTART); - wbuf |= t_params->hstop; + wbuf = FIELD_PREP(SDW_DPN_HCTRL_HSTART, t_params->hstart); + wbuf |= FIELD_PREP(SDW_DPN_HCTRL_HSTOP, t_params->hstop);
ret = sdw_write(slave, addr4, wbuf); if (ret < 0) @@ -157,8 +154,8 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus, }
/* Program DPN_PortCtrl register */ - wbuf = p_params->data_mode << SDW_REG_SHIFT(SDW_DPN_PORTCTRL_DATAMODE); - wbuf |= p_params->flow_mode; + wbuf = FIELD_PREP(SDW_DPN_PORTCTRL_DATAMODE, p_params->data_mode); + wbuf |= FIELD_PREP(SDW_DPN_PORTCTRL_FLOWMODE, p_params->flow_mode);
ret = sdw_update(s_rt->slave, addr1, 0xF, wbuf); if (ret < 0) {
use FIELD_{GET|PREP} in qcom driver to get/set field values instead of open coding masks and shift operations. Also, remove now unused register shift defines
Signed-off-by: Vinod Koul vkoul@kernel.org --- drivers/soundwire/qcom.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index 915c2cf0c274..dafa3f3dd1ab 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -43,13 +43,10 @@ #define SWRM_CMD_FIFO_RD_FIFO_ADDR 0x318 #define SWRM_ENUMERATOR_CFG_ADDR 0x500 #define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m) (0x101C + 0x40 * (m)) -#define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT 3 #define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK GENMASK(2, 0) #define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK GENMASK(7, 3) -#define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT 0 #define SWRM_MCP_CFG_ADDR 0x1048 #define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK GENMASK(21, 17) -#define SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT 0x11 #define SWRM_DEF_CMD_NO_PINGS 0x1f #define SWRM_MCP_STATUS 0x104C #define SWRM_MCP_STATUS_BANK_NUM_MASK BIT(0) @@ -284,8 +281,8 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl) u32 val;
/* Clear Rows and Cols */ - val = (SWRM_MAX_ROW_VAL << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT | - SWRM_MIN_COL_VAL << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT); + val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL); + val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MIN_COL_VAL);
ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
@@ -298,9 +295,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
/* Configure No pings */ ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR, &val); - val &= ~SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK; - val |= (SWRM_DEF_CMD_NO_PINGS << - SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_SHFT); + val |= FIELD_PREP(SWRM_MCP_CFG_MAX_NUM_OF_CMD_NO_PINGS_BMSK, SWRM_DEF_CMD_NO_PINGS); ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
/* Configure number of retries of a read/write cmd */ @@ -355,11 +350,8 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
ctrl->reg_read(ctrl, reg, &val);
- val &= ~SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK; - val &= ~SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK; - - val |= (SWRM_MAX_ROW_VAL << SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_SHFT | - SWRM_MAX_COL_VAL << SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_SHFT); + val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, SWRM_MAX_COL_VAL); + val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, SWRM_MAX_ROW_VAL);
return ctrl->reg_write(ctrl, reg, val); } @@ -693,8 +685,8 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
ctrl->reg_read(ctrl, SWRM_COMP_PARAMS, &val);
- ctrl->num_dout_ports = val & SWRM_COMP_PARAMS_DOUT_PORTS_MASK; - ctrl->num_din_ports = (val & SWRM_COMP_PARAMS_DIN_PORTS_MASK) >> 5; + ctrl->num_dout_ports = FIELD_GET(SWRM_COMP_PARAMS_DOUT_PORTS_MASK, val); + ctrl->num_din_ports = FIELD_GET(SWRM_COMP_PARAMS_DIN_PORTS_MASK, val);
ret = of_property_read_u32(np, "qcom,din-ports", &val); if (ret)
use FIELD_{GET|PREP} in cadence driver to get/set field values instead of open coding masks and shift operations.
Signed-off-by: Vinod Koul vkoul@kernel.org --- drivers/soundwire/cadence_master.c | 53 +++++++++++++----------------- 1 file changed, 22 insertions(+), 31 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 24eafe0aa1c3..ac9adf38ce94 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -417,8 +417,7 @@ cdns_fill_msg_resp(struct sdw_cdns *cdns,
/* fill response */ for (i = 0; i < count; i++) - msg->buf[i + offset] = cdns->response_buf[i] >> - SDW_REG_SHIFT(CDNS_MCP_RESP_RDATA); + msg->buf[i + offset] = FIELD_GET(CDNS_MCP_RESP_RDATA, cdns->response_buf[i]);
return SDW_CMD_OK; } @@ -441,14 +440,15 @@ _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, addr = msg->addr;
for (i = 0; i < count; i++) { - data = msg->dev_num << SDW_REG_SHIFT(CDNS_MCP_CMD_DEV_ADDR); - data |= cmd << SDW_REG_SHIFT(CDNS_MCP_CMD_COMMAND); - data |= addr++ << SDW_REG_SHIFT(CDNS_MCP_CMD_REG_ADDR_L); + data = FIELD_PREP(CDNS_MCP_CMD_DEV_ADDR, msg->dev_num); + data |= FIELD_PREP(CDNS_MCP_CMD_COMMAND, cmd); + data |= FIELD_PREP(CDNS_MCP_CMD_REG_ADDR_L, addr); + addr++;
if (msg->flags == SDW_MSG_FLAG_WRITE) data |= msg->buf[i + offset];
- data |= msg->ssp_sync << SDW_REG_SHIFT(CDNS_MCP_CMD_SSP_TAG); + data |= FIELD_PREP(CDNS_MCP_CMD_SSP_TAG, msg->ssp_sync); cdns_writel(cdns, base, data); base += CDNS_MCP_CMD_WORD_LEN; } @@ -483,12 +483,12 @@ cdns_program_scp_addr(struct sdw_cdns *cdns, struct sdw_msg *msg) cdns->msg_count = CDNS_SCP_RX_FIFOLEVEL; }
- data[0] = msg->dev_num << SDW_REG_SHIFT(CDNS_MCP_CMD_DEV_ADDR); - data[0] |= 0x3 << SDW_REG_SHIFT(CDNS_MCP_CMD_COMMAND); + data[0] = FIELD_PREP(CDNS_MCP_CMD_DEV_ADDR, msg->dev_num); + data[0] |= FIELD_PREP(CDNS_MCP_CMD_COMMAND, 0x3); data[1] = data[0];
- data[0] |= SDW_SCP_ADDRPAGE1 << SDW_REG_SHIFT(CDNS_MCP_CMD_REG_ADDR_L); - data[1] |= SDW_SCP_ADDRPAGE2 << SDW_REG_SHIFT(CDNS_MCP_CMD_REG_ADDR_L); + data[0] |= FIELD_PREP(CDNS_MCP_CMD_REG_ADDR_L, SDW_SCP_ADDRPAGE1); + data[1] |= FIELD_PREP(CDNS_MCP_CMD_REG_ADDR_L, SDW_SCP_ADDRPAGE2);
data[0] |= msg->addr_page1; data[1] |= msg->addr_page2; @@ -1043,7 +1043,7 @@ static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols) 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; + val = FIELD_PREP(CDNS_MCP_FRAME_SHAPE_ROW_OFFSET, r) | c;
return val; } @@ -1170,12 +1170,9 @@ static int cdns_port_params(struct sdw_bus *bus,
dpn_config = cdns_readl(cdns, dpn_config_off);
- dpn_config |= ((p_params->bps - 1) << - SDW_REG_SHIFT(CDNS_DPN_CONFIG_WL)); - dpn_config |= (p_params->flow_mode << - SDW_REG_SHIFT(CDNS_DPN_CONFIG_PORT_FLOW)); - dpn_config |= (p_params->data_mode << - SDW_REG_SHIFT(CDNS_DPN_CONFIG_PORT_DAT)); + dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_WL, (p_params->bps - 1)); + dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_FLOW, p_params->flow_mode); + dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_PORT_DAT, p_params->data_mode);
cdns_writel(cdns, dpn_config_off, dpn_config);
@@ -1212,23 +1209,17 @@ static int cdns_transport_params(struct sdw_bus *bus,
dpn_config = cdns_readl(cdns, dpn_config_off);
- dpn_config |= (t_params->blk_grp_ctrl << - SDW_REG_SHIFT(CDNS_DPN_CONFIG_BGC)); - dpn_config |= (t_params->blk_pkg_mode << - SDW_REG_SHIFT(CDNS_DPN_CONFIG_BPM)); + dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_BGC, t_params->blk_grp_ctrl); + dpn_config |= FIELD_PREP(CDNS_DPN_CONFIG_BPM, t_params->blk_pkg_mode); cdns_writel(cdns, dpn_config_off, dpn_config);
- dpn_offsetctrl |= (t_params->offset1 << - SDW_REG_SHIFT(CDNS_DPN_OFFSET_CTRL_1)); - dpn_offsetctrl |= (t_params->offset2 << - SDW_REG_SHIFT(CDNS_DPN_OFFSET_CTRL_2)); + dpn_offsetctrl |= FIELD_PREP(CDNS_DPN_OFFSET_CTRL_1, t_params->offset1); + dpn_offsetctrl |= FIELD_PREP(CDNS_DPN_OFFSET_CTRL_2, t_params->offset2); cdns_writel(cdns, dpn_offsetctrl_off, dpn_offsetctrl);
- dpn_hctrl |= (t_params->hstart << - SDW_REG_SHIFT(CDNS_DPN_HCTRL_HSTART)); - dpn_hctrl |= (t_params->hstop << SDW_REG_SHIFT(CDNS_DPN_HCTRL_HSTOP)); - dpn_hctrl |= (t_params->lane_ctrl << - SDW_REG_SHIFT(CDNS_DPN_HCTRL_LCTRL)); + dpn_hctrl |= FIELD_PREP(CDNS_DPN_HCTRL_HSTART, t_params->hstart); + dpn_hctrl |= FIELD_PREP(CDNS_DPN_HCTRL_HSTOP, t_params->hstop); + dpn_hctrl |= FIELD_PREP(CDNS_DPN_HCTRL_LCTRL, t_params->lane_ctrl);
cdns_writel(cdns, dpn_hctrl_off, dpn_hctrl); cdns_writel(cdns, dpn_samplectrl_off, (t_params->sample_interval - 1)); @@ -1534,7 +1525,7 @@ void sdw_cdns_config_stream(struct sdw_cdns *cdns,
val = pdi->num; val |= CDNS_PDI_CONFIG_SOFT_RESET; - val |= ((1 << ch) - 1) << SDW_REG_SHIFT(CDNS_PDI_CONFIG_CHANNEL); + val |= FIELD_PREP(CDNS_PDI_CONFIG_CHANNEL, (1 << ch) - 1); cdns_writel(cdns, CDNS_PDI_CONFIG(pdi->num), val); } EXPORT_SYMBOL(sdw_cdns_config_stream);
- val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
- val = FIELD_PREP(CDNS_MCP_FRAME_SHAPE_ROW_OFFSET, r) | c;
Confusion between shift and mask here.
Testing a fix now (attached), but may I suggest you use the SOF GitHub/Travis CI directly for any updates? You'll get an answer in 30mn for the CML RVP w/ SoundWire.
On 28-08-20, 13:13, Pierre-Louis Bossart wrote:
- val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
- val = FIELD_PREP(CDNS_MCP_FRAME_SHAPE_ROW_OFFSET, r) | c;
Confusion between shift and mask here.
thanks, yes I had doubts on this, folder the fix
Testing a fix now (attached), but may I suggest you use the SOF GitHub/Travis CI directly for any updates? You'll get an answer in 30mn for the CML RVP w/ SoundWire.
From 5d0ca63bee2c6e2456195499908ecdc8a7709238 Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Date: Fri, 28 Aug 2020 13:04:01 -0500 Subject: [PATCH] fixup! soundwire: cadence: use FIELD_{GET|PREP}
Fix confusion between shift and mask.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/cadence_master.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index b6796aa19aa8..5dd06483c835 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -58,7 +58,7 @@ MODULE_PARM_DESC(cdns_mcp_int_mask, "Cadence MCP IntMask"); #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_FRAME_SHAPE_ROW_MASK GENMASK(7, 3)
#define CDNS_MCP_CONFIG_UPDATE 0x18 #define CDNS_MCP_CONFIG_UPDATE_BIT BIT(0) @@ -1165,9 +1165,10 @@ static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols) int r;
r = sdw_find_row_index(n_rows);
- c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
- c = sdw_find_col_index(n_cols);
- val = FIELD_PREP(CDNS_MCP_FRAME_SHAPE_ROW_OFFSET, r) | c;
val = FIELD_PREP(CDNS_MCP_FRAME_SHAPE_ROW_MASK, r) |
FIELD_PREP(CDNS_MCP_FRAME_SHAPE_COL_MASK, c);return val;
}
2.25.1
use FIELD_{GET|PREP} in intel driver to get/set field values instead of open coding masks and shift operations.
Signed-off-by: Vinod Koul vkoul@kernel.org --- drivers/soundwire/intel.c | 40 ++++++++++++++------------------------- 1 file changed, 14 insertions(+), 26 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index ebca8ced59ec..8b3de114f3b3 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -324,8 +324,7 @@ static int intel_link_power_up(struct sdw_intel *sdw)
/* set SyncPRD period */ sync_reg = intel_readl(shim, SDW_SHIM_SYNC); - sync_reg |= (syncprd << - SDW_REG_SHIFT(SDW_SHIM_SYNC_SYNCPRD)); + sync_reg |= FIELD_PREP(SDW_SHIM_SYNC_SYNCPRD, syncprd);
/* Set SyncCPU bit */ sync_reg |= SDW_SHIM_SYNC_SYNCCPU; @@ -443,7 +442,7 @@ static int intel_shim_init(struct sdw_intel *sdw, bool clock_stop)
intel_shim_glue_to_master_ip(sdw);
- act |= 0x1 << SDW_REG_SHIFT(SDW_SHIM_CTMCTL_DOAIS); + act |= FIELD_PREP(SDW_SHIM_CTMCTL_DOAIS, 0x1); act |= SDW_SHIM_CTMCTL_DACTQE; act |= SDW_SHIM_CTMCTL_DODS; intel_writew(shim, SDW_SHIM_CTMCTL(link_id), act); @@ -568,12 +567,9 @@ static void intel_pdi_init(struct sdw_intel *sdw, /* PCM Stream Capability */ pcm_cap = intel_readw(shim, SDW_SHIM_PCMSCAP(link_id));
- config->pcm_bd = (pcm_cap & SDW_SHIM_PCMSCAP_BSS) >> - SDW_REG_SHIFT(SDW_SHIM_PCMSCAP_BSS); - config->pcm_in = (pcm_cap & SDW_SHIM_PCMSCAP_ISS) >> - SDW_REG_SHIFT(SDW_SHIM_PCMSCAP_ISS); - config->pcm_out = (pcm_cap & SDW_SHIM_PCMSCAP_OSS) >> - SDW_REG_SHIFT(SDW_SHIM_PCMSCAP_OSS); + config->pcm_bd = FIELD_GET(SDW_SHIM_PCMSCAP_BSS, pcm_cap); + config->pcm_in = FIELD_GET(SDW_SHIM_PCMSCAP_ISS, pcm_cap); + config->pcm_out = FIELD_GET(SDW_SHIM_PCMSCAP_OSS, pcm_cap);
dev_dbg(sdw->cdns.dev, "PCM cap bd:%d in:%d out:%d\n", config->pcm_bd, config->pcm_in, config->pcm_out); @@ -581,12 +577,9 @@ static void intel_pdi_init(struct sdw_intel *sdw, /* PDM Stream Capability */ pdm_cap = intel_readw(shim, SDW_SHIM_PDMSCAP(link_id));
- config->pdm_bd = (pdm_cap & SDW_SHIM_PDMSCAP_BSS) >> - SDW_REG_SHIFT(SDW_SHIM_PDMSCAP_BSS); - config->pdm_in = (pdm_cap & SDW_SHIM_PDMSCAP_ISS) >> - SDW_REG_SHIFT(SDW_SHIM_PDMSCAP_ISS); - config->pdm_out = (pdm_cap & SDW_SHIM_PDMSCAP_OSS) >> - SDW_REG_SHIFT(SDW_SHIM_PDMSCAP_OSS); + config->pdm_bd = FIELD_GET(SDW_SHIM_PDMSCAP_BSS, pdm_cap); + config->pdm_in = FIELD_GET(SDW_SHIM_PDMSCAP_ISS, pdm_cap); + config->pdm_out = FIELD_GET(SDW_SHIM_PDMSCAP_OSS, pdm_cap);
dev_dbg(sdw->cdns.dev, "PDM cap bd:%d in:%d out:%d\n", config->pdm_bd, config->pdm_in, config->pdm_out); @@ -613,8 +606,7 @@ intel_pdi_get_ch_cap(struct sdw_intel *sdw, unsigned int pdi_num, bool pcm)
} else { count = intel_readw(shim, SDW_SHIM_PDMSCAP(link_id)); - count = ((count & SDW_SHIM_PDMSCAP_CPSS) >> - SDW_REG_SHIFT(SDW_SHIM_PDMSCAP_CPSS)); + count = FIELD_GET(SDW_SHIM_PDMSCAP_CPSS, count); }
/* zero based values for channel count in register */ @@ -688,10 +680,9 @@ intel_pdi_shim_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi) else pdi_conf &= ~(SDW_SHIM_PCMSYCM_DIR);
- pdi_conf |= (pdi->intel_alh_id << - SDW_REG_SHIFT(SDW_SHIM_PCMSYCM_STREAM)); - pdi_conf |= (pdi->l_ch_num << SDW_REG_SHIFT(SDW_SHIM_PCMSYCM_LCHN)); - pdi_conf |= (pdi->h_ch_num << SDW_REG_SHIFT(SDW_SHIM_PCMSYCM_HCHN)); + pdi_conf |= FIELD_PREP(SDW_SHIM_PCMSYCM_STREAM, pdi->intel_alh_id); + pdi_conf |= FIELD_PREP(SDW_SHIM_PCMSYCM_LCHN, pdi->l_ch_num); + pdi_conf |= FIELD_PREP(SDW_SHIM_PCMSYCM_HCHN, pdi->h_ch_num);
intel_writew(shim, SDW_SHIM_PCMSYCHM(link_id, pdi->num), pdi_conf); } @@ -711,11 +702,8 @@ intel_pdi_alh_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi) /* Program Stream config ALH register */ conf = intel_readl(alh, SDW_ALH_STRMZCFG(pdi->intel_alh_id));
- conf |= (SDW_ALH_STRMZCFG_DMAT_VAL << - SDW_REG_SHIFT(SDW_ALH_STRMZCFG_DMAT)); - - conf |= ((pdi->ch_count - 1) << - SDW_REG_SHIFT(SDW_ALH_STRMZCFG_CHN)); + conf |= FIELD_PREP(SDW_ALH_STRMZCFG_DMAT, SDW_ALH_STRMZCFG_DMAT_VAL); + conf |= FIELD_PREP(SDW_ALH_STRMZCFG_CHN, pdi->ch_count - 1);
intel_writel(alh, SDW_ALH_STRMZCFG(pdi->intel_alh_id), conf); }
use FIELD_{GET|PREP} in intel_init driver to get/set field values instead of open coding masks and shift operations.
Signed-off-by: Vinod Koul vkoul@kernel.org --- drivers/soundwire/intel_init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index add46d8fc85c..66411bdf1832 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -382,7 +382,7 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, * Name(_ADR, 0x40000000), with bits 31..28 representing the * SoundWire link so filter accordingly */ - if ((adr & GENMASK(31, 28)) >> 28 != SDW_LINK_TYPE) + if (FIELD_GET(GENMASK(31, 28), adr) != SDW_LINK_TYPE) return AE_OK; /* keep going */
/* device found, stop namespace walk */
soundwire had defined SDW_REG_SHIFT to calculate shift values for bitmasks, but now that we have better things in bitfield.h, remove this.
Signed-off-by: Vinod Koul vkoul@kernel.org --- include/linux/soundwire/sdw_registers.h | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h index 5d3c271af7d1..f420e8059779 100644 --- a/include/linux/soundwire/sdw_registers.h +++ b/include/linux/soundwire/sdw_registers.h @@ -4,13 +4,6 @@ #ifndef __SDW_REGISTERS_H #define __SDW_REGISTERS_H
-/* - * typically we define register and shifts but if one observes carefully, - * the shift can be generated from MASKS using few bit primitaives like ffs - * etc, so we use that and avoid defining shifts - */ -#define SDW_REG_SHIFT(n) (ffs(n) - 1) - /* * SDW registers as defined by MIPI 1.2 Spec */
On 8/28/2020 3:20 PM, Vinod Koul wrote:
Use the FIELD_{GET|PREP} in soundwire subsytem and remove the local SDW_REG_SHIFT(). This makes code IMO look much neater
Tested this on db845c board
Bard, can you please verify this on intel boards.
Somehow it doesn't work on intel boards. I am still looking into it.
Vinod Koul (9): soundwire: define and use addr bit masks soundwire: bus: use FIELD_GET() soundwire: slave: use SDW_DISCO_LINK_ID() soundwire: stream: use FIELD_{GET|PREP} soundwire: qcom : use FIELD_{GET|PREP} soundwire: cadence: use FIELD_{GET|PREP} soundwire: intel: use FIELD_{GET|PREP} soundwire: intel_init: use FIELD_{GET|PREP} soundwire: remove SDW_REG_SHIFT()
drivers/soundwire/bus.c | 6 +-- drivers/soundwire/cadence_master.c | 53 ++++++++++--------------- drivers/soundwire/intel.c | 40 +++++++------------ drivers/soundwire/intel_init.c | 2 +- drivers/soundwire/qcom.c | 22 ++++------ drivers/soundwire/slave.c | 2 +- drivers/soundwire/stream.c | 13 +++--- include/linux/soundwire/sdw.h | 21 ++++++---- include/linux/soundwire/sdw_registers.h | 7 ---- 9 files changed, 67 insertions(+), 99 deletions(-)
participants (3)
-
Bard liao -
Pierre-Louis Bossart -
Vinod Koul