Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels
On Saturday 13 July 2013, Gerhard Sittig wrote: [ MPC8308 knowledge required, see below ] On Sat, Jul 13, 2013 at 09:17 +0200, Arnd Bergmann wrote: On Friday 12 July 2013, Gerhard Sittig wrote: +++ b/include/dt-bindings/dma/mpc512x-dma.h @@ -0,0 +1,21 @@ +/* + * This header file provides symbolic specifiers for DMA channels + * within the MPC512x SoC's DMA controller. Since requester lines + * directly map to channel numbers and no additional flexibility + * is involved, DMA channels can be considered directly associated + * with individual peripherals. + * + * This header file gets shared among DT bindings which provide + * hardware specs, and driver code which implements supporting logic. + */ + +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H +#define _DT_BINDINGS_DMA_MPC512x_DMA_H + +#define MPC512x_DMACHAN_SCLPC 26 +#define MPC512x_DMACHAN_SDHC 30 +#define MPC512x_DMACHAN_MDDRC 32 + +#define MPC512x_DMACHAN_MAX64 + I think these should not be in the header and should not bve part of the binding either. They are specific to an SoC that happens to be using this DMA controller but would be completely different for any other SoC with the same DMA engine. These belong into the dma descriptors of the slave drivers and don't need symbolic names. Thank you for the feedback. OK, so not adding the dt-bindings header leads to no change in the DTS nodes, which in turn collapses 5/8 into something local to the .c driver source (introduce an enum and replace a few magic numbers with names), and obsoletes 4/8 as a prerequisite. This will further reduce the patch set's size. Actually I think you will need extra changes: The dma-engine driver should not require knowledge of any channel-specific settings. I did not notice you had them until you mentioned the above, but from what I can tell, you need a few flags in the dma-specifier to replace code like /* only start explicitly on MDDRC channel */ - if (cid == 32) + if (cid == MPC512x_DMACHAN_MDDRC) mdesc-tcd-start = 1; with mdesc-tcd-start = dmaspec-explicit_start; or something along these lines, where dmaspec is a data structure derived from the fields in the DT dma specifier of the child node. I scanned chapter 12 (DMA controller) in the MPC8308 reference manual (rev 0 as of 2010-04) several times and could not find any hint about peripherals, request lines, or anything else related to flow control. Searching in the whole RM won't give a hint either. Does this suggest that the MPC8308 DMA controller's channels are free in their assignment to transfer tasks? Or are they memory transfers only? Or do they happily accept any XLB address (internal and external RAM, IMMR and IP bus space) but don't apply flow control, i.e. expect either peripherals to already hold the RX data, or peripherals to keep up with being fed random amounts of TX data? I tend to read the doc as the latter. It sounds to me that they are memory-to-memory only, which means you probably want to allow #dma-cells=0 as a special case to describe an instance that has no slave API support. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V3 1/3] dts: change Marvell prefix to 'marvell'
On Thursday 11 July 2013, Neil Zhang wrote: To do this properly, the drivers are going to have to be compatible with the old and the new names, and the binding docs updated to reflect the legacy name and the preferred name. Properly would be as above. You can stop using stock tickers for new company names, but anything that has been defined in a device tree before has to stay that way, and all the manufacturer prefixes to device names should be the same. What you're proposing is purely driver bloat and increasing the size of kernel. *I'm* not proposing to change the name, Neil is. I'm proposing that iff they chose to do that, don't break sh*t along the way. What's your opinion? We discussed the topic of fixing bad bindings vs. keeping backwards compatibility during last week's Linaro Connect. The main outcome was that we need to have a better review for new bindings getting merged to avoid this situation in the future, but the general consensus seems that bindings that are already in use but were never reviewed properly (which should have caught this) should be changed. We will likely establish an annotation in the binding soon to mark those that can not be changed as opposed to those that are not considered stable yet. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH RFC 5/8] dma: mpc512x: use symbolic specifiers for DMA channels
On Friday 12 July 2013, Gerhard Sittig wrote: +++ b/include/dt-bindings/dma/mpc512x-dma.h @@ -0,0 +1,21 @@ +/* + * This header file provides symbolic specifiers for DMA channels + * within the MPC512x SoC's DMA controller. Since requester lines + * directly map to channel numbers and no additional flexibility + * is involved, DMA channels can be considered directly associated + * with individual peripherals. + * + * This header file gets shared among DT bindings which provide + * hardware specs, and driver code which implements supporting logic. + */ + +#ifndef _DT_BINDINGS_DMA_MPC512x_DMA_H +#define _DT_BINDINGS_DMA_MPC512x_DMA_H + +#define MPC512x_DMACHAN_SCLPC 26 +#define MPC512x_DMACHAN_SDHC 30 +#define MPC512x_DMACHAN_MDDRC 32 + +#define MPC512x_DMACHAN_MAX64 + I think these should not be in the header and should not bve part of the binding either. They are specific to an SoC that happens to be using this DMA controller but would be completely different for any other SoC with the same DMA engine. These belong into the dma descriptors of the slave drivers and don't need symbolic names. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V3 2/3] clocksource: mmp: support CLOCKSOURCE OF DECLARE
On Tuesday 09 July 2013, Thomas Petazzoni wrote: Dear Neil Zhang, On Tue, 9 Jul 2013 14:42:45 +0800, Neil Zhang wrote: support CLOCKSOURCE OF DECLARE for mmp timer. Signed-off-by: Neil Zhang zhan...@marvell.com --- arch/arm/mach-mmp/mmp-dt.c |5 ++--- arch/arm/mach-mmp/mmp2-dt.c |3 +-- arch/arm/mach-mmp/time.c| 15 ++- 3 files changed, 5 insertions(+), 18 deletions(-) Maybe it would be good to take this opportunity to move arch/arm/mach-mmp/time.c into drivers/clocksource/. +1 Or we might want to have a more coordinated move of all clocksource drivers in arch/arm to drivers/clocksource now, as we have done for some other subsystems. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V3 3/3] ARM: mmp: bring up pxa988 with device tree support
On Tuesday 09 July 2013, Neil Zhang wrote:
+ soc {
+ compatible = simple-bus;
+ #address-cells = 1;
+ #size-cells = 1;
+ interrupt-parent = gic;
+ ranges;
+
+ gic: interrupt-controller@d1dfe100 {
+ compatible = arm,cortex-a9-gic;
+ #interrupt-cells = 3;
+ #address-cells = 1;
+ interrupt-controller;
+ reg = 0xd1dff000 0x1000,
+ 0xd1dfe100 0x0100;
+ };
+
+ L2: l2-cache-controller@d1dfb000 {
+ compatible = arm,pl310-cache;
+ reg = 0xd1dfb000 0x1000;
+ arm,data-latency = 2 1 1;
+ arm,tag-latency = 2 1 1;
+ arm,pwr-dynamic-clk-gating;
+ arm,pwr-standby-mode;
+ cache-unified;
+ cache-level = 2;
+ };
+
+ local-timer@d1dfe600 {
+ compatible = arm,cortex-a9-twd-timer;
+ reg = 0xd1dfe600 0x20;
+ interrupts = 1 13 0x304;
+ };
+
+ axi@d420 { /* AXI */
+ compatible = simple-bus;
+ #address-cells = 1;
+ #size-cells = 1;
+ ranges = 0xd420 0xd420 0x0020;
+
+ intc: wakeupgen@d4282000 {
+ compatible = marvell,mmp-intc;
+ reg = 0xd4282000 0x1000;
+ marvell,intc-wakeup = 0x114 0x3
+ 0x144 0x3;
+ };
+ };
I am guessing that the structure does not actually reflect the hardware.
Shouldn't AXI be the top-level bus, with the other stuff under it?
+
+
+ uart1: uart@d4017000 {
+ compatible = marvell,mmp-uart;
+ reg = 0xd4017000 0x1000;
+ interrupts = 0 27 0x4;
+ status = disabled;
+ };
The uart node should be called serial@d4017000 instead of uart@d4017000.
diff --git a/arch/arm/mach-mmp/reset.c b/arch/arm/mach-mmp/reset.c
new file mode 100644
index 000..b90ec54
--- /dev/null
+++ b/arch/arm/mach-mmp/reset.c
@@ -0,0 +1,66 @@
+/*
+ * linux/arch/arm/mach-mmp/reset.c
I think this could just be part of the smp.c file.
+ *
+ * Author: Neil Zhang zhan...@marvell.com
+ * Copyright:(C) 2012 Marvell International Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include linux/kernel.h
+#include linux/smp.h
+
+#include asm/io.h
+#include asm/cacheflush.h
+#include asm/mach/map.h
+
+#include mach/addr-map.h
+
+#include reset.h
+
+#define PMU_CC2_AP APMU_REG(0x0100)
+#define CIU_CA9_WARM_RESET_VECTORCIU_REG(0x00d8)
You should not hardcode the addresses here, better find them from the device
tree.
+
+#define CPU_CORE_RST(n) (1 ((n) * 4 + 16))
+#define CPU_DBG_RST(n) (1 ((n) * 4 + 18))
+#define CPU_WDOG_RST(n) (1 ((n) * 4 + 19))
This should probably go into a reset controller driver, in drivers/reset/
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 4/4] ARM: reinsert ARCH_MULTI_V4 Kconfig option
On Monday 08 July 2013, Jonas Jensen wrote: Arnd suggested removing one (or many) of the options: CPU_ABRT_EV4T CPU_CACHE_V4WT CPU_COPY_V4WB CPU_TLB_V4WBI However boot is still broken after removing all including CPU_32v4T. Selecting CPU_SA1100 instead, it boots normally and init works. The idea is, If you can help figure out what exactly breaks boot, the patch can be dropped. From these tests, I have created two branches ( arch/arm/mm have replacements for calls to no longer compiled in ): http://code.google.com/p/linux-next-20130703-moxart/source/detail?r=a88f33ad22d3f9a31fe126f1c95568e5b2e8f32dname=multi_V4_test_arm920t http://code.google.com/p/linux-next-20130703-moxart/source/detail?r=9f4c21f1123a122c51792cd374217ce6e426e74aname=multi_V4_test_sa1100 It looks like you select CPU_ARM926T but change the entry for CPU_ARM920T instead, which would have no effect then. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v6 00/21] MBus DT binding: PCIe strikes back
On Friday 05 July 2013, Ezequiel Garcia wrote:
See the previous version of this patchset for further context:
http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg35753.html
This new proposal is an attempt to address some issues raised about the PCIe
'fake' windows mapping present in the previous version.
Instead of defining a 'fake' MBUS_ID(0xf0, 0x02) region for the whole
PCIe memory and IO space, we use real target ID and attribute for the windows.
Hi Ezequiel,
This looks really nice, and I is getting very close to where I think it
needs to be headed. I'll have to do a more thorough review, but for now there
is only one detail that I think you should change (we can argue about that):
pcie-controller {
compatible = marvell,armada-xp-pcie;
status = okay;
device_type = pci;
#address-cells = 3;
#size-cells = 2;
ranges =
0x8200 0 0x4 MBUS_ID(0xf0, 0x01)
0x4 0 0x2000 /* Port 0.0 registers */
0x8200 0 0x42000 MBUS_ID(0xf0, 0x01)
0x42000 0 0x2000 /* Port 2.0 registers */
0x8200 0 0x44000 MBUS_ID(0xf0, 0x01)
0x44000 0 0x2000 /* Port 0.1 registers */
0x8200 0 0x48000 MBUS_ID(0xf0, 0x01)
0x48000 0 0x2000 /* Port 0.2 registers */
0x8200 0 0x4c000 MBUS_ID(0xf0, 0x01)
0x4c000 0 0x2000 /* Port 0.3 registers */
0x82000800 0 0xe000 MBUS_ID(0x04, 0xe8)
0xe000 0 0x0800 /* Port 0.0 MEM */
0x81000800 0 0 MBUS_ID(0x04, 0xe0)
0xe800 0 0x0010 /* Port 0.0 IO */;
From all I can tell, this is a correct representation of the
translation windows. The one change you made from what I suggested
(either intentionally or because I didn't make myself clear enough)
is that you keep encoding the aperture in the ranges property of
the pcie node.
0x82000800 0 0xe000 MBUS_ID(0x04, 0xe8) 0xe000 0 0x0800 /*
Port 0.0 MEM */
^^^
This describes the intended setting of the aperture in the mbus, not
the translation that is performed by the pcie-controller as I suggested.
That would look like
0x82000800 0 0 MBUS_ID(0x04, 0xe8) 0 1 0 /* Port 0.0 MEM */
iow the first 4GB of the 0x82000800 memory space get translated to 4GB
at MBUS_ID(0x04, 0xe8), and the aperture is applied by translating a
small section of that into host mmio space.
The advantage of your approach is that you can keep the existing code
that extracts the aperture (0xe000-0xe800) from the
pcie-controller node, and you don't have to find out the aperture
from separate DT property or from looking at the remaining available
address space in mbus.
The disadvantage is that you have mbus policy spread out between the
ranges properties of the mbus node and the pcie node, rather than
having the pcie-controller node just describe the hardware capabilities.
This is actually more of a problem for the I/O space, which is still not
correctly represented:
0x81000800 0 0 MBUS_ID(0x04, 0xe0) 0xe800 0 0x0010
is not what the hardware does, the hardware probably has something
like
0x81000800 0 0 MBUS_ID(0x04, 0xe0) 0 1 0
i.e. the PIO registers are at the start of the 4GB MBUS window and
you want them to get mapped at 0xe800, but that is no business
of the PCIe node. Since the addresses for I/O space are fixed (unlike
memory space, which is sized dynamically per port), it would be
straightforward to put the mappings into the mbus node:
MBUS_ID(0x04, 0xe0) 0x0 0xe800 0x1
MBUS_ID(0x04, 0xe1) 0x1 0xe801 0x1
MBUS_ID(0x04, 0xe2) 0x2 0xe802 0x1
Unfortunately, the number of mapping windows in mbus is very limited
on Armada XP, so you'd run out of windows too fast by mapping them
all at boot time. The options here are to either put them into the
ranges property anyway but get the mbus driver to not map them by
default, or to encode the policy in a different way. Note that the
physical addresses do not actually have to be contiguous any more now,
so one option would be for mbus to just fill the holes whenever
the pcie driver needs an I/O space window, but there we get close
to the fully dynamic model again.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v6 00/21] MBus DT binding: PCIe strikes back
On Saturday 06 July 2013, Jason Gunthorpe wrote: This is a good try, but this coding doesn't work... Recall the long discussion that came up during the original development of this binding. The OF spec says this: In particular, the phys.hi fields of the child address spaces in the ranges property for PCI does not contain the same information as reg property entries within PCI nodes. The only information that is present in ranges phys.hi entries are the non-relocatable, prefetchable and the PCI address space bits for which the en- try applies. I.e., only the n, p and ss bits are present; the , d, fff and fields are 0. When an address is to be mapped through a PCI bus bridge node, the phys.hi value of the address to be mapped and the child field of a ranges entry should be masked so that only the ss bits are compared. I.e., the only portion of phys.hi that should participate in the range determination is the address space indicator (the ss bits). Which forbids (0x82000800 .. ..) from being in a ranges ah, and I thought Ezequiel was being more clever than what I had suggested I don't have an idea how to encode MBUS_ID in the PCI-E ranges :( Arnd? Didn't you have some idea? The way I described it during the last review, I suggested using the middle cell of the PCI address here. Since the PCIe port is 32 bit only, it's otherwise unused. We can do one of two things here: a) put the port number in the second cell 0x8200 1 0xe000 MBUS_ID(0x04, 0xe8) 0xe000 0 0x0800 /* Port 0.0 MEM */ 0x8200 2 0xe000 MBUS_ID(0x04, 0xe9) 0xe000 0 0x0800 /* Port 1.0 MEM */ b) translate the entire MBUS space here 0x8200 0 0 0 0 0x100 0 /* all MBUS_IDs */ In both cases, the individual ports would have another non-empty ranges property a) 0x8200 1 0xe000 0x8200 0 0xe000 0x0800 /* ranges for port 0.0 */ b) 0x8200 MBUS_ID(0x04, 0xe8) 0xe000 0x8200 0 0xe000 0 0x0800 I left out the change I suggested in my other reply to not describe the aperture in this node but translate the entire 4GB space. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v6 00/21] MBus DT binding: PCIe strikes back
On Saturday 06 July 2013, Thomas Petazzoni wrote: Arnd, Jason, if you could confirm that you both agree with this DT binding soon, Ezequiel and I would quickly adapt the code accordingly, and hopefully converge towards a final patch set. Looks all good from what I can tell. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: Appended DTB files for multi-machine kernels
On Thursday 04 July 2013, Mark Brown wrote: On Thu, Jul 04, 2013 at 06:56:24PM +0200, Daniel Mack wrote: Unless I missed some recent discussion, this case is not easy to handle. Yes, I know that these kind of things should be handled by a next-generation bootloader, but in our case, we want to avoid a loader update of already shipped hardware by all means. There was some discussion about appending multiple DTBs recently. I can't actually recall anything about it though so that's not an entirely helpful thing... Yes, it keeps coming up, I think by now everybody agreed it's a good idea to extend the ATAGS compat mode, but so far nobody has implemented it. Magnus Damm was very interested in this feature last year and was planning to work on it, but I don't know how far he got. As a solution, I'm thinking of a small framework that could for example work as follows. a) A small mechanism allows storing multiple DTB binary files inside the kernel binary at compile time, and a simple function can extract them again by name at runtime (something like what the firmware framework does, but I don't know if that one can be used at such an early stage in the boot process). Another way of skinning this would be for either the kernel to contain a set of machine ID to compatible string mappings or for the device trees for the boards to have an additional properties giving the machine IDs that the boards match. The kernel could then look for multiple DTBs appended to the image and try to pick one based on ATAGs. IIRC there is actually an unused field in the dtb header that could be used to match a board ID. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 26 June 2013, Wolfram Sang wrote:
On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
This patch makes the SDA hold time configurable through device tree.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
Applied to for-next, thanks for keeping at it and providing lots of
useful information. Much appreciated!
Sorry, but I got a regression that I didn't find reported elsewhere
so far, even though it breaks a lot of the ARM defconfig builds:
drivers/built-in.o: In function `dw_i2c_probe':
/git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined
reference to `__udivdi3'
I suspect you want something like the change below.
Signed-off-by: Arnd Bergmann a...@arndb.de
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index def79b5..57e3a07 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
if (pdev-dev.of_node) {
u32 ht = 0;
u32 ic_clk = dev-get_clk_rate_khz(dev);
+ u64 hold_time;
of_property_read_u32(pdev-dev.of_node,
i2c-sda-hold-time-ns, ht);
- dev-sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
+ hold_time = (u64)ic_clk * ht + 50;
+ do_div(hold_time, 100);
+ dev-sda_hold_time = hold_time;
}
dev-functionality =
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 00/12] MBus DT binding: A new hope
On Tuesday 02 July 2013, Jason Gunthorpe wrote: On Sat, Jun 29, 2013 at 04:04:03PM -0300, Ezequiel Garcia wrote: Personally, I can't see any disadvantage, and the binding looks much cleaner now. But of course I can be wrong, and I'm open to discussion. There's another pending issue. Arnd Bergmann has required to add a property to specify the available space within the CPU address space for decoding windows. This property would allow to support fully dynamic mbus window allocation. I wonder if this range is implied - eg it is address space not covered by the memory node or any mbus ranges? Is there a situation where that is not sufficient? I think it's sufficient, yes. It feels odd to have the mbus code rely on the contents of the memory node for that, but it would work. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 0/4] mtd:nand:omap2: clean-up of supported ECC schemes
On Tuesday 02 July 2013, Pekon Gupta wrote: (+ CC: devicetree-discuss@lists.ozlabs.org) Changes v3 - v4 - [Patch 1/3] removed MTD_NAND_OMAP_BCH8 MTD_NAND_OMAP_BCH4 from nand/Kconfig ECC scheme selectable via nand DT (nand-ecc-opt). - [*] rebased for l2-mtd.git Do you also fix these build error? /git/arm-soc/drivers/mtd/nand/omap2.c: In function 'omap_nand_probe': /git/arm-soc/drivers/mtd/nand/omap2.c:1964:7: error: 'OMAP_ECC_BCH8_CODE_HW_DETECTION_SW' undeclared (first use in this function) case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW: ^ /git/arm-soc/drivers/mtd/nand/omap2.c:1964:7: note: each undeclared identifier is reported only once for each function it appears in /git/arm-soc/drivers/mtd/nand/omap2.c:1991:7: error: 'OMAP_ECC_BCH4_CODE_HW_DETECTION_SW' undeclared (first use in this function) case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: ^ Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 0/4] mtd:nand:omap2: clean-up of supported ECC schemes
On Wednesday 03 July 2013, Artem Bityutskiy wrote: On Wed, 2013-07-03 at 13:16 +, Gupta, Pekon wrote: [Pekon]: Yes, I'm not seeing these build issues, as I'm cleanly returning from probe with pr_err(), if the required libraries (/lib/bch.c) are not build-in the system. --- [Patch v4 1/4]: mtd:nand:omap2: clean-up BCHx_HW and BCHx_SW ECC.. @@static int omap_nand_probe(struct platform_device *pdev) + default: + pr_err(selected ECC scheme not supported or not enabled\n); + err = -EINVAL; + goto out_release_mem_region; + } --- However, if you are still seeing this, could you please send me your config? I compile tested your patches too, and did not see any issues with my omap2_defconfig. To clarify: I'm getting this error with randconfig on today's linux-next. I think it only happens with MTD_NAND_ECC_BCH enabled, which is not the default in omap2_defconfig. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote:
On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote:
On Wednesday 26 June 2013, Wolfram Sang wrote:
On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote:
This patch makes the SDA hold time configurable through device tree.
Signed-off-by: Christian Ruppert christian.rupp...@abilis.com
Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com
Applied to for-next, thanks for keeping at it and providing lots of
useful information. Much appreciated!
Sorry, but I got a regression that I didn't find reported elsewhere
so far, even though it breaks a lot of the ARM defconfig builds:
drivers/built-in.o: In function `dw_i2c_probe':
/git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined
reference to `__udivdi3'
I suspect you want something like the change below.
This looks similar to a patch Vincent Stehle submitted yesterday, see
https://lkml.org/lkml/2013/7/2/145
Thanks for the link. Actually his patch looks wrong to me, because
dev-sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100);
assigns the division remainder to sda_hold_time, not the quotient.
Arnd
Signed-off-by: Arnd Bergmann a...@arndb.de
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
b/drivers/i2c/busses/i2c-designware-platdrv.c
index def79b5..57e3a07 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -119,10 +119,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
if (pdev-dev.of_node) {
u32 ht = 0;
u32 ic_clk = dev-get_clk_rate_khz(dev);
+ u64 hold_time;
of_property_read_u32(pdev-dev.of_node,
i2c-sda-hold-time-ns, ht);
- dev-sda_hold_time = ((u64)ic_clk * ht + 50) / 100;
+ hold_time = (u64)ic_clk * ht + 50;
+ do_div(hold_time, 100);
+ dev-sda_hold_time = hold_time;
}
dev-functionality =
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote: On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote: On Wednesday 03 July 2013, Christian Ruppert wrote: On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote: On Wednesday 26 June 2013, Wolfram Sang wrote: On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote: This patch makes the SDA hold time configurable through device tree. Signed-off-by: Christian Ruppert christian.rupp...@abilis.com Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Applied to for-next, thanks for keeping at it and providing lots of useful information. Much appreciated! Sorry, but I got a regression that I didn't find reported elsewhere so far, even though it breaks a lot of the ARM defconfig builds: drivers/built-in.o: In function `dw_i2c_probe': /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3' I suspect you want something like the change below. This looks similar to a patch Vincent Stehle submitted yesterday, see https://lkml.org/lkml/2013/7/2/145 Thanks for the link. Actually his patch looks wrong to me, because dev-sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100); assigns the division remainder to sda_hold_time, not the quotient. Hrmmm... At least when I tested it this morning on an ARC architecture it worked as intended and returned the quotient. Does that mean we have an issue with this function on ARC? Can anyone who knows these functions better than I comment? ARC just uses the generic version of div_u64, which is defined in lib/div64.c. I suspect that the division remainder just happens to work well enough for you to not cause any run-time error. You could try adding a printk in that function to show the values you get on ARC. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Christian Ruppert wrote: On Wed, Jul 03, 2013 at 04:20:03PM +0200, Arnd Bergmann wrote: On Wednesday 03 July 2013, Christian Ruppert wrote: On Wed, Jul 03, 2013 at 01:43:11PM +0200, Arnd Bergmann wrote: On Wednesday 26 June 2013, Wolfram Sang wrote: On Wed, Jun 26, 2013 at 10:55:06AM +0200, Christian Ruppert wrote: This patch makes the SDA hold time configurable through device tree. Signed-off-by: Christian Ruppert christian.rupp...@abilis.com Signed-off-by: Pierrick Hascoet pierrick.hasc...@abilis.com Applied to for-next, thanks for keeping at it and providing lots of useful information. Much appreciated! Sorry, but I got a regression that I didn't find reported elsewhere so far, even though it breaks a lot of the ARM defconfig builds: drivers/built-in.o: In function `dw_i2c_probe': /git/arm-soc/drivers/i2c/busses/i2c-designware-platdrv.c:125: undefined reference to `__udivdi3' I suspect you want something like the change below. This looks similar to a patch Vincent Stehle submitted yesterday, see https://lkml.org/lkml/2013/7/2/145 Thanks for the link. Actually his patch looks wrong to me, because dev-sda_hold_time = div_u64((u64)ic_clk * ht + 50, 100); assigns the division remainder to sda_hold_time, not the quotient. Hrmmm... At least when I tested it this morning on an ARC architecture it worked as intended and returned the quotient. Does that mean we have an issue with this function on ARC? Can anyone who knows these functions better than I comment? ARC just uses the generic version of div_u64, which is defined in lib/div64.c. I suspect that the division remainder just happens to work well enough for you to not cause any run-time error. You could try adding a printk in that function to show the values you get on ARC. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10] i2c-designware: make SDA hold time configurable
On Wednesday 03 July 2013, Stehle Vincent-B46079 wrote:
From: Christian Ruppert [mailto:christian.rupp...@abilis.com]
(..)
Although this doesn't explicitly state what the function returns to me
this sounds more like the quotient is returned rather than the remainder?
Hi,
Yes div_u64() returns the quotient.
It is defined in linux/math64.h as:
static inline u64 div_u64(u64 dividend, u32 divisor)
{
u32 remainder;
return div_u64_rem(dividend, divisor, remainder);
}
The remainder is probably fully optimized out.
Ah, you are right, sorry for the confusion on my part.
I thought you had used do_div() rather than div_u64().
Everything's fine then, feel free to add my
Acked-by: Arnd Bergmann a...@arndb.de
to your patch.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 3/4] PCI: Add driver for i.MX6 PCI Express
On Tuesday 02 July 2013 10:11:55 Pratyush Anand wrote:
On 7/2/2013 9:16 AM, Sean Cross wrote:
I may be wrong, but from these offset it seems to me that it is SNPS
controller. If yes, then please go through comments of
[PATCH V1-10 0/4] PCIe support for Samsung Exynos5440 SoC
Exynos5440 appears to use the same port logic controller. However, the
PHYs are different. I'm not exactly certain which comments you want me to
notice in that series of patchsets, but I see references to splitting
Exynos-specific code into its own project. Based on that, it seems like
the best approach would be to:
1) Move Exynos code into its own file, say, pcie-exynos.c. This would
leave only Synopsys-specific ATC code in pcie-designware.c
2) Rename generic exynos functions to reflect the fact that they're
designware-generic functions.
Ouch, I missed the fact that Jingoo Han had only renamed the file, but not
also all the function identifiers. This change needs to be done anyway.
3) Have pcie-imx.c reference this generic designware ATC code.
I'll rework the patch and re-submit it following these three changes.
Correct, Exactly these steps has to be done. But, Mohit might be doing
similar work, so it would be better to get consensus on what has to be done.
If the platform specific parts are small enough, you can actually just leave
everything in one file, and use the of_device_id data field to point
to a structure describing the differences, e.g. using function pointers.
IMO, there should be three categories of functions. May be arnd can
comment if we can do even something better.
As a general comment in case you are wondering: In a situation like this,
the common code should always be structured as a library of functions
and the hardware specific drivers become the actual driver that register
to a particular device 'compatible' string in DT.
Group I: Initially, These functions should remain in pcie-designware.c
(offcourse by changing exynos tag to dw). Latter on, we can see if some
of them can even be moved to common pci layer.
sys_to_pcie
cfg_read
cfg_write
dw_pcie_prog_viewport_cfg0
dw_pcie_prog_viewport_cfg1
dw_pcie_prog_viewport_mem_outbound
dw_pcie_prog_viewport_io_outbound
dw_pcie_rd_other_conf
dw_pcie_wr_other_conf
dw_pcie_setup
dw_pcie_valid_config
dw_pcie_rd_conf
dw_pcie_wd_conf
dw_pcie_scan_bus
dw_pcie_map_irq
dw_pcie_setup_rc
add_pcie_port (after a bit of generalization)
dw_pcie_probe
dw_pcie_remove
The probe and remove functions might need be split up into a generic
part that gets called by the hardware specific part.
Group II: These functions should still remain as dummy in
pcie-designware.c , and should be classified as __week. So, each
implementer of designware controller say Exynos/SPEAr/imx will have to
define their own function to super-seed default dummy definitions.
dw_readl_rc
dw_writel_rc
dw_pcie_rd_own_conf
dw_pcie_wr_own_conf
dw_pcie_link_up
dw_pcie_host_init (will contain all platform specific and phy
initialization)
I don't like __weak functions really, and they don't work well with loadable
modules the way I suggested above. Instead, you probably need a structure
with function pointers that gets initialized by platform specific
driver.
struct dw_pcie_host_ops {
void (*readl_rc)(struct pcie_port *pp, void *dbi_base, u32
*val);
...
};
static inline void dw_pcie_readl_rc(struct pcie_port *pp, void *dbi_base, u32
*val);
{
if (pp-ops-readl_rc)
pp-ops-readl_rc(pp, dbi_base, val);
else
*val = readl(dbi_base);
}
dw_pcie_host_init would become the probe function that calls the generic
dw_pcie_probe.
Group III: Functions specific to Exynos, which should move to pcie-exynos.c
exynos_pcie_sideband_dbi_w_mode
exynos_pcie_sideband_dbi_r_mode
exynos_pcie_assert_core_reset
exynos_pcie_deassert_core_reset
exynos_pcie_assert_phy_reset
exynos_pcie_deassert_phy_reset
exynos_pcie_init_phy
exynos_pcie_assert_reset
exynos_pcie_establish_link
@Mohit, See if you have BW then please take it further.
arnd, are exynos patches applied to any branch of arm-soc git tree?
Yes, they are currently in the next/soc branch.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V10 1/4] pci: Add PCIe driver for Samsung Exynos
On Tuesday 25 June 2013, Kukjin Kim wrote: Bjorn Helgaas wrote: On Fri, Jun 21, 2013 at 04:24:54PM +0900, Jingoo Han wrote: Exynos5440 has a PCIe controller which can be used as Root Complex. This driver supports a PCIe controller as Root Complex mode. Signed-off-by: Surendranath Gurivireddy Balla suren.re...@samsung.com Signed-off-by: Siva Reddy Kallam siva.kal...@samsung.com Signed-off-by: Jingoo Han jg1@samsung.com Acked-by: Arnd Bergmann a...@arndb.de Acked-by: Bjorn Helgaas bhelg...@google.com Please merge this through arm-soc as you discussed. Sounds great. Arnd, please take this series into arm-soc tree directly by yourself with my ack on arch/exynos stuff if you want ;) Ok, done. I've put this on top of the next/soc tree, which already contains the other PCI host controller support. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Monday 24 June 2013 16:49:08 zhangfei gao wrote: Dear Arnd Vinod The suggestion of using dma_get_slave_channel instead of filter works here. Dma driver should modify accordingly. The changes all look good to me, thanks a lot for following up! However, you should really follow the procedure for posting patches even if it's just for the sake of discussion: * always provide a signed-off-by and a patch description * use an email client that doesn't break whitespace * I would have split this up into two separate patches, one for the subsystem and one for the new driver. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v12 05/11] edma: config: Enable config options for EDMA
On Saturday 22 June 2013, Joel A Fernandes wrote:
config TI_EDMA
tristate TI EDMA support
default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2
select DMA_ENGINE
select DMA_VIRTUAL_CHANNELS
MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The
'm' option will require some initramfs to load the module when needing
to MMC boot, I suggest lets leave it as y.
Ah, right: you still export a filter function from the edma driver
and use it in slave drivers:
drivers/mmc/host/davinci_mmc.c:
dma_request_slave_channel_compat(mask, edma_filter_fn,
drivers/mmc/host/davinci_mmc.c:
dma_request_slave_channel_compat(mask, edma_filter_fn,
drivers/spi/spi-davinci.c: dspi-dma_rx = dma_request_channel(mask,
edma_filter_fn,
drivers/spi/spi-davinci.c: dspi-dma_tx = dma_request_channel(mask,
edma_filter_fn,
As long as this is the case, you have to be careful with the dependencies
to make sure that davinci_mmc and spi-davinci either depend on TI_EDMA, or
edma_filter_fn gets defined to NULL when you are building for a DT-only
platform.
Yes sure, right now they are defined as follows in include/linux/edma.h:
#if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE)
bool edma_filter_fn(struct dma_chan *, void *);
#else
static inline bool edma_filter_fn(struct dma_chan *chan, void *param)
{
return false;
}
#endif
It's best to just define the filter function in the platform
code and pass a pointer to it through platform data. The way you do
it above makes the slave drivers inherently nonportable.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v12 05/11] edma: config: Enable config options for EDMA
On Monday 24 June 2013, Joel A Fernandes wrote:
Yes sure, right now they are defined as follows in include/linux/edma.h:
#if defined(CONFIG_TI_EDMA) || defined(CONFIG_TI_EDMA_MODULE)
bool edma_filter_fn(struct dma_chan *, void *);
#else
static inline bool edma_filter_fn(struct dma_chan *chan, void *param)
{
return false;
}
#endif
It's best to just define the filter function in the platform
code and pass a pointer to it through platform data. The way you do
it above makes the slave drivers inherently nonportable.
But with DT-only platforms, can you really do that?
The nice thing about this is that with a DT-only platform, the
filter function will automatically go away: you have no
platform_data, which means that if you are using
dma_request_slave_channel_compat, you just pass NULL as the
filter and the filter-data, same as just calling
dma_request_slave_channel.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V10 0/4] PCIe support for Samsung Exynos5440 SoC
On Friday 21 June 2013, Jingoo Han wrote: Changes between v9 and v10: * Changed the file name from 'pci-designware.c' to 'pcie-designware.c' guided by Pratyush Anand, because synopsis pcie and pci controllers are different. * Fixed the typos of document, reported by Sachin Kamat. Changes between v8 and v9: * Changed the file name from 'exynos-pcie.txt' to 'designware-pcie.txt'. * Added 'snps,dw-pcie' string to compatible property. Changes between v7 and v8: * Changed the file name from 'pci-exynos.c' to 'pci-designware.c', and added a generic string for compatible property to exynos-pcie.txt * Moved pci_add_resource_offset() for I/O space to the 'if' clause * Added Arnd's Acked-by Changes between v6 and v7: * Split ARM DT patch to two patches * Fixed node naming * Added Arnd's Acked-by Ok, that takes care of all my comments. Bjorn, are you still considering to merge this for 3.11 or have you closed your tree for the merge window? I think it would be good to get it in. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V10 0/4] PCIe support for Samsung Exynos5440 SoC
On Friday 21 June 2013, Thomas Petazzoni wrote: I am by far not an expert on how to solve merge strategies and so on, but to avoid conflicts at Linus's level while merging the arm-soc and pci trees, it would be better if this Samsung PCIe driver could go through arm-soc (with Bjorn ACK, of course), so that Arnd/Olof can make sure the ordering is correct with regard to the of/pci changes and the mvebu/pci driver. Yes, good point. The alternative would be that Bjorn also takes the PCI branch dependencies that are already in arm-soc into his tree. Either way works, but I agree that what you suggest would be simpler. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Friday 21 June 2013, Vinod Koul wrote:
On Mon, Jun 17, 2013 at 10:58:07PM +0200, Arnd Bergmann wrote:
On Monday 17 June 2013, Zhangfei Gao wrote:
int dma_get_slave_channel(struct dma_chan *chan)
{
/* lock against __dma_request_channel */
mutex_lock(dma_list_mutex);
if (chan-client_count == 0)
ret = dma_chan_get(chan);
else
ret = -EBUSY;
mutex_unlock(dma_list_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(dma_get_slave_channel);
and you add filter on top?
No, the idea is to no longer require a filter function when
we use dma_request_slave_channel with a DT specifier.
This is getting you any channel and maynot work where we need to do some
filtering.
This function would be called only by the dmaengine driver's
xlate function. That driver obviously has to ensure that the
channel works for the specification from DT (or ACPI or
something else), but that part is easy, since that is
the same information that we currently pass into the filter
function.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v12 05/11] edma: config: Enable config options for EDMA
On Friday 21 June 2013, Joel A Fernandes wrote: diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index b1c66a4..7d58cd9 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -841,6 +841,7 @@ config ARCH_DAVINCI select HAVE_IDE select NEED_MACH_GPIO_H select TI_PRIV_EDMA + select DMADEVICES It is generally a bad idea to force select on something that can be enabled using menuconfig. Unless used carefully, select causes unmet direct dependency warnings which folks are already fighting hard to fix. This leads to what Russell referred in the past as select madness [1] Are you concerned with bloat issues? I know your point of view but the idea was to build these options by default for these platforms even though in some cases it might not be used. I have seen folks including myself select the wrong option. Having the build system automatically select the correct option for the most common cases can be very useful I feel and not require manual configuration. For defaults, you should use the defconfig, not 'select' in Kconfig. A lot of the 'select' statements are actually wrong because they do not take dependencies into account where A selects B but not C, and B depends on C, which leads to broken builds when C is disabled by a user (or randconfig). You should only ever use 'select' from platforms on silent options that are not themselves user selectable like the above 'HAVE_IDE' and 'NEED_MACH_GPIO_H'. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v6 4/8] ARM: Add .init_platform() callback to machine descriptor
On Friday 21 June 2013, Tomasz Figa wrote: To me, this new hook is strictly equivalent to init_irq. What do we gain exactly? I didn't think init_irq was going away... I know init_irq is not pretty, and we tend to overload it with other stuff, but I don't really see the point of adding a new callback that has the exact same properties. Well, it doesn't really give us any functional benefits. However in my opinion it looks much saner in case of DT-only platforms that don't need any specific IRQ initialization, but need to call some platform specific initialization routines, after memory management, but before anything else is initialized. This way irqchip_init() doesn't have to be explicitly called in platform code. Anyway, I don't have any strong opinion on this. If it is perfectly fine to abuse irqchip_init() for anything that must be done at this stage of boot, then I'm fine with this either and will modify the board file from further patch from this series to not rely on this change any more. Your init_platform only has these two calls in it: + of_clk_init(NULL); + samsung_wdt_reset_of_init(); Presumably you need of_clk_init() for the watchdog to work. But do you actually need to initialize the reset logic this early? Why not turn samsung_wdt_reset_of_init into a standalone driver, or call it from init_machine? I would actually like to call of_clk_init from common code at some point between init_irq and init_time, although I'm not sure if some platforms need it to be called before init_irq. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V10 0/4] PCIe support for Samsung Exynos5440 SoC
On Friday 21 June 2013, Jason Cooper wrote: On Fri, Jun 21, 2013 at 10:30:47AM +0200, Arnd Bergmann wrote: On Friday 21 June 2013, Thomas Petazzoni wrote: I am by far not an expert on how to solve merge strategies and so on, but to avoid conflicts at Linus's level while merging the arm-soc and pci trees, it would be better if this Samsung PCIe driver could go through arm-soc (with Bjorn ACK, of course), so that Arnd/Olof can make sure the ordering is correct with regard to the of/pci changes and the mvebu/pci driver. Yes, good point. The alternative would be that Bjorn also takes the PCI branch dependencies that are already in arm-soc into his tree. Either way works, but I agree that what you suggest would be simpler. Yes, that is why we did it this way. It was my understanding based on previous comments by yourself and LinusW that you both had patches depending on (now called) mvebu/of_pci. So we got it into arm-soc early so those branches could depend on it. Right. I wasn't paying enough attention for the early merges that Olof did. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v12 05/11] edma: config: Enable config options for EDMA
On Friday 21 June 2013, Joel A Fernandes wrote: I haven't come across this problem but- are you saying there is a shortcoming in Kbuild/Kconfig that selects an option even if its dependency is not met? Well, the shortcoming is that it lets you specify impossible contraints. You get a warning from Kconfig when building such a configuration, but then it continues. The problem with defconfig is also too many options I feel for a common case. CONFIG_DMADEVICES=y CONFIG_TI_EDMA=y Most if not all future OMAPs from will use EDMA. Why not we can be explicit about it and just built it in anyway. If ARCH_OMAP and DMADEVICES are selected, then we can just build EDMA in by default. It's just not how we do things. Kconfig is a mess because we are not consistent in the way this is done. I agree maybe the option can be dropped from Davinci but I suggest let's keep it for OMAP. Is that ok? No, I would still like you to not add it to either one. I'm spending a lot of my time tracking down incorrect 'select' statements and I'd rather spend it in a different way. I've had to a number of 'select' statements from OMAP in the past, please don't add any new ones unless there is a strict build dependency (which normally should not exist). Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7] ethernet/arc/arc_emac - Add new driver
On Friday 21 June 2013, Joe Perches wrote:
On Fri, 2013-06-21 at 10:53 +, Alexey Brodkin wrote:
On 06/21/2013 02:32 PM, Joe Perches wrote:
On Fri, 2013-06-21 at 11:20 +0400, Alexey Brodkin wrote:
Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
instantiated in some legacy ARC (Synopsys) FPGA Boards such as
ARCAngel4/ML50x.
[]
+ rxbd-data = (unsigned char *)cpu_to_le32(rx_buff-skb-data);
32 bit only. Should the Kconfig block have some arch_arc dependency
so it can't get compiled for 64 bit systems?
[]
So for now I may easily add dependency on ARC if it makes acceptance of
driver easier.
I don't think it's a big problem.
Maybe add a Kconfig depends on !64BIT.
Another thing to do would be to run it through sparse
with make C=1
No, I think the driver should just be made 64-bit clean. Not because
anyone is going to need that of course, but to avoid having
to add silly dependencies.
The problem is that rxbd-data is the wrong type. It is declared
as a void*, but it is not a kernel pointer at all, it is a DMA
pointer. Look at this code in context:
+ addr = dma_map_single(ndev-dev, (void *)rx_buff-skb-data,
+ buflen, DMA_FROM_DEVICE);
+ if (dma_mapping_error(ndev-dev, addr)) {
+ if (net_ratelimit())
+ netdev_err(ndev, cannot dma map\n);
+ dev_kfree_skb(rx_buff-skb);
+ stats-rx_errors++;
+ continue;
+ }
+ dma_unmap_addr_set(rx_buff, mapping, addr);
+ dma_unmap_len_set(rx_buff, len, buflen);
+
+ rxbd-data = (unsigned char *)cpu_to_le32(rx_buff-skb-data);
the 'addr' returned by dma_map_single is what the device really
needs, although it is the same as rx_buff-skb-data with the
trivial definition of dma_map_single.
The last line here needs to be
rxbd-data = cpu_to_le32(addr);
which fixes the bug, and has no dependency on a 32 bit CPU.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v12 05/11] edma: config: Enable config options for EDMA
On Friday 21 June 2013, Joel A Fernandes wrote: I think we are talking about different things, I agree the 'select DMADEVICES' can be dropped but lets please keep the default y option (not adding new select statements, just saying that if someone select DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to EDMA). This will simply allow people to have a default. Thanks. Yes, that's ok. Also, if the driver doesn't strictly depend on these platforms but can build on anything, I would write it as config TI_EDMA tristate TI EDMA support default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 select DMA_ENGINE select DMA_VIRTUAL_CHANNELS Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v12 05/11] edma: config: Enable config options for EDMA
On Friday 21 June 2013, Joel A Fernandes wrote: Hi Arnd, On Fri, Jun 21, 2013 at 1:44 PM, Arnd Bergmann a...@arndb.de wrote: On Friday 21 June 2013, Joel A Fernandes wrote: I think we are talking about different things, I agree the 'select DMADEVICES' can be dropped but lets please keep the default y option (not adding new select statements, just saying that if someone select DMADEVICES in menuconfig and if they're ARCH_OMAP=1 , then default to EDMA). This will simply allow people to have a default. Thanks. Yes, that's ok. Ok, thanks. I will follow up with a patch in my next submissions that builds it. Perhaps a: default y if 'ARCH_OMAP2PLUS' and leave the existing as it is... default n if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2' would make most sense to me. Basically EDMA is seen on current and all new OMAP2PLUS. Ok config TI_EDMA tristate TI EDMA support default m if 'ARCH_DAVINCI || ARCH_OMAP1 || ARCH_OMAP2 select DMA_ENGINE select DMA_VIRTUAL_CHANNELS MMC depends on EDMA specially on AM33xx there's no PIO mode AFAIK. The 'm' option will require some initramfs to load the module when needing to MMC boot, I suggest lets leave it as y. Ah, right: you still export a filter function from the edma driver and use it in slave drivers: drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, drivers/mmc/host/davinci_mmc.c: dma_request_slave_channel_compat(mask, edma_filter_fn, drivers/spi/spi-davinci.c: dspi-dma_rx = dma_request_channel(mask, edma_filter_fn, drivers/spi/spi-davinci.c: dspi-dma_tx = dma_request_channel(mask, edma_filter_fn, As long as this is the case, you have to be careful with the dependencies to make sure that davinci_mmc and spi-davinci either depend on TI_EDMA, or edma_filter_fn gets defined to NULL when you are building for a DT-only platform. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V6 3/3] ARM: dts: Add pcie controller node for Samsung EXYNOS5440 SoC
On Thursday 20 June 2013, Jingoo Han wrote: Exynos5440 has two PCIe controllers which can be used as root complex for PCIe interface. Signed-off-by: Jingoo Han jg1@samsung.com Acked-by: Arnd Bergmann a...@arndb.de ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V6 1/3] pci: Add PCIe driver for Samsung Exynos
On Thursday 20 June 2013, Jingoo Han wrote:
Exynos5440 has a PCIe controller which can be used as Root Complex.
This driver supports a PCIe controller as Root Complex mode.
Signed-off-by: Surendranath Gurivireddy Balla suren.re...@samsung.com
Signed-off-by: Siva Reddy Kallam siva.kal...@samsung.com
Signed-off-by: Jingoo Han jg1@samsung.com
The code looks good now.
Acked-by: Arnd Bergmann a...@arndb.de
.../devicetree/bindings/pci/exynos-pcie.txt| 71 ++
drivers/pci/host/Kconfig |5 +
drivers/pci/host/Makefile |1 +
drivers/pci/host/pci-exynos.c | 1057
4 files changed, 1134 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/exynos-pcie.txt
create mode 100644 drivers/pci/host/pci-exynos.c
But please change the identifier to designware or synopsys as I
asked you to.
Have a look at http://permalink.gmane.org/gmane.linux.kernel.pci/18400
This was an earlier submission for a PCIe host driver obviously
based on the same IP block. I don't know why that driver never
made it into the kernel, but if it gets submitted again, or another
platform uses the same block, it should share most of your driver.
Whatever is exynos specific then can be moved out to a separate
file.
+Required properties:
+-compatible: should be samsung,exynos5440-pcie
Please also add a generic string, e.g.
- compatible: should contain snps,dwc-pcie to identify the
core, plus an identifier for the specific instance, such
as samsung,exynos5440-pcie.
+
+static unsigned long global_io_offset;
+
+static int exynos_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+ struct pcie_port *pp;
+
+ pp = sys_to_pcie(sys);
+
+ if (!pp)
+ return 0;
+
+ if (global_io_offset SZ_1M pp-config.io_size 0) {
+ sys-io_offset = global_io_offset - pp-config.io_bus_addr;
+ pci_ioremap_io(sys-io_offset, pp-io.start);
+ global_io_offset += SZ_64K;
+ }
+
+ sys-mem_offset = pp-mem.start - pp-config.mem_bus_addr;
+
+ pci_add_resource_offset(sys-resources, pp-io, sys-io_offset);
+ pci_add_resource_offset(sys-resources, pp-mem, sys-mem_offset);
+
+ return 1;
+}
Just noticed one detail here: the 'pci_add_resource_offset' for the
I/O window should be inside of the 'if' clause, we must not annouce
the I/O port range to the PCI core layer unless we have actually
mapped it.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V6 3/3] ARM: dts: Add pcie controller node for Samsung EXYNOS5440 SoC
On Thursday 20 June 2013, Jingoo Han wrote:
2. patch adding label to the pinctrl node (which is a prerequisite) and
board-specific properties of PCIe nodes.
[PATCH] ARM: dts: Add pcie controller node for exynos5440-ssdk5440
arch/arm/boot/dts/exynos5440-ssdk5440.dts
+
+ pcie0@29 {
+ reset-gpio = pin_ctrl 5 0;
+ };
+
+ pcie1@2a {
+ reset-gpio = pin_ctrl 22 0;
+ };
arch/arm/boot/dts/exynos5440.dtsi
- pinctrl {
+ pin_ctrl: pinctrl {
Note that you don't really have to use a label, you can also refer
to the pinctrl node by its full path, which is just /pinctrl
or /pinctrl@e.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V8 1/4] pci: Add PCIe driver for Samsung Exynos
On Thursday 20 June 2013, Jingoo Han wrote:
diff --git a/Documentation/devicetree/bindings/pci/exynos-pcie.txt
b/Documentation/devicetree/bindings/pci/exynos-pcie.txt
new file mode 100644
index 000..f71d835
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/exynos-pcie.txt
@@ -0,0 +1,73 @@
+* Samsung Exynos PCIe interface
Please adapt the name of the binding as well, and name it snps-dwc-pcie.txt
for instance, to match the change in the driver and in the binding.
+Required properties:
+-compatible: should contain snps,dwc-pcie to identify the
+ core, plus an identifier for the specific instance, such
+ as samsung,exynos5440-pcie.
Thanks for changing it here.
+
+ pcie@29 {
+ compatible = samsung,exynos5440-pcie;
+ reg = 0x29 0x1000
+ 0x27 0x1000
+ 0x271000 0x40;
but please also change the example and the .dts files to match.
It should say here
compatible = samsung,exynos5440-pcie, snps,dwc-pcie;
as now required by the binding.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 06/10] ARM:stixxxx: Add DEBUG_LL console support
On Thursday 20 June 2013, Srinivas KANDAGATLA wrote:
--- a/arch/arm/mach-sti/board-dt.c
+++ b/arch/arm/mach-sti/board-dt.c
@@ -11,6 +11,7 @@
#include linux/clocksource.h
#include linux/irq.h
#include asm/hardware/cache-l2x0.h
+#include asm/mach/map.h
#include asm/mach/arch.h
#include smp.h
@@ -42,6 +43,7 @@ static const char *stih41x_dt_match[] __initdata = {
};
DT_MACHINE_START(STM, STiH415/416 SoC with Flattened Device Tree)
+ .map_io = debug_ll_io_init,
.init_time = stih41x_timer_init,
.smp= smp_ops(sti_smp_ops),
.dt_compat = stih41x_dt_match,
This part of the patch is no longer needed, we do the same when .map_io
is not set.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 01/10] serial:st-asc: Add ST ASC driver.
On Thursday 20 June 2013, Srinivas KANDAGATLA wrote: This patch adds support to ASC (asynchronous serial controller) driver, which is basically a standard serial driver. This IP is common across all the ST parts for settop box platforms. ASC is embedded in ST COMMS IP block. It supports Rx Tx functionality. It support all industry standard baud rates. Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com CC: Stephen Gallimore stephen.gallim...@st.com CC: Stuart Menefy stuart.men...@st.com CC: Arnd Bergmann a...@arndb.de CC: Greg Kroah-Hartman gre...@linuxfoundation.org From my point of view the series is ready for merging in 3.11, I only had a few final comments. I would prefer the first three patches to go through the respective serial/clocksource/pinctrl trees, since there are no direct build dependencies to the platform parts. If there is a reason to take those through arm-soc, I would insist on getting Acked-by's from the subsystem maintainers. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 02/10] clocksource:arm_global_timer: Add ARM global timer support.
On Thursday 20 June 2013, Srinivas KANDAGATLA wrote:
+static u64 gt_counter_read(void)
+{
+ u64 counter;
+ u32 lower;
+ u32 upper, old_upper;
+
+ upper = __raw_readl(gt_base + GT_COUNTER1);
+ do {
+ old_upper = upper;
+ lower = __raw_readl(gt_base + GT_COUNTER0);
+ upper = __raw_readl(gt_base + GT_COUNTER1);
+ } while (upper != old_upper);
+
+ counter = upper;
+ counter = 32;
+ counter |= lower;
+ return counter;
+}
Please replace __raw_readl etc with the non-__raw variants
throughout the code.
The __raw accessors are not endian safe and are not meant
for device drivers. If you are worried about latency from
extra barriers, use readl_relaxed().
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 10/12] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes
On Wednesday 19 June 2013, Ezequiel Garcia wrote:
On Tue, Jun 18, 2013 at 06:16:26PM +0200, Arnd Bergmann wrote:
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+ devbus-bootcs {
+ compatible = marvell,mvebu-devbus;
+ reg = 0x0001 0x10400 0x8;
+ ranges = 0 MBUS_ID(0x01, 0x2f) 0 0x;
+ #address-cells = 1;
+ #size-cells = 1;
+ clocks = coreclk 0;
+ status = disabled;
+ };
This is a violation of the binding as far as I can tell, since you don't
specify ranges
to access the 0x0001 0x10400 address. However, as I explained in my
comment for
the binding, I think you should clarify the binding and leave the
implementation
as you have it here.
Mmm... again I got lost here. Which 'ranges' you say I don't specify to
access the (formerly) 0x0001?
AFAIK, 'ranges' are only for children translation, which means I don't
need to specify a ranges for that in the devbus node, but in its parent,
right?
This ranges thing can be very tricky, so please correct me if I'm
mistaken.
You already clarified that the binding was wrong. This was about the
part where you replied:
Do you really want to require the child to provide a ranges property?
I think this makes it more complicated to specify devices that belong
into the internal-regs category.
No, this text is actually a left-over from the previous patchset, in
current v3 patchset MBus children are not required to have any ranges.
On the otherside, although you will need one except in the most trivial
cases like for the BootROM.
With that change, everything above is ok.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Ezequiel Garcia wrote: Although I'd like the binding to take this into account, for there's no point in restricting it -a priori- I can't see any advantage on doing fully dynamic window configuration on devices that are fixed in the first place. It sounds like bloating the whole thing without a strong need. After the suggestions that Grant made about the of_bus, I think the fully dynamic model would actually be simpler than what you have here. You wouldn't actually have to dissect the ranges property at all, just keep the mapping table in memory for all devices that are in use, with a special case for the internal-regs. But I think that's fine, we can alway simplify the code later as long as the binding covers all cases. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Wednesday 19 June 2013, Ezequiel Garcia wrote:
Arnd,
Going through this a couple questions came out...
On Tue, Jun 18, 2013 at 11:35:50PM +0200, Arnd Bergmann wrote:
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+
+ ranges =
+ 0x8200 0 0x40x0001
0x4 0 0x2000
+ 0x8200 0 0x80x0001
0x8 0 0x2000
+ 0x8200 0 0xe000 0x0002 0
0 0x0800
+ 0x8100 0 0 0x0002
0x800 0 0x0010;
As pointed out on IRC, this is not a good representation of the memory
space,
since it requires a non-zero sys-mem_offset, and it conflicts with the
straight
mapping I suggested.
I think it should be
0x8200 0 0xe000 0x0002 0
0xe000 0x0800
So far, so good.
if we want to encode the aperture in the ranges property here, i.e. have
a 1:1 mapping between PCI memory space and MBUS space, and in mbus,
you need the corresponding
- 0x0002 0 0xe000 0x810
+ 0x0002 0xe000 0xe000 0x810
... I obviously got something wrong, but I thought you said that
this change allowed to *not* have any mbus-node ranges translation.
I wasn't making a specific requirement here on whether you have that
in the mbus ranges or not, but if you put it in there, you have to adapt
the contents as above.
so that mbus actually translates the right addresses. You could also
have the PCI memory space start at 0, which would mean
0x8200 0 0 0x0002 0 0 0x0800
and
0x0002 0 0xe000 0x810
Mmm.. and why is this option acceptable?
As I explained on IRC, there is no requirement to pick a specific bus
aperture. The only two sensible choices are to make the bus address
the same as the CPU address, or to make the bus address start at 0,
which is what this does.
Note that the driver doesn't actually handle the generic case correctly,
you would
need to apply this patch
This patch does not apply. I tried to understand what you did, but I'm
confused by the fact I don't need to apply any patch to make it work
on my boxes, using your proposed ranges translations.
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13a633b..aa674f4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -790,6 +790,7 @@ static int __init mvebu_pcie_probe(struct
platform_device *pdev)
}
if (restype == IORESOURCE_MEM) {
of_pci_range_to_resource(range, np, pcie-mem);
+ sys-mem_offset = range.cpu_addr - range.pci_addr;
pcie-mem.name = MEM;
}
}
to deal with the generic case where the bus address is different from the
CPU address.
This patch is needed only if you change the ranges to have the bus
aperture start at 0. However, if we pick a different way to specify
the aperture than putting it into both ranges properties, that point
is moot, since you wouldn't use of_pci_range_to_resource here
anyway.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support
On Wednesday 19 June 2013, Samuel Ortiz wrote: 2. Move the vexpress-sysreg platform management functions into misc (unless we get any better place for it) This is for Arnd and Greg to decide I suppose. I think when vexpress-sysreg was created, we didn't have the syscon driver yet, otherwise I think we should have used that, and put separate drivers on top. Not sure if it's too late for changing it to that now, given that we already have a binding. It seems we should use the same code for versatile and realview, or at least it will only need small modifications to apply to all of these platforms. What I think could be helpful here is: * export a syscon for the low-level registers * add a gpio driver based on the syscon interface, and move the gpiochip implementation there * move the low-leve config code from the sysreg driver into the vexpress-config driver and make it use the syscon. * move the other global functions from the driver into the callers and use syscon there. That would end up eliminating the sysreg driver, aside from maybe a one-line change to the syscon driver to allow it to probe the right device. 3. Move vexpress-config into drivers/bus as it is (however I see no one in MAINTAINERS for this directory) ISTR that Arnd originally created that directory, so he may help here. Arnd also had some concerns about implementing this code as a bus, mostly about it not being a discoverable bus. IMHO that's a valid concern, and this is why you ended up putting it under MFD which can be seen as some sort of platform devices bus. But I still believe the bus API would make this code look cleaner and easier to maintain. Sorry, I don't see why it would be a bus. I assume that there is code missing somewhere that is not yet merged, right? Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Wednesday 19 June 2013, Jingoo Han wrote:
Then, do you mean the following?
static int __exit exynos_pcie_remove(struct platform_device *pdev)
{
struct pcie_port *pp = platform_get_drvdata(pdev);
clk_disable_unprepare(pp-bus_clk);
clk_disable_unprepare(pp-clk);
return 0;
}
static struct platform_driver exynos_pcie_driver = {
.remove = __exit_p(exynos_pcie_remove),
[.]
/* Exynos PCIe driver does not allow module unload */
static int __init pcie_init(void)
{
hook_fault_code(16 + 6, exynos_pcie_abort, SIGBUS, 0,
imprecise external abort);
platform_driver_probe(exynos_pcie_driver, exynos_pcie_probe);
return 0;
}
subsys_initcall(pcie_init);
MODULE_AUTHOR(Jingoo Han jg1@samsung.com);
MODULE_DESCRIPTION(Samsung PCIe host controller driver);
MODULE_LICENSE(GPLv2);
Yes, this looks good. I would probably use platform_driver_register
rather than platform_driver_probe, but that is your choice. using
platform_driver_probe() mean you cannot deal with deferred probing.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v3 0/2] drivers: mfd: Versatile Express SPC support
On Wednesday 19 June 2013, Pawel Moll wrote: That would end up eliminating the sysreg driver, aside from maybe a one-line change to the syscon driver to allow it to probe the right device. ... but sysreg does more than just that. In particular it provides a pseudo-gpio controller (I don't think you want to hide this behind the syscon) and it can act as a bridge to the configuration bus - see below. In short - no, I don't think sysreg driver can disappear. It can be reduced in size, yes. As I said, the gpio part can be a separate driver that just handles gpio, and I think the configuration bridge can be part of the vexpress-config driver, building on top of syscon. I'm not completely sure about the latter part. 3. Move vexpress-config into drivers/bus as it is (however I see no one in MAINTAINERS for this directory) ISTR that Arnd originally created that directory, so he may help here. Arnd also had some concerns about implementing this code as a bus, mostly about it not being a discoverable bus. IMHO that's a valid concern, and this is why you ended up putting it under MFD which can be seen as some sort of platform devices bus. But I still believe the bus API would make this code look cleaner and easier to maintain. Sorry, I don't see why it would be a bus. I assume that there is code missing somewhere that is not yet merged, right? Well, different VE components (configuration microcontrollers on motherboard and daughterboards in particular) talk to each other over a bus (an SPI derivative, in case you were wondering). So there is a bus. A non-discoverable one, but it does 42 (approximately ;-) different things. We already have: clk, hwmon, regulator and reset drivers using it. Ah, got it. In this case I think what you are looking for is a custom 'regmap' interface that abstracts those devices. Regmap can already cover i2c, spi and mmio based sets of registers (syscon is one example for mmio), and IIRC there is a simple way of extending it to other register-level interfaces like this one. And, to make things more complicated, the SPC in question, can act as a bridge to some of the functions as well. What's a difference? About 190ms, in at least one case - accessing the energy monitor data (hwmon) can take up to 200ms going through sysreg and about 10ms going through SPC. And there are people interesting in getting this numbers as often as possible. But (obviously, to make things even more complex() only the daughterboard's components can be accessed through it. Eg. the motherboard clock generators must still be accessed through sysregs. Hope you see why the problem is not trivial. Yes, it definitely needs some detailed analysis, but I think regmap is a good fit to simplify this code. Please have a look at that and tell me if you see problems with it. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Wednesday 19 June 2013, Jason Gunthorpe wrote: Today 18:53:48 On Wed, Jun 19, 2013 at 02:11:58PM +0200, Arnd Bergmann wrote: Mmm.. and why is this option acceptable? As I explained on IRC, there is no requirement to pick a specific bus aperture. The only two sensible choices are to make the bus address the same as the CPU address, or to make the bus address start at 0, which is what this does. PCI bus addresses must not alias other addresess in the system or you'll get weirdness. For instance DMA initiated from the PCI bus at address 0, intended to read from SDRAM at 0 must not be claimed by another device on the PCI bus. IMHO, a 1:1 mapping between PCI and CPU is strongly preferred. Any other configuration will need some additional techniques to avoid aliasing. Ah, good point. You are obviously right, it should definitely be a 1:1 mapping, anything else just creates a mess. I was working on a system like that before, it wasn't pretty (you have to provide separate dma_map_ops then). Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding
On Wednesday 19 June 2013, Ezequiel Garcia wrote: What happens is that any decoding window that was setup by the bootloader, is wiped and completely new windows are allocated using the translations in the DT, as described by this binding. This was the case from the start with the old MBus driver. FWIW, I think it's actually the best choice that can be made: it makes the kernel independent of the previous setting. I know you've suggested differently in the past, but I'm not sure I understand what's the benefit in keeping the bootloaders configuration. The device tree normally describes things that are either wired up in hardware or set up by the boot loader. Describing things that the boot loader may or may not have set up and that the kernel should set up but may ignore if it wants to is a bit fishy, but it seems that you have decided to do it that way. You should definitely document the fact that all ranges except the internal-regs are just suggestions and cannot be relied on to be present at boot time. Hold on! I've just noticed this, and I want to clarify something, just to avoid mis-interpretations. The binding is not saying the windows described through this ranges are present at boot time. Rather, it is this binding will guarantee that the windows described in it will be present after the mbus allocates them. Does it sound too fishy? I don't think it's a guarantee that the binding can make. The binding describes the interface between the hardware/firmware and the kernel, not an interface between one kernel driver and another. You could instead write: The ranges property defines a set of mbus windows that are expected to be set by the operating system and that are guaranteed to be free of overlaps with one another or with the system memory ranges. Each entry in the property refers to exactly one window. If an operating system choses to use a different set of mbus windows, it must ensure that any address translations performed from downstream devices are adapted accordingly. The operating system may insert additional mbus windows that do not conflict with the ones listed in the ranges, e.g. for mapping PCIe devices. As a special case, the internal register window must be set up by the boot loader at the address listed in the ranges property, since the operating uses it to set up the other windows. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v11 0/8] ] DMA Engine support for AM33XX
On Tuesday 18 June 2013, Joel A Fernandes wrote: This series is a repost of Matt Porter's EDMA patches for AM33XX EDMA support with changes for few pending review comments on v9 series. Currently this is required for AM33XX (Beaglebone or EVM) to access MMC and be able mount to rootfs and boot till command prompt over MMC. Unless there are other pending review comments, I hope this series can make it into 3.11 merge window, the dependent series has been posted at [1] and completed review. Tested EDMA on AM1808 EVM and AM33XX Beaglebone with MMC. Acked-by: Arnd Bergmann a...@arndb.de The binding looks good now. My usual comment about the fact that we shouldn't use a filter function in .xlate() applies here, but that is more about the missing interface of the dmaengine layer than the particular driver. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/2] ARM: dts: Change dw-apb-timer-osc and dw-apb-timer-sp to just dw-apb-timer
On Tuesday 18 June 2013, dingu...@altera.com wrote:
@@ -476,27 +476,31 @@
};
timer0: timer0@ffc08000 {
- compatible = snps,dw-apb-timer-sp;
+ compatible = snps,dw-apb-timer;
interrupts = 0 167 4;
reg = 0xffc08000 0x1000;
+ clocks = osc;
};
timer1: timer1@ffc09000 {
- compatible = snps,dw-apb-timer-sp;
+ compatible = snps,dw-apb-timer;
interrupts = 0 168 4;
reg = 0xffc09000 0x1000;
+ clocks = osc;
};
I think it would make sense to fix the device name as well, they should
all be named timer, not timer0, so I would add
- timer0: timer0@ffc08000 {
+ timer0: timer@ffc08000 {
for all the timers in the dts file here.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/3] dmaengine: add support for DMA multiplexer DT nodes
On Tuesday 18 June 2013, Guennadi Liakhovetski wrote:
Hmm, you've clearly shown that this can work, but it feels like a really
odd way to
do this. I don't think we should do it this way, because it tries to be
really
generic but then cannot some of the interesting cases, e.g.
1. you have multiplexed dma engines, but they need different #dma-cells.
1. you have multiplexed dma engines, but they need different dma specifiers.
2. The mux is between devices that are not on the same parent bus.
Yes, I know about these restrictions. I'm not sure whether we really ever
will get DMA multiplexers with different #dma-cells or DMA specifiers, but
in any case we can make this less generic - either keep it as a generic
uniform multiplexer or making it shdma specific - I'm fine either way.
Ok, let's make it shdma specific then.
I think this should either not try to be fully generic and just limited to
the case you care about, i.e. shdma, or it should be more abstract and
multiplex between the individual channels. In either case, I guess
it would not need a change like this to the of_dma_request_slave_channel()
function, and the node pointer used by the slave would be the same node
that defines the address space for the channel descriptions with #dma-cells.
I think the easiest way would be the first of those two, so it would
essentially look like
dmac: dma-mux {
compatible = renesas,shdma-mux; /* not simple-bus! */
#dma-cells = 1;
#address-cells = 1;
#size-cells = 1;
dma@1000 {
compatible = renesas,shdma;
reg = 0x1000 0x1000;
interrupts = 10;
};
dma@2000 {
compatible = renesas,shdma;
reg = 0x1000 0x1000;
interrupts = 10;
};
}
You then register a device driver for the master device, which
will call of_dma_controller_register() for itself and create
its child devices.
Hmm, it is an interesting idea to only register one struct of_dma instance
for all compatible shdma instances instead of one per shdma controller,
and then call of_platform_populate() to instantiate all shdma instances. A
couple of drawbacks:
1. we'll always have to put a mux DT node in .dts, even if there's just
one DMAC instance on the system.
2. as AUXDATA for the new wrapper device we'll have to use an AUXDATA
array for all child nodes, to be passed to of_platform_populate().
My suggestion above is just one of the possible ways to do it, and I'm
less concerned about it if you make it specific to shdma. Other
options would be:
1 using phandles from the mux to the individual dma engine instances,
but have them as independent nodes.
2 same as 1, but using phandles from the individual instances to the mux
3 Keep using the simple-bus.
4 Have just one combined device node for all shdma instances, with multiple
'reg' and 'interrupts' properties, and handle the muxing in the shdma
driver. You could create platform_device_create_resndata from the
top-level driver to reuse most of the existing code.
In any of these cases you should be able to support both muxed and non-muxed
operation in the shdma driver if you want to. You'd just have two separate
ofdma registrations.
3. it seems confusing to me - having one ofdma instance for multiple
dmaengine devices.
I don't see a better way around this one, but I also don't see it as
problematic.
Wiht a mux, you always end up having just one node that the slaves
point to, but multiple dma_device structures in the kernel. struct ofdma
really refers to the first one.
The advantage is, of course, that we don't need to extend existing OF and
dmaengine APIs.
So, I think, it can be done this way, but do you really think it'd be an
improvement over my original proposal?
My main interest is to keep the shdma specifics out of the generic dmaengine
binding document, where it just complicates the common case.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 3/3] DMA: shdma: add DT support
On Tuesday 18 June 2013, Guennadi Liakhovetski wrote:
Hi Arnd
On Mon, 17 Jun 2013, Arnd Bergmann wrote:
On Thursday 06 June 2013, Guennadi Liakhovetski wrote:
+Required properties:
+- dmas: a list of [DMA controller phandle] [MID/RID value]
pairs
+- dma-names: a list of DMA channel names, one per dmas entry
Looks ok to me, although it might be helpful to clarify what MID/RID means,
by expanding the acronym and describing whether one needs to pass both
or just one of them. If both, what is the bit pattern?
One word: magic. MID/RID is what that value is called in the datasheet.
E.g. on APE6 (r8a73a4) it is indeed divided into 2 fields - 2 and 6 bits
for RID and MID respectively, I _guess_ 2 bits are to select a channel
within a slave device and 6 bits to pick up one of slaves, but that
doesn't fit with a slave with 8 channels (HSI), there are also slave
devices with different MID values, so, those values are really better
considered as a single magic value - an 8-bit channel request handle,
which is also how they are listed in a reference table.
Ok.
* services would have to provide their own filters, which first would
check
* the device driver, similar to how other DMAC drivers, e.g.,
sa11x0-dma.c, do
* this, and only then, in case of a match, call this common filter.
+ * NOTE 2: This filter function is also used in the DT case by
shdma_xlate().
+ * In that case the MID-RID value is used for slave channel filtering
and is
+ * passed to this function in the arg parameter.
*/
bool shdma_chan_filter(struct dma_chan *chan, void *arg)
{
struct shdma_chan *schan = to_shdma_chan(chan);
struct shdma_dev *sdev = to_shdma_dev(schan-dma_chan.device);
const struct shdma_ops *ops = sdev-ops;
- int slave_id = (int)arg;
+ int match = (int)arg;
int ret;
- if (slave_id 0)
+ if (match 0)
/* No slave requested - arbitrary channel */
return true;
- if (slave_id = slave_num)
+ if (!schan-dev-of_node match = slave_num)
return false;
- ret = ops-set_slave(schan, slave_id, true);
+ ret = ops-set_slave(schan, match, true);
if (ret 0)
return false;
This is complicated by the fact that you are using the same function for
two entirely different purposes. It would be easier to have a separate
filter for the DT case, rather than reusing the one that uses the slave_id
as an argument.
Hm, yes, I was considering either making 2 functions or reusing one, but
it's really the same code with only difference - the DT version wouldn't
have the slave_num check. So, I decided to use a single function
renaming slave_id to a neutral match. You really think it's become too
complex and should be copied for clarity?
I think it would be easier to understand if you have two functions, but
it's not very important. Especially if you make the new function specific
to shdma, that would be clearer.
@@ -867,6 +883,29 @@ void shdma_chan_remove(struct shdma_chan *schan)
}
EXPORT_SYMBOL(shdma_chan_remove);
+struct dma_chan *shdma_xlate(struct of_phandle_args *dma_spec,
+ struct of_dma *ofdma)
+{
+ struct shdma_dev *sdev = ofdma-of_dma_data;
+ u32 id = dma_spec-args[0];
+ dma_cap_mask_t mask;
+ struct dma_chan *chan;
+
+ if (dma_spec-args_count != 1 || !sdev)
+ return NULL;
+
+ dma_cap_zero(mask);
+ /* Only slave DMA channels can be allocated via DT */
+ dma_cap_set(DMA_SLAVE, mask);
+
+ chan = dma_request_channel(mask, shdma_chan_filter, (void *)id);
+ if (chan)
+ to_shdma_chan(chan)-hw_req = id;
+
+ return chan;
+}
+EXPORT_SYMBOL(shdma_xlate);
I would suggest keeping this to the drivers/dma/sh/shdma.c file
and not exporting it. The sudma would use a different binding anyway.
Ok, can do that and then see, how different sudma's .xlate() function will
be. If it's the same we can make this common again.
I hope we can get rid of the need for calling dma_request_channel() from
xlate soon, since that is a bit silly anyway. Ideally you would just pick
the right channel of the dma_device (or the first free one, depending on
the driver) and return that from xlate.
The pdata and slave_id should really not be needed here for the lookup in
the DT
case. Is this just temporary until all slave drivers use
dmaenging_slave_config
to pass the address? That should be clarified in a comment.
Also with DT we still use platform data, passed to the driver using
auxdata. This function finds a suitable struct sh_dmae_slave_config
channel configuration entry in the platform data and returns it to the
caller. I don't think this can be avoided without carrying all the
platform data over to DT.
I think that should be done anyway. A lot of the data can just be hardcoded
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Tuesday 18 June 2013, Jingoo Han wrote:
On Monday, June 17, 2013 9:45 PM, Arnd Bergmann wrote:
On Monday 17 June 2013 18:45:52 Jingoo Han wrote:
On Friday, June 14, 2013 9:54 PM, Arnd Bergmann wrote:
Please look up the documentation about inbound viewport and describe
in a code comment what it does. I /assume/ that this is how DMA accesses
from the bus get translated into AXI bus transactions. If so, you have
to let the window translate addresses from RAM. If it's something else,
then you should document what it is and how it needs to be set up.
One of our hardware engineer confirmed it.
He said that these inbound functions are unnecessary.
Also, I checked that PCIe works properly without these functions.
So, I will remove these inbound functions.
Ok, good. So DMA just gets translated 1:1 independent of the
inbound viewport? Have you tested this with PCI device using
DMA?
According to our hardware engineer,
He said that
That's correct. We tested it by doing a memory write from the device.
A device DMA is a series of memory r/w so I expect it to work same way.
Also, he thought that I already tested too, since it works after removing
the functions.
It could be that the default setting just creates a 1:1 map so that would
work, but if you tested it, that should be good enough.
I looked at the pci-mvebu driver,
Then I fixed it as the pci-mvebu driver did.
Also, I added 'global_io_offset'.
@@ -909,6 +909,12 @@ static int __init exynos_pcie_probe(struct
platform_device *pdev)
if (restype == IORESOURCE_IO) {
of_pci_range_to_resource(range, np, pp-io);
pp-io.name = I/O;
+ pp-io.start = max_t(resource_size_t,
+ PCIBIOS_MIN_IO,
+ range.pci_addr +
global_io_offset);
+ pp-io.end = min_t(resource_size_t,
+ IO_SPACE_LIMIT,
+ range.pci_addr + range.size +
global_io_offset);
pp-config.io_size = resource_size(pp-io);
pp-config.io_bus_addr = range.pci_addr;
In this case, boot message is as below:
PCI host bridge to bus :00
pci_bus :00: root bus resource [io 0x1000-0x1]
pci_bus :00: root bus resource [mem 0x40011000-0x5fff]
[.]
PCI host bridge to bus 0001:00
pci_bus 0001:00: root bus resource [io 0x1-0x2] (bus address
[0x-0x1])
pci_bus 0001:00: root bus resource [mem 0x60011000-0x7fff]
Ok, very good.
I will remove a 'remove' callback. Is it OK?
Or what should I do?
I think you should keep the remove function, but add a comment explaining that
you don't allow module unload and that in order to allow it, the remove function
will have to remove all pci buses and devices under the host bridge.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Tuesday 18 June 2013, zhangfei gao wrote:
On Tue, Jun 18, 2013 at 4:58 AM, Arnd Bergmann a...@arndb.de wrote:
+static struct of_dma_filter_info k3_dma_filter;
+static bool k3_dma_filter_fn(struct dma_chan *chan, void *param)
+{
+ return (*(int *)param == chan-chan_id);
+}
This filter function works only as long as there is no more than
one DMA engine in the system, which is something that needs to
be documented better. Unfortunately, providing a filter
function to be called by .xlate is currently the only way that
the dma-engine API supports, but we should really get over that.
Vinod: I think we need to add a way for a dmaengine driver to
just return one of its channels to the xlate function. The
current method is getting very silly, and it adds run-time and
code complexity without any need.
How about something like
int dma_get_slave_channel(struct dma_chan *chan)
{
/* lock against __dma_request_channel */
mutex_lock(dma_list_mutex);
if (chan-client_count == 0)
ret = dma_chan_get(chan);
else
ret = -EBUSY;
mutex_unlock(dma_list_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(dma_get_slave_channel);
Want to double check here.
Device need specific channel via request line, the requirement still
can be met, right?
The driver can have a simple array of pointers that is indexed by
the request number, so you end up with something like
struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
struct of_dma *ofdma)
{
struct k3_dma_dev *dev = ofdma-of_dma_data;
unsigned int vchan = dma_spec-args[0];
if (vchan dev-nr_channels)
return NULL;
return dma_get_slave_channel(dev-vchan[vchan]);
}
With no need to have a filter function.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 4/4] arm: add basic support for Rockchip RK3066a boards
On Tuesday 18 June 2013, Heiko Stübner wrote: Side comment: I think it would be nice if the generic code did this init if a l2x0 device node was in the device tree, since the only reason to override init_machine is to do this call in addition to of_platform_populate(). Arnd said similar things in the initial version :-). Currently I'm wondering if it wouldn't be enough to call always l2x0_of_init somewhere, because it does the checking for the dt nodes itself already. The only obstacle would be platforms having the need to use special aux-values or which are currently calling the function from some other parts of the boot process (tegra inits the cache in its tegra_init_early function for example). I think we can handle this by ensuring the function only gets called once, and all platforms with special requirements call it before the common code does. I tried to understand what the requirement for non-zero argument is however and couldn't figure it out. Shouldn't we just be able to specify all the bits as DT properties all the time? Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Tuesday 18 June 2013 22:22:17 zhangfei wrote: With no need to have a filter function. Cool, then I would like to wait for the patch. Maybe you can try to add the dma_get_slave_channel() function I proposed here as a first patch and add your driver on top. There may be issues I missed, and Vinod needs to agree to the concept first, but that would probably get his attention. Or you could send your the new interface as an add-on patch and convert your driver along with adding it. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+Required properties:
+
+- compatible: Should be set to one of the following:
+ marvell,armada370-mbus
+ marvell,armadaxp-mbus
+
+- reg:Device's register space.
+ Two entries are expected, see the examples below.
+ The first one controls the devices decoding window and
+ the second one controls the SDRAM decoding window.
+
+- address-cells: Must be '2'. The first cell for the MBus ID encoding,
+ the second cell for the address offset within the window.
+
+- size-cells:Must be '1'.
+
+- ranges:Must be set up to provide a proper translation for each
child.
+ See the examples below.
You should explain here what the policy is regarding windows set up by the
boot loader. Are the ranges in here required to be the ones that the boot
loader has preconfigured, or are they the ones that the mbus driver is supposed
to set up?
+Each child device needs at least a 'ranges' property. If the child is
avaiable
+(i.e. status not 'disabled'), then the MBus driver creates a decoding window
+for it. For instance, in the example below the BootROM child is specified:
+
+ soc {
+ compatible = marvell,armada370-mbus, simple-bus;
+ reg = 0xd002 0x100, 0xd0020180 0x20;
+ #address-cells = 2;
+ #size-cells = 1;
+
+ ranges = ... /* other entries */
+0x011d 0 0 0xfff0 0x10;
+
+ bootrom {
+ #address-cells = 1;
+ #size-cells = 1;
+ ranges = 0 0x011d 0 0x10;
+ };
+
+ /* other children */
+ ...
+ };
Do you really want to require the child to provide a ranges property?
I think this makes it more complicated to specify devices that belong
into the internal-regs category.
Is this mainly for convenience when settup up the windows?
+
+ ranges =
+0x8200 0 0x4 0x0001 0x4 0
0x2000 /* Port 0.0 registers */
+ 0x8200 0 0x42000 0x0001 0x42000 0
0x2000 /* Port 2.0 registers */
+ 0x8200 0 0x44000 0x0001 0x44000 0
0x2000 /* Port 0.1 registers */
+ 0x8200 0 0x48000 0x0001 0x48000 0
0x2000 /* Port 0.2 registers */
+ 0x8200 0 0x4c000 0x0001 0x4c000 0
0x2000 /* Port 0.3 registers */
+ 0x8200 0 0x8 0x0001 0x8 0
0x2000 /* Port 1.0 registers */
+ 0x8200 0 0x82000 0x0001 0x82000 0
0x2000 /* Port 3.0 registers */
+ 0x8200 0 0xe000 0x0002 0 0
0x0800 /* non-prefetchable memory */
+ 0x8100 0 0 0x0002 0x800 0
0x0010; /* downstream I/O */
Using 0x0002 as a placeholder for the pcie translation is definitely
better than 0x as you had before, but let me ask again in case
you missed it the last time (and sorry if I missed the answer):
Why not just put the actual translation here the way it happens for each
of the PCIe ports? With the definition here, the PCIe driver actually has no
way to figure out what settings the windows need to use!
On a side note, is there a reason why you use 0x0001 for the internal-regs
rather than just 0x1? This one isn't important as they both work as well, it's
just more to write your way.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 10/12] ARM: mvebu: Relocate Armada 370/XP DeviceBus device tree nodes
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+ devbus-bootcs {
+ compatible = marvell,mvebu-devbus;
+ reg = 0x0001 0x10400 0x8;
+ ranges = 0 MBUS_ID(0x01, 0x2f) 0 0x;
+ #address-cells = 1;
+ #size-cells = 1;
+ clocks = coreclk 0;
+ status = disabled;
+ };
This is a violation of the binding as far as I can tell, since you don't
specify ranges
to access the 0x0001 0x10400 address. However, as I explained in my comment
for
the binding, I think you should clarify the binding and leave the implementation
as you have it here.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+ ranges =
+ 0x8200 0 0x40x0001 0x4 0
0x2000
+ 0x8200 0 0x80x0001 0x8 0
0x2000
+ 0x8200 0 0xe000 0x0002 0
0 0x0800
+ 0x8100 0 0 0x0002 0x800
0 0x0010;
To clarify my earlier comment, I think it would be nicer to write this as
ranges =
0x8200 0 0x40x0001 0x4 0
0x2000
0x8200 0 0x80x0001 0x8 0
0x2000
0x8200 1 0 MBUS_ID(0x12, 0x34) 0 1 0
0x8200 2 0 MBUS_ID(0x13, 0x34) 0 1 0
0x8100 1 0 MBUS_ID(0x12, 0x35) 0 0 0x1;
0x8100 2 0 MBUS_ID(0x13, 0x35) 0 0 0x1;
The MBUS_ID numbers above are made up since I don't know them, but this way you
can
describe how the entire 4GB MMIO address space of the PCI bus is mapped into the
MBUS address space.
+ pcie@1,0 {
+ device_type = pci;
+ assigned-addresses = 0x82000800 0 0x4 0
0x2000;
+ reg = 0x0800 0 0 0 0;
+ #address-cells = 3;
+ #size-cells = 2;
+ #interrupt-cells = 1;
+ ranges;
Then you do a ranges property for each port with the high-order
address word equal to the port number:
ranges = 0x8200 1 0 0x8200 0 0 1 0
0x8100 1 0 0x8100 0 0 0 0x1;
+ interrupt-map-mask = 0 0 0 0;
+ interrupt-map = 0 0 0 0 mpic 58;
+ marvell,pcie-port = 0;
+ marvell,pcie-lane = 0;
+ clocks = gateclk 5;
+ status = disabled;
+ pcie@2,0 {
+ device_type = pci;
+ assigned-addresses = 0x82002800 0 0x8 0
0x2000;
+ reg = 0x1000 0 0 0 0;
+ #address-cells = 3;
+ #size-cells = 2;
+ #interrupt-cells = 1;
+ ranges;
ranges = 0x8200 2 0 0x8200 0 0 1 0
0x8100 2 0 0x8100 0 0 0 0x1;
+ interrupt-map-mask = 0 0 0 0;
+ interrupt-map = 0 0 0 0 mpic 62;
+ marvell,pcie-port = 1;
+ marvell,pcie-lane = 0;
+ clocks = gateclk 9;
+ status = disabled;
+ };
Does this make sense?
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3] DMA: shdma: add DT support
On Tuesday 18 June 2013, Guennadi Liakhovetski wrote: This patch adds Device Tree support to the shdma driver. No special DT properties are used, only standard DMA DT bindings are implemented. Since shdma controllers reside on SoCs, their configuration is SoC-specific and shall be passed to the driver from the SoC platform data, using the auxdata procedure. Signed-off-by: Guennadi Liakhovetski g.liakhovetski+rene...@gmail.com Acked-by: Arnd Bergmann a...@arndb.de ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding
On Tuesday 18 June 2013, Thomas Petazzoni wrote: Dear Arnd Bergmann, On Tue, 18 Jun 2013 18:14:33 +0200, Arnd Bergmann wrote: Using 0x0002 as a placeholder for the pcie translation is definitely better than 0x as you had before, but let me ask again in case you missed it the last time (and sorry if I missed the answer): Why not just put the actual translation here the way it happens for each of the PCIe ports? With the definition here, the PCIe driver actually has no way to figure out what settings the windows need to use! Come on Arnd, this is something we have already discussed countless times with you. We cannot define translations for each PCIe port because we don't know in advance how much I/O and memory space each PCIe device will request. This is the reason why we have one global range for I/O space and one global space for memory space, that are given to the Linux PCI core, which then dynamically assigns sub-ranges for each PCIe device into those two global ranges. Could you just stop assuming that I'm an idiot and read my suggestion? The information that this puts into the device-tree is not where things are mapped by how things are connected in hardware. The mbus mapping is the part that is variable, while the PCI mapping is actually fixed in hardware. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Thomas Petazzoni wrote: To clarify my earlier comment, I think it would be nicer to write this as ranges = 0x8200 0 0x40x0001 0x4 0 0x2000 0x8200 0 0x80x0001 0x8 0 0x2000 0x8200 1 0 MBUS_ID(0x12, 0x34) 0 1 0 0x8200 2 0 MBUS_ID(0x13, 0x34) 0 1 0 0x8100 1 0 MBUS_ID(0x12, 0x35) 0 0 0x1; 0x8100 2 0 MBUS_ID(0x13, 0x35) 0 0 0x1; The MBUS_ID numbers above are made up since I don't know them, but this way you can describe how the entire 4GB MMIO address space of the PCI bus is mapped into the MBUS address space. This is NOT possible because we don't know in advance how much memory space and I/O space each PCIe device will require. Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work? Where did I suggest static windows for PCIe devices? Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Thomas Petazzoni wrote: On Tue, 18 Jun 2013 19:18:58 +0200, Arnd Bergmann wrote: ranges = 0x8200 0 0x40x0001 0x4 0 0x2000 0x8200 0 0x80x0001 0x8 0 0x2000 0x8200 1 0 MBUS_ID(0x12, 0x34) 0 1 0 0x8200 2 0 MBUS_ID(0x13, 0x34) 0 1 0 0x8100 1 0 MBUS_ID(0x12, 0x35) 0 0 0x1; 0x8100 2 0 MBUS_ID(0x13, 0x35) 0 0 0x1; The MBUS_ID numbers above are made up since I don't know them, but this way you can describe how the entire 4GB MMIO address space of the PCI bus is mapped into the MBUS address space. This is NOT possible because we don't know in advance how much memory space and I/O space each PCIe device will require. Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work? Where did I suggest static windows for PCIe devices? Where does your new proposal buys us anything useful compared to the existing PCIe DT binding that has been discussed at length with you? I'm pretty sure I explained the idea above originally and was ignored. Jason Gunthorpe might remember better, but I think he liked it when I originally proposed doing it this way. The main differences are: * It unifies the binding for the PCIe case and all other registers. There is no need to treat PCIe special in the binding this way, which is more future-proof and more consistent. * Since the host physical address for the PCIe memory space window is set up dynamically anyway, there is no reason to hardcode it in DT. We want it to be as large as possible, and this way the mbus driver can just pick the largest free area itself after setting up all the other mappings from the ranges property. * The binding actually allows the PCIe translation to be discontiguous, so if we want to improve the code in the future, we can fill all the holes in the mbus space with PCIe windows without changing the binding. * It separates hardware description (in the PCIe node) from policy (in the mbus node), just like we do for all the other mbus children. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding
On Tuesday 18 June 2013, Sebastian Hesselbarth wrote: On 06/18/2013 07:46 PM, Jason Gunthorpe wrote: On Tue, Jun 18, 2013 at 08:25:28AM -0300, Ezequiel Garcia wrote: + +IIAA + +Where: + -- I = Marvell defined target ID for programmable windows + -- A = Marvell defined target attributes for programmable windows I thought we agreed to something like: SIAA Where 'S' is the designator for the special items like PCI-E and internal-regs. 0 = normal target ids, 0xF = special ids. The target is only 4 bits, the attr is 8, so a little doc update to clarify this should be enough, no need to change the DTs. +1 for SIAA, as it allows to use MBUS_ID also for those fake windows. It makes it more readable IMHO. +1 Also allows you to have up to 40b offset, which might be important with LPAE enabled. Not with the current generation I think, since the mbus windows are 32 bit only, but it would avoid having to come up with a new format for a potential future-generation mbus that has wider address. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Jason Gunthorpe wrote: On Tue, Jun 18, 2013 at 08:22:08PM +0200, Arnd Bergmann wrote: Arnd, we've discussed this at length with you while getting the PCIe driver merged, and we've explained this to you numerous times. Could you please understand that any of your proposal that suggests writing down static windows for PCIe devices will not work? Where did I suggest static windows for PCIe devices? Where does your new proposal buys us anything useful compared to the existing PCIe DT binding that has been discussed at length with you? I'm pretty sure I explained the idea above originally and was ignored. Jason Gunthorpe might remember better, but I think he liked it when I originally proposed doing it this way. I remember it took a bit to understand your proposal, but I thought it could work, but I admit I forget all the little details now :( Ah, if I can just rephrase simply - the notion was to move the determination of the aperture to use dynmic allocation and then restructure the ranges around the mbus target, since they no longer need to encode the aperture. Right. My concern: dynamically sizing the aperture is hard. There are three apertures that need to be picked, and the PCI core code has no support for dynamic apertures. Getting the aperture from the DT is a functional compromise. After some discussion on IRC with Ezequiel, I think it's best to leave the aperture listed in DT but say in the binding that the OS may override it. I would still want to see the actual MBUS IDs in the ranges property of the pci-controller nodes. Right now, the pcie driver has to call into the mbus driver passing a hardcoded string for setting up the mapping, which is then used to look up the MBUS ID. This is rather inconsistent and we should have all that information in DT. Ideally we should also use it, to ensure it's correct but for 3.11 it would be enough to get the DT representation right. We can replace the string passing in 3.12. * Since the host physical address for the PCIe memory space window is set up dynamically anyway, there is no reason to hardcode it in DT. We want it to be as large as possible, and this way the mbus driver can just pick the largest free area itself after setting up all the other mappings from the ranges property. This seems to get really complicated if the mbus driver is ever required to support dynamic mappings.. If PCI-E claims all memory and then you modprobe something it could fail. Yes, good point. However that is something we can figure out if it comes to that. With my suggestion of setting up the mbus windows in the of_bus xlate function, we would already create them at boot time when of_platform_populate gets called, so that wouldn't be a problem. IMHO, I go back to my original thoughts. There is no real need for any of this to be dynamic, we can use the values in the DT, presumably set by the bootloader and things will work well. The added complexity and failure modes for dynamic is simply not worth it.. I don't think it's too hard to be prepared for fully dynamic operation. As Grant said in his comment on v2, the real complexity comes from the fact that we are mixing dynamic and static configuration here, and the PCIe configuration is inherently dynamic. The change I'm proposing would just mean the DT representation reflects the dynamic nature of the PCIe windows. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 11/12] ARM: mvebu: Relocate Armada 370 PCIe device tree nodes
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+
+ ranges =
+ 0x8200 0 0x40x0001 0x4 0
0x2000
+ 0x8200 0 0x80x0001 0x8 0
0x2000
+ 0x8200 0 0xe000 0x0002 0
0 0x0800
+ 0x8100 0 0 0x0002 0x800
0 0x0010;
As pointed out on IRC, this is not a good representation of the memory space,
since it requires a non-zero sys-mem_offset, and it conflicts with the straight
mapping I suggested.
I think it should be
0x8200 0 0xe000 0x0002 0 0xe000
0x0800
if we want to encode the aperture in the ranges property here, i.e. have
a 1:1 mapping between PCI memory space and MBUS space, and in mbus,
you need the corresponding
- 0x0002 0 0xe000 0x810
+ 0x0002 0xe000 0xe000 0x810
so that mbus actually translates the right addresses. You could also
have the PCI memory space start at 0, which would mean
0x8200 0 0 0x0002 0 0 0x0800
and
0x0002 0 0xe000 0x810
Note that the driver doesn't actually handle the generic case correctly, you
would
need to apply this patch
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13a633b..aa674f4 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -790,6 +790,7 @@ static int __init mvebu_pcie_probe(struct platform_device
*pdev)
}
if (restype == IORESOURCE_MEM) {
of_pci_range_to_resource(range, np, pcie-mem);
+ sys-mem_offset = range.cpu_addr - range.pci_addr;
pcie-mem.name = MEM;
}
}
to deal with the generic case where the bus address is different from the
CPU address.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 03/12] bus: mvebu-mbus: Add static window allocation to the DT binding
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
On Tue, Jun 18, 2013 at 06:14:33PM +0200, Arnd Bergmann wrote:
On Tuesday 18 June 2013, Ezequiel Garcia wrote:
+Required properties:
+
+- compatible: Should be set to one of the following:
+ marvell,armada370-mbus
+ marvell,armadaxp-mbus
+
+- reg:Device's register space.
+ Two entries are expected, see the examples below.
+ The first one controls the devices decoding window and
+ the second one controls the SDRAM decoding window.
+
+- address-cells: Must be '2'. The first cell for the MBus ID encoding,
+ the second cell for the address offset within the
window.
+
+- size-cells:Must be '1'.
+
+- ranges:Must be set up to provide a proper translation for each
child.
+ See the examples below.
You should explain here what the policy is regarding windows set up by the
boot loader. Are the ranges in here required to be the ones that the boot
loader has preconfigured, or are they the ones that the mbus driver is
supposed
to set up?
Actually, I might be confused but the kernel MBus driver is completely
independent from the bootloader's -except in the internal register case,
(because we don't touch that from where the bootloader left it).
Right, then state that clearly in the binding.
What happens is that any decoding window that was setup by the bootloader,
is wiped and completely new windows are allocated using the translations
in the DT, as described by this binding.
This was the case from the start with the old MBus driver. FWIW, I think
it's actually the best choice that can be made: it makes the kernel
independent of the previous setting.
I know you've suggested differently in the past, but I'm not sure I
understand what's the benefit in keeping the bootloaders configuration.
The device tree /normally/ describes things that are either wired up
in hardware or set up by the boot loader. Describing things that the
boot loader may or may not have set up and that the kernel should
set up but may ignore if it wants to is a bit fishy, but it seems
that you have decided to do it that way. You should definitely
document the fact that all ranges except the internal-regs are just
suggestions and cannot be relied on to be present at boot time.
A cleaner approach would be to require that all ranges here are exactly
the ones that have been configured by the boot loader, and state the
OS can either assume that they are valid or can reprogram them as
needed. That would imply that you don't actually need an mbus driver
unless you want to dynamically reassign the windows.
Please also add a property that describes the address range in which
the windows might be reassigned, i.e. everything that is not RAM.
+Each child device needs at least a 'ranges' property. If the child is
avaiable
+(i.e. status not 'disabled'), then the MBus driver creates a decoding
window
+for it. For instance, in the example below the BootROM child is
specified:
+
+ soc {
+ compatible = marvell,armada370-mbus, simple-bus;
+ reg = 0xd002 0x100, 0xd0020180 0x20;
+ #address-cells = 2;
+ #size-cells = 1;
+
+ ranges = ... /* other entries */
+0x011d 0 0 0xfff0 0x10;
+
+ bootrom {
+ #address-cells = 1;
+ #size-cells = 1;
+ ranges = 0 0x011d 0 0x10;
+ };
+
+ /* other children */
+ ...
+ };
Do you really want to require the child to provide a ranges property?
I think this makes it more complicated to specify devices that belong
into the internal-regs category.
No, this text is actually a left-over from the previous patchset, in
current v3 patchset MBus children are *not* required to have any ranges.
On the otherside, although you will need one except in the most trivial
cases like for the BootROM.
Ok.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 3/3] DMA: shdma: add DT support
On Thursday 06 June 2013, Guennadi Liakhovetski wrote:
+Required properties:
+- dmas: a list of [DMA controller phandle] [MID/RID value]
pairs
+- dma-names: a list of DMA channel names, one per dmas entry
Looks ok to me, although it might be helpful to clarify what MID/RID means,
by expanding the acronym and describing whether one needs to pass both
or just one of them. If both, what is the bit pattern?
* services would have to provide their own filters, which first would check
* the device driver, similar to how other DMAC drivers, e.g., sa11x0-dma.c,
do
* this, and only then, in case of a match, call this common filter.
+ * NOTE 2: This filter function is also used in the DT case by shdma_xlate().
+ * In that case the MID-RID value is used for slave channel filtering and is
+ * passed to this function in the arg parameter.
*/
bool shdma_chan_filter(struct dma_chan *chan, void *arg)
{
struct shdma_chan *schan = to_shdma_chan(chan);
struct shdma_dev *sdev = to_shdma_dev(schan-dma_chan.device);
const struct shdma_ops *ops = sdev-ops;
- int slave_id = (int)arg;
+ int match = (int)arg;
int ret;
- if (slave_id 0)
+ if (match 0)
/* No slave requested - arbitrary channel */
return true;
- if (slave_id = slave_num)
+ if (!schan-dev-of_node match = slave_num)
return false;
- ret = ops-set_slave(schan, slave_id, true);
+ ret = ops-set_slave(schan, match, true);
if (ret 0)
return false;
This is complicated by the fact that you are using the same function for
two entirely different purposes. It would be easier to have a separate
filter for the DT case, rather than reusing the one that uses the slave_id
as an argument.
@@ -867,6 +883,29 @@ void shdma_chan_remove(struct shdma_chan *schan)
}
EXPORT_SYMBOL(shdma_chan_remove);
+struct dma_chan *shdma_xlate(struct of_phandle_args *dma_spec,
+ struct of_dma *ofdma)
+{
+ struct shdma_dev *sdev = ofdma-of_dma_data;
+ u32 id = dma_spec-args[0];
+ dma_cap_mask_t mask;
+ struct dma_chan *chan;
+
+ if (dma_spec-args_count != 1 || !sdev)
+ return NULL;
+
+ dma_cap_zero(mask);
+ /* Only slave DMA channels can be allocated via DT */
+ dma_cap_set(DMA_SLAVE, mask);
+
+ chan = dma_request_channel(mask, shdma_chan_filter, (void *)id);
+ if (chan)
+ to_shdma_chan(chan)-hw_req = id;
+
+ return chan;
+}
+EXPORT_SYMBOL(shdma_xlate);
I would suggest keeping this to the drivers/dma/sh/shdma.c file
and not exporting it. The sudma would use a different binding anyway.
+/*
+ * Find a slave channel configuration from the contoller list by either a
slave
+ * ID in the non-DT case, or by a MID/RID value in the DT case
+ */
static const struct sh_dmae_slave_config *dmae_find_slave(
- struct sh_dmae_chan *sh_chan, int slave_id)
+ struct sh_dmae_chan *sh_chan, int match)
{
struct sh_dmae_device *shdev = to_sh_dev(sh_chan);
struct sh_dmae_pdata *pdata = shdev-pdata;
const struct sh_dmae_slave_config *cfg;
int i;
- if (slave_id = SH_DMA_SLAVE_NUMBER)
- return NULL;
+ if (!sh_chan-shdma_chan.dev-of_node) {
+ if (match = SH_DMA_SLAVE_NUMBER)
+ return NULL;
- for (i = 0, cfg = pdata-slave; i pdata-slave_num; i++, cfg++)
- if (cfg-slave_id == slave_id)
- return cfg;
+ for (i = 0, cfg = pdata-slave; i pdata-slave_num; i++,
cfg++)
+ if (cfg-slave_id == match)
+ return cfg;
+ } else {
+ for (i = 0, cfg = pdata-slave; i pdata-slave_num; i++,
cfg++)
+ if (cfg-mid_rid == match) {
+ sh_chan-shdma_chan.slave_id = cfg-slave_id;
+ return cfg;
+ }
+ }
The pdata and slave_id should really not be needed here for the lookup in the DT
case. Is this just temporary until all slave drivers use dmaenging_slave_config
to pass the address? That should be clarified in a comment.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 0/3] dma: add support for DMA multiplexer DT nodes
On Wednesday 12 June 2013, Vinod Koul wrote: On Thu, Jun 06, 2013 at 05:47:22PM +0200, Guennadi Liakhovetski wrote: Next re-send of an earlier work. This series adds support for multiple compatible DMAC instances, capable of serving the same DMA slaves. Currently to specify such a configuration in DT, slaves would have to add DMA tuples for each requested channel for each suitable DMAC. This needlessly clutters the Device Tree. Instead we group such DMAC DT nodes together under a common parent. Then slaves can just reference that parent in their DMA bindings. Cc: Guennadi Liakhovetski g.liakhovetski+rene...@gmail.com wasnt this series Acked by Anrd previosuly. Neverthless, Anrd do you mind reviwing this Sorry about the delay. The binding in patch 3 looks fine, but I have a few comments about the implementation. Feel free to ignore those if you think the code is good enough. Trying to wrap my head around the mux binding now, to see if there is a better way to do that. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v10 5/8] dmaengine: edma: Add TI EDMA device tree binding
On Monday 17 June 2013, Fernandes, Joel A wrote: [Joel] Thanks for the suggestion, I updated it and it looks like this now: Required properties: - compatible : ti,edma3 - ti,hwmods: Name of the hwmods associated to the EDMA - ti,edma-regions: Number of regions - ti,edma-slots: Number of slots - #dma-cells: Should be set to 1 Clients should use a single number per DMA channel request. That still does not say what that number refers to. Is it a channel number, or a request line number, or something completely different? Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/3] dmaengine: add support for DMA multiplexer DT nodes
On Thursday 06 June 2013, Guennadi Liakhovetski wrote:
Documentation/devicetree/bindings/dma/dma.txt | 44
+
drivers/dma/of-dma.c | 31 +
2 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/dma/dma.txt
b/Documentation/devicetree/bindings/dma/dma.txt
index 8f504e6..a861298 100644
--- a/Documentation/devicetree/bindings/dma/dma.txt
+++ b/Documentation/devicetree/bindings/dma/dma.txt
@@ -31,6 +31,50 @@ Example:
dma-requests = 127;
};
+* DMA multiplexer
+
+Several DMA controllers with the same #dma-cells number and the same request
+line IDs can be grouped under a DMA multiplexer super-node, if slaves can use
+DMA channels on any of them.
+
+Required property:
+- compatible:Must include dma-mux.
+
+Some standard optional properties can be helpful:
+
+Optional properties:
+- compatible:You will probably also want to include
compatibility
+ with simple-bus to automatically create platform
+ devices from subnodes.
+- ranges:Map DMA controller memory areas in the parent address
+ space.
+- #address-cells:Number of address cells in case automatic propagation
+ with the help of ranges doesn't work.
+- #size-cells: Number of size cells.
+
+Example:
+
+ dmac: dma-mux {
+ compatible = simple-bus, dma-mux;
+ ranges;
+
+ dma0: dma@1000 {
+ #dma-cells = 1;
+ ...
+ };
+
+ dma1: dma@2000 {
+ #dma-cells = 1;
+ ...
+ };
+ };
+
+ mmc0: mmc@3000 {
+ dmas = dmac 1
+ dmac 2;
+ dma-names = tx, rx;
+ ...
+ };
* DMA client
Hmm, you've clearly shown that this can work, but it feels like a really odd
way to
do this. I don't think we should do it this way, because it tries to be really
generic but then cannot some of the interesting cases, e.g.
1. you have multiplexed dma engines, but they need different #dma-cells.
1. you have multiplexed dma engines, but they need different dma specifiers.
2. The mux is between devices that are not on the same parent bus.
I think this should either not try to be fully generic and just limited to
the case you care about, i.e. shdma, or it should be more abstract and
multiplex between the individual channels. In either case, I guess
it would not need a change like this to the of_dma_request_slave_channel()
function, and the node pointer used by the slave would be the same node
that defines the address space for the channel descriptions with #dma-cells.
I think the easiest way would be the first of those two, so it would
essentially look like
dmac: dma-mux {
compatible = renesas,shdma-mux; /* not simple-bus! */
#dma-cells = 1;
#address-cells = 1;
#size-cells = 1;
dma@1000 {
compatible = renesas,shdma;
reg = 0x1000 0x1000;
interrupts = 10;
};
dma@2000 {
compatible = renesas,shdma;
reg = 0x1000 0x1000;
interrupts = 10;
};
}
You then register a device driver for the master device, which
will call of_dma_controller_register() for itself and create
its child devices.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Monday 17 June 2013, Zhangfei Gao wrote:
Add dmaengine driver for hisilicon k3 platform based on virt_dma
Signed-off-by: Zhangfei Gao zhangfei@linaro.org
Tested-by: Kai Yang jean.yang...@huawei.com
Acked-by: Arnd Bergmann a...@arndb.de
diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt
b/Documentation/devicetree/bindings/dma/k3dma.txt
new file mode 100644
index 000..cf156f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/k3dma.txt
@@ -0,0 +1,44 @@
+* Hisilicon K3 DMA controller
+
+See dma.txt first
+
+Required properties:
+- compatible: Should be hisilicon,k3-dma-1.0
+- reg: Should contain DMA registers location and length.
+- interrupts: Should contain one interrupt shared by all channel
+- #dma-cells: see dma.txt, should be 1, para number
+- dma-channels: virtual channels supported, each virtual channel
+ have specific request line
+- clocks: clock required
+
+Example:
+
+Controller:
+ dma0: dma@fcd02000 {
+ compatible = hisilicon,k3-dma-1.0;
+ reg = 0xfcd02000 0x1000;
+ #dma-cells = 1;
+ dma-channels = 27;
+ interrupts = 0 12 4;
+ clocks = pclk;
+ status = disable;
+ };
+
+Client:
+Use specific request line passing from dmax
+For example, i2c0 read channel request line is 18, while write channel use 19
+
+ i2c0: i2c@fcb08000 {
+ compatible = snps,designware-i2c;
+ dmas = dma0 18 /* read channel */
+ dma0 19;/* write channel */
+ dma-names = rx, tx;
+ };
+
+ i2c1: i2c@fcb09000 {
+ compatible = snps,designware-i2c;
+ dmas = dma0 20 /* read channel */
+ dma0 21;/* write channel */
+ dma-names = rx, tx;
+ };
The binding looks good to me.
I'd like to make sure the dma-channels property is right though:
You specify that number in DT but ...
+#define DRIVER_NAME k3-dma
+#define NR_PHY_CHAN 16
+#define DMA_ALIGN3
... you also hardcode the number to 16. Shouldn't the channels be
allocated dynamically based on the dma-channels property?
You do allocate virtual channels based on the dma-channels
later. Wouldn't that be request line numbers, i.e. dma-requests
rather than dma-channels?
+static struct of_dma_filter_info k3_dma_filter;
+static bool k3_dma_filter_fn(struct dma_chan *chan, void *param)
+{
+ return (*(int *)param == chan-chan_id);
+}
This filter function works only as long as there is no more than
one DMA engine in the system, which is something that needs to
be documented better. Unfortunately, providing a filter
function to be called by .xlate is currently the only way that
the dma-engine API supports, but we should really get over that.
Vinod: I think we need to add a way for a dmaengine driver to
just return one of its channels to the xlate function. The
current method is getting very silly, and it adds run-time and
code complexity without any need.
How about something like
int dma_get_slave_channel(struct dma_chan *chan)
{
/* lock against __dma_request_channel */
mutex_lock(dma_list_mutex);
if (chan-client_count == 0)
ret = dma_chan_get(chan);
else
ret = -EBUSY;
mutex_unlock(dma_list_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(dma_get_slave_channel);
+ /* init virtual channel */
+ for (i = 0; i dma_channels; i++) {
+ struct k3_dma_chan *c;
+
+ c = devm_kzalloc(op-dev,
+ sizeof(struct k3_dma_chan), GFP_KERNEL);
+ if (c == NULL)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(c-node);
+ c-vc.desc_free = k3_dma_free_desc;
+ vchan_init(c-vc, d-slave);
+ }
Note that a single devm_kzalloc would be slightly more space efficient
here.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Friday 14 June 2013 12:53:11 Thierry Reding wrote: I think the biggest missing piece is pci_common_exit(), the counterpart of pci_common_init(), to cleanup a host bridge on ARM. I haven't looked in detail at the other architectures, but I suspect there must be some code to call when a host bridge is removed. Looking at drivers/pci/remove.c, it seems like pci_remove_root_bus() might be what we're looking at. It isn't exported so it can't be used by modules, but that can be changed. Is that how a host bridge is typically removed from the system? It's fairly new to have host bridges in loadable modules, so I'm pretty sure it's not supported by the core yet, but it also doesn't seem hard to do. I think you are right with regard to pci_remove_root_bus, and Bjorn might be able to provide more information. Ideally we should be able to load and unload the pci host driver in a loop indefinitely without crashing or exposing any races or leaks, but I would not bet on that working without bugs in the core, since that goes beyond the normal pci (device) hotplug case. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Friday 14 June 2013 17:18:46 Jingoo Han wrote:
On Thursday, June 13, 2013 11:14 PM, Arnd Bergmann wrote:
On Thursday 13 June 2013 22:22:31 Jingoo Han wrote:
+struct pcie_port {
+ struct device *dev;
+ u8 controller;
+ u8 root_bus_nr;
+ void __iomem*dbi_base;
+ void __iomem*elbi_base;
+ void __iomem*phy_base;
+ void __iomem*purple_base;
+ phys_addr_t cfg0_base;
+ void __iomem*va_cfg0_base;
+ phys_addr_t cfg1_base;
+ void __iomem*va_cfg1_base;
+ phys_addr_t io_base;
+ phys_addr_t mem_base;
+ spinlock_t conf_lock;
+ struct resource cfg;
+ struct resource io;
+ struct resource mem;
+ struct pcie_port_info config;
+ struct clk *clk;
+ struct clk *bus_clk;
+ int irq;
+ int reset_gpio;
+};
You mentioned that you'd use u64 for the resources here but did not.
Do you mean the following?
+ u64 cfg0_base;
+ u64 cfg1_base;
+ u64 io_base;
+ u64 mem_base;
Yes, anything you need to program into a 64 bit hardware register.
+static void exynos_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32
busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : OUTBOUND : CFG1 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG1, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-cfg1_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-cfg1_base + pp-config.cfg1_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
And you still don't set up the UPPER halves, which was really the point
of my comment. Same thing for all the other ones.
Do you mean the following?
+ writel_rc(pp, pp-cfg1_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, (pp-cfg1_base 32), dbi_base + PCIE_ATU_UPPER_BASE);
Right. Note that you could achieve the same using phys_addr_t instead of
u64, but it's more complicated to get that to work for both 32 and 64 bit
phys_addr_t types, since you cannot shift a 32 bit value by 32 bits in C.
+static void exynos_pcie_prog_viewport_mem_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : INBOUND : MEM */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_MEM, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE | PCIE_ATU_BAR_MODE_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr,
+ dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr + pp-config.mem_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u64)pp-mem_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
This makes even less sense than before, it seems like now you only
allow DMA between PCI devices but not to main memory.
Sorry, I cannot understand it.
Could you give me more details?
Also, pseudo-code will be very helpful.
Please look up the documentation about inbound viewport and describe
in a code comment what it does. I /assume/ that this is how DMA accesses
from the bus get translated into AXI bus transactions. If so, you have
to let the window translate addresses from RAM. If it's something else,
then you should document what it is and how it needs to be set up.
+static int exynos_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+ struct pcie_port *pp;
+
+ pp = sys_to_pcie(sys);
+
+ if (!pp)
+ return 0;
+
+ pci_add_resource(sys-resources, pp-io);
+ pci_add_resource(sys-resources, pp-mem);
Now you are missing the offsets. You have to at least pass an offset for one
of the I/O spaces, since they are using the same bus address. The offset
must
match the value you pass to pci_ioremap_io.
For the memory space, you should pass the difference between the physical
base address and the bus address. That address happens to be zero with
you DT setup, but I would advise to also try out some other values in DT,
just to ensure it still works.
Um, I cannot understand it, too.
Could you give me Psuedo-code
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Thursday 13 June 2013 22:22:31 Jingoo Han wrote:
+struct pcie_port_info {
+ u32 cfg0_size;
+ u32 cfg1_size;
+ u32 io_size;
+ u32 mem_size;
+ phys_addr_t io_bus_addr;
+ phys_addr_t mem_bus_addr;
+};
+
+struct pcie_port {
+ struct device *dev;
+ u8 controller;
+ u8 root_bus_nr;
+ void __iomem*dbi_base;
+ void __iomem*elbi_base;
+ void __iomem*phy_base;
+ void __iomem*purple_base;
+ phys_addr_t cfg0_base;
+ void __iomem*va_cfg0_base;
+ phys_addr_t cfg1_base;
+ void __iomem*va_cfg1_base;
+ phys_addr_t io_base;
+ phys_addr_t mem_base;
+ spinlock_t conf_lock;
+ struct resource cfg;
+ struct resource io;
+ struct resource mem;
+ struct pcie_port_info config;
+ struct clk *clk;
+ struct clk *bus_clk;
+ int irq;
+ int reset_gpio;
+};
You mentioned that you'd use u64 for the resources here but did not.
+
+static void exynos_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : OUTBOUND : CFG0 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, (u64)pp-cfg0_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-cfg0_base + pp-config.cfg0_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG0, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+}
+
+static void exynos_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : OUTBOUND : CFG1 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG1, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-cfg1_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-cfg1_base + pp-config.cfg1_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
And you still don't set up the UPPER halves, which was really the point
of my comment. Same thing for all the other ones.
+static void exynos_pcie_prog_viewport_mem_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : INBOUND : MEM */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_MEM, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE | PCIE_ATU_BAR_MODE_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr,
+ dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr + pp-config.mem_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u64)pp-mem_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
This makes even less sense than before, it seems like now you only
allow DMA between PCI devices but not to main memory.
+static void exynos_pcie_prog_viewport_io_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : INBOUND : IO */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_IO, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE | PCIE_ATU_BAR_MODE_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-config.io_bus_addr,
+ dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-config.io_bus_addr + pp-config.io_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u64)pp-io_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0,
Re: [PATCH V4 1/3] pci: Add PCIe driver for Samsung Exynos
On Thursday 13 June 2013 22:18:50 Jingoo Han wrote:
On Wednesday, June 12, 2013 8:23 PM, Arnd Bergmann wrote:
On Wednesday 12 June 2013 19:19:05 Jingoo Han wrote:
+
+/* synopsis specific PCIE configuration registers*/
+#define PCIE_PORT_LINK_CONTROL 0x710
+#define PORT_LINK_MODE_MASK (0x3f 16)
+#define PORT_LINK_MODE_4_LANES (0x7 16)
Do you mean this is a Synopsys designware part? In that case it
should really not be called exynos-pcie but designware-pcie
and you should make sure that the driver makes no assumptions about
the platform. A lot of other platforms also use designware
parts and should be able to reuse this driver.
Sorry, I don't think so.
Only core block is a Synopsys designware part IP block,
other parts are Exynos-specific.
So, it is hard to share with other PCIe IPs using synopsis core.
Just call it synopsys anyway and put a comment in to explain this.
That should be enough for the next person with a synopsys PCI core
to reuse your code and split out the exynos specific parts into a
separate file.
I think you should not assume that the physical base address is a 32
bit value. The hardware clearly supports lower and upper halves
for the address window, so when resource_size_t is 64 bit, you should
set the upper half accordingly. Since the hardware is always 64 bit,
you can use a u64 type rather than resource_size_t to simplify the
code here.
OK, I will replace u32 with u64.
Not what I meant, please read it again.
+static void exynos_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : OUTBOUND : IO */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_IO, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u32)pp-io_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u32)(pp-io_base + pp-config.io_size - 1),
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u32)pp-io_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
You don't actually map the I/O space anywhere into virtual memory.
I think you need to call pci_ioremap_io with the pp-io_base at
boot time.
Sorry, when pci_ioremap_io() is used, Exynos5440 hangs.
I don't know how to deal this.
You have to call it, there is no way around that. You will have to debug
this yourself, I'm sure there is an easy solution.
You hardcode the in_mem_size to 256 MB. Does that mean you only allow
PCI bus master DMA on the first part of RAM? Shouldn't it get
computed from the actual location and size of RAM?
I will remove the hard-coded in_mem_size, instead use the size of MEM region.
This should be system RAM, not the PCI memory space. What good would
it be to have an inbound window that only redirects back to the bus?
+static void exynos_pcie_prog_viewport_io_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+ struct pcie_port_info *config = pp-config;
+
+ /* Program viewport 1 : INBOUND : IO */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_IO, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE | PCIE_ATU_BAR_MODE_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, config-in_mem_size - 1, dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
I don't understand what in inbound I/O access actually means. What
does this do, is it for PCI target emulation?
I reviewed the manual, and I will fix it.
I will add bus addresses and host physical addresses to
PCIE_ATU_LOWER_BASE/ PCIE_ATU_LOWER_TARGET.
You have not answered my question.
+static int exynos_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+ struct pcie_port *pp;
+
+ pp = sys_to_pcie(sys);
+
+ if (!pp)
+ return 0;
+
+ pci_add_resource_offset(sys-resources, pp-io, sys-io_offset);
+ pci_add_resource_offset(sys-resources, pp-mem, sys-mem_offset);
+
+ return 1;
+}
You don't actually set up io_offset and mem_offset, right?
OK, I will replace pci_add_resource_offset() with pci_add_resource().
No, you have to set up the offsets.
Please don't post another version until you actually understand what
you need to do. If you don't understand the comments, ask back rather
than picking a random interpretation.
Arnd
Re: [PATCH V4 3/3] ARM: dts: Add pcie controller node for Samsung EXYNOS5440 SoC
Thanks for the update! A few comments again:
On Wednesday 12 June 2013 19:20:00 Jingoo Han wrote:
diff --git a/arch/arm/boot/dts/exynos5440-ssdk5440.dts
b/arch/arm/boot/dts/exynos5440-ssdk5440.dts
index d55042b..efe7d39 100644
--- a/arch/arm/boot/dts/exynos5440-ssdk5440.dts
+++ b/arch/arm/boot/dts/exynos5440-ssdk5440.dts
@@ -30,4 +30,12 @@
clock-frequency = 5000;
};
};
+
+ pcie0@4000 {
+ reset-gpio = 5;
+ };
+
+ pcie1@6000 {
+ reset-gpio = 22;
+ };
};
As mentioned before, please use the gpio binding to pass gpio numbers.
diff --git a/arch/arm/boot/dts/exynos5440.dtsi
b/arch/arm/boot/dts/exynos5440.dtsi
index f6b1c89..2c15f9d 100644
--- a/arch/arm/boot/dts/exynos5440.dtsi
+++ b/arch/arm/boot/dts/exynos5440.dtsi
@@ -216,4 +216,42 @@
clock-names = rtc;
status = disabled;
};
+
+ pcie0@0x29 {
+ compatible = samsung,exynos5440-pcie;
+ reg = 0x29 0x1000
+ 0x27 0x1000
+ 0x271000 0x40;
+ interrupts = 0 20 0, 0 21 0, 0 22 0;
+ clocks = clock 28, clock 27;
+ clock-names = pcie, pcie_bus;
+ #address-cells = ;
+ #size-cells = 2;
+ device_type = pci;
+ ranges = 0x0800 0 0x4000 0x4000 0 0x0020
/* configuration space */
+ 0x8100 0 0 0x4020 0 0x0001
/* downstream I/O */
+ 0x8200 0 0x4021 0x4021 0 0x1000;
/* non-prefetchable memory */
I think you did not reply to my question regarding the size of the
memory space. Does it extend from 0x4021 to 0x5021,
or from 0x4021 to 0x5000. You probably meant the latter
but wrote the former. If not, please add a comment for clarification.
+ #interrupt-cells = 1;
+ interrupt-map-mask = 0 0 0 0;
+ interrupt-map = 0x0 0 gic 53;
+ };
So all PCI IntA interrupts are mapped to a single gic interrupt? That
sounds like a bottleneck when you have a lot of devices on the bus.
Do you have MSI support?
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V4 1/3] pci: Add PCIe driver for Samsung Exynos
On Wednesday 12 June 2013 19:19:05 Jingoo Han wrote:
+
+struct pcie_port {
+ struct device *dev;
+ u8 controller;
+ u8 root_bus_nr;
+ void __iomem*dbi_base;
+ void __iomem*elbi_base;
+ void __iomem*phy_base;
+ void __iomem*purple_base;
+ phys_addr_t cfg0_base;
+ void __iomem*va_cfg0_base;
+ phys_addr_t cfg1_base;
+ void __iomem*va_cfg1_base;
+ phys_addr_t io_base;
+ phys_addr_t mem_base;
+ spinlock_t conf_lock;
+ struct resource cfg;
+ struct resource io;
+ struct resource mem;
+ struct pcie_port_info config;
+ struct clk *clk;
+ struct clk *bus_clk;
+ int irq;
+ int reset_gpio;
+};
This looks much better now.
+
+/* synopsis specific PCIE configuration registers*/
+#define PCIE_PORT_LINK_CONTROL 0x710
+#define PORT_LINK_MODE_MASK (0x3f 16)
+#define PORT_LINK_MODE_4_LANES (0x7 16)
Do you mean this is a Synopsys designware part? In that case it
should really not be called exynos-pcie but designware-pcie
and you should make sure that the driver makes no assumptions about
the platform. A lot of other platforms also use designware
parts and should be able to reuse this driver.
+static void exynos_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : OUTBOUND : CFG0 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, (u32)pp-cfg0_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u32)pp-cfg0_base + pp-config.cfg0_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG0, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+}
I think you should not assume that the physical base address is a 32
bit value. The hardware clearly supports lower and upper halves
for the address window, so when resource_size_t is 64 bit, you should
set the upper half accordingly. Since the hardware is always 64 bit,
you can use a u64 type rather than resource_size_t to simplify the
code here.
+static void exynos_pcie_prog_viewport_mem_outbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : OUTBOUND : MEM */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_MEM, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u32)pp-mem_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u32)(pp-mem_base + pp-config.mem_size - 1),
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u32)pp-mem_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
You probably should not assume that there is a 1:1 mapping between
bus addresses and host physical addresses, but rather read both
values from the DT individually. With the ranges defined as
0x8200 0 0x4021 0x4021 0 0x1000; /* non-prefetchable
memory */
the second and third cell should go into
PCIE_ATU_UPPER_TARGET/PCIE_ATU_LOWER_TARGET, while the translated address
(from the third cell) should go into PCIE_ATU_LOWER_BASE/PCIE_ATU_UPPER_BASE
The PCIE_ATU_LIMIT seems to correctly get translated from the last
cell.
+static void exynos_pcie_prog_viewport_io_outbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : OUTBOUND : IO */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_IO, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u32)pp-io_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u32)(pp-io_base + pp-config.io_size - 1),
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u32)pp-io_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
You don't actually map the I/O space anywhere
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Wednesday 12 June 2013 12:07:46 Grant Likely wrote: It actually seems a bit silly to put the internal regs into the ranges property at all. It's not like they need to be translated or provided to any child nodes. Just give the root node a reg property with the correct base for the internal regs. I think you are missing some background about the internal-regs: * These are SoC-wide registers including the UART and other things, not just the mbus setup * There are at least two different values used for the internal-regs mapping address depending on the SoC and boot loader version * We have to get the address from the DT since it is impossible to probe or change without knowing the current setting: The internal-regs also contains the registers in mbus used to relocate the internal-regs. As for regenerating the ranges; I have no problem with the kernel allocating ranges at runtime, but that code should not be creating a new ranges property and adding it to the tree. The knowledge should be kept internal to the driver and it should use an of_bus translator (drivers/of/address.c) to tap into the ranges parsings code. Ok, makes sense. I didn't know about the of_bus code. Thanks for the pointer! Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Wednesday 12 June 2013 12:54:59 Grant Likely wrote: * These are SoC-wide registers including the UART and other things, not just the mbus setup * There are at least two different values used for the internal-regs mapping address depending on the SoC and boot loader version * We have to get the address from the DT since it is impossible to probe or change without knowing the current setting: The internal-regs also contains the registers in mbus used to relocate the internal-regs. I wasn't actually suggesting to not get the address from DT, but your point is taken; there is a lot more to internal regs than I had assumed. Right, the first point I think is the important one, the rest was just information added in. At the moment, we actually hardcode the address in the kernel, but that is changing. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V3 1/3] Add PCIe driver for Samsung Exynos
On Wednesday 12 June 2013, Jingoo Han wrote:
On Friday, June 07, 2013 7:53 PM, Arnd Bergmann wrote:
On Friday 07 June 2013 18:22:50 Jingoo Han wrote:
+- ranges: ranges for the PCI memory and I/O regions
+- reset-gpio: gpio pin number of power good signal
The 'reset-gpio' property seems incorrect. I think this should either
use the gpio binding or the reset-controller binding. Specifying
bare numbers to use as gpio pins does not work, since the number
space for Linux internal gpio numbers is not necessarily the same
as used by the hardware.
As you mentioned, other Exynos SoCs such as Exynos5250 set
GPIO properties in DT, as below:
(./arch/arm/boot/dts/exynos5250-smdk5250.dts)
hdmi {
hpd-gpio = gpx3 7 0;
};
usb@1211 {
samsung,vbus-gpio = gpx2 6 0;
};
However, the situation of Exynos5440 GPIO is different.
The following bare numbers of GPIO work properly on Exynos5440.
(./arch/arm/boot/dts/exynos5440-ssdk5440.dts)
pcie0@4000 {
reset-gpio = 5;
}
pcie0@4000 {
reset-gpio = 22;
}
Thomas Abraham is the author of pinctrl driver for EXYNOS5440.
(./drivers/pinctrl/pinctrl-exynos5440.c)
Thomas Abraham or Kukjin Kim, can you confirm this?
If I am wrong, please let me know kindly. :)
This is not about the code working at the moment, it is about
it being correct. The current method you are using would stop
working if something changes in the pinctrl code, and would
not be portable to other SoCs.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 6/6] ARM: dts: Add pcie controller node for Samsung EXYNOS5440 SoC
On Tuesday 11 June 2013, Jingoo Han wrote: + ranges = 0x0800 0 0x4000 0x4000 0 0x0020 /* configuration space */ + 0x8100 0 0 0x4020 0 0x4000 /* downstream I/O */ + 0x8200 0 0 0x40204000 0 0x1000; /* non-prefetchable memory */ + }; ... Also, shouldn't your memory space end on a 256MB boundary, rather than extend up to 0x50203fff? According to the manual of Exynos PCIe, each memory space for Exynos PCIe can support 512MB, including I/O, CFG regions. Is there any problem when over 256MB boundary is used? Please let me know. :) No, that's not a problem, but I think you should have the window span the entire space that is provided in hardware. If there are 512 MB total, why not use them? You could use ranges = 0x8200 0 0 0x40204000 0 0x1fdfc000; to pass a range for the memory space that extends all the way until 0x5fff. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Wednesday 12 June 2013, Grant Likely wrote: I think we should have almost everything needed already to do this. of_bus allows arbitrary mapping code to be used at any stage in the translation. We might need to add some glue so that of_busses[] can be assembled by the linker to allow an mbus of_bus stanza to live outside of drivers/of/address.c Actually, the best thing about this solution is that we don't even have to bother setting up the mappings when loading the mbus driver: We don't need any ranges (other than internal-regs) in DT, and we don't need complex logic to search through the child devices to figure out what the mappings should be. The only thing one needs to do here is check if a mapping already exists when we get into the of_bus handler and create one for the device being translated if there isn't one! Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Wednesday 12 June 2013, Ezequiel Garcia wrote:
#define INTERNAL_REGS INTERNAL_REGS_ID 0
#define DEVBUS_BOOTCS 0x012f 0
#define BOOTROM 0x01e0 0
#define INTERNAL_REGS_BASE 0 0xd000
#define DEVBUS_BOOTCS_BASE 0 0xf000
#define BOOTROM_BASE 0 0xfff0
...
soc {
ranges = INTERNAL_REGS INTERNAL_REGS_BASE 0x10
BOOTROM BOOTROM_BASE 0x10
DEVBUS_BOOTCS DEVBUS_BOOTCS_BASE 0x100;
FWIW, I think using macros to pass INTERNAL_REGS_BASE is counterproductive.
It's much better to use literal numbers for those because then you
can see how things fit together by looking at the source.
The macros for the IDs seem fine, although I would actually find it
easier to have just one macro like
#define MBUS_ID(a, b) (((a) 0xff) 24 | ((b) 0xff) 16)
and pass the literals into that, to the effect of
soc {
ranges = 1 0 0 0xd000 0x10,
MBUS_ID(0x01, 0x2f) 0 0 0xfff0 0x10,
MBUS_ID(0x01, 0xe0) 0 0 0xf000 0x10;
I'd suggest having each macro expand only to a single value, to make
it clearer which numbers belong together in the ranges array.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Wednesday 12 June 2013, Ezequiel Garcia wrote: Right. And just to confirm: this kernel side dynamic implementation will be completely independent of the MBus DT layout proposal. So I think it's best to agree on this binding first. I'll post a v2 with the progress I've made using the preprocessor. Right. My thought was just that with the of_bus handler it might end up being /easier/ to do the dynamic mapping than it is to preconfigure it in the DT. Whether that results in an optimum mapping or not depends on the actual sizes for the child nodes. Since everything is a power-of-two size, a first-fit algorithm actually isn't bad at all. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Thursday 13 June 2013, Jason Gunthorpe wrote: On Wed, Jun 12, 2013 at 11:52:32PM +0200, Arnd Bergmann wrote: Whether that results in an optimum mapping or not depends on the actual sizes for the child nodes. Since everything is a power-of-two size, a first-fit algorithm actually isn't bad at all. The windows must be aligned to their size. eg 1M size window must be aligned to a 1M boundary. First fit only works if you allocate from largest alignment requirement to smallest, otherwise the worst case is something like nearly double the largest alignment wasted in alignment padding. Yes, I realize that, which is why I said it depends on the actual sizes. If you have a lot of windows that are all the same size and nothing larger, the worst case isn't too bad. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Thursday 13 June 2013, Arnd Bergmann wrote: Yes, I realize that, which is why I said it depends on the actual sizes. If you have a lot of windows that are all the same size and nothing larger, the worst case isn't too bad. Also, if the available space is a naturally aligned address range with a power-of-two size itself, the order does not matter at all and any order is as good as any other. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Tuesday 11 June 2013 10:31:45 Ezequiel Garcia wrote:
+
+ soc {
+ bootrom {
+ ranges = 0 0x01e0 0xfff0 0x10;
+ };
+ };
I think I'm a bit lost here. Is the soc node in this example the node
that is described as compatible=marvell,armada370-mbus? Maybe expand
the example a little here to clarify this.
Exactly: that's the mbus-compatible node. I'll clarify a bit the documentation
so this is clearer.
I kept the 'soc' name in the examples as that's the name in the dts files.
Do you think I should rename to 'mbus' in the dts files?
I haven't done that since that would imply to change all the dts
files.
The name of the device node is not very important, so you can leave
it alone if it causes problems, or change it something else if you
find it more appropriate.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Tuesday 11 June 2013 10:57:38 Ezequiel Garcia wrote:
Jason, Arnd:
On Sat, Jun 08, 2013 at 07:45:06PM -0600, Jason Gunthorpe wrote:
[...]
This is the mangling I was referring to. It needs to be the offset
into the target, it can't be something else.
I see both of you have explained this already, but I can't seem
to catch it yet.
Mind explaining me exaclty *why* it can't be something else?
IOW, why the window base address cannot be in the 2nd cell,
and/or what use cases would this encoding brake?
Most importantly that would meant that the 'ranges' property
does not actually reflect the setting in the hardware mbus window,
which in fact remaps a base address of the upstream bus to address
zero of the downstream bus, and that address has to be offsettable.
Taking your example from below:
ranges = 0x011d 0 0xfff0 0x010; /* BootROM */
You map the start '0' of the boot rom to 0xfff. Your proposal
would mean setting up the intermediate address space to use the
same offset that is used on the host side, like
ranges = 0x011d 0xfff0 0xfff0 0x010;
This essentially you would have to set up the mbus window so the
bus address subtracts the second cell from the third cell to
get the child bus address, right?
One clear example where it would break down is when you have a
PCI bus window and e.g. map the PCI bus address 0xd000 to
host physical address 0xd000. Doing the same calculation
would mean you need a ranges property like
ranges = 0x0abc 0xa000 0xd000 0x0100;
so that when setting up the mbus you can subtract 0xd000
from 0xa000 and get back (wrapping around through 0) to
the address 0xd000 that you actually want. That would be
really stupid and illogical.
Finally, it makes setting up the ranges at boot time extremely
hard, unless you always want to map each device to the same
physical address (which is not always possible), because you
would have to not only change the ranges property of the mbus
node when you change the window, but also every single 'reg'
or 'ranges' property in the children referring to it.
I understand your motivation, but this is not a good method of
encoding this in DT.
There is nothing especially wrong with updating the ranges at runtime,
but don't use the 2nd cell in the 2dw address to encode the desired
base address.
Regarding this, let me confirm your proposed layout using an example.
(later on, i'll try the preprocessor on this to make it look better).
mbus {
compatible = foo-mbus;
ranges = 0x0001 0 0xf100 0x100 /* Internal register entry
*/
0x011d 0 0xfff0 0x010 /* BootROM entry with base
address */
0x012f 0 0xe800 0x100 /* DevBus,NOR entry with
base address */
;
bootrom {
ranges = 0 0x011d 0 0x10;
};
devbus-bootcs {
ranges = 0 0x012f 0 0x100;
nor {
reg = 0 0x100;
};
};
};
Is the above correct?
That looks ok to me, yes. If you have just one device under some of the nodes
however, I think it's easier use an empty ranges property and do
devbus-bootcs {
ranges;
nor {
reg = 0x012f 0 0x100;
};
The 'ranges' here is most useful if you have multiple devices
add different offsets. I would also predefine those ranges to
be as large as possible so you don't have to adapt them when
the child device grows beyond it.
Now, if the above is OK, then the DT looks complete to me. Meaning:
1. There's no need to update any ranges property at runtime.
You still have to update the ranges every time you add, remove
or update a mapping window.
2. There's no need to use an algorithm to 'find' a base address for decoding
windows (such as first-fit as Arnd suggested in another mail).
The base address is already there in the ranges property, so the mbus can
simply allocate the window on that base address.
So, am I right or completely lost?
Since you already need code to update the ranges property, I think
it's best to discard the ranges at boot time and create a new mapping
that just maps the devices you actually want to use. That would make
it much easier to cope with boot loaders that don't set up the
mappings in the same way that is listed in the device tree. Just
put the one entry for the internal-regs in the ranges, since that
is needed to map do the mbus setup.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Tuesday 11 June 2013 15:50:23 Jason Gunthorpe wrote:
On Tue, Jun 11, 2013 at 05:26:47PM +0200, Arnd Bergmann wrote:
That looks ok to me, yes. If you have just one device under some of the
nodes
however, I think it's easier use an empty ranges property and do
devbus-bootcs {
ranges;
nor {
reg = 0x012f 0 0x100;
};
The 'ranges' here is most useful if you have multiple devices
add different offsets. I would also predefine those ranges to
be as large as possible so you don't have to adapt them when
the child device grows beyond it.
It isn't super critical, but the ranges does keep the 0x012f value
in the SOC .dtsi and the board level doesn't have to be exposed to the
value, it just uses 0 to setup the NOR.
Yes, good point.
Also, it makes it much simpler for the mbus driver to detect which
target id's are required so they can be allocated/setup - the rule
would be every child stanza should have a ranges with the target(s) it
needs.
Not sure if this is a good rule: As you said, it makes sense to
put the 'ranges' for the devbus-bootcs node into the per-soc .dtsi
file so the board file doesn't have to care. However the mbus code
needs both the 0x012f and the start/length of the child. Mapping
the entire window would work in theory but is likely going to cause
a significant waste of physical address space, because the (per-soc)
ranges property has to be set up for the largest possible external
device connected to the bus, but the mbus window only needs to cover the
device that is actually connected.
The full version probably looks like this:
mbus {
ranges
devbus-bootcs@10400 {
compatible = marvell,mvebu-devbus;
reg = INTERNAL_REGS + 0x10400 0x8;
ranges = 0 DEVBUS_BOOTCS 0x100;
#address-cells = 1;
#size-cells = 1;
clocks = coreclk 0;
// In board .dts:
nor@0 {
compatible = cfi-flash;
reg = 0 0x100;
bank-width = 2;
};
};
};
And notice here that we need to offset INTERNAL_REGS to make this
work. To me, this is the primary reason why the 2nd cell of the mbus
address format *must* be the address offset within the target, and not
anything else.
Right.
2. There's no need to use an algorithm to 'find' a base address for
decoding
windows (such as first-fit as Arnd suggested in another mail).
The base address is already there in the ranges property, so the mbus
can
simply allocate the window on that base address.
So, am I right or completely lost?
Since you already need code to update the ranges property, I think
it's best to discard the ranges at boot time and create a new mapping
that just maps the devices you actually want to use. That would make
it much easier to cope with boot loaders that don't set up the
mappings in the same way that is listed in the device tree. Just
put the one entry for the internal-regs in the ranges, since that
is needed to map do the mbus setup.
So, I think we can/have agree that the ranges in the FDT should
reflect the bootloader's settings, and if the ranges is missing an
element it means the bootloader didn't set it up.
A compatible bootloader should dump the entire mbus register table
into the ranges. The address encoding is such that we can represent
every mbus window entry in ranges. If no bootloader support is present
then the ranges value is forced into the mbus windows anyhow.
If there isn't a ranges entry, but there is a child that requires it
(how do we figure this out?)
I think we have to walk the entire tree of devices underneath mbus,
at least any device node that has a 'reg' property, following children
of any device node with a 'ranges' property. We might need to
add a variant of of_get_address() that does a partial translation
up to a given node (the mbus) instead of all the way to the root.
then dynamic assignment of that window
makes sense to me. (however, this also looks a bit tricky, how do you
avoid hitting the PCI-E window reservations, for instance?)
The PCI-E window would have 'ranges' but no 'regs', so we automatically
skip it.
Unconditional dynamic assignment is a bit more tricky, for instance
the boot rom has to be located at a specific address since it is used
as the SMP secondary startup trampoline. (Ezequiel: IMHO, you should
check for this and bail secondary startup if it is not setup)
I suppose we can identify the devices we don't want to reassign by
their compatible
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Wednesday 12 June 2013 00:22:29 Sebastian Hesselbarth wrote: When removing size from marvell,mbus-target above, mbus driver could also probe for required max size from the reg properties of the child nodes. Arnd already said that mbus isn't simple-bus anymore, why can't it just walk through the nodes and collect required windows? It would work, but feels wrong to me. The 'reg' properties of each device behind mbus would end up as software defined and need to be updated for any change to the mapping, whether that change is board specific or even at run-time. It would be even harder to do dynamic assignment when you cannot put a proper address into 'reg'. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Tuesday 11 June 2013 16:58:41 Jason Gunthorpe wrote: On Wed, Jun 12, 2013 at 12:34:14AM +0200, Arnd Bergmann wrote: a significant waste of physical address space, because the (per-soc) ranges property has to be set up for the largest possible external device connected to the bus, but the mbus window only needs to cover the device that is actually connected. Yes, but perhaps that is a good argument to not put the ranges in the dtsi? Fair enough. In that case not using ranges at all is probably easier so the board file only needs to update one place. I think we have to walk the entire tree of devices underneath mbus, at least any device node that has a 'reg' property, following children of any device node with a 'ranges' property. We might need to add a variant of of_get_address() that does a partial translation up to a given node (the mbus) instead of all the way to the root. Hum, how complex do you think this is? Run down the entire tree, translate every regs and maintain a max offset for every target id. It's the same thing we do for PCI. then dynamic assignment of that window makes sense to me. (however, this also looks a bit tricky, how do you avoid hitting the PCI-E window reservations, for instance?) The PCI-E window would have 'ranges' but no 'regs', so we automatically skip it. The trouble is not skipping PCI-E when parsing the dt. When the kernel goes to make a dynamic window allocation it needs to exclude the address space reserved for the PCI-E aperture. The mbus driver starts before the PCI-E driver, so how can this be resolved? Hardwire specialness about PCI-E into mbus? Make the PCI-E aperture fully dynamic? Starting to be pretty complex - for what gain? I think this can be simplified a lot by making PCI a known special case: We assign all regular (non-PCI) windows from the top, and whatever is left by the time PCI comes up can be used for that, starting at the bottom of the avaliable space. I'm not sure there is a good reason to reject the address map in the DT? You mean keep all windows that are listed by the boot loader but reassign the others? I guess it would be simpler but may result in a less optimal address map. Sure, but what does a more optimal address map get you? 1) More space to map SDRAM - however - the SDRAM map is controlled exclusively by the bootloader and the kernel doesn't have the information or the code to mess with it - so this isn't possible 2) More space for the PCI-E aperture - this is entirely controlled by the kernel, but today we are using the pre-set allocation from the DT, so it doesn't matter how tightly the rest of the stuff is packed. To me, it feels like alot of complex trouble to do actual dynamic allocation, and I don't really see any gain. We could of course decide to skip the dynamic part in the Linux implementation for the moment, but I would definitely want to see the binding written in a way that going fully dynamic can be done later without changing the binding if we decide we need to squeeze out more RAM or PCI space. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 07/11] ARM:stixxxx: Add STiH416 SOC support
On Monday 10 June 2013 10:27:05 Srinivas KANDAGATLA wrote:
+ soc {
+ pin-controller-sbc {
+ #address-cells = 1;
+ #size-cells = 1;
+ compatible = st,stih416-pinctrl, simple-bus;
Why is this both its own device with a compatible string and a
simple-bus at the same time? Wouldn't it be simpler to just
scan the child device nodes from the st,stih416-pinctrl
driver instead of having a separate platform_driver for them?
+ st,retime-in-delay = 0 300 500 750 1000 1250 1500
1750 2000 2250 2500 2750 3000 3250;
+ st,retime-out-delay = 0 300 500 750 1000 1250 1500
1750 2000 2250 2500 2750 3000 3250;
+ st,syscfg = syscfg_sbc;
+ st,syscfg-offsets = 0 40 50 60 100;
+ ranges;
+ PIO0: pinctrl@fe61 {
+ #gpio-cells = 1;
+ compatible = st,sti-gpio;
+ gpio-controller;
+ reg = 0xfe61 0x100;
+ st,bank-name = PIO0;
+ st,retime-pin-mask = 0xff;
+ };
+ PIO1: pinctrl@fe611000 {
+ #gpio-cells = 1;
+ compatible = st,sti-gpio;
+ gpio-controller;
+ reg = 0xfe611000 0x100;
+ st,bank-name = PIO1;
+ st,retime-pin-mask = 0xff;
+ };
What is in the ranges between these registers? It seems you have
256 bytes for each pinctrl node, with 4kb spacing. I wonder if
it would make sense to declare the entire range to belong to a single
pinctrl device. At least since all of the registers are in a single
range, you could add a property like
ranges = 0 0xfe61 0x1;
and use relative addresses in the sub-nodes.
Please don't use identifiers with 'xxx' in them. Instead use numbers
of actual chips, ideally using the first one that this is compatible
with.
+ syscfg_sbc:syscfg@fe60{
+ compatible = st,stih416-syscfg;
+ reg = 0xfe60 0x1000;
+ syscfg-range= 0 999;
+ syscfg-name = SYSCFG_SBC;
+ };
+ syscfg_front:syscfg@fee1{
+ compatible = st,stih416-syscfg;
+ reg = 0xfee1 0x1000;
+ syscfg-range= 1000 999;
+ syscfg-name = SYSCFG_FRONT;
+ };
Did you mean to declare ranges excluding 1000 and 2000 here?
Normally I would expect inclusive ranges like syscfg-range=0 1000;
What is the idea of the 'syscfg-name'? If the nodes are all different,
I would expect them to have distinct compatible values and not
need them.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 04/11] mfd:stixxxx-syscfg: Add ST System Configuration support.
On Monday 10 June 2013 14:52:38 Srinivas KANDAGATLA wrote: On 10/06/13 14:16, Linus Walleij wrote: On Mon, Jun 10, 2013 at 11:22 AM, Srinivas KANDAGATLA srinivas.kandaga...@st.com wrote: This mfd driver provides higher level inialization routines for various IPs like Ethernet, USB, PCIE, SATA and so on. Also it provides way to get to syscfg registers via standard regmap api which is usefull for drivers like pinctrl. This patch adds support to ST System Configuration registers, which can be configured by the drivers. Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com CC: Stuart Menefy stuart.men...@st.com CC: Stephen Gallimore stephen.gallim...@st.com CC: Linus Walleij linus.wall...@linaro.org CC: Mark Brown broo...@kernel.org What is this driver doing that drivers/mfd/syscon.c is not already doing? As of now, the driver is very much similar to syscon + some additional functionality, but we are planning to use this file to add higher level functions to configure different IPs like ethernet, usb, power, reset and so on which are very much specific to ST System Configuration Registers. I was expecting that you'd actually interface with the syscon code and build on top, rather than copy it. There are multiple ways of doing that, e.g. you could export a function from syscon.c that you call to register the device node and then import the regmap from syscon into your high-level driver again. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 6/6] ARM: dts: Add pcie controller node for Samsung EXYNOS5440 SoC
On Monday 10 June 2013, Jingoo Han wrote:
On Saturday, June 08, 2013 2:43 AM, Arnd Bergmann wrote:
For multiple domains, how can I fix the DT properties?
Domains are a Linux concept, you have to pick a new domain number for each
struct hw_pci you register.
Current DT properties are as below:
+ pcie0@4000 {
+ compatible = samsung,exynos5440-pcie;
+ reg = 0x4000 0x4000
+ 0x29 0x1000
+ 0x27 0x1000
+ 0x271000 0x40;
+ interrupts = 0 20 0, 0 21 0, 0 22 0;
+ #address-cells = 3;
+ #size-cells = 2;
+ device_type = pci;
+ ranges = 0x0800 0 0x4000 0x4000 0 0x0020 /*
configuration space */
+ 0x8100 0 0 0x4020 0 0x4000 /*
downstream I/O */
+ 0x8200 0 0 0x40204000 0 0x1000; /*
non-prefetchable memory */
+ };
An unrelated comment: your first reg field seems to overlap with part
of your configuration space. Is that intentional?
Also, shouldn't your memory space end on a 256MB boundary, rather than
extend up to 0x50203fff?
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Saturday 08 June 2013 15:38:52 Ezequiel Garcia wrote:
On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote:
Right. I think we have two options here for laying the DT ranges.
1) This is the proposal implied in the patchset I sent:
mbus {
ranges = we only put the internal-reg translation here
devbus-bootcs {
ranges = 0 {target_id/attribute} {window_physical_base} {size}
}
}
As Jason explained, you cannot have the window_physical_base in the child
device, that just wouldn't work. I don't know if that's a typo or a thinko ;-)
I also think {size} above there would just be 0x, right?
2) This is what Jason is proposing in his mail:
mbus {
ranges = {target_id/attribute} 0 {window_physical_base} {size}
devbus-bootcs {
ranges = 0 {target_id/attribute} 0 {size}
}
}
Of course this looks much cleaner, but it forces a lot of duplication
in the DT files. Now, if you see some of the recent patches we've been
sending, I think this duplication is very error-prone, and it'll be a
nightmare to maintain. Let me propose an example to show this
duplication:
I don't see where that duplication comes in. The ranges in the
devbus-bootcs are just describing how the hardware maps the
child address space into the global mbus space, and that never
changes. For the cases that only have a single device underneath,
you can actually put an empty ranges property in there and adapt
the regs property of whatever sits underneath it. These two
representations would do exactly the same for instance:
a)
mbus {
ranges = ...;
devbus-bootcs {
#address-cells = 1;
#size-cells = 1;
ranges = 0 MBUS_BOOTCS 0 0x;
flash {
regs = 0 0x10;
};
};
};
mbus {
ranges = ...;
devbus-bootcs {
#address-cells = 2;
#size-cells = 1;
ranges;
flash {
regs = MBUS_BOOTCS 0 0x10;
};
};
};
Let's suppose we have a board A with its armada-A.dts,
and a common one armada.dtsi.
The common dtsi file would have this ranges property:
/* armada.dtsi */
mbus {
ranges = internal_regs_id 0 internal_regs_base internal_regs_size
bootrom_id 0 bootrom_base bootrom_size
}
The A board has a NOR connected to some devbus, so we need to add it
to the ranges, but also need to duplicate the ones in the common dtsi:
/* armada-A.dts */
mbus {
ranges = internal_regs_id 0 internal_regs_base internal_regs_size
bootrom_id 0 bootrom_base bootrom_size
devbus0_id 0 devbus0_base devbus0_size
}
I think the mbus ranges property is best left only in the
board specific .dts file, since you don't know if all of the
mappings are actually set up to the same value by all boot
loaders. In order to avoid a situation where the mbus ranges
describes a setting that is not actually programmed into the
mbus controller by the boot loader, I would leave that empty
by default and only fill it when the OS actually assigns
a mapping.
Now, if we add something at the common level, and extend the ranges
property in the common armada.dtsi, we also have to go through *each* of
the per-board dts files (for *each* board) adding that entry, because
entries *need* to be duplicated. Otherwise you're effectively
shadowing the entries.
We can't really do that anyway, as that would imply we also change
all the boot loaders that have been deployed. I mentioned earlier that
we could also have the mappings we /want/ described in the DT rather the
ones that are set up, but after the discussion about the 0xd0/0xf1
window for the internal registers I think it's better to keep them
in sync all the time. We can leave out mappings here that are set
up but I'd prefer not to put mappings in there that are actually
incorrect.
When assigning the mappings, it's best to just go through all devices
sitting below the mbus and pick an appropriate address for each
'reg' value that gets put into the mbus hardware and into the ranges
property at the same time. The easiest algorithm would be to do
a 'first fit' starting at the highest address that is not already
occupied. If we need the physical address space to be more compact,
we can also first sort all the resources by size. The simpler approach
wastes at most the size difference between the smallest and the largest
range, and that is probably good enough.
I thought this was actually what you implemented already, but it seems
I was wrong.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/2] ARM: nomadik: add the new clocks to the device tree
On Sunday 09 June 2013, Linus Walleij wrote:
+ /*
+* IP AMBA bus clocks, driving the bus side of the
+* peripheral clocking, clock gates.
+*/
+
+ hclkdma0: hclkdma0@48M {
+ #clock-cells = 0;
+ compatible = st,nomadik-src-clock;
+ clock-id = 0;
+ clocks = hclk;
+ };
+ hclksmc: hclksmc@48M {
+ #clock-cells = 0;
+ compatible = st,nomadik-src-clock;
+ clock-id = 1;
+ clocks = hclk;
+ };
+ hclksdram: hclksdram@48M {
+ #clock-cells = 0;
+ compatible = st,nomadik-src-clock;
+ clock-id = 2;
+ clocks = hclk;
+ };
+ hclkdma1: hclkdma1@48M {
+ #clock-cells = 0;
+ compatible = st,nomadik-src-clock;
+ clock-id = 3;
+ clocks = hclk;
+ };
Sorry if I'm being slow to understand how the clock bindings work, but if
you have 63 identical clocks that only differ in ther clock-id, can't you
just have a single DT node for them instead with #clock-cells=1 to pass the
number from the device using it?
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 04/14] bus: mvebu-mbus: Add static window allocation to the DT binding
On Sunday 09 June 2013 11:34:24 Ezequiel Garcia wrote:
On Sun, Jun 09, 2013 at 03:42:24PM +0200, Arnd Bergmann wrote:
On Saturday 08 June 2013 15:38:52 Ezequiel Garcia wrote:
On Fri, Jun 07, 2013 at 02:00:54PM -0600, Jason Gunthorpe wrote:
Right. I think we have two options here for laying the DT ranges.
1) This is the proposal implied in the patchset I sent:
mbus {
ranges = we only put the internal-reg translation here
devbus-bootcs {
ranges = 0 {target_id/attribute} {window_physical_base}
{size}
}
}
As Jason explained, you cannot have the window_physical_base in the child
device, that just wouldn't work. I don't know if that's a typo or a thinko
I'm not sure what you mean by that just wouldn't work. I understand it
may be a crappy DT layout, but it definitely works.
I didn't mean to imply that you hadn't tested it. I guess it works fine
if you put the same window_physical_base address into both the source
and destination side mbus ranges property, but then you have to pass
it three times in total and it gets really messy when you reassign it
to a different physical address.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] ethernet/arc/arc_emac - Add new driver
On Tuesday 04 June 2013 16:21:50 Alexey Brodkin wrote:
drivers/net/ethernet/Kconfig |1 +
drivers/net/ethernet/Makefile|1 +
drivers/net/ethernet/arc/Kconfig | 29 +
drivers/net/ethernet/arc/Makefile|6 +
drivers/net/ethernet/arc/arc_emac_main.c | 905
++
drivers/net/ethernet/arc/arc_emac_main.h | 82 +++
drivers/net/ethernet/arc/arc_emac_mdio.c | 181 ++
drivers/net/ethernet/arc/arc_emac_mdio.h | 22 +
drivers/net/ethernet/arc/arc_emac_regs.h | 73 +++
I wonder if it would be better to name the directory synopsys or
designware rather than arc now. Is there a chance that the same
controller is used on non-arc CPUs?
+static int arc_emac_probe(struct platform_device *pdev)
+{
+ struct net_device *net_dev;
+ struct arc_emac_priv *priv;
+ int err;
+ unsigned int clock_frequency;
+ unsigned int id;
+ struct resource res_regs;
+#ifdef CONFIG_OF_IRQ
+ struct resource res_irq;
+#endif
+ const char *mac_addr = NULL;
Please remove the #ifdef here. The driver does not work without this
anyway, so better make it 'depend on OF_IRQ' in Kconfig.
+ /* Get phy from device tree */
+ priv-phy_node = of_parse_phandle(pdev-dev.of_node, phy, 0);
+ if (!priv-phy_node) {
+ dev_err(pdev-dev,
+ failed to retrieve phy description from device
tree\n);
+ err = -ENODEV;
+ goto out;
+ }
You should add a binding document in Documentation/devicetree/bindings that
describes what properties are required.
+ /* Get EMAC registers base address from device tree */
+ err = of_address_to_resource(pdev-dev.of_node, 0, res_regs);
+ if (err) {
+ dev_err(pdev-dev,
+ failed to retrieve base register from device tree\n);
+ err = -ENODEV;
+ goto out;
+ }
+
+ if (!devm_request_mem_region(pdev-dev, res_regs.start,
+ resource_size(res_regs), pdev-name)) {
+ dev_err(pdev-dev,
+ failed to request memory region for base registers\n);
+ err = -ENXIO;
+ goto out;
+ }
+
+ priv-reg_base_addr = (void *) devm_ioremap_nocache(pdev-dev,
+ res_regs.start, resource_size(res_regs));
+ if (!priv-reg_base_addr) {
+ dev_err(pdev-dev, failed to ioremap MAC registers\n);
+ err = -ENXIO;
+ goto out;
+ }
This block can be simplified to a single devm_ioremap_resource() now.
The cast to 'void *' is wrong: please make sure that you always use '__iomem'
pointers for MMIO mappings. You can use 'sparse' with 'make C=1' to check this
at build time.
+ /* Get MAC address from device tree */
+#ifdef CONFIG_OF_NET
+ mac_addr = of_get_mac_address(pdev-dev.of_node);
+#endif
Same as above, remove the #ifdef.
diff --git a/drivers/net/ethernet/arc/arc_emac_main.h
b/drivers/net/ethernet/arc/arc_emac_main.h
new file mode 100644
index 000..6f03d26
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_main.h
This header seems to be included only in the main .c file, just
move the contents there and remove this file.
+/**
+ * arc_mdio_probe - MDIO probe function.
+ * @dev_node:Pointer to device node.
+ * @priv:Pointer to ARC MDIO private data structure.
+ *
+ * returns: 0 on success, -ENOMEM when mdiobus_alloc
+ * (to allocate memory for MII bus structure) fails.
+ *
+ * Sets up and registers the MDIO interface.
+ */
+int arc_mdio_probe(struct device_node *dev_node, struct arc_mdio_priv *priv)
+{
+ struct device_node *mdio_np;
+ struct mii_bus *bus;
+ int error;
+
+ bus = mdiobus_alloc();
+ if (!bus) {
+ error = -ENOMEM;
+ goto cleanup;
+ }
+
+ priv-bus = bus;
+ bus-priv = priv;
+ bus-name = Synopsys MII Bus,
+ bus-read = arc_mdio_read;
+ bus-write = arc_mdio_write;
+
+ snprintf(bus-id, MII_BUS_ID_SIZE, %.8x,
+ (unsigned int)priv-reg_base_addr);
+
+ bus-parent = priv-dev;
+
+ mdio_np = of_find_node_by_name(NULL, mdio);
+ if (!mdio_np) {
+ dev_err(priv-dev, cannot find mdio in device tree\n);
+ error = -ENODEV;
+ goto cleanup;
+ }
of_find_node_by_name() is probably not what you want here, the name should
not be used as a primary key. Maybe it's better to use a standalone driver
for the phy and put it into drivers/net/phy/. I don't know what the official
policy is here though, since the phy is only used in this one driver.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V3 1/3] Add PCIe driver for Samsung Exynos
On Friday 07 June 2013 18:22:50 Jingoo Han wrote:
diff --git a/Documentation/devicetree/bindings/pci/exynos-pcie.txt
b/Documentation/devicetree/bindings/pci/exynos-pcie.txt
new file mode 100644
index 000..3eb4a2d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/exynos-pcie.txt
@@ -0,0 +1,56 @@
+* Samsung Exynos PCIe interface
+
+Required properties:
+-compatible: should be samsung,exynos5440-pcie
+-reg: base addresses and lengths of the pcie conteroller,
+ additional register for the pcie controller,
+ the phy controller,
+ additional register for the phy controller.
+- interrupts: interrupt values for level interrupt,
+ pulse interrupt, special interrupt.
+- device_type, set to pci
+- bus-range: PCI bus numbers covered
Why is it that only a subset of bus numbers are used? Can't you address
the entire range?
+- ranges: ranges for the PCI memory and I/O regions
+- reset-gpio: gpio pin number of power good signal
The 'reset-gpio' property seems incorrect. I think this should either
use the gpio binding or the reset-controller binding. Specifying
bare numbers to use as gpio pins does not work, since the number
space for Linux internal gpio numbers is not necessarily the same
as used by the hardware.
I think you also need an interrupt-map property as mandated by
the PCI binding, in order to use legacy interrupts, as well as
#address-cells and #size-cells.
+ pcie0@4000 {
+ compatible = samsung,exynos5440-pcie;
+ reg = 0x4000 0x4000
+ 0x29 0x1000
+ 0x27 0x1000
+ 0x271000 0x40;
+ interrupts = 0 20 0, 0 21 0, 0 22 0;
+ device_type = pci;
+ bus-range = 0x0 0xf;
+ ranges = 0x0800 0 0x4000 0x4000 0 0x0020
/* configuration space */
+ 0x8100 0 0 0x4020 0 0x4000
/* downstream I/O */
+ 0x8200 0 0 0x40204000 0 0x1000;
/* non-prefetchable memory */
+ };
+
+ pcie1@6000 {
+ compatible = samsung,exynos5440-pcie;
+ reg = 0x6000 0x4000
+ 0x2a 0x1000
+ 0x272000 0x1000
+ 0x271040 0x40;
+ interrupts = 0 23 0, 0 24 0, 0 25 0;
+ device_type = pci;
+ bus-range = 0x0 0xf;
+ ranges = 0x0800 0 0x6000 0x6000 0 0x0020
/* configuration space */
+ 0x8100 0 0 0x6020 0 0x4000
/* downstream I/O */
+ 0x8200 0 0 0x60204000 0 0x1000;
/* non-prefetchable memory */
+ };
Is it intentional that in this example you set up both buses to
have both memory and I/O space start at address 0 in bus space?
I think it would be more logical to have non-overlapping addresses.
You can also choose to have an identity mapping for memory
space where a PCI bus address maps directly to the physical address
used to access it, although that will prevent you from using legacy
VGA cards that require the use of the low 16 MB.
Using a 16kb I/O space rather than a 64KB I/O space per port will
lead to pci_ioremap_io() map the start of your memory space into
PCI_IO_VIRT_BASE, which you probably didn't intend.
If your hardware cannot handle a full 64KB window, I would recommend
to at least leave a hole before the start of the memory window.
+struct pcie_port {
+ struct device *dev;
+ u8 controller;
+ u8 root_bus_nr;
+ void __iomem*dbi_base;
+ void __iomem*va_dbi_base;
+ void __iomem*elbi_base;
+ void __iomem*va_elbi_base;
+ void __iomem*base;
+ void __iomem*phy_base;
+ void __iomem*va_phy_base;
+ void __iomem*purple_base;
+ void __iomem*va_purple_base;
+ void __iomem*cfg0_base;
+ void __iomem*va_cfg0_base;
+ void __iomem*cfg1_base;
+ void __iomem*va_cfg1_base;
+ void __iomem*io_base;
+ void __iomem*mem_base;
+ spinlock_t conf_lock;
+ struct resource io;
+ struct resource mem;
+ struct resource busn;
A lot of the fields above appear to be duplicated. If you
pass a physical address, that needs to be a phys_addr_t,
not void __iomem*. I think most of the physical addresses
can be removed there, and you just keep the virtual addresses
but drop the va_ prefix.
+static int add_pcie_port(struct pcie_port *pp, struct platform_device *pdev)
+{
+ struct resource *dbi_base;
+ struct resource *elbi_base;
+ struct resource *phy_base;
+
