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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 60AFBC3DA61 for ; Sun, 21 Jul 2024 09:00:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=1wS5BV/pANVkHBAjVAsLZ6TmEgjFKIzBgugnxT9gPrA=; b=OVtMVraV6t4tK3jTWXR30k9GMj JSpRJcJHpcGlYQWXHaxEY7R+3RBJoynzjYrBMqkZNQfifsmwyn0IQKEFRpMCASwLwopzQ0SDpbPvc h9gWtcASn93sji4KrcUiwAJ1h8no14XPCdvAfdoU0lv9NZ5ele4YCEE1D3gKG6xGZwNboyLHcrMAv JaIGtcHGf8477NWzUrXQ2ngL30K6+udbww/TaOn1CAaCHvvGRybX6N9V+ampLQ9kCj/FxquMlD9sF POIspMEX02bMEjAq6knapdIMP9sfTYYX4MAnxz40DQQMJkcGqXx6eBDnnvtnd4jHdoQP5+awdo8V0 oTf+OYPw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sVSPy-00000006W4B-0hfN; Sun, 21 Jul 2024 08:59:46 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sVSPa-00000006VzE-398J for linux-arm-kernel@lists.infradead.org; Sun, 21 Jul 2024 08:59:24 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 130D960AE4; Sun, 21 Jul 2024 08:59:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B3661C116B1; Sun, 21 Jul 2024 08:59:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1721552358; bh=ZuuqEi4S2iKFuJKwkxBMs+ZU/Sd67LLMFp02L/gM/5w=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=DC8sa8JW/xfOr2xbyypcs6KARdNf2mwJ0nWHTkrd1Rv0zrwMyGpHu3lqFR2BGvL0a uxtd0IifDa7kdKSG51qzjAn/bA7BcfdOzLWUiR60M0ijTyF2O0HbYoMxi2I/J5ukBn FlbRiwW4wQxrOziEa6wzEMpeyKkbQVgxcInKZ2hK76X4C/TjIM8LMIdV/KAKW7arCv bRIN0c9LRMctfPisVrMiPq4M3tNznh90IsvpKI+tYmuWjqRT2gDzS7ZwDoDdLvYJCw Bx7ADf0vTPjuk4AVG7qA12YYDkwxepFmpB27Crh4WuCLIhRIfFFvHyYa8TE9krCp/2 OQHVcf/7UcPjw== Date: Sun, 21 Jul 2024 09:59:12 +0100 From: Simon Horman To: Ayush Singh Cc: jkridner@beagleboard.org, robertcnelson@beagleboard.org, "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Nishanth Menon , Vignesh Raghavendra , Tero Kristo , Johan Hovold , Alex Elder , Greg Kroah-Hartman , greybus-dev@lists.linaro.org, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API Message-ID: <20240721085912.GB715661@kernel.org> References: <20240719-beagleplay_fw_upgrade-v1-0-8664d4513252@beagleboard.org> <20240719-beagleplay_fw_upgrade-v1-3-8664d4513252@beagleboard.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240719-beagleplay_fw_upgrade-v1-3-8664d4513252@beagleboard.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240721_015922_951008_AFDE7A26 X-CRM114-Status: GOOD ( 31.61 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Jul 19, 2024 at 03:15:12PM +0530, Ayush Singh wrote: > Register with firmware upload API to allow updating firmware on cc1352p7 > without resorting to overlay for using the userspace flasher. > > Communication with the bootloader can be moved out of gb-beagleplay > driver if required, but I am keeping it here since there are no > immediate plans to use the on-board cc1352p7 for anything other than > greybus (BeagleConnect Technology). Additionally, there do not seem to > any other devices using cc1352p7 or it's cousins as a co-processor. > > Boot and Reset GPIOs are used to enable cc1352p7 bootloader backdoor for > flashing. The delays while starting bootloader are taken from the > userspace flasher since the technical specification does not provide > sufficient information regarding it. > > Flashing is skipped in case we are trying to flash the same > image as the one that is currently present. This is determined by CRC32 > calculation of the supplied firmware and Flash data. > > We also do a CRC32 check after flashing to ensure that the firmware was > flashed properly. > > Link: https://www.ti.com/lit/ug/swcu192/swcu192.pdf Ti CC1352p7 Tecnical Specification > Link: https://openbeagle.org/beagleconnect/cc1352-flasher Userspace > Flasher > > Signed-off-by: Ayush Singh Hi Ayush, A few minor points from my side. ... > diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c ... > +/** > + * enum cc1352_bootloader_cmd: CC1352 Bootloader Commands nit: As this is a kernel doc, it should probably include documentation of the elements of the enum: * @COMMAND_DOWNLOAD: ... ... Flagged by W=1 allmodconfig builds and ./scripts/kernel-doc -none > + */ > +enum cc1352_bootloader_cmd { > + COMMAND_DOWNLOAD = 0x21, > + COMMAND_GET_STATUS = 0x23, > + COMMAND_SEND_DATA = 0x24, > + COMMAND_RESET = 0x25, > + COMMAND_CRC32 = 0x27, > + COMMAND_BANK_ERASE = 0x2c, > +}; > + > +/** > + * enum cc1352_bootloader_status: CC1352 Bootloader COMMAND_GET_STATUS response Likewise here. > + */ > +enum cc1352_bootloader_status { > + COMMAND_RET_SUCCESS = 0x40, > + COMMAND_RET_UNKNOWN_CMD = 0x41, > + COMMAND_RET_INVALID_CMD = 0x42, > + COMMAND_RET_INVALID_ADR = 0x43, > + COMMAND_RET_FLASH_FAIL = 0x44, > +}; ... > +/** > + * csum8: Calculate 8-bit checksum on data Similarly, as this is a kernel doc it should probably include documentation of of the function parameters. > + */ > +static u8 csum8(const u8 *data, size_t size, u8 base) > +{ > + size_t i; > + u8 sum = base; > + > + for (i = 0; i < size; ++i) > + sum += data[i]; > + > + return sum; > +} ... > +/** > + * cc1352_bootloader_empty_pkt: Calculate the number of empty bytes in the current packet Likewise here. > + */ > +static size_t cc1352_bootloader_empty_pkt(const u8 *data, size_t size) > +{ > + size_t i; > + > + for (i = 0; i < size && data[i] == 0xff; ++i) > + continue; > + > + return i; > +} ... > +static int gb_fw_init(struct gb_beagleplay *bg) > +{ > + int ret; > + struct fw_upload *fwl; > + struct gpio_desc *desc; > + > + bg->fwl = NULL; > + bg->boot_gpio = NULL; > + bg->rst_gpio = NULL; > + bg->flashing_mode = false; > + bg->fwl_cmd_response = 0; > + bg->fwl_ack = 0; > + init_completion(&bg->fwl_ack_com); > + init_completion(&bg->fwl_cmd_response_com); > + > + desc = devm_gpiod_get(&bg->sd->dev, "boot", GPIOD_IN); > + if (IS_ERR(desc)) > + return PTR_ERR(fwl); fwl is not initialised here, and it is desc that is tested for error. Perhaps this should be: return PTR_ERR(desc); Flagged by Smatch and Coccinelle. > + bg->boot_gpio = desc; > + > + desc = devm_gpiod_get(&bg->sd->dev, "reset", GPIOD_IN); > + if (IS_ERR(desc)) { > + ret = PTR_ERR(desc); > + goto free_boot; > + } > + bg->rst_gpio = desc; > + > + fwl = firmware_upload_register(THIS_MODULE, &bg->sd->dev, "cc1352p7", > + &cc1352_bootloader_ops, bg); > + if (IS_ERR(fwl)) { > + ret = PTR_ERR(fwl); > + goto free_reset; > + } > + bg->fwl = fwl; > + > + return 0; > + > +free_reset: > + devm_gpiod_put(&bg->sd->dev, bg->rst_gpio); > + bg->rst_gpio = NULL; > +free_boot: > + devm_gpiod_put(&bg->sd->dev, bg->boot_gpio); > + bg->boot_gpio = NULL; > + return ret; > +} > + > +static void gb_fw_deinit(struct gb_beagleplay *bg) > +{ > + firmware_upload_unregister(bg->fwl); > +} > + > static int gb_beagleplay_probe(struct serdev_device *serdev) > { > int ret = 0; > @@ -481,6 +1077,10 @@ static int gb_beagleplay_probe(struct serdev_device *serdev) > if (ret) > goto free_serdev; > > + ret = gb_fw_init(bg); > + if (ret) > + goto free_hdlc; > + > ret = gb_greybus_init(bg); > if (ret) > goto free_hdlc; I suspect this error path will leak resources allocated by gb_fw_init(). > @@ -500,6 +1100,7 @@ static void gb_beagleplay_remove(struct serdev_device *serdev) > { > struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev); > > + gb_fw_deinit(bg); > gb_greybus_deinit(bg); > gb_beagleplay_stop_svc(bg); > hdlc_deinit(bg); > > -- > 2.45.2 > >