| Message ID | 20231201-feature_poe-v2-0-56d8cac607fa@bootlin.com |
|---|---|
| Headers | show |
| Series | net: Add support for Power over Ethernet (PoE) | expand |
> +/** > + * enum - Types of PSE controller. > + * > + * @PSE_UNKNOWN: Type of PSE controller is unknown > + * @PSE_PODL: PSE controller which support PoDL > + * @PSE_C33: PSE controller which support Clause 33 (PoE) > + */ > +enum { > + PSE_UNKNOWN = BIT(0), > + PSE_PODL = BIT(1), > + PSE_C33 = BIT(2), > +}; Maybe this should be in the netlink API? I think you can imply it by looking at what properties are in the netlink reply, but having it explicitly is probably better. ethtool(1) can default to PSE_PODL if the property is missing for an older kernel. Andrew
> @@ -143,6 +150,43 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info) > return -EOPNOTSUPP; > } > > + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > + !tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]) > + return 0; -EINVAL? Is there a real use case for not passing either of them? > + > + if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > + !(pse_get_types(phydev->psec) & PSE_PODL)) { > + NL_SET_ERR_MSG_ATTR(info->extack, > + tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL], > + "setting PSE PoDL admin control not supported"); > + return -EOPNOTSUPP; > + } > + if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL] && > + !(pse_get_types(phydev->psec) & PSE_C33)) { > + NL_SET_ERR_MSG_ATTR(info->extack, > + tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL], > + "setting PSE PoE admin control not supported"); This probably should be C33, not PoE? I guess it depends on what the user space tools are using. Andrew
On Fri, Dec 01, 2023 at 06:10:23PM +0100, Kory Maincent wrote: > In commit 18ff0bcda6d1 ("ethtool: add interface to interact with Ethernet > Power Equipment"), the 'pse_control_config' structure was introduced, > housing a single member labeled 'admin_cotrol' responsible for maintaining > the operational state of the PoDL PSE functions. > > A noticeable typographical error exists in the naming of this field > ('cotrol' should be corrected to 'control'), which this commit aims to > rectify. > > Furthermore, with upcoming extensions of this structure to encompass PoE > functionalities, the field is being renamed to 'podl_admin_state' to > distinctly indicate that this state is tailored specifically for PoDL." > > Sponsored-by: Dent Project <dentproject@linuxfoundation.org> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de> Thx!
Thanks for your review! On Mon, 4 Dec 2023 13:51:31 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > On Fri, Dec 01, 2023 at 06:10:25PM +0100, Kory Maincent wrote: > > +u32 pse_get_types(struct pse_control *psec); > > I would add here some helper function. Something like: > pse_has_podl() or pse_has_c33() Instead of pse_get_types function? It is indeed maybe cleaner to use such helper functions instead. Regards,
Thanks for your review: On Sun, 3 Dec 2023 19:38:04 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > +/** > > + * enum - Types of PSE controller. > > + * > > + * @PSE_UNKNOWN: Type of PSE controller is unknown > > + * @PSE_PODL: PSE controller which support PoDL > > + * @PSE_C33: PSE controller which support Clause 33 (PoE) > > + */ > > +enum { > > + PSE_UNKNOWN = BIT(0), > > + PSE_PODL = BIT(1), > > + PSE_C33 = BIT(2), > > +}; > > Maybe this should be in the netlink API? > > I think you can imply it by looking at what properties are in the > netlink reply, but having it explicitly is probably better. > ethtool(1) can default to PSE_PODL if the property is missing for an > older kernel. Ok, I will add it to netlink API in v3 then. Regards,
On Sun, 3 Dec 2023 19:45:18 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > @@ -143,6 +150,43 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct > > genl_info *info) return -EOPNOTSUPP; > > } > > > > + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > > + !tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]) > > + return 0; > > -EINVAL? Is there a real use case for not passing either of them? No indeed. > > + > > + if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > > + !(pse_get_types(phydev->psec) & PSE_PODL)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, > > + tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL], > > + "setting PSE PoDL admin control not > > supported"); > > + return -EOPNOTSUPP; > > + } > > + if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL] && > > + !(pse_get_types(phydev->psec) & PSE_C33)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, > > + tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL], > > + "setting PSE PoE admin control not > > supported"); > > This probably should be C33, not PoE? > > I guess it depends on what the user space tools are using. Yes, I have hesitated on replacing that one. If you prefer c33 in the log, I will change it in next version Regards,
On Sun, Dec 03, 2023 at 07:45:18PM +0100, Andrew Lunn wrote: > > @@ -143,6 +150,43 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info) > > return -EOPNOTSUPP; > > } > > > > + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > > + !tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]) > > + return 0; > > -EINVAL? Is there a real use case for not passing either of them? > > > + > > + if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL] && > > + !(pse_get_types(phydev->psec) & PSE_PODL)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, > > + tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL], > > + "setting PSE PoDL admin control not supported"); > > + return -EOPNOTSUPP; > > + } > > + if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL] && > > + !(pse_get_types(phydev->psec) & PSE_C33)) { > > + NL_SET_ERR_MSG_ATTR(info->extack, > > + tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL], > > + "setting PSE PoE admin control not supported"); > > This probably should be C33, not PoE? > > I guess it depends on what the user space tools are using. The same problem is in the documentation. Mixing different naming schemes is problematic. Even unmixed this "PoE" is not really suitable for most use cases. Expanding this abbreviations make it probably more clear: - PSE PoE - Power Source Equipment Power over Ethernet - C33 PSE - Clause 33 Power Source Equipment
On Tue, Dec 05, 2023 at 07:36:06AM +0100, Oleksij Rempel wrote:
> CC regulator devs here. PSE is a regulator for network devices :)
Is there some question related to regulators here? I couldn't spot one.
On Tue, 5 Dec 2023 11:15:01 +0100 Köry Maincent <kory.maincent@bootlin.com> wrote: > On Tue, 5 Dec 2023 07:36:06 +0100 > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > I would expect a devicetree like this: > > > > ethernet-pse@3c { > > // controller compatible should be precise > > compatible = "microchip,pd69210"; > > reg = <0x3c>; > > #pse-cells = <1>; > > > > managers { > > manager@0 { > > // manager compatible should be included, since we are > > // able to campare it with communication results > > compatible = "microchip,pd69208t4" > > // addressing corresponding to the chip select addressing > > reg = <0>; > > > > physical-ports { > > phys0: port@0 { > > // each of physical ports is actually a regulator If this phys0 is a regulator, which device will be the consumer of this regulator? log_port0 as the phys0 regulator consumer seems a bit odd! A 8P8C node should be the consumer. > > reg = <0>; > > }; > > phys1: port@1 { > > reg = <1>; > > }; > > phys2: port@2 { > > reg = <2>; > > }; > > > > ... > > } > > > > // port matrix can be calculated by using this information > > logical-ports { > > log_port0: port@0 { > > // PoE4 port > > physical-ports = <&phys0, &phys1>; > > }; > > log_port1: port@1 { > > // PoE2 port > > physical-ports = <&phys2>; > > }; > > }; > > > > .... > > ethernet-phy@1 { > > reg = <1>; > > pses = <&log_port0>; > > } > > ethernet-phy@2 { > > reg = <2>; > > pses = <&log_port1>; > > } In fact if we want to really fit the PoE architecture topology we should wait for the support of 8P8C connector from Maxime. Then it will look like that: SoC --- i2c/uart --> controller -- spi --> manager0 -- phys_port0 --> 8P8C_connector0 (PoE4) | \- phys_port1 --> 8P8C_connector0 (PoE4) | \- phys_port2 --> 8P8C_connector1 (PoE2) | \- phys_port3 --> 8P8C_connector2 (PoE2) \- manager1 -- phys_port0 .. With this type of devicetree: ethernet-pse@3c { // controller compatible should be precise compatible = "microchip,pd69210"; reg = <0x3c>; #pse-cells = <1>; managers { manager@0 { // manager compatible should be included, since we are // able to compare it with communication results compatible = "microchip,pd69208t4" // addressing corresponding to the chip select addressing reg = <0>; physical-ports { phys_port0: port@0 { // each of physical ports is actually a regulator reg = <0>; }; phy_port1: port@1 { reg = <1>; }; phy_port2: port@2 { reg = <2>; }; ... } manager@1 { ... }; }; }; .... rj45_0:8p8c@0 { pses = <&phy_port0, &phy_port1>; }; rj45_1:8p8c@1 { pses = <&phy_port2>; }; ethernet-phy@1 { reg = <1>; connector = <&rj45_0>; }; ethernet-phy@2 { reg = <2>; connector = <&rj45_1>; } The drawback is that I don't really know for now, how port matrix can be calculated with this. Maybe by adding a "conf_pse_cell()" callback, call in the of_pse_control_get() for each ports. For now 8p8c connector are not supported, we could keep using pse phandle in the phy node like you described but for physical port: ethernet-phy@1 { reg = <1>; pses = <&phy_port0, &phy_port1>; } ethernet-phy@2 { reg = <2>; pses = <&phy_port2>; } Finally, the devicetree would not know the matrix between logical port and physical port, this would be cleaner. Did I miss something? Regards,
On Tue, 5 Dec 2023 15:21:47 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > On Tue, Dec 05, 2023 at 02:31:23PM +0100, Köry Maincent wrote: > > On Tue, 5 Dec 2023 11:15:01 +0100 > > Köry Maincent <kory.maincent@bootlin.com> wrote: > > > > > On Tue, 5 Dec 2023 07:36:06 +0100 > > > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > > > > I would expect a devicetree like this: > > > > > > > > ethernet-pse@3c { > > > > // controller compatible should be precise > > > > compatible = "microchip,pd69210"; > > > > reg = <0x3c>; > > > > #pse-cells = <1>; > > > > > > > > managers { > > > > manager@0 { > > > > // manager compatible should be included, since we are > > > > // able to campare it with communication results > > > > compatible = "microchip,pd69208t4" > > > > // addressing corresponding to the chip select addressing > > > > reg = <0>; > > > > > > > > physical-ports { > > > > phys0: port@0 { > > > > // each of physical ports is actually a regulator > > > > If this phys0 is a regulator, which device will be the consumer of this > > regulator? log_port0 as the phys0 regulator consumer seems a bit odd! > > Why? > > > A 8P8C node should be the consumer. > > PHY is not actual consumer of this regulator. State of the Ethernet PHY > is not related to the power supply. We should deliver power independent > of network interface state. There is no other local consumer we can > use in this case. Just to be clear, are you saying we should use the regulator framework or is it simply a way of speaking as it behaves like regulator? > > Finally, the devicetree would not know the matrix between logical port and > > physical port, this would be cleaner. > > > > Did I miss something? > > In case different PSE suppliers are linked withing the PHY node, we > loose most of information needed for PSE functionality. For example how > we will know if our log_port supports PoE4 and PoE2 mode, or only PoE2. > This information is vital for proper PSE configuration, this is why I > suggested to have logica-ports subnodes. With the price of hawing huge > DT on a switch with 48 ports. It could be known in the of_pse_control_get() function if there is two phandles in the "pses" parameter. Then we add a new enum c33_pse_mode member in the pse_control struct to store the mode. PoE2 and PoE4 is not a parameter of the logical port, it depends of the number of PSE ports wired to an 8P8C connector. In fact I am also working on the tps23881 driver which aimed to be added to this series soon. In the tps23881 case the logical port can only be configured to one physical port. Two physical ports (which mean two logical ports) can still be used to have PoE4 mode. For PoE4, in the pd692x0 driver we use one logical port (one pse_control->id) configured to two physical ports but in the tps23881 we will need two logical ports (two pse_control->id). So with the tps23881 driver we will need two phandle in the "pses" parameter to have PoE4, that's why my proposition seems relevant. The same goes with your pse-regulator driver, you can't do PoE4 if two regulators is needed for each two pairs group. Regards,
On Fri, Dec 01, 2023 at 06:10:26PM +0100, Kory Maincent wrote: > @@ -1741,6 +1757,7 @@ Request contents: > ====================================== ====== ============================= > ``ETHTOOL_A_PSE_HEADER`` nested request header > ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` u32 Control PoDL PSE Admin state > + ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state > ====================================== ====== ============================= I get htmldocs warning: ``` Documentation/networking/ethtool-netlink.rst:1760: WARNING: Malformed table. Text in column margin in table line 4. ====================================== ====== ============================= ``ETHTOOL_A_PSE_HEADER`` nested request header ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` u32 Control PoDL PSE Admin state ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state ====================================== ====== ============================= ``` I have to fix it up: ---- >8 ---- diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst index e02a7dabc673e2..8da5068105e3e9 100644 --- a/Documentation/networking/ethtool-netlink.rst +++ b/Documentation/networking/ethtool-netlink.rst @@ -1757,7 +1757,7 @@ Request contents: ====================================== ====== ============================= ``ETHTOOL_A_PSE_HEADER`` nested request header ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` u32 Control PoDL PSE Admin state - ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state + ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` u32 Control PSE Admin state ====================================== ====== ============================= When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is used Thanks.
This patch series aims at adding support for PoE (Power over Ethernet), based on the already existing support for PoDL (Power over Data Line) implementation. In addition, it adds support for one specific PoE controller, the Microchip PD692x0. The PD692x0 driver is based on the patch merged in an immutable branch from Jakub repo. It is Tagged at: git://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git firmware_loader-add-upload-error The patch is already merged in net-next. In detail: - Patch 1 to 6 prepare net to support PoE devices. - Patch 7 and 8 add PD692x0 PoE PSE controller driver and its binding. Changes in v2: - Extract "firmware_loader: Expand Firmware upload error codes patches" to send it alone and get it merge in an immutable branch. - Add "c33" prefix for PoE variables and enums. - Enhance few comments. - Add PSE Documentation. - Make several changes in pd692x0 driver, mainly for readibility. - Link to v1: https://lore.kernel.org/r/20231116-feature_poe-v1-0-be48044bf249@bootlin.com Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> --- Kory Maincent (8): net: pse-pd: Rectify and adapt the naming of admin_cotrol member of struct pse_control_config ethtool: Expand Ethernet Power Equipment with c33 (PoE) alongside PoDL net: pse-pd: Introduce PSE types enumeration net: ethtool: pse-pd: Expand pse commands with the PSE PoE interface netlink: specs: Modify pse attribute prefix netlink: specs: Expand the pse netlink command with PoE interface dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller net: pse-pd: Add PD692x0 PSE controller driver .../bindings/net/pse-pd/microchip,pd692x0.yaml | 77 ++ Documentation/netlink/specs/ethtool.yaml | 33 +- Documentation/networking/ethtool-netlink.rst | 20 + Documentation/networking/pse-pd/introduction.rst | 73 ++ MAINTAINERS | 7 + drivers/net/pse-pd/Kconfig | 11 + drivers/net/pse-pd/Makefile | 1 + drivers/net/pse-pd/pd692x0.c | 1025 ++++++++++++++++++++ drivers/net/pse-pd/pse_core.c | 9 + drivers/net/pse-pd/pse_regulator.c | 9 +- include/linux/pse-pd/pse.h | 35 +- include/uapi/linux/ethtool.h | 43 + include/uapi/linux/ethtool_netlink.h | 3 + net/ethtool/pse-pd.c | 64 +- tools/net/ynl/generated/ethtool-user.c | 54 +- tools/net/ynl/generated/ethtool-user.h | 81 +- 16 files changed, 1481 insertions(+), 64 deletions(-) --- base-commit: 98137c429a4854583210707a82114b4f5c171c5e change-id: 20231024-feature_poe-139490e73403 Best regards,