From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755112AbcHXUGB (ORCPT ); Wed, 24 Aug 2016 16:06:01 -0400 Received: from mout.kundenserver.de ([212.227.126.134]:49197 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751521AbcHXUFC (ORCPT ); Wed, 24 Aug 2016 16:05:02 -0400 From: Arnd Bergmann To: Rich Felker Cc: Daniel Lezcano , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sh@vger.kernel.org, Rob Herring , Mark Rutland , Thomas Gleixner , Marc Zyngier Subject: Re: [PATCH v6 2/2] clocksource: add J-Core timer/clocksource driver Date: Wed, 24 Aug 2016 22:01:08 +0200 Message-ID: <1909939.rYmWMrdImI@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-31-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <20160824174001.GW15995@brightrain.aerifal.cx> References: <57BDCE5D.2050609@linaro.org> <20160824174001.GW15995@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:my+Ynsso9NwdX/AzzNBMxWVIPs9PiudKruJAO+GTwU5miEFwxIm TE3Or0B8f26LQ+GNJ3tCFh90EwLdQAl2R5/9oKogAb84LAMYrUUW1d7PnUYwZ18teqt78Va VRgzMCq9WO17T4pCEqUNdIKbNgOoGOTrwJBpMdgfmxD38R9TpJuP7hPm74dlNDuy2iVEKwr E1Sn81Ors7iI/Zt+KLdtw== X-UI-Out-Filterresults: notjunk:1;V01:K0:cyDge/Q5DzI=:6PGYsRMD+C1Cf/laGJ78nG jyhaLgQYc4z1RHvGbfAvPguUxQZXcWRW1Mfthnk/2Ik9Qgmkgos1knaGaGSuTx4nwzAKyu6wA 6D825PCLwxgCKVrKH5toGrX1V5cR5Uo8lTlvSWhCJxfxJecSiXnhrC9lUtHo3oU+aZvcOTGyx bhprB+2rBfnsF4Xm0kUY0kmu68XeiIX846lirBUcqW16elwMTM1rKvm7dBj5mzDGqDy8K3PYq XR95pYKwAeDf70XhJcP2lrKCCRKdiicK+ngU+TUtQDgQxpt7UdiF2YVgFZDQx3An+KK0WcRXJ J7f3N/MRdYGdqNFUfqfGVJ5n57ewL5StaNLih7/WosQ1CiY7qU57qHXkI35litUGxo8CYVxfC pU46fysIV/CLWfkRZ9d+K1N4eS3k5gEWos6qxFVZeIjHI4GPoNJL++jdRLYcxgSuG87llhDgw M6KJULfEn9tVkFEQr+IXmy7qXEAMMaTcKxj8iwpni4vyPF/UXQkJo2eNl4u/OBc+y/Ee6qM+P L9GR6+P0SIk47KvWXGts6vXybfCZ9gzwn9ar+RkhskC6w8S4idY1mYrHh4HiIxyvLjXq1YNQL RRBimtwUp3ACJ11HIspJdgMy3HCyXsxrw71KrLyiv7k9434rcQNn0svUIMw8Du+9RCo6i2Qa1 JILHDqomwxlKxl+7O6dTaqLtoCxcClRV31N5KcI+HeFKBl0ZhNIW8DevJBDQmJM84a0u7riem K7KQTFv0LK5M+nob Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday, August 24, 2016 1:40:01 PM CEST Rich Felker wrote: > On Wed, Aug 24, 2016 at 06:42:05PM +0200, Daniel Lezcano wrote: > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > > index 5677886..3210ca5 100644 > > > --- a/drivers/clocksource/Kconfig > > > +++ b/drivers/clocksource/Kconfig > > > @@ -407,6 +407,15 @@ config SYS_SUPPORTS_SH_TMU > > > config SYS_SUPPORTS_EM_STI > > > bool > > > > > > +config CLKSRC_JCORE_PIT > > > + bool "J-Core PIT timer driver" > > > + depends on OF && (SUPERH || COMPILE_TEST) > > > > Even if this is correct, for the sake of consistency, it is preferable > > to change it to: > > > > bool "J-Core PIT timer driver" if COMPILE_TEST > > depends on SUPERH > > select CLKSRC_OF > > Is this functionally equivalent? If so that's non-obvious to me. What > about the dropped OF dependency? The intent is that the option should > always be available for SUPERH targets using OF, otherwise only > available with COMPILE_TEST. I think your version is correct here, Daniel made a mistake with the SUPERH dependency. Another way to write it would be config CLKSRC_JCORE_PIT bool "J-Core PIT timer driver" if COMPILE_TEST && !JCORE depends on OF default JCORE to make it impossible to disable when JCORE is set, but user-visible on all other architectures, SUPERH or otherwise. > > > +static int jcore_pit_set(unsigned long delta, struct jcore_pit *pit) > > > +{ > > > + jcore_pit_disable(pit); > > > + __raw_writel(delta, pit->base + REG_THROT); > > > + __raw_writel(pit->enable_val, pit->base + REG_PITEN); > > > + return 0; > > > > Why do you need to use __raw_writel ? > > > > s/__raw_writel/writel/ > > I actually tried multiple times to find good resources on policy for > which form to prefer, but didn't have much luck. My understanding is > that __raw_writel/__raw_readl always performs a native-endian > load/store, whereas writel/readl behavior depends on cpu endianness > and whether the arch declares that "pci bus" (that was the term I > found in the source, not relevant here) io is endian-swapped or not. > Can you give me a better explanation of why we might prefer one form > or the other? In general, a portable driver should use readl/writel because they are defined to have the behavior that x86 provides, which is what most developers understand best. Compared to __raw_readl/__raw_writel, it guarantees - correct endianess (in most cases, exceptions see below) - does not leak out of spinlocks - accesses are word-sized, the compiler cannot split them into byte accesses, or merge subsequent accesses into larger words - ordering against dma_map_*/dma_unmap_*/dma_sync_* is maintained The __raw_* accessors are used as building blocks for readl/outl/ioread32/ioread32_be/readl_relaxed/... and they can usually be used safely on device RAM such as framebuffers but not much else in portable code. Some architectures also use the __raw_* accessors for MMIO registers, but that code is generally not portable. Endianess is always more complicated than one thinks, and it's handled differently across architectures, sometimes because of hardware differences, sometimes for historic reasons that differ only in the Linux implementation. A large majority of devices are fixed to little-endian MMIO registers (as recommended by PCI), so that is a good default. Exceptions to this are: * Some architectures (e.g. PowerPC) typically run big-endian code, and hardware designers decided to make the MMIO registers the same. In this case, you can generally use ioread32_be/iowrite32_be. * Some devices have configurable endianess with a device specific register. Sometimes those registers are set to match the CPU endianess, but for Linux it tends to be better to just set them to little-endian and use readl/writel * Some drivers are configurable at HW synthesis time, and we have a mix of devices. If you have this, you usually need to support both ways in the driver with a helper function that uses ioread32 or ioread32_be depending on the configuration. This can be done using either compile-time or runtime configuration. * Some SoCs have boot-time configurable endianess and enforce that MMIO registers and the CPU always use the same way. We are inconsistent in handling those, I'd recommend creating a header file for that SoC that defines vendor specific helper functions. * Some (usually very old) SoCs have configurable endianess, but don't actually change the behavior of the SoC or the MMIO devices, but only flip the address lines on the memory interface. This is similar to the previous one, but has additional problems with FIFO registers. You generally cannot use portable drivers here. Arnd