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