[PATCH net-next v11 0/4] net: pse-pd: add Realtek PSE MCU support

[PATCH net-next v11 0/4] net: pse-pd: add Realtek PSE MCU support

[Jonas Jelonek]

This series adds a PSE-PD driver for the microcontroller (MCU) that
fronts the PSE silicon on a range of managed switches, together with its
DT binding.

Hardware model
==============

These boards do not expose the PSE chips to the host directly. A small
microcontroller sits on an I2C/SMBus or UART bus and manages one or more
PSE chips behind it; the host CPU only ever talks to that MCU, using a
fixed 12-byte request/response protocol with a trailing checksum. The
PSE silicon never appears on the bus.

Two generations of the protocol exist, both Realtek's: an older one on
boards with Broadcom PSE silicon (BCM59111, BCM59121) and a newer one
used with Realtek's own PSE silicon (RTL8238B, RTL8239, RTL8239C). They
diverge in opcode numbering and a few response layouts; the driver
abstracts that behind a per-dialect opcode table and parser hooks,
selected by the compatible. The specific PSE chip behind the MCU is
detected at runtime and only influences per-chip constants (power scaling
and the per-port cap).

The compatibles
===============

The protocol compatibles name two generations of the Realtek protocol,
with the I2C framing folded in:

  realtek,pse-mcu-gen1        gen1, UART
  realtek,pse-mcu-gen1-smbus  gen1, I2C/SMBus
  realtek,pse-mcu-gen2        gen2, UART
  realtek,pse-mcu-gen2-smbus  gen2, I2C/SMBus
  realtek,pse-mcu-gen2-i2c    gen2, raw I2C

and each board carries a device-specific compatible that falls back to one
of these, e.g.

  compatible = "zyxel,xs1930-12hp-pse", "realtek,pse-mcu-gen2-smbus";

The naming is the part most likely to raise questions, so the reasoning up
front (the binding documents it too):

  - The node describes the MCU together with its Realtek firmware, not a
    PSE chip and not the microcontroller silicon. The PSE chips sit behind
    the MCU, never appear on the bus, and are reported by the MCU and
    detected at runtime; the microcontroller itself is a general-purpose
    part (GigaDevice, Nuvoton, ...) that varies across boards. What is
    fixed and Realtek's is the firmware and its host protocol - hence the
    'realtek' prefix.

  - gen1 and gen2 are two generations of that protocol, both Realtek's:
    gen1 on older boards fronting Broadcom PSE silicon, gen2 the altered
    protocol used once Realtek shipped their own PSE silicon. The
    generation is fixed per board and is all the driver needs at DT-parse
    time, so the compatible encodes it.

  - On I2C the MCU firmware expects one of two framings - SMBus or raw
    I2C - which is a genuine programming-model difference, so it is part
    of the compatible ('-smbus' / '-i2c'). A UART attachment carries no
    framing suffix; the transport is given structurally by the parent
    'serial' node.

  - Each board additionally carries a device-specific compatible that
    falls back to the protocol one. The driver only ever binds on the
    protocol compatible; the device-specific string keeps the binding
    specific and reserves a place for a future per-board quirk without
    having to retrofit device trees already deployed in the field.

Testing
=======

 - Linksys LGS328MPCv2     (RTL8238B, I2C)
 - Zyxel GS1900-10HP A1    (BCM59121, UART)
 - Zyxel GS1900-10HP B1    (RTL8238B, UART)
 - Zyxel GS1920-24HPv2     (BCM59121, SMBus)
 - Zyxel XMG1915-10EP      (RTL8239C, UART)
 - Zyxel XS1930-12HP       (RTL8239, SMBus)

---
v10 -> v11:
 - patch 1: added Reviewed-by from Oleksij
 - patch 2: added Acked-by from Oleksij
 - rebase due to conflict in MAINTAINERS
v10: https://lore.kernel.org/netdev/20260728084307.2129515-1-jelonek.jonas@gmail.com/

v9 -> v10:
 - patch 2: fixed ordering in MAINTAINERS
 - core: dropped unused fields, can be re-added later when actually
   needed (Sashiko)
 - core: harden frame desync for I2C by including seq_num in _is_final
   helper (Sashiko)
 - core: return constant voltage when MCU reports zero, in case the
   port is not delivering. This avoids "Voltage null" errors in case a
   power limit is set while port is not delivering (Sashiko)
 - core: set_pw_limit: round up to avoid setting 0 in case the requested
   limit is below 1 LSB (Sashiko)
 - core: de-assert global port disable after MCU was discovered
   successfully (Sashiko)
 - core: fix and make more precise several comments (Sashiko)
v9: https://lore.kernel.org/all/20260726112223.1286074-1-jelonek.jonas@gmail.com/
Sashiko review: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260726112223.1286074-1-jelonek.jonas%40gmail.com

v8 -> v9:
 - dt-bindings: renamed enable-gpios -> reset-gpios, verified on boards
   that the GPIO which is used with this is connected to the MCU's
   nRESET.
 - dt-bindings: added disable-ports-gpios for GPIO that gates all ports
   (previously incorrectly assumed to be a rail gate, thus being wired
   via 'power-supply')
 - dt-bindings: dropped 'power-supply', not used anymore and confusing,
   easy to confuse with PI's vpwr-supply (Sander)
 - core: adjusted to match bindings, handling reset/disable-ports-gpio
   correctly and dropping 'power-supply' regulator stuff
 - patch 1,2: dropped Reviewed-by/Acked-by tags due to changes.
 - patch 3,4: added Reviewed-by from Kory
v8: https://lore.kernel.org/all/20260715075530.2491534-1-jelonek.jonas@gmail.com/

v7 -> v8:
 - dt-bindings: added Reviewed-by of Oleksij
 - uart: check and warn-on-error for serdev_* operations (Oleksij)
 - core, i2c, uart: added Acked-by of Oleksij
v7: https://lore.kernel.org/netdev/20260712192251.1413279-1-jelonek.jonas@gmail.com/

v6 -> v7:
 - dt-bindings: rename file to 'realtek,pse-mcu-gen1.yml', using
   specific compatible as file name (Conor)
 - dt-bindings: added Conor's Reviewed-by
 - core: replaced some leftover 'BCM/RTK' dialect framing in comments
 - core: use rolling sequence number instead of always hardcoded 0xff,
   adressing the issue that stale data might be accepted for new
   requests (Sashiko)
 - core: reordered MODULE_ calls to keep consistent across all driver
   parts
 - i2c: replaced some leftover 'BCM/RTK' dialect framing in comments
 - slight commit message adjustments
v6: https://lore.kernel.org/netdev/20260709194125.2784507-1-jelonek.jonas@gmail.com/

v5 -> v6:
 - dt-bindings: reworked the compatibles per DT-maintainer feedback
    - name the two protocol generations -gen1 / -gen2 (both Realtek's)
      instead of the -rtk / -brcm dialect suffix (Conor)
    - encode the I2C framing in the compatible (-smbus / raw -i2c) and
      drop the realtek,i2c-protocol property (Rob)
    - add device-specific (switch) compatibles that fall back to the
      protocol compatibles, with the board↔protocol pairing enforced in
      the schema (Conor)
    - rewrite the description accordingly
 - driver: track the binding rework
    - match on realtek,pse-mcu-gen{1,2}[-smbus|-i2c]; the I2C transport
      selects SMBus-vs-raw framing from a native_i2c match-data flag
      instead of reading the property (drops rtpse_mcu_needs_i2c_proto)
    - rename the internal dialect and parser symbols rtk/brcm → gen1/gen2
      (chip identifiers like RTL8238B/BCM59121 kept)
 - i2c: DMA-safe raw-I2C path — bounce each frame through a heap buffer,
   since i2c_master_send()/i2c_master_recv() may DMA and the core's
   frame buffers are on the stack (SMBus and UART paths unaffected)
   (sashiko-nipa)
 - includes: drop unused linux/mod_devicetable.h (core) (Uwe)
 - includes: drop unused linux/delay.h (uart); add
   linux/regulator/consumer.h (core) and linux/slab.h + linux/string.h
   (i2c)
 - commit messages — update the binding, core, and I2C messages to
   match (generations, framing-in-compatible, DMA note)
v5: https://lore.kernel.org/netdev/20260706112425.3149226-1-jelonek.jonas@gmail.com/

v4 -> v5:
 - split the single driver patch into three — core / I2C transport / UART
   transport. Binding stays patch 1, unchanged in shape. (Paolo)
   Please give guidance on how to if I should split more.
 - core: set_pw_limit: guard divide-by-zero on pw_set_lsb_mW; cap the
   programmed value with U8_MAX instead of a bare 0xff; prg_val is now u8.
   (Oleksij, Sashiko)
 - core: discover: also retry transient boot-time frames (-EBADMSG / -EBADE)
   within the bounded window, not just silence/NAK/not-ready (Sashiko).
 - core: pw_status: report Broadcom 0x3 → TEST and 0x5 → OTHERFAULT
   (new STS_TEST/STS_OTHER_FAULT); pw_class comment corrected (0x3/0x5
   aren't "other fault" on RTL; class-0-vs-fault note). (Sashiko)
 - core: dropped unused decoded fields — function_mode, cls_type,
   disconnect_type, pair_type, inrush_mode, limit_type, chip_addr,
   channel. (Oleksij)
 - core: removed forward declarations by moving the response structs above
   the dialect struct. (Oleksij)
 - core: get_pw_limit_ranges: reverse-Christmas-tree local ordering.
   (Oleksij)
 - core: dialect comment clarified (only divergent responses are hooked);
   commit message "parser hooks" tightened to "…for the responses that
   differ." (Sashiko)
 - core: made parse_system_info hook void, both implementations return
   hardcoded 0. (Paolo)
 - core: dropped GFP_KERNEL from kzalloc_obj. (Paolo)
 - core: dropped unneeded u32 cast
 - kept probe dev_info() for now deliberately, due to different opinions
   on whether a probe might print or not
 - NOT included Acked-by from Oleksij, due to several changes
v4: https://lore.kernel.org/netdev/20260630105651.756058-1-jelonek.jonas@gmail.com/

v3 -> v4:
 - move owner setting from core to transport, mitigating possible
   use-after-free (Sashiko)
 - resend because net-next was still closed
v3: https://lore.kernel.org/netdev/20260628222705.4052815-1-jelonek.jonas@gmail.com/

v2 -> v3:
 - dt-bindings: using brcm instead of bcm for Broadcom
 - rename the driver files and Kconfig symbols to realtek-pse-mcu-* /
   PSE_REALTEK_MCU* for consistency with the realtek,pse-mcu-* compatibles
 - rename driver-internal prefix from 'rtpse_' to 'rtpse_mcu' to
   emphasize this targets the MCU-centric setup (and leaves room open
   for eventual directly addressable PSE chips)
 - rework the vendor-prefix rationale (binding + commit message): the
   prefix names the protocol/firmware owner (Realtek documents the protocol
   and supplies the firmware), and -rtk/-brcm select the Realtek or Broadcom
   protocol dialect
 - core: reject zeroed/echo-mismatched responses via the echoed seq_num
   (a BCM PORT_ENABLE on port 0 was otherwise accepted from an all-zero
   frame)
 - core: enable the PoE supply before global-enabling the MCU, and roll
   back the global enable on probe failure or driver removal
 - core: drop inline from helpers (flagged by automated check)
 - uart: update the completion under rx_lock too, so a late frame can no
   longer make the next transaction fail spuriously with -EIO
v2: https://lore.kernel.org/netdev/20260612132944.460646-1-jelonek.jonas@gmail.com/

v1 -> v2:
 - all points flagged by Sashiko addressed:
 - uart: drop frame overflow (return count, not the stored length) so
   serdev retains no leftover bytes that would misalign the next response
 - uart: guard rx_buf/rx_len with a spinlock to close a data race between
   the async receive_buf callback and send/recv
 - i2c: return terminal MCU error opcodes (0xfd/0xfe) to the core
   immediately instead of polling to the 1 s timeout
 - core: cap BCM59121 at 30 W (802.3at) — the basic 8-bit set command
   can't program the advertised 60 W (it silently clamped to 51 W)
v1: https://lore.kernel.org/netdev/20260608205758.1830521-1-jelonek.jonas@gmail.com/

---
Jonas Jelonek (4):
  dt-bindings: net: pse-pd: add bindings for Realtek PSE MCU
  net: pse-pd: add Realtek PSE MCU core
  net: pse-pd: realtek-pse-mcu: add I2C transport
  net: pse-pd: realtek-pse-mcu: add UART transport

 .../net/pse-pd/realtek,pse-mcu-gen1.yaml      | 182 ++++
 MAINTAINERS                                   |   7 +
 drivers/net/pse-pd/Kconfig                    |  28 +
 drivers/net/pse-pd/Makefile                   |   3 +
 drivers/net/pse-pd/realtek-pse-mcu-core.c     | 988 ++++++++++++++++++
 drivers/net/pse-pd/realtek-pse-mcu-i2c.c      | 170 +++
 drivers/net/pse-pd/realtek-pse-mcu-uart.c     | 164 +++
 drivers/net/pse-pd/realtek-pse-mcu.h          |  93 ++
 8 files changed, 1635 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
 create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-core.c
 create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-i2c.c
 create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-uart.c
 create mode 100644 drivers/net/pse-pd/realtek-pse-mcu.h


base-commit: 69963a0678a347d57c4ac8b16939dba216eb95ce
-- 
2.53.0

[PATCH net-next v11 1/4] dt-bindings: net: pse-pd: add bindings for Realtek PSE MCU

[Jonas Jelonek]

Add a binding for the microcontroller (MCU) that fronts the PSE silicon
on a range of managed Realtek-based switches. The host talks only to the
MCU, over I2C/SMBus or UART, using a fixed message-based protocol; the
PSE chips behind it never appear on the bus.

The device is the MCU together with its Realtek firmware: the firmware
and its host protocol are what the binding describes, not the
general-purpose microcontroller they run on. The PSE silicon behind the
MCU (Realtek or Broadcom) is reported by the MCU and detected at runtime,
so it is not described here - hence the 'realtek' vendor prefix.

Two protocol generations exist, both Realtek's, selected by the
compatible: gen1 on older boards (fronting Broadcom PSE silicon) and gen2,
the altered protocol used with Realtek's own PSE silicon. On an I2C
attachment the framing the MCU firmware expects is part of the compatible
as well - '-smbus' or raw '-i2c'; a UART attachment carries no framing
suffix, as the transport is given by the parent serial node.

Each board additionally carries a device-specific compatible that falls
back to one of the protocol compatibles above. Drivers bind on the
protocol compatible; the device-specific string identifies the board and
reserves a place for a future per-board quirk without having to retrofit
device trees already in the field.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../net/pse-pd/realtek,pse-mcu-gen1.yaml      | 182 ++++++++++++++++++
 1 file changed, 182 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml

diff --git a/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml b/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
new file mode 100644
index 000000000000..3bb32349c28c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
@@ -0,0 +1,182 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/pse-pd/realtek,pse-mcu-gen1.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek PSE MCU
+
+maintainers:
+  - Jonas Jelonek <jelonek.jonas@gmail.com>
+
+description: |
+  A microcontroller (MCU) that manages the PSE (Power Sourcing Equipment)
+  hardware on a range of managed PoE switches. The host CPU talks only to
+  this MCU - over I2C/SMBus or UART - using a small message-based protocol;
+  the PSE silicon it drives sits behind the MCU and is never accessed
+  directly. For example, on the Zyxel GS1900-10HP the SoC reaches the MCU
+  over UART, and the MCU manages the on-board PSE chip.
+
+  This binding describes the MCU together with its Realtek firmware: the
+  firmware and its host protocol, which are stable across boards. The
+  microcontroller silicon is a general-purpose part that varies, and the
+  PSE silicon behind the MCU (Realtek RTL823x/RTL8239* or Broadcom
+  BCM59xxx) is reported by the MCU and detected at runtime - neither is
+  named here.
+
+  Two protocol generations exist, both Realtek's:
+    gen1  older boards, where the MCU fronts Broadcom PSE silicon
+    gen2  the altered protocol used with Realtek's own PSE silicon
+
+  On an I2C attachment the framing the MCU firmware expects is part of the
+  compatible: '-smbus' (reads carry a leading command byte and a repeated
+  start) or '-i2c' (bare block writes and reads). A UART attachment carries
+  no framing suffix; the transport is given by the parent 'serial' node.
+
+  Each board additionally carries a device-specific compatible that falls
+  back to one of the protocol compatibles above. Drivers bind on the
+  protocol compatible; the device-specific string identifies the board and
+  reserves a place for a future per-board quirk without having to retrofit
+  device trees already in the field.
+
+properties:
+  compatible:
+    oneOf:
+      # UART
+      - items:
+          - enum:
+              - zyxel,gs1900-10hp-a1-pse
+          - const: realtek,pse-mcu-gen1
+
+      # I2C, SMBus framing
+      - items:
+          - enum:
+              - zyxel,gs1920-24hp-v2-pse
+          - const: realtek,pse-mcu-gen1-smbus
+
+      # UART
+      - items:
+          - enum:
+              - zyxel,gs1900-10hp-b1-pse
+              - zyxel,xmg1915-10ep-pse
+          - const: realtek,pse-mcu-gen2
+
+      # I2C, SMBus framing
+      - items:
+          - enum:
+              - zyxel,xs1930-12hp-pse
+          - const: realtek,pse-mcu-gen2-smbus
+
+      # I2C, raw framing
+      - items:
+          - enum:
+              - linksys,lgs328mpc-v2-pse
+          - const: realtek,pse-mcu-gen2-i2c
+
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    description: Reset line of the MCU.
+    maxItems: 1
+
+  disable-ports-gpios:
+    description:
+      Hardware gate that forces all ports into admin-disabled state while
+      asserted.
+    maxItems: 1
+
+required:
+  - compatible
+
+allOf:
+  - $ref: pse-controller.yaml#
+  # A '-smbus'/'-i2c' compatible is an I2C attachment: it has 'reg' and
+  # cannot carry serial bus properties. A bare gen compatible is a UART
+  # attachment: no 'reg', the transport comes from the parent serial node.
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - realtek,pse-mcu-gen1-smbus
+              - realtek,pse-mcu-gen2-smbus
+              - realtek,pse-mcu-gen2-i2c
+    then:
+      required:
+        - reg
+      properties:
+        current-speed: false
+        max-speed: false
+    else:
+      allOf:
+        - $ref: /schemas/serial/serial-peripheral-props.yaml#
+
+      properties:
+        reg: false
+
+unevaluatedProperties: false
+
+examples:
+  # SMBus-framed I2C attachment
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-pse@20 {
+            compatible = "zyxel,xs1930-12hp-pse", "realtek,pse-mcu-gen2-smbus";
+            reg = <0x20>;
+
+            pse-pis {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                pse-pi@0 {
+                    reg = <0>;
+                    #pse-cells = <0>;
+                };
+            };
+        };
+    };
+
+  # Raw-I2C-framed attachment
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet-pse@20 {
+            compatible = "linksys,lgs328mpc-v2-pse", "realtek,pse-mcu-gen2-i2c";
+            reg = <0x20>;
+
+            pse-pis {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                pse-pi@0 {
+                    reg = <0>;
+                    #pse-cells = <0>;
+                };
+            };
+        };
+    };
+
+  # UART attachment
+  - |
+    serial {
+        ethernet-pse {
+            compatible = "zyxel,gs1900-10hp-a1-pse", "realtek,pse-mcu-gen1";
+            current-speed = <19200>;
+
+            pse-pis {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                pse-pi@0 {
+                    reg = <0>;
+                    #pse-cells = <0>;
+                };
+            };
+        };
+    };
-- 
2.53.0

[PATCH net-next v11 2/4] net: pse-pd: add Realtek PSE MCU core

[Jonas Jelonek]

A range of managed Realtek-based PoE switches use a small microcontroller
on the PCB to front the actual PSE silicon. The host CPU talks to that
MCU over I2C/SMBus or UART using a fixed 12-byte request/response
protocol with a trailing checksum; the PSE chips are managed by the MCU
and are not accessed directly. Two generations of the protocol exist -
both Realtek's - diverging in opcode numbering and a few response
layouts; the driver handles this with a per-dialect opcode table and
parser hooks for the responses that differ, selected by the compatible.
The specific PSE chip behind the MCU is detected at runtime and only
influences per-chip constants (power scaling and the per-port cap).

This core module implements the protocol, message framing, the dialect
machinery and the pse_controller_ops glue, and exports a registration
helper for transport modules. The I2C and UART transports that drive it
follow in the next patches; the core (PSE_REALTEK_MCU) is selected
automatically by those transports and is not user-selectable on its own.

The realtek-pse-mcu-* files and PSE_REALTEK_MCU* symbols match the
realtek,pse-mcu-* compatibles (see the binding for the naming rationale).
The two protocol generations - gen1 on older Broadcom-PSE boards, gen2 on
Realtek's own PSE silicon - are both Realtek's, handled by the same shared
core, each selecting its dialect via the compatible.

Power budgeting is left to the MCU firmware; the driver advertises
PSE_BUDGET_EVAL_STRAT_DYNAMIC accordingly.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 MAINTAINERS                               |   7 +
 drivers/net/pse-pd/Kconfig                |   6 +
 drivers/net/pse-pd/Makefile               |   1 +
 drivers/net/pse-pd/realtek-pse-mcu-core.c | 988 ++++++++++++++++++++++
 drivers/net/pse-pd/realtek-pse-mcu.h      |  93 ++
 5 files changed, 1095 insertions(+)
 create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-core.c
 create mode 100644 drivers/net/pse-pd/realtek-pse-mcu.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 932ea1db048e..fc7325dd51aa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22767,6 +22767,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
 F:	drivers/watchdog/realtek_otto_wdt.c
 
+REALTEK PSE MCU DRIVER
+M:	Jonas Jelonek <jelonek.jonas@gmail.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
+F:	drivers/net/pse-pd/realtek-pse-mcu*
+
 REALTEK RTL83xx SMI DSA ROUTER CHIPS
 M:	Linus Walleij <linusw@kernel.org>
 M:	Luiz Angelo Daros de Luca <luizluca@gmail.com>
diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
index 7ef29657ee5d..3b0c245a2bc7 100644
--- a/drivers/net/pse-pd/Kconfig
+++ b/drivers/net/pse-pd/Kconfig
@@ -13,6 +13,12 @@ menuconfig PSE_CONTROLLER
 
 if PSE_CONTROLLER
 
+config PSE_REALTEK_MCU
+	tristate
+	help
+	  Shared core for the Realtek PSE MCU driver. This is selected
+	  automatically by the transport options below.
+
 config PSE_REGULATOR
 	tristate "Regulator based PSE controller"
 	help
diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
index cc78f7ea7f5f..bf35e2a5b110 100644
--- a/drivers/net/pse-pd/Makefile
+++ b/drivers/net/pse-pd/Makefile
@@ -3,6 +3,7 @@
 
 obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
 
+obj-$(CONFIG_PSE_REALTEK_MCU) += realtek-pse-mcu-core.o
 obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
 obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
 obj-$(CONFIG_PSE_SI3474) += si3474.o
diff --git a/drivers/net/pse-pd/realtek-pse-mcu-core.c b/drivers/net/pse-pd/realtek-pse-mcu-core.c
new file mode 100644
index 000000000000..ed3b743470db
--- /dev/null
+++ b/drivers/net/pse-pd/realtek-pse-mcu-core.c
@@ -0,0 +1,988 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for the microcontroller (MCU) fronting PSE silicon on various
+ * Realtek-based managed switches. The MCU speaks a 12-byte fixed-frame
+ * management protocol; this driver covers two generations of the
+ * protocol via a per-dialect opcode table and response parsers.
+ *
+ * Many PoE switch designs put a dedicated microcontroller in front of the
+ * actual PSE silicon: the host CPU talks to the MCU over I2C/SMBus or
+ * UART, and the MCU in turn manages the PSE chips on the board. The MCU
+ * speaks a small message-based protocol. The PSE chips themselves are not
+ * accessed directly; everything goes through MCU commands.
+ *
+ * This driver targets that architecture for the Realtek-family protocol.
+ * Two generations are supported: Gen1 being used on older switches where
+ * the MCU fronts and manages Broadcom PSE silicon; Gen2 being used with
+ * Realtek PSE silicon. The two share frame format and a sum-mod-256
+ * checksum but diverge on opcode numbers and on a few response layouts;
+ * this is handled by the per-dialect opcode table and parser hooks.
+ *
+ * Out of scope: PSE chips that are interfaced directly from the host
+ * without a management MCU, MCU designs that speak an unrelated protocol
+ * family, and "dumb PSE" modes where no host control is wired up at all.
+ *
+ * This core module implements the protocol, decoding/encoding of MCU
+ * responses, and the pse_controller_ops integration. Transport modules
+ * (realtek-pse-mcu-i2c, realtek-pse-mcu-uart) provide the send/recv
+ * callbacks.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/jiffies.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/pse-pd/pse.h>
+#include <linux/unaligned.h>
+
+#include "realtek-pse-mcu.h"
+
+#define RTPSE_MCU_DEVICE_ID_RTL8238B		0x0138
+#define RTPSE_MCU_DEVICE_ID_RTL8239		0x0039
+#define RTPSE_MCU_DEVICE_ID_RTL8239C		0x0139
+#define RTPSE_MCU_DEVICE_ID_BCM59111		0xe111
+#define RTPSE_MCU_DEVICE_ID_BCM59121		0xe121
+
+#define RTPSE_MCU_PORT_STS_DISABLED		0x00
+#define RTPSE_MCU_PORT_STS_SEARCHING		0x01
+#define RTPSE_MCU_PORT_STS_DELIVERING		0x02
+#define RTPSE_MCU_PORT_STS_TEST			0x03	/* Gen1-only; reserved on Gen2 */
+#define RTPSE_MCU_PORT_STS_FAULT		0x04
+#define RTPSE_MCU_PORT_STS_OTHER_FAULT		0x05	/* Gen1-only; reserved on Gen2 */
+#define RTPSE_MCU_PORT_STS_REQUESTING		0x06
+
+/* RTPSE_MCU_PORT_SET_POWER_LIMIT_TYPE values */
+#define RTPSE_MCU_PORT_PW_LIMIT_TYPE_USER	0x02
+
+#define RTPSE_MCU_MAX_PORTS			48
+#define RTPSE_MCU_PORT_MAX_PRIORITY		3
+
+/* Nominal PSE rail; 802.3at/bt operating range. */
+#define RTPSE_MCU_PSE_VOLTAGE_UV		54000000
+
+enum rtpse_mcu_cmd {
+	RTPSE_MCU_CMD_SET_GLOBAL_STATE,
+	RTPSE_MCU_CMD_GET_SYSTEM_INFO,
+	RTPSE_MCU_CMD_GET_EXT_CONFIG,
+
+	RTPSE_MCU_CMD_PORT_ENABLE,
+	RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_TYPE,
+	RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT,
+	RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_EXT,
+	RTPSE_MCU_CMD_PORT_SET_PRIORITY,
+	RTPSE_MCU_CMD_PORT_GET_STATUS,
+	RTPSE_MCU_CMD_PORT_GET_POWER_STATS,
+	RTPSE_MCU_CMD_PORT_GET_CONFIG,
+	RTPSE_MCU_CMD_PORT_GET_EXT_CONFIG,
+
+	RTPSE_MCU_NUM_CMDS,
+};
+
+struct rtpse_mcu_opcode {
+	u8 op;
+	bool valid;
+};
+
+/* Shorthand for the designated-initializer entries in dialect opcode tables. */
+#define RTPSE_MCU_OP(opc)	{ .op = (opc), .valid = true }
+
+/* Parsed MCU response structures (decoded from rtpse_mcu_msg replies) */
+
+struct rtpse_mcu_info {
+	u8 max_ports;
+	bool system_enable;
+	u16 device_id;
+	u8 mcu_type;
+};
+
+struct rtpse_mcu_ext_config {
+	u8 num_of_pses;
+};
+
+struct rtpse_mcu_port_status {
+	u8 sts1;
+	u8 sts2;
+	u8 sts3;
+};
+
+struct rtpse_mcu_port_measurement {
+	u16 voltage_raw;	/* 64.45mV/LSB */
+	u16 current_raw;	/* 1mA/LSB */
+	u16 temperature_raw;	/* T(mC) = 1250 * (220 - raw) */
+	u16 power_raw;		/* 100mW/LSB */
+};
+
+struct rtpse_mcu_port_config {
+	bool enable;
+};
+
+struct rtpse_mcu_port_ext_config {
+	u8 max_power;
+	u8 priority;
+};
+
+struct rtpse_mcu_dialect {
+	struct rtpse_mcu_opcode opcode[RTPSE_MCU_NUM_CMDS];
+
+	/*
+	 * Response parsers for the fields that differ between dialects; each
+	 * dialect supplies its own. Other responses share one layout and are
+	 * decoded directly - a dialect that diverges there must add a hook,
+	 * as a mismatched layout cannot be detected (the checksum still passes).
+	 */
+	void (*parse_system_info)(const u8 *payload, struct rtpse_mcu_info *info);
+	int (*parse_port_class)(const struct rtpse_mcu_port_status *status);
+	const char *(*mcu_type_str)(unsigned int mcu_type);
+};
+
+struct rtpse_mcu_chip_info {
+	const char *name;
+	u32 max_mW_per_port;
+	enum rtpse_mcu_cmd pw_set_cmd;	/* command used by set_pw_limit */
+	u32 pw_set_lsb_mW;		/* LSB of pw_set_cmd value, in mW */
+	u32 pw_read_lsb_mW;		/* LSB of ext_config.max_power read-back, in mW */
+};
+
+static const struct rtpse_mcu_chip_info rtl8238b_info = {
+	.max_mW_per_port = 30000,
+	.name = "RTL8238B",
+	.pw_read_lsb_mW = 200,
+	.pw_set_cmd = RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT,
+	.pw_set_lsb_mW = 200,
+};
+
+static const struct rtpse_mcu_chip_info rtl8239_info = {
+	.max_mW_per_port = 90000,
+	.name = "RTL8239",
+	.pw_read_lsb_mW = 400,
+	.pw_set_cmd = RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_EXT,
+	.pw_set_lsb_mW = 400,
+};
+
+static const struct rtpse_mcu_chip_info rtl8239c_info = {
+	.max_mW_per_port = 90000,
+	.name = "RTL8239C",
+	.pw_read_lsb_mW = 400,
+	.pw_set_cmd = RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_EXT,
+	.pw_set_lsb_mW = 400,
+};
+
+static const struct rtpse_mcu_chip_info bcm59111_info = {
+	.max_mW_per_port = 30000,
+	.name = "BCM59111",
+	.pw_read_lsb_mW = 200,
+	.pw_set_cmd = RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT,
+	.pw_set_lsb_mW = 200,
+};
+
+static const struct rtpse_mcu_chip_info bcm59121_info = {
+	/*
+	 * BCM59121 is a 60W Type-3 part, but known boards run it at 802.3at
+	 * and the Gen1 dialect has only the 8-bit/0.2W set command (<=51W);
+	 * cap at the 30W the hardware actually offers.
+	 */
+	.max_mW_per_port = 30000,
+	.name = "BCM59121",
+	.pw_read_lsb_mW = 200,
+	.pw_set_cmd = RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT,
+	.pw_set_lsb_mW = 200,
+};
+
+/* Helpers and basic functions */
+
+static struct rtpse_mcu_ctrl *to_rtpse_mcu_ctrl(struct pse_controller_dev *pcdev)
+{
+	return container_of(pcdev, struct rtpse_mcu_ctrl, pcdev);
+}
+
+static void rtpse_mcu_msg_init(struct rtpse_mcu_msg *msg, u8 opcode)
+{
+	memset(msg, 0xff, sizeof(*msg));
+	msg->opcode = opcode;
+}
+
+static u8 rtpse_mcu_checksum(const u8 *buf, size_t len)
+{
+	u8 sum = 0;
+
+	while (len--)
+		sum += *buf++;
+	return sum;
+}
+
+static int rtpse_mcu_do_xfer(struct rtpse_mcu_ctrl *pse, struct rtpse_mcu_msg *req,
+			     struct rtpse_mcu_msg *resp)
+{
+	int ret;
+
+	scoped_guard(mutex, &pse->mutex) {
+		/* Rolling seq_num so a late reply can't pass as a later one. */
+		req->seq_num = pse->seq++;
+		req->checksum = rtpse_mcu_checksum((u8 *)req, RTPSE_MCU_MSG_SIZE - 1);
+
+		ret = pse->transport->send(pse, req);
+		if (ret)
+			return ret;
+
+		/*
+		 * The MCU needs a fixed base time between accepting a request and
+		 * having a reply ready, independent of transport. Pace that here; the
+		 * transport then finishes recv in its own model - a bounded poll of
+		 * the response register (I2C) or one wait on a pushed frame (UART) -
+		 * so the retry belongs in the transport, not here.
+		 */
+		msleep(RTPSE_MCU_RESPONSE_MS);
+
+		memset(resp, 0, sizeof(*resp));
+		ret = pse->transport->recv(pse, req, resp);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Explicit MCU error opcodes (observed on the Gen1 dialect; harmless
+	 * to check for Gen2 too). Catch these before the generic opcode/CRC
+	 * mismatch path so callers see a meaningful errno.
+	 */
+	switch (resp->opcode) {
+	case RTPSE_MCU_OPCODE_INCOMPLETE:
+		return -EBADE;
+	case RTPSE_MCU_OPCODE_BAD_CSUM:
+		return -EBADMSG;
+	case RTPSE_MCU_OPCODE_NOT_READY:
+		return -EAGAIN;
+	}
+
+	if (resp->opcode != req->opcode ||
+	    resp->seq_num != req->seq_num ||
+	    resp->checksum != rtpse_mcu_checksum((u8 *)resp, RTPSE_MCU_MSG_SIZE - 1))
+		return -EBADMSG;
+
+	return 0;
+}
+
+static int rtpse_mcu_port_query(struct rtpse_mcu_ctrl *pse, unsigned int port, u8 opcode,
+				struct rtpse_mcu_msg *resp)
+{
+	struct rtpse_mcu_msg req;
+	int ret;
+
+	rtpse_mcu_msg_init(&req, opcode);
+	req.payload[0] = port;
+
+	ret = rtpse_mcu_do_xfer(pse, &req, resp);
+	if (ret)
+		return ret;
+
+	if (resp->payload[0] != port)
+		return -EIO;
+
+	return 0;
+}
+
+static int rtpse_mcu_port_cmd(struct rtpse_mcu_ctrl *pse, unsigned int port, u8 opcode, u8 arg)
+{
+	struct rtpse_mcu_msg req, resp;
+	int ret;
+
+	rtpse_mcu_msg_init(&req, opcode);
+	req.payload[0] = port;
+	req.payload[1] = arg;
+
+	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
+	if (ret)
+		return ret;
+
+	if (resp.payload[0] != port || resp.payload[1] != 0)
+		return -EIO;
+
+	return 0;
+}
+
+/* Global operations */
+
+static int rtpse_mcu_get_info(struct rtpse_mcu_ctrl *pse, struct rtpse_mcu_info *info)
+{
+	struct rtpse_mcu_msg req, resp;
+	const struct rtpse_mcu_opcode *opc;
+	int ret;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_GET_SYSTEM_INFO];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	rtpse_mcu_msg_init(&req, opc->op);
+	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
+	if (ret)
+		return ret;
+
+	pse->dialect->parse_system_info(resp.payload, info);
+	return 0;
+}
+
+static int rtpse_mcu_get_ext_config(struct rtpse_mcu_ctrl *pse, struct rtpse_mcu_ext_config *config)
+{
+	struct rtpse_mcu_msg req, resp;
+	const struct rtpse_mcu_opcode *opc;
+	int ret;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_GET_EXT_CONFIG];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	rtpse_mcu_msg_init(&req, opc->op);
+	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
+	if (ret)
+		return ret;
+
+	config->num_of_pses = resp.payload[6];
+
+	return 0;
+}
+
+static int rtpse_mcu_set_global_state(struct rtpse_mcu_ctrl *pse, bool enable)
+{
+	struct rtpse_mcu_msg req, resp;
+	const struct rtpse_mcu_opcode *opc;
+	int ret;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_SET_GLOBAL_STATE];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	rtpse_mcu_msg_init(&req, opc->op);
+	req.payload[0] = enable ? 0x1 : 0x0;
+
+	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
+	if (ret)
+		return ret;
+
+	return (resp.payload[0] == 0x0) ? 0 : -EIO;
+}
+
+/* Port operations */
+
+static int rtpse_mcu_port_get_status(struct rtpse_mcu_ctrl *pse, unsigned int port,
+				     struct rtpse_mcu_port_status *status)
+{
+	const struct rtpse_mcu_opcode *opc;
+	struct rtpse_mcu_msg resp;
+	int ret;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_PORT_GET_STATUS];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	ret = rtpse_mcu_port_query(pse, port, opc->op, &resp);
+	if (ret)
+		return ret;
+
+	status->sts1 = resp.payload[1];
+	status->sts2 = resp.payload[2];
+	status->sts3 = resp.payload[3];
+
+	return 0;
+}
+
+static int rtpse_mcu_port_get_measurement(struct rtpse_mcu_ctrl *pse, unsigned int port,
+					  struct rtpse_mcu_port_measurement *measurement)
+{
+	const struct rtpse_mcu_opcode *opc;
+	struct rtpse_mcu_msg resp;
+	int ret;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_PORT_GET_POWER_STATS];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	ret = rtpse_mcu_port_query(pse, port, opc->op, &resp);
+	if (ret)
+		return ret;
+
+	measurement->voltage_raw = get_unaligned_be16(&resp.payload[1]);
+	measurement->current_raw = get_unaligned_be16(&resp.payload[3]);
+	measurement->temperature_raw = get_unaligned_be16(&resp.payload[5]);
+	measurement->power_raw = get_unaligned_be16(&resp.payload[7]);
+
+	return 0;
+}
+
+static int rtpse_mcu_port_get_config(struct rtpse_mcu_ctrl *pse, unsigned int port,
+				     struct rtpse_mcu_port_config *config)
+{
+	const struct rtpse_mcu_opcode *opc;
+	struct rtpse_mcu_msg resp;
+	int ret;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_PORT_GET_CONFIG];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	ret = rtpse_mcu_port_query(pse, port, opc->op, &resp);
+	if (ret)
+		return ret;
+
+	config->enable = (resp.payload[1] == 1);
+
+	return 0;
+}
+
+static int rtpse_mcu_port_get_ext_config(struct rtpse_mcu_ctrl *pse, unsigned int port,
+					 struct rtpse_mcu_port_ext_config *config)
+{
+	const struct rtpse_mcu_opcode *opc;
+	struct rtpse_mcu_msg resp;
+	int ret;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_PORT_GET_EXT_CONFIG];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	ret = rtpse_mcu_port_query(pse, port, opc->op, &resp);
+	if (ret)
+		return ret;
+
+	config->max_power = resp.payload[3];
+	config->priority = resp.payload[4];
+
+	return 0;
+}
+
+static int rtpse_mcu_port_set_state(struct rtpse_mcu_ctrl *pse, unsigned int port, bool enable)
+{
+	const struct rtpse_mcu_opcode *opc;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_PORT_ENABLE];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	return rtpse_mcu_port_cmd(pse, port, opc->op, enable ? 0x1 : 0x0);
+}
+
+/* PSE controller ops */
+
+static int rtpse_mcu_port_get_admin_state(struct pse_controller_dev *pcdev, int id,
+					  struct pse_admin_state *admin_state)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	struct rtpse_mcu_port_config config;
+	int ret;
+
+	ret = rtpse_mcu_port_get_config(pse, id, &config);
+	if (ret)
+		return ret;
+
+	admin_state->c33_admin_state = config.enable ? ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED :
+						       ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED;
+	return 0;
+}
+
+static int rtpse_mcu_port_get_pw_status(struct pse_controller_dev *pcdev, int id,
+					struct pse_pw_status *pw_status)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	struct rtpse_mcu_port_status status;
+	int ret;
+
+	ret = rtpse_mcu_port_get_status(pse, id, &status);
+	if (ret)
+		return ret;
+
+	switch (status.sts1) {
+	case RTPSE_MCU_PORT_STS_DISABLED:
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_DISABLED;
+		break;
+	case RTPSE_MCU_PORT_STS_SEARCHING:
+	case RTPSE_MCU_PORT_STS_REQUESTING:
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_SEARCHING;
+		break;
+	case RTPSE_MCU_PORT_STS_DELIVERING:
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_DELIVERING;
+		break;
+	case RTPSE_MCU_PORT_STS_TEST:
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_TEST;
+		break;
+	case RTPSE_MCU_PORT_STS_FAULT:
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_FAULT;
+		break;
+	case RTPSE_MCU_PORT_STS_OTHER_FAULT:
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_OTHERFAULT;
+		break;
+	default:
+		pw_status->c33_pw_status = ETHTOOL_C33_PSE_PW_D_STATUS_UNKNOWN;
+		break;
+	}
+
+	return 0;
+}
+
+static int rtpse_mcu_port_get_pw_class(struct pse_controller_dev *pcdev, int id)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	struct rtpse_mcu_port_status status;
+	int ret;
+
+	ret = rtpse_mcu_port_get_status(pse, id, &status);
+	if (ret)
+		return ret;
+
+	/*
+	 * As per datasheet, the classification result is only valid when in
+	 * one of those operational modes, otherwise not.
+	 */
+	switch (status.sts1) {
+	case RTPSE_MCU_PORT_STS_DISABLED:
+	case RTPSE_MCU_PORT_STS_SEARCHING:
+	case RTPSE_MCU_PORT_STS_DELIVERING:
+	case RTPSE_MCU_PORT_STS_REQUESTING:
+		return pse->dialect->parse_port_class(&status);
+	default:
+		/*
+		 * No class to report, return 0 instead. This is indistinguishable
+		 * from a real class-0 PD but userspace disambiguates via the
+		 * power status.
+		 */
+		return 0;
+	}
+}
+
+static int rtpse_mcu_port_get_actual_pw(struct pse_controller_dev *pcdev, int id)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	struct rtpse_mcu_port_measurement measurement;
+	int ret;
+
+	ret = rtpse_mcu_port_get_measurement(pse, id, &measurement);
+	if (ret)
+		return ret;
+
+	/* 100mW per LSB */
+	return measurement.power_raw * 100U;
+}
+
+static int rtpse_mcu_port_get_voltage(struct pse_controller_dev *pcdev, int id)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	struct rtpse_mcu_port_measurement measurement;
+	int ret;
+	u32 uV;
+
+	ret = rtpse_mcu_port_get_measurement(pse, id, &measurement);
+	if (ret)
+		return ret;
+
+	/* 64.45mV per LSB */
+	uV = measurement.voltage_raw * 64450U;
+
+	/*
+	 * Idle ports measure 0V, which the core rejects when turning a power
+	 * limit into a current limit. Fall back to the nominal rail so a limit
+	 * can be set before a PD is attached.
+	 */
+	if (!uV)
+		return RTPSE_MCU_PSE_VOLTAGE_UV;
+
+	return min_t(u32, uV, INT_MAX);
+}
+
+static int rtpse_mcu_port_enable(struct pse_controller_dev *pcdev, int id)
+{
+	return rtpse_mcu_port_set_state(to_rtpse_mcu_ctrl(pcdev), id, true);
+}
+
+static int rtpse_mcu_port_disable(struct pse_controller_dev *pcdev, int id)
+{
+	return rtpse_mcu_port_set_state(to_rtpse_mcu_ctrl(pcdev), id, false);
+}
+
+static int rtpse_mcu_port_get_pw_limit(struct pse_controller_dev *pcdev, int id)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	struct rtpse_mcu_port_ext_config config;
+	int ret;
+
+	ret = rtpse_mcu_port_get_ext_config(pse, id, &config);
+	if (ret)
+		return ret;
+
+	return config.max_power * pse->chip->pw_read_lsb_mW;
+}
+
+static int rtpse_mcu_port_set_pw_limit(struct pse_controller_dev *pcdev, int id, int max_mW)
+{
+	const struct rtpse_mcu_opcode *type_opc, *val_opc;
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	const struct rtpse_mcu_chip_info *chip = pse->chip;
+	u8 prg_val;
+	int ret;
+
+	if (max_mW < 0 || max_mW > chip->max_mW_per_port)
+		return -ERANGE;
+
+	type_opc = &pse->dialect->opcode[RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_TYPE];
+	val_opc = &pse->dialect->opcode[chip->pw_set_cmd];
+	/* pw_set_lsb_mW is the divisor below; reject a chip that lacks it. */
+	if (!type_opc->valid || !val_opc->valid || !chip->pw_set_lsb_mW)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Switch the port to user-defined limit mode first, then program the
+	 * limit value. If the second cmd fails, the port is left in
+	 * user-defined mode but with the previous limit value; the next
+	 * successful set_pw_limit call recovers it.
+	 */
+	ret = rtpse_mcu_port_cmd(pse, id, type_opc->op, RTPSE_MCU_PORT_PW_LIMIT_TYPE_USER);
+	if (ret)
+		return ret;
+
+	/*
+	 * Round up so a sub-LSB request maps to one LSB, not silently to 0;
+	 * an explicit 0 still yields 0, and LSB-aligned maxima can't overshoot.
+	 */
+	prg_val = min_t(unsigned int, DIV_ROUND_UP(max_mW, chip->pw_set_lsb_mW), U8_MAX);
+
+	return rtpse_mcu_port_cmd(pse, id, val_opc->op, prg_val);
+}
+
+static int rtpse_mcu_port_get_pw_limit_ranges(struct pse_controller_dev *pcdev, int id,
+					      struct pse_pw_limit_ranges *out)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	struct ethtool_c33_pse_pw_limit_range *range;
+
+	range = kzalloc_obj(*range);
+	if (!range)
+		return -ENOMEM;
+
+	range[0].min = 0;
+	range[0].max = pse->chip->max_mW_per_port;
+
+	out->c33_pw_limit_ranges = range;
+	return 1;
+}
+
+static int rtpse_mcu_port_get_prio(struct pse_controller_dev *pcdev, int id)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	struct rtpse_mcu_port_ext_config config;
+	int ret;
+
+	ret = rtpse_mcu_port_get_ext_config(pse, id, &config);
+	if (ret)
+		return ret;
+
+	return config.priority;
+}
+
+static int rtpse_mcu_port_set_prio(struct pse_controller_dev *pcdev, int id, unsigned int prio)
+{
+	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
+	const struct rtpse_mcu_opcode *opc;
+
+	if (prio > RTPSE_MCU_PORT_MAX_PRIORITY)
+		return -ERANGE;
+
+	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_PORT_SET_PRIORITY];
+	if (!opc->valid)
+		return -EOPNOTSUPP;
+
+	return rtpse_mcu_port_cmd(pse, id, opc->op, prio);
+}
+
+static const struct pse_controller_ops rtpse_mcu_ops = {
+	.pi_get_admin_state = rtpse_mcu_port_get_admin_state,
+	.pi_get_pw_status = rtpse_mcu_port_get_pw_status,
+	.pi_get_pw_class = rtpse_mcu_port_get_pw_class,
+	.pi_get_actual_pw = rtpse_mcu_port_get_actual_pw,
+	.pi_enable = rtpse_mcu_port_enable,
+	.pi_disable = rtpse_mcu_port_disable,
+	.pi_get_voltage = rtpse_mcu_port_get_voltage,
+	.pi_get_pw_limit = rtpse_mcu_port_get_pw_limit,
+	.pi_set_pw_limit = rtpse_mcu_port_set_pw_limit,
+	.pi_get_pw_limit_ranges = rtpse_mcu_port_get_pw_limit_ranges,
+	.pi_get_prio = rtpse_mcu_port_get_prio,
+	.pi_set_prio = rtpse_mcu_port_set_prio,
+};
+
+static int rtpse_mcu_discover(struct rtpse_mcu_ctrl *pse, struct rtpse_mcu_info *info)
+{
+	struct rtpse_mcu_ext_config ext_config;
+	unsigned long deadline;
+	int ret;
+
+	/*
+	 * A booting MCU may stay silent (-ETIMEDOUT), not ACK its address
+	 * (-ENXIO / -EREMOTEIO), report not-ready (-EAGAIN), or emit a
+	 * corrupt/partial frame (-EBADMSG / -EBADE). Retry those within a
+	 * bounded window; other errors (e.g. -EOPNOTSUPP) are fatal and fail
+	 * immediately.
+	 */
+	deadline = jiffies + msecs_to_jiffies(RTPSE_MCU_BOOT_TIMEOUT_MS);
+	do {
+		ret = rtpse_mcu_get_info(pse, info);
+		if (ret != -ETIMEDOUT && ret != -ENXIO && ret != -EREMOTEIO &&
+		    ret != -EAGAIN && ret != -EBADMSG && ret != -EBADE)
+			break;
+		msleep(RTPSE_MCU_BOOT_RETRY_MS);
+	} while (time_before(jiffies, deadline));
+	if (ret)
+		return dev_err_probe(pse->dev, ret, "failed to read MCU info\n");
+
+	switch (info->device_id) {
+	case RTPSE_MCU_DEVICE_ID_RTL8238B:
+		pse->chip = &rtl8238b_info;
+		break;
+	case RTPSE_MCU_DEVICE_ID_RTL8239:
+		pse->chip = &rtl8239_info;
+		break;
+	case RTPSE_MCU_DEVICE_ID_RTL8239C:
+		pse->chip = &rtl8239c_info;
+		break;
+	case RTPSE_MCU_DEVICE_ID_BCM59111:
+		pse->chip = &bcm59111_info;
+		break;
+	case RTPSE_MCU_DEVICE_ID_BCM59121:
+		pse->chip = &bcm59121_info;
+		break;
+	default:
+		return dev_err_probe(pse->dev, -EINVAL, "unknown PSE id 0x%x\n",
+				     info->device_id);
+	}
+
+	if (!info->max_ports || info->max_ports > RTPSE_MCU_MAX_PORTS)
+		return dev_err_probe(pse->dev, -EINVAL,
+				     "MCU reports invalid port count %u\n", info->max_ports);
+
+	ret = rtpse_mcu_get_ext_config(pse, &ext_config);
+	if (ret)
+		return dev_err_probe(pse->dev, ret, "failed to read MCU ext config\n");
+
+	dev_info(pse->dev, "%s MCU, %s (id 0x%04x), %u ports across %u PSE chip(s)\n",
+		 pse->dialect->mcu_type_str(info->mcu_type), pse->chip->name,
+		 info->device_id, info->max_ports, ext_config.num_of_pses);
+	return 0;
+}
+
+static void rtpse_mcu_global_disable(void *data)
+{
+	struct rtpse_mcu_ctrl *pse = data;
+
+	rtpse_mcu_set_global_state(pse, false);
+}
+
+int rtpse_mcu_register(struct rtpse_mcu_ctrl *pse)
+{
+	const struct rtpse_mcu_match_data *match;
+	struct rtpse_mcu_info info;
+	struct gpio_desc *gpiod;
+	int ret;
+
+	BUILD_BUG_ON(sizeof(struct rtpse_mcu_msg) != RTPSE_MCU_MSG_SIZE);
+
+	ret = devm_mutex_init(pse->dev, &pse->mutex);
+	if (ret)
+		return ret;
+
+	match = device_get_match_data(pse->dev);
+	if (!match)
+		return dev_err_probe(pse->dev, -ENODEV, "missing match data\n");
+	pse->dialect = match->dialect;
+
+	/*
+	 * Catch a dialect that forgot to set one of the required hooks at
+	 * probe time, rather than NULL-deref'ing later from a fast path.
+	 */
+	if (!pse->dialect ||
+	    !pse->dialect->parse_system_info ||
+	    !pse->dialect->parse_port_class ||
+	    !pse->dialect->mcu_type_str)
+		return dev_err_probe(pse->dev, -EINVAL,
+				     "dialect for chip is incomplete\n");
+
+	/*
+	 * Release the MCU from reset before the first transaction; the
+	 * boot-retry loop in discover() waits for it to answer.
+	 */
+	gpiod = devm_gpiod_get_optional(pse->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(gpiod))
+		return dev_err_probe(pse->dev, PTR_ERR(gpiod),
+				     "failed to get reset gpio\n");
+
+	ret = rtpse_mcu_discover(pse, &info);
+	if (ret)
+		return ret;
+
+	/*
+	 * Some boards gate all ports through a hardware line; deassert it only
+	 * once the MCU is confirmed present, so a failed probe leaves the ports
+	 * gated. From here the MCU protocol governs per-port admin state.
+	 */
+	gpiod = devm_gpiod_get_optional(pse->dev, "disable-ports", GPIOD_OUT_LOW);
+	if (IS_ERR(gpiod))
+		return dev_err_probe(pse->dev, PTR_ERR(gpiod),
+				     "failed to get disable-ports gpio\n");
+
+	if (!info.system_enable) {
+		ret = rtpse_mcu_set_global_state(pse, true);
+		/* Dialects without a global-state concept (e.g. Gen1) return
+		 * -EOPNOTSUPP; treat that as "no separate enable required".
+		 */
+		if (ret && ret != -EOPNOTSUPP)
+			return dev_err_probe(pse->dev, ret,
+					     "failed to enable PSE system\n");
+		if (!ret) {
+			ret = devm_add_action_or_reset(pse->dev,
+						       rtpse_mcu_global_disable, pse);
+			if (ret)
+				return ret;
+		}
+	}
+
+	/*
+	 * Depending on the MCU firmware configuration (which might be different
+	 * for every board), it isn't known whether the PoE subsystem is active or
+	 * inactive by default. At this stage, the PSE chips might already deliver
+	 * power to PDs without any explicit enable.
+	 */
+
+	/* pcdev.owner is set by the transport, so the registered controller
+	 * pins the transport module that owns the live device, not the core.
+	 */
+	pse->pcdev.ops      = &rtpse_mcu_ops;
+	pse->pcdev.dev      = pse->dev;
+	pse->pcdev.types    = ETHTOOL_PSE_C33;
+	pse->pcdev.nr_lines = info.max_ports;
+	pse->pcdev.pis_prio_max = RTPSE_MCU_PORT_MAX_PRIORITY;
+	pse->pcdev.supp_budget_eval_strategies = PSE_BUDGET_EVAL_STRAT_DYNAMIC;
+
+	return devm_pse_controller_register(pse->dev, &pse->pcdev);
+}
+EXPORT_SYMBOL_GPL(rtpse_mcu_register);
+
+static void rtpse_mcu_gen2_parse_system_info(const u8 *payload, struct rtpse_mcu_info *info)
+{
+	info->max_ports = payload[1];
+	info->system_enable = (payload[2] == 0x1);
+	info->device_id = get_unaligned_be16(&payload[3]);
+	info->mcu_type = payload[6];
+}
+
+static int rtpse_mcu_gen2_parse_port_class(const struct rtpse_mcu_port_status *status)
+{
+	/* Class lives in the upper nibble of sts2. */
+	return FIELD_GET(GENMASK(7, 4), status->sts2);
+}
+
+static const char *rtpse_mcu_gen2_mcu_type_str(unsigned int mcu_type)
+{
+	switch (mcu_type) {
+	case 0x00:	return "GigaDevice GD32F310";
+	case 0x01:	return "GigaDevice GD32F230";
+	case 0x02:	return "GigaDevice GD32F303";
+	case 0x03:	return "GigaDevice GD32F103";
+	case 0x04:	return "GigaDevice GD32E103";
+	case 0x10:	return "Nuvoton M0516";
+	case 0x11:	return "Nuvoton M0564";
+	case 0x12:	return "Nuvoton NUC029";
+	default:	return "unknown";
+	}
+}
+
+static void rtpse_mcu_gen1_parse_system_info(const u8 *payload, struct rtpse_mcu_info *info)
+{
+	info->max_ports = payload[1];
+	/* Gen1 has no explicit system_enable byte; the closest analog is the
+	 * "remote enable" bit in the system-status flags at payload[7].
+	 */
+	info->system_enable = !!(payload[7] & BIT(2));
+	info->device_id = get_unaligned_be16(&payload[3]);
+	info->mcu_type = payload[6];
+}
+
+static int rtpse_mcu_gen1_parse_port_class(const struct rtpse_mcu_port_status *status)
+{
+	/* Gen1 puts the detected class in payload[3] (== sts3) directly.
+	 * Mask to the low nibble; class is 0..8 and any high bits would be
+	 * noise.
+	 */
+	return status->sts3 & 0x0f;
+}
+
+static const char *rtpse_mcu_gen1_mcu_type_str(unsigned int mcu_type)
+{
+	switch (mcu_type) {
+	case 0x00:	return "ST Micro ST32F100";
+	case 0x01:	return "Nuvoton M05xx LAN";
+	case 0x02:	return "ST Micro STF030C8";
+	case 0x03:	return "Nuvoton M058SAN";
+	case 0x04:	return "Nuvoton NUC122";
+	default:	return "unknown";
+	}
+}
+
+/* Map each logical command the core issues to its per-dialect opcode. */
+static const struct rtpse_mcu_dialect rtpse_mcu_dialect_gen2 = {
+	.parse_system_info = rtpse_mcu_gen2_parse_system_info,
+	.parse_port_class  = rtpse_mcu_gen2_parse_port_class,
+	.mcu_type_str      = rtpse_mcu_gen2_mcu_type_str,
+	.opcode = {
+		[RTPSE_MCU_CMD_SET_GLOBAL_STATE]	= RTPSE_MCU_OP(0x00),
+		[RTPSE_MCU_CMD_GET_SYSTEM_INFO]		= RTPSE_MCU_OP(0x40),
+		[RTPSE_MCU_CMD_GET_EXT_CONFIG]		= RTPSE_MCU_OP(0x4a),
+
+		[RTPSE_MCU_CMD_PORT_ENABLE]		= RTPSE_MCU_OP(0x01),
+		[RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_TYPE] = RTPSE_MCU_OP(0x12),
+		[RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT]	= RTPSE_MCU_OP(0x13),
+		[RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_EXT] = RTPSE_MCU_OP(0x14),
+		[RTPSE_MCU_CMD_PORT_SET_PRIORITY]	= RTPSE_MCU_OP(0x15),
+		[RTPSE_MCU_CMD_PORT_GET_STATUS]		= RTPSE_MCU_OP(0x42),
+		[RTPSE_MCU_CMD_PORT_GET_POWER_STATS]	= RTPSE_MCU_OP(0x44),
+		[RTPSE_MCU_CMD_PORT_GET_CONFIG]		= RTPSE_MCU_OP(0x48),
+		[RTPSE_MCU_CMD_PORT_GET_EXT_CONFIG]	= RTPSE_MCU_OP(0x49),
+	},
+};
+
+static const struct rtpse_mcu_dialect rtpse_mcu_dialect_gen1 = {
+	.parse_system_info = rtpse_mcu_gen1_parse_system_info,
+	.parse_port_class  = rtpse_mcu_gen1_parse_port_class,
+	.mcu_type_str      = rtpse_mcu_gen1_mcu_type_str,
+	.opcode = {
+		[RTPSE_MCU_CMD_GET_SYSTEM_INFO]		= RTPSE_MCU_OP(0x20),
+		[RTPSE_MCU_CMD_GET_EXT_CONFIG]		= RTPSE_MCU_OP(0x2b),
+
+		[RTPSE_MCU_CMD_PORT_ENABLE]		= RTPSE_MCU_OP(0x00),
+		[RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_TYPE] = RTPSE_MCU_OP(0x15),
+		[RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT]	= RTPSE_MCU_OP(0x16),
+		[RTPSE_MCU_CMD_PORT_SET_PRIORITY]	= RTPSE_MCU_OP(0x1a),
+		[RTPSE_MCU_CMD_PORT_GET_STATUS]		= RTPSE_MCU_OP(0x21),
+		[RTPSE_MCU_CMD_PORT_GET_POWER_STATS]	= RTPSE_MCU_OP(0x30),
+		[RTPSE_MCU_CMD_PORT_GET_CONFIG]		= RTPSE_MCU_OP(0x25),
+		[RTPSE_MCU_CMD_PORT_GET_EXT_CONFIG]	= RTPSE_MCU_OP(0x26),
+	},
+};
+
+const struct rtpse_mcu_match_data rtpse_mcu_gen1_data = {
+	.dialect = &rtpse_mcu_dialect_gen1,
+};
+EXPORT_SYMBOL_GPL(rtpse_mcu_gen1_data);
+
+const struct rtpse_mcu_match_data rtpse_mcu_gen2_data = {
+	.dialect = &rtpse_mcu_dialect_gen2,
+};
+EXPORT_SYMBOL_GPL(rtpse_mcu_gen2_data);
+
+/* Same dialect as gen2, but the MCU expects raw-I2C framing. */
+const struct rtpse_mcu_match_data rtpse_mcu_gen2_i2c_data = {
+	.dialect = &rtpse_mcu_dialect_gen2,
+	.native_i2c = true,
+};
+EXPORT_SYMBOL_GPL(rtpse_mcu_gen2_i2c_data);
+
+MODULE_AUTHOR("Jonas Jelonek <jelonek.jonas@gmail.com>");
+MODULE_DESCRIPTION("Realtek PSE MCU driver (core)");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/pse-pd/realtek-pse-mcu.h b/drivers/net/pse-pd/realtek-pse-mcu.h
new file mode 100644
index 000000000000..52f9442d7117
--- /dev/null
+++ b/drivers/net/pse-pd/realtek-pse-mcu.h
@@ -0,0 +1,93 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _REALTEK_PSE_MCU_H
+#define _REALTEK_PSE_MCU_H
+
+#include <linux/mutex.h>
+#include <linux/pse-pd/pse.h>
+#include <linux/types.h>
+
+/*
+ * Time the MCU itself needs between accepting a request and having a
+ * response ready. These are properties of the MCU firmware, not of the
+ * underlying transport: the core paces transactions by RTPSE_MCU_RESPONSE_MS
+ * and both transports size their per-transaction recv ceiling from
+ * RTPSE_MCU_RESPONSE_MAX_MS, since some commands are documented as
+ * needing up to ~1s to produce a reply.
+ */
+#define RTPSE_MCU_RESPONSE_MS			25
+#define RTPSE_MCU_RESPONSE_MAX_MS		1000
+
+/*
+ * Total time to keep retrying the first MCU read at probe, and the pause
+ * between attempts. Right after reset-gpios is deasserted the MCU may not
+ * answer on the bus yet; give it a bounded window to come up before
+ * declaring the probe failed.
+ */
+#define RTPSE_MCU_BOOT_TIMEOUT_MS		3000
+#define RTPSE_MCU_BOOT_RETRY_MS			100
+
+#define RTPSE_MCU_MSG_SIZE			12
+
+struct rtpse_mcu_msg {
+	u8 opcode;
+	u8 seq_num;
+	u8 payload[9];
+	u8 checksum;
+} __packed;
+
+/*
+ * MCU status opcodes (seen on the Gen1 dialect; Gen2 never emits them).
+ * INCOMPLETE/BAD_CSUM are terminal; NOT_READY is transient.
+ */
+#define RTPSE_MCU_OPCODE_INCOMPLETE		0xfd	/* -EBADE   */
+#define RTPSE_MCU_OPCODE_BAD_CSUM		0xfe	/* -EBADMSG */
+#define RTPSE_MCU_OPCODE_NOT_READY		0xff	/* -EAGAIN  */
+
+/*
+ * A polling transport can stop here: the reply to this request (opcode and
+ * seq_num), or a terminal error. The seq_num rejects a stale reply left by an
+ * abandoned transaction, instead of desyncing by one frame.
+ */
+static inline bool rtpse_mcu_resp_is_final(const struct rtpse_mcu_msg *req,
+					   const struct rtpse_mcu_msg *resp)
+{
+	return (resp->opcode == req->opcode && resp->seq_num == req->seq_num) ||
+	       resp->opcode == RTPSE_MCU_OPCODE_INCOMPLETE ||
+	       resp->opcode == RTPSE_MCU_OPCODE_BAD_CSUM;
+}
+
+/* Opaque to transports; defined in realtek-pse-mcu-core.c. */
+struct rtpse_mcu_dialect;
+struct rtpse_mcu_chip_info;
+struct rtpse_mcu_ctrl;
+
+/* Per-compatible match data (the of_match .data). */
+struct rtpse_mcu_match_data {
+	const struct rtpse_mcu_dialect *dialect;
+	bool native_i2c;	/* raw-I2C framing (vs SMBus); I2C transport only */
+};
+
+struct rtpse_mcu_transport_ops {
+	int (*send)(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req);
+	int (*recv)(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req,
+		    struct rtpse_mcu_msg *resp);
+};
+
+struct rtpse_mcu_ctrl {
+	struct device *dev;
+	struct pse_controller_dev pcdev;
+	struct mutex mutex;	/* serializes MCU request/response transactions */
+	const struct rtpse_mcu_dialect *dialect;
+	const struct rtpse_mcu_chip_info *chip;
+	const struct rtpse_mcu_transport_ops *transport;
+	u8 seq;			/* rolling request seq_num, echoed by the MCU */
+};
+
+int rtpse_mcu_register(struct rtpse_mcu_ctrl *pse);
+
+extern const struct rtpse_mcu_match_data rtpse_mcu_gen1_data;
+extern const struct rtpse_mcu_match_data rtpse_mcu_gen2_data;
+extern const struct rtpse_mcu_match_data rtpse_mcu_gen2_i2c_data;
+
+#endif
-- 
2.53.0

[PATCH net-next v11 3/4] net: pse-pd: realtek-pse-mcu: add I2C transport

[Jonas Jelonek]

Add the I2C/SMBus transport for the Realtek PSE MCU core. It registers
the MCU on an I2C bus and provides the send/recv callbacks the core
uses to exchange the 12-byte frames.

The MCU firmware expects one of two framings on the I2C bus, and which one
is part of the compatible: '-smbus' (reads carry a leading command byte
and a repeated start) or raw '-i2c' (bare block writes and reads). The
match data flags the raw-I2C case; SMBus is the default because that's
what the majority of devices uses.

Because i2c_master_send()/i2c_master_recv() may DMA, the raw-I2C path
bounces each frame through a heap buffer rather than the core's stack
buffers; the SMBus path is unaffected.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/Kconfig               |  11 ++
 drivers/net/pse-pd/Makefile              |   1 +
 drivers/net/pse-pd/realtek-pse-mcu-i2c.c | 170 +++++++++++++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-i2c.c

diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
index 3b0c245a2bc7..6d14c8832e8b 100644
--- a/drivers/net/pse-pd/Kconfig
+++ b/drivers/net/pse-pd/Kconfig
@@ -19,6 +19,17 @@ config PSE_REALTEK_MCU
 	  Shared core for the Realtek PSE MCU driver. This is selected
 	  automatically by the transport options below.
 
+config PSE_REALTEK_MCU_I2C
+	tristate "Realtek PSE MCU driver (I2C transport)"
+	depends on I2C
+	select PSE_REALTEK_MCU
+	help
+	  Driver for the microcontroller (MCU) that fronts the PSE
+	  hardware on various Realtek-based managed switches, attached
+	  via I2C/SMBus. The MCU exposes a message-based protocol; the actual
+	  PSE silicon is not accessed directly. To compile this driver as a
+	  module, choose M here: the module will be called realtek-pse-mcu-i2c.
+
 config PSE_REGULATOR
 	tristate "Regulator based PSE controller"
 	help
diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
index bf35e2a5b110..ef869bba5ed9 100644
--- a/drivers/net/pse-pd/Makefile
+++ b/drivers/net/pse-pd/Makefile
@@ -4,6 +4,7 @@
 obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
 
 obj-$(CONFIG_PSE_REALTEK_MCU) += realtek-pse-mcu-core.o
+obj-$(CONFIG_PSE_REALTEK_MCU_I2C) += realtek-pse-mcu-i2c.o
 obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
 obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
 obj-$(CONFIG_PSE_SI3474) += si3474.o
diff --git a/drivers/net/pse-pd/realtek-pse-mcu-i2c.c b/drivers/net/pse-pd/realtek-pse-mcu-i2c.c
new file mode 100644
index 000000000000..e10f8c633b68
--- /dev/null
+++ b/drivers/net/pse-pd/realtek-pse-mcu-i2c.c
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pse-pd/pse.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include "realtek-pse-mcu.h"
+
+/*
+ * The core has already waited RTPSE_MCU_RESPONSE_MS before calling us, so
+ * the response is normally ready on the very first read. For commands the
+ * MCU produces more slowly, keep polling at the typical response cadence
+ * up to the worst-case ceiling.
+ */
+#define RTPSE_MCU_I2C_RETRY_MS	RTPSE_MCU_RESPONSE_MS
+#define RTPSE_MCU_I2C_MAX_TRIES	(RTPSE_MCU_RESPONSE_MAX_MS / RTPSE_MCU_I2C_RETRY_MS)
+
+static int rtpse_mcu_i2c_smbus_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
+{
+	struct i2c_client *client = to_i2c_client(pse->dev);
+
+	/* Send opcode as SMBus command byte; remaining 11 bytes as block data */
+	return i2c_smbus_write_i2c_block_data(client, req->opcode, RTPSE_MCU_MSG_SIZE - 1,
+					      (u8 *)req + 1);
+}
+
+static int rtpse_mcu_i2c_smbus_recv(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req,
+				    struct rtpse_mcu_msg *resp)
+{
+	struct i2c_client *client = to_i2c_client(pse->dev);
+	int tries, ret;
+
+	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
+		if (tries > 0)
+			msleep(RTPSE_MCU_I2C_RETRY_MS);
+
+		/* MCU needs 0x00 as command byte for read */
+		ret = i2c_smbus_read_i2c_block_data(client, 0x00,
+						    RTPSE_MCU_MSG_SIZE,
+						    (u8 *)resp);
+		if (ret < 0)
+			return ret;
+		if (ret == RTPSE_MCU_MSG_SIZE && rtpse_mcu_resp_is_final(req, resp))
+			return 0;
+	}
+
+	return -ETIMEDOUT;
+}
+
+static const struct rtpse_mcu_transport_ops rtpse_mcu_i2c_smbus_ops = {
+	.send = rtpse_mcu_i2c_smbus_send,
+	.recv = rtpse_mcu_i2c_smbus_recv,
+};
+
+static int rtpse_mcu_i2c_native_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
+{
+	struct i2c_client *client = to_i2c_client(pse->dev);
+	void *buf;
+	int ret;
+
+	/* i2c_master_send() may DMA, so the buffer must not be on the stack. */
+	buf = kmemdup(req, RTPSE_MCU_MSG_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = i2c_master_send(client, buf, RTPSE_MCU_MSG_SIZE);
+	kfree(buf);
+	if (ret < 0)
+		return ret;
+	return ret == RTPSE_MCU_MSG_SIZE ? 0 : -EIO;
+}
+
+static int rtpse_mcu_i2c_native_recv(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req,
+				     struct rtpse_mcu_msg *resp)
+{
+	struct i2c_client *client = to_i2c_client(pse->dev);
+	int tries, ret;
+	u8 *buf;
+
+	/* i2c_master_recv() may DMA, so read into an off-stack buffer. */
+	buf = kmalloc(RTPSE_MCU_MSG_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
+		if (tries > 0)
+			msleep(RTPSE_MCU_I2C_RETRY_MS);
+
+		ret = i2c_master_recv(client, buf, RTPSE_MCU_MSG_SIZE);
+		if (ret < 0)
+			goto out;
+		if (ret == RTPSE_MCU_MSG_SIZE) {
+			memcpy(resp, buf, RTPSE_MCU_MSG_SIZE);
+			if (rtpse_mcu_resp_is_final(req, resp)) {
+				ret = 0;
+				goto out;
+			}
+		}
+	}
+	ret = -ETIMEDOUT;
+out:
+	kfree(buf);
+	return ret;
+}
+
+static const struct rtpse_mcu_transport_ops rtpse_mcu_i2c_native_ops = {
+	.send = rtpse_mcu_i2c_native_send,
+	.recv = rtpse_mcu_i2c_native_recv,
+};
+
+static int rtpse_mcu_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	const struct rtpse_mcu_match_data *match;
+	struct rtpse_mcu_ctrl *pse;
+	bool use_native;
+
+	match = device_get_match_data(dev);
+	if (!match)
+		return dev_err_probe(dev, -ENODEV, "missing match data\n");
+
+	/* The framing (raw I2C vs SMBus) is carried by the match data. */
+	use_native = match->native_i2c;
+	if (use_native) {
+		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+			return dev_err_probe(dev, -EOPNOTSUPP,
+				"plain-I2C MCU protocol requires I2C-capable adapter\n");
+	} else {
+		if (!i2c_check_functionality(client->adapter,
+					     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
+					     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
+			return dev_err_probe(dev, -EOPNOTSUPP,
+				"SMBus MCU protocol requires SMBus I2C-block support\n");
+	}
+
+	pse = devm_kzalloc(dev, sizeof(*pse), GFP_KERNEL);
+	if (!pse)
+		return -ENOMEM;
+
+	pse->dev = dev;
+	pse->pcdev.owner = THIS_MODULE;
+	pse->transport = use_native ? &rtpse_mcu_i2c_native_ops : &rtpse_mcu_i2c_smbus_ops;
+
+	return rtpse_mcu_register(pse);
+}
+
+static const struct of_device_id rtpse_mcu_i2c_of_match[] = {
+	{ .compatible = "realtek,pse-mcu-gen1-smbus", .data = &rtpse_mcu_gen1_data },
+	{ .compatible = "realtek,pse-mcu-gen2-smbus", .data = &rtpse_mcu_gen2_data },
+	{ .compatible = "realtek,pse-mcu-gen2-i2c", .data = &rtpse_mcu_gen2_i2c_data },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rtpse_mcu_i2c_of_match);
+
+static struct i2c_driver rtpse_mcu_i2c_driver = {
+	.driver = {
+		.name		= "realtek-pse-mcu-i2c",
+		.of_match_table	= rtpse_mcu_i2c_of_match,
+	},
+	.probe		= rtpse_mcu_i2c_probe,
+};
+module_i2c_driver(rtpse_mcu_i2c_driver);
+
+MODULE_AUTHOR("Jonas Jelonek <jelonek.jonas@gmail.com>");
+MODULE_DESCRIPTION("Realtek PSE MCU driver (I2C transport)");
+MODULE_LICENSE("GPL");
-- 
2.53.0

[PATCH net-next v11 4/4] net: pse-pd: realtek-pse-mcu: add UART transport

[Jonas Jelonek]

Add the serdev (UART) transport for the Realtek PSE MCU core. It registers
the MCU as a serdev device and provides the send/recv callbacks the core
uses to exchange the 12-byte frames, receiving asynchronously via the
serdev receive_buf callback.

The baud rate defaults to 19200 and can be overridden per board with the
"current-speed" property.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/Kconfig                |  11 ++
 drivers/net/pse-pd/Makefile               |   1 +
 drivers/net/pse-pd/realtek-pse-mcu-uart.c | 164 ++++++++++++++++++++++
 3 files changed, 176 insertions(+)
 create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-uart.c

diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig
index 6d14c8832e8b..a0f2ae668c67 100644
--- a/drivers/net/pse-pd/Kconfig
+++ b/drivers/net/pse-pd/Kconfig
@@ -30,6 +30,17 @@ config PSE_REALTEK_MCU_I2C
 	  PSE silicon is not accessed directly. To compile this driver as a
 	  module, choose M here: the module will be called realtek-pse-mcu-i2c.
 
+config PSE_REALTEK_MCU_UART
+	tristate "Realtek PSE MCU driver (UART transport)"
+	depends on SERIAL_DEV_BUS
+	select PSE_REALTEK_MCU
+	help
+	  Driver for the microcontroller (MCU) that fronts the PSE
+	  hardware on various Realtek-based managed switches, attached
+	  via UART. The MCU exposes a message-based protocol; the actual PSE
+	  silicon is not accessed directly. To compile this driver as a
+	  module, choose M here: the module will be called realtek-pse-mcu-uart.
+
 config PSE_REGULATOR
 	tristate "Regulator based PSE controller"
 	help
diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile
index ef869bba5ed9..9cca5900fe34 100644
--- a/drivers/net/pse-pd/Makefile
+++ b/drivers/net/pse-pd/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PSE_CONTROLLER) += pse_core.o
 
 obj-$(CONFIG_PSE_REALTEK_MCU) += realtek-pse-mcu-core.o
 obj-$(CONFIG_PSE_REALTEK_MCU_I2C) += realtek-pse-mcu-i2c.o
+obj-$(CONFIG_PSE_REALTEK_MCU_UART) += realtek-pse-mcu-uart.o
 obj-$(CONFIG_PSE_REGULATOR) += pse_regulator.o
 obj-$(CONFIG_PSE_PD692X0) += pd692x0.o
 obj-$(CONFIG_PSE_SI3474) += si3474.o
diff --git a/drivers/net/pse-pd/realtek-pse-mcu-uart.c b/drivers/net/pse-pd/realtek-pse-mcu-uart.c
new file mode 100644
index 000000000000..9baa17d8d31f
--- /dev/null
+++ b/drivers/net/pse-pd/realtek-pse-mcu-uart.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pse-pd/pse.h>
+#include <linux/serdev.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#include "realtek-pse-mcu.h"
+
+#define RTPSE_MCU_UART_BAUD_DEFAULT	19200
+#define RTPSE_MCU_UART_TX_TIMEOUT	msecs_to_jiffies(100)
+#define RTPSE_MCU_UART_RX_TIMEOUT	msecs_to_jiffies(RTPSE_MCU_RESPONSE_MAX_MS)
+
+struct rtpse_mcu_uart {
+	struct rtpse_mcu_ctrl pse;
+	struct serdev_device *serdev;
+	struct completion rx_done;
+	spinlock_t rx_lock;		/* protects rx_buf and rx_len */
+	size_t rx_len;
+	u8 rx_buf[RTPSE_MCU_MSG_SIZE];
+};
+
+#define to_rtpse_mcu_uart(p)  container_of(p, struct rtpse_mcu_uart, pse)
+
+/*
+ * No framing is done here: a glitched frame costs one transaction, then
+ * the next _send re-frames from rx_len 0. Resync works by returning count
+ * (not take), dropping any overflow so serdev keeps no leftover to bleed
+ * into the next frame.
+ */
+static size_t rtpse_mcu_uart_receive(struct serdev_device *serdev,
+				     const u8 *buf, size_t count)
+{
+	struct rtpse_mcu_uart *ctx = serdev_device_get_drvdata(serdev);
+	size_t take;
+
+	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
+		take = min(count, sizeof(ctx->rx_buf) - ctx->rx_len);
+		if (take) {
+			memcpy(ctx->rx_buf + ctx->rx_len, buf, take);
+			ctx->rx_len += take;
+			if (ctx->rx_len == sizeof(ctx->rx_buf))
+				complete(&ctx->rx_done);
+		}
+	}
+
+	/* consume all to avoid desync/misalignment */
+	return count;
+}
+
+static const struct serdev_device_ops rtpse_mcu_uart_serdev_ops = {
+	.receive_buf = rtpse_mcu_uart_receive,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static int rtpse_mcu_uart_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
+{
+	struct rtpse_mcu_uart *ctx = to_rtpse_mcu_uart(pse);
+	int written;
+
+	/* clear any leftover rx state before transmitting */
+	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
+		reinit_completion(&ctx->rx_done);
+		ctx->rx_len = 0;
+	}
+
+	written = serdev_device_write(ctx->serdev, (const u8 *)req, sizeof(*req),
+				      RTPSE_MCU_UART_TX_TIMEOUT);
+	if (written < 0)
+		return written;
+	if (written != sizeof(*req))
+		return -EIO;
+
+	return 0;
+}
+
+static int rtpse_mcu_uart_recv(struct rtpse_mcu_ctrl *pse,
+			       const struct rtpse_mcu_msg *req,
+			       struct rtpse_mcu_msg *resp)
+{
+	struct rtpse_mcu_uart *ctx = to_rtpse_mcu_uart(pse);
+
+	if (!wait_for_completion_timeout(&ctx->rx_done, RTPSE_MCU_UART_RX_TIMEOUT))
+		return -ETIMEDOUT;
+
+	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
+		if (ctx->rx_len != sizeof(*resp))
+			return -EIO;
+
+		memcpy(resp, ctx->rx_buf, sizeof(*resp));
+	}
+	return 0;
+}
+
+static const struct rtpse_mcu_transport_ops rtpse_mcu_uart_transport_ops = {
+	.send = rtpse_mcu_uart_send,
+	.recv = rtpse_mcu_uart_recv,
+};
+
+static int rtpse_mcu_uart_probe(struct serdev_device *serdev)
+{
+	u32 speed = RTPSE_MCU_UART_BAUD_DEFAULT;
+	struct device *dev = &serdev->dev;
+	struct rtpse_mcu_uart *ctx;
+	unsigned int baud;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->serdev = serdev;
+	ctx->pse.dev = dev;
+	ctx->pse.pcdev.owner = THIS_MODULE;
+	ctx->pse.transport = &rtpse_mcu_uart_transport_ops;
+	init_completion(&ctx->rx_done);
+	spin_lock_init(&ctx->rx_lock);
+
+	serdev_device_set_drvdata(serdev, ctx);
+	serdev_device_set_client_ops(serdev, &rtpse_mcu_uart_serdev_ops);
+
+	ret = devm_serdev_device_open(dev, serdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to open serdev\n");
+
+	fwnode_property_read_u32(dev_fwnode(dev), "current-speed", &speed);
+
+	baud = serdev_device_set_baudrate(serdev, speed);
+	if (baud != speed)
+		dev_warn(dev, "could not set baudrate %u, controller uses %u\n",
+			 speed, baud);
+
+	serdev_device_set_flow_control(serdev, false);
+
+	ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
+	if (ret)
+		dev_warn(dev, "could not set parity to none: %d\n", ret);
+
+	return rtpse_mcu_register(&ctx->pse);
+}
+
+static const struct of_device_id rtpse_mcu_uart_of_match[] = {
+	{ .compatible = "realtek,pse-mcu-gen1", .data = &rtpse_mcu_gen1_data },
+	{ .compatible = "realtek,pse-mcu-gen2", .data = &rtpse_mcu_gen2_data },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rtpse_mcu_uart_of_match);
+
+static struct serdev_device_driver rtpse_mcu_uart_driver = {
+	.driver = {
+		.name = "realtek-pse-mcu-uart",
+		.of_match_table = rtpse_mcu_uart_of_match,
+	},
+	.probe  = rtpse_mcu_uart_probe,
+};
+module_serdev_device_driver(rtpse_mcu_uart_driver);
+
+MODULE_AUTHOR("Jonas Jelonek <jelonek.jonas@gmail.com>");
+MODULE_DESCRIPTION("Realtek PSE MCU driver (UART transport)");
+MODULE_LICENSE("GPL");
-- 
2.53.0

Re: [PATCH net-next v11 1/4] dt-bindings: net: pse-pd: add bindings for Realtek PSE MCU

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dt-bindings: net: pse-pd: add bindings for Realtek PSE MCU

This adds a new YAML binding for the MCU that fronts the PSE silicon on a
range of managed Realtek-based switches, covering both protocol generations
and the I2C/SMBus and UART attachments.  Two questions below about the file
naming and about the serial slave node name.

> diff --git a/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml b/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
> new file mode 100644
> index 0000000000000..3bb32349c28c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
> @@ -0,0 +1,182 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/pse-pd/realtek,pse-mcu-gen1.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#

[ ... ]

> +properties:
> +  compatible:
> +    oneOf:
> +      # UART
> +      - items:
> +          - enum:
> +              - zyxel,gs1900-10hp-a1-pse
> +          - const: realtek,pse-mcu-gen1
> +
> +      # I2C, SMBus framing
> +      - items:
> +          - enum:
> +              - zyxel,gs1920-24hp-v2-pse
> +          - const: realtek,pse-mcu-gen1-smbus
> +
> +      # UART
> +      - items:
> +          - enum:
> +              - zyxel,gs1900-10hp-b1-pse
> +              - zyxel,xmg1915-10ep-pse
> +          - const: realtek,pse-mcu-gen2
> +
> +      # I2C, SMBus framing
> +      - items:
> +          - enum:
> +              - zyxel,xs1930-12hp-pse
> +          - const: realtek,pse-mcu-gen2-smbus
> +
> +      # I2C, raw framing
> +      - items:
> +          - enum:
> +              - linksys,lgs328mpc-v2-pse
> +          - const: realtek,pse-mcu-gen2-i2c

[Severity: Low]
This isn't a bug, but should the file be named after the family rather than
after one compatible?

The file (and $id) is realtek,pse-mcu-gen1.yaml, while the compatible list
here documents five strings across two protocol generations:

    realtek,pse-mcu-gen1
    realtek,pse-mcu-gen1-smbus
    realtek,pse-mcu-gen2
    realtek,pse-mcu-gen2-smbus
    realtek,pse-mcu-gen2-i2c

There is no file bearing the name of any realtek,pse-mcu-gen2* compatible,
so searching by that compatible name finds nothing, and the commit message
does not mention that gen2 is described in a ...-gen1.yaml file.

Would realtek,pse-mcu.yaml (with the $id updated to match) describe the
actual scope better?

To be clear, the $id path itself is consistent with the current file path
(net/pse-pd/realtek,pse-mcu-gen1.yaml#), so $ref resolution and
dt_binding_check are not affected here - this is only about the declared
scope of the name.

> +required:
> +  - compatible
> +
> +allOf:
> +  - $ref: pse-controller.yaml#

[ ... ]

> +    else:
> +      allOf:
> +        - $ref: /schemas/serial/serial-peripheral-props.yaml#
> +
> +      properties:
> +        reg: false

[Severity: Low]
This else branch declares the bare gen compatibles to be UART (serial
slave) attachments, and the node name is forced to ethernet-pse by
pse-controller.yaml:

Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml
  $nodename:
    pattern: "^ethernet-pse(@.*|-([0-9]|[1-9][0-9]+))?$"

But serial.yaml only enumerates these serial slave child node names:

Documentation/devicetree/bindings/serial/serial.yaml
patternProperties:
  "^(bluetooth|bluetooth-gnss|embedded-controller|gnss|gps|mcu|onewire)$":
    if:
      type: object
    then:
      additionalProperties: true
      $ref: serial-peripheral-props.yaml#
      ...
      required:
        - compatible

Does a board device tree with an ethernet-pse child of serial then get the
shared slave-node subschema applied at all?  Since serial.yaml ends with
additionalProperties: true, the unmatched child looks like it just falls
through silently rather than being flagged.

Should serial.yaml's pattern list gain ethernet-pse as part of this series,
or is the intent to reuse the existing generic mcu name (which would
conflict with pse-controller.yaml's $nodename pattern)?

I checked the end of the series and serial.yaml is unchanged there, so no
counterpart update appears later either.

> +unevaluatedProperties: false
> +
> +examples:

[ ... ]

> +  # UART attachment
> +  - |
> +    serial {
> +        ethernet-pse {
> +            compatible = "zyxel,gs1900-10hp-a1-pse", "realtek,pse-mcu-gen1";
> +            current-speed = <19200>;

[ ... ]
-- 
pw-bot: cr

Re: [PATCH net-next v11 2/4] net: pse-pd: add Realtek PSE MCU core

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pse-pd: add Realtek PSE MCU core

This adds the shared core for the Realtek PSE MCU driver: the 12-byte
request/response protocol with sum-mod-256 checksum, the per-dialect opcode
tables and response parser hooks for the two protocol generations, and the
pse_controller_ops glue plus a registration helper for the transport modules.

> diff --git a/drivers/net/pse-pd/realtek-pse-mcu-core.c b/drivers/net/pse-pd/realtek-pse-mcu-core.c
> new file mode 100644
> index 0000000000000..ed3b743470dbf
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-mcu-core.c
> @@ -0,0 +1,988 @@

[ ... ]

> +	/*
> +	 * Explicit MCU error opcodes (observed on the Gen1 dialect; harmless
> +	 * to check for Gen2 too). Catch these before the generic opcode/CRC
> +	 * mismatch path so callers see a meaningful errno.
> +	 */
> +	switch (resp->opcode) {
> +	case RTPSE_MCU_OPCODE_INCOMPLETE:
> +		return -EBADE;
> +	case RTPSE_MCU_OPCODE_BAD_CSUM:
> +		return -EBADMSG;
> +	case RTPSE_MCU_OPCODE_NOT_READY:
> +		return -EAGAIN;
> +	}
> +
> +	if (resp->opcode != req->opcode ||
> +	    resp->seq_num != req->seq_num ||
> +	    resp->checksum != rtpse_mcu_checksum((u8 *)resp, RTPSE_MCU_MSG_SIZE - 1))
> +		return -EBADMSG;
> +
> +	return 0;
> +}

[Severity: Low]
Does this validation in rtpse_mcu_do_xfer() accept an all-zero frame as a
valid reply?

For a request with opcode 0x00 and seq_num 0x00, an all-zero 12-byte
response satisfies every term:

  resp->opcode  == 0 == req->opcode
  resp->seq_num == 0 == req->seq_num
  rtpse_mcu_checksum() over eleven 0x00 bytes == 0 == resp->checksum

Both dialects use 0x00 for a real command:

  rtpse_mcu_dialect_gen1: [RTPSE_MCU_CMD_PORT_ENABLE]      = RTPSE_MCU_OP(0x00)
  rtpse_mcu_dialect_gen2: [RTPSE_MCU_CMD_SET_GLOBAL_STATE] = RTPSE_MCU_OP(0x00)

and seq_num wraps back to 0x00 every 256 transactions since pse->seq is a u8
incremented unconditionally in rtpse_mcu_do_xfer():

  req->seq_num = pse->seq++;

Would it be worth adding a discriminator here, for instance rejecting a frame
whose bytes are all zero, or reserving seq_num 0?

> +static int rtpse_mcu_port_cmd(struct rtpse_mcu_ctrl *pse, unsigned int port, u8 opcode, u8 arg)
> +{
> +	struct rtpse_mcu_msg req, resp;
> +	int ret;
> +
> +	rtpse_mcu_msg_init(&req, opcode);
> +	req.payload[0] = port;
> +	req.payload[1] = arg;
> +
> +	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
> +	if (ret)
> +		return ret;
> +
> +	if (resp.payload[0] != port || resp.payload[1] != 0)
> +		return -EIO;
> +
> +	return 0;
> +}

[Severity: Low]
Related to the above: does this echo check catch the all-zero frame for port
0? An all-zero payload gives payload[0] == 0 == port and payload[1] == 0, so
a Gen1 port-0 enable/disable would be reported as successful even though the
frame carried no MCU data. rtpse_mcu_set_global_state() has the same shape,
it only requires resp.payload[0] == 0x0.

The same hole also shortens the poll in the I2C transport, since
rtpse_mcu_resp_is_final() declares an all-zero read final for opcode 0x00 /
seq_num 0x00 and the loop stops instead of waiting for the genuine reply.

[ ... ]

> +	/*
> +	 * Release the MCU from reset before the first transaction; the
> +	 * boot-retry loop in discover() waits for it to answer.
> +	 */
> +	gpiod = devm_gpiod_get_optional(pse->dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod))
> +		return dev_err_probe(pse->dev, PTR_ERR(gpiod),
> +				     "failed to get reset gpio\n");
> +
> +	ret = rtpse_mcu_discover(pse, &info);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Some boards gate all ports through a hardware line; deassert it only
> +	 * once the MCU is confirmed present, so a failed probe leaves the ports
> +	 * gated. From here the MCU protocol governs per-port admin state.
> +	 */
> +	gpiod = devm_gpiod_get_optional(pse->dev, "disable-ports", GPIOD_OUT_LOW);
> +	if (IS_ERR(gpiod))
> +		return dev_err_probe(pse->dev, PTR_ERR(gpiod),
> +				     "failed to get disable-ports gpio\n");

[Severity: Medium]
Does the comment's promise hold for failures after this point?

rtpse_mcu_register() drives disable-ports low here, but nothing re-asserts it
on the error paths that follow. The devres unwind only releases the
descriptor; gpiod_free_commit() in drivers/gpio/gpiolib.c clears descriptor
flags, drops the label and optionally calls gc->free(), it does not change the
driven output level:

  if (guard.gc && test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
          if (guard.gc->free)
                  guard.gc->free(guard.gc, gpiod_hwgpio(desc));
          clear_bit(GPIOD_FLAG_ACTIVE_LOW, &flags);

So if devm_add_action_or_reset() or devm_pse_controller_register() below fails
(the latter propagates -EINVAL/-ENODEV from of_load_pse_pis() for a
missing or mismatched pse-pis node, and -ENOMEM from the kfifo/devres
allocations), the board is left with all ports ungated and no driver bound.
The same applies on unbind or module unload.

Since the level also persists past the descriptor's lifetime, doesn't a
subsequent probe attempt start with the gate already open? That is exactly the
case the comment guards against: if rtpse_mcu_discover() then fails on a
re-probe, the ports are ungated while the MCU is unresponsive and unmanaged.

Would a devm action that re-asserts the line (and the reset line) fit here?

> +	if (!info.system_enable) {
> +		ret = rtpse_mcu_set_global_state(pse, true);
> +		/* Dialects without a global-state concept (e.g. Gen1) return
> +		 * -EOPNOTSUPP; treat that as "no separate enable required".
> +		 */
> +		if (ret && ret != -EOPNOTSUPP)
> +			return dev_err_probe(pse->dev, ret,
> +					     "failed to enable PSE system\n");
> +		if (!ret) {
> +			ret = devm_add_action_or_reset(pse->dev,
> +						       rtpse_mcu_global_disable, pse);
> +			if (ret)
> +				return ret;
> +		}
> +	}

[Severity: Medium]
Is there any compensating teardown for the Gen1 case? rtpse_mcu_dialect_gen1
has no RTPSE_MCU_CMD_SET_GLOBAL_STATE entry, so
rtpse_mcu_set_global_state() returns -EOPNOTSUPP and no devm action is
registered at all. The same happens on boards that already report
info.system_enable, where the whole block is skipped. For those boards
nothing disables power or re-gates the ports on a later probe failure or on
unbind.

[ ... ]

> diff --git a/drivers/net/pse-pd/realtek-pse-mcu.h b/drivers/net/pse-pd/realtek-pse-mcu.h
> new file mode 100644
> index 0000000000000..52f9442d7117c
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-mcu.h
> @@ -0,0 +1,93 @@

[ ... ]

> +/*
> + * A polling transport can stop here: the reply to this request (opcode and
> + * seq_num), or a terminal error. The seq_num rejects a stale reply left by an
> + * abandoned transaction, instead of desyncing by one frame.
> + */
> +static inline bool rtpse_mcu_resp_is_final(const struct rtpse_mcu_msg *req,
> +					   const struct rtpse_mcu_msg *resp)
> +{
> +	return (resp->opcode == req->opcode && resp->seq_num == req->seq_num) ||
> +	       resp->opcode == RTPSE_MCU_OPCODE_INCOMPLETE ||
> +	       resp->opcode == RTPSE_MCU_OPCODE_BAD_CSUM;
> +}

[Severity: Low]
This is the transport-side half of the same question: for a request with
opcode 0x00 and seq_num 0x00, an all-zero read is declared final here, so
rtpse_mcu_i2c_smbus_recv() / rtpse_mcu_i2c_native_recv() stop polling rather
than retrying for the real frame. Should this helper also reject a frame that
is entirely zero?

Re: [PATCH net-next v11 3/4] net: pse-pd: realtek-pse-mcu: add I2C transport

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pse-pd: realtek-pse-mcu: add I2C transport

This adds the I2C/SMBus transport for the Realtek PSE MCU core, providing
the send/recv callbacks used to exchange the 12-byte MCU frames. Two
framings are supported and selected by the compatible: SMBus block
transfers by default, and raw I2C when the match data sets native_i2c.

> diff --git a/drivers/net/pse-pd/realtek-pse-mcu-i2c.c b/drivers/net/pse-pd/realtek-pse-mcu-i2c.c
> new file mode 100644
> index 0000000000000..e10f8c633b68e
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-mcu-i2c.c
> @@ -0,0 +1,170 @@

[ ... ]

> +static int rtpse_mcu_i2c_smbus_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
> +{
> +	struct i2c_client *client = to_i2c_client(pse->dev);
> +
> +	/* Send opcode as SMBus command byte; remaining 11 bytes as block data */
> +	return i2c_smbus_write_i2c_block_data(client, req->opcode, RTPSE_MCU_MSG_SIZE - 1,
> +					      (u8 *)req + 1);
> +}
> +
> +static int rtpse_mcu_i2c_smbus_recv(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req,
> +				    struct rtpse_mcu_msg *resp)
> +{
> +	struct i2c_client *client = to_i2c_client(pse->dev);
> +	int tries, ret;
> +
> +	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
> +		if (tries > 0)
> +			msleep(RTPSE_MCU_I2C_RETRY_MS);
> +
> +		/* MCU needs 0x00 as command byte for read */
> +		ret = i2c_smbus_read_i2c_block_data(client, 0x00,
> +						    RTPSE_MCU_MSG_SIZE,
> +						    (u8 *)resp);

[ ... ]

> +static int rtpse_mcu_i2c_native_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
> +{
> +	struct i2c_client *client = to_i2c_client(pse->dev);
> +	void *buf;
> +	int ret;
> +
> +	/* i2c_master_send() may DMA, so the buffer must not be on the stack. */
> +	buf = kmemdup(req, RTPSE_MCU_MSG_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = i2c_master_send(client, buf, RTPSE_MCU_MSG_SIZE);
> +	kfree(buf);

[Severity: Low]
Is the premise in this comment correct?  i2c_master_send() and
i2c_master_recv() both go through i2c_transfer_buffer_flags(), which
builds the message as:

drivers/i2c/i2c-core-base.c:i2c_transfer_buffer_flags() {
	struct i2c_msg msg = {
		.addr = client->addr,
		.flags = flags | (client->flags & I2C_M_TEN),
		.len = count,
		.buf = buf,
	};
	...
}

I2C_M_DMA_SAFE is never set there, so an adapter that wants to DMA has to
obtain its buffer from i2c_get_dma_safe_msg_buf(), which bounces again
exactly because the flag is absent:

drivers/i2c/i2c-core-base.c:i2c_get_dma_safe_msg_buf() {
	...
	if (msg->flags & I2C_M_DMA_SAFE)
		return msg->buf;
	...
	if (msg->flags & I2C_M_RD)
		return kzalloc(msg->len, GFP_KERNEL);
	else
		return kmemdup(msg->buf, msg->len, GFP_KERNEL);
}

So does the kmemdup() here actually achieve anything?  On a DMA-capable
adapter the 12-byte frame is copied a second time by the core, and the
core's on-stack struct rtpse_mcu_msg was already legal for these APIs
(Documentation/i2c/dma-considerations.rst: "it is *not* mandatory that the
buffer of an I2C message is DMA safe").

The SMBus path in the same file already passes the caller-supplied stack
req/resp straight into i2c_smbus_write_i2c_block_data() and
i2c_smbus_read_i2c_block_data(), which seems to contradict the rationale
used for the raw-I2C path.

The same question applies to rtpse_mcu_i2c_native_recv() below, where the
kmalloc() plus the memcpy(resp, buf, RTPSE_MCU_MSG_SIZE) duplicate what
i2c_get_dma_safe_msg_buf()/i2c_put_dma_safe_msg_buf() already do for a
non-DMA-safe read buffer.  It also adds an -ENOMEM abort to a PSE control
transaction that the bus core could otherwise always complete.

If skipping the core's bounce is really the goal, should these buffers be
handed to i2c_master_send_dmasafe()/i2c_master_recv_dmasafe(), or to an
explicit i2c_msg with I2C_M_DMA_SAFE?  Otherwise, could the allocations
and both comments simply be dropped?

The commit message carries the same statement:

  "Because i2c_master_send()/i2c_master_recv() may DMA, the raw-I2C path
  bounces each frame through a heap buffer rather than the core's stack
  buffers; the SMBus path is unaffected."

Could this be reworded so it does not document a DMA rule that the I2C
core does not impose?

> +	if (ret < 0)
> +		return ret;
> +	return ret == RTPSE_MCU_MSG_SIZE ? 0 : -EIO;
> +}
> +
> +static int rtpse_mcu_i2c_native_recv(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req,
> +				     struct rtpse_mcu_msg *resp)
> +{
> +	struct i2c_client *client = to_i2c_client(pse->dev);
> +	int tries, ret;
> +	u8 *buf;
> +
> +	/* i2c_master_recv() may DMA, so read into an off-stack buffer. */
> +	buf = kmalloc(RTPSE_MCU_MSG_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
> +		if (tries > 0)
> +			msleep(RTPSE_MCU_I2C_RETRY_MS);
> +
> +		ret = i2c_master_recv(client, buf, RTPSE_MCU_MSG_SIZE);
> +		if (ret < 0)
> +			goto out;
> +		if (ret == RTPSE_MCU_MSG_SIZE) {
> +			memcpy(resp, buf, RTPSE_MCU_MSG_SIZE);

[ ... ]

Re: [PATCH net-next v11 4/4] net: pse-pd: realtek-pse-mcu: add UART transport

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pse-pd: realtek-pse-mcu: add UART transport

This adds a serdev (UART) transport for the Realtek PSE MCU core, with
send/recv callbacks that exchange the 12-byte frames and an asynchronous
receive_buf callback. The baud rate defaults to 19200 and can be overridden
with the "current-speed" property.

> diff --git a/drivers/net/pse-pd/realtek-pse-mcu-uart.c b/drivers/net/pse-pd/realtek-pse-mcu-uart.c
> new file mode 100644
> index 0000000000000..9baa17d8d31fa
> --- /dev/null
> +++ b/drivers/net/pse-pd/realtek-pse-mcu-uart.c
> @@ -0,0 +1,164 @@

[ ... ]

> +/*
> + * No framing is done here: a glitched frame costs one transaction, then
> + * the next _send re-frames from rx_len 0. Resync works by returning count
> + * (not take), dropping any overflow so serdev keeps no leftover to bleed
> + * into the next frame.
> + */
> +static size_t rtpse_mcu_uart_receive(struct serdev_device *serdev,
> +				     const u8 *buf, size_t count)
> +{
> +	struct rtpse_mcu_uart *ctx = serdev_device_get_drvdata(serdev);
> +	size_t take;
> +
> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
> +		take = min(count, sizeof(ctx->rx_buf) - ctx->rx_len);
> +		if (take) {
> +			memcpy(ctx->rx_buf + ctx->rx_len, buf, take);
> +			ctx->rx_len += take;
> +			if (ctx->rx_len == sizeof(ctx->rx_buf))
> +				complete(&ctx->rx_done);
> +		}
> +	}
> +
> +	/* consume all to avoid desync/misalignment */
> +	return count;
> +}

[ ... ]

> +static int rtpse_mcu_uart_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
> +{
> +	struct rtpse_mcu_uart *ctx = to_rtpse_mcu_uart(pse);
> +	int written;
> +
> +	/* clear any leftover rx state before transmitting */
> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
> +		reinit_completion(&ctx->rx_done);
> +		ctx->rx_len = 0;
> +	}

[ ... ]

> +static int rtpse_mcu_uart_recv(struct rtpse_mcu_ctrl *pse,
> +			       const struct rtpse_mcu_msg *req,
> +			       struct rtpse_mcu_msg *resp)
> +{
> +	struct rtpse_mcu_uart *ctx = to_rtpse_mcu_uart(pse);
> +
> +	if (!wait_for_completion_timeout(&ctx->rx_done, RTPSE_MCU_UART_RX_TIMEOUT))
> +		return -ETIMEDOUT;
> +
> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
> +		if (ctx->rx_len != sizeof(*resp))
> +			return -EIO;
> +
> +		memcpy(resp, ctx->rx_buf, sizeof(*resp));
> +	}
> +	return 0;
> +}

[Severity: Medium]
Should rtpse_mcu_uart_recv() apply rtpse_mcu_resp_is_final() before
accepting a frame?  The req argument is unused here, so the first complete
12-byte frame becomes the transaction result no matter what it contains.

realtek-pse-mcu.h describes one of those frames as transient:

  /*
   * MCU status opcodes (seen on the Gen1 dialect; Gen2 never emits them).
   * INCOMPLETE/BAD_CSUM are terminal; NOT_READY is transient.
   */
  #define RTPSE_MCU_OPCODE_NOT_READY		0xff	/* -EAGAIN  */

and the I2C transport keeps polling until the acceptance predicate holds,
so a NOT_READY frame never escapes the transaction:

  drivers/net/pse-pd/realtek-pse-mcu-i2c.c:rtpse_mcu_i2c_smbus_recv() {
	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
		...
		if (ret == RTPSE_MCU_MSG_SIZE && rtpse_mcu_resp_is_final(req, resp))
			return 0;
	}
	return -ETIMEDOUT;
  }

With the UART transport, a NOT_READY frame is copied into resp and
rtpse_mcu_do_xfer() turns it into an error for the caller:

  drivers/net/pse-pd/realtek-pse-mcu-core.c:rtpse_mcu_do_xfer() {
	case RTPSE_MCU_OPCODE_NOT_READY:
		return -EAGAIN;
  }

Is any runtime caller prepared for that?  The only place -EAGAIN is retried
is the probe-time boot loop in rtpse_mcu_discover(); the pse_controller_ops
paths propagate it, so an ethtool PSE get/set would fail.

Note that "realtek,pse-mcu-gen1" is in rtpse_mcu_uart_of_match[], which is
the dialect documented as emitting these status opcodes.

Can the real reply also be lost in that case?  Once rx_len is 12 the
receive callback computes take == 0 and drops the following bytes; if they
instead arrive after the next rtpse_mcu_uart_send() has reset rx_len to 0
(the core only sleeps RTPSE_MCU_RESPONSE_MS = 25 ms between send and recv),
they are consumed as the next transaction's response and rejected by:

  if (resp->opcode != req->opcode ||
      resp->seq_num != req->seq_num ||
      resp->checksum != rtpse_mcu_checksum((u8 *)resp, RTPSE_MCU_MSG_SIZE - 1))
	return -EBADMSG;

failing a second consecutive operation.

Would it make sense to loop on wait_for_completion_timeout() against a
remaining-time budget until rtpse_mcu_resp_is_final(req, resp) holds?  As
written only the first frame is waited for, leaving most of the
RTPSE_MCU_RESPONSE_MAX_MS (1000 ms) receive budget unused.

Re: [PATCH net-next v11 1/4] dt-bindings: net: pse-pd: add bindings for Realtek PSE MCU

[Jonas Jelonek]

On 06.08.26 03:11, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> dt-bindings: net: pse-pd: add bindings for Realtek PSE MCU
>
> This adds a new YAML binding for the MCU that fronts the PSE silicon on a
> range of managed Realtek-based switches, covering both protocol generations
> and the I2C/SMBus and UART attachments.  Two questions below about the file
> naming and about the serial slave node name.
>
>> diff --git a/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml b/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
>> new file mode 100644
>> index 0000000000000..3bb32349c28c4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
>> @@ -0,0 +1,182 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/pse-pd/realtek,pse-mcu-gen1.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> [ ... ]
>
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      # UART
>> +      - items:
>> +          - enum:
>> +              - zyxel,gs1900-10hp-a1-pse
>> +          - const: realtek,pse-mcu-gen1
>> +
>> +      # I2C, SMBus framing
>> +      - items:
>> +          - enum:
>> +              - zyxel,gs1920-24hp-v2-pse
>> +          - const: realtek,pse-mcu-gen1-smbus
>> +
>> +      # UART
>> +      - items:
>> +          - enum:
>> +              - zyxel,gs1900-10hp-b1-pse
>> +              - zyxel,xmg1915-10ep-pse
>> +          - const: realtek,pse-mcu-gen2
>> +
>> +      # I2C, SMBus framing
>> +      - items:
>> +          - enum:
>> +              - zyxel,xs1930-12hp-pse
>> +          - const: realtek,pse-mcu-gen2-smbus
>> +
>> +      # I2C, raw framing
>> +      - items:
>> +          - enum:
>> +              - linksys,lgs328mpc-v2-pse
>> +          - const: realtek,pse-mcu-gen2-i2c
> [Severity: Low]
> This isn't a bug, but should the file be named after the family rather than
> after one compatible?
>
> The file (and $id) is realtek,pse-mcu-gen1.yaml, while the compatible list
> here documents five strings across two protocol generations:
>
>     realtek,pse-mcu-gen1
>     realtek,pse-mcu-gen1-smbus
>     realtek,pse-mcu-gen2
>     realtek,pse-mcu-gen2-smbus
>     realtek,pse-mcu-gen2-i2c
>
> There is no file bearing the name of any realtek,pse-mcu-gen2* compatible,
> so searching by that compatible name finds nothing, and the commit message
> does not mention that gen2 is described in a ...-gen1.yaml file.
>
> Would realtek,pse-mcu.yaml (with the $id updated to match) describe the
> actual scope better?
>
> To be clear, the $id path itself is consistent with the current file path
> (net/pse-pd/realtek,pse-mcu-gen1.yaml#), so $ref resolution and
> dt_binding_check are not affected here - this is only about the declared
> scope of the name.

Could someone please take a decision on this and tell the bot how it
should be? This is being raised again and again... Conor explicitly said
the filename should carry one of the compatibles.


>> +required:
>> +  - compatible
>> +
>> +allOf:
>> +  - $ref: pse-controller.yaml#
> [ ... ]
>
>> +    else:
>> +      allOf:
>> +        - $ref: /schemas/serial/serial-peripheral-props.yaml#
>> +
>> +      properties:
>> +        reg: false
> [Severity: Low]
> This else branch declares the bare gen compatibles to be UART (serial
> slave) attachments, and the node name is forced to ethernet-pse by
> pse-controller.yaml:
>
> Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml
>   $nodename:
>     pattern: "^ethernet-pse(@.*|-([0-9]|[1-9][0-9]+))?$"
>
> But serial.yaml only enumerates these serial slave child node names:
>
> Documentation/devicetree/bindings/serial/serial.yaml
> patternProperties:
>   "^(bluetooth|bluetooth-gnss|embedded-controller|gnss|gps|mcu|onewire)$":
>     if:
>       type: object
>     then:
>       additionalProperties: true
>       $ref: serial-peripheral-props.yaml#
>       ...
>       required:
>         - compatible
>
> Does a board device tree with an ethernet-pse child of serial then get the
> shared slave-node subschema applied at all?  Since serial.yaml ends with
> additionalProperties: true, the unmatched child looks like it just falls
> through silently rather than being flagged.

It gets applied since it's referenced in my bindings. It just doesn't use the
"routing" present in serial.yaml.

> Should serial.yaml's pattern list gain ethernet-pse as part of this series,

I would avoid this since it adds no value in what is validated. From what
I've seen the pattern in serial.yaml isn't exhaustive so other bindings
like "qca,qca7000" also use a node name not being in that list.

> or is the intent to reuse the existing generic mcu name (which would
> conflict with pse-controller.yaml's $nodename pattern)?

No, reason already given in ( ).

> [ ... ]
>
>> +  # UART attachment
>> +  - |
>> +    serial {
>> +        ethernet-pse {
>> +            compatible = "zyxel,gs1900-10hp-a1-pse", "realtek,pse-mcu-gen1";
>> +            current-speed = <19200>;
> [ ... ]


Regards,
Jonas

Re: [PATCH net-next v11 2/4] net: pse-pd: add Realtek PSE MCU core

[Jonas Jelonek]

On 06.08.26 03:11, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: pse-pd: add Realtek PSE MCU core
>
> This adds the shared core for the Realtek PSE MCU driver: the 12-byte
> request/response protocol with sum-mod-256 checksum, the per-dialect opcode
> tables and response parser hooks for the two protocol generations, and the
> pse_controller_ops glue plus a registration helper for the transport modules.
>
>> diff --git a/drivers/net/pse-pd/realtek-pse-mcu-core.c b/drivers/net/pse-pd/realtek-pse-mcu-core.c
>> new file mode 100644
>> index 0000000000000..ed3b743470dbf
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/realtek-pse-mcu-core.c
>> @@ -0,0 +1,988 @@
> [ ... ]
>
>> +	/*
>> +	 * Explicit MCU error opcodes (observed on the Gen1 dialect; harmless
>> +	 * to check for Gen2 too). Catch these before the generic opcode/CRC
>> +	 * mismatch path so callers see a meaningful errno.
>> +	 */
>> +	switch (resp->opcode) {
>> +	case RTPSE_MCU_OPCODE_INCOMPLETE:
>> +		return -EBADE;
>> +	case RTPSE_MCU_OPCODE_BAD_CSUM:
>> +		return -EBADMSG;
>> +	case RTPSE_MCU_OPCODE_NOT_READY:
>> +		return -EAGAIN;
>> +	}
>> +
>> +	if (resp->opcode != req->opcode ||
>> +	    resp->seq_num != req->seq_num ||
>> +	    resp->checksum != rtpse_mcu_checksum((u8 *)resp, RTPSE_MCU_MSG_SIZE - 1))
>> +		return -EBADMSG;
>> +
>> +	return 0;
>> +}
> [Severity: Low]
> Does this validation in rtpse_mcu_do_xfer() accept an all-zero frame as a
> valid reply?
>
> For a request with opcode 0x00 and seq_num 0x00, an all-zero 12-byte
> response satisfies every term:
>
>   resp->opcode  == 0 == req->opcode
>   resp->seq_num == 0 == req->seq_num
>   rtpse_mcu_checksum() over eleven 0x00 bytes == 0 == resp->checksum
>
> Both dialects use 0x00 for a real command:
>
>   rtpse_mcu_dialect_gen1: [RTPSE_MCU_CMD_PORT_ENABLE]      = RTPSE_MCU_OP(0x00)
>   rtpse_mcu_dialect_gen2: [RTPSE_MCU_CMD_SET_GLOBAL_STATE] = RTPSE_MCU_OP(0x00)
>
> and seq_num wraps back to 0x00 every 256 transactions since pse->seq is a u8
> incremented unconditionally in rtpse_mcu_do_xfer():
>
>   req->seq_num = pse->seq++;
>
> Would it be worth adding a discriminator here, for instance rejecting a frame
> whose bytes are all zero, or reserving seq_num 0?

done in next version.

>> +static int rtpse_mcu_port_cmd(struct rtpse_mcu_ctrl *pse, unsigned int port, u8 opcode, u8 arg)
>> +{
>> +	struct rtpse_mcu_msg req, resp;
>> +	int ret;
>> +
>> +	rtpse_mcu_msg_init(&req, opcode);
>> +	req.payload[0] = port;
>> +	req.payload[1] = arg;
>> +
>> +	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (resp.payload[0] != port || resp.payload[1] != 0)
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
> [Severity: Low]
> Related to the above: does this echo check catch the all-zero frame for port
> 0? An all-zero payload gives payload[0] == 0 == port and payload[1] == 0, so
> a Gen1 port-0 enable/disable would be reported as successful even though the
> frame carried no MCU data. rtpse_mcu_set_global_state() has the same shape,
> it only requires resp.payload[0] == 0x0.
>
> The same hole also shortens the poll in the I2C transport, since
> rtpse_mcu_resp_is_final() declares an all-zero read final for opcode 0x00 /
> seq_num 0x00 and the loop stops instead of waiting for the genuine reply.

done in next version.

> [ ... ]
>
>> +	/*
>> +	 * Release the MCU from reset before the first transaction; the
>> +	 * boot-retry loop in discover() waits for it to answer.
>> +	 */
>> +	gpiod = devm_gpiod_get_optional(pse->dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(gpiod))
>> +		return dev_err_probe(pse->dev, PTR_ERR(gpiod),
>> +				     "failed to get reset gpio\n");
>> +
>> +	ret = rtpse_mcu_discover(pse, &info);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Some boards gate all ports through a hardware line; deassert it only
>> +	 * once the MCU is confirmed present, so a failed probe leaves the ports
>> +	 * gated. From here the MCU protocol governs per-port admin state.
>> +	 */
>> +	gpiod = devm_gpiod_get_optional(pse->dev, "disable-ports", GPIOD_OUT_LOW);
>> +	if (IS_ERR(gpiod))
>> +		return dev_err_probe(pse->dev, PTR_ERR(gpiod),
>> +				     "failed to get disable-ports gpio\n");
> [Severity: Medium]
> Does the comment's promise hold for failures after this point?
>
> rtpse_mcu_register() drives disable-ports low here, but nothing re-asserts it
> on the error paths that follow. The devres unwind only releases the
> descriptor; gpiod_free_commit() in drivers/gpio/gpiolib.c clears descriptor
> flags, drops the label and optionally calls gc->free(), it does not change the
> driven output level:
>
>   if (guard.gc && test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
>           if (guard.gc->free)
>                   guard.gc->free(guard.gc, gpiod_hwgpio(desc));
>           clear_bit(GPIOD_FLAG_ACTIVE_LOW, &flags);
>
> So if devm_add_action_or_reset() or devm_pse_controller_register() below fails
> (the latter propagates -EINVAL/-ENODEV from of_load_pse_pis() for a
> missing or mismatched pse-pis node, and -ENOMEM from the kfifo/devres
> allocations), the board is left with all ports ungated and no driver bound.
> The same applies on unbind or module unload.
>
> Since the level also persists past the descriptor's lifetime, doesn't a
> subsequent probe attempt start with the gate already open? That is exactly the
> case the comment guards against: if rtpse_mcu_discover() then fails on a
> re-probe, the ports are ungated while the MCU is unresponsive and unmanaged.
>
> Would a devm action that re-asserts the line (and the reset line) fit here?

Not added, possible failure here is seen less critical then having kill all PoE ports
in case anything in the probe might get wrong. This would even be the case if the
GPIO is asserted by default after reset, outside of the driver, and asserting this
would make PoE non-functional though the probe failure might not be critical.

>> +	if (!info.system_enable) {
>> +		ret = rtpse_mcu_set_global_state(pse, true);
>> +		/* Dialects without a global-state concept (e.g. Gen1) return
>> +		 * -EOPNOTSUPP; treat that as "no separate enable required".
>> +		 */
>> +		if (ret && ret != -EOPNOTSUPP)
>> +			return dev_err_probe(pse->dev, ret,
>> +					     "failed to enable PSE system\n");
>> +		if (!ret) {
>> +			ret = devm_add_action_or_reset(pse->dev,
>> +						       rtpse_mcu_global_disable, pse);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
> [Severity: Medium]
> Is there any compensating teardown for the Gen1 case? rtpse_mcu_dialect_gen1
> has no RTPSE_MCU_CMD_SET_GLOBAL_STATE entry, so
> rtpse_mcu_set_global_state() returns -EOPNOTSUPP and no devm action is
> registered at all. The same happens on boards that already report
> info.system_enable, where the whole block is skipped. For those boards
> nothing disables power or re-gates the ports on a later probe failure or on
> unbind.

IMO the driver shouldn't mess with what it hasn't touched before, so no teardown
if nothing has been activated/enabled before.

> [ ... ]
>
>> diff --git a/drivers/net/pse-pd/realtek-pse-mcu.h b/drivers/net/pse-pd/realtek-pse-mcu.h
>> new file mode 100644
>> index 0000000000000..52f9442d7117c
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/realtek-pse-mcu.h
>> @@ -0,0 +1,93 @@
> [ ... ]
>
>> +/*
>> + * A polling transport can stop here: the reply to this request (opcode and
>> + * seq_num), or a terminal error. The seq_num rejects a stale reply left by an
>> + * abandoned transaction, instead of desyncing by one frame.
>> + */
>> +static inline bool rtpse_mcu_resp_is_final(const struct rtpse_mcu_msg *req,
>> +					   const struct rtpse_mcu_msg *resp)
>> +{
>> +	return (resp->opcode == req->opcode && resp->seq_num == req->seq_num) ||
>> +	       resp->opcode == RTPSE_MCU_OPCODE_INCOMPLETE ||
>> +	       resp->opcode == RTPSE_MCU_OPCODE_BAD_CSUM;
>> +}
> [Severity: Low]
> This is the transport-side half of the same question: for a request with
> opcode 0x00 and seq_num 0x00, an all-zero read is declared final here, so
> rtpse_mcu_i2c_smbus_recv() / rtpse_mcu_i2c_native_recv() stop polling rather
> than retrying for the real frame. Should this helper also reject a frame that
> is entirely zero?

Covered by the fix not allowing seq_num to be 0.


Regards,
Jonas

Re: [PATCH net-next v11 3/4] net: pse-pd: realtek-pse-mcu: add I2C transport

[Jonas Jelonek]

On 06.08.26 03:11, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: pse-pd: realtek-pse-mcu: add I2C transport
>
> This adds the I2C/SMBus transport for the Realtek PSE MCU core, providing
> the send/recv callbacks used to exchange the 12-byte MCU frames. Two
> framings are supported and selected by the compatible: SMBus block
> transfers by default, and raw I2C when the match data sets native_i2c.
>
>> diff --git a/drivers/net/pse-pd/realtek-pse-mcu-i2c.c b/drivers/net/pse-pd/realtek-pse-mcu-i2c.c
>> new file mode 100644
>> index 0000000000000..e10f8c633b68e
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/realtek-pse-mcu-i2c.c
>> @@ -0,0 +1,170 @@
> [ ... ]
>
>> +static int rtpse_mcu_i2c_smbus_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
>> +{
>> +	struct i2c_client *client = to_i2c_client(pse->dev);
>> +
>> +	/* Send opcode as SMBus command byte; remaining 11 bytes as block data */
>> +	return i2c_smbus_write_i2c_block_data(client, req->opcode, RTPSE_MCU_MSG_SIZE - 1,
>> +					      (u8 *)req + 1);
>> +}
>> +
>> +static int rtpse_mcu_i2c_smbus_recv(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req,
>> +				    struct rtpse_mcu_msg *resp)
>> +{
>> +	struct i2c_client *client = to_i2c_client(pse->dev);
>> +	int tries, ret;
>> +
>> +	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
>> +		if (tries > 0)
>> +			msleep(RTPSE_MCU_I2C_RETRY_MS);
>> +
>> +		/* MCU needs 0x00 as command byte for read */
>> +		ret = i2c_smbus_read_i2c_block_data(client, 0x00,
>> +						    RTPSE_MCU_MSG_SIZE,
>> +						    (u8 *)resp);
> [ ... ]
>
>> +static int rtpse_mcu_i2c_native_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
>> +{
>> +	struct i2c_client *client = to_i2c_client(pse->dev);
>> +	void *buf;
>> +	int ret;
>> +
>> +	/* i2c_master_send() may DMA, so the buffer must not be on the stack. */
>> +	buf = kmemdup(req, RTPSE_MCU_MSG_SIZE, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	ret = i2c_master_send(client, buf, RTPSE_MCU_MSG_SIZE);
>> +	kfree(buf);
> [Severity: Low]
> Is the premise in this comment correct?  i2c_master_send() and
> i2c_master_recv() both go through i2c_transfer_buffer_flags(), which
> builds the message as:
>
> drivers/i2c/i2c-core-base.c:i2c_transfer_buffer_flags() {
> 	struct i2c_msg msg = {
> 		.addr = client->addr,
> 		.flags = flags | (client->flags & I2C_M_TEN),
> 		.len = count,
> 		.buf = buf,
> 	};
> 	...
> }
>
> I2C_M_DMA_SAFE is never set there, so an adapter that wants to DMA has to
> obtain its buffer from i2c_get_dma_safe_msg_buf(), which bounces again
> exactly because the flag is absent:
>
> drivers/i2c/i2c-core-base.c:i2c_get_dma_safe_msg_buf() {
> 	...
> 	if (msg->flags & I2C_M_DMA_SAFE)
> 		return msg->buf;
> 	...
> 	if (msg->flags & I2C_M_RD)
> 		return kzalloc(msg->len, GFP_KERNEL);
> 	else
> 		return kmemdup(msg->buf, msg->len, GFP_KERNEL);
> }
>
> So does the kmemdup() here actually achieve anything?  On a DMA-capable
> adapter the 12-byte frame is copied a second time by the core, and the
> core's on-stack struct rtpse_mcu_msg was already legal for these APIs
> (Documentation/i2c/dma-considerations.rst: "it is *not* mandatory that the
> buffer of an I2C message is DMA safe").
>
> The SMBus path in the same file already passes the caller-supplied stack
> req/resp straight into i2c_smbus_write_i2c_block_data() and
> i2c_smbus_read_i2c_block_data(), which seems to contradict the rationale
> used for the raw-I2C path.
>
> The same question applies to rtpse_mcu_i2c_native_recv() below, where the
> kmalloc() plus the memcpy(resp, buf, RTPSE_MCU_MSG_SIZE) duplicate what
> i2c_get_dma_safe_msg_buf()/i2c_put_dma_safe_msg_buf() already do for a
> non-DMA-safe read buffer.  It also adds an -ENOMEM abort to a PSE control
> transaction that the bus core could otherwise always complete.
>
> If skipping the core's bounce is really the goal, should these buffers be
> handed to i2c_master_send_dmasafe()/i2c_master_recv_dmasafe(), or to an
> explicit i2c_msg with I2C_M_DMA_SAFE?  Otherwise, could the allocations
> and both comments simply be dropped?
>
> The commit message carries the same statement:
>
>   "Because i2c_master_send()/i2c_master_recv() may DMA, the raw-I2C path
>   bounces each frame through a heap buffer rather than the core's stack
>   buffers; the SMBus path is unaffected."
>
> Could this be reworded so it does not document a DMA rule that the I2C
> core does not impose?

I'll drop the DMA stuff again, apparently the other AI bot suggesting this
fooled me.

>> +	if (ret < 0)
>> +		return ret;
>> +	return ret == RTPSE_MCU_MSG_SIZE ? 0 : -EIO;
>> +}
>> +
>> +static int rtpse_mcu_i2c_native_recv(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req,
>> +				     struct rtpse_mcu_msg *resp)
>> +{
>> +	struct i2c_client *client = to_i2c_client(pse->dev);
>> +	int tries, ret;
>> +	u8 *buf;
>> +
>> +	/* i2c_master_recv() may DMA, so read into an off-stack buffer. */
>> +	buf = kmalloc(RTPSE_MCU_MSG_SIZE, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
>> +		if (tries > 0)
>> +			msleep(RTPSE_MCU_I2C_RETRY_MS);
>> +
>> +		ret = i2c_master_recv(client, buf, RTPSE_MCU_MSG_SIZE);
>> +		if (ret < 0)
>> +			goto out;
>> +		if (ret == RTPSE_MCU_MSG_SIZE) {
>> +			memcpy(resp, buf, RTPSE_MCU_MSG_SIZE);
> [ ... ]


Regards,
Jonas

Re: [PATCH net-next v11 4/4] net: pse-pd: realtek-pse-mcu: add UART transport

[Jonas Jelonek]

On 06.08.26 03:11, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: pse-pd: realtek-pse-mcu: add UART transport
>
> This adds a serdev (UART) transport for the Realtek PSE MCU core, with
> send/recv callbacks that exchange the 12-byte frames and an asynchronous
> receive_buf callback. The baud rate defaults to 19200 and can be overridden
> with the "current-speed" property.
>
>> diff --git a/drivers/net/pse-pd/realtek-pse-mcu-uart.c b/drivers/net/pse-pd/realtek-pse-mcu-uart.c
>> new file mode 100644
>> index 0000000000000..9baa17d8d31fa
>> --- /dev/null
>> +++ b/drivers/net/pse-pd/realtek-pse-mcu-uart.c
>> @@ -0,0 +1,164 @@
> [ ... ]
>
>> +/*
>> + * No framing is done here: a glitched frame costs one transaction, then
>> + * the next _send re-frames from rx_len 0. Resync works by returning count
>> + * (not take), dropping any overflow so serdev keeps no leftover to bleed
>> + * into the next frame.
>> + */
>> +static size_t rtpse_mcu_uart_receive(struct serdev_device *serdev,
>> +				     const u8 *buf, size_t count)
>> +{
>> +	struct rtpse_mcu_uart *ctx = serdev_device_get_drvdata(serdev);
>> +	size_t take;
>> +
>> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
>> +		take = min(count, sizeof(ctx->rx_buf) - ctx->rx_len);
>> +		if (take) {
>> +			memcpy(ctx->rx_buf + ctx->rx_len, buf, take);
>> +			ctx->rx_len += take;
>> +			if (ctx->rx_len == sizeof(ctx->rx_buf))
>> +				complete(&ctx->rx_done);
>> +		}
>> +	}
>> +
>> +	/* consume all to avoid desync/misalignment */
>> +	return count;
>> +}
> [ ... ]
>
>> +static int rtpse_mcu_uart_send(struct rtpse_mcu_ctrl *pse, const struct rtpse_mcu_msg *req)
>> +{
>> +	struct rtpse_mcu_uart *ctx = to_rtpse_mcu_uart(pse);
>> +	int written;
>> +
>> +	/* clear any leftover rx state before transmitting */
>> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
>> +		reinit_completion(&ctx->rx_done);
>> +		ctx->rx_len = 0;
>> +	}
> [ ... ]
>
>> +static int rtpse_mcu_uart_recv(struct rtpse_mcu_ctrl *pse,
>> +			       const struct rtpse_mcu_msg *req,
>> +			       struct rtpse_mcu_msg *resp)
>> +{
>> +	struct rtpse_mcu_uart *ctx = to_rtpse_mcu_uart(pse);
>> +
>> +	if (!wait_for_completion_timeout(&ctx->rx_done, RTPSE_MCU_UART_RX_TIMEOUT))
>> +		return -ETIMEDOUT;
>> +
>> +	scoped_guard(spinlock_irqsave, &ctx->rx_lock) {
>> +		if (ctx->rx_len != sizeof(*resp))
>> +			return -EIO;
>> +
>> +		memcpy(resp, ctx->rx_buf, sizeof(*resp));
>> +	}
>> +	return 0;
>> +}
> [Severity: Medium]
> Should rtpse_mcu_uart_recv() apply rtpse_mcu_resp_is_final() before
> accepting a frame?  The req argument is unused here, so the first complete
> 12-byte frame becomes the transaction result no matter what it contains.
>
> realtek-pse-mcu.h describes one of those frames as transient:
>
>   /*
>    * MCU status opcodes (seen on the Gen1 dialect; Gen2 never emits them).
>    * INCOMPLETE/BAD_CSUM are terminal; NOT_READY is transient.
>    */
>   #define RTPSE_MCU_OPCODE_NOT_READY		0xff	/* -EAGAIN  */
>
> and the I2C transport keeps polling until the acceptance predicate holds,
> so a NOT_READY frame never escapes the transaction:
>
>   drivers/net/pse-pd/realtek-pse-mcu-i2c.c:rtpse_mcu_i2c_smbus_recv() {
> 	for (tries = 0; tries < RTPSE_MCU_I2C_MAX_TRIES; tries++) {
> 		...
> 		if (ret == RTPSE_MCU_MSG_SIZE && rtpse_mcu_resp_is_final(req, resp))
> 			return 0;
> 	}
> 	return -ETIMEDOUT;
>   }
>
> With the UART transport, a NOT_READY frame is copied into resp and
> rtpse_mcu_do_xfer() turns it into an error for the caller:
>
>   drivers/net/pse-pd/realtek-pse-mcu-core.c:rtpse_mcu_do_xfer() {
> 	case RTPSE_MCU_OPCODE_NOT_READY:
> 		return -EAGAIN;
>   }
>
> Is any runtime caller prepared for that?  The only place -EAGAIN is retried
> is the probe-time boot loop in rtpse_mcu_discover(); the pse_controller_ops
> paths propagate it, so an ethtool PSE get/set would fail.
>
> Note that "realtek,pse-mcu-gen1" is in rtpse_mcu_uart_of_match[], which is
> the dialect documented as emitting these status opcodes.
>
> Can the real reply also be lost in that case?  Once rx_len is 12 the
> receive callback computes take == 0 and drops the following bytes; if they
> instead arrive after the next rtpse_mcu_uart_send() has reset rx_len to 0
> (the core only sleeps RTPSE_MCU_RESPONSE_MS = 25 ms between send and recv),
> they are consumed as the next transaction's response and rejected by:
>
>   if (resp->opcode != req->opcode ||
>       resp->seq_num != req->seq_num ||
>       resp->checksum != rtpse_mcu_checksum((u8 *)resp, RTPSE_MCU_MSG_SIZE - 1))
> 	return -EBADMSG;
>
> failing a second consecutive operation.
>
> Would it make sense to loop on wait_for_completion_timeout() against a
> remaining-time budget until rtpse_mcu_resp_is_final(req, resp) holds?  As
> written only the first frame is waited for, leaving most of the
> RTPSE_MCU_RESPONSE_MAX_MS (1000 ms) receive budget unused.

Fixed in next version - uart_recv() now loops wait_for_completion_timeout()
against the remaining budget until resp_is_final(req, resp) holds, dropping
non-final frames (transient NOT_READY, or a stale frame from a prior
timeout), so it uses the full RESPONSE_MAX_MS and no longer surfaces a
transient frame as the transaction result.

Regards,
Jonas