[PATCH] 2/7 AU1100 MMC support

[PATCH] 2/7 AU1100 MMC support

[Rodolfo Giometti]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

Protect code on "BCSR" device.

Signed-off-by: Rodolfo Giometti <giometti@linux.it>

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

[-- Attachment #2: patch-mmc-de-bcsr --]
[-- Type: text/plain, Size: 1885 bytes --]

diff --git a/drivers/mmc/au1xmmc.c b/drivers/mmc/au1xmmc.c
index b0dc1d0..6084bb8 100644
--- a/drivers/mmc/au1xmmc.c
+++ b/drivers/mmc/au1xmmc.c
@@ -65,8 +65,8 @@ #endif
 const struct {
 	u32 iobase;
 	u32 tx_devid, rx_devid;
-	u16 bcsrpwr;
-	u16 bcsrstatus;
+	u16 power;
+	u16 status;
 	u16 wpstatus;
 } au1xmmc_card_table[] = {
 	{ SD0_BASE, DSCR_CMD0_SDMS_TX0, DSCR_CMD0_SDMS_RX0,
@@ -138,24 +138,42 @@ static inline void SEND_STOP(struct au1x
 static void au1xmmc_set_power(struct au1xmmc_host *host, int state)
 {
 
-	u32 val = au1xmmc_card_table[host->id].bcsrpwr;
+	u32 val;
 
+	val = au1xmmc_card_table[host->id].power;
+
+#if defined(CONFIG_MIPS_DB1200)
 	bcsr->board &= ~val;
 	if (state) bcsr->board |= val;
+#endif
 
 	au_sync_delay(1);
 }
 
 static inline int au1xmmc_card_inserted(struct au1xmmc_host *host)
 {
-	return (bcsr->sig_status & au1xmmc_card_table[host->id].bcsrstatus)
-		? 1 : 0;
+	u32 val, data = 1;
+
+	val = au1xmmc_card_table[host->id].status;
+
+#if defined(CONFIG_MIPS_DB1200)
+	data = bcsr->sig_status & val;
+#endif
+
+	return !!data;
 }
 
 static inline int au1xmmc_card_readonly(struct au1xmmc_host *host)
 {
-	return (bcsr->status & au1xmmc_card_table[host->id].wpstatus)
-		? 1 : 0;
+	u32 val, data = 0;
+
+	val = au1xmmc_card_table[host->id].wpstatus;
+
+#if defined(CONFIG_MIPS_DB1200)
+	data = bcsr->status & val;
+#endif
+
+	return !!data;
 }
 
 static void au1xmmc_finish_request(struct au1xmmc_host *host)
@@ -175,7 +193,9 @@ static void au1xmmc_finish_request(struc
 
 	host->status = HOST_S_IDLE;
 
+#if defined(CONFIG_MIPS_DB1200)
 	bcsr->disk_leds |= (1 << 8);
+#endif
 
 	mmc_request_done(host->mmc, mrq);
 }
@@ -670,7 +690,9 @@ static void au1xmmc_request(struct mmc_h
 	host->mrq = mrq;
 	host->status = HOST_S_CMD;
 
+#if defined(CONFIG_MIPS_DB1200)
 	bcsr->disk_leds &= ~(1 << 8);
+#endif
 
 	if (mrq->data) {
 		FLUSH_FIFO(host);

Re: [PATCH] 2/7 AU1100 MMC support

[Sergei Shtylyov]

Hello.

Rodolfo Giometti wrote:
> Protect code on "BCSR" device.

> Signed-off-by: Rodolfo Giometti <giometti@linux.it>

> ------------------------------------------------------------------------

> diff --git a/drivers/mmc/au1xmmc.c b/drivers/mmc/au1xmmc.c
> index b0dc1d0..6084bb8 100644
> --- a/drivers/mmc/au1xmmc.c
> +++ b/drivers/mmc/au1xmmc.c
> @@ -65,8 +65,8 @@ #endif
>  const struct {
>  	u32 iobase;
>  	u32 tx_devid, rx_devid;
> -	u16 bcsrpwr;
> -	u16 bcsrstatus;
> +	u16 power;
> +	u16 status;
>  	u16 wpstatus;
>  } au1xmmc_card_table[] = {
>  	{ SD0_BASE, DSCR_CMD0_SDMS_TX0, DSCR_CMD0_SDMS_RX0,
> @@ -138,24 +138,42 @@ static inline void SEND_STOP(struct au1x
>  static void au1xmmc_set_power(struct au1xmmc_host *host, int state)
>  {
>  
> -	u32 val = au1xmmc_card_table[host->id].bcsrpwr;
> +	u32 val;
>  
> +	val = au1xmmc_card_table[host->id].power;
> +
> +#if defined(CONFIG_MIPS_DB1200)
>  	bcsr->board &= ~val;
>  	if (state) bcsr->board |= val;
> +#endif
>  
>  	au_sync_delay(1);
>  }

    If DBAu1100 doesn't allow to control slot power, then I don't think 
pretending it does is a good thing. Shouldn't these #ifdef's be in 
au1xmmc_set_ios() instead (the function is void anyway but that would allow us 
to save on the code size a bit more)?

>  static inline int au1xmmc_card_inserted(struct au1xmmc_host *host)
>  {
> -	return (bcsr->sig_status & au1xmmc_card_table[host->id].bcsrstatus)
> -		? 1 : 0;
> +	u32 val, data = 1;
> +
> +	val = au1xmmc_card_table[host->id].status;
> +
> +#if defined(CONFIG_MIPS_DB1200)
> +	data = bcsr->sig_status & val;
> +#endif
> +
> +	return !!data;
>  }

    Hrm, are you sure there's no way to sense that the card is *really* 
inserted or not?

>  static inline int au1xmmc_card_readonly(struct au1xmmc_host *host)
>  {
> -	return (bcsr->status & au1xmmc_card_table[host->id].wpstatus)
> -		? 1 : 0;
> +	u32 val, data = 0;
> +
> +	val = au1xmmc_card_table[host->id].wpstatus;
> +
> +#if defined(CONFIG_MIPS_DB1200)
> +	data = bcsr->status & val;
> +#endif
> +
> +	return !!data;
>  }

    Ditto.

WBR, Sergei

Re: [PATCH] 2/7 AU1100 MMC support

[Rodolfo Giometti]

[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]

On Thu, Aug 10, 2006 at 05:29:38PM +0400, Sergei Shtylyov wrote:
> > static void au1xmmc_set_power(struct au1xmmc_host *host, int state)
> > {
> > 
> >-	u32 val = au1xmmc_card_table[host->id].bcsrpwr;
> >+	u32 val;
> > 
> >+	val = au1xmmc_card_table[host->id].power;
> >+
> >+#if defined(CONFIG_MIPS_DB1200)
> > 	bcsr->board &= ~val;
> > 	if (state) bcsr->board |= val;
> >+#endif
> > 
> > 	au_sync_delay(1);
> > }
> 
>    If DBAu1100 doesn't allow to control slot power, then I don't think 
> pretending it does is a good thing. Shouldn't these #ifdef's be in 
> au1xmmc_set_ios() instead (the function is void anyway but that would allow 
> us to save on the code size a bit more)?

Mmm. I proposed that solution but I don't know exaclty how several
DB1x00 boards work. I just protect the variable "bcsr" which is not
defined for my board.

> > static inline int au1xmmc_card_inserted(struct au1xmmc_host *host)
> > {
> >-	return (bcsr->sig_status & au1xmmc_card_table[host->id].bcsrstatus)
> >-		? 1 : 0;
> >+	u32 val, data = 1;
> >+
> >+	val = au1xmmc_card_table[host->id].status;
> >+
> >+#if defined(CONFIG_MIPS_DB1200)
> >+	data = bcsr->sig_status & val;
> >+#endif
> >+
> >+	return !!data;
> > }
> 
>    Hrm, are you sure there's no way to sense that the card is *really* 
> inserted or not?

Again as above. For my specific board I use:

   #if defined(CONFIG_MIPS_DB1200)
       data = bcsr->sig_status & val;
   #elif defined(CONFIG_MIPS_MYBOARD)
       specific code...
   #endif

Maybe we should modify my solution including other DB1x00 boards
ifdef. However, the important thing is to protect againt variable
"bcsr" if a specific board doesn't support it.

> > static inline int au1xmmc_card_readonly(struct au1xmmc_host *host)
> > {
> >-	return (bcsr->status & au1xmmc_card_table[host->id].wpstatus)
> >-		? 1 : 0;
> >+	u32 val, data = 0;
> >+
> >+	val = au1xmmc_card_table[host->id].wpstatus;
> >+
> >+#if defined(CONFIG_MIPS_DB1200)
> >+	data = bcsr->status & val;
> >+#endif
> >+
> >+	return !!data;
> > }
> 
>    Ditto.

The same as above. :)

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] 2/7 AU1100 MMC support

[Manuel Lauss]

Hi Rodolfo,

On Thu, Aug 10, 2006 at 03:36:58PM +0200, Rodolfo Giometti wrote:
> Again as above. For my specific board I use:
> 
>    #if defined(CONFIG_MIPS_DB1200)
>        data = bcsr->sig_status & val;
>    #elif defined(CONFIG_MIPS_MYBOARD)
>        specific code...
>    #endif
> 
> Maybe we should modify my solution including other DB1x00 boards
> ifdef. However, the important thing is to protect againt variable
> "bcsr" if a specific board doesn't support it.

FWIW, I think the whole Db/Pb board specific mess should be removed
from the driver entirely and placed in the respective boards' code.
The board should then pass on a struct to the driver with hooks
into those functions (like in arch/arm/mach-pxa/corgi.c).

It is on my list of things to fix for a later time. If noone beats me
to it, I'm going to do it, but it may take a while since I'm working
on other things.

Thanks,

-- 
 ml.

Re: [PATCH] 2/7 AU1100 MMC support

[Sergei Shtylyov]

Hello.

Rodolfo Giometti wrote:

>>>static void au1xmmc_set_power(struct au1xmmc_host *host, int state)
>>>{

>>>-	u32 val = au1xmmc_card_table[host->id].bcsrpwr;
>>>+	u32 val;

>>>+	val = au1xmmc_card_table[host->id].power;
>>>+
>>>+#if defined(CONFIG_MIPS_DB1200)
>>>	bcsr->board &= ~val;
>>>	if (state) bcsr->board |= val;
>>>+#endif

>>>	au_sync_delay(1);
>>>}

>>   If DBAu1100 doesn't allow to control slot power, then I don't think 
>>pretending it does is a good thing. Shouldn't these #ifdef's be in 
>>au1xmmc_set_ios() instead (the function is void anyway but that would allow 
>>us to save on the code size a bit more)?

> Mmm. I proposed that solution but I don't know exaclty how several
> DB1x00 boards work. I just protect the variable "bcsr" which is not
> defined for my board.

    That wasn't obvious from the patch that this is intended for some specific 
board and BCSRs (board control/status registers) seem to exist on all Alchemy 
development boards.

>>>static inline int au1xmmc_card_inserted(struct au1xmmc_host *host)
>>>{
>>>-	return (bcsr->sig_status & au1xmmc_card_table[host->id].bcsrstatus)
>>>-		? 1 : 0;
>>>+	u32 val, data = 1;
>>>+
>>>+	val = au1xmmc_card_table[host->id].status;
>>>+
>>>+#if defined(CONFIG_MIPS_DB1200)
>>>+	data = bcsr->sig_status & val;
>>>+#endif
>>>+
>>>+	return !!data;
>>>}

>>   Hrm, are you sure there's no way to sense that the card is *really* 
>>inserted or not?
> 
> 
> Again as above. For my specific board I use:

>    #if defined(CONFIG_MIPS_DB1200)
>        data = bcsr->sig_status & val;
>    #elif defined(CONFIG_MIPS_MYBOARD)
>        specific code...
>    #endif

> Maybe we should modify my solution including other DB1x00 boards
> ifdef. However, the important thing is to protect againt variable
> "bcsr" if a specific board doesn't support it.

    If you're not introducing the support for the new board, introducing those 
#ifdef's doesn't make much sense.
    Overall, the support for the boards other than Pb/DbAu1200 seems broken in 
that driver -- see below why...

>>>static inline int au1xmmc_card_readonly(struct au1xmmc_host *host)
>>>{
>>>-	return (bcsr->status & au1xmmc_card_table[host->id].wpstatus)
>>>-		? 1 : 0;
>>>+	u32 val, data = 0;
>>>+
>>>+	val = au1xmmc_card_table[host->id].wpstatus;
>>>+
>>>+#if defined(CONFIG_MIPS_DB1200)
>>>+	data = bcsr->status & val;
>>>+#endif
>>>+
>>>+	return !!data;
>>>}

>>   Ditto.

> The same as above. :)

    Indeed, some modifications are needed for the other Alchemy boards -- 
there's some discrepancy with the "board" register in particular, for the 
boards older than Pb/DBAu1200 it was called "specific" having much the same 
format and purpose. This is not so however with "sig_status" and "status" 
registers used to sense card's presence and for write protecting. Judging on 
<asm/mach-db1x00/db1x00.h>, "specific" reg. should be used instead. There are 
MMC macros in that file BTW for detecting the cards and applying power to them.

> Ciao,

WBR, Sergei