Skip to content

soc: egis_et171: update config and resolve issue of ram func #97960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+10 −11
Open

soc: egis_et171: update config and resolve issue of ram func #97960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c061245
boards: egis_et171: Remove unnecessary default configuration
Oct 21, 2025
d24d5e2
soc: egis_et171: Add weak description to soc_early_init_hook
Oct 21, 2025
4aeaa8a
soc: egis_et171: Fix the i-cache activation order issue
Oct 21, 2025
2dc54c6
doc: boards: update index.rst
Oct 21, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions boards/egis/egis_et171/Kconfig.defconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,2 @@
# Copyright (c) 2025 Egis Technology Inc.
# SPDX-License-Identifier: Apache-2.0

DT_CHOSEN_Z_FLASH := zephyr,flash

config FLASH_LOAD_SIZE
default $(dt_chosen_reg_size_hex,$(DT_CHOSEN_Z_FLASH))
6 changes: 1 addition & 5 deletions boards/egis/egis_et171/doc/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ The platform provides following hardware components:
- DMA
- USB

.. figure:: img/et171_snapshot.webp
:align: center
:alt: EGIS_ET171_SOC

Supported Features
==================

Expand All @@ -34,7 +30,7 @@ Supported Features
Connections and IOs
===================

The et171 platform has 1 GPIO controller. It providing 17 bits of IO.
The et171 platform has 1 GPIO controller. It is providing 17 bits of IO.
It is responsible for pin input/output, pull-up, etc.

The et171 platform has 3 SPI controllers, 2 of which can
Expand Down
10 changes: 9 additions & 1 deletion soc/egis/et171/soc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,16 @@
#include <zephyr/init.h>
#include <zephyr/kernel.h>

void soc_early_init_hook(void)
__weak void soc_early_init_hook(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What feature needs to be overridden instead of application appending the initialization via SYS_INIT()?
Copy link
Author

@jackylee-go jackylee-go Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, not all drivers for this SOC are completed.
For example, setting the root clock is implemented in 'soc_early_init_hook'.
Therefore, if you want to change the clock, you must override soc_early_init_hook.
Changing the root clock at runtime using SYS_INIT() can pose some risks.
(Zephyr does not expect clocks to be changed at runtime.)
Copy link
Author

@jackylee-go jackylee-go Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, other drivers that haven't issue a PR have been completed and verified as modules.
However, some implementations require to modify 'soc_early_init_hook', which will cause conflicts when using upstream to verify.
Add a 'weak' declaration can avoid those conflicts.
Copy link
Contributor

@jimmyzhe jimmyzhe Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clock requirement should be solved by implement a clock driver or add Egis Kconfig option to set the clock.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyross @peter-mitsis for comment on making soc_early_init_hook weak
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other vendor has a weak symbol for these and late/early soc hooks are used 174 times in zephyr, if you need it or have these issues I would say your drivers or system setup is badly architected or misdesigned
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I emphasize that not all drivers for this SOC are fully implemented.
This declaration is added solely to facilitate smoother development of subsequent drivers.
If the relevant driver has already been developed, this declaration can be removed.

If this approach is not appropriate for upstream , please let me know.
I’ll try to find an alternative solution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this basically reiterates why this soc should never have been added upstream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use SYS_INIT with INIT_LEVEL_EARLY, thats even happening before soc_early_init_hook or use board_early_init_hook , that is directly after soc_early_init_hook

In general if you are developing drivers, that you plan to contribute work in-tree
Copy link
Author

@jackylee-go jackylee-go Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that this isn't a order issue of 'SYS_INIT'.
It's that there might be different implementations that conflict with each other.

But based on the current upstream content, even if there is a conflict, there won't be any problem.
{
#if CONFIG_XIP && CONFIG_ICACHE
/* Since caching is enabled before z_data_copy(), RAM functions may still be cached
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean arch_data_copy ? there is no z_data_copy
Copy link
Author

@jackylee-go jackylee-go Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it named 'arch_data_copy' now.

commit 0f4e1acb

SHA-1: 94e171155ac0830020b8e41d31aa132cb37d2c7b * arch: init: rename z_data_copy -> arch_data_copy Do not use private API prefix and move to architecture interface as those functions are primarily used across arches and can be defined by the architecture.
* in the d-cache instead of being written to SRAM. In this case, the i-cache will
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what section from the linker is the relevant here, because, normally this should only copy data over?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ramfunc section is also copied through this function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we then do it in kernel/xip.c and use the cache api and implement the icache driver for riscv processors with the Zifencei extention?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the arch_cache_xxx functions are not yet implemented.
And cache enabling functionality is implemented directly in the assembly code.
Therefore, I performed the fix directly in the assembly code.

Otherwise, we must abandon the implementation in reset.S and fully implement arch_cache_xxx.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now we can leave it here, but in the long therm we should move over to the cache api
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, we must abandon the implementation in reset.S and fully implement arch_cache_xxx.

arch/riscv/custom/andes/reset.S enables I/D Cache for SoC using Andes Custom CSR, which speeds up the boot sequence. I don't prefer to remove this.

I think the main problem is that the RAM function copy operation doesn’t handle I/D-cache synchronization right now.

zephyr/arch/common/xip.c

Lines 31 to 34 in b677e82

#ifdef CONFIG_ARCH_HAS_RAMFUNC_SUPPORT
arch_early_memcpy(&__ramfunc_region_start, &__ramfunc_load_start,
__ramfunc_end - __ramfunc_region_start);
#endif /* CONFIG_ARCH_HAS_RAMFUNC_SUPPORT */

arch_data_copy() runs too early that cache driver (ARCH/EXTERNAL) not initialize yet. Maybe introducing arch_early_ramfunc_memcpy() is an option (execute fence.i in RISC-V port), since the current arch_early_memcpy() cannot handle instruction and data synchronization.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you create a general issue for that?
Then I'm fine with the quick fix.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jackylee-go for the feedback.
This issue also affects AE350, so I created #98046 to record it.

I am ok with this fix too.
* fetch the wrong content from SRAM. Thus, using "fence.i" to fix it.
*/
__asm__ volatile("fence.i" ::: "memory");
#endif

/* AHB ~200Mhz / 3 = 66MHz AHB , ~66Mhz / 2 / 2 = 18Mhz APB */
sys_write32(0x01 | 0x20 | 0x02, 0xF0100004U);

Expand Down