On 12.09.19 11:03, Lukasz Majewski wrote:
> 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
> [*])
Right.
>
>> 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
Right.
>
>
> 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.
Last time I looked such boards didn't exist, but I'll check again.
>
>>
>> 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).
Ok. Last time I worked on this, there was no board using SPI_FLASH_MTD
in SPL. But this might not be true anymore. Anyway, I'll add the option.
>
>> 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.
Ok.
>
>>
>>> 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.
Definitely!
>
> 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?
For the boards to work properly, it probably is necessary, but it builds
fine without, so as said above I will leave this fix for someone who
knows about socfpga.
>
>
>
> [*] - 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, I already use such buildman setup to test the 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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
