Hi Suman,

Could you please let us know your thoughts or comments?

BR

Fabien

-----Original Message-----
From: Bjorn Andersson <redacted>
Sent: lundi 5 août 2019 19:47
To: Fabien DESSENNE <redacted>
Cc: Ohad Ben-Cohen <redacted>; Rob Herring <robh+dt@kernel.org>;
Mark Rutland [off-list ref]; Maxime Coquelin
[off-list ref]; Alexandre TORGUE
[off-list ref]; Jonathan Corbet [off-list ref]; linux-
remoteproc@vger.kernel.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-arm-
kernel@lists.infradead.org; linux-doc@vger.kernel.org; Benjamin GAIGNARD
[off-list ref]
Subject: Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks

On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:

quoted

On 01/08/2019 9:14 PM, Bjorn Andersson wrote:

quoted

On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:

quoted

The current implementation does not allow two different devices to
use a common hwspinlock. This patch set proposes to have, as an
option, some hwspinlocks shared between several users.

Below is an example that explain the need for this:
	exti: interrupt-controller@5000d000 {
		compatible = "st,stm32mp1-exti", "syscon";
		interrupt-controller;
		#interrupt-cells = <2>;
		reg = <0x5000d000 0x400>;
		hwlocks = <&hsem 1>;
	};
The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
With the current hwspinlock implementation, only the first driver
succeeds in requesting (hwspin_lock_request_specific) the hwlock.
The second request fails.


The proposed approach does not modify the API, but extends the DT

'hwlocks'

quoted

quoted

quoted

property with a second optional parameter (the first one identifies
an
hwlock) that specifies whether an hwlock is requested for exclusive
usage (current behavior) or can be shared between several users.
Examples:
	hwlocks = <&hsem 8>;	Ref to hwlock #8 for exclusive usage
	hwlocks = <&hsem 8 0>;	Ref to hwlock #8 for exclusive (0) usage
	hwlocks = <&hsem 8 1>;	Ref to hwlock #8 for shared (1) usage

As a constraint, the #hwlock-cells value must be 1 or 2.
In the current implementation, this can have theorically any value but:
- all of the exisiting drivers use the same value : 1.
- the framework supports only one value : 1 (see implementation of
   of_hwspin_lock_simple_xlate())
Hence, it shall not be a problem to restrict this value to 1 or 2
since it won't break any driver.

Hi Fabien,

Your series looks good, but it makes me wonder why the hardware
locks should be an exclusive resource.

How about just making all (specific) locks shared?

Hi Bjorn,

Making all locks shared is a possible implementation (my first
implementation was going this way) but there are some drawbacks we
must be aware of:

A/ This theoretically break the legacy behavior (the legacy works with
exclusive (UNUSED radix tag) usage). As a consequence, an existing
driver that is currently failing to request a lock (already claimed by
another
user) would now work fine. Not sure that there are such drivers, so
this point is probably not a real issue.

Right, it's possible that a previously misconfigured system now successfully
probes more than one device that uses a particular spinlock. But such system
would be suffering from issues related to e.g.
probe ordering.

So I think we should ignore this issue.

quoted

B/ This would introduce some inconsistency between the two 'request'
API which are hwspin_lock_request() and hwspin_lock_request_specific().
hwspin_lock_request() looks for an unused lock, so requests for an
exclusive usage. On the other side, request_specific() would request shared

locks.

quoted

Worst the following sequence can transform an exclusive usage into a
shared

There is already an inconsistency in between these; as with above any system
that uses both request() and request_specific() will be suffering from intermittent
failures due to probe ordering.

quoted

one:
   -hwspin_lock_request() -> returns Id#0 (exclusive)
   -hwspin_lock_request() -> returns Id#1 (exclusive)
   -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0
shared Honestly I am not sure that this is a real issue, but it's
better to have it in mind before we take ay decision

The case where I can see a
problem with this would be if the two clients somehow would nest their locking
regions.

But generally I think this could consider this an improvement, because the
request_specific() would now be able to acquire its hwlock, with some additional
contention due to the multiple use.

quoted

I could not find any driver using the hwspin_lock_request() API, we
may decide to remove (or to make deprecated) this API, having
everything 'shared without any conditions'.

It would be nice to have an upstream user of this API.

quoted

I can see three options:
1- Keep my initial proposition
2- Have hwspin_lock_request_specific() using shared locks and
    hwspin_lock_request() using unused (so 'initially' exclusive) locks.
3- Have hwspin_lock_request_specific() using shared locks and
    remove/make deprecated hwspin_lock_request().

Just let me know what is your preference.

I think we should start with #2 and would like input from e.g. Suman regarding #3.

Regards,
Bjorn

quoted

BR

Fabien

quoted

Regards,
Bjorn

quoted

Fabien Dessenne (6):
   dt-bindings: hwlock: add support of shared locks
   hwspinlock: allow sharing of hwspinlocks
   dt-bindings: hwlock: update STM32 #hwlock-cells value
   ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
   ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
   ARM: dts: stm32: hwlocks for GPIO for stm32mp157

  .../devicetree/bindings/hwlock/hwlock.txt          | 27 +++++--
  .../bindings/hwlock/st,stm32-hwspinlock.txt        |  6 +-
  Documentation/hwspinlock.txt                       | 10 ++-
  arch/arm/boot/dts/stm32mp157-pinctrl.dtsi          |  2 +
  arch/arm/boot/dts/stm32mp157c.dtsi                 | 10 +++
  drivers/hwspinlock/hwspinlock_core.c               | 82 +++++++++++++++++-

----

quoted

quoted

quoted

  drivers/hwspinlock/hwspinlock_internal.h           |  2 +
  7 files changed, 108 insertions(+), 31 deletions(-)

--
2.7.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel