| Message ID | 87ec50c846bbc7afc09ba0855aba1cdea6473308.1512048582.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] mfd: syscon: Add hardware spinlock support | expand |
On Fri, Dec 15, 2017 at 11:42 AM, Lee Jones <lee.jones@linaro.org> wrote: >> @@ -87,6 +88,30 @@ static struct syscon *of_syscon_register(struct device_node *np) >> if (ret) >> reg_io_width = 4; >> >> + ret = of_hwspin_lock_get_id(np, 0); >> + if (ret > 0) { >> + syscon_config.hwlock_id = ret; >> + syscon_config.hwlock_mode = HWLOCK_IRQSTATE; >> + } else { >> + switch (ret) { >> + case -ENOENT: >> + /* Ignore missing hwlock, it's optional. */ >> + break; >> + case 0: >> + /* In case of the HWSPINLOCK is not enabled. */ >> + if (!IS_ENABLED(CONFIG_HWSPINLOCK)) >> + break; >> + >> + ret = -EINVAL; >> + /* fall-through */ >> + default: >> + pr_err("Failed to retrieve valid hwlock: %d\n", ret); >> + /* fall-through */ >> + case -EPROBE_DEFER: >> + goto err_regmap; >> + } The 'case 0' seems odd here, are we sure that this is always a failure? From the of_hwspin_lock_get_id() definition it looks like zero might be valid, and the !CONFIG_HWSPINLOCK implementation appears to be written so that we should consider '0' valid but unused and silently continue with that. If that is generally not the intended use, it should probably return -EINVAL or something like that. Arnd
On Mon, Dec 18, 2017 at 7:54 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 15 December 2017 at 21:13, Arnd Bergmann <arnd@arndb.de> wrote: >> On Fri, Dec 15, 2017 at 11:42 AM, Lee Jones <lee.jones@linaro.org> wrote: >> >>>> @@ -87,6 +88,30 @@ static struct syscon *of_syscon_register(struct device_node *np) >>>> if (ret) >>>> reg_io_width = 4; >>>> >>>> + ret = of_hwspin_lock_get_id(np, 0); >>>> + if (ret > 0) { >>>> + syscon_config.hwlock_id = ret; >>>> + syscon_config.hwlock_mode = HWLOCK_IRQSTATE; >>>> + } else { >>>> + switch (ret) { >>>> + case -ENOENT: >>>> + /* Ignore missing hwlock, it's optional. */ >>>> + break; >>>> + case 0: >>>> + /* In case of the HWSPINLOCK is not enabled. */ >>>> + if (!IS_ENABLED(CONFIG_HWSPINLOCK)) >>>> + break; >>>> + >>>> + ret = -EINVAL; >>>> + /* fall-through */ >>>> + default: >>>> + pr_err("Failed to retrieve valid hwlock: %d\n", ret); >>>> + /* fall-through */ >>>> + case -EPROBE_DEFER: >>>> + goto err_regmap; >>>> + } >> >> The 'case 0' seems odd here, are we sure that this is always a failure? >> From the of_hwspin_lock_get_id() definition it looks like zero might >> be valid, and the !CONFIG_HWSPINLOCK implementation appears >> to be written so that we should consider '0' valid but unused and >> silently continue with that. If that is generally not the intended >> use, it should probably return -EINVAL or something like that. > > Yes, 0 is valid for of_hwspin_lock_get_id(), but if we pass 'hwlock id > = 0' to regmap, the regmap core will not regard it as a valid hwlock > id to request the hwlock and will use default mutex lock instead of > hwlock, which will cause problems. Meanwhile if we silently continue > with case 0, users will not realize that they set one invalid hwlock > id to regmap core, so here we regarded case 0 as one invalid id to > print error messages for users. Something else still seems wrong then: If regmap doesn't accept a zero lock-id, then of_hwspin_lock_get_id() should never return that as a valid ID, right? Arnd
diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt index 8b92d45..25d9e9c 100644 --- a/Documentation/devicetree/bindings/mfd/syscon.txt +++ b/Documentation/devicetree/bindings/mfd/syscon.txt @@ -16,9 +16,17 @@ Required properties: Optional property: - reg-io-width: the size (in bytes) of the IO accesses that should be performed on the device. +- hwlocks: reference to a phandle of a hardware spinlock provider node. Examples: gpr: iomuxc-gpr@20e0000 { compatible = "fsl,imx6q-iomuxc-gpr", "syscon"; reg = <0x020e0000 0x38>; + hwlocks = <&hwlock1 1>; +}; + +hwlock1: hwspinlock@40500000 { + ... + reg = <0x40500000 0x1000>; + #hwlock-cells = <1>; }; diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index b93fe4c..a7ae391 100644 --- a/drivers/mfd/syscon.c +++ b/drivers/mfd/syscon.c @@ -13,6 +13,7 @@ */ #include <linux/err.h> +#include <linux/hwspinlock.h> #include <linux/io.h> #include <linux/module.h> #include <linux/list.h> @@ -87,6 +88,30 @@ static struct syscon *of_syscon_register(struct device_node *np) if (ret) reg_io_width = 4; + ret = of_hwspin_lock_get_id(np, 0); + if (ret > 0) { + syscon_config.hwlock_id = ret; + syscon_config.hwlock_mode = HWLOCK_IRQSTATE; + } else { + switch (ret) { + case -ENOENT: + /* Ignore missing hwlock, it's optional. */ + break; + case 0: + /* In case of the HWSPINLOCK is not enabled. */ + if (!IS_ENABLED(CONFIG_HWSPINLOCK)) + break; + + ret = -EINVAL; + /* fall-through */ + default: + pr_err("Failed to retrieve valid hwlock: %d\n", ret); + /* fall-through */ + case -EPROBE_DEFER: + goto err_regmap; + } + } + syscon_config.reg_stride = reg_io_width; syscon_config.val_bits = reg_io_width * 8; syscon_config.max_register = resource_size(&res) - reg_io_width;