Re: Pinmux driver model: per board vs. unified


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.

Join devel@lists.zephyrproject.org to automatically receive all group messages.