Re: [PATCH 1/3] [POWERPC] 8xx: mpc885ads pcmcia support

[Vitaly Bordug <vitb@kernel.crashing.org>]

On Tue, 8 May 2007 09:04:22 -0500
Kumar Gala wrote:

> 
> On May 8, 2007, at 4:50 AM, Vitaly Bordug wrote:
> 
> >
> > Adds support for PowerQuicc on-chip PCMCIA. The driver is  
> > implemented as
> > of_device, so only arch/powerpc stuff is capable to use it, which
> > now implies only mpc885ads reference board.
> >
> > To cope with the code that should be hooked inside driver, but is  
> > really
> > board specific (like set_voltage), global structure
> > mpc8xx_pcmcia_ops holds necessary function pointers that are filled
> > in the BSP code.
> >
> > Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > Acked-by: Olof Johansson <olof@lixom.net>
> >
> > ---
> >
> >  arch/powerpc/boot/dts/mpc885ads.dts          |   12 +
> >  arch/powerpc/platforms/8xx/m8xx_setup.c      |    5
> >  arch/powerpc/platforms/8xx/mpc885ads.h       |    8 +
> >  arch/powerpc/platforms/8xx/mpc885ads_setup.c |   72 +++++
> >  arch/powerpc/sysdev/fsl_soc.c                |   13 +
> >  arch/powerpc/sysdev/mpc8xx_pic.h             |    2
> >  drivers/pcmcia/Kconfig                       |    1
> >  drivers/pcmcia/m8xx_pcmcia.c                 |  351 +++++++++++ 
> > +--------------
> >  include/asm-powerpc/mpc8xx.h                 |    4
> >  include/linux/fsl_devices.h                  |    5
> >  10 files changed, 287 insertions(+), 186 deletions(-)
> >
> > diff --git a/arch/powerpc/boot/dts/mpc885ads.dts b/arch/powerpc/ 
> > boot/dts/mpc885ads.dts
> > index 110bf61..0786ac1 100644
> > --- a/arch/powerpc/boot/dts/mpc885ads.dts
> > +++ b/arch/powerpc/boot/dts/mpc885ads.dts
> > @@ -112,6 +112,18 @@
> >  			compatible = "CPM";
> >  		};
> >
> > +		pcmcia@0080 {
> > +			#address-cells = <3>;
> > +			#interrupt-cells = <1>;
> > +			#size-cells = <2>;
> > +			compatible = "fsl,pq-pcmcia";
> 
> should this be fsl,pq1-pcmcia or fsl,8xx-pcmcia?
>

there were a note that eventually fsl may create some 8xx that is not cpm...
I really think pq and pq1 is the same, for pq2, 3 etc number passed explicitly. 
> > +			device_type = "pcmcia";
> > +			reg = <80 80>;
> > +			clock-frequency = <2faf080>;
> 
> is the clock-freq fixed?
> 
gotta check, will follow-up.

> > +			interrupt-parent = <&mpc8xx-pic>;
> > +			interrupts = <d 1>;
> > +		};
> > +
> >  		cpm@ff000000 {
> >  			linux,phandle = <ff000000>;
> >  			#address-cells = <1>;
> > diff --git a/arch/powerpc/platforms/8xx/m8xx_setup.c
> > b/arch/powerpc/ platforms/8xx/m8xx_setup.c
> > index 0901dba..f169355 100644
> > --- a/arch/powerpc/platforms/8xx/m8xx_setup.c
> > +++ b/arch/powerpc/platforms/8xx/m8xx_setup.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/root_dev.h>
> >  #include <linux/time.h>
> >  #include <linux/rtc.h>
> > +#include <linux/fsl_devices.h>
> >
> >  #include <asm/mmu.h>
> >  #include <asm/reg.h>
> > @@ -49,6 +50,10 @@
> >
> >  #include "sysdev/mpc8xx_pic.h"
> >
> > +#ifdef CONFIG_PCMCIA_M8XX
> > +struct mpc8xx_pcmcia_ops m8xx_pcmcia_ops;
> > +#endif
> > +
> >  void m8xx_calibrate_decr(void);
> >  extern void m8xx_wdt_handler_install(bd_t *bp);
> >  extern int cpm_pic_init(void);
> > diff --git a/arch/powerpc/platforms/8xx/mpc885ads.h b/arch/powerpc/ 
> > platforms/8xx/mpc885ads.h
> > index 7c31aec..932b59a 100644
> > --- a/arch/powerpc/platforms/8xx/mpc885ads.h
> > +++ b/arch/powerpc/platforms/8xx/mpc885ads.h
> > @@ -91,5 +91,13 @@
> >  #define SICR_ENET_MASK	((uint)0x00ff0000)
> >  #define SICR_ENET_CLKRT	((uint)0x002c0000)
> >
> > +/*
> > + * Some internal interrupt registers use an 8-bit mask for the  
> > interrupt
> > + * level instead of a number.
> > + */
> > +static inline uint mk_int_int_mask(uint mask) {
> > +	return (1 << (7 - (mask/2)));
> > +}
> 
> would this be better off in sysdev/mpc8xx_pic.h?
> 
hmm, maybe.
> > +
> >  #endif /* __ASM_MPC885ADS_H__ */
> >  #endif /* __KERNEL__ */
> > diff --git a/arch/powerpc/platforms/8xx/mpc885ads_setup.c b/arch/ 
> > powerpc/platforms/8xx/mpc885ads_setup.c
> > index a57b577..80e7214 100644
> > --- a/arch/powerpc/platforms/8xx/mpc885ads_setup.c
> > +++ b/arch/powerpc/platforms/8xx/mpc885ads_setup.c
> > @@ -22,6 +22,7 @@
> >
> >  #include <linux/fs_enet_pd.h>
> >  #include <linux/fs_uart_pd.h>
> > +#include <linux/fsl_devices.h>
> >  #include <linux/mii.h>
> >
> >  #include <asm/delay.h>
> > @@ -51,6 +52,70 @@ static void init_smc1_uart_ioports(struct  
> > fs_uart_platform_info* fpi);
> >  static void init_smc2_uart_ioports(struct fs_uart_platform_info*  
> > fpi);
> >  static void init_scc3_ioports(struct fs_platform_info* ptr);
> >
> > +#ifdef CONFIG_PCMCIA_M8XX
> > +static void pcmcia_hw_setup(int slot, int enable)
> > +{
> > +	unsigned *bcsr_io;
> > +
> > +	bcsr_io = ioremap(BCSR1, sizeof(unsigned long));
> > +	if (enable)
> > +		clrbits32(bcsr_io, BCSR1_PCCEN);
> > +	else
> > +		setbits32(bcsr_io, BCSR1_PCCEN);
> > +
> > +	iounmap(bcsr_io);
> > +}
> > +
> > +static int pcmcia_set_voltage(int slot, int vcc, int vpp)
> > +{
> > +	u32 reg = 0;
> > +	unsigned *bcsr_io;
> > +
> > +	bcsr_io = ioremap(BCSR1, sizeof(unsigned long));
> > +
> > +	switch(vcc) {
> > +	case 0:
> > +		break;
> > +	case 33:
> > +		reg |= BCSR1_PCCVCC0;
> > +		break;
> > +	case 50:
> > +		reg |= BCSR1_PCCVCC1;
> > +		break;
> > +	default:
> > +		return 1;
> > +	}
> > +
> > +	switch(vpp) {
> > +	case 0:
> > +		break;
> > +	case 33:
> > +	case 50:
> > +	if(vcc == vpp)
> > +			reg |= BCSR1_PCCVPP1;
> > +		else
> > +			return 1;
> > +		break;
> > +	case 120:
> > +	if ((vcc == 33) || (vcc == 50))
> > +		reg |= BCSR1_PCCVPP0;
> > +	else
> > +		return 1;
> > +	default:
> > +		return 1;
> > +	}
> > +
> 
> seems like formatting is of (but that could just me my email reader)
> 

darn it, I'll better Lindent this whole file too in 2-nd patch. plenty of whitespace, spacetabs, formatting issues...

> > +	/* first, turn off all power */
> > +	clrbits32(bcsr_io, 0x00610000);
> > +
> > +	/* enable new powersettings */
> > +	setbits32(bcsr_io, reg);
> > +
> > +	iounmap(bcsr_io);
> > +	return 0;
> > +}
> > +#endif
> > +
> >  void __init mpc885ads_board_setup(void)
> >  {
> >  	cpm8xx_t *cp;
> > @@ -115,6 +180,12 @@ void __init mpc885ads_board_setup(void)
> >  	immr_unmap(io_port);
> >
> >  #endif
> > +
> > +#ifdef CONFIG_PCMCIA_M8XX
> > +	/*Set up board specific hook-ups*/
> > +	m8xx_pcmcia_ops.hw_ctrl = pcmcia_hw_setup;
> > +	m8xx_pcmcia_ops.voltage_set = pcmcia_set_voltage;
> > +#endif
> >  }
> >
> >
> > @@ -322,6 +393,7 @@ void init_smc_ioports(struct  
> > fs_uart_platform_info *data)
> >  	}
> >  }
> >
> > +
> 
> extra whitespace?
yes, ok.

[snip]
-- 
Sincerely, Vitaly