From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161785AbcBQQOl (ORCPT ); Wed, 17 Feb 2016 11:14:41 -0500 Received: from ni.piap.pl ([195.187.100.4]:50829 "EHLO ni.piap.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161048AbcBQQOj (ORCPT ); Wed, 17 Feb 2016 11:14:39 -0500 From: khalasa@piap.pl (Krzysztof =?utf-8?Q?Ha=C5=82asa?=) To: Arnd Bergmann Cc: linux-arm-kernel@lists.infradead.org, Felipe Balbi , Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Felipe Balbi , Haojian Zhuang , Daniel Mack , Imre Kaloz , Robert Jarzmik Subject: Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio References: <1453997722-3489596-1-git-send-email-arnd@arndb.de> <3062977.Ya2ztQYFaM@wuerfel> <2926671.iEYNiy920m@wuerfel> Date: Wed, 17 Feb 2016 17:14:35 +0100 In-Reply-To: <2926671.iEYNiy920m@wuerfel> (Arnd Bergmann's message of "Wed, 17 Feb 2016 11:36:49 +0100") Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-KLMS-Rule-ID: 1 X-KLMS-Message-Action: clean X-KLMS-AntiSpam-Lua-Profiles: 91452 [Feb 17 2016] X-KLMS-AntiSpam-Version: 5.5.9.33 X-KLMS-AntiSpam-Envelope-From: khalasa@piap.pl X-KLMS-AntiSpam-Rate: 0 X-KLMS-AntiSpam-Status: not_detected X-KLMS-AntiSpam-Method: none X-KLMS-AntiSpam-Moebius-Timestamps: 3967722, 3967764, 3966760 X-KLMS-AntiSpam-Info: LuaCore: 414 414 652141367f63b6038f5642fa97308d3a9d2edb46, Auth:dkim=none X-KLMS-AntiSpam-Interceptor-Info: scan successful X-KLMS-AntiPhishing: Clean, 2016/02/17 09:15:04 X-KLMS-AntiVirus: Kaspersky Security 8.0 for Linux Mail Server, version 8.0.1.721, bases: 2016/02/17 04:09:00 #7185775 X-KLMS-AntiVirus-Status: Clean, skipped Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Arnd Bergmann writes: > Ok, so I guess what this means is that ixp4xx (or xscale in general) > implements its big-endian mode by adding a byteswap on its DRAM > and PCI interfaces in be32 mode, rather than by changing the behavior of > the load/store operations (as be8 mode does) or by having the byteswap > in its load/store pipeline or the top-level AHB bridge? Hmm... IIRC, there is normally no swapping on DRAM bus. E.g. if you write 0x12345678 to RAM and change endianness, it will still read as 0x12345678. The CPU will still be able to execute opcodes after switching endianness, but byte-oriented data will be messed up. PCI swaps in BE mode, so byte order is preserved but readl() must "unswap" it. > I'm still unsure about > __indirect_readsl()/ioread32_rep()/insl()/readsl(). Indirect ops should behave the same as direct. I think I have tested them at some point. The "string" operations don't have to swap (on PCI) because the PCI bus controller does it for them (in BE mode). > insl() does a double-swap on big-endian, which seems right, as we > end up with four swaps total, preserving correct byte order. static inline void insl(u32 io_addr, void *p, u32 count) { u32 *vaddr = p; while (count--) *vaddr++ = le32_to_cpu(inl(io_addr)); } inl() does indirect input (preserving value, not byte order), so there seem to be just one swap here (le32_to_cpu) preserving byte order. > __raw_readsl() performs no swap, which would be correct for PCI > (same swap on PCI and RAM, so byteorder is preserved), No, a single swap on PCI, this means the byte order is preserved :-) > but wrong > for on-chip FIFO registers (one swap on RAM, no swap on MMIO). But there aren't any such registers. Basically, almost all registers are 32-bit, even if they only hold an 8-bit value. Exceptions such as 16550 UARTs are taken care of in platform structs (using offset = 3). > However, when CONFIG_IXP4XX_INDIRECT_PCI is set, both > ioread32_rep() and readsl() call __indirect_readsl(), which > in turn swaps the data once, so I think we actually need this patch: > > diff --git a/arch/arm/mach-ixp4xx/include/mach/io.h b/arch/arm/mach-ixp4xx/include/mach/io.h > @@ -100,7 +102,7 @@ static inline void __indirect_writesw(volatile void __iomem *bus_addr, > const u16 *vaddr = p; > > while (count--) > - writew(*vaddr++, bus_addr); > + writew((u16 __force)cpu_to_le32(*vaddr++), bus_addr); > } ... > Does that make sense to you? This is essentially the same thing we already > do for inw/inl/outw/outl. Well, we may need something like this. It seems writesw() (and thus __indirect_writesw()) etc. use le16 values (preserving byte order), so the above should probably use le16_to_cpu() instead (and le32_to_cpu in __indirect_writesl()). I think the only thing that can use it on my hw is VIA PATA adapter (throught ioread32_rep() etc). I will have to dig it up as well. I wouldn't rather touch this stuff without verifying that it fixes things up. Thanks for looking into this. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland