Re: [PATCH] libertas/sdio: 8686: set ECSI bit for 1-bit transfers

Previous thread: [PATCH] increase PREEMPT_BITS to 12 to avoid overflow when starting KVM by Rik van Riel on Tuesday, March 30, 2010 - 10:36 am. (43 messages)

Next thread: Re: kernel decompressor interface by H. Peter Anvin on Tuesday, March 30, 2010 - 10:45 am. (1 message)
From: Daniel Mack
Date: Tuesday, March 30, 2010 - 10:38 am

When operating in 1-bit mode, SDAT1 is used as dedicated interrupt line.
However, the 8686 will only drive this line when the ECSI and SCSI bits
are set in the CCCR_IF register.

Thanks to Alagu Sankar for pointing me in the right direction.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Alagu Sankar <alagusankar@embwise.com>
Cc: Volker Ernst <volker.ernst@txtr.com>
Cc: Dan Williams <dcbw@redhat.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Holger Schurig <hs4233@mail.mn-solutions.de>
Cc: Bing Zhao <bzhao@marvell.com>
Cc: libertas-dev@lists.infradead.org
Cc: linux-wireless@vger.kernel.org
Cc: linux-mmc@vger.kernel.org
---
 drivers/net/wireless/libertas/if_sdio.c |   24 ++++++++++++++++++++++++
 include/linux/mmc/sdio.h                |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 7a73f62..d3170f2 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -34,6 +34,8 @@
 #include <linux/mmc/card.h>
 #include <linux/mmc/sdio_func.h>
 #include <linux/mmc/sdio_ids.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/host.h>
 
 #include "host.h"
 #include "decl.h"
@@ -942,6 +944,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	int ret, i;
 	unsigned int model;
 	struct if_sdio_packet *packet;
+	struct mmc_host *host = func->card->host;
 
 	lbs_deb_enter(LBS_DEB_SDIO);
 
@@ -1022,6 +1025,27 @@ static int if_sdio_probe(struct sdio_func *func,
 	if (ret)
 		goto disable;
 
+	/* For 1-bit transfers, we need to enable the interrupt flags in
+	 * the CCCR register. Temporarily set the function number to 0
+	 * for that. */
+	if ((host->caps & MMC_CAP_SDIO_IRQ) &&
+	    (host->ios.bus_width == MMC_BUS_WIDTH_1)) {
+		unsigned int num = func->num;
+		u8 reg;
+
+		func->num = 0;
+		reg = sdio_readb(func, SDIO_CCCR_IF, &ret);
+		if (ret)
+			goto release_int;
+
+		reg |= SDIO_BUS_ECSI | ...
From: Dan Williams
Date: Tuesday, March 30, 2010 - 11:37 am

While this looks fine enough to me, I don't have enough knowledge of the
SDIO stack to know whether or not this would be considered a layering
violation or not.  Given that I don't think there's an active maintainer
for the SDIO subsystem though, I'd say it's fine enough for me...

Any thoughts David since it appears to be at least somewhat SPI related?



--

From: Daniel Mack
Date: Tuesday, March 30, 2010 - 11:40 am

I believe that these bits in question are just abused in this case. The
device is not in SPI mode, so according to the specs, the bits shouldn't
make any difference. This makes me think that this is specific to

Thanks!

Daniel

--

From: Alagu Sankar Vellaichamy
Date: Wednesday, March 31, 2010 - 1:23 am

SCSI is read-only.  We ideally should be checking this bit and then
set the ECSI accordingly, rather than setting both ECSI and SCSI.

- Alagu Sankar

--

From: Daniel Mack
Date: Wednesday, March 31, 2010 - 6:34 am

Thanks for noticing. However, the libertas chip does not set SCSI, so we
have to do it unconditionally. New patch has just been sent.

--

From: =?ISO-8859-2?Q?Micha=B3_Miros=B3aw?=
Date: Wednesday, March 31, 2010 - 2:07 am

2010/3/30 Daniel Mack <daniel@caiaq.de>:

You should probably just use mmc_io_rw_direct() in this case instead
of abusing func->num.

Best Regards,
Michał Mirosław
--

From: Daniel Mack
Date: Wednesday, March 31, 2010 - 2:08 am

Hmm, that function isn't exported, and I didn't want to change this. You
say you'd prefer that? I can cook up something that does it, no problem.


Daniel
--

From: =?ISO-8859-2?Q?Micha=B3_Miros=B3aw?=
Date: Wednesday, March 31, 2010 - 2:49 am

BTW, I can't see any exported functions to access fn#0 directly from
drivers. Maybe it's time to introduce them now - at least CCCR has
some vendor-defined parts that drivers may want to access and there is
a lot of place in CIS area that can be (ab)used by devices.

Best Regards,
Michał Mirosław
--

From: Daniel Mack
Date: Wednesday, March 31, 2010 - 6:08 am

In fact, there is sdio_f0_{read,write}b() - I overlooked them. Will
resend a new patch.

Thanks for checking,
Daniel
--

From: Daniel Mack
Date: Wednesday, March 31, 2010 - 6:31 am

When operating in 1-bit mode, SDAT1 is used as dedicated interrupt line.
However, the 8686 will only drive this line when the ECSI bit is set in
the CCCR_IF register.

Thanks to Alagu Sankar for pointing me in the right direction.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Alagu Sankar <alagusankar@embwise.com>
Cc: Volker Ernst <volker.ernst@txtr.com>
Cc: Dan Williams <dcbw@redhat.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Holger Schurig <hs4233@mail.mn-solutions.de>
Cc: Bing Zhao <bzhao@marvell.com>
Cc: libertas-dev@lists.infradead.org
Cc: linux-wireless@vger.kernel.org
Cc: linux-mmc@vger.kernel.org
---
 drivers/net/wireless/libertas/if_sdio.c |   21 +++++++++++++++++++++
 include/linux/mmc/sdio.h                |    2 ++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 7a73f62..f89bb8b 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -34,6 +34,8 @@
 #include <linux/mmc/card.h>
 #include <linux/mmc/sdio_func.h>
 #include <linux/mmc/sdio_ids.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/host.h>
 
 #include "host.h"
 #include "decl.h"
@@ -942,6 +944,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	int ret, i;
 	unsigned int model;
 	struct if_sdio_packet *packet;
+	struct mmc_host *host = func->card->host;
 
 	lbs_deb_enter(LBS_DEB_SDIO);
 
@@ -1022,6 +1025,24 @@ static int if_sdio_probe(struct sdio_func *func,
 	if (ret)
 		goto disable;
 
+	/* For 1-bit transfers, we need to enable the interrupt flag in
+	 * the CCCR register. Set the MMC_QUIRK_LENIENT_FN0 bit to allow
+	 * access to non-vendor registers. */
+	if ((host->caps & MMC_CAP_SDIO_IRQ) &&
+	    (host->ios.bus_width == MMC_BUS_WIDTH_1)) {
+		u8 reg;
+
+		func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
+		reg = sdio_f0_readb(func, SDIO_CCCR_IF, &ret);
+		if (ret)
+			goto release_int;
+
+		reg |= ...
From: Dan Williams
Date: Monday, April 5, 2010 - 7:42 pm

Could you modify the patch to do this *only* for the 8686?  Apparently
only the 8686 this hardware "quirk" and thus this might have odd
side-effects on the 8688 or the 8385.

Check the 'struct if_sdio_card' model member and compare it to
IF_SDIO_MODEL_8686 like if_sdio_probe() does.

Thanks!


--

From: Daniel Mack
Date: Tuesday, April 6, 2010 - 1:52 am

When operating in 1-bit mode, SDAT1 is used as dedicated interrupt line.
However, the 8686 will only drive this line when the ECSI bit is set in
the CCCR_IF register.

Thanks to Alagu Sankar for pointing me in the right direction.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Alagu Sankar <alagusankar@embwise.com>
Cc: Volker Ernst <volker.ernst@txtr.com>
Cc: Dan Williams <dcbw@redhat.com>
Cc: John W. Linville <linville@tuxdriver.com>
Cc: Holger Schurig <hs4233@mail.mn-solutions.de>
Cc: Bing Zhao <bzhao@marvell.com>
Cc: libertas-dev@lists.infradead.org
Cc: linux-wireless@vger.kernel.org
Cc: linux-mmc@vger.kernel.org
---
 drivers/net/wireless/libertas/if_sdio.c |   22 ++++++++++++++++++++++
 include/linux/mmc/sdio.h                |    2 ++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 7a73f62..33206a9 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -34,6 +34,8 @@
 #include <linux/mmc/card.h>
 #include <linux/mmc/sdio_func.h>
 #include <linux/mmc/sdio_ids.h>
+#include <linux/mmc/sdio.h>
+#include <linux/mmc/host.h>
 
 #include "host.h"
 #include "decl.h"
@@ -942,6 +944,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	int ret, i;
 	unsigned int model;
 	struct if_sdio_packet *packet;
+	struct mmc_host *host = func->card->host;
 
 	lbs_deb_enter(LBS_DEB_SDIO);
 
@@ -1022,6 +1025,25 @@ static int if_sdio_probe(struct sdio_func *func,
 	if (ret)
 		goto disable;
 
+	/* For 1-bit transfers to the 8686 model, we need to enable the
+	 * interrupt flag in the CCCR register. Set the MMC_QUIRK_LENIENT_FN0
+	 * bit to allow access to non-vendor registers. */
+	if ((card->model == IF_SDIO_MODEL_8686) &&
+	    (host->caps & MMC_CAP_SDIO_IRQ) &&
+	    (host->ios.bus_width == MMC_BUS_WIDTH_1)) {
+		u8 reg;
+
+		func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
+		reg = sdio_f0_readb(func, SDIO_CCCR_IF, ...
From: Dan Williams
Subject:
Date: Tuesday, April 6, 2010 - 9:07 am

From: Daniel Mack
Date: Tuesday, April 13, 2010 - 3:29 am

Was this picked by anyone?
Just asking because I didn't see it in the wireless-2.6.git yet.

Thanks,
--

From: John W. Linville
Date: Tuesday, April 13, 2010 - 6:06 am

It is in wireless-next-2.6, queued for 2.6.35.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.
--

From: Daniel Mack
Date: Tuesday, April 13, 2010 - 6:21 am

Thanks a lot!

Daniel
--

Previous thread: [PATCH] increase PREEMPT_BITS to 12 to avoid overflow when starting KVM by Rik van Riel on Tuesday, March 30, 2010 - 10:36 am. (43 messages)

Next thread: Re: kernel decompressor interface by H. Peter Anvin on Tuesday, March 30, 2010 - 10:45 am. (1 message)