[net-next,3/3] net: phy: bcm54140: add hwmon support

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

> +/* 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.