WMT SD/MMC Driver
Tony Prisk
read/write driver for the SD/MMC controller working.
Turns out the problem wasn't with my understanding of the code, but
the way that linux decodes the command type.
I have had to manually configure the cmdtype variable to get the
correct results.
NOTES:
1) It operates in polled mode which makes it a little CPU heavy, but
it can be task-switched so it doesn't bog down the system.
2) Reading and writing now works. I have only tried it on a single
FAT16 partitioned card, but I can't see any reason why it wouldn't
work on all setups.
3) It only implements read-single and write-single so the sub-system
overhead is a bit high.
4) It is untested on VT8500-based boards. Feedback welcome. I suspect
that the DMA controller layout is different, which will cause it to
hang when it gets to the first data command. Unremark all the 'printk'
lines before compiling, otherwise you may not see any output.
Arnd Bergmann
> After a few rather frustrating days, I have finally managed to get a
> read/write driver for the SD/MMC controller working.
> Turns out the problem wasn't with my understanding of the code, but
> the way that linux decodes the command type.
> I have had to manually configure the cmdtype variable to get the
> correct results.
Very nice to see you made this much progress on this!
I'm replying to the code with comments in the way that you would
get in a formal review on the linux-arm-kernel and mmc mailing
lists, to speed up the process of getting the driver included.
I hope this helps you, feel free to ask me when you have further
questions.
Obviously there are still quite a few things to be done, and I suppose
you are aware of most of them. First of all, the submission
format -- it should be sent to the lists as an inline patch with
a description and Signed-off-by line from you, see
Documentation/SubmittingPatches.
> // Driver module name - required for requesting platform_device resources
> #define DRIVER_NAME "wmt-sdmmc"
The kernel generally does not use C++ style comments, it should be
either
/* one-line comment */
or
/*
* multi-line comment */
*/
The particular comment for the DRIVER_NAME is rather useless though,
you could just as well remove it, or even remove the macro and open-code
the string in the two places where it is used.
> volatile int dma_completed;
Volatile does not mean what you want it to mean. If you want to communicate
atomically, you should use one of spinlock, atomic_t or bitops.
> struct wmt_dma_descriptor {
> u32 reqcount:16;
> u32 interrupt:1;
> u32 reserved0:13;
> u32 format:1;
> u32 end:1;
Bit fields are not particularly portable. It would be better to use a single
u32 (or two u16) here and mask the bits manually/
> u32 DataBufferAddr;
> u32 BranchAddr;
> u32 reserved1;
CaMelCase is against the kernel coding style, the more common way would
be to write "data_buffer_addr", or just "buffer" here.
> struct wmt_mci_priv {
> struct mmc_host *mmc;
> void __iomem *sdmmc_base;
>
> int irq_regular;
> int irq_dma;
>
> // dma buffers
> void *dma_data_buffer;
> u32 dma_data_cpuaddr;
>
> void *dma_desc_buffer;
> u32 dma_desc_cpuaddr;
>
> // sd data
> struct mmc_request *req;
> };
It's rather unusual to have pointers to the request in the same
data structure as the host information. You normally want to
keep the host information global and have pointers to that
from the request where needed.
> static irqreturn_t wmt_mci_dma_isr(int irq_num, void *data)
> {
> struct wmt_mci_priv *priv;
>
> // printk("<1>[MMC/SD] DMA ISR FIRED!!\n");
You should never pass literal "<1>" strings or similar to
printk. Instead, use the KERN_DEBUG etc macros. Even better,
use dev_dbg() or pr_debug() so you can decide at run-time
whether the messages make it to printk.
> priv = (struct wmt_mci_priv *)data;
>
> // disable the SD DMA controller
> writel(0, priv->sdmmc_base + SDDMA_DESPR);
> writel(0, priv->sdmmc_base + SDDMA_GCR);
>
> dma_completed = 1;
>
> return IRQ_HANDLED;
> }
The use of the dma_completed variable is wrong for a number
of reasons here.
One solution would be to instead use complete() in the ISR
and wait_for_completion() in the request function.
> static irqreturn_t wmt_mci_regular_isr(int irq_num, void *data)
> {
> // This function should never be called!
> // printk("<1>[MMC/SD] REGULAR ISR FIRED!!\n");
> return IRQ_HANDLED;
> }
Can you explain why it should not get called?
> // This code is hacky and should be fixed
> // Hardcoded GPIO bases should be passed from board definition
> // This code enables the GPIO pin that power the SD controller - could/should be done with GPIO functions
> // Also configures some general purpose pins for MMC/SD functions
right.
> // wait for command to send
> while ((readb(priv->sdmmc_base + SDMMC_CTLR) & CTLR_CMD_START) != 0);
>
> // wait for command to complete
> while ((readb(priv->sdmmc_base + SDMMC_STS1) & STS1_RSP_TIMEOUT) != STS1_RSP_TIMEOUT) {
> if (( readb(priv->sdmmc_base + SDMMC_STS1) & STS1_CMDRSP_DONE) == STS1_CMDRSP_DONE)
> break;
> }
How long do these take? For really long times, it would be good to sleep
inbetween, for short times, you should have a maximum time to block for.
Use something like
unsigned long timeout = jiffies + (TIMEOUT / HZ);
do {
...
} while (time_before(jiffies, timeout);
> if (priv->req->data->flags & MMC_DATA_READ) {
> // if we recieved data, copy it to the mmc scatterlist
> local_irq_save(flags);
Why the local_irq_save?
> dma_sync_sg_for_cpu(mmc_dev(mmc), priv->req->data->sg, priv->req->data->sg_len, DMA_FROM_DEVICE);
>
> buffer = (u8 *)priv->dma_data_buffer;
> dma_map_sg(mmc_dev(mmc), priv->req->data->sg, priv->req->data->sg_len, DMA_FROM_DEVICE);
> addr = (u8 *)(sg_dma_address(priv->req->data->sg));
> vaddr = kmap_atomic(sg_page(priv->req->data->sg), KM_BIO_SRC_IRQ) + priv->req->data->sg->offset;
>
> for (cnt = 0; cnt < sg_dma_len(priv->req->data->sg); cnt++)
> vaddr[cnt] = buffer[cnt];
>
> priv->req->data->bytes_xfered = priv->req->data->blksz;
>
> kunmap_atomic(vaddr, KM_BIO_SRC_IRQ);
> dma_unmap_sg(mmc_dev(mmc), priv->req->data->sg, priv->req->data->sg_len, DMA_FROM_DEVICE);
>
> local_irq_restore(flags);
There is some confusion in this code. You don't need dma_sync_sg_for_cpu
here, in fact it's invalid to call dma_sync_sg_for_cpu before mapping
the sglist.
The idea of the dma_map_sg is to get a list of entries that you can
pass to the device directly, so you don't need to copy the data
around manually. Note that priv->req->data->sg is an data structue,
not a single value, you need to walk through it using for_each_sg().
You should not need the kmap_atomic here, in fact it is quite harmful
to have it because you are mapping the same page that is still owned
by the device.
> if (!pdev) {
> printk("<1>[MMC/SD] Probe failed!\n");
> return -EINVAL;
> }
Not possible here, the probe function only gets called with a valid pdev.
Arnd
Tony Prisk
1) Thank you for the tip regarding comments, I wasn't aware of this -
first time I have coded anything for linux.
2) I will correct the bitfields in the structure for the DMA
descriptor - I originally thought this looked wrong, but didn't want
to change it before I had it working.
3) All the 'printk("<1>...' comments were for my benefit while
debugging. None of them need to be included in a submitted driver - it
was so I could monitor registers/data when the driver was non-
functional.
4) CaMelCase variables will also be corrected.
5) The dma_completed variable was originally intended to indicate that
the DMA interrupt was firing so I confirm whether another variable
should have been changing but wasn't. It is a left over from debugging
and will be removed. It isn't actually required as some of the code
which is currently 'polled' will be moved into the DMA handler.
6) The 'regular_isr' should never be called because the SD controller
is currently operating in Polled mode, so no interrupts are generated.
This is a 'too-be-done' still.
7) The 'while' loops you questioned will be written out once the
interrupt handlers are completed.
8) Thanks for the pointers regarding the last code block. To be
honest, I don't completely understand this code and it was mostly
copied from other driver source. The dma_map_sg is another left-over
piece of code that should have been removed. The reason is currently
doesn't walk the scatterlist is that the list driver is currently
limited to single-block requests only, and the mmc host driver seems
to always passes single-entry sg at the moment. kmap_atomic is, again,
another piece of code that was copied without understanding.
I would appreciate any comments you might have about correcting this
last block of code.
It's basic function is to copy the data from the dma buffer to the
scatterlist. There is an accompanying piece of code which performs the
same copy in the other direction.
Is it possibly to copy data from the scatterlist without mapping?
Would it be better to map the scatterlist and pass the bus_addr of the
mapped sg to the descriptor rather than copying it to my own buffer?
Thanks for your comments.
Sentient
1) Performance is dramatically improved (but still below what I expect).
The previous driver benchmarked around 77Kb/sec (write) on my setup.
This new driver benchmarked around 200Kb/sec (write) on the same setup.
This is mostly due to expanding the code for the PLL clock (now operates up
to 25Mhz).
Added support for card insert detection and write protect detection (haven't
checked the WP yet). Card Insert was required otherwise when the card was
removed the driver would go 'wonky' :)
2) I have fixed the source as per Arnd's notes.
All comments are now the correct style - I have removed some of the more
obvious ones.
Bitfields in the wmt_dma_descriptor struct are replaced with a u32 field and
masked.
CaMelCase has been removed (might still be a bit but I've removed what I've
noticed so far)
Removed the '<1>' literals and replaced with KERN_DEBUG, KERN_ERR where
appropriate
Removed the dma_completed variable and setup a completion struct.
TODO:
Remove the blocking code in the post_command function. Should be movable to
regular_isr() with a bit of work so its non-blocking.
Removing the mmc_request variable from the wmt_mci_priv struct - Not too
sure how I am going to accomplish this (feedback welcomed)
Fix up the data copy from DMA buffers to scatterlists (and vice-versa).
Implement multi-block requests.
Ed Spiridonov
> 1) It operates in polled mode which makes it a little CPU heavy, but
> it can be task-switched so it doesn't bog down the system.
It is temporary solution?
Tony Prisk
as crashes in the interrupt handler tended to crash the kernel.
The newer source is only partially polled - the DMA transfer is now
interrupt driven.
When I get some more time, I'll move the post_command functions to the
regular_isr handler, and it'll be back in interrupt-driven mode.
To be honest, there isn't a huge difference in performance.
Arnd Bergmann
> 8) Thanks for the pointers regarding the last code block. To be
> honest, I don't completely understand this code and it was mostly
> copied from other driver source. The dma_map_sg is another left-over
> piece of code that should have been removed. The reason is currently
> doesn't walk the scatterlist is that the list driver is currently
> limited to single-block requests only, and the mmc host driver seems
> to always passes single-entry sg at the moment. kmap_atomic is, again,
> another piece of code that was copied without understanding.
The idea of kmap_atomic is to access a user space page that may or may
not be in highmem. It creates a new page table entry so you can access
a page through a pointer.
> I would appreciate any comments you might have about correcting this
> last block of code.
> It's basic function is to copy the data from the dma buffer to the
> scatterlist. There is an accompanying piece of code which performs the
> same copy in the other direction.
>
> Is it possibly to copy data from the scatterlist without mapping?
> Would it be better to map the scatterlist and pass the bus_addr of the
> mapped sg to the descriptor rather than copying it to my own buffer?
That is indeed the intention of a scatterlist. The kernel uses scatterlists
in order to pass around pages to driver to do DMA in. After you have
done dma_map_sg, you are supposed to use use for_each_sg(), sg_dma_len()
and sg_dma_address() to access the bus addresses that you pass to the
device. Most DMA engines can operate on some kind of scatter-gather
list, so you fill that list with the data from sg_dma_address and
pass the list to the device. There is probably a way to tell the MMC
layer if the hardware cannot deal with these lists and needs to
get contiguous buffers.
Arnd
Arnd Bergmann
> Improved source code
>
> 1) Performance is dramatically improved (but still below what I expect).
> The previous driver benchmarked around 77Kb/sec (write) on my setup.
> This new driver benchmarked around 200Kb/sec (write) on the same setup.
> This is mostly due to expanding the code for the PLL clock (now operates up
> to 25Mhz).
This may well be because of the single-block access. Most SD cards can only
write blocks of multiples of 16 KB or larger, if you write smaller data,
you can easily get to the worst-case scenario of having the card write
4 MB for every 512 bytes you send it. In fact, many high-speed class 10
cards will have this behaviour, while slower class 2 or 4 cards will deal
a lot better with small accesses.
> 2) I have fixed the source as per Arnd's notes.
> All comments are now the correct style - I have removed some of the more
> obvious ones.
> Bitfields in the wmt_dma_descriptor struct are replaced with a u32 field and
> masked.
> CaMelCase has been removed (might still be a bit but I've removed what I've
> noticed so far)
> Removed the '<1>' literals and replaced with KERN_DEBUG, KERN_ERR where
> appropriate
> Removed the dma_completed variable and setup a completion struct.
very nice.
> TODO:
> Remove the blocking code in the post_command function. Should be movable to
> regular_isr() with a bit of work so its non-blocking.
> Removing the mmc_request variable from the wmt_mci_priv struct - Not too
> sure how I am going to accomplish this (feedback welcomed)
> Fix up the data copy from DMA buffers to scatterlists (and vice-versa).
> Implement multi-block requests.
sounds good.
> /*
> * release the dma buffers
> * TODO: fix these lines - currently crashes with an address invalid
> * dma_free_coherent(&pdev->dev, 2048, &priv->dma_data_cpuaddr, (u32)priv->dma_data_buffer);
> * dma_free_coherent(&pdev->dev, 512, &priv->dma_desc_cpuaddr, (u32)priv->dma_desc_buffer);
> */
You have mixed up the arguments in these, it should be:
dma_free_coherent(&pdev->dev, 2048, priv->dma_data_buffer, priv->dma_data_cpuaddr);
dma_free_coherent(&pdev->dev, 512, priv->dma_desc_buffer, priv->dma_desc_cpuaddr);
Note that what you call the 'buffer' is usually called the 'cpu address', it
is a pointer that the CPU can use. In turn, what you call the 'cpuaddr' is
normally called the 'dma address', it's the address that the device uses to
access the buffer, and should be of type dma_addr_t. When you use the correct
types and names, a lot of the confusion will go away.
Arnd
Sentient
NOTE: There are no new features or noticeable speed improvements in this
code.
1) Removed the intermediate buffer for DMA transfers - now copies from the
scatterlist to device and vice-versa. This save's on copying the data from
one buffer to another and simplify's the DMA setup code. Removed the
dma_data_buffer variable.
2) Fixed the memory leak with the descriptor DMA buffer not being released -
thanks Arnd. (Also changed the 'u32 dma_desc_cpuaddr' to 'dma_addr_t
dma_desc_device_addr')
TODO:
1) Remove the blocking code in post_command.
2) Implement multi-block transfers. I have done a bit of work on this, but
with no luck. Invalid data/kernel crashes :)
Arnd:
How would you recommend removing the mmc_request variable from the
wmt_mci_priv structure?
It is required in the interrupt routines, and the priv structure is the only
structure passed.
Ed Spiridonov
> On Sunday 23 January 2011, Sentient wrote:
> This may well be because of the single-block access. Most SD cards can only
> write blocks of multiples of 16 KB or larger, if you write smaller data,
> you can easily get to the worst-case scenario of having the card write
> 4 MB for every 512 bytes you send it. In fact, many high-speed class 10
> cards will have this behaviour, while slower class 2 or 4 cards will deal
> a lot better with small accesses.
sorry by the offtopic, but where I can read more about this?
Sentient
1) Implemented multi-block read/write (currently still limited to 16 blocks
due to a scatterlist mapping problem).
2) Write speed is improved to about 685KB/s (about 330% faster).
WARNING! There is a known fault in the scatterlist mapping which may result
in data loss (either when reading/writing). It is currently set to
max_blocks=16 which hasn't caused any issues for me in about 2 hours use,
but be warned!
TODO:
1) Check the STOP_TRANSMISSION code - I think something is passed in the
req->data->stop that relates to this but at the moment its hardcoded.
2) Fix the scatterlist mapping code. Something is wrong with it which might
cause problems (BE WARNED!) If the max_blocks is set to >16, then the
scatter mapping seems to allocate insufficient descriptors to cover the
requested blocks (eg. 20 blocks requested but only 12 descriptors allocated
- causes a kernel lockup).
3) Still have to remove the blocking code in post_command()
Sentient
1) Fixed the scatterlist mapping problem. Max_blocks=128 is now the default
(64k/request on FAT filesystem). No reason why it can't be higher except
that it would require more DMA memory. No point going higher than
max_blocks=2048 (fat is 512 bytes/block, 2048 blocks = 1Mb, max_request is
also 1Mb).
2) Write speed is improved again - getting about 780-800Kb/s write on my
card (I need to find another card as this one doesn't even have a class
rating on it :) )
This driver is definitely useable for those that don't want to wait :)
Also, I'd be interested to hear what kind of speeds people get with the
following commands:
'mount /dev/mmcblk0 /mnt/sd -o sync'
'time dd if=/dev/zero of=/mnt/sd/rubbish bs=1M count=5'
Time and Kb/s are of interest (especially if you have a class 4/6/10 card to
test on).
Arnd Bergmann
I'm still doing the research, but my findings so far are on
https://wiki.linaro.org/WorkingGroups/KernelConsolidation/Projects/FlashCardSurvey
Arnd
ar...@arndb.de
> Also, I'd be interested to hear what kind of speeds people get with the
> following commands:
>
> 'mount /dev/mmcblk0 /mnt/sd -o sync'
> 'time dd if=/dev/zero of=/mnt/sd/rubbish bs=1M count=5'
>
Note that performance values are highly unreliable if you do that,
because if the interaction of ext3 with the layout of the card,
especially if you mount with -o sync.
Better write to the raw card:
time dd if=/dev/zero of=/dev/mmcblk0 bs=1M count=5 conv=direct seek=16
Skipping the first 16 MB will make it not write to the FAT area,
which is often slower than the rest (while allowing random access).
Arnd
Tony Prisk
and an adapter and done some more testing:
All tests performed with max_block = 128 (giving a max_request = 64K)
on a FAT partitioned card.
2GB No-name No-class Standard SD card - max 750KB/s
4GB Lexar Class 2 Micro-SDHC w/ Adapter - max 2.2MB/s
4GB Adata Class 6 Micro-SDHC w/ Adapter - max 2.4MB/s
All tests performed with max_block = 512 (giving a max_request = 256K)
on a FAT partitioned card.
2GB No-name No-class Standard SD card - max 750KB/s
4GB Lexar Class 2 Micro-SDHC w/ Adapter - max 2.3MB/s
4GB Adata Class 6 Micro-SDHC w/ Adapter - max 2.4MB/s
Both of the Micro-SDHC cards are reported as High-Speed, but the
driver is currently limited to 25Mhz. Not sure if this is related -
will need to check.
Seems as though the driver is maxing out my no-name card, and the
transfer rate is maxed out on the two higher speed cards (although
class 2 is rated at 2Mb least-sustained write).
Ed Spiridonov
> With regards to performance, I have dug up some a few SD-micro cards
> and an adapter and done some more testing:
do you compare performance with binary kernel from VIA/WMT?
Arnd Bergmann
> time dd if=/dev/zero of=/dev/mmcblk0 bs=1M count=5 conv=direct seek=16
>
> Skipping the first 16 MB will make it not write to the FAT area,
> which is often slower than the rest (while allowing random access).
Actually, count and size combined also should be a multiple of 4MB.
Try it multiple times though, some cards need to get into the right
mood before they get fast.
It would be good to compare the same cards doing this using a
USB or sdhci reader on a PC, your driver, and the original VIA
driver.
Arnd
Tony Prisk
For comparison with the VIA binary driver I have run the same tests.
2GB No-name No-class Standard SD card - max 720KB/s
4GB Lexar Class 2 Micro-SDHC w/ Adapter - max 2.0MB/s
4GB Adata Class 6 Micro-SDHC w/ Adapter - max 2.2MB/s
I also tried configuring the 50Mhz clock for my SDHC class-6 card, but
it has made no apparent difference in speed, even with max_block=2048
(req_size=1M)
Bjoern
> This driver is definitely useable for those that don't want to wait :)
running my debian rootfs from a 4GB SDHC now. No error so far.
Since my rootfs is on the card, I ran the test on the mounted ext3
partition (-o sync).
wm8505:~# time dd if=/dev/zero of=/rubbish bs=1M count=5
5+0 records in
5+0 records out
5242880 bytes (5.2 MB) copied, 1.23722 s, 4.2 MB/s
Thank you guys. :)
Bjoern
Alexey Charkov
> Updated code:
>
> 1) Fixed the scatterlist mapping problem. Max_blocks=128 is now the default
> (64k/request on FAT filesystem). No reason why it can't be higher except
> that it would require more DMA memory. No point going higher than
> max_blocks=2048 (fat is 512 bytes/block, 2048 blocks = 1Mb, max_request is
> also 1Mb).
> 2) Write speed is improved again - getting about 780-800Kb/s write on my
> card (I need to find another card as this one doesn't even have a class
> rating on it :) )
>
> This driver is definitely useable for those that don't want to wait :)
Great work, Tony!
Would you like me to push this code to Gitorious on your behalf?
Best regards,
Alexey
Tony Prisk
Also, when you get a chance I'd be interested to hear how badly it
doesn't work on the vt8500 :)
I believe it should be a case of changing the DMA code to get it
working, so hopefully not a major effort - although they
troubleshooting will be 'difficult' since I don't have one to test
on :)
Ed Spiridonov
i got much higher values with:
dd if=/dev/mmcblk0 of=/dev/null iflag=direct bs=1M skip=32 (=16 for 32Mb card)
dd if=/dev/zero of=/dev/mmcblk0 oflag=direct bs=1M seek=32 (=16 for 32Mb card)
Transcend SD 150x, 2Gb (SLC):
- your code, reading: 8.0MB/s
- your code, writing: 8.0MB/s
- VIA/WMT kernel, reading: 18.4 MB/s
- VIA/WMT kernel, writing: 12.1 MB/s
- USB cardreader, reading: 21.1 MB/s
- USB cardreader, writing: 14.6 MB/s
Kingston MMC plus, 128Mb:
- your code, reading: 7.6MB/s
- your code, writing: 5.4MB/s
- VIA/WMT kernel, reading: does not work
- VIA/WMT kernel, writing: does not work
- USB cardreader, reading: 11.6 MB/s
- USB cardreader, writing: 10.4 MB/s
old Canon SD, 32Mb:
- your code, reading: 3.4 MB/s
- your code, writing: 1.3 MB/s
- VIA/WMT kernel, reading: 3.6 MB/s
- VIA/WMT kernel, writing: 1.4 MB/s
- USB cardreader, reading: 3.7 MB/s
- USB cardreader, writing: 1.3 MB/s
PS: after some testing I got error on netbook (with your mmc module):
dd: opening `/dev/null': Invalid argument.
I think your code is still buggy and cause memory corrution ;)
Ed Spiridonov
> - your code, reading: 7.6MB/s
> - your code, writing: 5.4MB/s
> - VIA/WMT kernel, reading: does not work
> - VIA/WMT kernel, writing: does not work
BTW this card don't work with u-boot, but work with WinCE.
Tony Prisk
I believe there are two reasons that my code is slower.
1) mmc->max_blk_count = 128 (line 837). This value is too low to get
efficient transfers.
For higher performance, change the value to 512 (or higher), and
change the following lines to match:
line 865: priv->dma_desc_buffer = dma_alloc_coherent(&pdev->dev,
2048 ...
to priv->dma_desc_buffer = dma_alloc_coherent(&pdev->dev, 8192 ...
To calculate the size of the DMA buffer - required_size =
max_blk_count * 16 (eg. max_blk_count=2048 requires
dma_alloc_coherent(.., 32768, ...)
The VIA binary driver uses a max_blk_count=2048 if you want to try
that.
2) Highspeed clocking is not supported in my published code so the SD
card is operating at 25Mhz max. A high speed card can support up to
50Mhz so you might get transfer speed improvements once this is done -
although I believe the VIA binary driver is locked to 25Mhz max
anyway, so we should be able to obtain similar speeds before
increasing the clock.
Tony Prisk
commands.
max_blk_count = 2048 (as per the VIA driver)
I have 50Mhz clock code running as well.
Read speed - 15.1MB/s
Write speed - 10.1MB/s
I haven't tested this card on the VIA binary driver with your test
commands so I don't have a comparison.
Will post my current source so you can try with the new max_blk and
50Mhz clock.
Sentient
1) Max_blks=2048 (same as the VIA binary driver).
2) 50Mhz clock now supported for high-speed cards.
3) Changed the coding for the DMA buffer allocation to be 16*max_blks so we
don't have problems with crashes due to wrong calcs :) Just change the
max_blks and the dma buffer will resize according.
Tony Prisk
Read speed - 12.72MB/s
Write speed - 11.52MB/s
Angus Gratton
> New source code for Ed :)
Hi Tony,
I just wanted to say thanks for all the work you're putting on this.
I haven't had time to test anything yet, sorry. I was going to make one
simple suggestion, though - have you considered forking Alexey's
repository on gitorious?
Then you can just say "I pushed new source to gitorious" instead of
needing to attach the source file each time, and people who want to test
can just do a 'git pull' and rebuild instead of having to save the
source file each time.
(Even if you don't adopt this suggestion, I do really appreciate the
work that you and the others are putting in on this!)
Cheers,
- Angus
ar...@arndb.de
> I haven't tested this card on the VIA binary driver with your test
> commands so I don't have a comparison.
> Will post my current source so you can try with the new max_blk and
> 50Mhz clock.
You can probably get a bit better by using double buffering dma,
which Per Forlin has recently implemented, see
http://lkml.org/lkml/2011/1/12/236
Your driver is starting to look really good now, you should probably
post it to the linu...@vger.kernel.org mailing list, following
the rules from the Documentation/SubmittingPatches file in the
kernel, to get more input. The only thing that still sticks out
is the size of the wmt_mci_request() function, it would be more
readable if you split out the read and write code into separate
functions, or possibly one function that takes the dma direction
as an argument.
There are also a few tiny things that scripts/checkpatch.pl should
warn about like whitespace changes, and making all functions 'static',
and of course the things that you already marked as TODO in your
comments.
Arnd
Ed Spiridonov
> New source code for Ed :)
now performance is reasonably good (current version uses logging
agressive, it can also cause slowdown)
> Transcend SD 150x, 2Gb (SLC):
> - your code, reading: 8.0MB/s
> - your code, writing: 8.0MB/s
> - VIA/WMT kernel, reading: 18.4 MB/s
> - VIA/WMT kernel, writing: 12.1 MB/s
> - USB cardreader, reading: 21.1 MB/s
> - USB cardreader, writing: 14.6 MB/s
new result: 16.0 MB/s read, 10.4 MB/s write.
what you think about this:
> PS: after some testing I got error on netbook (with your mmc module):
> dd: opening `/dev/null': Invalid argument.
>
> I think your code is still buggy and cause memory corrution ;)
I get similar errors twice with previous driver version.
In both cases error appear until reboot.
Arnd Bergmann
> You can probably get a bit better by using double buffering dma,
> which Per Forlin has recently implemented, see
> http://lkml.org/lkml/2011/1/12/236
One more idea: When reading through the sdhci driver, I noticed that
you don't actually have to wait in the request function. Just
move everything you do after wmt_mci_start_command() into the
interrupt handler and call mmc_request_done() when the interrupt
arrives. This will save you a full context switch for every
interrupt.
Arnd
Tony Prisk
Cheers for the feedback regarding /dev/null error - I will look into
it.
I haven't personally seen this problem yet, but will do some more
aggressive testing to see if I can't get some kind of fault to present
itself.
Presumably this occurs during write benchmarks?
Arnd,
Moving that code from the end of the wmt_mci_request() is part of my
TODO list :)
At the moment, there are no regular interrupts generated for
transfers. The only interrupt is for device insert.
The driver waits for the DMA completion (so the data transfer is
completed) and then polls the status register (although I suspect it
actually doesn't wait at all because the DMA is already done) and
completes the request.
Also, I'm not really interested in posting a kernel patch - I figure
once it's at a suitable state, someone is free to do it :)
Either that or Alexey will merge it with his kernel and it will get
submitted upstream eventually.
I did a 'git pull' on Alexey's repo originally and it took all of
about 2 hours for me to wreck my copy :) I'll just leave it in the too
hard basket.
Tony Prisk
I have been unable to replicate the issue you reported with /dev/null.
Could you provide some more details as to what you were doing to cause
it? :)
I have read/written >16GB data to my card's, and haven't had any issue
yet.
As requested, I forked Alexey's repo and the source is now hosted
here:
http://gitorious.org/linux-on-via-vt8500/tprisk-wm8505-sdmmc-dev
Code no longer contains the blocking code in post_command() - actually
post_command() is gone and its now read_response() because that's all
it does now :)
I ran the code through checkpatch.pl and tidied it up so now its all
warnings about >80 characters/line. (no errors)
Ed Spiridonov
> Ed,
> I have been unable to replicate the issue you reported with /dev/null.
> Could you provide some more details as to what you were doing to cause
> it? :)
I cannot reproduce error after this changes:
But now I find another trouble: if I run dd and remove card - dd hang.
Tony Prisk
run?
Ed Spiridonov
> Is that if you remove the card while dd is running, or after it has
> run?
while dd if running.
Tony Prisk
To be honest, this wasn't even something I considered.
The dma_completion would get stuck at the next regular_isr if the card
was no longer present.
I have tested removing the card during reading and writing, both seem
to just dump out a pile of timeouts and then return to normal.
Sentient
No more polling/blocking code - fully interrupt driven.
Doesn't seem to have made much improvement to the raw speed (although it
could be that my card is maxed out @ 15MB/s (read) 10MB/s (write) ) but it
will reduce the CPU time.
Tony Prisk
Unfortunately, I don't have the 'know-how' to patch the kernel to add the feature suggested, and therefore cannot implement the added functionality in my driver.
If the patch gets accepted, then it isn't a lot of work to add the functionality to the driver anyway.
Looks like an interesting idea.
Josiah Walker
Ed Spiridonov
> Is this in alexey's git or another? Where is the driver in the config so I
> can turn it on and test it?
It is in this tree:
http://gitorious.org/linux-on-via-vt8500/tprisk-wm8505-sdmmc-dev
(you must use origin v2.6.35-vt8500).
PS: I can publish diff's with velocity support for this tree.
Josiah Walker
Josiah Walker
The error I get is:
~/Desktop/tprisk-wm8505-sdmmc-dev$ make menuconfig
scripts/kconfig/mconf Kconfig
drivers/net/can/Kconfig:120: can't open file "drivers/net/can/softing/Kconfig"
make[1]: *** [menuconfig] Error 1
make: *** [menuconfig] Error 2
Is this file significant or do I just hack it out for now?
Tony Prisk
Josiah Walker
CC arch/arm/mach-vt8500/irq.o
arch/arm/mach-vt8500/irq.c: In function ‘vt8500_irq_set_type’:
arch/arm/mach-vt8500/irq.c:100: error: ‘irq_desc’ undeclared (first use in this function)
arch/arm/mach-vt8500/irq.c:100: error: (Each undeclared identifier is reported only once
arch/arm/mach-vt8500/irq.c:100: error: for each function it appears in.)
make[1]: *** [arch/arm/mach-vt8500/irq.o] Error 1
make: *** [arch/arm/mach-vt8500] Error 2
Is there an easy fix for this?
Tony Prisk
Ed Spiridonov
> I'm not sure how you got that error - I just did a 'make distclean', and
> recompiled with my .config - seems to work fine.
I got another error:
CC arch/arm/mach-vt8500/wm8505-clocks.o
arch/arm/mach-vt8500/wm8505-clocks.c:461: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:462: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:463: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:464: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:465: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:467: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:468: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:469: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:470: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:471: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:472: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:473: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:474: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:475: error: stray '\' in program
arch/arm/mach-vt8500/wm8505-clocks.c:476: error: stray '\' in program
make[1]: *** [arch/arm/mach-vt8500/wm8505-clocks.o] Error 1
Tony Prisk
Those errors occured when I tidied up the clocks code - when I tried to push the fixes, it seems to have changed from 2.6.35-vt8500 to master, and pushed 194 changes
Now I have no idea whats where or what the current state of the repo is.
The error is easily fixed just by tidying up the \ line breaks at the mentioned locations.
Josiah Walker
Tony Prisk
i have a feeling Alexey will know the answer as he wrote the irq code.
Josiah Walker
Alexey Charkov
Could you guys try to 'git bisect' the issue? Last time I checked, it worked fine, so there must have been some change in kernel.
On Feb 28, 2011 3:03 PM, "Josiah Walker" <mer...@gmail.com> wrote:
Yes, until then I guess I'm waiting for a fix to test all this on my machines...
On Sun, Feb 27, 2011 at 6:01 PM, Tony Prisk <sent...@digitalfantasy.co.nz> wrote:
>
> Even worse,...
Josiah Walker
Alexey Charkov
Try a bit earlier, in December or so. I haven't really had any opportunity to test stuff since then.
On Feb 28, 2011 5:05 PM, "Josiah Walker" <mer...@gmail.com> wrote:
Maybe I've messed up in how to use git, but doing a git bisect going back a month, and running make clean && make uImage each time I bisect... I get the same error every time.
On Mon, Feb 28, 2011 at 8:21 PM, Alexey Charkov <alc...@gmail.com> wrote:
>
> Could you guys try ...
Josiah Walker
Alexey Charkov
Well, Tony's tree is a descendant of my one, so it should contain all the commits. Just grep the changelog for something authored by me, most likely the culprit will be there:-)
As for a fresh copy of my tree, that may take long. As I mentioned in some other discussion, I've changed my job in January, so now I'm somewhat constrained on time and far away from the hardware.
Best,
Alexey
On Feb 28, 2011 5:33 PM, "Josiah Walker" <mer...@gmail.com> wrote:
I'm not sure how to do that from tony's tree, so it'll have to wait until I can grab a fresh copy of yours I guess.
On Mon, Feb 28, 2011 at 10:10 PM, Alexey Charkov <alc...@gmail.com> wrote:
>
> Try a bit earlier,...
Josiah Walker
8672edf5e8b3f7190cbd1e89ccda95a1e58c6d6c
This is one of Tony's commits (27 Jan), and I don't think it's related to the feature causing the problem - just that the feature causing the problem becomes enabled by default in the menuconfig by this time.
I say this because this build originally showed the same error (when I built a .config file for it), but after coming back to it using a .config file generated for previous commits, there was no error any more. Coming back to the head of the tree with the same .config file, there are no problems anymore either. My guess is this was never picked up because most people tend to use saved .config files.
Somebody probably needs to go through with git biset again and rebuild the .config file at every step to track down the feature(s) that cause the problem.
Josiah Walker
Josiah Walker
CC [M] drivers/mtd/nand/wmt_nand.o
drivers/mtd/nand/wmt_nand.c: In function ‘vt8500_nand_cmdfunc’:
drivers/mtd/nand/wmt_nand.c:2062: warning: suggest parentheses around operand of ‘!’ or change ‘&’ to ‘&&’ or ‘!’ to ‘~’
drivers/mtd/nand/wmt_nand.c:1426: warning: label ‘read_page_again’ defined but not used
drivers/mtd/nand/wmt_nand.c:1409: warning: unused variable ‘bank_stat1’
drivers/mtd/nand/wmt_nand.c: In function ‘vt8500_nand_read_oob’:
drivers/mtd/nand/wmt_nand.c:2368: warning: unused variable ‘info’
drivers/mtd/nand/wmt_nand.c: At top level:
drivers/mtd/nand/wmt_nand.c:613: warning: ‘disable_redunt_out_bch_ctrl’ defined but not used
drivers/mtd/nand/wmt_nand.c:2405: warning: ‘vt8500_nand_read_bb_oob’ defined but not used
make[3]: *** No rule to make target `drivers/mtd/nand/wmt2_nand.c', needed by `drivers/mtd/nand/wmt2_nand.o'. Stop.
make[2]: *** [drivers/mtd/nand] Error 2
make[1]: *** [drivers/mtd] Error 2
make: *** [drivers] Error 2
Josiah Walker
Ed Spiridonov
> I'm getting a blank screen on boot now... I'll double check the config
> details tonight though. It will be nice when I can finally install X and
> have internet! :-)
firefox is very slow on this device ;)
Alexey Charkov
Guys, I have just pushed a slight change to the IRQ code that is
supposed to fix the irq_desc problem. Could you please test this on
your hardware, as currently I have no access to mine?
Best,
Alexey