From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fabien DESSENNE Subject: RE: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks Date: Tue, 6 Aug 2019 07:43:37 +0000 Message-ID: <543ee96699164a3cb95fe6a9aab54eae@SFHDAG5NODE3.st.com> References: <1552492237-28810-1-git-send-email-fabien.dessenne@st.com> <20190801191403.GA7234@tuxbook-pro> <1a057176-81ab-e302-4375-2717ceef6924@st.com> <20190805174659.GA23928@tuxbook-pro> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20190805174659.GA23928@tuxbook-pro> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: "s-anna@ti.com" 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 , Bjorn Andersson List-Id: devicetree@vger.kernel.org Hi Suman, Could you please let us know your thoughts or comments? BR Fabien > -----Original Message----- > From: Bjorn Andersson > Sent: lundi 5 ao=FBt 2019 19:47 > 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-a= rm- > kernel@lists.infradead.org; linux-doc@vger.kernel.org; Benjamin GAIGNARD > > Subject: Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks >=20 > On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote: >=20 > > > > 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 =3D "st,stm32mp1-exti", "syscon"; > > >> interrupt-controller; > > >> #interrupt-cells =3D <2>; > > >> reg =3D <0x5000d000 0x400>; > > >> hwlocks =3D <&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 =3D <&hsem 8>; Ref to hwlock #8 for exclusive usage > > >> hwlocks =3D <&hsem 8 0>; Ref to hwlock #8 for exclusive (0) usage > > >> hwlocks =3D <&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 b= ut: > > >> - 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. > > >=20 > Right, it's possible that a previously misconfigured system now successfu= lly > probes more than one device that uses a particular spinlock. But such sys= tem > would be suffering from issues related to e.g. > probe ordering. >=20 > So I think we should ignore this issue. >=20 > > 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 sh= ared > locks. > > Worst the following sequence can transform an exclusive usage into a > > shared > > >=20 > There is already an inconsistency in between these; as with above any sys= tem > that uses both request() and request_specific() will be suffering from in= termittent > failures due to probe ordering. >=20 > > one: > > =A0 -hwspin_lock_request() -> returns Id#0 (exclusive) > > =A0 -hwspin_lock_request() -> returns Id#1 (exclusive) > > =A0 -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 >=20 > The case where I can see a > problem with this would be if the two clients somehow would nest their lo= cking > regions. >=20 > But generally I think this could consider this an improvement, because th= e > request_specific() would now be able to acquire its hwlock, with some add= itional > contention due to the multiple use. >=20 > > 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'. > > >=20 > It would be nice to have an upstream user of this API. >=20 > > > > I can see three options: > > 1- Keep my initial proposition > > 2- Have hwspin_lock_request_specific() using shared locks and > > =A0=A0 hwspin_lock_request() using unused (so 'initially' exclusive) l= ocks. > > 3- Have hwspin_lock_request_specific() using shared locks and > > =A0=A0 remove/make deprecated hwspin_lock_request(). > > > > Just let me know what is your preference. > > >=20 > I think we should start with #2 and would like input from e.g. Suman rega= rding #3. >=20 > Regards, > Bjorn >=20 > > 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 > > >>