Re: Pinmux driver model: per board vs. unified
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.toggle quoted messageShow quoted text
-----Original Message-----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.
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.Where the library code lands is an open question ATM. This is veryThe only thing that may complicate matters a bit is the point that today we
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.