On 09/03/2016, 16:13, "Maciek Borzecki" <maciek.borzecki(a)gmail.com> wrote:

On Wed, Mar 9, 2016 at 6:54 PM, Nashif, Anas <anas.nashif(a)intel.com> wrote:

On 9 Mar 2016, at 09:48, Maciek Borzecki <maciek.borzecki(a)gmail.com> wrote:

Hi,

I've pushed another set of updates. Hopefully I have not missed any
comments that were added to the previous versions.

I took the liberty of rebasing Daniel's patch on top of current
master, and so rebasing all of my patches on top of this one.

There are 2 major changes since the previous version.

One is extending of pinmux integration for STM32F1. That includes
structures that define alternate functions for IO pins plus some
helpers for selecting pin's function in a more convenient fashion.

The second change is adding UART_1 and UART_2 ports that map to USART2
and USART3 respectively.

Hopefully it'll be possible to merge at least some of these patches to
master.

Updated Changes:
https://gerrit.zephyrproject.org/r/645 st_stm32/stm32f1: introduce
STM32F1x SoC family

Maybe I was misunderstood in my email about the structure. I was suggesting to move the SoCs directly under soc/ and ski[ the st_stm32 level. If we are to do things the same way across architecture we would need to group all quarks under intel_quark.

Also, the st_ prefix is probably being used because we have ti_ and fsl_ for some of the existing SOCs. Maybe it is the right time now to drop the vendor part completely. (unrelated to this change, but we need to fix this with existing SOCs under arch/arm)

As more SOCs and boards are being added it is important to keep things consistent. The current structure allows adding a new SOC or board by just adding the files and structure and without changing any other files, we should keep it this way.

Summing up, what you're basically suggesting is having a structure
like this (assumig that we keep vendor prefix for the time being):

arch/
arm/
soc/
st_stm32f1/
Kconfig.soc
Kconfig
...
soc.c
st_stm32f2/
...
st_stm32l0/

If we're on the same page then i"ll post some patches tomorrow. Seems
like an easy fix.

Yes, the only different from what you have right now is having 1 level less. I think everything else should stay the same.

Anas

Cheers,
--
Maciek Borzecki

On Wed, Mar 9, 2016 at 6:54 PM, Nashif, Anas <anas.nashif(a)intel.com> wrote:

On 9 Mar 2016, at 09:48, Maciek Borzecki <maciek.borzecki(a)gmail.com> wrote:

Hi,

I've pushed another set of updates. Hopefully I have not missed any
comments that were added to the previous versions.

I took the liberty of rebasing Daniel's patch on top of current
master, and so rebasing all of my patches on top of this one.

There are 2 major changes since the previous version.

One is extending of pinmux integration for STM32F1. That includes
structures that define alternate functions for IO pins plus some
helpers for selecting pin's function in a more convenient fashion.

The second change is adding UART_1 and UART_2 ports that map to USART2
and USART3 respectively.

Hopefully it'll be possible to merge at least some of these patches to
master.

Updated Changes:
https://gerrit.zephyrproject.org/r/645 st_stm32/stm32f1: introduce
STM32F1x SoC family

Maybe I was misunderstood in my email about the structure. I was suggesting to move the SoCs directly under soc/ and ski[ the st_stm32 level. If we are to do things the same way across architecture we would need to group all quarks under intel_quark.

Also, the st_ prefix is probably being used because we have ti_ and fsl_ for some of the existing SOCs. Maybe it is the right time now to drop the vendor part completely. (unrelated to this change, but we need to fix this with existing SOCs under arch/arm)

As more SOCs and boards are being added it is important to keep things consistent. The current structure allows adding a new SOC or board by just adding the files and structure and without changing any other files, we should keep it this way.

Summing up, what you're basically suggesting is having a structure
like this (assumig that we keep vendor prefix for the time being):

arch/
arm/
soc/
st_stm32f1/
Kconfig.soc
Kconfig
...
soc.c
st_stm32f2/
...
st_stm32l0/

If we're on the same page then i"ll post some patches tomorrow. Seems
like an easy fix.

Cheers,
--
Maciek Borzecki

On Wed, 2016-03-09 at 14:35 -0500, Benjamin Walsh wrote:

Not that big of a change for a lot of people, but I pushed a revision to
documentation yesterday that enforces a 8-char tabs/80 column coding
style.

+1 from me

--
Andrew Boie
Staff Engineer - EOS Zephyr
Intel Open Source Technology Center

Folks,

Not that big of a change for a lot of people, but I pushed a revision to
documentation yesterday that enforces a 8-char tabs/80 column coding
style. Basically, this makes our coding style the same as for Linux with
the _only_ exception being that we require braces around single-line
code blocks:

ie.

if (blah) {
do_the_blah();
}

and not:

if (blah)
do_the_blah();

Same for for, while, do..while, etc.

The rationale behind that change is that the coding style we had was
kinda impossible to police w.r.t. tabs and columns, since it was
allowing tabs of 4 and 8, and columns of 80 and 100.

And the rationale for the choice of 8 vs 4 tabs is that we think
contributors will be used to that since we expect a non-negligible
amount of them to come from a Linux kernel background.

Cheers,
Ben

--
Benjamin Walsh, SMTS
Wind River Rocket
www.windriver.com
Zephyr kernel maintainer
www.zephyrproject.org

On 9 Mar 2016, at 09:48, Maciek Borzecki <maciek.borzecki(a)gmail.com> wrote:

Hi,

I've pushed another set of updates. Hopefully I have not missed any
comments that were added to the previous versions.

I took the liberty of rebasing Daniel's patch on top of current
master, and so rebasing all of my patches on top of this one.

There are 2 major changes since the previous version.

One is extending of pinmux integration for STM32F1. That includes
structures that define alternate functions for IO pins plus some
helpers for selecting pin's function in a more convenient fashion.

The second change is adding UART_1 and UART_2 ports that map to USART2
and USART3 respectively.

Hopefully it'll be possible to merge at least some of these patches to
master.

Updated Changes:
https://gerrit.zephyrproject.org/r/645 st_stm32/stm32f1: introduce
STM32F1x SoC family

Maybe I was misunderstood in my email about the structure. I was suggesting to move the SoCs directly under soc/ and ski[ the st_stm32 level. If we are to do things the same way across architecture we would need to group all quarks under intel_quark.

Also, the st_ prefix is probably being used because we have ti_ and fsl_ for some of the existing SOCs. Maybe it is the right time now to drop the vendor part completely. (unrelated to this change, but we need to fix this with existing SOCs under arch/arm)

As more SOCs and boards are being added it is important to keep things consistent. The current structure allows adding a new SOC or board by just adding the files and structure and without changing any other files, we should keep it this way.



Anas

https://gerrit.zephyrproject.org/r/647 clock_control/stm32f10x:
introduce new driver for STM32F10x RCC
https://gerrit.zephyrproject.org/r/648 soc/stm32f1: add GPIO
registers mapping
https://gerrit.zephyrproject.org/r/649 pinmux/stm32: add common
driver for STM32 pinmux
https://gerrit.zephyrproject.org/r/650 serial/stm32: add new driver
for STM32 UART
https://gerrit.zephyrproject.org/r/651 gpio/stm32: add common driver
for STM32 GPIO
https://gerrit.zephyrproject.org/r/652 boards/stm32_mini_a15: add
new board
https://gerrit.zephyrproject.org/r/653 samples/drivers/disco: add
'disco' sample program
https://gerrit.zephyrproject.org/r/698 arch: arm: move nmi to common
location
https://gerrit.zephyrproject.org/r/711 clock_control/Kconfig: move
quark_se entries to separate file
https://gerrit.zephyrproject.org/r/712 clock_control: extend API
with clock rate query operation
https://gerrit.zephyrproject.org/r/713 soc/stm32f1/gpio: implement
GPIO support
https://gerrit.zephyrproject.org/r/714 soc/stm32f1/pinmux: implement
STM32 pinmux integration
https://gerrit.zephyrproject.org/r/715 boards/nucleo_f103rb: add new
board

Cheers,

Hi Daniel,

Updated Changes:
https://gerrit.zephyrproject.org/r/645 st_stm32/stm32f1: introduce
STM32F1x SoC family

Depending upon others input, I think this is ready to go.

https://gerrit.zephyrproject.org/r/647 clock_control/stm32f10x:
introduce new driver for STM32F10x RCC

I'm still toying with the idea that this could be made more generic to work for all STM32 boards. Looking for general mailing list input on this.

Would it be better to accept what Maciek has right now that works on STM32F1 and then submit a follow up patch to make it generic for all STM32 MCUs? Or should we focus on getting a generic interface for all STM32 on the first pass?

Maybe we can move on with current patches yes.

Tomasz

Hey Maciek,

Hi,

I've pushed another set of updates. Hopefully I have not missed any
comments that were added to the previous versions.

I took the liberty of rebasing Daniel's patch on top of current
master, and so rebasing all of my patches on top of this one.

There are 2 major changes since the previous version.

One is extending of pinmux integration for STM32F1. That includes
structures that define alternate functions for IO pins plus some
helpers for selecting pin's function in a more convenient fashion.

The second change is adding UART_1 and UART_2 ports that map to USART2
and USART3 respectively.

Hopefully it'll be possible to merge at least some of these patches to
master.

Updated Changes:
https://gerrit.zephyrproject.org/r/645 st_stm32/stm32f1: introduce
STM32F1x SoC family
https://gerrit.zephyrproject.org/r/647 clock_control/stm32f10x:
introduce new driver for STM32F10x RCC
https://gerrit.zephyrproject.org/r/648 soc/stm32f1: add GPIO
registers mapping
https://gerrit.zephyrproject.org/r/649 pinmux/stm32: add common
driver for STM32 pinmux
https://gerrit.zephyrproject.org/r/650 serial/stm32: add new driver
for STM32 UART
https://gerrit.zephyrproject.org/r/651 gpio/stm32: add common driver
for STM32 GPIO
https://gerrit.zephyrproject.org/r/652 boards/stm32_mini_a15: add
new board
https://gerrit.zephyrproject.org/r/653 samples/drivers/disco: add
'disco' sample program
https://gerrit.zephyrproject.org/r/698 arch: arm: move nmi to common
location
https://gerrit.zephyrproject.org/r/711 clock_control/Kconfig: move
quark_se entries to separate file
https://gerrit.zephyrproject.org/r/712 clock_control: extend API
with clock rate query operation
https://gerrit.zephyrproject.org/r/713 soc/stm32f1/gpio: implement
GPIO support
https://gerrit.zephyrproject.org/r/714 soc/stm32f1/pinmux: implement
STM32 pinmux integration
https://gerrit.zephyrproject.org/r/715 boards/nucleo_f103rb: add new
board

Cheers,
--
Maciek Borzecki

Hi,

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;
__ASSERT(api->input, "%s not implemented", __func__);
return api->input(dev, pin, func);
}

Something like that yes, though I'd put empty lines in between ;)

Tomasz

On Wed, Mar 9, 2016 at 11:00 AM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Maciek,

An obligatory example:

<---- code --------->
struct pinmux_driver_api {
pmux_set set;
pmux_get get;
pmux_pullup pullup;
pmux_input input;
};
...

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;

/* proposed enhacement */
if (!api->input) {
return DEV_NO_SUPPORT;
}

Note that on many API, some calls have a mandatory support (let's say
spi_transceive),
so such test would not have to be generalized.

What about a different approach:

Thing is, Zephyr is an OS for embedded platform thus the developer is really
close to the hw he is using.
Thus, he should know what's available in terms of drivers and, finally, what
features these drivers expose.

So, instead of multiplying such test, which will take some bytes here and
there.
We could do this test through __ASSERT(). The user, while testing its app
could enable the assert checks
and verify he is not using unsupported features. And he could fix his code
accordingly, then disable the asserts.

If I understood correctly what you're suggesting, the example code
could look like this:

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;
__ASSERT(api->input, "%s not implemented", __func__);
return api->input(dev, pin, func);
}

Cheers,
--
Maciek Borzecki

Hi,


On Wed, 9 Mar 2016 11:00:49 +0100
Tomasz Bursztyka <tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Maciek,

An obligatory example:

<---- code --------->
struct pinmux_driver_api {
pmux_set set;
pmux_get get;
pmux_pullup pullup;
pmux_input input;
};
...

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;

/* proposed enhacement */
if (!api->input) {
return DEV_NO_SUPPORT;
}

There was a thread before [1] about differing between an API not supported and an API not
implemented for a given driver implementation. This would be done by returning DEV_NOT_IMPLEMENTED
instead of DEV_NO_SUPPORT. DEV_NOT_IMPLEMENTED hasn't been added to device.h yet, but the idea got
a green light back then from what I can tell.

This RFC would block that, so we need to keep this in mind and re-visit that discussion.

Note that on many API, some calls have a mandatory support (let's say
spi_transceive),
so such test would not have to be generalized.

What about a different approach:

Thing is, Zephyr is an OS for embedded platform thus the developer is
really close to the hw he is using.
Thus, he should know what's available in terms of drivers and, finally,
what features these drivers expose.

So, instead of multiplying such test, which will take some bytes here
and there.
We could do this test through __ASSERT(). The user, while testing its
app could enable the assert checks
and verify he is not using unsupported features. And he could fix his
code accordingly, then disable the asserts.

+1 for this to be considered.

Cheers,
jesus

[1]
https://lists.zephyrproject.org/archives/list/devel(a)lists.zephyrproject.org/thread/ONIGOHXRQWFFEAP2UBURZV7IOSFPQH5F/#ONIGOHXRQWFFEAP2UBURZV7IOSFPQH5F

Hi Maciek,

An obligatory example:

<---- code --------->
struct pinmux_driver_api {
pmux_set set;
pmux_get get;
pmux_pullup pullup;
pmux_input input;
};
...

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;

/* proposed enhacement */
if (!api->input) {
return DEV_NO_SUPPORT;
}

Note that on many API, some calls have a mandatory support (let's say
spi_transceive),
so such test would not have to be generalized.

What about a different approach:

Thing is, Zephyr is an OS for embedded platform thus the developer is
really close to the hw he is using.
Thus, he should know what's available in terms of drivers and, finally,
what features these drivers expose.

So, instead of multiplying such test, which will take some bytes here
and there.
We could do this test through __ASSERT(). The user, while testing its
app could enable the assert checks
and verify he is not using unsupported features. And he could fix his
code accordingly, then disable the asserts.

Not sure this would fit all uses case though, I might miss something.

Tomasz

Hi,

I'd like to make a proposal for enhancement of driver API
callbacks, but before jumping to code I'd like to get some feedback first.

Right now, whenever a call to an API is made, we directly jump to call
the driver's implementation. This is ok for the general case, when we
know that every driver will implement all callbacks.

However, this approach has some downsides in my opinion and might
become an issue once the number of drivers grows or the APIs become
larger.

First of all, in practice, not every driver will be able to implement all
callbacks. It is my understanding that the API will cover only a subset
of common functionality. However, as an example, the PWM driver for
STM32 may not have an applicable method of setting phase. In such case,
the driver would provide a dummy callback returning DEV_NO_SUPPORT.

Secondly, the current approach requires that whenever an API is
augmented with a new callback, someone must track down all driver
implementations and add this dummy callback.

Once the errno patches hit master, we'll need to make sure that
dummy callbacks (if present) return proper error code.

Given that, I'm proposing to add checks whether the driver implements
given callback in the public API calls. This has some benefits. Firstly,
we'll always have a default code path. Someone working on a driver
does not have add a dummy implementation.

Secondly, this will help to enforce consistent return values for a
case when a device lacks certain feature/functionality. Lack of
callback may return DEV_NO_SUPPORT, or ENOSYS/ENOTSUP.

We can rely on static *_driver_api struct allocation, and just fill in
the blanks. This is important when adding new API calls. IIRC Linux
does that a lot.

Lastly, I believe that this will give a minimal size reduction if one
happens to use drivers with dummy callbacks.

An obligatory example:

<---- code --------->
struct pinmux_driver_api {
pmux_set set;
pmux_get get;
pmux_pullup pullup;
pmux_input input;
};
...

static inline uint32_t pinmux_pin_input_enable(struct device *dev,
uint32_t pin,
uint8_t func)
{
struct pinmux_driver_api *api;

api = (struct pinmux_driver_api *) dev->driver_api;

/* proposed enhacement */
if (!api->input) {
return DEV_NO_SUPPORT;
}

return api->input(dev, pin, func);
}
<------ code ------------->

A similar thing is also a part of clock_control API review:
https://gerrit.zephyrproject.org/r/#/c/712/

Cheers,
--
Maciek Borzecki

On Tue, 2016-03-08 at 17:01 -0500, Benjamin Walsh wrote:

Hang on on this, I'm cleaning up a GDB server implementation that we
want to upstream, and it uses at least dynamic exceptions. I'll see
if I
can refactor it to use static ones instead.

Sure, let me know.
If it ends up you need a dynamic handler for one specific exception on
x86 we could treat it as a special case. irq_offload() is like this.

Andrew

Hang on on this, I'm cleaning up a GDB server implementation that we
want to upstream, and it uses at least dynamic exceptions. I'll see if I
can refactor it to use static ones instead.

Problem statement:
We currently have two separate implementations on how to connect
interrupts on all arches: static IRQs and dynamic IRQs. On x86 only we
also have static and dynamic APIs for connecting exception handlers.
Although static interrupts & exceptions impose some constraints on
developers, they save a nontrivial amount of RAM (generally 500b to 1K
depending on configuration) as certain data structures like the IDT do
not need to be writable and can remain in ROM. In the interest of
keeping the Zephyr codebase small and not having to maintain two
different ways of doing the same thing, I propose we deprecate and
eventually remove the dynamic implementations.

Details are below. In the discussion I only talk about interrupts, but
for x86 dynamic vs. static exceptions the same arguments apply. On ARC
and ARM all the exceptions are hard-coded to their handlers and there
is no API for them.

The static IRQ macro IRQ_CONNECT() imposes the constraint that all its
arguments be constants resolvable at build time. This includes the IRQ
line number and the context data pointer. This makes things a little
clumsy if you have a single driver that has multiple instances on the
system. Since you need to use build-time constants you cannot do
something like this:

int mydriver_init(struct device *dev)
{
    struct mydriver_config *config = dev->config->config_info;
    ...
    IRQ_CONNECT(config->irq, config->irq_prio, mydriver_isr, dev,      
                config->irq_flags);
    irq_enable(config->irq);
    ...
}

Instead, the idiom used in Zephyr drivers is to define a per-instance
config function and save a pointer to it in the config struct:

void mydriver_0_config_fn(void)
{
    IRQ_CONNECT(MYDRIVER_0_IRQ, MYDRIVER_0_PRI, mydriver_isr,
                DEVICE_GET(mydriver_0), MYDRIVER_0_FLAGS);
    irq_enable(MYDRIVER_0_IRQ);
}

struct mydriver_config mydriver_0_config {
    ...
    .config_func = mydriver_0_config_fn;
    ...
};

int mydriver_init(struct device *dev)
{
    struct mydriver_config
*config = dev->config->config_info;
    ...
    config->config_func();
   
 ...
}

IRQ_CONNECT is not actually a function; it does have a runtime
component (to program the interrupt controller) but it does linker or
assembly black magic to set up all the relevant data structures at
build time.

What about instances where the interrupt arguments are simply not known
at build time? I feel that for Zephyr this will never be a problem
given the classes of devices we need to support:

- PCI devices do announce what their interrupts are, but for
microcontrollers with PCI busses (like galileo) we already know this
information at build time anyway. Once you get to the point where the
hardware really does have hot-pluggable peripherals you might as well
run Linux on it; adding proper hotplug support to Zephyr is a much
larger thing than just having dynamic interrupts.

- Microkernel Task IRQs currently rely on dynamic interrupts, but this
is an implementation detail. We are considering removing these from
Zephyr as well, but if not, it's possible to refactor them to use
static.

- None of the drivers included in the Zephyr tree use dynamic
interrupts.

- A few of the footprint tests use dynamic interrupts, but again this
is an implementation detail.

The RAM savings come from a few areas which differ per arch.

x86:
- the IDT can remain in ROM. This is where most of the savings come
from.
- The _interrupt_vectors_allocated and dyn_irq_list data structures can
be omitted from RAM if no dynamic interrupts are used.
- The IRQ_CONNECT() macro as part of its job automatically generates
the assembly language stub. These stubs trampoline to common code which
fetches the context pointer associated with the interrupt; this is how
our ISRs can be passed parameters. For dynamic interrupts you need to
know how many dynamic IRQs you plan on using in advance and set
CONFIG_NUM_DYNAMIC_STUBS; if the number of calls to
irq_connect_dynamic() exceeds this you will crash.

ARC/ARM:
On these two arches both static and dynamic interrupts are tracked in
sw_isr_table, an array indexed by IRQ line with each element noting the
ISR and parameter for that IRQ. If no dynamic IRQs are used, this table
can remain in ROM.

In summary, I'm not currently seeing any scenarios where dynamic IRQs
are necessary, and would like to attach __attribute__((deprecated))
tags to their APIs and eventually remove them from the kernel.

Andrew

Hi Corey,

You'll have to wait k64f spi driver gets into the tree first, unless you
want to try under review patch.

You won't have to tell anything about any pins. The driver manages all
that for you, hopefully.
You don't have to use the driver itself. You'll just have to grab the
right instance via device_get_binding()
and use it through the API described in include/spi.h

Tomasz

Hi,

On Mon, Mar 07, 2016, Daniel Leung wrote:

Another thing is: it seems like that you expect the app developers
to statically allocate a bunch of this struct to have multiple callbacks.
This is, AFAIK, not a common practice when setting callbacks. Developers
may simply allocate a struct in stack (like inside a function) and
pass it to the function. This struct may go out of scope, and
the resulting errors and exceptions will be confusing to developers.
Could we do something to mitigate this?

I think the best we can do is to clearly document the life-time
expectation of the struct. I don't think there's really any other way
around it with asynchronous APIs that need to track more context than
just a simple function pointer. We've actually got several similar
approaches in the Bluetooth API as well.

Johan

Hi,

Im looking to use the zephyr project spi driver for writing to the SD card
slot on the FRDM K64f board. So far I have found little to no
documentation on how to use the spi driver. For example how do I tell it
which pins to use for the MISO and MOSI and so forth?

Thanks for any help,

Corey

On Mon, Mar 07, 2016 at 06:07:07PM +0200, Johan Hedberg wrote:

Hi Tomasz,

On Mon, Mar 07, 2016, Tomasz Bursztyka wrote:

-typedef void (*gpio_callback_t)(struct device *port, uint32_t pin);
+
+struct gpio_callback
+{
+ void (*handler)(struct device *port,
+ gpio_callback_t *cb,
+ uint32_t pins);
+ uint32_t pin_mask;
+
+ struct gpio_callback *_next;
+};
+
+typedef struct gpio_callback gpio_callback_t;

I realize this typedef is inherited from the original code, but do we
really need/want to enforce an opaque type here? The general preference
with the coding style (inherited from Linux and checkpatch even
complains about it) is to avoid typedefs whenever possible. I could e.g.
imagine a handler function wanting set/unset some pins in the pin_mask
when it gets called, in which case the struct couldn't be considered
completely opaque.

Another thing is: it seems like that you expect the app developers
to statically allocate a bunch of this struct to have multiple callbacks.
This is, AFAIK, not a common practice when setting callbacks. Developers
may simply allocate a struct in stack (like inside a function) and
pass it to the function. This struct may go out of scope, and
the resulting errors and exceptions will be confusing to developers.
Could we do something to mitigate this?


----------
Daniel Leung