[PATCH v3] mfd: syscon: Add hardware spinlock support

[PATCH v3] mfd: syscon: Add hardware spinlock support

[Baolin Wang]

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 <baolin.wang@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes since v2:
 - Add acked tag from Rob.

Changes since v1:
 - Remove timeout configuration.
 - Modify the binding file to add hwlocks.

Note: The hwspinlock support in regmap was merged by commit:
(8698b9364710: "regmap: Add hardware spinlock support")
The hwspinlock was changed to a bool by commit:
(d048236dfdfe: "hwspinlock: Change hwspinlock to a bool")
---
 Documentation/devicetree/bindings/mfd/syscon.txt | 1 +
 drivers/mfd/syscon.c                             | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
index 8b92d45..f464cc8 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.txt
+++ b/Documentation/devicetree/bindings/mfd/syscon.txt
@@ -16,6 +16,7 @@ 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 {
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index b93fe4c..f1dccce 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,12 @@ 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;
+	}
+
 	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;
-- 
1.9.1

Re: [PATCH v3] mfd: syscon: Add hardware spinlock support

[Arnd Bergmann]

On Mon, Nov 20, 2017 at 7:54 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> Some system control registers need hardware spinlock to synchronize
> between the multiple subsystems, so we should add hardware spinlock
> support for syscon.
>

> @@ -87,6 +88,12 @@ 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;
> +       }

Hi Baolin,

The error handling here seems insufficient, I think we have to treat
the following cases separately:

- ret>0 (this is fine)
- no spinlock in DT: of_hwspin_lock_get_id currently returns -EINVAL,
  we probably want to use a different return code (maybe -ESRCH ?)
  here so we can tell the difference beween no spinlock, and an
  invalid spinlock
- deferred probing: if the spinlock is there but the driver is not
  yet available, we may need to propagate the -EPROBE_DEFER
  here
- other error: this would probably be a fatal condition here, and
  we should print a warning and clean up.

       Arnd

Re: [PATCH v3] mfd: syscon: Add hardware spinlock support

[Baolin Wang]

Hi Arnd,

On 29 November 2017 at 18:07, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Nov 20, 2017 at 7:54 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> Some system control registers need hardware spinlock to synchronize
>> between the multiple subsystems, so we should add hardware spinlock
>> support for syscon.
>>
>
>> @@ -87,6 +88,12 @@ 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;
>> +       }
>
> Hi Baolin,
>
> The error handling here seems insufficient, I think we have to treat
> the following cases separately:
>
> - ret>0 (this is fine)
> - no spinlock in DT: of_hwspin_lock_get_id currently returns -EINVAL,
>   we probably want to use a different return code (maybe -ESRCH ?)
>   here so we can tell the difference beween no spinlock, and an
>   invalid spinlock
> - deferred probing: if the spinlock is there but the driver is not
>   yet available, we may need to propagate the -EPROBE_DEFER
>   here
> - other error: this would probably be a fatal condition here, and
>   we should print a warning and clean up.

You are correct and I will add some error handling. Thanks for your comments.

-- 
Baolin.wang
Best Regards