Subject: Re: [PATCH] MIPS: fix boot with DT passed via UHI
To: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>,
        Andrea Merello <andrea.merello@gmail.com>, ralf@linux-mips.org,
        linux-mips@linux-mips.org
Cc: linux-kernel@vger.kernel.org, Jonas Gorski <jogo@openwrt.org>
References: <1496776596-5045-1-git-send-email-andrea.merello@gmail.com>
 <fd8ad5f4-ab71-e8fc-a7ee-5177877cfb74@gmail.com>
From: David Daney <ddaney@caviumnetworks.com>
Message-ID: <2d887f2f-b83b-3540-37dd-182aeed1fc0e@caviumnetworks.com>
Date: Wed, 7 Jun 2017 15:47:02 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.0
MIME-Version: 1.0
In-Reply-To: <fd8ad5f4-ab71-e8fc-a7ee-5177877cfb74@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2017 22:47:14.8456 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR07MB3493
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3932
Lines: 93

On 06/07/2017 06:16 AM, Daniel Schwierzeck wrote:
> 
> 
> Am 06.06.2017 um 21:16 schrieb Andrea Merello:
>> commit 15f37e158892 ("MIPS: store the appended dtb address in a variable")
>> seems to have introduced code that relies on delay slots after branch,
>> however it seems that, since no directive ".set noreorder" is present, the
>> AS already fills delay slots with NOPs.
>>
>> This caused failure in assigning proper DT blob address to fw_passed_dtb
>> variable, causing failure when booting passing DT via UHI; this has been
>> seen on a Lantiq VR9 SoC (Fritzbox 3370) and u-boot as bootloader.
>>
>> [    0.000000] Linux version 4.12.0-fritz+ (andrea@horizon) (gcc version 4.9.0 (GCC) ) #29 SMP Tue Jun 6 20:49:59 CEST 2017
>> [    0.000000] SoC: xRX200 rev 1.2
>> [    0.000000] bootconsole [early0] enabled
>> [    0.000000] CPU0 revision is: 00019556 (MIPS 34Kc)
>> [    0.000000] Determined physical RAM map:
>> [    0.000000]  memory: 00696000 @ 00002000 (usable)
>> [    0.000000]  memory: 00038000 @ 00698000 (usable after init)
>> [    0.000000] Wasting 64 bytes for tracking 2 unused pages
>> [    0.000000] Kernel panic - not syncing: No memory area to place a bootmap bitmap
>> [    0.000000] Rebooting in 1 seconds..
>> [    0.000000] Reboot failed -- System halted
>>
>> This patch moves the instruction meant to be placed in the delay slot
>> before the preceding BEQ instruction, while the delay slot will be
>> filled with a NOP by the AS.
>>
>> After this patch the kernel fetches the DR correctly
>>
>> [    0.000000] Linux version 4.12.0-fritz+ (andrea@horizon) (gcc version 4.9.0 (GCC) ) #30 SMP
>> Tue Jun 6 20:52:40 CEST 2017
>> [    0.000000] SoC: xRX200 rev 1.2
>> [    0.000000] bootconsole [early0] enabled
>> [    0.000000] CPU0 revision is: 00019556 (MIPS 34Kc)
>> [    0.000000] MIPS: machine is FRITZ3370 - Fritz!Box WLAN 3370
>> [    0.000000] Determined physical RAM map:
>> [    0.000000]  memory: 08000000 @ 00000000 (usable)
>> [    0.000000] Detected 1 available secondary CPU(s)
>> [    0.000000] Primary instruction cache 32kB, VIPT, 4-way, linesize 32 bytes.
>> [    0.000000] Primary data cache 32kB, 4-way, VIPT, cache aliases, linesize 32 bytes
>> [    0.000000] Zone ranges:
>> [    0.000000]   Normal   [mem 0x0000000000000000-0x0000000007ffffff]
>> [    0.000000] Movable zone start for each node
>> [    0.000000] Early memory node ranges
>> [    0.000000]   node   0: [mem 0x0000000000000000-0x0000000007ffffff]
>> [    0.000000] Initmem setup node 0 [mem 0x0000000000000000-0x0000000007ffffff]
>> [    0.000000] percpu: Embedded 15 pages/cpu @8110c000 s30176 r8192 d23072 u61440
>> [    0.000000] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 32512
>> [    0.000000] Kernel command line: rootwait root=/dev/sda1 console=ttyLTQ0
>> ...
>>
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Jonas Gorski <jogo@openwrt.org>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>>
>> diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S
>> index cf05220..d1bb506 100644
>> --- a/arch/mips/kernel/head.S
>> +++ b/arch/mips/kernel/head.S
>> @@ -106,8 +106,8 @@ NESTED(kernel_entry, 16, sp)			# kernel entry point
>>   	beq		t0, t1, dtb_found
>>   #endif
>>   	li		t1, -2
>> -	beq		a0, t1, dtb_found
>>   	move		t2, a1
>> +	beq		a0, t1, dtb_found
>>   
>>   	li		t2, 0
>>   dtb_found:
>>
> 
> The fix looks correct. Without ".set noreorder" one should not

s/should/must/

> manually
> put instructions in the delay slot. This should be left to the AS as an
> option for optimization.

By definition, it is what the assembler does.  When ".set noreorder" is 
not in effect, the source code *must* be written as if branch delay 
slots do not exist.  There is no option here.

> 
> Acked-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>


Acked-by: David Daney <david.daney@cavium.com>

>