Re: [PATCH v2] arm: dts: k3-am625-beagleplay: Package TIFS Stub
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
> > > Signed-off-by: Dhruva Gole
> > > ---
> > >
> > > 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-g...@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 = <_gp>;
> > > + core = "secure";
> > > + load = <0x6>;
> > > + sw-rev = ;
> > > + 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 = <0x9dc0>;
> > > + entry = <0x9dc0>;
> >
> > 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
Re: [PATCH v2] arm: dts: k3-am625-beagleplay: Package TIFS Stub
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_keywords=yes=default
> I see no mention of the same?
I agree, documentation is lacking, will be sure to add that.
> >
> > Acked-by: Neha Malcom Francis
> > Signed-off-by: Dhruva Gole
> > ---
> >
> > 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-g...@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 = <_gp>;
> > + core = "secure";
> > + load = <0x6>;
> > + sw-rev = ;
> > + 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 = <0x9dc0>;
> > + entry = <0x9dc0>;
>
> 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.
--
Best regards,
Dhruva Gole
Re: [PATCH v2] arm: dts: k3-am625-beagleplay: Package TIFS Stub
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
> > Signed-off-by: Dhruva Gole
> > ---
> >
> > 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-g...@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 = <_gp>;
> > + core = "secure";
> > + load = <0x6>;
> > + sw-rev = ;
> > + 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 = <0x9dc0>;
> > + entry = <0x9dc0>;
>
> 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?
--
Best regards,
Dhruva Gole
Re: [PATCH v2] arm: dts: k3-am625-beagleplay: Package TIFS Stub
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
> Signed-off-by: Dhruva Gole
> ---
>
> 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-g...@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 = <_gp>;
> + core = "secure";
> + load = <0x6>;
> + sw-rev = ;
> + 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 = <0x9dc0>;
> + entry = <0x9dc0>;
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
>
Re: [PATCH v2] arm: dts: k3-am625-beagleplay: Package TIFS Stub
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_keywords=yes=default
I see no mention of the same?
>
> Acked-by: Neha Malcom Francis
> Signed-off-by: Dhruva Gole
> ---
>
> 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-g...@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 = <_gp>;
> + core = "secure";
> + load = <0x6>;
> + sw-rev = ;
> + 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 = <0x9dc0>;
> + entry = <0x9dc0>;
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
>
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5
849D 1736 249D
