| Message ID | 12131479795846@webcorp02g.yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 22, 2016 at 4:54 PM, Anton D. Kachalov <mouse@yandex-team.ru> wrote: > Hardware reset leads to clear all registers. > Make the hardware reset first before actual MAC address set in ftgmac100_initialization loop. > Based on datasheet only one bit (SW_RST) have to be set in order to proper MAC reset. > > Signed-off-by: Anton D. Kachalov <mouse@yandex-team.ru> Thanks for the patch! > --- > drivers/net/ftgmac100.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c > --- a/drivers/net/ftgmac100.c 2016-11-22 07:07:44.000000000 +0300 > +++ b/drivers/net/ftgmac100.c 2016-11-22 07:27:12.131940971 +0300 > @@ -457,7 +457,7 @@ static void ftgmac100_reset(struct eth_d > debug("%s()\n", __func__); > > //Ryan modify > - __raw_writel(__raw_readl(&ftgmac100->maccr) | FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); > + __raw_writel(FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); I assume all of the other values in MACCR will be reset when we do the software reset, so that's why you don't need to read-modify-write? > > while (__raw_readl(&ftgmac100->maccr) & FTGMAC100_MACCR_SW_RST); > > @@ -791,11 +791,11 @@ int ftgmac100_initialize(bd_t *bd) > miiphy_register(dev->name, ftgmac100_reg_read, ftgmac100_reg_write); > #endif > > + ftgmac100_reset(dev); > + This part of the change looks good. Cheers, Joel > /* set the ethernet address */ > ftgmac100_set_mac_from_env(dev); > > - ftgmac100_reset(dev); > - > card_number++; > } > return card_number; > _______________________________________________ > openbmc mailing list > openbmc@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/openbmc
23.11.2016, 07:57, "Joel Stanley" <joel@jms.id.au>: > On Tue, Nov 22, 2016 at 4:54 PM, Anton D. Kachalov <mouse@yandex-team.ru> wrote: >> Hardware reset leads to clear all registers. >> Make the hardware reset first before actual MAC address set in ftgmac100_initialization loop. >> Based on datasheet only one bit (SW_RST) have to be set in order to proper MAC reset. >> >> Signed-off-by: Anton D. Kachalov <mouse@yandex-team.ru> > > Thanks for the patch! > >> --- >> drivers/net/ftgmac100.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c >> --- a/drivers/net/ftgmac100.c 2016-11-22 07:07:44.000000000 +0300 >> +++ b/drivers/net/ftgmac100.c 2016-11-22 07:27:12.131940971 +0300 >> @@ -457,7 +457,7 @@ static void ftgmac100_reset(struct eth_d >> debug("%s()\n", __func__); >> >> //Ryan modify >> - __raw_writel(__raw_readl(&ftgmac100->maccr) | FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); >> + __raw_writel(FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); > > I assume all of the other values in MACCR will be reset when we do the > software reset, so that's why you don't need to read-modify-write? Regarding to the datasheet reset sequence, only one bit (SW_RST) have to be written. Within real hardware (AST2150) read-modify-write doesn't work. Sametime, kernel's driver has a proper init sequence too (only one bit set). > >> while (__raw_readl(&ftgmac100->maccr) & FTGMAC100_MACCR_SW_RST); >> >> @@ -791,11 +791,11 @@ int ftgmac100_initialize(bd_t *bd) >> miiphy_register(dev->name, ftgmac100_reg_read, ftgmac100_reg_write); >> #endif >> >> + ftgmac100_reset(dev); >> + > > This part of the change looks good. > > Cheers, > > Joel > >> /* set the ethernet address */ >> ftgmac100_set_mac_from_env(dev); >> >> - ftgmac100_reset(dev); >> - >> card_number++; >> } >> return card_number; >> _______________________________________________ >> openbmc mailing list >> openbmc@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/openbmc
ping 23.11.2016, 08:12, "Anton D. Kachalov" <mouse@yandex-team.ru>: > 23.11.2016, 07:57, "Joel Stanley" <joel@jms.id.au>: >> On Tue, Nov 22, 2016 at 4:54 PM, Anton D. Kachalov <mouse@yandex-team.ru> wrote: >>> Hardware reset leads to clear all registers. >>> Make the hardware reset first before actual MAC address set in ftgmac100_initialization loop. >>> Based on datasheet only one bit (SW_RST) have to be set in order to proper MAC reset. >>> >>> Signed-off-by: Anton D. Kachalov <mouse@yandex-team.ru> >> >> Thanks for the patch! >> >>> --- >>> drivers/net/ftgmac100.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c >>> --- a/drivers/net/ftgmac100.c 2016-11-22 07:07:44.000000000 +0300 >>> +++ b/drivers/net/ftgmac100.c 2016-11-22 07:27:12.131940971 +0300 >>> @@ -457,7 +457,7 @@ static void ftgmac100_reset(struct eth_d >>> debug("%s()\n", __func__); >>> >>> //Ryan modify >>> - __raw_writel(__raw_readl(&ftgmac100->maccr) | FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); >>> + __raw_writel(FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); >> >> I assume all of the other values in MACCR will be reset when we do the >> software reset, so that's why you don't need to read-modify-write? > > Regarding to the datasheet reset sequence, only one bit (SW_RST) have to be written. Within real hardware (AST2150) read-modify-write doesn't work. Sametime, kernel's driver has a proper init sequence too (only one bit set). > >>> while (__raw_readl(&ftgmac100->maccr) & FTGMAC100_MACCR_SW_RST); >>> >>> @@ -791,11 +791,11 @@ int ftgmac100_initialize(bd_t *bd) >>> miiphy_register(dev->name, ftgmac100_reg_read, ftgmac100_reg_write); >>> #endif >>> >>> + ftgmac100_reset(dev); >>> + >> >> This part of the change looks good. >> >> Cheers, >> >> Joel >> >>> /* set the ethernet address */ >>> ftgmac100_set_mac_from_env(dev); >>> >>> - ftgmac100_reset(dev); >>> - >>> card_number++; >>> } >>> return card_number; >>> _______________________________________________ >>> openbmc mailing list >>> openbmc@lists.ozlabs.org >>> https://lists.ozlabs.org/listinfo/openbmc > > -- > Anton D. Kachalov > > _______________________________________________ > openbmc mailing list > openbmc@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/openbmc
On 11/23/2016 06:01 AM, Anton D. Kachalov wrote: > > > 23.11.2016, 07:57, "Joel Stanley" <joel@jms.id.au>: >> On Tue, Nov 22, 2016 at 4:54 PM, Anton D. Kachalov <mouse@yandex-team.ru> wrote: >>> Hardware reset leads to clear all registers. >>> Make the hardware reset first before actual MAC address set in ftgmac100_initialization loop. >>> Based on datasheet only one bit (SW_RST) have to be set in order to proper MAC reset. >>> >>> Signed-off-by: Anton D. Kachalov <mouse@yandex-team.ru> >> >> Thanks for the patch! >> >>> --- >>> drivers/net/ftgmac100.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c >>> --- a/drivers/net/ftgmac100.c 2016-11-22 07:07:44.000000000 +0300 >>> +++ b/drivers/net/ftgmac100.c 2016-11-22 07:27:12.131940971 +0300 >>> @@ -457,7 +457,7 @@ static void ftgmac100_reset(struct eth_d >>> debug("%s()\n", __func__); >>> >>> //Ryan modify >>> - __raw_writel(__raw_readl(&ftgmac100->maccr) | FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); >>> + __raw_writel(FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); >> >> I assume all of the other values in MACCR will be reset when we do the >> software reset, so that's why you don't need to read-modify-write? > > Regarding to the datasheet reset sequence, only one bit (SW_RST) have to be written. I checked the specs. This is correct. However, there is also this quote (on AST2400 and AST2500) : After setting the SW RST (MAC50 [31]) = 1 to do the software reset, it need to delay at lease 10 us and then setting the SW RST (MAC50 [31]) = 1 to do the software reset again. This means the software reset need to be done at least twice Shouldn't we add a delay and an extra reset before polling on the maccr value ? Thanks, C. > Within real hardware (AST2150) read-modify-write doesn't work. Sametime, kernel's driver > has a proper init sequence too (only one bit set). > >> >>> while (__raw_readl(&ftgmac100->maccr) & FTGMAC100_MACCR_SW_RST); >>> >>> @@ -791,11 +791,11 @@ int ftgmac100_initialize(bd_t *bd) >>> miiphy_register(dev->name, ftgmac100_reg_read, ftgmac100_reg_write); >>> #endif >>> >>> + ftgmac100_reset(dev); >>> + >> >> This part of the change looks good. >> >> Cheers, >> >> Joel >> >>> /* set the ethernet address */ >>> ftgmac100_set_mac_from_env(dev); >>> >>> - ftgmac100_reset(dev); >>> - >>> card_number++; >>> } >>> return card_number; >>> _______________________________________________ >>> openbmc mailing list >>> openbmc@lists.ozlabs.org >>> https://lists.ozlabs.org/listinfo/openbmc >
diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c --- a/drivers/net/ftgmac100.c 2016-11-22 07:07:44.000000000 +0300 +++ b/drivers/net/ftgmac100.c 2016-11-22 07:27:12.131940971 +0300 @@ -457,7 +457,7 @@ static void ftgmac100_reset(struct eth_d debug("%s()\n", __func__); //Ryan modify - __raw_writel(__raw_readl(&ftgmac100->maccr) | FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); + __raw_writel(FTGMAC100_MACCR_SW_RST, &ftgmac100->maccr); while (__raw_readl(&ftgmac100->maccr) & FTGMAC100_MACCR_SW_RST); @@ -791,11 +791,11 @@ int ftgmac100_initialize(bd_t *bd) miiphy_register(dev->name, ftgmac100_reg_read, ftgmac100_reg_write); #endif + ftgmac100_reset(dev); + /* set the ethernet address */ ftgmac100_set_mac_from_env(dev); - ftgmac100_reset(dev); - card_number++; } return card_number;
Hardware reset leads to clear all registers. Make the hardware reset first before actual MAC address set in ftgmac100_initialization loop. Based on datasheet only one bit (SW_RST) have to be set in order to proper MAC reset. Signed-off-by: Anton D. Kachalov <mouse@yandex-team.ru> --- drivers/net/ftgmac100.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)