driver: i2c: cc13xx_cc26xx: to support i2c scan #38584

Pillar1989 · 2021-09-16T06:18:46Z

Sending address without data, make i2c scan I2C_0 work.

Signed-off-by: Baozhu Zuo [email protected]

cfriedt

@Pillar1989 - please file an issue and link it to this PR. Describe the bug in more detail.

I know what this is for, but other reviewers might not.

It's mainly just because we are in our stabilization period right before release.

Pillar1989 · 2021-09-18T00:25:39Z

thanks @cfriedt guide me , that's the issue. #38645

cfriedt · 2021-09-18T01:59:55Z

@Pillar1989 - just a few minor whitespace formatting issues. Please fix up so that CI passes and then it should be merged relatively quickly.

Pillar1989 · 2021-09-18T02:37:40Z

update

devanlai

I've tested this with an LPSTK-CC1352R with the i2c scan command from the i2c shell and I can confirm that scanning works as expected.

It also would've been great to have this patch a few weeks ago when I was bringing up some new hardware and was puzzled by the i2c scan not showing anything.

stephanosio

Blocking this until we can validate this change.

stephanosio · 2021-09-21T01:14:30Z

drivers/i2c/i2c_cc13xx_cc26xx.c

-	/* Sending address without data is not supported */
+	I2CMasterSlaveAddrSet(base, addr, false);
+
+	/* Sending address without data */
 	if (len == 0) {
-		return -EIO;
+
+		I2CMasterControl(base, I2C_MASTER_CMD_SINGLE_SEND);
+
+		k_sem_take(&data->complete, K_FOREVER);
+
+		return data->error == I2C_MASTER_ERR_NONE ? 0 : -EIO;
 	}

-	I2CMasterSlaveAddrSet(base, addr, false);


Does this really work as expected?

As far as I can see, I2C_MASTER_CMD_SINGLE_SEND sends both address and data (written to I2CMDR); and we do not write to I2CMDR in this case, so we will be sending garbage data here:

Can you post an oscilloscope or logic analyser capture of the SCL/SDA waveforms and confirm that no data is sent here?

Your trace matches what I see with a logic analyzer. It is disappointing that the peripheral doesn't seem to be able to send just the address without also pushing a data byte.

@leadercxn let's figure out why there is append a data.

I consult this document -- CC13x2, CC26x2 SimpleLink™ Wireless MCU Technical Reference Manual (Rev. D) , in chapter 25.3.5 , it introduces several models of I2C communication, but i can't see any modes that just send the address without I2C_MDR data.
In the chapter 23.5 , there are no any registers refer to how to just send the address. I try to modify the MCTRL register, but it doesn't work . so i don‘t think it's hardware I2C supports this feature that just send the address without I2C_MDR data.

CC13x2, CC26x2 SimpleLink™ Wireless MCU Technical Reference Manual

@devanlai @stephanosio It seems that after some research, this additional data is justified.

It seems that after some research, this additional data is justified.

@Pillar1989 Could you elaborate?

In chapter 23.5, which has many Master TRANSMIT modes. But every mode needs append data after the i2c address.

In chapter 23.5, which has many Master TRANSMIT modes. But every mode needs append data after the i2c address.

@Pillar1989 That means that the original comment is correct and this change is invalid.

/* Sending address without data is not supported */

cfriedt · 2021-09-21T01:16:21Z

@Pillar1989 - try to pull from main and rebase your branch. I vaguely recall that this MAINTAINERS issue was solved that way.

cfriedt · 2021-09-22T12:58:55Z

drivers/i2c/i2c_cc13xx_cc26xx.c

@@ -58,12 +58,18 @@ static int i2c_cc13xx_cc26xx_transmit(const struct device *dev,
 	const uint32_t base = get_dev_config(dev)->base;
 	struct i2c_cc13xx_cc26xx_data *data = get_dev_data(dev);

-	/* Sending address without data is not supported */


/* Sending address without data is not supported */

I'm not 100% convinced that this statement is true, as I'm sure is the case for everyone else working with this SoC.

Let's ask the contributor with arguably the most experience with the vendor API in question.

@vanti - is there a way to perform an I2C scan / probe using the simplelink I2C driverlib? I.e. sending the address bits without sending data bits (with some indication as to whether the connected peripheral has ACK'ed the address bits). If so, how do we do it?

For reference, a good description of the goal is here:
https://hackaday.com/2015/06/25/embed-with-elliot-i2c-bus-scanning/

and TI's API is here:
https://software-dl.ti.com/simplelink/esd/simplelink_msp432e4_sdk/2.30.00.14/docs/driverlib/msp432e4/html/group__i2c__api.html

@Pillar1989, @Vaishnav98 - given what everyone else has said, I think we may not find the answer in the SimpleLink SDK. However, i sort of wonder if this could be bit-banged 🤔

@cfriedt @stephanosio Assume that we need to add more code to i2c scan to support some SoC like CC13xx serial. any suggest? Or maybe this is a half-baked idea, and we still need to fix the problem of sending only addresses.

Pillar1989 · 2021-09-23T11:00:07Z

@Pillar1989 - try to pull from main and rebase your branch. I vaguely recall that this MAINTAINERS issue was solved that way.

let me try again

Pillar1989 · 2021-09-27T00:21:16Z

@stephanosio any more questions need to discuss ?

stephanosio

It looks like the I2C peripheral of the CC13xx and CC26xx SoCs do not support sending address without data as described by the original comment, so this change is invalid.

The current I2C scan implementation expects the I2C peripheral to be able to only send the address to probe the peripherals on the bus, and that is with a good reason because sending data byte can be an intrusive behaviour.

If you want to make the I2C scan function work for this SoC (and others that do not support sending address without data), you will need to modify the I2C scan function itself and introduce something like a "intrusive" scan mode.

cc @nashif since he initially implemented the I2C scan function.

Sending address without data, make `i2c scan I2C_0` work. Signed-off-by: Baozhu Zuo <[email protected]>

Pillar1989 · 2021-09-27T00:59:28Z

https://manpages.debian.org/unstable/i2c-tools/i2cdetect.8.en.html @stephanosio
I have tried i2cdetect on Raspberry Pi，It can't appear your want which just sends the address. It seems that these tools are inherently two-sided. If you want zephyr to keep the I2C bus uninsurable, I think it should be without introducing i2c scan.

stephanosio · 2021-09-27T01:21:43Z

The source of the problem is the I2C shell scan command implementation that requires a zero-length message to be sent, which the I2C peripheral of this particular SoC cannot support:

zephyr/drivers/i2c/i2c_shell.c

Line 64 in 21d1ad3

msgs[0].len = 0U;

The current behaviour of the I2C driver returning -EIO when zero-length message is specified is correct because this particular SoC indeed does not support sending a zero-length message.

It does not make sense to change/introduce an incorrect behaviour to the I2C driver in order to support the I2C shell scan command -- this is simply not a correct thing to do.

Instead, the I2C shell scan command should be changed to support the class of devices that do not allow sending zero-length message by design (e.g. through a separate "intrusive scan" feature or by making all scans intrusive; I leave this decision up to whomever maintains this feature).

cfriedt · 2021-09-27T22:00:52Z

The source of the problem is the I2C shell scan command implementation that requires a zero-length message to be sent, which the I2C peripheral of this particular SoC cannot support:

Has TI stated that? There could be a bug or undersupported feature in the SimpleLink SDK, but I wasn't able to find anything elaborating on the '-ENOTSUP' conclusion here. It would be surprising if a TI part could not accomplish zero-length transfers, because presumably TI parts have been doing that for a long time.

@Pillar1989 - have you asked about this on e2e.ti.com?

Instead, the I2C shell scan command should be changed to support the class of devices that do not allow sending zero-length message by design (e.g. through a separate "intrusive scan" feature or by making all scans intrusive; I leave this decision up to whomever maintains this feature).

I would second that as a feature.

cfriedt · 2021-09-28T00:20:48Z

The source of the problem is the I2C shell scan command implementation that requires a zero-length message to be sent, which the I2C peripheral of this particular SoC cannot support:

Has TI stated that? There could be a bug or undersupported feature in the SimpleLink SDK, but I wasn't able to find anything elaborating on the '-ENOTSUP' conclusion here. It would be surprising if a TI part could not accomplish zero-length transfers, because presumably TI parts have been doing that for a long time.

@Pillar1989 - have you asked about this on e2e.ti.com?

The same question asked on e2e.

https://e2e.ti.com/support/wireless-connectivity/sub-1-ghz-group/sub-1-ghz/f/sub-1-ghz-forum/898040/cc1310-zero-length-write-over-i2c-to-wake-up-a-sensor

@Vaishnav98 - are you aware if anyone at TI has investigated this issue? It seems to be a common problem and has an e2e ticket associated with it.

I sort of wonder if it could be bitbanged .. 🤔

cfriedt · 2021-09-28T01:31:38Z

I think this would be best handled by pinctrl, to be honest. e.g. a gpio i2c device that shares pins with the dedicated i2c device, add a devicetree property..

cfriedt

Will likely require some work involving the upcoming pinctrl feature

Pillar1989 · 2021-10-13T00:28:52Z

OK, close this PR

Pillar1989 requested a review from bwitherspoon as a code owner September 16, 2021 06:18

github-actions bot added the area: I2C label Sep 16, 2021

Pillar1989 mentioned this pull request Sep 16, 2021

driver: i2c: cc13xx_cc26xx: to support i2c scan jadonk/zephyr#10

Merged

cfriedt approved these changes Sep 17, 2021

View reviewed changes

cfriedt linked an issue Sep 18, 2021 that may be closed by this pull request

cc13xx and cc26xx serial boards i2c scan not work #38645

Closed

Pillar1989 force-pushed the main branch from 5a06cba to ba394ef Compare September 18, 2021 02:35

devanlai approved these changes Sep 21, 2021

View reviewed changes

stephanosio requested changes Sep 21, 2021

View reviewed changes

cfriedt reviewed Sep 22, 2021

View reviewed changes

stephanosio requested a review from vanti September 22, 2021 13:08

Pillar1989 force-pushed the main branch from ba394ef to 24b6bf7 Compare September 23, 2021 10:58

stephanosio requested changes Sep 27, 2021

View reviewed changes

driver: i2c: cc13xx_cc26xx: to support i2c scan

a9c3dde

Sending address without data, make `i2c scan I2C_0` work. Signed-off-by: Baozhu Zuo <[email protected]>

Pillar1989 force-pushed the main branch from 24b6bf7 to a9c3dde Compare September 27, 2021 00:38

cfriedt requested changes Oct 12, 2021

View reviewed changes

Pillar1989 closed this Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver: i2c: cc13xx_cc26xx: to support i2c scan #38584

driver: i2c: cc13xx_cc26xx: to support i2c scan #38584

Pillar1989 commented Sep 16, 2021

cfriedt left a comment

Pillar1989 commented Sep 18, 2021

cfriedt commented Sep 18, 2021

Pillar1989 commented Sep 18, 2021

devanlai left a comment

stephanosio left a comment

stephanosio Sep 21, 2021

devanlai Sep 21, 2021

Pillar1989 Sep 22, 2021

leadercxn Sep 23, 2021

Pillar1989 Sep 23, 2021

stephanosio Sep 23, 2021

Pillar1989 Sep 23, 2021

stephanosio Sep 27, 2021

cfriedt commented Sep 21, 2021

cfriedt Sep 22, 2021 •

edited

Loading

cfriedt Sep 22, 2021

cfriedt Sep 28, 2021

Pillar1989 Sep 28, 2021

Pillar1989 commented Sep 23, 2021

Pillar1989 commented Sep 27, 2021

stephanosio left a comment •

edited

Loading

Pillar1989 commented Sep 27, 2021

stephanosio commented Sep 27, 2021

cfriedt commented Sep 27, 2021 •

edited

Loading

cfriedt commented Sep 28, 2021 •

edited

Loading

cfriedt commented Sep 28, 2021

cfriedt left a comment

Pillar1989 commented Oct 13, 2021

driver: i2c: cc13xx_cc26xx: to support i2c scan #38584

driver: i2c: cc13xx_cc26xx: to support i2c scan #38584

Conversation

Pillar1989 commented Sep 16, 2021

cfriedt left a comment

Choose a reason for hiding this comment

Pillar1989 commented Sep 18, 2021

cfriedt commented Sep 18, 2021

Pillar1989 commented Sep 18, 2021

devanlai left a comment

Choose a reason for hiding this comment

stephanosio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfriedt commented Sep 21, 2021

cfriedt Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pillar1989 commented Sep 23, 2021

Pillar1989 commented Sep 27, 2021

stephanosio left a comment • edited Loading

Choose a reason for hiding this comment

Pillar1989 commented Sep 27, 2021

stephanosio commented Sep 27, 2021

cfriedt commented Sep 27, 2021 • edited Loading

cfriedt commented Sep 28, 2021 • edited Loading

cfriedt commented Sep 28, 2021

cfriedt left a comment

Choose a reason for hiding this comment

Pillar1989 commented Oct 13, 2021

cfriedt Sep 22, 2021 •

edited

Loading

stephanosio left a comment •

edited

Loading

cfriedt commented Sep 27, 2021 •

edited

Loading

cfriedt commented Sep 28, 2021 •

edited

Loading