Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Nikolay Borisov <nborisov@suse.com>]

On 12.02.20 г. 14:06 ч., Maxime Ripard wrote:
> On Tue, Feb 11, 2020 at 03:17:40PM +0200, Nikolay Borisov wrote:
>>
>>
>> On 11.02.20 г. 14:34 ч., Maxime Ripard wrote:
>>> On Tue, Feb 11, 2020 at 10:08:08AM +0200, Nikolay Borisov wrote:
>>>> On 11.02.20 г. 9:46 ч., Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Feb 10, 2020 at 07:01:41PM +0200, Nikolay Borisov wrote:
>>>>>> Based on the datasheet this implements support for the hwspinlock IP
>>>>>> block.
>>>>>
>>>>> How was this tested?
>>>>
>>>> I tested it on my pine64 lts e.g. loading the driver and reading the
>>>> reset/clock/sysstatus registers to ensure everything is unmasked and has
>>>> expected values.
>>>
>>> Isn't the point of hwspinlocks that it's shared between the ARISC core
>>> and the ARM cores. How did you test that the lock was actually taken
>>> on the other side just by using the ARM cores?
>>
>> I haven't. I'm really focuse don just enabling this on the linux side of
>> things. True, hw spinlocks are used to synchronize cpu running different
>> OS'es.
> 
> I'm sorry but this driver hasn't been really tested then. The whole
> point of it is to synchronise with something. If you tested without

I disagree, the whole point is to expose the facility for other drivers
which, in turn, might need to synchronize with that other thing. I see
the hwspinlock driver as a dumb provider of the interface. The only
pertinent contention point I see is how should the clock/soft reset
registers be programmed considering the spinlocks might be accessed
outside of linux.

> that something, it's just like testing a network driver without having
> anything connected on the network you're testing it on: it probably
> looks like it's working, but you really can't tell.
> 
>> It's still implementation defined which hwspinlock is used for
>> which component. Additionally if we assume the ARISC core uses spinlock
>> this means by the time linux is booted the spinlocks should already be
>> clocked and out of software reset so perhahps this is also redundant in
>> the driver?
> 
> Linux also likes to disable the clocks no one is using, so in such a
> situation, what would happen? Can the ARISC still use them, should we
> maintain the enabled all the time?
> 
> This is exactly the kind of corner-cases that we need a test for.


Fair point BUT, and this is one big BUT. This other thing (the ARISC fw)
doesn't have a contract with the kernel on synchronization. So should
the test be performed against allwinner's binary blob or against some of
the open source alternatives ( I only saw it mentioned on the sunxi web
page but no links to such open source alternatives).


Furthermore, if we assume we should be compatible with the binary blob
it needs to be RE'd in order to understand which hw spinlocks it's using
for synchronization and I'm not aware of any such effort.

> 
> Maxime
> 



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