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.

Sign up for GitHub

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

Jump to bottom

[Spi_host] Test a Macronix flash #18977

Merged
merged 3 commits into from
Jun 29, 2023
Merged

[Spi_host] Test a Macronix flash #18977

merged 3 commits into from
Jun 29, 2023

Conversation

engdoreis
Copy link
Contributor

@engdoreis engdoreis commented Jun 20, 2023 •
edited
Loading

This PR only add a new spi_host test. The test execute various commands of a Macronix flash on the BoB.

@engdoreis engdoreis requested a review from a team as a code owner June 20, 2023 10:18
@engdoreis engdoreis requested review from moidx, pamaury, johngt, nbdd0121, marnovandermaas, jwnrt, matutem and a-will and removed request for a team June 20, 2023 10:18
pamaury
pamaury approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@pamaury pamaury left a comment

Choose a reason for hiding this comment

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

Looks good to me except for the commented tests.

@johngt johngt requested a review from cfrantz June 21, 2023 09:08
johngt

This comment was marked as outdated.

Sign in to view

jwnrt
jwnrt approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@jwnrt jwnrt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM as far as I understand the test, only comments on the style but don't let them block this PR.

sw/device/tests/spi_host_flash_test_impl.h Outdated Show resolved Hide resolved
sw/device/tests/spi_host_flash_test_impl.c Outdated Show resolved Hide resolved
johngt
johngt previously approved these changes Jun 21, 2023
View reviewed changes
@johngt johngt self-requested a review June 21, 2023 10:42
@johngt johngt dismissed their stale review June 21, 2023 10:43

Error in process

johngt
johngt reviewed Jun 21, 2023
View reviewed changes
Copy link

@johngt johngt left a comment

Choose a reason for hiding this comment

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

Per previous comment - SGTM

HU90m
HU90m approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@HU90m HU90m left a comment

Choose a reason for hiding this comment

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

Looks good

As james mentioned, it would be nice to fix the docstring mismatch (addr_width vs addressing_width).

@engdoreis engdoreis requested a review from johngt June 21, 2023 13:44
@engdoreis engdoreis mentioned this pull request Jun 21, 2023
Closed
2 tasks
a-will
a-will reviewed Jun 21, 2023
View reviewed changes
sw/device/tests/spi_host_flash_test_impl.c Outdated
Comment on lines 132 to 133
addr_width = kDifSpiHostWidthDual;
case 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's intentional, because the case 1 is the value initialized, but I just realized that it would be much clearer if I remove the case 1.

@engdoreis engdoreis force-pushed the spi_host_flash_tests branch 2 times, most recently from 4e99d0a to e6c13c8 Compare June 22, 2023 09:02
cfrantz
cfrantz reviewed Jun 25, 2023
View reviewed changes
sw/device/tests/spi_host_macronix_flash_test.c Outdated Show resolved Hide resolved
sw/device/tests/spi_host_macronix_flash_test.c Outdated
Comment on lines 90 to 62
kPageQuadProgramOpcode = 0x38,
// The Macronix flash requires that the addr in sent using 4 lanes as the
// data when issuing the `kPageQuadProgramOpcode` operation.
kPageQuadProgramAddrWidth = 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this style of SPI flash IO is known as 1-4-4 mode (1 lane for the opcode, 4 lanes for the address phase and 4 lanes for the data phase). Support for this particular configuration should be described by the SFDP (and similarly, support for the 1-1-4 style used by the Winbond part should be described by its SFDP).

For the purpose of these tests, I don't think we need to be parsing the SFDPs, I just wanted to fill in a bit of background information.

The only thing you might want to change is the comment to use the SPI terminology:

// The Macronix flash `4PP` opcode operates in 1-4-4 mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the information! I changed the comment.

@engdoreis
[spi_host, testutils] Make the address width configurable
e3133ae 
 Some flash when performing the  page program in quad mode
 requires the address to also be sent using the four lanes.
 This commit makes this configurable.

Signed-off-by: Douglas Reis <[email protected]>
@engdoreis
[spi_host, testutils] Add a function setup pinmux
0454b5c 
 This commit add a function to connect spi_host1 to the BoB via
 pinmux.

Signed-off-by: Douglas Reis <[email protected]>
@engdoreis
[spi_host, flash] Add test for a macronix flash
bd5dba8 
Signed-off-by: Douglas Reis <[email protected]>
@engdoreis engdoreis requested review from a-will and cfrantz June 29, 2023 08:29
marnovandermaas
marnovandermaas approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Approve based on brief look at code and based on the reviews from other people in the software team.

@marnovandermaas marnovandermaas merged commit 21cebb3 into lowRISC:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwnrt jwnrt jwnrt approved these changes

@HU90m HU90m HU90m approved these changes

@marnovandermaas marnovandermaas marnovandermaas approved these changes

@pamaury pamaury pamaury approved these changes

@moidx moidx Awaiting requested review from moidx moidx is a code owner automatically assigned from lowRISC/ot-c-cpp-reviewers

@nbdd0121 nbdd0121 Awaiting requested review from nbdd0121

@matutem matutem Awaiting requested review from matutem

@johngt johngt Awaiting requested review from johngt

@a-will a-will Awaiting requested review from a-will

@cfrantz cfrantz Awaiting requested review from cfrantz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@engdoreis @pamaury @johngt @cfrantz @a-will @marnovandermaas @HU90m @jwnrt