Re: [PATCH 1/5] mmc: sdhci-esdhc-imx: add support for write protect on custom GPIO
Marc,
On Fri, Feb 11, 2011 at 09:24:38AM +1100, Marc Reilly wrote:
Tested-by: Marc Reilly m...@cpdesign.com.au
Thanks a lot!
+ if (boarddata) {
Perhaps (boarddata gpio_is_valid(boarddata-wp_gpio) as above?
For example what if someone sets up the boarddata for a card detect, but not
write protect.
Well, I couldn't do this here, because later the cd_pin-stuff will also
be added to the if-block. However, I see what you mean and that could
still be added before the gpio_request below. That would add another
level of indentation, though, for a case I'd consider to be more
theoretical (and definately not recommended). Maybe I will adjust the
warning message to be also understandable for the case a GPIO is preset
with an invalid number. I think the warning itself is justified.
+ err = gpio_request_one(boarddata-wp_gpio, GPIOF_IN,
ESDHC_WP);
+ if (err) {
+ dev_warn(mmc_dev(host-mmc), can't get wp_pin!\n);
+ boarddata-wp_gpio = err;
+ }
+ }
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] mmc: sdhci-esdhc: broken card detection is not a default quirk
On Thu, Feb 10, 2011 at 08:18:21PM +0100, Wolfram Sang wrote:
Adding Anton to Cc...
[...]
struct sdhci_pltfm_data sdhci_esdhc_imx_pdata = {
- .quirks = ESDHC_DEFAULT_QUIRKS | SDHCI_QUIRK_BROKEN_ADMA,
+ .quirks = ESDHC_DEFAULT_QUIRKS | SDHCI_QUIRK_BROKEN_ADMA
+ | SDHCI_QUIRK_BROKEN_CARD_DETECTION,
Minor nit: | should be on the previous line.
Otherwise,
Acked-by: Anton Vorontsov cbouatmai...@gmail.com
Thanks!
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] mmc: mxs-mmc: add mmc host driver for i.MX23/28
Hi Lothar, On Wed, Feb 09, 2011 at 08:46:18AM +0100, Lothar Waßmann wrote: Hi Shawn, Shawn Guo writes: This adds the mmc host driver for Freescale MXS-based SoC i.MX23/28. The driver calls into mxs-dma via generic dmaengine api for both pio and data transfer. Signed-off-by: Shawn Guo shawn@freescale.com --- arch/arm/mach-mxs/include/mach/mmc.h | 15 + drivers/mmc/host/Kconfig |9 + drivers/mmc/host/Makefile|1 + drivers/mmc/host/mxs-mmc.c | 884 ++ 4 files changed, 909 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-mxs/include/mach/mmc.h create mode 100644 drivers/mmc/host/mxs-mmc.c I've run the mmc-test kernel module with this driver on our TX28 module which fails in the following tests: |mmc0: Test case 15. Correct xfer_size at write (start failure)... |mmc0: Result: ERROR (-110) |mmc0: Test case 16. Correct xfer_size at read (start failure)... |mmc0: Result: ERROR (-110) |mmc0: Test case 17. Correct xfer_size at write (midway failure)... |mmc0: Result: ERROR (-110) |mmc0: Test case 18. Correct xfer_size at read (midway failure)... |mmc0: Result: ERROR (-110) Could you try the test on your hardware? I'm new to this test. I enabled MMC_TEST but did not see test under /sys for mmc. Can you please elaborate how to launch this test? Thanks. Regards, Shawn -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] mmc: mxs-mmc: add mmc host driver for i.MX23/28
Shawn Guo writes:
Hi Lothar,
On Tue, Feb 08, 2011 at 12:41:20PM +0100, Lothar Waßmann wrote:
Hi,
Shawn Guo writes:
[...]
+static int mxs_mmc_remove(struct platform_device *pdev)
+{
+ struct mmc_host *mmc = platform_get_drvdata(pdev);
+ struct mxs_mmc_host *host = mmc_priv(mmc);
+
+ platform_set_drvdata(pdev, NULL);
+
+ mmc_remove_host(mmc);
+
+ del_timer(host-timer);
+
+ free_irq(host-irq, host);
+
+ if (host-dmach)
+ dma_release_channel(host-dmach);
+
+ clk_disable(host-clk);
+ clk_put(host-clk);
+
+ iounmap(host-base);
+
+ mmc_free_host(mmc);
+
+ release_mem_region(host-res-start, resource_size(host-res));
When compiled with CONFIG_PAGE_POISON this leads to:
|mmc0: card cdef removed
|Unable to handle kernel paging request at virtual address 6b6b6b6b
|pgd = c6ea4000
|[6b6b6b6b] *pgd=
|Internal error: Oops: 1 [#1] PREEMPT
|last sysfs file: /sys/module/mxs_mmc/refcnt
|Modules linked in: mxs_mmc(-) evdev nand nand_ids nand_ecc tsc2007 pca953x
|CPU: 0Not tainted (2.6.37-karo+ #100)
|PC is at mxs_mmc_remove+0x78/0x94 [mxs_mmc]
|LR is at mark_held_locks+0x5c/0x84
|pc : [bf03310c]lr : [c0071da0]psr: 2013
|sp : c6e33ef8 ip : 6b6b6b6b fp : be825a38
|r10: r9 : c6e32000 r8 : c0037888
|r7 : 00591700 r6 : c78d24bc r5 : c6c6ea80 r4 : c6c6ed60
|r3 : 6b6b6b6b r2 : 0040 r1 : c6d833d0 r0 : c043af18
|Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
|Control: 0005317f Table: 46ea4000 DAC: 0015
|Process modprobe (pid: 1217, stack limit = 0xc6e32270)
|Stack: (0xc6e33ef8 to 0xc6e34000)
|3ee0: c78d2488
bf034100
|3f00: c78d24bc c0237034 c78d2488 c0235e68 c78d2488 bf034100 c78d24bc
c0235f34
|3f20: bf034100 0080 c045c3e8 c02351c4 bf034138 0080 c6e33f44
c007de4c
|3f40: be82599c 5f73786d 00636d6d c01efdf0 c6d830c0 c6d830c0
c03195ec
|3f60: 0001 c6dddbd8 c6e33f7c c0045fc4 c6d830c0 c00377d8 0001
0081
|3f80: 6010 c00720d4 be825a2c 0001 be825a2c 005916b0
0001
|3fa0: 0081 c00376c0 be825a2c 005916b0 00591700 0080 be825994
|3fc0: be825a2c 005916b0 0001 0081 00591700 c69c 005916bc
be825a38
|3fe0: 00591520 be8259a0 a42c 402aee3c 6010 00591700
|[bf03310c] (mxs_mmc_remove+0x78/0x94 [mxs_mmc]) from [c0237034]
(platform_drv_remove+0x18/0x1c)
|[c0237034] (platform_drv_remove+0x18/0x1c) from [c0235e68]
(__device_release_driver+0x64/0xa4)
|[c0235e68] (__device_release_driver+0x64/0xa4) from [c0235f34]
(driver_detach+0x8c/0xb4)
|[c0235f34] (driver_detach+0x8c/0xb4) from [c02351c4]
(bus_remove_driver+0x8c/0xb4)
|[c02351c4] (bus_remove_driver+0x8c/0xb4) from [c007de4c]
(sys_delete_module+0x1f4/0x260)
|[c007de4c] (sys_delete_module+0x1f4/0x260) from [c00376c0]
(ret_fast_syscall+0x0/0x38)
|Code: e1a5 eb48cd47 e5943008 e59f0014 (e8930006)
|---[ end trace bb06175839554c3b ]---
indicating a use_after_free BUG!
Thanks for catching this.
The struct mxs_mmc_host has been already freed here by the
preceding mmc_free_host() call. This should be:
struct resource *res = host-res;
...
mmc_free_host(mmc);
release_mem_region(res-start, resource_size(res));
How about fixing it like below?
release_mem_region(host-res-start, resource_size(host-res));
mmc_free_host(mmc);
It's also OK. Although I prefer to do the release_mem_region() as the
last action, because the corresponding request_mem_region() is the
first action of the driver.
I still have some more comments:
|static int mxs_mmc_remove(struct platform_device *pdev)
|{
| struct mmc_host *mmc = platform_get_drvdata(pdev);
| struct mxs_mmc_host *host = mmc_priv(mmc);
|
| platform_set_drvdata(pdev, NULL);
This should be done after the driver has been quiesced (i.e. after
free_irq). It's not relevant in this case right now, but cleaner
anyway since some timer or IRQ handler might still be triggered and
call platform_get_drvdata().
If you always care to do the actions in the remove() function in the
opposite order as the corresponding actions in the probe() function
things will usually be in the right order automatically.
|static int mxs_mmc_suspend(struct device *dev)
|{
| struct mmc_host *mmc = dev_get_drvdata(dev);
| struct mxs_mmc_host *host = mmc_priv(mmc);
| int ret = 0;
|
| if (mmc)
'mmc' cannot be NULL here. And as it has already been dereferenced
by mmc_priv(mmc) above, it makes even less sense to check it here.
|static int mxs_mmc_resume(struct device *dev)
|{
| struct mmc_host *mmc = dev_get_drvdata(dev);
| struct mxs_mmc_host *host = mmc_priv(mmc);
| int ret = 0;
|
| clk_enable(host-clk);
|
| if (mmc)
same as above.
Lothar Waßmann
--
Re: [PATCH 1/7] mmc: mxs-mmc: add mmc host driver for i.MX23/28
Hi,
Shawn Guo writes:
Hi Lothar,
On Wed, Feb 09, 2011 at 08:46:18AM +0100, Lothar Waßmann wrote:
Hi Shawn,
Shawn Guo writes:
This adds the mmc host driver for Freescale MXS-based SoC i.MX23/28.
The driver calls into mxs-dma via generic dmaengine api for both pio
and data transfer.
Signed-off-by: Shawn Guo shawn@freescale.com
---
arch/arm/mach-mxs/include/mach/mmc.h | 15 +
drivers/mmc/host/Kconfig |9 +
drivers/mmc/host/Makefile|1 +
drivers/mmc/host/mxs-mmc.c | 884
++
4 files changed, 909 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-mxs/include/mach/mmc.h
create mode 100644 drivers/mmc/host/mxs-mmc.c
I've run the mmc-test kernel module with this driver on our TX28
module which fails in the following tests:
|mmc0: Test case 15. Correct xfer_size at write (start failure)...
|mmc0: Result: ERROR (-110)
|mmc0: Test case 16. Correct xfer_size at read (start failure)...
|mmc0: Result: ERROR (-110)
|mmc0: Test case 17. Correct xfer_size at write (midway failure)...
|mmc0: Result: ERROR (-110)
|mmc0: Test case 18. Correct xfer_size at read (midway failure)...
|mmc0: Result: ERROR (-110)
Could you try the test on your hardware?
I'm new to this test. I enabled MMC_TEST but did not see test
under /sys for mmc. Can you please elaborate how to launch this
test? Thanks.
You need debugfs mounted and the mmc-test module loaded.
An inserted card will normally be grabbed by the mmc-block driver. You
can 'unbind' the driver and 'bind' it to the mmc-test driver using the
corresponding entries in sysfs. The mmc-test driver can be controlled
via debugfs.
The following script steals an inserted card from the mmc-block driver
and performs all mmc-test testcases (TRASHING THE CONTENTS OF THE
CARD!).
#!/bin/bash
drv_path=/sys/bus/mmc/drivers
dbg_path=/sys/kernel/debug
testdrv=mmc_test
shopt -s nullglob
card=
cleanup() {
if [ -n $card -a -n $driver -a $driver != $testdrv ];then
echo Restoring driver $driver
echo $card $drv_path/$testdrv/unbind
echo $card $drv_path/$driver/bind
fi
}
trap cleanup 0
set -e
cd $drv_path
for d in *;do
for l in $d/*;do
[ -h $l ] || continue
card=$(basename $l)
driver=$(dirname $l)
break
done
[ -n $card ] break
done
if [ -z $card ];then
echo No MMC/SD card found
exit 1
fi
if [ $driver != $testdrv ];then
echo Trying to claim card $card from driver $driver
echo $card $drv_path/$driver/unbind
echo $card $drv_path/$testdrv/bind
fi
echo WARNING: Card $card will be overwritten!
echo Hit ENTER to continue; CTRL-C to abort
read
# perform all testcases ( 0 for individual tests)
echo 0 $dbg_path/${card%:*}/$card/test
Lothar Waßmann
--
___
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | i...@karo-electronics.de
___
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] mmc: mxs-mmc: add mmc host driver for i.MX23/28
On Fri, Feb 11, 2011 at 03:52:52PM +0100, Lothar Waßmann wrote:
Hi,
Shawn Guo writes:
Hi Lothar,
On Wed, Feb 09, 2011 at 08:46:18AM +0100, Lothar Waßmann wrote:
Hi Shawn,
Shawn Guo writes:
This adds the mmc host driver for Freescale MXS-based SoC i.MX23/28.
The driver calls into mxs-dma via generic dmaengine api for both pio
and data transfer.
Signed-off-by: Shawn Guo shawn@freescale.com
---
arch/arm/mach-mxs/include/mach/mmc.h | 15 +
drivers/mmc/host/Kconfig |9 +
drivers/mmc/host/Makefile|1 +
drivers/mmc/host/mxs-mmc.c | 884
++
4 files changed, 909 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-mxs/include/mach/mmc.h
create mode 100644 drivers/mmc/host/mxs-mmc.c
I've run the mmc-test kernel module with this driver on our TX28
module which fails in the following tests:
|mmc0: Test case 15. Correct xfer_size at write (start failure)...
|mmc0: Result: ERROR (-110)
|mmc0: Test case 16. Correct xfer_size at read (start failure)...
|mmc0: Result: ERROR (-110)
|mmc0: Test case 17. Correct xfer_size at write (midway failure)...
|mmc0: Result: ERROR (-110)
|mmc0: Test case 18. Correct xfer_size at read (midway failure)...
|mmc0: Result: ERROR (-110)
Could you try the test on your hardware?
I'm new to this test. I enabled MMC_TEST but did not see test
under /sys for mmc. Can you please elaborate how to launch this
test? Thanks.
You need debugfs mounted and the mmc-test module loaded.
An inserted card will normally be grabbed by the mmc-block driver. You
can 'unbind' the driver and 'bind' it to the mmc-test driver using the
corresponding entries in sysfs. The mmc-test driver can be controlled
via debugfs.
The following script steals an inserted card from the mmc-block driver
and performs all mmc-test testcases (TRASHING THE CONTENTS OF THE
CARD!).
#!/bin/bash
drv_path=/sys/bus/mmc/drivers
dbg_path=/sys/kernel/debug
testdrv=mmc_test
shopt -s nullglob
card=
cleanup() {
if [ -n $card -a -n $driver -a $driver != $testdrv ];then
echo Restoring driver $driver
echo $card $drv_path/$testdrv/unbind
echo $card $drv_path/$driver/bind
fi
}
trap cleanup 0
set -e
cd $drv_path
for d in *;do
for l in $d/*;do
[ -h $l ] || continue
card=$(basename $l)
driver=$(dirname $l)
break
done
[ -n $card ] break
done
if [ -z $card ];then
echo No MMC/SD card found
exit 1
fi
if [ $driver != $testdrv ];then
echo Trying to claim card $card from driver $driver
echo $card $drv_path/$driver/unbind
echo $card $drv_path/$testdrv/bind
fi
echo WARNING: Card $card will be overwritten!
echo Hit ENTER to continue; CTRL-C to abort
read
# perform all testcases ( 0 for individual tests)
echo 0 $dbg_path/${card%:*}/$card/test
Thanks a lot for the test script. Yes, I'm seeing the same problem
here. I will try to get it fixed in v2.
Regards,
Shawn
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] mmc: mxs-mmc: add mmc host driver for i.MX23/28
On Saturday 12 February 2011, Shawn Guo wrote:
On Thu, Feb 10, 2011 at 06:17:41PM +0100, Arnd Bergmann wrote:
On Friday 11 February 2011, Shawn Guo wrote:
+ struct mxs_dma_data dma_data;
Why do you need host specific DMA structures? Please stick to
the generic dma-engine API if possible.
I'm sticking to the generic dmaengine api. The mxs dma hardware
has separate irq line for every single dma channel, which needs to
be told by client driver.
I'm not convinced, it still sounds like a layering violation to
have specific information about the DMA controller in the
platform data of a driver using the dma engine API.
It sounds like something about the dma controller, but it really
belongs to dma client device e.g. ssp here. Every single dma client
device has two dma related resources, dma channel and dma irq, which
should be defined in client device data.
That is true for the DMA controller you use, but it doesn't have
to be that way for all other DMA controllers, right? My point is
that the MMC driver should not make any assumptions about what
DMA controller is used, because a future SoC might combine it
with a different DMA controller, e.g. one that just just a single
interrupt for all channels.
Why can't you put the interrupt number into the platform data of
the dma engine device? Your filter function already identifies
the number of the DMA channel.
We have 16 channels for dma-apbh and dma-apbx respectively. And each
channel has fixed peripheral device and irq. You think we can define
2 x 16 x (channel number + channel irq) in dma engine driver? I'm
afraid not. The channel number can be identified in filter function
because dmaengine core code and mxs dma hw are indexing the channel
in the same way, so that the sw channel id can be used to address hw
channel, otherwise we have to pass hw channel id to dma driver just
like what we do with irq.
I don't understand. The sw channel id should be something that is
defined in an arbitrary way for each machine, and it should be the
only thing that a driver needs, right?
I have not looked much at other dmaengine drivers, but I'd be
surprised if they require the device driver to be written
for a specific implementation. If that was the case, you would
not even need a dmaengine API but could just as well write
to the DMA controller registers from the device driver directly.
What I meant is that you take care to avoid getting into the
interrupt handler while holding the spinlock, but in the handler,
you don't check if the lock is held. It can't be correct to
serialize just half the cases.
Thanks for the explanation. Please help review the fix below to see
if I understand the comment correctly.
if ((stat BM_SSP_CTRL1_SDIO_IRQ) (stat
BM_SSP_CTRL1_SDIO_IRQ_EN)) {
spin_lock_irqsave(host-lock, flags);
mmc_signal_sdio_irq(host-mmc);
spin_unlock_irqrestore(host-lock, flags);
}
This is still wrong for two reasons:
* You now don't hold the lock while reading the 'stat' variable. The point of
the
lock is to make sure that sdio doesn't get turned off after reading stat but
before signaling the sdio, so you have to hold it across both.
* In an interrupt controller, you should not disable interrupts again.
It's harmless, but slow and confusing to the reader.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Moving platform_data contents to device tree
On Fri, Feb 11, 2011 at 8:45 AM, Rob Herring robherri...@gmail.com wrote: Thomas, On 02/10/2011 09:29 PM, Thomas Abraham wrote: Hi, I am currently adding device tree support for Samsung's S5PV310 processor. I have a question about handling platform_data when adding device tree support in drivers, specifically about the sdhci-s3c driver. The platform data that is passed to the sdhci-s3c driver is defined in file arch/arm/plat-samsung/plat/sdhci.h, struct s3c_sdhci_platdata. In this structure, there are some function pointers that are passed to the driver. These function pointers are setup by the platform code in arch/arm/plat-samsung. But when platform devices are created from the device tree, how would such function pointers be passed to the driver? Any suggestions on the approach to handle the platform_data information when moving to device tree would be very helpful. As suggested by Grant, you can use bus notifiers. Here is an example: arch/powerpc/platforms/512x/pdm360ng.c Pure data (flags, quirks, chip select assignments, etc.) should ultimately go into the device tree. For board specific functions, use the bus notifiers. For SoC functions, put them in the driver and use the OF match table data pointer. Yes, Rob is right. However, things are simpler if the driver can be designed so that it *doesn't* need platform callbacks. Sometimes it is unavoidable, but in a lot of cases platform callback turn into a way to let platform code twiddle or read GPIOs. In those cases it would be better to turn those callbacks into real gpiolib drivers and design the driver to use the gpio api. See sdhci-of-core.c for an example. On a side note (and not related to sdhci-s3c); I'd like to be rid of both sdhci-of-core.c and sdhci-pltfm.c. I'm all for providing common infrastructure, but I think those two files go about it in the wrong way. Rather than having a single platform_device (well, 2 in this case, but of and non-of can be merged now) that matches against all 'platform' sdhci controllers, each specific controller should have its own platform_driver structure. The way it is done now creates too much indirection (IMHO) and it would be cleaner if the common code was available to the drivers a library function calls. I've put splitting them up onto my todo list if somebody else doesn't beat me to it. g. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] mmc: mxs-mmc: add mmc host driver for i.MX23/28
On Fri, Feb 11, 2011 at 04:51:35PM +0100, Arnd Bergmann wrote:
On Saturday 12 February 2011, Shawn Guo wrote:
On Thu, Feb 10, 2011 at 06:17:41PM +0100, Arnd Bergmann wrote:
On Friday 11 February 2011, Shawn Guo wrote:
+ struct mxs_dma_data dma_data;
Why do you need host specific DMA structures? Please stick to
the generic dma-engine API if possible.
I'm sticking to the generic dmaengine api. The mxs dma hardware
has separate irq line for every single dma channel, which needs to
be told by client driver.
I'm not convinced, it still sounds like a layering violation to
have specific information about the DMA controller in the
platform data of a driver using the dma engine API.
It sounds like something about the dma controller, but it really
belongs to dma client device e.g. ssp here. Every single dma client
device has two dma related resources, dma channel and dma irq, which
should be defined in client device data.
That is true for the DMA controller you use, but it doesn't have
to be that way for all other DMA controllers, right? My point is
that the MMC driver should not make any assumptions about what
DMA controller is used, because a future SoC might combine it
with a different DMA controller, e.g. one that just just a single
interrupt for all channels.
Your thought is right. But the fact is, from hw design point of
view, all client peripherals are coupled with mxs dma. As a real
example, gpmi (mxs nand controller) is one mxs dma client device.
When gpmi is integrated into i.MX50 to replace nfc (original i.mx
nand controller), dma-aphb has to be brought into mx50 together,
though mx50 already gets sdma there.
I tried hard to decouple client driver from mxs dmaengine driver, but
it can not be that thorough. As dma peripheral only trigger error irq,
mxs dma pio mode and dma irq need to be handled by client and dma
driver in a coupled way.
Why can't you put the interrupt number into the platform data of
the dma engine device? Your filter function already identifies
the number of the DMA channel.
We have 16 channels for dma-apbh and dma-apbx respectively. And each
channel has fixed peripheral device and irq. You think we can define
2 x 16 x (channel number + channel irq) in dma engine driver? I'm
afraid not. The channel number can be identified in filter function
because dmaengine core code and mxs dma hw are indexing the channel
in the same way, so that the sw channel id can be used to address hw
channel, otherwise we have to pass hw channel id to dma driver just
like what we do with irq.
I don't understand. The sw channel id should be something that is
defined in an arbitrary way for each machine, and it should be the
only thing that a driver needs, right?
Yes, the sw channel id can be defined in an arbitrary way by
dmaengine, but in that case dmaegine can not use sw channel id to
access hw channel, because each mxs dma client device gets a specific
hw channel. Of course, we can give device an arbitrary channel with
a fixed hw channel id attached on, and use hw id to access hw channel.
But in that case, we have one more info in device data to pass besides
irq.
I have not looked much at other dmaengine drivers, but I'd be
surprised if they require the device driver to be written
for a specific implementation. If that was the case, you would
not even need a dmaengine API but could just as well write
to the DMA controller registers from the device driver directly.
We need a specific implementation, but it's not so specific that we
have to access dma controller directly. Even it is, we still need
an API/interface, as there are so many client devices need to do the
same thing, right? ;)
What I meant is that you take care to avoid getting into the
interrupt handler while holding the spinlock, but in the handler,
you don't check if the lock is held. It can't be correct to
serialize just half the cases.
Thanks for the explanation. Please help review the fix below to see
if I understand the comment correctly.
if ((stat BM_SSP_CTRL1_SDIO_IRQ) (stat
BM_SSP_CTRL1_SDIO_IRQ_EN)) {
spin_lock_irqsave(host-lock, flags);
mmc_signal_sdio_irq(host-mmc);
spin_unlock_irqrestore(host-lock, flags);
}
This is still wrong for two reasons:
* You now don't hold the lock while reading the 'stat' variable. The point of
the
lock is to make sure that sdio doesn't get turned off after reading stat but
before signaling the sdio, so you have to hold it across both.
* In an interrupt controller, you should not disable interrupts again.
It's harmless, but slow and confusing to the reader.
Sorry for the dumb. So it should be something like the following?
spin_lock(host-lock);
stat = readl(host-base + HW_SSP_CTRL1);
writel(stat
Re: [PATCH 1/7] mmc: mxs-mmc: add mmc host driver for i.MX23/28
On Thu, Feb 10, 2011 at 06:17:41PM +0100, Arnd Bergmann wrote:
On Friday 11 February 2011, Shawn Guo wrote:
+ struct mxs_dma_data dma_data;
Why do you need host specific DMA structures? Please stick to
the generic dma-engine API if possible.
I'm sticking to the generic dmaengine api. The mxs dma hardware
has separate irq line for every single dma channel, which needs to
be told by client driver.
I'm not convinced, it still sounds like a layering violation to
have specific information about the DMA controller in the
platform data of a driver using the dma engine API.
It sounds like something about the dma controller, but it really
belongs to dma client device e.g. ssp here. Every single dma client
device has two dma related resources, dma channel and dma irq, which
should be defined in client device data.
Why can't you put the interrupt number into the platform data of
the dma engine device? Your filter function already identifies
the number of the DMA channel.
We have 16 channels for dma-apbh and dma-apbx respectively. And each
channel has fixed peripheral device and irq. You think we can define
2 x 16 x (channel number + channel irq) in dma engine driver? I'm
afraid not. The channel number can be identified in filter function
because dmaengine core code and mxs dma hw are indexing the channel
in the same way, so that the sw channel id can be used to address hw
channel, otherwise we have to pass hw channel id to dma driver just
like what we do with irq.
+static irqreturn_t mxs_mmc_irq_handler(int irq, void *dev_id)
+{
+ struct mxs_mmc_host *host = dev_id;
+ u32 stat;
+
+ stat = __raw_readl(host-base + HW_SSP_CTRL1);
+ __mxs_clrl(stat MXS_MMC_IRQ_BITS, host-base + HW_SSP_CTRL1);
+
+ if (host-cmd (stat MXS_MMC_ERR_BITS))
+ host-status = __raw_readl(host-base + HW_SSP_STATUS);
+
+ if ((stat BM_SSP_CTRL1_SDIO_IRQ) (stat
BM_SSP_CTRL1_SDIO_IRQ_EN))
+ mmc_signal_sdio_irq(host-mmc);
+
+ return IRQ_HANDLED;
+}
You use spin_lock_irqsave in mxs_mmc_enable_sdio_irq, but don't
actually use the spinlock in the interrupt handler. This means
that either your interrupt handler is broken because it doesn't
lock, or that you don't actually need the _irqsave part.
I do not understand this one. I'm seeing mxcmmc and pxamci use
spin_lock_irqsave/irqrestore in the similar way.
The difference is that e.g. mxcmci_irq() takes the spinlock that is
used to protect the host-use_sdio flag, serializing the the
code that initializes sdio with the code that uses it.
[Actually, mxcmci_irq() also looks wrong, because it releases the
spinlock before calling mmc_signal_sdio_irq(), so sdio may be
disabled by then, but that is a slightly different bug]
What I meant is that you take care to avoid getting into the
interrupt handler while holding the spinlock, but in the handler,
you don't check if the lock is held. It can't be correct to
serialize just half the cases.
Thanks for the explanation. Please help review the fix below to see
if I understand the comment correctly.
if ((stat BM_SSP_CTRL1_SDIO_IRQ) (stat
BM_SSP_CTRL1_SDIO_IRQ_EN)) {
spin_lock_irqsave(host-lock, flags);
mmc_signal_sdio_irq(host-mmc);
spin_unlock_irqrestore(host-lock, flags);
}
Regards,
Shawn
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] mmc: mxs-mmc: add mmc host driver for i.MX23/28
On Fri, Feb 11, 2011 at 03:40:34PM +0100, Lothar Waßmann wrote:
Shawn Guo writes:
Hi Lothar,
On Tue, Feb 08, 2011 at 12:41:20PM +0100, Lothar Waßmann wrote:
Hi,
Shawn Guo writes:
[...]
+static int mxs_mmc_remove(struct platform_device *pdev)
+{
+ struct mmc_host *mmc = platform_get_drvdata(pdev);
+ struct mxs_mmc_host *host = mmc_priv(mmc);
+
+ platform_set_drvdata(pdev, NULL);
+
+ mmc_remove_host(mmc);
+
+ del_timer(host-timer);
+
+ free_irq(host-irq, host);
+
+ if (host-dmach)
+ dma_release_channel(host-dmach);
+
+ clk_disable(host-clk);
+ clk_put(host-clk);
+
+ iounmap(host-base);
+
+ mmc_free_host(mmc);
+
+ release_mem_region(host-res-start, resource_size(host-res));
When compiled with CONFIG_PAGE_POISON this leads to:
|mmc0: card cdef removed
|Unable to handle kernel paging request at virtual address 6b6b6b6b
|pgd = c6ea4000
|[6b6b6b6b] *pgd=
|Internal error: Oops: 1 [#1] PREEMPT
|last sysfs file: /sys/module/mxs_mmc/refcnt
|Modules linked in: mxs_mmc(-) evdev nand nand_ids nand_ecc tsc2007
pca953x
|CPU: 0Not tainted (2.6.37-karo+ #100)
|PC is at mxs_mmc_remove+0x78/0x94 [mxs_mmc]
|LR is at mark_held_locks+0x5c/0x84
|pc : [bf03310c]lr : [c0071da0]psr: 2013
|sp : c6e33ef8 ip : 6b6b6b6b fp : be825a38
|r10: r9 : c6e32000 r8 : c0037888
|r7 : 00591700 r6 : c78d24bc r5 : c6c6ea80 r4 : c6c6ed60
|r3 : 6b6b6b6b r2 : 0040 r1 : c6d833d0 r0 : c043af18
|Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
|Control: 0005317f Table: 46ea4000 DAC: 0015
|Process modprobe (pid: 1217, stack limit = 0xc6e32270)
|Stack: (0xc6e33ef8 to 0xc6e34000)
|3ee0: c78d2488
bf034100
|3f00: c78d24bc c0237034 c78d2488 c0235e68 c78d2488 bf034100 c78d24bc
c0235f34
|3f20: bf034100 0080 c045c3e8 c02351c4 bf034138 0080 c6e33f44
c007de4c
|3f40: be82599c 5f73786d 00636d6d c01efdf0 c6d830c0 c6d830c0
c03195ec
|3f60: 0001 c6dddbd8 c6e33f7c c0045fc4 c6d830c0 c00377d8 0001
0081
|3f80: 6010 c00720d4 be825a2c 0001 be825a2c 005916b0
0001
|3fa0: 0081 c00376c0 be825a2c 005916b0 00591700 0080 be825994
|3fc0: be825a2c 005916b0 0001 0081 00591700 c69c 005916bc
be825a38
|3fe0: 00591520 be8259a0 a42c 402aee3c 6010 00591700
|[bf03310c] (mxs_mmc_remove+0x78/0x94 [mxs_mmc]) from [c0237034]
(platform_drv_remove+0x18/0x1c)
|[c0237034] (platform_drv_remove+0x18/0x1c) from [c0235e68]
(__device_release_driver+0x64/0xa4)
|[c0235e68] (__device_release_driver+0x64/0xa4) from [c0235f34]
(driver_detach+0x8c/0xb4)
|[c0235f34] (driver_detach+0x8c/0xb4) from [c02351c4]
(bus_remove_driver+0x8c/0xb4)
|[c02351c4] (bus_remove_driver+0x8c/0xb4) from [c007de4c]
(sys_delete_module+0x1f4/0x260)
|[c007de4c] (sys_delete_module+0x1f4/0x260) from [c00376c0]
(ret_fast_syscall+0x0/0x38)
|Code: e1a5 eb48cd47 e5943008 e59f0014 (e8930006)
|---[ end trace bb06175839554c3b ]---
indicating a use_after_free BUG!
Thanks for catching this.
The struct mxs_mmc_host has been already freed here by the
preceding mmc_free_host() call. This should be:
struct resource *res = host-res;
...
mmc_free_host(mmc);
release_mem_region(res-start, resource_size(res));
How about fixing it like below?
release_mem_region(host-res-start, resource_size(host-res));
mmc_free_host(mmc);
It's also OK. Although I prefer to do the release_mem_region() as the
last action, because the corresponding request_mem_region() is the
first action of the driver.
OK, will respect the comment.
I still have some more comments:
|static int mxs_mmc_remove(struct platform_device *pdev)
|{
| struct mmc_host *mmc = platform_get_drvdata(pdev);
| struct mxs_mmc_host *host = mmc_priv(mmc);
|
| platform_set_drvdata(pdev, NULL);
This should be done after the driver has been quiesced (i.e. after
free_irq). It's not relevant in this case right now, but cleaner
anyway since some timer or IRQ handler might still be triggered and
call platform_get_drvdata().
OK.
If you always care to do the actions in the remove() function in the
opposite order as the corresponding actions in the probe() function
things will usually be in the right order automatically.
I do care the action order in remove() vs. probe(), and that's why
I firstly call platform_set_drvdata(pdev, NULL) in remove() ;) So
should I move platform_set_drvdata(pdev, mmc) in probe() ahead of
request_irq()?
|static int mxs_mmc_suspend(struct device *dev)
|{
| struct mmc_host *mmc =
[Patch] mmc: add ricoh 0xe823 pci id
This patch enables RICOH MMC card reader with pci id 0xe823. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: add ricoh e823 pci id
Signed-off-by: Manoj Iyer manoj.i...@canonical.com
---
drivers/mmc/host/sdhci-pci.c |8
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index 0dc905b..f7e622c 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -547,6 +547,14 @@ static const struct pci_device_id pci_ids[] __devinitdata
= {
},
{
+ .vendor = PCI_VENDOR_ID_RICOH,
+ .device = 0xe823,
+ .subvendor = PCI_ANY_ID,
+ .subdevice = PCI_ANY_ID,
+ .driver_data= (kernel_ulong_t)sdhci_ricoh_mmc,
+ },
+
+ {
.vendor = PCI_VENDOR_ID_ENE,
.device = PCI_DEVICE_ID_ENE_CB712_SD,
.subvendor = PCI_ANY_ID,
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/3]mmc: enable TRIM/ERASE caps for SDHCI host
Hi Chris, These patches enable TRIM/ERASE capability for SDHCI host controller. patch001: mmc: set max_discard_sectors value for mmc queue Rightnow max_discard_sectors value for mmc queue is UINT_MAX which means block layer can accept unlimited sectors to erase at one time. But this may delay some other important I/O requests. So set a suitable value for max_discard_sectors value by using 'pref_erase' patch0002: mmc: enable TRIM/ERASE capability for SDHCI host. ERASE command needs R1B response, so before sending command, SDHCI host driver also need to set timeout control register for such command patch0003: mmc: fix division by zero when calculate erase timeout when enable clock gating feature, host clock may be zero when calculate erase timeout value and this will cause division by zero bug. Thanks Chuanxiao -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/3]mmc: set max_discard_sectors value for mmc queue
max_discard_sectors value is UINT_MAX which means kernel block layer can pass
down unlimited sectors to MMC driver to erase. But erasing so many sectors may
delay some other important I/O requests. This is not preferred.
So use 'pref_erase' to set a suitable max_discard_sectors value for mmc queue to
avoid erasing too many sectors at one time.
Signed-off-by: Chuanxiao Dong chuanxiao.d...@intel.com
---
drivers/mmc/card/queue.c |3 ++-
drivers/mmc/core/core.c | 20
include/linux/mmc/core.h |1 +
3 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 4e42d03..6c13859 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -131,7 +131,8 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card
*card, spinlock_t *lock
queue_flag_set_unlocked(QUEUE_FLAG_NONROT, mq-queue);
if (mmc_can_erase(card)) {
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, mq-queue);
- mq-queue-limits.max_discard_sectors = UINT_MAX;
+ mq-queue-limits.max_discard_sectors =
+ mmc_set_max_discard_sectors(card);
if (card-erased_byte == 0)
mq-queue-limits.discard_zeroes_data = 1;
if (!mmc_can_trim(card) is_power_of_2(card-erase_size)) {
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 34a7e8c..0eb27aa 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1470,6 +1470,26 @@ int mmc_erase_group_aligned(struct mmc_card *card,
unsigned int from,
}
EXPORT_SYMBOL(mmc_erase_group_aligned);
+/*
+ * Set max_discard_sectors for mmc queue.
+ * max_discard_sectors will determine how many sectors can be erased at one
+ * time. Sometimes user may want to erase a large area of SD or MMC card, it
may
+ * take long time which delay some other important I/O requests. So we would
+ * better set some values for max_discard_sectors which can forbid block layer
+ * pass too many sectors at one time.
+ *
+ * 'pref_erase' is initialized for this purpose. We can use it to set a
preferred
+ * value for it.
+ */
+int mmc_set_max_discard_sectors(struct mmc_card *card)
+{
+ if (card-pref_erase)
+ return card-pref_erase;
+ else
+ return UINT_MAX;
+}
+EXPORT_SYMBOL(mmc_set_max_discard_sectors);
+
int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen)
{
struct mmc_command cmd;
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 64e013f..a218d57 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -152,6 +152,7 @@ extern int mmc_can_trim(struct mmc_card *card);
extern int mmc_can_secure_erase_trim(struct mmc_card *card);
extern int mmc_erase_group_aligned(struct mmc_card *card, unsigned int from,
unsigned int nr);
+extern int mmc_set_max_discard_sectors(struct mmc_card *card);
extern int mmc_set_blocklen(struct mmc_card *card, unsigned int blocklen);
--
1.6.6.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/3]mmc: enable TRIM/ERASE caps for SDHCI host
SDHCI host controller has TRIM/ERASE capability, enable these caps for erasing
purpose.
ERASE command needs R1B response. So for such command with busy signal, before
sending command, SDHCI host driver will simply set a maximum value for timeout
control register which is safe to use.
Signed-off-by: Chuanxiao Dong chuanxiao.d...@intel.com
---
drivers/mmc/host/sdhci.c | 16 +++-
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 655617c..fe7cbd0 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -41,7 +41,6 @@
static unsigned int debug_quirks = 0;
-static void sdhci_prepare_data(struct sdhci_host *, struct mmc_data *);
static void sdhci_finish_data(struct sdhci_host *);
static void sdhci_send_command(struct sdhci_host *, struct mmc_command *);
@@ -652,16 +651,23 @@ static void sdhci_set_transfer_irqs(struct sdhci_host
*host)
sdhci_clear_set_irqs(host, dma_irqs, pio_irqs);
}
-static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
+static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command
*cmd)
{
u8 count;
u8 ctrl;
+ struct mmc_data *data = cmd-data;
int ret;
WARN_ON(host-data);
- if (data == NULL)
+ if (data == NULL) {
+ /*
+* set timeout to be maximum value for command with busy signal.
+*/
+ if (cmd-flags MMC_RSP_BUSY)
+ sdhci_writeb(host, 0xE, SDHCI_TIMEOUT_CONTROL);
return;
+ }
/* Sanity checks */
BUG_ON(data-blksz * data-blocks 524288);
@@ -921,7 +927,7 @@ static void sdhci_send_command(struct sdhci_host *host,
struct mmc_command *cmd)
host-cmd = cmd;
- sdhci_prepare_data(host, cmd-data);
+ sdhci_prepare_data(host, cmd);
sdhci_writel(host, cmd-arg, SDHCI_ARGUMENT);
@@ -1939,7 +1945,7 @@ int sdhci_add_host(struct sdhci_host *host)
mmc-f_min = host-max_clk / SDHCI_MAX_DIV_SPEC_200;
mmc-f_max = host-max_clk;
- mmc-caps |= MMC_CAP_SDIO_IRQ;
+ mmc-caps |= MMC_CAP_SDIO_IRQ | MMC_CAP_ERASE;
/*
* A controller may support 8-bit width, but the board itself
--
1.6.6.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/3]mmc: fix division by zero when cal erase timeout
Since if clock gating feature is enabled, the clock frequency may be zero when host clock is gated. In such scenario, mmc_set_mmc_erase_timeout() may have a division by zero bug. So this patch used mmc_host_clk_rate() to fix this. Signed-off-by: Chuanxiao Dong chuanxiao.d...@intel.com --- drivers/mmc/core/core.c | 10 -- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 0eb27aa..2f4576d 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -1201,8 +1201,14 @@ static void mmc_set_mmc_erase_timeout(struct mmc_card *card, * less but not that much less, so fudge it by multiplying by 2. */ timeout_clks = 1; - timeout_us += (timeout_clks * 1000) / - (card-host-ios.clock / 1000); + + /* +* at this moment, host controller maybe clock gated, so make +* sure we can get a correct host clock freq. +*/ + if (mmc_host_clk_rate(card-host)) + timeout_us += (timeout_clks * 1000) / + (mmc_host_clk_rate(card-host) / 1000); erase_timeout = timeout_us / 1000; -- 1.6.6.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
