Re: Adding support for Orange Pi Zero 3

Re: Adding support for Orange Pi Zero 3

[Andre Przywara]

On Sat, 4 Nov 2023 19:17:44 +0000
electricworry <worryelectric@gmail.com> wrote:

Hi,

> I would like to contribute improvements u-boot to enable support for
> the Orange Pi Zero 3 board. I have already gained a good understanding
> of the project and the standards for changes, and so I have made
> modifications and tested them on my board.

thanks for your interest and for reaching out!

I will answer your questions below, but for this particular topic there
are already support patches on the list (sorry for the disappointment!):
https://lore.kernel.org/u-boot/20231114013106.31336-1-andre.przywara@arm.com/

Can you please apply those patches on top of mainline U-Boot and test
them? Then reply to patch 3/3 above with a Tested-by: line, stating your
name and email address?
Having independent reports of that working would make me more confident in
merging the series.

As a general advice: try to reach out early, to avoid working in parallel
on something, and also to avoid working in the wrong direction.
There is the #linux-sunxi IRC channel on the OFTC IRC network[1] (you can
join via any browser), also there is a mailing list for Allwinner related
upstream work: linux-sunxi@lists.linux.dev [2]
Another resource is our wiki: https://linux-sunxi.org

[1] https://www.oftc.net/WebChat/
    https://oftc.irclog.whitequark.org/linux-sunxi
[2] https://subspace.kernel.org/lists.linux.dev.html

> Before submitting patches for review I've got a few questions I'd like
> to discuss.
> 
> The board is an Allwinner H618. From what I can gather this is just an
> H616 with minor changes. The changes are so minor that I've been able
> to implement the support as changes to the existing files for H616.

Just curious, what changes did you find in particular? So far we know
about the increased L2 cache, which is irrelevant from a software support
perspective, and the change of base address and layout of the CPUX_CFG
block, which is already covered by:
https://source.denx.de/u-boot/u-boot/-/commit/0a137ac5015933bf38ea2700abe70602ef63bbdd
Did you find anything else?

> For example, to add LPDDR4 support I have been able to make changes to
> arch/arm/mach-sunxi/dram_sun50i_h616.c and done so in a well

Out of interest, how did you do this? Disassembled the vendor boot0?
Dumping DRAM registers? We discussed and tested the LPDDR4 support already
a while ago, and support for that was merged recently:
https://source.denx.de/u-boot/u-boot/-/commit/4b02f0120a4bb2a5d7081aef8cef6a4ca57e9db2

> configured way that won't impact anything else. Is doing it in this
> way acceptable or would the preference be to have a separate file

In general we try to keep the code as much together as possible,
especially when there is a high overlap, like in this case.
So yeah, one file to drive all variants of DRAM for this SoC.

> (e.g. dram_sun50i_h618.c) to keep H616 and H618 separate? The way I've

Please note that the DRAM controller in both chips is the same, it's just
that the OrangePi boards using the H618 are the first easily available to
use LPDDR4. Mikhail was in fact developing his patch on a custom board
with LPDDR4 DRAM, using the Allwinner T507 (a repackaged H616).

> done it feels right because it really is just a minor upgrade, but
> there might be other opinions I should hear first.
> 
> Further, with regards to attribution, all of my changes are based on
> code in https://github.com/orangepi-xunlong/u-boot-orangepi. I've not
> been able to cherry-pick changes because that codebase has departed
> severely from the u-boot master branch and it's not as elegantly
> maintained, so I've had to surgically move just the changes required

I see you figured ;-)

> to get my board working. Their repo is correctly marked as GPL-2.0 so
> compatible with merging, but the exact author of every line of code
> will be difficult to attribute. Is it sufficient for me to cite that
> downstream repo as the source of the patches?

Yes, if you (need to) make significant changes, it's common practice to
make those your patches, and just mention the original work. At the end
the original author apparently couldn't be bothered to upstream this.
If possible, it's nice to try to reach the author, and ask for their
intention on this: often they don't really care (anymore), so are happy if
you do the upstreaming work.

Another way would be to take an original patch, adjust/rebase it, and
send it on behalf of the original author, keeping their authorship
(From:). This requires a Signed-off-by: line from the author, and *your*
Signed-off-by: line added, to confirm that you obtained those patches
legally and in compliance with the license.

Commonly this generates a series with a mixture of original patches,
rewritten patches (you as the author), and slightly adjusted patches (them
as the author).

Hope that helps!

Cheers,
Andre

> Thanks in advance. I've not submitted a patch here before so I could
> benefit from some hand-holding to not create problems for people, but
> I'm pretty confident that the changes I've made comply with the
> standards of the project and that there won't be much of an issue
> having them accepted.

Re: Adding support for Orange Pi Zero 3

[Bob McChesney]

On Thu, Nov 23, 2023 at 03:17:09PM +0000, Andre Przywara wrote:
> On Sat, 4 Nov 2023 19:17:44 +0000
> electricworry <worryelectric@gmail.com> wrote:
> 
> Hi,
> 
> > I would like to contribute improvements u-boot to enable support for
> > the Orange Pi Zero 3 board. I have already gained a good understanding
> > of the project and the standards for changes, and so I have made
> > modifications and tested them on my board.
> 
> thanks for your interest and for reaching out!
> 
> I will answer your questions below, but for this particular topic there
> are already support patches on the list (sorry for the disappointment!):
> https://lore.kernel.org/u-boot/20231114013106.31336-1-andre.przywara@arm.com/

That's good. I'm happy that more experienced people have been working on
this. It's been a good learning experience nevertheless.

> Can you please apply those patches on top of mainline U-Boot and test
> them? Then reply to patch 3/3 above with a Tested-by: line, stating your
> name and email address?
> Having independent reports of that working would make me more confident in
> merging the series.

I'm happy to do that and will be able to do it shortly. In this case
would I be applying the patches on top of the next branch?

> As a general advice: try to reach out early, to avoid working in parallel
> on something, and also to avoid working in the wrong direction.
> There is the #linux-sunxi IRC channel on the OFTC IRC network[1] (you can
> join via any browser), also there is a mailing list for Allwinner related
> upstream work: linux-sunxi@lists.linux.dev [2]
> Another resource is our wiki: https://linux-sunxi.org
> 
> [1] https://www.oftc.net/WebChat/
>     https://oftc.irclog.whitequark.org/linux-sunxi
> [2] https://subspace.kernel.org/lists.linux.dev.html

Thank you. I wasn't aware of this community and have been working alone.
It will be more productive to work and discuss with others so I'll join
that mailing list too.

> > Before submitting patches for review I've got a few questions I'd like
> > to discuss.
> > 
> > The board is an Allwinner H618. From what I can gather this is just an
> > H616 with minor changes. The changes are so minor that I've been able
> > to implement the support as changes to the existing files for H616.
> 
> Just curious, what changes did you find in particular? So far we know
> about the increased L2 cache, which is irrelevant from a software support
> perspective, and the change of base address and layout of the CPUX_CFG
> block, which is already covered by:
> https://source.denx.de/u-boot/u-boot/-/commit/0a137ac5015933bf38ea2700abe70602ef63bbdd
> Did you find anything else?

I am not what anyone could call a hardware expert so I've just been
working by diffing the codebase from the vendor against u-boot mainline.
Changes that I've created patches for include their full dtb, the LPDDR4
support, the AXP313A power, and - perhaps incorrectly - zeroing a
register so that the thermal driver in the kernel works properly and
doesn't get false high temperature readings. I say "perhaps incorrectly"
because I feel like the kernel driver should be doing that work itself
and the comment in the vendor u-boot code suggests it's a hack: "The
bit[16] of register reg[0x03000000] must be zero for the THS driver to
work properly in the kernel. The BSP u-boot is putting the whole
register to zero so we are doing the same."

> Out of interest, how did you do this? Disassembled the vendor boot0?
> Dumping DRAM registers? We discussed and tested the LPDDR4 support already
> a while ago, and support for that was merged recently:
> https://source.denx.de/u-boot/u-boot/-/commit/4b02f0120a4bb2a5d7081aef8cef6a4ca57e9db2

As above, I didn't do anything clever like probe hardware or
disassemble binaries; it was just a brute force loop of cherry-picking,
compiling and testing. And also doing it in a way that fitted the
incumbent style and practices of mainline. e.g. making use of configs
like CONFIG_DRAM_SUN50I_H616_DX_ODT for the board family rathern than
having board specific ifdefs. The work done is available if anyone wants
to see it, and I'd be happy to have it critiqued. Looking at the patches
proposed here I'm happy to see that we've come to the same conclusion on
some things (such as the above config) and others we diverge on, but as
I can see from the discussion in linux-sunxi the feeling is that some of
these difference don't matter.

> Hope that helps!
> 
> Cheers,
> Andre

Thanks for all of that good advice. It's immensely appreciated. I'll
keep this as a guide for contibuting patches in the future.

Now that there's a working u-boot for the board - I've got two to choose
from now :) - I'm going to be concentrating on trying to get the Mali
graphics working as it's a requirement for my project. I've got a
working kernel (6.1.31 with patches) but I can also see that work's been
underway to bring the support to 6.7. I tested 6.7-rc2 today and can't
get any output at all on the console after u-boot starts the kernel.

Cheers,
Bob

Re: Adding support for Orange Pi Zero 3

[Andre Przywara]

On Sun, 26 Nov 2023 11:32:35 +0000
Bob McChesney <bob@electricworry.net> wrote:

Hi Bob,

thanks for the reply!

CC:ing Jernej for the THS SRAM issue and the HDMI support mentioned
below.

> On Thu, Nov 23, 2023 at 03:17:09PM +0000, Andre Przywara wrote:
> > On Sat, 4 Nov 2023 19:17:44 +0000
> > electricworry <worryelectric@gmail.com> wrote:
> > 
> > Hi,
> >   
> > > I would like to contribute improvements u-boot to enable support for
> > > the Orange Pi Zero 3 board. I have already gained a good understanding
> > > of the project and the standards for changes, and so I have made
> > > modifications and tested them on my board.  
> > 
> > thanks for your interest and for reaching out!
> > 
> > I will answer your questions below, but for this particular topic there
> > are already support patches on the list (sorry for the disappointment!):
> > https://lore.kernel.org/u-boot/20231114013106.31336-1-andre.przywara@arm.com/  
> 
> That's good. I'm happy that more experienced people have been working on
> this. It's been a good learning experience nevertheless.
> 
> > Can you please apply those patches on top of mainline U-Boot and test
> > them? Then reply to patch 3/3 above with a Tested-by: line, stating your
> > name and email address?
> > Having independent reports of that working would make me more confident in
> > merging the series.  
> 
> I'm happy to do that and will be able to do it shortly. In this case
> would I be applying the patches on top of the next branch?

The normal master branch would be safer. I don't typically work on the
next branch, and there might be changes in there which subtly break
sunxi.
 
> > As a general advice: try to reach out early, to avoid working in parallel
> > on something, and also to avoid working in the wrong direction.
> > There is the #linux-sunxi IRC channel on the OFTC IRC network[1] (you can
> > join via any browser), also there is a mailing list for Allwinner related
> > upstream work: linux-sunxi@lists.linux.dev [2]
> > Another resource is our wiki: https://linux-sunxi.org
> > 
> > [1] https://www.oftc.net/WebChat/
> >     https://oftc.irclog.whitequark.org/linux-sunxi
> > [2] https://subspace.kernel.org/lists.linux.dev.html  
> 
> Thank you. I wasn't aware of this community and have been working alone.
> It will be more productive to work and discuss with others so I'll join
> that mailing list too.

Yeah, the IRC chat is the most responsive and most helpful, I find, so
try to join that.

> 
> > > Before submitting patches for review I've got a few questions I'd like
> > > to discuss.
> > > 
> > > The board is an Allwinner H618. From what I can gather this is just an
> > > H616 with minor changes. The changes are so minor that I've been able
> > > to implement the support as changes to the existing files for H616.  
> > 
> > Just curious, what changes did you find in particular? So far we know
> > about the increased L2 cache, which is irrelevant from a software support
> > perspective, and the change of base address and layout of the CPUX_CFG
> > block, which is already covered by:
> > https://source.denx.de/u-boot/u-boot/-/commit/0a137ac5015933bf38ea2700abe70602ef63bbdd
> > Did you find anything else?  
> 
> I am not what anyone could call a hardware expert so I've just been
> working by diffing the codebase from the vendor against u-boot mainline.

Most of us are indeed curious software developers, and we typically do
a mixture of looking at vendor code, checking manuals, and doing live
experiments to figure things out.

> Changes that I've created patches for include their full dtb, the LPDDR4
> support, the AXP313A power, and - perhaps incorrectly - zeroing a
> register so that the thermal driver in the kernel works properly and
> doesn't get false high temperature readings.

Ah, glad I am not the only one seeing this!
I strangely haven't seen this all the time, but I indeed now need to do
a "mw.l 0x3000000 0x7ffeffff" in U-Boot, otherwise I see the kernel
screaming about +200C temperatures.

> I say "perhaps incorrectly"
> because I feel like the kernel driver should be doing that work itself
> and the comment in the vendor u-boot code suggests it's a hack: "The
> bit[16] of register reg[0x03000000] must be zero for the THS driver to
> work properly in the kernel. The BSP u-boot is putting the whole
> register to zero so we are doing the same."

Very good, you are exactly on the same path as I am! I have a patch for
the THS driver that tries to clear the bit, via using the regmap
exported by syscon, but I need to test this further (was my plan for
today). I think this was somehow not working properly the last time I
checked.
Another approach was trying to claim an SRAM region - as this is what
this register does for the video engine (VE). I figured that apparently
just bit 0 switches the SRAM to the VE, and flipping all 31 bits might
be some anachronism from the olden A10 days.
So we might claim that bit 16 switches the SRAM for THS, but then I am
not sure it really has something to do with SRAM, or if it's just
another Allwinner special of "oh, we have an unused bit in this
register" ;-)
We will see what the list has to say about this.

> > Out of interest, how did you do this? Disassembled the vendor boot0?
> > Dumping DRAM registers? We discussed and tested the LPDDR4 support already
> > a while ago, and support for that was merged recently:
> > https://source.denx.de/u-boot/u-boot/-/commit/4b02f0120a4bb2a5d7081aef8cef6a4ca57e9db2  
> 
> As above, I didn't do anything clever like probe hardware or
> disassemble binaries; it was just a brute force loop of cherry-picking,
> compiling and testing. And also doing it in a way that fitted the
> incumbent style and practices of mainline. e.g. making use of configs
> like CONFIG_DRAM_SUN50I_H616_DX_ODT for the board family rathern than
> having board specific ifdefs. The work done is available if anyone wants
> to see it, and I'd be happy to have it critiqued. Looking at the patches
> proposed here I'm happy to see that we've come to the same conclusion on
> some things (such as the above config) and others we diverge on, but as
> I can see from the discussion in linux-sunxi the feeling is that some of
> these difference don't matter.
> 
> > Hope that helps!
> > 
> > Cheers,
> > Andre  
> 
> Thanks for all of that good advice. It's immensely appreciated. I'll
> keep this as a guide for contibuting patches in the future.
> 
> Now that there's a working u-boot for the board - I've got two to choose
> from now :) - I'm going to be concentrating on trying to get the Mali
> graphics working as it's a requirement for my project. I've got a
> working kernel (6.1.31 with patches) but I can also see that work's been
> underway to bring the support to 6.7. I tested 6.7-rc2 today and can't
> get any output at all on the console after u-boot starts the kernel.

Yeah, that's because we don't have any video (HDMI) output support
(something completely unrelated to Mali, which is just a 3D renderer!)
yet in mainline. Jernej was sitting on *some* patches for a while, but
didn't have the time to upstream them, and IIUC there is something odd
to fix first before it's mainline ready.
There are some rough bits in here, and in other repos floating around:
https://github.com/jernejsk/linux-1/commits/h616-hdmi-v2

Cheers,
Andre

Re: Adding support for Orange Pi Zero 3

[Bob McChesney]

On Sun, Nov 26, 2023 at 12:33:51PM +0000, Andre Przywara wrote:
> On Sun, 26 Nov 2023 11:32:35 +0000
> Bob McChesney <bob@electricworry.net> wrote:
> 
> Hi Bob,
> 
> thanks for the reply!
> 
> > Now that there's a working u-boot for the board - I've got two to choose
> > from now :) - I'm going to be concentrating on trying to get the Mali
> > graphics working as it's a requirement for my project. I've got a
> > working kernel (6.1.31 with patches) but I can also see that work's been
> > underway to bring the support to 6.7. I tested 6.7-rc2 today and can't
> > get any output at all on the console after u-boot starts the kernel.
> 
> Yeah, that's because we don't have any video (HDMI) output support
> (something completely unrelated to Mali, which is just a 3D renderer!)
> yet in mainline. Jernej was sitting on *some* patches for a while, but
> didn't have the time to upstream them, and IIUC there is something odd
> to fix first before it's mainline ready.
> There are some rough bits in here, and in other repos floating around:
> https://github.com/jernejsk/linux-1/commits/h616-hdmi-v2

The lack of console output I referred to was over the serial port. I
expected that when I swapped out my modified 6.1.31 kernel for 6.7-rc2
that I would still get serial output but nothing; the dtb in mainline
looks pretty sparse compared to mine.

> 
> Cheers,
> Andre

Regards,
Bob

Re: Adding support for Orange Pi Zero 3

[Jernej Škrabec]

Dne nedelja, 26. november 2023 ob 13:33:51 CET je Andre Przywara napisal(a):
> On Sun, 26 Nov 2023 11:32:35 +0000
> Bob McChesney <bob@electricworry.net> wrote:
> 
> Hi Bob,
> 
> thanks for the reply!
> 
> CC:ing Jernej for the THS SRAM issue and the HDMI support mentioned
> below.
> 
> > On Thu, Nov 23, 2023 at 03:17:09PM +0000, Andre Przywara wrote:
> > > On Sat, 4 Nov 2023 19:17:44 +0000
> > > electricworry <worryelectric@gmail.com> wrote:
> > > 
> > > Hi,
> > >   
> > > > I would like to contribute improvements u-boot to enable support for
> > > > the Orange Pi Zero 3 board. I have already gained a good understanding
> > > > of the project and the standards for changes, and so I have made
> > > > modifications and tested them on my board.  
> > > 
> > > thanks for your interest and for reaching out!
> > > 
> > > I will answer your questions below, but for this particular topic there
> > > are already support patches on the list (sorry for the disappointment!):
> > > https://lore.kernel.org/u-boot/20231114013106.31336-1-andre.przywara@arm.com/  
> > 
> > That's good. I'm happy that more experienced people have been working on
> > this. It's been a good learning experience nevertheless.
> > 
> > > Can you please apply those patches on top of mainline U-Boot and test
> > > them? Then reply to patch 3/3 above with a Tested-by: line, stating your
> > > name and email address?
> > > Having independent reports of that working would make me more confident in
> > > merging the series.  
> > 
> > I'm happy to do that and will be able to do it shortly. In this case
> > would I be applying the patches on top of the next branch?
> 
> The normal master branch would be safer. I don't typically work on the
> next branch, and there might be changes in there which subtly break
> sunxi.
>  
> > > As a general advice: try to reach out early, to avoid working in parallel
> > > on something, and also to avoid working in the wrong direction.
> > > There is the #linux-sunxi IRC channel on the OFTC IRC network[1] (you can
> > > join via any browser), also there is a mailing list for Allwinner related
> > > upstream work: linux-sunxi@lists.linux.dev [2]
> > > Another resource is our wiki: https://linux-sunxi.org
> > > 
> > > [1] https://www.oftc.net/WebChat/
> > >     https://oftc.irclog.whitequark.org/linux-sunxi
> > > [2] https://subspace.kernel.org/lists.linux.dev.html  
> > 
> > Thank you. I wasn't aware of this community and have been working alone.
> > It will be more productive to work and discuss with others so I'll join
> > that mailing list too.
> 
> Yeah, the IRC chat is the most responsive and most helpful, I find, so
> try to join that.
> 
> > 
> > > > Before submitting patches for review I've got a few questions I'd like
> > > > to discuss.
> > > > 
> > > > The board is an Allwinner H618. From what I can gather this is just an
> > > > H616 with minor changes. The changes are so minor that I've been able
> > > > to implement the support as changes to the existing files for H616.  
> > > 
> > > Just curious, what changes did you find in particular? So far we know
> > > about the increased L2 cache, which is irrelevant from a software support
> > > perspective, and the change of base address and layout of the CPUX_CFG
> > > block, which is already covered by:
> > > https://source.denx.de/u-boot/u-boot/-/commit/0a137ac5015933bf38ea2700abe70602ef63bbdd
> > > Did you find anything else?  
> > 
> > I am not what anyone could call a hardware expert so I've just been
> > working by diffing the codebase from the vendor against u-boot mainline.
> 
> Most of us are indeed curious software developers, and we typically do
> a mixture of looking at vendor code, checking manuals, and doing live
> experiments to figure things out.
> 
> > Changes that I've created patches for include their full dtb, the LPDDR4
> > support, the AXP313A power, and - perhaps incorrectly - zeroing a
> > register so that the thermal driver in the kernel works properly and
> > doesn't get false high temperature readings.
> 
> Ah, glad I am not the only one seeing this!
> I strangely haven't seen this all the time, but I indeed now need to do
> a "mw.l 0x3000000 0x7ffeffff" in U-Boot, otherwise I see the kernel
> screaming about +200C temperatures.
> 
> > I say "perhaps incorrectly"
> > because I feel like the kernel driver should be doing that work itself
> > and the comment in the vendor u-boot code suggests it's a hack: "The
> > bit[16] of register reg[0x03000000] must be zero for the THS driver to
> > work properly in the kernel. The BSP u-boot is putting the whole
> > register to zero so we are doing the same."
> 
> Very good, you are exactly on the same path as I am! I have a patch for
> the THS driver that tries to clear the bit, via using the regmap
> exported by syscon, but I need to test this further (was my plan for
> today). I think this was somehow not working properly the last time I
> checked.
> Another approach was trying to claim an SRAM region - as this is what
> this register does for the video engine (VE). I figured that apparently
> just bit 0 switches the SRAM to the VE, and flipping all 31 bits might
> be some anachronism from the olden A10 days.
> So we might claim that bit 16 switches the SRAM for THS, but then I am
> not sure it really has something to do with SRAM, or if it's just
> another Allwinner special of "oh, we have an unused bit in this
> register" ;-)
> We will see what the list has to say about this.
> 
> > > Out of interest, how did you do this? Disassembled the vendor boot0?
> > > Dumping DRAM registers? We discussed and tested the LPDDR4 support already
> > > a while ago, and support for that was merged recently:
> > > https://source.denx.de/u-boot/u-boot/-/commit/4b02f0120a4bb2a5d7081aef8cef6a4ca57e9db2  
> > 
> > As above, I didn't do anything clever like probe hardware or
> > disassemble binaries; it was just a brute force loop of cherry-picking,
> > compiling and testing. And also doing it in a way that fitted the
> > incumbent style and practices of mainline. e.g. making use of configs
> > like CONFIG_DRAM_SUN50I_H616_DX_ODT for the board family rathern than
> > having board specific ifdefs. The work done is available if anyone wants
> > to see it, and I'd be happy to have it critiqued. Looking at the patches
> > proposed here I'm happy to see that we've come to the same conclusion on
> > some things (such as the above config) and others we diverge on, but as
> > I can see from the discussion in linux-sunxi the feeling is that some of
> > these difference don't matter.
> > 
> > > Hope that helps!
> > > 
> > > Cheers,
> > > Andre  
> > 
> > Thanks for all of that good advice. It's immensely appreciated. I'll
> > keep this as a guide for contibuting patches in the future.
> > 
> > Now that there's a working u-boot for the board - I've got two to choose
> > from now :) - I'm going to be concentrating on trying to get the Mali
> > graphics working as it's a requirement for my project. I've got a
> > working kernel (6.1.31 with patches) but I can also see that work's been
> > underway to bring the support to 6.7. I tested 6.7-rc2 today and can't
> > get any output at all on the console after u-boot starts the kernel.
> 
> Yeah, that's because we don't have any video (HDMI) output support
> (something completely unrelated to Mali, which is just a 3D renderer!)
> yet in mainline. Jernej was sitting on *some* patches for a while, but
> didn't have the time to upstream them, and IIUC there is something odd
> to fix first before it's mainline ready.
> There are some rough bits in here, and in other repos floating around:
> https://github.com/jernejsk/linux-1/commits/h616-hdmi-v2

That one is old. Most recent development is in
https://github.com/jernejsk/linux-1/commits/h616-var

However, note that there is quite a lot of work to do before it could
be mainlined.

Best regards,
Jernej