linux-mips
[Top] [All Lists]

Re: [PATCH 1/12] mips: PMC MSP71xx core platform

To: Marc St-Jean <stjeanma@pmc-sierra.com>
Subject: Re: [PATCH 1/12] mips: PMC MSP71xx core platform
From: Ralf Baechle <ralf@linux-mips.org>
Date: Thu, 21 Jun 2007 17:55:01 +0100
Cc: linux-mips@linux-mips.org
In-reply-to: <200706142154.l5ELslhw021385@pasqua.pmc-sierra.bc.ca>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <200706142154.l5ELslhw021385@pasqua.pmc-sierra.bc.ca>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.14 (2007-02-12)
On Thu, Jun 14, 2007 at 03:54:47PM -0600, Marc St-Jean wrote:

General comment all the first ~ 2900 of your patch is that it's rather
heavy on #ifdef.  #ifdef is a nasty construct that has a tendency to hard
to read code which in trun results in bugs.  Or a test compile fails to
find real issues in the code because it's hidden in a dead #if construct.

> diff --git a/include/asm-mips/pmc-sierra/msp71xx/msp_regops.h 
> b/include/asm-mips/pmc-sierra/msp71xx/msp_regops.h
> new file mode 100644
> index 0000000..60a5a38
> --- /dev/null
> +++ b/include/asm-mips/pmc-sierra/msp71xx/msp_regops.h
> @@ -0,0 +1,236 @@
> +/*
> + * SMP/VPE-safe functions to access "registers" (see note).
> + *
> + * NOTES:
> +* - These macros use ll/sc instructions, so it is your responsibility to
> + * ensure these are available on your platform before including this file.
> + * - The MIPS32 spec states that ll/sc results are undefined for uncached
> + * accesses. This means they can't be used on HW registers accessed
> + * through kseg1. Code which requires these macros for this purpose must
> + * front-end the registers with cached memory "registers" and have a single
> + * thread update the actual HW registers.

You basically betting on undefined behaviour of the architecture.  I did
indeed verify this specific case with a processor design guy and the answer
was a "should be ok".  That is I heared people speak in a more convincing
tone already ;-)

A SC with an uncached address on some other MIPS processors will always fail.
So this isn't just a theoretical footnote in a manual.

The way things are implemented LL/SC I am certain that uncached LL/SC will
fail on any MIPS multiprocessor system.  Fortunately while SMTC pretends
to be multiprocessor it's really just a single core, which saves your day.

> + * - A maximum of 2k of code can be inserted between ll and sc. Every
> + * memory accesses between the instructions will increase the chance of
> + * sc failing and having to loop.

Any memory access between LL/SC makes the LL/SC sequence invalid that is
it will have undefined effects.

> + * - When using custom_read_reg32/custom_write_reg32 only perform the
> + * necessary logical operations on the register value in between these
> + * two calls. All other logic should be performed before the first call.
> +  * - There is a bug on the R10000 chips which has a workaround. If you
> + * are affected by this bug, make sure to define the symbol 'R10000_LLSC_WAR'
> + * to be non-zero.  If you are using this header from within linux, you may
> + * include <asm/war.h> before including this file to have this defined
> + * appropriately for you.

> +#ifndef __ASM_REGOPS_H__
> +#define __ASM_REGOPS_H__
> +
> +#include <linux/types.h>
> +
> +#include <asm/war.h>
> +
> +#ifndef R10000_LLSC_WAR
> +#define R10000_LLSC_WAR 0
> +#endif

This symbol is supposed to be defined by <asm/war.h> only.  Anyway, this
#ifndef will never be true because you already include <asm/war.h>, so
this is dead code.

> +#if R10000_LLSC_WAR == 1
> +#define __beqz       "beqzl  "
> +#else
> +#define __beqz       "beqz   "
> +#endif
> +
> +#ifndef _LINUX_TYPES_H
> +typedef unsigned int u32;
> +#endif

Redefining a stanard Linux type is a no-no as is relying on include
wrapper symbols like _LINUX_TYPES_H.  Anyway, this #ifndef will never
be true because you already include <linux/types.h>, so this is dead code.

> +static inline u32 read_reg32(volatile u32 *const addr,
> +                             u32 const mask)
> +{
> +     u32 temp;
> +
> +     __asm__ __volatile__(
> +     "       .set    push                            \n"
> +     "       .set    noreorder                       \n"
> +     "       lw      %0, %1          # read          \n"
> +     "       and     %0, %2          # mask          \n"
> +     "       .set    pop                             \n"
> +     : "=&r" (temp)
> +     : "m" (*addr), "ir" (mask));
> +
> +     return temp;
> +}

No need for inline assembler here; plain C can achieve the same.  Or just
use a standard Linux function such as readl() or ioread32() or similar.

> +/*
> + * For special strange cases only:
> + *
> + * If you need custom processing within a ll/sc loop, use the following 
> macros
> + * VERY CAREFULLY:
> + *
> + *   u32 tmp;                                <-- Define a variable to hold 
> the data
> + *
> + *   custom_read_reg32(address, tmp);        <-- Reads the address and put 
> the value
> + *                                           in the 'tmp' variable given
> + *
> + *   From here on out, you are (basicly) atomic, so don't do anything too
> + *   fancy!
> + *   Also, this code may loop if the end of this block fails to write
> + *   everything back safely due do the other CPU, so do NOT do anything
> + *   with side-effects!
> + *
> + *   custom_write_reg32(address, tmp);       <-- Writes back 'tmp' safely.
> + */
> +#define custom_read_reg32(address, tmp)                              \
> +     __asm__ __volatile__(                                   \
> +     "       .set    push                            \n"     \
> +     "       .set    mips3                           \n"     \
> +     "1:     ll      %0, %1  #custom_read_reg32      \n"     \
> +     "       .set    pop                             \n"     \
> +     : "=r" (tmp), "=m" (*address)                           \
> +     : "m" (*address))
> +
> +#define custom_write_reg32(address, tmp)                     \
> +     __asm__ __volatile__(                                   \
> +     "       .set    push                            \n"     \
> +     "       .set    mips3                           \n"     \
> +     "       sc      %0, %1  #custom_write_reg32     \n"     \
> +     "       "__beqz"%0, 1b                          \n"     \
> +     "       nop                                     \n"     \
> +     "       .set    pop                             \n"     \
> +     : "=&r" (tmp), "=m" (*address)                          \
> +     : "0" (tmp), "m" (*address))

These two are *really* fragile stuff.  Modern gcc rearranges code in
amazing ways, so you might end up with other loads or stores being moved
into the ll/sc sequence or the 1: label of another inline assembler
construct being taken as the destination of the branch.  So I would
suggest to safely store the two function in a nice yellow barrel ;-)

General suggestion, you can make about every access atomic if you do
something like

#include <linux/modules.h>
#include <linux/spinlocks.h>

DEFINE_SPINLOCK(register_lock);
EXPORT_SYMBOL(register_lock);

static inline void set_value_reg32(u32 *const addr,
                                       u32 const mask,
                                       u32 const value)
{
        unsigned long flags;
        u32 bits;

        spinlock_irqsave(&register_lock, flags);
        bits = readl(addr);
        bits &= mask;
        bits |= value;
        writel(bits, addr);
}

Maybe slower but definately more portable and not waiting before some
CPU designer screws your code by accident :-)

  Ralf

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