Topics

Pinmux driver model: per board vs. unified


Vinicius Costa Gomes
 

Hi,

(Sorry for the click bait-ish subject)

As per Dirk's suggestion moving the discussion on the pinmux model[1][2] to
the mailing list.

Quoting Dirk:
So I like the idea here to move all the code common to manipulating
the pinmux registers in to a single place.

The way this change goes about that is wrong IMHO. Each board should
have its own driver that uses the library code, not a single driver
that picks up the board specific code at link time. This forces the
user to look in their .config to figure out what board specific code
is included in their image.
Makes sense.


Where the library code lands is an open question ATM. This is very
similar to the QMSI stuff where we will be using a library for
accessing the registers in the IP block.
The only thing that may complicate matters a bit is the point that today
we have two ways of interacting with the pinmux registers, one that is
used by the board specific code and one that may be used by applications
(disabled by default).

Or do you mean we continue providing two pinmux APIs?


We need to figure out the correct long term model. This discussion
should happen on the mailing list instead of gerrit so all involved
can see whats going on.

Cheers,
--
Vinicius

[1] https://gerrit.zephyrproject.org/r/#/c/385

[2] https://gerrit.zephyrproject.org/r/#/c/386


Andre Guedes <andre.guedes@...>
 

Hi all,

Here is one proposal on how we can deal with pinmux drivers and board-
specific pinmux configurations:

1. All pinmux drivers land in driver/pinmux/ directory and they all
implement the include/pinmux.h API only.

2. Board-specific code (e.g. board/quark_d2000_crb/board.c) defines
the pinmux table and uses the pinmux APIs to apply its specific
configurations. To achieve that, we could define the board_init()
function in board.c which would handle any board-specific initialization,
including pinmux configuration. We could use SYS_INIT() so board_init()
is executed during kernel initialization.

In order to this approach work, we have to ensure the pinmux driver is
initialized before board_init() is called. This can be achieved by setting
pinmux driver initialization to PRIMARY level and board_init() to SECONDARY
level.

AFAICS, this approach would work for all boards, besides Galileo. Pinmux
in Galileo is quite different and it would require a special handling.

Since Galileo pinmux driver depends on others devices (I2C, PCA9535 and
PCAL9685) we cannot init it on PRIMARY level. So the Galileo board_init()
would be set to SECONDARY level and we use the 'prio' argument from
SYS_INIT() to ensure board_init() is executed after galileo pinmux
initialization.

Regards,

Andre

Quoting Vinicius Costa Gomes (2016-02-23 15:18:25)

So I like the idea here to move all the code common to manipulating
the pinmux registers in to a single place.

The way this change goes about that is wrong IMHO. Each board should
have its own driver that uses the library code, not a single driver
that picks up the board specific code at link time. This forces the
user to look in their .config to figure out what board specific code
is included in their image.
Makes sense.


Where the library code lands is an open question ATM. This is very
similar to the QMSI stuff where we will be using a library for
accessing the registers in the IP block.
The only thing that may complicate matters a bit is the point that today
we have two ways of interacting with the pinmux registers, one that is
used by the board specific code and one that may be used by applications
(disabled by default).

Or do you mean we continue providing two pinmux APIs?


We need to figure out the correct long term model. This discussion
should happen on the mailing list instead of gerrit so all involved
can see whats going on.

Cheers,
--
Vinicius

[1] https://gerrit.zephyrproject.org/r/#/c/385

[2] https://gerrit.zephyrproject.org/r/#/c/386


Kalowsky, Daniel <daniel.kalowsky@...>
 

In the future, when you're making architectural changes like this, send to the mailing list first. Submitting it to gerrit only is NOT advised, and limits the audience which can view the commentary. This highlights the entire reason why we have an RFC process in the first place.

-----Original Message-----
From: Vinicius Costa Gomes [mailto:vinicius.gomes(a)intel.com]
Sent: Tuesday, February 23, 2016 10:18 AM
To: devel(a)lists.zephyrproject.org
Subject: [devel] Pinmux driver model: per board vs. unified

Hi,

(Sorry for the click bait-ish subject)

As per Dirk's suggestion moving the discussion on the pinmux model[1][2] to
the mailing list.

Quoting Dirk:
So I like the idea here to move all the code common to manipulating
the pinmux registers in to a single place.

The way this change goes about that is wrong IMHO. Each board should
have its own driver that uses the library code, not a single driver
that picks up the board specific code at link time. This forces the
user to look in their .config to figure out what board specific code
is included in their image.
Makes sense.
This is a re-tread of something we already tried initially. Placing everything into a library wasn't a bad idea initially, but we did find a couple of issues with it. For example, it did not scale out cleanly to other architectures. On the other hand, in some devices we were being asked to shave out any extra bytes possible, the overhead of setting up the function call became a little too much. The footprint tests on this patch confirm an increase in the ROM footprint by an average of 312 bytes. Oddly it shows a slight decrease in RAM on the footprint-max tests only (everything else is an increase), which has an unknown root cause as of yet to me.

Second this is slow. Calling the _pinmux_dev_set() which reads+modifies+writes in a non-atomic fashion for two bits to the hardware repeatedly instead of making a single 32-bit write introduces a pretty big increase in execution time. As this driver/function is essential to the board bring up, you are now negatively impacting boot time.

Third, you do reduce LOC, which is a good thing and something I applaud. This change is really impacting code that is well tested and extremely static at this point. I'm extremely hesitant to change code like this for minimal/"no" benefit. Most of the LOCs removed are tied to specific boards selection for building, not impacting an everyday build, which is why I say no benefit.

Where the library code lands is an open question ATM. This is very
similar to the QMSI stuff where we will be using a library for
accessing the registers in the IP block.
The only thing that may complicate matters a bit is the point that today we
have two ways of interacting with the pinmux registers, one that is used by
the board specific code and one that may be used by applications (disabled
by default).
I'm not sure I follow the complication here. You can enable the PINMUX_DEV flag via PRJ for any application that needs to control the pinmux. We have explicitly disabled this by default for very specific reasons of not wanting to debug/support unknown variations of the pinmux (we learned from our mistake really quickly), and wanted end users to actively know they were moving into potentially "untested functionality" by changing these values. Since any application change already requires re-compiling the kernel, I'm not sure I see the concern with having an application enable this feature if needed.

In cases where applications really need to change the pinmux behavior, I believe any competent system integrator will be making changes directly to the pinmux.c file. Changing the default values rather than making them at application run-time provides a single point of board configuration. I further believe anyone developing a product using Zephyr will more than likely be creating their own boards/product-name which should be a self-contained product.

But for the sake of discussion, let's assume we move forward with the idea of making a library routine for everything. What needs to be done?

1) The name needs to be fixed. As noted in the patch already, calling this the generic Quark SOC pinmux is wrong and mis-leading. This doesn't support the X10x0 family, is unclear if it supports the D1000 family (I haven't looked), and is really specific only to the x86 CPU family/model.

2) Zephyr is multiple architectures. Changing the code-base architecture for Intel specific boards only, while ignoring the other architectures, is reason enough to -2 a patch. Please make sure when you're making changes like this in the future to reflect the change on all supported boards. If you're moving one, you're moving all of them to keep consistency. For example, the arduino_due could potentially have the pinmux moved into the same directory and be renamed to atmel_sam3x_pinmux.c (or some variation).

3) Investigate how to limit the r+m+w overhead of the calls to _pinmux_dev_set().

4) Verify that the Quark D2000 will continue to work when writing out mistakenly to registers it does not support, but the Quark SE does. It is a very subtle but important variation. I suspect that this won't be an issue, but it is one that has not been tested.


--
Vinicius

[1] https://gerrit.zephyrproject.org/r/#/c/385

[2] https://gerrit.zephyrproject.org/r/#/c/386


Vinicius Costa Gomes
 

Hi,

"Kalowsky, Daniel" <daniel.kalowsky(a)intel.com> writes:

In the future, when you're making architectural changes like this, send to the mailing list first. Submitting it to gerrit only is NOT advised, and limits the audience which can view the commentary. This highlights the entire reason why we have an RFC process in the first place.
Thanks for the well thought answer.

-----Original Message-----
From: Vinicius Costa Gomes [mailto:vinicius.gomes(a)intel.com]
Sent: Tuesday, February 23, 2016 10:18 AM
To: devel(a)lists.zephyrproject.org
Subject: [devel] Pinmux driver model: per board vs. unified

Hi,

(Sorry for the click bait-ish subject)

As per Dirk's suggestion moving the discussion on the pinmux model[1][2] to
the mailing list.

Quoting Dirk:
So I like the idea here to move all the code common to manipulating
the pinmux registers in to a single place.

The way this change goes about that is wrong IMHO. Each board should
have its own driver that uses the library code, not a single driver
that picks up the board specific code at link time. This forces the
user to look in their .config to figure out what board specific code
is included in their image.
Makes sense.
This is a re-tread of something we already tried initially. Placing
everything into a library wasn't a bad idea initially, but we did find
a couple of issues with it. For example, it did not scale out cleanly
to other architectures. On the other hand, in some devices we were
being asked to shave out any extra bytes possible, the overhead of
setting up the function call became a little too much. The footprint
tests on this patch confirm an increase in the ROM footprint by an
average of 312 bytes. Oddly it shows a slight decrease in RAM on the
footprint-max tests only (everything else is an increase), which has
an unknown root cause as of yet to me.
I am worried too about the ROM increase. Fair point.


Second this is slow. Calling the _pinmux_dev_set() which
reads+modifies+writes in a non-atomic fashion for two bits to the
hardware repeatedly instead of making a single 32-bit write introduces
a pretty big increase in execution time. As this driver/function is
essential to the board bring up, you are now negatively impacting boot
time.
The impact is negative, I agree, but I would guess that the impact is
minimal, perhaps even inside the error margin of any measurement (to be
fair, I did not do any experiments).


Third, you do reduce LOC, which is a good thing and something I
applaud. This change is really impacting code that is well tested and
extremely static at this point. I'm extremely hesitant to change code
like this for minimal/"no" benefit. Most of the LOCs removed are tied
to specific boards selection for building, not impacting an everyday
build, which is why I say no benefit.
The patch replaces three copies of well tested code by one copy of well
tested code. Specific boards that I am building for and using everyday
:-) I only changed how the code is called, so I guess the effect this
patch could have on weakening the value of previous tests is minor.


Where the library code lands is an open question ATM. This is very
similar to the QMSI stuff where we will be using a library for
accessing the registers in the IP block.
The only thing that may complicate matters a bit is the point that today we
have two ways of interacting with the pinmux registers, one that is used by
the board specific code and one that may be used by applications (disabled
by default).
I'm not sure I follow the complication here. You can enable the
PINMUX_DEV flag via PRJ for any application that needs to control the
pinmux. We have explicitly disabled this by default for very specific
reasons of not wanting to debug/support unknown variations of the
pinmux (we learned from our mistake really quickly), and wanted end
users to actively know they were moving into potentially "untested
functionality" by changing these values. Since any application change
already requires re-compiling the kernel, I'm not sure I see the
concern with having an application enable this feature if needed.

In cases where applications really need to change the pinmux behavior,
I believe any competent system integrator will be making changes
directly to the pinmux.c file. Changing the default values rather
than making them at application run-time provides a single point of
board configuration. I further believe anyone developing a product
using Zephyr will more than likely be creating their own
boards/product-name which should be a self-contained product.
One thing that I was considering was that the existance of the
PINMUX_DEV configuration option could be questioned, as it felt like
protecting against the developer.


But for the sake of discussion, let's assume we move forward with the
idea of making a library routine for everything. What needs to be
done?
This is the most important point. If the feeling is that I am trying to
fix what's not broken, I can abandon this happily.


1) The name needs to be fixed. As noted in the patch already, calling
this the generic Quark SOC pinmux is wrong and mis-leading. This
doesn't support the X10x0 family, is unclear if it supports the D1000
family (I haven't looked), and is really specific only to the x86 CPU
family/model.
Of course. I am still looking for a good name,
PINMUX_QUARK_MCU_EXCEPT_X1000_AND_D1000 doesn't sound good
enough (from a quick look D1000 seems different).


2) Zephyr is multiple architectures. Changing the code-base
architecture for Intel specific boards only, while ignoring the other
architectures, is reason enough to -2 a patch. Please make sure when
you're making changes like this in the future to reflect the change on
all supported boards. If you're moving one, you're moving all of them
to keep consistency. For example, the arduino_due could potentially
have the pinmux moved into the same directory and be renamed to
atmel_sam3x_pinmux.c (or some variation).
I didn't make those changes because I don't have the boards to test.


3) Investigate how to limit the r+m+w overhead of the calls to
_pinmux_dev_set().
Sure.


4) Verify that the Quark D2000 will continue to work when writing out
mistakenly to registers it does not support, but the Quark SE does.
It is a very subtle but important variation. I suspect that this
won't be an issue, but it is one that has not been tested.
Ok.



--
Vinicius

[1] https://gerrit.zephyrproject.org/r/#/c/385

[2] https://gerrit.zephyrproject.org/r/#/c/386
Cheers,
--
Vinicius


Dirk Brandewie <dirk.j.brandewie@...>
 

On 02/23/2016 02:06 PM, Andre Guedes wrote:
Hi all,

Here is one proposal on how we can deal with pinmux drivers and board-
specific pinmux configurations:

1. All pinmux drivers land in driver/pinmux/ directory and they all
implement the include/pinmux.h API only.
Don't hate it :-)

Although it occurs to me that the pinmux API is not all that useful
and could be considered harmful.

Setting up the pin configuration on an SOC/platform is an init time
only operation and should be by the integrator of the platform
so there is not a good reason to have public API where application
developers could break the rest of the system.

If someone comes up with use case where they need runtime (after init)
control of the platform pin configuration then they can build the
driver/API they need.


2. Board-specific code (e.g. board/quark_d2000_crb/board.c) defines
the pinmux table and uses the pinmux APIs to apply its specific
configurations.
To achieve that, we could define the board_init()
function in board.c which would handle any board-specific initialization,
including pinmux configuration. We could use SYS_INIT() so board_init()
is executed during kernel initialization.

In order to this approach work, we have to ensure the pinmux driver is
initialized before board_init() is called. This can be achieved by setting
pinmux driver initialization to PRIMARY level and board_init() to SECONDARY
level.
We could do this today but board_init() is really just a driver that is
called at the *right* time but does not expose any API.

We had all the init() functions for all driver defined in board.c back
in the day. board.c became a dumping ground so they were moved to the
driver implementations.

We spent a *lot of time and effort getting all the cruft out of
board.{c,h} we should not start putting it back.


AFAICS, this approach would work for all boards, besides Galileo. Pinmux
in Galileo is quite different and it would require a special handling.

Since Galileo pinmux driver depends on others devices (I2C, PCA9535 and
PCAL9685) we cannot init it on PRIMARY level. So the Galileo board_init()
would be set to SECONDARY level and we use the 'prio' argument from
SYS_INIT() to ensure board_init() is executed after galileo pinmux
initialization.
This is why we have 99 sub-levels in each major level so the integrator
has fine grained control over the order of initialization :-).

After writing this mail, looking at the code again I have a alternate
proposal.

1. move the current pinmux drivers back into drivers with reasonable
names.

2. remove the API support from all the drivers and move it to a
quark_pinmux_dev driver that just provides the API if people really
need runtime control of the pin configuration they can build the
driver.

So all the replicated code moves to a single place where it is needed
and the remaining pimux driver only do init() and are done.

The quark_se_devboard.c pinmux driver code becomes:

**** BEGIN ****
/* pinmux.c - general pinmux operation */

/*
* Copyright (c) 2015 Intel Corporation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include <nanokernel.h>
#include <device.h>
#include <init.h>
#include <pinmux.h>
#include <sys_io.h>
#include "pinmux/pinmux.h"


#ifndef CONFIG_PINMUX_DEV
#define PRINT(...) {; }
#else
#if defined(CONFIG_PRINTK)
#include <misc/printk.h>
#define PRINT printk
#elif defined(CONFIG_STDOUT_CONSOLE)
#define PRINT printf
#endif /* CONFIG_PRINTK */
#endif /*CONFIG_PINMUX_DEV */

#define MASK_2_BITS 0x3
#define PINMUX_PULLUP_OFFSET 0x00
#define PINMUX_SLEW_OFFSET 0x10
#define PINMUX_INPUT_OFFSET 0x20
#define PINMUX_SELECT_OFFSET 0x30

#define PINMUX_SELECT_REGISTER(base, reg_offset) \
(base + PINMUX_SELECT_OFFSET + (reg_offset << 2))

/*
* A little decyphering of what is going on here:
*
* Each pinmux register rperesents a bank of 16 pins, 2 bits per pin
for a total
* of four possible settings per pin.
*
* The first argument to the macro is name of the uint32_t's that is
being used
* to contain the bit patterns for all the configuration registers.
The pin
* number divided by 16 selects the correct register bank based on the pin
* number.
*
* The pin number % 16 * 2 selects the position within the register
bank for the
* bits controlling the pin.
*
* All but the lower two bits of the config values are masked off to ensure
* that we don't inadvertently affect other pins in the register bank.
*/
#define PIN_CONFIG(A, _pin, _func) \
(A[((_pin) / 16)] |= ((0x3 & (_func)) << (((_pin) % 16) * 2)))

/*
* This is the full pinmap that we have available on the board for
configuration
* including the ball position and the various modes that can be set.
In the
* _pinmux_defaults we do not spend any time setting values that are
using mode
* A as the hardware brings up all devices by default in mode A.
*/

/* pin, ball, mode A, mode B, mode C */
/* 0 F02, gpio_0, ain_0, spi_s_cs */
/* 1 G04, gpio_1, ain_1, spi_s_miso */
/* 2 H05, gpio_2, ain_2, spi_s_sck */
/* 3 J06, gpio_3, ain_3, spi_s_mosi */
/* 4 K06, gpio_4, ain_4, NA */ /* 15.4 GPIO */
/* 5 L06, gpio_5, ain_5, NA */ /* 15.4 GPIO */
/* 6 H04, gpio_6, ain_6, NA */ /* 15.4 GPIO */
/* 7 G03, gpio_7, ain_7, NA */
/* 8 L05, gpio_ss_0, ain_8, uart1_cts */ /* UART debug */
/* 9 M05, gpio_ss_1, ain_9, uart1_rts */ /* UART debug */
/* 10 K05, gpio_ss_2, ain_10 */
/* 11 G01, gpio_ss_3, ain_11 */
/* 12 J04, gpio_ss_4, ain_12 */
/* 13 G02, gpio_ss_5, ain_13 */
/* 14 F01, gpio_ss_6, ain_14 */
/* 15 J05, gpio_ss_7, ain_15 */
/* 16 L04, gpio_ss_8, ain_16, uart1_txd */ /* UART debug */
/* 17 M04, gpio_ss_9, ain_17, uart1_rxd */ /* UART debug */
/* 18 K04, uart0_rx, ain_18, NA */ /* BT UART */
/* 19 B02, uart0_tx, gpio_31, NA */ /* BT UART */
/* 20 C01, i2c0_scl, NA, NA */ /* EEPROM, BT, Light Sensor */
/* 21 C02, i2c0_sda, NA, NA */ /* EEPROM, BT, Light Sensor */
/* 22 D01, i2c1_scl, NA, NA */
/* 23 D02, i2c1_sda, NA, NA */
/* 24 E01, i2c0_ss_sda, NA, NA */
/* 25 E02, i2c0_ss_scl, NA, NA */
/* 26 B03, i2c1_ss_sda, NA, NA */ /* IMU */
/* 27 A03, i2c1_ss_scl, NA, NA */ /* IMU */
/* 28 C03, spi0_ss_miso, NA, NA */ /* IMU */
/* 29 E03, spi0_ss_mosi, NA, NA */ /* IMU */
/* 30 D03, spi0_ss_sck, NA, NA */ /* IMU */
/* 31 D04, spi0_ss_cs0, NA, NA */ /* IMU */
/* 32 C04, spi0_ss_cs1, NA, NA */
/* 33 B04, spi0_ss_cs2, gpio_29, NA */ /* 15.4 GPIO */
/* 34 A04, spi0_ss_cs3, gpio_30, NA */
/* 35 B05, spi1_ss_miso, NA, NA */
/* 36 C05, spi1_ss_mosi, NA, NA */
/* 37 D05, spi1_ss_sck, NA, NA */
/* 38 E05, spi1_ss_cs0, NA, NA */
/* 39 E04, spi1_ss_cs1, NA, NA */
/* 40 A06, spi1_ss_cs2, uart0_cts, NA */ /* BT UART */
/* 41 B06, spi1_ss_cs3, uart0_rts, NA */ /* BT UART */
/* 42 C06, gpio_8, spi1_m_sck, NA */ /* 15.4 SPI */
/* 43 D06, gpio_9, spi1_m_miso, NA */ /* 15.4 SPI */
/* 44 E06, gpio_10, spi1_m_mosi, NA */ /* 15.4 SPI */
/* 45 D07, gpio_11, spi1_m_cs0, NA */ /* 15.4 SPI GPIO CS */
/* 46 C07, gpio_12, spi1_m_cs1, NA */
/* 47 B07, gpio_13, spi1_m_cs2, NA */
/* 48 A07, gpio_14, spi1_m_cs3, NA */
/* 49 B08, gpio_15, i2s_rxd, NA */
/* 50 A08, gpio_16, i2s_rscki, NA */
/* 51 B09, gpio_17, i2s_rws, NA */
/* 52 A09, gpio_18, i2s_tsck, NA */
/* 53 C09, gpio_19, i2s_twsi, NA */
/* 54 D09, gpio_20, i2s_txd, NA */
/* 55 D08, gpio_21, spi0_m_sck, NA */ /* SPI Flash */
/* 56 E07, gpio_22, spi0_m_miso, NA */ /* SPI Flash */
/* 57 E09, gpio_23, spi0_m_mosi, NA */ /* SPI Flash */
/* 58 E08, gpio_24, spi0_m_cs0, NA */ /* SPI Flash */
/* 59 A10, gpio_25, spi0_m_cs1, NA */
/* 60 B10, gpio_26, spi0_m_cs2, NA */
/* 61 C10, gpio_27, spi0_m_cs3, NA */
/* 62 D10, gpio_28, NA, NA */
/* 63 E10, gpio_ss_10, pwm_0, NA */
/* 64 D11, gpio_ss_11, pwm_1, NA */
/* 65 C11, gpio_ss_12, pwm_2, NA */
/* 66 B11, gpio_ss_13, pwm_3, NA */
/* 67 D12, gpio_ss_14, clkout_32khz, NA */
/* 68 C12, gpio_ss_15, clkout_16mhz, NA */

/*
* On the QUARK_SE platform there are a minimum of 69 pins that can be
possibly
* set. This would be a total of 5 registers to store the
configuration as per
* the bit description from above
*/
#define PINMUX_MAX_REGISTERS 5
static void _pinmux_defaults(uint32_t base)
{
uint32_t mux_config[PINMUX_MAX_REGISTERS] = { 0, 0, 0, 0, 0};
int i = 0;

PIN_CONFIG(mux_config, 0, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 1, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 2, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 3, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 8, PINMUX_FUNC_C);
PIN_CONFIG(mux_config, 9, PINMUX_FUNC_C);
PIN_CONFIG(mux_config, 16, PINMUX_FUNC_C);
PIN_CONFIG(mux_config, 17, PINMUX_FUNC_C);
PIN_CONFIG(mux_config, 33, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 40, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 41, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 42, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 43, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 44, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 55, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 56, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 57, PINMUX_FUNC_B);
PIN_CONFIG(mux_config, 58, PINMUX_FUNC_B);

for (i = 0; i < PINMUX_MAX_REGISTERS; i++) {
PRINT("PINMUX: configuring register i=%d reg=%x", i,
mux_config[i]);
sys_write32(mux_config[i], PINMUX_SELECT_REGISTER(base, i));
}
}

static inline void _pinmux_pullups(uint32_t base_address) { };

int pinmux_initialize(struct device *port)
{
const struct pinmux_config *pmux = port->config->config_info;

_pinmux_defaults(pmux->base_address);
_pinmux_pullups(pmux->base_address);

return DEV_OK;
}

struct pinmux_config board_pmux = {
.base_address = CONFIG_PINMUX_BASE,
};

DEVICE_INIT(pmux, PINMUX_NAME, &pinmux_initialize,
NULL, &board_pmux,
SECONDARY, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT);

**** END ****

No code duplication, pinumx API only when you ask for it and the drivers
are back in the driver tree :-)

I think this satisfies the goals of the patch set and moves the pinmux
driver to not be a special case any more being off in the boards
directory

--Dirk

Regards,

Andre

Quoting Vinicius Costa Gomes (2016-02-23 15:18:25)
So I like the idea here to move all the code common to manipulating
the pinmux registers in to a single place.

The way this change goes about that is wrong IMHO. Each board should
have its own driver that uses the library code, not a single driver
that picks up the board specific code at link time. This forces the
user to look in their .config to figure out what board specific code
is included in their image.
Makes sense.


Where the library code lands is an open question ATM. This is very
similar to the QMSI stuff where we will be using a library for
accessing the registers in the IP block.
The only thing that may complicate matters a bit is the point that today
we have two ways of interacting with the pinmux registers, one that is
used by the board specific code and one that may be used by applications
(disabled by default).

Or do you mean we continue providing two pinmux APIs?


We need to figure out the correct long term model. This discussion
should happen on the mailing list instead of gerrit so all involved
can see whats going on.

Cheers,
--
Vinicius

[1] https://gerrit.zephyrproject.org/r/#/c/385

[2] https://gerrit.zephyrproject.org/r/#/c/386


Kalowsky, Daniel <daniel.kalowsky@...>
 

-----Original Message-----
From: Gomes, Vinicius
Sent: Wednesday, February 24, 2016 7:01 AM
To: Kalowsky, Daniel <daniel.kalowsky(a)intel.com>;
devel(a)lists.zephyrproject.org
Subject: RE: [devel] Pinmux driver model: per board vs. unified

Hi,

"Kalowsky, Daniel" <daniel.kalowsky(a)intel.com> writes:

In the future, when you're making architectural changes like this, send to
the mailing list first. Submitting it to gerrit only is NOT advised, and limits the
audience which can view the commentary. This highlights the entire reason
why we have an RFC process in the first place.
Thanks for the well thought answer.

-----Original Message-----
From: Vinicius Costa Gomes [mailto:vinicius.gomes(a)intel.com]
Sent: Tuesday, February 23, 2016 10:18 AM
To: devel(a)lists.zephyrproject.org
Subject: [devel] Pinmux driver model: per board vs. unified

Hi,

(Sorry for the click bait-ish subject)

As per Dirk's suggestion moving the discussion on the pinmux
model[1][2] to the mailing list.

Quoting Dirk:
So I like the idea here to move all the code common to manipulating
the pinmux registers in to a single place.

The way this change goes about that is wrong IMHO. Each board
should have its own driver that uses the library code, not a single
driver that picks up the board specific code at link time. This
forces the user to look in their .config to figure out what board
specific code is included in their image.
Makes sense.
This is a re-tread of something we already tried initially. Placing
everything into a library wasn't a bad idea initially, but we did find
a couple of issues with it. For example, it did not scale out cleanly
to other architectures. On the other hand, in some devices we were
being asked to shave out any extra bytes possible, the overhead of
setting up the function call became a little too much. The footprint
tests on this patch confirm an increase in the ROM footprint by an
average of 312 bytes. Oddly it shows a slight decrease in RAM on the
footprint-max tests only (everything else is an increase), which has
an unknown root cause as of yet to me.
I am worried too about the ROM increase. Fair point.


Second this is slow. Calling the _pinmux_dev_set() which
reads+modifies+writes in a non-atomic fashion for two bits to the
hardware repeatedly instead of making a single 32-bit write introduces
a pretty big increase in execution time. As this driver/function is
essential to the board bring up, you are now negatively impacting boot
time.
The impact is negative, I agree, but I would guess that the impact is minimal,
perhaps even inside the error margin of any measurement (to be fair, I did
not do any experiments).
While it is minimal, you do have to keep in mind that Zephyr itself may be just a library for a larger application with very strict needs for boot time. It's best to keep our impact as minimal as possible.

Third, you do reduce LOC, which is a good thing and something I
applaud. This change is really impacting code that is well tested and
extremely static at this point. I'm extremely hesitant to change code
like this for minimal/"no" benefit. Most of the LOCs removed are tied
to specific boards selection for building, not impacting an everyday
build, which is why I say no benefit.
The patch replaces three copies of well tested code by one copy of well
tested code. Specific boards that I am building for and using everyday
:-) I only changed how the code is called, so I guess the effect this patch could
have on weakening the value of previous tests is minor.
Agreed. And as Dirk pointed out to me off list, while the change is relatively minor right now if/when we have Quark SE++ or Quark D2000++, it's possible they will use the same interface and thus the code gets duplicated once again. So there is some good motivation to get this right now.

Where the library code lands is an open question ATM. This is very
similar to the QMSI stuff where we will be using a library for
accessing the registers in the IP block.
The only thing that may complicate matters a bit is the point that
today we have two ways of interacting with the pinmux registers, one
that is used by the board specific code and one that may be used by
applications (disabled by default).
I'm not sure I follow the complication here. You can enable the
PINMUX_DEV flag via PRJ for any application that needs to control the
pinmux. We have explicitly disabled this by default for very specific
reasons of not wanting to debug/support unknown variations of the
pinmux (we learned from our mistake really quickly), and wanted end
users to actively know they were moving into potentially "untested
functionality" by changing these values. Since any application change
already requires re-compiling the kernel, I'm not sure I see the
concern with having an application enable this feature if needed.

In cases where applications really need to change the pinmux behavior,
I believe any competent system integrator will be making changes
directly to the pinmux.c file. Changing the default values rather
than making them at application run-time provides a single point of
board configuration. I further believe anyone developing a product
using Zephyr will more than likely be creating their own
boards/product-name which should be a self-contained product.
One thing that I was considering was that the existance of the PINMUX_DEV
configuration option could be questioned, as it felt like protecting against the
developer.
My view on this is altering the pinmux_defaults() function should be what any final production product will more than likely use. The PINMUX_DEV flag is more useful for early prototyping.


But for the sake of discussion, let's assume we move forward with the
idea of making a library routine for everything. What needs to be
done?
This is the most important point. If the feeling is that I am trying to fix what's
not broken, I can abandon this happily.
It's worth looking into and seeing if it can be made to fit within the previous constraints.

1) The name needs to be fixed. As noted in the patch already, calling
this the generic Quark SOC pinmux is wrong and mis-leading. This
doesn't support the X10x0 family, is unclear if it supports the D1000
family (I haven't looked), and is really specific only to the x86 CPU
family/model.
Of course. I am still looking for a good name,
PINMUX_QUARK_MCU_EXCEPT_X1000_AND_D1000 doesn't sound good
enough (from a quick look D1000 seems different).
I like the new name, but would hate to type that in vim.

Oddly if I go to ark.intel.com and search for Quark, I get the following page:
http://ark.intel.com/products/family/79047/Intel-Quark-SoC

I do not see any mention of the Quark SE on that page (or in ARK at all). It may be worth calling this the PINMUX_QUARK_D2000 only until we can figure out what is going on with the ARK.

2) Zephyr is multiple architectures. Changing the code-base
architecture for Intel specific boards only, while ignoring the other
architectures, is reason enough to -2 a patch. Please make sure when
you're making changes like this in the future to reflect the change on
all supported boards. If you're moving one, you're moving all of them
to keep consistency. For example, the arduino_due could potentially
have the pinmux moved into the same directory and be renamed to
atmel_sam3x_pinmux.c (or some variation).
I didn't make those changes because I don't have the boards to test.
Talk to Inaky.

3) Investigate how to limit the r+m+w overhead of the calls to
_pinmux_dev_set().
Sure.


4) Verify that the Quark D2000 will continue to work when writing out
mistakenly to registers it does not support, but the Quark SE does.
It is a very subtle but important variation. I suspect that this
won't be an issue, but it is one that has not been tested.
Ok.


Vinicius Costa Gomes
 

Hi Dirk,

Dirk Brandewie <dirk.j.brandewie(a)intel.com> writes:

[...]


After writing this mail, looking at the code again I have a alternate
proposal.

1. move the current pinmux drivers back into drivers with reasonable
names.

2. remove the API support from all the drivers and move it to a
quark_pinmux_dev driver that just provides the API if people really
need runtime control of the pin configuration they can build the
driver.

So all the replicated code moves to a single place where it is needed
and the remaining pimux driver only do init() and are done.
I like it.

The only thing I am thinking about is polluting the drivers/pinmux/
directory when there are more than a couple of board variants. But this
may not be a problem.

I would only propose to have a different directory for the 'pinmux_dev'
driver. From what I understand, there would be no code shared between
the pinmux "board" driver and the pinmux "dev" driver (the only
information needed seems to be what is already provided by
'include/pinmux.h').

And all concerns that were raised about the increases in ROM size and boot
time overhead seems to be addressed.


Cheers,
--
Vinicius


Dirk Brandewie <dirk.j.brandewie@...>
 

On 02/24/2016 01:34 PM, Vinicius Costa Gomes wrote:
Hi Dirk,

Dirk Brandewie <dirk.j.brandewie(a)intel.com> writes:

[...]


After writing this mail, looking at the code again I have a alternate
proposal.

1. move the current pinmux drivers back into drivers with reasonable
names.

2. remove the API support from all the drivers and move it to a
quark_pinmux_dev driver that just provides the API if people really
need runtime control of the pin configuration they can build the
driver.

So all the replicated code moves to a single place where it is needed
and the remaining pimux driver only do init() and are done.
I like it.

The only thing I am thinking about is polluting the drivers/pinmux/
directory when there are more than a couple of board variants. But this
may not be a problem.

I would only propose to have a different directory for the 'pinmux_dev'
driver. From what I understand, there would be no code shared between
the pinmux "board" driver and the pinmux "dev" driver (the only
information needed seems to be what is already provided by
'include/pinmux.h').
yeah better idea :-D when the quark_se gets more design wins things
would get crowded.

Now the question is who is signing up to do the refactoring? :-)



And all concerns that were raised about the increases in ROM size and boot
time overhead seems to be addressed.


Cheers,
--
Vinicius


Vinicius Costa Gomes
 

Dirk Brandewie <dirk.j.brandewie(a)intel.com> writes:

[...]


yeah better idea :-D when the quark_se gets more design wins things
would get crowded.

Now the question is who is signing up to do the refactoring? :-)
I already started this. Hoping to have something to show later today or
tomorrow.



And all concerns that were raised about the increases in ROM size and boot
time overhead seems to be addressed.


Cheers,
--
Vinicius

Cheers,
--
Vinicius


Andre Guedes <andre.guedes@...>
 

Hi all,

Quoting Vinicius Costa Gomes (2016-02-24 18:34:16)
After writing this mail, looking at the code again I have a alternate
proposal.

1. move the current pinmux drivers back into drivers with reasonable
names.

2. remove the API support from all the drivers and move it to a
quark_pinmux_dev driver that just provides the API if people really
need runtime control of the pin configuration they can build the
driver.

So all the replicated code moves to a single place where it is needed
and the remaining pimux driver only do init() and are done.
I like it.

The only thing I am thinking about is polluting the drivers/pinmux/
directory when there are more than a couple of board variants. But this
may not be a problem.

I would only propose to have a different directory for the 'pinmux_dev'
driver. From what I understand, there would be no code shared between
the pinmux "board" driver and the pinmux "dev" driver (the only
information needed seems to be what is already provided by
'include/pinmux.h').
Yes, I also like this proposal and I think we should move with it instead
of with the proposal I did.

I have just one comment regarding the 'pinmux board driver'. The way I see
it, what we are calling 'pinmux board driver' is more like a board-specific
initialization code than a driver per se. And, as a board-specific code, I
think we should land it in board/.

For 'pinmux dev driver', yes, it totally make sense to me that we should
land them in driver/ and the user can build it if his/her *application*
requires it.

Regards,

Andre


Dmitriy Korovkin
 

It would be good to have design changes for pinmux outlined in one text. Are we talking about code consolidation for Quark SE/D2000 or the global change for all pinmuxes? Are we touching the part referred as "PINMUX_DEV"?
If we are just changing code for two SoCs, I have no problem with it at all. If we are changing the architecture, we really need the new approach clean, as the following concerns come up:
- pinmuxing may be implemented on a SoC or on a board level (Quark SE vs. Galileo), as well as other architectures may implement it on a different way (FRDM k64F, which is coming).
- reconfiguring pinmux from an application is not usual practice, but quite possible. Cutting this functionality off may make problems in case of "we have this pinmux configuration by default, but for this particular sample project we need to change it".
- pinmux_initialize() may and sometimes should not be implemented by calling set(), get() and other API functions, but implemented individually, just not to slow down the booting process.


Vinicius Costa Gomes
 

Hi Dmitriy,

Dmitriy Korovkin <dmitriy.korovkin(a)windriver.com> writes:

It would be good to have design changes for pinmux outlined in one
text. Are we talking about code consolidation for Quark SE/D2000 or
the global change for all pinmuxes? Are we touching the part referred
as "PINMUX_DEV"?
Good idea, this is going to be useful to see if what I have in mind is
aligned with what other's have.

This is a rough sketch of what's I am working on:

* Code organization:
drivers/
pinmux/
pinmux_arduino_101.c
pinmux_arduino_due.c
pinmux_galileo.c*
pinmux_galileo_priv.c
pinmux_galileo_priv.h
pinmux_quark_d2000_crb.c
pinmux_quark_se_dev.c
pinmux_dev/
pinmux_dev_quark_d2000.c
pinmux_dev_galileo.c
pinmux_dev_atmel_sam3x.c

* Galileo is a special case, and to avoid copied functions in the
pinmux_dev driver, '_galileo_set_pin()' will be made available to both
the "board" driver and "dev" driver via the 'pinmux_galileo_priv.h'
header.

The drivers in 'pinmux' will have only the equivalent of
'_pinmux_defaults()' part of the 'board/*/pinmux.c' files.

The drivers in 'pinmux_dev' will provide the pinmux reconfiguration part that
you refer below.

If we are just changing code for two SoCs, I have no problem with it
at all. If we are changing the architecture, we really need the new
approach clean, as the following concerns come up: - pinmuxing may be
implemented on a SoC or on a board level (Quark SE vs. Galileo), as
well as other architectures may implement it on a different way (FRDM
k64F, which is coming).
At first what comes to mind is that we could have a common header that
provides functions that are used by a family of boards. Or are you
talking about something else? (I don't know anything about FRDM k64F)

- reconfiguring pinmux from an application is not usual practice, but
quite possible. Cutting this functionality off may make problems in
case of "we have this pinmux configuration by default, but for this
particular sample project we need to change it".
We will provide the pinmux_dev driver for applications that wish to do this.

- pinmux_initialize() may and sometimes should not be implemented by
calling set(), get() and other API functions, but implemented
individually, just not to slow down the booting process.
The board drivers (in 'pinmux'), in the common case, will write directly
in the registers, configuring multiple pins at the same time,
maintaining what we have right now in Zephyr.


Cheers,
--
Vinicius


Dmitriy Korovkin
 

On 16-02-25 02:05 PM, Vinicius Costa Gomes wrote:
Hi Dmitriy,

Dmitriy Korovkin <dmitriy.korovkin(a)windriver.com> writes:

It would be good to have design changes for pinmux outlined in one
text. Are we talking about code consolidation for Quark SE/D2000 or
the global change for all pinmuxes? Are we touching the part referred
as "PINMUX_DEV"?
Good idea, this is going to be useful to see if what I have in mind is
aligned with what other's have.

This is a rough sketch of what's I am working on:

* Code organization:
drivers/
pinmux/
pinmux_arduino_101.c
pinmux_arduino_due.c
pinmux_galileo.c*
pinmux_galileo_priv.c
pinmux_galileo_priv.h
pinmux_quark_d2000_crb.c
pinmux_quark_se_dev.c
pinmux_dev/
pinmux_dev_quark_d2000.c
pinmux_dev_galileo.c
pinmux_dev_atmel_sam3x.c

* Galileo is a special case, and to avoid copied functions in the
pinmux_dev driver, '_galileo_set_pin()' will be made available to both
the "board" driver and "dev" driver via the 'pinmux_galileo_priv.h'
header.
I agree, Galileo is a special case and it's initialization code may/will
share enough code with development part. I think, pinmux_galileo_priv.c
will implement those common functions, am I right?

The drivers in 'pinmux' will have only the equivalent of
'_pinmux_defaults()' part of the 'board/*/pinmux.c' files.

The drivers in 'pinmux_dev' will provide the pinmux reconfiguration part that
you refer below.

If we are just changing code for two SoCs, I have no problem with it
at all. If we are changing the architecture, we really need the new
approach clean, as the following concerns come up: - pinmuxing may be
implemented on a SoC or on a board level (Quark SE vs. Galileo), as
well as other architectures may implement it on a different way (FRDM
k64F, which is coming).
At first what comes to mind is that we could have a common header that
provides functions that are used by a family of boards. Or are you
talking about something else? (I don't know anything about FRDM k64F)
As far as I see, FRDM k64f looks like something in between Galileo and
Quark SE in terms of complexity.

- reconfiguring pinmux from an application is not usual practice, but
quite possible. Cutting this functionality off may make problems in
case of "we have this pinmux configuration by default, but for this
particular sample project we need to change it".
We will provide the pinmux_dev driver for applications that wish to do this.

- pinmux_initialize() may and sometimes should not be implemented by
calling set(), get() and other API functions, but implemented
individually, just not to slow down the booting process.
The board drivers (in 'pinmux'), in the common case, will write directly
in the registers, configuring multiple pins at the same time,
maintaining what we have right now in Zephyr.
Agreed,
Regards,
Dmitriy


Cheers,
--
Vinicius


Vinicius Costa Gomes
 

Hi,

Dmitriy Korovkin <dmitriy.korovkin(a)windriver.com> writes:

On 16-02-25 02:05 PM, Vinicius Costa Gomes wrote:
Hi Dmitriy,

Dmitriy Korovkin <dmitriy.korovkin(a)windriver.com> writes:

It would be good to have design changes for pinmux outlined in one
text. Are we talking about code consolidation for Quark SE/D2000 or
the global change for all pinmuxes? Are we touching the part referred
as "PINMUX_DEV"?
Good idea, this is going to be useful to see if what I have in mind is
aligned with what other's have.

This is a rough sketch of what's I am working on:

* Code organization:
drivers/
pinmux/
pinmux_arduino_101.c
pinmux_arduino_due.c
pinmux_galileo.c*
pinmux_galileo_priv.c
pinmux_galileo_priv.h
pinmux_quark_d2000_crb.c
pinmux_quark_se_dev.c
pinmux_dev/
pinmux_dev_quark_d2000.c
pinmux_dev_galileo.c
pinmux_dev_atmel_sam3x.c

* Galileo is a special case, and to avoid copied functions in the
pinmux_dev driver, '_galileo_set_pin()' will be made available to both
the "board" driver and "dev" driver via the 'pinmux_galileo_priv.h'
header.
I agree, Galileo is a special case and it's initialization code may/will
share enough code with development part. I think, pinmux_galileo_priv.c
will implement those common functions, am I right?
Exactly, from what I can see, it will be only one common function.


The drivers in 'pinmux' will have only the equivalent of
'_pinmux_defaults()' part of the 'board/*/pinmux.c' files.

The drivers in 'pinmux_dev' will provide the pinmux reconfiguration part that
you refer below.

If we are just changing code for two SoCs, I have no problem with it
at all. If we are changing the architecture, we really need the new
approach clean, as the following concerns come up: - pinmuxing may be
implemented on a SoC or on a board level (Quark SE vs. Galileo), as
well as other architectures may implement it on a different way (FRDM
k64F, which is coming).
At first what comes to mind is that we could have a common header that
provides functions that are used by a family of boards. Or are you
talking about something else? (I don't know anything about FRDM k64F)
As far as I see, FRDM k64f looks like something in between Galileo and
Quark SE in terms of complexity.
I see. I wouldn't like that these _priv.{c,h} idiom would become the
norm, but two instances may be acceptable. If there are more cases like
this, I would consider providing a pinmux library, as was suggested
earlier in the thread, but this is better left for when this becomes a
real issue.


- reconfiguring pinmux from an application is not usual practice, but
quite possible. Cutting this functionality off may make problems in
case of "we have this pinmux configuration by default, but for this
particular sample project we need to change it".
We will provide the pinmux_dev driver for applications that wish to do this.

- pinmux_initialize() may and sometimes should not be implemented by
calling set(), get() and other API functions, but implemented
individually, just not to slow down the booting process.
The board drivers (in 'pinmux'), in the common case, will write directly
in the registers, configuring multiple pins at the same time,
maintaining what we have right now in Zephyr.
Agreed,
Regards,
Dmitriy


Cheers,
--
Vinicius

Cheers,
--
Vinicius