On Wed, Nov 18, 2020 at 08:36:24PM +0100, Wilken Gottwalt wrote: > On Wed, 18 Nov 2020 16:37:33 +0100 > Maxime Ripard wrote: > > Hi Wilken, > > > > On Wed, Nov 18, 2020 at 11:02:40AM +0100, Wilken Gottwalt wrote: > > > Adds the sunxi_hwspinlock driver and updates makefiles/maintainers. > > > > > > Signed-off-by: Wilken Gottwalt > > > > A more descriptive commit log would be welcome here, for example > > containing on which SoC this driver can be used, and on which it was > > tested. > > can you help me here a bit? I still try to figure out how to do patch sets > properly. Some kernel submitting documentation says everything goes into the > coverletter and other documentation only tells how to split the patches. So > what would be the right way? A quick example based on my patch set would be > really helpful. I mean, the split between your patches and so on is good, you got that right The thing I wanted better details on is the commit log itself, so the message attached to that patch. > > This is the third attempt at that driver, and you can find the previous > > versions here: > > https://patchwork.kernel.org/project/linux-arm-kernel/cover/20200210170143.20007-1-nborisov@suse.com/ > > https://lore.kernel.org/patchwork/patch/706512/ > > > > Most of the comments on those series still apply to yours. > > Oh, I wrote my driver 2-3 years ago and just prepared it for mainline. I > wasn't aware of other attempts. I really should have checked this. Though, > I really want to get to the point where this driver is good enough for > mainline. Hmmm, it is interesting how similar these drivers are. Looks like > the other developers also got inspired by the already existing hwspinlock > drivers. :D Yeah, it looks like you all got the same inspiration :) > > Most importantly, this hwspinlock is used to synchronize the ARM cores > > and the ARISC. How did you test this driver? > > Yes, you are right, I should have mentioned this. I have a simple test kernel > module for this. But I must admit, testing the ARISC is very hard and I have > no real idea how to do it. Testing the hwspinlocks in general seems to work > with my test kernel module, but I'm not sure if this is really sufficient. I > can provide the code for it if you like. What would be the best way? Github? > Just mailing a patch? > > The test module produces these results: > > # insmod /lib/modules/5.9.8/kernel/drivers/hwspinlock/sunxi_hwspinlock_test.ko > [ 45.395672] [init] sunxi hwspinlock test driver start > [ 45.400775] [init] start test locks > [ 45.404263] [run ] testing 32 locks > [ 45.407804] [test] testing lock 0 ----- > [ 45.411652] [test] taking lock attempt #0 succeded > [ 45.416438] [test] try taken lock attempt #0 > [ 45.420735] [test] unlock/take attempt #0 > [ 45.424752] [test] taking lock attempt #1 succeded > [ 45.429556] [test] try taken lock attempt #1 > [ 45.433823] [test] unlock/take attempt #1 > [ 45.437862] [test] testing lock 1 ----- > [ 45.441699] [test] taking lock attempt #0 succeded > [ 45.446484] [test] try taken lock attempt #0 > [ 45.450768] [test] unlock/take attempt #0 > [ 45.454774] [test] taking lock attempt #1 succeded > [ 45.459576] [test] try taken lock attempt #1 > [ 45.463843] [test] unlock/take attempt #1 > . > . > . > [ 46.309925] [test] testing lock 30 ----- > [ 46.313852] [test] taking lock attempt #0 succeded > [ 46.318654] [test] try taken lock attempt #0 > [ 46.322920] [test] unlock/take attempt #0 > [ 46.326944] [test] taking lock attempt #1 succeded > [ 46.331729] [test] try taken lock attempt #1 > [ 46.335994] [test] unlock/take attempt #1 > [ 46.340021] [test] testing lock 31 ----- > [ 46.343947] [test] taking lock attempt #0 succeded > [ 46.348749] [test] try taken lock attempt #0 > [ 46.353016] [test] unlock/take attempt #0 > [ 46.357040] [test] taking lock attempt #1 succeded > [ 46.361825] [test] try taken lock attempt #1 > [ 46.366090] [test] unlock/take attempt #1 > [ 46.370112] [init] end test locks That doesn't really test for contention though, and dealing with contention is mostly what this hardware is about. Could you make a small test with crust to see if when the arisc has taken the lock, the ARM cores can't take it? Maxime