linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
@ 2023-06-02 11:52 Matthias Schiffer
  2023-06-02 11:52 ` [PATCH 2/2] spi: add support for generic " Matthias Schiffer
  2023-06-02 12:22 ` [PATCH 1/2] spi: dt-bindings: introduce " Mark Brown
  0 siblings, 2 replies; 17+ messages in thread
From: Matthias Schiffer @ 2023-06-02 11:52 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Linus Walleij
  Cc: linux-spi, devicetree, linux-arm-kernel, linux-kernel, linux,
	Matthias Schiffer

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

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:
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/2] spi: add support for generic linux,use-rt-queue flag
  2023-06-02 11:52 [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag Matthias Schiffer
@ 2023-06-02 11:52 ` Matthias Schiffer
  2023-06-02 12:22 ` [PATCH 1/2] spi: dt-bindings: introduce " Mark Brown
  1 sibling, 0 replies; 17+ messages in thread
From: Matthias Schiffer @ 2023-06-02 11:52 UTC (permalink / raw)
  To: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Linus Walleij
  Cc: linux-spi, devicetree, linux-arm-kernel, linux-kernel, linux,
	Matthias Schiffer

Instead of requiring per-driver support to handle the message queue with
realtime priority, add handling for a linux,use-rt-queue DT flag to the
generic SPI controller initialization.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/spi/spi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9291b2a0e8871..f069f1aef5378 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3185,6 +3185,11 @@ int spi_register_controller(struct spi_controller *ctlr)
 		ctlr->mode_bits |= SPI_CS_HIGH;
 	}
 
+	/* Run message pump with realtime priority */
+	if (ctlr->dev.of_node &&
+	    of_property_read_bool(ctlr->dev.of_node, "linux,use-rt-queue"))
+		ctlr->rt = true;
+
 	/*
 	 * Even if it's just one always-selected device, there must
 	 * be at least one chipselect.
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-02 11:52 [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag Matthias Schiffer
  2023-06-02 11:52 ` [PATCH 2/2] spi: add support for generic " Matthias Schiffer
@ 2023-06-02 12:22 ` Mark Brown
  2023-06-06 14:37   ` Linus Walleij
  1 sibling, 1 reply; 17+ messages in thread
From: Mark Brown @ 2023-06-02 12:22 UTC (permalink / raw)
  To: Matthias Schiffer
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
	linux-spi, devicetree, linux-arm-kernel, linux-kernel, linux

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-02 12:22 ` [PATCH 1/2] spi: dt-bindings: introduce " Mark Brown
@ 2023-06-06 14:37   ` Linus Walleij
  2023-06-06 14:44     ` Mark Brown
  2023-06-14 19:30     ` Rob Herring
  0 siblings, 2 replies; 17+ messages in thread
From: Linus Walleij @ 2023-06-06 14:37 UTC (permalink / raw)
  To: Mark Brown
  Cc: Matthias Schiffer, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, linux-spi, devicetree, linux-arm-kernel,
	linux-kernel, linux

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-06 14:37   ` Linus Walleij
@ 2023-06-06 14:44     ` Mark Brown
  2023-06-07 12:55       ` Matthias Schiffer
  2023-06-14 19:30     ` Rob Herring
  1 sibling, 1 reply; 17+ messages in thread
From: Mark Brown @ 2023-06-06 14:44 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Matthias Schiffer, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, linux-spi, devicetree, linux-arm-kernel,
	linux-kernel, linux

[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-06 14:44     ` Mark Brown
@ 2023-06-07 12:55       ` Matthias Schiffer
  2023-06-07 14:21         ` Mark Brown
                           ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Matthias Schiffer @ 2023-06-07 12:55 UTC (permalink / raw)
  To: Mark Brown, Linus Walleij
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-spi,
	devicetree, linux-arm-kernel, linux-kernel, linux

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

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-07 12:55       ` Matthias Schiffer
@ 2023-06-07 14:21         ` Mark Brown
  2023-06-07 14:28         ` Krzysztof Kozlowski
  2023-06-09  7:41         ` Linus Walleij
  2 siblings, 0 replies; 17+ messages in thread
From: Mark Brown @ 2023-06-07 14:21 UTC (permalink / raw)
  To: Matthias Schiffer
  Cc: Linus Walleij, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-spi, devicetree, linux-arm-kernel, linux-kernel, linux

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-07 12:55       ` Matthias Schiffer
  2023-06-07 14:21         ` Mark Brown
@ 2023-06-07 14:28         ` Krzysztof Kozlowski
  2023-06-09  7:41         ` Linus Walleij
  2 siblings, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-07 14:28 UTC (permalink / raw)
  To: Matthias Schiffer, Mark Brown, Linus Walleij
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-spi,
	devicetree, linux-arm-kernel, linux-kernel, linux

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-07 12:55       ` Matthias Schiffer
  2023-06-07 14:21         ` Mark Brown
  2023-06-07 14:28         ` Krzysztof Kozlowski
@ 2023-06-09  7:41         ` Linus Walleij
  2023-06-09  8:15           ` Alexander Stein
  2 siblings, 1 reply; 17+ messages in thread
From: Linus Walleij @ 2023-06-09  7:41 UTC (permalink / raw)
  To: Matthias Schiffer
  Cc: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-spi, devicetree, linux-arm-kernel, linux-kernel, linux

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-09  7:41         ` Linus Walleij
@ 2023-06-09  8:15           ` Alexander Stein
  2023-06-09  8:42             ` Linus Walleij
  0 siblings, 1 reply; 17+ messages in thread
From: Alexander Stein @ 2023-06-09  8:15 UTC (permalink / raw)
  To: Matthias Schiffer, linux-arm-kernel
  Cc: Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	linux-spi, devicetree, linux-arm-kernel, linux-kernel, linux,
	Linus Walleij

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


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-09  8:15           ` Alexander Stein
@ 2023-06-09  8:42             ` Linus Walleij
  2023-06-09  9:13               ` Alexander Stein
  2023-06-09  9:33               ` Mark Brown
  0 siblings, 2 replies; 17+ messages in thread
From: Linus Walleij @ 2023-06-09  8:42 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Matthias Schiffer, linux-arm-kernel, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, linux-spi, devicetree,
	linux-kernel, linux

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-09  8:42             ` Linus Walleij
@ 2023-06-09  9:13               ` Alexander Stein
  2023-06-09  9:22                 ` Linus Walleij
  2023-06-09  9:34                 ` Mark Brown
  2023-06-09  9:33               ` Mark Brown
  1 sibling, 2 replies; 17+ messages in thread
From: Alexander Stein @ 2023-06-09  9:13 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Matthias Schiffer, linux-arm-kernel, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, linux-spi, devicetree,
	linux-kernel, linux

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


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-09  9:13               ` Alexander Stein
@ 2023-06-09  9:22                 ` Linus Walleij
  2023-06-09  9:34                 ` Mark Brown
  1 sibling, 0 replies; 17+ messages in thread
From: Linus Walleij @ 2023-06-09  9:22 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Matthias Schiffer, linux-arm-kernel, Mark Brown, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, linux-spi, devicetree,
	linux-kernel, linux

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-09  8:42             ` Linus Walleij
  2023-06-09  9:13               ` Alexander Stein
@ 2023-06-09  9:33               ` Mark Brown
  1 sibling, 0 replies; 17+ messages in thread
From: Mark Brown @ 2023-06-09  9:33 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Alexander Stein, Matthias Schiffer, linux-arm-kernel,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-spi,
	devicetree, linux-kernel, linux

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-09  9:13               ` Alexander Stein
  2023-06-09  9:22                 ` Linus Walleij
@ 2023-06-09  9:34                 ` Mark Brown
  1 sibling, 0 replies; 17+ messages in thread
From: Mark Brown @ 2023-06-09  9:34 UTC (permalink / raw)
  To: Alexander Stein
  Cc: Linus Walleij, Matthias Schiffer, linux-arm-kernel, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, linux-spi, devicetree,
	linux-kernel, linux

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-06 14:37   ` Linus Walleij
  2023-06-06 14:44     ` Mark Brown
@ 2023-06-14 19:30     ` Rob Herring
  2023-06-14 19:59       ` Linus Walleij
  1 sibling, 1 reply; 17+ messages in thread
From: Rob Herring @ 2023-06-14 19:30 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Mark Brown, Matthias Schiffer, Krzysztof Kozlowski, Conor Dooley,
	linux-spi, devicetree, linux-arm-kernel, linux-kernel, linux

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag
  2023-06-14 19:30     ` Rob Herring
@ 2023-06-14 19:59       ` Linus Walleij
  0 siblings, 0 replies; 17+ messages in thread
From: Linus Walleij @ 2023-06-14 19:59 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Brown, Matthias Schiffer, Krzysztof Kozlowski, Conor Dooley,
	linux-spi, devicetree, linux-arm-kernel, linux-kernel, linux,
	Wolfram Sang

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2023-06-14 20:00 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-02 11:52 [PATCH 1/2] spi: dt-bindings: introduce linux,use-rt-queue flag Matthias Schiffer
2023-06-02 11:52 ` [PATCH 2/2] spi: add support for generic " Matthias Schiffer
2023-06-02 12:22 ` [PATCH 1/2] spi: dt-bindings: introduce " Mark Brown
2023-06-06 14:37   ` Linus Walleij
2023-06-06 14:44     ` Mark Brown
2023-06-07 12:55       ` Matthias Schiffer
2023-06-07 14:21         ` Mark Brown
2023-06-07 14:28         ` Krzysztof Kozlowski
2023-06-09  7:41         ` Linus Walleij
2023-06-09  8:15           ` Alexander Stein
2023-06-09  8:42             ` Linus Walleij
2023-06-09  9:13               ` Alexander Stein
2023-06-09  9:22                 ` Linus Walleij
2023-06-09  9:34                 ` Mark Brown
2023-06-09  9:33               ` Mark Brown
2023-06-14 19:30     ` Rob Herring
2023-06-14 19:59       ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).