Add new board in future

[Evaneee]

Description

Will suport for z96a infuture...

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

• Test A: Image which compiled by new code has been flashed in z96a, and everything works fine.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules

[amazingfate]

It's better to add kernel patches to https://github.com/armbian/linux-rockchip.

[paolosabatino]

I am forcefully against the new family. We are in the situation where the whole rockchip brand has too many families are we are actively trying to reduce them. Adding another family would go in the opposite direction. I will take a look in detail for suggestions later

[Evaneee]

It's better to add kernel patches to https://github.com/armbian/linux-rockchip.

These drivers are OK for nomal use. z96a use some chips in a special way, So only z96a need this special driver.

[Evaneee]

. Adding another family would go in the opposite directi

But is there a way -- Patch kernel and do not Influence the other boards in the family (the other board do not need the patch).

[amazingfate]

It's better to add kernel patches to https://github.com/armbian/linux-rockchip.

These drivers are OK for nomal use. z96a use some chips in a special way, So only z96a need this special driver.

After quick look at the kernel patch, it's about rk817,battery driver. After searching it in the current kernel tree no other boards is using it. Only some rockchip's evb boards which armbian doesn't maintain use it.

So it's okay to add this patch to the current rk35xxx family. If in the future someone has a new board whose driver conflicts with this patch, we can try to split it in board devicetree level.

I didn't notice this pr is trying to add a new family, which is a bad idea. Although this is a vendor legacy kernel, we are always trying to make an universal kernel.

[Evaneee]

I am forcefully against the new family. We are in the situation where the whole rockchip brand has too many families are we are actively trying to reduce them. Adding another family would go in the opposite direction. I will take a look in detail for suggestions later

It's better to add kernel patches to https://github.com/armbian/linux-rockchip.

These drivers are OK for nomal use. z96a use some chips in a special way, So only z96a need this special driver.

After quick look at the kernel patch, it's about rk817,battery driver. After searching it in the current kernel tree no other boards is using it. Only some rockchip's evb boards which armbian doesn't maintain use it.

So it's okay to add this patch to the current rk35xxx family. If in the future someone has a new board whose driver conflicts with this patch, we can try to split it in board devicetree level.

I didn't notice this pr is trying to add a new family, which is a bad idea. Although this is a vendor legacy kernel, we are always trying to make an universal kernel.

We will start compatibility with current and edge versions soon. What should we do by then?

[amazingfate]

Current and edge are mainline kernel. For rk3568 you can add patches to dir like patch/kernel/archive/rockchip64-6.6/ or patch/kernel/archive/rockchip64-6.7/ as defined in config/sources/families/include/rockchip64_common.inc.

[Evaneee]

Current and edge are mainline kernel. For rk3568 you can add patches to dir like patch/kernel/archive/rockchip64-6.6/ or patch/kernel/archive/rockchip64-6.7/ as defined in config/sources/families/include/rockchip64_common.inc.

We need a way to do Individual kernel patch and kernel config for Individual board，but can not find clue in config/sources/families/include/rockchip64_common.inc so far. For test I compile Image for pinebook-pro which belongs to rockchip64 family, and found info: Summary: kernel patching: 118 total patches; 118 applied; 31 with problems; 31 needs_rebase .So many patches be imposed... It is difficult to determine which patche are useful even harmful. Is there a way include or exclude some of the patches？

[amazingfate]

If you have time to look at these patches one by one you can see many patches are from kernel mailing list which are not yet merged atm. Some patches are hack to make specific boards work. You can determine whether they will harm your board by reviewing the code. And these patches should have already solved some common issues for your board. You have to solve these issues one by one without these patches.

Take a look at kernel.org. Everyday many patches are merged to mainline and some may cause issues. But people still want to merge code upstream. One universal family will let more people get involved in maintenance of code and reduce repeated work of developer.

[paolosabatino]

We need a way to do Individual kernel patch and kernel config for Individual board，but can not find clue in config/sources/families/include/rockchip64_common.inc so far. For test I compile Image for pinebook-pro which belongs to rockchip64 family, and found info: Summary: kernel patching: 118 total patches; 118 applied; 31 with problems; 31 needs_rebase .So many patches be imposed... It is difficult to determine which patche are useful even harmful. Is there a way include or exclude some of the patches？

If you mean for testing and development purposes, just rename the patch files with .disabled suffix and they won't be included.

In production there is no way to exclude patches for individual boards, nor you can define individual kernel config because it would not be practical: each family (like rockchip64) has a single kernel per branch (branches usually are legacy/vendor, current and edge). Letting each board decide its own patches would create a mayhem of kernels with minimal differences that would be impractical to support, maintain and update via apt repository.

@amazingfate suggestions are totally right, I can add also that many (perhaps most?) existing patches just add device trees or do changes and fixes to existing device trees.

[Evaneee]

We need a way to do Individual kernel patch and kernel config for Individual board，but can not find clue in config/sources/families/include/rockchip64_common.inc so far. For test I compile Image for pinebook-pro which belongs to rockchip64 family, and found info: Summary: kernel patching: 118 total patches; 118 applied; 31 with problems; 31 needs_rebase .So many patches be imposed... It is difficult to determine which patche are useful even harmful. Is there a way include or exclude some of the patches？

If you mean for testing and development purposes, just rename the patch files with .disabled suffix and they won't be included.

In production there is no way to exclude patches for individual boards, nor you can define individual kernel config because it would not be practical: each family (like rockchip64) has a single kernel per branch (branches usually are legacy/vendor, current and edge). Letting each board decide its own patches would create a mayhem of kernels with minimal differences that would be impractical to support, maintain and update via apt repository.

@amazingfate suggestions are totally right, I can add also that many (perhaps most?) existing patches just add device trees or do changes and fixes to existing device trees.

The core problem is: Rk817(for eg) in a special design board need spetial driver patch.
how to solve the problem?

[amazingfate]

There is no such problem because your board is the only one using the driver.

[Evaneee]

If you have time to look at these patches one by one you can see many patches are from kernel mailing list which are not yet merged atm. Some patches are hack to make specific boards work. You can determine whether they will harm your board by reviewing the code. And these patches should have already solved some common issues for your board. You have to solve these issues one by one without these patches.

Take a look at kernel.org. Everyday many patches are merged to mainline and some may cause issues. But people still want to merge code upstream. One universal family will let more people get involved in maintenance of code and reduce repeated work of developer.

If you have time to look at these patches one by one you can see many patches are from kernel mailing list which are not yet merged atm. Some patches are hack to make specific boards work. You can determine whether they will harm your board by reviewing the code. And these patches should have already solved some common issues for your board. You have to solve these issues one by one without these patches.

Take a look at kernel.org. Everyday many patches are merged to mainline and some may cause issues. But people still want to merge code upstream. One universal family will let more people get involved in maintenance of code and reduce repeated work of developer.

if you see the names of the patches imposed to pinbook-pro
add-board-helios64.patch
board-nanopc-t4-add-typec-dp.patch
board-radxa-e25-sdmmc0-fix.patch
...
I do not think there is association board-helios64 and nanopc-t4 or radxa-e25 has strong relationship with pinbook-pro
If these patchs are useful for all, they should be mearge to one file.
If not, but all be imposed to one board, it is strange.
As time goes by, the patches number will be Increase to ah 100000+... files
How the user can determine whether they will harm their board one by one?
and if found some are harmful, how should they do? they can do nothing...
These work is not necessary and totally waste time.

[Evaneee]

There is no such problem because your board is the only one using the driver.

No,there is. but you do not see it. you choise, as a Contributor.

[amazingfate]

There is no such problem because your board is the only one using the driver.

No,there is. but you do not see it. you choise, as a Contributor.

Please tell me which board is using the driver you want to patch. I do not see one that armbian is maintaining.

[Evaneee]

There is no such problem because your board is the only one using the driver.

No,there is. but you do not see it. you choise, as a Contributor.

Please tell me which board is using the driver you want to patch. I do not see one that armbian is maintaining.

A laptop use rk3568, the introduce is in first floor

[amazingfate]

There is no such problem because your board is the only one using the driver.

No,there is. but you do not see it. you choise, as a Contributor.

Please tell me which board is using the driver you want to patch. I do not see one that armbian is maintaining.

A laptop use rk3568, the introduce is in first floor

What other boards will get influenced by your patch?

[Evaneee]

There is no such problem because your board is the only one using the driver.

No,there is. but you do not see it. you choise, as a Contributor.

Please tell me which board is using the driver you want to patch. I do not see one that armbian is maintaining.

A laptop use rk3568, the introduce is in first floor

What other boards will get influenced by your patch?

If I patch rk817 driver, other boards in family will not get the correct battery voltage and capacity. vice versa.

[paolosabatino]

I do not think there is association board-helios64 and nanopc-t4 or radxa-e25 has strong relationship with pinbook-pro
If these patchs are useful for all, they should be mearge to one file.

Perhaps they should not, since grouping them into a single patch file would make maintainance much harder for each developer/maintainer.

If not, but all be imposed to one board, it is strange.
As time goes by, the patches number will be Increase to ah 100000+... files

As time goes by, usually device trees gets updated or mainlined in the kernel and the armbian maintainer just deletes the "board" patch file.

How the user can determine whether they will harm their board one by one?
and if found some are harmful, how should they do? they can do nothing...
These work is not necessary and totally waste time.

Ok, it could be that a patch named board-nanopc-t4-add-typec-dp.patch alters a common driver, but it should not be. The original author had not the time or the will to split it in more parts, which was the right way to do it for easier future maintenance.

The real question is: why they should be harmful?
Normally they fix or enhance the existing drivers with some feature needed by boards; if such a fix or enhancement breaks something, then it is wrongly thought because it breaks existing compatibility. Breaking compatibility is something it should never happen, but happens even within the mainline kernel (a recent thing we dealt with, for example: #6258) so, to answer your original question, you can't be sure any of the patches is not harmful, but you can escalate the same question to kernel developers as well, and they probably would say that, at the end, is the same there too.

[paolosabatino]

If I patch rk817 driver, other boards in family will not get the correct battery voltage and capacity. vice versa.

Perhaps @amazingfate is right and there are no other rk817 boards using the driver, but obviously you principle question is correct as well.

The right answer to this is: device tree.
If your rk817 is a modified version/variant or has some subtle behaviour, you can define a property in the device tree that will be read by the kernel driver.
If the property is not in the device tree, the driver will behave as usual.
If the property is in the device tree, the driver will apply the different behaviour.

This kind of mechanism is quite simple and is applied everywhere when neeeded. On the most common place where I see this is the USB3 designware driver, which has several "quirks" specific for each implementor.

See, for example, some quirk properties in the device tree, and here in the driver they are read to modify the driver behaviour in case they are present; and here there is also some documentation.

If your device is not exactly an rk817 but a variant of it (an hypotethical "rk817a", for example), then you would rather go with slightly different compatible property in the device tree that will be handled by the driver assuming different chip capabilities, like in the ACT88xx regulator drivers.

I guess the last one is not your case though.

Another possibility is to duplicate the rk817 driver and provide a rk817-z96a driver that does the things in its own way, event thought it is a bit non-standard, but surely won't harm other boards

[Evaneee]

I do not think there is association board-helios64 and nanopc-t4 or radxa-e25 has strong relationship with pinbook-pro
If these patchs are useful for all, they should be mearge to one file.

Perhaps they should not, since grouping them into a single patch file would make maintainance much harder for each developer/maintainer.

If not, but all be imposed to one board, it is strange.
As time goes by, the patches number will be Increase to ah 100000+... files

As time goes by, usually device trees gets updated or mainlined in the kernel and the armbian maintainer just deletes the "board" patch file.

How the user can determine whether they will harm their board one by one?
and if found some are harmful, how should they do? they can do nothing...
These work is not necessary and totally waste time.

Ok, it could be that a patch named board-nanopc-t4-add-typec-dp.patch alters a common driver, but it should not be. The original author had not the time or the will to split it in more parts, which was the right way to do it for easier future maintenance.

The real question is: why they should be harmful? Normally they fix or enhance the existing drivers with some feature needed by boards; if such a fix or enhancement breaks something, then it is wrongly thought because it breaks existing compatibility. Breaking compatibility is something it should never happen, but happens even within the mainline kernel (a recent thing we dealt with, for example: #6258) so, to answer your original question, you can't be sure any of the patches is not harmful, but you can escalate the same question to kernel developers as well, and they probably would say that, at the end, is the same there too.

Thanks for patience and Careful explain.
Because I am sure that my patch will harmful for other boards in family.
z96a board designer use a adc pin to messure voltage but not rk817 chip supplied.
rk817-ori-driver vs. rk817-patch-driver. There can only be one exist. only one can be correct.
There are many similar non-standard usages, and they all have their own reasons, ​perhaps...

[amazingfate]

The patch introduces devicetree property io-channels, io-channel-names, adc_vol_ratio, adc_ref_vol and adc_res. Only when the driver find an io channel named battery-voltage then it will parse other adc_* properties. These properties is just like a switch to turn on the patched code. I think this is already clear for someone in the future who want to deal with rk817 battery stuff on other boards. The patched driver should work as not patched if devicetree doesn't have these properties, and if not, it should be easy for someone to fix.

[Evaneee]

If I patch rk817 driver, other boards in family will not get the correct battery voltage and capacity. vice versa.

Perhaps @amazingfate is right and there are no other rk817 boards using the driver, but obviously you principle question is correct as well.

The right answer to this is: device tree. If your rk817 is a modified version/variant or has some subtle behaviour, you can define a property in the device tree that will be read by the kernel driver. If the property is not in the device tree, the driver will behave as usual. If the property is in the device tree, the driver will apply the different behaviour.

This kind of mechanism is quite simple and is applied everywhere when neeeded. On the most common place where I see this is the USB3 designware driver, which has several "quirks" specific for each implementor.

See, for example, some quirk properties in the device tree, and here in the driver they are read to modify the driver behaviour in case they are present; and here there is also some documentation.

If your device is not exactly an rk817 but a variant of it (an hypotethical "rk817a", for example), then you would rather go with slightly different compatible property in the device tree that will be handled by the driver assuming different chip capabilities, like in the ACT88xx regulator drivers.

I guess the last one is not your case though.

Another possibility is to duplicate the rk817 driver and provide a rk817-z96a driver that does the things in its own way, event thought it is a bit non-standard, but surely won't harm other boards

On my board, rk817 is specially used and only has a little function changed.
It is not very useful for most kernel users.
Commit to kernel doesn't seem very appropriate.
So I choose duplicate the rk817 driver and provide a rk817-z96a driver
And I thought armbian is the place to absorb the slightly diffrence.....before.

[Evaneee]

The patch introduces devicetree property io-channels, io-channel-names, adc_vol_ratio, adc_ref_vol and adc_res. Only when the driver find an io channel named battery-voltage then it will parse other adc_* properties. These properties is just like a switch to turn on the patched code. I think this is already clear for someone in the future who want to deal with rk817 battery stuff on other boards. The patched driver should work as not patched if devicetree doesn't have these properties, and if not, it should be easy for someone to fix.

Looks like a bomb is being planted.

[amazingfate]

The patch introduces devicetree property io-channels, io-channel-names, adc_vol_ratio, adc_ref_vol and adc_res. Only when the driver find an io channel named battery-voltage then it will parse other adc_* properties. These properties is just like a switch to turn on the patched code. I think this is already clear for someone in the future who want to deal with rk817 battery stuff on other boards. The patched driver should work as not patched if devicetree doesn't have these properties, and if not, it should be easy for someone to fix.

Looks like a bomb is being planted.

You are the author of this patch, it should be not difficult for you to make the driver work as the original version if devicetree doesn't have these special properties in code level.

And at the moment this kernel doesn't have users that use rk817 battery driver. So it is a bomb which won't explode and if you deal with this patch properly, it will also not harm people who is gonna deal with rk817 driver in the future.

[Evaneee]

The patch introduces devicetree property io-channels, io-channel-names, adc_vol_ratio, adc_ref_vol and adc_res. Only when the driver find an io channel named battery-voltage then it will parse other adc_* properties. These properties is just like a switch to turn on the patched code. I think this is already clear for someone in the future who want to deal with rk817 battery stuff on other boards. The patched driver should work as not patched if devicetree doesn't have these properties, and if not, it should be easy for someone to fix.

Looks like a bomb is being planted.

You are the author of this patch, it should be not difficult for you to make the driver work as the original version if devicetree doesn't have these special properties in code level.

And at the moment this kernel doesn't have users that use rk817 battery driver. So it is a bomb which won't explode and if you deal with this patch properly, it will also not harm people who is gonna deal with rk817 driver in the future.

Because of a patch, both subsequent users and previous users will be affected. This is not a major functional upgrade, but an inconspicuous small application, because the application of a small group of people affects the majority of people (without their knowledge), It should not be encouraged.

[paolosabatino]

Because of a patch, both subsequent users and previous users will be affected. This is not a major functional upgrade, but an inconspicuous small application, because the application of a small group of people affects the majority of people (without their knowledge), It should not be encouraged.

I looked into the patch code with little bit more detail and it looks correct to me. Some code style could be fixed (for example, the various !IS_ERR() in the code could be changed to be more understandable and/or moved into a single function, some returns are superflous, ...), but the general idea there seems right: if the battery voltage is supplied by and external adc, declare the link in device tree node of the rk817 and the driver will use it to read the data. Otherwise the driver will use the existing pathway.

I would say that the patch to the driver is already thought to be backwards compatible, even thought the implementation is not perfect.

[Evaneee]

Because of a patch, both subsequent users and previous users will be affected. This is not a major functional upgrade, but an inconspicuous small application, because the application of a small group of people affects the majority of people (without their knowledge), It should not be encouraged.

I looked into the patch code with little bit more detail and it looks correct to me. Some code style could be fixed (for example, the various !IS_ERR() in the code could be changed to be more understandable and/or moved into a single function, some returns are superflous, ...), but the general idea there seems right: if the battery voltage is supplied by and external adc, declare the link in device tree node of the rk817 and the driver will use it to read the data. Otherwise the driver will use the existing pathway.

I would say that the patch to the driver is already thought to be backwards compatible, even thought the implementation is not perfect.

Thanks for looking into the code..
This is just 817, there may be countless 817...Authors vary in level.
Just feel uneasy knowing that if I enter the current release in the future, I will be forced to receive 100+ unnecessary patches for various functions.
I still hope the maintainers will seriously consider this issue, reduce the burden on users, and increase non-standard compatibility.

[amazingfate]

rockchip64 is a kernel family for all rockchip 64bit socs from rk33xx to rk35xx supported by mainline kernel. Ideally all the patches should be able to get merged upstream. And each time we bump kernel version there are patches removed because of getting merged upstream. But for many reasons there are dead patches like add-board-helios64.patch which comes from an old date is not touched by people for a long time. Unless somebody starts to maintain helios64 this patch will stay at the current status.

Over one hundred patches applied to rockchip64 family is inevitable because there are so many boards included in this family and there are issues not solved by upstream mainline kernel that need patches to get fixed.

All board-*.patch should be the patch applied to board level dts, so people will know which specific board a patch modifies by its name. And these patches should not include patches at higher level like soc devicetree like rk356x.dtsi or driver code.
Soc level devicetree patches should start with soc name like rk3399.
And driver code level patches should be started with general, wifi or other names to mke people know which kernel subsystem they modified.
These rules should make people easily know patches in a glance.

These patches are not burden, but are the wealth accumulated over the years. Armbian has to maintain a lot of boards with not so many developers, so it is always good to make our code more understandable by new developers. And patches generated by atomic commit will make our development and maintenance easier.

[SteeManMI]

Because of a patch, both subsequent users and previous users will be affected. This is not a major functional upgrade, but an inconspicuous small application, because the application of a small group of people affects the majority of people (without their knowledge), It should not be encouraged.

I want to elaborate on the reasons not to have a different family. While I understand your thoughts on the potential to impact others with your changes, there are a lot of reasons in Armbian we want to limit the number of distinct families. Each family results in a distinct kernel build. There is a lot of infrastructure necessary to maintain distinct kernels (not only build machines, testing environments, but also apt servers and overall disk storage). If every board had it's own kernel that would quickly become unmanageable. (And while we aren't there yet, any mainline based kernel, should be actively tracking upstream releases, which average one patch release a week, so you are quickly talking a lot of distinct kernel builds) But there are also a lot of developer resources necessary to maintain distinct kernels. Every time a patch is included into a soc based kernel, someone needs to decide should that be applied to all of the 'similar' families/kernels? Then how are all those tested when the developer likely only has a very limited set of boards personally, and the test farm is far from complete as well.

So Armbian struggles to minimize the number of distinct families/kernels (some have pushed that there should only be one kernel for everything), but also realize that sometimes multiple are needed. So the Armbian infrastructure provides the capability to have as many families as necessary, but it is through reviews of PRs like this that the core team gets to decide if the needs for a distinct family/kernel are worth the extra overhead and maintenance costs long term.

[Evaneee]

Because of a patch, both subsequent users and previous users will be affected. This is not a major functional upgrade, but an inconspicuous small application, because the application of a small group of people affects the majority of people (without their knowledge), It should not be encouraged.

I want to elaborate on the reasons not to have a different family. While I understand your thoughts on the potential to impact others with your changes, there are a lot of reasons in Armbian we want to limit the number of distinct families. Each family results in a distinct kernel build. There is a lot of infrastructure necessary to maintain distinct kernels (not only build machines, testing environments, but also apt servers and overall disk storage). If every board had it's own kernel that would quickly become unmanageable. (And while we aren't there yet, any mainline based kernel, should be actively tracking upstream releases, which average one patch release a week, so you are quickly talking a lot of distinct kernel builds) But there are also a lot of developer resources necessary to maintain distinct kernels. Every time a patch is included into a soc based kernel, someone needs to decide should that be applied to all of the 'similar' families/kernels? Then how are all those tested when the developer likely only has a very limited set of boards personally, and the test farm is far from complete as well.

So Armbian struggles to minimize the number of distinct families/kernels (some have pushed that there should only be one kernel for everything), but also realize that sometimes multiple are needed. So the Armbian infrastructure provides the capability to have as many families as necessary, but it is through reviews of PRs like this that the core team gets to decide if the needs for a distinct family/kernel are worth the extra overhead and maintenance costs long term.

To me, this explanation is convincing enough, thank you for providing such a real reason. So next, we have to hope how to solve rockchip’s chaotic family and structure.

[Evaneee]

rockchip64 is a kernel family for all rockchip 64bit socs from rk33xx to rk35xx supported by mainline kernel. Ideally all the patches should be able to get merged upstream. And each time we bump kernel version there are patches removed because of getting merged upstream. But for many reasons there are dead patches like add-board-helios64.patch which comes from an old date is not touched by people for a long time. Unless somebody starts to maintain helios64 this patch will stay at the current status.

Over one hundred patches applied to rockchip64 family is inevitable because there are so many boards included in this family and there are issues not solved by upstream mainline kernel that need patches to get fixed.

All board-*.patch should be the patch applied to board level dts, so people will know which specific board a patch modifies by its name. And these patches should not include patches at higher level like soc devicetree like rk356x.dtsi or driver code. Soc level devicetree patches should start with soc name like rk3399. And driver code level patches should be started with general, wifi or other names to mke people know which kernel subsystem they modified. These rules should make people easily know patches in a glance.

These patches are not burden, but are the wealth accumulated over the years. Armbian has to maintain a lot of boards with not so many developers, so it is always good to make our code more understandable by new developers. And patches generated by atomic commit will make our development and maintenance easier.

After understanding the underlying reasons of armbian, I will follow your suggestion and change the submission to the rk35xx family. Wish to participate in contributing to a more perfect Armbian.