Re: [PATCH v3] MMC: Add JZ4740 mmc driver
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Matt Fleming wrote: > On Mon, 28 Jun 2010 03:20:41 +0200, Lars-Peter Clausen > wrote: >> This patch adds support for the mmc controller on JZ4740 SoCs. >> >> Signed-off-by: Lars-Peter Clausen >> Cc: Andrew Morton >> Cc: Matt Fleming >> Cc: linux-mmc@vger.kernel.org >> >> --- >> Changes since v1 >> - Do not request IRQ with IRQF_DISABLED since it is a noop now >> - Use a generous slack for the timeout timer. It does not need to be >> accurate. >> Changes since v2 >> - Use sg_mapping_to iterate over sg elements in mmc read and write functions >> - Use bitops instead of a spinlock and a variable for testing whether a >> request >> has been finished. >> - Rework irq and timeout handling in order to get rid of locking in hot paths > > Acked-by: Matt Fleming > > Are you planning on maintaining this driver? If so, it'd be a good idea > to update MAINTAINERS. Hi Thanks for reviewing the patch. I guess I should send a MAINTAINERS patch which adds entries for all of the JZ4740 drivers. - - Lars -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iEYEARECAAYFAkwsuJUACgkQBX4mSR26RiNHVgCfTq+tc2I1QBniqijyUjNDxPIX GsEAn1xgPWz+L0uqHWthzJ+lMtFaUBtY =nVt9 -END PGP SIGNATURE- -- 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 v3] MMC: Add JZ4740 mmc driver
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Hi
Andrew Morton wrote:
> On Mon, 28 Jun 2010 03:20:41 +0200
> Lars-Peter Clausen wrote:
>
>> This patch adds support for the mmc controller on JZ4740 SoCs.
>>
>> Signed-off-by: Lars-Peter Clausen
>> Cc: Andrew Morton
>> Cc: Matt Fleming
>> Cc: linux-mmc@vger.kernel.org
>>
>> ...
>>
>> +#define JZ4740_MMC_MAX_TIMEOUT 1000
>
> That was a really big timeout. How long do 1e7 readw's take? Oh well.
>
Hm, right. It doesn't hurt though. I'll do some tests and to try to come up
with a
more realistic value.
>> ...
>>
>> +static void jz4740_mmc_clock_disable(struct jz4740_mmc_host *host)
>> +{
>> +uint32_t status;
>> +
>> +writew(JZ_MMC_STRPCL_CLOCK_STOP, host->base + JZ_REG_MMC_STRPCL);
>> +do {
>> +status = readl(host->base + JZ_REG_MMC_STATUS);
>> +} while (status & JZ_MMC_STATUS_CLK_EN);
>> +}
>> +
>> +static void jz4740_mmc_reset(struct jz4740_mmc_host *host)
>> +{
>> +uint32_t status;
>> +
>> +writew(JZ_MMC_STRPCL_RESET, host->base + JZ_REG_MMC_STRPCL);
>> +udelay(10);
>> +do {
>> +status = readl(host->base + JZ_REG_MMC_STATUS);
>> +} while (status & JZ_MMC_STATUS_IS_RESETTING);
>> +}
>
> Maybe these should have a timeout too?
Its very unlikely that these will ever timeout. But right, to be on the safe,
there
should be a timeout as well.
>
>> ...
>>
>> +static inline unsigned int jz4740_mmc_wait_irq(struct jz4740_mmc_host *host,
>> +unsigned int irq)
>> +{
>> +unsigned int timeout = JZ4740_MMC_MAX_TIMEOUT;
>> +uint16_t status;
>> +
>> +do {
>> +status = readw(host->base + JZ_REG_MMC_IREG);
>> +} while (!(status & irq) && --timeout);
>> +
>> +return timeout;
>> +}
>
> This guy's too big to inline. Recent gcc's know that and they tend to
> uninline such things behind your back anwyay.
Actually, even without the inline attribute and compiling with -Os gcc will
inline
this function by itself.
>
>> ...
>>
>> +struct jz4740_mmc_platform_data {
>> +int gpio_power;
>> +int gpio_card_detect;
>> +int gpio_read_only;
>> +unsigned card_detect_active_low:1;
>> +unsigned read_only_active_low:1;
>> +unsigned power_active_low:1;
>> +
>> +unsigned data_1bit:1;
>> +};
>
> The bitfields will all share the same word, so modification of one
> field can race against modification of another field. Hence some form
> of locking which covers *all* the bitfields is needed.
>
> Is that a problem in this driver?
>
They are all read-only in the driver.
Thanks for reviewing the patch
- - Lars
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkwsuDQACgkQBX4mSR26RiOiMACeMMNj4koCYFAUnxyM0LBr+wOZ
x6oAnizk5CaAvZjQ2doXrD6ZYgDeNjSr
=92D2
-END PGP SIGNATURE-
--
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 v3] MMC: Add JZ4740 mmc driver
On Mon, 28 Jun 2010 03:20:41 +0200
Lars-Peter Clausen wrote:
> This patch adds support for the mmc controller on JZ4740 SoCs.
>
> Signed-off-by: Lars-Peter Clausen
> Cc: Andrew Morton
> Cc: Matt Fleming
> Cc: linux-mmc@vger.kernel.org
>
> ...
>
> +#define JZ4740_MMC_MAX_TIMEOUT 1000
That was a really big timeout. How long do 1e7 readw's take? Oh well.
>
> ...
>
> +static void jz4740_mmc_clock_disable(struct jz4740_mmc_host *host)
> +{
> + uint32_t status;
> +
> + writew(JZ_MMC_STRPCL_CLOCK_STOP, host->base + JZ_REG_MMC_STRPCL);
> + do {
> + status = readl(host->base + JZ_REG_MMC_STATUS);
> + } while (status & JZ_MMC_STATUS_CLK_EN);
> +}
> +
> +static void jz4740_mmc_reset(struct jz4740_mmc_host *host)
> +{
> + uint32_t status;
> +
> + writew(JZ_MMC_STRPCL_RESET, host->base + JZ_REG_MMC_STRPCL);
> + udelay(10);
> + do {
> + status = readl(host->base + JZ_REG_MMC_STATUS);
> + } while (status & JZ_MMC_STATUS_IS_RESETTING);
> +}
Maybe these should have a timeout too?
>
> ...
>
> +static inline unsigned int jz4740_mmc_wait_irq(struct jz4740_mmc_host *host,
> + unsigned int irq)
> +{
> + unsigned int timeout = JZ4740_MMC_MAX_TIMEOUT;
> + uint16_t status;
> +
> + do {
> + status = readw(host->base + JZ_REG_MMC_IREG);
> + } while (!(status & irq) && --timeout);
> +
> + return timeout;
> +}
This guy's too big to inline. Recent gcc's know that and they tend to
uninline such things behind your back anwyay.
>
> ...
>
> +struct jz4740_mmc_platform_data {
> + int gpio_power;
> + int gpio_card_detect;
> + int gpio_read_only;
> + unsigned card_detect_active_low:1;
> + unsigned read_only_active_low:1;
> + unsigned power_active_low:1;
> +
> + unsigned data_1bit:1;
> +};
The bitfields will all share the same word, so modification of one
field can race against modification of another field. Hence some form
of locking which covers *all* the bitfields is needed.
Is that a problem in this driver?
--
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 v3] MMC: Add JZ4740 mmc driver
On Mon, 28 Jun 2010 03:20:41 +0200, Lars-Peter Clausen wrote: > This patch adds support for the mmc controller on JZ4740 SoCs. > > Signed-off-by: Lars-Peter Clausen > Cc: Andrew Morton > Cc: Matt Fleming > Cc: linux-mmc@vger.kernel.org > > --- > Changes since v1 > - Do not request IRQ with IRQF_DISABLED since it is a noop now > - Use a generous slack for the timeout timer. It does not need to be accurate. > Changes since v2 > - Use sg_mapping_to iterate over sg elements in mmc read and write functions > - Use bitops instead of a spinlock and a variable for testing whether a > request > has been finished. > - Rework irq and timeout handling in order to get rid of locking in hot paths Acked-by: Matt Fleming Are you planning on maintaining this driver? If so, it'd be a good idea to update MAINTAINERS. -- 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 v3] MMC: Add JZ4740 mmc driver
This patch adds support for the mmc controller on JZ4740 SoCs. Signed-off-by: Lars-Peter Clausen Cc: Andrew Morton Cc: Matt Fleming Cc: linux-mmc@vger.kernel.org --- Changes since v1 - Do not request IRQ with IRQF_DISABLED since it is a noop now - Use a generous slack for the timeout timer. It does not need to be accurate. Changes since v2 - Use sg_mapping_to iterate over sg elements in mmc read and write functions - Use bitops instead of a spinlock and a variable for testing whether a request has been finished. - Rework irq and timeout handling in order to get rid of locking in hot paths --- drivers/mmc/host/Kconfig |8 + drivers/mmc/host/Makefile |1 + drivers/mmc/host/jz4740_mmc.c | 969 include/linux/mmc/jz4740_mmc.h | 15 + 4 files changed, 993 insertions(+), 0 deletions(-) create mode 100644 drivers/mmc/host/jz4740_mmc.c create mode 100644 include/linux/mmc/jz4740_mmc.h diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index f06d06e..546fc49 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -81,6 +81,14 @@ config MMC_RICOH_MMC If unsure, say Y. +config MMC_JZ4740 + tristate "JZ4740 SD/Multimedia Card Interface support" + depends on MACH_JZ4740 + help + This selects the Ingenic Z4740 SD/Multimedia card Interface. + If you have an ngenic platform with a Multimedia Card slot, + say Y or M here. + config MMC_SDHCI_OF tristate "SDHCI support on OpenFirmware platforms" depends on MMC_SDHCI && PPC_OF diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile index e30c2ee..f4e53c9 100644 --- a/drivers/mmc/host/Makefile +++ b/drivers/mmc/host/Makefile @@ -36,6 +36,7 @@ obj-$(CONFIG_MMC_CB710) += cb710-mmc.o obj-$(CONFIG_MMC_VIA_SDMMC)+= via-sdmmc.o obj-$(CONFIG_SDH_BFIN) += bfin_sdh.o obj-$(CONFIG_MMC_SH_MMCIF) += sh_mmcif.o +obj-$(CONFIG_MMC_JZ4740) += jz4740_mmc.o obj-$(CONFIG_MMC_SDHCI_OF) += sdhci-of.o sdhci-of-y := sdhci-of-core.o diff --git a/drivers/mmc/host/jz4740_mmc.c b/drivers/mmc/host/jz4740_mmc.c new file mode 100644 index 000..b34f25b --- /dev/null +++ b/drivers/mmc/host/jz4740_mmc.c @@ -0,0 +1,969 @@ +/* + * Copyright (C) 2009-2010, Lars-Peter Clausen + * JZ4740 SD/MMC controller driver + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 675 Mass Ave, Cambridge, MA 02139, USA. + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + + +#define JZ_REG_MMC_STRPCL 0x00 +#define JZ_REG_MMC_STATUS 0x04 +#define JZ_REG_MMC_CLKRT 0x08 +#define JZ_REG_MMC_CMDAT 0x0C +#define JZ_REG_MMC_RESTO 0x10 +#define JZ_REG_MMC_RDTO0x14 +#define JZ_REG_MMC_BLKLEN 0x18 +#define JZ_REG_MMC_NOB 0x1C +#define JZ_REG_MMC_SNOB0x20 +#define JZ_REG_MMC_IMASK 0x24 +#define JZ_REG_MMC_IREG0x28 +#define JZ_REG_MMC_CMD 0x2C +#define JZ_REG_MMC_ARG 0x30 +#define JZ_REG_MMC_RESP_FIFO 0x34 +#define JZ_REG_MMC_RXFIFO 0x38 +#define JZ_REG_MMC_TXFIFO 0x3C + +#define JZ_MMC_STRPCL_EXIT_MULTIPLE BIT(7) +#define JZ_MMC_STRPCL_EXIT_TRANSFER BIT(6) +#define JZ_MMC_STRPCL_START_READWAIT BIT(5) +#define JZ_MMC_STRPCL_STOP_READWAIT BIT(4) +#define JZ_MMC_STRPCL_RESET BIT(3) +#define JZ_MMC_STRPCL_START_OP BIT(2) +#define JZ_MMC_STRPCL_CLOCK_CONTROL (BIT(1) | BIT(0)) +#define JZ_MMC_STRPCL_CLOCK_STOP BIT(0) +#define JZ_MMC_STRPCL_CLOCK_START BIT(1) + + +#define JZ_MMC_STATUS_IS_RESETTING BIT(15) +#define JZ_MMC_STATUS_SDIO_INT_ACTIVE BIT(14) +#define JZ_MMC_STATUS_PRG_DONE BIT(13) +#define JZ_MMC_STATUS_DATA_TRAN_DONE BIT(12) +#define JZ_MMC_STATUS_END_CMD_RES BIT(11) +#define JZ_MMC_STATUS_DATA_FIFO_AFULL BIT(10) +#define JZ_MMC_STATUS_IS_READWAIT BIT(9) +#define JZ_MMC_STATUS_CLK_EN BIT(8) +#define JZ_MMC_STATUS_DATA_FIFO_FULL BIT(7) +#define JZ_MMC_STATUS_DATA_FIFO_EMPTY BIT(6) +#define JZ_MMC_STATUS_CRC_RES_ERR BIT(5) +#define JZ_MMC_STATUS_CRC_READ_ERROR BIT(4) +#define JZ_MMC_STATUS_TIMEOUT_WRITE BIT(3) +#define JZ_MMC_STATUS_CRC_WRITE_ERROR BIT(2) +#define JZ_MMC_STATUS_TIMEOUT_RES BIT(1) +#define JZ_MMC_STATUS_TIMEOUT_READ BIT(0) + +#define JZ_MMC_STATUS_READ_ERROR_MASK (BIT(4) | BIT(0)) +#define JZ_MMC_STATUS_WRITE_ERROR_MASK (BIT(3) | BIT(2)) + + +#define JZ_MMC_CMDAT_IO_ABORT BIT(11) +#define JZ_MMC_CMDAT_BUS_WIDTH_4BIT BIT(10) +#define JZ_MMC_CMDAT_
