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