Skip to content

Conversation

@chrta
Copy link
Contributor

@chrta chrta commented Oct 28, 2019

This pull request adds SPI driver and its bindings using the USART peripheral for Silicon Labs EFM32 and EFR32 MCUs. It is a very basic driver which uses polling.

To actually use this driver, the SPI interface to the on-board flash MX25R8035F on the board efr32mg_sltb004a is also added.

The driver was tested by running the sample samples/drivers/spi_flash.

@zephyrbot
Copy link

zephyrbot commented Oct 28, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

tiny style issue, get an empty line before the if (it's valid if only the previously assigned variable is tested in the if)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

+-----------+------------+-------------------------------------+
| I2C | on-chip | i2c port-polling |
+-----------+------------+-------------------------------------+
| SPI(M) | on-chip | spi port-polling |
Copy link
Contributor

@dbkinder dbkinder Oct 28, 2019

Choose a reason for hiding this comment

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

What's the significance of the (M) notation? Master? Can this just be SPI or if the M is important it should be explained?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it means master. Slave-mode is not implemented by this driver. I copied the notation from https://docs.zephyrproject.org/latest/boards/arm/nrf52840_pca10059/doc/index.html#supported-features which contains SPI (M/S).
I do not know what the preferred way for this is. I can also remove the (M).

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think technical folks will know what this means, then it's OK by me. Otherwise, maybe say ```SPI Master`` to be clear.

Copy link
Contributor

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

I get the following with builds. Seems like some extra checking that isn't quite right in this case (@ulfalizer?).

-- Loading /mnt/devel/external/zp/zephyr/boards/arm/efr32mg_sltb004a/efr32mg_sltb004a.dts as base
efr32mg_sltb004a.dts.pre.tmp:73.26-81.5: Warning (spi_bus_bridge): /soc/usart@40010800: node name for SPI buses should be 'spi'
  also defined at efr32mg_sltb004a.dts.pre.tmp:280.9-298.3
efr32mg_sltb004a.dts_compiled: Warning (spi_bus_reg): Failed prerequisite 'spi_bus_bridge'

If you go into samples/drivers/spi_flash/boards and copy one of the existing conf files to efr32mg_sltb004a.conf this will verify the flash. It works for me.

I also added configuration to tests/subsys/fs/littlefs and confirmed to my satisfaction that it would work, except that this flash is too small to support the three test partitions the test assumes. Once this goes in I can update it to skip the large partition.

@chrta chrta force-pushed the efm32_add_spi branch 2 times, most recently from eb85a8f to 8cf0bda Compare October 29, 2019 08:33
@chrta chrta removed the DNM This PR should not be merged (Do Not Merge) label Nov 4, 2019
Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

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

@chrta Thanks, looks good with some minor comments. If you are willing to add support also for efr32_slwstk6061a board I will be happy to test it.

@chrta
Copy link
Contributor Author

chrta commented Nov 11, 2019

@mnkp Thanks for your review. I addressed all your comments and also added (untested) support for the efr32_slwstk6061a board.

Copy link
Contributor

@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.

Looks ok to me on driver side

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

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

@chrta Thanks, works very well! Verified on efr32_slwstk6061a board.

@mnkp
Copy link
Member

mnkp commented Nov 12, 2019

Actually, I have one more comment. Since Default board configuration guidelines generally require to configure all major components available on the evaluation board maybe we should ensure that spi_nor driver is enabled if flash subsystem is enabled. We could add respective section to the board configuration file, as in boards/arm/efr32_slwstk6061a/Kconfig.defconfig. Doing it manually is a bit tedious and may not be obvious to the new users.

@chrta
Copy link
Contributor Author

chrta commented Nov 12, 2019

Doing it manually is a bit tedious and may not be obvious to the new users.

I agree. Since no other board is doing it, i did not do it either. I would now add this to the Kconfig.defconfig of the two efr boards, ok @mnkp ?

if SPI_GECKO

config SPI_NOR
       default y

endif # SPI_GECKO

@chrta
Copy link
Contributor Author

chrta commented Nov 14, 2019

This pull request now targets master branch. I also cleaned up the reviewers that were added automatically by github .

@chrta
Copy link
Contributor Author

chrta commented Dec 17, 2019

@galak Is this now ready to merge?

Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

please fix the checkpatch warnings

@chrta
Copy link
Contributor Author

chrta commented Dec 19, 2019

please fix the checkpatch warnings

This is the code that produces the warning:

#ifndef CONFIG_SOC_GECKO_HAS_INDIVIDUAL_PIN_LOCATION
#error "This EFM32 USART SPI driver is only implemented for devices that \
 support individual pin locations"
#endif

Could you suggest a fix. I could also write "Not implemented for this SoC", but it is less detailed.

@MaureenHelm
Copy link
Member

please fix the checkpatch warnings

This is the code that produces the warning:

#ifndef CONFIG_SOC_GECKO_HAS_INDIVIDUAL_PIN_LOCATION
#error "This EFM32 USART SPI driver is only implemented for devices that \
 support individual pin locations"
#endif

Could you suggest a fix. I could also write "Not implemented for this SoC", but it is less detailed.

How about: "Individual pin location support is required"

chrta added 3 commits January 10, 2020 09:42
This commit adds SPI driver and its bindings using the USART peripheral
for Silicon Labs EFM32 and EFR32 MCUs.

Signed-off-by: Christian Taedcke <[email protected]>
This commit adds support for the on-board flash MX25R8035F that is
directly connected to the efr32mg soc.

Signed-off-by: Christian Taedcke <[email protected]>
This commit adds support for the on-board flash MX25R8035F that is
directly connected to the efr32fg soc.

Signed-off-by: Christian Taedcke <[email protected]>
@chrta
Copy link
Contributor Author

chrta commented Jan 10, 2020

@MaureenHelm Changed error message to your suggestion.

@chrta
Copy link
Contributor Author

chrta commented Jan 10, 2020

Shippable failed because of some unrelated failed test. I also rebased this pr onto the current master. The failed test:

2020-01-10 09:36:23,730 - INFO - 814/814 qemu_x86_64               tests/lib/mem_alloc/libraries.libc.minimal.mem_alloc FAILED Exited with 1 (qemu)

@MaureenHelm MaureenHelm merged commit 20aa2bc into master Jan 10, 2020
@MaureenHelm MaureenHelm deleted the efm32_add_spi branch January 10, 2020 13:14
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.

8 participants