Date:   Tue, 5 Apr 2022 10:52:48 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Nícolas F. R. A. Prado 
        <nfraprado@...labora.com>
Cc:     lee.jones@...aro.org, robh+dt@...nel.org, krzk+dt@...nel.org,
        arnd@...db.de, matthias.bgg@...il.com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap
 fast-io

Il 05/04/22 08:48, Krzysztof Kozlowski ha scritto:
> On 05/04/2022 00:26, Nícolas F. R. A. Prado wrote:
>> On Mon, Apr 04, 2022 at 11:39:49AM +0200, AngeloGioacchino Del Regno wrote:
>>> Il 04/04/22 10:55, Krzysztof Kozlowski ha scritto:
>>>> On 04/04/2022 10:40, AngeloGioacchino Del Regno wrote:
>>>>> Il 02/04/22 13:38, Krzysztof Kozlowski ha scritto:
>>>>>> On 01/04/2022 15:50, AngeloGioacchino Del Regno wrote:
>>>>>>> The syscon driver now enables the .fast_io regmap configuration when
>>>>>>> the 'fast-io' property is found in a syscon node.
>>>>>>>
>>>>>>> Keeping in mind that, in regmap, fast_io is checked only if we are
>>>>>>> not using hardware spinlocks, allow the fast-io property only if
>>>>>>> there is no hwlocks reference (and vice-versa).
>>>>>>
>>>>>> I have doubts you need a property for this. "fast" is subjective in
>>>>>> terms of hardware, so this looks more like a software property, not
>>>>>> hardware.
>>>>>>
>>>>>> I think most of MMIOs inside a SoC are considered fast. Usually also the
>>>>>> syscon/regmap consumer knows which regmap it gets, so knows that it is
>>>>>> fast or not.
>>>>>>
>>>>>
>>>>> Hello Krzysztof,
>>>>>
>>>>> well yes, this property is changing how software behaves - specifically,
>>>>> as you've correctly understood, what regmap does.
>>>>>
>>>>> It's true that most of MMIOs inside a SoC are considered fast.. the word "most" is
>>>>> the exact reason why I haven't proposed simply hardcoding '.fast_io = true' in
>>>>> syscon, or in regmap-mmio...
>>>>> There are too many different SoCs around, and I didn't want to end up breaking
>>>>> anything (even if it should be unlikely, since MMIO is fast by principle).
>>
>> Hi Angelo,
>>
>> I think I can see what Krzysztof means by saying this looks more like a software
>> property.
>>
>> This property isn't simply saying whether the hardware is fast or not by itself,
>> since that's relative. Rather, it means that this hardware is fast relative to
>> the time overhead of using a mutex for locking in regmap. Since this is a
>> software construct, the property as a whole is software-dependent. If for some
>> reason the locking in regmap were to be changed and was now a lot faster or
>> slower, the same hardware could now be considered "fast" or "slow". This seems
>> to me a good reason to avoid making "fastness" part of the ABI for each
>> hardware.
> 
> Thanks, that very good explanation!
> 
>>
>>>>
>>>> What I am proposing, is the regmap consumer knows whether access is fast
>>>> or not, so it could call get_regmap() or
>>>> syscon_regmap_lookup_by_phandle() with appropriate argument.
>>>>
>>>> Even if we stay with a DT property, I am not sure if this is an
>>>> attribute of syscon but rather of a bus.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> I'm sorry for sending a v2 so fast - apparently, I initially didn't fully
>>> understand your comment, but now it's clear.
>>>
>>> Actually, since locking in regmap's configuration does not use DT at all
>>> in any generic case, maybe bringing this change purely in code may be a
>>> good one... and I have evaluated that before proposing this kind of change.
>>>
>>> My concerns about that kind of approach are:
>>> - First of all, there are * a lot * of drivers, in various subsystems, that
>>>    are using syscon, so changing some function parameter in syscon.c would
>>>    result in a commit that would be touching hundreds of them... and some of
>>>    them would be incorrect, as the default would be no fast-io, while they
>>>    should indeed enable that. Of course this would have to be changed later
>>>    by the respective driver maintainer(s), potentially creating a lot of
>>>    commit noise with lots of Fixes tags, which I am trying to avoid;
>>> - Not all drivers are using the same syscon exported function to get a
>>>    handle to regmap and we're looking at 6 of them; changing only one of
>>>    the six would be rather confusing, and most probably logically incorrect
>>>    as well...
>>>
>>> Of course you know, but for the sake of making this easily understandable
>>> for any casual developers reading this, functions are:
>>> - device_node_to_regmap()
>>> - syscon_node_to_regmap()
>>> - syscon_regmap_lookup_by_compatible()
>>> - syscon_regmap_lookup_by_phandle()
>>> - syscon_regmap_lookup_by_phandle_args()
>>> - syscon_regmap_lookup_by_phandle_optional().
>>
>> What if a separate function was added with the additional regmap configuration
>> argument? That way setting the "fast_io" would be opt-in much like a DT property
>> would. The other drivers wouldn't need to be changed.
> 
> Exactly, there is no need to change all callers now.
> 1. You just add rename existing code and add argument:
> 
>    syscon_regmap_lookup_by_compatible_mmio(...., unsigned long flags);
> 
> 2. Add a wrappers (with old names):
> syscon_regmap_lookup_by_compatible() {
>    syscon_regmap_lookup_by_compatible_mmio(..., SYSCON_IO_DEFAULT);
> }
> 
> 3. and finally slowly convert the users where it is relevant.
> 
> Just one more thing. I was rather thinking out loud, instead of
> proposing a proper solution about clients defining speed. I am still not
> sure if this is correct approach, because actually the regmap provider
> knows the best whether it is slow or fast. Clients within SoC should
> know it, but what if one client asks for fast regmap, other for slow? Or
> not every client is updated to new API?
> 
> Another solution would be to add such property to the bus on which the
> syscon device is sitting. Although this is also not complete. Imagine:
> 
> syscon <--ahb-fast-bus--> some bus bridge <--apb-slow-bus--> syscon consumer
> 
> Although your original case also would not be accurate here.
> 
> Best regards,
> Krzysztof


Sorry for the double email.... but I've just realized that this was a bug on
my side!!!!! I am so sorry for raising all this dust.

regmap-mmio has .fast_io = true in its regmap_bus structure, hence, every
syscon user is already using spinlocks. I was doing some tests around and
during that operation I've created a condition in which that got ignored.

We can let this series die.

Apologies.

Regards,
Angelo

Powered by blists - more mailing lists