Initialize LEDs and set sane default values.
Read boottrap register and apply LED polarities accordingly to get
uniform behavior from all LEDs on MT7988.
Requires syscon phandle 'mediatek,pio' present in parenting MDIO bus
which should point to the syscon holding the boottrap register.
Signed-off-by: Daniel Golle <[email protected]>
---
drivers/net/phy/mediatek-ge-soc.c | 144 ++++++++++++++++++++++++++++--
1 file changed, 136 insertions(+), 8 deletions(-)
diff --git a/drivers/net/phy/mediatek-ge-soc.c b/drivers/net/phy/mediatek-ge-soc.c
index 95369171a7ba6..7bdcb4415509f 100644
--- a/drivers/net/phy/mediatek-ge-soc.c
+++ b/drivers/net/phy/mediatek-ge-soc.c
@@ -1,11 +1,13 @@
// SPDX-License-Identifier: GPL-2.0+
#include <linux/bitfield.h>
+#include <linux/mfd/syscon.h>
#include <linux/module.h>
#include <linux/nvmem-consumer.h>
#include <linux/of_address.h>
#include <linux/of_platform.h>
#include <linux/pinctrl/consumer.h>
#include <linux/phy.h>
+#include <linux/regmap.h>
#define MTK_GPHY_ID_MT7981 0x03a29461
#define MTK_GPHY_ID_MT7988 0x03a29481
@@ -208,9 +210,40 @@
#define MTK_PHY_DA_TX_R50_PAIR_C 0x53f
#define MTK_PHY_DA_TX_R50_PAIR_D 0x540
+/* 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)
+
+#define MTK_PHY_LED0_BLINK_CTRL 0x25
+#define MTK_PHY_LED1_BLINK_CTRL 0x27
+#define MTK_PHY_LED_1000TX BIT(0)
+#define MTK_PHY_LED_1000RX BIT(1)
+#define MTK_PHY_LED_100TX BIT(2)
+#define MTK_PHY_LED_100RX BIT(3)
+#define MTK_PHY_LED_10TX BIT(4)
+#define MTK_PHY_LED_10RX BIT(5)
+#define MTK_PHY_LED_COLLISION BIT(6)
+#define MTK_PHY_LED_RX_CRC_ERR BIT(7)
+#define MTK_PHY_LED_RX_IDLE_ERR BIT(8)
+#define MTK_PHY_LED_FORCE_BLINK BIT(9)
+
#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 */
+#define RG_GPIO_MISC_TPBANK0 0x6f0
+#define RG_GPIO_MISC_TPBANK0_BOOTMODE GENMASK(11, 8)
+
/* These macro privides efuse parsing for internal phy. */
#define EFS_DA_TX_I2MPB_A(x) (((x) >> 0) & GENMASK(5, 0))
#define EFS_DA_TX_I2MPB_B(x) (((x) >> 6) & GENMASK(5, 0))
@@ -238,13 +271,6 @@ enum {
PAIR_D,
};
-enum {
- GPHY_PORT0,
- GPHY_PORT1,
- GPHY_PORT2,
- GPHY_PORT3,
-};
-
enum calibration_mode {
EFUSE_K,
SW_K
@@ -263,6 +289,10 @@ enum CAL_MODE {
SW_M
};
+struct mtk_socphy_shared {
+ u32 boottrap;
+};
+
static int mtk_socphy_read_page(struct phy_device *phydev)
{
return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
@@ -1073,6 +1103,104 @@ static int mt798x_phy_config_init(struct phy_device *phydev)
return mt798x_phy_calibration(phydev);
}
+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);
+
+ phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_BLINK_CTRL,
+ led_blink_defaults);
+
+ phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_BLINK_CTRL,
+ led_blink_defaults);
+
+ pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "gbe-led");
+ if (IS_ERR(pinctrl))
+ dev_err(&phydev->mdio.bus->dev, "Failed to setup PHY LED\n");
+
+ return 0;
+}
+
+static int mt7988_phy_probe_shared(struct phy_device *phydev)
+{
+ struct device_node *np = dev_of_node(&phydev->mdio.bus->dev);
+ struct mtk_socphy_shared *priv = phydev->shared->priv;
+ struct regmap *regmap;
+ u32 reg;
+ int ret;
+
+ /* The LED0 of the 4 PHYs in MT7988 are wired to SoC pins LED_A, LED_B,
+ * LED_C and LED_D respectively. At the same time those pins are used to
+ * bootstrap configuration of the reference clock source (LED_A),
+ * DRAM DDRx16b x2/x1 (LED_B) and boot device (LED_C, LED_D).
+ * In practise this is done using a LED and a resistor pulling the pin
+ * either to GND or to VIO.
+ * The detected value at boot time is accessible at run-time using the
+ * TPBANK0 register located in the gpio base of the pinctrl, in order
+ * to read it here it needs to be referenced by a phandle called
+ * 'mediatek,pio' in the MDIO bus hosting the PHY.
+ * The 4 bits in TPBANK0 are kept as package shared data and are used to
+ * set LED polarity for each of the LED0.
+ */
+ regmap = syscon_regmap_lookup_by_phandle(np, "mediatek,pio");
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ ret = regmap_read(regmap, RG_GPIO_MISC_TPBANK0, ®);
+ if (ret)
+ return ret;
+
+ priv->boottrap = FIELD_GET(RG_GPIO_MISC_TPBANK0_BOOTMODE, reg);
+
+ return 0;
+}
+
+static bool mt7988_phy_get_boottrap_polarity(struct phy_device *phydev)
+{
+ struct mtk_socphy_shared *priv = phydev->shared->priv;
+
+ if (priv->boottrap & BIT(phydev->mdio.addr))
+ return false;
+
+ return true;
+}
+
+static int mt7988_phy_probe(struct phy_device *phydev)
+{
+ int err;
+
+ err = devm_phy_package_join(&phydev->mdio.dev, phydev, 0,
+ sizeof(struct mtk_socphy_shared));
+ if (err)
+ return err;
+
+ if (phy_package_probe_once(phydev)) {
+ err = mt7988_phy_probe_shared(phydev);
+ if (err)
+ return err;
+ }
+
+ mt798x_phy_setup_led(phydev, mt7988_phy_get_boottrap_polarity(phydev));
+
+ return mt798x_phy_calibration(phydev);
+}
+
static struct phy_driver mtk_socphy_driver[] = {
{
PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7981),
@@ -1092,7 +1220,7 @@ static struct phy_driver mtk_socphy_driver[] = {
.config_init = mt798x_phy_config_init,
.config_intr = genphy_no_config_intr,
.handle_interrupt = genphy_handle_interrupt_no_ack,
- .probe = mt798x_phy_calibration,
+ .probe = mt7988_phy_probe,
.suspend = genphy_suspend,
.resume = genphy_resume,
.read_page = mtk_socphy_read_page,
--
2.41.0
> +/* 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)
Would enable being clear result in the LED being off? You can force it
on with MTK_PHY_LED_FORCE_ON | MTK_PHY_LED_ENABLE? That gives you
enough to allow software control of the LED. You can then implement
the led_brightness_set() op, if you want.
I assume the above are specific to LED0? It would be good to include
the 0 in the name, to make that clear.
> +
> +#define MTK_PHY_LED0_BLINK_CTRL 0x25
> +#define MTK_PHY_LED1_BLINK_CTRL 0x27
> +#define MTK_PHY_LED_1000TX BIT(0)
So do this mean LINK1000 + blink on TX ?
> +#define MTK_PHY_LED_1000RX BIT(1)
So do this mean LINK1000 + blink on RX ?
It would be good to add a comment, because at some point it is likely
somebody will want to offload the ledtrig-netdev and will need to
understand what these really do.
> +#define MTK_PHY_LED_100TX BIT(2)
> +#define MTK_PHY_LED_100RX BIT(3)
> +#define MTK_PHY_LED_10TX BIT(4)
> +#define MTK_PHY_LED_10RX BIT(5)
> +#define MTK_PHY_LED_COLLISION BIT(6)
> +#define MTK_PHY_LED_RX_CRC_ERR BIT(7)
> +#define MTK_PHY_LED_RX_IDLE_ERR BIT(8)
> +#define MTK_PHY_LED_FORCE_BLINK BIT(9)
Is there a way to force the LED1 off/on? I guess not setting any of
these bits will force it off.
So the polarity and enable bits apply to LED1?
> +
> #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?
Should these actually be :
> +#define MTK_PHY_LED_ON_CTRL1 0x24
> +#define MTK_PHY_LED_ON_CTRL2 0x26
meaning each LED has two control registers?
MTK_PHY_LED_ON_LINK1000 should actually be MTK_PHY_LED_ON_CTRL1_LINK1000 ?
MTK_PHY_LED_100TX should be MTK_PHY_LED_CTRL2_100TX ?
I find it good practice to ensure a bit value #define includes enough
information in its name to clear indicate what register it applies to.
> +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?
Please add a lot of comments about how this hardware actually works!
> + phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_BLINK_CTRL,
> + led_blink_defaults);
> +
> + phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_BLINK_CTRL,
> + led_blink_defaults);
> +
> + 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.
> +static bool mt7988_phy_get_boottrap_polarity(struct phy_device *phydev)
> +{
> + struct mtk_socphy_shared *priv = phydev->shared->priv;
> +
> + if (priv->boottrap & BIT(phydev->mdio.addr))
> + return false;
This can be simplified to
return !priv->boottrap & BIT(phydev->mdio.addr);
Andrew
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)
>
> Would enable being clear result in the LED being off? You can force it
> on with MTK_PHY_LED_FORCE_ON | MTK_PHY_LED_ENABLE? That gives you
> enough to allow software control of the LED. You can then implement
> the led_brightness_set() op, if you want.
Yes, and that would be the next thing I was planning to work on.
>
> I assume the above are specific to LED0? It would be good to include
> the 0 in the name, to make that clear.
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.
>
> > +
> > +#define MTK_PHY_LED0_BLINK_CTRL 0x25
> > +#define MTK_PHY_LED1_BLINK_CTRL 0x27
> > +#define MTK_PHY_LED_1000TX BIT(0)
>
> So do this mean LINK1000 + blink on TX ?
Exactly.
>
> > +#define MTK_PHY_LED_1000RX BIT(1)
>
> So do this mean LINK1000 + blink on RX ?
>
Yep.
> It would be good to add a comment, because at some point it is likely
> somebody will want to offload the ledtrig-netdev and will need to
> understand what these really do.
>
> > +#define MTK_PHY_LED_100TX BIT(2)
> > +#define MTK_PHY_LED_100RX BIT(3)
> > +#define MTK_PHY_LED_10TX BIT(4)
> > +#define MTK_PHY_LED_10RX BIT(5)
> > +#define MTK_PHY_LED_COLLISION BIT(6)
> > +#define MTK_PHY_LED_RX_CRC_ERR BIT(7)
> > +#define MTK_PHY_LED_RX_IDLE_ERR BIT(8)
> > +#define MTK_PHY_LED_FORCE_BLINK BIT(9)
>
> Is there a way to force the LED1 off/on? I guess not setting any of
> these bits will force it off.
Both LED0 and LED1 of each PHY can be forced on by writing
MTK_PHY_LED_FORCE_ON | MTK_PHY_LED_ENABLE to MTK_PHY_LEDx_ON_CTRL.
They can be forced off by clearing MTK_PHY_LED_ENABLE in
MTK_PHY_LEDx_ON_CTRL.
Similarly, blinking can be forced by setting MTK_PHY_LED_FORCE_BLINK
in the MTK_PHY_LEDx_BLINK_CTRL register.
>
> So the polarity and enable bits apply to LED1?
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
--------------+--------------+
>
> > +
> > #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.
>
> Should these actually be :
>
> > +#define MTK_PHY_LED_ON_CTRL1 0x24
> > +#define MTK_PHY_LED_ON_CTRL2 0x26
>
> meaning each LED has two control registers?
No, each LED has one control register and each PHY has two LEDs, and
there are four PHYs total in the package.
>
> MTK_PHY_LED_ON_LINK1000 should actually be MTK_PHY_LED_ON_CTRL1_LINK1000 ?
Maybe MTK_PHY_LED_ON_CTRL_LINK1000 would be more appropriate.
> MTK_PHY_LED_100TX should be MTK_PHY_LED_CTRL2_100TX ?
Maybe better MTK_PHY_LED_BLINK_100TX.
>
> I find it good practice to ensure a bit value #define includes enough
> information in its name to clear indicate what register it applies to.
I agree and will make the names more clear, always using the
MTK_PHY_LED_ON_CTRL_ prefix for bits in the MTK_PHY_LEDx_ON_CTRL
register and MTK_PHY_LED_BLINK_CTRL_ prefix for all registers in
MTK_PHY_LEDx_BLINK_CTRL.
>
> > +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.
>
> Please add a lot of comments about how this hardware actually works!
>
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_BLINK_CTRL,
> > + led_blink_defaults);
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_BLINK_CTRL,
> > + led_blink_defaults);
> > +
> > + 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.
>
> > +static bool mt7988_phy_get_boottrap_polarity(struct phy_device *phydev)
> > +{
> > + struct mtk_socphy_shared *priv = phydev->shared->priv;
> > +
> > + if (priv->boottrap & BIT(phydev->mdio.addr))
> > + return false;
>
> This can be simplified to
>
> return !priv->boottrap & BIT(phydev->mdio.addr);
Ack.
>
> Andrew
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