| Message ID | 1503363fcfb92adfa765d1c1c3e69fc673e72dde.1490264661.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, interesting, but why is this property specific to bcm2835 when the priority is used in generic code only? Best regards, Alexander On Thursday 23 March 2017 11:36:45, Lukas Wunner wrote: > Revolution Pi (a set of IoT products based on the Raspberry Pi and > geared towards industrial usage) uses the bcm2835's SPI master to > interface with an IEC 61158 fieldbus, which requires its kworker thread > to run at a realtime priority. To this end, allow the platform to > specify the desired priority. > > Cc: Mark Brown <broonie@kernel.org> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Eric Anholt <eric@anholt.net> > Cc: Mathias Duckeck <m.duckeck@kunbus.de> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt | 4 ++++ > drivers/spi/spi-bcm2835.c | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt > b/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt index > f11f295c8450..13ffc2b15e7d 100644 > --- a/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt > +++ b/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt > @@ -10,6 +10,10 @@ Required properties: > - interrupts: Should contain interrupt. > - clocks: The clock feeding the SPI controller. > > +Optional properties: > +- bcm2835,rt_prio: Realtime priority of the message pump to minimize > latency + on the bus (0..99). > + > Example: > > spi@20204000 { > diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c > index f35cc10772f6..319523686f73 100644 > --- a/drivers/spi/spi-bcm2835.c > +++ b/drivers/spi/spi-bcm2835.c > @@ -755,6 +755,8 @@ static int bcm2835_spi_probe(struct platform_device > *pdev) master->handle_err = bcm2835_spi_handle_err; > master->prepare_message = bcm2835_spi_prepare_message; > master->dev.of_node = pdev->dev.of_node; > + of_property_read_u32(pdev->dev.of_node, > + "bcm2835,rt_prio", &master->rt_prio); > > bs = spi_master_get_devdata(master); -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 11:36:45AM +0100, Lukas Wunner wrote: > +Optional properties: > +- bcm2835,rt_prio: Realtime priority of the message pump to minimize latency > + on the bus (0..99). > + This is going to depend on the software running on the board, I'd expect it to be something configured at runtime rather than fixed in DT.
On Thu, Mar 23, 2017 at 11:51:33AM +0100, Alexander Stein wrote: > interesting, but why is this property specific to bcm2835 when the priority is > used in generic code only? Well, I'd be happy to rework the patches to introduce a generic "rt-prio" property if that is preferred? Maybe Mark or some driver maintainers can chime in? For pl022, I guess I could let the legacy "pl022,rt" property take precedence over a generic "rt-prio" property. Thanks, Lukas > > Best regards, > Alexander > > On Thursday 23 March 2017 11:36:45, Lukas Wunner wrote: > > Revolution Pi (a set of IoT products based on the Raspberry Pi and > > geared towards industrial usage) uses the bcm2835's SPI master to > > interface with an IEC 61158 fieldbus, which requires its kworker thread > > to run at a realtime priority. To this end, allow the platform to > > specify the desired priority. > > > > Cc: Mark Brown <broonie@kernel.org> > > Cc: Stephen Warren <swarren@wwwdotorg.org> > > Cc: Eric Anholt <eric@anholt.net> > > Cc: Mathias Duckeck <m.duckeck@kunbus.de> > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > --- > > Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt | 4 ++++ > > drivers/spi/spi-bcm2835.c | 2 ++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt > > b/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt index > > f11f295c8450..13ffc2b15e7d 100644 > > --- a/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt > > +++ b/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt > > @@ -10,6 +10,10 @@ Required properties: > > - interrupts: Should contain interrupt. > > - clocks: The clock feeding the SPI controller. > > > > +Optional properties: > > +- bcm2835,rt_prio: Realtime priority of the message pump to minimize > > latency + on the bus (0..99). > > + > > Example: > > > > spi@20204000 { > > diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c > > index f35cc10772f6..319523686f73 100644 > > --- a/drivers/spi/spi-bcm2835.c > > +++ b/drivers/spi/spi-bcm2835.c > > @@ -755,6 +755,8 @@ static int bcm2835_spi_probe(struct platform_device > > *pdev) master->handle_err = bcm2835_spi_handle_err; > > master->prepare_message = bcm2835_spi_prepare_message; > > master->dev.of_node = pdev->dev.of_node; > > + of_property_read_u32(pdev->dev.of_node, > > + "bcm2835,rt_prio", &master->rt_prio); > > > > bs = spi_master_get_devdata(master); > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 11:07:18AM +0000, Mark Brown wrote: > On Thu, Mar 23, 2017 at 11:36:45AM +0100, Lukas Wunner wrote: > > +Optional properties: > > +- bcm2835,rt_prio: Realtime priority of the message pump to minimize latency > > + on the bus (0..99). > > + > > This is going to depend on the software running on the board, I'd expect > it to be something configured at runtime rather than fixed in DT. On the Revolution Pi, the SPI master is hardwired to two KSZ8851 Ethernet controllers which are connected to various fieldbus gateways in a plug-and-play fashion. Low latencies are needed to achieve a decent cycle rate when reading/writing data to the fieldbus devices. Hence for this use case, the RT priority is mandated by the hardware platform, which is why I put it in the devicetree. It is needed regardless of which software is running. We could accommodate to setting the RT priority on the command line but we'd like to avoid always having to set it at runtime. That is already possible by invoking chrt(1) from user space after the SPI master driver has loaded and wouldn't require a kernel patch. Also, if you question setting the RT priority in the devicetree, why was that functionality allowed for pl022 in the first place? Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 01:02:57PM +0100, Lukas Wunner wrote: > On Thu, Mar 23, 2017 at 11:07:18AM +0000, Mark Brown wrote: > > This is going to depend on the software running on the board, I'd expect > > it to be something configured at runtime rather than fixed in DT. > On the Revolution Pi, the SPI master is hardwired to two KSZ8851 Ethernet > controllers which are connected to various fieldbus gateways in a > plug-and-play fashion. Low latencies are needed to achieve a decent > cycle rate when reading/writing data to the fieldbus devices. > Hence for this use case, the RT priority is mandated by the hardware > platform, which is why I put it in the devicetree. It is needed > regardless of which software is running. Caring about having low latency access to fieldbus devices is a property of the application, and achieving that via the use of RT priority for the SPI pump thread is a property of current Linux implementations. Neither of these things is a description of the hardware, the SPI pump thread bit is already changing (for a very large proportion of use cases we only use the thread to shut down devices once they become idle these days). > We could accommodate to setting the RT priority on the command line but > we'd like to avoid always having to set it at runtime. That is already > possible by invoking chrt(1) from user space after the SPI master driver > has loaded and wouldn't require a kernel patch. Right, there's already mechanisms to do this at runtime. > Also, if you question setting the RT priority in the devicetree, why > was that functionality allowed for pl022 in the first place? This was being done via platform data not via device tree, unlike platform data device tree should provide a long term stable OS neutral ABI.
On Thu, Mar 23, 2017 at 1:02 PM, Lukas Wunner <lukas@wunner.de> wrote: > On Thu, Mar 23, 2017 at 11:07:18AM +0000, Mark Brown wrote: > Also, if you question setting the RT priority in the devicetree, why > was that functionality allowed for pl022 in the first place? It was for the same usecase as yours essentially: the U8500 PL022 is in one specific product connected to a 4G modem. When the traffic on the system gets high, the thread submitting new packets to the modem gets too low priority and Linux starts to prioritize the wrong things over getting data in and out to the modem. What can happen otherwise is that the threads producing ever more data get higher or same priority as the thread pushing the data to the modem, eventually saturating all buffers and causing buffer contention and long latencies. On desktops we have a similar thing in userspace, where the rt-daemon is asked by pulseaudio to elevate the priority of the pulseaudio process to realtime to avoid skipping audio frames. Assigning priorities is a kernelwide problem, so whether it is the right thing to do or not, I do not know. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 12:21:18PM +0000, Mark Brown wrote: > > Also, if you question setting the RT priority in the devicetree, why > > was that functionality allowed for pl022 in the first place? > > This was being done via platform data not via device tree, unlike > platform data device tree should provide a long term stable OS neutral > ABI. No, specifying it via the device tree was subsequently added with commit 39a6ac11df65 ("spi/pl022: Devicetree support w/o platform data"), sorry for not mentioning this in the commit message. There's already a bool in struct spi_master for this functionality and device tree support for pl022, so I don't quite understand why converting the bool to an int and adding device tree support for bcm2835 would be the wrong approach? Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 2:43 PM, Lukas Wunner <lukas@wunner.de> wrote: > On Thu, Mar 23, 2017 at 12:21:18PM +0000, Mark Brown wrote: >> > Also, if you question setting the RT priority in the devicetree, why >> > was that functionality allowed for pl022 in the first place? >> >> This was being done via platform data not via device tree, unlike >> platform data device tree should provide a long term stable OS neutral >> ABI. > > No, specifying it via the device tree was subsequently added with > commit 39a6ac11df65 ("spi/pl022: Devicetree support w/o platform data"), > sorry for not mentioning this in the commit message. This was in 2012 when we were walking around like zombies in devicetreeland and walking into the wall and breaking things to the left and right, just trying to comply with the recent decision to move all of ARM development over to using device tree. If the property would be allowed today it would at *least* be prefixed with "linux,*" so "linux,realtime-priority". Even though linux-specific DT properties are generally frowned upon they do have their place sometimes. If this DT property was suggested today it would get a very different treatment than in 2012. We are older and wiser now. Mistakes were made, also by me. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 23.03.2017, at 13:21, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Mar 23, 2017 at 01:02:57PM +0100, Lukas Wunner wrote: >> On Thu, Mar 23, 2017 at 11:07:18AM +0000, Mark Brown wrote: > >>> This is going to depend on the software running on the board, I'd expect >>> it to be something configured at runtime rather than fixed in DT. > >> On the Revolution Pi, the SPI master is hardwired to two KSZ8851 Ethernet >> controllers which are connected to various fieldbus gateways in a >> plug-and-play fashion. Low latencies are needed to achieve a decent >> cycle rate when reading/writing data to the fieldbus devices. > >> Hence for this use case, the RT priority is mandated by the hardware >> platform, which is why I put it in the devicetree. It is needed >> regardless of which software is running. > > Caring about having low latency access to fieldbus devices is a property > of the application, and achieving that via the use of RT priority for > the SPI pump thread is a property of current Linux implementations. > Neither of these things is a description of the hardware, the SPI pump > thread bit is already changing (for a very large proportion of use cases > we only use the thread to shut down devices once they become idle these > days). Some observations: * we use an “inline” spi-pump if there are no messages in the message queue - the spi-message-pump-thread is not used in those cases. So those spi messages are NOT handled with the proposed RT priority, as they run within the context of the “caller” - which may even be the priority of userspace if spidev is used. * the only place where the main message-pump is used is when there are spi_messages queued. And then you have a latency waking up the calling thread (which typically does not run with RT priority, so it is not necessarily woken up immediately by the scheduler either) - that is unless the caller use the spi_async with custom callbacks, that do not require a wakeup. * There are a few more things to take into account on the bcm283X: * for short SPI messages (<10ms I believe) idle looping is used to get the fastest response time without any rescheduling overhead and thus to reduce latencies. * for long transfers (byte-wise >96 bytes) the driver uses DMA to avoid interrupt-scheduling latencies (setup time of the DMA is a bit expensive, so 2 interrupts in interrupt-driven mode are acceptable, but anything else is handled via DMA to reduce the interrupts to 1 - OK, it is actually 2 DMA interrupts, but that is a limitation of the DMA framework (requiring INT for RX and TX DMA end - while we only really require RX-DMA interrupts, but that is for “ease” of releasing memory correctly…) So a lot of care has already been taken to improve the latencies of the driver and the framework. From my experience the main issue is more the side of interrupt-scheduling and wakeup which this rt patch does not solve - especially if a driver has to respond as fast as possible to a GPIO change (interrupt request from the device) this is typically the limiting factor. I remember I had a similar patch that used a module parameter to control it instead, but I had dropped it because of all the above as mostly not necessary (measurements with a logic analyzer my CAN setup did not show any real improvement). As a consequence of all this to make it really work you would need RT priority for: * calling thread (when using the “inline" message pump) * spi-message pump (for queued SPI_messages) * dma-termination-thread (for DMA transfers), as this gets woken from DMA interrupt for termination of the DMA transfer, which then wakes the thread running the spi-message pump. > >> We could accommodate to setting the RT priority on the command line but >> we'd like to avoid always having to set it at runtime. That is already >> possible by invoking chrt(1) from user space after the SPI master driver >> has loaded and wouldn't require a kernel patch. > > Right, there's already mechanisms to do this at runtime. > >> Also, if you question setting the RT priority in the devicetree, why >> was that functionality allowed for pl022 in the first place? > > This was being done via platform data not via device tree, unlike > platform data device tree should provide a long term stable OS neutral > ABI. Well - if it is really a necessity and as the mantra for DT is: "it should describe the HW only", why not use a module parameter instead or use chrt() from userspace? Martin-- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 02:48:01PM +0100, Linus Walleij wrote: > On Thu, Mar 23, 2017 at 2:43 PM, Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, Mar 23, 2017 at 12:21:18PM +0000, Mark Brown wrote: > >> > Also, if you question setting the RT priority in the devicetree, why > >> > was that functionality allowed for pl022 in the first place? > >> > >> This was being done via platform data not via device tree, unlike > >> platform data device tree should provide a long term stable OS neutral > >> ABI. > > > > No, specifying it via the device tree was subsequently added with > > commit 39a6ac11df65 ("spi/pl022: Devicetree support w/o platform data"), > > sorry for not mentioning this in the commit message. > > This was in 2012 when we were walking around like zombies in > devicetreeland and walking into the wall and breaking things to the > left and right, just trying to comply with the recent decision to move > all of ARM development over to using device tree. > > If the property would be allowed today it would at *least* be prefixed > with "linux,*" so "linux,realtime-priority". > > Even though linux-specific DT properties are generally frowned upon > they do have their place sometimes. > > If this DT property was suggested today it would get a very different > treatment than in 2012. We are older and wiser now. Mistakes were > made, also by me. Thanks a lot Linus for providing the historic background, which was unbeknown to me. I'm still wondering though: If the master thread is supposed to be run at a realtime priority *always*, 100%, all the time, what is the best way to achieve this? Setting it with chrt from an initscript seems clumsy to me. It's racy as the spi driver module needs to be loaded, whenever the driver module is removed and re-inserted, the priority needs to be updated again, there's always the difficulty of uniquely identifying the right thread, and so on. Setting this from the device tree is much more elegant so I intend to keep the commits at least on our own branch. Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 02:43:36PM +0100, Lukas Wunner wrote: > On Thu, Mar 23, 2017 at 12:21:18PM +0000, Mark Brown wrote: > > This was being done via platform data not via device tree, unlike > > platform data device tree should provide a long term stable OS neutral > > ABI. > No, specifying it via the device tree was subsequently added with > commit 39a6ac11df65 ("spi/pl022: Devicetree support w/o platform data"), > sorry for not mentioning this in the commit message. > There's already a bool in struct spi_master for this functionality and > device tree support for pl022, so I don't quite understand why converting > the bool to an int and adding device tree support for bcm2835 would be > the wrong approach? The presence of the device tree support for pl022 is an oversight during the initial DT transition. It's perfectly fine for the functionality to be there and used in running systems but it's not something that makes sense in device tree as it's too much of an implementation detail.
On Thu, Mar 23, 2017 at 03:28:16PM +0100, Lukas Wunner wrote: > I'm still wondering though: If the master thread is supposed to be > run at a realtime priority *always*, 100%, all the time, what is the > best way to achieve this? Setting it with chrt from an initscript > seems clumsy to me. It's racy as the spi driver module needs to be > loaded, whenever the driver module is removed and re-inserted, the > priority needs to be updated again, there's always the difficulty of > uniquely identifying the right thread, and so on. Setting this from > the device tree is much more elegant so I intend to keep the commits > at least on our own branch. What would be great would be if the priority were communicated from the device driver using the controller (which might in turn want to get it from their users) rather than hard coding something into the middle of the implementation.
On Thu, Mar 23, 2017 at 03:08:01PM +0100, kernel@martin.sperl.org wrote: > * we use an “inline” spi-pump if there are no messages in the message > queue - the spi-message-pump-thread is not used in those cases. > So those spi messages are NOT handled with the proposed RT priority, > as they run within the context of the “caller” - which may even be > the priority of userspace if spidev is used. Yeah, this might actually make things worse if the RT thread is used since unless there's multiple contending users or async users the pump is only responsible for idling the controller when it goes idle so making it RT can result in more idle/resume cycles (though they are fairly cheap usually) and context thrashing doing that. > > This was being done via platform data not via device tree, unlike > > platform data device tree should provide a long term stable OS neutral > > ABI. > Well - if it is really a necessity and as the mantra for DT is: > "it should describe the HW only", why not use a module parameter > instead or use chrt() from userspace? Yes, those are fine.
Hi Lukas, Am 23.03.2017 um 11:36 schrieb Lukas Wunner: > Revolution Pi (a set of IoT products based on the Raspberry Pi and > geared towards industrial usage) uses the bcm2835's SPI master to > interface with an IEC 61158 fieldbus, which requires its kworker thread > to run at a realtime priority. To this end, allow the platform to > specify the desired priority. > just out of curiosity are there any plans to upstream the Revolution Pi dts file? Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 05:33:07PM +0100, Stefan Wahren wrote: > Am 23.03.2017 um 11:36 schrieb Lukas Wunner: > > Revolution Pi (a set of IoT products based on the Raspberry Pi and > > geared towards industrial usage) uses the bcm2835's SPI master to > > interface with an IEC 61158 fieldbus, which requires its kworker thread > > to run at a realtime priority. To this end, allow the platform to > > specify the desired priority. > > just out of curiosity are there any plans to upstream the Revolution Pi > dts file? Generally we do try to upstream as much as possible, but in the case of the devicetree we're not sure what the benefits would be for the community and for us, so we don't have an opinion on this yet. We use a few of the Foundation's overlays with parameters tailored to the Revolution Pi, plus a custom overlay. All of that is public: https://github.com/RevolutionPi/imagebakery/blob/master/templates/config.txt#L62 https://github.com/RevolutionPi/kernelbakery/blob/master/debian/kunbus.dts Thanks for your interest, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt b/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt index f11f295c8450..13ffc2b15e7d 100644 --- a/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt +++ b/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt @@ -10,6 +10,10 @@ Required properties: - interrupts: Should contain interrupt. - clocks: The clock feeding the SPI controller. +Optional properties: +- bcm2835,rt_prio: Realtime priority of the message pump to minimize latency + on the bus (0..99). + Example: spi@20204000 { diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c index f35cc10772f6..319523686f73 100644 --- a/drivers/spi/spi-bcm2835.c +++ b/drivers/spi/spi-bcm2835.c @@ -755,6 +755,8 @@ static int bcm2835_spi_probe(struct platform_device *pdev) master->handle_err = bcm2835_spi_handle_err; master->prepare_message = bcm2835_spi_prepare_message; master->dev.of_node = pdev->dev.of_node; + of_property_read_u32(pdev->dev.of_node, + "bcm2835,rt_prio", &master->rt_prio); bs = spi_master_get_devdata(master);
Revolution Pi (a set of IoT products based on the Raspberry Pi and geared towards industrial usage) uses the bcm2835's SPI master to interface with an IEC 61158 fieldbus, which requires its kworker thread to run at a realtime priority. To this end, allow the platform to specify the desired priority. Cc: Mark Brown <broonie@kernel.org> Cc: Stephen Warren <swarren@wwwdotorg.org> Cc: Eric Anholt <eric@anholt.net> Cc: Mathias Duckeck <m.duckeck@kunbus.de> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt | 4 ++++ drivers/spi/spi-bcm2835.c | 2 ++ 2 files changed, 6 insertions(+)