diff mbox series

[1/2] spi: dt-bindings: introduce linux,use-rt-queue flag

Message ID 20230602115201.415718-1-matthias.schiffer@ew.tq-group.com
State New
Headers show
Series [1/2] spi: dt-bindings: introduce linux,use-rt-queue flag | expand

Commit Message

Matthias Schiffer June 2, 2023, 11:52 a.m. UTC
We have seen a number of downstream patches that allow enabling the
realtime feature of the SPI subsystem to reduce latency. These were
usually implemented for a specific SPI driver, even though the actual
handling of the rt flag is happening in the generic SPI controller code.

Introduce a generic linux,use-rt-queue flag that can be used with any
controller driver. The now redundant driver-specific pl022,rt flag is
marked as deprecated.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 Documentation/devicetree/bindings/spi/spi-controller.yaml | 6 ++++++
 Documentation/devicetree/bindings/spi/spi-pl022.yaml      | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Linus Walleij June 6, 2023, 2:37 p.m. UTC | #1
On Fri, Jun 2, 2023 at 2:22 PM Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jun 02, 2023 at 01:52:00PM +0200, Matthias Schiffer wrote:

> > We have seen a number of downstream patches that allow enabling the
> > realtime feature of the SPI subsystem to reduce latency. These were
> > usually implemented for a specific SPI driver, even though the actual
> > handling of the rt flag is happening in the generic SPI controller code.
> >
> > Introduce a generic linux,use-rt-queue flag that can be used with any
> > controller driver. The now redundant driver-specific pl022,rt flag is
> > marked as deprecated.
>
> This is clearly OS specific tuning so out of scope for DT...

In a sense, but to be fair anything prefixed linux,* is out of scope for DT,
Documentation/devicetree/bindings/input/matrix-keymap.yaml being
the most obvious offender.

On the other hand I think the DT maintainers said it is basically fine
to use undocumented DT properties for this kind of thing. Having
completely undocumented DT properties might seem evil in another
sense, but I think Apple does nothing but...

Yours,
Linus Walleij
Mark Brown June 6, 2023, 2:44 p.m. UTC | #2
On Tue, Jun 06, 2023 at 04:37:08PM +0200, Linus Walleij wrote:
> On Fri, Jun 2, 2023 at 2:22 PM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Jun 02, 2023 at 01:52:00PM +0200, Matthias Schiffer wrote:

> > > We have seen a number of downstream patches that allow enabling the
> > > realtime feature of the SPI subsystem to reduce latency. These were
> > > usually implemented for a specific SPI driver, even though the actual
> > > handling of the rt flag is happening in the generic SPI controller code.

> > > Introduce a generic linux,use-rt-queue flag that can be used with any
> > > controller driver. The now redundant driver-specific pl022,rt flag is
> > > marked as deprecated.

> > This is clearly OS specific tuning so out of scope for DT...

> In a sense, but to be fair anything prefixed linux,* is out of scope for DT,
> Documentation/devicetree/bindings/input/matrix-keymap.yaml being
> the most obvious offender.

That's at least a description of hardware though.  This is a performance
tuning thing, if it needs to be configured at all it should be
configured at runtime.  Some applications might see things work better,
some might see performance reduced and new versions might have different
performance characteristics and need different configuration.
Matthias Schiffer June 7, 2023, 12:55 p.m. UTC | #3
On Tue, 2023-06-06 at 15:44 +0100, Mark Brown wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Jun 06, 2023 at 04:37:08PM +0200, Linus Walleij wrote:
> > On Fri, Jun 2, 2023 at 2:22 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Fri, Jun 02, 2023 at 01:52:00PM +0200, Matthias Schiffer wrote:
> 
> > > > We have seen a number of downstream patches that allow enabling the
> > > > realtime feature of the SPI subsystem to reduce latency. These were
> > > > usually implemented for a specific SPI driver, even though the actual
> > > > handling of the rt flag is happening in the generic SPI controller code.
> 
> > > > Introduce a generic linux,use-rt-queue flag that can be used with any
> > > > controller driver. The now redundant driver-specific pl022,rt flag is
> > > > marked as deprecated.
> 
> > > This is clearly OS specific tuning so out of scope for DT...
> 
> > In a sense, but to be fair anything prefixed linux,* is out of scope for DT,
> > Documentation/devicetree/bindings/input/matrix-keymap.yaml being
> > the most obvious offender.
> 
> That's at least a description of hardware though.  This is a performance
> tuning thing, if it needs to be configured at all it should be
> configured at runtime.  Some applications might see things work better,
> some might see performance reduced and new versions might have different
> performance characteristics and need different configuration.


It is not clear to me what alternative options we currently have if we
want a setting to be effective from the very beginning, before
userspace is running. Of course adding a cmdline option would work, but
that seems worse than having it in the DT in every possible way.

I can understand not wanting such tuning in Device Trees in the kernel
repo - I agree that these default DTs should only describe the hardware
and it makes sense to keep OS-specific tuning out of them.

Requiring such tuning for specific drivers or driver instances is
however a common issue for embedded systems, which is why we are seeing
(and occasionally writing) such patches - setting things up from
userspace may happen too late, or may not be possible at all if a
setting needs to be available during probe. And even when deferring
things to userspace is possible, making things configurable at runtime
always adds some complexity, even though it is often not a requirement
at all for embedded systems.

Just doing this through the DT is very convenient and robust. The
settings could be inserted into the default DT as an overlay applied
during build or by the bootloader.

Any alternative solution we could come up with would likely add more
complexity on the driver side, and be less convenient to use for
developers. Overall, the rationale for not supporting such bindings in
drivers seems much weaker to me than that for not having such settings
in default DTs...

Best regards,
Matthias


(ps. Sorry about our bouncing linux@ address. Should be fixed now.)
Mark Brown June 7, 2023, 2:21 p.m. UTC | #4
On Wed, Jun 07, 2023 at 02:55:31PM +0200, Matthias Schiffer wrote:

> It is not clear to me what alternative options we currently have if we
> want a setting to be effective from the very beginning, before
> userspace is running. Of course adding a cmdline option would work, but
> that seems worse than having it in the DT in every possible way.

Is it *really* that important that this be configured before userspace
is running?  With an initramfs you'd be able to do configuration before
even trying to mount filesystems if your primary storage is flash.  I'd
not expect the pre-userspace period to be under particular pressure
here.

Frankly I don't see the command line as being particularly worse here,
it's more tasteful and if you're doing some device specific
configuration it doesn't seem to make much difference.  Userspace looks
even better though.

> Requiring such tuning for specific drivers or driver instances is
> however a common issue for embedded systems, which is why we are seeing
> (and occasionally writing) such patches - setting things up from
> userspace may happen too late, or may not be possible at all if a
> setting needs to be available during probe. And even when deferring
> things to userspace is possible, making things configurable at runtime
> always adds some complexity, even though it is often not a requirement
> at all for embedded systems.

Using DT is all about adding complexity.
Krzysztof Kozlowski June 7, 2023, 2:28 p.m. UTC | #5
On 07/06/2023 14:55, Matthias Schiffer wrote:
> On Tue, 2023-06-06 at 15:44 +0100, Mark Brown wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Jun 06, 2023 at 04:37:08PM +0200, Linus Walleij wrote:
>>> On Fri, Jun 2, 2023 at 2:22 PM Mark Brown <broonie@kernel.org> wrote:
>>>> On Fri, Jun 02, 2023 at 01:52:00PM +0200, Matthias Schiffer wrote:
>>
>>>>> We have seen a number of downstream patches that allow enabling the
>>>>> realtime feature of the SPI subsystem to reduce latency. These were
>>>>> usually implemented for a specific SPI driver, even though the actual
>>>>> handling of the rt flag is happening in the generic SPI controller code.
>>
>>>>> Introduce a generic linux,use-rt-queue flag that can be used with any
>>>>> controller driver. The now redundant driver-specific pl022,rt flag is
>>>>> marked as deprecated.
>>
>>>> This is clearly OS specific tuning so out of scope for DT...
>>
>>> In a sense, but to be fair anything prefixed linux,* is out of scope for DT,
>>> Documentation/devicetree/bindings/input/matrix-keymap.yaml being
>>> the most obvious offender.
>>
>> That's at least a description of hardware though.  This is a performance
>> tuning thing, if it needs to be configured at all it should be
>> configured at runtime.  Some applications might see things work better,
>> some might see performance reduced and new versions might have different
>> performance characteristics and need different configuration.
> 
> 
> It is not clear to me what alternative options we currently have if we
> want a setting to be effective from the very beginning, before
> userspace is running. Of course adding a cmdline option would work, but
> that seems worse than having it in the DT in every possible way.
> 
> I can understand not wanting such tuning in Device Trees in the kernel
> repo - I agree that these default DTs should only describe the hardware
> and it makes sense to keep OS-specific tuning out of them.

It is not about the sense. It's the rule and the policy. If you want to
change the existing practice, don't do it via one patch that sneaks
something in, but change entire practice for entire DT.

> 
> Requiring such tuning for specific drivers or driver instances is
> however a common issue for embedded systems, which is why we are seeing
> (and occasionally writing) such patches - setting things up from
> userspace may happen too late, or may not be possible at all if a
> setting needs to be available during probe. And even when deferring
> things to userspace is possible, making things configurable at runtime
> always adds some complexity, even though it is often not a requirement
> at all for embedded systems.
> 
> Just doing this through the DT is very convenient and robust. The
> settings could be inserted into the default DT as an overlay applied
> during build or by the bootloader.
> 
> Any alternative solution we could come up with would likely add more
> complexity on the driver side, and be less convenient to use for
> developers. Overall, the rationale for not supporting such bindings in
> drivers seems much weaker to me than that for not having such settings
> in default DTs...

It's not a hardware property. Do not put software choices or policies
specific to only one OS into the DT. The DTS is used by different users,
including other operating systems, firmwares and bootloaders.

Your convenience is not justification for misusing the DT. That's the
same argument community is using since ages for every wish from someone
there wanting something "convenient for him". Same answer for all the
weird ABIs, weird new user-space interfaces, weird duplicated
subsystems, many different schedulers (yeah, because why improve
existing solution in the kernel...) etc. It's always easier for
contributor to solve only their one problem. No, we are less interested
in solving only your one specific problem if such solution breaks
existing rules and consensus.

I think Mark was here quite explicit, but since discussion is still going:

NAK for the linux,rt property.

Best regards,
Krzysztof
Linus Walleij June 9, 2023, 7:41 a.m. UTC | #6
On Wed, Jun 7, 2023 at 2:55 PM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:

> It is not clear to me what alternative options we currently have if we
> want a setting to be effective from the very beginning, before
> userspace is running. Of course adding a cmdline option would work, but
> that seems worse than having it in the DT in every possible way.

A agree with Mark that a command line option isn't that bad. It's something
that pertains to just the Linux kernel after all? And you can put that command
line option in the default device tree, in chosen, if you want. No-one
is going to
complain about that.

Yours,
Linus Walleij
Alexander Stein June 9, 2023, 8:15 a.m. UTC | #7
Hi all,

Am Freitag, 9. Juni 2023, 09:41:14 CEST schrieb Linus Walleij:
> On Wed, Jun 7, 2023 at 2:55 PM Matthias Schiffer
> 
> <matthias.schiffer@ew.tq-group.com> wrote:
> > It is not clear to me what alternative options we currently have if we
> > want a setting to be effective from the very beginning, before
> > userspace is running. Of course adding a cmdline option would work, but
> > that seems worse than having it in the DT in every possible way.
> 
> A agree with Mark that a command line option isn't that bad. It's something
> that pertains to just the Linux kernel after all? And you can put that
> command line option in the default device tree, in chosen, if you want.

I don't like the idea of a command line enabling realtime scheduling for all 
instances of the SPI controller driver or even all SPI controllers. Actually 
this might be worse if a non-rt SPI bus is considered for RT scheduling.
IMHO this should be configurable per SPI controller, e.g. a sysfs attribute.

> No-one is going to
> complain about that.

IIRC someone (maybe Greg K-H) opposed pretty hard against (module) parameters 
for (driver) configuration, but I can't find the post to back my statement.

Best regards,
Alexander

> Yours,
> Linus Walleij
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Linus Walleij June 9, 2023, 8:42 a.m. UTC | #8
On Fri, Jun 9, 2023 at 10:15 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> > A agree with Mark that a command line option isn't that bad. It's something
> > that pertains to just the Linux kernel after all? And you can put that
> > command line option in the default device tree, in chosen, if you want.
>
> I don't like the idea of a command line enabling realtime scheduling for all
> instances of the SPI controller driver or even all SPI controllers. Actually
> this might be worse if a non-rt SPI bus is considered for RT scheduling.
> IMHO this should be configurable per SPI controller,

OK that's a fair point.

I don't think command line arguments are necessarily global by
nature, AFAIK it's fine to invent something like pl022.4.rt_sched=1
where 4 is the instance number. Parsing it is just code.

> e.g. a sysfs attribute.

But it needs to be set before userspace is up :/

I fully sympathize with this problem, because I have faced
similar problems myself.

My fallback solution for this driver would be to keep using the
old DT property (which was merged when reviewing was
not as strict) if that works, or use undocumented DT properties,
it's not the end of the world but does leave the bad taste of
a work not finished.

Yours,
Linus Walleij
Alexander Stein June 9, 2023, 9:13 a.m. UTC | #9
Am Freitag, 9. Juni 2023, 10:42:04 CEST schrieb Linus Walleij:
> On Fri, Jun 9, 2023 at 10:15 AM Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > > A agree with Mark that a command line option isn't that bad. It's
> > > something
> > > that pertains to just the Linux kernel after all? And you can put that
> > > command line option in the default device tree, in chosen, if you want.
> > 
> > I don't like the idea of a command line enabling realtime scheduling for
> > all instances of the SPI controller driver or even all SPI controllers.
> > Actually this might be worse if a non-rt SPI bus is considered for RT
> > scheduling. IMHO this should be configurable per SPI controller,
> 
> OK that's a fair point.
> 
> I don't think command line arguments are necessarily global by
> nature, AFAIK it's fine to invent something like pl022.4.rt_sched=1
> where 4 is the instance number. Parsing it is just code.

Now we are touching the topic of non-deterministic device names/numbers. This 
gets worse if your SPI controller is attached to some other device, although 
RT capabilities are rather limited in that case anyway.

> > e.g. a sysfs attribute.
> 
> But it needs to be set before userspace is up :/

Does it? IMHO a realtime system is allowed to use blocking mechanism (e.g. 
dynamic memory allocatin and so on) during startup/configuration phase, 
ignoring any deadlines.
Once it starts operating this is a no-go.
This seems rather similar to configure scheduling priority for IRQ threads on 
RT preempt systems. IIRC according to RT folks, this is considered an 
administration task, thus the responsibility of userspace.

> I fully sympathize with this problem, because I have faced
> similar problems myself.

You mean RT-scheduling before userspace is up? Can you elaborate the issues 
you see?

> My fallback solution for this driver would be to keep using the
> old DT property (which was merged when reviewing was
> not as strict) if that works, or use undocumented DT properties,
> it's not the end of the world but does leave the bad taste of
> a work not finished.

I was surprised to see the driver specific property for configuring RT sched 
as well. I assume the intention of this series is to support this feature for 
other SPI controller drivers as well. So some kind of feature has to be added 
anyway.

Best regards,
Alexander

> Yours,
> Linus Walleij
Linus Walleij June 9, 2023, 9:22 a.m. UTC | #10
On Fri, Jun 9, 2023 at 11:13 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> > I fully sympathize with this problem, because I have faced
> > similar problems myself.
>
> You mean RT-scheduling before userspace is up? Can you elaborate the issues
> you see?

No. But choosing block layer scheduler (BFQ for MMC cards) before userspace
is up, which is currently done by udev scripts in eg Fedora :(

Yours,
Linus Walleij
Mark Brown June 9, 2023, 9:33 a.m. UTC | #11
On Fri, Jun 09, 2023 at 10:42:04AM +0200, Linus Walleij wrote:
> On Fri, Jun 9, 2023 at 10:15 AM Alexander Stein

> > e.g. a sysfs attribute.

> But it needs to be set before userspace is up :/

I'm really not clear that this is actually the case - I've not heard an
articulated use case here.
Mark Brown June 9, 2023, 9:34 a.m. UTC | #12
On Fri, Jun 09, 2023 at 11:13:39AM +0200, Alexander Stein wrote:
> Am Freitag, 9. Juni 2023, 10:42:04 CEST schrieb Linus Walleij:

> > I don't think command line arguments are necessarily global by
> > nature, AFAIK it's fine to invent something like pl022.4.rt_sched=1
> > where 4 is the instance number. Parsing it is just code.

> Now we are touching the topic of non-deterministic device names/numbers. This 
> gets worse if your SPI controller is attached to some other device, although 
> RT capabilities are rather limited in that case anyway.

I would use the address rather than the device number, and you could use
the compatible if you're worried about a stable name for the name part.
Rob Herring (Arm) June 14, 2023, 7:30 p.m. UTC | #13
On Tue, Jun 06, 2023 at 04:37:08PM +0200, Linus Walleij wrote:
> On Fri, Jun 2, 2023 at 2:22 PM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Jun 02, 2023 at 01:52:00PM +0200, Matthias Schiffer wrote:
> 
> > > We have seen a number of downstream patches that allow enabling the
> > > realtime feature of the SPI subsystem to reduce latency. These were
> > > usually implemented for a specific SPI driver, even though the actual
> > > handling of the rt flag is happening in the generic SPI controller code.
> > >
> > > Introduce a generic linux,use-rt-queue flag that can be used with any
> > > controller driver. The now redundant driver-specific pl022,rt flag is
> > > marked as deprecated.
> >
> > This is clearly OS specific tuning so out of scope for DT...
> 
> In a sense, but to be fair anything prefixed linux,* is out of scope for DT,
> Documentation/devicetree/bindings/input/matrix-keymap.yaml being
> the most obvious offender.
> 
> On the other hand I think the DT maintainers said it is basically fine
> to use undocumented DT properties for this kind of thing. Having
> completely undocumented DT properties might seem evil in another
> sense, but I think Apple does nothing but...

I don't don't know where you got that impression. I'm fine with them in 
the sense that I don't look at downstream and anything goes there.

Rob
Linus Walleij June 14, 2023, 7:59 p.m. UTC | #14
On Wed, Jun 14, 2023 at 9:30 PM Rob Herring <robh@kernel.org> wrote:
> On Tue, Jun 06, 2023 at 04:37:08PM +0200, Linus Walleij wrote:

> > On the other hand I think the DT maintainers said it is basically fine
> > to use undocumented DT properties for this kind of thing. Having
> > completely undocumented DT properties might seem evil in another
> > sense, but I think Apple does nothing but...
>
> I don't don't know where you got that impression. I'm fine with them in
> the sense that I don't look at downstream and anything goes there.

No I was mistaken.

This was me misremembering that the "sloppy logic analyzer" from
Wolfram Sang was OK to merge without any proper bindings, but the
reason there was that this is for debugging only, but I don't know if
someone told him that or it's his own claim.

This is not for debugging only so it doesn't apply anyway.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 524f6fe8c27b4..a24b4ea87443b 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -79,6 +79,12 @@  properties:
     description:
       The SPI controller acts as a slave, instead of a master.
 
+  linux,use-rt-queue:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Indicates that the controller should run the message pump with realtime
+      priority to minimise the transfer latency on the bus.
+
   slave:
     type: object
 
diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
index 91e540a92fafe..3c43fcc007e1f 100644
--- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
@@ -49,8 +49,10 @@  properties:
 
   pl022,rt:
     description: indicates the controller should run the message pump with realtime
-      priority to minimise the transfer latency on the bus (boolean)
+      priority to minimise the transfer latency on the bus (deprecated, use the
+      generic linux,use-rt-queue property)
     type: boolean
+    deprecated: true
 
   dmas:
     description: