[PATCH 0/2] Allow syscon to use spinlocks with regmap fast_io

[PATCH 0/2] Allow syscon to use spinlocks with regmap fast_io

[AngeloGioacchino Del Regno]

This series adds support for enabling the regmap's fast_io configuration
option for SoCs featuring very fast MMIO operations, for which mutexes
are introducing a lot of overhead / latency.

This has been tested locally on some MediaTek Chromebooks (but, of course,
that requires devicetree patches that are not included in this series).

AngeloGioacchino Del Regno (2):
  mfd: syscon: Allow using spinlocks with regmap fast_io
  dt-bindings: mfd: syscon: Add support for regmap fast-io

 Documentation/devicetree/bindings/mfd/syscon.yaml | 15 +++++++++++++++
 drivers/mfd/syscon.c                              |  4 ++++
 2 files changed, 19 insertions(+)

-- 
2.35.1

[PATCH 1/2] mfd: syscon: Allow using spinlocks with regmap fast_io

[AngeloGioacchino Del Regno]

On at least some SoCs accessing MMIO regions is very fast and in some
cases acquiring a mutex for every IO operation brings a significant
overhead: this is also the rationale of regmap's fast_io configuration
parameter, which makes it switch to using a spinlock instead.

Since the typical use-case for syscon is to give access to misc system
registers (not representing any specific type of device) to one or more
other drivers, and since this is done via regmap anyway, allow such
devices to let syscon configure regmap with fast_io enabled and to let
this happen, add a Devicetree property "fast-io": when this is found,
syscon will set '.fast_io = true' in the regmap config.

Of course, it makes little sense to do that if a syscon node declares
a phandle to a hardware spinlock provider node, so we check for this
property only if no hwlock is present.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/mfd/syscon.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 191fdb87c424..3fcd9afdb9ef 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -101,6 +101,10 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 		}
 	}
 
+	/* Checking for fast-io makes sense only if not using hwspinlock */
+	if (!syscon_config.use_hwlock && of_property_read_bool(np, "fast-io"))
+		syscon_config.fast_io = true;
+
 	syscon_config.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", np,
 				       (u64)res.start);
 	syscon_config.reg_stride = reg_io_width;
-- 
2.35.1

[PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[AngeloGioacchino Del Regno]

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

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 13baa452cc9d..85a2e83b5861 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -83,11 +83,26 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [1, 2, 4, 8]
 
+  fast-io:
+    description:
+      Indicates that this bus has a very fast IO, for which
+      acquiring a mutex would be significant overhead.
+      When present, regmap will use a spinlock instead.
+    type: boolean
+
   hwlocks:
     maxItems: 1
     description:
       Reference to a phandle of a hardware spinlock provider node.
 
+if:
+  required:
+    - hwlocks
+then:
+  not:
+    required:
+      - fast-io
+
 required:
   - compatible
   - reg
-- 
2.35.1

Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[Krzysztof Kozlowski]

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.

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  Documentation/devicetree/bindings/mfd/syscon.yaml | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> index 13baa452cc9d..85a2e83b5861 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> @@ -83,11 +83,26 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      enum: [1, 2, 4, 8]
>  
> +  fast-io:
> +    description:
> +      Indicates that this bus has a very fast IO, for which
> +      acquiring a mutex would be significant overhead.
> +      When present, regmap will use a spinlock instead.

Regmap is current implementation behind this, but it's not related to
hardware, so how about removing it from the description? Something like:
"..., for which different locking methods should be used to reduce
overhead (e.g. spinlock instead of mutex)."

> +    type: boolean
> +
>    hwlocks:
>      maxItems: 1
>      description:
>        Reference to a phandle of a hardware spinlock provider node.
>  
> +if:
> +  required:
> +    - hwlocks
> +then:
> +  not:
> +    required:
> +      - fast-io
> +
>  required:
>    - compatible
>    - reg


Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[AngeloGioacchino Del Regno]

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


>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   Documentation/devicetree/bindings/mfd/syscon.yaml | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
>> index 13baa452cc9d..85a2e83b5861 100644
>> --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
>> +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
>> @@ -83,11 +83,26 @@ properties:
>>       $ref: /schemas/types.yaml#/definitions/uint32
>>       enum: [1, 2, 4, 8]
>>   
>> +  fast-io:
>> +    description:
>> +      Indicates that this bus has a very fast IO, for which
>> +      acquiring a mutex would be significant overhead.
>> +      When present, regmap will use a spinlock instead.
> 
> Regmap is current implementation behind this, but it's not related to
> hardware, so how about removing it from the description? Something like:
> "..., for which different locking methods should be used to reduce
> overhead (e.g. spinlock instead of mutex)."
> 

That's a very good point. I didn't think about any future in which the
implementation would be changed from regmap to *new-name-here*... but
anyway it makes a lot more sense to "speak generic".

I'll change the description to match your proposal, thank you!

Regards,
Angelo

Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[Krzysztof Kozlowski]

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

Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[AngeloGioacchino Del Regno]

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@12345678 { blah... };
};

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

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

       something@12345678 { 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

Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[Nícolas F. R. A. Prado]

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.

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

Thanks,
Nícolas

Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[Krzysztof Kozlowski]

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

Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[AngeloGioacchino Del Regno]

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!
> 

Okay now that's clearing up my mind, makes me see the issue that was raised in
a different perspective. I've finally got it, and Krzysztof is definitely right
about that, as in the proposed solution raises potential future issues and
definitely cannot be considered for merging.

I should've sent this as a RFC instead; this topic is becoming larger than I
could foresee in the short term.

Thanks Krzysztof for raising this flag, and thank you Nic for finding a way to
wake up the hamster in my brain :-P

>>
>>>>
>>>> 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?

I didn't consider that kind of solution (adding yet one more function just for
the fast_io case, or wrapping the current ones) exactly because of the reasons
that you just explained...

...if we do that, we're going to also see issues like:
- Client #1 calls syscon fast-io, Client #2 is not converted
- Client #1 is expected to probe earlier than Client #2
- Probe order changes
- Now Client #1 also isn't fast-io

...unless reinitializing regmap mmio (regmap_init_mmio), but this would obviously
be a no-go, because races are under the *nearest* corner at that point. So.. no.

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

...but then that would still be a software construct and we'd be talking about
the same thing again, just in a different context.

I've also been thinking of adding a way to change the regmap locking on-the-fly
without having to re-init fully, but that also looks like being a no-go, as I
can foresee racing with that solution as well.

By the way, Let me give you some full-er context here, so that you can fully
understand, without any doubt, what I'm trying to do...

Here, on MediaTek SoCs, the HW ABI is in "full IP blocks" (warning: this "term"
may be wrong, but bear with me), for which we see each MMIO range having clocks,
resets, and other IP-specific registers, and sometimes these are "mixed" together.
There's no way to declare a small MMIO sub-range for just clocks, hence syscon is
really the only (relatively?) clean solution that can be used.

Thing is, MTK platforms are continuously locking/unlocking mutexes for simple and
fast operations, even "turning on/off" a clock (actually, due to the nature of the
clock tree for these SoCs, it's always more than one clock at once, so it's always
more than one mutex lock/unlock...), or operating on XHCI registers, etc etc etc.
This gives *a lot* of overhead and you can really experience the stutter sometimes,
and I mean experience with your own eyes, even on more performant chips...

I then checked some other non-MTK platforms, and I saw that it's not really just a
MTK thing (even though it's the heaviest user of syscon between the ones that I
checked)... so I saw a nice improvement pointer for lots of platforms... and I'm
sure that you immediately got that part.

I'm starting to get confused about what could be a good and acceptable solution
here, because at this point there are a lot of blockers around for any different
thing I can think of.

Regards,
Angelo

Re: [PATCH 2/2] dt-bindings: mfd: syscon: Add support for regmap fast-io

[AngeloGioacchino Del Regno]

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