Re: [RFC 2/5] ARM: dts: Add Cross Trigger Interface binding
On Thu, Dec 13, 2012 at 07:21:30PM +, Jon Hunter wrote: On 12/13/2012 11:41 AM, Will Deacon wrote: On Wed, Dec 12, 2012 at 09:43:05PM +, Jon Hunter wrote: Adds a device-tree binding for the ARM Cross Trigger Interface (CTI). The ARM Cross Trigger Interface provides a way to route events between processor modules. For example, on OMAP4430 we use the CTI module to route PMU events to the GIC interrupt module. Signed-off-by: Jon Hunter jon-hun...@ti.com --- Documentation/devicetree/bindings/arm/cti.txt | 32 + 1 file changed, 32 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/cti.txt diff --git a/Documentation/devicetree/bindings/arm/cti.txt b/Documentation/devicetree/bindings/arm/cti.txt new file mode 100644 index 000..4a0e2d3 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/cti.txt @@ -0,0 +1,32 @@ +* ARM Cross Trigger Interface (CTI) + +The ARM Cross Trigger Interface provides a way to route events between +processor modules. For example, debug events from one processor can be +broadcasted to other processors. The events that can be routed between +processors are specific to the device. + +Required properties: + +- compatible: Should be arm,primecell. +- interrupts: Interrupt associated with CTI module. +- reg:Contains timer register address range (base + address and length). +- arm,cti-name: A unique name for the CTI module, that will be + used when requesting the CTI module instance. + + +Optional properties: + +- arm-primecell-periphid: Primecell peripheral ID associated with CTI + module. For multi-cluster systems, I wouldn't be surprised to see multiple CTI instances, each with different CPU affinities. Can we include an affinity property following Mark's proposed binding? http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137290.html Yes I can take a look. Would something like that be applicable to pmu as well or is that unlikely to have different affinities? I am just wondering if there is something that we should implement in general for the various primecell components. Do you mean for describing the PMU's affinity to the perf subsystem or its wiring to the CTI? It's certainly applicable for the former; I've been working on a series to enable support for the PMUs in both clusters in a A15x2 A7x3 coretile using the binding, and I intend to post a series shortly. I'm not sure about the latter, as I don't have much of an understanding about the CTI. I'm not sure how many other components have affinity concerns, but the intention is for the binding to be reusable. Cheers Jon Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH 2/2] ARM: dts: OMAP2+: Add PMU nodes
On Fri, Dec 14, 2012 at 09:26:37PM +, Jon Hunter wrote: Add PMU nodes for OMAP2, OMAP3 and OMAP4460 devices. Please note that the node for OMAP4460 has been placed in a separate header file for OMAP4460, because the node is not compatible with OMAP4430. Signed-off-by: Jon Hunter jon-hun...@ti.com --- arch/arm/boot/dts/omap2.dtsi |5 + arch/arm/boot/dts/omap3.dtsi |6 ++ arch/arm/boot/dts/omap4-panda-es.dts |2 ++ arch/arm/boot/dts/omap4460.dtsi | 18 ++ 4 files changed, 31 insertions(+) create mode 100644 arch/arm/boot/dts/omap4460.dtsi diff --git a/arch/arm/boot/dts/omap2.dtsi b/arch/arm/boot/dts/omap2.dtsi index 761c4b6..27f5ea1 100644 --- a/arch/arm/boot/dts/omap2.dtsi +++ b/arch/arm/boot/dts/omap2.dtsi @@ -26,6 +26,11 @@ }; }; + pmu { + compatible = arm,arm1136-pmu; + interrupts = 3; + }; + soc { compatible = ti,omap-infra; mpu { diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi index 1acc261..6c63118 100644 --- a/arch/arm/boot/dts/omap3.dtsi +++ b/arch/arm/boot/dts/omap3.dtsi @@ -26,6 +26,12 @@ }; }; + pmu { + compatible = arm,cortex-a8-pmu; + interrupts = 3; + ti,hwmods = debugss; + }; + /* * The soc node represents the soc top level view. It is uses for IPs * that are not memory mapped in the MPU view or for the MPU itself. diff --git a/arch/arm/boot/dts/omap4-panda-es.dts b/arch/arm/boot/dts/omap4-panda-es.dts index 73bc1a6..2a6e344 100644 --- a/arch/arm/boot/dts/omap4-panda-es.dts +++ b/arch/arm/boot/dts/omap4-panda-es.dts @@ -5,7 +5,9 @@ * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. */ + /include/ omap4-panda.dts +/include/ omap4460.dtsi /* Audio routing is differnet between PandaBoard4430 and PandaBoardES */ sound { diff --git a/arch/arm/boot/dts/omap4460.dtsi b/arch/arm/boot/dts/omap4460.dtsi new file mode 100644 index 000..1270890 --- /dev/null +++ b/arch/arm/boot/dts/omap4460.dtsi @@ -0,0 +1,18 @@ +/* + * Device Tree Source for OMAP4460 SoC + * + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ + * + * This file is licensed under the terms of the GNU General Public License + * version 2. This program is licensed as is without any warranty of any + * kind, whether express or implied. + */ + +/ { + pmu { + compatible = arm,cortex-a9-pmu; + interrupts = 0 54 0x4 + 0 55 0x4; In other places I've seen interrupts properties written as: interrupts = irq1... , irq2... , irqN... ; Where each individual interrupt is surrounded by angle brackets. This produces the exact same dtb, but may appear easier to read. This might not be the right time and place to raise it, but it'd be nice if we used one style consistently. + ti,hwmods = debugss; + }; +}; -- 1.7.10.4 ___ devicetree-discuss mailing list devicetree-disc...@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND
[...] diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index 01ce462..f7de9eb 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -39,6 +39,7 @@ #include omap_device.h #include gpmc.h #include gpmc-nand.h +#include gpmc-onenand.h #define DEVICE_NAME omap-gpmc @@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct platform_device *pdev, } #endif +#ifdef CONFIG_MTD_ONENAND +static int gpmc_probe_onenand_child(struct platform_device *pdev, + struct device_node *child) +{ + u32 val; + struct omap_onenand_platform_data *gpmc_onenand_data; + + if (of_property_read_u32(child, reg, val) 0) { + dev_err(pdev-dev, %s has no 'reg' property\n, + child-full_name); + return -ENODEV; + } + + gpmc_onenand_data = devm_kzalloc(pdev-dev, sizeof(*gpmc_onenand_data), + GFP_KERNEL); + if (!gpmc_onenand_data) + return -ENOMEM; + + gpmc_onenand_data-cs = val; + gpmc_onenand_data-of_node = child; + gpmc_onenand_data-dma_channel = -1; + + if (!of_property_read_u32(child, dma-channel, val)) + gpmc_onenand_data-dma_channel = val; + + gpmc_onenand_init(gpmc_onenand_data); + + return 0; +} +#else +static int gpmc_probe_onenand_child(struct platform_device *pdev, + struct device_node *child) +{ + return 0; +} +#endif + static int gpmc_probe_dt(struct platform_device *pdev) { int ret; @@ -1276,6 +1314,12 @@ static int gpmc_probe_dt(struct platform_device *pdev) return ret; } This doesn't look right to me: + for_each_node_by_name(child, onenand) { + ret = gpmc_probe_onenand_child(pdev, child); + of_node_put(child); + if (ret 0) + return ret; + } for_each_node_by_name automatically calls of_node_put on each node once passed, and as far as I can tell, gpmc_probe_onenand_child doesn't do anything that'd increment a node's refcount. As far as I can see, you only need the of_node_put in the error case: for_each_node_by_name(child, onenand) { ret = gpmc_probe_onenand_child(pdev, child); if (ret 0) { of_node_put(child); return ret; } } Have I missed something here? return 0; } #else -- 1.7.8.6 ___ devicetree-discuss mailing list devicetree-disc...@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 5/6] usb: otg: add device tree support to otg library
+struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev, + const char *phandle, u8 index) +{ + struct usb_phy *phy = NULL, **ptr; + unsigned long flags; + struct device_node *node; + + if (!dev-of_node) { + dev_dbg(dev, device does not have a device node entry\n); + return ERR_PTR(-EINVAL); + } + + node = of_parse_phandle(dev-of_node, phandle, index); + if (!node) { + dev_dbg(dev, failed to get %s phandle in %s node\n, phandle, + dev-of_node-full_name); + return ERR_PTR(-ENODEV); + } At any point past this, node's refcount is incremented (done automatically by of_parse_phandle = of_find_node_by_phandle). + + ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) { + dev_dbg(dev, failed to allocate memory for devres\n); + return ERR_PTR(-ENOMEM); + } + + spin_lock_irqsave(phy_lock, flags); + + phy = __of_usb_find_phy(node); + if (IS_ERR(phy) || !try_module_get(phy-dev-driver-owner)) { + phy = ERR_PTR(-EPROBE_DEFER); + devres_free(ptr); + goto err0; + } + + *ptr = phy; + devres_add(dev, ptr); + + get_device(phy-dev); + +err0: + spin_unlock_irqrestore(phy_lock, flags); + + return phy; +} This means that on all of the exit paths, node's refcount is left incremented incorrectly. You'll need an of_node_put(node) on each exit path. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module
On Fri, Jan 25, 2013 at 10:23:57AM +, Kishon Vijay Abraham I wrote: Added a new driver for the usb part of control module. This has an API to power on the USB2 phy and an API to write to the mailbox depending on whether MUSB has to act in host mode or in device mode. Writing to control module registers for doing the above task which was previously done in omap glue and in omap-usb2 phy will be removed. Signed-off-by: Kishon Vijay Abraham I kis...@ti.com --- Documentation/devicetree/bindings/usb/omap-usb.txt | 30 +- Documentation/devicetree/bindings/usb/usb-phy.txt |5 + drivers/usb/phy/Kconfig| 10 + drivers/usb/phy/Makefile |1 + drivers/usb/phy/omap-control-usb.c | 295 include/linux/usb/omap_control_usb.h | 92 ++ 6 files changed, 432 insertions(+), 1 deletion(-) create mode 100644 drivers/usb/phy/omap-control-usb.c create mode 100644 include/linux/usb/omap_control_usb.h diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt b/Documentation/devicetree/bindings/usb/omap-usb.txt index 29a043e..2d8c6c4 100644 --- a/Documentation/devicetree/bindings/usb/omap-usb.txt +++ b/Documentation/devicetree/bindings/usb/omap-usb.txt @@ -1,4 +1,4 @@ -OMAP GLUE +OMAP GLUE AND OTHER OMAP SPECIFIC COMPONENTS OMAP MUSB GLUE - compatible : Should be ti,omap4-musb or ti,omap3-musb @@ -16,6 +16,10 @@ OMAP MUSB GLUE - power : Should be 50. This signifies the controller can supply upto 100mA when operating in host mode. +Optional properties: + - ctrl-module : phandle of the control module this glue uses to write to + mailbox + SOC specific device node entry usb_otg_hs: usb_otg_hs@4a0ab000 { compatible = ti,omap4-musb; @@ -23,6 +27,7 @@ usb_otg_hs: usb_otg_hs@4a0ab000 { multipoint = 1; num_eps = 16; ram_bits = 12; + ctrl-module = omap_control_usb; }; Board specific device node entry @@ -31,3 +36,26 @@ Board specific device node entry mode = 3; power = 50; }; + +OMAP CONTROL USB + +Required properties: + - compatible: Should be ti,omap-control-usb + - reg : Address and length of the register set for the device. It contains + the address of control_dev_conf and otghs_control or phy_power_usb Could you not use '-' over '_' here? + depending upon omap4 or omap5. + - reg-names: The names of the register addresses corresponding to the registers + filled in reg. + - ti,type: This is used to differentiate whether the control module has + usb mailbox or usb3 phy power. omap4 has usb mailbox in control module to + notify events to the musb core and omap5 has usb3 phy power register to + power on usb3 phy. Should be 1 if it has mailbox and 2 if it has usb3 + phy power. Why not make this a string property, perhaps values mailbox or register? That way it's easy for humans and code to verify the dts, and easy to expand arbitrarily in future if necessary. It also means you're not leaking kernel-side constants as an ABI. + +omap_control_usb: omap-control-usb@4a002300 { + compatible = ti,omap-control-usb; + reg = 0x4a002300 0x4, + 0x4a00233c 0x4; + reg-names = control_dev_conf, otghs_control; + ti,type = 1; +}; [...] +static int omap_control_usb_probe(struct platform_device *pdev) +{ + struct resource *res; + struct device_node *np = pdev-dev.of_node; + struct omap_control_usb_platform_data *pdata = pdev-dev.platform_data; + + control_usb = devm_kzalloc(pdev-dev, sizeof(*control_usb), + GFP_KERNEL); + if (!control_usb) { + dev_err(pdev-dev, unable to alloc memory for control usb\n); + return -ENOMEM; + } + + if (np) { + of_property_read_u32(np, ti,type, control_usb-type); + } else if (pdata) { + control_usb-type = pdata-type; + } else { + dev_err(pdev-dev, no pdata present\n); + return -EINVAL; + } Please do some sanity checking here on type. What if it's not OMAP_CTRL_DEV_TYPE1 or OMAP_CTRL_DEV_TYPE2? What if the values for OMAP_CTRL_DEV_TYPE{1,2} change? The binding will be broken. If you change ti,type to a string, the parsing also becomes sanity checking: if (np) { char *type; if (of_property_read_string(np, ti,type, type) != 0) return -EINVAL; if (strcmp(type, mailbox) == 0) control_usb-type = OMAP_CTRL_DEV_TYPE1; else if (strcmp(type, register) == 0) control_usb-type = OMAP_CTRL_DEV_TYPE2; else return -EINVAL; } else { ... pdata case ... } Also, TYPE1 and TYPE2 aren't very descriptive. These could probably be better named. + + control_usb-dev= pdev-dev; + + res =
Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module
[...] +OMAP CONTROL USB + +Required properties: + - compatible: Should be ti,omap-control-usb + - reg : Address and length of the register set for the device. It contains + the address of control_dev_conf and otghs_control or phy_power_usb Could you not use '-' over '_' here? that's part of omap hwmod. Aha, that makes sense (unlike my original comment, now I reread it). Sorry for the noise. + depending upon omap4 or omap5. + - reg-names: The names of the register addresses corresponding to the registers + filled in reg. + - ti,type: This is used to differentiate whether the control module has + usb mailbox or usb3 phy power. omap4 has usb mailbox in control module to + notify events to the musb core and omap5 has usb3 phy power register to + power on usb3 phy. Should be 1 if it has mailbox and 2 if it has usb3 + phy power. Why not make this a string property, perhaps values mailbox or register? NAK. Can I ask what your objection to using a string property is? As far as I can see, ti,type is only used by this driver, so there's no common convention to stick to. Using a string makes the binding easier for humans to read, and thus harder to mess up in a dts, and it decouples the binding from kernel-side constants. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] arm: omap2: gpmc: add DT bindings for OneNAND
Hi, I have a couple more comments after looking though this a bit more thoroughly. On Fri, Jan 25, 2013 at 12:23:11PM +, Ezequiel Garcia wrote: This patch adds device tree bindings for OMAP OneNAND devices. Tested on an OMAP3 3430 IGEPv2 board. Signed-off-by: Ezequiel Garcia ezequiel.gar...@free-electrons.com --- Changes from v2: * Remove unneeded of_node_put() as reported by Mark Rutland Changes from v1: * Fix typo in Documentation/devicetree/bindings/mtd/gpmc-onenand.txt .../devicetree/bindings/mtd/gpmc-onenand.txt | 43 +++ arch/arm/mach-omap2/gpmc.c | 45 2 files changed, 88 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-onenand.txt diff --git a/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt b/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt new file mode 100644 index 000..deec9da --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt @@ -0,0 +1,43 @@ +Device tree bindings for GPMC connected OneNANDs + +GPMC connected OneNAND (found on OMAP boards) are represented as child nodes of +the GPMC controller with a name of onenand. + +All timing relevant properties as well as generic gpmc child properties are +explained in a separate documents - please refer to +Documentation/devicetree/bindings/bus/ti-gpmc.txt Which tree can I find this in? + +Required properties: + + - reg: The CS line the peripheral is connected to + +Optional properties: + + - dma-channel: DMA Channel index + +For inline partiton table parsing (optional): + + - #address-cells: should be set to 1 + - #size-cells: should be set to 1 + +Example for an OMAP3430 board: + + gpmc: gpmc@6e00 { + compatible = ti,omap3430-gpmc; + ti,hwmods = gpmc; + reg = 0x6e00 0x100; + interrupts = 20; + gpmc,num-cs = 8; + gpmc,num-waitpins = 4; + #address-cells = 2; + #size-cells = 1; + + onenand@0 { + reg = 0 0 0; /* CS0, offset 0 */ + + #address-cells = 1; + #size-cells = 1; + + /* partitions go here */ + }; + }; diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index c6255f7..0636d0a 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -39,6 +39,7 @@ #include omap_device.h #include gpmc.h #include gpmc-nand.h +#include gpmc-onenand.h #define DEVICE_NAME omap-gpmc @@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct platform_device *pdev, } #endif +#ifdef CONFIG_MTD_ONENAND +static int gpmc_probe_onenand_child(struct platform_device *pdev, + struct device_node *child) +{ + u32 val; + struct omap_onenand_platform_data *gpmc_onenand_data; + + if (of_property_read_u32(child, reg, val) 0) { + dev_err(pdev-dev, %s has no 'reg' property\n, + child-full_name); + return -ENODEV; + } I don't understand the format of the reg property, but it seems odd that you only need to read one cell from it. Are the remaining address cell and size cell used anywhere? + + gpmc_onenand_data = devm_kzalloc(pdev-dev, sizeof(*gpmc_onenand_data), + GFP_KERNEL); + if (!gpmc_onenand_data) + return -ENOMEM; + + gpmc_onenand_data-cs = val; + gpmc_onenand_data-of_node = child; + gpmc_onenand_data-dma_channel = -1; + + if (!of_property_read_u32(child, dma-channel, val)) + gpmc_onenand_data-dma_channel = val; + + gpmc_onenand_init(gpmc_onenand_data); + + return 0; +} [...] Otherwise looks good. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module
On Fri, Jan 25, 2013 at 02:59:28PM +, Felipe Balbi wrote: Hi, On Fri, Jan 25, 2013 at 12:29:43PM +, Mark Rutland wrote: + depending upon omap4 or omap5. + - reg-names: The names of the register addresses corresponding to the registers + filled in reg. + - ti,type: This is used to differentiate whether the control module has + usb mailbox or usb3 phy power. omap4 has usb mailbox in control module to + notify events to the musb core and omap5 has usb3 phy power register to + power on usb3 phy. Should be 1 if it has mailbox and 2 if it has usb3 + phy power. Why not make this a string property, perhaps values mailbox or register? NAK. Can I ask what your objection to using a string property is? As far as I can see, ti,type is only used by this driver, so there's no common convention to stick to. Using a string makes the binding easier for humans to read, and thus harder to mess up in a dts, and it decouples the binding from kernel-side constants. IIRC there is some work going on to add #define-like support for DT, which would allow us to match against integers while still having meaningful symbolic representations. I was under the impression that the motivation for using the preprocessor on the DT was to allow symbolic names for device/soc-specific values like addresses, rather than what amounts to ABI values for the binding. I don't see the point in building a binding that depends on future functionality to be legible, especially as we can make it more readable, robust, and just as extensible today, with a simple change to the proposed binding. Even ignoring the above, the driver isn't doing appropriate sanity checking. If you use a string property, this sanity check is implicit in the parsing -- you've either matched a value you can handle or you haven't. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module
On Fri, Jan 25, 2013 at 04:23:46PM +, Felipe Balbi wrote: Hi, On Fri, Jan 25, 2013 at 04:14:02PM +, Mark Rutland wrote: On Fri, Jan 25, 2013 at 02:59:28PM +, Felipe Balbi wrote: Hi, On Fri, Jan 25, 2013 at 12:29:43PM +, Mark Rutland wrote: + depending upon omap4 or omap5. + - reg-names: The names of the register addresses corresponding to the registers + filled in reg. + - ti,type: This is used to differentiate whether the control module has + usb mailbox or usb3 phy power. omap4 has usb mailbox in control module to + notify events to the musb core and omap5 has usb3 phy power register to + power on usb3 phy. Should be 1 if it has mailbox and 2 if it has usb3 + phy power. Why not make this a string property, perhaps values mailbox or register? NAK. Can I ask what your objection to using a string property is? As far as I can see, ti,type is only used by this driver, so there's no common convention to stick to. Using a string makes the binding easier for humans to read, and thus harder to mess up in a dts, and it decouples the binding from kernel-side constants. IIRC there is some work going on to add #define-like support for DT, which would allow us to match against integers while still having meaningful symbolic representations. I was under the impression that the motivation for using the preprocessor on the DT was to allow symbolic names for device/soc-specific values like addresses, rather than what amounts to ABI values for the binding. I don't see the point in building a binding that depends on future functionality to be legible, especially as we can make it more readable, robust, and just as extensible today, with a simple change to the proposed binding. Even ignoring the above, the driver isn't doing appropriate sanity checking. If you use a string property, this sanity check is implicit in the parsing -- you've either matched a value you can handle or you haven't. Even IRQs use numbers (not talking about the IRQ number, but the IRQ flags), why would we depend on string comparisson ? It doesn't make sense. When describing interrupts, the flags are associated with the interrupt number, and don't really make sense on their own. devicetree does not allow you to mix strings and integers in a value, so you'd never be able to encode irq flags with strings without completely breaking away from the way ePAPR describes how to encode interrupts. By using a string property, the binding is self-describing and easy for humans to read and verify. It doesn't add a large overhead to either the driver or the dts, and is no worse (possibly better) for future extension of bindings to support new device variants while retaining backwards compatibility. See the standard status property, which is string encoded. This would still work were it an integer-encoded enumeration. Having it as a string makes the meaning clear to a reader of the dts, and it's trivial for code to deal with. I don't understand why you think encoding a more legible value in the dt does not make sense. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] arm: omap2: gpmc: add DT bindings for OneNAND
On Fri, Jan 25, 2013 at 06:11:28PM +, Ezequiel Garcia wrote: Hi Mark, First of all: thanks for reviewing. On Fri, Jan 25, 2013 at 12:56 PM, Mark Rutland mark.rutl...@arm.com wrote: Hi, I have a couple more comments after looking though this a bit more thoroughly. On Fri, Jan 25, 2013 at 12:23:11PM +, Ezequiel Garcia wrote: This patch adds device tree bindings for OMAP OneNAND devices. Tested on an OMAP3 3430 IGEPv2 board. Signed-off-by: Ezequiel Garcia ezequiel.gar...@free-electrons.com --- Changes from v2: * Remove unneeded of_node_put() as reported by Mark Rutland Changes from v1: * Fix typo in Documentation/devicetree/bindings/mtd/gpmc-onenand.txt .../devicetree/bindings/mtd/gpmc-onenand.txt | 43 +++ arch/arm/mach-omap2/gpmc.c | 45 2 files changed, 88 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-onenand.txt diff --git a/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt b/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt new file mode 100644 index 000..deec9da --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt @@ -0,0 +1,43 @@ +Device tree bindings for GPMC connected OneNANDs + +GPMC connected OneNAND (found on OMAP boards) are represented as child nodes of +the GPMC controller with a name of onenand. + +All timing relevant properties as well as generic gpmc child properties are +explained in a separate documents - please refer to +Documentation/devicetree/bindings/bus/ti-gpmc.txt Which tree can I find this in? GPMC binding was posted by Daniel Mack a while ago. Tony has recently pushed it to his master branch here: git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git Aha. Thanks, it's far easier to understand with the gpmc doc! + +Required properties: + + - reg: The CS line the peripheral is connected to + +Optional properties: + + - dma-channel: DMA Channel index + +For inline partiton table parsing (optional): + + - #address-cells: should be set to 1 + - #size-cells: should be set to 1 + +Example for an OMAP3430 board: + + gpmc: gpmc@6e00 { + compatible = ti,omap3430-gpmc; + ti,hwmods = gpmc; + reg = 0x6e00 0x100; + interrupts = 20; + gpmc,num-cs = 8; + gpmc,num-waitpins = 4; + #address-cells = 2; + #size-cells = 1; + + onenand@0 { + reg = 0 0 0; /* CS0, offset 0 */ + + #address-cells = 1; + #size-cells = 1; + + /* partitions go here */ + }; + }; diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index c6255f7..0636d0a 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -39,6 +39,7 @@ #include omap_device.h #include gpmc.h #include gpmc-nand.h +#include gpmc-onenand.h #define DEVICE_NAME omap-gpmc @@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct platform_device *pdev, } #endif +#ifdef CONFIG_MTD_ONENAND +static int gpmc_probe_onenand_child(struct platform_device *pdev, + struct device_node *child) +{ + u32 val; + struct omap_onenand_platform_data *gpmc_onenand_data; + + if (of_property_read_u32(child, reg, val) 0) { + dev_err(pdev-dev, %s has no 'reg' property\n, + child-full_name); + return -ENODEV; + } I don't understand the format of the reg property, but it seems odd that you only need to read one cell from it. Are the remaining address cell and size cell used anywhere? Okey, I'll give a shot and try to explain this myself: As you can see by Daniel's first patch [1] the reg property originally contained the chip select only, and after some discussion in that same thread, and in this one [2] It was decided to use a reg property that would also describe the base address and size of the gpmc sub-device, and use ranges for the address translation. This was reflected in Daniel's changelog when he submitted the v2 patch series [3]. [1] http://www.spinics.net/lists/arm-kernel/msg202169.html [2] http://web.archiveorange.com/archive/v/vEQ2yFI0tmpQJdigvAog [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129426.html So if I've understood correctly, the first address cell is the CS, and the second the offset within this (as the comment in the onenand@0 node hints)? If so, the code now makes sense to me :) I was having difficulty seeing where the base address of the child got translated via ranges, but I see in [3
Re: [PATCH 08/13] USB: ehci-omap: Add device tree support and binding information
On Mon, Feb 04, 2013 at 03:58:55PM +, Roger Quadros wrote: Allows the OMAP EHCI controller to be specified via device tree. Signed-off-by: Roger Quadros rog...@ti.com --- .../devicetree/bindings/usb/omap-ehci.txt | 34 ++ drivers/usb/host/ehci-omap.c | 36 +++- 2 files changed, 69 insertions(+), 1 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/omap-ehci.txt diff --git a/Documentation/devicetree/bindings/usb/omap-ehci.txt b/Documentation/devicetree/bindings/usb/omap-ehci.txt new file mode 100644 index 000..90e6e3a --- /dev/null +++ b/Documentation/devicetree/bindings/usb/omap-ehci.txt @@ -0,0 +1,34 @@ +OMAP HS USB EHCI controller + +This device is usually the child of the omap-usb-host +Documentation/devicetree/bindings/mfd/omap-usb-host.txt + +Required properties: + +- compatible: should be ti,omap-ehci +- reg: should contain one register range i.e. start and length +- interrupt-parent: phandle to the interrupt controller +- interrupts: description of the interrupt line + +Optional properties: + +- phy: list of phandles to PHY nodes. + This property is required if at least one of the ports are in + PHY mode i.e. OMAP_EHCI_PORT_MODE_PHY Any reason for not calling this phys, given it's a list? [...] Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/13] mfd: omap-usb-host: Add device tree support and binding information
Hi, I have a few comments on the binding and the way it's parsed. On Mon, Feb 04, 2013 at 03:58:56PM +, Roger Quadros wrote: Allows the OMAP HS USB host controller to be specified via device tree. CC: Samuel Ortiz sa...@linux.intel.com Signed-off-by: Roger Quadros rog...@ti.com --- .../devicetree/bindings/mfd/omap-usb-host.txt | 68 drivers/mfd/omap-usb-host.c| 83 ++-- 2 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt new file mode 100644 index 000..2196893 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt @@ -0,0 +1,68 @@ +OMAP HS USB Host + +Required properties: + +- compatible: should be ti,usbhs-host +- reg: should contain one register range i.e. start and length +- ti,hwmods: must contain usb_host_hs + +Optional properties: + +- nports: number of USB ports. Usually this is automatically detected + from the IP's revision register but can be overridden by specifying + this property. It would be nice if this were num-ports, as atmel-usb is already using that, and it's clear that it's a number of ports rather than some other meaning of 'n'. From a quick grep of binding documents, out of nTHING(s), nr-THINGs, and num-THINGs, num-THINGs seems to be the most common. It would be nice if new bindings could standardise this. + +- portN_mode: Integer specifying the port mode for port N, where N can be + from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode + in include/linux/platform_data/usb-omap.h + If the port mode is not specified, that port is treated as unused. I'm against devicetree bindings refering to Linux internals. It makes a poorly documented ABI that someone might change in future without realising the implications, and it makes it stupidly difficult to read a dts. Everything required should be specified in the binding document (or another linked binding document). It might be better to describe this with a string property that gets mapped by your dt parsing code to whatever internal representation you need. That way it's far easier for a human to verify the dts is correct, and you know by construction that the parsed value is something you can handle in the driver. It would be nicer is you used '-' rather than '_' for consistency with devicetree bindings in general. + +- single_ulpi_bypass: Must be present if the controller contains a single + ULPI bypass control bit. e.g. OMAP3 silicon = ES2.1 Again it would be nicer to have '-' rather than '_' here. It might be worth prefixing this ti,. + +Required properties if child node exists: + +- #address-cells: Must be 1 +- #size-cells: Must be 1 +- ranges: must be present + +Properties for children: + +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers. +See Documentation/devicetree/bindings/usb/omap-ehci.txt and +omap3-ohci.txt + +Example for OMAP4: + +usbhshost: usbhshost@0x4a064000 { + compatible = ti,usbhs-host; + reg = 0x4a064000 0x800; + ti,hwmods = usb_host_hs; + #address-cells = 1; + #size-cells = 1; + ranges; + + usbhsohci: ohci@0x4a064800 { + compatible = ti,omap3-ohci, usb-ohci; + reg = 0x4a064800 0x400; + interrupt-parent = gic; + interrupts = 0 76 0x4; + }; + + usbhsehci: ehci@0x4a064c00 { + compatible = ti,omap-ehci, usb-ehci; + reg = 0x4a064c00 0x400; + interrupt-parent = gic; + interrupts = 0 77 0x4; + }; +}; + +usbhshost { + port1_mode = 1; /* OMAP_EHCI_PORT_MODE_PHY */ + port2_mode = 2; /* OMAP_EHCI_PORT_MODE_TLL */ + port3_mode = 1; /* OMAP_EHCI_PORT_MODE_PHY */ With a string property, these values would be self-documenting. +}; + +usbhsehci { + phy = hsusb1_phy 0 hsusb3_phy; +}; diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c index f8ed08e..0f67856 100644 --- a/drivers/mfd/omap-usb-host.c +++ b/drivers/mfd/omap-usb-host.c @@ -1,8 +1,9 @@ /** * omap-usb-host.c - The USBHS core driver for OMAP EHCI OHCI * - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com * Author: Keshava Munegowda keshava_mgo...@ti.com + * Author: Roger Quadros rog...@ti.com * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 of @@ -27,6 +28,8 @@ #include linux/platform_device.h #include linux/platform_data/usb-omap.h #include linux/pm_runtime.h +#include linux/of.h +#include linux/of_platform.h #include omap-usb.h @@
Re: [PATCH 09/13] mfd: omap-usb-host: Add device tree support and binding information
[...] + +- single_ulpi_bypass: Must be present if the controller contains a single + ULPI bypass control bit. e.g. OMAP3 silicon = ES2.1 Again it would be nicer to have '-' rather than '_' here. It might be worth prefixing this ti,. Is prefixing with ti really required? how does it better? I thought single-ulpi-bypass sounded rather generic, but it probably is specific enough as-is. I don't know enough about USB hardware to have strong feelings either way. [...] Thanks for the in-depth review :). You're welcome. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: dts: add minimal DT support for DevKit8000
Hello, I have a couple of minor comments. On Thu, Feb 07, 2013 at 02:11:37PM +, Anil Kumar wrote: DevKit8000 is a beagle board clone from Timll, sold by armkits.com. The DevKit8000 has RS232 serial port, LCD, DVI-D, S-Video, Ethernet, SD/MMC, keyboard, camera, SPI, I2C, USB and JTAG interface. This patch adds the basic DT support for devkit8000. At this time, Information of twl4030, MMC1, I2C1, leds and there pim mux information are added. Signed-off-by: Anil Kumar anilk...@gmail.com Tested-by: Thomas Weber tho...@tomweber.eu --- -This patch is based on top of kernel 3.8-rc5. -Tested on Devkit8000. [...] diff --git a/arch/arm/boot/dts/omap3-devkit8000.dts b/arch/arm/boot/dts/omap3-devkit8000.dts new file mode 100644 index 000..9864fd7 --- /dev/null +++ b/arch/arm/boot/dts/omap3-devkit8000.dts @@ -0,0 +1,125 @@ +/* + * Anil Kumar anilk...@gmail.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +/dts-v1/; + +/include/ omap3.dtsi +/ { + model = TI OMAP3 Devkit8000; Should this not be TimLL Devkit8000 ? + compatible = ti,omap3-devkit8000, ti,omap3; timll,devkit8000 ? [...] Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 3/7] ARM: OMAP: gpmc-smsc911x: add DT dev node init function
Hi, I have a few comments on the binding and the way it's parsed. On Sat, Feb 09, 2013 at 08:44:27PM +, Javier Martinez Canillas wrote: This patch adds a helper function to parse a device node that contains all the properties needed to initialize an smsc911x device connected to an OMAP processor through OMAP's GPMC. Signed-off-by: Javier Martinez Canillas javier.marti...@collabora.co.uk --- .../devicetree/bindings/net/gpmc-smsc911x.txt | 39 arch/arm/mach-omap2/gpmc-smsc911x.c| 29 +++ arch/arm/mach-omap2/gpmc-smsc911x.h|6 +++ 3 files changed, 74 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/gpmc-smsc911x.txt diff --git a/Documentation/devicetree/bindings/net/gpmc-smsc911x.txt b/Documentation/devicetree/bindings/net/gpmc-smsc911x.txt new file mode 100644 index 000..8bb0df2 --- /dev/null +++ b/Documentation/devicetree/bindings/net/gpmc-smsc911x.txt @@ -0,0 +1,39 @@ +* GPMC connected Smart Mixed-Signal Connectivity (SMSC) LAN911x/912x Controller + +GPMC connected SMSC LAN911x/912x Controllers are represented as child nodes +of the OMAP General-Purpose Memory Controller with a name of gpmc_smsc911x. + +All timing relevant properties as well as generic gpmc child properties are +explained in a separate documents - please refer to +Documentation/devicetree/bindings/bus/ti-gpmc.txt + +Required properties: +- gpmc,cs : The chip select line the pheripheral is connected to +- gpmc,gpio_irq : The GPIO pin that is connected to the SMSC LAN IRQ line In devicetree land, we use '-' in preference of '_' in property names. So this should be gpio-irq + +Optional properties: +- gpmc,flags : SMSC LAN flags - please refer to include/linux/smsc911x.h Please don't do this. It should only be necessary to read binding documents and hardware datasheets to write a dts. Referring to Linux internals creates a flaky ABI that's only going to break when things get renamed/moved/modified. The flags variable used internally by the driver don't have to have a 1-1 correspondence with a single property in the binding. You can have boolean properties (named, but without a value) in the dt specifying each flag individually. That way the dts is easier to read, is less tied to linux internals (and every property is *fully* documented), and it's far easier to add new flags in future if necessary. +- gpmc,gpio_reset : The GPIO pin connected to the SMSC LAN reset line Preferably: gpio-reset. + +Note: Besides these properties, the gpmc_smsc911x device node could need +aditional setup such as pin/pad mux settings and voltage regulators. This +depend on how the pheripheral is wired and his board specific. + +Example (for an OMAP3 board): + +gpmc@6e00 { + compatible = ti,omap3430-gpmc; + ti,hwmods = gpmc; + reg = 0x6e00 0x100; + interrupts = 20; + gpmc,num-cs = 8; + gpmc,num-waitpins = 4; + #address-cells = 2; + #size-cells = 1; + + gpmc_smsc911x@0 { + gpmc,cs = 5; + gpmc,gpio_irq = 176; + gpmc,flags = 4; /* SMSC911X_USE_32BIT */ Here you could have a boolean property gpmc,use-32bit (or possibly gpmc,use-bits, which can either be 32 or 16). + }; +}; diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c index 5ce00ad2..59a2ee4 100644 --- a/arch/arm/mach-omap2/gpmc-smsc911x.c +++ b/arch/arm/mach-omap2/gpmc-smsc911x.c @@ -19,6 +19,7 @@ #include linux/interrupt.h #include linux/io.h #include linux/smsc911x.h +#include linux/of.h #include gpmc.h #include gpmc-smsc911x.h @@ -98,3 +99,31 @@ free1: pr_err(Could not initialize smsc911x device\n); } + +int gpmc_smsc911x_init_dt(struct device_node *node) +{ + struct omap_smsc911x_platform_data gpmc_cfg; + + if (WARN_ON(!node)) + return -ENODEV; + + if (of_property_read_u32(node, gpmc,cs, gpmc_cfg.cs)) { + pr_err(Unable to get GPMC smsc911x chip select\n); + return -EINVAL; + } + + if (of_property_read_u32(node, gpmc,gpio_irq, gpmc_cfg.gpio_irq)) { + pr_err(Unable to get GPMC smsc911x GPIO IRQ\n); + return -EINVAL; + } + + if (of_property_read_u32(node, gpmc,gpio_reset, gpmc_cfg.gpio_reset)) + gpmc_cfg.gpio_reset = -EINVAL; + + if (of_property_read_u32(node, gpmc,flags, gpmc_cfg.flags)) + gpmc_cfg.flags = 0; Is no sanity checking required for any of the above properties? To handle separate flags here, you can use something like: if (of_property_read_bool(node, gpmc,use-32bit) flags |= SMSC911X_USE_32BIT; [...] Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of
Re: [PATCH v2 01/14] usb: phy: nop: Add device tree support and binding information
Hello, On Thu, Feb 07, 2013 at 04:02:41PM +, Roger Quadros wrote: The PHY clock, clock rate, VCC regulator and RESET regulator can now be provided via device tree. Signed-off-by: Roger Quadros rog...@ti.com Acked-by: Felipe Balbi ba...@ti.com --- .../devicetree/bindings/usb/usb-nop-xceiv.txt | 34 ++ drivers/usb/otg/nop-usb-xceiv.c| 36 +++ 2 files changed, 62 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt diff --git a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt new file mode 100644 index 000..d7e2726 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt @@ -0,0 +1,34 @@ +USB NOP PHY + +Required properties: +- compatible: should be usb-nop-xceiv This might be better as linux,usb-no-xceiv, given this is a Linux-specific 'device'. Saying that, I'm not sure I understand why this device needs to be instantiated from devicetree. As I understand it from looking at the driver, it's purely a Linux implementation detail used in the case of autonomous PHYs, and not an actual piece of hardware or firmware system. I must admit to being unfamiliar with this area of hardware, have I misunderstood somethign here? Can we not have drivers for autonomous PHYs initialise/register nop-usb-xceiv? That way we don't leak Linux implementation details to the outside world, and the dt can describe purely what's actually present in the system. [...] Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/14] USB: ohci-omap3: Add device tree support and binding information
On Thu, Feb 07, 2013 at 04:02:47PM +, Roger Quadros wrote: Allows the OHCI controller found in OMAP3 and later chips to be specified via device tree. Signed-off-by: Roger Quadros rog...@ti.com Acked-by: Alan Stern st...@rowland.harvard.edu --- .../devicetree/bindings/usb/omap3-ohci.txt | 17 + drivers/usb/host/ohci-omap3.c | 19 +++ 2 files changed, 36 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/omap3-ohci.txt diff --git a/Documentation/devicetree/bindings/usb/omap3-ohci.txt b/Documentation/devicetree/bindings/usb/omap3-ohci.txt new file mode 100644 index 000..ad2ace0 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/omap3-ohci.txt @@ -0,0 +1,17 @@ +OMAP HS USB OHCI controller (OMAP3 and later) + +Required properties: + +- compatible: should be ti,ohci-omap3 +- reg: should contain one register range i.e. start and length +- interrupt-parent: phandle to the interrupt controller I'm not sure that needs to be documented as a required property. It's a standard property, and if it's defined for the parent node, you won't need it here. Otherwise, this looks fine to me. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/14] USB: ehci-omap: Add device tree support and binding information
On Thu, Feb 07, 2013 at 04:02:48PM +, Roger Quadros wrote: Allows the OMAP EHCI controller to be specified via device tree. Signed-off-by: Roger Quadros rog...@ti.com Acked-by: Alan Stern st...@rowland.harvard.edu --- .../devicetree/bindings/usb/omap-ehci.txt | 34 ++ drivers/usb/host/ehci-omap.c | 36 +++- 2 files changed, 69 insertions(+), 1 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/omap-ehci.txt diff --git a/Documentation/devicetree/bindings/usb/omap-ehci.txt b/Documentation/devicetree/bindings/usb/omap-ehci.txt new file mode 100644 index 000..b00a654 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/omap-ehci.txt @@ -0,0 +1,34 @@ +OMAP HS USB EHCI controller + +This device is usually the child of the omap-usb-host +Documentation/devicetree/bindings/mfd/omap-usb-host.txt + +Required properties: + +- compatible: should be ti,ehci-omap +- reg: should contain one register range i.e. start and length +- interrupt-parent: phandle to the interrupt controller Same comment as for ohci-omap3 here; I don't think it's necessary to document this as a required property. Otherwise, looks good. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/14] mfd: omap-usb-host: Add device tree support and binding information
On Thu, Feb 07, 2013 at 04:02:49PM +, Roger Quadros wrote: Allows the OMAP HS USB host controller to be specified via device tree. CC: Samuel Ortiz sa...@linux.intel.com Signed-off-by: Roger Quadros rog...@ti.com --- .../devicetree/bindings/mfd/omap-usb-host.txt | 80 ++ drivers/mfd/omap-usb-host.c| 160 +++- 2 files changed, 234 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt new file mode 100644 index 000..b381fa6 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt @@ -0,0 +1,80 @@ +OMAP HS USB Host + +Required properties: + +- compatible: should be ti,usbhs-host +- reg: should contain one register range i.e. start and length +- ti,hwmods: must contain usb_host_hs + +Optional properties: + +- num-ports: number of USB ports. Usually this is automatically detected + from the IP's revision register but can be overridden by specifying + this property. A maximum of 3 ports are supported at the moment. + +- portN-mode: String specifying the port mode for port N, where N can be + from 1 to 3. If the port mode is not specified, that port is treated + as unused. When specified, it must be one of the following. + ehci-phy, +ehci-tll, +ehci-hsic, +ohci-phy-6pin-datse0, +ohci-phy-6pin-dpdm, +ohci-phy-3pin-datse0, +ohci-phy-4pin-dpdm, +ohci-tll-6pin-datse0, +ohci-tll-6pin-dpdm, +ohci-tll-3pin-datse0, +ohci-tll-4pin-dpdm, +ohci-tll-2pin-datse0, +ohci-tll-2pin-dpdm, + [...] +/** + * Map 'enum usbhs_omap_port_mode' found in linux/platform_data/usb-omap.h + * to the device tree binding portN-mode found in + * 'Documentation/devicetree/bindings/mfd/omap-usb-host.txt' + */ +static const char *port_modes[] = { + [OMAP_USBHS_PORT_MODE_UNUSED] = , + [OMAP_EHCI_PORT_MODE_PHY] = ehci-phy, + [OMAP_EHCI_PORT_MODE_TLL] = ehci-tll, + [OMAP_EHCI_PORT_MODE_HSIC] = ehci-hsic, + [OMAP_OHCI_PORT_MODE_PHY_6PIN_DATSE0] = ohci-phy-6pin-datse0, + [OMAP_OHCI_PORT_MODE_PHY_6PIN_DPDM] = ohci-phy-6pin-dpdm, + [OMAP_OHCI_PORT_MODE_PHY_3PIN_DATSE0] = ohci-phy-3pin-datse0, + [OMAP_OHCI_PORT_MODE_PHY_4PIN_DPDM] = ohci-phy-4pin-dpdm, + [OMAP_OHCI_PORT_MODE_TLL_6PIN_DATSE0] = ohci-tll-6pin-datse0, + [OMAP_OHCI_PORT_MODE_TLL_6PIN_DPDM] = ohci-tll-6pin-dpdm, + [OMAP_OHCI_PORT_MODE_TLL_3PIN_DATSE0] = ohci-tll-3pin-datse0, + [OMAP_OHCI_PORT_MODE_TLL_4PIN_DPDM] = ohci-tll-4pin-dpdm, + [OMAP_OHCI_PORT_MODE_TLL_2PIN_DATSE0] = ohci-tll-2pin-datse0, + [OMAP_OHCI_PORT_MODE_TLL_2PIN_DPDM] = ohci-tll-2pin-dpdm, +}; + +/** + * omap_usbhs_get_dt_port_mode - Get the 'enum usbhs_omap_port_mode' + * from the port mode string. + * @mode: The port mode string, usually obtained from device tree. + * + * The function returns the 'enum usbhs_omap_port_mode' that matches the + * provided port mode string as per the port_modes table. + * If no match is found it returns -ENODEV + */ +static const int omap_usbhs_get_dt_port_mode(const char *mode) +{ + int i; + + for (i = 0; i ARRAY_SIZE(port_modes); i++) { + if (!strcasecmp(mode, port_modes[i])) + return i; + } Any reason for using strcasecmp rather than strcmp? The binding only specified lower case versions, and this allows people to write oHcI-PhY-6PiN-DaTsE0 with impunity. Otherwise, this looks fine to me. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/14] usb: phy: nop: Add device tree support and binding information
Hi Roger, On Mon, Feb 11, 2013 at 03:14:26PM +, Roger Quadros wrote: On 02/11/2013 01:40 PM, Mark Rutland wrote: Hello, On Thu, Feb 07, 2013 at 04:02:41PM +, Roger Quadros wrote: The PHY clock, clock rate, VCC regulator and RESET regulator can now be provided via device tree. Signed-off-by: Roger Quadros rog...@ti.com Acked-by: Felipe Balbi ba...@ti.com --- .../devicetree/bindings/usb/usb-nop-xceiv.txt | 34 ++ drivers/usb/otg/nop-usb-xceiv.c| 36 +++ 2 files changed, 62 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt diff --git a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt new file mode 100644 index 000..d7e2726 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt @@ -0,0 +1,34 @@ +USB NOP PHY + +Required properties: +- compatible: should be usb-nop-xceiv This might be better as linux,usb-no-xceiv, given this is a Linux-specific 'device'. Saying that, I'm not sure I understand why this device needs to be instantiated from devicetree. As I understand it from looking at the driver, it's purely a Linux implementation detail used in the case of autonomous PHYs, and not an actual piece of hardware or firmware system. I must admit to being unfamiliar with this area of hardware, have I misunderstood somethign here? The PHY is a physical device and may need resources like power and clock to be functional. The only reason that driver is named NOP is that many USB controllers know how to talk to the standard PHYs and don't need any interface/management software. Ok. That makes sense. Apologies for the noise. The PHY driver you are looking at most likely doesn't have the recent changes I wrote to manage the PHY clock/reset/power. i.e. patches 3, 4 and 5 in the series https://lkml.org/lkml/2013/1/28/275 Before this, the ehci-omap driver was trying to manage the PHY power and reset, which was wrong. Yes. That makes a lot more sense now. Thanks for the info! Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/14] USB: ohci-omap3: Add device tree support and binding information
+Required properties: + +- compatible: should be ti,ohci-omap3 +- reg: should contain one register range i.e. start and length +- interrupt-parent: phandle to the interrupt controller I'm not sure that needs to be documented as a required property. It's a standard property, and if it's defined for the parent node, you won't need it here. The last time I tried without 'interrupt-parent' it complained. Doesn't do it anymore. Maybe I did something wrong the last time. I'll remove it. Otherwise, this looks fine to me. Thanks. I'll add your reviewed-by tag after addressing the above comment. Sure. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 10/13] mailbox: create dbx500 mailbox driver
Hello, I have a few comments on the devicetree binding and the way it's parsed. +static const struct of_device_id dbx500_mailbox_match[] = { + { .compatible = stericsson,db8500-mailbox, + .data = (void *)db8500_mboxes, + }, + { .compatible = stericsson,db9540-mailbox, + .data = (void *)dbx540_mboxes, + }, + { /* sentinel */} +}; + The binding for the compatible strings above and property parsing below should be documented. +static int dbx500_mbox_probe(struct platform_device *pdev) +{ + const struct platform_device_id *platid = platform_get_device_id(pdev); + struct resource *mem; + int ret, i, legacy_offset = 0, upap_offset; + struct mailbox **list; + int irq; + struct dbx500_plat_data *pdata = dev_get_platdata(pdev-dev); + struct device_node *np = pdev-dev.of_node; + + if (!pdata) { + if (np) { + of_property_read_u32(np, legacy-offset, + legacy_offset); I see that legacy_offset is initialised to 0, and there's no check on the return value of of_property_read_u32. Does that mean this is an optional property? This needs to be documented. + of_property_read_u32(np, upap-offset, upap_offset); However, upap_offset isn't initialised, but there's no check on the return value. If upap-offset isn't defined in the dt, upap_offset won't have been initialised. Is there no basic sanity checking that could be done here? I assume there's a maximum offset we expect that's less than 4GB? Additionally, of_property_read_u32 takes a *u32, not *int. It would be nice to be consistent with the interface. I'm not familiar with the hardware. What's the difference between the legacy and upap cases? + list = (struct mailbox **)of_match_device( + dbx500_mailbox_match, pdev-dev)-data; + if (!list) { + /* No mailbox configuration */ + dev_err(pdev-dev, No configuration found\n); + return -ENODEV; + } + } else { + /* No mailbox configuration */ + dev_err(pdev-dev, No configuration found\n); + return -ENODEV; + } + } else { + list = (struct mailbox **)platid-driver_data; + legacy_offset = pdata-legacy_offset; + upap_offset = pdata-upap_offset; + } + + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, prcm_reg); Does this work for dt? I wasn't aware we got anything more than anonymous memory and interrupts. + mbox_base = devm_ioremap(pdev-dev, mem-start, resource_size(mem)); + if (!mbox_base) { + ret = -ENOMEM; + goto out; + } + + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, prcmu_tcdm); Same here. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 10/13] mailbox: create dbx500 mailbox driver
On Tue, Feb 12, 2013 at 08:01:05PM +, Loic PALLARDY wrote: Hi Mark, Thanks for your comments. On 02/12/2013 11:39 AM, Mark Rutland wrote: Hello, I have a few comments on the devicetree binding and the way it's parsed. +static const struct of_device_id dbx500_mailbox_match[] = { + { .compatible = stericsson,db8500-mailbox, + .data = (void *)db8500_mboxes, + }, + { .compatible = stericsson,db9540-mailbox, + .data = (void *)dbx540_mboxes, + }, + { /* sentinel */} +}; + The binding for the compatible strings above and property parsing below should be documented. Yes you're right. I will add a description in Documentation/devicetree/bindings/mailbox/... Great! [...] + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, prcm_reg); Does this work for dt? I wasn't aware we got anything more than anonymous memory and interrupts. Yes this is working with and without dt. dt declaration will be the following mailbox { compatible = stericsson,db8500-mailbox; reg = 0x80157000 0x1000, 0x801B8000 0x2000; reg-names = prcm-reg, prcmu-tcdm; interrupts = 0 47 0x4; interrupt-names = irq; legacy-offset = 0xdd4; }; Ah, I wasn't aware of reg-names. Thanks for the example. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: smp: Allow real broadcast device selection instead of always dummy
Hi Santosh, On Wed, Mar 13, 2013 at 09:28:22AM +, Santosh Shilimkar wrote: (Forgot to CC Thomas) On Wednesday 13 March 2013 02:36 PM, Santosh Shilimkar wrote: With recent arm broadcast time clean-up from Mark Rutland, the dummy broadcast device is always registered with timer subsystem. And since the rating of the dummy clock event is very high, it is preferred over a real broad-cast clock event. This is a change in behavior from past and not an intended one. So reduce the rating of the dummy clockevent so that real broadcast device is selected when available. Without this all the C states with C3STOP won't work since the broad cast notifier will take an abort. Cc: Mark Rutland mark.rutl...@arm.com Cc: Russell King li...@arm.linux.org.uk Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com --- Its a regression so hopefully can get into the 3.9-rcx. Noticed this one on A15 platform. A9 platform the issue may not be seen since the local timer check avoids dummy timer registration. Some one pointed me to a fix made by Mark which was discussed under '[BUG] ARM Architected timers appear broken in 3.9-rc1' subject. That patch seems to be more of work around since the root of the problem is incorrect dummy timer rating. Either way, both patches fix the issue. Is the problem that the dummy timer is being registered as the broadcast source, or that it is selected as a local timer in preference of real timers? If it is the former, Then I believe my patch solve the issue more generally - if you happen to register a dummy timer before other timers, it will become the broadcast source. Regardless of how temporary this is, it should never happen, and lowering the rating of the dummy won't fix this. If it is the latter, then this patch would ensure that a real timer with a rating over 100 is selected in preference to the dummy, which is certainly what we want. The proposed generic dummy timer in Stephen Boyd's local timer API removal series [1] similarly uses a low rating to ensure that real timers are selected in preference to an always-registered dummy. I note that the architected timer has a higher rating (450) than the dummy (400), so this shouldn't currently be a problem. Have I missed something? [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-March/153208.html Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: smp: Allow real broadcast device selection instead of always dummy
On Wed, Mar 13, 2013 at 11:24:01AM +, Santosh Shilimkar wrote: On Wednesday 13 March 2013 03:46 PM, Mark Rutland wrote: Hi Santosh, On Wed, Mar 13, 2013 at 09:28:22AM +, Santosh Shilimkar wrote: (Forgot to CC Thomas) On Wednesday 13 March 2013 02:36 PM, Santosh Shilimkar wrote: With recent arm broadcast time clean-up from Mark Rutland, the dummy broadcast device is always registered with timer subsystem. And since the rating of the dummy clock event is very high, it is preferred over a real broad-cast clock event. This is a change in behavior from past and not an intended one. So reduce the rating of the dummy clockevent so that real broadcast device is selected when available. Without this all the C states with C3STOP won't work since the broad cast notifier will take an abort. Cc: Mark Rutland mark.rutl...@arm.com Cc: Russell King li...@arm.linux.org.uk Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com --- Its a regression so hopefully can get into the 3.9-rcx. Noticed this one on A15 platform. A9 platform the issue may not be seen since the local timer check avoids dummy timer registration. Some one pointed me to a fix made by Mark which was discussed under '[BUG] ARM Architected timers appear broken in 3.9-rc1' subject. That patch seems to be more of work around since the root of the problem is incorrect dummy timer rating. Either way, both patches fix the issue. Is the problem that the dummy timer is being registered as the broadcast source, or that it is selected as a local timer in preference of real timers? Dummy timer is preferred over real broadcast timer. Ok. If it is the former, Then I believe my patch solve the issue more generally - if you happen to register a dummy timer before other timers, it will become the broadcast source. Regardless of how temporary this is, it should never happen, and lowering the rating of the dummy won't fix this. Well by the time we need active broadcast functionality, clock-events are already chosen if the ratings are appropriate. That's a fair point. I still think it's worth having the check in the core broadcast code - rejecting dummy timers is always a sensible thing to do, and it prevents future crashes if new timers are added without sensible rating values. If it is the latter, then this patch would ensure that a real timer with a rating over 100 is selected in preference to the dummy, which is certainly what we want. The proposed generic dummy timer in Stephen Boyd's local timer API removal series [1] similarly uses a low rating to ensure that real timers are selected in preference to an always-registered dummy. I note that the architected timer has a higher rating (450) than the dummy (400), so this shouldn't currently be a problem. Because we always register dummy broadcast on ARM now and with higher rating, it is picked as broadcast source. We definitely don't want such a behavior when we have real broadcast device is available. Agreed. We *never* want to pick a dummy timer as a broadcast source, as this never makes sense. With your patch, we are trying to avoid the registration which goes against the the whole idea of registering it always and picking the right clock-event based on rating by clock-event core. The clock-event core except the proper ratings to be provided based on the capability of the timer source and its resolution and in that case dummy should have the lowest rating which is what I tried to patch. Have I missed something? Not much. I was just looking at x86 code as well after your email to see how the LAPIC issue is handled. They seems to also have correct rating to take care of the selection. Probably we can merge both the fixes but from clock-event core code perspective, the ratings fix is more than enough. I do agree it'd be worth lowering the dummy timer's rating to ensure it doesn't override a real timer elsewhere. Thomas has already pulled my fix into tip timers/urgent [1]. [1] https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=timers/urgentid=a7dc19b8652c862d5b7c4d2339bd3c428bd29c4a Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: smp: Allow real broadcast device selection instead of always dummy
On Wed, Mar 13, 2013 at 03:44:03PM +, Santosh Shilimkar wrote: On Wednesday 13 March 2013 05:55 PM, Mark Rutland wrote: On Wed, Mar 13, 2013 at 11:24:01AM +, Santosh Shilimkar wrote: On Wednesday 13 March 2013 03:46 PM, Mark Rutland wrote: Hi Santosh, [..] Is the problem that the dummy timer is being registered as the broadcast source, or that it is selected as a local timer in preference of real timers? Dummy timer is preferred over real broadcast timer. Ok. [...] I do agree it'd be worth lowering the dummy timer's rating to ensure it doesn't override a real timer elsewhere. Yep. Can I add you acked-by tag then for $subject patch ? Would be good to get this one merged as well. Sure. Though could you reword the commit message? The patch solves the more general issue of a dummy being preferred over real hardware even outside of choosing the broadcast device. Acked-by: Mark Rutland mark.rutl...@arm.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: smp: Allow real broadcast device selection instead of always dummy
On Thu, Mar 14, 2013 at 07:45:14AM +, Santosh Shilimkar wrote: On Wednesday 13 March 2013 09:48 PM, Mark Rutland wrote: On Wed, Mar 13, 2013 at 03:44:03PM +, Santosh Shilimkar wrote: On Wednesday 13 March 2013 05:55 PM, Mark Rutland wrote: On Wed, Mar 13, 2013 at 11:24:01AM +, Santosh Shilimkar wrote: On Wednesday 13 March 2013 03:46 PM, Mark Rutland wrote: Hi Santosh, [..] Is the problem that the dummy timer is being registered as the broadcast source, or that it is selected as a local timer in preference of real timers? Dummy timer is preferred over real broadcast timer. Ok. [...] I do agree it'd be worth lowering the dummy timer's rating to ensure it doesn't override a real timer elsewhere. Yep. Can I add you acked-by tag then for $subject patch ? Would be good to get this one merged as well. Sure. Though could you reword the commit message? The patch solves the more general issue of a dummy being preferred over real hardware even outside of choosing the broadcast device. Acked-by: Mark Rutland mark.rutl...@arm.com Thanks. For record, patch is in end of the email which I plan to put into patch system. Regards, Santosh The below patch seems fine. Are you intending for this to go in as a fix for 3.9-rc*, or as a cleanup for 3.10? If you're aiming for the latter, it's going to clash with Stephen Boyd's local timer API removal [1], in which the generic dummy timer driver [2] (replacing the arm-specific dummy [3]) also has a rating of 100. It would be nice if we could reduce the possibility of a conflict. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-March/154724.html [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-March/154725.html [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-March/154726.html Thanks, Mark. From 57c501bcdc88c7ff26a5c63956be07e94a5083c5 Mon Sep 17 00:00:00 2001 From: Santosh Shilimkar santosh.shilim...@ti.com Date: Wed, 13 Mar 2013 12:33:16 +0530 Subject: [PATCH 1/2] ARM: smp: Avoid dummy clockevent being preferred over real hardware clock-event With recent arm broadcast time clean-up from Mark Rutland, the dummy broadcast device is always registered with timer subsystem. And since the rating of the dummy clock event is very high, it may be preferred over a real clock event. This is a change in behavior from past and not an intended one. So reduce the rating of the dummy clockevent so that real clockevent device is selected when available. Acked-by: Mark Rutland mark.rutl...@arm.com Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com --- arch/arm/kernel/smp.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 31644f1..79078ed 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -480,7 +480,7 @@ static void __cpuinit broadcast_timer_setup(struct clock_event_device *evt) evt-features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_DUMMY; - evt-rating = 400; + evt-rating = 100; evt-mult = 1; evt-set_mode = broadcast_timer_set_mode; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: convert arm/arm64 arch timer to use CLKSRC_OF init
Hi Rob, (adding Marc to Cc as he may have comments). On Wed, Mar 20, 2013 at 10:34:35PM +, Rob Herring wrote: From: Rob Herring rob.herr...@calxeda.com This converts arm and arm64 to use CLKSRC_OF DT based initialization for the arch timer. A new function arch_timer_arch_init is added to allow for arch specific setup. This has a side effect of enabling sched_clock on omap5 and exynos5. There should not be any reason not to use the arch timers for sched_clock. Nice! I was just about to post a (slightly updated) version of Thomas Abraham's arch_timer clocksource_of_init patch, but this seems much more comprehensive. I have some other arch_timer patches which may clash, but they could be rebased atop of this. Signed-off-by: Rob Herring rob.herr...@calxeda.com Cc: Russell King li...@arm.linux.org.uk Cc: Kukjin Kim kgene@samsung.com Cc: Tony Lindgren t...@atomide.com Cc: Simon Horman ho...@verge.net.au Cc: Magnus Damm magnus.d...@gmail.com Cc: Catalin Marinas catalin.mari...@arm.com Cc: Will Deacon will.dea...@arm.com Cc: John Stultz john.stu...@linaro.org Cc: Thomas Gleixner t...@linutronix.de Cc: linux-samsung-...@vger.kernel.org Cc: linux-omap@vger.kernel.org Cc: linux...@vger.kernel.org --- This is dependent on my CLKSRC_OF clean-up in arm-soc, my 64-bit sched_clock support series, and Arnd's default machine descriptor patch (for default clocksource_of_init call). This is only compile tested on arm. The full series (including sp804 work) is available here: git://sources.calxeda.com/kernel/linux.git arm-timers Rob [...] diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c index d0ad789..6215717 100644 --- a/arch/arm/mach-vexpress/v2m.c +++ b/arch/arm/mach-vexpress/v2m.c @@ -1,6 +1,7 @@ /* * Versatile Express V2M Motherboard Support */ +#include linux/clocksource.h #include linux/device.h #include linux/amba/bus.h #include linux/amba/mmci.h @@ -23,7 +24,6 @@ #include linux/regulator/machine.h #include linux/vexpress.h -#include asm/arch_timer.h #include asm/mach-types.h #include asm/sizes.h #include asm/mach/arch.h @@ -446,10 +446,7 @@ static void __init v2m_dt_timer_init(void) irq_of_parse_and_map(node, 0)); } - arch_timer_of_register(); - - if (arch_timer_sched_clock_init() != 0) - versatile_sched_clock_init(vexpress_get_24mhz_clock_base(), + versatile_sched_clock_init(vexpress_get_24mhz_clock_base(), 2400); } On TC2 this series leads to using the vexpress 24MHz clock as the sched clock in preference to the architected timer: Architected local timer running at 24.00MHz (virt). Switching to timer-based delay loop Registered arch_counter_get_cntvct+0x0/0x14 as sched_clock source sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 178956ms Registered versatile_read_sched_clock+0x0/0x28 as sched_clock source As they both have the same frequency, neither overrides the other, and whichever gets registered last is used as the sched_clock. As accesses to the architected timer are going to have a much lower overhead, this isn't very nice (and it could be better to use it even if it had a lower frequency). We could move the versatile_sched_clock_init call before the clocksource_of_init, but that doesn't feel like an ideal solution. We may have similar problems elsewhere. [...] diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c index b0ef18d..a551f88 100644 --- a/arch/arm64/kernel/time.c +++ b/arch/arm64/kernel/time.c @@ -32,6 +32,7 @@ #include linux/timer.h #include linux/irq.h #include linux/delay.h +#include linux/clocksource.h #include clocksource/arm_arch_timer.h @@ -77,10 +78,11 @@ void __init time_init(void) { u32 arch_timer_rate; - if (arch_timer_init()) - panic(Unable to initialise architected timer.\n); + clocksource_of_init(); arch_timer_rate = arch_timer_get_rate(); + if (!arch_timer_rate) + panic(Unable to initialise architected timer.\n); /* Cache the sched_clock multiplier to save a divide in the hot path. */ sched_clock_mult = NSEC_PER_SEC / arch_timer_rate; diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index e507ab7..d98e7e1 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -62,6 +62,7 @@ config CLKSRC_DBX500_PRCMU_SCHED_CLOCK config ARM_ARCH_TIMER bool + select CLKSRC_OF if OF config CLKSRC_METAG_GENERIC def_bool y if METAG [...] diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index d7ad425..afb70aa 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -337,24 +337,11 @@ out: return err; } -static const struct of_device_id
Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote: Add device tree bindings documentation for the TI WiLink modules. Currently only the WLAN part of the WiLink6, WiLink7 and WiLink8 modules is supported. Signed-off-by: Luciano Coelho coe...@ti.com --- I created a new directory under net to contain wireless bindings documentation. The actual implementation in the driver will follow separately. .../devicetree/bindings/net/wireless/ti-wilink.txt | 46 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/ti-wilink.txt diff --git a/Documentation/devicetree/bindings/net/wireless/ti-wilink.txt b/Documentation/devicetree/bindings/net/wireless/ti-wilink.txt new file mode 100644 index 000..d8e8bfbb --- /dev/null +++ b/Documentation/devicetree/bindings/net/wireless/ti-wilink.txt @@ -0,0 +1,46 @@ +TI WiLink Wireless Modules Device Tree Bindings +=== + +The WiLink modules provide wireless connectivity, such as WLAN, +Bluetooth, FM and NFC. + +There are several different modules available, which can be grouped by +their generation: WiLink6, WiLink7 and WiLink8. WiLink4 is not +currently supported with device tree. + +Currently, only the WLAN portion of the modules is supported with +device tree. + +Required properties: + + +- compatible: should be ti,wilink6, ti,wilink7 or ti,wilink8 +- interrupt-parent: the interrupt controller +- interrupts: out-of-band WLAN interrupt + See the interrupt controller's bindings documentation for + detailed definition. + +Optional properties: + + +- refclock: the internal WLAN reference clock frequency (required for + WiLink6 and WiLink7; not used for WiLink8). Must be one of the + following: + 0 = 19.2 MHz + 1 = 26.0 MHz + 2 = 38.4 MHz + 3 = 52.0 MHz + 4 = 38.4 MHz, XTAL + 5 = 26.0 MHz, XTAL + +- tcxoclock: the internal WLAN TCXO clock frequency (required for + WiLink7 not used for WiLink6 and WiLink8). Must be one of the + following: + 0 = 19.200 MHz + 1 = 26.000 MHz + 2 = 38.400 MHz + 3 = 52.000 MHz + 4 = 16.368 MHz + 5 = 32.736 MHz + 6 = 16.800 MHz + 7 = 33.600 MHz This looks suspiciously like what we have the common clock bindings for: refclk { compatible = fixed-clock; #clock-cells = 0; clock-frequency = 1920; } wilink { compatible = ti,wilink7; interrupt-parent = some_interrupt_controller; interrupts = 0 1 1; clocks = refclk, refclk; clock-names = refclk, txoclk; }; Could you not use them? Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote: On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote: On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote: +Optional properties: + + +- refclock: the internal WLAN reference clock frequency (required for + WiLink6 and WiLink7; not used for WiLink8). Must be one of the + following: + 0 = 19.2 MHz + 1 = 26.0 MHz + 2 = 38.4 MHz + 3 = 52.0 MHz + 4 = 38.4 MHz, XTAL + 5 = 26.0 MHz, XTAL + +- tcxoclock: the internal WLAN TCXO clock frequency (required for + WiLink7 not used for WiLink6 and WiLink8). Must be one of the + following: + 0 = 19.200 MHz + 1 = 26.000 MHz + 2 = 38.400 MHz + 3 = 52.000 MHz + 4 = 16.368 MHz + 5 = 32.736 MHz + 6 = 16.800 MHz + 7 = 33.600 MHz This looks suspiciously like what we have the common clock bindings for: refclk { compatible = fixed-clock; #clock-cells = 0; clock-frequency = 1920; } wilink { compatible = ti,wilink7; interrupt-parent = some_interrupt_controller; interrupts = 0 1 1; clocks = refclk, refclk; clock-names = refclk, txoclk; }; Could you not use them? Hmmm... this actually does look good. But these are internal clocks in the modules, they cannot be accessed from outside. Does it make sense to register them with the clock framework? Given we already have a common way of describing clocks, I think it makes sense to use it -- people already understand the common bindings, and it's less code to add add to the kernel. I don't think the fact these clocks are internal should prevent us from describing them as we would an external clock. Perhaps Mike Turquette [Cc'd] has an opinion on the matter. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
[resending with the correct address for Mike Turquette] On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote: On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote: On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote: +Optional properties: + + +- refclock: the internal WLAN reference clock frequency (required for + WiLink6 and WiLink7; not used for WiLink8). Must be one of the + following: + 0 = 19.2 MHz + 1 = 26.0 MHz + 2 = 38.4 MHz + 3 = 52.0 MHz + 4 = 38.4 MHz, XTAL + 5 = 26.0 MHz, XTAL + +- tcxoclock: the internal WLAN TCXO clock frequency (required for + WiLink7 not used for WiLink6 and WiLink8). Must be one of the + following: + 0 = 19.200 MHz + 1 = 26.000 MHz + 2 = 38.400 MHz + 3 = 52.000 MHz + 4 = 16.368 MHz + 5 = 32.736 MHz + 6 = 16.800 MHz + 7 = 33.600 MHz This looks suspiciously like what we have the common clock bindings for: refclk { compatible = fixed-clock; #clock-cells = 0; clock-frequency = 1920; } wilink { compatible = ti,wilink7; interrupt-parent = some_interrupt_controller; interrupts = 0 1 1; clocks = refclk, refclk; clock-names = refclk, txoclk; }; Could you not use them? Hmmm... this actually does look good. But these are internal clocks in the modules, they cannot be accessed from outside. Does it make sense to register them with the clock framework? Given we already have a common way of describing clocks, I think it makes sense to use it -- people already understand the common bindings, and it's less code to add add to the kernel. I don't think the fact these clocks are internal should prevent us from describing them as we would an external clock. Perhaps Mike Turquette [Cc'd] has an opinion on the matter. Thanks, Mark. ___ devicetree-discuss mailing list devicetree-disc...@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: dt: bindings: TI WiLink modules
[resending again with the doubly corrected address for Mike Turquette, apologies for the spam] On Fri, Jun 28, 2013 at 10:53:35AM +0100, Luciano Coelho wrote: On Fri, 2013-06-28 at 10:38 +0100, Mark Rutland wrote: On Tue, Jun 25, 2013 at 09:35:30AM +0100, Luciano Coelho wrote: +Optional properties: + + +- refclock: the internal WLAN reference clock frequency (required for + WiLink6 and WiLink7; not used for WiLink8). Must be one of the + following: + 0 = 19.2 MHz + 1 = 26.0 MHz + 2 = 38.4 MHz + 3 = 52.0 MHz + 4 = 38.4 MHz, XTAL + 5 = 26.0 MHz, XTAL + +- tcxoclock: the internal WLAN TCXO clock frequency (required for + WiLink7 not used for WiLink6 and WiLink8). Must be one of the + following: + 0 = 19.200 MHz + 1 = 26.000 MHz + 2 = 38.400 MHz + 3 = 52.000 MHz + 4 = 16.368 MHz + 5 = 32.736 MHz + 6 = 16.800 MHz + 7 = 33.600 MHz This looks suspiciously like what we have the common clock bindings for: refclk { compatible = fixed-clock; #clock-cells = 0; clock-frequency = 1920; } wilink { compatible = ti,wilink7; interrupt-parent = some_interrupt_controller; interrupts = 0 1 1; clocks = refclk, refclk; clock-names = refclk, txoclk; }; Could you not use them? Hmmm... this actually does look good. But these are internal clocks in the modules, they cannot be accessed from outside. Does it make sense to register them with the clock framework? Given we already have a common way of describing clocks, I think it makes sense to use it -- people already understand the common bindings, and it's less code to add add to the kernel. I don't think the fact these clocks are internal should prevent us from describing them as we would an external clock. Perhaps Mike Turquette [Cc'd] has an opinion on the matter. Thanks, Mark. ___ devicetree-discuss mailing list devicetree-disc...@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss ___ devicetree-discuss mailing list devicetree-disc...@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch V2 4/4] ARM: dts: AM33XX: update rtc node compatibility
On Tue, Jul 30, 2013 at 06:05:52AM +0100, Gururaja Hebbar wrote: Hi, On 7/3/2013 2:17 PM, Hebbar Gururaja wrote: Since AM33xx RTC IP has RTC_IRQWAKEEN to support Alarm Wake-up. Update the rtc compatible property to ti,am3352-rtc to enable handling of this feature inside rtc-omap driver. The other 2 rtc driver related patches have been pulled up. If you have no comments, can you please pull this up. Regards Gururaja Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com --- :100644 100644 77aa1b0... dde180a... M arch/arm/boot/dts/am33xx.dtsi arch/arm/boot/dts/am33xx.dtsi |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 77aa1b0..dde180a 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -297,7 +297,7 @@ }; rtc@44e3e000 { - compatible = ti,da830-rtc; + compatible = ti,am3352-rtc; Given that this is backwards compatible with the old binding, it would be nicer to have this as: compatible = ti,am3352-rtc, ti,da830-rtc; We must get into the habit of changing dts files in a backwards-compatible fashion. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch V2 4/4] ARM: dts: AM33XX: update rtc node compatibility
On Tue, Jul 30, 2013 at 05:21:14PM +0100, Sekhar Nori wrote: On 7/30/2013 8:25 PM, Mark Rutland wrote: On Tue, Jul 30, 2013 at 06:05:52AM +0100, Gururaja Hebbar wrote: Hi, On 7/3/2013 2:17 PM, Hebbar Gururaja wrote: Since AM33xx RTC IP has RTC_IRQWAKEEN to support Alarm Wake-up. Update the rtc compatible property to ti,am3352-rtc to enable handling of this feature inside rtc-omap driver. The other 2 rtc driver related patches have been pulled up. If you have no comments, can you please pull this up. Regards Gururaja Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com --- :100644 100644 77aa1b0... dde180a... March/arm/boot/dts/am33xx.dtsi arch/arm/boot/dts/am33xx.dtsi |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 77aa1b0..dde180a 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -297,7 +297,7 @@ }; rtc@44e3e000 { - compatible = ti,da830-rtc; + compatible = ti,am3352-rtc; Given that this is backwards compatible with the old binding, it would be nicer to have this as: compatible = ti,am3352-rtc, ti,da830-rtc; We must get into the habit of changing dts files in a backwards-compatible fashion. Right, I suggested this when v1 was posted. It turns out the current kernel does not handle the compatilble list correctly and the string selected actually depends on the order in which it appears in match table in driver instead. I saw there were patches being discussed to fix this issue, but until that is fixed, we cannot really use what you (and I before) suggested. A temporary solution would be to list the ti,am3352-rtc first in the of_match_table kernel-side. That way you can keep the old compatible string in the dts compatible list, and maintain backwards compatibilty with older kernels. Later if the way Linux matches compatible strings is changed, this shouldn't break the probe order, and the of_match_table order could be changed. Have I missed something? Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch V2 4/4] ARM: dts: AM33XX: update rtc node compatibility
On Fri, Aug 02, 2013 at 12:07:36PM +0100, Gururaja Hebbar wrote: On 8/1/2013 10:35 PM, Mark Rutland wrote: On Tue, Jul 30, 2013 at 05:21:14PM +0100, Sekhar Nori wrote: On 7/30/2013 8:25 PM, Mark Rutland wrote: On Tue, Jul 30, 2013 at 06:05:52AM +0100, Gururaja Hebbar wrote: Hi, On 7/3/2013 2:17 PM, Hebbar Gururaja wrote: Since AM33xx RTC IP has RTC_IRQWAKEEN to support Alarm Wake-up. Update the rtc compatible property to ti,am3352-rtc to enable handling of this feature inside rtc-omap driver. The other 2 rtc driver related patches have been pulled up. If you have no comments, can you please pull this up. Regards Gururaja Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com --- :100644 100644 77aa1b0... dde180a... M arch/arm/boot/dts/am33xx.dtsi arch/arm/boot/dts/am33xx.dtsi |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 77aa1b0..dde180a 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -297,7 +297,7 @@ }; rtc@44e3e000 { - compatible = ti,da830-rtc; + compatible = ti,am3352-rtc; Given that this is backwards compatible with the old binding, it would be nicer to have this as: compatible = ti,am3352-rtc, ti,da830-rtc; We must get into the habit of changing dts files in a backwards-compatible fashion. Right, I suggested this when v1 was posted. It turns out the current kernel does not handle the compatilble list correctly and the string selected actually depends on the order in which it appears in match table in driver instead. I saw there were patches being discussed to fix this issue, but until that is fixed, we cannot really use what you (and I before) suggested. A temporary solution would be to list the ti,am3352-rtc first in the of_match_table kernel-side. If above method is followed, then it would cause trouble on davinci platform because this rtc-omap driver is also used by Davinci platform. On davinci Plaform the driver would match with ti,am3352-rtc for compatible. Sorry, I don't follow. Does the davinci dt have ti,am3352-rtc in it's compatible string list? If so, and it's not compatible, the dts is wrong. If not, then we won't use the behaviour specific to ti,am3352-rtc, and there's no problem. What trouble would this cause? Thanks, Mark. Regards Gururaja That way you can keep the old compatible string in the dts compatible list, and maintain backwards compatibilty with older kernels. Later if the way Linux matches compatible strings is changed, this shouldn't break the probe order, and the of_match_table order could be changed. Have I missed something? Thanks, Mark. ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch V2 4/4] ARM: dts: AM33XX: update rtc node compatibility
On Fri, Aug 02, 2013 at 12:48:07PM +0100, Gururaja Hebbar wrote: On 8/2/2013 4:50 PM, Mark Rutland wrote: On Fri, Aug 02, 2013 at 12:07:36PM +0100, Gururaja Hebbar wrote: On 8/1/2013 10:35 PM, Mark Rutland wrote: On Tue, Jul 30, 2013 at 05:21:14PM +0100, Sekhar Nori wrote: On 7/30/2013 8:25 PM, Mark Rutland wrote: On Tue, Jul 30, 2013 at 06:05:52AM +0100, Gururaja Hebbar wrote: Hi, On 7/3/2013 2:17 PM, Hebbar Gururaja wrote: Since AM33xx RTC IP has RTC_IRQWAKEEN to support Alarm Wake-up. Update the rtc compatible property to ti,am3352-rtc to enable handling of this feature inside rtc-omap driver. The other 2 rtc driver related patches have been pulled up. If you have no comments, can you please pull this up. Regards Gururaja Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com --- :100644 100644 77aa1b0... dde180a... M arch/arm/boot/dts/am33xx.dtsi arch/arm/boot/dts/am33xx.dtsi |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 77aa1b0..dde180a 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -297,7 +297,7 @@ }; rtc@44e3e000 { - compatible = ti,da830-rtc; + compatible = ti,am3352-rtc; Given that this is backwards compatible with the old binding, it would be nicer to have this as: compatible = ti,am3352-rtc, ti,da830-rtc; We must get into the habit of changing dts files in a backwards-compatible fashion. Right, I suggested this when v1 was posted. It turns out the current kernel does not handle the compatilble list correctly and the string selected actually depends on the order in which it appears in match table in driver instead. I saw there were patches being discussed to fix this issue, but until that is fixed, we cannot really use what you (and I before) suggested. A temporary solution would be to list the ti,am3352-rtc first in the of_match_table kernel-side. If above method is followed, then it would cause trouble on davinci platform because this rtc-omap driver is also used by Davinci platform. On davinci Plaform the driver would match with ti,am3352-rtc for compatible. Sorry, I don't follow. Does the davinci dt have ti,am3352-rtc in it's compatible string list? no no. Davinci .dts uses ti,da830-rtc. Ok. I was saying if we reverse the order of of_device_id structure in rtc-omap driver, then it would work nicely for am335x but will cause trouble on davinci platform. I don't understand. If we place ti,am3352-rtc first in the list, it won't be match the davinci dts compatible string, and the code will go over the of_device_id entires until it finds the ti,da830-rtc entry used by davinci. What problem would this cause, and why? Thanks, Mark. If so, and it's not compatible, the dts is wrong. If not, then we won't use the behaviour specific to ti,am3352-rtc, and there's no problem. What trouble would this cause? Thanks, Mark. Regards Gururaja That way you can keep the old compatible string in the dts compatible list, and maintain backwards compatibilty with older kernels. Later if the way Linux matches compatible strings is changed, this shouldn't break the probe order, and the of_match_table order could be changed. Have I missed something? Thanks, Mark. ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/2] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DWC3 core
On Tue, Aug 06, 2013 at 12:53:10PM +0100, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com --- .../devicetree/bindings/usb/msm-ssusb.txt | 49 +++ drivers/usb/phy/Kconfig| 11 + drivers/usb/phy/Makefile |2 + drivers/usb/phy/phy-msm-dwc3-usb2.c| 342 + drivers/usb/phy/phy-msm-dwc3-usb3.c| 389 5 files changed, 793 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb2.c create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb3.c diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt new file mode 100644 index 000..550b496 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt @@ -0,0 +1,49 @@ +MSM SuperSpeed USB3.0 SoC controllers + +Required properities : +- compatible sould be qcom,dwc3-usb2; +- reg : offset and length of the register set in the memory map +- clocks: cxo, usb2a_phy_sleep_cxc; Huh? That doesn't describe what these are. These would be better explained with a reference to clock-names and a basic description as to what the input's called, what it drives, etc, as you've done done for the *-supply properties. +- clock-names: xo, sleep_a_clk; +supply-name-supply: phandle to the regulator device tree node +Required supply-name examples are: + v1p8 : 1.8v supply for HSPHY + v3p3 : 3.3v supply for HSPHY + vbus : vbus supply for host mode + vddcx : vdd supply for HS-PHY digital circuit operation + +Required properities : +- compatible sould be qcom,dwc3-usb3; +- reg : offset and length of the register set in the memory map +- clocks: cxo, usb30_mock_utmi_cxc; Similarly, this doesn't describe what the clocks are. +- clock-names: xo, ref_clk; +supply-name-supply: phandle to the regulator device tree node +Required supply-name examples are: + v1p8 : 1.8v supply for SS-PHY + vddcx : vdd supply for SS-PHY digital circuit operation + +Example device nodes: + + dwc3_usb2: phy@f92f8800 { + compatible = qcom,dwc3-usb2; + reg = 0xf92f8800 0x30; + + clocks = cxo, usb2a_phy_sleep_cxc; + clock-names = xo, sleep_a_clk; + + vbus-supply = supply; + vddcx-supply = supply; + v1p8-supply = supply; + v3p3-supply = supply; + }; + + dwc3_usb3: phy@f92f8830 { + compatible = qcom,dwc3-usb3; + reg = 0xf92f8830 0x30; + + clocks = cxo, usb30_mock_utmi_cxc; + clock-names = xo, ref_clk; + + vddcx-supply = supply; + v1p8-supply = supply; + }; Those regster banks look suspiciously close. Are these the same IP block? Can they ever appear separately? Do the drivers not trample each other when messing with shared clocks and regulators? Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 2/2] usb: dwc3: Add Qualcomm DWC3 glue layer driver
On Tue, Aug 06, 2013 at 12:53:11PM +0100, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com What does the glue layer do? Is it an actual piece of hardware, or just some platform-specific code? Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com --- .../devicetree/bindings/usb/msm-ssusb.txt | 39 + drivers/usb/dwc3/Kconfig |8 + drivers/usb/dwc3/Makefile |1 + drivers/usb/dwc3/dwc3-msm.c| 175 4 files changed, 223 insertions(+) create mode 100644 drivers/usb/dwc3/dwc3-msm.c diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt index 550b496..313ae0d 100644 --- a/Documentation/devicetree/bindings/usb/msm-ssusb.txt +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt @@ -22,6 +22,23 @@ Required supply-name examples are: v1p8 : 1.8v supply for SS-PHY vddcx : vdd supply for SS-PHY digital circuit operation +Required properties : +- compatible : should be qcom,dwc-usb3-msm +- reg : offset and length of the register set in the memory map + offset and length of the TCSR register for routing USB + signals to either picoPHY0 or picoPHY1. +- clocks = usb30_master_cxc, sys_noc_usb3_axi_cxc, usb30_sleep_cxc, usb30_mock_utmi_cxc; Similarly to my comment on patch 1, these need to be described better. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: dts: am33xx: Correct gpio #interrupt-cells property
On Wed, Aug 07, 2013 at 12:06:32PM +0100, Lars Poeschel wrote: From: Lars Poeschel poesc...@lemonage.de Following commit ff5c9059 and therefore other omap platforms using the gpio-omap driver correct the #interrupt-cells property on am33xx too. The omap gpio binding documentaion also states that the #interrupt-cells property should be 2. I take it there are no device nodes for which any of these nodes are an interrupt parent (which would need to be updated)? If so: Acked-by: Mark Rutland mark.rutl...@arm.com Thanks, Mark. Signed-off-by: Lars Poeschel poesc...@lemonage.de --- arch/arm/boot/dts/am33xx.dtsi |8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 38b446b..033c5dd 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -102,7 +102,7 @@ gpio-controller; #gpio-cells = 2; interrupt-controller; - #interrupt-cells = 1; + #interrupt-cells = 2; reg = 0x44e07000 0x1000; interrupts = 96; }; @@ -113,7 +113,7 @@ gpio-controller; #gpio-cells = 2; interrupt-controller; - #interrupt-cells = 1; + #interrupt-cells = 2; reg = 0x4804c000 0x1000; interrupts = 98; }; @@ -124,7 +124,7 @@ gpio-controller; #gpio-cells = 2; interrupt-controller; - #interrupt-cells = 1; + #interrupt-cells = 2; reg = 0x481ac000 0x1000; interrupts = 32; }; @@ -135,7 +135,7 @@ gpio-controller; #gpio-cells = 2; interrupt-controller; - #interrupt-cells = 1; + #interrupt-cells = 2; reg = 0x481ae000 0x1000; interrupts = 62; }; -- 1.7.10.4 ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/2] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DWC3 core
On Tue, Aug 06, 2013 at 03:36:33PM +0100, Ivan T. Ivanov wrote: Hi, On Tue, 2013-08-06 at 15:03 +0100, Mark Rutland wrote: On Tue, Aug 06, 2013 at 12:53:10PM +0100, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com --- .../devicetree/bindings/usb/msm-ssusb.txt | 49 +++ drivers/usb/phy/Kconfig| 11 + drivers/usb/phy/Makefile |2 + drivers/usb/phy/phy-msm-dwc3-usb2.c| 342 + drivers/usb/phy/phy-msm-dwc3-usb3.c| 389 5 files changed, 793 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb2.c create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb3.c diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt new file mode 100644 index 000..550b496 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt @@ -0,0 +1,49 @@ +MSM SuperSpeed USB3.0 SoC controllers + +Required properities : +- compatible sould be qcom,dwc3-usb2; +- reg : offset and length of the register set in the memory map +- clocks: cxo, usb2a_phy_sleep_cxc; Huh? That doesn't describe what these are. These would be better explained with a reference to clock-names and a basic description as to what the input's called, what it drives, etc, as you've done done for the *-supply properties. Ok, I will fix this. +- clock-names: xo, sleep_a_clk; +supply-name-supply: phandle to the regulator device tree node +Required supply-name examples are: + v1p8 : 1.8v supply for HSPHY + v3p3 : 3.3v supply for HSPHY + vbus : vbus supply for host mode + vddcx : vdd supply for HS-PHY digital circuit operation + +Required properities : +- compatible sould be qcom,dwc3-usb3; +- reg : offset and length of the register set in the memory map +- clocks: cxo, usb30_mock_utmi_cxc; Similarly, this doesn't describe what the clocks are. Understood. +- clock-names: xo, ref_clk; +supply-name-supply: phandle to the regulator device tree node +Required supply-name examples are: + v1p8 : 1.8v supply for SS-PHY + vddcx : vdd supply for SS-PHY digital circuit operation + +Example device nodes: + + dwc3_usb2: phy@f92f8800 { + compatible = qcom,dwc3-usb2; + reg = 0xf92f8800 0x30; + + clocks = cxo, usb2a_phy_sleep_cxc; + clock-names = xo, sleep_a_clk; + + vbus-supply = supply; + vddcx-supply = supply; + v1p8-supply = supply; + v3p3-supply = supply; + }; + + dwc3_usb3: phy@f92f8830 { + compatible = qcom,dwc3-usb3; + reg = 0xf92f8830 0x30; + + clocks = cxo, usb30_mock_utmi_cxc; + clock-names = xo, ref_clk; + + vddcx-supply = supply; + v1p8-supply = supply; + }; Those regster banks look suspiciously close. Are these the same IP block? Can they ever appear separately? They are part of the wrapper Qualcomm logic around Synopsys USB3 core. In this sense they are part of the one IP, I believe. Manage them from separate drivers simplify code. Hmmm. I'm not entirely certain on this. On the one hand, they're separate IP blocks, and have lgoically separate drivers, so describing them as two devices makes sense. On the other hand, they've been fused into one IP block with shared resources. Describing them as two devices probably makes sense given you have the wrapper driver. Do the drivers not trample each other when messing with shared clocks and regulators? Regulators and clocks have reference counting, right?, so this should be safe. Even if they are part of the one driver, clocks and regulators could be switched off only if both PHY's do not use them. Ok, I just wanted to be sure this had been considered :) Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] crypto: omap-sham: Add OMAP5/AM43XX SHAM Support
On Fri, Jul 26, 2013 at 07:59:15AM +0100, Lokesh Vutla wrote: Add support for the OMAP5 version of the SHAM module that is present on OMAP5 and AM43xx SoCs. This module is very simialar to OMAP4 version of SHAM module, and adds SHA384 SHA512 hardware-accelerated hash functions to it. To handle the higher digest size of SHA512, few SHA512_DIGEST_i (i=1-16, and first 8 registers are duplicated from SHA_DIGEST_i registers) registers are added at the end of register set. So adding the above register offsets and module info in pdata. Signed-off-by: Lokesh Vutla lokeshvu...@ti.com --- drivers/crypto/omap-sham.c | 44 1 file changed, 44 insertions(+) + { + .compatible = ti,omap5-sham, + .data = omap_sham_pdata_omap5, + }, No binding update? Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] ARM: dts: AM4372: cpu(s) node per latest binding
On Fri, Aug 02, 2013 at 02:46:13PM +0100, Afzal Mohammed wrote: Update AM4372 cpu node to the latest cpus/cpu bindings for ARM. Signed-off-by: Afzal Mohammed af...@ti.com --- arch/arm/boot/dts/am4372.dtsi |4 1 file changed, 4 insertions(+) diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi index ddc1df7..4635e7f 100644 --- a/arch/arm/boot/dts/am4372.dtsi +++ b/arch/arm/boot/dts/am4372.dtsi @@ -22,8 +22,12 @@ }; cpus { + #address-cells = 1; + #size-cells = 0; cpu@0 { compatible = arm,cortex-a9; + device_type = cpu; + reg = 0; }; }; This looks sane to me: Acked-by: Mark Rutland mark.rutl...@arm.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: dts: AM4372: add few nodes
On Mon, Aug 05, 2013 at 06:08:45AM +0100, Afzal Mohammed wrote: Hi Muguthan, On Saturday 03 August 2013 05:19 PM, Mugunthan V N wrote: On 8/2/2013 7:16 PM, Afzal Mohammed wrote: + mac: ethernet@4a10 { + compatible = ti,am4372-cpsw,ti,cpsw; compatibility ti,am4372-cpsw is not needed as driver has only ti,cpsw compatibility No, please read device tree documentation [1]. DT is a pure hardware description, it does not depend on driver, dependency is only vice versa. One point worth mentioning is that the ti,am4372-cpsw string isn't documented. Will the ti,am4372-cpsw binding definitely be a superset of the ti,cpsw binding, and if you were to take the DT as of this patch, and attempt to use it with a future kernel, can you guarantee it'll work? + reg = 0x4a10 0x800 + 0x4a101200 0x100; + interrupts = GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH +GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH +GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH +GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH; + ti,hwmods = cpgmac0; + status = disabled; + }; There are many other parameters which are missed here. Reason has been mentioned in the commit message, quoting relevant here again, Actually, as you've marked the nodes disabled, it's probably fine. But worth considering as properties are added... Thanks, Mark. For i2c, spi, cpsw pwm - only the properties that were sure to be correct has been added (main intention is to make hwmod happy and avoid any later modification to here added properties). I really wanted to avoid a later patch that has a line starting with minus on DTS. Since you are working on cpsw support, can you help here with a patch for other properties. Regards Afzal [1] http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/8] ARM: DRA7: dts: Add the dts files for dra7 SoC and dra7-evm board
On Tue, Jul 30, 2013 at 12:25:46PM +0100, Rajendra Nayak wrote: From: R Sricharan r.sricha...@ti.com Add minimal device tree source needed for DRA7 based SoCs. Also add a board dts file for the dra7-evm (based on dra752) which contains 1.5G of memory with 1G interleaved and 512MB non-interleaved. Also added in the board file are pin configuration details for i2c, mcspi and uart devices on board. Signed-off-by: R Sricharan r.sricha...@ti.com Signed-off-by: Rajendra Nayak rna...@ti.com Signed-off-by: Sourav Poddar sourav.pod...@ti.com --- arch/arm/boot/dts/Makefile |3 +- arch/arm/boot/dts/dra7-evm.dts | 163 ++ arch/arm/boot/dts/dra7.dtsi| 488 3 files changed, 653 insertions(+), 1 deletion(-) create mode 100644 arch/arm/boot/dts/dra7-evm.dts create mode 100644 arch/arm/boot/dts/dra7.dtsi [...] diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi new file mode 100644 index 000..8a0c08e --- /dev/null +++ b/arch/arm/boot/dts/dra7.dtsi @@ -0,0 +1,488 @@ +/* + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * Based on omap4.dtsi + */ + +/include/ skeleton.dtsi + +/ { + compatible = ti,dra7xx; + interrupt-parent = gic; + + aliases { + serial0 = uart1; + serial1 = uart2; + serial2 = uart3; + serial3 = uart4; + serial4 = uart5; + serial5 = uart6; + }; + + cpus { + cpu@0 { + compatible = arm,cortex-a15; + timer { + compatible = arm,armv7-timer; + /* +* PPI secure/nonsecure IRQ, +* active low level-sensitive +*/ + interrupts = 1 13 0x308, +1 14 0x308; + clock-frequency = 6144000; + }; + }; The cpu nodes should have a reg matching their unit-address, and a device_type = cpu. The timer nodes should *not* be under the CPU nodes. They should be under under the root node. I realise that it makes intuitive sense to describe per-cpu resources this way, but that's not the way the bindings are intended to be used (does thei DT even work?). No virtual/hypervisor interrupts? Do you really need the clock-frequency property? It's far preferrable to have your bootloader do the right thing and program CNTFRQ with the correct value. + + gic: interrupt-controller@48211000 { + compatible = arm,cortex-a15-gic; + interrupt-controller; + #interrupt-cells = 3; + reg = 0x48211000 0x1000, + 0x48212000 0x1000; Similarly, no GICH/GICV registers? Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 8/8] ARM: DRA7: dts: Add the dts files for dra7 SoC and dra7-evm board
[Adding Marc to Cc] On Tue, Aug 13, 2013 at 08:24:31AM +0100, Rajendra Nayak wrote: [].. + + cpus { + cpu@0 { + compatible = arm,cortex-a15; + timer { + compatible = arm,armv7-timer; + /* +* PPI secure/nonsecure IRQ, +* active low level-sensitive +*/ + interrupts = 1 13 0x308, +1 14 0x308; + clock-frequency = 6144000; + }; + }; The cpu nodes should have a reg matching their unit-address, and a device_type = cpu. The timer nodes should *not* be under the CPU nodes. They should be under under the root node. I realise that it makes intuitive sense to describe per-cpu resources this way, but that's not the way the bindings are intended to be used (does thei DT even work?). No virtual/hypervisor interrupts? Mark, all valid points. I just updated the patch to include all the missing interrupts and registers for timer and gic and moved the timer node out as its supposed to be. Great! Do you really need the clock-frequency property? It's far preferrable to have your bootloader do the right thing and program CNTFRQ with the correct value. I kept the clock-frequency property since our bootloader does not handle this and I am not sure if its a good idea to have the dependency on bootloader to do this. There is precedent for handling it this way, but it would be far nicer to fix the bootloader to set CNTFRQ. For one thing it's only writeable from the secure side, so a host os can't fix it up for guests that might depend on it rather than dt. I realise it's not necessarily as simple as it sounds to fix that up, however. [...] + timer { + compatible = arm,armv7-timer; + /* PPI secure/nonsecure IRQ */ The comment's now stale, and I don't think it's necessary - the binding defines the order these are in. + interrupts = GIC_PPI 13 (GIC_CPU_MASK_RAW(3) | IRQ_TYPE_LEVEL_LOW), +GIC_PPI 14 (GIC_CPU_MASK_RAW(3) | IRQ_TYPE_LEVEL_LOW), +GIC_PPI 11 (GIC_CPU_MASK_RAW(3) | IRQ_TYPE_LEVEL_LOW), +GIC_PPI 10 (GIC_CPU_MASK_RAW(3) | IRQ_TYPE_LEVEL_LOW); + clock-frequency = 6144000; + }; Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 02/31] CLK: TI: Add DPLL clock support
Hi, On Fri, Aug 02, 2013 at 05:25:21PM +0100, Tero Kristo wrote: The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes. Signed-off-by: Tero Kristo t-kri...@ti.com --- .../devicetree/bindings/clock/ti/dpll.txt | 70 +++ arch/arm/mach-omap2/clock.h| 144 +- arch/arm/mach-omap2/clock3xxx.h|2 - drivers/clk/Makefile |1 + drivers/clk/ti/Makefile|3 + drivers/clk/ti/dpll.c | 488 include/linux/clk/ti.h | 164 +++ 7 files changed, 727 insertions(+), 145 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile create mode 100644 drivers/clk/ti/dpll.c create mode 100644 include/linux/clk/ti.h diff --git a/Documentation/devicetree/bindings/clock/ti/dpll.txt b/Documentation/devicetree/bindings/clock/ti/dpll.txt new file mode 100644 index 000..dcd6e63 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/dpll.txt @@ -0,0 +1,70 @@ +Binding for Texas Instruments DPLL clock. + +This binding uses the common clock binding[1]. It assumes a +register-mapped DPLL with usually two selectable input clocks +(reference clock and bypass clock), with digital phase locked +loop logic for multiplying the input clock to a desired output +clock. This clock also typically supports different operation +modes (locked, low power stop etc.) This binding has several +sub-types, which effectively result in slightly different setup +for the actual DPLL clock. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +Required properties: +- compatible : shall be one of: + ti,omap4-dpll-x2-clock, + ti,omap3-dpll-clock, + ti,omap3-dpll-core-clock, + ti,omap3-dpll-per-clock, + ti,omap3-dpll-per-j-type-clock, + ti,omap4-dpll-clock, + ti,omap4-dpll-core-clock, + ti,omap4-dpll-m4xen-clock, + ti,omap4-dpll-j-type-clock, + ti,omap4-dpll-no-gate-clock, + ti,omap4-dpll-no-gate-j-type-clock, + +- #clock-cells : from common clock binding; shall be set to 0. +- clocks : link phandles of parent clocks (clk-ref and clk-bypass) It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input). +- reg : array of register base addresses for controlling the DPLL (some + of these can also be left as NULL): + reg[0] = control register + reg[1] = idle status register + reg[2] = autoidle register + reg[3] = multiplier / divider set register Are all of these always present? Using reg-names seems sensible here. +- ti,clk-ref : link phandle for the reference clock +- ti,clk-bypass : link phandle for the bypass clock You already have these in clocks, why do you need them again here? + +Optional properties: +- ti,modes : available modes for the DPLL Huh? What type is that property? What does it mean? +- ti,recal-en-bit : recalibration enable bit +- ti,recal-st-bit : recalibration status bit +- ti,auto-recal-bit : auto recalibration enable bit These seem rather low-level, but I see other clock bindings are doing similar things. When are these needed, and why? What type are they? +- ti,clkdm-name : clockdomain name for the DPLL Could you elaborate on what this is for? What constitutes a valid name? I'm not sure a string is the best way to define the linkage of several elements to a domain... + +Examples: + dpll_core_ck: dpll_core_ck@44e00490 { + #clock-cells = 0; + compatible = ti,omap4-dpll-core-clock; + clocks = sys_clkin_ck; + reg = 0x44e00490 0x4, 0x44e0045c 0x4, 0x0 0x4, + 0x44e00468 0x4; + ti,clk-ref = sys_clkin_ck; + ti,clk-bypass = sys_clkin_ck; Huh? Why aren't these both in the clocks property? [...] +static inline void __iomem *get_dt_register(struct device_node *node, + int index) +{ + u32 val; + + of_property_read_u32_index(node, reg, 2 * index, val); + if (val) + return of_iomap(node, index); + else + return NULL; NAK. Use reg-names to handle registers being optional. [...] + clkspec.np = of_parse_phandle(node, ti,clk-ref, 0); + dd-clk_ref = of_clk_get_from_provider(clkspec); + if (IS_ERR(dd-clk_ref)) { + pr_err(%s: ti,clk-ref for %s not found\n, __func__, + clk_name); + goto cleanup; + } + + clkspec.np = of_parse_phandle(node, ti,clk-bypass, 0); +
Re: [PATCHv5 05/31] CLK: TI: add support for OMAP gate clock
On Fri, Aug 02, 2013 at 05:25:24PM +0100, Tero Kristo wrote: This node adds support for a clock node which allows control to the clockdomain enable / disable. Signed-off-by: Tero Kristo t-kri...@ti.com --- .../devicetree/bindings/clock/ti/gate.txt | 41 arch/arm/mach-omap2/clock.h|9 -- drivers/clk/ti/Makefile|2 +- drivers/clk/ti/gate.c | 106 include/linux/clk/ti.h |8 ++ 5 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt create mode 100644 drivers/clk/ti/gate.c diff --git a/Documentation/devicetree/bindings/clock/ti/gate.txt b/Documentation/devicetree/bindings/clock/ti/gate.txt new file mode 100644 index 000..620a73d --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/gate.txt @@ -0,0 +1,41 @@ +Binding for Texas Instruments gate clock. + +This binding uses the common clock binding[1]. This clock is +quite much similar to the basic gate-clock [2], however, +it supports a number of additional features. If no register +is provided for this clock, the code assumes that a clockdomain +will be controlled instead and the corresponding hw-ops for +that is used. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt +[2] Documentation/devicetree/bindings/clock/gate-clock.txt + +Required properties: +- compatible : shall be ti,gate-clock +- #clock-cells : from common clock binding; shall be set to 0 +- clocks : link to phandle of parent clock + +Optional properties: +- reg : base address for register controlling adjustable gate Optional? That's odd. If I have a clock with registers, but don't specify the register, will it still work? i.e. are registerless clocks really compatible with clocks with registers?. +- ti,enable-bit : bit shift for programming the clock gate Why is this needed? Does the hardware vary wildly, or are several clocks sharing the same register(s)? +- ti,dss-clk : use DSS hardware OPS for the clock +- ti,am35xx-clk : use AM35xx hardware OPS for the clock Those last two sounds like the kind of thing that should be derived from the compatible string (e.g. ti,am35xx-gate-clock). +- ti,clkdm-name : clockdomain to control this gate As previously mentioned, I'm not a fan of this property. It would make more sense to describe the domain and have an explicit link to it (either nodes being children of the domain or having a phandle). [...] +void __init of_omap_gate_clk_setup(struct device_node *node) +{ + struct clk *clk; + struct clk_init_data init = { 0 }; + struct clk_hw_omap *clk_hw; + const char *clk_name = node-name; + int num_parents; + const char **parent_names = NULL; + int i; + u32 val; + + clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL); + if (!clk_hw) { + pr_err(%s: could not allocate clk_hw_omap\n, __func__); + return; + } + + clk_hw-hw.init = init; + + of_property_read_string(node, clock-output-names, clk_name); + of_property_read_string(node, ti,clkdm-name, clk_hw-clkdm_name); + + init.name = clk_name; + init.flags = 0; + + if (of_property_read_u32_index(node, reg, 0, val)) { + /* No register, clkdm control only */ + init.ops = omap_gate_clkdm_clk_ops; If they're truly compatible, you can just see if you can of_iomap, and if not, continue. Your reg values might be wider than 32 bits, and usig of_property_read_u32 to read this feels wrong. + } else { + init.ops = omap_gate_clk_ops; + clk_hw-enable_reg = of_iomap(node, 0); + of_property_read_u32(node, ti,enable-bit, val); + clk_hw-enable_bit = val; What if of_property_read_u32 failed to read the ti,enable-bit property? One might not be present, it's marked as optional in the binding description. + + clk_hw-ops = clkhwops_wait; + + if (of_property_read_bool(node, ti,dss-clk)) + clk_hw-ops = clkhwops_omap3430es2_dss_usbhost_wait; + + if (of_property_read_bool(node, ti,am35xx-clk)) + clk_hw-ops = clkhwops_am35xx_ipss_module_wait; I really don't like this. I think this should be done based on the compatible string. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 16/31] CLK: TI: DRA7: Add APLL support
On Fri, Aug 02, 2013 at 05:25:35PM +0100, Tero Kristo wrote: From: Keerthy j-keer...@ti.com The patch adds support for DRA7 PCIe APLL. The APLL sources the optional functional clocks for PCIe module. APLL stands for Analog PLL. This is different when comapred with DPLL meaning Digital PLL, the phase detection is done using an analog circuit. Signed-off-by: Keerthy j-keer...@ti.com Signed-off-by: Tero Kristo t-kri...@ti.com --- .../devicetree/bindings/clock/ti/apll.txt | 32 +++ arch/arm/mach-omap2/clock.h|1 - drivers/clk/ti/Makefile|2 +- drivers/clk/ti/apll.c | 209 include/linux/clk/ti.h |2 + 5 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/apll.txt create mode 100644 drivers/clk/ti/apll.c diff --git a/Documentation/devicetree/bindings/clock/ti/apll.txt b/Documentation/devicetree/bindings/clock/ti/apll.txt new file mode 100644 index 000..f7a82e9 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/apll.txt @@ -0,0 +1,32 @@ +Binding for Texas Instruments APLL clock. + +This binding uses the common clock binding[1]. It assumes a +register-mapped APLL with usually two selectable input clocks +(reference clock and bypass clock), with analog phase locked +loop logic for multiplying the input clock to a desired output +clock. This clock also typically supports different operation +modes (locked, low power stop etc.) APLL mostly behaves like +a subtype of a DPLL [2], although a simplified one at that. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt +[2] Documentation/devicetree/bindings/clock/ti/dpll.txt + +Required properties: +- compatible : shall be ti,dra7-apll-clock +- #clock-cells : from common clock binding; shall be set to 0. +- clocks : link phandles of parent clocks (clk-ref and clk-bypass) +- reg : array of register base addresses for controlling the APLL: + reg[0] = control register + reg[1] = idle status register Using reg-names is likely a good idea here. +- ti,clk-ref : link phandle for the reference clock +- ti,clk-bypass : link phandle for the bypass clock You don't need this. Use the clocks and clock-names properties. [...] +static int dra7_apll_enable(struct clk_hw *hw) +{ + struct clk_hw_omap *clk = to_clk_hw_omap(hw); + int r = 0, i = 0; + struct dpll_data *ad; + const char *clk_name; + u8 state = 1; + u32 v; + + ad = clk-dpll_data; + if (!ad) + return -EINVAL; + + clk_name = __clk_get_name(clk-hw.clk); + + state = __ffs(ad-idlest_mask); + + /* Check is already locked */ + if ((__raw_readl(ad-idlest_reg) ad-idlest_mask) == state) + return r; Why __raw_readl rather than raw_readl? + + v = __raw_readl(ad-control_reg); + v = ~ad-enable_mask; + v |= APLL_FORCE_LOCK __ffs(ad-enable_mask); + __raw_writel(v, ad-control_reg); Why not raw_writel? Do you not need the rmb() provided by writel, here or anywhere else? [...] +void __init of_dra7_apll_setup(struct device_node *node) +{ + const struct clk_ops *ops; + struct clk *clk; + const char *clk_name = node-name; + int num_parents; + const char **parent_names = NULL; + struct of_phandle_args clkspec; + u8 apll_flags = 0; + struct dpll_data *ad; + u32 idlest_mask = 0x1; + u32 autoidle_mask = 0x3; + int i; + + ops = apll_ck_ops; + ad = kzalloc(sizeof(*ad), GFP_KERNEL); + if (!ad) { + pr_err(%s: could not allocate dpll_data\n, __func__); + return; + } + + of_property_read_string(node, clock-output-names, clk_name); + + num_parents = of_clk_get_parent_count(node); + if (num_parents 1) { + pr_err(%s: omap dpll %s must have parent(s)\n, + __func__, node-name); + goto cleanup; + } + + parent_names = kzalloc(sizeof(char *) * num_parents, GFP_KERNEL); + + for (i = 0; i num_parents; i++) + parent_names[i] = of_clk_get_parent_name(node, i); + + clkspec.np = of_parse_phandle(node, ti,clk-ref, 0); + ad-clk_ref = of_clk_get_from_provider(clkspec); Use clocks, clock-names, and of_clk_get_by_name(). Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 18/31] CLK: DT: add support for set-rate-parent flag
On Fri, Aug 02, 2013 at 05:25:37PM +0100, Tero Kristo wrote: Adding set-rate-parent to clock node now allows a node to forward clk_set_rate request to its parent clock. Why do you need this? Is this a description of the hardware, or configuration for Linux? It feels like the latter, which shouldn't really be in DT. I'm not that familiar with the internals of the clock framework, and don't understand the implications of this. If this is necessary, the bindings need to be updated. Mike, thoughts? Mark. Signed-off-by: Tero Kristo t-kri...@ti.com --- drivers/clk/clk-divider.c |6 +- drivers/clk/clk-fixed-factor.c |6 +- drivers/clk/clk-gate.c |8 ++-- drivers/clk/clk-mux.c |6 +- 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index ff24ec2..01d967f 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -388,6 +388,7 @@ void of_divider_clk_setup(struct device_node *node) u32 mask = 0; u32 shift = 0; struct clk_div_table *table; + u32 flags = 0; of_property_read_string(node, clock-output-names, clk_name); @@ -418,12 +419,15 @@ void of_divider_clk_setup(struct device_node *node) if (of_property_read_bool(node, hiword-mask)) clk_divider_flags |= CLK_DIVIDER_HIWORD_MASK; + if (of_property_read_bool(node, set-rate-parent)) + flags |= CLK_SET_RATE_PARENT; + table = of_clk_get_div_table(node); if (IS_ERR(table)) return; clk = _register_divider(NULL, clk_name, - parent_name, 0, + parent_name, flags, reg, (u8) shift, mask, clk_divider_flags, table, NULL); diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c index 9ff7d51..30aa121 100644 --- a/drivers/clk/clk-fixed-factor.c +++ b/drivers/clk/clk-fixed-factor.c @@ -107,6 +107,7 @@ void __init of_fixed_factor_clk_setup(struct device_node *node) const char *clk_name = node-name; const char *parent_name; u32 div, mult; + u32 flags = 0; if (of_property_read_u32(node, clock-div, div)) { pr_err(%s Fixed factor clock %s must have a clock-div property\n, @@ -123,7 +124,10 @@ void __init of_fixed_factor_clk_setup(struct device_node *node) of_property_read_string(node, clock-output-names, clk_name); parent_name = of_clk_get_parent_name(node, 0); - clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0, + if (of_property_read_bool(node, set-rate-parent)) + flags |= CLK_SET_RATE_PARENT; + + clk = clk_register_fixed_factor(NULL, clk_name, parent_name, flags, mult, div); if (!IS_ERR(clk)) of_clk_add_provider(node, of_clk_src_simple_get, clk); diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c index cd595ec..0be25b9 100644 --- a/drivers/clk/clk-gate.c +++ b/drivers/clk/clk-gate.c @@ -176,6 +176,7 @@ void of_gate_clk_setup(struct device_node *node) const char *parent_name; u8 clk_gate_flags = 0; u32 bit_idx = 0; + u32 flags = 0; of_property_read_string(node, clock-output-names, clk_name); @@ -195,8 +196,11 @@ void of_gate_clk_setup(struct device_node *node) if (of_property_read_bool(node, hiword-mask)) clk_gate_flags |= CLK_GATE_HIWORD_MASK; - clk = clk_register_gate(NULL, clk_name, parent_name, 0, reg, (u8) bit_idx, - clk_gate_flags, NULL); + if (of_property_read_bool(node, set-rate-parent)) + flags |= CLK_SET_RATE_PARENT; + + clk = clk_register_gate(NULL, clk_name, parent_name, flags, reg, + (u8) bit_idx, clk_gate_flags, NULL); if (!IS_ERR(clk)) of_clk_add_provider(node, of_clk_src_simple_get, clk); diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c index 4751bce..890ddbf 100644 --- a/drivers/clk/clk-mux.c +++ b/drivers/clk/clk-mux.c @@ -184,6 +184,7 @@ void of_mux_clk_setup(struct device_node *node) u8 clk_mux_flags = 0; u32 mask = 0; u32 shift = 0; + u32 flags = 0; of_property_read_string(node, clock-output-names, clk_name); @@ -219,8 +220,11 @@ void of_mux_clk_setup(struct device_node *node) if (of_property_read_bool(node, hiword-mask)) clk_mux_flags |= CLK_MUX_HIWORD_MASK; + if (of_property_read_bool(node, set-rate-parent)) + flags |= CLK_SET_RATE_PARENT; + clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents, - 0, reg, (u8) shift, mask, clk_mux_flags, + flags, reg, (u8) shift, mask, clk_mux_flags, NULL, NULL); if
Re: [PATCHv5 22/31] CLK: TI: add interface clock support for OMAP3
On Fri, Aug 02, 2013 at 05:25:41PM +0100, Tero Kristo wrote: OMAP3 has interface clocks in addition to functional clocks, which require special handling for the autoidle and idle status register offsets mainly. Signed-off-by: Tero Kristo t-kri...@ti.com --- .../devicetree/bindings/clock/ti/interface.txt | 45 + arch/arm/mach-omap2/clock.h|6 -- drivers/clk/ti/Makefile|2 +- drivers/clk/ti/interface.c | 105 include/linux/clk/ti.h |7 ++ 5 files changed, 158 insertions(+), 7 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/interface.txt create mode 100644 drivers/clk/ti/interface.c diff --git a/Documentation/devicetree/bindings/clock/ti/interface.txt b/Documentation/devicetree/bindings/clock/ti/interface.txt new file mode 100644 index 000..8b09ae7 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/interface.txt @@ -0,0 +1,45 @@ +Binding for Texas Instruments interface clock. + +This binding uses the common clock binding[1]. This clock is +quite much similar to the basic gate-clock [2], however, +it supports a number of additional features, including +companion clock finding (match corresponding functional gate +clock) and hardware autoidle enable / disable. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt +[2] Documentation/devicetree/bindings/clock/gate-clock.txt + +Required properties: +- compatible : shall be ti,interface-clock It might make sense to be more specific: ti,omap3-interface-clock. +- #clock-cells : from common clock binding; shall be set to 0 +- clocks : link to phandle of parent clock +- reg : base address for the control register + +Optional properties: +- ti,enable-bit : bit shift for the bit enabling/disabling the clock + (default 0) +- ti,iclk-no-wait : flag for selecting non-waiting hw-ops +- ti,iclk-hsotgusb : flag for selecting hsotgusb hw-ops +- ti,iclk-dss : flag for selecting DSS interface clock hw-ops +- ti,iclk-ssi : flag for selecting SSI interface clock hw-ops +- ti,am35xx-clk : flag for selecting AM35xx interface clock hw-ops I think these should be selected based on the compatible string. They're mutually exclusive, and incompatible. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT
[Adding Mike Turquette and dt maintainers to Cc] On Tue, Jul 23, 2013 at 07:24:38AM +0100, Rajendra Nayak wrote: With clocks for OMAP moving to DT, its now possible to pass all optional clock data for each device from DT instead of having it in hwmod. Signed-off-by: Rajendra Nayak rna...@ti.com --- arch/arm/mach-omap2/omap_hwmod.c | 66 -- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 12fa589..e5c804b 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -805,6 +805,65 @@ static int _init_interface_clks(struct omap_hwmod *oh) return ret; } +static const char **_parse_opt_clks_dt(struct omap_hwmod *oh, +struct device_node *np, +int *opt_clks_cnt) +{ + int i, clks_cnt; + const char *clk_name; + const char **opt_clk_names; + + clks_cnt = of_property_count_strings(np, clock-names); + if (!clks_cnt) + return NULL; + + opt_clk_names = kzalloc(sizeof(char *)*clks_cnt, GFP_KERNEL); + if (!opt_clk_names) + return NULL; + + for (i = 0; i clks_cnt; i++) { + of_property_read_string_index(np, clock-names, i, clk_name); + if (!strcmp(clk_name, fck)) + continue; + opt_clks_cnt++; + opt_clk_names[i] = clk_name; + } + return opt_clk_names; +} + +static int _init_opt_clks_dt(struct omap_hwmod *oh, struct device_node *np) +{ + struct clk *c; + int i, opt_clks_cnt = 0; + int ret = 0; + const char **opt_clk_names; + + opt_clk_names = _parse_opt_clks_dt(oh, np, opt_clks_cnt); + if (!opt_clk_names) + return -EINVAL; + + oh-opt_clks = kzalloc(sizeof(struct omap_hwmod_opt_clk *) +* opt_clks_cnt, GFP_KERNEL); + if (!oh-opt_clks) + return -ENOMEM; + + oh-opt_clks_cnt = opt_clks_cnt; + + for (i = 0; i oh-opt_clks_cnt; i++) { + c = of_clk_get_by_name(np, opt_clk_names[i]); + if (IS_ERR(c)) { + pr_warn(omap_hwmod: %s: cannot clk_get opt_clk %s\n, + oh-name, opt_clk_names[i]); + ret = -EINVAL; + } + oh-opt_clks[i]._clk = c; + oh-opt_clks[i].role = opt_clk_names[i]; + oh-opt_clks_cnt++; + clk_prepare(oh-opt_clks[i]._clk); + } + return ret; +} I don't like this. clock-names is used to represent the names of clocks as inputs to the device. The driver must know the names of each and every one of the clock inputs it intends to use -- there's a finite number, and if it doesn't know about it it clearly has no idea how that clock's meant to be used. Consider a future revision of the hardware that has an additional clock input. Some new feature may require that clock, but your driver won't support that new feature, so you don't need it. Preparing that clock is a waste of power, and could cause issues if for some reason the clock was mutually exlcusive with another clock (so preparing it would make the hardware unusable). If the new revision *requires* that clock to provide the same interface otherwise, it's not backwards compatible and needs a new binding, and the driver needs to be extended to support it. Given that, preparing all the clocks you've been handed is a hack. Simply request by name the ones you know you need, and attempt to request the optional ones as necessary. Don't blindly go and prepare everything. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT
[Adding Mike Turquette and dt maintainers] On Wed, Aug 14, 2013 at 02:39:44PM +0100, Nishanth Menon wrote: On 08/14/2013 08:20 AM, Rajendra Nayak wrote: On Wednesday 14 August 2013 06:18 PM, Nishanth Menon wrote: Hi Rajendra, On Tue, Jul 23, 2013 at 1:24 AM, Rajendra Nayak rna...@ti.com wrote: [..] diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 12fa589..e5c804b 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -805,6 +805,65 @@ static int _init_interface_clks(struct omap_hwmod *oh) return ret; } +static const char **_parse_opt_clks_dt(struct omap_hwmod *oh, + struct device_node *np, + int *opt_clks_cnt) +{ + int i, clks_cnt; + const char *clk_name; + const char **opt_clk_names; + + clks_cnt = of_property_count_strings(np, clock-names); + if (!clks_cnt) + return NULL; + + opt_clk_names = kzalloc(sizeof(char *)*clks_cnt, GFP_KERNEL); + if (!opt_clk_names) + return NULL; + + for (i = 0; i clks_cnt; i++) { + of_property_read_string_index(np, clock-names, i, clk_name); + if (!strcmp(clk_name, fck)) Could we instead parse for names that are optional,role_name instead of assuming anything other than fck is optional clocks? you mean look for anything with optional,*? because the role names would change. yes. the idea being, we now have a meaning to the clock name - there are two types of clocks here.. functional and optional, we *might* have facility to add interface clock(we dont know interface clock handling yet, but something in the future).. we might increase the support for number of functional clocks.. it might help to keep the format such that it is a bit extendable. I completely disagree. The only things that should appear in clock-names are the names of the clock inputs that appear in the manual for the device. The driver should know which ones are optional, as that's a fixed property of the IP and the way the driver uses it. You should not be embedding additional semantics in name properties. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT
On Wed, Aug 14, 2013 at 02:54:57PM +0100, Rajendra Nayak wrote: On Wednesday 14 August 2013 07:15 PM, Mark Rutland wrote: [Adding Mike Turquette and dt maintainers to Cc] On Tue, Jul 23, 2013 at 07:24:38AM +0100, Rajendra Nayak wrote: With clocks for OMAP moving to DT, its now possible to pass all optional clock data for each device from DT instead of having it in hwmod. Signed-off-by: Rajendra Nayak rna...@ti.com --- arch/arm/mach-omap2/omap_hwmod.c | 66 -- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 12fa589..e5c804b 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -805,6 +805,65 @@ static int _init_interface_clks(struct omap_hwmod *oh) return ret; } +static const char **_parse_opt_clks_dt(struct omap_hwmod *oh, + struct device_node *np, + int *opt_clks_cnt) +{ + int i, clks_cnt; + const char *clk_name; + const char **opt_clk_names; + + clks_cnt = of_property_count_strings(np, clock-names); + if (!clks_cnt) + return NULL; + + opt_clk_names = kzalloc(sizeof(char *)*clks_cnt, GFP_KERNEL); + if (!opt_clk_names) + return NULL; + + for (i = 0; i clks_cnt; i++) { + of_property_read_string_index(np, clock-names, i, clk_name); + if (!strcmp(clk_name, fck)) + continue; + opt_clks_cnt++; + opt_clk_names[i] = clk_name; + } + return opt_clk_names; +} + +static int _init_opt_clks_dt(struct omap_hwmod *oh, struct device_node *np) +{ + struct clk *c; + int i, opt_clks_cnt = 0; + int ret = 0; + const char **opt_clk_names; + + opt_clk_names = _parse_opt_clks_dt(oh, np, opt_clks_cnt); + if (!opt_clk_names) + return -EINVAL; + + oh-opt_clks = kzalloc(sizeof(struct omap_hwmod_opt_clk *) + * opt_clks_cnt, GFP_KERNEL); + if (!oh-opt_clks) + return -ENOMEM; + + oh-opt_clks_cnt = opt_clks_cnt; + + for (i = 0; i oh-opt_clks_cnt; i++) { + c = of_clk_get_by_name(np, opt_clk_names[i]); + if (IS_ERR(c)) { + pr_warn(omap_hwmod: %s: cannot clk_get opt_clk %s\n, + oh-name, opt_clk_names[i]); + ret = -EINVAL; + } + oh-opt_clks[i]._clk = c; + oh-opt_clks[i].role = opt_clk_names[i]; + oh-opt_clks_cnt++; + clk_prepare(oh-opt_clks[i]._clk); + } + return ret; +} I don't like this. clock-names is used to represent the names of clocks as inputs to the device. The driver must know the names of each and every one of the clock inputs it intends to use -- there's a finite number, and if it doesn't know about it it clearly has no idea how that clock's meant to be used. Consider a future revision of the hardware that has an additional clock input. Some new feature may require that clock, but your driver won't support that new feature, so you don't need it. Preparing that clock is a waste of power, and could cause issues if for some reason the clock was mutually exlcusive with another clock (so preparing it would make the hardware unusable). If the new revision *requires* that clock to provide the same interface otherwise, it's not backwards compatible and needs a new binding, and the driver needs to be extended to support it. Given that, preparing all the clocks you've been handed is a hack. Mark, this is a piece of platform code (hwmod framework for omap) which does a enable/reset/idle of all devices on the SoC early at boot to get rid of bootloader dependencies. This isn;t something used by the drivers when they enable the devices. I don't see any issue with 'waste of power'. The framework (unlike the driver) has no knowledge of what clocks are needed and hence does a enable all momentarily to reset and put the device in a known state. Ok, I'd misunderstood there. Apologies for the noise. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT
On Wed, Aug 14, 2013 at 02:58:25PM +0100, Nishanth Menon wrote: On 08/14/2013 08:49 AM, Mark Rutland wrote: [Adding Mike Turquette and dt maintainers] On Wed, Aug 14, 2013 at 02:39:44PM +0100, Nishanth Menon wrote: On 08/14/2013 08:20 AM, Rajendra Nayak wrote: On Wednesday 14 August 2013 06:18 PM, Nishanth Menon wrote: Hi Rajendra, On Tue, Jul 23, 2013 at 1:24 AM, Rajendra Nayak rna...@ti.com wrote: [..] diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 12fa589..e5c804b 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -805,6 +805,65 @@ static int _init_interface_clks(struct omap_hwmod *oh) return ret; } +static const char **_parse_opt_clks_dt(struct omap_hwmod *oh, + struct device_node *np, + int *opt_clks_cnt) +{ + int i, clks_cnt; + const char *clk_name; + const char **opt_clk_names; + + clks_cnt = of_property_count_strings(np, clock-names); + if (!clks_cnt) + return NULL; + + opt_clk_names = kzalloc(sizeof(char *)*clks_cnt, GFP_KERNEL); + if (!opt_clk_names) + return NULL; + + for (i = 0; i clks_cnt; i++) { + of_property_read_string_index(np, clock-names, i, clk_name); + if (!strcmp(clk_name, fck)) Could we instead parse for names that are optional,role_name instead of assuming anything other than fck is optional clocks? you mean look for anything with optional,*? because the role names would change. yes. the idea being, we now have a meaning to the clock name - there are two types of clocks here.. functional and optional, we *might* have facility to add interface clock(we dont know interface clock handling yet, but something in the future).. we might increase the support for number of functional clocks.. it might help to keep the format such that it is a bit extendable. I completely disagree. The only things that should appear in clock-names are the names of the clock inputs that appear in the manual for the device. The driver should know which ones are optional, as that's a fixed property of the IP and the way the driver uses it. You should not be embedding additional semantics in name properties. we use an level of abstraction called omap_device and hwmod to allow devices to use a generic pm_runtime. drivers for specific blocks dont normally need to know about the clocks to deal with. This allows maximum reuse consider concept is generic enough. The traditional way of dealing with this has been to encode the *names* *inside* hwmod!!! which is obviously the wrong way to deal with with clock nodes in dts. This series moves away from that approach - which is good. Now, the question is: clocks-names = . is not sufficient to indicate a list of clocks of different functions 3 types of clocks may exist for OMAP h/w blocks: a) functional clock (1 usually, but we are starting to see multiple fclks). this drives the actual functionality b) optional clocks - additional clocks such as debounce clock etc. - there can be n such clocks c) interface clocks - this drives the register interface on the bus. - there can be n such clocks and additional headaches for interface - currently embedded inside hwmod. I don't see why these can't all be supported without embedding additional details in the clock-names property. When you say there can be n such clocks, do you mean that different devices may have different numbers of clocks, or that a given device could have an arbitrary set? Surely the design of the IP imposes a maximum limit on the number of clocks a device may have? you could do: A) embedd in the sematics of the name clock-names=fck, optional,role1, optional,role2; clocks= clk_a, clk_x, clk_y; OR: Does the name of the clock itself not encode this? Surely fclk would be your functional clock, dbncclk or similar would be an (optional) debounce clock, apbclk would be an interface clock? Surely you must know their type by which input they are and how they are to be used? From the sounds of your other reply, the issue is that you're poking clocks for arbitrary devices, without knowing the semantics of those clocks, because you're doing it form outside the driver. I don't have a good solution to this, but your proposed solution is not one I'm happy with. B) embedd in the property name optional-clock-names=optional,role1, optional,role2; optional-clocks=clk_x, clk_y; functional-clock-names=fck; functional-clocks=clk_a; are you suggesting (b) here? I'm suggesting neither. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo
Re: [PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT
On Wed, Aug 14, 2013 at 03:05:25PM +0100, Rajendra Nayak wrote: On Wednesday 14 August 2013 07:28 PM, Nishanth Menon wrote: On 08/14/2013 08:49 AM, Mark Rutland wrote: [Adding Mike Turquette and dt maintainers] On Wed, Aug 14, 2013 at 02:39:44PM +0100, Nishanth Menon wrote: On 08/14/2013 08:20 AM, Rajendra Nayak wrote: On Wednesday 14 August 2013 06:18 PM, Nishanth Menon wrote: Hi Rajendra, On Tue, Jul 23, 2013 at 1:24 AM, Rajendra Nayak rna...@ti.com wrote: [..] diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 12fa589..e5c804b 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -805,6 +805,65 @@ static int _init_interface_clks(struct omap_hwmod *oh) return ret; } +static const char **_parse_opt_clks_dt(struct omap_hwmod *oh, + struct device_node *np, + int *opt_clks_cnt) +{ + int i, clks_cnt; + const char *clk_name; + const char **opt_clk_names; + + clks_cnt = of_property_count_strings(np, clock-names); + if (!clks_cnt) + return NULL; + + opt_clk_names = kzalloc(sizeof(char *)*clks_cnt, GFP_KERNEL); + if (!opt_clk_names) + return NULL; + + for (i = 0; i clks_cnt; i++) { + of_property_read_string_index(np, clock-names, i, clk_name); + if (!strcmp(clk_name, fck)) Could we instead parse for names that are optional,role_name instead of assuming anything other than fck is optional clocks? you mean look for anything with optional,*? because the role names would change. yes. the idea being, we now have a meaning to the clock name - there are two types of clocks here.. functional and optional, we *might* have facility to add interface clock(we dont know interface clock handling yet, but something in the future).. we might increase the support for number of functional clocks.. it might help to keep the format such that it is a bit extendable. I completely disagree. The only things that should appear in clock-names are the names of the clock inputs that appear in the manual for the device. The driver should know which ones are optional, as that's a fixed property of the IP and the way the driver uses it. You should not be embedding additional semantics in name properties. we use an level of abstraction called omap_device and hwmod to allow devices to use a generic pm_runtime. drivers for specific blocks dont normally need to know about the clocks to deal with. This allows maximum reuse consider concept is generic enough. They do know about the optional clocks though and request and release them when needed. The need for hwmod to know about optional clocks (and enable all) arises from the fact that some of these devices need *some* optional clocks for a successful reset. And given hmwod has no knowledge about which optional ones (if at all) will be needed, it goes ahead and enables all before doing a reset. This is something done only at init time and *not* something thats done every time the device is enabled by the driver using pm_runtime. To clarify: I was initially confused as to the purpose of the code. I'm not against a one-off clock initialisation to put everything into a sane state. If we can't trust the bootloaders, that seems like a necessary evil. I'll leave Mike to comment on whether and how that should be done. I do not think we should be embedding clock semantics in clock-names. That's not the way the property is intended to be used, it breaks uniformity, and it's an abuse of the system that may come back to bite us later. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 0/2] TWL6030, TWL6032 GPADC driver
Hi, apologies for the late reply. On Thu, Jul 25, 2013 at 02:26:51PM +0100, Oleksandr Kozaruk wrote: Hello, v8 - removed unused test channels completely, removed die temperature channels, as it is not known how to convert ADC code to temperature. There if formula for twl6030, but no formula for twl6032. v7 - addressed clean up comments, removed test channels v6 - addressed comments about trim bits, checkpatch clean up v5 - gpadc DT node renamed from gpadc to generic adc, added temperature channels; raw code is corracted with calibration data. v4 - addressed comments: fixed style violation, bug in freeing memory, added comments explaining calibration method, removed test network channels from exposing to userspace, error handling for wait_for_complition v3 - fixed compiler warning v2 - the driver put in drivers/iio, and converted using iio facilities as suggested by Graeme. TWL603[02] GPADC is used to measure battery voltage, battery temperature, battery presence ID, and could be used to measure twl603[02] die temperature. This is used on TI blaze, blaze tablet platforms. The TWL6030/TWL6032 is a PMIC that has a GPADC with 17/19 channels respectively. Some channels have current source and are used for measuring voltage drop on resistive load for detecting battery ID resistance, or measuring voltage drop on NTC resistors for external temperature measurements, other channels measure voltage, (i.e. battery voltage), and have inbuilt voltage dividers, thus, capable to scale voltage. Some channels are dedicated for measuring die temperature. Some channels could be calibrated in 2 points, having offsets from ideal values in trim registers. The difference between GPADC in TWL6030 and TWL6032: - 10 bit vs 12 bit ADC; - 17 vs 19 channels; - channels have different purpose(i. e. battery voltage channel 8 vs channel 18); - trim values are interpreted differently. The driver is derived from git://git.omapzoom.org/kernel/omap.git The original driver's authors and contributors are Balaji T K, Graeme Gregory, Ambresh K, Girish S Ghongdemath. The changes to the original driver: - device tree adaptation; I couldn't see a binding document in this series or in mainline. Have I looked in the wrong places? Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions
Hi Benoit, On Fri, Aug 16, 2013 at 03:15:57PM +0100, Benoit Cousson wrote: Hi Gururaja, On 16/08/2013 13:36, Hebbar, Gururaja wrote: The syntax of compatible property in DT is to mention the Most specific match to most generic match. Since AM335x is the platform with latest IP revision, add it 1st in the device id table. I don't understand why? The order should not matter at all. I've tried to follow the thread you had with Mark on the v2, but AFAIK, you've never answered to his latest question. Moreover, checking the differences between the Davinci and the am3352 RTC IP, I would not claim that both are compatible. Sure you can use the am3352 with the Davinci driver, but you will lose the wakeup functionality without even being notify about that. Could you describe the wakeup functionality, and how it differs between the am3352-rtc and the da830-rtc? As I understand it, the am3352 functionality is a superset of the da830 functionality. You can use the old driver, and get some functionality, or use the new driver and get it all. That means that am3352-rtc is compatible with da830. As long as the kernel first checks for am3352-rtc, there will be *no* loss of functionality. All this does is enable older kernels to use the hardware in some fashion, and given the older kernel didn't have support for the am3352-rtc features, this is a *gain* in functionality, not a loss. For my point of view, compatible mean that the HW will still be fully functional with both versions of the driver, which is not the case here. What? A driver for any entry in the compatible list should be able to drive the hardware to *some* level of functionality. We list from most-specific to most-general to allow a graceful degradation from fully supported to bare minimum functionality. am3352 DTS must use the ti,am3352-rtc to have the expected behavior. Using the ti,da830-rtc version will not make the board working as expected. So we cannot claim the compatibility. Please can you explain what you mean by expected behaviour? This way, we can add new matching compatible as 1st and maintain old compatible string for backwards compatibility. ex: compatible = ti,am3352-rtc, ti,da830-rtc; Signed-off-by: Hebbar, Gururaja gururaja.heb...@ti.com CC: mark.rutl...@arm.com --- Changes in v3: - new patch :100644 100644 dc62cc3... 2f0968c... M drivers/rtc/rtc-omap.c drivers/rtc/rtc-omap.c |6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c index dc62cc3..2f0968c 100644 --- a/drivers/rtc/rtc-omap.c +++ b/drivers/rtc/rtc-omap.c @@ -330,12 +330,12 @@ static struct platform_device_id omap_rtc_devtype[] = { MODULE_DEVICE_TABLE(platform, omap_rtc_devtype); static const struct of_device_id omap_rtc_of_match[] = { - { .compatible = ti,da830-rtc, - .data = omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX], - }, { .compatible = ti,am3352-rtc, .data = omap_rtc_devtype[OMAP_RTC_DATA_AM335X_IDX], }, + { .compatible = ti,da830-rtc, + .data = omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX], + }, {}, }; MODULE_DEVICE_TABLE(of, omap_rtc_of_match); Bottom-line, if you get rid of the old compatible entry, you will not have to play some trick with the entries order. I don't think it's playing tricks. It's ensuring compatibility, and it's a simple change in ordering. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 02/31] CLK: TI: Add DPLL clock support
On Mon, Aug 19, 2013 at 02:34:45PM +0100, Tero Kristo wrote: On 08/13/2013 01:50 PM, Mark Rutland wrote: Hi, On Fri, Aug 02, 2013 at 05:25:21PM +0100, Tero Kristo wrote: The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes. Signed-off-by: Tero Kristo t-kri...@ti.com --- .../devicetree/bindings/clock/ti/dpll.txt | 70 +++ arch/arm/mach-omap2/clock.h| 144 +- arch/arm/mach-omap2/clock3xxx.h|2 - drivers/clk/Makefile |1 + drivers/clk/ti/Makefile|3 + drivers/clk/ti/dpll.c | 488 include/linux/clk/ti.h | 164 +++ 7 files changed, 727 insertions(+), 145 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile create mode 100644 drivers/clk/ti/dpll.c create mode 100644 include/linux/clk/ti.h diff --git a/Documentation/devicetree/bindings/clock/ti/dpll.txt b/Documentation/devicetree/bindings/clock/ti/dpll.txt new file mode 100644 index 000..dcd6e63 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/dpll.txt @@ -0,0 +1,70 @@ +Binding for Texas Instruments DPLL clock. + +This binding uses the common clock binding[1]. It assumes a +register-mapped DPLL with usually two selectable input clocks +(reference clock and bypass clock), with digital phase locked +loop logic for multiplying the input clock to a desired output +clock. This clock also typically supports different operation +modes (locked, low power stop etc.) This binding has several +sub-types, which effectively result in slightly different setup +for the actual DPLL clock. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +Required properties: +- compatible : shall be one of: + ti,omap4-dpll-x2-clock, + ti,omap3-dpll-clock, + ti,omap3-dpll-core-clock, + ti,omap3-dpll-per-clock, + ti,omap3-dpll-per-j-type-clock, + ti,omap4-dpll-clock, + ti,omap4-dpll-core-clock, + ti,omap4-dpll-m4xen-clock, + ti,omap4-dpll-j-type-clock, + ti,omap4-dpll-no-gate-clock, + ti,omap4-dpll-no-gate-j-type-clock, + +- #clock-cells : from common clock binding; shall be set to 0. +- clocks : link phandles of parent clocks (clk-ref and clk-bypass) It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input). All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always? If they're separate inputs, it's worth listing both (even if they're fed by the same provider). If it's possible one input might not be wired up, use clock-names. +- reg : array of register base addresses for controlling the DPLL (some + of these can also be left as NULL): + reg[0] = control register + reg[1] = idle status register + reg[2] = autoidle register + reg[3] = multiplier / divider set register Are all of these always present? Using reg-names seems sensible here. Not always, I'll change the code. I am quite new to DT and didn't actually know of the existence of reg-names prop. Ok. :) +- ti,recal-en-bit : recalibration enable bit +- ti,recal-st-bit : recalibration status bit +- ti,auto-recal-bit : auto recalibration enable bit These seem rather low-level, but I see other clock bindings are doing similar things. When are these needed, and why? What type are they? Bit shift values for the auto recalibration feature. HOWEVER, it seems these are actually legacy, kernel does not have support for these anymore if it ever had I think I can just drop these for now as they are unused by the support code even. Ok. +- ti,clkdm-name : clockdomain name for the DPLL Could you elaborate on what this is for? What constitutes a valid name? I'm not sure a string is the best way to define the linkage of several elements to a domain... Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this. I'll have to see the documentation, but I'd be very wary of putting that in as-is. Does having the clock domain string link this up to domains in platform data? I'm not sure how well we'll be able to maintain support for that in future if it requires other platform code to use now, and we're not sure how
Re: [PATCHv5 05/31] CLK: TI: add support for OMAP gate clock
On Mon, Aug 19, 2013 at 02:42:05PM +0100, Tero Kristo wrote: On 08/13/2013 02:04 PM, Mark Rutland wrote: On Fri, Aug 02, 2013 at 05:25:24PM +0100, Tero Kristo wrote: This node adds support for a clock node which allows control to the clockdomain enable / disable. Signed-off-by: Tero Kristo t-kri...@ti.com --- .../devicetree/bindings/clock/ti/gate.txt | 41 arch/arm/mach-omap2/clock.h|9 -- drivers/clk/ti/Makefile|2 +- drivers/clk/ti/gate.c | 106 include/linux/clk/ti.h |8 ++ 5 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt create mode 100644 drivers/clk/ti/gate.c diff --git a/Documentation/devicetree/bindings/clock/ti/gate.txt b/Documentation/devicetree/bindings/clock/ti/gate.txt new file mode 100644 index 000..620a73d --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/gate.txt @@ -0,0 +1,41 @@ +Binding for Texas Instruments gate clock. + +This binding uses the common clock binding[1]. This clock is +quite much similar to the basic gate-clock [2], however, +it supports a number of additional features. If no register +is provided for this clock, the code assumes that a clockdomain +will be controlled instead and the corresponding hw-ops for +that is used. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt +[2] Documentation/devicetree/bindings/clock/gate-clock.txt + +Required properties: +- compatible : shall be ti,gate-clock +- #clock-cells : from common clock binding; shall be set to 0 +- clocks : link to phandle of parent clock + +Optional properties: +- reg : base address for register controlling adjustable gate Optional? That's odd. If I have a clock with registers, but don't specify the register, will it still work? i.e. are registerless clocks really compatible with clocks with registers?. I think I implemented this in somewhat confusing manner. This could be split to: ti,gate-clock: requires reg and ti,enable-bit info ti,clkdm-clock: requires ti,clkdm-name clkdm clock is kind of a master clock for clockdomain, the clock is provided always if the clockdomain is active. Ok. +- ti,enable-bit : bit shift for programming the clock gate Why is this needed? Does the hardware vary wildly, or are several clocks sharing the same register(s)? Yea, same register is shared. Ok. Are those gate clocks are part of a larger gate clocks block, with the register that controls them? or are they really independent? Does the register control other items too? If they were part of some bigger unit, it might be possible to have a binding like the following (though maybe not): gate_clks: gate_clks { compatible = ti,gate-clocks-block; reg = 0x4003a000 0x1000; #clock-cells = 1; /* * has 4 gate clocks at bit shifts 0,1,2,3, * corresponding to input clocks 0,1,2,3 */ clocks = clk1 0 23, clk1 0 4, clk3 2, clk3 5; }; device { compatible = vendor,some-device; clocks = gate_clks 1; /* gate clock with bitshift 1*/ }; +- ti,dss-clk : use DSS hardware OPS for the clock +- ti,am35xx-clk : use AM35xx hardware OPS for the clock Those last two sounds like the kind of thing that should be derived from the compatible string (e.g. ti,am35xx-gate-clock). Hmm yea, I think I can change this and add some sub-types. +- ti,clkdm-name : clockdomain to control this gate As previously mentioned, I'm not a fan of this property. It would make more sense to describe the domain and have an explicit link to it (either nodes being children of the domain or having a phandle). Same comments as with patch #2. Same reply as to patch #2 :) [...] +void __init of_omap_gate_clk_setup(struct device_node *node) +{ + struct clk *clk; + struct clk_init_data init = { 0 }; + struct clk_hw_omap *clk_hw; + const char *clk_name = node-name; + int num_parents; + const char **parent_names = NULL; + int i; + u32 val; + + clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL); + if (!clk_hw) { + pr_err(%s: could not allocate clk_hw_omap\n, __func__); + return; + } + + clk_hw-hw.init = init; + + of_property_read_string(node, clock-output-names, clk_name); + of_property_read_string(node, ti,clkdm-name, clk_hw-clkdm_name); + + init.name = clk_name; + init.flags = 0; + + if (of_property_read_u32_index(node, reg, 0, val)) { + /* No register, clkdm control only */ + init.ops
Re: [PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions
On Fri, Aug 16, 2013 at 07:12:46PM +0100, Benoit Cousson wrote: Hi Mark, On 16/08/2013 19:20, Mark Rutland wrote: Hi Benoit, On Fri, Aug 16, 2013 at 03:15:57PM +0100, Benoit Cousson wrote: Hi Gururaja, On 16/08/2013 13:36, Hebbar, Gururaja wrote: The syntax of compatible property in DT is to mention the Most specific match to most generic match. Since AM335x is the platform with latest IP revision, add it 1st in the device id table. I don't understand why? The order should not matter at all. I've tried to follow the thread you had with Mark on the v2, but AFAIK, you've never answered to his latest question. Moreover, checking the differences between the Davinci and the am3352 RTC IP, I would not claim that both are compatible. Sure you can use the am3352 with the Davinci driver, but you will lose the wakeup functionality without even being notify about that. Could you describe the wakeup functionality, and how it differs between the am3352-rtc and the da830-rtc? AFAIK, da830-rtc does not have that functionality at all. This is something that was added to the am3352-rtc. Ok. So the am3352-rtc can be driven with the full functionality of the da830-rtc (ie. it's compatible with the da830-rtc programming model), or it can be driven as an am3352-rtc, for the OS to gain wakeup functionality in addition to the da830-rtc features. :) As I understand it, the am3352 functionality is a superset of the da830 functionality. You can use the old driver, and get some functionality, or use the new driver and get it all. Mmm, what your are saying now seems to make sense to me as well. So I'm even more confused :-) I'll convince you yet :) That means that am3352-rtc is compatible with da830. As long as the kernel first checks for am3352-rtc, there will be *no* loss of functionality. All this does is enable older kernels to use the hardware in some fashion, and given the older kernel didn't have support for the am3352-rtc features, this is a *gain* in functionality, not a loss. For my point of view, compatible mean that the HW will still be fully functional with both versions of the driver, which is not the case here. What? A driver for any entry in the compatible list should be able to drive the hardware to *some* level of functionality. We list from most-specific to most-general to allow a graceful degradation from fully supported to bare minimum functionality. OK, but where is it written in the DT spec that this is what the compatible is supposed to mean? I'm quoting it again: The compatible property value consists of one or more strings that define the specific programming model for the device. This list of strings should be used by a client program for device driver selection. The property value consists of a concatenated list of null terminated strings, from most specific to most general. They allow a device to express its compatibility with a family of similar devices, potentially allowing a single device driver to match against several devices. The graceful degradation or the loss of functionality is not something that I really understand in that text. I think it's implicit in the example that follows, where a failure to match against a specific device results in the OS falling back to a more general device. The more general device may not have all the features of a more specific device (conversely, the more general device may have more optional features that a more specific device is known not to implement). Anyway, I'm probably too tired... I'll go back home, and think about that after the week-end. Ok, let me know what you think. :) Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5 05/31] CLK: TI: add support for OMAP gate clock
On Mon, Aug 19, 2013 at 03:43:15PM +0100, Tero Kristo wrote: On 08/19/2013 05:29 PM, Mark Rutland wrote: On Mon, Aug 19, 2013 at 02:42:05PM +0100, Tero Kristo wrote: On 08/13/2013 02:04 PM, Mark Rutland wrote: On Fri, Aug 02, 2013 at 05:25:24PM +0100, Tero Kristo wrote: This node adds support for a clock node which allows control to the clockdomain enable / disable. Signed-off-by: Tero Kristo t-kri...@ti.com --- .../devicetree/bindings/clock/ti/gate.txt | 41 arch/arm/mach-omap2/clock.h|9 -- drivers/clk/ti/Makefile|2 +- drivers/clk/ti/gate.c | 106 include/linux/clk/ti.h |8 ++ 5 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/gate.txt create mode 100644 drivers/clk/ti/gate.c diff --git a/Documentation/devicetree/bindings/clock/ti/gate.txt b/Documentation/devicetree/bindings/clock/ti/gate.txt new file mode 100644 index 000..620a73d --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/gate.txt @@ -0,0 +1,41 @@ +Binding for Texas Instruments gate clock. + +This binding uses the common clock binding[1]. This clock is +quite much similar to the basic gate-clock [2], however, +it supports a number of additional features. If no register +is provided for this clock, the code assumes that a clockdomain +will be controlled instead and the corresponding hw-ops for +that is used. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt +[2] Documentation/devicetree/bindings/clock/gate-clock.txt + +Required properties: +- compatible : shall be ti,gate-clock +- #clock-cells : from common clock binding; shall be set to 0 +- clocks : link to phandle of parent clock + +Optional properties: +- reg : base address for register controlling adjustable gate Optional? That's odd. If I have a clock with registers, but don't specify the register, will it still work? i.e. are registerless clocks really compatible with clocks with registers?. I think I implemented this in somewhat confusing manner. This could be split to: ti,gate-clock: requires reg and ti,enable-bit info ti,clkdm-clock: requires ti,clkdm-name clkdm clock is kind of a master clock for clockdomain, the clock is provided always if the clockdomain is active. Ok. +- ti,enable-bit : bit shift for programming the clock gate Why is this needed? Does the hardware vary wildly, or are several clocks sharing the same register(s)? Yea, same register is shared. Ok. Are those gate clocks are part of a larger gate clocks block, with the register that controls them? or are they really independent? Does the register control other items too? Not really. Typically they only have the clockdomain in common, and the individual clocks are mostly controlled independently from each other. For example on omap3 we have following register: You say they typically only have the clockdomain in common. Do you mean that they always have the same clockdomain, but not necessarily other properties, or may they not even have the clockdomain in common? CM_FCLKEN_PER Physical Address 0x4800 5000 BIT31:19 RESERVED Write 0s for future compatibility. Read returns 0. BIT18 EN_UART4 UART4 functional clock control. BIT17 EN_GPIO6 GPIO6 functional clock control. BIT16 EN_GPIO5 GPIO5 functional clock control. BIT15 EN_GPIO4 GPIO4 functional clock control. BIT14 EN_GPIO3 GPIO3 functional clock control. BIT13 EN_GPIO2 GPIO2 functional clock control. BIT12 EN_WDT3 WDT3 functional clock control. BIT11 EN_UART3 Type UART3 functional clock control. BIT10 EN_GPT9 GPTIMER 9 functional clock control. BIT9 EN_GPT8 GPTIMER 8 functional clock control. BIT8 EN_GPT7 GPTIMER 7 functional clock control. BIT7 EN_GPT6 GPTIMER 6 functional clock control. BIT6 EN_GPT5 GPTIMER 5 functional clock control. BIT5 EN_GPT4 GPTIMER 4 functional clock control. BIT4 EN_GPT3GPTIMER 3 functional clock control. BIT3 EN_GPT2 GPTIMER 2 functional clock control. BIT2 EN_MCBSP4 McBSP 4 functional clock control. BIT1 EN_MCBSP3 McBSP3 functional clock control. BIT0 EN_MCBSP2 McBSP 2 functional clock control. So multiple drivers will be using this register for example. The point I was trying to get across is that this looks like a single logical block which controls the (independent) gating of several clocks, along the same lines as multiple swtiches bound together in a DIP switch. It's equally valid to view that as several clock gates which happen to have their control bits in close proximity in the memory map, as you suggest. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info
Re: [PATCHv5 02/31] CLK: TI: Add DPLL clock support
On Mon, Aug 19, 2013 at 04:09:37PM +0100, Tero Kristo wrote: On 08/19/2013 05:18 PM, Mark Rutland wrote: On Mon, Aug 19, 2013 at 02:34:45PM +0100, Tero Kristo wrote: On 08/13/2013 01:50 PM, Mark Rutland wrote: Hi, On Fri, Aug 02, 2013 at 05:25:21PM +0100, Tero Kristo wrote: The OMAP clock driver now supports DPLL clock type. This patch also adds support for DT DPLL nodes. Signed-off-by: Tero Kristo t-kri...@ti.com --- .../devicetree/bindings/clock/ti/dpll.txt | 70 +++ arch/arm/mach-omap2/clock.h| 144 +- arch/arm/mach-omap2/clock3xxx.h|2 - drivers/clk/Makefile |1 + drivers/clk/ti/Makefile|3 + drivers/clk/ti/dpll.c | 488 include/linux/clk/ti.h | 164 +++ 7 files changed, 727 insertions(+), 145 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/ti/dpll.txt create mode 100644 drivers/clk/ti/Makefile create mode 100644 drivers/clk/ti/dpll.c create mode 100644 include/linux/clk/ti.h diff --git a/Documentation/devicetree/bindings/clock/ti/dpll.txt b/Documentation/devicetree/bindings/clock/ti/dpll.txt new file mode 100644 index 000..dcd6e63 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/dpll.txt @@ -0,0 +1,70 @@ +Binding for Texas Instruments DPLL clock. + +This binding uses the common clock binding[1]. It assumes a +register-mapped DPLL with usually two selectable input clocks +(reference clock and bypass clock), with digital phase locked +loop logic for multiplying the input clock to a desired output +clock. This clock also typically supports different operation +modes (locked, low power stop etc.) This binding has several +sub-types, which effectively result in slightly different setup +for the actual DPLL clock. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +Required properties: +- compatible : shall be one of: + ti,omap4-dpll-x2-clock, + ti,omap3-dpll-clock, + ti,omap3-dpll-core-clock, + ti,omap3-dpll-per-clock, + ti,omap3-dpll-per-j-type-clock, + ti,omap4-dpll-clock, + ti,omap4-dpll-core-clock, + ti,omap4-dpll-m4xen-clock, + ti,omap4-dpll-j-type-clock, + ti,omap4-dpll-no-gate-clock, + ti,omap4-dpll-no-gate-j-type-clock, + +- #clock-cells : from common clock binding; shall be set to 0. +- clocks : link phandles of parent clocks (clk-ref and clk-bypass) It might be a good idea to use clock-names to clarify which clocks are present (I notice your examples contain only one clock input). All DPLLs have both bypass and reference clocks, but these can be the same. Is it better to just list both always (and hence drop clock-names), or provide clock-names always? If they're separate inputs, it's worth listing both (even if they're fed by the same provider). If it's possible one input might not be wired up, use clock-names. Ref and bypass clocks are used currently by all DPLLs (also the APLL) so I guess I just enforce both to be specified. Ok. It's always possible to add clock-names later if a platform doesn't wire an input. We lose the possibility of future compatibility, but backwards compatibility is easy enough to maintain. +- ti,clkdm-name : clockdomain name for the DPLL Could you elaborate on what this is for? What constitutes a valid name? I'm not sure a string is the best way to define the linkage of several elements to a domain... Well, I am not sure if we can do this any better at this point, we don't have DT nodes for clockdomain at the moment (I am not sure if we will have those either as there are only a handful of those per SoC) but I'll add some more documentation for this. I'll have to see the documentation, but I'd be very wary of putting that in as-is. Does having the clock domain string link this up to domains in platform data? I'm not sure how well we'll be able to maintain support for that in future if it requires other platform code to use now, and we're not sure how the domains themselves will be represented in dt. Hmm so, should I add a stub representation for the clockdomains to the DT then for now or how should this be handled? The stubs could then be mapped to the actual clock domains based on their node names. I unfortunately don't have a good answer here, because I'm not that familiar with how we handle clockdomains for power management purposes. As I understand it, each clock domain is essentially a clock gate controlling multiple clock signals, so it's possible to describe that with the common clock bindings, with a domain's clocks
Re: [PATCH v2 1/2] ARM: dts: Add SHAM data and documentation for AM33XX
On Wed, Jul 17, 2013 at 05:23:41PM +0100, Mark A. Greer wrote: From: Mark A. Greer mgr...@animalcreek.com Add the generic AM33XX SHAM module's device tree data and enable it for the am335x-evm, am335x-evmsk, and am335x-bone platforms. Also add Documentation file describing the data for the SHAM module. CC: Paul Walmsley p...@pwsan.com Signed-off-by: Mark A. Greer mgr...@animalcreek.com --- .../devicetree/bindings/crypto/omap-sham.txt | 33 ++ arch/arm/boot/dts/am335x-bone.dts | 4 +++ arch/arm/boot/dts/am335x-evm.dts | 4 +++ arch/arm/boot/dts/am335x-evmsk.dts | 4 +++ arch/arm/boot/dts/am33xx.dtsi | 10 +++ 5 files changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/omap-sham.txt diff --git a/Documentation/devicetree/bindings/crypto/omap-sham.txt b/Documentation/devicetree/bindings/crypto/omap-sham.txt new file mode 100644 index 000..c6d1202 --- /dev/null +++ b/Documentation/devicetree/bindings/crypto/omap-sham.txt @@ -0,0 +1,33 @@ +OMAP SoC SHA crypto Module + +Required properties: + +- compatible : Should contain entries for this and backward compatible + SHAM versions: + - ti,omap2-sham for OMAP2 OMAP3. + - ti,omap4-sham for OMAP4 and AM33XX. + Note that these two versions are incompatible. +- ti,hwmods: Name of the hwmod associated with the SHAM module +- reg : Offset and length of the register set for the module +- interrupt-parent : the phandle for the interrupt controller that + services interrupts for this module. +- interrupts : the interrupt number for the SHAM module. + +Optional properties: +- dmas: DMA controller phandle and DMA request ordered pair. + Only one rx pair is valid per SHAM module. This may be a little late, but... Nit: A dma specifier may have many cells, so calling it pair is not necessarily correct: dmas = dma0 432 7 5, dma1 3, dma0 212 1 13; You could instead say: - dmas: DMA specifier for the rx dma. See the DMA client binding, Documentation/devicetree/bindings/dma/dma.txt +- dma-names: DMA request name. This string corresponds 1:1 with + the ordered pair in dmas. The string naming is to be + rx for RX request. Similarly: - dma-names: DMA request name. Should be rx if a dma is present. It would be nice to get the bindings using consistent terminology so we don't confuse everyone further. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
Hi, I have a few comments, mostly on the DT binding and parsing. diff --git a/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt b/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt new file mode 100644 index 000..5d465cf --- /dev/null +++ b/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt @@ -0,0 +1,39 @@ +* IRQ CROSSBAR + +Some socs have a large number of interrupts requests to service +the needs of its many peripherals and subsystems. All of the +interrupt lines from the subsystems are not needed at the same +time, so they have to be muxed to the irq-controller appropriately. +In such places a interrupt controllers are preceded by an CROSSBAR +that provides flexibility in muxing the device requests to the controller +inputs. + +Required properties: +- compatible : Should be irq-crossbar Missing vendor prefix, this should be something like ti,irq-crossbar. Does this have a more specific name than CROSSBAR that can be used to qualify it? +- interrupt-parent: phandle to crossbar's interrupt parent. +- interrupt-controller: Identifies the node as an interrupt controller. +- interrupt-cells: Should be the same value as the interrupt parent. That doesn't make sense. The crossbar driver is necessarily interpreting these cells in a way the parent won't (as it supports more interrupts). What are the meaning of these cells? +- reg: Base address and the size of the crossbar registers. +- max-crossbar-lines: Total number of input lines of the crossbar. +- max-irqs: Total number of irqs available at the interrupt controller. Is this the maximum number of interrupts targeting the parent interrupt controller? Starting at what number, ending at what number? Can this have gaps? Is this a shortcut so in the GIC case you don't have to describe up to 160 interrupts? I can see why you don't want to, but there's a big loss of generality here... +- reg-size: size of the crossbar registers. As in the size of all the registers (the size component of reg)? Or is this the size of each individual register? Does that apply to all registers or only a subset of them? What units are these in, bytes? What are valid sizes? Is this really that configurable? +- irqs-reserved: List of the reserved irq lines that are not muxed using +crossbar. These interrupt lines are reserved in the soc, +so crossbar bar driver should not consider them as free +lines. Are these reserved inputs lines, or outputs to the parent interrupt controller? What is the format of each entry in this list? The example seems to be a different format to the parent interrupt controller (which per your binding also defined the crossbar's interrupt format). While 0 1 2 is a valid interrupt per the GIC binding (SPI 0 edge-triggered both ways), 3 5 6, 131 132 139, and 140 . . are not. + +Examples: + crossbar_mpu: @4a02 { + compatible = irq-crossbar; + interrupt-parent = gic; + interrupt-controller; + #interrupt-cells = 3; + reg = 0x4a002a48 0x130; + max-crossbar-lines = 512; + max-irqs = 160; + reg-size = 2; + irqs-reserved = 0 1 2 3 5 6 131 132 139 140; + #address-cells = 1; + #size-cells = 1; Why are there #address-cells and #size cells? This has no children, and this affects any interrupt-map property (as the parent unit address now must be a single cell, that isn't going to be used). [...] +static int crossbar_set_affinity(struct irq_data *d, +const struct cpumask *mask_val, +bool force) +{ + struct irq_chip *chip; + struct irq_data *data; + int ret = 0; + + crossbar_to_irq_chip_data(d-hwirq, chip, data); + + if (chip-irq_set_affinity) + ret = chip-irq_set_affinity(data, mask_val, force); + + return ret; So if our parent chip can't set affinity, we pretend it can? irq_set_affinity in kernel/irq/manage.c returns -EINVAL if an irqchip doesn't have irq_set_affinity. +/* + * Request and free are already called in atomic contexts + */ +unsigned int crossbar_request_irq(struct irq_data *d) +{ + int cb_no = d-hwirq; + int virq = allocate_free_irq(cb_no); + void *irq = cb-crossbar_map[cb_no].hwirq; + int err; + + err = request_threaded_irq(virq, crossbar_irq, NULL, + 0, CROSSBAR, irq); + if (err) + pr_err(\n request_irq failed for crossbar %d, cb_no); Why does the print begin with a newline rather than ending with one? + + return 0; +} [...] +static int crossbar_domain_xlate(struct irq_domain *d, +struct
Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
On Wed, Sep 25, 2013 at 10:29:03PM +0100, Brian Norris wrote: On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote: On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris computersforpe...@gmail.com wrote: Olof has given good advice on your DT binding and has (slowly) been responding to other requests for DT review that make it to his list. I see that he hasn't followed up on your changes (this v6), so pinging him (as you did) is probably the correct approach. But please do recognize that the DT list is very high volume, so it's hard to get good timely responses there. I am not a DT mainainer, but sometimes when I see a binding that appears to be wrong I speak up. In this case, the binding was one of those. Whoops, my bad. I was deceived by the responses I've seen from you on other issues (thanks, BTW). In that case, I haven't seen any response from a proper DT binding maintainer :( Hmm... this is the first email in this thread I've received, and I don't have older postings either. It looks like I must have cocked up subscribing to the devicetree list, but as I was CC'd directly on many patches I hadn't noticed. As far as I can see I wasn't CC'd directly on any version of this series, which would have helped to highlight the series as needing review. Apologies for that. I've attempted to correct the problem. Hopefully I've got this right and mails are not being silently dropped somewhere. Pekon, could you please re-send this version of the patches? Cheers, Mark. So, I have no more objections to it, and I hope you can get a quick review from a DT maintainer on the rest of the binding. At this point, I'm comfortable going ahead without their ack, since they obviously don't care/don't have the manpower to review. Brian -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] usb: dwc3: msm: Add device tree binding information
On Mon, Sep 23, 2013 at 08:31:48PM +0100, Felipe Balbi wrote: Hi, On Tue, Aug 20, 2013 at 12:56:03PM +0300, Ivan T. Ivanov wrote: From: Ivan T. Ivanov iiva...@mm-sol.com MSM USB3.0 core wrapper consist of USB3.0 IP from Synopsys (SNPS) and HS, SS PHY's control and configuration registers. It could operate in device mode (SS, HS, FS) and host mode (SS, HS, FS, LS). Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com Any acks for the DT part ? This patch has been pending forever. Apologies for the delay. I have a couple of comments that would be nice to fix up now. --- .../devicetree/bindings/usb/msm-ssusb.txt | 104 1 file changed, 104 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt b/Documentation/devicetree/bindings/usb/msm-ssusb.txt new file mode 100644 index 000..cacbd3b --- /dev/null +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt @@ -0,0 +1,104 @@ +MSM SuperSpeed DWC3 USB SoC controller + + +DWC3 Highspeed USB PHY +== +Required properities : +- compatible : sould be qcom,dwc3-hsphy; s/sould/should/ In general, compatible properties are should contain rather than should be, as we might have backwards compatible hardware in future. +- reg : offset and length of the register set in the memory map +- clocks : phandles to clock instances of the device tree nodes Clocks are referred to be phandle + clock-specifier pairs rather than phandles, it would be nice to fix the terminology here +- clock-names : + xo : External reference clock 19 MHz + sleep_a : Sleep clock, used when USB3 core goes into low + power mode (U3). I think it would be nicer to say: - clocks : A list of phandle + clock-specifier pairs for the clocks listed in clock-names - clock-names: Should contain the following: xo - External reference clock (19MHz) sleep_a - Sleep clock used when USB3 core goes into low power mode (U3). I'm not sure we need to describe the frequency of the xo clock -- either it's a requriement of the IP and thus doesn't need to be a part of the binding, or it's configurable by the driver and thus doesn't need to be documented. +supply-name-supply : phandle to the regulator device tree node +Required supply-name are: + v1p8 : 1.8v supply for HSPHY + v3p3 : 3.3v supply for HSPHY + vbus : vbus supply for host mode + vddcx : vdd supply for HS-PHY digital circuit operation Any reason for the HSPHY/HS-PHY difference? I'd list these separately with their full names: - v1p8-supply: phandle to the regulator for the 1.8v supply to HSPHY. - v3p3-supply: phandle to the regulator for the 3.3v supply to HSPHY. - vbus-supply: phandle to the regulator for the vbus supply for host mode. - vddcx-supply: phandle to the regulator for the vdd supply for HSPHY digital circuit operation. + +DWC3 Superspeed USB PHY +=== +Required properities : +- compatible : sould be qcom,dwc3-ssphy; +- reg : offset and length of the register set in the memory map +- clocks : phandles to clock instances of the device tree nodes +- clock-names : + xo : External reference clock 19 MHz + ref : Reference clock - used in host mode. +supply-name-supply : phandle to the regulator device tree node +Required supply-name are: + v1p8 : 1.8v supply for SS-PHY + vddcx : vdd supply for SS-PHY digital circuit operation The commments on compatible, clocks, clock-names and the regulators apply here. + +DWC3 controller wrapper +=== +Required properties : +- compatible : should be qcom,dwc3 +- reg : offset and length of the register set in the memory map + offset and length of the TCSR register for routing USB + signals to either picoPHY0 or picoPHY1. +- clocks : phandles to clock instances of the device tree nodes +- clock-names : + core : Master/Core clock, have to be = 125 MHz for SS + operation and = 60MHz for HS operation + iface : System bus AXI clock + sleep : Sleep clock, used when USB3 core goes into low + power mode (U3). + utmi : Generated by HS-PHY. Used to clock the low power + parts of thr HS Link layer. +Optional properties : +- gdsc-supply : phandle to the globally distributed switch controller + regulator node to the USB controller. The commments on compatible, clocks, and clock-names apply here too. I see the regulator is defined individually :) I'm fine with the binding itself, I'd just like the documentation fixed up. Cheers, Mark. +Required child node: +A child node must exist to represent the core DWC3 IP block. The name of +the node is not important. The content of the node is defined in dwc3.txt. + +Example device nodes: + +
Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
only matter of which path to take. Request you to please help me finalize this before I resend next patch series addressing other comments from Brian. + Mark Rutland mark.rutl...@arm.com + Pawel Moll pawel.m...@arm.com + Ian Campbell ijc+devicet...@hellion.org.uk + Stephen Warren swar...@wwwdotorg.org CC other DT maintainers, who were missed in this branch of mail-chain. with regards, pekon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/9] hwspinlock/core: add common dt bindings and OF helpers
Hi Suman, Apologies for replying to a subthread, due to an earlier mistake on my part I don't have the original to hand. On Fri, Sep 27, 2013 at 05:04:22PM +0100, Kumar Gala wrote: On Sep 17, 2013, at 2:30 PM, Suman Anna wrote: All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core. Signed-off-by: Suman Anna s-a...@ti.com --- .../devicetree/bindings/hwlock/hwlock.txt | 26 + drivers/hwspinlock/hwspinlock_core.c | 61 +- include/linux/hwspinlock.h | 11 ++-- 3 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt diff --git a/Documentation/devicetree/bindings/hwlock/hwlock.txt b/Documentation/devicetree/bindings/hwlock/hwlock.txt new file mode 100644 index 000..789930e --- /dev/null +++ b/Documentation/devicetree/bindings/hwlock/hwlock.txt @@ -0,0 +1,26 @@ +Generic hwlock bindings +=== + +Generic bindings that are common to all the hwlock platform specific driver +implementations, the retrieved values are used for registering the device +specific parameters with the hwspinlock core. + +The validity and need of these common properties may vary from one driver +implementation to another. Look through the individual hwlock driver +binding documentations for identifying which are mandatory and which are +optional for that specific driver. + +Common properties: +- hwlock-base-id: Base Id for the locks for a particular hwlock device. + This property is used for representing a set of locks + present in a hwlock device with a unique base id in + the driver core. This property is mandatory ONLY if a + SoC has several hwlock devices. + + See documentation on struct hwspinlock_pdata in + linux/hwspinlock.h for more details. I don't like this, as it seems to be encoding a Linux implementation detail (the ID of the lock, which means that we have to have a numeric representation of each hwlock) in the device tree. I don't see why the ID within Linux should be a concern of the device tree binding. I think that whatever internal identifier we have in Linux (be it an integer or struct) should be allocated by Linux. If a driver needs to request specific hwlocks, then we should have a phandle + args representation for referring to a specific hwlock that bidnings can use, and some code for parsing that and returning a Linux-internal identifier/struct as we do with interrupts and clocks. + +- hwlock-num-locks:Number of locks present in a hwlock device. This + property is needed on hwlock devices, where the number + of supported locks within a hwlock device cannot be + read from a register. Hmm... this seems generic, but it doesn't allow for sparse ID spaces on the hwlock module. It should probably be common for the moment, but if we encounter a hwlock module with a sparse ID space, we'll need to come up with a way of handling sparse IDs (that might be device-specific). diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c index 461a0d7..aec32e7 100644 --- a/drivers/hwspinlock/hwspinlock_core.c +++ b/drivers/hwspinlock/hwspinlock_core.c @@ -1,7 +1,7 @@ /* * Hardware spinlock framework * - * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com + * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com * * Contact: Ohad Ben-Cohen o...@wizery.com * @@ -27,6 +27,7 @@ #include linux/hwspinlock.h #include linux/pm_runtime.h #include linux/mutex.h +#include linux/of.h #include hwspinlock_internal.h @@ -308,6 +309,64 @@ out: } /** + * of_hwspin_lock_get_base_id() - OF helper to retrieve base id + * @dn: device node pointer + * + * This is an OF helper function that can be called by the underlying + * platform-specific implementations, to retrieve the base id for the + * set of locks present within a hwspinlock device instance. + * + * Returns the base id value on success, -ENODEV on generic failure or + * an appropriate error code as returned by the OF layer + */ +int of_hwspin_lock_get_base_id(struct device_node *dn) +{ + unsigned int val; + int ret = -ENODEV; + +#ifdef CONFIG_OF + if (!dn) + return -ENODEV; Checking !dn is done in of_property_read_u32, so you don't need to duplicate
Re: [PATCHv2 2/9] hwspinlock/omap: add support for dt nodes
Hi Suman, On Fri, Sep 27, 2013 at 05:06:38PM +0100, Kumar Gala wrote: On Sep 17, 2013, at 2:30 PM, Suman Anna wrote: HwSpinlock IP is present only on OMAP4 and other newer SoCs, which are all device-tree boot only. This patch adds the base support for parsing the DT nodes, and removes the code dealing with the traditional platform device instantiation. Signed-off-by: Suman Anna s-a...@ti.com --- .../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++ arch/arm/mach-omap2/Makefile | 3 -- arch/arm/mach-omap2/hwspinlock.c | 60 -- drivers/hwspinlock/omap_hwspinlock.c | 23 +++-- 4 files changed, 50 insertions(+), 67 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c diff --git a/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file mode 100644 index 000..235b7c5 --- /dev/null +++ b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt @@ -0,0 +1,31 @@ +OMAP4+ HwSpinlock Driver + + +Required properties: +- compatible: Currently supports only ti,omap4-hwspinlock for + OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs Currently supports is not something I expect to see in a binding document. That sounds like a description of the driver rather than the binding. How similar are these hardware modules? What are the differences? +- reg: Contains the hwspinlock register address range (base + address and length) Is there only one register bank for the hwlock module? +- ti,hwmods: Name of the hwmod associated with the hwspinlock device + +Common hwlock properties: +The following describes the usage of the common hwlock properties (defined in +Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP. + +- hwlock-base-id: There are currently no OMAP SoCs with multiple + hwspinlock devices. The OMAP driver uses a default + base id value of 0 for the locks present within the + single hwspinlock device on all SoCs. Driver details should not leak into bindngs... As mentioned in the other patch, I don't think this is the way to handle this. I think we need a phandle + args representation. +- hwlock-num-locks:This property is not required on OMAP SoCs, since the + number of locks present within a device can be deduced + from the SPINLOCK_SYSSTATUS device register. Huh? Why define this property at all here if we don't need it and don't use it? The common document should state that specific bindings may use it and should explicitly state if they do, rather than stating they don't... Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/9] ARM: dts: Add SHAM data and documentation for AM33XX
On Mon, Sep 30, 2013 at 04:13:00PM +0100, Joel Fernandes wrote: From: Mark A. Greer mgr...@animalcreek.com Add the generic AM33XX SHAM module's device tree data and enable it for the am335x-evm, am335x-evmsk, and am335x-bone platforms. Also add Documentation file describing the data for the SHAM module. [jo...@ti.com: Dropped interrupt-parrent property] CC: Paul Walmsley p...@pwsan.com Signed-off-by: Mark A. Greer mgr...@animalcreek.com --- .../devicetree/bindings/crypto/omap-sham.txt | 31 ++ arch/arm/boot/dts/am335x-bone.dts | 4 +++ arch/arm/boot/dts/am335x-evm.dts | 4 +++ arch/arm/boot/dts/am335x-evmsk.dts | 4 +++ arch/arm/boot/dts/am33xx.dtsi | 9 +++ 5 files changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/omap-sham.txt diff --git a/Documentation/devicetree/bindings/crypto/omap-sham.txt b/Documentation/devicetree/bindings/crypto/omap-sham.txt new file mode 100644 index 000..b97710f --- /dev/null +++ b/Documentation/devicetree/bindings/crypto/omap-sham.txt @@ -0,0 +1,31 @@ +OMAP SoC SHA crypto Module + +Required properties: + +- compatible : Should contain entries for this and backward compatible + SHAM versions: + - ti,omap2-sham for OMAP2 OMAP3. + - ti,omap4-sham for OMAP4 and AM33XX. + Note that these two versions are incompatible. +- ti,hwmods: Name of the hwmod associated with the SHAM module +- reg : Offset and length of the register set for the module +- interrupt-parent : the phandle for the interrupt controller that + services interrupts for this module. I don't think this is strictly speaking necessary -- it's mostly going to be implicit (it is in the dtsi below). As this is a standard property, you don't need to document it here. +- interrupts : the interrupt number for the SHAM module. Sorry, I missed this last time, but this should be interrupt-specifier rather than interrupt number. Otherwise, this looks good to me. With the fixups above: Acked-by: Mark Rutland mark.rutl...@arm.com + +Optional properties: +- dmas: DMA specifier for the rx dma. See the DMA client binding, + Documentation/devicetree/bindings/dma/dma.txt +- dma-names: DMA request name. Should be rx if a dma is present. + +Example: + /* AM335x */ + sham: sham@5310 { + compatible = ti,omap4-sham; + ti,hwmods = sham; + reg = 0x5310 0x200; + interrupt-parent = intc; + interrupts = 109; + dmas = edma 36; + dma-names = rx; + }; diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts index 0d63348..8a9802e 100644 --- a/arch/arm/boot/dts/am335x-bone.dts +++ b/arch/arm/boot/dts/am335x-bone.dts @@ -19,3 +19,7 @@ mmc1 { vmmc-supply = ldo3_reg; }; + +sham { + status = okay; +}; diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts index 23b0a3e..d59e51c 100644 --- a/arch/arm/boot/dts/am335x-evm.dts +++ b/arch/arm/boot/dts/am335x-evm.dts @@ -522,3 +522,7 @@ status = okay; vmmc-supply = vmmc_reg; }; + +sham { + status = okay; +}; diff --git a/arch/arm/boot/dts/am335x-evmsk.dts b/arch/arm/boot/dts/am335x-evmsk.dts index bc93895..d45a330 100644 --- a/arch/arm/boot/dts/am335x-evmsk.dts +++ b/arch/arm/boot/dts/am335x-evmsk.dts @@ -424,3 +424,7 @@ status = okay; vmmc-supply = vmmc_reg; }; + +sham { + status = okay; +}; diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 553adc6..299710b 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -710,5 +710,14 @@ #size-cells = 1; status = disabled; }; + + sham: sham@5310 { + compatible = ti,omap4-sham; + ti,hwmods = sham; + reg = 0x5310 0x200; + interrupts = 109; + dmas = edma 36; + dma-names = rx; + }; }; }; -- 1.8.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/9] ARM: dts: Add AES data and documentation for AM33XX
On Mon, Sep 30, 2013 at 04:13:01PM +0100, Joel Fernandes wrote: From: Mark A. Greer mgr...@animalcreek.com Add the generic AM33XX AES module's device tree data and enable it for the am335x-evm, am335x-evmsk, and am335x-bone platforms. Also add Documentation file describing the data for the AES module. [jo...@ti.com: Dropped interrupt-parent propert] CC: Paul Walmsley p...@pwsan.com Signed-off-by: Mark A. Greer mgr...@animalcreek.com --- .../devicetree/bindings/crypto/omap-aes.txt| 34 ++ arch/arm/boot/dts/am335x-bone.dts | 4 +++ arch/arm/boot/dts/am335x-evm.dts | 4 +++ arch/arm/boot/dts/am335x-evmsk.dts | 4 +++ arch/arm/boot/dts/am33xx.dtsi | 10 +++ 5 files changed, 56 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/omap-aes.txt diff --git a/Documentation/devicetree/bindings/crypto/omap-aes.txt b/Documentation/devicetree/bindings/crypto/omap-aes.txt new file mode 100644 index 000..4bb1e27 --- /dev/null +++ b/Documentation/devicetree/bindings/crypto/omap-aes.txt @@ -0,0 +1,34 @@ +OMAP SoC AES crypto Module + +Required properties: + +- compatible : Should contain entries for this and backward compatible + AES versions: + - ti,omap2-aes for OMAP2. + - ti,omap3-aes for OMAP3. + - ti,omap4-aes for OMAP4 and AM33XX. + Note that the OMAP2 and 3 versions are compatible (OMAP3 supports + more algorithms) but they are incompatible with OMAP4. +- ti,hwmods: Name of the hwmod associated with the AES odule +- reg : Offset and length of the register set for the module +- interrupt-parent : the phandle for the interrupt controller that + services interrupts for this module. +- interrupts : the interrupt number for the AES odule. Similar comments to the SHAM module here: * s/interrupt number/interrupt-specifier/ * Drop interrupt-parent. * s/AES odule/AES module/ + +Optional properties: +- dmas: DMA specifier for tx and rx dma. See the DMA client binding, + Documentation/devicetree/bindings/dma/dma.txt s/DMA specifier/DMA specifiers/ +- dma-names: DMA request names. Should be 'tx, rx' if dma is present. Nit: I'd prefer 'Should include tx and rx if present' -- I hope the driver's requesting these by name rather than relying on a specific ordering (it makes future expansion and optional components far easier to handle sanely). + +Example: + /* AM335x */ + aes: aes@5350 { + compatible = ti,omap4-aes; + ti,hwmods = aes; + reg = 0x5350 0xa0; + interrupt-parent = intc; + interrupts = 102; + dmas = edma 6 + edma 5; + dma-names = tx, rx; + }; Minor nit, but for consistency could you bracket the DMAs individually: dmas = edma 6, edma 5; diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts index 8a9802e..94ee427 100644 --- a/arch/arm/boot/dts/am335x-bone.dts +++ b/arch/arm/boot/dts/am335x-bone.dts @@ -23,3 +23,7 @@ sham { status = okay; }; + +aes { + status = okay; +}; diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts index d59e51c..86463fa 100644 --- a/arch/arm/boot/dts/am335x-evm.dts +++ b/arch/arm/boot/dts/am335x-evm.dts @@ -526,3 +526,7 @@ sham { status = okay; }; + +aes { + status = okay; +}; diff --git a/arch/arm/boot/dts/am335x-evmsk.dts b/arch/arm/boot/dts/am335x-evmsk.dts index d45a330..f577e65 100644 --- a/arch/arm/boot/dts/am335x-evmsk.dts +++ b/arch/arm/boot/dts/am335x-evmsk.dts @@ -428,3 +428,7 @@ sham { status = okay; }; + +aes { + status = okay; +}; diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 299710b..0daa1b2 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -719,5 +719,15 @@ dmas = edma 36; dma-names = rx; }; + + aes: aes@5350 { + compatible = ti,omap4-aes; + ti,hwmods = aes; + reg = 0x5350 0xa0; + interrupts = 102; + dmas = edma 6 + edma 5; Bracketing here too, please. Cheers, Mark. + dma-names = tx, rx; + }; }; }; -- 1.8.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/6] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes
On Fri, Oct 04, 2013 at 08:49:43PM +0100, Pekon Gupta wrote: OMAP NAND driver currently supports multiple flavours of 1-bit Hamming ecc-scheme, like: - OMAP_ECC_HAMMING_CODE_DEFAULT 1-bit hamming ecc code using software library - OMAP_ECC_HAMMING_CODE_HW 1-bit hamming ecc-code using GPMC h/w engine - OMAP_ECC_HAMMING_CODE_HW_ROMCODE 1-bit hamming ecc-code using GPMC h/w engin with ecc-layout compatible to ROM code. This patch combines above multiple ecc-schemes into single implementation: - OMAP_ECC_HAM1_CODE_HW 1-bit hamming ecc-code using GPMC h/w engine with ROM-code compatible ecc-layout. If I have my NAND formatted with one of the existing ECC schemes (e.g. OMAP_ECC_HAMMING_CODE_DEFAULT) will it work with the new OMAP_ECC_HAM1_CODE_HW scheme? Are they all compatible? Signed-off-by: Pekon Gupta pe...@ti.com --- Documentation/devicetree/bindings/mtd/gpmc-nand.txt | 8 arch/arm/mach-omap2/board-flash.c | 2 +- arch/arm/mach-omap2/gpmc.c | 4 +--- drivers/mtd/nand/omap2.c| 9 +++-- include/linux/platform_data/mtd-nand-omap2.h| 6 +- 5 files changed, 10 insertions(+), 19 deletions(-) diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt index df338cb..25ee232 100644 --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt @@ -22,10 +22,10 @@ Optional properties: width of 8 is assumed. - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: - - swSoftware method (default) - hwHardware method - hw-romcodegpmc hamming mode method romcode layout + swdeprecated use ham1 instead + hwdeprecated use ham1 instead + hw-romcodedeprecated use ham1 instead + ham1 1-bit Hamming ecc code bch4 4-bit BCH ecc code bch8 8-bit BCH ecc code diff --git a/arch/arm/mach-omap2/board-flash.c b/arch/arm/mach-omap2/board-flash.c index fc20a61..ac82512 100644 --- a/arch/arm/mach-omap2/board-flash.c +++ b/arch/arm/mach-omap2/board-flash.c @@ -142,7 +142,7 @@ __init board_nand_init(struct mtd_partition *nand_parts, u8 nr_parts, u8 cs, board_nand_data.nr_parts= nr_parts; board_nand_data.devsize = nand_type; - board_nand_data.ecc_opt = OMAP_ECC_HAMMING_CODE_DEFAULT; + board_nand_data.ecc_opt = OMAP_ECC_BCH8_CODE_HW; gpmc_nand_init(board_nand_data, gpmc_t); } #endif /* CONFIG_MTD_NAND_OMAP2 || CONFIG_MTD_NAND_OMAP2_MODULE */ diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index 9f4795a..1c45b72 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -1342,9 +1342,7 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np, #ifdef CONFIG_MTD_NAND static const char * const nand_ecc_opts[] = { - [OMAP_ECC_HAMMING_CODE_DEFAULT] = sw, - [OMAP_ECC_HAMMING_CODE_HW] = hw, - [OMAP_ECC_HAMMING_CODE_HW_ROMCODE] = hw-romcode, + [OMAP_ECC_HAM1_CODE_HW] = ham1, [OMAP_ECC_BCH4_CODE_HW] = bch4, [OMAP_ECC_BCH8_CODE_HW] = bch8, Won't this break existing dts which have sw, hw, or hw-romcode? Someone may try to use a new kernel with an old dt, and we marked them as deprecated, not removed. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/6] ARM: OMAP2+: cleaned-up DT support of various ECC schemes
On Fri, Oct 04, 2013 at 08:49:44PM +0100, Pekon Gupta wrote: OMAP NAND driver support multiple ECC scheme, which can used in following different flavours, depending on in-build Hardware engines supported by SoC. +---+---+---+ | ECC scheme|ECC calculation|Error detection| +---+---+---+ |OMAP_ECC_HAM1_CODE_HW |H/W (GPMC) |S/W| +---+---+---+ |OMAP_ECC_BCH8_CODE_HW_DETECTION_SW |H/W (GPMC) |S/W| |(requires CONFIG_MTD_NAND_ECC_BCH) | | | +---+---+---+ |OMAP_ECC_BCH8_CODE_HW |H/W (GPMC) |H/W (ELM) | |(requires CONFIG_MTD_NAND_OMAP_BCH | | | | ti,elm-id in DT) | | | +---+---+---+ To optimize footprint of omap2-nand driver, selection of some ECC schemes also require enabling following Kconfigs, in addition to setting appropriate DT bindings - Kconfig:CONFIG_MTD_NAND_ECC_BCHerror detection done in software - Kconfig:CONFIG_MTD_NAND_OMAP_BCH error detection done by h/w engine DT binding updates in this patch are: - ti,elm-id: replaces elm_id - ti,nand-ecc-opts: supported values ham1, bch4, and bch8 selection of h/w or s/w implementation depends on ti,elm-id Signed-off-by: Pekon Gupta pe...@ti.com --- .../devicetree/bindings/mtd/gpmc-nand.txt | 8 +++- arch/arm/mach-omap2/gpmc.c | 47 -- include/linux/platform_data/mtd-nand-omap2.h | 14 +-- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt index 25ee232..7785666 100644 --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt @@ -36,8 +36,12 @@ Optional properties: prefetch-dma Prefetch enabled sDMA mode prefetch-irq Prefetch enabled irq mode - - elm_id: Specifies elm device node. This is required to support BCH - error correction using ELM module. + - elm_id: deprecated use ti,elm-id instead + - ti,elm-id:Specifies pHandle of the ELM devicetree node. s/pHandle/phandle/ + ELM is an on-chip hardware engine on TI SoC which is used for + locating ECC errors for BCHx algorithms. SoC devices which have + ELM hardware engines should specify this device node in .dtsi + Using ELM for ECC error correction frees some CPU cycles. For inline partiton table parsing (optional): diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index 1c45b72..5a607fa 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -1341,12 +1341,6 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np, #ifdef CONFIG_MTD_NAND -static const char * const nand_ecc_opts[] = { - [OMAP_ECC_HAM1_CODE_HW] = ham1, - [OMAP_ECC_BCH4_CODE_HW] = bch4, - [OMAP_ECC_BCH8_CODE_HW] = bch8, -}; - static const char * const nand_xfer_types[] = { [NAND_OMAP_PREFETCH_POLLED] = prefetch-polled, [NAND_OMAP_POLLED] = polled, @@ -1361,6 +1355,8 @@ static int gpmc_probe_nand_child(struct platform_device *pdev, const char *s; struct gpmc_timings gpmc_t; struct omap_nand_platform_data *gpmc_nand_data; + const __be32 *phandle; I don't think you need this. With of_parse_phandle you should never need to see the raw phandle value. + int lenp; if (of_property_read_u32(child, reg, val) 0) { dev_err(pdev-dev, %s has no 'reg' property\n, @@ -1376,12 +1372,39 @@ static int gpmc_probe_nand_child(struct platform_device *pdev, gpmc_nand_data-cs = val; gpmc_nand_data-of_node = child; - if (!of_property_read_string(child, ti,nand-ecc-opt, s)) - for (val = 0; val ARRAY_SIZE(nand_ecc_opts); val++) - if (!strcasecmp(s, nand_ecc_opts[val])) { - gpmc_nand_data-ecc_opt = val; - break; - } + /* Detect availability of ELM module */ + phandle = of_get_property(child, ti,elm-id, lenp); + if ((phandle == NULL) || (lenp != sizeof(void *))) { + pr_warn(%s: ti,elm-id property not found\n, __func__); + gpmc_nand_data-elm_of_node = NULL; + } else { + gpmc_nand_data-elm_of_node
Re: [PATCH v7 5/6] ARM: dts: AM33xx: updated default ECC scheme in nand-ecc-opt
On Fri, Oct 04, 2013 at 08:49:47PM +0100, Pekon Gupta wrote: DT property values for OMAP based gpmc-nand have been updated to match changes in commit: 6faf096 ARM: OMAP2+: cleaned-up DT support of various ECC schemes Refer: Documentation/devicetree/bindings/mtd/gpmc-nand.txt Doesn't this mean these dts were broken between the changes to the driver and the changes to the dts? I think the existing properties need to continue to be supoprted for backwards compatibility. The dts can be fixed up to have the preferred binding style, but they shouldn't _have_ to be modified to continue to function... Thanks, Mark. Signed-off-by: Pekon Gupta pe...@ti.com --- arch/arm/boot/dts/am335x-evm.dts | 3 +-- arch/arm/boot/dts/omap3430-sdp.dts | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts index e8ec875..1aee6ac 100644 --- a/arch/arm/boot/dts/am335x-evm.dts +++ b/arch/arm/boot/dts/am335x-evm.dts @@ -269,7 +269,6 @@ reg = 0 0 0; /* CS0, offset 0 */ nand-bus-width = 8; ti,nand-ecc-opt = bch8; - gpmc,device-nand = true; gpmc,device-width = 1; gpmc,sync-clk-ps = 0; gpmc,cs-on-ns = 0; @@ -296,7 +295,7 @@ #address-cells = 1; #size-cells = 1; - elm_id = elm; + ti,elm-id = elm; /* MTD partition table */ partition@0 { diff --git a/arch/arm/boot/dts/omap3430-sdp.dts b/arch/arm/boot/dts/omap3430-sdp.dts index e2249bc..501f863 100644 --- a/arch/arm/boot/dts/omap3430-sdp.dts +++ b/arch/arm/boot/dts/omap3430-sdp.dts @@ -105,7 +105,7 @@ reg = 1 0 0x0800; nand-bus-width = 8; - ti,nand-ecc-opt = sw; + ti,nand-ecc-opt = ham1; gpmc,cs-on-ns = 0; gpmc,cs-rd-off-ns = 36; gpmc,cs-wr-off-ns = 36; -- 1.8.1 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v3 03/11] ASoC: davinci-mcasp: Add DMA register locations to DT
Hello, On Thu, Sep 26, 2013 at 08:18:28PM +0100, Jyri Sarha wrote: This patch adds DMA register location to mcasp DT bindings. On am33xx SoCs the McASP registers are mapped trough L4 interconnect, which is not accessible by the DMA controller, so McASP data port is mapped trough L3 to a different location. Signed-off-by: Hebbar, Gururaja gururaja.heb...@ti.com Signed-off-by: Darren Etheridge detheri...@ti.com Signed-off-by: Jyri Sarha jsa...@ti.com --- .../bindings/sound/davinci-mcasp-audio.txt |8 ++- sound/soc/davinci/davinci-mcasp.c | 59 +--- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt index 374e145..63b67ae 100644 --- a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt +++ b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt @@ -6,7 +6,11 @@ Required properties: ti,da830-mcasp-audio : for both DA830 DA850 platforms ti,omap2-mcasp-audio : for OMAP2 platforms (TI81xx, AM33xx) -- reg : Should contain McASP registers offset and length +- reg : Should contain McASP registers address and length for mpu and + optionally for dma controller access. +- reg-names : The mandatory reg-range must be named mpu and the optional DMA + reg-range must be named dma. For backward compatibility it is + good to keep mpu first in the list. I've never heard the term reg-range before. The should probably be something like reg entry. How about something like the following instead: - reg: Should contain reg specifiers for the entries in the reg-names property. - reg-names: Should contain: * mpu for the main registers (required). For compatibility with existing software, it is recommended this is the first entry. * dma for the DMA registers (optional). That way we don't end up describing each reg entry twice. I have some questions however. I took a look at the McASP (TMS320C6000) reference guide, and the registers appeared to all be in one contiguous bank, and mpu and dma don't appear to be names of particular registers or names of banks of particular registers. Am I looking in the wrong place? Is there up-to-date documentation I can look at? Why are these split across two reg entries, and which particular registers do they actually cover? I have some concern about the description of other properties too. If we're going to amend the binding, they should be fixed up too. - interrupts : Interrupt number for McASP The device also seems to be able to generate multiple interrupts -- which interrupt does this actually cover? The driver doesn't seem to use it (by a grep of irq|interrupt). Have I missed something? - op-mode : I2S/DIT ops mode. What type is this? What are valid values? - tdm-slots : Slots for TDM operation. Here too. This description is completely opaque. Taking a look at the version in kernel sources this appears to be a list of values, in groups of four? What is the format of this property? @@ -15,7 +19,6 @@ Required properties: to num-serializer parameter. Each entry is a number indication serializer pin direction. (0 - INACTIVE, 1 - TX, 2 - RX) - Optional properties: - ti,hwmods : Must be mcaspn, n is controller instance starting 0 @@ -31,6 +34,7 @@ mcasp0: mcasp0@1d0 { #address-cells = 1; #size-cells = 0; Why does this have #address-cells and #size-cells? There are no children in the example. reg = 0x10 0x3000; + reg-names mpu; interrupts = 82 83; op-mode = 0; /* MCASP_IIS_MODE */ tdm-slots = 2; A few questions from a brief skim of the documentation: There seem to be external clocks (AHCLKR and ACLKR), but they aren't described. Are we always able to use an internal clock instead? Is no external reference clock needed? The err_release_clk label in the error path confuses me -- which clock(s) does the preceding pm_runtime_get ensure remains active? One internal to the McASP? It looks like some pins can be used as GPIO -- is there not a need for something like pinctrl for configuring this? Cheers, Mark. diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 32ddb7f..a056fc5 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -1001,18 +1001,40 @@ static const struct snd_soc_component_driver davinci_mcasp_component = { .name = davinci-mcasp, }; +/* Some HW specific values and defaults. The rest is filled in from DT. */ +static struct snd_platform_data dm646x_mcasp_pdata = { + .tx_dma_offset = 0x400, + .rx_dma_offset = 0x400, + .asp_chan_q = EVENTQ_0, + .version = MCASP_VERSION_1, +}; + +static struct snd_platform_data
Re: [RESEND PATCH v3 04/11] ASoC: davinci-mcasp: Extract DMA channels directly from DT
On Thu, Sep 26, 2013 at 08:18:29PM +0100, Jyri Sarha wrote: Extract DMA channels directly from DT as they can not be found from platform resources anymore. This is a work-around until davinci audio driver is updated to use dmaengine. How long will this conversion take? Signed-off-by: Jyri Sarha jsa...@ti.com --- .../bindings/sound/davinci-mcasp-audio.txt |5 +++ include/linux/platform_data/davinci_asp.h |2 + sound/soc/davinci/davinci-mcasp.c | 47 +--- 3 files changed, 39 insertions(+), 15 deletions(-) diff --git a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt index 63b67ae..68e0f47 100644 --- a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt +++ b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt @@ -18,6 +18,11 @@ Required properties: - serial-dir : A list of serializer pin mode. The list number should be equal to num-serializer parameter. Each entry is a number indication serializer pin direction. (0 - INACTIVE, 1 - TX, 2 - RX) +- dmas: two element list of DMA controller phandles and DMA request line +ordered pairs. Please describe this in terms of dma-names. That makes it clear that elements cannot be retrieved consistently by index, and prevents duplicate descriptions. I'd prefer for the sake of consistent terminology that these were referred to as phandles + dma-specifiers rather than phandles and DMA request lines -- #dma-cells may be an arbitrary number of cells and encode arbitrary information for some abstract DMA engine. +- dma-names: identifier string for each DMA request line in the dmas property. + These strings correspond 1:1 with the ordered pairs in dmas. The dma + identifiers must be rx and tx. For consistency and future expandability: s/must be/should contain/ Cheers, Mark. Optional properties: diff --git a/include/linux/platform_data/davinci_asp.h b/include/linux/platform_data/davinci_asp.h index 8db5ae0..689a856 100644 --- a/include/linux/platform_data/davinci_asp.h +++ b/include/linux/platform_data/davinci_asp.h @@ -84,6 +84,8 @@ struct snd_platform_data { u8 version; u8 txnumevt; u8 rxnumevt; + int tx_dma_channel; + int rx_dma_channel; }; enum { diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index a056fc5..acbf5f8 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -1047,6 +1047,7 @@ static struct snd_platform_data *davinci_mcasp_set_pdata_from_of( struct snd_platform_data *pdata = NULL; const struct of_device_id *match = of_match_device(mcasp_dt_ids, pdev-dev); + struct of_phandle_args dma_spec; const u32 *of_serial_dir32; u8 *of_serial_dir; @@ -1109,6 +1110,28 @@ static struct snd_platform_data *davinci_mcasp_set_pdata_from_of( pdata-serial_dir = of_serial_dir; } + ret = of_property_match_string(np, dma-names, tx); + if (ret 0) + goto nodata; + + ret = of_parse_phandle_with_args(np, dmas, #dma-cells, ret, + dma_spec); + if (ret 0) + goto nodata; + + pdata-tx_dma_channel = dma_spec.args[0]; + + ret = of_property_match_string(np, dma-names, rx); + if (ret 0) + goto nodata; + + ret = of_parse_phandle_with_args(np, dmas, #dma-cells, ret, + dma_spec); + if (ret 0) + goto nodata; + + pdata-rx_dma_channel = dma_spec.args[0]; + ret = of_property_read_u32(np, tx-num-evt, val); if (ret = 0) pdata-txnumevt = val; @@ -1139,7 +1162,7 @@ nodata: static int davinci_mcasp_probe(struct platform_device *pdev) { struct davinci_pcm_dma_params *dma_data; - struct resource *mem, *ioarea, *res; + struct resource *mem, *ioarea, *res, *dma; struct snd_platform_data *pdata; struct davinci_audio_dev *dev; int ret; @@ -1213,15 +1236,11 @@ static int davinci_mcasp_probe(struct platform_device *pdev) dma_data-sram_size = pdata-sram_size_playback; dma_data-dma_addr = dma-start + pdata-tx_dma_offset; - /* first TX, then RX */ res = platform_get_resource(pdev, IORESOURCE_DMA, 0); - if (!res) { - dev_err(pdev-dev, no DMA resource\n); - ret = -ENODEV; - goto err_release_clk; - } - - dma_data-channel = res-start; + if (res) + dma_data-channel = res-start; + else + dma_data-channel = pdata-tx_dma_channel; dma_data = dev-dma_params[SNDRV_PCM_STREAM_CAPTURE]; dma_data-asp_chan_q = pdata-asp_chan_q; @@ -1231,13 +1250,11 @@ static int
Re: [RESEND PATCH v3 05/11] ASoC: davinci-mcasp: Interrupts property to optional and add interrupt-names
On Thu, Sep 26, 2013 at 08:18:30PM +0100, Jyri Sarha wrote: Makes interrupts property optional as the interrupts are not currently used by the driver and adds interrupt-names property to name listed interrupts. Currently know interrupt names are tx and rx. Signed-off-by: Jyri Sarha jsa...@ti.com --- .../bindings/sound/davinci-mcasp-audio.txt |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt index 68e0f47..2fd0bf2 100644 --- a/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt +++ b/Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt @@ -11,7 +11,6 @@ Required properties: - reg-names : The mandatory reg-range must be named mpu and the optional DMA reg-range must be named dma. For backward compatibility it is good to keep mpu first in the list. -- interrupts : Interrupt number for McASP - op-mode : I2S/DIT ops mode. - tdm-slots : Slots for TDM operation. - num-serializer : Serializers used by McASP. @@ -31,6 +30,8 @@ Optional properties: - rx-num-evt : FIFO levels. - sram-size-playback : size of sram to be allocated during playback - sram-size-capture : size of sram to be allocated during capture +- interrupts : Interrupt numbers for McASP, currently not used by the driver +- interrupt-names : Known interrupt names are tx and rx Are these _all_ the interrupts the McASP may generate? I was under the impression there were also separate interrupts for errors. Cheers, Mark. Example: @@ -41,6 +42,7 @@ mcasp0: mcasp0@1d0 { reg = 0x10 0x3000; reg-names mpu; interrupts = 82 83; + interrupts-names = tx, rx; op-mode = 0; /* MCASP_IIS_MODE */ tdm-slots = 2; num-serializer = 16; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v3 10/11] ARM/dts: am33xx: mcasp: Add new dma register location to reg-property
On Thu, Sep 26, 2013 at 08:18:35PM +0100, Jyri Sarha wrote: This patch adds an optional address range to reg property. The range describes the register location for DMA controller on am33xx. The both address ranges are named accordingly in the reg-names property. Signed-off-by: Hebbar, Gururaja gururaja.heb...@ti.com Signed-off-by: Darren Etheridge detheri...@ti.com Signed-off-by: Jyri Sarha jsa...@ti.com --- arch/arm/boot/dts/am33xx.dtsi | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index fe53ce0..4dc388a 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -556,19 +556,29 @@ mcasp0: mcasp@48038000 { compatible = ti,omap2-mcasp-audio; ti,hwmods = mcasp0; - reg = 0x48038000 0x2000; + reg = 0x48038000 0x2000, + 0x4640 0x40; + reg-names = mpu, dma; interrupts = 80 81; interrupts-names = tx, rx; status = disabled; + dmas = edma 8 + edma 9; For consistency with reg and other composite value properties, I'd prefer that each entry in the list were individually bracketed: dmas = edma 8, edma 9; It would also be nice if interrupts were written this way. + dma-names = tx, rx; }; mcasp1: mcasp@4803C000 { compatible = ti,omap2-mcasp-audio; ti,hwmods = mcasp1; - reg = 0x4803C000 0x2000; + reg = 0x4803C000 0x2000, + 0x4640 0x40; + reg-names = mpu, dma; interrupts = 82 83; interrupts-names = tx, rx; status = disabled; + dmas = edma 10 + edma 11; Similarly here. Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v3 11/11] ARM/dts: am335x-evm: Add audio support for am335x-evm.dts
On Thu, Sep 26, 2013 at 08:18:36PM +0100, Jyri Sarha wrote: From: Darren Etheridge detheri...@ti.com Adds sound, tlv320aic3x, mcasp1, and am335x_evm_audio_pin nodes. Signed-off-by: Darren Etheridge detheri...@ti.com Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com Signed-off-by: Jyri Sarha jsa...@ti.com --- arch/arm/boot/dts/am335x-evm.dts | 56 ++ 1 file changed, 56 insertions(+) diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts index 3aee1a4..4a49229 100644 --- a/arch/arm/boot/dts/am335x-evm.dts +++ b/arch/arm/boot/dts/am335x-evm.dts @@ -149,6 +149,16 @@ 0x14c (PIN_INPUT_PULLDOWN | MUX_MODE7) ; }; + + am335x_evm_audio_pins: am335x_evm_audio_pins { + pinctrl-single,pins = + 0x10c (PIN_INPUT_PULLDOWN | MUX_MODE4) /* mii1_rx_dv.mcasp1_aclkx */ + 0x110 (PIN_INPUT_PULLDOWN | MUX_MODE4) /* mii1_txd3.mcasp1_fsx */ + 0x108 (PIN_OUTPUT_PULLDOWN | MUX_MODE4) /* mii1_col.mcasp1_axr2 */ + 0x144 (PIN_INPUT_PULLDOWN | MUX_MODE4) /* rmii1_ref_clk.mcasp1_axr3 */ + ; + }; + }; ocp { @@ -215,6 +225,19 @@ compatible = ti,tmp275; reg = 0x48; }; + + tlv320aic3x: tlv320aic3x@1b { + compatible = ti,tlv320aic3x; + reg = 0x1b; + status = okay; + + /* Regulators */ + AVDD-supply = vaux2_reg; + IOVDD-supply = vaux2_reg; + DRVDD-supply = vaux2_reg; + DVDD-supply = vbat; + }; + }; elm: elm@4808 { @@ -311,6 +334,20 @@ }; }; }; + + sound { + compatible = ti,da830-evm-audio; + ti,model = DA830 EVM; + ti,audio-codec = tlv320aic3x; + ti,mcasp-controller = mcasp1; + ti,codec-clock-rate = 1200; + ti,audio-routing = + Headphone Jack, HPLOUT, + Headphone Jack, HPROUT, + LINE1L, Line In, + LINE1R, Line In; + }; + }; vbat: fixedregulator@0 { @@ -378,6 +415,25 @@ #include tps65910.dtsi +mcasp1 { + pinctrl-names = default; + pinctrl-0 = am335x_evm_audio_pins; I didn't see mention of pinctrl added to the binding. It should be. Thanks, Mark. + + status = okay; + + op-mode = 0; /* MCASP_IIS_MODE */ + tdm-slots = 2; + num-serializer = 16; + serial-dir = /* 0: INACTIVE, 1: TX, 2: RX */ + 0 0 1 2 + 0 0 0 0 + 0 0 0 0 + 0 0 0 0 + ; + tx-num-evt = 1; + rx-num-evt = 1; +}; + tps { vcc1-supply = vbat; vcc2-supply = vbat; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 1/9] hwspinlock/core: add common dt bindings and OF helpers
On Thu, Oct 03, 2013 at 05:04:15AM +0100, Suman Anna wrote: Hi Mark, On 10/01/2013 03:36 AM, Mark Rutland wrote: Hi Suman, Apologies for replying to a subthread, due to an earlier mistake on my part I don't have the original to hand. No issues, thanks for your review. On Fri, Sep 27, 2013 at 05:04:22PM +0100, Kumar Gala wrote: On Sep 17, 2013, at 2:30 PM, Suman Anna wrote: All the platform-specific hwlock driver implementations need the number of locks and the associated base id for registering the locks present within a hwspinlock device with the driver core. These two variables are represented by 'hwlock-num-locks' and 'hwlock-base-id' properties. The documentation and OF helpers to retrieve these common properties have been added to the driver core. Signed-off-by: Suman Anna s-a...@ti.com --- .../devicetree/bindings/hwlock/hwlock.txt | 26 + drivers/hwspinlock/hwspinlock_core.c | 61 +- include/linux/hwspinlock.h | 11 ++-- 3 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt diff --git a/Documentation/devicetree/bindings/hwlock/hwlock.txt b/Documentation/devicetree/bindings/hwlock/hwlock.txt new file mode 100644 index 000..789930e --- /dev/null +++ b/Documentation/devicetree/bindings/hwlock/hwlock.txt @@ -0,0 +1,26 @@ +Generic hwlock bindings +=== + +Generic bindings that are common to all the hwlock platform specific driver +implementations, the retrieved values are used for registering the device +specific parameters with the hwspinlock core. + +The validity and need of these common properties may vary from one driver +implementation to another. Look through the individual hwlock driver +binding documentations for identifying which are mandatory and which are +optional for that specific driver. + +Common properties: +- hwlock-base-id:Base Id for the locks for a particular hwlock device. + This property is used for representing a set of locks + present in a hwlock device with a unique base id in + the driver core. This property is mandatory ONLY if a + SoC has several hwlock devices. + + See documentation on struct hwspinlock_pdata in + linux/hwspinlock.h for more details. I don't like this, as it seems to be encoding a Linux implementation detail (the ID of the lock, which means that we have to have a numeric representation of each hwlock) in the device tree. I don't see why the ID within Linux should be a concern of the device tree binding. I think that whatever internal identifier we have in Linux (be it an integer or struct) should be allocated by Linux. If a driver needs to request specific hwlocks, then we should have a phandle + args representation for referring to a specific hwlock that bidnings can use, and some code for parsing that and returning a Linux-internal identifier/struct as we do with interrupts and clocks. This is based on gathering the information required by the platform implementation drivers for registering with the driver core. The driver core currently maintains all the locks from different instances as a radix tree, as it is simpler to manage when granting locks to users. The current version is based on allowing the platform implementation drivers to retrieve the required data for registering with the hwspinlock driver core. Ok. I don't see why this implementation detail needs to leak into the dt. The users would either get a lock dynamically by requesting any free one (and extract the id thereafter to share with others), or a specific one which is currently by id. I agree that the phandle + args approach is best suited for requesting a specific one through DT, but I would think that the args would still have to be a relative lock number from 0 w.r.t a phandle. I will look into the feasibility of such an approach retaining the radix tree implementation, as this requires new OF friendly registration and request functions. The value in the args would be a unique identifier within the unit pointed to be the phandle, but the mechanism by which it would refer to a particular lock would be binding-specific. It's perfectly fine for this to be an zero-based index in most bindings, but it should not be a global namespace as with the hwlock-base-id property approach. + +- hwlock-num-locks: Number of locks present in a hwlock device. This + property is needed on hwlock devices, where the number + of supported locks within a hwlock device cannot be + read from a register. Hmm... this seems generic, but it doesn't allow for sparse ID spaces on the hwlock module. It should
Re: [PATCHv2 2/9] hwspinlock/omap: add support for dt nodes
On Thu, Oct 03, 2013 at 05:12:15AM +0100, Suman Anna wrote: Hi Mark, On Fri, Sep 27, 2013 at 05:06:38PM +0100, Kumar Gala wrote: On Sep 17, 2013, at 2:30 PM, Suman Anna wrote: HwSpinlock IP is present only on OMAP4 and other newer SoCs, which are all device-tree boot only. This patch adds the base support for parsing the DT nodes, and removes the code dealing with the traditional platform device instantiation. Signed-off-by: Suman Anna s-a...@ti.com --- .../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++ arch/arm/mach-omap2/Makefile | 3 -- arch/arm/mach-omap2/hwspinlock.c | 60 -- drivers/hwspinlock/omap_hwspinlock.c | 23 +++-- 4 files changed, 50 insertions(+), 67 deletions(-) create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt delete mode 100644 arch/arm/mach-omap2/hwspinlock.c diff --git a/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt new file mode 100644 index 000..235b7c5 --- /dev/null +++ b/Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt @@ -0,0 +1,31 @@ +OMAP4+ HwSpinlock Driver + + +Required properties: +- compatible:Currently supports only ti,omap4-hwspinlock for + OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs Currently supports is not something I expect to see in a binding document. That sounds like a description of the driver rather than the binding. How similar are these hardware modules? What are the differences? The IP is almost the same, they all have the same revision id. The number of locks (each represented by a register) though vary from one SoC to another (OMAP4, OMAP5, DRA7 have same number of locks, and AM33xx/AM43xx have a different number). The number of locks is directly read by the driver from a module register. There is no separate .data associated with the of_device_id table, so I used a single compatible property for all the SoCs. Ok. Probeability is good, it keeps these simpler :) I think This can be reworded to say should contain rather than currently supports only: - compatible: Should contain ti,omap4-hwspinlock for OMAP44xx, OMAP54xx, AM33xx, AM43xx, or DRA7xx SoCs That way the binding allows for a future backwards-compatible variant, and doesn't mention the current level of support in Linux. +- reg: Contains the hwspinlock register address range (base + address and length) Is there only one register bank for the hwlock module? The lock registers start at a certain offset (0x800) within the module register space, and the offsets for various registers are identical between all SoCs. What are the other registers within the module? Are they shared with other devices, or are they simply unused by the hwspinlock driver? +- ti,hwmods: Name of the hwmod associated with the hwspinlock device + +Common hwlock properties: +The following describes the usage of the common hwlock properties (defined in +Documentation/devicetree/bindings/hwlock/hwlock.txt) on OMAP. + +- hwlock-base-id:There are currently no OMAP SoCs with multiple + hwspinlock devices. The OMAP driver uses a default + base id value of 0 for the locks present within the + single hwspinlock device on all SoCs. Driver details should not leak into bindngs... OK, will remove the info on driver details. As mentioned in the other patch, I don't think this is the way to handle this. I think we need a phandle + args representation. This is an optional parameter for now and I was going to revise the description based on comments from Kumar Gala on this thread, but I will wait and adjust this based on the outcome on the first patch. Ok. +- hwlock-num-locks: This property is not required on OMAP SoCs, since the + number of locks present within a device can be deduced + from the SPINLOCK_SYSSTATUS device register. Huh? Why define this property at all here if we don't need it and don't use it? The common document should state that specific bindings may use it and should explicitly state if they do, rather than stating they don't... Yeah, I wasn't sure how to go about the split between the common file and the platform-specific bindings. I will clean this up and revise the common bindings. Ok. Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] arm: dts: omap4+: Add DMM bindings
On Thu, Oct 10, 2013 at 07:36:33AM +0100, Archit Taneja wrote: Add Dynamic Memory Manager (DMM) bindings for OMAP4 and OMAP5 devices. DMM only requires address and irq information. Add documentation for the DMM bindings. Originally worked on by Andy Gross andy...@gmail.com Cc: Andy Gross andy...@gmail.com Signed-off-by: Archit Taneja arc...@ti.com --- Documentation/devicetree/bindings/arm/omap/dmm.txt | 16 arch/arm/boot/dts/omap4.dtsi | 7 +++ arch/arm/boot/dts/omap5.dtsi | 7 +++ 3 files changed, 30 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/omap/dmm.txt diff --git a/Documentation/devicetree/bindings/arm/omap/dmm.txt b/Documentation/devicetree/bindings/arm/omap/dmm.txt new file mode 100644 index 000..6fc3d79 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/omap/dmm.txt @@ -0,0 +1,16 @@ +OMAP Dynamic Memory Manager (DMM) bindings Is there any documentation? A brief description of what this is would be nice. + +Required properties: +- compatible:Must be ti,omap4-dmm for OMAP4 family + Must be ti,omap5-dmm for OMAP5 and DRA7x family s/must be/should contain/ +- reg: Contains timer register address range (base address and length) Huh? What's a timer got to do with the DMM? +- interrupts:Contains interrupt information (source, etc) for the DMM IRQ Is there a single interrupt? If so: - interrupts: Should contain an interrupt-specifier for the DMM IRQ. Assuming the DMM IRQ is well defined. If there's a name for it in documentation, using that's preferable. If a future revision may have multiple interrupts, please use interrupt-names now to save us endless pain in future. Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v3 03/11] ASoC: davinci-mcasp: Add DMA register locations to DT
On Tue, Oct 08, 2013 at 01:46:41AM +0100, Mark Brown wrote: On Mon, Oct 07, 2013 at 10:47:18PM +0100, Mark Rutland wrote: On Thu, Sep 26, 2013 at 08:18:28PM +0100, Jyri Sarha wrote: - interrupts : Interrupt number for McASP The device also seems to be able to generate multiple interrupts -- which interrupt does this actually cover? The driver doesn't seem to use it (by a grep of irq|interrupt). Have I missed something? No, they're not actually of much practical use to us at the minute but it was generally felt better to include the information and not use it so that if someone does come up with a use for them then the trees for deployed systems already have the information. Sure, but if this device can generate multiple interrupts, we should make it possible to describe all of them, unambiguously. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v3 03/11] ASoC: davinci-mcasp: Add DMA register locations to DT
On Thu, Oct 10, 2013 at 06:29:59PM +0100, Peter Ujfalusi wrote: On 10/10/2013 07:59 PM, Mark Rutland wrote: No, they're not actually of much practical use to us at the minute but it was generally felt better to include the information and not use it so that if someone does come up with a use for them then the trees for deployed systems already have the information. Sure, but if this device can generate multiple interrupts, we should make it possible to describe all of them, unambiguously. This is why Jyri added them to the DT. They are not used by the Linux driver, but the HW have interrupt lines (two of them: TX and RX). The binding only describes a single interrupt, and even if multiple interrupts were listed, there's no way to disambiguate them (e.g. interrupt-names). It would be nice to remedy this. Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] mmc: omap_hsmmc: start using generic non-removable DT binding
On Wed, Oct 16, 2013 at 04:18:22PM +0100, Balaji T K wrote: From: Sekhar Nori nsek...@ti.com add generic non-removable binding support for omap_hsmmc Signed-off-by: Sekhar Nori nsek...@ti.com Signed-off-by: Balaji T K balaj...@ti.com --- .../devicetree/bindings/mmc/ti-omap-hsmmc.txt |2 +- drivers/mmc/host/omap_hsmmc.c |3 +++ 2 files changed, 4 insertions(+), 1 deletions(-) diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt index 8c8908a..3b95719 100644 --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt @@ -17,7 +17,7 @@ Optional properties: ti,dual-volt: boolean, supports dual voltage cards supply-name-supply: phandle to the regulator device tree node supply-name examples are vmmc, vmmc_aux etc -ti,non-removable: non-removable slot (like eMMC) +ti,non-removable: non-removable eMMC with always on vccq and configurable vcc Why this change? What do vccq and vcc correspond to? The regulators are called vmmc and vmmc_aux... Why is no mention of non-removable added, given that it's added to the code? Is one preferred over the other? That should be noted. ti,needs-special-reset: Requires a special softreset sequence ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed dmas: List of DMA specifiers with the controller specific format diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 6ac63df..5992048 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -1738,6 +1738,9 @@ static struct omap_mmc_platform_data *of_get_hsmmc_pdata(struct device *dev) pdata-slots[0].switch_pin = cd_gpio; pdata-slots[0].gpio_wp = wp_gpio; + if (of_find_property(np, non-removable, NULL)) { + pdata-slots[0].nonremovable = true; + } This wasn't mentioned in the binding, and it seems to have different semantics to ti,non-removable. Why is it different? if (of_find_property(np, ti,non-removable, NULL)) { pdata-slots[0].nonremovable = true; pdata-slots[0].no_regulator_off_init = true; Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] mmc: omap_hsmmc: start using generic non-removable DT binding
On Thu, Oct 17, 2013 at 11:53:48AM +0100, Balaji T K wrote: On Thursday 17 October 2013 02:08 PM, Mark Rutland wrote: On Wed, Oct 16, 2013 at 04:18:22PM +0100, Balaji T K wrote: From: Sekhar Nori nsek...@ti.com add generic non-removable binding support for omap_hsmmc Signed-off-by: Sekhar Nori nsek...@ti.com Signed-off-by: Balaji T K balaj...@ti.com --- .../devicetree/bindings/mmc/ti-omap-hsmmc.txt |2 +- drivers/mmc/host/omap_hsmmc.c |3 +++ 2 files changed, 4 insertions(+), 1 deletions(-) diff --git a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt index 8c8908a..3b95719 100644 --- a/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt +++ b/Documentation/devicetree/bindings/mmc/ti-omap-hsmmc.txt @@ -17,7 +17,7 @@ Optional properties: ti,dual-volt: boolean, supports dual voltage cards supply-name-supply: phandle to the regulator device tree node supply-name examples are vmmc, vmmc_aux etc -ti,non-removable: non-removable slot (like eMMC) +ti,non-removable: non-removable eMMC with always on vccq and configurable vcc Why this change? Hi, earlier ti,non-removable was used for all eMMC and SDIO card, now it will be used only for eMMC with always on vccq and configurable vcc. Please expand the commit message to mention this. It wasn't clear why adding support for a property meant modifying the description of another. What do vccq and vcc correspond to? The regulators are called vmmc and vmmc_aux... vccq and vcc are supply names of eMMC part The binding has vmmc-supply and vmmc_aux-supply. How do {vmmc,vmmc_aux} and {vcc,vccq} relate? That should be clarified in the binding document, something like: - vmmc-supply: phandle of the regulator for the VCC input - vmmc_aux-supply: phandle of the regulator for the VCCQ input Why is no mention of non-removable added, given that it's added to the code? Because this file makes a reference to mmc.txt and the core properties described by mmc.txt are not added in ti-omap-hsmmc.txt There is room for confusion here. While non-removable is a generic property, it would be good to contrast non-removable and ti,non-removable in the binding as they imply different things. Is one preferred over the other? That should be noted. ti,needs-special-reset: Requires a special softreset sequence ti,needs-special-hs-handling: HSMMC IP needs special setting for handling High Speed dmas: List of DMA specifiers with the controller specific format diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 6ac63df..5992048 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -1738,6 +1738,9 @@ static struct omap_mmc_platform_data *of_get_hsmmc_pdata(struct device *dev) pdata-slots[0].switch_pin = cd_gpio; pdata-slots[0].gpio_wp = wp_gpio; + if (of_find_property(np, non-removable, NULL)) { + pdata-slots[0].nonremovable = true; + } This wasn't mentioned in the binding, and it seems to have different semantics to ti,non-removable. Why is it different? When ti,non-removable was added, Only OMAP platform that had eMMC was that on OMAP4 where power to eMMC cannot be switched off without sending CMD5 sleep command, so no_regulator_off_init was needed to get it detected during boot. Now start using generic non-removable for all removable cards which do not have such limitation. OK. I think this would be much clearer with something in the binding contrasting the two properties. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/9] mtd: nand: omap: combine different flavours of 1-bit hamming ecc schemes
Hi Pekon, On Tue, Oct 15, 2013 at 06:49:49AM +0100, Pekon Gupta wrote: OMAP NAND driver currently supports multiple flavours of 1-bit Hamming ecc-scheme, like: - OMAP_ECC_HAMMING_CODE_DEFAULT 1-bit hamming ecc code using software library - OMAP_ECC_HAMMING_CODE_HW 1-bit hamming ecc-code using GPMC h/w engine - OMAP_ECC_HAMMING_CODE_HW_ROMCODE 1-bit hamming ecc-code using GPMC h/w engin with ecc-layout compatible to ROM code. This patch combines above multiple ecc-schemes into single implementation: - OMAP_ECC_HAM1_CODE_HW 1-bit hamming ecc-code using GPMC h/w engine with ROM-code compatible ecc-layout. Signed-off-by: Pekon Gupta pe...@ti.com Reviewed-by: Felipe Balbi ba...@ti.com Acked-by: Tony Lindgren t...@atomide.com --- Documentation/devicetree/bindings/mtd/gpmc-nand.txt | 8 arch/arm/mach-omap2/board-flash.c | 2 +- arch/arm/mach-omap2/gpmc.c | 4 +--- drivers/mtd/nand/omap2.c| 9 +++-- include/linux/platform_data/mtd-nand-omap2.h| 6 +- 5 files changed, 10 insertions(+), 19 deletions(-) diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt index df338cb..25ee232 100644 --- a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt @@ -22,10 +22,10 @@ Optional properties: width of 8 is assumed. - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: - - swSoftware method (default) - hwHardware method - hw-romcodegpmc hamming mode method romcode layout + swdeprecated use ham1 instead + hwdeprecated use ham1 instead + hw-romcodedeprecated use ham1 instead + ham1 1-bit Hamming ecc code bch4 4-bit BCH ecc code bch8 8-bit BCH ecc code [...] diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c index 579697a..c9fb353 100644 --- a/arch/arm/mach-omap2/gpmc.c +++ b/arch/arm/mach-omap2/gpmc.c @@ -1342,9 +1342,7 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np, #ifdef CONFIG_MTD_NAND static const char * const nand_ecc_opts[] = { - [OMAP_ECC_HAMMING_CODE_DEFAULT] = sw, - [OMAP_ECC_HAMMING_CODE_HW] = hw, - [OMAP_ECC_HAMMING_CODE_HW_ROMCODE] = hw-romcode, + [OMAP_ECC_HAM1_CODE_HW] = ham1, [OMAP_ECC_BCH4_CODE_HW] = bch4, [OMAP_ECC_BCH8_CODE_HW] = bch8, }; As the parsing isn't updated until the next patch, doesn't this temporarily break DTBs with the deprecated ti,nand-ecc-opt values? Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/3] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DW PHY's
[...] + phy-sleep_a_clk = devm_clk_get(phy-dev, sleep_a); What means the _a in clock name? They are 2 PHY's on 8074 chip. This drivers is supposed to operate on PHY 0, thus sleep_a. PHY 1 is using sleep_b. Take a look here http://www.spinics.net/lists/arm-kernel/msg276945.html When you say two PHYs, do you mean the HS PHY and the SS PHY? Or are there two HS PHYs? If so, would the other HS PHY have a sleep_b clock? The clock-names property should describe the clocks from the view of the device itself. As we're describing the PHY in isolation, rather than a big block that contains the PHY, the presesnce or absence of other PHYs should not affect the name. If the _a suffix is only to differentiate the instance of PHY, it should be dropped. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] drm: omap: Enable DT support for DMM
On Tue, Oct 15, 2013 at 08:04:20AM +0100, Archit Taneja wrote: Enable use of DT for DMM/Tiler. Originally worked on by Andy Gross andy...@gmail.com Cc: Andy Gross andy...@gmail.com Cc: DRI Development dri-de...@lists.freedesktop.org Signed-off-by: Archit Taneja arc...@ti.com --- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index acf6678..59f17de 100644 --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c @@ -968,12 +968,23 @@ static const struct dev_pm_ops omap_dmm_pm_ops = { }; #endif +#if defined(CONFIG_OF) +static const struct of_device_id dmm_of_match[] = { + { .compatible = ti,omap4-dmm, }, + { .compatible = ti,omap5-dmm, }, + {}, +}; +#else +#define dmm_of_match NULL +#endif + struct platform_driver omap_dmm_driver = { .probe = omap_dmm_probe, .remove = omap_dmm_remove, .driver = { .owner = THIS_MODULE, .name = DMM_DRIVER_NAME, + .of_match_table = dmm_of_match, If you use of_match_ptr(dmm_of_match) here you don't need the #else case above. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: devicetree: Add bindings documentation for omap-des driver
Hi Joel, I realise I'm a little late in replying to this, but there are a few things that would be nice to fix up. On Fri, Nov 08, 2013 at 12:37:09AM +, Joel Fernandes wrote: Add documentation for the generic OMAP DES crypto module describing the device tree bindings. Signed-off-by: Joel Fernandes jo...@ti.com --- .../devicetree/bindings/crypto/omap-des.txt| 28 ++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/omap-des.txt diff --git a/Documentation/devicetree/bindings/crypto/omap-des.txt b/Documentation/devicetree/bindings/crypto/omap-des.txt new file mode 100644 index 000..0637647 --- /dev/null +++ b/Documentation/devicetree/bindings/crypto/omap-des.txt @@ -0,0 +1,28 @@ +OMAP SoC DES crypto Module + +Required properties: + +- compatible : Should be ti,omap4-des Nit: s/Should be/Should contain/ +- ti,hwmods: Name of the hwmod associated with the DES module +- reg : Offset and length of the register set for the module +- interrupts : the interrupt-specifier for the DES module Just to check: there's only one interrupt? +- clocks : phandle to the functional clock node of the DES module +- clock-names : Name of the functional clock What name is expected here? The intent of clock-names is to describe which clock input lines the clocks are wired to, so this should be well-defined. From the looks of the example, the clock input is called fck. Is that correct? When you have clock-names, the nicest thing to do is to define clocks in terms of clock-names. Something like: - clocks: A phandle + clock-specifier pair for each entry in clock-names. - clock-names: Should contain: * fck for the functional clock Which implies that the correct way to find clocks is to look in clock-names first to find the clock's index, rather than grabbing the clock by index. This makes it far easier to add/remove/change clocks in future. + +Optional properties: +- dmas: DMA specifiers for tx and rx dma. See the DMA client binding, + Documentation/devicetree/bindings/dma/dma.txt Similarly here it would be nice to have dmas refer to dma-names for the description of which dmas exist. +- dma-names: DMA request names should include tx and rx if present + +Example: + /* DRA7xx SoC */ + des: des@480a5000 { + compatible = ti,omap4-des; + ti,hwmods = des; + reg = 0x480a5000 0xa0; + interrupts = GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH; + dmas = sdma 117, sdma 116; + dma-names = tx, rx; + clocks = l3_iclk_div; + clock-names = fck; + }; Cheers, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: devicetree: Add bindings documentation for omap-des driver
On Mon, Nov 11, 2013 at 05:13:35PM +, Joel Fernandes wrote: On 11/11/2013 05:01 AM, Mark Rutland wrote: Hi Joel, I realise I'm a little late in replying to this, but there are a few things that would be nice to fix up. On Fri, Nov 08, 2013 at 12:37:09AM +, Joel Fernandes wrote: Add documentation for the generic OMAP DES crypto module describing the device tree bindings. Signed-off-by: Joel Fernandes jo...@ti.com --- .../devicetree/bindings/crypto/omap-des.txt| 28 ++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/crypto/omap-des.txt diff --git a/Documentation/devicetree/bindings/crypto/omap-des.txt b/Documentation/devicetree/bindings/crypto/omap-des.txt new file mode 100644 index 000..0637647 --- /dev/null +++ b/Documentation/devicetree/bindings/crypto/omap-des.txt @@ -0,0 +1,28 @@ +OMAP SoC DES crypto Module + +Required properties: + +- compatible : Should be ti,omap4-des Nit: s/Should be/Should contain/ ok +- ti,hwmods: Name of the hwmod associated with the DES module +- reg : Offset and length of the register set for the module +- interrupts : the interrupt-specifier for the DES module Just to check: there's only one interrupt? Yes. +- clocks : phandle to the functional clock node of the DES module +- clock-names : Name of the functional clock What name is expected here? The intent of clock-names is to describe which clock input lines the clocks are wired to, so this should be well-defined. From the looks of the example, the clock input is called fck. Is that correct? Yes. The clock name is used by PM code to get the clock for the particular device. Yes the code assumes it to be fck, I can change doc to be so. When you have clock-names, the nicest thing to do is to define clocks in terms of clock-names. Something like: - clocks: A phandle + clock-specifier pair for each entry in clock-names. - clock-names: Should contain: * fck for the functional clock Which implies that the correct way to find clocks is to look in clock-names first to find the clock's index, rather than grabbing the clock by index. This makes it far easier to add/remove/change clocks in future. Ok. I'll reword documentation of clocks as above. + +Optional properties: +- dmas: DMA specifiers for tx and rx dma. See the DMA client binding, + Documentation/devicetree/bindings/dma/dma.txt Similarly here it would be nice to have dmas refer to dma-names for the description of which dmas exist. Ok, will make the above 3 changes and resubmit, if its ok will add your Reviewed-by. That would be fine by me. With the changes: Reviewed-by: Mark Rutland mark.rutl...@arm.com Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 0/8] omap hwspinlock dt support
On Tue, Nov 12, 2013 at 06:16:42PM +, Anna, Suman wrote: Hi, This is an updated series addressing the review comments from the v2 series. The hwmod patches have been dropped from the repost as per Paul's request, they have already been queued. Mark, Hi Suman, Any comments on this series? Tony has picked up the OMAP DTS patches for 3.13, and so the ti,omap4-hwspinlock compatible string is showing up as undocumented in linux-next. How do you want me to proceed here? I will be separating out the bindings into separate patches in the future. The only thing I note that I'm not so keen on is that the hwlock-specifier is always one cell, rather than using a #hwlock-cells property on the provider (even if we required it to be 1 for the moment and just failed if it wasn't). If possible, I would like an amendment to always use #hwlock-cells, but otherwise this looks fine to me. Feel free to add my Ack: Acked-by: Mark Rutland mark.rutl...@arm.com Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/8] net: smc91x: Fix device tree based configuration so it's usable
On Thu, Nov 14, 2013 at 02:35:30AM +, Tony Lindgren wrote: Commit 89ce376c6bdc (drivers/net: Use of_match_ptr() macro in smc91x.c) added minimal device tree support to smc91x, but it's not working on many platforms because of the lack of some key configuration bits. Fix the issue by parsing the necessary configuration like the smc911x driver is doing. Cc: Nicolas Pitre n...@fluxnic.net Cc: David S. Miller da...@davemloft.net Cc: net...@vger.kernel.org Cc: devicet...@vger.kernel.org Signed-off-by: Tony Lindgren t...@atomide.com --- If this looks OK, I'd like to merge this as a fix via arm-soc tree along with the other patches in this series as my later patches depend on patches in this series. --- .../devicetree/bindings/net/smsc-lan91c111.txt | 4 ++ drivers/net/ethernet/smsc/smc91x.c | 52 +- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/Documentation/devicetree/bindings/net/smsc-lan91c111.txt b/Documentation/devicetree/bindings/net/smsc-lan91c111.txt index 953049b..53d69e3 100644 --- a/Documentation/devicetree/bindings/net/smsc-lan91c111.txt +++ b/Documentation/devicetree/bindings/net/smsc-lan91c111.txt @@ -8,3 +8,7 @@ Required properties: Optional properties: - phy-device : phandle to Ethernet phy - local-mac-address : Ethernet mac address to use +- reg-io-width : Specify the size (in bytes) of the IO accesses that + should be performed on the device. Valid value for SMSC LAN is + 1, 2 or 4. If it's omitted or invalid, the size would be 2. In the driver the supported access sizes are not mutually exclusive. It would be nice for the binding to have the same property. +- smsc,nowait : Setup for fast register access with no waits I'm confused by what this means. When would this be selected, and when wouldn't it be? Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] mmc: omap: Fix I2C dependency and make driver usable with device tree
On Thu, Nov 14, 2013 at 02:35:32AM +, Tony Lindgren wrote: Some features can be configured by the companion I2C chips, which may not be available at the probe time. Fix the issue by returning -EPROBE_DEFER when the MMC controller slots are not configured. While at it, let's also add minimal device tree support so omap24xx platforms can use this driver without legacy mode since we claim to support device tree for mach-omap2 based systems. Although adding the minimal device tree support is not strictly a fix, it does remove one of the last blockers for dropping a bunch of legacy platform data for mach-omap2. Cc: Chris Ball c...@laptop.org Cc: linux-...@vger.kernel.org Signed-off-by: Tony Lindgren t...@atomide.com --- If this looks OK, I'd like to merge this as a fix via arm-soc tree along with the other patches in this series as my later patches depend on patches in this series. --- drivers/mmc/host/omap.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c index ed56868..43c66ad 100644 --- a/drivers/mmc/host/omap.c +++ b/drivers/mmc/host/omap.c @@ -22,6 +22,7 @@ #include linux/delay.h #include linux/spinlock.h #include linux/timer.h +#include linux/of.h #include linux/omap-dma.h #include linux/mmc/host.h #include linux/mmc/card.h @@ -1330,7 +1331,7 @@ static int mmc_omap_probe(struct platform_device *pdev) } if (pdata-nr_slots == 0) { dev_err(pdev-dev, no slots\n); - return -ENXIO; + return -EPROBE_DEFER; } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -1553,6 +1554,12 @@ static int mmc_omap_resume(struct platform_device *pdev) #define mmc_omap_resume NULL #endif +#if IS_BUILTIN(CONFIG_OF) +static const struct of_device_id mmc_omap_match[] = { + { .compatible = ti,omap2420-mmc, }, Missing binding document. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/8] i2c: omap: Fix missing device tree flags for omap2
On Thu, Nov 14, 2013 at 02:35:33AM +, Tony Lindgren wrote: As we claim to support device tree for mach-omap2, we should have the necessary flags in the driver to make it usable. Cc: Wolfram Sang w...@the-dreams.de Cc: linux-...@vger.kernel.org Signed-off-by: Tony Lindgren t...@atomide.com --- If this looks OK, I'd like to merge this as a fix via arm-soc tree along with the other patches in this series as my later patches depend on patches in this series. --- drivers/i2c/busses/i2c-omap.c | 22 ++ 1 file changed, 22 insertions(+) [...] + { + .compatible = ti,omap2430-i2c, + .data = omap2430_pdata, + }, + { + .compatible = ti,omap2420-i2c, + .data = omap2420_pdata, Please update Documentation/devicetree/bindings/i2c/i2c-omap.txt Otherwise, this is fine. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] input: touchscreen: fix spelling mistake in TSC/ADC DT binding
On Tue, Oct 22, 2013 at 01:02:53PM +0100, Felipe Balbi wrote: Hi, On Tue, Oct 22, 2013 at 10:42:00AM +0200, Sebastian Andrzej Siewior wrote: On 10/21/2013 10:13 PM, Felipe Balbi wrote: diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c index e1c5300..b61df9d 100644 --- a/drivers/input/touchscreen/ti_am335x_tsc.c +++ b/drivers/input/touchscreen/ti_am335x_tsc.c @@ -348,9 +348,16 @@ static int titsc_parse_dt(struct platform_device *pdev, if (err 0) return err; - err = of_property_read_u32(node, ti,coordiante-readouts, + /* + * try with new binding first. If it fails, still try with + * bogus, miss-spelled version. + */ + err = of_property_read_u32(node, ti,coordinate-readouts, ts_dev-coordinate_readouts); if (err 0) + err = of_property_read_u32(node, ti,coordiante-readouts, + ts_dev-coordinate_readouts); + if (err 0) return err; Thanks, very good. Do we keep this fallback for ever or just for a year or two? That's for DT maintainers to decide but considering DT is an ABI, I guess we need to keep for 30 years or so :-p We keep it as long as we have to. If no-one's relying on the typo by the next merge window, I see no reason we'd have to keep support for the typo beyond that. If someone's shipped a device with a dtb with the typo hard-coded into some ROM, that's another matter... It might be worth printing a warning in the case of the typo'd version, suggesting correcting the DT. That will encourage anyone with a broken dt to get a fixed one. Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/4] DRIVERS: IRQCHIP: IRQ-GIC: Add support for routable irqs
On Thu, Nov 14, 2013 at 12:18:47PM +, Sricharan R wrote: In some socs the gic can be preceded by a crossbar IP which routes the peripheral interrupts to the gic inputs. The peripheral interrupts are associated with a fixed crossbar input line and the crossbar routes that to one of the free gic input line. The DT entries for peripherals provides the fixed crossbar input line as its interrupt number and the mapping code should associate this with a free gic input line. This patch adds the support inside the gic irqchip to handle such routable irqs. The routable irqs are registered in a linear domain. The registered routable domain's callback should be implemented to get a free irq and to configure the IP to route it. Cc: Thomas Gleixner t...@linutronix.de Cc: Linus Walleij linus.wall...@linaro.org Cc: Santosh Shilimkar santosh.shilim...@ti.com Cc: Russell King li...@arm.linux.org.uk Cc: Tony Lindgren t...@atomide.com Cc: Rajendra Nayak rna...@ti.com Cc: Marc Zyngier marc.zyng...@arm.com Cc: Grant Likely grant.lik...@linaro.org Cc: Rob Herring rob.herr...@calxeda.com Signed-off-by: Sricharan R r.sricha...@ti.com Acked-by: Santosh Shilimkar santosh.shilim...@ti.com --- [V2] Added default routable-irqs functions to avoid unnecessary if checks as per Thomas Gleixner comments and renamed routable-irq binding as per Kumar Gala ga...@codeaurora.org comments. [V3] Addressed unnecessary warn-on and updated default xlate function as per Thomas Gleixner comments Documentation/devicetree/bindings/arm/gic.txt |6 ++ drivers/irqchip/irq-gic.c | 81 ++--- include/linux/irqchip/arm-gic.h |7 ++- 3 files changed, 83 insertions(+), 11 deletions(-) diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt index 3dfb0c0..5357745 100644 --- a/Documentation/devicetree/bindings/arm/gic.txt +++ b/Documentation/devicetree/bindings/arm/gic.txt @@ -49,6 +49,11 @@ Optional regions, used when the GIC doesn't have banked registers. The offset is cpu-offset * cpu-nr. +- arm,routable-irqs : Total number of gic irq inputs which are not directly + connected from the peripherals, but are routed dynamically + by a crossbar/multiplexer preceding the GIC. The GIC irq + input line is assigned dynamically when the corresponding + peripheral's crossbar line is mapped. I'm not keen on the design of the arm,routable-irqs property. The set of IRQs which the crossbar IP can use is a property of which IRQ lines it has routed to the GIC. I don't see why that should be considered a property of the GIC; it's a property of the crossbar IP's attachment to the GIC. Given we already have a mechanism for describing the attachment (i.e. the interrupts property) where the property appears on the node for the device generating/propagating the interrupt, I don't see why we should do differently here. Listing 160 interrupts in the crossbar node is clearly something we don't want to have to do. If we had a property that we could use to define a range (or multiple ranges) of interrupts, then the crossbar driver could go and request those ranges from its interrupt-parent (the GIC) and the GIC driver could reserve/allocate the irqdomain at that time. This feels like a point-hack, counter in style to the vast majority of provider/consumer bindings. It only allows for one multiplexer before the GIC. What if we had multiple multiplexers feeding into the GIC? Describing the attachment on the multiplexer allows that to be handled, describing that on the GIC does not. Describing the attachement on the multiplexer would also prevent the duplication of information (i.e. the max-irqs property in the crossbar binding). Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 2/4] DRIVERS: IRQCHIP: CROSSBAR: Add support for Crossbar IP
On Thu, Nov 14, 2013 at 12:18:48PM +, Sricharan R wrote: Some socs have a large number of interrupts requests to service the needs of its many peripherals and subsystems. All of the interrupt lines from the subsystems are not needed at the same time, so they have to be muxed to the irq-controller appropriately. In such places a interrupt controllers are preceded by an CROSSBAR that provides flexibility in muxing the device requests to the controller inputs. This driver takes care a allocating a free irq and then configuring the crossbar IP as a part of the mpu's irqchip callbacks. crossbar_init should be called right before the irqchip_init, so that it is setup to handle the irqchip callbacks. Cc: Thomas Gleixner t...@linutronix.de Cc: Linus Walleij linus.wall...@linaro.org Cc: Santosh Shilimkar santosh.shilim...@ti.com Cc: Russell King li...@arm.linux.org.uk Cc: Tony Lindgren t...@atomide.com Cc: Rajendra Nayak rna...@ti.com Cc: Marc Zyngier marc.zyng...@arm.com Cc: Grant Likely grant.lik...@linaro.org Cc: Rob Herring rob.herr...@calxeda.com Signed-off-by: Sricharan R r.sricha...@ti.com Acked-by: Kumar Gala ga...@codeaurora.org (for DT binding portion) Acked-by: Santosh Shilimkar santosh.shilim...@ti.com --- [V2] Addressed Thomas Gleixner t...@linutronix.de comments and renamed the bindings as per Kumar Gala ga...@codeaurora.org comments. [V3] Changed static inline const to static inline int and removed unnecessary variable initialization as per Thomas Gleixner t...@linutronix.de. Updated commit tags [V4] Renamed crossbar_init as irqcrossbar_init as per Rajendra Nayak rna...@ti.com suggestion. .../devicetree/bindings/arm/omap/crossbar.txt | 27 +++ drivers/irqchip/Kconfig|8 + drivers/irqchip/Makefile |1 + drivers/irqchip/irq-crossbar.c | 206 include/linux/irqchip/irq-crossbar.h | 11 ++ 5 files changed, 253 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/omap/crossbar.txt create mode 100644 drivers/irqchip/irq-crossbar.c create mode 100644 include/linux/irqchip/irq-crossbar.h diff --git a/Documentation/devicetree/bindings/arm/omap/crossbar.txt b/Documentation/devicetree/bindings/arm/omap/crossbar.txt new file mode 100644 index 000..fb88585 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/omap/crossbar.txt @@ -0,0 +1,27 @@ +Some socs have a large number of interrupts requests to service +the needs of its many peripherals and subsystems. All of the +interrupt lines from the subsystems are not needed at the same +time, so they have to be muxed to the irq-controller appropriately. +In such places a interrupt controllers are preceded by an CROSSBAR +that provides flexibility in muxing the device requests to the controller +inputs. + +Required properties: +- compatible : Should be ti,irq-crossbar +- reg: Base address and the size of the crossbar registers. +- ti,max-irqs: Total number of irqs available at the interrupt controller. +- ti,reg-size: Size of a individual register in bytes. Every individual + register is assumed to be of same size. Valid sizes are 1, 2, 4. +- ti,irqs-reserved: List of the reserved irq lines that are not muxed using + crossbar. These interrupt lines are reserved in the soc, + so crossbar bar driver should not consider them as free + lines. The combination of the ti,max-irqs and ti,irqs-reserved properties seems backwards to me. Why can we not describe the set of IRQs that _can_ be used? + +Examples: + crossbar_mpu: @4a02 { + compatible = ti,irq-crossbar; + reg = 0x4a002a48 0x130; + ti,max-irqs = 160; + ti,reg-size = 2; + ti,irqs-reserved = 0 1 2 3 5 6 131 132 139 140; + }; [...] + /* Get and mark reserved irqs */ + irqsr = of_get_property(node, ti,irqs-reserved, size); + if (irqsr) { + size /= sizeof(__be32); + + for (i = 0; i size; i++) { + entry = be32_to_cpup(irqsr + i); + if (entry max) { + pr_err(Invalid reserved entry\n); + goto err3; + } + cb-irq_map[entry] = 0; + } + } Don't deal with the raw DTB. Use of_property_read_u32_index. + + cb-register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL); + if (!cb-register_offsets) + goto err3; + + of_property_read_u32(node, ti,reg-size, size); If ti,reg-size isn't present, size is uninitialized. Please check the return value of of_property_read_u32. Thanks, Mark. -- To unsubscribe from this list: send the line unsubscribe linux-omap in
Re: [PATCH V4 2/4] DRIVERS: IRQCHIP: CROSSBAR: Add support for Crossbar IP
On Thu, Nov 14, 2013 at 04:41:41PM +, Sricharan R wrote: Hi Mark, On Thursday 14 November 2013 07:42 PM, Mark Rutland wrote: On Thu, Nov 14, 2013 at 12:18:48PM +, Sricharan R wrote: Some socs have a large number of interrupts requests to service the needs of its many peripherals and subsystems. All of the interrupt lines from the subsystems are not needed at the same time, so they have to be muxed to the irq-controller appropriately. In such places a interrupt controllers are preceded by an CROSSBAR that provides flexibility in muxing the device requests to the controller inputs. This driver takes care a allocating a free irq and then configuring the crossbar IP as a part of the mpu's irqchip callbacks. crossbar_init should be called right before the irqchip_init, so that it is setup to handle the irqchip callbacks. Cc: Thomas Gleixner t...@linutronix.de Cc: Linus Walleij linus.wall...@linaro.org Cc: Santosh Shilimkar santosh.shilim...@ti.com Cc: Russell King li...@arm.linux.org.uk Cc: Tony Lindgren t...@atomide.com Cc: Rajendra Nayak rna...@ti.com Cc: Marc Zyngier marc.zyng...@arm.com Cc: Grant Likely grant.lik...@linaro.org Cc: Rob Herring rob.herr...@calxeda.com Signed-off-by: Sricharan R r.sricha...@ti.com Acked-by: Kumar Gala ga...@codeaurora.org (for DT binding portion) Acked-by: Santosh Shilimkar santosh.shilim...@ti.com --- [V2] Addressed Thomas Gleixner t...@linutronix.de comments and renamed the bindings as per Kumar Gala ga...@codeaurora.org comments. [V3] Changed static inline const to static inline int and removed unnecessary variable initialization as per Thomas Gleixner t...@linutronix.de. Updated commit tags [V4] Renamed crossbar_init as irqcrossbar_init as per Rajendra Nayak rna...@ti.com suggestion. .../devicetree/bindings/arm/omap/crossbar.txt | 27 +++ drivers/irqchip/Kconfig|8 + drivers/irqchip/Makefile |1 + drivers/irqchip/irq-crossbar.c | 206 include/linux/irqchip/irq-crossbar.h | 11 ++ 5 files changed, 253 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/omap/crossbar.txt create mode 100644 drivers/irqchip/irq-crossbar.c create mode 100644 include/linux/irqchip/irq-crossbar.h diff --git a/Documentation/devicetree/bindings/arm/omap/crossbar.txt b/Documentation/devicetree/bindings/arm/omap/crossbar.txt new file mode 100644 index 000..fb88585 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/omap/crossbar.txt @@ -0,0 +1,27 @@ +Some socs have a large number of interrupts requests to service +the needs of its many peripherals and subsystems. All of the +interrupt lines from the subsystems are not needed at the same +time, so they have to be muxed to the irq-controller appropriately. +In such places a interrupt controllers are preceded by an CROSSBAR +that provides flexibility in muxing the device requests to the controller +inputs. + +Required properties: +- compatible : Should be ti,irq-crossbar +- reg: Base address and the size of the crossbar registers. +- ti,max-irqs: Total number of irqs available at the interrupt controller. +- ti,reg-size: Size of a individual register in bytes. Every individual + register is assumed to be of same size. Valid sizes are 1, 2, 4. +- ti,irqs-reserved: List of the reserved irq lines that are not muxed using + crossbar. These interrupt lines are reserved in the soc, + so crossbar bar driver should not consider them as free + lines. The combination of the ti,max-irqs and ti,irqs-reserved properties seems backwards to me. Why can we not describe the set of IRQs that _can_ be used? Total set of irqs that are usable is max - reserved. Since reserved irqs are not continuous, we have to give the list. During the init we count the total number of reserved and get the usable one. So why not describe the set of usable IRQs, rather than a set of IRQs for which only some are usable then subtracting the set of unusable IRQs? It seems backwards to me to have a binding for a device describe resources it doesn't have. + +Examples: + crossbar_mpu: @4a02 { + compatible = ti,irq-crossbar; + reg = 0x4a002a48 0x130; + ti,max-irqs = 160; + ti,reg-size = 2; + ti,irqs-reserved = 0 1 2 3 5 6 131 132 139 140; + }; [...] + /* Get and mark reserved irqs */ + irqsr = of_get_property(node, ti,irqs-reserved, size); + if (irqsr) { + size /= sizeof(__be32); + + for (i = 0; i size; i++) { + entry = be32_to_cpup(irqsr + i); + if (entry max) { + pr_err
Re: [PATCH V4 1/4] DRIVERS: IRQCHIP: IRQ-GIC: Add support for routable irqs
On Thu, Nov 14, 2013 at 04:46:36PM +, Sricharan R wrote: Hi Mark, On Thursday 14 November 2013 07:31 PM, Mark Rutland wrote: On Thu, Nov 14, 2013 at 12:18:47PM +, Sricharan R wrote: In some socs the gic can be preceded by a crossbar IP which routes the peripheral interrupts to the gic inputs. The peripheral interrupts are associated with a fixed crossbar input line and the crossbar routes that to one of the free gic input line. The DT entries for peripherals provides the fixed crossbar input line as its interrupt number and the mapping code should associate this with a free gic input line. This patch adds the support inside the gic irqchip to handle such routable irqs. The routable irqs are registered in a linear domain. The registered routable domain's callback should be implemented to get a free irq and to configure the IP to route it. Cc: Thomas Gleixner t...@linutronix.de Cc: Linus Walleij linus.wall...@linaro.org Cc: Santosh Shilimkar santosh.shilim...@ti.com Cc: Russell King li...@arm.linux.org.uk Cc: Tony Lindgren t...@atomide.com Cc: Rajendra Nayak rna...@ti.com Cc: Marc Zyngier marc.zyng...@arm.com Cc: Grant Likely grant.lik...@linaro.org Cc: Rob Herring rob.herr...@calxeda.com Signed-off-by: Sricharan R r.sricha...@ti.com Acked-by: Santosh Shilimkar santosh.shilim...@ti.com --- [V2] Added default routable-irqs functions to avoid unnecessary if checks as per Thomas Gleixner comments and renamed routable-irq binding as per Kumar Gala ga...@codeaurora.org comments. [V3] Addressed unnecessary warn-on and updated default xlate function as per Thomas Gleixner comments Documentation/devicetree/bindings/arm/gic.txt |6 ++ drivers/irqchip/irq-gic.c | 81 ++--- include/linux/irqchip/arm-gic.h |7 ++- 3 files changed, 83 insertions(+), 11 deletions(-) diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt index 3dfb0c0..5357745 100644 --- a/Documentation/devicetree/bindings/arm/gic.txt +++ b/Documentation/devicetree/bindings/arm/gic.txt @@ -49,6 +49,11 @@ Optional regions, used when the GIC doesn't have banked registers. The offset is cpu-offset * cpu-nr. +- arm,routable-irqs : Total number of gic irq inputs which are not directly +connected from the peripherals, but are routed dynamically +by a crossbar/multiplexer preceding the GIC. The GIC irq +input line is assigned dynamically when the corresponding +peripheral's crossbar line is mapped. I'm not keen on the design of the arm,routable-irqs property. The set of IRQs which the crossbar IP can use is a property of which IRQ lines it has routed to the GIC. I don't see why that should be considered a property of the GIC; it's a property of the crossbar IP's attachment to the GIC. Given we already have a mechanism for describing the attachment (i.e. the interrupts property) where the property appears on the node for the device generating/propagating the interrupt, I don't see why we should do differently here. We did try using interrupts= property for all peripherals and mapping them as crossbar's parent. But that approach of representing crossbar as a interrupt parent was not accepted, since the crossbar was just routing the interrupts from peripherals to GIC and nothing more. Also mapping all the interrupts using interrupt-map like property by a fixed way in DTS itself was considered hacky I'm not suggesting you should interrupt-map. I agree that that interrupt-map is not suitable for a dynamically configurable device like the crossbar. When you say that the crossbar is just routing the interrupts, at what level is it doing so? Does it accept a logical interrupt and output another logical interrupt, or does it just connect the two lines electrically? We don't necessarily have to use the interrupts property, but I still think that the set of GIC input IRQ lines that the crossbar is wired to should be described on the crossbar node. Listing 160 interrupts in the crossbar node is clearly something we don't want to have to do. If we had a property that we could use to define a range (or multiple ranges) of interrupts, then the crossbar driver could go and request those ranges from its interrupt-parent (the GIC) and the GIC driver could reserve/allocate the irqdomain at that time. Again, this kind of approach of crossbar requesting irqs from GIC was tried earlier and it did not go anywhere. Subsequently after lot of discussions this design was considered the best one. http://www.spinics.net/lists/linux-omap/msg97085.html As far as I can see, the comment there was to use irqdomains, which I am not arguing against. I am arguing that the linkage of the GIC and the crossbar is being