[U-Boot] [PATCH v3 0/3] spi: Split CONFIG_DM_SPI* to CONFIG_{SPL_TPL}DM_SPI*

Thu Sep 12 09:03:06 UTC 2019

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.schrempf at 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: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190912/239828a5/attachment.sig>