| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7fc15ef7-ab5d-4af7-8d9b-bf0928f476a0@lunn.ch>
Date: Tue, 13 Jun 2023 16:14:46 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Daniel Golle <daniel@...rotopia.org>
Cc: netdev@...r.kernel.org, linux-mediatek@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Matthias Brugger <matthias.bgg@...il.com>,
Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>,
Russell King <linux@...linux.org.uk>,
Heiner Kallweit <hkallweit1@...il.com>,
SkyLake Huang <SkyLake.Huang@...iatek.com>,
Qingfang Deng <dqfext@...il.com>
Subject: 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:
> > > +/* 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
Powered by blists - more mailing lists