[U-Boot] [PATCH v2 0/6] Add support for MIPS Creator CI20

Wed Dec 12 20:43:53 UTC 2018

Am 12.12.18 um 19:03 schrieb Ezequiel Garcia:
> Hi Daniel,
> 
> On Wed, 2018-12-12 at 18:27 +0100, Daniel Schwierzeck wrote:
>> Hi Ezequiel,
>>
>> Am 12.12.18 um 14:58 schrieb Ezequiel Garcia:
>>> A new round.
>>>
>>> For this new round:
>>>
>>>  * Replaced infinite while loop with wait_for_bit.
>>>  * Added a MAINTAINERS file. If anyone wants to co-maintain this,
>>>    please let me know.
>>>
>>> This is based on top of yesterday's master (ee168783ae8) and has
>>> been tested by SD-card booting both U-Boot and Linux. Booting
>>> Linux via TFTP was also tested.
>>>
>>> Toolchain used to test:
>>>
>>> $ mips-linux-gcc -v
>>> Using built-in specs.
>>> COLLECT_GCC=mips-linux-gcc
>>> COLLECT_LTO_WRAPPER=/home/zeta/.buildman-toolchains/gcc-7.3.0-nolibc/mips-linux/bin/../libexec/gcc/mips-linux/7.3.0/lto-wrapper
>>> Target: mips-linux
>>> Configured with: /home/arnd/git/gcc/configure --target=mips-linux --enable-targets=all --prefix=/opt/crosstool/gcc-7.3.0-nolibc/mips-linux --
>>> enable-languages=c --without-headers --disable-bootstrap --disable-nls --disable-threads --disable-shared --disable-libmudflap --disable-libssp --
>>> disable-libgomp --disable-decimal-float --disable-libquadmath --disable-libatomic --disable-libcc1 --disable-libmpx --enable-checking=release
>>> Thread model: single
>>> gcc version 7.3.0 (GCC) 
>>>
>>> SPL size:
>>>
>>> $ size spl/u-boot-spl
>>>    text	   data	    bss	    dec	    hex	filename
>>>    9252	    752	    736	  10740	   29f4	spl/u-boot-spl
>>>
>>> I've pushed a branch to https://github.com/ezequielgarcia/u-boot/tree/ci20-v2
>>> and made sure travis passed.
>>>
>>
>> Thanks for working on this but the problem with the original patch
>> series was that it was not fully driver model compatible and that stuff
>> like watchdog, reset and pin-muxing was open-coded instead of using the
>> proper frameworks. As the ship is sailing towards having only driver
>> model and device tree in U-Boot proper, it doesn't make sense to accept
>> new SoC/boards with legacy code especially if there is no active
>> maintainer who would do the conversion work. And with the upcoming dead
>> lines for DM conversions, boards with legacy code will be removed when
>> not converted. This is also one of the reasons why the Ingenic SoC
>> support we already had in mainline was removed. And readding parts of
>> the removed code doesn't make sense either because the patch series is
>> based on that old Ingenic SoC code.
>>
> 
> I have to say I disagree with all of this.
> 
> The open coded drivers are used in SPL, which most likely can't affort
> any DM. Or **if** it can't afford it, it's a separate question that
> needs research.
> 
> The BootROM for this SoC can only load 14 KiB of code, and we are already
> at roughly 11 KiB.

I know that and I don't demand DM in SPL, only for U-Boot proper. And
there is always a ways of factoring out driver code which can be shared
by SPL and a DM compatible driver.

> 
> This series, on the other side, is quite clean, quite small, has quite
> simple drivers (with even a DM MMC driver for the second stage)...
> ... and most importantly it works.
> 
> Given how much history this particular series has, if we delay it now
> with yet more requirements, we risk not ever having it.

There are no more requirements, only the standard ones which blocked the
merge of the original series in the first place. If someone is willing
to work these out in the review process, the series will get merged. ?

> 
> We want to put this board on kernelCI labs, and having upstream U-Boot
> would be really nice for that, so we don't rely on ancient vendor forks.
> 
> As for a maintainer doing the work, I'm here and not going anywhere.
> I can do any follow-up work that's considered needed, including any DM
> conversions.
> 
> But, it would be really important to have a working starting point.
> 
>> I wanted to work on Marek's patch series to fix those issues because
>> it's quite some work which I couldn't expect from Marek ;)
>> But I got stuck with EJTAG because halting the CPU didn't work. Then I
>> hadn't time anymore.
>>
>> I'm little busy right now so I can't do a detailed review. But the
>> issues which should be addressed at first are:
>>
>> - fix all SPDX license identifiers
>> - convert GPIO driver to DM
>> - add a DM watchdog driver
>> - implement machine reset with the generic watchdog reset (see BMIPS for
>> an example)
>> - don't use SoC specific start.S and u-boot-spl.lds, the generic code is
>> now configurable enough and non-generic things could be done in a
>> lowlevel_init.S
>> - reduce the hundreds of definitions of register addresses to the ones
>> really needed in assembly or SPL. This stuff should come from device tree
>> - define the remaining register base addresses as physical addresses and
>> establish a mapping with ioremap_nocache()
>>
>>
> 
> This really sounds that work that can be done as follow-up patches.
> And, we don't know if any of this will bloat the SPL beyond the current
> limit :-)
> 
> I must admit, I have a hard time understanding this blocker policy,
> for a series that gives support to a new board, and affects no other platforms.
> 

I'm not blocking anything, it's only my experience because I already
have some boards where the original contributor is not reachable anymore
and no one is available to test or change the code. So It's either
merging code only with the required quality or to have a committment to
do the required work after the initial merge.

At least for v3 fix the SPDX identifiers (first line of the file and
beginning with //). Also the conversion of the GPIO driver to DM is
simple enogh and should be done now. The only user of the legacy GPIO
API in SPL is ci20_revision(). But most of the code is already
bit-banging on the GPIO registers for muxing and pull-ups. Thus it
doesn't matter in this context to replace gpio_direction_input() with
another bit-banging to drop the dependency on the GPIO driver ;)

-- 
- Daniel