[PATCH 0/7] au1xmmc updates, #2

[PATCH 0/7] au1xmmc updates, #2

[Manuel Lauss]

Hello,

The following set of patches remove demoboard-specific code from the
au1xmmc.c driver and add new features.

My main motivation was to let boards implement other carddetect schemes,
since on one of my boards the driver-implemented poll timer doesn't work
for some unknown reason.  But this board does have a dedicated carddetect
IRQ and card-present/card-readonly indicators which are incompatible with
the Db1200 implementation.  I also took the opportunity to clean up the
drivers probe() and irq() handlers to make it a "proper" platform device
(patches #3 and #4).

Patch #1 is required to get the driver to build as a module.
Patch #2 is required to be able to load/unload the driver > 16 times.
Patches #5 and #6 implement new features.
Patch #7 does a little codingstyle cleanup, no functional changes.

Change since V1:
- fix a bug in patch #6: SDIO irq should be checked for independently
  from other irq events.
- more trivial cleanups

Db1200 users, please test!  I verified the poll timer works on one of
older boards, however since I don't have Db1200 and Pb1200 boards I'm
not sure whether the driver still works with both SD controllers enabled!

Thanks!
	Manuel Lauss

[PATCH 1/7] Alchemy: export get_au1x00_speed for modules

[Manuel Lauss]

[PATCH 2/7] Alchemy: dbdma: add API to delete custom DDMA device ids

[Manuel Lauss]

[PATCH 3/7] au1xmmc: remove db1x00 board-specific functions from driver

[Manuel Lauss]

[PATCH 4/7] Alchemy: register mmc platform device for db1200/pb1200 boards

[Manuel Lauss]

[PATCH 5/7] au1xmmc: 4 bit transfer mode

[Manuel Lauss]

[PATCH 6/7] au1xmmc: wire up SDIO interrupt

[Manuel Lauss]

[PATCH 7/7] au1xmmc: codingstyle tidying

[Manuel Lauss]

Re: [PATCH 3/7] au1xmmc: remove db1x00 board-specific functions from driver

[Sergei Shtylyov]

Hello.

Manuel Lauss wrote:
> From 47ecf116465ed850d2202880f7795fcee4826184 Mon Sep 17 00:00:00 2001
> From: Manuel Lauss <mlau@msc-ge.com>
> Date: Wed, 7 May 2008 14:57:01 +0200
> Subject: [PATCH] au1xmmc: remove db1x00 board-specific functions from driver
>
> Remove the DB1200 board-specific functions (card present, read-only
> methods) and instead add platform data which is passed to the driver.
> This allows for platforms to implement other carddetect schemes
> (e.g. dedicated irq) without having to pollute the driver code.
> The poll timer (used for pb1200) is kept for compatibility.
>
> With the board-specific stuff gone, the driver no longer needs to know
> how many physical controllers the silicon actually has; every device
> can be registered as needed, update the code to reflect that.
>
> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
>   
[...]
> diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
> index cc5f7bc..8660f86 100644
> --- a/drivers/mmc/host/au1xmmc.c
> +++ b/drivers/mmc/host/au1xmmc.c
> @@ -49,7 +49,6 @@
>  #include <asm/mach-au1x00/au1100_mmc.h>
>  
>  #include <au1xxx.h>
> -#include "au1xmmc.h"
>   
   I think you should merge the header to the driver in a separate patch.
> @@ -174,8 +221,6 @@ static void au1xmmc_finish_request(struct au1xmmc_host *host)
>  
>  	host->status = HOST_S_IDLE;
>  
> -	bcsr->disk_leds |= (1 << 8);
> -
>   
   So, the LED support is gone with your patch? You should at least 
document this...
>  	mmc_request_done(host->mmc, mrq);
>  }
>  
> @@ -663,7 +708,9 @@ static void au1xmmc_request(struct mmc_host* mmc, struct mmc_request* mrq)
>  	host->mrq = mrq;
>  	host->status = HOST_S_CMD;
>  
> -	bcsr->disk_leds &= ~(1 << 8);
> +	au_writel(0, HOST_STATUS(host));
> +	au_sync();
> +	FLUSH_FIFO(host);
>   
   Hm, not an obvious change...
>  
>  	if (mrq->data) {
>  		FLUSH_FIFO(host);
> @@ -749,118 +796,87 @@ static void au1xmmc_dma_callback(int irq, void *dev_id)
>  
>  static irqreturn_t au1xmmc_irq(int irq, void *dev_id)
>  {
> -
> +	struct au1xmmc_host *host = dev_id;
>  	u32 status;
> -	int i, ret = 0;
>  
> -	disable_irq(AU1100_SD_IRQ);
> +	status = au_readl(HOST_STATUS(host));
>  
> -	for(i = 0; i < AU1XMMC_CONTROLLER_COUNT; i++) {
> -		struct au1xmmc_host * host = au1xmmc_hosts[i];
> -		u32 handled = 1;
> +	if (!(status & (1 << 15)))
>   
   Why not SD_STATUS_I?
> +		//IRQ_OFF(host, SD_CONFIG_TH|SD_CONFIG_RA|SD_CONFIG_RF);
>   
   No C99 // comments please -- checkpatch.pl would have given you error 
about them...
> +	} else if (status & 0x203FBC70) {
>   
   I think the mask should be changed to 0x203F3C70 since you're 
handling SD_STATUS_I but maybe I'm wrong...
> -static void au1xmmc_init_dma(struct au1xmmc_host *host)
> +static int au1xmmc_init_dma(struct au1xmmc_host *host)
>   
   I'd have called it au1xmmc_init_dbdma() instead since in Au1100 the 
controller works with its "old-style" DMA... though maybe the difference 
could be handled within this function via #ifdef...
> @@ -878,116 +896,201 @@ static const struct mmc_host_ops au1xmmc_ops = {
>  	.get_ro		= au1xmmc_card_readonly,
>  };
>  
> -static int __devinit au1xmmc_probe(struct platform_device *pdev)
> +static void au1xmmc_poll_event(unsigned long arg)
>  {
> +	struct au1xmmc_host *host = (struct au1xmmc_host *)arg;
>   
   Don't need new line here...
>  
> -	int i, ret = 0;
> +	int card = au1xmmc_card_inserted(host);
> +        int controller = (host->flags & HOST_F_ACTIVE) ? 1 : 0;
>   
   Remove extra space please. And what does this variable actually mean?
> +	host->iobase = (unsigned long)ioremap(r->start, 0xff);
>   
   You have the r->end specifying the resource end, why 0xff (well, 
actually 0x3c is enough)
> @@ -1004,21 +1107,32 @@ static struct platform_driver au1xmmc_driver = {
>  
>  static int __init au1xmmc_init(void)
>  {
> +	if (dma) {
> +		/* DSCR_CMD0_ALWAYS has a stride of 32 bits, we need a stride
> +		 * of 8 bits.  And since devices are shared, we need to create
> +		 * our own to avoid freaking out other devices
>   
   Missing period at end of statement.
> +		 */
> +		if (!memid)
>   
   Hm, is there a chance that it won't be NULL?
> +			memid = au1xxx_ddma_add_device(&au1xmmc_mem_dbdev);
> +		if (!memid) {
> +			printk(KERN_ERR "au1xmmc: cannot add memory dma dev\n");
> +			return -ENODEV;
> +		}
> +	}
>  	return platform_driver_register(&au1xmmc_driver);
>  }
[...]
> diff --git a/include/asm-mips/mach-au1x00/au1100_mmc.h b/include/asm-mips/mach-au1x00/au1100_mmc.h
> index 9e0028f..6474fac 100644
> --- a/include/asm-mips/mach-au1x00/au1100_mmc.h
> +++ b/include/asm-mips/mach-au1x00/au1100_mmc.h
> @@ -38,15 +38,46 @@
>  #ifndef __ASM_AU1100_MMC_H
>  #define __ASM_AU1100_MMC_H
>  
>   
[...]
> +struct au1xmmc_platdata {
>   
   I'd suggest au1xmmc_platform_data.
> +	int(*cd_setup)(void *mmc_host, int on);
> +	int(*card_inserted)(void *mmc_host);
> +	int(*card_readonly)(void *mmc_host);
> +	void(*set_power)(void *mmc_host, int state);
> +};
> +
> +struct au1xmmc_host {
> +	struct mmc_host *mmc;
> +	struct mmc_request *mrq;
> +
> +	u32 id;
> +
> +	u32 flags;
> +	u32 iobase;
> +	u32 clock;
> +
> +	int status;
> +
> +	struct {
> +		int len;
> +		int dir;
> +		u32 tx_chan;
> +		u32 rx_chan;
> +	} dma;
> +
> +	struct {
> +		int index;
> +		int offset;
> +		int len;
> +	} pio;
> +
> +	struct timer_list timer;
> +	struct tasklet_struct finish_task;
> +	struct tasklet_struct data_task;
> +
> +	struct platform_device *pdev;
> +	struct au1xmmc_platdata *platdata;
> +	int irq;
> +};
   Hm, do you need the above structure to be visible from the platform code?

WBR, Sergei

Re: [PATCH 3/7] au1xmmc: remove db1x00 board-specific functions from driver

[Sergei Shtylyov]

Hello.

Manuel Lauss wrote:
> Remove the DB1200 board-specific functions (card present, read-only
> methods) and instead add platform data which is passed to the driver.
> This allows for platforms to implement other carddetect schemes
> (e.g. dedicated irq) without having to pollute the driver code.
> The poll timer (used for pb1200) is kept for compatibility.
>
> With the board-specific stuff gone, the driver no longer needs to know
> how many physical controllers the silicon actually has; every device
> can be registered as needed, update the code to reflect that.
>
> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
[...]
> diff --git a/drivers/mmc/host/au1xmmc.c b/drivers/mmc/host/au1xmmc.c
> index cc5f7bc..8660f86 100644
> --- a/drivers/mmc/host/au1xmmc.c
> +++ b/drivers/mmc/host/au1xmmc.c
[...]
> +static int __devinit au1xmmc_probe(struct platform_device *pdev)
> +{
> +	struct mmc_host *mmc;
> +	struct au1xmmc_host *host;
> +	struct resource *r;
> +	int ret;
> +
> +	mmc = mmc_alloc_host(sizeof(struct au1xmmc_host), &pdev->dev);
> +	if (!mmc) {
> +		dev_err(&pdev->dev, "no memory for mmc host\n");
> +		ret = -ENOMEM;
> +		goto out0;
> +	}
>  
> -		if (!mmc) {
> -			printk(DRIVER_NAME "ERROR: no mem for host %d\n", i);
> -			au1xmmc_hosts[i] = 0;
> -			continue;
> -		}
> +	host = mmc_priv(mmc);
> +	host->mmc = mmc;
> +	host->platdata = pdev->dev.platform_data;
> +	host->pdev = pdev;
>  
> -		mmc->ops = &au1xmmc_ops;
> +	ret = -ENODEV;
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!r) {
> +		dev_err(&pdev->dev, "no mmio defined\n");
> +		goto out1;
> +	}
>   

   I forgot to mention that the driver should be calling 
request_mem_region() here...

WBR, Sergei

Re: [PATCH 3/7] au1xmmc: remove db1x00 board-specific functions from driver

[Manuel Lauss]

Hello Sergei,

>>  #include <asm/mach-au1x00/au1100_mmc.h>
>>   #include <au1xxx.h>
>> -#include "au1xmmc.h"
>>   
>   I think you should merge the header to the driver in a separate patch.

Okay...


>> @@ -174,8 +221,6 @@ static void au1xmmc_finish_request(struct au1xmmc_host 
>> *host)
>>   	host->status = HOST_S_IDLE;
>>  -	bcsr->disk_leds |= (1 << 8);
>> -
>>   
>   So, the LED support is gone with your patch? You should at least document 
> this...

Okay...


>>  	mmc_request_done(host->mmc, mrq);
>>  }
>>  @@ -663,7 +708,9 @@ static void au1xmmc_request(struct mmc_host* mmc, 
>> struct mmc_request* mrq)
>>  	host->mrq = mrq;
>>  	host->status = HOST_S_CMD;
>>  -	bcsr->disk_leds &= ~(1 << 8);
>> +	au_writel(0, HOST_STATUS(host));
>> +	au_sync();
>> +	FLUSH_FIFO(host);
>>   
>   Hm, not an obvious change...

Gone (leftovers from debugging MMC and other non-working cards)


>>   	if (mrq->data) {
>>  		FLUSH_FIFO(host);
>> @@ -749,118 +796,87 @@ static void au1xmmc_dma_callback(int irq, void 
>> *dev_id)
>>   static irqreturn_t au1xmmc_irq(int irq, void *dev_id)
>>  {
>> -
>> +	struct au1xmmc_host *host = dev_id;
>>  	u32 status;
>> -	int i, ret = 0;
>>  -	disable_irq(AU1100_SD_IRQ);
>> +	status = au_readl(HOST_STATUS(host));
>>  -	for(i = 0; i < AU1XMMC_CONTROLLER_COUNT; i++) {
>> -		struct au1xmmc_host * host = au1xmmc_hosts[i];
>> -		u32 handled = 1;
>> +	if (!(status & (1 << 15)))
>>   
>   Why not SD_STATUS_I?

Good point, I'll add named constants for other values as well.


>> +		//IRQ_OFF(host, SD_CONFIG_TH|SD_CONFIG_RA|SD_CONFIG_RF);
>>   
>   No C99 // comments please -- checkpatch.pl would have given you error 
> about them...

It did-- but since I was just shuffling code around I was hesitant to do
these changes as well in this particular patch (the i2c maintainer for
instance wants functional and cosmetic changes in separate patches;
and every maintainer has different preferences, so I left those lines
untouched to not add additional noise).


>> +	} else if (status & 0x203FBC70) {
>>   
>   I think the mask should be changed to 0x203F3C70 since you're handling 
> SD_STATUS_I but maybe I'm wrong...

Yes you're right (but this line never triggered while testing so it's
harmless).


>> -static void au1xmmc_init_dma(struct au1xmmc_host *host)
>> +static int au1xmmc_init_dma(struct au1xmmc_host *host)
>>   
>   I'd have called it au1xmmc_init_dbdma() instead since in Au1100 the 
> controller works with its "old-style" DMA... though maybe the difference 
> could be handled within this function via #ifdef...

I like the renamed function, but again, I was just shuffling code around
(more or less) so I didn't touch the name(s).
For the time being, I'll leave it as-is, and if someday Au1100 DMA is added
the functions can be renamed or beautified with tons of ifdefs.
What do you think?


>> @@ -878,116 +896,201 @@ static const struct mmc_host_ops au1xmmc_ops = {
>>  	.get_ro		= au1xmmc_card_readonly,
>>  };
>>  -static int __devinit au1xmmc_probe(struct platform_device *pdev)
>> +static void au1xmmc_poll_event(unsigned long arg)
>>  {
>> +	struct au1xmmc_host *host = (struct au1xmmc_host *)arg;
>>   
>   Don't need new line here...

Okay...


>>  -	int i, ret = 0;
>> +	int card = au1xmmc_card_inserted(host);
>> +        int controller = (host->flags & HOST_F_ACTIVE) ? 1 : 0;
>>   
>   Remove extra space please. And what does this variable actually mean?

Based on HOST_F_ACTIVE the driver determines if it is possible that there's
a card in the socket.  Again, I just did code shuffling here. 


>> +	host->iobase = (unsigned long)ioremap(r->start, 0xff);
>>   
>   You have the r->end specifying the resource end, why 0xff (well, actually 
> 0x3c is enough)

Okay...


>> @@ -1004,21 +1107,32 @@ static struct platform_driver au1xmmc_driver = {
>>   static int __init au1xmmc_init(void)
>>  {
>> +	if (dma) {
>> +		/* DSCR_CMD0_ALWAYS has a stride of 32 bits, we need a stride
>> +		 * of 8 bits.  And since devices are shared, we need to create
>> +		 * our own to avoid freaking out other devices
>>   
>   Missing period at end of statement.

Okay...


>> +		 */
>> +		if (!memid)
>>   
>   Hm, is there a chance that it won't be NULL?

Are global vars initialized to zero on module load?  Then it can go away of
course.


>> +			memid = au1xxx_ddma_add_device(&au1xmmc_mem_dbdev);
>> +		if (!memid) {
>> +			printk(KERN_ERR "au1xmmc: cannot add memory dma dev\n");
>> +			return -ENODEV;
>> +		}
>> +	}
>>  	return platform_driver_register(&au1xmmc_driver);
>>  }
> [...]
>> diff --git a/include/asm-mips/mach-au1x00/au1100_mmc.h 
>> b/include/asm-mips/mach-au1x00/au1100_mmc.h
>> index 9e0028f..6474fac 100644
>> --- a/include/asm-mips/mach-au1x00/au1100_mmc.h
>> +++ b/include/asm-mips/mach-au1x00/au1100_mmc.h
>> @@ -38,15 +38,46 @@
>>  #ifndef __ASM_AU1100_MMC_H
>>  #define __ASM_AU1100_MMC_H
>>    
> [...]
>> +struct au1xmmc_platdata {
>>   
>   I'd suggest au1xmmc_platform_data.

Too much to type for my taste, but okay, changed.


>> +	int(*cd_setup)(void *mmc_host, int on);
>> +	int(*card_inserted)(void *mmc_host);
>> +	int(*card_readonly)(void *mmc_host);
>> +	void(*set_power)(void *mmc_host, int state);
>> +};
>> +
>> +struct au1xmmc_host {
>> +	struct mmc_host *mmc;
>> +	struct mmc_request *mrq;
>> +
>> +	u32 id;
>> +
>> +	u32 flags;
>> +	u32 iobase;
>> +	u32 clock;
>> +
>> +	int status;
>> +
>> +	struct {
>> +		int len;
>> +		int dir;
>> +		u32 tx_chan;
>> +		u32 rx_chan;
>> +	} dma;
>> +
>> +	struct {
>> +		int index;
>> +		int offset;
>> +		int len;
>> +	} pio;
>> +
>> +	struct timer_list timer;
>> +	struct tasklet_struct finish_task;
>> +	struct tasklet_struct data_task;
>> +
>> +	struct platform_device *pdev;
>> +	struct au1xmmc_platdata *platdata;
>> +	int irq;
>> +};
>   Hm, do you need the above structure to be visible from the platform code?

The db1200 board stuff references the ->id member to determine which BCSR
bits it should pay attention to.  It can go into the driver code if you are
okay with adding platform data for every one of both SD controllers on the
PB1200 (which means lots of duplicated code which only differs in BCSR
constants).  I'm okay with eiher solution, which do you prefer?


> WBR, Sergei

Thanks for having a look!
	Manuel Lauss