-
Notifications
You must be signed in to change notification settings - Fork 2
Mchp zephyr dspic release v3.0 #2
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.
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
Conversation
Highlights: - Added support for xc-dsc toolchain: CMake scripts, linker files - Arch bring-up for dsPIC (Curiosity board): thread creation, early boot, swap - CMake: Improved path detection, removed debug messages, static lib fixes - Interrupt vector table (IVT) initialization and macro definition - MISRA compliance fixes - Various bug fixes, stylistic cleanup, and compliance-related updates Changes: * cmake: Add xc-dsc toolchain support and linker script * arch: dspic: Rename isr_table_vt.ld to vector_table.ld * camke: Fix path detection for build dependencies * cmake: Resolve include path issues * cmake: Remove debug messages * arch: dspic: Add thread creation and init logic * arch: Resolve arch_swap implicit declaration warning * arch: Add vector tables and common ISR wrapper * cmake: Enable compiling assembly files * cmake: Fix static library linking * arch: Support IVT table placement and macros * cmake: Skip init priorities check for xc-dsc * toolchain: Remove stdlib usage (use picolibc instead) * arch: Remove use of unimplemented functions * arch: Early boot sequence implementation * arch: Task swap and main thread entry logic Signed-off-by: Udhayanandhan Jayakumar <[email protected]>
Add copyright and license headers to Kconfigs and other files Signed-off-by: Muhammed Zamroodh <[email protected]>
The custom const sections are placed in FLASH and being copied from .dinit to Flash again, We can avoid that by placing the section in RAM to begin with Signed-off-by: Muhammed Zamroodh <[email protected]>
Highlights: - Context switching and thread creation - Timer driver and system timer integartion - UART driver and LOG integration - Fixes for linking and toolchain support - Power management (idle state) - Interrupt support in OS - Twister and flash support - General fixes Signed-off-by: Muhammed Zamroodh <[email protected]>
3e081af
to
e120593
Compare
static inline __attribute__((always_inline)) void z_dspic_save_context(void) | ||
{ | ||
/*Adjust stack for co-operative swap*/ | ||
__asm__ volatile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of inline assembly isn't it good to use pure assembly code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean can we have this functions as part of .S file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that, initially this was part of core zephyr hooks.
But can now do that as we separated it from the hooks, will take this as a task for next release (As we might need to test the build and functionalities after)
address = PCTRAP; | ||
LOG_ERR("ERROR !!! Exception reason = %s, address = 0x%x\n", "Illegal Instruction", | ||
address); | ||
INTCON1bits.BADOPERR = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for using different approaches in different fucntions for clearing the trap (using mask or writing to 0 to the bitfield)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came from different developers, we will unify the approaches.
failed to catch this in the review
ANSELB = 0x033FUL; | ||
|
||
/* Assign U1TX to RP23 and U1RX to RP24*/ | ||
_RP23R = 9; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are pins are configured as part of driver? Shouldn't it be part of dts ? There is no guarantee that these pins will be available in variants of dsPIC33AK(Low pin count devices)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are developing a pin control driver and with that the configurations for the pin mux will come from the dts files.
You can expect this in the next release
volatile uint32_t *UxTXREG = (void *)(cfg->base + OFFSET_TXREG); | ||
|
||
/* Wait until there is space in the TX FIFO */ | ||
while ((bool)(void *)((*UxSTA) & BIT_TXBF)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this enough -> "while (*UxSTA & BIT_TXBF)" , why do we need the multiple typecasting here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to satisfy the MISRA compliance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this linker file is specific to dsPIC33AK128MC106? should we have this file as part of folder dsPIC33A? we do have other variants of dsPICs(dsPIC33E, dsPIC33C) as well , the linker scripts for those may vary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have plans to to do that, this will be addressed with the support for the 2nd chip
|
||
void arch_irq_enable(unsigned int irq) | ||
{ | ||
volatile uint32_t *int_enable_reg[] = {&IEC0, &IEC1, &IEC2, &IEC3, &IEC4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these registers can differ between dsPIC33A , dsPIC33C and other dsPIC architectures. should we have specific folders for this , instead having this part of "dsPIC" folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will assess how to modify this and redesign.
__start: | ||
;; Initialize stack pointer and limit | ||
mov.l #__SP_init, w15 ; Stack pointer (W15) | ||
mov.l #__SPLIM_init, w14 ; Stack limit (W14) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the comment still not addressed https://github.com/Zephyr4Microchip/zephyr/pull/1/files#r2200117916
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will add a hot fix for this. We missed to merge the commit.
Will push this as part of the v3 itself
dspic: Windows host fixes for west flash West flash tool was failing, modified flash tool script In windows build was failing with Path name with spaces, using modified cmake script. Signed-off-by: Adhil Xavier <[email protected]>
|
||
int arch_irq_is_enabled(unsigned int irq) | ||
{ | ||
volatile uint32_t *int_enable_reg[] = {&IEC0, &IEC1, &IEC2, &IEC3, &IEC4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dsPIC33AK128MC106 has IEC0 to IEC8
dsPIC33AK512MPS512 has IEC0 to IEC11 , shouldn't we handle this in a proper way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will assess how to modify this and redesign.
* clear all the interrupts, set the interrupt flag status | ||
* registers to zero. | ||
*/ | ||
IFS0 = 0x0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dsPIC33AK128MC106 has IFS0 to IFS8
dsPIC33AK512MP512 has IFS0 to IFS11 , shouldn't we handle this in a proper way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will assess how to modify this and redesign.
|
||
static int uart_dspic_init(const struct device *dev) | ||
{ | ||
LATB = 0x0040UL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is right to add the pin configuration code here, this can vary from device to devices having same architecture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uart file name not following RFC: zephyrproject-rtos#92168
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harishagari : this change will come as part of pin control driver
@NhMchp : Noted, we will make the changes
|
||
/* | ||
** ============== Equates for SFR Addresses ============= | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linker script varies from device to device , and these(SFR) addresses as well. Compiler/DFP provides linker scripts for each device. Dont we need an generic way to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in works, will be addressed with the support for the 2nd chip
Setting CXX compiler evironment variable to xc-dsc-gcc Toolchain path issues fixed. Signed-off-by: Adhil Xavier <[email protected]>
@MuhammedZamroodh Please resolve the conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please resolve the conflicts and sync the branch with develop branch(if not already done)
Release V3.3