On Sat, Mar 06, 2021 at 12:05:22PM +0100, Wilken Gottwalt wrote: > On Tue, 2 Mar 2021 18:20:02 +0100 > Maxime Ripard <max...@cerno.tech> wrote: > > > Hi, > > > > On Mon, Mar 01, 2021 at 03:06:08PM +0100, Wilken Gottwalt wrote: > > > On Mon, 1 Mar 2021 14:13:05 +0100 > > > Maxime Ripard <mrip...@kernel.org> wrote: > > > > > > > On Sat, Feb 27, 2021 at 02:03:54PM +0100, Wilken Gottwalt wrote: > > > > > Adds the sun6i_hwspinlock driver for the hardware spinlock unit found > > > > > in > > > > > most of the sun6i compatible SoCs. > > > > > > > > > > This unit provides at least 32 spinlocks in hardware. The > > > > > implementation > > > > > supports 32, 64, 128 or 256 32bit registers. A lock can be taken by > > > > > reading a register and released by writing a 0 to it. This driver > > > > > supports all 4 spinlock setups, but for now only the first setup (32 > > > > > locks) seem to exist in available devices. This spinlock unit is > > > > > shared > > > > > between all ARM cores and the embedded companion core. All of them can > > > > > take/release a lock with a single cycle operation. It can be used to > > > > > sync access to devices shared by the ARM cores and the companion core. > > > > > > > > > > There are two ways to check if a lock is taken. The first way is to > > > > > read > > > > > a lock. If a 0 is returned, the lock was free and is taken now. If an > > > > > 1 > > > > > is returned, the caller has to try again. Which means the lock is > > > > > taken. > > > > > The second way is to read a 32bit wide status register where every bit > > > > > represents one of the 32 first locks. According to the datasheets this > > > > > status register supports only the 32 first locks. This is the reason > > > > > the > > > > > first way (lock read/write) approach is used to be able to cover all > > > > > 256 > > > > > locks in future devices. The driver also reports the amount of > > > > > supported > > > > > locks via debugfs. > > > > > > > > > > Signed-off-by: Wilken Gottwalt <wilken.gottw...@posteo.net> > > > > > > Nope, I had to replace the devm_hwspin_lock_register function by the > > > hwspin_lock_register function because like Bjorn pointed out that it can > > > fail and needs to handled correctly. And having a devm_* function does not > > > play well with the non-devm clock/reset functions and winding back if an > > > error occurs. It also messes with the call order in the remove function. > > > So > > > I went back to the classic way where I have full control over the call > > > order. > > > > If you're talking about the clock and reset line reassertion, I don't > > really see what the trouble is. Sure, it's not going to be in the exact > > same order in remove, but it's still going to execute in the proper > > order (ie, clock disable, then reset disable, then clock put and reset > > put). And you can use devm_add_action if you want to handle things > > automatically. > > See, in v5 zje result of devm_hwspin_lock_register was returned directly. The > remove callback or the bank_fail/clk_fail labels would not run, if the > registering > fails. In v6 it is fixed.
Yeah, and it's indeed an issue... > + platform_set_drvdata(pdev, priv); > + > + return devm_hwspin_lock_register(&pdev->dev, priv->bank, > &sun6i_hwspinlock_ops, > + SPINLOCK_BASE_ID, priv->nlocks); > +bank_fail: > + clk_disable_unprepare(priv->ahb_clk); > +clk_fail: > + reset_control_assert(priv->reset); > + > + return err; > +} > > So, is v6 fine for you even if it uses a more classic approach? ... but completely getting rid of the devm_ calls isn't a solution, if that's what you're calling the more classic approach. if (devm_hwspin_lock_register(..)) goto bank_fail return 0; works, and using devm_add_action like I suggested works as well to fix the issue you mentioned above Maxime
signature.asc
Description: PGP signature