Re: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Russell King - ARM Linux]

On Mon, Jan 04, 2016 at 03:27:57PM +0200, Tero Kristo wrote:
> On 01/04/2016 12:21 PM, Geert Uytterhoeven wrote:
> >FWIW, there are small loops with just a cpu_relax() in various clock drivers
> >under drivers/clk/shmobile/.
> 
> Just did a quick profiling round, and the clk_enable/disable delay loops
> take anything from 0...1500ns, most typically consuming some 400-600ns. So,
> based on this, dropping the udelay and adding cpu_relax instead looks like a
> good change. I just verified that changing the udelay to cpu_relax works
> fine also, I just need to change the bail-out period to be something sane.

Was that profiling done with lockdep/lock debugging enabled or disabled?

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ARM: ATAGS: move atags.h to include/asm so it can be included by files outside kernel/

[Russell King - ARM Linux]

On Thu, Dec 24, 2015 at 04:00:56PM +0200, Ivaylo Dimitrov wrote:
> This is needed by a follow-up patch that saves atags on RX51 device
> 
> Signed-off-by: Ivaylo Dimitrov 
> ---
>  arch/arm/include/asm/atags.h  | 20 
>  arch/arm/kernel/atags.h   | 20 

Please generate diffs with -M so that it can detect renames and provide
a more sensible diffstat and patch output (so we can see any changes
through the rename.)  However...

>  arch/arm/kernel/atags_parse.c |  3 +--
>  arch/arm/kernel/setup.c   |  3 +--
>  4 files changed, 22 insertions(+), 24 deletions(-)
>  create mode 100644 arch/arm/include/asm/atags.h
>  delete mode 100644 arch/arm/kernel/atags.h
> 
> diff --git a/arch/arm/include/asm/atags.h b/arch/arm/include/asm/atags.h
> new file mode 100644
> index 000..ec4164d
> --- /dev/null
> +++ b/arch/arm/include/asm/atags.h
> @@ -0,0 +1,20 @@
> +#ifdef CONFIG_ATAGS_PROC
> +extern void save_atags(struct tag *tags);
> +#else
> +static inline void save_atags(struct tag *tags) { }
> +#endif
> +
> +void convert_to_tag_list(struct tag *tags);
> +
> +#ifdef CONFIG_ATAGS
> +const struct machine_desc *setup_machine_tags(phys_addr_t __atags_pointer,
> + unsigned int machine_nr);
> +#else
> +static inline const struct machine_desc *
> +setup_machine_tags(phys_addr_t __atags_pointer, unsigned int machine_nr)
> +{
> + early_print("no ATAGS support: can't continue\n");
> + while (true);
> + unreachable();
> +}
> +#endif
> diff --git a/arch/arm/kernel/atags.h b/arch/arm/kernel/atags.h
> deleted file mode 100644
> index ec4164d..000
> --- a/arch/arm/kernel/atags.h
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -#ifdef CONFIG_ATAGS_PROC
> -extern void save_atags(struct tag *tags);
> -#else
> -static inline void save_atags(struct tag *tags) { }
> -#endif

I'd think I'd prefer moving just the above save_atags declaration to
arch/arm/include/asm/setup.h so we're not allowing the rest of this
to be exposed to anyone who includes asm/atags.h.

Thanks.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

[Russell King - ARM Linux]

On Tue, Dec 15, 2015 at 10:33:25AM +0100, Pali Rohár wrote:
> So am I understand correctly that solution would be to hack
> arch/arm/mm/mmu.c to not overwrite page at PHYS_OFFSET?

That's completely unnecessary: there are enough platform hooks to cope
with whatever the platform requires.

If you want to reserve the memory, then you have the ->reserve callback,
where you can call:

memblock_reserve(PHYS_OFFSET, PAGE_SIZE);

if you wish to prevent the first page being overwritten.  You're then
responsible for freeing this page later in the boot sequence, or you
could just keep it around and refer to the atags in that page directly.

You could also just save_atags() in there, with a comment saying that
this is a work-around for N900 which needs the ATAGs saved, and this
is allowed in ->reserve as a special exception.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Russell King - ARM Linux]

On Fri, Dec 11, 2015 at 07:48:54AM -0800, Tony Lindgren wrote:
> There's a problem with MAX_CON_ID 16 hardcoded allocated name length.

Absolutely right...

> In this case I have 13 instances of plls with 3 - 4 outputs each and I'd
> like to use "481c5040.adpll.clkout" style naming starting with the instance
> address. Also "adpll5.clkdcoldo" style naming would work,

Because it's a connection ID, not a clock _name_.  I see that we're still
making all the same mistakes with clocks that were made years ago, and
which lead down the path of amazingly complex drivers having conditional
clock names and the like.

Is there a reason why you can't split this into separate device and input
names, and use clk_get_sys() rather than passing NULL as the device to
clk_get().  This is exactly why we've ended up with clk_get_sys(), to
cater for the cases where there is no struct device to associate with the
connection ID: the idea behind clk_get_sys() is that you pass the device
name explicitly, along with the connection ID.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL

[Russell King - ARM Linux]

On Thu, Dec 10, 2015 at 06:26:32PM -0800, Tony Lindgren wrote:
> + /* Released with kfree() by clkdev_drop() */
> + cl = kzalloc(sizeof(*cl), GFP_KERNEL);
> + if (!cl)
> + return -ENOMEM;
> +
> + /* Use clkdev_add, clk_register_clkdev limits length to MAX_CON_ID */
> + cl->con_id = name;
> + cl->clk = clock;
> + cl->clk_hw = __clk_get_hw(clock);
> + clkdev_add(cl);
> + d->clocks[index].cl = cl;

NAK.  I've no idea why you're open-coding the clkdev internals (which
seems to have been a historical habbit in OMAP code.)  Please stop
doing this.

You are provided with clkdev_alloc() which will allocate the structure
and initialise it for you, and clkdev_add() which will add the allocated
and initialised struct to the list of lookups.  Everything you're doing
above can be done with clkdev_alloc() + clkdev_add() which have been
there for a _very_ long time.  They're even documented (thanks for
providing me with more proof that documentation is nothing but a waste
of time. :))

Even better is clkdev_create() which eliminates the two step clkdev_alloc()
and clkdev_add() process.

So, the whole of the above can be reduced down to:

cl = clkdev_create(clock, name, NULL);
if (!cl)
return -ENOMEM;

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

[Russell King - ARM Linux]

On Sun, Nov 29, 2015 at 07:19:18PM +0100, Pali Rohár wrote:
> On Sunday 29 November 2015 19:09:39 Russell King - ARM Linux wrote:
> > On Sat, Nov 28, 2015 at 12:34:23PM -0500, Nicolas Pitre wrote:
> > > Good. And Arnd likes the idea too. So we might be converging at
> > > last which is a good thing.
> > 
> > I disagree with the idea that there is convergence.  There might be
> > convergence towards an idea, but... Here's a mail extract, from July
> > 7th, from earlier in this very thread:
> > 
> > Pali:
> > > Me:
> > > > Are the ATAGs at a fixed address on the N900?
> > > 
> > > Yes, in board-rx51.c is:
> > > 
> > > .atag_offset= 0x100
> > > 
> > > and Nokia Bootloader (proprietary) store them to that address.
> > > 
> > > > Can that be handled in
> > > > some kind of legacy file for the N900 which calls save_atags() on
> > > > it, so we don't end up introducing yet more stuff that we have
> > > > to maintain into the distant future?  If not, what about copying
> > > > a known working atag structure into a legacy file for the N900?
> > > 
> > > I already asked question if it is possible to read ATAGs from DT
> > > booted kernel. And somebody (do not remember who) wrote to ML,
> > > that it is not possible and it can be done in that uncompress
> > > code.
> > 
> > So you're converging on an idea that has already been rejected. 
> > That's not a good thing, IMHO.
> 
> Or in other case show that such implementation is possible...

Only those with the problem can do that.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

[Russell King - ARM Linux]

On Sat, Nov 28, 2015 at 12:34:23PM -0500, Nicolas Pitre wrote:
> Good. And Arnd likes the idea too. So we might be converging at last 
> which is a good thing.

I disagree with the idea that there is convergence.  There might be
convergence towards an idea, but... Here's a mail extract, from July
7th, from earlier in this very thread:

Pali:
> Me:
> > Are the ATAGs at a fixed address on the N900?
> 
> Yes, in board-rx51.c is:
> 
> .atag_offset= 0x100
> 
> and Nokia Bootloader (proprietary) store them to that address.
> 
> > Can that be handled in
> > some kind of legacy file for the N900 which calls save_atags() on it, so
> > we don't end up introducing yet more stuff that we have to maintain into
> > the distant future?  If not, what about copying a known working atag
> > structure into a legacy file for the N900?
> >
> 
> I already asked question if it is possible to read ATAGs from DT booted
> kernel. And somebody (do not remember who) wrote to ML, that it is not
> possible and it can be done in that uncompress code.

So you're converging on an idea that has already been rejected.  That's
not a good thing, IMHO.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

[Russell King - ARM Linux]

On Fri, Nov 27, 2015 at 06:28:50PM -0500, Nicolas Pitre wrote:
> On Fri, 27 Nov 2015, Arnd Bergmann wrote:
> 
> > I don't mind creating the /proc/atags compatibility hack from the kernel
> > for a DT based N700 kernel, as long as we limit it as much as we can
> > to the machines that need it. Leaving a board file for the N700 in place
> > that contains the procfs code (and not much more) seems reasonable
> > here, as we are talking about a board specific hack and the whole point
> > appears to be running unmodified user space.
> > 
> > Regarding how to get the data into the kernel in the first place, my
> > preferred choice would still be to have an intermediate bootloader
> > such as pxa-impedance-matcher, but I won't complain if others are
> > happy enough about putting it into the ATAGS compat code we already
> > have, as long as it's limited to the boards we know need it.
> 
> Assuming you have a N700 board file for special procfs code, then why 
> not getting at the atags in memory where the bootloader has put them 
> directly from that same board file? This way it'll really be limited to 
> the board we know needs it and the special exception will be contained 
> to that one file.  Amongst the machine specific hooks, there is one that 
> gets invoked early during boot before those atags are overwritten.

I've already suggested that.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

[Russell King - ARM Linux]

On Sat, Nov 28, 2015 at 01:27:07PM +0100, Arnd Bergmann wrote:
> On Friday 27 November 2015 18:28:50 Nicolas Pitre wrote:
> > On Fri, 27 Nov 2015, Arnd Bergmann wrote:
> > 
> > > I don't mind creating the /proc/atags compatibility hack from the kernel
> > > for a DT based N700 kernel, as long as we limit it as much as we can
> > > to the machines that need it. Leaving a board file for the N700 in place
> > > that contains the procfs code (and not much more) seems reasonable
> > > here, as we are talking about a board specific hack and the whole point
> > > appears to be running unmodified user space.
> > > 
> > > Regarding how to get the data into the kernel in the first place, my
> > > preferred choice would still be to have an intermediate bootloader
> > > such as pxa-impedance-matcher, but I won't complain if others are
> > > happy enough about putting it into the ATAGS compat code we already
> > > have, as long as it's limited to the boards we know need it.
> > 
> > Assuming you have a N700 board file for special procfs code, then why 
> > not getting at the atags in memory where the bootloader has put them 
> > directly from that same board file? This way it'll really be limited to 
> > the board we know needs it and the special exception will be contained 
> > to that one file.  Amongst the machine specific hooks, there is one that 
> > gets invoked early during boot before those atags are overwritten.
> 
> I didn't realize this was possible, as we don't know the atags pointer
> when we instead get a DTB pointer. However you are right: the board
> file knows exactly that the atag_offset is 0x100, so we can grab it
> from there, and that will make the implementation really easy and
> contained to a single file that has access to the atags and that
> can create the /proc/atags file for it.

I've made several suggestions over the year or so that this problem has
been around, and solving this problem appears to be getting nowhere...
(because we _still_ have the problem today.)  When the same suggestions
start to be made by other people, I think there's not much more that can
be done to help resolve the situation.  It's probably time to walk away
from the problem, and let those who are supposedly motivated to use
these troublesome platforms just get on with it.

I'm not sure what Tony does at this point: if he rips out the non-DT
OMAP code, it'll cause a regression, but at the same time, it provides
additional motivation to get the problem resolved.  I can quite well
see Pavel going off and whinging at Linus, Linus getting stressed at
us for intentionally breaking something that used to work, and telling
everyone that they shouldn't be working on the kernel, in his usual
friendly way.

So, I think if the non-DT OMAP stuff is getting in the way of further
OMAP development, then the only solution is to put pressure on those
who are holding it up: in other words, put pressure on those to get
this damned problem solved.

The only thing I can think of doing is to give the N900 people notice
that they're causing a problem here, explaining exactly why - maybe
explaining that it's been causing a problem however long it has and
that the only option is going to be to fork mainline and effectively
leave the code in mainline unmaintained because of this.

Then, of course, those who have caused this situation then get the fun
job of maintaining _all_ the OMAP code in mainline on their own, which
I think would bury them under such a huge mountain that the code would
end up being terminally broken, and ripe for deletion. At which point,
it'd make sense to merge the maintained fork back into mainline, which
of course wouldn't have the troublesome code platforms by that time. :)

Yes, it's not particularly nice, but I don't see this problem getting
resolved.

(Maybe this email will be enough to motivate the N900 users to sort this
out, but I suspect they'll prefer to spend time whinging and moaning at
me in email rather than doing what needs to be done and fixing the
problem.)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

[Russell King - ARM Linux]

On Fri, Nov 27, 2015 at 01:27:23PM +, Russell King - ARM Linux wrote:
> It is possible to redirect any program to open any other file.  You can
> do it via a LD preload, and intercepting the open(), and possibly the
> read() calls if you want to do something more fancy.  The down-side is
> that you have to arrange for the preloaded object to be used by the
> linker, and the additional overhead it places on the intercepted
> functions.

Another idea if people don't like the preload idea.

We could create a zero-sized /proc/atags, and then use a bind mount in
userspace to bind some other file containing the required information
on top.  That could even be the atag blob from /sys/firmware/whatever.
The N700 (or whatever platform needs it) could be responsible for
creating the zero-sized /proc/atags so that we don't have it everywhere.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT "/chosen/linux,atags" entry

[Russell King - ARM Linux]

On Thu, Nov 26, 2015 at 10:07:39AM +0100, Pali Rohár wrote:
> On Wednesday 25 November 2015 20:19:21 Frank Rowand wrote:
> > > Or populate /proc/atags only for the ones that need it from machine
> > > specific init_early?
> > 
> > This is circling back to the first comment from Russell King where
> > he suggested a legacy file for the N900 which calls save_atags():
> > 
> > Are the ATAGs at a fixed address on the N900?  Can that be handled in
> > some kind of legacy file for the N900 which calls save_atags() on it, so
> > we don't end up introducing yet more stuff that we have to maintain into
> > the distant future?  If not, what about copying a known working atag
> > structure into a legacy file for the N900?
> > 
> > It seems to me that patches 1, 2, 4, and 5 could be replaced by this
> > approach.
> 
> Hi Frank, in this case I will ask my question again: It is possible to
> read atags from that legacy file. And if yes how? I was not thinking
> about this approach because somebody in past wrote that this is not
> possible...

It is possible to redirect any program to open any other file.  You can
do it via a LD preload, and intercepting the open(), and possibly the
read() calls if you want to do something more fancy.  The down-side is
that you have to arrange for the preloaded object to be used by the
linker, and the additional overhead it places on the intercepted
functions.

Eg,

openatags.c:

#define open libc_open
#include 
#undef open
#include 

int open(const char *pathname, int flags, mode_t mode)
{
static int (*old_open)(const char *pathname, int flags, mode_t mode);

if (strcmp(pathname, "/proc/atags") == 0)
pathname = "/tmp/my-atags";

if (!old_open)
old_open = dlsym(RTLD_NEXT, "open");

return old_open(pathname, flags, mode);
}

Build the above (untested) with:
gcc -O2 -o openatags.o -c openatags.c
gcc -shared -o openatags.so openatags.o -ldl

Now, when running one of these programs, you can test it with:
LD_PRELOAD=openatags.so /name/of/program

You could also list the full pathname to openatags.so in /etc/ld.so.preload,
but test it first, because it will always be used by the linker in that
case, and you wouldn't want normal commands to misbehave.

Note that putting it in /etc/ld.so.preload will also have the effect that
cat /proc/atags will also get redirected to /tmp/my-atags too.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BISECTED] v4.3-rc5: OMAP1 boot hang

[Russell King - ARM Linux]

On Wed, Nov 11, 2015 at 11:44:44PM +0200, Aaro Koskinen wrote:
> Hi,
> 
> Any suggestions how to debug this further? This happens also with v4.3
> final. Is the CPU_SW_DOMAIN_PAN supposed to work with this CPU?
> 
> I tried to disable various drivers (e.g. NAND, USB) and it still
> hangs... And it seems always at the same printk time stamp (roughly at
> 25 seconds).

No idea what so ever, and I have zero knowledge of this ARM925 thing -
never had one, and never seen any specs on it.  The weird thing is
that it's not producing any oops dump - I guess the kernel is totally
dead and unresponsive.  I'd ask if you have a heartbeat LED, but I'd
guess the answer will be that the hardware is too limited.

Any additional information you can give might help, but I wouldn't hold
out any hopes...

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Russell King - ARM Linux]

On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote:
> Hi Mike, Russell,
> 
> On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette
>  wrote:
> > Why not keep the reference to the struct clk after get'ing it the first
> > time?
> 
> And store it where?

Not my problem :)

Users are supposed to hold on to the reference obtained via clk_get()
while they're making use of the clock: in some implementations, this
increments the module use count if the clock driver is a module, and
may have other effects too.

Dropping that while you're still requiring the clock to be enabled is
unsafe: if it is provided by a module, then removing and reinserting
the module may very well change the enabled state of the clock, it
most certainly will disrupt the enable count.

It's always been this way, right from the outset, and when I've seen
people doing this bollocks, I've always pointed out that it's wrong.
Generally, people will fix it once they become aware of it, so it's
really that people just don't like reading and conforming to published
API requirements.

I think the root cause is that people just don't like reading what
other people write in terms of documentation, and they prefer to go
off and do their own thing, provided it works for them.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BISECTED] Nokia 770 framebuffer breakage

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 11:50:03AM +0300, Aaro Koskinen wrote:
> On Mon, Oct 19, 2015 at 11:50:33PM +0100, Russell King - ARM Linux wrote:
> > It shouldn't (I've been through the resulting assembly code.)  I wonder
> > if this is fixing the problem, but it's now failing elsewhere instead.
> > 
> > What happens if you change it to:
> > 
> > l = clkdev_create(r, alias, alias_dev_name);
> > 
> > which should be 100% identical to how older kernels behaved
> 
> Ok, this and the earlier patch works on top of
> 2568999835d7797afce3dcc3a3f368051ffcaf1f, but not on 4.2 or newer - so
> it fixes the clk_add_alias issue, and newer kernels have other issues...

Thanks, so this fixes the regression you were seeing with the above
mentioned commit, so I'll get that into mainline and stable kernels.

Can I add a tested-by tag for it please?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC RFT 2/3] clk: clk_put WARNs if user has not disabled clk

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 05:40:00AM -0700, Michael Turquette wrote:
> Why not keep the reference to the struct clk after get'ing it the first
> time?

Yes, that's exactly what you're supposed to do in this case.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BISECTED] Nokia 770 framebuffer breakage

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 10:07:00AM +0100, Russell King - ARM Linux wrote:
> On Tue, Oct 20, 2015 at 11:50:03AM +0300, Aaro Koskinen wrote:
> > On Mon, Oct 19, 2015 at 11:50:33PM +0100, Russell King - ARM Linux wrote:
> > > It shouldn't (I've been through the resulting assembly code.)  I wonder
> > > if this is fixing the problem, but it's now failing elsewhere instead.
> > > 
> > > What happens if you change it to:
> > > 
> > >   l = clkdev_create(r, alias, alias_dev_name);
> > > 
> > > which should be 100% identical to how older kernels behaved
> > 
> > Ok, this and the earlier patch works on top of
> > 2568999835d7797afce3dcc3a3f368051ffcaf1f, but not on 4.2 or newer - so
> > it fixes the clk_add_alias issue, and newer kernels have other issues...
> 
> Thanks, so this fixes the regression you were seeing with the above
> mentioned commit, so I'll get that into mainline and stable kernels.
> 
> Can I add a tested-by tag for it please?

Also make that reported-by as well.

I'll hold the patch back pending a reply...

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BISECTED] Nokia 770 framebuffer breakage

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 07:14:28PM +0300, Aaro Koskinen wrote:
> Hi,
> 
> On Tue, Oct 20, 2015 at 05:05:24PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Oct 20, 2015 at 10:07:00AM +0100, Russell King - ARM Linux wrote:
> > > On Tue, Oct 20, 2015 at 11:50:03AM +0300, Aaro Koskinen wrote:
> > > > On Mon, Oct 19, 2015 at 11:50:33PM +0100, Russell King - ARM Linux 
> > > > wrote:
> > > > > It shouldn't (I've been through the resulting assembly code.)  I 
> > > > > wonder
> > > > > if this is fixing the problem, but it's now failing elsewhere instead.
> > > > > 
> > > > > What happens if you change it to:
> > > > > 
> > > > >   l = clkdev_create(r, alias, alias_dev_name);
> > > > > 
> > > > > which should be 100% identical to how older kernels behaved
> > > > 
> > > > Ok, this and the earlier patch works on top of
> > > > 2568999835d7797afce3dcc3a3f368051ffcaf1f, but not on 4.2 or newer - so
> > > > it fixes the clk_add_alias issue, and newer kernels have other issues...
> > > 
> > > Thanks, so this fixes the regression you were seeing with the above
> > > mentioned commit, so I'll get that into mainline and stable kernels.
> > > 
> > > Can I add a tested-by tag for it please?
> > 
> > Also make that reported-by as well.
> > 
> > I'll hold the patch back pending a reply...
> 
> Yes, both or either are fine.

Thanks, queued for 4.3 and stable.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BISECTED] Nokia 770 framebuffer breakage

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 10:37:59PM +0300, Aaro Koskinen wrote:
> Hi,
> 
> Somewhere between 4.1 .. 4.3-rc6 framebuffer on Nokia 770 stopped working.
> 
> Bisection points to:
> 
> commit 2568999835d7797afce3dcc3a3f368051ffcaf1f
> Author: Russell King <rmk+ker...@arm.linux.org.uk>
> Date:   Mon Mar 2 15:40:29 2015 +
> 
> clkdev: add clkdev_create() helper
> 
> The commit cannot be reverted cleanly from current trees, but I re-tested
> that commit 2568999835d7797afce3dcc3a3f368051ffcaf1f does not work and
> commit 2568999835d7797afce3dcc3a3f368051ffcaf1f^1 indeed works.

Really need more information than that, like a kernel log or something.

clk_register_clkdev() should not have changed in any way, since the
change there effectively changes the sequence from:

va_start(ap, dev_fmt);
-   cl = vclkdev_alloc(__clk_get_hw(clk), con_id, dev_fmt, ap);
va_end(ap);
-   if (!cl)
-   return -ENOMEM;
-   clkdev_add(cl);
-   return 0;

to:

va_start(ap, dev_fmt);
+   cl = vclkdev_alloc(__clk_get_hw(clk), con_id, dev_fmt, ap);
+   if (cl)
+   __clkdev_add(cl);
va_end(ap);
+   return cl ? 0 : -ENOMEM;

So I'm guessing this isn't the problem.

However, clk_add_alias() changes slightly, from:

fmt = alias_dev_name;
va_start(ap, fmt);
l = vclkdev_alloc(__clk_get_hw(clk), con_id, fmt, ap)
va_end(ap);
-   if (!l)
-   return -ENODEV;
-   clkdev_add(l);
-   return 0;

to (effectively):

fmt = "%s"
va_start(ap, fmt);
cl = vclkdev_alloc(__clk_get_hw(clk), con_id, fmt, ap);
if (cl)
clkdev_add(cl);
va_end(ap);
+   return l ? 0 : -ENODEV;

In other words, there's the addition of a "%s" in the format string
which wasn't there before - which is reasonable as clk_add_alias()
doesn't take a format string.  This should improve the safety of the
function.

I guess things might go wrong if you pass a NULL alias device name?

Can you try this patch please?

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index c0eaf0973bd2..779b6ff0c7ad 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -333,7 +333,8 @@ int clk_add_alias(const char *alias, const char 
*alias_dev_name,
if (IS_ERR(r))
return PTR_ERR(r);
 
-   l = clkdev_create(r, alias, "%s", alias_dev_name);
+   l = clkdev_create(r, alias, alias_dev_name ? "%s" : NULL,
+ alias_dev_name);
clk_put(r);
 
return l ? 0 : -ENODEV;


-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BISECTED] Nokia 770 framebuffer breakage

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 01:24:13AM +0300, Aaro Koskinen wrote:
> On Mon, Oct 19, 2015 at 09:05:22PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 19, 2015 at 10:37:59PM +0300, Aaro Koskinen wrote:
> > > Hi,
> > > 
> > > Somewhere between 4.1 .. 4.3-rc6 framebuffer on Nokia 770 stopped working.
> > > 
> > > Bisection points to:
> > > 
> > > commit 2568999835d7797afce3dcc3a3f368051ffcaf1f
> > > Author: Russell King <rmk+ker...@arm.linux.org.uk>
> > > Date:   Mon Mar 2 15:40:29 2015 +
> > > 
> > > clkdev: add clkdev_create() helper
> > > 
> > > The commit cannot be reverted cleanly from current trees, but I re-tested
> > > that commit 2568999835d7797afce3dcc3a3f368051ffcaf1f does not work and
> > > commit 2568999835d7797afce3dcc3a3f368051ffcaf1f^1 indeed works.
> > 
> > Really need more information than that, like a kernel log or something.
> 
> This is a device with normal consumer electronics mechnics so serial
> port access is PITA. However I managed to get a boot log, see below,
> it's from commit 2568999835d7797afce3dcc3a3f368051ffcaf1f.

Hmm, that points to a driver that doesn't bother checking whether it
successfully got any resources:

hwa742.sys_ck = clk_get(NULL, "hwa_sys_ck");

spin_lock_init(_lock);

if ((r = hwa742.int_ctrl->init(fbdev, 1, req_vram)) < 0)
goto err1;

if ((r = hwa742.extif->init(fbdev)) < 0)
goto err2;

ext_clk = clk_get_rate(hwa742.sys_ck);

There's no check here that clk_get() actually worked, and this will be
why you then end up with the division by zeros.  That's someone elses
problem though. :)

> > Can you try this patch please?
> 
> Unfortunately with the patch it dies even earlier, without any output.

It shouldn't (I've been through the resulting assembly code.)  I wonder
if this is fixing the problem, but it's now failing elsewhere instead.

What happens if you change it to:

l = clkdev_create(r, alias, alias_dev_name);

which should be 100% identical to how older kernels behaved - if that
works, can you print out 'alias' and 'alias_dev_name' for every call
to clk_add_alias() and send that please?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] ARM: catch pending imprecise abort on unmask

[Russell King - ARM Linux]

On Wed, Oct 14, 2015 at 04:48:30PM +0200, Lucas Stach wrote:
> Install a non-faulting handler just before unmasking imprecise aborts
> and switch back to the regular one after unmasking is done.
> 
> This catches any pending imprecise abort that the firmware/bootloader
> may have left behind that would normally crash the kernel at that point.
> As there are apparently a lot of bootlaoders out there that do such a
> thing it makes sense to handle it in the common startup code.
> 
> Signed-off-by: Lucas Stach 
> ---
>  arch/arm/mm/fault.c | 15 +++
>  arch/arm/mm/fault.h |  2 ++
>  arch/arm/mm/mmu.c   | 19 ++-
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index 0d629b8f973f..519f694ec9db 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -538,6 +538,21 @@ hook_fault_code(int nr, int (*fn)(unsigned long, 
> unsigned int, struct pt_regs *)
>   fsr_info[nr].name = name;
>  }
>  
> +void * __init
> +swap_fault_function(int nr,
> + int (*fn)(unsigned long, unsigned int, struct pt_regs *))
> +{
> + void *old_fn;
> +
> + if (nr < 0 || nr >= ARRAY_SIZE(fsr_info))
> + BUG();
> +
> + old_fn = fsr_info[nr].fn;
> + fsr_info[nr].fn = fn;
> +
> + return old_fn;
> +}
> +

Please move the abort enable and handler into fault.c - I don't want this
exposed to the rest of the kernel as people will get the impression that
they can make use of this, even though it's not in arch/arm/include/, as
a way to hook and then subsequently unhook a handler.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] ARM: catch pending imprecise abort on unmask

[Russell King - ARM Linux]

On Thu, Oct 15, 2015 at 12:32:20PM +0200, Lucas Stach wrote:
> Install a non-faulting handler just before unmasking imprecise aborts
> and switch back to the regular one after unmasking is done.
> 
> This catches any pending imprecise abort that the firmware/bootloader
> may have left behind that would normally crash the kernel at that point.
> As there are apparently a lot of bootlaoders out there that do such a
> thing it makes sense to handle it in the common startup code.
> 
> Signed-off-by: Lucas Stach 

Much better.  Please feel free to add it to the patch system, thanks.

I think, given that the original seems to be breaking platforms, this
patch needs to go into -rc kernels, right?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] ARM: catch pending imprecise abort on unmask

[Russell King - ARM Linux]

On Thu, Oct 15, 2015 at 08:39:15AM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <li...@arm.linux.org.uk> [151015 08:37]:
> > On Thu, Oct 15, 2015 at 12:32:20PM +0200, Lucas Stach wrote:
> > > Install a non-faulting handler just before unmasking imprecise aborts
> > > and switch back to the regular one after unmasking is done.
> > > 
> > > This catches any pending imprecise abort that the firmware/bootloader
> > > may have left behind that would normally crash the kernel at that point.
> > > As there are apparently a lot of bootlaoders out there that do such a
> > > thing it makes sense to handle it in the common startup code.
> > > 
> > > Signed-off-by: Lucas Stach <l.st...@pengutronix.de>
> > 
> > Much better.  Please feel free to add it to the patch system, thanks.
> > 
> > I think, given that the original seems to be breaking platforms, this
> > patch needs to go into -rc kernels, right?
> 
> Hmm do we still see a trace where the issue happened though with this
> one?

That's not the intention of this specific patch.

This is solely to detect the bootloader induced imprecise exception,
nothing more.  A backtrace for that won't be useful in any shape or
form - in fact, the backtrace will be well known (it'll be from the
site where the imprecise exceptions are unmasked.)

Any imprecise exception which happens after this will be handled in
the normal way: it'll raise a kernel oops, and that will have all the
details that a kernel oops normally has.

The difference is, rather than the boot loader provoking the kernel to
oops at this point, we're able to report the event and continue on.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP2: erratum I688 handling disabled for AM335x

[Russell King - ARM Linux]

On Tue, Oct 13, 2015 at 12:10:45PM +, Woodruff, Richard wrote:
> > From: Lucas Stach [mailto:l.st...@pengutronix.de]
> > Sent: Tuesday, October 13, 2015 5:01 AM
> 
> > So please help me to get this straight:
> > 
> > Errata I688 only affects OMAP4 which is consequently the only user of
> > omap_interconnect_sync() in it's WFI enter sequence, which in turn is
> > the only user of the SRAM scratch area to work around the erratum.
> > 
> > The OMAP specific barrier implementation which should be used also on
> > other SoCs does not need any SRAM scratch, but uses a part of DRAM to do
> > the strongly ordered access.
> > 
> > So it is safe to say that we only ever need to run the initcall
> > allocating the SRAM scratch area on OMAP4.
> 
> There are 2 separate things here.  One is the bus sync function and the
> other is the errata which requires a bus sync near WFI to avoid an errata.
> 
> The rational for the bus sync is similar to why there is a writel() and a
> writel_releaxed().  The bus sync has been used for a long time to ensure
> writes have landed and are not stuck in some posting buffer on path.
> 
> A lot of historical drivers use a writel() where perhaps they could choose
> a more granular construct.  If drivers were audited maybe the bus sync
> could be minimized on writel() path.

No, we're not going around that discussion loop again.

Linux requirements are that writel() at the CPU should be ordered with
respect to other writel()s and memory accesses which occur before the
writel().

However, buffering of the write by down-stream busses is permitted, and
where drivers want to ensure that the write has hit the device, a
read-back must be performed.  This requirement comes directly from the
PCI specification, and is *not* actually something that is specific to
Linux.  Linux only adopts it from PCI.

We're not going to ever relax these rules: if people want to perform
accesses which do not conform to the above, they are free to - if they
don't care about the timing of the write hitting the device, they can
omit the read-back.  If they don't care about the write being ordered,
they can use writel_relaxed() (relaxed, because it doesn't have the
ordering guarantees of standard writel().)

It's up to the driver author to use the correct accessor(s) in their
drivers.  It's not for the architecture to decide that it can relax
these rules (if it does, it risks breaking a load of drivers out there.)

So, if people want to avoid the expensive OMAP bus sync on every access
in their drivers, they _have_ to consider whether each load or store
needs to be ordered.  In general, a sequence of writes to a device
should be implemented as a sequence of writel_relaxed(), and if it needs
to be ordered, the last write should be a writel() or accompanied by a
barrier.  An example of this would be writing DMA controller configuration.
All those writes should be writel_relaxed() except for the final writel()
which kicks off the DMA operation.

If you implement drivers using nothing but writel() and readl(), then your
performance _will_ suck, but that's entirely the driver's fault.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sched_clock: add data pointer argument to read callback

[Russell King - ARM Linux]

On Sat, Oct 10, 2015 at 01:42:47AM +0100, Måns Rullgård wrote:
> Russell King - ARM Linux <li...@arm.linux.org.uk> writes:
> 
> > On Sat, Oct 10, 2015 at 12:48:22AM +0100, Måns Rullgård wrote:
> >> Russell King - ARM Linux <li...@arm.linux.org.uk> writes:
> >> 
> >> > On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote:
> >> >> This passes a data pointer specified in the sched_clock_register()
> >> >> call to the read callback allowing simpler implementations thereof.
> >> >> 
> >> >> In this patch, existing uses of this interface are simply updated
> >> >> with a null pointer.
> >> >
> >> > This is a bad description.  It tells us what the patch is doing,
> >> > (which we can see by reading the patch) but not _why_.  Please include
> >> > information on why the change is necessary - describe what you are
> >> > trying to achieve.
> >> 
> >> Currently most of the callbacks use a global variable to store the
> >> address of a counter register.  This has several downsides:
> >> 
> >> - Loading the address of a global variable can be more expensive than
> >>   keeping a pointer next to the function pointer.
> >> 
> >> - It makes it impossible to have multiple instances of a driver call
> >>   sched_clock_register() since the caller can't know which clock will
> >>   win in the end.
> >> 
> >> - Many of the existing callbacks are practically identical and could be
> >>   replaced with a common generic function if it had a pointer argument.
> >> 
> >> If I've missed something that makes this a stupid idea, please tell.
> >
> > So my next question is whether you intend to pass an iomem pointer
> > through this, or a some kind of structure, or both.  It matters,
> > because iomem pointers have a __iomem attribute to keep sparse
> > happy.  Having to force that attribute on and off pointers is frowned
> > upon, as it defeats the purpose of the sparse static checker.
> 
> So this is an instance where tools like sparse get in the way of doing
> the simplest, most efficient, and obviously correct thing.  Who wins in
> such cases?

In that case, NAK on the patch.  I don't have time for your stupid games.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sched_clock: add data pointer argument to read callback

[Russell King - ARM Linux]

On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote:
> This passes a data pointer specified in the sched_clock_register()
> call to the read callback allowing simpler implementations thereof.
> 
> In this patch, existing uses of this interface are simply updated
> with a null pointer.

This is a bad description.  It tells us what the patch is doing,
(which we can see by reading the patch) but not _why_.  Please include
information on why the change is necessary - describe what you are
trying to achieve.

I generally don't accept patches what add new stuff to the kernel with
no users of that new stuff - that's called experience, experience of
people who submit stuff like that, and then vanish leaving their junk
in the kernel without any users.  Please ensure that this gets a user
very quickly, or better still, submit this patch as part of a series
which makes use of it.

Also, copying soo many people is guaranteed to be silently dropped by
mailing lists.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sched_clock: add data pointer argument to read callback

[Russell King - ARM Linux]

On Sat, Oct 10, 2015 at 12:48:22AM +0100, Måns Rullgård wrote:
> Russell King - ARM Linux <li...@arm.linux.org.uk> writes:
> 
> > On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote:
> >> This passes a data pointer specified in the sched_clock_register()
> >> call to the read callback allowing simpler implementations thereof.
> >> 
> >> In this patch, existing uses of this interface are simply updated
> >> with a null pointer.
> >
> > This is a bad description.  It tells us what the patch is doing,
> > (which we can see by reading the patch) but not _why_.  Please include
> > information on why the change is necessary - describe what you are
> > trying to achieve.
> 
> Currently most of the callbacks use a global variable to store the
> address of a counter register.  This has several downsides:
> 
> - Loading the address of a global variable can be more expensive than
>   keeping a pointer next to the function pointer.
> 
> - It makes it impossible to have multiple instances of a driver call
>   sched_clock_register() since the caller can't know which clock will
>   win in the end.
> 
> - Many of the existing callbacks are practically identical and could be
>   replaced with a common generic function if it had a pointer argument.
> 
> If I've missed something that makes this a stupid idea, please tell.

So my next question is whether you intend to pass an iomem pointer
through this, or a some kind of structure, or both.  It matters,
because iomem pointers have a __iomem attribute to keep sparse
happy.  Having to force that attribute on and off pointers is frowned
upon, as it defeats the purpose of the sparse static checker.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Thu, Oct 08, 2015 at 01:40:21AM -0700, Tony Lindgren wrote:
> Well the way distros deal with issues like this is have everything
> possible as loadable modules. We should get the regulator_pbiaa
> loaded automatically in that case as long as it's in the dts.. And
> as long as we have the MODULE_DEVICE_TABLE entries right.

Assuming you have a rootfs which doesn't depend on one of those modules,
or an initramfs, and a way to get the built modules into that initial
filesystem.  Automated test boot systems do not have that luxury: the
generation of initramfs is distro specific, and would be very large if
it were to include all possible modules.

Some distros need their initramfs statically configured between "mount
a local rootfs" and "mount a nfs rootfs" and can't be changed once
generated.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Thu, Oct 08, 2015 at 02:56:36AM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <li...@arm.linux.org.uk> [151008 02:40]:
> > On Thu, Oct 08, 2015 at 01:40:21AM -0700, Tony Lindgren wrote:
> > > Well the way distros deal with issues like this is have everything
> > > possible as loadable modules. We should get the regulator_pbiaa
> > > loaded automatically in that case as long as it's in the dts.. And
> > > as long as we have the MODULE_DEVICE_TABLE entries right.
> > 
> > Assuming you have a rootfs which doesn't depend on one of those modules,
> > or an initramfs, and a way to get the built modules into that initial
> > filesystem.  Automated test boot systems do not have that luxury: the
> > generation of initramfs is distro specific, and would be very large if
> > it were to include all possible modules.
> 
> Right, we need to keep the kernel usable in all these configurations
> somehow.
> 
> For the kernel generated minimal initramfs we could grep for the
> needed modules in the dts file.

Yes, you might be able to do that, but the only thing that can generate
the initramfs is the target system itself, or a similar target system.
For example, you can't sanely generate an initramfs for an ARM Ubuntu
system on an x86 Fedora host.

Distros have the luxury of always being able to do the initramfs
generation on the target system at installation or distro kernel update
time.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Tue, Oct 06, 2015 at 08:37:11AM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <li...@arm.linux.org.uk> [151006 08:04]:
> > On Tue, Oct 06, 2015 at 02:44:25AM -0700, Tony Lindgren wrote:
> > > 
> > > Hmm DT-based boot finds the MMC card for LDP, dmesg below from DT boot 
> > > [1].
> > > Looks like you're on on -rc4 and not -rc3. My guess is that MMC is not
> > > working for you with DT-based booting because you don't seem to have
> > > CONFIG_REGULATOR_PBIAS in your seed config for. Care to try enabling that
> > > for both your omap3 and omap4 seed config files?
> > 
> > This is precisely the kind of crap I'm objecting to.  New kernel versions
> > come along, and things break because people add extra Kconfig symbols that
> > previous versions did not rely upon - and there's no communication of
> > what's required for new kernel versions.
> > 
> > This stuff needs documenting, so that people are aware what changes they
> > need to make - please put something in Documentation/arm/OMAP which
> > tracks what new additions are required to the Kconfig to keep things
> > working.
> 
> Good idea, how about something like the following? AFAIK that's the
> only .config option needed as MFD_SYSCON is selected by Kconfig
> already.

Right, that resolves the issue.  Now, what's the story on the revert
and the other patch - have they already been taken for -rc kernels?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Wed, Oct 07, 2015 at 03:41:46PM +0200, Ulf Hansson wrote:
> > http://marc.info/?l=linux-omap=144422416621373=2
> > http://marc.info/?l=linux-omap=144422416921375=2
> 
> Russell, may I add your tested by tag for these?

You may -

Tested-by: Russell King <rmk+ker...@arm.linux.org.uk>

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Tue, Oct 06, 2015 at 04:06:09PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Tuesday 06 October 2015 03:41 PM, Ulf Hansson wrote:
> > On 6 October 2015 at 11:44, Tony Lindgren <t...@atomide.com> wrote:
> >> * Russell King - ARM Linux <li...@arm.linux.org.uk> [151006 02:04]:
> >>> On Mon, Oct 05, 2015 at 07:38:13PM +0100, Russell King - ARM Linux wrote:
> >>>> On Mon, Oct 05, 2015 at 10:11:56AM -0700, Tony Lindgren wrote:
> >>>>> * Tony Lindgren <t...@atomide.com> [151005 07:57]:
> >>>>>> * Tony Lindgren <t...@atomide.com> [151005 07:44]:
> >>>>>>> * Tony Lindgren <t...@atomide.com> [151005 04:28]:
> >>>>>>>
> >>>>>>> Based on some tests it seems that the duovero unpaired regulator usage
> >>>>>>> is fixed by reverting:
> >>>>>>>
> >>>>>>> c55d7a055364 ("mmc: host: omap_hsmmc: use regulator_is_enabled to
> >>>>>>> find pbias status")
> >>>>>>
> >>>>>> With commit c55d7a055364 my guess is that the PBIAS regulator is
> >>>>>> already on from an earlier MMC probe and getting re-enabled when
> >>>>>> a deferred probe happens?
> >>>>>
> >>>>> Unless somebody has a better fix in mind for the above, I suggest
> >>>>> we revert it for the -rc kernel.
> >>>>
> >>>> Let me try reverting that in my build tree, and...
> >>>>
> >>>>>>> And it seems that omap3 legacy MMC is broken earlier in the
> >>>>>>> series with:
> >>>>>>>
> >>>>>>> 7d607f917008 ("mmc: host: omap_hsmmc: use
> >>>>>>> devm_regulator_get_optional() for vmmc")
> >>>>>>>
> >>>>>>> This one does not cleanly revert so have not yet tried reverting
> >>>>>>> it.
> >>>>>>
> >>>>>> And with commit 7d607f917008 I'm guessing we can't return anHi,
> >>>>>> error if the PBIAS regulator does not exist as that's not there
> >>>>>> for the legacy booting.
> >>>>>
> >>>>> For omap3 legacy booting, we keep getting -EPROBE_DEFER for
> >>>>> all the optional regulators.
> >>>>>
> >>>>> Something like the following might be the minimal fix for the -rc
> >>>>> cycle?
> >>>>
> >>>> applying this patch.  If that gets things going again, then we
> >>>> _definitely_ should get both of these to Linus ASAP.  The breakage
> >>>> has been around far too long already.
> >>>
> >>> Last night's build shows that this fixes the non-DT LDP3430 booting, but
> >>> DT-based LDP3430 and SDP4430 both remain broken for the same reason -
> >>> neither can find their SD cards.
> >>
> >> Hmm DT-based boot finds the MMC card for LDP, dmesg below from DT boot [1].
> >> Looks like you're on on -rc4 and not -rc3. My guess is that MMC is not
> >> working for you with DT-based booting because you don't seem to have
> >> CONFIG_REGULATOR_PBIAS in your seed config for. Care to try enabling that
> >> for both your omap3 and omap4 seed config files?
> >>
> >>> We're at -rc4.  That means we're could be only three weeks away from 4.3
> >>> being released, and DT OMAP has yet to boot _once_ here.
> >>>
> >>> What I find *totally* unacceptable is the lack of reaction from the MMC
> >>> and TI people: it's just like "we'll break your crap, and we'll ignore
> >>> reports of regressions."  That is *not* acceptable in any shape or form,
> >>> and people who do this _should_ have their future patches ignored until
> >>> they demonstrate that they understand the need to not break stuff.
> >>>
> >>> I'm at the point of suggesting that there should be a moritorium on all
> >>> OMAP related development for 4.4: delay all development to 4.5, and have
> >>> 4.4 as a pure bug-fixing _only_ cycle for OMAP.  If 4.3 is released and
> >>> OMAP is still broken, then I don't think there's an option on that.
> >>>
> >>> Maybe the idea that development work won't hit mainline for another 4
> >>> months will help focus the minds on not breaking stuff and then ignoring
> >>> r

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Tue, Oct 06, 2015 at 02:44:25AM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <li...@arm.linux.org.uk> [151006 02:04]:
> > On Mon, Oct 05, 2015 at 07:38:13PM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Oct 05, 2015 at 10:11:56AM -0700, Tony Lindgren wrote:
> > > > * Tony Lindgren <t...@atomide.com> [151005 07:57]:
> > > > > * Tony Lindgren <t...@atomide.com> [151005 07:44]:
> > > > > > * Tony Lindgren <t...@atomide.com> [151005 04:28]:
> > > > > > 
> > > > > > Based on some tests it seems that the duovero unpaired regulator 
> > > > > > usage
> > > > > > is fixed by reverting:
> > > > > > 
> > > > > > c55d7a055364 ("mmc: host: omap_hsmmc: use regulator_is_enabled to
> > > > > > find pbias status")
> > > > > 
> > > > > With commit c55d7a055364 my guess is that the PBIAS regulator is
> > > > > already on from an earlier MMC probe and getting re-enabled when
> > > > > a deferred probe happens?
> > > > 
> > > > Unless somebody has a better fix in mind for the above, I suggest
> > > > we revert it for the -rc kernel.
> > > 
> > > Let me try reverting that in my build tree, and...
> > > 
> > > > > > And it seems that omap3 legacy MMC is broken earlier in the
> > > > > > series with:
> > > > > > 
> > > > > > 7d607f917008 ("mmc: host: omap_hsmmc: use
> > > > > > devm_regulator_get_optional() for vmmc")
> > > > > > 
> > > > > > This one does not cleanly revert so have not yet tried reverting
> > > > > > it.
> > > > > 
> > > > > And with commit 7d607f917008 I'm guessing we can't return an
> > > > > error if the PBIAS regulator does not exist as that's not there
> > > > > for the legacy booting.
> > > > 
> > > > For omap3 legacy booting, we keep getting -EPROBE_DEFER for
> > > > all the optional regulators.
> > > > 
> > > > Something like the following might be the minimal fix for the -rc
> > > > cycle?
> > > 
> > > applying this patch.  If that gets things going again, then we
> > > _definitely_ should get both of these to Linus ASAP.  The breakage
> > > has been around far too long already.
> > 
> > Last night's build shows that this fixes the non-DT LDP3430 booting, but
> > DT-based LDP3430 and SDP4430 both remain broken for the same reason -
> > neither can find their SD cards.
> 
> Hmm DT-based boot finds the MMC card for LDP, dmesg below from DT boot [1].
> Looks like you're on on -rc4 and not -rc3. My guess is that MMC is not
> working for you with DT-based booting because you don't seem to have
> CONFIG_REGULATOR_PBIAS in your seed config for. Care to try enabling that
> for both your omap3 and omap4 seed config files?

This is precisely the kind of crap I'm objecting to.  New kernel versions
come along, and things break because people add extra Kconfig symbols that
previous versions did not rely upon - and there's no communication of
what's required for new kernel versions.

This stuff needs documenting, so that people are aware what changes they
need to make - please put something in Documentation/arm/OMAP which
tracks what new additions are required to the Kconfig to keep things
working.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Wed, Oct 07, 2015 at 12:59:29AM +0530, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Tuesday 06 October 2015 08:37 PM, Russell King - ARM Linux wrote:
> > On Tue, Oct 06, 2015 at 04:06:09PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi,
> >>
> >> On Tuesday 06 October 2015 03:41 PM, Ulf Hansson wrote:
> >>> On 6 October 2015 at 11:44, Tony Lindgren <t...@atomide.com> wrote:
> >>>> * Russell King - ARM Linux <li...@arm.linux.org.uk> [151006 02:04]:
> >>>>> On Mon, Oct 05, 2015 at 07:38:13PM +0100, Russell King - ARM Linux 
> >>>>> wrote:
> >>>>>> On Mon, Oct 05, 2015 at 10:11:56AM -0700, Tony Lindgren wrote:
> >>>>>>> * Tony Lindgren <t...@atomide.com> [151005 07:57]:
> >>>>>>>> * Tony Lindgren <t...@atomide.com> [151005 07:44]:
> >>>>>>>>> * Tony Lindgren <t...@atomide.com> [151005 04:28]:
> >>>>>>>>>
> >>>>>>>>> Based on some tests it seems that the duovero unpaired regulator 
> >>>>>>>>> usage
> >>>>>>>>> is fixed by reverting:
> >>>>>>>>>
> >>>>>>>>> c55d7a055364 ("mmc: host: omap_hsmmc: use regulator_is_enabled to
> >>>>>>>>> find pbias status")
> >>>>>>>>
> >>>>>>>> With commit c55d7a055364 my guess is that the PBIAS regulator is
> >>>>>>>> already on from an earlier MMC probe and getting re-enabled when
> >>>>>>>> a deferred probe happens?
> >>>>>>>
> >>>>>>> Unless somebody has a better fix in mind for the above, I suggest
> >>>>>>> we revert it for the -rc kernel.
> >>>>>>
> >>>>>> Let me try reverting that in my build tree, and...
> >>>>>>
> >>>>>>>>> And it seems that omap3 legacy MMC is broken earlier in the
> >>>>>>>>> series with:
> >>>>>>>>>
> >>>>>>>>> 7d607f917008 ("mmc: host: omap_hsmmc: use
> >>>>>>>>> devm_regulator_get_optional() for vmmc")
> >>>>>>>>>
> >>>>>>>>> This one does not cleanly revert so have not yet tried reverting
> >>>>>>>>> it.
> >>>>>>>>
> >>>>>>>> And with commit 7d607f917008 I'm guessing we can't return anHi,
> >>>>>>>> error if the PBIAS regulator does not exist as that's not there
> >>>>>>>> for the legacy booting.
> >>>>>>>
> >>>>>>> For omap3 legacy booting, we keep getting -EPROBE_DEFER for
> >>>>>>> all the optional regulators.
> >>>>>>>
> >>>>>>> Something like the following might be the minimal fix for the -rc
> >>>>>>> cycle?
> >>>>>>
> >>>>>> applying this patch.  If that gets things going again, then we
> >>>>>> _definitely_ should get both of these to Linus ASAP.  The breakage
> >>>>>> has been around far too long already.
> >>>>>
> >>>>> Last night's build shows that this fixes the non-DT LDP3430 booting, but
> >>>>> DT-based LDP3430 and SDP4430 both remain broken for the same reason -
> >>>>> neither can find their SD cards.
> >>>>
> >>>> Hmm DT-based boot finds the MMC card for LDP, dmesg below from DT boot 
> >>>> [1].
> >>>> Looks like you're on on -rc4 and not -rc3. My guess is that MMC is not
> >>>> working for you with DT-based booting because you don't seem to have
> >>>> CONFIG_REGULATOR_PBIAS in your seed config for. Care to try enabling that
> >>>> for both your omap3 and omap4 seed config files?
> >>>>
> >>>>> We're at -rc4.  That means we're could be only three weeks away from 4.3
> >>>>> being released, and DT OMAP has yet to boot _once_ here.
> >>>>>
> >>>>> What I find *totally* unacceptable is the lack of reaction from the MMC
> >>>>> and TI people: it's just like "we'll break your crap, and we'll ignore
> >>>>> reports of regressions."  That is *not* acceptable in any sha

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Mon, Oct 05, 2015 at 07:38:13PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 05, 2015 at 10:11:56AM -0700, Tony Lindgren wrote:
> > * Tony Lindgren <t...@atomide.com> [151005 07:57]:
> > > * Tony Lindgren <t...@atomide.com> [151005 07:44]:
> > > > * Tony Lindgren <t...@atomide.com> [151005 04:28]:
> > > > 
> > > > Based on some tests it seems that the duovero unpaired regulator usage
> > > > is fixed by reverting:
> > > > 
> > > > c55d7a055364 ("mmc: host: omap_hsmmc: use regulator_is_enabled to
> > > > find pbias status")
> > > 
> > > With commit c55d7a055364 my guess is that the PBIAS regulator is
> > > already on from an earlier MMC probe and getting re-enabled when
> > > a deferred probe happens?
> > 
> > Unless somebody has a better fix in mind for the above, I suggest
> > we revert it for the -rc kernel.
> 
> Let me try reverting that in my build tree, and...
> 
> > > > And it seems that omap3 legacy MMC is broken earlier in the
> > > > series with:
> > > > 
> > > > 7d607f917008 ("mmc: host: omap_hsmmc: use
> > > > devm_regulator_get_optional() for vmmc")
> > > > 
> > > > This one does not cleanly revert so have not yet tried reverting
> > > > it.
> > > 
> > > And with commit 7d607f917008 I'm guessing we can't return an
> > > error if the PBIAS regulator does not exist as that's not there
> > > for the legacy booting.
> > 
> > For omap3 legacy booting, we keep getting -EPROBE_DEFER for
> > all the optional regulators.
> > 
> > Something like the following might be the minimal fix for the -rc
> > cycle?
> 
> applying this patch.  If that gets things going again, then we
> _definitely_ should get both of these to Linus ASAP.  The breakage
> has been around far too long already.

Last night's build shows that this fixes the non-DT LDP3430 booting, but
DT-based LDP3430 and SDP4430 both remain broken for the same reason -
neither can find their SD cards.

We're at -rc4.  That means we're could be only three weeks away from 4.3
being released, and DT OMAP has yet to boot _once_ here.

What I find *totally* unacceptable is the lack of reaction from the MMC
and TI people: it's just like "we'll break your crap, and we'll ignore
reports of regressions."  That is *not* acceptable in any shape or form,
and people who do this _should_ have their future patches ignored until
they demonstrate that they understand the need to not break stuff.

I'm at the point of suggesting that there should be a moritorium on all
OMAP related development for 4.4: delay all development to 4.5, and have
4.4 as a pure bug-fixing _only_ cycle for OMAP.  If 4.3 is released and
OMAP is still broken, then I don't think there's an option on that.

Maybe the idea that development work won't hit mainline for another 4
months will help focus the minds on not breaking stuff and then ignoring
regression reports.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: All OMAP platforms: MMC is broken

[Russell King - ARM Linux]

On Mon, Oct 05, 2015 at 10:11:56AM -0700, Tony Lindgren wrote:
> * Tony Lindgren  [151005 07:57]:
> > * Tony Lindgren  [151005 07:44]:
> > > * Tony Lindgren  [151005 04:28]:
> > > 
> > > Based on some tests it seems that the duovero unpaired regulator usage
> > > is fixed by reverting:
> > > 
> > > c55d7a055364 ("mmc: host: omap_hsmmc: use regulator_is_enabled to
> > > find pbias status")
> > 
> > With commit c55d7a055364 my guess is that the PBIAS regulator is
> > already on from an earlier MMC probe and getting re-enabled when
> > a deferred probe happens?
> 
> Unless somebody has a better fix in mind for the above, I suggest
> we revert it for the -rc kernel.

Let me try reverting that in my build tree, and...

> > > And it seems that omap3 legacy MMC is broken earlier in the
> > > series with:
> > > 
> > > 7d607f917008 ("mmc: host: omap_hsmmc: use
> > > devm_regulator_get_optional() for vmmc")
> > > 
> > > This one does not cleanly revert so have not yet tried reverting
> > > it.
> > 
> > And with commit 7d607f917008 I'm guessing we can't return an
> > error if the PBIAS regulator does not exist as that's not there
> > for the legacy booting.
> 
> For omap3 legacy booting, we keep getting -EPROBE_DEFER for
> all the optional regulators.
> 
> Something like the following might be the minimal fix for the -rc
> cycle?

applying this patch.  If that gets things going again, then we
_definitely_ should get both of these to Linus ASAP.  The breakage
has been around far too long already.

> 8< 
> From: Tony Lindgren 
> Date: Mon, 5 Oct 2015 09:37:36 -0700
> Subject: [PATCH] mmc: host: omap_hsmmc: Fix MMC for omap3 legacy booting
> 
> Starting with commit 7d607f917008 ("mmc: host: omap_hsmmc: use
> devm_regulator_get_optional() for vmmc") MMC on omap3 stopped working
> for legacy booting.
> 
> This is because legacy booting sets up some of the resource in the
> platform init code, and for optional regulators always seem to
> return -EPROBE_DEFER for the legacy booting.
> 
> Let's fix the issue by checking for device tree based booting for
> now. Then when omap3 boots in device tree only mode, this patch
> can be just reverted.
> 
> Fixes: 7d607f917008 ("mmc: host: omap_hsmmc: use
> devm_regulator_get_optional() for vmmc")
> Signed-off-by: Tony Lindgren 
> 
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -478,7 +478,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host 
> *host)
>   mmc->supply.vmmc = devm_regulator_get_optional(host->dev, "vmmc");
>   if (IS_ERR(mmc->supply.vmmc)) {
>   ret = PTR_ERR(mmc->supply.vmmc);
> - if (ret != -ENODEV)
> + if ((ret != -ENODEV) && host->dev->of_node)
>   return ret;
>   dev_dbg(host->dev, "unable to get vmmc regulator %ld\n",
>   PTR_ERR(mmc->supply.vmmc));
> @@ -493,7 +493,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host 
> *host)
>   mmc->supply.vqmmc = devm_regulator_get_optional(host->dev, "vmmc_aux");
>   if (IS_ERR(mmc->supply.vqmmc)) {
>   ret = PTR_ERR(mmc->supply.vqmmc);
> - if (ret != -ENODEV)
> + if ((ret != -ENODEV) && host->dev->of_node)
>   return ret;
>   dev_dbg(host->dev, "unable to get vmmc_aux regulator %ld\n",
>   PTR_ERR(mmc->supply.vqmmc));
> @@ -503,7 +503,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host 
> *host)
>   host->pbias = devm_regulator_get_optional(host->dev, "pbias");
>   if (IS_ERR(host->pbias)) {
>   ret = PTR_ERR(host->pbias);
> - if (ret != -ENODEV)
> + if ((ret != -ENODEV) && host->dev->of_node)
>   return ret;
>   dev_dbg(host->dev, "unable to get pbias regulator %ld\n",
>   PTR_ERR(host->pbias));

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Russell King - ARM Linux]

On Mon, Sep 28, 2015 at 11:01:34AM +0200, Arnaud Pouliquen wrote:
> few questions/remarks
> BR,
> Arnaud
> 
> >+static void hdmi_codec_abort(struct device *dev)
> >+{
> >+struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
> >+
> >+dev_dbg(dev, "%s()\n", __func__);
> >+
> >+mutex_lock(>current_stream_lock);
> >+if (hcp->current_stream && hcp->current_stream->runtime &&
> >+snd_pcm_running(hcp->current_stream)) {
> >+dev_info(dev, "HDMI audio playback aborted\n");
> >+snd_pcm_stream_lock_irq(hcp->current_stream);
> >+snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
> >+snd_pcm_stream_unlock_irq(hcp->current_stream);
> >+}
> >+mutex_unlock(>current_stream_lock);
> >+}
> Does driver should stop the stream in case of unplug?
> This could generate unexpected behavior at application level.
> Perhaps should be better if this part is managed in DRM driver. if HDMI
> master, I2S bus should be maintained ON to consume the audio stream until
> application action.

If it does, that's really horrid.

Firstly, do you expect your video playback to stop just because you've
unplugged the TV?  Maybe you've got a dumb HDMI switch, and you've
switched from one input to another to momentarily check something on
another input - should the video playback suddenly end up with its
audio path being dumped on the floor because of that?

Also, as I've said before, there's hardware out there where the SPDIF
audio output is routed to more than just the HDMI interface, and the
capabilities of HDMI really don't apply in that situation - you may
have an AV receiver connected to the SPDIF output which is able to
accept much more than the basic audio that most TVs accept.  Having
stuff restricted to the union of the abilities is far too restrictive.
Stopping the audio output because the TV went away in this case is also
plain idiotic.

SPDIF is something that can be routed to multiple devices simultaneously,
and there's no capability or connection reporting involved in it.
Imposing the status of one "SPDIF listener" on the entire audio system is
unreasonable.

> >+
> >+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
> >+struct snd_pcm_hw_params *params,
> >+struct snd_soc_dai *dai)
> >+{
> >+struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
> >+struct hdmi_codec_params hp = {
> >+.iec = {
> >+.status = { 0 },
> >+.subcode = { 0 },
> >+.pad = 0,
> >+.dig_subframe = { 0 },
> >+}
> >+};
> >+int ret;
> >+
> >+dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
> >+params_width(params), params_rate(params),
> >+params_channels(params));
> >+
> >+ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
> >+   sizeof(hp.iec.status));
> Tell me if i am wrong, but in case of SPDIF format, IEC status is managed by
> cpu_dai not by the codec.

Correct.  I2S needs the IEC958 status programmed into the HDMI interface
though, because HDMI is basically SPDIF.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: OMAP2: erratum I688 handling disabled for AM335x

[Russell King - ARM Linux]

On Fri, Sep 25, 2015 at 12:01:13PM +0200, Bastian Stender wrote:
> Signed-off-by: Bastian Stender 
> ---
>  arch/arm/mach-omap2/omap4-common.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap4-common.c 
> b/arch/arm/mach-omap2/omap4-common.c
> index 949696b..a3a0cd1 100644
> --- a/arch/arm/mach-omap2/omap4-common.c
> +++ b/arch/arm/mach-omap2/omap4-common.c
> @@ -131,6 +131,12 @@ static int __init omap4_sram_init(void)
>   struct device_node *np;
>   struct gen_pool *sram_pool;
>  
> + /* AM335x is OMAP2+, so no erratum I688 handling needed
> +  * (see CONFIG_OMAP4_ERRATA_I688) needed

This makes no sense.  OMAP4 is OMAP2+ as well, but it needs the erratum.
In fact, all code in mach-omap2 is "OMAP2+".

So, AM335x being "OMAP2+" is no reason at all why I688 should be disabled.

Please fix this comment.  Please also put something in the commit message
which explains fully why you are making this change.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clk: ti: Fix FAPLL udelay in clk_enable with clk_prepare

[Russell King - ARM Linux]

On Tue, Sep 22, 2015 at 02:23:05PM -0700, Tony Lindgren wrote:
> As recently pointed out (again) by Thomas and Russell, we must not
> wait in in clk_enable. The wait for PLL to lock needs to happen
> in clk_prepare instead.
> 
> It seems this is a common copy paste error with the PLL drivers,
> and similar fixes should be applied to other PLL drivers after
> testing.
> 
> Cc: Brian Hutchinson <b.hutch...@gmail.com>
> Cc: Felipe Balbi <ba...@ti.com>
> Cc: Grygorii Strashko <grygorii.stras...@ti.com>
> Cc: Nishanth Menon <n...@ti.com>
> Cc: Russell King - ARM Linux <li...@arm.linux.org.uk>

As this moves things in the right direction (and only based on that):

Acked-by: Russell King <rmk+ker...@arm.linux.org.uk>

> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Sekhar Nori <nsek...@ti.com>
> Signed-off-by: Tony Lindgren <t...@atomide.com>
> ---
>  drivers/clk/ti/fapll.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/ti/fapll.c b/drivers/clk/ti/fapll.c
> index f4b2e98..e1db74a 100644
> --- a/drivers/clk/ti/fapll.c
> +++ b/drivers/clk/ti/fapll.c
> @@ -37,7 +37,7 @@
>  #define FAPLL_PWD_OFFSET 4
>  
>  #define MAX_FAPLL_OUTPUTS7
> -#define FAPLL_MAX_RETRIES1000
> +#define FAPLL_MAX_RETRIES5
>  
>  #define to_fapll(_hw)container_of(_hw, struct fapll_data, hw)
>  #define to_synth(_hw)container_of(_hw, struct fapll_synth, 
> hw)
> @@ -126,7 +126,7 @@ static int ti_fapll_wait_lock(struct fapll_data *fd)
>   if (retries-- <= 0)
>   break;
>  
> - udelay(1);
> + usleep_range(200, 300);
>   }
>  
>   pr_err("%s failed to lock\n", fd->name);
> @@ -134,7 +134,7 @@ static int ti_fapll_wait_lock(struct fapll_data *fd)
>   return -ETIMEDOUT;
>  }
>  
> -static int ti_fapll_enable(struct clk_hw *hw)
> +static int ti_fapll_prepare(struct clk_hw *hw)
>  {
>   struct fapll_data *fd = to_fapll(hw);
>   u32 v = readl_relaxed(fd->base);
> @@ -146,7 +146,7 @@ static int ti_fapll_enable(struct clk_hw *hw)
>   return 0;
>  }
>  
> -static void ti_fapll_disable(struct clk_hw *hw)
> +static void ti_fapll_unprepare(struct clk_hw *hw)
>  {
>   struct fapll_data *fd = to_fapll(hw);
>   u32 v = readl_relaxed(fd->base);
> @@ -155,7 +155,7 @@ static void ti_fapll_disable(struct clk_hw *hw)
>   writel_relaxed(v, fd->base);
>  }
>  
> -static int ti_fapll_is_enabled(struct clk_hw *hw)
> +static int ti_fapll_is_prepared(struct clk_hw *hw)
>  {
>   struct fapll_data *fd = to_fapll(hw);
>   u32 v = readl_relaxed(fd->base);
> @@ -261,7 +261,7 @@ static int ti_fapll_set_rate(struct clk_hw *hw, unsigned 
> long rate,
>   v |= pre_div_p << FAPLL_MAIN_DIV_P_SHIFT;
>   v |= mult_n << FAPLL_MAIN_MULT_N_SHIFT;
>   writel_relaxed(v, fd->base);
> - if (ti_fapll_is_enabled(hw))
> + if (ti_fapll_is_prepared(hw))
>   ti_fapll_wait_lock(fd);
>   ti_fapll_clear_bypass(fd);
>  
> @@ -269,9 +269,9 @@ static int ti_fapll_set_rate(struct clk_hw *hw, unsigned 
> long rate,
>  }
>  
>  static struct clk_ops ti_fapll_ops = {
> - .enable = ti_fapll_enable,
> - .disable = ti_fapll_disable,
> - .is_enabled = ti_fapll_is_enabled,
> + .prepare = ti_fapll_prepare,
> + .unprepare = ti_fapll_unprepare,
> + .is_prepared = ti_fapll_is_prepared,
>   .recalc_rate = ti_fapll_recalc_rate,
>   .get_parent = ti_fapll_get_parent,
>   .round_rate = ti_fapll_round_rate,
> -- 
> 2.1.4
> 

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v4 3/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Russell King - ARM Linux]

On Sat, Sep 19, 2015 at 10:54:51AM -0700, Mark Brown wrote:
> On Fri, Sep 18, 2015 at 02:06:40PM +0300, Jyri Sarha wrote:
> > +#define SPDIF_FORMATS  (SNDRV_PCM_FMTBIT_S16_LE | 
> > SNDRV_PCM_FMTBIT_S16_BE |\
> > +SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
> > +SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
> > +SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
> > +
> > +#define I2S_FORMATS(SNDRV_PCM_FMTBIT_S16_LE | 
> > SNDRV_PCM_FMTBIT_S16_BE |\
> > +SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE |\
> > +SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
> > +SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
> > +SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
> > +SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)
> 
> I'm not sure how abstracted this I2S and S/PDIF DAI business is - it
> doesn't feel like something that's going to be a property of all HDMI
> devices, and the specific sets of formats above cause me to raise a bit
> of an eyebrow.  Should this be an interface where the HDMI chip
> registers multiple interfaces if it has them instead of automatically
> getting this split as is?

The inclusion of the 32-bit formats does raise an eyebrow here too.
Audio transmission across the HDMI link is S/PDIF, supporting up to
24-bit uncompressed audio (aka L-PCM).

The device may accept 32 bit I2S, but it would have to be truncated to
24 bit before transmitting it to the sink.  This should be mentioned
somewhere.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v4 2/8] ALSA: pcm: add IEC958 channel status helper for hw_params

[Russell King - ARM Linux]

On Fri, Sep 18, 2015 at 02:06:39PM +0300, Jyri Sarha wrote:
> Add IEC958 channel status helper that gets the audio properties from
> snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to
> produce the channel status bits already in audio stream configuration
> phase.

What is the reason for doing this early?

ALSA documentation (which may be out of date) says that the hw_params
callback can be called multiple times during stream setup.  Do we want
to be repeatedly programming the HDMI infoframe with different settings,
potentially confusing the attached device, or would it be better to do
it slightly later (in the prepare callback) after the parameters have
been fully negotiated?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mysterious crashes on OMAP5 uevm

[Russell King - ARM Linux]

On Tue, Sep 15, 2015 at 08:31:44PM +0300, Grazvydas Ignotas wrote:
> On Mon, Sep 14, 2015 at 10:35 PM, Dr. H. Nikolaus Schaller
> <h...@goldelico.com> wrote:
> >
> > Am 14.09.2015 um 21:02 schrieb Tony Lindgren <t...@atomide.com>:
> >
> >> * Russell King - ARM Linux <li...@arm.linux.org.uk> [150914 05:16]:
> >>> On Fri, Sep 11, 2015 at 03:03:07PM +0100, Russell King - ARM Linux wrote:
> >>>>
> >>>> Merely changing __LINUX_ARM_ARCH__ >= 7 to >= 6 should fix the problem,
> >>>> and I doubt there's any ARMv6 non-T2 systems out there that would be
> >>>> affected by clearing the IT state bits.
> >>>
> >>> Please test the following patch:
> >>
> >> While we're waiting for Grazvydas to test.. Looks good to me:
> >>
> >> Acked-by: Tony Lindgren <t...@atomide.com>
> >
> > I have tested on:
> > * GTA04 with DM3730 (OMAP3)
> > * Pyra prototype with OMAP5432
> > No X server crashes seen any more.
> >
> > Tested-by: H. Nikolaus Schaller <h...@goldelico.com>
> 
> Tested-by: Grazvydas Ignotas <nota...@gmail.com>
> on OMAP5 uevm running v4.2 built with omap2plus_defconfig.
> On v4.3-rc1 hsmmc controller probe is deferred for whatever reason and
> never reprobes, so my rootfs is never mounted and I could not test,
> but that looks unrelated.

Thanks.

> I guess it's worth marking this one for stable.

Indeed.

Having looked closer at the ARM ARM, these bits on older CPUs are marked
as UNK/SBZP (unknown, should be zero or preserved).  So it's safe to get
rid of that #if entirely.  Removing that #if won't affect the validity
of your testing as you've only tested on ARMv7 platforms with ARMv6
included in the kernel.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mysterious crashes on OMAP5 uevm

[Russell King - ARM Linux]

On Fri, Sep 11, 2015 at 03:03:07PM +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 11, 2015 at 03:27:13PM +0200, Grazvydas Ignotas wrote:
> > On Thu, Sep 10, 2015 at 10:30 AM, Russell King - ARM Linux
> > <li...@arm.linux.org.uk> wrote:
> > > On Thu, Sep 10, 2015 at 08:42:57AM +0200, Dr. H. Nikolaus Schaller wrote:
> > >> ...
> > >>
> > >> Now, disabling CONFIG_ARCH_MULTI_V6 also makes the bug go away and 
> > >> adding the
> > >> >> #if 0 //__LINUX_ARM_ARCH__ >= 7
> > >> makes it re-appear.
> > >>
> > >> A while ago I tried to debug running the x-server under strace and could 
> > >> find that it also has
> > >> something to do with SIGALRM.
> > >>
> > >> And that is very consistent with “enable/disable” by modifying 
> > >> arch/arm/kernel/signal.c
> > >
> > > It would be really nice if someone could diagnose what's going on here.
> > > What exception is causing the X server to be killed (someone said a
> > > segfault)?  What is the register state at the point that happens?  What
> > > does the code look like  Is it happening inside the SIGALRM handler, or
> > > when the SIGALRM handler has returned?
> > >
> > > I'd suggest attaching gdb to the X server, but remember to set gdb to
> > > ignore SIGPIPEs.
> > 
> > It's actually pretty random, see some debug sessions in [1].
> > The first one is the most useful one, but I haven't though of checking
> > what pixman_rasterize_edges() was doing when the signal arrived, and
> > most often the "less useful" segfaults occur. However from the
> > disassembly (see debug1_libpixman.gz) it can be seen that the signal
> > arrived right after IT.
> > 
> > [1] http://notaz.gp2x.de/tmp/thumb_segfault/
> 
> We're not going from ARM -> Thumb or Thumb -> ARM here, but Thumb code
> in libpixman is being interrupted calling a Thumb signal handler.
> 
> Working through the code:
> 
>0x7f717ec8 : ldr r2, [pc, #20]   ; = 0x0004112e
>0x7f717eca <SmartScheduleTimer+2>:   ldr r1, [pc, #24]   ; = 0x0c48
>0x7f717ecc <SmartScheduleTimer+4>:   ldr r3, [pc, #24]   ; = 0x0e6c
>0x7f717ece <SmartScheduleTimer+6>:   add r2, pc
>0x7f717ed0 <SmartScheduleTimer+8>:   ldr r1, [r2, r1]
>0x7f717ed2 <SmartScheduleTimer+10>:  ldr r3, [r2, r3]
> => 0x7f717ed4 <SmartScheduleTimer+12>:  ldr r2, [r1, #0]
> 
> The instruction at 0x7f717ed4 was trying to access 0xd1242963 which
> is in kernel space, and this is the faulting instruction.
> 
> At this point, r2 should contain 0x0004112e plus the PC value.  r2 in
> the register dump was 0x7f717fa0.  Let's calculate the value that PC
> should be here.  0x7f717fa0 - 0x0004112e = 0x7f6d6e72, which is
> clearly wrong.
> 
> So, I don't think the first instruction here was executed by the CPU.
> 
> gdb indicates that the parent context to the signal frame, pc was at
> 0xb6dd87f8, which works out at 0x297f8 into the libpixman-1 library:
> 
>297f0:   449cadd ip, r3
>297f2:   f1bc 0fff   cmp.w   ip, #255; 0xff
>297f6:   bfd4ite le
>297f8:   fa5f fc8c   uxtble.wip, ip
>297fc:   f04f 0cff   movgt.w ip, #255; 0xff
>29800:   f88a c000   strb.w  ip, [sl]
> 
> and as you say, is just after an IT instruction, which would have
> set the IT execution state to appropriately skip either the first or
> the second instruction.
> 
> Unfortunately, the IT instruction's condition is being carried forward
> to the signal handler, causing either the first or second instruction
> there to be skipped.
> 
> Looking back at the history, the original commit introducing the
> clearing of the PSR_IT_MASK bits is just wrong:
> 
> -   if (thumb)
> +   if (thumb) {
> cpsr |= PSR_T_BIT;
> -   else
> +#if __LINUX_ARM_ARCH__ >= 7
> +   /* clear the If-Then Thumb-2 execution state */
> +   cpsr &= ~PSR_IT_MASK;
> +#endif
> +   } else
> cpsr &= ~PSR_T_BIT;
> 
> This shouldn't be a compile-time decision at all, and it certainly should
> not be dependent on __LINUX_ARM_ARCH__, which marks the _lowest_ supported
> architecture.
> 
> However, even the idea that it's ARMv7 or later is wrong.  According to
> the ARM ARM, the IT instruction is present in ARMv6T2 as well, which
> means it's ARMv6 too (whi

Re: mysterious crashes on OMAP5 uevm

[Russell King - ARM Linux]

On Fri, Sep 11, 2015 at 04:12:21PM +, Woodruff, Richard wrote:
> > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> > ow...@vger.kernel.org] On Behalf Of Russell King - ARM Linux
> > Sent: Friday, September 11, 2015 9:03 AM
> > To: Grazvydas Ignotas
> 
> > However, even the idea that it's ARMv7 or later is wrong.  According to
> > the ARM ARM, the IT instruction is present in ARMv6T2 as well, which
> > means it's ARMv6 too (which would have __LINUX_ARM_ARCH__ = 6).
> 
> I recall seeing ARMv6T2 first implemented in the ARM1156 which is a
> v6 CPU with T2 option added.

Exactly, which is why we need to be dealing with the IT bits in signal
handling for >= ARMv6, not >= ARMv7.

> > Looking at the ARM ARM, these bits are "reserved" in previous non-T2
> > architectures, have an undefined value at reset, and are probably zero
> > anyway.
> > 
> > Merely changing __LINUX_ARM_ARCH__ >= 7 to >= 6 should fix the
> > problem,
> > and I doubt there's any ARMv6 non-T2 systems out there that would be
> > affected by clearing the IT state bits.
> 
> Probably you already looked, but cpsr.it usage is not restricted to this
> one spot.

Other places:

arch/arm/mm/extable.c-#ifdef CONFIG_THUMB2_KERNEL
arch/arm/mm/extable.c-  /* Clear the IT state to avoid nasty surprises 
in the fixup */
arch/arm/mm/extable.c:  regs->ARM_cpsr &= ~PSR_IT_MASK;
arch/arm/mm/extable.c-#endif

which is irrelevant here.  This code only deals with kernel mode, and
the only time that this makes sense is when the kernel is built using
Thumb2 instructions.  CONFIG_THUMB2_KERNEL covers the case properly.

arch/arm/probes/kprobes/test-core.c-regs->ARM_lr = val ^ (14 << 8);
arch/arm/probes/kprobes/test-core.c:regs->ARM_cpsr &= ~(APSR_MASK | 
PSR_IT_MASK);
arch/arm/probes/kprobes/test-core.c-regs->ARM_cpsr |= 
test_context_cpsr(scenario);

>From what I can see, this happens unconditionally.

KVM and Xen code... that requires virtualisation support, which is ARMv7.

arch/arm/probes/kprobes/actions-thumb.c... emulating an IT instruction.
arch/arm/probes/decode.h::it_advance... emulating Thumb2.

So really there's no other places that need fixing.

> Looking back at old notes I think both debug and signal handler code
> keyed on bit usage.  I see from LXR kernel KVM code also uses in some
> capacity.

Frankly, Richard, you're getting on my nerves in this thread - you
seem to know all about this problem, yet you never reported the problem
upstream, so people are effectively having to waste time re-doing the
work that you've already done.

Nothing annoys me more than having people say "oh yes, I found that
problem and worked on it" and nothing coming of it (no report, no
patch, no nothing.)

As you have "old notes" you've already investigated this issue, and
presumably you came up with a patch.  Where is it?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mysterious crashes on OMAP5 uevm

[Russell King - ARM Linux]

On Fri, Sep 11, 2015 at 03:27:13PM +0200, Grazvydas Ignotas wrote:
> On Thu, Sep 10, 2015 at 10:30 AM, Russell King - ARM Linux
> <li...@arm.linux.org.uk> wrote:
> > On Thu, Sep 10, 2015 at 08:42:57AM +0200, Dr. H. Nikolaus Schaller wrote:
> >> ...
> >>
> >> Now, disabling CONFIG_ARCH_MULTI_V6 also makes the bug go away and adding 
> >> the
> >> >> #if 0 //__LINUX_ARM_ARCH__ >= 7
> >> makes it re-appear.
> >>
> >> A while ago I tried to debug running the x-server under strace and could 
> >> find that it also has
> >> something to do with SIGALRM.
> >>
> >> And that is very consistent with “enable/disable” by modifying 
> >> arch/arm/kernel/signal.c
> >
> > It would be really nice if someone could diagnose what's going on here.
> > What exception is causing the X server to be killed (someone said a
> > segfault)?  What is the register state at the point that happens?  What
> > does the code look like  Is it happening inside the SIGALRM handler, or
> > when the SIGALRM handler has returned?
> >
> > I'd suggest attaching gdb to the X server, but remember to set gdb to
> > ignore SIGPIPEs.
> 
> It's actually pretty random, see some debug sessions in [1].
> The first one is the most useful one, but I haven't though of checking
> what pixman_rasterize_edges() was doing when the signal arrived, and
> most often the "less useful" segfaults occur. However from the
> disassembly (see debug1_libpixman.gz) it can be seen that the signal
> arrived right after IT.
> 
> [1] http://notaz.gp2x.de/tmp/thumb_segfault/

We're not going from ARM -> Thumb or Thumb -> ARM here, but Thumb code
in libpixman is being interrupted calling a Thumb signal handler.

Working through the code:

   0x7f717ec8 : ldr r2, [pc, #20]   ; = 0x0004112e
   0x7f717eca <SmartScheduleTimer+2>:   ldr r1, [pc, #24]   ; = 0x0c48
   0x7f717ecc <SmartScheduleTimer+4>:   ldr r3, [pc, #24]   ; = 0x0e6c
   0x7f717ece <SmartScheduleTimer+6>:   add r2, pc
   0x7f717ed0 <SmartScheduleTimer+8>:   ldr r1, [r2, r1]
   0x7f717ed2 <SmartScheduleTimer+10>:  ldr r3, [r2, r3]
=> 0x7f717ed4 <SmartScheduleTimer+12>:  ldr r2, [r1, #0]

The instruction at 0x7f717ed4 was trying to access 0xd1242963 which
is in kernel space, and this is the faulting instruction.

At this point, r2 should contain 0x0004112e plus the PC value.  r2 in
the register dump was 0x7f717fa0.  Let's calculate the value that PC
should be here.  0x7f717fa0 - 0x0004112e = 0x7f6d6e72, which is
clearly wrong.

So, I don't think the first instruction here was executed by the CPU.

gdb indicates that the parent context to the signal frame, pc was at
0xb6dd87f8, which works out at 0x297f8 into the libpixman-1 library:

   297f0:   449cadd ip, r3
   297f2:   f1bc 0fff   cmp.w   ip, #255; 0xff
   297f6:   bfd4ite le
   297f8:   fa5f fc8c   uxtble.wip, ip
   297fc:   f04f 0cff   movgt.w ip, #255; 0xff
   29800:   f88a c000   strb.w  ip, [sl]

and as you say, is just after an IT instruction, which would have
set the IT execution state to appropriately skip either the first or
the second instruction.

Unfortunately, the IT instruction's condition is being carried forward
to the signal handler, causing either the first or second instruction
there to be skipped.

Looking back at the history, the original commit introducing the
clearing of the PSR_IT_MASK bits is just wrong:

-   if (thumb)
+   if (thumb) {
cpsr |= PSR_T_BIT;
-   else
+#if __LINUX_ARM_ARCH__ >= 7
+   /* clear the If-Then Thumb-2 execution state */
+   cpsr &= ~PSR_IT_MASK;
+#endif
+   } else
cpsr &= ~PSR_T_BIT;

This shouldn't be a compile-time decision at all, and it certainly should
not be dependent on __LINUX_ARM_ARCH__, which marks the _lowest_ supported
architecture.

However, even the idea that it's ARMv7 or later is wrong.  According to
the ARM ARM, the IT instruction is present in ARMv6T2 as well, which
means it's ARMv6 too (which would have __LINUX_ARM_ARCH__ = 6).

Looking at the ARM ARM, these bits are "reserved" in previous non-T2
architectures, have an undefined value at reset, and are probably zero
anyway.

Merely changing __LINUX_ARM_ARCH__ >= 7 to >= 6 should fix the problem,
and I doubt there's any ARMv6 non-T2 systems out there that would be
affected by clearing the IT state bits.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mysterious crashes on OMAP5 uevm

[Russell King - ARM Linux]

On Thu, Sep 10, 2015 at 08:42:57AM +0200, Dr. H. Nikolaus Schaller wrote:
> 
> Am 08.09.2015 um 23:07 schrieb Tony Lindgren :
> 
> > * Grazvydas Ignotas  [150908 13:44]:
> >> On Tue, Sep 8, 2015 at 4:38 PM, Tony Lindgren  wrote:
> >>> * Grazvydas Ignotas  [150908 05:50]:
>  Hi,
>  
>  this is a longstanding problem I'm seeing since the very beginning,
>  which was around 3.12 or so (when I've first got the hardware) and it
>  seems 4.2 is affected by it still. Basically what happens is Xorg
>  randomly segfaults at some "impossible" location. I don't have the
>  details at the moment (could get them is needed), but from what I
>  examined with gdb some time ago the situation did not make any sense.
>  
>  There are 2 workarounds that I know which make the problem go away
>  (one is enough):
>  - recompile Xorg with -marm (I'm using Debian armhf so it's thumb2 by 
>  default)
>  - disable ARCH_MULTI_V6 in the kernel config
>  
>  Because of the above workarounds I have forgotten about it several
>  times, but it regularly comes back and bites again. It would look like
>  some missing erratum workaround, but I have all of them enabled in the
>  kernel.
>  
>  Does anyone know about this? Perhaps some missing erratum workaround
>  in the bootloader? u-boot isn't too old here (2015.07).
> >>> 
> >>> Seems like some incorrect handling with CONFIG_CPU_V6 compiled in..
> >>> Maybe try to narrow it down by commenting out some CONFIG_CPU_V6 and
> >>> __LINUX_ARM_ARCH__ = 6 ifdefs in the git grep CONFIG_CPU_V6
> >>> places ignoring uncompress and davinci code.
> >> 
> >> ok with that it was quite easy to find. On a kernel with ARCH_MULTI_V6
> >> disabled, it is enough to just do this:
> >> 
> >> --- a/arch/arm/kernel/signal.c
> >> +++ b/arch/arm/kernel/signal.c
> >> @@ -340,13 +340,13 @@ setup_return(struct pt_regs *regs, struct ksignal 
> >> *ksig,
> >>/*
> >> * The LSB of the handler determines if we're going to
> >> * be using THUMB or ARM mode for this signal handler.
> >> */
> >>thumb = handler & 1;
> >> 
> >> -#if __LINUX_ARM_ARCH__ >= 7
> >> +#if 0 //__LINUX_ARM_ARCH__ >= 7
> >>/*
> >> * Clear the If-Then Thumb-2 execution state
> >> * ARM spec requires this to be all 000s in ARM mode
> >> * Snapdragon S4/Krait misbehaves on a Thumb=>ARM
> >> * signal transition without this.
> >> */
> >> 
> >> ... and the problem appears, so I guess this needs some real
> >> multiplatform handling,.
> > 
> > OK nice to hear you found it. Yeah looks like some runtime
> > capability check is needed.
> > 
> >>> Do you have some easy way to reproduce this issue?
> >> 
> >> Just moving a browser window around with mouse usually triggers it
> >> within a minute.
> > 
> > OK good to know.
> 
> It looks as if this is the solution for the same symptom on our OMAP3 board 
> (gta04).
> There, it suffices to draw on the touch screen for ~10 seconds to make the 
> xserver segfault.
> 
> [we are using the binary xserver from debian wheezy
> ii  xserver-xorg-core2:1.12.4-6+deb7u5 
> armhfXorg X server - core server]
> 
> We know about this bug for a while, but so far did think that some touch 
> screen
> event bit has changed and we have to fix our touch screen driver.
> 
> Now, disabling CONFIG_ARCH_MULTI_V6 also makes the bug go away and adding the
> >> #if 0 //__LINUX_ARM_ARCH__ >= 7
> makes it re-appear.
> 
> A while ago I tried to debug running the x-server under strace and could find 
> that it also has
> something to do with SIGALRM.
> 
> And that is very consistent with “enable/disable” by modifying 
> arch/arm/kernel/signal.c

It would be really nice if someone could diagnose what's going on here.
What exception is causing the X server to be killed (someone said a
segfault)?  What is the register state at the point that happens?  What
does the code look like  Is it happening inside the SIGALRM handler, or
when the SIGALRM handler has returned?

I'd suggest attaching gdb to the X server, but remember to set gdb to
ignore SIGPIPEs.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFT PATCH] ARM: OMAP: Change all cpu_is_* occurences to soc_is_*

[Russell King - ARM Linux]

On Tue, Aug 18, 2015 at 03:40:01PM +0530, Keerthy wrote:
 Currently apart from dra7, omap5 and amx3 all the other SoCs
 are identified using cpu_is_* functions which is not right since
 they are all SoCs(System on Chips). Hence changing the SoC
 identificätion code to use soc_is instead of cpu_is and keeping
   ^
typo

 defines for cpu_is where needed. This allows us to replace the
 rest of cpu_is usage along with other fixes as needed.

Good to see this change to a more sensible naming of these, despite
the obvious churn.

Acked-by: Russell King rmk+ker...@arm.linux.org.uk

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v3 3/7] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Russell King - ARM Linux]

On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:
 +static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 + struct snd_pcm_hw_params *params,
 + struct snd_soc_dai *dai)
 +{
 + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
 + struct hdmi_codec_params hp = {
 + .cea = { 0 },

Unnecessary initialisation - because you are initialising this structure,
all unnamed fields will be zeroed.

 + .iec = {
 + .status = {
 + IEC958_AES0_CON_NOT_COPYRIGHT,
 + IEC958_AES1_CON_GENERAL,
 + IEC958_AES2_CON_SOURCE_UNSPEC,
 + IEC958_AES3_CON_CLOCK_VARIABLE,
 + },

...

 + hdmi_audio_infoframe_init(hp.cea);
 + hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_PCM;

Something tells me here that you haven't read the HDMI specification.
HDMI says that the coding type will be zero (refer to stream header).
The same goes for much of the CEA audio infoframe.  Please see the
Audio InfoFrame details in the HDMI specification.

 + hp.cea.channels = params_channels(params);
 +
 + switch (params_width(params)) {
 + case 16:
 + hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_20_16;
 + hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
 + break;
 + case 18:
 + hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_22_18;
 + hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_20;
 + break;
 + case 20:
 + hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_24_20;
 + hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_20;
 + break;
 + case 24:
 + case 32:
 + hp.iec.status[4] |= IEC958_AES4_CON_MAX_WORDLEN_24 |
 + IEC958_AES4_CON_WORDLEN_24_20;

Why not use the generic code to generate the AES channel status bits?
See sound/core/pcm_iec958.c.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v3 6/7] drm/i2c: tda998x: Register ASoC HDMI codec for audio functionality

[Russell King - ARM Linux]

On Fri, Aug 14, 2015 at 12:30:44PM +0300, Jyri Sarha wrote:
 +static int tda998x_write_aif(struct tda998x_priv *priv,
 +  struct hdmi_audio_infoframe *cea)
 +{
 + uint8_t buf[HDMI_INFOFRAME_SIZE(AUDIO)];
 + int len;
 +
 + len = hdmi_audio_infoframe_pack(cea, buf, sizeof(buf));
 + if (len  0) {
 + dev_err(priv-hdmi-dev,
 + Failed to pack audio infoframe: %d\n, len);
 + return len;
 + }
 +
 + /* Write the audio information packet */
 + tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf, len);
 + return 0;
 +}
 +

I have such a function already queued up, but I can't push it out at the
moment because of too many conflicts across all my DRM work.  I'm waiting
for after 4.3-rc1 before publishing anything from or accepting anything
else into DRM branches.

  static void
  tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
  {
 @@ -670,19 +691,24 @@ static void tda998x_audio_mute(struct tda998x_priv 
 *priv, bool on)
   }
  }
  
 -static void
 +static int
  tda998x_configure_audio(struct tda998x_priv *priv,
 - struct drm_display_mode *mode, struct tda998x_encoder_params *p)
 + int mode_clock,
 + int ena_ap,
 + int dai_format,
 + int sample_width,
 + int sample_rate,
 + const u8 *status)

I don't think this is an improvement.

 +static int tda998x_audio_get_eld(struct device *dev, uint8_t *buf, size_t 
 len)
 +{
 + struct tda998x_priv *priv = dev_get_drvdata(dev);
 + struct drm_mode_config *config = priv-encoder-dev-mode_config;
 + struct drm_connector *connector;
 + int ret = -ENODEV;
 +
 + mutex_lock(config.mutex);
 + list_for_each_entry(connector, config-connector_list, head) {
 + if (priv-encoder == connector-encoder) {
 + memcpy(buf, connector-eld, 
 +min(sizeof(connector-eld), len));
 + ret = 0;
 + }
 + }
 + mutex_unlock(config.mutex);

Obviously untested.  Should be config-mutex.

But in any case, when I kill the DRM slave encoder code in here, this
becomes unnecessary.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH-next 1/4] Revert ARM: 7655/1: smp_twd: make twd_local_timer_of_register() no-op for nosmp

[Russell King - ARM Linux]

On Wed, Aug 12, 2015 at 02:56:53PM -0500, Felipe Balbi wrote:
 This reverts commit 904464b91eca8c665acea033489225af02eeb75a.
 
 The problem pointed out by commit 904464b91eca (ARM: 7655/1:
 smp_twd: make twd_local_timer_of_register() no-op for nosmp)
 doesn't exist anymore.
 
 We can safely boot with nosmp and the warning won't show up.
 
 The other side benefit of this patch is that TWD has a chance
 to probe on single-core A9 systems such as AM437x which sport
 TWD.

I don't remember all the details from Feb 2013 on why we made that
change.  If this is proven safe, then I guess it's okay.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()

[Russell King - ARM Linux]

On Tue, Aug 11, 2015 at 03:28:52PM +0530, Vinod Koul wrote:
 On Fri, Aug 07, 2015 at 10:00:18PM +0200, Sebastian Andrzej Siewior wrote:
  In 8250-omap I learned it the hard way that ignoring the return code
  of dmaengine_pause() might be bad because the underlying DMA driver
  might not support the function at all and so not doing what one is
  expecting.
  This patch adds the __must_check annotation as suggested by Russell King.
  
  Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
  ---
   include/linux/dmaengine.h | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
  
  diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
  index 8ad9a4e839f6..4eac4716bded 100644
  --- a/include/linux/dmaengine.h
  +++ b/include/linux/dmaengine.h
  @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct 
  dma_chan *chan)
  return -ENOSYS;
   }
   
  -static inline int dmaengine_pause(struct dma_chan *chan)
  +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
   {
  if (chan-device-device_pause)
  return chan-device-device_pause(chan);
 
 Give that there are bunch of users of this call which may or maynot be using
 this, I think putting this check is too stringent

I think what people need to learn is that an API in the kernel which
returns an int _can_ fail - it returns an int so it _can_ return an
error code.  If it _can_ return an error code, there _will_ be
implementations which _do_.

If you don't check the return code, either your code doesn't care whether
the function was successful or not, or you're playing with fire.  This is
a prime example of playing with fire.

Let's leave the crappy userspace laziness with regard to error checking
to userspace, and keep it out of the kernel.

Yes, the DMA engine capabilities may not be sufficient to describe every
detail of DMA engines, but that's absolutely no reason to skimp on error
checking.  Had there been some kind of error checking at the site, this
problem would have been spotted before the 8250-omap driver was merged.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Tue, Aug 11, 2015 at 03:02:44PM +0300, Peter Ujfalusi wrote:
 On 08/07/2015 11:00 PM, Sebastian Andrzej Siewior wrote:
  +   /*
  +* We do not allow DMA_MEM_TO_DEV transfers to be paused.
  +* From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer:
  +* When a channel is disabled during a transfer, the channel undergoes
  +* an abort, unless it is hardware-source-synchronized ….
  +* A source-synchronised channel is one where the fetching of data is
  +* under control of the device. In other words, a device-to-memory
  +* transfer. So, a destination-synchronised channel (which would be a
  +* memory-to-device transfer) undergoes an abort if the the CCR_ENABLE
  +* bit is cleared.
  +* From 16.1.4.20.4.6.2 Abort: If an abort trigger occurs, the channel
  +* aborts immediately after completion of current read/write
  +* transactions and then the FIFO is cleaned up. The term cleaned up
  +* is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE
  +* are both clear _before_ disabling the channel, otherwise data loss
  +* will occur.
  +* The problem is that if the channel is active, then device activity
  +* can result in DMA activity starting between reading those as both
  +* clear and the write to DMA_CCR to clear the enable bit hitting the
  +* hardware. If the DMA hardware can't drain the data in its FIFO to the
  +* destination, then data loss might occur (say if we write to an UART
  +* and the UART is not accepting any further data).
 
 I don't know if you have checked it, but probably the TX DMA could be also
 used when the PRZEFETCH is disabled for the channel? Just a guess

The docs aren't very clear on that... and iirc Santosh's reply didn't
suggest that the prefetch bit had any influence on this behaviour.  Given
the wording in the documentation which seems to be quite explicit about
the conditions, and it omits talking about the prefetch bit, I can only
assume that the prefetch bit has no influence over this behaviour.

For example, what happens if the DMA to the device has started - the
device has raised its DMA request line.  The DMA controller has then gone
to memory and has fetched some data and incremented the source address.
Meanwhile, we've cleared the ENABLE bit.  What happens then?  Does the
DMA controller drain the read data to the device, or does it clean up
the FIFO by discarding the data?

Given that the conditions under which the FIFO is drained to the
destination are very specific, and which explicitly excludes destination-
synchronised transfers, the only conclusion that's possible without
knowing the implementation intimately is that the FIFO is cleaned up
which suggests that it's discarded rather than drained to the destination.

As this DMA controller is in all of the OMAP devices and similar, I
don't think we can rely on the behaviour of any one implementation
either - we don't know what the differences are between the
implementations in different generations of devices without TI providing
more detailed documentation in this area across their various devices.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Russell King - ARM Linux]

On Mon, Aug 10, 2015 at 09:00:29AM -0400, Peter Hurley wrote:
 Russell seemed to think that the current dma operation was necessary 
 information to
 differentiate types of pause support, but I don't think that's required.
 As Sebastian's omap-dma driver patch shows, partial pause support has more
 to do with how it's being used.

Do you think you can rewrite the first sentence above in gramatically
correct English please, I'm failing to understand what you're saying.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 08:28:57PM -0400, Peter Hurley wrote:
 Even dma_get_slave_caps() returns _true_ for cmd_pause support; ok, that
 interface is pointless.

How about reporting that as a bug then, because if you look back in the
git history, as you are fully capable of, you will find that the slave
capability stuff went in _after_ omap-dma, and *many* DMA engine drivers
have not been updated.  Here, let me do _your_ work for you:

commit cb8cea513c80db1dfe2dce468d2d0772005bb9a1
Author: Maxime Ripard maxime.rip...@free-electrons.com
Date:   Mon Nov 17 14:42:04 2014 +0100

dmaengine: Create a generic dma_slave_caps callback

commit 2dcdf570936168d488acf90be9b04a3d32dafce7
Author: Peter Ujfalusi peter.ujfal...@ti.com
Date:   Fri Sep 14 15:05:45 2012 +0300

dmaengine: omap: Add support for pause/resume in cyclic dma mode

Oh look, omap-dma pre-dates the creation of dma_slave_caps by over two
years!

However, it's *not* as trivial as you think: the dma_slave_caps() call
has no knowledge whether a channel will be used in cyclic mode or not,
or which direction it will be used.  So, it really _can't_ report
whether pause mode is supported or not.  So actually, this is something
that can't be fixed trivially, and was a detail missed when the slave
caps was reviewed (I probably missed the review or missed the point.)

So, how about you stop playing the blame game and trying to shovel your
shit onto DMA engine.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 09:41:19PM -0400, Peter Hurley wrote:
 That's your assertion; I've seen no documentation to back that up
 (other than the de facto commit).

So, you can't be bothered to read the thread where I quoted bits from
the manual, including paraphrasing emails I had discussing this issue
with TI.

What you're effectively saying that I'm a liar.  Fine, there's no point
in engaging in further discussion.  This will be my last reply to you.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Russell King - ARM Linux]

On Sat, Aug 08, 2015 at 11:32:05AM +0200, Sebastian Andrzej Siewior wrote:
 This might not happen at all. At 115200 I *never* encouraged this. Once
 the FIFO is filled with less than RX-trigger size than the UART sends
 the time-out interrupt and the DMA *never* completes.

Careful with statements like that, they have a habbit of backfiring :)

It just needs the right timing - you need a character sequence which
has a pause long enough to trigger the timeout, but short enough to
get characters in between the UART deciding to report a timeout, and
the DMA being terminated.

You'll then be in the situation where the UART is receiving characters
and using the DMA, but you've been interrupted to handle the RX timeout
event.  Whether the UART resets the IIR in that situation to indicate
something other than a RX timeout, I'm not sure - if it doesn't, then
there's a race there (which from your behaviour at faster rates seems
likely.)

The more loaded the system, the longer an IRQ may take to be serviced
(especially if there are interrupt handlers which aren't fast) and the
bigger the window is for that to happen.  I've seen some long service
times with USB with HID in older kernels (milliseconds), but that seems
to have been fixed now - longest IRQ handling I'm now seeing is around
500us.  Given 115200 baud, the character time is 87us, that's probably
more than enough if you get the timing just right.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 03:42:06PM +0200, Sebastian Andrzej Siewior wrote:
 On 08/07/2015 03:22 PM, Russell King - ARM Linux wrote:
  On Fri, Aug 07, 2015 at 12:36:14PM +0200, Sebastian Andrzej Siewior wrote:
  On 08/07/2015 11:44 AM, Peter Ujfalusi wrote:
  with a short testing audio did not broke (the only user of pause/resume)
  Some comments embedded.
 
  Cc: sta...@vger.kernel.org
 
  Why stable? This is not fixing any bugs since the PAUSE was not allowed 
  for
  non cyclic transfers.
 
  Hmmm. The DRA7x was using pause before for UART. I just did not see it
  coming that it was not allowed here. John made a similar change to the
  edma driver and I assumed it went stable but now I see that it was just
  cherry-picked into the ti tree.
  
  This is *NOT* stable material.
  
  Pause of these channels is something that omap-dma has *never* supported.
  Therefore, it is *not* a regression.  What you are doing is *adding* a
  feature to the omap-dma driver.  That is not stable material in any sense.
  Stable is for bug fixes to existing code, not feature enhancements.
 
 I didn't consider this as a feature.

As it is something that the driver has _not_ supported, you are clearly
adding a feature to an existing driver.  It's not a bug fix.

  If something else has been converted to pause channels and that is causing
  a problem, then _that_ conversion is where the bug lies, not the lack of
  support in the omap-dma.
 
 So we had the 8250-DMA doing pause and all its current users implement
 it. We have a DMA driver tree which is not used and it not implementing
 pause (not implementing pause at all). Later we get a combo of 8250-DMA
 + DMA driver that is broken because the lack of pause and this is
 noticed a few kernel releases later.

Right, so the patch which caused the regression is the one which arranged
for the 8250-dma + omap-dma combination to work together, not the missing
pause support in omap-dma.

It doesn't matter that it's several releases old, it's that change caused
the regression you have today.  That change is incorrect today, and it was
just as incorrect at the time that it was merged.

 The only way of fixing the bug is by implementing the pause feature.

That's not the only way of fixing the bug.

As the binding of drivers is controlled by DT, you can disable the binding
of these two drivers there and 8250 will go back to using non-DMA mode -
which is the situation which existed prior to the change which coupled the
two drivers together.  That's an acceptable change for -stable trees,
because it's reverting the change which caused the regression, taking us
back to a situation we _know_ worked previously.

Then, in mainline during the next merge window, we can introduce the pause
feature to omap-dma, and re-enable the 8250 driver to use it.  _Once_ that's
proven stable, we can then take a view whether those changes should _then_
be backported to stable kernels with greater confidence that backporting
the feature addition won't itself cause a new regression.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 02:35:45PM +0200, Sebastian Andrzej Siewior wrote:
 On 08/07/2015 12:55 PM, Russell King - ARM Linux wrote:
  On Fri, Aug 07, 2015 at 10:41:57AM +0200, Sebastian Andrzej Siewior wrote:
  This DMA driver is used by 8250-omap on DRA7-evm. There is one
  requirement that is to pause a transfer. This is currently used on the RX
  side. It is possible that the UART HW aborted the RX (UART's RX-timeout)
  but the DMA controller starts the transfer shortly after.
  Before we can manually purge the FIFO we need to pause the transfer,
  check how many bytes it already received and terminate the transfer
  without it making any progress.
 
  From testing on the TX side it seems that it is possible that we invoke
  pause once the transfer has completed which is indicated by the missing
  CCR_ENABLE bit but before the interrupt has been noticed. In that case the
  interrupt will come even after disabling it.
  
  How do you cope with the OMAP DMA hardware clearing its FIFO when you
  pause it?
 
 I don't

... and so you introduce a silent data loss bug into the driver.  That's
not very clever.

 Right now the 820-omap (8250-dma in general, too but they don't use
 this driver) pause only the RX transfer in an error condition. This
 means it is only device-to-mem transfer. I only mentioned the TX
 transfer here since this was easier to test.

That may be how 8250 works, but 8250 is not everything.  You can't ignore
this problem.  You have to deal with it - either by not allowing a channel
that would loose data to be paused, or by recovering from that condition.
You're not doing either in your patch.

Therefore, I have no other option but to NAK your change.  Sorry.

Please fix this.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 12:36:14PM +0200, Sebastian Andrzej Siewior wrote:
 On 08/07/2015 11:44 AM, Peter Ujfalusi wrote:
  with a short testing audio did not broke (the only user of pause/resume)
  Some comments embedded.
  
  Cc: sta...@vger.kernel.org
  
  Why stable? This is not fixing any bugs since the PAUSE was not allowed for
  non cyclic transfers.
 
 Hmmm. The DRA7x was using pause before for UART. I just did not see it
 coming that it was not allowed here. John made a similar change to the
 edma driver and I assumed it went stable but now I see that it was just
 cherry-picked into the ti tree.

This is *NOT* stable material.

Pause of these channels is something that omap-dma has *never* supported.
Therefore, it is *not* a regression.  What you are doing is *adding* a
feature to the omap-dma driver.  That is not stable material in any sense.
Stable is for bug fixes to existing code, not feature enhancements.

If something else has been converted to pause channels and that is causing
a problem, then _that_ conversion is where the bug lies, not the lack of
support in the omap-dma.

If it's a result of using some new driver with omap-dma, then the problem
is with whatever introduced that new combination - it's not that omap-dma
is buggy.

Don't fix bugs in -stable by adding features.  That's _no_ way to fix bugs.

NAK on this feature patch having any kind of stable tag.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 03:22:56PM +0200, Sebastian Andrzej Siewior wrote:
 On 08/07/2015 03:17 PM, Russell King - ARM Linux wrote:
  On Fri, Aug 07, 2015 at 02:35:45PM +0200, Sebastian Andrzej Siewior wrote:
  On 08/07/2015 12:55 PM, Russell King - ARM Linux wrote:
  On Fri, Aug 07, 2015 at 10:41:57AM +0200, Sebastian Andrzej Siewior wrote:
  This DMA driver is used by 8250-omap on DRA7-evm. There is one
  requirement that is to pause a transfer. This is currently used on the RX
  side. It is possible that the UART HW aborted the RX (UART's RX-timeout)
  but the DMA controller starts the transfer shortly after.
  Before we can manually purge the FIFO we need to pause the transfer,
  check how many bytes it already received and terminate the transfer
  without it making any progress.
 
  From testing on the TX side it seems that it is possible that we invoke
  pause once the transfer has completed which is indicated by the missing
  CCR_ENABLE bit but before the interrupt has been noticed. In that case 
  the
  interrupt will come even after disabling it.
 
  How do you cope with the OMAP DMA hardware clearing its FIFO when you
  pause it?
 
  I don't
  
  ... and so you introduce a silent data loss bug into the driver.  That's
  not very clever.
  
  Right now the 820-omap (8250-dma in general, too but they don't use
  this driver) pause only the RX transfer in an error condition. This
  means it is only device-to-mem transfer. I only mentioned the TX
  transfer here since this was easier to test.
  
  That may be how 8250 works, but 8250 is not everything.  You can't ignore
  this problem.  You have to deal with it - either by not allowing a channel
  that would loose data to be paused, or by recovering from that condition.
  You're not doing either in your patch.
  
  Therefore, I have no other option but to NAK your change.  Sorry.
  
  Please fix this.
 
 Would it be okay if I only allow pause for RX-transfers?

Yes, that satisfies one of my suggestions above.

 For TX-transfers, I would need to update the start-address so the
 transfers begins where it stopped. However based on your concern I
 can't really assume that the position reported by the HW is the correct
 one.

Exactly - I don't believe that existing OMAP DMA hardware can ever support
pausing a mem-to-device transfer without data loss as you have no idea
how many bytes have been prefetched by the DMA, and therefore you have
no idea how many bytes to unwind the hardware position.  It gets worse
than that when you have to cross into the previous descriptor.  It's
really not nice.

So, disallowing pause for mem-to-device is entirely reasonable given the
data loss implications.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 10:41:57AM +0200, Sebastian Andrzej Siewior wrote:
 This DMA driver is used by 8250-omap on DRA7-evm. There is one
 requirement that is to pause a transfer. This is currently used on the RX
 side. It is possible that the UART HW aborted the RX (UART's RX-timeout)
 but the DMA controller starts the transfer shortly after.
 Before we can manually purge the FIFO we need to pause the transfer,
 check how many bytes it already received and terminate the transfer
 without it making any progress.
 
 From testing on the TX side it seems that it is possible that we invoke
 pause once the transfer has completed which is indicated by the missing
 CCR_ENABLE bit but before the interrupt has been noticed. In that case the
 interrupt will come even after disabling it.

How do you cope with the OMAP DMA hardware clearing its FIFO when you
pause it?

The problem is that on mem-to-device transfers, the DMA hardware can
prefetch some data from memory into its FIFO before the device wants
the data.  If you then disable the channel, the hardware clears the
FIFO.

It is unspecified whether the hardware updates the source address in
this case, or by how much.  So it's pretty hard to undo the prefetching
in software.

The net result is: data loss.

This is why I explicitly did not implement pause/resume for non-cyclic
channels.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 05:36:05PM +0200, Sebastian Andrzej Siewior wrote:
 + /*
 +  * We do not allow DMA_MEM_TO_DEV transfers to be paused.
 +  * According to RMK the OMAP hardware might prefetch bytes from
 +  * memory into its FIFO and not send it to the device due to the
 +  * pause. The bytes in the FIFO are cleared on pause. It is
 +  * unspecified by how many bytes the source address is updated
 +  * if at all.

Would you mind rewording the above.

Please take the time to read the manuals for the device you are playing
with.  It's mostly documented in there.  See the OMAP4430 ES2.x TRM,
16.4.18 and 16.4.19.

16.4.19 states that:

 When a source-synchronized channel is disabled during a transfer...
 In all other cases, the channel undergoes an abort.

A source-synchronised channel is one where the fetching of data is under
control of the device.  In other words, a device-to-memory transfer.

So, a destination-synchronised channel (which would be a memory-to-device
transfer) undergoes an abort if you clear the enable bit.

16.4.20.4.6.2 defines what happens when a channel aborts:

 If an abort trigger occurs, the channel aborts immediately after
  completion of current read/write transactions and then FIFO is
  cleaned up.

It doesn't define what cleaned up means, so that's open to some
interpretation.  That's why I contacted TI about this back in 2012:
| What is the behaviour of the OMAP DMA hardware when the DMA4_CCRi[7]
| enable bit is cleared and subsequently set without any additional
| reprogramming?  I'm thinking specifically about memory-to-device DMA
| operations, in particular the UART ports.
| 
| Will this allow a transfer to be temporarily paused, and then
| subsequently resumed with out loss of data in the DMA hardware, as if
| nothing had actually happened?

The answer I received was to check that RD_ACTIVE and WR_ACTIVE are both
clear _before_ disabling the channel, otherwise data loss will occur.

The problem is that if the channel is active, then device activity can
result in DMA activity starting between reading those as both clear and
the write to DMA_CCR to clear the enable bit hitting the hardware.

The explanation went on to say that if the DMA hardware can't drain
the data in its FIFO to the destination, then data loss might occur.

That will occur if we're destination-synchronised (eg) to a UART and
the UART is not accepting any further data.

Due to this, the OMAP DMA engine driver was submitted with this in the
cover note:

For the OMAP DMAengine driver, there's a few short-comings:

1. pause/resume support is not implemented; it's not clear whether the
   OMAP hardware is capable of supporting this sanely.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 06:20:44PM +0200, Sebastian Andrzej Siewior wrote:
 On 08/07/2015 06:07 PM, Peter Hurley wrote:
  If we look at what 8250-dma.c is doing:
 
  if (dma-rx_running) {
  dmaengine_pause(dma-rxchan);
 
  It's 8250-dma.c which is silently _ignoring_ the return code, failing
  to check that the operation it requested worked.  Maybe this should be
  WARN_ON(dmaengine_pause(dma-rxchan)) or at least it should print a
  message?
  
  Thanks for the suggestion; I'll hold on to that and push it after we add
  the 8250 omap dma pause in mainline.
 
 I have a patch ready with WARN_ON_ONCE() for 8250-omap and 8250-dma.
 This warning would trigger on am335x/edma until v4.2-rc1 and omap-dma
 based version is open. I could post it if you want me to.
 Besides that those two, there are four other drivers ignoring the
 return code dmaengine_pause().

It'll probably be a good idea if those are fixed too, and also have a
__must_check annotation on the declaration of dmaengine_pause() to
prevent future mishaps like this.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:
 [ + Greg KH ]
 
 On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:
  As it is something that the driver has _not_ supported, you are clearly
  adding a feature to an existing driver.  It's not a bug fix.
  
  If something else has been converted to pause channels and that is causing
  a problem, then _that_ conversion is where the bug lies, not the lack of
  support in the omap-dma.
 
 FWIW, the actual bug is the api that silently does nothing.

Incorrect.

static int omap_dma_pause(struct dma_chan *chan)
{
struct omap_chan *c = to_omap_dma_chan(chan);

/* Pause/Resume only allowed with cyclic mode */
if (!c-cyclic)
return -EINVAL;

Asking for the channel to be paused will return an error code indicating
that the request failed.  That will be propagated back through to the
return code of dmaengine_pause().

If we look at what 8250-dma.c is doing:

if (dma-rx_running) {
dmaengine_pause(dma-rxchan);

It's 8250-dma.c which is silently _ignoring_ the return code, failing
to check that the operation it requested worked.  Maybe this should be
WARN_ON(dmaengine_pause(dma-rxchan)) or at least it should print a
message?

  Right, so the patch which caused the regression is the one which arranged
  for the 8250-dma + omap-dma combination to work together, not the missing
  pause support in omap-dma.
 
 That would be the original submission patch set for an entire driver,
 the 8250_omap driver.

Well, that's where the bug lies, and I don't agree with your assessment
that it would mean reverting the whole thing.

The binding between the two drivers is controlled via DT.  DT tells it
which DMA controller and which DMA input to use.  So, as DMA is currently
broken on this, the solution is to break that link so that 8250-omap goes
back to using PIO only.

  As the binding of drivers is controlled by DT, you can disable the binding
  of these two drivers
 
 No. 8250 dma is not a stand-alone driver. Even if it were, how would you go
 back and fix DTs in the wild?

Well, we have reached an impass then.

I'm not going to accept a feature addition to a driver as a stable patch
without it being adequately tested over _several_ kernel revisions to
ensure that we don't end up cocking up some driver which uses the DMA
interfaces correctly.  It's too big a risk.

So, I guess that means that older kernels will just have to remain broken -
all because the basic testing of the original code was never undertaken
to ensure that basic stuff like reception of characters worked properly.

Sorry, I have little sympathy here.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 05:44:03PM +0200, Sebastian Andrzej Siewior wrote:
 On 08/07/2015 05:29 PM, Russell King - ARM Linux wrote:
  On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:
  [ + Greg KH ]
 
  On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:
  As it is something that the driver has _not_ supported, you are clearly
  adding a feature to an existing driver.  It's not a bug fix.
 
  If something else has been converted to pause channels and that is 
  causing
  a problem, then _that_ conversion is where the bug lies, not the lack of
  support in the omap-dma.
 
  FWIW, the actual bug is the api that silently does nothing.
  
  Incorrect.
  
  static int omap_dma_pause(struct dma_chan *chan)
  {
  struct omap_chan *c = to_omap_dma_chan(chan);
  
  /* Pause/Resume only allowed with cyclic mode */
  if (!c-cyclic)
  return -EINVAL;
  
  Asking for the channel to be paused will return an error code indicating
  that the request failed.  That will be propagated back through to the
  return code of dmaengine_pause().
  
  If we look at what 8250-dma.c is doing:
  
  if (dma-rx_running) {
  dmaengine_pause(dma-rxchan);
  
  It's 8250-dma.c which is silently _ignoring_ the return code, failing
  to check that the operation it requested worked.  Maybe this should be
  WARN_ON(dmaengine_pause(dma-rxchan)) or at least it should print a
  message?
 
 I think this is what Peter meant by saying silently does nothing.

Maybe Peter should phrase his replies better.  the actual bug is the
api that silently does nothing. to me means he is complaining that
dmaengine_pause() had no effect.  _That_ is what I'm objecting to,
and claiming that Peter's comment is wrong.

He's now blaming me for snide remarks.  I could call his one above an
incorrect snide remark against the quality of DMA engine code.  He's
totally wrong, and been proven wrong by my analysis above, plain and
simple.

He should now accept that he's wrong and move along with sorting out
this mess _without_ requiring optional features to be implemented in
other subsystems to fix bugs in code he's supposed to be maintaining.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 02:21:59PM -0400, Peter Hurley wrote:
 [ + Heikki ]
 
 On 08/07/2015 12:33 PM, Russell King - ARM Linux wrote:
  What you have is a race condition in the code you a responsible for
  maintaining, caused by poorly implemented code.  Fix it, rather than
  whinging about drivers outside of your subsystem having never implemented
  _optional_ things that you choose to merge broken code which relied upon
  it _without_ checking that the operation succeeded.
  
  It is _entirely_ your code which is wrong here.
  
  I will wait for that to be fixed before acking the omap-dma change since
  you obviously need something to test with.
 
 I'm not sure to what you're referring here.
 
 A WARNing fixes nothing.

The warning can wait.

 If you mean some patch, as yet unwritten, that handles the dma cases when
 dmaengine_pause() is unimplemented without data loss, ok, but please confirm
 that's what you mean.

But the regression needs fixing.

 However, at some point one must look at the api and wonder if the separation
 of concern has been drawn in the right place.

It _is_ in the right place.  dmaengine_pause() always has been permitted
to fail.  It's the responsibility of the user of this API to _check_ the
return code to find out whether it had the desired effect.  Not checking
the return code is a bug in the caller's code.

If that wasn't the case, dmaengine_pause() would have a void return type.
It doesn't.  It has an 'int' to allow failure, or to allow non-
implementation for cases where the underlying hardware can't pause the
channel without causing data loss.

What would you think is better: an API which silently loses data, or
one which refuses to stop the transfer and reports an error code back
to the caller.

You seem to be arguing for the former, and as such, there's no way I
can take you seriously.

In any case, Greg has now commented on the patch adding the feature,
basically refusing it for stable tree inclusion.  So the matter is
settled: omap-dma isn't going to get the pause feature added in stable
trees any time soon.  So a different solution now needs to be found,
which is what I've been saying all along...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 01:23:20PM -0400, Peter Hurley wrote:
 The omap-serial driver which doesn't use dma is still the preferred
 stable driver for omap, for the moment.
 
 One of the main features of the 8250_omap integration was the addition
 of dma support. Without it, 8250_omap is ttyO in ttyS clothing.

Correct.  omap-serial used to have broken DMA support, and I had a
series of patches fixing it.  Unfortunately, despite TI asking me to
work on this, TI went ahead and removed the DMA support behind my
back, which totally screwed my patch series after I merged some of
its pre-requists.  It was then decided that DMA support was no longer
an important feature, so I dropped the patch set.

So, I'm well aware of the issues here, and the problems of using the
OMAP sDMA with the UARTs.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 07:55:48PM +0200, Sebastian Andrzej Siewior wrote:
 /* 
  * We do not allow DMA_MEM_TO_DEV transfers to be paused.
  * From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer:
  * When a channel is disabled during a transfer, the channel undergoes
  * an abort, unless it is hardware-source-synchronized ….
  * A source-synchronised channel is one where the fetching of data is
  * under control of the device. In other words, a device-to-memory
  * transfer. So, a destination-synchronised channel (which would be a
  * memory-to-device transfer) undergoes an abort if the the CCR_ENABLE
  * bit is cleared.
  * From 16.1.4.20.4.6.2 Abort: If an abort trigger occurs, the channel
  * aborts immediately after completion of current read/write
  * transactions and then the FIFO is cleaned up. The term cleaned up
  * is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE
  * are both clear _before_ disabling the channel, otherwise data loss
  * will occur.
  * The problem is that if the channel is active, then device activity
  * can result in DMA activity starting between reading those as both
  * clear and the write to DMA_CCR to clear the enable bit hitting the
  * hardware. If the DMA hardware can't drain the data in its FIFO to the
  * destination, then data loss might occur (say if we write to an UART
  * and the UART is not accepting any further data).
  */
 
 would that be okay?

Better, if a tad verbose.  I guess no one will miss that. :)

 If I google for it, I find it. pause/resume support for cyclic was added
 later without a note why it is only supported for cyclic.

Try searching for the [PATCH 11/11] ASoC: omap-pcm: Convert to use
dmaengine thread, which was the initial round of patches converting
omap-pcm to DMA engine, and where there was some discussion of how
to handle it at that time.

The result of that was it was felt that the safest approach was to
limit it to cyclic transfers for ASoC, and to use the method which
had been well proven over previous years/decade on numerous different
OMAP hardware.

It's all entirely sensible given that omap-dma has to cope with many
different hardware revisions with two major hardware versions and
people having limited testing resources for validation.  So you will
understand, given the data loss issues here, why it was decided that
omap-dma will only provide pause/resume support for cases where it
has been proven to work sufficiently well and where data loss is not
that a major issue (if a few samples of audio get lost over a
suspend/resume, it's not going to corrupt your data.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 12:07:11PM -0400, Peter Hurley wrote:
 On 08/07/2015 11:29 AM, Russell King - ARM Linux wrote:
  On Fri, Aug 07, 2015 at 11:08:48AM -0400, Peter Hurley wrote:
  [ + Greg KH ]
 
  On 08/07/2015 09:57 AM, Russell King - ARM Linux wrote:
  As it is something that the driver has _not_ supported, you are clearly
  adding a feature to an existing driver.  It's not a bug fix.
 
  If something else has been converted to pause channels and that is 
  causing
  a problem, then _that_ conversion is where the bug lies, not the lack of
  support in the omap-dma.
 
  FWIW, the actual bug is the api that silently does nothing.
  
  Incorrect.
  
  static int omap_dma_pause(struct dma_chan *chan)
  {
  struct omap_chan *c = to_omap_dma_chan(chan);
  
  /* Pause/Resume only allowed with cyclic mode */
  if (!c-cyclic)
  return -EINVAL;
  
  Asking for the channel to be paused will return an error code indicating
  that the request failed.  That will be propagated back through to the
  return code of dmaengine_pause().
  
  If we look at what 8250-dma.c is doing:
  
  if (dma-rx_running) {
  dmaengine_pause(dma-rxchan);
  
  It's 8250-dma.c which is silently _ignoring_ the return code, failing
  to check that the operation it requested worked.  Maybe this should be
  WARN_ON(dmaengine_pause(dma-rxchan)) or at least it should print a
  message?
 
 Thanks for the suggestion; I'll hold on to that and push it after we add
 the 8250 omap dma pause in mainline.

Why wait?  You're hiding a data loss bug which is clearly the result of
code you allegedly maintain.

 Well, instead of me saying something snide about the lack of upstream serial
 driver unit tests, I'll say I've been working on cleaning up and organizing
 my own tty/serial subsystem and driver units tests which I hope to upstream
 in the fall.
 
 Those include i/o validators that ran this driver for days at time without
 error at max line rate. Unfortunately, that hardware does not exhibit the
 same problem as the DRA7 noted in the changelog.

What you have is a race condition in the code you a responsible for
maintaining, caused by poorly implemented code.  Fix it, rather than
whinging about drivers outside of your subsystem having never implemented
_optional_ things that you choose to merge broken code which relied upon
it _without_ checking that the operation succeeded.

It is _entirely_ your code which is wrong here.

I will wait for that to be fixed before acking the omap-dma change since
you obviously need something to test with.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Russell King - ARM Linux]

On Thu, Aug 06, 2015 at 06:14:00PM +0200, Geert Uytterhoeven wrote:
 On Thu, Aug 6, 2015 at 3:51 PM, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
  On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:
  On the whole following are my requirements:
  1. to be able to communicate with non -flash SPI devices via config port
  ( this functionality is supported by current driver, I dont want to
  break it). Or pump any spi_message on to SPI bus directly.
  2. take advantage of memory mapped port in order to increase read
  throughput( and use dma in future) when the slave is a m25p80 type flash.
  3. handle m25p80 as well as other slave on multiple chipselects.
 
  I just need to know whether the user that requested the transfer is
  m25p80 driver. If yes, ti-qspi driver can take advantage of memory
  mapped interface, else just use config port to access SPI bus directly.
 
  The problem with this approach is that it's an abomination.  It's adding
  a SPI-user specific hack which is detected by a specific driver.  That's
  really not sane - what happens when we have lots of these kinds of I'm
  an X SPI-user with drivers detecting that?  It's not maintainable in the
  long term.
 
  Yes, your requirements _today_ seem simple and easy, but you're only
  thinking about today, not tomorrow when you've moved on and someone else
  has to maintain the mess left behind (or delete it from mainline because
  they're sick of dealing with a hack.)
 
  The spi_message that is received in transfer_one_message() is too
  generic to imply the slave device that is on the other side of the wire.
  IMO, the read command does not imply that the slave is m25p80 flash
  (besides the read opcodes vary across vendors of m25p80 and across modes).
 
  I can see both sides of the argument.
 
  Mark is saying: if the SPI driver detects that the message to be transmitted
  is a read command followed by the appropriate number of dummy bytes, and
  then the data being read _and_ it's using quad-mode access, and the hardware
  generates _exactly_ that in hardware using the memory mapped mode, there is
  no reason _not_ to use the hardware to achieve that SPI transaction.  The
  bus activity will be identical to what happens when the SPI controller is
  used manually to achieve that bus sequence.
 
  You're saying: but the documentation says you can't use it for anything
  except m25p80.  If you look at 24.5.4.1.2, it tells you what the SFI
  generates on the bus, which is:
 
  1. CS active
  2. Read command byte sent
  3. 1-4 address bytes sent
  4. 0-3 dummy bytes sent
  5. data bytes read from bus
  6. CS inactive
 
  So, Mark's point is if we can detect a transaction which fits _that_
  bus activity, there's no reason not to use this acceleration for the
  transaction.
 
  What you're failing to counter with is: we don't have enough information
  in the SPI driver to know how many dummy bytes there are between the
  address bytes and the data read from the bus.
 
 Irrespective of the dummy bytes.
 What if the spi device is not a FLASH ROM, but some other device,
 which receives a data packet that accidentally looks like an m25p80 READ
 command?

Well, for the most part it looks like it should still work, but there
could be a gotcha, but first, let's get rid of a myth there.

The QSPI is _not_ specific to the M25P80.  The manual says nothing
about being specific to that device.  What it says is that it's for
SPI NOR memory.  It will work with bus widths of 1, 2 or 4 data lines,
so it probably works with non-M25P80 SPI NOR devices too - and the fact
that the read and write commands are completely programmable suggests
that using it with SPI NOR devices which do not use the M25P80 read
command value is intended.


The SFI is a state machine based translator which sits behind the SPI
interface (look at the manual).  It sequence sthe SPI bus through a
series of standard SPI states which happen to be the states I detailed
above.

Now, the first byte of the SFI-generated SPI message can be programmed
to any 8 bit value.  So the first byte of the SPI message is totally
under software control.  The next one to four bytes which comprise the
address can be controlled to by deciding where in the memory map to
start reading from.  Hence, the value of those bytes is also totally
under software control.  The number of dummy bytes can be programmed
too.  So far so good.

So, if we know that we have a SPI message which says send 0x01 0x20
0x30, send one dummy byte, read 32 bytes, if we program the SFI to
send a read command as 0x01, program an address length of 2 bytes
with one dummy byte, and then read the next 32 bytes at the appropriate
offset in the memory mapping to cause the next two bytes to be 0x20,
0x30, then what we end up with on the bus is:

send 0x01, 0x20, 0x30
send one dummy byte

That much is good, but now is the problem - how does the SFI know that
we're going to require to read 32 bytes?  I think

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Russell King - ARM Linux]

On Thu, Aug 06, 2015 at 12:01:37PM +0200, Michal Suchanek wrote:
 Disclaimer: I am not familiar with the hardware for which this patch
 adds support.
 
 However, I am familiar m25p80.c and as I understand it the controller
 is basically supposed to implement m25p80.c in hardware when this flag
 is set.

That, to me, sounds like what you have is:

---m25p80 specific interface---SPI bus---m25p80 device

Where the m25p80 specific interface does not expose direct access to the
SPI bus?

If that's the case, then maybe you should consider whether using the SPI
bus infrastructure is really the best way forward.  Would it make more
sense instead to adopt a different software structure, something more
high-level like:

 +---+
 |  m25p80 high-level driver |
 +--++
 |   SPI m25p80 driver  ||
 +--+|
 |  SPI layer   |  Special driver|
 +--+|
 |SPI bus driver||
 +--++
 | SPI hardware |  Special hardware  |
 +--++

Rather than what you seem to be trying to do, which seems to be:

 +--+
 |   SPI m25p80 driver  |
 +--+
 |  SPI layer   |
 +--+
 |  Translation driver  |
 +--+
 |   Special hardware   |
 +--+

where this requires M25P80 specific hacks to be introduced into the SPI
layer so that you can communicate additional information between the SPI
M25P80 driver and the translation driver.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Russell King - ARM Linux]

On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:
 On the whole following are my requirements:
 1. to be able to communicate with non -flash SPI devices via config port
 ( this functionality is supported by current driver, I dont want to
 break it). Or pump any spi_message on to SPI bus directly.
 2. take advantage of memory mapped port in order to increase read
 throughput( and use dma in future) when the slave is a m25p80 type flash.
 3. handle m25p80 as well as other slave on multiple chipselects.
 
 I just need to know whether the user that requested the transfer is
 m25p80 driver. If yes, ti-qspi driver can take advantage of memory
 mapped interface, else just use config port to access SPI bus directly.

The problem with this approach is that it's an abomination.  It's adding
a SPI-user specific hack which is detected by a specific driver.  That's
really not sane - what happens when we have lots of these kinds of I'm
an X SPI-user with drivers detecting that?  It's not maintainable in the
long term.

Yes, your requirements _today_ seem simple and easy, but you're only
thinking about today, not tomorrow when you've moved on and someone else
has to maintain the mess left behind (or delete it from mainline because
they're sick of dealing with a hack.)

 The spi_message that is received in transfer_one_message() is too
 generic to imply the slave device that is on the other side of the wire.
 IMO, the read command does not imply that the slave is m25p80 flash
 (besides the read opcodes vary across vendors of m25p80 and across modes).

I can see both sides of the argument.

Mark is saying: if the SPI driver detects that the message to be transmitted
is a read command followed by the appropriate number of dummy bytes, and
then the data being read _and_ it's using quad-mode access, and the hardware
generates _exactly_ that in hardware using the memory mapped mode, there is
no reason _not_ to use the hardware to achieve that SPI transaction.  The
bus activity will be identical to what happens when the SPI controller is
used manually to achieve that bus sequence.

You're saying: but the documentation says you can't use it for anything
except m25p80.  If you look at 24.5.4.1.2, it tells you what the SFI
generates on the bus, which is:

1. CS active
2. Read command byte sent
3. 1-4 address bytes sent
4. 0-3 dummy bytes sent
5. data bytes read from bus
6. CS inactive

So, Mark's point is if we can detect a transaction which fits _that_
bus activity, there's no reason not to use this acceleration for the
transaction.

What you're failing to counter with is: we don't have enough information
in the SPI driver to know how many dummy bytes there are between the
address bytes and the data read from the bus.

The M25P80 driver just appends additional bytes to the message to
achieve this:

struct m25p *flash = nor-priv;
unsigned int dummy = nor-read_dummy;

/* convert the dummy cycles to the number of bytes */
dummy /= 8;

flash-command[0] = nor-read_opcode;
m25p_addr2cmd(nor, from, flash-command);

t[0].tx_buf = flash-command;
t[0].len = m25p_cmdsz(nor) + dummy;
spi_message_add_tail(t[0], m);

The reason that the number of dummy bytes can't be detected is because
it's all hidden in the first transaction as the total number of bytes to
be transmitted - and the dummy bytes are uninitialised, so you can't
make any assumptions what value they are.  There is no way for the SPI
driver to know whether these dummy bytes are dummy bytes or whether they
have an effect on the targetted device.

I think that comprehensively rules out Mark's idea with the SPI core as
it stands today: there is no way for the SPI driver to reliably detect
a message like a SFI read command by parsing the SPI transmitted message
prior to sending it as we can never be sure whether there are dummy bytes
to be transmitted.

What may make more sense from the SPI point of view is to communicate to
all SPI drivers how many dummy bytes are to be transferred.  I'm not fully
up on SPI, but maybe something like this:

t[0].tx_buf = flash-command;
t[0].len = m25p_cmdsz(nor);
spi_message_add_tail(t[0], m);
t[1].tx_buf = dummy_buffer;
t[1].len = dummy;
t[1].dummy = 1;
spi_message_add_tail(t[1], m);

This way, we're describing the transfer to the SPI core, and explicitly
indicating that there are some dummy bytes.  The SPI driver can then
tell that these are dummy bytes, and if the SPI message consists of:

- transmit 2 to 5 bytes where the first byte is a recognised read command
- transmit 0 to 3 dummy bytes
- read some bytes

then it can make use of the SFI mode to accelerate the operation.

This would not be a hack to the SPI code: we're describing to the SPI
code what we want to achieve in terms of the activity on the bus, and
providing that level of description then allows the SPI driver to make
informed decisions on whether 

Re: [PATCH 4/4] ARM: omap2: restore OMAP4 barrier behaviour

[Russell King - ARM Linux]

On Mon, Jul 27, 2015 at 04:23:45PM -0500, Dan Murphy wrote:
 Russell
 
 On 07/15/2015 12:47 PM, Russell King wrote:
  +#ifdef CONFIG_OMAP_INTERCONNECT_BARRIER
  +
   /* Used to implement memory barrier on DRAM path */
   #define OMAP4_DRAM_BARRIER_VA  0xfe60
   
  -void __iomem *dram_sync, *sram_sync;
  +static void __iomem *dram_sync, *sram_sync;
  +static phys_addr_t dram_sync_paddr;
  +static u32 dram_sync_size;
  +
  +/*
  + * The OMAP4 bus structure contains asynchrnous bridges which can buffer
  + * data writes from the MPU. These asynchronous bridges can be found on
  + * paths between the MPU to EMIF, and the MPU to L3 interconnects.
  + *
  + * We need to be careful about re-ordering which can happen as a result
  + * of different accesses being performed via different paths, and
  + * therefore different asynchronous bridges.
  + */
   
  -static phys_addr_t paddr;
  -static u32 size;
  +/*
  + * OMAP4 interconnect barrier which is called for each mb() and wmb().
  + * This is to ensure that normal paths to DRAM (normal memory, cacheable
  + * accesses) are properly synchronised with writes to DMA coherent memory
  + * (normal memory, uncacheable) and device writes.
  + *
  + * The mb() and wmb() barriers only operate only on the MPU-MA-EMIF
  + * path, as we need to ensure that data is visible to other system
  + * masters prior to writes to those system masters being seen.
  + *
  + * Note: the SRAM path is not synchronised via mb() and wmb().
  + */
  +static void omap4_mb(void)
 
 Sorry for the late response but this throws a warning when CONFIG_ARCH_OMAP4
 is not configured.
 
 arch/arm/mach-omap2/omap4-common.c:85:13: warning: 'omap4_mb' defined but not 
 used [-Wunused-function]

Sorry, I'm going to have to disagree with you and your compiler on this.
This is how the code is structured:

#ifdef CONFIG_OMAP_INTERCONNECT_BARRIER
...
static void omap4_mb(void)
{
if (dram_sync)
writel_relaxed(0, dram_sync);
}
...
void __init omap_barriers_init(void)
{
...
soc_mb = omap4_mb;
}

#endif

So, the definition of omap4_mb(), and the use of the same are both
under the same ifdef.  You can't build omap4_mb() without also building
omap_barriers_init() as well.

So, there's no way that omap4_mb() would be defined but not used.

I think you have some changes in your kernel which, maybe, place an
ifdef around omap_barriers_init() which _would_ cause the warning you're
seeing if they aren't set.

Also, please trim context before the relevant part to which you're
replying. I almost gave up paging down to see whether there was actually
anything worth replying to.  If there's nothing in the first 50 lines of
the message, it's a waste of space.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: errata 430973: move !ARCH_MULTIPLATFORM to Kconfig

[Russell King - ARM Linux]

On Mon, Jul 27, 2015 at 10:59:56AM +0100, Ben Dooks wrote:
 This isn't the only place ARM_ERRATA_430973 is used, and if
 you make it configurable on !ARCH_MULTIPLATFORM then it makes
 it impossible to use a ARCH_MULTIPLATFORM kernel on something
 that is an Cortex-A8.
 
 See arch/arm/mm/proc-v7-2level.S

That has been fixed so that the BTAC/BTB always gets flushed on each
context switch for all Cortex-A8 CPUs, but none of the other v7 CPUs.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: errata 430973: move !ARCH_MULTIPLATFORM to Kconfig

[Russell King - ARM Linux]

On Fri, Jul 24, 2015 at 02:16:06AM +0200, Sebastian Reichel wrote:
 Hi Russel,
 
 On Thu, Jul 23, 2015 at 01:35:53PM +0100, Russell King - ARM Linux wrote:
  On Thu, Jul 23, 2015 at 02:48:03AM +0200, Sebastian Reichel wrote:
   Having the !ARCH_MULTIPLATFORM dependency in the Kconfig file results
   in one option less to think about when configuring the kernel.
  
   -#if defined(CONFIG_ARM_ERRATA_430973)  
   !defined(CONFIG_ARCH_MULTIPLATFORM)
   +#ifdef CONFIG_ARM_ERRATA_430973
 teq r3, #0x0010 @ only present in r1p*
 mrceq   p15, 0, r0, c1, c0, 1   @ read aux control register
 orreq   r0, r0, #(1  6)   @ set IBE to 1
  
  NAK.  Please read the mailing list history, I'm not repeating myself
  again on this.  Thanks.
 
 It's a bit hard to search the mailing list history without a bit
 more information.

You were Cc'd on the previous round of review...

 I guess you prefer to just add the !ARCH_MULTIPLATFORM dependency to
 the Kconfig entry without removing the additional check in the code?

I was referring to the above change.

However, having discussed with Will Deacon and checked the manuals, I
think the change is okay after all: the auxillary control register is
banked on secure parts, and the bit we'll be trying to change will be
read-only in non-secure mode - and importantly won't fault.

So, the change is fine, thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: errata 430973: move !ARCH_MULTIPLATFORM to Kconfig

[Russell King - ARM Linux]

On Thu, Jul 23, 2015 at 02:48:03AM +0200, Sebastian Reichel wrote:
 Having the !ARCH_MULTIPLATFORM dependency in the Kconfig file results
 in one option less to think about when configuring the kernel.

 -#if defined(CONFIG_ARM_ERRATA_430973)  !defined(CONFIG_ARCH_MULTIPLATFORM)
 +#ifdef CONFIG_ARM_ERRATA_430973
   teq r3, #0x0010 @ only present in r1p*
   mrceq   p15, 0, r0, c1, c0, 1   @ read aux control register
   orreq   r0, r0, #(1  6)   @ set IBE to 1

NAK.  Please read the mailing list history, I'm not repeating myself
again on this.  Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] ARM: omap2: restore OMAP4 barrier behaviour

[Russell King - ARM Linux]

On Wed, Jul 15, 2015 at 11:48:54PM -0700, Tony Lindgren wrote:
 Hi,
 
 * Russell King rmk+ker...@arm.linux.org.uk [150715 10:50]:
  Restore the OMAP4 barrier behaviour using the new implementation which
  allows multiplatform systems to hook into the mb() and wmb() ARM
  implementations to perform any necessary additional barrier maintanence.
 
 I'm getthing this with omap2plus_defconfig with the last patch
 applied:
 
 arch/arm/mach-omap2/omap4-common.c: In function ‘omap4_sram_init’:
 arch/arm/mach-omap2/omap4-common.c:138:14: error: implicit declaration of 
 function ‘of_get_named_gen_pool’ [-Werror=implicit-function-declaration]
   sram_pool = of_get_named_gen_pool(np, sram, 0);
   ^
 arch/arm/mach-omap2/omap4-common.c:138:12: warning: assignment makes pointer 
 from integer without a cast [-Wint-conversion]
   sram_pool = of_get_named_gen_pool(np, sram, 0);
 ^

I'd forgotten to merge in the merge window fixes for this... which have now
been lost.  This needs to become of_gen_pool_get() in 4.2-rc1 and later.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] ARM: move heavy barrier support out of line

[Russell King]

The existing memory barrier macro causes a significant amount of code
to be inserted inline at every call site.  For example, in
gpio_set_irq_type(), we have this for mb():

c0344c08:   f57ff04edsb st
c0344c0c:   e59f8190ldr r8, [pc, #400]  ; c0344da4 
gpio_set_irq_type+0x230
c0344c10:   e3590004cmp r9, #4
c0344c14:   e5983014ldr r3, [r8, #20]
c0344c18:   0a54beq c0344d70 gpio_set_irq_type+0x1fc
c0344c1c:   e353cmp r3, #0
c0344c20:   0a04beq c0344c38 gpio_set_irq_type+0xc4
c0344c24:   e50b2030str r2, [fp, #-48]  ; 0xffd0
c0344c28:   e50bc034str ip, [fp, #-52]  ; 0xffcc
c0344c2c:   e12fff33blx r3
c0344c30:   e51bc034ldr ip, [fp, #-52]  ; 0xffcc
c0344c34:   e51b2030ldr r2, [fp, #-48]  ; 0xffd0
c0344c38:   e5963004ldr r3, [r6, #4]

Moving the outer_cache_sync() call out of line reduces the impact of
the barrier:

c0344968:   f57ff04edsb st
c034496c:   e35a0004cmp sl, #4
c0344970:   e50b2030str r2, [fp, #-48]  ; 0xffd0
c0344974:   0a44beq c0344a8c gpio_set_irq_type+0x1b8
c0344978:   ebf363ddbl  c001d8f4 arm_heavy_mb
c034497c:   e5953004ldr r3, [r5, #4]

This should reduce the cache footprint of this code.  Overall, this
results in a reduction of around 20K in the kernel size:

textdata  bss  dec hex filename
10773970  667392 10369656 21811018 14ccf4a ../build/imx6/vmlinux-old
10754219  667392 10369656 21791267 14c8223 ../build/imx6/vmlinux-new

Another advantage to this approach is that we can finally resolve the
issue of SoCs which have their own memory barrier requirements within
multiplatform kernels (such as OMAP.)  Here, the bus interconnects
need additional handling to ensure that writes become visible in the
correct order (eg, between dma_map() operations, writes to DMA
coherent memory, and MMIO accesses.)

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/include/asm/barrier.h| 12 +---
 arch/arm/include/asm/outercache.h | 17 -
 arch/arm/kernel/irq.c |  1 +
 arch/arm/mach-prima2/pm.c |  1 +
 arch/arm/mach-ux500/cache-l2x0.c  |  1 +
 arch/arm/mm/Kconfig   |  4 
 arch/arm/mm/flush.c   | 11 +++
 7 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/barrier.h b/arch/arm/include/asm/barrier.h
index 6c2327e1c732..fea99b0e2087 100644
--- a/arch/arm/include/asm/barrier.h
+++ b/arch/arm/include/asm/barrier.h
@@ -2,7 +2,6 @@
 #define __ASM_BARRIER_H
 
 #ifndef __ASSEMBLY__
-#include asm/outercache.h
 
 #define nop() __asm__ __volatile__(mov\tr0,r0\t@ nop\n\t);
 
@@ -37,12 +36,19 @@
 #define dmb(x) __asm__ __volatile__ ( : : : memory)
 #endif
 
+#ifdef CONFIG_ARM_HEAVY_MB
+extern void arm_heavy_mb(void);
+#define __arm_heavy_mb(x...) do { dsb(x); arm_heavy_mb(); } while (0)
+#else
+#define __arm_heavy_mb(x...) dsb(x)
+#endif
+
 #ifdef CONFIG_ARCH_HAS_BARRIERS
 #include mach/barriers.h
 #elif defined(CONFIG_ARM_DMA_MEM_BUFFERABLE) || defined(CONFIG_SMP)
-#define mb()   do { dsb(); outer_sync(); } while (0)
+#define mb()   __arm_heavy_mb()
 #define rmb()  dsb()
-#define wmb()  do { dsb(st); outer_sync(); } while (0)
+#define wmb()  __arm_heavy_mb(st)
 #define dma_rmb()  dmb(osh)
 #define dma_wmb()  dmb(oshst)
 #else
diff --git a/arch/arm/include/asm/outercache.h 
b/arch/arm/include/asm/outercache.h
index 563b92fc2f41..c2bf24f40177 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -129,21 +129,4 @@ static inline void outer_resume(void) { }
 
 #endif
 
-#ifdef CONFIG_OUTER_CACHE_SYNC
-/**
- * outer_sync - perform a sync point for outer cache
- *
- * Ensure that all outer cache operations are complete and any store
- * buffers are drained.
- */
-static inline void outer_sync(void)
-{
-   if (outer_cache.sync)
-   outer_cache.sync();
-}
-#else
-static inline void outer_sync(void)
-{ }
-#endif
-
 #endif /* __ASM_OUTERCACHE_H */
diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 350f188c92d2..b96c8ed1723a 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -39,6 +39,7 @@
 #include linux/export.h
 
 #include asm/hardware/cache-l2x0.h
+#include asm/outercache.h
 #include asm/exception.h
 #include asm/mach/arch.h
 #include asm/mach/irq.h
diff --git a/arch/arm/mach-prima2/pm.c b/arch/arm/mach-prima2/pm.c
index d99d08eeb966..83e94c95e314 100644
--- a/arch/arm/mach-prima2/pm.c
+++ b/arch/arm/mach-prima2/pm.c
@@ -16,6 +16,7 @@
 #include linux/of_platform.h
 #include linux/io.h
 #include linux/rtc/sirfsoc_rtciobrg.h
+#include asm/outercache.h
 #include asm/suspend.h
 #include asm/hardware/cache-l2x0.h
 
diff

[PATCH 3/4] Revert ARM: OMAP4: remove dead kconfig option OMAP4_ERRATA_I688

[Russell King]

This reverts commit 606da4826b89b044b51e3a84958b802204cfe4c7.

We actually need this code for proper behaviour of OMAP4, and it needs
fixing a different way other than just removing the code.  Disabling
code which is necessary in the hopes of persuing multiplatform kernels
is a stupid approach.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mach-omap2/Kconfig| 21 
 arch/arm/mach-omap2/common.c   |  1 +
 arch/arm/mach-omap2/common.h   |  3 ++
 arch/arm/mach-omap2/io.c   |  2 ++
 arch/arm/mach-omap2/omap-secure.h  |  7 
 arch/arm/mach-omap2/omap4-common.c | 69 ++
 arch/arm/mach-omap2/sleep44xx.S|  2 ++
 7 files changed, 105 insertions(+)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index ecc04ff13e95..2128441430ad 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -240,6 +240,27 @@ config OMAP3_SDRC_AC_TIMING
  wish to say no.  Selecting yes without understanding what is
  going on could result in system crashes;
 
+config OMAP4_ERRATA_I688
+   bool OMAP4 errata: Async Bridge Corruption
+   depends on (ARCH_OMAP4 || SOC_OMAP5)  !ARCH_MULTIPLATFORM
+   select ARCH_HAS_BARRIERS
+   help
+ If a data is stalled inside asynchronous bridge because of back
+ pressure, it may be accepted multiple times, creating pointer
+ misalignment that will corrupt next transfers on that data path
+ until next reset of the system (No recovery procedure once the
+ issue is hit, the path remains consistently broken). Async bridge
+ can be found on path between MPU to EMIF and MPU to L3 interconnect.
+ This situation can happen only when the idle is initiated by a
+ Master Request Disconnection (which is trigged by software when
+ executing WFI on CPU).
+ The work-around for this errata needs all the initiators connected
+ through async bridge must ensure that data path is properly drained
+ before issuing WFI. This condition will be met if one Strongly ordered
+ access is performed to the target right before executing the WFI.
+ In MPU case, L3 T2ASYNC FIFO and DDR T2ASYNC FIFO needs to be drained.
+ IO barrier ensure that there is no synchronisation loss on initiators
+ operating on both interconnect port simultaneously.
 endmenu
 
 endif
diff --git a/arch/arm/mach-omap2/common.c b/arch/arm/mach-omap2/common.c
index eae6a0e87c90..484cdadfb187 100644
--- a/arch/arm/mach-omap2/common.c
+++ b/arch/arm/mach-omap2/common.c
@@ -30,4 +30,5 @@ int __weak omap_secure_ram_reserve_memblock(void)
 void __init omap_reserve(void)
 {
omap_secure_ram_reserve_memblock();
+   omap_barrier_reserve_memblock();
 }
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index cf3cf22ecd42..46e24581d624 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -200,6 +200,9 @@ void __init omap4_map_io(void);
 void __init omap5_map_io(void);
 void __init ti81xx_map_io(void);
 
+/* omap_barriers_init() is OMAP4 only */
+void omap_barriers_init(void);
+
 /**
  * omap_test_timeout - busy-loop, testing a condition
  * @cond: condition to test until it evaluates to true
diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 820dde8b5b04..7743e3672f98 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -306,6 +306,7 @@ void __init am33xx_map_io(void)
 void __init omap4_map_io(void)
 {
iotable_init(omap44xx_io_desc, ARRAY_SIZE(omap44xx_io_desc));
+   omap_barriers_init();
 }
 #endif
 
@@ -313,6 +314,7 @@ void __init omap4_map_io(void)
 void __init omap5_map_io(void)
 {
iotable_init(omap54xx_io_desc, ARRAY_SIZE(omap54xx_io_desc));
+   omap_barriers_init();
 }
 #endif
 /*
diff --git a/arch/arm/mach-omap2/omap-secure.h 
b/arch/arm/mach-omap2/omap-secure.h
index af2851fbcdf0..dec2b05d184b 100644
--- a/arch/arm/mach-omap2/omap-secure.h
+++ b/arch/arm/mach-omap2/omap-secure.h
@@ -70,6 +70,13 @@ extern u32 rx51_secure_dispatcher(u32 idx, u32 process, u32 
flag, u32 nargs,
 extern u32 rx51_secure_update_aux_cr(u32 set_bits, u32 clear_bits);
 extern u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag);
 
+#ifdef CONFIG_OMAP4_ERRATA_I688
+extern int omap_barrier_reserve_memblock(void);
+#else
+static inline void omap_barrier_reserve_memblock(void)
+{ }
+#endif
+
 #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
 void set_cntfreq(void);
 #else
diff --git a/arch/arm/mach-omap2/omap4-common.c 
b/arch/arm/mach-omap2/omap4-common.c
index 16350eefa66c..7bb116a6f86f 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -51,6 +51,75 @@ static void __iomem *twd_base;
 
 #define IRQ_LOCALTIMER 29
 
+#ifdef CONFIG_OMAP4_ERRATA_I688
+/* Used to implement memory barrier on DRAM path */
+#define OMAP4_DRAM_BARRIER_VA

[PATCH 0/4] Fix OMAP4 barrier support

[Russell King - ARM Linux]

OMAP4 (and other TI CPUs) allow for weak ordering of writes through
different buses in their interconnects.  In order to ensure that
accesses are performed in the correct order, we need to extend some
of the kernel barriers to ensure that the bus interconnects are
correctly ordered.

As an example, it is possible for the following code sequence:

cpu = dma_alloc_coherent(dev, ..., dma);

*cpu = 1;
writel(dma, dev_addr);

Normal kernel semantics ensures that the write to *cpu will become
visible at the ARM domain boundary before the write to dev_addr.

However, because of the TI interconnects, it is possible for the
write to dev_addr to hit the peripheral before the write to *cpu
hits memory.  Thus, if the write to the device causes the device to
attempt to read from *cpu (which could be via a different path)
the device won't see that write.

Hence, we need to extend the barriers to ensure proper ordering
further down the bus chain than we do already to the L2 cache.

 arch/arm/include/asm/barrier.h  |  13 ++-
 arch/arm/include/asm/outercache.h   |  17 
 arch/arm/kernel/irq.c   |   1 +
 arch/arm/mach-omap2/Kconfig |   7 ++
 arch/arm/mach-omap2/common.c|   1 +
 arch/arm/mach-omap2/common.h|   9 +++
 arch/arm/mach-omap2/include/mach/barriers.h |  33 
 arch/arm/mach-omap2/io.c|   2 +
 arch/arm/mach-omap2/omap4-common.c  | 121 
 arch/arm/mach-omap2/sleep44xx.S |   8 +-
 arch/arm/mach-prima2/pm.c   |   1 +
 arch/arm/mach-ux500/cache-l2x0.c|   1 +
 arch/arm/mm/Kconfig |   4 +
 arch/arm/mm/flush.c |  15 
 14 files changed, 175 insertions(+), 58 deletions(-)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] ARM: omap2: restore OMAP4 barrier behaviour

[Russell King]

Restore the OMAP4 barrier behaviour using the new implementation which
allows multiplatform systems to hook into the mb() and wmb() ARM
implementations to perform any necessary additional barrier maintanence.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mach-omap2/Kconfig | 28 +++--
 arch/arm/mach-omap2/common.h| 12 +++-
 arch/arm/mach-omap2/include/mach/barriers.h | 33 ---
 arch/arm/mach-omap2/omap-secure.h   |  7 ---
 arch/arm/mach-omap2/omap4-common.c  | 90 +++--
 arch/arm/mach-omap2/sleep44xx.S | 10 +---
 6 files changed, 90 insertions(+), 90 deletions(-)
 delete mode 100644 arch/arm/mach-omap2/include/mach/barriers.h

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 2128441430ad..8427997e09c4 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -29,6 +29,7 @@ config ARCH_OMAP4
select HAVE_ARM_SCU if SMP
select HAVE_ARM_TWD if SMP
select OMAP_INTERCONNECT
+   select OMAP_INTERCONNECT_BARRIER
select PL310_ERRATA_588369 if CACHE_L2X0
select PL310_ERRATA_727915 if CACHE_L2X0
select PM_OPP if PM
@@ -46,6 +47,7 @@ config SOC_OMAP5
select HAVE_ARM_TWD if SMP
select HAVE_ARM_ARCH_TIMER
select ARM_ERRATA_798181 if SMP
+   select OMAP_INTERCONNECT_BARRIER
 
 config SOC_AM33XX
bool TI AM33XX
@@ -70,6 +72,7 @@ config SOC_DRA7XX
select HAVE_ARM_ARCH_TIMER
select IRQ_CROSSBAR
select ARM_ERRATA_798181 if SMP
+   select OMAP_INTERCONNECT_BARRIER
 
 config ARCH_OMAP2PLUS
bool
@@ -91,6 +94,10 @@ config ARCH_OMAP2PLUS
help
  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
 
+config OMAP_INTERCONNECT_BARRIER
+   bool
+   select ARM_HEAVY_MB
+   
 
 if ARCH_OMAP2PLUS
 
@@ -240,27 +247,6 @@ config OMAP3_SDRC_AC_TIMING
  wish to say no.  Selecting yes without understanding what is
  going on could result in system crashes;
 
-config OMAP4_ERRATA_I688
-   bool OMAP4 errata: Async Bridge Corruption
-   depends on (ARCH_OMAP4 || SOC_OMAP5)  !ARCH_MULTIPLATFORM
-   select ARCH_HAS_BARRIERS
-   help
- If a data is stalled inside asynchronous bridge because of back
- pressure, it may be accepted multiple times, creating pointer
- misalignment that will corrupt next transfers on that data path
- until next reset of the system (No recovery procedure once the
- issue is hit, the path remains consistently broken). Async bridge
- can be found on path between MPU to EMIF and MPU to L3 interconnect.
- This situation can happen only when the idle is initiated by a
- Master Request Disconnection (which is trigged by software when
- executing WFI on CPU).
- The work-around for this errata needs all the initiators connected
- through async bridge must ensure that data path is properly drained
- before issuing WFI. This condition will be met if one Strongly ordered
- access is performed to the target right before executing the WFI.
- In MPU case, L3 T2ASYNC FIFO and DDR T2ASYNC FIFO needs to be drained.
- IO barrier ensure that there is no synchronisation loss on initiators
- operating on both interconnect port simultaneously.
 endmenu
 
 endif
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index 46e24581d624..82f88b4ec15f 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -189,6 +189,15 @@ static inline void omap44xx_restart(enum reboot_mode mode, 
const char *cmd)
 }
 #endif
 
+#ifdef CONFIG_OMAP_INTERCONNECT_BARRIER
+void omap_barrier_reserve_memblock(void);
+void omap_barriers_init(void);
+#else
+static inline void omap_barrier_reserve_memblock(void)
+{
+}
+#endif
+
 /* This gets called from mach-omap2/io.c, do not call this */
 void __init omap2_set_globals_tap(u32 class, void __iomem *tap);
 
@@ -200,9 +209,6 @@ void __init omap4_map_io(void);
 void __init omap5_map_io(void);
 void __init ti81xx_map_io(void);
 
-/* omap_barriers_init() is OMAP4 only */
-void omap_barriers_init(void);
-
 /**
  * omap_test_timeout - busy-loop, testing a condition
  * @cond: condition to test until it evaluates to true
diff --git a/arch/arm/mach-omap2/include/mach/barriers.h 
b/arch/arm/mach-omap2/include/mach/barriers.h
deleted file mode 100644
index 1c582a8592b9..
--- a/arch/arm/mach-omap2/include/mach/barriers.h
+++ /dev/null
@@ -1,33 +0,0 @@
-/*
- * OMAP memory barrier header.
- *
- * Copyright (C) 2011 Texas Instruments, Inc.
- *  Santosh Shilimkar santosh.shilim...@ti.com
- *  Richard Woodruff r-woodru...@ti.com
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation

[PATCH 2/4] ARM: add soc memory barrier extension

[Russell King]

Add an extension to the heavy barrier code to allow a SoC specific
memory barrier function to be provided.  This is needed for platforms
where the interconnect has weak ordering, and thus needs assistance
to ensure that memory writes are properly visible in the correct order
to other parts of the system.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/include/asm/barrier.h | 1 +
 arch/arm/mm/flush.c| 4 
 2 files changed, 5 insertions(+)

diff --git a/arch/arm/include/asm/barrier.h b/arch/arm/include/asm/barrier.h
index fea99b0e2087..3d8f1d3ad9a7 100644
--- a/arch/arm/include/asm/barrier.h
+++ b/arch/arm/include/asm/barrier.h
@@ -37,6 +37,7 @@
 #endif
 
 #ifdef CONFIG_ARM_HEAVY_MB
+extern void (*soc_mb)(void);
 extern void arm_heavy_mb(void);
 #define __arm_heavy_mb(x...) do { dsb(x); arm_heavy_mb(); } while (0)
 #else
diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index ce6c2960d5ac..1ec8e7590fc6 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -22,12 +22,16 @@
 #include mm.h
 
 #ifdef CONFIG_ARM_HEAVY_MB
+void (*soc_mb)(void);
+
 void arm_heavy_mb(void)
 {
 #ifdef CONFIG_OUTER_CACHE_SYNC
if (outer_cache.sync)
outer_cache.sync();
 #endif
+   if (soc_mb)
+   soc_mb();
 }
 EXPORT_SYMBOL(arm_heavy_mb);
 #endif
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] arm: boot: store ATAGs structure into DT /chosen/linux,atags entry

[Russell King - ARM Linux]

On Mon, Jul 06, 2015 at 10:26:13PM +0200, Pali Rohár wrote:
 Legacy bootloaders can pass additional information for kernel or legacy
 userspace applications. When booting DT kernel then ATAGs structure is not
 more visible after running kernel uncompress code. This patch stores full
 ATAGs structure into DT /chosen/linux,atags entry, so kernel can later
 reuse it and export via /proc/atags to userspace.

I think you need to go through your commit messages and improve them,
especially the ones with TODO in them.  As long as there's still things
to be done, they're obviously not ready for merging.

Moreover, exporting the ATAGS is questionable, even _if_ there are non-
kexec programs making use of this.  The ATAGs have _never_ been exported
to userspace when kexec disabled is the kernel - it was introduced for
kexec, and has always had this:

config ATAGS_PROC
bool Export atags in procfs
depends on ATAGS  KEXEC
default y

Now, the fact that someone decided to start using it is pretty sad,
because it means that if you disable KEXEC, userspace breaks.  That's
not a kernel regression in any shape or form, because /proc/atags has
never been there without KEXEC enabled.  That's a userspace bug, plain
and simple.

Given that, I'm in two minds about whether to accept the last two
patches which make this more than just for KEXEC use to enable a KEXEC
kernel to be booted.

Had it been provided without the KEXEC conditional, then I don't have
a problem with these two patches.

It also sets a precedent: by adding this into DT, it is creating a new
DT ABI as well, and we'll end up seeing dts files with an ATAG block
patched into them.

Are the ATAGs at a fixed address on the N900?  Can that be handled in
some kind of legacy file for the N900 which calls save_atags() on it, so
we don't end up introducing yet more stuff that we have to maintain into
the distant future?  If not, what about copying a known working atag
structure into a legacy file for the N900?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ARM: /proc/atags: Export also for DT

[Russell King - ARM Linux]

On Fri, May 15, 2015 at 09:50:05PM +0200, Pali Rohár wrote:
 This patch adds support for DT /atags and stores ATAG structure to DT.
 
 It depends on ARM: /proc/cpuinfo: DT: Add support for Revision patches.
 
 Pali Rohár (2):
   arm: devtree: Save atags if are in DT atags field
   arm: boot: store ATAG structure into DT atag field
 
  arch/arm/boot/compressed/atags_to_fdt.c |6 +-
  arch/arm/kernel/devtree.c   |6 ++
  2 files changed, 11 insertions(+), 1 deletion(-)

What these patches are missing is a description of _why_ this is required
in any of the commit messages.  The commit messages seem to be describing
_what_ the change is doing, which is something that can be seen by reading
the patches, but it leaves the question of why this change is necessary
entirely open.

Please update the commit messages on the next patch revision.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND] [PATCH v2 1/2] arm: devtree: Set system_rev from DT revision

[Russell King - ARM Linux]

On Wed, May 06, 2015 at 01:44:17PM +0200, Pali Rohár wrote:
 On Wednesday 06 May 2015 13:04:01 Arnd Bergmann wrote:
  What I mean is which code accesses this variable that early?
  
 
 ATAG code is doing it at same early stage, so I added it to same early
 stage...

ATAG code does it early because ATAGs are only available early on, and
it's simpler to parse them all in one go, rather than having to do
multiple passes over the structure - especially when most instances are
just storing an integer to some BSS variable.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC 1/2] ARM: move heavy barrier support out of line

[Russell King]

The existing memory barrier macro causes a significant amount of code
to be inserted inline at every call site.  For example, in
gpio_set_irq_type(), we have this for mb():

c0344c08:   f57ff04edsb st
c0344c0c:   e59f8190ldr r8, [pc, #400]  ; c0344da4 
gpio_set_irq_type+0x230
c0344c10:   e3590004cmp r9, #4
c0344c14:   e5983014ldr r3, [r8, #20]
c0344c18:   0a54beq c0344d70 gpio_set_irq_type+0x1fc
c0344c1c:   e353cmp r3, #0
c0344c20:   0a04beq c0344c38 gpio_set_irq_type+0xc4
c0344c24:   e50b2030str r2, [fp, #-48]  ; 0xffd0
c0344c28:   e50bc034str ip, [fp, #-52]  ; 0xffcc
c0344c2c:   e12fff33blx r3
c0344c30:   e51bc034ldr ip, [fp, #-52]  ; 0xffcc
c0344c34:   e51b2030ldr r2, [fp, #-48]  ; 0xffd0
c0344c38:   e5963004ldr r3, [r6, #4]

Moving the outer_cache_sync() call out of line reduces the impact of
the barrier:

c0344968:   f57ff04edsb st
c034496c:   e35a0004cmp sl, #4
c0344970:   e50b2030str r2, [fp, #-48]  ; 0xffd0
c0344974:   0a44beq c0344a8c gpio_set_irq_type+0x1b8
c0344978:   ebf363ddbl  c001d8f4 arm_heavy_mb
c034497c:   e5953004ldr r3, [r5, #4]

This should reduce the cache footprint of this code.  Overall, this
results in a reduction of around 20K in the kernel size:

textdata  bss  dec hex filename
10773970  667392 10369656 21811018 14ccf4a ../build/imx6/vmlinux-old
10754219  667392 10369656 21791267 14c8223 ../build/imx6/vmlinux-new

Another advantage to this approach is that we can finally resolve the
issue of SoCs which have their own memory barrier requirements within
multiplatform kernels (such as OMAP.)  Here, the bus interconnects
need additional handling to ensure that writes become visible in the
correct order (eg, between dma_map() operations, writes to DMA
coherent memory, and MMIO accesses.)

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/include/asm/barrier.h  | 12 ---
 arch/arm/include/asm/outercache.h   | 17 ---
 arch/arm/kernel/irq.c   |  1 +
 arch/arm/mach-omap2/include/mach/barriers.h | 33 -
 arch/arm/mm/Kconfig |  4 
 arch/arm/mm/flush.c | 11 ++
 6 files changed, 25 insertions(+), 53 deletions(-)
 delete mode 100644 arch/arm/mach-omap2/include/mach/barriers.h

diff --git a/arch/arm/include/asm/barrier.h b/arch/arm/include/asm/barrier.h
index d2f81e6b8c1c..646539a903a2 100644
--- a/arch/arm/include/asm/barrier.h
+++ b/arch/arm/include/asm/barrier.h
@@ -2,7 +2,6 @@
 #define __ASM_BARRIER_H
 
 #ifndef __ASSEMBLY__
-#include asm/outercache.h
 
 #define nop() __asm__ __volatile__(mov\tr0,r0\t@ nop\n\t);
 
@@ -37,12 +36,19 @@
 #define dmb(x) __asm__ __volatile__ ( : : : memory)
 #endif
 
+#ifdef CONFIG_ARM_HEAVY_MB
+extern void arm_heavy_mb(void);
+#define __arm_heavy_mb(x...) do { dsb(x); arm_heavy_mb(); } while (0)
+#else
+#define __arm_heavy_mb(x...) dsb(x)
+#endif
+
 #ifdef CONFIG_ARCH_HAS_BARRIERS
 #include mach/barriers.h
 #elif defined(CONFIG_ARM_DMA_MEM_BUFFERABLE) || defined(CONFIG_SMP)
-#define mb()   do { dsb(); outer_sync(); } while (0)
+#define mb()   __arm_heavy_mb()
 #define rmb()  dsb()
-#define wmb()  do { dsb(st); outer_sync(); } while (0)
+#define wmb()  __arm_heavy_mb(st)
 #define dma_rmb()  dmb(osh)
 #define dma_wmb()  dmb(oshst)
 #else
diff --git a/arch/arm/include/asm/outercache.h 
b/arch/arm/include/asm/outercache.h
index 563b92fc2f41..c2bf24f40177 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -129,21 +129,4 @@ static inline void outer_resume(void) { }
 
 #endif
 
-#ifdef CONFIG_OUTER_CACHE_SYNC
-/**
- * outer_sync - perform a sync point for outer cache
- *
- * Ensure that all outer cache operations are complete and any store
- * buffers are drained.
- */
-static inline void outer_sync(void)
-{
-   if (outer_cache.sync)
-   outer_cache.sync();
-}
-#else
-static inline void outer_sync(void)
-{ }
-#endif
-
 #endif /* __ASM_OUTERCACHE_H */
diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 350f188c92d2..b96c8ed1723a 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -39,6 +39,7 @@
 #include linux/export.h
 
 #include asm/hardware/cache-l2x0.h
+#include asm/outercache.h
 #include asm/exception.h
 #include asm/mach/arch.h
 #include asm/mach/irq.h
diff --git a/arch/arm/mach-omap2/include/mach/barriers.h 
b/arch/arm/mach-omap2/include/mach/barriers.h
deleted file mode 100644
index 1c582a8592b9..
--- a/arch/arm/mach-omap2/include/mach/barriers.h
+++ /dev/null
@@ -1,33 +0,0 @@
-/*
- * OMAP memory barrier

Re: ARM errata 430973 on multi platform kernels

[Russell King - ARM Linux]

On Thu, Apr 23, 2015 at 07:17:28AM -0700, Tony Lindgren wrote:
 * Russell King - ARM Linux li...@arm.linux.org.uk [150423 03:26]:
  However, I don't think anyone is willing to say that they have a
  solution to this problem - obviously, you can't build OMAP as a
  non-multiplatform kernel anymore, so in effect you can never have
  the kernel enable this errata.  And you can't detect whether you're
  running in secure mode or not.
  
  We could do the only write the bit if it was originally clear but
  we still have the problem that doing so may cause other people
  regressions.
 
 How about we keep the bit writing part !multiplatform conditional
 (or even remove it) but always do the flush for ca-8?
 
 Then we could also do a warning for a misconfigured ca-8 later on.

Yes, we could do this - we'll have to rely on the boot loader doing
the right thing and setting this bit appropriately.  For those
platforms where this doesn't apply, I don't see any solution as (iirc)
OMAP now requires MULTIPLATFORM to be enabled.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ARM errata 430973 on multi platform kernels

[Russell King - ARM Linux]

On Fri, Apr 24, 2015 at 10:54:29AM +0200, Matthijs van Duin wrote:
 On 23 April 2015 at 12:25, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
  And you can't detect whether you're running in secure mode or not.
 
 If not, you get an undefined instruction exception, which you could trap.
 
 This may not be convenient, but can't detect is an overstatement.

It's these kinds of statements that really piss me off.

At this stage in the boot, there's no memory allocators.  There's no
MMU.  There's really not very much.  There's no guarantee that the
location where the vectors are is writable on all platforms.

It's pretty much _impossible_ to do generically.

Can't detect is _not_ an overstatement.  It's a statement that I'm
giving you through my experience and knowledge of the Linux kernel,
the ARM archtecture, the capabilities of the platforms we have to
deal with, and how we want the kernel to work.

Sure, we _can_ detect it if (and only if) we code specifically for a
platform which has RAM at the CPU vector location.  Unfortunately,
that's a _very_ small proportion which approximates a number very
close to zero.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ARM errata 430973 on multi platform kernels

[Russell King - ARM Linux]

On Mon, Apr 20, 2015 at 04:40:32PM -0700, Tony Lindgren wrote:
 * Sebastian Reichel s...@kernel.org [150417 11:43]:
  On Thu, Apr 16, 2015 at 09:08:58AM -0700, Tony Lindgren wrote:
   * Sebastian Reichel s...@kernel.org [150415 09:32]:
Hi,

On Thu, Apr 09, 2015 at 02:48:43PM +0100, Russell King - ARM Linux 
wrote:
 On Thu, Apr 09, 2015 at 12:06:58AM +0100, Russell King - ARM Linux 
 wrote:
  On Tue, Apr 07, 2015 at 08:22:08AM -0700, Tony Lindgren wrote:
   Works for me. The above needs the following fix folded in to 
   build:
   
   --- a/arch/arm/mm/proc-v7.S
   +++ b/arch/arm/mm/proc-v7.S
   @@ -532,7 +532,7 @@ __v7_ca9mp_proc_info:
__v7_ca8_proc_info:
 .long   0x410fc080
 .long   0xff00
   - __v7_proc __v7_ca8mp_proc_info, proc_fns = 
   ca8_processor_functions
   + __v7_proc __v7_ca8_proc_info, __v7_setup, proc_fns = 
   ca8_processor_functions
 .size   __v7_ca8_proc_info, . - __v7_ca8_proc_info

#endif   /* CONFIG_ARM_LPAE */
  
  Thanks, merged into the original patch.
 
 Do you want to give me an ack for this, thanks?

I tried to test this together with Tony's follow up patch, but I get
this after applying the patch to v4.0:

sre@earth ~/src/linux [430973-fix] % make -j4
  CHK include/config/kernel.release
  CHK include/generated/uapi/linux/version.h
  CHK include/generated/utsrelease.h
make[1]: 'include/generated/mach-types.h' is up to date.
  CALLscripts/checksyscalls.sh
  CHK include/generated/compile.h
  AS  arch/arm/mm/proc-v7.o
arch/arm/mm/proc-v7.S: Assembler messages:
arch/arm/mm/proc-v7.S:535: Error: invalid operands (*ABS* and .text 
sections) for `|'
arch/arm/mm/proc-v7.S:535: Error: invalid operands (*ABS* and .text 
sections) for `|'
scripts/Makefile.build:294: recipe for target 'arch/arm/mm/proc-v7.o' 
failed
make[1]: *** [arch/arm/mm/proc-v7.o] Error 1
Makefile:947: recipe for target 'arch/arm/mm' failed
make: *** [arch/arm/mm] Error 2
make: *** Waiting for unfinished jobs
   
   Maybe test the version in Linux next:
   
   a6d746789825 (ARM: proc-v7: avoid errata 430973 workaround for 
   non-Cortex A8 CPUs)
  
  DONE with your your patch added on top:
  
  Tested-By: Sebastian Reichel s...@kernel.org
  
  (on N900)
 
 OK thanks, patch now uploaded to Russell's patch system:
 
 http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8345/1

I have a concern with that patch.

The reason that it's disabled for multiplatform is because we can't
guarantee that the auxctrl register will be writable.  The solution
we came up with for multiplatform was to require firmware to be
updated to enable this bit.

Enabling it on a platform where firmware has not been updated, but
runs in the non-secure world will lead to the kernel hanging in the
early assembly code.

I've discussed it with Catalin, and Catalin's position is that we
should not remove the !multiplatform conditional.  That's something
which I find that I'm agreeing with Catalin on - any other non-secure
Cortex A8 user who is setting this bit in firmware will instantly
break if this patch is applied.

However, I don't think anyone is willing to say that they have a
solution to this problem - obviously, you can't build OMAP as a
non-multiplatform kernel anymore, so in effect you can never have
the kernel enable this errata.  And you can't detect whether you're
running in secure mode or not.

We could do the only write the bit if it was originally clear but
we still have the problem that doing so may cause other people
regressions.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fscking OMAP

[Russell King - ARM Linux]

On Mon, Apr 20, 2015 at 04:49:13PM +0100, Russell King - ARM Linux wrote:
 Guys,
 
 If you're going to introduce a dependency on some infrastructure which
 you absolutely require for booting, then _please_ ensure that the
 configuration system is updated such that pre-existing kernel
 configurations continue to work.
 
 Don't leave it such that the infrastructure ends up returning -ENOSYS,
 which then filters down into your SoC specific code, and aborts the
 initialisation of critical infrastructure, leaving you with a totally
 silent boot failure.
 
 In this case, it's the addition of syscon to OMAP internals,
 specifically in omap_control_init().
 
 This is explicitly targetted at OMAP people, who have wasted much of
 my day today investigating why their platforms no longer boot in DT
 mode.

Let's be clear about how this needs to be fixed:

1. OMAP must select MFD_SYSCON in its Kconfig when DT is enabled, since
   OMAP silently fails to boot when this symbol is not enabled.

2. omap_clk_init() really needs to have some diagnostic printk()s in
   it to indicate when something like omap_control_init() fails.

3. omap_clk_init() probably should _not_ return immediately when one
   of the children (such as omap_control_init) fails to initialise, as
   omitting the clocks pretty much ensures that the kernel will BUG()
   a few moments later in the timer code.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fscking OMAP

[Russell King - ARM Linux]

On Mon, Apr 20, 2015 at 09:25:16AM -0700, Tony Lindgren wrote:
 * Russell King - ARM Linux li...@arm.linux.org.uk [150420 08:50]:
  Guys,
  
  If you're going to introduce a dependency on some infrastructure which
  you absolutely require for booting, then _please_ ensure that the
  configuration system is updated such that pre-existing kernel
  configurations continue to work.
  
  Don't leave it such that the infrastructure ends up returning -ENOSYS,
  which then filters down into your SoC specific code, and aborts the
  initialisation of critical infrastructure, leaving you with a totally
  silent boot failure.
  
  In this case, it's the addition of syscon to OMAP internals,
  specifically in omap_control_init().
  
  This is explicitly targetted at OMAP people, who have wasted much of
  my day today investigating why their platforms no longer boot in DT
  mode.
 
 Ouch, yeah missing dependencies are a pain to debug.
 
 Looks like we get MFD_SYSCON selected with omap2plus_defconfig,
 but that's not always the case like you pointed out.
 
 Does the patch below fix the issue for you?

Yes, that fixes it, thanks.

 8 ---
 From: Tony Lindgren t...@atomide.com
 Date: Mon, 20 Apr 2015 09:23:25 -0700
 Subject: [PATCH] ARM: OMAP2+: Fix booting with configs that don't have 
 MFD_SYSCON
 
 With the recent changes omaps have developed a dependency to MFD_SYSCON.
 This is used for system control module generic register area and some
 clocks.
 
 We do have it selected in omap2plus_defconfig, but targeted config
 files may not have it selected. Let's make sure it's selected like
 few other ARM platforms are already doing.
 
 Reported-by: Russell King rmk+ker...@arm.linux.org.uk
 Signed-off-by: Tony Lindgren t...@atomide.com
 
 --- a/arch/arm/mach-omap2/Kconfig
 +++ b/arch/arm/mach-omap2/Kconfig
 @@ -81,6 +81,7 @@ config ARCH_OMAP2PLUS
   select GENERIC_IRQ_CHIP
   select MACH_OMAP_GENERIC
   select MEMORY
 + select MFD_SYSCON
   select OMAP_DM_TIMER
   select OMAP_GPMC
   select PINCTRL

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fscking OMAP

[Russell King - ARM Linux]

Guys,

If you're going to introduce a dependency on some infrastructure which
you absolutely require for booting, then _please_ ensure that the
configuration system is updated such that pre-existing kernel
configurations continue to work.

Don't leave it such that the infrastructure ends up returning -ENOSYS,
which then filters down into your SoC specific code, and aborts the
initialisation of critical infrastructure, leaving you with a totally
silent boot failure.

In this case, it's the addition of syscon to OMAP internals,
specifically in omap_control_init().

This is explicitly targetted at OMAP people, who have wasted much of
my day today investigating why their platforms no longer boot in DT
mode.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: AM335x OMAP2 common clock external fixed-clock registration

[Russell King - ARM Linux]

On Fri, Apr 17, 2015 at 11:12:03AM +0200, Sebastian Hesselbarth wrote:
 On 17.04.2015 04:00, Michael Welling wrote:
 On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
 On 17.04.2015 00:09, Michael Welling wrote:
 On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
 On 16.04.2015 18:17, Michael Welling wrote:
 [...]
 What would be the proper error path?
 What cleanup is required?
 
 A proper error path would be to release any claimed resource
 on any error. If you look at the code, the only resources that
 need to be released are the two clocks in question.
 
 So for every error return in the probe function and in the of si5351_dt_parse
 it needs to clk_put first right?
 
 Not quite. The driver should clk_put() every clock that it called a
 [of_]clk_get() for. The thing is that clocks can be passed by
 platform_data and we never claim them.

I've always said clocks (as in struct clk) should never be passed through
platform data.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: AM335x OMAP2 common clock external fixed-clock registration

[Russell King - ARM Linux]

On Fri, Apr 17, 2015 at 02:06:23PM -0500, Michael Welling wrote:
 On Fri, Apr 17, 2015 at 11:18:33AM +0100, Russell King - ARM Linux wrote:
  On Fri, Apr 17, 2015 at 11:12:03AM +0200, Sebastian Hesselbarth wrote:
   On 17.04.2015 04:00, Michael Welling wrote:
   On Fri, Apr 17, 2015 at 01:23:50AM +0200, Sebastian Hesselbarth wrote:
   On 17.04.2015 00:09, Michael Welling wrote:
   On Thu, Apr 16, 2015 at 10:37:19PM +0200, Sebastian Hesselbarth wrote:
   On 16.04.2015 18:17, Michael Welling wrote:
   [...]
   What would be the proper error path?
   What cleanup is required?
   
   A proper error path would be to release any claimed resource
   on any error. If you look at the code, the only resources that
   need to be released are the two clocks in question.
   
   So for every error return in the probe function and in the of 
   si5351_dt_parse
   it needs to clk_put first right?
   
   Not quite. The driver should clk_put() every clock that it called a
   [of_]clk_get() for. The thing is that clocks can be passed by
   platform_data and we never claim them.
  
  I've always said clocks (as in struct clk) should never be passed through
  platform data.
 
 
 What is the alternative for systems that still use the old platform files?

clkdev, which has pre-existed DT.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND][PATCH] ARM errata, 430973: update the affected revisions

[Russell King - ARM Linux]

On Mon, Apr 13, 2015 at 09:28:09AM -0700, Tony Lindgren wrote:
 * Tony Lindgren t...@atomide.com [150413 07:46]:
  If what Russell and I are saying is correct, with the above two
  patches your system should behave properly with 430973 even if
  bit 6 in the aux ctrl register is set (or unset) by the bootloader.
  
  If r1p7 is behaves with bit 6 cleared and errata 430973 set, then
  we know r1p7 is unaffected by 430973.
 
 Sorry let's take this part again:
 
 If r1p7 is behaves with bit 6 cleared and errata 430973 _unset_,
 then we know r1p7 is unaffected by 430973.

I've asked.  The errata applies to _all_ Cortex A8 r1pX versions.
This is actually what the code in the kernel does today, but the
documentation does not reflect it.  So, I've updated the
documentation to reflect (a) the code and (b) the info I received:

http://ftp.arm.linux.org.uk/cgit/linux-arm.git/commit/?id=59552cc87cb6

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ARM errata 430973 on multi platform kernels

[Russell King - ARM Linux]

On Thu, Apr 09, 2015 at 08:09:19AM -0700, Tony Lindgren wrote:
 * Russell King - ARM Linux li...@arm.linux.org.uk [150409 06:49]:
  On Thu, Apr 09, 2015 at 12:06:58AM +0100, Russell King - ARM Linux wrote:
   On Tue, Apr 07, 2015 at 08:22:08AM -0700, Tony Lindgren wrote:
Works for me. The above needs the following fix folded in to build:

--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -532,7 +532,7 @@ __v7_ca9mp_proc_info:
 __v7_ca8_proc_info:
.long   0x410fc080
.long   0xff00
-   __v7_proc __v7_ca8mp_proc_info, proc_fns = 
ca8_processor_functions
+   __v7_proc __v7_ca8_proc_info, __v7_setup, proc_fns = 
ca8_processor_functions
.size   __v7_ca8_proc_info, . - __v7_ca8_proc_info
 
 #endif /* CONFIG_ARM_LPAE */
   
   Thanks, merged into the original patch.
  
  Do you want to give me an ack for this, thanks?
 
 The patch below works for me:
 
 Tested-by: Tony Lindgren t...@atomide.com
 
 I'm wondering if this and the follow-up patch should be tagged
 cc: stable?
 
 They together fix apps segfaulting both with and without 430973
 for some common use cases for distro kernels.

If we do, I'll want to merge both patches together...  For the time being,
I'll queue it without the stable tag.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ARM errata 430973 on multi platform kernels

[Russell King - ARM Linux]

On Thu, Apr 09, 2015 at 12:06:58AM +0100, Russell King - ARM Linux wrote:
 On Tue, Apr 07, 2015 at 08:22:08AM -0700, Tony Lindgren wrote:
  Works for me. The above needs the following fix folded in to build:
  
  --- a/arch/arm/mm/proc-v7.S
  +++ b/arch/arm/mm/proc-v7.S
  @@ -532,7 +532,7 @@ __v7_ca9mp_proc_info:
   __v7_ca8_proc_info:
  .long   0x410fc080
  .long   0xff00
  -   __v7_proc __v7_ca8mp_proc_info, proc_fns = ca8_processor_functions
  +   __v7_proc __v7_ca8_proc_info, __v7_setup, proc_fns = 
  ca8_processor_functions
  .size   __v7_ca8_proc_info, . - __v7_ca8_proc_info
   
   #endif /* CONFIG_ARM_LPAE */
 
 Thanks, merged into the original patch.

Do you want to give me an ack for this, thanks?

8===
From: Russell King rmk+ker...@arm.linux.org.uk
Subject: [PATCH] ARM: proc-v7: avoid errata 430973 workaround for non-Cortex
 A8 CPUs

Avoid the errata 430973 workaround for non-Cortex A8 CPUs.  Having this
workaround enabled introduces an additional branch target buffer flush
into the context switching path, something we wish to avoid.  To allow
this errata to be enabled in multiplatform kernels while reducing its
impact, rearrange the Cortex-A8 CPU support to avoid impacting on other
Version 7 CPUs.

Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 arch/arm/mm/proc-v7-2level.S | 12 
 arch/arm/mm/proc-v7.S| 28 
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/proc-v7-2level.S b/arch/arm/mm/proc-v7-2level.S
index bc86be205c04..fa385140715f 100644
--- a/arch/arm/mm/proc-v7-2level.S
+++ b/arch/arm/mm/proc-v7-2level.S
@@ -37,15 +37,18 @@
  * It is assumed that:
  * - we are not using split page tables
  */
-ENTRY(cpu_v7_switch_mm)
+ENTRY(cpu_ca8_switch_mm)
 #ifdef CONFIG_MMU
mov r2, #0
-   mmidr1, r1  @ get mm-context.id
-   ALT_SMP(orr r0, r0, #TTB_FLAGS_SMP)
-   ALT_UP(orr  r0, r0, #TTB_FLAGS_UP)
 #ifdef CONFIG_ARM_ERRATA_430973
mcr p15, 0, r2, c7, c5, 6   @ flush BTAC/BTB
 #endif
+#endif
+ENTRY(cpu_v7_switch_mm)
+#ifdef CONFIG_MMU
+   mmidr1, r1  @ get mm-context.id
+   ALT_SMP(orr r0, r0, #TTB_FLAGS_SMP)
+   ALT_UP(orr  r0, r0, #TTB_FLAGS_UP)
 #ifdef CONFIG_PID_IN_CONTEXTIDR
mrc p15, 0, r2, c13, c0, 1  @ read current context ID
lsr r2, r2, #8  @ extract the PID
@@ -61,6 +64,7 @@ ENTRY(cpu_v7_switch_mm)
 #endif
bx  lr
 ENDPROC(cpu_v7_switch_mm)
+ENDPROC(cpu_ca8_switch_mm)
 
 /*
  * cpu_v7_set_pte_ext(ptep, pte)
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index b1d19ea5e1af..003190ae9cd8 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -153,6 +153,21 @@ ENDPROC(cpu_v7_do_resume)
 #endif
 
 /*
+ * Cortex-A8
+ */
+   globl_equ   cpu_ca8_proc_init,  cpu_v7_proc_init
+   globl_equ   cpu_ca8_proc_fin,   cpu_v7_proc_fin
+   globl_equ   cpu_ca8_reset,  cpu_v7_reset
+   globl_equ   cpu_ca8_do_idle,cpu_v7_do_idle
+   globl_equ   cpu_ca8_dcache_clean_area, cpu_v7_dcache_clean_area
+   globl_equ   cpu_ca8_set_pte_ext,cpu_v7_set_pte_ext
+   globl_equ   cpu_ca8_suspend_size,   cpu_v7_suspend_size
+#ifdef CONFIG_ARM_CPU_SUSPEND
+   globl_equ   cpu_ca8_do_suspend, cpu_v7_do_suspend
+   globl_equ   cpu_ca8_do_resume,  cpu_v7_do_resume
+#endif
+
+/*
  * Cortex-A9 processor functions
  */
globl_equ   cpu_ca9mp_proc_init,cpu_v7_proc_init
@@ -471,7 +486,10 @@ __v7_setup_stack:
 
@ define struct processor (see asm/proc-fns.h and proc-macros.S)
define_processor_functions v7, dabort=v7_early_abort, pabort=v7_pabort, 
suspend=1
+#ifndef CONFIG_ARM_LPAE
+   define_processor_functions ca8, dabort=v7_early_abort, 
pabort=v7_pabort, suspend=1
define_processor_functions ca9mp, dabort=v7_early_abort, 
pabort=v7_pabort, suspend=1
+#endif
 #ifdef CONFIG_CPU_PJ4B
define_processor_functions pj4b, dabort=v7_early_abort, 
pabort=v7_pabort, suspend=1
 #endif
@@ -527,6 +545,16 @@ __v7_ca9mp_proc_info:
__v7_proc __v7_ca9mp_proc_info, __v7_ca9mp_setup, proc_fns = 
ca9mp_processor_functions
.size   __v7_ca9mp_proc_info, . - __v7_ca9mp_proc_info
 
+   /*
+* ARM Ltd. Cortex A8 processor.
+*/
+   .type   __v7_ca8_proc_info, #object
+__v7_ca8_proc_info:
+   .long   0x410fc080
+   .long   0xff00
+   __v7_proc __v7_ca8_proc_info, __v7_setup, proc_fns = 
ca8_processor_functions
+   .size   __v7_ca8_proc_info, . - __v7_ca8_proc_info
+
 #endif /* CONFIG_ARM_LPAE */
 
/*
-- 
1.8.3.1

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord

Re: ARM errata 430973 on multi platform kernels

[Russell King - ARM Linux]

On Tue, Apr 07, 2015 at 08:22:08AM -0700, Tony Lindgren wrote:
 Works for me. The above needs the following fix folded in to build:
 
 --- a/arch/arm/mm/proc-v7.S
 +++ b/arch/arm/mm/proc-v7.S
 @@ -532,7 +532,7 @@ __v7_ca9mp_proc_info:
  __v7_ca8_proc_info:
   .long   0x410fc080
   .long   0xff00
 - __v7_proc __v7_ca8mp_proc_info, proc_fns = ca8_processor_functions
 + __v7_proc __v7_ca8_proc_info, __v7_setup, proc_fns = 
 ca8_processor_functions
   .size   __v7_ca8_proc_info, . - __v7_ca8_proc_info
  
  #endif   /* CONFIG_ARM_LPAE */

Thanks, merged into the original patch.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ARM errata 430973 on multi platform kernels

[Russell King - ARM Linux]

On Tue, Apr 07, 2015 at 08:44:05AM -0700, Tony Lindgren wrote:
 And then on top of that patch, we can fix the sefaulting issues with the
 following, what do you guys think?

Has this change been tested on OMAP secure parts?

 8 
 From: Tony Lindgren t...@atomide.com
 Date: Tue, 7 Apr 2015 07:52:39 -0700
 Subject: [PATCH] ARM: mm: Fix Cortex-A8 erratum 430973 segfaults for 
 bootloaders and multiarch
 
 Looks like apps can be made to segfault easily on armhf distros
 just by running cpuburn-a8 in the background, then starting apt
 get update unless erratum 430973 workaround is enabled. This happens
 on r3p2 also, which has 430973 fixed in hardware.
 
 Turns out the reason for this is some bootloaders incorrectly
 setting the auxilary register IBE bit, which probably causes us
 to hit erratum 687067 on Cortex-A8 later than r1p2.
 
 Now that Cortex-A8 has it's own cpu_ca8_switch_mm, we can fix these
 issues:
 
 1. If the bootloader incorrectly sets the IBE bit in the auxilary
control register for Cortex-A8 revisions with 430973 fixed
in hardware, we need to call flush BTAC/BTB to avoid segfaults
probably caused by erratum 687067. So let's flush BTAC/BTB
unconditionally for Cortex-A8. It won't do anything unless the
IBE bit is set.
 
 2. We can and should now keep 430973 enabled for multiarch builds
as it no longer causes issues with Cortex-A9 as we have a separate
cpu_ca8_switch_mm.
 
 Note that SoCs probably should also add checks and print warnings
 for the misconfigured IBE bit depending on the Cortex-A8 revision
 so the bootloaders can be fixed Cortex-A8 revisions later than
 r1p2 to not set the IBE bit.
 
 Signed-off-by: Tony Lindgren t...@atomide.com
 
 --- a/arch/arm/mm/proc-v7-2level.S
 +++ b/arch/arm/mm/proc-v7-2level.S
 @@ -36,14 +36,16 @@
   *
   *   It is assumed that:
   *   - we are not using split page tables
 + *
 + *   Note that we always need to flush BTAC/BTB if IBE is set
 + *   even on Cortex-A8 revisions not affected by 430973.
 + *   If IBE is not set, the flush BTAC/BTB won't do anything.
   */
  ENTRY(cpu_ca8_switch_mm)
  #ifdef CONFIG_MMU
   mov r2, #0
 -#ifdef CONFIG_ARM_ERRATA_430973
   mcr p15, 0, r2, c7, c5, 6   @ flush BTAC/BTB
  #endif
 -#endif
  ENTRY(cpu_v7_switch_mm)
  #ifdef CONFIG_MMU
   mmidr1, r1  @ get mm-context.id
 --- a/arch/arm/mm/proc-v7.S
 +++ b/arch/arm/mm/proc-v7.S
 @@ -352,7 +352,7 @@ __v7_setup:
   ldr r10, =0x0c08@ Cortex-A8 primary part number
   teq r0, r10
   bne 2f
 -#if defined(CONFIG_ARM_ERRATA_430973)  !defined(CONFIG_ARCH_MULTIPLATFORM)
 +#if defined(CONFIG_ARM_ERRATA_430973)
  
   teq r5, #0x0010 @ only present in r1p*
   mrceq   p15, 0, r10, c1, c0, 1  @ read aux control register

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/14] clkdev: get rid of redundant clk_add_alias() prototype in linux/clk.h

[Russell King - ARM Linux]

On Mon, Apr 06, 2015 at 12:04:21PM -0700, Stephen Boyd wrote:
 On 04/04/15 05:43, Robert Jarzmik wrote:
  Russell King rmk+ker...@arm.linux.org.uk writes:
 
  clk_add_alias() is provided by clkdev, and is not part of the clk API.
  Howver, it is prototyped in two locations: linux/clkdev.h and
  linux/clk.h.  This is a mess.  Get rid of the redundant and unnecessary
  version in linux/clk.h.
 
  Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
  Tested-by: Robert Jarzmik robert.jarz...@free.fr
 
  Actually, this serie fixes a regression I've seen in linux-next, and which 
  was
  triggering the Oops in [1] on lubbock. With your serie, the kernel boots 
  fine.
 
 
 
 Is this with the lubbock_defconfig? Is it a regression in 4.0-rc series
 or is it due to some pending -next patches interacting with the per-user
 clock patches? It looks like the latter because __clk_get_hw() should be
 inlined on lubbock_defconfig where CONFIG_COMMON_CLK=n.

It's a regression with anything that uses clk_add_alias() or which
open codes the aliasing of clocks, or registration of clocks into
clkdev.  It's quite nasty.

The problem is that the way you updated clkdev, by adding __clk_get_hw()
at clk_get() time, the struct clk which we're passing in there could
well have been already clk_put()'d, and therefore freed, and no longer
valid.

It's a regression ever since the per-user clk patches went in, caused
_entirely_ by those patches.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/14] clkdev: add clkdev_create() helper

[Russell King - ARM Linux]

On Mon, Apr 06, 2015 at 01:19:33PM -0700, Stephen Boyd wrote:
 On 04/03/15 10:12, Russell King wrote:
  @@ -316,6 +329,29 @@ clkdev_alloc(struct clk *clk, const char *con_id, 
  const char *dev_fmt, ...)
   }
   EXPORT_SYMBOL(clkdev_alloc);
   
  +/**
  + * clkdev_create - allocate and add a clkdev lookup structure
  + * @clk: struct clk to associate with all clk_lookups
  + * @con_id: connection ID string on device
  + * @dev_fmt: format string describing device name
  + *
  + * Returns a clk_lookup structure, which can be later unregistered and
  + * freed.
 
 Please add that this returns NULL on failure.

Will do, but please remember that _I'm_ taking the clkdev patches through
my tree, as I'm the maintainer for clkdev.  Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html