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 D6654C4332F for ; Thu, 9 Nov 2023 10:06:19 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1973787113; Thu, 9 Nov 2023 11:06:18 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=baylibre-com.20230601.gappssmtp.com header.i=@baylibre-com.20230601.gappssmtp.com header.b="AIO2BBgw"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id ABBFA8716D; Thu, 9 Nov 2023 11:06:16 +0100 (CET) Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) (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 3DB3D87075 for ; Thu, 9 Nov 2023 11:06:14 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=baylibre.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=mkorpershoek@baylibre.com Received: by mail-wm1-x32a.google.com with SMTP id 5b1f17b1804b1-40859c466efso4347305e9.3 for ; Thu, 09 Nov 2023 02:06:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1699524373; x=1700129173; darn=lists.denx.de; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:from:to:cc:subject:date:message-id:reply-to; bh=1O0fWiOIxy25xqJ0GOGF6cIfEBXDXLOlq7RLODXbD9A=; b=AIO2BBgw7M3nzSAfrFvDQc4byN36dGj7PRKH2lJXPgjd/zjIwXC+sihAN3+Iwz0I2L jwhm73sRna4GPq2huMcVA9BAtXH6JfrZ+FjxcG2TZXVbJECU17xix5CN44T70s967weL wNaShUrb8lQ/HKAB81N0LLv1iOM/fZ/aTA3meF7Gxi199zmY8fDvFEk1TwUGt6WOxmU3 jrATcTg6qPVFdOtY2yBoWjBXeuYJYtdxzK1jZC1X/RhHzW5SUcSOBheo4O1lyHAOzAhP ZxrtZXRwb0P5HcheWF9LIpC//hncN1NV3HG3lycTTDZRRAdNbyPKWYiUdu76uYlPI9mr ONpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699524373; x=1700129173; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=1O0fWiOIxy25xqJ0GOGF6cIfEBXDXLOlq7RLODXbD9A=; b=avZAXK0u7OShMVdi6rq0hBCRnfOxN1VJw3s9NdiODt/c99Wbu6da2dtBDyvKJAtjH3 h+AtmKhldFXzmqWH8cGhV8AJe6bTgfNhwIe6OlYIDmjkjz2Ky5SE8qfAKx7FX4LSZ8oO J9eBCRQvfso5sLnUFthzrHIFIpHJcvrl1JAWQzHKgYzXaajCgWsoM+01i9m6aDoXqQ9C cS666WCqbeot0bxV7iqWDljucxQ+Rv5CnEL1DcF3ok6WrNaB9s9CJWyUTbhcYZ9Aw9dY S4FRRUXm3wvy5cU3CUjZzRm6k9zvQ7L+PTbBTAL7InfyZIQccNv8/HLhadzuCrqQJiJO Dhsg== X-Gm-Message-State: AOJu0YyQFYW9kcWTNXD2gGkugeoSS3hRGqpIUTkjpN3I9ttbq+qr5rr5 T/7xTJ+8ZcLTpiImouXd3Ezp7w== X-Google-Smtp-Source: AGHT+IEHBD+q9CWNzIUUCQy/oFbjUIClAMYYUbKdXGBO6hFWM8Yt4DTa5h/nhoWsjOsTdgPn1m8iOQ== X-Received: by 2002:a05:600c:4f52:b0:409:295:9c6e with SMTP id m18-20020a05600c4f5200b0040902959c6emr3900695wmq.30.1699524373598; Thu, 09 Nov 2023 02:06:13 -0800 (PST) Received: from localhost ([82.66.159.240]) by smtp.gmail.com with ESMTPSA id l19-20020a05600c4f1300b0040772138bb7sm1605274wmq.2.2023.11.09.02.06.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Nov 2023 02:06:13 -0800 (PST) From: Mattijs Korpershoek To: Dmitrii Merkurev , u-boot@lists.denx.de Cc: rammuthiah@google.com, Dmitrii Merkurev , Eugeniu Rosca , Ying-Chun Liu , Simon Glass , Sean Anderson , Cody Schuffelen Subject: Re: [PATCH 1/2] cmd: bcb: support various block device interfaces for BCB command In-Reply-To: <87a5rn9sdm.fsf@baylibre.com> References: <20231109003634.577152-1-dimorinny@google.com> <20231109003634.577152-2-dimorinny@google.com> <87a5rn9sdm.fsf@baylibre.com> Date: Thu, 09 Nov 2023 11:06:12 +0100 Message-ID: <874jhv9r6z.fsf@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain 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 On jeu., nov. 09, 2023 at 10:40, Mattijs Korpershoek wrote: > Hi Dmitrii, > > Thank you for your patch. > > Thank you as well for taking the time to upstream things from: > https://android.googlesource.com/platform/external/u-boot/ > > On jeu., nov. 09, 2023 at 00:36, Dmitrii Merkurev wrote: > >> Currently BCB command-line, C APIs and implementation only >> support MMC interface. Extend it to allow various block >> device interfaces. >> >> Signed-off-by: Dmitrii Merkurev >> Cc: Eugeniu Rosca >> Cc: Ying-Chun Liu (PaulLiu) >> Cc: Simon Glass >> Cc: Mattijs Korpershoek >> Cc: Sean Anderson >> Cc: Cody Schuffelen > > This breaks vim3/vim3l android boot flow because both rely on the usage > of the bcb command in their U-Boot environment: > > Error: 1572575691 110528528: read failed (-19) > Warning: BCB is corrupted or does not exist > > The following diff fixes it: > > diff --git a/include/configs/meson64_android.h b/include/configs/meson64_android.h > index c0e977abb01f..442233e827d8 100644 > --- a/include/configs/meson64_android.h > +++ b/include/configs/meson64_android.h > @@ -164,7 +164,7 @@ > "fi; " \ > "fi;" \ > "if test \"${run_fastboot}\" -eq 0; then " \ > - "if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \ > + "if bcb load mmc " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \ > CONTROL_PARTITION "; then " \ > "if bcb test command = bootonce-bootloader; then " \ > "echo BCB: Bootloader boot...; " \ > @@ -195,7 +195,7 @@ > "echo Recovery button is pressed;" \ > "setenv run_recovery 1;" \ > "fi; " \ > - "if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \ > + "if bcb load mmc " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \ > CONTROL_PARTITION "; then " \ > "if bcb test command = boot-recovery; then " \ > "echo BCB: Recovery boot...; " \ > diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h > index 4e5aa74147d4..f571744adaf8 100644 > --- a/include/configs/ti_omap5_common.h > +++ b/include/configs/ti_omap5_common.h > @@ -168,7 +168,7 @@ > "mmc dev $mmcdev; " \ > "mmc rescan; " \ > AB_SELECT_SLOT \ > - "if bcb load " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \ > + "if bcb load mmc " __stringify(CONFIG_FASTBOOT_FLASH_MMC_DEV) " " \ > CONTROL_PARTITION "; then " \ > "setenv ardaddr -; " \ > "if bcb test command = bootonce-bootloader; then " \ > > Can you consider including this as part of this patch ? > >> --- >> cmd/Kconfig | 1 - >> cmd/bcb.c | 70 ++++++++++++++++++++++-------------- >> doc/android/bcb.rst | 34 +++++++++--------- >> drivers/fastboot/fb_common.c | 2 +- >> include/bcb.h | 5 +-- >> 5 files changed, 65 insertions(+), 47 deletions(-) >> >> diff --git a/cmd/Kconfig b/cmd/Kconfig >> index df6d71c103..ce2435bbb8 100644 >> --- a/cmd/Kconfig >> +++ b/cmd/Kconfig >> @@ -981,7 +981,6 @@ config CMD_ADC >> >> config CMD_BCB >> bool "bcb" >> - depends on MMC >> depends on PARTITIONS >> help >> Read/modify/write the fields of Bootloader Control Block, usually >> diff --git a/cmd/bcb.c b/cmd/bcb.c >> index 02d0c70d87..5d8c232663 100644 >> --- a/cmd/bcb.c >> +++ b/cmd/bcb.c >> @@ -25,6 +25,7 @@ enum bcb_cmd { >> BCB_CMD_STORE, >> }; >> >> +static enum uclass_id bcb_uclass_id = UCLASS_INVALID; >> static int bcb_dev = -1; >> static int bcb_part = -1; >> static struct bootloader_message bcb __aligned(ARCH_DMA_MINALIGN) = { { 0 } }; >> @@ -53,6 +54,9 @@ static int bcb_is_misused(int argc, char *const argv[]) >> >> switch (cmd) { >> case BCB_CMD_LOAD: >> + if (argc != 3 && argc != 4) >> + goto err; >> + break; >> case BCB_CMD_FIELD_SET: >> if (argc != 3) >> goto err; >> @@ -115,7 +119,7 @@ static int bcb_field_get(char *name, char **fieldp, int *sizep) >> return 0; >> } >> >> -static int __bcb_load(int devnum, const char *partp) >> +static int __bcb_load(const char *iface, int devnum, const char *partp) >> { >> struct blk_desc *desc; >> struct disk_partition info; >> @@ -123,14 +127,14 @@ static int __bcb_load(int devnum, const char *partp) >> char *endp; >> int part, ret; >> >> - desc = blk_get_devnum_by_uclass_id(UCLASS_MMC, devnum); >> + desc = blk_get_dev(iface, devnum); >> if (!desc) { >> ret = -ENODEV; >> goto err_read_fail; >> } >> >> /* >> - * always select the USER mmc hwpart in case another >> + * always select the first hwpart in case another >> * blk operation selected a different hwpart >> */ >> ret = blk_dselect_hwpart(desc, 0); >> @@ -161,18 +165,20 @@ static int __bcb_load(int devnum, const char *partp) >> goto err_read_fail; >> } >> >> + bcb_uclass_id = desc->uclass_id; >> bcb_dev = desc->devnum; >> bcb_part = part; >> - debug("%s: Loaded from mmc %d:%d\n", __func__, bcb_dev, bcb_part); >> + debug("%s: Loaded from %s %d:%d\n", __func__, iface, bcb_dev, bcb_part); >> >> return CMD_RET_SUCCESS; >> err_read_fail: >> - printf("Error: mmc %d:%s read failed (%d)\n", devnum, partp, ret); >> + printf("Error: %s %d:%s read failed (%d)\n", iface, devnum, partp, ret); >> goto err; >> err_too_small: >> - printf("Error: mmc %d:%s too small!", devnum, partp); >> + printf("Error: %s %d:%s too small!", iface, devnum, partp); >> goto err; >> err: >> + bcb_uclass_id = UCLASS_INVALID; >> bcb_dev = -1; >> bcb_part = -1; >> >> @@ -182,15 +188,23 @@ err: >> static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, >> char * const argv[]) >> { >> + int devnum; >> char *endp; >> - int devnum = simple_strtoul(argv[1], &endp, 0); >> + char *iface = "mcc"; > > Should this be mmc instead of mcc ? Just to clarify: having "mmc" instead of "mcc" also fixes the bcb reading on vim3. > >> + >> + if (argc == 4) { >> + iface = argv[1]; >> + argc--; >> + argv++; >> + } >> >> + devnum = simple_strtoul(argv[1], &endp, 0); >> if (*endp != '\0') { >> printf("Error: Device id '%s' not a number\n", argv[1]); >> return CMD_RET_FAILURE; >> } >> >> - return __bcb_load(devnum, argv[2]); >> + return __bcb_load(iface, devnum, argv[2]); >> } >> >> static int __bcb_set(char *fieldp, const char *valp) >> @@ -298,7 +312,7 @@ static int __bcb_store(void) >> u64 cnt; >> int ret; >> >> - desc = blk_get_devnum_by_uclass_id(UCLASS_MMC, bcb_dev); >> + desc = blk_get_devnum_by_uclass_id(bcb_uclass_id, bcb_dev); >> if (!desc) { >> ret = -ENODEV; >> goto err; >> @@ -317,7 +331,7 @@ static int __bcb_store(void) >> >> return CMD_RET_SUCCESS; >> err: >> - printf("Error: mmc %d:%d write failed (%d)\n", bcb_dev, bcb_part, ret); >> + printf("Error: %d %d:%d write failed (%d)\n", bcb_uclass_id, bcb_dev, bcb_part, ret); >> >> return CMD_RET_FAILURE; >> } >> @@ -328,11 +342,11 @@ static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, >> return __bcb_store(); >> } >> >> -int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp) >> +int bcb_write_reboot_reason(const char *iface, int devnum, char *partp, const char *reasonp) >> { >> int ret; >> >> - ret = __bcb_load(devnum, partp); >> + ret = __bcb_load(iface, devnum, partp); >> if (ret != CMD_RET_SUCCESS) >> return ret; >> >> @@ -385,21 +399,23 @@ static int do_bcb(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) >> U_BOOT_CMD( >> bcb, CONFIG_SYS_MAXARGS, 1, do_bcb, >> "Load/set/clear/test/dump/store Android BCB fields", >> - "load - load BCB from mmc :\n" >> - "bcb set - set BCB to \n" >> - "bcb clear [] - clear BCB or all fields\n" >> - "bcb test - test BCB against \n" >> - "bcb dump - dump BCB \n" >> - "bcb store - store BCB back to mmc\n" >> + "load - load BCB from :\n" >> + "load - load BCB from mmc :\n" >> + "bcb set - set BCB to \n" >> + "bcb clear [] - clear BCB or all fields\n" >> + "bcb test - test BCB against \n" >> + "bcb dump - dump BCB \n" >> + "bcb store - store BCB back to \n" >> "\n" >> "Legend:\n" >> - " - MMC device index containing the BCB partition\n" >> - " - MMC partition index or name containing the BCB\n" >> - " - one of {command,status,recovery,stage,reserved}\n" >> - " - the binary operator used in 'bcb test':\n" >> - " '=' returns true if matches the string stored in \n" >> - " '~' returns true if matches a subset of 's string\n" >> - " - string/text provided as input to bcb {set,test}\n" >> - " NOTE: any ':' character in will be replaced by line feed\n" >> - " during 'bcb set' and used as separator by upper layers\n" >> + " - storage device interface (virtio, mmc, etc)\n" >> + " - storage device index containing the BCB partition\n" >> + " - partition index or name containing the BCB\n" >> + " - one of {command,status,recovery,stage,reserved}\n" >> + " - the binary operator used in 'bcb test':\n" >> + " '=' returns true if matches the string stored in \n" >> + " '~' returns true if matches a subset of 's string\n" >> + " - string/text provided as input to bcb {set,test}\n" >> + " NOTE: any ':' character in will be replaced by line feed\n" >> + " during 'bcb set' and used as separator by upper layers\n" >> ); >> diff --git a/doc/android/bcb.rst b/doc/android/bcb.rst >> index 8861608300..2226517d39 100644 >> --- a/doc/android/bcb.rst >> +++ b/doc/android/bcb.rst >> @@ -41,23 +41,25 @@ requirements enumerated above. Below is the command's help message:: >> bcb - Load/set/clear/test/dump/store Android BCB fields >> >> Usage: >> - bcb load - load BCB from mmc : >> - bcb set - set BCB to >> - bcb clear [] - clear BCB or all fields >> - bcb test - test BCB against >> - bcb dump - dump BCB >> - bcb store - store BCB back to mmc >> + bcb load - load BCB from : >> + load - load BCB from mmc : >> + bcb set - set BCB to >> + bcb clear [] - clear BCB or all fields >> + bcb test - test BCB against >> + bcb dump - dump BCB >> + bcb store - store BCB back to >> >> Legend: >> - - MMC device index containing the BCB partition >> - - MMC partition index or name containing the BCB >> - - one of {command,status,recovery,stage,reserved} >> - - the binary operator used in 'bcb test': >> - '=' returns true if matches the string stored in >> - '~' returns true if matches a subset of 's string >> - - string/text provided as input to bcb {set,test} >> - NOTE: any ':' character in will be replaced by line feed >> - during 'bcb set' and used as separator by upper layers >> + - storage device interface (virtio, mmc, etc) >> + - storage device index containing the BCB partition >> + - partition index or name containing the BCB >> + - one of {command,status,recovery,stage,reserved} >> + - the binary operator used in 'bcb test': >> + '=' returns true if matches the string stored in >> + '~' returns true if matches a subset of 's string >> + - string/text provided as input to bcb {set,test} >> + NOTE: any ':' character in will be replaced by line feed >> + during 'bcb set' and used as separator by upper layers >> >> >> 'bcb'. Example of getting reboot reason >> @@ -91,7 +93,7 @@ The following Kconfig options must be enabled:: >> >> CONFIG_PARTITIONS=y >> CONFIG_MMC=y >> - CONFIG_BCB=y >> + CONFIG_CMD_BCB=y >> >> .. [1] https://android.googlesource.com/platform/bootable/recovery >> .. [2] https://source.android.com/devices/bootloader >> diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c >> index 4e9d9b719c..2a6608b28c 100644 >> --- a/drivers/fastboot/fb_common.c >> +++ b/drivers/fastboot/fb_common.c >> @@ -105,7 +105,7 @@ int __weak fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) >> if (reason >= FASTBOOT_REBOOT_REASONS_COUNT) >> return -EINVAL; >> >> - return bcb_write_reboot_reason(mmc_dev, "misc", boot_cmds[reason]); >> + return bcb_write_reboot_reason("mmc", mmc_dev, "misc", boot_cmds[reason]); >> } >> >> /** >> diff --git a/include/bcb.h b/include/bcb.h >> index 5edb17aa47..a6326523c4 100644 >> --- a/include/bcb.h >> +++ b/include/bcb.h >> @@ -9,10 +9,11 @@ >> #define __BCB_H__ >> >> #if IS_ENABLED(CONFIG_CMD_BCB) >> -int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp); >> +int bcb_write_reboot_reason(const char *iface, int devnum, char *partp, const char *reasonp); >> #else >> #include >> -static inline int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp) >> +static inline int bcb_write_reboot_reason(const char *iface, int devnum, >> + char *partp, const char *reasonp) >> { >> return -EOPNOTSUPP; >> } >> -- >> 2.42.0.869.gea05f2083d-goog