linux-mips
[Top] [All Lists]

Re: [PATCH 06/36] Add Cavium OCTEON processor CSR definitions

To: Ralf Baechle <ralf@linux-mips.org>
Subject: Re: [PATCH 06/36] Add Cavium OCTEON processor CSR definitions
From: David Daney <ddaney@caviumnetworks.com>
Date: Thu, 30 Oct 2008 11:21:44 -0700
Cc: Christoph Hellwig <hch@lst.de>, linux-mips@linux-mips.org, Tomaso Paoletti <tpaoletti@caviumnetworks.com>
In-reply-to: <20081030111354.GF26256@linux-mips.org>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <490655B6.4030406@caviumnetworks.com> <1225152181-3221-1-git-send-email-ddaney@caviumnetworks.com> <1225152181-3221-2-git-send-email-ddaney@caviumnetworks.com> <1225152181-3221-3-git-send-email-ddaney@caviumnetworks.com> <1225152181-3221-4-git-send-email-ddaney@caviumnetworks.com> <1225152181-3221-5-git-send-email-ddaney@caviumnetworks.com> <1225152181-3221-6-git-send-email-ddaney@caviumnetworks.com> <20081029184552.GB32500@lst.de> <4908B717.3010603@caviumnetworks.com> <20081030111354.GF26256@linux-mips.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Thunderbird 2.0.0.16 (X11/20080723)
Ralf Baechle wrote:

struct foo {
        unsigned int    x:1;
        unsigned int    y:4;
};

void bar(volatile struct foo *p)
{
        p->x = 1;
        p->y++;
}

which gcc 4.3 for a MIPS32R2 target will compile into:

        lw      $3, 0($4)
        li      $2, 1
        ins     $3, $2, 31, 1
        sw      $3, 0($4)
        lw      $2, 0($4)
        lw      $3, 0($4)
        ext     $2, $2, 27, 4
        addiu   $2, $2, 1
        ins     $3, $2, 27, 4
        sw      $3, 0($4)
        j       $31
        nop

Imagine struct foo was describing a hardware register so the pointer to it
was marked volatile.  A human coder wouldn't have done multiple loads /
stores.  Worse, if you actually want to change multiple fields in a
register atomically then with bitfields you have _no_ possibility to enforce
that.


This is bogus as you use a false premise. For your test case the compiler is generating exactly the accesses you are requesting by declaring the thing as volatile.

The Linux programming programming model relies on accessor functions like
readl, ioread32 etc.  Those take addresses as arguments - but bitfields
don't have addresses in C ...


We too use accessor functions, as they are required on certain classes of registers to obtain correct semantics.

The real question is if manipulating the values after they are obtained with the accessor is done correctly. My assertion is that although it may not be the One True Kernel Way, our structure bitfield definitions result in correct semantics and have better compile time error checking than can be obtained when coding a bunch of explicit masks and shifts.

That is not to say that I am adverse to eliminating the structure bitfield definitions, but we should not use incorrect register access semantics as the reason, as it is not a valid argument.

David Daney

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