Th1520 v1.8 #22
Open
Th1520 v1.8 #22
Conversation
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
Signed-off-by: Han Gao <rabenda.cn@gmail.com>
add light_ppu for thead,light refer from: revyos/thead-opensbi@f880f3e Signed-off-by: Xiang W <wxjstz@126.com>
refer from: revyos/thead-opensbi@f880f3e Signed-off-by: Xiang W <wxjstz@126.com>
Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
There is a lot of code in light_ext.c that is duplicated in thead-generic.c, so delete it. Signed-off-by: Xiang W <wxjstz@126.com>
Signed-off-by: David Li <davidli.li@linux.alibaba.com>
Signed-off-by: David Li <davidli.li@linux.alibaba.com>
Signed-off-by: David Li <davidli.li@linux.alibaba.com>
This reverts commit 0dd8a26.
Signed-off-by: Esther Z <Esther.Z@linux.aliabab.com>
Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
Signed-off-by: Han Gao <gaohan@iscas.ac.cn> Signed-off-by: Han Gao <rabenda.cn@gmail.com>
Signed-off-by: Xiang W <wxjstz@126.com>
Signed-off-by: Xiang W <wxjstz@126.com>
Signed-off-by: Xiang W <wxjstz@126.com>
The mailbox binding for it isn't properly defined. Hardcode it instead of requireing a non-standard DT node, and remove the non-upstream "mbox" code from Xuantie (which is incompatible with later upstream mailbox code). The proper AON dt compatible string "thead,th1520-aon" is also added, and a bogus ipc.h header file is cleaned up. Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
Reviewer's GuideAdds TH1520 v1.8 platform support for T-HEAD Light/TH1520 SoCs, including low-power / suspend and CPU hotplug via AON mailbox, vendor PMU/PMP SBI extensions, XThead SSTC timer handling, and CI build workflow, while refactoring generic T-HEAD platform hooks to use errata flags and new light-specific implementations. Sequence diagram for TH1520 AON CPU hotplug via mailboxsequenceDiagram
actor S_Mode_OS
participant SBI_HSM as SBI_HSM
participant light_ppu as light_ppu
participant thead_aon as thead_aon
participant th1520_mbox as th1520_mbox
participant AON_Firmware as AON_Firmware
S_Mode_OS->>SBI_HSM: request hart_stop(hartid)
SBI_HSM->>light_ppu: hart_stop(hartid)
alt hart_is_core0
light_ppu-->>SBI_HSM: log not_available_for_hotplug
SBI_HSM-->>S_Mode_OS: error
else auxiliary_core
light_ppu->>thead_aon: thead_aon_cpuhp(hartid, OFFLINE)
thead_aon->>th1520_mbox: th1520_mbox_write(AON_CHANNEL, cpu_off_msg)
th1520_mbox->>AON_Firmware: send_cpu_off_request
AON_Firmware-->>th1520_mbox: TH1520_MBOX_ACK_MAGIC
th1520_mbox-->>thead_aon: success
thead_aon-->>light_ppu: success
light_ppu->>light_ppu: thead_aon_cpuhp(current_hartid, OFFLINE)
light_ppu->>light_ppu: light_auxcore_save()
light_ppu->>light_ppu: wfi()
light_ppu->>SBI_HSM: sbi_hart_hang()
end
S_Mode_OS->>SBI_HSM: request hart_start(hartid)
SBI_HSM->>light_ppu: hart_start(hartid, saddr)
alt core0
light_ppu->>SBI_HSM: sbi_ipi_raw_send(hartindex, true)
SBI_HSM-->>S_Mode_OS: success
else auxiliary_core
light_ppu->>light_ppu: light_auxcore_restore(hartid)
light_ppu->>thead_aon: thead_aon_cpuhp(hartid, ONLINE)
thead_aon->>th1520_mbox: th1520_mbox_write(AON_CHANNEL, cpu_on_msg)
th1520_mbox->>AON_Firmware: send_cpu_on_request
AON_Firmware-->>th1520_mbox: TH1520_MBOX_ACK_MAGIC
th1520_mbox-->>thead_aon: success
thead_aon-->>light_ppu: success
light_ppu-->>SBI_HSM: success
SBI_HSM-->>S_Mode_OS: success
end
Sequence diagram for vendor PMU/PMP SBI ecall light handlingsequenceDiagram
actor S_Mode_OS
participant SBI_ECALL as SBI_ECALL
participant ecall_light as ecall_light
participant PMU_Handler as sbi_thead_pmu_set
participant PMP_Handler as sbi_thead_pmp_set
participant CSR_Hardware as CSR_HW
participant PMP_HW as PMP_HW
S_Mode_OS->>SBI_ECALL: ecall(extid=SBI_EXT_VENDOR_PMU, a0=type, a1=idx, a2=event_id)
SBI_ECALL->>ecall_light: handle(extid, funcid, regs, out)
alt extid_is_SBI_EXT_VENDOR_PMU
ecall_light->>PMU_Handler: sbi_thead_pmu_set(type, idx, event_id)
alt type_equals_2
PMU_Handler->>PMU_Handler: sbi_thead_pmu_map(idx, event_id)
PMU_Handler->>CSR_Hardware: csr_write(MHPMEVENT[idx], event_id)
else other_type
PMU_Handler->>PMU_Handler: sbi_thead_pmu_init()
PMU_Handler->>CSR_Hardware: configure_MIDELEG_MCOUNTERWEN_MHPMEVENT3_28
end
PMU_Handler-->>ecall_light: success
else extid_is_SBI_EXT_VENDOR_PMP
ecall_light->>PMP_Handler: sbi_thead_pmp_set(funcid, regs->a0)
PMP_Handler->>PMP_HW: ensure_reserved_region_pmp_entry28
alt funcid_equals_0
PMP_Handler->>PMP_HW: configure_TCM0_PMP_entry26(auth)
else funcid_equals_1
PMP_Handler->>PMP_HW: configure_TCM1_PMP_entry27(auth)
end
PMP_Handler-->>ecall_light: success
end
ecall_light-->>SBI_ECALL: out->value=0
SBI_ECALL-->>S_Mode_OS: return_success
Class diagram for TH1520 AON, mailbox, and Light platform integrationclassDiagram
class thead_generic_quirks {
u64 errata
}
class generic_platform_ops {
+early_init(cold_boot)
+extensions_init(hfeatures)
+final_init(cold_boot)
+final_exit()
+vendor_ext_provider(funcid, regs, out)
+cold_boot_allowed(hartid)
}
class thead_generic_platform_init {
+thead_generic_platform_init(fdt, nodeoff, match)
}
thead_generic_quirks <.. thead_generic_platform_init : uses
thead_generic_platform_init ..> generic_platform_ops : configures
class th1520_mbox {
uint32_t *cur_cpu_ch_base
enum th1520_mbox_icu_cpu_id cur_icu_cpu_id
uint32_t *comm_local_base[TH1520_MBOX_CHANS]
uint32_t *comm_remote_base[TH1520_MBOX_CHANS]
+th1520_mbox_write(ia, remote_channel, buffer, len)
+th1520_mbox_read(ia, src_channel, buffer, len, timeout_us)
}
class thead_aon {
th1520_mbox *mbox
+thead_aon_init()
+thead_aon_cpuhp(cpu, status)
+thead_aon_system_suspend()
}
class th1520_aon_rpc_msg_hdr {
uint8_t ver
uint8_t size
uint8_t svc
uint8_t func
}
class rpc_msg_cpu_info {
u16 cpu_id
u16 status
u32 reserved[5]
}
class th1520_aon_rpc_lpm_msg_hdr {
th1520_aon_rpc_msg_hdr hdr
rpc_func_t rpc
}
class th1520_aon_rpc_ack_common {
th1520_aon_rpc_msg_hdr hdr
u8 err_code
}
class sbi_hsm_device_light_ppu {
+const char *name
+hart_start(hartid, saddr)
+hart_stop()
}
class sbi_system_suspend_device_th1520_susp {
+const char *name
+system_suspend_check(sleep_type)
+system_suspend(suspend_type, resume_addr)
}
class light_c910_platform {
+hotplug_flag
+cpu_performance_disable()
+light_mastercore_save()
+light_auxcore_save()
+light_auxcore_restore(hartid)
+light_hart_start(hartid, saddr)
+light_hart_stop()
+th1520_system_suspend(suspend_type, resume_addr)
+th1520_system_suspend_check(sleep_type)
}
th1520_mbox <.. thead_aon : contains
thead_aon <.. light_c910_platform : uses
th1520_aon_rpc_msg_hdr <|-- th1520_aon_rpc_lpm_msg_hdr
th1520_aon_rpc_msg_hdr <|-- th1520_aon_rpc_ack_common
rpc_msg_cpu_info <.. th1520_aon_rpc_lpm_msg_hdr : field
light_c910_platform <.. sbi_hsm_device_light_ppu : provides_callbacks
light_c910_platform <.. sbi_system_suspend_device_th1520_susp : provides_callbacks
class ecall_light_extension {
+const char *name
+u64 extid_start
+u64 extid_end
+register_extensions()
+handle(extid, funcid, regs, out)
}
class sbi_thead_pmu_pmp_handlers {
+sbi_thead_pmu_init()
+sbi_thead_pmu_set(type, idx, event_id)
+sbi_thead_pmp_set(idx, auth)
}
ecall_light_extension ..> sbi_thead_pmu_pmp_handlers : calls
Flow diagram for ACLINT MTIMER handling with XThead SSTC extensionflowchart TD
A[start_mtimer_event] --> B[load_sbi_scratch]
B --> C[check sbi_hart_has_extension scratch SBI_HART_EXT_XTHEADSSTC]
C -->|true| D[time_cmp_addr = mtimer.mtimecmp_addr + 0x9000]
C -->|false| E[time_cmp_addr = mtimer.mtimecmp_addr]
D --> F[compute target_index = target_hart - first_hartid]
E --> F
F --> G[write compare value via mtimer.time_wr]
G --> H[mtimer_event_complete]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 8 issues, and left some high level feedback:
- In
thead_generic_cold_boot_allowed(),selected_hartidis au32initialized to 0 but compared to-1, so theselected_hartid != -1branch is always taken; clarify the sentinel scheme (or change the type/value) so the cold-boot restriction logic actually reflects intent. - In
thead_aon_init()the mailbox base addresses are hardcoded into a localregs[]array; consider deriving these from the DT node (e.g.regproperties) so the implementation follows the hardware description instead of embedding magic addresses. - The new TH1520 mailbox code reintroduces simple MMIO helpers (
iowrite32/ioread32) while other parts of the patch usewritel/readl; it would be cleaner and more consistent to use the existingriscv_ioaccessors throughout the mailbox implementation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `thead_generic_cold_boot_allowed()`, `selected_hartid` is a `u32` initialized to 0 but compared to `-1`, so the `selected_hartid != -1` branch is always taken; clarify the sentinel scheme (or change the type/value) so the cold-boot restriction logic actually reflects intent.
- In `thead_aon_init()` the mailbox base addresses are hardcoded into a local `regs[]` array; consider deriving these from the DT node (e.g. `reg` properties) so the implementation follows the hardware description instead of embedding magic addresses.
- The new TH1520 mailbox code reintroduces simple MMIO helpers (`iowrite32`/`ioread32`) while other parts of the patch use `writel`/`readl`; it would be cleaner and more consistent to use the existing `riscv_io` accessors throughout the mailbox implementation.
## Individual Comments
### Comment 1
<location> `platform/generic/thead/thead-generic.c:442-444` </location>
<code_context>
+ return generic_final_init(cold_boot);
+}
+
+static bool thead_generic_cold_boot_allowed(u32 hartid)
+{
+ if (selected_hartid != -1)
+ return (selected_hartid == hartid);
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Comparing unsigned `selected_hartid` against `-1` makes the check always true and effectively pins cold boot to hart 0.
`selected_hartid` is a `u32` initialized to `0`, but `thead_generic_cold_boot_allowed` checks `if (selected_hartid != -1)`. Since `-1` promotes to `UINT_MAX` for the comparison, this condition is always true and only `hartid == selected_hartid` (currently 0) is allowed to cold boot. If the intent is “any hart unless explicitly selected,” use a sentinel that’s representable (e.g., make `selected_hartid` signed and init to `-1`, or track selection with a separate boolean).
</issue_to_address>
### Comment 2
<location> `platform/generic/thead/thead-generic.c:405-410` </location>
<code_context>
+ return sbi_ecall_register_extension(&ecall_light);
+}
+
+struct sbi_ecall_extension ecall_light = {
+ .name = "light",
+ .extid_start = SBI_EXT_VENDOR_PMU,
+ .extid_end = SBI_EXT_VENDOR_PMP,
+ .register_extensions = sbi_ecall_light_register_extensions,
+ .handle = sbi_ecall_light_handler,
+};
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The `ecall_light` extension calls `sbi_ecall_register_extension(&ecall_light)` from its own `register_extensions` hook, which is likely recursive.
Here `ecall_light.register_extensions` points to `sbi_ecall_light_register_extensions()`, which in turn calls `sbi_ecall_register_extension(&ecall_light)`. Since `register_extensions` is invoked by the core during extension registration, this will re-register the same extension and can cause recursion or double registration. If there are no sub-extensions, either set `.register_extensions = NULL` and only register `ecall_light` from `thead_generic_final_init()`, or keep the hook and avoid explicitly registering it again in `final_init`.
</issue_to_address>
### Comment 3
<location> `platform/generic/include/thead/thead_aon.h:62-65` </location>
<code_context>
+ uint8_t *data = (uint8_t *)(MESG); \
+ data[OFFSET] = (SET_DATA)&0xFF; \
+ } while (0)
+#define RPC_GET_BE64(MESG, OFFSET, PTR) \
+ do { \
+ uint8_t *data = (uint8_t *)(MESG); \
+ *(uint32_t *)(PTR) = \
+ (data[OFFSET + 7] | data[OFFSET + 6] << 8 | \
+ data[OFFSET + 5] << 16 | data[OFFSET + 4] << 24 | \
</code_context>
<issue_to_address>
**issue (bug_risk):** `RPC_GET_BE64` writes a 64-bit value through a `uint32_t *`, which is a type and size mismatch.
This macro computes a 64-bit value but assigns it through a `uint32_t *`, causing truncation on 32-bit targets and violating strict-aliasing/size assumptions on 64-bit targets. The store should use a 64-bit type (e.g. `*(uint64_t *)(PTR)`), or the macro should return the 64-bit value instead, matching the behavior of `RPC_SET_BE64`.
</issue_to_address>
### Comment 4
<location> `platform/generic/include/thead/thead_aon.h:23-28` </location>
<code_context>
+#define RPC_GET_VER(MESG) ((MESG)->ver)
+#define RPC_SET_VER(MESG, VER) ((MESG)->ver = (VER))
+#define RPC_GET_SVC_ID(MESG) ((MESG)->svc & 0x3F)
+#define RPC_SET_SVC_ID(MESG, ID) ((MESG)->svc |= 0x3F & (ID))
+#define RPC_GET_SVC_FLAG_MSG_TYPE(MESG) (((MESG)->svc & 0x80) >> 7)
+#define RPC_SET_SVC_FLAG_MSG_TYPE(MESG, TYPE) ((MESG)->svc |= (TYPE) << 7)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `RPC_SET_SVC_ID` only ORs in bits and never clears previous service ID bits, which can accumulate stale state.
Since it uses `|=` without first clearing the lower 6 bits, calling it on a message with an existing `svc` value (or calling it multiple times with different IDs) can leave old ID bits set. A safer pattern would be: `((MESG)->svc = ((MESG)->svc & ~0x3F) | ((ID) & 0x3F))`. Consider applying a similar mask-and-set approach to the flag setters if messages may be reused.
```suggestion
#define RPC_GET_SVC_ID(MESG) ((MESG)->svc & 0x3F)
#define RPC_SET_SVC_ID(MESG, ID) \
((MESG)->svc = ((MESG)->svc & ~0x3F) | ((ID) & 0x3F))
#define RPC_GET_SVC_FLAG_MSG_TYPE(MESG) (((MESG)->svc & 0x80) >> 7)
#define RPC_SET_SVC_FLAG_MSG_TYPE(MESG, TYPE) \
((MESG)->svc = ((MESG)->svc & ~0x80) | (((TYPE) & 0x1) << 7))
#define RPC_GET_SVC_FLAG_ACK_TYPE(MESG) (((MESG)->svc & 0x40) >> 6)
#define RPC_SET_SVC_FLAG_ACK_TYPE(MESG, ACK) \
((MESG)->svc = ((MESG)->svc & ~0x40) | (((ACK) & 0x1) << 6))
```
</issue_to_address>
### Comment 5
<location> `platform/generic/thead/th1520_mbox.c:140-149` </location>
<code_context>
+ return val;
+}
+
+int th1520_mbox_write(struct th1520_mbox *ia,
+ uint8_t remote_channel, uint8_t *buffer,
+ int len)
+{
+ uint32_t count = 0;
+ uint32_t ack = 0;
+
+ /* set value */
+ wj_mbox_chan_wr_data(ia, remote_channel, buffer, true);
+ wj_mbox_chan_rmw(ia, remote_channel, TH1520_MBOX_GEN,
+ TH1520_MBOX_GEN_TX_DATA, 0, true);
+ do {
+ ack = wj_mbox_chan_read(ia, remote_channel,
+ TH1520_MBOX_INFO7, false);
+ sbi_timer_udelay(1);
+ count++;
+ } while (ack != TH1520_MBOX_ACK_MAGIC &&
+ count <= TH1520_ACK_MAGIC_TIMEOUT);
+
+ if (ack != TH1520_MBOX_ACK_MAGIC) {
+ sbi_printf("mbox send faild, no ack magic receive %d\n", count);
+ return SBI_ETIMEDOUT;
+ } else {
+ //clear ack
</code_context>
<issue_to_address>
**issue (bug_risk):** `th1520_mbox_write` ignores its `len` parameter and always sends a fixed-size payload.
The `len` parameter is unused; `th1520_mbox_write()` always sends `sizeof(struct th1520_aon_rpc_lpm_msg_hdr)` via `wj_mbox_chan_wr_data`, which assumes `TH1520_MBOX_INFO_NUM` words. Either honor `len` (with bounds checks) to control the number of bytes/words sent, or remove `len` and clearly document that callers must provide a fixed-size buffer matching the wire format.
</issue_to_address>
### Comment 6
<location> `platform/generic/thead/th1520_mbox.c:171-168` </location>
<code_context>
+ return 0;
+}
+
+int th1520_mbox_read(struct th1520_mbox *ia,
+ uint8_t *src_channel, uint8_t *buffer,
+ int *len, int timeout_us)
+{
+ return 0;
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** `th1520_mbox_read` is currently a stub that always returns success without filling the buffer.
This stub ignores all parameters, performs no mailbox access, and still returns success. Any caller expecting data or timeout handling will get misleading results. Please either implement the read logic (mirroring the write path where appropriate) or return `SBI_ENOSYS` or another explicit error so callers can detect that reading is unsupported.
</issue_to_address>
### Comment 7
<location> `platform/generic/thead/thead-generic.c:299-301` </location>
<code_context>
+ sync_is();
+}
+
+static void sbi_thead_tcm0_pmp_set(unsigned long auth)
+{
+ sbi_printf("%s: auth:%lx \n", __func__, auth);
+ unsigned int num, reg_val;
+
</code_context>
<issue_to_address>
**nitpick (performance):** Verbose `sbi_printf` in `sbi_thead_tcm0_pmp_set` may be too noisy for a hot path.
This function logs on every call, including the `auth` value. If it’s invoked frequently (e.g. via SBI calls from the kernel), this can flood the console and degrade performance. Please gate this behind a debug option or remove it once bring-up/validation is done.
</issue_to_address>
### Comment 8
<location> `platform/generic/thead/light_c910.c:194-201` </location>
<code_context>
+ light_auxcore_entryboot_set();
+}
+
+static int light_hart_start(u32 hartid, ulong saddr)
+{
+ int ret;
+
+ /* send ipi to triger already plugout core which will be waiting in sbi_hsm_hart_wait
+ * after reset.
+ */
+ if (sbi_entry_count(hartid) == C910_CORE0)
+ return sbi_ipi_raw_send(sbi_hartid_to_hartindex(hartid), true);
+
</code_context>
<issue_to_address>
**question (bug_risk):** `light_hart_start` uses `sbi_entry_count(hartid)` to special-case core0, which is likely not what `sbi_entry_count` is intended for.
This condition is effectively using `sbi_entry_count(hartid)` as a hart identifier to decide when to send an IPI. However, `sbi_entry_count()` is meant to count how many times a hart has entered OpenSBI, not which hart it is, so its value will change over time. If you need to special‑case the boot hart (e.g. hart 0), please check `hartid` (or a platform-specific boot hart ID) directly instead of relying on the entry count.
</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
+442
to
+444
| static bool thead_generic_cold_boot_allowed(u32 hartid) | ||
| { | ||
| if (selected_hartid != -1) |
There was a problem hiding this comment.
issue (bug_risk): Comparing unsigned selected_hartid against -1 makes the check always true and effectively pins cold boot to hart 0.
selected_hartid is a u32 initialized to 0, but thead_generic_cold_boot_allowed checks if (selected_hartid != -1). Since -1 promotes to UINT_MAX for the comparison, this condition is always true and only hartid == selected_hartid (currently 0) is allowed to cold boot. If the intent is “any hart unless explicitly selected,” use a sentinel that’s representable (e.g., make selected_hartid signed and init to -1, or track selection with a separate boolean).
Comment on lines
+405
to
+410
| struct sbi_ecall_extension ecall_light = { | ||
| .name = "light", | ||
| .extid_start = SBI_EXT_VENDOR_PMU, | ||
| .extid_end = SBI_EXT_VENDOR_PMP, | ||
| .register_extensions = sbi_ecall_light_register_extensions, | ||
| .handle = sbi_ecall_light_handler, |
There was a problem hiding this comment.
issue (bug_risk): The ecall_light extension calls sbi_ecall_register_extension(&ecall_light) from its own register_extensions hook, which is likely recursive.
Here ecall_light.register_extensions points to sbi_ecall_light_register_extensions(), which in turn calls sbi_ecall_register_extension(&ecall_light). Since register_extensions is invoked by the core during extension registration, this will re-register the same extension and can cause recursion or double registration. If there are no sub-extensions, either set .register_extensions = NULL and only register ecall_light from thead_generic_final_init(), or keep the hook and avoid explicitly registering it again in final_init.
Comment on lines
+62
to
+65
| #define RPC_GET_BE64(MESG, OFFSET, PTR) \ | ||
| do { \ | ||
| uint8_t *data = (uint8_t *)(MESG); \ | ||
| *(uint32_t *)(PTR) = \ |
There was a problem hiding this comment.
issue (bug_risk): RPC_GET_BE64 writes a 64-bit value through a uint32_t *, which is a type and size mismatch.
This macro computes a 64-bit value but assigns it through a uint32_t *, causing truncation on 32-bit targets and violating strict-aliasing/size assumptions on 64-bit targets. The store should use a 64-bit type (e.g. *(uint64_t *)(PTR)), or the macro should return the 64-bit value instead, matching the behavior of RPC_SET_BE64.
Comment on lines
+23
to
+28
| #define RPC_GET_SVC_ID(MESG) ((MESG)->svc & 0x3F) | ||
| #define RPC_SET_SVC_ID(MESG, ID) ((MESG)->svc |= 0x3F & (ID)) | ||
| #define RPC_GET_SVC_FLAG_MSG_TYPE(MESG) (((MESG)->svc & 0x80) >> 7) | ||
| #define RPC_SET_SVC_FLAG_MSG_TYPE(MESG, TYPE) ((MESG)->svc |= (TYPE) << 7) | ||
| #define RPC_GET_SVC_FLAG_ACK_TYPE(MESG) (((MESG)->svc & 0x40) >> 6) | ||
| #define RPC_SET_SVC_FLAG_ACK_TYPE(MESG, ACK) ((MESG)->svc |= (ACK) << 6) |
There was a problem hiding this comment.
suggestion (bug_risk): RPC_SET_SVC_ID only ORs in bits and never clears previous service ID bits, which can accumulate stale state.
Since it uses |= without first clearing the lower 6 bits, calling it on a message with an existing svc value (or calling it multiple times with different IDs) can leave old ID bits set. A safer pattern would be: ((MESG)->svc = ((MESG)->svc & ~0x3F) | ((ID) & 0x3F)). Consider applying a similar mask-and-set approach to the flag setters if messages may be reused.
Suggested change
| #define RPC_GET_SVC_ID(MESG) ((MESG)->svc & 0x3F) | |
| #define RPC_SET_SVC_ID(MESG, ID) ((MESG)->svc |= 0x3F & (ID)) | |
| #define RPC_GET_SVC_FLAG_MSG_TYPE(MESG) (((MESG)->svc & 0x80) >> 7) | |
| #define RPC_SET_SVC_FLAG_MSG_TYPE(MESG, TYPE) ((MESG)->svc |= (TYPE) << 7) | |
| #define RPC_GET_SVC_FLAG_ACK_TYPE(MESG) (((MESG)->svc & 0x40) >> 6) | |
| #define RPC_SET_SVC_FLAG_ACK_TYPE(MESG, ACK) ((MESG)->svc |= (ACK) << 6) | |
| #define RPC_GET_SVC_ID(MESG) ((MESG)->svc & 0x3F) | |
| #define RPC_SET_SVC_ID(MESG, ID) \ | |
| ((MESG)->svc = ((MESG)->svc & ~0x3F) | ((ID) & 0x3F)) | |
| #define RPC_GET_SVC_FLAG_MSG_TYPE(MESG) (((MESG)->svc & 0x80) >> 7) | |
| #define RPC_SET_SVC_FLAG_MSG_TYPE(MESG, TYPE) \ | |
| ((MESG)->svc = ((MESG)->svc & ~0x80) | (((TYPE) & 0x1) << 7)) | |
| #define RPC_GET_SVC_FLAG_ACK_TYPE(MESG) (((MESG)->svc & 0x40) >> 6) | |
| #define RPC_SET_SVC_FLAG_ACK_TYPE(MESG, ACK) \ | |
| ((MESG)->svc = ((MESG)->svc & ~0x40) | (((ACK) & 0x1) << 6)) |
Comment on lines
+299
to
+301
| static void sbi_thead_tcm0_pmp_set(unsigned long auth) | ||
| { | ||
| sbi_printf("%s: auth:%lx \n", __func__, auth); |
There was a problem hiding this comment.
nitpick (performance): Verbose sbi_printf in sbi_thead_tcm0_pmp_set may be too noisy for a hot path.
This function logs on every call, including the auth value. If it’s invoked frequently (e.g. via SBI calls from the kernel), this can flood the console and degrade performance. Please gate this behind a debug option or remove it once bring-up/validation is done.
Comment on lines
+194
to
+201
| static int light_hart_start(u32 hartid, ulong saddr) | ||
| { | ||
| int ret; | ||
|
|
||
| /* send ipi to triger already plugout core which will be waiting in sbi_hsm_hart_wait | ||
| * after reset. | ||
| */ | ||
| if (sbi_entry_count(hartid) == C910_CORE0) |
There was a problem hiding this comment.
question (bug_risk): light_hart_start uses sbi_entry_count(hartid) to special-case core0, which is likely not what sbi_entry_count is intended for.
This condition is effectively using sbi_entry_count(hartid) as a hart identifier to decide when to send an IPI. However, sbi_entry_count() is meant to count how many times a hart has entered OpenSBI, not which hart it is, so its value will change over time. If you need to special‑case the boot hart (e.g. hart 0), please check hartid (or a platform-specific boot hart ID) directly instead of relying on the entry count.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by Sourcery
Add TH1520/light SoC-specific support for power management, hotplug, PMU, PMP, and mailbox-based AON coordination on the T-Head generic platform, integrate XTHEADSSTC timer handling and vendor SBI extensions, and wire the new platform pieces into generic init, trap, timer, and build/CI flows.
New Features:
Enhancements:
CI: