| Message ID | 20240227-feature_poe-v5-0-28f0aa48246d@bootlin.com |
|---|---|
| Headers | show |
| Series | net: Add support for Power over Ethernet (PoE) | expand |
On Tue, Feb 27, 2024 at 9:43 AM Kory Maincent <kory.maincent@bootlin.com> wrote: > > This patch series aims at adding support for PoE (Power over Ethernet), > based on the already existing support for PoDL (Power over Data Line) > implementation. In addition, it adds support for two specific PoE > controller, the Microchip PD692x0 and the TI TPS23881. > > This patch series is sponsored by Dent Project > <dentproject@linuxfoundation.org>. Sorry, couldnt resist because it sounded like a commercial;-> And likely i am out of touch. I am all for giving credit but does it have to be explicitly called out as "this patch is sponsored by X"? cheers, jamal > In detail: > - Patch 1 to 13 prepare net to support PoE devices. > - Patch 14 and 15 add PD692x0 PoE PSE controller driver and its binding. > - Patch 16 and 17 add TI TPS23881 PSE controller driver and its binding. > > Changes in v5: > - Fix bindings nit. > - Add supported-polarity parameter to bindings. > - Fix yamllint binding errors. > - Remove the nested lock brought by the use of regulator framework. > - Link to v4: https://lore.kernel.org/r/20240215-feature_poe-v4-0-35bb4c23266c@bootlin.com > > Changes in v4: > - Replaced sponsored-by tag by a simple sentence. > - Fix pse_pi node bindings. > - Add pse pi documentation written by Oleksij. > - Link to v3: https://lore.kernel.org/r/20240208-feature_poe-v3-0-531d2674469e@bootlin.com > > Changes in v3: > - Add patches to add Oleksij and myself to PSE MAINTAINERS. > - Add patches to add pse devlink. > - Add TI TPS23881 PSE controller driver with its binding. > - Replace pse_get_types helper by pse_has_podl and pse_has_c33 > - Changed the PSE core bindings. > - Add a setup_pi_matrix callback. > - Register regulator for each PSE PI (Power Interface). > - Changed the PD692x0 bindings. > - Updated PD692x0 drivers to new bindings and PSE PI description. > - Updated PD692x0 drivers according to the reviews and made fixes. > - Link to v2: https://lore.kernel.org/r/20231201-feature_poe-v2-0-56d8cac607fa@bootlin.com > > 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 (17): > MAINTAINERS: net: Add Oleksij to pse-pd maintainers > of: property: Add fw_devlink support for pse parent > 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 > MAINTAINERS: Add myself to pse networking maintainer > net: pse-pd: Add support for PSE PIs > dt-bindings: net: pse-pd: Add another way of describing several PSE PIs > net: pse-pd: Add support for setup_pi_matrix callback > net: pse-pd: Use regulator framework within PSE framework > dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller > net: pse-pd: Add PD692x0 PSE controller driver > dt-bindings: net: pse-pd: Add bindings for TPS23881 PSE controller > net: pse-pd: Add TI TPS23881 PSE controller driver > > .../bindings/net/pse-pd/microchip,pd692x0.yaml | 158 +++ > .../bindings/net/pse-pd/pse-controller.yaml | 100 +- > .../bindings/net/pse-pd/ti,tps23881.yaml | 93 ++ > Documentation/netlink/specs/ethtool.yaml | 33 +- > Documentation/networking/ethtool-netlink.rst | 20 + > Documentation/networking/index.rst | 1 + > Documentation/networking/pse-pd/index.rst | 10 + > Documentation/networking/pse-pd/introduction.rst | 73 ++ > Documentation/networking/pse-pd/pse-pi.rst | 302 +++++ > MAINTAINERS | 8 + > drivers/net/mdio/fwnode_mdio.c | 29 +- > drivers/net/pse-pd/Kconfig | 20 + > drivers/net/pse-pd/Makefile | 2 + > drivers/net/pse-pd/pd692x0.c | 1223 ++++++++++++++++++++ > drivers/net/pse-pd/pse_core.c | 429 ++++++- > drivers/net/pse-pd/pse_regulator.c | 49 +- > drivers/net/pse-pd/tps23881.c | 818 +++++++++++++ > drivers/of/property.c | 2 + > include/linux/pse-pd/pse.h | 86 +- > include/uapi/linux/ethtool.h | 55 + > include/uapi/linux/ethtool_netlink.h | 3 + > net/ethtool/pse-pd.c | 60 +- > 22 files changed, 3451 insertions(+), 123 deletions(-) > --- > base-commit: f308eae1e1cdacca3cef65c7f4f691dfcb0c8976 > change-id: 20231024-feature_poe-139490e73403 > > Best regards, > -- > Köry Maincent, Bootlin > Embedded Linux and kernel engineering > https://bootlin.com > >
On Tue, 27 Feb 2024 15:42:55 +0100 Kory Maincent wrote: > + psec->ps = devm_regulator_get_exclusive(dev, > + rdev_get_name(pcdev->pi[index].rdev)); > + if (IS_ERR(psec->ps)) { > + kfree(psec); > + return ERR_CAST(psec->ps); coccinelle says: ERROR: reference preceded by free on line 458
On Tue, Feb 27, 2024 at 03:42:55PM +0100, Kory Maincent wrote: ... > diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h > index 522115cc6cef..a3e297cc2150 100644 > --- a/include/linux/pse-pd/pse.h > +++ b/include/linux/pse-pd/pse.h > @@ -55,10 +55,10 @@ struct pse_controller_ops { > int (*ethtool_get_status)(struct pse_controller_dev *pcdev, > unsigned long id, struct netlink_ext_ack *extack, > struct pse_control_status *status); > - int (*ethtool_set_config)(struct pse_controller_dev *pcdev, > - unsigned long id, struct netlink_ext_ack *extack, > - const struct pse_control_config *config); > int (*setup_pi_matrix)(struct pse_controller_dev *pcdev); > + int (*pi_is_enabled)(struct pse_controller_dev *pcdev, int id); > + int (*pi_enable)(struct pse_controller_dev *pcdev, int id); > + int (*pi_disable)(struct pse_controller_dev *pcdev, int id); Hi Kory, Please update the Kernel doc for struct pse_controller_ops to reflect the added and removed fields. > }; > > struct module; > @@ -90,10 +90,14 @@ struct pse_pi_pairset { > * > * @pairset: table of the PSE PI pinout alternative for the two pairset > * @np: device node pointer of the PSE PI node > + * @rdev: regulator represented by the PSE PI > + * @enabled: PI enabled state > */ > struct pse_pi { > struct pse_pi_pairset pairset[2]; > struct device_node *np; > + struct regulator_dev *rdev; > + bool enabled; > }; > > /** > @@ -107,6 +111,8 @@ struct pse_pi { > * @of_pse_n_cells: number of cells in PSE line specifiers > * @nr_lines: number of PSE controls in this controller device > * @lock: Mutex for serialization access to the PSE controller > + * @lock_owner: current owner of the mutex > + * @ref_cnt: mutex's reference count These newly documented fields don't seem to exist in struct pse_controller_dev. Perhaps this is an left over from earlier development? > * @types: types of the PSE controller > * @pi: table of PSE PIs described in this controller device > * @of_legacy: flag set if the pse_pis devicetree node is not used > @@ -132,7 +138,8 @@ struct device; > int devm_pse_controller_register(struct device *dev, > struct pse_controller_dev *pcdev); > > -struct pse_control *of_pse_control_get(struct device_node *node); > +struct pse_control *of_pse_control_get(struct device *dev, > + struct device_node *node); > void pse_control_put(struct pse_control *psec); > > int pse_ethtool_get_status(struct pse_control *psec, > @@ -147,7 +154,8 @@ bool pse_has_c33(struct pse_control *psec); > > #else > > -static inline struct pse_control *of_pse_control_get(struct device_node *node) > +static inline struct pse_control *of_pse_control_get(struct device *dev, > + struct device_node *node) > { > return ERR_PTR(-ENOENT); > } > > -- > 2.25.1 > >
On Tue, Feb 27, 2024 at 03:42:59PM +0100, Kory Maincent wrote: > Add a new driver for the TI TPS23881 I2C Power Sourcing Equipment > controller. > > This patch is sponsored by Dent Project <dentproject@linuxfoundation.org>. > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> ... > +static int tps23881_flash_fw_part(struct i2c_client *client, > + const char *fw_name, > + const struct tps23881_fw_conf *fw_conf) > +{ > + const struct firmware *fw = NULL; > + int i, ret; > + > + ret = request_firmware(&fw, fw_name, &client->dev); > + if (ret) > + return ret; > + > + dev_info(&client->dev, "Flashing %s\n", fw_name); > + > + /* Prepare device for RAM download */ > + while (fw_conf->reg) { > + ret = i2c_smbus_write_byte_data(client, fw_conf->reg, > + fw_conf->val); > + if (ret < 0) Hi Kory, Should fw be released here. > + return ret; > + > + fw_conf++; > + } > + > + /* Flash the firmware file */ > + for (i = 0; i < fw->size; i++) { > + ret = i2c_smbus_write_byte_data(client, > + TPS23881_REG_SRAM_DATA, > + fw->data[i]); > + if (ret < 0) And here? Flagged by Smatch. > + return ret; > + } > + > + release_firmware(fw); > + > + return 0; > +} ...
On Wed, 28 Feb 2024 12:48:01 +0000 Simon Horman <horms@kernel.org> wrote: > On Tue, Feb 27, 2024 at 03:42:55PM +0100, Kory Maincent wrote: > > ... > > > diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h > > index 522115cc6cef..a3e297cc2150 100644 > > --- a/include/linux/pse-pd/pse.h > > +++ b/include/linux/pse-pd/pse.h > > @@ -55,10 +55,10 @@ struct pse_controller_ops { > > int (*ethtool_get_status)(struct pse_controller_dev *pcdev, > > unsigned long id, struct netlink_ext_ack *extack, > > struct pse_control_status *status); > > - int (*ethtool_set_config)(struct pse_controller_dev *pcdev, > > - unsigned long id, struct netlink_ext_ack *extack, > > - const struct pse_control_config *config); > > int (*setup_pi_matrix)(struct pse_controller_dev *pcdev); > > + int (*pi_is_enabled)(struct pse_controller_dev *pcdev, int id); > > + int (*pi_enable)(struct pse_controller_dev *pcdev, int id); > > + int (*pi_disable)(struct pse_controller_dev *pcdev, int id); > > Hi Kory, > > Please update the Kernel doc for struct pse_controller_ops to reflect the > added and removed fields. Hello Simon, Sorry I totally forgot to update the kernel doc on that patch. Regards,
On Tue, 27 Feb 2024 16:56:59 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 27 Feb 2024 15:42:55 +0100 Kory Maincent wrote: > > + psec->ps = devm_regulator_get_exclusive(dev, > > + > > rdev_get_name(pcdev->pi[index].rdev)); > > + if (IS_ERR(psec->ps)) { > > + kfree(psec); > > + return ERR_CAST(psec->ps); > > coccinelle says: ERROR: reference preceded by free on line 458 Ouch, indeed, missed this error. Regards,
On Wed, 28 Feb 2024 12:53:44 +0000 Simon Horman <horms@kernel.org> wrote: > On Tue, Feb 27, 2024 at 03:42:59PM +0100, Kory Maincent wrote: > > Add a new driver for the TI TPS23881 I2C Power Sourcing Equipment > > controller. > > > > This patch is sponsored by Dent Project <dentproject@linuxfoundation.org>. > > > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> > > ... > > > +static int tps23881_flash_fw_part(struct i2c_client *client, > > + const char *fw_name, > > + const struct tps23881_fw_conf *fw_conf) > > +{ > > + const struct firmware *fw = NULL; > > + int i, ret; > > + > > + ret = request_firmware(&fw, fw_name, &client->dev); > > + if (ret) > > + return ret; > > + > > + dev_info(&client->dev, "Flashing %s\n", fw_name); > > + > > + /* Prepare device for RAM download */ > > + while (fw_conf->reg) { > > + ret = i2c_smbus_write_byte_data(client, fw_conf->reg, > > + fw_conf->val); > > + if (ret < 0) > > Hi Kory, > > Should fw be released here. Hello Simon, Indeed I have missed it, thanks.
Hi Kory, On Tue, Feb 27, 2024 at 03:42:52PM +0100, Kory Maincent wrote: .... > diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c > index fed006cbc185..9124eb3d6492 100644 > --- a/drivers/net/pse-pd/pse_core.c > +++ b/drivers/net/pse-pd/pse_core.c > @@ -27,38 +27,137 @@ struct pse_control { > struct kref refcnt; > }; > > -/** > - * of_pse_zero_xlate - dummy function for controllers with one only control > - * @pcdev: a pointer to the PSE controller device > - * @pse_spec: PSE line specifier as found in the device tree > - * > - * This static translation function is used by default if of_xlate in > - * :c:type:`pse_controller_dev` is not set. It is useful for all PSE > - * controllers with #pse-cells = <0>. > - */ Please replace documentation > -static int of_pse_zero_xlate(struct pse_controller_dev *pcdev, > - const struct of_phandle_args *pse_spec) > +static int of_load_pse_pi_pairsets(struct device_node *node, > + struct pse_pi *pi, > + int npairsets) > { > - return 0; > + struct device_node *pairset_np; > + const char *name; > + int i, ret; > + > + for (i = 0; i < npairsets; i++) { please move this scope to a separate function. > + ret = of_property_read_string_index(node, > + "pairset-names", > + i, &name); > + if (ret) > + break; > + > + if (strcmp(name, "alternative-a")) { strcmp returns 0 if it matching if (!strcmp(name, "alternative-a")) { > + pi->pairset[i].pinout = ALTERNATIVE_A; > + } else if (strcmp(name, "alternative-b")) { if (!strcmp(name, "alternative-a")) { > + pi->pairset[i].pinout = ALTERNATIVE_B; > + } else { > + pr_err("pse: wrong pairset-names value %s\n", name); > + ret = -EINVAL; > + break; > + } > + > + pairset_np = of_parse_phandle(node, "pairsets", i); > + if (!pairset_np) { > + ret = -ENODEV; > + break; > + } > + > + pi->pairset[i].np = pairset_np; > + } > + > + if (i == 2 && pi->pairset[0].pinout == pi->pairset[1].pinout) { > + pr_err("pse: two PI pairsets can not have identical pinout"); > + ret = -EINVAL; > + } > + > + /* If an error appears on the second pairset load, release the first > + * pairset device node kref > + */ > + if (ret) { > + of_node_put(pi->pairset[0].np); > + pi->pairset[0].np = NULL; > + of_node_put(pi->pairset[1].np); > + pi->pairset[1].np = NULL; > + } > + > + return ret; > } > > -/** > - * of_pse_simple_xlate - translate pse_spec to the PSE line number > - * @pcdev: a pointer to the PSE controller device > - * @pse_spec: PSE line specifier as found in the device tree > - * > - * This static translation function is used by default if of_xlate in > - * :c:type:`pse_controller_dev` is not set. It is useful for all PSE > - * controllers with 1:1 mapping, where PSE lines can be indexed by number > - * without gaps. > - */ Please replace documentation > -static int of_pse_simple_xlate(struct pse_controller_dev *pcdev, > - const struct of_phandle_args *pse_spec) > +static int of_load_pse_pis(struct pse_controller_dev *pcdev) > { > - if (pse_spec->args[0] >= pcdev->nr_lines) > - return -EINVAL; > + struct device_node *np = pcdev->dev->of_node; > + struct device_node *node, *pis; > + int ret, i; > > - return pse_spec->args[0]; > + if (!np) > + return -ENODEV; > + > + pcdev->pi = kcalloc(pcdev->nr_lines, sizeof(*pcdev->pi), GFP_KERNEL); > + if (!pcdev->pi) > + return -ENOMEM; > + > + pis = of_get_child_by_name(np, "pse-pis"); > + if (!pis) { Do we need to allocate pcdev->pi if there are no pse-pis? > + /* Legacy OF description of PSE PIs */ > + pcdev->of_legacy = true; It is not "legacy" :) PoDL do not providing definition of PSE PI since there is only one pair. May be: single_pair, no_pse_pi or any other idea. > + return 0; > + } > + > + for_each_child_of_node(pis, node) { please move this scope to a separate function. > + struct pse_pi pi = {0}; > + int npairsets; > + u32 id; > + > + if (!of_node_name_eq(node, "pse-pi")) > + continue; > + > + ret = of_property_read_u32(node, "reg", &id); > + if (ret) error: can't get "reg" property for node "%s" > + goto out; > + > + if (id >= pcdev->nr_lines || pcdev->pi[id].np) { Here two different kind of errors: - (id >= pcdev->nr_lines) reg value is out of range. print value and max range. - (pcdev->pi[id].np) other node with same reg value was already registered... print both node names. > + dev_err(pcdev->dev, "wrong id of pse pi: %u\n", > + id); > + ret = -EINVAL; > + goto out; > + } > + > + ret = of_property_count_strings(node, "pairset-names"); > + if (ret <= 0) if (ret < 0) error: can't get "pairset-names" property: %pe if (ret < 1 || ret > 2) error: wrong number of pairset-names. Should be 1 or 2, got %i > + goto out; > + npairsets = ret; > + > + ret = of_count_phandle_with_args(node, "pairsets", NULL); > + if (ret <= 0) same as for pairset-names > + goto out; > + > + /* npairsets is limited to value one or two */ > + if (ret != npairsets || ret > 2) { (ret > 2) will be handled by previous checks (ret != npairsets) amount of pairsets and pairset-names is not equal %i != %i > + dev_err(pcdev->dev, > + "wrong number of pairsets or pairset-names for pse pi %d\n", > + id); > + ret = -EINVAL; > + goto out; > + } > + > + ret = of_load_pse_pi_pairsets(node, &pi, npairsets); > + if (ret) > + goto out; > + > + of_node_get(node); > + pi.np = node; > + memcpy(&pcdev->pi[id], &pi, sizeof(pi)); > + } > + > + of_node_put(pis); > + return 0; > + > +out: > + for (i = 0; i <= pcdev->nr_lines; i++) { > + of_node_put(pcdev->pi[i].pairset[0].np); > + of_node_put(pcdev->pi[i].pairset[1].np); > + of_node_put(pcdev->pi[i].np); > + } > + of_node_put(node); > + of_node_put(pis); > + kfree(pcdev->pi); > + return ret; > } > > /** > @@ -67,16 +166,18 @@ static int of_pse_simple_xlate(struct pse_controller_dev *pcdev, > */ > int pse_controller_register(struct pse_controller_dev *pcdev) > { > - if (!pcdev->of_xlate) { > - if (pcdev->of_pse_n_cells == 0) > - pcdev->of_xlate = of_pse_zero_xlate; > - else if (pcdev->of_pse_n_cells == 1) > - pcdev->of_xlate = of_pse_simple_xlate; > - } > + int ret; > > mutex_init(&pcdev->lock); > INIT_LIST_HEAD(&pcdev->pse_control_head); > > + if (!pcdev->nr_lines) > + pcdev->nr_lines = 1; > + > + ret = of_load_pse_pis(pcdev); > + if (ret) > + return ret; > + > mutex_lock(&pse_list_mutex); > list_add(&pcdev->list, &pse_controller_list); > mutex_unlock(&pse_list_mutex); > @@ -91,6 +192,14 @@ EXPORT_SYMBOL_GPL(pse_controller_register); > */ > void pse_controller_unregister(struct pse_controller_dev *pcdev) > { > + int i; > + > + for (i = 0; i <= pcdev->nr_lines; i++) { > + of_node_put(pcdev->pi[i].pairset[0].np); > + of_node_put(pcdev->pi[i].pairset[1].np); > + of_node_put(pcdev->pi[i].np); > + } > + kfree(pcdev->pi); Same pattern was already used. It is better to move it to a separate function. > mutex_lock(&pse_list_mutex); > list_del(&pcdev->list); > mutex_unlock(&pse_list_mutex); > @@ -203,8 +312,33 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index) > return psec; > } > > -struct pse_control * > -of_pse_control_get(struct device_node *node) > +static int of_pse_match_pi(struct pse_controller_dev *pcdev, > + struct device_node *np) > +{ > + int i; > + > + for (i = 0; i <= pcdev->nr_lines; i++) { > + if (pcdev->pi[i].np == np) > + return i; > + } > + > + return -EINVAL; > +} > + > +static int psec_id_legacy_xlate(struct pse_controller_dev *pcdev, > + const struct of_phandle_args *pse_spec) rename legacy to some other name in all places. > +{ > + if (!pcdev->of_pse_n_cells) > + return 0; > + > + if (pcdev->of_pse_n_cells > 1 || > + pse_spec->args[0] >= pcdev->nr_lines) > + return -EINVAL; > + > + return pse_spec->args[0]; > +} > + > +struct pse_control *of_pse_control_get(struct device_node *node) > { > struct pse_controller_dev *r, *pcdev; > struct of_phandle_args args; > @@ -222,7 +356,14 @@ of_pse_control_get(struct device_node *node) > mutex_lock(&pse_list_mutex); > pcdev = NULL; > list_for_each_entry(r, &pse_controller_list, list) { > - if (args.np == r->dev->of_node) { > + if (!r->of_legacy) { > + ret = of_pse_match_pi(r, args.np); > + if (ret >= 0) { > + pcdev = r; > + psec_id = ret; > + break; > + } > + } else if (args.np == r->dev->of_node) { > pcdev = r; > break; > } > @@ -238,10 +379,12 @@ of_pse_control_get(struct device_node *node) > goto out; > } > > - psec_id = pcdev->of_xlate(pcdev, &args); > - if (psec_id < 0) { > - psec = ERR_PTR(psec_id); > - goto out; > + if (pcdev->of_legacy) { > + psec_id = psec_id_legacy_xlate(pcdev, &args); > + if (psec_id < 0) { > + psec = ERR_PTR(psec_id); > + goto out; > + } > } > > /* pse_list_mutex also protects the pcdev's pse_control list */ > diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h > index 19589571157f..01b3b9adfe2a 100644 > --- a/include/linux/pse-pd/pse.h > +++ b/include/linux/pse-pd/pse.h > @@ -64,6 +64,36 @@ struct device_node; > struct of_phandle_args; > struct pse_control; > > +/* PSE PI pairset pinout can either be Alternative A or Alternative B */ > +enum pse_pi_pairset_pinout { > + ALTERNATIVE_A, > + ALTERNATIVE_B, > +}; > + > +/** > + * struct pse_pi_pairset - PSE PI pairset entity describing the pinout > + * alternative ant its phandle > + * > + * @pinout: description of the pinout alternative > + * @np: device node pointer describing the pairset phandle > + */ > +struct pse_pi_pairset { > + enum pse_pi_pairset_pinout pinout; > + struct device_node *np; > +}; > + > +/** > + * struct pse_pi - PSE PI (Power Interface) entity as described in > + * IEEE 802.3-2022 145.2.4 > + * > + * @pairset: table of the PSE PI pinout alternative for the two pairset > + * @np: device node pointer of the PSE PI node > + */ > +struct pse_pi { > + struct pse_pi_pairset pairset[2]; > + struct device_node *np; > +}; > + > /** > * struct pse_controller_dev - PSE controller entity that might > * provide multiple PSE controls > @@ -73,11 +103,11 @@ struct pse_control; > * @pse_control_head: head of internal list of requested PSE controls > * @dev: corresponding driver model device struct > * @of_pse_n_cells: number of cells in PSE line specifiers > - * @of_xlate: translation function to translate from specifier as found in the > - * device tree to id as given to the PSE control ops > * @nr_lines: number of PSE controls in this controller device > * @lock: Mutex for serialization access to the PSE controller > * @types: types of the PSE controller > + * @pi: table of PSE PIs described in this controller device > + * @of_legacy: flag set if the pse_pis devicetree node is not used > */ > struct pse_controller_dev { > const struct pse_controller_ops *ops; > @@ -86,11 +116,11 @@ struct pse_controller_dev { > struct list_head pse_control_head; > struct device *dev; > int of_pse_n_cells; > - int (*of_xlate)(struct pse_controller_dev *pcdev, > - const struct of_phandle_args *pse_spec); > unsigned int nr_lines; > struct mutex lock; > enum ethtool_pse_types types; > + struct pse_pi *pi; > + bool of_legacy; > }; > > #if IS_ENABLED(CONFIG_PSE_CONTROLLER) Regards, Oleksij
Hello Oleskij, Thanks you for the review. On Fri, 1 Mar 2024 15:24:07 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > -static int of_pse_simple_xlate(struct pse_controller_dev *pcdev, > > - const struct of_phandle_args *pse_spec) > > +static int of_load_pse_pis(struct pse_controller_dev *pcdev) > > { > > - if (pse_spec->args[0] >= pcdev->nr_lines) > > - return -EINVAL; > > + struct device_node *np = pcdev->dev->of_node; > > + struct device_node *node, *pis; > > + int ret, i; > > > > - return pse_spec->args[0]; > > + if (!np) > > + return -ENODEV; > > + > > + pcdev->pi = kcalloc(pcdev->nr_lines, sizeof(*pcdev->pi), > > GFP_KERNEL); > > + if (!pcdev->pi) > > + return -ENOMEM; > > + > > + pis = of_get_child_by_name(np, "pse-pis"); > > + if (!pis) { > > Do we need to allocate pcdev->pi if there are no pse-pis? In fact it is not needed in this patch but in the patch 13 which use regulator framework, as the regulator is described on each pi structure. I will update them accordingly. > > + /* Legacy OF description of PSE PIs */ > > + pcdev->of_legacy = true; > > It is not "legacy" :) PoDL do not providing definition of PSE PI since there > is only one pair. May be: single_pair, no_pse_pi or any other idea. You right it is not needed for PoDL. Maybe no_pse_pi is better according to the following thoughts. Just wondering, how a pse controller that support PoE and PoDL simultaneously would be exposed in the binding. In that case I suppose all the PIs (PoE and PoDL) need to use the pse-pi subnode. Then the "alternative pinout" and "polarity" parameter would not be requested for PoDL PIs. > > + dev_err(pcdev->dev, "wrong id of pse pi: %u\n", > > + id); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + ret = of_property_count_strings(node, "pairset-names"); > > + if (ret <= 0) > > if (ret < 0) > error: can't get "pairset-names" property: %pe > if (ret < 1 || ret > 2) > error: wrong number of pairset-names. Should be 1 or 2, got %i Need to modify this to be able to have PoDL PIs without pairset description. Regards,
Hay Köry, On Fri, Mar 01, 2024 at 05:10:05PM +0100, Köry Maincent wrote: > Hello Oleskij, > > Thanks you for the review. I'll try to review more at weekend. > > > + /* Legacy OF description of PSE PIs */ > > > + pcdev->of_legacy = true; > > > > It is not "legacy" :) PoDL do not providing definition of PSE PI since there > > is only one pair. May be: single_pair, no_pse_pi or any other idea. > > You right it is not needed for PoDL. Maybe no_pse_pi is better according to the > following thoughts. > > Just wondering, how a pse controller that support PoE and PoDL simultaneously > would be exposed in the binding. In that case I suppose all the PIs (PoE and > PoDL) need to use the pse-pi subnode. Then the "alternative pinout" and > "polarity" parameter would not be requested for PoDL PIs. In case of hybrid device I would expect that we will have an 4 pair connector where only one pair will be used. In this case we will need to know what pair and polarity is supported or can be configured for PoDL. It will be full blown PSE PI node with PoDL specific extras. Don't worry about it right now. Regards, Oleksij
On Tue, Feb 27, 2024 at 03:42:55PM +0100, Kory Maincent wrote: > Integrate the regulator framework to the PSE framework for enhanced > access to features such as voltage, power measurement, and limits, which > are akin to regulators. Additionally, PSE features like port priorities > could potentially enhance the regulator framework. Note that this > integration introduces some implementation complexity, including wrapper > callbacks and nested locks, but the potential benefits make it worthwhile. > > Regulator are using enable counter with specific behavior. > Two calls to regulator_disable will trigger kernel warnings. > If the counter exceeds one, regulator_disable call won't disable the > PSE PI. These behavior isn't suitable for PSE control. > Added a boolean 'enabled' state to prevent multiple calls to Please rename rename "enabled" to "admin_state_enabled". This variable do not reflect real device state, it reflects only user configuration. ... > @@ -120,15 +118,9 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus, > u32 phy_id; > int rc; > > - psec = fwnode_find_pse_control(child); > - if (IS_ERR(psec)) > - return PTR_ERR(psec); > - > mii_ts = fwnode_find_mii_timestamper(child); > - if (IS_ERR(mii_ts)) { > - rc = PTR_ERR(mii_ts); > - goto clean_pse; > - } > + if (IS_ERR(mii_ts)) > + return PTR_ERR(mii_ts); > > is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); > if (is_c45 || fwnode_get_phy_id(child, &phy_id)) > @@ -161,6 +153,12 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus, > goto clean_phy; > } > > + psec = dev_find_pse_control(&phy->mdio.dev); > + if (IS_ERR(psec)) { > + rc = PTR_ERR(psec); > + goto unregister_phy; > + } > + I do not think it is a good idea to make PSE controller depend on phy->mdio.dev. The only reason why we have fwnode_find_pse_control() here was the missing port abstraction. ... > +static int > +devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev, > + char *name, int id) > +{ > + struct regulator_config rconfig = {0}; > + struct regulator_desc *rdesc; > + struct regulator_dev *rdev; > + > + rdesc = devm_kzalloc(pcdev->dev, sizeof(*rdesc), GFP_KERNEL); > + if (!rdesc) > + return -ENOMEM; > + > + /* Regulator descriptor id have to be the same as its associated > + * PSE PI id for the well functioning of the PSE controls. > + */ > + rdesc->id = id; > + rdesc->name = name; > + rdesc->type = REGULATOR_CURRENT; > + rdesc->ops = &pse_pi_ops; > + rdesc->owner = pcdev->owner; > + > + rconfig.dev = pcdev->dev; > + rconfig.driver_data = pcdev; > + rconfig.init_data = &pse_pi_initdata; Please add input supply to track all dependencies: if (of_property_present(np, "vin-supply")) config->input_supply = "vin"; May be better to make it not optional... Should be tested, but if, instead of "vin-supply", we will use "pse-supply" it will make most part of pse_regulator.c obsolete. .... > @@ -310,6 +452,20 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index) > return ERR_PTR(-ENODEV); > } > > + psec->ps = devm_regulator_get_exclusive(dev, > + rdev_get_name(pcdev->pi[index].rdev)); > + if (IS_ERR(psec->ps)) { > + kfree(psec); > + return ERR_CAST(psec->ps); > + } > + > + ret = regulator_is_enabled(psec->ps); > + if (ret < 0) { > + kfree(psec); > + return ERR_PTR(ret); > + } > + pcdev->pi[index].enabled = ret; If I see it correctly, it will prevent us to refcount a request from user space. So, the runtime PM may suspend PI. > + > psec->pcdev = pcdev; > list_add(&psec->list, &pcdev->pse_control_head); > psec->id = index; > @@ -344,7 +500,8 @@ static int psec_id_legacy_xlate(struct pse_controller_dev *pcdev, > return pse_spec->args[0]; > } > > -struct pse_control *of_pse_control_get(struct device_node *node) > +struct pse_control *of_pse_control_get(struct device *dev, > + struct device_node *node) > { > struct pse_controller_dev *r, *pcdev; > struct of_phandle_args args; > @@ -394,7 +551,7 @@ struct pse_control *of_pse_control_get(struct device_node *node) > } > > /* pse_list_mutex also protects the pcdev's pse_control list */ > - psec = pse_control_get_internal(pcdev, psec_id); > + psec = pse_control_get_internal(dev, pcdev, psec_id); > > out: > mutex_unlock(&pse_list_mutex); > @@ -443,21 +600,24 @@ int pse_ethtool_set_config(struct pse_control *psec, > struct netlink_ext_ack *extack, > const struct pse_control_config *config) > { > - const struct pse_controller_ops *ops; > - int err; > - > - ops = psec->pcdev->ops; > + int err = 0; > > - if (!ops->ethtool_set_config) { > - NL_SET_ERR_MSG(extack, > - "PSE driver does not configuration"); > - return -EOPNOTSUPP; > + /* Look at enabled status to not call regulator_enable or > + * regulator_disable twice creating a regulator counter mismatch > + */ > + switch (config->c33_admin_control) { > + case ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED: > + if (!psec->pcdev->pi[psec->id].enabled) > + err = regulator_enable(psec->ps); > + break; > + case ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED: > + if (psec->pcdev->pi[psec->id].enabled) > + err = regulator_disable(psec->ps); > + break; > + default: > + err = -EOPNOTSUPP; > } This change seems to break PoDL support > - mutex_lock(&psec->pcdev->lock); > - err = ops->ethtool_set_config(psec->pcdev, psec->id, extack, config); > - mutex_unlock(&psec->pcdev->lock); > - > return err; Regards, Oleksij
Hello Oleksij, Thanks for your review!! On Sat, 2 Mar 2024 22:35:52 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > On Tue, Feb 27, 2024 at 03:42:55PM +0100, Kory Maincent wrote: > > Integrate the regulator framework to the PSE framework for enhanced > > access to features such as voltage, power measurement, and limits, which > > are akin to regulators. Additionally, PSE features like port priorities > > could potentially enhance the regulator framework. Note that this > > integration introduces some implementation complexity, including wrapper > > callbacks and nested locks, but the potential benefits make it worthwhile. > > > > Regulator are using enable counter with specific behavior. > > Two calls to regulator_disable will trigger kernel warnings. > > If the counter exceeds one, regulator_disable call won't disable the > > PSE PI. These behavior isn't suitable for PSE control. > > Added a boolean 'enabled' state to prevent multiple calls to > > Please rename rename "enabled" to "admin_state_enabled". This variable > do not reflect real device state, it reflects only user configuration. Right, > > ... > > @@ -120,15 +118,9 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus, > > u32 phy_id; > > int rc; > > > > - psec = fwnode_find_pse_control(child); > > - if (IS_ERR(psec)) > > - return PTR_ERR(psec); > > - > > mii_ts = fwnode_find_mii_timestamper(child); > > - if (IS_ERR(mii_ts)) { > > - rc = PTR_ERR(mii_ts); > > - goto clean_pse; > > - } > > + if (IS_ERR(mii_ts)) > > + return PTR_ERR(mii_ts); > > > > is_c45 = fwnode_device_is_compatible(child, > > "ethernet-phy-ieee802.3-c45"); if (is_c45 || fwnode_get_phy_id(child, > > &phy_id)) @@ -161,6 +153,12 @@ int fwnode_mdiobus_register_phy(struct > > mii_bus *bus, goto clean_phy; > > } > > > > + psec = dev_find_pse_control(&phy->mdio.dev); > > + if (IS_ERR(psec)) { > > + rc = PTR_ERR(psec); > > + goto unregister_phy; > > + } > > + > > I do not think it is a good idea to make PSE controller depend on > phy->mdio.dev. The only reason why we have fwnode_find_pse_control() > here was the missing port abstraction. I totally agree that having port abstraction would be more convenient. Maxime Chevallier is currently working on this and will post it after his multi-phy series get merged. Meanwhile, we still need a device pointer for getting the regulator. The phy->mdio.dev is the only one I can think of as a regulator consumer. Another idea? > ... > > +static int > > +devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev, > > + char *name, int id) > > +{ > > + struct regulator_config rconfig = {0}; > > + struct regulator_desc *rdesc; > > + struct regulator_dev *rdev; > > + > > + rdesc = devm_kzalloc(pcdev->dev, sizeof(*rdesc), GFP_KERNEL); > > + if (!rdesc) > > + return -ENOMEM; > > + > > + /* Regulator descriptor id have to be the same as its associated > > + * PSE PI id for the well functioning of the PSE controls. > > + */ > > + rdesc->id = id; > > + rdesc->name = name; > > + rdesc->type = REGULATOR_CURRENT; > > + rdesc->ops = &pse_pi_ops; > > + rdesc->owner = pcdev->owner; > > + > > + rconfig.dev = pcdev->dev; > > + rconfig.driver_data = pcdev; > > + rconfig.init_data = &pse_pi_initdata; > > Please add input supply to track all dependencies: > if (of_property_present(np, "vin-supply")) > config->input_supply = "vin"; > > May be better to make it not optional... Does the "vin-supply" property be added at the pse-pi node level or the pse-controller node level or at the hardware port node level or the manager node level for the pd692x0? Maybe better at the pse-pi node level and each PIs of the manager will get the same regulator? What do you think? > Should be tested, but if, instead of "vin-supply", we will use > "pse-supply" it will make most part of pse_regulator.c obsolete. Don't know, if it is done at the pse-pi node level it may not break pse_regulator.c. Not sure about it. > .... > > @@ -310,6 +452,20 @@ pse_control_get_internal(struct pse_controller_dev > > *pcdev, unsigned int index) return ERR_PTR(-ENODEV); > > } > > > > + psec->ps = devm_regulator_get_exclusive(dev, > > + > > rdev_get_name(pcdev->pi[index].rdev)); > > + if (IS_ERR(psec->ps)) { > > + kfree(psec); > > + return ERR_CAST(psec->ps); > > + } > > + > > + ret = regulator_is_enabled(psec->ps); > > + if (ret < 0) { > > + kfree(psec); > > + return ERR_PTR(ret); > > + } > > + pcdev->pi[index].enabled = ret; > > If I see it correctly, it will prevent us to refcount a request from > user space. So, the runtime PM may suspend PI. I don't think so as the regulator_get_exclusive() does the same and refcount it: https://elixir.bootlin.com/linux/v6.7.8/source/drivers/regulator/core.c#L2268 > > + switch (config->c33_admin_control) { > > + case ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED: > > + if (!psec->pcdev->pi[psec->id].enabled) > > + err = regulator_enable(psec->ps); > > + break; > > + case ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED: > > + if (psec->pcdev->pi[psec->id].enabled) > > + err = regulator_disable(psec->ps); > > + break; > > + default: > > + err = -EOPNOTSUPP; > > } > > This change seems to break PoDL support Oh that's totally true sorry for that mistake. Regards,
On Mon, Mar 04, 2024 at 10:27:08AM +0100, Köry Maincent wrote: > Hello Oleksij, > > > + psec = dev_find_pse_control(&phy->mdio.dev); > > > + if (IS_ERR(psec)) { > > > + rc = PTR_ERR(psec); > > > + goto unregister_phy; > > > + } > > > + > > > > I do not think it is a good idea to make PSE controller depend on > > phy->mdio.dev. The only reason why we have fwnode_find_pse_control() > > here was the missing port abstraction. > > I totally agree that having port abstraction would be more convenient. > Maxime Chevallier is currently working on this and will post it after his > multi-phy series get merged. > Meanwhile, we still need a device pointer for getting the regulator. The > phy->mdio.dev is the only one I can think of as a regulator consumer. > Another idea? I would say, in current code state, PSE controller is regulator provider and consumer - both are same devices. Otherwise, it will be impossible to unregistered PHY devices without shutting down PSE-PI. Mostly, we should be able to continue to provide the power even if network interface is down. > > > + rconfig.dev = pcdev->dev; > > > + rconfig.driver_data = pcdev; > > > + rconfig.init_data = &pse_pi_initdata; > > > > Please add input supply to track all dependencies: > > if (of_property_present(np, "vin-supply")) > > config->input_supply = "vin"; > > > > May be better to make it not optional... > > Does the "vin-supply" property be added at the pse-pi node level or the > pse-controller node level or at the hardware port node level or the manager node > level for the pd692x0? > Maybe better at the pse-pi node level and each PIs of the manager will get the > same regulator? > What do you think? Yes, I agree. PSE-PI should share same parent regulator. Different PSE managers may have different power supplies. One port (PSE PI) - not. > > > Should be tested, but if, instead of "vin-supply", we will use > > "pse-supply" it will make most part of pse_regulator.c obsolete. > > Don't know, if it is done at the pse-pi node level it may not break > pse_regulator.c. Not sure about it. me too. Before your patch set, the regulator topology for PoDL PSE was following: power-source fixed-regulator PoDL_PSE-consumer Now it will be: power-source fixed-regulator PoDL_PSE-consumer PSE-PI-provider PSE-PI-consumer By porting porting PSE framework to regulator, probably it make sense to remove two levels of regulators? power-source fixed-regulator PSE-PI-consumer > > .... > > > @@ -310,6 +452,20 @@ pse_control_get_internal(struct pse_controller_dev > > > *pcdev, unsigned int index) return ERR_PTR(-ENODEV); > > > } > > > > > > + psec->ps = devm_regulator_get_exclusive(dev, > > > + > > > rdev_get_name(pcdev->pi[index].rdev)); > > > + if (IS_ERR(psec->ps)) { > > > + kfree(psec); > > > + return ERR_CAST(psec->ps); > > > + } > > > + > > > + ret = regulator_is_enabled(psec->ps); > > > + if (ret < 0) { > > > + kfree(psec); > > > + return ERR_PTR(ret); > > > + } > > > + pcdev->pi[index].enabled = ret; > > > > If I see it correctly, it will prevent us to refcount a request from > > user space. So, the runtime PM may suspend PI. > > I don't think so as the regulator_get_exclusive() does the same and refcount it: > https://elixir.bootlin.com/linux/v6.7.8/source/drivers/regulator/core.c#L2268 ok, thx.
> > > + psec = dev_find_pse_control(&phy->mdio.dev); > > > + if (IS_ERR(psec)) { > > > + rc = PTR_ERR(psec); > > > + goto unregister_phy; > > > + } > > > + > > > > I do not think it is a good idea to make PSE controller depend on > > phy->mdio.dev. The only reason why we have fwnode_find_pse_control() > > here was the missing port abstraction. > > I totally agree that having port abstraction would be more convenient. > Maxime Chevallier is currently working on this and will post it after his > multi-phy series get merged. > Meanwhile, we still need a device pointer for getting the regulator. The > phy->mdio.dev is the only one I can think of as a regulator consumer. > Another idea? Sorry, i've not been keeping up... Doesn't the device tree binding determine this? Where is the consumer in the tree? Andrew
On Mon, Mar 04, 2024 at 02:32:50PM +0100, Andrew Lunn wrote: > > > > + psec = dev_find_pse_control(&phy->mdio.dev); > > > > + if (IS_ERR(psec)) { > > > > + rc = PTR_ERR(psec); > > > > + goto unregister_phy; > > > > + } > > > > + > > > > > > I do not think it is a good idea to make PSE controller depend on > > > phy->mdio.dev. The only reason why we have fwnode_find_pse_control() > > > here was the missing port abstraction. > > > > I totally agree that having port abstraction would be more convenient. > > Maxime Chevallier is currently working on this and will post it after his > > multi-phy series get merged. > > Meanwhile, we still need a device pointer for getting the regulator. The > > phy->mdio.dev is the only one I can think of as a regulator consumer. > > Another idea? > > Sorry, i've not been keeping up... > > Doesn't the device tree binding determine this? Where is the consumer > in the tree? The real consumer is outside of the system. Withing the system, it would be the RJ45 port, but we have no abstraction for ports so far.
On Mon, Mar 04, 2024 at 02:39:08PM +0100, Oleksij Rempel wrote: > On Mon, Mar 04, 2024 at 02:32:50PM +0100, Andrew Lunn wrote: > > > > > + psec = dev_find_pse_control(&phy->mdio.dev); > > > > > + if (IS_ERR(psec)) { > > > > > + rc = PTR_ERR(psec); > > > > > + goto unregister_phy; > > > > > + } > > > > > + > > > > > > > > I do not think it is a good idea to make PSE controller depend on > > > > phy->mdio.dev. The only reason why we have fwnode_find_pse_control() > > > > here was the missing port abstraction. > > > > > > I totally agree that having port abstraction would be more convenient. > > > Maxime Chevallier is currently working on this and will post it after his > > > multi-phy series get merged. > > > Meanwhile, we still need a device pointer for getting the regulator. The > > > phy->mdio.dev is the only one I can think of as a regulator consumer. > > > Another idea? > > > > Sorry, i've not been keeping up... > > > > Doesn't the device tree binding determine this? Where is the consumer > > in the tree? > > The real consumer is outside of the system. The device on the other end of the cable? > Withing the system, it would be the RJ45 port, but we have no > abstraction for ports so far. A Linux regulator is generally used in a producer/consumer pair. If there is no consumer device, why have a producer? What is going to use the consumer API? When we have a port representor, do we expect it to have active elements? Something which will consume this regulator? Andrew
On Mon, Mar 04, 2024 at 02:53:54PM +0100, Andrew Lunn wrote: > On Mon, Mar 04, 2024 at 02:39:08PM +0100, Oleksij Rempel wrote: > > On Mon, Mar 04, 2024 at 02:32:50PM +0100, Andrew Lunn wrote: > > > > > > + psec = dev_find_pse_control(&phy->mdio.dev); > > > > > > + if (IS_ERR(psec)) { > > > > > > + rc = PTR_ERR(psec); > > > > > > + goto unregister_phy; > > > > > > + } > > > > > > + > > > > > > > > > > I do not think it is a good idea to make PSE controller depend on > > > > > phy->mdio.dev. The only reason why we have fwnode_find_pse_control() > > > > > here was the missing port abstraction. > > > > > > > > I totally agree that having port abstraction would be more convenient. > > > > Maxime Chevallier is currently working on this and will post it after his > > > > multi-phy series get merged. > > > > Meanwhile, we still need a device pointer for getting the regulator. The > > > > phy->mdio.dev is the only one I can think of as a regulator consumer. > > > > Another idea? > > > > > > Sorry, i've not been keeping up... > > > > > > Doesn't the device tree binding determine this? Where is the consumer > > > in the tree? > > > > The real consumer is outside of the system. > > The device on the other end of the cable? yes. > > Withing the system, it would be the RJ45 port, but we have no > > abstraction for ports so far. > > A Linux regulator is generally used in a producer/consumer pair. If > there is no consumer device, why have a producer? What is going to use > the consumer API? We already consulted Mark Brown in precious iterations of this patch series and got his OK. I also described all advantages of using regulator framework within the PSE subsystem. I need to search it. Short answer, it is relatively common to have open-ended regulator with consumer outside of the system. A PSE system can be relatively complex, representing all supply dependencies from power supplies (one or multiple) to the ports will help to provide needed diagnostic information and power saving if port are disabled. Some functionality is currently not supported by the regulator framework, but need to be extended - power budged, priorities and reservation. All of this are not exclusive PSE challenges. So, using regulator framework seems to be a straightforward decision. > When we have a port representor, do we expect it to have active > elements? Something which will consume this regulator? Not in a usual sense. There are two levels of PSE control: - autodetected by PSE controller - fine tuned by using LLDP wich may respond to PD requests by allocating more or reducing/disabling power. Regards, Oleksij
On Tue, 27 Feb 2024 10:31:05 -0500 Jamal Hadi Salim <jhs@mojatatu.com> wrote: > On Tue, Feb 27, 2024 at 9:43 AM Kory Maincent <kory.maincent@bootlin.com> > wrote: > > > > This patch series aims at adding support for PoE (Power over Ethernet), > > based on the already existing support for PoDL (Power over Data Line) > > implementation. In addition, it adds support for two specific PoE > > controller, the Microchip PD692x0 and the TI TPS23881. > > > > This patch series is sponsored by Dent Project > > <dentproject@linuxfoundation.org>. > > Sorry, couldnt resist because it sounded like a commercial;-> And > likely i am out of touch. I am all for giving credit but does it have > to be explicitly called out as "this patch is sponsored by X"? Hello Jamal, I will remove the line and add it in the From field as suggested by Jakub. It seems to be the usual way to do it. Regards,
Hello Oleksij, On Mon, 4 Mar 2024 11:31:19 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > > > Should be tested, but if, instead of "vin-supply", we will use > > > "pse-supply" it will make most part of pse_regulator.c obsolete. > > > > Don't know, if it is done at the pse-pi node level it may not break > > pse_regulator.c. Not sure about it. > > me too. Before your patch set, the regulator topology for PoDL PSE was > following: > power-source > fixed-regulator > PoDL_PSE-consumer > > Now it will be: > power-source > fixed-regulator > PoDL_PSE-consumer > PSE-PI-provider > PSE-PI-consumer > > By porting porting PSE framework to regulator, probably it make sense to > remove two levels of regulators? > power-source > fixed-regulator > PSE-PI-consumer Sorry, I forgot to reply about this. This is specific to pse_regulator driver. Could we tackle this change in another patch series when the current patch series got applied? Also I don't have the hardware to test it. Regards,
On Thu, Mar 21, 2024 at 05:15:24PM +0100, Kory Maincent wrote: > Hello Oleksij, > > On Mon, 4 Mar 2024 11:31:19 +0100 > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > > > > > > Should be tested, but if, instead of "vin-supply", we will use > > > > "pse-supply" it will make most part of pse_regulator.c obsolete. > > > > > > Don't know, if it is done at the pse-pi node level it may not break > > > pse_regulator.c. Not sure about it. > > > > me too. Before your patch set, the regulator topology for PoDL PSE was > > following: > > power-source > > fixed-regulator > > PoDL_PSE-consumer > > > > Now it will be: > > power-source > > fixed-regulator > > PoDL_PSE-consumer > > PSE-PI-provider > > PSE-PI-consumer > > > > By porting porting PSE framework to regulator, probably it make sense to > > remove two levels of regulators? > > power-source > > fixed-regulator > > PSE-PI-consumer > > Sorry, I forgot to reply about this. > This is specific to pse_regulator driver. Could we tackle this change in another > patch series when the current patch series got applied? > Also I don't have the hardware to test it. ACK, no problem.
Hello Oleksij, On Thu, 21 Mar 2024 17:43:14 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > On Thu, Mar 21, 2024 at 05:15:24PM +0100, Kory Maincent wrote: > > Hello Oleksij, > > Sorry, I forgot to reply about this. > > This is specific to pse_regulator driver. Could we tackle this change in > > another patch series when the current patch series got applied? > > Also I don't have the hardware to test it. > > ACK, no problem. I have a question unrelated to this. Why do you add refcount on the pse_control struct? The pse control is related to the RJ45 port. Each port is exclusively related to one pse control. Shouldn't we return an error in case of two get of the same pse control index? Do you see use cases where a pse control could be get two times? Regards,
Hay Kory, On Fri, Mar 22, 2024 at 11:39:50AM +0100, Kory Maincent wrote: > Hello Oleksij, > > On Thu, 21 Mar 2024 17:43:14 +0100 > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > On Thu, Mar 21, 2024 at 05:15:24PM +0100, Kory Maincent wrote: > > > Hello Oleksij, > > > Sorry, I forgot to reply about this. > > > This is specific to pse_regulator driver. Could we tackle this change in > > > another patch series when the current patch series got applied? > > > Also I don't have the hardware to test it. > > > > ACK, no problem. > > I have a question unrelated to this. > Why do you add refcount on the pse_control struct? > The pse control is related to the RJ45 port. Each port is exclusively related > to one pse control. > Shouldn't we return an error in case of two get of the same pse control index? > Do you see use cases where a pse control could be get two times? I assume, any instance which need coordinate PSE behavior with own actions. For example - PHY will probably need to coordinate PHY state with PSE PD classification process. Regards, Oleksij
On Fri, 22 Mar 2024 15:07:45 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > Hay Kory, > > On Fri, Mar 22, 2024 at 11:39:50AM +0100, Kory Maincent wrote: > > Hello Oleksij, > > > > On Thu, 21 Mar 2024 17:43:14 +0100 > > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > > > On Thu, Mar 21, 2024 at 05:15:24PM +0100, Kory Maincent wrote: > > > > Hello Oleksij, > > > > Sorry, I forgot to reply about this. > > > > This is specific to pse_regulator driver. Could we tackle this change in > > > > another patch series when the current patch series got applied? > > > > Also I don't have the hardware to test it. > > > > > > ACK, no problem. > > > > I have a question unrelated to this. > > Why do you add refcount on the pse_control struct? > > The pse control is related to the RJ45 port. Each port is exclusively > > related to one pse control. > > Shouldn't we return an error in case of two get of the same pse control > > index? Do you see use cases where a pse control could be get two times? > > I assume, any instance which need coordinate PSE behavior with own actions. > For example - PHY will probably need to coordinate PHY state with PSE PD > classification process. Indeed, I was focused on devicetree and didn't thought of coordination between PHY and PSE. Thanks for your reply. Regards,
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 two specific PoE controller, the Microchip PD692x0 and the TI TPS23881. This patch series is sponsored by Dent Project <dentproject@linuxfoundation.org>. In detail: - Patch 1 to 13 prepare net to support PoE devices. - Patch 14 and 15 add PD692x0 PoE PSE controller driver and its binding. - Patch 16 and 17 add TI TPS23881 PSE controller driver and its binding. Changes in v5: - Fix bindings nit. - Add supported-polarity parameter to bindings. - Fix yamllint binding errors. - Remove the nested lock brought by the use of regulator framework. - Link to v4: https://lore.kernel.org/r/20240215-feature_poe-v4-0-35bb4c23266c@bootlin.com Changes in v4: - Replaced sponsored-by tag by a simple sentence. - Fix pse_pi node bindings. - Add pse pi documentation written by Oleksij. - Link to v3: https://lore.kernel.org/r/20240208-feature_poe-v3-0-531d2674469e@bootlin.com Changes in v3: - Add patches to add Oleksij and myself to PSE MAINTAINERS. - Add patches to add pse devlink. - Add TI TPS23881 PSE controller driver with its binding. - Replace pse_get_types helper by pse_has_podl and pse_has_c33 - Changed the PSE core bindings. - Add a setup_pi_matrix callback. - Register regulator for each PSE PI (Power Interface). - Changed the PD692x0 bindings. - Updated PD692x0 drivers to new bindings and PSE PI description. - Updated PD692x0 drivers according to the reviews and made fixes. - Link to v2: https://lore.kernel.org/r/20231201-feature_poe-v2-0-56d8cac607fa@bootlin.com 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 (17): MAINTAINERS: net: Add Oleksij to pse-pd maintainers of: property: Add fw_devlink support for pse parent 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 MAINTAINERS: Add myself to pse networking maintainer net: pse-pd: Add support for PSE PIs dt-bindings: net: pse-pd: Add another way of describing several PSE PIs net: pse-pd: Add support for setup_pi_matrix callback net: pse-pd: Use regulator framework within PSE framework dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller net: pse-pd: Add PD692x0 PSE controller driver dt-bindings: net: pse-pd: Add bindings for TPS23881 PSE controller net: pse-pd: Add TI TPS23881 PSE controller driver .../bindings/net/pse-pd/microchip,pd692x0.yaml | 158 +++ .../bindings/net/pse-pd/pse-controller.yaml | 100 +- .../bindings/net/pse-pd/ti,tps23881.yaml | 93 ++ Documentation/netlink/specs/ethtool.yaml | 33 +- Documentation/networking/ethtool-netlink.rst | 20 + Documentation/networking/index.rst | 1 + Documentation/networking/pse-pd/index.rst | 10 + Documentation/networking/pse-pd/introduction.rst | 73 ++ Documentation/networking/pse-pd/pse-pi.rst | 302 +++++ MAINTAINERS | 8 + drivers/net/mdio/fwnode_mdio.c | 29 +- drivers/net/pse-pd/Kconfig | 20 + drivers/net/pse-pd/Makefile | 2 + drivers/net/pse-pd/pd692x0.c | 1223 ++++++++++++++++++++ drivers/net/pse-pd/pse_core.c | 429 ++++++- drivers/net/pse-pd/pse_regulator.c | 49 +- drivers/net/pse-pd/tps23881.c | 818 +++++++++++++ drivers/of/property.c | 2 + include/linux/pse-pd/pse.h | 86 +- include/uapi/linux/ethtool.h | 55 + include/uapi/linux/ethtool_netlink.h | 3 + net/ethtool/pse-pd.c | 60 +- 22 files changed, 3451 insertions(+), 123 deletions(-) --- base-commit: f308eae1e1cdacca3cef65c7f4f691dfcb0c8976 change-id: 20231024-feature_poe-139490e73403 Best regards,