LinuxLists.cc - [patch] split MMC_CAP_4_BIT

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Tue, 8 Jan 2008 13:29:39 -0500
Mike Frysinger <[email protected]> wrote:

> The on-chip Blackfin MMC/SD/SDIO host controller has the ability to do 1-bit
> MMC, 1-bit/4-bit SD, and 1-bit/4-bit SDIO. Thus the current convention of
> MMC_CAP_4_BIT_DATA meaning "your host controller can do 1-bit or 4-bit for all
> modes" is insufficient for our needs. The attached patch splits
> MMC_CAP_4_BIT_DATA into MMC_CAP_MMC_4_BIT_DATA and MMC_CAP_SD_4_BIT_DATA and
> updates all host controllers to include these in their caps and then changes
> existing code to check the new defines. At the moment, SD/SDIO are lumped
> into MMC_CAP_SD_4_BIT_DATA ... should I bother with splitting that into SD and
> SDIO as well while I'm doing this ?
>
> Signed-off-by: Mike Frysinger <[email protected]>

I fail to see why you need to split MMC and SD. Could you elaborate why the controller won't work with MMC cards? I haven't seen any differences from SD.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-08 19:41:09

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Tuesday 08 January 2008 14:21:57 Pierre Ossman wrote:
> Mike Frysinger <[email protected]> wrote:
> > The on-chip Blackfin MMC/SD/SDIO host controller has the ability to do
> > 1-bit MMC, 1-bit/4-bit SD, and 1-bit/4-bit SDIO. Thus the current
> > convention of MMC_CAP_4_BIT_DATA meaning "your host controller can do
> > 1-bit or 4-bit for all modes" is insufficient for our needs. The
> > attached patch splits MMC_CAP_4_BIT_DATA into MMC_CAP_MMC_4_BIT_DATA and
> > MMC_CAP_SD_4_BIT_DATA and updates all host controllers to include these
> > in their caps and then changes existing code to check the new defines.
> > At the moment, SD/SDIO are lumped into MMC_CAP_SD_4_BIT_DATA ... should I
> > bother with splitting that into SD and SDIO as well while I'm doing this
> > ?
> >
> > Signed-off-by: Mike Frysinger <[email protected]>
>
> I fail to see why you need to split MMC and SD. Could you elaborate why the
> controller won't work with MMC cards? I haven't seen any differences from
> SD.

i dont understand what's confusing. the Blackfin on chip host controller only
supports 1-bit MMC, but it supports 4-bit SD/SDIO. this is a fact. while it
may be a stupid decision, it is what it is, and i need the framework made
more flexible in order to get the Blackfin driver merged cleanly. we do
software for hardware, we dont do hardware.
-mike

2008-01-08 20:49:59

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Tue, 8 Jan 2008 14:40:49 -0500
Mike Frysinger <[email protected]> wrote:

>
> i dont understand what's confusing. the Blackfin on chip host controller only
> supports 1-bit MMC, but it supports 4-bit SD/SDIO. this is a fact. while it
> may be a stupid decision, it is what it is, and i need the framework made
> more flexible in order to get the Blackfin driver merged cleanly. we do
> software for hardware, we dont do hardware.

Well, since I've seen no _hardware_ differences between 4-bit MMC and 4-bit SD, "support" in this case must me "vendor will guarantee it works". And that is not the kind of "support" that needs a distinction in the code.

So, again, if you feel that there is a hardware difference between 4-bit MMC and 4-bit SD then please elaborate as it is my understanding that they are identical.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-08 21:44:24

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 8, 2008 3:49 PM, Pierre Ossman <[email protected]> wrote:
> On Tue, 8 Jan 2008 14:40:49 -0500
> Mike Frysinger <[email protected]> wrote:
> > i dont understand what's confusing. the Blackfin on chip host controller only
> > supports 1-bit MMC, but it supports 4-bit SD/SDIO. this is a fact. while it
> > may be a stupid decision, it is what it is, and i need the framework made
> > more flexible in order to get the Blackfin driver merged cleanly. we do
> > software for hardware, we dont do hardware.
>
> Well, since I've seen no _hardware_ differences between 4-bit MMC and 4-bit SD, "support" in this case must me "vendor will guarantee it works". And that is not the kind of "support" that needs a distinction in the code.

regardless of where the limitation is, the change is necessary.
however you wish to classify the issue, it doesnt matter to me. the
Blackfin controller needs to declare the following capabilities:
- 1-bit MMC
- 1-bit SD / 4-bit SD
- 1-bit SDIO / 4-bit SDIO
the existing framework is not flexible enough to account for this,
thus the patch posted.

> So, again, if you feel that there is a hardware difference between 4-bit MMC and 4-bit SD then please elaborate as it is my understanding that they are identical.

you may be 100% correct, i have no idea, i'm not really familiar with
MMC/SD/SDIO at all.
-mike

2008-01-09 02:33:07

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 9, 2008 4:49 AM, Pierre Ossman <[email protected]> wrote:
> On Tue, 8 Jan 2008 14:40:49 -0500
> Mike Frysinger <[email protected]> wrote:
>
> >
> > i dont understand what's confusing. the Blackfin on chip host controller only
> > supports 1-bit MMC, but it supports 4-bit SD/SDIO. this is a fact. while it
> > may be a stupid decision, it is what it is, and i need the framework made
> > more flexible in order to get the Blackfin driver merged cleanly. we do
> > software for hardware, we dont do hardware.
>
> Well, since I've seen no _hardware_ differences between 4-bit MMC and 4-bit SD, "support" in this case must me "vendor will guarantee it works". And that is not the kind of "support" that needs a distinction in the code.
>
> So, again, if you feel that there is a hardware difference between 4-bit MMC and 4-bit SD then please elaborate as it is my understanding that they are identical.
>

As Mike said, the reason split this flag is because Blackfin on-chip
SDIO controller's limitation.
Cliff is working on it for a long time, so I dropped him in. Hope he
can clarify the confusing things.

Thanks
-Bryan Wu

2008-01-09 03:30:27

by Cai, Cliff

[permalink] [raw]

Subject: RE: [patch] split MMC_CAP_4_BIT_DATA

Hi,all

I'd like to say something about this issue.
Currently,the blackfin on chip SD host ONLY support 1-bit MMC while
support 1-bit/4-bit SD/SDIO.
And we want our driver to support both 1-bit MMC and 4-bit SD/SDIO.but
the current MMC driver framework
Only allow us to set one kind of bus width,either 1-bit or 4-bit.So in
order to meet our case,we need more flexible mechanism
To inform the upper commom driver to know our situation.

Cliff

-----Original Message-----
From: Bryan Wu [mailto:[email protected]]
Sent: Wednesday, January 09, 2008 10:33 AM
To: Pierre Ossman; [email protected]
Cc: Mike Frysinger; [email protected]
Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 9, 2008 4:49 AM, Pierre Ossman <[email protected]> wrote:
> On Tue, 8 Jan 2008 14:40:49 -0500
> Mike Frysinger <[email protected]> wrote:
>
> >
> > i dont understand what's confusing. the Blackfin on chip host
> > controller only supports 1-bit MMC, but it supports 4-bit SD/SDIO.
> > this is a fact. while it may be a stupid decision, it is what it
> > is, and i need the framework made more flexible in order to get the
> > Blackfin driver merged cleanly. we do software for hardware, we
dont do hardware.
>
> Well, since I've seen no _hardware_ differences between 4-bit MMC and
4-bit SD, "support" in this case must me "vendor will guarantee it
works". And that is not the kind of "support" that needs a distinction
in the code.
>
> So, again, if you feel that there is a hardware difference between
4-bit MMC and 4-bit SD then please elaborate as it is my understanding
that they are identical.
>

As Mike said, the reason split this flag is because Blackfin on-chip
SDIO controller's limitation.
Cliff is working on it for a long time, so I dropped him in. Hope he can
clarify the confusing things.

Thanks
-Bryan Wu

2008-01-09 04:23:18

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 9, 2008 2:29 AM, Mike Frysinger <[email protected]> wrote:
> The on-chip Blackfin MMC/SD/SDIO host controller has the ability to do 1-bit
> MMC, 1-bit/4-bit SD, and 1-bit/4-bit SDIO. Thus the current convention of
> MMC_CAP_4_BIT_DATA meaning "your host controller can do 1-bit or 4-bit for all
> modes" is insufficient for our needs. The attached patch splits
> MMC_CAP_4_BIT_DATA into MMC_CAP_MMC_4_BIT_DATA and MMC_CAP_SD_4_BIT_DATA and
> updates all host controllers to include these in their caps and then changes
> existing code to check the new defines. At the moment, SD/SDIO are lumped
> into MMC_CAP_SD_4_BIT_DATA ... should I bother with splitting that into SD and
> SDIO as well while I'm doing this ?
>
> Signed-off-by: Mike Frysinger <[email protected]>
> ---
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 68c0e3b..ca12db7 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -397,7 +397,7 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
> * Activate wide bus (if supported).
> */
> if ((card->csd.mmca_vsn >= CSD_SPEC_VER_4) &&
> - (host->caps & MMC_CAP_4_BIT_DATA)) {
> + (host->caps & MMC_CAP_MMC_4_BIT_DATA)) {
> err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> EXT_CSD_BUS_WIDTH, EXT_CSD_BUS_WIDTH_4);
> if (err)
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index d1c1e0f..974b63d 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -441,7 +441,7 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
> /*
> * Switch to wider bus (if supported).
> */
> - if ((host->caps & MMC_CAP_4_BIT_DATA) &&
> + if ((host->caps & MMC_CAP_SD_4_BIT_DATA) &&
> (card->scr.bus_widths & SD_SCR_BUS_WIDTH_4)) {
> err = mmc_app_set_bus_width(card, MMC_BUS_WIDTH_4);
> if (err)
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 87a50f4..1d389c8 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -143,7 +143,7 @@ static int sdio_enable_wide(struct mmc_card *card)
> int ret;
> u8 ctrl;
>
> - if (!(card->host->caps & MMC_CAP_4_BIT_DATA))
> + if (!(card->host->caps & MMC_CAP_SD_4_BIT_DATA))
> return 0;
>
> if (card->cccr.low_speed && !card->cccr.wide_bus)
> diff --git a/drivers/mmc/host/at91_mci.c b/drivers/mmc/host/at91_mci.c
> index b1edcef..63d89b0 100644
> --- a/drivers/mmc/host/at91_mci.c
> +++ b/drivers/mmc/host/at91_mci.c
> @@ -851,7 +851,7 @@ static int __init at91_mci_probe(struct platform_device *pdev)
> host->board = pdev->dev.platform_data;
> if (host->board->wire4) {
> if (cpu_is_at91sam9260() || cpu_is_at91sam9263())
> - mmc->caps |= MMC_CAP_4_BIT_DATA;
> + mmc->caps |= (MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA);
> else
> printk("AT91 MMC: 4 wire bus mode not supported"
> " - using 1 wire\n");
> diff --git a/drivers/mmc/host/imxmmc.c b/drivers/mmc/host/imxmmc.c
> index f2070a1..67d4bc0 100644
> --- a/drivers/mmc/host/imxmmc.c
> +++ b/drivers/mmc/host/imxmmc.c
> @@ -975,7 +975,7 @@ static int imxmci_probe(struct platform_device *pdev)
> mmc->f_min = 150000;
> mmc->f_max = CLK_RATE/2;
> mmc->ocr_avail = MMC_VDD_32_33;
> - mmc->caps = MMC_CAP_4_BIT_DATA;
> + mmc->caps = (MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA);
>
> /* MMC core transfer sizes tunable parameters */
> mmc->max_hw_segs = 64;
> diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c
> index 971e18b..b1ae793 100644
> --- a/drivers/mmc/host/omap.c
> +++ b/drivers/mmc/host/omap.c
> @@ -1079,7 +1079,7 @@ static int __init mmc_omap_probe(struct platform_device *pdev)
> mmc->caps = MMC_CAP_MULTIWRITE | MMC_CAP_BYTEBLOCK;
>
> if (minfo->wire4)
> - mmc->caps |= MMC_CAP_4_BIT_DATA;
> + mmc->caps |= (MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA);
>
> /* Use scatterlist DMA to reduce per-transfer costs.
> * NOTE max_seg_size assumption that small blocks aren't
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 1654a33..4fa00f1 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -527,7 +527,7 @@ static int pxamci_probe(struct platform_device *pdev)
> mmc->caps = 0;
> host->cmdat = 0;
> if (!cpu_is_pxa21x() && !cpu_is_pxa25x()) {
> - mmc->caps |= MMC_CAP_4_BIT_DATA | MMC_CAP_SDIO_IRQ;
> + mmc->caps |= MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA | MMC_CAP_SDIO_IRQ;
> host->cmdat |= CMDAT_SDIO_INT_EN;
> }
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 785bbdc..09c4358 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1405,7 +1405,7 @@ static int __devinit sdhci_probe_slot(struct pci_dev *pdev, int slot)
> mmc->ops = &sdhci_ops;
> mmc->f_min = host->max_clk / 256;
> mmc->f_max = host->max_clk;
> - mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_MULTIWRITE | MMC_CAP_SDIO_IRQ;
> + mmc->caps = MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA | MMC_CAP_MULTIWRITE | MMC_CAP_SDIO_IRQ;
>
> if (caps & SDHCI_CAN_DO_HISPD)
> mmc->caps |= MMC_CAP_SD_HIGHSPEED;
> diff --git a/drivers/mmc/host/tifm_sd.c b/drivers/mmc/host/tifm_sd.c
> index 20d5c7b..78cad6b 100644
> --- a/drivers/mmc/host/tifm_sd.c
> +++ b/drivers/mmc/host/tifm_sd.c
> @@ -973,7 +973,7 @@ static int tifm_sd_probe(struct tifm_dev *sock)
>
> mmc->ops = &tifm_sd_ops;
> mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> - mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_MULTIWRITE;
> + mmc->caps = MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA | MMC_CAP_MULTIWRITE;
> mmc->f_min = 20000000 / 60;
> mmc->f_max = 24000000;
>
> diff --git a/drivers/mmc/host/wbsd.c b/drivers/mmc/host/wbsd.c
> index 4d5f374..70d6071 100644
> --- a/drivers/mmc/host/wbsd.c
> +++ b/drivers/mmc/host/wbsd.c
> @@ -1219,7 +1219,7 @@ static int __devinit wbsd_alloc_mmc(struct device *dev)
> mmc->f_min = 375000;
> mmc->f_max = 24000000;
> mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> - mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_MULTIWRITE;
> + mmc->caps = MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA | MMC_CAP_MULTIWRITE;
>
> spin_lock_init(&host->lock);
>
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 7ab962f..142fa69 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -88,12 +88,13 @@ struct mmc_host {
>
> unsigned long caps; /* Host capabilities */
>
> -#define MMC_CAP_4_BIT_DATA (1 << 0) /* Can the host do 4 bit transfers */
> +#define MMC_CAP_SD_4_BIT_DATA (1 << 0) /* Can the host do SD 4 bit transfers */
> #define MMC_CAP_MULTIWRITE (1 << 1) /* Can accurately report bytes sent to card on error */
> #define MMC_CAP_MMC_HIGHSPEED (1 << 2) /* Can do MMC high-speed timing */
> #define MMC_CAP_SD_HIGHSPEED (1 << 3) /* Can do SD high-speed timing */
> #define MMC_CAP_SDIO_IRQ (1 << 4) /* Can signal pending SDIO IRQs */
> #define MMC_CAP_SPI (1 << 5) /* Talks only SPI protocols */
> +#define MMC_CAP_MMC_4_BIT_DATA (1 << 6) /* Can the host do MMC 4 bit transfers */
>

One suggestion:

#define MMC_CAP_4_BIT_DATA ( MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA)

Then no need to change all MMC host drivers, like
drivers/mmc/host/wbsd.c

-Bryan Wu

2008-01-09 07:21:44

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Tue, 8 Jan 2008 16:44:08 -0500
"Mike Frysinger" <[email protected]> wrote:

> On Jan 8, 2008 3:49 PM, Pierre Ossman <[email protected]> wrote:
> > So, again, if you feel that there is a hardware difference between 4-bit MMC and 4-bit SD then please elaborate as it is my understanding that they are identical.
>
> you may be 100% correct, i have no idea, i'm not really familiar with
> MMC/SD/SDIO at all.

The patch adds complexity to the system. So until you can convince me that complexity is actually needed, I'm afraid the answer is NAK.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-09 07:23:38

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Wed, 9 Jan 2008 11:21:40 +0800
"Cai, Cliff" <[email protected]> wrote:

>
> Hi,all
>
> I'd like to say something about this issue.
> Currently,the blackfin on chip SD host ONLY support 1-bit MMC while
> support 1-bit/4-bit SD/SDIO.
> And we want our driver to support both 1-bit MMC and 4-bit SD/SDIO.but
> the current MMC driver framework
> Only allow us to set one kind of bus width,either 1-bit or 4-bit.So in
> order to meet our case,we need more flexible mechanism
> To inform the upper commom driver to know our situation.
>

That's just iterating what's already been said. My claim is that 4-bit is 4-bit, regardless if it's MMC or SD. So if you want this patch to go in you need to explain why there is a difference for the blackfin controller.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-09 16:45:39

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 9, 2008 3:23 PM, Pierre Ossman <[email protected]> wrote:
> On Wed, 9 Jan 2008 11:21:40 +0800
> "Cai, Cliff" <[email protected]> wrote:
>
> >
> > Hi,all
> >
> > I'd like to say something about this issue.
> > Currently,the blackfin on chip SD host ONLY support 1-bit MMC while
> > support 1-bit/4-bit SD/SDIO.
> > And we want our driver to support both 1-bit MMC and 4-bit SD/SDIO.but
> > the current MMC driver framework
> > Only allow us to set one kind of bus width,either 1-bit or 4-bit.So in
> > order to meet our case,we need more flexible mechanism
> > To inform the upper commom driver to know our situation.
> >
>
> That's just iterating what's already been said. My claim is that 4-bit is 4-bit, regardless if it's MMC or SD. So if you want this patch to go in you need to explain why there is a difference for the blackfin controller.
>

Actually, Blackfin BF54x on-chip SDIO host controller is supposed to
support 1-bit and 4-bit MMC or SD. But in the real platform, when MMC
works at 4-bit mode there will be a FIFO underrun error
(http://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_item_id=3535).
While SD works at 4-bit mode is OK. We believe this is a hardware
issue of the SDIO host controller of BF54x.

We intend to workaround in our own BF54x SDIO host controller driver,
but there is no place for us to determine MMC or SD. One choice is to
hack common code like this:
----
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -358,6 +358,9 @@
/*
* Activate wide bus (if supported).
*/
+/*Currently,Blackfin 54x only support 1 bit MMC,while support 4 bit SD */
+/*So if card type is MMC don't enable 4 bit mode*/
+#if !defined(CONFIG_BF54x)
if ((card->csd.mmca_vsn >= CSD_SPEC_VER_4) &&
(host->caps & MMC_CAP_4_BIT_DATA)) {
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
@@ -367,7 +370,7 @@

mmc_set_bus_width(card->host, MMC_BUS_WIDTH_4);
}
-
+#endif
if (!oldcard)
host->card = card;

----

Another choice is to use Mike's patch to split the 4-bit mode into
MMC-4-BIT and SD-4-BIT. So we can support 1-bit and 4-bit SD as well
as 1-bit MMC.

Thanks
-Bryan Wu

2008-01-10 08:54:43

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Thu, 10 Jan 2008 00:45:27 +0800
"Bryan Wu" <[email protected]> wrote:

>
> Actually, Blackfin BF54x on-chip SDIO host controller is supposed to
> support 1-bit and 4-bit MMC or SD. But in the real platform, when MMC
> works at 4-bit mode there will be a FIFO underrun error
> (http://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_item_id=3535).
> While SD works at 4-bit mode is OK. We believe this is a hardware
> issue of the SDIO host controller of BF54x.

Odd. As there shouldn't be a difference, I'm inclined to believe that this is card dependent. So it could as well appear with some SD cards as well.

How much have you tested this issue? That tracker entry isn't very verbose.

Also, how can an underrun appear? The system should have flow control. And an underrun does not sound like a bus problem, rather that you have a card with different speed characteristics than the hardware/driver can handle.

>
> We intend to workaround in our own BF54x SDIO host controller driver,
> but there is no place for us to determine MMC or SD. One choice is to
> hack common code like this:

_If_ it truly cannot handle 4-bit MMC, then Mike's patch is the way to go. But I'm not yet convinced that the problem is related to MMC vs SD.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-10 09:23:30

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 10, 2008 4:54 PM, Pierre Ossman <[email protected]> wrote:
> On Thu, 10 Jan 2008 00:45:27 +0800
> "Bryan Wu" <[email protected]> wrote:
>
> >
> > Actually, Blackfin BF54x on-chip SDIO host controller is supposed to
> > support 1-bit and 4-bit MMC or SD. But in the real platform, when MMC
> > works at 4-bit mode there will be a FIFO underrun error
> > (http://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_item_id=3535).
> > While SD works at 4-bit mode is OK. We believe this is a hardware
> > issue of the SDIO host controller of BF54x.
>
> Odd. As there shouldn't be a difference, I'm inclined to believe that this is card dependent. So it could as well appear with some SD cards as well.
>

At page 4-3 of ADSP-BF54x Blackfin(R) Processor Peripheral Hardware
Reference, there is a table which guide us the SDH controller does not
support 4-bit mode MMC card. Please found the table picture in the
attachment or get the document at:
http://www.analog.com/UploadedFiles/Associated_Docs/61460151169789ADSP_BF54x_Blackfin_Processor_Peripheral_Hardware_Reference.pdf

> How much have you tested this issue? That tracker entry isn't very verbose.
>
> Also, how can an underrun appear? The system should have flow control. And an underrun does not sound like a bus problem, rather that you have a card with different speed characteristics than the hardware/driver can handle.
>

I am sorry for this mistake. This tracker is not for this 4-bit issue,
it is another bug which should be fixed by Cliff.

> >
> > We intend to workaround in our own BF54x SDIO host controller driver,
> > but there is no place for us to determine MMC or SD. One choice is to
> > hack common code like this:
>
> _If_ it truly cannot handle 4-bit MMC, then Mike's patch is the way to go. But I'm not yet convinced that the problem is related to MMC vs SD.
>

Thanks, actually we are not yet convinced by our hardware designer
why BF54x SDH does not support 4-bit MMC.

>
> Rgds
> --
> -- Pierre Ossman
>
> Linux kernel, MMC maintainer http://www.kernel.org
> PulseAudio, core developer http://pulseaudio.org
> rdesktop, core developer http://www.rdesktop.org
>

Attachments:

(No filename) (2.18 kB)
sdh.jpg (116.21 kB)
Download all attachments

2008-01-10 11:58:24

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Thu, 10 Jan 2008 17:22:55 +0800
"Bryan Wu" <[email protected]> wrote:

>
> At page 4-3 of ADSP-BF54x Blackfin(R) Processor Peripheral Hardware
> Reference, there is a table which guide us the SDH controller does not
> support 4-bit mode MMC card. Please found the table picture in the
> attachment or get the document at:
> http://www.analog.com/UploadedFiles/Associated_Docs/61460151169789ADSP_BF54x_Blackfin_Processor_Peripheral_Hardware_Reference.pdf
>

Ok, but this just means the controller wasn't designed with 4-bit MMC in mind. As several other "SD-only" controller have been tested with modern MMC cards without a problem, this is not sufficient to explain any problem.

>
> Thanks, actually we are not yet convinced by our hardware designer
> why BF54x SDH does not support 4-bit MMC.
>

Please keep me informed on how it progresses. I'd like an at least plausible explanation, preferably also some empirical data, before I'm ready to accept Mike's patch.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-11 06:17:18

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 10, 2008 7:57 PM, Pierre Ossman <[email protected]> wrote:
> On Thu, 10 Jan 2008 17:22:55 +0800
> "Bryan Wu" <[email protected]> wrote:
>
> >
> > At page 4-3 of ADSP-BF54x Blackfin(R) Processor Peripheral Hardware
> > Reference, there is a table which guide us the SDH controller does not
> > support 4-bit mode MMC card. Please found the table picture in the
> > attachment or get the document at:
> > http://www.analog.com/UploadedFiles/Associated_Docs/61460151169789ADSP_BF54x_Blackfin_Processor_Peripheral_Hardware_Reference.pdf
> >
>
> Ok, but this just means the controller wasn't designed with 4-bit MMC in mind. As several other "SD-only" controller have been tested with modern MMC cards without a problem, this is not sufficient to explain any problem.
>

This should be some HW design issue and BF54x including SDH controller
is in mass production.
There is no chance to change the silicon, we just wanna use software
driver to workaround this issue.
So Mike's patch is here.

> >
> > Thanks, actually we are not yet convinced by our hardware designer
> > why BF54x SDH does not support 4-bit MMC.
> >
>
> Please keep me informed on how it progresses. I'd like an at least plausible explanation, preferably also some empirical data, before I'm ready to accept Mike's patch.
>

We were told this is an hardware design issue, so please help us to
workaround it in software side with Mike's patch.
And how do you think my suggestion to Mike's patch,
#define MMC_CAP_4_BIT_DATA ( MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA)
Then no need to change other MMC host drivers

Thanks
Regards,
-Bryan Wu

2008-01-11 06:25:34

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 11, 2008 1:17 AM, Bryan Wu <[email protected]> wrote:
> On Jan 10, 2008 7:57 PM, Pierre Ossman <[email protected]> wrote:
> > On Thu, 10 Jan 2008 17:22:55 +0800
> > "Bryan Wu" <[email protected]> wrote:
> >
> > >
> > > At page 4-3 of ADSP-BF54x Blackfin(R) Processor Peripheral Hardware
> > > Reference, there is a table which guide us the SDH controller does not
> > > support 4-bit mode MMC card. Please found the table picture in the
> > > attachment or get the document at:
> > > http://www.analog.com/UploadedFiles/Associated_Docs/61460151169789ADSP_BF54x_Blackfin_Processor_Peripheral_Hardware_Reference.pdf
> > >
> >
> > Ok, but this just means the controller wasn't designed with 4-bit MMC in mind. As several other "SD-only" controller have been tested with modern MMC cards without a problem, this is not sufficient to explain any problem.
> >
>
> This should be some HW design issue and BF54x including SDH controller
> is in mass production.
> There is no chance to change the silicon, we just wanna use software
> driver to workaround this issue.
> So Mike's patch is here.
>
> > >
> > > Thanks, actually we are not yet convinced by our hardware designer
> > > why BF54x SDH does not support 4-bit MMC.
> > >
> >
> > Please keep me informed on how it progresses. I'd like an at least plausible explanation, preferably also some empirical data, before I'm ready to accept Mike's patch.
> >
>
> We were told this is an hardware design issue, so please help us to
> workaround it in software side with Mike's patch.
> And how do you think my suggestion to Mike's patch,
> #define MMC_CAP_4_BIT_DATA ( MMC_CAP_SD_4_BIT_DATA | MMC_CAP_MMC_4_BIT_DATA)
> Then no need to change other MMC host drivers

i can send an updated patch as needed. i think Bryan's suggestion
here is a good one.
-mike

2008-01-11 08:40:55

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Fri, 11 Jan 2008 14:17:01 +0800
"Bryan Wu" <[email protected]> wrote:

> We were told this is an hardware design issue, so please help us to
> workaround it in software side with Mike's patch.

I'm afraid that's insufficient motivation for this change. All documentation and real world tests say that MMC and SD 4-bit data is 100% compatible. So it's far more probable that you've misdiagnosed your error than this being the actual problem. The fact that 4-bit MMC support has been present for some time and you're only now seeing a problem further supports that.

In light of that, the suggested patch would not only be incorrect, it would also be worse than doing nothing. You would be papering over a symptom, making the real problem harder to find.

If you want something ready for upstream, for now I suggest removing MMC_CAP_4_BIT_DATA completely from your driver's caps field.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-11 09:09:21

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 11, 2008 3:40 AM, Pierre Ossman <[email protected]> wrote:
> "Bryan Wu" <[email protected]> wrote:
> > We were told this is an hardware design issue, so please help us to
> > workaround it in software side with Mike's patch.
>
> So it's far more probable that you've misdiagnosed your error than this being the actual problem.

the guys who design the silicon are telling us it doesnt work. our
tests show it not working. i'm not sure what else you want here.

> The fact that 4-bit MMC support has been present for some time and you're only now seeing a problem further supports that.

not in the least actually. we just started developing the driver. it
has seen no real world use. we've been testing things internally to
make sure the setup is sane. 4-bit MMC is coming up as not working.
-mike

2008-01-11 09:35:49

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Fri, 11 Jan 2008 04:08:53 -0500
"Mike Frysinger" <[email protected]> wrote:

> On Jan 11, 2008 3:40 AM, Pierre Ossman <[email protected]> wrote:
> > So it's far more probable that you've misdiagnosed your error than this being the actual problem.
>
> the guys who design the silicon are telling us it doesnt work. our
> tests show it not working. i'm not sure what else you want here.
>

More information. You have not provided any speculation as to why it doesn't work, or what you've done to figure it out. You haven't even described how it fails, just that it does. No information about what cards you've tested and how you've tested them to confirm that your theory on 4-bit data is accurate.

You have no track record with me, and you're proposing something that goes against all known information. So you have to understand that I'm sceptical about your claims. I won't dismiss them off hand, but I need to be convinced you haven't made a mistake when coming to this conclusion.

> > The fact that 4-bit MMC support has been present for some time and you're only now seeing a problem further supports that.
>
> not in the least actually. we just started developing the driver. it
> has seen no real world use. we've been testing things internally to
> make sure the setup is sane. 4-bit MMC is coming up as not working.

Fair enough. But if it's a new driver that hasn't seen real-world testing, then there might be other bugs influencing things.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-11 09:47:52

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 11, 2008 4:35 AM, Pierre Ossman <[email protected]> wrote:
> "Mike Frysinger" <[email protected]> wrote:
> > On Jan 11, 2008 3:40 AM, Pierre Ossman <[email protected]> wrote:
> > > So it's far more probable that you've misdiagnosed your error than this being the actual problem.
> >
> > the guys who design the silicon are telling us it doesnt work. our
> > tests show it not working. i'm not sure what else you want here.
>
> More information. You have not provided any speculation as to why it doesn't work, or what you've done to figure it out. You haven't even described how it fails, just that it does. No information about what cards you've tested and how you've tested them to confirm that your theory on 4-bit data is accurate.

unfortunately, i cant fill these gaps myself directly. all i bring to
the table is proper integration with the common frameworks. i'm
relying on the guys developing the drivers and the guys who actually
designed the hardware for information. they tell me it doesnt work, i
just believe them ;).

Cliff should be able to enumerate the cards he has tested and the
issues he's run into. i'll see if i cant get more in depth
information from the hardware guys beyond the documentation on the sdh
already available in the hardware manual (which i'm guessing has not
been enough to appease you).

> You have no track record with me, and you're proposing something that goes against all known information. So you have to understand that I'm sceptical about your claims. I won't dismiss them off hand, but I need to be convinced you haven't made a mistake when coming to this conclusion.

that's fair. i could actually be a gnome sitting outside in your
lawn. you have no idea.
-mike

2008-01-11 10:05:25

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Fri, 11 Jan 2008 04:47:44 -0500
"Mike Frysinger" <[email protected]> wrote:

> Cliff should be able to enumerate the cards he has tested and the
> issues he's run into. i'll see if i cant get more in depth
> information from the hardware guys beyond the documentation on the sdh
> already available in the hardware manual (which i'm guessing has not
> been enough to appease you).
>

Great. And you're right in that the spec did not give any indication that MMC shouldn't work.

I did notice one thing though. The pull-ups and pull-downs seem to be controllable. If I remember correctly, DAT3 needs to have a controller-side pull-up for MMC cards. And the docs said the default was not to provide that (in order to use the internal pull-up that most, but not all, SD cards have for detection). Might be something to look at if you haven't already.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-11 10:22:25

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 11, 2008 6:05 PM, Pierre Ossman <[email protected]> wrote:
> On Fri, 11 Jan 2008 04:47:44 -0500
> "Mike Frysinger" <[email protected]> wrote:
>
> > Cliff should be able to enumerate the cards he has tested and the
> > issues he's run into. i'll see if i cant get more in depth
> > information from the hardware guys beyond the documentation on the sdh
> > already available in the hardware manual (which i'm guessing has not
> > been enough to appease you).
> >
>
> Great. And you're right in that the spec did not give any indication that MMC shouldn't work.
>

Our tester will reopen a tracker of this issue. When it's fired, you
will be updated with this information.

> I did notice one thing though. The pull-ups and pull-downs seem to be controllable. If I remember correctly, DAT3 needs to have a controller-side pull-up for MMC cards. And the docs said the default was not to provide that (in order to use the internal pull-up that most, but not all, SD cards have for detection). Might be something to look at if you haven't already.
>

I guess it is for MMC/SD card insert detection. Is it related with the
4-bit MMC and 4-bit SD?
Actually, there are some issues about the card insert detection on
BF54x SDH. Following is some comments of our bfin-sdh driver which is
in the development.

---
/* In term of ADSP_BF54x_Blackfin_Processor_Peripheral_Hardware_Reference,
* the SDH allows software to detect a card when it is inserted into its slot.
* The SD_DATA3 pin powers up low due to a special pull-down resistor. When an
* SD Card is inserted in its slot, the resistance increases and a rising edge
* is detected by the SDH module.
* But this doesn't work sometimes. When a MMC/SD card is inserted, the voltage
* doesn't rise on SD_DATA3. In term of The MultiMediaCard System Specification,
* SD_DATA3 is used as CS pin in SPI mode. The MultiMediaCard wakes up in the
* MultiMediaCard mode. During the scan procedure, host will send CMD0 to reset
* MMC card, if CS pin is low, MMC card will enter SPI mode. Of course Secure
* Digital Host controller is not a SPI controller. So the Card detect function
* has to be disabled. After card is inserted run "echo 0 > /proc/driver/sdh"
* to trigger card scanning */
---

Thanks
-Bryan Wu

2008-01-11 11:19:18

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Fri, 11 Jan 2008 18:22:14 +0800
"Bryan Wu" <[email protected]> wrote:

>
> I guess it is for MMC/SD card insert detection. Is it related with the
> 4-bit MMC and 4-bit SD?
> Actually, there are some issues about the card insert detection on
> BF54x SDH. Following is some comments of our bfin-sdh driver which is
> in the development.
>

Right. As you've noticed, that pin is not a reliable way of detecting cards. It works badly, if all, for MMC cards. And some SD cards also lack the necessary pull-up.

But for the matter at hand, some cards might rely on the pull-up being there, so you'll get bit-errors without it. I don't have time to double check the specs right now, but it might explain why you're seeing problems with MMC and not SD.

Btw, the MMC core sends you ios information on how you should fiddle with that bit. It was needed on a Winbond controller (the wbsd driver) as some models relied on it for card detection.

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org

2008-01-11 17:51:52

by Robin Getz

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Fri 11 Jan 2008 04:35, Pierre Ossman pondered:
> On Fri, 11 Jan 2008 04:08:53 -0500
> "Mike Frysinger" <[email protected]> wrote:
>
> > On Jan 11, 2008 3:40 AM, Pierre Ossman <[email protected]> wrote:
> > > So it's far more probable that you've misdiagnosed your error than
> this being the actual problem.
> >
> > the guys who design the silicon are telling us it doesnt work. our
> > tests show it not working. i'm not sure what else you want here.
> >
>
> More information. You have not provided any speculation as to why it
> doesn't work, or what you've done to figure it out.

I have been trying to get the detailed information from the hardware (silicon
designer) guy about why he believes support for 4-bit MMC is not there, but I
have not heard back yet.

As soon as I hear from them - and either understand why 4-bit MMC doesn't
work, or if the Blackfin docs are incorrect (either is OK outcome) - I will
let you know.

Thanks
-Robin

2008-01-12 13:01:40

by Robin Getz

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Fri 11 Jan 2008 12:52, Robin Getz pondered:
> On Fri 11 Jan 2008 04:35, Pierre Ossman pondered:
> > On Fri, 11 Jan 2008 04:08:53 -0500
> > "Mike Frysinger" <[email protected]> wrote:
> >
> > > On Jan 11, 2008 3:40 AM, Pierre Ossman <[email protected]> wrote:
> > > > So it's far more probable that you've misdiagnosed your error than
> > this being the actual problem.
> > >
> > > the guys who design the silicon are telling us it doesnt work. our
> > > tests show it not working. i'm not sure what else you want here.
> > >
> >
> > More information. You have not provided any speculation as to why it
> > doesn't work, or what you've done to figure it out.
>
> I have been trying to get the detailed information from the hardware
> (silicon designer) guy about why he believes support for 4-bit MMC is
> not there, but I have not heard back yet.
>
> As soon as I hear from them - and either understand why 4-bit MMC doesn't
> work, or if the Blackfin docs are incorrect (either is OK outcome) - I will
> let you know.

According to the HW folks - it is exactly as Pierre indicated - in theory it
should work, 4-bit MMC requires usage of different set of commands as
compared to 4-bit SD, so it should be just software - although no one has
tested it yet.

So I guess this is back on Bryan/Cliff to make work....

Thanks for pushing back.

-Robin

2008-01-12 15:25:11

[permalink] [raw]

Subject: Re: [patch] split MMC_CAP_4_BIT_DATA

On Jan 12, 2008 9:02 PM, Robin Getz <[email protected]> wrote:
> On Fri 11 Jan 2008 12:52, Robin Getz pondered:
>
> > On Fri 11 Jan 2008 04:35, Pierre Ossman pondered:
> > > On Fri, 11 Jan 2008 04:08:53 -0500
> > > "Mike Frysinger" <[email protected]> wrote:
> > >
> > > > On Jan 11, 2008 3:40 AM, Pierre Ossman <[email protected]> wrote:
> > > > > So it's far more probable that you've misdiagnosed your error than
> > > this being the actual problem.
> > > >
> > > > the guys who design the silicon are telling us it doesnt work. our
> > > > tests show it not working. i'm not sure what else you want here.
> > > >
> > >
> > > More information. You have not provided any speculation as to why it
> > > doesn't work, or what you've done to figure it out.
> >
> > I have been trying to get the detailed information from the hardware
> > (silicon designer) guy about why he believes support for 4-bit MMC is
> > not there, but I have not heard back yet.
> >
> > As soon as I hear from them - and either understand why 4-bit MMC doesn't
> > work, or if the Blackfin docs are incorrect (either is OK outcome) - I will
> > let you know.
>
> According to the HW folks - it is exactly as Pierre indicated - in theory it
> should work, 4-bit MMC requires usage of different set of commands as
> compared to 4-bit SD, so it should be just software - although no one has
> tested it yet.
>

Vivi is helping us to do the test again, and she will fire a bug
tracker about this issue next week.
So we can debug it again.

> So I guess this is back on Bryan/Cliff to make work....
>
> Thanks for pushing back.
>

Yes, Thanks a lot Pierre.
-Bryan Wu

2008-01-12 15:39:21