From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7DDEDEB64D9 for ; Fri, 7 Jul 2023 17:10:29 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5F22E861E6; Fri, 7 Jul 2023 19:10:27 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=konsulko.com header.i=@konsulko.com header.b="CmDegLC6"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 32C588623A; Fri, 7 Jul 2023 19:10:26 +0200 (CEST) Received: from mail-yb1-xb31.google.com (mail-yb1-xb31.google.com [IPv6:2607:f8b0:4864:20::b31]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 1528C861A1 for ; Fri, 7 Jul 2023 19:10:23 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=konsulko.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=trini@konsulko.com Received: by mail-yb1-xb31.google.com with SMTP id 3f1490d57ef6-bff89873d34so2170137276.2 for ; Fri, 07 Jul 2023 10:10:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; t=1688749822; x=1691341822; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=iFv9kBiz9r6oHH/ZnYnW2SXP40cOc5En1sNLQhLUEOk=; b=CmDegLC66+h/424BL2l+JZskQy+Er1GBwbEzj9RAKUwIPUyolc5ncURgkVHd+APqxb VxsCWyCCIkpeBKv1REbljwDHzAv3WuoVoujcXfBkdWq9AXK4I61BxvJWHxLkzYvxgURx C5QUTlDzNttjloVs30NrPjOR9UMfFdhuUbM7g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688749822; x=1691341822; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=iFv9kBiz9r6oHH/ZnYnW2SXP40cOc5En1sNLQhLUEOk=; b=gpvItphHemyXiDV8P8cwjf6vjQXFXwM8fiMIpi91lp6Fv18FyL6Mwanq6watoYnOk9 rT6Hw7la9B3QUEbN0b6fJ3EhVtK0Kinhu1f0TK4yjzxl1GKTqNnk5CASxygtCQYwhr5g bOdi3FVoJkpfknzUq2jGIfr0sQSi+z5oyAN1C6JrZLTnjIBhX6CcdrlIQaZ+4fpJTHtB IvDP2vOWWjv2yft8vuLlI7VDIJ81xAhiYyN0H/zmdMZpN1ObukHZxQT6iKxPBuuopRj5 hbxmAz+bLdJx7Ed6xUoQBYT65KjY+KgKK+oCEHYkSgX3mIfqSjRkN4YVP/Unvf+Bbwpm 56ug== X-Gm-Message-State: ABy/qLZs7ypKxpu7nFi9DB7N/9GJ4TrX7JDUZG3qoxpB7pQf6qBX8qX2 i50XmURUnV1ATbZ6Fk6R07cIU9usvikZoXQm+B/o7w== X-Google-Smtp-Source: APBJJlFEcRNIEUSDc8NqEzBV9RAOm8lsXzQM6T9dPeCEA3cCnCL0HDPzSSjXFbpAKMQRd70V76R6sA== X-Received: by 2002:a25:23c1:0:b0:c6d:f875:520e with SMTP id j184-20020a2523c1000000b00c6df875520emr1635578ybj.49.1688749821656; Fri, 07 Jul 2023 10:10:21 -0700 (PDT) Received: from bill-the-cat (2603-6081-7b00-6400-21ea-8e86-6208-34bf.res6.spectrum.com. [2603:6081:7b00:6400:21ea:8e86:6208:34bf]) by smtp.gmail.com with ESMTPSA id v62-20020a25c541000000b00bf4d24fd976sm1036227ybe.10.2023.07.07.10.10.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 07 Jul 2023 10:10:21 -0700 (PDT) Date: Fri, 7 Jul 2023 13:10:19 -0400 From: Tom Rini To: Pali =?iso-8859-1?Q?Roh=E1r?= Cc: Jaehoon Chung , u-boot@lists.denx.de Subject: Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit Message-ID: <20230707171019.GD148062@bill-the-cat> References: <20230413211057.10975-2-pali@kernel.org> <20230706173502.2796-1-pali@kernel.org> <20230706174218.GB7930@bill-the-cat> <20230706174918.iupb2gatj3s7w7jd@pali> <20230706175214.GC7930@bill-the-cat> <20230707164639.d6twn4r5gumzprsa@pali> <20230707165458.GB148062@bill-the-cat> <20230707170545.tdzzeigeghmesalb@pali> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="zn+0xsUnUr6Dk02Q" Content-Disposition: inline In-Reply-To: <20230707170545.tdzzeigeghmesalb@pali> X-Clacks-Overhead: GNU Terry Pratchett X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean --zn+0xsUnUr6Dk02Q Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 07, 2023 at 07:05:45PM +0200, Pali Roh=E1r wrote: > On Friday 07 July 2023 12:54:58 Tom Rini wrote: > > On Fri, Jul 07, 2023 at 06:46:39PM +0200, Pali Roh=E1r wrote: > > > On Thursday 06 July 2023 13:52:14 Tom Rini wrote: > > > > On Thu, Jul 06, 2023 at 07:49:18PM +0200, Pali Roh=E1r wrote: > > > > > On Thursday 06 July 2023 13:42:18 Tom Rini wrote: > > > > > > On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Roh=E1r wrote: > > > > > > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_= partition() > > > > > > > function better understandable, rewrite it via explicit switc= h-case code > > > > > > > pattern. > > > > > > >=20 > > > > > > > Also add a warning when eMMC EXT_CSD[179] register is configu= red by user to > > > > > > > value which is not suitable for eMMC booting and SPL do not k= now how to > > > > > > > interpret it. > > > > > > >=20 > > > > > > > Note that when booting from eMMC device via EXT_CSD[179] regi= ster is > > > > > > > explicitly disabled then SPL still loads and boots from this = eMMC device > > > > > > > from User Area partition. This behavior was not changed in th= is commit and > > > > > > > should be revisited in the future. > > > > > > >=20 > > > > > > > Signed-off-by: Pali Roh=E1r > > > > > > > --- > > > > > > > Changes in v2: > > > > > > > * Disable showing warning on sama5d2_xplained due to size res= trictions > > > > > > > --- > > > > > > > This patch depends on another patch: > > > > > > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partitio= n() > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/202304042028= 05.8523-1-pali@kernel.org/ > > > > > > > --- > > > > > > > common/spl/Kconfig | 7 +++++++ > > > > > > > common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++= ++-------- > > > > > > > 2 files changed, 45 insertions(+), 8 deletions(-) > > > > > > >=20 > > > > > > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig > > > > > > > index 865571d4579c..0574d22b3b25 100644 > > > > > > > --- a/common/spl/Kconfig > > > > > > > +++ b/common/spl/Kconfig > > > > > > > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE > > > > > > > help > > > > > > > Enable write access to MMC and SD Cards in SPL > > > > > > > =20 > > > > > > > +config SPL_MMC_WARNINGS > > > > > > > + bool "Print MMC warnings" > > > > > > > + depends on SPL_MMC > > > > > > > + default y if !TARGET_SAMA5D2_XPLAINED > > > > > > > + help > > > > > > > + Print SPL MMC warnings. You can disable this option to re= duce SPL size. > > > > > > > + > > > > > > > =20 > > > > > > > config SPL_MPC8XXX_INIT_DDR > > > > > > > bool "Support MPC8XXX DDR init" > > > > > > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c > > > > > > > index f7a42a11477d..ec424ceded0e 100644 > > > > > > > --- a/common/spl/spl_mmc.c > > > > > > > +++ b/common/spl/spl_mmc.c > > > > > > > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition= (struct mmc *mmc) > > > > > > > * > > > > > > > * Note: See difference between EXT_CSD_EXTRACT_PARTITION_A= CCESS > > > > > > > * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area= value. > > > > > > > - * > > > > > > > - * FIXME: When booting from this eMMC device is explicitly > > > > > > > - * disabled then we use User area for booting. This is inco= rrect. > > > > > > > - * Probably we should skip this eMMC device and select the = next > > > > > > > - * one for booting. Or at least throw warning about this fa= llback. > > > > > > > */ > > > > > > > - part =3D EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config); > > > > > > > - if (part =3D=3D 7) > > > > > > > - part =3D 0; > > > > > > > + if (mmc->part_config =3D=3D MMCPART_NOAVAILABLE) > > > > > > > + part =3D 0; /* If partitions are not supported then we hav= e only User Area partition */ > > > > > > > + else { > > > > > > > + switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) { > > > > > > > + case 0: /* Booting from this eMMC device is disabled */ > > > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > > > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS > > > > > > > + puts("spl: WARNING: Booting from this eMMC device is disa= bled in EXT_CSD[179] register\n"); > > > > > > > + puts("spl: WARNING: Continuing anyway and selecting User = Area partition for booting\n"); > > > > > > > +#else > > > > > > > + puts("spl: mmc: fallback to user area\n"); > > > > > > > +#endif > > > > > > > +#endif > > > > > > > + /* FIXME: This is incorrect and probably we should select= next eMMC device for booting */ > > > > > > > + part =3D 0; > > > > > > > + break; > > > > > > > + case 1: /* Boot partition 1 is used for booting */ > > > > > > > + part =3D 1; > > > > > > > + break; > > > > > > > + case 2: /* Boot partition 2 is used for booting */ > > > > > > > + part =3D 2; > > > > > > > + break; > > > > > > > + case 7: /* User area is used for booting */ > > > > > > > + part =3D 0; > > > > > > > + break; > > > > > > > + default: /* Other values are reserved */ > > > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT > > > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS > > > > > > > + puts("spl: WARNING: EXT_CSD[179] register is configured t= o boot from Reserved value\n"); > > > > > > > + puts("spl: WARNING: Selecting User Area partition for boo= ting\n"); > > > > > > > +#else > > > > > > > + puts("spl: mmc: fallback to user area\n"); > > > > > > > +#endif > > > > > > > +#endif > > > > > > > + part =3D 0; > > > > > > > + break; > > > > > > > + } > > > > > > > + } > > > > > > > #endif > > > > > >=20 > > > > > > Please just use debug() for these messages. > > > > >=20 > > > > > All other error/warning messages in this file are printed via put= s(). > > > > > So I'm just following the current style (and I'm really not going= to > > > > > change all occurrences in this patch). > > > >=20 > > > > Except for the messages in that file which use debug() they use put= s(), > > > > yes. Since none of these are fatal messages (you're falling through) > > > > please switch them to debug() rather than introduce a new CONFIG sy= mbol, > > > > so that if someone is bringing up a platform where this is a problem > > > > they'll be able to debug it, but the general case does not increase > > > > the binary size of most platforms. I'm not asking you to change any= thing > > > > existing in the file, only what you're adding. > > >=20 > > > We should at those two places fail. But I do not want to break existi= ng > > > improperly configured setups, so warning a good way to show people th= at > > > they have something misconfigured. Later in future we switch warnings= to > > > fatal errors. But if we do not show anything at these points, nobody > > > would figure out that has improper setup configuration. debug() is II= RC > > > not shown by default. > >=20 > > Ah, so the plan is they should be fatal, and there's a way to fix the > > configuration? >=20 > Yes, EXT_CSD[179] is configurable register, you can change boot > partition bits (those are non-volatile) via your favorite emmc config > tool. U-Boot has also tool "mmc partconf" which can do that. But you > first need to be able enter into u-boot console and also you need to > enable this tool your board config file. >=20 > > Lets just panic() them now, instead. I really really do > > not want to grow every SPL+MMC using board (which is a lot of them) by > > several hundred bytes of strings. Lets catch the mis-configuration, > > merge it in early in the cycle (right now for v2023.10 for example, > > especially since the MMC custodian is promising to review things and get > > a PR out ASAP), and see what falls out. A panic() and a big comment > > explaining things should suffice. >=20 > Ok, I can change "reserved values" to panic(). >=20 > About "booting is disabled" - I do not think that panic is correct here. > See inline comments in code. Rather do the correct thing - skip emmc and > let SPL to boot from next source. But some message needs to be printed > here why emmc was skipped.... How verbose all of this needs to be depends on who is likely to encounter this type of problem. Are we talking about systems in the wild today that are misconfigured, or are we talking about problems that will arise as part of bringing up a new design and if you don't do X correctly then Y will happen oddly/wrongly? --=20 Tom --zn+0xsUnUr6Dk02Q Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmSoRvsACgkQFHw5/5Y0 tyxqPwv/UBCqcXR+0G3naieJdYmkHUEFUofRKpmJjqMpac4+d5BmHquxUxo+jrGu RKhT78L1ZVT0Rtqq3sKHT51JNBTNQ9RHcFvADl/y5UGLgub6GdruPusvWIaj3k/v rOZgMvMRKRKgHyfT6rHCaP4HvukczJcBdtR1oCxiOvrF9lW+r8kATFKJjbSQ9XfE /cqQJwSoWUoT84RtOKPl2x/liYIWp55CO21ufFYEr/jzK6/XSFfFBKvQ/54Hp/30 puzMq4K4deHI39OscTn3VrwCO7ygdOGZ42pfn484AM35NZw2tJ0gP5UQ1CfOkizn Izhg1XRo+jB8RKIirYNpEARk5IRMwmL8+nmqjTP3NWh0M3kWG9ujS8L6PJobNHQO vHy5cJdBhjOjBhyRoUXa26zDJu+MvkTkNHT4qi3HgqNrEMsRFxfDjBTPJh9t/icA 6oBw09+P1hi7NXRJ5jKrgNtHZyDxLdjo6EQskOAD/KH1TZAZHSYAH16YR5nlsWv+ C4/6XunC =3caP -----END PGP SIGNATURE----- --zn+0xsUnUr6Dk02Q--