Add Raspberry Pi 5 - Githubissues

floion commented 9 months ago

- What I did

- How I did it

agherzan commented 8 months ago

@floion I think this looks alright to me now. Shall we have one last squash? The only thing I would keep is the config hack commit. So it will bother us enough to actually fix it in the future :)

floion commented 8 months ago

@agherzan which commits do you want squashed? Let's take into account that @leon-anavi might want to have his commits highlighted so let me know how you want to proceed.

floion commented 7 months ago

@agherzan @kraj please advise how you want to have the commits arranged. Which do you want squashed, how to keep the Signed-off-by fields and so on. CC @leon-anavi

leon-anavi commented 7 months ago

@agherzan @kraj please advise how you want to have the commits arranged. Which do you want squashed, how to keep the Signed-off-by fields and so on. CC @leon-anavi

@floion thank you for following up on this pull request. I don't have any remarks or objections at the moment. It is up to the maintainers to decide how to proceed.

agherzan commented 7 months ago

@floion, here is what I propose - squash everything (excluding the commits from Leon) in the "Add Raspberry Pi 5" commit and make sure this is the first in the branch (rebased before Leon's commits). Does that sound reasonable to you?

olivierschonken commented 7 months ago

Another kernel version bump (I bumped it to 6.1.64 locally) is probably required to pick up this fix - https://github.com/raspberrypi/linux/commit/526b986030c2ef61e2dd61641838290810da61f9 - Without it, opkg complains about Error: Package name contains illegal characters, (other than [a-z0-9.+-]) when building kernel modules. I can get it to build and boot for the Pi5 when also removing the U-boot changes of this PR. The u-boot changes breaks the build currently. ERROR: Nothing PROVIDES 'u-boot' u-boot was skipped: incompatible with machine raspberrypi5

leon-anavi commented 7 months ago

ERROR: Nothing PROVIDES 'u-boot' u-boot was skipped: incompatible with machine raspberrypi5

@olivierschonken, this is expected because U-Boot still doesn't support Raspberry Pi 5. Even if you build an image with U-Boot as of the moment it will not boot. Therefore to avoid this error remove RPI_USE_U_BOOT = "1" from your build environment.

floion commented 7 months ago

Done @agherzan Let me know if there's anything else

agherzan commented 7 months ago

@floion, the two commits at the end fail DCO. Can you either add your SoB or change the author to Leon?

floion commented 7 months ago

Done @agherzan , thanks.

gportay commented 7 months ago

Hello @floion, IMO, I guess you should really squash commit 2, 3, 5, 6 and 7 to 1, because it is like the first commit is deliberately adding bugs that you are fixing afterward.

What about squashing them and adding the SoB to the first commit? Would you agree @leon-anavi?

However, very thanks for your work, I will use it soon.

floion commented 7 months ago

@gportay I am ok with adding @leon-anavi to SoB and squashing the commits if he agrees.

leon-anavi commented 7 months ago

What about squashing them and adding the SoB to the first commit? Would you agree @leon-anavi?

My only concern is that my related efforts in this particular pull request are sponsored by GOVCERT.LU. Therefore the company I work for Konsulko Group needs to be able show the commits with appropriate note in them about this. I am very happy and grateful that there are companies supporting open source upstream work and they deserve a credit.

floion commented 7 months ago

Ok, then let's leave it as it is now @leon-anavi

leon-anavi commented 7 months ago

@gportay I don't like the idea about RPI_CONFIG_STRIP as developers can easily "shoot themselves in the foot" with it and build an image that doesn't boot. As @agherzan mentioned previously probably in long term config.txt should be generated in entirely different way instead of using as a template https://github.com/Evilpaul/RPi-config

gportay commented 7 months ago

@gportay I don't like the idea about RPI_CONFIG_STRIP as developers can easily "shoot themselves in the foot" with it and build an image that doesn't boot. As @agherzan mentioned previously probably in long term config.txt should be generated in entirely different way instead of using as a template https://github.com/Evilpaul/RPi-config

Well, the RPI_CONFIG_SCRIPT gives the control to the user to strip or not its config.txt. I really dislike the idea of automatically stripping the file because of pi 5 has more limitation about the file size than the other hardware. The user must be free to do whatever he likes and he may have better reasons that upstream to do it: this is why I suggest to control the stripping with that variable; automatically set if raspberrypi5.

Stripping the file will not save your foot if the config.txt is larger than 16k in the end. I think I have suggested to make the "real runtime" test instead: checking for the file size and dies it it reaches more that 16k.

Hope it will convince you.

leon-anavi commented 7 months ago

@gportay I don't like the idea about RPI_CONFIG_STRIP as developers can easily "shoot themselves in the foot" with it and build an image that doesn't boot. As @agherzan mentioned previously probably in long term config.txt should be generated in entirely different way instead of using as a template https://github.com/Evilpaul/RPi-config

Well, the RPI_CONFIG_SCRIPT gives the control to the user to strip or not its config.txt. I really dislike the idea of automatically stripping the file because of pi 5 has more limitation about the file size than the other hardware.

The automatic stripping of the comments is a temporary work around.

The user must be free to do whatever he likes and he may have better reasons that upstream to do it: this is why I suggest to control the stripping with that variable; automatically set if raspberrypi5.

Advanced users can always make a bbappend file for recipe rpi-config and modify config.txt in it.

Stripping the file will not save your foot if the config.txt is larger than 16k in the end. I think I have suggested to make the "real runtime" test instead: checking for the file size and dies it it reaches more that 16k.

Hope it will convince you.

Sorry, no, I still think in long term rpi-config should be completely changed to generate config.txt on the fly instead of using the template from https://github.com/Evilpaul/RPi-config. This RPi-config repo is not even actively maintained right now. The last commit was made 2 years ago. My GitHub pull request with a small fix has been waiting for a review for 3 weeks.

gportay commented 7 months ago

And to be honest, the approach a taking an unofficial config.txt do not make me happy at all. I prefer meta-raspberrypi to ship its own minimalist file instead, with a comment linking to the official documentation.

The content in config.txt needs to be updated at every version of the firwmare and kernel, and it is hard to maintain to be honest (I mean documenting he whole properties).

Instead, users should bbappend the recipe if he wants to ship its own custom file, and benefits from the features in the recipe such as the warning if file is too big or the stripping...

So I guess yes, I agree for a refactor at long term. Here is my 2-cents.

leon-anavi commented 7 months ago

So I guess yes, I agree for a refactor at long term. Here is my 2-cents.

It looks like we are all on the same page for long term plans. However, the refactoring of rpi-config recipe and config.txt is another topic because it affects all Raspberry Pi models and versions. Probably it deserves a separate issue and pull requests. Particularly, for this pull request I propose to focus and finalize the efforts started by @floion for adding Raspberry Pi 5 initial support in meta-raspberrypi.

gportay commented 7 months ago

@leon-anavi, I think we agrees on things.

The automatic stripping of the comments is a temporary work around.

Agree. Also you discovered that file must be smaller than 16k...

Advanced users can always make a bbappend file for recipe rpi-config and modify config.txt in it.

... and since the Advanced users can provide their own file so...

Stripping the file will not save your foot if the config.txt is larger than 16k in the end. I think I have suggested to make the "real runtime" test instead: checking for the file size and dies it it reaches more that 16k. Hope it will convince you.

... their file can be larger than 16k, and the strip DOES NOT guarantee the final file got reduced to fit in the 16k (to boot a pi 5), right? There are plenty of reasons for that: such as generating it on the fly with a bad algorithm (but not necessarily bad anyway).

So I am saying that we should implement a check for the file size to rise an error if it is larger than 16k for pi 5 that addresses the issue you faced. And make the strip optional to not modify the actual behavior for pi 1-4.

I still think in long term rpi-config should be completely changed to generate config.txt on the fly instead of using the template from https://github.com/Evilpaul/RPi-config. This RPi-config repo is not even actively maintained right now. The last commit was made 2 years ago. My GitHub pull request with a small fix has been waiting for a review for 3 weeks.

Agree.

PS: sorry, for delays.

leon-anavi commented 7 months ago

... their file can be larger than 16k, and the strip DOES NOT guarantee the final file got reduced to fit in the 16k (to boot a pi 5), right? There are plenty of reasons for that: such as generating it on the fly with a bad algorithm (but not necessarily bad anyway).

So I am saying that we should implement a check for the file size to rise an error if it is larger than 16k for pi 5 that addresses the issue you faced. And make the strip optional to not modify the actual behavior for pi 1-4.

We have already discussed config.txt in previous comments. Almost a month ago @floion opened a thread in the Raspberry Pi forum. There are different limits for different Raspberry Pi models and versions. The recommendation of jamesh (Raspberry Pi Engineer & Forum) is: "Just get rid of the unnecessary comments". This is exactly what the work around implements. I don't think we should over-engineer it further.

However, this is not a Raspberry Pi 5 specific problem. We have a problem in general with config.txt and it looks like there is an agreement on the vision how to improve/fix it in future. Although I am not a maintainer, I propose to keep this pull request for Raspberry Pi 5 only and other fixes and features like config.txt to be further discussed in separate issues and pull requests.

gportay commented 7 months ago

This is exactly what the work around implements. I don't think we should over-engineer it further.

Well this is the point I disagree. Your workaround could be nicely updated to guardrail instead to prevent (as you said) the developers to "shoot themselves in the foot" by rising the error if the config.txt is too big, as you noticed.

Considering the over-engineer part, I guess everyone has his point of view, and to give you mine, the following code is (far) from what I call over-engineer.

size=$(stat -c "%s" "$CONFIG")
if [ "$size" -ge 4096 ]; then
  bbfatal "$CONFIG is to big please to get read by the raspberrypi firmware. Please consider strip it down to feat in 4k; You can enable RPI_CONFIG_SRTIP =  \"1\" to drop off the comments."
fi

Note: the config.txt could easily be big since it could be generic to every hardware with specific features using the model filters ([pi4], [pi400], [cm4], [cm4s]... for same family of SoC).

I won't argue anymore if I have no chance to convince you. Anyway, it is up to the maintainers to decide.

agherzan commented 7 months ago

@gportay @leon-anavi I am not against a check like that. The only issue is that we must both be sure of that limitation (in terms of the size) and maintain it as and if it changes. I'm concerned about our ability to do so as I am not aware of a way to get that information from the foundation consistently (and even the forum has opinions - not documentation). I get the annoyance now that rpi4, for example, which is not affected here, will end up out of nowhere with comments removed because rpi5 cannot cope with the same config file. As a middle ground, we could strip down the config only for rpi5. It's ugly, and I don't really like any solution, but I feel bad for blocking this, as people like @floion need this over the fence. And extra variable is equally dirty and temporary. @kraj , any thoughts?

kraj commented 7 months ago

@gportay @leon-anavi I am not against a check like that. The only issue is that we must both be sure of that limitation (in terms of the size) and maintain it as and if it changes. I'm concerned about our ability to do so as I am not aware of a way to get that information from the foundation consistently (and even the forum has opinions - not documentation). I get the annoyance now that rpi4, for example, which is not affected here, will end up out of nowhere with comments removed because rpi5 cannot cope with the same config file. As a middle ground, we could strip down the config only for rpi5. It's ugly, and I don't really like any solution, but I feel bad for blocking this, as people like @floion need this over the fence. And extra variable is equally dirty and temporary. @kraj , any thoughts?

lets add a check for size and add a disclaimer to check with latest state on pi forums if something changes. These can be very effective way to catch errors which otherwise takes days to diagnose otherwise.

gportay commented 7 months ago

lets add a check for size and add a disclaimer to check with latest state on pi forums if something changes. These can be very effective way to catch errors which otherwise takes days to diagnose otherwise.

Let me make a PR by today.

EDIT; #1255, @agherzan, @kraj, @leon-anavi, please yell if you disagree :)

gportay commented 7 months ago

@floion, LTGM despites my last comments.

Also, you may consider rewording the commit title of your very first commit to match the implicit standard given by the git history :)

leon-anavi commented 6 months ago

@floion @agherzan based on my experience KERNEL_IMAGETYPE_DIRECT must be set to "Image" to successfully build and boot the kernel. I made a fix and submitted it as a GitHub pull request to @floion's fork: https://github.com/floion/meta-raspberrypi/pull/4

Best regards, Leon

agherzan commented 6 months ago

@floion this failed CI. @leon-anavi has a fix proposed to your fork.

cordlandwehr commented 6 months ago

Hi, I did a test of this branch (including the latest changes from https://github.com/floion/meta-raspberrypi/pull/4), but I do get a fully reproducible bootup hang at

platform axi:gpu: bcm2712_iommu_attach_dev: MMU 1000005200.iommu
bcm2712_-iommu 1000005280.iommu: bcm2712_iommu_probe: Success
bcm2712_-iommu 1000005280.iommu: bcm2712_iommu_ini: DEBUG_INFO = 0x20804774
bcm2712_-iommu 1000005280.iommu: bcm2712_iommu_probe: Success
vc4-drm axi:gpu: bcm2712_iommu_of_xlate: MMU 1000005200.iommu

after that no further boot output. Has anybody an idea what might go wrong? I am on nanbield with meta-raspberrypi put to this patchset.

kraj commented 6 months ago

Hi, I did a test of this branch (including the latest changes from floion#4), but I do get a fully reproducible bootup hang at
platform axi:gpu: bcm2712_iommu_attach_dev: MMU 1000005200.iommu
bcm2712_-iommu 1000005280.iommu: bcm2712_iommu_probe: Success
bcm2712_-iommu 1000005280.iommu: bcm2712_iommu_ini: DEBUG_INFO = 0x20804774
bcm2712_-iommu 1000005280.iommu: bcm2712_iommu_probe: Success
vc4-drm axi:gpu: bcm2712_iommu_of_xlate: MMU 1000005200.iommu
after that no further boot output. Has anybody an idea what might go wrong? I am on nanbield with meta-raspberrypi put to this patchset.

Try all master for best results

cordlandwehr commented 6 months ago

Thanks for the hint! Together with poky master I got core-image-weston running.

mcbullisisu commented 6 months ago

Thanks for the hint! Together with poky master I got core-image-weston running.

I can also confirm that I was able to boot an image derived from core-image-base. UART & HDMI output seem to work fine. I didn't test any further since my sample camera app is built on top of legacy python3-picamera, which needs some COMPATIBLE_HOST and/or dependency chain updates for the Pi 5.

Commits used:

meta-raspberrypi:leon-anavi/add_rpi_fix_kernel_image - https://github.com/leon-anavi/meta-raspberrypi-1/commit/db4bb8d14b458f98823437e4dea26c3377f8c61d
poky:master - https://github.com/yoctoproject/poky/commit/7af374c90c349af2c7ae9cf7d4fa14eeeb23b108
meta-openembedded:master - https://github.com/openembedded/meta-openembedded/commit/0f90c7dc981263de7b2ab9e9719f462c1c48c990

leon-anavi commented 6 months ago

@agherzan I think its ready for merge.

@kraj I think we still need this patch for KERNEL_IMAGETYPE_DIRECT that I have been trying to merge in Florin's repo for this GitHub pull request: https://github.com/floion/meta-raspberrypi/pull/4/commits/db4bb8d14b458f98823437e4dea26c3377f8c61d https://github.com/floion/meta-raspberrypi/pull/4

kraj commented 6 months ago

@agherzan I think its ready for merge.

@kraj I think we still need this patch for KERNEL_IMAGETYPE_DIRECT that I have been trying to merge in Florin's repo for this GitHub pull request: floion@db4bb8d floion#4

I would suggest that we retarget that as an incremental patch directly as a fresh pull after the merge.

agherzan commented 6 months ago

@kraj CI won't pass without, and it is going to complain in all the PR later. @floion do you have a timeline for including @leon-anavi 's patch?

kraj commented 6 months ago

@kraj CI won't pass without, and it is going to complain in all the PR later. @floion do you have a timeline for including @leon-anavi 's patch?

if we can do a new combined PR ? where this PR and added patches are put together ? @leon-anavi can you help putting that together ? Sadly, I dont have a rpi5 to do it on my own

leon-anavi commented 6 months ago

@kraj CI won't pass without, and it is going to complain in all the PR later. @floion do you have a timeline for including @leon-anavi 's patch?

if we can do a new combined PR ? where this PR and added patches are put together ? @leon-anavi can you help putting that together ? Sadly, I dont have a rpi5 to do it on my own

Ok, yes, I will give it a try tomorrow and provide a combined PR.

floion commented 6 months ago

@kraj @agherzan @leon-anavi I added the requested commit to this PR

leon-anavi commented 6 months ago

@kraj @agherzan @leon-anavi I added the requested commit to this PR

Thank you. Today I updated and I tested again this PR on Raspberry Pi 5. I was able to successfully build core-image-minimal and boot it. It looks good and I have no remarks.

agherzan commented 6 months ago

Looks good. I've triggered CI, and it will merge if it succeeds.

agherzan / meta-raspberrypi

Add Raspberry Pi 5 #1237