[PATCH] net: phy: xilinx: Break while loop over ethernet phy
Michal Simek
michal.simek at xilinx.com
Wed Apr 28 17:03:43 CEST 2021
Hi Bin,
On 4/28/21 4:37 PM, Bin Meng wrote:
> Hi Michal,
>
> On Tue, Apr 27, 2021 at 7:22 PM Michal Simek <michal.simek at xilinx.com> wrote:
>>
>> Hi Bin,
>>
>> On 4/27/21 7:17 AM, Bin Meng wrote:
>>> Hi Michal,
>>>
>>> On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek at xilinx.com> wrote:
>>>>
>>>> The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF
>>>> tree") change driver behavior to while loop which wasn't correct because
>>>> the driver was looping over again and again. The reason was that
>>>> ofnode_valid() is taking 0 as correct value.
>>>
>>> I am still trying to understand the problem. The changes in
>>> 6c993815bbea sound correct from an fdtdec <=> OF API mapping
>>> perspective. If the new OF API does not work, the old fdtdec may fail
>>> too. Could you please explain a little bit?
>>
>> here is behavior of origin code.
>>
>> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id
>> phy_connect_gmii2rgmii sn 11348
>> phy_connect_gmii2rgmii 1off -1
>> phy_connect_gmii2rgmii 2off -1
>> phy_connect_gmii2rgmii sn2 11752
>> phy_connect_gmii2rgmii 1off -1
>> phy_connect_gmii2rgmii 2off -1
>> phy_connect_gmii2rgmii sn2 -1
>> phy_connect_gmii2rgmii phydev 0000000000000000
>> eth0: ethernet at ff0e0000
>> Scanning disk mmc at ff170000.blk...
>> Found 4 disks
>> Hit any key to stop autoboot: 0
>> ZynqMP>
>>
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 89e3076bfd25..d0960d93ae08 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -956,22 +956,28 @@ static struct phy_device
>> *phy_connect_gmii2rgmii(struct mii_dev *bus,
>> int sn = dev_of_offset(dev);
>> int off;
>>
>> + printf("%s sn %d\n", __func__, sn);
>> while (sn > 0) {
>> off = fdt_node_offset_by_compatible(gd->fdt_blob, sn,
>>
>> "xlnx,gmii-to-rgmii-1.0");
>> + printf("%s 1off %d\n", __func__, off);
>> if (off > 0) {
>> phydev = phy_device_create(bus, off,
>> PHY_GMII2RGMII_ID, false,
>> interface);
>> break;
>> }
>> - if (off == -FDT_ERR_NOTFOUND)
>> + printf("%s 2off %d\n", __func__, off);
>> +
>> + if (off == -FDT_ERR_NOTFOUND) {
>> sn = fdt_first_subnode(gd->fdt_blob, sn);
>> - else
>> + printf("%s sn2 %d\n", __func__, sn);
>> + } else
>> printf("%s: Error finding compat string:%d\n",
>> __func__, off);
>> }
>>
>> + printf("%s phydev %p\n", __func__, phydev);
>> return phydev;
>> }
>> #endif
>>
>>
>> With latest code and some prints
>> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id
>> ofnode_valid: node.of_offset 11348
>> ofnode_valid: node.of_offset 11348
>> ofnode_valid: node.of_offset 11348
>> ofnode_valid: node.of_offset 2952
>> ofnode_valid: node.of_offset 11348
>> ofnode_valid: node.of_offset -1
>> ofnode_valid: node.of_offset 11348
>> ofnode_valid: node.of_offset 11348
>> phy_connect_gmii2rgmii 1valid 1 ethernet at ff0e0000
>> ofnode_valid: node.of_offset 11348
>> ofnode_valid: node.of_offset 11348
>> ofnode_valid: node.of_offset 11348
>> phy_connect_gmii2rgmii 2valid 1 ethernet at ff0e0000
>> ofnode_valid: node.of_offset -1
>> ofnode_valid: node.of_offset -1
>> ofnode_valid: node.of_offset 0
>> ofnode_valid: node.of_offset 0
>> phy_connect_gmii2rgmii 3valid 1
>> ofnode_valid: node.of_offset 0
>> ofnode_valid: node.of_offset 0
>> ofnode_valid: node.of_offset 0
>> phy_connect_gmii2rgmii 2valid 1
>> ofnode_valid: node.of_offset -1
>> ofnode_valid: node.of_offset -1
>> ofnode_valid: node.of_offset 0
>> ofnode_valid: node.of_offset 0
>> phy_connect_gmii2rgmii 3valid 1
>> ...
>>
>>
>> [u-boot](debian-sent)$ git diff
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index dcdef9e661d6..56072ad55216 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -950,7 +950,10 @@ static struct phy_device
>> *phy_connect_gmii2rgmii(struct mii_dev *bus,
>> struct phy_device *phydev = NULL;
>> ofnode node = dev_ofnode(dev);
>>
>> + printf("%s 1valid %d %s\n", __func__, ofnode_valid(node),
>> ofnode_get_name(node));
>> +
>> while (ofnode_valid(node)) {
>> + printf("%s 2valid %d %s\n", __func__,
>> ofnode_valid(node), ofnode_get_name(node));
>> node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0");
>> if (ofnode_valid(node)) {
>> phydev = phy_device_create(bus, 0,
>> @@ -962,6 +965,7 @@ static struct phy_device
>> *phy_connect_gmii2rgmii(struct mii_dev *bus,
>> }
>>
>> node = ofnode_first_subnode(node);
>> + printf("%s 3valid %d %s\n", __func__,
>> ofnode_valid(node), ofnode_get_name(node));
>> }
>>
>> return phydev;
>>
>>
>> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
>> index 2c0597c40739..7bfc06165e92 100644
>> --- a/include/dm/ofnode.h
>> +++ b/include/dm/ofnode.h
>> @@ -128,8 +128,10 @@ static inline bool ofnode_valid(ofnode node)
>> {
>> if (of_live_active())
>> return node.np != NULL;
>> - else
>> + else {
>> + printf("ofnode_valid: node.of_offset %ld\n",
>> node.of_offset);
>> return node.of_offset >= 0;
>> + }
>> }
>>
>> /**
>>
>>
>> It means ofnode_first_subnode is returning any node which has
>> node.of_offset == 0 which is consider based on ofnode_valid() as valid
>
> This sounds suspicious to me that ofnode_first_subnode() returns a
> node with node.of_offset being zero. I think it should return 11752?
Gem driver is pointing via phy-handle directly to now which should be
used. That's why there is no subnode but maybe it can be.
Do you have any idea how to debug this to see that node content for
OF_LIVE and !OF_LIVE?
Anyway I need to get this fix sooner rather than later. One option is
this patch and the second is disable this bridge for now.
Thanks,
Michal
More information about the U-Boot
mailing list