[08/14] soundwire: Add Slave status handling helpers

Commit Message

Vinod Koul Oct. 19, 2017, 3:03 a.m. UTC

From: Sanyog Kale <[e-mail]>

SoundWire Slaves report status to bus. Add helpers to handle
the status changes.

Signed-off-by: Hardik T Shah <[e-mail]>
Signed-off-by: Sanyog Kale <[e-mail]>
Signed-off-by: Vinod Koul <[e-mail]>
---
 drivers/soundwire/bus.c       | 211 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h |   1 +
 2 files changed, 212 insertions(+)

Comments

Takashi Iwai Oct. 19, 2017, 1:44 p.m. UTC | #1

On Thu, 19 Oct 2017 05:03:24 +0200,
Vinod Koul wrote:
> 
> +static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
> +{
> +	struct sdw_slave *slave;
> +
> +	list_for_each_entry(slave, &bus->slaves, node) {
> +		if (slave->dev_num == i)
> +			return slave;
> +	}
> +
> +	return NULL;

Is this performed always in bus_lock, right?
Better to document it.

> +static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
> +{
> +
> +	if ((slave->id.unique_id != id.unique_id) ||
> +			(slave->id.mfg_id != id.mfg_id) ||
> +			(slave->id.part_id != id.part_id) ||
> +			(slave->id.class_id != id.class_id))

Align indentations.

> +static int sdw_get_device_num(struct sdw_slave *slave)
> +{
> +	bool assigned = false;
> +	int i;
> +
> +	mutex_lock(&slave->bus->bus_lock);
> +	for (i = 1; i <= SDW_MAX_DEVICES; i++) {
> +		if (slave->bus->assigned[i] == true)
> +			continue;
> +
> +		slave->bus->assigned[i] = true;
> +		assigned = true;
> +
> +		/*
> +		 * Do not update dev_num in Slave data structure here,
> +		 * Update once program dev_num is successful
> +		 */
> +		break;

With the bitmap, it's easier, you can use find_next_zero_bit() :)


> +static int sdw_program_device_num(struct sdw_bus *bus)
> +{
> +	u8 buf[SDW_NUM_DEV_ID_REGISTERS] = {0};
> +	unsigned long long addr;

Use u64.

> +	struct sdw_slave *slave;
> +	struct sdw_slave_id id;
> +	struct sdw_msg msg;
> +	bool found = false;
> +	int ret;
> +
> +	/* No Slave, so use raw xfer api */
> +	sdw_fill_msg(&msg, SDW_SCP_DEVID_0, SDW_NUM_DEV_ID_REGISTERS,
> +					0, SDW_MSG_FLAG_READ, buf);
> +
> +	do {
> +		ret = sdw_transfer(bus, NULL, &msg);
> +		if (ret == -ENODATA)
> +			break;
> +		if (ret < 0) {
> +			dev_err(bus->dev, "DEVID read fail:%d\n", ret);
> +			break;

So here we break, and the function returns zero.  Is this the expected
behavior?

> +		}
> +
> +		/*
> +		 * Construct the addr and extract. Cast the higher shift
> +		 * bits to avoid truncation due to size limit.
> +		 */
> +		addr = buf[5] | (buf[4] << 8) | (buf[3] << 16) |
> +			(buf[2] << 24) | ((unsigned long long)buf[1] << 32) |
> +			((unsigned long long)buf[0] << 40);
> +
> +		sdw_extract_slave_id(bus, addr, &id);
> +
> +		/* Now compare with entries */
> +		list_for_each_entry(slave, &bus->slaves, node) {

Isn't this function protected under bus_lock...?

> +			if (sdw_compare_devid(slave, id) == 0) {
> +				found = true;
> +
> +				/*
> +				 * Assign a new dev_num to this Slave and
> +				 * not mark it present. It will be marked
> +				 * present after it reports ATTACHED on new
> +				 * dev_num
> +				 */
> +				ret = sdw_assign_device_num(slave);
> +				if (ret) {
> +					dev_err(slave->bus->dev,
> +						"Assign dev_num failed:%d",
> +						ret);
> +					return ret;
> +				}
> +
> +				break;
> +			}
> +		}
> +
> +		if (found == false) {
> +			/* TODO: Park this device in Group 13 */
> +			dev_err(bus->dev, "Slave Entry not found");

No break here?  Otherwise...

> +		}
> +
> +	} while (ret == 0);

... the outer loop may go endlessly.
This condition doesn't look effective.

> +
> +	return 0;

... and here returns no error?


thanks,

Takashi

Vinod Koul Oct. 31, 2017, 1:04 p.m. UTC | #2

On Thu, Oct 19, 2017 at 03:44:12PM +0200, Takashi Iwai wrote:
> On Thu, 19 Oct 2017 05:03:24 +0200,
> Vinod Koul wrote:

Sorry looks like I missed replying on this one, my apologies

> > +static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
> > +{
> > +	struct sdw_slave *slave;
> > +
> > +	list_for_each_entry(slave, &bus->slaves, node) {
> > +		if (slave->dev_num == i)
> > +			return slave;
> > +	}
> > +
> > +	return NULL;
> 
> Is this performed always in bus_lock, right?
> Better to document it.

Thanks we need to have lock here, fixed

> > +static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
> > +{
> > +
> > +	if ((slave->id.unique_id != id.unique_id) ||
> > +			(slave->id.mfg_id != id.mfg_id) ||
> > +			(slave->id.part_id != id.part_id) ||
> > +			(slave->id.class_id != id.class_id))
> 
> Align indentations.

sure

> > +static int sdw_get_device_num(struct sdw_slave *slave)
> > +{
> > +	bool assigned = false;
> > +	int i;
> > +
> > +	mutex_lock(&slave->bus->bus_lock);
> > +	for (i = 1; i <= SDW_MAX_DEVICES; i++) {
> > +		if (slave->bus->assigned[i] == true)
> > +			continue;
> > +
> > +		slave->bus->assigned[i] = true;
> > +		assigned = true;
> > +
> > +		/*
> > +		 * Do not update dev_num in Slave data structure here,
> > +		 * Update once program dev_num is successful
> > +		 */
> > +		break;
> 
> With the bitmap, it's easier, you can use find_next_zero_bit() :)

yes already done

> > +static int sdw_program_device_num(struct sdw_bus *bus)
> > +{
> > +	u8 buf[SDW_NUM_DEV_ID_REGISTERS] = {0};
> > +	unsigned long long addr;
> 
> Use u64.

yes fixed

> > +	struct sdw_slave *slave;
> > +	struct sdw_slave_id id;
> > +	struct sdw_msg msg;
> > +	bool found = false;
> > +	int ret;
> > +
> > +	/* No Slave, so use raw xfer api */
> > +	sdw_fill_msg(&msg, SDW_SCP_DEVID_0, SDW_NUM_DEV_ID_REGISTERS,
> > +					0, SDW_MSG_FLAG_READ, buf);
> > +
> > +	do {
> > +		ret = sdw_transfer(bus, NULL, &msg);
> > +		if (ret == -ENODATA)
> > +			break;
> > +		if (ret < 0) {
> > +			dev_err(bus->dev, "DEVID read fail:%d\n", ret);
> > +			break;
> 
> So here we break, and the function returns zero.  Is this the expected
> behavior?

nope, thats a bug fixed now

> > +		if (found == false) {
> > +			/* TODO: Park this device in Group 13 */
> > +			dev_err(bus->dev, "Slave Entry not found");
> 
> No break here?  Otherwise...

Thats intentional. We want to still read next device that show up

> 
> > +		}
> > +
> > +	} while (ret == 0);
> 
> ... the outer loop may go endlessly.
> This condition doesn't look effective.

not really. We cant keep reading successfully. At some point all slaves will
ignore and return ENODATA and we exit. Bus errors will also make it exit

BUT given that we have seen stuff i am inclined to add a counter, we cant
have more than 11 device so that's a sane value to use :)

Pierre-Louis Bossart Oct. 31, 2017, 9:19 p.m. UTC | #3

> 
>>> +		if (found == false) {
>>> +			/* TODO: Park this device in Group 13 */
>>> +			dev_err(bus->dev, "Slave Entry not found");
>>
>> No break here?  Otherwise...
> 
> Thats intentional. We want to still read next device that show up
> 
>>
>>> +		}
>>> +
>>> +	} while (ret == 0);
>>
>> ... the outer loop may go endlessly.
>> This condition doesn't look effective.
> 
> not really. We cant keep reading successfully. At some point all slaves will
> ignore and return ENODATA and we exit. Bus errors will also make it exit
> 
> BUT given that we have seen stuff i am inclined to add a counter, we cant
> have more than 11 device so that's a sane value to use :)

Yep. Keep in mind however that there could be theoretical corner cases: 
if a device is enumerated, loses sync and becomes attached again while 
you deal with others, you'd have more than 11 iterations.

Vinod Koul Nov. 1, 2017, 9:08 a.m. UTC | #4

On Wed, Nov 01, 2017 at 02:49:15AM +0530, Pierre-Louis Bossart wrote:
> 
> >
> >>>+		if (found == false) {
> >>>+			/* TODO: Park this device in Group 13 */
> >>>+			dev_err(bus->dev, "Slave Entry not found");
> >>
> >>No break here?  Otherwise...
> >
> >Thats intentional. We want to still read next device that show up
> >
> >>
> >>>+		}
> >>>+
> >>>+	} while (ret == 0);
> >>
> >>... the outer loop may go endlessly.
> >>This condition doesn't look effective.
> >
> >not really. We cant keep reading successfully. At some point all slaves will
> >ignore and return ENODATA and we exit. Bus errors will also make it exit
> >
> >BUT given that we have seen stuff i am inclined to add a counter, we cant
> >have more than 11 device so that's a sane value to use :)
> 
> Yep. Keep in mind however that there could be theoretical corner cases: if a
> device is enumerated, loses sync and becomes attached again while you deal
> with others, you'd have more than 11 iterations.

Not really as that would be another interrupt and another status report.

Pierre-Louis Bossart Nov. 1, 2017, 9:10 p.m. UTC | #5

On 11/1/17 4:08 AM, Vinod Koul wrote:
> On Wed, Nov 01, 2017 at 02:49:15AM +0530, Pierre-Louis Bossart wrote:
>>
>>>
>>>>> +		if (found == false) {
>>>>> +			/* TODO: Park this device in Group 13 */
>>>>> +			dev_err(bus->dev, "Slave Entry not found");
>>>>
>>>> No break here?  Otherwise...
>>>
>>> Thats intentional. We want to still read next device that show up
>>>
>>>>
>>>>> +		}
>>>>> +
>>>>> +	} while (ret == 0);
>>>>
>>>> ... the outer loop may go endlessly.
>>>> This condition doesn't look effective.
>>>
>>> not really. We cant keep reading successfully. At some point all slaves will
>>> ignore and return ENODATA and we exit. Bus errors will also make it exit
>>>
>>> BUT given that we have seen stuff i am inclined to add a counter, we cant
>>> have more than 11 device so that's a sane value to use :)
>>
>> Yep. Keep in mind however that there could be theoretical corner cases: if a
>> device is enumerated, loses sync and becomes attached again while you deal
>> with others, you'd have more than 11 iterations.
> 
> Not really as that would be another interrupt and another status report.

You are in a loop where you keep reading the devId registers, and that 
really has nothing to do with interrupts or status report. The point was 
that the number of times you read may be higher that the number of 
devices with a device being handled several times.
As mentioned above you need to limit this loop to a sane value.

Vinod Koul Nov. 2, 2017, 3:28 a.m. UTC | #6

On Wed, Nov 01, 2017 at 04:10:08PM -0500, Pierre-Louis Bossart wrote:
> On 11/1/17 4:08 AM, Vinod Koul wrote:
> >On Wed, Nov 01, 2017 at 02:49:15AM +0530, Pierre-Louis Bossart wrote:
> >>>
> >>>BUT given that we have seen stuff i am inclined to add a counter, we cant
> >>>have more than 11 device so that's a sane value to use :)
> >>
> >>Yep. Keep in mind however that there could be theoretical corner cases: if a
> >>device is enumerated, loses sync and becomes attached again while you deal
> >>with others, you'd have more than 11 iterations.
> >
> >Not really as that would be another interrupt and another status report.
> 
> You are in a loop where you keep reading the devId registers, and that
> really has nothing to do with interrupts or status report. The point was
> that the number of times you read may be higher that the number of devices
> with a device being handled several times.
> As mentioned above you need to limit this loop to a sane value.

Oh yes, I was thinking from status point of view which triggers this but yes
we keep reading so your point is valid. Lets add 2x buffer for that.

Patch

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 9ac22eab11bb..f206e78d4b68 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -393,6 +393,95 @@  int sdw_write(struct sdw_slave *slave, u32 addr, u8 value)
 }
 EXPORT_SYMBOL(sdw_write);
 
+/*
+ * SDW alert handling
+ */
+static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
+{
+	struct sdw_slave *slave;
+
+	list_for_each_entry(slave, &bus->slaves, node) {
+		if (slave->dev_num == i)
+			return slave;
+	}
+
+	return NULL;
+}
+
+static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
+{
+
+	if ((slave->id.unique_id != id.unique_id) ||
+			(slave->id.mfg_id != id.mfg_id) ||
+			(slave->id.part_id != id.part_id) ||
+			(slave->id.class_id != id.class_id))
+		return -ENODEV;
+
+	return 0;
+}
+
+static int sdw_get_device_num(struct sdw_slave *slave)
+{
+	bool assigned = false;
+	int i;
+
+	mutex_lock(&slave->bus->bus_lock);
+	for (i = 1; i <= SDW_MAX_DEVICES; i++) {
+		if (slave->bus->assigned[i] == true)
+			continue;
+
+		slave->bus->assigned[i] = true;
+		assigned = true;
+
+		/*
+		 * Do not update dev_num in Slave data structure here,
+		 * Update once program dev_num is successful
+		 */
+		break;
+	}
+
+	mutex_unlock(&slave->bus->bus_lock);
+	if (assigned)
+		return i;
+	else
+		return -ENODEV;
+}
+
+static int sdw_assign_device_num(struct sdw_slave *slave)
+{
+	int ret, dev_num;
+
+	/* check first if device number is assigned, if so reuse that */
+	if (!slave->dev_num) {
+		dev_num = sdw_get_device_num(slave);
+		if (dev_num < 0) {
+			dev_err(slave->bus->dev, "Get dev_num failed: %d",
+								dev_num);
+			return dev_num;
+		}
+	} else {
+		dev_info(slave->bus->dev,
+				"Slave already registered dev_num:%d",
+				slave->dev_num);
+
+		/* Clear the slave->dev_num to transfer message on device 0 */
+		dev_num = slave->dev_num;
+		slave->dev_num = 0;
+
+	}
+
+	ret = sdw_write(slave, SDW_SCP_DEVNUMBER, dev_num);
+	if (ret < 0) {
+		dev_err(&slave->dev, "Program device_num failed: %d", ret);
+		return ret;
+	}
+
+	/* After xfer of msg, restore dev_num */
+	slave->dev_num = dev_num;
+
+	return 0;
+}
+
 void sdw_extract_slave_id(struct sdw_bus *bus,
 			unsigned long long addr, struct sdw_slave_id *id)
 {
@@ -421,3 +510,125 @@  void sdw_extract_slave_id(struct sdw_bus *bus,
 				id->unique_id, id->sdw_version);
 
 }
+
+static int sdw_program_device_num(struct sdw_bus *bus)
+{
+	u8 buf[SDW_NUM_DEV_ID_REGISTERS] = {0};
+	unsigned long long addr;
+	struct sdw_slave *slave;
+	struct sdw_slave_id id;
+	struct sdw_msg msg;
+	bool found = false;
+	int ret;
+
+	/* No Slave, so use raw xfer api */
+	sdw_fill_msg(&msg, SDW_SCP_DEVID_0, SDW_NUM_DEV_ID_REGISTERS,
+					0, SDW_MSG_FLAG_READ, buf);
+
+	do {
+		ret = sdw_transfer(bus, NULL, &msg);
+		if (ret == -ENODATA)
+			break;
+		if (ret < 0) {
+			dev_err(bus->dev, "DEVID read fail:%d\n", ret);
+			break;
+		}
+
+		/*
+		 * Construct the addr and extract. Cast the higher shift
+		 * bits to avoid truncation due to size limit.
+		 */
+		addr = buf[5] | (buf[4] << 8) | (buf[3] << 16) |
+			(buf[2] << 24) | ((unsigned long long)buf[1] << 32) |
+			((unsigned long long)buf[0] << 40);
+
+		sdw_extract_slave_id(bus, addr, &id);
+
+		/* Now compare with entries */
+		list_for_each_entry(slave, &bus->slaves, node) {
+			if (sdw_compare_devid(slave, id) == 0) {
+				found = true;
+
+				/*
+				 * Assign a new dev_num to this Slave and
+				 * not mark it present. It will be marked
+				 * present after it reports ATTACHED on new
+				 * dev_num
+				 */
+				ret = sdw_assign_device_num(slave);
+				if (ret) {
+					dev_err(slave->bus->dev,
+						"Assign dev_num failed:%d",
+						ret);
+					return ret;
+				}
+
+				break;
+			}
+		}
+
+		if (found == false) {
+			/* TODO: Park this device in Group 13 */
+			dev_err(bus->dev, "Slave Entry not found");
+		}
+
+	} while (ret == 0);
+
+	return 0;
+}
+
+static void sdw_modify_slave_status(struct sdw_slave *slave,
+				enum sdw_slave_status status)
+{
+	mutex_lock(&slave->bus->bus_lock);
+	slave->status = status;
+	mutex_unlock(&slave->bus->bus_lock);
+}
+
+static int sdw_initialize_slave(struct sdw_slave *slave)
+{
+	struct sdw_slave_prop *prop = &slave->prop;
+	int val, ret;
+
+	val = sdw_read(slave, SDW_SCP_INTMASK1);
+	if (val < 0) {
+		dev_err(slave->bus->dev,
+				"SDW_SCP_INTMASK1 read failed:%d", val);
+		return val;
+	}
+
+	/* Set bus clash and parity interrupt mask */
+	val |= SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
+
+	/* Enable SCP interrupts */
+	ret = sdw_write(slave, SDW_SCP_INTMASK1, val);
+	if (ret < 0) {
+		dev_err(slave->bus->dev,
+				"SDW_SCP_INTMASK1 write failed:%d", val);
+		return ret;
+	}
+
+	/* No need to continue if DP0 is not present */
+	if (!slave->prop.dp0_prop)
+		return 0;
+
+	/* Enable DP0 interrupts */
+	val = sdw_read(slave, SDW_DP0_INTMASK);
+	if (val < 0) {
+		dev_err(slave->bus->dev,
+				"SDW_DP0_INTMASK read failed:%d", val);
+		return val;
+	}
+
+	val |= prop->dp0_prop->device_interrupts;
+	val |= SDW_DP0_INT_PORT_READY | SDW_DP0_INT_BRA_FAILURE;
+
+	ret = sdw_write(slave, SDW_DP0_INTMASK, val);
+	if (ret < 0) {
+		dev_err(slave->bus->dev,
+				"SDW_DP0_INTMASK write failed:%d", val);
+		return ret;
+	}
+
+	return 0;
+}
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index af94c0a29024..c25315bc5331 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -65,6 +65,7 @@  struct sdw_slave;
 /* SDW Enumeration Device Number */
 #define SDW_ENUM_DEV_NUM		0
 
+#define SDW_NUM_DEV_ID_REGISTERS	6
 #define SDW_MAX_DEVICES			11
 
 /**