Hi,
This is an updated series for adding the device tree support to
the OMAP hwspinlock driver. The series is based on 3.12-rc1, and
includes patches on hwspinlock driver, OMAP hwmod data files and
OMAP DTS files. The updated series adds new patches to enable the
hwspinlock driver on OMAP5 and AM335x SoCs as well.
Tested this on Panda4, BeagleBone Black and OMAP5 uEVM (with
out-of-tree clock patches from Tero).
Changes new in v2:
- Added a new common DT binding documentation and OF helpers (Patch1),
addressing the review comments from v1 [2]. The MSM support [1] needs
to be reworked on top of this common patch.
- Revised OMAP DT parse support to use the new OF helper (Patch2)
- OMAP5 hwspinlock support including the hwmod entry and DT node (Patches 4, 5)
- Add AM335x support to OMAP hwspinlock driver, including a fix
needed in driver given that AM335 spinlock module requires s/w wakeup
(Patches 6, 7)
- AM335 DT node for spinlock, and a hwmod change to enable smart-idle
for AM335 (Patches 8, 9). The sysc patch is not essential for AM335
spinlock functionality, but is needed for smart-idling the IP when
the module is enabled.
- OMAP4 DT node patch is unchanged (Patch 3)
v1:
- Add DT parse support to OMAP hwspinlock driver
- Add OMAP4 DT node and bindings information
http://marc.info/?l=linux-omap&m=137823082308009&w=2
[1] https://lkml.org/lkml/2013/8/14/528
[2] http://marc.info/?t=137823090300005&r=1&w=2
Suman Anna (9):
hwspinlock/core: add common dt bindings and OF helpers
hwspinlock/omap: add support for dt nodes
ARM: dts: OMAP4: Add hwspinlock node
ARM: OMAP5: hwmod data: Add spinlock data
ARM: dts: OMAP5: Add hwspinlock node
hwspinlock/omap: support AM33xx
hwspinlock/omap: enable module before reading SYSSTATUS register
ARM: dts: AM33XX: Add hwspinlock node
ARM: AM33xx: hwmod_data: add the sysc configuration for spinlock
.../devicetree/bindings/hwlock/hwlock.txt | 26 +++++++++
.../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
arch/arm/boot/dts/am33xx.dtsi | 6 +++
arch/arm/boot/dts/omap4.dtsi | 6 +++
arch/arm/boot/dts/omap5.dtsi | 6 +++
arch/arm/mach-omap2/Makefile | 3 --
arch/arm/mach-omap2/hwspinlock.c | 60 ---------------------
arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 13 +++++
arch/arm/mach-omap2/omap_hwmod_54xx_data.c | 44 ++++++++++++++++
drivers/hwspinlock/Kconfig | 2 +-
drivers/hwspinlock/hwspinlock_core.c | 61 +++++++++++++++++++++-
drivers/hwspinlock/omap_hwspinlock.c | 44 ++++++++++++----
include/linux/hwspinlock.h | 11 ++--
13 files changed, 233 insertions(+), 80 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt
create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
delete mode 100644 arch/arm/mach-omap2/hwspinlock.c
--
1.8.3.3
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 <[email protected]>
---
.../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 0000000..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
+- reg: Contains the hwspinlock register address range (base
+ address and length)
+- 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.
+- 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.
+
+
+Example:
+
+/* OMAP4 */
+hwspinlock: spinlock@4a0f6000 {
+ compatible = "ti,omap4-hwspinlock";
+ reg = <0x4a0f6000 0x1000>;
+ ti,hwmods = "spinlock";
+};
diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index afb457c..960cf45 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -298,9 +298,6 @@ obj-y += $(smc91x-m) $(smc91x-y)
smsc911x-$(CONFIG_SMSC911X) := gpmc-smsc911x.o
obj-y += $(smsc911x-m) $(smsc911x-y)
-ifneq ($(CONFIG_HWSPINLOCK_OMAP),)
-obj-y += hwspinlock.o
-endif
emac-$(CONFIG_TI_DAVINCI_EMAC) := am35xx-emac.o
obj-y += $(emac-m) $(emac-y)
diff --git a/arch/arm/mach-omap2/hwspinlock.c b/arch/arm/mach-omap2/hwspinlock.c
deleted file mode 100644
index ef175ac..0000000
--- a/arch/arm/mach-omap2/hwspinlock.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
- * OMAP hardware spinlock device initialization
- *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
- *
- * Contact: Simon Que <[email protected]>
- * Hari Kanigeri <[email protected]>
- *
- * 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.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * General Public License for more details.
- */
-
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/err.h>
-#include <linux/hwspinlock.h>
-
-#include "soc.h"
-#include "omap_hwmod.h"
-#include "omap_device.h"
-
-static struct hwspinlock_pdata omap_hwspinlock_pdata __initdata = {
- .base_id = 0,
-};
-
-static int __init hwspinlocks_init(void)
-{
- int retval = 0;
- struct omap_hwmod *oh;
- struct platform_device *pdev;
- const char *oh_name = "spinlock";
- const char *dev_name = "omap_hwspinlock";
-
- /*
- * Hwmod lookup will fail in case our platform doesn't support the
- * hardware spinlock module, so it is safe to run this initcall
- * on all omaps
- */
- oh = omap_hwmod_lookup(oh_name);
- if (oh == NULL)
- return -EINVAL;
-
- pdev = omap_device_build(dev_name, 0, oh, &omap_hwspinlock_pdata,
- sizeof(struct hwspinlock_pdata));
- if (IS_ERR(pdev)) {
- pr_err("Can't build omap_device for %s:%s\n", dev_name,
- oh_name);
- retval = PTR_ERR(pdev);
- }
-
- return retval;
-}
-/* early board code might need to reserve specific hwspinlock instances */
-omap_postcore_initcall(hwspinlocks_init);
diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
index 292869c..1c99391 100644
--- a/drivers/hwspinlock/omap_hwspinlock.c
+++ b/drivers/hwspinlock/omap_hwspinlock.c
@@ -1,7 +1,7 @@
/*
* OMAP hardware spinlock driver
*
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com
+ * Copyright (C) 2010-2013 Texas Instruments Incorporated - http://www.ti.com
*
* Contact: Simon Que <[email protected]>
* Hari Kanigeri <[email protected]>
@@ -27,6 +27,7 @@
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/hwspinlock.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include "hwspinlock_internal.h"
@@ -80,16 +81,23 @@ static const struct hwspinlock_ops omap_hwspinlock_ops = {
static int omap_hwspinlock_probe(struct platform_device *pdev)
{
- struct hwspinlock_pdata *pdata = pdev->dev.platform_data;
+ struct device_node *node = pdev->dev.of_node;
struct hwspinlock_device *bank;
struct hwspinlock *hwlock;
struct resource *res;
void __iomem *io_base;
int num_locks, i, ret;
+ int base_id = 0;
- if (!pdata)
+ if (!node)
return -ENODEV;
+ ret = of_hwspin_lock_get_base_id(node);
+ if (ret < 0 && ret != -EINVAL)
+ return -ENODEV;
+ else if (!ret)
+ base_id = ret;
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
return -ENODEV;
@@ -128,7 +136,7 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
ret = hwspin_lock_register(bank, &pdev->dev, &omap_hwspinlock_ops,
- pdata->base_id, num_locks);
+ base_id, num_locks);
if (ret)
goto reg_fail;
@@ -161,12 +169,19 @@ static int omap_hwspinlock_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id omap_hwspinlock_of_match[] = {
+ { .compatible = "ti,omap4-hwspinlock", },
+ { /* end */ },
+};
+MODULE_DEVICE_TABLE(of, omap_hwspinlock_of_match);
+
static struct platform_driver omap_hwspinlock_driver = {
.probe = omap_hwspinlock_probe,
.remove = omap_hwspinlock_remove,
.driver = {
.name = "omap_hwspinlock",
.owner = THIS_MODULE,
+ .of_match_table = omap_hwspinlock_of_match,
},
};
--
1.8.3.3
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 <[email protected]>
---
.../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 0000000..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.
+
+- 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.
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 <[email protected]>
*
@@ -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;
+
+ ret = of_property_read_u32(dn, "hwlock-base-id", &val);
+ if (!ret)
+ ret = val;
+#endif
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
+
+/**
+ * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks
+ * @dn: device node pointer
+ *
+ * This is an OF helper function that can be called by the underlying
+ * platform-specific implementations, to retrieve the number of locks
+ * present within a hwspinlock device instance.
+ *
+ * Returns a positive number of locks on success, -ENODEV on generic
+ * failure or an appropriate error code as returned by the OF layer
+ */
+int of_hwspin_lock_get_num_locks(struct device_node *dn)
+{
+ unsigned int val;
+ int ret = -ENODEV;
+
+#ifdef CONFIG_OF
+ if (!dn)
+ return -ENODEV;
+
+ ret = of_property_read_u32(dn, "hwlock-num-locks", &val);
+ if (!ret)
+ ret = val ? val : -ENODEV;
+#endif
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
+
+/**
* hwspin_lock_register() - register a new hw spinlock device
* @bank: the hwspinlock device, which usually provides numerous hw locks
* @dev: the backing device
diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
index 3343298..1f6a5b8 100644
--- a/include/linux/hwspinlock.h
+++ b/include/linux/hwspinlock.h
@@ -1,7 +1,7 @@
/*
* Hardware spinlock public header
*
- * 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 <[email protected]>
*
@@ -26,6 +26,7 @@
#define HWLOCK_IRQ 0x02 /* Disable interrupts, don't save state */
struct device;
+struct device_node;
struct hwspinlock;
struct hwspinlock_device;
struct hwspinlock_ops;
@@ -60,6 +61,8 @@ struct hwspinlock_pdata {
#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
+int of_hwspin_lock_get_base_id(struct device_node *dn);
+int of_hwspin_lock_get_num_locks(struct device_node *dn);
int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
const struct hwspinlock_ops *ops, int base_id, int num_locks);
int hwspin_lock_unregister(struct hwspinlock_device *bank);
@@ -80,9 +83,9 @@ void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
* code path get compiled away. This way, if CONFIG_HWSPINLOCK is not
* required on a given setup, users will still work.
*
- * The only exception is hwspin_lock_register/hwspin_lock_unregister, with which
- * we _do_ want users to fail (no point in registering hwspinlock instances if
- * the framework is not available).
+ * The only exception is hwspin_lock_register/hwspin_lock_unregister and
+ * associated OF helpers, with which we _do_ want users to fail (no point
+ * in registering hwspinlock instances if the framework is not available).
*
* Note: ERR_PTR(-ENODEV) will still be considered a success for NULL-checking
* users. Others, which care, can still check this with IS_ERR.
--
1.8.3.3
Add the hwmod data for the spinlock IP in OMAP5 SoC.
This is needed to be able to enable the OMAP spinlock
support for OMAP5.
Signed-off-by: Suman Anna <[email protected]>
---
arch/arm/mach-omap2/omap_hwmod_54xx_data.c | 44 ++++++++++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/arch/arm/mach-omap2/omap_hwmod_54xx_data.c b/arch/arm/mach-omap2/omap_hwmod_54xx_data.c
index cde4155..84712e4 100644
--- a/arch/arm/mach-omap2/omap_hwmod_54xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_54xx_data.c
@@ -1146,6 +1146,41 @@ static struct omap_hwmod omap54xx_mpu_hwmod = {
};
/*
+ * 'spinlock' class
+ * spinlock provides hardware assistance for synchronizing the processes
+ * running on multiple processors
+ */
+
+static struct omap_hwmod_class_sysconfig omap54xx_spinlock_sysc = {
+ .rev_offs = 0x0000,
+ .sysc_offs = 0x0010,
+ .syss_offs = 0x0014,
+ .sysc_flags = (SYSC_HAS_AUTOIDLE | SYSC_HAS_CLOCKACTIVITY |
+ SYSC_HAS_ENAWAKEUP | SYSC_HAS_SIDLEMODE |
+ SYSC_HAS_SOFTRESET | SYSS_HAS_RESET_STATUS),
+ .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+ .sysc_fields = &omap_hwmod_sysc_type1,
+};
+
+static struct omap_hwmod_class omap54xx_spinlock_hwmod_class = {
+ .name = "spinlock",
+ .sysc = &omap54xx_spinlock_sysc,
+};
+
+/* spinlock */
+static struct omap_hwmod omap54xx_spinlock_hwmod = {
+ .name = "spinlock",
+ .class = &omap54xx_spinlock_hwmod_class,
+ .clkdm_name = "l4cfg_clkdm",
+ .prcm = {
+ .omap4 = {
+ .clkctrl_offs = OMAP54XX_CM_L4CFG_SPINLOCK_CLKCTRL_OFFSET,
+ .context_offs = OMAP54XX_RM_L4CFG_SPINLOCK_CONTEXT_OFFSET,
+ },
+ },
+};
+
+/*
* 'timer' class
* general purpose timer module with accurate 1ms tick
* This class contains several variants: ['timer_1ms', 'timer']
@@ -1960,6 +1995,14 @@ static struct omap_hwmod_ocp_if omap54xx_l4_cfg__mpu = {
.user = OCP_USER_MPU | OCP_USER_SDMA,
};
+/* l4_cfg -> spinlock */
+static struct omap_hwmod_ocp_if omap54xx_l4_cfg__spinlock = {
+ .master = &omap54xx_l4_cfg_hwmod,
+ .slave = &omap54xx_spinlock_hwmod,
+ .clk = "l4_root_clk_div",
+ .user = OCP_USER_MPU | OCP_USER_SDMA,
+};
+
/* l4_wkup -> timer1 */
static struct omap_hwmod_ocp_if omap54xx_l4_wkup__timer1 = {
.master = &omap54xx_l4_wkup_hwmod,
@@ -2163,6 +2206,7 @@ static struct omap_hwmod_ocp_if *omap54xx_hwmod_ocp_ifs[] __initdata = {
&omap54xx_l4_per__mmc4,
&omap54xx_l4_per__mmc5,
&omap54xx_l4_cfg__mpu,
+ &omap54xx_l4_cfg__spinlock,
&omap54xx_l4_wkup__timer1,
&omap54xx_l4_per__timer2,
&omap54xx_l4_per__timer3,
--
1.8.3.3
Add the hwspinlock device tree node for OMAP5 SoCs.
Signed-off-by: Suman Anna <[email protected]>
---
arch/arm/boot/dts/omap5.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index 07be2cd..449be92 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -276,6 +276,12 @@
ti,hwmods = "i2c5";
};
+ hwspinlock: spinlock@4a0f6000 {
+ compatible = "ti,omap4-hwspinlock";
+ reg = <0x4a0f6000 0x1000>;
+ ti,hwmods = "spinlock";
+ };
+
mcspi1: spi@48098000 {
compatible = "ti,omap4-mcspi";
reg = <0x48098000 0x200>;
--
1.8.3.3
Add the hwspinlock device tree node for AM33xx family
of SoCs.
Signed-off-by: Suman Anna <[email protected]>
---
arch/arm/boot/dts/am33xx.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index f9c5da9..4371257 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -228,6 +228,12 @@
status = "disabled";
};
+ hwspinlock: spinlock@480ca000 {
+ compatible = "ti,omap4-hwspinlock";
+ reg = <0x480ca000 0x1000>;
+ ti,hwmods = "spinlock";
+ };
+
wdt2: wdt@44e35000 {
compatible = "ti,omap3-wdt";
ti,hwmods = "wd_timer2";
--
1.8.3.3
Add the missing sysc configuration to the AM335 spinlock hwmod
data. This ensures that smart-idle is enabled whenever the module
is enabled by the driver.
Signed-off-by: Suman Anna <[email protected]>
---
arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
index 215894f..1084f2c 100644
--- a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
@@ -1278,8 +1278,21 @@ static struct omap_hwmod am33xx_spi1_hwmod = {
* spinlock provides hardware assistance for synchronizing the
* processes running on multiple processors
*/
+
+static struct omap_hwmod_class_sysconfig am33xx_spinlock_sysc = {
+ .rev_offs = 0x0000,
+ .sysc_offs = 0x0010,
+ .syss_offs = 0x0014,
+ .sysc_flags = (SYSC_HAS_AUTOIDLE | SYSC_HAS_CLOCKACTIVITY |
+ SYSC_HAS_ENAWAKEUP | SYSC_HAS_SIDLEMODE |
+ SYSC_HAS_SOFTRESET | SYSS_HAS_RESET_STATUS),
+ .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+ .sysc_fields = &omap_hwmod_sysc_type1,
+};
+
static struct omap_hwmod_class am33xx_spinlock_hwmod_class = {
.name = "spinlock",
+ .sysc = &am33xx_spinlock_sysc,
};
static struct omap_hwmod am33xx_spinlock_hwmod = {
--
1.8.3.3
The number of hwspinlocks are determined based on the value read
from the IP block's SYSSTATUS register. However, the module may
not be enabled and clocked, and the read may result in a bus error.
This particular issue is seen rather easily on AM33XX, since the
module wakeup is software controlled, and it is disabled out of
reset. Make sure the module is enabled and clocked before reading
the SYSSTATUS register.
Signed-off-by: Suman Anna <[email protected]>
---
drivers/hwspinlock/omap_hwspinlock.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/hwspinlock/omap_hwspinlock.c b/drivers/hwspinlock/omap_hwspinlock.c
index 1c99391..f73b00f 100644
--- a/drivers/hwspinlock/omap_hwspinlock.c
+++ b/drivers/hwspinlock/omap_hwspinlock.c
@@ -106,10 +106,23 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
if (!io_base)
return -ENOMEM;
+ /*
+ * make sure the module is enabled and clocked before reading
+ * the module SYSSTATUS register
+ */
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_get_sync(&pdev->dev);
+
/* Determine number of locks */
i = readl(io_base + SYSSTATUS_OFFSET);
i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;
+ /*
+ * runtime PM will make sure the clock of this module is
+ * enabled again iff at least one lock is requested
+ */
+ pm_runtime_put(&pdev->dev);
+
/* one of the four lsb's must be set, and nothing else */
if (hweight_long(i & 0xf) != 1 || i > 8) {
ret = -EINVAL;
@@ -129,12 +142,6 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
for (i = 0, hwlock = &bank->lock[0]; i < num_locks; i++, hwlock++)
hwlock->priv = io_base + LOCK_BASE_OFFSET + sizeof(u32) * i;
- /*
- * runtime PM will make sure the clock of this module is
- * enabled iff at least one lock is requested
- */
- pm_runtime_enable(&pdev->dev);
-
ret = hwspin_lock_register(bank, &pdev->dev, &omap_hwspinlock_ops,
base_id, num_locks);
if (ret)
@@ -143,9 +150,9 @@ static int omap_hwspinlock_probe(struct platform_device *pdev)
return 0;
reg_fail:
- pm_runtime_disable(&pdev->dev);
kfree(bank);
iounmap_base:
+ pm_runtime_disable(&pdev->dev);
iounmap(io_base);
return ret;
}
--
1.8.3.3
AM33XX device family also supports hwspinlocks. The IP
is identical to that of OMAP4/OMAP5, except for the
number of locks.
Signed-off-by: Suman Anna <[email protected]>
---
drivers/hwspinlock/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwspinlock/Kconfig b/drivers/hwspinlock/Kconfig
index 70637d2..34ff9cd 100644
--- a/drivers/hwspinlock/Kconfig
+++ b/drivers/hwspinlock/Kconfig
@@ -10,7 +10,7 @@ menu "Hardware Spinlock drivers"
config HWSPINLOCK_OMAP
tristate "OMAP Hardware Spinlock device"
- depends on ARCH_OMAP4 || SOC_OMAP5
+ depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_AM33XX
select HWSPINLOCK
help
Say y here to support the OMAP Hardware Spinlock device (firstly
--
1.8.3.3
Add the hwspinlock device tree node for OMAP4 family
of SoCs.
Signed-off-by: Suman Anna <[email protected]>
---
arch/arm/boot/dts/omap4.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 22d9f2b..a8cc274 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -248,6 +248,12 @@
clock-frequency = <48000000>;
};
+ hwspinlock: spinlock@4a0f6000 {
+ compatible = "ti,omap4-hwspinlock";
+ reg = <0x4a0f6000 0x1000>;
+ ti,hwmods = "spinlock";
+ };
+
i2c1: i2c@48070000 {
compatible = "ti,omap4-i2c";
reg = <0x48070000 0x100>;
--
1.8.3.3
On 09/17/2013 02:30 PM, Suman Anna wrote:
> Hi,
>
> This is an updated series for adding the device tree support to
> the OMAP hwspinlock driver. The series is based on 3.12-rc1, and
> includes patches on hwspinlock driver, OMAP hwmod data files and
> OMAP DTS files. The updated series adds new patches to enable the
> hwspinlock driver on OMAP5 and AM335x SoCs as well.
>
> Tested this on Panda4, BeagleBone Black and OMAP5 uEVM (with
> out-of-tree clock patches from Tero).
Ohad, Benoit,
A gentle reminder - can you please provide your review comments on these
patches. I am hoping that this series can make it to 3.13.
Paul,
The hwmod data patches needs to be merged only after the respective DT
node patches are merged, without which the hwmod entry will not have a
base address while enabling and idling (using sysc) the hwmod during
hwmod initialization.
regards
Suman
>
> Changes new in v2:
> - Added a new common DT binding documentation and OF helpers (Patch1),
> addressing the review comments from v1 [2]. The MSM support [1] needs
> to be reworked on top of this common patch.
> - Revised OMAP DT parse support to use the new OF helper (Patch2)
> - OMAP5 hwspinlock support including the hwmod entry and DT node (Patches 4, 5)
> - Add AM335x support to OMAP hwspinlock driver, including a fix
> needed in driver given that AM335 spinlock module requires s/w wakeup
> (Patches 6, 7)
> - AM335 DT node for spinlock, and a hwmod change to enable smart-idle
> for AM335 (Patches 8, 9). The sysc patch is not essential for AM335
> spinlock functionality, but is needed for smart-idling the IP when
> the module is enabled.
> - OMAP4 DT node patch is unchanged (Patch 3)
>
> v1:
> - Add DT parse support to OMAP hwspinlock driver
> - Add OMAP4 DT node and bindings information
> http://marc.info/?l=linux-omap&m=137823082308009&w=2
>
> [1] https://lkml.org/lkml/2013/8/14/528
> [2] http://marc.info/?t=137823090300005&r=1&w=2
>
>
> Suman Anna (9):
> hwspinlock/core: add common dt bindings and OF helpers
> hwspinlock/omap: add support for dt nodes
> ARM: dts: OMAP4: Add hwspinlock node
> ARM: OMAP5: hwmod data: Add spinlock data
> ARM: dts: OMAP5: Add hwspinlock node
> hwspinlock/omap: support AM33xx
> hwspinlock/omap: enable module before reading SYSSTATUS register
> ARM: dts: AM33XX: Add hwspinlock node
> ARM: AM33xx: hwmod_data: add the sysc configuration for spinlock
>
> .../devicetree/bindings/hwlock/hwlock.txt | 26 +++++++++
> .../devicetree/bindings/hwlock/omap-hwspinlock.txt | 31 +++++++++++
> arch/arm/boot/dts/am33xx.dtsi | 6 +++
> arch/arm/boot/dts/omap4.dtsi | 6 +++
> arch/arm/boot/dts/omap5.dtsi | 6 +++
> arch/arm/mach-omap2/Makefile | 3 --
> arch/arm/mach-omap2/hwspinlock.c | 60 ---------------------
> arch/arm/mach-omap2/omap_hwmod_33xx_data.c | 13 +++++
> arch/arm/mach-omap2/omap_hwmod_54xx_data.c | 44 ++++++++++++++++
> drivers/hwspinlock/Kconfig | 2 +-
> drivers/hwspinlock/hwspinlock_core.c | 61 +++++++++++++++++++++-
> drivers/hwspinlock/omap_hwspinlock.c | 44 ++++++++++++----
> include/linux/hwspinlock.h | 11 ++--
> 13 files changed, 233 insertions(+), 80 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt
> create mode 100644 Documentation/devicetree/bindings/hwlock/omap-hwspinlock.txt
> delete mode 100644 arch/arm/mach-omap2/hwspinlock.c
>
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 <[email protected]>
> ---
> .../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 0000000..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.
> +
> +- 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.
> 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 <[email protected]>
> *
> @@ -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
> +
> + ret = of_property_read_u32(dn, "hwlock-base-id", &val);
> + if (!ret)
> + ret = val;
> +#endif
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
> +
> +/**
> + * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks
> + * @dn: device node pointer
> + *
> + * This is an OF helper function that can be called by the underlying
> + * platform-specific implementations, to retrieve the number of locks
> + * present within a hwspinlock device instance.
> + *
> + * Returns a positive number of locks on success, -ENODEV on generic
> + * failure or an appropriate error code as returned by the OF layer
> + */
> +int of_hwspin_lock_get_num_locks(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
> +
> + ret = of_property_read_u32(dn, "hwlock-num-locks", &val);
> + if (!ret)
> + ret = val ? val : -ENODEV;
> +#endif
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
> +
> +/**
> * hwspin_lock_register() - register a new hw spinlock device
> * @bank: the hwspinlock device, which usually provides numerous hw locks
> * @dev: the backing device
> diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
> index 3343298..1f6a5b8 100644
> --- a/include/linux/hwspinlock.h
> +++ b/include/linux/hwspinlock.h
> @@ -1,7 +1,7 @@
> /*
> * Hardware spinlock public header
> *
> - * 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 <[email protected]>
> *
> @@ -26,6 +26,7 @@
> #define HWLOCK_IRQ 0x02 /* Disable interrupts, don't save state */
>
> struct device;
> +struct device_node;
> struct hwspinlock;
> struct hwspinlock_device;
> struct hwspinlock_ops;
> @@ -60,6 +61,8 @@ struct hwspinlock_pdata {
>
> #if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
>
> +int of_hwspin_lock_get_base_id(struct device_node *dn);
> +int of_hwspin_lock_get_num_locks(struct device_node *dn);
> int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
> const struct hwspinlock_ops *ops, int base_id, int num_locks);
> int hwspin_lock_unregister(struct hwspinlock_device *bank);
> @@ -80,9 +83,9 @@ void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
> * code path get compiled away. This way, if CONFIG_HWSPINLOCK is not
> * required on a given setup, users will still work.
> *
> - * The only exception is hwspin_lock_register/hwspin_lock_unregister, with which
> - * we _do_ want users to fail (no point in registering hwspinlock instances if
> - * the framework is not available).
> + * The only exception is hwspin_lock_register/hwspin_lock_unregister and
> + * associated OF helpers, with which we _do_ want users to fail (no point
> + * in registering hwspinlock instances if the framework is not available).
> *
> * Note: ERR_PTR(-ENODEV) will still be considered a success for NULL-checking
> * users. Others, which care, can still check this with IS_ERR.
> --
> 1.8.3.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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 <[email protected]>
> ---
> .../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 0000000..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
> +- reg: Contains the hwspinlock register address range (base
> + address and length)
> +- 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.
> +- 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.
If you are going to be explicit about this properties, you should probably be a bit more clear about them NOT being in the OMAP dts because of the reasons you specify.
> +
> +
> +Example:
> +
> +/* OMAP4 */
> +hwspinlock: spinlock@4a0f6000 {
> + compatible = "ti,omap4-hwspinlock";
> + reg = <0x4a0f6000 0x1000>;
> + ti,hwmods = "spinlock";
> +};
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On 09/27/2013 11:06 AM, 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 <[email protected]>
>> ---
>> .../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 0000000..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
>> +- reg: Contains the hwspinlock register address range (base
>> + address and length)
>> +- 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.
>> +- 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.
>
> If you are going to be explicit about this properties, you should probably be a bit more clear about them NOT being in the OMAP dts because of the reasons you specify.
You mean on the hwlock-base-id? I wanted to document how these
properties (do not) apply for OMAP, I will revise the text to be a bit
more explicit. Thanks for the review.
regards
Suman
Kumar,
On 09/27/2013 11:04 AM, 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 <[email protected]>
>> ---
>> .../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 0000000..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.
>> +
>> +- 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.
>> 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 <[email protected]>
>> *
>> @@ -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
I have added this specifically since my intention is to differentiate
the validity of the node vs the presence of the property within a node.
This property may be optional for some platforms and a must for some
others, and differentiating would allow the individual driver
implementations to make the distinction.
>
>> +
>> + ret = of_property_read_u32(dn, "hwlock-base-id", &val);
>> + if (!ret)
>> + ret = val;
>> +#endif
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
>> +
>> +/**
>> + * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks
>> + * @dn: device node pointer
>> + *
>> + * This is an OF helper function that can be called by the underlying
>> + * platform-specific implementations, to retrieve the number of locks
>> + * present within a hwspinlock device instance.
>> + *
>> + * Returns a positive number of locks on success, -ENODEV on generic
>> + * failure or an appropriate error code as returned by the OF layer
>> + */
>> +int of_hwspin_lock_get_num_locks(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
Same comment as above.
regards
Suman
>
>> +
>> + ret = of_property_read_u32(dn, "hwlock-num-locks", &val);
>> + if (!ret)
>> + ret = val ? val : -ENODEV;
>> +#endif
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
>> +
>> +/**
>> * hwspin_lock_register() - register a new hw spinlock device
>> * @bank: the hwspinlock device, which usually provides numerous hw locks
>> * @dev: the backing device
>> diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
>> index 3343298..1f6a5b8 100644
>> --- a/include/linux/hwspinlock.h
>> +++ b/include/linux/hwspinlock.h
>> @@ -1,7 +1,7 @@
>> /*
>> * Hardware spinlock public header
>> *
>> - * 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 <[email protected]>
>> *
>> @@ -26,6 +26,7 @@
>> #define HWLOCK_IRQ 0x02 /* Disable interrupts, don't save state */
>>
>> struct device;
>> +struct device_node;
>> struct hwspinlock;
>> struct hwspinlock_device;
>> struct hwspinlock_ops;
>> @@ -60,6 +61,8 @@ struct hwspinlock_pdata {
>>
>> #if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
>>
>> +int of_hwspin_lock_get_base_id(struct device_node *dn);
>> +int of_hwspin_lock_get_num_locks(struct device_node *dn);
>> int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
>> const struct hwspinlock_ops *ops, int base_id, int num_locks);
>> int hwspin_lock_unregister(struct hwspinlock_device *bank);
>> @@ -80,9 +83,9 @@ void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
>> * code path get compiled away. This way, if CONFIG_HWSPINLOCK is not
>> * required on a given setup, users will still work.
>> *
>> - * The only exception is hwspin_lock_register/hwspin_lock_unregister, with which
>> - * we _do_ want users to fail (no point in registering hwspinlock instances if
>> - * the framework is not available).
>> + * The only exception is hwspin_lock_register/hwspin_lock_unregister and
>> + * associated OF helpers, with which we _do_ want users to fail (no point
>> + * in registering hwspinlock instances if the framework is not available).
>> *
>> * Note: ERR_PTR(-ENODEV) will still be considered a success for NULL-checking
>> * users. Others, which care, can still check this with IS_ERR.
>> --
>> 1.8.3.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Sep 27, 2013, at 11:48 AM, Suman Anna wrote:
> Kumar,
>
> On 09/27/2013 11:04 AM, 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 <[email protected]>
>>> ---
>>> .../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 0000000..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.
>>> +
>>> +- 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.
Was meaning to say this before, another reason for hwlock-num-locks is to limit the # of locks available to the processors running linux vs what other processors in the SoC might be using.
>>> 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 <[email protected]>
>>> *
>>> @@ -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
>
> I have added this specifically since my intention is to differentiate
> the validity of the node vs the presence of the property within a node.
> This property may be optional for some platforms and a must for some
> others, and differentiating would allow the individual driver
> implementations to make the distinction.
Maybe add a comment for both cases.
>
>>
>>> +
>>> + ret = of_property_read_u32(dn, "hwlock-base-id", &val);
>>> + if (!ret)
>>> + ret = val;
>>> +#endif
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_base_id);
>>> +
>>> +/**
>>> + * of_hwspin_lock_get_num_locks() - OF helper to retrieve number of locks
>>> + * @dn: device node pointer
>>> + *
>>> + * This is an OF helper function that can be called by the underlying
>>> + * platform-specific implementations, to retrieve the number of locks
>>> + * present within a hwspinlock device instance.
>>> + *
>>> + * Returns a positive number of locks on success, -ENODEV on generic
>>> + * failure or an appropriate error code as returned by the OF layer
>>> + */
>>> +int of_hwspin_lock_get_num_locks(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
>
> Same comment as above.
>
> regards
> Suman
>
>>
>>> +
>>> + ret = of_property_read_u32(dn, "hwlock-num-locks", &val);
>>> + if (!ret)
>>> + ret = val ? val : -ENODEV;
>>> +#endif
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_hwspin_lock_get_num_locks);
>>> +
>>> +/**
>>> * hwspin_lock_register() - register a new hw spinlock device
>>> * @bank: the hwspinlock device, which usually provides numerous hw locks
>>> * @dev: the backing device
>>> diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
>>> index 3343298..1f6a5b8 100644
>>> --- a/include/linux/hwspinlock.h
>>> +++ b/include/linux/hwspinlock.h
>>> @@ -1,7 +1,7 @@
>>> /*
>>> * Hardware spinlock public header
>>> *
>>> - * 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 <[email protected]>
>>> *
>>> @@ -26,6 +26,7 @@
>>> #define HWLOCK_IRQ 0x02 /* Disable interrupts, don't save state */
>>>
>>> struct device;
>>> +struct device_node;
>>> struct hwspinlock;
>>> struct hwspinlock_device;
>>> struct hwspinlock_ops;
>>> @@ -60,6 +61,8 @@ struct hwspinlock_pdata {
>>>
>>> #if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE)
>>>
>>> +int of_hwspin_lock_get_base_id(struct device_node *dn);
>>> +int of_hwspin_lock_get_num_locks(struct device_node *dn);
>>> int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev,
>>> const struct hwspinlock_ops *ops, int base_id, int num_locks);
>>> int hwspin_lock_unregister(struct hwspinlock_device *bank);
>>> @@ -80,9 +83,9 @@ void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
>>> * code path get compiled away. This way, if CONFIG_HWSPINLOCK is not
>>> * required on a given setup, users will still work.
>>> *
>>> - * The only exception is hwspin_lock_register/hwspin_lock_unregister, with which
>>> - * we _do_ want users to fail (no point in registering hwspinlock instances if
>>> - * the framework is not available).
>>> + * The only exception is hwspin_lock_register/hwspin_lock_unregister and
>>> + * associated OF helpers, with which we _do_ want users to fail (no point
>>> + * in registering hwspinlock instances if the framework is not available).
>>> *
>>> * Note: ERR_PTR(-ENODEV) will still be considered a success for NULL-checking
>>> * users. Others, which care, can still check this with IS_ERR.
>>> --
>>> 1.8.3.3
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Sep 27, 2013, at 11:21 AM, Suman Anna wrote:
> On 09/27/2013 11:06 AM, 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 <[email protected]>
>>> ---
>>> .../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 0000000..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
>>> +- reg: Contains the hwspinlock register address range (base
>>> + address and length)
>>> +- 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.
>>> +- 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.
>>
>> If you are going to be explicit about this properties, you should probably be a bit more clear about them NOT being in the OMAP dts because of the reasons you specify.
>
> You mean on the hwlock-base-id? I wanted to document how these
> properties (do not) apply for OMAP, I will revise the text to be a bit
> more explicit. Thanks for the review.
I meant for both, I think slightly revised text will help.
- k
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Kumar,
>>
>> On 09/27/2013 11:04 AM, 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 <[email protected]>
>>>> ---
>>>> .../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 0000000..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.
>>>> +
>>>> +- 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.
>
> Was meaning to say this before, another reason for hwlock-num-locks is to limit the # of locks available to the processors running linux vs what other processors in the SoC might be using.
Yes, I understand the usecase/scenario since we had a similar need on
our product. That said, I guess this should be left to the individual
driver implementation/integration/documentation since this can be
achieved in different ways and depends on how you partition the
resources on your system (static partition vs resource reservation at
linux init time). Mentioning that here begs the question if you are
gonna use the starting 'n' locks, ending 'n' locks or range of 'n' locks
beginning in the middle. It would also imply it has to work together
with the hwlock-base-id as well in the last case. I would prefer to keep
the documentation generic here.
>
>
>>>> 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 <[email protected]>
>>>> *
>>>> @@ -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
>>
>> I have added this specifically since my intention is to differentiate
>> the validity of the node vs the presence of the property within a node.
>> This property may be optional for some platforms and a must for some
>> others, and differentiating would allow the individual driver
>> implementations to make the distinction.
>
> Maybe add a comment for both cases.
OK, will do.
regards
Suman
Hi
On Fri, 27 Sep 2013, Suman Anna wrote:
> Paul,
> The hwmod data patches needs to be merged only after the respective DT
> node patches are merged, without which the hwmod entry will not have a
> base address while enabling and idling (using sysc) the hwmod during
> hwmod initialization.
Hmm, ideally there shouldn't be a merge dependency here. Could you please
send a patch to the hwmod core that logs a warning and skips the operation
in these circumstances? Or maybe just skips the registration of the
device?
- Paul
On 09/29/2013 10:12 PM, Paul Walmsley wrote:
> Hi
>
> On Fri, 27 Sep 2013, Suman Anna wrote:
>
>> Paul,
>> The hwmod data patches needs to be merged only after the respective DT
>> node patches are merged, without which the hwmod entry will not have a
>> base address while enabling and idling (using sysc) the hwmod during
>> hwmod initialization.
>
> Hmm, ideally there shouldn't be a merge dependency here. Could you please
> send a patch to the hwmod core that logs a warning and skips the operation
> in these circumstances? Or maybe just skips the registration of the
> device?
>
Yes, will look into this. This is not specific to hwspinlock, but may be
seen with others as well if the omap_hwmod_addr_space or its equivalent
is not populated.
regards
Suman
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 <[email protected]>
> > ---
> > .../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 0000000..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 <[email protected]>
> > *
> > @@ -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
>
> > +
> > + ret = of_property_read_u32(dn, "hwlock-base-id", &val);
> > + if (!ret)
> > + ret = val;
> > +#endif
Do we need the CONFIG_OF check? of_property_read_u32 is defined to
return -ENOSYS if CONFIG_OF isn't defined. Or would that confuse a
higher layer?
Similarly elsewhere in the file.
Cheers,
Mark.
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 <[email protected]>
> > ---
> > .../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 0000000..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.
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 <[email protected]>
>>> ---
>>> .../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 0000000..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.
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.
>
>>> +
>>> +- 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).
Agreed.
>
>>> 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 <[email protected]>
>>> *
>>> @@ -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
>>
>>> +
>>> + ret = of_property_read_u32(dn, "hwlock-base-id", &val);
>>> + if (!ret)
>>> + ret = val;
>>> +#endif
>
> Do we need the CONFIG_OF check? of_property_read_u32 is defined to
> return -ENOSYS if CONFIG_OF isn't defined. Or would that confuse a
> higher layer?
These are primarily OF helpers and provided for the SoC implementation
drivers, and I have used the CONFIG_OF check within the function to
streamline the function prototypes and behavior in the common
hwspinlock.h header file between combinations of CONFIG_HWSPINLOCK and
CONFIG_OF.
The check for !dn is done deliberately to help the implementation drivers.
regards
Suman
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 <[email protected]>
>>> ---
>>> .../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 0000000..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.
>
>>> +- 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.
>
>>> +- 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.
>
>>> +- 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.
Thanks
Suman
On Tue, 17 Sep 2013, Suman Anna wrote:
> Add the missing sysc configuration to the AM335 spinlock hwmod
> data. This ensures that smart-idle is enabled whenever the module
> is enabled by the driver.
>
> Signed-off-by: Suman Anna <[email protected]>
Thanks, queued. You can omit this one from future reposts of this series.
- Paul
On Tue, 17 Sep 2013, Suman Anna wrote:
> Add the hwmod data for the spinlock IP in OMAP5 SoC.
> This is needed to be able to enable the OMAP spinlock
> support for OMAP5.
>
> Signed-off-by: Suman Anna <[email protected]>
Thanks, queued. You can omit this one from future reposts of this series.
- Paul
On 10/09/2013 02:12 AM, Paul Walmsley wrote:
> On Tue, 17 Sep 2013, Suman Anna wrote:
>
>> Add the hwmod data for the spinlock IP in OMAP5 SoC.
>> This is needed to be able to enable the OMAP spinlock
>> support for OMAP5.
>>
>> Signed-off-by: Suman Anna <[email protected]>
>
> Thanks, queued. You can omit this one from future reposts of this series.
>
Thanks. I will be reposting the series (soon), and will omit this patch
and the AM335 sysc patch in the repost.
regards
Suman
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 <[email protected]>
> >>> ---
> >>> .../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 0000000..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 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).
>
> Agreed.
>
> >
> >>> 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 <[email protected]>
> >>> *
> >>> @@ -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
> >>
> >>> +
> >>> + ret = of_property_read_u32(dn, "hwlock-base-id", &val);
> >>> + if (!ret)
> >>> + ret = val;
> >>> +#endif
> >
> > Do we need the CONFIG_OF check? of_property_read_u32 is defined to
> > return -ENOSYS if CONFIG_OF isn't defined. Or would that confuse a
> > higher layer?
>
> These are primarily OF helpers and provided for the SoC implementation
> drivers, and I have used the CONFIG_OF check within the function to
> streamline the function prototypes and behavior in the common
> hwspinlock.h header file between combinations of CONFIG_HWSPINLOCK and
> CONFIG_OF.
Ok. Due to the !CONFIG_OF stub for of_property_read_u32, and the check for !dn
done in of_property_read_u32, you could just have:
int of_hwspin_lock_get_base_id(struct device_node *dn)
{
u32 val;
if (of_property_read_u32(dn, "hwlock-base-id", &val) != 0)
return -ENODEV;
return val;
}
Which would work regardless of CONFIG_OF. That said, I don't think this
function is necessary because a phandle + args approach would be fundamantally
better.
>
> The check for !dn is done deliberately to help the implementation drivers.
As I mentioned above, of_property_read_u32 already checks for !dn, so the check
here is redundant.
Cheers,
Mark.
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 <[email protected]>
> >>> ---
> >>> .../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 0000000..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.
>
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 <[email protected]>
>>>>> ---
>>>>> .../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 0000000..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.
Yes, that is the change I have made in my current working set as well.
>
>>
>>>
>>>>> +- 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?
No, they are not shared with other devices. These are like revision
register, and a SYSCONFIG register which is used by the OMAP hwmod
layer. This register definition is in line with other modules on OMAP.
>
>>
>>>
>>>>> +- 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.
I have removed this property altogether in my current working set. Will
post the v3 of the series soon.
regards
Suman