| 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: <Z4THa8DvQJu96Ycl@eichest-laptop>
Date: Mon, 13 Jan 2025 08:57:31 +0100
From: Stefan Eichenberger <eichest@...il.com>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
Gregor Herburger <gregor.herburger@...tq-group.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: marvell-88q2xxx: Add support for PHY LEDs on
88q2xxx
Hi Dimitri,
On Fri, Jan 10, 2025 at 07:30:58PM +0100, Dimitri Fedrau wrote:
> Hi Stefan,
>
> Am Fri, Jan 10, 2025 at 06:27:43PM +0100 schrieb Stefan Eichenberger:
> > Hi Dimitri ,
> >
> > On Fri, Jan 10, 2025 at 04:10:04PM +0100, Dimitri Fedrau wrote:
> > > Marvell 88Q2XXX devices support up to two configurable Light Emitting
> > > Diode (LED). Add minimal LED controller driver supporting the most common
> > > uses with the 'netdev' trigger.
> > >
> > > Signed-off-by: Dimitri Fedrau <dima.fedrau@...il.com>
> > > ---
> > > drivers/net/phy/marvell-88q2xxx.c | 161 ++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 161 insertions(+)
> > >
> > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> > > index 5107f58338aff4ed6cfea4d91e37282d9bb60ba5..bef3357b9d279fca5d1f86ff0eaa0d45a699e3f9 100644
> > > --- a/drivers/net/phy/marvell-88q2xxx.c
> > > +++ b/drivers/net/phy/marvell-88q2xxx.c
> > > @@ -8,6 +8,7 @@
> > > */
> > > #include <linux/ethtool_netlink.h>
> > > #include <linux/marvell_phy.h>
> > > +#include <linux/of.h>
> > > #include <linux/phy.h>
> > > #include <linux/hwmon.h>
> > >
> > > @@ -27,6 +28,9 @@
> > > #define MDIO_MMD_AN_MV_STAT2_100BT1 0x2000
> > > #define MDIO_MMD_AN_MV_STAT2_1000BT1 0x4000
> > >
> > > +#define MDIO_MMD_PCS_MV_RESET_CTRL 32768
> > > +#define MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE 0x8
> > > +
> > > #define MDIO_MMD_PCS_MV_INT_EN 32784
> > > #define MDIO_MMD_PCS_MV_INT_EN_LINK_UP 0x0040
> > > #define MDIO_MMD_PCS_MV_INT_EN_LINK_DOWN 0x0080
> > > @@ -40,6 +44,15 @@
> > > #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL 32787
> > > #define MDIO_MMD_PCS_MV_GPIO_INT_CTRL_TRI_DIS 0x0800
> > >
> > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL 32790
> > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK GENMASK(7, 4)
> > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK GENMASK(3, 0)
> > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK 0x0 /* Link established */
> > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_RX_TX 0x1 /* Link established, blink for rx or tx activity */
> > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_RX_TX 0x4 /* Receive or Transmit activity */
> > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX 0x5 /* Transmit activity */
> > > +#define MDIO_MMD_PCS_MV_LED_FUNC_CTRL_LINK_1000BT1 0x7 /* 1000BT1 link established */
> > > +
> > > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1 32833
> > > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_RAW_INT 0x0001
> > > #define MDIO_MMD_PCS_MV_TEMP_SENSOR1_INT 0x0040
> > > @@ -95,6 +108,9 @@
> > >
> > > #define MDIO_MMD_PCS_MV_TDR_OFF_CUTOFF 65246
> > >
> > > +#define MV88Q2XXX_LED_INDEX_TX_ENABLE 0
> > > +#define MV88Q2XXX_LED_INDEX_GPIO 1
> >
> > Not sure if I understand this. TX_ENABLE would be LED0 and GPIO would be
> > LED1? In my datasheet the 88Q222x only has a GPIO pin (which is also
> > TX_ENABLE), is this a problem? Would we need a led_count variable per
> > chip?
> >
> Yes you understand it correctly.
> Looking at the datasheets for 88Q212x, 88Q211x and 88Q222x, they have all
> TX_ENABLE and GPIO pin. Registers are also the same. Did I miss anything ?
> For which device GPIO pin and TX_ENABLE are the same ?
>
Hmm in my datasheet the bits 4:7 in Register 3:0x8016 are marked as
reserved. However, maybe my datasheet is outdated. I have the TD-000217
Rev. 9 from November 10, 2023. It is for the following chips:
88Q2220/88Q2220M/88Q2221/88Q2221M/88Q1200M/88Q1201M
If it is documented in your datasheet, then it is fine. Maybe they just
forgot to document them in the one I have.
> > In the 88Q2110 I can see that there is a TX_ENABLE (0) and a GPIO (1)
> > pin. In the register description they just call it LED [0] Control and
> > LED [1] Control. Maybe calling it LED_0 and LED_1 would be easier to
> > understand? Same for MDIO_MMD_PCS_MV_LED_FUNC_CTRL_GPIO_MASK and
> > MDIO_MMD_PCS_MV_LED_FUNC_CTRL_TX_EN_MASK.
> >
> I named them just as the pin. Probably it would be easier to understand,
> but the mapping between pin and index would be lost. What do you think ?
>
> > > +
> > > struct mmd_val {
> > > int devad;
> > > u32 regnum;
> > > @@ -741,8 +757,58 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > > }
> > > #endif
> > >
> > > +#if IS_ENABLED(CONFIG_OF_MDIO)
> > > +static int mv88q2xxx_leds_probe(struct phy_device *phydev)
> > > +{
> > > + struct device_node *node = phydev->mdio.dev.of_node;
> > > + struct device_node *leds;
> > > + int ret = 0;
> > > + u32 index;
> > > +
> > > + if (!node)
> > > + return 0;
> > > +
> > > + leds = of_get_child_by_name(node, "leds");
> > > + if (!leds)
> > > + return 0;
> > > +
> > > + for_each_available_child_of_node_scoped(leds, led) {
> > > + ret = of_property_read_u32(led, "reg", &index);
> > > + if (ret)
> > > + goto exit;
> > > +
> > > + if (index > MV88Q2XXX_LED_INDEX_GPIO) {
> > > + ret = -EINVAL;
> > > + goto exit;
> > > + }
> > > +
> > > + if (index == MV88Q2XXX_LED_INDEX_TX_ENABLE)
> > > + ret = phy_clear_bits_mmd(phydev, MDIO_MMD_PCS,
> > > + MDIO_MMD_PCS_MV_RESET_CTRL,
> > > + MDIO_MMD_PCS_MV_RESET_CTRL_TX_DISABLE);
> >
> > If I understand it correctly, this switches the function of the pin from
> > TX_DISABLE to GPIO? Can you maybe add a comment here?
> >
> You are right, will add the comment.
Perfekt, thanks.
Regards,
Stefan
Powered by blists - more mailing lists