Arnd,
> Some system control registers need hardware spinlock to synchronize
> between the multiple subsystems, so we should add hardware spinlock
> support for syscon.
>
> Signed-off-by: Baolin Wang <[email protected]>
> Acked-by: Rob Herring <[email protected]>
> ---
> Changes since v5:
> - Fix the case that hwspinlock is not enabled.
>
> Changes since v4:
> - Add one exapmle to show how to add hwlock.
> - Fix the coding style issue.
>
> Changes since v3:
> - Add error handling for of_hwspin_lock_get_id()
>
> Changes since v2:
> - Add acked tag from Rob.
>
> Changes since v1:
> - Remove timeout configuration.
> - Modify the binding file to add hwlocks.
> ---
> Documentation/devicetree/bindings/mfd/syscon.txt | 8 +++++++
> drivers/mfd/syscon.c | 25 ++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> 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;
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Fri, Dec 15, 2017 at 11:42 AM, Lee Jones <[email protected]> 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 15 December 2017 at 21:13, Arnd Bergmann <[email protected]> wrote:
> On Fri, Dec 15, 2017 at 11:42 AM, Lee Jones <[email protected]> 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.
--
Baolin.wang
Best Regards
On Mon, Dec 18, 2017 at 7:54 AM, Baolin Wang <[email protected]> wrote:
> On 15 December 2017 at 21:13, Arnd Bergmann <[email protected]> wrote:
>> On Fri, Dec 15, 2017 at 11:42 AM, Lee Jones <[email protected]> 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
On 18 December 2017 at 20:44, Arnd Bergmann <[email protected]> wrote:
> On Mon, Dec 18, 2017 at 7:54 AM, Baolin Wang <[email protected]> wrote:
>> On 15 December 2017 at 21:13, Arnd Bergmann <[email protected]> wrote:
>>> On Fri, Dec 15, 2017 at 11:42 AM, Lee Jones <[email protected]> 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?
Um, why regmap doesn't accept a zero lock-id, that because regmap will
reguest hwlock depending on the 'regmap_config->hwlock_id' is not
zero, if regmap regard a zero lock-id as valid which will affect other
'struct regmap_config' definition. So users should not assign the zero
lock-id to regmap.
Now of_hwspin_lock_get_id() can return 0 as valid, which depend on
what is the base id registered by hwspinlock driver. So you think we
should not regard 0 as valid from of_hwspin_lock_get_id(), I can try
to send another patch to fix.
But for this patch I still think we need regard the zero lock-id as
invalid and gave error messages to users.
--
Baolin.wang
Best Regards
On Tue, Dec 19, 2017 at 02:55:47PM +0800, Baolin Wang wrote:
> Um, why regmap doesn't accept a zero lock-id, that because regmap will
> reguest hwlock depending on the 'regmap_config->hwlock_id' is not
> zero, if regmap regard a zero lock-id as valid which will affect other
> 'struct regmap_config' definition. So users should not assign the zero
> lock-id to regmap.
> Now of_hwspin_lock_get_id() can return 0 as valid, which depend on
> what is the base id registered by hwspinlock driver. So you think we
> should not regard 0 as valid from of_hwspin_lock_get_id(), I can try
> to send another patch to fix.
If 0 is a valid hwspinlock ID you need to add a flag to the regmap
config to say if a hwspinlock should be used. That way we're not
rejecting valid values.
On 19 December 2017 at 17:21, Mark Brown <[email protected]> wrote:
> On Tue, Dec 19, 2017 at 02:55:47PM +0800, Baolin Wang wrote:
>
>> Um, why regmap doesn't accept a zero lock-id, that because regmap will
>> reguest hwlock depending on the 'regmap_config->hwlock_id' is not
>> zero, if regmap regard a zero lock-id as valid which will affect other
>> 'struct regmap_config' definition. So users should not assign the zero
>> lock-id to regmap.
>
>> Now of_hwspin_lock_get_id() can return 0 as valid, which depend on
>> what is the base id registered by hwspinlock driver. So you think we
>> should not regard 0 as valid from of_hwspin_lock_get_id(), I can try
>> to send another patch to fix.
>
> If 0 is a valid hwspinlock ID you need to add a flag to the regmap
> config to say if a hwspinlock should be used. That way we're not
> rejecting valid values.
Fine, that seems reasonable if you prefer another new flag to indicate
if a hwspinlock should be used. Thanks Arnd and Mark's comments.
--
Baolin.wang
Best Regards