Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs/LPC_Additional_LPC_Cycles.md: document added #36

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

mgabryelski1
Copy link
Contributor

Documentation for the test of additional LPC protocol cycles.

Signed-off-by: Maciej Gabryelski [email protected]

… LPC cycles different than I/O or TPM updated

Signed-off-by: Maciej Gabryelski <[email protected]>
docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
In the implementation of the LPC (LPC Host and LPC Peripheral) protocol we wrote
earlier, only two types of LPC cycles were supported:
+ I / O cycles (Read and Write)
+ TPM cycles
Copy link

@arturkow2000 arturkow2000 Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TPM cycles actually are not supported - those are basically the same as I/O cycles but have a different START field. Peripheral checks START == b0000` before transitioning from IDLE to START.
Please also see this comment #36 (comment)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgabryelski1 This is not fixed, we can't claim we support TPM cycles when actually we don't

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These issues were corrected by me.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgabryelski1 This is still not fixed, until lpn-plant/lpntpm-lpc-verilog#3 came there was no support for TPM cycles.

docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
mgabryelski1 and others added 26 commits October 4, 2022 10:21
fixed

Co-authored-by: Artur Kowalski <[email protected]>
fixed

Co-authored-by: Artur Kowalski <[email protected]>
fixed

Co-authored-by: Artur Kowalski <[email protected]>
fixed

Co-authored-by: Artur Kowalski <[email protected]>
fixed

Co-authored-by: Artur Kowalski <[email protected]>
fixed

Co-authored-by: Artur Kowalski <[email protected]>
fixed

Co-authored-by: Artur Kowalski <[email protected]>
In the implementation of the LPC (LPC Host and LPC Peripheral) protocol we wrote
earlier, only two types of LPC cycles were supported:
+ I / O cycles (Read and Write)
+ TPM cycles

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgabryelski1 This is not fixed, we can't claim we support TPM cycles when actually we don't

docs/LPC_Additional_LPC_Cycles.md Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Show resolved Hide resolved
docs/LPC_Additional_LPC_Cycles.md Outdated Show resolved Hide resolved
Copy link

@arturkow2000 arturkow2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgabryelski1 I have fixed few things - rendering and typos. Only a few issues left, please address them.
Please check rendering before committing changes, use mkdocs serve for preview. And please shorten section headers - this doesn't render well.

Copy link

@arturkow2000 arturkow2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgabryelski1
After your last commit I/O and TPM cycles are mixed and in a few places used interchangeably.

Document describes changes done regarding the CYCTYPE field which is used to differentiate between I/O and other cycle types. There is no direct mention that TPM cycles differ only by the START field - TPM cycle is essentially an I/O cycle with START=b0101, all other cycles must be ignored (if START or CYCTYPE does not match).

Original implementation supported I/O cycles only and would not handle other cycle types properly. We added a CYCTYPE field check to skip non-I/O cycles and extended LPC Host and testbench with other cycle types, and that's fine.
The next step was to require START=b0101 to filter out what is not ment for TPM.
Please describe this step in a separate section and then provide simulation results showing that cycles with START != b0101 are skipped and that cycles with a wrong CYCTYPE are also skipped.

In the implementation of the LPC (LPC Host and LPC Peripheral) protocol we wrote
earlier, only two types of LPC cycles were supported:
+ I / O cycles (Read and Write)
+ TPM cycles

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgabryelski1 This is still not fixed, until lpn-plant/lpntpm-lpc-verilog#3 came there was no support for TPM cycles.

First, let's look at the similarities and differences between the TPM and memory
cycles. For this purpose we will look at Intel's [LPC Protocol](https://www.intel.com/content/dam/www/program/design/us/en/documents/low-pin-count-interface-specification.pdf) reference document.
See screen shots from this document. First we have description of TPM cycles
![LPC IO Cycles](images/TPMReadCycle.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both images have gray borders on side and black dots (cut text) on the bottom, please fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants