| Message ID | 1321009198-20695-3-git-send-email-wg@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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 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.
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. >
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 */
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(-)