| Message ID | 20220819120109.3857571-1-o.rempel@pengutronix.de |
|---|---|
| Headers | show |
| Series | add generic PSE support | expand |
On Fri, Aug 19, 2022 at 02:01:06PM +0200, Oleksij Rempel wrote: > Add generic driver to support simple Power Sourcing Equipment without > automatic classification support. > > This driver was tested on 10Bast-T1L switch with regulator based PoDL PSE. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/pse-pd/Kconfig | 11 +++ > drivers/net/pse-pd/Makefile | 2 + > drivers/net/pse-pd/pse_generic.c | 146 +++++++++++++++++++++++++++++++ > 3 files changed, 159 insertions(+) > create mode 100644 drivers/net/pse-pd/pse_generic.c > > diff --git a/drivers/net/pse-pd/Kconfig b/drivers/net/pse-pd/Kconfig > index 49c7f0bcff526..a804c9f1af2bc 100644 > --- a/drivers/net/pse-pd/Kconfig > +++ b/drivers/net/pse-pd/Kconfig > @@ -9,3 +9,14 @@ menuconfig PSE_CONTROLLER > Generic Power Sourcing Equipment Controller support. > > If unsure, say no. > + > +if PSE_CONTROLLER > + > +config PSE_GENERIC > + tristate "Generic PSE driver" > + help > + This module provides support for simple Ethernet Power Sourcing > + Equipment without automatic classification support. For example for > + PoDL (802.3bu) specification. > + > +endif > diff --git a/drivers/net/pse-pd/Makefile b/drivers/net/pse-pd/Makefile > index cbb79fc2e2706..80ef39ad68f10 100644 > --- a/drivers/net/pse-pd/Makefile > +++ b/drivers/net/pse-pd/Makefile > @@ -2,3 +2,5 @@ > # Makefile for Linux PSE drivers > > obj-$(CONFIG_PSE_CONTROLLER) += pse-core.o > + > +obj-$(CONFIG_PSE_GENERIC) += pse_generic.o > diff --git a/drivers/net/pse-pd/pse_generic.c b/drivers/net/pse-pd/pse_generic.c > new file mode 100644 > index 0000000000000..f264d4d589f59 > --- /dev/null > +++ b/drivers/net/pse-pd/pse_generic.c > @@ -0,0 +1,146 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Driver for the Generic Ethernet Power Sourcing Equipment, without > +// auto classification support. > +// > +// Copyright (c) 2022 Pengutronix, Oleksij Rempel <kernel@pengutronix.de> > +// > + > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pse-pd/pse.h> > +#include <linux/regulator/consumer.h> > + > +struct gen_pse_priv { > + struct pse_controller_dev pcdev; > + struct regulator *ps; /*power source */ > + enum ethtool_podl_pse_admin_state admin_state; > +}; > + > +static struct gen_pse_priv *to_gen_pse(struct pse_controller_dev *pcdev) > +{ > + return container_of(pcdev, struct gen_pse_priv, pcdev); > +} > + > +static int > +gen_pse_podl_get_admin_sate(struct pse_controller_dev *pcdev, unsigned long id) Should that be state? > +{ > + struct gen_pse_priv *priv = to_gen_pse(pcdev); > + > + /* aPoDLPSEAdminState can be different to aPoDLPSEPowerDetectionStatus > + * which is provided by the regulator. > + */ > + return priv->admin_state; > +} > + > +static int > +gen_pse_podl_set_admin_control(struct pse_controller_dev *pcdev, > + unsigned long id, > + enum ethtool_podl_pse_admin_state state) > +{ > + struct gen_pse_priv *priv = to_gen_pse(pcdev); > + int ret; > + > + if (priv->admin_state == state) > + goto set_state; return 0; ? > + > + switch (state) { > + case ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED: > + ret = regulator_enable(priv->ps); > + break; > + case ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED: > + ret = regulator_disable(priv->ps); > + break; > + default: > + dev_err(pcdev->dev, "Unknown admin state %i\n", state); > + ret = -ENOTSUPP; > + } > + > + if (ret) > + return ret; > + > +set_state: > + priv->admin_state = state; > + > + return 0; > +} > + > +static int > +gen_pse_podl_get_pw_d_status(struct pse_controller_dev *pcdev, unsigned long id) > +{ > + struct gen_pse_priv *priv = to_gen_pse(pcdev); > + int ret; > + > + ret = regulator_is_enabled(priv->ps); > + if (ret < 0) > + return ret; > + > + if (!ret) > + return ETHTOOL_PODL_PSE_PW_D_STATUS_DISABLED; > + > + return ETHTOOL_PODL_PSE_PW_D_STATUS_DELIVERING; > +} > + > +static const struct pse_control_ops gen_pse_ops = { > + .get_podl_pse_admin_sate = gen_pse_podl_get_admin_sate, > + .set_podl_pse_admin_control = gen_pse_podl_set_admin_control, > + .get_podl_pse_pw_d_status = gen_pse_podl_get_pw_d_status, > +}; > + > +static int > +gen_pse_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct gen_pse_priv *priv; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + if (!pdev->dev.of_node) > + return -ENOENT; > + > + priv->ps = devm_regulator_get(dev, "ieee802.3-podl-pse"); > + if (IS_ERR(priv->ps)) { > + dev_err(dev, "failed to get PSE regulator (%pe)\n", priv->ps); > + return PTR_ERR(priv->ps); > + } > + > + platform_set_drvdata(pdev, priv); > + > + priv->admin_state = ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED; There is the comment earlier: /* aPoDLPSEAdminState can be different to aPoDLPSEPowerDetectionStatus * which is provided by the regulator. Is this because the regulator might of been turned on, but it has detected a short and turned itself off? So it is administratively on, but off in order to stop the magic smoke escaping? But what about the other way around? Something has already turned the regulator on, e.g. the bootloader. Should the default be ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED even thought power is being delivered? Do we want to put the regulator into the off state at probe, so it is in a well defined state? Or set priv->admin_state to whatever regulator_is_enabled() indicates? Andrew
> $ ip l > ... > 5: t1l1@eth0: <BROADCAST,MULTICAST> .. > ... > > $ ethtool --show-pse t1l1 > PSE attributs for t1l1: > PoDL PSE Admin State: disabled > PoDL PSE Power Detection Status: disabled > > $ ethtool --set-pse t1l1 podl-pse-admin-control enable > $ ethtool --show-pse t1l1 > PSE attributs for t1l1: > PoDL PSE Admin State: enabled > PoDL PSE Power Detection Status: delivering power Here you seem to indicate that delivering power is totally independent of the interface admin status, <BROADCAST,MULTICAST>. The interface is admin down, yet you can make it deliver power. I thought there might be a link between interface admin status and power? Do the standards say anything about this? Is there some sort of industrial norm? I'm also wondering about the defaults. It seems like the defaults you are proposing is power is off by default, and you have to use ethtool to enable power. That does not seem like the most friendly settings. Why not an 'auto' mode where if the PHY has PoDL PSE capabilities, on ifup it is enabled, on ifdown it is disabled? And you can put it into a 'manual' mode where you control it independent of administrative status of the interface? Andrew
On Fri, Aug 19, 2022 at 02:01:06PM +0200, Oleksij Rempel wrote: > Add generic driver to support simple Power Sourcing Equipment without > automatic classification support. > > This driver was tested on 10Bast-T1L switch with regulator based PoDL PSE. Do you have access to a PHY which implements clause 45.2.9? That seems like a better reference implementation? I don't know the market, what is more likely, a simple regulator, or something more capable with an interface like 45.2.9? netlink does allow us to keep adding more attributes, so we don't need to be perfect first time, but it seems like 45.2.9 is what IEEE expect vendors to provide, so at some point Linux should implement it. Andrew
On Fri, Aug 19, 2022 at 10:54:14PM +0200, Andrew Lunn wrote: > > +static int > > +gen_pse_podl_get_admin_sate(struct pse_controller_dev *pcdev, unsigned long id) > > Should that be state? ack. fixed. > > +{ > > + struct gen_pse_priv *priv = to_gen_pse(pcdev); > > + > > + /* aPoDLPSEAdminState can be different to aPoDLPSEPowerDetectionStatus > > + * which is provided by the regulator. > > + */ > > + return priv->admin_state; > > +} > > + > > +static int > > +gen_pse_podl_set_admin_control(struct pse_controller_dev *pcdev, > > + unsigned long id, > > + enum ethtool_podl_pse_admin_state state) > > +{ > > + struct gen_pse_priv *priv = to_gen_pse(pcdev); > > + int ret; > > + > > + if (priv->admin_state == state) > > + goto set_state; > > return 0; ? ack. done > > + platform_set_drvdata(pdev, priv); > > + > > + priv->admin_state = ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED; > > There is the comment earlier: > > /* aPoDLPSEAdminState can be different to aPoDLPSEPowerDetectionStatus > * which is provided by the regulator. > > Is this because the regulator might of been turned on, but it has > detected a short and turned itself off? So it is administratively on, > but off in order to stop the magic smoke escaping? ack. According to 30.15.1.1.3 aPoDLPSEPowerDetectionStatus, we may have following: /** * enum ethtool_podl_pse_pw_d_status - power detection status of the PoDL PSE. * IEEE 802.3-2018 30.15.1.1.3 aPoDLPSEPowerDetectionStatus: * @ETHTOOL_PODL_PSE_PW_D_STATUS_UNKNOWN: PoDL PSE * @ETHTOOL_PODL_PSE_PW_D_STATUS_DISABLED: "The enumeration “disabled” is * asserted true when the PoDL PSE state diagram variable mr_pse_enable is * false" * @ETHTOOL_PODL_PSE_PW_D_STATUS_SEARCHING: "The enumeration “searching” is * asserted true when either of the PSE state diagram variables * pi_detecting or pi_classifying is true." * @ETHTOOL_PODL_PSE_PW_D_STATUS_DELIVERING: "The enumeration “deliveringPower” * is asserted true when the PoDL PSE state diagram variable pi_powered is * true." * @ETHTOOL_PODL_PSE_PW_D_STATUS_SLEEP: "The enumeration “sleep” is asserted * true when the PoDL PSE state diagram variable pi_sleeping is true." * @ETHTOOL_PODL_PSE_PW_D_STATUS_IDLE: "The enumeration “idle” is asserted true * when the logical combination of the PoDL PSE state diagram variables * pi_prebiased*!pi_sleeping is true." * @ETHTOOL_PODL_PSE_PW_D_STATUS_ERROR: "The enumeration “error” is asserted * true when the PoDL PSE state diagram variable overload_held is true." */ Probably all of them, except of ETHTOOL_PODL_PSE_PW_D_STATUS_SEARCHING can be potentially implemented in this driver on top of regulator framework. > But what about the other way around? Something has already turned the > regulator on, e.g. the bootloader. Should the default be > ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED even thought power is being > delivered? Do we want to put the regulator into the off state at > probe, so it is in a well defined state? Or set priv->admin_state to > whatever regulator_is_enabled() indicates? Good question. I assume, automotive folks would love be able to enable regulator in the boot loader on the PSE to let the Powered Device boot parallel to the PSE. Regards, Oleksij
On Fri, Aug 19, 2022 at 11:32:00PM +0200, Andrew Lunn wrote: > On Fri, Aug 19, 2022 at 02:01:06PM +0200, Oleksij Rempel wrote: > > Add generic driver to support simple Power Sourcing Equipment without > > automatic classification support. > > > > This driver was tested on 10Bast-T1L switch with regulator based PoDL PSE. > > Do you have access to a PHY which implements clause 45.2.9? That seems > like a better reference implementation? Suddenly I do not have access to any of them. > I don't know the market, what is more likely, a simple regulator, or > something more capable with an interface like 45.2.9? So far I was not able to find any PoDL PES IC (with classification support). Or PHY with PoDL PSE on one package. If some one know any, please let me know. > netlink does allow us to keep adding more attributes, so we don't need > to be perfect first time, but it seems like 45.2.9 is what IEEE expect > vendors to provide, so at some point Linux should implement it. ack. Regards, Oleksij
On Fri, Aug 19, 2022 at 11:15:09PM +0200, Andrew Lunn wrote: > > $ ip l > > ... > > 5: t1l1@eth0: <BROADCAST,MULTICAST> .. > > ... > > > > $ ethtool --show-pse t1l1 > > PSE attributs for t1l1: > > PoDL PSE Admin State: disabled > > PoDL PSE Power Detection Status: disabled > > > > $ ethtool --set-pse t1l1 podl-pse-admin-control enable > > $ ethtool --show-pse t1l1 > > PSE attributs for t1l1: > > PoDL PSE Admin State: enabled > > PoDL PSE Power Detection Status: delivering power > > Here you seem to indicate that delivering power is totally independent > of the interface admin status, <BROADCAST,MULTICAST>. The interface is > admin down, yet you can make it deliver power. I thought there might > be a link between interface admin status and power? Do the standards > say anything about this? Is there some sort of industrial norm? > > I'm also wondering about the defaults. It seems like the defaults you > are proposing is power is off by default, and you have to use ethtool > to enable power. That does not seem like the most friendly > settings. Why not an 'auto' mode where if the PHY has PoDL PSE > capabilities, on ifup it is enabled, on ifdown it is disabled? And you > can put it into a 'manual' mode where you control it independent of > administrative status of the interface? Hm. I would say, safe option is to enable PSE manually. Here are my reasons: - some system may require to have power be enabled on boot, before we start to care about administrative state of the interface. - in some cases powered device should stay enabled, even if we do ifup/ifdown I assume, safe defaults should be: - keep PSE always off, except system was configured to enable it on boot. - keep PSE on after it was enabled, even on if up/down - bind PSE admin state to the interface state only if user explicitly requested it. At this round is only default, manual mode is implemented. Automatic mode can be added later if needed. These are my points, but i'm open for discussion. Regards, Oleksij
On Fri, Aug 19, 2022 at 02:01:09PM +0200, Oleksij Rempel wrote: > Add interface to support Power Sourcing Equipment. At current step it > provides generic way to address all variants of PSE devices as defined > in IEEE 802.3-2018 but support only objects specified for IEEE 802.3-2018 104.4 > PoDL Power Sourcing Equipment (PSE). > > Currently supported and mandatory objects are: > IEEE 802.3-2018 30.15.1.1.3 aPoDLPSEPowerDetectionStatus > IEEE 802.3-2018 30.15.1.1.2 aPoDLPSEAdminState > IEEE 802.3-2018 30.15.1.2.1 acPoDLPSEAdminControl > > This is minimal interface needed to control PSE on each separate > ethernet port but it provides not all mandatory objects specified in > IEEE 802.3-2018. > +static int pse_get_pse_attributs(struct net_device *dev, > + struct pse_reply_data *data) > +{ > + struct phy_device *phydev = dev->phydev; > + int ret; > + > + if (!phydev) > + return -EOPNOTSUPP; > + > + mutex_lock(&phydev->lock); > + if (!phydev->psec) { > + ret = -EOPNOTSUPP; > + goto error_unlock; > + } > + > + ret = pse_podl_get_admin_sate(phydev->psec); > + if (ret < 0) > + goto error_unlock; > + > + data->podl_pse_admin_state = ret; > + > + ret = pse_podl_get_pw_d_status(phydev->psec); > + if (ret < 0) > + goto error_unlock; > + > + data->podl_pse_pw_d_status = ret; I'm wondering how this is going to scale. At some point, i expect there will be an implementation that follows C45.2.9. I see 14 values which could be returned. I don't think 14 ops in the driver structure makes sense. Plus c30.15.1 defines other values. The nice thing about netlink is you can have as many or are little attributes in the message as you want. For cable testing, i made use of this. There is no standardisation, different PHYs offer different sorts of results. So i made the API flexible. The PHY puts whatever results it has into the message, and ethtool(1) just walks the message and prints what is in it. I'm wondering if we want a similar sort of API here? net/ethtool/pse-pd.c allocates the netlink messages, adds the header, and then passes it to the driver. The driver then uses helpers from ethtool to add whatever attributes it wants to the message. pse-pd then completes the message, and returns it to user space? This seems like it will scale better. Andrew
> +static int pse_get_pse_attributs(struct net_device *dev, > + struct pse_reply_data *data) > +{ > + struct phy_device *phydev = dev->phydev; > + int ret; > + > + if (!phydev) > + return -EOPNOTSUPP; > + > + mutex_lock(&phydev->lock); > + if (!phydev->psec) { > + ret = -EOPNOTSUPP; > + goto error_unlock; > + } > + > + ret = pse_podl_get_admin_sate(phydev->psec); > + if (ret < 0) > + goto error_unlock; The locking is triggering all sorts of questions in my mind... I don't have the answers yet, so consider this more a discussion. You need somewhere to place the psec. Since we are talking power over copper lines, there will be some sort of PHY, so phydev->psec seems reasonable. However, there is a general trend of moving all DSA Ethernet switches to phylink, which is going to make this a bit tricker to actually get to the phydev object. But using phydev->lock? Humm. At least in the PoE world, there seems to be lots of I2C or SPI controllers. Why hold the phydev lock when performing an I2C transaction? You have a generic linux regulator driver. How would you see a generic C45.2.9 driver? If it calls in the PHY driver, the lock is already held, and we have to be careful to not deadlock. I'm more thinking along the lines of psec should have a lock of its own. pse_podl_get_admin_state(), pse_podl_get_pw_d_status() etc should take that mutex before calling to the actual driver. For a PHY which actually supports C45.2.9, i hope that the phylib core looks at the phy driver structure, sees that some pse_podl ops are implemented, and instantiates and registers a psec object. The phylib core provides wrappers, which take the phylib lock before calling into the driver. And if the PHY strictly follows C45.2.9, the calls are actually into phylib helpers. Otherwise the PHY driver can do its own implementation. Andrew
On Sat, Aug 20, 2022 at 08:16:18PM +0200, Andrew Lunn wrote: > On Fri, Aug 19, 2022 at 02:01:09PM +0200, Oleksij Rempel wrote: > > Add interface to support Power Sourcing Equipment. At current step it > > provides generic way to address all variants of PSE devices as defined > > in IEEE 802.3-2018 but support only objects specified for IEEE 802.3-2018 104.4 > > PoDL Power Sourcing Equipment (PSE). > > > > Currently supported and mandatory objects are: > > IEEE 802.3-2018 30.15.1.1.3 aPoDLPSEPowerDetectionStatus > > IEEE 802.3-2018 30.15.1.1.2 aPoDLPSEAdminState > > IEEE 802.3-2018 30.15.1.2.1 acPoDLPSEAdminControl > > > > This is minimal interface needed to control PSE on each separate > > ethernet port but it provides not all mandatory objects specified in > > IEEE 802.3-2018. > > > +static int pse_get_pse_attributs(struct net_device *dev, > > + struct pse_reply_data *data) > > +{ > > + struct phy_device *phydev = dev->phydev; > > + int ret; > > + > > + if (!phydev) > > + return -EOPNOTSUPP; > > + > > + mutex_lock(&phydev->lock); > > + if (!phydev->psec) { > > + ret = -EOPNOTSUPP; > > + goto error_unlock; > > + } > > + > > + ret = pse_podl_get_admin_sate(phydev->psec); > > + if (ret < 0) > > + goto error_unlock; > > + > > + data->podl_pse_admin_state = ret; > > + > > + ret = pse_podl_get_pw_d_status(phydev->psec); > > + if (ret < 0) > > + goto error_unlock; > > + > > + data->podl_pse_pw_d_status = ret; > > I'm wondering how this is going to scale. At some point, i expect > there will be an implementation that follows C45.2.9. I see 14 values > which could be returned. I don't think 14 ops in the driver structure > makes sense. Plus c30.15.1 defines other values. > > The nice thing about netlink is you can have as many or are little > attributes in the message as you want. For cable testing, i made use > of this. There is no standardisation, different PHYs offer different > sorts of results. So i made the API flexible. The PHY puts whatever > results it has into the message, and ethtool(1) just walks the message > and prints what is in it. > > I'm wondering if we want a similar sort of API here? > net/ethtool/pse-pd.c allocates the netlink messages, adds the header, > and then passes it to the driver. The driver then uses helpers from > ethtool to add whatever attributes it wants to the message. pse-pd > then completes the message, and returns it to user space? This seems > like it will scale better. Yes. Sounds good. I'll make a new version. Regards, Oleksij