drivers: dma: microchip: Introduce G1 DMA Driver - #96300
Conversation
zephyrbot
added
platform: Microchip MEC
There was a problem hiding this comment.
A good number of things I see that need sorting out, might see some more things once the noise is cut down a bit. If you are using an LLM to generate some stuff, I recommend not doing so (it appears like you might, as there's a lot of redundent commentary in my opinion).
| * | ||
| * Represents the different states a DMA channel can be in during its lifecycle. | ||
| */ | ||
| typedef enum dma_mchp_ch_state { |
There was a problem hiding this comment.
no need to typedef this enum
There was a problem hiding this comment.
Removed typedef as suggested.
| * This enumeration defines the possible interrupt status codes | ||
| * that indicate the outcome of a DMA transaction. | ||
| */ | ||
| typedef enum { |
There was a problem hiding this comment.
no need to typedef this enum
There was a problem hiding this comment.
Removed typedef as suggested.
| * This enumeration defines attribute types that describe hardware-specific | ||
| * constraints and capabilities for DMA transfers. | ||
| */ | ||
| typedef enum dma_mchp_attribute_type { |
There was a problem hiding this comment.
I'm seeing a pattern, but yeah like the others, don't typedef enums/structs please. We somewhat follow linux coding style and typedefs like this aren't warranted.
https://kernel.org/doc/html/latest/process/coding-style.html#typedefs
There was a problem hiding this comment.
Thanks for the reference. Removed typedef as suggested.
| { | ||
| dmac_reg->DMAC_CTRL |= DMAC_CTRL_DMAENABLE(0); | ||
| dmac_reg->DMAC_CTRL |= DMAC_CTRL_SWRST_Msk; | ||
| while ((dmac_reg->DMAC_CTRL & DMAC_CTRL_SWRST_Msk) >> DMAC_CTRL_SWRST_Pos) { |
There was a problem hiding this comment.
use the WAIT_FOR macro here with a time limit if the hardware has some guarantees on time to reset you can follow and fail on
There was a problem hiding this comment.
Used WAIT_FOR macro with timeout
| ret = 0; | ||
| } while (0); | ||
|
|
||
| if (irq_locked == true) { |
There was a problem hiding this comment.
not needed if you use goto and labels or early returns, please fix and drop do { } while(0);
There was a problem hiding this comment.
We follow MISRA C rule 15.5, which recommends having a single point of exit at the end of a function. While there are multiple ways to achieve this, we generally avoid using goto, as it’s often discouraged. Using the do { } while (0) construct is one valid way to meet this rule.
What’s your opinion on this approach within Zephyr’s coding style?
There was a problem hiding this comment.
@teburd We have already discussed the use of do { } while(0) in another PR: #93450 (comment) , which has now been approved and merged.
MISRA C:2012 Rule 15.1 – The goto statement should not be used:
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_15_01.c
MISRA C:2012 Rule 15.5 – A function should have a single point of exit at the end:
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_15_05.c
We aim to comply with both rules wherever possible, which is why we use the do { } while(0) style here.
There was a problem hiding this comment.
Zephyr is massively not compliant then already as almost everything uses goto, including the kernel in large numbers of functions.
do {} while(0) is a relatively new approach to this in the zephyr codebase, and not one that seems to add readability in my view, @nashif @kartben do we have any guidelines on these misra rules? I'd argue readability matters a great deal more and this is less readable to me at least than the much more commonly practiced style that we have throughout Zephyr.
if (some_err_branch_check) {
ret = -EIO;
goto out;
}
// more code
out
return ret;As far as I can see the rule 15.1 is "unrestricted use of goto is bad" but I don't know for certain as the rules are not published publicly.
But Misra has a few rules around usage of goto according to mathworks.
https://www.mathworks.com/help/bugfinder/ref/misrac2023rule15.2.html
https://www.mathworks.com/help/bugfinder/ref/misrac2023rule15.3.html
Why have any rules around using goto if you can't use it, makes no sense.
There was a problem hiding this comment.
I understand that the intention here is to avoid multiple exit point from a function (https://in.mathworks.com/help/bugfinder/ref/misrac2023rule15.5.html). This is an advisory Misra C rule. To avoid multiple returns, there are multiple methods.
Some are
- Restructure the logic
- encapsulate inside do-while(0) with break statement
- use goto statement.
First preference should be option 1. Second option can be option 2, do-while(0). The usage of goto is not recommended as it violates another advisory Misra C rule(https://in.mathworks.com/help/bugfinder/ref/misrac2023rule15.1.html) . Usage of goto is like violating one advisory rule to fix another advisory rule.
So I would suggest to live with multiple returns whenever do-while(0) doesn't solve the multiple return issue.
@ArunMCHP, in this particular case, i could see that the issue can be solved by option 1, by restructuring the code without using do-while. Could you please explore that option?
@teburd , the Misra rules you mentioned (15.2 and 15.3) are required rules.
I hope the zephyr will not prevent anyone from following Misra rules as long as it doesn't violates any of the zephyr coding guidelines.
There was a problem hiding this comment.
@carlescufi can we add this topic, specifically around the usage of do { } while(0) in place of goto for single exit?
There was a problem hiding this comment.
@ArunMCHP this whole conversation is absurd, let's put aside that many MISRA rules are ambiguous and there's no publicly available documentation for them, 15.1 and 15.5 are not even mentioned in https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html. So that is not an argument and even less so is the fact that you managed to drive maintainers to exhaustion in #93450 and get an instance of that merged.
What you are doing here is abusing do{} while() loops and going against the coding style and common practice of keeping the normal execution flow in line and limiting the indentation in functions, and making these drivers incoherent with the rest of the code base as well. Plus wasting reviewers time since you got told that from multiple person already.
I understand the good intentions, but this has to stop. Please fix the coding style, here and in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/clock_control/clock_control_mchp_sam_d5x_e5x.c as well. We don't need more anti-patterns in the code base.
Thanks.
There was a problem hiding this comment.
And probably drivers/serial/uart_mchp_sercom_g1.c while you are at it.
There was a problem hiding this comment.
@ArunMCHP this whole conversation is absurd, let's put aside that many MISRA rules are ambiguous and there's no publicly available documentation for them, 15.1 and 15.5 are not even mentioned in https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html. So that is not an argument and even less so is the fact that you managed to drive maintainers to exhaustion in #93450 and get an instance of that merged.
What you are doing here is abusing
do{} while()loops and going against the coding style and common practice of keeping the normal execution flow in line and limiting the indentation in functions, and making these drivers incoherent with the rest of the code base as well. Plus wasting reviewers time since you got told that from multiple person already.I understand the good intentions, but this has to stop. Please fix the coding style, here and in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/clock_control/clock_control_mchp_sam_d5x_e5x.c as well. We don't need more anti-patterns in the code base.
Thanks.
This topic is under Architecture WG discussion. The changes will be done based on the decision made in WG.
There was a problem hiding this comment.
Hi @fabiobaltieri,
Thanks for the feedback. In the Architecture WG meeting, we concluded not to introduce new do { } while (0) constructs going forward.
I’ve already updated the code in this PR to align with Zephyr’s coding style. The file https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/clock_control/clock_control_mchp_sam_d5x_e5x.c will be updated in the next PR.
Thanks for the clarification.
| dma_mchp_channel_config_t *dma_channel_config; | ||
|
|
||
| /* Pool of descriptor */ | ||
| struct k_fifo dma_desc_pool; |
There was a problem hiding this comment.
k_fifo really isn't the right tool for this, either use a k_mem_slab, sys_block, or array with atomic bitmap signaling used/unused. If you want the cheapest computational route make an array of arrays, one array for each channel with some number of descriptors. No locks, no lists, no checks needed at all since channels are single owner.
51a1edd to
1b50326
Compare
| LOG_MODULE_REGISTER(dma_mchp_dmac_g1, CONFIG_DMA_LOG_LEVEL); | ||
|
|
||
| /* Shortcut to access DMA registers from device configuration */ | ||
| #undef DMAC_REGS |
There was a problem hiding this comment.
This undef looks suspicious, if nothing else its going to lead to confusion if someone greps around for DMAC_REGS and sees there's two defines for it. If the token is already defined then use a new one, no need to undef and redef.
| /* Enable the DMA controller. */ | ||
| static inline void dmac_enable(dmac_registers_t *dmac_reg) | ||
| { | ||
|
|
There was a problem hiding this comment.
extra unnecessary spacing here
There was a problem hiding this comment.
Removed extra line.
| int ret = 0, key = 0; | ||
| bool irq_locked = false; | ||
|
|
||
| do { |
There was a problem hiding this comment.
Really a goto here is just fine, we use them everywhere in Zephyr for single exits, you can drop the irq_locked flag as well and have two exit labels.
There was a problem hiding this comment.
Replaced the do { } while (0) single-exit construct with if-else based flow for better clarity wherever possible.
Also removed irq_lock() since no shared resources are accessed among channels.
| } | ||
|
|
||
| /* Lock interrupts to ensure atomic operation */ | ||
| key = irq_lock(); |
There was a problem hiding this comment.
irq_lock still here it appears, if the register that is being manipulated isn't shared (it doesn't appear to be) among channels, then it is safe to do so without locking.
1b50326 to
df0c403
Compare
df0c403 to
08bf531
Compare
08bf531 to
3b030d1
Compare
NhMchp
removed
the
Architecture Review
a2122f8 to
2a68be6
Compare
| }; | ||
|
|
||
| struct dma_mchp_dmac { | ||
|
|
| }; | ||
|
|
||
| struct dma_mchp_dev_config { | ||
|
|
| struct dma_mchp_dmac *dmac_desc_data; | ||
| struct dma_mchp_channel_config *dma_channel_config; |
There was a problem hiding this comment.
the pointer themselves are not changing right? so they can go in config, save few bytes of sram
There was a problem hiding this comment.
These pointers are constant, so moving them to config would save about 8 bytes, but it would make the code a bit less readable and harder to maintain. Do you still want me to move them?
| { | ||
| /* Read interrupt status */ | ||
| uint16_t pend = dmac_reg->DMAC_INTPEND; | ||
| int8_t interrupt_status_code = 0; |
There was a problem hiding this comment.
no need to initialize this, drop, then the compiler can do its job and warn if there's a code path where this is used uninitialized
There was a problem hiding this comment.
Thanks, I’ve removed the variable initialization.
|
|
||
| } else { | ||
| LOG_ERR("Invalid parameter for DMA channel direction"); | ||
| ret = -EINVAL; |
There was a problem hiding this comment.
return -EINVAL, return 0 at the end of the function and drop ret
There was a problem hiding this comment.
dropped ret and used the return as suggested. Thanks.
| uint32_t requested_channel = 0; | ||
| bool ret = false; |
| /* Get device configuration */ | ||
| const struct dma_mchp_dev_config *dev_cfg = dev->config; | ||
|
|
||
| int ret = 0; |
| #define DMA_MCHP_IRQ_CONNECT(idx, n) \ | ||
| IF_ENABLED(DT_INST_IRQ_HAS_IDX(n, idx), ( \ | ||
| /** Connect the IRQ to the DMA ISR */ \ | ||
| IRQ_CONNECT(DT_INST_IRQ_BY_IDX(n, idx, irq), \ | ||
| DT_INST_IRQ_BY_IDX(n, idx, priority), \ | ||
| dma_mchp_isr, \ | ||
| DEVICE_DT_INST_GET(n), 0); \ | ||
| /** Enable the IRQ */ \ | ||
| irq_enable(DT_INST_IRQ_BY_IDX(n, idx, irq)); \ | ||
| )) |
There was a problem hiding this comment.
use \ consistently, preferably aligned on the right but at least consistently
| /** Connect all IRQs for this instance */ \ | ||
| LISTIFY(\ | ||
| DT_NUM_IRQS(DT_DRV_INST(n)), \ | ||
| DMA_MCHP_IRQ_CONNECT, \ | ||
| (), \ | ||
| n\ | ||
| ) \ |
| .dma_ctx = \ | ||
| { \ |
There was a problem hiding this comment.
nit: this should be .dma_ctx = {, bracket inline, pass if you are using clang-format since that's a lost battle but in that case you probably forgot to rerun it?
There was a problem hiding this comment.
Used clang-format, still the bracket goes to next line.
2a68be6 to
f05ff79
Compare
Add the device tree node and the binding file for microchip dma G1 Peripheral. Signed-off-by: Arunprasath P <arunprasath.p@microchip.com>
Add G1 DMA driver for Microchip DMA Peripherals. Signed-off-by: Arunprasath P <arunprasath.p@microchip.com>
Update sam_e54_xpro.yml to include DMA in the supported features list. Signed-off-by: Arunprasath P <arunprasath.p@microchip.com>
f05ff79 to
14adfc8
Compare
|
|
Hi @fabiobaltieri , Could you revisit ? |
|
This missed the feature freeze, so the milestone should probably be updated? |
- Ports the merged changes in zephyr main to bitbucket - Contains the changes from zephyrproject-rtos#96300 Signed-off-by: Arunprasath P <arunprasath.p@microchip.com>
10 participants
This pull request adds DMA driver support for the Microchip DMAC peripheral. It introduces the DMA driver implementation and updates the devicetree for the SAM D5x/E5x series to include the required node and bindings.