[U-Boot] [PATCH 0/3] smsc95xx: Fix MAC address programming and some minor issues

[U-Boot] [PATCH 0/3] smsc95xx: Fix MAC address programming and some minor issues

[Wolfgang Grandegger]

Wolfgang Grandegger (3):
  smsc95xx: Fix MAC address programming
  smsc95xx: in smsc95xx_set_multicast write to reg
  smsc95xx: remove an unecessary debug messages

 drivers/usb/eth/smsc95xx.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

-- 
1.7.4.1

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Wolfgang Grandegger]

Commit 79ad54400932d6484178a372fb3b659e3437473b broke the MAC
address programming. Fix this by using the method from Linux'
smsc95xx_set_mac_address().

Signed-off-by: Wolfgang Grandegger <wg@denx.de>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
---
 drivers/usb/eth/smsc95xx.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
index 7ee4f87..eb529f1 100644
--- a/drivers/usb/eth/smsc95xx.c
+++ b/drivers/usb/eth/smsc95xx.c
@@ -372,13 +372,14 @@ static int smsc95xx_init_mac_address(struct eth_device *eth,
 static int smsc95xx_write_hwaddr(struct eth_device *eth)
 {
 	struct ueth_data *dev = (struct ueth_data *)eth->priv;
-	u32 addr_lo, addr_hi;
+	u32 addr_lo = eth->enetaddr[0] | eth->enetaddr[1] << 8 |
+		eth->enetaddr[2] << 16 | eth->enetaddr[3] << 24;
+	u32 addr_hi = eth->enetaddr[4] | eth->enetaddr[5] << 8;
 	int ret;
 
 	/* set hardware address */
 	debug("** %s()\n", __func__);
-	addr_lo = cpu_to_le32(*eth->enetaddr);
-	addr_hi = cpu_to_le16(*((u16 *)(eth->enetaddr + 4)));
+
 	ret = smsc95xx_write_reg(dev, ADDRL, addr_lo);
 	if (ret < 0) {
 		debug("Failed to write ADDRL: %d\n", ret);
-- 
1.7.4.1

[U-Boot] [PATCH 2/3] smsc95xx: in smsc95xx_set_multicast write to reg

[Wolfgang Grandegger]

The write to the mac_cr register was missing. This usually not
cause an issue before, since the next function writing the
register's shadow copy into the register would do it as a side
effect.

Signed-off-by: Wolfgang Grandegger <wg@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
 drivers/usb/eth/smsc95xx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
index eb529f1..16e24bd 100644
--- a/drivers/usb/eth/smsc95xx.c
+++ b/drivers/usb/eth/smsc95xx.c
@@ -428,6 +428,8 @@ static void smsc95xx_set_multicast(struct ueth_data *dev)
 {
 	/* No multicast in u-boot */
 	dev->mac_cr &= ~(MAC_CR_PRMS_ | MAC_CR_MCPAS_ | MAC_CR_HPFILT_);
+
+	smsc95xx_write_reg(dev, MAC_CR, dev->mac_cr);
 }
 
 /* starts the TX path */
-- 
1.7.4.1

[U-Boot] [PATCH 3/3] smsc95xx: remove an unecessary debug messages

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
 drivers/usb/eth/smsc95xx.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
index 16e24bd..40f7f5b 100644
--- a/drivers/usb/eth/smsc95xx.c
+++ b/drivers/usb/eth/smsc95xx.c
@@ -380,15 +380,14 @@ static int smsc95xx_write_hwaddr(struct eth_device *eth)
 	/* set hardware address */
 	debug("** %s()\n", __func__);
 
-	ret = smsc95xx_write_reg(dev, ADDRL, addr_lo);
-	if (ret < 0) {
-		debug("Failed to write ADDRL: %d\n", ret);
+	ret = smsc95xx_write_reg(dev, ADDRL, adddr_lo);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = smsc95xx_write_reg(dev, ADDRH, addr_hi);
 	if (ret < 0)
 		return ret;
+
 	debug("MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
 		eth->enetaddr[0], eth->enetaddr[1],
 		eth->enetaddr[2], eth->enetaddr[3],
-- 
1.7.4.1

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Marek Vasut]

> Commit 79ad54400932d6484178a372fb3b659e3437473b broke the MAC
> address programming. Fix this by using the method from Linux'
> smsc95xx_set_mac_address().
> 
> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  drivers/usb/eth/smsc95xx.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
> index 7ee4f87..eb529f1 100644
> --- a/drivers/usb/eth/smsc95xx.c
> +++ b/drivers/usb/eth/smsc95xx.c
> @@ -372,13 +372,14 @@ static int smsc95xx_init_mac_address(struct
> eth_device *eth, static int smsc95xx_write_hwaddr(struct eth_device *eth)
>  {
>  	struct ueth_data *dev = (struct ueth_data *)eth->priv;
> -	u32 addr_lo, addr_hi;
> +	u32 addr_lo = eth->enetaddr[0] | eth->enetaddr[1] << 8 |
> +		eth->enetaddr[2] << 16 | eth->enetaddr[3] << 24;
> +	u32 addr_hi = eth->enetaddr[4] | eth->enetaddr[5] << 8;
>  	int ret;
> 
>  	/* set hardware address */
>  	debug("** %s()\n", __func__);
> -	addr_lo = cpu_to_le32(*eth->enetaddr);
> -	addr_hi = cpu_to_le16(*((u16 *)(eth->enetaddr + 4)));
> +
>  	ret = smsc95xx_write_reg(dev, ADDRL, addr_lo);
>  	if (ret < 0) {
>  		debug("Failed to write ADDRL: %d\n", ret);

Hey,

didn't Mike Frysinger send similar patch yesterday? Also, do you have the 
hardware? If so, that's good, we know you can provide tested fix.

Mike, are you good with using this fix instead or can you two negotiate?

M

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Wolfgang Grandegger]

On 11/11/2011 12:04 PM, Marek Vasut wrote:
>> Commit 79ad54400932d6484178a372fb3b659e3437473b broke the MAC
>> address programming. Fix this by using the method from Linux'
>> smsc95xx_set_mac_address().
>>
>> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
>> Cc: Marek Vasut <marek.vasut@gmail.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>  drivers/usb/eth/smsc95xx.c |    7 ++++---
>>  1 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
>> index 7ee4f87..eb529f1 100644
>> --- a/drivers/usb/eth/smsc95xx.c
>> +++ b/drivers/usb/eth/smsc95xx.c
>> @@ -372,13 +372,14 @@ static int smsc95xx_init_mac_address(struct
>> eth_device *eth, static int smsc95xx_write_hwaddr(struct eth_device *eth)
>>  {
>>  	struct ueth_data *dev = (struct ueth_data *)eth->priv;
>> -	u32 addr_lo, addr_hi;
>> +	u32 addr_lo = eth->enetaddr[0] | eth->enetaddr[1] << 8 |
>> +		eth->enetaddr[2] << 16 | eth->enetaddr[3] << 24;
>> +	u32 addr_hi = eth->enetaddr[4] | eth->enetaddr[5] << 8;
>>  	int ret;
>>
>>  	/* set hardware address */
>>  	debug("** %s()\n", __func__);
>> -	addr_lo = cpu_to_le32(*eth->enetaddr);
>> -	addr_hi = cpu_to_le16(*((u16 *)(eth->enetaddr + 4)));
>> +
>>  	ret = smsc95xx_write_reg(dev, ADDRL, addr_lo);
>>  	if (ret < 0) {
>>  		debug("Failed to write ADDRL: %d\n", ret);
> 
> Hey,
> 
> didn't Mike Frysinger send similar patch yesterday? Also, do you have the 

Ah, I obviously missed that.

> hardware? If so, that's good, we know you can provide tested fix.

Well, yes, I know somebody who has smsc95xx hardware ;-).

> Mike, are you good with using this fix instead or can you two negotiate?

Let's await Mike's answer (now on CC as well). I need to re-send the
series anyway due to a stupid typo.

Wolfgang

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Mike Frysinger]

On Friday 11 November 2011 07:33:00 Wolfgang Grandegger wrote:
> On 11/11/2011 12:04 PM, Marek Vasut wrote:
> >> Commit 79ad54400932d6484178a372fb3b659e3437473b broke the MAC
> >> address programming. Fix this by using the method from Linux'
> >> smsc95xx_set_mac_address().
> >> 
> >> --- a/drivers/usb/eth/smsc95xx.c
> >> +++ b/drivers/usb/eth/smsc95xx.c
> >> 
> >> -	u32 addr_lo, addr_hi;
> >> +	u32 addr_lo = eth->enetaddr[0] | eth->enetaddr[1] << 8 |
> >> +		eth->enetaddr[2] << 16 | eth->enetaddr[3] << 24;
> >> +	u32 addr_hi = eth->enetaddr[4] | eth->enetaddr[5] << 8;
> >> 
> >>  	int ret;
> >>  	
> >>  	/* set hardware address */
> >>  	debug("** %s()\n", __func__);
> >> 
> >> -	addr_lo = cpu_to_le32(*eth->enetaddr);
> >> -	addr_hi = cpu_to_le16(*((u16 *)(eth->enetaddr + 4)));
> >> +
> >> 
> > 
> > didn't Mike Frysinger send similar patch yesterday? Also, do you have the
> 
> Ah, I obviously missed that.
> 
> > hardware? If so, that's good, we know you can provide tested fix.
> 
> Well, yes, I know somebody who has smsc95xx hardware ;-).
> 
> > Mike, are you good with using this fix instead or can you two negotiate?
> 
> Let's await Mike's answer (now on CC as well). I need to re-send the
> series anyway due to a stupid typo.

mine might take a little longer due to wrangling with wolfgang.  i'd suggest 
we go with your patch (although i have feedback on it).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111111/8f9764e6/attachment.pgp 

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Mike Frysinger]

On Friday 11 November 2011 05:59:56 Wolfgang Grandegger wrote:
> Commit 79ad54400932d6484178a372fb3b659e3437473b broke the MAC
> address programming. Fix this by using the method from Linux'
> smsc95xx_set_mac_address().
> 
> --- a/drivers/usb/eth/smsc95xx.c
> +++ b/drivers/usb/eth/smsc95xx.c
>
> -	u32 addr_lo, addr_hi;
> +	u32 addr_lo = eth->enetaddr[0] | eth->enetaddr[1] << 8 |
> +		eth->enetaddr[2] << 16 | eth->enetaddr[3] << 24;
> +	u32 addr_hi = eth->enetaddr[4] | eth->enetaddr[5] << 8;

please use:
	#include <asm/unaligned.h>
	u32 addr_lo = __get_unaligned_le32(&eth->enetaddr[0]);
	u32 addr_hi = __get_unaligned_le16(&eth->enetaddr[4]);
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111111/1228de54/attachment.pgp 

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Mike Frysinger]

On Friday 11 November 2011 05:59:56 Wolfgang Grandegger wrote:
> Commit 79ad54400932d6484178a372fb3b659e3437473b broke the MAC
> address programming. Fix this by using the method from Linux'
> smsc95xx_set_mac_address().

oh, and while you're here, test out this fix:
-   debug("MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
-       eth->enetaddr[0], eth->enetaddr[1],
-       eth->enetaddr[2], eth->enetaddr[3],
-       eth->enetaddr[4], eth->enetaddr[5]);
+   debug("MAC %pM\n", eth->enetaddr);
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111111/c34a3d49/attachment.pgp 

[U-Boot] [PATCH 3/3] smsc95xx: remove an unecessary debug messages

[Simon Glass]

Hi Wolfgang,

On Fri, Nov 11, 2011 at 2:59 AM, Wolfgang Grandegger <wg@denx.de> wrote:
> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> ?drivers/usb/eth/smsc95xx.c | ? ?7 +++----
> ?1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
> index 16e24bd..40f7f5b 100644
> --- a/drivers/usb/eth/smsc95xx.c
> +++ b/drivers/usb/eth/smsc95xx.c
> @@ -380,15 +380,14 @@ static int smsc95xx_write_hwaddr(struct eth_device *eth)
> ? ? ? ?/* set hardware address */
> ? ? ? ?debug("** %s()\n", __func__);
>
> - ? ? ? ret = smsc95xx_write_reg(dev, ADDRL, addr_lo);
> - ? ? ? if (ret < 0) {
> - ? ? ? ? ? ? ? debug("Failed to write ADDRL: %d\n", ret);
> + ? ? ? ret = smsc95xx_write_reg(dev, ADDRL, adddr_lo);

addr_lo?

> + ? ? ? if (ret < 0)
> ? ? ? ? ? ? ? ?return ret;
> - ? ? ? }
>
> ? ? ? ?ret = smsc95xx_write_reg(dev, ADDRH, addr_hi);
> ? ? ? ?if (ret < 0)
> ? ? ? ? ? ? ? ?return ret;
> +

Since you are adding a blank line here, you need to fix the line below
:-) See Mike's email...

> ? ? ? ?debug("MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
> ? ? ? ? ? ? ? ?eth->enetaddr[0], eth->enetaddr[1],
> ? ? ? ? ? ? ? ?eth->enetaddr[2], eth->enetaddr[3],
> --
> 1.7.4.1
>
>

Regards,
Simon

[U-Boot] [PATCH 2/3] smsc95xx: in smsc95xx_set_multicast write to reg

[Simon Glass]

Hi Wolfgang,

On Fri, Nov 11, 2011 at 2:59 AM, Wolfgang Grandegger <wg@denx.de> wrote:
> The write to the mac_cr register was missing. This usually not
> cause an issue before, since the next function writing the
> register's shadow copy into the register would do it as a side
> effect.
>
> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> ?drivers/usb/eth/smsc95xx.c | ? ?2 ++
> ?1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
> index eb529f1..16e24bd 100644
> --- a/drivers/usb/eth/smsc95xx.c
> +++ b/drivers/usb/eth/smsc95xx.c
> @@ -428,6 +428,8 @@ static void smsc95xx_set_multicast(struct ueth_data *dev)
> ?{
> ? ? ? ?/* No multicast in u-boot */
> ? ? ? ?dev->mac_cr &= ~(MAC_CR_PRMS_ | MAC_CR_MCPAS_ | MAC_CR_HPFILT_);
> +
> + ? ? ? smsc95xx_write_reg(dev, MAC_CR, dev->mac_cr);

It seems a bit odd - what problem does your addition actually fix? At
present both smsc95xx_start_tx_path() and smsc95xx_start_rx_path()
write to this register, so it will be written three times in quick
succession. If there are no other callers to smsc95xx_set_multicast()
outside init, then perhaps we should just write it once in init, after
calling the three functions?

Regards,
Simon

> ?}
>
> ?/* starts the TX path */
> --
> 1.7.4.1
>
>

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Wolfgang Grandegger]

On 11/11/2011 04:18 PM, Mike Frysinger wrote:
> On Friday 11 November 2011 07:33:00 Wolfgang Grandegger wrote:
>> On 11/11/2011 12:04 PM, Marek Vasut wrote:
>>>> Commit 79ad54400932d6484178a372fb3b659e3437473b broke the MAC
>>>> address programming. Fix this by using the method from Linux'
>>>> smsc95xx_set_mac_address().
>>>>
>>>> --- a/drivers/usb/eth/smsc95xx.c
>>>> +++ b/drivers/usb/eth/smsc95xx.c
>>>>
>>>> -	u32 addr_lo, addr_hi;
>>>> +	u32 addr_lo = eth->enetaddr[0] | eth->enetaddr[1] << 8 |
>>>> +		eth->enetaddr[2] << 16 | eth->enetaddr[3] << 24;
>>>> +	u32 addr_hi = eth->enetaddr[4] | eth->enetaddr[5] << 8;
>>>>
>>>>  	int ret;
>>>>  	
>>>>  	/* set hardware address */
>>>>  	debug("** %s()\n", __func__);
>>>>
>>>> -	addr_lo = cpu_to_le32(*eth->enetaddr);
>>>> -	addr_hi = cpu_to_le16(*((u16 *)(eth->enetaddr + 4)));
>>>> +
>>>>
>>>
>>> didn't Mike Frysinger send similar patch yesterday? Also, do you have the
>>
>> Ah, I obviously missed that.
>>
>>> hardware? If so, that's good, we know you can provide tested fix.
>>
>> Well, yes, I know somebody who has smsc95xx hardware ;-).
>>
>>> Mike, are you good with using this fix instead or can you two negotiate?
>>
>> Let's await Mike's answer (now on CC as well). I need to re-send the
>> series anyway due to a stupid typo.
> 
> mine might take a little longer due to wrangling with wolfgang.  i'd suggest 
> we go with your patch (although i have feedback on it).
> -mike

Well, I have no problem with your patch if it fixes the same issue
correctly... and preparing and testing a new patch series takes time. I
will simply add my "Tested-by" when I have feedback and we then can drop
my other patches.

Wolfgang.

[U-Boot] [PATCH 3/3] smsc95xx: remove an unecessary debug messages

[Wolfgang Grandegger]

On 11/11/2011 04:26 PM, Simon Glass wrote:
> Hi Wolfgang,
> 
> On Fri, Nov 11, 2011 at 2:59 AM, Wolfgang Grandegger <wg@denx.de> wrote:
>> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>  drivers/usb/eth/smsc95xx.c |    7 +++----
>>  1 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
>> index 16e24bd..40f7f5b 100644
>> --- a/drivers/usb/eth/smsc95xx.c
>> +++ b/drivers/usb/eth/smsc95xx.c
>> @@ -380,15 +380,14 @@ static int smsc95xx_write_hwaddr(struct eth_device *eth)
>>        /* set hardware address */
>>        debug("** %s()\n", __func__);
>>
>> -       ret = smsc95xx_write_reg(dev, ADDRL, addr_lo);
>> -       if (ret < 0) {
>> -               debug("Failed to write ADDRL: %d\n", ret);
>> +       ret = smsc95xx_write_reg(dev, ADDRL, adddr_lo);
> 
> addr_lo?

Oh yeah, sneaked in somehow, sorry ... I'm now focusing on Mike's patch
fixing the same issue. Hope to have some feedback later today.

Wolfgang.

[U-Boot] [PATCH 2/3] smsc95xx: in smsc95xx_set_multicast write to reg

[Wolfgang Grandegger]

Hi Simon,

On 11/11/2011 04:35 PM, Simon Glass wrote:
> Hi Wolfgang,
> 
> On Fri, Nov 11, 2011 at 2:59 AM, Wolfgang Grandegger <wg@denx.de> wrote:
>> The write to the mac_cr register was missing. This usually not
>> cause an issue before, since the next function writing the
>> register's shadow copy into the register would do it as a side
>> effect.
>>
>> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> ---
>>  drivers/usb/eth/smsc95xx.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
>> index eb529f1..16e24bd 100644
>> --- a/drivers/usb/eth/smsc95xx.c
>> +++ b/drivers/usb/eth/smsc95xx.c
>> @@ -428,6 +428,8 @@ static void smsc95xx_set_multicast(struct ueth_data *dev)
>>  {
>>        /* No multicast in u-boot */
>>        dev->mac_cr &= ~(MAC_CR_PRMS_ | MAC_CR_MCPAS_ | MAC_CR_HPFILT_);
>> +
>> +       smsc95xx_write_reg(dev, MAC_CR, dev->mac_cr);
> 
> It seems a bit odd - what problem does your addition actually fix? At
> present both smsc95xx_start_tx_path() and smsc95xx_start_rx_path()
> write to this register, so it will be written three times in quick
> succession. If there are no other callers to smsc95xx_set_multicast()
> outside init, then perhaps we should just write it once in init, after
> calling the three functions?

The name "set_multicast" associated to me that the relevant register is
really set. But from the functional poit of few, it is not necessary.
Well, it's a minor issue and maybe not worth fixing. So, let's drop this
patch.

Wolfgang.

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Mike Frysinger]

On Monday 14 November 2011 03:25:37 Wolfgang Grandegger wrote:
> On 11/11/2011 04:18 PM, Mike Frysinger wrote:
> > mine might take a little longer due to wrangling with wolfgang.  i'd
> > suggest we go with your patch (although i have feedback on it).
> 
> Well, I have no problem with your patch if it fixes the same issue
> correctly... and preparing and testing a new patch series takes time. I
> will simply add my "Tested-by" when I have feedback and we then can drop
> my other patches.

let's not go with the one i submitted earlier.  tweak your patches with the 
feedback posted and we'll go with those.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111114/d32c6d25/attachment.pgp 

[U-Boot] [PATCH 2/3] smsc95xx: in smsc95xx_set_multicast write to reg

[Simon Glass]

Hi Wolfgang,

On Mon, Nov 14, 2011 at 4:18 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Hi Simon,
>
> On 11/11/2011 04:35 PM, Simon Glass wrote:
>> Hi Wolfgang,
>>
>> On Fri, Nov 11, 2011 at 2:59 AM, Wolfgang Grandegger <wg@denx.de> wrote:
>>> The write to the mac_cr register was missing. This usually not
>>> cause an issue before, since the next function writing the
>>> register's shadow copy into the register would do it as a side
>>> effect.
>>>
>>> Signed-off-by: Wolfgang Grandegger <wg@denx.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> ---
>>> ?drivers/usb/eth/smsc95xx.c | ? ?2 ++
>>> ?1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
>>> index eb529f1..16e24bd 100644
>>> --- a/drivers/usb/eth/smsc95xx.c
>>> +++ b/drivers/usb/eth/smsc95xx.c
>>> @@ -428,6 +428,8 @@ static void smsc95xx_set_multicast(struct ueth_data *dev)
>>> ?{
>>> ? ? ? ?/* No multicast in u-boot */
>>> ? ? ? ?dev->mac_cr &= ~(MAC_CR_PRMS_ | MAC_CR_MCPAS_ | MAC_CR_HPFILT_);
>>> +
>>> + ? ? ? smsc95xx_write_reg(dev, MAC_CR, dev->mac_cr);
>>
>> It seems a bit odd - what problem does your addition actually fix? At
>> present both smsc95xx_start_tx_path() and smsc95xx_start_rx_path()
>> write to this register, so it will be written three times in quick
>> succession. If there are no other callers to smsc95xx_set_multicast()
>> outside init, then perhaps we should just write it once in init, after
>> calling the three functions?
>
> The name "set_multicast" associated to me that the relevant register is
> really set.

Yes, although it is a static function.

> But from the functional poit of few, it is not necessary.
> Well, it's a minor issue and maybe not worth fixing. So, let's drop this
> patch.

If you like, although I agree a clean-up would be useful here.

Regards,
Simon

>
> Wolfgang.
>

[U-Boot] [PATCH 1/3] smsc95xx: Fix MAC address programming

[Wolfgang Grandegger]

On 11/14/2011 05:50 PM, Mike Frysinger wrote:
> On Monday 14 November 2011 03:25:37 Wolfgang Grandegger wrote:
>> On 11/11/2011 04:18 PM, Mike Frysinger wrote:
>>> mine might take a little longer due to wrangling with wolfgang.  i'd
>>> suggest we go with your patch (although i have feedback on it).
>>
>> Well, I have no problem with your patch if it fixes the same issue
>> correctly... and preparing and testing a new patch series takes time. I
>> will simply add my "Tested-by" when I have feedback and we then can drop
>> my other patches.
> 
> let's not go with the one i submitted earlier.  tweak your patches with the 
> feedback posted and we'll go with those.

OK, just sent v2.

Wolfgang.