diff mbox

[U-Boot] drivers: net: phy: atheros: apply the previous register setting for AR8031 to fix the voltage peak issue

Message ID a4d08fed-beb0-52c0-ac13-25d7b6eb9314@ti.com
State Rejected
Delegated to: Joe Hershberger
Headers show

Commit Message

Sekhar Nori Feb. 7, 2017, 8:50 a.m. UTC 
On Monday 06 February 2017 11:06 PM, Ken.Lin wrote:

>>> The register setting would turn out to be 0x3D47 on our project boards and
>> our signal measurement results show the patch (v2 version,
>> https://patchwork.ozlabs.org/patch/723461/)) could fix the voltage peak issue.
>>> The proposed solution is to follow the implementation in previous commits,
>> which include the reserved bits settings in SerDes Test and System Mode Control
>> register.

>> So what does the register setting turn out to be with my patch below ?
>>
>> What are the "previous commits" you refer to ?

Thanks for the references to the commits. You left out answering my 
question about what register settings you see with my patch.

I have included another patch now with some debug enabled. Can you 
apply this patch to latest u-boot master, run on your board and send me 
the log ? Here is what I see on AM335x EVM-SK:

U-Boot 2017.03-rc1-00058-g1216f9a0851f (Feb 07 2017 - 13:55:38 +0530)

CPU  : AM335X-GP rev 1.0
Model: TI AM335x EVM-SK
DRAM:  256 MiB
NAND:  0 MiB
MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
reading uboot.env
ERROR: No USB device found

at ../drivers/usb/gadget/ether.c:2709/usb_ether_init()
Net:   ar8031_config: value read back 0x2c47, written: 0x2d47
eth0: ethernet@4a100000
Hit any key to stop autoboot:  0 

Thanks,
Sekhar

---8<---

Comments

Sekhar Nori Feb. 8, 2017, 6:26 a.m. UTC | #1 
On Wednesday 08 February 2017 12:36 AM, Yung-Ching LIN wrote:
> On Tue, Feb 7, 2017 at 12:50 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>> On Monday 06 February 2017 11:06 PM, Ken.Lin wrote:
>>
>>>>> The register setting would turn out to be 0x3D47 on our project boards and
>>>> our signal measurement results show the patch (v2 version,
>>>> https://patchwork.ozlabs.org/patch/723461/)) could fix the voltage peak issue.
>>>>> The proposed solution is to follow the implementation in previous commits,
>>>> which include the reserved bits settings in SerDes Test and System Mode Control
>>>> register.
>>
>>>> So what does the register setting turn out to be with my patch below ?
>>>>
>>>> What are the "previous commits" you refer to ?
>>
>> Thanks for the references to the commits. You left out answering my
>> question about what register settings you see with my patch.
>>
>> I have included another patch now with some debug enabled. Can you
>> apply this patch to latest u-boot master, run on your board and send me
>> the log ? Here is what I see on AM335x EVM-SK:
>>
>> U-Boot 2017.03-rc1-00058-g1216f9a0851f (Feb 07 2017 - 13:55:38 +0530)
>>
>> CPU  : AM335X-GP rev 1.0
>> Model: TI AM335x EVM-SK
>> DRAM:  256 MiB
>> NAND:  0 MiB
>> MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
>> reading uboot.env
>> ERROR: No USB device found
>>
>> at ../drivers/usb/gadget/ether.c:2709/usb_ether_init()
>> Net:   ar8031_config: value read back 0x2c47, written: 0x2d47
>> eth0: ethernet@4a100000
>> Hit any key to stop autoboot:  0
>>
>> Thanks,
>> Sekhar
>>
>> ---8<---
>> diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c
>> index b34cdd3d87dc..5c0a36676ce9 100644
>> --- a/drivers/net/phy/atheros.c
>> +++ b/drivers/net/phy/atheros.c
>> @@ -28,12 +28,18 @@ static int ar8021_config(struct phy_device *phydev)
>>
>>  static int ar8031_config(struct phy_device *phydev)
>>  {
>> +       int regval;
>> +
>>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>>             phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>>                 phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG,
>>                           AR803x_DEBUG_REG_5);
>> +               regval = phy_read(phydev, MDIO_DEVAD_NONE,
>> +                                 AR803x_PHY_DEBUG_DATA_REG);
>> +               printf("%s: value read back 0x%x, written: 0x%x\n",
>> +                               __func__, regval, regval | AR803x_RGMII_TX_CLK_DLY);
>>                 phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG,
>> -                         AR803x_RGMII_TX_CLK_DLY);
>> +                         regval | AR803x_RGMII_TX_CLK_DLY);
>>         }
>>
>>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>>
> 
> 
> 
> Hi ,
> 
> Your patch doesn't work for the issue case on our project board.
> I added more debug messages for your reference.
> 

[...]

> 
> U-Boot 2017.03-rc1-00057-gc83a824e62-dirty (Feb 08 2017 - 02:54:43 +0800)
> 
> CPU:   Freescale i.MX6D rev1.5 at 792 MHz
> Reset cause: POR
> BOARD: General Electric B850v3
> I2C:   ready
> DRAM:  2 GiB
> MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
> SF: Detected n25q32 with page size 256 Bytes, erase size 4 KiB, total 4 MiB
> *** Warning - bad CRC, using default environment
> 
> In:    serial
> Out:   serial
> Err:   serial
> Net:   eth_init: fec_probe(bd, -1, 4) @ 02188000
> fec_mii_setspeed: mii_speed 0000001a
> fec_mdio_read: phy: 04 reg:02 val:0x4d
> fec_mdio_read: phy: 04 reg:03 val:0xd074
> fec_mii_setspeed: mii_speed 0000001a
> fec_mdio_write: phy: 04 reg:00 val:0x8000
> fec_mdio_read: phy: 04 reg:00 val:0x0
> fec_mdio_write: phy: 04 reg:0d val:0x7
> fec_mdio_write: phy: 04 reg:0e val:0x8016
> fec_mdio_write: phy: 04 reg:0d val:0x4007
> fec_mdio_write: phy: 04 reg:0e val:0x18
> fec_mdio_write: phy: 04 reg:1d val:0x5
> fec_mdio_write: phy: 04 reg:1e val:0x100
> fec_mdio_write: phy: 04 reg:1d val:0x5
> fec_mdio_read: phy: 04 reg:1e val:0x100
> ar8031_config check: value read back 0x100, written: 0x100

Hmm, so in effect you are forced to use the magic value of 0x3c47 as the
reset default when actually it is 0x100 on your board. And there is no
backing public documentation on why the reset default should be 0x3c47.
And whether it will work for all boards that use this PHY.

Thats pretty unmaintainable in my opinion. If this value does not work
for the next person that comes along, we will be in trouble again.

I guess the best thing that can be done at this point is to use this
magic reset default value in a board specific way. By specifying it
using device-tree, perhaps. I would wait for some feedback from Joe
Hershberger though.

BTW, do you have the same problem in kernel ? Because AFAICS, even
drivers/net/phy/at803x.c in kernel does not have any such provision for
magic reset default values.

> fec_mdio_read: phy: 04 reg:04 val:0x1de1
> fec_mdio_write: phy: 04 reg:04 val:0x11e1
> fec_mdio_read: phy: 04 reg:01 val:0x7949
> fec_mdio_read: phy: 04 reg:09 val:0x200
> fec_mdio_write: phy: 04 reg:09 val:0x300
> fec_mdio_read: phy: 04 reg:00 val:0x0
> fec_mdio_write: phy: 04 reg:00 val:0x1200
> fec_mdio_read: phy: 04 reg:00 val:0x1000
> fec_mdio_write: phy: 04 reg:00 val:0x1200
> FEC [PRIME]
> Error: FEC address not set.
> 
> Hit any key to stop autoboot:  1  0

Thanks,
Sekhar
Joe Hershberger Feb. 8, 2017, 6:47 p.m. UTC | #2 
On Wed, Feb 8, 2017 at 12:26 AM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Wednesday 08 February 2017 12:36 AM, Yung-Ching LIN wrote:
>> On Tue, Feb 7, 2017 at 12:50 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>> On Monday 06 February 2017 11:06 PM, Ken.Lin wrote:
>>>
>>>>>> The register setting would turn out to be 0x3D47 on our project boards and
>>>>> our signal measurement results show the patch (v2 version,
>>>>> https://patchwork.ozlabs.org/patch/723461/)) could fix the voltage peak issue.
>>>>>> The proposed solution is to follow the implementation in previous commits,
>>>>> which include the reserved bits settings in SerDes Test and System Mode Control
>>>>> register.
>>>
>>>>> So what does the register setting turn out to be with my patch below ?
>>>>>
>>>>> What are the "previous commits" you refer to ?
>>>
>>> Thanks for the references to the commits. You left out answering my
>>> question about what register settings you see with my patch.
>>>
>>> I have included another patch now with some debug enabled. Can you
>>> apply this patch to latest u-boot master, run on your board and send me
>>> the log ? Here is what I see on AM335x EVM-SK:
>>>
>>> U-Boot 2017.03-rc1-00058-g1216f9a0851f (Feb 07 2017 - 13:55:38 +0530)
>>>
>>> CPU  : AM335X-GP rev 1.0
>>> Model: TI AM335x EVM-SK
>>> DRAM:  256 MiB
>>> NAND:  0 MiB
>>> MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
>>> reading uboot.env
>>> ERROR: No USB device found
>>>
>>> at ../drivers/usb/gadget/ether.c:2709/usb_ether_init()
>>> Net:   ar8031_config: value read back 0x2c47, written: 0x2d47
>>> eth0: ethernet@4a100000
>>> Hit any key to stop autoboot:  0
>>>
>>> Thanks,
>>> Sekhar
>>>
>>> ---8<---
>>> diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c
>>> index b34cdd3d87dc..5c0a36676ce9 100644
>>> --- a/drivers/net/phy/atheros.c
>>> +++ b/drivers/net/phy/atheros.c
>>> @@ -28,12 +28,18 @@ static int ar8021_config(struct phy_device *phydev)
>>>
>>>  static int ar8031_config(struct phy_device *phydev)
>>>  {
>>> +       int regval;
>>> +
>>>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>>>             phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>>>                 phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG,
>>>                           AR803x_DEBUG_REG_5);
>>> +               regval = phy_read(phydev, MDIO_DEVAD_NONE,
>>> +                                 AR803x_PHY_DEBUG_DATA_REG);
>>> +               printf("%s: value read back 0x%x, written: 0x%x\n",
>>> +                               __func__, regval, regval | AR803x_RGMII_TX_CLK_DLY);
>>>                 phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG,
>>> -                         AR803x_RGMII_TX_CLK_DLY);
>>> +                         regval | AR803x_RGMII_TX_CLK_DLY);
>>>         }
>>>
>>>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>>>
>>
>>
>>
>> Hi ,
>>
>> Your patch doesn't work for the issue case on our project board.
>> I added more debug messages for your reference.
>>
>
> [...]
>
>>
>> U-Boot 2017.03-rc1-00057-gc83a824e62-dirty (Feb 08 2017 - 02:54:43 +0800)
>>
>> CPU:   Freescale i.MX6D rev1.5 at 792 MHz
>> Reset cause: POR
>> BOARD: General Electric B850v3
>> I2C:   ready
>> DRAM:  2 GiB
>> MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
>> SF: Detected n25q32 with page size 256 Bytes, erase size 4 KiB, total 4 MiB
>> *** Warning - bad CRC, using default environment
>>
>> In:    serial
>> Out:   serial
>> Err:   serial
>> Net:   eth_init: fec_probe(bd, -1, 4) @ 02188000
>> fec_mii_setspeed: mii_speed 0000001a
>> fec_mdio_read: phy: 04 reg:02 val:0x4d
>> fec_mdio_read: phy: 04 reg:03 val:0xd074
>> fec_mii_setspeed: mii_speed 0000001a
>> fec_mdio_write: phy: 04 reg:00 val:0x8000
>> fec_mdio_read: phy: 04 reg:00 val:0x0
>> fec_mdio_write: phy: 04 reg:0d val:0x7
>> fec_mdio_write: phy: 04 reg:0e val:0x8016
>> fec_mdio_write: phy: 04 reg:0d val:0x4007
>> fec_mdio_write: phy: 04 reg:0e val:0x18
>> fec_mdio_write: phy: 04 reg:1d val:0x5
>> fec_mdio_write: phy: 04 reg:1e val:0x100
>> fec_mdio_write: phy: 04 reg:1d val:0x5
>> fec_mdio_read: phy: 04 reg:1e val:0x100
>> ar8031_config check: value read back 0x100, written: 0x100
>
> Hmm, so in effect you are forced to use the magic value of 0x3c47 as the
> reset default when actually it is 0x100 on your board. And there is no
> backing public documentation on why the reset default should be 0x3c47.
> And whether it will work for all boards that use this PHY.

Well that's quite unfortunate.

> Thats pretty unmaintainable in my opinion. If this value does not work
> for the next person that comes along, we will be in trouble again.
>
> I guess the best thing that can be done at this point is to use this
> magic reset default value in a board specific way. By specifying it
> using device-tree, perhaps. I would wait for some feedback from Joe
> Hershberger though.

I think we can wait until someone else says they have a problem with
this value before making it a board option.

> BTW, do you have the same problem in kernel ? Because AFAICS, even
> drivers/net/phy/at803x.c in kernel does not have any such provision for
> magic reset default values.

I'm interested to know this too. Is the kernel depending on the
bootloader to set this and it just avoids changing it?

>> fec_mdio_read: phy: 04 reg:04 val:0x1de1
>> fec_mdio_write: phy: 04 reg:04 val:0x11e1
>> fec_mdio_read: phy: 04 reg:01 val:0x7949
>> fec_mdio_read: phy: 04 reg:09 val:0x200
>> fec_mdio_write: phy: 04 reg:09 val:0x300
>> fec_mdio_read: phy: 04 reg:00 val:0x0
>> fec_mdio_write: phy: 04 reg:00 val:0x1200
>> fec_mdio_read: phy: 04 reg:00 val:0x1000
>> fec_mdio_write: phy: 04 reg:00 val:0x1200
>> FEC [PRIME]
>> Error: FEC address not set.
>>
>> Hit any key to stop autoboot:  1  0
>
> Thanks,
> Sekhar
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Ken Lin Feb. 9, 2017, 1:35 a.m. UTC | #3 
On Wed, Feb 8, 2017 at 10:47 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> On Wed, Feb 8, 2017 at 12:26 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>> On Wednesday 08 February 2017 12:36 AM, Yung-Ching LIN wrote:
>>> On Tue, Feb 7, 2017 at 12:50 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>>> On Monday 06 February 2017 11:06 PM, Ken.Lin wrote:
>>>>
>>>>>>> The register setting would turn out to be 0x3D47 on our project boards and
>>>>>> our signal measurement results show the patch (v2 version,
>>>>>> https://patchwork.ozlabs.org/patch/723461/)) could fix the voltage peak issue.
>>>>>>> The proposed solution is to follow the implementation in previous commits,
>>>>>> which include the reserved bits settings in SerDes Test and System Mode Control
>>>>>> register.
>>>>
>>>>>> So what does the register setting turn out to be with my patch below ?
>>>>>>
>>>>>> What are the "previous commits" you refer to ?
>>>>
>>>> Thanks for the references to the commits. You left out answering my
>>>> question about what register settings you see with my patch.
>>>>
>>>> I have included another patch now with some debug enabled. Can you
>>>> apply this patch to latest u-boot master, run on your board and send me
>>>> the log ? Here is what I see on AM335x EVM-SK:
>>>>
>>>> U-Boot 2017.03-rc1-00058-g1216f9a0851f (Feb 07 2017 - 13:55:38 +0530)
>>>>
>>>> CPU  : AM335X-GP rev 1.0
>>>> Model: TI AM335x EVM-SK
>>>> DRAM:  256 MiB
>>>> NAND:  0 MiB
>>>> MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
>>>> reading uboot.env
>>>> ERROR: No USB device found
>>>>
>>>> at ../drivers/usb/gadget/ether.c:2709/usb_ether_init()
>>>> Net:   ar8031_config: value read back 0x2c47, written: 0x2d47
>>>> eth0: ethernet@4a100000
>>>> Hit any key to stop autoboot:  0
>>>>
>>>> Thanks,
>>>> Sekhar
>>>>
>>>> ---8<---
>>>> diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c
>>>> index b34cdd3d87dc..5c0a36676ce9 100644
>>>> --- a/drivers/net/phy/atheros.c
>>>> +++ b/drivers/net/phy/atheros.c
>>>> @@ -28,12 +28,18 @@ static int ar8021_config(struct phy_device *phydev)
>>>>
>>>>  static int ar8031_config(struct phy_device *phydev)
>>>>  {
>>>> +       int regval;
>>>> +
>>>>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>>>>             phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>>>>                 phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG,
>>>>                           AR803x_DEBUG_REG_5);
>>>> +               regval = phy_read(phydev, MDIO_DEVAD_NONE,
>>>> +                                 AR803x_PHY_DEBUG_DATA_REG);
>>>> +               printf("%s: value read back 0x%x, written: 0x%x\n",
>>>> +                               __func__, regval, regval | AR803x_RGMII_TX_CLK_DLY);
>>>>                 phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG,
>>>> -                         AR803x_RGMII_TX_CLK_DLY);
>>>> +                         regval | AR803x_RGMII_TX_CLK_DLY);
>>>>         }
>>>>
>>>>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>>>>
>>>
>>>
>>>
>>> Hi ,
>>>
>>> Your patch doesn't work for the issue case on our project board.
>>> I added more debug messages for your reference.
>>>
>>
>> [...]
>>
>>>
>>> U-Boot 2017.03-rc1-00057-gc83a824e62-dirty (Feb 08 2017 - 02:54:43 +0800)
>>>
>>> CPU:   Freescale i.MX6D rev1.5 at 792 MHz
>>> Reset cause: POR
>>> BOARD: General Electric B850v3
>>> I2C:   ready
>>> DRAM:  2 GiB
>>> MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
>>> SF: Detected n25q32 with page size 256 Bytes, erase size 4 KiB, total 4 MiB
>>> *** Warning - bad CRC, using default environment
>>>
>>> In:    serial
>>> Out:   serial
>>> Err:   serial
>>> Net:   eth_init: fec_probe(bd, -1, 4) @ 02188000
>>> fec_mii_setspeed: mii_speed 0000001a
>>> fec_mdio_read: phy: 04 reg:02 val:0x4d
>>> fec_mdio_read: phy: 04 reg:03 val:0xd074
>>> fec_mii_setspeed: mii_speed 0000001a
>>> fec_mdio_write: phy: 04 reg:00 val:0x8000
>>> fec_mdio_read: phy: 04 reg:00 val:0x0
>>> fec_mdio_write: phy: 04 reg:0d val:0x7
>>> fec_mdio_write: phy: 04 reg:0e val:0x8016
>>> fec_mdio_write: phy: 04 reg:0d val:0x4007
>>> fec_mdio_write: phy: 04 reg:0e val:0x18
>>> fec_mdio_write: phy: 04 reg:1d val:0x5
>>> fec_mdio_write: phy: 04 reg:1e val:0x100
>>> fec_mdio_write: phy: 04 reg:1d val:0x5
>>> fec_mdio_read: phy: 04 reg:1e val:0x100
>>> ar8031_config check: value read back 0x100, written: 0x100
>>
>> Hmm, so in effect you are forced to use the magic value of 0x3c47 as the
>> reset default when actually it is 0x100 on your board. And there is no
>> backing public documentation on why the reset default should be 0x3c47.
>> And whether it will work for all boards that use this PHY.
>
> Well that's quite unfortunate.
>
>> Thats pretty unmaintainable in my opinion. If this value does not work
>> for the next person that comes along, we will be in trouble again.
>>
>> I guess the best thing that can be done at this point is to use this
>> magic reset default value in a board specific way. By specifying it
>> using device-tree, perhaps. I would wait for some feedback from Joe
>> Hershberger though.
>
> I think we can wait until someone else says they have a problem with
> this value before making it a board option.
>
>> BTW, do you have the same problem in kernel ? Because AFAICS, even
>> drivers/net/phy/at803x.c in kernel does not have any such provision for
>> magic reset default values.
>
> I'm interested to know this too. Is the kernel depending on the
> bootloader to set this and it just avoids changing it?
>

Yes, I think so.Here are the code snippets in drivers/net/phy/at803x.c.
I found it wouldn't change the bootloader setting when I dumped the
value in kernel.

static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
                                 u16 clear, u16 set)
{
        u16 val;
        int ret;

        ret = at803x_debug_reg_read(phydev, reg);
        if (ret < 0)
                return ret;

        val = ret & 0xffff;
        val &= ~clear;
        val |= set;

        return phy_write(phydev, AT803X_DEBUG_DATA, val);
}


static inline int at803x_enable_tx_delay(struct phy_device *phydev)
{
        return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
                                        AT803X_DEBUG_TX_CLK_DLY_EN);
}




>>> fec_mdio_read: phy: 04 reg:04 val:0x1de1
>>> fec_mdio_write: phy: 04 reg:04 val:0x11e1
>>> fec_mdio_read: phy: 04 reg:01 val:0x7949
>>> fec_mdio_read: phy: 04 reg:09 val:0x200
>>> fec_mdio_write: phy: 04 reg:09 val:0x300
>>> fec_mdio_read: phy: 04 reg:00 val:0x0
>>> fec_mdio_write: phy: 04 reg:00 val:0x1200
>>> fec_mdio_read: phy: 04 reg:00 val:0x1000
>>> fec_mdio_write: phy: 04 reg:00 val:0x1200
>>> FEC [PRIME]
>>> Error: FEC address not set.
>>>
>>> Hit any key to stop autoboot:  1  0
>>
>> Thanks,
>> Sekhar
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
Ken Lin March 3, 2017, 1:58 a.m. UTC | #4 
On Wed, Feb 8, 2017 at 5:35 PM, Yung-Ching LIN <yungching0725@gmail.com> wrote:
> On Wed, Feb 8, 2017 at 10:47 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> On Wed, Feb 8, 2017 at 12:26 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>> On Wednesday 08 February 2017 12:36 AM, Yung-Ching LIN wrote:
>>>> On Tue, Feb 7, 2017 at 12:50 AM, Sekhar Nori <nsekhar@ti.com> wrote:
>>>>> On Monday 06 February 2017 11:06 PM, Ken.Lin wrote:
>>>>>
>>>>>>>> The register setting would turn out to be 0x3D47 on our project boards and
>>>>>>> our signal measurement results show the patch (v2 version,
>>>>>>> https://patchwork.ozlabs.org/patch/723461/)) could fix the voltage peak issue.
>>>>>>>> The proposed solution is to follow the implementation in previous commits,
>>>>>>> which include the reserved bits settings in SerDes Test and System Mode Control
>>>>>>> register.
>>>>>
>>>>>>> So what does the register setting turn out to be with my patch below ?
>>>>>>>
>>>>>>> What are the "previous commits" you refer to ?
>>>>>
>>>>> Thanks for the references to the commits. You left out answering my
>>>>> question about what register settings you see with my patch.
>>>>>
>>>>> I have included another patch now with some debug enabled. Can you
>>>>> apply this patch to latest u-boot master, run on your board and send me
>>>>> the log ? Here is what I see on AM335x EVM-SK:
>>>>>
>>>>> U-Boot 2017.03-rc1-00058-g1216f9a0851f (Feb 07 2017 - 13:55:38 +0530)
>>>>>
>>>>> CPU  : AM335X-GP rev 1.0
>>>>> Model: TI AM335x EVM-SK
>>>>> DRAM:  256 MiB
>>>>> NAND:  0 MiB
>>>>> MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
>>>>> reading uboot.env
>>>>> ERROR: No USB device found
>>>>>
>>>>> at ../drivers/usb/gadget/ether.c:2709/usb_ether_init()
>>>>> Net:   ar8031_config: value read back 0x2c47, written: 0x2d47
>>>>> eth0: ethernet@4a100000
>>>>> Hit any key to stop autoboot:  0
>>>>>
>>>>> Thanks,
>>>>> Sekhar
>>>>>
>>>>> ---8<---
>>>>> diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c
>>>>> index b34cdd3d87dc..5c0a36676ce9 100644
>>>>> --- a/drivers/net/phy/atheros.c
>>>>> +++ b/drivers/net/phy/atheros.c
>>>>> @@ -28,12 +28,18 @@ static int ar8021_config(struct phy_device *phydev)
>>>>>
>>>>>  static int ar8031_config(struct phy_device *phydev)
>>>>>  {
>>>>> +       int regval;
>>>>> +
>>>>>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>>>>>             phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
>>>>>                 phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG,
>>>>>                           AR803x_DEBUG_REG_5);
>>>>> +               regval = phy_read(phydev, MDIO_DEVAD_NONE,
>>>>> +                                 AR803x_PHY_DEBUG_DATA_REG);
>>>>> +               printf("%s: value read back 0x%x, written: 0x%x\n",
>>>>> +                               __func__, regval, regval | AR803x_RGMII_TX_CLK_DLY);
>>>>>                 phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG,
>>>>> -                         AR803x_RGMII_TX_CLK_DLY);
>>>>> +                         regval | AR803x_RGMII_TX_CLK_DLY);
>>>>>         }
>>>>>
>>>>>         if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>>>>>
>>>>
>>>>
>>>>


Your read-modify-write patch makes sense to me at some point.
I added some debug messages in the u-boot source.
ar8031_config gets called after the board specific mx6_rgmii_rework callback
In our board test case, the phydev interface uses
PHY_INTERFACE_MODE_RGMII, so it won't hit the condition and run the
code statements you implemented.


U-Boot 2016.11-g922cf19526-dirty (Mar 03 2017 - 07:13:39 +0800)

CPU:   Freescale i.MX6D rev1.5 at 792 MHz
Reset cause: POR
BOARD: General Electric B850v3
I2C:   ready
DRAM:  2 GiB
MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
SF: Detected N25Q32 with page size 256 Bytes, erase size 4 KiB, total 4 MiB
*** Warning - bad CRC, using default environment

In:    serial
Out:   serial
Err:   serial
Net:   eth_init: fec_probe(bd, -1, 4) @ 02188000
fec_mii_setspeed: mii_speed 0000001a
fec_mdio_read: phy: 04 reg:02 val:0x4d
fec_mdio_read: phy: 04 reg:03 val:0xd074
fec_mii_setspeed: mii_speed 0000001a
fec_mdio_write: phy: 04 reg:00 val:0x8000
fec_mdio_read: phy: 04 reg:00 val:0x0
++mx6_rgmii_rework
fec_mdio_write: phy: 04 reg:0d val:0x7
fec_mdio_write: phy: 04 reg:0e val:0x8016
fec_mdio_write: phy: 04 reg:0d val:0x4007
fec_mdio_write: phy: 04 reg:0e val:0x18
fec_mdio_write: phy: 04 reg:1d val:0x5
fec_mdio_write: phy: 04 reg:1e val:0x3d47
--mx6_rgmii_rework
++ar8031_config
fec_mdio_read: phy: 04 reg:04 val:0x1de1
fec_mdio_write: phy: 04 reg:04 val:0x11e1
fec_mdio_read: phy: 04 reg:01 val:0x7949
fec_mdio_read: phy: 04 reg:09 val:0x200
fec_mdio_write: phy: 04 reg:09 val:0x300
fec_mdio_read: phy: 04 reg:00 val:0x0
fec_mdio_write: phy: 04 reg:00 val:0x1200
fec_mdio_read: phy: 04 reg:00 val:0x1000
fec_mdio_write: phy: 04 reg:00 val:0x1200
--ar8031_config
FEC [PRIME]
Error: FEC address not set.




>>>
>>> Hmm, so in effect you are forced to use the magic value of 0x3c47 as the
>>> reset default when actually it is 0x100 on your board. And there is no
>>> backing public documentation on why the reset default should be 0x3c47.
>>> And whether it will work for all boards that use this PHY.
>>
>> Well that's quite unfortunate.
>>
>>> Thats pretty unmaintainable in my opinion. If this value does not work
>>> for the next person that comes along, we will be in trouble again.
>>>
>>> I guess the best thing that can be done at this point is to use this
>>> magic reset default value in a board specific way. By specifying it
>>> using device-tree, perhaps. I would wait for some feedback from Joe
>>> Hershberger though.
>>
>> I think we can wait until someone else says they have a problem with
>> this value before making it a board option.
>>

Sorry for causing the confusion.
Our board specific requirement(issue) for setting proper register
value could be done in the board file.
I resubmitted the patch at
https://lists.denx.de/pipermail/u-boot/2017-February/281894.html


>>> BTW, do you have the same problem in kernel ? Because AFAICS, even
>>> drivers/net/phy/at803x.c in kernel does not have any such provision for
>>> magic reset default values.
>>
>> I'm interested to know this too. Is the kernel depending on the
>> bootloader to set this and it just avoids changing it?
>>
>
> Yes, I think so.Here are the code snippets in drivers/net/phy/at803x.c.
> I found it wouldn't change the bootloader setting when I dumped the
> value in kernel.
>
> static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
>                                  u16 clear, u16 set)
> {
>         u16 val;
>         int ret;
>
>         ret = at803x_debug_reg_read(phydev, reg);
>         if (ret < 0)
>                 return ret;
>
>         val = ret & 0xffff;
>         val &= ~clear;
>         val |= set;
>
>         return phy_write(phydev, AT803X_DEBUG_DATA, val);
> }
>
>
> static inline int at803x_enable_tx_delay(struct phy_device *phydev)
> {
>         return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
>                                         AT803X_DEBUG_TX_CLK_DLY_EN);
> }
>
>
>
>

>>>
>>> Thanks,
>>> Sekhar
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot@lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
diff mbox

Patch

diff --git a/drivers/net/phy/atheros.c b/drivers/net/phy/atheros.c
index b34cdd3d87dc..5c0a36676ce9 100644
--- a/drivers/net/phy/atheros.c
+++ b/drivers/net/phy/atheros.c
@@ -28,12 +28,18 @@  static int ar8021_config(struct phy_device *phydev)
 
 static int ar8031_config(struct phy_device *phydev)
 {
+       int regval;
+
        if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
            phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
                phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_ADDR_REG,
                          AR803x_DEBUG_REG_5);
+               regval = phy_read(phydev, MDIO_DEVAD_NONE,
+                                 AR803x_PHY_DEBUG_DATA_REG);
+               printf("%s: value read back 0x%x, written: 0x%x\n",
+                               __func__, regval, regval | AR803x_RGMII_TX_CLK_DLY);
                phy_write(phydev, MDIO_DEVAD_NONE, AR803x_PHY_DEBUG_DATA_REG,
-                         AR803x_RGMII_TX_CLK_DLY);
+                         regval | AR803x_RGMII_TX_CLK_DLY);
        }
 
        if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||