Hi Frieder, > Hi Lukasz, > > On 10.09.19 12:22, Lukasz Majewski wrote: > > Hi Frieder, > > > >> On Mon, 9 Sep 2019 11:11:50 +0000 > >> Schrempf Frieder <frieder.schre...@kontron.de> wrote: > >> > >>> Hi Lukasz, > >>> > >>> On 05.09.19 20:09, Tom Rini wrote: > >>>> On Thu, Sep 05, 2019 at 12:16:36AM +0200, Lukasz Majewski wrote: > >>>> > >>>>> This patch series introduces new SPL and TPL specific Kconfig > >>>>> entries for DM_SPI* options. Such change allows using the spi > >>>>> driver in SPL/TPL or U-Boot proper. > >>>>> > >>>>> First two patches - related to ls10{42}* NXP soc fix some issues > >>>>> with defining the DM_SPI* defines in <board>.h file instead of > >>>>> Kconfig. > >>>>> > >>>>> This series doesn't introduce build breaks, but board > >>>>> maintainers are kindly asked to check if their boards still > >>>>> boots. > >>>>> > >>>>> Buildman setup for binary size regression checking: > >>>>> > >>>>> ./tools/buildman/buildman.py -b HEAD --count=4 ls1043 > >>>>> --output-dir=../BUILD/ --force-build -CveE > >>>>> ./tools/buildman/buildman.py -b HEAD --count=4 ls1043 > >>>>> --output-dir=../BUILD/ -Ssdel > >>>> > >>>> So you did fix the ls1043 problems but ls1046 is still a problem. > >>>> > >>> > >>> I was trying to clean up this config mess some weeks ago. I > >>> stumbled over the same issues (size deltas below) when I tested > >>> with buildman and finally gave up on it. This was my testing > >>> branch for reference: [1]. > >>> > >>> Thanks for your work and I hope you/we can get this sorted out > >>> somehow... > >> > >> For now I've only posted the patch to introduce SPL_DM_SPI in > >> Kconig: https://patchwork.ozlabs.org/patch/1159655/ > > > > However, I've looked on your patchset and IMHO this work could be > > divided (as doing it at once is not feasible). > > > > For example the CONFIG_SPI_FLASH_MTD could be converted to > > (SPL_TPL_)SPI_FLASH_MTD and then one could use > > > > #if CONFIG_IS_ENABLED(SPI_FLASH_MTD) in drivers/mtd/spi/sf_probe.c > > (as it is only used there). > > > > Then we could avoid situations where code is added as you remove it > > here [1]: > > https://github.com/fschrempf/u-boot/commit/b6489fb5928c2b41d7e4cb39933f078659b4f10e#diff-9d3e174d033b8b9c9d380a22a81600aaL136 > > > > What I'm afraid though, is that split of SPI_FLASH_MTD will require > > adding unwillingly SPL_(TPL_)SPI_FLASH_MTD to all boards which > > already define it (and only drop ones, which use in <config>.h file > > pattern as [1]). > > Yes, this looks like what I've tried to do separately in this branch > [1]. > > The problem with some socfpga boards is, that they enable > CONFIG_SPI_FLASH_MTD in socfpga_common.h, without enabling > CONFIG_SPI_FLASH, which is probably wrong.
It looks to me like the code in: https://github.com/fschrempf/u-boot/commit/059d67efa34da92aaf738758e596f436203c84c2#diff-9d3e174d033b8b9c9d380a22a81600aaL136 is to prevent ALL socfpgas from compiling in FLASH MTD support to SPL, as it causes build breaks (as I do have such situation in one of my boards - it uses tiny SPI in SPL to read data from SPI-NOR, without the need to enable MTD there) . In other words those boards only use FLASH MTD driver in U-Boot proper. (and probably there shall not be any deltas in buildman build binaries [*]) > So I tried to correct > this, but looking at it again, this should be done separately. > > So if I remove the added "CONFIG_SPI_FLASH=y" from my patches and > rebase, this should be ok. I think yes. I guess that ALL socfpgas shall have added CONFIG_SPI_FLASH_MTD=y to their _defconfigs It may also happen that boards, which define CONFIG_SPI_FLASH_MTD would require both CONFIG_SPI_FLASH_MTD and CONFIG_SPL_SPI_FLASH_MTD defined (if they don't use socfpga style <config>.h code) to have the same binaries build. > > For this set I have still one question: Should I split the patches as > currently done in [1]? This would mean after the first patch some > boards miss SPI_FLASH_MTD code and the subsequent board config > patches correct it afterwards. Or should I merge all the changes to a > single patch, to not break the boards in between. I would opt for preparing one single patch with conversion (to avoid build breaks). This would also allow easy buildman testing [*] to see if there is any difference in sizes of binaries (elf sections to be precise). I would also add the patch to define CONFIG_SPL_SPI_FLASH_MTD in Kconfig to show that such option is available for use after the conversion (IMHO it shall be added before the conversion patch). > Unfortunately I can't do it the other way round and apply the board > config changes first, as this breaks the build. The volume of changes is rather small - so single patch would be optimal here. > > > Frieder, would you be able to work on this topic any time soon? > > I can try to find some time this weekend and try to get [1] ready. Great, thanks :-) > But I probably won't be able to spend serious amounts of time anytime > soon on the remaining tasks. I think that we shall do it step by step. As we both learned from the experience - doing it at once is not feasible. My comments to the patch set [1]: 1. https://github.com/fschrempf/u-boot/commit/059d67efa34da92aaf738758e596f436203c84c2#diff-94a725bbe2cb8781105dab5153da9209R44 Is the CONFIG_SPI_FLASH = y necessary? [*] - Buildman setup for testing (shared with me by Tom): Set date: export SOURCE_DATE_EPOCH=`date +%s` Build the code: ./tools/buildman/buildman.py -b HEAD --count=1 socfpga dh_imx6 <other> --output-dir=../BUILD/ --force-build -CveE Check build results (including binary deltas): ./tools/buildman/buildman.py -b HEAD --count=1 socfpga dh_imx6 <other> --output-dir=../BUILD/ -SsdelB And you shall see the build results (with binary deltas). > > Thanks, > Frieder > > [1]: https://github.com/fschrempf/u-boot/commits/spi_flash_mtd_cleanup Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lu...@denx.de
pgpDc3Z3GVhNp.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot