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

drivers: flash: merger Generic SPI Flash driver #7561

Closed

Conversation

findlayfeng
Copy link
Contributor

Merged #5456.
Update spi api.
Add the CS/gpio logic.
Independent of spi flash config.

@findlayfeng findlayfeng requested a review from nashif as a code owner May 16, 2018 01:46
@findlayfeng
Copy link
Contributor Author

#7532

@codecov-io
Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #7561 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7561   +/-   ##
=======================================
  Coverage   53.22%   53.22%           
=======================================
  Files         217      217           
  Lines       26668    26668           
  Branches     5914     5914           
=======================================
  Hits        14193    14193           
  Misses      10059    10059           
  Partials     2416     2416

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51c7b5f...ca75a7e. Read the comment docs.

Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

Seems fine to me (remove the 'etc' in last patch's commit title).

Note that you don't have to close and create a new PR each time you make changes to your patches: a git push -f will just do, and all will be updated in existing PR.

@findlayfeng
Copy link
Contributor Author

嗯,我尝试过push -f但是,不清楚是因为有三次commit 还是因为是一个空格改制表符的空提交,一直成功但没有被修改。正好提示有一个不兼容的合并,于是我就新拉取了一个分支做处理。
Below from Google Translate
Well, I tried push -f , and it's unclear whether there were three commit or because it was an empty commit of a space change tab, it was successful but not modified. Just in case there is an incompatible merge, so I took a new branch to do the processing.

@findlayfeng findlayfeng force-pushed the fix_spi_flash branch 2 times, most recently from 2bcf0a4 to cf3b296 Compare May 17, 2018 00:34
@findlayfeng findlayfeng force-pushed the fix_spi_flash branch 3 times, most recently from 23ad35f to 10994fe Compare June 4, 2018 02:13
@findlayfeng
Copy link
Contributor Author

@tbursztyka Made more changes in this patch

@nashif nashif added this to the v1.13.0 milestone Jun 6, 2018
Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

you will need to rebase and update that against latest SPI API. Also, add support for CS GPIO (see existing code such as spi_flash_w25qxxdv.c)

default ""

config SPI_FLASH_SFDP_INIT_PRIORITY
int
Copy link
Collaborator

Choose a reason for hiding this comment

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

no prompt for that one?

Flash Discoverable Parameters (SFDP).

config SPI_FLASH_SFDP_SPI_NAME
string
Copy link
Collaborator

Choose a reason for hiding this comment

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

get rid of prompt and put the prompt message here.

phdr[i].major_ver, phdr[i].minor_ver,
get_phdr_addr(&phdr[i]),
phdr[i].length);
switch (get_phdr_id(&phdr[i])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why a switch if there is only 1 case?

wait_for_flash_idle(dev);
i = 0;
hdr[i++] = data->read_opcode;
if (data->addr_len == 4) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add an empty line before

if (data->addr_len == 4) {
hdr[i++] = offset >> 24;
}
hdr[i++] = offset >> 16;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

hdr[i++] = offset >> 16;
hdr[i++] = offset >> 8;
hdr[i++] = offset;
r = spi_transceive(&data->config, txbufs, ARRAY_SIZE(txbufs),
Copy link
Collaborator

Choose a reason for hiding this comment

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

same


SYS_LOG_DBG("write: @%x len %x, opcode %x (page size %d)",
offset, len,
data->write_opcode, data->page_size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

hum, you can put that on previous line


i = 0;
hdr[i++] = data->write_opcode;
if (data->addr_len == 4) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add an empty line before (basicall: if assigned value is not used in the if condition: break a line).

if (data->addr_len == 4) {
hdr[i++] = offset >> 24;
}
hdr[i++] = offset >> 16;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here (break a line if it's not another '}' etc...)

@findlayfeng
Copy link
Contributor Author

findlayfeng commented Jun 15, 2018

88688ce 来自 Merged #5456,我只是让它变得可合并而已,我在 ff2e7ec 中做了自己的工作,包括gpio_cs,几乎重写了。
From google translate
88688ce from Merged #5456, I just made it mergeable, I did my work in ff2e7ec, including gpio_cs, which was almost rewritten.
@tbursztyka

@findlayfeng findlayfeng force-pushed the fix_spi_flash branch 3 times, most recently from 020b1f0 to 88c6f2e Compare June 27, 2018 09:51
#define CONFIG_SPI_FLASH_SMPT_SIZE 16
#endif

typedef u32_t dword_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this typedef? Seems unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dword 是SFDP文档里定义的最小组成单位
From Google Translate
Dword is the smallest unit of definition defined in the SFDP document

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok

@@ -1,558 +1,993 @@
/*
* Copyright (c) 2017 Crypta Labs Ltd
* Copyright (c) 2018 Findlay Feng
Copy link
Collaborator

Choose a reason for hiding this comment

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

Merge this specific modification of sfdp to your first commit.

And this actual commit becomes one for adding microchip table support then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it

.frequency = CONFIG_SPI_FLASH_SFDP_SPI_FREQ_0,
.operation = SPI_WORD_SET(8),
};
#if defined(SPI_FLASH_GPIO_SPI_CS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not a Kconfig option for this? So it would be CONFIG_ prefixed then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh...
This is a lapse

@findlayfeng findlayfeng force-pushed the fix_spi_flash branch 4 times, most recently from b16d060 to 6068e8a Compare June 29, 2018 07:37
@fabled
Copy link
Collaborator

fabled commented Aug 4, 2018

Looks interesting and seems to supercede my work. However, is dual/quad supported tested? I don't see any existing SPI driver supporting them, and my CEC QMSPI driver cannot support it until the descriptors are setup properly to indicate direction correctly - in these modes the data flows one direction only per cycle and the code and SPI API needs to support signalling which bytes are to be written and which are to be read.

@fabled
Copy link
Collaborator

fabled commented Aug 4, 2018

Also overriding the configuration SPI freq does not seem right. This happens for reading SFDP table, and when microchip flash is detected.

@findlayfeng
Copy link
Contributor Author

I only tried the standard spi, it is estimated that the dual spi is working, the quad spi still needs some extra work, I have left a lot of todo, and can improve it when needed.
Currently I don't have the right hardware to test.

@findlayfeng
Copy link
Contributor Author

There is a definition of the read frequency that should be used in the SFDP document. This frequency is not necessarily the same as the one specified in the chip documentation.

@findlayfeng
Copy link
Contributor Author

I use the nrf5x spi driver, the memory address of the configuration information does not change without modifying the actual configuration of the spi. . .

@fabled
Copy link
Collaborator

fabled commented Aug 4, 2018

It is still normal that lower frequency is needed to be used. I think the configuration provided frequency should be honored. I rebased my spi sfdp driver on top of current head just to get everything I needed working - e.g. seems the microchip global unlock is not yet supported properly.

@findlayfeng
Copy link
Contributor Author

My idea about the frequency is that when the device is not found, the speed specified by SFDP can be viewed at https://www.jedec.org/system/files/docs/JESD216B.pdf. Switch to the rate supported by the target device after the type.
In addition, the global unlocking problem of the Microchip chip, which is tested here, works fine.
Here, the Microchip chip only provides the global unlock command, but there is no fast global lock operation. Here I will call the unlock command the first time, then the software will control whether it is allowed to be written.

@findlayfeng
Copy link
Contributor Author

page 3

4.4 Clock Rate
SFDP compliant devices must support 50 MHz operation for the Read SFDP command (instruction 5Ah).
Devices may support a wider frequency range, but a controller can always run SFDP cycles at 50 MHz or
less and get valid results. 

fabled and others added 3 commits November 2, 2018 16:41
This drivers supports chips with Serial Flash Discoverable
Parameters (SFDP) information.

Add Kconfig.spi
Add spi_flash_sfdp.c
Add spi_flash_sfdp.h
Support spi dual mode
Partial support spi quad mode

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
Signed-off-by: Findlay Feng <i@fengch.me>
Supports Microchip SFDP table

Add Kconfig.spi.microchip
Add spi_flash_sfdp_microchip.h
Add spi_flash_sfdp_microchip.c

Signed-off-by: Findlay Feng <i@fengch.me>
id SPI_FLASH_0_ID;
name CONFIG_SPI_FLASH_DRV_NAME;

Signed-off-by: Findlay Feng <i@fengch.me>
@galak
Copy link
Collaborator

galak commented Sep 19, 2019

We have a spi_nor driver in tree. Please update any features like SFDP to that driver. Closing this PR at this point.

@galak galak closed this Sep 19, 2019
@findlayfeng findlayfeng deleted the fix_spi_flash branch April 19, 2023 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants