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

allwinner: Added support for allwinner H616 processor #58

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

didiercrunch
Copy link

Added support for the allwinner H616 processor. Apparently, this also works for H618 processor.

Helps with #57 but doesn't not completely fix it.

Copy link
Member

@maruel maruel left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution!

I see a few failures unrelated to your change on the CI, I'll take a look to try to fix these. The PR will fail for the license header check at least, so you'll want to fix that.
A few changes requested, nothing significant.

allwinner/address_test.go Outdated Show resolved Hide resolved
allwinner/address_test.go Outdated Show resolved Hide resolved
allwinner/address_test.go Outdated Show resolved Hide resolved
allwinner/address_test.go Outdated Show resolved Hide resolved
allwinner/h6.go Outdated Show resolved Hide resolved
allwinner/h6.go Outdated Show resolved Hide resolved
allwinner/h6.go Show resolved Hide resolved
@maruel
Copy link
Member

maruel commented Aug 9, 2024

I fixed the checks on origin/main, can you rebase and push again? Thanks!

@didiercrunch
Copy link
Author

I fixed the checks on origin/main, can you rebase and push again? Thanks!

done

Copy link
Member

@maruel maruel left a comment

Choose a reason for hiding this comment

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

Did you figure out why the checks still fail? Do you want help with diagnosing the problem?

allwinner/h6_test.go Show resolved Hide resolved
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 36.26374% with 58 lines in your changes missing coverage. Please review.

Project coverage is 28.2%. Comparing base (50d4ed0) to head (4abefc9).

Files with missing lines Patch % Lines
allwinner/address.go 54.7% 14 Missing and 10 partials ⚠️
allwinner/h6.go 17.4% 19 Missing ⚠️
allwinner/gpio.go 0.0% 9 Missing ⚠️
allwinner/detect.go 0.0% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main     #58     +/-   ##
=======================================
+ Coverage   27.4%   28.2%   +0.8%     
=======================================
  Files         89      91      +2     
  Lines      11011    8915   -2096     
=======================================
- Hits        3014    2511    -503     
+ Misses      7864    6263   -1601     
- Partials     133     141      +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@maruel maruel left a comment

Choose a reason for hiding this comment

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

Sounds good, thanks again!

There's a test failure to fix on Windows and then it's good to go.

@peergum
Copy link

peergum commented Sep 9, 2024

@didiercrunch Thanks for this. I was working on creating an orange pi variation for H616, and I just found your PR.

I'm working on an Orange Pi Zero 2W, and your work may help me (at least to figure what else I should fix)

@irootyou
Copy link

I am trying to get allwinner h64 h6 model b; should I use this as the basis for the addition?

@didiercrunch
Copy link
Author

@irootyou probably. where is the documentation for this cpu?

@irootyou
Copy link

irootyou commented Nov 17, 2024

@irootyou probably. where is the documentation for this cpu?

https://files.pine64.org/doc/datasheet/pine-h64/Allwinner_H6%20V200_User_Manual_V1.1.pdf

I don't mind taking the JSON file, and implementing it in the exact format that the H5 and the other allwinner chips are implemented. Keeping everything completely consistent and ensuring it all builds.

One issue I was having is that I have the dts file in my boot section, and I can obviously just directly use the host/periph library or even a more basic interaction with the gpio system; but when I do gpiodetect and gpioinfo:

All of the lines show unnamed. I thought that had to do with the dts file that is definitely present and populated with information. It and the manual were my plan on how to build out the H6 functionality.

I'd love to get experience doing this. So I would appreciate the opportunity to contribute in this way.

Anyone have any idea on the unnamed lines of the gpiochip when trying to do gpioinfo. I know I don't have to solve it, its more I want to solve it so I understand everything better and how it fits together.

Extra Resources

https://files.pine64.org/doc/Pine%20H64/Pine%20H64%20model%20B%20Port%20Assignment%20rev2.0.pdf

https://pine64.org/devices/pine_h64_model_b/

@maruel
Copy link
Member

maruel commented Nov 17, 2024

@gsexton FYI, he implemented the gpiochip support.

@irootyou
Copy link

Yes, of course, but its not quite implemented in a way that is consistent with the rest of the implemented chips.

The alternative is sysfs which would already work. The biggest thing is to ensure all the specific details for the chips are provided in easy to use sub-libraries that build, test, and are completely consistent with existing design patterns.

I have 2 other similar chips, I will guarantee they are all supported while I take a pass at this

@gsexton
Copy link
Contributor

gsexton commented Nov 26, 2024

I tested the host/gpioioctl package using an OrangePi Zero 2. I used "gpio readall" to find the GPIO number. I then called:

pin:=gpioioctl.Chips[0].ByNumber(73) // wiring pi 2
mode:=gpio.Low
for range 10 {
  mode=!mode
  pin.Out(mode)
  time.Sleep(time.Second)
}

and the library worked as expected. The LED blinked :)

I also tested WaitForEdge by wiring GPIO73 to GPIO226 and it worked as expected.

Examining the chips by using fmt.Println(gpioioctl.Chips[0]) showed that none of the lines had names assigned. Since I'm using the ioctl() to get the chip/line info, this is a kernel problem, and not a library problem.

I'm running the Debian distribution with kernel 6.1.31.

@maruel I see that the allwinner/gpio.go is using sysfs. allwinner should probably be updated in the same manner that the brcm283x package was. It would probably be best if someone who was more familiar with the architecture than me did it.

@gsexton
Copy link
Contributor

gsexton commented Nov 26, 2024

Yes, of course, but it’s not quite implemented in a way that is consistent with the rest of the implemented chips.

the implementation models the kernel interface. A set of chips that have lines. It is compatible with the sysfs implementation.. I replaced the bcm283x use of sysfs in a couple of hours. The pr for that is here.

#63

The alternative is sysfs which would already work. The biggest thing is to ensure all the specific details for the chips are provided in easy to use sub-libraries that build, test, and are completely consistent with existing design patterns.

I have 2 other similar chips, I will guarantee they are all supported while I take a pass at this

WaitForEdge does not work with Kernel 6.x with sysfs. That was the motivation for gpioioctl. I’ve tested with an H616 and the gpioioctl package works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants