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

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

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.

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