diff mbox series

sunxi: board: provide CPU idle states to loaded OS

Message ID 20230904205430.1647654-1-andrej.skvortzov@gmail.com
State New
Delegated to: Andre Przywara
Headers show
Series sunxi: board: provide CPU idle states to loaded OS | expand

Commit Message

Andrey Skvortsov Sept. 4, 2023, 8:54 p.m. UTC
When using SCPI as the PSCI backend, firmware can wake up the CPUs and
cluster from sleep, so CPU idle states are available for loaded OS to
use. TF-A modifies DTB to advertise available CPU idle states, when
SCPI is detected. This change copies nodes added by TF-A to any new
dtb that is used for loaded OS.

Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
---
 board/sunxi/board.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

Comments

Andre Przywara Sept. 5, 2023, 8:27 a.m. UTC | #1
On Mon,  4 Sep 2023 23:54:30 +0300
Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:

Hi Andrey,

> When using SCPI as the PSCI backend, firmware can wake up the CPUs and
> cluster from sleep, so CPU idle states are available for loaded OS to
> use. TF-A modifies DTB to advertise available CPU idle states, when
> SCPI is detected. This change copies nodes added by TF-A to any new
> dtb that is used for loaded OS.

Why do you need that, exactly? Why not just use $fdtcontroladdr for the
kernel? We now keep the U-Boot copy of the .dts files in sync with the
kernel. If you need to modify the DT in U-Boot, for instance by applying
overlays, you can copy that DTB into a better suitable location first:
=> fdt move $fdtcontroladdr $fdt_addr_r

In any case, there shall be only one DT, that one in the U-Boot image. Why
do you need to load another one for the kernel?

Cheers,
Andre

> Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>

> ---
>  board/sunxi/board.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index f321cd58a6..e88bd11a99 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -870,6 +870,125 @@ int board_late_init(void)
>  	return 0;
>  }
>  
> +static int cpuidle_dt_fixup_copy_node(ofnode src, int dst_offset, void *dst_blob,
> +				      u32 *count, u32 *phandle)
> +{
> +	int offs, len, ret;
> +	struct ofprop prop;
> +	ofnode subnode;
> +
> +	ofnode_for_each_prop(prop, src) {
> +		const char *name;
> +		const char *val;
> +
> +		val = ofprop_get_property(&prop, &name, &len);
> +		if (!val)
> +			return -EINVAL;
> +		if (!strcmp(name, "phandle")) {
> +			if (ofnode_device_is_compatible(src, "arm,idle-state")) {
> +				fdt_setprop_u32(dst_blob, dst_offset, name, *phandle);
> +				(*phandle)++;
> +				(*count)++;
> +			} else {
> +				log_err("Unexpected phandle node: %s\n", name);
> +				return -EINVAL;
> +			}
> +		} else {
> +			ret = fdt_setprop(dst_blob, dst_offset, name, val, len);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
> +	ofnode_for_each_subnode(subnode, src) {
> +		const char *name = ofnode_get_name(subnode);
> +
> +		if (!name)
> +			return -EINVAL;
> +		offs = fdt_add_subnode(dst_blob, dst_offset, name);
> +		if (offs < 0)
> +			return offs;
> +		ret = cpuidle_dt_fixup_copy_node(subnode, offs, dst_blob, count, phandle);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +static int cpuidle_dt_fixup(void *blob)
> +{
> +	ofnode idle_states_node;
> +	ofnode subnode;
> +	u32 count, phandle;
> +	const struct property *prop;
> +	int offs, len, ret;
> +
> +	idle_states_node = ofnode_path("/cpus/idle-states");
> +	if (!ofnode_valid(idle_states_node)) {
> +		log_err("No idle-states node in old fdt, nothing to do");
> +		return 0;
> +	}
> +
> +	/*
> +	 * Do not proceed if the target dt already has an idle-states node.
> +	 * In this case assume that the system knows better somehow,
> +	 * so do not interfere.
> +	 */
> +	if (fdt_path_offset(blob, "/cpus/idle-states") >= 0) {
> +		log_err("idle-states node already exists in target");
> +		return 0;
> +	}
> +
> +	offs = fdt_path_offset(blob, "/cpus");
> +	if (offs < 0)
> +		return offs;
> +
> +	offs = fdt_add_subnode(blob, offs, "idle-states");
> +	if (offs < 0)
> +		return offs;
> +
> +	/* copy "/cpus/idle-states" node to destination fdt */
> +	ret = fdt_find_max_phandle(blob, &phandle);
> +	if (ret < 0)
> +		return ret;
> +	phandle++;
> +	count = 0;
> +	ret =  cpuidle_dt_fixup_copy_node(idle_states_node, offs, blob, &count, &phandle);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* copy "cpu-idle-state" property for all cpus */
> +	ofnode_for_each_subnode(subnode, ofnode_path("/cpus")) {
> +		char path[32];
> +		fdt32_t *value;
> +
> +		prop = ofnode_get_property(subnode, "cpu-idle-states", &len);
> +		if (!prop)
> +			continue;
> +
> +		/* find the same node in a new device-tree */
> +		ret = ofnode_get_path(subnode, path, sizeof(path));
> +		if (ret)
> +			return ret;
> +
> +		offs = fdt_path_offset(blob, path);
> +		if (offs < 0)
> +			return offs;
> +
> +		/* Allocate space for the list of phandles. */
> +		ret = fdt_setprop_placeholder(blob, offs, "cpu-idle-states",
> +					      count * sizeof(phandle),
> +					      (void **)&value);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Fill in the phandles of the idle state nodes. */
> +		for (u32 i = 0U; i < count; ++i)
> +			value[i] = cpu_to_fdt32(phandle - count + i);
> +	}
> +	return 0;
> +}
> +
>  static void bluetooth_dt_fixup(void *blob)
>  {
>  	/* Some devices ship with a Bluetooth controller default address.
> @@ -914,6 +1033,7 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>  	setup_environment(blob);
>  	fdt_fixup_ethernet(blob);
>  
> +	cpuidle_dt_fixup(blob);
>  	bluetooth_dt_fixup(blob);
>  
>  #ifdef CONFIG_VIDEO_DT_SIMPLEFB
Andrey Skvortsov Sept. 5, 2023, 8:37 a.m. UTC | #2
Hi Andrey,

On 23-09-05 09:27, Andre Przywara wrote:
> On Mon,  4 Sep 2023 23:54:30 +0300
> Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> 
> Hi Andrey,
> 
> > When using SCPI as the PSCI backend, firmware can wake up the CPUs and
> > cluster from sleep, so CPU idle states are available for loaded OS to
> > use. TF-A modifies DTB to advertise available CPU idle states, when
> > SCPI is detected. This change copies nodes added by TF-A to any new
> > dtb that is used for loaded OS.
> 
> Why do you need that, exactly? Why not just use $fdtcontroladdr for the
> kernel? We now keep the U-Boot copy of the .dts files in sync with the
> kernel. If you need to modify the DT in U-Boot, for instance by applying
> overlays, you can copy that DTB into a better suitable location first:
> => fdt move $fdtcontroladdr $fdt_addr_r
> 
> In any case, there shall be only one DT, that one in the U-Boot image. Why
> do you need to load another one for the kernel?

extlinux is used by distributions (sometimes with device-specific changes especially
for platforms not fully supported by mainline yet), then U-Boot loads DT defined in
extlinux.conf file. u-boot scripts are not used in case of extlinux at all.
Andre Przywara Sept. 6, 2023, 12:12 a.m. UTC | #3
On Tue, 5 Sep 2023 11:37:31 +0300
Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:

Hi,

> On 23-09-05 09:27, Andre Przywara wrote:
> > On Mon,  4 Sep 2023 23:54:30 +0300
> > Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> > 
> > Hi Andrey,
> >   
> > > When using SCPI as the PSCI backend, firmware can wake up the CPUs and
> > > cluster from sleep, so CPU idle states are available for loaded OS to
> > > use. TF-A modifies DTB to advertise available CPU idle states, when
> > > SCPI is detected. This change copies nodes added by TF-A to any new
> > > dtb that is used for loaded OS.  
> > 
> > Why do you need that, exactly? Why not just use $fdtcontroladdr for the
> > kernel? We now keep the U-Boot copy of the .dts files in sync with the
> > kernel. If you need to modify the DT in U-Boot, for instance by applying
> > overlays, you can copy that DTB into a better suitable location first:  
> > => fdt move $fdtcontroladdr $fdt_addr_r  
> > 
> > In any case, there shall be only one DT, that one in the U-Boot image. Why
> > do you need to load another one for the kernel?  
> 
> extlinux is used by distributions (sometimes with device-specific changes especially

What distros are that? I guess some special ones, targeting embedded
devices, like the Pinephone?
And who is generating extlinux.conf then? Is that some distro specific
scripting, similar to how grub is configured?
Honest questions, I am not a user of extlinux, I mostly use UEFI
booting, or type U-Boot commands directly for experiments, or use
boot.scr, as a quick-and-dirty hack.

> for platforms not fully supported by mainline yet),

Do you need any changes to the DT? Do you need to apply overlays?
If you run on a non-mainlined platform, you could still put your DT
into the U-Boot tree, then you wouldn't need to load another DTB, which
also simplifies the deployment on the kernel/distro side.

> then U-Boot loads DT defined in
> extlinux.conf file. u-boot scripts are not used in case of extlinux at all.

That's fine, you don't need any U-Boot scripts for this to work. If
there is no "fdt" or "fdtdir" label in extlinux.conf, then the U-Boot
PXE code will eventually fall back to $fdtcontroladdr - I just tested
that.
So could you make that work for you? I guess all you need to change is
to remove any fdtdir label from extlinux.conf?

Cheers,
Andre
Andrey Skvortsov Sept. 6, 2023, 8:53 p.m. UTC | #4
Hi Andre,

On 23-09-06 01:12, Andre Przywara wrote:
> On Tue, 5 Sep 2023 11:37:31 +0300
> Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> 
> Hi,
> 
> > On 23-09-05 09:27, Andre Przywara wrote:
> > > On Mon,  4 Sep 2023 23:54:30 +0300
> > > Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> > > 
> > > Hi Andrey,
> > >   
> > > > When using SCPI as the PSCI backend, firmware can wake up the CPUs and
> > > > cluster from sleep, so CPU idle states are available for loaded OS to
> > > > use. TF-A modifies DTB to advertise available CPU idle states, when
> > > > SCPI is detected. This change copies nodes added by TF-A to any new
> > > > dtb that is used for loaded OS.  
> > > 
> > > Why do you need that, exactly? Why not just use $fdtcontroladdr for the
> > > kernel? We now keep the U-Boot copy of the .dts files in sync with the
> > > kernel. If you need to modify the DT in U-Boot, for instance by applying
> > > overlays, you can copy that DTB into a better suitable location first:  
> > > => fdt move $fdtcontroladdr $fdt_addr_r  
> > > 
> > > In any case, there shall be only one DT, that one in the U-Boot image. Why
> > > do you need to load another one for the kernel?  
> > 
> > extlinux is used by distributions (sometimes with device-specific changes especially
> 
> What distros are that? I guess some special ones, targeting embedded
> devices, like the Pinephone?

It's more likely universal operating system. In my particular case
this is Mobian (several device-specific packages on top of
Debian). It's very-very close to Debian with a goal to be
pure Debian in the near future.

Rhino Linux is using extlinux as well and will benefit for this change
as well.

> And who is generating extlinux.conf then? Is that some distro specific
> scripting, similar to how grub is configured?

extlinux.conf is automatically generated by standard Debian
u-boot-menu package. See [1] and [2]. 

> Honest questions, I am not a user of extlinux, I mostly use UEFI
> booting, or type U-Boot commands directly for experiments, or use
> boot.scr, as a quick-and-dirty hack.


> > for platforms not fully supported by mainline yet),
> 
> Do you need any changes to the DT? Do you need to apply overlays?
> If you run on a non-mainlined platform, you could still put your DT
> into the U-Boot tree, then you wouldn't need to load another DTB, which
> also simplifies the deployment on the kernel/distro side.

Currently Mobian linux kernel for sunxi-devices contains 36 extra patches with DT
modifications (add/remove nodes, modify existing properties). One of
them unconditionally adds cpuidle states to DT, that I'm trying to fix
upstream here with the proposed change.
DT overlays are not used.

Honestly I don't think that using dtb from u-boot will simplify
distribution deployment and maintenance. IMHO, it will make things
more complex. Currently on my device there are records in extlinux.conf
for 5.9, 5.10, 5.15, several 6.1 kernels, that I'm working on. All of
them have slightly (or sometimes more) different device-trees. Having
kernel and dtb deployed together makes life so much easier.

Mobian's dtb can't be put into u-boot. Mobian doesn't provide
device-specific bootloader (u-boot) and there is on-going work on
making device-independent universal images. 
As many other mobile Linux distributions Mobian relies on
Tow-Boot. That is used as a cross-distribution firmware (u-boot/tf-a/crust). [3]

> > then U-Boot loads DT defined in
> > extlinux.conf file. u-boot scripts are not used in case of extlinux at all.
> 
> That's fine, you don't need any U-Boot scripts for this to work. If
> there is no "fdt" or "fdtdir" label in extlinux.conf, then the U-Boot
> PXE code will eventually fall back to $fdtcontroladdr - I just tested
> that.
> So could you make that work for you? I guess all you need to change is
> to remove any fdtdir label from extlinux.conf?
> 
> Cheers,
> Andre

Regardless proposed change. Changes to dtb nodes already copied in
u-boot on the fly in boot/image-fdt.c:image_setup_libfdt. For example,
created optee nodes are copied there. I've just put platform specific
changes into platform specific ft_board_setup, that was made
apparently exactly for that.


1. https://packages.debian.org/trixie/u-boot-menu
2. https://salsa.debian.org/debian/u-boot-menu
3. https://tow-boot.org/
Andre Przywara Sept. 11, 2023, 10:15 p.m. UTC | #5
On Wed, 6 Sep 2023 23:53:51 +0300
Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:

Hi Andrey,

we seem to have a very different approach to the whole booting process,
which makes this conversation quite involved.

I understand how things have been traditionally handled in the past, in
the embedded world (DTBs bundled with the kernel, distros installing
boot firmware), but we should really move on from this ill-fated
approach. For a while now we try hard to keep the DTs compatible across
several kernel version, making only compatible changes. In particular I
have been deliberately keeping compatibility with pre-v5.13 kernels in
the U-Boot copy of the Allwinner DTs, so you can always take the latest
version and boot that with a wide range of kernels.

> On 23-09-06 01:12, Andre Przywara wrote:
> > On Tue, 5 Sep 2023 11:37:31 +0300
> > Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> > 
> > Hi,
> >   
> > > On 23-09-05 09:27, Andre Przywara wrote:  
> > > > On Mon,  4 Sep 2023 23:54:30 +0300
> > > > Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> > > > 
> > > > Hi Andrey,
> > > >     
> > > > > When using SCPI as the PSCI backend, firmware can wake up the CPUs and
> > > > > cluster from sleep, so CPU idle states are available for loaded OS to
> > > > > use. TF-A modifies DTB to advertise available CPU idle states, when
> > > > > SCPI is detected. This change copies nodes added by TF-A to any new
> > > > > dtb that is used for loaded OS.    
> > > > 
> > > > Why do you need that, exactly? Why not just use $fdtcontroladdr for the
> > > > kernel? We now keep the U-Boot copy of the .dts files in sync with the
> > > > kernel. If you need to modify the DT in U-Boot, for instance by applying
> > > > overlays, you can copy that DTB into a better suitable location first:    
> > > > => fdt move $fdtcontroladdr $fdt_addr_r    
> > > > 
> > > > In any case, there shall be only one DT, that one in the U-Boot image. Why
> > > > do you need to load another one for the kernel?    
> > > 
> > > extlinux is used by distributions (sometimes with device-specific changes especially  
> > 
> > What distros are that? I guess some special ones, targeting embedded
> > devices, like the Pinephone?  
> 
> It's more likely universal operating system. In my particular case
> this is Mobian (several device-specific packages on top of
> Debian). It's very-very close to Debian with a goal to be
> pure Debian in the near future.
> 
> Rhino Linux is using extlinux as well and will benefit for this change
> as well.

We seem to have a very different understanding of "universal operating
systems" ;-)
You seem to talk about specific distros for embedded devices, whereas I
think of Ubuntu/Debian/Fedora/Arch/you-name-it.

And I still don't understand what a particular distribution has to do
with that: this is purely a question of boot firmware and who provides
the DTB.

> > And who is generating extlinux.conf then? Is that some distro specific
> > scripting, similar to how grub is configured?  
> 
> extlinux.conf is automatically generated by standard Debian
> u-boot-menu package. See [1] and [2]. 

Ah, thanks, I was looking for the extlinux package, which only seems to
exist for x86.

> > Honest questions, I am not a user of extlinux, I mostly use UEFI
> > booting, or type U-Boot commands directly for experiments, or use
> > boot.scr, as a quick-and-dirty hack.  
> 
> 
> > > for platforms not fully supported by mainline yet),  
> > 
> > Do you need any changes to the DT? Do you need to apply overlays?
> > If you run on a non-mainlined platform, you could still put your DT
> > into the U-Boot tree, then you wouldn't need to load another DTB, which
> > also simplifies the deployment on the kernel/distro side.  
> 
> Currently Mobian linux kernel for sunxi-devices contains 36 extra patches with DT

That does not sound good. Why are those patches not upstream?

> modifications (add/remove nodes, modify existing properties). One of
> them unconditionally adds cpuidle states to DT, that I'm trying to fix
> upstream here with the proposed change.

That's very honourable, but not every patch that works(TM) is the right
approach to take upstream.

> DT overlays are not used.
> 
> Honestly I don't think that using dtb from u-boot will simplify
> distribution deployment and maintenance. IMHO, it will make things
> more complex. Currently on my device there are records in extlinux.conf
> for 5.9, 5.10, 5.15, several 6.1 kernels, that I'm working on. All of
> them have slightly (or sometimes more) different device-trees.

And why is that, exactly? The DT describes the *device*, so why do you
have different DTs for different kernels? Sure, they evolve, devices
get added and we will have fixes, but conceptually there should be one
best DT for each device, for every kernel, and that could be the one
in the U-Boot tree - or the one in the latest kernel *tree*.

And why do you have different kernels to begin with? Are those various
vendor kernels, for different SoCs, each with their own pile of
downstream patches?

> Having
> kernel and dtb deployed together makes life so much easier.

Easier in the long term, because you can make quick hacks, but it's all
getting a mess over time.

> Mobian's dtb can't be put into u-boot.

Why not? It's a matter of copying whatever .dts files you have into
U-Boot's arch/arm/dts directory and making sure the Makefile covers
every file. Ignoring the question what "Mobian's DTB" really is.

> Mobian doesn't provide
> device-specific bootloader (u-boot) and there is on-going work on
> making device-independent universal images.

With "universal image" you mean a single rootfs, and a single kernel for
every device? Of course, this is how Linux works everywhere, and the
arm64 kernel was a single-image kernel from day one (same as x86). And
taking the DTs out there and into firmware is just even more
consequential.

> As many other mobile Linux distributions Mobian relies on
> Tow-Boot. That is used as a cross-distribution firmware (u-boot/tf-a/crust). [3]

I don't know much about tow-boot, but it seems just like a U-Boot
*distribution*, so I don't see why you can't use upstream U-Boot
techniques like sharing U-Boot's control DTB with the kernel
(by using $fdtcontroladdr).

> > > then U-Boot loads DT defined in
> > > extlinux.conf file. u-boot scripts are not used in case of extlinux at all.  
> > 
> > That's fine, you don't need any U-Boot scripts for this to work. If
> > there is no "fdt" or "fdtdir" label in extlinux.conf, then the U-Boot
> > PXE code will eventually fall back to $fdtcontroladdr - I just tested
> > that.
> > So could you make that work for you? I guess all you need to change is
> > to remove any fdtdir label from extlinux.conf?
> > 
> > Cheers,
> > Andre  
> 
> Regardless proposed change. Changes to dtb nodes already copied in
> u-boot on the fly in boot/image-fdt.c:image_setup_libfdt. For example,
> created optee nodes are copied there.

Yes, that is an example, just not a particularly good one. This seems
to originate from 2019, again building on the idea that you need to load
a DTB for the kernel from somewhere. I don't like to proliferate those
ideas anymore.

> I've just put platform specific
> changes into platform specific ft_board_setup, that was made
> apparently exactly for that.

Not really, that's for platform specific runtime *adjustments*, like
adding a unique MAC address, or adding simplefb DT nodes.

Cheers,
Andre

> 1. https://packages.debian.org/trixie/u-boot-menu
> 2. https://salsa.debian.org/debian/u-boot-menu
> 3. https://tow-boot.org/
>
Andrey Skvortsov Sept. 14, 2023, 8:22 p.m. UTC | #6
Hi Andre,

On 23-09-11 23:15, Andre Przywara wrote:
> On Wed, 6 Sep 2023 23:53:51 +0300
> Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> 
> > > > > > When using SCPI as the PSCI backend, firmware can wake up the CPUs and
> > > > > > cluster from sleep, so CPU idle states are available for loaded OS to
> > > > > > use. TF-A modifies DTB to advertise available CPU idle states, when
> > > > > > SCPI is detected. This change copies nodes added by TF-A to any new
> > > > > > dtb that is used for loaded OS.    
> > > > > 
> > > > > Why do you need that, exactly? Why not just use $fdtcontroladdr for the
> > > > > kernel? We now keep the U-Boot copy of the .dts files in sync with the
> > > > > kernel. If you need to modify the DT in U-Boot, for instance by applying
> > > > > overlays, you can copy that DTB into a better suitable location first:    
> > > > > => fdt move $fdtcontroladdr $fdt_addr_r    
> > > > > 
> > > > > In any case, there shall be only one DT, that one in the U-Boot image. Why
> > > > > do you need to load another one for the kernel?    
> > > > 
> > > > extlinux is used by distributions (sometimes with device-specific changes especially  
> > > 
> > > What distros are that? I guess some special ones, targeting embedded
> > > devices, like the Pinephone?  
> > 
> > It's more likely universal operating system. In my particular case
> > this is Mobian (several device-specific packages on top of
> > Debian). It's very-very close to Debian with a goal to be
> > pure Debian in the near future.
> > 
> > Rhino Linux is using extlinux as well and will benefit for this change
> > as well.
> 
> We seem to have a very different understanding of "universal operating
> systems" ;-)
> You seem to talk about specific distros for embedded devices, whereas I
> think of Ubuntu/Debian/Fedora/Arch/you-name-it.

I was talking about Debian (The universal operating system ;-)
initially. Even if PinePhone in my case runs Mobian,
it uses the same packages and scripts and repositories from Debian. The
difference is quite small (kernel and some other small changes).

> And I still don't understand what a particular distribution has to do
> with that: this is purely a question of boot firmware and who provides
> the DTB.
> > 
> > > > for platforms not fully supported by mainline yet),  
> > > 
> > > Do you need any changes to the DT? Do you need to apply overlays?
> > > If you run on a non-mainlined platform, you could still put your DT
> > > into the U-Boot tree, then you wouldn't need to load another DTB, which
> > > also simplifies the deployment on the kernel/distro side.  
> > 
> > Currently Mobian linux kernel for sunxi-devices contains 36 extra patches with DT
> 
> That does not sound good. Why are those patches not upstream?

Agree, I ask the same question. Some of the patches were posted for
review and stalled, some of them haven't been posted. I'm preparing
table to track current status of the patches. Maybe ping some of them.

There are some big and complex changes like wlan/bt driver, that it's hard to
upstream in the current shape.

> > modifications (add/remove nodes, modify existing properties). One of
> > them unconditionally adds cpuidle states to DT, that I'm trying to fix
> > upstream here with the proposed change.
> 
> That's very honourable, but not every patch that works(TM) is the right
> approach to take upstream.

Sure, this one will probably stay in our patches until all DT changes
are upstream.

> > Mobian doesn't provide
> > device-specific bootloader (u-boot) and there is on-going work on
> > making device-independent universal images.
> 
> With "universal image" you mean a single rootfs, and a single kernel for
> every device? Of course, this is how Linux works everywhere, and the
> arm64 kernel was a single-image kernel from day one (same as x86). And
> taking the DTs out there and into firmware is just even more
> consequential.
> 

> > 
> > Regardless proposed change. Changes to dtb nodes already copied in
> > u-boot on the fly in boot/image-fdt.c:image_setup_libfdt. For example,
> > created optee nodes are copied there.
> 
> Yes, that is an example, just not a particularly good one. This seems
> to originate from 2019, again building on the idea that you need to load
> a DTB for the kernel from somewhere. I don't like to proliferate those
> ideas anymore.
> 
> > I've just put platform specific
> > changes into platform specific ft_board_setup, that was made
> > apparently exactly for that.
> 
> Not really, that's for platform specific runtime *adjustments*, like
> adding a unique MAC address, or adding simplefb DT nodes.

Thanks again for taking time and explaining your position. I agree, that in
a long term dtb should be provided by u-boot. In that case proposed
change will not be needed. Hopefully we'll get all changes upstream
and could use $fdtcontroladdr some day.
diff mbox series

Patch

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index f321cd58a6..e88bd11a99 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -870,6 +870,125 @@  int board_late_init(void)
 	return 0;
 }
 
+static int cpuidle_dt_fixup_copy_node(ofnode src, int dst_offset, void *dst_blob,
+				      u32 *count, u32 *phandle)
+{
+	int offs, len, ret;
+	struct ofprop prop;
+	ofnode subnode;
+
+	ofnode_for_each_prop(prop, src) {
+		const char *name;
+		const char *val;
+
+		val = ofprop_get_property(&prop, &name, &len);
+		if (!val)
+			return -EINVAL;
+		if (!strcmp(name, "phandle")) {
+			if (ofnode_device_is_compatible(src, "arm,idle-state")) {
+				fdt_setprop_u32(dst_blob, dst_offset, name, *phandle);
+				(*phandle)++;
+				(*count)++;
+			} else {
+				log_err("Unexpected phandle node: %s\n", name);
+				return -EINVAL;
+			}
+		} else {
+			ret = fdt_setprop(dst_blob, dst_offset, name, val, len);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	ofnode_for_each_subnode(subnode, src) {
+		const char *name = ofnode_get_name(subnode);
+
+		if (!name)
+			return -EINVAL;
+		offs = fdt_add_subnode(dst_blob, dst_offset, name);
+		if (offs < 0)
+			return offs;
+		ret = cpuidle_dt_fixup_copy_node(subnode, offs, dst_blob, count, phandle);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
+static int cpuidle_dt_fixup(void *blob)
+{
+	ofnode idle_states_node;
+	ofnode subnode;
+	u32 count, phandle;
+	const struct property *prop;
+	int offs, len, ret;
+
+	idle_states_node = ofnode_path("/cpus/idle-states");
+	if (!ofnode_valid(idle_states_node)) {
+		log_err("No idle-states node in old fdt, nothing to do");
+		return 0;
+	}
+
+	/*
+	 * Do not proceed if the target dt already has an idle-states node.
+	 * In this case assume that the system knows better somehow,
+	 * so do not interfere.
+	 */
+	if (fdt_path_offset(blob, "/cpus/idle-states") >= 0) {
+		log_err("idle-states node already exists in target");
+		return 0;
+	}
+
+	offs = fdt_path_offset(blob, "/cpus");
+	if (offs < 0)
+		return offs;
+
+	offs = fdt_add_subnode(blob, offs, "idle-states");
+	if (offs < 0)
+		return offs;
+
+	/* copy "/cpus/idle-states" node to destination fdt */
+	ret = fdt_find_max_phandle(blob, &phandle);
+	if (ret < 0)
+		return ret;
+	phandle++;
+	count = 0;
+	ret =  cpuidle_dt_fixup_copy_node(idle_states_node, offs, blob, &count, &phandle);
+	if (ret < 0)
+		return ret;
+
+	/* copy "cpu-idle-state" property for all cpus */
+	ofnode_for_each_subnode(subnode, ofnode_path("/cpus")) {
+		char path[32];
+		fdt32_t *value;
+
+		prop = ofnode_get_property(subnode, "cpu-idle-states", &len);
+		if (!prop)
+			continue;
+
+		/* find the same node in a new device-tree */
+		ret = ofnode_get_path(subnode, path, sizeof(path));
+		if (ret)
+			return ret;
+
+		offs = fdt_path_offset(blob, path);
+		if (offs < 0)
+			return offs;
+
+		/* Allocate space for the list of phandles. */
+		ret = fdt_setprop_placeholder(blob, offs, "cpu-idle-states",
+					      count * sizeof(phandle),
+					      (void **)&value);
+		if (ret < 0)
+			return ret;
+
+		/* Fill in the phandles of the idle state nodes. */
+		for (u32 i = 0U; i < count; ++i)
+			value[i] = cpu_to_fdt32(phandle - count + i);
+	}
+	return 0;
+}
+
 static void bluetooth_dt_fixup(void *blob)
 {
 	/* Some devices ship with a Bluetooth controller default address.
@@ -914,6 +1033,7 @@  int ft_board_setup(void *blob, struct bd_info *bd)
 	setup_environment(blob);
 	fdt_fixup_ethernet(blob);
 
+	cpuidle_dt_fixup(blob);
 	bluetooth_dt_fixup(blob);
 
 #ifdef CONFIG_VIDEO_DT_SIMPLEFB