Date   

Daily Gerrit Digest

donotreply@...
 

NEW within last 24 hours:
- https://gerrit.zephyrproject.org/r/4791 : Bluetooth: Controller: Refactor HCI files
- https://gerrit.zephyrproject.org/r/4788 : linker: fix typos
- https://gerrit.zephyrproject.org/r/4796 : Bluetooth: L2CAP: Handle security procedure non successful path
- https://gerrit.zephyrproject.org/r/4795 : Bluetooth: L2CAP: Refactor handling connection response
- https://gerrit.zephyrproject.org/r/4794 : Bluetooth: A2DP: Initialization of A2DP
- https://gerrit.zephyrproject.org/r/4789 : unified/x86: fix IAMCU build
- https://gerrit.zephyrproject.org/r/4790 : fs: Fixes a bug that limits volume size to 1MB
- https://gerrit.zephyrproject.org/r/4785 : unified: Add timeslice support
- https://gerrit.zephyrproject.org/r/4784 : unified: Preemption check to include sched lock

UPDATED within last 24 hours:
- https://gerrit.zephyrproject.org/r/4487 : Bluetooth: SDP: Server: Support service record registration
- https://gerrit.zephyrproject.org/r/4450 : tests: Add gcov support
- https://gerrit.zephyrproject.org/r/4357 : tests: Add a sample for testing natively
- https://gerrit.zephyrproject.org/r/4356 : tests: convert tests/net/buf to the new framework
- https://gerrit.zephyrproject.org/r/4355 : ztest: Add simple integration and unit tests
- https://gerrit.zephyrproject.org/r/4354 : ztest: Add documentation
- https://gerrit.zephyrproject.org/r/4353 : ztest: Add native building support
- https://gerrit.zephyrproject.org/r/4118 : tests: Add a generic testing framework
- https://gerrit.zephyrproject.org/r/4486 : Bluetooth: SDP: Server: Initialize and accept incoming connections
- https://gerrit.zephyrproject.org/r/4489 : Bluetooth: SDP: Server: Support ServiceAttrReq and ServiceSearchAttrReq
- https://gerrit.zephyrproject.org/r/4488 : Bluetooth: SDP: Server: Support ServiceSearchRequest
- https://gerrit.zephyrproject.org/r/4321 : Bluetooth: BR/EDR: Refactor distribution of security procedure status
- https://gerrit.zephyrproject.org/r/4447 : Bluetooth: AVDTP: Module Initialization
- https://gerrit.zephyrproject.org/r/4101 : tests/mem_safe: place test buffers at the edges of RAM
- https://gerrit.zephyrproject.org/r/4100 : build: allow specifying a custom linker script relative to project
- https://gerrit.zephyrproject.org/r/4774 : intel_quark: move X86_IAMCU to defconfig
- https://gerrit.zephyrproject.org/r/4621 : eth: Initial release to tx semaphore for the ENC28J60 driver.
- https://gerrit.zephyrproject.org/r/4623 : eth: Adjust ENC28J60 transmission/reception return codes.
- https://gerrit.zephyrproject.org/r/4127 : qmsi:wdt: remove duplicated wdt clock enable code
- https://gerrit.zephyrproject.org/r/4511 : unified/doc: Kernel primer for unified kernel
- https://gerrit.zephyrproject.org/r/3922 : lib: Add HTTP support for Zephyr
- https://gerrit.zephyrproject.org/r/4780 : unified: Fix semaphore group tests
- https://gerrit.zephyrproject.org/r/4779 : unified: Add support for semaphore groups
- https://gerrit.zephyrproject.org/r/4778 : unified: Add SEMAPHORE_GROUPS Kconfig option
- https://gerrit.zephyrproject.org/r/4781 : unified: Enable semaphore group use in test_mail
- https://gerrit.zephyrproject.org/r/3924 : lib/http: Add test-case for HTTP header fields
- https://gerrit.zephyrproject.org/r/4776 : unified: Conditionally define __printf_like() macro
- https://gerrit.zephyrproject.org/r/4624 : eth: Add ENC28J60 support into Paho's MQTT publisher example
- https://gerrit.zephyrproject.org/r/4777 : unified: Include _timeout structure in tcs_base
- https://gerrit.zephyrproject.org/r/4625 : eth: ENC28J60 sample application
- https://gerrit.zephyrproject.org/r/4622 : eth: Add full-duplex configuration to ENC28J60
- https://gerrit.zephyrproject.org/r/4670 : intel_quark: Group Quark SoCs under intel_quark/
- https://gerrit.zephyrproject.org/r/4420 : boards: rename Quark SE Devboard to Quark SE C1000 (Sensor Subsystem)
- https://gerrit.zephyrproject.org/r/4423 : MAINTAINERS: add maintainer for some of the boards
- https://gerrit.zephyrproject.org/r/4687 : arduino 101: make factory bootloader config the default
- https://gerrit.zephyrproject.org/r/4684 : quark_x1000: move the X1000 into the intel_quark family
- https://gerrit.zephyrproject.org/r/4422 : boards: rename Quark SE Devboard to Quark SE C1000
- https://gerrit.zephyrproject.org/r/4671 : soc: intel_quark: move quark d2000 to intel_quark family
- https://gerrit.zephyrproject.org/r/4689 : parse board defconfig at the very end
- https://gerrit.zephyrproject.org/r/4757 : sanity: test_context build only for qemu arm
- https://gerrit.zephyrproject.org/r/4688 : boards: rename arduino_101_sss to arduino_101_ss

MERGED within last 24 hours:
- https://gerrit.zephyrproject.org/r/4793 : Bluetooth: HCI: Fix updating RPA too early
- https://gerrit.zephyrproject.org/r/4792 : Bluetooth: Init: Updated filiter options for test_20
- https://gerrit.zephyrproject.org/r/4783 : ipm.h: fix erroneous edit of documentation
- https://gerrit.zephyrproject.org/r/4747 : tinycrypt: Solve style issues
- https://gerrit.zephyrproject.org/r/4745 : tinycrypt: Rename current tests to avoid confusions with new algorithms
- https://gerrit.zephyrproject.org/r/4770 : tinycrypt: Add test case for the CTR PRNG algorithm
- https://gerrit.zephyrproject.org/r/4771 : net: tests: Remove broadcast and multicast tests from ARP
- https://gerrit.zephyrproject.org/r/4707 : net: yaip: Fix slip multipackets reception
- https://gerrit.zephyrproject.org/r/4708 : net: yaip: Fix arp/ethernet broadcast and multcast addr scenario
- https://gerrit.zephyrproject.org/r/4710 : net: yaip: Fix IPv4 packet reception
- https://gerrit.zephyrproject.org/r/4711 : net: tests: Add include dir only when specific options enabled
- https://gerrit.zephyrproject.org/r/4713 : net: yaip: Fix net address state
- https://gerrit.zephyrproject.org/r/4715 : net: yaip: Add DHCPv4 client support
- https://gerrit.zephyrproject.org/r/4716 : net: tests: Add DHCPv4 client unit tests
- https://gerrit.zephyrproject.org/r/4717 : net: apps: Add DHCPv4 client sample application
- https://gerrit.zephyrproject.org/r/4782 : samples: zoap server: exclude quark d2000 not enough ram
- https://gerrit.zephyrproject.org/r/4768 : known issues: update SKIP regex
- https://gerrit.zephyrproject.org/r/4585 : samples: use printf/printk directly instead of macros
- https://gerrit.zephyrproject.org/r/4551 : tests: move test code from samples to tests
- https://gerrit.zephyrproject.org/r/4773 : Bluetooth: Controller: Make HCI endianness-independent
- https://gerrit.zephyrproject.org/r/4775 : gpio: stm32: Fix bug introduced by removing API 1.0 support


Re: [RFC]PWM API Update

Andy Ross
 

Liu, Baohong wrote:
As for the unit for period and pulse width, I understand that both time
(micro-second) and clock cycles are popularly used. To cater for this,
the above-mentioned API will be expanded into two.

pwm_set_pin_usec(uint8_t pin, uint32_t period_usec, uint32_t pulse_usec)
pwm_set_pin_cycles(uint8_t pin, uint32_t period_cycles, uint32_t pulse_cycles)
When PWM is used correctly, this shouldn't make any difference at all
because the period will be much, much longer than the underlying clock (which
is the whole point).

Why bother with having two ways to do this when they're going to be
exactly equivalent in all but the weirdest apps? Just set them in
arbitrary units of "cycles". And if an application really, truly
needs to know the underlying cycle time of the hardware (which is
going to be device-dependent), give them an API like
"pwm_get_cycle_time()" which returns a cycle time in picoseconds or
something.

Andy


Re: Porting to Cortex-M0+

Patel, Niheer <Niheer.Patel@...>
 

Hi Piotr,


On Sep 15, 2016, at 7:06 AM, Piotr Mienkowski <Piotr.Mienkowski(a)schmid-telecom.ch> wrote:

I brought this to the TSC yesterday, and the decision was to first ask Atmel if they can
change to a standard license already approved by the Zephyr governing board.
Another member of the TSC will follow up with Atmel.
Hi Maureen,

Do you have maybe any news about Atmel's header files license?
I have taken the action from Maureen to find out more information from Atmel. Unfortunately, I have not received any response from the folks I have reached out to. Please standby as I continue to follow up.

Regards,
Niheer


Cheers,
Piotr


Re: Porting to Cortex-M0+

Piotr Mienkowski <piotr.mienkowski@...>
 

I brought this to the TSC yesterday, and the decision was to first ask Atmel if they can
change to a standard license already approved by the Zephyr governing board.
Another member of the TSC will follow up with Atmel.
Hi Maureen,

Do you have maybe any news about Atmel's header files license?

Cheers,
Piotr


Re: [RFC] SPI API update

Tomasz Bursztyka
 

Hi Jon,

I was assuming 'lock' == mutex/semaphore and would be the same method
you would use in a single function. E.g.

Single function...
lock
do some work
unlock

Separate function1
lock
do some work
Separate function2
do some work
Separate function3
do some work
unlock

The SPI bus is a shared resource, and users claim use of if for using
it.
As I said your example does not work unless you keep track of who owns
the lock.
What about your lock that you'll use in the RFC's proposed of
spi_tranceive? I think I've must have misunderstood something
fundamental, because I see so difference between

A();
B();
C();

and

void F()
{
A();
B();
C();
}

F();
We don't have a spin_lock() in Zephyr. I think you missed that.
So there is a big difference for handling the lock, as the semaphore
object has no clue who owns it.

But anyway, it's not nice to expose too low level SPI concept (CS on/off
thingy) to the user.
That would be like going backward, from user experience point of view.

and the compiler would also agree.
Compiler has nothing to do with that, seriously.

I'm not trying to press my API suggestions on people, I'll code to
whatever Zephyr specifies. I was carrying on these emails to try and
clear up my own misunderstanding of this as much as anything. I'll stop
now, and review any code that is forthcoming. It's a lot easier to talk
about real code and say things like 'what if we are prempted at this
line here...' to which the answer may well be 'we can't be preempted
here because of X'.
Sure, we will need as many pair of eyes as possible.


Tomasz


Re: [RFC] SPI API update

Tomasz Bursztyka
 

Hi Baohong,


-----Original Message-----
From: Tomasz Bursztyka [mailto:tomasz.bursztyka(a)linux.intel.com]
Sent: Wednesday, September 14, 2016 12:31 AM
To: Liu, Baohong <baohong.liu(a)intel.com>; devel(a)lists.zephyrproject.org
Subject: Re: [devel] [RFC] SPI API update

Hi Baohong,

inline int spi_transceive(struct spi_config *conf, uint32_t slave,
struct spi_buffer *tx, uint32_t nb_tx_buf,
struct spi_buffer *rx, uint32_t
nb_rx_buf);

GPIO is and will be used for virtually any cases since the maximum
duration of the CS from the controller is so short. So, slave select
is just giving a non-zero value to make the controller happy and the
GPIO pin is selecting the slave device. But, in the current code, only
one GPIO pin is defined for each controller by Kconfig. In other words, one
controller is only able to control one slave. This needs to be addressed.

This GPIO used as CS is a hack for - currently - DW controllers. When hw
integration makes it available (it's possible on QuarkSE X86 core, not on
QuarkSE ARC core for instance).

So indeed, it's very much limited for now, but I don't see any reason to
expose anything about it in the public API. Hack should stay hidden, deep, in
driver's specific code.
User or app writer needs to be aware of this. User needs the info to connect
the external or on-board devices. This will be part of user-configurable setting.
It can't be hidden.
As I said, it's board specific. So up to the user to provide the right
info in board.h (and even some
code in board.c, if relevant), but from SPI API point of view, no.

I'll find some time, probably not this week, to make a PoC on that.
You'll see it's not going to
be complicated for the user.

Tomasz


Re: A possible case of TCP server no-response, was: Re: bufs lost in TCP connection establishment

Flavio Santes <flavio.santes@...>
 

Hello,

Hello Rohit, et al,

Thanks for the information below. I didn't yet have chance to test
multiple TCP/IPv4 connections in row, because I have issue with
reliably handling one. Just as before, my setup is a scripting language
(MicroPython), using combination of Zephyr and uIP APIs to emulate BSD
sockets. I however proceeded to play with mbedTLS on top of sockets
implemented in such way. That means multiple bidirectional packet
exchanges which exposes quite a number of edge conditions. There're
packets can be dropped anytime.

One particularly naughty conditions I experienced is that while my
usual testcase usually proceeded beyond connect() call, sometimes I got
time "strips" when connect() call just hanged (I don't use timeouts,
like BSD sockets by default) - with consecutive dozen or so of QEMU
restarts (I'm using QEMU virtual networking still so far). Then it
suddenly started working again, to repeat soon again. Looking with
Wireshark, Zephyr app just was sending SYN packets, with zero reply
from a Linux side. At this time, it's easy to start wonder what gets
wrong - Linux handling of TUN interface, tunslip's setting it up, socat
which is involved in connecting tunslip and QEMU, Wireshark, or
something else.

I set to debug just that weird case, with initial hypothesis that Linux
SYN flood protection may be involved. With some googling and netstat
grepping, I found that when I get those can't-connect time strips,
there's actually ESTABLISHED connection on the host. That led me to
http://stackoverflow.com/questions/6825036/what-will-happen-if-i-send-a-s...
,
which finished the picture.

So, uIP/Zephyr don't use source port randomization, and always open
connection from port 1025. While code snippets I ran included closing
a socket, if something went wrong, like a packet was lost during
initial connection or TLS exchange, the connection wasn't closed
explicitly, but left open on server. On QEMU restart, it started to
send SYN to establish new connection from the same source port, which,
as explained in the link above, gets zero response from a Linux host.
Eventually Apache on host side times out that client connection, then
the situation recovers.

The solution is thus to check whether there's ESTABLISHED connection on
host side with netstat and restart a server process which owns it
(apache in my case).

The same situation may surely happen with a real device too, so I hope
this mail may save debugging time to some readers.
This is a hardware-agnostic issue, see: https://jira.zephyrproject.org/browse/ZEP-428.
The issue description is not really useful, although comments will help.
BTW, we apply the same workaround: restart the server :)


Thanks,
Paul


On Wed, 7 Sep 2016 15:26:23 +0000
Rohit Grover <Rohit.Grover(a)arm.com&gt; wrote:
Regards,
Flavio


[RFC]PWM API Update

Liu, Baohong
 

Hi everyone,

I would like to propose the re-design of PWM API interfaces.
(https://jira.zephyrproject.org/browse/ZEP-745).

PWM APIs had been through a few times of updates from different aspects.
The following are the known problems.
--------

- Argument "flags" in PWM config API is not used and its meaning is not clear.

- Usage of PWM set phase API is not clear since only zero phase is supported.
The definition of phase is not meaningful for most of the PWM devices.

- PWM set values API seems redundant since PWM set period API and PWM
set duty cycle API together can accomplish the same thing.

Proposed Solution
--------

Period, pulse width are the only parameters needed for PWM.
So, the following API will be enough to set up PWM.

pwm_set_pin(uint8_t pin, uint32_t period, uint32_t pulse_width)

As for the unit for period and pulse width, I understand that both time
(micro-second) and clock cycles are popularly used. To cater for this,
the above-mentioned API will be expanded into two.

pwm_set_pin_usec(uint8_t pin, uint32_t period_usec, uint32_t pulse_usec)
pwm_set_pin_cycles(uint8_t pin, uint32_t period_cycles, uint32_t pulse_cycles)

These two APIs are equivalent. User only needs to use one of them based on
the preference for unit.

Final API Format
--------

pwm_set_pin_usec(struct device *dev, uint8_t pin, uint32_t period_usec,
uint32_t pulse_usec)
pwm_set_pin_cycles(struct device *dev, uint8_t pin, uint32_t period_cycles,
uint32_t pulse_cycles)

Suggestions and comments are welcome.


Re: [RFC] SPI API update

Liu, Baohong
 

-----Original Message-----
From: Tomasz Bursztyka [mailto:tomasz.bursztyka(a)linux.intel.com]
Sent: Wednesday, September 14, 2016 12:31 AM
To: Liu, Baohong <baohong.liu(a)intel.com>; devel(a)lists.zephyrproject.org
Subject: Re: [devel] [RFC] SPI API update

Hi Baohong,

inline int spi_transceive(struct spi_config *conf, uint32_t slave,
struct spi_buffer *tx, uint32_t nb_tx_buf,
struct spi_buffer *rx, uint32_t
nb_rx_buf);

GPIO is and will be used for virtually any cases since the maximum
duration of the CS from the controller is so short. So, slave select
is just giving a non-zero value to make the controller happy and the
GPIO pin is selecting the slave device. But, in the current code, only
one GPIO pin is defined for each controller by Kconfig. In other words, one
controller is only able to control one slave. This needs to be addressed.

This GPIO used as CS is a hack for - currently - DW controllers. When hw
integration makes it available (it's possible on QuarkSE X86 core, not on
QuarkSE ARC core for instance).

So indeed, it's very much limited for now, but I don't see any reason to
expose anything about it in the public API. Hack should stay hidden, deep, in
driver's specific code.
User or app writer needs to be aware of this. User needs the info to connect
the external or on-board devices. This will be part of user-configurable setting.
It can't be hidden.


For dw, we could create some array of gpio pins: index would be the slave. It
could be provided by the relevant board.h file. We could provide "generic"
spi_config_cs()/spi_control_cs() but only for the drivers, like in some
drivers/spi/spi_utils.h file. Just a quick idea.

Tomasz


A possible case of TCP server no-response, was: Re: bufs lost in TCP connection establishment

Paul Sokolovsky
 

Hello Rohit, et al,

Thanks for the information below. I didn't yet have chance to test
multiple TCP/IPv4 connections in row, because I have issue with
reliably handling one. Just as before, my setup is a scripting language
(MicroPython), using combination of Zephyr and uIP APIs to emulate BSD
sockets. I however proceeded to play with mbedTLS on top of sockets
implemented in such way. That means multiple bidirectional packet
exchanges which exposes quite a number of edge conditions. There're
packets can be dropped anytime.

One particularly naughty conditions I experienced is that while my
usual testcase usually proceeded beyond connect() call, sometimes I got
time "strips" when connect() call just hanged (I don't use timeouts,
like BSD sockets by default) - with consecutive dozen or so of QEMU
restarts (I'm using QEMU virtual networking still so far). Then it
suddenly started working again, to repeat soon again. Looking with
Wireshark, Zephyr app just was sending SYN packets, with zero reply
from a Linux side. At this time, it's easy to start wonder what gets
wrong - Linux handling of TUN interface, tunslip's setting it up, socat
which is involved in connecting tunslip and QEMU, Wireshark, or
something else.

I set to debug just that weird case, with initial hypothesis that Linux
SYN flood protection may be involved. With some googling and netstat
grepping, I found that when I get those can't-connect time strips,
there's actually ESTABLISHED connection on the host. That led me to
http://stackoverflow.com/questions/6825036/what-will-happen-if-i-send-a-syn-packet-to-the-server-when-there-has-already-bee ,
which finished the picture.

So, uIP/Zephyr don't use source port randomization, and always open
connection from port 1025. While code snippets I ran included closing
a socket, if something went wrong, like a packet was lost during
initial connection or TLS exchange, the connection wasn't closed
explicitly, but left open on server. On QEMU restart, it started to
send SYN to establish new connection from the same source port, which,
as explained in the link above, gets zero response from a Linux host.
Eventually Apache on host side times out that client connection, then
the situation recovers.

The solution is thus to check whether there's ESTABLISHED connection on
host side with netstat and restart a server process which owns it
(apache in my case).

The same situation may surely happen with a real device too, so I hope
this mail may save debugging time to some readers.


Thanks,
Paul


On Wed, 7 Sep 2016 15:26:23 +0000
Rohit Grover <Rohit.Grover(a)arm.com> wrote:

Jukka, Paul,

I find that uIP leaks one TX buf with every TCP connection due to
incorrectly managed ref-counts. I'm able to setup and teardown the
same number of connections as the value of CONFIG_IP_BUF_TX_SIZE.

The initial buf for the SYN packet gets its ref-count bumped to 2 (by
tcpip_poll_tcp()), but then this count never goes down to 0. It seems
to me that when the tcpip_event is posted to process_thread_tcp()
upon the sending of the SYN buf, the following code fragment

if (buf && uip_connected(buf)) {
struct net_context *context = user_data;
NET_DBG("Connection established context %p\n",
user_data);
context->connection_status = -EALREADY;
data = INT_TO_POINTER(TCP_WRITE_EVENT);
goto try_send;
}

is able is able to discover the transition to connected state, and
cycles back to call handle_tcp_connection(); but the ref-count of SYN
buf isn't decremented.

Can you help?

Thanks,
Rohit.

IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient, please notify the sender immediately and do not disclose
the contents to any other person, use it for any purpose, or store or
copy the information in any medium. Thank you.


--
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog


Daily JIRA Digest

donotreply@...
 

NEW JIRA items within last 24 hours: 3
[ZEP-865] convert filesystem sample to a runnable test
https://jira.zephyrproject.org/browse/ZEP-865

[ZEP-869] TCP connection fails if communication slows down.
https://jira.zephyrproject.org/browse/ZEP-869

[ZEP-868] TCP Connections are broken on Galileo
https://jira.zephyrproject.org/browse/ZEP-868


UPDATED JIRA items within last 24 hours: 10
[ZEP-540] add APIs for asynchronous transfer callbacks
https://jira.zephyrproject.org/browse/ZEP-540

[ZEP-750] Arduino 101 board should support one configuration using original bootloader
https://jira.zephyrproject.org/browse/ZEP-750

[ZEP-817] Neighbor Discovery Optimization for IPv6 over 6LowPAN
https://jira.zephyrproject.org/browse/ZEP-817

[ZEP-245] Restructure Documentation content
https://jira.zephyrproject.org/browse/ZEP-245

[ZEP-852] SPI API Update
https://jira.zephyrproject.org/browse/ZEP-852

[ZEP-737] Update host tools from upstream: fixdep.c
https://jira.zephyrproject.org/browse/ZEP-737

[ZEP-240] printk/printf usage in samples
https://jira.zephyrproject.org/browse/ZEP-240

[ZEP-584] warn user if SDK is out of date
https://jira.zephyrproject.org/browse/ZEP-584

[ZEP-748] Enable mbedtls_sslclient sample to run on quark se board
https://jira.zephyrproject.org/browse/ZEP-748

[ZEP-637] Building and linking static libraries
https://jira.zephyrproject.org/browse/ZEP-637


CLOSED JIRA items within last 24 hours: 3
[ZEP-863] (Cannot Reproduce) minimal libc 'size_t' conflicting with ISSM toolchain
https://jira.zephyrproject.org/browse/ZEP-863

[ZEP-756] (Duplicate) Remove Sandboxes from documentation
https://jira.zephyrproject.org/browse/ZEP-756

[ZEP-763] (Fixed) Samples:README: samples/drivers/I2c_stts751 describes error
https://jira.zephyrproject.org/browse/ZEP-763


RESOLVED JIRA items within last 24 hours: 1
[ZEP-640] (Fixed) Remove dynamic IRQs/exceptions from Zephyr
https://jira.zephyrproject.org/browse/ZEP-640


Re: [RFC] SPI API update

Jon Medhurst (Tixy) <tixy@...>
 

On Wed, 2016-09-14 at 16:29 +0200, Tomasz Bursztyka wrote:
Hi Jon,

On Wed, 2016-09-14 at 13:42 +0200, Tomasz Bursztyka wrote:
Tixy wrote
With my proposal SPI_ENABLE_CS_BEFORE would acquire the lock and
SPI_DISABLE_CS_AFTER release it. All that's different is that one
function call is replaced by N Function calls.
For this to work, you would need to relate the lock to the caller, to
block any other caller
to mess up. It's possible (fiber/task id), but a bit more bloated than
doing all at once in one function.
I was assuming 'lock' == mutex/semaphore and would be the same method
you would use in a single function. E.g.

Single function...
lock
do some work
unlock

Separate function1
lock
do some work
Separate function2
do some work
Separate function3
do some work
unlock

The SPI bus is a shared resource, and users claim use of if for using
it.
As I said your example does not work unless you keep track of who owns
the lock.
What about your lock that you'll use in the RFC's proposed of
spi_tranceive? I think I've must have misunderstood something
fundamental, because I see so difference between

A();
B();
C();

and

void F()
{
A();
B();
C();
}

F();

and the compiler would also agree.

[...]

You _cannot_ know in your code, if another part of the system has
touched the device configuration.
So the code always need to make sure the dev is running the right
configuration.

Either we always ask the user to run spi_configure(), as ugly as it is,
before each spi_ io calls.
Or we ask him to provide a statically allocated configuration structure,
with a content not meant
to change, and give it for each spi_ io calls.

Btw, if you run such code in a task, you can be pre-empted after your
spi_configure(),
then another task/fiber kicks in and also runs spi_configure(): when the
kernel is
back at your task/fiber, you are not anymore running on your
configuration...
And your alternative of a single function call that checks a config arg
and updates the config before then transferring data is different? What
if it gets preempted between updating the config in hardware and
transferring the data? It's my same confusion as to why it should be
that "A() B() C()" != F()"

I'm not trying to press my API suggestions on people, I'll code to
whatever Zephyr specifies. I was carrying on these emails to try and
clear up my own misunderstanding of this as much as anything. I'll stop
now, and review any code that is forthcoming. It's a lot easier to talk
about real code and say things like 'what if we are prempted at this
line here...' to which the answer may well be 'we can't be preempted
here because of X'.

--
Tixy


Daily Gerrit Digest

donotreply@...
 

NEW within last 24 hours:
- https://gerrit.zephyrproject.org/r/4773 : Bluetooth: Controller: Make HCI endianness-independent
- https://gerrit.zephyrproject.org/r/4774 : intel_quark: move X86_IAMCU to defconfig
- https://gerrit.zephyrproject.org/r/4757 : sanity: test_context build only for qemu arm
- https://gerrit.zephyrproject.org/r/4782 : samples: zoap server: exclude quark d2000 not enough ram
- https://gerrit.zephyrproject.org/r/4780 : unified: Fix semaphore group tests
- https://gerrit.zephyrproject.org/r/4781 : unified: Enable semaphore group use in test_mail
- https://gerrit.zephyrproject.org/r/4779 : unified: Add support for semaphore groups
- https://gerrit.zephyrproject.org/r/4778 : unified: Add SEMAPHORE_GROUPS Kconfig option
- https://gerrit.zephyrproject.org/r/4777 : unified: Include _timeout structure in tcs_base
- https://gerrit.zephyrproject.org/r/4776 : unified: Conditionally define __printf_like() macro
- https://gerrit.zephyrproject.org/r/4770 : tinycrypt: Add test case for the CTR PRNG algorithm
- https://gerrit.zephyrproject.org/r/4771 : net: tests: Remove broadcast and multicast tests from ARP
- https://gerrit.zephyrproject.org/r/4768 : known issues: update SKIP regex
- https://gerrit.zephyrproject.org/r/4755 : sdk: zephyr: check for minimum required version of SDK

UPDATED within last 24 hours:
- https://gerrit.zephyrproject.org/r/4687 : arduino 101: make factory bootloader config the default
- https://gerrit.zephyrproject.org/r/4671 : soc: intel_quark: move quark d2000 to intel_quark family
- https://gerrit.zephyrproject.org/r/4670 : intel_quark: Group Quark SoCs under intel_quark/
- https://gerrit.zephyrproject.org/r/4684 : quark_x1000: move the X1000 into the intel_quark family
- https://gerrit.zephyrproject.org/r/4422 : boards: rename Quark SE Devboard to Quark SE C1000
- https://gerrit.zephyrproject.org/r/4420 : boards: rename Quark SE Devboard to Quark SE C1000 (Sensor Subsystem)
- https://gerrit.zephyrproject.org/r/4423 : MAINTAINERS: add maintainer for some of the boards
- https://gerrit.zephyrproject.org/r/4689 : parse board defconfig at the very end
- https://gerrit.zephyrproject.org/r/4688 : boards: rename arduino_101_sss to arduino_101_ss
- https://gerrit.zephyrproject.org/r/4715 : net: yaip: Add DHCPv4 client support
- https://gerrit.zephyrproject.org/r/4623 : eth: Adjust ENC28J60 transmission/reception return codes.
- https://gerrit.zephyrproject.org/r/4717 : net: apps: Add DHCPv4 client sample application
- https://gerrit.zephyrproject.org/r/4716 : net: tests: Add DHCPv4 client unit tests
- https://gerrit.zephyrproject.org/r/4585 : samples: use printf/printk directly instead of macros
- https://gerrit.zephyrproject.org/r/4551 : tests: move test code from samples to tests
- https://gerrit.zephyrproject.org/r/3922 : lib: Add HTTP support for Zephyr
- https://gerrit.zephyrproject.org/r/3924 : lib/http: Add test-case for HTTP header fields
- https://gerrit.zephyrproject.org/r/4711 : net: tests: Add include dir only when specific options enabled
- https://gerrit.zephyrproject.org/r/4713 : net: yaip: Fix net address state
- https://gerrit.zephyrproject.org/r/4745 : tinycrypt: Rename current tests to avoid confusions with new algorithms
- https://gerrit.zephyrproject.org/r/4747 : tinycrypt: Solve style issues
- https://gerrit.zephyrproject.org/r/4555 : Bluetooth: HFP HF: SLC connection-Send/Parse BRSF
- https://gerrit.zephyrproject.org/r/4489 : Bluetooth: SDP: Server: Support ServiceAttrReq and ServiceSearchAttrReq
- https://gerrit.zephyrproject.org/r/4488 : Bluetooth: SDP: Server: Support ServiceSearchRequest
- https://gerrit.zephyrproject.org/r/4486 : Bluetooth: SDP: Server: Initialize and accept incoming connections
- https://gerrit.zephyrproject.org/r/4487 : Bluetooth: SDP: Server: Support service record registration
- https://gerrit.zephyrproject.org/r/4708 : net: yaip: Fix arp/ethernet broadcast and multcast addr scenario
- https://gerrit.zephyrproject.org/r/4707 : net: yaip: Fix slip multipackets reception
- https://gerrit.zephyrproject.org/r/4452 : Bluetooth: A2DP: Initialization of A2DP.
- https://gerrit.zephyrproject.org/r/4447 : Bluetooth: AVDTP: Module Initialization
- https://gerrit.zephyrproject.org/r/4540 : Bluetooth: AVDTP: Connect and Disconnect
- https://gerrit.zephyrproject.org/r/4710 : net: yaip: Fix IPv4 packet reception
- https://gerrit.zephyrproject.org/r/4562 : Bluetooth: Sample: handsfree sample application
- https://gerrit.zephyrproject.org/r/4321 : Bluetooth: L2CAP: Cleanup hanging channel when initiator
- https://gerrit.zephyrproject.org/r/4750 : Bluetooth: HCI: Fix updating RPA too early
- https://gerrit.zephyrproject.org/r/4749 : nano_work: Add nano_work_pending
- https://gerrit.zephyrproject.org/r/4699 : nano_work: Add nano_delayed_work_pending
- https://gerrit.zephyrproject.org/r/4600 : samples: heartrate-monitor: Fixes for following issues.
- https://gerrit.zephyrproject.org/r/4625 : eth: ENC28J60 sample application
- https://gerrit.zephyrproject.org/r/4683 : ARM: irq: Add _arch_irq_is_priority_equal external interrupt API
- https://gerrit.zephyrproject.org/r/4676 : irq: Add irq_enable_keep external interrupt API
- https://gerrit.zephyrproject.org/r/3895 : tests/kernel/test_multilib: Test for proper multilib selection.
- https://gerrit.zephyrproject.org/r/4704 : power_mgmt: Update sample and drivers according to new pm device API
- https://gerrit.zephyrproject.org/r/4463 : power_mgmt: Update Power Management device driver API
- https://gerrit.zephyrproject.org/r/4552 : build: support pre-built host tools (DO NOT MERGE)
- https://gerrit.zephyrproject.org/r/3363 : Commit: validate accent in name.
- https://gerrit.zephyrproject.org/r/4622 : eth: Add full-duplex configuration to ENC28J60
- https://gerrit.zephyrproject.org/r/4624 : eth: Add ENC28J60 support into Paho's MQTT publisher example
- https://gerrit.zephyrproject.org/r/4511 : unified/doc: Kernel primer for unified kernel
- https://gerrit.zephyrproject.org/r/4621 : eth: Initial release to tx semaphore for the ENC28J60 driver.
- https://gerrit.zephyrproject.org/r/4620 : eth: Adjust ENC28J60 MAC configuration.
- https://gerrit.zephyrproject.org/r/4681 : ARM: irq: Add _arch_irq_pending_clear external interrupt API
- https://gerrit.zephyrproject.org/r/4680 : irq: Add irq_pending_clear external interrupt API
- https://gerrit.zephyrproject.org/r/4679 : ARM: irq: Add _arch_irq_is_enabled external interrupt API
- https://gerrit.zephyrproject.org/r/4678 : irq: Add irq_is_enabled external interrupt API
- https://gerrit.zephyrproject.org/r/4662 : irq: Add irq_pending_set external interrupt API
- https://gerrit.zephyrproject.org/r/4635 : serial: make nrf5 driver more generic, prepare for nrf51

MERGED within last 24 hours:
- https://gerrit.zephyrproject.org/r/4775 : gpio: stm32: Fix bug introduced by removing API 1.0 support
- https://gerrit.zephyrproject.org/r/4756 : known-issues: fix regex to catch summary messages to ignore
- https://gerrit.zephyrproject.org/r/4769 : samples/task_profile: fix testcase.ini's long lines
- https://gerrit.zephyrproject.org/r/4772 : Bluetooth: SMP: Factor out BR/EDR encryption check to helper
- https://gerrit.zephyrproject.org/r/4754 : Bluetooth: SMP: Fix encryption key size check in BR/EDR pairing req
- https://gerrit.zephyrproject.org/r/4753 : Bluetooth: Add support for reading encryption key size for BR/EDR
- https://gerrit.zephyrproject.org/r/4360 : ksdk: Add KSDK RNGA driver.
- https://gerrit.zephyrproject.org/r/4697 : gpio: Remove obsolete API 1.0 callback mechanism
- https://gerrit.zephyrproject.org/r/4729 : Bluetooth: SMP: Add helper for reporting BR/EDR pairing complete
- https://gerrit.zephyrproject.org/r/4728 : Bluetooth: SMP: Add support for Signing Information over BR/EDR
- https://gerrit.zephyrproject.org/r/4727 : Bluetooth: SMP: Add support for Identity Information over BR/EDR
- https://gerrit.zephyrproject.org/r/4730 : Bluetooth: SMP: Add support for sending Pairing Request over BR/EDR
- https://gerrit.zephyrproject.org/r/4726 : Bluetooth: SMP: Add support for LTK derivation from LinkKey
- https://gerrit.zephyrproject.org/r/4725 : Bluetooth: SMP: Allow to force BR/EDR without SC support
- https://gerrit.zephyrproject.org/r/4724 : Bluetooth: SMP: Support Pairing Response over BR/EDR
- https://gerrit.zephyrproject.org/r/4752 : Bluetooth: Controller: Implement LE_RAND command
- https://gerrit.zephyrproject.org/r/4516 : workqueue: use kernel.h for workqueue header file
- https://gerrit.zephyrproject.org/r/4674 : x86: load _nanokernel in %edi in _Swap()
- https://gerrit.zephyrproject.org/r/4515 : atomic: fix bug in ATOMIC_INIT()
- https://gerrit.zephyrproject.org/r/4521 : zperf_shell: add unified kernel string for unified kernel case
- https://gerrit.zephyrproject.org/r/4530 : unified: initial unified kernel implementation
- https://gerrit.zephyrproject.org/r/4513 : unified/arm: add unified kernel support for ARM arch
- https://gerrit.zephyrproject.org/r/4508 : build: only generate the SSE group for x86
- https://gerrit.zephyrproject.org/r/4525 : unified/test_pipe: adapt to not use sem groups
- https://gerrit.zephyrproject.org/r/4510 : arm: only compile gdb stubs when CONFIG_GDB_INFO=y
- https://gerrit.zephyrproject.org/r/4528 : unified/test_sema: fix isr wrapper names
- https://gerrit.zephyrproject.org/r/4526 : unified/test_timer: adapt for unified kernel
- https://gerrit.zephyrproject.org/r/4514 : unified/x86: add unified kernel support for x86 arch
- https://gerrit.zephyrproject.org/r/4506 : build: make sysgen take optional command line arguments
- https://gerrit.zephyrproject.org/r/4675 : unified/test_mutex: adapt to run on unified kernel
- https://gerrit.zephyrproject.org/r/4522 : unified/tests: tag working some tests kernel as 'unified_capable'
- https://gerrit.zephyrproject.org/r/4529 : unified/test_fp: mark test so that it runs the nanokernel version
- https://gerrit.zephyrproject.org/r/4527 : unified: Fix test_sema/microkernel
- https://gerrit.zephyrproject.org/r/4571 : checkpatch: do not check for min_t/max_t
- https://gerrit.zephyrproject.org/r/4503 : slist: add sys_slist_get() to fetch and remove the head
- https://gerrit.zephyrproject.org/r/4509 : arm: add __ASSERT() for stack alignment
- https://gerrit.zephyrproject.org/r/4523 : unified/test_context: adapt test to run on unified kernel
- https://gerrit.zephyrproject.org/r/4531 : unified/build: allow building the unified kernel
- https://gerrit.zephyrproject.org/r/4505 : kernel: add CONFIG_MDEF
- https://gerrit.zephyrproject.org/r/4512 : unified/build: adapt Kbuild for unified kernel
- https://gerrit.zephyrproject.org/r/4519 : unified/sys_timer: guard microkernel announce with !KERNEL_V2
- https://gerrit.zephyrproject.org/r/4504 : slist: add sys_slist_append_list and sys_slist_merge_slist()
- https://gerrit.zephyrproject.org/r/4507 : sysgen: add --kernel_type argument
- https://gerrit.zephyrproject.org/r/4524 : unified/test_mail: adapt test to not use sem groups and mem pools
- https://gerrit.zephyrproject.org/r/4518 : unified/drivers: adapt timer drivers to unified kernel
- https://gerrit.zephyrproject.org/r/4502 : dlist: add static initialization macro
- https://gerrit.zephyrproject.org/r/4520 : unified/object_tracing: disable object tracing in unified kernel
- https://gerrit.zephyrproject.org/r/4570 : checkpatch: add --ignore DATE_TIME
- https://gerrit.zephyrproject.org/r/4517 : unified: include kernel.h via major top-level header files
- https://gerrit.zephyrproject.org/r/4501 : dlist: add SYS_DLIST_FOR_EACH_NODE/_SAFE
- https://gerrit.zephyrproject.org/r/4741 : doc: workaround for __deprecated functions
- https://gerrit.zephyrproject.org/r/4593 : Clean dev and latest docs sites before deploy
- https://gerrit.zephyrproject.org/r/4659 : task profiler: Adds the task profiler samples to the sanity check
- https://gerrit.zephyrproject.org/r/4657 : task profiler: project configuration files clean up
- https://gerrit.zephyrproject.org/r/4658 : task profiler: README file update
- https://gerrit.zephyrproject.org/r/4751 : Bluetooth: Controller: Clean up HCI macros
- https://gerrit.zephyrproject.org/r/4748 : Bluetooth: Controller: Use hci.h for ACL data
- https://gerrit.zephyrproject.org/r/4746 : Bluetooth: Fix giving back pkts semaphore when disconnecting
- https://gerrit.zephyrproject.org/r/4744 : libc: replace null.h and size_t.h with stddef.h


Re: [RFC] SPI API update

Tomasz Bursztyka
 

Hi Jon,

On Wed, 2016-09-14 at 13:42 +0200, Tomasz Bursztyka wrote:
Tixy wrote
With my proposal SPI_ENABLE_CS_BEFORE would acquire the lock and
SPI_DISABLE_CS_AFTER release it. All that's different is that one
function call is replaced by N Function calls.
For this to work, you would need to relate the lock to the caller, to
block any other caller
to mess up. It's possible (fiber/task id), but a bit more bloated than
doing all at once in one function.
I was assuming 'lock' == mutex/semaphore and would be the same method
you would use in a single function. E.g.

Single function...
lock
do some work
unlock

Separate function1
lock
do some work
Separate function2
do some work
Separate function3
do some work
unlock

The SPI bus is a shared resource, and users claim use of if for using
it.
As I said your example does not work unless you keep track of who owns
the lock.

And anyway, it looks ugly to handle low level SPI concept (CS) in user API.

If we wanted, we could also crowbar the slave select value into those
flags. Then if we created a config function that operated per slave:

spi_configure(dev, config, slave_id);

and that saved the config in an array somewhere, then the driver or
framework could reconfigure the hardware if the slave changes.
Same problem with the existing API: you will need to call
spi_configure() before
every spi transaction call, because you won't know if another part of
the system did access
or not the same device, and changed the config/slave etc...
The SPI driver can remember the last-slave-used and check if it's
changed for the new transaction and if so, do a re-configure. That's
what you proposed doing and would work here too.

And if the user called spi_configure to change the config used for a
slave, and that was the current last-slave-used then the last-slave-used
value could be invalidated to force a re-configure before next
transaction.

All that would go wrong is if two parts of the system were tring to
access the same slave with different configs. But in that case, they are
likely to be interfering with each other in more ways that just changing
the config. The slave device itself is likely to have state that would
get trashed as well.
The driver will remember the config and the slave anyway (in same
struct, or separate).

I really don't see why we should keep spi_configure() around, that's all.
It lets the driver know the config has changed.

If you pass a pointer to a struct containing the config, then do you
assume same pointer == same config or copy the config data and do a
memcmp to see if it was changed between calls? If you just compare
pointers, then anyone wanting to change config has to use another struct
at a different address, and know this implementation detail of the API.
We'd also get some weird bugs if anyone used a config struct allocated
on the stack, where the address may or may not match the current stashed
pointer depending on program flow and compilation options. So a config
may or may not actually be used.
You _cannot_ know in your code, if another part of the system has
touched the device configuration.
So the code always need to make sure the dev is running the right
configuration.

Either we always ask the user to run spi_configure(), as ugly as it is,
before each spi_ io calls.
Or we ask him to provide a statically allocated configuration structure,
with a content not meant
to change, and give it for each spi_ io calls.

Btw, if you run such code in a task, you can be pre-empted after your
spi_configure(),
then another task/fiber kicks in and also runs spi_configure(): when the
kernel is
back at your task/fiber, you are not anymore running on your
configuration...

Second option is much simpler and safer, spi_config can be seen as a
"handle" in a way.
We can document things to make sure the user does not mess up too much
(not allocation of the struct on stack, etc...). It's not super perfect,
but we don't have the luxury
of being able to use as much memory we want to, in order to properly
keep track of config/owners
etc and transparently switch to one or the other when relevant.

And no, it has nothing to do with personal preference.

Tomasz


Re: [RFC] SPI API update

Jon Medhurst (Tixy) <tixy@...>
 

On Wed, 2016-09-14 at 13:42 +0200, Tomasz Bursztyka wrote:
Tixy wrote
With my proposal SPI_ENABLE_CS_BEFORE would acquire the lock and
SPI_DISABLE_CS_AFTER release it. All that's different is that one
function call is replaced by N Function calls.
For this to work, you would need to relate the lock to the caller, to
block any other caller
to mess up. It's possible (fiber/task id), but a bit more bloated than
doing all at once in one function.
I was assuming 'lock' == mutex/semaphore and would be the same method
you would use in a single function. E.g.

Single function...
lock
do some work
unlock

Separate function1
lock
do some work
Separate function2
do some work
Separate function3
do some work
unlock

The SPI bus is a shared resource, and users claim use of if for using
it.

And it still lets the possibility for a code to introduce a bug by never
releasing the lock anyway.
There's endless ways for code to be buggy.

If we wanted, we could also crowbar the slave select value into those
flags. Then if we created a config function that operated per slave:

spi_configure(dev, config, slave_id);

and that saved the config in an array somewhere, then the driver or
framework could reconfigure the hardware if the slave changes.
Same problem with the existing API: you will need to call
spi_configure() before
every spi transaction call, because you won't know if another part of
the system did access
or not the same device, and changed the config/slave etc...
The SPI driver can remember the last-slave-used and check if it's
changed for the new transaction and if so, do a re-configure. That's
what you proposed doing and would work here too.

And if the user called spi_configure to change the config used for a
slave, and that was the current last-slave-used then the last-slave-used
value could be invalidated to force a re-configure before next
transaction.

All that would go wrong is if two parts of the system were tring to
access the same slave with different configs. But in that case, they are
likely to be interfering with each other in more ways that just changing
the config. The slave device itself is likely to have state that would
get trashed as well.
The driver will remember the config and the slave anyway (in same
struct, or separate).

I really don't see why we should keep spi_configure() around, that's all.
It lets the driver know the config has changed.

If you pass a pointer to a struct containing the config, then do you
assume same pointer == same config or copy the config data and do a
memcmp to see if it was changed between calls? If you just compare
pointers, then anyone wanting to change config has to use another struct
at a different address, and know this implementation detail of the API.
We'd also get some weird bugs if anyone used a config struct allocated
on the stack, where the address may or may not match the current stashed
pointer depending on program flow and compilation options. So a config
may or may not actually be used.

I guess it boils down to a personal preference for functions with simple
explicit actions that you can use as building bricks to achieve the end
you want, rather than more monolithic functions that mandate a
particular way of working, even if it doesn't fit your use case.

Perhaps I should create a Jira to rewrite Zephyr in Forth :-)

--
Tixy


Re: [RFC] SPI API update

Tomasz Bursztyka
 

Hi Jon,
On Wed, 2016-09-14 at 11:36 +0200, Tomasz Bursztyka wrote:
Hi Jon,

1. What about DMA integration? I haven't spotted much in the way of DMA
support in driver APIs and the DMA API itself doesn't support scatter
gather so that's looking like a whole area that's yet to be sorted out?
DMA is not meant to support such scatter-gather buffer, but it would be
easy to wrap dma primitives to do so.

(However, I don't know why it separates transfer cb and error cb, these
could be the same,
it would just require a status code in cb parameter. That's off subject
however).

That said, I don't see any requirement for exposing anything about DMA
in public SPI API.
I'm was just wondering how to tell a generic SPI driver, which DMA to
use. Or how it requests one itself. Or if these things we're going to
have to be hard coded at build time in a device specific manner, which
would negate writing a generic driver for some given IP.
I guess we would have to think about something driver generic,
configured through board.h also.
There isn't much other way. Or it can be done through Kconfig as well,
though that could end bloated.

But I haven't really looked into DMA properly yet, so I'm all fuzzy on
the possibilities and requirements :-)
It's a good thing you raised this DMA issue, because DMA API itself
would need some work it seems.

I am bit skeptical on some part, like the handshake_interface, this
really looks like something qmsi centric,
and that's a bit bad because Zephyr device driver APIs should not be
tight to any HAL, it should be the contrary: HAL
should do what's necessary to fit into Zephyr's API. Unless I
misunderstood the point of that attribute.

Anyway, I'll probably send another mail about DMA later.

Tomasz


Re: [RFC] SPI API update

Jon Medhurst (Tixy) <tixy@...>
 

On Wed, 2016-09-14 at 13:12 +0200, Tomasz Bursztyka wrote:
I call it a hack in DW driver, because this controller has a CS...
which
is broken.

So, rather than upstreaming this driver along with other board
support, which is what my brief is, I'll just park this work in a
fork
somewhere until Zephyr's APIs settle down to something more
usable. :-(

Legacy or new API: your driver will have to deal with CS internally,
either controlling the controller's capability or
a GPIO line. That has nothing to do with zero-copy btw.
Except as I want zero copy now, with current APIs, I have to handle CS
manually, outside the SPI driver; that's what I meant. I didn't want to
write code to handle GPIO chip select in the driver which I would then
have to '#if 0' out again :-)

--
Tixy


Re: [RFC] SPI API update

Tomasz Bursztyka
 

Hi Jon,

Orthogonal to my comment above. How about separating out config into a
separate 'begin' function call. If that also enables chip select, then
we can build arbitrary complex transactions (with zero-copy) using the
current API's

spi_begin(dev, conf, slave) /* Enables chip select for slave */
...
spi_write(dev, ...)
spi_tranceive(dev, ...)
spi_read(dev, ...)
...
spi_end(dev) /* Disables chip select */
See my comment below, this idea is affected by the same flaw.

An alternative to that for zero copy support would be to add a 'flags'
parameter to spi_{read,write,transceive} with 2 flags bits:

SPI_ENABLE_CS_BEFORE
SPI_DISABLE_CS_AFTER

That would gain the arbitrary complex and zero copy transaction support,
without forcing a begin/end call. E.g. I could send my bitmap to the LCD
controller without copying it by

spi_write(dev, &id_byte, sizeof(id_byte), SPI_ENABLE_CS_BEFORE);
spi_write(dev, &bitmap, sizeof(bitmap), SPI_DISABLE_CS_AFTER);

And the current implementation of spi_write would be equivalent to

spi_write(dev, data, size, SPI_ENABLE_CS_BEFORE|SPI_DISABLE_CS_AFTER)
This would open quite a nasty can of worms for concurrent access.

One fiber/task won't know the CS status, and might ask to disable it,
while the original
fiber/task enabling CS will still think it's enabled when it's not, etc...
I don't see that concurrent access is any more of a problem with this
than without it. What if a task calls the single transceive() function
you propose, then that task gets preempted and another one calls
tranceive() for a different slave? The current implementation don't have
any locking to prevent things going badly wrong.

With my proposal SPI_ENABLE_CS_BEFORE would acquire the lock and
SPI_DISABLE_CS_AFTER release it. All that's different is that one
function call is replaced by N Function calls.
For this to work, you would need to relate the lock to the caller, to
block any other caller
to mess up. It's possible (fiber/task id), but a bit more bloated than
doing all at once in one function.

And it still lets the possibility for a code to introduce a bug by never
releasing the lock anyway.

If we wanted, we could also crowbar the slave select value into those
flags. Then if we created a config function that operated per slave:

spi_configure(dev, config, slave_id);

and that saved the config in an array somewhere, then the driver or
framework could reconfigure the hardware if the slave changes.
Same problem with the existing API: you will need to call
spi_configure() before
every spi transaction call, because you won't know if another part of
the system did access
or not the same device, and changed the config/slave etc...
The SPI driver can remember the last-slave-used and check if it's
changed for the new transaction and if so, do a re-configure. That's
what you proposed doing and would work here too.

And if the user called spi_configure to change the config used for a
slave, and that was the current last-slave-used then the last-slave-used
value could be invalidated to force a re-configure before next
transaction.

All that would go wrong is if two parts of the system were tring to
access the same slave with different configs. But in that case, they are
likely to be interfering with each other in more ways that just changing
the config. The slave device itself is likely to have state that would
get trashed as well.
The driver will remember the config and the slave anyway (in same
struct, or separate).

I really don't see why we should keep spi_configure() around, that's all.

Tomasz


Re: [RFC] SPI API update

Jon Medhurst (Tixy) <tixy@...>
 

On Wed, 2016-09-14 at 11:36 +0200, Tomasz Bursztyka wrote:
Hi Jon,

1. What about DMA integration? I haven't spotted much in the way of DMA
support in driver APIs and the DMA API itself doesn't support scatter
gather so that's looking like a whole area that's yet to be sorted out?
DMA is not meant to support such scatter-gather buffer, but it would be
easy to wrap dma primitives to do so.

(However, I don't know why it separates transfer cb and error cb, these
could be the same,
it would just require a status code in cb parameter. That's off subject
however).

That said, I don't see any requirement for exposing anything about DMA
in public SPI API.
I'm was just wondering how to tell a generic SPI driver, which DMA to
use. Or how it requests one itself. Or if these things we're going to
have to be hard coded at build time in a device specific manner, which
would negate writing a generic driver for some given IP.

But I haven't really looked into DMA properly yet, so I'm all fuzzy on
the possibilities and requirements :-)

--
Tixy


Re: [RFC] SPI API update

Jon Medhurst (Tixy) <tixy@...>
 

On Wed, 2016-09-14 at 10:16 +0200, Tomasz Bursztyka wrote:
On 13/09/2016 17:18, Jon Medhurst (Tixy) wrote:
[...]

Orthogonal to my comment above. How about separating out config into a
separate 'begin' function call. If that also enables chip select, then
we can build arbitrary complex transactions (with zero-copy) using the
current API's

spi_begin(dev, conf, slave) /* Enables chip select for slave */
...
spi_write(dev, ...)
spi_tranceive(dev, ...)
spi_read(dev, ...)
...
spi_end(dev) /* Disables chip select */
See my comment below, this idea is affected by the same flaw.

An alternative to that for zero copy support would be to add a 'flags'
parameter to spi_{read,write,transceive} with 2 flags bits:

SPI_ENABLE_CS_BEFORE
SPI_DISABLE_CS_AFTER

That would gain the arbitrary complex and zero copy transaction support,
without forcing a begin/end call. E.g. I could send my bitmap to the LCD
controller without copying it by

spi_write(dev, &id_byte, sizeof(id_byte), SPI_ENABLE_CS_BEFORE);
spi_write(dev, &bitmap, sizeof(bitmap), SPI_DISABLE_CS_AFTER);

And the current implementation of spi_write would be equivalent to

spi_write(dev, data, size, SPI_ENABLE_CS_BEFORE|SPI_DISABLE_CS_AFTER)
This would open quite a nasty can of worms for concurrent access.

One fiber/task won't know the CS status, and might ask to disable it,
while the original
fiber/task enabling CS will still think it's enabled when it's not, etc...
I don't see that concurrent access is any more of a problem with this
than without it. What if a task calls the single transceive() function
you propose, then that task gets preempted and another one calls
tranceive() for a different slave? The current implementation don't have
any locking to prevent things going badly wrong.

With my proposal SPI_ENABLE_CS_BEFORE would acquire the lock and
SPI_DISABLE_CS_AFTER release it. All that's different is that one
function call is replaced by N Function calls.



If we wanted, we could also crowbar the slave select value into those
flags. Then if we created a config function that operated per slave:

spi_configure(dev, config, slave_id);

and that saved the config in an array somewhere, then the driver or
framework could reconfigure the hardware if the slave changes.
Same problem with the existing API: you will need to call
spi_configure() before
every spi transaction call, because you won't know if another part of
the system did access
or not the same device, and changed the config/slave etc...
The SPI driver can remember the last-slave-used and check if it's
changed for the new transaction and if so, do a re-configure. That's
what you proposed doing and would work here too.

And if the user called spi_configure to change the config used for a
slave, and that was the current last-slave-used then the last-slave-used
value could be invalidated to force a re-configure before next
transaction.

All that would go wrong is if two parts of the system were tring to
access the same slave with different configs. But in that case, they are
likely to be interfering with each other in more ways that just changing
the config. The slave device itself is likely to have state that would
get trashed as well.

--
Tixy

6581 - 6600 of 8046