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 507EDC54798 for ; Thu, 7 Mar 2024 19:40:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Y5cpwQj6T+3SkcwO/b4JHmP3SEQt/GC++7ip9+X2zKo=; b=OHF55XjZmQ9V+8 OHT6F10Slfcd8gR5+w+DI5Fv6yvjNV42FlFJ7nYfsuAoFUwurbvxth4DbjopQPbVdUzOCxWbMN/E7 GO/wah+ojA3GQwmCqfZlEnC+PH75wH5YFQB/kXAaIBJtPBdzyhX8ug7fijEocZdHGpz1RN1C4AvgP 4B9EKUQ/jKSlUEtzgxq4dfoFX947zNUS9GSLGcwMUFlxLyTIJBFZcuGMYPkMDE5I4Do+CyZau2cP1 GwQheOeovbG3utiUOPGPKAg2DcNP1uFX/SzGRF6XUaMFwFQC3PsvmMSE4qSGDvWg+ghZpoEk/SCUv fStShakb8pC6Wxu/LOoA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1riJbd-0000000679r-46WJ; Thu, 07 Mar 2024 19:40:41 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1riJba-00000006799-1PCE for linux-arm-kernel@lists.infradead.org; Thu, 07 Mar 2024 19:40:40 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9751B1FB; Thu, 7 Mar 2024 11:41:09 -0800 (PST) Received: from e130802.arm.com (unknown [10.57.70.143]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9BE883F762; Thu, 7 Mar 2024 11:40:29 -0800 (PST) Date: Thu, 7 Mar 2024 19:40:26 +0000 From: Abdellatif El Khlifi To: Mathieu Poirier Cc: Bjorn Andersson , Rob Herring , Liviu Dudau , Sudeep Holla , Lorenzo Pieralisi , Krzysztof Kozlowski , Conor Dooley , Drew.Reed@arm.com, Adam.Johnston@arm.com, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-remoteproc@vger.kernel.org Subject: Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver Message-ID: <20240307194026.GA355455@e130802.arm.com> References: <20240301164227.339208-1-abdellatif.elkhlifi@arm.com> <20240301164227.339208-2-abdellatif.elkhlifi@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240307_114038_549452_D19B2BFA X-CRM114-Status: GOOD ( 22.77 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Mathieu, > > + do { > > + state_reg = readl(priv->reset_cfg.state_reg); > > + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg); > > + > > + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) { > > + dev_err(dev, "unexpected RST_ACK value: 0x%x\n", > > + *rst_ack); > > + return -EINVAL; > > + } > > + > > + /* expected ACK value read */ > > + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack)) > > I'm not sure why the second condition in this if() statement is needed. As far > as I can tell the first condition will trigger and the second one won't be > reached. The second condition takes care of the following: exp_ack and *rst_ack are both 0. This case happens when RST_REQ bit is cleared (meaning: No reset requested) and we expect the RST_ACK to be 00 afterwards. > > +/** > > + * arm_rproc_load() - Load firmware to memory function for rproc_ops > > + * @rproc: pointer to the remote processor object > > + * @fw: pointer to the firmware > > + * > > + * Does nothing currently. > > + * > > + * Return: > > + * > > + * 0 for success. > > + */ > > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw) > > +{ > > What is the point of doing rproc_of_parse_firmware() if the firmware image is > not loaded to memory? Does the remote processor have some kind of default ROM > image to run if it doesn't find anything in memory? Yes, the remote processor has a default FW image already loaded by default. rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename provided. Please correct me if I'm wrong. [1]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947 [2]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863 > > +module_platform_driver(arm_rproc_driver); > > + > > I am echoing Krzysztof view about how generic this driver name is. This has to > be related to a family of processors or be made less generic in some way. Have > a look at what TI did for their K3 lineup [1] - I would like to see the same > thing done here. Thank you, I'll take care of that and of all the other comments made. Cheers, Abdellatif _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel