Message-ID: <DS7PR10MB49260FFA60F0B3A5AB7AD69EFA879@DS7PR10MB4926.namprd10.prod.outlook.com>
Date:   Thu, 23 Mar 2023 08:02:43 +0000
From:   "Buzarra, Arturo" <Arturo.Buzarra@...i.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Heiner Kallweit <hkallweit1@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Subject: RE: [PATCH] net: phy: return EPROBE_DEFER if PHY is not accessible

Hi,

" You have two MACs. Do you have two MDIO busses, with one PHY on each bus, or do you have just one MDIO bus in use, with two PHYs on it?"
I have two Ethernet MACs, with one MDIO bus connected to two different PHYs

"There is a pull up on the MDIO data line, so that if nothing drives it low, it reads 1. This was one of Florian comments. Have you check the value of that pull up resistor?"
Yes, but with/without this pull-up we obtain the same behavior

"Which is odd, because the MDIO bus probe code would assume there is no PHY there given those two values, and then would not bother trying to read the abilities. So you are somehow forcing the core to assume there is a PHY there. Do you have the PHY ID in DT?"
Yes, we have both PHYs defined in the DT

"Where is the clock coming from? Does each PHY have its own crystal? Is the clock coming from one of the MACs? Is one PHY a clock source and the other a clock sync?"
Gigabit PHY has its own Crystal, however the 10/100 PHY uses a clock from the CPU and it is the root cause of the issue because when Linux asks the PHY capabilities the clock is not ready yet.

"We first want to understand the problem before adding such hacks. It really sounds like something the PHY needs is missing, a clock, time after a reset, etc. If we can figure that out, we can avoid such hacks"
We have identified the root cause of the 0xFFFF issue, it is because the two PHYs are defined in the same MDIO bus node inside the first Ethernet MAC node, and when the 10/100 PHY is probed the PHY Clock from the CPU is not ready, this is the DT definition:
---------
/* Gigabit Ethernet */
&eth1 {
	status = "okay";
	pinctrl-0 = <&eth1_rgmii_pins>;
	pinctrl-names = "default";
	phy-mode = "rgmii-id";
	max-speed = <1000>;
	phy-handle = <&phy0_eth1>;

	mdio1 {
		#address-cells = <1>;
		#size-cells = <0>;
		compatible = "snps,dwmac-mdio";

		phy0_eth1: ethernet-phy@0 {
			reg = <0>;
			compatible = "ethernet-phy-id0141.0dd0"; /* PHY ID for Marvell 88E1512 */
			reset-gpios = <&gpioi 2 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
			reset-assert-us = <1000>;
			reset-deassert-us = <2000>;
		};

		phy0_eth2: ethernet-phy@1 {
			reg = <1>;
			compatible = "ethernet-phy-id0007.c0f0"; /* PHY ID for SMSC LAN8720Ai */
			reset-gpios = <&gpioh 7 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
			interrupt-parent = <&gpioh>;
			interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
		};
	};
};

/* 10/100 Ethernet */
&eth2 {
	status = "okay";
	pinctrl-0 = <&eth2_rmii_pins>;
	pinctrl-names = "default";
	phy-mode = "rmii";
	max-speed = <100>;
	phy-handle = <&phy0_eth2>;
	st,ext-phyclk;
};
---------
This is the power-up sequence:
- ST MAC driver (dwmac-stm32.c) initializes the first Ethernet interface in RGMII mode
- Linux kernel perform the Gigabit PHY probe
- Linux kernel perform the 10/100 PHY probe ( and reads a wrong value from the MII_BMSR register because the PHY clock from the CPU is not ready)
- ST MAC driver (dwmac-stm32.c) initializes the second Ethernet interface in RMII mode (Here the CPU initializes the PHY Clock)

So the 10/100 PHY is probed BEFORE the PHY Clock is initialized.

In spite of this corner case issue that we have with our particular setup, I think that consider a 0x0000 or 0xFFFF read value as a valid value is wrong. For our issue I have several fixes: hardcoded the PHY capabilities in the smsc.c driver with " .features  = PHY_BASIC_FEATURES" , another fix is in the same driver adding a custom function for " .get_features" where if I read 0xFFFF or 0x0000 return a EPROBE_DEFER. But the real motivation to send that patch was that after review several drivers that also checks the result of read MII_BMSR against 0x0000 and 0xFFFF , I tried to send a common fix in the PHY core, to avoid maintain this verification in different drivers.

Thanks,

Arturo.

-----Original Message-----
From: Andrew Lunn <andrew@...n.ch> 
Sent: lunes, 20 de marzo de 2023 13:01
To: Buzarra, Arturo <Arturo.Buzarra@...i.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>; netdev@...r.kernel.org; Florian Fainelli <f.fainelli@...il.com>; Russell King - ARM Linux <linux@...linux.org.uk>
Subject: Re: [PATCH] net: phy: return EPROBE_DEFER if PHY is not accessible

[EXTERNAL E-MAIL] Warning! This email originated outside of the organization! Do not click links or open attachments unless you recognize the sender and know the content is safe.



On Mon, Mar 20, 2023 at 09:45:38AM +0000, Buzarra, Arturo wrote:
> Hi,
>
> I will try to answer all your questions:
>
> - "We need more specifics here, what type of PHY device are you seeing this with?"
> - " So best start with some details about your use case, which MAC, which PHY, etc"
> I'm using a LAN8720A PHY (10/100 on RMII mode) with a ST MAC ( in particular is a stm32mp1 processor).
> We have two PHYs one is a Gigabit PHY (RGMII mode) and the another one is a 10/100 (RMII mode).

Where is the clock coming from? Does each PHY have its own crystal? Is the clock coming from one of the MACs? Is one PHY a clock source and the other a clock sync?

> In the boot process, I think that there is a race condition between 
> configuring the Ethernet MACs and the two PHYs. At same point the 
> RGMII Ethernet MAC is configured and starts the PHY probes.

You have two MACs. Do you have two MDIO busses, with one PHY on each bus, or do you have just one MDIO bus in use, with two PHYs on it?

Please show us your Device Tree description of the hardware.

> When the 10/100 PHY starts the probe, it tries to read the
> genphy_read_abilities() and always reads 0xFFFF ( I assume that this 
> is the default electrical values for that lines before it are 
> configured).

There is a pull up on the MDIO data line, so that if nothing drives it low, it reads 1. This was one of Florian comments. Have you check the value of that pull up resistor?

> - " Does the device reliably enumerate on the bus, i.e. reading registers 2 and 3 to get its ID?"
> Reading the registers PHYSID1 and PHYSID2 reports also 0xFFFF

Which is odd, because the MDIO bus probe code would assume there is no PHY there given those two values, and then would not bother trying to read the abilities. So you are somehow forcing the core to assume there is a PHY there. Do you have the PHY ID in DT?

> - " If the PHY is broken, by some yet to be determined definition of broken, we try to limit the workaround to as narrow as possible. So it should not be in the core probe code. It should be in the PHY specific driver, and ideally for only its ID, not the whole vendors family of PHYs"
> I have several workarounds/fixed for that, the easy way is set the PHY capabilities in the smsc.c driver " .features  = PHY_BASIC_FEATURES" like in the past and it works fine. Also I have another fix in the same driver adding a custom function for " get_features" where if I read 0xFFFF or 0x0000 return a EPROBE_DEFER. However after review another drivers (net/usb/pegasus.c , net/Ethernet/toshiba/spider_net.c, net/sis/sis190.c, and more...)  that also checks the result of read MII_BMSR against 0x0000 and 0xFFFF , I tried to send a common fix in the PHY core. From my point of view read a 0x0000 or 0xFFFF value is an error in the probe step like if the phy_read() returns an error, so I think that the PHY core should consider return a EPROBE_DEFER to at least provide a second try for that PHY device.

We first want to understand the problem before adding such hacks. It really sounds like something the PHY needs is missing, a clock, time after a reset, etc. If we can figure that out, we can avoid such hacks.

        Andrew

Powered by blists - more mailing lists