| Message ID | 20240618045610.271884-1-d-gole@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2] arm: dts: k3-am625-beagleplay: Package TIFS Stub | expand |
On 10:26-20240618, Dhruva Gole wrote: > Add support for packaging the TIFS Stub as it's required for basic Low > Power Modes like Deep Sleep. What the heck is tifs stub? https://docs.u-boot.org/en/latest/search.html?q=tifs&check_keywords=yes&area=default I see no mention of the same? > > Acked-by: Neha Malcom Francis <n-francis@ti.com> > Signed-off-by: Dhruva Gole <d-gole@ti.com> > --- > > No changes from v1, just picked Neha's ack and rebased on master again. > Link to v1: > https://lore.kernel.org/u-boot/20240612062351.3690091-1-d-gole@ti.com/ > > arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 33 +++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > index fb2032068d1c..5e2248a4a668 100644 > --- a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > +++ b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > @@ -69,6 +69,23 @@ > }; > }; > > + tifsstub-gp { > + filename = "tifsstub.bin_gp"; > + ti-secure-rom { > + content = <&tifsstub_gp>; > + core = "secure"; > + load = <0x60000>; > + sw-rev = <CONFIG_K3_X509_SWRV>; > + keyfile = "ti-degenerate-key.pem"; > + tifsstub; > + }; > + tifsstub_gp: tifsstub-gp.bin { > + filename = "ti-sysfw/ti-fs-stub-firmware-am62x-gp.bin"; > + type = "blob-ext"; > + optional; > + }; > + }; > + > ti-spl_unsigned { > filename = "tispl.bin_unsigned"; > pad-byte = <0xff>; > @@ -105,6 +122,19 @@ > }; > }; > > + tifsstub-gp { > + description = "tifsstub"; > + type = "firmware"; > + arch = "arm32"; > + compression = "none"; > + os = "tifsstub-gp"; > + load = <0x9dc00000>; > + entry = <0x9dc00000>; two issues with this: a) if the tifsstub-gp is not automatically consumed by tifs by the time u-boot is up or kernel is up, this is going to get clobbered by OS https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts#n78 Should be updated before this is done. b) Documentation update - please always make sure you update documentation when doing this kind of change https://docs.u-boot.org/en/latest/board/beagle/am62x_beagleplay.html#image-formats > + blob-ext { > + filename = "tifsstub.bin_gp"; > + }; > + }; > + > dm { > description = "DM binary"; > type = "firmware"; > @@ -148,7 +178,8 @@ > conf-0 { > description = "k3-am625-beagleplay"; > firmware = "atf"; > - loadables = "tee", "dm", "spl"; > + loadables = "tee", "dm", "spl", > + "tifsstub-gp"; > fdt = "fdt-0"; > }; > }; > > base-commit: 16324b43db3f2b4fbbc3b701893fcfc4104f33fb > -- > 2.34.1 >
On June 18, 2024 thus sayeth Dhruva Gole: > Add support for packaging the TIFS Stub as it's required for basic Low > Power Modes like Deep Sleep. > > Acked-by: Neha Malcom Francis <n-francis@ti.com> > Signed-off-by: Dhruva Gole <d-gole@ti.com> > --- > > No changes from v1, just picked Neha's ack and rebased on master again. > Link to v1: > https://lore.kernel.org/u-boot/20240612062351.3690091-1-d-gole@ti.com/ > > arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 33 +++++++++++++++++++- > 1 file changed, 32 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > index fb2032068d1c..5e2248a4a668 100644 > --- a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > +++ b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > @@ -69,6 +69,23 @@ > }; > }; > > + tifsstub-gp { > + filename = "tifsstub.bin_gp"; > + ti-secure-rom { > + content = <&tifsstub_gp>; > + core = "secure"; > + load = <0x60000>; > + sw-rev = <CONFIG_K3_X509_SWRV>; > + keyfile = "ti-degenerate-key.pem"; > + tifsstub; > + }; > + tifsstub_gp: tifsstub-gp.bin { > + filename = "ti-sysfw/ti-fs-stub-firmware-am62x-gp.bin"; > + type = "blob-ext"; > + optional; > + }; > + }; > + > ti-spl_unsigned { > filename = "tispl.bin_unsigned"; > pad-byte = <0xff>; > @@ -105,6 +122,19 @@ > }; > }; > > + tifsstub-gp { > + description = "tifsstub"; > + type = "firmware"; > + arch = "arm32"; > + compression = "none"; > + os = "tifsstub-gp"; > + load = <0x9dc00000>; > + entry = <0x9dc00000>; Is this stub position independent? Or is this address compiled in at build time? We have some variants of 62x that don't have these addresses backed by DRAM. ~Bryan > + blob-ext { > + filename = "tifsstub.bin_gp"; > + }; > + }; > + > dm { > description = "DM binary"; > type = "firmware"; > @@ -148,7 +178,8 @@ > conf-0 { > description = "k3-am625-beagleplay"; > firmware = "atf"; > - loadables = "tee", "dm", "spl"; > + loadables = "tee", "dm", "spl", > + "tifsstub-gp"; > fdt = "fdt-0"; > }; > }; > > base-commit: 16324b43db3f2b4fbbc3b701893fcfc4104f33fb > -- > 2.34.1 >
Bryan, On Jun 19, 2024 at 16:44:48 -0500, Bryan Brattlof wrote: > On June 18, 2024 thus sayeth Dhruva Gole: > > Add support for packaging the TIFS Stub as it's required for basic Low > > Power Modes like Deep Sleep. > > > > Acked-by: Neha Malcom Francis <n-francis@ti.com> > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > > --- > > > > No changes from v1, just picked Neha's ack and rebased on master again. > > Link to v1: > > https://lore.kernel.org/u-boot/20240612062351.3690091-1-d-gole@ti.com/ > > > > arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 33 +++++++++++++++++++- > > 1 file changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > index fb2032068d1c..5e2248a4a668 100644 > > --- a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > +++ b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > @@ -69,6 +69,23 @@ > > }; > > }; > > > > + tifsstub-gp { > > + filename = "tifsstub.bin_gp"; > > + ti-secure-rom { > > + content = <&tifsstub_gp>; > > + core = "secure"; > > + load = <0x60000>; > > + sw-rev = <CONFIG_K3_X509_SWRV>; > > + keyfile = "ti-degenerate-key.pem"; > > + tifsstub; > > + }; > > + tifsstub_gp: tifsstub-gp.bin { > > + filename = "ti-sysfw/ti-fs-stub-firmware-am62x-gp.bin"; > > + type = "blob-ext"; > > + optional; > > + }; > > + }; > > + > > ti-spl_unsigned { > > filename = "tispl.bin_unsigned"; > > pad-byte = <0xff>; > > @@ -105,6 +122,19 @@ > > }; > > }; > > > > + tifsstub-gp { > > + description = "tifsstub"; > > + type = "firmware"; > > + arch = "arm32"; > > + compression = "none"; > > + os = "tifsstub-gp"; > > + load = <0x9dc00000>; > > + entry = <0x9dc00000>; > > Is this stub position independent? Or is this address compiled in at > build time? We have some variants of 62x that don't have these addresses > backed by DRAM. The stub is not position independent and is indeed fixed at build time. Can you talk more about these am62x variants that don't have the address backed by DRAM? What would be the suggested address range in those cases? For BeaglePlay atleast I don't see an issue. So I don't think we'd have issues with this patch in particular?
Nishanth, On Jun 19, 2024 at 13:47:36 -0500, Nishanth Menon wrote: > On 10:26-20240618, Dhruva Gole wrote: > > Add support for packaging the TIFS Stub as it's required for basic Low > > Power Modes like Deep Sleep. > > What the heck is tifs stub? > https://docs.u-boot.org/en/latest/search.html?q=tifs&check_keywords=yes&area=default > I see no mention of the same? I agree, documentation is lacking, will be sure to add that. > > > > Acked-by: Neha Malcom Francis <n-francis@ti.com> > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > > --- > > > > No changes from v1, just picked Neha's ack and rebased on master again. > > Link to v1: > > https://lore.kernel.org/u-boot/20240612062351.3690091-1-d-gole@ti.com/ > > > > arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 33 +++++++++++++++++++- > > 1 file changed, 32 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > index fb2032068d1c..5e2248a4a668 100644 > > --- a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > +++ b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > @@ -69,6 +69,23 @@ > > }; > > }; > > > > + tifsstub-gp { > > + filename = "tifsstub.bin_gp"; > > + ti-secure-rom { > > + content = <&tifsstub_gp>; > > + core = "secure"; > > + load = <0x60000>; > > + sw-rev = <CONFIG_K3_X509_SWRV>; > > + keyfile = "ti-degenerate-key.pem"; > > + tifsstub; > > + }; > > + tifsstub_gp: tifsstub-gp.bin { > > + filename = "ti-sysfw/ti-fs-stub-firmware-am62x-gp.bin"; > > + type = "blob-ext"; > > + optional; > > + }; > > + }; > > + > > ti-spl_unsigned { > > filename = "tispl.bin_unsigned"; > > pad-byte = <0xff>; > > @@ -105,6 +122,19 @@ > > }; > > }; > > > > + tifsstub-gp { > > + description = "tifsstub"; > > + type = "firmware"; > > + arch = "arm32"; > > + compression = "none"; > > + os = "tifsstub-gp"; > > + load = <0x9dc00000>; > > + entry = <0x9dc00000>; > > two issues with this: > a) if the tifsstub-gp is not automatically consumed by tifs by the time > u-boot is up or kernel is up, this is going to get clobbered by OS > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts#n78 > Should be updated before this is done. This won't be much of a concern, the TIFS Stub is loaded into the R5 ATCM as soon as the DM R5 core comes up [0] : See the Deep Sleep Exit part, it talks about this stub. [0] https://software-dl.ti.com/processor-sdk-linux/esd/AM62AX/09_01_00/exports/docs/linux/Foundational_Components/Kernel/Kernel_Drivers/Power_Management/pm_sw_arch.html > b) Documentation update - please always make sure you update > documentation when doing this kind of change > https://docs.u-boot.org/en/latest/board/beagle/am62x_beagleplay.html#image-formats Will do.
On June 21, 2024 thus sayeth Dhruva Gole: > Bryan, > > On Jun 19, 2024 at 16:44:48 -0500, Bryan Brattlof wrote: > > On June 18, 2024 thus sayeth Dhruva Gole: > > > Add support for packaging the TIFS Stub as it's required for basic Low > > > Power Modes like Deep Sleep. > > > > > > Acked-by: Neha Malcom Francis <n-francis@ti.com> > > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > > > --- > > > > > > No changes from v1, just picked Neha's ack and rebased on master again. > > > Link to v1: > > > https://lore.kernel.org/u-boot/20240612062351.3690091-1-d-gole@ti.com/ > > > > > > arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 33 +++++++++++++++++++- > > > 1 file changed, 32 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > > index fb2032068d1c..5e2248a4a668 100644 > > > --- a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > > +++ b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi > > > @@ -69,6 +69,23 @@ > > > }; > > > }; > > > > > > + tifsstub-gp { > > > + filename = "tifsstub.bin_gp"; > > > + ti-secure-rom { > > > + content = <&tifsstub_gp>; > > > + core = "secure"; > > > + load = <0x60000>; > > > + sw-rev = <CONFIG_K3_X509_SWRV>; > > > + keyfile = "ti-degenerate-key.pem"; > > > + tifsstub; > > > + }; > > > + tifsstub_gp: tifsstub-gp.bin { > > > + filename = "ti-sysfw/ti-fs-stub-firmware-am62x-gp.bin"; > > > + type = "blob-ext"; > > > + optional; > > > + }; > > > + }; > > > + > > > ti-spl_unsigned { > > > filename = "tispl.bin_unsigned"; > > > pad-byte = <0xff>; > > > @@ -105,6 +122,19 @@ > > > }; > > > }; > > > > > > + tifsstub-gp { > > > + description = "tifsstub"; > > > + type = "firmware"; > > > + arch = "arm32"; > > > + compression = "none"; > > > + os = "tifsstub-gp"; > > > + load = <0x9dc00000>; > > > + entry = <0x9dc00000>; > > > > Is this stub position independent? Or is this address compiled in at > > build time? We have some variants of 62x that don't have these addresses > > backed by DRAM. > > The stub is not position independent and is indeed fixed at build time. > Can you talk more about these am62x variants that don't have the address > backed by DRAM? What would be the suggested address range in those > cases? > We've always had a 'soft' limit of 512MB so you're correct this isn't an issue today and I haven't seen any 'u-boot shall support' language internally however there have been a few questions about minimum DDR capacities we can support. Some have asked about 256MB. (This 512MB 'soft' limit is most likely why this stub and OP-TEE is where it is today) > > For BeaglePlay atleast I don't see an issue. So I don't think we'd have > issues with this patch in particular? > No you're correct however knowing that this address location is compiled into other firmware means we would need to have separate firmware builds if we wanted this stub in another location and it's awkwardly in the middle of DRAM and not at the bottom like I would prefer. So while yes this is a board/ level decision because it's also dealing with mach-k3/ level firmware this is an unfortunate thing we're hard-coding. This is essentially hard-coding a 512MB minimum limit for the entire AM62x SoC family. My question was trying to determine if we should/could add this address to Kconfig so we can pass in the address to tispl.bin here and somehow get this into board-cfg.yaml. (This will be some work for us) All of that to say, I'm okay with this change however I wish there was a 'there be dragons' trailer I could add to this ~Bryan
diff --git a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi index fb2032068d1c..5e2248a4a668 100644 --- a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi +++ b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi @@ -69,6 +69,23 @@ }; }; + tifsstub-gp { + filename = "tifsstub.bin_gp"; + ti-secure-rom { + content = <&tifsstub_gp>; + core = "secure"; + load = <0x60000>; + sw-rev = <CONFIG_K3_X509_SWRV>; + keyfile = "ti-degenerate-key.pem"; + tifsstub; + }; + tifsstub_gp: tifsstub-gp.bin { + filename = "ti-sysfw/ti-fs-stub-firmware-am62x-gp.bin"; + type = "blob-ext"; + optional; + }; + }; + ti-spl_unsigned { filename = "tispl.bin_unsigned"; pad-byte = <0xff>; @@ -105,6 +122,19 @@ }; }; + tifsstub-gp { + description = "tifsstub"; + type = "firmware"; + arch = "arm32"; + compression = "none"; + os = "tifsstub-gp"; + load = <0x9dc00000>; + entry = <0x9dc00000>; + blob-ext { + filename = "tifsstub.bin_gp"; + }; + }; + dm { description = "DM binary"; type = "firmware"; @@ -148,7 +178,8 @@ conf-0 { description = "k3-am625-beagleplay"; firmware = "atf"; - loadables = "tee", "dm", "spl"; + loadables = "tee", "dm", "spl", + "tifsstub-gp"; fdt = "fdt-0"; }; };