[PATCH 1/2] spi: Allow master drivers to set realtime priority

[PATCH 1/2] spi: Allow master drivers to set realtime priority

[Lukas Wunner]

Commit 14af60b6fb3b ("spi/pl022: Add high priority message pump
support") extended the pl022 driver to optionally run the message pump
kworker thread with realtime priority, subject to a bool set by the
platform.

Commit ffbbdd21329f ("spi: create a message queueing infrastructure")
moved a large portion of pl022 to generic code, making the realtime
priority support available to other drivers.

However the priority is set to MAX_RT_PRIO - 1, higher than most IRQs
and identical to the CPU timer clock threads.  This seems excessive so
make the priority configurable.

Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Chris Blair <chris.blair-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Mathias Duckeck <m.duckeck-XB/JSsFECOqzQB+pC5nmwQ@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/spi/spi-pl022.c | 4 +++-
 drivers/spi/spi.c       | 8 +++++---
 include/linux/spi/spi.h | 4 ++--
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index f7f7ba17b40e..f66cc3b6d489 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2153,9 +2153,11 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	master->auto_runtime_pm = true;
 	master->transfer_one_message = pl022_transfer_one_message;
 	master->unprepare_transfer_hardware = pl022_unprepare_transfer_hardware;
-	master->rt = platform_info->rt;
 	master->dev.of_node = dev->of_node;
 
+	if (platform_info->rt)
+		master->rt_prio = MAX_RT_PRIO - 1;
+
 	if (platform_info->num_chipselect && platform_info->chipselects) {
 		for (i = 0; i < num_cs; i++)
 			pl022->chipselects[i] = platform_info->chipselects[i];
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index f699ea530b88..3220f6d87a66 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1286,7 +1286,7 @@ static void spi_pump_messages(struct kthread_work *work)
 
 static int spi_init_queue(struct spi_master *master)
 {
-	struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
+	struct sched_param param = { .sched_priority = master->rt_prio };
 
 	master->running = false;
 	master->busy = false;
@@ -1308,9 +1308,10 @@ static int spi_init_queue(struct spi_master *master)
 	 * request and the scheduling of the message pump thread. Without this
 	 * setting the message pump thread will remain at default priority.
 	 */
-	if (master->rt) {
+	if (master->rt_prio >= 0 && master->rt_prio < MAX_RT_PRIO) {
 		dev_info(&master->dev,
-			"will run message pump with realtime priority\n");
+			 "will run message pump with realtime priority %d\n",
+			 master->rt_prio);
 		sched_setscheduler(master->kworker_task, SCHED_FIFO, &param);
 	}
 
@@ -1862,6 +1863,7 @@ struct spi_master *spi_alloc_master(struct device *dev, unsigned size)
 
 	device_initialize(&master->dev);
 	master->bus_num = -1;
+	master->rt_prio = -1;
 	master->num_chipselect = 1;
 	master->dev.class = &spi_master_class;
 	master->dev.parent = dev;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 5a8c4b24f2dc..5ec405669668 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -341,11 +341,11 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @xfer_completion: used by core transfer_one_message()
  * @busy: message pump is busy
  * @running: message pump is running
- * @rt: whether this queue is set to run as a realtime task
  * @auto_runtime_pm: the core should ensure a runtime PM reference is held
  *                   while the hardware is prepared, using the parent
  *                   device for the spidev
  * @max_dma_len: Maximum length of a DMA transfer for the device.
+ * @rt_prio: realtime priority of the message pump (-1 to use default prio)
  * @prepare_transfer_hardware: a message will soon arrive from the queue
  *	so the subsystem requests the driver to prepare the transfer hardware
  *	by issuing this call
@@ -522,12 +522,12 @@ struct spi_master {
 	bool				idling;
 	bool				busy;
 	bool				running;
-	bool				rt;
 	bool				auto_runtime_pm;
 	bool                            cur_msg_prepared;
 	bool				cur_msg_mapped;
 	struct completion               xfer_completion;
 	size_t				max_dma_len;
+	int				rt_prio;
 
 	int (*prepare_transfer_hardware)(struct spi_master *master);
 	int (*transfer_one_message)(struct spi_master *master,
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

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

Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
Cc: Mathias Duckeck <m.duckeck-XB/JSsFECOqzQB+pC5nmwQ@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 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);
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Alexander Stein]

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> Cc: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> Cc: Mathias Duckeck <m.duckeck-XB/JSsFECOqzQB+pC5nmwQ@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Mark Brown]

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

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.

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

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Lukas Wunner]

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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> > Cc: Eric Anholt <eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org>
> > Cc: Mathias Duckeck <m.duckeck-XB/JSsFECOqzQB+pC5nmwQ@public.gmane.org>
> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > ---
> >  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Lukas Wunner]

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

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Mark Brown]

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

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.

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

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Linus Walleij]

On Thu, Mar 23, 2017 at 1:02 PM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] spi: Allow master drivers to set realtime priority

[Linus Walleij]

On Thu, Mar 23, 2017 at 11:36 AM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:

> Commit 14af60b6fb3b ("spi/pl022: Add high priority message pump
> support") extended the pl022 driver to optionally run the message pump
> kworker thread with realtime priority, subject to a bool set by the
> platform.
>
> Commit ffbbdd21329f ("spi: create a message queueing infrastructure")
> moved a large portion of pl022 to generic code, making the realtime
> priority support available to other drivers.
>
> However the priority is set to MAX_RT_PRIO - 1, higher than most IRQs
> and identical to the CPU timer clock threads.  This seems excessive so
> make the priority configurable.
>
> Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Chris Blair <chris.blair-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Mathias Duckeck <m.duckeck-XB/JSsFECOqzQB+pC5nmwQ@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>

Looks reasonable to me.
Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Lukas Wunner]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Linus Walleij]

On Thu, Mar 23, 2017 at 2:43 PM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[kernel-TqfNSX0MhmxHKSADF0wUEw]

> 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
_______________________________________________
linux-rpi-kernel mailing list
linux-rpi-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Lukas Wunner]

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-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Mark Brown]

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

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.

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

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Mark Brown]

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

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.

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

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Mark Brown]

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

On Thu, Mar 23, 2017 at 03:08:01PM +0100, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.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.

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

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Stefan Wahren]

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

Re: [PATCH 2/2] spi: bcm2835: Allow platform to set realtime priority

[Lukas Wunner]

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