linux-kernel - Re: [PATCH v3 1/9] pinctrl: mvebu: pinctrl driver core

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5050FA4C.8040406@wwwdotorg.org>
Date:	Wed, 12 Sep 2012 15:10:36 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
CC:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Lior Amsalem <alior@...vell.com>,
	Russell King <linux@....linux.org.uk>,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Linus Walleij <linus.walleij@...aro.org>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	Rob Herring <rob.herring@...xeda.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Ben Dooks <ben.dooks@...ethink.co.uk>,
	Rob Landley <rob@...dley.net>,
	Gregory CLEMENT <gregory.clement@...e-electrons.com>,
	devicetree-discuss@...ts.ozlabs.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 1/9] pinctrl: mvebu: pinctrl driver core

On 09/12/2012 12:54 AM, Thomas Petazzoni wrote:
> Le Tue, 11 Sep 2012 16:17:13 -0600,
> Stephen Warren <swarren@...dotorg.org> a écrit :
> 
>>
>> +static struct mvebu_mpp_mode dove_mpp_modes[] = {
>> +	MPP_MODE(0,
>> +		MPP_FUNCTION(0x00, "gpio", NULL),
>> +		MPP_FUNCTION(0x02, "uart2", "rts"),
>> +		MPP_FUNCTION(0x03, "sdio0", "cd"),
>> +		MPP_FUNCTION(0x0f, "lcd0", "pwm"),
>> +		MPP_FUNCTION(0x10, "pmu", NULL)),
>>
>> it's defining the functions within the context of a particular group
>> ("mode" in the drivers terminology, I think...) rather than defining
>> functions and groups as separate top-level tables.
> 
> This data structure really reflects what the datasheet says. Typically,
> for SoCs where each pin is independently muxable (AT91, i.MX23/28,
> Marvell, and probably many more), the datasheet has a big array, with
> one line per pin, and then several columns which tell for a given pin,
> what is "function 0", "function 1", "function 2", "function 3", etc.
> 
> So the data structure that Sebastian has implemented in the mvebu
> driver immediately reflects this. In fact, the pinctrl table code for
> Armada 370 and Armada XP was semi-automatically generated from CSV data
> of the pinmux functions, directly coming from the datasheets.

OK, that seems like a reasonable explanation.

Still, doing data manipulation at run-time when it could easily be done
by the script that converts your CSV into the driver tables seem
inefficient at least. I agree with LinusW that it's not a big deal though.

> Having to
> create a global list of all possible functions seems useless and
> painful, since the functions only make sense in the context of one
> particular pin.

Surely some of your functions can be selected onto 1 of n pins? If
that's true, then the functions don't only exist within the context of a
single pin.

Note that some pinctrl driver authors have decided to create functions
based on just the mux register values (e.g. mux0, mux1, mux2, mux3 for a
2-bit field) rather than semantically (e.g. uart1, uart2, i2c1, i2c2),
in which case, all functions are global and available on any pin. I
actually somewhat wonder if I shouldn't have done that for Tegra.

> Could you be more specific as to what representation you're looking
> after? You're proposing to "define functions and groups as separate
> top-level tables", but then how to you map which functions are
> available for which group,

The definition of a function is a function name (struct
pinmux_ops.get_function_name) and a list of groups onto which it can be
selected (struct pinmux_ops.get_function_groups).

The pinctrl core leaves it up to individual drivers how to represent how
to actually program a given group with a given function. ...

> and what is the number of that function is
> this group (which we need to actually configure the mapping). I'd
> really like to see (with a short code example) what is your view of how
> pinmux should be handled for SoCs having independently muxable pins.

 As an example, see drivers/pinctrl/pinctrl-tegra20.c variable
tegra20_groups[] where each group definition contains additional
information beyond the basic information that the pinctrl core requires
(which is just struct pinctrl_ops get_group_name get_group_pins) - i.e.
the global function ID that is selected by each of the 4 values you can
write into that pin's/group's register.

Incidentally, I see that tegra20_groups[] is almost exactly equivalent
to the data-structure we're talking about here; the difference is that
the Tegra driver additionally has pre-generated arrays like xio_groups[]
(the list of groups where function xio can be selected) in order to
short-circuit the run-time calculations that your driver is doing.

(I don't believe any of this discussion is affected by whether the HW
muxes at a per-pin level or per-pin-group level; Tegra30 muxes per-pin
and the driver uses the exact same data-structures as Tegra20 which
muxes per-group).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/