[PATCH v3 3/3] defconfig: rockpro64: Enable SF EFI var store

[Simon Glass]

Hi Shantur,

On Sun, 3 Dec 2023 at 14:49, Shantur Rathore <i at shantur.com> wrote:
>
> Hi Simon,
>
> On Sun, Dec 3, 2023 at 5:44 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Shantur,
> >
> > On Sat, 2 Dec 2023 at 16:12, Shantur Rathore <i at shantur.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, Dec 1, 2023 at 6:44 PM Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Shantur,
> > > >
> > > > On Sun, 26 Nov 2023 at 15:09, Shantur Rathore <i at shantur.com> wrote:
> > > > >
> > > > > RockPro64 uses SPI Flash for storing env, also use it store
> > > > > EFI variables.
> > > > >
> > > > > Signed-off-by: Shantur Rathore <i at shantur.com>
> > > > > ---
> > > > >
> > > > >  configs/rockpro64-rk3399_defconfig | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> > > > > index 4cd6b76665..f550f2ae43 100644
> > > > > --- a/configs/rockpro64-rk3399_defconfig
> > > > > +++ b/configs/rockpro64-rk3399_defconfig
> > > > > @@ -8,6 +8,8 @@ CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> > > > >  CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x300000
> > > > >  CONFIG_ENV_SIZE=0x8000
> > > > >  CONFIG_ENV_OFFSET=0x3F8000
> > > > > +CONFIG_EFI_VARIABLE_SF_STORE=y
> > > > > +CONFIG_EFI_VARIABLE_SF_OFFSET=0x7D0000
> > > >
> > > > Can we use this offset in binman when creating the SPI-flash image?
> > >
> > > Unless I missed something, binman is used when something is bundled
> > > with a u-boot image.
> > > This is similar to ENV_OFFSET letting u-boot know where it can write
> > > the EFI variables.
> > > I didn't see ENV_OFFSET referred in rk3399-u-boot.dtsi or
> > > rk3399-rockpro64-u-boot.dtsi
> > >
> > > Please correct me if I am wrong.
> >
> > Well, this is what I see in arch/arm/dts/rk3399-u-boot.dtsi :
> >
> > #if defined(CONFIG_ROCKCHIP_SPI_IMAGE) && defined(CONFIG_HAS_ROM)
> > &binman {
> >    multiple-images;
> >    rom {
> >       filename = "u-boot.rom";
> >       size = <0x400000>;
> >       pad-byte = <0xff>;
> >
> >       mkimage {
> >          args = "-n rk3399 -T rkspi";
> >          u-boot-spl {
> >          };
> >       };
> >       u-boot-img {
> >          offset = <0x40000>;
> >       };
> >       u-boot {
> >          offset = <0x300000>;
> >       };
> >       fdtmap {
> >       };
> >    };
> > };
> > #endif /* CONFIG_ROCKCHIP_SPI_IMAGE && CONFIG_HAS_ROM */
> >
> > So it looks like 0x3f8000 fits in there somewhere.
> >
>
> Thanks for clarifying. Now I see how ENV space is declared as used by u-boot.
>
>
> > In fact, I wonder if we should add something like:
> >
> > u-boot-env {
> >    offset = <CONFIG_ENV_OFFSET>;
> > };
> >
> > so the environment gets filled into the SPI flash when we write the image?
> >
>
> Do we want to reset the environment to default on every u-boot update?
> I believe this would overwrite the environment space isn't it.

It depends on how the update is done. If the image written is smaller
than the full size of the SPI flash and the environment is at the end
of it, then it would be preserved.

But in fact the u-boot.rom file is 0x400000 bytes (4MB) so I don't see
how that would work. So we might as well add the environment in?

>
> > I suppose we could also add the EFI variable store, although that
> > would require adding a new entry type to binman.
> >
>
> I am not sure about this but surely EFI vars can be moved to a location
> just after the u-boot environment and increase the u-boot image size.

Yes.

>
> Would you still need an entry in dts?

My point was really just that the binman description should contain
the full layout of the flash. Otherwise someone might adjust something
such that the environment overlays U-Boot code, etc.

Regards,
Simon