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
MHU: Add MHU driver support for SSE-200 subsystem #12722
Conversation
All checks are passing now. Review history of this comment for details about previous failed status. |
Codecov Report
@@ Coverage Diff @@
## master #12722 +/- ##
=======================================
Coverage 52.46% 52.46%
=======================================
Files 322 322
Lines 46597 46597
Branches 10769 10769
=======================================
Hits 24446 24446
Misses 17246 17246
Partials 4905 4905 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at implementing the IPM interface (include/ipm.h
) rather than introducing a new interface.
Agreed, please use the IPM interface for this driver. Also, do we need some code and support for bringing up the second core in Zephyr, and running Zephyr on it? |
Okay, updated to ipm interface. I was thought that the MHU has no data transfer compare to ipm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have some comments to the commit message:
Maybe it should start with "drivers/ipm: "?
Shouldn't the word "enables" be here: MHU (Message Handling Unit) enable software
And "It enabled in SSE 200 subsystems." seems to miss "is" perhaps.
drivers/ipm/ipm_mhu.c
Outdated
#define DEV_DATA(dev) \ | ||
((struct ipm_mhu_data *)(dev)->driver_data) | ||
#define IPM_MHU_REGS(dev) \ | ||
((volatile struct ipm_mhu_reg_map_t *)(DEV_CFG(dev))->base) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two spaces between ipm_mhu_reg_map_t and *
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all you corrections, I will update them at next push.
drivers/ipm/Kconfig
Outdated
help | ||
Driver for SSE 200 MHU (Message Handling Unit) | ||
|
||
config IPM_MHU0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need instance defines, can we get this from DT?
DT_ARM_MHU_0
or DT_ARM_MHU_1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DT_ARM_MHU_0 and DT_ARM_MHU_1 are always defined after mhu added into dts file, are they?
The config here provide a choice which MHU device should add as in this sub system include 2 MHU.
When enabling MHU device, there will be settings in the prj.conf like:
CONFIG_IPM=y
CONFIG_IPM_MHU=y
CONFIG_IPM_MHU0=y
CONFIG_IPM_MHU1=y
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that choice of which MHU can and should be handled by whats in the DTS. I don't think we need the Kconfig options here.
drivers/ipm/ipm_mhu.c
Outdated
#define IPM_MHU_REGS(dev) \ | ||
((volatile struct ipm_mhu_reg_map_t *)(DEV_CFG(dev))->base) | ||
|
||
#if defined(CONFIG_IPM_MHU0) || defined(CONFIG_IPM_MHU1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ifdef shouldn't be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONFIG_IPM=y
CONFIG_IPM_MHU=y
#CONFIG_IPM_MHU0=y
#CONFIG_IPM_MHU1=y
If config file above, there are warnings about some symbols declared but not used. Do we need to consider this scenario?
drivers/ipm/ipm_mhu.c
Outdated
|
||
p_mhu_dev_base = (volatile u32_t *)IPM_MHU_REGS(d); | ||
|
||
p_cpu_id = (volatile u32_t *)(((u32_t)p_mhu_dev_base & 0xF0000000) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a #define for the magic number 0xF0000000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some minor review comments in a few places.
c291990
to
1a0632c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting closer on the docs...
Thanks, updated now. |
MHU (Message Handling Unit) enables software to raise interrupts to the processor cores. It is enabled in SSE 200 subsystems. This patch aims to implement inter processor communication. Signed-off-by: Karl Zhang <karl.zhang@linaro.org>
Sample walk through: 1. CPU 0 will wake up CPU 1 after initialization 2. CPU 1 will send to CPU 0 an interrupt over MHU0 3. CPU 0 return the same to CPU 1 when received MHU0 interrupt 4. Test done when CPU 1 received MHU0 interrupt The wake up second core and private core ID are soc specific. Signed-off-by: Karl Zhang <karl.zhang@linaro.org>
MHU (Message Handling Unit) enable software to raise interrupts to
the processor cores.
This patch enable MHU driver on Musca A which has 2 MHU elements. It helps to communicate between 2 cores.
Signed-off-by: Karl Zhang karl.zhang@linaro.org