[PATCH net-next 0/4] net: phy: marvell10g: Firmware loading and LED support for 88X3310

[PATCH net-next 0/4] net: phy: marvell10g: Firmware loading and LED support for 88X3310

[Tobias Waldekranz]

There are two boot options for a 88X3310 PHY:

1. Device loads its firmware from a dedicated serial FLASH
2. Device waits for its firmware to be downloaded over XMDIO

1/4 adds support for the second option. The device reports which mode
it is in via a register, so we only attempt to load a firmware in this
situation. Crucially, if firmware is not available in this case, the
device is not usable _at all_, so we are forced to fail the probe
entirely.

2/4 extends the power up sequence to cover cases where the device has
been hardware strapped to start powered down, in which case all
internal units will be powered down.

3/4 adds support for the LED controller in the PHY. A special DT
attribute is added to control the polarity and drive behavior of each
LED, which we document in 4/4.

Tobias Waldekranz (4):
  net: phy: marvell10g: Support firmware loading on 88X3310
  net: phy: marvell10g: Fix power-up when strapped to start powered down
  net: phy: marvell10g: Add LED support for 88X3310
  dt-bindings: net: marvell10g: Document LED polarity

 .../bindings/net/marvell,marvell10g.yaml      |  60 ++
 MAINTAINERS                                   |   1 +
 drivers/net/phy/marvell10g.c                  | 602 +++++++++++++++++-
 3 files changed, 660 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/marvell,marvell10g.yaml

-- 
2.34.1

[PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Tobias Waldekranz]

When probing, if a device is waiting for firmware to be loaded into
its RAM, ask userspace for the binary and load it over XMDIO.

We have no choice but to bail out of the probe if firmware is not
available, as the device does not have any built-in image on which to
fall back.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/phy/marvell10g.c | 149 +++++++++++++++++++++++++++++++++++
 1 file changed, 149 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index ad43e280930c..83233b30d7b0 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -25,6 +25,7 @@
 #include <linux/bitfield.h>
 #include <linux/ctype.h>
 #include <linux/delay.h>
+#include <linux/firmware.h>
 #include <linux/hwmon.h>
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
@@ -50,6 +51,13 @@ enum {
 	MV_PMA_21X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH	= 0x6,
 	MV_PMA_BOOT		= 0xc050,
 	MV_PMA_BOOT_FATAL	= BIT(0),
+	MV_PMA_BOOT_PRGS_MASK	= 0x0006,
+	MV_PMA_BOOT_PRGS_INIT	= 0x0000,
+	MV_PMA_BOOT_PRGS_WAIT	= 0x0002,
+	MV_PMA_BOOT_PRGS_CSUM	= 0x0004,
+	MV_PMA_BOOT_PRGS_JRAM	= 0x0006,
+	MV_PMA_BOOT_APP_STARTED	= BIT(4),
+	MV_PMA_BOOT_APP_LOADED	= BIT(6),
 
 	MV_PCS_BASE_T		= 0x0000,
 	MV_PCS_BASE_R		= 0x1000,
@@ -96,6 +104,12 @@ enum {
 	MV_PCS_PORT_INFO_NPORTS_MASK	= 0x0380,
 	MV_PCS_PORT_INFO_NPORTS_SHIFT	= 7,
 
+	/* Firmware downloading */
+	MV_PCS_FW_ADDR_LOW	= 0xd0f0,
+	MV_PCS_FW_ADDR_HIGH	= 0xd0f1,
+	MV_PCS_FW_DATA		= 0xd0f2,
+	MV_PCS_FW_CSUM		= 0xd0f3,
+
 	/* SerDes reinitialization 88E21X0 */
 	MV_AN_21X0_SERDES_CTRL2	= 0x800f,
 	MV_AN_21X0_SERDES_CTRL2_AUTO_INIT_DIS	= BIT(13),
@@ -156,6 +170,7 @@ struct mv3310_chip {
 
 	const struct mv3310_mactype *mactypes;
 	size_t n_mactypes;
+	const char *firmware_path;
 
 #ifdef CONFIG_HWMON
 	int (*hwmon_read_temp_reg)(struct phy_device *phydev);
@@ -506,6 +521,132 @@ static const struct sfp_upstream_ops mv3310_sfp_ops = {
 	.module_insert = mv3310_sfp_insert,
 };
 
+struct mv3310_fw_hdr {
+	struct {
+		u32 size;
+		u32 addr;
+		u16 csum;
+	} __packed data;
+
+	u8 flags;
+#define MV3310_FW_HDR_DATA_ONLY BIT(6)
+
+	u8 port_skip;
+	u32 next_hdr;
+	u16 csum;
+
+	u8 pad[14];
+} __packed;
+
+static int mv3310_load_fw_sect(struct phy_device *phydev,
+			       const struct mv3310_fw_hdr *hdr, const u8 *data)
+{
+	int err = 0;
+	size_t i;
+	u16 csum;
+
+	dev_dbg(&phydev->mdio.dev, "Loading %u byte %s section at 0x%08x\n",
+		hdr->data.size,
+		(hdr->flags & MV3310_FW_HDR_DATA_ONLY) ? "data" : "executable",
+		hdr->data.addr);
+
+	for (i = 0, csum = 0; i < hdr->data.size; i++)
+		csum += data[i];
+
+	if ((u16)~csum != hdr->data.csum) {
+		dev_err(&phydev->mdio.dev, "Corrupt section data\n");
+		return -EINVAL;
+	}
+
+	phy_lock_mdio_bus(phydev);
+
+	/* Any existing checksum is cleared by a read */
+	__phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
+
+	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_LOW,  hdr->data.addr & 0xffff);
+	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_HIGH, hdr->data.addr >> 16);
+
+	for (i = 0; i < hdr->data.size; i += 2) {
+		__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_DATA,
+				(data[i + 1] << 8) | data[i]);
+	}
+
+	csum = __phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
+	if ((u16)~csum != hdr->data.csum) {
+		dev_err(&phydev->mdio.dev, "Download failed\n");
+		err = -EIO;
+		goto unlock;
+	}
+
+	if (hdr->flags & MV3310_FW_HDR_DATA_ONLY)
+		goto unlock;
+
+	__phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT, 0, MV_PMA_BOOT_APP_LOADED);
+	mdelay(200);
+	if (!(__phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT) & MV_PMA_BOOT_APP_STARTED)) {
+		dev_err(&phydev->mdio.dev, "Application did not startup\n");
+		err = -ENODEV;
+	}
+
+unlock:
+	phy_unlock_mdio_bus(phydev);
+	return err;
+}
+
+static int mv3310_load_fw(struct phy_device *phydev)
+{
+	const struct mv3310_chip *chip = to_mv3310_chip(phydev);
+	const struct firmware *fw;
+	struct mv3310_fw_hdr hdr;
+	const u8 *sect;
+	size_t i;
+	u16 csum;
+	int err;
+
+	if (!chip->firmware_path)
+		return -EOPNOTSUPP;
+
+	err = request_firmware(&fw, chip->firmware_path, &phydev->mdio.dev);
+	if (err)
+		return err;
+
+	if (fw->size & 1) {
+		err = -EINVAL;
+		goto release;
+	}
+
+	for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {
+		memcpy(&hdr, sect, sizeof(hdr));
+		hdr.data.size = cpu_to_le32(hdr.data.size);
+		hdr.data.addr = cpu_to_le32(hdr.data.addr);
+		hdr.data.csum = cpu_to_le16(hdr.data.csum);
+		hdr.next_hdr = cpu_to_le32(hdr.next_hdr);
+		hdr.csum = cpu_to_le16(hdr.csum);
+
+		for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++)
+			csum += sect[i];
+
+		if ((u16)~csum != hdr.csum) {
+			dev_err(&phydev->mdio.dev, "Corrupt section header\n");
+			err = -EINVAL;
+			break;
+		}
+
+		err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr));
+		if (err)
+			break;
+
+		if (!hdr.next_hdr)
+			break;
+
+		sect = fw->data + hdr.next_hdr;
+	}
+
+release:
+	release_firmware(fw);
+	return err;
+}
+
 static int mv3310_probe(struct phy_device *phydev)
 {
 	const struct mv3310_chip *chip = to_mv3310_chip(phydev);
@@ -527,6 +668,12 @@ static int mv3310_probe(struct phy_device *phydev)
 		return -ENODEV;
 	}
 
+	if ((ret & MV_PMA_BOOT_PRGS_MASK) == MV_PMA_BOOT_PRGS_WAIT) {
+		ret = mv3310_load_fw(phydev);
+		if (ret)
+			return ret;
+	}
+
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -1219,6 +1366,7 @@ static const struct mv3310_chip mv3310_type = {
 
 	.mactypes = mv3310_mactypes,
 	.n_mactypes = ARRAY_SIZE(mv3310_mactypes),
+	.firmware_path = "mrvl/x3310fw.hdr",
 
 #ifdef CONFIG_HWMON
 	.hwmon_read_temp_reg = mv3310_hwmon_read_temp_reg,
@@ -1489,4 +1637,5 @@ static struct mdio_device_id __maybe_unused mv3310_tbl[] = {
 };
 MODULE_DEVICE_TABLE(mdio, mv3310_tbl);
 MODULE_DESCRIPTION("Marvell Alaska X/M multi-gigabit Ethernet PHY driver");
+MODULE_FIRMWARE("mrvl/x3310fw.hdr");
 MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Andrew Lunn]

On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
> When probing, if a device is waiting for firmware to be loaded into
> its RAM, ask userspace for the binary and load it over XMDIO.

Does a device without firmware have valid ID registers? Is the driver
going to probe via bus enumeration, or is it necessary to use a
compatible with ID values?

> +	for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {

This validates that the firmware is big enough to hold the header...

> +		memcpy(&hdr, sect, sizeof(hdr));
> +		hdr.data.size = cpu_to_le32(hdr.data.size);
> +		hdr.data.addr = cpu_to_le32(hdr.data.addr);
> +		hdr.data.csum = cpu_to_le16(hdr.data.csum);
> +		hdr.next_hdr = cpu_to_le32(hdr.next_hdr);

I'm surprised sparse is not complaining about this. You have the same
source and destination, and sparse probably wants the destination to
be marked as little endian.

> +		hdr.csum = cpu_to_le16(hdr.csum);
> +
> +		for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++)
> +			csum += sect[i];
> +
> +		if ((u16)~csum != hdr.csum) {
> +			dev_err(&phydev->mdio.dev, "Corrupt section header\n");
> +			err = -EINVAL;
> +			break;
> +		}
> +
> +		err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr));

What i don't see is any validation that the firmware left at sect +
sizeof(hdr) big enough to contain hdr.data.size bytes.

	    Andrew

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Russell King (Oracle)]

On Fri, Dec 15, 2023 at 03:30:09PM +0100, Andrew Lunn wrote:
> On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
> > When probing, if a device is waiting for firmware to be loaded into
> > its RAM, ask userspace for the binary and load it over XMDIO.
> 
> Does a device without firmware have valid ID registers?

Yes it does - remember one of the ZII dev boards had a 3310 without
firmware, and that can be successfully probed by the driver, which
is key to my userspace tooling being able to access the PHY (since
userspace can only access devices on MDIO buses that are bound to
a netdev.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Tobias Waldekranz]

On fre, dec 15, 2023 at 15:30, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
>> When probing, if a device is waiting for firmware to be loaded into
>> its RAM, ask userspace for the binary and load it over XMDIO.
>
> Does a device without firmware have valid ID registers? Is the driver
> going to probe via bus enumeration, or is it necessary to use a
> compatible with ID values?
>
>> +	for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {
>
> This validates that the firmware is big enough to hold the header...
>
>> +		memcpy(&hdr, sect, sizeof(hdr));
>> +		hdr.data.size = cpu_to_le32(hdr.data.size);
>> +		hdr.data.addr = cpu_to_le32(hdr.data.addr);
>> +		hdr.data.csum = cpu_to_le16(hdr.data.csum);
>> +		hdr.next_hdr = cpu_to_le32(hdr.next_hdr);
>
> I'm surprised sparse is not complaining about this. You have the same
> source and destination, and sparse probably wants the destination to
> be marked as little endian.
>
>> +		hdr.csum = cpu_to_le16(hdr.csum);
>> +
>> +		for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++)
>> +			csum += sect[i];
>> +
>> +		if ((u16)~csum != hdr.csum) {
>> +			dev_err(&phydev->mdio.dev, "Corrupt section header\n");
>> +			err = -EINVAL;
>> +			break;
>> +		}
>> +
>> +		err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr));
>
> What i don't see is any validation that the firmware left at sect +
> sizeof(hdr) big enough to contain hdr.data.size bytes.
>

Thanks Andrew and Russel, for the review!

You both make valid points, I'll try to be less clever about this whole
section in v2.

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Andrew Lunn]

> +	for (i = 0, csum = 0; i < hdr->data.size; i++)
> +		csum += data[i];

> +	for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {
> +		memcpy(&hdr, sect, sizeof(hdr));
> +		hdr.data.size = cpu_to_le32(hdr.data.size);

hdr.data.size is little endian. Doing a for loop using a little endian
test seems wrong. Should this actually be le32_to_cpu()?

       Andrew

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Russell King (Oracle)]

On Thu, Dec 14, 2023 at 09:14:39PM +0100, Tobias Waldekranz wrote:
> +	MV_PMA_BOOT_PRGS_MASK	= 0x0006,
> +	MV_PMA_BOOT_PRGS_INIT	= 0x0000,
> +	MV_PMA_BOOT_PRGS_WAIT	= 0x0002,
> +	MV_PMA_BOOT_PRGS_CSUM	= 0x0004,
> +	MV_PMA_BOOT_PRGS_JRAM	= 0x0006,

You only seem to use PRGS_WAIT, the rest seem unused.

> +struct mv3310_fw_hdr {
> +	struct {
> +		u32 size;
> +		u32 addr;
> +		u16 csum;
> +	} __packed data;

It's probably better to get rid of this embedded struct and just place
the members in the parent struct (although csum woul dneed to be
renamed).

> +
> +	u8 flags;
> +#define MV3310_FW_HDR_DATA_ONLY BIT(6)
> +
> +	u8 port_skip;
> +	u32 next_hdr;
> +	u16 csum;
> +
> +	u8 pad[14];
> +} __packed;
> +
> +static int mv3310_load_fw_sect(struct phy_device *phydev,
> +			       const struct mv3310_fw_hdr *hdr, const u8 *data)
> +{
> +	int err = 0;
> +	size_t i;
> +	u16 csum;
> +
> +	dev_dbg(&phydev->mdio.dev, "Loading %u byte %s section at 0x%08x\n",
> +		hdr->data.size,
> +		(hdr->flags & MV3310_FW_HDR_DATA_ONLY) ? "data" : "executable",
> +		hdr->data.addr);
> +
> +	for (i = 0, csum = 0; i < hdr->data.size; i++)
> +		csum += data[i];
> +
> +	if ((u16)~csum != hdr->data.csum) {
> +		dev_err(&phydev->mdio.dev, "Corrupt section data\n");
> +		return -EINVAL;
> +	}
> +
> +	phy_lock_mdio_bus(phydev);
> +
> +	/* Any existing checksum is cleared by a read */
> +	__phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
> +
> +	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_LOW,  hdr->data.addr & 0xffff);
> +	__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_ADDR_HIGH, hdr->data.addr >> 16);
> +
> +	for (i = 0; i < hdr->data.size; i += 2) {
> +		__phy_write_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_DATA,
> +				(data[i + 1] << 8) | data[i]);
> +	}
> +
> +	csum = __phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_FW_CSUM);
> +	if ((u16)~csum != hdr->data.csum) {
> +		dev_err(&phydev->mdio.dev, "Download failed\n");
> +		err = -EIO;
> +		goto unlock;
> +	}
> +
> +	if (hdr->flags & MV3310_FW_HDR_DATA_ONLY)
> +		goto unlock;
> +
> +	__phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT, 0, MV_PMA_BOOT_APP_LOADED);
> +	mdelay(200);
> +	if (!(__phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MV_PMA_BOOT) & MV_PMA_BOOT_APP_STARTED)) {
> +		dev_err(&phydev->mdio.dev, "Application did not startup\n");
> +		err = -ENODEV;
> +	}

I'm confused why this is done here - after each section in the firmware
file, rather than having loaded all sections in the firmware file and
only then starting the application. Surely if there's multiple sections
that we're going to load, we want to load _all_ sections before starting
the application?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[kernel test robot]

Hi Tobias,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Tobias-Waldekranz/net-phy-marvell10g-Support-firmware-loading-on-88X3310/20231215-041703
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231214201442.660447-2-tobias%40waldekranz.com
patch subject: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310
config: x86_64-randconfig-123-20231216 (https://download.01.org/0day-ci/archive/20231216/202312162238.aJCgm39Y-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231216/202312162238.aJCgm39Y-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312162238.aJCgm39Y-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/marvell10g.c:620:31: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] size @@     got restricted __le32 [usertype] @@
   drivers/net/phy/marvell10g.c:620:31: sparse:     expected unsigned int [addressable] [usertype] size
   drivers/net/phy/marvell10g.c:620:31: sparse:     got restricted __le32 [usertype]
>> drivers/net/phy/marvell10g.c:621:31: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] addr @@     got restricted __le32 [usertype] @@
   drivers/net/phy/marvell10g.c:621:31: sparse:     expected unsigned int [addressable] [usertype] addr
   drivers/net/phy/marvell10g.c:621:31: sparse:     got restricted __le32 [usertype]
>> drivers/net/phy/marvell10g.c:622:31: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned short [addressable] [usertype] csum @@     got restricted __le16 [usertype] @@
   drivers/net/phy/marvell10g.c:622:31: sparse:     expected unsigned short [addressable] [usertype] csum
   drivers/net/phy/marvell10g.c:622:31: sparse:     got restricted __le16 [usertype]
>> drivers/net/phy/marvell10g.c:623:30: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned int [addressable] [usertype] next_hdr @@     got restricted __le32 [usertype] @@
   drivers/net/phy/marvell10g.c:623:30: sparse:     expected unsigned int [addressable] [usertype] next_hdr
   drivers/net/phy/marvell10g.c:623:30: sparse:     got restricted __le32 [usertype]
   drivers/net/phy/marvell10g.c:624:26: sparse: sparse: incorrect type in assignment (different base types) @@     expected unsigned short [addressable] [usertype] csum @@     got restricted __le16 [usertype] @@
   drivers/net/phy/marvell10g.c:624:26: sparse:     expected unsigned short [addressable] [usertype] csum
   drivers/net/phy/marvell10g.c:624:26: sparse:     got restricted __le16 [usertype]

vim +620 drivers/net/phy/marvell10g.c

   595	
   596	static int mv3310_load_fw(struct phy_device *phydev)
   597	{
   598		const struct mv3310_chip *chip = to_mv3310_chip(phydev);
   599		const struct firmware *fw;
   600		struct mv3310_fw_hdr hdr;
   601		const u8 *sect;
   602		size_t i;
   603		u16 csum;
   604		int err;
   605	
   606		if (!chip->firmware_path)
   607			return -EOPNOTSUPP;
   608	
   609		err = request_firmware(&fw, chip->firmware_path, &phydev->mdio.dev);
   610		if (err)
   611			return err;
   612	
   613		if (fw->size & 1) {
   614			err = -EINVAL;
   615			goto release;
   616		}
   617	
   618		for (sect = fw->data; (sect + sizeof(hdr)) < (fw->data + fw->size);) {
   619			memcpy(&hdr, sect, sizeof(hdr));
 > 620			hdr.data.size = cpu_to_le32(hdr.data.size);
 > 621			hdr.data.addr = cpu_to_le32(hdr.data.addr);
 > 622			hdr.data.csum = cpu_to_le16(hdr.data.csum);
 > 623			hdr.next_hdr = cpu_to_le32(hdr.next_hdr);
   624			hdr.csum = cpu_to_le16(hdr.csum);
   625	
   626			for (i = 0, csum = 0; i < offsetof(struct mv3310_fw_hdr, csum); i++)
   627				csum += sect[i];
   628	
   629			if ((u16)~csum != hdr.csum) {
   630				dev_err(&phydev->mdio.dev, "Corrupt section header\n");
   631				err = -EINVAL;
   632				break;
   633			}
   634	
   635			err = mv3310_load_fw_sect(phydev, &hdr, sect + sizeof(hdr));
   636			if (err)
   637				break;
   638	
   639			if (!hdr.next_hdr)
   640				break;
   641	
   642			sect = fw->data + hdr.next_hdr;
   643		}
   644	
   645	release:
   646		release_firmware(fw);
   647		return err;
   648	}
   649	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Marek Behún]

On Thu, 14 Dec 2023 21:14:39 +0100
Tobias Waldekranz <tobias@waldekranz.com> wrote:

> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");

And do you have permission to publish this firmware into linux-firmware?

Because when we tried this with Marvell, their lawyer guy said we can't
do that...

Marek

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Tobias Waldekranz]

On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
> On Thu, 14 Dec 2023 21:14:39 +0100
> Tobias Waldekranz <tobias@waldekranz.com> wrote:
>
>> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
>
> And do you have permission to publish this firmware into linux-firmware?

No, I do not.

> Because when we tried this with Marvell, their lawyer guy said we can't
> do that...

I don't even have good enough access to ask the question, much less get
rejected by Marvell :) I just used that path so that it would line up
with linux-firmware if Marvell was to publish it in the future.

Should MODULE_FIRMWARE be avoided for things that are not in
linux-firmware?

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Marek Behún]

On Tue, 19 Dec 2023 11:15:41 +0100
Tobias Waldekranz <tobias@waldekranz.com> wrote:

> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
> > On Thu, 14 Dec 2023 21:14:39 +0100
> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
> >  
> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");  
> >
> > And do you have permission to publish this firmware into linux-firmware?  
> 
> No, I do not.
> 
> > Because when we tried this with Marvell, their lawyer guy said we can't
> > do that...  
> 
> I don't even have good enough access to ask the question, much less get
> rejected by Marvell :) I just used that path so that it would line up
> with linux-firmware if Marvell was to publish it in the future.

Yeah, it was pretty stupid in my opinion. The lawyer guy's reasoning
was that to download the firmware from Marvell's Customer Portal you
have to agree with Terms & Conditions, so it can't be distributed to
people who did not agree to Terms & Conditions. We told him that anyone
can get access to the firmware without agreeing anyway, since they can
just read the SPI NOR module connected to the PHY if we burn the
firmware in manufacture...

> Should MODULE_FIRMWARE be avoided for things that are not in
> linux-firmware?

I don't know.

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Tobias Waldekranz]

On tis, dec 19, 2023 at 11:49, Marek Behún <kabel@kernel.org> wrote:
> On Tue, 19 Dec 2023 11:15:41 +0100
> Tobias Waldekranz <tobias@waldekranz.com> wrote:
>
>> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
>> > On Thu, 14 Dec 2023 21:14:39 +0100
>> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
>> >  
>> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");  
>> >
>> > And do you have permission to publish this firmware into linux-firmware?  
>> 
>> No, I do not.
>> 
>> > Because when we tried this with Marvell, their lawyer guy said we can't
>> > do that...  
>> 
>> I don't even have good enough access to ask the question, much less get
>> rejected by Marvell :) I just used that path so that it would line up
>> with linux-firmware if Marvell was to publish it in the future.
>
> Yeah, it was pretty stupid in my opinion. The lawyer guy's reasoning
> was that to download the firmware from Marvell's Customer Portal you
> have to agree with Terms & Conditions, so it can't be distributed to
> people who did not agree to Terms & Conditions. We told him that anyone
> can get access to the firmware without agreeing anyway, since they can
> just read the SPI NOR module connected to the PHY if we burn the
> firmware in manufacture...

Yeah, they are needlessly secretive in lots of ways - much to their own
detriment, IMO. They also protect their functional specs as if you could
just run them through `pdf2rtl`, email the output to TSMC, and have your
own 7nm ASIC in the mail the following week.

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Russell King (Oracle)]

On Tue, Dec 19, 2023 at 11:15:41AM +0100, Tobias Waldekranz wrote:
> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
> > On Thu, 14 Dec 2023 21:14:39 +0100
> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
> >
> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
> >
> > And do you have permission to publish this firmware into linux-firmware?
> 
> No, I do not.
> 
> > Because when we tried this with Marvell, their lawyer guy said we can't
> > do that...
> 
> I don't even have good enough access to ask the question, much less get
> rejected by Marvell :) I just used that path so that it would line up
> with linux-firmware if Marvell was to publish it in the future.
> 
> Should MODULE_FIRMWARE be avoided for things that are not in
> linux-firmware?

Without the firmware being published, what use is having this code in
mainline kernels?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Tobias Waldekranz]

On tis, jan 02, 2024 at 10:12, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Tue, Dec 19, 2023 at 11:15:41AM +0100, Tobias Waldekranz wrote:
>> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
>> > On Thu, 14 Dec 2023 21:14:39 +0100
>> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
>> >
>> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
>> >
>> > And do you have permission to publish this firmware into linux-firmware?
>> 
>> No, I do not.
>> 
>> > Because when we tried this with Marvell, their lawyer guy said we can't
>> > do that...
>> 
>> I don't even have good enough access to ask the question, much less get
>> rejected by Marvell :) I just used that path so that it would line up
>> with linux-firmware if Marvell was to publish it in the future.
>> 
>> Should MODULE_FIRMWARE be avoided for things that are not in
>> linux-firmware?
>
> Without the firmware being published, what use is having this code in
> mainline kernels?

Personally, I primarily want this merged so that future contributions to
the driver are easier to develop, since I'll be able test them on top of
a clean net-next base.

More broadly, I suppose it will help others who are looking to support
similar boards to run the latest kernel, without the need to juggle
external patches which are likely to break as the driver evolves.

Having a single, canonical, implementation of firmware loading, instead
of multiple slightly-broken-in-different-ways ones floating around, also
seems like a net positive.

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Daniel Golle]

Hi Tobias,

On Tue, Jan 02, 2024 at 02:09:24PM +0100, Tobias Waldekranz wrote:
> On tis, jan 02, 2024 at 10:12, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > On Tue, Dec 19, 2023 at 11:15:41AM +0100, Tobias Waldekranz wrote:
> >> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
> >> > On Thu, 14 Dec 2023 21:14:39 +0100
> >> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
> >> >
> >> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
> >> >
> >> > And do you have permission to publish this firmware into linux-firmware?
> >> 
> >> No, I do not.
> >> 
> >> > Because when we tried this with Marvell, their lawyer guy said we can't
> >> > do that...
> >> 
> >> I don't even have good enough access to ask the question, much less get
> >> rejected by Marvell :) I just used that path so that it would line up
> >> with linux-firmware if Marvell was to publish it in the future.
> >> 
> >> Should MODULE_FIRMWARE be avoided for things that are not in
> >> linux-firmware?
> >
> > Without the firmware being published, what use is having this code in
> > mainline kernels?
> 
> Personally, I primarily want this merged so that future contributions to
> the driver are easier to develop, since I'll be able test them on top of
> a clean net-next base.

I've been pointed to your series by Krzysztof Kozlowski who had reviewed
the DT part of it. Are you still working on that or going to eventually
re-submit it?

I understand that the suggested LED support pre-dates commit

7ae215ee7bb8 net: phy: add support for PHY LEDs polarity modes

which would allow using generic properties 'active-low' and
'inactive-high-impedance'. I assume that would be applicable to the LED
patch which was part of this series as well?

In that case, we would no longer need a vendor-specific property for that
purpose. If the LEDs are active-low by default (or early boot firmware
setting) and you would need a property for setting them to 'active-high'
instead, I just suggested that in

https://patchwork.kernel.org/project/netdevbpf/patch/e91ca84ac836fc40c94c52733f8fc607bcbed64c.1728145095.git.daniel@makrotopia.org/

which is why I'm now contacting you, as I was a bit confused by Krzysztof's
suggestion to take a look at marvell,marvell10g.yaml which would have been
introduced by your series.

Imho it would be better to use the (now existing) generic properties than
resorting to a vendor-specific one.

In every case, if you have a minute to look at commit 7ae215ee7bb8 and let
us know whether that structure, with or without my suggested addition,
would be suitable for your case as well, that would be nice.


Thank you for your time and support!


Daniel

Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Tobias Waldekranz]

On sön, okt 06, 2024 at 17:15, Daniel Golle <daniel@makrotopia.org> wrote:
> Hi Tobias,

Hi Daniel,

> On Tue, Jan 02, 2024 at 02:09:24PM +0100, Tobias Waldekranz wrote:
>> On tis, jan 02, 2024 at 10:12, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>> > On Tue, Dec 19, 2023 at 11:15:41AM +0100, Tobias Waldekranz wrote:
>> >> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
>> >> > On Thu, 14 Dec 2023 21:14:39 +0100
>> >> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
>> >> >
>> >> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
>> >> >
>> >> > And do you have permission to publish this firmware into linux-firmware?
>> >> 
>> >> No, I do not.
>> >> 
>> >> > Because when we tried this with Marvell, their lawyer guy said we can't
>> >> > do that...
>> >> 
>> >> I don't even have good enough access to ask the question, much less get
>> >> rejected by Marvell :) I just used that path so that it would line up
>> >> with linux-firmware if Marvell was to publish it in the future.
>> >> 
>> >> Should MODULE_FIRMWARE be avoided for things that are not in
>> >> linux-firmware?
>> >
>> > Without the firmware being published, what use is having this code in
>> > mainline kernels?
>> 
>> Personally, I primarily want this merged so that future contributions to
>> the driver are easier to develop, since I'll be able test them on top of
>> a clean net-next base.
>
> I've been pointed to your series by Krzysztof Kozlowski who had reviewed
> the DT part of it. Are you still working on that or going to eventually
> re-submit it?

I'm not actively working on it, no, but I still want to get it
merged. I, perhaps wrongly, interpreted Russell's lack of reply to my
motivation for accepting the firmware loading without having the binary
in linux-firmware, as a NAK, and moved on to other things.

> I understand that the suggested LED support pre-dates commit
>
> 7ae215ee7bb8 net: phy: add support for PHY LEDs polarity modes
>
> which would allow using generic properties 'active-low' and
> 'inactive-high-impedance'. I assume that would be applicable to the LED
> patch which was part of this series as well?
>
> In that case, we would no longer need a vendor-specific property for that
> purpose. If the LEDs are active-low by default (or early boot firmware
> setting) and you would need a property for setting them to 'active-high'
> instead, I just suggested that in
>
> https://patchwork.kernel.org/project/netdevbpf/patch/e91ca84ac836fc40c94c52733f8fc607bcbed64c.1728145095.git.daniel@makrotopia.org/
>
> which is why I'm now contacting you, as I was a bit confused by Krzysztof's
> suggestion to take a look at marvell,marvell10g.yaml which would have been
> introduced by your series.
>
> Imho it would be better to use the (now existing) generic properties than
> resorting to a vendor-specific one.
>
> In every case, if you have a minute to look at commit 7ae215ee7bb8 and let
> us know whether that structure, with or without my suggested addition,
> would be suitable for your case as well, that would be nice.

To me, it seems cleaner to have a single attribute that defines the
behavior you want on the pin (as a string enum). That way you can also
explicitly declare that the kernel shouldn't mess with the settings
(e.g., `polarity = "keep";`, like you can do with the initial
brightness).

If you go that way, I would prefer if the "old" attributes where
deprecated and only evaluated in case the new attribute is not
available.

As for how generic it should be: To me it doesn't seem like there's
anything PHY-specific about it. I suppose where it might be confusing
would be when you have a gpio-led, when that is already taken care of at
the GPIO layer.

[PATCH net-next 2/4] net: phy: marvell10g: Fix power-up when strapped to start powered down

[Tobias Waldekranz]

On devices which are hardware strapped to start powered down (PDSTATE
== 1), make sure that we clear the power-down bit on all units
affected by this setting.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/phy/marvell10g.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 83233b30d7b0..1c1333d867fb 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev)
 
 static int mv3310_power_up(struct phy_device *phydev)
 {
+	static const u16 resets[][2] = {
+		{ MDIO_MMD_PCS,    MV_PCS_BASE_R    + MDIO_CTRL1 },
+		{ MDIO_MMD_PCS,    MV_PCS_1000BASEX + MDIO_CTRL1 },
+		{ MDIO_MMD_PCS,    MV_PCS_BASE_T    + MDIO_CTRL1 },
+		{ MDIO_MMD_PMAPMD, MDIO_CTRL1 },
+		{ MDIO_MMD_VEND2,  MV_V2_PORT_CTRL },
+	};
 	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
-	int ret;
+	int i, ret;
 
-	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
-				 MV_V2_PORT_CTRL_PWRDOWN);
+	for (i = 0; i < ARRAY_SIZE(resets); i++) {
+		ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1],
+					 MV_V2_PORT_CTRL_PWRDOWN);
+		if (ret)
+			break;
+	}
 
 	/* Sometimes, the power down bit doesn't clear immediately, and
 	 * a read of this register causes the bit not to clear. Delay
-- 
2.34.1

Re: [PATCH net-next 2/4] net: phy: marvell10g: Fix power-up when strapped to start powered down

[Russell King (Oracle)]

On Thu, Dec 14, 2023 at 09:14:40PM +0100, Tobias Waldekranz wrote:
> On devices which are hardware strapped to start powered down (PDSTATE
> == 1), make sure that we clear the power-down bit on all units
> affected by this setting.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/net/phy/marvell10g.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 83233b30d7b0..1c1333d867fb 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev)
>  
>  static int mv3310_power_up(struct phy_device *phydev)
>  {
> +	static const u16 resets[][2] = {
> +		{ MDIO_MMD_PCS,    MV_PCS_BASE_R    + MDIO_CTRL1 },
> +		{ MDIO_MMD_PCS,    MV_PCS_1000BASEX + MDIO_CTRL1 },

This is not necessary. The documentation states that the power down
bit found at each of these is the same physical bit appearing in two
different locations. So only one is necessary.

> +		{ MDIO_MMD_PCS,    MV_PCS_BASE_T    + MDIO_CTRL1 },
> +		{ MDIO_MMD_PMAPMD, MDIO_CTRL1 },
> +		{ MDIO_MMD_VEND2,  MV_V2_PORT_CTRL },
> +	};
>  	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
> -	int ret;
> +	int i, ret;
>  
> -	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
> -				 MV_V2_PORT_CTRL_PWRDOWN);
> +	for (i = 0; i < ARRAY_SIZE(resets); i++) {
> +		ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1],
> +					 MV_V2_PORT_CTRL_PWRDOWN);

While MV_V2_PORT_CTRL_PWRDOWN may correspond with the correct bit for
the MDIO CTRL1 register, we have MDIO_CTRL1_LPOWER which describes
this bit. Probably the simplest solution would be to leave the
existing phy_clear_bits_mmd(), remove the vendor 2 entry from the
table, and run through that table first.

Lastly, how does this impact a device which has firmware, and the
firmware manages the power-down state (the manual states that unused
blocks will be powered down - I assume by the firmware.) If this
causes blocks which had been powered down by the firmware because
they're not being used to then be powered up, that is a regression.
Please check that this is not the case.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/4] net: phy: marvell10g: Fix power-up when strapped to start powered down

[Tobias Waldekranz]

On fre, dec 15, 2023 at 15:44, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Thu, Dec 14, 2023 at 09:14:40PM +0100, Tobias Waldekranz wrote:
>> On devices which are hardware strapped to start powered down (PDSTATE
>> == 1), make sure that we clear the power-down bit on all units
>> affected by this setting.
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>> ---
>>  drivers/net/phy/marvell10g.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>> index 83233b30d7b0..1c1333d867fb 100644
>> --- a/drivers/net/phy/marvell10g.c
>> +++ b/drivers/net/phy/marvell10g.c
>> @@ -344,11 +344,22 @@ static int mv3310_power_down(struct phy_device *phydev)
>>  
>>  static int mv3310_power_up(struct phy_device *phydev)
>>  {
>> +	static const u16 resets[][2] = {
>> +		{ MDIO_MMD_PCS,    MV_PCS_BASE_R    + MDIO_CTRL1 },
>> +		{ MDIO_MMD_PCS,    MV_PCS_1000BASEX + MDIO_CTRL1 },
>
> This is not necessary. The documentation states that the power down
> bit found at each of these is the same physical bit appearing in two
> different locations. So only one is necessary.

Right, I'll remove the entry for 1000BASE-X in v2.

>> +		{ MDIO_MMD_PCS,    MV_PCS_BASE_T    + MDIO_CTRL1 },
>> +		{ MDIO_MMD_PMAPMD, MDIO_CTRL1 },
>> +		{ MDIO_MMD_VEND2,  MV_V2_PORT_CTRL },
>> +	};
>>  	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
>> -	int ret;
>> +	int i, ret;
>>  
>> -	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
>> -				 MV_V2_PORT_CTRL_PWRDOWN);
>> +	for (i = 0; i < ARRAY_SIZE(resets); i++) {
>> +		ret = phy_clear_bits_mmd(phydev, resets[i][0], resets[i][1],
>> +					 MV_V2_PORT_CTRL_PWRDOWN);
>
> While MV_V2_PORT_CTRL_PWRDOWN may correspond with the correct bit for
> the MDIO CTRL1 register, we have MDIO_CTRL1_LPOWER which describes
> this bit. Probably the simplest solution would be to leave the
> existing phy_clear_bits_mmd(), remove the vendor 2 entry from the
> table, and run through that table first.

Yes, I'll fix this in v2.

> Lastly, how does this impact a device which has firmware, and the
> firmware manages the power-down state (the manual states that unused
> blocks will be powered down - I assume by the firmware.) If this
> causes blocks which had been powered down by the firmware because
> they're not being used to then be powered up, that is a regression.
> Please check that this is not the case.

This will be very hard for me to test, as I only have access to boards
without dedicated FLASHes. That said, I don't think I understand how
this is related to how the devices load their firmware. As I understand
it, we should pick up the device in the exact same state after the MDIO
load as we would if it had booted on its own, via a serial FLASH.

The selection of PDSTATE, based on the sample-at-reset pins, is
independent of how firmware is loaded.

From the manual:

    The following registers can be set to force the units to power down.

I interpret this as the power-down bits only acting as gates to stop
firmware from powering up a particular unit. Conversely, clearing one of
these bits merely indicates that the firmware is free to power up the
unit in question.

On a device strapped to PDSTATE==0, I would expect all of these bits to
already be cleared, since the manual states that the value of PDSTATE is
latched into all these bits at reset.

[PATCH net-next 3/4] net: phy: marvell10g: Add LED support for 88X3310

[Tobias Waldekranz]

Pickup the LEDs from the state in which the hardware reset or
bootloader left them, but also support further configuration via
device tree. This is primarily needed because the electrical polarity
and drive behavior is software controlled and not possible to set via
hardware strapping.

Trigger support:
- "none"
- "timer": Map 60-100 ms periods to the fast rate (81ms) and 1000-1600
  	   ms periods to the slow rate (1300ms). Defer everything else to
	   software blinking
- "netdev": Offload link or duplex information to the solid behavior;
  	    tx and/or rx activity to blink behavior.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/net/phy/marvell10g.c | 436 +++++++++++++++++++++++++++++++++++
 1 file changed, 436 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 1c1333d867fb..73d74977dd05 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -28,6 +28,7 @@
 #include <linux/firmware.h>
 #include <linux/hwmon.h>
 #include <linux/marvell_phy.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/sfp.h>
 #include <linux/netdevice.h>
@@ -136,6 +137,14 @@ enum {
 	MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_NO_SGMII_AN	= 0x5,
 	MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER_RATE_MATCH	= 0x6,
 	MV_V2_33X0_PORT_CTRL_MACTYPE_USXGMII			= 0x7,
+	MV_V2_LED0_CONTROL	= 0xf020,
+	MV_V2_LED_CONTROL_POLARITY_MASK		= 0x0003,
+	MV_V2_LED_CONTROL_POLARITY_SHIFT	= 0,
+	MV_V2_LED_CONTROL_BLINK_RATE		= BIT(2),
+	MV_V2_LED_CONTROL_SOLID_FUNC_MASK	= 0x00f8,
+	MV_V2_LED_CONTROL_SOLID_FUNC_SHIFT	= 3,
+	MV_V2_LED_CONTROL_BLINK_FUNC_MASK	= 0x1f00,
+	MV_V2_LED_CONTROL_BLINK_FUNC_SHIFT	= 8,
 	MV_V2_PORT_INTR_STS		= 0xf040,
 	MV_V2_PORT_INTR_MASK		= 0xf043,
 	MV_V2_PORT_INTR_STS_WOL_EN	= BIT(8),
@@ -164,6 +173,7 @@ struct mv3310_mactype {
 struct mv3310_chip {
 	bool (*has_downshift)(struct phy_device *phydev);
 	void (*init_supported_interfaces)(unsigned long *mask);
+	int (*leds_probe)(struct phy_device *phydev);
 	int (*get_mactype)(struct phy_device *phydev);
 	int (*set_mactype)(struct phy_device *phydev, int mactype);
 	int (*select_mactype)(unsigned long *interfaces);
@@ -177,6 +187,13 @@ struct mv3310_chip {
 #endif
 };
 
+#define MV3310_N_LEDS 4
+
+struct mv3310_led {
+	u8 index;
+	u16 fw_ctrl;
+};
+
 struct mv3310_priv {
 	DECLARE_BITMAP(supported_interfaces, PHY_INTERFACE_MODE_MAX);
 	const struct mv3310_mactype *mactype;
@@ -186,6 +203,8 @@ struct mv3310_priv {
 
 	struct device *hwmon_dev;
 	char *hwmon_name;
+
+	struct mv3310_led led[MV3310_N_LEDS];
 };
 
 static const struct mv3310_chip *to_mv3310_chip(struct phy_device *phydev)
@@ -193,6 +212,413 @@ static const struct mv3310_chip *to_mv3310_chip(struct phy_device *phydev)
 	return phydev->drv->driver_data;
 }
 
+enum mv3310_led_func {
+	MV3310_LED_FUNC_OFF		 = 0x00,
+	MV3310_LED_FUNC_TX_RX		 = 0x01,
+	MV3310_LED_FUNC_TX		 = 0x02,
+	MV3310_LED_FUNC_RX		 = 0x03,
+	MV3310_LED_FUNC_COLLISION	 = 0x04,
+	MV3310_LED_FUNC_LINK_COPPER	 = 0x05,
+	MV3310_LED_FUNC_LINK_FIBER	 = 0x06,
+	MV3310_LED_FUNC_LINK		 = 0x07,
+	MV3310_LED_FUNC_10M_LINK	 = 0x08,
+	MV3310_LED_FUNC_100M_LINK	 = 0x09,
+	MV3310_LED_FUNC_1G_LINK		 = 0x0a,
+	MV3310_LED_FUNC_10G_LINK	 = 0x0b,
+	MV3310_LED_FUNC_10M_100M_LINK	 = 0x0c,
+	MV3310_LED_FUNC_10M_100M_1G_LINK = 0x0d,
+	MV3310_LED_FUNC_100M_10G_LINK	 = 0x0e,
+	MV3310_LED_FUNC_1G_10G_LINK	 = 0x0f,
+	MV3310_LED_FUNC_1G_10G_SLAVE	 = 0x10,
+	MV3310_LED_FUNC_1G_10G_MASTER	 = 0x11,
+	MV3310_LED_FUNC_HALF_DUPLEX	 = 0x12,
+	MV3310_LED_FUNC_FULL_DUPLEX	 = 0x13,
+	MV3310_LED_FUNC_LINK_EEE	 = 0x14,
+	MV3310_LED_FUNC_2G5_LINK	 = 0x15,
+	MV3310_LED_FUNC_5G_LINK		 = 0x16,
+	MV3310_LED_FUNC_ON		 = 0x17,
+	MV3310_LED_FUNC_2G5_5G_SLAVE	 = 0x18,
+	MV3310_LED_FUNC_2G5_5G_MASTER	 = 0x19,
+
+	MV3310_LED_FUNC_SPEED_BLINK	 = 0x1f
+};
+
+static int mv3310_led_flags_from_funcs(struct mv3310_led *led,
+				       enum mv3310_led_func solid,
+				       enum mv3310_led_func blink,
+				       unsigned long *flagsp)
+{
+	unsigned long flags = 0;
+
+	switch (solid) {
+	case MV3310_LED_FUNC_OFF:
+		break;
+	case MV3310_LED_FUNC_LINK_COPPER:
+	case MV3310_LED_FUNC_LINK_FIBER:
+	case MV3310_LED_FUNC_LINK:
+		flags |= TRIGGER_NETDEV_LINK;
+		break;
+	case MV3310_LED_FUNC_HALF_DUPLEX:
+		flags |= TRIGGER_NETDEV_HALF_DUPLEX;
+		break;
+	case MV3310_LED_FUNC_FULL_DUPLEX:
+		flags |= TRIGGER_NETDEV_FULL_DUPLEX;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (blink) {
+	case MV3310_LED_FUNC_OFF:
+		break;
+	case MV3310_LED_FUNC_TX:
+		flags |= TRIGGER_NETDEV_TX;
+		break;
+	case MV3310_LED_FUNC_RX:
+		flags |= TRIGGER_NETDEV_RX;
+		break;
+	case MV3310_LED_FUNC_TX_RX:
+		flags |= TRIGGER_NETDEV_TX | TRIGGER_NETDEV_RX;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*flagsp = flags;
+	return 0;
+}
+
+static int mv3310_led_funcs_from_flags(struct mv3310_led *led,
+				       unsigned long flags,
+				       enum mv3310_led_func *solid,
+				       enum mv3310_led_func *blink)
+{
+	unsigned long activity, duplex, link;
+
+	if (flags & ~(BIT(TRIGGER_NETDEV_LINK) |
+		      BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
+		      BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
+		      BIT(TRIGGER_NETDEV_TX) |
+		      BIT(TRIGGER_NETDEV_RX)))
+		return -EINVAL;
+
+	link = flags & BIT(TRIGGER_NETDEV_LINK);
+
+	duplex = flags & (BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
+			  BIT(TRIGGER_NETDEV_FULL_DUPLEX));
+
+	activity = flags & (BIT(TRIGGER_NETDEV_TX) |
+			    BIT(TRIGGER_NETDEV_RX));
+
+	if (link && duplex)
+		return -EINVAL;
+
+	if (solid) {
+		if (link) {
+			*solid = MV3310_LED_FUNC_LINK;
+		} else if (duplex) {
+			switch (duplex) {
+			case BIT(TRIGGER_NETDEV_HALF_DUPLEX):
+				*solid = MV3310_LED_FUNC_HALF_DUPLEX;
+				break;
+			case BIT(TRIGGER_NETDEV_FULL_DUPLEX):
+				*solid = MV3310_LED_FUNC_FULL_DUPLEX;
+				break;
+			default:
+				break;
+			}
+		} else {
+			*solid = MV3310_LED_FUNC_OFF;
+		}
+	}
+
+	if (blink) {
+		switch (activity) {
+		case 0:
+			*blink = MV3310_LED_FUNC_OFF;
+			break;
+		case BIT(TRIGGER_NETDEV_TX):
+			*blink = MV3310_LED_FUNC_TX;
+			break;
+		case BIT(TRIGGER_NETDEV_RX):
+			*blink = MV3310_LED_FUNC_RX;
+			break;
+		default:
+			*blink = MV3310_LED_FUNC_TX_RX;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int mv3310_led_get(struct phy_device *phydev,
+			  struct mv3310_led *led,
+			  enum mv3310_led_func *solid,
+			  enum mv3310_led_func *blink,
+			  bool *slow_blink)
+{
+	int ctrl;
+
+	ctrl = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+			    MV_V2_LED0_CONTROL + led->index);
+	if (ctrl < 0)
+		return ctrl;
+
+	*solid = (ctrl & MV_V2_LED_CONTROL_SOLID_FUNC_MASK) >>
+		MV_V2_LED_CONTROL_SOLID_FUNC_SHIFT;
+	*blink = (ctrl & MV_V2_LED_CONTROL_BLINK_FUNC_MASK) >>
+		MV_V2_LED_CONTROL_BLINK_FUNC_SHIFT;
+
+	*slow_blink = !!(ctrl & MV_V2_LED_CONTROL_BLINK_RATE);
+	return 0;
+}
+
+static int mv3310_led_set(struct phy_device *phydev,
+			  struct mv3310_led *led,
+			  enum mv3310_led_func solid,
+			  enum mv3310_led_func blink,
+			  bool slow_blink)
+{
+	u16 ctrl = led->fw_ctrl;
+
+	ctrl &= ~MV_V2_LED_CONTROL_SOLID_FUNC_MASK;
+	ctrl &= ~MV_V2_LED_CONTROL_BLINK_FUNC_MASK;
+	ctrl &= ~MV_V2_LED_CONTROL_BLINK_RATE;
+
+	ctrl |= solid << MV_V2_LED_CONTROL_SOLID_FUNC_SHIFT;
+	ctrl |= blink << MV_V2_LED_CONTROL_BLINK_FUNC_SHIFT;
+
+	if (slow_blink)
+		ctrl |= MV_V2_LED_CONTROL_BLINK_RATE;
+
+	return phy_write_mmd(phydev, MDIO_MMD_VEND2,
+			     MV_V2_LED0_CONTROL + led->index, ctrl);
+}
+
+enum mv3310_led_polarity {
+	MV3310_LED_POLARITY_ACTIVE_LOW = 0x0,
+	MV3310_LED_POLARITY_ACTIVE_HIGH = 0x1,
+	MV3310_LED_POLARITY_ACTIVE_LOW_TRISTATE = 0x2,
+	MV3310_LED_POLARITY_ACTIVE_HIGH_TRISTATE = 0x3,
+};
+
+static const char * const mv3310_led_polarity_name[] = {
+	[MV3310_LED_POLARITY_ACTIVE_LOW]	   = "active-low",
+	[MV3310_LED_POLARITY_ACTIVE_HIGH]	   = "active-high",
+	[MV3310_LED_POLARITY_ACTIVE_LOW_TRISTATE]  = "active-low-tristate",
+	[MV3310_LED_POLARITY_ACTIVE_HIGH_TRISTATE] = "active-high-tristate",
+};
+
+static int mv3310_led_polarity_from_str(const char *str,
+					enum mv3310_led_polarity *polarity)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(mv3310_led_polarity_name); i++) {
+		if (!mv3310_led_polarity_name[i])
+			continue;
+
+		if (!strcmp(mv3310_led_polarity_name[i], str)) {
+			*polarity = i;
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
+
+static int mv3310_led_probe_of(struct phy_device *phydev,
+			       struct device_node *np)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	enum mv3310_led_polarity polarity;
+	struct mv3310_led *led;
+	const char *str;
+	u32 index;
+	u16 ctrl;
+	int err;
+
+	err = of_property_read_u32(np, "reg", &index);
+	if (err)
+		return err;
+
+	if (index >= MV3310_N_LEDS)
+		return -EINVAL;
+
+	led = &priv->led[index];
+	ctrl = led->fw_ctrl;
+
+	err = of_property_read_string(np, "marvell,polarity", &str);
+	err = err ? : mv3310_led_polarity_from_str(str, &polarity);
+	if (!err) {
+		ctrl &= ~MV_V2_LED_CONTROL_POLARITY_MASK;
+		ctrl |= polarity << MV_V2_LED_CONTROL_POLARITY_SHIFT;
+	}
+
+	if (ctrl != led->fw_ctrl) {
+		led->fw_ctrl = ctrl;
+
+		return phy_write_mmd(phydev, MDIO_MMD_VEND2,
+				     MV_V2_LED0_CONTROL + index, ctrl);
+	}
+
+	return 0;
+}
+
+static int mv3310_leds_probe(struct phy_device *phydev)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *pnp, *np;
+	int err, val, index;
+
+	/* Save the config left by HW reset or bootloader, to make
+	 * sure that we do not loose any polarity config made by
+	 * firmware. This will be overridden by info from DT, if
+	 * available.
+	 */
+	for (index = 0; index < MV3310_N_LEDS; index++) {
+		val = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+				   MV_V2_LED0_CONTROL + index);
+		if (val < 0)
+			return val;
+
+		priv->led[index] = (struct mv3310_led) {
+			.index = index,
+			.fw_ctrl = val,
+		};
+	}
+
+	if (!node)
+		return 0;
+
+	pnp = of_get_child_by_name(node, "leds");
+	if (!pnp)
+		return 0;
+
+	for_each_available_child_of_node(pnp, np) {
+		err = mv3310_led_probe_of(phydev, np);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int mv3310_led_brightness_set(struct phy_device *phydev,
+				     u8 index, enum led_brightness value)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	enum mv3310_led_func solid;
+	struct mv3310_led *led;
+
+	if (index >= MV3310_N_LEDS)
+		return -ENODEV;
+
+	led = &priv->led[index];
+
+	if (value == LED_OFF)
+		solid = MV3310_LED_FUNC_OFF;
+	else
+		solid = MV3310_LED_FUNC_ON;
+
+	return mv3310_led_set(phydev, led, solid, MV3310_LED_FUNC_OFF, false);
+}
+
+static int mv3310_led_blink_set(struct phy_device *phydev, u8 index,
+				unsigned long *delay_on,
+				unsigned long *delay_off)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	struct mv3310_led *led;
+	bool slow_blink;
+	int err;
+
+	if (index >= MV3310_N_LEDS)
+		return -ENODEV;
+
+	led = &priv->led[index];
+
+	if (*delay_on != *delay_off)
+		/* Defer anything other than 50% duty cycles to
+		 * software.
+		 */
+		return -EINVAL;
+
+	/* Accept values within ~20% of our supported rates (80ms or
+	 * 1300ms periods).
+	 */
+	if ((*delay_on >= 30) && (*delay_on <= 50))
+		slow_blink = false;
+	else if (((*delay_on >= 500) && (*delay_on <= 800)) || (*delay_on == 0))
+		slow_blink = true;
+	else
+		return -EINVAL;
+
+	err = mv3310_led_set(phydev, led, MV3310_LED_FUNC_OFF,
+			     MV3310_LED_FUNC_ON, slow_blink);
+	if (!err)
+		*delay_on = *delay_off = slow_blink ? 650 : 40;
+
+	return err;
+}
+
+static int mv3310_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				      unsigned long flags)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	struct mv3310_led *led;
+
+	if (index >= MV3310_N_LEDS)
+		return -ENODEV;
+
+	led = &priv->led[index];
+
+	return mv3310_led_funcs_from_flags(led, flags, NULL, NULL);
+}
+
+static int mv3310_led_hw_control_set(struct phy_device *phydev, u8 index,
+				     unsigned long flags)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	enum mv3310_led_func solid, blink;
+	struct mv3310_led *led;
+	int err;
+
+	if (index >= MV3310_N_LEDS)
+		return -ENODEV;
+
+	led = &priv->led[index];
+
+	err = mv3310_led_funcs_from_flags(led, flags, &solid, &blink);
+	if (err)
+		return err;
+
+	return mv3310_led_set(phydev, led, solid, blink, false);
+}
+
+static int mv3310_led_hw_control_get(struct phy_device *phydev, u8 index,
+				     unsigned long *flags)
+{
+	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
+	enum mv3310_led_func solid, blink;
+	struct mv3310_led *led;
+	bool slow_blink;
+	int err;
+
+	if (index >= MV3310_N_LEDS)
+		return -ENODEV;
+
+	led = &priv->led[index];
+
+	err = mv3310_led_get(phydev, led, &solid, &blink, &slow_blink);
+	if (err)
+		return err;
+
+	return  mv3310_led_flags_from_funcs(led, solid, blink, flags);
+}
+
 #ifdef CONFIG_HWMON
 static umode_t mv3310_hwmon_is_visible(const void *data,
 				       enum hwmon_sensor_types type,
@@ -719,6 +1145,10 @@ static int mv3310_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	ret = chip->leds_probe ? chip->leds_probe(phydev) : 0;
+	if (ret)
+		return ret;
+
 	chip->init_supported_interfaces(priv->supported_interfaces);
 
 	return phy_sfp_probe(phydev, &mv3310_sfp_ops);
@@ -1371,6 +1801,7 @@ static void mv2111_init_supported_interfaces(unsigned long *mask)
 static const struct mv3310_chip mv3310_type = {
 	.has_downshift = mv3310_has_downshift,
 	.init_supported_interfaces = mv3310_init_supported_interfaces,
+	.leds_probe = mv3310_leds_probe,
 	.get_mactype = mv3310_get_mactype,
 	.set_mactype = mv3310_set_mactype,
 	.select_mactype = mv3310_select_mactype,
@@ -1579,6 +2010,11 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_loopback	= genphy_c45_loopback,
 		.get_wol	= mv3110_get_wol,
 		.set_wol	= mv3110_set_wol,
+		.led_brightness_set = mv3310_led_brightness_set,
+		.led_blink_set	= mv3310_led_blink_set,
+		.led_hw_is_supported = mv3310_led_hw_is_supported,
+		.led_hw_control_set = mv3310_led_hw_control_set,
+		.led_hw_control_get = mv3310_led_hw_control_get,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88X3310,
-- 
2.34.1

Re: [PATCH net-next 3/4] net: phy: marvell10g: Add LED support for 88X3310

[Andrew Lunn]

> +static int mv3310_led_funcs_from_flags(struct mv3310_led *led,
> +				       unsigned long flags,
> +				       enum mv3310_led_func *solid,
> +				       enum mv3310_led_func *blink)
> +{
> +	unsigned long activity, duplex, link;
> +
> +	if (flags & ~(BIT(TRIGGER_NETDEV_LINK) |
> +		      BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
> +		      BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
> +		      BIT(TRIGGER_NETDEV_TX) |
> +		      BIT(TRIGGER_NETDEV_RX)))
> +		return -EINVAL;

This probably should be -EOPNOTSUPP. The trigger will then do the
blinking in software.

> +
> +	link = flags & BIT(TRIGGER_NETDEV_LINK);
> +
> +	duplex = flags & (BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
> +			  BIT(TRIGGER_NETDEV_FULL_DUPLEX));
> +
> +	activity = flags & (BIT(TRIGGER_NETDEV_TX) |
> +			    BIT(TRIGGER_NETDEV_RX));
> +
> +	if (link && duplex)
> +		return -EINVAL;

It is an odd combination, but again, if the hardware cannot do it,
return -EOPNOTSUPP and leave it to the software.

       Andrew

Re: [PATCH net-next 3/4] net: phy: marvell10g: Add LED support for 88X3310

[Russell King (Oracle)]

On Fri, Dec 15, 2023 at 03:44:07PM +0100, Andrew Lunn wrote:
> > +static int mv3310_led_funcs_from_flags(struct mv3310_led *led,
> > +				       unsigned long flags,
> > +				       enum mv3310_led_func *solid,
> > +				       enum mv3310_led_func *blink)
> > +{
> > +	unsigned long activity, duplex, link;
> > +
> > +	if (flags & ~(BIT(TRIGGER_NETDEV_LINK) |
> > +		      BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
> > +		      BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
> > +		      BIT(TRIGGER_NETDEV_TX) |
> > +		      BIT(TRIGGER_NETDEV_RX)))
> > +		return -EINVAL;
> 
> This probably should be -EOPNOTSUPP. The trigger will then do the
> blinking in software.
> 
> > +
> > +	link = flags & BIT(TRIGGER_NETDEV_LINK);
> > +
> > +	duplex = flags & (BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
> > +			  BIT(TRIGGER_NETDEV_FULL_DUPLEX));
> > +
> > +	activity = flags & (BIT(TRIGGER_NETDEV_TX) |
> > +			    BIT(TRIGGER_NETDEV_RX));
> > +
> > +	if (link && duplex)
> > +		return -EINVAL;
> 
> It is an odd combination, but again, if the hardware cannot do it,
> return -EOPNOTSUPP and leave it to the software.

I don't recall how the LED triggers work (whether they logically OR
or AND). The hardware supports indicating whether it has a half or
full duplex link, and if the LED is programmed for that, then it will
illuminate when it has link _and_ the duplex is of the specified type.
If the link is down, or the duplex is of the other type, then it won't.

I'm guessing that if link is set and duplex is set, then what userspace
is asking for is the LED to be illuminated whenever the link is up _or_
the duplex is of the specified type, which basically logically resolves
to _only_ "link is up" (because a link that is down has no duplex.)

So, I'm not sure "leaving it to software" is good, that combination is
effectively just "has link".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 3/4] net: phy: marvell10g: Add LED support for 88X3310

[Simon Horman]

On Thu, Dec 14, 2023 at 09:14:41PM +0100, Tobias Waldekranz wrote:
> Pickup the LEDs from the state in which the hardware reset or
> bootloader left them, but also support further configuration via
> device tree. This is primarily needed because the electrical polarity
> and drive behavior is software controlled and not possible to set via
> hardware strapping.
> 
> Trigger support:
> - "none"
> - "timer": Map 60-100 ms periods to the fast rate (81ms) and 1000-1600
>   	   ms periods to the slow rate (1300ms). Defer everything else to
> 	   software blinking
> - "netdev": Offload link or duplex information to the solid behavior;
>   	    tx and/or rx activity to blink behavior.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

...

> +static int mv3310_leds_probe(struct phy_device *phydev)
> +{
> +	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct device_node *pnp, *np;
> +	int err, val, index;
> +
> +	/* Save the config left by HW reset or bootloader, to make
> +	 * sure that we do not loose any polarity config made by
> +	 * firmware. This will be overridden by info from DT, if
> +	 * available.
> +	 */
> +	for (index = 0; index < MV3310_N_LEDS; index++) {
> +		val = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> +				   MV_V2_LED0_CONTROL + index);
> +		if (val < 0)
> +			return val;
> +
> +		priv->led[index] = (struct mv3310_led) {
> +			.index = index,
> +			.fw_ctrl = val,
> +		};
> +	}
> +
> +	if (!node)
> +		return 0;
> +
> +	pnp = of_get_child_by_name(node, "leds");
> +	if (!pnp)
> +		return 0;
> +
> +	for_each_available_child_of_node(pnp, np) {
> +		err = mv3310_led_probe_of(phydev, np);
> +		if (err)

Hi Tobias,

I think a call to of_node_put(np) is required here to avoid leaking a
reference.

Flagged by Coccinelle.

> +			return err;
> +	}
> +
> +	return 0;
> +}

...

[PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Tobias Waldekranz]

Hardware supports multiple ways of driving attached LEDs, but this is
not configurable via any sample-at-reset pins - rather it must be set
via software.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 .../bindings/net/marvell,marvell10g.yaml      | 60 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/marvell,marvell10g.yaml

diff --git a/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml b/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
new file mode 100644
index 000000000000..37ff7fdfdd3d
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/marvell,marvell10g.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Alaska X 10G Ethernet PHY
+
+maintainers:
+  - Tobias Waldekranz <tobias@waldekranz.com>
+
+description: |
+  Bindings for Marvell Alaska X 10G Ethernet PHYs
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+properties:
+  leds:
+    type: object
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      '^led@[a-f0-9]+$':
+        $ref: /schemas/leds/common.yaml#
+
+        properties:
+          marvell,polarity:
+            description: |
+              Electrical polarity and drive type for this LED. In the
+              active state, hardware may drive the pin either low or
+              high. In the inactive state, the pin can either be
+              driven to the opposite logic level, or be tristated.
+            $ref: /schemas/types.yaml#/definitions/string
+            enum:
+              - active-low
+              - active-high
+              - active-low-tristate
+              - active-high-tristate
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-phy@0 {
+            reg = <0>;
+
+            marvell,polarity = "active-low-tristate";
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index a151988646fe..2def66789f9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12849,6 +12849,7 @@ M:	Russell King <linux@armlinux.org.uk>
 M:	Marek Behún <kabel@kernel.org>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
 F:	drivers/net/phy/marvell10g.c
 
 MARVELL MVEBU THERMAL DRIVER
-- 
2.34.1

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Krzysztof Kozlowski]

On 14/12/2023 21:14, Tobias Waldekranz wrote:
> Hardware supports multiple ways of driving attached LEDs, but this is
> not configurable via any sample-at-reset pins - rather it must be set
> via software.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  .../bindings/net/marvell,marvell10g.yaml      | 60 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml b/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
> new file mode 100644
> index 000000000000..37ff7fdfdd3d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/marvell,marvell10g.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/marvell,marvell10g.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Alaska X 10G Ethernet PHY
> +
> +maintainers:
> +  - Tobias Waldekranz <tobias@waldekranz.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.

> +  Bindings for Marvell Alaska X 10G Ethernet PHYs

Drop Bindings for and describe the hardware. You are repeating title, so
it is useless.

> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +properties:

How is this schema selected/applied? I guess you have exactly the same
problem as recently talked about other ethernet PHY bindings.

See:
https://lore.kernel.org/linux-devicetree/20231209014828.28194-1-ansuelsmth@gmail.com/

> +  leds:
> +    type: object
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      '^led@[a-f0-9]+$':
> +        $ref: /schemas/leds/common.yaml#

Are you sure you need to repeat all this?

> +
> +        properties:
> +          marvell,polarity:
> +            description: |
> +              Electrical polarity and drive type for this LED. In the
> +              active state, hardware may drive the pin either low or
> +              high. In the inactive state, the pin can either be
> +              driven to the opposite logic level, or be tristated.
> +            $ref: /schemas/types.yaml#/definitions/string
> +            enum:
> +              - active-low
> +              - active-high
> +              - active-low-tristate
> +              - active-high-tristate
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy@0 {
> +            reg = <0>;
> +
> +            marvell,polarity = "active-low-tristate";

It is clearly visible here that your schema is an no-op. You do not
allow such property in the phy, but in leds!



Best regards,
Krzysztof

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Andrew Lunn]

> +        properties:
> +          marvell,polarity:
> +            description: |
> +              Electrical polarity and drive type for this LED. In the
> +              active state, hardware may drive the pin either low or
> +              high. In the inactive state, the pin can either be
> +              driven to the opposite logic level, or be tristated.
> +            $ref: /schemas/types.yaml#/definitions/string
> +            enum:
> +              - active-low
> +              - active-high
> +              - active-low-tristate
> +              - active-high-tristate

Christian is working on adding a generic active-low property, which
any PHY LED could use. The assumption being if the bool property is
not present, it defaults to active-high.

So we should consider, how popular are these two tristate values? Is
this a Marvell only thing, or do other PHYs also have them? Do we want
to make them part of the generic PHY led binding? Also, is an enum the
correct representation? Maybe tristate should be another bool
property? Hi/Low and tristate seem to be orthogonal, so maybe two
properties would make it cleaner with respect to generic properties?

Please work with Christian on this.

Thanks
	Andrew

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Christian Marangi]

> > +        properties:
> > +          marvell,polarity:
> > +            description: |
> > +              Electrical polarity and drive type for this LED. In the
> > +              active state, hardware may drive the pin either low or
> > +              high. In the inactive state, the pin can either be
> > +              driven to the opposite logic level, or be tristated.
> > +            $ref: /schemas/types.yaml#/definitions/string
> > +            enum:
> > +              - active-low
> > +              - active-high
> > +              - active-low-tristate
> > +              - active-high-tristate
> 
> Christian is working on adding a generic active-low property, which
> any PHY LED could use. The assumption being if the bool property is
> not present, it defaults to active-high.
> 

Hi, it was pointed out this series sorry for not noticing before.

> So we should consider, how popular are these two tristate values? Is
> this a Marvell only thing, or do other PHYs also have them? Do we want
> to make them part of the generic PHY led binding? Also, is an enum the
> correct representation? Maybe tristate should be another bool
> property? Hi/Low and tristate seem to be orthogonal, so maybe two
> properties would make it cleaner with respect to generic properties?

For parsing it would make it easier to have the thing split.

But on DT I feel an enum like it's done here might be more clear.

Assuming the property define the LED polarity, it would make sense
to have a single one instead of a sum of boolean.

The boolean idea might be problematic in the future for device that
devisates from what we expect.

Example: A device set the LED to active-high by default and we want a
way in DT to define active-low. With the boolean idea of having
"active-high" and assume active-low if not defined we would have to put
active-high in every PHY node (to reflect the default settings)

Having a property instead permits us to support more case.

Ideally on code side we would have an enum that map the string to the
different modes and we would pass to a .led_set_polarity the enum.
(or if we really want a bitmask)


If we feel tristate is special enough we can consider leaving that
specific to marvell (something like marvell,led-tristate)

But if we notice it's more generic then we will have to keep
compatibility for both.

> 
> Please work with Christian on this.

Think since the current idea is to support this in the LED api with set
polarity either the 2 series needs to be merged or the polarity part
needs to be detached and submitted later until we sort the generic way
to set it?

-- 
	Ansuel

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Andrew Lunn]

On Fri, Dec 15, 2023 at 03:22:11PM +0100, Christian Marangi wrote:
> > > +        properties:
> > > +          marvell,polarity:
> > > +            description: |
> > > +              Electrical polarity and drive type for this LED. In the
> > > +              active state, hardware may drive the pin either low or
> > > +              high. In the inactive state, the pin can either be
> > > +              driven to the opposite logic level, or be tristated.
> > > +            $ref: /schemas/types.yaml#/definitions/string
> > > +            enum:
> > > +              - active-low
> > > +              - active-high
> > > +              - active-low-tristate
> > > +              - active-high-tristate
> > 
> > Christian is working on adding a generic active-low property, which
> > any PHY LED could use. The assumption being if the bool property is
> > not present, it defaults to active-high.
> > 
> 
> Hi, it was pointed out this series sorry for not noticing before.
> 
> > So we should consider, how popular are these two tristate values? Is
> > this a Marvell only thing, or do other PHYs also have them? Do we want
> > to make them part of the generic PHY led binding? Also, is an enum the
> > correct representation? Maybe tristate should be another bool
> > property? Hi/Low and tristate seem to be orthogonal, so maybe two
> > properties would make it cleaner with respect to generic properties?
> 
> For parsing it would make it easier to have the thing split.
> 
> But on DT I feel an enum like it's done here might be more clear.

I took a look at a datasheet for a standalone 1G Marvell PHY. It has
the same capabilities. So this is something which can be reused by a
few devices.

So an enum in DT, and an enum for the API to the PHY driver seems like
a good idea. I doubt there will be too many more variants, but it does
give us an easy way to add more values.

	Andrew

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Christian Marangi]

On Fri, Dec 15, 2023 at 03:22:11PM +0100, Christian Marangi wrote:
> > > +        properties:
> > > +          marvell,polarity:
> > > +            description: |
> > > +              Electrical polarity and drive type for this LED. In the
> > > +              active state, hardware may drive the pin either low or
> > > +              high. In the inactive state, the pin can either be
> > > +              driven to the opposite logic level, or be tristated.
> > > +            $ref: /schemas/types.yaml#/definitions/string
> > > +            enum:
> > > +              - active-low
> > > +              - active-high
> > > +              - active-low-tristate
> > > +              - active-high-tristate
> > 
> > Christian is working on adding a generic active-low property, which
> > any PHY LED could use. The assumption being if the bool property is
> > not present, it defaults to active-high.
> > 
> 
> Hi, it was pointed out this series sorry for not noticing before.
> 
> > So we should consider, how popular are these two tristate values? Is
> > this a Marvell only thing, or do other PHYs also have them? Do we want
> > to make them part of the generic PHY led binding? Also, is an enum the
> > correct representation? Maybe tristate should be another bool
> > property? Hi/Low and tristate seem to be orthogonal, so maybe two
> > properties would make it cleaner with respect to generic properties?
> 
> For parsing it would make it easier to have the thing split.
> 
> But on DT I feel an enum like it's done here might be more clear.
> 
> Assuming the property define the LED polarity, it would make sense
> to have a single one instead of a sum of boolean.
> 
> The boolean idea might be problematic in the future for device that
> devisates from what we expect.
> 
> Example: A device set the LED to active-high by default and we want a
> way in DT to define active-low. With the boolean idea of having
> "active-high" and assume active-low if not defined we would have to put
> active-high in every PHY node (to reflect the default settings)
> 
> Having a property instead permits us to support more case.
> 
> Ideally on code side we would have an enum that map the string to the
> different modes and we would pass to a .led_set_polarity the enum.
> (or if we really want a bitmask)
> 
> 
> If we feel tristate is special enough we can consider leaving that
> specific to marvell (something like marvell,led-tristate)
> 
> But if we notice it's more generic then we will have to keep
> compatibility for both.
> 
> > 
> > Please work with Christian on this.
> 
> Think since the current idea is to support this in the LED api with set
> polarity either the 2 series needs to be merged or the polarity part
> needs to be detached and submitted later until we sort the generic way
> to set it?
>

Hi Andrew,

I asked some further info to Tobias. With a better look at the
Documentation, it was notice that tristate is only to drive the LED off.

So to drive LED ON:
- active-low
- active-high

And to drive LED OFF:
- low
- high
- tristate

I feel introducing description to how to drive the LED inactive might be
too much.

Would it be ok to have something like

polarity:
- "active-low"
- "active-high"

And a bool with "marvel,led-inactive-tristate" specific to this PHY?

In alternative we can list all the modes as done in the qca808x series
currently proposed. (more flexible for other PHY and expandable but can
pose the risk of bloating the property with all kind of modes)

PHY driver wise, with the set_polarity function or even probe function
they can handle this with a priv struct and operate in the
set_brightness function for these special handling.

-- 
	Ansuel

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Marek Behún]

On Tue, 19 Dec 2023 11:13:43 +0100
Christian Marangi <ansuelsmth@gmail.com> wrote:

> On Fri, Dec 15, 2023 at 03:22:11PM +0100, Christian Marangi wrote:
> > > > +        properties:
> > > > +          marvell,polarity:
> > > > +            description: |
> > > > +              Electrical polarity and drive type for this LED. In the
> > > > +              active state, hardware may drive the pin either low or
> > > > +              high. In the inactive state, the pin can either be
> > > > +              driven to the opposite logic level, or be tristated.
> > > > +            $ref: /schemas/types.yaml#/definitions/string
> > > > +            enum:
> > > > +              - active-low
> > > > +              - active-high
> > > > +              - active-low-tristate
> > > > +              - active-high-tristate  
> > > 
> > > Christian is working on adding a generic active-low property, which
> > > any PHY LED could use. The assumption being if the bool property is
> > > not present, it defaults to active-high.
> > >   
> > 
> > Hi, it was pointed out this series sorry for not noticing before.
> >   
> > > So we should consider, how popular are these two tristate values? Is
> > > this a Marvell only thing, or do other PHYs also have them? Do we want
> > > to make them part of the generic PHY led binding? Also, is an enum the
> > > correct representation? Maybe tristate should be another bool
> > > property? Hi/Low and tristate seem to be orthogonal, so maybe two
> > > properties would make it cleaner with respect to generic properties?  
> > 
> > For parsing it would make it easier to have the thing split.
> > 
> > But on DT I feel an enum like it's done here might be more clear.
> > 
> > Assuming the property define the LED polarity, it would make sense
> > to have a single one instead of a sum of boolean.
> > 
> > The boolean idea might be problematic in the future for device that
> > devisates from what we expect.
> > 
> > Example: A device set the LED to active-high by default and we want a
> > way in DT to define active-low. With the boolean idea of having
> > "active-high" and assume active-low if not defined we would have to put
> > active-high in every PHY node (to reflect the default settings)
> > 
> > Having a property instead permits us to support more case.
> > 
> > Ideally on code side we would have an enum that map the string to the
> > different modes and we would pass to a .led_set_polarity the enum.
> > (or if we really want a bitmask)
> > 
> > 
> > If we feel tristate is special enough we can consider leaving that
> > specific to marvell (something like marvell,led-tristate)
> > 
> > But if we notice it's more generic then we will have to keep
> > compatibility for both.
> >   
> > > 
> > > Please work with Christian on this.  
> > 
> > Think since the current idea is to support this in the LED api with set
> > polarity either the 2 series needs to be merged or the polarity part
> > needs to be detached and submitted later until we sort the generic way
> > to set it?
> >  
> 
> Hi Andrew,
> 
> I asked some further info to Tobias. With a better look at the
> Documentation, it was notice that tristate is only to drive the LED off.
> 
> So to drive LED ON:
> - active-low
> - active-high
> 
> And to drive LED OFF:
> - low
> - high
> - tristate
> 
> I feel introducing description to how to drive the LED inactive might be
> too much.
> 
> Would it be ok to have something like
> 
> polarity:
> - "active-low"
> - "active-high"
> 
> And a bool with "marvel,led-inactive-tristate" specific to this PHY?
* marvell

The "tristate" in LED off state means high impendance (or
alternatively: open, Z), see:
  https://en.wikipedia.org/wiki/Three-state_logic

Marvell calling this high impedance state "tristate" is IMO confusing,
since "tristate" means 3 state logic, the three states being:
- connected to high voltage
- connected to low voltage
- not connected to any voltage

I would propose something like
  inactive-hi-z;
or even better
  inactive-high-impedance;

Krzysztof, what do you think?

Marek

Re: [PATCH net-next 4/4] dt-bindings: net: marvell10g: Document LED polarity

[Christian Marangi]

On Tue, Dec 19, 2023 at 11:58:07AM +0100, Marek Behún wrote:
> On Tue, 19 Dec 2023 11:13:43 +0100
> Christian Marangi <ansuelsmth@gmail.com> wrote:
> 
> > On Fri, Dec 15, 2023 at 03:22:11PM +0100, Christian Marangi wrote:
> > > > > +        properties:
> > > > > +          marvell,polarity:
> > > > > +            description: |
> > > > > +              Electrical polarity and drive type for this LED. In the
> > > > > +              active state, hardware may drive the pin either low or
> > > > > +              high. In the inactive state, the pin can either be
> > > > > +              driven to the opposite logic level, or be tristated.
> > > > > +            $ref: /schemas/types.yaml#/definitions/string
> > > > > +            enum:
> > > > > +              - active-low
> > > > > +              - active-high
> > > > > +              - active-low-tristate
> > > > > +              - active-high-tristate  
> > > > 
> > > > Christian is working on adding a generic active-low property, which
> > > > any PHY LED could use. The assumption being if the bool property is
> > > > not present, it defaults to active-high.
> > > >   
> > > 
> > > Hi, it was pointed out this series sorry for not noticing before.
> > >   
> > > > So we should consider, how popular are these two tristate values? Is
> > > > this a Marvell only thing, or do other PHYs also have them? Do we want
> > > > to make them part of the generic PHY led binding? Also, is an enum the
> > > > correct representation? Maybe tristate should be another bool
> > > > property? Hi/Low and tristate seem to be orthogonal, so maybe two
> > > > properties would make it cleaner with respect to generic properties?  
> > > 
> > > For parsing it would make it easier to have the thing split.
> > > 
> > > But on DT I feel an enum like it's done here might be more clear.
> > > 
> > > Assuming the property define the LED polarity, it would make sense
> > > to have a single one instead of a sum of boolean.
> > > 
> > > The boolean idea might be problematic in the future for device that
> > > devisates from what we expect.
> > > 
> > > Example: A device set the LED to active-high by default and we want a
> > > way in DT to define active-low. With the boolean idea of having
> > > "active-high" and assume active-low if not defined we would have to put
> > > active-high in every PHY node (to reflect the default settings)
> > > 
> > > Having a property instead permits us to support more case.
> > > 
> > > Ideally on code side we would have an enum that map the string to the
> > > different modes and we would pass to a .led_set_polarity the enum.
> > > (or if we really want a bitmask)
> > > 
> > > 
> > > If we feel tristate is special enough we can consider leaving that
> > > specific to marvell (something like marvell,led-tristate)
> > > 
> > > But if we notice it's more generic then we will have to keep
> > > compatibility for both.
> > >   
> > > > 
> > > > Please work with Christian on this.  
> > > 
> > > Think since the current idea is to support this in the LED api with set
> > > polarity either the 2 series needs to be merged or the polarity part
> > > needs to be detached and submitted later until we sort the generic way
> > > to set it?
> > >  
> > 
> > Hi Andrew,
> > 
> > I asked some further info to Tobias. With a better look at the
> > Documentation, it was notice that tristate is only to drive the LED off.
> > 
> > So to drive LED ON:
> > - active-low
> > - active-high
> > 
> > And to drive LED OFF:
> > - low
> > - high
> > - tristate
> > 
> > I feel introducing description to how to drive the LED inactive might be
> > too much.
> > 
> > Would it be ok to have something like
> > 
> > polarity:
> > - "active-low"
> > - "active-high"
> > 
> > And a bool with "marvel,led-inactive-tristate" specific to this PHY?
> * marvell
> 
> The "tristate" in LED off state means high impendance (or
> alternatively: open, Z), see:
>   https://en.wikipedia.org/wiki/Three-state_logic
> 
> Marvell calling this high impedance state "tristate" is IMO confusing,
> since "tristate" means 3 state logic, the three states being:
> - connected to high voltage
> - connected to low voltage
> - not connected to any voltage
> 
> I would propose something like
>   inactive-hi-z;
> or even better
>   inactive-high-impedance;
> 
> Krzysztof, what do you think?

Considering we want to use a property called polarity that might intend
the full configuration of the LED.

Wonder if
- active-low
- active-high
- active-low-open
- active-high-open

And describe them that in
- active-low
- active-high

low or high voltage is used for the other pin.

And for active-low-open and active-high-open the other pin is not
connected.

But maybe open might be even confusing (since I don't think they are not
connected bu as you said just attached to something high impedance.

-- 
	Ansuel