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 36B8FC77B73 for ; Fri, 26 May 2023 15:13:33 +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: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:References: List-Owner; bh=QsvH71pnPgGVfdxDnqJ0/2X6tctBwlWSrBaauuRxOww=; b=034FnN8mwMDtTi Fw98TVGVlVLxo6A9ttfI5HroMMU4dIdBzSci2EUtcaIOsXNAFn1EdmRMqmyOeHJCkFIy33UOIbhTk TumPYwwN9U1wXp99znlHIu/nDreFN086Jv2DHRmEIqS3VMxDUCu2fKaKl/GGcvmMDzKdOucuf9u49 r935nHtYZQwwtx60PgB1EvpCM0SwBooMsmW1+c1ozlMp8ZVxGynfYFFVawL0FsN8uoPOdR8S3fZVK cJFatemlwPaMLNvyPK10YuwoNe2F3axWKF0ajRyPj5AwoK5thGICWEZGjK0Dxy/l3NXxKq1cmlOnX MkaBAkCnMOVLXQ5yU5oQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q2Z7t-002vju-1Y; Fri, 26 May 2023 15:13:09 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1q2Z7q-002vjD-2z for linux-arm-kernel@lists.infradead.org; Fri, 26 May 2023 15:13:08 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E7BD0650C7; Fri, 26 May 2023 15:13:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id EC43CC433EF; Fri, 26 May 2023 15:13:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1685113985; bh=jllZZGrA4gsgP+ByBARANyOtGQ+rmmd98Jzv7HPlTtM=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=leU2fLqF7TAy3ZQfMA/VaRbD4uDhEmiX15IqnWZOCCw4uRGfKNvDiF6+p5zj0yG8f Mrmqi3WVOv4zmpTSvLnDUZ0BGWhcdpM8Rkw7WvCilgqM6pVBBbcMrUX21h5GIStTqJ buqU3Rn9DcoiydEaUPXWzX8zRrd30EhbYgM4CKrQl3zi2ObESsisLnyMjYwFMIvoaq NvgX0RkFvjAzJTsNIxsjxVQSEIldKRxLcbM8T5xqQuqcO6W8O35CJTIjnaOMS6y4qs /TlxqQyBAb8jhugZoGv5rPyp+ZZrg3iZ6SKPs31tTORSpgeiv/iJJ8WvnLKw6dICXm P/blECHss4aFg== Date: Fri, 26 May 2023 17:13:00 +0200 From: Simon Horman To: Ahmad Fatoum Cc: Mark Brown , Shawn Guo , Sascha Hauer , jiada wang , linux-spi@vger.kernel.org, NXP Linux Team , Pengutronix Kernel Team , Fabio Estevam , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] spi: imx: correct handling of MXC_CSPIRXDATA value endianness Message-ID: 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-20230526_081307_011791_FB9CF75C X-CRM114-Status: GOOD ( 24.55 ) 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 On Fri, May 26, 2023 at 04:09:48PM +0200, Ahmad Fatoum wrote: > Hello Simon, > > On 26.05.23 16:03, Simon Horman wrote: > > The existing code seems be intended to handle MXC_CSPIRXDATA values > > which are in big endian. However, it seems that this is only > > handled correctly in the case where the host is little endian. > > > > First, consider the read case. > > > > u32 val = be32_to_cpu(readl(...)) > > > > readl() will read a 32bit value and return it after applying le32_to_cpu(). > > On a little endian host le32_to_cpu() is a noop. So the raw value is > > returned. This is then converted from big endian to host byte-order - > > the value is byte-swapped - using be32_to_cpu(). Assuming the raw value > > is big endian a host byte-order value is obtained. This seems correct. > > > > However, on a big endian system, le32_to_cpu() will perform a byte-swap, > > while be32_to_cpu() is a noop. Assuming the underlying value is big > > endian this is incorrect, because it should not be byte-swapped to > > obtain the value in host byte-order - big endian. > > > > Surveying other kernel code it seems that a correct approach is: > > > > be32_to_cpu((__force __be32)__raw_readl(...)) > > How about using ioread32be? ... On Fri, May 26, 2023 at 04:12:46PM +0200, Ahmad Fatoum wrote: > On 26.05.23 16:09, Ahmad Fatoum wrote: > >> - writel(val, spi_imx->base + MXC_CSPITXDATA); > >> + __raw_writel((__force u32)cpu_to_be32(val), > >> + spi_imx->base + MXC_CSPITXDATA); > >> } > > On more thing: __raw_writel doesn't involve a write barrier (at least > on ARM). That means above code introduces a bug as the CPU may now reorder > writes that were sequential before. Both iowrite32be() and readl() > have a __iowmb(); on ARM before doing the write itself. Thanks Ahmad, I agree that ioread32be() and iowrite32be() look like a better solution. I'll plan to spin a v2 accordingly. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel