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

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)

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.

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

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

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

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

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

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

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.

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

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

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