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

BigTreeTech CB1 upstreaming #6656

Merged
merged 8 commits into from
Aug 11, 2024
Merged

Conversation

JohnTheCoolingFan
Copy link
Contributor

@JohnTheCoolingFan JohnTheCoolingFan commented May 23, 2024

Description

Improved support for BigTreeTech Pi and CB1

Jira reference number AR-2312

How Has This Been Tested?

Build and test on real hardware

  • HDMI
  • Ethernet
  • WiFi
  • USB type c UART

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@github-actions github-actions bot added size/large PR with 250 lines or more Hardware Hardware related like kernel, U-Boot, ... Framework Framework components labels May 23, 2024
@github-actions github-actions bot added size/small PR with less then 50 lines and removed size/large PR with 250 lines or more labels May 23, 2024
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines and removed size/small PR with less then 50 lines labels Jun 13, 2024
@JohnTheCoolingFan JohnTheCoolingFan changed the base branch from btt-cb1-AR-2312 to main July 1, 2024 21:09
@github-actions github-actions bot added the Patches Patches related to kernel, U-Boot, ... label Jul 1, 2024
@igorpecovnik igorpecovnik added the Work in progress Unfinished / work in progress label Jul 3, 2024
@JohnTheCoolingFan
Copy link
Contributor Author

Current problem with ethernet is that the sunxi-gmac driver can't find PHY, that's what gets reported in logs.

@JohnTheCoolingFan
Copy link
Contributor Author

JohnTheCoolingFan commented Jul 26, 2024

To bump and provide more info, here's the error message that gets reported:

sunxi-gmac 5030000.ethernet end0: No PHY found!
sunxi-gmac 5030000.ethernet end0: phy init again...
sunxi-gmac 5030000.ethernet end0: No PHY found!
sunxi-gmac 5030000.ethernet end0: phy init again...

Typed out snippet of the kernel log. @armbian/boards-allwinner any ideas on why the driver unable to find PHY?

@ivan95603
Copy link

Hi @JohnTheCoolingFan, Do you know around when this will be merged because apt update on
image
breaks wifi also. On the clean image doing apt update kills the wifi and the eth wasn't working anyway.
Thank you in advance.

@JohnTheCoolingFan
Copy link
Contributor Author

@ivan95603 Unfortunately, this depends on when I can figure out what's wrong with ethernet in this branch. That should be the only major peripheral left before I can call this a proper update.

@ivan95603
Copy link

@JohnTheCoolingFan Ok. Thank you. Update regarding wifi it doesn't need system update to stop working just the reboot of the board and it will stop working.

@jernejsk
Copy link
Contributor

jernejsk commented Aug 7, 2024

@JohnTheCoolingFan If I get schematic right, internal PHY is used, which provides 100 Mbps speed, right? If so, this actually needs AC200 EPHY patches from H6 and separate PWM driver in order to properly set up PHY. However, your version could work too, if you rely on U-Boot to initialize PHY (I use such hack on my development systems).

@JohnTheCoolingFan
Copy link
Contributor Author

JohnTheCoolingFan commented Aug 7, 2024

@jernejsk Well, this device tree is based off of the one BTT uses in their kernel (which is 4.19 I think), and they use the sunxi-gmac driver, which is what I'm trying to use here. For AC200, could you point me to what boards are similar in that regard so that I can use them as an example? And putting the init in u-boot would also be great, although I would like to first get it working in kernel so that it doesn't rely on u-boot that much.

@jernejsk
Copy link
Contributor

jernejsk commented Aug 7, 2024

No need to port BSP driver. A few months ago I made initial EPHY driver for H616 here: https://github.com/jernejsk/linux-1/commits/ac300-net/ Different board but ethernet part should be the same and it's independent from U-Boot. Note that this driver could be unified with H6 and replace old AC200 H6 patches. It also fixes one major downside that H6 AC200 driver had - driver load order was important (built in drivers vs. modules). Above branch avoids that issue.

igorpecovnik added a commit that referenced this pull request Aug 11, 2024
Need to be fixed after release. Some background in comments #6656
Changed maintainer to th current person
Switched board family to sun50iw9, removed legacy kernel target, removed
DEFAULT_CONSOLE
JohnTheCoolingFan pushed a commit to JohnTheCoolingFan/armbian-build that referenced this pull request Aug 11, 2024
Need to be fixed after release. Some background in comments armbian#6656
@JohnTheCoolingFan
Copy link
Contributor Author

Cherry-picked the commit from #7059

@The-going
Copy link
Contributor

Good health to all participants of this pull request.

I have a very big request.
Please pay attention to the design of the patches.
The string with the "Subject:" field must match the patch name.
What follows is a description of what was done and why exactly in this way.

Many patches with small changes have a strategic advantage over one large patch.

Please understand me correctly.
I assume that you have done a good job, but tomorrow someone else will try to improve
it or apply it to a larger version of the kernel.
I'm just moving these patches and trying to apply them to the new kernel version.
I have no way to check the correctness of the work on the platform, only the compilation process.

When I see only one line like: Enable Ethernet on SOC, and then there are changes in ten files
and changes cannot be applied to two of them, in this case it is very difficult to make the right decision.
There is no description of why the changes to these files were made in this way.

With respect

@The-going
Copy link
Contributor

In my ac300-net branch, I have PWM driver which will probably work for other cases too, if you're interested.

The changes that work are always interesting.
But, why exactly is this so?
pwm: sun20i: modify for h616 support
Do these changes mean that the driver no longer works for d1?
The compatible line has been changed.

This is not a question about the code.
This is a question about the design of the message in the header of the patch (commit).

@jernejsk
Copy link
Contributor

Do these changes mean that the driver no longer works for d1?

Yes, because I wasn't interested in D1, but H616. I believe apritzel has some good ideas how to do this properly, but I forgot where they are. Maybe his GH, maybe linux-sunxi ML...

@JohnTheCoolingFan
Copy link
Contributor Author

I have a very big request.
Please pay attention to the design of the patches.
The string with the "Subject:" field must match the patch name.
What follows is a description of what was done and why exactly in this way.

thanks for pointing this out, apologies for improper formatting, I'll fix this now.

@JohnTheCoolingFan
Copy link
Contributor Author

Alright, I've split the ethernet patch into 3: h616 device tree (addign emac1), i2c3 and PHY setup.

@The-going
Copy link
Contributor

Alright, I've split the ethernet patch into 3: h616 device tree (addign emac1), i2c3 and PHY setup.

The u-boot patches for this particular board remain out of my focus. I don't have this device.
But one remark.
It seems your editor is configured so that it replaces the tab character with 4 spaces.

@JohnTheCoolingFan
Copy link
Contributor Author

Well, then I am a bit confused about what patches you meant that needed splitting. The two kernel patches simply modify the device tree for btt cb1, one enabled hdmi and the other emac1. But I did just notice that the subject field is messed up so I will fix that as well.
As for the editor, yes, it is configured to use 4 spaces as tabs and treat them as such, and it also converts when pasting content in, which is overriden by file format config where needed. Is there something wrong with the patches caused by this?

@The-going
Copy link
Contributor

Well, then I am a bit confused about what patches you meant that needed splitting.

I apologize if my grumbling causes misunderstanding.
Please don't pay attention.

As for the editor, yes, it is configured to use 4 spaces as tabs

The style of the file that has been edited implies that the tab character is a tab character, and the space character is a space character.

@JohnTheCoolingFan
Copy link
Contributor Author

The style of the file that has been edited implies that the tab character is a tab character, and the space character is a space character.

Oh, thanks for pointing this out! This made me look into plugins for auto-detecting indent style when possible. Fixed the indentation in patches, squashed it into the commit that originally added them, should be fine now.

@igorpecovnik igorpecovnik self-requested a review August 11, 2024 20:05
Copy link
Member

@igorpecovnik igorpecovnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's roll this in. The rest in new PRs.

@igorpecovnik igorpecovnik merged commit f8dd32f into armbian:main Aug 11, 2024
7 checks passed
@JohnTheCoolingFan
Copy link
Contributor Author

Well, good to see this finally merged, but a lot of stuff ahead that still needs to be done.

@The-going
Copy link
Contributor

should be fine now.

Unfortunately, I didn't explain very well about the patch file name.
Explanations:
Patches in the series are extracted using the mk_format_patch script from the linux kernel git repository.
That is, git format-patch ... command.
This command creates a patch file name from the string "Subject:"

When we promote a series of patches to a new kernel version,
the patches are first applied to the git kernel repository using the "git am" command
patch by patch by reading the series.conf file.
As a result, we get this: sunxi-6.6-rebase

The name of your patch: arm64-dts-sun50i-h616-bigtreetech-cb1-enable-emac1.patch
but
Subject: ARM64: dts: sun50i-h616: BigTreeTech CB1: Enable EMAC1

This file will be renamed automatically the next time you extract it.

With respect

Dangku pushed a commit to Dangku/armbian-build that referenced this pull request Sep 13, 2024
Need to be fixed after release. Some background in comments armbian/build#6656
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
08 Milestone: Third quarter release Framework Framework components Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/large PR with 250 lines or more
Development

Successfully merging this pull request may close these issues.

6 participants