[PATCH v4 06/22] net: phy: xilinx: Be compatible with live OF tree
Michal Simek
monstr at monstr.eu
Fri Apr 23 13:52:03 CEST 2021
Hi,
On 4/23/21 11:23 AM, Bin Meng wrote:
> On Fri, Apr 23, 2021 at 5:14 PM Bin Meng <bmeng.cn at gmail.com> wrote:
>>
>> Hi Michal,
>>
>> On Fri, Apr 23, 2021 at 3:35 PM Michal Simek <monstr at monstr.eu> wrote:
>>>
>>> Hi Bin,
>>>
>>> ne 14. 3. 2021 v 13:17 odesílatel Bin Meng <bmeng.cn at gmail.com> napsal:
>>>>
>>>> Following the same updates that were done to the fixed phy driver,
>>>> use ofnode_ APIs instead of fdt_ APIs so that the Xilinx PHY driver
>>>> can support live DT.
>>>>
>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>>
>>>> ---
>>>>
>>>> (no changes since v2)
>>>>
>>>> Changes in v2:
>>>> - move device tree parsing from xilinxgmiitorgmii_probe() to
>>>> xilinxgmiitorgmii_config() and use OF APIs
>>>>
>>>> drivers/net/phy/phy.c | 23 +++++------
>>>> drivers/net/phy/xilinx_gmii2rgmii.c | 61 ++++++++++++++---------------
>>>> 2 files changed, 40 insertions(+), 44 deletions(-)
>>>>
>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>>> index eae40cc0d6..d464379121 100644
>>>> --- a/drivers/net/phy/phy.c
>>>> +++ b/drivers/net/phy/phy.c
>>>> @@ -953,23 +953,20 @@ static struct phy_device *phy_connect_gmii2rgmii(struct mii_dev *bus,
>>>> #endif
>>>> {
>>>> struct phy_device *phydev = NULL;
>>>> - int sn = dev_of_offset(dev);
>>>> - int off;
>>>> -
>>>> - while (sn > 0) {
>>>> - off = fdt_node_offset_by_compatible(gd->fdt_blob, sn,
>>>> - "xlnx,gmii-to-rgmii-1.0");
>>>> - if (off > 0) {
>>>> - phydev = phy_device_create(bus, off,
>>>> + ofnode node = dev_ofnode(dev);
>>>> +
>>>> + while (ofnode_valid(node)) {
>>>> + node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0");
>>>> + if (ofnode_valid(node)) {
>>>> + phydev = phy_device_create(bus, 0,
>>>> PHY_GMII2RGMII_ID, false,
>>>> interface);
>>>> + if (phydev)
>>>> + phydev->node = node;
>>>> break;
>>>> }
>>>> - if (off == -FDT_ERR_NOTFOUND)
>>>> - sn = fdt_first_subnode(gd->fdt_blob, sn);
>>>> - else
>>>> - printf("%s: Error finding compat string:%d\n",
>>>> - __func__, off);
>>>> +
>>>> + node = ofnode_first_subnode(node);
>>>> }
>>>>
>>>> return phydev;
>>>> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
>>>> index 74105c0b7d..635c0570ef 100644
>>>> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
>>>> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
>>>> @@ -18,9 +18,38 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> static int xilinxgmiitorgmii_config(struct phy_device *phydev)
>>>> {
>>>> - struct phy_device *ext_phydev = phydev->priv;
>>>> + ofnode node = phy_get_ofnode(phydev);
>>>> + struct phy_device *ext_phydev;
>>>> + struct ofnode_phandle_args phandle;
>>>> + int ext_phyaddr = -1;
>>>> + int ret;
>>>>
>>>> debug("%s\n", __func__);
>>>> +
>>>> + if (!ofnode_valid(node))
>>>> + return -EINVAL;
>>>> +
>>>> + phydev->addr = ofnode_read_u32_default(node, "reg", -1);
>>>> + ret = ofnode_parse_phandle_with_args(node, "phy-handle",
>>>> + NULL, 0, 0, &phandle);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ext_phyaddr = ofnode_read_u32_default(phandle.node, "reg", -1);
>>>> + ext_phydev = phy_find_by_mask(phydev->bus,
>>>> + 1 << ext_phyaddr,
>>>> + PHY_INTERFACE_MODE_RGMII);
>>>> + if (!ext_phydev) {
>>>> + printf("%s, No external phy device found\n", __func__);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ext_phydev->node = phandle.node;
>>>> + phydev->priv = ext_phydev;
>>>> +
>>>> + debug("%s, gmii2rgmmi:0x%x, extphy:0x%x\n", __func__, phydev->addr,
>>>> + ext_phyaddr);
>>>> +
>>>> if (ext_phydev->drv->config)
>>>> ext_phydev->drv->config(ext_phydev);
>>>>
>>>> @@ -83,11 +112,6 @@ static int xilinxgmiitorgmii_startup(struct phy_device *phydev)
>>>>
>>>> static int xilinxgmiitorgmii_probe(struct phy_device *phydev)
>>>> {
>>>> - int ofnode = phydev->addr;
>>>> - u32 phy_of_handle;
>>>> - int ext_phyaddr = -1;
>>>> - struct phy_device *ext_phydev;
>>>> -
>>>> debug("%s\n", __func__);
>>>>
>>>> if (phydev->interface != PHY_INTERFACE_MODE_GMII) {
>>>> @@ -95,31 +119,6 @@ static int xilinxgmiitorgmii_probe(struct phy_device *phydev)
>>>> return -EINVAL;
>>>> }
>>>>
>>>> - /*
>>>> - * Read the phy address again as the one we read in ethernet driver
>>>> - * was overwritten for the purpose of storing the ofnode
>>>> - */
>>>> - phydev->addr = fdtdec_get_int(gd->fdt_blob, ofnode, "reg", -1);
>>>> - phy_of_handle = fdtdec_lookup_phandle(gd->fdt_blob, ofnode,
>>>> - "phy-handle");
>>>> - if (phy_of_handle > 0)
>>>> - ext_phyaddr = fdtdec_get_int(gd->fdt_blob,
>>>> - phy_of_handle,
>>>> - "reg", -1);
>>>> - ext_phydev = phy_find_by_mask(phydev->bus,
>>>> - 1 << ext_phyaddr,
>>>> - PHY_INTERFACE_MODE_RGMII);
>>>> - if (!ext_phydev) {
>>>> - printf("%s, No external phy device found\n", __func__);
>>>> - return -EINVAL;
>>>> - }
>>>> -
>>>> - ext_phydev->node = offset_to_ofnode(phy_of_handle);
>>>> - phydev->priv = ext_phydev;
>>>> -
>>>> - debug("%s, gmii2rgmmi:0x%x, extphy:0x%x\n", __func__, phydev->addr,
>>>> - ext_phyaddr);
>>>> -
>>>> phydev->flags |= PHY_FLAG_BROKEN_RESET;
>>>>
>>>> return 0;
>>>> --
>>>> 2.25.1
>>>>
>>>
>>> This patch is breaking u-boot on ZynqMP. In phy_connect_gmii2rgmii you
>>> get a valid node from dev_ofnode(dev)
>>> and you enter the while loop because ofnode_valid(node) returns true.
>>> ofnode_by_compatible() will fail because it is not normally
>>> gmii-to-rgmii bridge and will go to ofnode_first_subnode() which
>>> returns 0
>
> A second thought, I am curious why ofnode_first_subnode() returns a
> valid ofnode if this fails? Is there a DT example?
>
> As ofnode_for_each_subnode() also uses ofnode_valid() to check.
>
>>> which goes back to ofnode_valid() which pass again because 0 is valid
>>> and it keeps going in this while loop.
>>> This is with xilinx_zynqmp_virt_defconfig configuration with OF_LIVE
>>> off but with OF_LIVE on behavior is the same.
>>>
>>> It means that the while loop should be replaced by something more deterministic.
>>> What about?
>>> ofnode_for_each_subnode(node, dev_ofnode(dev)) {
>>
>> This sounds good to me.
>>
>> I cannot find an in-tree DTS that contains the compatible string
>> "xlnx,gmii-to-rgmii-1.0". Is this an out-of-tree DT?
dt binding is here.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/xilinx_gmii2rgmii.txt?h=v5.12-rc8
Also with example.
And I expect this can work fine when node is there.
You should be able just to enable that driver and see it. I am using
zcu104. Dt is in the tree.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
More information about the U-Boot
mailing list