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

[Andrew Lunn]

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:
> > > +/* 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.

> > >  #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
> > 
> > > +#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.

> > > +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.

> > > +	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