Re: [PATCH net-next] net: phy: mediatek-ge-soc: initialize MT7988 PHY LEDs default state

On Tue, Jun 13, 2023 at 02:13:28PM +0100, Daniel Golle wrote:

Hi Andrew,

On Tue, Jun 13, 2023 at 05:23:25AM +0200, Andrew Lunn wrote:

quoted

quoted

+/* Registers on MDIO_MMD_VEND2 */
+#define MTK_PHY_LED0_ON_CTRL			0x24
+#define MTK_PHY_LED1_ON_CTRL			0x26
+#define   MTK_PHY_LED_ON_MASK			GENMASK(6, 0)
+#define   MTK_PHY_LED_ON_LINK1000		BIT(0)
+#define   MTK_PHY_LED_ON_LINK100		BIT(1)
+#define   MTK_PHY_LED_ON_LINK10			BIT(2)
+#define   MTK_PHY_LED_ON_LINKDOWN		BIT(3)
+#define   MTK_PHY_LED_ON_FDX			BIT(4) /* Full duplex */
+#define   MTK_PHY_LED_ON_HDX			BIT(5) /* Half duplex */
+#define   MTK_PHY_LED_FORCE_ON			BIT(6)
+#define   MTK_PHY_LED_POLARITY			BIT(14)
+#define   MTK_PHY_LED_ENABLE			BIT(15)

The PHY has two LED outputs, LED0 and LED1. Both have an ON_CTRL and
a BLINK_CTRL register each with identical layouts for LED0_ON_CTRL and
LED1_ON_CTRL as well as LED0_BLINK_CTRL as well as LED1_BLINK_CTRL.

 
O.K. So i think the naming could be better

#define MTK_PHY_LED0_ON_CTRL			0x24
#define MTK_PHY_LED1_ON_CTRL			0x26
#define   MTK_PHY_LEDX_ON_CTRL_MASK		GENMASK(6, 0)
#define   MTK_PHY_LEDX_ON_CTRL_LINK1000		BIT(0)
#define   MTK_PHY_LEDX_ON_CTRL_LINK100		BIT(1)

#define MTK_PHY_LED0_BLINK_CTRL			0x25
#define MTK_PHY_LED1_BLINK_CTRL			0x27
#define   MTK_PHY_LEDX_BLINK_CTRL_1000TX	BIT(0)
#define   MTK_PHY_LEDX_BLINK_CTRL_1000RX	BIT(1)

I've taken over code where i found a few examples of a register write
which used the wrong macro for bits because there was no clear
indication which register is belonged to.

	   phy_write(phydev, MTK_PHY_LED1_ON_CTRL,
	   	     MTK_PHY_LEDX_BLINK_CTRL_1000TX |
	             MTK_PHY_LEDX_BLINK_CTRL_1000RX)

is pretty obvious it is wrong.

The LED controller of both LEDs are identical. The SoC's pinmux assigns
LED0 as LED_A, LED_B, LED_C and LED_D respectively. And those pins are
used for bootstrapping board configuration bits, and that then implies
the polarity of the connected LED.

Ie.
-----------------------------+ SoC pin
--------------+-------+      |
       port 0 = PHY 0 - LED0 - LED_A
              +-------\ LED1 - JTAG_JTDI
       port 1 = PHY 1 - LED0 - LED_B
MT7530        +-------\ LED1 - JTAG_JTDO
       port 2 = PHY 2 - LED0 - LED_C
              +-------\ LED1 - JTAG_JTMS
       port 3 = PHY 3 - LED0 - LED_D
--------------+-------\ LED1 - JTAG_JTCLK
       2P5G PHY       - LED0 - LED_E
----------------------\ LED1 - JTAG_JTRST_N
--------------+--------------+

So you can skip the JTAG interface and have two LEDs. This is purely
down to pinmux settings. In fact, with careful design, it might be
possible to have both LEDs and JTAG.

quoted

quoted

 #define MTK_PHY_RG_BG_RASEL			0x115
 #define   MTK_PHY_RG_BG_RASEL_MASK		GENMASK(2, 0)
 
+/* Register in boottrap syscon defining the initial state of the 4 PHY LEDs */

Should this be bootstrap? You had the same in the commit message.

Also, i'm confused. Here you say 4 PHY LEDs, yet

quoted

+#define MTK_PHY_LED0_ON_CTRL			0x24
+#define MTK_PHY_LED1_ON_CTRL			0x26

suggests there are two LEDs?

There are 4 PHYs with two LEDs each. Only LED0 of each PHY is using
a pin which is used for bootstrapping, hence LED polarity is known
only for those, polarity of LED1 is unknown and always set the default
at this point.

So hopefully you can see my sources of confusion and can add some
comments to make this clearer.

quoted

quoted

+static int mt798x_phy_setup_led(struct phy_device *phydev, bool inverted)
+{
+	struct pinctrl *pinctrl;
+	const u16 led_on_ctrl_defaults = MTK_PHY_LED_ENABLE      |
+					 MTK_PHY_LED_ON_LINK1000 |
+					 MTK_PHY_LED_ON_LINK100  |
+					 MTK_PHY_LED_ON_LINK10;
+	const u16 led_blink_defaults = MTK_PHY_LED_1000TX |
+				       MTK_PHY_LED_1000RX |
+				       MTK_PHY_LED_100TX  |
+				       MTK_PHY_LED_100RX  |
+				       MTK_PHY_LED_10TX   |
+				       MTK_PHY_LED_10RX;
+
+	phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_ON_CTRL,
+		      led_on_ctrl_defaults ^
+		      (inverted ? MTK_PHY_LED_POLARITY : 0));
+
+	phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_ON_CTRL,
+		      led_on_ctrl_defaults);
+

Now i'm even more confused. Both have the same value, expect the
polarity bit?

Only LED0 polarity of each PHY is affected by the bootstrap pins
LED_A, LED_B, LED_C and LED_D of the SoC, see above.
Hence we need to XOR polarity only for LED0.

However, if there are two LEDs, you are likely to want to configure
them to show different things, not identical. You are setting the
defaults here which all boards will get. So i would set LED1 to
something different, something which makes sense when there are two
LEDs.

quoted

quoted

+	pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "gbe-led");

This is also very unusual. At minimum, it needs a comment as to why it
is needed. But more likely, because no other driver in driver/net does
this, it makes me think it is wrong.

The reason for not using the "default" pinctrl name is simply that then
the "default" state will already be requested by device model *before*
the LED registers are actually setup. This results in a short but unwanted
blink of the LEDs during setup.
Requesting the pinctrl state only once the setup is done allows avoiding
that, but of course this is of purely aesthetic nature.

So i assume the pin can have other functions? Just for an example, say
it can also be an I2C clock line, which might be connected to an
SFP. The I2C node in DT will have a pinmux to configure the pin for
I2C. What happens when the PHY driver loads and executes this?

     Andrew