Added POLARBERRY board support (SundanceDSP company) #11
Conversation
Signed-off-by: Roman.K <roman.k@sundancedsp.com>
There was a problem hiding this comment.
Hi @Mans-sdsp,
Thank you for submitting the PR to add Polarberry support to Linux4Microchip's Buildroot-External. We have reviewed your PR and made comments, suggesting improvements and asking for further clarification on certain things.
The overall major comments are to remove the HSS package and to make use of (or update, if necessary) the existing Linux and U-Boot dts and config files that are in our existing repositories to support the Polarberry board.
We hope you find this feedback helpful and we look forward to your updates.
| @@ -0,0 +1 @@ | |||
| images | |||
There was a problem hiding this comment.
This directory should not be here and therefore there should be no need to add this file. Delete this file.
There was a problem hiding this comment.
This file is not necessary. You should not be hardcoding/setting any of these statically. You can attain all of this information from Buildroot/the system itself, should it be required.
There was a problem hiding this comment.
This file is used for auto-flashing procedure based on BASH scripts. This procedure requires UART (device name in Linux)/ETHERNET(IP addresses of the PC and Board) and the base buildroot directory used for compilation. The UART can not be obtained from the system, due to user PC configuration may be different: PC may contain more than one USB/UART converter, etc. For example - my working PC contains two usb/uart converters that can be in "off state" time to time, it cause the issue: the polarberry board connected to a PC can be on a different device (/dev/ttyUSB0, /dev/ttyUSB1, /dev/ttyUSB2, etc.) and this can not be predicted/predefined. User shall set manually set UART device user for Polarberry... Also for IP-Addresses (PC/Polarberry). I can obtain Polarberry IP from buildroot configuration, but PC IP address also can not be obtained/predicted, due to PC may have more than one Ethernets, and only user knows what Ethernet Device user for Polarberry connection (For example, my PC has 3 Ethernet cards (one for office network and two for devices)... Also for buildroot directory: only user knows what directory is used for buildroot as "BR2_EXTERNAL", there are no other way to get the path to buildroot sources, except ask the user (via configuration file, for example). So, this is not hardcoded/statically settings, this is settings user shall to change/fix according to PC's configuration... Automatic process for board flashing is very useful for end-user. Is it possible to leave this file?
There was a problem hiding this comment.
Since you are running this outside of the Buildroot build and the end-user is going to be passing their own values, please make these arguments instead of having this file in-tree with these values set.
There was a problem hiding this comment.
Can you please rename this file? Get rid of all the capitalisation. This should also have a yaml extension. Consider renaming to: payload-config.yaml
There was a problem hiding this comment.
Please rename file - remove random capitalisation. Similar to Linux programming script. Can you make use of the full generated sdcard.img instead of programming u-boot and linux separately? This is more prone to errors such as overriding partitions.
There was a problem hiding this comment.
Polarberry has no SD card. Image is split/separated for "uboot image" + "Linux image" due to not all users reflash the board with both SW (uboot+linux). In must cases I have faced, only virgin boards programmed via "programs_all", and next only Linux (less often uboot) is reflashed. That's why we created different scripts for different tasks.
There was a problem hiding this comment.
The sdcard.img can be used to program U-Boot and Linux via eMMC. If you want to provide separate scripts for separate U-Boot and Linux programming for the reason above, that is fine. I am asking if you can utilise the sdcard.img for when users want to program both. The HSS is not to be programmed in this way (see comment on adding the HSS package for more detail).
There was a problem hiding this comment.
Please rename file - remove capitalisation.
| BR2_TARGET_UBOOT=y | ||
| BR2_TARGET_UBOOT_BUILD_SYSTEM_KCONFIG=y | ||
| BR2_TARGET_UBOOT_CUSTOM_GIT=y | ||
| BR2_TARGET_UBOOT_CUSTOM_REPO_URL="https://github.com/polarfire-soc/u-boot.git" |
There was a problem hiding this comment.
Use release tarballs with instead of git repo links and branches. You can see an example here
There was a problem hiding this comment.
will be fixed to "$(call github,polarfire-soc,u-boot)mpfs-uboot-2023.07-next.tar.gz".
| BR2_PACKAGE_HOST_HSS_PAYLOAD_GENERATOR=y | ||
| BR2_PACKAGE_HOST_HSS_PAYLOAD_GENERATOR_CFG="$(BR2_EXTERNAL_MCHP_PATH)/board/microchip/polarberry/HSS_payload_config" | ||
| BR2_PACKAGE_HOST_HSS_PAYLOAD_GENERATOR_SRC="output/images/u-boot.bin" | ||
| BR2_PACKAGE_HART_SOFTWARE_SERVICES=y |
There was a problem hiding this comment.
Why are you adding and using the hss package like this? The HSS is highly coupled to the FPGA flow rather than the Linux build system flow. It also depends on Libero/SoftConsole being installed on the build system. We do not want the Buildroot build to depend on any of this. For these reasons, the HSS should be flashed outside of the Linux build system involvement. Please remove the addition of the Hart Software Services package.
There was a problem hiding this comment.
Actually HSS is just a bootloader, like FSBL, uboot, barebox, etc. Buildroot able to compile uboot, barebox, uboot-spl, and I think will be useful to add other one bootloader for Microchip. Yes, you are partly right - final stage of HSS building requires additional SW from microchip ("Softconsole" from Microchip shall be installed and "Program debug tools" from Microchip). This software is available for free, and in case user has not installed it, buildroot building will be successful anyway. If user have no such software, building will print "Warning about additional software". That is why I added HSS as a package. Believe, will be better to add it to the "botloaders" section in the buildroot menuconfig, but package was simpler. Maybe I can ADD some environment checking, and HSS package will be unavailable if user have no required SW ("SoftConsole" "and program debug tools")? Additional bottloader from Microchip in the buildroot will improve buildroot functionality I sure... What doe you think?
There was a problem hiding this comment.
Firstly, if the HSS package were to be included, and the user does not have SoftConsole (or any other necessary requirements) installed then the build should absolutely fail with a loud error as this would be vital for the build - should the user be using this feature. Adding this feature is not a general feature we want added to our buildroot-external codebase. Finally, the HSS should not be programmed without the use of SoftConsole as this uses the correct programming tools and device (Embedded Flash Pro 6 over eNVM). This heavy reliance on SoftConsole is not something we want featured in our linux build systems as the build system should be separate to the HSS build in the FPGA flow.
| @@ -11,6 +11,7 @@ source "$BR2_EXTERNAL_MCHP_PATH/package/wilcmchp_firmware/Config.in" | |||
| source "$BR2_EXTERNAL_MCHP_PATH/package/9bit/Config.in" | |||
| source "$BR2_EXTERNAL_MCHP_PATH/package/plplot/Config.in" | |||
| source "$BR2_EXTERNAL_MCHP_PATH/package/mpfs_examples/Config.in" | |||
| source "$BR2_EXTERNAL_MCHP_PATH/package/hart-software-services/Config.in" | |||
There was a problem hiding this comment.
Remove this line (14).
| BR2_PACKAGE_LIBSYSFS=y | ||
| BR2_PACKAGE_JPEG_TURBO=y | ||
| BR2_PACKAGE_LIBSVG_CAIRO=y | ||
| BR2_PACKAGE_DROPBEAR=y |
There was a problem hiding this comment.
Please ensure this, and the entire Buildroot build, is tested and working with Buildroot version 2023.02.
There was a problem hiding this comment.
"BR2_PACKAGE_JPEG_TURBO=y" and "BR2_PACKAGE_LIBSVG_CAIRO=y" is removed, and checked with "2023.08.1" and "2023.02.7" versions, successful.
|
Reminder to please also see the other 15 earlier comments (in case they were missed). There should be a "load more..." link to click under text that says "15 hidden conversations" below the "Program_HSS_to_eNVM.sh" conversation. |
Add support for PolarBerry.