Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Stephen Warren]

On 02/20/2013 12:08 AM, Shawn Guo wrote:
> This turns the imx pin function number defined by binding document
> into #define constants in header which can be used in dts and handled
> by pre-processor to improve the readability of device tree sources.

> diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx35-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx35-pinctrl.txt

> -See below for available PIN_FUNC_ID for imx35:
> -0 MX35_PAD_CAPTURE__GPT_CAPIN1
...
> -951 MX35_PAD_TEST_MODE__TCU_TEST_MODE
> +Refer to arch/arm/boot/dts/imx35-pinfunc.h for all available imx35 PIN_FUNC_ID.

So that path is specific to the Linux kernel. The DT binding
documentation isn't supposed to be specific to the Linux kernel. I
wonder if we shouldn't put all the header files into
Documentation/devicetree/bindings rather than arch/*/boot/dts. That way,
you could encode relative path names into the DT binding docs. The
headers are logically part of the documentation anyway.

This all plays into whatever plans exist for removing the DT binding
docs and/or .dts files from the kernel, and how things will end up
getting laid out then though. That's something we should discuss with at
least Grant and Rob. I note that the DT maintainers weren't Cc'd on this
series though. I added them for this reply.

Perhaps I should just send a patch to the dtc and kernel include paths
which implements that, and see what people say.

> diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt

>  See below for available PIN_FUNC_ID for imx6q:
> -MX6Q_PAD_SD2_DAT1__USDHC2_DAT1			0

You forgot to add a reference to the header filename to this file.

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Matt Sealey]

On Wed, Feb 20, 2013 at 12:46 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 02/20/2013 12:08 AM, Shawn Guo wrote:
>> This turns the imx pin function number defined by binding document
>> into #define constants in header which can be used in dts and handled
>> by pre-processor to improve the readability of device tree sources.
>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx35-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx35-pinctrl.txt
>
>> -See below for available PIN_FUNC_ID for imx35:
>> -0 MX35_PAD_CAPTURE__GPT_CAPIN1
> ...
>> -951 MX35_PAD_TEST_MODE__TCU_TEST_MODE
>> +Refer to arch/arm/boot/dts/imx35-pinfunc.h for all available imx35 PIN_FUNC_ID.
>
> So that path is specific to the Linux kernel. The DT binding
> documentation isn't supposed to be specific to the Linux kernel.

I have to violently NACK this patchset for this very reason.

Shawn, we've discussed this before!

Please try not to use the device tree to "help" Linux drivers somehow
look up internal Linux data. This means the device tree is not
portable to other operating systems without copying out data from a
GPL source to a non-GPL source if this is required, meaning device
trees ONLY apply to Linux, and as Stephen pointed out, this is bad.
You cannot have ANY entries in the device tree that cannot be
described outside of the device tree or explained away directly in the
binding (i.e. you can get around this by making a PDF of the binding
and pasting that include data in there, but that is obtuse). Having a
binding that maps an arbitrary string (and these strings ARE arbitrary
since "pin" 951 maps to nowhere but a big array).

Please also do not use the device tree to "help" people "read" the
device tree. A device tree is not meant to be human-readable, it's
meant to be machine-parseable. It has the same fundamental concept as
XML - if you read an XML file and are pulling information out of it
with your eyes and brain you are Doing It Wrong. You're supposed to
use an XML parser. However, that does not stop people with the
appropriate talents writing XML files in a text editor using their
knowledge of syntax. We are all programmers here - and board
designers. Nobody else is going to be writing device tree data,
certainly not a secretary or someone who doesn't understand what 3
pairs of hexadecimal values does.

XML has comments, and so do device trees. Preprocessing the device
tree is to convert pin names into values not something anyone who
designs boards will be doing. Most board designers don't even use
Freescale's IOMUX gui tool. Neither will programmers think "pasting in
a huge arbitrary string" is any easier than "pasting in 6 values from
an appropriate reference". Programmers will also add comments, as
already existed, if they need to cross-reference which pin this is
without decoding a number in their heads. These comments get stripped
when it's compiled. Comments are not enemies.

By simply pasting in the 3 pairs into your target device tree, all the
values would be directly referenced in the documentation of the
appropriate processor. They would be internally consistent - i.e. no
data structures (even macros) referenced that end up living outside
the device tree itself.

Here is an idea; write some documentation (in the pin binding if you
like) that essentially looks like the C header, only without the
#define part. Put that directly in the binding as *examples*.
Programmers and board designers doing initial bring-up can use these
as a quick reference that SUPPLEMENTS the information in the CPU
manual.

As examples for developers of device trees it will come in much
handier than having preprocessing go on. In the event that - in very
many circumstances - the default pin configuration in the list from
the binding document is not relevant to your board (if pullups or
pulldowns or logic inverters are present on the PCB rather than
derived in pad settings) then you will end up copying some of them as
reference and pasting it in and modifying the values anyway, so

        MX51_PIN_X__SD3_DAT4 nnnn

In my device tree doesn't work. I will have to intersperse
preprocessor items with real values for the chip making it terrible to
debug device trees, and defeating the preprocessing step. What your
patch will do, implicitly, is encourage people to *ADD* pad settings
as a reference to the binding, which is the mess we got into with the
old macro solution in the first place (when a pin config doesn't
match, you have to add pins to the include and give it a new name..) -
so this ends up in my target board tree as

        MX51_PIN_X__SD3_DAT1 nnnn
        MX51_PIN_X__SD3_DAT2 nnnn
        MX51_PIN_X__SD3_DAT3 nnnn
        aaa bbb ccc xxx yyy zzz /* MX51_PIN_X_SD3_DAT4 with some mod I did */
        MX51_PIN_X__SD3_WP nnnn
        ddd eee fff uuu vvv www /* MX51_PIN_X__SD3_CD logic is on PCB */
        /* und so weiter */

How does preprocessing the tree here help? What you end up with after
preprocessing is 3 pairs of values in any event. Why not just paste
them into the tree directly and use comments?

Also, since the #define NAME_OF_PIN__FUNCTION is arbitrary (only the
part of the name before the double underscore exists in docs, and
sometimes this changes between chip revisions) it helps nobody.

Also, since the pad config value is STILL using the silly SION bit
information, that would have to go away too (please, please, do not
put a magic SION bit in the PAD configuration value. It doesn't go
into this register and means the driver has to mask out the value and
do special work...)

Preprocessing device trees is useful to keep redundant or repeatative
data out of a single device tree (for instance, if a chip has 24 timer
modules all absolutely identical except the address and an interrupt
number, but there are 10 other information items that are identical,
this is a target for preprocessing and expanding a macro). It
shouldn't be used to allow storing "data" (i.e. arbitrary lists or
tables) in any other place than the device tree (besides the processor
documentation shipped by the vendor) as a clever way to "clean up"
trees to make them more "readable".

I am not sure I am getting this point across, but.. damn it.. nack nack nack :D

--
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Stephen Warren]

On 02/20/2013 05:03 PM, Matt Sealey wrote:
> On Wed, Feb 20, 2013 at 12:46 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 02/20/2013 12:08 AM, Shawn Guo wrote:
>>> This turns the imx pin function number defined by binding document
>>> into #define constants in header which can be used in dts and handled
>>> by pre-processor to improve the readability of device tree sources.
>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx35-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx35-pinctrl.txt
>>
>>> -See below for available PIN_FUNC_ID for imx35:
>>> -0 MX35_PAD_CAPTURE__GPT_CAPIN1
>> ...
>>> -951 MX35_PAD_TEST_MODE__TCU_TEST_MODE
>>> +Refer to arch/arm/boot/dts/imx35-pinfunc.h for all available imx35 PIN_FUNC_ID.
>>
>> So that path is specific to the Linux kernel. The DT binding
>> documentation isn't supposed to be specific to the Linux kernel.
> 
> I have to violently NACK this patchset for this very reason.
> 
> Shawn, we've discussed this before!
> 
> Please try not to use the device tree to "help" Linux drivers somehow
> look up internal Linux data. This means the device tree is not
> portable to other operating systems without copying out data from a
> GPL source to a non-GPL source if this is required, meaning device
> trees ONLY apply to Linux, and as Stephen pointed out, this is bad.

I believe my comment was directed somewhere a little different than
yours are.

My comment was simply that the *pathname* of the header file
imx35-pinfunc.h was Linux-specific. I would advocate replacing it with
e.g. <dt-bindings/pinctrl/imx35-pinfunc.h>. My belief is that the header
file is *part* of the binding, and so there will be a tree (currently
Documentation/devicetree/bindings in the kernel's copy) that contains
the *.txt bindings, and another tree (which I'm proposing storing in
include/dt-bindings as the kernel's copy) which contains the header
files, and which is known as the source of include files reference under
<dt-bindings/>.

Re: the license question: What is the license of the DT bindings
documentation? I don't recall having seen any license header in any of
the files, so I'd have to assume they're all GPLv2 since that's the
license of the kernel. Hence, Shawn having licensed his new headers
under the same license explicitly seems entirely consistent.

> You cannot have ANY entries in the device tree that cannot be
> described outside of the device tree or explained away directly in the
> binding

Sure. But note that I explicitly consider the header files as part of
the binding; it's just that for tabular/constant data, it's more
convenient to represent it in a header file than a more free-form table
in the DT binding document itself.

> (i.e. you can get around this by making a PDF of the binding
> and pasting that include data in there, but that is obtuse). Having a
> binding that maps an arbitrary string (and these strings ARE arbitrary
> since "pin" 951 maps to nowhere but a big array).
> 
> Please also do not use the device tree to "help" people "read" the
> device tree. A device tree is not meant to be human-readable, it's
> meant to be machine-parseable.

Just because something is machine parseable doesn't mean it shouldn't
also be human readable. Your argument could be applied to any piece of C
code too; it's meant for consumption by the compiler, so it doesn't
matter if people can read or maintain it?

> It has the same fundamental concept as
> XML - if you read an XML file and are pulling information out of it
> with your eyes and brain you are Doing It Wrong. You're supposed to
> use an XML parser. However, that does not stop people with the
> appropriate talents writing XML files in a text editor using their
> knowledge of syntax. We are all programmers here - and board
> designers. Nobody else is going to be writing device tree data,
> certainly not a secretary or someone who doesn't understand what 3
> pairs of hexadecimal values does.

Understanding is one thing. Nobody is going to argue against that. The
issue is remembering which bits are which, accidental typos that aren't
obvious, etc.

> XML has comments, and so do device trees. Preprocessing the device
> tree is to convert pin names into values not something anyone who
> designs boards will be doing.

You state that as fact, but I disagree.

> Most board designers don't even use
> Freescale's IOMUX gui tool. Neither will programmers think "pasting in
> a huge arbitrary string" is any easier than "pasting in 6 values from
> an appropriate reference".

That's simply not true. I and Shawn are both programmers, and we would
find using a CPP macro name to set up GPIO IDs, IRQ/GPIO flags, etc.
much easier than referring to a DT binding document, manually or'ing
some bits together, and writing the result into the .dts file. It's
simply error-prone to do this manually, and unreadable after it's done.

> Programmers will also add comments, as
> already existed, if they need to cross-reference which pin this is
> without decoding a number in their heads. These comments get stripped
> when it's compiled. Comments are not enemies.

Comments aren't compiled. So, it's easy to make a typo and get a comment
right but the integer value they comment on wrong. By using
defines/macros, the values *are* comments, just in a human-readable form.

> By simply pasting in the 3 pairs into your target device tree, all the
> values would be directly referenced in the documentation of the
> appropriate processor. They would be internally consistent - i.e. no
> data structures (even macros) referenced that end up living outside
> the device tree itself.
> 
> Here is an idea; write some documentation (in the pin binding if you
> like) that essentially looks like the C header, only without the
> #define part. Put that directly in the binding as *examples*.
> Programmers and board designers doing initial bring-up can use these
> as a quick reference that SUPPLEMENTS the information in the CPU
> manual.
> 
> As examples for developers of device trees it will come in much
> handier than having preprocessing go on. In the event that - in very
> many circumstances - the default pin configuration in the list from
> the binding document is not relevant to your board (if pullups or
> pulldowns or logic inverters are present on the PCB rather than
> derived in pad settings) then you will end up copying some of them as
> reference and pasting it in and modifying the values anyway, so
> 
>         MX51_PIN_X__SD3_DAT4 nnnn
> 
> In my device tree doesn't work. I will have to intersperse
> preprocessor items with real values for the chip making it terrible to
> debug device trees, and defeating the preprocessing step. What your
> patch will do, implicitly, is encourage people to *ADD* pad settings
> as a reference to the binding, which is the mess we got into with the
> old macro solution in the first place (when a pin config doesn't
> match, you have to add pins to the include and give it a new name..) -
> so this ends up in my target board tree as
> 
>         MX51_PIN_X__SD3_DAT1 nnnn
>         MX51_PIN_X__SD3_DAT2 nnnn
>         MX51_PIN_X__SD3_DAT3 nnnn
>         aaa bbb ccc xxx yyy zzz /* MX51_PIN_X_SD3_DAT4 with some mod I did */
>         MX51_PIN_X__SD3_WP nnnn
>         ddd eee fff uuu vvv www /* MX51_PIN_X__SD3_CD logic is on PCB */
>         /* und so weiter */
> 
> How does preprocessing the tree here help? What you end up with after
> preprocessing is 3 pairs of values in any event. Why not just paste
> them into the tree directly and use comments?

Sorry, I'm having a hard time understanding that. The header should
already contain all defined fields/values, so there wouldn't be a need
to add more to it. If something ended up missing through oversight, it'd
be entirely appropriate to add it. I can't see why you'd end up with a
mix of defines and literals.

> Also, since the #define NAME_OF_PIN__FUNCTION is arbitrary (only the
> part of the name before the double underscore exists in docs, and
> sometimes this changes between chip revisions) it helps nobody.

The value is arbitrary.

The concept of selecting a specific pinmux function on a particular pin
is certainly something that exists in HW. It's the very purpose of a
pinmux IP block, and there are explicitly register fields for it.

The fact that the IMX binding has a single integer that represents both
the pin ID and the function muxed onto it, rather than separately
representing the pin ID and the function as separate cells/fields does
admittedly seem rather odd to me. However, that fact already exists now,
before this patch, and so isn't anything to do with this patch.

> Preprocessing device trees is useful to keep redundant or repeatative
> data out of a single device tree (for instance, if a chip has 24 timer
> modules all absolutely identical except the address and an interrupt
> number, but there are 10 other information items that are identical,
> this is a target for preprocessing and expanding a macro). It
> shouldn't be used to allow storing "data" (i.e. arbitrary lists or
> tables) in any other place than the device tree (besides the processor
> documentation shipped by the vendor) as a clever way to "clean up"
> trees to make them more "readable".
> 
> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D

Well, I certainly understand you don't want to use defines instead of
literals, but the simple fact is that many people writing device trees
/do/ very explicitly want that.

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Shawn Guo]

On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
> 
Do you see any downgrade side that the series introduces over the
existing implementation?

Sorry, I do not really understand your nack.

Shawn

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Matt Sealey]

On Wed, Feb 20, 2013 at 11:02 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
>> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
>>
> Do you see any downgrade side that the series introduces over the
> existing implementation?

Because it replaces the horribly stupid existing implementation with
something that doesn't solve the fundamental logical problems caused
by the existing implementation. There are three completely obvious
logical inconsistencies with the existing implementation of a pair of
<arbitrary_pin_index pad_mode>;

1) the pin index is completely arbitrary and in any binding every
published for any of these SoC, either broken by design (MX5 and MX6
have duplicated pin data soaking up arbitrary pin ids in the current
binding, or are not defined in SoC register order (i.e. arbitrary
renumbering).

2) the pin index is not internally consistent within the device tree
binding - it is a reference to an array inside source code. Your patch
hopes to solve this, but also hopes to solve other things too. It
fails on the second part.

3) the pad setting value has been hijacked to include bits that
otherwise would not be set in the same register (SION bit and a
special flag to mean, set up everything except the pad settings)

What you've fixed it to do, as I read this patch, is this;

<arbitrary_pin_name pad_mode>

which the preprocessor expands to;

<reg_mux mux_mode reg_insel insel_mode reg_pad pad_mode>

The real problem here is as follows;

1) the pin name and function name are still completely arbitrary (in
so far as the old iomux.h macros way, the newer iomux-v3.h way, the
current bindings and the new binding you're pushing do not match the
CPU documentation at all)

2) copy pasting a line of 5 values from an example document and adding
your pad mode bits to the end is no more time consuming or
space-consuming than copying a 38-character macro name. New board
designs will use data from the FSL IOMUX tool or other sources and not
bother using macro definitions to define pads.

3) macros can be wrong and they will inherit into every device tree,
breaking every board that uses them. That said, so can the examples,
but at least reading the device tree for a board you can cross-check
all the values against, say, the output of many tools provided by
Freescale or existing code or platform support without doing a back
reference or preprocessing the device tree and removing comments).

I am a big fan of a device tree, in and of itself, being internally
consistent (I keep using that phrase, and I mean it). That means not
only does it not try and reference any magic outside data inside some
other code (e.g. table indices in arbitrary packing and format) but it
also does not cause a chain of cross-referencing where values are
hidden behind other values. To confirm device tree pinctrl settings
with this patch I need to preprocess my device tree - and then go over
the output with all my comments stripped out to look at the arrays of
numbers the macros produce. I have therefore five references to look
at: my original device tree (unprocessed, commented), the macro
definitions, the preprocessed output (macros expanded, comments
stripped by preprocessor), and the actual CPU documentation and board
schematics/design data that the values are all derived from.

Why would anyone here need anything more than the device tree as
unprocessed data with comments, and the CPU documentation and board
data defining the function? We cut here 5 down to 3. You cannot do
without these 3 - you cannot magically "guess" that a pad setting will
work just because a macro has the correct name.

In the event a macro does not exist, people will end up either adding
that macro (therefore changing the binding constantly, since the
macros are part of the binding) or putting in the raw values to avoid
the preprocessor. There's a really good reason standards documents
don't change that often; standards need to be frozen and we're not
talking here about freezing standards, but allowing opportunities for
constant, "agile" development of the board boot process and driver
data passing.

I would rather that we go think of a better solution here. We're using
a preprocessor and just using it to expand a name into another value -
why don't we actually use macros to ease the coding of the device tree
entries? That way data isn't moved elsewhere, but the macro can easily
check ranges and report errors in values.. for instance, instead of

MX51_PIN_EIM_23__GPIO1_2 0x6528

<
MUX_REG(0x174) ALT(0x5|SION) INS_REG(0x3d8) INS(0x7) PAD_REG(0x654)
PAD(0x7633|PU_100K), /* MX51_EIM_23 GPIO1_2 */
>;

With the ability to expand the bits required - adding SION ot the ALT
mode, using macros to define the pin bits instead of an opaque value.
Stick a comment after it and it's clear the definition. Copy paste
that into a tree and preprocess it and it becomes the same 6 values,
but in the tree itself it is clear which registers are set (you can
search the docs for these values, where the semi-arbitrary pin naming
scheme in the binding does NOT have a *clearly* searchable part) and
the macro can confirm you didn't put a weird value in there (in this
case, all the registers are in certain ranges and all of them are
32-bit aligned so some extra parsing can be done).

Or even one big macro - IOMUXV3(MX51_EIM_23_GPIO__1_2, 0x174,
0x5|SION, 0x3d8, 0x7, 0x654, 0x7633|PU_100K),

Just discard the first argument, validate the rest against masks etc.
and alignment, and off you go. No information is lost internally to
the tree.

This way we get a little closer back to the original FSL BSP method of
defining iomux and the values above are directly copyable from the
iomux tool, existing source code, bootloaders, other operating systems
(licensing notwithstanding) and hand-written scribbles by the board
designers. There was nothing wrong with this, but there has been some
significant refactoring going on that does not make functionality any
better and has only been done with a view to reduce the precompiled
file size of some arbitrary file - because a descriptive string is
seemingly "better" than the actual values. By reducing the amount of
real data actually contained in the source tree, you make the source
code harder to verify. By increasing the amount of needless
cross-referencing and obfuscation of the real data, you increase the
difficulty of actually creating device trees. This is especially true
since none of the CPU docs or board design data will use any of the
semantics of the "Linux" side of the binding - but they WILL use
register values and ball grids and data from IBIS models.

> Sorry, I do not really understand your nack.

Simple; I've been designing with device trees since before the
arch/ppc->arch/powerpc move, with real OpenFirmware, with a good
understanding of the original specifications and bindings, the intents
and the correct ways of doing things. I figure out board designs,
requirements and then do the software on prototype boards and final
consumer-facing and industrial designs.

Every incremental improvement to the iomux definition model has made
it harder to use; to the point that without keeping track of this list
and then going through and comparing the models it is very, very
difficult to go from older code to new device trees. I've set the task
of porting one board or another to a modern device tree and the
engineers end up asking what the hell is going on with the new method.
That means I have to figure it out myself and train them; and every
time, it gets more and more difficult to explain WHY it's being done
the way it's currently done in lieu of the much easier methods in the
past.

If we go from my point of view - and I take a somewhat "holistic"
approach to product design - what we're doing here is increasing time
to market because I can't use existing tools or existing code to
manage the bring-up of a new design. Unfortunately, in my experience,
there isn't a board engineer on the planet who designs his board from
the firmware binding upwards, and certainly not by "how Linux wants to
do it."

In the "fight" between the best hardware design and bringing up the
software, it pains me to have to add more work to the schedule.

Preprocessing trees is a really nice functional improvement, but this
patch only serves to change nothing (by obfuscating the real data
*behind* a binding) or increase work for programmers taking design
data into a tree. It may look like since there is "less data" in the
tree it is easier to check, but that is not the case and has never
been the case. Also, the data in the binding sometimes does not match
the board design because the current bindings are entirely derived
from existing boards - some of which are not implementing current
design methodologies with the appropriate SoCs or using errata
workarounds for hardware components, which means all these pin
definitions may not apply - do we add a new macro to the binding to
support it or just paste in 6 values for the same effect? How does
that make the device trees look? Ugly? Yes. But they're not meant to
be pretty. Having an aligned list of exactly-the-same-length strings
in a column gains you NOTHING above using the real values and adding a
comment.

It is not the purpose of the device tree concept to "make your driver
code smaller" by encoding information that is easily attainable at
runtime. The tree needs to provide enough information, however, to
make that information easily attainable. That stipulation is also true
of the readability of device trees - just as XML developers do not
read data out by hand, but use parsers, so do we do with device trees.
Just as XML developers hand-code their XML files sometimes, we
sometimes hand-code our device trees. But most XML processing is
machine-driven and translating data from one format to another, to and
from XML. There is always input data, though. That input data must be
clearly defined. The less easy it is to read a device tree entry and
figure out what the crap it was for, the harder it is to verify it's
correctness. Moving values into the preprocessor stage does nothing
except turn an array of seemingly obtuse, uncommented values into a
string literal. I say, use the obtuse values and comment them in your
trees. Use the preprocessor for VALIDATION of those values (or
expansion of offset into absolute address for example), but not for
generation. You are adding too many steps between writing the tree and
the final binary output.

If we do have full C preprocessor support for a device tree now then
we should use the preprocessor to it's fullest, rather than using it
as an overcomplicated replacement for "sed" or "awk".

--
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Matt Sealey]

I feel like I should be more concise, so here are what I would
consider MY goals for iomux definition on Freescale processors;

Freescale provide spreadsheets, GUI tools and XML and C header output
for the iomux model.

If you can't copy it right from the documentation or a script or
program (C, Python, whatever) any of the above into a device tree
without first encoding or parsing the binding in a suitable format for
the program, then the binding is not desirable - for me, at least. If
we can't go back to the original data from BSP kernels and port it to
new device trees without a 3-step cross-reference and a preprocessing
step for verification, you are adding items to my in tray...

Usually it's spit out from the IOMUX tool as XML and example C source
(with certain definitions) with header files for the chip so you can
drop it right into your bringup tool, bootloader, OS or whatever.
Using that as a reference worked fine - it's register offsets
(absolute from IOMUX tool), values. Copy pasting from the spreadsheet
worked fine. Printing out the docs page and marking the bits worked
fine. You're now telling me with this patchset that I have to have
another documentation source and additionally verify that it even
applies to the board by comparing it to all of the above.

Therefore, nack :D

Either use the preprocessor to really verify the data, but don't use
it just to turn 5 values into a string literal.

-- 
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Sascha Hauer]

On Thu, Feb 21, 2013 at 11:36:36AM -0600, Matt Sealey wrote:
> On Wed, Feb 20, 2013 at 11:02 PM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
> >> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
> >>
> > Do you see any downgrade side that the series introduces over the
> > existing implementation?
> 
> Because it replaces the horribly stupid existing implementation with
> something that doesn't solve the fundamental logical problems caused
> by the existing implementation. There are three completely obvious
> logical inconsistencies with the existing implementation of a pair of
> <arbitrary_pin_index pad_mode>;
> 
> 1) the pin index is completely arbitrary and in any binding every
> published for any of these SoC, either broken by design (MX5 and MX6
> have duplicated pin data soaking up arbitrary pin ids in the current
> binding, or are not defined in SoC register order (i.e. arbitrary
> renumbering).

The pin index is indeed completely arbitrary, mostly because the iomuxv3
does not make it possible to number pins. More sane SoCs offered that,
there a Pin number could be easily translated into a register offset. I
don't know what drugs the FSL engineers have taken to come up with a
register layout in which each pin is configured via three register sets
which do not stand in any relationship to each other.

Unfortunately the pinctrl framework forces us to use a pin index as it
needs something to detect resource conflicts. BUT, I think I agree here
with you: This pin number should not be exposed to the devicetree, it
only adds a level of indirection.

> 
> 2) the pin index is not internally consistent within the device tree
> binding - it is a reference to an array inside source code. Your patch
> hopes to solve this, but also hopes to solve other things too. It
> fails on the second part.
> 
> 3) the pad setting value has been hijacked to include bits that
> otherwise would not be set in the same register (SION bit and a
> special flag to mean, set up everything except the pad settings)
> 
> What you've fixed it to do, as I read this patch, is this;
> 
> <arbitrary_pin_name pad_mode>
> 
> which the preprocessor expands to;
> 
> <reg_mux mux_mode reg_insel insel_mode reg_pad pad_mode>
> 
> The real problem here is as follows;
> 
> 1) the pin name and function name are still completely arbitrary (in
> so far as the old iomux.h macros way, the newer iomux-v3.h way, the
> current bindings and the new binding you're pushing do not match the
> CPU documentation at all)
> 
> 2) copy pasting a line of 5 values from an example document and adding
> your pad mode bits to the end is no more time consuming or
> space-consuming than copying a 38-character macro name. New board
> designs will use data from the FSL IOMUX tool or other sources and not
> bother using macro definitions to define pads.
> 
> 3) macros can be wrong and they will inherit into every device tree,
> breaking every board that uses them. That said, so can the examples,
> but at least reading the device tree for a board you can cross-check
> all the values against, say, the output of many tools provided by
> Freescale or existing code or platform support without doing a back
> reference or preprocessing the device tree and removing comments).
>

Yes, macros can be wrong, but they tend to get fixed. Duplicated
information almost never gets fixed in all instances they occur.

> 
> MX51_PIN_EIM_23__GPIO1_2 0x6528
> 
> <
> MUX_REG(0x174) ALT(0x5|SION) INS_REG(0x3d8) INS(0x7) PAD_REG(0x654)
> PAD(0x7633|PU_100K), /* MX51_EIM_23 GPIO1_2 */
> >;

This is basically what the current iomux header do, only that we added
the mux/padctrl/selinput register triplet to a single macro. This in my
eyes still makes sense, because digging through the datasheet guessing
that for a single pin I have to configure registers 0x174, 0x3d8 and
0x654 is cumbersome.

Where it became ugly is the predefined values for pullup/dse.

Yes, I would really like to have something like you describe above in
the devicetree together with the register triplets as handy macros.
Also I still like the <pinname>__<function> naming since that's another
thing I don't have to look up in the datasheet everytime.
If your data comes from the iomux tool then you simply wouldn't use the
register triplet macros.

This would leave the question how we make up a pin number for the
pinctrl framework, but as said, this should be solved inside the kernel
and not pushed into the devicetree.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Shawn Guo]

On Thu, Feb 21, 2013 at 10:43:03PM +0100, Sascha Hauer wrote:
> This would leave the question how we make up a pin number for the
> pinctrl framework, but as said, this should be solved inside the kernel
> and not pushed into the devicetree.
> 
Ok, Dong has the same opinion to remove pin number from device tree,
I start working it out, and will roll out the new version.

Shawn

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Shawn Guo]

On Thu, Feb 21, 2013 at 11:36:36AM -0600, Matt Sealey wrote:
> On Wed, Feb 20, 2013 at 11:02 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
> >> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
> >>
> > Do you see any downgrade side that the series introduces over the
> > existing implementation?
> 
> Because it replaces the horribly stupid existing implementation with
> something that doesn't solve the fundamental logical problems caused
> by the existing implementation.

When did I say that the series is targeting to solve those "fundamental
logical problems" in *your* view?

...

> What you've fixed it to do, as I read this patch, is this;
> 
> <arbitrary_pin_name pad_mode>
> 
No, it's not arbitrary_pin_name.  It's pin function name which comes
from hardware manual.  It may not exactly match the public reference
manual, but they are obvious to be identified.  For imx6q pad SD2_DAT1
example, the manual says:

Select 1 of 6 iomux modes to be used for pad: SD2_DAT1.
  000 ALT0 — Select signal SD2_DATA1.
  001 ALT1 — Select signal ECSPI5_SS0.
  010 ALT2 — Select signal EIM_CS2.
  011 ALT3 — Select signal AUD4_TXFS.
  100 ALT4 — Select signal KEY_COL7.
  101 ALT5 — Select signal GPIO1_IO14.

And imx6q-pinfunc.h gives:

  MX6Q_PAD_SD2_DAT1__USDHC2_DAT1
  MX6Q_PAD_SD2_DAT1__ECSPI5_SS0
  MX6Q_PAD_SD2_DAT1__WEIM_WEIM_CS_2
  MX6Q_PAD_SD2_DAT1__AUDMUX_AUD4_TXFS
  MX6Q_PAD_SD2_DAT1__KPP_COL_7
  MX6Q_PAD_SD2_DAT1__GPIO_1_14

Trust me, no one would manually and arbitrarily write down those huge
number of names.  It comes from Freescale BSP code which is in turn
retrieved from an early design database during BSP pre-coding phase.
It happens all the time that the names in the final document change
than early developing one.

> which the preprocessor expands to;
> 
> <reg_mux mux_mode reg_insel insel_mode reg_pad pad_mode>
> 
> The real problem here is as follows;
> 
> 1) the pin name and function name are still completely arbitrary (in
> so far as the old iomux.h macros way, the newer iomux-v3.h way, the
> current bindings and the new binding you're pushing do not match the
> CPU documentation at all)
> 
No, it's not true, as explained above.

> 2) copy pasting a line of 5 values from an example document and adding
> your pad mode bits to the end is no more time consuming or
> space-consuming than copying a 38-character macro name.

DTB is meant to be machine-parseable, but DTS is meant to be
human-readable because it's edited by human-being, and macro name
is obviously more friendly here.

...

> 3) macros can be wrong and they will inherit into every device tree,
> breaking every board that uses them.

The macros should eventually come from some tool/program and design
database.  It should stand little chance to be wrong.  Even there is
something wrong with the macros, it should be noticed from the very
beginning, exactly because it will get any board device tree using
the macros wrong.

With the help of macros, patch #3 changes the PIN_FUNC_ID from an
integer to a tuple of integers without touching any DTS files at all.

Shawn

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Sascha Hauer]

On Fri, Feb 22, 2013 at 01:52:04PM +0800, Shawn Guo wrote:
> On Thu, Feb 21, 2013 at 11:36:36AM -0600, Matt Sealey wrote:
> > On Wed, Feb 20, 2013 at 11:02 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > > On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
> > >> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
> > >>
> > > Do you see any downgrade side that the series introduces over the
> > > existing implementation?
> > 
> > Because it replaces the horribly stupid existing implementation with
> > something that doesn't solve the fundamental logical problems caused
> > by the existing implementation.
> 
> When did I say that the series is targeting to solve those "fundamental
> logical problems" in *your* view?
> 
> ...
> 
> > What you've fixed it to do, as I read this patch, is this;
> > 
> > <arbitrary_pin_name pad_mode>
> > 
> No, it's not arbitrary_pin_name.  It's pin function name which comes
> from hardware manual.  It may not exactly match the public reference
> manual, but they are obvious to be identified.  For imx6q pad SD2_DAT1
> example, the manual says:
> 
> Select 1 of 6 iomux modes to be used for pad: SD2_DAT1.
>   000 ALT0 — Select signal SD2_DATA1.
>   001 ALT1 — Select signal ECSPI5_SS0.
>   010 ALT2 — Select signal EIM_CS2.
>   011 ALT3 — Select signal AUD4_TXFS.
>   100 ALT4 — Select signal KEY_COL7.
>   101 ALT5 — Select signal GPIO1_IO14.

What makes them arbitrary is the fact that 110 and 111 are not encoded,
so there is no way to calculate the register number from the pin number.

Also

commit 4a5f7eff8b0b34354e5c63272835e5e2dfe1c933
Author: Dong Aisheng <dong.aisheng@linaro.org>
Date:   Fri Jul 6 17:09:23 2012 +0800

    pinctrl: pinctrl-imx6q: add missed mux function for USBOTG_ID

Shows that they are indeed arbitrary.

I'm really with Matt here when he says that this number doesn't have to
be in the binding.

What we make from this is another story. Changing this will be painful.
It will get even more painful though when the bindings are more
established in the future.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Shawn Guo]

On Fri, Feb 22, 2013 at 08:27:43AM +0100, Sascha Hauer wrote:
> On Fri, Feb 22, 2013 at 01:52:04PM +0800, Shawn Guo wrote:
> > On Thu, Feb 21, 2013 at 11:36:36AM -0600, Matt Sealey wrote:
> > > On Wed, Feb 20, 2013 at 11:02 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
> > > >> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
> > > >>
> > > > Do you see any downgrade side that the series introduces over the
> > > > existing implementation?
> > > 
> > > Because it replaces the horribly stupid existing implementation with
> > > something that doesn't solve the fundamental logical problems caused
> > > by the existing implementation.
> > 
> > When did I say that the series is targeting to solve those "fundamental
> > logical problems" in *your* view?
> > 
> > ...
> > 
> > > What you've fixed it to do, as I read this patch, is this;
> > > 
> > > <arbitrary_pin_name pad_mode>
> > > 
> > No, it's not arbitrary_pin_name.  It's pin function name which comes
> > from hardware manual.  It may not exactly match the public reference
> > manual, but they are obvious to be identified.  For imx6q pad SD2_DAT1
> > example, the manual says:
> > 
> > Select 1 of 6 iomux modes to be used for pad: SD2_DAT1.
> >   000 ALT0 — Select signal SD2_DATA1.
> >   001 ALT1 — Select signal ECSPI5_SS0.
> >   010 ALT2 — Select signal EIM_CS2.
> >   011 ALT3 — Select signal AUD4_TXFS.
> >   100 ALT4 — Select signal KEY_COL7.
> >   101 ALT5 — Select signal GPIO1_IO14.
> 
> What makes them arbitrary is the fact that 110 and 111 are not encoded,
> so there is no way to calculate the register number from the pin number.
> 
> Also
> 
> commit 4a5f7eff8b0b34354e5c63272835e5e2dfe1c933
> Author: Dong Aisheng <dong.aisheng@linaro.org>
> Date:   Fri Jul 6 17:09:23 2012 +0800
> 
>     pinctrl: pinctrl-imx6q: add missed mux function for USBOTG_ID
> 
> Shows that they are indeed arbitrary.
> 
> I'm really with Matt here when he says that this number doesn't have to
> be in the binding.

I think you are still talking about that arbitrary index in the old
binding, while I'm arguing against Matt's the comment on the new one,
saying the macro/pin name is arbitrary.

Shawn

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Sascha Hauer]

On Fri, Feb 22, 2013 at 03:36:32PM +0800, Shawn Guo wrote:
> On Fri, Feb 22, 2013 at 08:27:43AM +0100, Sascha Hauer wrote:
> > On Fri, Feb 22, 2013 at 01:52:04PM +0800, Shawn Guo wrote:
> > > On Thu, Feb 21, 2013 at 11:36:36AM -0600, Matt Sealey wrote:
> > > > On Wed, Feb 20, 2013 at 11:02 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
> > > > >> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
> > > > >>
> > > > > Do you see any downgrade side that the series introduces over the
> > > > > existing implementation?
> > > > 
> > > > Because it replaces the horribly stupid existing implementation with
> > > > something that doesn't solve the fundamental logical problems caused
> > > > by the existing implementation.
> > > 
> > > When did I say that the series is targeting to solve those "fundamental
> > > logical problems" in *your* view?
> > > 
> > > ...
> > > 
> > > > What you've fixed it to do, as I read this patch, is this;
> > > > 
> > > > <arbitrary_pin_name pad_mode>
> > > > 
> > > No, it's not arbitrary_pin_name.  It's pin function name which comes
> > > from hardware manual.  It may not exactly match the public reference
> > > manual, but they are obvious to be identified.  For imx6q pad SD2_DAT1
> > > example, the manual says:
> > > 
> > > Select 1 of 6 iomux modes to be used for pad: SD2_DAT1.
> > >   000 ALT0 — Select signal SD2_DATA1.
> > >   001 ALT1 — Select signal ECSPI5_SS0.
> > >   010 ALT2 — Select signal EIM_CS2.
> > >   011 ALT3 — Select signal AUD4_TXFS.
> > >   100 ALT4 — Select signal KEY_COL7.
> > >   101 ALT5 — Select signal GPIO1_IO14.
> > 
> > What makes them arbitrary is the fact that 110 and 111 are not encoded,
> > so there is no way to calculate the register number from the pin number.
> > 
> > Also
> > 
> > commit 4a5f7eff8b0b34354e5c63272835e5e2dfe1c933
> > Author: Dong Aisheng <dong.aisheng@linaro.org>
> > Date:   Fri Jul 6 17:09:23 2012 +0800
> > 
> >     pinctrl: pinctrl-imx6q: add missed mux function for USBOTG_ID
> > 
> > Shows that they are indeed arbitrary.
> > 
> > I'm really with Matt here when he says that this number doesn't have to
> > be in the binding.
> 
> I think you are still talking about that arbitrary index in the old
> binding, while I'm arguing against Matt's the comment on the new one,
> saying the macro/pin name is arbitrary.

Sorry, I haven't read the above carefully enough. We are talking about
the names here, not the numbers I referred to.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Matt Sealey]

On Thu, Feb 21, 2013 at 11:52 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Thu, Feb 21, 2013 at 11:36:36AM -0600, Matt Sealey wrote:
>> On Wed, Feb 20, 2013 at 11:02 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> > On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
>> >> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
>> >>
>> > Do you see any downgrade side that the series introduces over the
>> > existing implementation?
>>
>> Because it replaces the horribly stupid existing implementation with
>> something that doesn't solve the fundamental logical problems caused
>> by the existing implementation.
>
> When did I say that the series is targeting to solve those "fundamental
> logical problems" in *your* view?
>
> ...

Why would you submit a series that doesn't solve such problems? :)

To replace an illogical, ridiculous system of arbitrary pin numbering
with an illogical mapping of registers to arbitrary names is..
illogical.

>> What you've fixed it to do, as I read this patch, is this;
>>
>> <arbitrary_pin_name pad_mode>
>>
> No, it's not arbitrary_pin_name.  It's pin function name which comes
> from hardware manual.  It may not exactly match the public reference
> manual, but they are obvious to be identified.  For imx6q pad SD2_DAT1
> example, the manual says:
>
> Select 1 of 6 iomux modes to be used for pad: SD2_DAT1.
>   000 ALT0 — Select signal SD2_DATA1.
>   001 ALT1 — Select signal ECSPI5_SS0.
>   010 ALT2 — Select signal EIM_CS2.
>   011 ALT3 — Select signal AUD4_TXFS.
>   100 ALT4 — Select signal KEY_COL7.
>   101 ALT5 — Select signal GPIO1_IO14.
>
> And imx6q-pinfunc.h gives:
>
>   MX6Q_PAD_SD2_DAT1__USDHC2_DAT1
>   MX6Q_PAD_SD2_DAT1__ECSPI5_SS0
>   MX6Q_PAD_SD2_DAT1__WEIM_WEIM_CS_2
>   MX6Q_PAD_SD2_DAT1__AUDMUX_AUD4_TXFS
>   MX6Q_PAD_SD2_DAT1__KPP_COL_7
>   MX6Q_PAD_SD2_DAT1__GPIO_1_14

So, SD2_DAT1 with function SD2_DAT1 in the manual is SD2_DAT1__USDHC2_DAT1 ?

KEY_COL7 -> KPP_COL_7?
EIM_CS2 -> WEIM_WEIM_CS_2?
AUD4_TXFS -> AUDMUX_AUD4_TXFS?
GPIO1_IO14 -> GPIO_1_14?

This is what I call arbitrary.

Since the documented NAMING of pins changes between SoC revisions
(where pins change definition or group), manual revisions (where
people fix definitions), and even existing source code, it makes no
sense whatsoever to give things like this a name. Especially as it is
possible to break out the pin definitions and see that your "use a
macro to encode 5 values at once and leave the most-often changing
value to the device tree" method misses one thing: it is not easy to
set the SION bit in the ALT register for each of these without
replacing the macro. Sometimes SION is important for debugging,
sometimes it is not... some pins need SION *forever* and other times..
not. On i.MX6 using RMII ethernet, you need SION to loopback the clock
input to the MAC.. but it's possible you'd want to temporarily disable
this in a build. The only way here is to replace the pin definition
macro part with the actual register values, set your SION bit, and
rebuild the device tree.

The only canonical thing you can guarantee is that in general the
registers fit within certain ranges (although on i.MX51 the ranges did
change between revisions, this is another problem), and accept certain
values.

The only reasonable thing we can be doing with pre-processing IOMUX
data is make sure it fits those ranges (within reason) and possibly
use the preprocessing to ensure that invalid values never make it into
a device tree (i.e. there are bits in the pad settings which are
never, ever set on a particular SoC, and masking and bailing out using
#error or pitching a #warning would be much more useful than allowing
random bits to go into register areas that are "reserved".

I half-agree with Sascha. I am responsible for porting the i.MX51
iomux-v3.h stuff to U-Boot because using the naming scheme worked so
much better there than mxc_request_iomux clutter since most boards
came with almost exactly the same settings on exactly the same buses
because it was a limited choice on i.MX51. This MOSTLY works but there
are several - if not a ridiculous number - of entries which needed to
be modified for any particular board. And the remit came from the
U-Boot guys simply that we should not commit into the tree any pins
which no board makes use of.

Here, we have potential for a huge file full of macros of which a good
deal of them will end up with NOBODY using, and therefore just clutter
the file. If we go with the U-Boot model, we only add macros to define
a pin configuration set if at least ONE board requires that
definition. This will keep it nice and clean and make sure nobody
commits huge swaths of macros which nobody is testing.

Since I did that work, I've changed my mind; actually naming the pins
by their intended function simply doesn't ADD anything to the device
tree functionality, nor does it really help Sascha out any more than
simply having a *really good reference* would help him out.

For example, please explain to me why;

MX51_PIN_EIM_23__GPIO1_2 0x6528

is clearer than;

0x174 0x5 0x3d8 0x7 0x654 0x7633, /* EIM_23 GPIO1_2 and some pullup
settings or so comment comment comment */

In the processed device tree all you're going to see is the numbers.
No pin naming at all. No comments. They certainly don't appear in the
blob. Looking at the device tree in /proc/device-tree or processed
output before compiling, the magic macro information is COMPLETELY
lost, which means all it does is add an arbitrary binding of a name to
a set of values in the raw, unprocessed source - just to "explain"
what that pin does.

How does a comment not do this already in the raw, unprocessed source?

How does giving it a macro name help people looking at trees that have
gone through processing?

Answer: it doesn't. We're adding work (preprocessing) for no
significant benefit except to stop a raw, source code device tree from
being a clutter of numbers. Since it ends up as a clutter of numbers
anyway at runtime, adding cross-referencing and potential for
typographical errors at all stages plus any possible automation
errors..

>> 2) copy pasting a line of 5 values from an example document and adding
>> your pad mode bits to the end is no more time consuming or
>> space-consuming than copying a 38-character macro name.
>
> DTB is meant to be machine-parseable, but DTS is meant to be
> human-readable because it's edited by human-being, and macro name
> is obviously more friendly here.

Last I checked, I can read numbers just fine, I can even convert hex
to decimal in my head; when I feel sleepy and not quite up to it, I
have a calculator. I can also paste the numbers directly into my PDF
reader to find the exact register definition within the IOMUX chapter
- instant lookup. Looking at the DT, then looking at the macro header,
then searching for the PDF is an extra step. To me, it is not obvious
that this is "more friendly," simply because it is adding a level of
indirection.

>> 3) macros can be wrong and they will inherit into every device tree,
>> breaking every board that uses them.
>
> The macros should eventually come from some tool/program and design
> database.  It should stand little chance to be wrong.  Even there is
> something wrong with the macros, it should be noticed from the very
> beginning, exactly because it will get any board device tree using
> the macros wrong.

Your examples seem to have mismatching database revisions, then?

> With the help of macros, patch #3 changes the PIN_FUNC_ID from an
> integer to a tuple of integers without touching any DTS files at all.

Understood, but I don't see the benefit of converting "98" to
"MX51_PIN_EIM_23__GPIO_1_2" when eventually it gets converted into a
string of 5 numbers which are in the manual. At least how the benefit
outweighs a comment in the DTS if there really needs to be some
explanation of the fields (these already exist in the current trees to
explain what the numbers mean in the first place).

Especially as nobody can agree what the canonical pin naming should be
despite several (I count 6) implementations of naming the pins, and
that in several cases the values returned by the macros may not be
EXACTLY what you wanted (SION bit is the biggest example), it still
maps an arbitrary value (number in the old version, string literal
macro name in the series you sent).

If someone gets the macro wrong - a typo or missed pin - or needs to
continually add new macros for new boards - this is going to create an
inordinate amount of churn in the macro file and therefore in a
binding. God forbid if the binding has to change. Please don't submit
a file with every pin you THINK is in the i.MX5/6 and think it is
done, because we know already from the patch Sascha referenced..
people miss things and get things wrong.

If the binding is simply "an array of 6 values per pin configuration,
please see the manual for the register offsets and bit definitions"
then the ONLY error can be in the authorship of the device tree source
code and the onus is on.. that guy. Not the "Linux source code being
wrong" or "the binding being out of date". You cannot by any means
introduce automation errors or mistakes in a binding this way.
Bindings - and any preprocessor macros which they depend on - should
be fixed. Not constantly fluctuating at the time someone finds a
mistake, or on the assumption that someone will be cross-checking
every single value in a huge list of pins in a binding where it is
possible it cannot be tested on a real board.

--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Sascha Hauer]

On Wed, Feb 27, 2013 at 12:51:56AM -0600, Matt Sealey wrote:
> On Thu, Feb 21, 2013 at 11:52 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Thu, Feb 21, 2013 at 11:36:36AM -0600, Matt Sealey wrote:
> >> On Wed, Feb 20, 2013 at 11:02 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> >> > On Wed, Feb 20, 2013 at 06:03:39PM -0600, Matt Sealey wrote:
> >> >> I am not sure I am getting this point across, but.. damn it.. nack nack nack :D
> >> >>
> >> > Do you see any downgrade side that the series introduces over the
> >> > existing implementation?
> >>
> >> Because it replaces the horribly stupid existing implementation with
> >> something that doesn't solve the fundamental logical problems caused
> >> by the existing implementation.
> >
> > When did I say that the series is targeting to solve those "fundamental
> > logical problems" in *your* view?
> >
> > ...
> 
> Why would you submit a series that doesn't solve such problems? :)
> 
> To replace an illogical, ridiculous system of arbitrary pin numbering
> with an illogical mapping of registers to arbitrary names is..
> illogical.
> 
> >> What you've fixed it to do, as I read this patch, is this;
> >>
> >> <arbitrary_pin_name pad_mode>
> >>
> > No, it's not arbitrary_pin_name.  It's pin function name which comes
> > from hardware manual.  It may not exactly match the public reference
> > manual, but they are obvious to be identified.  For imx6q pad SD2_DAT1
> > example, the manual says:
> >
> > Select 1 of 6 iomux modes to be used for pad: SD2_DAT1.
> >   000 ALT0 — Select signal SD2_DATA1.
> >   001 ALT1 — Select signal ECSPI5_SS0.
> >   010 ALT2 — Select signal EIM_CS2.
> >   011 ALT3 — Select signal AUD4_TXFS.
> >   100 ALT4 — Select signal KEY_COL7.
> >   101 ALT5 — Select signal GPIO1_IO14.
> >
> > And imx6q-pinfunc.h gives:
> >
> >   MX6Q_PAD_SD2_DAT1__USDHC2_DAT1
> >   MX6Q_PAD_SD2_DAT1__ECSPI5_SS0
> >   MX6Q_PAD_SD2_DAT1__WEIM_WEIM_CS_2
> >   MX6Q_PAD_SD2_DAT1__AUDMUX_AUD4_TXFS
> >   MX6Q_PAD_SD2_DAT1__KPP_COL_7
> >   MX6Q_PAD_SD2_DAT1__GPIO_1_14
> 
> So, SD2_DAT1 with function SD2_DAT1 in the manual is SD2_DAT1__USDHC2_DAT1 ?
> 
> KEY_COL7 -> KPP_COL_7?
> EIM_CS2 -> WEIM_WEIM_CS_2?
> AUD4_TXFS -> AUDMUX_AUD4_TXFS?
> GPIO1_IO14 -> GPIO_1_14?
> 
> This is what I call arbitrary.
> 
> Since the documented NAMING of pins changes between SoC revisions
> (where pins change definition or group), manual revisions (where
> people fix definitions), and even existing source code, it makes no
> sense whatsoever to give things like this a name. Especially as it is
> possible to break out the pin definitions and see that your "use a
> macro to encode 5 values at once and leave the most-often changing
> value to the device tree" method misses one thing: it is not easy to
> set the SION bit in the ALT register for each of these without
> replacing the macro. Sometimes SION is important for debugging,
> sometimes it is not... some pins need SION *forever* and other times..
> not. On i.MX6 using RMII ethernet, you need SION to loopback the clock
> input to the MAC.. but it's possible you'd want to temporarily disable
> this in a build. The only way here is to replace the pin definition
> macro part with the actual register values, set your SION bit, and
> rebuild the device tree.
> 
> The only canonical thing you can guarantee is that in general the
> registers fit within certain ranges (although on i.MX51 the ranges did
> change between revisions, this is another problem), and accept certain
> values.
> 
> The only reasonable thing we can be doing with pre-processing IOMUX
> data is make sure it fits those ranges (within reason) and possibly
> use the preprocessing to ensure that invalid values never make it into
> a device tree (i.e. there are bits in the pad settings which are
> never, ever set on a particular SoC, and masking and bailing out using
> #error or pitching a #warning would be much more useful than allowing
> random bits to go into register areas that are "reserved".
> 
> I half-agree with Sascha. I am responsible for porting the i.MX51
> iomux-v3.h stuff to U-Boot because using the naming scheme worked so
> much better there than mxc_request_iomux clutter since most boards
> came with almost exactly the same settings on exactly the same buses
> because it was a limited choice on i.MX51. This MOSTLY works but there
> are several - if not a ridiculous number - of entries which needed to
> be modified for any particular board. And the remit came from the
> U-Boot guys simply that we should not commit into the tree any pins
> which no board makes use of.
> 
> Here, we have potential for a huge file full of macros of which a good
> deal of them will end up with NOBODY using, and therefore just clutter
> the file. If we go with the U-Boot model, we only add macros to define
> a pin configuration set if at least ONE board requires that
> definition. This will keep it nice and clean and make sure nobody
> commits huge swaths of macros which nobody is testing.

Great, so you end up patching this file over and over again, maintaining
out of boards always require patches to the iomux file, maintainers have
to deal with merge conflicts.

> 
> Since I did that work, I've changed my mind; actually naming the pins
> by their intended function simply doesn't ADD anything to the device
> tree functionality, nor does it really help Sascha out any more than
> simply having a *really good reference* would help him out.
> 
> For example, please explain to me why;
> 
> MX51_PIN_EIM_23__GPIO1_2 0x6528
> 
> is clearer than;
> 
> 0x174 0x5 0x3d8 0x7 0x654 0x7633, /* EIM_23 GPIO1_2 and some pullup
> settings or so comment comment comment */

Look what you have to do to get the pin muxing for a single pin from the
datasheet.

- identify the name, grep for it in the datasheet, find 0x78 for the mux
  register.
- identify the mode wanted, it's ALT1 for GPIO1_2
- Look if it is involved in daisy chain, grep again if it is
- grep again for EIM_D23, find 0x40c for the PAD_CTL register

That's exactly the steps that are annoying, error prone and you can get
rid of by having a macro like we currently have. If you found a bug in
the macro, fix it and everyone benefits.

Also you should notice that with the new patches Shawn has posted nobody
is forced to use these macros. If you don't like them, go and dump the
raw hex numbers into your devicetrees.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Matt Sealey]

On Wed, Feb 27, 2013 at 1:44 AM, Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Wed, Feb 27, 2013 at 12:51:56AM -0600, Matt Sealey wrote:
>> For example, please explain to me why;
>>
>> MX51_PIN_EIM_23__GPIO1_2 0x6528
>>
>> is clearer than;
>>
>> 0x174 0x5 0x3d8 0x7 0x654 0x7633, /* EIM_23 GPIO1_2 and some pullup
>> settings or so comment comment comment */
>
> Look what you have to do to get the pin muxing for a single pin from the
> datasheet.
>
> - identify the name, grep for it in the datasheet, find 0x78 for the mux
>   register.
> - identify the mode wanted, it's ALT1 for GPIO1_2
> - Look if it is involved in daisy chain, grep again if it is
> - grep again for EIM_D23, find 0x40c for the PAD_CTL register
>
> That's exactly the steps that are annoying, error prone and you can get
> rid of by having a macro like we currently have. If you found a bug in
> the macro, fix it and everyone benefits.

And everyone ends up with broken device trees until someone notices.
In my experience, nobody usually notices until a very, very long time
has passed. At least if the register values you're picking doing board
support for your boards will only affect your board if they're wrong
and not be global.

> Also you should notice that with the new patches Shawn has posted nobody
> is forced to use these macros. If you don't like them, go and dump the
> raw hex numbers into your devicetrees.

I do notice that, what I'm a little perturbed by is that it doesn't
seem to improve the situation (the amount of work required to use and
VERIFY the values in each macro - on the assumption that any of them
could be wrong and cross-check them with the schematics, the board
designer if necessary, and then put them in a device tree as known
working. Not all of the data files used in PCB design packages
actually match the docs anyway so there are some cross-checks against
yet another Freescale design database. If I asked someone here to go
find the pin settings for USDHC2 on a board we have, they would first
have to figure out which pad this was coming out of, go to the manual,
find the signals that exit on that pad, look at the IOMUX, cross check
it with some example code and the FSL IOMUX tool... pre-processing and
using macros doesn't make that any easier until we have a single,
fixed, totally verified and unchangeable set of macros which will
cover all the usual cases. If the intent is that we just glob in 2500
pin definitions at the start and "hope" that someone "eventually"
notices any errors.. this is not normal in embedded design.

I think that in the grand scheme of things it will just serve to hide
errors behind a macro..

Shawn, I might be much happier about the patch if I knew for sure that it

a) followed some Freescale design database export of pins vs. alt
modes and default mux settings (this must exist because the manuals
and IOMUX tool data are generated from some XML files) and that
b) that data was available to generate the macros used in the binding.

Assuming the manual is correct is not a bad way to go - sometimes it
is definitely NOT, but the information we're using right now is
derived from there anyway.. it should still follow the names listed
there (i.e. SD2_DAT4__SD2_DAT4 and not SD2_DAT4__USDHC2_DATA4 if the
first one is what can be derived from the manual)

I would also be MUCH happier if macros were also present to support
cross-checking the ranges of iomux registers being defined such that
they can be cross-checked at pre-processing time for those people who
do not want to use the macros verbatim. This way immediate errors
(accidental typo putting a wrong register in a field which is out of
range for the valid addresses for that register block) will be caught
in using the macros.

I guess I will have to capitulate on it since I've got absolutely no
power to lock Shawn in a room until it's done right, but I loathe
automation for automation's sake. There always has to be a human
present and the amount of work he has to do is not "annoying" when it
is looking something up in the manual and coding something to suit.
That's what we all do for a living, for crying out loud. And when a
human is doing some amount of work, making it easier to make a mistake
and creating an assumption that there is an easy way out via
preprocessor macros is counter-productive (someone still has to write
every macro, and someone still has to verify every single one of them
at some point).

-- 
Matt Sealey <matt-sEEEE4iEDtaXzmuOJsdVMQ@public.gmane.org>
Product Development Analyst, Genesi USA, Inc.

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Sascha Hauer]

On Wed, Feb 27, 2013 at 12:16:20PM -0600, Matt Sealey wrote:
> On Wed, Feb 27, 2013 at 1:44 AM, Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > On Wed, Feb 27, 2013 at 12:51:56AM -0600, Matt Sealey wrote:
> 
> I do notice that, what I'm a little perturbed by is that it doesn't
> seem to improve the situation (the amount of work required to use and
> VERIFY the values in each macro - on the assumption that any of them
> could be wrong and cross-check them with the schematics, the board
> designer if necessary, and then put them in a device tree as known
> working. Not all of the data files used in PCB design packages
> actually match the docs anyway so there are some cross-checks against
> yet another Freescale design database. If I asked someone here to go
> find the pin settings for USDHC2 on a board we have, they would first
> have to figure out which pad this was coming out of, go to the manual,
> find the signals that exit on that pad, look at the IOMUX, cross check
> it with some example code and the FSL IOMUX tool... pre-processing and
> using macros doesn't make that any easier until we have a single,
> fixed, totally verified and unchangeable set of macros which will
> cover all the usual cases. If the intent is that we just glob in 2500
> pin definitions at the start and "hope" that someone "eventually"
> notices any errors.. this is not normal in embedded design.

If the macro is unused then who cares about the bugs in it? If on the
other hand it is used, then people will notice the bugs quite fast and
will fix them. If you don't like the macros, then don't use them. If
you have a better idea how the macros should look like, send patches.

Please try and make your point in the amount of text I quoted above,
preferably even shorter, because that's the maximum amount of text I
think most people are willing to read per mail. The shorter you write
the more people will read your mails up to the end and maybe react to
it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Shawn Guo]

On Wed, Feb 27, 2013 at 12:16:20PM -0600, Matt Sealey wrote:
> Shawn, I might be much happier about the patch if I knew for sure that it
> 
> a) followed some Freescale design database export of pins vs. alt
> modes and default mux settings

This is the case today.

> (this must exist because the manuals
> and IOMUX tool data are generated from some XML files) and that

The problem seems to be that the ALT function names are a little
inconsistent between XML files and design database.  I'm checking with
design team right now to see how this happens, and ensure this will
never happen again in the future.

And I will ensure the macro for later SoCs, e.g. imx6dl and imx6sl,
will be consistent with Reference Manual.

> b) that data was available to generate the macros used in the binding.

Ok.  I will check with IOMUX-tool people to see if we can have the data
integrated and exported with the tool.

Shawn

Re: [PATCH 2/3] ARM: dts: imx: replace magic number with pin function name

[Shawn Guo]

On Wed, Feb 20, 2013 at 11:46:41AM -0700, Stephen Warren wrote:
> On 02/20/2013 12:08 AM, Shawn Guo wrote:
> > This turns the imx pin function number defined by binding document
> > into #define constants in header which can be used in dts and handled
> > by pre-processor to improve the readability of device tree sources.
> 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx35-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx35-pinctrl.txt
> 
> > -See below for available PIN_FUNC_ID for imx35:
> > -0 MX35_PAD_CAPTURE__GPT_CAPIN1
> ...
> > -951 MX35_PAD_TEST_MODE__TCU_TEST_MODE
> > +Refer to arch/arm/boot/dts/imx35-pinfunc.h for all available imx35 PIN_FUNC_ID.
> 
> So that path is specific to the Linux kernel.

The only reason for that is device tree sources are currently sitting
in the Linux kernel tree.  I can reword it as below.

Refer to imx35-pinfunc.h in the device tree source folder for all
available imx35 PIN_FUNC_ID.

> The DT binding
> documentation isn't supposed to be specific to the Linux kernel. I
> wonder if we shouldn't put all the header files into
> Documentation/devicetree/bindings rather than arch/*/boot/dts. That way,
> you could encode relative path names into the DT binding docs. The
> headers are logically part of the documentation anyway.
> 
> This all plays into whatever plans exist for removing the DT binding
> docs and/or .dts files from the kernel, and how things will end up
> getting laid out then though. That's something we should discuss with at
> least Grant and Rob. I note that the DT maintainers weren't Cc'd on this
> series though. I added them for this reply.
> 
> Perhaps I should just send a patch to the dtc and kernel include paths
> which implements that, and see what people say.
> 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt
> 
> >  See below for available PIN_FUNC_ID for imx6q:
> > -MX6Q_PAD_SD2_DAT1__USDHC2_DAT1			0
> 
> You forgot to add a reference to the header filename to this file.

Thanks, just added it.

Shawn

Re: [PATCH 3/3] pinctrl: imx: move hard-coding data into device tree

[Stephen Warren]

On 02/20/2013 12:08 AM, Shawn Guo wrote:
> Currently, all imx pinctrl drivers maintain a big array of struct
> imx_pin_reg which hard-codes data like register offset and mux mode
> setting for each pin function.  Every time a new imx SoC support is
> added, we need to add such a big mount of data.  With moving to single
> kernel build, it's only matter of time to be blamed on memory consuming.
> 
> With DTC pre-processor support in place, the patch moves all these data
> into device tree by redefining the PIN_FUNC_ID in imxXX-pinfunc.h and
> changing the PIN_FUNC_ID parsing code a little bit.

There are a couple of potential issues with this patch, which I'm in two
minds about:

1)

This is an incompatible change to the DT binding definition. A DT
written to the old specification won't work with a newer kernel and
vice-versa. This isn't supposed to happen with device tree.

Right now I believe we're still being flexible about DT binding changes,
but I've seen hints that this kind of thing will start getting lots of
push-back in the near future...

That all said, I'd also love to change the Tegra pinctrl binding now
that we have CPP, since I could replace all the strings in the current
binding with integers and presumably save some space in the .dtb. Hence,
I'd love to maintain the flexibility to keep changing the .dts and
kernel code in lock-step.

2)

This patch removes a bunch of tables from the kernel. I like having the
tables in the kernel, since in theory it could allow a future debugfs or
sysfs interface to pinctrl that allows manipulation of the pinctrl HW
state at a semantic level. This is only possible if the DT binding
includes details such as "set this pin to this function", and the driver
includes the tables to convert that into details such as register
address and value. I don't think such an "API" could be implemented for
IMX after this patch. Still, given the IMX binding already merged "pin"
and "function" into a single integer in the binding, I'm not sure if IMX
could support that kind of "API" before anyway?

This is part of the reason I pushed hard for the pinctrl APIs to operate
at the semantic pin/group/function level, and that Tegra's pinctrl
drivers explicitly have tables listing all the pins/groups/functions,
rather than just putting a bunch of register values into the DT.

I'm not sure how much of an issue this is, though. LinusW and/or the DT
maintainers should probably chime in here.

I didn't actually read the driver implementation changes in this patch.

Re: [PATCH 3/3] pinctrl: imx: move hard-coding data into device tree

[Shawn Guo]

On Wed, Feb 20, 2013 at 12:04:58PM -0700, Stephen Warren wrote:
> There are a couple of potential issues with this patch, which I'm in two
> minds about:
> 
> 1)
> 
> This is an incompatible change to the DT binding definition. A DT
> written to the old specification won't work with a newer kernel and
> vice-versa. This isn't supposed to happen with device tree.
> 
> Right now I believe we're still being flexible about DT binding changes,
> but I've seen hints that this kind of thing will start getting lots of
> push-back in the near future...
> 
Last time I heard of the time frame about doing this is when we get
device tree source maintained in a separate repository from the kernel
tree.  I'm not sure how far we're still away from that.  But most of
sub-architectures (including IMX) have not been converted over to
device tree.  And as far as I know, none of IMX based product runs
device tree kernel yet.  So I think at least for IMX we are still
in a phase of being flexible about DT binding changes.

> That all said, I'd also love to change the Tegra pinctrl binding now
> that we have CPP, since I could replace all the strings in the current
> binding with integers and presumably save some space in the .dtb. Hence,
> I'd love to maintain the flexibility to keep changing the .dts and
> kernel code in lock-step.
> 
> 2)
> 
> This patch removes a bunch of tables from the kernel. I like having the
> tables in the kernel, since in theory it could allow a future debugfs or
> sysfs interface to pinctrl that allows manipulation of the pinctrl HW
> state at a semantic level. This is only possible if the DT binding
> includes details such as "set this pin to this function", and the driver
> includes the tables to convert that into details such as register
> address and value. I don't think such an "API" could be implemented for
> IMX after this patch. Still, given the IMX binding already merged "pin"
> and "function" into a single integer in the binding, I'm not sure if IMX
> could support that kind of "API" before anyway?
> 
Right, we do not support that even before the patch.  As the platform
maintainer and user, I do not think this kind of support is so useful
for IMX.

> This is part of the reason I pushed hard for the pinctrl APIs to operate
> at the semantic pin/group/function level, and that Tegra's pinctrl
> drivers explicitly have tables listing all the pins/groups/functions,
> rather than just putting a bunch of register values into the DT.
> 
> I'm not sure how much of an issue this is, though. LinusW and/or the DT
> maintainers should probably chime in here.
> 
IIRC, LinusW said it can be a decision of platform.

Shawn

> I didn't actually read the driver implementation changes in this patch.

Re: [PATCH 3/3] pinctrl: imx: move hard-coding data into device tree

[Sascha Hauer]

On Thu, Feb 21, 2013 at 01:30:22PM +0800, Shawn Guo wrote:
> On Wed, Feb 20, 2013 at 12:04:58PM -0700, Stephen Warren wrote:
> > There are a couple of potential issues with this patch, which I'm in two
> > minds about:
> > 
> > 1)
> > 
> > This is an incompatible change to the DT binding definition. A DT
> > written to the old specification won't work with a newer kernel and
> > vice-versa. This isn't supposed to happen with device tree.
> > 
> > Right now I believe we're still being flexible about DT binding changes,
> > but I've seen hints that this kind of thing will start getting lots of
> > push-back in the near future...
> > 
> Last time I heard of the time frame about doing this is when we get
> device tree source maintained in a separate repository from the kernel
> tree.  I'm not sure how far we're still away from that.  But most of
> sub-architectures (including IMX) have not been converted over to
> device tree.  And as far as I know, none of IMX based product runs
> device tree kernel yet.

The nature of open source makes it hard to know which devices ship out
there, so please don't assume there are none just because you know of
none.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] pinctrl: imx: move hard-coding data into device tree

[Dong Aisheng]

On 21 February 2013 03:04, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 02/20/2013 12:08 AM, Shawn Guo wrote:
>> Currently, all imx pinctrl drivers maintain a big array of struct
>> imx_pin_reg which hard-codes data like register offset and mux mode
>> setting for each pin function.  Every time a new imx SoC support is
>> added, we need to add such a big mount of data.  With moving to single
>> kernel build, it's only matter of time to be blamed on memory consuming.
>>
>> With DTC pre-processor support in place, the patch moves all these data
>> into device tree by redefining the PIN_FUNC_ID in imxXX-pinfunc.h and
>> changing the PIN_FUNC_ID parsing code a little bit.
>
> There are a couple of potential issues with this patch, which I'm in two
> minds about:
>
> 1)
>
> This is an incompatible change to the DT binding definition. A DT
> written to the old specification won't work with a newer kernel and
> vice-versa. This isn't supposed to happen with device tree.
>
> Right now I believe we're still being flexible about DT binding changes,
> but I've seen hints that this kind of thing will start getting lots of
> push-back in the near future...
>
I wonder it may be hard to avoid it, especially for some complicated devices or
non-standard devices during development process.
No sure if any better way to control it.

> That all said, I'd also love to change the Tegra pinctrl binding now
> that we have CPP, since I could replace all the strings in the current
> binding with integers and presumably save some space in the .dtb. Hence,
> I'd love to maintain the flexibility to keep changing the .dts and
> kernel code in lock-step.
>
> 2)
>
> This patch removes a bunch of tables from the kernel. I like having the
> tables in the kernel, since in theory it could allow a future debugfs or
> sysfs interface to pinctrl that allows manipulation of the pinctrl HW
> state at a semantic level.

Right, that's one of the considerations why i kept the register table in driver
in the first binding design.

> This is only possible if the DT binding
> includes details such as "set this pin to this function", and the driver
> includes the tables to convert that into details such as register
> address and value. I don't think such an "API" could be implemented for
> IMX after this patch.

Theoretically It can, because device tree contains the pin registers
informations
in this way. But the problem is that it can only manipulate the used pins which
are parsed from devices tree. While for those unused pins, since the driver does
not have the pin register information, it can not manipulate it.
But what i'm wondering now is do we really need to manipulate the unused pins
from sysfs or debugfs?
I still can not think out a real using case now.
Anyone else can think one?

> Still, given the IMX binding already merged "pin"
> and "function" into a single integer in the binding, I'm not sure if IMX
> could support that kind of "API" before anyway?
>
At least PIN config could support.
For PIN mux, it may also be implementable since we have all the needed
information
for a pin in driver whether it's used or not.

Regards
Dong Aisheng

> This is part of the reason I pushed hard for the pinctrl APIs to operate
> at the semantic pin/group/function level, and that Tegra's pinctrl
> drivers explicitly have tables listing all the pins/groups/functions,
> rather than just putting a bunch of register values into the DT.
>
> I'm not sure how much of an issue this is, though. LinusW and/or the DT
> maintainers should probably chime in here.
>
> I didn't actually read the driver implementation changes in this patch.

Re: [PATCH 3/3] pinctrl: imx: move hard-coding data into device tree

[Stephen Warren]

On 02/21/2013 02:36 AM, Dong Aisheng wrote:
> On 21 February 2013 03:04, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 02/20/2013 12:08 AM, Shawn Guo wrote:
>>> Currently, all imx pinctrl drivers maintain a big array of struct
>>> imx_pin_reg which hard-codes data like register offset and mux mode
>>> setting for each pin function.  Every time a new imx SoC support is
>>> added, we need to add such a big mount of data.  With moving to single
>>> kernel build, it's only matter of time to be blamed on memory consuming.
>>>
>>> With DTC pre-processor support in place, the patch moves all these data
>>> into device tree by redefining the PIN_FUNC_ID in imxXX-pinfunc.h and
>>> changing the PIN_FUNC_ID parsing code a little bit.
...
>> This patch removes a bunch of tables from the kernel. I like having the
>> tables in the kernel, since in theory it could allow a future debugfs or
>> sysfs interface to pinctrl that allows manipulation of the pinctrl HW
>> state at a semantic level.
> 
> Right, that's one of the considerations why i kept the register table in driver
> in the first binding design.
> 
>> This is only possible if the DT binding
>> includes details such as "set this pin to this function", and the driver
>> includes the tables to convert that into details such as register
>> address and value. I don't think such an "API" could be implemented for
>> IMX after this patch.
> 
> Theoretically It can, because device tree contains the pin registers
> informations

> in this way. But the problem is that it can only manipulate the used pins which
> are parsed from devices tree. While for those unused pins, since the driver does
> not have the pin register information, it can not manipulate it.

Right, that's the main issue I was trying to highlight; the driver no
longer provides complete knowledge of the HW, but only learns about
stuff that happens to be used/defined in the board-specific DT.

> But what i'm wondering now is do we really need to manipulate the unused pins
> from sysfs or debugfs?
> I still can not think out a real using case now.
> Anyone else can think one?

For early board bringup, it may be useful to boot with a minimal set of
entries in the DT (enough to support e.g. serial console and some device
for a root filesystem) and then interactively tweak the rest of the
pinmux to define the rest of the board, and perhaps even save the result
somehow, and add it to the next iteration of the DT.

Also, consider a hobbyist-oriented board that has 50 unused pins that
could be GPIO, or mapped to some otherwise unused SD, I2C, SPI, ...
controller. Do we want to force the user to write a new DT and reboot,
or would it be better to provide some runtime API to set up those pins
based on what they had connected to the board, and then instantiate
drivers or use the sysfs GPIO interface, after doing so?

Re: [PATCH 3/3] pinctrl: imx: move hard-coding data into device tree

[Dong Aisheng]

On Thu, Feb 21, 2013 at 12:57:02PM -0700, Stephen Warren wrote:
> On 02/21/2013 02:36 AM, Dong Aisheng wrote:
> > On 21 February 2013 03:04, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> >> On 02/20/2013 12:08 AM, Shawn Guo wrote:
> >>> Currently, all imx pinctrl drivers maintain a big array of struct
> >>> imx_pin_reg which hard-codes data like register offset and mux mode
> >>> setting for each pin function.  Every time a new imx SoC support is
> >>> added, we need to add such a big mount of data.  With moving to single
> >>> kernel build, it's only matter of time to be blamed on memory consuming.
> >>>
> >>> With DTC pre-processor support in place, the patch moves all these data
> >>> into device tree by redefining the PIN_FUNC_ID in imxXX-pinfunc.h and
> >>> changing the PIN_FUNC_ID parsing code a little bit.
> ...
> >> This patch removes a bunch of tables from the kernel. I like having the
> >> tables in the kernel, since in theory it could allow a future debugfs or
> >> sysfs interface to pinctrl that allows manipulation of the pinctrl HW
> >> state at a semantic level.
> > 
> > Right, that's one of the considerations why i kept the register table in driver
> > in the first binding design.
> > 
> >> This is only possible if the DT binding
> >> includes details such as "set this pin to this function", and the driver
> >> includes the tables to convert that into details such as register
> >> address and value. I don't think such an "API" could be implemented for
> >> IMX after this patch.
> > 
> > Theoretically It can, because device tree contains the pin registers
> > informations
> 
> > in this way. But the problem is that it can only manipulate the used pins which
> > are parsed from devices tree. While for those unused pins, since the driver does
> > not have the pin register information, it can not manipulate it.
> 
> Right, that's the main issue I was trying to highlight; the driver no
> longer provides complete knowledge of the HW, but only learns about
> stuff that happens to be used/defined in the board-specific DT.
> 
> > But what i'm wondering now is do we really need to manipulate the unused pins
> > from sysfs or debugfs?
> > I still can not think out a real using case now.
> > Anyone else can think one?
> 
> For early board bringup, it may be useful to boot with a minimal set of
> entries in the DT (enough to support e.g. serial console and some device
> for a root filesystem) and then interactively tweak the rest of the
> pinmux to define the rest of the board, and perhaps even save the result
> somehow, and add it to the next iteration of the DT.
> 
> Also, consider a hobbyist-oriented board that has 50 unused pins that
> could be GPIO, or mapped to some otherwise unused SD, I2C, SPI, ...
> controller. Do we want to force the user to write a new DT and reboot,
> or would it be better to provide some runtime API to set up those pins
> based on what they had connected to the board, and then instantiate
> drivers or use the sysfs GPIO interface, after doing so?
> 

Yes, it can be useful sometimes.
In order to do that, we have to add the whole register table in driver.
Considering the defects, since using memtool can do the same things,
maybe it's not worth for IMX to do that.

Regards
Dong Aisheng