diff mbox series

[2/3] soundwire: qcom: add auto enumeration support

Message ID 20210226170250.9067-3-srinivas.kandagatla@linaro.org (mailing list archive)
State New, archived
Headers show
Series soundwire: qcom: add Clock Stop and Auto Enumeration support | expand

Commit Message

Srinivas Kandagatla Feb. 26, 2021, 5:02 p.m. UTC
Qualcomm SoundWire controller supports Auto Enumeration of the
devices within the IP. This patch enables support for this feature.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/qcom.c | 84 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 78 insertions(+), 6 deletions(-)

Comments

Pierre-Louis Bossart Feb. 26, 2021, 5:44 p.m. UTC | #1
> +static int qcom_swrm_enumerate(struct sdw_bus *bus)
> +{
> +	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	struct sdw_slave *slave, *_s;
> +	struct sdw_slave_id id;
> +	u32 val1, val2;
> +	u64 addr;
> +	int i;
> +	char *buf1 = (char *)&val1, *buf2 = (char *)&val2;
> +
> +	for (i = 1; i < (SDW_MAX_DEVICES + 1); i++) {

I don't understand the (SDW_MAX_DEVICES + 1)?


> +		/*SCP_Devid5 - Devid 4*/
> +		ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_1(i), &val1);
> +
> +		/*SCP_Devid3 - DevId 2 Devid 1 Devid 0*/
> +		ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_2(i), &val2);

Do you mind explaining a bit what happens here?
Does the hardware issue commands to read all DevID registers and set the 
device number automagically?
If yes, then in SoundWire parlance the enumeration is complete. What you 
are doing below is no longer part of the enumeration.


> +
> +		if (!val1 && !val2)
> +			break;
> +
> +		addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
> +			((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
> +			((u64)buf1[0] << 40);
> +
> +		sdw_extract_slave_id(bus, addr, &id);
> +		/* Now compare with entries */
> +		list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
> +			if (sdw_compare_devid(slave, id) == 0) {
> +				u32 status = qcom_swrm_get_n_device_status(ctrl, i);
> +				if (status == SDW_SLAVE_ATTACHED) {
> +					slave->dev_num = i;
> +					mutex_lock(&bus->bus_lock);
> +					set_bit(i, bus->assigned);
> +					mutex_unlock(&bus->bus_lock);
> +
> +				}

And that part is strange as well. The bus->assigned bit should be set 
even if the Slave is not in the list provided by platform firmware. It's 
really tracking the state of the hardware, and it should not be 
influenced by what software knows to manage.

> +				break;
> +			}
> +		}
> +	}
> +
> +	complete(&ctrl->enumeration);

you have init_completion() and complete() in this patch, but no 
wait_for_completion(), so that should be added in a later patch, no?

> +	return 0;
> +}
> +
>   static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   {
>   	struct qcom_swrm_ctrl *swrm = dev_id;
> -	u32 value, intr_sts, intr_sts_masked;
> +	u32 value, intr_sts, intr_sts_masked, slave_status;
>   	u32 i;
>   	u8 devnum = 0;
>   	int ret = IRQ_HANDLED;
> @@ -382,10 +443,19 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   				break;
>   			case SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED:
>   			case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
> -				dev_err_ratelimited(swrm->dev, "%s: SWR new slave attached\n",
> +				dev_err_ratelimited(swrm->dev, "%s: SWR slave status changed\n",
>   					__func__);
> -				qcom_swrm_get_device_status(swrm);
> -				sdw_handle_slave_status(&swrm->bus, swrm->status);
> +				swrm->reg_read(swrm, SWRM_MCP_SLV_STATUS, &slave_status);
> +				if (swrm->slave_status == slave_status) {
> +					dev_err(swrm->dev, "Slave status not changed %x\n",
> +						slave_status);
> +					break;
> +				} else {
> +					dev_err(swrm->dev, "Slave status handle %x\n", slave_status);
> +					qcom_swrm_get_device_status(swrm);
> +					qcom_swrm_enumerate(&swrm->bus);
> +					sdw_handle_slave_status(&swrm->bus, swrm->status);
> +				}
>   				break;
>   			case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET:
>   				dev_err_ratelimited(swrm->dev,
> @@ -472,8 +542,8 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>   
>   	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
>   
> -	/* Disable Auto enumeration */
> -	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
> +	/* Enable Auto enumeration */
> +	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 1);
>   
>   	ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK;
>   	/* Mask soundwire interrupts */
> @@ -507,6 +577,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>   		ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN,
>   				SWRM_INTERRUPT_STATUS_RMSK);
>   	}
> +	ctrl->slave_status = 0;
>   	return 0;
>   }
>   
> @@ -1068,6 +1139,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>   	dev_set_drvdata(&pdev->dev, ctrl);
>   	mutex_init(&ctrl->port_lock);
>   	init_completion(&ctrl->broadcast);
> +	init_completion(&ctrl->enumeration);
>   
>   	ctrl->bus.ops = &qcom_swrm_ops;
>   	ctrl->bus.port_ops = &qcom_swrm_port_ops;
>
Srinivas Kandagatla March 2, 2021, 10:33 a.m. UTC | #2
On 26/02/2021 17:44, Pierre-Louis Bossart wrote:
> 
>> +static int qcom_swrm_enumerate(struct sdw_bus *bus)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>> +    struct sdw_slave *slave, *_s;
>> +    struct sdw_slave_id id;
>> +    u32 val1, val2;
>> +    u64 addr;
>> +    int i;
>> +    char *buf1 = (char *)&val1, *buf2 = (char *)&val2;
>> +
>> +    for (i = 1; i < (SDW_MAX_DEVICES + 1); i++) {
> 
> I don't understand the (SDW_MAX_DEVICES + 1)?
If you mean +1 then probably we can add <= instead of just < to make it 
look like other parts of code in bus.c

for (i = 1; i <= SDW_MAX_DEVICES; i++)

> 
> 
>> +        /*SCP_Devid5 - Devid 4*/
>> +        ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_1(i), &val1);
>> +
>> +        /*SCP_Devid3 - DevId 2 Devid 1 Devid 0*/
>> +        ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_2(i), &val2);
> 
> Do you mind explaining a bit what happens here?
> Does the hardware issue commands to read all DevID registers and set the 
> device number automagically?

exactly the hardware assigns device numbers to slaves automatically, and 
the devnum can be figured out by the controller driver by reading 
SWRM_ENUMERATOR_SLAVE_DEV_ID_1/2 registers!

> If yes, then in SoundWire parlance the enumeration is complete. What you 
> are doing below is no longer part of the enumeration.

yes, enumeration is complete by the hardware, however the controller 
driver need to know the dev number assigned by the hardware, this 
routine is doing that!
> 
> 
>> +
>> +        if (!val1 && !val2)
>> +            break;
>> +
>> +        addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
>> +            ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
>> +            ((u64)buf1[0] << 40);
>> +
>> +        sdw_extract_slave_id(bus, addr, &id);
>> +        /* Now compare with entries */
>> +        list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>> +            if (sdw_compare_devid(slave, id) == 0) {
>> +                u32 status = qcom_swrm_get_n_device_status(ctrl, i);
>> +                if (status == SDW_SLAVE_ATTACHED) {
>> +                    slave->dev_num = i;
>> +                    mutex_lock(&bus->bus_lock);
>> +                    set_bit(i, bus->assigned);
>> +                    mutex_unlock(&bus->bus_lock);
>> +
>> +                }
> 
> And that part is strange as well. The bus->assigned bit should be set 
> even if the Slave is not in the list provided by platform firmware. It's 
> really tracking the state of the hardware, and it should not be 
> influenced by what software knows to manage.

Am not 100% sure If I understand the concern here, but In normal (non 
auto enum) cases this bit is set by the bus code while its doing 
enumeration to assign a dev number from the assigned bitmap!

However in this case where auto enumeration happens it makes sense to 
set this here with matching dev number!

AFAIU from code, each bit in this bitmap corresponds to slave dev number!


> 
>> +                break;
>> +            }
>> +        }
>> +    }
>> +
>> +    complete(&ctrl->enumeration);
> 
> you have init_completion() and complete() in this patch, but no 
> wait_for_completion(), so that should be added in a later patch, no?

make sense, will move that to other patch!

--srini
> 
>> +    return 0;
>> +}
>> +
>>   static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>>   {
>>       struct qcom_swrm_ctrl *swrm = dev_id;
>> -    u32 value, intr_sts, intr_sts_masked;
>> +    u32 value, intr_sts, intr_sts_masked, slave_status;
>>       u32 i;
>>       u8 devnum = 0;
>>       int ret = IRQ_HANDLED;
>> @@ -382,10 +443,19 @@ static irqreturn_t qcom_swrm_irq_handler(int 
>> irq, void *dev_id)
>>                   break;
>>               case SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED:
>>               case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
>> -                dev_err_ratelimited(swrm->dev, "%s: SWR new slave 
>> attached\n",
>> +                dev_err_ratelimited(swrm->dev, "%s: SWR slave status 
>> changed\n",
>>                       __func__);
>> -                qcom_swrm_get_device_status(swrm);
>> -                sdw_handle_slave_status(&swrm->bus, swrm->status);
>> +                swrm->reg_read(swrm, SWRM_MCP_SLV_STATUS, 
>> &slave_status);
>> +                if (swrm->slave_status == slave_status) {
>> +                    dev_err(swrm->dev, "Slave status not changed %x\n",
>> +                        slave_status);
>> +                    break;
>> +                } else {
>> +                    dev_err(swrm->dev, "Slave status handle %x\n", 
>> slave_status);
>> +                    qcom_swrm_get_device_status(swrm);
>> +                    qcom_swrm_enumerate(&swrm->bus);
>> +                    sdw_handle_slave_status(&swrm->bus, swrm->status);
>> +                }
>>                   break;
>>               case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET:
>>                   dev_err_ratelimited(swrm->dev,
>> @@ -472,8 +542,8 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl 
>> *ctrl)
>>       ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
>> -    /* Disable Auto enumeration */
>> -    ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
>> +    /* Enable Auto enumeration */
>> +    ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 1);
>>       ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK;
>>       /* Mask soundwire interrupts */
>> @@ -507,6 +577,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl 
>> *ctrl)
>>           ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN,
>>                   SWRM_INTERRUPT_STATUS_RMSK);
>>       }
>> +    ctrl->slave_status = 0;
>>       return 0;
>>   }
>> @@ -1068,6 +1139,7 @@ static int qcom_swrm_probe(struct 
>> platform_device *pdev)
>>       dev_set_drvdata(&pdev->dev, ctrl);
>>       mutex_init(&ctrl->port_lock);
>>       init_completion(&ctrl->broadcast);
>> +    init_completion(&ctrl->enumeration);
>>       ctrl->bus.ops = &qcom_swrm_ops;
>>       ctrl->bus.port_ops = &qcom_swrm_port_ops;
>>
Pierre-Louis Bossart March 2, 2021, 2:34 p.m. UTC | #3
>>> +        if (!val1 && !val2)
>>> +            break;
>>> +
>>> +        addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
>>> +            ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
>>> +            ((u64)buf1[0] << 40);
>>> +
>>> +        sdw_extract_slave_id(bus, addr, &id);
>>> +        /* Now compare with entries */
>>> +        list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>>> +            if (sdw_compare_devid(slave, id) == 0) {
>>> +                u32 status = qcom_swrm_get_n_device_status(ctrl, i);
>>> +                if (status == SDW_SLAVE_ATTACHED) {
>>> +                    slave->dev_num = i;
>>> +                    mutex_lock(&bus->bus_lock);
>>> +                    set_bit(i, bus->assigned);
>>> +                    mutex_unlock(&bus->bus_lock);
>>> +
>>> +                }
>>
>> And that part is strange as well. The bus->assigned bit should be set 
>> even if the Slave is not in the list provided by platform firmware. 
>> It's really tracking the state of the hardware, and it should not be 
>> influenced by what software knows to manage.
> 
> Am not 100% sure If I understand the concern here, but In normal (non 
> auto enum) cases this bit is set by the bus code while its doing 
> enumeration to assign a dev number from the assigned bitmap!
> 
> However in this case where auto enumeration happens it makes sense to 
> set this here with matching dev number!
> 
> AFAIU from code, each bit in this bitmap corresponds to slave dev number!

Yes, but the point was "why do you compare with information coming from 
platform firmware"? if the hardware reports the presence of devices on 
the link, why not use the information as is?

You recently added code that helps us deal with devices that are not 
listed in DT or ACPI tables, so why would we filter in this specific loop?

>>> +    complete(&ctrl->enumeration);
>>
>> you have init_completion() and complete() in this patch, but no 
>> wait_for_completion(), so that should be added in a later patch, no?
> 
> make sense, will move that to other patch!

Actually on this one comment that I missed last time is that you are 
using a completion only for the resume() case, and I think it should 
also be used for the regular probe() case, no?
Srinivas Kandagatla March 3, 2021, 9:38 a.m. UTC | #4
On 02/03/2021 14:34, Pierre-Louis Bossart wrote:
> 
> 
>>>> +        if (!val1 && !val2)
>>>> +            break;
>>>> +
>>>> +        addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
>>>> +            ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
>>>> +            ((u64)buf1[0] << 40);
>>>> +
>>>> +        sdw_extract_slave_id(bus, addr, &id);
>>>> +        /* Now compare with entries */
>>>> +        list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>>>> +            if (sdw_compare_devid(slave, id) == 0) {
>>>> +                u32 status = qcom_swrm_get_n_device_status(ctrl, i);
>>>> +                if (status == SDW_SLAVE_ATTACHED) {
>>>> +                    slave->dev_num = i;
>>>> +                    mutex_lock(&bus->bus_lock);
>>>> +                    set_bit(i, bus->assigned);
>>>> +                    mutex_unlock(&bus->bus_lock);
>>>> +
>>>> +                }
>>>
>>> And that part is strange as well. The bus->assigned bit should be set 
>>> even if the Slave is not in the list provided by platform firmware. 
>>> It's really tracking the state of the hardware, and it should not be 
>>> influenced by what software knows to manage.
>>
>> Am not 100% sure If I understand the concern here, but In normal (non 
>> auto enum) cases this bit is set by the bus code while its doing 
>> enumeration to assign a dev number from the assigned bitmap!
>>
>> However in this case where auto enumeration happens it makes sense to 
>> set this here with matching dev number!
>>
>> AFAIU from code, each bit in this bitmap corresponds to slave dev number!
> 
> Yes, but the point was "why do you compare with information coming from 
> platform firmware"? if the hardware reports the presence of devices on 

This is the logic that hardware IP document suggests to use to get get 
the correct the device number associated with the slave!


> the link, why not use the information as is?
> 
> You recently added code that helps us deal with devices that are not 
> listed in DT or ACPI tables, so why would we filter in this specific loop?
> 
>>>> +    complete(&ctrl->enumeration);
>>>
>>> you have init_completion() and complete() in this patch, but no 
>>> wait_for_completion(), so that should be added in a later patch, no?
>>
>> make sense, will move that to other patch!
> 
> Actually on this one comment that I missed last time is that you are 
> using a completion only for the resume() case, and I think it should 
> also be used for the regular probe() case, no?
Good Idea, I can try that and see how to works out!

--srini
>
Pierre-Louis Bossart March 3, 2021, 4:35 p.m. UTC | #5
On 3/3/21 3:38 AM, Srinivas Kandagatla wrote:
> 
> 
> On 02/03/2021 14:34, Pierre-Louis Bossart wrote:
>>
>>
>>>>> +        if (!val1 && !val2)
>>>>> +            break;
>>>>> +
>>>>> +        addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
>>>>> +            ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
>>>>> +            ((u64)buf1[0] << 40);
>>>>> +
>>>>> +        sdw_extract_slave_id(bus, addr, &id);
>>>>> +        /* Now compare with entries */
>>>>> +        list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>>>>> +            if (sdw_compare_devid(slave, id) == 0) {
>>>>> +                u32 status = qcom_swrm_get_n_device_status(ctrl, i);
>>>>> +                if (status == SDW_SLAVE_ATTACHED) {
>>>>> +                    slave->dev_num = i;
>>>>> +                    mutex_lock(&bus->bus_lock);
>>>>> +                    set_bit(i, bus->assigned);
>>>>> +                    mutex_unlock(&bus->bus_lock);
>>>>> +
>>>>> +                }
>>>>
>>>> And that part is strange as well. The bus->assigned bit should be 
>>>> set even if the Slave is not in the list provided by platform 
>>>> firmware. It's really tracking the state of the hardware, and it 
>>>> should not be influenced by what software knows to manage.
>>>
>>> Am not 100% sure If I understand the concern here, but In normal (non 
>>> auto enum) cases this bit is set by the bus code while its doing 
>>> enumeration to assign a dev number from the assigned bitmap!
>>>
>>> However in this case where auto enumeration happens it makes sense to 
>>> set this here with matching dev number!
>>>
>>> AFAIU from code, each bit in this bitmap corresponds to slave dev 
>>> number!
>>
>> Yes, but the point was "why do you compare with information coming 
>> from platform firmware"? if the hardware reports the presence of 
>> devices on 
> 
> This is the logic that hardware IP document suggests to use to get get 
> the correct the device number associated with the slave!
> 
> 
>> the link, why not use the information as is?
>>
>> You recently added code that helps us deal with devices that are not 
>> listed in DT or ACPI tables, so why would we filter in this specific 
>> loop?

I don't think my point was understood, so let me try to explain it 
differently.

it's my understanding that the hardware reads the DevID registers and 
writes a Device Number - so that the entire enumeration sequence started 
by reading DevID0 and finished by a successful write to SCP_DevNum is 
handled in hardware.

The question is: what happens if that device is NOT described in the 
Device Tree data? The loop over bus->slaves will not find this device by 
comparing with known devID values, so the set_bit(i, bus->assigned) will 
not happen.
Srinivas Kandagatla March 5, 2021, 10:39 a.m. UTC | #6
On 03/03/2021 16:35, Pierre-Louis Bossart wrote:
> 
> 
> On 3/3/21 3:38 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 02/03/2021 14:34, Pierre-Louis Bossart wrote:
>>>
>>>
>>>>>> +        if (!val1 && !val2)
>>>>>> +            break;
>>>>>> +
>>>>>> +        addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
>>>>>> +            ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
>>>>>> +            ((u64)buf1[0] << 40);
>>>>>> +
>>>>>> +        sdw_extract_slave_id(bus, addr, &id);
>>>>>> +        /* Now compare with entries */
>>>>>> +        list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>>>>>> +            if (sdw_compare_devid(slave, id) == 0) {
>>>>>> +                u32 status = qcom_swrm_get_n_device_status(ctrl, i);
>>>>>> +                if (status == SDW_SLAVE_ATTACHED) {
>>>>>> +                    slave->dev_num = i;
>>>>>> +                    mutex_lock(&bus->bus_lock);
>>>>>> +                    set_bit(i, bus->assigned);
>>>>>> +                    mutex_unlock(&bus->bus_lock);
>>>>>> +
>>>>>> +                }
>>>>>
>>>>> And that part is strange as well. The bus->assigned bit should be 
>>>>> set even if the Slave is not in the list provided by platform 
>>>>> firmware. It's really tracking the state of the hardware, and it 
>>>>> should not be influenced by what software knows to manage.
>>>>
>>>> Am not 100% sure If I understand the concern here, but In normal 
>>>> (non auto enum) cases this bit is set by the bus code while its 
>>>> doing enumeration to assign a dev number from the assigned bitmap!
>>>>
>>>> However in this case where auto enumeration happens it makes sense 
>>>> to set this here with matching dev number!
>>>>
>>>> AFAIU from code, each bit in this bitmap corresponds to slave dev 
>>>> number!
>>>
>>> Yes, but the point was "why do you compare with information coming 
>>> from platform firmware"? if the hardware reports the presence of 
>>> devices on 
>>
>> This is the logic that hardware IP document suggests to use to get get 
>> the correct the device number associated with the slave!
>>
>>
>>> the link, why not use the information as is?
>>>
>>> You recently added code that helps us deal with devices that are not 
>>> listed in DT or ACPI tables, so why would we filter in this specific 
>>> loop?
> 
> I don't think my point was understood, so let me try to explain it 
> differently.
> 
> it's my understanding that the hardware reads the DevID registers and 
> writes a Device Number - so that the entire enumeration sequence started 
> by reading DevID0 and finished by a successful write to SCP_DevNum is 
> handled in hardware.
> 
> The question is: what happens if that device is NOT described in the 
> Device Tree data? The loop over bus->slaves will not find this device by 
> comparing with known devID values, so the set_bit(i, bus->assigned) will 
> not happen.

yes, that is true, There is no way we can assign a dev_number to the 
device which is not enumerated on the bus!

Am sure this is the same behavior with soundwire core too, atleast form 
the code I can see it sets the assigned bit for only the devices that 
are enumerated on the bus! Not all the devices specified in DT!
Unless I missed something!

--srini


--srini
> 
>
Pierre-Louis Bossart March 5, 2021, 4:19 p.m. UTC | #7
>>>>>>> +        if (!val1 && !val2)
>>>>>>> +            break;
>>>>>>> +
>>>>>>> +        addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
>>>>>>> +            ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
>>>>>>> +            ((u64)buf1[0] << 40);
>>>>>>> +
>>>>>>> +        sdw_extract_slave_id(bus, addr, &id);
>>>>>>> +        /* Now compare with entries */
>>>>>>> +        list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>>>>>>> +            if (sdw_compare_devid(slave, id) == 0) {
>>>>>>> +                u32 status = qcom_swrm_get_n_device_status(ctrl, 
>>>>>>> i);
>>>>>>> +                if (status == SDW_SLAVE_ATTACHED) {
>>>>>>> +                    slave->dev_num = i;
>>>>>>> +                    mutex_lock(&bus->bus_lock);
>>>>>>> +                    set_bit(i, bus->assigned);
>>>>>>> +                    mutex_unlock(&bus->bus_lock);
>>>>>>> +
>>>>>>> +                }
>>>>>>
>>>>>> And that part is strange as well. The bus->assigned bit should be 
>>>>>> set even if the Slave is not in the list provided by platform 
>>>>>> firmware. It's really tracking the state of the hardware, and it 
>>>>>> should not be influenced by what software knows to manage.
>>>>>
>>>>> Am not 100% sure If I understand the concern here, but In normal 
>>>>> (non auto enum) cases this bit is set by the bus code while its 
>>>>> doing enumeration to assign a dev number from the assigned bitmap!
>>>>>
>>>>> However in this case where auto enumeration happens it makes sense 
>>>>> to set this here with matching dev number!
>>>>>
>>>>> AFAIU from code, each bit in this bitmap corresponds to slave dev 
>>>>> number!
>>>>
>>>> Yes, but the point was "why do you compare with information coming 
>>>> from platform firmware"? if the hardware reports the presence of 
>>>> devices on 
>>>
>>> This is the logic that hardware IP document suggests to use to get 
>>> get the correct the device number associated with the slave!
>>>
>>>
>>>> the link, why not use the information as is?
>>>>
>>>> You recently added code that helps us deal with devices that are not 
>>>> listed in DT or ACPI tables, so why would we filter in this specific 
>>>> loop?
>>
>> I don't think my point was understood, so let me try to explain it 
>> differently.
>>
>> it's my understanding that the hardware reads the DevID registers and 
>> writes a Device Number - so that the entire enumeration sequence 
>> started by reading DevID0 and finished by a successful write to 
>> SCP_DevNum is handled in hardware.
>>
>> The question is: what happens if that device is NOT described in the 
>> Device Tree data? The loop over bus->slaves will not find this device 
>> by comparing with known devID values, so the set_bit(i, bus->assigned) 
>> will not happen.
> 
> yes, that is true, There is no way we can assign a dev_number to the 
> device which is not enumerated on the bus!
> 
> Am sure this is the same behavior with soundwire core too, atleast form 
> the code I can see it sets the assigned bit for only the devices that 
> are enumerated on the bus! Not all the devices specified in DT!
> Unless I missed something!

I am talking about the other way around, where a device is present and 
enumerated on the bus but not listed in DT. In that case the hardware 
did assign a device number but bus->assigned will not be set.
Srinivas Kandagatla March 5, 2021, 4:57 p.m. UTC | #8
On 05/03/2021 16:19, Pierre-Louis Bossart wrote:
>>>
>>>
>>> The question is: what happens if that device is NOT described in the 
>>> Device Tree data? The loop over bus->slaves will not find this device 
>>> by comparing with known devID values, so the set_bit(i, 
>>> bus->assigned) will not happen.
>>
>> yes, that is true, There is no way we can assign a dev_number to the 
>> device which is not enumerated on the bus!
>>
>> Am sure this is the same behavior with soundwire core too, atleast 
>> form the code I can see it sets the assigned bit for only the devices 
>> that are enumerated on the bus! Not all the devices specified in DT!
>> Unless I missed something!
> 
> I am talking about the other way around, where a device is present and 
> enumerated on the bus but not listed in DT. In that case the hardware 
> did assign a device number but bus->assigned will not be set.

thanks for your patience!

Ah, I understand it now!, yes that part is missing!

adding Something like what core does in qcom driver should fix it!

if (!found) {
	sdw_slave_add(bus, &id, NULL);
	dev_err(bus->dev, "Slave Entry not found\n");
}

--srini
diff mbox series

Patch

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 8438f9812d7c..d90eba6a1e88 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -57,6 +57,8 @@ 
 #define SWRM_CMD_FIFO_RD_FIFO_ADDR				0x318
 #define SWRM_RD_FIFO_CMD_ID_MASK				GENMASK(11, 8)
 #define SWRM_ENUMERATOR_CFG_ADDR				0x500
+#define SWRM_ENUMERATOR_SLAVE_DEV_ID_1(m)		(0x530 + 0x8 * (m))
+#define SWRM_ENUMERATOR_SLAVE_DEV_ID_2(m)   		(0x534 + 0x8 * (m))
 #define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m)		(0x101C + 0x40 * (m))
 #define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK			GENMASK(2, 0)
 #define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK			GENMASK(7, 3)
@@ -121,6 +123,7 @@  struct qcom_swrm_ctrl {
 	struct regmap *regmap;
 	void __iomem *mmio;
 	struct completion broadcast;
+	struct completion enumeration;
 	struct work_struct slave_work;
 	/* Port alloc/free lock */
 	struct mutex port_lock;
@@ -143,6 +146,7 @@  struct qcom_swrm_ctrl {
 	enum sdw_slave_status status[SDW_MAX_DEVICES];
 	int (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg, u32 *val);
 	int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val);
+	u32 slave_status;
 };
 
 struct qcom_swrm_data {
@@ -342,6 +346,7 @@  static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
 	int i;
 
 	ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
+	ctrl->slave_status = val;
 
 	for (i = 0; i < SDW_MAX_DEVICES; i++) {
 		u32 s;
@@ -352,10 +357,66 @@  static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl)
 	}
 }
 
+static int qcom_swrm_get_n_device_status(struct qcom_swrm_ctrl *ctrl, int devnum)
+{
+	u32 val;
+
+	ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val);
+	val = (val >> (devnum * SWRM_MCP_SLV_STATUS_SZ));
+	val &= SWRM_MCP_SLV_STATUS_MASK;
+
+	return val;
+}
+
+static int qcom_swrm_enumerate(struct sdw_bus *bus)
+{
+	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+	struct sdw_slave *slave, *_s;
+	struct sdw_slave_id id;
+	u32 val1, val2;
+	u64 addr;
+	int i;
+	char *buf1 = (char *)&val1, *buf2 = (char *)&val2;
+
+	for (i = 1; i < (SDW_MAX_DEVICES + 1); i++) {
+		/*SCP_Devid5 - Devid 4*/
+		ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_1(i), &val1);
+
+		/*SCP_Devid3 - DevId 2 Devid 1 Devid 0*/
+		ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_2(i), &val2);
+
+		if (!val1 && !val2)
+			break;
+
+		addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
+			((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
+			((u64)buf1[0] << 40);
+
+		sdw_extract_slave_id(bus, addr, &id);
+		/* Now compare with entries */
+		list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
+			if (sdw_compare_devid(slave, id) == 0) {
+				u32 status = qcom_swrm_get_n_device_status(ctrl, i);
+				if (status == SDW_SLAVE_ATTACHED) {
+					slave->dev_num = i;
+					mutex_lock(&bus->bus_lock);
+					set_bit(i, bus->assigned);
+					mutex_unlock(&bus->bus_lock);
+
+				}
+				break;
+			}
+		}
+	}
+
+	complete(&ctrl->enumeration);
+	return 0;
+}
+
 static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 {
 	struct qcom_swrm_ctrl *swrm = dev_id;
-	u32 value, intr_sts, intr_sts_masked;
+	u32 value, intr_sts, intr_sts_masked, slave_status;
 	u32 i;
 	u8 devnum = 0;
 	int ret = IRQ_HANDLED;
@@ -382,10 +443,19 @@  static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 				break;
 			case SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED:
 			case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
-				dev_err_ratelimited(swrm->dev, "%s: SWR new slave attached\n",
+				dev_err_ratelimited(swrm->dev, "%s: SWR slave status changed\n",
 					__func__);
-				qcom_swrm_get_device_status(swrm);
-				sdw_handle_slave_status(&swrm->bus, swrm->status);
+				swrm->reg_read(swrm, SWRM_MCP_SLV_STATUS, &slave_status);
+				if (swrm->slave_status == slave_status) {
+					dev_err(swrm->dev, "Slave status not changed %x\n",
+						slave_status);
+					break;
+				} else {
+					dev_err(swrm->dev, "Slave status handle %x\n", slave_status);
+					qcom_swrm_get_device_status(swrm);
+					qcom_swrm_enumerate(&swrm->bus);
+					sdw_handle_slave_status(&swrm->bus, swrm->status);
+				}
 				break;
 			case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET:
 				dev_err_ratelimited(swrm->dev,
@@ -472,8 +542,8 @@  static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 
 	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
 
-	/* Disable Auto enumeration */
-	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0);
+	/* Enable Auto enumeration */
+	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 1);
 
 	ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK;
 	/* Mask soundwire interrupts */
@@ -507,6 +577,7 @@  static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 		ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN,
 				SWRM_INTERRUPT_STATUS_RMSK);
 	}
+	ctrl->slave_status = 0;
 	return 0;
 }
 
@@ -1068,6 +1139,7 @@  static int qcom_swrm_probe(struct platform_device *pdev)
 	dev_set_drvdata(&pdev->dev, ctrl);
 	mutex_init(&ctrl->port_lock);
 	init_completion(&ctrl->broadcast);
+	init_completion(&ctrl->enumeration);
 
 	ctrl->bus.ops = &qcom_swrm_ops;
 	ctrl->bus.port_ops = &qcom_swrm_port_ops;