drivers: i2c: mchp: Add DMA support for I2C g1 driver #100431
Conversation
add properties for dma support in binding yaml for i2c Signed-off-by: Fabin V Martin <Fabinv.Martin@microchip.com>
zephyrbot
added
area: Devicetree Bindings
area: I2C
platform: Microchip MEC
zephyrbot
requested review from
ArunMCHP,
VenkatKotakonda,
albertofloyd,
jvasanth1,
nandojve,
stephanosio and
teburd
fabin-mchp
force-pushed
the
g1_i2c_driver_dma_support
branch
from
9d8a96b to
fd461c8
Compare
|
nandojve
left a comment
nandojve
left a comment
There was a problem hiding this comment.
DMA require samples and tests working in mainline.
| #define I2C_MCHP_DMA_CHANNELS(n) \ | ||
| .i2c_dma.dma_dev = DEVICE_DT_GET(MCHP_DT_INST_DMA_CTLR(n, tx)), \ | ||
| .i2c_dma.tx_dma_request = MCHP_DT_INST_DMA_TRIGSRC(n, tx), \ | ||
| .i2c_dma.tx_dma_channel = MCHP_DT_INST_DMA_CHANNEL(n, tx), \ | ||
| .i2c_dma.rx_dma_request = MCHP_DT_INST_DMA_TRIGSRC(n, rx), \ | ||
| .i2c_dma.rx_dma_channel = MCHP_DT_INST_DMA_CHANNEL(n, rx), |
There was a problem hiding this comment.
This assumes that DMA definition are always available.
What happen if that information is not ?
How the driver should behave ? It should fall back to interrupt mode ?
I assume that it is a valid scenario using one I2C with DMA and another with Interrupt. Is the driver ready for that situation ?
There was a problem hiding this comment.
Yes. If the DMA definitions are not available, the driver will fall back to interrupt mode.
Using one I2C instance with DMA and another with interrupts is a valid scenario. However, the current driver does not support this configuration. We can consider adding support for mixed DMA/interrupt configurations as a future feature enhancement.
There was a problem hiding this comment.
However, the current driver does not support this configuration.
Ok, but then user can configure the system wrongly because he need to add DMA lines for all instances with status=OKAY, right ? If the driver do not support mixed configuration maybe we need to add some assert to pass the message:
you are selection DMA to be used in I2C but you do not configure the DMA lines for the instance X. You need configure all I2C DMA lines to enable I2C DMA support.
There was a problem hiding this comment.
I’ve added a compile-time build assert as suggested. This way, if DMA is enabled for I2C, all instances need to have their DMA lines correctly configured, helping to catch any misconfigurations during the build.
fabin-mchp
force-pushed
the
g1_i2c_driver_dma_support
branch
from
fd461c8 to
d046416
Compare
| #define I2C_MCHP_DMA_CHANNELS(n) \ | ||
| .i2c_dma.dma_dev = DEVICE_DT_GET(MCHP_DT_INST_DMA_CTLR(n, tx)), \ | ||
| .i2c_dma.tx_dma_request = MCHP_DT_INST_DMA_TRIGSRC(n, tx), \ | ||
| .i2c_dma.tx_dma_channel = MCHP_DT_INST_DMA_CHANNEL(n, tx), \ | ||
| .i2c_dma.rx_dma_request = MCHP_DT_INST_DMA_TRIGSRC(n, rx), \ | ||
| .i2c_dma.rx_dma_channel = MCHP_DT_INST_DMA_CHANNEL(n, rx), |
There was a problem hiding this comment.
However, the current driver does not support this configuration.
Ok, but then user can configure the system wrongly because he need to add DMA lines for all instances with status=OKAY, right ? If the driver do not support mixed configuration maybe we need to add some assert to pass the message:
you are selection DMA to be used in I2C but you do not configure the DMA lines for the instance X. You need configure all I2C DMA lines to enable I2C DMA support.
fabin-mchp
force-pushed
the
g1_i2c_driver_dma_support
branch
from
d046416 to
b9d6219
Compare
@nandojve Thanks for your comment. I have a doubt regarding this. Do you mean that we need add the board name |
drivers/i2c/i2c_mchp_sercom_g1.c
Outdated
| #include <zephyr/irq.h> | ||
| #include <zephyr/logging/log.h> | ||
| #include <zephyr/drivers/i2c.h> | ||
| #ifdef CONFIG_I2C_MCHP_DMA_DRIVEN |
There was a problem hiding this comment.
ifdef really needed? Any build issues if removed?
There was a problem hiding this comment.
The build works fine without the #ifdef. I added it just to include these headers only when DMA support is enabled.
Should I remove it?
There was a problem hiding this comment.
Removed #ifdef as per suggestion.
| { | ||
| struct i2c_mchp_dev_data *data = dev->data; | ||
|
|
||
| /* left-shift address by 1 for R/W bit. */ |
There was a problem hiding this comment.
Dropping the comment here seems unrelated
yes, otherwise it will not build in CI. When a sample/testcase.yaml is filtering by platform-allow, usually only those run in CI. |
@nandojve |
dma support added for i2c transfer Signed-off-by: Fabin V Martin <Fabinv.Martin@microchip.com>
add dma properties in i2c node. Signed-off-by: Fabin V Martin <Fabinv.Martin@microchip.com>
fabin-mchp
force-pushed
the
g1_i2c_driver_dma_support
branch
from
b9d6219 to
6b436c4
Compare
|
Hi @fabin-mchp , I'm really against bring content without a way to keep it sane. I think we really should work in the direction to bring content with tests because it is the only way to serve well community and customers. Otherwise instead focus in the improvements and new features we will start at some point to only fix problems in the tree because we did not tested well. I understand that you are testing locally but Zephyr changes so much that it is very easy to someone create a problem if we don't have a way to catch it. I would say you should bring the test here and make it generic enough to capture not only Microchip but other devices. You can be surprise with the problems. If that happens, we make the test available only to MCHP and then people can fix at their time. |
4 participants
This pull request introduces DMA support for I2C transfers in the Microchip I2C g1 driver and updates the device tree nodes for the sam_e54_xpro board to enable DMA functionality for I2C peripherals.