mbox series

[V3,0/4] Low Power Mode: Pakage TIFS Stub on BeaglePlay

Message ID 20240628093252.1864609-1-d-gole@ti.com
Headers show
Series Low Power Mode: Pakage TIFS Stub on BeaglePlay | expand

Message

Dhruva Gole June 28, 2024, 9:32 a.m. UTC
This series includes the binman related changes required to package tIFS
Stub to support Low Power Modes on BeaglePlay.
Also, based on comments from previous patch [0] documentation has been
added to describe small addition in boot flow as well as tispl image
format.

I am aware that the new boot flow image will need to be updated in
other places like am62a, am62p and even other boards that use am62x.
However, I would like to keep this series beagleplay TIFSStub specific
and so I will be sending a follow up series to update other places
seperately if that's ok.

Changelog:
* Add new image format for TISPL
* Add new changes in boot flow for am62 family of devices.

[0] https://lore.kernel.org/u-boot/20240618045610.271884-1-d-gole@ti.com/

Dhruva Gole (4):
  arm: dts: k3-am625-beagleplay: Package TIFS Stub
  doc: beagle: am62x_beagleplay: Update the boot flow to show TIFS Stub
  doc: beagle: am62x_beagleplay: Add TIFS Stub in image format
  doc: ti: k3: Add TIFS Stub documentation

 arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 33 +++++++++++++++++++-
 doc/board/beagle/am62x_beagleplay.rst        |  4 +--
 doc/board/ti/img/boot_diagram_am62.svg       |  4 +++
 doc/board/ti/img/tifsstub_dm_tispl.bin.svg   |  4 +++
 doc/board/ti/k3.rst                          |  5 +++
 5 files changed, 47 insertions(+), 3 deletions(-)
 create mode 100644 doc/board/ti/img/boot_diagram_am62.svg
 create mode 100644 doc/board/ti/img/tifsstub_dm_tispl.bin.svg


base-commit: c53b344475734d0d29f522b7b1d80c5b8204442d

Comments

Nishanth Menon June 28, 2024, 1:03 p.m. UTC | #1
On 15:02-20240628, Dhruva Gole wrote:
> This series includes the binman related changes required to package tIFS
> Stub to support Low Power Modes on BeaglePlay.
> Also, based on comments from previous patch [0] documentation has been
> added to describe small addition in boot flow as well as tispl image
> format.
> 
> I am aware that the new boot flow image will need to be updated in
> other places like am62a, am62p and even other boards that use am62x.
> However, I would like to keep this series beagleplay TIFSStub specific
> and so I will be sending a follow up series to update other places
> seperately if that's ok.
> 
> Changelog:
> * Add new image format for TISPL
> * Add new changes in boot flow for am62 family of devices.
> 
> [0] https://lore.kernel.org/u-boot/20240618045610.271884-1-d-gole@ti.com/
> 
> Dhruva Gole (4):
>   arm: dts: k3-am625-beagleplay: Package TIFS Stub
>   doc: beagle: am62x_beagleplay: Update the boot flow to show TIFS Stub
>   doc: beagle: am62x_beagleplay: Add TIFS Stub in image format
>   doc: ti: k3: Add TIFS Stub documentation
> 
>  arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 33 +++++++++++++++++++-
>  doc/board/beagle/am62x_beagleplay.rst        |  4 +--
>  doc/board/ti/img/boot_diagram_am62.svg       |  4 +++
>  doc/board/ti/img/tifsstub_dm_tispl.bin.svg   |  4 +++
>  doc/board/ti/k3.rst                          |  5 +++
>  5 files changed, 47 insertions(+), 3 deletions(-)
>  create mode 100644 doc/board/ti/img/boot_diagram_am62.svg
>  create mode 100644 doc/board/ti/img/tifsstub_dm_tispl.bin.svg


Maybe you can help clarify a bit. I understand from [1]
that you are betting on timing to keep tifs stub safe. But then, why
not plug in this firmware blob along with DM itself? that allows DM
to manage itself the way it wants to and control it's own memory map?
DM initialization itself takes a few ms, just because TFA is not
touching any part of DDR does not mean that we can assume system is
interference free, no? What if DM architecture changes such that PLL
initialization or some other long pole item is performed prior to
loading the tifs stub?

[1] https://lore.kernel.org/u-boot/20240621054337.qqjftv72ofiinlhv@dhruva/
Sebin Francis June 28, 2024, 2:09 p.m. UTC | #2
On 28/06/24 18:33, Nishanth Menon wrote:
> On 15:02-20240628, Dhruva Gole wrote:
>> This series includes the binman related changes required to package tIFS
>> Stub to support Low Power Modes on BeaglePlay.
>> Also, based on comments from previous patch [0] documentation has been
>> added to describe small addition in boot flow as well as tispl image
>> format.
>>
>> I am aware that the new boot flow image will need to be updated in
>> other places like am62a, am62p and even other boards that use am62x.
>> However, I would like to keep this series beagleplay TIFSStub specific
>> and so I will be sending a follow up series to update other places
>> seperately if that's ok.
>>
>> Changelog:
>> * Add new image format for TISPL
>> * Add new changes in boot flow for am62 family of devices.
>>
>> [0] https://lore.kernel.org/u-boot/20240618045610.271884-1-d-gole@ti.com/
>>
>> Dhruva Gole (4):
>>    arm: dts: k3-am625-beagleplay: Package TIFS Stub
>>    doc: beagle: am62x_beagleplay: Update the boot flow to show TIFS Stub
>>    doc: beagle: am62x_beagleplay: Add TIFS Stub in image format
>>    doc: ti: k3: Add TIFS Stub documentation
>>
>>   arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 33 +++++++++++++++++++-
>>   doc/board/beagle/am62x_beagleplay.rst        |  4 +--
>>   doc/board/ti/img/boot_diagram_am62.svg       |  4 +++
>>   doc/board/ti/img/tifsstub_dm_tispl.bin.svg   |  4 +++
>>   doc/board/ti/k3.rst                          |  5 +++
>>   5 files changed, 47 insertions(+), 3 deletions(-)
>>   create mode 100644 doc/board/ti/img/boot_diagram_am62.svg
>>   create mode 100644 doc/board/ti/img/tifsstub_dm_tispl.bin.svg
>
> Maybe you can help clarify a bit. I understand from [1]
> that you are betting on timing to keep tifs stub safe. But then, why
> not plug in this firmware blob along with DM itself? that allows DM
> to manage itself the way it wants to and control it's own memory map?
> DM initialization itself takes a few ms, just because TFA is not
> touching any part of DDR does not mean that we can assume system is
> interference free, no? What if DM architecture changes such that PLL
> initialization or some other long pole item is performed prior to
> loading the tifs stub?


In Linux DT the address space in which FS stub is copied is part of DM 
firmware carve-out in DDR,
so if fs stub can get corrupted then DM also can get corrupted.

Regards
Sebin

>
> [1] https://lore.kernel.org/u-boot/20240621054337.qqjftv72ofiinlhv@dhruva/
>
Nishanth Menon June 28, 2024, 5:40 p.m. UTC | #3
On 19:39-20240628, Sebin Francis wrote:
> 
> On 28/06/24 18:33, Nishanth Menon wrote:
> > On 15:02-20240628, Dhruva Gole wrote:
> > > This series includes the binman related changes required to package tIFS
> > > Stub to support Low Power Modes on BeaglePlay.
> > > Also, based on comments from previous patch [0] documentation has been
> > > added to describe small addition in boot flow as well as tispl image
> > > format.
> > > 
> > > I am aware that the new boot flow image will need to be updated in
> > > other places like am62a, am62p and even other boards that use am62x.
> > > However, I would like to keep this series beagleplay TIFSStub specific
> > > and so I will be sending a follow up series to update other places
> > > seperately if that's ok.

[...]
> > 
> > Maybe you can help clarify a bit. I understand from [1]
> > that you are betting on timing to keep tifs stub safe. But then, why
> > not plug in this firmware blob along with DM itself? that allows DM
> > to manage itself the way it wants to and control it's own memory map?
> > DM initialization itself takes a few ms, just because TFA is not
> > touching any part of DDR does not mean that we can assume system is
> > interference free, no? What if DM architecture changes such that PLL
> > initialization or some other long pole item is performed prior to
> > loading the tifs stub?
> 
> In Linux DT the address space in which FS stub is copied is part of DM
> firmware carve-out in DDR,
> so if fs stub can get corrupted then DM also can get corrupted.

OK - so the memory map we are copying to is already reserved in device
tree?

See this thread[1] - we are arguing here that the reserved region is
meant for bootloader to fill up and keep protected. DT itself from
kernel is shared between u-boot and kernel OF_UPSTREAM.

Now, the consumer of the binary is DM, the load area is already part of
carveout for DM, it sounds like it should have been packaged with DM
itself instead of making the packaging problem the problem that everyone
image creation system has to solve - not to mention signing etc..

Why not merge this with DM?


[1] https://lore.kernel.org/all/59c391a7-c6fe-4b04-891a-c6905ef29f20@ti.com/
Sebin Francis July 1, 2024, 9:55 a.m. UTC | #4
On 28/06/24 23:10, Nishanth Menon wrote:
> On 19:39-20240628, Sebin Francis wrote:
>> On 28/06/24 18:33, Nishanth Menon wrote:
>>> On 15:02-20240628, Dhruva Gole wrote:
>>>> This series includes the binman related changes required to package tIFS
>>>> Stub to support Low Power Modes on BeaglePlay.
>>>> Also, based on comments from previous patch [0] documentation has been
>>>> added to describe small addition in boot flow as well as tispl image
>>>> format.
>>>>
>>>> I am aware that the new boot flow image will need to be updated in
>>>> other places like am62a, am62p and even other boards that use am62x.
>>>> However, I would like to keep this series beagleplay TIFSStub specific
>>>> and so I will be sending a follow up series to update other places
>>>> seperately if that's ok.
> [...]
>>> Maybe you can help clarify a bit. I understand from [1]
>>> that you are betting on timing to keep tifs stub safe. But then, why
>>> not plug in this firmware blob along with DM itself? that allows DM
>>> to manage itself the way it wants to and control it's own memory map?
>>> DM initialization itself takes a few ms, just because TFA is not
>>> touching any part of DDR does not mean that we can assume system is
>>> interference free, no? What if DM architecture changes such that PLL
>>> initialization or some other long pole item is performed prior to
>>> loading the tifs stub?
>> In Linux DT the address space in which FS stub is copied is part of DM
>> firmware carve-out in DDR,
>> so if fs stub can get corrupted then DM also can get corrupted.
> OK - so the memory map we are copying to is already reserved in device
> tree?
>
> See this thread[1] - we are arguing here that the reserved region is
> meant for bootloader to fill up and keep protected. DT itself from
> kernel is shared between u-boot and kernel OF_UPSTREAM.
>
> Now, the consumer of the binary is DM, the load area is already part of
> carveout for DM, it sounds like it should have been packaged with DM
> itself instead of making the packaging problem the problem that everyone
> image creation system has to solve - not to mention signing etc..
>
> Why not merge this with DM?
>
>
> [1] https://lore.kernel.org/all/59c391a7-c6fe-4b04-891a-c6905ef29f20@ti.com/
For HS-FS device we need to sign the fs-stub with customer key. DM firmware
cannot have a component which is signed using customer key.
Thanks
Sebin
Nishanth Menon July 1, 2024, 3:24 p.m. UTC | #5
On 15:25-20240701, Sebin Francis wrote:
> For HS-FS device we need to sign the fs-stub with customer key. DM firmware
> cannot have a component which is signed using customer key.

Please explain please why DM cannot have a component signed using a
customer key for the public record?
Sebin Francis July 3, 2024, 12:01 p.m. UTC | #6
On 01/07/24 20:54, Nishanth Menon wrote:
> On 15:25-20240701, Sebin Francis wrote:
>> For HS-FS device we need to sign the fs-stub with customer key. DM firmware
>> cannot have a component which is signed using customer key.
> Please explain please why DM cannot have a component signed using a
> customer key for the public record?


For HS device customer is owning the customer key and only customer

has the access for the customer key. Because of this the signing has to 
happen

from the customer side.

DM is release by TI, Since TI doesn't have access to the customer key it 
cannot

have a component that is signed by customer key.


>
Nishanth Menon July 5, 2024, 3:17 p.m. UTC | #7
On 17:31-20240703, Sebin Francis wrote:
> 
> On 01/07/24 20:54, Nishanth Menon wrote:
> > On 15:25-20240701, Sebin Francis wrote:
> > > For HS-FS device we need to sign the fs-stub with customer key. DM firmware
> > > cannot have a component which is signed using customer key.
> > Please explain please why DM cannot have a component signed using a
> > customer key for the public record?
> 
> 
> For HS device customer is owning the customer key and only customer
> 
> has the access for the customer key. Because of this the signing has to
> happen
> 
> from the customer side.
> 
> DM is release by TI, Since TI doesn't have access to the customer key it
> cannot
> 
> have a component that is signed by customer key.

please resubmit the series with this documented in documentation and
commit message.