From: Arnd Bergmann <arnd@arndb.de>
To: "Mike Frysinger" <vapier.adi@gmail.com>
Cc: "Luke Yang" <luke.adi@gmail.com>,
linux-kernel@vger.kernel.org, "Andrew Morton" <akpm@osdl.org>
Subject: Re: [PATCH 1/4] Blackfin: arch patch for 2.6.18
Date: Sat, 23 Sep 2006 15:07:54 +0200 [thread overview]
Message-ID: <200609231507.55198.arnd@arndb.de> (raw)
In-Reply-To: <8bd0f97a0609230415v6a31a784kf6a381f274cf7ef6@mail.gmail.com>
On Saturday 23 September 2006 13:15, Mike Frysinger wrote:
> On 9/23/06, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Saturday 23 September 2006 08:50, Mike Frysinger wrote:
> > > On 9/22/06, Arnd Bergmann <arnd@arndb.de> wrote:
> > > > What's the point, are you getting paid by lines of code? Just use
> > > > the registers directly!
> > >
> > > in our last submission we were doing exactly that ... and we were told
> > > to switch to a function style method of reading/writing memory mapped
> > > registers
> >
> > It's hard to imagine that what you have here was intended by the comment
> > then. Do you have an archive link about that discussion?
>
> no as i was not around for said discussion. but it should be in the
> threads covering the submission of blackfin for 2.6.13 ...
Ok, I found http://marc.theaimsgroup.com/?l=linux-kernel&m=114298753207549&w=2
from akpm, and I fear you heavily misunderstood him.
Your original patch had code like
/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */
#define SICA_SWRST 0xFFC00100·····/* Software Reset register */
#define SICA_SYSCR 0xFFC00104·····/* System Reset Configuration register */
#define SICA_RVECT 0xFFC00108·····/* SIC Reset Vector Address Register */
#define SICA_IMASK 0xFFC0010C·····/* SIC Interrupt Mask register 0 - hack to fix old tests */
#define SICA_IMASK0 0xFFC0010C·····/* SIC Interrupt Mask register 0 */
#define SICA_IMASK1 0xFFC00110·····/* SIC Interrupt Mask register 1 */
#define SICA_IAR0 0xFFC00124·····/* SIC Interrupt Assignment Register 0 */
#define SICA_IAR1 0xFFC00128·····/* SIC Interrupt Assignment Register 1 */
#define SICA_IAR2 0xFFC0012C·····/* SIC Interrupt Assignment Register 2 */
#define SICA_IAR3 0xFFC00130·····/* SIC Interrupt Assignment Register 3 */
#define SICA_IAR4 0xFFC00134·····/* SIC Interrupt Assignment Register 4 */
#define SICA_IAR5 0xFFC00138·····/* SIC Interrupt Assignment Register 5 */
#define SICA_IAR6 0xFFC0013C·····/* SIC Interrupt Assignment Register 6 */
#define SICA_IAR7 0xFFC00140·····/* SIC Interrupt Assignment Register 7 */
#define SICA_ISR0 0xFFC00114·····/* SIC Interrupt Status register 0 */
#define SICA_ISR1 0xFFC00118·····/* SIC Interrupt Status register 1 */
#define SICA_IWR0 0xFFC0011C·····/* SIC Interrupt Wakeup-Enable register 0 */
#define SICA_IWR1 0xFFC00120·····/* SIC Interrupt Wakeup-Enable register 1 */
...
#define pSICA_SWRST (volatile unsigned short *)SICA_SWRST
#define pSICA_SYSCR (volatile unsigned short *)SICA_SYSCR
#define pSICA_RVECT (volatile unsigned short *)SICA_RVECT
#define pSICA_IMASK (volatile unsigned long *)SICA_IMASK
#define pSICA_IMASK0 (volatile unsigned long *)SICA_IMASK0
#define pSICA_IMASK1 (volatile unsigned long *)SICA_IMASK1
#define pSICA_IAR0 ((volatile unsigned long *)SICA_IAR0)
#define pSICA_IAR1 (volatile unsigned long *)SICA_IAR1
#define pSICA_IAR2 (volatile unsigned long *)SICA_IAR2
#define pSICA_IAR3 (volatile unsigned long *)SICA_IAR3
#define pSICA_IAR4 (volatile unsigned long *)SICA_IAR4
#define pSICA_IAR5 (volatile unsigned long *)SICA_IAR5
#define pSICA_IAR6 (volatile unsigned long *)SICA_IAR6
#define pSICA_IAR7 (volatile unsigned long *)SICA_IAR7
#define pSICA_ISR0 (volatile unsigned long *)SICA_ISR0
#define pSICA_ISR1 (volatile unsigned long *)SICA_ISR1
#define pSICA_IWR0 (volatile unsigned long *)SICA_IWR0
#define pSICA_IWR1 (volatile unsigned long *)SICA_IWR1
static void bf561_internal_mask_irq(unsigned int irq)
{
unsigned long irq_mask;
if ((irq - (IRQ_CORETMR + 1)) < 32) {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
*pSICA_IMASK0 &= ~irq_mask;
} else {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
*pSICA_IMASK1 &= ~irq_mask;
}
}
which Andrew objected to, on multiple grounds. You now converted this to
/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */
#define SICA_SWRST 0xFFC00100·····/* Software Reset register */
#define SICA_SYSCR 0xFFC00104·····/* System Reset Configuration register */
#define SICA_RVECT 0xFFC00108·····/* SIC Reset Vector Address Register */
#define SICA_IMASK 0xFFC0010C·····/* SIC Interrupt Mask register 0 - hack to fix old tests */
#define SICA_IMASK0 0xFFC0010C·····/* SIC Interrupt Mask register 0 */
#define SICA_IMASK1 0xFFC00110·····/* SIC Interrupt Mask register 1 */
#define SICA_IAR0 0xFFC00124·····/* SIC Interrupt Assignment Register 0 */
#define SICA_IAR1 0xFFC00128·····/* SIC Interrupt Assignment Register 1 */
#define SICA_IAR2 0xFFC0012C·····/* SIC Interrupt Assignment Register 2 */
#define SICA_IAR3 0xFFC00130·····/* SIC Interrupt Assignment Register 3 */
#define SICA_IAR4 0xFFC00134·····/* SIC Interrupt Assignment Register 4 */
#define SICA_IAR5 0xFFC00138·····/* SIC Interrupt Assignment Register 5 */
#define SICA_IAR6 0xFFC0013C·····/* SIC Interrupt Assignment Register 6 */
#define SICA_IAR7 0xFFC00140·····/* SIC Interrupt Assignment Register 7 */
#define SICA_ISR0 0xFFC00114·····/* SIC Interrupt Status register 0 */
#define SICA_ISR1 0xFFC00118·····/* SIC Interrupt Status register 1 */
#define SICA_IWR0 0xFFC0011C·····/* SIC Interrupt Wakeup-Enable register 0 */
#define SICA_IWR1 0xFFC00120·····/* SIC Interrupt Wakeup-Enable register 1 */
/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */
#define bfin_read_SICA_SWRST() bfin_read16(SICA_SWRST)
#define bfin_write_SICA_SWRST(val) bfin_write16(SICA_SWRST,val)
#define bfin_read_SICA_SYSCR() bfin_read16(SICA_SYSCR)
#define bfin_write_SICA_SYSCR(val) bfin_write16(SICA_SYSCR,val)
#define bfin_read_SICA_RVECT() bfin_read16(SICA_RVECT)
#define bfin_write_SICA_RVECT(val) bfin_write16(SICA_RVECT,val)
#define bfin_read_SICA_IMASK() bfin_read32(SICA_IMASK)
#define bfin_write_SICA_IMASK(val) bfin_write32(SICA_IMASK,val)
#define bfin_read_SICA_IMASK0() bfin_read32(SICA_IMASK0)
#define bfin_write_SICA_IMASK0(val) bfin_write32(SICA_IMASK0,val)
#define bfin_read_SICA_IMASK1() bfin_read32(SICA_IMASK1)
#define bfin_write_SICA_IMASK1(val) bfin_write32(SICA_IMASK1,val)
#define bfin_read_SICA_IAR0() bfin_read32(SICA_IAR0)
#define bfin_write_SICA_IAR0(val) bfin_write32(SICA_IAR0,val)
#define bfin_read_SICA_IAR1() bfin_read32(SICA_IAR1)
#define bfin_write_SICA_IAR1(val) bfin_write32(SICA_IAR1,val)
#define bfin_read_SICA_IAR2() bfin_read32(SICA_IAR2)
#define bfin_write_SICA_IAR2(val) bfin_write32(SICA_IAR2,val)
#define bfin_read_SICA_IAR3() bfin_read32(SICA_IAR3)
#define bfin_write_SICA_IAR3(val) bfin_write32(SICA_IAR3,val)
#define bfin_read_SICA_IAR4() bfin_read32(SICA_IAR4)
#define bfin_write_SICA_IAR4(val) bfin_write32(SICA_IAR4,val)
#define bfin_read_SICA_IAR5() bfin_read32(SICA_IAR5)
#define bfin_write_SICA_IAR5(val) bfin_write32(SICA_IAR5,val)
#define bfin_read_SICA_IAR6() bfin_read32(SICA_IAR6)
#define bfin_write_SICA_IAR6(val) bfin_write32(SICA_IAR6,val)
#define bfin_read_SICA_IAR7() bfin_read32(SICA_IAR7)
#define bfin_write_SICA_IAR7(val) bfin_write32(SICA_IAR7,val)
#define bfin_read_SICA_ISR0() bfin_read32(SICA_ISR0)
#define bfin_write_SICA_ISR0(val) bfin_write32(SICA_ISR0,val)
#define bfin_read_SICA_ISR1() bfin_read32(SICA_ISR1)
#define bfin_write_SICA_ISR1(val) bfin_write32(SICA_ISR1,val)
#define bfin_read_SICA_IWR0() bfin_read32(SICA_IWR0)
#define bfin_write_SICA_IWR0(val) bfin_write32(SICA_IWR0,val)
#define bfin_read_SICA_IWR1() bfin_read32(SICA_IWR1)
#define bfin_write_SICA_IWR1(val) bfin_write32(SICA_IWR1,val)
...
static void bf561_internal_mask_irq(unsigned int irq)
{
unsigned long irq_mask;
if ((irq - (IRQ_CORETMR + 1)) < 32) {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
bfin_write_SICA_IMASK0(bfin_read_SICA_IMASK0() & ~irq_mask);
} else {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
bfin_write_SICA_IMASK1(bfin_read_SICA_IMASK1() & ~irq_mask);
}
}
which is about just as bad, but for different reasons. Now, there are
at least two common methods for how to do this better, both involving
the readl/writel low-level accessors (or something similar).
The first one uses enumerated register numers:
/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */
enum bfin_sica_regs {
SICA_SWRST = 100,·····/* Software Reset register */
SICA_SYSCR = 104,·····/* System Reset Configuration register */
SICA_RVECT = 108,·····/* SIC Reset Vector Address Register */
SICA_IMASK = 10C,·····/* SIC Interrupt Mask register 0 - hack to fix old tests */
SICA_IMASK0 = 10C,·····/* SIC Interrupt Mask register 0 */
SICA_IMASK1 = 110,·····/* SIC Interrupt Mask register 1 */
SICA_IAR0 = 124,·····/* SIC Interrupt Assignment Register 0 */
SICA_IAR1 = 128,·····/* SIC Interrupt Assignment Register 1 */
SICA_IAR2 = 12C,·····/* SIC Interrupt Assignment Register 2 */
SICA_IAR3 = 130,·····/* SIC Interrupt Assignment Register 3 */
SICA_IAR4 = 134,·····/* SIC Interrupt Assignment Register 4 */
SICA_IAR5 = 138,·····/* SIC Interrupt Assignment Register 5 */
SICA_IAR6 = 13C,·····/* SIC Interrupt Assignment Register 6 */
SICA_IAR7 = 140,·····/* SIC Interrupt Assignment Register 7 */
SICA_ISR0 = 114,·····/* SIC Interrupt Status register 0 */
SICA_ISR1 = 118,·····/* SIC Interrupt Status register 1 */
SICA_IWR0 = 11C,·····/* SIC Interrupt Wakeup-Enable register 0 */
SICA_IWR1 = 120,·····/* SIC Interrupt Wakeup-Enable register 1 */
};
...
static void __iomem *bfin_sica = (void __iomem *)0xffc00100ul;
static inline __le32 bfin_sica_read(int reg)
{
return (__le32)readl(bfin_sica + reg);
}
static inline void bfin_sica_write(int reg, __le32 val)
{
writel((uint32_t)val, bfin_sica + reg);
}
static void bf561_internal_mask_irq(unsigned int irq)
{
unsigned long irq_mask;
if ((irq - (IRQ_CORETMR + 1)) < 32) {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1)));
bfin_sica_write(SICA_IMASK0,
bfin_sica_read(SICA_IMASK0) & ~irq_mask);
} else {
irq_mask = (1 << (irq - (IRQ_CORETMR + 1) - 32));
bfin_sica_write(SICA_IMASK0,
bfin_sica_read(SICA_IMASK0) & ~irq_mask);
}
}
The alternative approach uses a structure:
/* System Reset and Interrupt Controller registers for core A (0xFFC0 0100-0xFFC0 01FF) */
struct bfin_sica_regs {
__le32 swrst; /* Software Reset register */
__le32 syscr; /* System Reset Configuration register */
__le32 rvect; /* SIC Reset Vector Address Register */
__le32 imask[2]; /* SIC Interrupt Mask register 0-1 */
__le32 iar[8]; /* SIC Interrupt Assignment Register 0-7 */
__le32 isr[2]; /* SIC Interrupt Status register 0-1 */
__le32 iwr[2]; /* SIC Interrupt Wakeup-Enable register 0-2 */
};
...
static struct bfin_sica_regs __iomem *bfin_sica = (void __iomem *)0xffc00100ul;
static void bf561_internal_mask_irq(unsigned int irq)
{
int irqnr = irq - (IRQ_CORETMR + 1);
__le32 __iomem *reg = &bfin_sica->imask[irqnr >> 5];
unsigned long irq_mask = 1 << (irqnr & 0x1f);
writel(reg, readl(reg) & ~irq_mask);
}
I'd personally prefer the last approach because of its compactness.
Arnd <><
next prev parent reply other threads:[~2006-09-23 13:08 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-09-21 3:32 [PATCH 1/4] Blackfin: arch patch for 2.6.18 Luke Yang
2006-09-21 9:59 ` Luke Yang
2006-09-23 0:18 ` Arnd Bergmann
2006-09-23 1:18 ` Randy.Dunlap
2006-09-23 1:24 ` Roland Dreier
2006-09-23 1:58 ` Mike Frysinger
2006-09-23 6:50 ` Mike Frysinger
2006-09-23 11:03 ` Arnd Bergmann
2006-09-23 11:15 ` Mike Frysinger
2006-09-23 11:29 ` Arnd Bergmann
2006-09-23 13:07 ` Arnd Bergmann [this message]
2006-09-23 11:28 ` Matthieu CASTET
2006-09-23 11:35 ` Mike Frysinger
2006-09-23 19:43 ` Arnd Bergmann
2006-09-24 3:49 ` Luke Yang
2006-09-24 3:35 ` Aubrey
2006-09-24 3:50 ` Randy Dunlap
2006-09-24 4:28 ` Aubrey
2006-09-25 6:54 ` Arnd Bergmann
2006-09-25 7:49 ` Aubrey
2006-09-25 9:26 ` Arnd Bergmann
2006-09-25 9:39 ` Luke Yang
2006-09-25 9:45 ` Aubrey
2006-09-25 15:39 ` Aubrey
2006-09-25 17:05 ` Arnd Bergmann
2006-09-26 3:42 ` Aubrey
2006-09-26 9:43 ` Arnd Bergmann
2006-09-27 10:04 ` Aubrey
2006-09-27 11:37 ` Arnd Bergmann
2006-09-23 21:27 ` David Woodhouse
2006-09-25 16:52 ` Randy Dunlap
2006-09-25 18:05 ` Mike Frysinger
-- strict thread matches above, loose matches on Subject: below --
2006-09-23 16:29 Robin Getz
2006-09-23 18:10 ` Arnd Bergmann
2006-09-23 17:57 Robin Getz
2006-09-23 23:25 Robin Getz
2006-09-24 7:29 ` David Woodhouse
2006-09-25 23:21 Robin Getz
2006-09-27 16:25 Robin Getz
2006-09-27 16:36 ` Randy Dunlap
2006-09-27 16:41 ` Arnd Bergmann
2006-09-27 17:19 ` Robin Getz
2006-09-27 20:57 ` Arnd Bergmann
2006-09-28 9:31 ` Bernd Schmidt
2006-09-28 11:04 ` Arnd Bergmann
2006-09-28 11:39 ` Bernd Schmidt
2006-09-28 12:35 ` Arnd Bergmann
2006-09-27 17:47 Robin Getz
2006-09-27 19:19 ` Jörn Engel
2006-09-27 21:22 Robin Getz
2006-09-27 21:36 ` Arnd Bergmann
2006-09-27 22:56 Robin Getz
2006-09-27 23:01 Robin Getz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200609231507.55198.arnd@arndb.de \
--to=arnd@arndb.de \
--cc=akpm@osdl.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luke.adi@gmail.com \
--cc=vapier.adi@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.