[U-Boot] [PATCH] drivers: net: phy: atheros: apply the previous register setting for AR8031 to fix the voltage peak issue
Yung-Ching LIN
yungching0725 at gmail.com
Fri Mar 3 01:58:12 UTC 2017
On Wed, Feb 8, 2017 at 5:35 PM, Yung-Ching LIN <yungching0725 at gmail.com> wrote:
> On Wed, Feb 8, 2017 at 10:47 AM, Joe Hershberger
> <joe.hershberger at gmail.com> wrote:
>> On Wed, Feb 8, 2017 at 12:26 AM, Sekhar Nori <nsekhar at 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 at 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 at 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 at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list