Skip to content
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

Add STM32H7Rx/Sx #972

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add STM32H7Rx/Sx #972

wants to merge 5 commits into from

Conversation

jspngh
Copy link
Contributor

@jspngh jspngh commented Mar 29, 2024

I'd like to add support for these newly released devices.

However, I am unsure if they should be added to the STM32H7 family or added as a separate family.
On their site, ST lists them under the "STM32H7" category, but for the rest they seem to treat them like a new family (with a separate Cube package and HAL).
What do you think?

One annoying thing about these new SVD's, is that they tend to use the bit position of a field as the name.

Example
              <field>
                <name>UE</name>
                <description>USART enable ...</description>
                <bitOffset>0</bitOffset>
                <bitWidth>1</bitWidth>
                <access>read-write</access>
                <enumeratedValues>
                  <enumeratedValue>
                    <name>B_0x0</name>
                    <description>USART prescaler and outputs disabled, low-power mode</description>
                    <value>0x0</value>
                  </enumeratedValue>
                  <enumeratedValue>
                    <name>B_0x1</name>
                    <description>USART enabled</description>
                    <value>0x1</value>
                  </enumeratedValue>
                </enumeratedValues>
              </field>

I'm not sure how to solve this without manually overwriting the values with meaningful names.

@burrbull
Copy link
Contributor

burrbull commented Mar 29, 2024

just delete all those enums with "_clear_fields"

_clear_fields: ["*"]

@jspngh
Copy link
Contributor Author

jspngh commented Mar 29, 2024

just delete all those enums with "_clear_fields"

Thanks, that seems like the best option.

Any opinion on where they belong (in STM32H7 or a separate family)?

@adamgreig
Copy link
Member

I think keeping them in the stm32h7 crate is probably best, we also kept the stm32l4+ in the l4 crate and I think it's a similar distinction.

It doesn't really matter what they have in common as nothing is shared between devices in a crate anyway; it's just about keeping each crate a reasonable size and having things where people expect to find them.

@jspngh
Copy link
Contributor Author

jspngh commented Mar 29, 2024

I think keeping them in the stm32h7 crate is probably best, we also kept the stm32l4+ in the l4 crate and I think it's a similar distinction.

As far as I can see, ST uses the same cube package and hal for L4 and L4+, so it's slightly different for these chips. But I'm perfectly fine with keeping them in the stm32h7 crate.

I guess we can always split them off later if necessary?

@jspngh
Copy link
Contributor Author

jspngh commented Mar 29, 2024

The "compare mmaps" step will continue to fail because of the new SVD files, I guess?

@jspngh jspngh marked this pull request as ready for review March 29, 2024 20:45
@adamgreig
Copy link
Member

Yep, for security reasons it can only run on the SVD files already present in the repository. Do you think this is otherwise ready for review? Have you been able to test it at all?

@burrbull
Copy link
Contributor

Yep, for security reasons it can only run on the SVD files already present in the repository. Do you think this is otherwise ready for review? Have you been able to test it at all?

#949

@jspngh
Copy link
Contributor Author

jspngh commented Mar 29, 2024

Do you think this is otherwise ready for review? Have you been able to test it at all?

It's definitely not perfect yet, but I hope it is a good enough starting point to be mergeable.
I'm going to start with a blinky/hello world now, so we could wait for that before merging.

I don't expect big issues with getting something to run, but maybe those are famous last words 😅

@jspngh
Copy link
Contributor Author

jspngh commented Mar 30, 2024

A simple blinky works.
I'll start working with some of the peripherals by trying to add support to stm32h7xx-hal for these devices,
but that will probably take quite some time.

@adamgreig, is there anything you would like to see checked or tested in particular or are you okay with merging this now and then fixing potential issues in a follow-up PR?

@adamgreig
Copy link
Member

Thanks, looks good. The only thing I wanted to double check is deriving all the GPIOs from GPIOA. Typically GPIOA (and B) have a different reset value for MODER, OSPEEDR, and PUPDR than the other ports because of the JTAG/SWD pins there. IF everything derives from GPIOA, doing something like gpioc.moder.write(|w| w) would incorrectly set PC13/14/15 to alternate function. You can see devices/common_patches/gpio/f3_reset_values.yaml for an example of handling this where there's GPIOA, GPIOB, and GPIOC, and all other ports derive from GPIOC.

@jspngh
Copy link
Contributor Author

jspngh commented Mar 30, 2024

Thanks, looks good. The only thing I wanted to double check is deriving all the GPIOs from GPIOA. Typically GPIOA (and B) have a different reset value for MODER, OSPEEDR, and PUPDR than the other ports because of the JTAG/SWD pins there. IF everything derives from GPIOA, doing something like gpioc.moder.write(|w| w) would incorrectly set PC13/14/15 to alternate function. You can see devices/common_patches/gpio/f3_reset_values.yaml for an example of handling this where there's GPIOA, GPIOB, and GPIOC, and all other ports derive from GPIOC.

You are correct, that is indeed the case (the different reset value), I didn't know it mattered.
I'll fix it!

@jspngh
Copy link
Contributor Author

jspngh commented Mar 31, 2024

I was looking into this and comparing with the other STM32H7 devices.
I noticed this issue is present there as well: all GPIOs are derived from GPIOA, but not all reset values are the same.

I propose to fix this for the entire family, but maybe that should be a separate PR?
And just to check, this means that each "different reset value" will result in a new gpiox module with distinct RegisterBlock, right?

@jspngh
Copy link
Contributor Author

jspngh commented Apr 1, 2024

I created #973 to fix the issue for the current STM32H7 devices.
When that is merged, I can rebase this PR and fix it in the same way for the new devices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants