linux-mips
[Top] [All Lists]

Re: [PATCH V10 12/13] MIPS: Loongson 3: Add CPU hotplug support

To: Huacai Chen <chenhc@lemote.com>
Subject: Re: [PATCH V10 12/13] MIPS: Loongson 3: Add CPU hotplug support
From: Ralf Baechle <ralf@linux-mips.org>
Date: Fri, 28 Jun 2013 09:05:53 +0200
Cc: John Crispin <john@phrozen.org>, linux-mips@linux-mips.org, Fuxin Zhang <zhangfx@lemote.com>, Zhangjin Wu <wuzhangjin@gmail.com>, Hongliang Tao <taohl@lemote.com>, Hua Yan <yanh@lemote.com>
In-reply-to: <1366030028-5084-13-git-send-email-chenhc@lemote.com>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <1366030028-5084-1-git-send-email-chenhc@lemote.com> <1366030028-5084-13-git-send-email-chenhc@lemote.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Apr 15, 2013 at 08:47:07PM +0800, Huacai Chen wrote:

> +/* To shutdown a core in Loongson 3, the target core should go to CKSEG1 and
> + * flush all L1 entries at first. Then, another core (usually Core 0) can
> + * safely disable the clock of the target core. loongson3_play_dead() is
> + * called via CKSEG1 (uncached and unmmaped) */
> +void loongson3_play_dead(int *state_addr)
> +{
> +     __asm__ __volatile__(
> +             "      .set push                         \n"
> +             "      .set noreorder                    \n"
> +             "      li $t0, 0x80000000                \n" /* KSEG0 */
> +             "      li $t1, 512                       \n" /* num of L1 
> entries */
> +             "flush_loop:                             \n" /* flush L1 */

Please don't use normale in inline assembler.  This might result in build
errors.  it's horrible to read but number local labels like:

1:      subu    $t0, $t0, 1
        bnez    $t0, 1b

are safe.  "1b" means the label 1 backwards from the users but you can
also use 1f to branch forward:

        bnez    $t0, 1f
        ...
1:      jr      $ra

> +             "      cache 0, 0($t0)                   \n" /* ICache */
> +             "      cache 0, 1($t0)                   \n"
> +             "      cache 0, 2($t0)                   \n"
> +             "      cache 0, 3($t0)                   \n"
> +             "      cache 1, 0($t0)                   \n" /* DCache */
> +             "      cache 1, 1($t0)                   \n"
> +             "      cache 1, 2($t0)                   \n"
> +             "      cache 1, 3($t0)                   \n"
> +             "      addiu $t0, $t0, 0x20              \n"
> +             "      bnez  $t1, flush_loop             \n"
> +             "      addiu $t1, $t1, -1                \n"
> +             "      li    $t0, 0x7                    \n" /* *state_addr = 
> CPU_DEAD; */
> +             "      sw    $t0, 0($a0)                 \n"
> +             "      sync                              \n"
> +             "      cache 21, 0($a0)                  \n" /* flush entry of 
> *state_addr */
> +             "      .set pop                          \n");
> +
> +     __asm__ __volatile__(
> +             "      .set push                         \n"
> +             "      .set noreorder                    \n"
> +             "      .set mips64                       \n"
> +             "      mfc0  $t2, $15, 1                 \n"
> +             "      andi  $t2, 0x3ff                  \n"
> +             "      .set mips3                        \n"
> +             "      dli   $t0, 0x900000003ff01000     \n"

I'm wondering about the .set mips3 here.  It's redundant following a
.set mips64.  But I'm wondering, are you doing this on a 32 bit kernel?
On 32 bit this is only safe as long as interrupts are off.  If an interrupt
is taken registers will be corrupted.

If yes, this is only safe on as long as interrupts are disabled.

> +             "      andi  $t3, $t2, 0x3               \n"
> +             "      sll   $t3, 8                      \n"  /* get cpu id */
> +             "      or    $t0, $t0, $t3               \n"
> +             "      andi  $t1, $t2, 0xc               \n"
> +             "      dsll  $t1, 42                     \n"  /* get node id */
> +             "      or    $t0, $t0, $t1               \n"
> +             "wait_for_init:                          \n"
> +             "      li    $a0, 0x100                  \n"
> +             "idle_loop:                              \n"
> +             "      bnez  $a0, idle_loop              \n"
> +             "      addiu $a0, -1                     \n"
> +             "      lw    $v0, 0x20($t0)              \n"  /* get PC via 
> mailbox */
> +             "      nop                               \n"

Useless nop - only R2000/R3000 has load delay slots.

> +             "      beqz  $v0, wait_for_init          \n"
> +             "      nop                               \n"

Ditto.

> +             "      ld    $sp, 0x28($t0)              \n"  /* get SP via 
> mailbox */
> +             "      nop                               \n"

Ditto.

> +             "      ld    $gp, 0x30($t0)              \n"  /* get GP via 
> mailbox */
> +             "      nop                               \n"

Ditto.

> +             "      ld    $a1, 0x38($t0)              \n"
> +             "      nop                               \n"

Ditto.

> +             "      jr  $v0                           \n"  /* jump to 
> initial PC */
> +             "      nop                               \n"
> +             "      .set pop                          \n");
> +}

Much of this inline assembler code can be replaced with C.  Even direct
register manipulation is possible in C:

        register unsigned long sp __asm__("$29");

        sp = ptr->mailbox;

But you probably want to leave that part in assembler, so you get something
like:

        __asm__ __volatile__(
        "       move    $sp, %[sp]                      \n"
        "       move    $gp, %[gp]                      \n"
        "       move    $a1, %[a1]                      \n"
        "       jr      %[dst]                          \n"
        : /* No outputs */
        : [sp]  "r" (mailbox->sp),
          [gp]  "r" (mailbox->gp),
          [a1]  "r" (mailbox->a1),
          [dst] "r" (mailbox->whatever));

        unreachable();

No more weird hardcoded offsets, safe, easier to read.

  Ralf

<Prev in Thread] Current Thread [Next in Thread>