[PATCH v1 1/2] net: phy: Let gmiitorgmii converter create additional PHY

[Michal Simek]

On 1/22/25 08:31, Tapio Reijonen wrote:
> Hi Michal,
> 
> On 2025-01-21 15:14, Michal Simek wrote:
>> [You don't often get email from monstr at monstr.eu. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Hi,
>>
>> čt 16. 1. 2025 v 14:07 odesílatel Tapio Reijonen
>> <tapio.reijonen at vaisala.com> napsal:
>>>
>>> Change the order to connect gmiitorgmii before PHY creation.
>>> The gmiitorgmii create additional in DTS configured PHY during
>>> it's configuration. This ensures, that converter sits between the
>>> MAC and the external phy
>>> MAC <==> GMII2RGMII <==> RGMII_PHY.
>>>
>>> Fixes: commit a744a284e354 ("net: phy: Add support for ethernet-phy-id with gpio reset")
>>
>>  From format. Look at link in 2/2.
> 
> Fixing format in next version.
> 
>>
>>> Signed-off-by: Tapio Reijonen <tapio.reijonen at vaisala.com>
>>> ---
>>>
>>>   drivers/net/phy/phy.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index 716a1d46111..740533adeca 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -939,14 +939,14 @@ struct phy_device *phy_connect(struct mii_dev *bus, int addr,
>>>                  phydev = phy_device_create(bus, 0, PHY_NCSI_ID, false);
>>>   #endif
>>>
>>> -#ifdef CONFIG_PHY_ETHERNET_ID
>>> +#ifdef CONFIG_PHY_XILINX_GMII2RGMII
>>>          if (!phydev)
>>> -               phydev = phy_connect_phy_id(bus, dev, addr);
>>> +               phydev = phy_connect_gmii2rgmii(bus, dev);
>>>   #endif
>>>
>>> -#ifdef CONFIG_PHY_XILINX_GMII2RGMII
>>> +#ifdef CONFIG_PHY_ETHERNET_ID
>>>          if (!phydev)
>>> -               phydev = phy_connect_gmii2rgmii(bus, dev);
>>> +               phydev = phy_connect_phy_id(bus, dev, addr);
>>>   #endif
>>>
>>>          if (!phydev)
>>> --
>>> 2.39.5
>>>
>>
>> I can't see any issue with this swap but I would like to check your DT
>> description first.
>> Can you please share your DT fragment which describes this?
> 
> Here is our board's DT fragment:
> &gem1 {
> 	status = "okay";
> 	phy-handle = <&phy1>;
> 	phy-mode = "rgmii-id";
> 
> 	mdio {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		phy1: ethernet-phy at 1 {
> 			reg = <1>;
> 			#phy-cells = <1>;
> 			compatible = "ethernet-phy-id2000.a231";
> 			reset-assert-us = <10>;
> 			reset-deassert-us = <400>;
> 			reset-gpios = <&gpio0 54 GPIO_ACTIVE_LOW>;
> 			ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> 			ti,tx-internal-delay = <DP83867_RGMIIDCTL_2_50_NS>;
> 			ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> 		};
> 
> 		gmii_to_rgmii_0: gmiitorgmii at 8 {
> 			reg = <8>;
> 			compatible = "xlnx,gmii-to-rgmii-1.0";
> 			phy-handle = <&phy1>;
> 		};
> 	};
> };

That's what I was expecting that you have and it is not your fault.
I have been in touch with our team in connection to proper description of this 
situation and description above is unfortunately what Linux expects but it is 
not correct.

Description should be
gem - phy_handle points to bridge and bridge point to phy

what you have above that gem and bridge are pointing to the same phy.

As far as I know there was no way in Linux kernel to describe it like this 
that's why dt binding for linux kernel is different. (And by purpose Linux dt 
binding is not describing this connection from IP).

It means I am fine with the patch above which let kernel binding to work but 
please describe this reason in commit message.

Thanks,
Michal