2009-03-22 00:27:15

by Marek Vasut

[permalink] [raw]
Subject: [PATCH] Marvell CF8381

Hi,
I've finally got this card working ...
See the patches below.


Attachments:
(No filename) (66.00 B)
0002-Add-support-for-CF8381-WiFi-card.patch (1.94 kB)
0001-Correct-return-value-of-firmware-loading-functions-h.patch (944.00 B)
Download all attachments

2009-03-24 10:28:53

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Tue, 2009-03-24 at 00:42 +0100, Marek Vasut wrote:
> Ok, here is another version, hopefully last one.
>
> From a7522df9d610702cfc3066861a2e315552e29086 Mon Sep 17 00:00:00 2001
> From: Marek Vasut <[email protected]>
> Date: Tue, 24 Mar 2009 00:40:44 +0100
> Subject: [PATCH] Add support for CF8381 WiFi card.
> A detection function was added for identifying CF8381.
>
> Signed-off-by: Marek Vasut <[email protected]>

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

> ---
> drivers/net/wireless/libertas/if_cs.c | 34 +++++++++++++++++++++++++++++++-
> 1 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/libertas/if_cs.c
> b/drivers/net/wireless/libertas/if_cs.c
> index 3f02e6a..1da23e5 100644
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -273,7 +273,28 @@ static int if_cs_poll_while_fw_download(struct if_cs_card
> *card, uint addr, u8 r
> */
> #define IF_CS_PRODUCT_ID 0x0000001C
> #define IF_CS_CF8385_B1_REV 0x12
> +#define IF_CS_CF8381_B3_REV 0x04
>
> +/*
> + * Used to detect other cards than CF8385 since their revisions of silicon
> + * doesn't match those from CF8385, eg. CF8381 B3 works with this driver.
> + */
> +#define CF8381_MANFID 0x02db
> +#define CF8381_CARDID 0x6064
> +#define CF8385_MANFID 0x02df
> +#define CF8385_CARDID 0x8103
> +
> +static inline int if_cs_hw_is_cf8381(struct pcmcia_device *p_dev)
> +{
> + return (p_dev->manf_id == CF8381_MANFID &&
> + p_dev->card_id == CF8381_CARDID);
> +}
> +
> +static inline int if_cs_hw_is_cf8385(struct pcmcia_device *p_dev)
> +{
> + return (p_dev->manf_id == CF8385_MANFID &&
> + p_dev->card_id == CF8385_CARDID);
> +}
>
> /********************************************************************/
> /* I/O and interrupt handling */
> @@ -757,6 +778,7 @@ static void if_cs_release(struct pcmcia_device *p_dev)
> static int if_cs_probe(struct pcmcia_device *p_dev)
> {
> int ret = -ENOMEM;
> + unsigned int prod_id;
> struct lbs_private *priv;
> struct if_cs_card *card;
> /* CIS parsing */
> @@ -859,7 +881,14 @@ static int if_cs_probe(struct pcmcia_device *p_dev)
> p_dev->io.BasePort1 + p_dev->io.NumPorts1 - 1);
>
> /* Check if we have a current silicon */
> - if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
> + prod_id = if_cs_read8(card, IF_CS_PRODUCT_ID);
> + if (if_cs_hw_is_cf8381(p_dev) && prod_id < IF_CS_CF8381_B3_REV) {
> + lbs_pr_err("old chips like 8381 rev B3 aren't supported\n");
> + ret = -ENODEV;
> + goto out2;
> + }
> +
> + if (if_cs_hw_is_cf8385(p_dev) && prod_id < IF_CS_CF8385_B1_REV) {
> lbs_pr_err("old chips like 8385 rev B1 aren't supported\n");
> ret = -ENODEV;
> goto out2;
> @@ -950,7 +979,8 @@ static void if_cs_detach(struct pcmcia_device *p_dev)
> /********************************************************************/
>
> static struct pcmcia_device_id if_cs_ids[] = {
> - PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
> + PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
> + PCMCIA_DEVICE_MANF_CARD(CF8385_MANFID, CF8385_CARDID),
> PCMCIA_DEVICE_NULL,
> };
> MODULE_DEVICE_TABLE(pcmcia, if_cs_ids);
> --
> 1.6.2
>


2009-03-24 20:33:37

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Tuesday 24 of March 2009 21:01:18 John W. Linville wrote:
> On Tue, Mar 24, 2009 at 06:26:27AM -0400, Dan Williams wrote:
> > On Tue, 2009-03-24 at 00:42 +0100, Marek Vasut wrote:
> > > Ok, here is another version, hopefully last one.
> > >
> > > From a7522df9d610702cfc3066861a2e315552e29086 Mon Sep 17 00:00:00 2001
> > > From: Marek Vasut <[email protected]>
> > > Date: Tue, 24 Mar 2009 00:40:44 +0100
> > > Subject: [PATCH] Add support for CF8381 WiFi card.
> > > A detection function was added for identifying CF8381.
> > >
> > > Signed-off-by: Marek Vasut <[email protected]>
> >
> > Acked-by: Dan Williams <[email protected]>
>
> The patch is whitespace-damaged. Please resubmit w/o whitespace
> damage in a separate email.
Sending as an attachment so it wont get broken.
>
> Thanks,
>
> John



Attachments:
(No filename) (814.00 B)
0001-Add-support-for-CF8381-WiFi-card.patch (2.85 kB)
Download all attachments

2009-03-23 16:13:34

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH1/2] Fix return value handling

On Mon, 2009-03-23 at 16:59 +0100, Marek Vasut wrote:
> On Monday 23 of March 2009 16:57:24 Marek Vasut wrote:
> > Hi,
> > here is a resend. One patch per mail.
> sorry, inlining
>
> From fd2e610a87a8372cbc513e336fa71e3438742c9d Mon Sep 17 00:00:00 2001
> From: Marek Vasut <[email protected]>
> Date: Mon, 23 Mar 2009 15:57:11 +0100
> Subject: [PATCH 1/2] Firmware loading functions can return possitive values
> This is not a bug, but take it into consideration and handle it properly.

This patch should no longer be required, since the patch "libertas: fix
CF firmware loading for some cards" was applied. The firmware helpers
should only return an error (< 0) or 0 (success).

Dan

> Signed-off-by: Marek Vasut <[email protected]>
> ---
> drivers/net/wireless/libertas/if_cs.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/libertas/if_cs.c
> b/drivers/net/wireless/libertas/if_cs.c
> index 842a08d..3f02e6a 100644
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -867,9 +867,9 @@ static int if_cs_probe(struct pcmcia_device *p_dev)
>
> /* Load the firmware early, before calling into libertas.ko */
> ret = if_cs_prog_helper(card);
> - if (ret == 0)
> + if (ret >= 0)
> ret = if_cs_prog_real(card);
> - if (ret)
> + if (ret < 0)
> goto out2;
>
> /* Make this card known to the libertas driver */


2009-03-22 04:11:43

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Sunday 22 of March 2009 01:27:21 Marek Vasut wrote:
> Hi,
> I've finally got this card working ...
> See the patches below.


One more thing - I also had to apply the following patch in order to get
region code detected properly.

le16_to_cpu(cmd.regioncode) = 0x3031 for me and Im definitelly not in spain,
but 0x30 (eu) looks reasonable.

diff --git a/drivers/net/wireless/libertas/cmd.c
b/drivers/net/wireless/libertas/cmd.c
index 639dd02..ce32bc9 100644
--- a/drivers/net/wireless/libertas/cmd.c
+++ b/drivers/net/wireless/libertas/cmd.c
@@ -123,7 +123,7 @@ int lbs_update_hw_spec(struct lbs_private *priv)
* only ever be 8-bit, even though the field size is 16-bit. Some
firmware
* returns non-zero high 8 bits here.
*/
- priv->regioncode = le16_to_cpu(cmd.regioncode) & 0xFF;
+ priv->regioncode = (le16_to_cpu(cmd.regioncode) & 0xFF00) >> 8;

for (i = 0; i < MRVDRV_MAX_REGION_CODE; i++) {
/* use the region code to search for the index */

2009-03-23 15:58:58

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381 and CF8305

Here's the other patch.
I also added cf8305 support (if_cs.c patched this way works for it too).


Attachments:
(No filename) (97.00 B)
0002-Add-support-for-CF8381-and-CF8305-WiFi-card.patch (3.13 kB)
Download all attachments

2009-03-22 13:01:52

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Sunday 22 of March 2009 09:04:08 Johannes Berg wrote:
> On Sun, 2009-03-22 at 05:11 +0100, Marek Vasut wrote:
> > One more thing - I also had to apply the following patch in order to get
> > region code detected properly.
> >
> > le16_to_cpu(cmd.regioncode) = 0x3031 for me and Im definitelly not in
> > spain, but 0x30 (eu) looks reasonable.
> >
> > diff --git a/drivers/net/wireless/libertas/cmd.c
> > b/drivers/net/wireless/libertas/cmd.c
> > index 639dd02..ce32bc9 100644
> > --- a/drivers/net/wireless/libertas/cmd.c
> > +++ b/drivers/net/wireless/libertas/cmd.c
> > @@ -123,7 +123,7 @@ int lbs_update_hw_spec(struct lbs_private *priv)
> > * only ever be 8-bit, even though the field size is 16-bit.
> > Some firmware
> > * returns non-zero high 8 bits here.
> > */
> > - priv->regioncode = le16_to_cpu(cmd.regioncode) & 0xFF;
> > + priv->regioncode = (le16_to_cpu(cmd.regioncode) & 0xFF00) >> 8;
>
> I'd be more inclined to think that this was an endian bug? Does your
> machine happen to be big endian?

intel pxa270 (little endian). Btw. that macro le16_to_cpu should handle the
endianness, shouldn't it ?

>
> johannes



2009-03-23 12:13:21

by Holger Schurig

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

Hi Marek !

Great.


Some nitpicks:

1. you attached the patches, not inline
2. There's no signed-off
3. You didn't submit one mail per patch

All of this makes review harder. If you adhere a bit to
Documentation/SubmittingPatches you'd get the fame all by
yourself :-)


Some more rants:


--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -273,7 +273,14 @@ static int if_cs_poll_while_fw_download(struct if_cs_card *card, uint addr, u8 r
*/
#define IF_CS_PRODUCT_ID 0x0000001C
#define IF_CS_CF8385_B1_REV 0x12
+#define IF_CS_CF8381_B3_REV 0x04

Here's a tab, not a space. Maybe you run scripts/checkpatch.pl on it as well?




- if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
+ if (!(p_dev->manf_id == CF8381_MANFID &&
+ p_dev->card_id == CF8381_CARDID) &&
+ (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV)) {

I think change then breaks the existing logic for 8385 cards.
code broke then the 8385 case. Therefore a NAK for now.




Kindly repost both patches again, with signed-off and fixes, thanks!

2009-03-23 16:06:26

by Holger Schurig

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381 and CF8305

> +#define CF8305_MANFID 0x02db
> +#define CF8305_CARDID 0x8103

It's 8385, not 8305.

> /* Check if we have a current silicon */
> - if (if_cs_read8(card, IF_CS_PRODUCT_ID) <
> IF_CS_CF8385_B1_REV) { - lbs_pr_err("old chips like 8385 rev
> B1 aren't supported\n"); + prod_id = if_cs_read8(card,
> IF_CS_PRODUCT_ID);
> + if (!(if_cs_hw_is_cf8305(p_dev) ||
> + (if_cs_hw_is_cf8381(p_dev) &&
> + prod_id >= IF_CS_CF8381_B3_REV)) &&
> + (prod_id < IF_CS_CF8385_B1_REV)) {
> + lbs_pr_err("old chips like 8385 rev B1 or "
> + "8381 rev B3 aren't supported\n");

I still find this if hard to read. Why not something like this:

if ((if_cs_is_8385() && prod_id < IF_CS_CF8385_B1_REV) ||
(if_cs_Is_8381() && prod_id < IF_CS_CF8381_B3_REV)) {
....
}

> static struct pcmcia_device_id if_cs_ids[] = {
> + PCMCIA_DEVICE_MANF_CARD(CF8305_MANFID, CF8305_CARDID),
> + PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
> PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
> PCMCIA_DEVICE_NULL,
> };

Now we end with two entries of 0x02df, 0x8103 :-/

2009-03-23 17:09:36

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381 and CF8305

On Monday 23 of March 2009 17:06:16 Holger Schurig wrote:
> > +#define CF8305_MANFID 0x02db
> > +#define CF8305_CARDID 0x8103
>
> It's 8385, not 8305.
it's 8305 ... that's even older card ;)
>
> > /* Check if we have a current silicon */
> > - if (if_cs_read8(card, IF_CS_PRODUCT_ID) <
> > IF_CS_CF8385_B1_REV) { - lbs_pr_err("old chips like 8385 rev
> > B1 aren't supported\n"); + prod_id = if_cs_read8(card,
> > IF_CS_PRODUCT_ID);
> > + if (!(if_cs_hw_is_cf8305(p_dev) ||
> > + (if_cs_hw_is_cf8381(p_dev) &&
> > + prod_id >= IF_CS_CF8381_B3_REV)) &&
> > + (prod_id < IF_CS_CF8385_B1_REV)) {
> > + lbs_pr_err("old chips like 8385 rev B1 or "
> > + "8381 rev B3 aren't supported\n");
>
> I still find this if hard to read. Why not something like this:
>
> if ((if_cs_is_8385() && prod_id < IF_CS_CF8385_B1_REV) ||
> (if_cs_Is_8381() && prod_id < IF_CS_CF8381_B3_REV)) {
> ....
> }
>
> > static struct pcmcia_device_id if_cs_ids[] = {
> > + PCMCIA_DEVICE_MANF_CARD(CF8305_MANFID, CF8305_CARDID),
> > + PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
> > PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
> > PCMCIA_DEVICE_NULL,
> > };
>
> Now we end with two entries of 0x02df, 0x8103 :-/



2009-03-23 23:42:34

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

Ok, here is another version, hopefully last one.

>From a7522df9d610702cfc3066861a2e315552e29086 Mon Sep 17 00:00:00 2001
From: Marek Vasut <[email protected]>
Date: Tue, 24 Mar 2009 00:40:44 +0100
Subject: [PATCH] Add support for CF8381 WiFi card.
A detection function was added for identifying CF8381.

Signed-off-by: Marek Vasut <[email protected]>
---
drivers/net/wireless/libertas/if_cs.c | 34 +++++++++++++++++++++++++++++++-
1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_cs.c
b/drivers/net/wireless/libertas/if_cs.c
index 3f02e6a..1da23e5 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -273,7 +273,28 @@ static int if_cs_poll_while_fw_download(struct if_cs_card
*card, uint addr, u8 r
*/
#define IF_CS_PRODUCT_ID 0x0000001C
#define IF_CS_CF8385_B1_REV 0x12
+#define IF_CS_CF8381_B3_REV 0x04

+/*
+ * Used to detect other cards than CF8385 since their revisions of silicon
+ * doesn't match those from CF8385, eg. CF8381 B3 works with this driver.
+ */
+#define CF8381_MANFID 0x02db
+#define CF8381_CARDID 0x6064
+#define CF8385_MANFID 0x02df
+#define CF8385_CARDID 0x8103
+
+static inline int if_cs_hw_is_cf8381(struct pcmcia_device *p_dev)
+{
+ return (p_dev->manf_id == CF8381_MANFID &&
+ p_dev->card_id == CF8381_CARDID);
+}
+
+static inline int if_cs_hw_is_cf8385(struct pcmcia_device *p_dev)
+{
+ return (p_dev->manf_id == CF8385_MANFID &&
+ p_dev->card_id == CF8385_CARDID);
+}

/********************************************************************/
/* I/O and interrupt handling */
@@ -757,6 +778,7 @@ static void if_cs_release(struct pcmcia_device *p_dev)
static int if_cs_probe(struct pcmcia_device *p_dev)
{
int ret = -ENOMEM;
+ unsigned int prod_id;
struct lbs_private *priv;
struct if_cs_card *card;
/* CIS parsing */
@@ -859,7 +881,14 @@ static int if_cs_probe(struct pcmcia_device *p_dev)
p_dev->io.BasePort1 + p_dev->io.NumPorts1 - 1);

/* Check if we have a current silicon */
- if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
+ prod_id = if_cs_read8(card, IF_CS_PRODUCT_ID);
+ if (if_cs_hw_is_cf8381(p_dev) && prod_id < IF_CS_CF8381_B3_REV) {
+ lbs_pr_err("old chips like 8381 rev B3 aren't supported\n");
+ ret = -ENODEV;
+ goto out2;
+ }
+
+ if (if_cs_hw_is_cf8385(p_dev) && prod_id < IF_CS_CF8385_B1_REV) {
lbs_pr_err("old chips like 8385 rev B1 aren't supported\n");
ret = -ENODEV;
goto out2;
@@ -950,7 +979,8 @@ static void if_cs_detach(struct pcmcia_device *p_dev)
/********************************************************************/

static struct pcmcia_device_id if_cs_ids[] = {
- PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
+ PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
+ PCMCIA_DEVICE_MANF_CARD(CF8385_MANFID, CF8385_CARDID),
PCMCIA_DEVICE_NULL,
};
MODULE_DEVICE_TABLE(pcmcia, if_cs_ids);
--
1.6.2


2009-03-23 17:01:06

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381 and CF8305

On Mon, 2009-03-23 at 17:00 +0100, Marek Vasut wrote:
> On Monday 23 of March 2009 16:59:05 Marek Vasut wrote:
> > Here's the other patch.
> > I also added cf8305 support (if_cs.c patched this way works for it too).
>
> here as well...inlining
>
> From 4c7fcf79de8ba900c4a9a56e68c9da89491dff27 Mon Sep 17 00:00:00 2001
> From: Marek Vasut <[email protected]>
> Date: Mon, 23 Mar 2009 16:50:30 +0100
> Subject: [PATCH 2/2] Add support for CF8381 and CF8305 WiFi card.
> Two detection functions were added for identifying CF8381 and CF8305.
> Also, CF8305 uses only one-stage firmware, so handle this as well.
>
> Signed-off-by: Marek Vasut <[email protected]>
> ---
> drivers/net/wireless/libertas/if_cs.c | 35 ++++++++++++++++++++++++++++++--
> 1 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/libertas/if_cs.c
> b/drivers/net/wireless/libertas/if_cs.c
> index 3f02e6a..e115c8f 100644
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -273,7 +273,28 @@ static int if_cs_poll_while_fw_download(struct if_cs_card
> *card, uint addr, u8 r
> */
> #define IF_CS_PRODUCT_ID 0x0000001C
> #define IF_CS_CF8385_B1_REV 0x12
> +#define IF_CS_CF8381_B3_REV 0x04
>
> +/*
> + * Used to detect other cards than CF8385 since their revisions of silicon
> + * doesn't match those from CF8385, eg. CF8381 B3 works with this driver.
> + */
> +#define CF8305_MANFID 0x02db
> +#define CF8305_CARDID 0x8103
> +#define CF8381_MANFID 0x02db
> +#define CF8381_CARDID 0x6064
> +
> +static inline int if_cs_hw_is_cf8305(struct pcmcia_device *p_dev)
> +{
> + return (p_dev->manf_id == CF8305_MANFID &&
> + p_dev->card_id == CF8305_CARDID);
> +}
> +
> +static inline int if_cs_hw_is_cf8381(struct pcmcia_device *p_dev)
> +{
> + return (p_dev->manf_id == CF8381_MANFID &&
> + p_dev->card_id == CF8381_CARDID);
> +}
>
> /********************************************************************/
> /* I/O and interrupt handling */
> @@ -757,6 +778,7 @@ static void if_cs_release(struct pcmcia_device *p_dev)
> static int if_cs_probe(struct pcmcia_device *p_dev)
> {
> int ret = -ENOMEM;
> + unsigned int prod_id;
> struct lbs_private *priv;
> struct if_cs_card *card;
> /* CIS parsing */
> @@ -859,15 +881,20 @@ static int if_cs_probe(struct pcmcia_device *p_dev)
> p_dev->io.BasePort1 + p_dev->io.NumPorts1 - 1);
>
> /* Check if we have a current silicon */
> - if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
> - lbs_pr_err("old chips like 8385 rev B1 aren't supported\n");
> + prod_id = if_cs_read8(card, IF_CS_PRODUCT_ID);
> + if (!(if_cs_hw_is_cf8305(p_dev) ||
> + (if_cs_hw_is_cf8381(p_dev) &&
> + prod_id >= IF_CS_CF8381_B3_REV)) &&
> + (prod_id < IF_CS_CF8385_B1_REV)) {
> + lbs_pr_err("old chips like 8385 rev B1 or "
> + "8381 rev B3 aren't supported\n");

This gets ugly fast; couldn't we instead do something like:

if (if_cs_hw_is_cf8381(p_dev) && (prod_id < IF_CS_CF8381_B3_REV)) {
lbs_pr_err("Only 88w8381 hardware revisions B3 and later"
" are supported.\n"
ret = -ENODEV;
goto out2;
}

if (if_cs_hw_is_cf8385(p_dev) && (prod_id <= IF_CS_CF8385_B1_REV)) {
lbs_pr_err("Only 88w8385 hardware revisions B2 and later"
" are supported.\n"
ret = -ENODEV;
goto out2;
}

instead?

> ret = -ENODEV;
> goto out2;
> }
>
> /* Load the firmware early, before calling into libertas.ko */
> ret = if_cs_prog_helper(card);
> - if (ret >= 0)
> + if (ret >= 0 && !if_cs_hw_is_cf8305(p_dev))
> ret = if_cs_prog_real(card);

Again, not required with current wireless-testing.

Dan

> if (ret < 0)
> goto out2;
> @@ -950,6 +977,8 @@ static void if_cs_detach(struct pcmcia_device *p_dev)
> /********************************************************************/
>
> static struct pcmcia_device_id if_cs_ids[] = {
> + PCMCIA_DEVICE_MANF_CARD(CF8305_MANFID, CF8305_CARDID),
> + PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
> PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
> PCMCIA_DEVICE_NULL,
> };


2009-03-23 21:58:52

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Monday 23 of March 2009 17:15:27 Dan Williams wrote:
> On Mon, 2009-03-23 at 13:27 +0100, Holger Schurig wrote:
> > > - priv->regioncode = le16_to_cpu(cmd.regioncode) & 0xFF;
> > > + priv->regioncode = (le16_to_cpu(cmd.regioncode) &
> >
> > 0xFF00) >> 8;
> >
> > Hmm, the change that this breaks 8385 is quite high, maybe 100
> > percent.
>
> The cf8385-5.0.16.p0-26306 (gumstix) driver has:
>
> Adapter->RegionCode = wlan_le16_to_cpu(hwspec->RegionCode) >> 8;
>
> So it appears there's precedent for this even on 8385 parts.
>
> Dan
>
> > It could simply be the case that your firmware returns the value
> > in a different order than mine. So we either need to test for the
> > firmware version or for the hardware before getting the value via
> > one or the other method.
> >
> > Maybe it's worthwhile to create some hw_is_8381() function and
> > then do the things that need to be differently based on it's
> > return value?
> >
> >
> >
> > The documentation for CMG_GET_HW_SPEC that I have says
> >
> > RegionCode UINT16 Set to 0
> >
> > so I see no indicator that we have two 8 bit values here. But
> > we've seen bugs in docs before :-)
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wireless"
> > in the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html

Ok, so this should be it. I dropped the 8305 support since I found I have some
issues with it still.

>From 3ac8f34db9c4d96197fbdc2776ebbcd571e3fbbe Mon Sep 17 00:00:00 2001
From: Marek Vasut <[email protected]>
Date: Mon, 23 Mar 2009 22:55:51 +0100
Subject: [PATCH] Add support for CF8381 WiFi card.
A detection function was added for identifying CF8381.

Signed-off-by: <[email protected]>
---
drivers/net/wireless/libertas/if_cs.c | 24 +++++++++++++++++++++++-
1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_cs.c
b/drivers/net/wireless/libertas/if_cs.c
index 3f02e6a..56b6475 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -273,7 +273,20 @@ static int if_cs_poll_while_fw_download(struct if_cs_card
*card, uint addr, u8 r
*/
#define IF_CS_PRODUCT_ID 0x0000001C
#define IF_CS_CF8385_B1_REV 0x12
+#define IF_CS_CF8381_B3_REV 0x04

+/*
+ * Used to detect other cards than CF8385 since their revisions of silicon
+ * doesn't match those from CF8385, eg. CF8381 B3 works with this driver.
+ */
+#define CF8381_MANFID 0x02db
+#define CF8381_CARDID 0x6064
+
+static inline int if_cs_hw_is_cf8381(struct pcmcia_device *p_dev)
+{
+ return (p_dev->manf_id == CF8381_MANFID &&
+ p_dev->card_id == CF8381_CARDID);
+}

/********************************************************************/
/* I/O and interrupt handling */
@@ -757,6 +770,7 @@ static void if_cs_release(struct pcmcia_device *p_dev)
static int if_cs_probe(struct pcmcia_device *p_dev)
{
int ret = -ENOMEM;
+ unsigned int prod_id;
struct lbs_private *priv;
struct if_cs_card *card;
/* CIS parsing */
@@ -859,7 +873,14 @@ static int if_cs_probe(struct pcmcia_device *p_dev)
p_dev->io.BasePort1 + p_dev->io.NumPorts1 - 1);

/* Check if we have a current silicon */
- if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
+ prod_id = if_cs_read8(card, IF_CS_PRODUCT_ID);
+ if (if_cs_hw_is_cf8381(p_dev) && prod_id < IF_CS_CF8381_B3_REV) {
+ lbs_pr_err("old chips like 8381 rev B3 aren't supported\n");
+ ret = -ENODEV;
+ goto out2;
+ }
+
+ if (prod_id < IF_CS_CF8385_B1_REV) {
lbs_pr_err("old chips like 8385 rev B1 aren't supported\n");
ret = -ENODEV;
goto out2;
@@ -950,6 +971,7 @@ static void if_cs_detach(struct pcmcia_device *p_dev)
/********************************************************************/

static struct pcmcia_device_id if_cs_ids[] = {
+ PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
PCMCIA_DEVICE_NULL,
};
--
1.6.2


2009-03-22 08:04:15

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Sun, 2009-03-22 at 05:11 +0100, Marek Vasut wrote:

> One more thing - I also had to apply the following patch in order to get
> region code detected properly.
>
> le16_to_cpu(cmd.regioncode) = 0x3031 for me and Im definitelly not in spain,
> but 0x30 (eu) looks reasonable.
>
> diff --git a/drivers/net/wireless/libertas/cmd.c
> b/drivers/net/wireless/libertas/cmd.c
> index 639dd02..ce32bc9 100644
> --- a/drivers/net/wireless/libertas/cmd.c
> +++ b/drivers/net/wireless/libertas/cmd.c
> @@ -123,7 +123,7 @@ int lbs_update_hw_spec(struct lbs_private *priv)
> * only ever be 8-bit, even though the field size is 16-bit. Some
> firmware
> * returns non-zero high 8 bits here.
> */
> - priv->regioncode = le16_to_cpu(cmd.regioncode) & 0xFF;
> + priv->regioncode = (le16_to_cpu(cmd.regioncode) & 0xFF00) >> 8;

I'd be more inclined to think that this was an endian bug? Does your
machine happen to be big endian?

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2009-03-23 12:27:27

by Holger Schurig

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

> - priv->regioncode = le16_to_cpu(cmd.regioncode) & 0xFF;
> + priv->regioncode = (le16_to_cpu(cmd.regioncode) &
0xFF00) >> 8;

Hmm, the change that this breaks 8385 is quite high, maybe 100
percent.

It could simply be the case that your firmware returns the value
in a different order than mine. So we either need to test for the
firmware version or for the hardware before getting the value via
one or the other method.

Maybe it's worthwhile to create some hw_is_8381() function and
then do the things that need to be differently based on it's
return value?



The documentation for CMG_GET_HW_SPEC that I have says

RegionCode UINT16 Set to 0

so I see no indicator that we have two 8 bit values here. But
we've seen bugs in docs before :-)

2009-03-23 14:34:18

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Monday 23 of March 2009 13:13:10 Holger Schurig wrote:
> Hi Marek !
>
> Great.
>
>
> Some nitpicks:
>
> 1. you attached the patches, not inline
Yes, I know, I just needed some comments if they are OK
> 2. There's no signed-off
cat ../0001-Correct-return-value-of-firmware-loading-functions-h.patch |
head -n 6
~~~~~~~
Signed-off-by: Marek Vasut <[email protected]>

cat ../0002-Add-support-for-CF8381-WiFi-card.patch | head -n 6
~~~~~~~
Signed-off-by: Marek Vasut <[email protected]>

So as you see, there is.
> 3. You didn't submit one mail per patch
Yes, I know, see 1.
>
> All of this makes review harder. If you adhere a bit to
> Documentation/SubmittingPatches you'd get the fame all by
> yourself :-)
>
>
> Some more rants:
>
>
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -273,7 +273,14 @@ static int if_cs_poll_while_fw_download(struct
> if_cs_card *card, uint addr, u8 r */
> #define IF_CS_PRODUCT_ID 0x0000001C
> #define IF_CS_CF8385_B1_REV 0x12
> +#define IF_CS_CF8381_B3_REV 0x04
>
> Here's a tab, not a space. Maybe you run scripts/checkpatch.pl on it as
> well?
# ./scripts/checkpatch.pl ../0001-Correct-return-value-of-firmware-loading-functions-h.patch
total: 0 errors, 0 warnings, 11 lines checked

../0001-Correct-return-value-of-firmware-loading-functions-h.patch has no
obvious style problems and is ready for submission.
# ./scripts/checkpatch.pl ../0002-Add-support-for-CF8381-WiFi-card.patch
total: 0 errors, 0 warnings, 31 lines checked

../0002-Add-support-for-CF8381-WiFi-card.patch has no obvious style problems
and is ready for submission.

As you can see, I did. Otherwise I wont send them.
>
>
>
>
> - if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
> + if (!(p_dev->manf_id == CF8381_MANFID &&
> + p_dev->card_id == CF8381_CARDID) &&
> + (if_cs_read8(card, IF_CS_PRODUCT_ID) <
> IF_CS_CF8385_B1_REV)) {
>
> I think change then breaks the existing logic for 8385 cards.
> code broke then the 8385 case. Therefore a NAK for now.
This might need review ... hang on, I'll recheck
>
>
>
>
> Kindly repost both patches again, with signed-off and fixes, thanks!



2009-03-24 20:15:42

by John W. Linville

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Tue, Mar 24, 2009 at 06:26:27AM -0400, Dan Williams wrote:
> On Tue, 2009-03-24 at 00:42 +0100, Marek Vasut wrote:
> > Ok, here is another version, hopefully last one.
> >
> > From a7522df9d610702cfc3066861a2e315552e29086 Mon Sep 17 00:00:00 2001
> > From: Marek Vasut <[email protected]>
> > Date: Tue, 24 Mar 2009 00:40:44 +0100
> > Subject: [PATCH] Add support for CF8381 WiFi card.
> > A detection function was added for identifying CF8381.
> >
> > Signed-off-by: Marek Vasut <[email protected]>
>
> Acked-by: Dan Williams <[email protected]>

The patch is whitespace-damaged. Please resubmit w/o whitespace
damage in a separate email.

Thanks,

John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.

2009-03-23 17:22:13

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381 and CF8305

On Mon, 2009-03-23 at 18:09 +0100, Marek Vasut wrote:
> On Monday 23 of March 2009 17:06:16 Holger Schurig wrote:
> > > +#define CF8305_MANFID 0x02db
> > > +#define CF8305_CARDID 0x8103
> >
> > It's 8385, not 8305.
> it's 8305 ... that's even older card ;)

v4 firmware or v5?

Dan

> >
> > > /* Check if we have a current silicon */
> > > - if (if_cs_read8(card, IF_CS_PRODUCT_ID) <
> > > IF_CS_CF8385_B1_REV) { - lbs_pr_err("old chips like 8385 rev
> > > B1 aren't supported\n"); + prod_id = if_cs_read8(card,
> > > IF_CS_PRODUCT_ID);
> > > + if (!(if_cs_hw_is_cf8305(p_dev) ||
> > > + (if_cs_hw_is_cf8381(p_dev) &&
> > > + prod_id >= IF_CS_CF8381_B3_REV)) &&
> > > + (prod_id < IF_CS_CF8385_B1_REV)) {
> > > + lbs_pr_err("old chips like 8385 rev B1 or "
> > > + "8381 rev B3 aren't supported\n");
> >
> > I still find this if hard to read. Why not something like this:
> >
> > if ((if_cs_is_8385() && prod_id < IF_CS_CF8385_B1_REV) ||
> > (if_cs_Is_8381() && prod_id < IF_CS_CF8381_B3_REV)) {
> > ....
> > }
> >
> > > static struct pcmcia_device_id if_cs_ids[] = {
> > > + PCMCIA_DEVICE_MANF_CARD(CF8305_MANFID, CF8305_CARDID),
> > > + PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
> > > PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
> > > PCMCIA_DEVICE_NULL,
> > > };
> >
> > Now we end with two entries of 0x02df, 0x8103 :-/
>
>
>
> _______________________________________________
> libertas-dev mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/libertas-dev


2009-03-23 15:59:42

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH1/2] Fix return value handling

On Monday 23 of March 2009 16:57:24 Marek Vasut wrote:
> Hi,
> here is a resend. One patch per mail.
sorry, inlining

>From fd2e610a87a8372cbc513e336fa71e3438742c9d Mon Sep 17 00:00:00 2001
From: Marek Vasut <[email protected]>
Date: Mon, 23 Mar 2009 15:57:11 +0100
Subject: [PATCH 1/2] Firmware loading functions can return possitive values
This is not a bug, but take it into consideration and handle it properly.

Signed-off-by: Marek Vasut <[email protected]>
---
drivers/net/wireless/libertas/if_cs.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_cs.c
b/drivers/net/wireless/libertas/if_cs.c
index 842a08d..3f02e6a 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -867,9 +867,9 @@ static int if_cs_probe(struct pcmcia_device *p_dev)

/* Load the firmware early, before calling into libertas.ko */
ret = if_cs_prog_helper(card);
- if (ret == 0)
+ if (ret >= 0)
ret = if_cs_prog_real(card);
- if (ret)
+ if (ret < 0)
goto out2;

/* Make this card known to the libertas driver */
--
1.6.2



2009-03-22 13:03:34

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Sun, 2009-03-22 at 14:01 +0100, Marek Vasut wrote:

> > > diff --git a/drivers/net/wireless/libertas/cmd.c
> > > b/drivers/net/wireless/libertas/cmd.c
> > > index 639dd02..ce32bc9 100644
> > > --- a/drivers/net/wireless/libertas/cmd.c
> > > +++ b/drivers/net/wireless/libertas/cmd.c
> > > @@ -123,7 +123,7 @@ int lbs_update_hw_spec(struct lbs_private *priv)
> > > * only ever be 8-bit, even though the field size is 16-bit.
> > > Some firmware
> > > * returns non-zero high 8 bits here.
> > > */
> > > - priv->regioncode = le16_to_cpu(cmd.regioncode) & 0xFF;
> > > + priv->regioncode = (le16_to_cpu(cmd.regioncode) & 0xFF00) >> 8;
> >
> > I'd be more inclined to think that this was an endian bug? Does your
> > machine happen to be big endian?
>
> intel pxa270 (little endian). Btw. that macro le16_to_cpu should handle the
> endianness, shouldn't it ?

Yes, but maybe that part is _not_ a le16 but two u8s. :) But if you're
on LE too then I guess that's not it.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2009-03-23 23:10:56

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Mon, 2009-03-23 at 22:58 +0100, Marek Vasut wrote:
> On Monday 23 of March 2009 17:15:27 Dan Williams wrote:
> > On Mon, 2009-03-23 at 13:27 +0100, Holger Schurig wrote:
> > > > - priv->regioncode = le16_to_cpu(cmd.regioncode) & 0xFF;
> > > > + priv->regioncode = (le16_to_cpu(cmd.regioncode) &
> > >
> > > 0xFF00) >> 8;
> > >
> > > Hmm, the change that this breaks 8385 is quite high, maybe 100
> > > percent.
> >
> > The cf8385-5.0.16.p0-26306 (gumstix) driver has:
> >
> > Adapter->RegionCode = wlan_le16_to_cpu(hwspec->RegionCode) >> 8;
> >
> > So it appears there's precedent for this even on 8385 parts.
> >
> > Dan
> >
> > > It could simply be the case that your firmware returns the value
> > > in a different order than mine. So we either need to test for the
> > > firmware version or for the hardware before getting the value via
> > > one or the other method.
> > >
> > > Maybe it's worthwhile to create some hw_is_8381() function and
> > > then do the things that need to be differently based on it's
> > > return value?
> > >
> > >
> > >
> > > The documentation for CMG_GET_HW_SPEC that I have says
> > >
> > > RegionCode UINT16 Set to 0
> > >
> > > so I see no indicator that we have two 8 bit values here. But
> > > we've seen bugs in docs before :-)
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-wireless"
> > > in the body of a message to [email protected]
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Ok, so this should be it. I dropped the 8305 support since I found I have some
> issues with it still.
>
> From 3ac8f34db9c4d96197fbdc2776ebbcd571e3fbbe Mon Sep 17 00:00:00 2001
> From: Marek Vasut <[email protected]>
> Date: Mon, 23 Mar 2009 22:55:51 +0100
> Subject: [PATCH] Add support for CF8381 WiFi card.
> A detection function was added for identifying CF8381.
>
> Signed-off-by: <[email protected]>
> ---
> drivers/net/wireless/libertas/if_cs.c | 24 +++++++++++++++++++++++-
> 1 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/libertas/if_cs.c
> b/drivers/net/wireless/libertas/if_cs.c
> index 3f02e6a..56b6475 100644
> --- a/drivers/net/wireless/libertas/if_cs.c
> +++ b/drivers/net/wireless/libertas/if_cs.c
> @@ -273,7 +273,20 @@ static int if_cs_poll_while_fw_download(struct if_cs_card
> *card, uint addr, u8 r
> */
> #define IF_CS_PRODUCT_ID 0x0000001C
> #define IF_CS_CF8385_B1_REV 0x12
> +#define IF_CS_CF8381_B3_REV 0x04
>
> +/*
> + * Used to detect other cards than CF8385 since their revisions of silicon
> + * doesn't match those from CF8385, eg. CF8381 B3 works with this driver.
> + */
> +#define CF8381_MANFID 0x02db
> +#define CF8381_CARDID 0x6064
> +
> +static inline int if_cs_hw_is_cf8381(struct pcmcia_device *p_dev)
> +{
> + return (p_dev->manf_id == CF8381_MANFID &&
> + p_dev->card_id == CF8381_CARDID);
> +}
>
> /********************************************************************/
> /* I/O and interrupt handling */
> @@ -757,6 +770,7 @@ static void if_cs_release(struct pcmcia_device *p_dev)
> static int if_cs_probe(struct pcmcia_device *p_dev)
> {
> int ret = -ENOMEM;
> + unsigned int prod_id;
> struct lbs_private *priv;
> struct if_cs_card *card;
> /* CIS parsing */
> @@ -859,7 +873,14 @@ static int if_cs_probe(struct pcmcia_device *p_dev)
> p_dev->io.BasePort1 + p_dev->io.NumPorts1 - 1);
>
> /* Check if we have a current silicon */
> - if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
> + prod_id = if_cs_read8(card, IF_CS_PRODUCT_ID);
> + if (if_cs_hw_is_cf8381(p_dev) && prod_id < IF_CS_CF8381_B3_REV) {
> + lbs_pr_err("old chips like 8381 rev B3 aren't supported\n");
> + ret = -ENODEV;
> + goto out2;
> + }
> +
> + if (prod_id < IF_CS_CF8385_B1_REV) {

You'll still want to implement a if_cs_hw_is_cf8385() here though;
otherwise, wouldn't 8381 get caught by this 8385 check and get rejected?

Dan

> lbs_pr_err("old chips like 8385 rev B1 aren't supported\n");
> ret = -ENODEV;
> goto out2;
> @@ -950,6 +971,7 @@ static void if_cs_detach(struct pcmcia_device *p_dev)
> /********************************************************************/
>
> static struct pcmcia_device_id if_cs_ids[] = {
> + PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
> PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
> PCMCIA_DEVICE_NULL,
> };
> --
> 1.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


2009-03-23 17:39:23

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381 and CF8305

On Monday 23 of March 2009 18:19:55 Dan Williams wrote:
> On Mon, 2009-03-23 at 18:09 +0100, Marek Vasut wrote:
> > On Monday 23 of March 2009 17:06:16 Holger Schurig wrote:
> > > > +#define CF8305_MANFID 0x02db
> > > > +#define CF8305_CARDID 0x8103
> > >
> > > It's 8385, not 8305.
> >
> > it's 8305 ... that's even older card ;)
>
> v4 firmware or v5?

8381 is v4, 8305 is v3

>
> Dan
>
> > > > /* Check if we have a current silicon */
> > > > - if (if_cs_read8(card, IF_CS_PRODUCT_ID) <
> > > > IF_CS_CF8385_B1_REV) { - lbs_pr_err("old chips like 8385 rev
> > > > B1 aren't supported\n"); + prod_id = if_cs_read8(card,
> > > > IF_CS_PRODUCT_ID);
> > > > + if (!(if_cs_hw_is_cf8305(p_dev) ||
> > > > + (if_cs_hw_is_cf8381(p_dev) &&
> > > > + prod_id >= IF_CS_CF8381_B3_REV)) &&
> > > > + (prod_id < IF_CS_CF8385_B1_REV)) {
> > > > + lbs_pr_err("old chips like 8385 rev B1 or "
> > > > + "8381 rev B3 aren't supported\n");
> > >
> > > I still find this if hard to read. Why not something like this:
> > >
> > > if ((if_cs_is_8385() && prod_id < IF_CS_CF8385_B1_REV) ||
> > > (if_cs_Is_8381() && prod_id < IF_CS_CF8381_B3_REV)) {
> > > ....
> > > }
> > >
> > > > static struct pcmcia_device_id if_cs_ids[] = {
> > > > + PCMCIA_DEVICE_MANF_CARD(CF8305_MANFID, CF8305_CARDID),
> > > > + PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
> > > > PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
> > > > PCMCIA_DEVICE_NULL,
> > > > };
> > >
> > > Now we end with two entries of 0x02df, 0x8103 :-/
> >
> > _______________________________________________
> > libertas-dev mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/libertas-dev



2009-03-23 16:00:14

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381 and CF8305

On Monday 23 of March 2009 16:59:05 Marek Vasut wrote:
> Here's the other patch.
> I also added cf8305 support (if_cs.c patched this way works for it too).

here as well...inlining

>From 4c7fcf79de8ba900c4a9a56e68c9da89491dff27 Mon Sep 17 00:00:00 2001
From: Marek Vasut <[email protected]>
Date: Mon, 23 Mar 2009 16:50:30 +0100
Subject: [PATCH 2/2] Add support for CF8381 and CF8305 WiFi card.
Two detection functions were added for identifying CF8381 and CF8305.
Also, CF8305 uses only one-stage firmware, so handle this as well.

Signed-off-by: Marek Vasut <[email protected]>
---
drivers/net/wireless/libertas/if_cs.c | 35 ++++++++++++++++++++++++++++++--
1 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/libertas/if_cs.c
b/drivers/net/wireless/libertas/if_cs.c
index 3f02e6a..e115c8f 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -273,7 +273,28 @@ static int if_cs_poll_while_fw_download(struct if_cs_card
*card, uint addr, u8 r
*/
#define IF_CS_PRODUCT_ID 0x0000001C
#define IF_CS_CF8385_B1_REV 0x12
+#define IF_CS_CF8381_B3_REV 0x04

+/*
+ * Used to detect other cards than CF8385 since their revisions of silicon
+ * doesn't match those from CF8385, eg. CF8381 B3 works with this driver.
+ */
+#define CF8305_MANFID 0x02db
+#define CF8305_CARDID 0x8103
+#define CF8381_MANFID 0x02db
+#define CF8381_CARDID 0x6064
+
+static inline int if_cs_hw_is_cf8305(struct pcmcia_device *p_dev)
+{
+ return (p_dev->manf_id == CF8305_MANFID &&
+ p_dev->card_id == CF8305_CARDID);
+}
+
+static inline int if_cs_hw_is_cf8381(struct pcmcia_device *p_dev)
+{
+ return (p_dev->manf_id == CF8381_MANFID &&
+ p_dev->card_id == CF8381_CARDID);
+}

/********************************************************************/
/* I/O and interrupt handling */
@@ -757,6 +778,7 @@ static void if_cs_release(struct pcmcia_device *p_dev)
static int if_cs_probe(struct pcmcia_device *p_dev)
{
int ret = -ENOMEM;
+ unsigned int prod_id;
struct lbs_private *priv;
struct if_cs_card *card;
/* CIS parsing */
@@ -859,15 +881,20 @@ static int if_cs_probe(struct pcmcia_device *p_dev)
p_dev->io.BasePort1 + p_dev->io.NumPorts1 - 1);

/* Check if we have a current silicon */
- if (if_cs_read8(card, IF_CS_PRODUCT_ID) < IF_CS_CF8385_B1_REV) {
- lbs_pr_err("old chips like 8385 rev B1 aren't supported\n");
+ prod_id = if_cs_read8(card, IF_CS_PRODUCT_ID);
+ if (!(if_cs_hw_is_cf8305(p_dev) ||
+ (if_cs_hw_is_cf8381(p_dev) &&
+ prod_id >= IF_CS_CF8381_B3_REV)) &&
+ (prod_id < IF_CS_CF8385_B1_REV)) {
+ lbs_pr_err("old chips like 8385 rev B1 or "
+ "8381 rev B3 aren't supported\n");
ret = -ENODEV;
goto out2;
}

/* Load the firmware early, before calling into libertas.ko */
ret = if_cs_prog_helper(card);
- if (ret >= 0)
+ if (ret >= 0 && !if_cs_hw_is_cf8305(p_dev))
ret = if_cs_prog_real(card);
if (ret < 0)
goto out2;
@@ -950,6 +977,8 @@ static void if_cs_detach(struct pcmcia_device *p_dev)
/********************************************************************/

static struct pcmcia_device_id if_cs_ids[] = {
+ PCMCIA_DEVICE_MANF_CARD(CF8305_MANFID, CF8305_CARDID),
+ PCMCIA_DEVICE_MANF_CARD(CF8381_MANFID, CF8381_CARDID),
PCMCIA_DEVICE_MANF_CARD(0x02df, 0x8103),
PCMCIA_DEVICE_NULL,
};
--
1.6.2



2009-03-23 16:17:44

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] Marvell CF8381

On Mon, 2009-03-23 at 13:27 +0100, Holger Schurig wrote:
> > - priv->regioncode = le16_to_cpu(cmd.regioncode) & 0xFF;
> > + priv->regioncode = (le16_to_cpu(cmd.regioncode) &
> 0xFF00) >> 8;
>
> Hmm, the change that this breaks 8385 is quite high, maybe 100
> percent.

The cf8385-5.0.16.p0-26306 (gumstix) driver has:

Adapter->RegionCode = wlan_le16_to_cpu(hwspec->RegionCode) >> 8;

So it appears there's precedent for this even on 8385 parts.

Dan

> It could simply be the case that your firmware returns the value
> in a different order than mine. So we either need to test for the
> firmware version or for the hardware before getting the value via
> one or the other method.
>
> Maybe it's worthwhile to create some hw_is_8381() function and
> then do the things that need to be differently based on it's
> return value?
>
>
>
> The documentation for CMG_GET_HW_SPEC that I have says
>
> RegionCode UINT16 Set to 0
>
> so I see no indicator that we have two 8 bit values here. But
> we've seen bugs in docs before :-)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html