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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox