mbox series

[0/6] soundwire: qcom: various improvements

Message ID 20210129173248.5941-1-srinivas.kandagatla@linaro.org
Headers show
Series soundwire: qcom: various improvements | expand

Message

Srinivas Kandagatla Jan. 29, 2021, 5:32 p.m. UTC 
During testing SoundWire controller on SM8250 MTP, we found
few issues like all the interrupts are not handled,
all transport parameters are not read from device tree.

Other major issue was register read/writes which was interrupt based
was an overhead and puts lot of limitation on context it can be used from.

So this patchset add various improvements to the existing driver
to address above issues.

Tested it on SM8250 MTP with 2x WSA881x speakers, HeadPhones on
WCD938x via lpass-rx-macro and Analog MICs via lpass-tx-macro.
Also tested on DragonBoard DB845c with 2xWSA881x speakers.

Srinivas Kandagatla (6):
  soundwire: qcom: add support to missing transport params
  soundwire: qcom: extract version field
  soundwire: qcom: set continue execution flag for ignored commands
  soundwire: qcom: start the clock during initialization
  soundwire: qcom: update register read/write routine
  soundwire: qcom: add support to new interrupts

 drivers/soundwire/qcom.c | 471 ++++++++++++++++++++++++++++++---------
 1 file changed, 366 insertions(+), 105 deletions(-)

Comments

Pierre-Louis Bossart Jan. 29, 2021, 7:21 p.m. UTC | #1 
On 1/29/21 11:32 AM, Srinivas Kandagatla wrote:
> version 1.5.1 and higher IPs of this controller required to set
> continue execution on ingored command flag. This patch sets this flag.

typo: ignored.
Vinod Koul Feb. 1, 2021, 2:13 p.m. UTC | #2 
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> Some of the transport parameters derived from device tree
> are not fully parsed by the driver.
> 
> This patch adds support to parse those missing parameters.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/qcom.c | 107 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 103 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 6d22df01f354..36e273795cbe 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -54,7 +54,13 @@
>  #define SWRM_MCP_SLV_STATUS					0x1090
>  #define SWRM_MCP_SLV_STATUS_MASK				GENMASK(1, 0)
>  #define SWRM_DP_PORT_CTRL_BANK(n, m)	(0x1124 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DP_PORT_CTRL_2_BANK(n, m)	(0x1128 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DP_BLOCK_CTRL_1(n)		(0x112C + 0x100 * (n - 1))
> +#define SWRM_DP_BLOCK_CTRL2_BANK(n, m)	(0x1130 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DP_PORT_HCTRL_BANK(n, m)	(0x1134 + 0x100 * (n - 1) + 0x40 * m)
>  #define SWRM_DP_BLOCK_CTRL3_BANK(n, m)	(0x1138 + 0x100 * (n - 1) + 0x40 * m)
> +#define SWRM_DIN_DPn_PCM_PORT_CTRL(n)	(0x1054 + 0x100 * (n - 1))
> +
>  #define SWRM_DP_PORT_CTRL_EN_CHAN_SHFT				0x18
>  #define SWRM_DP_PORT_CTRL_OFFSET2_SHFT				0x10
>  #define SWRM_DP_PORT_CTRL_OFFSET1_SHFT				0x08
> @@ -73,12 +79,20 @@
>  #define QCOM_SDW_MAX_PORTS	14
>  #define DEFAULT_CLK_FREQ	9600000
>  #define SWRM_MAX_DAIS		0xF
> +#define SWR_INVALID_PARAM 0xFF
> +#define SWR_HSTOP_MAX_VAL 0xF
> +#define SWR_HSTART_MIN_VAL 0x0
>  
>  struct qcom_swrm_port_config {
>  	u8 si;
>  	u8 off1;
>  	u8 off2;
>  	u8 bp_mode;
> +	u8 hstart;
> +	u8 hstop;
> +	u8 word_length;
> +	u8 bgp_count;
> +	u8 lane_control;
>  };
>  
>  struct qcom_swrm_ctrl {
> @@ -396,7 +410,13 @@ static int qcom_swrm_port_params(struct sdw_bus *bus,
>  				 struct sdw_port_params *p_params,
>  				 unsigned int bank)
>  {
> -	/* TBD */
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +
> +	if (p_params->bps != SWR_INVALID_PARAM)
> +		return ctrl->reg_write(ctrl,
> +				       SWRM_DP_BLOCK_CTRL_1(p_params->num),
> +				       p_params->bps - 1);
> +
>  	return 0;
>  }
>  
> @@ -415,10 +435,32 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus,
>  
>  	ret = ctrl->reg_write(ctrl, reg, value);
>  
> -	if (!ret && params->blk_pkg_mode) {
> -		reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
> +	if (params->lane_ctrl != SWR_INVALID_PARAM) {
> +		reg = SWRM_DP_PORT_CTRL_2_BANK(params->port_num, bank);
> +		value = params->lane_ctrl;
> +		ret = ctrl->reg_write(ctrl, reg, value);
> +	}
>  
> -		ret = ctrl->reg_write(ctrl, reg, 1);
> +	if (params->blk_grp_ctrl != SWR_INVALID_PARAM) {
> +		reg = SWRM_DP_BLOCK_CTRL2_BANK(params->port_num, bank);
> +		value = params->blk_grp_ctrl;
> +		ret = ctrl->reg_write(ctrl, reg, value);
> +	}
> +
> +	if (params->hstart != SWR_INVALID_PARAM
> +			&& params->hstop != SWR_INVALID_PARAM) {
> +		reg = SWRM_DP_PORT_HCTRL_BANK(params->port_num, bank);
> +		value = (params->hstop << 4) | params->hstart;
> +		ret = ctrl->reg_write(ctrl, reg, value);
> +	} else {
> +		reg = SWRM_DP_PORT_HCTRL_BANK(params->port_num, bank);
> +		value = (SWR_HSTOP_MAX_VAL << 4) | SWR_HSTART_MIN_VAL;
> +		ret = ctrl->reg_write(ctrl, reg, value);
> +	}
> +
> +	if (params->blk_pkg_mode != SWR_INVALID_PARAM) {
> +		reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
> +		ret = ctrl->reg_write(ctrl, reg, params->blk_pkg_mode);
>  	}
>  
>  	return ret;
> @@ -470,6 +512,17 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
>  			p_rt->transport_params.offset1 = pcfg->off1;
>  			p_rt->transport_params.offset2 = pcfg->off2;
>  			p_rt->transport_params.blk_pkg_mode = pcfg->bp_mode;
> +			p_rt->transport_params.blk_grp_ctrl = pcfg->bgp_count;
> +			p_rt->transport_params.hstart = pcfg->hstart;
> +			p_rt->transport_params.hstop = pcfg->hstop;
> +			p_rt->transport_params.lane_ctrl = pcfg->lane_control;
> +			if (pcfg->word_length != SWR_INVALID_PARAM) {
> +				sdw_fill_port_params(&p_rt->port_params,
> +					     p_rt->num,  pcfg->word_length + 1,
> +					     SDW_PORT_FLOW_MODE_ISOCH,
> +					     SDW_PORT_DATA_MODE_NORMAL);
> +			}
> +
>  		}
>  
>  		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> @@ -481,6 +534,18 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
>  				p_rt->transport_params.offset1 = pcfg->off1;
>  				p_rt->transport_params.offset2 = pcfg->off2;
>  				p_rt->transport_params.blk_pkg_mode = pcfg->bp_mode;
> +				p_rt->transport_params.blk_grp_ctrl = pcfg->bgp_count;
> +
> +				p_rt->transport_params.hstart = pcfg->hstart;
> +				p_rt->transport_params.hstop = pcfg->hstop;
> +				p_rt->transport_params.lane_ctrl = pcfg->lane_control;
> +				if (pcfg->word_length != SWR_INVALID_PARAM) {
> +					sdw_fill_port_params(&p_rt->port_params,
> +						     p_rt->num,
> +						     pcfg->word_length + 1,
> +						     SDW_PORT_FLOW_MODE_ISOCH,
> +						     SDW_PORT_DATA_MODE_NORMAL);
> +				}
>  				i++;
>  			}
>  		}
> @@ -728,6 +793,11 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
>  	u8 off2[QCOM_SDW_MAX_PORTS];
>  	u8 si[QCOM_SDW_MAX_PORTS];
>  	u8 bp_mode[QCOM_SDW_MAX_PORTS] = { 0, };
> +	u8 hstart[QCOM_SDW_MAX_PORTS];
> +	u8 hstop[QCOM_SDW_MAX_PORTS];
> +	u8 word_length[QCOM_SDW_MAX_PORTS];
> +	u8 bgp_count[QCOM_SDW_MAX_PORTS];
> +	u8 lane_control[QCOM_SDW_MAX_PORTS];
>  	int i, ret, nports, val;
>  
>  	ctrl->reg_read(ctrl, SWRM_COMP_PARAMS, &val);
> @@ -772,11 +842,40 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
>  
>  	ret = of_property_read_u8_array(np, "qcom,ports-block-pack-mode",
>  					bp_mode, nports);
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-hstart", hstart, nports);
> +	if (ret)
> +		memset(hstart, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-hstop", hstop, nports);
> +	if (ret)
> +		memset(hstop, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);

why not memset the whole area here and then populate it..?
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-word-length",
> +					word_length, nports);

> +	if (ret)
> +		memset(word_length, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-block-group-count",
> +					bgp_count, nports);
> +	if (ret)
> +		memset(bgp_count, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> +
> +	ret = of_property_read_u8_array(np, "qcom,ports-lane-control",
> +					lane_control, nports);
> +	if (ret)
> +		memset(lane_control, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> +
>  	for (i = 0; i < nports; i++) {
>  		ctrl->pconfig[i].si = si[i];
>  		ctrl->pconfig[i].off1 = off1[i];
>  		ctrl->pconfig[i].off2 = off2[i];
>  		ctrl->pconfig[i].bp_mode = bp_mode[i];
> +		ctrl->pconfig[i].hstart = hstart[i];
> +		ctrl->pconfig[i].hstop = hstop[i];
> +		ctrl->pconfig[i].word_length = word_length[i];
> +		ctrl->pconfig[i].bgp_count = bgp_count[i];
> +		ctrl->pconfig[i].lane_control = lane_control[i];
Vinod Koul Feb. 1, 2021, 2:16 p.m. UTC | #3 
On 29-01-21, 17:32, Srinivas Kandagatla wrote:
> version 1.5.1 and higher IPs of this controller required to set
> continue execution on ingored command flag. This patch sets this flag.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/soundwire/qcom.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index da6e0d4e9622..3669bac11a32 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -40,6 +40,7 @@
>  #define SWRM_CMD_FIFO_CMD					0x308
>  #define SWRM_CMD_FIFO_STATUS					0x30C
>  #define SWRM_CMD_FIFO_CFG_ADDR					0x314
> +#define SWRM_CONTINUE_EXEC_ON_CMD_IGNORE			BIT(31)
>  #define SWRM_RD_WR_CMD_RETRIES					0x7
>  #define SWRM_CMD_FIFO_RD_FIFO_ADDR				0x318
>  #define SWRM_ENUMERATOR_CFG_ADDR				0x500
> @@ -345,7 +346,16 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>  	ctrl->reg_write(ctrl, SWRM_MCP_CFG_ADDR, val);
>  
>  	/* Configure number of retries of a read/write cmd */
> -	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, SWRM_RD_WR_CMD_RETRIES);
> +	if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
> +	    ctrl->version_step >= 1) {

why not use raw version value?

        if (ctrl->raw > 0x10501 )

> +		/* Only for versions >= 1.5.1 */
> +		ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR,
> +				SWRM_RD_WR_CMD_RETRIES |
> +				SWRM_CONTINUE_EXEC_ON_CMD_IGNORE);
> +	} else {
> +		ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR,
> +				SWRM_RD_WR_CMD_RETRIES);
> +	}
>  
>  	/* Set IRQ to PULSE */
>  	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,
> -- 
> 2.21.0
Srinivas Kandagatla Feb. 1, 2021, 3:50 p.m. UTC | #4 
On 29/01/2021 19:20, Pierre-Louis Bossart wrote:
> 
>>   struct qcom_swrm_port_config {
>>       u8 si;
>>       u8 off1;
>>       u8 off2;
>>       u8 bp_mode;
>> +    u8 hstart;
>> +    u8 hstop;
>> +    u8 word_length;
>> +    u8 bgp_count;
> 
> I couldn't figure out what 'bgp' was and had to search. Not sure how you 
> came up with this abbreviation of "qcom,ports-block-group-count". Adding 
> a comment wouldn't hurt.

I will rename this to blk_group_count which makes more sense!

> 
>> +    u8 lane_control;
> 
> Are you able to use lane_control != 0 ? I thought we were missing stuff 
> at the bus.c level?
Am not sure what is missing in bus.c but we do use lane_control for RX 
slave on WCD938x codec. This uses datalane 1 for HPH and lane0 for 
Compander/Class-H and other ports.

And it works!

--srini
>
Srinivas Kandagatla Feb. 1, 2021, 3:50 p.m. UTC | #5 
On 01/02/2021 14:13, Vinod Koul wrote:
>> +
>> +	ret = of_property_read_u8_array(np, "qcom,ports-hstop", hstop, nports);
>> +	if (ret)
>> +		memset(hstop, SWR_INVALID_PARAM, QCOM_SDW_MAX_PORTS);
> why not memset the whole area here and then populate it..?

That is other way to do it!, I can do that in next spin!


--srini
>> +
>> +	ret = of_property_read_u8_array(np, "qcom,ports-word-length",
>> +					word_length, nports);
Srinivas Kandagatla Feb. 1, 2021, 3:50 p.m. UTC | #6 
On 01/02/2021 14:16, Vinod Koul wrote:
>>   	/* Configure number of retries of a read/write cmd */
>> -	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, SWRM_RD_WR_CMD_RETRIES);
>> +	if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
>> +	    ctrl->version_step >= 1) {
> why not use raw version value?
> 
>          if (ctrl->raw > 0x10501 )
> 
We can do that way as well, but Major Minor seems much clear to readers!

--srini
Pierre-Louis Bossart Feb. 1, 2021, 4:33 p.m. UTC | #7 
>>>   struct qcom_swrm_port_config {
>>>       u8 si;
>>>       u8 off1;
>>>       u8 off2;
>>>       u8 bp_mode;
>>> +    u8 hstart;
>>> +    u8 hstop;
>>> +    u8 word_length;
>>> +    u8 bgp_count;
>>
>> I couldn't figure out what 'bgp' was and had to search. Not sure how 
>> you came up with this abbreviation of "qcom,ports-block-group-count". 
>> Adding a comment wouldn't hurt.
> 
> I will rename this to blk_group_count which makes more sense!

sounds good.


>>
>>> +    u8 lane_control;
>>
>> Are you able to use lane_control != 0 ? I thought we were missing 
>> stuff at the bus.c level?
> Am not sure what is missing in bus.c but we do use lane_control for RX 
> slave on WCD938x codec. This uses datalane 1 for HPH and lane0 for 
> Compander/Class-H and other ports.
> 
> And it works!

Ah, good to know, thanks for the pointer.
Vinod Koul Feb. 2, 2021, 4:46 a.m. UTC | #8 
On 01-02-21, 15:50, Srinivas Kandagatla wrote:
> 
> 
> On 01/02/2021 14:16, Vinod Koul wrote:
> > >   	/* Configure number of retries of a read/write cmd */
> > > -	ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CFG_ADDR, SWRM_RD_WR_CMD_RETRIES);
> > > +	if (ctrl->version_major == 1 && ctrl->version_minor >= 5 &&
> > > +	    ctrl->version_step >= 1) {
> > why not use raw version value?
> > 
> >          if (ctrl->raw > 0x10501 )
> > 
> We can do that way as well, but Major Minor seems much clear to readers!

yes but comparison with numbers is always easiest and better :) We can
always add comment that check version 1.5.1 which will make it clear