From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Andersson Subject: Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks Date: Mon, 5 Aug 2019 10:46:59 -0700 Message-ID: <20190805174659.GA23928@tuxbook-pro> References: <1552492237-28810-1-git-send-email-fabien.dessenne@st.com> <20190801191403.GA7234@tuxbook-pro> <1a057176-81ab-e302-4375-2717ceef6924@st.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <1a057176-81ab-e302-4375-2717ceef6924@st.com> Sender: linux-kernel-owner@vger.kernel.org To: Fabien DESSENNE Cc: Ohad Ben-Cohen , Rob Herring , Mark Rutland , Maxime Coquelin , Alexandre TORGUE , Jonathan Corbet , "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 List-Id: devicetree@vger.kernel.org On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote: > > On 01/08/2019 9:14 PM, Bjorn Andersson wrote: > > On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote: > > > >> 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' > >> 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. > 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. > 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. > 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. > 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. > > 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 > BR > > Fabien > > > > > Regards, > > Bjorn > > > >> 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 +++++++++++++++++----- > >> drivers/hwspinlock/hwspinlock_internal.h | 2 + > >> 7 files changed, 108 insertions(+), 31 deletions(-) > >> > >> -- > >> 2.7.4 > >>