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

spi: sam0: use Device Tree for configuration. #5865

Merged
merged 0 commits into from Mar 5, 2018

Conversation

nzmichaelh
Copy link
Collaborator

@nzmichaelh nzmichaelh commented Jan 28, 2018

Define a Device Tree binding for the SAM0 SPI and use it instead of
Kconfig for enabling / disabling instances

Signed-off-by: Michael Hope mlhx@google.com

@codecov-io
Copy link

codecov-io commented Jan 28, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5865   +/-   ##
=======================================
  Coverage   53.08%   53.08%           
=======================================
  Files         430      430           
  Lines       41040    41040           
  Branches     7908     7908           
=======================================
  Hits        21785    21785           
  Misses      16032    16032           
  Partials     3223     3223

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 cf07caa...edd22b9. Read the comment docs.

@galak
Copy link
Collaborator

galak commented Jan 31, 2018

You could merge this with #5731

@nzmichaelh nzmichaelh changed the title [WIP] spi: sam0: use Device Tree for configuration. spi: sam0: use Device Tree for configuration. Feb 6, 2018
@nzmichaelh
Copy link
Collaborator Author

@galak @tbursztyka this is now ready for review.

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

We need to rework how we deal w/SPI & UART since they are all just modes of SERCOM.

I think we should have a node per SERCOM, have multiple compatibles in the node. One for “atmel,sercom”, and one for “atmel,sercom-spi”, “atmel,sercom-i2c” or “atmel,sercom-uart”. Not sure if we should expand the compats to match the all the modes (CTRLA.MODE) can be set to.

@galak
Copy link
Collaborator

galak commented Feb 14, 2018

@nzmichaelh any updates here?

@nzmichaelh
Copy link
Collaborator Author

Hey @galak I'm happy to rework this but I'll need some pointers on Device Tree :) The peripheral reset and clock configuration code is basically the same for all SERCOMs. The peripheral is pretty simple so the actual serial, SPI, etc drivers are dominated by how different the Zephyr API is.

@nzmichaelh
Copy link
Collaborator Author

@galak let me know what the changes would look like and I'll start working on them

@nzmichaelh
Copy link
Collaborator Author

I've also rebased onto master.

@galak galak added platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) area: SPI SPI bus area: Devicetree labels Feb 21, 2018
@nzmichaelh
Copy link
Collaborator Author

Hi @galak I've rebased this and updated it for the Trinket M0.

re: the many modes of SERCOM, I had a look around the code and couldn't find an example of how to do support multiple modes of one peripheral. Could you tell me how, else could we land this patch and merge the SERCOM modes in a follow up?

@galak
Copy link
Collaborator

galak commented Feb 23, 2018

Hi @galak I've rebased this and updated it for the Trinket M0.

re: the many modes of SERCOM, I had a look around the code and couldn't find an example of how to do support multiple modes of one peripheral. Could you tell me how, else could we land this patch and merge the SERCOM modes in a follow up?

I need to play around with this, the straight forward method is to modify the compatible in the board .dts file to set it to uart or spi. I need to chat with a few people to see if there are examples of how this should be handled.

@galak
Copy link
Collaborator

galak commented Feb 27, 2018

Ok, after a little discussion I think the compatible solution is the way to go. So I think we should have sercom nodes in dts/arm/atmel/samd21.dtsi - w/o any compatible. In the board.dts we than add the compatible in addition to status = "ok";

So something like:

&sercom0 {
compatible = "atmel,sam0-uart";
status = "ok";
current-speed = <115200>;
};

&sercom4 {
compatible = "atmel,sam0-spi";
status = "ok";
};

Make sense?

@nzmichaelh
Copy link
Collaborator Author

@galak done. It turned out pretty clean - the Device Tree generator uses the compatible string as part of the #define, so I didn't need to make any other changes.

Copy link
Collaborator

@ydamigos ydamigos left a comment

Choose a reason for hiding this comment

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

DT node inheritance is not clear to me. For example, the interrupt property exists in atmel,sam0-uart.yaml but I cannot find it in atmel,sam0-spi.yaml. Does SAM0 spi driver support interrupt?

sercom0: uart@42000800 {
compatible = "atmel,sam0-uart";
sercom0: sercom@42000800 {
compatible = "atmel,sam0-sercom";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't find this constrain in any yaml file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@nzmichaelh
Copy link
Collaborator Author

re: sercom, uart, and spi: the SPI driver doesn't use interrupts as the SAM0 has no FIFO so it's faster and cheaper to use polling.

re: inheritance, it seems the Device Tree generator doesn't support multiple inheritance so, for example, the sam0-uart binding can't inherit from both uart.yaml and atmel,sam0-sercom.yaml.

@ydamigos
Copy link
Collaborator

ydamigos commented Mar 1, 2018

re: sercom, uart, and spi: the SPI driver doesn't use interrupts as the SAM0 has no FIFO so it's faster and cheaper to use polling.
re: inheritance, it seems the Device Tree generator doesn't support multiple inheritance so, for example, the sam0-uart binding can't inherit from both uart.yaml and atmel,sam0-sercom.yaml.

I propose to remove atmel,sam0-sercom.yaml, compatible = "atmel,sam0-sercom"; and declare sercomX node in samd21.dtsi:

sercomX: sercom@address {
			reg = <address 0x40>;
			status = "disabled";
			label = "SERCOM0";
};

In board's dtsi we add uart as:

sercomΧ {
 	compatible = "atmel,sam0-uart";
 	current-speed = <115200>;
 	interrupts = <IRQn 0>;
	status = "ok";
};

and SPI as you already did.

In board level, you could use aliases and override label property (e.g. SPI_1, UART_1) to make things more clear.

WDYT?

If SPI HW uses interrupts we could leave interrupts property in sercomX nodes in samd21.dtsi file. The SPI driver won't use them.

@nzmichaelh
Copy link
Collaborator Author

The SPI HW does support interrupts so leaving them in samd21.dtsi SGTM. They're just not used as the driver is master only.

@henrikbrixandersen
Copy link
Member

@ydamigos SGTM. Looking forward to seeing this land in the tree.

@henrikbrixandersen
Copy link
Member

The SPI loopback config for the Arduino Zero (tests/drivers/spi/spi_loopback/boards/arduino_zero.conf) will need to be updated as well (replacing SPI4 with SERCOM4).

@galak galak changed the base branch from master to arm March 5, 2018 18:01
@galak galak merged this pull request into zephyrproject-rtos:arm Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree area: SPI SPI bus platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants