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

Atmel: SAM: Convert XDMAC & SSC-I2S to devicetree #24948

Merged
merged 3 commits into from
May 8, 2020

Conversation

galak
Copy link
Collaborator

@galak galak commented May 4, 2020

No description provided.

@galak galak requested review from mnkp and stephanosio May 4, 2020 20:27
@galak galak added area: Devicetree platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) labels May 4, 2020
@galak galak added this to the v2.3.0 milestone May 4, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented May 4, 2020

All checks passed.

checkpatch (informational only, not a failure)

-:183: WARNING:LONG_LINE: line over 80 characters
#183: FILE: drivers/i2s/i2s_sam_ssc.c:925:
+	dev_data->dev_dma = device_get_binding(DT_INST_DMAS_LABEL_BY_NAME(0, tx));

-:186: WARNING:LONG_LINE: line over 80 characters
#186: FILE: drivers/i2s/i2s_sam_ssc.c:927:
+		LOG_ERR("%s device not found", DT_INST_DMAS_LABEL_BY_NAME(0, tx));

- total: 0 errors, 2 warnings, 637 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

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

dts/arm/atmel/dma_atmel_sam.h Outdated Show resolved Hide resolved
dts/bindings/arm/atmel,sam-ssc.yaml Outdated Show resolved Hide resolved
Comment on lines -967 to +971
static const struct soc_gpio_pin i2s0_pins[] = {
PIN_SSC0_TK,
PIN_SSC0_TF,
PIN_SSC0_TD,
#ifdef CONFIG_I2S_SAM_SSC_0_PIN_RK_EN
PIN_SSC0_RK,
#endif
#ifdef CONFIG_I2S_SAM_SSC_0_PIN_RF_EN
PIN_SSC0_RF,
#endif
PIN_SSC0_RD,
};
static const struct soc_gpio_pin i2s0_pins[] = ATMEL_SAM_DT_PINS(0);
Copy link
Member

Choose a reason for hiding this comment

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

It's going to complicate implementation, however it's a fairly common configuration where RK, RF pins are not connected.

Screenshot from 2020-05-05 21-12-52

It seems like at the moment there is no possibility to have RK, RF signals disconnected.

Copy link
Member

Choose a reason for hiding this comment

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

I underestimated the new DT generation scheme. There is no need to change the implementation. It's enough that user defines their own pinctrl-0 property for the ssc node.

The one issue that remains is if we should provide 'default' pinctrl-0 property at the SoC level.

Atmel is a bit special, its pinmux is fairly rigid. In any given peripheral few pins, if any, can be assigned to an alternative location. But these choices exist and it feels improper to provide default configuration at the SoC level. Pin assignments are board specific and as soon as there exist a choice any default is wrong. Connecting incorrect signal to a pin can potentially damage the board.

At the same time if a peripheral doesn't allow to connect any of its pins to an alternate location forcing the user to define pinctrl-0 property at the board level also feels like an overkill.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to provide pinctrl-0 in the SoC dtsi files in the cases that either make sense or if there is only one option for a given peripheral. I'm find if because of how SSC is used if we want to move the pinctrl-0 setting to the board file instead.

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.

I've verified the PR with tests/drivers/i2s/* testcases on sam_e70_xplained board (after applying #25005). One of the tests was failing, however the error is unrelated / exists also on master.

There is one more issue: this PR uses the label "SSC_0" which is consistent with the node name. However, the tests rely on old "I2S_0" label. We could handle it also in a follow up PR.

Regarding location of pinctrl-0 property, let's discuss it at the dev-review meeting since more peripherals: e.g. uart, spi are affected.

dts/bindings/arm/atmel,sam-ssc.yaml Outdated Show resolved Hide resolved
dts/bindings/arm/atmel,sam-ssc.yaml Outdated Show resolved Hide resolved
@galak galak added the dev-review To be discussed in dev-review meeting label May 6, 2020
Convert sam_xdmac driver to utilize devicetree.  As part of the
controller binding we specify that dmas should contain a channel and the
perid for the DMA transaction.

Signed-off-by: Kumar Gala <[email protected]>
@galak galak force-pushed the sam-xdmac branch 2 times, most recently from 2e50a97 to 9346f3e Compare May 8, 2020 01:51
galak added 2 commits May 7, 2020 20:52
Convert i2s_sam_ssc driver to utilize devicetree.  We replace Kconfig
options for specifying the DMA configuration (channel, DMA device name)
with getting that from devicetree.  We also get pincfg from devicetree,
however we still have Kconfig sybmols to specify if the RF or RK pin is
enabled.

Signed-off-by: Kumar Gala <[email protected]>
All pin configuration for ATMEL SAM SoC come from devicetree so we can
now remove the soc_pinmap.h header files.

Signed-off-by: Kumar Gala <[email protected]>
@galak
Copy link
Collaborator Author

galak commented May 8, 2020

Should be updated with all the changes. Let me know if there's anything I missed.

@galak galak requested a review from mnkp May 8, 2020 01:53
@galak galak merged commit 10a5c43 into zephyrproject-rtos:master May 8, 2020
@galak galak deleted the sam-xdmac branch May 8, 2020 03:25
@pabigot pabigot removed the dev-review To be discussed in dev-review meeting label May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants