[PATCH 0/3] soundwire: qcom: add Clock Stop and Auto Enumeration support

[PATCH 0/3] soundwire: qcom: add Clock Stop and Auto Enumeration support

[Srinivas Kandagatla]

Qualcomm Soundwire controller have IP built-in feature to enumerate
SoundWire slave devices aka Auto Enumeration.
This patchset adds support to this feature along with Clock stop
feature using runtime PM.

Tested it on DB845c, RB5 and SM8250-MTP.

thanks,
srini

Srinivas Kandagatla (3):
  soundwire: export sdw_compare_devid() and sdw_extract_slave_id()
  soundwire: qcom: add auto enumeration support
  soundwire: qcom: add clock stop via runtime pm support

 drivers/soundwire/bus.c       |   4 +-
 drivers/soundwire/qcom.c      | 187 +++++++++++++++++++++++++++++++---
 include/linux/soundwire/sdw.h |   2 +
 3 files changed, 179 insertions(+), 14 deletions(-)

-- 
2.21.0

[PATCH 1/3] soundwire: export sdw_compare_devid() and sdw_extract_slave_id()

[Srinivas Kandagatla]

Exporting these two functions makes sense as it can be used by
other controllers like Qualcomm during auto-enumeration!

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/soundwire/bus.c       | 4 +++-
 include/linux/soundwire/sdw.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 48577b78f282..46d3407c097f 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -608,7 +608,7 @@ static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
 	return NULL;
 }
 
-static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
+int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
 {
 	if (slave->id.mfg_id != id.mfg_id ||
 	    slave->id.part_id != id.part_id ||
@@ -619,6 +619,7 @@ static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
 
 	return 0;
 }
+EXPORT_SYMBOL(sdw_compare_devid);
 
 /* called with bus_lock held */
 static int sdw_get_device_num(struct sdw_slave *slave)
@@ -703,6 +704,7 @@ void sdw_extract_slave_id(struct sdw_bus *bus,
 		"SDW Slave class_id 0x%02x, mfg_id 0x%04x, part_id 0x%04x, unique_id 0x%x, version 0x%x\n",
 		id->class_id, id->mfg_id, id->part_id, id->unique_id, id->sdw_version);
 }
+EXPORT_SYMBOL(sdw_extract_slave_id);
 
 static int sdw_program_device_num(struct sdw_bus *bus)
 {
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index d08039d65825..8478a1f5f833 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -1009,5 +1009,7 @@ int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value);
 int sdw_read_no_pm(struct sdw_slave *slave, u32 addr);
 int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val);
 int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val);
+int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id);
+void sdw_extract_slave_id(struct sdw_bus *bus, u64 addr, struct sdw_slave_id *id);
 
 #endif /* __SOUNDWIRE_H */
-- 
2.21.0

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

[Srinivas Kandagatla]

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(-)

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;
-- 
2.21.0

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

[Pierre-Louis Bossart]

> +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;
> 

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

[Srinivas Kandagatla]

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;
>>

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

[Pierre-Louis Bossart]

>>> +        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?

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

[Srinivas Kandagatla]

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
> 

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

[Pierre-Louis Bossart]

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.

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

[Srinivas Kandagatla]

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
> 
> 

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

[Pierre-Louis Bossart]

>>>>>>> +        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.

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

[Srinivas Kandagatla]

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

[PATCH 3/3] soundwire: qcom: add clock stop via runtime pm support

[Srinivas Kandagatla]

Add Clock stop feature support using runtime PM.

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

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index d90eba6a1e88..0cf8c5c724e2 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -10,6 +10,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/slimbus.h>
@@ -19,6 +20,8 @@
 #include <sound/soc.h>
 #include "bus.h"
 
+#define SWRM_COMP_SW_RESET					(0x008)
+#define SWRM_COMP_STATUS					(0x014)
 #define SWRM_COMP_HW_VERSION					0x00
 #define SWRM_COMP_CFG_ADDR					0x04
 #define SWRM_COMP_CFG_IRQ_LEVEL_OR_PULSE_MSK			BIT(1)
@@ -408,6 +411,9 @@ static int qcom_swrm_enumerate(struct sdw_bus *bus)
 			}
 		}
 	}
+	pm_runtime_get_sync(ctrl->dev);
+	pm_runtime_mark_last_busy(ctrl->dev);
+	pm_runtime_put_autosuspend(ctrl->dev);
 
 	complete(&ctrl->enumeration);
 	return 0;
@@ -421,6 +427,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 	u8 devnum = 0;
 	int ret = IRQ_HANDLED;
 
+	clk_prepare_enable(swrm->hclk);
 	swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
 	intr_sts_masked = intr_sts & swrm->intr_mask;
 
@@ -529,6 +536,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 		intr_sts_masked = intr_sts & swrm->intr_mask;
 	} while (intr_sts_masked);
 
+	clk_disable_unprepare(swrm->hclk);
 	return ret;
 }
 
@@ -587,6 +595,8 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
 	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
 	int ret, i, len;
 
+	pm_runtime_get_sync(ctrl->dev);
+
 	if (msg->flags == SDW_MSG_FLAG_READ) {
 		for (i = 0; i < msg->len;) {
 			if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
@@ -598,7 +608,7 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
 							msg->addr + i, len,
 						       &msg->buf[i]);
 			if (ret)
-				return ret;
+				goto done;
 
 			i = i + len;
 		}
@@ -607,12 +617,20 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
 			ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
 							msg->dev_num,
 						       msg->addr + i);
-			if (ret)
-				return SDW_CMD_IGNORED;
+			if (ret) {
+				ret = SDW_CMD_IGNORED;
+				goto done;
+			}
 		}
 	}
 
+	pm_runtime_put_autosuspend(ctrl->dev);
+	pm_runtime_mark_last_busy(ctrl->dev);
 	return SDW_CMD_OK;
+done:
+	pm_runtime_mark_last_busy(ctrl->dev);
+	pm_runtime_put_autosuspend(ctrl->dev);
+	return ret;
 }
 
 static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
@@ -620,13 +638,19 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
 	u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
 	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
 	u32 val;
+	int ret;
 
+	pm_runtime_get_sync(ctrl->dev);
 	ctrl->reg_read(ctrl, reg, &val);
 
 	u32p_replace_bits(&val, ctrl->cols_index, SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK);
 	u32p_replace_bits(&val, ctrl->rows_index, SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK);
 
-	return ctrl->reg_write(ctrl, reg, val);
+	ret = ctrl->reg_write(ctrl, reg, val);
+	pm_runtime_mark_last_busy(ctrl->dev);
+	pm_runtime_put_autosuspend(ctrl->dev);
+
+	return ret;
 }
 
 static int qcom_swrm_port_params(struct sdw_bus *bus,
@@ -634,13 +658,18 @@ static int qcom_swrm_port_params(struct sdw_bus *bus,
 				 unsigned int bank)
 {
 	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
+	int ret = 0;
+	pm_runtime_get_sync(ctrl->dev);
 
 	if (p_params->bps != SWR_INVALID_PARAM)
-		return ctrl->reg_write(ctrl,
+		ret = ctrl->reg_write(ctrl,
 				       SWRM_DP_BLOCK_CTRL_1(p_params->num),
 				       p_params->bps - 1);
+	pm_runtime_mark_last_busy(ctrl->dev);
+	pm_runtime_put_autosuspend(ctrl->dev);
 
-	return 0;
+
+	return ret;
 }
 
 static int qcom_swrm_transport_params(struct sdw_bus *bus,
@@ -651,6 +680,7 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus,
 	u32 value;
 	int reg = SWRM_DP_PORT_CTRL_BANK((params->port_num), bank);
 	int ret;
+	pm_runtime_get_sync(ctrl->dev);
 
 	value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT;
 	value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT;
@@ -685,6 +715,9 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus,
 		reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
 		ret = ctrl->reg_write(ctrl, reg, params->blk_pkg_mode);
 	}
+	pm_runtime_mark_last_busy(ctrl->dev);
+	pm_runtime_put_autosuspend(ctrl->dev);
+
 
 	return ret;
 }
@@ -696,6 +729,9 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus,
 	u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
 	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
 	u32 val;
+	int ret;
+
+	pm_runtime_get_sync(ctrl->dev);
 
 	ctrl->reg_read(ctrl, reg, &val);
 
@@ -704,7 +740,11 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus,
 	else
 		val &= ~(0xff << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
 
-	return ctrl->reg_write(ctrl, reg, val);
+	ret  = ctrl->reg_write(ctrl, reg, val);
+	pm_runtime_mark_last_busy(ctrl->dev);
+	pm_runtime_put_autosuspend(ctrl->dev);
+
+	return ret;
 }
 
 static const struct sdw_master_port_ops qcom_swrm_port_ops = {
@@ -1194,6 +1234,13 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
 		 ctrl->version & 0xffff);
 
+	pm_runtime_set_autosuspend_delay(dev, 30000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_mark_last_busy(dev);
+
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	return 0;
 
 err_master_add:
@@ -1214,6 +1261,47 @@ static int qcom_swrm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int swrm_runtime_resume(struct device *dev)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
+
+	reinit_completion(&ctrl->enumeration);
+	clk_prepare_enable(ctrl->hclk);
+	ctrl->reg_write(ctrl, SWRM_COMP_SW_RESET, 0x01);
+	qcom_swrm_get_device_status(ctrl);
+	sdw_handle_slave_status(&ctrl->bus, ctrl->status);
+	qcom_swrm_init(ctrl);
+	wait_for_completion_timeout(&ctrl->enumeration,
+					msecs_to_jiffies(TIMEOUT_MS));
+	usleep_range(100, 105);
+
+	pm_runtime_mark_last_busy(dev);
+
+	return 0;
+}
+
+static int __maybe_unused swrm_runtime_suspend(struct device *dev)
+{
+	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
+
+	/* Mask bus clash interrupt */
+	ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
+	ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, ctrl->intr_mask);
+	ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask);
+	/* clock stop sequence */
+	qcom_swrm_cmd_fifo_wr_cmd(ctrl, 0x2, 0xF, SDW_SCP_CTRL);
+
+	clk_disable_unprepare(ctrl->hclk);
+
+	usleep_range(100, 105);
+
+	return 0;
+}
+
+static const struct dev_pm_ops swrm_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL)
+};
+
 static const struct of_device_id qcom_swrm_of_match[] = {
 	{ .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data },
 	{ .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data },
@@ -1228,6 +1316,7 @@ static struct platform_driver qcom_swrm_driver = {
 	.driver = {
 		.name	= "qcom-soundwire",
 		.of_match_table = qcom_swrm_of_match,
+		.pm = &swrm_dev_pm_ops,
 	}
 };
 module_platform_driver(qcom_swrm_driver);
-- 
2.21.0

Re: [PATCH 3/3] soundwire: qcom: add clock stop via runtime pm support

[Pierre-Louis Bossart]

> +	pm_runtime_get_sync(ctrl->dev);

if this fails you've got to do a put_noidle().

we use this for Intel:

	ret = pm_runtime_get_sync(cdns->dev);
	if (ret < 0 && ret != -EACCES) {
		dev_err_ratelimited(cdns->dev,
				    "pm_runtime_get_sync failed in %s, ret %d\n",
				    __func__, ret);
		pm_runtime_put_noidle(cdns->dev);
		return ret;
	}


> +	pm_runtime_mark_last_busy(ctrl->dev);
> +	pm_runtime_put_autosuspend(ctrl->dev);
>   
>   	complete(&ctrl->enumeration);
>   	return 0;
> @@ -421,6 +427,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   	u8 devnum = 0;
>   	int ret = IRQ_HANDLED;
>   
> +	clk_prepare_enable(swrm->hclk);
>   	swrm->reg_read(swrm, SWRM_INTERRUPT_STATUS, &intr_sts);
>   	intr_sts_masked = intr_sts & swrm->intr_mask;
>   
> @@ -529,6 +536,7 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>   		intr_sts_masked = intr_sts & swrm->intr_mask;
>   	} while (intr_sts_masked);
>   
> +	clk_disable_unprepare(swrm->hclk);
>   	return ret;
>   }
>   
> @@ -587,6 +595,8 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
>   	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>   	int ret, i, len;
>   
> +	pm_runtime_get_sync(ctrl->dev);
> +
>   	if (msg->flags == SDW_MSG_FLAG_READ) {
>   		for (i = 0; i < msg->len;) {
>   			if ((msg->len - i) < QCOM_SWRM_MAX_RD_LEN)
> @@ -598,7 +608,7 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
>   							msg->addr + i, len,
>   						       &msg->buf[i]);
>   			if (ret)
> -				return ret;
> +				goto done;
>   
>   			i = i + len;
>   		}
> @@ -607,12 +617,20 @@ static enum sdw_command_response qcom_swrm_xfer_msg(struct sdw_bus *bus,
>   			ret = qcom_swrm_cmd_fifo_wr_cmd(ctrl, msg->buf[i],
>   							msg->dev_num,
>   						       msg->addr + i);
> -			if (ret)
> -				return SDW_CMD_IGNORED;
> +			if (ret) {
> +				ret = SDW_CMD_IGNORED;
> +				goto done;
> +			}
>   		}
>   	}
>   
> +	pm_runtime_put_autosuspend(ctrl->dev);
> +	pm_runtime_mark_last_busy(ctrl->dev);

wrong order, you've got to mark_last_busy before the put().

>   	return SDW_CMD_OK;
> +done:
> +	pm_runtime_mark_last_busy(ctrl->dev);
> +	pm_runtime_put_autosuspend(ctrl->dev);
> +	return ret;

this looks weird. why not reuse the same flow and return ret in all cases?

>   }
>   
>   static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
> @@ -620,13 +638,19 @@ static int qcom_swrm_pre_bank_switch(struct sdw_bus *bus)
>   	u32 reg = SWRM_MCP_FRAME_CTRL_BANK_ADDR(bus->params.next_bank);
>   	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>   	u32 val;
> +	int ret;
>   
> +	pm_runtime_get_sync(ctrl->dev);
>   	ctrl->reg_read(ctrl, reg, &val);
>   
>   	u32p_replace_bits(&val, ctrl->cols_index, SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK);
>   	u32p_replace_bits(&val, ctrl->rows_index, SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK);
>   
> -	return ctrl->reg_write(ctrl, reg, val);
> +	ret = ctrl->reg_write(ctrl, reg, val);
> +	pm_runtime_mark_last_busy(ctrl->dev);
> +	pm_runtime_put_autosuspend(ctrl->dev);
> +
> +	return ret;
>   }
>   
>   static int qcom_swrm_port_params(struct sdw_bus *bus,
> @@ -634,13 +658,18 @@ static int qcom_swrm_port_params(struct sdw_bus *bus,
>   				 unsigned int bank)
>   {
>   	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> +	int ret = 0;
> +	pm_runtime_get_sync(ctrl->dev);
>   
>   	if (p_params->bps != SWR_INVALID_PARAM)
> -		return ctrl->reg_write(ctrl,
> +		ret = ctrl->reg_write(ctrl,
>   				       SWRM_DP_BLOCK_CTRL_1(p_params->num),
>   				       p_params->bps - 1);
> +	pm_runtime_mark_last_busy(ctrl->dev);
> +	pm_runtime_put_autosuspend(ctrl->dev);

it feels like all you pm_runtime_get/put() should be moved to your 
register read/write operations?

>   
> -	return 0;
> +
> +	return ret;
>   }
>   
>   static int qcom_swrm_transport_params(struct sdw_bus *bus,
> @@ -651,6 +680,7 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus,
>   	u32 value;
>   	int reg = SWRM_DP_PORT_CTRL_BANK((params->port_num), bank);
>   	int ret;
> +	pm_runtime_get_sync(ctrl->dev);
>   
>   	value = params->offset1 << SWRM_DP_PORT_CTRL_OFFSET1_SHFT;
>   	value |= params->offset2 << SWRM_DP_PORT_CTRL_OFFSET2_SHFT;
> @@ -685,6 +715,9 @@ static int qcom_swrm_transport_params(struct sdw_bus *bus,
>   		reg = SWRM_DP_BLOCK_CTRL3_BANK(params->port_num, bank);
>   		ret = ctrl->reg_write(ctrl, reg, params->blk_pkg_mode);
>   	}
> +	pm_runtime_mark_last_busy(ctrl->dev);
> +	pm_runtime_put_autosuspend(ctrl->dev);
> +
>   
>   	return ret;
>   }
> @@ -696,6 +729,9 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus,
>   	u32 reg = SWRM_DP_PORT_CTRL_BANK(enable_ch->port_num, bank);
>   	struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>   	u32 val;
> +	int ret;
> +
> +	pm_runtime_get_sync(ctrl->dev);
>   
>   	ctrl->reg_read(ctrl, reg, &val);
>   
> @@ -704,7 +740,11 @@ static int qcom_swrm_port_enable(struct sdw_bus *bus,
>   	else
>   		val &= ~(0xff << SWRM_DP_PORT_CTRL_EN_CHAN_SHFT);
>   
> -	return ctrl->reg_write(ctrl, reg, val);
> +	ret  = ctrl->reg_write(ctrl, reg, val);
> +	pm_runtime_mark_last_busy(ctrl->dev);
> +	pm_runtime_put_autosuspend(ctrl->dev);
> +
> +	return ret;
>   }
>   
>   static const struct sdw_master_port_ops qcom_swrm_port_ops = {
> @@ -1194,6 +1234,13 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>   		 (ctrl->version >> 24) & 0xff, (ctrl->version >> 16) & 0xff,
>   		 ctrl->version & 0xffff);
>   
> +	pm_runtime_set_autosuspend_delay(dev, 30000);

30s? that sounds very very long for an audio device.

> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_mark_last_busy(dev);
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>   	return 0;
>   
>   err_master_add:
> @@ -1214,6 +1261,47 @@ static int qcom_swrm_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static int swrm_runtime_resume(struct device *dev)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	reinit_completion(&ctrl->enumeration);
> +	clk_prepare_enable(ctrl->hclk);
> +	ctrl->reg_write(ctrl, SWRM_COMP_SW_RESET, 0x01);
> +	qcom_swrm_get_device_status(ctrl);
> +	sdw_handle_slave_status(&ctrl->bus, ctrl->status);
> +	qcom_swrm_init(ctrl);
> +	wait_for_completion_timeout(&ctrl->enumeration,
> +					msecs_to_jiffies(TIMEOUT_MS));
> +	usleep_range(100, 105);
> +
> +	pm_runtime_mark_last_busy(dev);

Humm, what 'clock stop' are we talking about here?

In SoundWire 1.x devices, you can stop the BUS clock and not have to 
redo any enumeration on resume, devices are required to save their 
context.  You have to also follow the pattern of preparing and 
broadcasting the CLOCK STOP NOW message.

It looks like you are stopping something else, and completely resetting 
the hardware. It's fine, it's just a reset but not clock stop support as 
defined in the SoundWire spec.

> +
> +	return 0;
> +}
> +
> +static int __maybe_unused swrm_runtime_suspend(struct device *dev)
> +{
> +	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	/* Mask bus clash interrupt */
> +	ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
> +	ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, ctrl->intr_mask);
> +	ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask);
> +	/* clock stop sequence */
> +	qcom_swrm_cmd_fifo_wr_cmd(ctrl, 0x2, 0xF, SDW_SCP_CTRL);

Humm, this looks like writing in SCP_CTRL::ClockStopNow, so why is 
enumeration required on restart?

If you take down the bus and reset everything, you don't need to do this 
sequence. a hardware reset will do...

> +
> +	clk_disable_unprepare(ctrl->hclk);
> +
> +	usleep_range(100, 105);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops swrm_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL)
> +};
> +
>   static const struct of_device_id qcom_swrm_of_match[] = {
>   	{ .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data },
>   	{ .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data },
> @@ -1228,6 +1316,7 @@ static struct platform_driver qcom_swrm_driver = {
>   	.driver = {
>   		.name	= "qcom-soundwire",
>   		.of_match_table = qcom_swrm_of_match,
> +		.pm = &swrm_dev_pm_ops,
>   	}
>   };
>   module_platform_driver(qcom_swrm_driver);
> 

Re: [PATCH 3/3] soundwire: qcom: add clock stop via runtime pm support

[Srinivas Kandagatla]

Thanks Pierre for reviewing this in detail!


On 26/02/2021 17:41, Pierre-Louis Bossart wrote:
...

>>       return 0;
>>   err_master_add:
>> @@ -1214,6 +1261,47 @@ static int qcom_swrm_remove(struct 
>> platform_device *pdev)
>>       return 0;
>>   }
>> +static int swrm_runtime_resume(struct device *dev)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
>> +
>> +    reinit_completion(&ctrl->enumeration);
>> +    clk_prepare_enable(ctrl->hclk);
>> +    ctrl->reg_write(ctrl, SWRM_COMP_SW_RESET, 0x01);
>> +    qcom_swrm_get_device_status(ctrl);
>> +    sdw_handle_slave_status(&ctrl->bus, ctrl->status);
>> +    qcom_swrm_init(ctrl);
>> +    wait_for_completion_timeout(&ctrl->enumeration,
>> +                    msecs_to_jiffies(TIMEOUT_MS));
>> +    usleep_range(100, 105);
>> +
>> +    pm_runtime_mark_last_busy(dev);
> 
> Humm, what 'clock stop' are we talking about here?
> 
> In SoundWire 1.x devices, you can stop the BUS clock and not have to 
> redo any enumeration on resume, devices are required to save their 
> context.  You have to also follow the pattern of preparing and 
> broadcasting the CLOCK STOP NOW message.
> 
> It looks like you are stopping something else, and completely resetting 
> the hardware. It's fine, it's just a reset but not clock stop support as 
> defined in the SoundWire spec.
> 

This is clock stop that Soundwire Spec refers to.

However I think I messed up this patch! :-)




>> +
>> +    return 0;
>> +}
>> +
>> +static int __maybe_unused swrm_runtime_suspend(struct device *dev)
>> +{
>> +    struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
>> +
>> +    /* Mask bus clash interrupt */
>> +    ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR, ctrl->intr_mask);
>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, ctrl->intr_mask);
>> +    /* clock stop sequence */
>> +    qcom_swrm_cmd_fifo_wr_cmd(ctrl, 0x2, 0xF, SDW_SCP_CTRL);
> 
> Humm, this looks like writing in SCP_CTRL::ClockStopNow, so why is 
> enumeration required on restart?
> 
One of the controller instance needed a full reset so there is a mix of 
code for both clock stop and reset here!

Am working on cleaning up this in a better way!

I will also address the runtime pm comments that you have noticed in 
next version!

--srini


> If you take down the bus and reset everything, you don't need to do this 
> sequence. a hardware reset will do...
> 
>> +
>> +    clk_disable_unprepare(ctrl->hclk);
>> +
>> +    usleep_range(100, 105);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct dev_pm_ops swrm_dev_pm_ops = {
>> +    SET_RUNTIME_PM_OPS(swrm_runtime_suspend, swrm_runtime_resume, NULL)
>> +};
>> +
>>   static const struct of_device_id qcom_swrm_of_match[] = {
>>       { .compatible = "qcom,soundwire-v1.3.0", .data = &swrm_v1_3_data },
>>       { .compatible = "qcom,soundwire-v1.5.1", .data = &swrm_v1_5_data },
>> @@ -1228,6 +1316,7 @@ static struct platform_driver qcom_swrm_driver = {
>>       .driver = {
>>           .name    = "qcom-soundwire",
>>           .of_match_table = qcom_swrm_of_match,
>> +        .pm = &swrm_dev_pm_ops,
>>       }
>>   };
>>   module_platform_driver(qcom_swrm_driver);
>>