linux-kernel - Re: [PATCH v3 00/13] pinctrl: mvebu: restructure resource allocation

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]
Date:	Thu, 13 Feb 2014 17:59:14 +0100
From:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
To:	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	Ezequiel Garcia <ezequiel.garcia@...e-electrons.com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 00/13] pinctrl: mvebu: restructure resource
 allocation

Dear Sebastian Hesselbarth,

On Thu, 13 Feb 2014 17:41:02 +0100, Sebastian Hesselbarth wrote:

> > Thanks again for working on this! I have boot tested this successfully
> > on an Armada XP platform, and it seems to behave normally, the debugfs
> > pinctrl contents make sense.
> 
> I guess this is a Tested-by ?

Yes. My tests were admittedly fairly light, but I believe good enough :)

> > I am not sure what you mean here in terms of the ordering for the
> > patches. I'm attaching several patches, and the first three patches
> > adapt your patch series to also cover 375 and 38x, assuming the pinctrl
> > support for 375 and 38x is merged before your patch series.
> 
> Right. If 375/38x pinctrl goes in first (what I expect), I'd have to add
> corresponding patches. You already sent them, I'll pick them up.

Ok, cool. Hopefully we can sort out the merging of those two patch
series for 3.15 with Linus Walleij.

> > I must say I dislike quite a bit this unnamed mpp controls mechanism.
> > Why isn't the name statically defined in the source code by the
> > MPP_MODE macro, which already takes as first argument the pin number?
> 
> Honestly, the unnamed mpp control thing is a bit odd. But if you tell
> me how to create ~60 statically defined one pin groups out of a
> single-line macro, we can change that easily.
> 
> Back when that unnamed mpp control thing was invented, I must have been
> to lazy to write e.g.
> 
> MPP_FUNC_CTRL(0, 0, "mpp0", armada_xp_mpp_ctrl),
> MPP_FUNC_CTRL(1, 1, "mpp1", armada_xp_mpp_ctrl),
> MPP_FUNC_CTRL(2, 2, "mpp2", armada_xp_mpp_ctrl),
> ...
> MPP_FUNC_CTRL(66, 66, "mpp66", armada_xp_mpp_ctrl),
> 
> instead of
> 
> MPP_FUNC_CTRL(0, 66, NULL, armada_xp_mpp_ctrl),
> 
> and generate the 66 names dynamically.

Right. But what I meant is that we already have a place where we have
one macro call for each pin: when defining the MPP modes. So I was
thinking of simplifying the whole stuff by "merging" the notion of MPP
control with the notion of MPP mode. This way, when you do:

	MPP_MODE(0,
		   MPP_FUNCTION(...),
		   MPP_FUNCTION(...)),
	MPP_MODE(1,
		   MPP_FUNCTION(...),
		   MPP_FUNCTION(...)),
	MPP_MODE(2,
		   MPP_FUNCTION(...),
		   MPP_FUNCTION(...)),
[...]
	MPP_MODE(65,
		   MPP_FUNCTION(...),
		   MPP_FUNCTION(...)),

You can take this opportunity to generate:

	{ "mpp0", ... },
	{ "mpp1", ... },
	{ "mpp2", ... },
	...
	{ "mpp65", ... },

> > This is definitely good, but I'm wondering why the core cannot provide
> > helper functions for the generic case where we have 4 bits per pin in
> > contiguous registers. This would avoid duplicating the helper function
> > six times (you have four in your patch series, and we'll need two more
> > for A375 and A38x).
> 
> I thought about it too, but we would need a soc specific callback
> anyway as you'll have to pass the base address somehow (and that is now
> known by soc specific stub only). My quick rule of thumb was that the
> amount of code replication would be almost the same.

In pinctrl-mvebu.h, we could have:

static inline int default_mpp_ctrl_get(void __iomem *base, unsigned int pid, unsigned long *config)
{
        unsigned off = (pid / MVEBU_MPPS_PER_REG) * MVEBU_MPP_BITS;
        unsigned shift = (pid % MVEBU_MPPS_PER_REG) * MVEBU_MPP_BITS;

        *config = (readl(base + off) >> shift) & MVEBU_MPP_MASK;

        return 0;
}

static inline int default_mpp_ctrl_set(void __iomem *base, unsigned int pid, unsigned long config)
{
        unsigned off = (pid / MVEBU_MPPS_PER_REG) * MVEBU_MPP_BITS;
        unsigned shift = (pid % MVEBU_MPPS_PER_REG) * MVEBU_MPP_BITS;
        unsigned long reg;

        reg = readl(base + off) & ~(MVEBU_MPP_MASK << shift);
        writel(reg | (config << shift), base + off);

        return 0;
}

which would slightly reduce the per-SoC code to:

static int armada_370_mpp_ctrl_get(unsigned pid, unsigned long *config)
{
	return default_mpp_ctrl_get(mpp_base, pid, config);
}

static int armada_370_mpp_ctrl_set(unsigned pid, unsigned long config)
{
	return default_mpp_ctrl_set(mpp_base, pid, config);
}

but we admittedly cannot completely remove the per-SoC function, since
the mpp_base is now only known to each per-SoC driver.

> > Feel free to squash these patches into the appropriate patches.
> 
> Yep, thanks for these! I'll squash them in and send an updated v4 as
> soon as the discussion here stalls.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
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/