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

[TW#16843] spi_slave.c #1346

Closed
reklof opened this Issue Dec 3, 2017 · 9 comments

Comments

Projects
None yet
6 participants
@reklof
Copy link

reklof commented Dec 3, 2017

The code for setting the hardware according to the 'mode' parameter is incorrect. mode == 2 and mode == 3 should swap (see spi_master.c)

if (slave_config->mode == 0) {
spihost[host]->hw->pin.ck_idle_edge = 0;
spihost[host]->hw->user.ck_i_edge = 1;
spihost[host]->hw->ctrl2.miso_delay_mode = nodelay ? 0 : 2;
} else if (slave_config->mode == 1) {
spihost[host]->hw->pin.ck_idle_edge = 0;
spihost[host]->hw->user.ck_i_edge = 0;
spihost[host]->hw->ctrl2.miso_delay_mode = nodelay ? 0 : 1;
} else if (slave_config->mode == 2) {
spihost[host]->hw->pin.ck_idle_edge = 1;
spihost[host]->hw->user.ck_i_edge = 0;
spihost[host]->hw->ctrl2.miso_delay_mode = nodelay ? 0 : 1;
} else if (slave_config->mode == 3) {
spihost[host]->hw->pin.ck_idle_edge = 1;
spihost[host]->hw->user.ck_i_edge = 1;
spihost[host]->hw->ctrl2.miso_delay_mode = nodelay ? 0 : 2;
}

@FayeY FayeY changed the title spi_slave.c [TW#16843] spi_slave.c Dec 6, 2017

@Spritetm

This comment has been minimized.

Copy link
Member

Spritetm commented Dec 9, 2017

Can I ask how you figured out this is incorrect? Does it not match other documentation, or do you have issues using mode 2 or 3 in practice?

@jlujan-na

This comment has been minimized.

Copy link

jlujan-na commented Jan 5, 2018

The driver code does not match Table 27: Clock Polarity and Phase, and Corresponding SPI Register Values for SPI Slave from the technical reference.

Registers mode0 mode1 mode2 mode3
SPI_CK_IDLE_EDGE 0 0 1 1
SPI_CK_I_EDGE 0 1 1 0
SPI_MISO_DELAY_MODE 0 0 0 0
SPI_MISO_DELAY_NUM 0 0 0 0
SPI_MOSI_DELAY_MODE 2 1 1 2
SPI_MOSI_DELAY_NUM 0 0 0 0

It also has hardcoded nodelay=true;

bool nodelay = true;

@jlujan-na

This comment has been minimized.

Copy link

jlujan-na commented Jan 5, 2018

There is also an ambiguous assignment

spihost[host]->hw->slave.sync_reset = 1;

@negativekelvin

This comment has been minimized.

Copy link
Contributor

negativekelvin commented Jan 5, 2018

It seems more likely the table row SPI_CK_I_EDGE is wrong and values should be inverted bc that would match all code. But nodelay should be false to match table.

sync_reset is not ambiguous, it is set and then cleared intentionally.

@jlujan-na

This comment has been minimized.

Copy link

jlujan-na commented Jan 5, 2018

The mode configuration in spi_master.c matches the table.

sync_reset is not ambiguous, it is set and then cleared intentionally.

I see

@negativekelvin

This comment has been minimized.

Copy link
Contributor

negativekelvin commented Jan 5, 2018

Well if table is right then all values of ck_i_edge should be inverted in code. But not swap 2&3.

@jlujan-na

This comment has been minimized.

Copy link

jlujan-na commented Jan 5, 2018

I think the code should also be setting mosi_delay_mode instead of miso_delay_mode. Would make the code to match the tech ref:

if (slave_config->mode == 0) {
        spihost[host]->hw->pin.ck_idle_edge = 0;
        spihost[host]->hw->user.ck_i_edge = 0;
        spihost[host]->hw->ctrl2.mosi_delay_mode = 2;
    } else if (slave_config->mode == 1) {
        spihost[host]->hw->pin.ck_idle_edge = 0;
        spihost[host]->hw->user.ck_i_edge = 1;
        spihost[host]->hw->ctrl2.mosi_delay_mode = 1;
    } else if (slave_config->mode == 2) {
        spihost[host]->hw->pin.ck_idle_edge = 1;
        spihost[host]->hw->user.ck_i_edge = 1;
        spihost[host]->hw->ctrl2.mosi_delay_mode = 1;
    } else if (slave_config->mode == 3) {
        spihost[host]->hw->pin.ck_idle_edge = 1;
        spihost[host]->hw->user.ck_i_edge = 0;
        spihost[host]->hw->ctrl2.mosi_delay_mode = 2;
    }
@WolfWalter

This comment has been minimized.

Copy link

WolfWalter commented Jan 9, 2018

I checked the MISO for a mode3 spi-slave with an oscilloscope. To fix the timing I needed to change spihost[host]->hw->ctrl2.mosi_delay_mode to 1.

@ginkgm

This comment has been minimized.

Copy link
Collaborator

ginkgm commented Feb 27, 2018

As far as I know, the table given by the Technical Reference Manual can produce correct timing. But sometimes it does not work well with DMA, so there're 2 modes with different values with the TRM.

Please try:

  1. If the current settings work for your design, suggest ignore the timing. Due to the output delay, the SPI still works well for most cases.
  2. If you want to fix the timing, first set the register as the TRM described. If everything is well, skip step 3 and 4.
  3. Please note the slave readings may get corrupted due to a DMA issue. If unfortunately your reading is not correct, to see if this is the reason, initialize without DMA and retry.
  4. You have 2 ways to get around this issue:
    1. use other SPI modes, currently mode 0 and 2 (or maybe 1 and 3, I can't remember the detail.) cannot work with DMA well, but the other 2 modes do.
    2. turn off the DMA.

I prepare to refactor the code to make the workaround more obviously and influence smaller region.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.