Skip to content

Conversation

kokas-a
Copy link

@kokas-a kokas-a commented Apr 10, 2023

Minimal HSDK4xD support

@kokas-a kokas-a force-pushed the arc_hs4x branch 7 times, most recently from 8455a43 to e37e29b Compare April 12, 2023 14:46
@kokas-a
Copy link
Author

kokas-a commented Apr 12, 2023

@evgeniy-paltsev Could you please take a look? PR has same issues with checks, but it wold be nice to have your feedback on key points

@kokas-a kokas-a force-pushed the arc_hs4x branch 5 times, most recently from 9d3ad81 to 1218bbb Compare April 17, 2023 12:39
@kokas-a kokas-a requested a review from evgeniy-paltsev April 17, 2023 13:43
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this is still related to some issue?

Comment on lines 9 to 11
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
zephyr_compile_options(-arcv2hs -core4 -Xdual_issue -Xcode_density -Hrgf_banked_regs=32 -HL
-Xatomic -Xll64 -Xunaligned -Xdiv_rem=radix4 -Xswap -Xbitscan -Xmpy_option=qmpyh -Xshift_assist
-Xbarrel_shifter -Xtimer0 -Xtimer1 -Xrtc -Hld_cycles=2 -Xlpb_size=128)
zephyr_compile_options(-arcv2hs -core4 -Xdual_issue -Xcode_density -Hrgf_banked_regs=32 -HL
-Xatomic -Xll64 -Xunaligned -Xdiv_rem=radix4 -Xswap -Xbitscan -Xmpy_option=qmpyh -Xshift_assist
-Xbarrel_shifter -Xtimer0 -Xtimer1 -Xrtc -Hld_cycles=2)

Alignment fix and drop Xlpb_size as we disable LPB anyway.

Comment on lines 16 to 17
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if DT_NODE_HAS_PROP(DT_CHOSEN(zephyr_sram), reg) && \
(DT_REG_SIZE(DT_CHOSEN(zephyr_sram)) > 0)
#if DT_NODE_HAS_PROP(DT_CHOSEN(zephyr_sram), reg) && (DT_REG_SIZE(DT_CHOSEN(zephyr_sram)) > 0)

This fits to 100 character per line limit.

arch/arc/Kconfig Outdated
Comment on lines 391 to 392
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rephrase it to:

Call SoC per-core setup code on early stage initialization (before C runtime initialization).
Setup code is called in form of soc_early_asm_init_percpu assembler macro.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Done: #57348

Copy link
Contributor

@abrodkin abrodkin left a comment

Choose a reason for hiding this comment

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

A couple of minor notes below. And what about documentation update or maybe even a separate docs page for that board?

arch/arc/Kconfig Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

satge -> stage

Comment on lines 9 to 10
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also shouldn't be just a TODO.

Minimal HSDK4xD support

Signed-off-by: Nikolay Agishev <[email protected]>
soc_early_asm_init_percpu
#endif

_dsp_extension_probe
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this dsp_extension__probe (and the CONFIG_DSP_TURNED_OFF) more?
The 'probe' name implies it is testing if the DSP extension is present or not, but the implementation and comments say it is turning off DSP.
Is the mechanism for turning off DSP generic for ARC? does it work on EM as well? If not, maybe move this to the SOC specific early_soc_init instead of this generic reset.S module.

Copy link
Author

Choose a reason for hiding this comment

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

Initially the macro looked like:

.macro _dsp_extension_probe
	lr	 r0, [_ARC_V2_DSP_BUILD]
	bmsk r0, r0, 7
	breq r0, 0, skip_dsp_setup
#ifdef CONFIG_ARC_DSP_TURNED_OFF
	mov	 r0, 0 /* DSP_CTRL_DISABLED_ALL */
	sr	 r0, [_ARC_V2_DSP_CTRL]
#endif
skip_dsp_setup :
.endm

After all stages of review it turned into what you may see. Indeed, in current state it's more appropriate to move it into early_soc_init.

Copy link
Member

Choose a reason for hiding this comment

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

I see it was already merged. Please do so in a follow-up PR (this was minimal enablement only, anyway, and documentation is missing).
Does writing 0 to DSP_CTRL really 'turn DSP off"? Or just initializes some control flags?

Copy link
Author

Choose a reason for hiding this comment

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

DSP_CTRL does not really turn off DSP. Writing 0 disables some DSP features

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for confirming, that's what I remembered. So, not only the 'probe' but also the "DSP_TURNED_OFF" should be renamed. Actually, I don't think we need a kconfig option for that, it is just standard initialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants