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 C4843EB64D9 for ; Fri, 7 Jul 2023 18:17:13 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id A8306861A5; Fri, 7 Jul 2023 20:17:11 +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="kjrbgbZa"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id B673A8622D; Fri, 7 Jul 2023 20:17:10 +0200 (CEST) Received: from mail-yw1-x1131.google.com (mail-yw1-x1131.google.com [IPv6:2607:f8b0:4864:20::1131]) (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 AF40D86122 for ; Fri, 7 Jul 2023 20:17:07 +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-yw1-x1131.google.com with SMTP id 00721157ae682-579efc32377so29145537b3.1 for ; Fri, 07 Jul 2023 11:17:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; t=1688753826; x=1691345826; 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=Ht78aCYDbrsBD5hMa/yhLA6VjCnVn87PGdnqlR0CStQ=; b=kjrbgbZavIPmwZ9KZAuinjFallGxgPC3yu0f81dC4XfBrCfbO8syH6o3Qib+WlF0Ck zlWllU51x+SYVryBtyCtDFl2DARpgvZzIrwT1vMW8eSR+Q222upCY/UAllNPhTphA2Dv Ti1R57pJ2J1+MhwNaybGDQV3ejQOv+VMBQjWQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688753826; x=1691345826; 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=Ht78aCYDbrsBD5hMa/yhLA6VjCnVn87PGdnqlR0CStQ=; b=c0+/DkqyS3Hdxo3o5Dn9mgDtnLDFiKX4pQsvN2hyutB2ARfFdOsrU9G1fo6FTZnEV1 qexXr5HzDa5OhrBU0gZgoUSSWikEyC5e+1cMrF3U9rNM3pwMsgdYXVZJ+d8Lx59rhcdu lvHXHmGZ+H3q6GUSfbe7ewu10DbhHs9aUS3LcUIdsownsnDU3HHsX3eX1bqXZkC05DYq tjBkrRGtmJ6AiNDidvGhfFE9PF9E3k5esQm27nCecJbCdBoOozxZn1TnAmrYp0gZVAY0 QHlCOG2uR/+UjKt6IzBfMfbteCHmzYQ7VH1MZfWc76aP4UukiKf1wtdJJxy7K6BoPi9P 3EZg== X-Gm-Message-State: ABy/qLZ67EwQ8t7vp4zWmVCXA9djB09efMUoLo36KpfjUmEaCDTC55Bh asP6vlGXYtc7ei+ddueXGsMjtmE8Uom3UnP/WUbDWw== X-Google-Smtp-Source: APBJJlHxCDFjinSYiRbKnu4PikGZzuTAFyyd4Vt5ATay0hRH3TPJWAwWK5Bv4VOEKL4gG6OmOi3nHw== X-Received: by 2002:a0d:fa44:0:b0:577:cd0:681f with SMTP id k65-20020a0dfa44000000b005770cd0681fmr6044695ywf.26.1688753826354; Fri, 07 Jul 2023 11:17:06 -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 v76-20020a81484f000000b0054fba955474sm1198805ywa.17.2023.07.07.11.17.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 07 Jul 2023 11:17:05 -0700 (PDT) Date: Fri, 7 Jul 2023 14:17:04 -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: <20230707181704.GG148062@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> <20230707171019.GD148062@bill-the-cat> <20230707175656.ppkpi3redzqddyml@pali> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Dajbyvcx6ZeKq+2B" Content-Disposition: inline In-Reply-To: <20230707175656.ppkpi3redzqddyml@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 --Dajbyvcx6ZeKq+2B Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 07, 2023 at 07:56:56PM +0200, Pali Roh=E1r wrote: > On Friday 07 July 2023 13:10:19 Tom Rini wrote: > > 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 wrot= e: > > > > > > > > > To make eMMC partition choosing in default_spl_mmc_emmc_b= oot_partition() > > > > > > > > > function better understandable, rewrite it via explicit s= witch-case code > > > > > > > > > pattern. > > > > > > > > >=20 > > > > > > > > > Also add a warning when eMMC EXT_CSD[179] register is con= figured by user to > > > > > > > > > value which is not suitable for eMMC booting and SPL do n= ot know how to > > > > > > > > > interpret it. > > > > > > > > >=20 > > > > > > > > > Note that when booting from eMMC device via EXT_CSD[179] = register is > > > > > > > > > explicitly disabled then SPL still loads and boots from t= his eMMC device > > > > > > > > > from User Area partition. This behavior was not changed i= n this 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= restrictions > > > > > > > > > --- > > > > > > > > > This patch depends on another patch: > > > > > > > > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_part= ition() > > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404= 202805.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 t= o reduce 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_parti= tion(struct mmc *mmc) > > > > > > > > > * > > > > > > > > > * Note: See difference between EXT_CSD_EXTRACT_PARTITI= ON_ACCESS > > > > > > > > > * and EXT_CSD_EXTRACT_BOOT_PART, specially about User = area value. > > > > > > > > > - * > > > > > > > > > - * FIXME: When booting from this eMMC device is explici= tly > > > > > > > > > - * disabled then we use User area for booting. This is = incorrect. > > > > > > > > > - * Probably we should skip this eMMC device and select = the next > > > > > > > > > - * one for booting. Or at least throw warning about thi= s fallback. > > > > > > > > > */ > > > > > > > > > - 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= have 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 = disabled in EXT_CSD[179] register\n"); > > > > > > > > > + puts("spl: WARNING: Continuing anyway and selecting U= ser Area partition for booting\n"); > > > > > > > > > +#else > > > > > > > > > + puts("spl: mmc: fallback to user area\n"); > > > > > > > > > +#endif > > > > > > > > > +#endif > > > > > > > > > + /* FIXME: This is incorrect and probably we should se= lect 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 configur= ed to boot from Reserved value\n"); > > > > > > > > > + puts("spl: WARNING: Selecting User Area partition for= booting\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= puts(). > > > > > > > So I'm just following the current style (and I'm really not g= oing to > > > > > > > change all occurrences in this patch). > > > > > >=20 > > > > > > Except for the messages in that file which use debug() they use= puts(), > > > > > > yes. Since none of these are fatal messages (you're falling thr= ough) > > > > > > please switch them to debug() rather than introduce a new CONFI= G symbol, > > > > > > so that if someone is bringing up a platform where this is a pr= oblem > > > > > > they'll be able to debug it, but the general case does not incr= ease > > > > > > the binary size of most platforms. I'm not asking you to change= anything > > > > > > existing in the file, only what you're adding. > > > > >=20 > > > > > We should at those two places fail. But I do not want to break ex= isting > > > > > improperly configured setups, so warning a good way to show peopl= e that > > > > > they have something misconfigured. Later in future we switch warn= ings to > > > > > fatal errors. But if we do not show anything at these points, nob= ody > > > > > would figure out that has improper setup configuration. debug() i= s IIRC > > > > > not shown by default. > > > >=20 > > > > Ah, so the plan is they should be fatal, and there's a way to fix t= he > > > > 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 an= d 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 he= re. > > > 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.... > >=20 > > 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 > > --=20 > > Tom >=20 > I have no idea. We already seen that not all people understood how it > works. And also people are always creative in inventing hacks. So > this is mainly for existing systems. >=20 > For future in mmc area I have just cleanup patches which should not > result in code change or in behavior change. OK, so how about something like: case 0: /* Booting from this eMMC device is disabled */ /* Long comment to explain likely mis-configuration and how to fix it */ puts("spl: mmc: eMMC misconfigured, falling back to eMMC user area\n"); fallthrough; case 7: 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; As that shouldn't be too much of a size increase from strings, but still be "scary" enough that users will report the message to find out what's going on, and we can investigate from there (my first open question is what case the TI ARCH_OMAP2PLUS trigger as ROM does not support the boot partitions there for booting, that was only fixed in the K3 parts). --=20 Tom --Dajbyvcx6ZeKq+2B Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQGzBAABCgAdFiEEGjx/cOCPqxcHgJu/FHw5/5Y0tywFAmSoVpsACgkQFHw5/5Y0 tyxX1AwAlMDcH/nfkpbezdRWF9nuNTE2cXb79Yfmj3qjV34y3SPzfHnEc7y26BKl me+7tTjfrWTd0rD0k0jDfB4o+eXdzBnGNKyTnUwPDUnQcPt1hjWbW0CfrUj/FspS 3GTuLi69mEbTDaRQEHcPJMwMLfSXQbCKdtfTUegnXBU0rbXicuPbxEU0CulHOzut L9iwRpmd3vf8fC2lLq21SChma5tEy511Q/DbRAkTsR8ug/WArMT+x5DAszEaxlha bjCRkMF8hpV+NGKfYGiwwe+LV7P/uJ7fZDqrzLNmFYtAw4FZZyUsu+3ZJAgfHIJ2 ppluQWcgxHiCHh0tk3Fyy3NAEUIgD0qhSw19MB2B1g4A6lK2V41MdzhlKYlHFb0J A/9dCQGzFWcwdU2WC/d7ZSLyjECIJQpGGqk3VsOPYXuGrK+UbvzwDx29J98xMuTX 6UTPwiUkFChMXw1lCZav9EPg7zakeLHQD+VHbGNmU4X8Y11RWQaDrmcPU8yEadmD yK0XQ2rz =8Z3A -----END PGP SIGNATURE----- --Dajbyvcx6ZeKq+2B--