[PATCH] net: via-rhine: fix compiler warning

[PATCH] net: via-rhine: fix compiler warning

[Jan Moskyto Matejka]

Fixed different size cast warning:

	drivers/net/ethernet/via/via-rhine.c: In function ‘rhine_init_one_platform’:
	drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
	  revision = (u32)match->data;
		     ^

That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765
("net: via-rhine: add OF bus binding").

Signed-off-by: Jan Moskyto Matejka <mq@suse.cz>
---
 drivers/net/ethernet/via/via-rhine.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 4fa9201..76d18e0 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -288,7 +288,7 @@ MODULE_DEVICE_TABLE(pci, rhine_pci_tbl);
  * (for quirks etc.)
  */
 static struct of_device_id rhine_of_tbl[] = {
-	{ .compatible = "via,vt8500-rhine", .data = (void *)0x84 },
+	{ .compatible = "via,vt8500-rhine", .data = (u32 []) { 0x84 } },
 	{ }	/* terminate list */
 };
 MODULE_DEVICE_TABLE(of, rhine_of_tbl);
@@ -1129,7 +1129,7 @@ static int rhine_init_one_platform(struct platform_device *pdev)
 	if (!irq)
 		return -EINVAL;
 
-	revision = (u32)match->data;
+	revision = *((u32 *) match->data);
 	if (!revision)
 		return -EINVAL;
 
-- 
1.8.4.5

Re: [PATCH] net: via-rhine: fix compiler warning

[Alexey Charkov]

2014-04-29 21:36 GMT+04:00 Jan Moskyto Matejka <mq@suse.cz>:
> Fixed different size cast warning:
>
>         drivers/net/ethernet/via/via-rhine.c: In function ‘rhine_init_one_platform’:
>         drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>           revision = (u32)match->data;
>                      ^
>
> That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765
> ("net: via-rhine: add OF bus binding").
>
> Signed-off-by: Jan Moskyto Matejka <mq@suse.cz>

Jan, thanks a lot for catching this. Have to admit that I didn't
compile it on 64bit.

Acked-by: Alexey Charkov <alchark@gmail.com>

Looks like the same would apply to e.g. drivers/clk/samsung/clk.c and
maybe some others... Also, a number of drivers cast OF data from (void
*) to (int) or (unsigned int) - isn't this also problematic on 64bit?

Thanks a lot,
Alexey

Re: [PATCH] net: via-rhine: fix compiler warning

[Jan Moskyto Matejka]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

On Tue, Apr 29, 2014 at 10:09:01PM +0400, Alexey Charkov wrote:
> 2014-04-29 21:36 GMT+04:00 Jan Moskyto Matejka <mq@suse.cz>:
> > Fixed different size cast warning:
> >
> >         drivers/net/ethernet/via/via-rhine.c: In function ‘rhine_init_one_platform’:
> >         drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
> >           revision = (u32)match->data;
> >                      ^
> 
> Looks like the same would apply to e.g. drivers/clk/samsung/clk.c and
> maybe some others... Also, a number of drivers cast OF data from (void
> *) to (int) or (unsigned int) - isn't this also problematic on 64bit?

Some of them are, some not, sometimes nobody knows. If it were up to me,
I would personally put a single comment "this throws a compilation
warning because this and that" at every place where the warning is
thrown and is to be ignored.

So I do fix almost every warning of this kind I come across. Better to
have that fixed than to believe that the warning is harmless (as this
probably really was).

I'm just running a script that checks the tree for new build warnings
and then analysing the script's output ... and sometimes it's worth
fixing, sometimes not.

		Moskyto

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] net: via-rhine: fix compiler warning

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1794 bytes --]

From: Jan Moskyto Matejka
> Fixed different size cast warning:
> 
> 	drivers/net/ethernet/via/via-rhine.c: In function rhine_init_one_platform:
> 	drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different
> size [-Wpointer-to-int-cast]
> 	  revision = (u32)match->data;
> 		     ^
> 
> That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765
> ("net: via-rhine: add OF bus binding").
...
> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
> index 4fa9201..76d18e0 100644
> --- a/drivers/net/ethernet/via/via-rhine.c
> +++ b/drivers/net/ethernet/via/via-rhine.c
> @@ -288,7 +288,7 @@ MODULE_DEVICE_TABLE(pci, rhine_pci_tbl);
>   * (for quirks etc.)
>   */
>  static struct of_device_id rhine_of_tbl[] = {
> -	{ .compatible = "via,vt8500-rhine", .data = (void *)0x84 },
> +	{ .compatible = "via,vt8500-rhine", .data = (u32 []) { 0x84 } },
>  	{ }	/* terminate list */
>  };
>  MODULE_DEVICE_TABLE(of, rhine_of_tbl);
> @@ -1129,7 +1129,7 @@ static int rhine_init_one_platform(struct platform_device *pdev)
>  	if (!irq)
>  		return -EINVAL;
> 
> -	revision = (u32)match->data;
> +	revision = *((u32 *) match->data);
>  	if (!revision)
>  		return -EINVAL;

Both those casts look horrid.
I'm not entirely convinced that the first is valid C - It would have to be
something specific to C99 initialisers.
Casts like *(u32 *)foo are also likely to be bugs (esp. on BE systems)
so themselves start ringing alarm bells.

So why not just:
	revision = (unsigned long)match->data;
and add a comment that the 0x84 is the revision - #define ??

	David


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: via-rhine: fix compiler warning

[Alexey Charkov]

2014-04-30 12:49 GMT+04:00 David Laight <David.Laight@aculab.com>:
> From: Jan Moskyto Matejka
>> Fixed different size cast warning:
>>
>>       drivers/net/ethernet/via/via-rhine.c: In function rhine_init_one_platform:
>>       drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different
>> size [-Wpointer-to-int-cast]
>>         revision = (u32)match->data;
>>                    ^
>>
>> That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765
>> ("net: via-rhine: add OF bus binding").
> ...
>> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
>> index 4fa9201..76d18e0 100644
>> --- a/drivers/net/ethernet/via/via-rhine.c
>> +++ b/drivers/net/ethernet/via/via-rhine.c
>> @@ -288,7 +288,7 @@ MODULE_DEVICE_TABLE(pci, rhine_pci_tbl);
>>   * (for quirks etc.)
>>   */
>>  static struct of_device_id rhine_of_tbl[] = {
>> -     { .compatible = "via,vt8500-rhine", .data = (void *)0x84 },
>> +     { .compatible = "via,vt8500-rhine", .data = (u32 []) { 0x84 } },
>>       { }     /* terminate list */
>>  };
>>  MODULE_DEVICE_TABLE(of, rhine_of_tbl);
>> @@ -1129,7 +1129,7 @@ static int rhine_init_one_platform(struct platform_device *pdev)
>>       if (!irq)
>>               return -EINVAL;
>>
>> -     revision = (u32)match->data;
>> +     revision = *((u32 *) match->data);
>>       if (!revision)
>>               return -EINVAL;
>
> Both those casts look horrid.
> I'm not entirely convinced that the first is valid C - It would have to be
> something specific to C99 initialisers.
> Casts like *(u32 *)foo are also likely to be bugs (esp. on BE systems)
> so themselves start ringing alarm bells.
>
> So why not just:
>         revision = (unsigned long)match->data;
> and add a comment that the 0x84 is the revision - #define ??

There is no particular reason why it should be u32 now - this is a
leftover from the previous iteration of code where revision was a
separate property in DT (sized u32). It actually mirrors the
respective field in struct pci_dev, which is u8 - don't see any issue
defining it as unsigned long (and also changing the definition in
struct rhine_private).

The comment that it's the revision is right above the match table (cut
off in the patch) :)

Jan, would you prefer to adjust your patch, or shall I send another
one to change rp->revision and friends to unsigned long?

Thanks a lot,
Alexey

RE: [PATCH] net: via-rhine: fix compiler warning

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2750 bytes --]

From: Alexey Charkov [mailto:alchark@gmail.com]
 
> 2014-04-30 12:49 GMT+04:00 David Laight <David.Laight@aculab.com>:
> > From: Jan Moskyto Matejka
> >> Fixed different size cast warning:
> >>
> >>       drivers/net/ethernet/via/via-rhine.c: In function rhine_init_one_platform:
> >>       drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different
> >> size [-Wpointer-to-int-cast]
> >>         revision = (u32)match->data;
> >>                    ^
> >>
> >> That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765
> >> ("net: via-rhine: add OF bus binding").
> > ...
> >> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
> >> index 4fa9201..76d18e0 100644
> >> --- a/drivers/net/ethernet/via/via-rhine.c
> >> +++ b/drivers/net/ethernet/via/via-rhine.c
> >> @@ -288,7 +288,7 @@ MODULE_DEVICE_TABLE(pci, rhine_pci_tbl);
> >>   * (for quirks etc.)
> >>   */
> >>  static struct of_device_id rhine_of_tbl[] = {
> >> -     { .compatible = "via,vt8500-rhine", .data = (void *)0x84 },
> >> +     { .compatible = "via,vt8500-rhine", .data = (u32 []) { 0x84 } },
> >>       { }     /* terminate list */
> >>  };
> >>  MODULE_DEVICE_TABLE(of, rhine_of_tbl);
> >> @@ -1129,7 +1129,7 @@ static int rhine_init_one_platform(struct platform_device *pdev)
> >>       if (!irq)
> >>               return -EINVAL;
> >>
> >> -     revision = (u32)match->data;
> >> +     revision = *((u32 *) match->data);
> >>       if (!revision)
> >>               return -EINVAL;
> >
> > Both those casts look horrid.
> > I'm not entirely convinced that the first is valid C - It would have to be
> > something specific to C99 initialisers.
> > Casts like *(u32 *)foo are also likely to be bugs (esp. on BE systems)
> > so themselves start ringing alarm bells.
> >
> > So why not just:
> >         revision = (unsigned long)match->data;
> > and add a comment that the 0x84 is the revision - #define ??
> 
> There is no particular reason why it should be u32 now - this is a
> leftover from the previous iteration of code where revision was a
> separate property in DT (sized u32). It actually mirrors the
> respective field in struct pci_dev, which is u8 - don't see any issue
> defining it as unsigned long (and also changing the definition in
> struct rhine_private).

I'd guess that the field in 'struct rhine_private' only needs to
be large enough for the domain of the values that are actually
saved in it.
If these are versions (of something) then a fixed size type
(or just int) is probably more appropriate.

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: via-rhine: fix compiler warning

[Jan Moskyto Matejka]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

> > So why not just:
> >         revision = (unsigned long)match->data;
> > and add a comment that the 0x84 is the revision - #define ??
> 
> There is no particular reason why it should be u32 now - this is a
> leftover from the previous iteration of code where revision was a
> separate property in DT (sized u32). It actually mirrors the
> respective field in struct pci_dev, which is u8 - don't see any issue
> defining it as unsigned long (and also changing the definition in
> struct rhine_private).
> 
> The comment that it's the revision is right above the match table (cut
> off in the patch) :)
> 
> Jan, would you prefer to adjust your patch, or shall I send another
> one to change rp->revision and friends to unsigned long?

I prefer you to make another patch, you obviously know more about this
driver. Also thanks for suggesting (void*)->(unsigned long), I didn't
know that these two are defined to have the same size (in kernel code).
-- 
Jan Matějka <mq@suse.cz>
SUSE Labs

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] net: via-rhine: fix compiler warning

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1516 bytes --]

From: Jan Moskyto Matejka
> > > So why not just:
> > >         revision = (unsigned long)match->data;
> > > and add a comment that the 0x84 is the revision - #define ??
> >
> > There is no particular reason why it should be u32 now - this is a
> > leftover from the previous iteration of code where revision was a
> > separate property in DT (sized u32). It actually mirrors the
> > respective field in struct pci_dev, which is u8 - don't see any issue
> > defining it as unsigned long (and also changing the definition in
> > struct rhine_private).
> >
> > The comment that it's the revision is right above the match table (cut
> > off in the patch) :)
> >
> > Jan, would you prefer to adjust your patch, or shall I send another
> > one to change rp->revision and friends to unsigned long?
> 
> I prefer you to make another patch, you obviously know more about this
> driver. Also thanks for suggesting (void*)->(unsigned long), I didn't
> know that these two are defined to have the same size (in kernel code).

The kernel assumes that throughout - the double cast is common.
The C99 type is uintptr_t - but I don't think that is defined in kernel.
The only place I know where sizeof (long) != sizeof (void *) is 64bit
windows. So anyone trying to compile a linux driver to run in the
windows kernel might have issues (never mind the GPL ones).

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: via-rhine: fix compiler warning

[Jan Moskyto Matejka]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

> > Also thanks for suggesting (void*)->(unsigned long), I didn't
> > know that these two are defined to have the same size (in kernel code).
> 
> The kernel assumes that throughout - the double cast is common.
> The C99 type is uintptr_t - but I don't think that is defined in kernel.
> The only place I know where sizeof (long) != sizeof (void *) is 64bit
> windows. So anyone trying to compile a linux driver to run in the
> windows kernel might have issues (never mind the GPL ones).

My colleague here has just explained me the same. :)

Being a linux-kernel newbie, I'm learning every day. Kernel programming
is more different from userspace than I ever thought. Thank you for your
patience.
-- 
Jan Matějka <mq@suse.cz>
SUSE Labs

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] net: via-rhine: Fix compiler warning re: pointer casting on 64bit

[Alexey Charkov]

Fixed different size cast warning:

        drivers/net/ethernet/via/via-rhine.c: In function ‘rhine_init_one_platform’:
        drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
          revision = (u32)match->data;
                     ^

That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765
("net: via-rhine: add OF bus binding").

This patch removes the cast altogether, and instead stores an actual
pointer to u8 in match->data. All instances of 'revision' are also
unified to u8 instead of an assortment of different integer types,
in line with the definition of 'revision' in struct pci_dev.

Tested in platform configuration on a VIA WM8950 APC Rock board.

Reported-by: Jan Moskyto Matejka <mq@suse.cz>
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/net/ethernet/via/via-rhine.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 4fa9201..80cdc91 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -287,8 +287,9 @@ MODULE_DEVICE_TABLE(pci, rhine_pci_tbl);
  * The .data field is currently only used to store chip revision
  * (for quirks etc.)
  */
+static u8 vt8500_revision = 0x84;
 static struct of_device_id rhine_of_tbl[] = {
-	{ .compatible = "via,vt8500-rhine", .data = (void *)0x84 },
+	{ .compatible = "via,vt8500-rhine", .data = &vt8500_revision },
 	{ }	/* terminate list */
 };
 MODULE_DEVICE_TABLE(of, rhine_of_tbl);
@@ -459,7 +460,7 @@ struct rhine_private {
 	unsigned char *tx_bufs;
 	dma_addr_t tx_bufs_dma;
 
-	int revision;
+	u8 revision;
 	int irq;
 	long pioaddr;
 	struct net_device *dev;
@@ -882,7 +883,7 @@ static const struct net_device_ops rhine_netdev_ops = {
 #endif
 };
 
-static int rhine_init_one_common(struct device *hwdev, int revision,
+static int rhine_init_one_common(struct device *hwdev, u8 revision,
 				 long pioaddr, void __iomem *ioaddr, int irq)
 {
 	struct net_device *dev;
@@ -1111,7 +1112,7 @@ err_out:
 static int rhine_init_one_platform(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
-	u32 revision;
+	const u8 *revision;
 	int irq;
 	struct resource *res;
 	void __iomem *ioaddr;
@@ -1129,11 +1130,11 @@ static int rhine_init_one_platform(struct platform_device *pdev)
 	if (!irq)
 		return -EINVAL;
 
-	revision = (u32)match->data;
+	revision = match->data;
 	if (!revision)
 		return -EINVAL;
 
-	return rhine_init_one_common(&pdev->dev, revision,
+	return rhine_init_one_common(&pdev->dev, *revision,
 				     (long)ioaddr, ioaddr, irq);
 }
 
-- 
1.9.1

[PATCH] net: via-rhine: Drop revision property, use quirks instead

[Alexey Charkov]

This adds two new flags to quirks and thus removes the need to carry
revision in rhine_private. As a result, the init logic is simplified
a bit.

This also fixes a compiler warning in OF code on 64bit due to pointer
casting:

        drivers/net/ethernet/via/via-rhine.c: In function ‘rhine_init_one_platform’:
        drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
          revision = (u32)match->data;
                     ^

That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765
("net: via-rhine: add OF bus binding").

Tested in platform configuration on a VIA WM8950 APC Rock board.

Reported-by: Jan Moskyto Matejka <mq@suse.cz>
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---

This supercedes a previously submitted patch titled:

net: via-rhine: Fix compiler warning re: pointer casting on 64bit

I thought it would be a good idea to also streamline the common code somewhat
while at it, and storing quirks directly in the match table sounded more
appropriate than storing an abstract revision that doesn't come from the
hardware.

This also further reduces the rhine_private structure and makes runtime checks
more uniform throughout the driver.

 drivers/net/ethernet/via/via-rhine.c | 77 ++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 4fa9201..13cfcce 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -264,6 +264,8 @@ enum rhine_quirks {
 	rq6patterns	= 0x0040,	/* 6 instead of 4 patterns for WOL */
 	rqStatusWBRace	= 0x0080,	/* Tx Status Writeback Error possible */
 	rqRhineI	= 0x0100,	/* See comment below */
+	rqIntPHY	= 0x0200,	/* Integrated PHY */
+	rqMgmt		= 0x0400,	/* Management adapter */
 };
 /*
  * rqRhineI: VT86C100A (aka Rhine-I) uses different bits to enable
@@ -284,11 +286,11 @@ static DEFINE_PCI_DEVICE_TABLE(rhine_pci_tbl) = {
 MODULE_DEVICE_TABLE(pci, rhine_pci_tbl);
 
 /* OpenFirmware identifiers for platform-bus devices
- * The .data field is currently only used to store chip revision
- * (for quirks etc.)
+ * The .data field is currently only used to store quirks
  */
+static u32 vt8500_quirks = rqWOL | rqForceReset | rq6patterns;
 static struct of_device_id rhine_of_tbl[] = {
-	{ .compatible = "via,vt8500-rhine", .data = (void *)0x84 },
+	{ .compatible = "via,vt8500-rhine", .data = &vt8500_quirks },
 	{ }	/* terminate list */
 };
 MODULE_DEVICE_TABLE(of, rhine_of_tbl);
@@ -459,7 +461,6 @@ struct rhine_private {
 	unsigned char *tx_bufs;
 	dma_addr_t tx_bufs_dma;
 
-	int revision;
 	int irq;
 	long pioaddr;
 	struct net_device *dev;
@@ -882,7 +883,7 @@ static const struct net_device_ops rhine_netdev_ops = {
 #endif
 };
 
-static int rhine_init_one_common(struct device *hwdev, int revision,
+static int rhine_init_one_common(struct device *hwdev, u32 quirks,
 				 long pioaddr, void __iomem *ioaddr, int irq)
 {
 	struct net_device *dev;
@@ -906,31 +907,13 @@ static int rhine_init_one_common(struct device *hwdev, int revision,
 
 	rp = netdev_priv(dev);
 	rp->dev = dev;
-	rp->revision = revision;
+	rp->quirks = quirks;
 	rp->pioaddr = pioaddr;
 	rp->base = ioaddr;
 	rp->irq = irq;
 	rp->msg_enable = netif_msg_init(debug, RHINE_MSG_DEFAULT);
 
-	phy_id = 0;
-	name = "Rhine";
-	if (revision < VTunknown0) {
-		rp->quirks = rqRhineI;
-	} else if (revision >= VT6102) {
-		rp->quirks = rqWOL | rqForceReset;
-		if (revision < VT6105) {
-			name = "Rhine II";
-			rp->quirks |= rqStatusWBRace;	/* Rhine-II exclusive */
-		} else {
-			phy_id = 1;	/* Integrated PHY, phy_id fixed to 1 */
-			if (revision >= VT6105_B0)
-				rp->quirks |= rq6patterns;
-			if (revision < VT6105M)
-				name = "Rhine III";
-			else
-				name = "Rhine III (Management Adapter)";
-		}
-	}
+	phy_id = rp->quirks & rqIntPHY ? 1 : 0;
 
 	u64_stats_init(&rp->tx_stats.syncp);
 	u64_stats_init(&rp->rx_stats.syncp);
@@ -975,7 +958,7 @@ static int rhine_init_one_common(struct device *hwdev, int revision,
 	if (rp->quirks & rqRhineI)
 		dev->features |= NETIF_F_SG|NETIF_F_HW_CSUM;
 
-	if (rp->revision >= VT6105M)
+	if (rp->quirks & rqMgmt)
 		dev->features |= NETIF_F_HW_VLAN_CTAG_TX |
 				 NETIF_F_HW_VLAN_CTAG_RX |
 				 NETIF_F_HW_VLAN_CTAG_FILTER;
@@ -985,6 +968,15 @@ static int rhine_init_one_common(struct device *hwdev, int revision,
 	if (rc)
 		goto err_out_free_netdev;
 
+	if (rp->quirks & rqRhineI)
+		name = "Rhine";
+	else if (rp->quirks & rqStatusWBRace)
+		name = "Rhine II";
+	else if (rp->quirks & rqMgmt)
+		name = "Rhine III (Management Adapter)";
+	else
+		name = "Rhine III";
+
 	netdev_info(dev, "VIA %s at 0x%lx, %pM, IRQ %d\n",
 		    name, (long)ioaddr, dev->dev_addr, rp->irq);
 
@@ -1031,7 +1023,7 @@ static int rhine_init_one_pci(struct pci_dev *pdev,
 	long pioaddr, memaddr;
 	void __iomem *ioaddr;
 	int io_size = pdev->revision < VTunknown0 ? 128 : 256;
-	u32 quirks = pdev->revision < VTunknown0 ? rqRhineI : 0;
+	u32 quirks;
 #ifdef USE_MMIO
 	int bar = 1;
 #else
@@ -1047,6 +1039,21 @@ static int rhine_init_one_pci(struct pci_dev *pdev,
 	if (rc)
 		goto err_out;
 
+	if (pdev->revision < VTunknown0) {
+		quirks = rqRhineI;
+	} else if (pdev->revision >= VT6102) {
+		quirks = rqWOL | rqForceReset;
+		if (pdev->revision < VT6105) {
+			quirks |= rqStatusWBRace;
+		} else {
+			quirks |= rqIntPHY;
+			if (pdev->revision >= VT6105_B0)
+				quirks |= rq6patterns;
+			if (pdev->revision >= VT6105M)
+				quirks |= rqMgmt;
+		}
+	}
+
 	/* sanity check */
 	if ((pci_resource_len(pdev, 0) < io_size) ||
 	    (pci_resource_len(pdev, 1) < io_size)) {
@@ -1093,7 +1100,7 @@ static int rhine_init_one_pci(struct pci_dev *pdev,
 	}
 #endif /* USE_MMIO */
 
-	rc = rhine_init_one_common(&pdev->dev, pdev->revision,
+	rc = rhine_init_one_common(&pdev->dev, quirks,
 				   pioaddr, ioaddr, pdev->irq);
 	if (!rc)
 		return 0;
@@ -1111,7 +1118,7 @@ err_out:
 static int rhine_init_one_platform(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
-	u32 revision;
+	const u32 *quirks;
 	int irq;
 	struct resource *res;
 	void __iomem *ioaddr;
@@ -1129,11 +1136,11 @@ static int rhine_init_one_platform(struct platform_device *pdev)
 	if (!irq)
 		return -EINVAL;
 
-	revision = (u32)match->data;
-	if (!revision)
+	quirks = match->data;
+	if (!quirks)
 		return -EINVAL;
 
-	return rhine_init_one_common(&pdev->dev, revision,
+	return rhine_init_one_common(&pdev->dev, *quirks,
 				     (long)ioaddr, ioaddr, irq);
 }
 
@@ -1523,7 +1530,7 @@ static void init_registers(struct net_device *dev)
 
 	rhine_set_rx_mode(dev);
 
-	if (rp->revision >= VT6105M)
+	if (rp->quirks & rqMgmt)
 		rhine_init_cam_filter(dev);
 
 	napi_enable(&rp->napi);
@@ -2160,7 +2167,7 @@ static void rhine_set_rx_mode(struct net_device *dev)
 		/* Too many to match, or accept all multicasts. */
 		iowrite32(0xffffffff, ioaddr + MulticastFilter0);
 		iowrite32(0xffffffff, ioaddr + MulticastFilter1);
-	} else if (rp->revision >= VT6105M) {
+	} else if (rp->quirks & rqMgmt) {
 		int i = 0;
 		u32 mCAMmask = 0;	/* 32 mCAMs (6105M and better) */
 		netdev_for_each_mc_addr(ha, dev) {
@@ -2182,7 +2189,7 @@ static void rhine_set_rx_mode(struct net_device *dev)
 		iowrite32(mc_filter[1], ioaddr + MulticastFilter1);
 	}
 	/* enable/disable VLAN receive filtering */
-	if (rp->revision >= VT6105M) {
+	if (rp->quirks & rqMgmt) {
 		if (dev->flags & IFF_PROMISC)
 			BYTE_REG_BITS_OFF(BCR1_VIDFR, ioaddr + PCIBusConfig1);
 		else
-- 
1.9.1

RE: [PATCH] net: via-rhine: Fix compiler warning re: pointer casting on 64bit

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 893 bytes --]

From: Alexey Charkov [mailto:alchark@gmail.com]
...
> This patch removes the cast altogether, and instead stores an actual
> pointer to u8 in match->data. All instances of 'revision' are also
> unified to u8 instead of an assortment of different integer types,
> in line with the definition of 'revision' in struct pci_dev.
...
> +static u8 vt8500_revision = 0x84;
>  static struct of_device_id rhine_of_tbl[] = {
> -	{ .compatible = "via,vt8500-rhine", .data = (void *)0x84 },
> +	{ .compatible = "via,vt8500-rhine", .data = &vt8500_revision },
>  	{ }	/* terminate list */

Actually the above looks strange.
Why does the vt8500 have a revision number of 0x84 ?
Surely it should be 0x85, or even 0x8500 (or decimal 85000).

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] net: via-rhine: Fix compiler warning re: pointer casting on 64bit

[Alexey Charkov]

2014-05-01 13:30 GMT+04:00 David Laight <David.Laight@aculab.com>:
> From: Alexey Charkov [mailto:alchark@gmail.com]
> ...
>> This patch removes the cast altogether, and instead stores an actual
>> pointer to u8 in match->data. All instances of 'revision' are also
>> unified to u8 instead of an assortment of different integer types,
>> in line with the definition of 'revision' in struct pci_dev.
> ...
>> +static u8 vt8500_revision = 0x84;
>>  static struct of_device_id rhine_of_tbl[] = {
>> -     { .compatible = "via,vt8500-rhine", .data = (void *)0x84 },
>> +     { .compatible = "via,vt8500-rhine", .data = &vt8500_revision },
>>       { }     /* terminate list */
>
> Actually the above looks strange.
> Why does the vt8500 have a revision number of 0x84 ?
> Surely it should be 0x85, or even 0x8500 (or decimal 85000).

David, vt8500 is the SoC version (the first one I know where it
appeared, might have been used earlier though), not the version of the
Rhine core itself (no clue what that is).

Here's the source for 0x84 (see line 1031):

https://github.com/wondermedia/wm8850/blob/master/ANDROID_3.0.8/arch/arm/common/pci_wmt.c

Vendor kernel uses an emulated (shadow-only) PCI bus to bind some
obscure PCI driver (not upstream) to the integrated Rhine core, and
that emulated PCI bus has Rhine listed with revision 0x84 on it.

Anyway, I've now submitted a new patch instead of this one where I got
rid of the 0x84 altogether - see https://lkml.org/lkml/2014/4/30/500

Best,
Alexey

Re: [PATCH] net: via-rhine: Drop revision property, use quirks instead

[David Miller]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: Text/Plain; charset=iso-8859-7, Size: 1103 bytes --]

From: Alexey Charkov <alchark@gmail.com>
Date: Wed, 30 Apr 2014 22:21:09 +0400

> This adds two new flags to quirks and thus removes the need to carry
> revision in rhine_private. As a result, the init logic is simplified
> a bit.
> 
> This also fixes a compiler warning in OF code on 64bit due to pointer
> casting:
> 
>         drivers/net/ethernet/via/via-rhine.c: In function ¡rhine_init_one_platform¢:
>         drivers/net/ethernet/via/via-rhine.c:1132:13: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>           revision = (u32)match->data;
>                      ^
> 
> That code was added in commit 2d283862dc62daead9db0dc89cd0d0351e91f765
> ("net: via-rhine: add OF bus binding").
> 
> Tested in platform configuration on a VIA WM8950 APC Rock board.
> 
> Reported-by: Jan Moskyto Matejka <mq@suse.cz>
> Signed-off-by: Alexey Charkov <alchark@gmail.com>

This looks a lot better, applied, thanks.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH] net: via-rhine: Convert #ifdef USE_MMIO to a runtime flag

[Alexey Charkov]

This introduces another flag in 'quirks' to replace the preprocessor
define (USE_MMIO) used to indicate whether the device needs a
separate enable routine to operate in MMIO mode.

All of the currently known platform Rhine cores operate in MMIO
mode by default, and on PCI it is preferred over PIO for performance
reasons. However, a comment in code suggests that some (?) early
Rhine cores only work in PIO mode, so they should not be switched
to MMIO.

Enabling MMIO on PCI is still triggered by the same Kconfig option
to avoid breaking user configs needlessly, but this can be changed
going forward towards automatic runtime detection in case a list of
PIO-only Rhine revisions can be compiled.

This also fixes a couple of compiler warnings detected by Fengguang
Wu's test bot (!USE_MMIO case):

   drivers/net/ethernet/via/via-rhine.c: In function 'rhine_init_one_pci':
   drivers/net/ethernet/via/via-rhine.c:1108:1: warning: label 'err_out_unmap' defined but not used [-Wunused-label]
    err_out_unmap:
    ^
   drivers/net/ethernet/via/via-rhine.c:1022:6: warning: unused variable 'i' [-Wunused-variable]
     int i, rc;
         ^
   drivers/net/ethernet/via/via-rhine.c:916:22: warning: 'quirks' may be used uninitialized in this function [-Wmaybe-uninitialized]
     phy_id = rp->quirks & rqIntPHY ? 1 : 0;
                         ^
   drivers/net/ethernet/via/via-rhine.c:1026:6: note: 'quirks' was declared here
     u32 quirks;
         ^

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/net/ethernet/via/via-rhine.c | 102 +++++++++++++++++++----------------
 1 file changed, 55 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index 13cfcce..981be015 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -120,13 +120,6 @@ static const int multicast_filter_limit = 32;
 static const char version[] =
 	"v1.10-LK" DRV_VERSION " " DRV_RELDATE " Written by Donald Becker";
 
-/* This driver was written to use PCI memory space. Some early versions
-   of the Rhine may only work correctly with I/O space accesses. */
-#ifdef CONFIG_VIA_RHINE_MMIO
-#define USE_MMIO
-#else
-#endif
-
 MODULE_AUTHOR("Donald Becker <becker@scyld.com>");
 MODULE_DESCRIPTION("VIA Rhine PCI Fast Ethernet driver");
 MODULE_LICENSE("GPL");
@@ -266,6 +259,10 @@ enum rhine_quirks {
 	rqRhineI	= 0x0100,	/* See comment below */
 	rqIntPHY	= 0x0200,	/* Integrated PHY */
 	rqMgmt		= 0x0400,	/* Management adapter */
+	rqNeedEnMMIO	= 0x0800,	/* Whether the core needs to be
+					 * switched from PIO mode to MMIO
+					 * (only applies to PCI)
+					 */
 };
 /*
  * rqRhineI: VT86C100A (aka Rhine-I) uses different bits to enable
@@ -353,13 +350,11 @@ enum bcr1_bits {
 	BCR1_MED1=0x80,		/* for VT6102 */
 };
 
-#ifdef USE_MMIO
 /* Registers we check that mmio and reg are the same. */
 static const int mmio_verify_registers[] = {
 	RxConfig, TxConfig, IntrEnable, ConfigA, ConfigB, ConfigC, ConfigD,
 	0
 };
-#endif
 
 /* Bits in the interrupt status/mask registers. */
 enum intr_status_bits {
@@ -664,20 +659,46 @@ static void rhine_chip_reset(struct net_device *dev)
 		   "failed" : "succeeded");
 }
 
-#ifdef USE_MMIO
 static void enable_mmio(long pioaddr, u32 quirks)
 {
 	int n;
-	if (quirks & rqRhineI) {
-		/* More recent docs say that this bit is reserved ... */
-		n = inb(pioaddr + ConfigA) | 0x20;
-		outb(n, pioaddr + ConfigA);
-	} else {
-		n = inb(pioaddr + ConfigD) | 0x80;
-		outb(n, pioaddr + ConfigD);
+
+	if (quirks & rqNeedEnMMIO) {
+		if (quirks & rqRhineI) {
+			/* More recent docs say that this bit is reserved */
+			n = inb(pioaddr + ConfigA) | 0x20;
+			outb(n, pioaddr + ConfigA);
+		} else {
+			n = inb(pioaddr + ConfigD) | 0x80;
+			outb(n, pioaddr + ConfigD);
+		}
 	}
 }
-#endif
+
+static inline int verify_mmio(struct device *hwdev,
+			      long pioaddr,
+			      void __iomem *ioaddr,
+			      u32 quirks)
+{
+	if (quirks & rqNeedEnMMIO) {
+		int i = 0;
+
+		/* Check that selected MMIO registers match the PIO ones */
+		while (mmio_verify_registers[i]) {
+			int reg = mmio_verify_registers[i++];
+			unsigned char a = inb(pioaddr+reg);
+			unsigned char b = readb(ioaddr+reg);
+
+			if (a != b) {
+				dev_err(hwdev,
+					"MMIO do not match PIO [%02x] (%02x != %02x)\n",
+					reg, a, b);
+				return -EIO;
+			}
+		}
+	}
+	return 0;
+}
 
 /*
  * Loads bytes 0x00-0x05, 0x6E-0x6F, 0x78-0x7B from EEPROM
@@ -697,14 +718,12 @@ static void rhine_reload_eeprom(long pioaddr, struct net_device *dev)
 	if (i > 512)
 		pr_info("%4d cycles used @ %s:%d\n", i, __func__, __LINE__);
 
-#ifdef USE_MMIO
 	/*
 	 * Reloading from EEPROM overwrites ConfigA-D, so we must re-enable
 	 * MMIO. If reloading EEPROM was done first this could be avoided, but
 	 * it is not known if that still works with the "win98-reboot" problem.
 	 */
 	enable_mmio(pioaddr, rp->quirks);
-#endif
 
 	/* Turn off EEPROM-controlled wake-up (magic packet) */
 	if (rp->quirks & rqWOL)
@@ -1019,15 +1038,20 @@ static int rhine_init_one_pci(struct pci_dev *pdev,
 			      const struct pci_device_id *ent)
 {
 	struct device *hwdev = &pdev->dev;
-	int i, rc;
+	int rc;
 	long pioaddr, memaddr;
 	void __iomem *ioaddr;
 	int io_size = pdev->revision < VTunknown0 ? 128 : 256;
-	u32 quirks;
-#ifdef USE_MMIO
-	int bar = 1;
+
+/* This driver was written to use PCI memory space. Some early versions
+ * of the Rhine may only work correctly with I/O space accesses.
+ * TODO: determine for which revisions this is true and assign the flag
+ *	 in code as opposed to this Kconfig option (???)
+ */
+#ifdef CONFIG_VIA_RHINE_MMIO
+	u32 quirks = rqNeedEnMMIO;
 #else
-	int bar = 0;
+	u32 quirks = 0;
 #endif
 
 /* when built into the kernel, we only print version if device is found */
@@ -1040,9 +1064,9 @@ static int rhine_init_one_pci(struct pci_dev *pdev,
 		goto err_out;
 
 	if (pdev->revision < VTunknown0) {
-		quirks = rqRhineI;
+		quirks |= rqRhineI;
 	} else if (pdev->revision >= VT6102) {
-		quirks = rqWOL | rqForceReset;
+		quirks |= rqWOL | rqForceReset;
 		if (pdev->revision < VT6105) {
 			quirks |= rqStatusWBRace;
 		} else {
@@ -1071,7 +1095,7 @@ static int rhine_init_one_pci(struct pci_dev *pdev,
 	if (rc)
 		goto err_out_pci_disable;
 
-	ioaddr = pci_iomap(pdev, bar, io_size);
+	ioaddr = pci_iomap(pdev, (quirks & rqNeedEnMMIO ? 1 : 0), io_size);
 	if (!ioaddr) {
 		rc = -EIO;
 		dev_err(hwdev,
@@ -1080,25 +1104,11 @@ static int rhine_init_one_pci(struct pci_dev *pdev,
 		goto err_out_free_res;
 	}
 
-#ifdef USE_MMIO
 	enable_mmio(pioaddr, quirks);
 
-	/* Check that selected MMIO registers match the PIO ones */
-	i = 0;
-	while (mmio_verify_registers[i]) {
-		int reg = mmio_verify_registers[i++];
-		unsigned char a = inb(pioaddr+reg);
-		unsigned char b = readb(ioaddr+reg);
-
-		if (a != b) {
-			rc = -EIO;
-			dev_err(hwdev,
-				"MMIO do not match PIO [%02x] (%02x != %02x)\n",
-				reg, a, b);
-			goto err_out_unmap;
-		}
-	}
-#endif /* USE_MMIO */
+	rc = verify_mmio(hwdev, pioaddr, ioaddr, quirks);
+	if (rc)
+		goto err_out_unmap;
 
 	rc = rhine_init_one_common(&pdev->dev, quirks,
 				   pioaddr, ioaddr, pdev->irq);
@@ -2458,9 +2468,7 @@ static int rhine_resume(struct device *device)
 	if (!netif_running(dev))
 		return 0;
 
-#ifdef USE_MMIO
 	enable_mmio(rp->pioaddr, rp->quirks);
-#endif
 	rhine_power_init(dev);
 	free_tbufs(dev);
 	free_rbufs(dev);
-- 
1.9.1

Re: [PATCH] net: via-rhine: Convert #ifdef USE_MMIO to a runtime flag

[David Miller]

From: Alexey Charkov <alchark@gmail.com>
Date: Sat,  3 May 2014 16:40:53 +0400

> This introduces another flag in 'quirks' to replace the preprocessor
> define (USE_MMIO) used to indicate whether the device needs a
> separate enable routine to operate in MMIO mode.
> 
> All of the currently known platform Rhine cores operate in MMIO
> mode by default, and on PCI it is preferred over PIO for performance
> reasons. However, a comment in code suggests that some (?) early
> Rhine cores only work in PIO mode, so they should not be switched
> to MMIO.
> 
> Enabling MMIO on PCI is still triggered by the same Kconfig option
> to avoid breaking user configs needlessly, but this can be changed
> going forward towards automatic runtime detection in case a list of
> PIO-only Rhine revisions can be compiled.
> 
> This also fixes a couple of compiler warnings detected by Fengguang
> Wu's test bot (!USE_MMIO case):
 ...
> Signed-off-by: Alexey Charkov <alchark@gmail.com>

Applied, but in the future please make it clear what tree you are
targetting by saying "[PATCH net]" or "[PATCH net-next]" in your
subject line.

Thanks.