Sg2042 v1.8 #21
Open
Sg2042 v1.8 #21
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
pisces system reboot will ecall gpio chip but not watchdog, and pull up cpu gpio5 to tell CPLD. CPLD will help for cold reboot. Add the function, gpio chip will pull up gpio16 after chip is probed. Signed-off-by: chunzhi.lin <chunzhi.lin@sophgo.com>
This problem was found on sg2042 server platform with the litmus test. This patch is based on Guo Ren's patch [1]. [1] c-sky/csky-linux@c539252 riscv: qspinlock: errata: Add ERRATA_THEAD_WRITE_ONCE fixup The early version of T-Head C9xx cores has a store merge buffer delay problem. The store merge buffer could improve the store queue performance by merging multi-store requests, but when there are not continued store requests, the prior single store request would be waiting in the store queue for a long time. That would cause significant problems for communication between multi-cores. This problem was found on sg2042 & th1520 platforms with the qspinlock lock torture test. So appending a fence w.o could immediately flush the store merge buffer and let other cores see the write result. This will apply the WRITE_ONCE errata to handle the non-standard behavior via appending a fence w.o instruction for WRITE_ONCE(). Signed-off-by: haijiao.liu@sophgo.com <haijiao.liu@sophgo.com>
… deletion process Signed-off-by: haijiao.liu <haijiao.liu@sophgo.com>
Signed-off-by: Inochi Amaoto <inochiama@outlook.com>
Add a virtual global timer device driver. It uses a global timer
register as timer counter, and it uses mtimecmp CSR of clint as
interrupt source.
Using this virtul timer resolves dual way timer sync issue.
On dual way system, we should enable this driver and on single way
system, we should use standard risc-v clint timer.
1. Add a virtual global timer device driver, it is probed by fdt
sg2042-global-mtimer@70300101c0 {
compatible = "sophgo,sg2042-global-mtimer";
reg = < 0x00000070 0x300101c0 0x00000000 0x00000008
0x00000070 0xac000000 0x00000000 0x00200000>;
clock-frequency = <50000000 50000000>;
};
Its compatible should be "sophgo,sg2042-global-mtimer".
And the first region delcared in reg prop is the address of global
timer. The second region delcared in reg prop is the base of clint
timers.
The first clock in clock-frequency is the actual frequency of global
timer, the second clock is the actual frequency of clint timer.
2. Add a quirk in platform specific code.
Add a function sbi_platform_force_emulate_time_csr, it indicates if
opensbi should skip time CSR detection and make software in
supervisor and user mode use emulated time CSR.
Signed-off-by: Chao Wei <chao.wei@sophgo.com>
Signed-off-by: Han Gao <gaohan@iscas.ac.cn>
Reviewer's GuideAdds Sophgo SG2042/Mango platform support for global timer (GMT), GPIO, and multiple reset mechanisms, introduces a platform hook to force emulated time CSR, tightens memory-ordering primitives and spinlock/coldboot fences, and fixes PMU overflow handling and warm-boot hart comparison while wiring in new FDT compatibles and build options. Sequence diagram for SG2042 GMT timer initialization from FDTsequenceDiagram
participant Boot as Bootloader
participant Fw as OpenSBI_firmware
participant FDT as FDT_blob
participant FdtCore as fdt_timer
participant Drv as fdt_timer_sg2042_gmt
participant GMT as sg2042gmt_cold_timer_init
participant TimerCore as sbi_timer
Boot->>Fw: pass FDT pointer
Fw->>FdtCore: fdt_timer_cold_init(FDT)
FdtCore->>FDT: find_node(compatible = sophgo,sg2042-global-mtimer)
FDT-->>FdtCore: nodeoff
FdtCore->>Drv: init(FDT, nodeoff)
Drv->>FDT: fdt_get_node_addr_size(index 0)
FDT-->>Drv: mtimer_base, mtimer_size
Drv->>FDT: fdt_get_node_addr_size(index 1)
FDT-->>Drv: mtimecmp_base, mtimecmp_size
Drv->>FDT: fdt_parse_timebase_frequency
FDT-->>Drv: declared_freq
Drv->>FDT: getprop(clock-frequency)
FDT-->>Drv: actual_freq, timecmp_freq
Drv->>GMT: sg2042gmt_cold_timer_init(mtimer_base, mtimecmp_base, mtimecmp_size, declared_freq, actual_freq, timecmp_freq)
GMT->>TimerCore: sbi_timer_set_device(&sg2042gmt_timer)
TimerCore-->>GMT: OK
GMT-->>Drv: OK
Drv-->>FdtCore: OK
FdtCore-->>Fw: timers initialized
Updated class diagram for SG2042 timer and FDT driver structuresclassDiagram
class sg2042gmt_data {
+unsigned long mtimer_base
+unsigned long mtimecmp_base
+unsigned long mtimecmp_size
+unsigned long declared_freq
+unsigned long actual_freq
+unsigned long timecmp_freq
}
class sbi_timer_device {
+char* name
+unsigned long timer_freq
+u64 (*timer_value)()
+void (*timer_event_start)(u64 next_event)
+void (*timer_event_stop)()
+int (*warm_init)()
}
class sg2042gmt_timer_device {
<<instance_of_sbi_timer_device>>
+name = "sg2042gmt"
+timer_freq
+timer_value()
+timer_event_start(u64 next_event)
+timer_event_stop()
+warm_init()
}
class fdt_driver {
+const struct fdt_match* match_table
+int (*init)(const void* fdt, int nodeoff, const struct fdt_match* match)
}
class fdt_driver_sg2042_gmt {
<<instance_of_fdt_driver>>
+match_table = timer_sg2042gmt_match
+init = fdt_sg2042gmt_cold_timer_init
}
class sg2042_gmt_api {
+int sg2042gmt_cold_timer_init(unsigned long mtimer_base, unsigned long mtimecmp_base, unsigned long mtimecmp_size, unsigned long delcared_freq, unsigned long actual_freq, unsigned long timecmp_freq)
+int sg2042gmt_warm_timer_init()
}
sg2042gmt_data <.. sg2042_gmt_api : uses
sg2042gmt_timer_device <.. sg2042_gmt_api : configured_by
sg2042gmt_timer_device --|> sbi_timer_device
fdt_driver_sg2042_gmt --|> fdt_driver
fdt_driver_sg2042_gmt ..> sg2042_gmt_api : calls
Updated class diagram for Sophgo GPIO and Mango reset devicesclassDiagram
class gpio_chip {
+const void* driver
+int id
+unsigned int ngpio
+int (*direction_output)(struct gpio_pin* gp, int value)
+void (*set)(struct gpio_pin* gp, int value)
}
class gpio_pin {
+struct gpio_chip* chip
+unsigned int offset
}
class sophgo_gpio_chip {
+unsigned long addr
+struct gpio_chip chip
}
class fdt_gpio {
+struct fdt_driver driver
+int (*xlate)(...)
}
class fdt_gpio_sophgo_instance {
<<instance of fdt_gpio>>
+driver.match_table = sophgo_gpio_match
+driver.init = sophgo_gpio_init
+xlate = fdt_gpio_simple_xlate
}
class sbi_system_reset_device {
+char* name
+int (*system_reset_check)(u32 type, u32 reason)
+void (*system_reset)(u32 type, u32 reason)
}
class cpld_reset {
+struct gpio_pin pin
+u32 active_delay
+u32 inactive_delay
+gpio_pin* pin
}
class mango_reset_gpio_poweroff_device {
<<instance of sbi_system_reset_device>>
+name = "mango-cpld"
+system_reset_check = cpld_system_poweroff_check
+system_reset = cpld_system_poweroff
}
class mango_reset_gpio_reboot_device {
<<instance of sbi_system_reset_device>>
+name = "mango-cpld"
+system_reset_check = cpld_system_reboot_check
+system_reset = cpld_system_reboot
}
class i2c_adapter {
+int nr
+int (*reg_read)(...)
+int (*reg_write)(...)
}
class mango_mcu_state {
+struct i2c_adapter* adapter
+u32 reg
}
class mango_reset_i2c_device {
<<instance of sbi_system_reset_device>>
+name = "mango-reset"
+system_reset_check = mango_system_reset_check
+system_reset = mango_system_reset
}
class mango_reset_wdt_device {
<<instance of sbi_system_reset_device>>
+name = "mango-wdt"
+system_reset_check = sophgo_wdt_system_reset_check
+system_reset = sophgo_wdt_system_reset
}
sophgo_gpio_chip --> gpio_chip : embeds
cpld_reset --> gpio_pin : uses
fdt_gpio_sophgo_instance --> fdt_gpio : instance_of
fdt_gpio_sophgo_instance ..> sophgo_gpio_chip : init_chips
mango_reset_gpio_poweroff_device --> sbi_system_reset_device
mango_reset_gpio_reboot_device --> sbi_system_reset_device
mango_reset_i2c_device --> sbi_system_reset_device
mango_mcu_state --> i2c_adapter : holds
mango_reset_wdt_device --> sbi_system_reset_device
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 4 issues, and left some high level feedback:
- In fdt_gpio_sophgo.c, sophgo_gpio_addr_get() derives the base address from the parent node's "reg" assuming a fixed 2-cell 64-bit address layout; consider using fdt_get_node_addr_size() on the GPIO node itself (respecting #address-cells/#size-cells) to avoid hard‑coding and potential misparsing on different DTs.
- The GPIO chip size in fdt_gpio_sophgo.c is set to SOPHGO_GPIO_PINS_MAX (31) even though offsets are 0–31 inclusive; if the hardware exposes 32 pins, set ngpio accordingly (e.g., use SOPHGO_GPIO_PINS_MAX + 1) to avoid silently ignoring the last GPIO.
- In sg2042_gmt.c, the frequency ratio arithmetic (e.g., actual_freq / declared_freq, timecmp_freq / actual_freq) assumes these divisors are nonzero and divide cleanly; consider adding basic validation (and perhaps using 64-bit-safe scaling or gcd-based reduction) to avoid divide-by-zero or large rounding errors if DT values are unexpected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In fdt_gpio_sophgo.c, sophgo_gpio_addr_get() derives the base address from the parent node's "reg" assuming a fixed 2-cell 64-bit address layout; consider using fdt_get_node_addr_size() on the GPIO node itself (respecting #address-cells/#size-cells) to avoid hard‑coding and potential misparsing on different DTs.
- The GPIO chip size in fdt_gpio_sophgo.c is set to SOPHGO_GPIO_PINS_MAX (31) even though offsets are 0–31 inclusive; if the hardware exposes 32 pins, set ngpio accordingly (e.g., use SOPHGO_GPIO_PINS_MAX + 1) to avoid silently ignoring the last GPIO.
- In sg2042_gmt.c, the frequency ratio arithmetic (e.g., actual_freq / declared_freq, timecmp_freq / actual_freq) assumes these divisors are nonzero and divide cleanly; consider adding basic validation (and perhaps using 64-bit-safe scaling or gcd-based reduction) to avoid divide-by-zero or large rounding errors if DT values are unexpected.
## Individual Comments
### Comment 1
<location> `lib/utils/gpio/fdt_gpio_sophgo.c:132-134` </location>
<code_context>
+ return rc;
+
+ chip->addr = addr;
+ chip->chip.driver = &fdt_gpio_sophgo;
+ chip->chip.id = nodeoff;
+ chip->chip.ngpio = SOPHGO_GPIO_PINS_MAX;
+ chip->chip.direction_output = sophgo_gpio_direction_output;
+ chip->chip.set = sophgo_gpio_set;
</code_context>
<issue_to_address>
**issue (bug_risk):** GPIO ngpio should likely be count, not max index
`SOPHGO_GPIO_PINS_MAX` is 31, but `ngpio` should be the count of GPIOs rather than the highest index. For pins 0..31, `ngpio` should be 32 (e.g., `SOPHGO_GPIO_PINS_MAX + 1`), otherwise the last pin may not be addressable and the range semantics will be off.
</issue_to_address>
### Comment 2
<location> `lib/utils/reset/fdt_reset_sophgo_wdt.c:73-74` </location>
<code_context>
+ const fdt32_t *val;
+ int len, noff;
+
+ val = fdt_getprop(fdt, nodeoff, "subctrl-syscon", &len);
+ if (val || len >= sizeof(fdt32_t)) {
+ noff = fdt_node_offset_by_phandle(fdt, fdt32_to_cpu(*val));
+ if (noff < 0)
</code_context>
<issue_to_address>
**issue (bug_risk):** Logical condition should likely use && instead of || when checking FDT property
`if (val || len >= sizeof(fdt32_t))` will succeed whenever either the pointer is non-NULL or `len` is large enough, which means you can call `fdt32_to_cpu(*val)` when `val` is NULL or when the property is too short. This risks reading an invalid phandle. Use `if (val && len >= sizeof(fdt32_t))` so you only proceed when the property exists and contains at least one 32-bit cell.
</issue_to_address>
### Comment 3
<location> `lib/sbi/riscv_locks.c:67` </location>
<code_context>
#error "need A or Zaamo or Zalrsc"
#endif
+ "fence w, o\n"
+
/* Did we get the lock? */
</code_context>
<issue_to_address>
**suggestion (performance):** Additional fence in spin_lock hot path may hurt performance without clear benefit
The `fence w, o` inside the spin_lock loop adds a barrier on every failed CAS in the contended path and may degrade scalability, given that `__smp_store_release` and other barriers already provide ordering for lock acquire/release. Please confirm that this specific fence is needed for a particular device-ordering requirement; if not, consider removing it or relocating it outside the hot retry loop.
</issue_to_address>
### Comment 4
<location> `include/sbi_utils/timer/sg2042_gmt.h:13-18` </location>
<code_context>
+int sg2042gmt_cold_timer_init(unsigned long mtimer_base,
+ unsigned long mtimecmp_base,
+ unsigned long mtimecmp_size,
+ unsigned long delcared_freq,
+ unsigned long actual_freq,
+ unsigned long timecmp_freq);
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in parameter name `delcared_freq` in SG2042 GMT timer header
The prototype uses `delcared_freq` but the implementation uses `declared_freq`. Please align the spelling here to avoid confusion when searching or reading the header.
```suggestion
int sg2042gmt_cold_timer_init(unsigned long mtimer_base,
unsigned long mtimecmp_base,
unsigned long mtimecmp_size,
unsigned long declared_freq,
unsigned long actual_freq,
unsigned long timecmp_freq);
```
</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
+132
to
+134
| chip->chip.driver = &fdt_gpio_sophgo; | ||
| chip->chip.id = nodeoff; | ||
| chip->chip.ngpio = SOPHGO_GPIO_PINS_MAX; |
There was a problem hiding this comment.
issue (bug_risk): GPIO ngpio should likely be count, not max index
SOPHGO_GPIO_PINS_MAX is 31, but ngpio should be the count of GPIOs rather than the highest index. For pins 0..31, ngpio should be 32 (e.g., SOPHGO_GPIO_PINS_MAX + 1), otherwise the last pin may not be addressable and the range semantics will be off.
Comment on lines
+73
to
+74
| val = fdt_getprop(fdt, nodeoff, "subctrl-syscon", &len); | ||
| if (val || len >= sizeof(fdt32_t)) { |
There was a problem hiding this comment.
issue (bug_risk): Logical condition should likely use && instead of || when checking FDT property
if (val || len >= sizeof(fdt32_t)) will succeed whenever either the pointer is non-NULL or len is large enough, which means you can call fdt32_to_cpu(*val) when val is NULL or when the property is too short. This risks reading an invalid phandle. Use if (val && len >= sizeof(fdt32_t)) so you only proceed when the property exists and contains at least one 32-bit cell.
| #error "need A or Zaamo or Zalrsc" | ||
| #endif | ||
|
|
||
| "fence w, o\n" |
There was a problem hiding this comment.
suggestion (performance): Additional fence in spin_lock hot path may hurt performance without clear benefit
The fence w, o inside the spin_lock loop adds a barrier on every failed CAS in the contended path and may degrade scalability, given that __smp_store_release and other barriers already provide ordering for lock acquire/release. Please confirm that this specific fence is needed for a particular device-ordering requirement; if not, consider removing it or relocating it outside the hot retry loop.
Comment on lines
+13
to
+18
| int sg2042gmt_cold_timer_init(unsigned long mtimer_base, | ||
| unsigned long mtimecmp_base, | ||
| unsigned long mtimecmp_size, | ||
| unsigned long delcared_freq, | ||
| unsigned long actual_freq, | ||
| unsigned long timecmp_freq); |
There was a problem hiding this comment.
nitpick (typo): Typo in parameter name delcared_freq in SG2042 GMT timer header
The prototype uses delcared_freq but the implementation uses declared_freq. Please align the spelling here to avoid confusion when searching or reading the header.
Suggested change
| int sg2042gmt_cold_timer_init(unsigned long mtimer_base, | |
| unsigned long mtimecmp_base, | |
| unsigned long mtimecmp_size, | |
| unsigned long delcared_freq, | |
| unsigned long actual_freq, | |
| unsigned long timecmp_freq); | |
| int sg2042gmt_cold_timer_init(unsigned long mtimer_base, | |
| unsigned long mtimecmp_base, | |
| unsigned long mtimecmp_size, | |
| unsigned long declared_freq, | |
| unsigned long actual_freq, | |
| unsigned long timecmp_freq); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 Sophgo SG2042 Mango platform support including timer, reset, GPIO, and PMU handling improvements
New Features:
Bug Fixes:
Enhancements:
Build: