Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1456 
was reviewed by Christian Mauderer

--
  
Christian Mauderer started a new discussion: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1456#note_158124


Could you rebase it to the current main again and remove the commits that are 
already in main ("libdl: *", "libcsupport: *")? That would make it simpler to 
review.

--
  
Christian Mauderer started a new discussion on 
bsps/arm/stm32u5/start/bspstarthooks.c: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1456#note_158125

 > +  stm32u5_init_peripheral_clocks();
 > +#endif
 > +

You duplicate the initialization functions. I think I would only put the `if` 
in the `ifdef`. Something like
```
#ifdef STM32U5_MEMORY_OCTOSPI_1_SIZE
  if ( (void*) stm32u5_init_octospi < (void*) stm32u5_memory_octospi_1_begin ||
       (void*) stm32u5_init_octospi > (void*) stm32u5_memory_octospi_1_end ) {
#else
  if ( true ) {
#endif
```

--
  
Christian Mauderer started a new discussion on 
spec/build/bsps/arm/stm32u5/grp.yml: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1456#note_158126

 >  - role: build-dependency
 > -  uid: optmemintsramsz
 > +  uid: optmemsramsz

Why did you rename that RAM? It's still internal even if it is the only one, 
isn't it?

--
  
Christian Mauderer started a new discussion on 
spec/build/bsps/arm/stm32u5/optenusart2.yml: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1456#note_158127

 > +# If your BSP is listed in the first enabled-by
 > +# operator, this usart will become your console
 > +#

Why don't you add that comment to the `description` field instead of a comment 
at the top of the yml file? If it's important information, it could be useful 
in the output of the `waf bspdefaults`.

That's true for all of the comments that you added to option yml files.

--
  
Christian Mauderer started a new discussion on 
spec/build/bsps/arm/stm32u5/optmemflashsz.yml: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1456#note_158128

 > +- enabled-by:
 > +  - arm/stm32u5-grisp-nano
 > +  - arm/stm32u5-discovery

If all currently supported BSPs use the same value, just stick to `enabled-by: 
true`. Usually only BSPs with a different value have an explicit `enabled-by`.

--
  
Christian Mauderer started a new discussion on 
spec/build/bsps/arm/stm32u5/linkcmds/linkcmds-discovery.yml: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1456#note_158129

 > +  REGION_ALIAS ("REGION_STACK", SRAM);
 > +  REGION_ALIAS ("REGION_NOCACHE", SRAM);
 > +  REGION_ALIAS ("REGION_NOCACHE_LOAD", FLASH);

It seems that this is more or less the same like the existing `linkcmds.flash`. 
Why didn't you just extend the linkcmds.memory to include the extra `BKUP_SRAM` 
and `LP_SRAM`? If they are not default to the STM32U5, you can just add an 
option for the size of these. Code can do something depending on whether size 
is 0 or not 0.


-- 
View it on GitLab: 
https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1456
You're receiving this email because of your account on gitlab.rtems.org. 
Unsubscribe from this thread: 
https://gitlab.rtems.org/-/sent_notifications/5-4h5bsruj9gvdaizg9lt7f7yi2-1d/unsubscribe
 | Manage all notifications: https://gitlab.rtems.org/-/profile/notifications | 
Help: https://gitlab.rtems.org/help


_______________________________________________
bugs mailing list
[email protected]
http://lists.rtems.org/mailman/listinfo/bugs

Reply via email to