[marvell-armhf] Armada A385 soc support

[antony-rheneus]

Add platform specific SOC kernel configuration based on CONFIGURED_PLATFORM variable passed from sonic-buildimage.

Marvell patches applied to Linux 4.19 are removed,
Kernel driver for SFF, optoe Eeprom are enabled for Armhf architecture.
Due to a platform limitation the (compressed) Linux kernel size can only be 6 MB, so the Linux kernel configuration is shrinked to reduce the size.
Enable MTD parts and command line to access the nand flash

[antony-rheneus]

Duplicate of PR 172 #172,
Opened this to addess the review comments, which I could not proceed in the same PR 172 due to multiple merge conflicts during git rebase.

[lguohan]

@paulmenzel, can you take a look at this pr?

[antony-rheneus]

@paulmenzel , Would you please review the PR?

[paulmenzel]

Sorry, I thought, I replied in #172 to keep discussion there.

[antony-rheneus]

Sorry, I thought, I replied in #172 to keep discussion there.

Marvell armhf requires configuration from "sonic-linux-kernel/linux-4.19.118/arch/arm/configs/mvebu_v7_defconfig" however it is not so when debian prepares the .config.

[carl-nokia]

Paul - what is missing or required from Marvell/Nokia teams to merge this PR? -- we need to get this merged ASAP please

[paulmenzel]

Paul - what is missing or required from Marvell/Nokia teams to merge this PR? -- we need to get this merged ASAP please

Please read the discussion in merge/pull request #172.

[carl-nokia]

Please forgive my lack of clarity on this PR, but I did read PR 172 before asking you for help on this PR 176. I just re-read it again. I want to help resolve whatever is needed ASAP.
The best I can tell is that you are still waiting for the PR description and 23 commit messages to follow "How to Write a Git Commit Message" best practices properly.

Is my understanding correct? ... or are you also looking for a rebase to better define the list of commits for review?

[paulmenzel]

Yes, your understanding is correct.

Then for example, it might become clear, why you have to add certain configs to the inclusion file, and it’s opposite configuration to the exclusion file at the same time (CONFIG_KERNEL_GZIP=y vs. CONFIG_KERNEL_XZ. Only one should be necessary, as it’s a choice.

[paulmenzel]

Having to merge/pull requests is now even more confusing, but so be it. For example 9976144:

[kernel] [marvell-armhf]

Set cpu frequenc to 100HZ as per armada cpu
Enable periodic timer and not dynamic for better performance of a385 soc

I asked several times to follow the guide on how to write commit messages. The summary belongs in the first line for example, and that’s what it at the very beginning and also well seen in the Linux kernel and git project repository.

Why the kernel tag? It’s the sonic-linux-kernel repository. Maybe:

marvell-armhf: armada: Set CPU frequency to 100HZ and use periodic timer

Enable periodic timer and not dynamic for better performance of a385 soc.

[Add explanation here, why 100 HZ increases the performance, how you tested it, and what the performance increase is.]

[antony-rheneus]

@paulmenzel, @lguohan,
We cleaned up all the commit messages to make them more descriptive, following established github guidelines. The "Why" of all the commits were carefully added. Please review and let us know if any additional changes are required.

[paulmenzel]

Thank you, much better!

Some nits:

1. Why is GZIP in the inclusions not enough? Does the script have an error, so that KERNEL_XZ has to be added to the excludes?
2. For the kernel image size reduction commit, can you please document what the old and new size is?
3. not appened → not appended in the last commit message.

[antony-rheneus]

Thank you, much better!

Some nits:

1. Why is GZIP in the inclusions not enough? Does the script have an error, so that KERNEL_XZ has to be added to the excludes?
2. For the kernel image size reduction commit, can you please document what the old and new size is?
3. not appened → not appended in the last commit message.

@paulmenzel
Please review the update commits in the PR, Let us know if any additional changes required.

#1 Since only one compression method can be selected, added config to exclude XZ as it came as part of default config. The conf script didn't report any error.

#2 Documented the file size in the commit description as suggested, please check and confirm if that is fine.
Before size: 6197760
After size: 5192480

#3 Corrected the typo "appened" in commit description.

[lguohan]

@antony-rheneus, it looks like this gz v.s xz issue has been brought up multiple times, and looks like xz is prefered over gz. since armhf maybe used for other sku in the future, can you change to xz instead of gz, so that in the future we won't need to discuss this option again.

[antony-rheneus]

@antony-rheneus, it looks like this gz v.s xz issue has been brought up multiple times, and looks like xz is prefered over gz. since armhf maybe used for other sku in the future, can you change to xz instead of gz, so that in the future we won't need to discuss this option again.

@lguohan, @paulmenzel ,
As requested, I have reverted the GZIP compression, and tested the same in our platform with XZ compression method.
Also I hase rebased to master to resolve the conflicts with patch/series.
Please do review the recent changes.

[lguohan]

lgtm

[paulmenzel]

Sorry, I feel like I am missing something, that I have all these questions, nobody else has during review. Also, I am now spent over one hour with the merge/pull request, which actually tries to do a simple thing. I know you spent more time on it, but I actually do not get paid for this, and only stumbled upon in, while looking at the “project status” of SONiC.

[paulmenzel]

Why is that added here? The config defaults to n, doesn’t it?

[carl-nokia]

Paul - I will set up a meeting with you - this merge is blocking many people - lets get on a call please to answer your questions - we need to wrap this up please. I will send you invite and cc Guohan now.

[paulmenzel]

Paul - I will set up a meeting with you - this merge is blocking many people - lets get on a call please to answer your questions - we need to wrap this up please. I will send you invite and cc Guohan now.

Thanks. It would be nice though to get some written reply to my comments beforehand.

[antony-rheneus]

@paulmenzel , @lguohan ,
I have reverted the configurations to be in sync with the upstreamed version for mvebu_def_config. Let us know if you have any comments.

[antony-rheneus]

Below are the recent commits under review.

$ git log --oneline -n 7
bd6a99e armhf: Update linux debian image file name
873ee9f Marvell-armhf: Disable uefi firmware for marvell
305dc69 Marvell-armhf: Reduce kernel image size for u-boot
b6ee695 Marvell-armhf: Enable optoe and i2c mux
b78363e Marvell-armhf: armada: Set gpio and mtd platform specific configuration
65f6779 Marvell-armhf: Enable platform specific kconfig
a3d548c Marvell-armhf: Remove obsolete patches applied for marvell arm

[radha-danda]

Paul - I will set up a meeting with you - this merge is blocking many people - lets get on a call please to answer your questions - we need to wrap this up please. I will send you invite and cc Guohan now.

Thanks. It would be nice though to get some written reply to my comments beforehand.

@paulmenzel, Antony addressed your comment and pushed the changes. Can you please review and let us know if you have any further comments

[paulmenzel]

Thank you for the last changes. In commit 305dc69 you cite the numbers below:

Testing:

• Check kernel boots without any memory overlapping errors
• kernel binary size before this commit : 6197760 bytes
• kernel binary size after this commit : 5192480 bytes

Is that a hardware limitation? Or a U-Boot issue? Do you have an upstream bug report for that?

So as talked about in the conference, it would be great, if you documented the maximum allowed size, and what compression algorithm was used (if it actually plays a role). The merge/pull request description would be something like (just a made-up suggestion):

Add platform specific SOC kernel configuration.

Marvell patches applied to Linux 4.19 are removed, and due to a platform limitation the (compressed) Linux kernel size can only be 6 MB, so the Linux kernel configuration is shrinked to reduce the size.

[antony-rheneus]

Thank you for the last changes. In commit 305dc69 you cite the numbers below:

Testing:

• Check kernel boots without any memory overlapping errors
• kernel binary size before this commit : 6197760 bytes
• kernel binary size after this commit : 5192480 bytes

Is that a hardware limitation? Or a U-Boot issue? Do you have an upstream bug report for that?

So as talked about in the conference, it would be great, if you documented the maximum allowed size, and what compression algorithm was used (if it actually plays a role). The merge/pull request description would be something like (just a made-up suggestion):

Add platform specific SOC kernel configuration.
Marvell patches applied to Linux 4.19 are removed, and due to a platform limitation the (compressed) Linux kernel size can only be 6 MB, so the Linux kernel configuration is shrinked to reduce the size.

Size constraint is hardware specific limitation for u-boot based platforms.

[antony-rheneus]

@paulmenzel , I have updated the comment description, and also the PR description. Would you please review?

[paulmenzel]

Size constraint is hardware specific limitation for u-boot based platforms.

Do you have a reference for that?

[paulmenzel]

Nice, looks good.

Just to be sure: I made up the number in “Linux kernel size can only be 6 MB,”. That is correct.

Also, just to be sure:

Testing:

• Check kernel boots without any memory overlapping errors
• kernel binary size before this commit : 6197760 bytes
• kernel binary size after this commit : 3600624 bytes

The 6.2 MB is really XZ compressed image or was it with GZIP? Because if you try XZ which results in smaller size, maybe you can use the default kernel configuration.

[paulmenzel]

Just to be clear, although I am interested in the answers of the questions above, I do not want to hold anything up. So, if @lguohan wants to accept this, please go ahead.

[lguohan]

@paulmenzel , thanks for the reviewing. the pr is good. @antony-rheneus , please try to answer the questions, I am also curious to learn.

[paulmenzel]

@antony-rheneus, it’d be great if you replied to my questions.

[antony-rheneus]

@antony-rheneus, it’d be great if you replied to my questions.

@paulmenzel , sure, I will test the kernel with the changes and reply back.

[antony-rheneus]

@antony-rheneus, it’d be great if you replied to my questions.

@paulmenzel , sure, I will test the kernel with the changes and reply back.

@paulmenzel ,
After removing all the SOCs define in "exclusion" file, now the image is reduced to 4.2MB with xz compression

ls -l linux-4.19.118/debian/build/build_armhf_none_armmp/arch/arm/boot/zImage
-rwxr-xr-x 1 root root 4260352 Dec 21 23:10 linux-4.19.118/debian/build/build_armhf_none_armmp/arch/arm/boot/zImage

However, the kernel hangs, so there is some conflicting CONFIG variable when all SOCs are defined. I will require some more time to debug this to narrow down the invalid configuration.

Starting kernel ...

<<<HUNG>>>

[paulmenzel]

Over three months have passed. Were you able to fix the issue?

[antony-rheneus]

Over three months have passed. Were you able to fix the issue?

Not yet, we will get back.

[paulmenzel]

@carl-nokia, could you (or the responsible project manager) please allocate time for @antony-rheneus or some other developer/engineer to work on this?

[paulmenzel]

Over a year has passed, and the promised work, convincing the reviewers to merge the changes, has not happened yet.

[antony-rheneus]

@paulmenzel,
Redirecting the request to the Project Manager (@radha-danda)

[radha-danda]

@paulmenzel, If I recollect the issue,

However, the kernel hangs, so there is some conflicting CONFIG variable when all SOCs are defined. I will require some more time to debug this to narrow down the invalid configuration.

This was the question remained unanswered. Please help me recollect the issue.

And to add more details, if above was the issue that left unanswered, we had similar issue with Kernel hang due to image size
and here is the rootcause sonic-net/sonic-buildimage#10204 and we are planning to backport the PR to 202012 branch.