[PATCH net-next 0/9] net: Add support for Power over Ethernet (PoE)

[PATCH net-next 0/9] net: Add support for Power over Ethernet (PoE)

[Kory Maincent]

This patch series aims at adding support for PoE (Power over Ethernet),
based on the already existing support for PoDL (Power over Data Line)
implementation. In addition, it adds support for one specific PoE
controller, the Microchip PD692x0.

In detail:
- Patch 1 to 6 prepare net to support PoE devices.
- Patch 7 adds a new error code to firmware upload API.
- Patch 8 and 9 add PD692x0 PoE PSE controller driver and its binding.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Kory Maincent (9):
      net: pse-pd: Rectify and adapt the naming of admin_cotrol member of struct pse_control_config
      ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL
      net: pse-pd: Introduce PSE types enumeration
      net: ethtool: pse-pd: Expand pse commands with the PSE PoE interface
      netlink: specs: Modify pse attribute prefix
      netlink: specs: Expand the pse netlink command with PoE interface
      firmware_loader: Expand Firmware upload error codes
      dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller
      net: pse-pd: Add PD692x0 PSE controller driver

 .../bindings/net/pse-pd/microchip,pd692x0_i2c.yaml |   70 ++
 Documentation/netlink/specs/ethtool.yaml           |   33 +-
 Documentation/networking/ethtool-netlink.rst       |   20 +
 MAINTAINERS                                        |    7 +
 drivers/base/firmware_loader/sysfs_upload.c        |    1 +
 drivers/net/pse-pd/Kconfig                         |   11 +
 drivers/net/pse-pd/Makefile                        |    1 +
 drivers/net/pse-pd/pd692x0.c                       | 1049 ++++++++++++++++++++
 drivers/net/pse-pd/pse_core.c                      |    9 +
 drivers/net/pse-pd/pse_regulator.c                 |    9 +-
 include/linux/firmware.h                           |    2 +
 include/linux/pse-pd/pse.h                         |   35 +-
 include/uapi/linux/ethtool.h                       |   43 +
 include/uapi/linux/ethtool_netlink.h               |    3 +
 net/ethtool/pse-pd.c                               |   64 +-
 15 files changed, 1332 insertions(+), 25 deletions(-)
---
base-commit: 23dd60286589d9d49c8135dee937fd54efa5643c
change-id: 20231024-feature_poe-139490e73403

Best regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next 1/9] net: pse-pd: Rectify and adapt the naming of admin_cotrol member of struct pse_control_config

[Kory Maincent]

In commit 18ff0bcda6d1 ("ethtool: add interface to interact with Ethernet
Power Equipment"), the 'pse_control_config' structure was introduced,
housing a single member labeled 'admin_cotrol' responsible for maintaining
the operational state of the PoDL PSE functions.

A noticeable typographical error exists in the naming of this field
('cotrol' should be corrected to 'control'), which this commit aims to
rectify.

Furthermore, with upcoming extensions of this structure to encompass PoE
functionalities, the field is being renamed to 'podl_admin_state' to
distinctly indicate that this state is tailored specifically for PoDL."

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pse_regulator.c | 8 ++++----
 include/linux/pse-pd/pse.h         | 4 ++--
 net/ethtool/pse-pd.c               | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/pse-pd/pse_regulator.c b/drivers/net/pse-pd/pse_regulator.c
index e2bf8306ca90..1dedf4de296e 100644
--- a/drivers/net/pse-pd/pse_regulator.c
+++ b/drivers/net/pse-pd/pse_regulator.c
@@ -31,10 +31,10 @@ pse_reg_ethtool_set_config(struct pse_controller_dev *pcdev, unsigned long id,
 	struct pse_reg_priv *priv = to_pse_reg(pcdev);
 	int ret;
 
-	if (priv->admin_state == config->admin_cotrol)
+	if (priv->admin_state == config->podl_admin_control)
 		return 0;
 
-	switch (config->admin_cotrol) {
+	switch (config->podl_admin_control) {
 	case ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED:
 		ret = regulator_enable(priv->ps);
 		break;
@@ -43,14 +43,14 @@ pse_reg_ethtool_set_config(struct pse_controller_dev *pcdev, unsigned long id,
 		break;
 	default:
 		dev_err(pcdev->dev, "Unknown admin state %i\n",
-			config->admin_cotrol);
+			config->podl_admin_control);
 		ret = -ENOTSUPP;
 	}
 
 	if (ret)
 		return ret;
 
-	priv->admin_state = config->admin_cotrol;
+	priv->admin_state = config->podl_admin_control;
 
 	return 0;
 }
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index fb724c65c77b..199cf4ae3cf2 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -15,11 +15,11 @@ struct pse_controller_dev;
 /**
  * struct pse_control_config - PSE control/channel configuration.
  *
- * @admin_cotrol: set PoDL PSE admin control as described in
+ * @podl_admin_control: set PoDL PSE admin control as described in
  *	IEEE 802.3-2018 30.15.1.2.1 acPoDLPSEAdminControl
  */
 struct pse_control_config {
-	enum ethtool_podl_pse_admin_state admin_cotrol;
+	enum ethtool_podl_pse_admin_state podl_admin_control;
 };
 
 /**
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index cc478af77111..aef57a058f0d 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -130,7 +130,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 	struct phy_device *phydev;
 
 	/* this values are already validated by the ethnl_pse_set_policy */
-	config.admin_cotrol = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]);
+	config.podl_admin_control = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]);
 
 	phydev = dev->phydev;
 	if (!phydev) {

-- 
2.25.1

[PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Kory Maincent]

In the current PSE interface for Ethernet Power Equipment, support is
limited to PoDL. This patch extends the interface to accommodate the
objects specified in IEEE 802.3-2022 145.2 for Power sourcing
Equipment (PSE).

The following objects are now supported and considered mandatory:
- IEEE 802.3-2022 30.9.1.1.5 aPSEPowerDetectionStatus
- IEEE 802.3-2022 30.9.1.1.2 aPSEAdminState
- IEEE 802.3-2022 30.9.1.2.1 aPSEAdminControl

To avoid confusion between "PoDL PSE" and "PSE", which have similar names
but distinct values, we have followed the suggestion of Oleksij Rempel to
maintain separate naming schemes for each. You can find more details in the
discussion thread here:
https://lore.kernel.org/netdev/20230912110637.GI780075@pengutronix.de/

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 include/linux/pse-pd/pse.h           |  9 ++++++++
 include/uapi/linux/ethtool.h         | 43 ++++++++++++++++++++++++++++++++++++
 include/uapi/linux/ethtool_netlink.h |  3 +++
 3 files changed, 55 insertions(+)

diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 199cf4ae3cf2..25490d0c682d 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -17,9 +17,12 @@ struct pse_controller_dev;
  *
  * @podl_admin_control: set PoDL PSE admin control as described in
  *	IEEE 802.3-2018 30.15.1.2.1 acPoDLPSEAdminControl
+ * @admin_control: set PSE admin control as described in
+ *	IEEE 802.3-2022 30.9.1.2.1 acPSEAdminControl
  */
 struct pse_control_config {
 	enum ethtool_podl_pse_admin_state podl_admin_control;
+	enum ethtool_pse_admin_state admin_control;
 };
 
 /**
@@ -29,10 +32,16 @@ struct pse_control_config {
  *	functions. IEEE 802.3-2018 30.15.1.1.2 aPoDLPSEAdminState
  * @podl_pw_status: power detection status of the PoDL PSE.
  *	IEEE 802.3-2018 30.15.1.1.3 aPoDLPSEPowerDetectionStatus:
+ * @admin_state: operational state of the PSE
+ *	functions. IEEE 802.3-2022 30.9.1.1.2 aPSEAdminState
+ * @pw_status: power detection status of the PSE.
+ *	IEEE 802.3-2022 30.9.1.1.5 aPSEPowerDetectionStatus:
  */
 struct pse_control_status {
 	enum ethtool_podl_pse_admin_state podl_admin_state;
 	enum ethtool_podl_pse_pw_d_status podl_pw_status;
+	enum ethtool_pse_admin_state admin_state;
+	enum ethtool_pse_pw_d_status pw_status;
 };
 
 /**
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f7fba0dc87e5..eaf7f7ff41f1 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -752,6 +752,49 @@ enum ethtool_module_power_mode {
 	ETHTOOL_MODULE_POWER_MODE_HIGH,
 };
 
+/**
+ * enum ethtool_pse_admin_state - operational state of the PoDL PSE
+ *	functions. IEEE 802.3-2022 30.9.1.1.2 aPSEAdminState
+ * @ETHTOOL_PSE_ADMIN_STATE_UNKNOWN: state of PSE functions is unknown
+ * @ETHTOOL_PSE_ADMIN_STATE_DISABLED: PSE functions are disabled
+ * @ETHTOOL_PSE_ADMIN_STATE_ENABLED: PSE functions are enabled
+ */
+enum ethtool_pse_admin_state {
+	ETHTOOL_PSE_ADMIN_STATE_UNKNOWN = 1,
+	ETHTOOL_PSE_ADMIN_STATE_DISABLED,
+	ETHTOOL_PSE_ADMIN_STATE_ENABLED,
+};
+
+/**
+ * enum ethtool_pse_pw_d_status - power detection status of the PSE.
+ *	IEEE 802.3-2022 30.9.1.1.3 aPoDLPSEPowerDetectionStatus:
+ * @ETHTOOL_PSE_PW_D_STATUS_UNKNOWN: PSE status is unknown
+ * @ETHTOOL_PSE_PW_D_STATUS_DISABLED: "The enumeration “disabled”
+ *	indicates that the PSE State diagram is in the state DISABLED."
+ * @ETHTOOL_PSE_PW_D_STATUS_SEARCHING: "The enumeration “searching”
+ *	indicates the PSE State diagram is in a state other than those
+ *	listed."
+ * @ETHTOOL_PSE_PW_D_STATUS_DELIVERING: "The enumeration
+ *	“deliveringPower” indicates that the PSE State diagram is in the
+ *	state POWER_ON."
+ * @ETHTOOL_PSE_PW_D_STATUS_TEST: "The enumeration “test” indicates that
+ *	the PSE State diagram is in the state TEST_MODE."
+ * @ETHTOOL_PSE_PW_D_STATUS_FAULT: "The enumeration “fault” indicates that
+ *	the PSE State diagram is in the state TEST_ERROR."
+ * @ETHTOOL_PSE_PW_D_STATUS_OTHERFAULT: "The enumeration “otherFault”
+ *	indicates that the PSE State diagram is in the state IDLE due to
+ *	the variable error_condition = true."
+ */
+enum ethtool_pse_pw_d_status {
+	ETHTOOL_PSE_PW_D_STATUS_UNKNOWN = 1,
+	ETHTOOL_PSE_PW_D_STATUS_DISABLED,
+	ETHTOOL_PSE_PW_D_STATUS_SEARCHING,
+	ETHTOOL_PSE_PW_D_STATUS_DELIVERING,
+	ETHTOOL_PSE_PW_D_STATUS_TEST,
+	ETHTOOL_PSE_PW_D_STATUS_FAULT,
+	ETHTOOL_PSE_PW_D_STATUS_OTHERFAULT,
+};
+
 /**
  * enum ethtool_podl_pse_admin_state - operational state of the PoDL PSE
  *	functions. IEEE 802.3-2018 30.15.1.1.2 aPoDLPSEAdminState
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 73e2c10dc2cc..2a27f37c71f7 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -895,6 +895,9 @@ enum {
 	ETHTOOL_A_PODL_PSE_ADMIN_STATE,		/* u32 */
 	ETHTOOL_A_PODL_PSE_ADMIN_CONTROL,	/* u32 */
 	ETHTOOL_A_PODL_PSE_PW_D_STATUS,		/* u32 */
+	ETHTOOL_A_PSE_ADMIN_STATE,		/* u32 */
+	ETHTOOL_A_PSE_ADMIN_CONTROL,		/* u32 */
+	ETHTOOL_A_PSE_PW_D_STATUS,		/* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_PSE_CNT,

-- 
2.25.1

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Andrew Lunn]

On Thu, Nov 16, 2023 at 03:01:34PM +0100, Kory Maincent wrote:
> In the current PSE interface for Ethernet Power Equipment, support is
> limited to PoDL. This patch extends the interface to accommodate the
> objects specified in IEEE 802.3-2022 145.2 for Power sourcing
> Equipment (PSE).

Sorry for taking a while getting to these patches. Plumbers and other
patches have been keeping me busy.

I'm trying to get my head around naming... Is there some sort of
hierarchy? Is PSE the generic concept for putting power down the
cable? Then you have the sub-type PoDL, and the sub-type PoE?

>  struct pse_control_config {
>  	enum ethtool_podl_pse_admin_state podl_admin_control;
> +	enum ethtool_pse_admin_state admin_control;

When i look at this, it seems to me admin_control should be generic
across all schemes which put power down the cable, and
podl_admin_control is specific to how PoDL puts power down the cable.

Since you appear to be adding support for a second way to put power
down the cable, i would expect something like poe_admin_control being
added here. But maybe that is in a later patch?

      Andrew

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Köry Maincent]

+Oleksij

Sorry forgot to CC you the series.
Maybe you should add yourself to the MAINTAINERS of pse-pd drivers subsystem?

On Sat, 18 Nov 2023 18:38:43 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Nov 16, 2023 at 03:01:34PM +0100, Kory Maincent wrote:
> > In the current PSE interface for Ethernet Power Equipment, support is
> > limited to PoDL. This patch extends the interface to accommodate the
> > objects specified in IEEE 802.3-2022 145.2 for Power sourcing
> > Equipment (PSE).  
> 
> Sorry for taking a while getting to these patches. Plumbers and other
> patches have been keeping me busy.

Don't worry you are doing a great job as a net maintainer and I won't raise any
remarks on delay considering how you are doing your job.
Thanks again for your review!!

> I'm trying to get my head around naming... Is there some sort of
> hierarchy? Is PSE the generic concept for putting power down the
> cable? Then you have the sub-type PoDL, and the sub-type PoE?

In fact as we discussed with Oleksij I decided to keep the naming as close as
possible to the IEEE 802.3 standard.
On the standard the PODL is naming like this aPoDLPSE* (ex: aPoDLPSEAdminState)
and the PSE is naming like this aPSE* (ex: aPSEAdminState) without any PoE
prefix. Maybe it is due to PoE being supported before PoDL and they didn't
expect the PoDL part.

> >  struct pse_control_config {
> >  	enum ethtool_podl_pse_admin_state podl_admin_control;
> > +	enum ethtool_pse_admin_state admin_control;  
> 
> When i look at this, it seems to me admin_control should be generic
> across all schemes which put power down the cable, and
> podl_admin_control is specific to how PoDL puts power down the cable.
>
> Since you appear to be adding support for a second way to put power
> down the cable, i would expect something like poe_admin_control being
> added here. But maybe that is in a later patch?

No as said above admin_control is for PoE and podl_admin_control is for PoDL.
Maybe you prefer to use poe_admin_control, and add poe prefix in the poe
variables. It will differ a bit from the IEEE standard naming but I agreed that
it would be more understandable in the development part.

I am open to the change.
Oleksij do you agree?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Oleksij Rempel]

Köry,

On Mon, Nov 20, 2023 at 11:09:44AM +0100, Köry Maincent wrote:
> +Oleksij
> 
> Sorry forgot to CC you the series.
> Maybe you should add yourself to the MAINTAINERS of pse-pd drivers subsystem?

ack, i'll take a look at this.

> On Sat, 18 Nov 2023 18:38:43 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Thu, Nov 16, 2023 at 03:01:34PM +0100, Kory Maincent wrote:
> > > In the current PSE interface for Ethernet Power Equipment, support is
> > > limited to PoDL. This patch extends the interface to accommodate the
> > > objects specified in IEEE 802.3-2022 145.2 for Power sourcing
> > > Equipment (PSE).  
> > 
> > Sorry for taking a while getting to these patches. Plumbers and other
> > patches have been keeping me busy.
> 
> Don't worry you are doing a great job as a net maintainer and I won't raise any
> remarks on delay considering how you are doing your job.
> Thanks again for your review!!
> 
> > I'm trying to get my head around naming... Is there some sort of
> > hierarchy? Is PSE the generic concept for putting power down the
> > cable? Then you have the sub-type PoDL, and the sub-type PoE?
> 
> In fact as we discussed with Oleksij I decided to keep the naming as close as
> possible to the IEEE 802.3 standard.
> On the standard the PODL is naming like this aPoDLPSE* (ex: aPoDLPSEAdminState)
> and the PSE is naming like this aPSE* (ex: aPSEAdminState) without any PoE
> prefix. Maybe it is due to PoE being supported before PoDL and they didn't
> expect the PoDL part.

"PoE" (initially Power via MDI?) and PoDL have kind of different technologies.
They use different negotiation and need different physical implementation.
IEEE 802.3 standard is trying to be backwards and kind of forwards compatible
for PoE. But not compatible between PoE and PoDL.

In general, it is not just about enabling or disabling power.
"admin_state" == enable is other way to say - "do the right thing".
And the "right thing" may include some kind of communication between PSE
(Power Source Equipment) and PD (Powered Device).

Since, some variants of  Single Pair Ethernet (SPE) are using same
auto negotiation protocol as not SPE variants. I can imagine, that some
day we will see a hybrid (SPE+nonSPE) devices. Wich will need to
support both: PoE and PoDL. I assume, in that case, we wont to be able
to control both variants separately.

This is why I prefer to have mapping of IEEE 802.3 specification to the
user space as close as possible.

> > >  struct pse_control_config {
> > >  	enum ethtool_podl_pse_admin_state podl_admin_control;
> > > +	enum ethtool_pse_admin_state admin_control;  
> > 
> > When i look at this, it seems to me admin_control should be generic
> > across all schemes which put power down the cable, and
> > podl_admin_control is specific to how PoDL puts power down the cable.
> >
> > Since you appear to be adding support for a second way to put power
> > down the cable, i would expect something like poe_admin_control being
> > added here. But maybe that is in a later patch?
> 
> No as said above admin_control is for PoE and podl_admin_control is for PoDL.
> Maybe you prefer to use poe_admin_control, and add poe prefix in the poe
> variables. It will differ a bit from the IEEE standard naming but I agreed that
> it would be more understandable in the development part.

Official name for "PoE" is "Power via Media Dependent Interface". PoE is
not used in the IEEE 802.3-2018. Using names not used in the specification,
make development even harder :)
Especially since there are even more marketing names (names not used in the
specification) for different PoE variants:
- 802.3af (802.3at Type 1), PoE
- 802.3at Type 2, PoE+
- 802.3bt Type 3, 4PPoE or PoE++
- 802.3bt Type 4, 4PPoE or PoE++

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Andrew Lunn]

> > > >  struct pse_control_config {
> > > >  	enum ethtool_podl_pse_admin_state podl_admin_control;
> > > > +	enum ethtool_pse_admin_state admin_control;  
> > > 
> > > When i look at this, it seems to me admin_control should be generic
> > > across all schemes which put power down the cable, and
> > > podl_admin_control is specific to how PoDL puts power down the cable.
> > >
> > > Since you appear to be adding support for a second way to put power
> > > down the cable, i would expect something like poe_admin_control being
> > > added here. But maybe that is in a later patch?
> > 
> > No as said above admin_control is for PoE and podl_admin_control is for PoDL.
> > Maybe you prefer to use poe_admin_control, and add poe prefix in the poe
> > variables. It will differ a bit from the IEEE standard naming but I agreed that
> > it would be more understandable in the development part.
> 
> Official name for "PoE" is "Power via Media Dependent Interface". PoE is
> not used in the IEEE 802.3-2018. Using names not used in the specification,
> make development even harder :)
> Especially since there are even more marketing names (names not used in the
> specification) for different PoE variants:
> - 802.3af (802.3at Type 1), PoE
> - 802.3at Type 2, PoE+
> - 802.3bt Type 3, 4PPoE or PoE++
> - 802.3bt Type 4, 4PPoE or PoE++

From the 2018 standard:

  1.4.407 Power Sourcing Equipment (PSE): A DTE or midspan device that
  provides the power to a single link section. PSEs are defined for
  use with two different types of balanced twisted-pair PHYs. When
  used with 2 or 4 pair balanced twisted-pair (BASE-T) PHYs, (see IEEE
  Std 802.3, Clause 33), DTE powering is intended to provide a single
  10BASE-T, 100BASE-TX, or 1000BASE-T device with a unified interface
  for both the data it requires and the power to process these
  data. When used with single balanced twisted-pair (BASE-T1) PHYs
  (see IEEE Std 802.3, Clause 104), DTE powering is intended to
  provide a single 100BASE-T1 or 1000BASE-T1 device with a unified
  interface for both the data it requires and the power to process
  these data. A PSE used with balanced single twisted-pair PHYs is
  also referred to as a PoDL PSE.

So it seems like, anything not PoDL PSE does not have a name :-(

However, everything not PoDL PSE seems to be clause 33. So how about:

	enum ethtool_podl_pse_admin_state podl_admin_control;
	enum ethtool_c33_pse_admin_state c33_admin_control;  

At least inside the kernel we use c22, c45, c37 etc. I'm not sure they
are visible to userspace, but if we don't have a better name, maybe we
have to use c33 in userspace as well.

I do think naming like this makes it clear we are talking about two
parallel technologies, not a generic layer and then extensions for
podl.

What do you think?

	Andrew

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Oleksij Rempel]

On Mon, Nov 20, 2023 at 07:00:03PM +0100, Andrew Lunn wrote:
> > > > >  struct pse_control_config {
> > > > >  	enum ethtool_podl_pse_admin_state podl_admin_control;
> > > > > +	enum ethtool_pse_admin_state admin_control;  
> > > > 
> > > > When i look at this, it seems to me admin_control should be generic
> > > > across all schemes which put power down the cable, and
> > > > podl_admin_control is specific to how PoDL puts power down the cable.
> > > >
> > > > Since you appear to be adding support for a second way to put power
> > > > down the cable, i would expect something like poe_admin_control being
> > > > added here. But maybe that is in a later patch?
> > > 
> > > No as said above admin_control is for PoE and podl_admin_control is for PoDL.
> > > Maybe you prefer to use poe_admin_control, and add poe prefix in the poe
> > > variables. It will differ a bit from the IEEE standard naming but I agreed that
> > > it would be more understandable in the development part.
> > 
> > Official name for "PoE" is "Power via Media Dependent Interface". PoE is
> > not used in the IEEE 802.3-2018. Using names not used in the specification,
> > make development even harder :)
> > Especially since there are even more marketing names (names not used in the
> > specification) for different PoE variants:
> > - 802.3af (802.3at Type 1), PoE
> > - 802.3at Type 2, PoE+
> > - 802.3bt Type 3, 4PPoE or PoE++
> > - 802.3bt Type 4, 4PPoE or PoE++
> 
> From the 2018 standard:
> 
>   1.4.407 Power Sourcing Equipment (PSE): A DTE or midspan device that
>   provides the power to a single link section. PSEs are defined for
>   use with two different types of balanced twisted-pair PHYs. When
>   used with 2 or 4 pair balanced twisted-pair (BASE-T) PHYs, (see IEEE
>   Std 802.3, Clause 33), DTE powering is intended to provide a single
>   10BASE-T, 100BASE-TX, or 1000BASE-T device with a unified interface
>   for both the data it requires and the power to process these
>   data. When used with single balanced twisted-pair (BASE-T1) PHYs
>   (see IEEE Std 802.3, Clause 104), DTE powering is intended to
>   provide a single 100BASE-T1 or 1000BASE-T1 device with a unified
>   interface for both the data it requires and the power to process
>   these data. A PSE used with balanced single twisted-pair PHYs is
>   also referred to as a PoDL PSE.
> 
> So it seems like, anything not PoDL PSE does not have a name :-(
> 
> However, everything not PoDL PSE seems to be clause 33. So how about:
> 
> 	enum ethtool_podl_pse_admin_state podl_admin_control;
> 	enum ethtool_c33_pse_admin_state c33_admin_control;  
> 
> At least inside the kernel we use c22, c45, c37 etc. I'm not sure they
> are visible to userspace, but if we don't have a better name, maybe we
> have to use c33 in userspace as well.
> 
> I do think naming like this makes it clear we are talking about two
> parallel technologies, not a generic layer and then extensions for
> podl.
> 
> What do you think?

I'm OK with it.

Köry, can you please include some kernel documentation in your patches?
Something like this. I hope it will help to clarify things :) :

Power Sourcing Equipment (PSE) in IEEE 802.3 Standard
=====================================================

Overview
--------

Power Sourcing Equipment (PSE) is essential in networks for delivering power
along with data over Ethernet cables. It usually refers to devices like
switches and hubs that supply power to Powered Devices (PDs) such as IP
cameras, VoIP phones, and wireless access points.

PSE vs. PoDL PSE
----------------

PSE in the IEEE 802.3 standard generally refers to equipment that provides
power alongside data over Ethernet cables, typically associated with Power over
Ethernet (PoE). 

PoDL PSE, or Power over Data Lines PSE, specifically denotes PSEs operating
with single balanced twisted-pair PHYs, as per Clause 104 of IEEE 802.3. PoDL
is significant in contexts like automotive and industrial controls where power
and data delivery over a single pair is advantageous.

IEEE 802.3-2018 Addendums and Related Clauses
----------------------------------------------

Key addenda to the IEEE 802.3-2018 standard relevant to power delivery over
Ethernet are as follows:

- **802.3af (Approved in 2003-06-12)**: Known as PoE in the market, detailed in
  Clause 33, delivering up to 15.4W of power.
- **802.3at (Approved in 2009-09-11)**: Marketed as PoE+, enhancing PoE as
  covered in Clause 33, increasing power delivery to up to 30W.
- **802.3bt (Approved in 2018-09-27)**: Known as 4PPoE in the market, outlined
  in Clause 33. Type 3 delivers up to 60W, and Type 4 up to 100W.
- **802.3bu (Approved in 2016-12-07)**: Formerly referred to as PoDL, detailed
  in Clause 104. Introduces Classes 0 - 9. Class 9 PoDL PSE delivers up to ~65W

Kernel Naming Convention Recommendations
----------------------------------------

For clarity and consistency within the Linux kernel's networking subsystem, the
following naming conventions are recommended:

- For general PSE (PoE) code, use "c33_pse" key words. For example:
  ``enum ethtool_c33_pse_admin_state c33_admin_control;``.
  This aligns with Clause 33, encompassing various PoE forms.

- For PoDL PSE - specific code, use "podl_pse". For example:
  ``enum ethtool_podl_pse_admin_state podl_admin_control;`` to differentiate
  PoDL PSE settings according to Clause 104.

Summary of Clause 33: Data Terminal Equipment (DTE) Power via Media Dependent Interface (MDI)
-------------------------------------------------------------------------------------------

Clause 33 of the IEEE 802.3 standard defines the functional and electrical
characteristics of Powered Device (PD) and Power Sourcing Equipment (PSE).
These entities enable power delivery using the same generic cabling as for data
transmission, integrating power with data communication for devices such as
10BASE-T, 100BASE-TX, or 1000BASE-T.

Summary of Clause 104: Power over Data Lines (PoDL) of Single Balanced Twisted-Pair Ethernet
-------------------------------------------------------------------------------------------

Clause 104 of the IEEE 802.3 standard delineates the functional and electrical
characteristics of PoDL Powered Devices (PDs) and PoDL Power Sourcing Equipment
(PSEs). These are designed for use with single balanced twisted-pair Ethernet
Physical Layers. In this clause, 'PSE' refers specifically to PoDL PSE, and
'PD' to PoDL PD. The key intent is to provide devices with a unified interface
for both data and the power required to process this data over a single
balanced twisted-pair Ethernet connection.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Andrew Lunn]

> > However, everything not PoDL PSE seems to be clause 33. So how about:
> > 
> > 	enum ethtool_podl_pse_admin_state podl_admin_control;
> > 	enum ethtool_c33_pse_admin_state c33_admin_control;  
> > 
> > At least inside the kernel we use c22, c45, c37 etc. I'm not sure they
> > are visible to userspace, but if we don't have a better name, maybe we
> > have to use c33 in userspace as well.
> > 
> > I do think naming like this makes it clear we are talking about two
> > parallel technologies, not a generic layer and then extensions for
> > podl.
> > 
> > What do you think?
> 
> I'm OK with it.

Great.

> 
> Köry, can you please include some kernel documentation in your patches?
> Something like this. I hope it will help to clarify things :) :

This is good. I'm just wondering where to put it. Ideally we want to
cross reference to it in both this header file, and in the netlink
UAPI.

	Andrew

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Oleksij Rempel]

On Mon, Nov 20, 2023 at 11:14:32PM +0100, Andrew Lunn wrote:
> > > However, everything not PoDL PSE seems to be clause 33. So how about:
> > > 
> > > 	enum ethtool_podl_pse_admin_state podl_admin_control;
> > > 	enum ethtool_c33_pse_admin_state c33_admin_control;  
> > > 
> > > At least inside the kernel we use c22, c45, c37 etc. I'm not sure they
> > > are visible to userspace, but if we don't have a better name, maybe we
> > > have to use c33 in userspace as well.
> > > 
> > > I do think naming like this makes it clear we are talking about two
> > > parallel technologies, not a generic layer and then extensions for
> > > podl.
> > > 
> > > What do you think?
> > 
> > I'm OK with it.
> 
> Great.
> 
> > 
> > Köry, can you please include some kernel documentation in your patches?
> > Something like this. I hope it will help to clarify things :) :
> 
> This is good. I'm just wondering where to put it. Ideally we want to
> cross reference to it in both this header file, and in the netlink
> UAPI.

Documentation/networking/pse-pd/introduction.rst ?

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Köry Maincent]

On Mon, 20 Nov 2023 19:00:03 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > Official name for "PoE" is "Power via Media Dependent Interface". PoE is
> > not used in the IEEE 802.3-2018. Using names not used in the specification,
> > make development even harder :)
> > Especially since there are even more marketing names (names not used in the
> > specification) for different PoE variants:
> > - 802.3af (802.3at Type 1), PoE
> > - 802.3at Type 2, PoE+
> > - 802.3bt Type 3, 4PPoE or PoE++
> > - 802.3bt Type 4, 4PPoE or PoE++  
> 
> From the 2018 standard:
> 
>   1.4.407 Power Sourcing Equipment (PSE): A DTE or midspan device that
>   provides the power to a single link section. PSEs are defined for
>   use with two different types of balanced twisted-pair PHYs. When
>   used with 2 or 4 pair balanced twisted-pair (BASE-T) PHYs, (see IEEE
>   Std 802.3, Clause 33), DTE powering is intended to provide a single
>   10BASE-T, 100BASE-TX, or 1000BASE-T device with a unified interface
>   for both the data it requires and the power to process these
>   data. When used with single balanced twisted-pair (BASE-T1) PHYs
>   (see IEEE Std 802.3, Clause 104), DTE powering is intended to
>   provide a single 100BASE-T1 or 1000BASE-T1 device with a unified
>   interface for both the data it requires and the power to process
>   these data. A PSE used with balanced single twisted-pair PHYs is
>   also referred to as a PoDL PSE.
> 
> So it seems like, anything not PoDL PSE does not have a name :-(
> 
> However, everything not PoDL PSE seems to be clause 33. So how about:
> 
> 	enum ethtool_podl_pse_admin_state podl_admin_control;
> 	enum ethtool_c33_pse_admin_state c33_admin_control;  
> 
> At least inside the kernel we use c22, c45, c37 etc. I'm not sure they
> are visible to userspace, but if we don't have a better name, maybe we
> have to use c33 in userspace as well.
> 
> I do think naming like this makes it clear we are talking about two
> parallel technologies, not a generic layer and then extensions for
> podl.
> 
> What do you think?

If we decide to add a prefix, "c33" is precise but less easily understandable,
why not using simply "poe" prefix?
Maybe as POE were originally PMDI you prefer to use c33 which won't change over
time? 

Should I also modify the content of the enum?
ETHTOOL_PSE_ADMIN_STATE_* to ETHTOOL_C33_PSE_ADMIN_*
ETHTOOL_PSE_PW_D_STATUS_* to ETHTOOL_C33_PSE_PW_D_STATUS_*


-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Andrew Lunn]

> > However, everything not PoDL PSE seems to be clause 33. So how about:
> > 
> > 	enum ethtool_podl_pse_admin_state podl_admin_control;
> > 	enum ethtool_c33_pse_admin_state c33_admin_control;  
> > 
> > At least inside the kernel we use c22, c45, c37 etc. I'm not sure they
> > are visible to userspace, but if we don't have a better name, maybe we
> > have to use c33 in userspace as well.
> > 
> > I do think naming like this makes it clear we are talking about two
> > parallel technologies, not a generic layer and then extensions for
> > podl.
> > 
> > What do you think?
> 
> If we decide to add a prefix, "c33" is precise but less easily understandable,
> why not using simply "poe" prefix?

I suspect poe has the exact opposite problem, its too imprecise. Its
too much of a marketing name, with no clear meaning. It could even be
some people call podl poe.

To some extent, this is a user space UX problem. We can be precises in
the kernel and the kAPI. What ethtool decides to show to the user
could be different. Although it basically is the same problem.

Do you have ethtool patches? What does the output look like?  Oleksij
did say a hybrid could be possible, so we probably want ethtool to
group these properties together and make it clear what is PoDL and
!PoDL.

> Maybe as POE were originally PMDI you prefer to use c33 which won't change over
> time? 
> 
> Should I also modify the content of the enum?
> ETHTOOL_PSE_ADMIN_STATE_* to ETHTOOL_C33_PSE_ADMIN_*
> ETHTOOL_PSE_PW_D_STATUS_* to ETHTOOL_C33_PSE_PW_D_STATUS_*

Yes. That will help avoid getting PODL and C33 properties missed up.

     Andrew

Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Köry Maincent]

On Tue, 21 Nov 2023 15:19:19 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > > However, everything not PoDL PSE seems to be clause 33. So how about:
> > > 
> > > 	enum ethtool_podl_pse_admin_state podl_admin_control;
> > > 	enum ethtool_c33_pse_admin_state c33_admin_control;  
> > > 
> > > At least inside the kernel we use c22, c45, c37 etc. I'm not sure they
> > > are visible to userspace, but if we don't have a better name, maybe we
> > > have to use c33 in userspace as well.
> > > 
> > > I do think naming like this makes it clear we are talking about two
> > > parallel technologies, not a generic layer and then extensions for
> > > podl.
> > > 
> > > What do you think?  
> > 
> > If we decide to add a prefix, "c33" is precise but less easily
> > understandable, why not using simply "poe" prefix?  
> 
> I suspect poe has the exact opposite problem, its too imprecise. Its
> too much of a marketing name, with no clear meaning. It could even be
> some people call podl poe.
> 
> To some extent, this is a user space UX problem. We can be precises in
> the kernel and the kAPI. What ethtool decides to show to the user
> could be different. Although it basically is the same problem.

Alright, thanks for your answer.

> Do you have ethtool patches? What does the output look like?  Oleksij
> did say a hybrid could be possible, so we probably want ethtool to
> group these properties together and make it clear what is PoDL and
> !PoDL.

No I don't, I am only using ynl for now.
I would be similar to podl:
https://kernel.googlesource.com/pub/scm/network/ethtool/ethtool/+/e6cc6807f87c74d4e5b1f1e9d21d3a74e75a258b/netlink/pse-pd.c

Duplicating the PoDL part with c33. Using the same --set-pse and --show-pse
options.

> > Maybe as POE were originally PMDI you prefer to use c33 which won't change
> > over time? 
> > 
> > Should I also modify the content of the enum?
> > ETHTOOL_PSE_ADMIN_STATE_* to ETHTOOL_C33_PSE_ADMIN_*
> > ETHTOOL_PSE_PW_D_STATUS_* to ETHTOOL_C33_PSE_PW_D_STATUS_*  
> 
> Yes. That will help avoid getting PODL and C33 properties missed up.

Alright.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next 3/9] net: pse-pd: Introduce PSE types enumeration

[Kory Maincent]

Introduce an enumeration to define PSE types (PoE or PoDL),
utilizing a bitfield for potential future support of both types.
Include 'pse_get_types' helper for external access to PSE type info

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pse_core.c      |  9 +++++++++
 drivers/net/pse-pd/pse_regulator.c |  1 +
 include/linux/pse-pd/pse.h         | 22 ++++++++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 146b81f08a89..2959c94f7798 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -312,3 +312,12 @@ int pse_ethtool_set_config(struct pse_control *psec,
 	return err;
 }
 EXPORT_SYMBOL_GPL(pse_ethtool_set_config);
+
+u32 pse_get_types(struct pse_control *psec)
+{
+	if (!psec->pcdev)
+		return PSE_UNKNOWN;
+	else
+		return psec->pcdev->types;
+}
+EXPORT_SYMBOL_GPL(pse_get_types);
diff --git a/drivers/net/pse-pd/pse_regulator.c b/drivers/net/pse-pd/pse_regulator.c
index 1dedf4de296e..e34ab8526067 100644
--- a/drivers/net/pse-pd/pse_regulator.c
+++ b/drivers/net/pse-pd/pse_regulator.c
@@ -116,6 +116,7 @@ pse_reg_probe(struct platform_device *pdev)
 	priv->pcdev.owner = THIS_MODULE;
 	priv->pcdev.ops = &pse_reg_ops;
 	priv->pcdev.dev = dev;
+	priv->pcdev.types = PSE_PODL;
 	ret = devm_pse_controller_register(dev, &priv->pcdev);
 	if (ret) {
 		dev_err(dev, "failed to register PSE controller (%pe)\n",
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 25490d0c682d..67a0ff5e480c 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -44,6 +44,19 @@ struct pse_control_status {
 	enum ethtool_pse_pw_d_status pw_status;
 };
 
+/**
+ * enum - Types of PSE controller.
+ *
+ * @PSE_UNKNOWN: Type of PSE controller is unknown
+ * @PSE_PODL: PSE controller which support PoDL
+ * @PSE_POE: PSE controller which support PoE
+ */
+enum {
+	PSE_UNKNOWN = BIT(0),
+	PSE_PODL = BIT(1),
+	PSE_POE = BIT(2),
+};
+
 /**
  * struct pse_controller_ops - PSE controller driver callbacks
  *
@@ -77,6 +90,7 @@ struct pse_control;
  *            device tree to id as given to the PSE control ops
  * @nr_lines: number of PSE controls in this controller device
  * @lock: Mutex for serialization access to the PSE controller
+ * @types: types of the PSE controller
  */
 struct pse_controller_dev {
 	const struct pse_controller_ops *ops;
@@ -89,6 +103,7 @@ struct pse_controller_dev {
 			const struct of_phandle_args *pse_spec);
 	unsigned int nr_lines;
 	struct mutex lock;
+	u32 types;
 };
 
 #if IS_ENABLED(CONFIG_PSE_CONTROLLER)
@@ -108,6 +123,8 @@ int pse_ethtool_set_config(struct pse_control *psec,
 			   struct netlink_ext_ack *extack,
 			   const struct pse_control_config *config);
 
+u32 pse_get_types(struct pse_control *psec);
+
 #else
 
 static inline struct pse_control *of_pse_control_get(struct device_node *node)
@@ -133,6 +150,11 @@ static inline int pse_ethtool_set_config(struct pse_control *psec,
 	return -ENOTSUPP;
 }
 
+static u32 pse_get_types(struct pse_control *psec)
+{
+	return PSE_UNKNOWN;
+}
+
 #endif
 
 #endif

-- 
2.25.1

Re: [PATCH net-next 3/9] net: pse-pd: Introduce PSE types enumeration

[Simon Horman]

On Thu, Nov 16, 2023 at 03:01:35PM +0100, Kory Maincent wrote:
> Introduce an enumeration to define PSE types (PoE or PoDL),
> utilizing a bitfield for potential future support of both types.
> Include 'pse_get_types' helper for external access to PSE type info
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

...

> diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h

...

> @@ -133,6 +150,11 @@ static inline int pse_ethtool_set_config(struct pse_control *psec,
>  	return -ENOTSUPP;
>  }
>  
> +static u32 pse_get_types(struct pse_control *psec)

Hi Kory,

a minor nit from my side: as this is a function in a header file,
it should be static inline

> +{
> +	return PSE_UNKNOWN;
> +}

...

[PATCH net-next 4/9] net: ethtool: pse-pd: Expand pse commands with the PSE PoE interface

[Kory Maincent]

Add PSE PoE interface support in the ethtool pse command.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/networking/ethtool-netlink.rst | 20 +++++++++
 net/ethtool/pse-pd.c                         | 64 +++++++++++++++++++++++-----
 2 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 2540c70952ff..a0b6437e1497 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1711,6 +1711,10 @@ Kernel response contents:
                                                   PSE functions
   ``ETHTOOL_A_PODL_PSE_PW_D_STATUS``         u32  power detection status of the
                                                   PoDL PSE.
+  ``ETHTOOL_A_PSE_ADMIN_STATE``              u32  Operational state of the PoE
+                                                  PSE functions.
+  ``ETHTOOL_A_PSE_PW_D_STATUS``              u32  power detection status of the
+                                                  PoE PSE.
   ======================================  ======  =============================
 
 When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_STATE`` attribute identifies
@@ -1722,6 +1726,12 @@ aPoDLPSEAdminState. Possible values are:
 .. kernel-doc:: include/uapi/linux/ethtool.h
     :identifiers: ethtool_podl_pse_admin_state
 
+The same goes for ``ETHTOOL_A_PSE_ADMIN_STATE`` implementing
+``IEEE 802.3-2022`` 30.9.1.1.2 aPSEAdminState.
+
+.. kernel-doc:: include/uapi/linux/ethtool.h
+    :identifiers: ethtool_pse_admin_state
+
 When set, the optional ``ETHTOOL_A_PODL_PSE_PW_D_STATUS`` attribute identifies
 the power detection status of the PoDL PSE.  The status depend on internal PSE
 state machine and automatic PD classification support. This option is
@@ -1731,6 +1741,12 @@ Possible values are:
 .. kernel-doc:: include/uapi/linux/ethtool.h
     :identifiers: ethtool_podl_pse_pw_d_status
 
+The same goes for ``ETHTOOL_A_PSE_ADMIN_PW_D_STATUS`` implementing
+``IEEE 802.3-2022`` 30.9.1.1.5 aPSEPowerDetectionStatus.
+
+.. kernel-doc:: include/uapi/linux/ethtool.h
+    :identifiers: ethtool_pse_pw_d_status
+
 PSE_SET
 =======
 
@@ -1741,6 +1757,7 @@ Request contents:
   ======================================  ======  =============================
   ``ETHTOOL_A_PSE_HEADER``                nested  request header
   ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL``       u32  Control PoDL PSE Admin state
+  ``ETHTOOL_A_PSE_ADMIN_CONTROL``            u32  Control PSE Admin state
   ======================================  ======  =============================
 
 When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is used
@@ -1748,6 +1765,9 @@ to control PoDL PSE Admin functions. This option is implementing
 ``IEEE 802.3-2018`` 30.15.1.2.1 acPoDLPSEAdminControl. See
 ``ETHTOOL_A_PODL_PSE_ADMIN_STATE`` for supported values.
 
+The same goes for ``ETHTOOL_A_PSE_ADMIN_CONTROL`` implementing
+``IEEE 802.3-2022`` 30.9.1.2.1 acPSEAdminControl.
+
 RSS_GET
 =======
 
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index aef57a058f0d..88ab5e3576bc 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -82,6 +82,10 @@ static int pse_reply_size(const struct ethnl_req_info *req_base,
 		len += nla_total_size(sizeof(u32)); /* _PODL_PSE_ADMIN_STATE */
 	if (st->podl_pw_status > 0)
 		len += nla_total_size(sizeof(u32)); /* _PODL_PSE_PW_D_STATUS */
+	if (st->admin_state > 0)
+		len += nla_total_size(sizeof(u32)); /* _PSE_ADMIN_STATE */
+	if (st->pw_status > 0)
+		len += nla_total_size(sizeof(u32)); /* _PSE_PW_D_STATUS */
 
 	return len;
 }
@@ -103,6 +107,16 @@ static int pse_fill_reply(struct sk_buff *skb,
 			st->podl_pw_status))
 		return -EMSGSIZE;
 
+	if (st->admin_state > 0 &&
+	    nla_put_u32(skb, ETHTOOL_A_PSE_ADMIN_STATE,
+			st->admin_state))
+		return -EMSGSIZE;
+
+	if (st->pw_status > 0 &&
+	    nla_put_u32(skb, ETHTOOL_A_PSE_PW_D_STATUS,
+			st->pw_status))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
@@ -113,25 +127,18 @@ const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1] = {
 	[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] =
 		NLA_POLICY_RANGE(NLA_U32, ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED,
 				 ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED),
+	[ETHTOOL_A_PSE_ADMIN_CONTROL] =
+		NLA_POLICY_RANGE(NLA_U32, ETHTOOL_PSE_ADMIN_STATE_DISABLED,
+				 ETHTOOL_PSE_ADMIN_STATE_ENABLED),
 };
 
 static int
 ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
-{
-	return !!info->attrs[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL];
-}
-
-static int
-ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 {
 	struct net_device *dev = req_info->dev;
-	struct pse_control_config config = {};
 	struct nlattr **tb = info->attrs;
 	struct phy_device *phydev;
 
-	/* this values are already validated by the ethnl_pse_set_policy */
-	config.podl_admin_control = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]);
-
 	phydev = dev->phydev;
 	if (!phydev) {
 		NL_SET_ERR_MSG(info->extack, "No PHY is attached");
@@ -143,6 +150,43 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		return -EOPNOTSUPP;
 	}
 
+	if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] &&
+	    !tb[ETHTOOL_A_PSE_ADMIN_CONTROL])
+		return 0;
+
+	if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] &&
+	    !(pse_get_types(phydev->psec) & PSE_PODL)) {
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL],
+				    "setting PSE PoDL admin control not supported");
+		return -EOPNOTSUPP;
+	}
+	if (tb[ETHTOOL_A_PSE_ADMIN_CONTROL] &&
+	    !(pse_get_types(phydev->psec) & PSE_POE)) {
+		NL_SET_ERR_MSG_ATTR(info->extack,
+				    tb[ETHTOOL_A_PSE_ADMIN_CONTROL],
+				    "setting PSE admin control not supported");
+		return -EOPNOTSUPP;
+	}
+
+	return 1;
+}
+
+static int
+ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
+{
+	struct net_device *dev = req_info->dev;
+	struct pse_control_config config = {};
+	struct nlattr **tb = info->attrs;
+	struct phy_device *phydev;
+
+	phydev = dev->phydev;
+	/* These values are already validated by the ethnl_pse_set_policy */
+	if (pse_get_types(phydev->psec) & PSE_PODL)
+		config.podl_admin_control = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]);
+	if (pse_get_types(phydev->psec) & PSE_POE)
+		config.admin_control = nla_get_u32(tb[ETHTOOL_A_PSE_ADMIN_CONTROL]);
+
 	/* Return errno directly - PSE has no notification */
 	return pse_ethtool_set_config(phydev->psec, info->extack, &config);
 }

-- 
2.25.1

[PATCH net-next 5/9] netlink: specs: Modify pse attribute prefix

[Kory Maincent]

Remove podl from the attribute prefix to prepare the support of PoE pse
netlink spec.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 5c7a65b009b4..e1bf75099264 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -878,17 +878,17 @@ attribute-sets:
         type: nest
         nested-attributes: header
       -
-        name: admin-state
+        name: podl-pse-admin-state
         type: u32
-        name-prefix: ethtool-a-podl-pse-
+        name-prefix: ethtool-a-
       -
-        name: admin-control
+        name: podl-pse-admin-control
         type: u32
-        name-prefix: ethtool-a-podl-pse-
+        name-prefix: ethtool-a-
       -
-        name: pw-d-status
+        name: podl-pse-pw-d-status
         type: u32
-        name-prefix: ethtool-a-podl-pse-
+        name-prefix: ethtool-a-
   -
     name: rss
     attributes:
@@ -1568,9 +1568,9 @@ operations:
         reply:
           attributes: &pse
             - header
-            - admin-state
-            - admin-control
-            - pw-d-status
+            - podl-pse-admin-state
+            - podl-pse-admin-control
+            - podl-pse-pw-d-status
       dump: *pse-get-op
     -
       name: pse-set

-- 
2.25.1

Re: [PATCH net-next 5/9] netlink: specs: Modify pse attribute prefix

[Jakub Kicinski]

On Thu, 16 Nov 2023 15:01:37 +0100 Kory Maincent wrote:
> Remove podl from the attribute prefix to prepare the support of PoE pse
> netlink spec.

You need to run ./tools/net/ynl/ynl-regen.sh

Re: [PATCH net-next 5/9] netlink: specs: Modify pse attribute prefix

[Köry Maincent]

On Sat, 18 Nov 2023 15:57:02 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 16 Nov 2023 15:01:37 +0100 Kory Maincent wrote:
> > Remove podl from the attribute prefix to prepare the support of PoE pse
> > netlink spec.  
> 
> You need to run ./tools/net/ynl/ynl-regen.sh

Ok, should I also send a patch with the newly generated files? Or is it
something done by the maintainers?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 5/9] netlink: specs: Modify pse attribute prefix

[Jakub Kicinski]

On Mon, 20 Nov 2023 11:19:14 +0100 Köry Maincent wrote:
> > You need to run ./tools/net/ynl/ynl-regen.sh  
> 
> Ok, should I also send a patch with the newly generated files? Or is it
> something done by the maintainers?

It needs to be part of the series. We don't have very clear guidelines
on how to carry the regeneration. But for small changes like this you
can squash the regenerated code into the spec change.

[PATCH net-next 6/9] netlink: specs: Expand the pse netlink command with PoE interface

[Kory Maincent]

Add the PoE pse attributes prefix to be able to use PoE interface.

Example usage:
./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do pse-get \
             --json '{"header":{"dev-name":"eth0"}}'
{'header': {'dev-index': 4, 'dev-name': 'eth0'},
 'pse-admin-state': 3,
 'pse-pw-d-status': 4}

./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do pse-set \
             --json '{"header":{"dev-name":"eth0"}, "pse-admin-control":3}'

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index e1bf75099264..6e1525106a9e 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -889,6 +889,18 @@ attribute-sets:
         name: podl-pse-pw-d-status
         type: u32
         name-prefix: ethtool-a-
+      -
+        name: pse-admin-state
+        type: u32
+        name-prefix: ethtool-a-
+      -
+        name: pse-admin-control
+        type: u32
+        name-prefix: ethtool-a-
+      -
+        name: pse-pw-d-status
+        type: u32
+        name-prefix: ethtool-a-
   -
     name: rss
     attributes:
@@ -1571,6 +1583,9 @@ operations:
             - podl-pse-admin-state
             - podl-pse-admin-control
             - podl-pse-pw-d-status
+            - pse-admin-state
+            - pse-admin-control
+            - pse-pw-d-status
       dump: *pse-get-op
     -
       name: pse-set

-- 
2.25.1

Re: [PATCH net-next 6/9] netlink: specs: Expand the pse netlink command with PoE interface

[Jakub Kicinski]

On Thu, 16 Nov 2023 15:01:38 +0100 Kory Maincent wrote:
> +        name: pse-admin-state
> +        type: u32
> +        name-prefix: ethtool-a-
> +      -
> +        name: pse-admin-control
> +        type: u32
> +        name-prefix: ethtool-a-
> +      -
> +        name: pse-pw-d-status
> +        type: u32
> +        name-prefix: ethtool-a-

The default prefix is ethtool-a-pse-
Why don't you leave that be and drop the pse- from the names?

Re: [PATCH net-next 6/9] netlink: specs: Expand the pse netlink command with PoE interface

[Köry Maincent]

On Sat, 18 Nov 2023 16:01:31 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 16 Nov 2023 15:01:38 +0100 Kory Maincent wrote:
> > +        name: pse-admin-state
> > +        type: u32
> > +        name-prefix: ethtool-a-
> > +      -
> > +        name: pse-admin-control
> > +        type: u32
> > +        name-prefix: ethtool-a-
> > +      -
> > +        name: pse-pw-d-status
> > +        type: u32
> > +        name-prefix: ethtool-a-  
> 
> The default prefix is ethtool-a-pse-
> Why don't you leave that be and drop the pse- from the names?

Oh right, thanks, I copied blindly the PoDL lines.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next 7/9] firmware_loader: Expand Firmware upload error codes

[Kory Maincent]

No error code are available to signal an invalid firmware content.
Drivers that can check the firmware content validity can not return this
specific failure to the user-space

Expand the firmware error code with an additional code:
- "firmware invalid" code which can be used when the provided firmware
  is invalid

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/base/firmware_loader/sysfs_upload.c | 1 +
 include/linux/firmware.h                    | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
index a0af8f5f13d8..829270067d16 100644
--- a/drivers/base/firmware_loader/sysfs_upload.c
+++ b/drivers/base/firmware_loader/sysfs_upload.c
@@ -27,6 +27,7 @@ static const char * const fw_upload_err_str[] = {
 	[FW_UPLOAD_ERR_INVALID_SIZE] = "invalid-file-size",
 	[FW_UPLOAD_ERR_RW_ERROR]     = "read-write-error",
 	[FW_UPLOAD_ERR_WEAROUT]	     = "flash-wearout",
+	[FW_UPLOAD_ERR_FW_INVALID]   = "firmware-invalid",
 };
 
 static const char *fw_upload_progress(struct device *dev,
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index de7fea3bca51..0311858b46ce 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -27,6 +27,7 @@ struct firmware {
  * @FW_UPLOAD_ERR_INVALID_SIZE: invalid firmware image size
  * @FW_UPLOAD_ERR_RW_ERROR: read or write to HW failed, see kernel log
  * @FW_UPLOAD_ERR_WEAROUT: FLASH device is approaching wear-out, wait & retry
+ * @FW_UPLOAD_ERR_FW_INVALID: invalid firmware file
  * @FW_UPLOAD_ERR_MAX: Maximum error code marker
  */
 enum fw_upload_err {
@@ -38,6 +39,7 @@ enum fw_upload_err {
 	FW_UPLOAD_ERR_INVALID_SIZE,
 	FW_UPLOAD_ERR_RW_ERROR,
 	FW_UPLOAD_ERR_WEAROUT,
+	FW_UPLOAD_ERR_FW_INVALID,
 	FW_UPLOAD_ERR_MAX
 };
 

-- 
2.25.1

Re: [PATCH net-next 7/9] firmware_loader: Expand Firmware upload error codes

[Luis Chamberlain]

On Thu, Nov 16, 2023 at 03:01:39PM +0100, Kory Maincent wrote:
> No error code are available to signal an invalid firmware content.
> Drivers that can check the firmware content validity can not return this
> specific failure to the user-space
> 
> Expand the firmware error code with an additional code:
> - "firmware invalid" code which can be used when the provided firmware
>   is invalid
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH net-next 7/9] firmware_loader: Expand Firmware upload error codes

[Greg Kroah-Hartman]

On Thu, Nov 16, 2023 at 03:01:39PM +0100, Kory Maincent wrote:
> No error code are available to signal an invalid firmware content.
> Drivers that can check the firmware content validity can not return this
> specific failure to the user-space
> 
> Expand the firmware error code with an additional code:
> - "firmware invalid" code which can be used when the provided firmware
>   is invalid
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  drivers/base/firmware_loader/sysfs_upload.c | 1 +
>  include/linux/firmware.h                    | 2 ++
>  2 files changed, 3 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH net-next 7/9] firmware_loader: Expand Firmware upload error codes

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

On Thu, Nov 16, 2023 at 03:01:39PM +0100, Kory Maincent wrote:
> No error code are available to signal an invalid firmware content.
> Drivers that can check the firmware content validity can not return this
> specific failure to the user-space
> 
> Expand the firmware error code with an additional code:
> - "firmware invalid" code which can be used when the provided firmware
>   is invalid
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

This would be rather helpful to me for some stuff that I am currently
working on and was hoping to send to Arnd for inclusion in 6.8:
https://lore.kernel.org/all/20231020-series-uncooked-077b107af3ae@spud/

I'm currently returning a "HW_ERROR" for something that this would fit
the bill for (in mpfs_auto_update_write()). What would the ETA for this
stuff landing via the net tree be?
Since I am not a netdev contributor its hard to tell how controversial
these patches are!

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next 7/9] firmware_loader: Expand Firmware upload error codes

[Andrew Lunn]

> This would be rather helpful to me for some stuff that I am currently
> working on and was hoping to send to Arnd for inclusion in 6.8:
> https://lore.kernel.org/all/20231020-series-uncooked-077b107af3ae@spud/
> 
> I'm currently returning a "HW_ERROR" for something that this would fit
> the bill for (in mpfs_auto_update_write()). What would the ETA for this
> stuff landing via the net tree be?
> Since I am not a netdev contributor its hard to tell how controversial
> these patches are!

It already has the needed ACKs, so it could be merged
anytime. However, it seems like two different subsystems are
interested in it. So rather than merge it via netdev, it might make
sense to merge it via its normal tree, driver-core. Then ask for a
stable branch which can be pulled into netdev and arm-soc.

      Andrew

Re: [PATCH net-next 7/9] firmware_loader: Expand Firmware upload error codes

[Köry Maincent]

On Thu, 16 Nov 2023 22:56:10 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > This would be rather helpful to me for some stuff that I am currently
> > working on and was hoping to send to Arnd for inclusion in 6.8:
> > https://lore.kernel.org/all/20231020-series-uncooked-077b107af3ae@spud/
> > 
> > I'm currently returning a "HW_ERROR" for something that this would fit
> > the bill for (in mpfs_auto_update_write()). What would the ETA for this
> > stuff landing via the net tree be?
> > Since I am not a netdev contributor its hard to tell how controversial
> > these patches are!  
> 
> It already has the needed ACKs, so it could be merged
> anytime. However, it seems like two different subsystems are
> interested in it. So rather than merge it via netdev, it might make
> sense to merge it via its normal tree, driver-core. Then ask for a
> stable branch which can be pulled into netdev and arm-soc.

Ok, I will remove this patch from this series in v2 and send it through normal
tree.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next 8/9] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller

[Kory Maincent]

Add the PD692x0 I2C Power Sourcing Equipment controller device tree
bindings documentation.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 .../bindings/net/pse-pd/microchip,pd692x0_i2c.yaml | 70 ++++++++++++++++++++++
 MAINTAINERS                                        |  6 ++
 2 files changed, 76 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml
new file mode 100644
index 000000000000..c42bbc427988
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pse-pd/microchip,pd692x0_i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PD692x0 Power Sourcing Equipment controller
+
+maintainers:
+  - Kory Maincent <kory.maincent@bootlin.com>
+
+allOf:
+  - $ref: pse-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - microchip,pd69200
+      - microchip,pd69210
+      - microchip,pd69220
+
+  reg:
+    maxItems: 1
+
+  '#pse-cells':
+    const: 1
+
+  ports-matrix:
+    description: Port conversion matrix configuration
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    minItems: 1
+    maxItems: 48
+    items:
+      items:
+        - description: Logical port number
+          minimum: 0
+          maximum: 47
+        - description: Physical port number A (0xff for undefined)
+          oneOf:
+            - minimum: 0
+              maximum: 95
+            - const: 0xff
+        - description: Physical port number B (0xff for undefined)
+          oneOf:
+            - minimum: 0
+              maximum: 95
+            - const: 0xff
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-pse@3c {
+          compatible = "microchip,pd69200";
+          reg = <0x3c>;
+          #pse-cells = <1>;
+          ports-matrix = <0 2 5
+                          1 3 6
+                          2 0 0xff
+                          3 1 0xff>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 350d00657f6b..1e154dacef67 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14248,6 +14248,12 @@ L:	linux-serial@vger.kernel.org
 S:	Maintained
 F:	drivers/tty/serial/8250/8250_pci1xxxx.c
 
+MICROCHIP PD692X0 PSE DRIVER
+M:	Kory Maincent <kory.maincent@bootlin.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml
+
 MICROCHIP POLARFIRE FPGA DRIVERS
 M:	Conor Dooley <conor.dooley@microchip.com>
 R:	Vladimir Georgiev <v.georgiev@metrotek.ru>

-- 
2.25.1

Re: [PATCH net-next 8/9] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller

[Krzysztof Kozlowski]

On 16/11/2023 15:01, Kory Maincent wrote:
> Add the PD692x0 I2C Power Sourcing Equipment controller device tree
> bindings documentation.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  .../bindings/net/pse-pd/microchip,pd692x0_i2c.yaml | 70 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 ++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml
> new file mode 100644
> index 000000000000..c42bbc427988
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml

Filename should match compatibles, so drop i2c suffix.

> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/pse-pd/microchip,pd692x0_i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PD692x0 Power Sourcing Equipment controller
> +
> +maintainers:
> +  - Kory Maincent <kory.maincent@bootlin.com>
> +
> +allOf:
> +  - $ref: pse-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,pd69200
> +      - microchip,pd69210
> +      - microchip,pd69220

Your driver suggests these are compatible.

> +
> +  reg:
> +    maxItems: 1
> +
> +  '#pse-cells':
> +    const: 1
> +
> +  ports-matrix:
> +    description: Port conversion matrix configuration

I do not see such property defined anywhere. Your description should
explain what the purpose is and what it is exactly. Currently you just
repeat property name, so quite pointless.


> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    minItems: 1
> +    maxItems: 48
> +    items:
> +      items:
> +        - description: Logical port number
> +          minimum: 0
> +          maximum: 47
> +        - description: Physical port number A (0xff for undefined)
> +          oneOf:
> +            - minimum: 0
> +              maximum: 95
> +            - const: 0xff
> +        - description: Physical port number B (0xff for undefined)
> +          oneOf:
> +            - minimum: 0
> +              maximum: 95
> +            - const: 0xff
> +
> +additionalProperties: false

unevaluatedProperties: false instead

> +
> +required:
> +  - compatible
> +  - reg
> 

Best regards,
Krzysztof

Re: [PATCH net-next 8/9] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller

[Andrew Lunn]

> > +  ports-matrix:
> > +    description: Port conversion matrix configuration
> 
> I do not see such property defined anywhere. Your description should
> explain what the purpose is and what it is exactly. Currently you just
> repeat property name, so quite pointless.

I have to agree. You appear to have a device which can supply power to
48 RJ-45 connectors on the front panel? Which probably maps to 48
ports of a Ethernet switch. How do i use these properties described
here to say that eth42 connects to port 42 of the PSE?

> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    minItems: 1
> > +    maxItems: 48
> > +    items:
> > +      items:
> > +        - description: Logical port number
> > +          minimum: 0
> > +          maximum: 47
> > +        - description: Physical port number A (0xff for undefined)
> > +          oneOf:
> > +            - minimum: 0
> > +              maximum: 95
> > +            - const: 0xff
> > +        - description: Physical port number B (0xff for undefined)

It would be good to explain what Port A and B are. It might be obvious
to somebody who knows PSE, but i have no idea...

   Andrew

[PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Kory Maincent]

Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller.
This driver only support i2c communication for now.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 MAINTAINERS                  |    1 +
 drivers/net/pse-pd/Kconfig   |   11 +
 drivers/net/pse-pd/Makefile  |    1 +
 drivers/net/pse-pd/pd692x0.c | 1049 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1062 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1e154dacef67..34cf007965a8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14253,6 +14253,7 @@ M:	Kory Maincent <kory.maincent@bootlin.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0_i2c.yaml
+F:	drivers/net/pse-pd/pd69200.c
 
 MICROCHIP POLARFIRE FPGA DRIVERS
 M:	Conor Dooley <conor.dooley@microchip.com>
diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
index 687dec49c1e1..e3a6ba669f20 100644
--- a/drivers/net/pse-pd/Kconfig
+++ b/drivers/net/pse-pd/Kconfig
@@ -20,4 +20,15 @@ config PSE_REGULATOR
 	  Sourcing Equipment without automatic classification support. For
 	  example for basic implementation of PoDL (802.3bu) specification.
 
+config PSE_PD692X0
+	tristate "PD692X0 PSE controller"
+	depends on I2C
+	select FW_UPLOAD
+	help
+	  This module provides support for PD692x0 regulator based Ethernet
+	  Power Sourcing Equipment.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called pd692x0.
+
 endif
diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
index 1b8aa4c70f0b..9c12c4a65730 100644
--- a/drivers/net/pse-pd/Makefile
+++ b/drivers/net/pse-pd/Makefile
@@ -4,3 +4,4 @@
 obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
 
 obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
+obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
new file mode 100644
index 000000000000..f6b8f89a3afe
--- /dev/null
+++ b/drivers/net/pse-pd/pd692x0.c
@@ -0,0 +1,1049 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the Microchip PD692X0 PoE PSE Controller driver (I2C bus)
+ *
+ * Copyright (c) 2023 Bootlin, Kory Maincent <kory.maincent@bootlin.com>
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pse-pd/pse.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+
+#define PD692X0_PSE_NAME "pd692x0_pse"
+
+#define PD692X0_MAX_LOGICAL_PORTS	48
+#define PD692X0_MAX_HW_PORTS	96
+
+#define PD69200_BT_PROD_VER	24
+#define PD69210_BT_PROD_VER	26
+#define PD69220_BT_PROD_VER	29
+
+#define PD692X0_FW_MAJ_VER	3
+#define PD692X0_FW_MIN_VER	5
+#define PD692X0_FW_PATCH_VER	5
+
+enum pd692x0_fw_state {
+	PD692X0_FW_UNKNOWN,
+	PD692X0_FW_OK,
+	PD692X0_FW_BROKEN,
+	PD692X0_FW_NEED_UPDATE,
+	PD692X0_FW_PREPARE,
+	PD692X0_FW_WRITE,
+	PD692X0_FW_COMPLETE,
+};
+
+struct pd692x0_msg_content {
+	u8 key;
+	u8 echo;
+	u8 sub[3];
+	u8 data[8];
+	__be16 chksum;
+} __packed;
+
+struct pd692x0_msg_ver {
+	u8 prod;
+	u8 maj_sw_ver;
+	u8 min_sw_ver;
+	u8 pa_sw_ver;
+	u8 param;
+	u8 build;
+};
+
+enum {
+	PD692X0_KEY_CMD,
+	PD692X0_KEY_PRG,
+	PD692X0_KEY_REQ,
+	PD692X0_KEY_TLM,
+	PD692X0_KEY_TEST,
+	PD692X0_KEY_REPORT = 0x52
+};
+
+enum {
+	PD692X0_MSG_RESET,
+	PD692X0_MSG_GET_SW_VER,
+	PD692X0_MSG_SET_TMP_PORT_MATRIX,
+	PD692X0_MSG_PRG_PORT_MATRIX,
+	PD692X0_MSG_SET_PORT_PARAM,
+	PD692X0_MSG_GET_PORT_STATUS,
+	PD692X0_MSG_DOWNLOAD_CMD,
+
+	/* add new message above here */
+	PD692X0_MSG_CNT
+};
+
+struct pd692x0_msg {
+	struct pd692x0_msg_content content;
+	u16 delay_recv;
+};
+
+struct pd692x0_priv {
+	struct i2c_client *client;
+	struct pse_controller_dev pcdev;
+
+	enum pd692x0_fw_state fw_state;
+	struct fw_upload *fwl;
+	bool cancel_request:1;
+
+	u8 msg_id;
+	bool last_cmd_key:1;
+	unsigned long last_cmd_key_time;
+
+	enum ethtool_pse_admin_state admin_state[PD692X0_MAX_LOGICAL_PORTS];
+};
+
+/* Template list of the fixed bytes of the communication messages */
+static const struct pd692x0_msg pd692x0_msg_template_list[PD692X0_MSG_CNT] = {
+	[PD692X0_MSG_RESET] = {
+		.content = {
+			.key = PD692X0_KEY_CMD,
+			.sub = {0x07, 0x55, 0x00},
+			.data = {0x55, 0x00, 0x55, 0x4e,
+				 0x4e, 0x4e, 0x4e, 0x4e},
+		},
+	},
+	[PD692X0_MSG_GET_SW_VER] = {
+		.content = {
+			.key = PD692X0_KEY_REQ,
+			.sub = {0x07, 0x1e, 0x21},
+			.data = {0x4e, 0x4e, 0x4e, 0x4e,
+				 0x4e, 0x4e, 0x4e, 0x4e},
+		},
+	},
+	[PD692X0_MSG_SET_TMP_PORT_MATRIX] = {
+		.content = {
+			.key = PD692X0_KEY_CMD,
+			.sub	 = {0x05, 0x43},
+			.data = {   0, 0x4e, 0x4e, 0x4e,
+				 0x4e, 0x4e, 0x4e, 0x4e},
+		},
+	},
+	[PD692X0_MSG_PRG_PORT_MATRIX] = {
+		.content = {
+			.key = PD692X0_KEY_CMD,
+			.sub = {0x07, 0x43, 0x4e},
+			.data = {0x4e, 0x4e, 0x4e, 0x4e,
+				 0x4e, 0x4e, 0x4e, 0x4e},
+		},
+	},
+	[PD692X0_MSG_SET_PORT_PARAM] = {
+		.content = {
+			.key = PD692X0_KEY_CMD,
+			.sub = {0x05, 0xc0},
+			.data = {   0, 0xff, 0xff, 0xff,
+				 0x4e, 0x4e, 0x4e, 0x4e},
+		},
+	},
+	[PD692X0_MSG_GET_PORT_STATUS] = {
+		.content = {
+			.key = PD692X0_KEY_REQ,
+			.sub = {0x05, 0xc1},
+			.data = {0x4e, 0x4e, 0x4e, 0x4e,
+				 0x4e, 0x4e, 0x4e, 0x4e},
+		},
+	},
+	[PD692X0_MSG_DOWNLOAD_CMD] = {
+		.content = {
+			.key = PD692X0_KEY_PRG,
+			.sub = {0xff, 0x99, 0x15},
+			.data = {0x16, 0x16, 0x99, 0x4e,
+				 0x4e, 0x4e, 0x4e, 0x4e},
+		},
+	},
+};
+
+static u8 pd692x0_build_msg(struct pd692x0_msg_content *msg, u8 echo)
+{
+	u8 *data = (u8 *)msg;
+	u16 chksum = 0;
+	int i;
+
+	msg->echo = echo++;
+	if (echo == 0xff)
+		echo = 0;
+
+	for (i = 0; i < sizeof(*msg) - sizeof(msg->chksum); i++)
+		chksum += data[i];
+
+	msg->chksum = cpu_to_be16(chksum);
+
+	return echo;
+}
+
+static int pd692x0_send_msg(struct pd692x0_priv *priv, struct pd692x0_msg *msg)
+{
+	const struct i2c_client *client = priv->client;
+	int ret;
+
+	if (msg->content.key == PD692X0_KEY_CMD && priv->last_cmd_key) {
+		while (time_is_after_jiffies(msecs_to_jiffies(30) + priv->last_cmd_key_time))
+			usleep_range(1000, 2000);
+	}
+
+	/* Add echo and checksum bytes to the message */
+	priv->msg_id = pd692x0_build_msg(&msg->content, priv->msg_id);
+
+	ret = i2c_master_send(client, (u8 *)&msg->content,
+			      sizeof(msg->content));
+	if (ret != sizeof(msg->content))
+		return -EIO;
+
+	return 0;
+}
+
+static int pd692x0_reset(struct pd692x0_priv *priv)
+{
+	struct pd692x0_msg msg = pd692x0_msg_template_list[PD692X0_MSG_RESET];
+	const struct i2c_client *client = priv->client;
+	struct pd692x0_msg_content buf = {0};
+	int ret;
+
+	ret = pd692x0_send_msg(priv, &msg);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to reset the controller (%pe)\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	msleep(30);
+
+	ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
+	if (ret != sizeof(buf))
+		return ret < 0 ? ret : -EIO;
+
+	/* Is the reply a successful report message */
+	if (buf.key != PD692X0_KEY_REPORT || buf.sub[0] || buf.sub[1])
+		return -EIO;
+
+	msleep(300);
+
+	ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
+	if (ret != sizeof(buf))
+		return ret < 0 ? ret : -EIO;
+
+	/* Is the boot status without error */
+	if (buf.key != 0x03 || buf.echo != 0xff || buf.sub[0] & 0x1) {
+		dev_err(&client->dev, "PSE controller error\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/* Implementation of the i2c communication in particular when there is
+ * a communication loss. See the "Synchronization During Communication Loss"
+ * paragraph of the Communication Protocol document.
+ */
+static int pd692x0_recv_msg(struct pd692x0_priv *priv,
+			    struct pd692x0_msg *msg,
+			    struct pd692x0_msg_content *buf)
+{
+	const struct i2c_client *client = priv->client;
+	int ret;
+
+	memset(buf, 0, sizeof(*buf));
+	if (msg->delay_recv)
+		msleep(msg->delay_recv);
+	else
+		msleep(30);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	msleep(100);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	ret = pd692x0_send_msg(priv, msg);
+	if (ret)
+		return ret;
+
+	if (msg->delay_recv)
+		msleep(msg->delay_recv);
+	else
+		msleep(30);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	msleep(100);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	msleep(10000);
+
+	ret = pd692x0_send_msg(priv, msg);
+	if (ret)
+		return ret;
+
+	if (msg->delay_recv)
+		msleep(msg->delay_recv);
+	else
+		msleep(30);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	msleep(100);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	return pd692x0_reset(priv);
+
+out:
+	if (msg->content.key == PD692X0_KEY_CMD) {
+		priv->last_cmd_key = true;
+		priv->last_cmd_key_time = jiffies;
+	} else {
+		priv->last_cmd_key = false;
+	}
+
+	return 0;
+}
+
+static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv,
+				struct pd692x0_msg *msg,
+				struct pd692x0_msg_content *buf)
+{
+	struct device *dev = &priv->client->dev;
+	int ret;
+
+	ret = pd692x0_send_msg(priv, msg);
+	if (ret)
+		return ret;
+
+	ret = pd692x0_recv_msg(priv, msg, buf);
+	if (ret)
+		return ret;
+
+	if (msg->content.echo != buf->echo) {
+		dev_err(dev, "Wrong match in message ID\n");
+		return -EIO;
+	}
+
+	/* If the reply is a report message is it successful */
+	if (buf->key == PD692X0_KEY_REPORT &&
+	    (buf->sub[0] || buf->sub[1])) {
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static struct pd692x0_priv *to_pd692x0_priv(struct pse_controller_dev *pcdev)
+{
+	return container_of(pcdev, struct pd692x0_priv, pcdev);
+}
+
+static int pd692x0_fw_unavailable(struct pd692x0_priv *priv)
+{
+	switch (priv->fw_state) {
+	case PD692X0_FW_OK:
+		return 0;
+	case PD692X0_FW_PREPARE:
+	case PD692X0_FW_WRITE:
+	case PD692X0_FW_COMPLETE:
+		dev_err(&priv->client->dev, "Firmware update in progress!\n");
+		return -EBUSY;
+	case PD692X0_FW_BROKEN:
+	case PD692X0_FW_NEED_UPDATE:
+	default:
+		dev_err(&priv->client->dev,
+			"Firmware issue. Please update it!\n");
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pd692x0_ethtool_set_config(struct pse_controller_dev *pcdev,
+				      unsigned long id,
+				      struct netlink_ext_ack *extack,
+				      const struct pse_control_config *config)
+{
+	struct pd692x0_priv *priv = to_pd692x0_priv(pcdev);
+	struct pd692x0_msg_content buf = {0};
+	struct pd692x0_msg msg;
+	int ret;
+
+	ret = pd692x0_fw_unavailable(priv);
+	if (ret)
+		return ret;
+
+	if (priv->admin_state[id] == config->admin_control)
+		return 0;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
+	switch (config->admin_control) {
+	case ETHTOOL_PSE_ADMIN_STATE_ENABLED:
+		msg.content.data[0] = 0x1;
+		break;
+	case ETHTOOL_PSE_ADMIN_STATE_DISABLED:
+		msg.content.data[0] = 0x0;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	msg.content.sub[2] = id;
+	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+	if (ret < 0)
+		return ret;
+
+	priv->admin_state[id] = config->admin_control;
+
+	return 0;
+}
+
+static int pd692x0_ethtool_get_status(struct pse_controller_dev *pcdev,
+				      unsigned long id,
+				      struct netlink_ext_ack *extack,
+				      struct pse_control_status *status)
+{
+	struct pd692x0_priv *priv = to_pd692x0_priv(pcdev);
+	struct pd692x0_msg_content buf = {0};
+	struct pd692x0_msg msg;
+	int ret;
+
+	ret = pd692x0_fw_unavailable(priv);
+	if (ret)
+		return ret;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_GET_PORT_STATUS];
+	msg.content.sub[2] = id;
+	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+	if (ret < 0)
+		return ret;
+
+	/* Compare Port Status (Communication Protocol Document par. 7.1) */
+	if ((buf.sub[0] & 0xf0) == 0x80 || (buf.sub[0] & 0xf0) == 0x90)
+		status->pw_status = ETHTOOL_PSE_PW_D_STATUS_DELIVERING;
+	else if (buf.sub[0] == 0x1b || buf.sub[0] == 0x22)
+		status->pw_status = ETHTOOL_PSE_PW_D_STATUS_SEARCHING;
+	else if (buf.sub[0] == 0x12)
+		status->pw_status = ETHTOOL_PSE_PW_D_STATUS_FAULT;
+	else
+		status->pw_status = ETHTOOL_PSE_PW_D_STATUS_DISABLED;
+
+	if (buf.sub[1])
+		status->admin_state = ETHTOOL_PSE_ADMIN_STATE_ENABLED;
+	else
+		status->admin_state = ETHTOOL_PSE_ADMIN_STATE_DISABLED;
+
+	priv->admin_state[id] = status->admin_state;
+
+	return 0;
+}
+
+static struct pd692x0_msg_ver pd692x0_get_sw_version(struct pd692x0_priv *priv)
+{
+	struct pd692x0_msg msg = pd692x0_msg_template_list[PD692X0_MSG_GET_SW_VER];
+	struct device *dev = &priv->client->dev;
+	struct pd692x0_msg_content buf = {0};
+	struct pd692x0_msg_ver ver = {0};
+	int ret;
+
+	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get PSE version (%pe)\n", ERR_PTR(ret));
+		return ver;
+	}
+
+	/* Extract version from the message */
+	ver.prod = buf.sub[2];
+	ver.maj_sw_ver = (buf.data[0] << 8 | buf.data[1]) / 100;
+	ver.min_sw_ver = ((buf.data[0] << 8 | buf.data[1]) / 10) % 10;
+	ver.pa_sw_ver = (buf.data[0] << 8 | buf.data[1]) % 10;
+	ver.param = buf.data[2];
+	ver.build = buf.data[3];
+
+	return ver;
+}
+
+static const struct pse_controller_ops pd692x0_ops = {
+	.ethtool_get_status = pd692x0_ethtool_get_status,
+	.ethtool_set_config = pd692x0_ethtool_set_config,
+};
+
+struct matrix {
+	u8 hw_port_a;
+	u8 hw_port_b;
+};
+
+static int
+pd692x0_set_ports_matrix(struct pd692x0_priv *priv,
+			 const struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS])
+{
+	struct pd692x0_msg_content buf;
+	struct pd692x0_msg msg;
+	int ret, i;
+
+	/* Write temporary Matrix */
+	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_TMP_PORT_MATRIX];
+	for (i = 0; i < PD692X0_MAX_LOGICAL_PORTS; i++) {
+		msg.content.sub[2] = i;
+		msg.content.data[0] = port_matrix[i].hw_port_a;
+		msg.content.data[1] = port_matrix[i].hw_port_b;
+
+		ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Program Matrix */
+	msg = pd692x0_msg_template_list[PD692X0_MSG_PRG_PORT_MATRIX];
+	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int
+pd692x0_get_of_matrix(struct device *dev,
+		      struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS])
+{
+	int ret, i, ports_matrix_size;
+	u32 val[PD692X0_MAX_LOGICAL_PORTS * 3];
+
+	ports_matrix_size = device_property_count_u32(dev, "ports-matrix");
+	if (ports_matrix_size <= 0)
+		return -EINVAL;
+	if (ports_matrix_size % 3 ||
+	    ports_matrix_size > PD692X0_MAX_LOGICAL_PORTS * 3) {
+		dev_err(dev, "Not valid ports-matrix property size: %d\n",
+			ports_matrix_size);
+		return -EINVAL;
+	}
+
+	ret = device_property_read_u32_array(dev, "ports-matrix", val,
+					     ports_matrix_size);
+	if (ret < 0)
+		return ret;
+
+	/* Init Matrix */
+	for (i = 0; i < PD692X0_MAX_LOGICAL_PORTS; i++) {
+		port_matrix[i].hw_port_a = 0xff;
+		port_matrix[i].hw_port_b = 0xff;
+	}
+
+	/* Update with values from DT */
+	for (i = 0; i < ports_matrix_size; i += 3) {
+		unsigned int logical_port;
+
+		if (val[i] >= PD692X0_MAX_LOGICAL_PORTS) {
+			dev_err(dev, "Not valid ports-matrix property\n");
+			return -EINVAL;
+		}
+		logical_port = val[i];
+
+		if (val[i + 1] < PD692X0_MAX_HW_PORTS)
+			port_matrix[logical_port].hw_port_a = val[i + 1];
+
+		if (val[i + 2] < PD692X0_MAX_HW_PORTS)
+			port_matrix[logical_port].hw_port_b = val[i + 2];
+	}
+
+	return 0;
+}
+
+static int pd692x0_update_matrix(struct pd692x0_priv *priv)
+{
+	struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS];
+	struct device *dev = &priv->client->dev;
+	int ret;
+
+	ret = pd692x0_get_of_matrix(dev, port_matrix);
+	if (ret < 0) {
+		dev_warn(dev,
+			 "Unable to parse port-matrix, saved matrix will be used\n");
+		return 0;
+	}
+
+	ret = pd692x0_set_ports_matrix(priv, port_matrix);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+#define PD692X0_FW_LINE_MAX_SZ 128
+static int pd692x0_fw_get_next_line(const u8 *data,
+				    char *line, size_t size)
+{
+	size_t line_size;
+	int i;
+
+	line_size = min_t(size_t, size, (size_t)PD692X0_FW_LINE_MAX_SZ);
+
+	memset(line, 0, PD692X0_FW_LINE_MAX_SZ);
+	for (i = 0; i < line_size - 1; i++) {
+		if (*data == '\r' && *(data + 1) == '\n') {
+			line[i] = '\r';
+			line[i + 1] = '\n';
+			return i + 2;
+		}
+		line[i] = *data;
+		data++;
+	}
+
+	return 0;
+}
+
+static enum fw_upload_err
+pd692x0_fw_recv_resp(const struct i2c_client *client, unsigned long ms_timeout,
+		     const char *msg_ok, unsigned int msg_size)
+{
+	/* Maximum controller response size */
+	char fw_msg_buf[5] = {0};
+	unsigned long timeout;
+	int ret;
+
+	if (msg_size > sizeof(fw_msg_buf))
+		return FW_UPLOAD_ERR_RW_ERROR;
+
+	/* Read until we get something */
+	timeout = msecs_to_jiffies(ms_timeout) + jiffies;
+	while (true) {
+		if (time_is_before_jiffies(timeout))
+			return FW_UPLOAD_ERR_TIMEOUT;
+
+		ret = i2c_master_recv(client, fw_msg_buf, 1);
+		if (ret < 0 || *fw_msg_buf == 0) {
+			usleep_range(1000, 2000);
+			continue;
+		} else {
+			break;
+		}
+	}
+
+	/* Read remaining characters */
+	ret = i2c_master_recv(client, fw_msg_buf + 1, msg_size - 1);
+	if (!strncmp(fw_msg_buf, msg_ok, msg_size)) {
+		return FW_UPLOAD_ERR_NONE;
+	} else {
+		dev_err(&client->dev,
+			"Wrong FW download process answer (%*pE)\n",
+			msg_size, fw_msg_buf);
+		return FW_UPLOAD_ERR_HW_ERROR;
+	}
+}
+
+static int pd692x0_fw_write_line(const struct i2c_client *client,
+				 const char line[PD692X0_FW_LINE_MAX_SZ],
+				 const bool last_line)
+{
+	int ret;
+
+	while (*line != 0) {
+		ret = i2c_master_send(client, line, 1);
+		if (ret < 0)
+			return FW_UPLOAD_ERR_RW_ERROR;
+		line++;
+	}
+
+	if (last_line) {
+		ret = pd692x0_fw_recv_resp(client, 100, "TP\r\n",
+					   sizeof("TP\r\n") - 1);
+		if (ret)
+			return ret;
+	} else {
+		ret = pd692x0_fw_recv_resp(client, 100, "T*\r\n",
+					   sizeof("T*\r\n") - 1);
+		if (ret)
+			return ret;
+	}
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err pd692x0_fw_reset(const struct i2c_client *client)
+{
+	const struct pd692x0_msg_content zero = {0};
+	struct pd692x0_msg_content buf = {0};
+	unsigned long timeout;
+	char cmd[] = "RST";
+	int ret;
+
+	ret = i2c_master_send(client, cmd, strlen(cmd));
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to reset the controller (%pe)\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	timeout = msecs_to_jiffies(10000) + jiffies;
+	while (true) {
+		if (time_is_before_jiffies(timeout))
+			return FW_UPLOAD_ERR_TIMEOUT;
+
+		ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
+		if (ret < 0 ||
+		    !memcmp(&buf, &zero, sizeof(buf)))
+			usleep_range(1000, 2000);
+		else
+			break;
+	}
+
+	/* Is the reply a successful report message */
+	if (buf.key != PD692X0_KEY_TLM || buf.echo != 0xff ||
+	    buf.sub[0] & 0x01) {
+		dev_err(&client->dev, "PSE controller error\n");
+		return FW_UPLOAD_ERR_HW_ERROR;
+	}
+
+	/* Is the firmware operational */
+	if (buf.sub[0] & 0x02) {
+		dev_err(&client->dev,
+			"PSE firmware error. Please update it.\n");
+		return FW_UPLOAD_ERR_HW_ERROR;
+	}
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err pd692x0_fw_prepare(struct fw_upload *fwl,
+					     const u8 *data, u32 size)
+{
+	struct pd692x0_priv *priv = fwl->dd_handle;
+	const struct i2c_client *client = priv->client;
+	enum pd692x0_fw_state last_fw_state;
+	int ret;
+
+	priv->cancel_request = false;
+	last_fw_state = priv->fw_state;
+
+	priv->fw_state = PD692X0_FW_PREPARE;
+
+	/* Enter program mode */
+	if (last_fw_state == PD692X0_FW_BROKEN) {
+		const char *msg = "ENTR";
+		const char *c;
+
+		c = msg;
+		do {
+			ret = i2c_master_send(client, c, 1);
+			if (ret < 0)
+				return FW_UPLOAD_ERR_RW_ERROR;
+			if (*(c + 1))
+				usleep_range(10000, 20000);
+		} while (*(++c));
+	} else {
+		struct pd692x0_msg_content buf;
+		struct pd692x0_msg msg;
+
+		msg = pd692x0_msg_template_list[PD692X0_MSG_DOWNLOAD_CMD];
+		ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+		if (ret < 0) {
+			dev_err(&client->dev,
+				"Failed to enter programming mode (%pe)\n",
+				ERR_PTR(ret));
+			return FW_UPLOAD_ERR_RW_ERROR;
+		}
+	}
+
+	ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1);
+	if (ret)
+		goto err_out;
+
+	if (priv->cancel_request) {
+		ret = FW_UPLOAD_ERR_CANCELED;
+		goto err_out;
+	}
+
+	return FW_UPLOAD_ERR_NONE;
+
+err_out:
+	pd692x0_fw_reset(priv->client);
+	priv->fw_state = last_fw_state;
+	return ret;
+}
+
+static enum fw_upload_err pd692x0_fw_write(struct fw_upload *fwl,
+					   const u8 *data, u32 offset,
+					   u32 size, u32 *written)
+{
+	struct pd692x0_priv *priv = fwl->dd_handle;
+	char line[PD692X0_FW_LINE_MAX_SZ];
+	const struct i2c_client *client;
+	int ret, i;
+	char cmd;
+
+	client = priv->client;
+	priv->fw_state = PD692X0_FW_WRITE;
+
+	/* Erase */
+	cmd = 'E';
+	ret = i2c_master_send(client, &cmd, 1);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to boot programming mode (%pe)\n",
+			ERR_PTR(ret));
+		return FW_UPLOAD_ERR_RW_ERROR;
+	}
+
+	ret = pd692x0_fw_recv_resp(client, 100, "TOE\r\n", sizeof("TOE\r\n") - 1);
+	if (ret)
+		return ret;
+
+	ret = pd692x0_fw_recv_resp(client, 5000, "TE\r\n", sizeof("TE\r\n") - 1);
+	if (ret)
+		dev_warn(&client->dev,
+			 "Failed to erase internal memory, however still try to write Firmware\n");
+
+	ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1);
+	if (ret)
+		dev_warn(&client->dev,
+			 "Failed to erase internal memory, however still try to write Firmware\n");
+
+	if (priv->cancel_request)
+		return FW_UPLOAD_ERR_CANCELED;
+
+	/* Program */
+	cmd = 'P';
+	ret = i2c_master_send(client, &cmd, sizeof(char));
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to boot programming mode (%pe)\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	ret = pd692x0_fw_recv_resp(client, 100, "TOP\r\n", sizeof("TOP\r\n") - 1);
+	if (ret)
+		return ret;
+
+	i = 0;
+	while (i < size) {
+		ret = pd692x0_fw_get_next_line(data, line, size - i);
+		if (!ret) {
+			ret = FW_UPLOAD_ERR_FW_INVALID;
+			goto err;
+		}
+
+		i += ret;
+		data += ret;
+		if (line[0] == 'S' && line[1] == '0') {
+			continue;
+		} else if (line[0] == 'S' && line[1] == '7') {
+			ret = pd692x0_fw_write_line(client, line, true);
+			if (ret)
+				goto err;
+		} else {
+			ret = pd692x0_fw_write_line(client, line, false);
+			if (ret)
+				goto err;
+		}
+
+		if (priv->cancel_request) {
+			ret = FW_UPLOAD_ERR_CANCELED;
+			goto err;
+		}
+	}
+	*written = i;
+
+	msleep(400);
+
+	return FW_UPLOAD_ERR_NONE;
+
+err:
+	pd692x0_fw_write_line(client, "S7\r\n", true);
+	return ret;
+}
+
+static enum fw_upload_err pd692x0_fw_poll_complete(struct fw_upload *fwl)
+{
+	struct pd692x0_priv *priv = fwl->dd_handle;
+	const struct i2c_client *client = priv->client;
+	struct pd692x0_msg_ver ver;
+	int ret;
+
+	priv->fw_state = PD692X0_FW_COMPLETE;
+
+	ret = pd692x0_fw_reset(client);
+	if (ret)
+		return ret;
+
+	ver = pd692x0_get_sw_version(priv);
+	if (ver.maj_sw_ver != PD692X0_FW_MAJ_VER) {
+		dev_err(&client->dev,
+			"Too old firmware version. Please update it\n");
+		priv->fw_state = PD692X0_FW_NEED_UPDATE;
+		return FW_UPLOAD_ERR_FW_INVALID;
+	}
+
+	ret = pd692x0_update_matrix(priv);
+	if (ret < 0) {
+		dev_err(&client->dev, "Error configuring ports matrix (%pe)\n",
+			ERR_PTR(ret));
+		priv->fw_state = PD692X0_FW_NEED_UPDATE;
+		return FW_UPLOAD_ERR_HW_ERROR;
+	}
+
+	priv->fw_state = PD692X0_FW_OK;
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void pd692x0_fw_cancel(struct fw_upload *fwl)
+{
+	struct pd692x0_priv *priv = fwl->dd_handle;
+
+	priv->cancel_request = true;
+}
+
+static void pd692x0_fw_cleanup(struct fw_upload *fwl)
+{
+	struct pd692x0_priv *priv = fwl->dd_handle;
+
+	switch (priv->fw_state) {
+	case PD692X0_FW_WRITE:
+		pd692x0_fw_reset(priv->client);
+		fallthrough;
+	case PD692X0_FW_COMPLETE:
+		priv->fw_state = PD692X0_FW_BROKEN;
+		break;
+	default:
+		break;
+	}
+}
+
+static const struct fw_upload_ops pd692x0_fw_ops = {
+	.prepare = pd692x0_fw_prepare,
+	.write = pd692x0_fw_write,
+	.poll_complete = pd692x0_fw_poll_complete,
+	.cancel = pd692x0_fw_cancel,
+	.cleanup = pd692x0_fw_cleanup,
+};
+
+static int pd692x0_i2c_probe(struct i2c_client *client)
+{
+	struct pd692x0_msg_content buf = {0};
+	struct device *dev = &client->dev;
+	struct pd692x0_msg_ver ver;
+	struct pd692x0_priv *priv;
+	struct fw_upload *fwl;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(dev, "i2c check functionality failed\n");
+		return -ENXIO;
+	}
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	i2c_set_clientdata(client, priv);
+
+	priv->pcdev.owner = THIS_MODULE;
+	priv->pcdev.ops = &pd692x0_ops;
+	priv->pcdev.dev = dev;
+	priv->pcdev.types = PSE_POE;
+	priv->pcdev.of_pse_n_cells = 1;
+	priv->pcdev.nr_lines = PD692X0_MAX_LOGICAL_PORTS;
+	ret = devm_pse_controller_register(dev, &priv->pcdev);
+	if (ret) {
+		return dev_err_probe(dev, ret,
+				     "failed to register PSE controller\n");
+	}
+
+	fwl = firmware_upload_register(THIS_MODULE, dev, dev_name(dev),
+				       &pd692x0_fw_ops, priv);
+	if (IS_ERR(fwl)) {
+		dev_err(dev, "Failed to register to the Firmware Upload API\n");
+		ret = PTR_ERR(fwl);
+		return ret;
+	}
+	priv->fwl = fwl;
+
+	ret = i2c_master_recv(client, (u8 *)&buf, sizeof(buf));
+	if (ret != sizeof(buf)) {
+		dev_err(dev, "Failed to get device status\n");
+		ret = -EIO;
+		goto err_fw_unregister;
+	}
+
+	if (buf.key != 0x03 || buf.echo != 0xff || buf.sub[0] & 0x01) {
+		dev_err(dev, "PSE controller error\n");
+		ret = -EIO;
+		goto err_fw_unregister;
+	}
+
+	if (buf.sub[0] & 0x02) {
+		dev_err(dev, "PSE firmware error. Please update it.\n");
+		priv->fw_state = PD692X0_FW_BROKEN;
+		return 0;
+	}
+
+	ver = pd692x0_get_sw_version(priv);
+	dev_info(&client->dev, "Software version %d.%02d.%d.%d\n", ver.prod,
+		 ver.maj_sw_ver, ver.min_sw_ver, ver.pa_sw_ver);
+
+	if (ver.maj_sw_ver != PD692X0_FW_MAJ_VER) {
+		dev_err(dev, "Too old firmware version. Please update it\n");
+		priv->fw_state = PD692X0_FW_NEED_UPDATE;
+		return 0;
+	}
+
+	ret = pd692x0_update_matrix(priv);
+	if (ret < 0) {
+		dev_err(dev, "Error configuring ports matrix (%pe)\n",
+			ERR_PTR(ret));
+		goto err_fw_unregister;
+	}
+
+	priv->fw_state = PD692X0_FW_OK;
+	return 0;
+
+err_fw_unregister:
+	firmware_upload_unregister(priv->fwl);
+	return ret;
+}
+
+static void pd692x0_i2c_remove(struct i2c_client *client)
+{
+	struct pd692x0_priv *priv = i2c_get_clientdata(client);
+
+	firmware_upload_unregister(priv->fwl);
+}
+
+static const struct i2c_device_id pd692x0_id[] = {
+	{ PD692X0_PSE_NAME, 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, pd692x0_id);
+
+static const struct of_device_id pd692x0_of_match[] = {
+	{ .compatible = "microchip,pd69200", },
+	{ .compatible = "microchip,pd69210", },
+	{ .compatible = "microchip,pd69220", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pd692x0_of_match);
+
+static struct i2c_driver pd692x0_driver = {
+	.probe		= pd692x0_i2c_probe,
+	.remove		= pd692x0_i2c_remove,
+	.id_table	= pd692x0_id,
+	.driver		= {
+		.name		= PD692X0_PSE_NAME,
+		.of_match_table = of_match_ptr(pd692x0_of_match),
+	},
+};
+module_i2c_driver(pd692x0_driver);
+
+MODULE_AUTHOR("Kory Maincent <kory.maincent@bootlin.com>");
+MODULE_DESCRIPTION("Microchip PD692x0 PoE PSE Controller driver");
+MODULE_LICENSE("GPL");

-- 
2.25.1

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Krzysztof Kozlowski]

On 16/11/2023 15:01, Kory Maincent wrote:
> Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller.
> This driver only support i2c communication for now.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  MAINTAINERS                  |    1 +
>  drivers/net/pse-pd/Kconfig   |   11 +
>  drivers/net/pse-pd/Makefile  |    1 +
>  drivers/net/pse-pd/pd692x0.c | 1049 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1062 insertions(+)

....

> +
> +err_fw_unregister:
> +	firmware_upload_unregister(priv->fwl);
> +	return ret;
> +}
> +
> +static void pd692x0_i2c_remove(struct i2c_client *client)
> +{
> +	struct pd692x0_priv *priv = i2c_get_clientdata(client);
> +
> +	firmware_upload_unregister(priv->fwl);
> +}
> +
> +static const struct i2c_device_id pd692x0_id[] = {
> +	{ PD692X0_PSE_NAME, 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, pd692x0_id);
> +
> +static const struct of_device_id pd692x0_of_match[] = {
> +	{ .compatible = "microchip,pd69200", },
> +	{ .compatible = "microchip,pd69210", },
> +	{ .compatible = "microchip,pd69220", },

So they are the same from driver point of view.

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, pd692x0_of_match);
> +
> +static struct i2c_driver pd692x0_driver = {
> +	.probe		= pd692x0_i2c_probe,
> +	.remove		= pd692x0_i2c_remove,
> +	.id_table	= pd692x0_id,
> +	.driver		= {
> +		.name		= PD692X0_PSE_NAME,
> +		.of_match_table = of_match_ptr(pd692x0_of_match),

Drop of_match_ptr, leads to warnings.


Best regards,
Krzysztof

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Köry Maincent]

Thanks Krzysztof for your reviews!

On Thu, 16 Nov 2023 15:29:24 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 16/11/2023 15:01, Kory Maincent wrote:
> > Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller.
> > This driver only support i2c communication for now.
> > 
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> > ---
> >  MAINTAINERS                  |    1 +
> >  drivers/net/pse-pd/Kconfig   |   11 +
> >  drivers/net/pse-pd/Makefile  |    1 +
> >  drivers/net/pse-pd/pd692x0.c | 1049
> > ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1062
> > insertions(+)  
> 
> ....
> 
> > +
> > +err_fw_unregister:
> > +	firmware_upload_unregister(priv->fwl);
> > +	return ret;
> > +}
> > +
> > +static void pd692x0_i2c_remove(struct i2c_client *client)
> > +{
> > +	struct pd692x0_priv *priv = i2c_get_clientdata(client);
> > +
> > +	firmware_upload_unregister(priv->fwl);
> > +}
> > +
> > +static const struct i2c_device_id pd692x0_id[] = {
> > +	{ PD692X0_PSE_NAME, 0 },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, pd692x0_id);
> > +
> > +static const struct of_device_id pd692x0_of_match[] = {
> > +	{ .compatible = "microchip,pd69200", },
> > +	{ .compatible = "microchip,pd69210", },
> > +	{ .compatible = "microchip,pd69220", },  
> 
> So they are the same from driver point of view.

Yes.
I only have the pd69200 version but the three versions are theoretically
compatible and microchip advise obviously to use the last one.
I describe the three names in case of future specific things even if I hope
there won't be and to have a clear version of which version is supported. Do you
prefer to use pd692x0 compatible instead?

Regards,

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Andrew Lunn]

> +static int pd692x0_send_msg(struct pd692x0_priv *priv, struct pd692x0_msg *msg)
> +{
> +	const struct i2c_client *client = priv->client;
> +	int ret;
> +
> +	if (msg->content.key == PD692X0_KEY_CMD && priv->last_cmd_key) {
> +		while (time_is_after_jiffies(msecs_to_jiffies(30) + priv->last_cmd_key_time))
> +			usleep_range(1000, 2000);

That is a bit odd. Could you not just calculate how long a sleep is
needed, rather than loop?

	Andrew

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Köry Maincent]

On Thu, 16 Nov 2023 23:38:08 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +static int pd692x0_send_msg(struct pd692x0_priv *priv, struct pd692x0_msg
> > *msg) +{
> > +	const struct i2c_client *client = priv->client;
> > +	int ret;
> > +
> > +	if (msg->content.key == PD692X0_KEY_CMD && priv->last_cmd_key) {
> > +		while (time_is_after_jiffies(msecs_to_jiffies(30) +
> > priv->last_cmd_key_time))
> > +			usleep_range(1000, 2000);  
> 
> That is a bit odd. Could you not just calculate how long a sleep is
> needed, rather than loop?

Oh, right indeed! Don't know why my brain wanted a loop here.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Andrew Lunn]

> +struct pd692x0_msg {
> +	struct pd692x0_msg_content content;
> +	u16 delay_recv;
> +};

> +	if (msg->delay_recv)
> +		msleep(msg->delay_recv);
> +	else
> +		msleep(30);

> +	if (msg->delay_recv)
> +		msleep(msg->delay_recv);
> +	else
> +		msleep(30);

> +	if (msg->delay_recv)
> +		msleep(msg->delay_recv);
> +	else
> +		msleep(30);
> +

As far as i can see with a quick search, nothing ever sets delay_recv?

	Andrew

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Köry Maincent]

Thanks for your review!

On Thu, 16 Nov 2023 23:41:55 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > +struct pd692x0_msg {
> > +	struct pd692x0_msg_content content;
> > +	u16 delay_recv;
> > +};  
> 
> > +	if (msg->delay_recv)
> > +		msleep(msg->delay_recv);
> > +	else
> > +		msleep(30);  
> 
> > +	if (msg->delay_recv)
> > +		msleep(msg->delay_recv);
> > +	else
> > +		msleep(30);  
> 
> > +	if (msg->delay_recv)
> > +		msleep(msg->delay_recv);
> > +	else
> > +		msleep(30);
> > +  
> 
> As far as i can see with a quick search, nothing ever sets delay_recv?
> 
> 	Andrew

In fact I wrote the driver taking into account that there are two commands (save
and restore) that need a different delay response. As currently we do not
support them I can indeed drop it for now and add it back when I will add their
support.

Regards,

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Andrew Lunn]

On Fri, Nov 17, 2023 at 12:22:36PM +0100, Köry Maincent wrote:
> Thanks for your review!
> 
> On Thu, 16 Nov 2023 23:41:55 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > > +struct pd692x0_msg {
> > > +	struct pd692x0_msg_content content;
> > > +	u16 delay_recv;
> > > +};  
> > 
> > > +	if (msg->delay_recv)
> > > +		msleep(msg->delay_recv);
> > > +	else
> > > +		msleep(30);  
> > 
> > > +	if (msg->delay_recv)
> > > +		msleep(msg->delay_recv);
> > > +	else
> > > +		msleep(30);  
> > 
> > > +	if (msg->delay_recv)
> > > +		msleep(msg->delay_recv);
> > > +	else
> > > +		msleep(30);
> > > +  
> > 
> > As far as i can see with a quick search, nothing ever sets delay_recv?
> > 
> > 	Andrew
> 
> In fact I wrote the driver taking into account that there are two commands (save
> and restore) that need a different delay response. As currently we do not
> support them I can indeed drop it for now and add it back when I will add their
> support.

When you add it back, maybe just arrange for delay_recv to be set to
30?

	Andrew

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Andrew Lunn]

I'm reading this patch first, so this might be a dumb question...

> +static int pd692x0_recv_msg(struct pd692x0_priv *priv,
> +			    struct pd692x0_msg *msg,
> +			    struct pd692x0_msg_content *buf)
> +{

...

> +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> +	if (buf->key)
> +		goto out;
> +
> +	msleep(10000);

That is 10 seconds, right?

> +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv,
> +				struct pd692x0_msg *msg,
> +				struct pd692x0_msg_content *buf)
> +{
> +	struct device *dev = &priv->client->dev;
> +	int ret;
> +
> +	ret = pd692x0_send_msg(priv, msg);
> +	if (ret)
> +		return ret;
> +
> +	ret = pd692x0_recv_msg(priv, msg, buf);

So this function takes at least 10 seconds?

> +static int pd692x0_ethtool_set_config(struct pse_controller_dev *pcdev,
> +				      unsigned long id,
> +				      struct netlink_ext_ack *extack,
> +				      const struct pse_control_config *config)
> +{

....

> +	switch (config->admin_control) {
> +	case ETHTOOL_PSE_ADMIN_STATE_ENABLED:
> +		msg.content.data[0] = 0x1;
> +		break;
> +	case ETHTOOL_PSE_ADMIN_STATE_DISABLED:
> +		msg.content.data[0] = 0x0;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	msg.content.sub[2] = id;
> +	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);

So this is also 10 seconds? 

Given its name, it looks like this is called via ethtool? Is the
ethtool core holding RTNL? It is generally considered bad to hold RTNL for
that long.

     Andrew

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Köry Maincent]

Thanks for your reviews!

On Thu, 16 Nov 2023 23:54:02 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> I'm reading this patch first, so this might be a dumb question...
> 
> > +static int pd692x0_recv_msg(struct pd692x0_priv *priv,
> > +			    struct pd692x0_msg *msg,
> > +			    struct pd692x0_msg_content *buf)
> > +{  
> 
> ...
> 
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		goto out;
> > +
> > +	msleep(10000);  
> 
> That is 10 seconds, right?

Yes, it is in the communication loss procedure.

> 
> > +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv,
> > +				struct pd692x0_msg *msg,
> > +				struct pd692x0_msg_content *buf)
> > +{
> > +	struct device *dev = &priv->client->dev;
> > +	int ret;
> > +
> > +	ret = pd692x0_send_msg(priv, msg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = pd692x0_recv_msg(priv, msg, buf);  
> 
> So this function takes at least 10 seconds?

No, on normal communication it takes a bit more than 30ms.
It could be more if there are communication loss, even reset the controller.
See the communication loss procedure in "PD692x0 BT Serial Communication
Protocol User Guide" for details.

> > +static int pd692x0_ethtool_set_config(struct pse_controller_dev *pcdev,
> > +				      unsigned long id,
> > +				      struct netlink_ext_ack *extack,
> > +				      const struct pse_control_config
> > *config) +{  
> 
> ....
> 
> > +	switch (config->admin_control) {
> > +	case ETHTOOL_PSE_ADMIN_STATE_ENABLED:
> > +		msg.content.data[0] = 0x1;
> > +		break;
> > +	case ETHTOOL_PSE_ADMIN_STATE_DISABLED:
> > +		msg.content.data[0] = 0x0;
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	msg.content.sub[2] = id;
> > +	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);  
> 
> So this is also 10 seconds? 
> 
> Given its name, it looks like this is called via ethtool? Is the
> ethtool core holding RTNL? It is generally considered bad to hold RTNL for
> that long.

Yes it is holding RTNL lock. Should I consider another behavior in case of
communication loss to not holding RTNL lock so long?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Andrew Lunn]

> > > +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv,
> > > +				struct pd692x0_msg *msg,
> > > +				struct pd692x0_msg_content *buf)
> > > +{
> > > +	struct device *dev = &priv->client->dev;
> > > +	int ret;
> > > +
> > > +	ret = pd692x0_send_msg(priv, msg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = pd692x0_recv_msg(priv, msg, buf);  
> > 
> > So this function takes at least 10 seconds?
> 
> No, on normal communication it takes a bit more than 30ms.

So i think the first step is to refactor this code to make it clear
what the normal path is, and what the exception path is, and the
timing of each.

> > > +	msg.content.sub[2] = id;
> > > +	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);  
> > 
> > So this is also 10 seconds? 
> > 
> > Given its name, it looks like this is called via ethtool? Is the
> > ethtool core holding RTNL? It is generally considered bad to hold RTNL for
> > that long.
> 
> Yes it is holding RTNL lock. Should I consider another behavior in case of
> communication loss to not holding RTNL lock so long?

How often does it happen? On the scale of its a theoretical
possibility, through to it happens every N calls? Also, does it happen
on cold boot and reboot?

If its never supposed to happen, i would keep holding RTNL, and add a
pr_warn() that the PSE has crashed and burned, waiting for it to
reboot. If this is likely to happen on the first communication with
the device, we might want to do a dummy transfer during probe to get
is synchronized before we start using it with the RTNL held.

   Andrew

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Köry Maincent]

On Wed, 22 Nov 2023 17:54:53 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > > > +static int pd692x0_sendrecv_msg(struct pd692x0_priv *priv,
> > > > +				struct pd692x0_msg *msg,
> > > > +				struct pd692x0_msg_content *buf)
> > > > +{
> > > > +	struct device *dev = &priv->client->dev;
> > > > +	int ret;
> > > > +
> > > > +	ret = pd692x0_send_msg(priv, msg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = pd692x0_recv_msg(priv, msg, buf);    
> > > 
> > > So this function takes at least 10 seconds?  
> > 
> > No, on normal communication it takes a bit more than 30ms.  
> 
> So i think the first step is to refactor this code to make it clear
> what the normal path is, and what the exception path is, and the
> timing of each.

Ok I will try to refactor it to makes it more readable.

> > > > +	msg.content.sub[2] = id;
> > > > +	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);    
> > > 
> > > So this is also 10 seconds? 
> > > 
> > > Given its name, it looks like this is called via ethtool? Is the
> > > ethtool core holding RTNL? It is generally considered bad to hold RTNL for
> > > that long.  
> > 
> > Yes it is holding RTNL lock. Should I consider another behavior in case of
> > communication loss to not holding RTNL lock so long?  
> 
> How often does it happen? On the scale of its a theoretical
> possibility, through to it happens every N calls? Also, does it happen
> on cold boot and reboot?
> 
> If its never supposed to happen, i would keep holding RTNL, and add a
> pr_warn() that the PSE has crashed and burned, waiting for it to
> reboot. If this is likely to happen on the first communication with
> the device, we might want to do a dummy transfer during probe to get
> is synchronized before we start using it with the RTNL held.

I would say it never supposed to happen.
I never faced the issue playing with the controller. The first communication is
a simple i2c_master_recv of the controller status without entering the
pd692x0_sendrecv_msg function, therefore it won't be an issue.

Another solution could be to raise a flag if I enter in communication loss and
release the rtnlock. We would lock again the rtnl when the flags got disabled.
The controler won't be accessible until the flag is disabled.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Andrew Lunn]

> I would say it never supposed to happen.
> I never faced the issue playing with the controller. The first communication is
> a simple i2c_master_recv of the controller status without entering the
> pd692x0_sendrecv_msg function, therefore it won't be an issue.

Great. Do a pr_info() or similar and keep holding the lock. KISS.

       Andrew

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Andrew Lunn]

> +struct pd692x0_priv {
> +	struct i2c_client *client;
> +	struct pse_controller_dev pcdev;
> +
> +	enum pd692x0_fw_state fw_state;
> +	struct fw_upload *fwl;
> +	bool cancel_request:1;
> +
> +	u8 msg_id;
> +	bool last_cmd_key:1;

Does a bool bit field of size 1 make any sense?  I would also put the
two bitfields next to each other, and the compiler might then pack
them into the same word. The base type of a u8 would allow the compile
to put it next to the msg_id without any padding.

> +	unsigned long last_cmd_key_time;
> +
> +	enum ethtool_pse_admin_state admin_state[PD692X0_MAX_LOGICAL_PORTS];
> +};
> +
> +/* Template list of the fixed bytes of the communication messages */
> +static const struct pd692x0_msg pd692x0_msg_template_list[PD692X0_MSG_CNT] = {
> +	[PD692X0_MSG_RESET] = {
> +		.content = {
> +			.key = PD692X0_KEY_CMD,
> +			.sub = {0x07, 0x55, 0x00},
> +			.data = {0x55, 0x00, 0x55, 0x4e,
> +				 0x4e, 0x4e, 0x4e, 0x4e},
> +		},
> +	},

Is there any documentation about what all these magic number mean?

> +/* Implementation of the i2c communication in particular when there is
> + * a communication loss. See the "Synchronization During Communication Loss"
> + * paragraph of the Communication Protocol document.
> + */

Is this document public?

> +static int pd692x0_recv_msg(struct pd692x0_priv *priv,
> +			    struct pd692x0_msg *msg,
> +			    struct pd692x0_msg_content *buf)
> +{
> +	const struct i2c_client *client = priv->client;
> +	int ret;
> +
> +	memset(buf, 0, sizeof(*buf));
> +	if (msg->delay_recv)
> +		msleep(msg->delay_recv);
> +	else
> +		msleep(30);
> +
> +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> +	if (buf->key)
> +		goto out;

This is the first attempt to receive the message. I assume buf->key
not being 0 indicates something has been received?

> +
> +	msleep(100);
> +
> +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> +	if (buf->key)
> +		goto out;

So this is a second attempt. Should there be another memset? Could the
first failed transfer fill the buffer with random junk in the higher
bytes, and a successful read here could be a partial read and the end
of the buffer still contains the junk.

> +
> +	ret = pd692x0_send_msg(priv, msg);
> +	if (ret)
> +		return ret;

So now we are re-transmitting the request.

> +
> +	if (msg->delay_recv)
> +		msleep(msg->delay_recv);
> +	else
> +		msleep(30);
> +
> +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> +	if (buf->key)
> +		goto out;
> +
> +	msleep(100);
> +
> +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> +	if (buf->key)
> +		goto out;
> +
> +	msleep(10000);

And two more attemps to receive it.

> +
> +	ret = pd692x0_send_msg(priv, msg);
> +	if (ret)
> +		return ret;
> +
> +	if (msg->delay_recv)
> +		msleep(msg->delay_recv);
> +	else
> +		msleep(30);
> +
> +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> +	if (buf->key)
> +		goto out;
> +
> +	msleep(100);
> +
> +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> +	if (buf->key)
> +		goto out;

Another resend and two more attempts to receive.

Is there a reason to not uses for loops here? And maybe put
send/receive/receive into a helper? And maybe make the first send part
of this, rather then separate? I think the code will be more readable
when restructured.

> +static int pd692x0_ethtool_set_config(struct pse_controller_dev *pcdev,
> +				      unsigned long id,
> +				      struct netlink_ext_ack *extack,
> +				      const struct pse_control_config *config)
> +{
> +	struct pd692x0_priv *priv = to_pd692x0_priv(pcdev);
> +	struct pd692x0_msg_content buf = {0};
> +	struct pd692x0_msg msg;
> +	int ret;
> +
> +	ret = pd692x0_fw_unavailable(priv);
> +	if (ret)
> +		return ret;

It seems a bit late to check if the device has any firmware. I would
of expected probe to check that, and maybe attempt to download
firmware. If that fails, fail the probe, since the PSE is a brick.

> +static struct pd692x0_msg_ver pd692x0_get_sw_version(struct pd692x0_priv *priv)
> +{
> +	struct pd692x0_msg msg = pd692x0_msg_template_list[PD692X0_MSG_GET_SW_VER];
> +	struct device *dev = &priv->client->dev;
> +	struct pd692x0_msg_content buf = {0};
> +	struct pd692x0_msg_ver ver = {0};
> +	int ret;
> +
> +	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to get PSE version (%pe)\n", ERR_PTR(ret));
> +		return ver;

I _think_ that return is wrong ???

> +static enum fw_upload_err pd692x0_fw_write(struct fw_upload *fwl,
> +					   const u8 *data, u32 offset,
> +					   u32 size, u32 *written)
> +{
> +	struct pd692x0_priv *priv = fwl->dd_handle;
> +	char line[PD692X0_FW_LINE_MAX_SZ];
> +	const struct i2c_client *client;
> +	int ret, i;
> +	char cmd;
> +
> +	client = priv->client;
> +	priv->fw_state = PD692X0_FW_WRITE;
> +
> +	/* Erase */
> +	cmd = 'E';
> +	ret = i2c_master_send(client, &cmd, 1);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to boot programming mode (%pe)\n",
> +			ERR_PTR(ret));
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +	}
> +
> +	ret = pd692x0_fw_recv_resp(client, 100, "TOE\r\n", sizeof("TOE\r\n") - 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = pd692x0_fw_recv_resp(client, 5000, "TE\r\n", sizeof("TE\r\n") - 1);
> +	if (ret)
> +		dev_warn(&client->dev,
> +			 "Failed to erase internal memory, however still try to write Firmware\n");
> +
> +	ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1);
> +	if (ret)
> +		dev_warn(&client->dev,
> +			 "Failed to erase internal memory, however still try to write Firmware\n");
> +
> +	if (priv->cancel_request)
> +		return FW_UPLOAD_ERR_CANCELED;
> +
> +	/* Program */
> +	cmd = 'P';
> +	ret = i2c_master_send(client, &cmd, sizeof(char));
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to boot programming mode (%pe)\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = pd692x0_fw_recv_resp(client, 100, "TOP\r\n", sizeof("TOP\r\n") - 1);
> +	if (ret)
> +		return ret;
> +
> +	i = 0;
> +	while (i < size) {
> +		ret = pd692x0_fw_get_next_line(data, line, size - i);
> +		if (!ret) {
> +			ret = FW_UPLOAD_ERR_FW_INVALID;
> +			goto err;
> +		}
> +
> +		i += ret;
> +		data += ret;
> +		if (line[0] == 'S' && line[1] == '0') {
> +			continue;
> +		} else if (line[0] == 'S' && line[1] == '7') {
> +			ret = pd692x0_fw_write_line(client, line, true);
> +			if (ret)
> +				goto err;

Is the firmware in Motorola SREC format? I thought the kernel had a
helper for that, but a quick search did not find it. So maybe i'm
remembering wrongly. But it seems silly for every driver to implement
an SREC parser.

   Andrew

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Köry Maincent]

On Sat, 18 Nov 2023 19:54:30 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> 
> > +	unsigned long last_cmd_key_time;
> > +
> > +	enum ethtool_pse_admin_state
> > admin_state[PD692X0_MAX_LOGICAL_PORTS]; +};
> > +
> > +/* Template list of the fixed bytes of the communication messages */
> > +static const struct pd692x0_msg pd692x0_msg_template_list[PD692X0_MSG_CNT]
> > = {
> > +	[PD692X0_MSG_RESET] = {
> > +		.content = {
> > +			.key = PD692X0_KEY_CMD,
> > +			.sub = {0x07, 0x55, 0x00},
> > +			.data = {0x55, 0x00, 0x55, 0x4e,
> > +				 0x4e, 0x4e, 0x4e, 0x4e},
> > +		},
> > +	},  
> 
> Is there any documentation about what all these magic number mean?
> 
> > +/* Implementation of the i2c communication in particular when there is
> > + * a communication loss. See the "Synchronization During Communication
> > Loss"
> > + * paragraph of the Communication Protocol document.
> > + */  
> 
> Is this document public?

Yes:
https://www.microchip.com/en-us/software-library/p3-55-firmware-for-pd69200-for-ieee802-3bt

> 
> > +static int pd692x0_recv_msg(struct pd692x0_priv *priv,
> > +			    struct pd692x0_msg *msg,
> > +			    struct pd692x0_msg_content *buf)
> > +{
> > +	const struct i2c_client *client = priv->client;
> > +	int ret;
> > +
> > +	memset(buf, 0, sizeof(*buf));
> > +	if (msg->delay_recv)
> > +		msleep(msg->delay_recv);
> > +	else
> > +		msleep(30);
> > +
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		goto out;  
> 
> This is the first attempt to receive the message. I assume buf->key
> not being 0 indicates something has been received?

Yes, 

> 
> > +
> > +	msleep(100);
> > +
> > +	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
> > +	if (buf->key)
> > +		goto out;  
> 
> So this is a second attempt. Should there be another memset? Could the
> first failed transfer fill the buffer with random junk in the higher
> bytes, and a successful read here could be a partial read and the end
> of the buffer still contains the junk.

Not sure. The message read should have the right size.
I will maybe add the memset to prevent any weird behavior.

> Another resend and two more attempts to receive.
> 
> Is there a reason to not uses for loops here? And maybe put
> send/receive/receive into a helper? And maybe make the first send part
> of this, rather then separate? I think the code will be more readable
> when restructured.

I have written it like that to describe literally the communication loss
procedure. I may restructured it for better understanding.

> > +static int pd692x0_ethtool_set_config(struct pse_controller_dev *pcdev,
> > +				      unsigned long id,
> > +				      struct netlink_ext_ack *extack,
> > +				      const struct pse_control_config
> > *config) +{
> > +	struct pd692x0_priv *priv = to_pd692x0_priv(pcdev);
> > +	struct pd692x0_msg_content buf = {0};
> > +	struct pd692x0_msg msg;
> > +	int ret;
> > +
> > +	ret = pd692x0_fw_unavailable(priv);
> > +	if (ret)
> > +		return ret;  
> 
> It seems a bit late to check if the device has any firmware. I would
> of expected probe to check that, and maybe attempt to download
> firmware. If that fails, fail the probe, since the PSE is a brick.

The PSE can still be flashed it never become an unusable brick.
We can flash the wrong Firmware, or having issue in the flashing process. This
way we could reflash the controller firmware several times. 

> > +static struct pd692x0_msg_ver pd692x0_get_sw_version(struct pd692x0_priv
> > *priv) +{
> > +	struct pd692x0_msg msg =
> > pd692x0_msg_template_list[PD692X0_MSG_GET_SW_VER];
> > +	struct device *dev = &priv->client->dev;
> > +	struct pd692x0_msg_content buf = {0};
> > +	struct pd692x0_msg_ver ver = {0};
> > +	int ret;
> > +
> > +	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to get PSE version (%pe)\n",
> > ERR_PTR(ret));
> > +		return ver;  
> 
> I _think_ that return is wrong ???

No, this return will return an empty struct pd692x0_msg_ver.
Which means the firmware has not any version.

> Is the firmware in Motorola SREC format? I thought the kernel had a
> helper for that, but a quick search did not find it. So maybe i'm
> remembering wrongly. But it seems silly for every driver to implement
> an SREC parser.

Oh, I didn't know this format. The firmware seems indeed to match this format
specification.
I found two reference of this Firmware format in the kernel:
https://elixir.bootlin.com/linux/v6.5.7/source/sound/soc/codecs/zl38060.c#L178
https://elixir.bootlin.com/linux/v6.5.7/source/drivers/staging/wlan-ng/prism2fw.c

Any preference in the choice? The zl38060 fw usage is maybe the easiest.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Andrew Lunn]

> > Is the firmware in Motorola SREC format? I thought the kernel had a
> > helper for that, but a quick search did not find it. So maybe i'm
> > remembering wrongly. But it seems silly for every driver to implement
> > an SREC parser.
> 
> Oh, I didn't know this format.

Its often used in small deeply embedded systems. Microcontrollers,
rather than something which can run Linux.

> The firmware seems indeed to match this format
> specification.
> I found two reference of this Firmware format in the kernel:
> https://elixir.bootlin.com/linux/v6.5.7/source/sound/soc/codecs/zl38060.c#L178
> https://elixir.bootlin.com/linux/v6.5.7/source/drivers/staging/wlan-ng/prism2fw.c

Ah, all inside a header file. Probably why i missed it. But ihex is
not SREC. ihex came from Intel. SREC from Motorola.

So i would follow the basic flow in include/linux/ihex.h, add an
include/linux/srec.h but adapt it for SREC.

	Andrew

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Köry Maincent]

On Wed, 22 Nov 2023 18:11:25 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > > Is the firmware in Motorola SREC format? I thought the kernel had a
> > > helper for that, but a quick search did not find it. So maybe i'm
> > > remembering wrongly. But it seems silly for every driver to implement
> > > an SREC parser.  
> > 
> > Oh, I didn't know this format.  
> 
> Its often used in small deeply embedded systems. Microcontrollers,
> rather than something which can run Linux.
> 
> > The firmware seems indeed to match this format
> > specification.
> > I found two reference of this Firmware format in the kernel:
> > https://elixir.bootlin.com/linux/v6.5.7/source/sound/soc/codecs/zl38060.c#L178
> > https://elixir.bootlin.com/linux/v6.5.7/source/drivers/staging/wlan-ng/prism2fw.c
> >  
> 
> Ah, all inside a header file. Probably why i missed it. But ihex is
> not SREC. ihex came from Intel. SREC from Motorola.
> 
> So i would follow the basic flow in include/linux/ihex.h, add an
> include/linux/srec.h but adapt it for SREC.

In fact the ihex.h header is only adding the ihex_validate_fw and the
request_ihex_firmware functions. In my case I do not use request firmware but
sysfs firmware loader. I could add srec_validate_fw but I am already
checking the firmware during the flashing process due to its special flashing
process.
I could not treat the firmware to one blob to be send. Each byte need to be
send in one i2c messages and at the end of eaxch line we need to wait a "\r\n"
(within 30ms) before sending next line. Yes, it takes time to be flashed!

Do you see generic helper that I could add? 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller driver

[Simon Horman]

On Thu, Nov 16, 2023 at 03:01:41PM +0100, Kory Maincent wrote:
> Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller.
> This driver only support i2c communication for now.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Hi Kory,

some minor feedback from my side.

...

> diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c

...

> +static int
> +pd692x0_get_of_matrix(struct device *dev,
> +		      struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS])
> +{
> +	int ret, i, ports_matrix_size;
> +	u32 val[PD692X0_MAX_LOGICAL_PORTS * 3];

nit: reverse xmas tree please.

...

> +static int pd692x0_fw_write_line(const struct i2c_client *client,
> +				 const char line[PD692X0_FW_LINE_MAX_SZ],
> +				 const bool last_line)
> +{
> +	int ret;
> +
> +	while (*line != 0) {
> +		ret = i2c_master_send(client, line, 1);
> +		if (ret < 0)
> +			return FW_UPLOAD_ERR_RW_ERROR;
> +		line++;
> +	}
> +
> +	if (last_line) {
> +		ret = pd692x0_fw_recv_resp(client, 100, "TP\r\n",
> +					   sizeof("TP\r\n") - 1);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = pd692x0_fw_recv_resp(client, 100, "T*\r\n",
> +					   sizeof("T*\r\n") - 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return FW_UPLOAD_ERR_NONE;
> +}

...

> +static enum fw_upload_err pd692x0_fw_write(struct fw_upload *fwl,
> +					   const u8 *data, u32 offset,
> +					   u32 size, u32 *written)
> +{
> +	struct pd692x0_priv *priv = fwl->dd_handle;
> +	char line[PD692X0_FW_LINE_MAX_SZ];
> +	const struct i2c_client *client;
> +	int ret, i;
> +	char cmd;
> +
> +	client = priv->client;
> +	priv->fw_state = PD692X0_FW_WRITE;
> +
> +	/* Erase */
> +	cmd = 'E';
> +	ret = i2c_master_send(client, &cmd, 1);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to boot programming mode (%pe)\n",
> +			ERR_PTR(ret));
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +	}
> +
> +	ret = pd692x0_fw_recv_resp(client, 100, "TOE\r\n", sizeof("TOE\r\n") - 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = pd692x0_fw_recv_resp(client, 5000, "TE\r\n", sizeof("TE\r\n") - 1);
> +	if (ret)
> +		dev_warn(&client->dev,
> +			 "Failed to erase internal memory, however still try to write Firmware\n");
> +
> +	ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1);
> +	if (ret)
> +		dev_warn(&client->dev,
> +			 "Failed to erase internal memory, however still try to write Firmware\n");
> +
> +	if (priv->cancel_request)
> +		return FW_UPLOAD_ERR_CANCELED;
> +
> +	/* Program */
> +	cmd = 'P';
> +	ret = i2c_master_send(client, &cmd, sizeof(char));
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to boot programming mode (%pe)\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = pd692x0_fw_recv_resp(client, 100, "TOP\r\n", sizeof("TOP\r\n") - 1);
> +	if (ret)
> +		return ret;
> +
> +	i = 0;
> +	while (i < size) {
> +		ret = pd692x0_fw_get_next_line(data, line, size - i);
> +		if (!ret) {
> +			ret = FW_UPLOAD_ERR_FW_INVALID;
> +			goto err;
> +		}
> +
> +		i += ret;
> +		data += ret;
> +		if (line[0] == 'S' && line[1] == '0') {
> +			continue;
> +		} else if (line[0] == 'S' && line[1] == '7') {
> +			ret = pd692x0_fw_write_line(client, line, true);
> +			if (ret)
> +				goto err;
> +		} else {
> +			ret = pd692x0_fw_write_line(client, line, false);
> +			if (ret)
> +				goto err;
> +		}
> +
> +		if (priv->cancel_request) {
> +			ret = FW_UPLOAD_ERR_CANCELED;
> +			goto err;
> +		}
> +	}
> +	*written = i;
> +
> +	msleep(400);
> +
> +	return FW_UPLOAD_ERR_NONE;
> +
> +err:
> +	pd692x0_fw_write_line(client, "S7\r\n", true);

gcc-13 (W=1) seems a bit upset about this.

 drivers/net/pse-pd/pd692x0.c: In function 'pd692x0_fw_write':
 drivers/net/pse-pd/pd692x0.c:861:9: warning: 'pd692x0_fw_write_line' reading 128 bytes from a region of size 5 [-Wstringop-overread]
   861 |         pd692x0_fw_write_line(client, "S7\r\n", true);
       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 drivers/net/pse-pd/pd692x0.c:861:9: note: referencing argument 2 of type 'const char[128]'
 drivers/net/pse-pd/pd692x0.c:642:12: note: in a call to function 'pd692x0_fw_write_line'
   642 | static int pd692x0_fw_write_line(const struct i2c_client *client,
       |            ^~~~~~~~~~~~~~~~~~~~~

> +	return ret;
> +}

...

Re: [PATCH net-next 0/9] net: Add support for Power over Ethernet (PoE)

[Jakub Kicinski]

On Thu, 16 Nov 2023 15:01:32 +0100 Kory Maincent wrote:
> This patch series aims at adding support for PoE (Power over Ethernet),
> based on the already existing support for PoDL (Power over Data Line)
> implementation. In addition, it adds support for one specific PoE
> controller, the Microchip PD692x0.
> 
> In detail:
> - Patch 1 to 6 prepare net to support PoE devices.
> - Patch 7 adds a new error code to firmware upload API.
> - Patch 8 and 9 add PD692x0 PoE PSE controller driver and its binding.

You haven't CCed Oleksij or am I blind?

Re: [PATCH net-next 0/9] net: Add support for Power over Ethernet (PoE)

[Köry Maincent]

On Sat, 18 Nov 2023 15:59:37 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 16 Nov 2023 15:01:32 +0100 Kory Maincent wrote:
> > This patch series aims at adding support for PoE (Power over Ethernet),
> > based on the already existing support for PoDL (Power over Data Line)
> > implementation. In addition, it adds support for one specific PoE
> > controller, the Microchip PD692x0.
> > 
> > In detail:
> > - Patch 1 to 6 prepare net to support PoE devices.
> > - Patch 7 adds a new error code to firmware upload API.
> > - Patch 8 and 9 add PD692x0 PoE PSE controller driver and its binding.  
> 
> You haven't CCed Oleksij or am I blind?


Oh right, I forgot he was not described as maintainer for pse-pd drivers
subsystem.

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com