[PATCH v7 04/22] lib: Makefile: build crc7.c when CONFIG_MMC_SPI

[Pragnesh Patel]

Hi Heinrich,

>-----Original Message-----
>From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>Sent: 02 May 2020 18:29
>To: Bin Meng <bmeng.cn at gmail.com>
>Cc: Pragnesh Patel <pragnesh.patel at sifive.com>; U-Boot Mailing List <u-
>boot at lists.denx.de>; Jagan Teki <jagan at amarulasolutions.com>; Atish Patra
><atish.patra at wdc.com>; Palmer Dabbelt <palmerdabbelt at google.com>; Paul
>Walmsley <paul.walmsley at sifive.com>; Troy Benjegerdes
><troy.benjegerdes at sifive.com>; Anup Patel <anup.patel at wdc.com>; Sagar
>Kadam <sagar.kadam at sifive.com>; Rick Chen <rick at andestech.com>; Peng
>Fan <peng.fan at nxp.com>; Lukasz Majewski <lukma at denx.de>; Simon
>Goldschmidt <simon.k.r.goldschmidt at gmail.com>; Simon Glass
><sjg at chromium.org>; Markus Klotzbuecher
><markus.klotzbuecher at kistler.com>; Baruch Siach <baruch at tkos.co.il>; Joel
>Johnson <mrjoel at lixil.net>; Anatolij Gustschin <agust at denx.de>; AKASHI
>Takahiro <takahiro.akashi at linaro.org>; Marek Behún <marek.behun at nic.cz>
>Subject: Re: [PATCH v7 04/22] lib: Makefile: build crc7.c when
>CONFIG_MMC_SPI
>
>[External Email] Do not click links or attachments unless you recognize the
>sender and know the content is safe
>
>Am May 2, 2020 11:47:10 AM UTC schrieb Bin Meng <bmeng.cn at gmail.com>:
>>Hi Heinrich,
>>
>>On Sat, May 2, 2020 at 6:30 PM Heinrich Schuchardt <xypron.glpk at gmx.de>
>>wrote:
>>>
>>> On 5/2/20 12:06 PM, Pragnesh Patel wrote:
>>> > When build U-Boot SPL, meet an issue of undefined reference to
>>> > 'crc7' for drivers/mmc/mmc_spi.c, so let's compile crc7.c when
>>> > CONFIG_MMC_SPI selected.
>>> >
>>> > Signed-off-by: Pragnesh Patel <pragnesh.patel at sifive.com>
>>> > Reviewed-by: Jagan Teki <jagan at amarulasolutions.com>
>>> > Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
>>> > ---
>>> >  common/spl/Kconfig  | 6 ++++++
>>> >  drivers/mmc/Kconfig | 1 +
>>> >  lib/Makefile        | 1 +
>>> >  3 files changed, 8 insertions(+)
>>> >
>>> > diff --git a/common/spl/Kconfig b/common/spl/Kconfig index
>>> > ef5bf66696..d1f0e6bc4c 100644
>>> > --- a/common/spl/Kconfig
>>> > +++ b/common/spl/Kconfig
>>> > @@ -401,6 +401,12 @@ config SPL_CRC32_SUPPORT
>>> >         for detected accidental image corruption. For secure
>>applications you
>>> >         should consider SHA1 or SHA256.
>>> >
>>> > +config SPL_CRC7_SUPPORT
>>> > +     bool "Support CRC7"
>>> > +     help
>>> > +       Enable CRC7 hashing for drivers which are using in SPL.
>>> > +       This is a 32-bit checksum value that can be used to verify
>>images.
>>> > +
>>> >  config SPL_MD5_SUPPORT
>>> >       bool "Support MD5"
>>> >       depends on SPL_FIT
>>> > diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig index
>>> > 8f0df568b9..139599072a 100644
>>> > --- a/drivers/mmc/Kconfig
>>> > +++ b/drivers/mmc/Kconfig
>>> > @@ -49,6 +49,7 @@ if MMC
>>> >  config MMC_SPI
>>> >       bool "Support for SPI-based MMC controller"
>>> >       depends on DM_MMC && DM_SPI
>>> > +     select SPL_CRC7_SUPPORT if SPL
>>> >       help
>>> >         This selects SPI-based MMC controllers.
>>> >         If you have an MMC controller on a SPI bus, say Y here.
>>> > diff --git a/lib/Makefile b/lib/Makefile index
>>> > c6f862b0c2..fcd934857f 100644
>>> > --- a/lib/Makefile
>>> > +++ b/lib/Makefile
>>> > @@ -80,6 +80,7 @@ endif
>>> >
>>> >  ifdef CONFIG_SPL_BUILD
>>> >  obj-$(CONFIG_SPL_YMODEM_SUPPORT) += crc16.o
>>> > +obj-$(CONFIG_SPL_CRC7_SUPPORT) += crc7.o
>>>
>>> crc7.o is not needed in main U-Boot if MMC_SPI is not selected.
>>>
>>> So instead of adding a new configuration variable simply correct the
>>> existing line in lib/Makefile
>>>
>>> -obj-y += crc7.o
>>> +obj-$(CONFIG_MMC_SPI) += crc7.o
>>
>>This looks incorrect to me. CRC7 can be useful for other drivers too.
>>It should not depend on CONFIG_MMC_SPI.
>
>Using this argument you would always compile everything. Compiling files that
>are not used is simply a waste of CPU time.
>
>Following your argument. you could also always compile crc7.c for SPL and
>hope the compiler will eliminate it.
>
>Either way we do not need a new config symbol.

I think this is already discussed in v3.
https://patchwork.ozlabs.org/project/uboot/patch/20200124055026.30787-4-pragnesh.patel@sifive.com/

>
>Best regards
>
>Heinrich
>
>>
>>>
>>> and move that line inside lib/Makefile so that is does not depend on
>>> CONFIG_SPL_BUILD.
>>>
>>
>>Regards,
>>Bin