SNOOP:phytium:update phytium SNOOP driver support to 6.6.0.4#1766
SNOOP:phytium:update phytium SNOOP driver support to 6.6.0.4#1766Chengyulai wants to merge 4 commits into
Conversation
This patch update the DT description for the Phytium snoop controller. Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
This patch update the description for the Phytium snoop controller to ensure its driver is functioning normally. Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
This patch add snoop description for pe220x platforms, which usually used as BMC cards. Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
This patch update the snoop node description for pe220x platforms to ensure its driver is functioning normally. Signed-off-by: Li Yuze <liyuze@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn> Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn>
Reviewer's GuideUpdates Phytium BMC Snoop controller support by adding a formal DT binding schema and wiring up/adjusting the snoop controller node for pe220x BMC platforms so the driver can probe correctly on 6.6.0.4. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @Chengyulai. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the binding,
regis defined but not listed as required, which seems inconsistent with the example and typical controller bindings; consider addingregtorequiredif the controller always has an MMIO region. - The
compatibleschema only constrains the first string tophytium,snoop-ctrl, but the example also uses"syscon"; ifsysconis expected, model this explicitly (e.g., withoneOfanditems/additionalItems: false) so the example passes validation and the allowed combinations are clear. - Consider tightening the
snoop-portsproperty by specifying constraints such asminItems, a sensible maximum, and possibly value ranges to catch misconfigurations early.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the binding, `reg` is defined but not listed as required, which seems inconsistent with the example and typical controller bindings; consider adding `reg` to `required` if the controller always has an MMIO region.
- The `compatible` schema only constrains the first string to `phytium,snoop-ctrl`, but the example also uses `"syscon"`; if `syscon` is expected, model this explicitly (e.g., with `oneOf` and `items`/`additionalItems: false`) so the example passes validation and the allowed combinations are clear.
- Consider tightening the `snoop-ports` property by specifying constraints such as `minItems`, a sensible maximum, and possibly value ranges to catch misconfigurations early.
## Individual Comments
### Comment 1
<location path="Documentation/devicetree/bindings/misc/phytium,snoop-ctrl.yaml" line_range="21-30" />
<code_context>
+ items:
+ - enum:
+ - phytium,snoop-ctrl
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ snoop-ports:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: Ports to snoop
+
+required:
+ - compatible
+ - interrupts
+ - snoop-ports
+
+examples:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `reg` is defined but not listed as required, which may not reflect the hardware requirement.
The example node uses `reg`, and this type of controller typically needs a register region. Since `reg` is defined but not required, please confirm whether MMIO is always present. If so, add `reg` to `required:`; if not, ensure the driver handles the absence of `reg` correctly.
Suggested implementation:
```
required:
- compatible
- reg
- interrupts
- snoop-ports
```
The change assumes that the hardware always exposes an MMIO region and that the driver depends on it. If that's not true, the alternative would be to leave `reg` out of `required:` and instead adjust the driver to tolerate the absence of a register region and document that behavior here.
</issue_to_address>
### Comment 2
<location path="Documentation/devicetree/bindings/misc/phytium,snoop-ctrl.yaml" line_range="27-29" />
<code_context>
+ interrupts:
+ maxItems: 1
+
+ snoop-ports:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: Ports to snoop
+
+required:
</code_context>
<issue_to_address>
**suggestion:** `snoop-ports` could benefit from tighter schema constraints (e.g. item size or count).
`snoop-ports` is currently just a `uint32-array`:
```yaml
snoop-ports:
$ref: /schemas/types.yaml#/definitions/uint32-array
description: Ports to snoop
```
If the hardware only supports a limited number of snooped ports or a specific port-number range, please consider:
- Adding `minItems`/`maxItems` if there is a fixed or maximum number of ports.
- Constraining values with `minimum`/`maximum` if ports are within a defined range.
This will catch invalid DTs earlier via `dt_binding_check` and better document the hardware limits.
Suggested implementation:
```
snoop-ports:
$ref: /schemas/types.yaml#/definitions/uint32-array
description: Ports to snoop
minItems: 1
uniqueItems: true
```
```
interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>;
snoop-ports = <0 1>;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| reg: | ||
| maxItems: 1 | ||
|
|
||
| interrupts: | ||
| maxItems: 1 | ||
|
|
||
| snoop-ports: | ||
| $ref: /schemas/types.yaml#/definitions/uint32-array | ||
| description: Ports to snoop | ||
|
|
There was a problem hiding this comment.
suggestion (bug_risk): reg is defined but not listed as required, which may not reflect the hardware requirement.
The example node uses reg, and this type of controller typically needs a register region. Since reg is defined but not required, please confirm whether MMIO is always present. If so, add reg to required:; if not, ensure the driver handles the absence of reg correctly.
Suggested implementation:
required:
- compatible
- reg
- interrupts
- snoop-ports
The change assumes that the hardware always exposes an MMIO region and that the driver depends on it. If that's not true, the alternative would be to leave reg out of required: and instead adjust the driver to tolerate the absence of a register region and document that behavior here.
| snoop-ports: | ||
| $ref: /schemas/types.yaml#/definitions/uint32-array | ||
| description: Ports to snoop |
There was a problem hiding this comment.
suggestion: snoop-ports could benefit from tighter schema constraints (e.g. item size or count).
snoop-ports is currently just a uint32-array:
snoop-ports:
$ref: /schemas/types.yaml#/definitions/uint32-array
description: Ports to snoopIf the hardware only supports a limited number of snooped ports or a specific port-number range, please consider:
- Adding
minItems/maxItemsif there is a fixed or maximum number of ports. - Constraining values with
minimum/maximumif ports are within a defined range.
This will catch invalid DTs earlier via dt_binding_check and better document the hardware limits.
Suggested implementation:
snoop-ports:
$ref: /schemas/types.yaml#/definitions/uint32-array
description: Ports to snoop
minItems: 1
uniqueItems: true
interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>;
snoop-ports = <0 1>;
This patches update the support for phytium SNOOP driver.
1.update the DT description for the Phytium snoop controller.
2.add snoop description for pe220x platforms, which usually used as BMC cards.
3.update the snoop node description for pe220x platforms to ensure its driver is functioning normally.
Summary by Sourcery
Documentation: