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 E41D8C6FD1D for ; Mon, 27 Mar 2023 16:25:48 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7CB7A85CB5; Mon, 27 Mar 2023 18:25:38 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=amazon.de 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=amazon.de header.i=@amazon.de header.b="S3kPv6RX"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 0AEA785BC5; Mon, 27 Mar 2023 17:19:27 +0200 (CEST) Received: from smtp-fw-80006.amazon.com (smtp-fw-80006.amazon.com [99.78.197.217]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id DCB0C83623 for ; Mon, 27 Mar 2023 17:19:21 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=quarantine dis=none) header.from=amazon.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=prvs=443c9913a=ptyadav@amazon.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.de; i=@amazon.de; q=dns/txt; s=amazon201209; t=1679930363; x=1711466363; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=brGzytSLTq9kru0fwfLYnqNMvgmYDXbYHZN4Jst6g3A=; b=S3kPv6RXD8C2RBc5pzVfc/u2Fh9nIr0Rl/lb/y6GQV4t+kAdq76kxXdO BNXfdEhHNFwIodX211Xcx/Qxb+BYsGB9kaKNl+SwZjuDpNoMQyi8P9HB/ KZE1S/YLd8JKFfCup2Y61i1Pvu7A8PKIjCVlcnkBtPEB6cDVzxcDEJh3M w=; X-IronPort-AV: E=Sophos;i="5.98,295,1673913600"; d="scan'208";a="197883270" Received: from pdx4-co-svc-p1-lb2-vlan2.amazon.com (HELO email-inbound-relay-iad-1e-m6i4x-b538c141.us-east-1.amazon.com) ([10.25.36.210]) by smtp-border-fw-80006.pdx80.corp.amazon.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Mar 2023 15:19:16 +0000 Received: from EX19MTAUWB002.ant.amazon.com (iad12-ws-svc-p26-lb9-vlan2.iad.amazon.com [10.40.163.34]) by email-inbound-relay-iad-1e-m6i4x-b538c141.us-east-1.amazon.com (Postfix) with ESMTPS id 819B5A25BA; Mon, 27 Mar 2023 15:19:13 +0000 (UTC) Received: from EX19MTAUWC001.ant.amazon.com (10.250.64.145) by EX19MTAUWB002.ant.amazon.com (10.250.64.231) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.25; Mon, 27 Mar 2023 15:19:12 +0000 Received: from dev-dsk-ptyadav-1c-37607b33.eu-west-1.amazon.com (10.15.11.255) by mail-relay.amazon.com (10.250.64.145) with Microsoft SMTP Server id 15.2.1118.25 via Frontend Transport; Mon, 27 Mar 2023 15:19:12 +0000 Received: by dev-dsk-ptyadav-1c-37607b33.eu-west-1.amazon.com (Postfix, from userid 23027615) id CF8C920E17; Mon, 27 Mar 2023 17:19:10 +0200 (CEST) From: Pratyush Yadav To: Dhruva Gole CC: Jagan Teki , Vignesh , Vaishnav Achath , , "Apurva Nandan" Subject: Re: [PATCH 1/2] spi: cadence-quadspi: Fix check condition for DTR ops References: <20230323164408.1043725-1-d-gole@ti.com> <20230323164408.1043725-2-d-gole@ti.com> Date: Mon, 27 Mar 2023 17:19:10 +0200 In-Reply-To: <20230323164408.1043725-2-d-gole@ti.com> (Dhruva Gole's message of "Thu, 23 Mar 2023 22:14:07 +0530") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Mailman-Approved-At: Mon, 27 Mar 2023 18:25:32 +0200 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 Thu, Mar 23 2023, Dhruva Gole wrote: > buswidth and dtr fields in spi_mem_op are only valid when the > corresponding spi_mem_op phase has a non-zero length. For example, Right. > SPI NAND core doesn't set buswidth when using SPI_MEM_OP_NO_ADDR > phase. > > Fix the dtr checks in set_protocol() to ignore empty spi_mem_op > phases, as checking for dtr field in empty phase will result in > false negatives. > > Signed-off-by: Apurva Nandan > Signed-off-by: Dhruva Gole > --- > drivers/spi/cadence_qspi.c | 11 +++++++++-- > drivers/spi/cadence_qspi_apb.c | 9 ++++++++- > 2 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c > index c7f10c501320..a858a62888e4 100644 > --- a/drivers/spi/cadence_qspi.c > +++ b/drivers/spi/cadence_qspi.c > @@ -362,8 +362,15 @@ static bool cadence_spi_mem_supports_op(struct spi_slave *slave, > { > bool all_true, all_false; > > - all_true = op->cmd.dtr && op->addr.dtr && op->dummy.dtr && > - op->data.dtr; > + /* > + * op->dummy.dtr is required for converting nbytes into ncycles. > + * Also, don't check the dtr field of the op phase having zero nbytes. > + */ > + all_true = op->cmd.dtr && > + (!op->addr.nbytes || op->addr.dtr) && > + (!op->dummy.nbytes || op->dummy.dtr) && > + (!op->data.nbytes || op->data.dtr); Looks good! > + > all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && > !op->data.dtr; Since we treat DTR as "do not care" when the phase does not exist, you should check for that here as well. What if someone _sets_ DTR for a field with nbytes == 0? This check would fail. Since no one reasonable should do this, I will not insist upon this. Fine by me if you don't do this. Your choice. > > diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c > index 21fe2e655c5f..2b04b58124a5 100644 > --- a/drivers/spi/cadence_qspi_apb.c > +++ b/drivers/spi/cadence_qspi_apb.c > @@ -120,7 +120,14 @@ static int cadence_qspi_set_protocol(struct cadence_spi_priv *priv, > { > int ret; > > - priv->dtr = op->data.dtr && op->cmd.dtr && op->addr.dtr; > + /* > + * For an op to be DTR, cmd phase along with every other non-empty > + * phase should have dtr field set to 1. If an op phase has zero > + * nbytes, ignore its dtr field; otherwise, check its dtr field. > + */ > + priv->dtr = op->cmd.dtr && > + (!op->addr.nbytes || op->addr.dtr) && > + (!op->data.nbytes || op->data.dtr); Why not check dummy? Since supports_op() already checks that _all_ or _none_ of the fields are DTR, you can get away with just checking for op->cmd.dtr here (do add a comment here or it can be a bit confusing). BTW, I think it would be better if you get rid of cadence_qspi_set_protocol() entirely. I see no point in carrying the state around. Wherever you use priv->dtr or priv->inst_width, etc. you also have access to the spi_mem_op. You can derive that information from the op. Something to fix when you have some free time on your hands. Will make the driver a bit simpler. > > ret = cadence_qspi_buswidth_to_inst_type(op->cmd.buswidth); > if (ret < 0) > -- > 2.25.1 > -- Regards, Pratyush Yadav Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879