ftgmac100: fixup MAC address set during board bringup

Submitted by Anton D. Kachalov on Nov. 22, 2016, 6:24 a.m.

Details

Message ID 12131479795846@webcorp02g.yandex-team.ru
State New
Headers show

Commit Message

Anton D. Kachalov Nov. 22, 2016, 6:24 a.m.
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(-)

Comments

Joel Stanley Nov. 23, 2016, 4:56 a.m.
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
Anton D. Kachalov Nov. 23, 2016, 5:01 a.m.
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 Dec. 6, 2016, 1:43 p.m.
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
Cédric Le Goater Dec. 6, 2016, 4:25 p.m.
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
>

Patch hide | download patch | download mbox

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;