From: Pali =?utf-8?q?Roh=C3=A1r?= <pali.rohar@gmail.com>
To: Javier Martinez Canillas <javier@osg.samsung.com>
Subject: Re: [PATCH] arm: dt: Initialize boot_command_line from CONFIG_CMDLINE in case DT does not provide /chosen/bootargs
Date: Fri, 16 Dec 2016 13:32:36 +0100
User-Agent: KMail/1.13.7 (Linux/3.13.0-105-generic; KDE/4.14.2; x86_64; ; )
Cc: Tony Lindgren <tony@atomide.com>,
        "Russell King - ARM Linux" <linux@armlinux.org.uk>,
        Arnd Bergmann <arnd@arndb.de>, Robin Murphy <robin.murphy@arm.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Ben Dooks <ben-linux@fluff.org>,
        Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>,
        Sebastian Reichel <sre@kernel.org>,
        Aaro Koskinen <aaro.koskinen@iki.fi>, Pavel Machek <pavel@ucw.cz>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <1481749963-8664-1-git-send-email-pali.rohar@gmail.com> <201612161246.12155@pali> <029ec718-89c0-b08e-de7e-279946c1a9bd@osg.samsung.com>
In-Reply-To: <029ec718-89c0-b08e-de7e-279946c1a9bd@osg.samsung.com>
MIME-Version: 1.0
Content-Type: multipart/signed;
  boundary="nextPart10591345.bRCaLpB8pg";
  protocol="application/pgp-signature";
  micalg=pgp-sha1
Content-Transfer-Encoding: 7bit
Message-Id: <201612161332.36406@pali>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4651
Lines: 121

--nextPart10591345.bRCaLpB8pg
Content-Type: Text/Plain;
  charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi!

On Friday 16 December 2016 13:13:34 Javier Martinez Canillas wrote:
> Hello Pali,
>=20
> On 12/16/2016 08:46 AM, Pali Roh=C3=A1r wrote:
> > On Thursday 15 December 2016 01:09:20 Pali Roh=C3=A1r wrote:
> >> On Thursday 15 December 2016 00:52:24 Russell King - ARM Linux
> >> wrote:
> >>> On Wed, Dec 14, 2016 at 10:12:43PM +0100, Pali Roh=C3=A1r wrote:
> >>>> Commit 008a2ebcd677 ("ARM: dts: omap3: Remove skeleton.dtsi
> >>>> usage") broke support for setting cmdline on Nokia N900 via
> >>>> CONFIG_CMDLINE.
> >>>>=20
> >>>> It is because arm code booted in DT mode parse cmdline only via
> >>>> function early_init_dt_scan_chosen() and that function does not
> >>>> fill variable boot_command_line when DTB does not contain
> >>>> /chosen entry. It is called from function
> >>>> early_init_dt_scan_nodes() in setup_machine_fdt().
> >>>>=20
> >>>> This patch fixes it by explicitly filling boot_command_line in
> >>>> function setup_machine_fdt() after calling
> >>>> early_init_dt_scan_nodes() in case boot_command_line still
> >>>> remains empty.
> >>>=20
> >>> This looks like a hack.
> >>>=20
> >>> First, the matter of the ATAGs compatibility.  The decompressor
> >>> relies on there being a pre-existing /chosen node to insert the
> >>> command line and other parameters into.  If we've dropped it (by
> >>> dropping skeleton.dtsi) then we've just regressed more than just
> >>> N900 - the decompressor won't be able to merge the ATAGs into the
> >>> concatenated FDT.
> >>=20
> >> Hm... I did not think about it. But right this can be broken
> >> too...
> >=20
> > Tony, Javier: are you aware of above =E2=86=91=E2=86=91=E2=86=91 proble=
m?
> >=20
> > It looks like commit 008a2ebcd677 ("ARM: dts: omap3: Remove
> > skeleton.dtsi usage") should be really reverted.
>=20
> I don't think reverting the mentioned commit is the correct fix for
> your problem. We are trying to get rid of skeleton.dtsi for many
> reasons, see commit commit ("3ebee5a2e141 arm64: dts: kill
> skeleton.dtsi").

$ git show 3ebee5a2e141

* The default empty /chosen and /aliases are somewhat useless...

That is not truth, they are not useless as Russell King wrote --=20
removing them break ATAG support.

(But that commit is for arm64 which probably is not using ATAGs... But I=20
do not know. At least it is not truth for 32bit arm.)

> Also, the chosen node is mentioned to be optional in the ePAPR
> document and u-boot creates a chosen node if isn't found [0] so this
> issue is only present in boards that don't use u-boot like the
> N900/N950/N9 phones.

Linux arm decompressor does not propagate ATAGs when /chosen is missing.=20
Sorry, but if for Linux /chosen is required (and without it is broken!)=20
then some ePARP document does not apply there. Either Linux code needs=20
to be fixed (so /chosen will be really only optional) or /chosen stay in=20
Linux required. There is no other option.

And I hope that U-boot is not the only one bootloader which Linux kernel=20
supports. I thought that I can use *any* bootloader to boot Linux kernel=20
not just U-Boot which is doing some magic...

With this step you are basically going to break booting Linux kernel=20
with all others bootloaders... And personally I really dislike this=20
idea.

> So if NOLO doesn't do the same than u-boot and the kernel expects a
> chosen node, I suggest to add an empty chosen node in the
> omap3-n900.dts and omap3-n950-n9.dtsi device tree source files.

That would fix a problem for N900, N950 and N9. But not for all other=20
ARM devices which bootloader pass some ATAGs.

IIRC rule of kernel is not to break compatibility and that commit=20
008a2ebcd677 really did it.

Note: I'm not saying if 008a2ebcd677 is good or bad. I'm just saying=20
that it cause problems which need to be properly fixed. And if fixing=20
them is harder and will take more time, then correct option is to revert=20
008a2ebcd677 due to breaking support for more devices.

> [0]:
> http://git.denx.de/?p=3Du-boot.git;a=3Dblob;f=3Dcommon/fdt_support.c;h=3D=
c9f
> 7019e38e8de1469f506cdd57353fd27d8e134;hb=3DHEAD#l226
>=20
> Best regards,

=2D-=20
Pali Roh=C3=A1r
pali.rohar@gmail.com

--nextPart10591345.bRCaLpB8pg
Content-Type: application/pgp-signature; name=signature.asc 
Content-Description: This is a digitally signed message part.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEABECAAYFAlhT3uQACgkQi/DJPQPkQ1JCrgCeJhPkoz/GxP4/kGF4KE6WUt1L
5M4An3VFiFjOkm98wyzCy5bXKlyJdG/1
=T30k
-----END PGP SIGNATURE-----

--nextPart10591345.bRCaLpB8pg--