Re: [coreboot] Kconfig: Move `CBFS_SIZE` out of main section

[Peter Stuge]

Julius Werner wrote:
> > What section should
> >
> > (0x400000) Size of CBFS filesystem in ROM
> >
> > be moved to?
>
> The option is closely related to ROM_SIZE, so maybe put it under
> 'Mainboard -->' with that?

Sounds good.
> > Furthermore, should it even be visible or be hidden as an expert option?
>
> Hiding it as an expert option probably makes sense. It is used to
> restrict the amount of space coreboot (CBFS) takes up on the ROM in
> case you want to put other stuff on there as well, which is probably a
> pretty limited use case.

I think making it depend on EXPERT is a great idea.

I'd also like the interface to be friendlier. What granularity is
actually supported for the option? I doubt that it's single bytes?

Maybe make it decimal kb? The default value depends on ROM_SIZE, right?


//Peter

Kconfig: Move `CBFS_SIZE` out of main section

[Julius Werner]

> I'd also like the interface to be friendlier. What granularity is
> actually supported for the option? I doubt that it's single bytes?
>
> Maybe make it decimal kb? The default value depends on ROM_SIZE, right?

I would very much prefer it to stay byte sized. This makes the
Makefiles that use it easier to write and avoids confusion compared to
the byte sized CONFIG_ROM_SIZE. (IMHO building a unit like kilo or
mega into a variable or interface is always the less desirable choice
because it adds another opportunity to screw up and become confused
for the programmer. Sometimes it can be justified with the risk of
overflow of the respective data type, but for Kconfig that doesn't
count.)

The actually supported granularity is anything that CBFS/cbfstool
supports as an image size, which I think is in theory arbitrary down
to individual bytes.

Kconfig: Move `CBFS_SIZE` out of main section

[Paul Menzel]

Dear Julius, dear Peter,


thank you for your answers.


Am Freitag, den 01.05.2015, 20:30 +0200 schrieb Peter Stuge:
> Julius Werner wrote:
> > > What section should
> > >
> > > (0x400000) Size of CBFS filesystem in ROM
> > >
> > > be moved to?
> >
> > The option is closely related to ROM_SIZE, so maybe put it under
> > 'Mainboard -->' with that?
>
> Sounds good.

I pushed a change set doing just that for review [1], but it
unfortunately fails to build.

As far as I can see, the problem is, that some boards overwrite that
variable by redefining it. I guess the ordering is different in the
mainboard section now, that the default value `ROM_SIZE` is used instead
of the value defined in the mainboard directory.

        src/soc/intel/baytrail/Kconfig:config CBFS_SIZE
        src/soc/intel/baytrail/Kconfig- hex "Size of CBFS filesystem in ROM"
        src/soc/intel/baytrail/Kconfig- default 0x100000
        src/soc/intel/baytrail/Kconfig- help
        src/soc/intel/baytrail/Kconfig-   On Bay Trail systems the firmware image has to store a lot more

Here is an example for the GIGABYTE GA-B75M-D3H:

        $ diff -u /tmp/config-master .config
        --- /tmp/config-master  2015-05-02 09:47:28.438117491 +0200
        +++ .config     2015-05-02 09:56:57.415420886 +0200
        @@ -143,7 +143,7 @@
         CONFIG_BOOT_MEDIA_SPI_BUS=0
         CONFIG_TTYS0_LCS=3
         CONFIG_CACHE_ROM_SIZE_OVERRIDE=0
        -CONFIG_CBFS_SIZE=0x100000
        +CONFIG_CBFS_SIZE=0x800000
         CONFIG_POST_IO=y
         CONFIG_POST_DEVICE=y
         # CONFIG_HAVE_GBE_BIN is not set

[…]

How should that problem be solved in Kconfig? Remove these redefinitions
and have conditional defaults? Then they still would be hard to override
for specific boards though.


Thanks,

Paul


[1] http://review.coreboot.org/10061