Just saw this - am I right in thinking someone has written SDIO support for TMIO ? Im afraid I dont appear to have the preceeding emails. -- Ian Molton Linux, Automotive, and other hacking: http://www.mnementh.co.uk/ On 14 May 2010 09:35, Guennadi Liakhovetski <g.liakhovetski@xxxxxx> wrote: > (added linux-mmc and tmio_mmc maintainer to CC) > > Hi Charles > > On Thu, 13 May 2010, Charles D. Krebs wrote: > >> Guennadi, >> >> I'm currently using the BSP 2.0 release (2.6.33) for the Renesas 7724 >> Ecovec platform. I have two patches (see attached) that enable MMC >> support and SDIO support in that version of the Kernel. > > Great, thanks for the patches! > > Regarding MMCIF, have you seen these patches: > > http://article.gmane.org/gmane.linux.kernel.mmc/1712 > http://article.gmane.org/gmane.linux.ports.sh.devel/7860 > >> I've seen quite a bit of traffic lately over the linux-sh mailing list >> regarding the addition of DMA to the tmio driver. Unfortunately, I >> really have no good idea as to what the status is of that development, >> and whether or not it would apply to both MMC and SDIO modes of >> operation. > > The status is: it passes my tests, as long as cache is in write-through > mode. However, on ecovec I seem to have problems with SD: a simple > mount-compare-overwrite-umount loop aborts even without DMA after a few > hundreds of iterations (under 600 in one of my tests) and it happens even > faster with DMA. However, I haven't repeated this test with the latest > version of the patches, will do it at some point. > >> That said, it looks like at least patch [4/b9 v4] has a few >> parts that accomplish both MMC and SDIO support separately. > > Really? I was unaware of that;) I don't think there is any SDIO specific > code in that or in any other of my patches for tmio_mmc. > >> We have an SDIO based 802.11 b/g/n radio that could definitely benefit >> from DMA support. At this point, what patches I would need to apply, >> and what configuration changes to the kernel are necessary to test this >> functionality out? Are there any known limitations to the new code for >> SDIO support? > > This is the basis patch series: > > http://thread.gmane.org/gmane.linux.kernel.mmc/1780 > > in which patch 4/9 you replace with these three patches: > > http://thread.gmane.org/gmane.linux.ports.sh.devel/8068 > > As for SDIO, there's only one limitation: it's not implemented yet;) But > looking at your patch, it looks like it doesn't touch data paths, only > command paths. So, so far I don't see any reason, why DMA shouldn't work > for you out of the box... However, you seem to have much more knowledge > about tmio unit internals, so, maybe you can just check with your > documentation. As for changes - do not forget to enable the shdma > dmaengine driver. And look in /proc/interrupts during your tests to see if > you're getting any DMA interrupts. You shall see two DMA channels, used > per SDHI controller - for Tx and Rx. > >> We have a team at Redpine Signals in India that is currently working on >> the implementation of the radio driver. I'd like to introduce Sailaja >> Sankabathula, Applications Engineering Manager, who is managing the >> project. I know that the Redpine team is eager to work through a couple >> of issues that they are seeing with the current code that we have, and >> that they are more than happy to help debug the new DMA enabled driver >> code and share the results with the community via any means you think >> best (I'd presume at least by copying the linux-sh mailing list). > > That right, and also the mmc list and the driver maintainer. > >> I'm still rather new to the community development aspect of the Linux kernel - >> Is there somewhere I can read up about the general development procedure >> here and how to best interface with the mailing lists? > > There are a couple of documents in the Linux kernel source tree, that you > might find useful: > > Documentation/SubmitChecklist > Documentation/SubmittingPatches > Documentation/SubmittingDrivers > Documentation/CodingStyle > Documentation/email-clients.txt > >> I tend to feel a little lost in not knowing what version of the kernel >> these patches are referenced to, if the patches are regularly checked >> into the kernel repository, etc. > > Documents under > > Documentation/development-process/ > > should give you an idea. In short: kernel is being developed in multiple > topic trees, dedicated to various architectures, drivers, subsystems, etc. > You work with one or several of those trees, relevant for your > development, submit your patches to respective mailing lists and > maintainers, and during the merge window (first two weeks after a final > kernel version release) those trees get merged into the central kernel > tree. Bug-fixes can be merged in at any time, of course. > > As for your patch, I think, Ian will have a good look at it, so far a > couple of quick comments: > >> diff -Naur sh-2.6_bsp/arch/sh/kernel/cpu/sh4a/setup-sh7724.c sh-2.6_presdio/arch/sh/kernel/cpu/sh4a/setup-sh7724.c >> --- sh-2.6_bsp/arch/sh/kernel/cpu/sh4a/setup-sh7724.c 2010-03-25 04:48:00.000000000 +0000 >> +++ sh-2.6_presdio/arch/sh/kernel/cpu/sh4a/setup-sh7724.c 2010-04-06 07:48:37.000000000 +0000 >> @@ -890,7 +890,7 @@ >> static struct intc_mask_reg mask_registers[] __initdata = { >> { 0xa4080080, 0xa40800c0, 8, /* IMR0 / IMCR0 */ >> { 0, TMU1_TUNI2, TMU1_TUNI1, TMU1_TUNI0, >> - 0, DISABLED, ENABLED, ENABLED } }, >> + 0, ENABLED, ENABLED, ENABLED } }, >> { 0xa4080084, 0xa40800c4, 8, /* IMR1 / IMCR1 */ >> { VIO_VOU, VIO_VEU1, VIO_BEU0, VIO_CEU0, >> DMAC0A_DEI3, DMAC0A_DEI2, DMAC0A_DEI1, DMAC0A_DEI0 } }, >> diff -Naur sh-2.6_bsp/drivers/mfd/sh_mobile_sdhi.c sh-2.6_presdio/drivers/mfd/sh_mobile_sdhi.c >> --- sh-2.6_bsp/drivers/mfd/sh_mobile_sdhi.c 2010-03-25 04:48:00.000000000 +0000 >> +++ sh-2.6_presdio/drivers/mfd/sh_mobile_sdhi.c 2010-04-06 07:48:37.000000000 +0000 >> @@ -97,7 +97,7 @@ >> >> priv->mmc_data.hclk = clk_get_rate(priv->clk); >> priv->mmc_data.set_pwr = sh_mobile_sdhi_set_pwr; >> - priv->mmc_data.capabilities = MMC_CAP_MMC_HIGHSPEED; >> + priv->mmc_data.capabilities = MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SDIO_IRQ; >> >> memcpy(&priv->cell_mmc, &sh_mobile_sdhi_cell, sizeof(priv->cell_mmc)); >> priv->cell_mmc.driver_data = &priv->mmc_data; >> diff -Naur sh-2.6_bsp/drivers/mmc/host/tmio_mmc.c sh-2.6_presdio/drivers/mmc/host/tmio_mmc.c >> --- sh-2.6_bsp/drivers/mmc/host/tmio_mmc.c 2010-03-25 04:48:00.000000000 +0000 >> +++ sh-2.6_presdio/drivers/mmc/host/tmio_mmc.c 2010-04-06 07:48:37.000000000 +0000 >> @@ -30,6 +30,7 @@ >> #include <linux/device.h> >> #include <linux/delay.h> >> #include <linux/mmc/host.h> >> +#include <linux/mmc/sdio.h> >> #include <linux/mfd/core.h> >> #include <linux/mfd/tmio.h> >> >> @@ -146,6 +147,28 @@ >> c |= TRANSFER_READ; >> } >> >> + switch (cmd->opcode) { >> + case SD_IO_RW_DIRECT: >> + sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001); >> + enable_mmc_sdio_irqs(host, TMIO_SDIO_STAT_IOIRQ); >> + break; >> + case SD_IO_RW_EXTENDED: >> + sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001); >> + enable_mmc_sdio_irqs(host, TMIO_SDIO_STAT_IOIRQ); >> + if (data->flags & MMC_DATA_READ) { >> + if (data->sg->length <= 512) >> + c = CMD_SD_IO_RW_EXTENDED_SREAD; >> + else >> + c = CMD_SD_IO_RW_EXTENDED_MREAD; >> + } else { /* MMC_DATA_WRITE */ >> + if (data->sg->length <= 512) >> + c = CMD_SD_IO_RW_EXTENDED_SWRITE; >> + else >> + c = CMD_SD_IO_RW_EXTENDED_MWRITE; >> + } >> + break; >> + } >> + >> enable_mmc_irqs(host, TMIO_MASK_CMD); >> >> /* Fire off the command */ >> @@ -235,8 +258,13 @@ >> if (stop) { >> if (stop->opcode == 12 && !stop->arg) >> sd_ctrl_write16(host, CTL_STOP_INTERNAL_ACTION, 0x000); >> +#if 0 > > I realise, this wasn't a final patch submission, but just a remark - you > don't want commented out code in the final version > >> + /* CMD_SD_IO_RW_EXTENDED_MREAD and CMD_SD_IO_RW_EXTENDED_MWRITE >> + * are not necessary. >> + */ > > multi-line comment style is wrong. Should have been > > + /* > + * CMD_SD_IO_RW_EXTENDED_MREAD and CMD_SD_IO_RW_EXTENDED_MWRITE > + * are not necessary. > + */ > >> else >> BUG(); >> +#endif > > But essentially, maybe you can preserve this check and just replace: > >> - BUG(); >> + BUG_ON(host->cmd->opcode != SD_IO_RW_EXTENDED); > > or something similar. > >> } >> >> tmio_mmc_finish_request(host); >> @@ -298,6 +326,7 @@ >> { >> struct tmio_mmc_host *host = devid; >> unsigned int ireg, irq_mask, status; >> + unsigned short ireg_io, irq_mask_io, status_io; > > I would use u16 to explicitly specify 16-bit data. > >> >> pr_debug("MMC IRQ begin\n"); >> >> @@ -305,15 +334,31 @@ >> irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK); >> ireg = status & TMIO_MASK_IRQ & ~irq_mask; >> >> + status_io = sd_ctrl_read16(host, CTL_SDIO_STATUS); >> + irq_mask_io = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK); >> + ireg_io = status_io & TMIO_SDIO_MASK_IRQ & ~irq_mask_io; >> + >> pr_debug_status(status); >> pr_debug_status(ireg); >> >> if (!ireg) { >> + if (ireg_io & TMIO_SDIO_STAT_IOIRQ) { >> + ack_mmc_sdio_irqs(host, TMIO_SDIO_STAT_IOIRQ); >> + mmc_signal_sdio_irq(host->mmc); >> + } else if (ireg_io & TMIO_SDIO_STAT_EXWT) >> + ack_mmc_sdio_irqs(host, TMIO_SDIO_STAT_EXWT); >> + else if (ireg_io & TMIO_SDIO_STAT_EXPUB52) >> + ack_mmc_sdio_irqs(host, TMIO_SDIO_STAT_EXPUB52); >> + else >> + ack_mmc_sdio_irqs(host, TMIO_SDIO_STAT_RESERVE); >> + >> +#if 0 >> disable_mmc_irqs(host, status & ~irq_mask); >> >> pr_debug("tmio_mmc: Spurious irq, disabling! " >> "0x%08x 0x%08x 0x%08x\n", status, irq_mask, ireg); >> pr_debug_status(status); >> +#endif > > Same here, I think, you can preserve this interrupt disabling, if you > extend the test to mean "no SD and no SDIO bit is set." > >> >> goto out; >> } >> @@ -321,6 +366,12 @@ >> while (ireg) { >> /* Card insert / remove attempts */ >> if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) { >> + if (ireg & TMIO_STAT_CARD_REMOVE) { >> + sd_ctrl_write16(host, >> + CTL_TRANSACTION_CTL, 0x0000); >> + disable_mmc_sdio_irqs(host, >> + TMIO_SDIO_STAT_IOIRQ); >> + } >> ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT | >> TMIO_STAT_CARD_REMOVE); >> mmc_detect_change(host->mmc, msecs_to_jiffies(100)); >> @@ -462,10 +513,20 @@ >> return (sd_ctrl_read16(host, CTL_STATUS) & TMIO_STAT_WRPROTECT) ? 0 : 1; >> } >> >> +static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) >> +{ >> + struct tmio_mmc_host *host = mmc_priv(mmc); >> + if (enable) >> + enable_mmc_sdio_irqs(host, TMIO_SDIO_STAT_IOIRQ); >> + else >> + disable_mmc_sdio_irqs(host, TMIO_SDIO_STAT_IOIRQ); >> +} >> + >> static struct mmc_host_ops tmio_mmc_ops = { >> .request = tmio_mmc_request, >> .set_ios = tmio_mmc_set_ios, >> .get_ro = tmio_mmc_get_ro, >> + .enable_sdio_irq = tmio_mmc_enable_sdio_irq, > > Don't be shy, add one more TAB to all struct members to align them > properly;) > >> }; >> >> #ifdef CONFIG_PM >> diff -Naur sh-2.6_bsp/drivers/mmc/host/tmio_mmc.h sh-2.6_presdio/drivers/mmc/host/tmio_mmc.h >> --- sh-2.6_bsp/drivers/mmc/host/tmio_mmc.h 2010-03-25 04:48:00.000000000 +0000 >> +++ sh-2.6_presdio/drivers/mmc/host/tmio_mmc.h 2010-04-06 07:48:37.000000000 +0000 >> @@ -24,6 +24,8 @@ >> #define CTL_SD_ERROR_DETAIL_STATUS 0x2c >> #define CTL_SD_DATA_PORT 0x30 >> #define CTL_TRANSACTION_CTL 0x34 >> +#define CTL_SDIO_STATUS 0x36 >> +#define CTL_SDIO_IRQ_MASK 0x38 >> #define CTL_RESET_SD 0xe0 >> #define CTL_SDIO_REGS 0x100 >> #define CTL_CLK_AND_WAIT_CTL 0x138 >> @@ -62,6 +64,23 @@ >> #define TMIO_MASK_IRQ (TMIO_MASK_READOP | TMIO_MASK_WRITEOP | TMIO_MASK_CMD) >> >> >> +/* Definitions for values the CTRL_SDIO_STATUS register can take. */ >> +#define TMIO_SDIO_STAT_RESERVE 0x0006 /* reserve bit */ >> +#define TMIO_SDIO_STAT_IOIRQ 0x0001 >> +#define TMIO_SDIO_STAT_EXPUB52 0x4000 >> +#define TMIO_SDIO_STAT_EXWT 0x8000 >> + >> +#define CMD_SD_IO_RW_EXTENDED_SREAD 0x1C35 >> +#define CMD_SD_IO_RW_EXTENDED_MREAD 0x7C35 >> +#define CMD_SD_IO_RW_EXTENDED_SWRITE 0x0C35 >> +#define CMD_SD_IO_RW_EXTENDED_MWRITE 0x6C35 > > You can put these four lines in tmio_mmc.c, and maybe "SDIO" without an > underscore? > >> + >> +/* Define some SDIO IRQ masks */ >> +#define TMIO_SDIO_MASK_IRQ (TMIO_SDIO_STAT_IOIRQ | \ >> + TMIO_SDIO_STAT_EXPUB52 | \ >> + TMIO_SDIO_STAT_EXWT | \ >> + TMIO_SDIO_STAT_RESERVE) >> + >> #define enable_mmc_irqs(host, i) \ >> do { \ >> u32 mask;\ >> @@ -86,6 +105,29 @@ >> sd_ctrl_write32((host), CTL_STATUS, mask); \ >> } while (0) >> >> +#define enable_mmc_sdio_irqs(host, i) \ >> + do { \ >> + u16 mask;\ >> + mask = sd_ctrl_read16((host), CTL_SDIO_IRQ_MASK); \ >> + mask &= ~((i) & TMIO_SDIO_MASK_IRQ); \ >> + sd_ctrl_write16((host), CTL_SDIO_IRQ_MASK, mask); \ >> + } while (0) >> + >> +#define disable_mmc_sdio_irqs(host, i) \ >> + do { \ >> + u16 mask;\ >> + mask = sd_ctrl_read16((host), CTL_SDIO_IRQ_MASK); \ >> + mask |= ((i) & TMIO_SDIO_MASK_IRQ); \ >> + sd_ctrl_write16((host), CTL_SDIO_IRQ_MASK, mask); \ >> + } while (0) >> + >> +#define ack_mmc_sdio_irqs(host, i) \ >> + do { \ >> + u16 mask;\ >> + mask = sd_ctrl_read16((host), CTL_SDIO_STATUS); \ >> + mask &= ~((i) & TMIO_SDIO_MASK_IRQ); \ >> + sd_ctrl_write16((host), CTL_SDIO_STATUS, mask); \ >> + } while (0) >> >> struct tmio_mmc_host { >> void __iomem *ctl; > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
