Additional SPI-master parameters per SPI-Device in device-tree

Additional SPI-master parameters per SPI-Device in device-tree

[Martin Sperl]

Hi!

How could we implement additional per spi-device parameters that
are interesting to the spi-master.

Examples for the spi-bcm2835 master for example could be:
* set minimum transfer size for dma to something else but the default
    (of 96 bytes - setting for something high would disable it)
* change polling transfer limits (i.e disable it)

Implementing these on a spi-master level is easy during probe.

So I wonder if the following approach would be acceptable
to implement device-tree parsing on the device for master
specific parameters:

int bcm2835_spi_setup(struct spi_device *spi)
{
   struct bcm2835_spi_dev *cs = spi->controller_state;
   if (!cs) {
     cs = devm_kzalloc(&spi->dev, sizeof(struct bcm2835_spi_dev), 0);
     if (!cs)
         return -ENOMEM;
     spi->controller_state = cs;

     /* parse device-tree and assign to controller_state */
     cs->polling_limit = master_bs->polling_limit;
     of_property_read_u32(nc, "spi-bcm2835-polling-limit", 
&cs->polling_limit);
     cs->dma_limit = master_bs->dma_limit;
     of_property_read_u32(nc, "spi-bcm2835-dma-limit", &cs->polling_limit);
   }
   ...
   return 0;
}

Or would it be better if we included an additional master method
that would get called from spi_add_device to do this kind of setup?
say:
   int add_device(struct spi_master *master, struct spi_device *spi);
(possibly also one called when removing the device)

Thanks,
                 Martin

--
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: Additional SPI-master parameters per SPI-Device in device-tree

[Mark Brown]

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

On Tue, Aug 04, 2015 at 08:39:20AM +0200, Martin Sperl wrote:

> How could we implement additional per spi-device parameters that
> are interesting to the spi-master.

Define parameters for the slave and read them from the slave device
node at some appropriate point.  Please send patches for review, your
sketches look fine without context but when written as a proper patch
other issues may become obvious.  Given that setup() gets called
repeatedly a one time call may be more sensible.

> Examples for the spi-bcm2835 master for example could be:
> * set minimum transfer size for dma to something else but the default
>    (of 96 bytes - setting for something high would disable it)
> * change polling transfer limits (i.e disable it)

Why would these things be configured in the device tree in the first
place?  They don't look like things that should be device tree
properties, they look like something the kernel should figure out at
runtime - the system tuning may change as the kernel evolves or
depending on userspace needs.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Additional SPI-master parameters per SPI-Device in device-tree

[Martin Sperl]

On 04.08.2015 11:54, Mark Brown wrote:
> On Tue, Aug 04, 2015 at 08:39:20AM +0200, Martin Sperl wrote:
>> Examples for the spi-bcm2835 master for example could be:
>> * set minimum transfer size for dma to something else but the default
>>     (of 96 bytes - setting for something high would disable it)
>> * change polling transfer limits (i.e disable it)
>
> Why would these things be configured in the device tree in the first
> place?  They don't look like things that should be device tree
> properties, they look like something the kernel should figure out at
> runtime - the system tuning may change as the kernel evolves or
> depending on userspace needs.

Sometimes it may be necessary to define different values than the
"kernel" thinks may be necessary (hardcoded).

We just had one issue with fbtft devices where it would have been
nice to change those values on a per device basis (even if it was
just for some testing to see why it was behaving badly
- see the still un-merged patch:
"spi: bcm2835: set up spi-mode before asserting cs-gpio").

So I first thought of adding it as a module parameter for the driver
and then exposing it via sysfs for modification, but now I think it
might be even more beneficial to "tune" the parameters per device.

Say for fbtft devices to use DMA for any xfer-size above 4 bytes.
But that policy may not apply to the corresponding touch screen device
also connected to the same SPI bus.

But then - I now come to realize - that this "dt-property" idea
may again break the "philosophy" of "device-tree only contains
a description of the HW".

So maybe a per device sysfs approach together with udev-rules to
change those settings may be the better approach.

But even those may require those device registration hooks I had
mentioned.

So I was wondering what was feasible/acceptable.

Martin
--
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: Additional SPI-master parameters per SPI-Device in device-tree

[Mark Brown]

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

On Tue, Aug 04, 2015 at 12:09:52PM +0200, Martin Sperl wrote:
> On 04.08.2015 11:54, Mark Brown wrote:

> >Why would these things be configured in the device tree in the first
> >place?  They don't look like things that should be device tree
> >properties, they look like something the kernel should figure out at
> >runtime - the system tuning may change as the kernel evolves or
> >depending on userspace needs.

> Sometimes it may be necessary to define different values than the
> "kernel" thinks may be necessary (hardcoded).

So improve the kernel or (if you're really desparate) provide runtime
configuration - if you're just randomly bashing on things you can
rebuild the kernel just as well as the DT.  We don't want random magic
number performance tuning becoming an ABI, that way if the kernel
changes we're stuck with trying to work with the old magic numbers even
if they no longer work or perform well.

> We just had one issue with fbtft devices where it would have been
> nice to change those values on a per device basis (even if it was
> just for some testing to see why it was behaving badly
> - see the still un-merged patch:
> "spi: bcm2835: set up spi-mode before asserting cs-gpio").

I'm not finding that patch particularly enlightening here, sorry...  can
you be more explicit please?

> So I was wondering what was feasible/acceptable.

There are properties that might make sense on a per device basis like
descriptions of which chip select to use in the hardware, it's just that
these don't seem like such properties.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Additional SPI-master parameters per SPI-Device in device-tree

[Martin Sperl]

On 04.08.2015 13:15, Mark Brown wrote:
>> We just had one issue with fbtft devices where it would have been
>> nice to change those values on a per device basis (even if it was
>> just for some testing to see why it was behaving badly
>> - see the still un-merged patch:
>> "spi: bcm2835: set up spi-mode before asserting cs-gpio").
>
> I'm not finding that patch particularly enlightening here, sorry...  can
> you be more explicit please?
Well, while trying to figure the issue out we tried to disable or tune
different parameters to see where the issue really came from.

At that time we had to recompile the kernel several times, which is a 
much longer turn-arround than tuning parameters on the fly.

In the end the issue (which the patch fixes) turned out not to be
related to any of those parameters, but a default-clock-polarity change
while GPIO-CS was already asserted resulting in an extra level-change
on the clock getting interpreted by the HW as a clock change, which the
above mentioned patch fixes by setting up polarity before GPIO-CS.

That is where the idea of configuring it in DT or via sysfs came from.

Martin



--
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: Additional SPI-master parameters per SPI-Device in device-tree

[Mark Brown]

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

On Tue, Aug 04, 2015 at 01:37:48PM +0200, Martin Sperl wrote:
> On 04.08.2015 13:15, Mark Brown wrote:

> >I'm not finding that patch particularly enlightening here, sorry...  can
> >you be more explicit please?

> Well, while trying to figure the issue out we tried to disable or tune
> different parameters to see where the issue really came from.

> At that time we had to recompile the kernel several times, which is a much
> longer turn-arround than tuning parameters on the fly.

Right, and runtime tuning is much easier since it can be done with less
of a strong ABI than the DT.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]