* [PATCH 3/3] arm64: dump: Add checking for writable and exectuable pages
From: Kees Cook @ 2016-09-30 17:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160930164147.GD1729@remoulade>
On Fri, Sep 30, 2016 at 9:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Sep 30, 2016 at 09:25:45AM -0700, Kees Cook wrote:
>> On Fri, Sep 30, 2016 at 8:58 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>
>> > Would it be worth verifying that all kernel mappings are UXN, too?
>> >
>> > ARMv8 allows execute-only mappings, and a !UXN mapping could result in an info
>> > leak (e.g. pointers in MOVZ+MOVK sequences), or potential asynchronous issues
>> > (e.g. user instruction fetches accessing read-destructive device registers).
>> > All kernel mappings *should* be UXN.
>>
>> I love this idea, but based on what came up with hardened usercopy,
>> there are a lot of readers of kernel memory still. I think the
>> expectations around UXN need to be clarified so we can reason about
>> things like perf that want to read the kernel text.
>
> The UXN (User eXecute Never) bit only controls whether userspace can execute a
> page, not whether the kernel can read it. The RW permissions come from the AP
> bits regardless.
Ah! Sorry, I misunderstood. Yeah, UXN checking makes sense there then. :)
> We already try to ensure that all kernel memory is UXN by construction, so this
> would just be a sanity check, as with the rest of the W^X checks.
>
> The MOVZ+MOVK case above is where a sequence of 16-bit immediate MOVs are used
> to encode a pointer. If a kernel mapping lacked UXN, userspace could execute it
> (unprivileged), and extract the pointer generated into a GPR.
>
> Having kernel exec-only memory is a different story entirely, though I agree
> it's something to look into.
Yeah, this'll need to get sorted out for x86 too.
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply
* [PATCH] mtd: mtk: avoid warning in mtk_ecc_encode
From: Arnd Bergmann @ 2016-09-30 17:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160930185139.15c8be66@bbrezillon>
On Friday 30 September 2016, Boris Brezillon wrote:
> > + /* copy into possibly unaligned OOB region with actual length */
> > + memcpy(data + bytes, eccdata, len);
>
> Is it better than
>
> for (i = 0; i < len; i += 4) {
> u32 val = __raw_readl(ecc->regs + ECC_ENCPAR(i / 4));
>
> memcpy(data + bytes + i, &val, min(len, 4));
> }
>
> I'm probably missing something, but what's the point of creating a
> temporary buffer of 112 bytes on the stack since you'll have to copy
> this data to the oob buffer at some point?
I tried something like that first, but wasn't too happy with it for
a number of small reasons:
- __raw_readl in a driver is not usually the right API, __memcpy32_from_io
uses it internally, but it's better for a driver not to rely on that,
in case we need some barriers (which we may in factt need for other drivers).
- the min(len,4) expression is incorrect, fixing that makes it more complicated
again
- I didn't like to call memcpy() multiple times, as that might get turned
into an external function call (the compiler is free to optimize small
memcpy calls or not).
I agree that he 112 byte buffer isn't ideal either, it just seemed to
be the lesser annoyance.
Arnd
^ permalink raw reply
* [RFC] arm64: Enforce observed order for spinlock and data
From: Brent DeGraaf @ 2016-09-30 17:40 UTC (permalink / raw)
To: linux-arm-kernel
Prior spinlock code solely used load-acquire and store-release
semantics to ensure ordering of the spinlock lock and the area it
protects. However, store-release semantics and ordinary stores do
not protect against accesses to the protected area being observed
prior to the access that locks the lock itself.
While the load-acquire and store-release ordering is sufficient
when the spinlock routines themselves are strictly used, other
kernel code that references the lock values directly (e.g. lockrefs)
could observe changes to the area protected by the spinlock prior
to observance of the lock itself being in a locked state, despite
the fact that the spinlock logic itself is correct.
Barriers were added to all the locking routines wherever necessary
to ensure that outside observers which read the lock values directly
will not observe changes to the protected data before the lock itself
is observed.
Signed-off-by: Brent DeGraaf <bdegraaf@codeaurora.org>
---
arch/arm64/include/asm/spinlock.h | 59 ++++++++++++++++++++++++++++++++++++---
1 file changed, 55 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index 89206b5..4dd0977 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -106,7 +106,20 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
/* Did we get the lock? */
" eor %w1, %w0, %w0, ror #16\n"
-" cbz %w1, 3f\n"
+" cbnz %w1, 4f\n"
+ /*
+ * Yes: The store done on this cpu was the one that locked the lock.
+ * Store-release one-way barrier on LL/SC means that accesses coming
+ * after this could be reordered into the critical section of the
+ * load-acquire/store-release, where we did not own the lock. On LSE,
+ * even the one-way barrier of the store-release semantics is missing,
+ * so LSE needs an explicit barrier here as well. Without this, the
+ * changed contents of the area protected by the spinlock could be
+ * observed prior to the lock.
+ */
+" dmb ish\n"
+" b 3f\n"
+"4:\n"
/*
* No: spin on the owner. Send a local event to avoid missing an
* unlock before the exclusive load.
@@ -116,7 +129,15 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
" ldaxrh %w2, %4\n"
" eor %w1, %w2, %w0, lsr #16\n"
" cbnz %w1, 2b\n"
- /* We got the lock. Critical section starts here. */
+ /*
+ * We got the lock and have observed the prior owner's store-release.
+ * In this case, the one-way barrier of the prior owner that we
+ * observed combined with the one-way barrier of our load-acquire is
+ * enough to ensure accesses to the protected area coming after this
+ * are not accessed until we own the lock. In this case, other
+ * observers will not see our changes prior to observing the lock
+ * itself. Critical locked section starts here.
+ */
"3:"
: "=&r" (lockval), "=&r" (newval), "=&r" (tmp), "+Q" (*lock)
: "Q" (lock->owner), "I" (1 << TICKET_SHIFT)
@@ -137,6 +158,13 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
" add %w0, %w0, %3\n"
" stxr %w1, %w0, %2\n"
" cbnz %w1, 1b\n"
+ /*
+ * We got the lock with a successful store-release: Store-release
+ * one-way barrier means accesses coming after this could be observed
+ * before the lock is observed as locked.
+ */
+ " dmb ish\n"
+ " nop\n"
"2:",
/* LSE atomics */
" ldr %w0, %2\n"
@@ -146,6 +174,13 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
" casa %w0, %w1, %2\n"
" and %w1, %w1, #0xffff\n"
" eor %w1, %w1, %w0, lsr #16\n"
+ " cbnz %w1, 1f\n"
+ /*
+ * We got the lock with the LSE casa store.
+ * A barrier is required to ensure accesses coming from the
+ * critical section of the lock are not observed before our lock.
+ */
+ " dmb ish\n"
"1:")
: "=&r" (lockval), "=&r" (tmp), "+Q" (*lock)
: "I" (1 << TICKET_SHIFT)
@@ -212,6 +247,12 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
" cbnz %w0, 1b\n"
" stxr %w0, %w2, %1\n"
" cbnz %w0, 2b\n"
+ /*
+ * Lock is not ours until the store, which has no implicit barrier.
+ * Barrier is needed so our writes to the protected area are not
+ * observed before our lock ownership is observed.
+ */
+ " dmb ish\n"
" nop",
/* LSE atomics */
"1: mov %w0, wzr\n"
@@ -221,7 +262,12 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
" cbz %w0, 2b\n"
" wfe\n"
" b 1b\n"
- "3:")
+ /*
+ * Casa doesn't use store-release semantics. Even if it did,
+ * it would not protect us from our writes being observed before
+ * our ownership is observed. Barrier is required.
+ */
+ "3: dmb ish")
: "=&r" (tmp), "+Q" (rw->lock)
: "r" (0x80000000)
: "memory");
@@ -299,7 +345,12 @@ static inline void arch_read_lock(arch_rwlock_t *rw)
" tbnz %w1, #31, 1b\n"
" casa %w0, %w1, %2\n"
" sbc %w0, %w1, %w0\n"
- " cbnz %w0, 2b")
+ " cbnz %w0, 2b\n"
+ /*
+ * Need to ensure that our reads of the area protected by the lock
+ * are not observed before our lock ownership is observed.
+ */
+ " dmb ish\n")
: "=&r" (tmp), "=&r" (tmp2), "+Q" (rw->lock)
:
: "cc", "memory");
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related
* [PATCH][V2] dmaengine: coh901318: fix integer overflow when shifting more than 32 places
From: Vinod Koul @ 2016-09-30 17:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160929181438.14374-1-colin.king@canonical.com>
On Thu, Sep 29, 2016 at 07:14:38PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently U300_DMA_CHANNELS is set to 40, meaning that the shift of 1 can
> be more than 32 places, which leads to a 32 bit integer overflow. Fix this
> by using 1ULL instead of 1 before shifting it. Also add braces on the
> for-loop to keep with coding style conventions.
Applied, thanks
--
~Vinod
^ permalink raw reply
* [kernel-hardening] Re: [PATCH v3 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
From: Kees Cook @ 2016-09-30 18:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160929224452.GA71670@samitolvanen.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 3:44 PM, Sami Tolvanen <samitolvanen@google.com> wrote:
> On Thu, Sep 15, 2016 at 05:20:45PM +0100, Mark Rutland wrote:
>> Likewise, how do we handle __flush_cache_user_range and
>> flush_icache_range? Some callers (e.g. __do_compat_cache_op) pass in
>> __user addresses.
>
> Also EXEC_USERSPACE in lkdtm passes a user space address to flush_icache_range
> and causes the process to hang when I tested these patches on HiKey.
>
> Adding uaccess_{enable,disable}_not_uao to __flush_cache_user_range appears to
> fix the problem.
I had a thought just now on this: is lkdtm maybe doing the wrong thing
here? i.e. should lkdtm be the one do to the uaccess_en/disable
instead of flush_icache_range() itself, since it's the one abusing the
API?
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply
* [RFC] arm64: Enforce observed order for spinlock and data
From: Robin Murphy @ 2016-09-30 18:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475257257-23072-1-git-send-email-bdegraaf@codeaurora.org>
Hi Brent,
On 30/09/16 18:40, Brent DeGraaf wrote:
> Prior spinlock code solely used load-acquire and store-release
> semantics to ensure ordering of the spinlock lock and the area it
> protects. However, store-release semantics and ordinary stores do
> not protect against accesses to the protected area being observed
> prior to the access that locks the lock itself.
>
> While the load-acquire and store-release ordering is sufficient
> when the spinlock routines themselves are strictly used, other
> kernel code that references the lock values directly (e.g. lockrefs)
> could observe changes to the area protected by the spinlock prior
> to observance of the lock itself being in a locked state, despite
> the fact that the spinlock logic itself is correct.
>
> Barriers were added to all the locking routines wherever necessary
> to ensure that outside observers which read the lock values directly
> will not observe changes to the protected data before the lock itself
> is observed.
>
> Signed-off-by: Brent DeGraaf <bdegraaf@codeaurora.org>
> ---
> arch/arm64/include/asm/spinlock.h | 59 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
> index 89206b5..4dd0977 100644
> --- a/arch/arm64/include/asm/spinlock.h
> +++ b/arch/arm64/include/asm/spinlock.h
> @@ -106,7 +106,20 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
>
> /* Did we get the lock? */
> " eor %w1, %w0, %w0, ror #16\n"
> -" cbz %w1, 3f\n"
> +" cbnz %w1, 4f\n"
> + /*
> + * Yes: The store done on this cpu was the one that locked the lock.
> + * Store-release one-way barrier on LL/SC means that accesses coming
> + * after this could be reordered into the critical section of the
> + * load-acquire/store-release, where we did not own the lock. On LSE,
> + * even the one-way barrier of the store-release semantics is missing,
> + * so LSE needs an explicit barrier here as well. Without this, the
> + * changed contents of the area protected by the spinlock could be
> + * observed prior to the lock.
What is that last sentence supposed to mean? If the lock is free, then
surely any previous writes to the data it's protecting would have
already been observed by the release semantics of the previous unlock?
If the lock is currently held, what do we care about the state of the
data while we're still spinning on the lock itself? And if someone's
touching the data without having acquired *or* released the lock, why is
there a lock in the first place?
This seems like a very expensive way of papering over broken callers :/
Robin.
> + */
> +" dmb ish\n"
> +" b 3f\n"
> +"4:\n"
> /*
> * No: spin on the owner. Send a local event to avoid missing an
> * unlock before the exclusive load.
> @@ -116,7 +129,15 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
> " ldaxrh %w2, %4\n"
> " eor %w1, %w2, %w0, lsr #16\n"
> " cbnz %w1, 2b\n"
> - /* We got the lock. Critical section starts here. */
> + /*
> + * We got the lock and have observed the prior owner's store-release.
> + * In this case, the one-way barrier of the prior owner that we
> + * observed combined with the one-way barrier of our load-acquire is
> + * enough to ensure accesses to the protected area coming after this
> + * are not accessed until we own the lock. In this case, other
> + * observers will not see our changes prior to observing the lock
> + * itself. Critical locked section starts here.
> + */
> "3:"
> : "=&r" (lockval), "=&r" (newval), "=&r" (tmp), "+Q" (*lock)
> : "Q" (lock->owner), "I" (1 << TICKET_SHIFT)
> @@ -137,6 +158,13 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
> " add %w0, %w0, %3\n"
> " stxr %w1, %w0, %2\n"
> " cbnz %w1, 1b\n"
> + /*
> + * We got the lock with a successful store-release: Store-release
> + * one-way barrier means accesses coming after this could be observed
> + * before the lock is observed as locked.
> + */
> + " dmb ish\n"
> + " nop\n"
> "2:",
> /* LSE atomics */
> " ldr %w0, %2\n"
> @@ -146,6 +174,13 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
> " casa %w0, %w1, %2\n"
> " and %w1, %w1, #0xffff\n"
> " eor %w1, %w1, %w0, lsr #16\n"
> + " cbnz %w1, 1f\n"
> + /*
> + * We got the lock with the LSE casa store.
> + * A barrier is required to ensure accesses coming from the
> + * critical section of the lock are not observed before our lock.
> + */
> + " dmb ish\n"
> "1:")
> : "=&r" (lockval), "=&r" (tmp), "+Q" (*lock)
> : "I" (1 << TICKET_SHIFT)
> @@ -212,6 +247,12 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
> " cbnz %w0, 1b\n"
> " stxr %w0, %w2, %1\n"
> " cbnz %w0, 2b\n"
> + /*
> + * Lock is not ours until the store, which has no implicit barrier.
> + * Barrier is needed so our writes to the protected area are not
> + * observed before our lock ownership is observed.
> + */
> + " dmb ish\n"
> " nop",
> /* LSE atomics */
> "1: mov %w0, wzr\n"
> @@ -221,7 +262,12 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
> " cbz %w0, 2b\n"
> " wfe\n"
> " b 1b\n"
> - "3:")
> + /*
> + * Casa doesn't use store-release semantics. Even if it did,
> + * it would not protect us from our writes being observed before
> + * our ownership is observed. Barrier is required.
> + */
> + "3: dmb ish")
> : "=&r" (tmp), "+Q" (rw->lock)
> : "r" (0x80000000)
> : "memory");
> @@ -299,7 +345,12 @@ static inline void arch_read_lock(arch_rwlock_t *rw)
> " tbnz %w1, #31, 1b\n"
> " casa %w0, %w1, %2\n"
> " sbc %w0, %w1, %w0\n"
> - " cbnz %w0, 2b")
> + " cbnz %w0, 2b\n"
> + /*
> + * Need to ensure that our reads of the area protected by the lock
> + * are not observed before our lock ownership is observed.
> + */
> + " dmb ish\n")
> : "=&r" (tmp), "=&r" (tmp2), "+Q" (rw->lock)
> :
> : "cc", "memory");
>
^ permalink raw reply
* [RFC] arm64: Enforce observed order for spinlock and data
From: Peter Zijlstra @ 2016-09-30 18:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475257257-23072-1-git-send-email-bdegraaf@codeaurora.org>
On Fri, Sep 30, 2016 at 01:40:57PM -0400, Brent DeGraaf wrote:
> Prior spinlock code solely used load-acquire and store-release
> semantics to ensure ordering of the spinlock lock and the area it
> protects. However, store-release semantics and ordinary stores do
> not protect against accesses to the protected area being observed
> prior to the access that locks the lock itself.
>
> While the load-acquire and store-release ordering is sufficient
> when the spinlock routines themselves are strictly used, other
> kernel code that references the lock values directly (e.g. lockrefs)
> could observe changes to the area protected by the spinlock prior
> to observance of the lock itself being in a locked state, despite
> the fact that the spinlock logic itself is correct.
>
> Barriers were added to all the locking routines wherever necessary
> to ensure that outside observers which read the lock values directly
> will not observe changes to the protected data before the lock itself
> is observed.
I cannot see this going in. You're making spinlocks far more expensive
in the common case that doesn't need this.
Please enumerate the special cases (there's more than just lockref?) and
fix those.
^ permalink raw reply
* [PATCH] ARM: multi_v7_defconfig: Enable BCM47xx/BCM5301x drivers
From: Florian Fainelli @ 2016-09-30 19:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1472339552-9960-1-git-send-email-f.fainelli@gmail.com>
On 08/27/2016 04:12 PM, Florian Fainelli wrote:
> Add a bunch of required drivers and subsystems:
>
> - BCMA is the on-chip discoverable bus which registers a bunch of
> peripherals
> - Enable the BCM47xx watchdog driver to get working system reboot
> - Enable the BCM47xx NVRAM/SPROM drivers to be able to fetch MAC
> addresses and other variables needed for system operation
> - Make BGMAC (built-in Ethernet adapter) a module
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Applied to defconfig/next.
--
Florian
^ permalink raw reply
* [PATCH 1/3] ARM: BCM5301X: Add separated DTS include file for BCM47094
From: Florian Fainelli @ 2016-09-30 19:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160921205838.15627-1-zajec5@gmail.com>
On 09/21/2016 01:58 PM, Rafa? Mi?ecki wrote:
> From: Rafa? Mi?ecki <rafal@milecki.pl>
>
> Use it to store BCM47094 specific properties/values and avoid repeating
> them in device DTS files.
Series applied, thanks!
--
Florian
^ permalink raw reply
* [PATCH 1/2] ARM: BCM5301X: Add DT for Luxul XAP-1510
From: Florian Fainelli @ 2016-09-30 19:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474997231-27700-1-git-send-email-dhaab@luxul.com>
On 09/27/2016 10:27 AM, Dan Haab wrote:
> Luxul XAP-1510 is an AP device based on BCM4708 SoC with 2 x BCM4360
> chipsets on PCB connected using PCIe.
>
> Signed-off-by: Dan Haab <dhaab@luxul.com>
Series applied, thanks!
--
Florian
^ permalink raw reply
* [RFC] arm64: Enforce observed order for spinlock and data
From: Peter Zijlstra @ 2016-09-30 19:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475257257-23072-1-git-send-email-bdegraaf@codeaurora.org>
On Fri, Sep 30, 2016 at 01:40:57PM -0400, Brent DeGraaf wrote:
> Prior spinlock code solely used load-acquire and store-release
> semantics to ensure ordering of the spinlock lock and the area it
> protects. However, store-release semantics and ordinary stores do
> not protect against accesses to the protected area being observed
> prior to the access that locks the lock itself.
>
> While the load-acquire and store-release ordering is sufficient
> when the spinlock routines themselves are strictly used, other
> kernel code that references the lock values directly (e.g. lockrefs)
Isn't the problem with lockref the fact that arch_spin_value_unlocked()
isn't a load-acquire, and therefore the CPU in question doesn't need to
observe the contents of the critical section etc..?
That is, wouldn't fixing arch_spin_value_unlocked() by making that an
smp_load_acquire() fix things much better?
> could observe changes to the area protected by the spinlock prior
> to observance of the lock itself being in a locked state, despite
> the fact that the spinlock logic itself is correct.
^ permalink raw reply
* [PATCH 6/6] ARM: da850: adjust memory settings for tilcdc
From: Peter Ujfalusi @ 2016-09-30 19:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMpxmJXafyT5G8ZyD8RBAtzD6R9HmvyS0NBpz5i2fJ0SwOhXBA@mail.gmail.com>
On 09/30/2016 06:06 PM, Bartosz Golaszewski wrote:
> 2016-09-30 14:59 GMT+02:00 Peter Ujfalusi <peter.ujfalusi@ti.com>:
>> On 09/29/16 19:31, Bartosz Golaszewski wrote:
>>> Default memory settings of da850 do not meet the throughput/latency
>>> requirements of tilcdc. This results in the image displayed being
>>> incorrect and the following warning being displayed by the LCDC
>>> drm driver:
>>>
>>> tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow
>>>
>>> Reconfigure the LCDC priority to the highest. This is a workaround
>>> for the da850-lcdk board which has the LCD controller enabled in
>>> the device tree, but a long-term, system-wide fix is needed for
>>> all davinci boards.
>>>
>>> This patch has been modified for mainline linux. It comes from a
>>> downstream TI release for da850[1].
>>>
>>> Original author: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com>
>>>
>
> [snip]
>
>>
>> Is this safe to do for all da850 boards (to change PR_OLD_COUNT from 0xff to
>> 0x20)? Most probably it is, but this setting has nothing to do with LCDC.
>>
>> The whole priority configuration has nothing to do with the LCDC, it is a
>> system level priority.
>>
>> Now you have lowered the eDMA3_0-TPTC0/1 priority. Audio is serviced by
>> eDMA3_0-TPTC1. So are we going to see issues in audio if LCDC is taking the
>> highest priority?
>>
>
> Just ran a quick test with speaker-test -c2 -twav. Besides the fact
> that the left and right channels are inverted (I'm looking into that),
> I didn't notice any problems. Even at 1024x768 resolution, playing
> audio at the same time seems to work fine.
That's good to hear, but I think the priorities should be set:
LCDC and EDMA30TC1 to highest priority
EDMA30TC0 to priority 2
The 0TC0 is used by MMC and if you want to play a video you might need the
servicing TC to be higher priority then other masters.
If audio playback would trigger sync losts in lcdc then we might need to move
0TC1 to priority 1.
I agree that LCDC priority needs to be higher, but I do wonder why the default
(5) is not working and if it is not working why it is 5...
My guess is that the change in the PBBPR register is the one actually helping
here.
--
P?ter
^ permalink raw reply
* [GIT PULL 1/4] Broadcom devicetree changes for 4.9 (part 2)
From: Florian Fainelli @ 2016-09-30 19:23 UTC (permalink / raw)
To: linux-arm-kernel
The following changes since commit 7260ecd22baa3c62ef6efaf59e7de53f4c8df800:
Merge tag 'bcm2835-dt-next-2016-08-29' into devicetree/next (2016-08-30 20:02:04 -0700)
are available in the git repository at:
http://github.com/Broadcom/stblinux.git tags/arm-soc/for-4.9/devicetree-part2
for you to fetch changes up to ef3bc318adeb15b38688df6a583bafea2befce43:
ARM: BCM5301X: Add DT for Luxul XWR-3100 (2016-09-30 11:58:31 -0700)
----------------------------------------------------------------
This pull request contains Broadcom ARM-based SoCs Device Tree changes for 4.9,
please pull the following:
- Rafal adds support for the Netgear R8500 routers, adds basic support
for the Tenda AC9 router which uses the new BCM53573 SoC (single core Cortex
A7). He also enables the UART on the Netgear R8000 and restructures the
include files a bit for the BCM47094 SoC, finally he adds USB 3.0 PHY nodes
which enables USB 3.0 on BCM5301X devices that support it
- Kamal adds support for the QSPI controller on the Northstar Plus SoCs and updates
the bcm958625k reference board to have it enabled
- Dan adds support for the Luxul XAP-1510 and XWR-3100 devices
----------------------------------------------------------------
Dan Haab (2):
ARM: BCM5301X: Add DT for Luxul XAP-1510
ARM: BCM5301X: Add DT for Luxul XWR-3100
Kamal Dasu (1):
ARM: dts: NSP: Add QSPI nodes to NSPI and bcm958625k DTSes
Rafa? Mi?ecki (5):
ARM: BCM5301X: Add DT for Netgear R8500
ARM: BCM5301X: Add basic dts for BCM53573 based Tenda AC9
ARM: BCM5301X: Add separated DTS include file for BCM47094
ARM: BCM5301X: Enable UART on Netgear R8000
ARM: BCM5301X: Specify USB 3.0 PHY in DT
arch/arm/boot/dts/Makefile | 5 +
arch/arm/boot/dts/bcm-nsp.dtsi | 31 ++++-
arch/arm/boot/dts/bcm4708-luxul-xap-1510.dts | 64 ++++++++++
arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts | 2 +-
arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts | 2 +-
arch/arm/boot/dts/bcm4709-netgear-r7000.dts | 2 +-
arch/arm/boot/dts/bcm4709-netgear-r8000.dts | 6 +-
arch/arm/boot/dts/bcm4709.dtsi | 11 ++
arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts | 3 +-
arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dts | 111 ++++++++++++++++
arch/arm/boot/dts/bcm47094-netgear-r8500.dts | 103 +++++++++++++++
arch/arm/boot/dts/bcm47094.dtsi | 17 +++
arch/arm/boot/dts/bcm47189-tenda-ac9.dts | 74 +++++++++++
arch/arm/boot/dts/bcm5301x-nand-cs0-bch4.dtsi | 13 ++
arch/arm/boot/dts/bcm5301x.dtsi | 7 ++
arch/arm/boot/dts/bcm53573.dtsi | 147 ++++++++++++++++++++++
arch/arm/boot/dts/bcm958625k.dts | 34 +++++
17 files changed, 625 insertions(+), 7 deletions(-)
create mode 100644 arch/arm/boot/dts/bcm4708-luxul-xap-1510.dts
create mode 100644 arch/arm/boot/dts/bcm4709.dtsi
create mode 100644 arch/arm/boot/dts/bcm47094-luxul-xwr-3100.dts
create mode 100644 arch/arm/boot/dts/bcm47094-netgear-r8500.dts
create mode 100644 arch/arm/boot/dts/bcm47094.dtsi
create mode 100644 arch/arm/boot/dts/bcm47189-tenda-ac9.dts
create mode 100644 arch/arm/boot/dts/bcm5301x-nand-cs0-bch4.dtsi
create mode 100644 arch/arm/boot/dts/bcm53573.dtsi
^ permalink raw reply
* [GIT PULL 2/4] Broadcom devicetree-arm64 changes for 4.9 (part 2)
From: Florian Fainelli @ 2016-09-30 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475263395-27653-1-git-send-email-f.fainelli@gmail.com>
The following changes since commit 5072ed1fa29fa7cef73a7fb82f696e50973e02dc:
arm64: dts: Add PWM DT node for NS2 (2016-08-08 11:03:58 -0700)
are available in the git repository at:
http://github.com/Broadcom/stblinux.git tags/arm-soc/for-4.9/devicetree-arm64-part2
for you to fetch changes up to 33fb674b66f2529701d0b5b5e78a446fc377a8f1:
ARM64: dts: NS2: Add QSPI Device Tree node (2016-09-14 13:30:50 -0700)
----------------------------------------------------------------
This pull request contains Broadcom ARM64-based SoCs Device Tree changes for
4.9, please pull the following:
- Kamal adds the QSPI Device Tree node to the Northstar 2 SoC and updates the
Northstar 2 SVK reference board DTS file with it enabled.
----------------------------------------------------------------
Kamal Dasu (1):
ARM64: dts: NS2: Add QSPI Device Tree node
arch/arm64/boot/dts/broadcom/ns2-svk.dts | 34 ++++++++++++++++++++++++++++++++
arch/arm64/boot/dts/broadcom/ns2.dtsi | 18 +++++++++++++++++
2 files changed, 52 insertions(+)
^ permalink raw reply
* [GIT PULL 3/4] Broadcom defconfig changes for 4.9 (part 2)
From: Florian Fainelli @ 2016-09-30 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475263395-27653-1-git-send-email-f.fainelli@gmail.com>
The following changes since commit 29b4817d4018df78086157ea3a55c1d9424a7cfc:
Linux 4.8-rc1 (2016-08-07 18:18:00 -0700)
are available in the git repository at:
http://github.com/Broadcom/stblinux.git tags/arm-soc/for-4.9/defconfig
for you to fetch changes up to ca2a92f87d9d8dc070f91f88ecd0f0d4c5f4dfad:
ARM: multi_v7_defconfig: Enable BCM47xx/BCM5301x drivers (2016-09-30 11:53:55 -0700)
----------------------------------------------------------------
This pull request contains Broadcom ARM-based defconfig changes for 4.9, please
pull the following:
- Florian updates the multi_v7_defconfig with the relevant basic drivers needed
for the Broadcom BCM5301x (Northstar) SoCs to reboot, have PCIe, and Ethernet
----------------------------------------------------------------
Florian Fainelli (1):
ARM: multi_v7_defconfig: Enable BCM47xx/BCM5301x drivers
arch/arm/configs/multi_v7_defconfig | 13 +++++++++++++
1 file changed, 13 insertions(+)
^ permalink raw reply
* [GIT PULL 4/4] Broadcom defconfig-arm64 changes for 4.9 (part 2)
From: Florian Fainelli @ 2016-09-30 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475263395-27653-1-git-send-email-f.fainelli@gmail.com>
The following changes since commit 29b4817d4018df78086157ea3a55c1d9424a7cfc:
Linux 4.8-rc1 (2016-08-07 18:18:00 -0700)
are available in the git repository at:
http://github.com/Broadcom/stblinux.git tags/arm-soc/for-4.9/defconfig-arm64
for you to fetch changes up to 51e3fb1d3f514cd336faf185df73b25fca194773:
Merge tag 'bcm2835-defconfig-64-next-2016-09-22' into defconfig-arm64/next (2016-09-30 12:02:29 -0700)
----------------------------------------------------------------
This pull request contains Broadcom ARM64-based SoCs defconfig changes for 4.9,
please pull the following changes:
- Eric updates the ARMv8 defconfig to contain everything that is needed to run
a 64-bit kernel on the Raspberry Pi 3
----------------------------------------------------------------
Eric Anholt (1):
arm64: Add BCM2835 (Raspberry Pi 3) support to the defconfig
Florian Fainelli (1):
Merge tag 'bcm2835-defconfig-64-next-2016-09-22' into defconfig-arm64/next
arch/arm64/configs/defconfig | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
^ permalink raw reply
* [PATCH 0/3] Support userspace irqchip with arch timers
From: Alexander Graf @ 2016-09-30 19:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160930154335.GB7996@cbox>
On 30.09.16 17:43, Christoffer Dall wrote:
> On Fri, Sep 30, 2016 at 05:38:11PM +0200, Alexander Graf wrote:
>>
>>
>> On 30.09.16 16:54, Alexander Graf wrote:
>>>
>>>
>>> On 27.09.16 21:08, Christoffer Dall wrote:
>>>> Hi Alex,
>>>>
>>>> Marc and I have been looking at this during Linaro connect and have
>>>> slightly reworked your patch into this small series.
>>>>
>>>> It would be good if you could have a look at it and test it out.
>>>>
>>>> I've tested it with your QEMU, and it works for UP, but secondary CPUs
>>>> fail to come up, and it looks like the kernel never gets an IPI for
>>>> those CPUs from userspace. Any chance you're willing to take a look at
>>>> that?
>>>
>>> I still need to see whether I can come up with a prettier solution, but
>>> for now this works:
>>>
>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>
>> Eh, no, not in i386 code :). But the problem seems to be a missing
>> mpstate sync.
>>
> Yeah, that looked really dodgy. Have you tested it? :)
This time around tested with the correct command line parameters I hope
:). I'll send a pretty patch later.
diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index b4c8fe2..b549f00 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -173,6 +173,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
*/
kvm_async_interrupts_allowed = true;
+ /*
+ * PSCI wakes up secondary cores, so we always need to
+ * have vCPUs waiting in kernel space
+ */
+ kvm_halt_in_kernel_allowed = true;
+
cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);
type_register_static(&host_arm_cpu_type_info);
Alex
^ permalink raw reply related
* [RFC] arm64: Enforce observed order for spinlock and data
From: Mark Rutland @ 2016-09-30 19:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1475257257-23072-1-git-send-email-bdegraaf@codeaurora.org>
On Fri, Sep 30, 2016 at 01:40:57PM -0400, Brent DeGraaf wrote:
> Prior spinlock code solely used load-acquire and store-release
> semantics to ensure ordering of the spinlock lock and the area it
> protects. However, store-release semantics and ordinary stores do
> not protect against accesses to the protected area being observed
> prior to the access that locks the lock itself.
>
> While the load-acquire and store-release ordering is sufficient
> when the spinlock routines themselves are strictly used, other
> kernel code that references the lock values directly (e.g. lockrefs)
> could observe changes to the area protected by the spinlock prior
> to observance of the lock itself being in a locked state, despite
> the fact that the spinlock logic itself is correct.
If the spinlock logic is correct, why are we changing that, and not the lockref
code that you say has a problem?
What exactly goes wrong in the lockref code? Can you give a concrete example?
Why does the lockref code accesses lock-protected fields without taking the
lock first? Wouldn't concurrent modification be a problem regardless?
> + /*
> + * Yes: The store done on this cpu was the one that locked the lock.
> + * Store-release one-way barrier on LL/SC means that accesses coming
> + * after this could be reordered into the critical section of the
I assume you meant s/store-release/load-acquire/ here. This does not make sense
to me otherwise.
> + * load-acquire/store-release, where we did not own the lock. On LSE,
> + * even the one-way barrier of the store-release semantics is missing,
Likewise (for the LSE case description).
> + * so LSE needs an explicit barrier here as well. Without this, the
> + * changed contents of the area protected by the spinlock could be
> + * observed prior to the lock.
> + */
By whom? We generally expect that if data is protected by a lock, you take the
lock before accessing it. If you expect concurrent lockless readers, then
there's a requirement on the writer side to explicitly provide the ordering it
requires -- spinlocks are not expected to provide that.
So, why aren't those observers taking the lock?
What pattern of accesses are made by readers and writers such that there is a
problem?
What does this result in?
> +" dmb ish\n"
> +" b 3f\n"
> +"4:\n"
> /*
> * No: spin on the owner. Send a local event to avoid missing an
> * unlock before the exclusive load.
> @@ -116,7 +129,15 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
> " ldaxrh %w2, %4\n"
> " eor %w1, %w2, %w0, lsr #16\n"
> " cbnz %w1, 2b\n"
> - /* We got the lock. Critical section starts here. */
> + /*
> + * We got the lock and have observed the prior owner's store-release.
> + * In this case, the one-way barrier of the prior owner that we
> + * observed combined with the one-way barrier of our load-acquire is
> + * enough to ensure accesses to the protected area coming after this
> + * are not accessed until we own the lock. In this case, other
> + * observers will not see our changes prior to observing the lock
> + * itself. Critical locked section starts here.
> + */
Each of these comments ends up covers, and their repeated presence makes the
code harder to read. If there's a common problem, note it once at the top of
the file.
Thanks,
Mark.
^ permalink raw reply
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative
From: Ville Syrjälä @ 2016-09-30 20:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160930183348.75541582@bbrezillon>
On Fri, Sep 30, 2016 at 06:33:48PM +0200, Boris Brezillon wrote:
> On Fri, 30 Sep 2016 19:22:11 +0300
> Ville Syrj?l? <ville.syrjala@linux.intel.com> wrote:
>
> > On Fri, Sep 30, 2016 at 06:08:26PM +0200, Boris Brezillon wrote:
> > > On Fri, 30 Sep 2016 16:33:20 +0200
> > > Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > >
> > > > Our planes cannot be set at negative coordinates. Make sure we reject such
> > > > configuration.
> > > >
> > > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > ---
> > > > drivers/gpu/drm/sun4i/sun4i_layer.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > index f0035bf5efea..f5463c4c2cde 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > > @@ -29,6 +29,9 @@ struct sun4i_plane_desc {
> > > > static int sun4i_backend_layer_atomic_check(struct drm_plane *plane,
> > > > struct drm_plane_state *state)
> > > > {
> > > > + if ((state->crtc_x < 0) || (state->crtc_y < 0))
> > > > + return -EINVAL;
> > > > +
> > >
> > > Hm, I think it's a perfectly valid use case from the DRM framework and
> > > DRM user PoV: you may want to place your plane at a negative CRTC
> > > offset (which means part of the plane will be hidden).
> > >
> > > Maybe I'm wrong, but it seems you can support that by adapting the
> > > start address of your framebuffer pointer and the layer size.
> > >
> > > Have you tried doing something like that?
> >
> > You shouldn't even be looking at the user provided coordinates. Also
> > you can't return an error from the atomic update hook. The void return
> > value should have been a decent hint ;)
>
> Note that Maxime is not returning a value from the atomic update
> implementation (it's done in the atomic_check implem),
Ah, missed that. Anyway it's still the wrong thing to check since
negative coordinates are a perfectly valid thing to have.
> and I'm not
> checking crtc_x,y consistency at all (which is obviously wrong), I'm
> just blindly patching the values in sun4i_backend helpers.
>
> > The right fix would be
> > to move all the error handling into the atomic check hook, which
> > probably should just call the helper to clip the coordinates and
> > whatnot. Then the update hook can just use at the clipped
> > coordinates when programming the hw registers.
>
> That's probably the best approach indeed, but that means having our
> private sun4i_plane_state struct where we would store the patched
> crtc_{w,h,x,y} info.
Nope. The src/dst rects are now stored in drm_plane_state. Assuming my
patches went in already. I think they did.
>
> Anyway, before we do that, that's probably better to check if it really
> works on this HW (which is why I sent this informal patch).
Most hardware can't do negative coordinates natively. But when things
get clipped, the negative coordinates disappear. Then the only question
left is whether the resulting coordinates are fullscreen or not, and
whether the hardware can handle the "not" case.
>
> >
> > >
> > > --->8---
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > index 3ab560450a82..6b68804f3035 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > > @@ -110,15 +110,30 @@ int sun4i_backend_update_layer_coord(struct sun4i_backend *backend,
> > > {
> > > struct drm_plane_state *state = plane->state;
> > > struct drm_framebuffer *fb = state->fb;
> > > + int crtc_w, crtc_h, crtc_x, crtc_y;
> > >
> > > DRM_DEBUG_DRIVER("Updating layer %d\n", layer);
> > >
> > > + crtc_x = state->crtc_x;
> > > + crtc_y = state->crtc_y;
> > > + crtc_w = state->crtc_w;
> > > + crtc_h = state->crtc_h;
> > > +
> > > + if (crtc_x < 0) {
> > > + crtc_w += crtx_x;
> > > + crtc_x = 0;
> > > + }
> > > +
> > > + if (crtc_y < 0) {
> > > + crtc_h += crtx_y;
> > > + crtc_y = 0;
> > > + }
> > > +
> > > if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > > DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n",
> > > state->crtc_w, state->crtc_h);
> > > regmap_write(backend->regs, SUN4I_BACKEND_DISSIZE_REG,
> > > - SUN4I_BACKEND_DISSIZE(state->crtc_w,
> > > - state->crtc_h));
> > > + SUN4I_BACKEND_DISSIZE(crtc_w, crtc_h));
> > > }
> > >
> > > /* Set the line width */
> > > @@ -130,15 +145,13 @@ int sun4i_backend_update_layer_coord(struct sun4i_backend *backend,
> > > DRM_DEBUG_DRIVER("Layer size W: %u H: %u\n",
> > > state->crtc_w, state->crtc_h);
> > > regmap_write(backend->regs, SUN4I_BACKEND_LAYSIZE_REG(layer),
> > > - SUN4I_BACKEND_LAYSIZE(state->crtc_w,
> > > - state->crtc_h));
> > > + SUN4I_BACKEND_LAYSIZE(crtc_w, crtc_h));
> > >
> > > /* Set base coordinates */
> > > DRM_DEBUG_DRIVER("Layer coordinates X: %d Y: %d\n",
> > > state->crtc_x, state->crtc_y);
> > > regmap_write(backend->regs, SUN4I_BACKEND_LAYCOOR_REG(layer),
> > > - SUN4I_BACKEND_LAYCOOR(state->crtc_x,
> > > - state->crtc_y));
> > > + SUN4I_BACKEND_LAYCOOR(crtc_x, crtc_y));
> > >
> > > return 0;
> > > }
> > > @@ -198,6 +211,12 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend,
> > > paddr += (state->src_x >> 16) * bpp;
> > > paddr += (state->src_y >> 16) * fb->pitches[0];
> > >
> > > + if (state->crtc_x < 0)
> > > + paddr -= bpp * state->crtc_x;
> > > +
> > > + if (state->crtc_y < 0)
> > > + paddr -= fb->pitches[0] * state->crtc_y;
> > > +
> > > DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);
> > >
> > > /* Write the 32 lower bits of the address (in bits) */
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
--
Ville Syrj?l?
Intel OTC
^ permalink raw reply
* [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A SoC
From: Bhaskar U @ 2016-09-30 21:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <VI1PR0401MB2638683943A984F9C638F4A88DC10@VI1PR0401MB2638.eurprd04.prod.outlook.com>
>-----Original Message-----
>From: Stuart Yoder
>Sent: Friday, September 30, 2016 7:26 PM
>To: Bhaskar U <bhaskar.upadhaya@nxp.com>; Shawn Guo
><shawnguo@kernel.org>
>Cc: devicetree at vger.kernel.org; oss at buserror.net; Prabhakar Kushwaha
><prabhakar.kushwaha@nxp.com>; linux-devel at gforge.freescale.net; Pratiyush
>Srivastava <pratiyush.srivastava@nxp.com>; linux-arm-
>kernel at lists.infradead.org
>Subject: RE: [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A SoC
>
>> >> +&qspi {
>> >> + num-cs = <2>;
>> >> + bus-num = <0>;
>> >> + status = "disabled";
>> >
>> >Why is it being disabled?
>>
>> Ok, will change like below.
>> status = "okay";
>
>The comment was not "change this to okay". The question is why is this disabled?
>Can you explain why it was disabled? Should it have been disasbled? Is qspi
>working and tested on this board?
The intension of putting the status in disabled state is that the qspi functionality is not tested with the up-streamed kernel.
Yes qspi is working and tested on this board with 4.1 kernel version.
>
>>
>> >
>> >> + fsl,ddr-sampling-point = <4>;
>> >
>> >I do not find the bindings for this property, neither how driver supports it.
>>
>> Yes the QSPI DDR mode is not yet up-streamed, so I will remove this property
>as of now.
>>
>> >
>> >> +
>> >> + qflash0: s25fs512s at 0 {
>> >> + compatible = "spansion,m25p80";
>> >> + #address-cells = <1>;
>> >> + #size-cells = <1>;
>> >> + spi-max-frequency = <20000000>;
>> >> + m25p,fast-read;
>> >> + reg = <0>;
>> >> + };
>> >> +};
>> >> +
>> >> +&i2c0 {
>> >> + status = "okay";
>> >> +
>> >> + codec: sgtl5000 at a {
>> >> + #sound-dai-cells = <0>;
>> >> + compatible = "fsl,sgtl5000";
>> >> + reg = <0xa>;
>> >> + VDDA-supply = <®_1p8v>;
>> >> + VDDIO-supply = <®_1p8v>;
>> >> + clocks = <&sys_mclk>;
>> >> + };
>> >> +};
>> >> +
>> >> +&duart0 {
>> >> + status = "okay";
>> >> +};
>> >> +
>> >> +&esdhc0 {
>> >> + status = "disabled";
>> >
>> >We prefer to disable devices which have board level options by
>> >default in <soc>.dtsi, and enable them per availability in <board>.dts.
>>
>> Ok , will make the status as okay i.e. status = "okay";
>
>Again, the feedback was not "set this to okay". Why was esdhc0 set to "disabled"
>here in the first place? Was there a reason?
>
>The comment is that if there are certain boards where esdhc0 is not available,
>then fsl-ls1012a.dtsi should set this to "disabled" and board .dts files should
>override it.
esdhc0 is not there on this board so shall we mark the status in disabled state ?
>
>> >
>> >> +};
>> >> +
>> >> +&esdhc1 {
>> >> + status = "disabled";
>> >> +};
>> >> +
>> >> +&sai2 {
>> >> + status = "disabled";
>> >> +};
>
>Same comment for the above nodes. The fsl-ls1012a.dtsi should set them to
>disabled and any .dts file should override with "ok" if applicable.
esdhc1 is not there on the board, so shall we keep the status of esdhc1 in disabled state ?
sai2 is working and tested on this board, so shall we put the sai2 status as "okay" ?
Earlier when we kept sai2 status as disabled, by that time sai2 was not tested but now it is working fine.
>
>Stuart
^ permalink raw reply
* [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A SoC
From: Bhaskar U @ 2016-09-30 21:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <VI1PR0401MB2638C032B77C9313C17BBE898DC10@VI1PR0401MB2638.eurprd04.prod.outlook.com>
>-----Original Message-----
>From: Stuart Yoder
>Sent: Friday, September 30, 2016 7:28 PM
>To: Bhaskar U <bhaskar.upadhaya@nxp.com>; devicetree at vger.kernel.org;
>shawnguo at kernel.org
>Cc: oss at buserror.net; linux-arm-kernel at lists.infradead.org; linux-
>devel at gforge.freescale.net; Bhaskar U <bhaskar.upadhaya@nxp.com>;
>Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>; Pratiyush Srivastava
><pratiyush.srivastava@nxp.com>
>Subject: RE: [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A SoC
>
>> + dspi0: dspi at 2100000 {
>> + compatible = "fsl,ls1012a-dspi",
>> + "fsl,ls1043a-dspi",
>> + "fsl,ls1021a-v1.0-dspi";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <0x0 0x2100000 0x0 0x10000>;
>> + interrupts = <0 64 0x4>;
>> + clock-names = "dspi";
>> + clocks = <&clockgen 4 0>;
>> + spi-num-chipselects = <5>;
>> + big-endian;
>> + status = "enabled";
>
>"enabled" is not a valid status value.
>
So shall I put the status = "okay" ?
But there are other nodes like i2c below having status = "enabled", so do we need to edit them also ? If yes then what should be written in the status ?
i2c0: i2c at 2180000 {
compatible = "fsl,vf610-i2c";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x0 0x2180000 0x0 0x10000>;
interrupts = <0 56 0x4>;
clock-names = "i2c";
clocks = <&clockgen 4 0>;
status = "enabled";
};
i2c1: i2c at 2190000 {
compatible = "fsl,vf610-i2c";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x0 0x2190000 0x0 0x10000>;
interrupts = <0 57 0x4>;
clock-names = "i2c";
clocks = <&clockgen 4 0>;
status = "enabled";
};
>Stuart
^ permalink raw reply
* [linux-devel] [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A SoC
From: Leo Li @ 2016-09-30 21:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AM4PR0401MB2275E76A1EC44FE6DA19D4978CC10@AM4PR0401MB2275.eurprd04.prod.outlook.com>
> -----Original Message-----
> From: linux-devel-bounces at gforge.freescale.net [mailto:linux-devel-
> bounces at gforge.freescale.net] On Behalf Of Bhaskar U
> Sent: Friday, September 30, 2016 4:19 PM
> To: Stuart Yoder <stuart.yoder@nxp.com>; devicetree at vger.kernel.org;
> shawnguo at kernel.org
> Cc: oss at buserror.net; linux-devel at gforge.freescale.net; Pratiyush Srivastava
> <pratiyush.srivastava@nxp.com>; Prabhakar Kushwaha
> <prabhakar.kushwaha@nxp.com>; linux-arm-kernel at lists.infradead.org
> Subject: Re: [linux-devel] [PATCH v2 1/1] arm64: Add DTS support for FSL's
> LS1012A SoC
>
>
>
> >-----Original Message-----
> >From: Stuart Yoder
> >Sent: Friday, September 30, 2016 7:28 PM
> >To: Bhaskar U <bhaskar.upadhaya@nxp.com>; devicetree at vger.kernel.org;
> >shawnguo at kernel.org
> >Cc: oss at buserror.net; linux-arm-kernel at lists.infradead.org; linux-
> >devel at gforge.freescale.net; Bhaskar U <bhaskar.upadhaya@nxp.com>;
> >Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>; Pratiyush Srivastava
> ><pratiyush.srivastava@nxp.com>
> >Subject: RE: [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A
> >SoC
> >
> >> + dspi0: dspi at 2100000 {
> >> + compatible = "fsl,ls1012a-dspi",
> >> + "fsl,ls1043a-dspi",
> >> + "fsl,ls1021a-v1.0-dspi";
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + reg = <0x0 0x2100000 0x0 0x10000>;
> >> + interrupts = <0 64 0x4>;
> >> + clock-names = "dspi";
> >> + clocks = <&clockgen 4 0>;
> >> + spi-num-chipselects = <5>;
> >> + big-endian;
> >> + status = "enabled";
> >
> >"enabled" is not a valid status value.
> >
>
> So shall I put the status = "okay" ?
>
> But there are other nodes like i2c below having status = "enabled", so do we
> need to edit them also ? If yes then what should be written in the status ?
> i2c0: i2c at 2180000 {
> compatible = "fsl,vf610-i2c";
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0x0 0x2180000 0x0 0x10000>;
> interrupts = <0 56 0x4>;
> clock-names = "i2c";
> clocks = <&clockgen 4 0>;
> status = "enabled";
> };
>
> i2c1: i2c at 2190000 {
> compatible = "fsl,vf610-i2c";
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0x0 0x2190000 0x0 0x10000>;
> interrupts = <0 57 0x4>;
> clock-names = "i2c";
> clocks = <&clockgen 4 0>;
> status = "enabled";
> };
The default is "okay". You should just remove these status properties.
Regards,
Leo
^ permalink raw reply
* [linux-devel] [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A SoC
From: Shawn Guo @ 2016-09-30 21:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AM4PR0401MB22757E27E45F67FA5657AAAF8CC10@AM4PR0401MB2275.eurprd04.prod.outlook.com>
On Fri, Sep 30, 2016 at 11:38:29AM +0000, Bhaskar U wrote:
>
>
> >-----Original Message-----
> >From: Scott Wood
> >Sent: Tuesday, September 06, 2016 10:36 PM
> >To: Shawn Guo <shawnguo@kernel.org>; Stuart Yoder <stuart.yoder@nxp.com>;
> >Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>
> >Cc: devicetree at vger.kernel.org; Bhaskar U <bhaskar.upadhaya@nxp.com>; Scott
> >Wood <oss@buserror.net>; Prabhakar Kushwaha
> ><prabhakar.kushwaha@nxp.com>; linux-devel at gforge.freescale.net; Pratiyush
> >Srivastava <pratiyush.srivastava@nxp.com>; linux-arm-
> >kernel at lists.infradead.org
> >Subject: Re: [linux-devel] [PATCH v2 1/1] arm64: Add DTS support for FSL's
> >LS1012A SoC
> >
> >On 09/04/2016 08:47 PM, Shawn Guo wrote:
> >> On Tue, Aug 30, 2016 at 02:07:17PM +0000, Stuart Yoder wrote:
> >>>> On Mon, Aug 29, 2016 at 12:51:01PM -0500, Scott Wood wrote:
> >>>>> On Mon, 2016-08-29 at 17:52 +0800, Shawn Guo wrote:
> >>>>>> On Fri, Aug 26, 2016 at 03:57:21PM +0530, Bhaskar Upadhaya wrote:
> >>>>>>>
> >>>>>>> + clockgen: clocking at 1ee1000 {
> >>>>>>> + compatible = "fsl,ls1012a-clockgen";
> >>>>>> The compatible cannot be found in binding docs.
> >>>>>
> >>>>> From Documentation/devicetree/bindings/clock/qoriq-clock.txt:
> >>>>>
> >>>>> - compatible: Should contain a chip-specific clock block compatible
> >>>>> string and (if applicable) may contain a chassis-version clock
> >>>>> compatible string.
> >>>>>
> >>>>> Chip-specific strings are of the form "fsl,<chip>-clockgen", such as:
> >>>>> * "fsl,p2041-clockgen"
> >>>>> * "fsl,p3041-clockgen"
> >>>>> * "fsl,p4080-clockgen"
> >>>>> * "fsl,p5020-clockgen"
> >>>>> * "fsl,p5040-clockgen"
> >>>>> * "fsl,t4240-clockgen"
> >>>>> * "fsl,b4420-clockgen"
> >>>>> * "fsl,b4860-clockgen"
> >>>>> * "fsl,ls1021a-clockgen"
> >>>>> Chassis-version clock strings include:
> >>>>> * "fsl,qoriq-clockgen-1.0": for chassis 1.0 clocks
> >>>>> * "fsl,qoriq-clockgen-2.0": for chassis 2.0 clocks
> >>>>>
> >>>>> I really hope we don't have to update every single
> >>>>> fsl,<chip>-whatever binding every time a new chip comes out. There
> >>>>> are already other chips not listed, FWIW (e.g. t1040, t2080, ls1043a, and
> >ls2080a). That's why it says "such as".
> >>>>
> >>>> If I remember correctly, DT maintainers want every supported
> >>>> compatible string explicitly listed in bindings doc. And they even
> >>>> added a check into checkpatch.pl with commit bff5da433525
> >>>> ("checkpatch: add DT compatible string documentation checks").
> >>>
> >>> See Documentation/devicetree/bindings/submitting-patches.txt:
> >>>
> >>> 5) The wildcard "<chip>" may be used in compatible strings, as in
> >>> the following example:
> >>>
> >>> - compatible: Must contain '"nvidia,<chip>-pcie",
> >>> "nvidia,tegra20-pcie"' where <chip> is tegra30, tegra132, ...
> >>>
> >>> As in the above example, the known values of "<chip>" should be
> >>> documented if it is used.
> >>>
> >>> It _is_ allowed to use the <chip> wildcard, and so you will not find
> >>> all full compatible strings explicitly listed in bindings. However,
> >>> the chips themselves "should" be listed.
> >>
> >> + Rob and Mark
> >>
> >> Oops, I'm not aware of this DT document. In that case, the DT
> >> document and checkpatch is basically asking for conflicting thing.
> >> Rob, Mark, can you guys please clarify?
> >
> >Checkpatch is a useful tool but it can't get everything right all the time.
> >
>
> So what should we do, shall I add "compatible = "fsl,ls1012a-clockgen";" in Documentation/devicetree/bindings/clock/qoriq-clock.txt ?
I would encourage you to do that.
Shawn
^ permalink raw reply
* [linux-devel] [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A SoC
From: Leo Li @ 2016-09-30 21:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AM4PR0401MB2275F9FBAE954BD420AA67F98CC10@AM4PR0401MB2275.eurprd04.prod.outlook.com>
> -----Original Message-----
> From: linux-devel-bounces at gforge.freescale.net [mailto:linux-devel-
> bounces at gforge.freescale.net] On Behalf Of Bhaskar U
> Sent: Friday, September 30, 2016 4:13 PM
> To: Stuart Yoder <stuart.yoder@nxp.com>; Shawn Guo <shawnguo@kernel.org>
> Cc: devicetree at vger.kernel.org; Pratiyush Srivastava
> <pratiyush.srivastava@nxp.com>; oss at buserror.net; Prabhakar Kushwaha
> <prabhakar.kushwaha@nxp.com>; linux-devel at gforge.freescale.net; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [linux-devel] [PATCH v2 1/1] arm64: Add DTS support for FSL's
> LS1012A SoC
>
>
>
> >-----Original Message-----
> >From: Stuart Yoder
> >Sent: Friday, September 30, 2016 7:26 PM
> >To: Bhaskar U <bhaskar.upadhaya@nxp.com>; Shawn Guo
> ><shawnguo@kernel.org>
> >Cc: devicetree at vger.kernel.org; oss at buserror.net; Prabhakar Kushwaha
> ><prabhakar.kushwaha@nxp.com>; linux-devel at gforge.freescale.net;
> >Pratiyush Srivastava <pratiyush.srivastava@nxp.com>; linux-arm-
> >kernel at lists.infradead.org
> >Subject: RE: [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A
> >SoC
> >
> >> >> +&qspi {
> >> >> + num-cs = <2>;
> >> >> + bus-num = <0>;
> >> >> + status = "disabled";
> >> >
> >> >Why is it being disabled?
> >>
> >> Ok, will change like below.
> >> status = "okay";
> >
> >The comment was not "change this to okay". The question is why is this
> disabled?
> >Can you explain why it was disabled? Should it have been disasbled? Is qspi
> >working and tested on this board?
>
> The intension of putting the status in disabled state is that the qspi functionality
> is not tested with the up-streamed kernel.
> Yes qspi is working and tested on this board with 4.1 kernel version.
> >
> >>
> >> >
> >> >> + fsl,ddr-sampling-point = <4>;
> >> >
> >> >I do not find the bindings for this property, neither how driver supports it.
> >>
> >> Yes the QSPI DDR mode is not yet up-streamed, so I will remove this
> >> property
> >as of now.
> >>
> >> >
> >> >> +
> >> >> + qflash0: s25fs512s at 0 {
> >> >> + compatible = "spansion,m25p80";
> >> >> + #address-cells = <1>;
> >> >> + #size-cells = <1>;
> >> >> + spi-max-frequency = <20000000>;
> >> >> + m25p,fast-read;
> >> >> + reg = <0>;
> >> >> + };
> >> >> +};
> >> >> +
> >> >> +&i2c0 {
> >> >> + status = "okay";
> >> >> +
> >> >> + codec: sgtl5000 at a {
> >> >> + #sound-dai-cells = <0>;
> >> >> + compatible = "fsl,sgtl5000";
> >> >> + reg = <0xa>;
> >> >> + VDDA-supply = <®_1p8v>;
> >> >> + VDDIO-supply = <®_1p8v>;
> >> >> + clocks = <&sys_mclk>;
> >> >> + };
> >> >> +};
> >> >> +
> >> >> +&duart0 {
> >> >> + status = "okay";
> >> >> +};
> >> >> +
> >> >> +&esdhc0 {
> >> >> + status = "disabled";
> >> >
> >> >We prefer to disable devices which have board level options by
> >> >default in <soc>.dtsi, and enable them per availability in <board>.dts.
> >>
> >> Ok , will make the status as okay i.e. status = "okay";
> >
> >Again, the feedback was not "set this to okay". Why was esdhc0 set to
> "disabled"
> >here in the first place? Was there a reason?
> >
> >The comment is that if there are certain boards where esdhc0 is not
> >available, then fsl-ls1012a.dtsi should set this to "disabled" and
> >board .dts files should override it.
>
> esdhc0 is not there on this board so shall we mark the status in disabled state ?
>
> >
> >> >
> >> >> +};
> >> >> +
> >> >> +&esdhc1 {
> >> >> + status = "disabled";
> >> >> +};
> >> >> +
> >> >> +&sai2 {
> >> >> + status = "disabled";
> >> >> +};
> >
> >Same comment for the above nodes. The fsl-ls1012a.dtsi should set them
> >to disabled and any .dts file should override with "ok" if applicable.
>
> esdhc1 is not there on the board, so shall we keep the status of esdhc1 in
> disabled state ?
> sai2 is working and tested on this board, so shall we put the sai2 status as
> "okay" ?
> Earlier when we kept sai2 status as disabled, by that time sai2 was not tested but
> now it is working fine.
The convention is to set the status of these optional nodes to "disabled" in the SoC dtsi. And enable the nodes needed in board dts by overwriting the status to "okay". It could be confusing if you use both "disabled" and "okay" in the board dts file.
Regards,
Leo
^ permalink raw reply
* [PATCH v2 1/1] arm64: Add DTS support for FSL's LS1012A SoC
From: Shawn Guo @ 2016-09-30 21:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <AM4PR0401MB2275F9FBAE954BD420AA67F98CC10@AM4PR0401MB2275.eurprd04.prod.outlook.com>
On Fri, Sep 30, 2016 at 09:13:11PM +0000, Bhaskar U wrote:
> >> >> +&qspi {
> >> >> + num-cs = <2>;
> >> >> + bus-num = <0>;
> >> >> + status = "disabled";
> >> >
> >> >Why is it being disabled?
> >>
> >> Ok, will change like below.
> >> status = "okay";
> >
> >The comment was not "change this to okay". The question is why is this disabled?
> >Can you explain why it was disabled? Should it have been disasbled? Is qspi
> >working and tested on this board?
>
> The intension of putting the status in disabled state is that the qspi functionality is not tested with the up-streamed kernel.
> Yes qspi is working and tested on this board with 4.1 kernel version.
Please only add those board level device node after it's been tested on
the board.
> >> >> +&esdhc0 {
> >> >> + status = "disabled";
> >> >
> >> >We prefer to disable devices which have board level options by
> >> >default in <soc>.dtsi, and enable them per availability in <board>.dts.
> >>
> >> Ok , will make the status as okay i.e. status = "okay";
> >
> >Again, the feedback was not "set this to okay". Why was esdhc0 set to "disabled"
> >here in the first place? Was there a reason?
> >
> >The comment is that if there are certain boards where esdhc0 is not available,
> >then fsl-ls1012a.dtsi should set this to "disabled" and board .dts files should
> >override it.
>
> esdhc0 is not there on this board so shall we mark the status in disabled state ?
For device that has pin-out on board, we should mark it disabled in
<soc>.dtsi by default, and enable it in <board>.dts.
> >> >> +&esdhc1 {
> >> >> + status = "disabled";
> >> >> +};
> >> >> +
> >> >> +&sai2 {
> >> >> + status = "disabled";
> >> >> +};
> >
> >Same comment for the above nodes. The fsl-ls1012a.dtsi should set them to
> >disabled and any .dts file should override with "ok" if applicable.
>
> esdhc1 is not there on the board, so shall we keep the status of esdhc1 in disabled state ?
With esdhc1 being marked "disabled" in <soc>.dtsi by default, we need
to do nothing for board that doesn't have the device.
> sai2 is working and tested on this board, so shall we put the sai2 status as "okay" ?
> Earlier when we kept sai2 status as disabled, by that time sai2 was not tested but now it is working fine.
With sai2 being marked "disabled" in <soc>.dtsi by default, you add
board level node with "okay" after you test the device working.
Shawn
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox