| Message ID | 20200417192858.6997-3-michael@walle.cc (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [net-next,1/3] net: phy: broadcom: add helper to write/read RDB registers | expand |
> +/* Check if one PHY has already done the init of the parts common to all PHYs > + * in the Quad PHY package. > + */ > +static bool bcm54140_is_pkg_init(struct phy_device *phydev) > +{ > + struct mdio_device **map = phydev->mdio.bus->mdio_map; > + struct bcm54140_phy_priv *priv; > + struct phy_device *phy; > + int i, addr; > + > + /* Quad PHY */ > + for (i = 0; i < 4; i++) { > + priv = phydev->priv; > + addr = priv->base_addr + i; > + > + if (!map[addr]) > + continue; > + > + phy = container_of(map[addr], struct phy_device, mdio); I don't particularly like a PHY driver having knowledge of the mdio bus core. Please add a helper in the core to get you the phydev for a particular address. There is also the question of locking. What happens if the PHY devices is unbound while you have an instance of its phydev? What happens if the base PHY is unbound? Are the three others then unusable? I think we need to take a step back and look at how we handle quad PHYs in general. The VSC8584 has many of the same issues. Andrew
Am 2020-04-17 21:50, schrieb Andrew Lunn: >> +/* Check if one PHY has already done the init of the parts common to >> all PHYs >> + * in the Quad PHY package. >> + */ >> +static bool bcm54140_is_pkg_init(struct phy_device *phydev) >> +{ >> + struct mdio_device **map = phydev->mdio.bus->mdio_map; >> + struct bcm54140_phy_priv *priv; >> + struct phy_device *phy; >> + int i, addr; >> + >> + /* Quad PHY */ >> + for (i = 0; i < 4; i++) { >> + priv = phydev->priv; >> + addr = priv->base_addr + i; >> + >> + if (!map[addr]) >> + continue; >> + >> + phy = container_of(map[addr], struct phy_device, mdio); > > I don't particularly like a PHY driver having knowledge of the mdio > bus core. Please add a helper in the core to get you the phydev for a > particular address. > > There is also the question of locking. What happens if the PHY devices > is unbound while you have an instance of its phydev? What happens if > the base PHY is unbound? Are the three others then unusable? > > I think we need to take a step back and look at how we handle quad > PHYs in general. The VSC8584 has many of the same issues. Correct, and this function was actually stolen from there ;) This was actually stolen from the mscc PHY ;) -michael
> Correct, and this function was actually stolen from there ;) This was > actually stolen from the mscc PHY ;) Which in itself indicates it is time to make it a helper :-) Andrew
Am 2020-04-17 22:13, schrieb Andrew Lunn: >> Correct, and this function was actually stolen from there ;) This was >> actually stolen from the mscc PHY ;) > > Which in itself indicates it is time to make it a helper :-) Sure, do you have any suggestions? -michael
On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote: > Am 2020-04-17 22:13, schrieb Andrew Lunn: > > > Correct, and this function was actually stolen from there ;) This was > > > actually stolen from the mscc PHY ;) > > > > Which in itself indicates it is time to make it a helper :-) > > Sure, do you have any suggestions? mdiobus_get_phy() does the bit i was complaining about, the mdiobus internal knowledge. Andrew
On 4/17/20 12:28 PM, Michael Walle wrote: > The PHY supports monitoring its die temperature as well as two analog > voltages. Add support for it. > > Signed-off-by: Michael Walle <michael@walle.cc> This will probably fail to compile if HWMON is not enabled or if it is built as module and this driver is built into the kernel. > --- > Documentation/hwmon/bcm54140.rst | 45 ++++ > Documentation/hwmon/index.rst | 1 + > drivers/net/phy/bcm54140.c | 377 +++++++++++++++++++++++++++++++ > 3 files changed, 423 insertions(+) > create mode 100644 Documentation/hwmon/bcm54140.rst > > diff --git a/Documentation/hwmon/bcm54140.rst b/Documentation/hwmon/bcm54140.rst > new file mode 100644 > index 000000000000..bc6ea4b45966 > --- /dev/null > +++ b/Documentation/hwmon/bcm54140.rst > @@ -0,0 +1,45 @@ > +.. SPDX-License-Identifier: GPL-2.0-only > + > +Broadcom BCM54140 Quad SGMII/QSGMII PHY > +======================================= > + > +Supported chips: > + > + * Broadcom BCM54140 > + > + Datasheet: not public > + > +Author: Michael Walle <michael@walle.cc> > + > +Description > +----------- > + > +The Broadcom BCM54140 is a Quad SGMII/QSGMII PHY which supports monitoring > +its die temperature as well as two analog voltages. > + > +The AVDDL is a 1.0V analogue voltage, the AVDDH is a 3.3V analogue voltage. > +Both voltages and the temperature are measured in a round-robin fashion. > + > +Sysfs entries > +------------- > + > +The following attributes are supported. > + > +======================= ======================================================== > +in0_label "AVDDL" > +in0_input Measured AVDDL voltage. > +in0_min Minimum AVDDL voltage. > +in0_max Maximum AVDDL voltage. > +in0_alarm AVDDL voltage alarm. > + > +in1_label "AVDDH" > +in1_input Measured AVDDH voltage. > +in1_min Minimum AVDDH voltage. > +in1_max Maximum AVDDH voltage. > +in1_alarm AVDDH voltage alarm. > + > +temp1_input Die temperature. > +temp1_min Minimum die temperature. > +temp1_max Maximum die temperature. > +temp1_alarm Die temperature alarm. > +======================= ======================================================== > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > index f022583f96f6..19ad0846736d 100644 > --- a/Documentation/hwmon/index.rst > +++ b/Documentation/hwmon/index.rst > @@ -42,6 +42,7 @@ Hardware Monitoring Kernel Drivers > asb100 > asc7621 > aspeed-pwm-tacho > + bcm54140 > bel-pfe > coretemp > da9052 > diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c > index 97465491b41b..97c364ce05e3 100644 > --- a/drivers/net/phy/bcm54140.c > +++ b/drivers/net/phy/bcm54140.c > @@ -6,6 +6,7 @@ > > #include <linux/bitfield.h> > #include <linux/brcmphy.h> > +#include <linux/hwmon.h> > #include <linux/module.h> > #include <linux/phy.h> > > @@ -50,6 +51,54 @@ > #define BCM54140_RDB_TOP_IMR_PORT1 BIT(5) > #define BCM54140_RDB_TOP_IMR_PORT2 BIT(6) > #define BCM54140_RDB_TOP_IMR_PORT3 BIT(7) > +#define BCM54140_RDB_MON_CTRL 0x831 /* monitor control */ > +#define BCM54140_RDB_MON_CTRL_V_MODE BIT(3) /* voltage mode */ > +#define BCM54140_RDB_MON_CTRL_SEL_MASK GENMASK(2, 1) > +#define BCM54140_RDB_MON_CTRL_SEL_TEMP 0 /* meassure temperature */ > +#define BCM54140_RDB_MON_CTRL_SEL_1V0 1 /* meassure AVDDL 1.0V */ > +#define BCM54140_RDB_MON_CTRL_SEL_3V3 2 /* meassure AVDDH 3.3V */ > +#define BCM54140_RDB_MON_CTRL_SEL_RR 3 /* meassure all round-robin */ > +#define BCM54140_RDB_MON_CTRL_PWR_DOWN BIT(0) /* power-down monitor */ > +#define BCM54140_RDB_MON_TEMP_VAL 0x832 /* temperature value */ > +#define BCM54140_RDB_MON_TEMP_MAX 0x833 /* temperature high thresh */ > +#define BCM54140_RDB_MON_TEMP_MIN 0x834 /* temperature low thresh */ > +#define BCM54140_RDB_MON_TEMP_DATA_MASK GENMASK(9, 0) > +#define BCM54140_RDB_MON_1V0_VAL 0x835 /* AVDDL 1.0V value */ > +#define BCM54140_RDB_MON_1V0_MAX 0x836 /* AVDDL 1.0V high thresh */ > +#define BCM54140_RDB_MON_1V0_MIN 0x837 /* AVDDL 1.0V low thresh */ > +#define BCM54140_RDB_MON_1V0_DATA_MASK GENMASK(10, 0) > +#define BCM54140_RDB_MON_3V3_VAL 0x838 /* AVDDH 3.3V value */ > +#define BCM54140_RDB_MON_3V3_MAX 0x839 /* AVDDH 3.3V high thresh */ > +#define BCM54140_RDB_MON_3V3_MIN 0x83a /* AVDDH 3.3V low thresh */ > +#define BCM54140_RDB_MON_3V3_DATA_MASK GENMASK(11, 0) > +#define BCM54140_RDB_MON_ISR 0x83b /* interrupt status */ > +#define BCM54140_RDB_MON_ISR_3V3 BIT(2) /* AVDDH 3.3V alarm */ > +#define BCM54140_RDB_MON_ISR_1V0 BIT(1) /* AVDDL 1.0V alarm */ > +#define BCM54140_RDB_MON_ISR_TEMP BIT(0) /* temperature alarm */ > + > +/* According to the datasheet the formula is: > + * T = 413.35 - (0.49055 * bits[9:0]) > + */ > +#define BCM54140_HWMON_TO_TEMP(v) (413350L - (v) * 491) > +#define BCM54140_HWMON_FROM_TEMP(v) DIV_ROUND_CLOSEST_ULL(413350L - (v), 491) > + > +/* According to the datasheet the formula is: > + * U = bits[11:0] / 1024 * 220 / 0.2 > + * > + * Normalized: > + * U = bits[11:0] / 4096 * 2514 > + */ > +#define BCM54140_HWMON_TO_IN_1V0(v) ((v) * 2514 >> 11) > +#define BCM54140_HWMON_FROM_IN_1V0(v) DIV_ROUND_CLOSEST_ULL(((v) << 11), 2514) > + > +/* According to the datasheet the formula is: > + * U = bits[10:0] / 1024 * 880 / 0.7 > + * > + * Normalized: > + * U = bits[10:0] / 2048 * 4400 > + */ > +#define BCM54140_HWMON_TO_IN_3V3(v) ((v) * 4400 >> 12) > +#define BCM54140_HWMON_FROM_IN_3V3(v) DIV_ROUND_CLOSEST_ULL(((v) << 12), 4400) > > #define BCM54140_DEFAULT_DOWNSHIFT 5 > #define BCM54140_MAX_DOWNSHIFT 9 > @@ -57,6 +106,258 @@ > struct bcm54140_phy_priv { > int port; > int base_addr; > + bool pkg_init; > + u16 alarm; > +}; > + > +static umode_t bcm54140_hwmon_is_visible(const void *data, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_in: > + switch (attr) { > + case hwmon_in_min: > + case hwmon_in_max: > + return 0644; > + case hwmon_in_label: > + case hwmon_in_input: > + case hwmon_in_alarm: > + return 0444; > + default: > + return 0; > + } > + case hwmon_temp: > + switch (attr) { > + case hwmon_temp_min: > + case hwmon_temp_max: > + return 0644; > + case hwmon_temp_input: > + case hwmon_temp_alarm: > + return 0444; > + default: > + return 0; > + } > + default: > + return 0; > + } > +} > + > +static int bcm54140_hwmon_read_alarm(struct device *dev, unsigned int bit, > + long *val) > +{ > + struct phy_device *phydev = dev_get_drvdata(dev); > + struct bcm54140_phy_priv *priv = phydev->priv; > + u16 tmp; > + > + /* latch any alarm bits */ > + tmp = bcm_phy_read_rdb(phydev, BCM54140_RDB_MON_ISR); > + if (tmp < 0) > + return tmp; > + priv->alarm |= tmp; > + > + *val = !!(priv->alarm & bit); > + priv->alarm &= ~bit;> + > + return 0; > +} > + > +static int bcm54140_hwmon_read_temp(struct device *dev, u32 attr, > + int channel, long *val) > +{ > + struct phy_device *phydev = dev_get_drvdata(dev); > + u16 reg, tmp; > + > + switch (attr) { > + case hwmon_temp_input: > + reg = BCM54140_RDB_MON_TEMP_VAL; > + break; > + case hwmon_temp_min: > + reg = BCM54140_RDB_MON_TEMP_MIN; > + break; > + case hwmon_temp_max: > + reg = BCM54140_RDB_MON_TEMP_MAX; > + break; > + case hwmon_temp_alarm: > + return bcm54140_hwmon_read_alarm(dev, > + BCM54140_RDB_MON_ISR_TEMP, > + val); > + default: > + return -EOPNOTSUPP; > + } > + > + tmp = bcm_phy_read_rdb(phydev, reg); > + if (tmp < 0) > + return tmp; > + > + *val = BCM54140_HWMON_TO_TEMP(tmp & BCM54140_RDB_MON_TEMP_DATA_MASK); > + > + return 0; > +} > + > +static int bcm54140_hwmon_read_in(struct device *dev, u32 attr, > + int channel, long *val) > +{ > + struct phy_device *phydev = dev_get_drvdata(dev); > + u16 mask = (!channel) ? BCM54140_RDB_MON_1V0_DATA_MASK > + : BCM54140_RDB_MON_3V3_DATA_MASK; > + u16 bit, reg, tmp; > + > + switch (attr) { > + case hwmon_in_input: > + reg = (!channel) ? BCM54140_RDB_MON_1V0_VAL > + : BCM54140_RDB_MON_3V3_VAL; I am personally neither a friend of unnecessary () nor of unnecessary negations. Why not the following ? reg = channel ? BCM54140_RDB_MON_3V3_VAL : BCM54140_RDB_MON_1V0_VAL; Another option would be to read all those values from a set of defines, given the expressions are repeated several times. > + break; > + case hwmon_in_min: > + reg = (!channel) ? BCM54140_RDB_MON_1V0_MIN > + : BCM54140_RDB_MON_3V3_MIN; > + break; > + case hwmon_in_max: > + reg = (!channel) ? BCM54140_RDB_MON_1V0_MAX > + : BCM54140_RDB_MON_3V3_MAX; > + break; > + case hwmon_in_alarm: > + bit = (!channel) ? BCM54140_RDB_MON_ISR_1V0 > + : BCM54140_RDB_MON_ISR_3V3; > + return bcm54140_hwmon_read_alarm(dev, bit, val); > + default: > + return -EOPNOTSUPP; > + } > + > + tmp = bcm_phy_read_rdb(phydev, reg); > + if (tmp < 0) > + return tmp; > + > + if (!channel) > + *val = BCM54140_HWMON_TO_IN_1V0(tmp & mask); > + else > + *val = BCM54140_HWMON_TO_IN_3V3(tmp & mask); > + > + return 0; > +} > + > +static int bcm54140_hwmon_read(struct device *dev, > + enum hwmon_sensor_types type, u32 attr, > + int channel, long *val) > +{ > + switch (type) { > + case hwmon_temp: > + return bcm54140_hwmon_read_temp(dev, attr, channel, val); > + case hwmon_in: > + return bcm54140_hwmon_read_in(dev, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static const char *const bcm54140_hwmon_in_labels[] = { > + "AVDDL", > + "AVDDH", > +}; > + > +static int bcm54140_hwmon_read_string(struct device *dev, > + enum hwmon_sensor_types type, u32 attr, > + int channel, const char **str) > +{ > + switch (type) { > + case hwmon_in: > + switch (attr) { > + case hwmon_in_label: > + *str = bcm54140_hwmon_in_labels[channel]; > + return 0; > + default: > + return -EOPNOTSUPP; > + } > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int bcm54140_hwmon_write_temp(struct device *dev, u32 attr, > + int channel, long val) > +{ > + struct phy_device *phydev = dev_get_drvdata(dev); > + u16 reg; > + > + switch (attr) { > + case hwmon_temp_min: > + reg = BCM54140_RDB_MON_TEMP_MIN; > + break; > + case hwmon_temp_max: > + reg = BCM54140_RDB_MON_TEMP_MAX; > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + return bcm_phy_modify_rdb(phydev, reg, BCM54140_RDB_MON_TEMP_DATA_MASK, > + BCM54140_HWMON_FROM_TEMP(val)); > +} > + > +static int bcm54140_hwmon_write_in(struct device *dev, u32 attr, > + int channel, long val) > +{ > + struct phy_device *phydev = dev_get_drvdata(dev); > + u16 mask = (!channel) ? BCM54140_RDB_MON_1V0_DATA_MASK > + : BCM54140_RDB_MON_3V3_DATA_MASK; > + unsigned long raw = (!channel) ? BCM54140_HWMON_FROM_IN_1V0(val) > + : BCM54140_HWMON_FROM_IN_3V3(val); > + u16 reg; > + > + raw = clamp_val(raw, 0, mask); > + > + switch (attr) { > + case hwmon_in_min: > + reg = (!channel) ? BCM54140_RDB_MON_1V0_MIN > + : BCM54140_RDB_MON_3V3_MIN; > + break; > + case hwmon_in_max: > + reg = (!channel) ? BCM54140_RDB_MON_1V0_MAX > + : BCM54140_RDB_MON_3V3_MAX; > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + return bcm_phy_modify_rdb(phydev, reg, mask, raw); > +} > + > +static int bcm54140_hwmon_write(struct device *dev, > + enum hwmon_sensor_types type, u32 attr, > + int channel, long val) > +{ > + switch (type) { > + case hwmon_temp: > + return bcm54140_hwmon_write_temp(dev, attr, channel, val); > + case hwmon_in: > + return bcm54140_hwmon_write_in(dev, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static const struct hwmon_channel_info *bcm54140_hwmon_info[] = { > + HWMON_CHANNEL_INFO(temp, > + HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | > + HWMON_T_ALARM), > + HWMON_CHANNEL_INFO(in, > + HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | > + HWMON_I_ALARM | HWMON_I_LABEL, > + HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | > + HWMON_I_ALARM | HWMON_I_LABEL), > + NULL > +}; > + > +static const struct hwmon_ops bcm54140_hwmon_ops = { > + .is_visible = bcm54140_hwmon_is_visible, > + .read = bcm54140_hwmon_read, > + .read_string = bcm54140_hwmon_read_string, > + .write = bcm54140_hwmon_write, > +}; > + > +static const struct hwmon_chip_info bcm54140_chip_info = { > + .ops = &bcm54140_hwmon_ops, > + .info = bcm54140_hwmon_info, > }; > > static int bcm54140_phy_base_read_rdb(struct phy_device *phydev, u16 rdb) > @@ -203,6 +504,74 @@ static int bcm54140_get_base_addr_and_port(struct phy_device *phydev) > return 0; > } > > +/* Check if one PHY has already done the init of the parts common to all PHYs > + * in the Quad PHY package. > + */ > +static bool bcm54140_is_pkg_init(struct phy_device *phydev) > +{ > + struct mdio_device **map = phydev->mdio.bus->mdio_map; > + struct bcm54140_phy_priv *priv; > + struct phy_device *phy; > + int i, addr; > + > + /* Quad PHY */ > + for (i = 0; i < 4; i++) { > + priv = phydev->priv; > + addr = priv->base_addr + i; > + > + if (!map[addr]) > + continue; > + > + phy = container_of(map[addr], struct phy_device, mdio); > + > + if ((phy->phy_id & phydev->drv->phy_id_mask) != > + (phydev->drv->phy_id & phydev->drv->phy_id_mask)) > + continue; > + > + priv = phy->priv; > + > + if (priv && priv->pkg_init) > + return true; > + } > + > + return false; > +} > + > +static int bcm54140_enable_monitoring(struct phy_device *phydev) > +{ > + u16 mask, set; > + > + /* 3.3V voltage mode */ > + set = BCM54140_RDB_MON_CTRL_V_MODE; > + > + /* select round-robin */ > + mask = BCM54140_RDB_MON_CTRL_SEL_MASK; > + set |= FIELD_PREP(BCM54140_RDB_MON_CTRL_SEL_MASK, > + BCM54140_RDB_MON_CTRL_SEL_RR); > + > + /* remove power-down bit */ > + mask |= BCM54140_RDB_MON_CTRL_PWR_DOWN; > + > + return bcm_phy_modify_rdb(phydev, BCM54140_RDB_MON_CTRL, mask, set); > +} > + > +static int bcm54140_phy_probe_once(struct phy_device *phydev) > +{ > + struct device *hwmon; > + int ret; > + > + /* enable hardware monitoring */ > + ret = bcm54140_enable_monitoring(phydev); > + if (ret) > + return ret; > + > + hwmon = devm_hwmon_device_register_with_info(&phydev->mdio.dev, > + "BCM54140", phydev, > + &bcm54140_chip_info, > + NULL); > + return PTR_ERR_OR_ZERO(hwmon); > +} > + > static int bcm54140_phy_probe(struct phy_device *phydev) > { > struct bcm54140_phy_priv *priv; > @@ -218,6 +587,14 @@ static int bcm54140_phy_probe(struct phy_device *phydev) > if (ret) > return ret; > > + if (!bcm54140_is_pkg_init(phydev)) { > + ret = bcm54140_phy_probe_once(phydev); > + if (ret) > + return ret; > + } > + > + priv->pkg_init = true; > + > dev_info(&phydev->mdio.dev, > "probed (port %d, base PHY address %d)\n", > priv->port, priv->base_addr); >
Am 2020-04-17 23:28, schrieb Andrew Lunn: > On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote: >> Am 2020-04-17 22:13, schrieb Andrew Lunn: >> > > Correct, and this function was actually stolen from there ;) This was >> > > actually stolen from the mscc PHY ;) >> > >> > Which in itself indicates it is time to make it a helper :-) >> >> Sure, do you have any suggestions? > > mdiobus_get_phy() does the bit i was complaining about, the mdiobus > internal knowledge. But that doesn't address your other comment. > There is also the question of locking. What happens if the PHY devices > is unbound while you have an instance of its phydev? Is there any lock one could take to avoid that? > What happens if the base PHY is unbound? Are the three others then > unusable? In my case, this would mean the hwmon device is also removed. I don't see any other way to do it right now. I guess it would be better to have the hwmon device registered to some kind of parent device. > I think we need to take a step back and look at how we handle quad > PHYs in general. The VSC8584 has many of the same issues. For the BCM54140 there are three different functions: (1) PHY functions accessible by the PHYs own address (ie PHY status/control) (2) PHY functions but only accessible by the global registers (ie interrupt enables per PHY of the shared interrupt pin) (3) global functions (like sensors, global configuration) (1) is already supported in the current PHY framework. (2) and (3) need the "hack" which uses mdiobus_read/write() with the base address. -michael
On Sun, Apr 19, 2020 at 12:29:23PM +0200, Michael Walle wrote: > Am 2020-04-17 23:28, schrieb Andrew Lunn: > > On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote: > > > Am 2020-04-17 22:13, schrieb Andrew Lunn: > > > > > Correct, and this function was actually stolen from there ;) This was > > > > > actually stolen from the mscc PHY ;) > > > > > > > > Which in itself indicates it is time to make it a helper :-) > > > > > > Sure, do you have any suggestions? > > > > mdiobus_get_phy() does the bit i was complaining about, the mdiobus > > internal knowledge. > > But that doesn't address your other comment. Yes, you are right. But i don't think you can easily generalize the rest. It needs knowledge of the driver private structure to reference pkg_init. You would have to move that into phy_device. > > > There is also the question of locking. What happens if the PHY devices > > is unbound while you have an instance of its phydev? > > Is there any lock one could take to avoid that? phy_attach_direct() does a get_device(). That at least means the struct device will not go away. I don't know the code well enough to know if that will also stop the phy_device structure from being freed. We might need mdiobus_get_phy() to also do a get_device(), and add a mdiobus_put_phy() which does a put_device(). > > What happens if the base PHY is unbound? Are the three others then > > unusable? > > In my case, this would mean the hwmon device is also removed. I don't > see any other way to do it right now. I guess it would be better to > have the hwmon device registered to some kind of parent device. The phydev structure might go away. But the hardware is still there. You can access it via address on the bus. What you have to be careful of is using the phydev for a different phy. > For the BCM54140 there are three different functions: > (1) PHY functions accessible by the PHYs own address (ie PHY > status/control) > (2) PHY functions but only accessible by the global registers (ie > interrupt enables per PHY of the shared interrupt pin) > (3) global functions (like sensors, global configuration) > > (1) is already supported in the current PHY framework. (2) and (3) > need the "hack" which uses mdiobus_read/write() with the base > address. Is the _is_pkg_init() function the only place you need to access some other phy_device structure. Maybe we need a phydev->shared structure, which all PHYs in one package share? Get the core to do reference counting on the structure? Add helpers phy_read_shared(), phy_write_shared(), etc, which does MDIO accesses on the base device, taking care of the locking. pkg_init is a member of this shared structure. And have a void * priv in shared for shared driver private data? Just "thinking out loud" Andrew
Am 2020-04-19 18:29, schrieb Andrew Lunn: > On Sun, Apr 19, 2020 at 12:29:23PM +0200, Michael Walle wrote: >> Am 2020-04-17 23:28, schrieb Andrew Lunn: >> > On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote: >> > > Am 2020-04-17 22:13, schrieb Andrew Lunn: >> > > > > Correct, and this function was actually stolen from there ;) This was >> > > > > actually stolen from the mscc PHY ;) >> > > > >> > > > Which in itself indicates it is time to make it a helper :-) >> > > >> > > Sure, do you have any suggestions? >> > >> > mdiobus_get_phy() does the bit i was complaining about, the mdiobus >> > internal knowledge. >> >> But that doesn't address your other comment. > > Yes, you are right. But i don't think you can easily generalize the > rest. It needs knowledge of the driver private structure to reference > pkg_init. You would have to move that into phy_device. > >> >> > There is also the question of locking. What happens if the PHY devices >> > is unbound while you have an instance of its phydev? >> >> Is there any lock one could take to avoid that? > > phy_attach_direct() does a get_device(). That at least means the > struct device will not go away. I don't know the code well enough to > know if that will also stop the phy_device structure from being freed. > We might need mdiobus_get_phy() to also do a get_device(), and add a > mdiobus_put_phy() which does a put_device(). > >> > What happens if the base PHY is unbound? Are the three others then >> > unusable? >> >> In my case, this would mean the hwmon device is also removed. I don't >> see any other way to do it right now. I guess it would be better to >> have the hwmon device registered to some kind of parent device. > > The phydev structure might go away. But the hardware is still > there. You can access it via address on the bus. What you have to be > careful of is using the phydev for a different phy. But the hwmon is registered to the device of the PHY which might be unbound. So it will also be removed, correct? FWIW I don't think that is likely to happen in my case ;) > >> For the BCM54140 there are three different functions: >> (1) PHY functions accessible by the PHYs own address (ie PHY >> status/control) >> (2) PHY functions but only accessible by the global registers (ie >> interrupt enables per PHY of the shared interrupt pin) >> (3) global functions (like sensors, global configuration) >> >> (1) is already supported in the current PHY framework. (2) and (3) >> need the "hack" which uses mdiobus_read/write() with the base >> address. > > Is the _is_pkg_init() function the only place you need to access some > other phy_device structure. yes. > Maybe we need a phydev->shared structure, which all PHYs in one > package share? That came to my mind too. But how could the PHY core find out which shared structure belongs to which phydev? I guess the phydev have to find out, but then how does it tell the PHY core that it wants such a shared structure. Have the (base) PHY address as an identifier? > Get the core to do reference counting on the structure? > Add helpers phy_read_shared(), phy_write_shared(), etc, which does > MDIO accesses on the base device, taking care of the locking. The "base" access is another thing, I guess, which has nothing to do with the shared structure. Also I presume not every PHY has the base address as some global register access. Eg. this PHY also have "base + 4" (or depending on the configuration base + 3, that is the last PHY of the four) as a special register access. > pkg_init > is a member of this shared structure. And have a void * priv in shared > for shared driver private data? if you have a void *priv, why would you need pkg_init, which is an implementation detail of the phydev. I guess it is enough to just have a void *shared (I don't know about the locking for now). -michael
> > Maybe we need a phydev->shared structure, which all PHYs in one > > package share? > > That came to my mind too. But how could the PHY core find out which > shared structure belongs to which phydev? I guess the phydev have to > find out, but then how does it tell the PHY core that it wants such > a shared structure. Have the (base) PHY address as an identifier? Yes. I was thinking along those lines. phy_package_join(phydev, base) If this is the first call with that value of base, allocate the structure, set the ref count to 1, and set phydev->shared to point to it. For subsequent calls, increment the reference count, and set phydev->shared. phy_package_leave(phydev) Decrement the reference count, and set phydev->shared to NULL. If the reference count goes to 0, free the structure. > > Get the core to do reference counting on the structure? > > Add helpers phy_read_shared(), phy_write_shared(), etc, which does > > MDIO accesses on the base device, taking care of the locking. > > The "base" access is another thing, I guess, which has nothing to do > with the shared structure. I'm making the assumption that all global addresses are at the base address. If we don't want to make that assumption, we need the change the API above so you pass a cookie, and all PHYs need to use the same cookie to identify the package. Maybe base is the wrong name, since MSCC can have the base as the high address of the four, not the low? Still just thinking aloud.... Andrew
On Sun, Apr 19, 2020 at 06:29:28PM +0200, Andrew Lunn wrote: > On Sun, Apr 19, 2020 at 12:29:23PM +0200, Michael Walle wrote: > > Am 2020-04-17 23:28, schrieb Andrew Lunn: > > > On Fri, Apr 17, 2020 at 11:08:56PM +0200, Michael Walle wrote: > > > > Am 2020-04-17 22:13, schrieb Andrew Lunn: > > > > > > Correct, and this function was actually stolen from there ;) This was > > > > > > actually stolen from the mscc PHY ;) > > > > > > > > > > Which in itself indicates it is time to make it a helper :-) > > > > > > > > Sure, do you have any suggestions? > > > > > > mdiobus_get_phy() does the bit i was complaining about, the mdiobus > > > internal knowledge. > > > > But that doesn't address your other comment. > > Yes, you are right. But i don't think you can easily generalize the > rest. It needs knowledge of the driver private structure to reference > pkg_init. You would have to move that into phy_device. > > > > > > There is also the question of locking. What happens if the PHY devices > > > is unbound while you have an instance of its phydev? > > > > Is there any lock one could take to avoid that? > > phy_attach_direct() does a get_device(). That at least means the > struct device will not go away. I don't know the code well enough to > know if that will also stop the phy_device structure from being freed. Well, struct device is embedded in struct mdio_device, which in turn is embedded in struct phy_device. So, if struct device can't go away because its refcount is held, the same is true of the structs embedding it.
Am 2020-04-19 19:05, schrieb Andrew Lunn: >> > Maybe we need a phydev->shared structure, which all PHYs in one >> > package share? >> >> That came to my mind too. But how could the PHY core find out which >> shared structure belongs to which phydev? I guess the phydev have to >> find out, but then how does it tell the PHY core that it wants such >> a shared structure. Have the (base) PHY address as an identifier? > > Yes. I was thinking along those lines. > > phy_package_join(phydev, base) > > If this is the first call with that value of base, allocate the > structure, set the ref count to 1, and set phydev->shared to point to > it. For subsequent calls, increment the reference count, and set > phydev->shared. > > phy_package_leave(phydev) > > Decrement the reference count, and set phydev->shared to NULL. If the > reference count goes to 0, free the structure. > >> > Get the core to do reference counting on the structure? >> > Add helpers phy_read_shared(), phy_write_shared(), etc, which does >> > MDIO accesses on the base device, taking care of the locking. >> >> The "base" access is another thing, I guess, which has nothing to do >> with the shared structure. > > I'm making the assumption that all global addresses are at the base > address. But what does that have to do with the shared structure? I don't think you have to "bundle" the shared structure with the "access the global registers" method. The phy drivers just have to know some common key, which can be anything arbitrary, correct? So we can say its the lowest address, but it could also be any other address, as long as each PHY driver instance can deduce the same key. > If we don't want to make that assumption, we need the change > the API above so you pass a cookie, and all PHYs need to use the same > cookie to identify the package. whats the difference between a PHY address and a cookie, given that the phy core doesn't actually use the phy address for anything. -michael > Maybe base is the wrong name, since MSCC can have the base as the high > address of the four, not the low? > > Still just thinking aloud.... > > Andrew
> But what does that have to do with the shared structure? I don't think > you have to "bundle" the shared structure with the "access the global > registers" method. We don't need to. But it would be a good way to clean up code which locks the mdio bus, does a register access on some other device, and then unlocks the bus. As a general rule of thumb, it is better to have the core do the locking, rather than the driver. Driver writers don't always think about locking, so it is better to give driver writers safe APIs to use. Andrew
Hi Andrew, Am 2020-04-19 23:55, schrieb Andrew Lunn: >> But what does that have to do with the shared structure? I don't think >> you have to "bundle" the shared structure with the "access the global >> registers" method. > > We don't need to. But it would be a good way to clean up code which > locks the mdio bus, does a register access on some other device, and > then unlocks the bus. I'd like do an RFC for that. But how should I proceed with the original patch series? Should I send an updated version; you didn't reply to the LED stuff. That is the last remark for now. > As a general rule of thumb, it is better to have the core do the > locking, rather than the driver. Driver writers don't always think > about locking, so it is better to give driver writers safe APIs to > use. Ok I see, but what locking do you have in mind? We could have something like __phy_package_write(struct phy_device *dev, u32 regnum, u16 val) { return __mdiobus_write(phydev->mdio.bus, phydev->shared->addr, regnum, val); } and its phy_package_write() equivalent. But that would just be convenience functions, nothing where you actually help the user with locking. Am I missing something? >>> Get the core to do reference counting on the structure? >>> Add helpers phy_read_shared(), phy_write_shared(), etc, which does >>> MDIO accesses on the base device, taking care of the locking. >>> >> The "base" access is another thing, I guess, which has nothing to do >> with the shared structure. >> > I'm making the assumption that all global addresses are at the base > address. If we don't want to make that assumption, we need the change > the API above so you pass a cookie, and all PHYs need to use the same > cookie to identify the package. how would a phy driver deduce a common cookie? And how would that be a difference to using a PHY address. > Maybe base is the wrong name, since MSCC can have the base as the high > address of the four, not the low? I'd say it might be any of the four addresses as long as it is the same across the PHYs in the same package. And in that case you can also have the phy_package_read/write() functions. -michael
> Ok I see, but what locking do you have in mind? We could have something > like > > __phy_package_write(struct phy_device *dev, u32 regnum, u16 val) > { > return __mdiobus_write(phydev->mdio.bus, phydev->shared->addr, > regnum, val); > } > > and its phy_package_write() equivalent. But that would just be > convenience functions, nothing where you actually help the user with > locking. Am I missing something? In general, drivers should not be using __foo functions. We want drivers to make use of phy_package_write() which would do the bus locking. Look at a typical PHY driver. There is no locking what so ever. Just lots of phy_read() and phy write(). The locking is done by the core and so should be correct. > > > > Get the core to do reference counting on the structure? > > > > Add helpers phy_read_shared(), phy_write_shared(), etc, which does > > > > MDIO accesses on the base device, taking care of the locking. > > > > > > > The "base" access is another thing, I guess, which has nothing to do > > > with the shared structure. > > > > > I'm making the assumption that all global addresses are at the base > > address. If we don't want to make that assumption, we need the change > > the API above so you pass a cookie, and all PHYs need to use the same > > cookie to identify the package. > > how would a phy driver deduce a common cookie? And how would that be a > difference to using a PHY address. For a cookie, i don't care how the driver decides on the cookie. The core never uses it, other than comparing cookies to combine individual PHYs into a package. It could be a PHY address. It could be the PHY address where the global registers are. Or it could be anything else. > > Maybe base is the wrong name, since MSCC can have the base as the high > > address of the four, not the low? > > I'd say it might be any of the four addresses as long as it is the same > across the PHYs in the same package. And in that case you can also have > the phy_package_read/write() functions. Yes. That is the semantics which is think is most useful. But then we don't have a cookie, the value has real significance, and we need to document what is should mean. Andrew
Am 2020-04-20 17:36, schrieb Andrew Lunn: >> Ok I see, but what locking do you have in mind? We could have >> something >> like >> >> __phy_package_write(struct phy_device *dev, u32 regnum, u16 val) >> { >> return __mdiobus_write(phydev->mdio.bus, phydev->shared->addr, >> regnum, val); >> } >> >> and its phy_package_write() equivalent. But that would just be >> convenience functions, nothing where you actually help the user with >> locking. Am I missing something? > > In general, drivers should not be using __foo functions. We want > drivers to make use of phy_package_write() which would do the bus > locking. Look at a typical PHY driver. There is no locking what so > ever. Just lots of phy_read() and phy write(). The locking is done by > the core and so should be correct. Ok, but for example the BCM54140 uses indirect register access and thus need to lock the mdio bus itself in which case I need the __funcs. >> > > > Get the core to do reference counting on the structure? >> > > > Add helpers phy_read_shared(), phy_write_shared(), etc, which does >> > > > MDIO accesses on the base device, taking care of the locking. >> > > > >> > > The "base" access is another thing, I guess, which has nothing to do >> > > with the shared structure. >> > > >> > I'm making the assumption that all global addresses are at the base >> > address. If we don't want to make that assumption, we need the change >> > the API above so you pass a cookie, and all PHYs need to use the same >> > cookie to identify the package. >> >> how would a phy driver deduce a common cookie? And how would that be a >> difference to using a PHY address. > > For a cookie, i don't care how the driver decides on the cookie. The > core never uses it, other than comparing cookies to combine individual > PHYs into a package. It could be a PHY address. It could be the PHY > address where the global registers are. Or it could be anything else. > >> > Maybe base is the wrong name, since MSCC can have the base as the high >> > address of the four, not the low? >> >> I'd say it might be any of the four addresses as long as it is the >> same >> across the PHYs in the same package. And in that case you can also >> have >> the phy_package_read/write() functions. > > Yes. That is the semantics which is think is most useful. But then we > don't have a cookie, the value has real significance, and we need to > document what is should mean. Agreed. I will post a RFC shortly. -michael
On Mon, Apr 20, 2020 at 05:10:19PM +0200, Michael Walle wrote: > Hi Andrew, > > Am 2020-04-19 23:55, schrieb Andrew Lunn: > > > But what does that have to do with the shared structure? I don't think > > > you have to "bundle" the shared structure with the "access the global > > > registers" method. > > > > We don't need to. But it would be a good way to clean up code which > > locks the mdio bus, does a register access on some other device, and > > then unlocks the bus. > > I'd like do an RFC for that. But how should I proceed with the original > patch series? Should I send an updated version; you didn't reply to the > LED stuff. That is the last remark for now. The LED stuff is something that there isn't a solution for at the moment. There's been talk about coming up with some generic way to describe the PHY LED configuration in DT, but given that almost every PHY has quite different ways to configure LEDs, I fear such a task is virtually impossible. Very few PHYs under Linux have their LEDs operating "correctly" or in a meaningful or sensible way because of this, and it's been this way for years.
diff --git a/Documentation/hwmon/bcm54140.rst b/Documentation/hwmon/bcm54140.rst new file mode 100644 index 000000000000..bc6ea4b45966 --- /dev/null +++ b/Documentation/hwmon/bcm54140.rst @@ -0,0 +1,45 @@ +.. SPDX-License-Identifier: GPL-2.0-only + +Broadcom BCM54140 Quad SGMII/QSGMII PHY +======================================= + +Supported chips: + + * Broadcom BCM54140 + + Datasheet: not public + +Author: Michael Walle <michael@walle.cc> + +Description +----------- + +The Broadcom BCM54140 is a Quad SGMII/QSGMII PHY which supports monitoring +its die temperature as well as two analog voltages. + +The AVDDL is a 1.0V analogue voltage, the AVDDH is a 3.3V analogue voltage. +Both voltages and the temperature are measured in a round-robin fashion. + +Sysfs entries +------------- + +The following attributes are supported. + +======================= ======================================================== +in0_label "AVDDL" +in0_input Measured AVDDL voltage. +in0_min Minimum AVDDL voltage. +in0_max Maximum AVDDL voltage. +in0_alarm AVDDL voltage alarm. + +in1_label "AVDDH" +in1_input Measured AVDDH voltage. +in1_min Minimum AVDDH voltage. +in1_max Maximum AVDDH voltage. +in1_alarm AVDDH voltage alarm. + +temp1_input Die temperature. +temp1_min Minimum die temperature. +temp1_max Maximum die temperature. +temp1_alarm Die temperature alarm. +======================= ======================================================== diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index f022583f96f6..19ad0846736d 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -42,6 +42,7 @@ Hardware Monitoring Kernel Drivers asb100 asc7621 aspeed-pwm-tacho + bcm54140 bel-pfe coretemp da9052 diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c index 97465491b41b..97c364ce05e3 100644 --- a/drivers/net/phy/bcm54140.c +++ b/drivers/net/phy/bcm54140.c @@ -6,6 +6,7 @@ #include <linux/bitfield.h> #include <linux/brcmphy.h> +#include <linux/hwmon.h> #include <linux/module.h> #include <linux/phy.h> @@ -50,6 +51,54 @@ #define BCM54140_RDB_TOP_IMR_PORT1 BIT(5) #define BCM54140_RDB_TOP_IMR_PORT2 BIT(6) #define BCM54140_RDB_TOP_IMR_PORT3 BIT(7) +#define BCM54140_RDB_MON_CTRL 0x831 /* monitor control */ +#define BCM54140_RDB_MON_CTRL_V_MODE BIT(3) /* voltage mode */ +#define BCM54140_RDB_MON_CTRL_SEL_MASK GENMASK(2, 1) +#define BCM54140_RDB_MON_CTRL_SEL_TEMP 0 /* meassure temperature */ +#define BCM54140_RDB_MON_CTRL_SEL_1V0 1 /* meassure AVDDL 1.0V */ +#define BCM54140_RDB_MON_CTRL_SEL_3V3 2 /* meassure AVDDH 3.3V */ +#define BCM54140_RDB_MON_CTRL_SEL_RR 3 /* meassure all round-robin */ +#define BCM54140_RDB_MON_CTRL_PWR_DOWN BIT(0) /* power-down monitor */ +#define BCM54140_RDB_MON_TEMP_VAL 0x832 /* temperature value */ +#define BCM54140_RDB_MON_TEMP_MAX 0x833 /* temperature high thresh */ +#define BCM54140_RDB_MON_TEMP_MIN 0x834 /* temperature low thresh */ +#define BCM54140_RDB_MON_TEMP_DATA_MASK GENMASK(9, 0) +#define BCM54140_RDB_MON_1V0_VAL 0x835 /* AVDDL 1.0V value */ +#define BCM54140_RDB_MON_1V0_MAX 0x836 /* AVDDL 1.0V high thresh */ +#define BCM54140_RDB_MON_1V0_MIN 0x837 /* AVDDL 1.0V low thresh */ +#define BCM54140_RDB_MON_1V0_DATA_MASK GENMASK(10, 0) +#define BCM54140_RDB_MON_3V3_VAL 0x838 /* AVDDH 3.3V value */ +#define BCM54140_RDB_MON_3V3_MAX 0x839 /* AVDDH 3.3V high thresh */ +#define BCM54140_RDB_MON_3V3_MIN 0x83a /* AVDDH 3.3V low thresh */ +#define BCM54140_RDB_MON_3V3_DATA_MASK GENMASK(11, 0) +#define BCM54140_RDB_MON_ISR 0x83b /* interrupt status */ +#define BCM54140_RDB_MON_ISR_3V3 BIT(2) /* AVDDH 3.3V alarm */ +#define BCM54140_RDB_MON_ISR_1V0 BIT(1) /* AVDDL 1.0V alarm */ +#define BCM54140_RDB_MON_ISR_TEMP BIT(0) /* temperature alarm */ + +/* According to the datasheet the formula is: + * T = 413.35 - (0.49055 * bits[9:0]) + */ +#define BCM54140_HWMON_TO_TEMP(v) (413350L - (v) * 491) +#define BCM54140_HWMON_FROM_TEMP(v) DIV_ROUND_CLOSEST_ULL(413350L - (v), 491) + +/* According to the datasheet the formula is: + * U = bits[11:0] / 1024 * 220 / 0.2 + * + * Normalized: + * U = bits[11:0] / 4096 * 2514 + */ +#define BCM54140_HWMON_TO_IN_1V0(v) ((v) * 2514 >> 11) +#define BCM54140_HWMON_FROM_IN_1V0(v) DIV_ROUND_CLOSEST_ULL(((v) << 11), 2514) + +/* According to the datasheet the formula is: + * U = bits[10:0] / 1024 * 880 / 0.7 + * + * Normalized: + * U = bits[10:0] / 2048 * 4400 + */ +#define BCM54140_HWMON_TO_IN_3V3(v) ((v) * 4400 >> 12) +#define BCM54140_HWMON_FROM_IN_3V3(v) DIV_ROUND_CLOSEST_ULL(((v) << 12), 4400) #define BCM54140_DEFAULT_DOWNSHIFT 5 #define BCM54140_MAX_DOWNSHIFT 9 @@ -57,6 +106,258 @@ struct bcm54140_phy_priv { int port; int base_addr; + bool pkg_init; + u16 alarm; +}; + +static umode_t bcm54140_hwmon_is_visible(const void *data, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_in: + switch (attr) { + case hwmon_in_min: + case hwmon_in_max: + return 0644; + case hwmon_in_label: + case hwmon_in_input: + case hwmon_in_alarm: + return 0444; + default: + return 0; + } + case hwmon_temp: + switch (attr) { + case hwmon_temp_min: + case hwmon_temp_max: + return 0644; + case hwmon_temp_input: + case hwmon_temp_alarm: + return 0444; + default: + return 0; + } + default: + return 0; + } +} + +static int bcm54140_hwmon_read_alarm(struct device *dev, unsigned int bit, + long *val) +{ + struct phy_device *phydev = dev_get_drvdata(dev); + struct bcm54140_phy_priv *priv = phydev->priv; + u16 tmp; + + /* latch any alarm bits */ + tmp = bcm_phy_read_rdb(phydev, BCM54140_RDB_MON_ISR); + if (tmp < 0) + return tmp; + priv->alarm |= tmp; + + *val = !!(priv->alarm & bit); + priv->alarm &= ~bit; + + return 0; +} + +static int bcm54140_hwmon_read_temp(struct device *dev, u32 attr, + int channel, long *val) +{ + struct phy_device *phydev = dev_get_drvdata(dev); + u16 reg, tmp; + + switch (attr) { + case hwmon_temp_input: + reg = BCM54140_RDB_MON_TEMP_VAL; + break; + case hwmon_temp_min: + reg = BCM54140_RDB_MON_TEMP_MIN; + break; + case hwmon_temp_max: + reg = BCM54140_RDB_MON_TEMP_MAX; + break; + case hwmon_temp_alarm: + return bcm54140_hwmon_read_alarm(dev, + BCM54140_RDB_MON_ISR_TEMP, + val); + default: + return -EOPNOTSUPP; + } + + tmp = bcm_phy_read_rdb(phydev, reg); + if (tmp < 0) + return tmp; + + *val = BCM54140_HWMON_TO_TEMP(tmp & BCM54140_RDB_MON_TEMP_DATA_MASK); + + return 0; +} + +static int bcm54140_hwmon_read_in(struct device *dev, u32 attr, + int channel, long *val) +{ + struct phy_device *phydev = dev_get_drvdata(dev); + u16 mask = (!channel) ? BCM54140_RDB_MON_1V0_DATA_MASK + : BCM54140_RDB_MON_3V3_DATA_MASK; + u16 bit, reg, tmp; + + switch (attr) { + case hwmon_in_input: + reg = (!channel) ? BCM54140_RDB_MON_1V0_VAL + : BCM54140_RDB_MON_3V3_VAL; + break; + case hwmon_in_min: + reg = (!channel) ? BCM54140_RDB_MON_1V0_MIN + : BCM54140_RDB_MON_3V3_MIN; + break; + case hwmon_in_max: + reg = (!channel) ? BCM54140_RDB_MON_1V0_MAX + : BCM54140_RDB_MON_3V3_MAX; + break; + case hwmon_in_alarm: + bit = (!channel) ? BCM54140_RDB_MON_ISR_1V0 + : BCM54140_RDB_MON_ISR_3V3; + return bcm54140_hwmon_read_alarm(dev, bit, val); + default: + return -EOPNOTSUPP; + } + + tmp = bcm_phy_read_rdb(phydev, reg); + if (tmp < 0) + return tmp; + + if (!channel) + *val = BCM54140_HWMON_TO_IN_1V0(tmp & mask); + else + *val = BCM54140_HWMON_TO_IN_3V3(tmp & mask); + + return 0; +} + +static int bcm54140_hwmon_read(struct device *dev, + enum hwmon_sensor_types type, u32 attr, + int channel, long *val) +{ + switch (type) { + case hwmon_temp: + return bcm54140_hwmon_read_temp(dev, attr, channel, val); + case hwmon_in: + return bcm54140_hwmon_read_in(dev, attr, channel, val); + default: + return -EOPNOTSUPP; + } +} + +static const char *const bcm54140_hwmon_in_labels[] = { + "AVDDL", + "AVDDH", +}; + +static int bcm54140_hwmon_read_string(struct device *dev, + enum hwmon_sensor_types type, u32 attr, + int channel, const char **str) +{ + switch (type) { + case hwmon_in: + switch (attr) { + case hwmon_in_label: + *str = bcm54140_hwmon_in_labels[channel]; + return 0; + default: + return -EOPNOTSUPP; + } + default: + return -EOPNOTSUPP; + } +} + +static int bcm54140_hwmon_write_temp(struct device *dev, u32 attr, + int channel, long val) +{ + struct phy_device *phydev = dev_get_drvdata(dev); + u16 reg; + + switch (attr) { + case hwmon_temp_min: + reg = BCM54140_RDB_MON_TEMP_MIN; + break; + case hwmon_temp_max: + reg = BCM54140_RDB_MON_TEMP_MAX; + break; + default: + return -EOPNOTSUPP; + } + + return bcm_phy_modify_rdb(phydev, reg, BCM54140_RDB_MON_TEMP_DATA_MASK, + BCM54140_HWMON_FROM_TEMP(val)); +} + +static int bcm54140_hwmon_write_in(struct device *dev, u32 attr, + int channel, long val) +{ + struct phy_device *phydev = dev_get_drvdata(dev); + u16 mask = (!channel) ? BCM54140_RDB_MON_1V0_DATA_MASK + : BCM54140_RDB_MON_3V3_DATA_MASK; + unsigned long raw = (!channel) ? BCM54140_HWMON_FROM_IN_1V0(val) + : BCM54140_HWMON_FROM_IN_3V3(val); + u16 reg; + + raw = clamp_val(raw, 0, mask); + + switch (attr) { + case hwmon_in_min: + reg = (!channel) ? BCM54140_RDB_MON_1V0_MIN + : BCM54140_RDB_MON_3V3_MIN; + break; + case hwmon_in_max: + reg = (!channel) ? BCM54140_RDB_MON_1V0_MAX + : BCM54140_RDB_MON_3V3_MAX; + break; + default: + return -EOPNOTSUPP; + } + + return bcm_phy_modify_rdb(phydev, reg, mask, raw); +} + +static int bcm54140_hwmon_write(struct device *dev, + enum hwmon_sensor_types type, u32 attr, + int channel, long val) +{ + switch (type) { + case hwmon_temp: + return bcm54140_hwmon_write_temp(dev, attr, channel, val); + case hwmon_in: + return bcm54140_hwmon_write_in(dev, attr, channel, val); + default: + return -EOPNOTSUPP; + } +} + +static const struct hwmon_channel_info *bcm54140_hwmon_info[] = { + HWMON_CHANNEL_INFO(temp, + HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | + HWMON_T_ALARM), + HWMON_CHANNEL_INFO(in, + HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | + HWMON_I_ALARM | HWMON_I_LABEL, + HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | + HWMON_I_ALARM | HWMON_I_LABEL), + NULL +}; + +static const struct hwmon_ops bcm54140_hwmon_ops = { + .is_visible = bcm54140_hwmon_is_visible, + .read = bcm54140_hwmon_read, + .read_string = bcm54140_hwmon_read_string, + .write = bcm54140_hwmon_write, +}; + +static const struct hwmon_chip_info bcm54140_chip_info = { + .ops = &bcm54140_hwmon_ops, + .info = bcm54140_hwmon_info, }; static int bcm54140_phy_base_read_rdb(struct phy_device *phydev, u16 rdb) @@ -203,6 +504,74 @@ static int bcm54140_get_base_addr_and_port(struct phy_device *phydev) return 0; } +/* Check if one PHY has already done the init of the parts common to all PHYs + * in the Quad PHY package. + */ +static bool bcm54140_is_pkg_init(struct phy_device *phydev) +{ + struct mdio_device **map = phydev->mdio.bus->mdio_map; + struct bcm54140_phy_priv *priv; + struct phy_device *phy; + int i, addr; + + /* Quad PHY */ + for (i = 0; i < 4; i++) { + priv = phydev->priv; + addr = priv->base_addr + i; + + if (!map[addr]) + continue; + + phy = container_of(map[addr], struct phy_device, mdio); + + if ((phy->phy_id & phydev->drv->phy_id_mask) != + (phydev->drv->phy_id & phydev->drv->phy_id_mask)) + continue; + + priv = phy->priv; + + if (priv && priv->pkg_init) + return true; + } + + return false; +} + +static int bcm54140_enable_monitoring(struct phy_device *phydev) +{ + u16 mask, set; + + /* 3.3V voltage mode */ + set = BCM54140_RDB_MON_CTRL_V_MODE; + + /* select round-robin */ + mask = BCM54140_RDB_MON_CTRL_SEL_MASK; + set |= FIELD_PREP(BCM54140_RDB_MON_CTRL_SEL_MASK, + BCM54140_RDB_MON_CTRL_SEL_RR); + + /* remove power-down bit */ + mask |= BCM54140_RDB_MON_CTRL_PWR_DOWN; + + return bcm_phy_modify_rdb(phydev, BCM54140_RDB_MON_CTRL, mask, set); +} + +static int bcm54140_phy_probe_once(struct phy_device *phydev) +{ + struct device *hwmon; + int ret; + + /* enable hardware monitoring */ + ret = bcm54140_enable_monitoring(phydev); + if (ret) + return ret; + + hwmon = devm_hwmon_device_register_with_info(&phydev->mdio.dev, + "BCM54140", phydev, + &bcm54140_chip_info, + NULL); + return PTR_ERR_OR_ZERO(hwmon); +} + static int bcm54140_phy_probe(struct phy_device *phydev) { struct bcm54140_phy_priv *priv; @@ -218,6 +587,14 @@ static int bcm54140_phy_probe(struct phy_device *phydev) if (ret) return ret; + if (!bcm54140_is_pkg_init(phydev)) { + ret = bcm54140_phy_probe_once(phydev); + if (ret) + return ret; + } + + priv->pkg_init = true; + dev_info(&phydev->mdio.dev, "probed (port %d, base PHY address %d)\n", priv->port, priv->base_addr);
The PHY supports monitoring its die temperature as well as two analog voltages. Add support for it. Signed-off-by: Michael Walle <michael@walle.cc> --- Documentation/hwmon/bcm54140.rst | 45 ++++ Documentation/hwmon/index.rst | 1 + drivers/net/phy/bcm54140.c | 377 +++++++++++++++++++++++++++++++ 3 files changed, 423 insertions(+) create mode 100644 Documentation/hwmon/bcm54140.rst