drivers: spi_nrfx_spim: Fixes related to 32 Mbps speed on nRF5340 #37632

anangl · 2021-08-12T09:01:30Z

According to the nRF5340 PS, SPIM4 only supports 32 Mbps when the application core is running at 128 MHz. Drive configuration H0H1 must also be used for its pins for this speed.

Fixes #37221.

nordicjm · 2021-08-12T09:35:00Z

drivers/spi/spi_nrfx_spim.c

What if someone wants to use H1H0 on slower speeds?

I do have this open #34854, but it may need to be updated to do the init at this location instead of at driver init.

What if someone wants to use H1H0 on slower speeds?

It is not supported now by the driver, so I actually don't know what I could do about it in this PR. Do you have any suggestion?

The Kconfig option works for me, alternatively this could check the status of the pins and force them to H1H0 if they are not H1H0 but if the speed is under 32MHz then it does not change the status to S1S0 and leaves it at what it is (either S1S0 or H1H0)

somewhat related: #37572 (pinctrl could allow to set arbitrary pin settings from DT).

JordanYates · 2021-08-12T11:35:22Z

drivers/spi/spi_nrfx_spim.c

Just realised that the HAL drivers already perform this configuration in nrfx_spim_init

I'm sorry that it took me so long to get back to this one. I reworked the PR to take advantage of the pin configuration performed by the nrfx driver. Please take a look once again.

According to the nRF5340 PS, SPIM4 only supports 32 Mbps when the application core is running at 128 MHz. This patch adds the corresponding check. Signed-off-by: Andrzej Głąbek <[email protected]>

According to the nRF5340 PS, for 32 Mbps high-speed SPI using SPIM4, drive configuration H0H1 must be used. The underlying nrfx_spim driver does it properly in its initialization function, so change the shim to (re)initialize the driver when the SPI configuration is to be changed (only then the speed to use is known), to avoid the need of duplicating the corresponding code in the shim itself. Signed-off-by: Andrzej Głąbek <[email protected]>

anangl · 2021-09-09T12:01:46Z

Rebased.

Apply the same changes as the previous commit made in the spi_nrfx_spim shim, to keep these two shims aligned. Signed-off-by: Andrzej Głąbek <[email protected]>

carlescufi · 2021-09-15T09:32:07Z

@JordanYates could you please take another look?

carlescufi · 2021-09-15T09:32:30Z

@gmarull can you approve if happy please?

gmarull

I only have one comment regarding PM.

gmarull · 2021-09-15T09:53:14Z

drivers/spi/spi_nrfx_spim.c

+		/* No action needed at this point, nrfx_spim_init() will be
+		 * called at configuration before the next transfer.
+		 */


PM hooks are no longer doing what is expected, ie, put the device into operational state when resume is called. Why is this change required?

Previously, when resume was called, the device was brought to the state it was right after its initialization - it still needed to be reconfigured before the next transfer:

zephyr/drivers/spi/spi_nrfx_spim.c

Lines 336 to 338 in 7ccc1a4

ret = init_spim(dev);

/* Force reconfiguration before next transfer */

data->ctx.config = NULL;

(at the device initialization, the nrfx driver was initialized with a default configuration, which was then altered accordingly when a transfer was requested). Now the difference is that the initialization of the device does not include the initialization of the nrfx driver - this is done when the device is configured right before a transfer is performed (only then we know the frequency, SPI mode, etc. that should be used), so there is no point in calling nrfx_spim_init() here and then, right before the next transfer, calling nrfx_spim_uninit() in order to be able to call nrfx_spim_init() again.
Alternatively, I would need to store the currently used nrfx driver configuration to be able to use it here in the call to nrfx_spim_init(). Then it would be not needed to reconfigure the device before the next transfer, provided that transfer would be requested with the same configuration as the last transfer before the device was suspended. But I'm not sure if it is worth spending more RAM just to achieve this possibility.

gmarull · 2021-09-15T09:57:17Z

drivers/spi/spi_nrfx_spi.c

-		data->ctx.config = NULL;
+		/* No action needed at this point, nrfx_spi_init() will be
+		 * called at configuration before the next transfer.
+		 */


same as previous comment

carlescufi · 2021-09-22T10:36:02Z

@JordanYates please take another look

anangl added bug The issue is a bug, or the PR is fixing a bug area: Drivers area: SPI SPI bus platform: nRF Nordic nRFx labels Aug 12, 2021

anangl requested review from JordanYates, carlescufi and nika-nordic August 12, 2021 09:01

anangl requested a review from tbursztyka as a code owner August 12, 2021 09:01

nordicjm reviewed Aug 12, 2021

View reviewed changes

nika-nordic approved these changes Aug 12, 2021

View reviewed changes

JordanYates approved these changes Aug 12, 2021

View reviewed changes

JordanYates requested changes Aug 12, 2021

View reviewed changes

zephyrbot requested a review from ioannisg August 19, 2021 12:41

zephyrbot assigned ioannisg Aug 19, 2021

carlescufi requested a review from JordanYates September 8, 2021 09:47

anangl added 2 commits September 9, 2021 13:59

drivers: spi_nrfx_spim: Fix limiting of SPIM frequency on nRF5340

6c86817

According to the nRF5340 PS, SPIM4 only supports 32 Mbps when the application core is running at 128 MHz. This patch adds the corresponding check. Signed-off-by: Andrzej Głąbek <[email protected]>

anangl force-pushed the fix_max_spim_freq_on_nrf5340 branch from b618963 to 60e3d95 Compare September 9, 2021 12:01

anangl force-pushed the fix_max_spim_freq_on_nrf5340 branch from 60e3d95 to 73c8c5c Compare September 9, 2021 12:02

anangl requested a review from nika-nordic September 9, 2021 12:08

drivers: spi_nrfx_spi: Change the way the nrfx driver is configured

4e1cf36

Apply the same changes as the previous commit made in the spi_nrfx_spim shim, to keep these two shims aligned. Signed-off-by: Andrzej Głąbek <[email protected]>

anangl force-pushed the fix_max_spim_freq_on_nrf5340 branch from 73c8c5c to 4e1cf36 Compare September 9, 2021 12:14

gmarull reviewed Sep 15, 2021

View reviewed changes

nika-nordic approved these changes Sep 20, 2021

View reviewed changes

JordanYates approved these changes Sep 22, 2021

View reviewed changes

nashif merged commit 36451e6 into zephyrproject-rtos:main Sep 23, 2021

anangl deleted the fix_max_spim_freq_on_nrf5340 branch September 24, 2021 12:22

anangl mentioned this pull request Sep 24, 2021

SPI4 on nRF5340 not working when using k_sleep() in main #35945

Closed

JordanYates mentioned this pull request Jan 31, 2022

spi: nRF HAL driver asserts when PM is used #42299

Closed

	ret = init_spim(dev);
	/* Force reconfiguration before next transfer */
	data->ctx.config = NULL;

drivers: spi_nrfx_spim: Fixes related to 32 Mbps speed on nRF5340 #37632

drivers: spi_nrfx_spim: Fixes related to 32 Mbps speed on nRF5340 #37632

Uh oh!

Conversation

anangl commented Aug 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anangl commented Sep 9, 2021

Uh oh!

carlescufi commented Sep 15, 2021

Uh oh!

carlescufi commented Sep 15, 2021

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anangl Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Sep 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

anangl Sep 16, 2021 •

edited

Loading