[PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Marcelo Tosatti]

Hi,

The following patch adds big endian version of ld_/st_ macros 
and converts core 8xx code to use them.

Other than making IO accesses explicit (which is a plus for
readability), a common set of macros provides a unified place for the
volatile flag to constraint compiler code reordering.

There are several unlucky places at the moment which lack the 
volatile flag.


diff --git a/arch/ppc/8xx_io/commproc.c b/arch/ppc/8xx_io/commproc.c
--- a/arch/ppc/8xx_io/commproc.c
+++ b/arch/ppc/8xx_io/commproc.c
@@ -73,7 +73,7 @@ cpm_mask_irq(unsigned int irq)
 {
 	int cpm_vec = irq - CPM_IRQ_OFFSET;
 
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr &= ~(1 << cpm_vec);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr, ld_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr) & ~(1 << cpm_vec));
 }
 
 static void
@@ -81,7 +81,7 @@ cpm_unmask_irq(unsigned int irq)
 {
 	int cpm_vec = irq - CPM_IRQ_OFFSET;
 
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr |= (1 << cpm_vec);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr, ld_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr) | (1 << cpm_vec));
 }
 
 static void
@@ -95,7 +95,7 @@ cpm_eoi(unsigned int irq)
 {
 	int cpm_vec = irq - CPM_IRQ_OFFSET;
 
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cisr = (1 << cpm_vec);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cisr, (1 << cpm_vec));
 }
 
 struct hw_interrupt_type cpm_pic = {
@@ -133,7 +133,7 @@ m8xx_cpm_reset(void)
 	 * manual recommends it.
 	 * Bit 25, FAM can also be set to use FEC aggressive mode (860T).
 	 */
-	imp->im_siu_conf.sc_sdcr = 1;
+	st_be32(&imp->im_siu_conf.sc_sdcr, 1);
 
 	/* Reclaim the DP memory for our use. */
 	m8xx_cpm_dpinit();
@@ -178,10 +178,10 @@ cpm_interrupt_init(void)
 
 	/* Initialize the CPM interrupt controller.
 	*/
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr =
+	st_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr,
 	    (CICR_SCD_SCC4 | CICR_SCC_SCC3 | CICR_SCB_SCC2 | CICR_SCA_SCC1) |
-		((CPM_INTERRUPT/2) << 13) | CICR_HP_MASK;
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr = 0;
+		((CPM_INTERRUPT/2) << 13) | CICR_HP_MASK);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr, 0);
 
         /* install the CPM interrupt controller routines for the CPM
          * interrupt vectors
@@ -198,7 +198,7 @@ cpm_interrupt_init(void)
 	if (setup_irq(CPM_IRQ_OFFSET + CPMVEC_ERROR, &cpm_error_irqaction))
 		panic("Could not allocate CPM error IRQ!");
 
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr |= CICR_IEN;
+	st_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr, ld_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr) | CICR_IEN);
 }
 
 /*
@@ -212,8 +212,8 @@ cpm_get_irq(struct pt_regs *regs)
 	/* Get the vector by setting the ACK bit and then reading
 	 * the register.
 	 */
-	((volatile immap_t *)IMAP_ADDR)->im_cpic.cpic_civr = 1;
-	cpm_vec = ((volatile immap_t *)IMAP_ADDR)->im_cpic.cpic_civr;
+	st_be16(&((volatile immap_t *)IMAP_ADDR)->im_cpic.cpic_civr, 1);
+	cpm_vec = ld_be16(&((volatile immap_t *)IMAP_ADDR)->im_cpic.cpic_civr);
 	cpm_vec >>= 11;
 
 	return cpm_vec;
diff --git a/arch/ppc/syslib/m8xx_setup.c b/arch/ppc/syslib/m8xx_setup.c
--- a/arch/ppc/syslib/m8xx_setup.c
+++ b/arch/ppc/syslib/m8xx_setup.c
@@ -144,12 +144,12 @@ void __init m8xx_calibrate_decr(void)
 	int freq, fp, divisor;
 
 	/* Unlock the SCCR. */
-	((volatile immap_t *)IMAP_ADDR)->im_clkrstk.cark_sccrk = ~KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_clkrstk.cark_sccrk = KAPWR_KEY;
+	st_be32(&((immap_t *)IMAP_ADDR)->im_clkrstk.cark_sccrk, ~KAPWR_KEY);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_clkrstk.cark_sccrk, KAPWR_KEY);
 
 	/* Force all 8xx processors to use divide by 16 processor clock. */
-	((volatile immap_t *)IMAP_ADDR)->im_clkrst.car_sccr |= 0x02000000;
-
+	st_be32(&((immap_t *)IMAP_ADDR)->im_clkrst.car_sccr, 
+		ld_be32(&((immap_t *)IMAP_ADDR)->im_clkrst.car_sccr)|0x02000000);
 	/* Processor frequency is MHz.
 	 * The value 'fp' is the number of decrementer ticks per second.
 	 */
@@ -175,28 +175,24 @@ void __init m8xx_calibrate_decr(void)
 	 * we guarantee the registers are locked, then we unlock them
 	 * for our use.
 	 */
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_tbscrk = ~KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck = ~KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk    = ~KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_tbscrk =  KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck =  KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk    =  KAPWR_KEY;
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbscrk, ~KAPWR_KEY);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck, ~KAPWR_KEY);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk, ~KAPWR_KEY);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbscrk, KAPWR_KEY);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck, KAPWR_KEY);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk, KAPWR_KEY);
 
 	/* Disable the RTC one second and alarm interrupts. */
-	((volatile immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc &=
-						~(RTCSC_SIE | RTCSC_ALE);
+	st_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc, ld_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc) & ~(RTCSC_SIE | RTCSC_ALE));
 	/* Enable the RTC */
-	((volatile immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc |=
-						(RTCSC_RTF | RTCSC_RTE);
+	st_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc, ld_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc) | (RTCSC_RTF | RTCSC_RTE));
 
 	/* Enabling the decrementer also enables the timebase interrupts
 	 * (or from the other point of view, to get decrementer interrupts
 	 * we have to enable the timebase).  The decrementer interrupt
 	 * is wired into the vector table, nothing to do here for that.
 	 */
-	((volatile immap_t *)IMAP_ADDR)->im_sit.sit_tbscr =
-				((mk_int_int_mask(DEC_INTERRUPT) << 8) |
-					 (TBSCR_TBF | TBSCR_TBE));
+	st_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_tbscr, (mk_int_int_mask(DEC_INTERRUPT) << 8) | (TBSCR_TBF | TBSCR_TBE));
 
 	if (setup_irq(DEC_INTERRUPT, &tbint_irqaction))
 		panic("Could not allocate timer IRQ!");
@@ -216,9 +212,9 @@ void __init m8xx_calibrate_decr(void)
 static int
 m8xx_set_rtc_time(unsigned long time)
 {
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_rtck = KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sit.sit_rtc = time;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_rtck = ~KAPWR_KEY;
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtck, KAPWR_KEY);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtc, time);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtck, ~KAPWR_KEY);
 	return(0);
 }
 
@@ -226,7 +222,7 @@ static unsigned long
 m8xx_get_rtc_time(void)
 {
 	/* Get time from the RTC. */
-	return((unsigned long)(((immap_t *)IMAP_ADDR)->im_sit.sit_rtc));
+	return (unsigned long) ld_be32(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtc);
 }
 
 static void
@@ -235,13 +231,13 @@ m8xx_restart(char *cmd)
 	__volatile__ unsigned char dummy;
 
 	local_irq_disable();
-	((immap_t *)IMAP_ADDR)->im_clkrst.car_plprcr |= 0x00000080;
+	st_be32(&((immap_t *)IMAP_ADDR)->im_clkrst.car_plprcr, ld_be32(&((immap_t *)IMAP_ADDR)->im_clkrst.car_plprcr) | 0x00000080);
 
 	/* Clear the ME bit in MSR to cause checkstop on machine check
 	*/
 	mtmsr(mfmsr() & ~0x1000);
 
-	dummy = ((immap_t *)IMAP_ADDR)->im_clkrst.res[0];
+	dummy = ld_be8(&((immap_t *)IMAP_ADDR)->im_clkrst.res[0]);
 	printk("Restart failed\n");
 	while(1);
 }
@@ -306,8 +302,7 @@ m8xx_init_IRQ(void)
 	i8259_init(0);
 
 	/* The i8259 cascade interrupt must be level sensitive. */
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_siel &=
-		~(0x80000000 >> ISA_BRIDGE_INT);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_siel, ld_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_siel & ~(0x80000000 >> ISA_BRIDGE_INT)));
 
 	if (setup_irq(ISA_BRIDGE_INT, &mbx_i8259_irqaction))
 		enable_irq(ISA_BRIDGE_INT);
diff --git a/arch/ppc/syslib/m8xx_wdt.c b/arch/ppc/syslib/m8xx_wdt.c
--- a/arch/ppc/syslib/m8xx_wdt.c
+++ b/arch/ppc/syslib/m8xx_wdt.c
@@ -29,8 +29,8 @@ void m8xx_wdt_reset(void)
 {
 	volatile immap_t *imap = (volatile immap_t *)IMAP_ADDR;
 
-	imap->im_siu_conf.sc_swsr = 0x556c;	/* write magic1 */
-	imap->im_siu_conf.sc_swsr = 0xaa39;	/* write magic2 */
+	st_be16(imap->im_siu_conf.sc_swsr, 0x556c);	/* write magic1 */
+	st_be16(imap->im_siu_conf.sc_swsr, 0xaa39);	/* write magic2 */
 }
 
 static irqreturn_t m8xx_wdt_interrupt(int irq, void *dev, struct pt_regs *regs)
@@ -39,7 +39,7 @@ static irqreturn_t m8xx_wdt_interrupt(in
 
 	m8xx_wdt_reset();
 
-	imap->im_sit.sit_piscr |= PISCR_PS;	/* clear irq */
+	st_be16(imap->im_sit.sit_piscr, ld_be16(imap->im_sit.sit_piscr | PISCR_PS));	/* clear irq */
 
 	return IRQ_HANDLED;
 }
@@ -51,7 +51,7 @@ void __init m8xx_wdt_handler_install(bd_
 	u32 sypcr;
 	u32 pitrtclk;
 
-	sypcr = imap->im_siu_conf.sc_sypcr;
+	sypcr = ld_be32(imap->im_siu_conf.sc_sypcr);
 
 	if (!(sypcr & 0x04)) {
 		printk(KERN_NOTICE "m8xx_wdt: wdt disabled (SYPCR: 0x%08X)\n",
@@ -87,9 +87,9 @@ void __init m8xx_wdt_handler_install(bd_
 	else
 		pitc = pitrtclk * wdt_timeout / binfo->bi_intfreq / 2;
 
-	imap->im_sit.sit_pitc = pitc << 16;
-	imap->im_sit.sit_piscr =
-	    (mk_int_int_mask(PIT_INTERRUPT) << 8) | PISCR_PIE | PISCR_PTE;
+	st_be32(imap->im_sit.sit_pitc, pitc << 16);
+
+	st_be16(imap->im_sit.sit_piscr, (mk_int_int_mask(PIT_INTERRUPT) << 8) | PISCR_PIE | PISCR_PTE);
 
 	if (setup_irq(PIT_INTERRUPT, &m8xx_wdt_irqaction))
 		panic("m8xx_wdt: error setting up the watchdog irq!");
diff --git a/arch/ppc/syslib/ppc8xx_pic.c b/arch/ppc/syslib/ppc8xx_pic.c
--- a/arch/ppc/syslib/ppc8xx_pic.c
+++ b/arch/ppc/syslib/ppc8xx_pic.c
@@ -29,8 +29,7 @@ static void m8xx_mask_irq(unsigned int i
 	word = irq_nr >> 5;
 
 	ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask =
-						ppc_cached_irq_mask[word];
+	st_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask, ppc_cached_irq_mask[word]);
 }
 
 static void m8xx_unmask_irq(unsigned int irq_nr)
@@ -41,8 +40,7 @@ static void m8xx_unmask_irq(unsigned int
 	word = irq_nr >> 5;
 
 	ppc_cached_irq_mask[word] |= (1 << (31-bit));
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask =
-						ppc_cached_irq_mask[word];
+	st_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask, ppc_cached_irq_mask[word]);
 }
 
 static void m8xx_end_irq(unsigned int irq_nr)
@@ -55,8 +53,7 @@ static void m8xx_end_irq(unsigned int ir
 		word = irq_nr >> 5;
 
 		ppc_cached_irq_mask[word] |= (1 << (31-bit));
-		((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask =
-			ppc_cached_irq_mask[word];
+		st_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask, ppc_cached_irq_mask[word]);
 	}
 }
 
@@ -69,9 +66,8 @@ static void m8xx_mask_and_ack(unsigned i
 	word = irq_nr >> 5;
 
 	ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask =
-						ppc_cached_irq_mask[word];
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_sipend = 1 << (31-bit);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask, ppc_cached_irq_mask[word]);
+	st_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_sipend, 1 << (31-bit));
 }
 
 struct hw_interrupt_type ppc8xx_pic = {
@@ -93,7 +89,7 @@ m8xx_get_irq(struct pt_regs *regs)
 	/* For MPC8xx, read the SIVEC register and shift the bits down
 	 * to get the irq number.
 	 */
-	irq = ((immap_t *)IMAP_ADDR)->im_siu_conf.sc_sivec >> 26;
+	irq = ld_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_sivec) >> 26;
 
 	/*
 	 * When we read the sivec without an interrupt to process, we will
diff --git a/include/asm-ppc/byteorder.h b/include/asm-ppc/byteorder.h
--- a/include/asm-ppc/byteorder.h
+++ b/include/asm-ppc/byteorder.h
@@ -7,6 +7,48 @@
 #ifdef __GNUC__
 #ifdef __KERNEL__
 
+/* big endian */
+extern __inline__ unsigned ld_be8(const volatile unsigned char *addr)
+{
+	unsigned val;
+
+	__asm__ __volatile__ ("lbz %0,0(%1)" : "=r" (val) : "r" (addr));
+	return val;
+}
+
+extern __inline__ void st_be8(volatile unsigned char *addr, const unsigned val)
+{
+	__asm__ __volatile__ ("stb %1,0(%2)" : "=m" (*addr) : "r" (val), "r" (addr));
+}
+
+extern __inline__ unsigned ld_be16(const volatile unsigned short *addr)
+{
+	unsigned val;
+
+	__asm__ __volatile__ ("lhz %0,0(%1)" : "=r" (val) : "r" (addr));
+	return val;
+}
+
+extern __inline__ void st_be16(volatile unsigned short *addr, const unsigned val)
+{
+	__asm__ __volatile__ ("sth %1,0(%2)" : "=m" (*addr) : "r" (val), "r" (addr));
+}
+
+extern __inline__ unsigned ld_be32(const volatile unsigned *addr)
+{
+	unsigned val;
+
+	__asm__ __volatile__ ("lwz %0,0(%1)" : "=r" (val) : "r" (addr));
+	return val;
+}
+
+extern __inline__ void st_be32(volatile unsigned *addr, const unsigned val)
+{
+	__asm__ __volatile__ ("stw %1,0(%2)" : "=m" (*addr) : "r" (val), "r" (addr));
+}
+
+/* little endian (reversed) */
+
 extern __inline__ unsigned ld_le16(const volatile unsigned short *addr)
 {
 	unsigned val;

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Benjamin Herrenschmidt]

On Wed, 2005-09-07 at 20:03 -0300, Marcelo Tosatti wrote:
> Hi,
> 
> The following patch adds big endian version of ld_/st_ macros 
> and converts core 8xx code to use them.
> 
> Other than making IO accesses explicit (which is a plus for
> readability), a common set of macros provides a unified place for the
> volatile flag to constraint compiler code reordering.
> 
> There are several unlucky places at the moment which lack the 
> volatile flag.

I'm not fan of the approach. You should use in_/out_ macros for IOs. If
you don't need eieio on 8xx , then just #ifdef it out of the
implementation of these.

Ben.

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Dan Malek]

On Sep 7, 2005, at 8:28 PM, Benjamin Herrenschmidt wrote:

> I'm not fan of the approach. You should use in_/out_ macros for IOs. If
> you don't need eieio on 8xx , then just #ifdef it out of the
> implementation of these.

Yeah, #ifdef :-)  That patch wouldn't go anywhere (well,
to /dev/null perhaps).

We can just use the in_/out_ macros that are there.  The eieio
doesn't hurt anything on the 8xx.

Thanks.

	-- Dan

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Marcelo Tosatti]

On Thu, Sep 08, 2005 at 10:28:52AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2005-09-07 at 20:03 -0300, Marcelo Tosatti wrote:
> > Hi,
> > 
> > The following patch adds big endian version of ld_/st_ macros 
> > and converts core 8xx code to use them.
> > 
> > Other than making IO accesses explicit (which is a plus for
> > readability), a common set of macros provides a unified place for the
> > volatile flag to constraint compiler code reordering.
> > 
> > There are several unlucky places at the moment which lack the 
> > volatile flag.
> 
> I'm not fan of the approach. You should use in_/out_ macros for IOs. If
> you don't need eieio on 8xx , then just #ifdef it out of the
> implementation of these.

The reason for that is that in_/out_ are supposed to be the standard 
IO macros (conformance)? In practice most drivers using the std macros
can benefit from the change. 

A common routine is shared by all architectures. Doing something like

/*
 * 8, 16 and 32 bit, big and little endian I/O operations, with barrier.
 */
extern inline int in_8(volatile unsigned char __iomem *addr)
{
        int ret;

        __asm__ __volatile__(
                "lbz%U1%X1 %0,%1;\n"
#ifndef CONFIG_8xx
                "twi 0,%0,0;\n"
                "isync" : "=r" (ret) : "m" (*addr));
#else
                : "=r" (ret) : "m" (*addr));
#endif
        return ret;
}

Seems somewhat ugly?

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Benjamin Herrenschmidt]

On Wed, 2005-09-07 at 20:42 -0400, Dan Malek wrote:
> On Sep 7, 2005, at 8:28 PM, Benjamin Herrenschmidt wrote:
> 
> > I'm not fan of the approach. You should use in_/out_ macros for IOs. If
> > you don't need eieio on 8xx , then just #ifdef it out of the
> > implementation of these.
> 
> Yeah, #ifdef :-)  That patch wouldn't go anywhere (well,
> to /dev/null perhaps).

Heh, it can be done properly: use an eieio() macro/inline and have
_that_ be #ifdef'ed to nothing on 8xx...

> We can just use the in_/out_ macros that are there.  The eieio
> doesn't hurt anything on the 8xx.

Ok, in that case, I don't see any problem.

Ben.

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Marcelo Tosatti]

On Wed, Sep 07, 2005 at 08:42:42PM -0400, Dan Malek wrote:
> 
> On Sep 7, 2005, at 8:28 PM, Benjamin Herrenschmidt wrote:
> 
> >I'm not fan of the approach. You should use in_/out_ macros for IOs. If
> >you don't need eieio on 8xx , then just #ifdef it out of the
> >implementation of these.
> 
> Yeah, #ifdef :-)  That patch wouldn't go anywhere (well,
> to /dev/null perhaps).

Well, its easy to s/st/out/ - s/ld/in/, so its still a useful patch. ;)

> We can just use the in_/out_ macros that are there.  The eieio
> doesn't hurt anything on the 8xx.

eieio/isync are not required. There's no need to execute redudant
instructions and occupy kernel space.

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Benjamin Herrenschmidt]

> e is shared by all architectures. Doing something like
> 
> /*
>  * 8, 16 and 32 bit, big and little endian I/O operations, with barrier.
>  */
> extern inline int in_8(volatile unsigned char __iomem *addr)
> {
>         int ret;
> 
>         __asm__ __volatile__(
>                 "lbz%U1%X1 %0,%1;\n"
> #ifndef CONFIG_8xx
>                 "twi 0,%0,0;\n"
>                 "isync" : "=r" (ret) : "m" (*addr));
> #else
>                 : "=r" (ret) : "m" (*addr));
> #endif
>         return ret;
> }
> 
> Seems somewhat ugly?

Yes. You can work around that by macro-generating the functions with
different rules for different CPUs, or having the twi/isync be a
separate asm block that is itself in a macro or inline that gets defined
differently on 8xx

Ben.

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Paul Mackerras]

Marcelo Tosatti writes:

> eieio/isync are not required. There's no need to execute redudant
> instructions and occupy kernel space.

Given how long it takes to access I/O devices, I don't believe an
extra instruction or two is going to make a measurable difference.
Feel free to show me measurements proving me wrong, of course... :)

Paul.

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Dan Malek]

On Sep 8, 2005, at 12:27 AM, Paul Mackerras wrote:

> Given how long it takes to access I/O devices, I don't believe an
> extra instruction or two is going to make a measurable difference.

That's kind of true.  However, the access to the CPM is nearly
instruction speed, except in the case when you actually use a
real IO register (like a GPIO on a port), then it's a little slower.


	-- Dan

Re: [PATCH] add big endian version of ld_/st_ IO access macros and convert main 8xx code to use it

[Marcelo Tosatti]

On Thu, Sep 08, 2005 at 02:27:11PM +1000, Paul Mackerras wrote:
> Marcelo Tosatti writes:
> 
> > eieio/isync are not required. There's no need to execute redudant
> > instructions and occupy kernel space.
> 
> Given how long it takes to access I/O devices, I don't believe an
> extra instruction or two is going to make a measurable difference.
> Feel free to show me measurements proving me wrong, of course... :)

I will try to measure it later on and change out_/in_ macros as         
suggested.                                                              

For now are there any objections against this? Otherwise I'll merge it.

There are a few more places which need conversion, plan to change them
later on also.

Thanks everyone.

diff --git a/arch/ppc/8xx_io/commproc.c b/arch/ppc/8xx_io/commproc.c
--- a/arch/ppc/8xx_io/commproc.c
+++ b/arch/ppc/8xx_io/commproc.c
@@ -73,7 +73,7 @@ cpm_mask_irq(unsigned int irq)
 {
 	int cpm_vec = irq - CPM_IRQ_OFFSET;
 
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr &= ~(1 << cpm_vec);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr, in_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr) & ~(1 << cpm_vec));
 }
 
 static void
@@ -81,7 +81,7 @@ cpm_unmask_irq(unsigned int irq)
 {
 	int cpm_vec = irq - CPM_IRQ_OFFSET;
 
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr |= (1 << cpm_vec);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr, in_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr) | (1 << cpm_vec));
 }
 
 static void
@@ -95,7 +95,7 @@ cpm_eoi(unsigned int irq)
 {
 	int cpm_vec = irq - CPM_IRQ_OFFSET;
 
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cisr = (1 << cpm_vec);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cisr, (1 << cpm_vec));
 }
 
 struct hw_interrupt_type cpm_pic = {
@@ -133,7 +133,7 @@ m8xx_cpm_reset(void)
 	 * manual recommends it.
 	 * Bit 25, FAM can also be set to use FEC aggressive mode (860T).
 	 */
-	imp->im_siu_conf.sc_sdcr = 1;
+	out_be32(&imp->im_siu_conf.sc_sdcr, 1),
 
 	/* Reclaim the DP memory for our use. */
 	m8xx_cpm_dpinit();
@@ -178,10 +178,10 @@ cpm_interrupt_init(void)
 
 	/* Initialize the CPM interrupt controller.
 	*/
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr =
+	out_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr,
 	    (CICR_SCD_SCC4 | CICR_SCC_SCC3 | CICR_SCB_SCC2 | CICR_SCA_SCC1) |
-		((CPM_INTERRUPT/2) << 13) | CICR_HP_MASK;
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr = 0;
+		((CPM_INTERRUPT/2) << 13) | CICR_HP_MASK);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cimr, 0);
 
         /* install the CPM interrupt controller routines for the CPM
          * interrupt vectors
@@ -198,7 +198,7 @@ cpm_interrupt_init(void)
 	if (setup_irq(CPM_IRQ_OFFSET + CPMVEC_ERROR, &cpm_error_irqaction))
 		panic("Could not allocate CPM error IRQ!");
 
-	((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr |= CICR_IEN;
+	out_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr, in_be32(&((immap_t *)IMAP_ADDR)->im_cpic.cpic_cicr) | CICR_IEN);
 }
 
 /*
@@ -212,8 +212,8 @@ cpm_get_irq(struct pt_regs *regs)
 	/* Get the vector by setting the ACK bit and then reading
 	 * the register.
 	 */
-	((volatile immap_t *)IMAP_ADDR)->im_cpic.cpic_civr = 1;
-	cpm_vec = ((volatile immap_t *)IMAP_ADDR)->im_cpic.cpic_civr;
+	out_be16(&((volatile immap_t *)IMAP_ADDR)->im_cpic.cpic_civr, 1);
+	cpm_vec = in_be16(&((volatile immap_t *)IMAP_ADDR)->im_cpic.cpic_civr);
 	cpm_vec >>= 11;
 
 	return cpm_vec;
diff --git a/arch/ppc/syslib/m8xx_setup.c b/arch/ppc/syslib/m8xx_setup.c
--- a/arch/ppc/syslib/m8xx_setup.c
+++ b/arch/ppc/syslib/m8xx_setup.c
@@ -144,12 +144,12 @@ void __init m8xx_calibrate_decr(void)
 	int freq, fp, divisor;
 
 	/* Unlock the SCCR. */
-	((volatile immap_t *)IMAP_ADDR)->im_clkrstk.cark_sccrk = ~KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_clkrstk.cark_sccrk = KAPWR_KEY;
+	out_be32(&((immap_t *)IMAP_ADDR)->im_clkrstk.cark_sccrk, ~KAPWR_KEY);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_clkrstk.cark_sccrk, KAPWR_KEY);
 
 	/* Force all 8xx processors to use divide by 16 processor clock. */
-	((volatile immap_t *)IMAP_ADDR)->im_clkrst.car_sccr |= 0x02000000;
-
+	out_be32(&((immap_t *)IMAP_ADDR)->im_clkrst.car_sccr, 
+		in_be32(&((immap_t *)IMAP_ADDR)->im_clkrst.car_sccr)|0x02000000);
 	/* Processor frequency is MHz.
 	 * The value 'fp' is the number of decrementer ticks per second.
 	 */
@@ -175,28 +175,24 @@ void __init m8xx_calibrate_decr(void)
 	 * we guarantee the registers are locked, then we unlock them
 	 * for our use.
 	 */
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_tbscrk = ~KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck = ~KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk    = ~KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_tbscrk =  KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck =  KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk    =  KAPWR_KEY;
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbscrk, ~KAPWR_KEY);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck, ~KAPWR_KEY);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk, ~KAPWR_KEY);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbscrk, KAPWR_KEY);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtcsck, KAPWR_KEY);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_tbk, KAPWR_KEY);
 
 	/* Disable the RTC one second and alarm interrupts. */
-	((volatile immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc &=
-						~(RTCSC_SIE | RTCSC_ALE);
+	out_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc, in_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc) & ~(RTCSC_SIE | RTCSC_ALE));
 	/* Enable the RTC */
-	((volatile immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc |=
-						(RTCSC_RTF | RTCSC_RTE);
+	out_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc, in_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtcsc) | (RTCSC_RTF | RTCSC_RTE));
 
 	/* Enabling the decrementer also enables the timebase interrupts
 	 * (or from the other point of view, to get decrementer interrupts
 	 * we have to enable the timebase).  The decrementer interrupt
 	 * is wired into the vector table, nothing to do here for that.
 	 */
-	((volatile immap_t *)IMAP_ADDR)->im_sit.sit_tbscr =
-				((mk_int_int_mask(DEC_INTERRUPT) << 8) |
-					 (TBSCR_TBF | TBSCR_TBE));
+	out_be16(&((immap_t *)IMAP_ADDR)->im_sit.sit_tbscr, (mk_int_int_mask(DEC_INTERRUPT) << 8) | (TBSCR_TBF | TBSCR_TBE));
 
 	if (setup_irq(DEC_INTERRUPT, &tbint_irqaction))
 		panic("Could not allocate timer IRQ!");
@@ -216,9 +212,9 @@ void __init m8xx_calibrate_decr(void)
 static int
 m8xx_set_rtc_time(unsigned long time)
 {
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_rtck = KAPWR_KEY;
-	((volatile immap_t *)IMAP_ADDR)->im_sit.sit_rtc = time;
-	((volatile immap_t *)IMAP_ADDR)->im_sitk.sitk_rtck = ~KAPWR_KEY;
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtck, KAPWR_KEY);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtc, time);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_sitk.sitk_rtck, ~KAPWR_KEY);
 	return(0);
 }
 
@@ -226,7 +222,7 @@ static unsigned long
 m8xx_get_rtc_time(void)
 {
 	/* Get time from the RTC. */
-	return((unsigned long)(((immap_t *)IMAP_ADDR)->im_sit.sit_rtc));
+	return (unsigned long) in_be32(&((immap_t *)IMAP_ADDR)->im_sit.sit_rtc);
 }
 
 static void
@@ -235,13 +231,13 @@ m8xx_restart(char *cmd)
 	__volatile__ unsigned char dummy;
 
 	local_irq_disable();
-	((immap_t *)IMAP_ADDR)->im_clkrst.car_plprcr |= 0x00000080;
+	out_be32(&((immap_t *)IMAP_ADDR)->im_clkrst.car_plprcr, in_be32(&((immap_t *)IMAP_ADDR)->im_clkrst.car_plprcr) | 0x00000080);
 
 	/* Clear the ME bit in MSR to cause checkstop on machine check
 	*/
 	mtmsr(mfmsr() & ~0x1000);
 
-	dummy = ((immap_t *)IMAP_ADDR)->im_clkrst.res[0];
+	dummy = in_8(&((immap_t *)IMAP_ADDR)->im_clkrst.res[0]);
 	printk("Restart failed\n");
 	while(1);
 }
@@ -306,8 +302,7 @@ m8xx_init_IRQ(void)
 	i8259_init(0);
 
 	/* The i8259 cascade interrupt must be level sensitive. */
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_siel &=
-		~(0x80000000 >> ISA_BRIDGE_INT);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_siel, in_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_siel & ~(0x80000000 >> ISA_BRIDGE_INT)));
 
 	if (setup_irq(ISA_BRIDGE_INT, &mbx_i8259_irqaction))
 		enable_irq(ISA_BRIDGE_INT);
diff --git a/arch/ppc/syslib/m8xx_wdt.c b/arch/ppc/syslib/m8xx_wdt.c
--- a/arch/ppc/syslib/m8xx_wdt.c
+++ b/arch/ppc/syslib/m8xx_wdt.c
@@ -29,8 +29,8 @@ void m8xx_wdt_reset(void)
 {
 	volatile immap_t *imap = (volatile immap_t *)IMAP_ADDR;
 
-	imap->im_siu_conf.sc_swsr = 0x556c;	/* write magic1 */
-	imap->im_siu_conf.sc_swsr = 0xaa39;	/* write magic2 */
+	out_be16(imap->im_siu_conf.sc_swsr, 0x556c);	/* write magic1 */
+	out_be16(imap->im_siu_conf.sc_swsr, 0xaa39);	/* write magic2 */
 }
 
 static irqreturn_t m8xx_wdt_interrupt(int irq, void *dev, struct pt_regs *regs)
@@ -39,7 +39,7 @@ static irqreturn_t m8xx_wdt_interrupt(in
 
 	m8xx_wdt_reset();
 
-	imap->im_sit.sit_piscr |= PISCR_PS;	/* clear irq */
+	out_be16(imap->im_sit.sit_piscr, in_be16(imap->im_sit.sit_piscr | PISCR_PS));	/* clear irq */
 
 	return IRQ_HANDLED;
 }
@@ -51,7 +51,7 @@ void __init m8xx_wdt_handler_install(bd_
 	u32 sypcr;
 	u32 pitrtclk;
 
-	sypcr = imap->im_siu_conf.sc_sypcr;
+	sypcr = in_be32(imap->im_siu_conf.sc_sypcr);
 
 	irch/ppc/syslib/ppc8xx_pic.c:32:f (!(sypcr & 0x04)) {
 		printk(KERN_NOTICE "m8xx_wdt: wdt disabled (SYPCR: 0x%08X)\n",
@@ -87,9 +87,9 @@ void __init m8xx_wdt_handler_install(bd_
 	else
 		pitc = pitrtclk * wdt_timeout / binfo->bi_intfreq / 2;
 
-	imap->im_sit.sit_pitc = pitc << 16;
-	imap->im_sit.sit_piscr =
-	    (mk_int_int_mask(PIT_INTERRUPT) << 8) | PISCR_PIE | PISCR_PTE;
+	out_be32(imap->im_sit.sit_pitc, pitc << 16);
+
+	out_be16(imap->im_sit.sit_piscr, (mk_int_int_mask(PIT_INTERRUPT) << 8) | PISCR_PIE | PISCR_PTE);
 
 	if (setup_irq(PIT_INTERRUPT, &m8xx_wdt_irqaction))
 		panic("m8xx_wdt: error setting up the watchdog irq!");
diff --git a/arch/ppc/syslib/ppc8xx_pic.c b/arch/ppc/syslib/ppc8xx_pic.c
--- a/arch/ppc/syslib/ppc8xx_pic.c
+++ b/arch/ppc/syslib/ppc8xx_pic.c
@@ -6,6 +6,7 @@
 #include <linux/signal.h>
 #include <linux/interrupt.h>
 #include <asm/irq.h>
+#include <asm/io.h>
 #include <asm/8xx_immap.h>
 #include <asm/mpc8xx.h>
 #include "ppc8xx_pic.h"
@@ -29,8 +30,7 @@ static void m8xx_mask_irq(unsigned int i
 	word = irq_nr >> 5;
 
 	ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask =
-						ppc_cached_irq_mask[word];
+	out_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask, ppc_cached_irq_mask[word]);
 }
 
 static void m8xx_unmask_irq(unsigned int irq_nr)
@@ -41,8 +41,7 @@ static void m8xx_unmask_irq(unsigned int
 	word = irq_nr >> 5;
 
 	ppc_cached_irq_mask[word] |= (1 << (31-bit));
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask =
-						ppc_cached_irq_mask[word];
+	out_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask, ppc_cached_irq_mask[word]);
 }
 
 static void m8xx_end_irq(unsigned int irq_nr)
@@ -55,8 +54,7 @@ static void m8xx_end_irq(unsigned int ir
 		word = irq_nr >> 5;
 
 		ppc_cached_irq_mask[word] |= (1 << (31-bit));
-		((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask =
-			ppc_cached_irq_mask[word];
+		out_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask, ppc_cached_irq_mask[word]);
 	}
 }
 
@@ -69,9 +67,8 @@ static void m8xx_mask_and_ack(unsigned i
 	word = irq_nr >> 5;
 
 	ppc_cached_irq_mask[word] &= ~(1 << (31-bit));
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask =
-						ppc_cached_irq_mask[word];
-	((immap_t *)IMAP_ADDR)->im_siu_conf.sc_sipend = 1 << (31-bit);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_simask, ppc_cached_irq_mask[word]);
+	out_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_sipend, 1 << (31-bit));
 }
 
 struct hw_interrupt_type ppc8xx_pic = {
@@ -93,7 +90,7 @@ m8xx_get_irq(struct pt_regs *regs)
 	/* For MPC8xx, read the SIVEC register and shift the bits down
 	 * to get the irq number.
 	 */
-	irq = ((immap_t *)IMAP_ADDR)->im_siu_conf.sc_sivec >> 26;
+	irq = in_be32(&((immap_t *)IMAP_ADDR)->im_siu_conf.sc_sivec) >> 26;
 
 	/*
 	 * When we read the sivec without an interrupt to process, we will