[doc] I2c doc redux #1047

martin-lueker · 2019-11-21T21:23:53Z

(Repeat PR of #686)

Translation to Markdown of https://docs.google.com/document/d/1xVGRX7c5jhQyrJ__iubYD2NmKRO7NlXYuspthZIfvfI

Minor changes from Google Doc:
Elaboration of timing registers.
Removal of incomplete section on START, STOP, transmit and read sequences.
Removal of section headers related to incompete programmer's manual section (future PR).

New to this PR:
-Includes minor style guide comments from #686.
-Hugofied.
-Line wrapping sorted out (Now that I finally understand the request. Many thanks to @tjaychen and @imphil, and sorry if I gave you too much grief in the meantime.)

martin-lueker · 2019-11-21T21:25:03Z

FYI: @igk8, @tunghoang290780

imphil

Thanks for preparing this PR @martin-lueker! It looks like we're almost there. I've noted a couple style nits, and please squash the commits into one to get CI green (it currently fails since not all commits have a signed-off-by line).

hw/ip/i2c/doc/_index.md

imphil · 2019-11-22T10:40:48Z

hw/ip/i2c/doc/_index.md

+
+
+This feature is not required by the I2C specification.
+However it could be used in SMBus applications to support SMBus timeouts.


Is there any other SMBus compatibility in this module? If so, this should probably be mentioned in the features section at the top.

No, but this was in response to a question by @tjaychen a while ago. I can soften this a bit though.

Hi @imphil, please take a look at the revised text to see if that is clear.

hw/ip/i2c/doc/_index.md

sjgitty

just made it about 1/3 way through, putting these comments out there. will get back to it this weekend.

hw/ip/i2c/doc/_index.md

martin-lueker · 2019-11-22T20:17:10Z

Does any one have any idea why the commit lint doesn't like my signoff?

imphil · 2019-11-22T23:58:15Z

Your name in the commit is Martin LB, while the signed off by line has your full name. Please use the -s flag to git commit to prevent such problems.

martin-lueker · 2019-11-23T01:55:48Z

Thanks @imphil, okay thanks. I saw the author as Martin LB in the log. (I have no idea where that name came from.) A --reset-author fixed it in the log, at least from my machine. It is now listed as Martin Lueker-Boden and the signoff matches. But the CI is still failing. Hmm. Is it perhaps still Martin LB on github?

martin-lueker · 2019-11-23T02:04:25Z

Thanks @imphil, okay thanks. I saw the author as Martin LB in the log. (I have no idea where that name came from.) A --reset-author fixed it in the log, at least from my machine. It is now listed as Martin Lueker-Boden and the signoff matches. But the CI is still failing. Hmm. Is it perhaps still Martin LB on github?

Okay, well I don't know why it didn't like the last push, but it seems to be working now. Thanks @imphil!

imphil

LGTM on styling and the git commit. Didn't look that much at content, assuming that's already been reviewed in the GDoc.

imphil · 2019-12-02T21:36:16Z

@sjgitty can you have a look at the content, or suggest someone to review it?

hw/ip/i2c/doc/_index.md

sjgitty

Looking good, sorry for the slow review. I have left a few comments for clean up.

hw/ip/i2c/doc/_index.md

sjgitty · 2019-12-03T22:08:08Z

hw/ip/i2c/doc/_index.md

+To read a larger byte stream, multiple 256B reads can be chained together using the RCONT flag.
+- RCONT (corresponds to FIFO inputs {{< regref FDATA.RCONT >}}, only used with READ):
+If RCONT is set, the Format Byte represents part of a longer sequence of reads, allowing for reads to be chained indefinitely.
+The RCONT flag indicates the the final byte returned with the current read


one line on two lines.

one line on two lines.

Hi @sjgitty, no problem on the delay, I understand. I think I've fixed this one, but could you please look back at this line? I couldn't see the problem (and suspect I actually fixed it a while ago, but didn't commit).

hw/ip/i2c/doc/_index.md

sjgitty · 2019-12-03T22:08:46Z

hw/ip/i2c/doc/_index.md

+should be responded to which an ACK, allowing the target to continue sending data.
+(Note that the first R-1 bytes read will still be acknowledged regardless of whether RCONT is asserted or not.)
+- START (corresponds to {{< regref FDATA.START >}}, Ignored when used with READ):
+Issue a START condition before transmitting the Format Byte on the bus.


ditto: sub-bullet for better readability?

ditto: sub-bullet for better readability?

Hi @sjgitty,
If you have a chance, take a look at the latest and see if that is what you had in mind. The current version looks nice in the render.

yep, looks good thanks

hw/ip/i2c/doc/_index.md

sjgitty · 2019-12-03T22:14:34Z

hw/ip/i2c/doc/_index.md

+If should be noted that the `scl_interference` interrupt is not raised in the case when the target device is stretching the clock.
+(However, it may be raised if the target allows SCL to go high and then pulls SCL down before the end of the current clock cycle.)
+
+Though normal clock stretching does not count as SCL interference, if the module detects that a target device has held SCL low and stretched the any given SCL cycle for more than {{< regref TIMEOUT_CTRL.VAL >}} clock ticks this will cause the stretch timeout interrupt to be asserted.


some wavejson diagrams for clock stretching would be helpful.

Good idea. I've added a couple in commit 65bc6d7

(Note will be squashed at the end of this)

hw/ip/i2c/doc/_index.md

tjaychen · 2019-12-03T22:33:41Z

hw/ip/i2c/doc/_index.md

+
+To enter override mode, the register field {{< regref OVRD.TXOVRDEN >}} is asserted by software.
+In this state the output drivers scl_tx_o and sda_tx_o are controlled directly by the register fields {{< regref OVRD.SCLVAL >}} () and {{< regref OVRD.SDAVAL >}}.
+When {{< regref OVRD.SCLVAL >}} and {{< regref OVRD.SDAVAL >}} are set high, the virtual open drain configuration will leave the output resistively pulled high, and controllable by remote targets.


i do not believe virtual open drain is mentioned prior. Would it be helpful to have a brief section talking about integration this IP into the system? And maybe mentioning virtual open drain as one flexible option in addition to real open drain.

Hi @tjaychen, that's a good point. Added in the latest commit. Check out: 53f8431
For fix.
Note to future readers: This commit will be squashed before merge.

thanks @martin-lueker ! sorry for the late reply!

hw/ip/i2c/doc/_index.md

tjaychen · 2019-12-03T22:50:56Z

hw/ip/i2c/doc/_index.md

+- T<sub>LOW</sub>: set in register {{< regref TIMING0.TLOW >}}.
+- T<sub>HIGH</sub>: set in register {{< regref TIMING0.THIGH >}}.
+- T<sub>r</sub>: set in register {{< regref TIMING1.T_R >}}.
+(Note: The rise time cannot be explicitly controlled by internal hardware, and will be a function of the capacitance of the bus.


this may just be me, but i tend to find an example or two very helpful when discussing setting of timing parameters.

Hi @tjaychen, Alright at long last, these examples are in the latest commit (21fad6f)
Please take a look, and let me know if you have any comments or suggestions.

imphil · 2019-12-23T11:38:51Z

@martin-lueker can you have a look at the review comments and do a (hopefully last) update?

igk8

I've noticed three minor points below.

Also, it would be good to use the terms "target device" or "target" throughout the document when referring to the target device. In a few places, the term "device" is used. Although one can figure out from context that "target device" is implied, it might confuse someone into thinking that "device" is either "host device" or "target device". Looks good other than that.

Thanks

hw/ip/i2c/doc/_index.md

@imphil

Translation to Markdown of https://docs.google.com/document/d/1xVGRX7c5jhQyrJ__iubYD2NmKRO7NlXYuspthZIfvfI Minor changes from Google Doc: Elaboration of timing registers. Removal of incomplete section on START, STOP, transmit and read sequences. Removal of section headers related to incompete programmer's manual section (future PR). New to this PR: -Includes minor style guide comments from lowRISC#686. -Hugofied. -Line wrapping sorted out Update 2019-11-22: - Responses to Reviews by @imphil and @sjgitty Nov. 22, 2019 Signed-off-by: Martin Lueker-Boden <martin.lueker-boden@wdc.com> Update 2020-01-10: Maybe changes in response to Dec 3. reviews. Signed-off-by: Martin Lueker-Boden <martin.lueker-boden@wdc.com>

Signed-off-by: Martin Lueker-Boden <martin.lueker-boden@wdc.com>

martin-lueker · 2020-01-11T01:01:53Z

@martin-lueker can you have a look at the review comments and do a (hopefully last) update?

Hi Phillip,
Yes, thanks for the prod. I'm added most of the fixes today. Though I still have two more requests for a wavedrom and a timing example. Hopefully I can get those in by the weekend.

sjgitty

Hey @martin-lueker is this close to landing? Would be good to get some momentum on i2c.

Looks like there are a couple of pending comments.

sjgitty · 2020-01-14T23:27:08Z

hw/ip/i2c/doc/_index.md

+should be responded to which an ACK, allowing the target to continue sending data.
+(Note that the first R-1 bytes read will still be acknowledged regardless of whether RCONT is asserted or not.)
+- START (corresponds to {{< regref FDATA.START >}}, Ignored when used with READ):
+Issue a START condition before transmitting the Format Byte on the bus.


yep, looks good thanks

Signed-off-by: Martin Lueker-Boden <martin.lueker-boden@wdc.com>

martin-lueker · 2020-01-16T07:49:20Z

Hey @martin-lueker is this close to landing? Would be good to get some momentum on i2c.

Looks like there are a couple of pending comments.

Yes it is very close. I'm just putting together an example of the timings now.

To motivate some timing parameter examples requested as part of this PR, lowRISC#1047, the programmers guide was started in order to help explain the examples. The programmers guide at this point only consists of a text description of the tuning algorithm. Also fixed a few minor typos. Signed-off-by: Martin Lueker-Boden <martin.lueker-boden@wdc.com>

martin-lueker · 2020-01-20T19:51:11Z

hw/ip/i2c/doc/_index.md

-These timings are then fed directly to the I2C state machine to control the bus timing.  
+These timing parameters are then fed directly to the I2C state machine to control the bus timing.
+
+A detailed description of the algorithm for determining these parameters--as well as a couple of concrete examples--are given in the [Programmers Guide section of this document.]({{<relref "#timing-parameter-tuning-algorithm">}})


Link to nascent Programmers Guide, with examples, as requested by @tjaychen.

martin-lueker · 2020-01-20T19:53:04Z

hw/ip/i2c/doc/_index.md

@@ -263,6 +265,109 @@ However, all transitions which are dependent on formatting flags are shown expli

 ![](I2C_state_diagram.svg)

+# Programmers guide


Programmers Guide created to hold examples of timing parameter generation. C examples to follow in a later pull request.

martin-lueker · 2020-01-20T19:59:54Z

Hey @martin-lueker is this close to landing? Would be good to get some momentum on i2c.
Looks like there are a couple of pending comments.

Yes it is very close. I'm just putting together an example of the timings now.

Alright: @tjaychen, @sjgitty, @igk8, @imphil: With 21fad6f I think we are now good.

This (fairly large) commit includes the examples which @tjaychen requested, and the beginnings of a Programmers Guide section to motivate these examples. Unless there are any other points of feedback, I think this PR is done. (Though I acknowledge with such a large commit there is a bit more new text to review, hence my choice to leave it as a new commit rather than squashing/amending it).

imphil · 2020-01-27T15:06:47Z

@tjaychen would you mind having a look at this PR again to get it merged soonish?

tjaychen · 2020-01-27T18:46:43Z

thanks for the reminder @imphil

tjaychen · 2020-01-27T18:47:16Z

I'll merge this now.

thanks @martin-lueker !

martin-lueker · 2020-01-28T20:08:03Z

Thank /you/ @tjaychen!

martin-lueker requested a review from eunchan as a code owner November 21, 2019 21:23

martin-lueker requested review from imphil and tjaychen November 21, 2019 21:24

martin-lueker mentioned this pull request Nov 21, 2019

[i2c/doc] Initial Markdown documentation for I2C #686

Closed

imphil reviewed Nov 22, 2019

View reviewed changes

sjgitty reviewed Nov 22, 2019

View reviewed changes

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

martin-lueker force-pushed the i2c_doc_redux branch 5 times, most recently from 3838b5c to b6c7c35 Compare November 22, 2019 20:08

msfschaffner self-requested a review November 22, 2019 20:45

martin-lueker force-pushed the i2c_doc_redux branch 3 times, most recently from ca89306 to ac8c39e Compare November 23, 2019 01:50

imphil approved these changes Nov 25, 2019

View reviewed changes

tjaychen reviewed Dec 3, 2019

View reviewed changes

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

sjgitty reviewed Dec 3, 2019

View reviewed changes

tjaychen reviewed Dec 3, 2019

View reviewed changes

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

tjaychen reviewed Dec 3, 2019

View reviewed changes

hw/ip/i2c/doc/_index.md Show resolved Hide resolved

tjaychen reviewed Dec 3, 2019

View reviewed changes

mdhayter mentioned this pull request Jan 1, 2020

[util] New i2csvg utility to make pictures of I2C transactions #1255

Merged

igk8 self-requested a review January 7, 2020 23:12

igk8 reviewed Jan 8, 2020

View reviewed changes

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

hw/ip/i2c/doc/_index.md Outdated Show resolved Hide resolved

martin-lueker force-pushed the i2c_doc_redux branch from ac8c39e to 523081b Compare January 10, 2020 23:49

Added section on Virtual Open Drain.

53f8431

Signed-off-by: Martin Lueker-Boden <martin.lueker-boden@wdc.com>

sjgitty reviewed Jan 14, 2020

View reviewed changes

Adding Clock stretching example Wavedroms

65bc6d7

Signed-off-by: Martin Lueker-Boden <martin.lueker-boden@wdc.com>

martin-lueker commented Jan 20, 2020

View reviewed changes

tjaychen self-requested a review January 27, 2020 18:46

tjaychen approved these changes Jan 27, 2020

View reviewed changes

tjaychen merged commit a2b1528 into lowRISC:master Jan 27, 2020

martin-lueker deleted the i2c_doc_redux branch October 18, 2020 14:58



		This feature is not required by the I2C specification.
		However it could be used in SMBus applications to support SMBus timeouts.

		@@ -263,6 +265,109 @@ However, all transitions which are dependent on formatting flags are shown expli

		![](I2C_state_diagram.svg)

		# Programmers guide

[doc] I2c doc redux #1047

[doc] I2c doc redux #1047

Conversation

martin-lueker commented Nov 21, 2019

martin-lueker commented Nov 21, 2019

imphil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-lueker Nov 22, 2019 • edited

Choose a reason for hiding this comment

sjgitty left a comment

Choose a reason for hiding this comment

martin-lueker commented Nov 22, 2019

imphil commented Nov 22, 2019

martin-lueker commented Nov 23, 2019

martin-lueker commented Nov 23, 2019

imphil left a comment

Choose a reason for hiding this comment

imphil commented Dec 2, 2019

sjgitty 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

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

imphil commented Dec 23, 2019

igk8 left a comment

Choose a reason for hiding this comment

martin-lueker commented Jan 11, 2020

sjgitty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-lueker commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-lueker commented Jan 20, 2020

imphil commented Jan 27, 2020

tjaychen commented Jan 27, 2020

tjaychen commented Jan 27, 2020

martin-lueker commented Jan 28, 2020

martin-lueker Nov 22, 2019 •

edited