Hi Michal,
On Wed, Apr 28, 2021 at 11:03 PM Michal Simek <michal.si...@xilinx.com> wrote:
>
> 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.si...@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.si...@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@ff0e0000
> >> Scanning disk m...@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@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@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?
I created a test case below, and reproduced the endless loop you
mentioned, but on 2021.04, IOW, without my OF API change patch.
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 2600360224..8543fbe497 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -1360,6 +1360,18 @@
mdio: mdio-test {
compatible = "sandbox,mdio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy: ethernet-phy@0 {
+ reg = <0>;
+ };
+
+ gmiitorgmii: gmiitorgmii@8 {
+ compatible = "xlnx,gmii-to-rgmii-1.0";
+ reg = <8>;
+ phy-handle = <&phy>;
+ };
};
pm-bus-test {
diff --git a/test/dm/mdio.c b/test/dm/mdio.c
index 64347e1275..19fa5a01e9 100644
--- a/test/dm/mdio.c
+++ b/test/dm/mdio.c
@@ -25,8 +25,9 @@
static int dm_test_mdio(struct unit_test_state *uts)
{
struct uclass *uc;
- struct udevice *dev;
+ struct udevice *dev, *eth_dev;
struct mdio_ops *ops;
+ struct phy_device *phy;
u16 reg;
ut_assertok(uclass_get(UCLASS_MDIO, &uc));
@@ -52,6 +53,10 @@ static int dm_test_mdio(struct unit_test_state *uts)
SANDBOX_PHY_REG);
ut_asserteq(reg, 0);
+ ut_assertok(uclass_first_device(UCLASS_ETH, ð_dev));
+ phy = dm_mdio_phy_connect(dev, 0, eth_dev, PHY_INTERFACE_MODE_MII);
+ ut_assertnonnull(phy);
+
return 0;
}
So this means the original fdtdec_ version codes also have an issue,
but I have no idea why you did not encounter such an issue before.
> 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.
>
Regards,
Bin
