Skip to content

soc: microchip: Introduce pic32cxsg sg41 #86952

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

MyGh64605
Copy link
Contributor

Commits for pic32cxsg SG41 Curiosity Ultra board, 'Hello World' basics, arm, pinctrl, serial, board

@scottwcpg scottwcpg requested a review from nandojve March 11, 2025 18:01
@nandojve nandojve self-assigned this Mar 11, 2025

id: device_id@8061fc {
compatible = "atmel,sam0-id";
reg = <0x008061FC 0x4>,
Copy link
Contributor

Choose a reason for hiding this comment

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

double space after equals

Copy link
Contributor

Choose a reason for hiding this comment

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

not addressed

Comment on lines 1 to 3
# Microchip PIC32CXSG MCU family configuration options

# Copyright (c) 2024 Microchip
# SPDX-License-Identifier: Apache-2.0
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
# Microchip PIC32CXSG MCU family configuration options
# Copyright (c) 2024 Microchip
# SPDX-License-Identifier: Apache-2.0
# Microchip PIC32CXSG MCU family configuration options
# Copyright (c) 2024 Microchip
# SPDX-License-Identifier: Apache-2.0

having all headers the same would be nice

*
* SPDX-License-Identifier: Apache-2.0
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

this file already exists, move the existing file to a common place and include it

Copy link
Member

Choose a reason for hiding this comment

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

what could be a common place for 2 different vendors?

Copy link
Contributor

Choose a reason for hiding this comment

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

atmel is microchip so it can be microchip, or maybe even soc/common

This option enables the SERCOMx USART driver for Microchip PIC32CXSG MCUs.

config UART_PIC32CXSG_ASYNC
bool "Async UART support for Microchip PIC32CXSG series."
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
bool "Async UART support for Microchip PIC32CXSG series."
bool "Async UART support for Microchip PIC32CXSG series"

Copy link
Contributor

Choose a reason for hiding this comment

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

not fixed?

Copy link
Contributor

Choose a reason for hiding this comment

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

all images -> webp then use https://tinypng.com/

Copy link
Contributor

Choose a reason for hiding this comment

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

not addressed

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

The pic32cxsg41_cult board configuration supports the following hardware
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
The pic32cxsg41_cult board configuration supports the following hardware
The ``pic32cxsg41_cult`` board supports the following hardware

@MyGh64605 MyGh64605 force-pushed the pic32cxsg_helloworld branch from fb11141 to 7ff07ad Compare April 15, 2025 17:41
@MyGh64605
Copy link
Contributor Author

@nandojve @nordicjm Completed compliance checks

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

fix raised issues

@MyGh64605
Copy link
Contributor Author

@nordicjm please clarify your issues pertaining to:

  •   pinmux_c: pinmux@41008100 {
    
  •   	compatible = "atmel,sam0-pinmux";
    
  •   	reg = <0x41008100 0x80>;
    
  •   };
    
  •   pinmux_d: pinmux@41008180 {
    
  •   	compatible = "atmel,sam0-pinmux";
    
  •   	reg = <0x41008180 0x80>;
    
  •   };
    
  •   wdog: watchdog@40002000 {
    
  •   	compatible = "atmel,sam0-watchdog";
    
  •   	reg = <0x40002000 13>;
    
  •   	interrupts = <10 0>;
    
  •   };
    

Note. this file was fixed as follows:
wdog: watchdog@40002000 {
compatible = "atmel,sam0-watchdog";
reg = <0x40002000 13>;
interrupts = <10 0>;
status = "disabled";
};
I cannot disable the 'pinmux's since they are integral to 'pinctrl' i.e. peripheral mux selection

  • };
  • soc {
  •   sram0: memory@20000000 {
    
  •   	compatible = "mmio-sram";
    
  •   	reg = <0x20000000 0x40000>;
    
  •   };
    
  •   backup0: memory@47000000 {
    
  •   	compatible = "mmio-sram";
    
  •   	reg = <0x47000000 0x2000>;
    
  •   };
    
  •   id: device_id@8061fc {
    
  •   	compatible = "atmel,sam0-id";
    
  •   	reg =	<0x008061FC 0x4>,
    

not addressed

Thank you.

@nordicjm
Copy link
Contributor

5856d44#diff-7862f1c4371a54c6987f57ec439dff007b41c783916fd8a9fa40dab8b6ea433eR84

double space after equals, original comment raised, not fixed comment raised, still not fixed

5856d44#diff-7862f1c4371a54c6987f57ec439dff007b41c783916fd8a9fa40dab8b6ea433eR170

nodes should be disabled in files in dts folder then enabled by the board, nodes that do not have status = "disabled"; will be enabled, original comment raised, not fixed comment raised, still not fixed

4ecdf64#diff-b4bd73e4a70f8a965919730caf1e466cef1f54de68132c629187f352e1562506R16

Missing newline, between elements, original comment raised, still not fixed

etc.

@MyGh64605
Copy link
Contributor Author

@nandojve @nordicjm @scottwcpg @TheSilverBB

We force pushed more changes on 04/15/2025, Zephyr ran validation tests afterwards.
We made some more changes due to these checks but cannot push due to conflicts. We noticed that the PR branch did not update, nor did our 'microchiptech' branch. we are wondering why the changes were not accepted, is it because draft PRs are handled differently?

@kartben
Copy link
Contributor

kartben commented Apr 17, 2025

We made some more changes due to these checks but cannot push due to conflicts.

You need to resolve the conflicts and force push again. Please share more details if you still experience issues

@MyGh64605
Copy link
Contributor Author

@nandojve @scottwcpg @nordicjm @TheSilverBB
Since the original PR was submitted from a windows repo and the changes were submitted from a WSL repo. We believe it is best that we cancel this PR and then we will create a new.
Any objections or suggestions ?

@kartben
Copy link
Contributor

kartben commented Apr 25, 2025

I don't think there should be anything special about a "windows" or a "WSL" repo. Just sync / force push the changes you made to this PR's branch?
Creating a new PR should really be avoided as history of reviews/discussions would be lost.

@scottwcpg
Copy link
Contributor

@kartben
Hi. I have been help the PR author. We observed force pushing changes up triggers CI but the files are not being updated with the local changes. The WSL setup was created by creating the same branch name not by pulling down the PR. This may be why it does not update the files. Should the PR author save off local files, delete the local branch, pull down the PR, rebase applying the local changes, then force push? Would this re-sync local and the PR?

@MyGh64605 MyGh64605 force-pushed the pic32cxsg_helloworld branch from 7ff07ad to 138a957 Compare April 29, 2025 18:57
@nandojve nandojve requested a review from nordicjm April 29, 2025 19:39
@MyGh64605
Copy link
Contributor Author

@nandojve
Are the tests run on each individual commit ?
We tried your command. "git push -f --set-upstream origin pic32cxsg_helloworld"
And all of our files reverted back to before our changes, even locally.

We want to kill this PR and start over, using WSL.

If we separate into different PRs, would this help ?

We add soc, dts, etc..
The last PR would be 'boards' .
At that time would all the compliance tests be executed ?
Or do the tests run on each PR ?

@nandojve
Copy link
Member

Hi @MyGh64605 ,

@nandojve Are the tests run on each individual commit ?
No, they only run on top of latest commit.

We tried your command. "git push -f --set-upstream origin pic32cxsg_helloworld" And all of our files reverted back to before our changes, even locally.

We want to kill this PR and start over, using WSL.

If we separate into different PRs, would this help ?

My afraid is that if you close this and open another PR that will be the 4th attempt. This means that mostly on each changes that we request you open a new PR and we lost all the history. In my impression, the way you are managing the git workflow is not OK. I work with people everyday and I don't this kind of problems. for example, you could copy the content from the problematic machine and transfer to another one to send the new content.

git is a very reliable tool and if you know it well you know that content don't vanish. Try to check your git reflog for instance.

We add soc, dts, etc.. The last PR would be 'boards' . At that time would all the compliance tests be executed ? Or do the tests run on each PR ?

I believe that documentation is clear about this and we should only bring content that can be build. At moment, the most painful job in Zephyr is bring a new SoC. It is really exhausting and I'm not the only one that think that this should be changed to allow somehow easy adoption.

On my perspective the content that is on this PR is the minimum to introduce the platform in a way that you can use it. What is necessary is figure out how you can review the request in 24h and send the updates ASAP.

In regards to close this PR and open a new one I'll abstain myself and let that to @nordicjm and @kartben to decide.

@nordicjm
Copy link
Contributor

nordicjm commented May 9, 2025

In regards to close this PR and open a new one I'll abstain myself and let that to @nordicjm and @kartben to decide.

Yeah please just fix the pr as-is, don't care how you do it on your choice of OS but millions of people use github and are able to submit PRs so fix your environment and update the PR but do not close this and open a duplicate PR, that's not helpful to anyone

@MyGh64605 MyGh64605 force-pushed the pic32cxsg_helloworld branch 2 times, most recently from 92df3a3 to c22e022 Compare May 21, 2025 17:19
@MyGh64605
Copy link
Contributor Author

@nandojve @scottwcpg @nordicjm @TheSilverBB
We have successfully passed all checks.
Please review and let us know the next step(s).
When can we press the 'Ready for review' button ?

@nordicjm
Copy link
Contributor

@nandojve @scottwcpg @nordicjm @TheSilverBB We have successfully passed all checks. Please review and let us know the next step(s). When can we press the 'Ready for review' button ?

Can press that button whenever you feel it is ready to review, it will then ping people asking to review

@MyGh64605 MyGh64605 marked this pull request as ready for review May 22, 2025 12:57
@github-actions github-actions bot added area: HWINFO Hardware Information Driver platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) area: Pinctrl platform: Microchip MEC Microchip MEC Platform area: UART Universal Asynchronous Receiver-Transmitter labels May 22, 2025
@TheSilverBB
Copy link

TheSilverBB commented Aug 1, 2025

This PR is still not anywhere near ready, the soc part is a mess of mismatches values and referencing things that don't even exist, other comments have still not been fixed 2 months on. Raised issues need to be fixed, this PR has wasted so much of my time coming back and re-reviewing it only to see that previously raised issues are still present, it takes time out of my day to review things which could be spent on other reviews

@nordicjm Thank you very much for all your time in reviewing this PR. We feel the same pain as you.

We believe all raised issues are now addressed.

@nandojve @kartben @NhMchp @parthitce

@nandojve
Copy link
Member

nandojve commented Aug 1, 2025

Hi Michael ,

Please, reorder the commit in the following order:

manifest: update west.yml for snakecase changes
modules: microchip Addition of pic32cxsg modules
dts: arm: microchip Addition of pic32cxsg
soc: microchip: pic32cxsg Addition of pic32cxsg
drivers: pinctrl Addition of pic32cxsg pin control
drivers: serial: microchip Addition of pic32cxsg serial
boards: microchip Addition of pic32cxsg board

After reorder, you should extract DTS information from commits with other content. This will result in more commits as below:

manifest: update west.yml for snakecase changes
modules: microchip Addition of pic32cxsg modules
dts: arm: microchip Addition of pic32cxsg
soc: microchip: pic32cxsg Addition of pic32cxsg
dts: pinctrl: xxx
drivers: pinctrl Addition of pic32cxsg pin control
dts: serial: xxx
drivers: serial: microchip Addition of pic32cxsg serial
boards: microchip Addition of pic32cxsg board

In the dts: arm: microchip Addition of pic32cxsg you will reduce to the minimal, see 2cc4cb6 if you have any doubt.
In the dts: pinctrl: xxx and dts: serial: xxx you will add all the information related, see 0088612

Make sure that all devicetree nodes are ordered by address, not name.

@nandojve nandojve changed the title Pic32cxsg helloworld - commits for pic32cxsg SG41 soc: microchip: Introduce pic32cxsg sg41 Aug 1, 2025
@robertperkel
Copy link
Contributor

Can you add an alias for one of the built-in LEDs? Currently blinky does not build for this board

@TheSilverBB
Copy link

@robertperkel

Can you add an alias for one of the built-in LEDs? Currently blinky does not build for this board

Short Answer: No.

Long Answer: Later.
When a new soc is added, after the vendor hal is merged, a board PR is created which must support the Hello World project, and only peripherals required to support the Hello World project are included.
Once the board PR for Hello World is merged, then PRs for each remaining peripheral drivers are created and merged and verified on the board.
Blinky uses GPIOs as a peripheral, which will be added after this PR is merged.
Blinky will build for this board after the GPIO driver is added.
PWM Blinky will build after the PWM driver is added.

@nandojve @NhMchp

@MyGh64605 MyGh64605 force-pushed the pic32cxsg_helloworld branch from e0da164 to 455db95 Compare August 4, 2025 15:56
@TheSilverBB
Copy link

@nandojve @kartben @NhMchp @parthitce @nordicjm
Is this PR ready to merge?
Is it ok to change the west.yml commit from “revision: pull/34/head” to the latest hal_microchip hash now?
Thanks.

@parthitce
Copy link
Member

@nandojve @kartben @NhMchp @parthitce @nordicjm Is this PR ready to merge? Is it ok to change the west.yml commit from “revision: pull/34/head” to the latest hal_microchip hash now? Thanks.

If the HAL changes / PR is already merged, please update the manifest and rebase this PR. This will help the CI pass fully.

Addition of pic32cxsg modules Kconfig

Signed-off-by: Michael D Sherwood <[email protected]>
Addition of pic32cxsg 'dtsi' files

Signed-off-by: Michael D Sherwood <[email protected]>
@scottwcpg scottwcpg force-pushed the pic32cxsg_helloworld branch from 455db95 to e6b42ca Compare August 7, 2025 14:44
@github-actions github-actions bot removed manifest manifest-hal_microchip DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Aug 7, 2025
@scottwcpg
Copy link
Contributor

@parthitce @nandojve @TheSilverBB
The original author of the PR, Michael has retired. I was asked to do the final update.
The PR has been rebased to upstream which pulled in an updated west.yaml pointing to a fix for cmsis_6 and Michael's last fix for this PR.

@TheSilverBB
Copy link

@nandojve @NhMchp The HAL hash code has been updated. Please press the Resolve Conflicts button.
@scottwcpg

Comment on lines 8 to 13
bool "The external 32 kHz crystal oscillator"
depends on DT_HAS_ATMEL_SAM0_OSC32KCTRL_ENABLED
help
Say y to enable the external 32 kHZ crystal oscillator at
startup. This can then be selected as the main clock source
for the SOC.
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this need to be a Kconfig if the crystal is enabled or disabled in devicetree? If the node is there and has status okay that that means it's present and should be used

Choose a reason for hiding this comment

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

@nordicjm Your request has been incorporated, Please mark as Resolved. Thanks.

Copy link
Contributor

@scottwcpg scottwcpg Aug 14, 2025

Choose a reason for hiding this comment

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

@nordicjm Updated Kconfig.pic32cxsg and removed the ones you pointed out. The remaining setting is set to y if osc32kctrl DT node is enabled.

#define TCC3 (0x42001000) /**< \brief (TCC3) APB Base Address */
#define TCC4 (0x43001000) /**< \brief (TCC4) APB Base Address */
#define TRNG (0x42002800) /**< \brief (TRNG) APB Base Address */
#define USB (0x41000000) /**< \brief (USB) APB Base Address */
Copy link
Contributor

@jfischer-no jfischer-no Aug 11, 2025

Choose a reason for hiding this comment

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

I cannot imagine the use case for these defines. That is what DT_INST_REG_ADDR(n) is for in the drivers. There is no way the soc.h could have something like #define USB.

By the way, the Doxygen comments in soc.h are absolutely pointless.

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably comes from the DFP pack for the device. These macros are intended for compatibility with other compiler toolchains outside of Zephyr.

The way I understand it, the direction going forward is to keep the DFP packs as unedited in Zephyr as possible, so it is easy to upgrade them, but to store them into the hal project, rather than mainline. Perhaps the DFP files should be placed there, and soc.h should only include the appropriate header? Here's a snippet of my WIP soc.h, for reference:

#if defined(CONFIG_SOC_PIC32CM5112GC00100)
#define __PIC32CM5112GC00100__
#elif defined(CONFIG_SOC_PIC32CM5112GC00064)
#define __PIC32CM5112GC00064__
#elif defined(CONFIG_SOC_PIC32CM5112GC00048)
#define __PIC32CM5112GC00048__
#else
#error Device not supported
#endif

#include <pic32c.h>

(Inside pic32c.h, the DFP macros grab the correct includes / definitions for the specific device)

Copy link
Contributor

@scottwcpg scottwcpg Aug 14, 2025

Choose a reason for hiding this comment

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

@jfischer-no I removed all the defines except two: MCLK and GCLK which are required by the Atmel SAM0 UART driver. MCLK and GCLK get their values using DT_REG_ADDR().

Addition of pic32cxsg current family of devices

Signed-off-by: Michael D Sherwood <[email protected]>
Added pinctrl node to PIC32CXSG dtsi without driver compatible

Signed-off-by: Michael Sherwood <[email protected]>
Addition of pic32cxsg pin control including yaml, bindings, Kconfig,

Signed-off-by: Michael D Sherwood <[email protected]>
Use microchip pic32cxsg compatiable for sercom

Signed-off-by: Michael Sherwood <[email protected]>
Addition of pic32cxsg serial driver, Kconfig, yaml

Signed-off-by: Michael D Sherwood <[email protected]>
Addition of pic32cxsg SG41 Curiosity Ultra board

Signed-off-by: Michael D Sherwood <[email protected]>
@scottwcpg scottwcpg force-pushed the pic32cxsg_helloworld branch from e6b42ca to daa9f91 Compare August 14, 2025 20:09
Copy link

Comment on lines +15 to +16
config HAS_PIC32C_HAL
bool "Microchip PIC32C HAL drivers support"
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this used? And why does it have a prompt?

Comment on lines +22 to +25
config HAS_PIC32CXSG_HAL
bool "Microchip PIC32CXSG HAL drivers support"
select HAS_CMSIS_CORE
depends on SOC_FAMILY_MICROCHIP_PIC32CXSG
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this used? And why does it have a prompt?

@nordicjm nordicjm dismissed their stale review August 15, 2025 07:32

soc/board files OK

# SPDX-License-Identifier: Apache-2.0

config SOC_FAMILY_MICROCHIP_PIC32CXSG
select PIC32C
Copy link
Member

Choose a reason for hiding this comment

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

@MyGh64605 ,

It seems that you want select options to your HAL based in the family. Why not just select all this options in the module itself, something like:

config PIC32C
	bool
	select HAS_CMSIS_CORE

config HAS_PIC32C_HAL
	bool
	select PIC32C

config HAS_PIC32CXSG_HAL
	bool
	select HAS_PIC32C_HAL

# SPDX-License-Identifier: Apache-2.0

config SOC_FAMILY_MICROCHIP_PIC32CXSG
select PIC32C
Copy link
Member

Choose a reason for hiding this comment

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

What you want is define all the HAL at once. This can be achieved by

-       select PIC32C
+       select HAS_PIC32CXSG_HAL


/ {
soc {
nvmctrl@41004000 {
Copy link
Member

Choose a reason for hiding this comment

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

Order by memory address. This is the devicetree standard we use.
Review all to make sure we did not missed any.


if SOC_FAMILY_MICROCHIP_PIC32CXSG

config SOC_MICROCHIP_PIC32CXSG_XOSC32K
Copy link
Member

Choose a reason for hiding this comment

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

Since it is on devicetree, this config is useless and must be dropped.

@@ -0,0 +1,404 @@
/*
* Copyright (c) 2024 Microchip
Copy link
Member

Choose a reason for hiding this comment

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

If you copy and rename, you should keep original copyrights.
Add original copyrights.

# Copyright (c) 2024 Microchip
# SPDX-License-Identifier: Apache-2.0

description: Microchip PIC32CXSG SERCOM UART driver
Copy link
Member

Choose a reason for hiding this comment

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

this is the devicetree binding, should be in the previous commit.

@@ -64,6 +64,7 @@ zephyr_library_sources_ifdef(CONFIG_UART_NUMICRO uart_numicro.c)
zephyr_library_sources_ifdef(CONFIG_UART_NXP_S32_LINFLEXD uart_nxp_s32_linflexd.c)
zephyr_library_sources_ifdef(CONFIG_UART_OPENTITAN uart_opentitan.c)
zephyr_library_sources_ifdef(CONFIG_UART_PDL_INFINEON_CAT1 uart_ifx_cat1_pdl.c)
zephyr_library_sources_ifdef(CONFIG_UART_PIC32CXSG uart_sam0.c)
Copy link
Member

Choose a reason for hiding this comment

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

This driver support only one binding at moment.

#define DT_DRV_COMPAT atmel_sam0_uart

Comment on lines +79 to +91
&tcc0 {
compatible = "atmel,sam0-tcc-pwm";

/* Gives a maximum period of 1.1s for 120MHz main clock */

prescaler = <8>;

#pwm-cells = <2>;

pinctrl-0 = <&pwm_default>;
pinctrl-names = "default";
status = "okay";
};
Copy link
Member

Choose a reason for hiding this comment

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

The only drivers that should be available are pinctrl and serial. Drop all others.


zephyr_sources(soc_port.c)

zephyr_sources_ifdef(CONFIG_BOOTLOADER_BOSSA bossa.c)
Copy link
Member

Choose a reason for hiding this comment

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

This should be dropped for now. When you add on a board (another PR) you can add here.

# Copyright (c) 2024 Microchip
# SPDX-License-Identifier: Apache-2.0

if ETH_SAM_GMAC
Copy link
Member

Choose a reason for hiding this comment

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

This is not essential.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: HWINFO Hardware Information Driver area: Pinctrl area: UART Universal Asynchronous Receiver-Transmitter platform: Microchip MEC Microchip MEC Platform platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants