You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "lipengfei28 (via GitHub)" <gi...@apache.org> on 2024/10/12 09:30:24 UTC
[PR] arm64: default SPI affinity routing to CPU0 [nuttx]
lipengfei28 opened a new pull request, #14172:
URL: https://github.com/apache/nuttx/pull/14172
1. Set SPI interrupt affinity routing default to CPU0, and keep them unchanged unless `up_affinity_irq` is called.
*Note: Please adhere to [Contributing Guidelines](https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md).*
## Summary
*Update this section with information on why change is necessary,
what it exactly does and how, if new feature shows up, provide
references (dependencies, similar problems and solutions), etc.*
## Impact
*Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.*
## Testing
*Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#issuecomment-2410663723
improved by https://github.com/apache/nuttx/pull/14235, let's close this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#issuecomment-2408478488
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
## NuttX PR Requirements Review
Let's analyze your PR against the NuttX requirements:
**Summary:**
* **Missing Information:** The summary section lacks crucial details. You need to explain:
* **Why is this change necessary?** What problem does it solve? Is there a bug report or feature request this addresses?
* **What exactly does it do?** Explain how setting the SPI interrupt affinity to CPU0 by default and controlling it with `up_affinity_irq` works.
* **How does it work?** Provide a technical overview of the changes made to the code.
**Impact:**
* **Incomplete:** While you've provided the framework, you need to fill in the details for each impact area:
* **Is new feature added? Is existing feature changed?** - Clearly state if this is a new feature or modifies an existing one.
* **Impact on user:** Will users experience any changes in behavior? Will they need to modify their applications or configurations?
* **Impact on build:** Does this PR introduce new dependencies, configurations, or build steps?
* **Impact on hardware:** Which architectures, boards, or drivers are specifically affected by this change?
* **Impact on documentation:** Does the documentation need updating to reflect this change? Have you updated it?
* **Impact on security:** Are there any potential security implications introduced or mitigated by this change?
* **Impact on compatibility:** Does this affect backward compatibility, forward compatibility, or interoperability with other systems?
**Testing:**
* **Insufficient Information:**
* **Build Host(s):** Specify the operating system, CPU architecture, and compiler version used for building NuttX.
* **Target(s):** Detail the target architecture (e.g., simulator, RISC-V, ARM), the specific board, and the configuration used for testing.
* **Testing logs:** Provide snippets of relevant log output from both *before* and *after* the change. Highlight the key differences that demonstrate the change's effectiveness.
**Contributing Guidelines:**
* **Review:** Ensure your code adheres to the NuttX [Contributing Guidelines](https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md). Pay attention to coding style, commit message conventions, and testing requirements.
**In Conclusion**
Your PR needs significant improvements to meet the NuttX requirements. You must provide comprehensive information in the Summary, Impact, and Testing sections. Ensure your code follows the contributing guidelines and clearly demonstrate the change's purpose, implementation, and testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "XuNeo (via GitHub)" <gi...@apache.org>.
XuNeo commented on code in PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#discussion_r1798724457
##########
arch/arm64/src/common/arm64_gicv3.c:
##########
@@ -249,16 +249,6 @@ void arm64_gic_irq_enable(unsigned int intid)
uint32_t mask = BIT(intid & (GIC_NUM_INTR_PER_REG - 1));
uint32_t idx = intid / GIC_NUM_INTR_PER_REG;
- /* Affinity routing is enabled for Non-secure state (GICD_CTLR.ARE_NS
- * is set to '1' when GIC distributor is initialized) ,so need to set
- * SPI's affinity, now set it to be the PE on which it is enabled.
- */
-
- if (GIC_IS_SPI(intid))
- {
- arm64_gic_write_irouter((GET_MPIDR() & MPIDR_ID_MASK), intid);
Review Comment:
Hi @anchao,
This PR is to align with arm-v7a behavior and avoid changing affinity unexpectedly when enabling IRQ.
Driver may want to change the affinity by calling `up_affinity_irq`
https://github.com/apache/nuttx/blob/master/arch/arm/src/armv7-a/arm_gicv2.c#L208-L212
I'm unfamiliar with what kernel code requires, does arm-v7a have same problem?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 closed pull request #14172: arm64: default SPI affinity routing to CPU0
URL: https://github.com/apache/nuttx/pull/14172
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#discussion_r1797705346
##########
arch/arm64/src/common/arm64_gicv3.c:
##########
@@ -249,16 +249,6 @@ void arm64_gic_irq_enable(unsigned int intid)
uint32_t mask = BIT(intid & (GIC_NUM_INTR_PER_REG - 1));
uint32_t idx = intid / GIC_NUM_INTR_PER_REG;
- /* Affinity routing is enabled for Non-secure state (GICD_CTLR.ARE_NS
- * is set to '1' when GIC distributor is initialized) ,so need to set
- * SPI's affinity, now set it to be the PE on which it is enabled.
- */
-
- if (GIC_IS_SPI(intid))
- {
- arm64_gic_write_irouter((GET_MPIDR() & MPIDR_ID_MASK), intid);
Review Comment:
We are using SPI to route to other CPUs, If you have special requirement please add a configuration option to disable routing to different CPUs， please set default value to n if smp case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#discussion_r1797684965
##########
arch/arm64/src/common/arm64_gicv3.c:
##########
@@ -249,16 +249,6 @@ void arm64_gic_irq_enable(unsigned int intid)
uint32_t mask = BIT(intid & (GIC_NUM_INTR_PER_REG - 1));
uint32_t idx = intid / GIC_NUM_INTR_PER_REG;
- /* Affinity routing is enabled for Non-secure state (GICD_CTLR.ARE_NS
- * is set to '1' when GIC distributor is initialized) ,so need to set
- * SPI's affinity, now set it to be the PE on which it is enabled.
- */
-
- if (GIC_IS_SPI(intid))
- {
- arm64_gic_write_irouter((GET_MPIDR() & MPIDR_ID_MASK), intid);
Review Comment:
why remove the dynamic routing？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "lipengfei28 (via GitHub)" <gi...@apache.org>.
lipengfei28 commented on code in PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#discussion_r1798674378
##########
arch/arm64/src/common/arm64_gicv3.c:
##########
@@ -249,16 +249,6 @@ void arm64_gic_irq_enable(unsigned int intid)
uint32_t mask = BIT(intid & (GIC_NUM_INTR_PER_REG - 1));
uint32_t idx = intid / GIC_NUM_INTR_PER_REG;
- /* Affinity routing is enabled for Non-secure state (GICD_CTLR.ARE_NS
- * is set to '1' when GIC distributor is initialized) ,so need to set
- * SPI's affinity, now set it to be the PE on which it is enabled.
- */
-
- if (GIC_IS_SPI(intid))
- {
- arm64_gic_write_irouter((GET_MPIDR() & MPIDR_ID_MASK), intid);
Review Comment:
arm64_gic_irq_enable:The funtion should be only enable irq,
set affinity should be call up_affinity_irq,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#discussion_r1798739483
##########
arch/arm64/src/common/arm64_gicv3.c:
##########
@@ -249,16 +249,6 @@ void arm64_gic_irq_enable(unsigned int intid)
uint32_t mask = BIT(intid & (GIC_NUM_INTR_PER_REG - 1));
uint32_t idx = intid / GIC_NUM_INTR_PER_REG;
- /* Affinity routing is enabled for Non-secure state (GICD_CTLR.ARE_NS
- * is set to '1' when GIC distributor is initialized) ,so need to set
- * SPI's affinity, now set it to be the PE on which it is enabled.
- */
-
- if (GIC_IS_SPI(intid))
- {
- arm64_gic_write_irouter((GET_MPIDR() & MPIDR_ID_MASK), intid);
Review Comment:
@XuNeo @lipengfei28
https://github.com/apache/nuttx/pull/14172#discussion_r1797705346
as I mentioned before that this feature could be add a new config, please review this PR https://github.com/apache/nuttx/pull/14235
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#discussion_r1798683652
##########
arch/arm64/src/common/arm64_gicv3.c:
##########
@@ -249,16 +249,6 @@ void arm64_gic_irq_enable(unsigned int intid)
uint32_t mask = BIT(intid & (GIC_NUM_INTR_PER_REG - 1));
uint32_t idx = intid / GIC_NUM_INTR_PER_REG;
- /* Affinity routing is enabled for Non-secure state (GICD_CTLR.ARE_NS
- * is set to '1' when GIC distributor is initialized) ,so need to set
- * SPI's affinity, now set it to be the PE on which it is enabled.
- */
-
- if (GIC_IS_SPI(intid))
- {
- arm64_gic_write_irouter((GET_MPIDR() & MPIDR_ID_MASK), intid);
Review Comment:
So, this design will caused all kernel code must to call up_affinity_irq() before up_enable_irq()? Why is the behavior not which CPU is enabled and routing to which CPU to? Why don't you guys call up_affinity_irq(0) before up_enable_irq() to routing to CPU0?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arm64: default SPI affinity routing to CPU0 [nuttx]
Posted by "lipengfei28 (via GitHub)" <gi...@apache.org>.
lipengfei28 commented on code in PR #14172:
URL: https://github.com/apache/nuttx/pull/14172#discussion_r1797695353
##########
arch/arm64/src/common/arm64_gicv3.c:
##########
@@ -249,16 +249,6 @@ void arm64_gic_irq_enable(unsigned int intid)
uint32_t mask = BIT(intid & (GIC_NUM_INTR_PER_REG - 1));
uint32_t idx = intid / GIC_NUM_INTR_PER_REG;
- /* Affinity routing is enabled for Non-secure state (GICD_CTLR.ARE_NS
- * is set to '1' when GIC distributor is initialized) ,so need to set
- * SPI's affinity, now set it to be the PE on which it is enabled.
- */
-
- if (GIC_IS_SPI(intid))
- {
- arm64_gic_write_irouter((GET_MPIDR() & MPIDR_ID_MASK), intid);
Review Comment:
Set SPI interrupt affinity routing default to CPU0 first, and keep them unchanged unless up_affinity_irq is called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org