lbc: phytium: Bugfix issue with computing device#1667
Open
mamingrui123 wants to merge 1 commit intodeepin-community:linux-6.6.yfrom
Open
lbc: phytium: Bugfix issue with computing device#1667mamingrui123 wants to merge 1 commit intodeepin-community:linux-6.6.yfrom
mamingrui123 wants to merge 1 commit intodeepin-community:linux-6.6.yfrom
Conversation
This patch fixes the issue of computing device capacity, and adds some debug printing code for debugging. Signed-off-by: Ma Mingrui <mamingrui1243@phytium.com.cn> Signed-off-by: Wang Hanmo <wanghanmo2242@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Reviewer's GuideFixes Phytium localbus controller device capacity computation and improves diagnostics, including changing the device base address handling and refining error messages for DT/ACPI probing. Flow diagram for updated phytium_lbc_calc_device_size logicflowchart TD
A[phytium_lbc_calc_device_size device_size] --> B{device_size}
B --> C[SZ_16K]
B --> D[SZ_32K]
B --> E[SZ_64K]
B --> F[SZ_128K]
B --> G[SZ_256K]
B --> H[SZ_512K]
B --> I[SZ_1M]
B --> J[SZ_2M]
B --> K[SZ_4M]
B --> L[SZ_8M]
B --> M[SZ_16M]
B --> N[SZ_32M]
B --> O[SZ_64M]
B --> P[SZ_128M]
B --> Q[SZ_256M]
B --> R[Other]
C --> C1[Set value = 0x0]
D --> D1[Set value = 0x1]
E --> E1[Set value = 0x2]
F --> F1[Set value = 0x3]
G --> G1[Set value = 0x4]
H --> H1[Set value = 0x5]
I --> I1[Set value = 0x6]
J --> J1[Set value = 0x7]
K --> K1[Set value = 0x8]
L --> L1[Set value = 0x9]
M --> M1[Set value = 0xa]
N --> N1[Set value = 0xb]
O --> O1[Set value = 0xc]
P --> P1[Set value = 0xd]
Q --> Q1[Set value = 0xe]
R --> R1[Set value = -EINVAL]
R1 --> R2[pr_err device size is invalid or not supported]
C1 --> Z[return value]
D1 --> Z
E1 --> Z
F1 --> Z
G1 --> Z
H1 --> Z
I1 --> Z
J1 --> Z
K1 --> Z
L1 --> Z
M1 --> Z
N1 --> Z
O1 --> Z
P1 --> Z
Q1 --> Z
R2 --> Z
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[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 |
|
Hi @mamingrui123. Thanks for your PR. 😃 |
|
Hi @mamingrui123. 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. |
![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 1 issue, and left some high level feedback:
- In
phytium_lbc_calc_device_size(), assigning-EINVALto au32and returning it through anintwill result in a large positive value rather than a proper error code; consider returning-EINVALdirectly from the default case and keeping the encoded size in a separate signed/unsigned variable to avoid this mismatch. - Changing
LBC_DEVICE_ADDRfrom a fixed macro to a static global set fromres->startmay cause issues if multiple controllers are present; it would be safer to store the base address in thephytium_lbcinstance (or another per-device structure) instead of a single global.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `phytium_lbc_calc_device_size()`, assigning `-EINVAL` to a `u32` and returning it through an `int` will result in a large positive value rather than a proper error code; consider returning `-EINVAL` directly from the default case and keeping the encoded size in a separate signed/unsigned variable to avoid this mismatch.
- Changing `LBC_DEVICE_ADDR` from a fixed macro to a static global set from `res->start` may cause issues if multiple controllers are present; it would be safer to store the base address in the `phytium_lbc` instance (or another per-device structure) instead of a single global.
## Individual Comments
### Comment 1
<location path="drivers/mtd/maps/phytium_lbc.c" line_range="44-47" />
<code_context>
#define LBC_FTIM2_CS_GPCM(n) (0x1a4+0xc*n)
-#define LBC_DEVICE_ADDR 0x10000000
+static uint64_t LBC_DEVICE_ADDR;
#define PHYTIUM_MAX_SRAM_BLOCK 8
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid using a single global LBC_DEVICE_ADDR for what may be per-device state.
This makes the base address effectively shared by all driver instances: any later probe will overwrite the value for earlier ones, and multi-controller or multi-platform-device setups will misbehave. Please store this per-instance (e.g. in `struct phytium_lbc` or by using `map->phys` / the resource) instead of a file-scope static.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+44
to
47
| static uint64_t LBC_DEVICE_ADDR; | ||
| #define PHYTIUM_MAX_SRAM_BLOCK 8 | ||
|
|
||
| #define PHYTIUM_LOCALBUS_DRIVER_VERSION "1.0.0" |
There was a problem hiding this comment.
issue (bug_risk): Avoid using a single global LBC_DEVICE_ADDR for what may be per-device state.
This makes the base address effectively shared by all driver instances: any later probe will overwrite the value for earlier ones, and multi-controller or multi-platform-device setups will misbehave. Please store this per-instance (e.g. in struct phytium_lbc or by using map->phys / the resource) instead of a file-scope static.
There was a problem hiding this comment.
Pull request overview
Fixes Phytium Localbus Controller (LBC) MTD mapping issues by tightening device-size encoding and using the probed MMIO resource base for device addressing, with improved probe diagnostics.
Changes:
- Replace linear “multiple of 16K” size encoding with a discrete supported-size mapping and emit an error for unsupported sizes.
- Improve probe failure messages to distinguish device-tree vs ACPI probing failures.
- Use the probed memory resource start address as the device base address (instead of a hard-coded constant).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
107
to
163
| static int phytium_lbc_calc_device_size(int device_size) | ||
| { | ||
| int ret = 0; | ||
| u32 value; | ||
|
|
||
| if (device_size%SZ_16K == 0) { | ||
| value = device_size/SZ_16K - 1; | ||
| } else { | ||
| ret = -EINVAL; | ||
| return ret; | ||
| switch (device_size) { | ||
| case SZ_16K: | ||
| value = 0x0; | ||
| break; | ||
| case SZ_32K: | ||
| value = 0x1; | ||
| break; | ||
| case SZ_64K: | ||
| value = 0x2; | ||
| break; | ||
| case SZ_128K: | ||
| value = 0x3; | ||
| break; | ||
| case SZ_256K: | ||
| value = 0x4; | ||
| break; | ||
| case SZ_512K: | ||
| value = 0x5; | ||
| break; | ||
| case SZ_1M: | ||
| value = 0x6; | ||
| break; | ||
| case SZ_2M: | ||
| value = 0x7; | ||
| break; | ||
| case SZ_4M: | ||
| value = 0x8; | ||
| break; | ||
| case SZ_8M: | ||
| value = 0x9; | ||
| break; | ||
| case SZ_16M: | ||
| value = 0xa; | ||
| break; | ||
| case SZ_32M: | ||
| value = 0xb; | ||
| break; | ||
| case SZ_64M: | ||
| value = 0xc; | ||
| break; | ||
| case SZ_128M: | ||
| value = 0xd; | ||
| break; | ||
| case SZ_256M: | ||
| value = 0xe; | ||
| break; | ||
| default: | ||
| value = -EINVAL; | ||
| pr_err("lbc: device size is invalid or not supported.\n"); | ||
| break; | ||
| } | ||
|
|
||
| return value; |
Comment on lines
41
to
46
| #define LBC_FTIM1_CS_GPCM(n) (0x1a0+0xc*n) | ||
| #define LBC_FTIM2_CS_GPCM(n) (0x1a4+0xc*n) | ||
|
|
||
| #define LBC_DEVICE_ADDR 0x10000000 | ||
| static uint64_t LBC_DEVICE_ADDR; | ||
| #define PHYTIUM_MAX_SRAM_BLOCK 8 | ||
|
|
Comment on lines
501
to
502
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch fixes the issue of computing device capacity, and adds some debug printing code for debugging.
Summary by Sourcery
Fix Phytium local bus controller device capacity handling and improve diagnostics.
Bug Fixes:
Enhancements: