mbox series

[v2,0/5] soundwire: add static port map support

Message ID 20210309141514.24744-1-srinivas.kandagatla@linaro.org
Headers show
Series soundwire: add static port map support | expand

Message

Srinivas Kandagatla March 9, 2021, 2:15 p.m. UTC 
In some cases, SoundWire device ports are statically mapped to Controller
ports during design, however there is no way to expose this information
to the controller. Controllers like Qualcomm ones use this info to setup
static bandwith parameters for those ports.

A generic port allocation is not possible in this cases!
This patch adds a new member m_port_map to SoundWire device so that
it can populate the static master port map and share it with controller
to be able to setup correct bandwidth parameters.

As a user of this feature this patchset also adds new bindings for
wsa881x smart speaker which has 4 ports which are statically mapped
to the 3 output and 1 input port of the controller.

Tested it on DB845c and SM8250 MTP.

thanks,
srini

Srinivas Kandagatla (5):
  soundwire: add static port mapping support
  soundwire: qcom: update port map allocation bit mask
  soundwire: qcom: add static port map support
  ASoC: dt-bindings: wsa881x: add bindings for port mapping
  ASoC: codecs: wsa881x: add static port map support

 .../bindings/sound/qcom,wsa881x.yaml          |  9 ++++++
 drivers/soundwire/qcom.c                      | 30 +++++++++++++++----
 include/linux/soundwire/sdw.h                 |  2 ++
 sound/soc/codecs/wsa881x.c                    |  5 ++++
 4 files changed, 40 insertions(+), 6 deletions(-)

Comments

Pierre-Louis Bossart March 9, 2021, 3:55 p.m. UTC | #1 
On 3/9/21 8:15 AM, Srinivas Kandagatla wrote:
> currently the internal bitmask used for allocating ports starts with offset 0.
> This is bit confusing as data port numbers on Qualcomm controller are valid
> from 1 to 14. So adjust this bit mask accordingly, this will also help while
> adding static port map support.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>   drivers/soundwire/qcom.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 6d22df01f354..f4f1c5f2af0b 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -519,7 +519,7 @@ static void qcom_swrm_stream_free_ports(struct qcom_swrm_ctrl *ctrl,
>   			port_mask = &ctrl->din_port_mask;
>   
>   		list_for_each_entry(p_rt, &m_rt->port_list, port_node)
> -			clear_bit(p_rt->num - 1, port_mask);
> +			clear_bit(p_rt->num, port_mask);
>   	}
>   
>   	mutex_unlock(&ctrl->port_lock);
> @@ -552,13 +552,13 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>   			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
>   				/* Port numbers start from 1 - 14*/
>   				pn = find_first_zero_bit(port_mask, maxport);
> -				if (pn > (maxport - 1)) {
> +				if (pn > (maxport)) {

nit-pick: useless parentheses

>   					dev_err(ctrl->dev, "All ports busy\n");
>   					ret = -EBUSY;
>   					goto err;
>   				}
>   				set_bit(pn, port_mask);
> -				pconfig[nports].num = pn + 1;
> +				pconfig[nports].num = pn;
>   				pconfig[nports].ch_mask = p_rt->ch_mask;
>   				nports++;
>   			}
> @@ -580,7 +580,7 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>   err:
>   	if (ret) {
>   		for (i = 0; i < nports; i++)
> -			clear_bit(pconfig[i].num - 1, port_mask);
> +			clear_bit(pconfig[i].num, port_mask);
>   	}
>   
>   	mutex_unlock(&ctrl->port_lock);
> @@ -754,6 +754,9 @@ static int qcom_swrm_get_port_config(struct qcom_swrm_ctrl *ctrl)
>   	ctrl->num_dout_ports = val;
>   
>   	nports = ctrl->num_dout_ports + ctrl->num_din_ports;
> +	/* port numbers are non zero, so mark port 0 */

mask?

> +	set_bit(0, &ctrl->dout_port_mask);
> +	set_bit(0, &ctrl->din_port_mask);
>   
>   	ret = of_property_read_u8_array(np, "qcom,ports-offset1",
>   					off1, nports);
>
Pierre-Louis Bossart March 9, 2021, 4:10 p.m. UTC | #2 
>   	list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
> @@ -473,8 +475,13 @@ static int qcom_swrm_compute_params(struct sdw_bus *bus)
>   		}
>   
>   		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> +			slave = s_rt->slave;
>   			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> -				pcfg = &ctrl->pconfig[i];
> +				m_port = slave->m_port_map[_rtp->num - 1];
> +				if (m_port)
> +					pcfg = &ctrl->pconfig[m_port - 1];
> +				else
> +					pcfg = &ctrl->pconfig[i];

Maybe add a short comment on port allocation, I had to think a bit to 
figure out why the -1 was required on both peripheral and manager but it 
is not below [1]

>   				p_rt->transport_params.port_num = p_rt->num;
>   				p_rt->transport_params.sample_interval =
>   					pcfg->si + 1;
> @@ -535,8 +542,10 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>   	struct sdw_master_runtime *m_rt;
>   	struct sdw_slave_runtime *s_rt;
>   	struct sdw_port_runtime *p_rt;
> +	struct sdw_slave *slave;
>   	unsigned long *port_mask;
>   	int i, maxport, pn, nports = 0, ret = 0;
> +	unsigned int m_port;
>   
>   	mutex_lock(&ctrl->port_lock);
>   	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> @@ -549,9 +558,15 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>   		}
>   
>   		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> +			slave = s_rt->slave;
>   			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> +				m_port = slave->m_port_map[p_rt->num - 1];
>   				/* Port numbers start from 1 - 14*/
> -				pn = find_first_zero_bit(port_mask, maxport);
> +				if (m_port)
> +					pn = m_port;
> +				else
> +					pn = find_first_zero_bit(port_mask, maxport);

[1]

> +
>   				if (pn > (maxport)) {
>   					dev_err(ctrl->dev, "All ports busy\n");
>   					ret = -EBUSY;
>
Srinivas Kandagatla March 11, 2021, 2:29 p.m. UTC | #3 
On 09/03/2021 16:10, Pierre-Louis Bossart wrote:
> 
> 
> 
>>       list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
>> @@ -473,8 +475,13 @@ static int qcom_swrm_compute_params(struct 
>> sdw_bus *bus)
>>           }
>>           list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
>> +            slave = s_rt->slave;
>>               list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
>> -                pcfg = &ctrl->pconfig[i];
>> +                m_port = slave->m_port_map[_rtp->num - 1];
>> +                if (m_port)
>> +                    pcfg = &ctrl->pconfig[m_port - 1];
>> +                else
>> +                    pcfg = &ctrl->pconfig[i];
> 
> Maybe add a short comment on port allocation, I had to think a bit to 
> figure out why the -1 was required on both peripheral and manager but it 
> is not below [1]

I agree, will add some comment here to explain the offsets correctly!

--srini

> 
>>                   p_rt->transport_params.port_num = p_rt->num;
>>                   p_rt->transport_params.sample_interval =
>>                       pcfg->si + 1;
>> @@ -535,8 +542,10 @@ static int qcom_swrm_stream_alloc_ports(struct 
>> qcom_swrm_ctrl *ctrl,
>>       struct sdw_master_runtime *m_rt;
>>       struct sdw_slave_runtime *s_rt;
>>       struct sdw_port_runtime *p_rt;
>> +    struct sdw_slave *slave;
>>       unsigned long *port_mask;
>>       int i, maxport, pn, nports = 0, ret = 0;
>> +    unsigned int m_port;
>>       mutex_lock(&ctrl->port_lock);
>>       list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>> @@ -549,9 +558,15 @@ static int qcom_swrm_stream_alloc_ports(struct 
>> qcom_swrm_ctrl *ctrl,
>>           }
>>           list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
>> +            slave = s_rt->slave;
>>               list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
>> +                m_port = slave->m_port_map[p_rt->num - 1];
>>                   /* Port numbers start from 1 - 14*/
>> -                pn = find_first_zero_bit(port_mask, maxport);
>> +                if (m_port)
>> +                    pn = m_port;
>> +                else
>> +                    pn = find_first_zero_bit(port_mask, maxport);
> 
> [1]
> 
>> +
>>                   if (pn > (maxport)) {
>>                       dev_err(ctrl->dev, "All ports busy\n");
>>                       ret = -EBUSY;
>>
Srinivas Kandagatla March 11, 2021, 2:29 p.m. UTC | #4 
On 09/03/2021 15:55, Pierre-Louis Bossart wrote:
> 
> 
> On 3/9/21 8:15 AM, Srinivas Kandagatla wrote:
>> currently the internal bitmask used for allocating ports starts with 
>> offset 0.
>> This is bit confusing as data port numbers on Qualcomm controller are 
>> valid
>> from 1 to 14. So adjust this bit mask accordingly, this will also help 
>> while
>> adding static port map support.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>   drivers/soundwire/qcom.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 6d22df01f354..f4f1c5f2af0b 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -519,7 +519,7 @@ static void qcom_swrm_stream_free_ports(struct 
>> qcom_swrm_ctrl *ctrl,
>>               port_mask = &ctrl->din_port_mask;
>>           list_for_each_entry(p_rt, &m_rt->port_list, port_node)
>> -            clear_bit(p_rt->num - 1, port_mask);
>> +            clear_bit(p_rt->num, port_mask);
>>       }
>>       mutex_unlock(&ctrl->port_lock);
>> @@ -552,13 +552,13 @@ static int qcom_swrm_stream_alloc_ports(struct 
>> qcom_swrm_ctrl *ctrl,
>>               list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
>>                   /* Port numbers start from 1 - 14*/
>>                   pn = find_first_zero_bit(port_mask, maxport);
>> -                if (pn > (maxport - 1)) {
>> +                if (pn > (maxport)) {
> 
> nit-pick: useless parentheses

I agree, will remove this and address the other comment too in next spin!

--srini
> 
>>                       dev_err(ctrl->dev, "All ports busy\n");
>>                       ret = -EBUSY;
>>                       goto err;
>>                   }
>>                   set_bit(pn, port_mask);
>> -                pconfig[nports].num = pn + 1;
>> +                pconfig[nports].num = pn;
>>                   pconfig[nports].ch_mask = p_rt->ch_mask;
>>                   nports++;
>>               }
>> @@ -580,7 +580,7 @@ static int qcom_swrm_stream_alloc_ports(struct 
>> qcom_swrm_ctrl *ctrl,
>>   err:
>>       if (ret) {
>>           for (i = 0; i < nports; i++)
>> -            clear_bit(pconfig[i].num - 1, port_mask);
>> +            clear_bit(pconfig[i].num, port_mask);
>>       }
>>       mutex_unlock(&ctrl->port_lock);
>> @@ -754,6 +754,9 @@ static int qcom_swrm_get_port_config(struct 
>> qcom_swrm_ctrl *ctrl)
>>       ctrl->num_dout_ports = val;
>>       nports = ctrl->num_dout_ports + ctrl->num_din_ports;
>> +    /* port numbers are non zero, so mark port 0 */
> 
> mask?
> 
>> +    set_bit(0, &ctrl->dout_port_mask);
>> +    set_bit(0, &ctrl->din_port_mask);
>>       ret = of_property_read_u8_array(np, "qcom,ports-offset1",
>>                       off1, nports);
>>