2010-03-30 17:39:43

by Daniel Mack

[permalink] [raw]
Subject: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

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 <[email protected]>
Cc: Alagu Sankar <[email protected]>
Cc: Volker Ernst <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: John W. Linville <[email protected]>
Cc: Holger Schurig <[email protected]>
Cc: Bing Zhao <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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 | SDIO_BUS_SCSI;
+ sdio_writeb(func, reg, SDIO_CCCR_IF, &ret);
+ if (ret)
+ goto release_int;
+
+ func->num = num;
+ }
+
card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
if (ret)
goto release_int;
diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
index 0ebaef5..329a8fa 100644
--- a/include/linux/mmc/sdio.h
+++ b/include/linux/mmc/sdio.h
@@ -94,6 +94,8 @@

#define SDIO_BUS_WIDTH_1BIT 0x00
#define SDIO_BUS_WIDTH_4BIT 0x02
+#define SDIO_BUS_ECSI 0x20 /* Enable continuous SPI interrupt */
+#define SDIO_BUS_SCSI 0x40 /* Support continuous SPI interrupt */

#define SDIO_BUS_ASYNC_INT 0x20

--
1.7.0



2010-03-31 13:32:22

by Daniel Mack

[permalink] [raw]
Subject: [PATCH] libertas/sdio: set ECSI bit for 1-bit transfers

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 <[email protected]>
Cc: Alagu Sankar <[email protected]>
Cc: Volker Ernst <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: John W. Linville <[email protected]>
Cc: Holger Schurig <[email protected]>
Cc: Bing Zhao <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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 |= SDIO_BUS_ECSI;
+ sdio_f0_writeb(func, reg, SDIO_CCCR_IF, &ret);
+ if (ret)
+ goto release_int;
+ }
+
card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
if (ret)
goto release_int;
diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
index 0ebaef5..329a8fa 100644
--- a/include/linux/mmc/sdio.h
+++ b/include/linux/mmc/sdio.h
@@ -94,6 +94,8 @@

#define SDIO_BUS_WIDTH_1BIT 0x00
#define SDIO_BUS_WIDTH_4BIT 0x02
+#define SDIO_BUS_ECSI 0x20 /* Enable continuous SPI interrupt */
+#define SDIO_BUS_SCSI 0x40 /* Support continuous SPI interrupt */

#define SDIO_BUS_ASYNC_INT 0x20

--
1.7.0


2010-03-30 18:38:08

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

On Tue, 2010-03-30 at 19:38 +0200, Daniel Mack wrote:
> 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.

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?

Acked-by: Dan Williams <[email protected]>

> Thanks to Alagu Sankar for pointing me in the right direction.
>
> Signed-off-by: Daniel Mack <[email protected]>
> Cc: Alagu Sankar <[email protected]>
> Cc: Volker Ernst <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: John W. Linville <[email protected]>
> Cc: Holger Schurig <[email protected]>
> Cc: Bing Zhao <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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 | SDIO_BUS_SCSI;
> + sdio_writeb(func, reg, SDIO_CCCR_IF, &ret);
> + if (ret)
> + goto release_int;
> +
> + func->num = num;
> + }
> +
> card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
> if (ret)
> goto release_int;
> diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> index 0ebaef5..329a8fa 100644
> --- a/include/linux/mmc/sdio.h
> +++ b/include/linux/mmc/sdio.h
> @@ -94,6 +94,8 @@
>
> #define SDIO_BUS_WIDTH_1BIT 0x00
> #define SDIO_BUS_WIDTH_4BIT 0x02
> +#define SDIO_BUS_ECSI 0x20 /* Enable continuous SPI interrupt */
> +#define SDIO_BUS_SCSI 0x40 /* Support continuous SPI interrupt */
>
> #define SDIO_BUS_ASYNC_INT 0x20
>



2010-03-31 08:23:22

by Alagu Sankar Vellaichamy

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

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

On Tue, Mar 30, 2010 at 11:08 PM, Daniel Mack <[email protected]> wrote:
> 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 <[email protected]>
> Cc: Alagu Sankar <[email protected]>
> Cc: Volker Ernst <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: John W. Linville <[email protected]>
> Cc: Holger Schurig <[email protected]>
> Cc: Bing Zhao <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> ?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 | SDIO_BUS_SCSI;
> + ? ? ? ? ? ? ? sdio_writeb(func, reg, SDIO_CCCR_IF, &ret);
> + ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? goto release_int;
> +
> + ? ? ? ? ? ? ? func->num = num;
> + ? ? ? }
> +
> ? ? ? ?card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
> ? ? ? ?if (ret)
> ? ? ? ? ? ? ? ?goto release_int;
> diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> index 0ebaef5..329a8fa 100644
> --- a/include/linux/mmc/sdio.h
> +++ b/include/linux/mmc/sdio.h
> @@ -94,6 +94,8 @@
>
> ?#define ?SDIO_BUS_WIDTH_1BIT ? 0x00
> ?#define ?SDIO_BUS_WIDTH_4BIT ? 0x02
> +#define ?SDIO_BUS_ECSI ? ? ? ? 0x20 ? ?/* Enable continuous SPI interrupt */
> +#define ?SDIO_BUS_SCSI ? ? ? ? 0x40 ? ?/* Support continuous SPI interrupt */
>
> ?#define ?SDIO_BUS_ASYNC_INT ? ?0x20
>
> --
> 1.7.0
>
>

2010-03-31 09:08:42

by Daniel Mack

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

On Wed, Mar 31, 2010 at 11:07:06AM +0200, Michał Mirosław wrote:
> 2010/3/30 Daniel Mack <[email protected]>:
> [...]
> > @@ -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 | SDIO_BUS_SCSI;
> > +               sdio_writeb(func, reg, SDIO_CCCR_IF, &ret);
> > +               if (ret)
> > +                       goto release_int;
> > +
> > +               func->num = num;
> > +       }
> > +
> >        card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
> >        if (ret)
> >                goto release_int;
>
> You should probably just use mmc_io_rw_direct() in this case instead
> of abusing func->num.

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

2010-03-31 13:08:59

by Daniel Mack

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

On Wed, Mar 31, 2010 at 11:49:49AM +0200, Michał Mirosław wrote:
> W dniu 31 marca 2010 11:08 użytkownik Daniel Mack <[email protected]> napisał:
> > 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.
>
> 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.

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

Thanks for checking,
Daniel

2010-03-31 13:34:19

by Daniel Mack

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

On Wed, Mar 31, 2010 at 01:53:21PM +0530, Alagu Sankar Vellaichamy wrote:
> SCSI is read-only. We ideally should be checking this bit and then
> set the ECSI accordingly, rather than setting both ECSI and SCSI.

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

Daniel




> On Tue, Mar 30, 2010 at 11:08 PM, Daniel Mack <[email protected]> wrote:
> > 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 <[email protected]>
> > Cc: Alagu Sankar <[email protected]>
> > Cc: Volker Ernst <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Cc: John W. Linville <[email protected]>
> > Cc: Holger Schurig <[email protected]>
> > Cc: Bing Zhao <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > ?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 | SDIO_BUS_SCSI;
> > + ? ? ? ? ? ? ? sdio_writeb(func, reg, SDIO_CCCR_IF, &ret);
> > + ? ? ? ? ? ? ? if (ret)
> > + ? ? ? ? ? ? ? ? ? ? ? goto release_int;
> > +
> > + ? ? ? ? ? ? ? func->num = num;
> > + ? ? ? }
> > +
> > ? ? ? ?card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
> > ? ? ? ?if (ret)
> > ? ? ? ? ? ? ? ?goto release_int;
> > diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> > index 0ebaef5..329a8fa 100644
> > --- a/include/linux/mmc/sdio.h
> > +++ b/include/linux/mmc/sdio.h
> > @@ -94,6 +94,8 @@
> >
> > ?#define ?SDIO_BUS_WIDTH_1BIT ? 0x00
> > ?#define ?SDIO_BUS_WIDTH_4BIT ? 0x02
> > +#define ?SDIO_BUS_ECSI ? ? ? ? 0x20 ? ?/* Enable continuous SPI interrupt */
> > +#define ?SDIO_BUS_SCSI ? ? ? ? 0x40 ? ?/* Support continuous SPI interrupt */
> >
> > ?#define ?SDIO_BUS_ASYNC_INT ? ?0x20
> >
> > --
> > 1.7.0
> >
> >

2010-03-30 18:40:50

by Daniel Mack

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

On Tue, Mar 30, 2010 at 11:37:39AM -0700, Dan Williams wrote:
> On Tue, 2010-03-30 at 19:38 +0200, Daniel Mack wrote:
> > 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.
>
> 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?

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
libertas, and doesn't belong in any lower level.

> Acked-by: Dan Williams <[email protected]>

Thanks!

Daniel


2010-03-31 09:49:53

by Michał Mirosław

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

W dniu 31 marca 2010 11:08 u?ytkownik Daniel Mack <[email protected]> napisa?:
> On Wed, Mar 31, 2010 at 11:07:06AM +0200, Micha? Miros?aw wrote:
>> 2010/3/30 Daniel Mack <[email protected]>:
>> [...]
>> > @@ -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 | SDIO_BUS_SCSI;
>> > + ? ? ? ? ? ? ? sdio_writeb(func, reg, SDIO_CCCR_IF, &ret);
>> > + ? ? ? ? ? ? ? if (ret)
>> > + ? ? ? ? ? ? ? ? ? ? ? goto release_int;
>> > +
>> > + ? ? ? ? ? ? ? func->num = num;
>> > + ? ? ? }
>> > +
>> > ? ? ? ?card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
>> > ? ? ? ?if (ret)
>> > ? ? ? ? ? ? ? ?goto release_int;
>>
>> You should probably just use mmc_io_rw_direct() in this case instead
>> of abusing func->num.
>
> 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.

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

2010-03-31 09:07:08

by Michał Mirosław

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI and SCSI bits for 1-bit transfers

2010/3/30 Daniel Mack <[email protected]>:
[...]
> @@ -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 | SDIO_BUS_SCSI;
> + ? ? ? ? ? ? ? sdio_writeb(func, reg, SDIO_CCCR_IF, &ret);
> + ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? goto release_int;
> +
> + ? ? ? ? ? ? ? func->num = num;
> + ? ? ? }
> +
> ? ? ? ?card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
> ? ? ? ?if (ret)
> ? ? ? ? ? ? ? ?goto release_int;

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

Best Regards,
Micha? Miros?aw

2010-04-06 02:43:27

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] libertas/sdio: set ECSI bit for 1-bit transfers

On Wed, 2010-03-31 at 15:31 +0200, Daniel Mack wrote:
> 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.

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!
Dan

> Signed-off-by: Daniel Mack <[email protected]>
> Cc: Alagu Sankar <[email protected]>
> Cc: Volker Ernst <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: John W. Linville <[email protected]>
> Cc: Holger Schurig <[email protected]>
> Cc: Bing Zhao <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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 |= SDIO_BUS_ECSI;
> + sdio_f0_writeb(func, reg, SDIO_CCCR_IF, &ret);
> + if (ret)
> + goto release_int;
> + }
> +
> card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
> if (ret)
> goto release_int;
> diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> index 0ebaef5..329a8fa 100644
> --- a/include/linux/mmc/sdio.h
> +++ b/include/linux/mmc/sdio.h
> @@ -94,6 +94,8 @@
>
> #define SDIO_BUS_WIDTH_1BIT 0x00
> #define SDIO_BUS_WIDTH_4BIT 0x02
> +#define SDIO_BUS_ECSI 0x20 /* Enable continuous SPI interrupt */
> +#define SDIO_BUS_SCSI 0x40 /* Support continuous SPI interrupt */
>
> #define SDIO_BUS_ASYNC_INT 0x20
>



2010-04-06 08:53:00

by Daniel Mack

[permalink] [raw]
Subject: [PATCH] libertas/sdio: 8686: set ECSI bit for 1-bit transfers

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 <[email protected]>
Cc: Alagu Sankar <[email protected]>
Cc: Volker Ernst <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: John W. Linville <[email protected]>
Cc: Holger Schurig <[email protected]>
Cc: Bing Zhao <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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, &ret);
+ if (ret)
+ goto release_int;
+
+ reg |= SDIO_BUS_ECSI;
+ sdio_f0_writeb(func, reg, SDIO_CCCR_IF, &ret);
+ if (ret)
+ goto release_int;
+ }
+
card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
if (ret)
goto release_int;
diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
index 0ebaef5..329a8fa 100644
--- a/include/linux/mmc/sdio.h
+++ b/include/linux/mmc/sdio.h
@@ -94,6 +94,8 @@

#define SDIO_BUS_WIDTH_1BIT 0x00
#define SDIO_BUS_WIDTH_4BIT 0x02
+#define SDIO_BUS_ECSI 0x20 /* Enable continuous SPI interrupt */
+#define SDIO_BUS_SCSI 0x40 /* Support continuous SPI interrupt */

#define SDIO_BUS_ASYNC_INT 0x20

--
1.7.0.3


2010-04-13 13:15:21

by John W. Linville

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

On Tue, Apr 13, 2010 at 12:29:54PM +0200, Daniel Mack wrote:
> On Tue, Apr 06, 2010 at 09:07:34AM -0700, Dan Williams wrote:
> > On Tue, 2010-04-06 at 10:52 +0200, Daniel Mack wrote:
> > > 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 <[email protected]>
> > > Cc: Alagu Sankar <[email protected]>
> > > Cc: Volker Ernst <[email protected]>
> > > Cc: Dan Williams <[email protected]>
> > > Cc: John W. Linville <[email protected]>
> > > Cc: Holger Schurig <[email protected]>
> > > Cc: Bing Zhao <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected]
> >
> > Acked-by: Dan Williams <[email protected]>
>
> Was this picked by anyone?
> Just asking because I didn't see it in the wireless-2.6.git yet.

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
[email protected] might be all we have. Be ready.

2010-04-06 16:08:08

by Dan Williams

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

On Tue, 2010-04-06 at 10:52 +0200, Daniel Mack wrote:
> 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 <[email protected]>
> Cc: Alagu Sankar <[email protected]>
> Cc: Volker Ernst <[email protected]>
> Cc: Dan Williams <[email protected]>
> Cc: John W. Linville <[email protected]>
> Cc: Holger Schurig <[email protected]>
> Cc: Bing Zhao <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]

Acked-by: Dan Williams <[email protected]>

> ---
> 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, &ret);
> + if (ret)
> + goto release_int;
> +
> + reg |= SDIO_BUS_ECSI;
> + sdio_f0_writeb(func, reg, SDIO_CCCR_IF, &ret);
> + if (ret)
> + goto release_int;
> + }
> +
> card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
> if (ret)
> goto release_int;
> diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> index 0ebaef5..329a8fa 100644
> --- a/include/linux/mmc/sdio.h
> +++ b/include/linux/mmc/sdio.h
> @@ -94,6 +94,8 @@
>
> #define SDIO_BUS_WIDTH_1BIT 0x00
> #define SDIO_BUS_WIDTH_4BIT 0x02
> +#define SDIO_BUS_ECSI 0x20 /* Enable continuous SPI interrupt */
> +#define SDIO_BUS_SCSI 0x40 /* Support continuous SPI interrupt */
>
> #define SDIO_BUS_ASYNC_INT 0x20
>



2010-04-13 10:30:01

by Daniel Mack

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

On Tue, Apr 06, 2010 at 09:07:34AM -0700, Dan Williams wrote:
> On Tue, 2010-04-06 at 10:52 +0200, Daniel Mack wrote:
> > 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 <[email protected]>
> > Cc: Alagu Sankar <[email protected]>
> > Cc: Volker Ernst <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Cc: John W. Linville <[email protected]>
> > Cc: Holger Schurig <[email protected]>
> > Cc: Bing Zhao <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
>
> Acked-by: Dan Williams <[email protected]>

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

Thanks,
Daniel



>
> > ---
> > 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, &ret);
> > + if (ret)
> > + goto release_int;
> > +
> > + reg |= SDIO_BUS_ECSI;
> > + sdio_f0_writeb(func, reg, SDIO_CCCR_IF, &ret);
> > + if (ret)
> > + goto release_int;
> > + }
> > +
> > card->ioport = sdio_readb(func, IF_SDIO_IOPORT, &ret);
> > if (ret)
> > goto release_int;
> > diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
> > index 0ebaef5..329a8fa 100644
> > --- a/include/linux/mmc/sdio.h
> > +++ b/include/linux/mmc/sdio.h
> > @@ -94,6 +94,8 @@
> >
> > #define SDIO_BUS_WIDTH_1BIT 0x00
> > #define SDIO_BUS_WIDTH_4BIT 0x02
> > +#define SDIO_BUS_ECSI 0x20 /* Enable continuous SPI interrupt */
> > +#define SDIO_BUS_SCSI 0x40 /* Support continuous SPI interrupt */
> >
> > #define SDIO_BUS_ASYNC_INT 0x20
> >
>
>

2010-04-13 13:22:02

by Daniel Mack

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

On Tue, Apr 13, 2010 at 09:06:54AM -0400, John W. Linville wrote:
> On Tue, Apr 13, 2010 at 12:29:54PM +0200, Daniel Mack wrote:
> > Was this picked by anyone?
> > Just asking because I didn't see it in the wireless-2.6.git yet.
>
> It is in wireless-next-2.6, queued for 2.6.35.

Thanks a lot!

Daniel