Date:   Mon, 4 Apr 2022 11:39:49 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        lee.jones@...aro.org
Cc:     robh+dt@...nel.org, krzk+dt@...nel.org, arnd@...db.de,
        matthias.bgg@...il.com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, nfraprado@...labora.com,
        kernel@...labora.com
Subject: Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap
 fast-io

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).
> 
> 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().


Regarding making this a bus-wide property... premise: it is possible that
I haven't fully understood what you're trying to say - in which case, I'm
sorry again. Replying on the base of what my brain understands.

 From what I get, if we make this a bus-wide property, this would mean that
we would have something like:

soc {
   ...something...
   fast-io;

   something@...45678 { blah... };
};

.... or we may have instead something like:

soc {
   compatible = "simple-bus";
   ...something...
   some-fast-mmio {
     compatible = "simple-bus";
     fast-io;

       something@...45678 { blah };
   };
};

I would expect the first one to be used in the vast majority of the cases,
while the second one would be used in corner cases in which a SoC has only
a portion of MMIO that can be considered "fast" and the rest cannot, which
I think would be pretty much uncommon (but obviously has to be taken into
account for flexibility).

So. Recapping.

I would tend to say that the first strategy (changing syscon.c function
signatures) is not the best way to proceed... while the second one may
actually be smart.

I'm not sure, though, how would I express the condition for which you can
use hwlocks only when fast-io is not set (and vice-versa): the issue about
this is that, at that point, the binding to be changed wouldn't be just syscon,
because it's not the only driver that may use hwlocks, there's others as
well, which may or may not use both (both = hwlocks *and* fast-io), depending
on the implementation, so would we end up changing a lot of dt-bindings in
that case?

And if not, I can see a long story of misunderstandments about what each
driver does since on some, fast-io excludes hwlocks, but *not* on others.
Of course, I had evaluated most of (but not all of) that before deciding
to send this series... and I was definitely expecting to get some constructive
resistance on this (so thank you for that!).

Besides, I realized that this reply is a big wall of text, hope it won't be
too boring to read.

Regards,
Angelo

Powered by blists - more mailing lists